From b34e13d2e2e8a8309d35ad1f79ee58bdd2cf5936 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Mon, 10 Oct 2022 11:36:50 +0200 Subject: [PATCH 1/4] 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(); }); }); From ccf50846fdfac7c89a48c1d295e4acaf2c82e585 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Sun, 28 May 2023 10:12:57 +0200 Subject: [PATCH 2/4] Quotest, sendFile(): actually test file contents Also: simplify the expression in FINISH_TEST. --- quotest/quotest.cpp | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/quotest/quotest.cpp b/quotest/quotest.cpp index 9cf177780..913805875 100644 --- a/quotest/quotest.cpp +++ b/quotest/quotest.cpp @@ -410,6 +410,8 @@ TEST_IMPL(sendReaction) return false; } +static constexpr auto fileContent = "Test"; + TEST_IMPL(sendFile) { auto* tf = new QTemporaryFile; @@ -417,7 +419,7 @@ TEST_IMPL(sendFile) clog << "Failed to create a temporary file" << endl; FAIL_TEST(); } - tf->write("Test"); + tf->write(fileContent); tf->close(); QFileInfo tfi { *tf }; // QFileInfo::fileName brings only the file name; QFile::fileName brings @@ -464,20 +466,23 @@ struct DownloadRunner { using result_type = QNetworkReply::NetworkError; - QNetworkReply::NetworkError operator()(int) const + result_type operator()(int) const { QEventLoop el; - QScopedPointer reply { + const QScopedPointer reply { NetworkAccessManager::instance()->get(QNetworkRequest(url)) }; QObject::connect( reply.data(), &QNetworkReply::finished, &el, [&el] { el.exit(); }, Qt::QueuedConnection); el.exec(); - return reply->error(); + return reply->error() != QNetworkReply::NoError ? reply->error() + : reply->readAll() != fileContent + ? QNetworkReply::UnknownContentError + : QNetworkReply::NoError; } - static QVector run(const QUrl& url, int threads) + static QVector run(const QUrl& url, int threads) { return QtConcurrent::blockingMapped(QVector(threads), DownloadRunner{ url }); @@ -492,7 +497,7 @@ bool TestSuite::testDownload(const TestToken& thisTest, const QUrl& mxcUrl) "Network/allow_direct_media_requests"_ls; s.setValue(DirectMediaRequestsSetting, true); if (const auto result = DownloadRunner::run(mxcUrl, 1); - result.back() != QNetworkReply::NoError) { + result.front() != QNetworkReply::NoError) { clog << "Direct media request to " << mxcUrl.toDisplayString().toStdString() << " was allowed but failed" << endl; @@ -500,20 +505,21 @@ bool TestSuite::testDownload(const TestToken& thisTest, const QUrl& mxcUrl) } s.setValue(DirectMediaRequestsSetting, false); if (const auto result = DownloadRunner::run(mxcUrl, 1); - result.back() == QNetworkReply::NoError) { + result.front() == QNetworkReply::NoError) { clog << "Direct media request to " << mxcUrl.toDisplayString().toStdString() << " was disallowed but succeeded" << endl; FAIL_TEST(); } + static constexpr auto ThreadsCount = 3; const auto httpUrl = targetRoom->connection()->makeMediaUrl(mxcUrl); - const auto results = DownloadRunner::run(httpUrl, 3); + const auto results = DownloadRunner::run(httpUrl, ThreadsCount); // 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; - })); + // NB: remove explicit template argument once entirely at Qt 6 or C++23 + FINISH_TEST(results + == QVector(ThreadsCount, + QNetworkReply::NoError)); } bool TestSuite::checkFileSendingOutcome(const TestToken& thisTest, From e33737958cd1f73fdcbfa38073fb1a683d243e3c Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Sun, 28 May 2023 10:11:57 +0200 Subject: [PATCH 3/4] Use std::atomic_flag Going through QSettings doesn't work quite well on Windows; setting it in Quotest and immediately testing through another QSettings instance inside NetworkAccessManager (in another thread) returns the previous value. In fact, there's no general guarantee for that, as almost all QSettings member functions are reentrant but not thread-safe. The flag is only initialised from QSettings once at the first NAM creation, and its changes are only propagated to QSettings but never read again. Implication: changes in QSettings files when the client is running will only be reflected at the next run (or may be overwritten altogether), so just don't change settings files while the client is running, ok? --- Quotient/networkaccessmanager.cpp | 40 ++++++++++++++++++++++++++++--- Quotient/networkaccessmanager.h | 4 +++- quotest/quotest.cpp | 7 ++---- 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/Quotient/networkaccessmanager.cpp b/Quotient/networkaccessmanager.cpp index 34f5f26ba..0e92023a6 100644 --- a/Quotient/networkaccessmanager.cpp +++ b/Quotient/networkaccessmanager.cpp @@ -21,6 +21,10 @@ using namespace Quotient; namespace { + +static constexpr auto DirectMediaRequestsSetting = + "Network/allow_direct_media_requests"_ls; + class { public: void addBaseUrl(const QString& accountId, const QUrl& baseUrl) @@ -47,15 +51,42 @@ class { { return QReadLocker{ &namLock }, ignoredSslErrors; } + void allowDirectMediaRequests(bool allow) + { + if (allow) + directMediaRequestsAreAllowed.test_and_set(); + else + directMediaRequestsAreAllowed.clear(); + } + bool directMediaRequestsAllowed() const + { + return directMediaRequestsAreAllowed.test(); + } private: mutable QReadWriteLock namLock{}; QHash baseUrls{}; QList ignoredSslErrors{}; + // This one is small enough to be atomic and not need a read-write lock + std::atomic_flag directMediaRequestsAreAllowed{ false }; } d; +std::once_flag directMediaRequestsInitFlag; + } // anonymous namespace +void NetworkAccessManager::allowDirectMediaRequests(bool allow, bool permanent) +{ + d.allowDirectMediaRequests(allow); + if (permanent) + QSettings().setValue(DirectMediaRequestsSetting, allow); +} + +bool NetworkAccessManager::directMediaRequestsAllowed() +{ + return d.directMediaRequestsAllowed(); +} + void NetworkAccessManager::addBaseUrl(const QString& accountId, const QUrl& homeserver) { @@ -85,6 +116,11 @@ void NetworkAccessManager::clearIgnoredSslErrors() NetworkAccessManager* NetworkAccessManager::instance() { + // Initialise direct media requests allowance at the very first NAM creation + std::call_once(directMediaRequestsInitFlag, [] { + NetworkAccessManager::allowDirectMediaRequests( + QSettings().value(DirectMediaRequestsSetting).toBool()); + }); thread_local auto* nam = [] { auto* namInit = new NetworkAccessManager(); connect(QThread::currentThread(), &QThread::finished, namInit, @@ -122,9 +158,7 @@ QNetworkReply* NetworkAccessManager::createRequest( if (accountId.isEmpty()) { // Using QSettings here because Quotient::NetworkSettings // doesn't provide multi-threading guarantees - if (static thread_local const QSettings s; - s.value("Network/allow_direct_media_requests"_ls).toBool()) // - { + if (directMediaRequestsAllowed()) { // Best effort with an unauthenticated request directly to the media // homeserver (rather than via own homeserver) auto* mxcReply = new MxcReply(MxcReply::Deferred); diff --git a/Quotient/networkaccessmanager.h b/Quotient/networkaccessmanager.h index 41c108d59..cc71ac315 100644 --- a/Quotient/networkaccessmanager.h +++ b/Quotient/networkaccessmanager.h @@ -12,7 +12,9 @@ namespace Quotient { class QUOTIENT_API NetworkAccessManager : public QNetworkAccessManager { Q_OBJECT public: - using QNetworkAccessManager::QNetworkAccessManager; + static void allowDirectMediaRequests(bool allow = true, + bool permanent = true); + static bool directMediaRequestsAllowed(); static void addBaseUrl(const QString& accountId, const QUrl& homeserver); static void dropBaseUrl(const QString& accountId); diff --git a/quotest/quotest.cpp b/quotest/quotest.cpp index 913805875..334d64287 100644 --- a/quotest/quotest.cpp +++ b/quotest/quotest.cpp @@ -492,10 +492,7 @@ struct DownloadRunner { bool TestSuite::testDownload(const TestToken& thisTest, const QUrl& mxcUrl) { // Testing direct media requests needs explicit allowance - QSettings s; - static constexpr auto DirectMediaRequestsSetting = - "Network/allow_direct_media_requests"_ls; - s.setValue(DirectMediaRequestsSetting, true); + NetworkAccessManager::allowDirectMediaRequests(true, false); if (const auto result = DownloadRunner::run(mxcUrl, 1); result.front() != QNetworkReply::NoError) { clog << "Direct media request to " @@ -503,7 +500,7 @@ bool TestSuite::testDownload(const TestToken& thisTest, const QUrl& mxcUrl) << " was allowed but failed" << endl; FAIL_TEST(); } - s.setValue(DirectMediaRequestsSetting, false); + NetworkAccessManager::allowDirectMediaRequests(false, false); if (const auto result = DownloadRunner::run(mxcUrl, 1); result.front() == QNetworkReply::NoError) { clog << "Direct media request to " From 3080875ff75b176984b0d9958b60a24d76e749f8 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Sun, 28 May 2023 11:35:46 +0200 Subject: [PATCH 4/4] Default-initialise std::atomic_flag std::atomic_flag(bool) is not a standard constructor, and MSVC doesn't have it, for one. --- Quotient/networkaccessmanager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Quotient/networkaccessmanager.cpp b/Quotient/networkaccessmanager.cpp index 0e92023a6..1fe9833ec 100644 --- a/Quotient/networkaccessmanager.cpp +++ b/Quotient/networkaccessmanager.cpp @@ -68,7 +68,7 @@ class { QHash baseUrls{}; QList ignoredSslErrors{}; // This one is small enough to be atomic and not need a read-write lock - std::atomic_flag directMediaRequestsAreAllowed{ false }; + std::atomic_flag directMediaRequestsAreAllowed{}; } d; std::once_flag directMediaRequestsInitFlag;