NAM: implement direct media requests fallback

This is behind a configuration switch because direct unauthenticated requests are at odds with the Matrix model where clients stick to their homeservers and only homeservers talk to each other. The implementation is also quite inefficient atm, causing a .well-known resolution roundtrip every single time a direct media request is made.
quotient-im · May 27, 2023 · b34e13d · b34e13d
1 parent 26aa367
commit b34e13d
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 38 deletions.
diff --git a/Quotient/mxcreply.cpp b/Quotient/mxcreply.cpp
@@ -20,9 +20,15 @@ class Q_DECL_HIDDEN MxcReply::Private
 
 MxcReply::MxcReply(QNetworkReply* reply,
                    const EncryptedFileMetadata& fileMetadata)
-    : d(makeImpl<Private>(reply, fileMetadata.isValid() ? nullptr : reply))
 {
-    reply->setParent(this);
+    setNetworkReply(reply, fileMetadata);
+}
+
+void MxcReply::setNetworkReply(QNetworkReply* reply,
+                               const EncryptedFileMetadata& fileMetadata)
+{
+    d = makeImpl<Private>(reply, fileMetadata.isValid() ? nullptr : reply);
+    d->m_reply->setParent(this);
     connect(d->m_reply, &QNetworkReply::finished, this, [this, fileMetadata] {
         setError(d->m_reply->error(), d->m_reply->errorString());
 
@@ -34,16 +40,18 @@ MxcReply::MxcReply(QNetworkReply* reply,
             d->m_device = buffer;
         }
 #endif
+        setFinished(true);
         setOpenMode(ReadOnly);
         emit finished();
     });
 }
 
-MxcReply::MxcReply()
-    : d(ZeroImpl<Private>())
+MxcReply::MxcReply(DeferredFlag) {}
+
+MxcReply::MxcReply(ErrorFlag)
 {
     static const auto BadRequestPhrase = tr("Bad Request");
-    QMetaObject::invokeMethod(this, [this]() {
+    QMetaObject::invokeMethod(this, [this] {
             setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 400);
             setAttribute(QNetworkRequest::HttpReasonPhraseAttribute,
                          BadRequestPhrase);
@@ -55,7 +63,7 @@ MxcReply::MxcReply()
         }, Qt::QueuedConnection);
 }
 
-qint64 MxcReply::readData(char *data, qint64 maxSize)
+qint64 MxcReply::readData(char* data, qint64 maxSize)
 {
     if(d != nullptr && d->m_device != nullptr) {
         return d->m_device->read(data, maxSize);

diff --git a/Quotient/mxcreply.h b/Quotient/mxcreply.h
@@ -12,9 +12,16 @@ class QUOTIENT_API MxcReply : public QNetworkReply
 {
     Q_OBJECT
 public:
-    explicit MxcReply();
+    enum DeferredFlag { Deferred };
+    enum ErrorFlag { Error };
+
     explicit MxcReply(QNetworkReply* reply,
                       const EncryptedFileMetadata& fileMetadata);
+    explicit MxcReply(DeferredFlag);
+    explicit MxcReply(ErrorFlag);
+
+    void setNetworkReply(QNetworkReply* newReply,
+                         const EncryptedFileMetadata& fileMetadata = {});
 
     qint64 bytesAvailable() const override;
 
@@ -26,6 +33,6 @@ public Q_SLOTS:
 
 private:
     class Private;
-    ImplPtr<Private> d;
+    ImplPtr<Private> d = ZeroImpl<Private>();
 };
-}
+} // namespace Quotient
diff --git a/Quotient/networkaccessmanager.cpp b/Quotient/networkaccessmanager.cpp
@@ -5,6 +5,7 @@
 
 #include "logging.h"
 #include "mxcreply.h"
+#include "connection.h"
 
 #include "events/filesourceinfo.h"
 
@@ -103,6 +104,19 @@ QNetworkReply* NetworkAccessManager::createRequest(
         reply->ignoreSslErrors(d.getIgnoredSslErrors());
         return reply;
     }
+    Q_ASSERT(!url.isRelative());
+
+    const auto createImplRequest = [this, op, request, outgoingData,
+                                    url](const QUrl& baseUrl) {
+        QNetworkRequest rewrittenRequest(request);
+        rewrittenRequest.setUrl(DownloadFileJob::makeRequestUrl(baseUrl, url));
+        auto* implReply = QNetworkAccessManager::createRequest(op,
+                                                               rewrittenRequest,
+                                                               outgoingData);
+        implReply->ignoreSslErrors(d.getIgnoredSslErrors());
+        return implReply;
+    };
+
     const QUrlQuery query{ url.query() };
     const auto accountId = query.queryItemValue(QStringLiteral("user_id"));
     if (accountId.isEmpty()) {
@@ -111,30 +125,39 @@ QNetworkReply* NetworkAccessManager::createRequest(
         if (static thread_local const QSettings s;
             s.value("Network/allow_direct_media_requests"_ls).toBool()) //
         {
-            // TODO: Make the best effort with a direct unauthenticated request
-            // to the media server
-            qCWarning(NETWORK)
-                << "Direct unauthenticated mxc requests are not implemented";
-            return new MxcReply();
+            // Best effort with an unauthenticated request directly to the media
+            // homeserver (rather than via own homeserver)
+            auto* mxcReply = new MxcReply(MxcReply::Deferred);
+            // Connection class is, by the moment of this call, reentrant (it
+            // is not early on when user/room object factories and E2EE are set;
+            // but if you have an mxc link you are already well past that, most
+            // likely) so we can create and use it here, even if a connection
+            // to the same homeserver exists already.
+            auto* c = new Connection(mxcReply);
+            connect(c, &Connection::homeserverChanged, mxcReply,
+                    [mxcReply, createImplRequest, c](const QUrl& baseUrl) {
+                        mxcReply->setNetworkReply(createImplRequest(baseUrl));
+                        c->deleteLater();
+                    });
+            // Hack up a minimum "viable" MXID on the target homeserver
+            // to satisfy resolveServer()
+            c->resolveServer("@:"_ls % request.url().host());
+            return mxcReply;
         }
         qCWarning(NETWORK)
             << "No connection specified, cannot convert mxc request";
-        return new MxcReply();
+        return new MxcReply(MxcReply::Error);
     }
     const auto& baseUrl = d.getBaseUrl(accountId);
     if (!baseUrl.isValid()) {
         // Strictly speaking, it should be an assert...
         qCCritical(NETWORK) << "Homeserver for" << accountId
                             << "not found, cannot convert mxc request";
-        return new MxcReply();
+        return new MxcReply(MxcReply::Error);
     }
 
     // Convert mxc:// URL into normal http(s) for the given homeserver
-    QNetworkRequest rewrittenRequest(request);
-    rewrittenRequest.setUrl(DownloadFileJob::makeRequestUrl(baseUrl, url));
-
-    auto* implReply = QNetworkAccessManager::createRequest(op, rewrittenRequest);
-    implReply->ignoreSslErrors(d.getIgnoredSslErrors());
+    auto* implReply = createImplRequest(baseUrl);
     const auto& fileMetadata = FileMetadataMap::lookup(
         query.queryItemValue(QStringLiteral("room_id")),
         query.queryItemValue(QStringLiteral("event_id")));

diff --git a/quotest/quotest.cpp b/quotest/quotest.cpp
@@ -125,6 +125,8 @@ private slots:
     [[nodiscard]] bool checkFileSendingOutcome(const TestToken& thisTest,
                                                const QString& txnId,
                                                const QString& fileName);
+    [[nodiscard]] bool testDownload(const TestToken& thisTest,
+                                    const QUrl& mxcUrl);
     [[nodiscard]] bool checkRedactionOutcome(const QByteArray& thisTest,
                                              const QString& evtIdToRedact);
 
@@ -474,18 +476,44 @@ struct DownloadRunner {
         el.exec();
         return reply->error();
     }
+
+    static QVector<QNetworkReply::NetworkError> run(const QUrl& url, int threads)
+    {
+        return QtConcurrent::blockingMapped(QVector<int>(threads),
+                                            DownloadRunner{ url });
+    }
 };
 
-bool testDownload(const QUrl& url)
+bool TestSuite::testDownload(const TestToken& thisTest, const QUrl& mxcUrl)
 {
-    // Move out actual test from the multithreaded code
-    // to help debugging
-    auto results = QtConcurrent::blockingMapped(QVector<int> { 1, 2, 3 },
-                                                DownloadRunner { url });
-    return std::all_of(results.cbegin(), results.cend(),
-                        [](QNetworkReply::NetworkError ne) {
-                            return ne == QNetworkReply::NoError;
-                        });
+    // Testing direct media requests needs explicit allowance
+    QSettings s;
+    static constexpr auto DirectMediaRequestsSetting =
+        "Network/allow_direct_media_requests"_ls;
+    s.setValue(DirectMediaRequestsSetting, true);
+    if (const auto result = DownloadRunner::run(mxcUrl, 1);
+        result.back() != QNetworkReply::NoError) {
+        clog << "Direct media request to "
+             << mxcUrl.toDisplayString().toStdString()
+             << " was allowed but failed" << endl;
+        FAIL_TEST();
+    }
+    s.setValue(DirectMediaRequestsSetting, false);
+    if (const auto result = DownloadRunner::run(mxcUrl, 1);
+        result.back() == QNetworkReply::NoError) {
+        clog << "Direct media request to "
+             << mxcUrl.toDisplayString().toStdString()
+             << " was disallowed but succeeded" << endl;
+        FAIL_TEST();
+    }
+
+    const auto httpUrl = targetRoom->connection()->makeMediaUrl(mxcUrl);
+    const auto results = DownloadRunner::run(httpUrl, 3);
+    // Move out actual test from the multithreaded code to help debugging
+    FINISH_TEST(std::all_of(results.begin(), results.end(),
+                            [](QNetworkReply::NetworkError ne) {
+                                return ne == QNetworkReply::NoError;
+                            }));
 }
 
 bool TestSuite::checkFileSendingOutcome(const TestToken& thisTest,
@@ -519,14 +547,15 @@ bool TestSuite::checkFileSendingOutcome(const TestToken& thisTest,
                 *evt,
                 [&](const RoomMessageEvent& e) {
                     // TODO: check #366 once #368 is implemented
-                    FINISH_TEST(
-                        !e.id().isEmpty()
-                        && pendingEvents[size_t(pendingIdx)]->transactionId()
-                               == txnId
-                        && e.hasFileContent()
-                        && e.content()->fileInfo()->originalName == fileName
-                        && testDownload(targetRoom->connection()->makeMediaUrl(
-                            e.content()->fileInfo()->url())));
+                    if (e.id().isEmpty()
+                        || pendingEvents[size_t(pendingIdx)]->transactionId()
+                               != txnId
+                        || !e.hasFileContent()
+                        || e.content()->fileInfo()->originalName != fileName) {
+                        clog << "Malformed file event";
+                        FAIL_TEST();
+                    }
+                    return testDownload(thisTest, e.content()->fileInfo()->url());
                 },
                 [this, thisTest](const RoomEvent&) { FAIL_TEST(); });
         });