Move token and relpath validity checks to edit locally verification job

Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>

diff --git a/src/gui/editlocallyjob.cpp b/src/gui/editlocallyjob.cpp
index e2d7f5639a1508ce9e3353884699d61dc2b9cc1d..8f0a906740f1b946c80347a3a947a5acbd3e5fa5 100644 (file)
--- a/src/gui/editlocallyjob.cpp
+++ b/src/gui/editlocallyjob.cpp
@@ -44,9 +44,9 @@ void EditLocallyJob::startSetup()
 {
     if (_token.isEmpty() || _relPath.isEmpty() || _userId.isEmpty()) {
         qCWarning(lcEditLocallyJob) << "Could not start setup."
-                                        << "token:" << _token
-                                        << "relPath:" << _relPath
-                                        << "userId" << _userId;
+                                    << "token:" << _token
+                                    << "relPath:" << _relPath
+                                    << "userId" << _userId;
         return;
     }
 
@@ -54,35 +54,14 @@ void EditLocallyJob::startSetup()
     // verified the token
     Systray::instance()->createEditFileLocallyLoadingDialog({});
 
-    // We check the input data locally first, without modifying any state or
-    // showing any potentially misleading data to the user
-    if (!isTokenValid(_token)) {
-        qCWarning(lcEditLocallyJob) << "Edit locally request is missing a valid token, will not open file. "
-                                        << "Token received was:" << _token;
-        showError(tr("Invalid token received."), tr("Please try again."));
-        return;
-    }
-
-    if (!isRelPathValid(_relPath)) {
-        qCWarning(lcEditLocallyJob) << "Provided relPath was:" << _relPath << "which is not canonical.";
-        showError(tr("Invalid file path was provided."), tr("Please try again."));
-        return;
-    }
-
     _accountState = AccountManager::instance()->accountFromUserId(_userId);
 
-    if (!_accountState) {
-        qCWarning(lcEditLocallyJob) << "Could not find an account " << _userId << " to edit file " << _relPath << " locally.";
-        showError(tr("Could not find an account for local editing."), tr("Please try again."));
-        return;
-    }
-
     // We now ask the server to verify the token, before we again modify any
     // state or look at local files
-    startTokenRemoteCheck();
+    startCheck();
 }
 
-void EditLocallyJob::startTokenRemoteCheck()
+void EditLocallyJob::startCheck()
 {
     const auto verificationJob = new EditLocallyVerificationJob(_accountState, _relPath, _token);
     connect(verificationJob, &EditLocallyVerificationJob::error, this, &EditLocallyJob::showError);
@@ -300,43 +279,6 @@ const QString EditLocallyJob::getRelativePathParent() const
     return QStringLiteral("/");
 }
 
-bool EditLocallyJob::isTokenValid(const QString &token)
-{
-    if (token.isEmpty()) {
-        return false;
-    }
-
-    // Token is an alphanumeric string 128 chars long.
-    // Ensure that is what we received and what we are sending to the server.
-    static const QRegularExpression tokenRegex("^[a-zA-Z0-9]{128}$");
-    const auto regexMatch = tokenRegex.match(token);
-
-    return regexMatch.hasMatch();
-}
-
-bool EditLocallyJob::isRelPathValid(const QString &relPath)
-{
-    if (relPath.isEmpty()) {
-        return false;
-    }
-
-    // We want to check that the path is canonical and not relative
-    // (i.e. that it doesn't contain ../../) but we always receive
-    // a relative path, so let's make it absolute by prepending a
-    // slash
-    const auto slashPrefixedPath = prefixSlashToPath(relPath);
-
-    // Let's check that the filepath is canonical, and that the request
-    // contains no funny behaviour regarding paths
-    const auto cleanedPath = QDir::cleanPath(slashPrefixedPath);
-
-    if (cleanedPath != slashPrefixedPath) {
-        return false;
-    }
-
-    return true;
-}
-
 OCC::Folder *EditLocallyJob::findFolderForFile(const QString &relPath, const QString &userId)
 {
     if (relPath.isEmpty()) {

diff --git a/src/gui/editlocallyjob.h b/src/gui/editlocallyjob.h
index 84bb11123e88da6f98bc58309eee3a3e26f33443..32f096ab8f2508acf9101a8e8f040f29fe129323 100644 (file)
--- a/src/gui/editlocallyjob.h
+++ b/src/gui/editlocallyjob.h
@@ -37,8 +37,6 @@ public:
                             const QString &token,
                             QObject *parent = nullptr);
 
-    [[nodiscard]] static bool isTokenValid(const QString &token);
-    [[nodiscard]] static bool isRelPathValid(const QString &relPath);
     [[nodiscard]] static OCC::Folder *findFolderForFile(const QString &relPath, const QString &userId);
     [[nodiscard]] static QString prefixSlashToPath(const QString &path);
 
@@ -55,7 +53,7 @@ private slots:
     void fetchRemoteFileParentInfo();
     void startSyncBeforeOpening();
 
-    void startTokenRemoteCheck();
+    void startCheck();
     void proceedWithSetup();
     void findAfolderAndConstructPaths();
 

diff --git a/src/gui/editlocallyverificationjob.cpp b/src/gui/editlocallyverificationjob.cpp
index 861c4c09d322d5f5a9b7fb3c8465389dbf439e2b..8331a8dda8982830f55a46f1f83fff93709ae91f 100644 (file)
--- a/src/gui/editlocallyverificationjob.cpp
+++ b/src/gui/editlocallyverificationjob.cpp
@@ -14,11 +14,21 @@
 
 #include "editlocallyverificationjob.h"
 
+#include <QDir>
 #include <QLoggingCategory>
 #include <QUrlQuery>
 
 #include "libsync/networkjobs.h"
 
+namespace {
+
+QString prefixSlashToPath(const QString &path)
+{
+    return path.startsWith('/') ? path : '/' + path;
+}
+
+}
+
 namespace OCC
 {
 
@@ -35,16 +45,64 @@ EditLocallyVerificationJob::EditLocallyVerificationJob(const AccountStatePtr &ac
 {
 }
 
+bool EditLocallyVerificationJob::isTokenValid(const QString &token)
+{
+    if (token.isEmpty()) {
+        return false;
+    }
+
+    // Token is an alphanumeric string 128 chars long.
+    // Ensure that is what we received and what we are sending to the server.
+    static const QRegularExpression tokenRegex("^[a-zA-Z0-9]{128}$");
+    const auto regexMatch = tokenRegex.match(token);
+
+    return regexMatch.hasMatch();
+}
+
+bool EditLocallyVerificationJob::isRelPathValid(const QString &relPath)
+{
+    if (relPath.isEmpty()) {
+        return false;
+    }
+
+    // We want to check that the path is canonical and not relative
+    // (i.e. that it doesn't contain ../../) but we always receive
+    // a relative path, so let's make it absolute by prepending a
+    // slash
+    const auto slashPrefixedPath = prefixSlashToPath(relPath);
+
+    // Let's check that the filepath is canonical, and that the request
+    // contains no funny behaviour regarding paths
+    const auto cleanedPath = QDir::cleanPath(slashPrefixedPath);
+
+    if (cleanedPath != slashPrefixedPath) {
+        return false;
+    }
+
+    return true;
+}
+
 void EditLocallyVerificationJob::start()
 {
-    if (!_accountState || _relPath.isEmpty() || _token.isEmpty()) {
-        qCWarning(lcEditLocallyVerificationJob) << "Could not start token check."
-                                                << "accountState:" << _accountState 
-                                                << "relPath:" << _relPath 
-                                                << "token:" << _token;
+    // We check the input data locally first, without modifying any state or
+    // showing any potentially misleading data to the user
+    if (!isTokenValid(_token)) {
+        qCWarning(lcEditLocallyVerificationJob) << "Edit locally request is missing a valid token, will not open file. "
+                                                << "Token received was:" << _token;
+        emit error(tr("Invalid token received."), tr("Please try again."));
+        return;
+    }
 
-        emit error(tr("Could not start editing locally."), 
-                   tr("An error occurred trying to verify the request to edit locally."));
+    if (!isRelPathValid(_relPath)) {
+        qCWarning(lcEditLocallyVerificationJob) << "Provided relPath was:" << _relPath 
+                                                << "which is not canonical.";
+        emit error(tr("Invalid file path was provided."), tr("Please try again."));
+        return;
+    }
+
+    if (!_accountState) {
+        qCWarning(lcEditLocallyVerificationJob) << "No account found to edit file " << _relPath << " locally.";
+        emit error(tr("Could not find an account for local editing."), tr("Please try again."));
         return;
     }
 
@@ -52,7 +110,7 @@ void EditLocallyVerificationJob::start()
     const auto encodedRelPath = QUrl::toPercentEncoding(_relPath); // Sanitise the relPath
     const auto checkTokenJob = new SimpleApiJob(_accountState->account(), 
                                                 QStringLiteral("/ocs/v2.php/apps/files/api/v1/openlocaleditor/%1").arg(encodedToken));
-    const auto slashedPath = encodedRelPath.startsWith('/') ? encodedRelPath : '/' + encodedRelPath;
+    const auto slashedPath = prefixSlashToPath(encodedRelPath);
 
     QUrlQuery params;
     params.addQueryItem(QStringLiteral("path"), slashedPath);

diff --git a/src/gui/editlocallyverificationjob.h b/src/gui/editlocallyverificationjob.h
index a7ca224294b12d6ddbe00016638f94b3b3ec4647..1bd5b13955a18bc13fbad2ac9fca9da82ae5e093 100644 (file)
--- a/src/gui/editlocallyverificationjob.h
+++ b/src/gui/editlocallyverificationjob.h
@@ -30,6 +30,9 @@ public:
                                         const QString &token,
                                         QObject *const parent = nullptr);
 
+    [[nodiscard]] static bool isTokenValid(const QString &token);
+    [[nodiscard]] static bool isRelPathValid(const QString &relPath);
+
 signals:
     void error(const QString &message, const QString &informativeText);
     void finished();