Fix list of shares #1590
Merged
Fix list of shares #1590
Conversation
This file contains 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
ildyria
reviewed
Nov 15, 2022
| ->select(['id', 'username']) | ||
| ->where('id', '>', 0) | ||
| ->orderBy('username', 'ASC') | ||
| // Existing users with whom an album can be shared optionally |
There was a problem hiding this comment.
Is this filtering really useful ?
ildyria
approved these changes
Nov 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes the list of shares (from the left menu) for non-admin, authenticated users. As @kamil4 reported on Gitter at Nov 7th 2022 18:20 non-admin user receive a
403 Forbiddenresponse. This is a regression due to #1519.Before #1519 the request
Sharing::listhad unconditionally returned a list of all active shares for all users and all albums as well as a list of all existing albums and all users. The the frontend filtered the received list for the the album or user of interest. The frontend code was not only ugly as hell, but the whole approach had been inefficient. There is no reason to query and transmit everything only to throw way 97% on the frontend. Hence, #1519 introduced two query parametersalbumIDanduserIDto enable filtering on the backend.However, I went one step further and thought it would clever to forbid unrestricted queries
, but it did no occur to me that non-admin users can also open the page with all sharings from the left menu.
Share::listfor non-admin users. This was fine when one used the dialog from the toolbarI also discovered related, lurking bug. Do you remember that @kamil4 once said that he doesn't like the attribute name
user2on a request? Here, this actually gave leeway to a trap. The requestSharing::listis one of the rare instances with actually three users being involved: the authenticated user who makes the request, the user who owns the album and the user with whom the album is shared.user2was used for the last two, but surely they are different. I renamed them toownerandparticipant.