Do not remove files from a Group folder and its nested folders whe it is renamed or removed while not allowed.

Signed-off-by: alex-z <blackslayer4@gmail.com>

src/libsync/discovery.cpp

@@ -1295,8 +1295,11 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
             chopVirtualFileSuffix(serverOriginalPath);
         auto job = new RequestEtagJob(_discoveryData->_account, serverOriginalPath, this);
         connect(job, &RequestEtagJob::finishedWithResult, this, [=](const HttpResult<QByteArray> &etag) mutable {
-            if (!etag || (etag.get() != base._etag && !item->isDirectory()) || _discoveryData->isRenamed(originalPath)) {
-                qCInfo(lcDisco) << "Can't rename because the etag has changed or the directory is gone" << originalPath;
+            
+
+            if (!etag || (etag.get() != base._etag && !item->isDirectory()) || _discoveryData->isRenamed(originalPath)
+                || (isAnyParentBeingRestored(originalPath) && !isRename(originalPath))) {
+                qCInfo(lcDisco) << "Can't rename because the etag has changed or the directory is gone or we are restoring one of the file's parents." << originalPath;
                 // Can't be a rename, leave it as a new.
                 postProcessLocalNew();
             } else {
@@ -1436,7 +1439,7 @@ void ProcessDirectoryJob::processFileFinalize(
         job->setInsideEncryptedTree(isInsideEncryptedTree() || item->_isEncrypted);
         if (removed) {
             job->setParent(_discoveryData);
-            _discoveryData->_queuedDeletedDirectories[path._original] = job;
+            _discoveryData->enqueueDirectoryToDelete(path._original, job);
         } else {
             connect(job, &ProcessDirectoryJob::finished, this, &ProcessDirectoryJob::subJobFinished);
             _queuedJobs.push_back(job);
@@ -1550,7 +1553,8 @@ bool ProcessDirectoryJob::checkPermissions(const OCC::SyncFileItemPtr &item)
             // No permissions set
             return true;
         }
-        if (!perms.hasPermission(RemotePermissions::CanDelete)) {
+        if (!perms.hasPermission(RemotePermissions::CanDelete) || isAnyParentBeingRestored(item->_file))
+        {
             item->_instruction = CSYNC_INSTRUCTION_NEW;
             item->_direction = SyncFileItem::Down;
             item->_isRestoration = true;
@@ -1566,6 +1570,35 @@ bool ProcessDirectoryJob::checkPermissions(const OCC::SyncFileItemPtr &item)
     return true;
 }
 
+bool ProcessDirectoryJob::isAnyParentBeingRestored(const QString &file) const
+{
+    for (const auto &directoryNameToRestore : _discoveryData->_directoryNamesToRestoreOnPropagation) {
+        if (file.startsWith(QString(directoryNameToRestore + QLatin1Char('/')))) {
+            qCWarning(lcDisco) << "File" << file << " is within the tree that's being restored" << directoryNameToRestore;
+            return true;
+        }
+    }
+    return false;
+}
+
+bool ProcessDirectoryJob::isRename(const QString &originalPath) const
+{
+    return (originalPath.startsWith(_currentFolder._original)
+        && originalPath.lastIndexOf('/') == _currentFolder._original.size());
+
+    /* TODO: This was needed at some point to cover an edge case which I am no longer to reproduce and it might no longer be the case.
+    *  Still, leaving this here just in case the edge case is caught at some point in future.
+    * 
+    OCC::SyncJournalFileRecord base;
+    // are we allowed to rename?
+    if (!_discoveryData || !_discoveryData->_statedb || !_discoveryData->_statedb->getFileRecord(originalPath, &base)) {
+        return false;
+    }
+    qCWarning(lcDisco) << "isRename from" << originalPath << " to" << targetPath << " :"
+                       << base._remotePerm.hasPermission(RemotePermissions::CanRename);
+    return base._remotePerm.hasPermission(RemotePermissions::CanRename);
+    */
+}
 
 auto ProcessDirectoryJob::checkMovePermissions(RemotePermissions srcPerm, const QString &srcPath,
                                                bool isDirectory)

src/libsync/discovery.h

@@ -193,6 +193,10 @@ private:
      */
     bool checkPermissions(const SyncFileItemPtr &item);
 
+    bool isAnyParentBeingRestored(const QString &file) const;
+
+    bool isRename(const QString &originalPath) const;
+
     struct MovePermissionResult
     {
         // whether moving/renaming the source is ok

src/libsync/discoveryphase.cpp

@@ -203,6 +203,17 @@ QPair<bool, QByteArray> DiscoveryPhase::findAndCancelDeletedJob(const QString &o
     return { result, oldEtag };
 }
 
+void DiscoveryPhase::enqueueDirectoryToDelete(const QString &path, ProcessDirectoryJob* const directoryJob)
+{
+    _queuedDeletedDirectories[path] = directoryJob;
+
+    if (directoryJob->_dirItem && directoryJob->_dirItem->_isRestoration
+        && directoryJob->_dirItem->_direction == SyncFileItem::Down
+        && directoryJob->_dirItem->_instruction == CSYNC_INSTRUCTION_NEW) {
+        _directoryNamesToRestoreOnPropagation.push_back(path);
+    }
+}
+
 void DiscoveryPhase::startJob(ProcessDirectoryJob *job)
 {
     ENFORCE(!_currentRootJob);

src/libsync/discoveryphase.h

@@ -181,6 +181,8 @@ class DiscoveryPhase : public QObject
      */
     QMap<QString, SyncFileItemPtr> _deletedItem;
 
+    QVector<QString> _directoryNamesToRestoreOnPropagation;
+
     /** Maps the db-path of a deleted folder to its queued job.
      *
      * If a folder is deleted and must be recursed into, its job isn't
@@ -249,6 +251,8 @@ class DiscoveryPhase : public QObject
      */
     QPair<bool, QByteArray> findAndCancelDeletedJob(const QString &originalPath);
 
+    void enqueueDirectoryToDelete(const QString &path, ProcessDirectoryJob* const directoryJob);
+
 public:
     // input
     QString _localDir; // absolute path to the local directory. ends with '/'

test/testpermissions.cpp

@@ -216,13 +216,10 @@ private slots:
         currentLocalState = fakeFolder.currentLocalState();
         QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/cannotBeRemoved_PERM_WVN_.data"));
         QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_"));
-        // the subdirectory had delete permissions, so the contents were deleted
-        QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_"));
+        // the subdirectory had delete permissions, but, it was within the recovered directory, so must also get recovered
+        QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_"));
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
-        // restore
-        fakeFolder.remoteModifier().mkdir("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_");
-        fakeFolder.remoteModifier().insert("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data");
         applyPermissionsFromName(fakeFolder.remoteModifier());
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
@@ -499,16 +496,52 @@ private slots:
         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");
 
-
-        auto expectedState = fakeFolder.currentLocalState();
+        expectedState = fakeFolder.currentLocalState();
 
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), expectedState);
         QCOMPARE(fakeFolder.currentRemoteState(), expectedState);
     }
+
+    void testParentMoveNotAllowedChildrenRestored()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+        auto &lm = fakeFolder.localModifier();
+        auto &rm = fakeFolder.remoteModifier();
+        rm.mkdir("forbidden-move");
+        rm.mkdir("forbidden-move/sub1");
+        rm.insert("forbidden-move/sub1/file1.txt", 100);
+        rm.mkdir("forbidden-move/sub2");
+        rm.insert("forbidden-move/sub2/file2.txt", 100);
+
+        rm.find("forbidden-move")->permissions = RemotePermissions::fromServerString("WNCK");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        lm.rename("forbidden-move", "forbidden-move-new");
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        // verify that original folder did not get wiped (files are still there)
+        QVERIFY(fakeFolder.currentRemoteState().find("forbidden-move/sub1/file1.txt"));
+        QVERIFY(fakeFolder.currentRemoteState().find("forbidden-move/sub2/file2.txt"));
+
+        // verify that the duplicate folder has been created when trying to rename a folder that has its move permissions forbidden
+        QVERIFY(fakeFolder.currentRemoteState().find("forbidden-move-new/sub1/file1.txt"));
+        QVERIFY(fakeFolder.currentRemoteState().find("forbidden-move-new/sub2/file2.txt"));
+
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+    }
 };
 
 QTEST_GUILESS_MAIN(TestPermissions)