Skip to content

Conversation

@nfebe
Copy link
Contributor

@nfebe nfebe commented Dec 2, 2024

The share API currently always adds read permissions sent in share request with the argument that all shares must have read permissions. That this not true as link and email shares allowed not to.

In addition to the above, there is a check that ensures any share which is not a link or email share must have read permissions. There is also protection for legacy integrations where if no permissions are sent at all default permissions are set.

So it does not make sense to make any sort of additions to the permissions that a client has sent, as the response would be different from what the client expects.

@nfebe nfebe marked this pull request as ready for review December 2, 2024 18:36
@nfebe nfebe requested review from CarlSchwan, artonge, come-nc and provokateurin and removed request for come-nc and provokateurin December 2, 2024 18:36
@nfebe nfebe requested a review from skjnldsv December 2, 2024 18:37
@nfebe nfebe force-pushed the fix/no-issue/show-file-drop-permissions-correctly branch 2 times, most recently from c401885 to a0b10ae Compare December 2, 2024 21:39
@nfebe
Copy link
Contributor Author

nfebe commented Dec 2, 2024

Tests related to allowPublicUploads are failing, however, this param to the share API is not used in any client within the server code base.

I am not sure if that parameter is useful in the context of the new sharing UI.

CC: @artonge

Comment on lines -733 to -735
if ($this->shareManager->outgoingServer2ServerSharesAllowed()) {
$permissions |= Constants::PERMISSION_SHARE;
}
Copy link
Member

Choose a reason for hiding this comment

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

This might break federation

The share API currently always adds read permissions sent in share request with the argument that
all shares must have read permissions. That this not true as link and email shares allowed not to.

In addition to the above, there is a check that ensures any share which is not a link or email share
must have read permissions. There is also protection for legacy integrations where if no permissions are sent
at all default permissions are set.

So it does not make sense to make any sort of additions to the permissions that a client has sent, as the
response would be different from what the client expects.

Signed-off-by: nfebe <[email protected]>
@nfebe nfebe force-pushed the fix/no-issue/show-file-drop-permissions-correctly branch from a0b10ae to 5ed3db9 Compare December 3, 2024 08:20
@skjnldsv skjnldsv changed the title fix(share_api): Respect requested permissions or error out [stable29] fix(share_api): Respect requested permissions or error out Dec 3, 2024
@skjnldsv
Copy link
Member

skjnldsv commented Dec 3, 2024

Why is this PR against stable 29 ?
Is this not needed for 30 and master?
If so, please start with master and work your way down :)

@nfebe
Copy link
Contributor Author

nfebe commented Dec 3, 2024

Why is this PR against stable 29 ?
Is this not needed for 30 and master?
If so, please start with master and work your way down :)

This is related to a bug that more visible on 29 with "File drop" when the client sends permissions: 4 it comes back as permissions: 17 (+ share permissions which is 16)

File requests on 30, sends 4 but the client would handle 20 as still a file request. That works, it continues to build on incorrect complexity?

@susnux susnux added this to the Nextcloud 29.0.14 milestone Mar 2, 2025
@Altahrim Altahrim mentioned this pull request Mar 18, 2025
28 tasks
@blizzz blizzz mentioned this pull request Mar 19, 2025
This was referenced Apr 3, 2025
@skjnldsv skjnldsv mentioned this pull request Apr 10, 2025
15 tasks
@skjnldsv
Copy link
Member

29 is now EOL

@skjnldsv skjnldsv closed this Apr 11, 2025
@github-project-automation github-project-automation bot moved this from 🏗️ In progress to ☑️ Done in 📁 Files team Apr 11, 2025
@susnux susnux deleted the fix/no-issue/show-file-drop-permissions-correctly branch April 23, 2025 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: ☑️ Done

Development

Successfully merging this pull request may close these issues.

5 participants