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 create share API permissions for links #29143

Closed
wants to merge 2 commits into from

Conversation

julien-nc
Copy link
Member

When publicUpload is false:

  • default share link permissions (set in settings/admin/sharing)
  • and the permissions request parameter

are ignored. PERMISSION_READ is always set.

The createShare api method still has some permission issues with publicUpload (#17504) and it's still impossible to create a share without expiration date if a default one is defined (#10178). But these issues are a bit trickier to fix.

@julien-nc julien-nc added this to the Nextcloud 23 milestone Oct 8, 2021
@julien-nc julien-nc requested review from a team, nickvergessen, icewind1991 and come-nc and removed request for a team October 8, 2021 14:04
@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@skjnldsv
Copy link
Member

are ignored. PERMISSION_READ is always set.

I mean, that seems logical, no?
When nothing is provided, the minimum for a public share should be READ, no?

@julien-nc
Copy link
Member Author

@skjnldsv Hmmm the minimum yes, but there it's forced to PERMISSION_READ even if some permissions were provided via a request param. Even the default permissions are ignored.

@skjnldsv
Copy link
Member

Can you give more details about the issue you faced?
Public share links have certains preset for sharing, they cannot have random permissions, to me this is on purpose, not a mistake. Either it's a public upload, or just a read-only access 🤔

@skjnldsv skjnldsv modified the milestones: Nextcloud 23, Nextcloud 24 Oct 15, 2021
@julien-nc
Copy link
Member Author

@skjnldsv Sorry for the lack of details.

When creating a share link for a file (publicUpload is false) by calling /ocs/v2.php/apps/files_sharing/api/v1/shares (POST), even if you provide some permissions with the permissions param, they are ignored and PERMISSION_READ is set.

Afterwards, you can still edit the share link and set the permissions you want with /ocs/v2.php/apps/files_sharing/api/v1/shares/SHARE_ID (PUT) but it's not possible to directly set the permissions on creation.

For example, one cannot allow edition when creating a share link on a file. This can be useful when creating a share link for a markdown file.

@skjnldsv
Copy link
Member

Afterwards, you can still edit the share link and set the permissions you want with /ocs/v2.php/apps/files_sharing/api/v1/shares/SHARE_ID (PUT) but it's not possible to directly set the permissions on creation.

this is the issue then, it needs to be limited to the three options we support ?

@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
@PVince81 PVince81 requested a review from artonge April 5, 2022 12:13
This was referenced Apr 7, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@come-nc come-nc removed their request for review May 5, 2022 14:46
@julien-nc julien-nc force-pushed the fix/noid/created-file-share-permissions branch from 87219fe to e2a7e6b Compare July 26, 2022 10:03
@julien-nc julien-nc force-pushed the fix/noid/created-file-share-permissions branch from e2a7e6b to 0f87e0a Compare July 26, 2022 10:05
@julien-nc
Copy link
Member Author

julien-nc commented Jul 26, 2022

@skjnldsv Here is a cleaner approach:

When creating a public link (with publicUpload request param to false):

  • If there was no permissions request param: set permissions to READ
  • Else, use those requested permissions

it needs to be limited to the three options we support ?

I might be wrong but I think the limitation is already applied earlier in this method: https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/Controller/ShareAPIController.php#L504-L508

Signed-off-by: Julien Veyssier <[email protected]>
@blizzz blizzz mentioned this pull request Sep 9, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
@skjnldsv
Copy link
Member

skjnldsv commented May 9, 2023

What's the status? Is this still needed @julien-nc ?

@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv
Copy link
Member

no activity, closing

@skjnldsv skjnldsv closed this Feb 23, 2024
@skjnldsv skjnldsv removed the 3. to review Waiting for reviews label Feb 23, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Feb 23, 2024
@skjnldsv skjnldsv deleted the fix/noid/created-file-share-permissions branch March 14, 2024 07:47
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.

3 participants