fix(libsync): set read-only permissions after rename, not before#10213
fix(libsync): set read-only permissions after rename, not before#10213Lenart12 wants to merge 1 commit into
Conversation
Lenart12
requested review from
camilasan,
i2h3,
mgallien and
nilsding
as code owners
setFileReadOnly() on Windows adds an ACCESS_DENIED_ACE for BUILTIN\Users to the file's DACL. Applying this to the temp file before renaming it to its final destination blocks the rename, because Windows requires Delete access on the source path to complete the operation. This caused a persistent Error 5 (Access Denied) retry loop for read-only shares and Talk attachments on Windows. Move the permission block to after uncheckedRenameReplace() succeeds, targeting the final filename instead of _tmpFile.fileName(). Fixes: nextcloud#9885 Signed-off-by: Lenart Kos <koslenart@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a Windows-specific download failure where applying “read-only” ACLs to the temporary download file prevents the subsequent rename/replace operation, causing persistent Error 5 (Access Denied) retry loops for read-only shares (including Talk attachments). It does so by moving the read-only permission application to after the final rename succeeds.
Changes:
- Move
setFileReadOnly*()calls from the temporary file (_tmpFile.fileName()) to the final destination path (filename) afterFileSystem::uncheckedRenameReplace()succeeds. - Ensure the final file is un-hidden (
setFileHidden(filename, false)) before applying the read-only/read-write decision.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (_item->_locked == SyncFileItem::LockStatus::LockedItem && (_item->_lockOwnerType != SyncFileItem::LockOwnerType::UserLock || _item->_lockOwnerId != propagator()->account()->davUser())) { | ||
| qCDebug(lcPropagateDownload()) << filename << "file is locked: making it read only"; | ||
| FileSystem::setFileReadOnly(filename, true); | ||
| } else { | ||
| qCDebug(lcPropagateDownload()) << filename << "file is not locked: making it" | ||
| << ((!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) ? "read only" | ||
| : "read write"); | ||
| FileSystem::setFileReadOnlyWeak(filename, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); | ||
| } |
There was a problem hiding this comment.
This condition was moved as-is, so the divergence from updateMetadata() is pre-existing. And updateMetadata() runs right after on the same path with the full UserLock/TokenLock check, so it overwrites this block anyway. Behavior is unchanged. Aligning the checks is a fine cleanup but out of scope here.
| if (_item->_locked == SyncFileItem::LockStatus::LockedItem && (_item->_lockOwnerType != SyncFileItem::LockOwnerType::UserLock || _item->_lockOwnerId != propagator()->account()->davUser())) { | ||
| qCDebug(lcPropagateDownload()) << filename << "file is locked: making it read only"; | ||
| FileSystem::setFileReadOnly(filename, true); | ||
| } else { | ||
| qCDebug(lcPropagateDownload()) << filename << "file is not locked: making it" | ||
| << ((!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) ? "read only" | ||
| : "read write"); | ||
| FileSystem::setFileReadOnlyWeak(filename, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); | ||
| } |
There was a problem hiding this comment.
Real behavioral change, good catch. That said, a case-clash conflict file is a local artifact the user has to resolve manually, so dropping read-only may actually be preferable. @ maintainer is read-only intended on these files? If so, the fix belongs in createCaseClashConflict() after its rename, not in downloadFinished().
|
I checked Copilot suggestions, I think it might need some maintainer input though. |
|
Also I have checked the sync code paths and in theory all cases should handle an existing read-only temp file, so this bug should be "self healing" without any additional changes. |
setFileReadOnly() on Windows adds an ACCESS_DENIED_ACE for BUILTIN\Users
to the file's DACL. Applying this to the temp file before renaming it to
its final destination blocks the rename, because Windows requires Delete
access on the source path to complete the operation. This caused a
persistent Error 5 (Access Denied) retry loop for read-only shares and
Talk attachments on Windows.
Move the permission block to after uncheckedRenameReplace() succeeds,
targeting the final filename instead of _tmpFile.fileName().
Fixes: #9885