From 52e78b3ecef41d86b251a075e75bdfaa67cc48e4 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 0cf1a51e4..07a1d8f2a 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 d1029376c..d84c5467d 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 a2547dafa..8b8bb0c16 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 ae32d0c6b..071ae448b 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 0b20a3551..82f0376dd 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 28165f053..dcf1fb623 100644 --- a/src/libsync/account.h +++ b/src/libsync/account.h @@ -345,7 +345,7 @@ public: 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 77e48afcb..b5e31e9f2 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 9b8be46c5..269f1e91d 100644 --- a/src/libsync/lockfilejobs.h +++ b/src/libsync/lockfilejobs.h @@ -24,6 +24,7 @@ public: 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 @@ private: QString _lockToken; QString _remoteSyncPathWithTrailingSlash; QString _localSyncPath; + QString _existingEtag; }; } diff --git a/test/testlockfile.cpp b/test/testlockfile.cpp index ce91016f7..24d6ffa15 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); -- 2.30.2