[Libreoffice-commits] online.git: 2 commits - net/Socket.cpp test/httpwstest.cpp test/integration-http-server.cpp wsd/LOOLWSD.cpp

Ashod Nakashian (via logerrit) logerrit at kemper.freedesktop.org
Tue Oct 29 01:32:40 UTC 2019


 net/Socket.cpp                   |    2 +-
 test/httpwstest.cpp              |   14 +++++++-------
 test/integration-http-server.cpp |   36 ++++++++++++++++++++++++++++++------
 wsd/LOOLWSD.cpp                  |   26 ++++++++++++++------------
 4 files changed, 52 insertions(+), 26 deletions(-)

New commits:
commit c3632491a47f0fe7d0cac435caad213eff6218c1
Author:     Ashod Nakashian <ashod.nakashian at collabora.co.uk>
AuthorDate: Mon Oct 21 00:48:51 2019 -0400
Commit:     Ashod Nakashian <ashnakash at gmail.com>
CommitDate: Tue Oct 29 02:32:35 2019 +0100

    test: improve HTTPServerTest::testConvertToWithForwardedClientIP
    
    Extend the timeout, as often DNS lookup takes several seconds
    and that delays the response from WSD.
    
    Reviewed-on: https://gerrit.libreoffice.org/81198
    Reviewed-by: Andras Timar <andras.timar at collabora.com>
    Tested-by: Andras Timar <andras.timar at collabora.com>
    (cherry picked from commit ae085428dfb11b7965b73df0f40ac4fd1ec98a75)
    
    Change-Id: Ie51bff31782fa33eb5559d28af1477e1947382a3
    Reviewed-on: https://gerrit.libreoffice.org/81574
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/net/Socket.cpp b/net/Socket.cpp
index cf7087170..cbe5a0a52 100644
--- a/net/Socket.cpp
+++ b/net/Socket.cpp
@@ -785,7 +785,7 @@ bool StreamSocket::parseHeader(const char *clientName,
     }
     catch (const std::exception& exc)
     {
-        LOG_DBG("parseHeader exception caught.");
+        LOG_DBG("parseHeader exception caught: " << exc.what());
         // Probably don't have enough data just yet.
         // TODO: timeout if we never get enough.
         return false;
diff --git a/test/integration-http-server.cpp b/test/integration-http-server.cpp
index 8f36c8205..cf4efcdb4 100644
--- a/test/integration-http-server.cpp
+++ b/test/integration-http-server.cpp
@@ -370,11 +370,17 @@ void HTTPServerTest::testConvertTo()
 
 void HTTPServerTest::testConvertToWithForwardedClientIP()
 {
+    const std::string testname = "convertToWithForwardedClientIP-";
+    constexpr int TimeoutSeconds = 10; // Sometimes dns resolving is slow.
+
     // Test a forwarded IP which is not allowed to use convert-to feature
+    try
     {
-        const std::string srcPath = FileUtil::getTempFilePath(TDOC, "hello.odt", "testConvertToWithForwardedClientIP_");
+        TST_LOG("Converting from a disallowed IP.");
+
+        const std::string srcPath = FileUtil::getTempFilePath(TDOC, "hello.odt", testname + "disallowed");
         std::unique_ptr<Poco::Net::HTTPClientSession> session(helpers::createSession(_uri));
-        session->setTimeout(Poco::Timespan(2, 0)); // 2 seconds.
+        session->setTimeout(Poco::Timespan(TimeoutSeconds, 0));
 
         Poco::Net::HTTPRequest request(Poco::Net::HTTPRequest::HTTP_POST, "/lool/convert-to");
         CPPUNIT_ASSERT(!request.has("X-Forwarded-For"));
@@ -406,12 +412,19 @@ void HTTPServerTest::testConvertToWithForwardedClientIP()
         std::string actualString = actualStream.str();
         CPPUNIT_ASSERT(actualString.empty()); // <- we did not get the converted file
     }
+    catch(const Poco::Exception& exc)
+    {
+        CPPUNIT_FAIL(exc.displayText() + ": " + (exc.nested() ? exc.nested()->displayText() : ""));
+    }
 
     // Test a forwarded IP which is allowed to use convert-to feature
+    try
     {
-        const std::string srcPath = FileUtil::getTempFilePath(TDOC, "hello.odt", "testConvertToWithForwardedClientIP_");
+        TST_LOG("Converting from an allowed IP.");
+
+        const std::string srcPath = FileUtil::getTempFilePath(TDOC, "hello.odt", testname + "allowed");
         std::unique_ptr<Poco::Net::HTTPClientSession> session(helpers::createSession(_uri));
-        session->setTimeout(Poco::Timespan(2, 0)); // 2 seconds.
+        session->setTimeout(Poco::Timespan(TimeoutSeconds, 0));
 
         Poco::Net::HTTPRequest request(Poco::Net::HTTPRequest::HTTP_POST, "/lool/convert-to");
         CPPUNIT_ASSERT(!request.has("X-Forwarded-For"));
@@ -442,12 +455,19 @@ void HTTPServerTest::testConvertToWithForwardedClientIP()
             actualString = actualString.substr(3);
         CPPUNIT_ASSERT_EQUAL(expectedStream.str(), actualString); // <- we got the converted file
     }
+    catch(const Poco::Exception& exc)
+    {
+        CPPUNIT_FAIL(exc.displayText() + ": " + (exc.nested() ? exc.nested()->displayText() : ""));
+    }
 
     // Test a forwarded header with three IPs, one is not allowed -> request is denied.
+    try
     {
-        const std::string srcPath = FileUtil::getTempFilePath(TDOC, "hello.odt", "testConvertToWithForwardedClientIP_");
+        TST_LOG("Converting from multiple IPs, on disallowed.");
+
+        const std::string srcPath = FileUtil::getTempFilePath(TDOC, "hello.odt", testname + "disallowed-multi");
         std::unique_ptr<Poco::Net::HTTPClientSession> session(helpers::createSession(_uri));
-        session->setTimeout(Poco::Timespan(2, 0)); // 2 seconds.
+        session->setTimeout(Poco::Timespan(TimeoutSeconds, 0));
 
         Poco::Net::HTTPRequest request(Poco::Net::HTTPRequest::HTTP_POST, "/lool/convert-to");
         CPPUNIT_ASSERT(!request.has("X-Forwarded-For"));
@@ -472,6 +492,10 @@ void HTTPServerTest::testConvertToWithForwardedClientIP()
         std::string actualString = actualStream.str();
         CPPUNIT_ASSERT(actualString.empty()); // <- we did not get the converted file
     }
+    catch(const Poco::Exception& exc)
+    {
+        CPPUNIT_FAIL(exc.displayText() + ": " + (exc.nested() ? exc.nested()->displayText() : ""));
+    }
 }
 
 CPPUNIT_TEST_SUITE_REGISTRATION(HTTPServerTest);
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index e43b827ec..f6d5132dc 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -2037,6 +2037,8 @@ public:
                     break;
                 }
             }
+
+            init = true;
         }
         return hosts.match(address);
     }
@@ -2056,7 +2058,7 @@ public:
         // Handle forwarded header and make sure all participating IPs are allowed
         if(request.has("X-Forwarded-For"))
         {
-            std::string fowardedData = request.get("X-Forwarded-For");
+            const std::string fowardedData = request.get("X-Forwarded-For");
             std::vector<std::string> tokens = LOOLProtocol::tokenize(fowardedData, ',');
             for(std::string& token : tokens)
             {
@@ -2538,7 +2540,7 @@ private:
             // Validate sender - FIXME: should do this even earlier.
             if (!allowConvertTo(socket->clientAddress(), request, true))
             {
-                LOG_TRC("Conversion not allowed from this address");
+                LOG_WRN("Conversion requests not allowed from this address: " << socket->clientAddress());
                 std::ostringstream oss;
                 oss << "HTTP/1.1 403\r\n"
                     "Date: " << Util::getHttpTimeNow() << "\r\n"
@@ -2579,19 +2581,19 @@ private:
                     bFullSheetPreview = false;
                 }
 
-                    // This lock could become a bottleneck.
-                    // In that case, we can use a pool and index by publicPath.
-                    std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
+                // This lock could become a bottleneck.
+                // In that case, we can use a pool and index by publicPath.
+                std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
 
-                    LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
-                    auto docBroker = std::make_shared<ConvertToBroker>(fromPath, uriPublic, docKey);
-                    handler.takeFile();
+                LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
+                auto docBroker = std::make_shared<ConvertToBroker>(fromPath, uriPublic, docKey);
+                handler.takeFile();
 
-                    cleanupDocBrokers();
+                cleanupDocBrokers();
 
-                    LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
-                    DocBrokers.emplace(docKey, docBroker);
-                    LOG_TRC("Have " << DocBrokers.size() << " DocBrokers after inserting [" << docKey << "].");
+                LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
+                DocBrokers.emplace(docKey, docBroker);
+                LOG_TRC("Have " << DocBrokers.size() << " DocBrokers after inserting [" << docKey << "].");
 
                 // Load the document.
                 // TODO: Move to DocumentBroker.
commit c3bd1b8bf66e9628fc811b2e9667fb065fd5b2ca
Author:     Ashod Nakashian <ashod.nakashian at collabora.co.uk>
AuthorDate: Mon Oct 21 00:47:55 2019 -0400
Commit:     Ashod Nakashian <ashnakash at gmail.com>
CommitDate: Tue Oct 29 02:32:22 2019 +0100

    test: improve HTTPWSTest::testUndoConflict
    
    Failure rate is now reduced.
    
    Change-Id: I46c26582126c7dba2a3fc86dbbf6ec5f12aa740f
    Reviewed-on: https://gerrit.libreoffice.org/81197
    Reviewed-by: Andras Timar <andras.timar at collabora.com>
    Tested-by: Andras Timar <andras.timar at collabora.com>
    (cherry picked from commit a12092c85d3b8e8e3b3c54b0eabb6bd3725b9542)
    Reviewed-on: https://gerrit.libreoffice.org/81573
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/test/httpwstest.cpp b/test/httpwstest.cpp
index a6e2c2f59..40b0d604f 100644
--- a/test/httpwstest.cpp
+++ b/test/httpwstest.cpp
@@ -2343,22 +2343,22 @@ void HTTPWSTest::testUndoConflict()
         response = getResponseString(socket1, "invalidatecursor:", testname + "1 ");
 
         // edit first view
-        sendTextFrame(socket0, "key type=input char=97 key=0", testname);
-        response = getResponseString(socket0, "invalidatecursor:", testname + "0 ");
+        sendChar(socket0, 'A', skNone, testname + "0 ");
+        response = getResponseString(socket0, "invalidateviewcursor: ", testname + "0 ");
         // edit second view
-        sendTextFrame(socket1, "key type=input char=98 key=0", testname);
-        response = getResponseString(socket1, "invalidatecursor:", testname + "1 ");
+        sendChar(socket1, 'B', skNone, testname + "1 ");
+        response = getResponseString(socket1, "invalidateviewcursor: ", testname + "1 ");
         // try to undo first view
         sendTextFrame(socket0, "uno .uno:Undo", testname);
         // undo conflict
         response = getResponseString(socket0, "unocommandresult:", testname + "0 ");
         Poco::JSON::Object::Ptr objJSON = parser.parse(response.substr(17)).extract<Poco::JSON::Object::Ptr>();
-        CPPUNIT_ASSERT_EQUAL(objJSON->get("commandName").toString(), std::string(".uno:Undo"));
-        CPPUNIT_ASSERT_EQUAL(objJSON->get("success").toString(), std::string("true"));
+        CPPUNIT_ASSERT_EQUAL(std::string(".uno:Undo"), objJSON->get("commandName").toString());
+        CPPUNIT_ASSERT_EQUAL(std::string("true"), objJSON->get("success").toString());
         CPPUNIT_ASSERT(objJSON->has("result"));
         const Poco::Dynamic::Var parsedResultJSON = objJSON->get("result");
         const auto& resultObj = parsedResultJSON.extract<Poco::JSON::Object::Ptr>();
-        CPPUNIT_ASSERT_EQUAL(resultObj->get("type").toString(), std::string("long"));
+        CPPUNIT_ASSERT_EQUAL(std::string("long"), resultObj->get("type").toString());
         CPPUNIT_ASSERT(Poco::strToInt(resultObj->get("value").toString(), conflict, 10));
         CPPUNIT_ASSERT(conflict > 0); /*UNDO_CONFLICT*/
     }


More information about the Libreoffice-commits mailing list