Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Narrow down ReadWrite folder permission to owner #6949

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

suiso67
Copy link
Contributor

@suiso67 suiso67 commented Jul 26, 2024

#6839 seem to add group_write and others_write along with owner_write permission when setting FileSystem::FolderPermissions::ReadWrite permission to a folder, which is addressed in #6863. Please let me know if I'm missing any context.

Resolves #6863

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

After further feedback it seems this code has deeper issues

We are investigating a broader rewrite of this right now

Thank you for your contribution, though -- we definitely have issues properly handling reassigning the correct permissions

@mgallien
Copy link
Collaborator

@claucambra I am not sure I would want to skip this change
It looks really weird that the desktop client is adding too much permissions
@suiso67 can you rebase ? I cannot do it missing write access to your clone

@claucambra
Copy link
Collaborator

@claucambra I am not sure I would want to skip this change

It looks really weird that the desktop client is adding too much permissions

@suiso67 can you rebase ? I cannot do it missing write access to your clone

Aren't these the same permissions we applied prior to the last patch, though? Those were causing even more issues

Signed-off-by: suiso67 <suiso67@macadamia.rocks>
@suiso67 suiso67 force-pushed the fix/excessive-folder-permission branch from 1b65073 to a270756 Compare August 21, 2024 13:12
@suiso67
Copy link
Contributor Author

suiso67 commented Aug 21, 2024

@claucambra I am not sure I would want to skip this change It looks really weird that the desktop client is adding too much permissions @suiso67 can you rebase ? I cannot do it missing write access to your clone

I did rebase and force-pushed this branch, but it seems like there're other issues which I don't have context of. Can you give me issue/pr links related to this?

@bjoernv
Copy link

bjoernv commented Aug 21, 2024

This PR does not seem to solve this use case: #6863 (comment)

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

@suiso67 Upon discussion and a rethink I think we can merge this in the mean time, as setting all perms is not a viable solution and we need a better way to handle setting/restoring perms

@claucambra
Copy link
Collaborator

@suiso67 could you rebase once more?

@mgallien
Copy link
Collaborator

mgallien commented Sep 9, 2024

/backport to stable-3.13

@mgallien
Copy link
Collaborator

mgallien commented Sep 9, 2024

/backport to stable-3.14

@mgallien mgallien merged commit 9fc15d4 into nextcloud:master Sep 9, 2024
9 of 13 checks passed
@suiso67
Copy link
Contributor Author

suiso67 commented Sep 10, 2024

@suiso67 could you rebase once more?

Sorry for being late, it has been merged already.

If you don't mind, I'd like to know if there're more context about related issue you're talking about. I came here completely blind and kind of just reverted the change, and I'd like to contribute to the real issue even thought I can't guarantee anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Folder permissions are changed during synchronization (3.13.1 & 3.13.2 & 3.13.3)
5 participants