Merge pull request #7010 from nextcloud/bugfix/deleteReadonlyFolders

Bugfix/delete readonly folders
nextcloud · Aug 21, 2024 · 8cb1db4 · 8cb1db4
2 parents dfe9ada + 8dcbdeb
commit 8cb1db4
Show file tree

Hide file tree

Showing 4 changed files with 183 additions and 4 deletions.
diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp
@@ -480,6 +480,25 @@ bool FileSystem::isFolderReadOnly(const std::filesystem::path &path) noexcept
         return false;
     }
 }
+
+FileSystem::FilePermissionsRestore::FilePermissionsRestore(const QString &path, FolderPermissions temporaryPermissions)
+    : _path(path)
+{
+    const auto stdStrPath = _path.toStdWString();
+    _initialPermissions = FileSystem::isFolderReadOnly(stdStrPath) ? OCC::FileSystem::FolderPermissions::ReadOnly : OCC::FileSystem::FolderPermissions::ReadWrite;
+    if (_initialPermissions != temporaryPermissions) {
+        _rollbackNeeded = true;
+        FileSystem::setFolderPermissions(_path, temporaryPermissions);
+    }
+}
+
+FileSystem::FilePermissionsRestore::~FilePermissionsRestore()
+{
+    if (_rollbackNeeded) {
+        FileSystem::setFolderPermissions(_path, _initialPermissions);
+    }
+}
+
 #endif
 
 } // namespace OCC
diff --git a/src/libsync/filesystem.h b/src/libsync/filesystem.h
@@ -41,6 +41,19 @@ class SyncJournal;
  * @brief This file contains file system helper
  */
 namespace FileSystem {
+    class OWNCLOUDSYNC_EXPORT FilePermissionsRestore {
+    public:
+        explicit FilePermissionsRestore(const QString &path,
+                                        FileSystem::FolderPermissions temporaryPermissions);
+
+        ~FilePermissionsRestore();
+
+    private:
+        QString _path;
+        FileSystem::FolderPermissions _initialPermissions;
+        bool _rollbackNeeded = false;
+    };
+
     struct OWNCLOUDSYNC_EXPORT FileLockingInfo {
         enum class Type { Unset = -1, Locked, Unlocked };
         QString path;

diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp
@@ -63,6 +63,9 @@ bool PropagateLocalRemove::removeRecursively(const QString &path)
     QStringList errors;
     QList<QPair<QString, bool>> deleted;
 #if !defined(Q_OS_MACOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_15
+    const auto fileInfo = QFileInfo{absolute};
+    const auto parentFolderPath = fileInfo.dir().absolutePath();
+    const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite};
     FileSystem::setFolderPermissions(absolute, FileSystem::FolderPermissions::ReadWrite);
 #endif
     bool success = FileSystem::removeRecursively(
@@ -126,10 +129,18 @@ void PropagateLocalRemove::start()
                 return;
             }
         } else {
-            if (FileSystem::fileExists(filename)
-                && !FileSystem::remove(filename, &removeError)) {
-                done(SyncFileItem::NormalError, removeError, ErrorCategory::GenericError);
-                return;
+            if (FileSystem::fileExists(filename)) {
+#if !defined(Q_OS_MACOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_15
+                const auto fileInfo = QFileInfo{filename};
+                const auto parentFolderPath = fileInfo.dir().absolutePath();
+
+                const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite};
+#endif
+
+                if (!FileSystem::remove(filename, &removeError)) {
+                    done(SyncFileItem::NormalError, removeError, ErrorCategory::GenericError);
+                    return;
+                }
             }
         }
     }
@@ -368,6 +379,8 @@ void PropagateLocalRename::start()
         };
 #endif
 
+        const auto folderPermissionsHandler = FileSystem::FilePermissionsRestore{existingFile, FileSystem::FolderPermissions::ReadWrite};
+
         emit propagator()->touchedFile(existingFile);
         emit propagator()->touchedFile(targetFile);
         if (QString renameError; !FileSystem::rename(existingFile, targetFile, &renameError)) {

diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp
@@ -769,6 +769,140 @@ private slots:
         QVERIFY(testFolderStatus.permissions() & std::filesystem::perms::owner_read);
         QVERIFY(!static_cast<bool>(testFolderStatus.permissions() & std::filesystem::perms::owner_write));
     }
+
+    void testDeleteChildItemsInReadOnlyFolder()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+
+        auto &remote = fakeFolder.remoteModifier();
+
+        remote.mkdir("readOnlyFolder");
+        remote.mkdir("readOnlyFolder/test");
+        remote.insert("readOnlyFolder/readOnlyFile.txt");
+
+        remote.find("readOnlyFolder")->permissions = RemotePermissions::fromServerString("M");
+        remote.find("readOnlyFolder/test")->permissions = RemotePermissions::fromServerString("m");
+        remote.find("readOnlyFolder/readOnlyFile.txt")->permissions = RemotePermissions::fromServerString("m");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        remote.remove("readOnlyFolder/test");
+        remote.remove("readOnlyFolder/readOnlyFile.txt");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        const auto ensureReadOnlyItem = [&fakeFolder] (const auto path) -> bool {
+            const auto itemStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + path).toStdWString());
+            return static_cast<bool>(itemStatus.permissions() & std::filesystem::perms::owner_read);
+        };
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder"));
+    }
+
+    void testRenameChildItemsInReadOnlyFolder()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+
+        auto &remote = fakeFolder.remoteModifier();
+
+        remote.mkdir("readOnlyFolder");
+        remote.mkdir("readOnlyFolder/test");
+        remote.insert("readOnlyFolder/readOnlyFile.txt");
+
+        remote.find("readOnlyFolder")->permissions = RemotePermissions::fromServerString("M");
+        remote.find("readOnlyFolder/test")->permissions = RemotePermissions::fromServerString("m");
+        remote.find("readOnlyFolder/readOnlyFile.txt")->permissions = RemotePermissions::fromServerString("m");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        remote.rename("readOnlyFolder/test", "readOnlyFolder/testRename");
+        remote.rename("readOnlyFolder/readOnlyFile.txt", "readOnlyFolder/readOnlyFileRename.txt");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        const auto ensureReadOnlyItem = [&fakeFolder] (const auto path) -> bool {
+            const auto itemStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + path).toStdWString());
+            return static_cast<bool>(itemStatus.permissions() & std::filesystem::perms::owner_read);
+        };
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder"));
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder/testRename"));
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder/readOnlyFileRename.txt"));
+    }
+
+    void testMoveChildItemsInReadOnlyFolder()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+
+        auto &remote = fakeFolder.remoteModifier();
+
+        remote.mkdir("readOnlyFolder");
+        remote.mkdir("readOnlyFolder/child");
+        remote.mkdir("readOnlyFolder/test");
+        remote.insert("readOnlyFolder/readOnlyFile.txt");
+
+        remote.find("readOnlyFolder")->permissions = RemotePermissions::fromServerString("M");
+        remote.find("readOnlyFolder/child")->permissions = RemotePermissions::fromServerString("m");
+        remote.find("readOnlyFolder/test")->permissions = RemotePermissions::fromServerString("m");
+        remote.find("readOnlyFolder/readOnlyFile.txt")->permissions = RemotePermissions::fromServerString("m");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        remote.rename("readOnlyFolder/test", "readOnlyFolder/child/test");
+        remote.rename("readOnlyFolder/readOnlyFile.txt", "readOnlyFolder/child/readOnlyFile.txt");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        const auto ensureReadOnlyItem = [&fakeFolder] (const auto path) -> bool {
+            const auto itemStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + path).toStdWString());
+            return static_cast<bool>(itemStatus.permissions() & std::filesystem::perms::owner_read);
+        };
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder"));
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder/child"));
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder/child/test"));
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder/child/readOnlyFile.txt"));
+    }
+
+    void testModifyChildItemsInReadOnlyFolder()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+
+        auto &remote = fakeFolder.remoteModifier();
+
+        remote.mkdir("readOnlyFolder");
+        remote.mkdir("readOnlyFolder/test");
+        remote.insert("readOnlyFolder/readOnlyFile.txt");
+
+        remote.find("readOnlyFolder")->permissions = RemotePermissions::fromServerString("M");
+        remote.find("readOnlyFolder/test")->permissions = RemotePermissions::fromServerString("m");
+        remote.find("readOnlyFolder/readOnlyFile.txt")->permissions = RemotePermissions::fromServerString("m");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        remote.insert("readOnlyFolder/test/newFile.txt");
+        remote.find("readOnlyFolder/test/newFile.txt")->permissions = RemotePermissions::fromServerString("m");
+        remote.mkdir("readOnlyFolder/test/newFolder");
+        remote.find("readOnlyFolder/test/newFolder")->permissions = RemotePermissions::fromServerString("m");
+        remote.appendByte("readOnlyFolder/readOnlyFile.txt");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        const auto ensureReadOnlyItem = [&fakeFolder] (const auto path) -> bool {
+            const auto itemStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + path).toStdWString());
+            return static_cast<bool>(itemStatus.permissions() & std::filesystem::perms::owner_read);
+        };
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder"));
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder/test"));
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder/readOnlyFile.txt"));
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder/test/newFile.txt"));
+        QVERIFY(ensureReadOnlyItem("/readOnlyFolder/newFolder"));
+    }
 #endif
 };