Skip to content

Commit

Permalink
Merge pull request #7532 from nextcloud/bugfix/narrowDownPermissionsD…
Browse files Browse the repository at this point in the history 
…uringSync

Bugfix/narrow down permissions during sync
  • Loading branch information
@mgallien
mgallien authored Nov 21, 2024
2 parents 728cff3 + 1417e8c commit 1e378a9
Show file tree
Hide file tree
Showing 10 changed files with 39 additions and 83 deletions.
2 changes: 2 additions & 0 deletions src/csync/csync.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ struct OCSYNC_EXPORT csync_file_stat_s {
bool is_hidden BITFIELD(1); // Not saved in the DB, only used during discovery for local files.
bool isE2eEncrypted BITFIELD(1);
bool is_metadata_missing BITFIELD(1); // Indicates the file has missing metadata, f.ex. the file is not a placeholder in case of vfs.
bool isPermissionsInvalid BITFIELD(1);

QByteArray path;
QByteArray rename_path;
Expand Down Expand Up @@ -244,6 +245,7 @@ struct OCSYNC_EXPORT csync_file_stat_s {
, is_hidden(false)
, isE2eEncrypted(false)
, is_metadata_missing(false)
, isPermissionsInvalid(false)
{ }
};

Expand Down
2 changes: 2 additions & 0 deletions src/csync/vio/csync_vio_local_unix.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,5 +170,7 @@ static int _csync_vio_local_stat_mb(const mbchar_t *wuri, csync_file_stat_t *buf
buf->inode = sb.st_ino;
buf->modtime = sb.st_mtime;
buf->size = sb.st_size;
buf->isPermissionsInvalid = (sb.st_mode & S_IWOTH) == S_IWOTH;

return 0;
}
11 changes: 11 additions & 0 deletions src/libsync/discovery.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,10 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
if (_queryLocal != NormalQuery && _queryServer != NormalQuery)
recurse = false;

if (localEntry.isPermissionsInvalid) {
recurse = true;
}

if ((item->_direction == SyncFileItem::Down || item->_instruction == CSYNC_INSTRUCTION_CONFLICT || item->_instruction == CSYNC_INSTRUCTION_NEW || item->_instruction == CSYNC_INSTRUCTION_SYNC) &&
(item->_modtime <= 0 || item->_modtime >= 0xFFFFFFFF)) {
item->_instruction = CSYNC_INSTRUCTION_ERROR;
Expand Down Expand Up @@ -1097,6 +1101,13 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
}
}

if (localEntry.isPermissionsInvalid && item->_instruction == CSyncEnums::CSYNC_INSTRUCTION_NONE) {
item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
item->_direction = SyncFileItem::Down;
}

item->isPermissionsInvalid = localEntry.isPermissionsInvalid;

auto recurseQueryLocal = _queryLocal == ParentNotChanged ? ParentNotChanged : localEntry.isDirectory || item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist;
processFileFinalize(item, path, recurse, recurseQueryLocal, recurseQueryServer);
};
Expand Down
1 change: 1 addition & 0 deletions src/libsync/discoveryphase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ void DiscoverySingleLocalDirectoryJob::run() {
i.isSymLink = dirent->type == ItemTypeSoftLink;
i.isVirtualFile = dirent->type == ItemTypeVirtualFile || dirent->type == ItemTypeVirtualFileDownload;
i.isMetadataMissing = dirent->is_metadata_missing;
i.isPermissionsInvalid = dirent->isPermissionsInvalid;
i.type = dirent->type;
results.push_back(i);
}
Expand Down
1 change: 1 addition & 0 deletions src/libsync/discoveryphase.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ struct LocalInfo
bool isVirtualFile = false;
bool isSymLink = false;
bool isMetadataMissing = false;
bool isPermissionsInvalid = false;
[[nodiscard]] bool isValid() const { return !name.isNull(); }
};

Expand Down
1 change: 1 addition & 0 deletions src/libsync/filesystem.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ bool FileSystem::setFolderPermissions(const QString &path,
case OCC::FileSystem::FolderPermissions::ReadOnly:
break;
case OCC::FileSystem::FolderPermissions::ReadWrite:
std::filesystem::permissions(stdStrPath, std::filesystem::perms::others_write, std::filesystem::perm_options::remove);
std::filesystem::permissions(stdStrPath, std::filesystem::perms::owner_write, std::filesystem::perm_options::add);
break;
}
Expand Down
35 changes: 12 additions & 23 deletions src/libsync/owncloudpropagator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1457,21 +1457,13 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
#if !defined(Q_OS_MACOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_15
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 (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) {
FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadOnly);
qCDebug(lcDirectory) << "old permissions" << static_cast<int>(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<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions());
}
if (!_item->_renameTarget.isEmpty() && FileSystem::fileExists(propagator()->fullLocalPath(_item->_renameTarget))) {
FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadOnly);
qCDebug(lcDirectory) << "old permissions" << static_cast<int>(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<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions());
}
}
catch (const std::filesystem::filesystem_error &e)
Expand All @@ -1480,23 +1472,20 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
_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))) {
} else {
try {
if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) {
FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadWrite);
qCDebug(lcDirectory) << "old permissions" << static_cast<int>(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<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions());
const auto permissionsChangeHelper = [] (const auto fileName)
{
qCDebug(lcDirectory) << fileName << "permissions changed: old permissions" << static_cast<int>(std::filesystem::status(fileName.toStdWString()).permissions());
FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadWrite);
qCDebug(lcDirectory) << fileName << "applied new permissions" << static_cast<int>(std::filesystem::status(fileName.toStdWString()).permissions());
};

if (const auto fileName = propagator()->fullLocalPath(_item->_file); FileSystem::fileExists(fileName)) {
permissionsChangeHelper(fileName);
}
if (!_item->_renameTarget.isEmpty() && FileSystem::fileExists(propagator()->fullLocalPath(_item->_renameTarget))) {
FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadWrite);
qCDebug(lcDirectory) << "old permissions" << static_cast<int>(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<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions());
if (const auto fileName = propagator()->fullLocalPath(_item->_renameTarget); !_item->_renameTarget.isEmpty() && FileSystem::fileExists(fileName)) {
permissionsChangeHelper(fileName);
}
}
catch (const std::filesystem::filesystem_error &e)
Expand Down
4 changes: 4 additions & 0 deletions src/libsync/syncengine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,10 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item)
const bool isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite);
modificationHappened = FileSystem::setFileReadOnlyWeak(filePath, isReadOnly);
}
if (item->isPermissionsInvalid) {
const auto isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite);
FileSystem::setFileReadOnly(filePath, isReadOnly);
}

modificationHappened |= item->_size != prev._fileSize;

Expand Down
2 changes: 2 additions & 0 deletions src/libsync/syncfileitem.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,8 @@ class OWNCLOUDSYNC_EXPORT SyncFileItem
bool _isLivePhoto = false;
QString _livePhotoFile;

bool isPermissionsInvalid = false;

QString _discoveryResult;
};

Expand Down
63 changes: 3 additions & 60 deletions test/testpermissions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,63 +564,6 @@ private slots:
QVERIFY(cls.find("zallowed/sub2/file"));
}

// Test for issue #7293
void testAllowedMoveForbiddenDelete() {
FakeFolder fakeFolder{FileInfo{}};

QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
for(const auto &oneFolder : folders) {
FileSystem::removeRecursively(localPath + oneFolder->_file);
}
callback(false);
});

// Some of this test depends on the order of discovery. With threading
// that order becomes effectively random, but we want to make sure to test
// all cases and thus disable threading.
auto syncOpts = fakeFolder.syncEngine().syncOptions();
syncOpts._parallelNetworkJobs = 1;
fakeFolder.syncEngine().setSyncOptions(syncOpts);

auto &lm = fakeFolder.localModifier();
auto &rm = fakeFolder.remoteModifier();
rm.mkdir("changeonly");
rm.mkdir("changeonly/sub1");
rm.insert("changeonly/sub1/file1");
rm.insert("changeonly/sub1/filetorname1a");
rm.insert("changeonly/sub1/filetorname1z");
rm.mkdir("changeonly/sub2");
rm.insert("changeonly/sub2/file2");
rm.insert("changeonly/sub2/filetorname2a");
rm.insert("changeonly/sub2/filetorname2z");

setAllPerm(rm.find("changeonly"), RemotePermissions::fromServerString("NSV"));

QVERIFY(fakeFolder.syncOnce());

lm.rename("changeonly/sub1/filetorname1a", "changeonly/sub1/aaa1_renamed");
lm.rename("changeonly/sub1/filetorname1z", "changeonly/sub1/zzz1_renamed");

lm.rename("changeonly/sub2/filetorname2a", "changeonly/sub2/aaa2_renamed");
lm.rename("changeonly/sub2/filetorname2z", "changeonly/sub2/zzz2_renamed");

auto expectedState = fakeFolder.currentLocalState();

QVERIFY(fakeFolder.syncOnce());
QCOMPARE(fakeFolder.currentLocalState(), expectedState);
QCOMPARE(fakeFolder.currentRemoteState(), expectedState);

lm.rename("changeonly/sub1", "changeonly/aaa");
lm.rename("changeonly/sub2", "changeonly/zzz");

expectedState = fakeFolder.currentLocalState();

QVERIFY(fakeFolder.syncOnce());
QCOMPARE(fakeFolder.currentLocalState(), expectedState);
QCOMPARE(fakeFolder.currentRemoteState(), expectedState);
}

void testParentMoveNotAllowedChildrenRestored()
{
FakeFolder fakeFolder{FileInfo{}};
Expand Down Expand Up @@ -722,7 +665,7 @@ private slots:

remote.mkdir("readWriteFolder");

remote.find("readWriteFolder")->permissions = RemotePermissions::fromServerString("WDNVRSM");
remote.find("readWriteFolder")->permissions = RemotePermissions::fromServerString("CKWDNVRSM");

QVERIFY(fakeFolder.syncOnce());
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
Expand Down Expand Up @@ -757,7 +700,7 @@ private slots:
QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read);
QVERIFY(!static_cast<bool>(folderStatus.permissions() & std::filesystem::perms::owner_write));

remote.find("testFolder")->permissions = RemotePermissions::fromServerString("WDNVRSM");
remote.find("testFolder")->permissions = RemotePermissions::fromServerString("CKWDNVRSM");

QVERIFY(fakeFolder.syncOnce());
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
Expand Down Expand Up @@ -815,7 +758,7 @@ private slots:
QVERIFY(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_read);
QVERIFY(!static_cast<bool>(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_write));

remote.find("testFolder/subFolderReadOnly")->permissions = RemotePermissions::fromServerString("WDNVRSm");
remote.find("testFolder/subFolderReadOnly")->permissions = RemotePermissions::fromServerString("CKWDNVRSm");
remote.find("testFolder/subFolderReadWrite")->permissions = RemotePermissions::fromServerString("m");
remote.mkdir("testFolder/newSubFolder");
remote.create("testFolder/testFile", 12, '9');
Expand Down

0 comments on commit 1e378a9

Please sign in to comment.