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.

Sign up for GitHub

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

Jump to bottom

fix(files_sharing): Respect permissions passed when creating link shares #50270

Merged
merged 3 commits into from
Jan 28, 2025
Merged

fix(files_sharing): Respect permissions passed when creating link shares #50270

merged 3 commits into from
Jan 28, 2025

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 20, 2025 •
edited
Loading

Summary

Given:

  • User creates a link or email share with permissions=4 (create only = file drop).

Problem:

  • Currently the permissions are automatically extended to permissions = 5 (READ + CREATE).
  • Workaround was to create the share and directly update it.

Solution:

  • Respect what the user is requesting, create a file drop share.

Checklist

@susnux susnux added this to the Nextcloud 31 milestone Jan 20, 2025
@susnux
Copy link
Contributor Author

susnux commented Jan 20, 2025

/backport to stable30

@susnux susnux mentioned this pull request Jan 20, 2025
Closed
@Altahrim Altahrim mentioned this pull request Jan 21, 2025
Merged
@susnux susnux requested a review from come-nc January 21, 2025 08:26
@susnux susnux force-pushed the fix/share-api-create--permissions branch 7 times, most recently from 8476ff3 to 8283538 Compare January 22, 2025 18:35
@susnux susnux requested a review from come-nc January 22, 2025 18:35
@Altahrim Altahrim mentioned this pull request Jan 23, 2025
Merged
@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2025

/backport to stable31

@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2025

/backport to stable29

provokateurin
provokateurin approved these changes Jan 28, 2025
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

I found it a bit hard to follow all the changes in the first commit, maybe you can split such big refactorings into multiple atomic commits that are easier to review.
Other than that this LGTM, just one minor comment.

apps/files_sharing/lib/Controller/ShareAPIController.php
if ($permissions === null) {
if ($shareType === IShare::TYPE_LINK
|| $shareType === IShare::TYPE_EMAIL) {
assert($this->userId !== null);
Copy link
Member

Choose a reason for hiding this comment

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

Should we really use assertions?
In theory we know that the userid is not null, but we should still check.
Assertions can be disabled, so idk if it's the best to check this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we already do so would consider this a living standard. But yes could improve - but again this would mean our middleware is broken 🙈

@susnux susnux force-pushed the fix/share-api-create--permissions branch from b00f992 to 1b560ea Compare January 28, 2025 15:29
susnux and others added 3 commits January 28, 2025 16:40
@susnux @come-nc
fix(files_sharing): Respect permissions passed when creating link shares
253f434 
Given:
User creates a link or email share with permissions=4 (create only = file drop).

Problem:
Currently the permissions are automatically extended to permissions = 5
(READ + CREATE). Work around was to create the share and directly update
it.

Solution:
Respect what the user is requesting, create a file drop share.

Co-authored-by: Ferdinand Thiessen <[email protected]>
Co-authored-by: Côme Chilliet <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
fix(sharing): Move permission validation to share manager
0a0f562 
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
test: Reset sharing app config after test
0baab8f 
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/share-api-create--permissions branch from 1b560ea to 0baab8f Compare January 28, 2025 15:40
@susnux susnux merged commit e40b635 into master Jan 28, 2025
189 checks passed
@susnux susnux deleted the fix/share-api-create--permissions branch January 28, 2025 17:24
This was referenced Jan 28, 2025
Merged
Merged
Merged
@susnux
Copy link
Contributor Author

susnux commented Jan 31, 2025

/backport to stable28

@backportbot backportbot bot mentioned this pull request Jan 31, 2025
Merged
2 tasks
@susnux
Copy link
Contributor Author

susnux commented Jan 31, 2025

/backport to stable27

@susnux
Copy link
Contributor Author

susnux commented Jan 31, 2025

/backport to stable26

@backportbot backportbot bot mentioned this pull request Jan 31, 2025
Merged
2 tasks
@backportbot backportbot bot mentioned this pull request Jan 31, 2025
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@provokateurin provokateurin provokateurin approved these changes

@come-nc come-nc come-nc approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug feature: sharing
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.

Create Share API not honoring "permissions" parameter
3 participants
@susnux @provokateurin @come-nc