Simplify download processdistro/cib/libreoffice-6-2

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@gmail.com>
Tested-by: Michael Meeks <michael.meeks@collabora.com>
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>

7 files changed, 79 insertions, 36 deletions

diff --git a/kit/ChildSession.cpp b/kit/ChildSession.cpp
index cff93a276..ad66f485a 100644
--- a/kit/ChildSession.cpp
+++ b/kit/ChildSession.cpp
@@ -687,7 +687,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();
 
@@ -703,7 +704,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 21a2cf4a3..d2989dd3d 100644
--- a/loleaflet/src/core/Socket.js
+++ b/loleaflet/src/core/Socket.js
@@ -944,6 +944,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 dab81ba49..be3e8cd34 100644
--- a/loleaflet/src/layer/tile/TileLayer.js
+++ b/loleaflet/src/layer/tile/TileLayer.js
@@ -590,7 +590,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 f0aee365a..3c266604e 100644
--- a/test/httpwstest.cpp
+++ b/test/httpwstest.cpp
@@ -1262,21 +1262,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 83f942b5d..da6bf8a2a 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -1259,6 +1259,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)
 {
@@ -1305,6 +1321,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 70a6bfb27..8a3734615 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -362,6 +362,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:
 
     /// Shutdown all client connections with the given reason.
@@ -484,6 +490,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 7b217cdb5..a701ec356 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -2495,7 +2495,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!
@@ -2511,30 +2511,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();
 
@@ -2557,7 +2550,6 @@ private:
                 try
                 {
                     HttpHelper::sendFile(socket, filePath.toString(), contentType, response);
-                    responded = true;
                 }
                 catch (const Exception& exc)
                 {
@@ -2569,9 +2561,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;
         }