From: alex-z Date: Wed, 16 Feb 2022 11:14:47 +0000 (+0200) Subject: Do not remove files from a Group folder and its nested folders whe it is renamed... X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~17^2~120^2 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=01eb050cd85457e7e222bea9cad55e22af9426eb;p=nextcloud-desktop.git 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 --- diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 80d57201b..c9a3b1d53 100644 --- a/src/libsync/discovery.cpp +++ b/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 &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) diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h index 661315916..97c11d9cf 100644 --- a/src/libsync/discovery.h +++ b/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 diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index 0e842906e..38f941dde 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -203,6 +203,17 @@ QPair 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); diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h index f5950e8dc..cd210a644 100644 --- a/src/libsync/discoveryphase.h +++ b/src/libsync/discoveryphase.h @@ -181,6 +181,8 @@ class DiscoveryPhase : public QObject */ QMap _deletedItem; + QVector _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 findAndCancelDeletedJob(const QString &originalPath); + void enqueueDirectoryToDelete(const QString &path, ProcessDirectoryJob* const directoryJob); + public: // input QString _localDir; // absolute path to the local directory. ends with '/' diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 6836c331f..373b66ade 100644 --- a/test/testpermissions.cpp +++ b/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)