Merge pull request #9642 from nextcloud/backport/9637/stable-4.0

nilsding · web-flow · commit 4a6bdfbbcf57 · 2026-03-18T12:49:30.000+01:00
[stable-4.0] Set folders writable if remote perms require it, recurse into subdirs if etag and remoteperms differ
diff --git a/src/common/remotepermissions.h b/src/common/remotepermissions.h
@@ -79,6 +79,18 @@ class OCSYNC_EXPORT RemotePermissions
     {
         return _value & (1 << static_cast<int>(p));
     }
+
+    /// Returns true if the local folder needs to be read/writeable for the current remote permissions set
+    [[nodiscard]] bool hasPermissionsForReadWrite() const
+    {
+        return hasPermission(CanWrite) ||
+            hasPermission(CanDelete) ||
+            hasPermission(CanRename) ||
+            hasPermission(CanMove) ||
+            hasPermission(CanAddFile) ||
+            hasPermission(CanAddSubDirectories);
+    }
+
     void setPermission(Permissions p)
     {
         _value |= (1 << static_cast<int>(p)) | notNullMask;
diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
@@ -1070,7 +1070,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(const SyncFileItemPtr &it
                 _discoveryData->findAndCancelDeletedJob(originalPath);
 
                 postProcessRename(path);
-                if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm != dbEntry._remotePerm) {
+                if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm == dbEntry._remotePerm) {
                     _queryServer = ParentNotChanged;
                 }
                 processFileFinalize(item, path, item->isDirectory(), item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist, _queryServer);
@@ -1260,7 +1260,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         item->isPermissionsInvalid = localEntry.isPermissionsInvalid;
 
         auto recurseQueryLocal = _queryLocal == ParentNotChanged ? ParentNotChanged : localEntry.isDirectory || item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist;
-        if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm != dbEntry._remotePerm) {
+        if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm == dbEntry._remotePerm) {
             recurseQueryServer = ParentNotChanged;
         }
         processFileFinalize(item, path, recurse, recurseQueryLocal, recurseQueryServer);
@@ -1699,7 +1699,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
                 processRename(path);
                 recurseQueryServer = etag.get() == base._etag ? ParentNotChanged : NormalQuery;
             }
-            if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm != dbEntry._remotePerm) {
+            if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm == dbEntry._remotePerm) {
                 recurseQueryServer = ParentNotChanged;
             }
             processFileFinalize(item, path, item->isDirectory(), NormalQuery, recurseQueryServer);
diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
@@ -1506,8 +1506,7 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
             || _item->_instruction == CSYNC_INSTRUCTION_UPDATE_METADATA) {
 
             if (!_item->_remotePerm.isNull() &&
-                !_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) &&
-                !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories)) {
+                !_item->_remotePerm.hasPermissionsForReadWrite()) {
                 try {
                     if (const auto fileName = propagator()->fullLocalPath(_item->_file); FileSystem::fileExists(fileName)) {
                         FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadOnly);
diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp
@@ -266,8 +266,7 @@ void PropagateLocalMkdir::startLocalMkdir()
     }
 
     if (!_item->_remotePerm.isNull() &&
-        !_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) &&
-        !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories)) {
+        !_item->_remotePerm.hasPermissionsForReadWrite()) {
         try {
             FileSystem::setFolderPermissions(newDirStr, FileSystem::FolderPermissions::ReadOnly);
         }
diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp
@@ -18,6 +18,7 @@
 #include <iostream>
 
 using namespace OCC;
+using namespace Qt::StringLiterals;
 
 static void applyPermissionsFromName(FileInfo &info) {
     static QRegularExpression rx("_PERM_([^_]*)_[^/]*$");
@@ -969,7 +970,64 @@ private slots:
         fakeFolder.remoteModifier().insert("groupFolder/simpleChildFolder/otherFile", 12);
 
         QVERIFY(fakeFolder.syncOnce());
-        QCOMPARE(propfindCounter, 8);
+        QCOMPARE(propfindCounter, 10);
+    }
+
+    void testFolderReadonlyWhenRemotePermissionsWithoutEtagChanged_data()
+    {
+        QTest::addColumn<bool>("isReadOnly");
+        QTest::newRow("G")        << true;
+        QTest::newRow("GS")       << true;
+        QTest::newRow("GR")       << true;
+        QTest::newRow("GRS")      << true;
+        QTest::newRow("GRM")      << true;
+        QTest::newRow("GRMS")     << true;
+        QTest::newRow("GW")       << false;
+        QTest::newRow("GWS")      << false;
+        QTest::newRow("GD")       << false;
+        QTest::newRow("GDS")      << false;
+        QTest::newRow("GN")       << false;
+        QTest::newRow("GV")       << false;
+        QTest::newRow("GC")       << false;
+        QTest::newRow("GK")       << false;
+        QTest::newRow("GWD")      << false;
+        QTest::newRow("GWDS")     << false;
+        QTest::newRow("GDNCV")    << false;
+        QTest::newRow("GDNCVS")   << false;
+        QTest::newRow("GDNVCKR")  << false;
+        QTest::newRow("GDNVCKRS") << false;
+    }
+
+    void testFolderReadonlyWhenRemotePermissionsWithoutEtagChanged()
+    {
+        const auto remotePerm = QString::fromLocal8Bit(QTest::currentDataTag());
+        QFETCH(bool, isReadOnly);
+        auto remotePermChange = isReadOnly ? (remotePerm + "W") : u"G"_s;
+
+        FakeFolder fakeFolder{FileInfo{}};
+
+        const auto isFolderReadOnly = [&fakeFolder](const QString &relativePath) -> bool {
+            return FileSystem::isFolderReadOnly(std::filesystem::path{FileSystem::joinPath(fakeFolder.localPath(), relativePath).toStdWString()});
+        };
+
+        fakeFolder.remoteModifier().mkdir("root");
+        fakeFolder.remoteModifier().mkdir("root/subdir");
+        fakeFolder.remoteModifier().mkdir("root/subdir/subsubdir");
+        auto rootFolder = fakeFolder.remoteModifier().find("root");
+
+        qInfo() << "initial sync with" << remotePerm;
+        setAllPerm(rootFolder, RemotePermissions::fromServerString(remotePerm));
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE_EQ(isFolderReadOnly(u"root"_s), isReadOnly);
+        QCOMPARE_EQ(isFolderReadOnly(u"root/subdir"_s), isReadOnly);
+        QCOMPARE_EQ(isFolderReadOnly(u"root/subdir/subsubdir"_s), isReadOnly);
+
+        qInfo() << "remote update with" << remotePermChange;
+        setAllPerm(rootFolder, RemotePermissions::fromServerString(remotePermChange));
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE_EQ(isFolderReadOnly(u"root"_s), !isReadOnly);
+        QCOMPARE_EQ(isFolderReadOnly(u"root/subdir"_s), !isReadOnly);
+        QCOMPARE_EQ(isFolderReadOnly(u"root/subdir/subsubdir"_s), !isReadOnly);
     }
 };
 

Original file line number	Diff line number	Diff line change
`@@ -79,6 +79,18 @@ class OCSYNC_EXPORT RemotePermissions`
`79`	`79`	`{`
`80`	`80`	`return _value & (1 << static_cast<int>(p));`
`81`	`81`	`}`
	`82`	`+`
	`83`	`+ /// Returns true if the local folder needs to be read/writeable for the current remote permissions set`
	`84`	`+ [[nodiscard]] bool hasPermissionsForReadWrite() const`
	`85`	`+ {`
	`86`	`+ return hasPermission(CanWrite) \|\|`
	`87`	`+ hasPermission(CanDelete) \|\|`
	`88`	`+ hasPermission(CanRename) \|\|`
	`89`	`+ hasPermission(CanMove) \|\|`
	`90`	`+ hasPermission(CanAddFile) \|\|`
	`91`	`+ hasPermission(CanAddSubDirectories);`
	`92`	`+ }`
	`93`	`+`
`82`	`94`	`void setPermission(Permissions p)`
`83`	`95`	`{`
`84`	`96`	`_value \|= (1 << static_cast<int>(p)) \| notNullMask;`
Original file line number	Diff line number	Diff line change
`@@ -1070,7 +1070,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(const SyncFileItemPtr &it`
`1070`	`1070`	`_discoveryData->findAndCancelDeletedJob(originalPath);`
`1071`	`1071`
`1072`	`1072`	`postProcessRename(path);`
`1073`		`- if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm != dbEntry._remotePerm) {`
	`1073`	`+ if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm == dbEntry._remotePerm) {`
`1074`	`1074`	`_queryServer = ParentNotChanged;`
`1075`	`1075`	`}`
`1076`	`1076`	`processFileFinalize(item, path, item->isDirectory(), item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist, _queryServer);`
`@@ -1260,7 +1260,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(`
`1260`	`1260`	`item->isPermissionsInvalid = localEntry.isPermissionsInvalid;`
`1261`	`1261`
`1262`	`1262`	`auto recurseQueryLocal = _queryLocal == ParentNotChanged ? ParentNotChanged : localEntry.isDirectory \|\| item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist;`
`1263`		`- if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm != dbEntry._remotePerm) {`
	`1263`	`+ if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm == dbEntry._remotePerm) {`
`1264`	`1264`	`recurseQueryServer = ParentNotChanged;`
`1265`	`1265`	`}`
`1266`	`1266`	`processFileFinalize(item, path, recurse, recurseQueryLocal, recurseQueryServer);`
`@@ -1699,7 +1699,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(`
`1699`	`1699`	`processRename(path);`
`1700`	`1700`	`recurseQueryServer = etag.get() == base._etag ? ParentNotChanged : NormalQuery;`
`1701`	`1701`	`}`
`1702`		`- if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm != dbEntry._remotePerm) {`
	`1702`	`+ if (item->isDirectory() && serverEntry.isValid() && dbEntry.isValid() && serverEntry.etag == dbEntry._etag && serverEntry.remotePerm == dbEntry._remotePerm) {`
`1703`	`1703`	`recurseQueryServer = ParentNotChanged;`
`1704`	`1704`	`}`
`1705`	`1705`	`processFileFinalize(item, path, item->isDirectory(), NormalQuery, recurseQueryServer);`
Original file line number	Diff line number	Diff line change
`@@ -266,8 +266,7 @@ void PropagateLocalMkdir::startLocalMkdir()`
`266`	`266`	`}`
`267`	`267`
`268`	`268`	`if (!_item->_remotePerm.isNull() &&`
`269`		`- !_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) &&`
`270`		`- !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories)) {`
	`269`	`+ !_item->_remotePerm.hasPermissionsForReadWrite()) {`
`271`	`270`	`try {`
`272`	`271`	`FileSystem::setFolderPermissions(newDirStr, FileSystem::FolderPermissions::ReadOnly);`
`273`	`272`	`}`