fix(delete): fix move to trash in read-only folders

src/common/filesystembase.cpp

@@ -381,8 +381,7 @@ bool FileSystem::fileExists(const QString &filename, const QFileInfo &fileInfo)
     // not valid. There needs to be one initialised here. Otherwise the incoming
     // fileInfo is re-used.
     if (fileInfo.filePath() != filename) {
-        QFileInfo myFI(filename);
-        re = myFI.exists();
+        re = QFileInfo::exists(filename);
     }
     return re;
 }

src/libsync/propagatorjobs.cpp

@@ -1,3 +1,3 @@
 /*
  * SPDX-FileCopyrightText: 2020 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2014 ownCloud GmbH
@@ -98,26 +98,60 @@
     }
 
     QString removeError;
-    if (_moveToTrash && propagator()->syncOptions()._vfs->mode() != OCC::Vfs::WindowsCfApi) {
-        if ((QDir(filename).exists() || FileSystem::fileExists(filename))
-            && !FileSystem::moveToTrash(filename, &removeError)) {
-            done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError);
-            return;
+    auto moveToTrashIsFeasible = true;
+    if (propagator()->syncOptions()._vfs->mode() != OCC::Vfs::WindowsCfApi) {
+        moveToTrashIsFeasible = false;
+    }
+    const auto fileInfo = QFileInfo{filename};
+    if (fileInfo.isDir()) {
+        try {
+            if (FileSystem::isFolderReadOnly(fileInfo.filesystemAbsolutePath())) {
+                moveToTrashIsFeasible = false;
+            }
+        }
+        catch (const std::filesystem::filesystem_error &e)
+        {
+            qCWarning(lcPropagateLocalRemove) << "exception when checking parent folder read only status" << e.what() << e.path1().c_str() << e.path2().c_str();
+        }
+        catch (const std::system_error &e)
+        {
+            qCWarning(lcPropagateLocalRemove) << "exception when checking parent folder read only status" << e.what();
+        }
+        catch (...)
+        {
+            qCWarning(lcPropagateLocalRemove) << "exception when checking parent folder read only status";
+        }
+    } else {
+        if (!FileSystem::isWritable(filename, fileInfo)) {
+            moveToTrashIsFeasible = false;
+        }
+    }
+    if (_moveToTrash && moveToTrashIsFeasible) {
+        if (FileSystem::fileExists(filename, fileInfo)) {
+            const auto parentFolderPath = fileInfo.dir().absolutePath();
+            const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite};
+
+            if (!FileSystem::moveToTrash(filename, &removeError)) {
+                qCWarning(lcPropagateLocalRemove()) << "move to trash failed" << filename << removeError;
+                done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError);
+                return;
+            }
+        } else {
+            qCWarning(lcPropagateLocalRemove()) << "move to trash failed" << filename << "was already deleted";
         }
     } else {
         if (_item->isDirectory()) {
-            if (QDir(filename).exists() && !removeRecursively(QString())) {
+            if (FileSystem::fileExists(filename, fileInfo) && !removeRecursively(QString())) {
                 done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError);
                 return;
             }
         } else {
-            if (FileSystem::fileExists(filename)) {
-                const auto fileInfo = QFileInfo{filename};
+            if (FileSystem::fileExists(filename, fileInfo)) {
                 const auto parentFolderPath = fileInfo.dir().absolutePath();
-
                 const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite};
 
                 if (!FileSystem::remove(filename, &removeError)) {
+                    qCWarning(lcPropagateLocalRemove()) << "remove failed" << filename << removeError;
                     done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError);
                     return;
                 }

test/testpermissions.cpp

@@ -97,9 +97,23 @@ private slots:
         QStandardPaths::setTestModeEnabled(true);
     }
 
+    void t7pl_data()
+    {
+        QTest::addColumn<bool>("moveToTrashEnabled");
+        QTest::newRow("move to trash") << true;
+        QTest::newRow("delete") << false;
+    }
+
     void t7pl()
     {
+        QFETCH(bool, moveToTrashEnabled);
+
         FakeFolder fakeFolder{ FileInfo() };
+
+        auto syncOptions = fakeFolder.syncEngine().syncOptions();
+        syncOptions._moveFilesToTrash = moveToTrashEnabled;
+        fakeFolder.syncEngine().setSyncOptions(syncOptions);
+
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
         // Some of this test depends on the order of discovery. With threading

test/testsyncengine.cpp

@@ -8,7 +8,7 @@
  * any purpose.
  */
 
 #include <QtTest>
 #include <QTextCodec>
 
 #include "syncenginetestutils.h"
@@ -219,8 +219,23 @@
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }
 
+
+    void testLocalDelete_data()
+    {
+        QTest::addColumn<bool>("moveToTrashEnabled");
+        QTest::newRow("move to trash") << true;
+        QTest::newRow("delete") << false;
+    }
+
     void testLocalDelete() {
+        QFETCH(bool, moveToTrashEnabled);
+
         FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
+
+        auto syncOptions = fakeFolder.syncEngine().syncOptions();
+        syncOptions._moveFilesToTrash = moveToTrashEnabled;
+        fakeFolder.syncEngine().setSyncOptions(syncOptions);
+
         ItemCompletedSpy completeSpy(fakeFolder);
         fakeFolder.remoteModifier().remove("A/a1");
         fakeFolder.syncOnce();
@@ -237,10 +252,23 @@
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }
 
+    void testLocalDeleteWithReuploadForNewLocalFiles_data()
+    {
+        QTest::addColumn<bool>("moveToTrashEnabled");
+        QTest::newRow("move to trash") << true;
+        QTest::newRow("delete") << false;
+    }
+
     void testLocalDeleteWithReuploadForNewLocalFiles()
     {
+        QFETCH(bool, moveToTrashEnabled);
+
         FakeFolder fakeFolder{FileInfo{}};
 
+        auto syncOptions = fakeFolder.syncEngine().syncOptions();
+        syncOptions._moveFilesToTrash = moveToTrashEnabled;
+        fakeFolder.syncEngine().setSyncOptions(syncOptions);
+
         // create folders hierarchy with some nested dirs and files
         fakeFolder.localModifier().mkdir("A");
         fakeFolder.localModifier().insert("A/existingfile_A.txt", 100);
@@ -1566,9 +1594,23 @@
         QCOMPARE(fileThirdSync->lastModified.toSecsSinceEpoch(), CURRENT_MTIME);
     }
 
+    void testFolderRemovalWithCaseClash_data()
+    {
+        QTest::addColumn<bool>("moveToTrashEnabled");
+        QTest::newRow("move to trash") << true;
+        QTest::newRow("delete") << false;
+    }
+
     void testFolderRemovalWithCaseClash()
     {
-        FakeFolder fakeFolder{ FileInfo{} };
+        QFETCH(bool, moveToTrashEnabled);
+
+        FakeFolder fakeFolder{FileInfo{}};
+
+        auto syncOptions = fakeFolder.syncEngine().syncOptions();
+        syncOptions._moveFilesToTrash = moveToTrashEnabled;
+        fakeFolder.syncEngine().setSyncOptions(syncOptions);
+
         fakeFolder.remoteModifier().mkdir("A");
         fakeFolder.remoteModifier().mkdir("toDelete");
         fakeFolder.remoteModifier().insert("A/file");
@@ -1851,8 +1893,17 @@
         }
     }
 
+    void testServer_caseClash_createConflict_thenRemoveOneRemoteFile_data()
+    {
+        QTest::addColumn<bool>("moveToTrashEnabled");
+        QTest::newRow("move to trash") << true;
+        QTest::newRow("delete") << false;
+    }
+
     void testServer_caseClash_createConflict_thenRemoveOneRemoteFile()
     {
+        QFETCH(bool, moveToTrashEnabled);
+
         constexpr auto testLowerCaseFile = "test";
         constexpr auto testUpperCaseFile = "TEST";
 
@@ -1864,6 +1915,10 @@
 
         FakeFolder fakeFolder{FileInfo{}};
 
+        auto syncOptions = fakeFolder.syncEngine().syncOptions();
+        syncOptions._moveFilesToTrash = moveToTrashEnabled;
+        fakeFolder.syncEngine().setSyncOptions(syncOptions);
+
         fakeFolder.remoteModifier().insert("otherFile.txt");
         fakeFolder.remoteModifier().insert(testLowerCaseFile);
         fakeFolder.remoteModifier().insert(testUpperCaseFile);