From 4923873ac829bc0e870b5abde445c78d19de02b7 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 20 Nov 2024 12:03:37 +0100 Subject: [PATCH 1/3] ensure folder permissions are read-only when needed a folder item must be set read-only if files or sub-folders cannot be created a folder item must be set read-write in all other situations Signed-off-by: Matthieu Gallien --- src/libsync/owncloudpropagator.cpp | 8 +--- test/testpermissions.cpp | 63 ++---------------------------- 2 files changed, 4 insertions(+), 67 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index c8b85657bb0c8..193da94df3e9a 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1457,8 +1457,6 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) #if !defined(Q_OS_MACOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_15 if (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) && - !_item->_remotePerm.hasPermission(RemotePermissions::CanRename) && - !_item->_remotePerm.hasPermission(RemotePermissions::CanMove) && !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories)) { try { if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) { @@ -1480,11 +1478,7 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) _item->_status = SyncFileItem::NormalError; _item->_errorString = tr("The folder %1 cannot be made read-only: %2").arg(_item->_file, e.what()); } - } else if (!_item->_remotePerm.isNull() && - (_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) || - !_item->_remotePerm.hasPermission(RemotePermissions::CanRename) || - !_item->_remotePerm.hasPermission(RemotePermissions::CanMove) || - !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories))) { + } else { try { if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) { FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadWrite); diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index f334b22de5abc..a090d3b6fe141 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -564,63 +564,6 @@ private slots: QVERIFY(cls.find("zallowed/sub2/file")); } - // Test for issue #7293 - void testAllowedMoveForbiddenDelete() { - FakeFolder fakeFolder{FileInfo{}}; - - QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders, - [&](const QList &folders, const QString &localPath, std::function callback) { - for(const auto &oneFolder : folders) { - FileSystem::removeRecursively(localPath + oneFolder->_file); - } - callback(false); - }); - - // Some of this test depends on the order of discovery. With threading - // that order becomes effectively random, but we want to make sure to test - // all cases and thus disable threading. - auto syncOpts = fakeFolder.syncEngine().syncOptions(); - syncOpts._parallelNetworkJobs = 1; - fakeFolder.syncEngine().setSyncOptions(syncOpts); - - auto &lm = fakeFolder.localModifier(); - auto &rm = fakeFolder.remoteModifier(); - rm.mkdir("changeonly"); - rm.mkdir("changeonly/sub1"); - rm.insert("changeonly/sub1/file1"); - rm.insert("changeonly/sub1/filetorname1a"); - rm.insert("changeonly/sub1/filetorname1z"); - rm.mkdir("changeonly/sub2"); - rm.insert("changeonly/sub2/file2"); - rm.insert("changeonly/sub2/filetorname2a"); - rm.insert("changeonly/sub2/filetorname2z"); - - setAllPerm(rm.find("changeonly"), RemotePermissions::fromServerString("NSV")); - - QVERIFY(fakeFolder.syncOnce()); - - lm.rename("changeonly/sub1/filetorname1a", "changeonly/sub1/aaa1_renamed"); - lm.rename("changeonly/sub1/filetorname1z", "changeonly/sub1/zzz1_renamed"); - - lm.rename("changeonly/sub2/filetorname2a", "changeonly/sub2/aaa2_renamed"); - lm.rename("changeonly/sub2/filetorname2z", "changeonly/sub2/zzz2_renamed"); - - auto expectedState = fakeFolder.currentLocalState(); - - QVERIFY(fakeFolder.syncOnce()); - QCOMPARE(fakeFolder.currentLocalState(), expectedState); - QCOMPARE(fakeFolder.currentRemoteState(), expectedState); - - lm.rename("changeonly/sub1", "changeonly/aaa"); - lm.rename("changeonly/sub2", "changeonly/zzz"); - - expectedState = fakeFolder.currentLocalState(); - - QVERIFY(fakeFolder.syncOnce()); - QCOMPARE(fakeFolder.currentLocalState(), expectedState); - QCOMPARE(fakeFolder.currentRemoteState(), expectedState); - } - void testParentMoveNotAllowedChildrenRestored() { FakeFolder fakeFolder{FileInfo{}}; @@ -722,7 +665,7 @@ private slots: remote.mkdir("readWriteFolder"); - remote.find("readWriteFolder")->permissions = RemotePermissions::fromServerString("WDNVRSM"); + remote.find("readWriteFolder")->permissions = RemotePermissions::fromServerString("CKWDNVRSM"); QVERIFY(fakeFolder.syncOnce()); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); @@ -757,7 +700,7 @@ private slots: QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read); QVERIFY(!static_cast(folderStatus.permissions() & std::filesystem::perms::owner_write)); - remote.find("testFolder")->permissions = RemotePermissions::fromServerString("WDNVRSM"); + remote.find("testFolder")->permissions = RemotePermissions::fromServerString("CKWDNVRSM"); QVERIFY(fakeFolder.syncOnce()); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); @@ -815,7 +758,7 @@ private slots: QVERIFY(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_read); QVERIFY(!static_cast(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_write)); - remote.find("testFolder/subFolderReadOnly")->permissions = RemotePermissions::fromServerString("WDNVRSm"); + remote.find("testFolder/subFolderReadOnly")->permissions = RemotePermissions::fromServerString("CKWDNVRSm"); remote.find("testFolder/subFolderReadWrite")->permissions = RemotePermissions::fromServerString("m"); remote.mkdir("testFolder/newSubFolder"); remote.create("testFolder/testFile", 12, '9'); From 5b2af166d3d9c8537c565922750392d4a3f6610e Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 20 Nov 2024 14:09:08 +0100 Subject: [PATCH 2/3] ensure no any user writable permissions in Nextcloud sync folder past versions may have wrongly set the write permissions for other users (UNIX style permissions) remove this under the hypothesis that newly created files or folders gets more restrictive permissions and that past files or folders should be updated to get the same permissions Signed-off-by: Matthieu Gallien --- src/csync/csync.h | 2 ++ src/csync/vio/csync_vio_local_unix.cpp | 2 ++ src/libsync/discovery.cpp | 11 +++++++++++ src/libsync/discoveryphase.cpp | 1 + src/libsync/discoveryphase.h | 1 + src/libsync/filesystem.cpp | 1 + src/libsync/owncloudpropagator.cpp | 14 +++----------- src/libsync/syncengine.cpp | 4 ++++ src/libsync/syncfileitem.h | 2 ++ 9 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/csync/csync.h b/src/csync/csync.h index 5c8fbc0978b6b..9da7497f75c7f 100644 --- a/src/csync/csync.h +++ b/src/csync/csync.h @@ -217,6 +217,7 @@ struct OCSYNC_EXPORT csync_file_stat_s { bool is_hidden BITFIELD(1); // Not saved in the DB, only used during discovery for local files. bool isE2eEncrypted BITFIELD(1); bool is_metadata_missing BITFIELD(1); // Indicates the file has missing metadata, f.ex. the file is not a placeholder in case of vfs. + bool isPermissionsInvalid BITFIELD(1); QByteArray path; QByteArray rename_path; @@ -244,6 +245,7 @@ struct OCSYNC_EXPORT csync_file_stat_s { , is_hidden(false) , isE2eEncrypted(false) , is_metadata_missing(false) + , isPermissionsInvalid(false) { } }; diff --git a/src/csync/vio/csync_vio_local_unix.cpp b/src/csync/vio/csync_vio_local_unix.cpp index c5e22abb3c8b0..ec47ab3c7c994 100644 --- a/src/csync/vio/csync_vio_local_unix.cpp +++ b/src/csync/vio/csync_vio_local_unix.cpp @@ -170,5 +170,7 @@ static int _csync_vio_local_stat_mb(const mbchar_t *wuri, csync_file_stat_t *buf buf->inode = sb.st_ino; buf->modtime = sb.st_mtime; buf->size = sb.st_size; + buf->isPermissionsInvalid = (sb.st_mode & S_IWOTH) == S_IWOTH; + return 0; } diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 05bdf1554bc0b..7300a4c5f9a1e 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -1070,6 +1070,10 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( if (_queryLocal != NormalQuery && _queryServer != NormalQuery) recurse = false; + if (localEntry.isPermissionsInvalid) { + recurse = true; + } + if ((item->_direction == SyncFileItem::Down || item->_instruction == CSYNC_INSTRUCTION_CONFLICT || item->_instruction == CSYNC_INSTRUCTION_NEW || item->_instruction == CSYNC_INSTRUCTION_SYNC) && (item->_modtime <= 0 || item->_modtime >= 0xFFFFFFFF)) { item->_instruction = CSYNC_INSTRUCTION_ERROR; @@ -1097,6 +1101,13 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( } } + if (localEntry.isPermissionsInvalid && item->_instruction == CSyncEnums::CSYNC_INSTRUCTION_NONE) { + item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA; + item->_direction = SyncFileItem::Down; + } + + item->isPermissionsInvalid = localEntry.isPermissionsInvalid; + auto recurseQueryLocal = _queryLocal == ParentNotChanged ? ParentNotChanged : localEntry.isDirectory || item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist; processFileFinalize(item, path, recurse, recurseQueryLocal, recurseQueryServer); }; diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index 3ca34e94f50fb..6cd226f2ee1a5 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -348,6 +348,7 @@ void DiscoverySingleLocalDirectoryJob::run() { i.isSymLink = dirent->type == ItemTypeSoftLink; i.isVirtualFile = dirent->type == ItemTypeVirtualFile || dirent->type == ItemTypeVirtualFileDownload; i.isMetadataMissing = dirent->is_metadata_missing; + i.isPermissionsInvalid = dirent->isPermissionsInvalid; i.type = dirent->type; results.push_back(i); } diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h index 2e801de34cd53..bb932f568b4db 100644 --- a/src/libsync/discoveryphase.h +++ b/src/libsync/discoveryphase.h @@ -106,6 +106,7 @@ struct LocalInfo bool isVirtualFile = false; bool isSymLink = false; bool isMetadataMissing = false; + bool isPermissionsInvalid = false; [[nodiscard]] bool isValid() const { return !name.isNull(); } }; diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp index 2931a3d877c7e..9dbdde6a7474c 100644 --- a/src/libsync/filesystem.cpp +++ b/src/libsync/filesystem.cpp @@ -468,6 +468,7 @@ bool FileSystem::setFolderPermissions(const QString &path, case OCC::FileSystem::FolderPermissions::ReadOnly: break; case OCC::FileSystem::FolderPermissions::ReadWrite: + std::filesystem::permissions(stdStrPath, std::filesystem::perms::others_write, std::filesystem::perm_options::remove); std::filesystem::permissions(stdStrPath, std::filesystem::perms::owner_write, std::filesystem::perm_options::add); break; } diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 193da94df3e9a..f10fc6135913c 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1461,15 +1461,9 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) try { if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) { FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadOnly); - qCDebug(lcDirectory) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); - std::filesystem::permissions(propagator()->fullLocalPath(_item->_file).toStdWString(), std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write, std::filesystem::perm_options::remove); - qCDebug(lcDirectory) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); } if (!_item->_renameTarget.isEmpty() && FileSystem::fileExists(propagator()->fullLocalPath(_item->_renameTarget))) { FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadOnly); - qCDebug(lcDirectory) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); - std::filesystem::permissions(propagator()->fullLocalPath(_item->_renameTarget).toStdWString(), std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write, std::filesystem::perm_options::remove); - qCDebug(lcDirectory) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); } } catch (const std::filesystem::filesystem_error &e) @@ -1481,15 +1475,13 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) } else { try { if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) { + qCDebug(lcDirectory) << propagator()->fullLocalPath(_item->_file) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadWrite); - qCDebug(lcDirectory) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); - std::filesystem::permissions(propagator()->fullLocalPath(_item->_file).toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); - qCDebug(lcDirectory) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); + qCDebug(lcDirectory) << propagator()->fullLocalPath(_item->_file) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); } if (!_item->_renameTarget.isEmpty() && FileSystem::fileExists(propagator()->fullLocalPath(_item->_renameTarget))) { - FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadWrite); qCDebug(lcDirectory) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); - std::filesystem::permissions(propagator()->fullLocalPath(_item->_renameTarget).toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadWrite); qCDebug(lcDirectory) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); } } diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index ac48c0be2c2cf..0f8691cd05960 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -363,6 +363,10 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) const bool isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite); modificationHappened = FileSystem::setFileReadOnlyWeak(filePath, isReadOnly); } + if (item->isPermissionsInvalid) { + const auto isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite); + FileSystem::setFileReadOnly(filePath, isReadOnly); + } modificationHappened |= item->_size != prev._fileSize; diff --git a/src/libsync/syncfileitem.h b/src/libsync/syncfileitem.h index d90348af4ebda..46ee49621c688 100644 --- a/src/libsync/syncfileitem.h +++ b/src/libsync/syncfileitem.h @@ -343,6 +343,8 @@ class OWNCLOUDSYNC_EXPORT SyncFileItem bool _isLivePhoto = false; QString _livePhotoFile; + bool isPermissionsInvalid = false; + QString _discoveryResult; }; From 1417e8cb60e84762f94345b21d587fb54bc90b51 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 21 Nov 2024 11:52:04 +0100 Subject: [PATCH 3/3] better logs and factor common code in folder permissions handling Signed-off-by: Matthieu Gallien --- src/libsync/owncloudpropagator.cpp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index f10fc6135913c..ceb41fd84d021 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1474,15 +1474,18 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) } } else { try { - if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) { - qCDebug(lcDirectory) << propagator()->fullLocalPath(_item->_file) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); - FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadWrite); - qCDebug(lcDirectory) << propagator()->fullLocalPath(_item->_file) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); + const auto permissionsChangeHelper = [] (const auto fileName) + { + qCDebug(lcDirectory) << fileName << "permissions changed: old permissions" << static_cast(std::filesystem::status(fileName.toStdWString()).permissions()); + FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadWrite); + qCDebug(lcDirectory) << fileName << "applied new permissions" << static_cast(std::filesystem::status(fileName.toStdWString()).permissions()); + }; + + if (const auto fileName = propagator()->fullLocalPath(_item->_file); FileSystem::fileExists(fileName)) { + permissionsChangeHelper(fileName); } - if (!_item->_renameTarget.isEmpty() && FileSystem::fileExists(propagator()->fullLocalPath(_item->_renameTarget))) { - qCDebug(lcDirectory) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); - FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadWrite); - qCDebug(lcDirectory) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); + if (const auto fileName = propagator()->fullLocalPath(_item->_renameTarget); !_item->_renameTarget.isEmpty() && FileSystem::fileExists(fileName)) { + permissionsChangeHelper(fileName); } } catch (const std::filesystem::filesystem_error &e)