From: Claudio Cambra Date: Wed, 26 Oct 2022 17:10:21 +0000 (+0200) Subject: Refactor large editFileLocally FolderMan method into smaller, clearer separate methods X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~11^2~177^2~7 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=224621bb855e7be0c7b638eb904d2a79d8e237ca;p=nextcloud-desktop.git Refactor large editFileLocally FolderMan method into smaller, clearer separate methods Signed-off-by: Claudio Cambra --- diff --git a/src/gui/accountmanager.cpp b/src/gui/accountmanager.cpp index cce3621e7..331db6297 100644 --- a/src/gui/accountmanager.cpp +++ b/src/gui/accountmanager.cpp @@ -369,6 +369,21 @@ AccountStatePtr AccountManager::account(const QString &name) return it != _accounts.cend() ? *it : AccountStatePtr(); } +AccountStatePtr AccountManager::accountFromUserId(const QString &id) const +{ + for (const auto &account : accounts()) { + const auto isUserIdWithPort = id.split(QLatin1Char(':')).size() > 1; + const auto port = isUserIdWithPort ? account->account()->url().port() : -1; + const auto portString = (port > 0 && port != 80 && port != 443) ? QStringLiteral(":%1").arg(port) : QStringLiteral(""); + const QString davUserId = QStringLiteral("%1@%2").arg(account->account()->davUser(), account->account()->url().host()) + portString; + + if (davUserId == id) { + return account; + } + } + return {}; +} + AccountState *AccountManager::addAccount(const AccountPtr &newAccount) { auto id = newAccount->id(); diff --git a/src/gui/accountmanager.h b/src/gui/accountmanager.h index 2440f91a9..04ce8e57b 100644 --- a/src/gui/accountmanager.h +++ b/src/gui/accountmanager.h @@ -65,6 +65,12 @@ public: */ AccountStatePtr account(const QString &name); + /** + * Return the account state pointer for an account from its id + */ + + [[nodiscard]] AccountStatePtr accountFromUserId(const QString &id) const; + /** * Delete the AccountState */ diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp index 44bc904d6..045c50e26 100644 --- a/src/gui/folderman.cpp +++ b/src/gui/folderman.cpp @@ -1439,55 +1439,79 @@ void FolderMan::setDirtyNetworkLimits() void FolderMan::editFileLocally(const QString &userId, const QString &relPath, const QString &token) { - const auto showError = [this](const OCC::AccountStatePtr accountState, const QString &errorMessage, const QString &subject) { - if (accountState && accountState->account()) { - const auto foundFolder = std::find_if(std::cbegin(map()), std::cend(map()), [accountState](const auto &folder) { - return accountState->account()->davUrl() == folder->remoteUrl(); - }); - - if (foundFolder != std::cend(map())) { - (*foundFolder)->syncEngine().addErrorToGui(SyncFileItem::SoftError, errorMessage, subject); - } - } + const auto accountFound = AccountManager::instance()->accountFromUserId(userId); - // to make sure the error is not missed, show a message box in addition - const auto messageBox = new QMessageBox; - messageBox->setAttribute(Qt::WA_DeleteOnClose); - messageBox->setText(errorMessage); - messageBox->setInformativeText(subject); - messageBox->setIcon(QMessageBox::Warning); - messageBox->addButton(QMessageBox::StandardButton::Ok); - messageBox->show(); - messageBox->activateWindow(); - messageBox->raise(); - }; + if (!accountFound) { + qCWarning(lcFolderMan) << "Could not find an account " << userId << " to edit file " << relPath << " locally."; + showEditLocallyError(accountFound, tr("Could not find an account for local editing"), userId); + return; + } - if (token.isEmpty()) { - qCWarning(lcFolderMan) << "Edit locally request is missing a valid token. Impossible to open the file."; - showError({}, tr("Edit locally request is not valid. Opening the file is forbidden."), userId); + if (!isEditLocallyTokenValid(token)) { + qCWarning(lcFolderMan) << "Edit locally request is missing a valid token, will not open file. Token received was:" << token; + showEditLocallyError(accountFound, tr("Invalid token received."), tr("Please try again.")); return; } - const auto accountFound = [&userId]() { - for (const auto &account : AccountManager::instance()->accounts()) { - const auto isUserIdWithPort = userId.split(QLatin1Char(':')).size() > 1; - const auto port = isUserIdWithPort ? account->account()->url().port() : -1; - const auto portString = (port > 0 && port != 80 && port != 443) ? QStringLiteral(":%1").arg(port) : QStringLiteral(""); - const QString davUserId = QStringLiteral("%1@%2").arg(account->account()->davUser(), account->account()->url().host()) + portString; + if (!isEditLocallyRelPathValid(relPath)) { + qCWarning(lcFolderMan) << "Provided relPath was:" << relPath << "which is not canonical."; + showEditLocallyError(accountFound, tr("Invalid file path was provided."), tr("Please try again.")); + return; + } - if (davUserId == userId) { - return account; - } + const auto foundFiles = findFileInLocalFolders(relPath, accountFound->account()); + + if (foundFiles.isEmpty()) { + if (isEditLocallyRelPathExcluded(relPath)) { + showEditLocallyError(accountFound, tr("Could not find a file for local editing. Make sure it is not excluded via selective sync."), relPath); + } else { + showEditLocallyError(accountFound, tr("Could not find a file for local editing. Make sure its path is valid and it is synced locally."), relPath); } - return AccountStatePtr{}; - }(); + return; + } - if (!accountFound) { - qCWarning(lcFolderMan) << "Could not find an account " << userId << " to edit file " << relPath << " locally."; - showError(accountFound, tr("Could not find an account for local editing"), userId); + const auto localFilePath = foundFiles.first(); + const auto folderForFile = folderForPath(localFilePath); + + if (!folderForFile) { + showEditLocallyError(accountFound, tr("Could not find a folder to sync."), relPath); return; } + const auto relPathSplit = relPath.split(QLatin1Char('/')); + if (relPathSplit.size() == 0) { + showEditLocallyError(accountFound, tr("Could not find a file for local editing. Make sure its path is valid and it is synced locally."), relPath); + return; + } + + startEditLocally(accountFound, relPath, token, relPathSplit.last(), localFilePath, folderForFile); +} + +bool FolderMan::isEditLocallyTokenValid(const QString &token) const +{ + 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. + const QRegularExpression tokenRegex("^[a-zA-Z0-9]{128}$"); + const auto regexMatch = tokenRegex.match(token); + + // Means invalid token type received, be cautious with bad token + if(!regexMatch.hasMatch()) { + return false; + } + + return true; +} + +bool FolderMan::isEditLocallyRelPathValid(const QString &relPath) const +{ + 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 @@ -1503,93 +1527,132 @@ void FolderMan::editFileLocally(const QString &userId, const QString &relPath, c const auto cleanedPath = QDir::cleanPath(slashPrefixedPath); if (cleanedPath != slashPrefixedPath) { - qCWarning(lcFolderMan) << "Provided relPath was:" << relPath - << "which is not canonical (cleaned path was:" << cleanedPath << ")"; - showError(accountFound, tr("Invalid file path was provided."), tr("Please try again.")); - return; + return false; } - const auto foundFiles = findFileInLocalFolders(relPath, accountFound->account()); + return true; +} - if (foundFiles.isEmpty()) { - for (const auto &folder : map()) { - bool result = false; - const auto excludedThroughSelectiveSync = folder->journalDb()->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &result); - for (const auto &excludedPath : excludedThroughSelectiveSync) { - if (relPath.startsWith(excludedPath)) { - showError(accountFound, tr("Could not find a file for local editing. Make sure it is not excluded via selective sync."), relPath); - return; - } +bool FolderMan::isEditLocallyRelPathExcluded(const QString &relPath) const +{ + if (relPath.isEmpty()) { + return true; + } + + for (const auto &folder : map()) { + bool result = false; + const auto excludedThroughSelectiveSync = folder->journalDb()->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &result); + for (const auto &excludedPath : excludedThroughSelectiveSync) { + if (relPath.startsWith(excludedPath)) { + return false; } } - - showError(accountFound, tr("Could not find a file for local editing. Make sure its path is valid and it is synced locally."), relPath); - return; } - const auto localFilePath = foundFiles.first(); - const auto folderForFile = folderForPath(localFilePath); + return true; +} - if (!folderForFile) { - showError(accountFound, tr("Could not find a folder to sync."), relPath); - return; - } +void FolderMan::showEditLocallyError(const AccountStatePtr &accountState, const QString &message, const QString &informativeText) const +{ + showEditLocallyErrorNotification(accountState, message, informativeText); + // to make sure the error is not missed, show a message box in addition + showEditLocallyErrorMessageBox(message, informativeText); +} - // Token is an alphanumeric string 128 chars long. - // Ensure that is what we received and what we are sending to the server. - const QRegularExpression tokenRegex("^[a-zA-Z0-9]{128}$"); - const auto regexMatch = tokenRegex.match(token); +void FolderMan::showEditLocallyErrorNotification(const AccountStatePtr &accountState, const QString &message, const QString &informativeText) const +{ + if (accountState && accountState->account()) { + const auto foundFolder = std::find_if(std::cbegin(map()), std::cend(map()), [accountState](const auto &folder) { + return accountState->account()->davUrl() == folder->remoteUrl(); + }); - // Means invalid token type received, be cautious with bad token - if(!regexMatch.hasMatch()) { - showError(accountFound, tr("Invalid token received."), tr("Please try again.")); - return; + if (foundFolder != std::cend(map())) { + (*foundFolder)->syncEngine().addErrorToGui(SyncFileItem::SoftError, message, informativeText); + } } +} - const auto relPathSplit = relPath.split(QLatin1Char('/')); - if (relPathSplit.size() > 0) { - Systray::instance()->createEditFileLocallyLoadingDialog(relPathSplit.last()); - } else { - showError(accountFound, tr("Could not find a file for local editing. Make sure its path is valid and it is synced locally."), relPath); - return; - } +void FolderMan::showEditLocallyErrorMessageBox(const QString &message, const QString &informativeText) const +{ + const auto messageBox = new QMessageBox; + messageBox->setAttribute(Qt::WA_DeleteOnClose); + messageBox->setText(message); + messageBox->setInformativeText(informativeText); + messageBox->setIcon(QMessageBox::Warning); + messageBox->addButton(QMessageBox::StandardButton::Ok); + messageBox->show(); + messageBox->activateWindow(); + messageBox->raise(); +} + +void FolderMan::startEditLocally(const AccountStatePtr &accountState, + const QString &relPath, + const QString &token, + const QString &fileName, + const QString &localFilePath, + Folder *folderForFile) +{ + Systray::instance()->createEditFileLocallyLoadingDialog(fileName); const auto encodedToken = QString::fromUtf8(QUrl::toPercentEncoding(token)); // Sanitise the token - const auto encodedRelPath = QUrl::toPercentEncoding(slashPrefixedPath); // Sanitise the relPath - const auto checkTokenForEditLocally = new SimpleApiJob(accountFound->account(), QStringLiteral("/ocs/v2.php/apps/files/api/v1/openlocaleditor/%1").arg(encodedToken)); + 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"), slashPrefixedPath); - checkTokenForEditLocally->addQueryParams(params); - checkTokenForEditLocally->setVerb(SimpleApiJob::Verb::Post); - connect(checkTokenForEditLocally, &SimpleApiJob::resultReceived, checkTokenForEditLocally, [this, folderForFile, localFilePath, showError, accountFound, relPath] (int statusCode) { - constexpr auto HTTP_OK_CODE = 200; - if (statusCode != HTTP_OK_CODE) { - Systray::instance()->destroyEditFileLocallyLoadingDialog(); - showError(accountFound, tr("Could not validate the request to open a file from server."), relPath); - qCInfo(lcFolderMan()) << "token check result" << statusCode; - return; - } + params.addQueryItem(QStringLiteral("path"), QString{"/" + relPath}); + checkEditLocallyToken->addQueryParams(params); - folderForFile->startSync(); - _localFileEditingSyncFinishedConnections.insert(localFilePath, QObject::connect(folderForFile, &Folder::syncFinished, this, - [this, localFilePath](const OCC::SyncResult &result) { - Q_UNUSED(result); - const auto foundConnectionIt = _localFileEditingSyncFinishedConnections.find(localFilePath); - if (foundConnectionIt != std::end(_localFileEditingSyncFinishedConnections) && foundConnectionIt.value()) { - QObject::disconnect(foundConnectionIt.value()); - _localFileEditingSyncFinishedConnections.erase(foundConnectionIt); - } - // 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 - // if the VFS is enabled - we just always call it from a separate thread. - QtConcurrent::run([localFilePath]() { - QDesktopServices::openUrl(QUrl::fromLocalFile(localFilePath)); - Systray::instance()->destroyEditFileLocallyLoadingDialog(); - }); - })); + connect(checkEditLocallyToken, &SimpleApiJob::resultReceived, checkEditLocallyToken, [this, folderForFile, localFilePath, accountState, relPath] (int statusCode) { + editLocallyTokenCheckFinished(accountState, relPath, localFilePath, folderForFile, statusCode); + }); + checkEditLocallyToken->start(); +} + +void FolderMan::editLocallyTokenCheckFinished(const AccountStatePtr &accountState, + const QString &relPath, + const QString &localFilePath, + Folder *folderForFile, + const int statusCode) +{ + constexpr auto HTTP_OK_CODE = 200; + if (statusCode != HTTP_OK_CODE) { + Systray::instance()->destroyEditFileLocallyLoadingDialog(); + showEditLocallyError(accountState, tr("Could not validate the request to open a file from server."), relPath); + qCInfo(lcFolderMan()) << "token check result" << statusCode; + return; + } + + folderForFile->startSync(); + const auto syncFinishedConnection = connect(folderForFile, &Folder::syncFinished, this, [this, localFilePath](const OCC::SyncResult &result) { + Q_UNUSED(result); + editLocallyFolderSyncFinished(localFilePath); + }); + _editLocallySyncFinishedConnections.insert(localFilePath, syncFinishedConnection); +} + +void FolderMan::editLocallyFolderSyncFinished(const QString &localFilePath) +{ + disconnectEditLocallySyncFinishedConnections(localFilePath); + openEditLocallyFile(localFilePath); +} + +void FolderMan::disconnectEditLocallySyncFinishedConnections(const QString &localFilePath) +{ + if (const auto existingConnection = _editLocallySyncFinishedConnections.value(localFilePath)) { + disconnect(existingConnection); + _editLocallySyncFinishedConnections.remove(localFilePath); + } +} + +void FolderMan::openEditLocallyFile(const QString &localFilePath) const +{ + // 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 + // if the VFS is enabled - we just always call it from a separate thread. + QtConcurrent::run([localFilePath]() { + QDesktopServices::openUrl(QUrl::fromLocalFile(localFilePath)); + Systray::instance()->destroyEditFileLocallyLoadingDialog(); }); - checkTokenForEditLocally->start(); } void FolderMan::trayOverallStatus(const QList &folders, diff --git a/src/gui/folderman.h b/src/gui/folderman.h index 55db5baba..e6322e90a 100644 --- a/src/gui/folderman.h +++ b/src/gui/folderman.h @@ -311,6 +311,27 @@ private slots: void slotProcessFilesPushNotification(Account *account); void slotConnectToPushNotifications(Account *account); + void showEditLocallyError(const AccountStatePtr &accountState, const QString &message, const QString &informativeText) const; + void showEditLocallyErrorNotification(const AccountStatePtr &accountState, const QString &message, const QString &informativeText) const; + void showEditLocallyErrorMessageBox(const QString &message, const QString &informativeText) const; + + void startEditLocally(const AccountStatePtr &accountState, + const QString &relPath, + const QString &token, + const QString &fileName, + const QString &localFilePath, + Folder *folderForFile); + + void editLocallyTokenCheckFinished(const AccountStatePtr &accountState, + const QString &relPath, + const QString &localFilePath, + Folder *folderForFile, + const int statusCode); + + void disconnectEditLocallySyncFinishedConnections(const QString &localFilePath); + void editLocallyFolderSyncFinished(const QString &localFilePath); + void openEditLocallyFile(const QString &localFilePath) const; + private: /** Adds a new folder, does not add it to the account settings and * does not set an account on the new folder. @@ -343,6 +364,11 @@ private: [[nodiscard]] bool isSwitchToVfsNeeded(const FolderDefinition &folderDefinition) const; + [[nodiscard]] bool isEditLocallyTokenValid(const QString &token) const; + [[nodiscard]] bool isEditLocallyRelPathValid(const QString &relPath) const; + [[nodiscard]] bool isEditLocallyRelPathExcluded(const QString &relPath) const; + [[nodiscard]] bool editLocallyAccount(const AccountStatePtr &acountState) const; + QSet _disabledFolders; Folder::Map _folderMap; QString _folderConfigPath; @@ -377,7 +403,7 @@ private: bool _appRestartRequired = false; - QMap _localFileEditingSyncFinishedConnections; + QHash _editLocallySyncFinishedConnections; static FolderMan *_instance; explicit FolderMan(QObject *parent = nullptr);