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. |
|
It has been two weeks with no comment from anyone. This is ready for review, can anyone take a look at this please @mgallien |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
@Lenart12 I am sorry for the delay |
There was a problem hiding this comment.
@Lenart12 sorry again for the delay
since you are modifying syncing behavior, please add some automated tests changes to validate that
let us know if you have questions on that
automated tests changes are usually required for this part of the code base
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