loleaflet/dist/errormessages.js | 5 ++-- loleaflet/main.js | 2 - loleaflet/src/core/Socket.js | 23 +++++++++++++++++--- loleaflet/src/map/handler/Map.WOPI.js | 6 +++-- wsd/DocumentBroker.cpp | 11 ++++++++- wsd/Exceptions.hpp | 16 +++++++------- wsd/LOOLWSD.cpp | 14 +++++++++--- wsd/Storage.cpp | 38 +++++++++++++++++++++++----------- 8 files changed, 82 insertions(+), 33 deletions(-)
New commits: commit bcf958c50056637d4899fc9cd38b87a15dfa57b0 Author: Pranav Kant <[email protected]> Date: Thu May 18 23:35:45 2017 +0530 wsd: Fail gracefully when storage misbehaves When WOPI's CheckFileInfo or GetFile responds with status code other than HTTP 200, show a message to the user indicating some problem in the storage. Currently, we open an empty document if storage doesn't return a document which surely is not correct. Mention the storage server address when asking user to contact the server administrator to be more friendly. Change-Id: I15f0489f36db8689b43d42f6b691fdd21815e4fa diff --git a/loleaflet/dist/errormessages.js b/loleaflet/dist/errormessages.js index d1eccac1..89f31fc7 100644 --- a/loleaflet/dist/errormessages.js +++ b/loleaflet/dist/errormessages.js @@ -10,6 +10,7 @@ exports.sessionexpired = _('Your session has been expired. Further changes to do exports.faileddocloading = _('Failed to load the document. Please ensure the file type is supported and not corrupted, and try again.'); exports.storage = { - savediskfull: _('Save failed due to no disk space left on storage server. Document will now be read-only. Please contact the server administrator to continue editing.'), - savefailed: _('Document cannot be saved to storage. Check your permissions or contact the storage server administrator.') + loadfailed: _('Failed to read document from storage. Please contact your storage server (%storageserver) administrator.'), + savediskfull: _('Save failed due to no disk space left on storage server. Document will now be read-only. Please contact the server (%storageserver) administrator to continue editing.'), + savefailed: _('Document cannot be saved to storage. Check your permissions or contact the storage server (%storageserver) administrator.') }; diff --git a/loleaflet/main.js b/loleaflet/main.js index fed59570..48200cd9 100644 --- a/loleaflet/main.js +++ b/loleaflet/main.js @@ -113,7 +113,7 @@ var map = L.map('map', { wopi: isWopi, alwaysActive: alwaysActive, idleTimeoutSecs: idleTimeoutSecs, // Dim when user is idle. - outOfFocusTimeoutSecs: outOfFocusTimeoutSecs, // Dim after switching tabs. + outOfFocusTimeoutSecs: outOfFocusTimeoutSecs // Dim after switching tabs. }); // toolbar.js (loaded in <script> tag accesses map as global variable, // so expose it diff --git a/loleaflet/src/core/Socket.js b/loleaflet/src/core/Socket.js index 03643901..1047c4b7 100644 --- a/loleaflet/src/core/Socket.js +++ b/loleaflet/src/core/Socket.js @@ -312,13 +312,28 @@ L.Socket = L.Class.extend({ return; } else if (textMsg.startsWith('error:') && command.errorCmd === 'storage') { + var storageError; if (command.errorKind === 'savediskfull') { - this._map.fire('error', {msg: errorMessages.storage.savediskfull}); + storageError = errorMessages.storage.savediskfull; } else if (command.errorKind === 'savefailed') { - // Just warn the user - this._map.fire('warn', {msg: errorMessages.storage.savefailed}); - } + storageError = errorMessages.storage.savefailed; + } + else if (command.errorKind === 'loadfailed') { + storageError = errorMessages.storage.loadfailed; + // Since this is a document load failure, wsd will disconnect the socket anyway, + // better we do it first so that another error message doesn't override this one + // upon socket close. + this._map.hideBusy(); + this.close(); + } + + // Parse the storage url as link + var tmpLink = document.createElement('a'); + tmpLink.href = this._map.options.doc; + // Insert the storage server address to be more friendly + storageError = storageError.replace('%storageserver', tmpLink.host); + this._map.fire('warn', {msg: storageError}); return; } diff --git a/loleaflet/src/map/handler/Map.WOPI.js b/loleaflet/src/map/handler/Map.WOPI.js index 110c0adb..20d11a6f 100644 --- a/loleaflet/src/map/handler/Map.WOPI.js +++ b/loleaflet/src/map/handler/Map.WOPI.js @@ -4,8 +4,10 @@ /* global title */ L.Map.WOPI = L.Handler.extend({ - - PostMessageOrigin: false, + // If the CheckFileInfo call fails on server side, we won't have any PostMessageOrigin. + // So use '*' because we still needs to send 'close' message to the parent frame which + // wouldn't be possible otherwise. + PostMessageOrigin: '*', DocumentLoadedTime: false, HidePrintOption: false, HideSaveOption: false, diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp index e5924f87..7f8b9bce 100644 --- a/wsd/DocumentBroker.cpp +++ b/wsd/DocumentBroker.cpp @@ -452,7 +452,10 @@ bool DocumentBroker::load(const std::shared_ptr<ClientSession>& session, const s std::ostringstream ossWopiInfo; wopiInfo->stringify(ossWopiInfo); - session->sendTextFrame("wopi: " + ossWopiInfo.str()); + // Contains PostMessageOrigin property which is necessary to post messages to parent + // frame. Important to send this message immediately and not enqueue it so that in case + // document load fails, loleaflet is able to tell its parent frame via PostMessage API. + session->sendMessage("wopi: " + ossWopiInfo.str()); // Mark the session as 'Document owner' if WOPI hosts supports it if (userid == _storage->getFileInfo()._ownerId) @@ -813,6 +816,12 @@ size_t DocumentBroker::addSession(const std::shared_ptr<ClientSession>& session) throw std::runtime_error(msg); } } + catch (const StorageConnectionException& exc) + { + // Alert user about failed load + session->sendMessage("error: cmd=storage kind=loadfailed"); + throw; + } catch (const StorageSpaceLowException&) { LOG_ERR("Out of storage while loading document with URI [" << session->getPublicUri().toString() << "]."); diff --git a/wsd/Exceptions.hpp b/wsd/Exceptions.hpp index a6fbe848..3a3d242f 100644 --- a/wsd/Exceptions.hpp +++ b/wsd/Exceptions.hpp @@ -34,6 +34,14 @@ public: using LoolException::LoolException; }; +/// General exception thrown when we are not able to +/// connect to storage. +class StorageConnectionException : public LoolException +{ +public: + using LoolException::LoolException; +}; + /// A bad-request exception that is meant to signify, /// and translate into, an HTTP bad request. class BadRequestException : public LoolException diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp index 1c6c91d6..84d332a7 100644 --- a/wsd/LOOLWSD.cpp +++ b/wsd/LOOLWSD.cpp @@ -1883,7 +1883,7 @@ private: docBroker->addCallback([docBroker, moveSocket, clientSession, format]() { - auto streamSocket = std::static_pointer_cast<StreamSocket>(moveSocket); + auto streamSocket = std::static_pointer_cast<StreamSocket>(moveSocket); clientSession->setSaveAsSocket(streamSocket); // Move the socket into DocBroker. @@ -2125,10 +2125,18 @@ private: catch (const std::exception& exc) { LOG_ERR("Error while handling loading : " << exc.what()); - const std::string msg = "error: cmd=internal kind=unauthorized"; - clientSession->sendMessage(msg); + // only send our default error message if we haven't handled the + // exception already up the stack + if (std::uncaught_exception()) + { + // FIXME: Are we sure we want to say that all other failures due + // to an 'unauthorized' WOPI host ? + const std::string msg = "error: cmd=internal kind=unauthorized"; + clientSession->sendMessage(msg); + } docBroker->stop(); } + }); }); } diff --git a/wsd/Storage.cpp b/wsd/Storage.cpp index 12f0a49e..e259836b 100644 --- a/wsd/Storage.cpp +++ b/wsd/Storage.cpp @@ -462,6 +462,12 @@ std::unique_ptr<WopiStorage::WOPIFileInfo> WopiStorage::getWOPIFileInfo(const st LOG_END(logger); } + if (response.getStatus() != Poco::Net::HTTPResponse::HTTP_OK) + { + LOG_ERR("WOPI::CheckFileInfo failed with " + response.getStatus() + response.getReason()); + throw StorageConnectionException("WOPI::CheckFileInfo failed"); + } + Poco::StreamCopier::copyToString(rs, resMsg); } catch(const Poco::Exception& pexc) @@ -582,15 +588,25 @@ std::string WopiStorage::loadStorageFileToLocal(const std::string& accessToken) LOG_END(logger); } - _jailedFilePath = Poco::Path(getLocalRootPath(), _fileInfo._filename).toString(); - std::ofstream ofs(_jailedFilePath); - std::copy(std::istreambuf_iterator<char>(rs), - std::istreambuf_iterator<char>(), - std::ostreambuf_iterator<char>(ofs)); - - LOG_INF("WOPI::GetFile downloaded " << getFileSize(_jailedFilePath) << " bytes from [" << uriObject.toString() << - "] -> " << _jailedFilePath << " in " << diff.count() << "s : " << - response.getStatus() << " " << response.getReason()); + if (response.getStatus() != Poco::Net::HTTPResponse::HTTP_OK) + { + LOG_ERR("WOPI::GetFile failed with " + response.getStatus() + response.getReason()); + throw StorageConnectionException("WOPI::GetFile failed"); + } + else // Successful + { + _jailedFilePath = Poco::Path(getLocalRootPath(), _fileInfo._filename).toString(); + std::ofstream ofs(_jailedFilePath); + std::copy(std::istreambuf_iterator<char>(rs), + std::istreambuf_iterator<char>(), + std::ostreambuf_iterator<char>(ofs)); + LOG_INF("WOPI::GetFile downloaded " << getFileSize(_jailedFilePath) << " bytes from [" << uriObject.toString() << + "] -> " << _jailedFilePath << " in " << diff.count() << "s"); + + _isLoaded = true; + // Now return the jailed path. + return Poco::Path(_jailPath, _fileInfo._filename).toString(); + } } catch(const Poco::Exception& pexc) { @@ -599,9 +615,7 @@ std::string WopiStorage::loadStorageFileToLocal(const std::string& accessToken) throw; } - _isLoaded = true; - // Now return the jailed path. - return Poco::Path(_jailPath, _fileInfo._filename).toString(); + return ""; } StorageBase::SaveResult WopiStorage::saveLocalFileToStorage(const std::string& accessToken) commit 2e9e6a753d9721dc6776362d34350380f797dab0 Author: Pranav Kant <[email protected]> Date: Fri May 19 11:43:43 2017 +0530 Bin unused Exception class Change-Id: I961af47f6c112b6d9bb2e9d4ad184e162377506f diff --git a/wsd/Exceptions.hpp b/wsd/Exceptions.hpp index 2c4905a6..a6fbe848 100644 --- a/wsd/Exceptions.hpp +++ b/wsd/Exceptions.hpp @@ -58,14 +58,6 @@ public: using LoolException::LoolException; }; -/// An access-denied exception that is meant to signify -/// a storage authentication error. -class AccessDeniedException : public LoolException -{ -public: - using LoolException::LoolException; -}; - /// A service-unavailable exception that is meant to signify /// an internal error. class ServiceUnavailableException : public LoolException _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
