Skip to content

Commit

Permalink
when locking a file set If-Match header to ensure etag is correct
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@mgallien
mgallien committed Dec 9, 2024
1 parent 63d07ac commit bd741dc
Show file tree
Hide file tree
Showing 9 changed files with 33 additions and 6 deletions.
5 changes: 3 additions & 2 deletions src/gui/editlocallyjob.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/gui/editlocallyjob.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions src/gui/folder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,7 @@ void Folder::slotFilesLockReleased(const QSet<QString> &files)
_accountState->account()->setLockFileState(remoteFilePath,
remotePathTrailingSlash(),
path(),
rec._etag,
journalDb(),
SyncFileItem::LockStatus::UnlockedItem,
lockOwnerType);
Expand Down Expand Up @@ -744,6 +745,7 @@ void Folder::slotLockedFilesFound(const QSet<QString> &files)
_accountState->account()->setLockFileState(remoteFilePath,
remotePathTrailingSlash(),
path(),
rec._etag,
journalDb(),
SyncFileItem::LockStatus::LockedItem,
SyncFileItem::LockOwnerType::TokenLock);
Expand Down
1 change: 1 addition & 0 deletions src/gui/socketapi/socketapi.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<SyncFileItem::LockOwnerType>(record._lockstate._lockOwnerType) : SyncFileItem::LockOwnerType::UserLock);
Expand Down
3 changes: 2 additions & 1 deletion src/libsync/account.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,7 @@ std::shared_ptr<UserStatusConnector> 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)
Expand All @@ -993,7 +994,7 @@ void Account::setLockFileState(const QString &serverRelativePath,
return;
}
lockStatusJobInProgress.push_back(lockStatus);
auto job = std::make_unique<LockFileJob>(sharedFromThis(), journal, serverRelativePath, remoteSyncPathWithTrailingSlash, localSyncPath, lockStatus, lockOwnerType);
auto job = std::make_unique<LockFileJob>(sharedFromThis(), journal, serverRelativePath, remoteSyncPathWithTrailingSlash, localSyncPath, etag, lockStatus, lockOwnerType);
connect(job.get(), &LockFileJob::finishedWithoutError, this, [this, serverRelativePath, lockStatus]() {
removeLockStatusChangeInprogress(serverRelativePath, lockStatus);
Q_EMIT lockFileSuccess();
Expand Down
2 changes: 1 addition & 1 deletion src/libsync/account.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
8 changes: 7 additions & 1 deletion src/libsync/lockfilejobs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -39,6 +40,7 @@ LockFileJob::LockFileJob(const AccountPtr account,
, _requestedLockOwnerType(lockOwnerType)
, _remoteSyncPathWithTrailingSlash(remoteSyncPathWithTrailingSlash)
, _localSyncPath(localSyncPath)
, _existingEtag(etag)
{
if (!_localSyncPath.endsWith(QLatin1Char('/'))) {
_localSyncPath.append(QLatin1Char('/'));
Expand All @@ -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;
Expand All @@ -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();
Expand Down
2 changes: 2 additions & 0 deletions src/libsync/lockfilejobs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -62,6 +63,7 @@ class OWNCLOUDSYNC_EXPORT LockFileJob : public AbstractNetworkJob
QString _lockToken;
QString _remoteSyncPathWithTrailingSlash;
QString _localSyncPath;
QString _existingEtag;
};

}
Expand Down
14 changes: 14 additions & 0 deletions test/testlockfile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ private slots:
fakeFolder.account()->setLockFileState(QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
&fakeFolder.syncJournal(),
OCC::SyncFileItem::LockStatus::LockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);
Expand Down Expand Up @@ -89,6 +90,7 @@ private slots:
fakeFolder.account()->setLockFileState(QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
&fakeFolder.syncJournal(),
OCC::SyncFileItem::LockStatus::LockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);
Expand All @@ -114,6 +116,7 @@ private slots:
fakeFolder.account()->setLockFileState(QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
&fakeFolder.syncJournal(),
OCC::SyncFileItem::LockStatus::LockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);
Expand Down Expand Up @@ -142,6 +145,7 @@ private slots:
fakeFolder.account()->setLockFileState(QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
&fakeFolder.syncJournal(),
OCC::SyncFileItem::LockStatus::LockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);
Expand All @@ -168,6 +172,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::LockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand Down Expand Up @@ -207,6 +212,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::LockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand All @@ -225,6 +231,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::UnlockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand Down Expand Up @@ -285,6 +292,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::LockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand Down Expand Up @@ -339,6 +347,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::LockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand Down Expand Up @@ -393,6 +402,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::UnlockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand Down Expand Up @@ -445,6 +455,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::UnlockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand Down Expand Up @@ -484,6 +495,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::LockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand All @@ -502,6 +514,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::UnlockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand Down Expand Up @@ -556,6 +569,7 @@ private slots:
QStringLiteral("/") + testFileName,
QStringLiteral("/"),
fakeFolder.localPath(),
{},
OCC::SyncFileItem::LockStatus::UnlockedItem,
OCC::SyncFileItem::LockOwnerType::UserLock);

Expand Down

0 comments on commit bd741dc

Please sign in to comment.