Problem/Motivation
From #2983179-16: [META] Implement stricter access checking for the media library:
Finally, the media library view has two displays (grid and table), and neither can really be used without a valid state object. Yet there is nothing enforcing that constraint -- if you have the "view media" permission, you can see the media library. So we want to deny access to all displays of the media library view (except the standalone ones at /admin/content/media) if there is no valid state object present.
Proposed resolution
Deny access to all widget displays of the media library view if there is no valid state object.
Remaining tasks
Write patch
Review
Commit
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#14 | 3038350-14.patch | 3.74 KB | seanB |
#10 | 3038350-10.patch | 4.38 KB | seanB |
#10 | 3038350-10-test-only.patch | 2.71 KB | seanB |
Comments
Comment #2
seanBComment #4
Wim LeersI don't understand the issue summary because I don't know enough about the Media Library implementation details — only today I found out about
\Drupal\media_library\MediaLibraryState
!Comment #5
Wim LeersHaving dug deeper, I think I understand it now. What this is saying, is that the Media Library selection dialog (
\Drupal\media_library\MediaLibraryUiBuilder::buildUi()
or/media-library
) should result in a 403 when it is opened without a validMediaLibraryState
.However, didn't #3038241: Implement a tamper-proof hash for the media library state already do this? It added
\Drupal\media_library\MediaLibraryUiBuilder::checkAccess()
with this code:AFAICT that already covers the scope of this issue?
Comment #6
seanBThe media library view contains 2 page displays that are supposed to be used only within the media library modal (URLs
admin/content/media-widget
andadmin/content/media-widget-table
)). This issue is about adding an extra check for a valid media library state when using the page display URLs of the views displays directly.The access to the
/media-library
is indeed already handled by the other issues.Comment #7
Wim LeersAh, I see, this is about adding similar access restrictions to not just the route (which already exists, done in #3038241: Implement a tamper-proof hash for the media library state), but also to the view?
Comment #8
seanBCorrect :)
Comment #9
Wim Leers👍— attempted to clarify the title for the next person looking at this issue :)
Comment #10
seanBHere is a first version of the patch. I incorporated the change in #3057370: MediaLibraryState::fromRequest() may result in invalid MediaLibraryState::create() call to prevent that from failing the new tests. We can remove it from this patch once the issue is committed.
Comment #11
Wim LeersWow, that's much simpler than I expected :D Assuming the test-only patch fails and the other one passes, this is definitely ready!
Comment #13
Wim LeersThat worked as expected! 👍 🚢
Comment #14
seanBRerolled now #3057370: MediaLibraryState::fromRequest() may result in invalid MediaLibraryState::create() call landed.
Comment #15
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCurrently in HEAD, if you go to
/admin/structure/views
, then for the Media Library view, in the Displays column, you see links for/admin/content/media-widget
and/admin/content/media-widget-table
. In HEAD, clicking on them causes an unhandled exception, coming fromMediaLibraryState::validateParameters()
. Because of this, when you edit the Media Library view, and one of those displays, the live preview at the bottom doesn't work.This patch replaces the unhandled exception with a 403, for all users, including user 1. That then continues to leave the live preview broken.
Is there anything we can/should do here to make the live preview work? And maybe also make the links from
/admin/structure/views
work? At least for some set of users (like ones with permission to edit the view)? I think that would make it easier for people to make changes to that View.I don't know if that's really in-scope for this issue, since the live preview is already broken in HEAD, so curious what others think about whether to hold this up on that.
Comment #16
phenaproximaI think we should fix that in another stable-blocking issue, which I will file.
Comment #17
phenaproximaOpened #3060603: Live preview is broken when editing the media_library view. I think I'm going to send this back to RTBC, since the bug is pre-existing in HEAD and this issue does not exacerbate it.
Comment #18
phenaproximaForgot to remove the tag.
Comment #19
alexpottNice bug find @effulgentsia. I agree with @phenaproxima that it is not in scope here and we now have the follow-up so that's good.
Crediting @Wim Leers and @effulgentsia for issue review.
Committed and pushed a3ce6e70ed to 8.8.x and 734b9190b3 to 8.7.x. Thanks!