loleaflet/dist/errormessages.js | 5 ++-- loleaflet/main.js | 2 - loleaflet/src/core/Socket.js | 21 ++++++++++++++++-- loleaflet/src/map/handler/Map.WOPI.js | 6 +++-- wsd/DocumentBroker.cpp | 5 +++- wsd/Exceptions.hpp | 16 +++++++------- wsd/LOOLWSD.cpp | 10 ++++++-- wsd/Storage.cpp | 38 +++++++++++++++++++++++----------- 8 files changed, 71 insertions(+), 32 deletions(-)
New commits: commit 0c609596fbab743de7a8f023c0071d870404c193 Author: Pranav Kant <[email protected]> Date: Fri May 19 11:43:43 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. Combination of following cherry-picks from master: (cherry picked from commit 2e9e6a753d9721dc6776362d34350380f797dab0) (cherry picked from commit bcf958c50056637d4899fc9cd38b87a15dfa57b0) (cherry picked from commit aed840ea04ce2e7ae120698f50e78268b542db12) (cherry picked from commit 1ea87b627e077590c6eefa455ce3c8f7d0564068) Change-Id: I7e2455d3c4da8be2c476460b94812174c34fe529 Reviewed-on: https://gerrit.libreoffice.org/37903 Reviewed-by: Jan Holesovsky <[email protected]> Tested-by: Jan Holesovsky <[email protected]> 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 0e8374d4..25f5f83f 100644 --- a/loleaflet/src/core/Socket.js +++ b/loleaflet/src/core/Socket.js @@ -313,13 +313,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 b13c75f7..5b15fff1 100644 --- a/wsd/DocumentBroker.cpp +++ b/wsd/DocumentBroker.cpp @@ -431,7 +431,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) diff --git a/wsd/Exceptions.hpp b/wsd/Exceptions.hpp index 11c9de59..f0bf82b7 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 @@ -58,14 +66,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 diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp index 5f9fd929..8ec8423b 100644 --- a/wsd/LOOLWSD.cpp +++ b/wsd/LOOLWSD.cpp @@ -2116,12 +2116,16 @@ private: const std::string msg = "error: cmd=internal kind=unauthorized"; clientSession->sendMessage(msg); } - catch (const std::exception& exc) + catch (const StorageConnectionException& exc) { - LOG_ERR("Error while handling loading : " << exc.what()); - const std::string msg = "error: cmd=internal kind=unauthorized"; + // Alert user about failed load + const std::string msg = "error: cmd=storage kind=loadfailed"; clientSession->sendMessage(msg); } + catch (const std::exception& exc) + { + LOG_ERR("Error while loading : " << exc.what()); + } }); }); } diff --git a/wsd/Storage.cpp b/wsd/Storage.cpp index 3c800535..5b3107c9 100644 --- a/wsd/Storage.cpp +++ b/wsd/Storage.cpp @@ -441,6 +441,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) @@ -560,15 +566,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) { @@ -577,9 +593,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) _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
