Skip to content

fix: allow renaming files with just update permissions#57374

Open
icewind1991 wants to merge 3 commits intomasterfrom
rename-dav-permissions
Open

fix: allow renaming files with just update permissions#57374
icewind1991 wants to merge 3 commits intomasterfrom
rename-dav-permissions

Conversation

@icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Jan 6, 2026 •
edited by susnux
Loading

Only moving to another folder needs the delete+create permissions, for moving in the same folder just update is enough.

TODO:

  • Update files webui permissions check

@icewind1991 icewind1991 added the 2. developing Work in progress label Jan 6, 2026
@icewind1991
Copy link
Member Author

icewind1991 commented Jan 6, 2026

/compile

@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 6, 2026
@icewind1991 icewind1991 added this to the Nextcloud 33 milestone Jan 6, 2026
@icewind1991 icewind1991 marked this pull request as ready for review January 6, 2026 16:37
@icewind1991 icewind1991 requested review from a team and skjnldsv as code owners January 6, 2026 16:37
@icewind1991 icewind1991 requested review from Altahrim, ArtificialOwl, leftybournes, provokateurin, susnux and szaimen and removed request for a team January 6, 2026 16:37
szaimen
szaimen approved these changes Jan 7, 2026
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@nextcloud-bot nextcloud-bot mentioned this pull request Jan 7, 2026
Merged
@icewind1991
Copy link
Member Author

icewind1991 commented Jan 7, 2026

(I'll rebase etc once reviewed)

@nextcloud-bot nextcloud-bot mentioned this pull request Jan 9, 2026
Merged
mgallien
mgallien approved these changes Jan 9, 2026
View reviewed changes
Copy link
Contributor

@mgallien mgallien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested with the desktop client and works fine

provokateurin
provokateurin previously requested changes Jan 9, 2026
View reviewed changes
@icewind1991 icewind1991 force-pushed the rename-dav-permissions branch from 0f7b6d9 to de3587d Compare January 9, 2026 20:30
@nextcloud-bot nextcloud-bot mentioned this pull request Jan 14, 2026
Merged
@mgallien
Copy link
Contributor

mgallien commented Jan 15, 2026

@icewind1991 will this change be backported to stable branches ?

@nextcloud-bot nextcloud-bot mentioned this pull request Jan 20, 2026
Merged
@icewind1991 icewind1991 force-pushed the rename-dav-permissions branch from de3587d to 8a6bf8a Compare January 20, 2026 17:50
@icewind1991 icewind1991 force-pushed the rename-dav-permissions branch from 8a6bf8a to cd2a74f Compare January 20, 2026 17:58
@icewind1991
Copy link
Member Author

icewind1991 commented Jan 20, 2026

/compile

susnux
susnux reviewed Jan 20, 2026
View reviewed changes
apps/dav/lib/Connector/Sabre/Node.php
return true;
}

// we allow renaming the file if either the file has update permissions
Copy link
Contributor

@susnux susnux Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But does this not just mean write permissions to the file?
Do we not need to check the containing folder for update permission?

Copy link
Member Author

@icewind1991 icewind1991 Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be the delete+create case I think

Copy link
Contributor

@susnux susnux Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I share a folder with a file and only allow read+write then recipient still can rename the file inside the folder?
Would this not be unexpected as renaming is destructive if you expect the file to have a specific name?

If that is expected behavior then all good!

@nextcloud-bot nextcloud-bot mentioned this pull request Jan 22, 2026
Merged
This was referenced Jan 29, 2026
Merged
Merged
This was referenced Feb 11, 2026
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 33, Nextcloud 34 Feb 16, 2026
mgallien

This comment was marked as outdated.

Sign in to view

mgallien
mgallien approved these changes Mar 16, 2026
View reviewed changes
Copy link
Contributor

@mgallien mgallien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong test with only partial patch applied
I can confirm that this is working fine when applied on 30.0.11

@icewind1991 icewind1991 force-pushed the rename-dav-permissions branch 4 times, most recently from 625e755 to bc56723 Compare March 16, 2026 16:44
@icewind1991
fix: allow renaming files with just update permissions
645ecc3 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
fix: update rename permissions check in files ui
ed3f042 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 force-pushed the rename-dav-permissions branch from 4118102 to ed3f042 Compare March 16, 2026 16:54
@icewind1991
Copy link
Member Author

icewind1991 commented Mar 16, 2026

/compile

susnux
susnux approved these changes Mar 16, 2026
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense but would it not make more sense to directly expose the permission in the backend?

apps/files/src/actions/renameAction.ts
Comment on lines +44 to +45
Boolean(node.permissions & Permission.DELETE)
&& Boolean(parentPermissions & Permission.CREATE)
Copy link
Contributor

@susnux susnux Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not better to move the logic to the backend?

server/lib/public/Files/DavUtil.php

Line 36 in c826288

public static function getDavPermissions(FileInfo $info): string {

@nextcloud-command
chore(assets): Recompile assets
e2fdd6c 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgallien mgallien mgallien approved these changes

@susnux susnux susnux approved these changes

@provokateurin provokateurin provokateurin approved these changes

@szaimen szaimen szaimen approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv is a code owner

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner automatically assigned from nextcloud/server-backend

@leftybournes leftybournes Awaiting requested review from leftybournes leftybournes is a code owner automatically assigned from nextcloud/server-backend

Assignees

No one assigned

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 34

Development

Successfully merging this pull request may close these issues.

[Bug]: Unable to rename files in shared folders without delete permission

7 participants

@icewind1991 @mgallien @susnux @provokateurin @szaimen @blizzz @nextcloud-command