From bd741dcd3e39166c72435ea2846ce8685b64fb23 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Mon, 2 Dec 2024 09:02:13 +0100 Subject: [PATCH] when locking a file set If-Match header to ensure etag is correct the file being locked may have been changed sinc the client synced it so the server side file may have a modified etag. In such cases we do not want to lock the file and would rather sync the nex server changes before being able to lock the file that would ensure that on client side the file being locked is matching teh state of teh file on server side and would prevent inadvertently overriding server changes Signed-off-by: Matthieu Gallien --- src/gui/editlocallyjob.cpp | 5 +++-- src/gui/editlocallyjob.h | 2 +- src/gui/folder.cpp | 2 ++ src/gui/socketapi/socketapi.cpp | 1 + src/libsync/account.cpp | 3 ++- src/libsync/account.h | 2 +- src/libsync/lockfilejobs.cpp | 8 +++++++- src/libsync/lockfilejobs.h | 2 ++ test/testlockfile.cpp | 14 ++++++++++++++ 9 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/gui/editlocallyjob.cpp b/src/gui/editlocallyjob.cpp index 0cf1a51e40400..07a1d8f2afb7c 100644 --- a/src/gui/editlocallyjob.cpp +++ b/src/gui/editlocallyjob.cpp @@ -467,11 +467,11 @@ void EditLocallyJob::processLocalItem() if (rec.isDirectory() || !_accountState->account()->capabilities().filesLockAvailable()) { openFile(); } else { - lockFile(); + lockFile(rec._etag); } } -void EditLocallyJob::lockFile() +void EditLocallyJob::lockFile(const QString &etag) { Q_ASSERT(_accountState); Q_ASSERT(_accountState->account()); @@ -506,6 +506,7 @@ void EditLocallyJob::lockFile() _folderForFile->accountState()->account()->setLockFileState(_relPath, _folderForFile->remotePathTrailingSlash(), _folderForFile->path(), + etag, _folderForFile->journalDb(), SyncFileItem::LockStatus::LockedItem, SyncFileItem::LockOwnerType::TokenLock); diff --git a/src/gui/editlocallyjob.h b/src/gui/editlocallyjob.h index d1029376c23e5..d84c5467dd565 100644 --- a/src/gui/editlocallyjob.h +++ b/src/gui/editlocallyjob.h @@ -65,7 +65,7 @@ private slots: void processLocalItem(); void openFile(); - void lockFile(); + void lockFile(const QString &etag); void fileAlreadyLocked(); void fileLockSuccess(const OCC::SyncFileItemPtr &item); diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index a2547dafafa69..8b8bb0c16f828 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -696,6 +696,7 @@ void Folder::slotFilesLockReleased(const QSet &files) _accountState->account()->setLockFileState(remoteFilePath, remotePathTrailingSlash(), path(), + rec._etag, journalDb(), SyncFileItem::LockStatus::UnlockedItem, lockOwnerType); @@ -744,6 +745,7 @@ void Folder::slotLockedFilesFound(const QSet &files) _accountState->account()->setLockFileState(remoteFilePath, remotePathTrailingSlash(), path(), + rec._etag, journalDb(), SyncFileItem::LockStatus::LockedItem, SyncFileItem::LockOwnerType::TokenLock); diff --git a/src/gui/socketapi/socketapi.cpp b/src/gui/socketapi/socketapi.cpp index ae32d0c6be3aa..071ae448b3814 100644 --- a/src/gui/socketapi/socketapi.cpp +++ b/src/gui/socketapi/socketapi.cpp @@ -1096,6 +1096,7 @@ void SocketApi::setFileLock(const QString &localFile, const SyncFileItem::LockSt shareFolder->accountState()->account()->setLockFileState(fileData.serverRelativePath, shareFolder->remotePathTrailingSlash(), shareFolder->path(), + record._etag, shareFolder->journalDb(), lockState, (lockState == SyncFileItem::LockStatus::UnlockedItem) ? static_cast(record._lockstate._lockOwnerType) : SyncFileItem::LockOwnerType::UserLock); diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index 0b20a3551bdc9..82f0376dd5e85 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -983,6 +983,7 @@ std::shared_ptr Account::userStatusConnector() const void Account::setLockFileState(const QString &serverRelativePath, const QString &remoteSyncPathWithTrailingSlash, const QString &localSyncPath, + const QString &etag, SyncJournalDb * const journal, const SyncFileItem::LockStatus lockStatus, const SyncFileItem::LockOwnerType lockOwnerType) @@ -993,7 +994,7 @@ void Account::setLockFileState(const QString &serverRelativePath, return; } lockStatusJobInProgress.push_back(lockStatus); - auto job = std::make_unique(sharedFromThis(), journal, serverRelativePath, remoteSyncPathWithTrailingSlash, localSyncPath, lockStatus, lockOwnerType); + auto job = std::make_unique(sharedFromThis(), journal, serverRelativePath, remoteSyncPathWithTrailingSlash, localSyncPath, etag, lockStatus, lockOwnerType); connect(job.get(), &LockFileJob::finishedWithoutError, this, [this, serverRelativePath, lockStatus]() { removeLockStatusChangeInprogress(serverRelativePath, lockStatus); Q_EMIT lockFileSuccess(); diff --git a/src/libsync/account.h b/src/libsync/account.h index 28165f05320fd..dcf1fb623bd64 100644 --- a/src/libsync/account.h +++ b/src/libsync/account.h @@ -345,7 +345,7 @@ class OWNCLOUDSYNC_EXPORT Account : public QObject void setLockFileState(const QString &serverRelativePath, const QString &remoteSyncPathWithTrailingSlash, - const QString &localSyncPath, + const QString &localSyncPath, const QString &etag, SyncJournalDb * const journal, const SyncFileItem::LockStatus lockStatus, const SyncFileItem::LockOwnerType lockOwnerType); diff --git a/src/libsync/lockfilejobs.cpp b/src/libsync/lockfilejobs.cpp index 77e48afcbc66f..b5e31e9f2fe97 100644 --- a/src/libsync/lockfilejobs.cpp +++ b/src/libsync/lockfilejobs.cpp @@ -30,6 +30,7 @@ LockFileJob::LockFileJob(const AccountPtr account, const QString &path, const QString &remoteSyncPathWithTrailingSlash, const QString &localSyncPath, + const QString &etag, const SyncFileItem::LockStatus requestedLockState, const SyncFileItem::LockOwnerType lockOwnerType, QObject *parent) @@ -39,6 +40,7 @@ LockFileJob::LockFileJob(const AccountPtr account, , _requestedLockOwnerType(lockOwnerType) , _remoteSyncPathWithTrailingSlash(remoteSyncPathWithTrailingSlash) , _localSyncPath(localSyncPath) + , _existingEtag(etag) { if (!_localSyncPath.endsWith(QLatin1Char('/'))) { _localSyncPath.append(QLatin1Char('/')); @@ -65,8 +67,12 @@ void LockFileJob::start() switch(_requestedLockState) { case SyncFileItem::LockStatus::LockedItem: + { + const auto etagValue = QLatin1String("\"%1\"").arg(_existingEtag.toLatin1()); + request.setRawHeader(QByteArrayLiteral("If-Match"), etagValue.toLatin1()); verb = "LOCK"; break; + } case SyncFileItem::LockStatus::UnlockedItem: verb = "UNLOCK"; break; @@ -79,7 +85,7 @@ void LockFileJob::start() bool LockFileJob::finished() { if (reply()->error() != QNetworkReply::NoError) { - qCInfo(lcLockFileJob()) << "finished with error" << reply()->error() << reply()->errorString() << _requestedLockState << _requestedLockOwnerType; + qCInfo(lcLockFileJob()) << "finished with error" << reply()->error() << reply()->errorString() << _requestedLockState << _requestedLockOwnerType << _existingEtag; const auto httpErrorCode = reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); if (httpErrorCode == LOCKED_HTTP_ERROR_CODE) { const auto record = handleReply(); diff --git a/src/libsync/lockfilejobs.h b/src/libsync/lockfilejobs.h index 9b8be46c5523f..269f1e91d32d2 100644 --- a/src/libsync/lockfilejobs.h +++ b/src/libsync/lockfilejobs.h @@ -24,6 +24,7 @@ class OWNCLOUDSYNC_EXPORT LockFileJob : public AbstractNetworkJob const QString &path, const QString &remoteSyncPathWithTrailingSlash, const QString &localSyncPath, + const QString &etag, const SyncFileItem::LockStatus requestedLockState, const SyncFileItem::LockOwnerType lockOwnerType, QObject *parent = nullptr); @@ -62,6 +63,7 @@ class OWNCLOUDSYNC_EXPORT LockFileJob : public AbstractNetworkJob QString _lockToken; QString _remoteSyncPathWithTrailingSlash; QString _localSyncPath; + QString _existingEtag; }; } diff --git a/test/testlockfile.cpp b/test/testlockfile.cpp index ce91016f7ae09..24d6ffa15d54f 100644 --- a/test/testlockfile.cpp +++ b/test/testlockfile.cpp @@ -43,6 +43,7 @@ private slots: fakeFolder.account()->setLockFileState(QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, &fakeFolder.syncJournal(), OCC::SyncFileItem::LockStatus::LockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -89,6 +90,7 @@ private slots: fakeFolder.account()->setLockFileState(QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, &fakeFolder.syncJournal(), OCC::SyncFileItem::LockStatus::LockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -114,6 +116,7 @@ private slots: fakeFolder.account()->setLockFileState(QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, &fakeFolder.syncJournal(), OCC::SyncFileItem::LockStatus::LockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -142,6 +145,7 @@ private slots: fakeFolder.account()->setLockFileState(QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, &fakeFolder.syncJournal(), OCC::SyncFileItem::LockStatus::LockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -168,6 +172,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::LockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -207,6 +212,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::LockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -225,6 +231,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::UnlockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -285,6 +292,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::LockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -339,6 +347,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::LockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -393,6 +402,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::UnlockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -445,6 +455,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::UnlockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -484,6 +495,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::LockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -502,6 +514,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::UnlockedItem, OCC::SyncFileItem::LockOwnerType::UserLock); @@ -556,6 +569,7 @@ private slots: QStringLiteral("/") + testFileName, QStringLiteral("/"), fakeFolder.localPath(), + {}, OCC::SyncFileItem::LockStatus::UnlockedItem, OCC::SyncFileItem::LockOwnerType::UserLock);