From 3a8167e2954b6678bd9fc512cb7dd8d9590d730e Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Mon, 19 Feb 2024 17:46:02 +0100 Subject: [PATCH] fix failing automated tests and add new ones to make folders read-only Signed-off-by: Matthieu Gallien --- src/common/filesystembase.cpp | 31 +++++++++++---- src/libsync/owncloudpropagator.cpp | 11 ++++-- src/libsync/propagatorjobs.cpp | 53 +++++++++++++++++++++++++ test/testpermissions.cpp | 62 ++++++++++++++++++++++++++++++ test/testsyncengine.cpp | 7 +++- test/testsyncmove.cpp | 6 +++ 6 files changed, 159 insertions(+), 11 deletions(-) diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index ef9ba1efd6778..da3308252d795 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -26,6 +26,8 @@ #include #include +#include + #include #include @@ -146,12 +148,16 @@ bool FileSystem::rename(const QString &originFileName, } } else #endif + + try { + std::filesystem::rename(originFileName.toStdWString(), destinationFileName.toStdWString()); + success = true; + } + catch (const std::filesystem::filesystem_error &e) { - QFile orig(originFileName); - success = orig.rename(destinationFileName); - if (!success) { - error = orig.errorString(); - } + qCWarning(lcFileSystem) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + success = false; + error = QString::fromUtf8(e.what()); } if (!success) { @@ -183,10 +189,21 @@ bool FileSystem::uncheckedRenameReplace(const QString &originFileName, success = false; } if (success) { - success = orig.rename(destinationFileName); + try { + std::filesystem::rename(originFileName.toStdString(), destinationFileName.toStdString()); + success = true; + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcFileSystem) << "exception when checking parent folder access rights" << e.what() << e.path1().c_str() << e.path2().c_str(); + success = false; + *errorString = QString::fromUtf8(e.what()); + } + } else { + *errorString = orig.errorString(); } + if (!success) { - *errorString = orig.errorString(); qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString; return false; } diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 84c17d0e2f328..9bcd3053ebceb 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1474,13 +1474,18 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) !_item->_remotePerm.hasPermission(RemotePermissions::CanMove) || !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories))) { try { - std::filesystem::permissions(propagator()->fullLocalPath(_item->_file).toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + if (QFileInfo::exists(propagator()->fullLocalPath(_item->_file))) { + std::filesystem::permissions(propagator()->fullLocalPath(_item->_file).toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + } + if (QFileInfo::exists(propagator()->fullLocalPath(_item->_renameTarget))) { + std::filesystem::permissions(propagator()->fullLocalPath(_item->_renameTarget).toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add); + } } 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()); + _item->_errorString = tr("The folder %1 cannot be made read-only: %2").arg(e.path1().c_str(), e.what()); } } @@ -1496,7 +1501,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/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index dc8d955ac4adc..6e86cc906fca8 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -303,12 +303,65 @@ void PropagateLocalRename::start() return; } + auto targetParentFolderPath = std::filesystem::path{}; + auto targetParentPermissions = std::filesystem::perms{}; + try { + const auto newDirPath = std::filesystem::path{targetFile.toStdWString()}; + Q_ASSERT(newDirPath.has_parent_path()); + targetParentFolderPath = newDirPath.parent_path(); + auto parentFolderStatus = std::filesystem::status(targetParentFolderPath); + targetParentPermissions = parentFolderStatus.permissions(); + if ((targetParentPermissions & std::filesystem::perms::owner_write) != std::filesystem::perms::owner_write) { + std::filesystem::permissions(targetParentFolderPath, targetParentPermissions | std::filesystem::perms::owner_write, std::filesystem::perm_options::replace); + 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 originParentPermissions = std::filesystem::perms{}; + try { + const auto newDirPath = std::filesystem::path{existingFile.toStdWString()}; + Q_ASSERT(newDirPath.has_parent_path()); + originParentFolderPath = newDirPath.parent_path(); + auto parentFolderStatus = std::filesystem::status(originParentFolderPath); + originParentPermissions = parentFolderStatus.permissions(); + if ((originParentPermissions & std::filesystem::perms::owner_write) != std::filesystem::perms::owner_write) { + std::filesystem::permissions(originParentFolderPath, originParentPermissions | std::filesystem::perms::owner_write, std::filesystem::perm_options::replace); + 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, const auto &parentPermissions) { + try { + if ((parentPermissions & std::filesystem::perms::owner_write) != std::filesystem::perms::owner_write) { + std::filesystem::permissions(parentFolderPath, parentPermissions, std::filesystem::perm_options::replace); + 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)) { + restoreTargetPermissions(targetParentFolderPath, targetParentPermissions); + restoreTargetPermissions(originParentFolderPath, originParentPermissions); done(SyncFileItem::NormalError, renameError, ErrorCategory::GenericError); return; } + restoreTargetPermissions(targetParentFolderPath, targetParentPermissions); + restoreTargetPermissions(originParentFolderPath, originParentPermissions); } SyncJournalFileRecord oldRecord; diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 18cbac772f703..cda94d407dffd 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -661,6 +661,7 @@ private slots: 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"); @@ -678,6 +679,67 @@ private slots: 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)); } }; 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 } diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 04f29f8bebf96..5c119e8b580be 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -84,6 +84,12 @@ class TestSyncMove : public QObject Q_OBJECT private slots: + void initTestCase() + { + Logger::instance()->setLogFlush(true); + Logger::instance()->setLogDebug(true); + } + void testMoveCustomRemoteRoot() { FileInfo subFolder(QStringLiteral("AS"), { { QStringLiteral("f1"), 4 } });