Discovery: If a move is forbidden, restore the source

Previously the source was deleted (or attempted to be deleted), even if
the new location was not acceptable for upload. This could make data
unavilable on the server.

For #7410

diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
index d08adbb79f4612b6c8fd3809bf7ec42fe74a98fd..de5704588c40278979ca37e21a1bca6d5426f11d 100644 (file)
--- a/src/libsync/discovery.cpp
+++ b/src/libsync/discovery.cpp
@@ -654,7 +654,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         // conflict we don't need to recurse into it. (local c1.owncloud, c1/ ; remote: c1)
         if (item->_instruction == CSYNC_INSTRUCTION_CONFLICT && !item->isDirectory())
             recurse = false;
-        if (_queryLocal != NormalQuery && _queryServer != NormalQuery && !item->_isRestoration)
+        if (_queryLocal != NormalQuery && _queryServer != NormalQuery)
             recurse = false;
 
         auto recurseQueryLocal = _queryLocal == ParentNotChanged ? ParentNotChanged : localEntry.isDirectory || item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist;
@@ -689,13 +689,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
             item->_type = ItemTypeVirtualFile;
         } else if (!serverModified) {
             // Removed locally: also remove on the server.
-            if (_dirItem && _dirItem->_isRestoration && _dirItem->_instruction == CSYNC_INSTRUCTION_NEW) {
-                // Also restore everything
-                item->_instruction = CSYNC_INSTRUCTION_NEW;
-                item->_direction = SyncFileItem::Down;
-                item->_isRestoration = true;
-                item->_errorString = tr("Not allowed to remove, restoring");
-            } else if (!dbEntry._serverHasIgnoredFiles) {
+            if (!dbEntry._serverHasIgnoredFiles) {
                 item->_instruction = CSYNC_INSTRUCTION_REMOVE;
                 item->_direction = SyncFileItem::Up;
             }
@@ -900,20 +894,48 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
             return false;
         }
 
-        // Check local permission if we are allowed to put move the file here
-        // Technically we should use the one from the server, but we'll assume it is the same
-        if (!checkMovePermissions(base._remotePerm, originalPath, item->isDirectory())) {
-            qCInfo(lcDisco) << "Not a move, no permission to rename base file";
-            return false;
-        }
-
         return true;
     };
 
-    // Finally make it a NEW or a RENAME
+    // If it's not a move it's just a local-NEW
     if (!moveCheck()) {
        postProcessLocalNew();
     } else {
+        // Check local permission if we are allowed to put move the file here
+        // Technically we should use the permissions from the server, but we'll assume it is the same
+        auto movePerms = checkMovePermissions(base._remotePerm, originalPath, item->isDirectory());
+        if (!movePerms.sourceOk || !movePerms.destinationOk) {
+            qCInfo(lcDisco) << "Move without permission to rename base file, "
+                            << "source:" << movePerms.sourceOk
+                            << ", target:" << movePerms.destinationOk
+                            << ", targetNew:" << movePerms.destinationNewOk;
+
+            // If we can create the destination, do that.
+            // Permission errors on the destination will be handled by checkPermissions later.
+            postProcessLocalNew();
+            finalize();
+
+            // If the destination upload will work, we're fine with the source deletion.
+            // If the source deletion can't work, checkPermissions will error.
+            if (movePerms.destinationNewOk)
+                return;
+
+            // Here we know the new location can't be uploaded: must prevent the source delete.
+            // Two cases: either the source item was already processed or not.
+            auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath);
+            if (wasDeletedOnClient.first) {
+                // More complicated. The REMOVE is canceled. Restore will happen next sync.
+                qCInfo(lcDisco) << "Undid remove instruction on source" << originalPath;
+                _discoveryData->_statedb->deleteFileRecord(originalPath, true);
+                _discoveryData->_anotherSyncNeeded = true;
+            } else {
+                // Signal to future checkPermissions() to forbid the REMOVE and set to restore instead
+                qCInfo(lcDisco) << "Preventing future remove on source" << originalPath;
+                _discoveryData->_forbiddenDeletes[originalPath + '/'] = true;
+            }
+            return;
+        }
+
         auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath);
 
         auto processRename = [item, originalPath, base, this](PathTuple &path) {
@@ -1084,8 +1106,12 @@ void ProcessDirectoryJob::processFileFinalize(
 
     if (item->isDirectory() && item->_instruction == CSYNC_INSTRUCTION_SYNC)
         item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
-    if (!checkPermissions(item))
+    if (checkPermissions(item)) {
+        if (item->_isRestoration && item->isDirectory())
+            recurse = true;
+    } else {
         recurse = false;
+    }
     if (recurse) {
         auto job = new ProcessDirectoryJob(path, item, recurseQueryLocal, recurseQueryServer, this);
         if (item->_instruction == CSYNC_INSTRUCTION_REMOVE) {
@@ -1185,6 +1211,20 @@ bool ProcessDirectoryJob::checkPermissions(const OCC::SyncFileItemPtr &item)
         break;
     }
     case CSYNC_INSTRUCTION_REMOVE: {
+        QString fileSlash = item->_file + '/';
+        auto forbiddenIt = _discoveryData->_forbiddenDeletes.upperBound(fileSlash);
+        if (forbiddenIt != _discoveryData->_forbiddenDeletes.begin())
+            forbiddenIt -= 1;
+        if (forbiddenIt != _discoveryData->_forbiddenDeletes.end()
+            && fileSlash.startsWith(forbiddenIt.key())) {
+
+            qCWarning(lcDisco) << "checkForPermission: RESTORING" << item->_file;
+            item->_instruction = CSYNC_INSTRUCTION_NEW;
+            item->_direction = SyncFileItem::Down;
+            item->_isRestoration = true;
+            item->_errorString = tr("Moved to invalid target, restoring");
+            return true; // restore sub items
+        }
         const auto perms = item->_remotePerm;
         if (perms.isNull()) {
             // No permissions set
@@ -1207,8 +1247,9 @@ bool ProcessDirectoryJob::checkPermissions(const OCC::SyncFileItemPtr &item)
 }
 
 
-bool ProcessDirectoryJob::checkMovePermissions(RemotePermissions srcPerm, const QString &srcPath,
+auto ProcessDirectoryJob::checkMovePermissions(RemotePermissions srcPerm, const QString &srcPath,
                                                bool isDirectory)
+    -> MovePermissionResult
 {
     auto destPerms = !_rootPermissions.isNull() ? _rootPermissions
                                                 : _dirItem ? _dirItem->_remotePerm : _rootPermissions;
@@ -1218,12 +1259,15 @@ bool ProcessDirectoryJob::checkMovePermissions(RemotePermissions srcPerm, const
         && srcPath.lastIndexOf('/') == _currentFolder._original.size();
     // Check if we are allowed to move to the destination.
     bool destinationOK = true;
-    if (isRename || destPerms.isNull()) {
-        // no need to check for the destination dir permission
-        destinationOK = true;
+    bool destinationNewOK = true;
+    if (destPerms.isNull()) {
     } else if (isDirectory && !destPerms.hasPermission(RemotePermissions::CanAddSubDirectories)) {
-        destinationOK = false;
+        destinationNewOK = false;
     } else if (!isDirectory && !destPerms.hasPermission(RemotePermissions::CanAddFile)) {
+        destinationNewOK = false;
+    }
+    if (!isRename && !destinationNewOK) {
+        // no need to check for the destination dir permission for renames
         destinationOK = false;
     }
 
@@ -1235,16 +1279,7 @@ bool ProcessDirectoryJob::checkMovePermissions(RemotePermissions srcPerm, const
         // We are not allowed to move or rename this file
         sourceOK = false;
     }
-    if (!sourceOK || !destinationOK) {
-        qCInfo(lcDisco) << "Not a move because permission does not allow it." << sourceOK << destinationOK;
-        if (!sourceOK) {
-            // This is the behavior that we had in the client <= 2.5.
-            // but that might not be needed anymore
-            _discoveryData->_statedb->avoidRenamesOnNextSync(srcPath);
-        }
-        return false;
-    }
-    return true;
+    return MovePermissionResult{sourceOK, destinationOK, destinationNewOK};
 }
 
 void ProcessDirectoryJob::subJobFinished()

diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h
index 20d27b73df480fe768c07aec8e4d4959c2d1f1a5..96295c9dbeb35c988a82b81668ca8ab1c7ad3603 100644 (file)
--- a/src/libsync/discovery.h
+++ b/src/libsync/discovery.h
@@ -167,11 +167,21 @@ private:
      */
     bool checkPermissions(const SyncFileItemPtr &item);
 
+    struct MovePermissionResult
+    {
+        // whether moving/renaming the source is ok
+        bool sourceOk = false;
+        // whether the destination accepts (always true for renames)
+        bool destinationOk = false;
+        // whether creating a new file/dir in the destination is ok
+        bool destinationNewOk = false;
+    };
+
     /**
      * Check if the move is of a specified file within this directory is allowed.
      * Return true if it is allowed, false otherwise
      */
-    bool checkMovePermissions(RemotePermissions srcPerm, const QString &srcPath, bool isDirectory);
+    MovePermissionResult checkMovePermissions(RemotePermissions srcPerm, const QString &srcPath, bool isDirectory);
 
     void processBlacklisted(const PathTuple &, const LocalInfo &, const SyncJournalFileRecord &dbEntry);
     void subJobFinished();

diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h
index beddbecc5c7e67cff816e6cf1799ec11808b8e4f..8fbaafc7c9761791e6d7dd4021a3ccadea6fbc5b 100644 (file)
--- a/src/libsync/discoveryphase.h
+++ b/src/libsync/discoveryphase.h
@@ -155,12 +155,22 @@ class DiscoveryPhase : public QObject
     QMap<QString, QString> _renamedItemsRemote;
     QMap<QString, QString> _renamedItemsLocal;
 
+    // set of paths that should not be removed even though they are removed locally:
+    // there was a move to an invalid destination and now the source should be restored
+    //
+    // This applies recursively to subdirectories.
+    // All entries should have a trailing slash (even files), so lookup with
+    // lowerBound() is reliable.
+    //
+    // The value of this map doesn't matter.
+    QMap<QString, bool> _forbiddenDeletes;
+
     /** Returns whether the db-path has been renamed locally or on the remote.
      *
      * Useful for avoiding processing of items that have already been claimed in
      * a rename (would otherwise be discovered as deletions).
      */
-    bool isRenamed(const QString &p) { return _renamedItemsLocal.contains(p) || _renamedItemsRemote.contains(p); }
+    bool isRenamed(const QString &p) const { return _renamedItemsLocal.contains(p) || _renamedItemsRemote.contains(p); }
 
     int _currentlyActiveJobs = 0;
 
@@ -217,6 +227,7 @@ public:
 
     // output
     QByteArray _dataFingerprint;
+    bool _anotherSyncNeeded = false;
 
 signals:
     void fatalError(const QString &errorString);

diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h
index 8c874a019d1b91f3a630fa7a62841fdbfa73097e..2cc537d5b7d861178ace7226e726566ed5738529 100644 (file)
--- a/src/libsync/owncloudpropagator.h
+++ b/src/libsync/owncloudpropagator.h
@@ -191,7 +191,7 @@ public:
             return false;
         }
         const char *instruction_str = csync_instruction_str(_item->_instruction);
-        qCInfo(lcPropagator) << "Starting" << instruction_str << "propagation of" << _item->_file << "by" << this;
+        qCInfo(lcPropagator) << "Starting" << instruction_str << "propagation of" << _item->destination() << "by" << this;
 
         _state = Running;
         QMetaObject::invokeMethod(this, "start"); // We could be in a different thread (neon jobs)

diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp
index 4e75da8e0e16497d9779a05c924020b2cceb2bf2..33ad10c03bc0f912376aab2df98b5a2aabb27e2c 100644 (file)
--- a/src/libsync/syncengine.cpp
+++ b/src/libsync/syncengine.cpp
@@ -738,6 +738,10 @@ void SyncEngine::slotDiscoveryFinished()
         restoreOldFiles(_syncItems);
     }
 
+    if (_discoveryPhase->_anotherSyncNeeded && _anotherSyncNeeded == NoFollowUpSync) {
+        _anotherSyncNeeded = ImmediateFollowUp;
+    }
+
     // Sort items per destination
     std::sort(_syncItems.begin(), _syncItems.end());
 

diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h
index 276e3443525467a633b5383494bccd8aaf4febff..01e385323cb2f3facac119c7e050ea3007756048 100644 (file)
--- a/test/syncenginetestutils.h
+++ b/test/syncenginetestutils.h
@@ -581,6 +581,8 @@ public:
         QString fileName = getFilePathFromUrl(request.url());
         Q_ASSERT(!fileName.isEmpty());
         fileInfo = remoteRootFileInfo.find(fileName);
+        if (!fileInfo)
+            qWarning() << "Could not find file" << fileName << "on the remote";
         QMetaObject::invokeMethod(this, "respond", Qt::QueuedConnection);
     }
 

diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp
index ad02ca0a9bb8b34d4d638be36f8b2b1ec9007ee0..6640d50f2b2ab9ba7b4b6dddf79f34cc68f92302 100644 (file)
--- a/test/testpermissions.cpp
+++ b/test/testpermissions.cpp
@@ -41,6 +41,36 @@ static void assertCsyncJournalOk(SyncJournalDb &journal)
 #endif
 }
 
+SyncFileItemPtr findItem(const QSignalSpy &spy, const QString &path)
+{
+    for (const QList<QVariant> &args : spy) {
+        auto item = args[0].value<SyncFileItemPtr>();
+        if (item->destination() == path)
+            return item;
+    }
+    return SyncFileItemPtr(new SyncFileItem);
+}
+
+SyncFileItemPtr findDiscoveryItem(const SyncFileItemVector &spy, const QString &path)
+{
+    for (const auto &item : spy) {
+        if (item->destination() == path)
+            return item;
+    }
+    return SyncFileItemPtr(new SyncFileItem);
+}
+
+bool itemInstruction(const QSignalSpy &spy, const QString &path, const csync_instructions_e instr)
+{
+    auto item = findItem(spy, path);
+    return item->_instruction == instr;
+}
+
+bool discoveryInstruction(const SyncFileItemVector &spy, const QString &path, const csync_instructions_e instr)
+{
+    auto item = findDiscoveryItem(spy, path);
+    return item->_instruction == instr;
+}
 
 class TestPermissions : public QObject
 {
@@ -188,7 +218,16 @@ private slots:
         assertCsyncJournalOk(fakeFolder.syncJournal());
         currentLocalState = fakeFolder.currentLocalState();
         QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/cannotBeRemoved_PERM_WVN_.data"));
-        QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.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_"));
+        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());
 
 
@@ -203,14 +242,23 @@ private slots:
         currentLocalState = fakeFolder.currentLocalState();
 
         // old name restored
-        QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_"));
-        QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data"));
+        QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_"));
+        // contents moved (had move permissions)
+        QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_"));
+        QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data"));
 
         // new still exist  (and is uploaded)
         QVERIFY(currentLocalState.find("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data"));
 
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
+        // restore for further tests
+        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());
+
 
         //######################################################################
         qInfo( "rename a directory in a read only folder and move a directory to a read-only" );
@@ -220,6 +268,8 @@ private slots:
         QVERIFY(fakeFolder.syncOnce());
         assertCsyncJournalOk(fakeFolder.syncJournal());
 
+        QVERIFY(fakeFolder.currentLocalState().find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data" ));
+
         //1. rename a directory in a read only folder
         //Missing directory should be restored
         //new directory should stay but not be uploaded
@@ -234,6 +284,8 @@ private slots:
 
         //1.
         // old name restored
+        QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_" ));
+        // including contents
         QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data" ));
         // new still exist
         QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/newname_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data" ));
@@ -257,6 +309,10 @@ private slots:
         //######################################################################
         qInfo( "multiple restores of a file create different conflict files" );
 
+        fakeFolder.remoteModifier().insert("readonlyDirectory_PERM_M_/cannotBeModified_PERM_DVN_.data");
+        applyPermissionsFromName(fakeFolder.remoteModifier());
+        QVERIFY(fakeFolder.syncOnce());
+
         editReadOnly("readonlyDirectory_PERM_M_/cannotBeModified_PERM_DVN_.data");
         fakeFolder.localModifier().setContents("readonlyDirectory_PERM_M_/cannotBeModified_PERM_DVN_.data", 's');
         //do the sync
@@ -286,6 +342,118 @@ private slots:
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }
 
+    static void setAllPerm(FileInfo *fi, RemotePermissions perm)
+    {
+        fi->permissions = perm;
+        for (auto &subFi : fi->children)
+            setAllPerm(&subFi, perm);
+    }
+
+    // What happens if the source can't be moved or the target can't be created?
+    void testForbiddenMoves()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+
+        auto &lm = fakeFolder.localModifier();
+        auto &rm = fakeFolder.remoteModifier();
+        rm.mkdir("allowed");
+        rm.mkdir("norename");
+        rm.mkdir("nomove");
+        rm.mkdir("nocreatefile");
+        rm.mkdir("nocreatedir");
+        rm.mkdir("zallowed"); // order of discovery matters
+
+        rm.mkdir("allowed/sub");
+        rm.mkdir("allowed/sub2");
+        rm.insert("allowed/file");
+        rm.insert("allowed/sub/file");
+        rm.insert("allowed/sub2/file");
+        rm.mkdir("norename/sub");
+        rm.insert("norename/file");
+        rm.insert("norename/sub/file");
+        rm.mkdir("nomove/sub");
+        rm.insert("nomove/file");
+        rm.insert("nomove/sub/file");
+        rm.mkdir("zallowed/sub");
+        rm.mkdir("zallowed/sub2");
+        rm.insert("zallowed/file");
+        rm.insert("zallowed/sub/file");
+        rm.insert("zallowed/sub2/file");
+
+        setAllPerm(rm.find("norename"), RemotePermissions::fromServerString("WDVCK"));
+        setAllPerm(rm.find("nomove"), RemotePermissions::fromServerString("WDNCK"));
+        setAllPerm(rm.find("nocreatefile"), RemotePermissions::fromServerString("WDNVK"));
+        setAllPerm(rm.find("nocreatedir"), RemotePermissions::fromServerString("WDNVC"));
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        // Renaming errors
+        lm.rename("norename/file", "norename/file_renamed");
+        lm.rename("norename/sub", "norename/sub_renamed");
+        // Moving errors
+        lm.rename("nomove/file", "allowed/file_moved");
+        lm.rename("nomove/sub", "allowed/sub_moved");
+        // Createfile errors
+        lm.rename("allowed/file", "nocreatefile/file");
+        lm.rename("zallowed/file", "nocreatefile/zfile");
+        lm.rename("allowed/sub", "nocreatefile/sub"); // TODO: probably forbidden because it contains file children?
+        // Createdir errors
+        lm.rename("allowed/sub2", "nocreatedir/sub2");
+        lm.rename("zallowed/sub2", "nocreatedir/zsub2");
+
+        // also hook into discovery!!
+        SyncFileItemVector discovery;
+        connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToPropagate, this, [&discovery](auto v) { discovery = v; });
+        QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
+        QVERIFY(!fakeFolder.syncOnce());
+
+        // if renaming doesn't work, just delete+create
+        QVERIFY(itemInstruction(completeSpy, "norename/file", CSYNC_INSTRUCTION_REMOVE));
+        QVERIFY(itemInstruction(completeSpy, "norename/sub", CSYNC_INSTRUCTION_NONE));
+        QVERIFY(discoveryInstruction(discovery, "norename/sub", CSYNC_INSTRUCTION_REMOVE));
+        QVERIFY(itemInstruction(completeSpy, "norename/file_renamed", CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "norename/sub_renamed", CSYNC_INSTRUCTION_NEW));
+        // the contents can _move_
+        QVERIFY(itemInstruction(completeSpy, "norename/sub_renamed/file", CSYNC_INSTRUCTION_RENAME));
+
+        // simiilarly forbidding moves becomes delete+create
+        QVERIFY(itemInstruction(completeSpy, "nomove/file", CSYNC_INSTRUCTION_REMOVE));
+        QVERIFY(itemInstruction(completeSpy, "nomove/sub", CSYNC_INSTRUCTION_NONE));
+        QVERIFY(discoveryInstruction(discovery, "nomove/sub", CSYNC_INSTRUCTION_REMOVE));
+        // nomove/sub/file is removed as part of the dir
+        QVERIFY(itemInstruction(completeSpy, "allowed/file_moved", CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "allowed/sub_moved", CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "allowed/sub_moved/file", CSYNC_INSTRUCTION_NEW));
+
+        // when moving to an invalid target, the targets should be an error
+        QVERIFY(itemInstruction(completeSpy, "nocreatefile/file", CSYNC_INSTRUCTION_ERROR));
+        QVERIFY(itemInstruction(completeSpy, "nocreatefile/zfile", CSYNC_INSTRUCTION_ERROR));
+        QVERIFY(itemInstruction(completeSpy, "nocreatefile/sub", CSYNC_INSTRUCTION_RENAME)); // TODO: What does a real server say?
+        QVERIFY(itemInstruction(completeSpy, "nocreatedir/sub2", CSYNC_INSTRUCTION_ERROR));
+        QVERIFY(itemInstruction(completeSpy, "nocreatedir/zsub2", CSYNC_INSTRUCTION_ERROR));
+
+        // and the sources of the invalid moves should be restored, not deleted
+        // (depending on the order of discovery a follow-up sync is needed)
+        QVERIFY(itemInstruction(completeSpy, "allowed/file", CSYNC_INSTRUCTION_NONE));
+        QVERIFY(itemInstruction(completeSpy, "allowed/sub2", CSYNC_INSTRUCTION_NONE));
+        QVERIFY(itemInstruction(completeSpy, "zallowed/file", CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "zallowed/sub2", CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "zallowed/sub2/file", CSYNC_INSTRUCTION_NEW));
+        QCOMPARE(fakeFolder.syncEngine().isAnotherSyncNeeded(), ImmediateFollowUp);
+
+        // A follow-up sync will restore allowed/file and allowed/sub2 and maintain the createdir/file errors
+        completeSpy.clear();
+        QVERIFY(!fakeFolder.syncOnce());
+
+        QVERIFY(itemInstruction(completeSpy, "nocreatefile/file", CSYNC_INSTRUCTION_ERROR));
+        QVERIFY(itemInstruction(completeSpy, "nocreatefile/zfile", CSYNC_INSTRUCTION_ERROR));
+        QVERIFY(itemInstruction(completeSpy, "nocreatedir/sub2", CSYNC_INSTRUCTION_ERROR));
+        QVERIFY(itemInstruction(completeSpy, "nocreatedir/zsub2", CSYNC_INSTRUCTION_ERROR));
+
+        QVERIFY(itemInstruction(completeSpy, "allowed/file", CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "allowed/sub2", CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "allowed/sub2/file", CSYNC_INSTRUCTION_NEW));
+    }
 };
 
 QTEST_GUILESS_MAIN(TestPermissions)

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
index ae1e16efc6e88b5badc9dd55264985580046ad56..c3bc049914f57275d12717d4a25d8821cd81689b 100644 (file)
--- a/test/testsyncengine.cpp
+++ b/test/testsyncengine.cpp
@@ -11,22 +11,34 @@
 
 using namespace OCC;
 
-bool itemDidComplete(const QSignalSpy &spy, const QString &path)
+SyncFileItemPtr findItem(const QSignalSpy &spy, const QString &path)
 {
-    for(const QList<QVariant> &args : spy) {
+    for (const QList<QVariant> &args : spy) {
         auto item = args[0].value<SyncFileItemPtr>();
         if (item->destination() == path)
-            return item->_instruction != CSYNC_INSTRUCTION_NONE && item->_instruction != CSYNC_INSTRUCTION_UPDATE_METADATA;
+            return item;
+    }
+    return SyncFileItemPtr(new SyncFileItem);
+}
+
+bool itemDidComplete(const QSignalSpy &spy, const QString &path)
+{
+    if (auto item = findItem(spy, path)) {
+        return item->_instruction != CSYNC_INSTRUCTION_NONE && item->_instruction != CSYNC_INSTRUCTION_UPDATE_METADATA;
     }
     return false;
 }
 
+bool itemInstruction(const QSignalSpy &spy, const QString &path, const csync_instructions_e instr)
+{
+    auto item = findItem(spy, path);
+    return item->_instruction == instr;
+}
+
 bool itemDidCompleteSuccessfully(const QSignalSpy &spy, const QString &path)
 {
-    for(const QList<QVariant> &args : spy) {
-        auto item = args[0].value<SyncFileItemPtr>();
-        if (item->destination() == path)
-            return item->_status == SyncFileItem::Success;
+    if (auto item = findItem(spy, path)) {
+        return item->_status == SyncFileItem::Success;
     }
     return false;
 }