From 0039ffd7da0ae40cf439573ca9725fa86afd73d2 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 23 Jun 2023 13:49:22 +0200 Subject: [PATCH 1/4] always propagate locked status to read-only or read/write for real file make sure that a file locked that would not be modifiable by the current client is read-only on storage also make sure we make a file read/write when modification can happen again Close #5537 Signed-off-by: Matthieu Gallien --- src/libsync/propagatedownload.cpp | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 2edd3c4c5ba82..93b66526ca9f2 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -1198,6 +1198,16 @@ void PropagateDownloadFile::downloadFinished() } } + if (_item->_locked == SyncFileItem::LockStatus::LockedItem && (_item->_lockOwnerType != SyncFileItem::LockOwnerType::UserLock || _item->_lockOwnerId != propagator()->account()->davUser())) { + qCDebug(lcPropagateDownload()) << _tmpFile << "file is locked: making it read only"; + FileSystem::setFileReadOnly(filename, true); + } else { + qCDebug(lcPropagateDownload()) << _tmpFile << "file is not locked: making it" + << ((!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) ? "read only" + : "read write"); + FileSystem::setFileReadOnly(filename, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); + } + // Apply the remote permissions FileSystem::setFileReadOnlyWeak(_tmpFile.fileName(), !_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)); @@ -1262,12 +1272,6 @@ void PropagateDownloadFile::downloadFinished() return; } - qCInfo(lcPropagateDownload()) << propagator()->account()->davUser() << propagator()->account()->davDisplayName() << propagator()->account()->displayName(); - if (_item->_locked == SyncFileItem::LockStatus::LockedItem && (_item->_lockOwnerType != SyncFileItem::LockOwnerType::UserLock || _item->_lockOwnerId != propagator()->account()->davUser())) { - qCInfo(lcPropagateDownload()) << "file is locked: making it read only"; - FileSystem::setFileReadOnly(filename, true); - } - FileSystem::setFileHidden(filename, false); // Maybe we downloaded a newer version of the file than we thought we would... @@ -1346,6 +1350,14 @@ void PropagateDownloadFile::updateMetadata(bool isConflict) handleRecallFile(fn, propagator()->localPath(), *propagator()->_journal); } + if (_item->_locked == SyncFileItem::LockStatus::LockedItem && (_item->_lockOwnerType != SyncFileItem::LockOwnerType::UserLock || _item->_lockOwnerId != propagator()->account()->davUser())) { + qCDebug(lcPropagateDownload()) << fn << "file is locked: making it read only"; + FileSystem::setFileReadOnly(fn, true); + } else { + qCDebug(lcPropagateDownload()) << fn << "file is not locked: making it" << ((!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) ? "read only" : "read write"); + FileSystem::setFileReadOnly(fn, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); + } + qint64 duration = _stopwatch.elapsed(); if (isLikelyFinishedQuickly() && duration > 5 * 1000) { qCWarning(lcPropagateDownload) << "WARNING: Unexpectedly slow connection, took" << duration << "msec for" << _item->_size - _resumeStart << "bytes for" << _item->_file; From 505021af039a9ede61e69e1d7deebff27bdbc5b6 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Mon, 10 Jul 2023 17:56:16 +0200 Subject: [PATCH 2/4] enable debug log for some automated tests should help investigate failure for automated tests Signed-off-by: Matthieu Gallien --- test/testpermissions.cpp | 4 ++++ test/testsyncengine.cpp | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 0aba5f501c42e..6aecc3a5cc9c3 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -67,6 +67,10 @@ class TestPermissions : public QObject Q_OBJECT private slots: + void initTestCase() + { + OCC::Logger::instance()->setLogDebug(true); + } void t7pl() { diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 746ccde3dc322..ec5aebeb24892 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -85,6 +85,11 @@ class TestSyncEngine : public QObject Q_OBJECT private slots: + void initTestCase() + { + OCC::Logger::instance()->setLogDebug(true); + } + void testFileDownload() { FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; ItemCompletedSpy completeSpy(fakeFolder); From 845c66666e68849a5ca4ca3ec0ec404d278c886e Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Tue, 11 Jul 2023 10:35:34 +0200 Subject: [PATCH 3/4] fix failing tests by using FileSystem::setFileReadOnlyWeak we try to preserve complex existing permissions unless the client is required to change them significantly Signed-off-by: Matthieu Gallien --- src/libsync/propagatedownload.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 93b66526ca9f2..13d1ba58c6975 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -1205,7 +1205,7 @@ void PropagateDownloadFile::downloadFinished() qCDebug(lcPropagateDownload()) << _tmpFile << "file is not locked: making it" << ((!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) ? "read only" : "read write"); - FileSystem::setFileReadOnly(filename, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); + FileSystem::setFileReadOnlyWeak(filename, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); } // Apply the remote permissions @@ -1355,7 +1355,7 @@ void PropagateDownloadFile::updateMetadata(bool isConflict) FileSystem::setFileReadOnly(fn, true); } else { qCDebug(lcPropagateDownload()) << fn << "file is not locked: making it" << ((!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) ? "read only" : "read write"); - FileSystem::setFileReadOnly(fn, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); + FileSystem::setFileReadOnlyWeak(fn, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); } qint64 duration = _stopwatch.elapsed(); From bc28bc43c3fdd0fbc3b532aae4671e9a878df1af Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 12 Jul 2023 13:38:17 +0200 Subject: [PATCH 4/4] set read only/read file on the newly downloaded file will fix behavior on windows by properly setting the new temp file just downloaded have the proper state (read only or read/write) and not the already existing file that will be replaced by the newly odwnloaded one Signed-off-by: Matthieu Gallien --- src/libsync/propagatedownload.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 13d1ba58c6975..08ab484891501 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -1200,17 +1200,14 @@ void PropagateDownloadFile::downloadFinished() if (_item->_locked == SyncFileItem::LockStatus::LockedItem && (_item->_lockOwnerType != SyncFileItem::LockOwnerType::UserLock || _item->_lockOwnerId != propagator()->account()->davUser())) { qCDebug(lcPropagateDownload()) << _tmpFile << "file is locked: making it read only"; - FileSystem::setFileReadOnly(filename, true); + FileSystem::setFileReadOnly(_tmpFile.fileName(), true); } else { qCDebug(lcPropagateDownload()) << _tmpFile << "file is not locked: making it" << ((!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) ? "read only" : "read write"); - FileSystem::setFileReadOnlyWeak(filename, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); + FileSystem::setFileReadOnlyWeak(_tmpFile.fileName(), (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); } - // Apply the remote permissions - FileSystem::setFileReadOnlyWeak(_tmpFile.fileName(), !_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)); - const auto isConflict = (_item->_instruction == CSYNC_INSTRUCTION_CONFLICT && (QFileInfo(filename).isDir() || !FileSystem::fileEquals(filename, _tmpFile.fileName()))) || _item->_instruction == CSYNC_INSTRUCTION_CASE_CLASH_CONFLICT;