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

fix: permission check in public share update #4622

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

micbar
Copy link
Member

@micbar micbar commented Apr 10, 2024

Description

We fixed the permission check for updating public shares. When updating the permissions of a public share while not providing a password, the check must be against the new permissions to take into account that users can opt out only for view permissions.

relates to https://github.com/owncloud/enterprise/issues/6558

@micbar micbar requested review from labkode, glpatcern and a team as code owners April 10, 2024 09:47
@micbar micbar requested review from kobergj and rhafer April 10, 2024 09:49
@micbar
Copy link
Member Author

micbar commented Apr 10, 2024

Found one more corner case 🙈

@micbar
Copy link
Member Author

micbar commented Apr 10, 2024

ready for review.

Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

LGTM

(it really sucks that the publicshareprovider doesn't have any unit tests)

@micbar
Copy link
Member Author

micbar commented Apr 10, 2024

(it really sucks that the publicshareprovider doesn't have any unit tests)

I am on it. We can also wait for them.

@micbar
Copy link
Member Author

micbar commented Apr 10, 2024

Hm, @butonic The publicsharemanager is currently not testable, it creates its own gatewaySelector. We would need to refactor that to make it testable.

@rhafer
Copy link
Contributor

rhafer commented Apr 10, 2024

I am on it. We can also wait for them.

Cool 🎉 . I am fine to merge this as is and have another PR for the unit test.

@micbar
Copy link
Member Author

micbar commented Apr 10, 2024

Cool 🎉 . I am fine to merge this as is and have another PR for the unit test.

Fine. I need to refactor the New() Method which creates a lot of noise. So i would merge that now here, backport to stable and then refactor it on edge.

@micbar micbar merged commit a6eb0f4 into cs3org:edge Apr 10, 2024
9 checks passed
@micbar micbar mentioned this pull request Jun 19, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants