From 9d6d28971e2ab49595d9db9d5bd4f4bd534da5e5 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 23 Feb 2024 10:16:04 +0100 Subject: [PATCH 01/10] correct category for some log output Signed-off-by: Matthieu Gallien --- src/libsync/propagatorjobs.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 82167f5b9c817..4cc6e495b08b6 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -129,7 +129,7 @@ void PropagateLocalRemove::start() } propagator()->reportProgress(*_item, 0); if (!propagator()->_journal->deleteFileRecord(_item->_originalFile, _item->isDirectory())) { - qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << _item->_originalFile; + qCWarning(lcPropagateLocalRemove()) << "could not delete file from local DB" << _item->_originalFile; done(SyncFileItem::NormalError, tr("Could not delete file record %1 from local DB").arg(_item->_originalFile), ErrorCategory::GenericError); return; } From e98c25af8a489c1adc6388f586ba9380aece79aa Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 7 Feb 2024 12:25:04 +0100 Subject: [PATCH 02/10] switch to newer CI images should solve the AppImage build issue Signed-off-by: Matthieu Gallien --- .drone.yml | 14 +++++++------- .github/workflows/linux-clang-compile-tests.yml | 2 +- .github/workflows/linux-gcc-compile-tests.yml | 2 +- .github/workflows/sonarcloud.yml | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.drone.yml b/.drone.yml index d697ef71bf29b..43850d333b67e 100644 --- a/.drone.yml +++ b/.drone.yml @@ -4,7 +4,7 @@ name: qt-5.15 steps: - name: cmake - image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-14 + image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-15 volumes: - name: build path: /drone/build @@ -13,7 +13,7 @@ steps: - cmake -G Ninja -DCMAKE_C_COMPILER=gcc-11 -DCMAKE_CXX_COMPILER=g++-11 -DCMAKE_BUILD_TYPE=Debug -DQUICK_COMPILER=ON -DBUILD_UPDATER=ON -DBUILD_TESTING=1 -DADD_E2E_TESTS=ON -DECM_ENABLE_SANITIZERS=address -DCMAKE_CXX_FLAGS=-Werror -DOPENSSL_ROOT_DIR=/usr/local/lib64 ../src - name: compile - image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-14 + image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-15 volumes: - name: build path: /drone/build @@ -22,7 +22,7 @@ steps: - ninja - name: test - image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-14 + image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-15 volumes: - name: build path: /drone/build @@ -74,7 +74,7 @@ name: qt-5.15-clang steps: - name: cmake - image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-14 + image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-15 volumes: - name: build path: /drone/build @@ -82,7 +82,7 @@ steps: - cd /drone/build - cmake -G Ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_C_COMPILER=clang-14 -DCMAKE_CXX_COMPILER=clang++-14 -DCMAKE_BUILD_TYPE=Debug -DQUICK_COMPILER=ON -DBUILD_UPDATER=ON -DBUILD_TESTING=1 -DADD_E2E_TESTS=ON -DECM_ENABLE_SANITIZERS=address -DCMAKE_CXX_FLAGS=-Werror -DOPENSSL_ROOT_DIR=/usr/local/lib64 ../src - name: compile - image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-14 + image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-15 volumes: - name: build path: /drone/build @@ -90,7 +90,7 @@ steps: - cd /drone/build - ninja - name: test - image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-14 + image: ghcr.io/nextcloud/continuous-integration-client:client-5.15-15 volumes: - name: build path: /drone/build @@ -142,7 +142,7 @@ name: AppImage steps: - name: build - image: ghcr.io/nextcloud/continuous-integration-client-appimage:client-appimage-9 + image: ghcr.io/nextcloud/continuous-integration-client-appimage:client-appimage-10 environment: CI_UPLOAD_GIT_TOKEN: from_secret: CI_UPLOAD_GIT_TOKEN diff --git a/.github/workflows/linux-clang-compile-tests.yml b/.github/workflows/linux-clang-compile-tests.yml index d2f6a097951e2..d072fc7f788bd 100644 --- a/.github/workflows/linux-clang-compile-tests.yml +++ b/.github/workflows/linux-clang-compile-tests.yml @@ -6,7 +6,7 @@ jobs: build: name: Linux Clang compilation and tests runs-on: ubuntu-22.04 - container: ghcr.io/nextcloud/continuous-integration-client:client-5.15-14 + container: ghcr.io/nextcloud/continuous-integration-client:client-5.15-15 steps: - uses: actions/checkout@v4 with: diff --git a/.github/workflows/linux-gcc-compile-tests.yml b/.github/workflows/linux-gcc-compile-tests.yml index 60d1297f2c635..1c91e1116d464 100644 --- a/.github/workflows/linux-gcc-compile-tests.yml +++ b/.github/workflows/linux-gcc-compile-tests.yml @@ -6,7 +6,7 @@ jobs: build: name: Linux GCC compilation and tests runs-on: ubuntu-22.04 - container: ghcr.io/nextcloud/continuous-integration-client:client-5.15-14 + container: ghcr.io/nextcloud/continuous-integration-client:client-5.15-15 steps: - uses: actions/checkout@v4 with: diff --git a/.github/workflows/sonarcloud.yml b/.github/workflows/sonarcloud.yml index 29cce35b8b3c6..87ae75eb8aa8a 100644 --- a/.github/workflows/sonarcloud.yml +++ b/.github/workflows/sonarcloud.yml @@ -6,7 +6,7 @@ jobs: build: name: SonarCloud analysis runs-on: ubuntu-22.04 - container: ghcr.io/nextcloud/continuous-integration-client:client-5.15-14 + container: ghcr.io/nextcloud/continuous-integration-client:client-5.15-15 env: SONAR_SERVER_URL: "https://sonarcloud.io" BUILD_WRAPPER_OUT_DIR: build_wrapper_output_directory # Directory where build-wrapper output will be placed From 969a873a655f0eb5d7ca8e18cd7e8eadaf941d37 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 23 Feb 2024 16:52:24 +0100 Subject: [PATCH 03/10] update drone signature Signed-off-by: Matthieu Gallien --- .drone.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.drone.yml b/.drone.yml index 43850d333b67e..4a76132e95f59 100644 --- a/.drone.yml +++ b/.drone.yml @@ -196,6 +196,6 @@ trigger: - push --- kind: signature -hmac: d72110d7f9cba086ca21f9f4f4032ae87f3d9555ab4c5f55d3aeb3df99cab540 +hmac: a8fd97516ee53ca8c938ad413e030dc7df483f116c4b19b5811e359960b7b44d ... From b0a2d5ff81b40d4bfb6673156a41795f926eb794 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 7 Feb 2024 17:38:11 +0100 Subject: [PATCH 04/10] adjust AppImage build script to the new build image Signed-off-by: Matthieu Gallien --- admin/linux/build-appimage.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/admin/linux/build-appimage.sh b/admin/linux/build-appimage.sh index f31dcf0e21eb6..fb18983287349 100755 --- a/admin/linux/build-appimage.sh +++ b/admin/linux/build-appimage.sh @@ -85,7 +85,7 @@ chmod a+x linuxdeployqt.AppImage rm ./linuxdeployqt.AppImage cp -r ./squashfs-root ./linuxdeployqt-squashfs-root unset QTDIR; unset QT_PLUGIN_PATH ; unset LD_LIBRARY_PATH -export LD_LIBRARY_PATH=/usr/local/lib/x86_64-linux-gnu +export LD_LIBRARY_PATH=/usr/local/lib:/usr/local/lib/x86_64-linux-gnu ./squashfs-root/AppRun ${DESKTOP_FILE} -bundle-non-qt-libs -qmldir=${DESKTOP_CLIENT_ROOT}/src/gui # Set origin From 5dfeb55e0f4835e701e78fad49614c1eb822dbd2 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 2 Feb 2024 10:54:32 +0100 Subject: [PATCH 05/10] we require c++-17 Signed-off-by: Matthieu Gallien --- CMakeLists.txt | 14 ++++++++------ src/libsync/CMakeLists.txt | 5 +++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a196dbba5ce67..6877335888139 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,20 +1,22 @@ cmake_minimum_required(VERSION 3.16) -set(CMAKE_CXX_STANDARD 17) cmake_policy(SET CMP0071 NEW) # Enable use of QtQuick compiler/generated code -find_program(CLANG_TIDY_EXE NAMES "clang-tidy") -if (CLANG_TIDY_EXE) - set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_EXE} -checks=-*,modernize-use-auto,modernize-use-using,modernize-use-nodiscard,modernize-use-nullptr,modernize-use-override,cppcoreguidelines-pro-type-static-cast-downcast,modernize-use-default-member-init,cppcoreguidelines-pro-type-member-init,cppcoreguidelines-init-variables) -endif() - project(client) if(APPLE) set(CMAKE_OSX_DEPLOYMENT_TARGET "12.0" CACHE STRING "Minimum OSX deployment version") endif() +set(CMAKE_CXX_STANDARD 17) +set(CMAKE_CXX_STANDARD_REQUIRED 17) + include(FeatureSummary) +find_program(CLANG_TIDY_EXE NAMES "clang-tidy") +if (CLANG_TIDY_EXE) + set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_EXE} -checks=-*,modernize-use-auto,modernize-use-using,modernize-use-nodiscard,modernize-use-nullptr,modernize-use-override,cppcoreguidelines-pro-type-static-cast-downcast,modernize-use-default-member-init,cppcoreguidelines-pro-type-member-init,cppcoreguidelines-init-variables) +endif() + set(CMAKE_XCODE_ATTRIBUTE_ENABLE_HARDENED_RUNTIME YES) set(BIN_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin") diff --git a/src/libsync/CMakeLists.txt b/src/libsync/CMakeLists.txt index 6749ff4adc5e2..461f6dea213e8 100644 --- a/src/libsync/CMakeLists.txt +++ b/src/libsync/CMakeLists.txt @@ -214,6 +214,11 @@ target_link_libraries(nextcloudsync KF5::Archive ) +target_compile_features(nextcloudsync + PRIVATE + cxx_std_17 +) + find_package(Qt5 REQUIRED COMPONENTS Gui Widgets Svg) target_link_libraries(nextcloudsync PUBLIC Qt5::Gui Qt5::Widgets Qt5::Svg) From 706d9697d497300cfda529200e50e36cd4b084d1 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 24 Jan 2024 14:29:26 +0100 Subject: [PATCH 06/10] ensure we get proper logs for permissions automated tests Signed-off-by: Matthieu Gallien --- test/testpermissions.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index ce9c3c9acdf7a..5b2a461ded476 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -69,7 +69,8 @@ class TestPermissions : public QObject private slots: void initTestCase() { - OCC::Logger::instance()->setLogDebug(true); + Logger::instance()->setLogFlush(true); + Logger::instance()->setLogDebug(true); } void t7pl() From bf7f87492aa09abadf74e8df1d2db658a1758484 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 1 Feb 2024 12:12:28 +0100 Subject: [PATCH 07/10] update automated test to work with read only folders Signed-off-by: Matthieu Gallien --- test/syncenginetestutils.cpp | 12 +++--- test/testpermissions.cpp | 77 ++++++++++++++++++++++++++++-------- 2 files changed, 67 insertions(+), 22 deletions(-) diff --git a/test/syncenginetestutils.cpp b/test/syncenginetestutils.cpp index 88ece82c1e7e9..50f6fddebc9f1 100644 --- a/test/syncenginetestutils.cpp +++ b/test/syncenginetestutils.cpp @@ -17,8 +17,7 @@ #include #include - - +#include PathComponents::PathComponents(const char *path) : PathComponents { QString::fromUtf8(path) } @@ -48,10 +47,13 @@ PathComponents PathComponents::subComponents() const & void DiskFileModifier::remove(const QString &relativePath) { QFileInfo fi { _rootDir.filePath(relativePath) }; - if (fi.isFile()) + if (fi.isFile()) { QVERIFY(_rootDir.remove(relativePath)); - else - QVERIFY(QDir { fi.filePath() }.removeRecursively()); + } else { + const auto pathToDelete = fi.filePath().toStdWString(); + std::filesystem::permissions(pathToDelete, std::filesystem::perms::owner_exec, std::filesystem::perm_options::add); + QVERIFY(std::filesystem::remove_all(pathToDelete)); + } } void DiskFileModifier::insert(const QString &relativePath, qint64 size, char contentChar) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 5b2a461ded476..6eea5fe7af182 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -5,11 +5,14 @@ * */ -#include #include "syncenginetestutils.h" #include #include "common/ownsql.h" +#include + +#include + using namespace OCC; static void applyPermissionsFromName(FileInfo &info) { @@ -112,18 +115,58 @@ private slots: assertCsyncJournalOk(fakeFolder.syncJournal()); qInfo("Do some changes and see how they propagate"); + const auto removeReadOnly = [&] (const QString &file) { + const auto fileInfoToDelete = QFileInfo(fakeFolder.localPath() + file); + QFile(fakeFolder.localPath() + file).setPermissions(QFile::WriteOwner | QFile::ReadOwner); + const auto isReadOnly = !static_cast(std::filesystem::status(fileInfoToDelete.absolutePath().toStdWString()).permissions() & std::filesystem::perms::owner_write); + if (isReadOnly) { + std::filesystem::permissions(fileInfoToDelete.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + } + fakeFolder.localModifier().remove(file); + if (isReadOnly) { + std::filesystem::permissions(fileInfoToDelete.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::remove); + } + }; + + const auto renameReadOnly = [&] (const QString &relativePath, const QString &relativeDestinationDirectory) { + const auto sourceFileInfo = QFileInfo(fakeFolder.localPath() + relativePath); + const auto destinationFileInfo = QFileInfo(fakeFolder.localPath() + relativeDestinationDirectory); + const auto isSourceReadOnly = !static_cast(std::filesystem::status(sourceFileInfo.absolutePath().toStdWString()).permissions() & std::filesystem::perms::owner_write); + const auto isDestinationReadOnly = !static_cast(std::filesystem::status(destinationFileInfo.absolutePath().toStdWString()).permissions() & std::filesystem::perms::owner_write); + if (isSourceReadOnly) { + std::filesystem::permissions(sourceFileInfo.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + } + if (isDestinationReadOnly) { + std::filesystem::permissions(destinationFileInfo.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + } + fakeFolder.localModifier().rename(relativePath, relativeDestinationDirectory); + if (isSourceReadOnly) { + std::filesystem::permissions(sourceFileInfo.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::remove); + } + if (isDestinationReadOnly) { + std::filesystem::permissions(destinationFileInfo.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::remove); + } + }; + + const auto insertReadOnly = [&] (const QString &file, const int fileSize) { + const auto fileInfo = QFileInfo(fakeFolder.localPath() + file); + const auto isReadOnly = !static_cast(std::filesystem::status(fileInfo.absolutePath().toStdWString()).permissions() & std::filesystem::perms::owner_write); + if (isReadOnly) { + std::filesystem::permissions(fileInfo.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + } + fakeFolder.localModifier().insert(file, fileSize); + if (isReadOnly) { + std::filesystem::permissions(fileInfo.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::remove); + } + }; + //1. remove the file than cannot be removed // (they should be recovered) fakeFolder.localModifier().remove("normalDirectory_PERM_CKDNV_/cannotBeRemoved_PERM_WVN_.data"); - fakeFolder.localModifier().remove("readonlyDirectory_PERM_M_/cannotBeRemoved_PERM_WVN_.data"); + removeReadOnly("readonlyDirectory_PERM_M_/cannotBeRemoved_PERM_WVN_.data"); //2. remove the file that can be removed // (they should properly be gone) - auto removeReadOnly = [&] (const QString &file) { - QVERIFY(!QFileInfo(fakeFolder.localPath() + file).permission(QFile::WriteOwner)); - QFile(fakeFolder.localPath() + file).setPermissions(QFile::WriteOwner | QFile::ReadOwner); - fakeFolder.localModifier().remove(file); - }; removeReadOnly("normalDirectory_PERM_CKDNV_/canBeRemoved_PERM_D_.data"); removeReadOnly("readonlyDirectory_PERM_M_/canBeRemoved_PERM_D_.data"); @@ -175,7 +218,7 @@ private slots: QCOMPARE(c2->size, cannotBeModifiedSize + 1); // remove the conflicts for the next state comparison fakeFolder.localModifier().remove(c1->path()); - fakeFolder.localModifier().remove(c2->path()); + removeReadOnly(c2->path()); //4. File should be updated, that's tested by assertLocalAndRemoteDir QCOMPARE(currentLocalState.find("normalDirectory_PERM_CKDNV_/canBeModified_PERM_W_.data")->size, canBeModifiedSize + 1); @@ -192,7 +235,7 @@ private slots: //6. Create a new file in a read only folder // (they should not be uploaded) - fakeFolder.localModifier().insert("readonlyDirectory_PERM_M_/newFile_PERM_WDNV_.data", 105 ); + insertReadOnly("readonlyDirectory_PERM_M_/newFile_PERM_WDNV_.data", 105 ); applyPermissionsFromName(fakeFolder.remoteModifier()); // error: can't upload to readonly @@ -206,7 +249,7 @@ private slots: QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/newFile_PERM_WDNV_.data")); QVERIFY(!fakeFolder.currentRemoteState().find("readonlyDirectory_PERM_M_/newFile_PERM_WDNV_.data")); // remove it so next test succeed. - fakeFolder.localModifier().remove("readonlyDirectory_PERM_M_/newFile_PERM_WDNV_.data"); + removeReadOnly("readonlyDirectory_PERM_M_/newFile_PERM_WDNV_.data"); // Both side should still be the same QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); @@ -214,7 +257,7 @@ private slots: //###################################################################### qInfo( "remove the read only directory" ); // -> It must be recovered - fakeFolder.localModifier().remove("readonlyDirectory_PERM_M_"); + removeReadOnly("readonlyDirectory_PERM_M_"); applyPermissionsFromName(fakeFolder.remoteModifier()); QVERIFY(fakeFolder.syncOnce()); assertCsyncJournalOk(fakeFolder.syncJournal()); @@ -235,7 +278,7 @@ private slots: //Missing directory should be restored //new directory should be uploaded - fakeFolder.localModifier().rename("readonlyDirectory_PERM_M_/subdir_PERM_CK_", "normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_"); + renameReadOnly("readonlyDirectory_PERM_M_/subdir_PERM_CK_", "normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_"); applyPermissionsFromName(fakeFolder.remoteModifier()); QVERIFY(fakeFolder.syncOnce()); currentLocalState = fakeFolder.currentLocalState(); @@ -272,10 +315,10 @@ private slots: //1. rename a directory in a read only folder //Missing directory should be restored //new directory should stay but not be uploaded - fakeFolder.localModifier().rename("readonlyDirectory_PERM_M_/subdir_PERM_CK_", "readonlyDirectory_PERM_M_/newname_PERM_CK_" ); + renameReadOnly("readonlyDirectory_PERM_M_/subdir_PERM_CK_", "readonlyDirectory_PERM_M_/newname_PERM_CK_" ); //2. move a directory from read to read only (move the directory from previous step) - fakeFolder.localModifier().rename("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_", "readonlyDirectory_PERM_M_/moved_PERM_CK_" ); + renameReadOnly("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_", "readonlyDirectory_PERM_M_/moved_PERM_CK_" ); // error: can't upload to readonly! QVERIFY(!fakeFolder.syncOnce()); @@ -289,7 +332,7 @@ private slots: // new still exist QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/newname_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data" )); // but is not on server: so remove it locally for the future comparison - fakeFolder.localModifier().remove("readonlyDirectory_PERM_M_/newname_PERM_CK_"); + removeReadOnly("readonlyDirectory_PERM_M_/newname_PERM_CK_"); //2. // old removed @@ -299,7 +342,7 @@ private slots: // new still there QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/moved_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data" )); //but not on server - fakeFolder.localModifier().remove("readonlyDirectory_PERM_M_/moved_PERM_CK_"); + removeReadOnly("readonlyDirectory_PERM_M_/moved_PERM_CK_"); fakeFolder.remoteModifier().remove("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_"); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); @@ -333,7 +376,7 @@ private slots: int count = 0; while (auto i = findConflict(currentLocalState, "readonlyDirectory_PERM_M_/cannotBeModified_PERM_DVN_.data")) { QVERIFY((i->contentChar == 's') || (i->contentChar == 'd')); - fakeFolder.localModifier().remove(i->path()); + removeReadOnly(i->path()); currentLocalState = fakeFolder.currentLocalState(); count++; } From bbc976c920f161b630129aa152c7031c8c10d33a Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 1 Feb 2024 15:48:40 +0100 Subject: [PATCH 08/10] gather more information on exceptions that happen when running tests Signed-off-by: Matthieu Gallien --- test/testsyncengine.cpp | 60 +++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 8ef503956740a..15ac965f166f2 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -15,6 +15,8 @@ #include #include +#include + using namespace OCC; namespace { @@ -813,36 +815,42 @@ private slots: QVERIFY(fakeFolder.currentLocalState().find("A/t𠜎t")); #if !defined(Q_OS_MAC) && !defined(Q_OS_WIN) - // Try again with a locale that can represent ö but not 𠜎 (4-byte utf8). - QTextCodec::setCodecForLocale(QTextCodec::codecForName("ISO-8859-15")); - QVERIFY(QTextCodec::codecForLocale()->mibEnum() == 111); + try { + // Try again with a locale that can represent ö but not 𠜎 (4-byte utf8). + QTextCodec::setCodecForLocale(QTextCodec::codecForName("ISO-8859-15")); + QVERIFY(QTextCodec::codecForLocale()->mibEnum() == 111); - fakeFolder.remoteModifier().insert("B/tößt"); - fakeFolder.remoteModifier().insert("B/t𠜎t"); - QVERIFY(fakeFolder.syncOnce()); - QVERIFY(fakeFolder.currentLocalState().find("B/tößt")); - QVERIFY(!fakeFolder.currentLocalState().find("B/t𠜎t")); - QVERIFY(!fakeFolder.currentLocalState().find("B/t?t")); - QVERIFY(!fakeFolder.currentLocalState().find("B/t??t")); - QVERIFY(!fakeFolder.currentLocalState().find("B/t???t")); - QVERIFY(!fakeFolder.currentLocalState().find("B/t????t")); - QVERIFY(fakeFolder.syncOnce()); - QVERIFY(fakeFolder.currentRemoteState().find("B/tößt")); - QVERIFY(fakeFolder.currentRemoteState().find("B/t𠜎t")); + fakeFolder.remoteModifier().insert("B/tößt"); + fakeFolder.remoteModifier().insert("B/t𠜎t"); + QVERIFY(fakeFolder.syncOnce()); + QVERIFY(fakeFolder.currentLocalState().find("B/tößt")); + QVERIFY(!fakeFolder.currentLocalState().find("B/t𠜎t")); + QVERIFY(!fakeFolder.currentLocalState().find("B/t?t")); + QVERIFY(!fakeFolder.currentLocalState().find("B/t??t")); + QVERIFY(!fakeFolder.currentLocalState().find("B/t???t")); + QVERIFY(!fakeFolder.currentLocalState().find("B/t????t")); + QVERIFY(fakeFolder.syncOnce()); + QVERIFY(fakeFolder.currentRemoteState().find("B/tößt")); + QVERIFY(fakeFolder.currentRemoteState().find("B/t𠜎t")); - // Try again with plain ascii - QTextCodec::setCodecForLocale(QTextCodec::codecForName("ASCII")); - QVERIFY(QTextCodec::codecForLocale()->mibEnum() == 3); + // Try again with plain ascii + QTextCodec::setCodecForLocale(QTextCodec::codecForName("ASCII")); + QVERIFY(QTextCodec::codecForLocale()->mibEnum() == 3); - fakeFolder.remoteModifier().insert("C/tößt"); - QVERIFY(fakeFolder.syncOnce()); - QVERIFY(!fakeFolder.currentLocalState().find("C/tößt")); - QVERIFY(!fakeFolder.currentLocalState().find("C/t??t")); - QVERIFY(!fakeFolder.currentLocalState().find("C/t????t")); - QVERIFY(fakeFolder.syncOnce()); - QVERIFY(fakeFolder.currentRemoteState().find("C/tößt")); + fakeFolder.remoteModifier().insert("C/tößt"); + QVERIFY(fakeFolder.syncOnce()); + QVERIFY(!fakeFolder.currentLocalState().find("C/tößt")); + QVERIFY(!fakeFolder.currentLocalState().find("C/t??t")); + QVERIFY(!fakeFolder.currentLocalState().find("C/t????t")); + QVERIFY(fakeFolder.syncOnce()); + QVERIFY(fakeFolder.currentRemoteState().find("C/tößt")); - QTextCodec::setCodecForLocale(utf8Locale); + QTextCodec::setCodecForLocale(utf8Locale); + } + catch (const std::filesystem::filesystem_error &e) + { + qCritical() << e.what() << e.path1().c_str() << e.path2().c_str() << e.code().message().c_str(); + } #endif } From 4844f326c1639ba7c546a8d270999f31ee8bef6f Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 11 Jan 2024 00:16:43 +0100 Subject: [PATCH 09/10] newly created folders will be read-only when needed Close #6296 Signed-off-by: Matthieu Gallien --- src/common/filesystembase.h | 9 +- src/libsync/filesystem.cpp | 179 ++++++++++++++++++++++++++++- src/libsync/filesystem.h | 14 ++- src/libsync/owncloudpropagator.cpp | 55 ++++++++- src/libsync/propagatedownload.cpp | 31 ++++- src/libsync/propagatedownload.h | 5 + src/libsync/propagatorjobs.cpp | 105 +++++++++++++++++ test/testpermissions.cpp | 151 ++++++++++++++++++++++++ test/testsyncengine.cpp | 7 +- 9 files changed, 540 insertions(+), 16 deletions(-) diff --git a/src/common/filesystembase.h b/src/common/filesystembase.h index fd0572804dcb2..6a3baa7afb6e3 100644 --- a/src/common/filesystembase.h +++ b/src/common/filesystembase.h @@ -20,12 +20,13 @@ #include "config.h" +#include "csync/ocsynclib.h" + #include -#include #include #include -#include +#include class QFile; @@ -42,6 +43,10 @@ OCSYNC_EXPORT Q_DECLARE_LOGGING_CATEGORY(lcFileSystem) * @brief This file contains file system helper */ namespace FileSystem { + enum class FolderPermissions { + ReadOnly, + ReadWrite, + }; /** * @brief Mark the file as hidden (only has effects on windows) diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp index 7c58c7b2bde45..2d5f27f76547a 100644 --- a/src/libsync/filesystem.cpp +++ b/src/libsync/filesystem.cpp @@ -15,15 +15,20 @@ #include "filesystem.h" #include "common/utility.h" +#include "csync.h" +#include "vio/csync_vio_local.h" +#include "std/c_time.h" + #include #include #include #include #include -#include "csync.h" -#include "vio/csync_vio_local.h" -#include "std/c_time.h" +#ifdef Q_OS_WIN +#include +#include +#endif namespace OCC { @@ -189,5 +194,173 @@ bool FileSystem::getInode(const QString &filename, quint64 *inode) return false; } +bool FileSystem::setFolderPermissions(const QString &path, + FileSystem::FolderPermissions permissions) noexcept +{ + try { + switch (permissions) { + case OCC::FileSystem::FolderPermissions::ReadOnly: + std::filesystem::permissions(path.toStdWString(), std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write, std::filesystem::perm_options::remove); + break; + case OCC::FileSystem::FolderPermissions::ReadWrite: + break; + } + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << e.path1().c_str() << e.path2().c_str(); + return false; + } + +#ifdef Q_OS_WIN + SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION; + std::unique_ptr securityDescriptor; + auto neededLength = 0ul; + + if (!GetFileSecurityW(path.toStdWString().c_str(), info, nullptr, 0, &neededLength)) { + const auto lastError = GetLastError(); + if (lastError != ERROR_INSUFFICIENT_BUFFER) { + qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << lastError; + return false; + } + + securityDescriptor.reset(new char[neededLength]); + + if (!GetFileSecurityW(path.toStdWString().c_str(), info, securityDescriptor.get(), neededLength, &neededLength)) { + qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << GetLastError(); + return false; + } + } + + int daclPresent = false, daclDefault = false; + PACL resultDacl = nullptr; + if (!GetSecurityDescriptorDacl(securityDescriptor.get(), &daclPresent, &resultDacl, &daclDefault)) { + qCWarning(lcFileSystem) << "error when calling GetSecurityDescriptorDacl" << path << GetLastError(); + return false; + } + if (!daclPresent) { + qCWarning(lcFileSystem) << "error when calling DACL needed to set a folder read-only or read-write is missing" << path; + return false; + } + + PSID sid = nullptr; + if (!ConvertStringSidToSidW(L"S-1-5-32-545", &sid)) + { + qCWarning(lcFileSystem) << "error when calling ConvertStringSidToSidA" << path << GetLastError(); + return false; + } + + ACL_SIZE_INFORMATION aclSize; + if (!GetAclInformation(resultDacl, &aclSize, sizeof(aclSize), AclSizeInformation)) { + qCWarning(lcFileSystem) << "error when calling GetAclInformation" << path << GetLastError(); + return false; + } + + const auto newAclSize = aclSize.AclBytesInUse + sizeof(ACCESS_DENIED_ACE) + GetLengthSid(sid); + qCDebug(lcFileSystem) << "allocated a new DACL object of size" << newAclSize; + + std::unique_ptr newDacl{reinterpret_cast(new char[newAclSize])}; + if (!InitializeAcl(newDacl.get(), newAclSize, ACL_REVISION)) { + const auto lastError = GetLastError(); + if (lastError != ERROR_INSUFFICIENT_BUFFER) { + qCWarning(lcFileSystem) << "insufficient memory error when calling InitializeAcl" << path; + return false; + } + + qCWarning(lcFileSystem) << "error when calling InitializeAcl" << path << lastError; + return false; + } + + if (permissions == FileSystem::FolderPermissions::ReadOnly) { + qCInfo(lcFileSystem) << path << "will be read only"; + if (!AddAccessDeniedAce(newDacl.get(), ACL_REVISION, FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA | FILE_DELETE_CHILD, sid)) { + qCWarning(lcFileSystem) << "error when calling AddAccessDeniedAce << path" << GetLastError(); + return false; + } + } + + for (int i = 0; i < aclSize.AceCount; ++i) { + void *currentAce = nullptr; + if (!GetAce(resultDacl, i, ¤tAce)) { + qCWarning(lcFileSystem) << "error when calling GetAce" << path << GetLastError(); + return false; + } + + const auto currentAceHeader = reinterpret_cast(currentAce); + + if (permissions == FileSystem::FolderPermissions::ReadWrite) { + qCInfo(lcFileSystem) << path << "will be read write"; + } + if (permissions == FileSystem::FolderPermissions::ReadWrite && (ACCESS_DENIED_ACE_TYPE == (currentAceHeader->AceType & ACCESS_DENIED_ACE_TYPE))) { + qCWarning(lcFileSystem) << "AceHeader" << path << currentAceHeader->AceFlags << currentAceHeader->AceSize << currentAceHeader->AceType; + continue; + } + + if (!AddAce(newDacl.get(), ACL_REVISION, i + 1, currentAce, currentAceHeader->AceSize)) { + const auto lastError = GetLastError(); + if (lastError != ERROR_INSUFFICIENT_BUFFER) { + qCWarning(lcFileSystem) << "insufficient memory error when calling AddAce" << path; + return false; + } + + if (lastError != ERROR_INVALID_PARAMETER) { + qCWarning(lcFileSystem) << "invalid parameter error when calling AddAce" << path << "ACL size" << newAclSize; + return false; + } + + qCWarning(lcFileSystem) << "error when calling AddAce" << path << lastError << "acl index" << (i + 1); + return false; + } + } + + SECURITY_DESCRIPTOR newSecurityDescriptor; + if (!InitializeSecurityDescriptor(&newSecurityDescriptor, SECURITY_DESCRIPTOR_REVISION)) { + qCWarning(lcFileSystem) << "error when calling InitializeSecurityDescriptor" << path << GetLastError(); + return false; + } + + if (!SetSecurityDescriptorDacl(&newSecurityDescriptor, true, newDacl.get(), false)) { + qCWarning(lcFileSystem) << "error when calling SetSecurityDescriptorDacl" << path << GetLastError(); + return false; + } + + if (!SetFileSecurityW(path.toStdWString().c_str(), info, &newSecurityDescriptor)) { + qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << path << GetLastError(); + return false; + } +#endif + + try { + switch (permissions) { + case OCC::FileSystem::FolderPermissions::ReadOnly: + break; + case OCC::FileSystem::FolderPermissions::ReadWrite: + std::filesystem::permissions(path.toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + break; + } + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << e.path1().c_str() << e.path2().c_str(); + return false; + } + + return true; +} + +bool FileSystem::isFolderReadOnly(const std::filesystem::path &path) noexcept +{ + try { + const auto folderStatus = std::filesystem::status(path); + const auto folderPermissions = folderStatus.permissions(); + return (folderPermissions & std::filesystem::perms::owner_write) != std::filesystem::perms::owner_write; + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcFileSystem()) << "exception when checking folder permissions" << e.what() << e.path1().c_str() << e.path2().c_str(); + return false; + } +} + } // namespace OCC diff --git a/src/libsync/filesystem.h b/src/libsync/filesystem.h index 602f74bb7f2bc..102a79f5563ce 100644 --- a/src/libsync/filesystem.h +++ b/src/libsync/filesystem.h @@ -16,13 +16,14 @@ #include "config.h" +#include "owncloudlib.h" +#include "common/filesystembase.h" + #include + #include #include - -#include -// Chain in the base include and extend the namespace -#include "common/filesystembase.h" +#include class QFile; @@ -96,6 +97,11 @@ namespace FileSystem { bool OWNCLOUDSYNC_EXPORT removeRecursively(const QString &path, const std::function &onDeleted = nullptr, QStringList *errors = nullptr); + + bool OWNCLOUDSYNC_EXPORT setFolderPermissions(const QString &path, + FileSystem::FolderPermissions permissions) noexcept; + + bool OWNCLOUDSYNC_EXPORT isFolderReadOnly(const std::filesystem::path &path) noexcept; } /** @} */ diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 1e46940b62b11..28c0c18c3e769 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1442,6 +1442,59 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) if (_item->_instruction == CSYNC_INSTRUCTION_RENAME || _item->_instruction == CSYNC_INSTRUCTION_NEW || _item->_instruction == CSYNC_INSTRUCTION_UPDATE_METADATA) { + + if (!_item->_remotePerm.isNull() && + !_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) && + !_item->_remotePerm.hasPermission(RemotePermissions::CanRename) && + !_item->_remotePerm.hasPermission(RemotePermissions::CanMove) && + !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories)) { + try { + if (QFileInfo::exists(propagator()->fullLocalPath(_item->_file))) { + FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadOnly); + qCDebug(lcDirectory) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); + std::filesystem::permissions(propagator()->fullLocalPath(_item->_file).toStdWString(), std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write, std::filesystem::perm_options::remove); + qCDebug(lcDirectory) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); + } + if (!_item->_renameTarget.isEmpty() && QFileInfo::exists(propagator()->fullLocalPath(_item->_renameTarget))) { + FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadOnly); + qCDebug(lcDirectory) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); + std::filesystem::permissions(propagator()->fullLocalPath(_item->_renameTarget).toStdWString(), std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write, std::filesystem::perm_options::remove); + qCDebug(lcDirectory) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); + } + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcDirectory) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + _item->_status = SyncFileItem::NormalError; + _item->_errorString = tr("The folder %1 cannot be made read-only: %2").arg(_item->_file, e.what()); + } + } else if (!_item->_remotePerm.isNull() && + (_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) || + !_item->_remotePerm.hasPermission(RemotePermissions::CanRename) || + !_item->_remotePerm.hasPermission(RemotePermissions::CanMove) || + !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories))) { + try { + if (QFileInfo::exists(propagator()->fullLocalPath(_item->_file))) { + FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadWrite); + qCDebug(lcDirectory) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); + std::filesystem::permissions(propagator()->fullLocalPath(_item->_file).toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + qCDebug(lcDirectory) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions()); + } + if (!_item->_renameTarget.isEmpty() && QFileInfo::exists(propagator()->fullLocalPath(_item->_renameTarget))) { + FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadWrite); + qCDebug(lcDirectory) << "old permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); + std::filesystem::permissions(propagator()->fullLocalPath(_item->_renameTarget).toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + qCDebug(lcDirectory) << "new permissions" << static_cast(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions()); + } + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcDirectory) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + _item->_status = SyncFileItem::NormalError; + _item->_errorString = tr("The folder %1 cannot be made read-only: %2").arg(e.path1().c_str(), e.what()); + } + } + const auto result = propagator()->updateMetadata(*_item); if (!result) { status = _item->_status = SyncFileItem::FatalError; @@ -1454,7 +1507,7 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) } } _state = Finished; - qCInfo(lcPropagator) << "PropagateDirectory::slotSubJobsFinished" << "emit finished" << status; + qCInfo(lcPropagator) << "PropagateDirectory::slotSubJobsFinished" << "emit finished" << status << _item->_file; emit finished(status); } diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 11b9935a4e6ca..8859b15e9a02b 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -32,11 +32,8 @@ #include #include #include -#include -#ifdef Q_OS_UNIX -#include -#endif +#include namespace OCC { @@ -672,8 +669,26 @@ void PropagateDownloadFile::startDownload() // Can't open(Append) read-only files, make sure to make // file writable if it exists. - if (_tmpFile.exists()) + if (_tmpFile.exists()) { FileSystem::setFileReadOnly(_tmpFile.fileName(), false); + } + + try { + const auto newDirPath = std::filesystem::path{_tmpFile.fileName().toStdWString()}; + Q_ASSERT(newDirPath.has_parent_path()); + _parentPath = newDirPath.parent_path(); + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcPropagateDownload) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + } + + if (FileSystem::isFolderReadOnly(_parentPath)) { + FileSystem::setFolderPermissions(QString::fromStdWString(_parentPath.wstring()), FileSystem::FolderPermissions::ReadWrite); + emit propagator()->touchedFile(QString::fromStdWString(_parentPath.wstring())); + _needParentFolderRestorePermissions = true; + } + if (!_tmpFile.open(QIODevice::Append | QIODevice::Unbuffered)) { qCWarning(lcPropagateDownload) << "could not open temporary file" << _tmpFile.fileName(); done(SyncFileItem::NormalError, _tmpFile.errorString(), ErrorCategory::GenericError); @@ -1272,6 +1287,12 @@ void PropagateDownloadFile::downloadFinished() return; } + if (_needParentFolderRestorePermissions) { + FileSystem::setFolderPermissions(QString::fromStdWString(_parentPath.wstring()), FileSystem::FolderPermissions::ReadWrite); + emit propagator()->touchedFile(QString::fromStdWString(_parentPath.wstring())); + _needParentFolderRestorePermissions = false; + } + FileSystem::setFileHidden(filename, false); // Maybe we downloaded a newer version of the file than we thought we would... diff --git a/src/libsync/propagatedownload.h b/src/libsync/propagatedownload.h index a56cdd4a1b0d4..23c72f0aebaea 100644 --- a/src/libsync/propagatedownload.h +++ b/src/libsync/propagatedownload.h @@ -23,6 +23,8 @@ #include #include +#include + namespace OCC { class PropagateDownloadEncrypted; @@ -260,5 +262,8 @@ private slots: QElapsedTimer _stopwatch; PropagateDownloadEncrypted *_downloadEncryptedHelper = nullptr; + + std::filesystem::path _parentPath; + bool _needParentFolderRestorePermissions = false; }; } diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 4cc6e495b08b6..e016fd0652d84 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -32,6 +32,7 @@ #include #include +#include #include @@ -181,6 +182,24 @@ void PropagateLocalMkdir::startLocalMkdir() done(SyncFileItem::FileNameClash, tr("Folder %1 cannot be created because of a local file or folder name clash!").arg(newDirStr), ErrorCategory::GenericError); return; } + + auto parentFolderPath = std::filesystem::path{}; + auto parentNeedRollbackPermissions = false; + try { + const auto newDirPath = std::filesystem::path{newDirStr.toStdWString()}; + Q_ASSERT(newDirPath.has_parent_path()); + parentFolderPath = newDirPath.parent_path(); + if (FileSystem::isFolderReadOnly(parentFolderPath)) { + FileSystem::setFolderPermissions(QString::fromStdWString(parentFolderPath.wstring()), FileSystem::FolderPermissions::ReadWrite); + parentNeedRollbackPermissions = true; + emit propagator()->touchedFile(QString::fromStdWString(parentFolderPath.wstring())); + } + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcPropagateLocalMkdir) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + } + emit propagator()->touchedFile(newDirStr); QDir localDir(propagator()->localPath()); if (!localDir.mkpath(_item->_file)) { @@ -188,6 +207,33 @@ void PropagateLocalMkdir::startLocalMkdir() return; } + if (!_item->_remotePerm.isNull() && + !_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) && + !_item->_remotePerm.hasPermission(RemotePermissions::CanRename) && + !_item->_remotePerm.hasPermission(RemotePermissions::CanMove) && + !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories)) { + try { + FileSystem::setFolderPermissions(newDirStr, FileSystem::FolderPermissions::ReadOnly); + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcPropagateLocalMkdir) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + done(SyncFileItem::NormalError, tr("The folder %1 cannot be made read-only: %2").arg(_item->_file, e.what()), ErrorCategory::GenericError); + return; + } + } + + try { + if (parentNeedRollbackPermissions) { + FileSystem::setFolderPermissions(QString::fromStdWString(parentFolderPath.wstring()), FileSystem::FolderPermissions::ReadOnly); + emit propagator()->touchedFile(QString::fromStdWString(parentFolderPath.wstring())); + } + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcPropagateLocalMkdir) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + } + // Insert the directory into the database. The correct etag will be set later, // once all contents have been propagated, because should_update_metadata is true. // Adding an entry with a dummy etag to the database still makes sense here @@ -257,12 +303,71 @@ void PropagateLocalRename::start() return; } + auto targetParentFolderPath = std::filesystem::path{}; + auto targetParentFolderWasReadOnly = false; + try { + const auto newDirPath = std::filesystem::path{targetFile.toStdWString()}; + Q_ASSERT(newDirPath.has_parent_path()); + targetParentFolderPath = newDirPath.parent_path(); + if (FileSystem::isFolderReadOnly(targetParentFolderPath)) { + targetParentFolderWasReadOnly = true; + FileSystem::setFolderPermissions(QString::fromStdWString(targetParentFolderPath.wstring()), FileSystem::FolderPermissions::ReadWrite); + emit propagator()->touchedFile(QString::fromStdWString(targetParentFolderPath.wstring())); + } + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcPropagateLocalRename) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + } + + auto originParentFolderPath = std::filesystem::path{}; + auto originParentFolderWasReadOnly = false; + try { + const auto newDirPath = std::filesystem::path{existingFile.toStdWString()}; + Q_ASSERT(newDirPath.has_parent_path()); + originParentFolderPath = newDirPath.parent_path(); + if (FileSystem::isFolderReadOnly(originParentFolderPath)) { + originParentFolderWasReadOnly = true; + FileSystem::setFolderPermissions(QString::fromStdWString(originParentFolderPath.wstring()), FileSystem::FolderPermissions::ReadWrite); + emit propagator()->touchedFile(QString::fromStdWString(originParentFolderPath.wstring())); + } + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcPropagateLocalRename) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + } + + const auto restoreTargetPermissions = [this] (const auto &parentFolderPath) { + try { + FileSystem::setFolderPermissions(QString::fromStdWString(parentFolderPath.wstring()), FileSystem::FolderPermissions::ReadOnly); + emit propagator()->touchedFile(QString::fromStdWString(parentFolderPath.wstring())); + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcPropagateLocalRename) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + } + }; + emit propagator()->touchedFile(existingFile); emit propagator()->touchedFile(targetFile); if (QString renameError; !FileSystem::rename(existingFile, targetFile, &renameError)) { + if (targetParentFolderWasReadOnly) { + restoreTargetPermissions(targetParentFolderPath); + } + if (originParentFolderWasReadOnly) { + restoreTargetPermissions(originParentFolderPath); + } + done(SyncFileItem::NormalError, renameError, ErrorCategory::GenericError); return; } + + if (targetParentFolderWasReadOnly) { + restoreTargetPermissions(targetParentFolderPath); + } + if (originParentFolderWasReadOnly) { + restoreTargetPermissions(originParentFolderPath); + } } SyncJournalFileRecord oldRecord; diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 6eea5fe7af182..401915e6db6ba 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -12,6 +12,7 @@ #include #include +#include using namespace OCC; @@ -590,6 +591,156 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); } + + static void demo_perms(std::filesystem::perms p) + { + using std::filesystem::perms; + auto show = [=](char op, perms perm) + { + std::cout << (perms::none == (perm & p) ? '-' : op); + }; + show('r', perms::owner_read); + show('w', perms::owner_write); + show('x', perms::owner_exec); + show('r', perms::group_read); + show('w', perms::group_write); + show('x', perms::group_exec); + show('r', perms::others_read); + show('w', perms::others_write); + show('x', perms::others_exec); + std::cout << std::endl; + } + + void testReadOnlyFolderIsReallyReadOnly() + { + FakeFolder fakeFolder{FileInfo{}}; + + auto &remote = fakeFolder.remoteModifier(); + + remote.mkdir("readOnlyFolder"); + + remote.find("readOnlyFolder")->permissions = RemotePermissions::fromServerString("M"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + const auto folderStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/readOnlyFolder")).toStdWString()); + QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read); + } + + void testReadWriteFolderIsReallyReadWrite() + { + FakeFolder fakeFolder{FileInfo{}}; + + auto &remote = fakeFolder.remoteModifier(); + + remote.mkdir("readWriteFolder"); + + remote.find("readWriteFolder")->permissions = RemotePermissions::fromServerString("WDNVRSM"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + const auto folderStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/readWriteFolder")).toStdWString()); + QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_write); + } + + void testChangePermissionsFolder() + { + FakeFolder fakeFolder{FileInfo{}}; + + auto &remote = fakeFolder.remoteModifier(); + + remote.mkdir("testFolder"); + + remote.find("testFolder")->permissions = RemotePermissions::fromServerString("M"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + auto folderStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()); + QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(!static_cast(folderStatus.permissions() & std::filesystem::perms::owner_write)); + + remote.find("testFolder")->permissions = RemotePermissions::fromServerString("WDNVRSM"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + folderStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()); + QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_write); + + remote.find("testFolder")->permissions = RemotePermissions::fromServerString("M"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + folderStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()); + QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(!static_cast(folderStatus.permissions() & std::filesystem::perms::owner_write)); + } + + void testChangePermissionsForFolderHierarchy() + { + FakeFolder fakeFolder{FileInfo{}}; + + auto &remote = fakeFolder.remoteModifier(); + + remote.mkdir("testFolder"); + + remote.find("testFolder")->permissions = RemotePermissions::fromServerString("M"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + remote.mkdir("testFolder/subFolderReadWrite"); + remote.mkdir("testFolder/subFolderReadOnly"); + + remote.find("testFolder/subFolderReadOnly")->permissions = RemotePermissions::fromServerString("m"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + auto testFolderStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()); + QVERIFY(testFolderStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(!static_cast(testFolderStatus.permissions() & std::filesystem::perms::owner_write)); + auto subFolderReadWriteStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadWrite")).toStdWString()); + QVERIFY(subFolderReadWriteStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(subFolderReadWriteStatus.permissions() & std::filesystem::perms::owner_write); + auto subFolderReadOnlyStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadOnly")).toStdWString()); + QVERIFY(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(!static_cast(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_write)); + + remote.find("testFolder/subFolderReadOnly")->permissions = RemotePermissions::fromServerString("WDNVRSm"); + remote.find("testFolder/subFolderReadWrite")->permissions = RemotePermissions::fromServerString("m"); + remote.mkdir("testFolder/newSubFolder"); + remote.create("testFolder/testFile", 12, '9'); + remote.create("testFolder/testReadOnlyFile", 13, '8'); + remote.find("testFolder/testReadOnlyFile")->permissions = RemotePermissions::fromServerString("m"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + subFolderReadWriteStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadWrite")).toStdWString()); + QVERIFY(subFolderReadWriteStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(!static_cast(subFolderReadWriteStatus.permissions() & std::filesystem::perms::owner_write)); + subFolderReadOnlyStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadOnly")).toStdWString()); + QVERIFY(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_write); + + remote.rename("testFolder/subFolderReadOnly", "testFolder/subFolderReadWriteNew"); + remote.rename("testFolder/subFolderReadWrite", "testFolder/subFolderReadOnlyNew"); + remote.rename("testFolder/testFile", "testFolder/testFileNew"); + remote.rename("testFolder/testReadOnlyFile", "testFolder/testReadOnlyFileNew"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + testFolderStatus = std::filesystem::status(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()); + QVERIFY(testFolderStatus.permissions() & std::filesystem::perms::owner_read); + QVERIFY(!static_cast(testFolderStatus.permissions() & std::filesystem::perms::owner_write)); + } }; QTEST_GUILESS_MAIN(TestPermissions) diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 15ac965f166f2..82a3c23906775 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -95,6 +95,11 @@ private slots: Logger::instance()->setLogDebug(true); } + void init() + { + QTextCodec::setCodecForLocale(QTextCodec::codecForName("UTF-8")); + } + void testFileDownload() { FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; ItemCompletedSpy completeSpy(fakeFolder); @@ -845,12 +850,12 @@ private slots: QVERIFY(fakeFolder.syncOnce()); QVERIFY(fakeFolder.currentRemoteState().find("C/tößt")); - QTextCodec::setCodecForLocale(utf8Locale); } catch (const std::filesystem::filesystem_error &e) { qCritical() << e.what() << e.path1().c_str() << e.path2().c_str() << e.code().message().c_str(); } + QTextCodec::setCodecForLocale(utf8Locale); #endif } From ff9953b36b5644ec751682687f7257fe9cedfb2b Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Mon, 11 Mar 2024 14:57:24 +0100 Subject: [PATCH 10/10] make folder read-write before deleting it Signed-off-by: Matthieu Gallien --- src/libsync/propagatorjobs.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index e016fd0652d84..41b253bb09874 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -60,6 +60,7 @@ bool PropagateLocalRemove::removeRecursively(const QString &path) QString absolute = propagator()->fullLocalPath(_item->_file + path); QStringList errors; QList> deleted; + FileSystem::setFolderPermissions(absolute, FileSystem::FolderPermissions::ReadWrite); bool success = FileSystem::removeRecursively( absolute, [&deleted](const QString &path, bool isDir) {