[Libreoffice-commits] online.git: loolwsd/Exceptions.hpp loolwsd/LOOLWSD.cpp loolwsd/UserMessages.hpp
Ashod Nakashian
ashod.nakashian at collabora.co.uk
Sat Apr 16 17:57:13 UTC 2016
loolwsd/Exceptions.hpp | 52 ++++++++++++++
loolwsd/LOOLWSD.cpp | 167 ++++++++++++++++++++++-------------------------
loolwsd/UserMessages.hpp | 19 +++++
3 files changed, 152 insertions(+), 86 deletions(-)
New commits:
commit 33854ec4aca95d92bb6e0c5eb21cd2b22dde7c10
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date: Sat Apr 16 12:56:23 2016 -0400
loolwsd: cleanup and improve WSD request handlers
Standardized error handling in request-handlers.
There is a new family of internal exeptions designed
to signify the type of error and how to handle it.
All handlers must throw one of those errors
and they will be translated to the correct HTTP
response when caught.
Since some requests send a response as part of their
handling (convert-to, for example) those handlers
must return a flag signlaning whether or not they
sent a response. If not, HTTP OK response is sent
at the end of the handler.
To complicate things, some requests upgrade the
connection to WebSocket. In those cases errors
must be sent via the WebSocket and not as an
HTTP response. The error message sent can (and
in most cases should) be displayed to the end-user.
A new file, UserMessages.hpp, has been added to
hold user-visible messages that can be
reviewed and translated.
Change-Id: Icc725f3313446d4514cf6d092635158ee7171f5d
Reviewed-on: https://gerrit.libreoffice.org/24133
Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
Tested-by: Ashod Nakashian <ashnakash at gmail.com>
diff --git a/loolwsd/Exceptions.hpp b/loolwsd/Exceptions.hpp
new file mode 100644
index 0000000..0384bde
--- /dev/null
+++ b/loolwsd/Exceptions.hpp
@@ -0,0 +1,52 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+// Exception classes to differentiate between the
+// different error situations and handling.
+#ifndef INCLUDED_EXCEPTIONS_HPP
+#define INCLUDED_EXCEPTIONS_HPP
+
+#include <exception>
+
+// Generic LOOL errors and base for others.
+class LoolException : public std::runtime_error
+{
+protected:
+ using std::runtime_error::runtime_error;
+};
+
+/// A bad-request exception that is means to signify,
+/// and translate into, an HTTP bad request.
+class BadRequestException : public LoolException
+{
+public:
+ using LoolException::LoolException;
+};
+
+/// An authorization exception that is means to signify,
+/// and translate into, an HTTP unauthorized error.
+class UnauthorizedRequestException : public LoolException
+{
+public:
+ using LoolException::LoolException;
+};
+
+/// An generic error-message exception meant to
+/// propagate via a valid WebSocket to the client.
+/// The contents of what() will be displayed on screen.
+class WebSocketErrorMessageException : public LoolException
+{
+public:
+ using LoolException::LoolException;
+};
+
+
+#endif
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 802850b..43c23a6 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -82,14 +82,16 @@
#include "Admin.hpp"
#include "Auth.hpp"
#include "Common.hpp"
+#include "Exceptions.hpp"
#include "FileServer.hpp"
+#include "IoUtil.hpp"
#include "LOOLProtocol.hpp"
#include "LOOLSession.hpp"
#include "LOOLWSD.hpp"
#include "MasterProcessSession.hpp"
#include "QueueHandler.hpp"
#include "Storage.hpp"
-#include "IoUtil.hpp"
+#include "UserMessages.hpp"
#include "Util.hpp"
#include "Unit.hpp"
#include "UnitHTTP.hpp"
@@ -299,7 +301,10 @@ private:
return isFound;
}
- static void handlePostRequest(HTTPServerRequest& request, HTTPServerResponse& response, const std::string& id)
+ /// Handle POST requests.
+ /// Always throw on error, do not set response status here.
+ /// Returns true if a response has been sent.
+ static bool handlePostRequest(HTTPServerRequest& request, HTTPServerResponse& response, const std::string& id)
{
Log::info("Post request: [" + request.getURI() + "]");
StringTokenizer tokens(request.getURI(), "/?");
@@ -322,10 +327,7 @@ private:
if (!child)
{
// Let the client know we can't serve now.
- response.setStatusAndReason(Poco::Net::HTTPResponse::HTTP_SERVICE_UNAVAILABLE);
- response.setContentLength(0);
- response.send();
- return;
+ throw std::runtime_error("Failed to spawn lokit child.");
}
auto uriPublic = DocumentBroker::sanitizeURI(fromPath);
@@ -336,7 +338,7 @@ private:
// In that case, we can use a pool and index by publicPath.
std::unique_lock<std::mutex> lock(docBrokersMutex);
- //FIXME: What if the same document is already open? Need a fake dockey here.
+ //FIXME: What if the same document is already open? Need a fake dockey here?
Log::debug("New DocumentBroker for docKey [" + docKey + "].");
docBrokers.emplace(docKey, docBroker);
@@ -350,12 +352,8 @@ private:
if (!waitBridgeCompleted(session, docBroker))
{
- Log::error(session->getName() + ": Failed to connect to lokit child.");
// Let the client know we can't serve now.
- response.setStatusAndReason(Poco::Net::HTTPResponse::HTTP_SERVICE_UNAVAILABLE);
- response.setContentLength(0);
- response.send();
- return;
+ throw std::runtime_error("Failed to connect to lokit child.");
}
// Now the bridge between the client and kit processes is connected
// Let messages flow
@@ -402,10 +400,11 @@ private:
if (!sent)
{
- response.setStatus(HTTPResponse::HTTP_BAD_REQUEST);
- response.setContentLength(0);
- response.send();
+ //TODO: We should differentiate between bad request and failed conversion.
+ throw BadRequestException("Failed to convert and send file.");
}
+
+ return true;
}
else if (tokens.count() >= 2 && tokens[1] == "insertfile")
{
@@ -418,17 +417,13 @@ private:
ConvertToPartHandler handler(tmpPath);
HTMLForm form(request, request.stream(), handler);
- bool goodRequest = form.has("childid") && form.has("name");
- std::string formChildid(form.get("childid"));
- std::string formName(form.get("name"));
-
- // protect against attempts to inject something funny here
- if (goodRequest && formChildid.find('/') != std::string::npos && formName.find('/') != std::string::npos)
- goodRequest = false;
-
- if (goodRequest)
+ if (form.has("childid") && form.has("name"))
{
- try
+ const std::string formChildid(form.get("childid"));
+ const std::string formName(form.get("name"));
+
+ // protect against attempts to inject something funny here
+ if (formChildid.find('/') == std::string::npos && formName.find('/') == std::string::npos)
{
Log::info() << "Perform insertfile: " << formChildid << ", " << formName << Log::end;
const std::string dirPath = LOOLWSD::ChildRoot + formChildid
@@ -436,27 +431,15 @@ private:
File(dirPath).createDirectories();
std::string fileName = dirPath + Path::separator() + form.get("name");
File(tmpPath).moveTo(fileName);
-
- response.setStatus(HTTPResponse::HTTP_OK);
- response.send();
- }
- catch (const IOException& exc)
- {
- Log::info() << "ClientRequestHandler::handlePostRequest: IOException: " << exc.message() << Log::end;
- response.setStatus(HTTPResponse::HTTP_BAD_REQUEST);
- response.send();
+ return false;
}
}
- else
- {
- response.setStatus(HTTPResponse::HTTP_BAD_REQUEST);
- response.send();
- }
}
else if (tokens.count() >= 4)
{
Log::info("File download request.");
// The user might request a file to download
+ //TODO: Check that the user in question has access to this file!
const std::string dirPath = LOOLWSD::ChildRoot + tokens[1]
+ JAILED_DOCUMENT_ROOT + tokens[2];
std::string fileName;
@@ -468,30 +451,21 @@ private:
{
response.set("Access-Control-Allow-Origin", "*");
HTMLForm form(request);
- std::string mimeType = "application/octet-stream";
- if (form.has("mime_type"))
- mimeType = form.get("mime_type");
+ const std::string mimeType = form.has("mime_type")
+ ? form.get("mime_type")
+ : "application/octet-stream";
response.sendFile(filePath, mimeType);
+ //TODO: Cleanup on error.
Util::removeFile(dirPath, true);
+ return true;
}
- else
- {
- response.setStatus(HTTPResponse::HTTP_NOT_FOUND);
- response.setContentLength(0);
- response.send();
- Log::info("file not found.");
- }
- }
- else
- {
- Log::info("Bad request.");
- response.setStatus(HTTPResponse::HTTP_BAD_REQUEST);
- response.setContentLength(0);
- response.send();
}
+
+ throw BadRequestException("Invalid or unknown request.");
}
- static void handleGetRequest(HTTPServerRequest& request, HTTPServerResponse& response, const std::string& id)
+ /// Handle GET requests.
+ static void handleGetRequest(HTTPServerRequest& request, std::shared_ptr<WebSocket>& ws, const std::string& id)
{
Log::info("Starting GET request handler for session [" + id + "].");
@@ -502,20 +476,6 @@ private:
uri.erase(0, 1);
}
- // accept websocket connection with client
- std::shared_ptr<WebSocket> ws;
- try
- {
- ws = std::make_shared<WebSocket>(request, response);
- }
- catch (WebSocketException& exc)
- {
- response.setStatusAndReason(HTTPResponse::HTTP_BAD_REQUEST);
- response.setContentLength(0);
- response.send();
- throw;
- }
-
// indicator to the client that document broker is searching
std::string status("statusindicator: find");
ws->sendFrame(status.data(), (int) status.size());
@@ -576,11 +536,10 @@ private:
if (!waitBridgeCompleted(session, docBroker))
{
// Let the client know we can't serve now.
- status = "statusindicator: fail";
- ws->sendFrame(status.data(), (int) status.size());
- ws->shutdown();
- throw WebSocketException("Failed to connect to lokit process", WebSocket::WS_ENDPOINT_GOING_AWAY);
+ Log::error(session->getName() + ": Failed to connect to lokit process. Client cannot serve now.");
+ throw WebSocketErrorMessageException(SERVICE_UNAVALABLE_INTERNAL_ERROR);
}
+
// Now the bridge beetween the client and kit process is connected
// Let messages flow
status = "statusindicator: ready";
@@ -635,7 +594,10 @@ private:
}
}
- static void handleGetDiscovery(HTTPServerRequest& request, HTTPServerResponse& response)
+ /// Sends back the WOPI Discovery XML.
+ /// The XML needs to be preprocessed to stamp the correct URL etc.
+ /// Returns true if a response has been sent.
+ static bool handleGetWOPIDiscovery(HTTPServerRequest& request, HTTPServerResponse& response)
{
std::string discoveryPath = Path(Application::instance().commandPath()).parent().toString() + "discovery.xml";
if (!File(discoveryPath).exists())
@@ -672,6 +634,7 @@ private:
std::ostream& ostr = response.send();
ostr << ostrXML.str();
Log::debug("Sent discovery.xml successfully.");
+ return true;
}
public:
@@ -694,35 +657,70 @@ public:
Log::debug("Thread started.");
+ bool responded = false;
try
{
if (request.getMethod() == HTTPRequest::HTTP_GET && request.getURI() == "/hosting/discovery")
{
// http://server/hosting/discovery
- handleGetDiscovery(request, response);
+ responded = handleGetWOPIDiscovery(request, response);
}
else if (!(request.find("Upgrade") != request.end() && Poco::icompare(request["Upgrade"], "websocket") == 0))
{
- handlePostRequest(request, response, id);
+ responded = handlePostRequest(request, response, id);
}
else
{
- handleGetRequest(request, response, id);
+ auto ws = std::make_shared<WebSocket>(request, response);
+ try
+ {
+ responded = true; // After upgrading to WS we should not set HTTP response.
+ handleGetRequest(request, ws, id);
+ }
+ catch (const WebSocketErrorMessageException& exc)
+ {
+ Log::error(std::string("ClientRequestHandler::handleRequest: WebSocketErrorMessageException: ") + exc.what());
+ try
+ {
+ const std::string msg = std::string("error: ") + exc.what();
+ ws->sendFrame(msg.data(), msg.size());
+ ws->shutdown();
+ }
+ catch (const std::exception& exc2)
+ {
+ Log::error(std::string("ClientRequestHandler::handleRequest: exception while sending WS error message: ") + exc2.what());
+ }
+ }
}
}
catch (const Exception& exc)
{
- Log::error() << "ClientRequestHandler::handleRequest: Exception: " << exc.displayText()
+ Log::error() << "ClientRequestHandler::handleRequest: PocoException: " << exc.displayText()
<< (exc.nested() ? " (" + exc.nested()->displayText() + ")" : "")
<< Log::end;
+ response.setStatusAndReason(HTTPResponse::HTTP_SERVICE_UNAVAILABLE);
+ }
+ catch (const UnauthorizedRequestException& exc)
+ {
+ Log::error(std::string("ClientRequestHandler::handleRequest: UnauthorizedException: ") + exc.what());
+ response.setStatusAndReason(HTTPResponse::HTTP_UNAUTHORIZED);
+ }
+ catch (const BadRequestException& exc)
+ {
+ Log::error(std::string("ClientRequestHandler::handleRequest: BadRequestException: ") + exc.what());
+ response.setStatusAndReason(HTTPResponse::HTTP_BAD_REQUEST);
+
}
catch (const std::exception& exc)
{
Log::error(std::string("ClientRequestHandler::handleRequest: Exception: ") + exc.what());
+ response.setStatusAndReason(HTTPResponse::HTTP_SERVICE_UNAVAILABLE);
}
- catch (...)
+
+ if (!responded)
{
- Log::error("ClientRequestHandler::handleRequest: Unexpected exception");
+ response.setContentLength(0);
+ response.send();
}
Log::debug("Thread finished.");
@@ -885,6 +883,7 @@ public:
{
assert(false);
}
+
Log::info("Adding doc " + docKey + " to Admin");
Admin::instance().addDoc(docKey, pid, docBroker->getFilename(), Util::decodeId(sessionId));
@@ -914,10 +913,6 @@ public:
{
Log::error(std::string("PrisonerRequestHandler::handleRequest: Exception: ") + exc.what());
}
- catch (...)
- {
- Log::error("PrisonerRequestHandler::handleRequest: Unexpected exception");
- }
if (!jailId.empty())
{
diff --git a/loolwsd/UserMessages.hpp b/loolwsd/UserMessages.hpp
new file mode 100644
index 0000000..93e3e57
--- /dev/null
+++ b/loolwsd/UserMessages.hpp
@@ -0,0 +1,19 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+// A list of user-visible messages.
+// This list is intended to be centralized for review and i18n support.
+#ifndef INCLUDED_USERMESSAGES_HPP
+#define INCLUDED_USERMESSAGES_HPP
+
+constexpr auto SERVICE_UNAVALABLE_INTERNAL_ERROR = "Service is unavailable. Please try again later and report to your administrator if the issue persists.";
+
+#endif
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
More information about the Libreoffice-commits
mailing list