Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/gui/tray/usermodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1280,7 +1280,7 @@
return item->_status == SyncFileItem::Conflict && !Utility::isConflictFile(item->_file);
}

void User::processCompletedSyncItem(const Folder *folder, const SyncFileItemPtr &item)

Check warning on line 1283 in src/gui/tray/usermodel.cpp

View workflow job for this annotation

GitHub Actions / build

src/gui/tray/usermodel.cpp:1283:12 [readability-function-cognitive-complexity]

function 'processCompletedSyncItem' has cognitive complexity of 38 (threshold 25)
{
if (item->_direction == SyncFileItem::Down && item->_instruction == CSYNC_INSTRUCTION_SYNC) {
qCDebug(lcActivity) << "Skipping activities about changes coming from server.";
Expand Down Expand Up @@ -1390,6 +1390,11 @@
activity._links = {buttonActivityLink};
}
_activityModel->addErrorToActivityList(activity, ActivityListModel::ErrorType::SyncError);
} else if (!item->_errorString.isEmpty()) {
// The item was ignored for a concrete reason (e.g. it lives in a read-only
// folder and cannot be uploaded). Surface it passively in the Not-synced list
// so the user can see why it did not sync.
_activityModel->addErrorToActivityList(activity, ActivityListModel::ErrorType::SyncError);
}
}
}
Expand Down
14 changes: 12 additions & 2 deletions src/libsync/discovery.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1637,6 +1637,14 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
return;
}

// The move target is in a read-only folder so it can't be uploaded, but its data is safe
// at the source (restored below). Emitting remnantReadOnlyFolderDiscovered() schedules this
// local copy for deletion at the end of the sync, removing the duplicate. A genuinely new
// local file never reaches this move branch and is kept by checkPermissions (#7797/#10099).
if (!localEntry.isVirtualFile && item->_instruction == CSYNC_INSTRUCTION_IGNORE) {
emit _discoveryData->remnantReadOnlyFolderDiscovered(item);
}

// 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);
Expand Down Expand Up @@ -2017,13 +2025,15 @@ bool ProcessDirectoryJob::checkPermissions(const OCC::SyncFileItemPtr &item)
qCWarning(lcDisco) << "checkForPermission: Not allowed because you don't have permission to add subfolders to that folder:" << item->_file;
item->_instruction = CSYNC_INSTRUCTION_IGNORE;
item->_errorString = tr("Not allowed because you don't have permission to add subfolders to that folder");
emit _discoveryData->remnantReadOnlyFolderDiscovered(item);
// Do NOT schedule the new local folder for deletion: it may contain data the user just
// created and that exists nowhere else. (see read-only folder regression, issues #7797/#10099).
return false;
} else if (!item->isDirectory() && !perms.hasPermission(RemotePermissions::CanAddFile)) {
qCWarning(lcDisco) << "checkForPermission: Not allowed because you don't have permission to add files in that folder:" << item->_file;
item->_instruction = CSYNC_INSTRUCTION_IGNORE;
item->_errorString = tr("Not allowed because you don't have permission to add files in that folder");
emit _discoveryData->remnantReadOnlyFolderDiscovered(item);
// Do NOT schedule the new local file for deletion: it may be data the user just created
// and that exists nowhere else. (see read-only folder regression, issues #7797/#10099).
return false;
}
break;
Expand Down
4 changes: 1 addition & 3 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,7 @@ nextcloud_add_test(Blacklist)
nextcloud_add_test(LocalDiscovery)
nextcloud_add_test(RemoteDiscovery)

if (NOT APPLE)
nextcloud_add_test(Permissions)
endif()
nextcloud_add_test(Permissions)

nextcloud_add_test(SelectiveSync)
nextcloud_add_test(DatabaseError)
Expand Down
91 changes: 88 additions & 3 deletions test/testpermissions.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
/*
* SPDX-FileCopyrightText: 2022 Nextcloud GmbH and Nextcloud contributors
* SPDX-FileCopyrightText: 2018 ownCloud, Inc.
Expand Down Expand Up @@ -297,9 +297,12 @@
currentLocalState = fakeFolder.currentLocalState();

//6.
// The file should not exist on the remote, and not be there
QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_MG_/newFile_PERM_GWDNV_.data"));
// The file cannot be uploaded to the read-only folder, but it MUST be kept locally:
// it is local-only data and silently deleting it would lose it (issues #7797/#10099).
QVERIFY(currentLocalState.find("readonlyDirectory_PERM_MG_/newFile_PERM_GWDNV_.data"));
QVERIFY(!fakeFolder.currentRemoteState().find("readonlyDirectory_PERM_MG_/newFile_PERM_GWDNV_.data"));
// Remove the kept local-only file so the following sections can compare local and remote.
removeReadOnly("readonlyDirectory_PERM_MG_/newFile_PERM_GWDNV_.data");
// Both side should still be the same
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());

Expand Down Expand Up @@ -379,7 +382,8 @@
QVERIFY(currentLocalState.find("readonlyDirectory_PERM_MG_/subdir_PERM_CKG_" ));
// including contents
QVERIFY(currentLocalState.find("readonlyDirectory_PERM_MG_/subdir_PERM_CKG_/subsubdir_PERM_CKDNVG_/normalFile_PERM_GWVND_.data" ));
// new no longer exists
// new no longer exists: it was a move of an existing (server-side) item into a read-only
// folder, so the local copy is cleaned up while its data is restored at the source above.
QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_MG_/newname_PERM_CK_/subsubdir_PERM_CKDNVG_/normalFile_PERM_GWVND_.data" ));
// but is not on server: should have been locally removed
QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_MG_/newname_PERM_CK_"));
Expand Down Expand Up @@ -561,6 +565,87 @@
QVERIFY(cls.find("zallowed/sub2/file"));
}

// A brand new local-only file dropped into a read-only folder cannot be uploaded.
// It must be ignored and kept on disk, not silently deleted, and a follow-up sync
// must be stable rather than looping (issues #7797/#10099).
void testNewLocalFileInReadOnlyFolderIsKept()
{
FakeFolder fakeFolder{FileInfo{}};
auto &lm = fakeFolder.localModifier();
auto &rm = fakeFolder.remoteModifier();
rm.mkdir("readonly");
rm.insert("readonly/existing");
// No CanAddFile ('C') and no CanAddSubDirectories ('K'): nothing new may be created here.
setAllPerm(rm.find("readonly"), RemotePermissions::fromServerString("GWDNV"));
QVERIFY(fakeFolder.syncOnce());

// The user drops a new local-only file into the read-only folder.
lm.insert("readonly/newlocal", 42);

ItemCompletedSpy completeSpy(fakeFolder);
QVERIFY(fakeFolder.syncOnce());

// It is a genuine new file (no remote counterpart) so it is ignored, never removed,
// and stays on disk. This is the move-branch's !isVirtualFile/IGNORE guard NOT firing.
QVERIFY(itemInstruction(completeSpy, "readonly/newlocal", CSYNC_INSTRUCTION_IGNORE));
QVERIFY(fakeFolder.currentLocalState().find("readonly/newlocal"));
QVERIFY(!fakeFolder.currentRemoteState().find("readonly/newlocal"));

// The second sync must not loop: the file is still ignored+kept and no follow-up is requested.
completeSpy.clear();
QVERIFY(fakeFolder.syncOnce());
QVERIFY(itemInstruction(completeSpy, "readonly/newlocal", CSYNC_INSTRUCTION_IGNORE));
QVERIFY(fakeFolder.currentLocalState().find("readonly/newlocal"));
QCOMPARE(fakeFolder.syncEngine().isAnotherSyncNeeded(), NoFollowUpSync);
}

// A synced file moved into a read-only folder cannot land at its new location. The duplicate
// copy in the read-only folder must be cleaned up, while the original is restored from the
// server with its content byte-intact - the rejected move must never destroy data (#7797/#10099).
void testMoveSyncedFileIntoReadOnlyFolderRestoresContent()
{
FakeFolder fakeFolder{FileInfo{}};

// Discovery order decides whether the restore lands on the first or a follow-up sync;
// pin it so the test is deterministic.
auto syncOpts = fakeFolder.syncEngine().syncOptions();
syncOpts._parallelNetworkJobs = 1;
fakeFolder.syncEngine().setSyncOptions(syncOpts);

auto &lm = fakeFolder.localModifier();
auto &rm = fakeFolder.remoteModifier();
rm.mkdir("allowed");
rm.insert("allowed/file", 200, 'X'); // distinctive content to prove it survives
rm.mkdir("readonly");
// No CanAddFile ('C') and no CanAddSubDirectories ('K'): nothing new may be created here.
setAllPerm(rm.find("readonly"), RemotePermissions::fromServerString("GWDNV"));
QVERIFY(fakeFolder.syncOnce());

// The user moves an already-synced file into the read-only folder.
lm.rename("allowed/file", "readonly/file");

QVERIFY(fakeFolder.syncOnce());
// The forbidden-move restore can need a follow-up sync to settle.
if (fakeFolder.syncEngine().isAnotherSyncNeeded() == ImmediateFollowUp) {
QVERIFY(fakeFolder.syncOnce());
}

// The duplicate in the read-only folder is cleaned up locally and never reaches the server.
QVERIFY(!fakeFolder.currentLocalState().find("readonly/file"));
QVERIFY(!fakeFolder.currentRemoteState().find("readonly/file"));

// The original is restored with its content intact (size and bytes), not just its path.
auto localState = fakeFolder.currentLocalState();
auto *restored = localState.find("allowed/file");
QVERIFY(restored);
QCOMPARE(restored->size, qint64{200});
QCOMPARE(restored->contentChar, 'X');

// Local and remote agree on everything, content included, and the sync has settled.
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
QCOMPARE(fakeFolder.syncEngine().isAnotherSyncNeeded(), NoFollowUpSync);
}

void testParentMoveNotAllowedChildrenRestored()
{
FakeFolder fakeFolder{FileInfo{}};
Expand Down
Loading