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

Honor permissions of new link share via OCS API #35213

Merged
merged 1 commit into from
Dec 22, 2022
Merged

Honor permissions of new link share via OCS API #35213

merged 1 commit into from
Dec 22, 2022

Conversation

jplitza
Copy link
Contributor

@jplitza jplitza commented Nov 16, 2022

The API currently overrides the supplied permissions with "read only" when a file is shared via link. It allows to update the permissions later, however.

Fixes #32611

@szaimen szaimen added the 3. to review Waiting for reviews label Nov 16, 2022
@szaimen szaimen added this to the Nextcloud 26 milestone Nov 16, 2022
@szaimen szaimen requested review from juliushaertl, come-nc, nickvergessen, a team and ArtificialOwl and removed request for a team November 16, 2022 22:57
@juliushaertl
Copy link
Member

It would be still nice to preserve the default of read only permissions for the share links, maybe by moving that to the first condition in https://github.com/nextcloud/server/pull/35213/files#diff-dbbe017dd357504abc442a6f1d0305166520ebf80353f42814b3f879a3e241bcR481

What do you think?

@jplitza
Copy link
Contributor Author

jplitza commented Dec 6, 2022

+1 for keeping defaults - although it seems wrong to willfully ignore the setting. Changed nonetheless.

@jplitza
Copy link
Contributor Author

jplitza commented Dec 6, 2022

The 3 failing tests all explicitly sets PERMISSION_ALL but expects the share to only have read permissions. Fixed two of them to expect PERMISSION_ALL and one to not set permissions at all and expect READ|SHARE.

The API currently overrides the supplied permissions with "read only"
when a file is shared via link. It allows to update the permissions
later, however.

This keeps the default to "read only" but honors the permissions
supplied by API call if any.

Signed-off-by: Jan-Philipp Litza <jpl@plutex.de>
@jplitza
Copy link
Contributor Author

jplitza commented Dec 7, 2022

The amount of tests that cement the old behavior is staggering. After fixing 4 unit tests, now acceptance-app-files-sharing is failing in a way I don't quite understand yet. Apparently a readonly user (!) share can now be re-shared readwrite. I don't yet see any connection to my patch, but it's close enough to probably be caused by it.

@nickvergessen
Copy link
Member

The acceptance tests can be randomly failing from time to time.

@nickvergessen nickvergessen removed their request for review December 7, 2022 10:34
@juliushaertl
Copy link
Member

I've restarted the drone job to double check

@jplitza
Copy link
Contributor Author

jplitza commented Dec 7, 2022

Indeed, now the a acceptance-app-files-sharing tests all succeed (but acceptance-app-theming still fails, though probably unrelatedly, and samba now failed, which it previously didn't).

@juliushaertl
Copy link
Member

Yes, seems all unrelated.

@jplitza
Copy link
Contributor Author

jplitza commented Dec 20, 2022

Sooo... what is the process to get a second review on this?

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Looks good.

@skjnldsv you added a TODO about it in the front-end, but I don't understand what it is referring too. Should we do something there too ? As this PR seems to "fix the createShare method".

// we do not allow setting the publicUpload
// before the share creation.
// Todo: We also need to fix the createShare method in
// lib/Controller/ShareAPIController.php to allow file drop
// (currently not supported on create, only update)

@skjnldsv
Copy link
Member

@skjnldsv you added a TODO about it in the front-end, but I don't understand what it is referring too. Should we do something there too ? As this PR seems to "fix the createShare method".

Backend permissions are always tricky for sharing.
I haven't looked deep into that PR, but we need to make sure the front is compliant to this PR. I don't remember adding any check, but this is usually the thing to adjust and check

@jplitza
Copy link
Contributor Author

jplitza commented Dec 22, 2022

Without having looked into the frontend code, I don't see how this PR could pose a problem. It preserves the default, and the frontend always first creates the share link without specifying permissions, and only allows setting permissions later on.

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

👍

@juliushaertl juliushaertl added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 22, 2022
@juliushaertl
Copy link
Member

Failures unrelated

@juliushaertl juliushaertl merged commit 628b0d8 into nextcloud:master Dec 22, 2022
@juliushaertl
Copy link
Member

Thanks for the pull request @jplitza 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OCS Share API - "permissions" parameter has no effect
6 participants