Move URL creation to Room/Connection; use query instead of fragment

The query is easier to manipulate; and the original mxc URL is not used
for the real network request anyway.

6 files changed, 80 insertions, 45 deletions

diff --git a/lib/connection.cpp b/lib/connection.cpp
index 222c3b71..51946b2f 100644
--- a/lib/connection.cpp
+++ b/lib/connection.cpp
@@ -836,6 +836,15 @@ inline auto splitMediaId(const QString& mediaId)
     return idParts;
 }
 
+QUrl Connection::makeMediaUrl(QUrl mxcUrl) const
+{
+    Q_ASSERT(mxcUrl.scheme() == "mxc");
+    QUrlQuery q(mxcUrl.query());
+    q.addQueryItem(QStringLiteral("user_id"), userId());
+    mxcUrl.setQuery(q);
+    return mxcUrl;
+}
+
 MediaThumbnailJob* Connection::getThumbnail(const QString& mediaId,
                                             QSize requestedSize,
                                             RunningPolicy policy)
diff --git a/lib/connection.h b/lib/connection.h
index ecbb1a19..1a6ca9b0 100644
--- a/lib/connection.h
+++ b/lib/connection.h
@@ -529,6 +529,8 @@ public Q_SLOTS:
     void stopSync();
     QString nextBatchToken() const;
 
+    Q_INVOKABLE QUrl makeMediaUrl(QUrl mxcUrl) const;
+
     virtual MediaThumbnailJob*
     getThumbnail(const QString& mediaId, QSize requestedSize,
                  RunningPolicy policy = BackgroundRequest);
diff --git a/lib/networkaccessmanager.cpp b/lib/networkaccessmanager.cpp
index dc1c139c..3b0dc92b 100644
--- a/lib/networkaccessmanager.cpp
+++ b/lib/networkaccessmanager.cpp
@@ -3,24 +3,43 @@
 
 #include "networkaccessmanager.h"
 
-#include <QtCore/QCoreApplication>
-#include <QtNetwork/QNetworkReply>
-#include <QtCore/QThreadStorage>
+#include "connection.h"
+#include "room.h"
 #include "accountregistry.h"
 #include "mxcreply.h"
-#include "connection.h"
 
-#include "room.h"
+#include <QtCore/QCoreApplication>
+#include <QtCore/QThreadStorage>
+#include <QtCore/QSettings>
+#include <QtNetwork/QNetworkReply>
 
 using namespace Quotient;
 
 class NetworkAccessManager::Private {
 public:
+    explicit Private(NetworkAccessManager* q)
+        : q(q)
+    {}
+
+    QNetworkReply* createImplRequest(Operation op,
+                                     const QNetworkRequest& outerRequest,
+                                     Connection* connection)
+    {
+        Q_ASSERT(outerRequest.url().scheme() == "mxc");
+        QNetworkRequest r(outerRequest);
+        r.setUrl(QUrl(QStringLiteral("%1/_matrix/media/r0/download/%2")
+                          .arg(connection->homeserver().toString(),
+                               outerRequest.url().authority()
+                                   + outerRequest.url().path())));
+        return q->createRequest(op, r);
+    }
+
+    NetworkAccessManager* q;
     QList<QSslError> ignoredSslErrors;
 };
 
 NetworkAccessManager::NetworkAccessManager(QObject* parent)
-    : QNetworkAccessManager(parent), d(std::make_unique<Private>())
+    : QNetworkAccessManager(parent), d(std::make_unique<Private>(this))
 {}
 
 QList<QSslError> NetworkAccessManager::ignoredSslErrors() const
@@ -54,6 +73,8 @@ static NetworkAccessManager* createNam()
 NetworkAccessManager* NetworkAccessManager::instance()
 {
     static QThreadStorage<NetworkAccessManager*> storage;
+    // FIXME: createNam() returns an object parented to
+    // QCoreApplication::instance() that lives in the main thread
     if(!storage.hasLocalData()) {
         storage.setLocalData(createNam());
     }
@@ -65,38 +86,38 @@ NetworkAccessManager::~NetworkAccessManager() = default;
 QNetworkReply* NetworkAccessManager::createRequest(
     Operation op, const QNetworkRequest& request, QIODevice* outgoingData)
 {
-    if(request.url().scheme() == QStringLiteral("mxc")) {
-        const auto fragment = request.url().fragment();
-        const auto fragmentParts = fragment.split(QLatin1Char('/'));
-        const auto mediaId = request.url().toString(QUrl::RemoveScheme | QUrl::RemoveFragment);
-        if (fragmentParts.size() == 3) {
-            auto connection = AccountRegistry::instance().get(QUrl::fromPercentEncoding(fragmentParts[0].toLatin1()));
-            if(!connection) {
-                qWarning() << "Connection not found";
+    const auto& mxcUrl = request.url();
+    if (mxcUrl.scheme() == "mxc") {
+        const QUrlQuery query(mxcUrl.query());
+        const auto accountId = query.queryItemValue(QStringLiteral("user_id"));
+        if (accountId.isEmpty()) {
+            // Using QSettings here because Quotient::NetworkSettings
+            // doesn't provide multithreading guarantees
+            static thread_local QSettings s;
+            if (!s.value("Network/allow_direct_media_requests").toBool()) {
                 return new MxcReply();
             }
-            auto room = connection->room(fragmentParts[1]);
-            if(!room) {
-                qWarning() << "Room not found";
+            // TODO: Make the best effort with a direct unauthenticated request
+            // to the media server
+        } else {
+            auto* const connection = AccountRegistry::instance().get(accountId);
+            if (!connection) {
+                qCWarning(NETWORK) << "Connection not found";
                 return new MxcReply();
             }
-            QNetworkRequest r(request);
-            r.setUrl(QUrl(QStringLiteral("%1/_matrix/media/r0/download/%2").arg(connection->homeserver().toString(), mediaId)));
-            auto reply = createRequest(QNetworkAccessManager::GetOperation, r);
-            return new MxcReply(reply, room, QUrl::fromPercentEncoding(fragmentParts[2].toLatin1()));
-        } else if(fragmentParts.size() == 1) {
-            auto connection = AccountRegistry::instance().get(fragment);
-            if(!connection) {
-                qWarning() << "Connection not found";
-                return new MxcReply();
+            const auto roomId = query.queryItemValue(QStringLiteral("room_id"));
+            if (!roomId.isEmpty()) {
+                auto room = connection->room(roomId);
+                if (!room) {
+                    qCWarning(NETWORK) << "Room not found";
+                    return new MxcReply();
+                }
+                return new MxcReply(
+                    d->createImplRequest(op, request, connection), room,
+                    query.queryItemValue(QStringLiteral("event_id")));
             }
-            QNetworkRequest r(request);
-            r.setUrl(QUrl(QStringLiteral("%1/_matrix/media/r0/download/%2").arg(connection->homeserver().toString(), mediaId)));
-            auto reply = createRequest(QNetworkAccessManager::GetOperation, r);
-            return new MxcReply(reply);
-        } else {
-            qWarning() << "Invalid request";
-            return new MxcReply();
+            return new MxcReply(
+                d->createImplRequest(op, request, connection));
         }
     }
     auto reply = QNetworkAccessManager::createRequest(op, request, outgoingData);
@@ -110,13 +131,3 @@ QStringList NetworkAccessManager::supportedSchemesImplementation() const
     schemes += QStringLiteral("mxc");
     return schemes;
 }
-
-QUrl NetworkAccessManager::urlForRoomEvent(Room *room, const QString &eventId, const QString &mediaId)
-{
-    return QUrl(QStringLiteral("mxc:%1#%2/%3/%4").arg(mediaId, QString(QUrl::toPercentEncoding(room->connection()->userId())), room->id(), QString(QUrl::toPercentEncoding(eventId))));
-}
-
-QUrl NetworkAccessManager::urlForFile(Connection *connection, const QString &mediaId)
-{
-    return QUrl(QStringLiteral("mxc:%1#%2").arg(mediaId, QString(QUrl::toPercentEncoding(connection->userId()))));
-}
\ No newline at end of file
diff --git a/lib/networkaccessmanager.h b/lib/networkaccessmanager.h
index 5d262f98..87bc12a1 100644
--- a/lib/networkaccessmanager.h
+++ b/lib/networkaccessmanager.h
@@ -25,8 +25,7 @@ public:
 
 public Q_SLOTS:
     QStringList supportedSchemesImplementation() const;
-    QUrl urlForRoomEvent(Room *room, const QString &eventId, const QString &mediaId);
-    QUrl urlForFile(Connection *connection, const QString &mediaId);
+
 private:
     QNetworkReply* createRequest(Operation op, const QNetworkRequest& request,
                                  QIODevice* outgoingData = Q_NULLPTR) override;
diff --git a/lib/room.cpp b/lib/room.cpp
index 890da13d..72b37f62 100644
--- a/lib/room.cpp
+++ b/lib/room.cpp
@@ -1116,6 +1116,17 @@ QList<User*> Room::directChatUsers() const
     return connection()->directChatUsers(this);
 }
 
+QUrl Room::makeMediaUrl(const QString& eventId, const QUrl& mxcUrl) const
+{
+    auto url = connection()->makeMediaUrl(mxcUrl);
+    QUrlQuery q(url.query());
+    Q_ASSERT(q.hasQueryItem("user_id"));
+    q.addQueryItem("room_id", id());
+    q.addQueryItem("event_id", eventId);
+    url.setQuery(q);
+    return url;
+}
+
 QString safeFileName(QString rawName)
 {
     return rawName.replace(QRegularExpression("[/\\<>|\"*?:]"), "_");
diff --git a/lib/room.h b/lib/room.h
index d3a7466d..9daca076 100644
--- a/lib/room.h
+++ b/lib/room.h
@@ -458,6 +458,9 @@ public:
     /// Get the list of users this room is a direct chat with
     QList<User*> directChatUsers() const;
 
+    Q_INVOKABLE QUrl makeMediaUrl(const QString& eventId,
+                                  const QUrl &mxcUrl) const;
+
     Q_INVOKABLE QUrl urlToThumbnail(const QString& eventId) const;
     Q_INVOKABLE QUrl urlToDownload(const QString& eventId) const;