Fix federated link sharing permissions#20726
Merged
Conversation
skjnldsv
added
19-feedback
3. to review
npmbuildbot-nextcloud
bot
force-pushed
the
fix/sharing-link-default-permissions
branch
from
20b049e to
109f614
Compare
skjnldsv
force-pushed
the
fix/sharing-link-default-permissions
branch
from
109f614 to
d221e77
Compare
skjnldsv
requested review from
ChristophWurst,
MorrisJobke,
juliusknorr,
nickvergessen and
rullzer
Member
|
Needs a rebase and then time to get it in |
skjnldsv
force-pushed
the
fix/sharing-link-default-permissions
branch
from
d221e77 to
fd7b0b1
Compare
Member
Author
|
/compile amend / |
npmbuildbot-nextcloud
bot
force-pushed
the
fix/sharing-link-default-permissions
branch
from
fd7b0b1 to
6322ede
Compare
gary-kim
approved these changes
May 4, 2020
Member
|
Conflicts |
MorrisJobke
added
2. developing
skjnldsv
force-pushed
the
fix/sharing-link-default-permissions
branch
from
6322ede to
5d0ffce
Compare
skjnldsv
added
4. to release
Member
Author
|
/compile amend / |
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com> Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
npmbuildbot-nextcloud
bot
force-pushed
the
fix/sharing-link-default-permissions
branch
from
5d0ffce to
ff20da6
Compare
skjnldsv
added
2. developing
Member
Author
|
Lots of test sfailures 😞 |
Member
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 free
to 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