[Libreoffice-commits] online.git: Branch 'distro/collabora/co-4-2' - kit/ChildSession.cpp loleaflet/src test/UnitSession.cpp wsd/DocumentBroker.cpp wsd/DocumentBroker.hpp wsd/LOOLWSD.cpp
Szymon KÅ‚os (via logerrit)
logerrit at kemper.freedesktop.org
Tue Sep 8 10:31:41 UTC 2020
kit/ChildSession.cpp | 10 ++++++-
loleaflet/src/core/Socket.js | 3 ++
loleaflet/src/layer/tile/TileLayer.js | 2 -
test/UnitSession.cpp | 16 ++++--------
wsd/DocumentBroker.cpp | 27 ++++++++++++++++++++
wsd/DocumentBroker.hpp | 9 ++++++
wsd/LOOLWSD.cpp | 45 ++++++++++++----------------------
7 files changed, 71 insertions(+), 41 deletions(-)
New commits:
commit 3a821c2af65505e0ac6d317d7141dc5475f13730
Author: Szymon Kłos <szymon.klos at collabora.com>
AuthorDate: Thu Sep 3 11:34:13 2020 +0200
Commit: Andras Timar <andras.timar at collabora.com>
CommitDate: Tue Sep 8 12:31:21 2020 +0200
Simplify download process
Use hash to identify download and pass that to the client.
This allows us to reduce parameters for download requests.
DocBroker maps download ids to URL in the file system.
Change-Id: I254d4f0ccaf3cff9f038a817c8162510ae228bc5
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/101992
Tested-by: Jenkins
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice at gmail.com>
Tested-by: Michael Meeks <michael.meeks at collabora.com>
Reviewed-by: Michael Meeks <michael.meeks at collabora.com>
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/102185
Reviewed-by: Andras Timar <andras.timar at collabora.com>
diff --git a/kit/ChildSession.cpp b/kit/ChildSession.cpp
index 4a5aebcb8..5c7df7272 100644
--- a/kit/ChildSession.cpp
+++ b/kit/ChildSession.cpp
@@ -952,7 +952,8 @@ bool ChildSession::downloadAs(const char* /*buffer*/, int /*length*/, const Stri
// The file is removed upon downloading.
const std::string tmpDir = FileUtil::createRandomDir(jailDoc);
- const std::string url = jailDoc + tmpDir + "/" + filenameParam.getFileName();
+ const std::string urlToSend = tmpDir + "/" + filenameParam.getFileName();
+ const std::string url = jailDoc + urlToSend;
const std::string urlAnonym = jailDoc + tmpDir + "/" + Poco::Path(nameAnonym).getFileName();
LOG_DBG("Calling LOK's saveAs with: url='" << urlAnonym << "', format='" <<
@@ -963,7 +964,12 @@ bool ChildSession::downloadAs(const char* /*buffer*/, int /*length*/, const Stri
format.empty() ? nullptr : format.c_str(),
filterOptions.empty() ? nullptr : filterOptions.c_str());
- sendTextFrame("downloadas: jail=" + _jailId + " dir=" + tmpDir + " name=" + name +
+ // Register download id -> URL mapping in the DocumentBroker
+ std::string docBrokerMessage = "registerdownload: downloadid=" + tmpDir + " url=" + urlToSend;
+ _docManager->sendFrame(docBrokerMessage.c_str(), docBrokerMessage.length());
+
+ // Send download id to the client
+ sendTextFrame("downloadas: downloadid=" + tmpDir +
" port=" + std::to_string(ClientPortNumber) + " id=" + id);
#endif
return true;
diff --git a/loleaflet/src/core/Socket.js b/loleaflet/src/core/Socket.js
index 95588578e..32a601aca 100644
--- a/loleaflet/src/core/Socket.js
+++ b/loleaflet/src/core/Socket.js
@@ -1132,6 +1132,9 @@ L.Socket = L.Class.extend({
else if (tokens[i].substring(0, 4) === 'dir=') {
command.dir = tokens[i].substring(4);
}
+ else if (tokens[i].substring(0, 11) === 'downloadid=') {
+ command.downloadid = tokens[i].substring(11);
+ }
else if (tokens[i].substring(0, 5) === 'name=') {
command.name = tokens[i].substring(5);
}
diff --git a/loleaflet/src/layer/tile/TileLayer.js b/loleaflet/src/layer/tile/TileLayer.js
index ba0157ac7..7e46e112b 100644
--- a/loleaflet/src/layer/tile/TileLayer.js
+++ b/loleaflet/src/layer/tile/TileLayer.js
@@ -914,7 +914,7 @@ L.TileLayer = L.GridLayer.extend({
wopiSrc = '?WOPISrc=' + this._map.options.wopiSrc;
}
var url = this._map.options.webserver + this._map.options.serviceRoot + '/' + this._map.options.urlPrefix + '/' +
- encodeURIComponent(this._map.options.doc) + '/' + command.jail + '/' + command.dir + '/' + command.name + wopiSrc;
+ encodeURIComponent(this._map.options.doc) + '/download/' + command.downloadid + wopiSrc;
this._map.hideBusy();
if (this._map['wopi'].DownloadAsPostMessage) {
diff --git a/test/UnitSession.cpp b/test/UnitSession.cpp
index ec8d1f9bf..bbff4a09e 100644
--- a/test/UnitSession.cpp
+++ b/test/UnitSession.cpp
@@ -185,15 +185,11 @@ UnitBase::TestResult UnitSession::testSlideShow()
!response.empty());
StringVector tokens(Util::tokenize(response.substr(11), ' '));
- // "downloadas: jail= dir= name=slideshow.svg port= id=slideshow"
- const std::string jail = tokens[0].substr(std::string("jail=").size());
- const std::string dir = tokens[1].substr(std::string("dir=").size());
- const std::string name = tokens[2].substr(std::string("name=").size());
- const int port = std::stoi(tokens[3].substr(std::string("port=").size()));
- const std::string id = tokens[4].substr(std::string("id=").size());
- LOK_ASSERT(!jail.empty());
- LOK_ASSERT(!dir.empty());
- LOK_ASSERT_EQUAL(std::string("slideshow.svg"), name);
+ // "downloadas: downloadId= port= id=slideshow"
+ const std::string downloadId = tokens[0].substr(std::string("downloadId=").size());
+ const int port = std::stoi(tokens[1].substr(std::string("port=").size()));
+ const std::string id = tokens[2].substr(std::string("id=").size());
+ LOK_ASSERT(!downloadId.empty());
LOK_ASSERT_EQUAL(static_cast<int>(Poco::URI(helpers::getTestServerURI()).getPort()),
port);
LOK_ASSERT_EQUAL(std::string("slideshow"), id);
@@ -201,7 +197,7 @@ UnitBase::TestResult UnitSession::testSlideShow()
std::string encodedDoc;
Poco::URI::encode(documentPath, ":/?", encodedDoc);
const std::string ignoredSuffix = "%3FWOPISRC=madness"; // cf. iPhone.
- const std::string path = "/lool/" + encodedDoc + '/' + jail + '/' + dir + '/' + name + ignoredSuffix;
+ const std::string path = "/lool/" + encodedDoc + "/download/" + downloadId + '/' + ignoredSuffix;
std::unique_ptr<Poco::Net::HTTPClientSession> session(
helpers::createSession(Poco::URI(helpers::getTestServerURI())));
Poco::Net::HTTPRequest requestSVG(Poco::Net::HTTPRequest::HTTP_GET, path);
diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 05bd288f8..d92e1bc75 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -1605,6 +1605,22 @@ void DocumentBroker::alertAllUsers(const std::string& msg)
}
}
+std::string DocumentBroker::getDownloadURL(const std::string& downloadId)
+{
+ auto aFound = _registeredDownloadLinks.find(downloadId);
+ if (aFound != _registeredDownloadLinks.end())
+ return aFound->second;
+
+ return "";
+}
+
+void DocumentBroker::unregisterDownloadId(const std::string& downloadId)
+{
+ auto aFound = _registeredDownloadLinks.find(downloadId);
+ if (aFound != _registeredDownloadLinks.end())
+ _registeredDownloadLinks.erase(aFound);
+}
+
/// Handles input from the prisoner / child kit process
bool DocumentBroker::handleInput(const std::vector<char>& payload)
{
@@ -1641,6 +1657,17 @@ bool DocumentBroker::handleInput(const std::vector<char>& payload)
LOG_CHECK_RET(kind != "", false);
Util::alertAllUsers(cmd, kind);
}
+ else if (command == "registerdownload:")
+ {
+ LOG_CHECK_RET(message->tokens().size() == 3, false);
+ std::string downloadid, url;
+ LOOLProtocol::getTokenString((*message)[1], "downloadid", downloadid);
+ LOG_CHECK_RET(downloadid != "", false);
+ LOOLProtocol::getTokenString((*message)[2], "url", url);
+ LOG_CHECK_RET(url != "", false);
+
+ _registeredDownloadLinks[downloadid] = url;
+ }
else
{
LOG_ERR("Unexpected message: [" << msg << "].");
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index db5f2b4c4..f3ba8c461 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -301,6 +301,12 @@ public:
/// Estimate memory usage / bytes
size_t getMemorySize() const;
+ /// Get URL for corresponding download id if registered, or empty string otherwise
+ std::string getDownloadURL(const std::string& downloadId);
+
+ /// Remove download id mapping
+ void unregisterDownloadId(const std::string& downloadId);
+
private:
/// get the session id of a session that can write the document for save / locking.
std::string getWriteableSessionId() const;
@@ -447,6 +453,9 @@ private:
/// Unique DocBroker ID for tracing and debugging.
static std::atomic<unsigned> DocBrokerId;
+
+ // Maps download id -> URL
+ std::map<std::string, std::string> _registeredDownloadLinks;
};
#if !MOBILEAPP
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index afc2b9c58..aef5d38c9 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -2859,7 +2859,7 @@ private:
}
}
}
- else if (requestDetails.size() >= 5)
+ else if (requestDetails.equals(2, "download"))
{
LOG_INF("File download request.");
// TODO: Check that the user in question has access to this file!
@@ -2875,38 +2875,23 @@ private:
throw BadRequestException("DocKey [" + docKey + "] is invalid.");
}
- // 2. Cross-check if received child id is correct
- if (docBrokerIt->second->getJailId() != requestDetails[2])
- {
- throw BadRequestException("ChildId does not correspond to docKey");
- }
+ std::string downloadId = requestDetails[3];
+ std::string url = docBrokerIt->second->getDownloadURL(downloadId);
+ docBrokerIt->second->unregisterDownloadId(downloadId);
+ std::string jailId = docBrokerIt->second->getJailId();
- // 3. Don't let user download the file in main doc directory containing
- // the document being edited otherwise we will end up deleting main directory
- // after download finishes
- if (docBrokerIt->second->getJailId() == requestDetails[3])
- {
- throw BadRequestException("RandomDir cannot be equal to ChildId");
- }
docBrokersLock.unlock();
- std::string fileName;
- URI::decode(requestDetails[4], fileName);
- // sanitize if we can
- auto it = fileName.find_first_of("?&%#!");
- if (it != std::string::npos)
- {
- std::string cleanedName = fileName.substr(0, it);
- LOG_DBG("Cleaned unexpected parameters from filename: '" << fileName << "' to '" << cleanedName << "'");
- fileName = cleanedName;
- }
+ bool foundDownloadId = !url.empty();
- const Path filePath(LOOLWSD::ChildRoot + requestDetails[2]
- + JAILED_DOCUMENT_ROOT + requestDetails[3] + '/' + fileName);
+ const Path filePath(LOOLWSD::ChildRoot + jailId + JAILED_DOCUMENT_ROOT + url);
const std::string filePathAnonym = LOOLWSD::anonymizeUrl(filePath.toString());
- LOG_INF("HTTP request for: " << filePathAnonym);
- if (filePath.isAbsolute() && File(filePath).exists())
+
+ if (foundDownloadId && filePath.isAbsolute() && File(filePath).exists())
{
+ LOG_INF("HTTP request for: " << filePathAnonym);
+
+ std::string fileName = filePath.getFileName();
const Poco::URI postRequestUri(request.getURI());
const Poco::URI::QueryParameters postRequestQueryParams = postRequestUri.getQueryParameters();
@@ -2940,7 +2925,11 @@ private:
}
else
{
- LOG_ERR("Download file [" << filePathAnonym << "] not found.");
+ if (foundDownloadId)
+ LOG_ERR("Download file [" << filePathAnonym << "] not found.");
+ else
+ LOG_ERR("Download with id [" << downloadId << "] not found.");
+
std::ostringstream oss;
oss << "HTTP/1.1 404 Not Found\r\n"
<< "Date: " << Util::getHttpTimeNow() << "\r\n"
More information about the Libreoffice-commits
mailing list