[Libreoffice-commits] online.git: cypress_test/integration_tests 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
Mon Sep 7 13:00:41 UTC 2020
cypress_test/integration_tests/mobile/calc/hamburger_menu_spec.js | 8 -
cypress_test/integration_tests/mobile/impress/hamburger_menu_spec.js | 8 -
cypress_test/integration_tests/mobile/writer/hamburger_menu_spec.js | 12 +-
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 +++-------
10 files changed, 85 insertions(+), 55 deletions(-)
New commits:
commit 70827c372c31f5d11dc9578269cdd850eee639ef
Author: Szymon Kłos <szymon.klos at collabora.com>
AuthorDate: Thu Sep 3 11:34:13 2020 +0200
Commit: Michael Meeks <michael.meeks at collabora.com>
CommitDate: Mon Sep 7 15:00:20 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>
diff --git a/cypress_test/integration_tests/mobile/calc/hamburger_menu_spec.js b/cypress_test/integration_tests/mobile/calc/hamburger_menu_spec.js
index 7b71b3c16..19d49cdab 100644
--- a/cypress_test/integration_tests/mobile/calc/hamburger_menu_spec.js
+++ b/cypress_test/integration_tests/mobile/calc/hamburger_menu_spec.js
@@ -69,7 +69,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.pdf');
+ .should('contain', 'download');
});
it('Download as ODS', function() {
@@ -85,7 +85,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.ods');
+ .should('contain', 'download');
});
it('Download as XLS', function() {
@@ -101,7 +101,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.xls');
+ .should('contain', 'download');
});
it('Download as XLSX', function() {
@@ -117,7 +117,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.xlsx');
+ .should('contain', 'download');
});
it('Undo/redo.', function() {
diff --git a/cypress_test/integration_tests/mobile/impress/hamburger_menu_spec.js b/cypress_test/integration_tests/mobile/impress/hamburger_menu_spec.js
index 73c58ee13..92d3e6dbc 100644
--- a/cypress_test/integration_tests/mobile/impress/hamburger_menu_spec.js
+++ b/cypress_test/integration_tests/mobile/impress/hamburger_menu_spec.js
@@ -80,7 +80,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.pdf');
+ .should('contain', 'download');
});
it('Download as ODP', function() {
@@ -96,7 +96,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.odp');
+ .should('contain', 'download');
});
it('Download as PPT', function() {
@@ -112,7 +112,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.ppt');
+ .should('contain', 'download');
});
it('Download as PPTX', function() {
@@ -128,7 +128,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.pptx');
+ .should('contain', 'download');
});
it('Undo/redo.', function() {
diff --git a/cypress_test/integration_tests/mobile/writer/hamburger_menu_spec.js b/cypress_test/integration_tests/mobile/writer/hamburger_menu_spec.js
index 3c23f9163..8c2d6e985 100644
--- a/cypress_test/integration_tests/mobile/writer/hamburger_menu_spec.js
+++ b/cypress_test/integration_tests/mobile/writer/hamburger_menu_spec.js
@@ -99,7 +99,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.pdf');
+ .should('contain', 'download');
});
it('Download as ODT', function() {
@@ -113,7 +113,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.odt');
+ .should('contain', 'download');
});
it('Download as DOC', function() {
@@ -127,7 +127,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.doc');
+ .should('contain', 'download');
});
it('Download as DOCX', function() {
@@ -141,7 +141,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.docx');
+ .should('contain', 'download');
});
it('Download as RTF', function() {
@@ -155,7 +155,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.rtf');
+ .should('contain', 'download');
});
it('Download as EPUB', function() {
@@ -169,7 +169,7 @@ describe('Trigger hamburger menu options.', function() {
cy.get('iframe')
.should('have.attr', 'data-src')
- .should('contain', 'document.epub');
+ .should('contain', 'download');
});
it('Undo/redo.', function() {
diff --git a/kit/ChildSession.cpp b/kit/ChildSession.cpp
index bae3cce4f..96bf87372 100644
--- a/kit/ChildSession.cpp
+++ b/kit/ChildSession.cpp
@@ -949,7 +949,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='" <<
@@ -960,7 +961,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 fa78bf1a3..8a22e0c8d 100644
--- a/loleaflet/src/core/Socket.js
+++ b/loleaflet/src/core/Socket.js
@@ -1151,6 +1151,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 e7558562c..16744d019 100644
--- a/loleaflet/src/layer/tile/TileLayer.js
+++ b/loleaflet/src/layer/tile/TileLayer.js
@@ -923,7 +923,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 4924d5c9f..909c8db1b 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -1653,6 +1653,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)
{
@@ -1689,6 +1705,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 e59a2d6b4..474df3838 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -303,6 +303,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;
@@ -463,6 +469,9 @@ private:
// Relevant only in the mobile apps
const unsigned _mobileAppDocId;
+
+ // Maps download id -> URL
+ std::map<std::string, std::string> _registeredDownloadLinks;
};
#if !MOBILEAPP
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index a901f08b8..579267e5e 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -3099,7 +3099,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!
@@ -3115,38 +3115,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();
@@ -3180,7 +3165,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