nextcloud
/
desktop
mirror of https://github.com/nextcloud/desktop
0
0
Fork 0
Code Issues Releases Wiki Activity

Merge pull request #4292 from nextcloud/bugfix/group-folder-files-erased-when-rename-or-delete-folder 

Do not remove files from a Group folder and its nested folders when it is renamed or removed while not allowed.
This commit is contained in:
Matthieu Gallien 2022-03-09 11:43:08 +01:00 committed by GitHub
parent c347b4713a 01eb050cd8
commit 4e5ef728bc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 98 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
src/libsync/discovery.cpp
View File

@ -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)

4
src/libsync/discovery.h
View File

@ -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

11
src/libsync/discoveryphase.cpp
View File

@ -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);

4
src/libsync/discoveryphase.h
View File

@ -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 '/'

51
test/testpermissions.cpp
View File

@ -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,15 +496,51 @@ private slots:
lm.rename("changeonly/sub2/filetorname2a", "changeonly/sub2/aaa2_renamed");
lm.rename("changeonly/sub2/filetorname2z", "changeonly/sub2/zzz2_renamed");
lm.rename("changeonly/sub1", "changeonly/aaa");
lm.rename("changeonly/sub2", "changeonly/zzz");
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");
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());
}
};