Merge pull request #7532 from nextcloud/bugfix/narrowDownPermissionsDuringSync

mgallien · web-flow · commit 1e378a9b4a8b · 2024-11-21T12:07:07.000+01:00
Bugfix/narrow down permissions during sync
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1457,21 +1457,13 @@ 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))) {
                         FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadOnly);
-                        qCDebug(lcDirectory) << "old permissions" << static_cast<int>(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<int>(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<int>(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<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions());
                     }
                 }
                 catch (const std::filesystem::filesystem_error &e)
@@ -1480,23 +1472,20 @@ 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);
-                        qCDebug(lcDirectory) << "old permissions" << static_cast<int>(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<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions());
+                    const auto permissionsChangeHelper = [] (const auto fileName)
+                    {
+                        qCDebug(lcDirectory) << fileName << "permissions changed: old permissions" << static_cast<int>(std::filesystem::status(fileName.toStdWString()).permissions());
+                        FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadWrite);
+                        qCDebug(lcDirectory) << fileName << "applied new permissions" << static_cast<int>(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))) {
-                        FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadWrite);
-                        qCDebug(lcDirectory) << "old permissions" << static_cast<int>(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);
-                        qCDebug(lcDirectory) << "new permissions" << static_cast<int>(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)
diff --git 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
@@ -343,6 +343,8 @@ class OWNCLOUDSYNC_EXPORT SyncFileItem
     bool _isLivePhoto = false;
     QString _livePhotoFile;
 
+    bool isPermissionsInvalid = false;
+
     QString _discoveryResult;
 };
 
diff --git 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<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> 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<bool>(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<bool>(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');

Original file line number	Diff line number	Diff line change
`@@ -170,5 +170,7 @@ static int _csync_vio_local_stat_mb(const mbchar_t wuri, csync_file_stat_t buf`
`170`	`170`	`buf->inode = sb.st_ino;`
`171`	`171`	`buf->modtime = sb.st_mtime;`
`172`	`172`	`buf->size = sb.st_size;`
	`173`	`+ buf->isPermissionsInvalid = (sb.st_mode & S_IWOTH) == S_IWOTH;`
	`174`	`+`
`173`	`175`	`return 0;`
`174`	`176`	`}`
Original file line number	Diff line number	Diff line change
`@@ -348,6 +348,7 @@ void DiscoverySingleLocalDirectoryJob::run() {`
`348`	`348`	`i.isSymLink = dirent->type == ItemTypeSoftLink;`
`349`	`349`	`i.isVirtualFile = dirent->type == ItemTypeVirtualFile \|\| dirent->type == ItemTypeVirtualFileDownload;`
`350`	`350`	`i.isMetadataMissing = dirent->is_metadata_missing;`
	`351`	`+ i.isPermissionsInvalid = dirent->isPermissionsInvalid;`
`351`	`352`	`i.type = dirent->type;`
`352`	`353`	`results.push_back(i);`
`353`	`354`	`}`
Original file line number	Diff line number	Diff line change
`@@ -468,6 +468,7 @@ bool FileSystem::setFolderPermissions(const QString &path,`
`468`	`468`	`case OCC::FileSystem::FolderPermissions::ReadOnly:`
`469`	`469`	`break;`
`470`	`470`	`case OCC::FileSystem::FolderPermissions::ReadWrite:`
	`471`	`+ std::filesystem::permissions(stdStrPath, std::filesystem::perms::others_write, std::filesystem::perm_options::remove);`
`471`	`472`	`std::filesystem::permissions(stdStrPath, std::filesystem::perms::owner_write, std::filesystem::perm_options::add);`
`472`	`473`	`break;`
`473`	`474`	`}`
Original file line number	Diff line number	Diff line change
`@@ -363,6 +363,10 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item)`
`363`	`363`	`const bool isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite);`
`364`	`364`	`modificationHappened = FileSystem::setFileReadOnlyWeak(filePath, isReadOnly);`
`365`	`365`	`}`
	`366`	`+ if (item->isPermissionsInvalid) {`
	`367`	`+ const auto isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite);`
	`368`	`+ FileSystem::setFileReadOnly(filePath, isReadOnly);`
	`369`	`+ }`
`366`	`370`
`367`	`371`	`modificationHappened \|= item->_size != prev._fileSize;`
`368`	`372`