From b34e13d2e2e8a8309d35ad1f79ee58bdd2cf5936 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Mon, 10 Oct 2022 11:36:50 +0200 Subject: [PATCH] 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/mxcreply.cpp | 20 +++++++--- Quotient/mxcreply.h | 13 +++++-- Quotient/networkaccessmanager.cpp | 47 +++++++++++++++++------ quotest/quotest.cpp | 63 ++++++++++++++++++++++--------- 4 files changed, 105 insertions(+), 38 deletions(-) diff --git a/Quotient/mxcreply.cpp b/Quotient/mxcreply.cpp index 2fc150ddc..2f0168f81 100644 --- 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(reply, fileMetadata.isValid() ? nullptr : reply)) { - reply->setParent(this); + setNetworkReply(reply, fileMetadata); +} + +void MxcReply::setNetworkReply(QNetworkReply* reply, + const EncryptedFileMetadata& fileMetadata) +{ + d = makeImpl(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()) +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 index 3fd5dfe24..2bab52f74 100644 --- 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 d; + ImplPtr d = ZeroImpl(); }; -} +} // namespace Quotient diff --git a/Quotient/networkaccessmanager.cpp b/Quotient/networkaccessmanager.cpp index 2a598ee73..34f5f26ba 100644 --- 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 index 7baddbb4d..9cf177780 100644 --- 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 run(const QUrl& url, int threads) + { + return QtConcurrent::blockingMapped(QVector(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 { 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(); }); });