Uh oh!
There was an error while loading. Please reload this page.
Fix federated link sharing permissions - #20726
Merged
Merged
Conversation
npmbuildbot-nextcloudBotforce-pushed
the
fix/sharing-link-default-permissions
branch
from
April 29, 2020 14:54
20b049e to
109f614Compareskjnldsvforce-pushed
the
fix/sharing-link-default-permissions
branch
from
April 29, 2020 14:58
109f614 to
d221e77Compareskjnldsv
marked this pull request as ready for review
April 29, 2020 14:58
skjnldsv
requested review from
ChristophWurst, MorrisJobke, juliusknorr, nickvergessen and rullzerApril 29, 2020 14:58
rullzer
commented
May 2, 2020
Member
Needs a rebase and then time to get it in |
skjnldsvforce-pushed
the
fix/sharing-link-default-permissions
branch
from
May 2, 2020 10:13
d221e77 to
fd7b0b1Compareskjnldsv
commented
May 2, 2020
MemberAuthor
/compile amend / |
npmbuildbot-nextcloudBotforce-pushed
the
fix/sharing-link-default-permissions
branch
from
May 2, 2020 10:22
fd7b0b1 to
6322edeComparegary-kim
approved these changes
May 4, 2020
MorrisJobke
commented
May 4, 2020
Member
Conflicts |
skjnldsvforce-pushed
the
fix/sharing-link-default-permissions
branch
from
May 4, 2020 11:37
6322ede to
5d0ffceCompareskjnldsv
commented
May 4, 2020
MemberAuthor
/compile amend / |
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com> Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
npmbuildbot-nextcloudBotforce-pushed
the
fix/sharing-link-default-permissions
branch
from
May 4, 2020 11:48
5d0ffce to
ff20da6Compareskjnldsv
commented
May 4, 2020
MemberAuthor
Lots of test sfailures 😞 |
MorrisJobke
commented
May 5, 2020
Member
rullzer
commented
May 5, 2020
Member
mmm Did I hit merge on the wrong PR... guess so... |
rullzer
reviewed
May 6, 2020
| } | ||
| if ($share->getShareType() === Share::SHARE_TYPE_USER) { | ||
| // TODO: It might make sense to have a dedicated setting to allow/deny converting link shares into federated ones |
Member
There was a problem hiding this comment.
I remember the discussion here but I guess I only now see the full impact. The permission needs to be persisted in the DB else the share conversion still doesn't happen. Because other part of the NC code don't use this they uyse the share object.
So either we do not care at all. ANd just check where we need to. Or we need to also make sure it is correct in the db.
Member
There was a problem hiding this comment.
I guess this basically reverts what we tried to do in #19793
Let me prepare a fix
Merged
This was referenced May 26, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
So, it of course means we don't store the SHARE permissions for link shares. We only add it on api GET.
It was mostly because of this
server/apps/files_sharing/lib/Controller/ShareAPIController.php
Lines 1002 to 1005 in d221e77
i don't really know what we should do for this. So instead of changing 2 years old code that looks super speficic, I rathered make sure we keep it consistent. Maybe it's wrong, please suggest any better way 👋
In any case, this is a bit odd because when changint the link share to something like READ, we now see in the response that they have the SHARE permission, despite not really being asked for, but I guess it's fine as it behave like an enforced setting?
Reference: #19793 (comment)
It might fix#20692