From 0aba0b296eb731a6447a95a0c0f99015242a6130 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 17 Apr 2025 11:57:22 +0200 Subject: [PATCH 1/6] feat(windows native API): use windows specific API for read-only folders Signed-off-by: Matthieu Gallien --- src/libsync/filesystem.cpp | 129 ++++++++++++++++++++++++++++--------- 1 file changed, 97 insertions(+), 32 deletions(-) diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp index 5371ae35a02a4..ac8a1814bcf84 100644 --- a/src/libsync/filesystem.cpp +++ b/src/libsync/filesystem.cpp @@ -334,34 +334,6 @@ bool FileSystem::getInode(const QString &filename, quint64 *inode) bool FileSystem::setFolderPermissions(const QString &path, FileSystem::FolderPermissions permissions) noexcept { - static constexpr auto writePerms = std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write; - const auto stdStrPath = path.toStdWString(); - try - { - switch (permissions) { - case OCC::FileSystem::FolderPermissions::ReadOnly: - std::filesystem::permissions(stdStrPath, writePerms, 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() << "- path1:" << e.path1().c_str() << "- path2:" << e.path2().c_str(); - return false; - } - catch (const std::system_error &e) - { - qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << "- path:" << stdStrPath; - return false; - } - catch (...) - { - qCWarning(lcFileSystem()) << "exception when modifying folder permissions - path:" << stdStrPath; - return false; - } - #ifdef Q_OS_WIN SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION; std::unique_ptr securityDescriptor; @@ -429,6 +401,10 @@ bool FileSystem::setFolderPermissions(const QString &path, } } + if (permissions == FileSystem::FolderPermissions::ReadWrite) { + qCInfo(lcFileSystem) << path << "will be read write"; + } + for (int i = 0; i < aclSize.AceCount; ++i) { void *currentAce = nullptr; if (!GetAce(resultDacl, i, ¤tAce)) { @@ -438,9 +414,6 @@ bool FileSystem::setFolderPermissions(const QString &path, 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; @@ -478,7 +451,34 @@ bool FileSystem::setFolderPermissions(const QString &path, qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << path << GetLastError(); return false; } -#endif +#else + static constexpr auto writePerms = std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write; + const auto stdStrPath = path.toStdWString(); + try + { + switch (permissions) { + case OCC::FileSystem::FolderPermissions::ReadOnly: + std::filesystem::permissions(stdStrPath, writePerms, 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() << "- path1:" << e.path1().c_str() << "- path2:" << e.path2().c_str(); + return false; + } + catch (const std::system_error &e) + { + qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << "- path:" << stdStrPath; + return false; + } + catch (...) + { + qCWarning(lcFileSystem()) << "exception when modifying folder permissions - path:" << stdStrPath; + return false; + } try { @@ -506,12 +506,76 @@ bool FileSystem::setFolderPermissions(const QString &path, qCWarning(lcFileSystem()) << "exception when modifying folder permissions - path:" << stdStrPath; return false; } +#endif return true; } bool FileSystem::isFolderReadOnly(const std::filesystem::path &path) noexcept { +#ifdef Q_OS_WIN + qCInfo(lcFileSystem()) << "is it read-only folder:" << path.wstring().c_str(); + + SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION; + std::unique_ptr securityDescriptor; + auto neededLength = 0ul; + + if (!GetFileSecurityW(path.wstring().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.wstring().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 || !resultDacl) { + 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; + } + + 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 ((ACCESS_DENIED_ACE_TYPE == (currentAceHeader->AceType & ACCESS_DENIED_ACE_TYPE))) { + qCInfo(lcFileSystem()) << "detected access denied ACL: assuming read-only folder:" << path.wstring().c_str(); + return true; + } + } + + return false; +#else try { const auto folderStatus = std::filesystem::status(path); @@ -533,6 +597,7 @@ bool FileSystem::isFolderReadOnly(const std::filesystem::path &path) noexcept qCWarning(lcFileSystem()) << "exception when checking folder permissions - path:" << path; return false; } +#endif } FileSystem::FilePermissionsRestore::FilePermissionsRestore(const QString &path, FolderPermissions temporaryPermissions) From aeca14f5228de35a1307646ee4c072674bad451e Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 17 Apr 2025 14:35:02 +0200 Subject: [PATCH 2/6] fix(read only folder): allow renaming new file inside read-only folders needed to download a new file inside a read-only folder Signed-off-by: Matthieu Gallien --- src/common/filesystembase.cpp | 48 ----------------------------------- src/common/filesystembase.h | 8 ------ src/libsync/filesystem.cpp | 48 +++++++++++++++++++++++++++++++++++ src/libsync/filesystem.h | 8 ++++++ test/testpermissions.cpp | 42 +++++++++++------------------- 5 files changed, 71 insertions(+), 83 deletions(-) diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index 660e2d382c648..9cae216b0bac9 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -245,54 +245,6 @@ bool FileSystem::rename(const QString &originFileName, return success; } -bool FileSystem::uncheckedRenameReplace(const QString &originFileName, - const QString &destinationFileName, - QString *errorString) -{ -#ifndef Q_OS_WIN - bool success = false; - QFile orig(originFileName); - // We want a rename that also overwrites. QFile::rename does not overwrite. - // Qt 5.1 has QSaveFile::renameOverwrite we could use. - // ### FIXME - success = true; - bool destExists = fileExists(destinationFileName); - if (destExists && !QFile::remove(destinationFileName)) { - *errorString = orig.errorString(); - qCWarning(lcFileSystem) << "Target file could not be removed."; - success = false; - } - if (success) { - success = orig.rename(destinationFileName); - } - if (!success) { - *errorString = orig.errorString(); - qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString; - return false; - } - -#else //Q_OS_WIN - // You can not overwrite a read-only file on windows. - if (!isWritable(destinationFileName)) { - setFileReadOnly(destinationFileName, false); - } - - BOOL ok = 0; - QString orig = longWinPath(originFileName); - QString dest = longWinPath(destinationFileName); - - ok = MoveFileEx((wchar_t *)orig.utf16(), - (wchar_t *)dest.utf16(), - MOVEFILE_REPLACE_EXISTING + MOVEFILE_COPY_ALLOWED + MOVEFILE_WRITE_THROUGH); - if (!ok) { - *errorString = Utility::formatWinError(GetLastError()); - qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString; - return false; - } -#endif - return true; -} - bool FileSystem::openAndSeekFileSharedRead(QFile *file, QString *errorOrNull, qint64 seek) { QString errorDummy; diff --git a/src/common/filesystembase.h b/src/common/filesystembase.h index 8f5554000e079..e9547d2d0a7b6 100644 --- a/src/common/filesystembase.h +++ b/src/common/filesystembase.h @@ -132,14 +132,6 @@ namespace FileSystem { const QString &destinationFileName, QString *errorString = nullptr); - /** - * Rename the file \a originFileName to \a destinationFileName, and - * overwrite the destination if it already exists - without extra checks. - */ - bool OCSYNC_EXPORT uncheckedRenameReplace(const QString &originFileName, - const QString &destinationFileName, - QString *errorString); - /** * Removes a file. * diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp index ac8a1814bcf84..002a031de3a5d 100644 --- a/src/libsync/filesystem.cpp +++ b/src/libsync/filesystem.cpp @@ -633,4 +633,52 @@ FileSystem::FilePermissionsRestore::~FilePermissionsRestore() } } +bool FileSystem::uncheckedRenameReplace(const QString &originFileName, const QString &destinationFileName, QString *errorString) +{ +#ifndef Q_OS_WIN + bool success = false; + QFile orig(originFileName); + // We want a rename that also overwrites. QFile::rename does not overwrite. + // Qt 5.1 has QSaveFile::renameOverwrite we could use. + // ### FIXME + success = true; + bool destExists = fileExists(destinationFileName); + if (destExists && !QFile::remove(destinationFileName)) { + *errorString = orig.errorString(); + qCWarning(lcFileSystem) << "Target file could not be removed."; + success = false; + } + if (success) { + success = orig.rename(destinationFileName); + } + if (!success) { + *errorString = orig.errorString(); + qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString; + return false; + } +#else //Q_OS_WIN + const auto originFileInfo = QFileInfo{originFileName}; + const auto originParentFolderPath = originFileInfo.dir().absolutePath(); + FilePermissionsRestore renameEnabler{originParentFolderPath, FileSystem::FolderPermissions::ReadWrite}; + // You can not overwrite a read-only file on windows. + if (!isWritable(destinationFileName)) { + setFileReadOnly(destinationFileName, false); + } + + BOOL ok = 0; + QString orig = longWinPath(originFileName); + QString dest = longWinPath(destinationFileName); + + ok = MoveFileEx((wchar_t *)orig.utf16(), + (wchar_t *)dest.utf16(), + MOVEFILE_REPLACE_EXISTING + MOVEFILE_COPY_ALLOWED + MOVEFILE_WRITE_THROUGH); + if (!ok) { + *errorString = Utility::formatWinError(GetLastError()); + qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString; + return false; + } +#endif + return true; +} + } // namespace OCC diff --git a/src/libsync/filesystem.h b/src/libsync/filesystem.h index 26c999994394f..2682f180b84d5 100644 --- a/src/libsync/filesystem.h +++ b/src/libsync/filesystem.h @@ -130,6 +130,14 @@ namespace FileSystem { FileSystem::FolderPermissions permissions) noexcept; bool OWNCLOUDSYNC_EXPORT isFolderReadOnly(const std::filesystem::path &path) noexcept; + + /** + * Rename the file \a originFileName to \a destinationFileName, and + * overwrite the destination if it already exists - without extra checks. + */ + bool OWNCLOUDSYNC_EXPORT uncheckedRenameReplace(const QString &originFileName, + const QString &destinationFileName, + QString *errorString); } /** @} */ diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 8e457e0b65c66..39aeb8589e0f0 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -45,6 +45,11 @@ static void assertCsyncJournalOk(SyncJournalDb &journal) #endif } +static bool isReadOnlyFolder(const std::wstring &path) +{ + return FileSystem::isFolderReadOnly(std::filesystem::path{path}); +} + SyncFileItemPtr findDiscoveryItem(const SyncFileItemVector &spy, const QString &path) { for (const auto &item : spy) { @@ -644,27 +649,21 @@ private slots: 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)); + QVERIFY(isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString())); remote.find("testFolder")->permissions = RemotePermissions::fromServerString("CKWDNVRSM"); 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); + QVERIFY(!isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString())); 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)); + QVERIFY(isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString())); } void testChangePermissionsForFolderHierarchy() @@ -688,15 +687,9 @@ private slots: 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)); + QVERIFY(isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString())); + QVERIFY(!isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadWrite")).toStdWString())); + QVERIFY(isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadOnly")).toStdWString())); remote.find("testFolder/subFolderReadOnly")->permissions = RemotePermissions::fromServerString("CKWDNVRSm"); remote.find("testFolder/subFolderReadWrite")->permissions = RemotePermissions::fromServerString("m"); @@ -708,12 +701,9 @@ private slots: 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); + QVERIFY(isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString())); + QVERIFY(isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadWrite")).toStdWString())); + QVERIFY(!isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadOnly")).toStdWString())); remote.rename("testFolder/subFolderReadOnly", "testFolder/subFolderReadWriteNew"); remote.rename("testFolder/subFolderReadWrite", "testFolder/subFolderReadOnlyNew"); @@ -722,9 +712,7 @@ private slots: 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)); + QVERIFY(isReadOnlyFolder(static_cast(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString())); } void testDeleteChildItemsInReadOnlyFolder() From 98599661834ddc295012924a403ca99639507a43 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 17 Apr 2025 17:26:12 +0200 Subject: [PATCH 3/6] fix(permissions): solve issues with file permissions automated tests Signed-off-by: Matthieu Gallien --- test/syncenginetestutils.cpp | 19 ++++++++++++----- test/testpermissions.cpp | 41 ++++++++++++++++-------------------- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/test/syncenginetestutils.cpp b/test/syncenginetestutils.cpp index e4a5d12a7c682..58fecc6e8e6cd 100644 --- a/test/syncenginetestutils.cpp +++ b/test/syncenginetestutils.cpp @@ -51,9 +51,12 @@ void DiskFileModifier::remove(const QString &relativePath) if (fi.isFile()) { QVERIFY(_rootDir.remove(relativePath)); } 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)); + const auto pathToDelete = fi.filePath(); + const auto result = OCC::FileSystem::removeRecursively(pathToDelete); + if (!result) { + qDebug() << "delete failed for:" << pathToDelete; + QVERIFY(result); + } } } @@ -70,7 +73,9 @@ void DiskFileModifier::insert(const QString &relativePath, qint64 size, char con file.close(); // Set the mtime 30 seconds in the past, for some tests that need to make sure that the mtime differs. OCC::FileSystem::setModTime(file.fileName(), OCC::Utility::qDateTimeToTime_t(QDateTime::currentDateTimeUtc().addSecs(-30))); - QCOMPARE(file.size(), size); + if (file.size() != size) { + QCOMPARE(file.size(), size); + } } void DiskFileModifier::setContents(const QString &relativePath, char contentChar) @@ -100,7 +105,11 @@ void DiskFileModifier::mkdir(const QString &relativePath) void DiskFileModifier::rename(const QString &from, const QString &to) { QVERIFY(_rootDir.exists(from)); - QVERIFY(_rootDir.rename(from, to)); + const auto result = _rootDir.rename(from, to); + if (!result) { + qDebug() << "failed to rename from:" << from << "to:" << to; + QVERIFY(result); + } } void DiskFileModifier::setModTime(const QString &relativePath, const QDateTime &modTime) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 39aeb8589e0f0..6bd3723fa87dd 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -134,28 +134,32 @@ private slots: qInfo("Do some changes and see how they propagate"); const auto removeReadOnly = [&] (const QString &file) { - try { - 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); + const auto fileInfoToDelete = QFileInfo(fakeFolder.localPath() + file); + FileSystem::FilePermissionsRestore enabler{fileInfoToDelete.absolutePath(), FileSystem::FolderPermissions::ReadWrite}; + if (!fileInfoToDelete.isDir()) { + QString errorString; + const auto result = FileSystem::remove(fileInfoToDelete.absoluteFilePath(), &errorString); + if (!result) { + qDebug() << "fail to delete:" << fileInfoToDelete.absoluteFilePath() << errorString; + //QVERIFY(result); } - fakeFolder.localModifier().remove(file); - if (isReadOnly) { - std::filesystem::permissions(fileInfoToDelete.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::remove); + } else { + const auto result = FileSystem::removeRecursively(fileInfoToDelete.absoluteFilePath()); + if (!result) { + qDebug() << "fail to delete:" << fileInfoToDelete.absoluteFilePath(); + QVERIFY(result); } } - catch (const std::exception& e) - { - qWarning() << e.what(); - } }; const auto renameReadOnly = [&] (const QString &relativePath, const QString &relativeDestinationDirectory) { try { const auto sourceFileInfo = QFileInfo(fakeFolder.localPath() + relativePath); + FileSystem::FilePermissionsRestore sourceEnabler{sourceFileInfo.absolutePath(), FileSystem::FolderPermissions::ReadWrite}; + const auto destinationFileInfo = QFileInfo(fakeFolder.localPath() + relativeDestinationDirectory); + FileSystem::FilePermissionsRestore destinationEnabler{destinationFileInfo.absolutePath(), FileSystem::FolderPermissions::ReadWrite}; + 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) { @@ -181,14 +185,8 @@ private slots: const auto insertReadOnly = [&] (const QString &file, const int fileSize) { try { 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); - } + FileSystem::FilePermissionsRestore enabler{fileInfo.absolutePath(), FileSystem::FolderPermissions::ReadWrite}; fakeFolder.localModifier().insert(file, fileSize); - if (isReadOnly) { - std::filesystem::permissions(fileInfo.absolutePath().toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::remove); - } } catch (const std::exception& e) { @@ -239,9 +237,6 @@ private slots: //2. // File should be deleted QVERIFY(!currentLocalState.find("normalDirectory_PERM_CKDNV_/canBeRemoved_PERM_D_.data")); -#ifdef Q_OS_WINDOWS - QEXPECT_FAIL("", "", Abort); -#endif QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_M_/canBeRemoved_PERM_D_.data")); //3. From 29ccf5950d2329304aa3ab7969d9c396771a3d93 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 17 Apr 2025 19:18:40 +0200 Subject: [PATCH 4/6] refactor(logs): improve logs around deletions and permissions Signed-off-by: Matthieu Gallien --- src/common/filesystembase.cpp | 1 + src/libsync/filesystem.cpp | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index 9cae216b0bac9..c6dcb49d1c9d0 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -550,6 +550,7 @@ bool FileSystem::remove(const QString &fileName, QString *errorString) qCWarning(lcFileSystem()) << "File is already deleted" << fileName; return false; } + qCInfo(lcFileSystem()) << "delete" << fileName; } catch (const std::filesystem::filesystem_error &e) { diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp index 002a031de3a5d..52e8f11ad6760 100644 --- a/src/libsync/filesystem.cpp +++ b/src/libsync/filesystem.cpp @@ -281,6 +281,7 @@ bool FileSystem::removeRecursively(const QString &path, const std::function securityDescriptor; @@ -569,7 +571,7 @@ bool FileSystem::isFolderReadOnly(const std::filesystem::path &path) noexcept const auto currentAceHeader = reinterpret_cast(currentAce); if ((ACCESS_DENIED_ACE_TYPE == (currentAceHeader->AceType & ACCESS_DENIED_ACE_TYPE))) { - qCInfo(lcFileSystem()) << "detected access denied ACL: assuming read-only folder:" << path.wstring().c_str(); + qCInfo(lcFileSystem()) << "detected access denied ACL: assuming read-only folder:" << QString::fromStdWString(path.wstring()); return true; } } From f85ea35b4efdbd12ec1a29356a55dbee283c6ed8 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 17 Apr 2025 19:21:49 +0200 Subject: [PATCH 5/6] fix(autotests): do not remove read-only files already removed the sync engine will remove invalid items inside read-only folders not needed to remove them in tests and rather checks that they were indeed removed Signed-off-by: Matthieu Gallien --- test/testpermissions.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 6bd3723fa87dd..38970d8ef1d03 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -141,7 +141,7 @@ private slots: const auto result = FileSystem::remove(fileInfoToDelete.absoluteFilePath(), &errorString); if (!result) { qDebug() << "fail to delete:" << fileInfoToDelete.absoluteFilePath() << errorString; - //QVERIFY(result); + QVERIFY(result); } } else { const auto result = FileSystem::removeRecursively(fileInfoToDelete.absoluteFilePath()); @@ -282,8 +282,6 @@ private slots: // The file should not exist on the remote, and not be there 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. - removeReadOnly("readonlyDirectory_PERM_M_/newFile_PERM_WDNV_.data"); // Both side should still be the same QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); @@ -365,8 +363,8 @@ private slots: QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/subdir_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data" )); // new no longer exists 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 - removeReadOnly("readonlyDirectory_PERM_M_/newname_PERM_CK_"); + // but is not on server: should have been locally removed + QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_M_/newname_PERM_CK_")); //2. // old removed @@ -375,8 +373,8 @@ private slots: QVERIFY(fakeFolder.currentRemoteState().find("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_")); // new no longer exists QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_M_/moved_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data" )); - //but not on server - removeReadOnly("readonlyDirectory_PERM_M_/moved_PERM_CK_"); + // should have been cleaned up as invalid item inside read-only folder + QVERIFY(!currentLocalState.find("readonlyDirectory_PERM_M_/moved_PERM_CK_")); fakeFolder.remoteModifier().remove("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_"); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); From 710e5fac94af7217da4dea7df50ccc806d1488a7 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 18 Apr 2025 16:05:07 +0200 Subject: [PATCH 6/6] fix(rmdir): switch to another API for folder removal current QDir::rmdir API does not provide an error message when failing to delete retuse FileSystem::remove that may just works with folders Signed-off-by: Matthieu Gallien --- src/libsync/filesystem.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp index 52e8f11ad6760..55d4c3c8b6b8c 100644 --- a/src/libsync/filesystem.cpp +++ b/src/libsync/filesystem.cpp @@ -304,7 +304,8 @@ bool FileSystem::removeRecursively(const QString &path, const std::function