[Libreoffice-commits] online.git: Branch 'distro/collabora/collabora-online-4-0' - kit/ChildSession.cpp loleaflet/src test/httpwstest.cpp wsd/DocumentBroker.cpp wsd/DocumentBroker.hpp wsd/LOOLWSD.cpp
Szymon KÅ‚os (via logerrit)
logerrit at kemper.freedesktop.org
Tue Sep 15 19:12:42 UTC 2020
kit/ChildSession.cpp | 10 +++++--
loleaflet/src/core/Socket.js | 3 ++
loleaflet/src/layer/tile/TileLayer.js | 2 -
test/httpwstest.cpp | 16 ++++-------
wsd/DocumentBroker.cpp | 27 +++++++++++++++++++
wsd/DocumentBroker.hpp | 9 ++++++
wsd/LOOLWSD.cpp | 48 +++++++++++++++++-----------------
7 files changed, 79 insertions(+), 36 deletions(-)
New commits:
commit ab162b6f9580315700a01c3bc10becd510a2ead4
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 15 21:12: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/+/102227
Reviewed-by: Andras Timar <andras.timar at collabora.com>
diff --git a/kit/ChildSession.cpp b/kit/ChildSession.cpp
index 23224c3f8..a108ed6c0 100644
--- a/kit/ChildSession.cpp
+++ b/kit/ChildSession.cpp
@@ -883,7 +883,8 @@ bool ChildSession::downloadAs(const char* /*buffer*/, int /*length*/, const std:
// Prevent user inputting anything funny here.
// A "name" should always be a name, not a path
const Poco::Path filenameParam(name);
- const std::string url = JAILED_DOCUMENT_ROOT + tmpDir + "/" + filenameParam.getFileName();
+ const std::string urlToSend = tmpDir + "/" + filenameParam.getFileName();
+ const std::string url = JAILED_DOCUMENT_ROOT + urlToSend;
const std::string nameAnonym = anonymizeUrl(name);
const std::string urlAnonym = JAILED_DOCUMENT_ROOT + tmpDir + "/" + Poco::Path(nameAnonym).getFileName();
@@ -899,7 +900,12 @@ bool ChildSession::downloadAs(const char* /*buffer*/, int /*length*/, const std:
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);
return true;
}
diff --git a/loleaflet/src/core/Socket.js b/loleaflet/src/core/Socket.js
index ec55e72b1..2711dda5e 100644
--- a/loleaflet/src/core/Socket.js
+++ b/loleaflet/src/core/Socket.js
@@ -956,6 +956,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 c70012314..c789dc484 100644
--- a/loleaflet/src/layer/tile/TileLayer.js
+++ b/loleaflet/src/layer/tile/TileLayer.js
@@ -589,7 +589,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 (command.id === 'print') {
diff --git a/test/httpwstest.cpp b/test/httpwstest.cpp
index 8fd886365..383f35cf5 100644
--- a/test/httpwstest.cpp
+++ b/test/httpwstest.cpp
@@ -1298,21 +1298,17 @@ void HTTPWSTest::testSlideShow()
CPPUNIT_ASSERT_MESSAGE("did not receive a downloadas: message as expected", !response.empty());
Poco::StringTokenizer tokens(response.substr(11), " ", Poco::StringTokenizer::TOK_IGNORE_EMPTY | Poco::StringTokenizer::TOK_TRIM);
- // "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());
- CPPUNIT_ASSERT(!jail.empty());
- CPPUNIT_ASSERT(!dir.empty());
- CPPUNIT_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());
+ CPPUNIT_ASSERT(!downloadId.empty());
CPPUNIT_ASSERT_EQUAL(static_cast<int>(_uri.getPort()), port);
CPPUNIT_ASSERT_EQUAL(std::string("slideshow"), id);
std::string encodedDoc;
Poco::URI::encode(documentPath, ":/?", encodedDoc);
- const std::string path = "/lool/" + encodedDoc + "/" + jail + "/" + dir + "/" + name;
+ const std::string path = "/lool/" + encodedDoc + "/download/" + downloadId;
std::unique_ptr<Poco::Net::HTTPClientSession> session(helpers::createSession(_uri));
Poco::Net::HTTPRequest requestSVG(Poco::Net::HTTPRequest::HTTP_GET, path);
TST_LOG("Requesting SVG from " << path);
diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index ae7d2ecf5..bdf88f867 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -1346,6 +1346,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)
{
@@ -1392,6 +1408,17 @@ bool DocumentBroker::handleInput(const std::vector<char>& payload)
}
}
#endif
+ 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 439b33849..733a5c700 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -345,6 +345,12 @@ public:
/// Sets the initialization flag of a given initial setting.
void setInitialSetting(const std::string& name);
+ /// 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:
/// Loads a document from the public URI into the jail.
@@ -482,6 +488,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;
};
class ConvertToBroker : public DocumentBroker
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 44d43ae35..4f8e108fa 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -2517,7 +2517,7 @@ private:
}
}
}
- else if (tokens.count() >= 6)
+ else if (tokens.count() > 3 && tokens[3] == "download")
{
LOG_INF("File download request.");
// TODO: Check that the user in question has access to this file!
@@ -2533,30 +2533,23 @@ private:
throw BadRequestException("DocKey [" + docKey + "] is invalid.");
}
- // 2. Cross-check if received child id is correct
- if (docBrokerIt->second->getJailId() != tokens[3])
- {
- throw BadRequestException("ChildId does not correspond to docKey");
- }
+ std::string downloadId = tokens[4];
+ 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() == tokens[4])
- {
- throw BadRequestException("RandomDir cannot be equal to ChildId");
- }
docBrokersLock.unlock();
- std::string fileName;
- URI::decode(tokens[5], fileName);
- const Path filePath(LOOLWSD::ChildRoot + tokens[3]
- + JAILED_DOCUMENT_ROOT + tokens[4] + "/" + fileName);
+ bool foundDownloadId = !url.empty();
+
+ const Path filePath(LOOLWSD::ChildRoot + jailId + JAILED_DOCUMENT_ROOT + url);
const std::string filePathAnonym = LOOLWSD::anonymizeUrl(filePath.toString());
- LOG_INF("HTTP request for: " << filePathAnonym);
- bool responded = false;
- 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();
@@ -2579,7 +2572,6 @@ private:
try
{
HttpHelper::sendFile(socket, filePath.toString(), contentType, response);
- responded = true;
}
catch (const Exception& exc)
{
@@ -2591,9 +2583,19 @@ 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"
+ << "User-Agent: " << HTTP_AGENT_STRING << "\r\n"
+ << "Content-Length: 0\r\n"
+ << "\r\n";
+ socket->send(oss.str());
+ socket->shutdown();
}
- (void)responded;
return;
}
More information about the Libreoffice-commits
mailing list