[Libreoffice-commits] online.git: loolwsd/ClientSession.cpp loolwsd/ClientSession.hpp loolwsd/DocumentBroker.cpp loolwsd/DocumentBroker.hpp loolwsd/LOOLWSD.cpp loolwsd/PrisonerSession.cpp loolwsd/Storage.cpp loolwsd/Storage.hpp
Pranav Kant
pranavk at collabora.co.uk
Sun Oct 16 22:02:43 UTC 2016
loolwsd/ClientSession.cpp | 4 +-
loolwsd/ClientSession.hpp | 14 ++++++-
loolwsd/DocumentBroker.cpp | 83 +++++++++++++++++++++-----------------------
loolwsd/DocumentBroker.hpp | 6 +--
loolwsd/LOOLWSD.cpp | 15 +------
loolwsd/PrisonerSession.cpp | 2 -
loolwsd/Storage.cpp | 49 +++++++++----------------
loolwsd/Storage.hpp | 23 ++++--------
8 files changed, 88 insertions(+), 108 deletions(-)
New commits:
commit 9ebd7a15e2864973911e03a4c40eb83ed924515e
Author: Pranav Kant <pranavk at collabora.co.uk>
Date: Sun Oct 16 22:10:52 2016 +0530
loolwsd: Improved loolwsd -> storage communication
Reduces the number of WOPI calls made during a document load. Earlier
effort was made in edfd3266f89708afd8cbec0adf88bac63f90cd24
This commit cleans up and uses better approach for the same.
Other than that, access token of each session is now correctly
used when interacting with the storage. Earlier, we used to
use the same access token for each upload to storage which means
that irrespective of who clicked the save button, changes to the
document were only made on behalf of one person (of whom the
access token is used). This is fixed now.
Also includes minor cleanup left and right.
Change-Id: Id32702ff02aea4f63b7cc6afa9f62664807bb57d
Reviewed-on: https://gerrit.libreoffice.org/29931
Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
Tested-by: Ashod Nakashian <ashnakash at gmail.com>
diff --git a/loolwsd/ClientSession.cpp b/loolwsd/ClientSession.cpp
index 32a2c1f..37f7f45 100644
--- a/loolwsd/ClientSession.cpp
+++ b/loolwsd/ClientSession.cpp
@@ -37,9 +37,11 @@ using Poco::StringTokenizer;
ClientSession::ClientSession(const std::string& id,
std::shared_ptr<Poco::Net::WebSocket> ws,
std::shared_ptr<DocumentBroker> docBroker,
- bool readOnly) :
+ const Poco::URI& uriPublic,
+ const bool readOnly) :
LOOLSession(id, Kind::ToClient, ws),
_docBroker(std::move(docBroker)),
+ _uriPublic(uriPublic),
_isReadOnly(readOnly),
_loadPart(-1)
{
diff --git a/loolwsd/ClientSession.hpp b/loolwsd/ClientSession.hpp
index ba8d985..6f76bb1 100644
--- a/loolwsd/ClientSession.hpp
+++ b/loolwsd/ClientSession.hpp
@@ -13,6 +13,8 @@
#include "LOOLSession.hpp"
#include "MessageQueue.hpp"
+#include <Poco/URI.h>
+
class DocumentBroker;
class PrisonerSession;
@@ -23,7 +25,8 @@ public:
ClientSession(const std::string& id,
std::shared_ptr<Poco::Net::WebSocket> ws,
std::shared_ptr<DocumentBroker> docBroker,
- bool isReadOnly = false);
+ const Poco::URI& uriPublic,
+ const bool isReadOnly = false);
virtual ~ClientSession();
@@ -52,6 +55,10 @@ public:
std::shared_ptr<DocumentBroker> getDocumentBroker() const { return _docBroker; }
+ /// Exact URI (including query params - access tokens etc.) with which
+ /// client made the request to us
+ const Poco::URI& getPublicUri() const { return _uriPublic; }
+
private:
virtual bool _handleInput(const char *buffer, int length) override;
@@ -72,7 +79,10 @@ private:
std::shared_ptr<DocumentBroker> _docBroker;
- // Whether the session is opened as readonly
+ /// URI with which client made request to us
+ const Poco::URI _uriPublic;
+
+ /// Whether the session is opened as readonly
bool _isReadOnly;
/// Our peer that connects us to the child.
diff --git a/loolwsd/DocumentBroker.cpp b/loolwsd/DocumentBroker.cpp
index 67710bb..c562048 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -152,42 +152,25 @@ DocumentBroker::DocumentBroker(const Poco::URI& uriPublic,
Log::info("DocumentBroker [" + _uriPublic.toString() + "] created. DocKey: [" + _docKey + "]");
}
-const StorageBase::FileInfo DocumentBroker::validate(const Poco::URI& uri)
+bool DocumentBroker::load(const std::string& sessionId, const std::string& jailId)
{
- const auto uriString = uri.toString();
- Log::info("Validating: " + uriString);
- try
- {
- _storage = StorageBase::create(uri, "", "");
- auto fileinfo = _storage->getFileInfo();
- Log::info("After checkfileinfo: " + uriString + " -> " + fileinfo._filename);
- if (!fileinfo.isValid())
- {
- Log::error("Invalid file info for uri " + uriString);
- throw BadRequestException("Invalid URI or access denied.");
- }
-
- return fileinfo;
- }
- catch (const std::exception& ex)
+ if (_markToDestroy)
{
- Log::error("Exception while getting file info for uri " + uriString + ": " + ex.what());
- throw BadRequestException("Invalid URI or access denied.");
+ // Tearing down.
+ return false;
}
-}
-
-bool DocumentBroker::load(const std::string& jailId)
-{
- Log::debug("Loading from URI: " + _uriPublic.toString());
std::unique_lock<std::mutex> lock(_mutex);
-
- if (_markToDestroy)
+ auto it = _sessions.find(sessionId);
+ if (it == _sessions.end())
{
- // Tearing down.
+ Log::error("Session with sessionId [" + sessionId + "] not found while loading");
return false;
}
+ const Poco::URI& uriPublic = it->second->getPublicUri();
+ Log::debug("Loading from URI: " + uriPublic.toString());
+
_jailId = jailId;
// The URL is the publicly visible one, not visible in the chroot jail.
@@ -204,28 +187,36 @@ bool DocumentBroker::load(const std::string& jailId)
if (_storage == nullptr)
{
- _storage = StorageBase::create(_uriPublic, jailRoot, jailPath.toString());
+ // TODO: Maybe better to pass docKey to storage here instead of uriPublic here because
+ // uriPublic would be different for each view of the document (due to
+ // different query params like access token etc.)
+ Log::debug("Creating new storage instance for URI [" + uriPublic.toString() + "].");
+ _storage = StorageBase::create(uriPublic, jailRoot, jailPath.toString());
}
- else
+
+ if (_storage)
{
+ // Set the username for the session
+ // TODO: security: Set the permission (readonly etc.) of the session here also
+ const auto fileInfo = _storage->getFileInfo(uriPublic);
+ if (!fileInfo.isValid())
+ {
+ Log::error("Invalid fileinfo for URI [" + uriPublic.toString() + "].");
+ return false;
+ }
+ Log::debug("Setting username of the session to: " + fileInfo._userName);
+ it->second->setUserName(fileInfo._userName);
+
+ // Lets load the document now
if (_storage->isLoaded())
{
// Already loaded. Nothing to do.
return true;
}
- _storage->setLocalStorePath(jailRoot);
- _storage->setJailPath(jailPath.toString());
- }
-
-
- if (_storage)
- {
- const auto fileInfo = _storage->getFileInfo();
- _filename = fileInfo._filename;
-
const auto localPath = _storage->loadStorageFileToLocal();
_uriJailed = Poco::URI(Poco::URI("file://"), localPath);
+ _filename = fileInfo._filename;
// Use the local temp file's timestamp.
_lastFileModifiedTime = Poco::File(_storage->getLocalRootPath()).getLastModified();
@@ -237,11 +228,19 @@ bool DocumentBroker::load(const std::string& jailId)
return false;
}
-bool DocumentBroker::save(bool success, const std::string& result)
+bool DocumentBroker::save(const std::string& sessionId, bool success, const std::string& result)
{
std::unique_lock<std::mutex> lock(_saveMutex);
- const auto uri = _uriPublic.toString();
+ const auto it = _sessions.find(sessionId);
+ if (it == _sessions.end())
+ {
+ Log::error("Session with sessionId [" + sessionId + "] not found while saving");
+ return false;
+ }
+
+ const Poco::URI& uriPublic = it->second->getPublicUri();
+ const auto uri = uriPublic.toString();
// If save requested, but core didn't save because document was unmodified
// notify the waiting thread, if any.
@@ -268,7 +267,7 @@ bool DocumentBroker::save(bool success, const std::string& result)
Log::debug("Saving to URI [" + uri + "].");
assert(_storage && _tileCache);
- if (_storage->saveLocalFileToStorage())
+ if (_storage->saveLocalFileToStorage(uriPublic))
{
_isModified = false;
_tileCache->setUnsavedChanges(false);
diff --git a/loolwsd/DocumentBroker.hpp b/loolwsd/DocumentBroker.hpp
index 72b507a..a1aa06d 100644
--- a/loolwsd/DocumentBroker.hpp
+++ b/loolwsd/DocumentBroker.hpp
@@ -152,15 +152,13 @@ public:
<< " sessions left." << Log::end;
}
- const StorageBase::FileInfo validate(const Poco::URI& uri);
-
/// Loads a document from the public URI into the jail.
- bool load(const std::string& jailId);
+ bool load(const std::string& sessionId, const std::string& jailId);
bool isLoaded() const { return _isLoaded; }
void setLoaded() { _isLoaded = true; }
/// Save the document to Storage if needs persisting.
- bool save(bool success, const std::string& result = "");
+ bool save(const std::string& sesionId, bool success, const std::string& result = "");
bool isModified() const { return _isModified; }
void setModified(const bool value);
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 4d85ddd..f9938c0 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -470,7 +470,7 @@ private:
// Load the document.
std::shared_ptr<WebSocket> ws;
- auto session = std::make_shared<ClientSession>(id, ws, docBroker);
+ auto session = std::make_shared<ClientSession>(id, ws, docBroker, uriPublic);
// Request the child to connect to us and add this session.
auto sessionsCount = docBroker->addSession(session);
@@ -782,10 +782,6 @@ private:
throw WebSocketErrorMessageException(SERVICE_UNAVAILABLE_INTERNAL_ERROR);
}
- // Validate the URI and Storage before moving on.
- const auto fileinfo = docBroker->validate(uriPublic);
- Log::debug("Validated [" + uriPublic.toString() + "]");
-
if (newDoc)
{
std::unique_lock<std::mutex> lock(docBrokersMutex);
@@ -806,12 +802,7 @@ private:
std::shared_ptr<ClientSession> session;
try
{
- session = std::make_shared<ClientSession>(id, ws, docBroker, isReadOnly);
- if (!fileinfo._userName.empty())
- {
- Log::debug(uriPublic.toString() + " requested with username [" + fileinfo._userName + "]");
- session->setUserName(fileinfo._userName);
- }
+ session = std::make_shared<ClientSession>(id, ws, docBroker, uriPublic, isReadOnly);
// Request the child to connect to us and add this session.
auto sessionsCount = docBroker->addSession(session);
@@ -1258,7 +1249,7 @@ public:
try
{
- docBroker->load(jailId);
+ docBroker->load(sessionId, jailId);
}
catch (const StorageSpaceLowException&)
{
diff --git a/loolwsd/PrisonerSession.cpp b/loolwsd/PrisonerSession.cpp
index b1b5f91..09783d1 100644
--- a/loolwsd/PrisonerSession.cpp
+++ b/loolwsd/PrisonerSession.cpp
@@ -86,7 +86,7 @@ bool PrisonerSession::_handleInput(const char *buffer, int length)
result = resultObj->get("value").toString();
}
- if (!_docBroker->save(success, result))
+ if (!_docBroker->save(getId(), success, result))
peer->sendTextFrame("error: cmd=internal kind=diskfull");
return true;
}
diff --git a/loolwsd/Storage.cpp b/loolwsd/Storage.cpp
index 3236c28..ca4ca25 100644
--- a/loolwsd/Storage.cpp
+++ b/loolwsd/Storage.cpp
@@ -165,10 +165,10 @@ std::unique_ptr<StorageBase> StorageBase::create(const Poco::URI& uri, const std
unsigned LocalStorage::LastLocalStorageId = 0;
-StorageBase::FileInfo LocalStorage::getFileInfo()
+StorageBase::FileInfo LocalStorage::getFileInfo(const Poco::URI& uriPublic)
{
- const auto path = Poco::Path(_uri.getPath());
- Log::debug("Getting info for local uri [" + _uri.toString() + "], path [" + path.toString() + "].");
+ const auto path = Poco::Path(uriPublic.getPath());
+ Log::debug("Getting info for local uri [" + uriPublic.toString() + "], path [" + path.toString() + "].");
const auto& filename = path.getFileName();
const auto file = Poco::File(path);
const auto lastModified = file.getLastModified();
@@ -222,20 +222,20 @@ std::string LocalStorage::loadStorageFileToLocal()
return Poco::Path(_jailPath, filename).toString();
}
-bool LocalStorage::saveLocalFileToStorage()
+bool LocalStorage::saveLocalFileToStorage(const Poco::URI& uriPublic)
{
try
{
// Copy the file back.
if (_isCopy && Poco::File(_jailedFilePath).exists())
{
- Log::info("Copying " + _jailedFilePath + " to " + _uri.getPath());
- Poco::File(_jailedFilePath).copyTo(_uri.getPath());
+ Log::info("Copying " + _jailedFilePath + " to " + uriPublic.getPath());
+ Poco::File(_jailedFilePath).copyTo(uriPublic.getPath());
}
}
catch (const Poco::Exception& exc)
{
- Log::error("copyTo(\"" + _jailedFilePath + "\", \"" + _uri.getPath() + "\") failed: " + exc.displayText());
+ Log::error("copyTo(\"" + _jailedFilePath + "\", \"" + uriPublic.getPath() + "\") failed: " + exc.displayText());
throw;
}
@@ -253,20 +253,14 @@ Poco::Net::HTTPClientSession* getHTTPClientSession(const Poco::URI& uri)
} // anonymous namespace
-StorageBase::FileInfo WopiStorage::getFileInfo()
+StorageBase::FileInfo WopiStorage::getFileInfo(const Poco::URI& uriPublic)
{
- Log::debug("Getting info for wopi uri [" + _uri.toString() + "].");
-
- if (_fileInfo.isValid())
- {
- Log::debug("Returning cached fileinfo for wopi uri [" + _uri.toString() + "].");
- return _fileInfo;
- }
+ Log::debug("Getting info for wopi uri [" + uriPublic.toString() + "].");
const auto startTime = std::chrono::steady_clock::now();
- std::unique_ptr<Poco::Net::HTTPClientSession> psession(getHTTPClientSession(_uri));
+ std::unique_ptr<Poco::Net::HTTPClientSession> psession(getHTTPClientSession(uriPublic));
- Poco::Net::HTTPRequest request(Poco::Net::HTTPRequest::HTTP_GET, _uri.getPathAndQuery(), Poco::Net::HTTPMessage::HTTP_1_1);
+ Poco::Net::HTTPRequest request(Poco::Net::HTTPRequest::HTTP_GET, uriPublic.getPathAndQuery(), Poco::Net::HTTPMessage::HTTP_1_1);
request.set("User-Agent", "LOOLWSD WOPI Agent");
psession->sendRequest(request);
@@ -274,7 +268,7 @@ StorageBase::FileInfo WopiStorage::getFileInfo()
std::istream& rs = psession->receiveResponse(response);
auto logger = Log::trace();
- logger << "WOPI::CheckFileInfo header for URI [" << _uri.toString() << "]:\n";
+ logger << "WOPI::CheckFileInfo header for URI [" << uriPublic.toString() << "]:\n";
for (auto& pair : response)
{
logger << '\t' + pair.first + ": " + pair.second << " / ";
@@ -319,13 +313,6 @@ std::string WopiStorage::loadStorageFileToLocal()
{
Log::info("Downloading URI [" + _uri.toString() + "].");
- getFileInfo();
- if (!_fileInfo.isValid())
- {
- //TODO: Should throw a more appropriate exception.
- throw std::runtime_error("Failed to load file from storage.");
- }
-
// WOPI URI to download files ends in '/contents'.
// Add it here to get the payload instead of file info.
Poco::URI uriObject(_uri);
@@ -369,12 +356,12 @@ std::string WopiStorage::loadStorageFileToLocal()
return Poco::Path(_jailPath, _fileInfo._filename).toString();
}
-bool WopiStorage::saveLocalFileToStorage()
+bool WopiStorage::saveLocalFileToStorage(const Poco::URI& uriPublic)
{
- Log::info("Uploading URI [" + _uri.toString() + "] from [" + _jailedFilePath + "].");
+ Log::info("Uploading URI [" + uriPublic.toString() + "] from [" + _jailedFilePath + "].");
const auto size = getFileSize(_jailedFilePath);
- Poco::URI uriObject(_uri);
+ Poco::URI uriObject(uriPublic);
uriObject.setPath(uriObject.getPath() + "/contents");
Log::debug("Wopi posting: " + uriObject.toString());
@@ -403,9 +390,9 @@ bool WopiStorage::saveLocalFileToStorage()
return success;
}
-StorageBase::FileInfo WebDAVStorage::getFileInfo()
+StorageBase::FileInfo WebDAVStorage::getFileInfo(const Poco::URI& uriPublic)
{
- Log::debug("Getting info for webdav uri [" + _uri.toString() + "].");
+ Log::debug("Getting info for webdav uri [" + uriPublic.toString() + "].");
assert(false && "Not Implemented!");
return FileInfo({"bazinga", Poco::Timestamp(), 0, "admin", "admin"});
}
@@ -417,7 +404,7 @@ std::string WebDAVStorage::loadStorageFileToLocal()
return _uri.toString();
}
-bool WebDAVStorage::saveLocalFileToStorage()
+bool WebDAVStorage::saveLocalFileToStorage(const Poco::URI& /*uriPublic*/)
{
// TODO: implement webdav PUT.
return false;
diff --git a/loolwsd/Storage.hpp b/loolwsd/Storage.hpp
index 36edd1d..2e53b6a 100644
--- a/loolwsd/Storage.hpp
+++ b/loolwsd/Storage.hpp
@@ -76,22 +76,15 @@ public:
bool isLoaded() const { return _isLoaded; }
- void setLocalStorePath(const std::string& localStorePath) { _localStorePath = localStorePath; }
-
- void setJailPath(const std::string& jailPath) { _jailPath = jailPath; }
-
/// Returns information about the file.
- virtual FileInfo getFileInfo() = 0;
+ virtual FileInfo getFileInfo(const Poco::URI& uriPublic) = 0;
/// Returns a local file path for the given URI.
/// If necessary copies the file locally first.
virtual std::string loadStorageFileToLocal() = 0;
/// Writes the contents of the file back to the source.
- /// TODO: Should we save to the specific client's URI?
- /// The advantage is that subseqent views (to the first)
- /// will not depend on the token of the first.
- virtual bool saveLocalFileToStorage() = 0;
+ virtual bool saveLocalFileToStorage(const Poco::URI& uriPublic) = 0;
static
size_t getFileSize(const std::string& filename);
@@ -133,11 +126,11 @@ public:
"], jailPath: [" + jailPath + "], uri: [" + uri.toString() + "].");
}
- FileInfo getFileInfo() override;
+ FileInfo getFileInfo(const Poco::URI& uriPublic) override;
std::string loadStorageFileToLocal() override;
- bool saveLocalFileToStorage() override;
+ bool saveLocalFileToStorage(const Poco::URI& uriPublic) override;
private:
/// True if the jailed file is not linked but copied.
@@ -161,12 +154,12 @@ public:
"], jailPath: [" + jailPath + "], uri: [" + uri.toString() + "].");
}
- FileInfo getFileInfo() override;
+ FileInfo getFileInfo(const Poco::URI& uriPublic) override;
/// uri format: http://server/<...>/wopi*/files/<id>/content
std::string loadStorageFileToLocal() override;
- bool saveLocalFileToStorage() override;
+ bool saveLocalFileToStorage(const Poco::URI& uriPublic) override;
/// Total time taken for making WOPI calls during load (includes GetFileInfo calls)
const std::chrono::duration<double> getWopiLoadDuration() const { return _wopiLoadDuration; }
@@ -191,11 +184,11 @@ public:
"], jailPath: [" + jailPath + "], uri: [" + uri.toString() + "].");
}
- FileInfo getFileInfo() override;
+ FileInfo getFileInfo(const Poco::URI& uriPublic) override;
std::string loadStorageFileToLocal() override;
- bool saveLocalFileToStorage() override;
+ bool saveLocalFileToStorage(const Poco::URI& uriPublic) override;
private:
std::unique_ptr<AuthBase> _authAgent;
More information about the Libreoffice-commits
mailing list