Verify token before checking disk or modifying state

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

src/gui/application.cpp

@@ -777,12 +777,14 @@ void Application::handleEditLocally(const QUrl &url) const
 
     // We need to make sure the handler sticks around until it is finished
     const auto editLocallyHandler = new EditLocallyHandler(userId, fileRemotePath, token);
-    if (editLocallyHandler->ready()) {
+    const auto editLocallyHandlerDeleter = [editLocallyHandler]{ delete editLocallyHandler; };
-        connect(editLocallyHandler, &EditLocallyHandler::finished, this, [&editLocallyHandler] { delete editLocallyHandler; });
+    connect(editLocallyHandler, &EditLocallyHandler::error, this, editLocallyHandlerDeleter);
-        editLocallyHandler->startEditLocally();
+    connect(editLocallyHandler, &EditLocallyHandler::fileOpened, this, editLocallyHandlerDeleter);
-    } else {
+
-        delete editLocallyHandler;
+    connect(editLocallyHandler, &EditLocallyHandler::setupFinished,
-    }
+            editLocallyHandler, [editLocallyHandler]{ editLocallyHandler->startEditLocally(); });
+    editLocallyHandler->startSetup();
+
 }
 
 QString substLang(const QString &lang)

src/gui/editlocallyhandler.cpp

@@ -35,36 +35,101 @@ EditLocallyHandler::EditLocallyHandler(const QString &userId,
                                        const QString &token,
                                        QObject *parent)
     : QObject{parent}
+    , _userId(userId)
     , _relPath(relPath)
     , _token(token)
 {
-    _accountState = AccountManager::instance()->accountFromUserId(userId);
+}
 
-    if (!_accountState) {
+void EditLocallyHandler::startSetup()
-        qCWarning(lcEditLocallyHandler) << "Could not find an account " << userId << " to edit file " << relPath << " locally.";
+{
-        showError(tr("Could not find an account for local editing"), userId);
+    if (_token.isEmpty() || _relPath.isEmpty() || _userId.isEmpty()) {
+        qCWarning(lcEditLocallyHandler) << "Could not start setup."
+                                        << "token:" << _token
+                                        << "relPath:" << _relPath
+                                        << "userId" << _userId;
         return;
     }
 
-    if (!isTokenValid(token)) {
+    // We check the input data locally first, without modifying any state or
-        qCWarning(lcEditLocallyHandler) << "Edit locally request is missing a valid token, will not open file. Token received was:" << token;
+    // showing any potentially misleading data to the user
+    if (!isTokenValid(_token)) {
+        qCWarning(lcEditLocallyHandler) << "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)) {
+    if (!isRelPathValid(_relPath)) {
-        qCWarning(lcEditLocallyHandler) << "Provided relPath was:" << relPath << "which is not canonical.";
+        qCWarning(lcEditLocallyHandler) << "Provided relPath was:" << _relPath << "which is not canonical.";
         showError(tr("Invalid file path was provided."), tr("Please try again."));
         return;
     }
 
-    const auto foundFiles = FolderMan::instance()->findFileInLocalFolders(relPath, _accountState->account());
+    _accountState = AccountManager::instance()->accountFromUserId(_userId);
+
+    if (!_accountState) {
+        qCWarning(lcEditLocallyHandler) << "Could not find an account " << _userId << " to edit file " << _relPath << " locally.";
+        showError(tr("Could not find an account for local editing."), _userId);
+        return;
+    }
+
+    // We now ask the server to verify the token, before we again modify any
+    // state or look at local files
+    startTokenRemoteCheck();
+}
+
+void EditLocallyHandler::startTokenRemoteCheck()
+{
+    if (!_accountState || _relPath.isEmpty() || _token.isEmpty()) {
+        qCWarning(lcEditLocallyHandler) << "Could not start token check."
+                                        << "accountState:" << _accountState
+                                        << "relPath:" << _relPath
+                                        << "token:" << _token;
+        return;
+    }
+
+    const auto encodedToken = QString::fromUtf8(QUrl::toPercentEncoding(_token)); // Sanitise the token
+    const auto encodedRelPath = QUrl::toPercentEncoding(_relPath); // Sanitise the relPath
+    const auto checkEditLocallyToken = new SimpleApiJob(_accountState->account(), QStringLiteral("/ocs/v2.php/apps/files/api/v1/openlocaleditor/%1").arg(encodedToken));
+
+    QUrlQuery params;
+    params.addQueryItem(QStringLiteral("path"), prefixSlashToPath(encodedRelPath));
+    checkEditLocallyToken->addQueryParams(params);
+    checkEditLocallyToken->setVerb(SimpleApiJob::Verb::Post);
+    connect(checkEditLocallyToken, &SimpleApiJob::resultReceived, this, &EditLocallyHandler::remoteTokenCheckResultReceived);
+
+    checkEditLocallyToken->start();
+}
+
+void EditLocallyHandler::remoteTokenCheckResultReceived(const int statusCode)
+{
+    qCInfo(lcEditLocallyHandler) << "token check result" << statusCode;
+
+    constexpr auto HTTP_OK_CODE = 200;
+    _tokenVerified = statusCode == HTTP_OK_CODE;
+
+    if (!_tokenVerified) {
+        showError(tr("Could not validate the request to open a file from server."), _relPath);
+    }
+
+    proceedWithSetup();
+}
+
+void EditLocallyHandler::proceedWithSetup()
+{
+    if (!_tokenVerified) {
+        qCWarning(lcEditLocallyHandler) << "Could not proceed with setup as token is not verified.";
+        return;
+    }
+
+    const auto foundFiles = FolderMan::instance()->findFileInLocalFolders(_relPath, _accountState->account());
 
     if (foundFiles.isEmpty()) {
-        if (isRelPathExcluded(relPath)) {
+        if (isRelPathExcluded(_relPath)) {
-            showError(tr("Could not find a file for local editing. Make sure it is not excluded via selective sync."), relPath);
+            showError(tr("Could not find a file for local editing. Make sure it is not excluded via selective sync."), _relPath);
         } else {
-            showError(tr("Could not find a file for local editing. Make sure its path is valid and it is synced locally."), relPath);
+            showError(tr("Could not find a file for local editing. Make sure its path is valid and it is synced locally."), _relPath);
         }
         return;
     }
@@ -73,24 +138,19 @@ EditLocallyHandler::EditLocallyHandler(const QString &userId,
     _folderForFile = FolderMan::instance()->folderForPath(_localFilePath);
 
     if (!_folderForFile) {
-        showError(tr("Could not find a folder to sync."), relPath);
+        showError(tr("Could not find a folder to sync."), _relPath);
         return;
     }
 
-    const auto relPathSplit = relPath.split(QLatin1Char('/'));
+    const auto relPathSplit = _relPath.split(QLatin1Char('/'));
-    if (relPathSplit.size() == 0) {
+    if (relPathSplit.isEmpty()) {
-        showError(tr("Could not find a file for local editing. Make sure its path is valid and it is synced locally."), relPath);
+        showError(tr("Could not find a file for local editing. Make sure its path is valid and it is synced locally."), _relPath);
         return;
     }
 
     _fileName = relPathSplit.last();
 
-    _ready = true;
+    Q_EMIT setupFinished();
-}
-
-bool EditLocallyHandler::ready() const
-{
-    return _ready;
 }
 
 QString EditLocallyHandler::prefixSlashToPath(const QString &path)
@@ -165,11 +225,12 @@ bool EditLocallyHandler::isRelPathExcluded(const QString &relPath)
     return true;
 }
 
-void EditLocallyHandler::showError(const QString &message, const QString &informativeText) const
+void EditLocallyHandler::showError(const QString &message, const QString &informativeText)
 {
     showErrorNotification(message, informativeText);
     // to make sure the error is not missed, show a message box in addition
     showErrorMessageBox(message, informativeText);
+    Q_EMIT error(message, informativeText);
 }
 
 void EditLocallyHandler::showErrorNotification(const QString &message, const QString &informativeText) const
@@ -203,36 +264,16 @@ void EditLocallyHandler::showErrorMessageBox(const QString &message, const QStri
 
 void EditLocallyHandler::startEditLocally()
 {
-    if (!_ready) {
+    if (_fileName.isEmpty() || _localFilePath.isEmpty() || !_folderForFile) {
+        qCWarning(lcEditLocallyHandler) << "Could not start to edit locally."
+                                        << "fileName:" << _fileName
+                                        << "localFilePath:" << _localFilePath
+                                        << "folderForFile:" << _folderForFile;
         return;
     }
 
     Systray::instance()->createEditFileLocallyLoadingDialog(_fileName);
 
-    const auto encodedToken = QString::fromUtf8(QUrl::toPercentEncoding(_token)); // Sanitise the token
-    const auto encodedRelPath = QUrl::toPercentEncoding(_relPath); // Sanitise the relPath
-    const auto checkEditLocallyToken = new SimpleApiJob(_accountState->account(), QStringLiteral("/ocs/v2.php/apps/files/api/v1/openlocaleditor/%1").arg(encodedToken));
-
-    QUrlQuery params;
-    params.addQueryItem(QStringLiteral("path"), prefixSlashToPath(_relPath));
-    checkEditLocallyToken->addQueryParams(params);
-    checkEditLocallyToken->setVerb(SimpleApiJob::Verb::Post);
-    connect(checkEditLocallyToken, &SimpleApiJob::resultReceived, this, &EditLocallyHandler::remoteTokenCheckFinished);
-
-    checkEditLocallyToken->start();
-}
-
-void EditLocallyHandler::remoteTokenCheckFinished(const int statusCode)
-{
-    constexpr auto HTTP_OK_CODE = 200;
-    if (statusCode != HTTP_OK_CODE) {
-        Systray::instance()->destroyEditFileLocallyLoadingDialog();
-
-        showError(tr("Could not validate the request to open a file from server."), _relPath);
-        qCInfo(lcEditLocallyHandler) << "token check result" << statusCode;
-        return;
-    }
-
     _folderForFile->startSync();
     const auto syncFinishedConnection = connect(_folderForFile, &Folder::syncFinished,
                                                 this, &EditLocallyHandler::folderSyncFinished);
@@ -248,6 +289,10 @@ void EditLocallyHandler::folderSyncFinished(const OCC::SyncResult &result)
 
 void EditLocallyHandler::disconnectSyncFinished() const
 {
+    if(_localFilePath.isEmpty()) {
+        return;
+    }
+
     if (const auto existingConnection = editLocallySyncFinishedConnections.value(_localFilePath)) {
         disconnect(existingConnection);
         editLocallySyncFinishedConnections.remove(_localFilePath);
@@ -256,6 +301,11 @@ void EditLocallyHandler::disconnectSyncFinished() const
 
 void EditLocallyHandler::openFile()
 {
+    if(_localFilePath.isEmpty()) {
+        qCWarning(lcEditLocallyHandler) << "Could not edit locally. Invalid local file path.";
+        return;
+    }
+
     const auto localFilePath = _localFilePath;
     // In case the VFS mode is enabled and a file is not yet hydrated, we must call QDesktopServices::openUrl
     // from a separate thread, or, there will be a freeze. To avoid searching for a specific folder and checking
@@ -265,7 +315,7 @@ void EditLocallyHandler::openFile()
         Systray::instance()->destroyEditFileLocallyLoadingDialog();
     });
 
-    Q_EMIT finished();
+    Q_EMIT fileOpened();
 }
 
 }

src/gui/editlocallyhandler.h

@@ -36,30 +36,34 @@ public:
     [[nodiscard]] static bool isRelPathExcluded(const QString &relPath);
     [[nodiscard]] static QString prefixSlashToPath(const QString &path);
 
-    [[nodiscard]] bool ready() const;
-
 signals:
-    void finished();
+    void setupFinished();
+    void error(const QString &message, const QString &informativeText);
+    void fileOpened();
 
 public slots:
+    void startSetup();
     void startEditLocally();
     void startTokenRemoteCheck();
 
 private slots:
-    void showError(const QString &message, const QString &informativeText) const;
+    void proceedWithSetup();
+
+    void showError(const QString &message, const QString &informativeText);
     void showErrorNotification(const QString &message, const QString &informativeText) const;
     void showErrorMessageBox(const QString &message, const QString &informativeText) const;
 
-    void remoteTokenCheckFinished(const int statusCode);
+    void remoteTokenCheckResultReceived(const int statusCode);
     void folderSyncFinished(const OCC::SyncResult &result);
 
     void disconnectSyncFinished() const;
     void openFile();
 
 private:
-    bool _ready = false;
+    bool _tokenVerified = false;
 
     AccountStatePtr _accountState;
+    QString _userId;
     QString _relPath;
     QString _token;