fix(discovery): do not sync shares with "Allow download and sync" unset

src/libsync/account.cpp

@@ -9,6 +9,7 @@
 #include "accountfwd.h"
 #include "capabilities.h"
 #include "clientsideencryptionjobs.h"
+#include "common/remotepermissions.h"
 #include "common/utility.h"
 #include "configfile.h"
 #include "cookiejar.h"
@@ -1289,6 +1290,11 @@ void Account::listRemoteFolder(QPromise<OCC::PlaceholderCreateInfo> *promise, co
             newEntry.size = 0;
         }
 
+        if (!newEntry.remotePerm.isNull() && !newEntry.remotePerm.hasPermission(RemotePermissions::CanRead)) {
+            qCWarning(lcAccount()) << "skip non-readable item" << absoluteItemPathName;
+            return;
+        }
+
         promise->emplaceResult(itemFileName, itemFileName.toStdWString(), absoluteItemPathName, newEntry);
     });
 

src/libsync/discovery.cpp

@@ -843,7 +843,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(const SyncFileItemPtr &it
             item->_direction = SyncFileItem::Down;
             item->_instruction = CSYNC_INSTRUCTION_SYNC;
             item->_type = ItemTypeVirtualFileDownload;
-        } else if (serverEntry.isValid() && !serverEntry.isDirectory && !serverEntry.remotePerm.isNull() && !serverEntry.remotePerm.hasPermission(RemotePermissions::CanRead)) {
+        } else if (serverEntry.isValid() && !serverEntry.remotePerm.isNull() && !serverEntry.remotePerm.hasPermission(RemotePermissions::CanRead)) {
             item->_instruction = CSYNC_INSTRUCTION_REMOVE;
             item->_direction = SyncFileItem::Down;
         } else if (dbEntry._etag != serverEntry.etag) {
@@ -938,7 +938,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(const SyncFileItemPtr &it
         return;
     }
 
-    if (serverEntry.isValid() && !serverEntry.isDirectory && !serverEntry.remotePerm.isNull() && !serverEntry.remotePerm.hasPermission(RemotePermissions::CanRead)) {
+    if (serverEntry.isValid() && !serverEntry.remotePerm.isNull() && !serverEntry.remotePerm.hasPermission(RemotePermissions::CanRead)) {
         item->_instruction = CSYNC_INSTRUCTION_IGNORE;
         emit _discoveryData->itemDiscovered(item);
 

test/testaccount.cpp

@@ -1,3 +1,3 @@
 /*
  * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2017 ownCloud GmbH
@@ -8,7 +8,7 @@
  * any purpose.
  */
 
 #include <qglobal.h>
 #include <QTemporaryDir>
 #include <QtTest>
 
@@ -18,6 +18,7 @@
 #include "syncenginetestutils.h"
 
 using namespace OCC;
+using namespace Qt::StringLiterals;
 
 class TestAccount: public QObject
 {
@@ -108,9 +109,12 @@
         QTest::newRow("root = /; requesting A/") << "" << "A/" << QStringList{ "A/a1", "A/a2", "A/sub1" };
         QTest::newRow("root = /; requesting A/sub1/") << "" << "A/sub1/" << QStringList{ "A/sub1/sub2" };
         QTest::newRow("root = /; requesting A/sub1/sub2") << "" << "A/sub1/sub2/" << QStringList{};
+        QTest::newRow("root = /; requesting A/sub_nodownload/") << "" << "A/sub_nodownload/" << QStringList{};
         QTest::newRow("root = /A; requesting A/") << "/A" << "A/" << QStringList{ "a1", "a2", "sub1" };
         QTest::newRow("root = /A; requesting A/sub1") << "/A" << "A/sub1/" << QStringList{ "sub1/sub2" };
         QTest::newRow("root = /A; requesting A/sub1/sub2") << "/A" << "A/sub1/sub2/" << QStringList{};
+        QTest::newRow("root = /A; requesting A/sub_nodownload/") << "/A" << "A/sub_nodownload/" << QStringList{};
+        QTest::newRow("root = /shared_nodownload; requesting shared_nodownload/") << "/shared_nodownload" << "shared_nodownload/" << QStringList{};
     }
 
     void testAccount_listRemoteFolder()
@@ -122,6 +126,26 @@
         FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
         fakeFolder.remoteModifier().mkdir("A/sub1");
         fakeFolder.remoteModifier().mkdir("A/sub1/sub2");
+        fakeFolder.remoteModifier().mkdir("A/sub1/sub2");
+
+        // prepare shares that are not allowed to be downloaded
+        std::function<void(FileInfo &)> makeSharedNoDownload;
+        makeSharedNoDownload = [&makeSharedNoDownload](FileInfo &fileInfo) -> void {
+            fileInfo.isShared = true;
+            fileInfo.downloadForbidden = true;
+            for (auto &subItem : fileInfo.children) {
+                makeSharedNoDownload(subItem);
+            }
+        };
+        FileInfo sharedNoDownload { "shared_nodownload"_L1, { { "file1"_L1, 32 }, { "file2"_L1, 32 } } };
+        FileInfo sharedNoDownloadSub { "subdir"_L1, { { "file3"_L1, 32 }, { "file4"_L1, 32 } } };
+        FileInfo aSubSharedNoDownload { "sub_nodownload"_L1, { { "file4"_L1, 32 }, { "file5"_L1, 32 } } };
+        sharedNoDownload.children.insert(sharedNoDownloadSub.name, std::move(sharedNoDownloadSub));
+        makeSharedNoDownload(sharedNoDownload);
+        makeSharedNoDownload(aSubSharedNoDownload);
+        fakeFolder.remoteModifier().children.insert(sharedNoDownload.name, std::move(sharedNoDownload));
+        fakeFolder.remoteModifier().find("A")->children.insert(aSubSharedNoDownload.name, std::move(aSubSharedNoDownload));
+
         const auto account = fakeFolder.account();
 
         QEventLoop localEventLoop;

test/testpermissions.cpp

@@ -876,11 +876,19 @@ private slots:
         });
 
         fakeFolder.remoteModifier().insert("file");
+        fakeFolder.remoteModifier().mkdir("folder");
+        fakeFolder.remoteModifier().insert("folder/file");
+
+        const auto setDownloadForbiddenShare = [&fakeFolder](const QString &relativePath) -> void {
+            auto fileInfo = fakeFolder.remoteModifier().find(relativePath);
+            Q_ASSERT(fileInfo);
+            fileInfo->isShared = true;
+            fileInfo->downloadForbidden = true;
+        };
 
-        auto fileItem = fakeFolder.remoteModifier().find("file");
-        Q_ASSERT(fileItem);
-        fileItem->isShared = true;
-        fileItem->downloadForbidden = true;
+        setDownloadForbiddenShare("file");
+        setDownloadForbiddenShare("folder");
+        setDownloadForbiddenShare("folder/file");
 
         // also hook into discovery!!
         SyncFileItemVector discovery;
@@ -890,6 +898,8 @@ private slots:
 
         QVERIFY(itemInstruction(completeSpy, "file", CSYNC_INSTRUCTION_IGNORE));
         QVERIFY(discoveryInstruction(discovery, "file", CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "folder", CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(discoveryInstruction(discovery, "folder", CSYNC_INSTRUCTION_IGNORE));
     }
 
     void testExistingFileBecomeForbiddenDownload()
@@ -898,9 +908,18 @@ private slots:
         QObject parent;
 
         fakeFolder.remoteModifier().insert("file");
-        auto fileInfo = fakeFolder.remoteModifier().find("file");
-        Q_ASSERT(fileInfo);
-        fileInfo->isShared = true;
+        fakeFolder.remoteModifier().mkdir("folder");
+        fakeFolder.remoteModifier().insert("folder/file");
+
+        const auto setShared = [&fakeFolder](const QString &relativePath) -> void {
+            auto fileInfo = fakeFolder.remoteModifier().find(relativePath);
+            Q_ASSERT(fileInfo);
+            fileInfo->isShared = true;
+        };
+
+        setShared("file");
+        setShared("folder");
+        setShared("folder/file");
 
         QVERIFY(fakeFolder.syncOnce());
 
@@ -914,10 +933,16 @@ private slots:
             return nullptr;
         });
 
-        auto fileItem = fakeFolder.remoteModifier().find("file", FileInfo::Invalidate);
-        Q_ASSERT(fileItem);
-        fileItem->isShared = true;
-        fileItem->downloadForbidden = true;
+        const auto setDownloadForbidden = [&fakeFolder](const QString &relativePath) -> void {
+            auto fileInfo = fakeFolder.remoteModifier().find(relativePath, FileInfo::Invalidate);
+            Q_ASSERT(fileInfo);
+            fileInfo->isShared = true;
+            fileInfo->downloadForbidden = true;
+        };
+
+        setDownloadForbidden("file");
+        setDownloadForbidden("folder");
+        setDownloadForbidden("folder/file");
 
         // also hook into discovery!!
         SyncFileItemVector discovery;
@@ -927,6 +952,10 @@ private slots:
 
         QVERIFY(itemInstruction(completeSpy, "file", CSYNC_INSTRUCTION_REMOVE));
         QVERIFY(discoveryInstruction(discovery, "file", CSYNC_INSTRUCTION_REMOVE));
+        QVERIFY(itemInstruction(completeSpy, "folder", CSYNC_INSTRUCTION_REMOVE));
+        QVERIFY(discoveryInstruction(discovery, "folder", CSYNC_INSTRUCTION_REMOVE));
+        QVERIFY(itemInstruction(completeSpy, "folder/file", CSYNC_INSTRUCTION_NONE));
+        QVERIFY(discoveryInstruction(discovery, "folder/file", CSYNC_INSTRUCTION_REMOVE));
     }
 
     void testChangingPermissionsWithoutEtagChange()