Delegate media library access to the "thing" that opened the library

1. +++ b/core/modules/media_library/media_library.services.yml
   @@ -1,4 +1,9 @@
   +  media_library.widget_event_subscriber:
   
   +++ b/core/modules/media_library/src/Event/MediaLibraryCheckAccessEvent.php
   @@ -0,0 +1,90 @@
   +class MediaLibraryCheckAccessEvent extends Event {
   
   +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -0,0 +1,83 @@
   +    // If the user has access to the field we also allow access to the media
   +    // library.
   +    $event->setAccessResult($this->entityTypeManager
   +      ->getAccessControlHandler($entity_type)
   +      ->fieldAccess('edit', $field_definitions[$field_name], $event->getAccount(), NULL, TRUE));
   
   +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -175,8 +208,21 @@ public function checkAccess(AccountInterface $account = NULL) {
   +    $this->eventDispatcher->dispatch(MediaLibraryEvents::CHECK_ACCESS, $access_event);
   

   I was concerned how this would impact a future alternative media library UI that is decoupled. How would this work in that case?

   In the end, all of this infrastructure boils down to making it possible to ensure that the underlying media field is allowed to be edited.

   How could a decoupled implementation achieve the same?

2. +++ b/core/modules/media_library/src/Event/MediaLibraryCheckAccessEvent.php
   @@ -0,0 +1,90 @@
   + * Event fired when access is checked for the media library.
   

   Can't there be different operations on the media library?

   Opening?
   Selecting?
   Something else?

3. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -175,8 +208,21 @@ public function checkAccess(AccountInterface $account = NULL) {
   +    $access_event = new MediaLibraryCheckAccessEvent($account, $state, AccessResult::forbidden('Access not allowed for the provided opener ID.'));
   

   🤔 Can we change this to a more useful (less abstract) reason string?

4. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -175,8 +208,21 @@ public function checkAccess(AccountInterface $account = NULL) {
   +    // Merge the access result from the event.
   +    return $access_result->andIf($access_event->getAccessResult());
   

   🤔 It's not merging: we require access to be granted for both.

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -392,7 +400,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $field_widget_id = $field_id . static::$widgetIdSeparator . $id_suffix;
   +    $opener_id = static::$openerIdPrefix . $field_widget_id;
   

   ✔ Woah, that's a lot of "special snowflake string stuff". Not a problem, but surely tricky. This seems to be purely UI implementation details, so doesn't matter.

On point 5: it'd be helpful if both the event docs listed concrete use cases (@phenaproxima told me in chat: "fields AND CKEditor" are two concrete use cases.

Also: documentation in the media_library.api.php.

#7: thanks, that addresses all my concerns. Removed Needs documentation.

Also marking this as a task instead of a feature request, since it blocks CKEditor integration in Drupal 8.8.

Another round of review, getting very close to final review! Leaving marked as Needs review to encourage others to review it too.

1. +++ b/core/modules/media_library/media_library.services.yml
   @@ -1,4 +1,9 @@
   +      - { name: 'event_subscriber' }
   

   🤓 Übernit: the quotes are unnecessary!

2. +++ b/core/modules/media_library/src/Event/MediaLibraryCheckAccessEvent.php
   @@ -0,0 +1,95 @@
   +   * @param \Drupal\Core\Access\AccessResultInterface $access_result
   +   *   The access result.
   ...
   +  public function __construct(AccountInterface $account, MediaLibraryState $state, AccessResultInterface $access_result) {
   ...
   +    $this->accessResult = $access_result;
   

   This is the default access result…

3. +++ b/core/modules/media_library/src/Event/MediaLibraryCheckAccessEvent.php
   @@ -0,0 +1,95 @@
   +  public function setAccessResult(AccessResultInterface $access_result) {
   +    $this->accessResult = $access_result;
   +  }
   
   +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -0,0 +1,83 @@
   +    $event->setAccessResult($this->entityTypeManager
   

   … and this allows overwriting it.

4. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -0,0 +1,83 @@
   +    // If the user has access to the field we also allow access to the media
   

   s/has access to the/is allowed to edit the

5. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -189,8 +215,21 @@ public function checkAccess(AccountInterface $account = NULL, MediaLibraryState
   +    // library. By default, access is forbidden.
   +    $access_event = new MediaLibraryCheckAccessEvent($account, $state, AccessResult::forbidden('Access not allowed for the provided media library state parameters.'));
   

   We shouldn't forbid access by default. We should use AccessResult::neutral() by default.

   This then allows us to get rid of the overwriting, and instead use ->orIf(…).

   That's also how access checking for e.g. Entity/Field API works: default to "neutral" (= no access, but something else can choose to grant access"), rather than "forbidden" (= "no access, ever, because this is explicitly always forbidden").

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -745,7 +757,7 @@ protected static function setFieldState(array $element, FormStateInterface $form
   -   * Get the field ID of the widget from an opener ID.
   +   * Get the field ID from an opener ID.
       *
   
   @@ -756,8 +768,30 @@ protected static function setFieldState(array $element, FormStateInterface $form
   +   * Get the widget ID from an opener ID.
   

   🤓 Übernit: third person singular.

7. +++ b/core/modules/media_library/tests/modules/media_library_test/src/EventSubscriber/MediaLibraryTestEventSubscriber.php
   @@ -0,0 +1,36 @@
   + * Event subscriber for media library field widgets.
   ...
   +class MediaLibraryTestEventSubscriber implements EventSubscriberInterface {
   

   Inaccurate name.

I think my overarching architectural concern right now is that MediaLibraryUiBuilder has too many responsibilities. I think we should probably abstract out the access checking into its own dedicated route-level access checker class. This will give us more flexibility in the future to determine context-specific access to the media library.

So, in my ideal world, what we'd do is something like this in media_library.routing.yml:

media_library.ui:
  path: '/media-library'
  defaults:
    _controller: 'media_library.ui_builder:buildUi'
  requirements:
    _permission: 'view media'
    _media_library_ui: 'TRUE'

The _permission access check verifies user permissions, and then _media_library_ui is responsible for verifying the media library state parameters, then delegating the access check to the event subscribers. MediaLibraryUiBuilder is then free to focus on doing one thing: namely, building the renderable UI.

The other benefit here is that, in #3038350: Deny access to all media library View Displays if there is no valid state object, we'll be able to more easily modify the routes for the media library widget views; all we need to do is add a _media_library_ui requirement to them, and access checking is dealt with by the routing system.

I'm willing to implement this, but would like a second opinion before I proceed.

After tinkering more with this, I'm thinking that the whole "move the access check into a service" might be better done in #3038350: Deny access to all media library View Displays if there is no valid state object. It's not really in scope here, and that issue will benefit much more from such refactoring, since it means we will be able to add a simple _media_library_state: 'TRUE' requirement to Views routes, rather than having to do bizarre jiu-jitsu around the routing system.

So for now, I guess we can just get this delegation feature done, and then tweak the architecture in the next issue.

OK, I addressed part of Wim's feedback in #10. Namely: the access event now sets AccessResult::neutral() by default, and expects subscribers to call andIf() and orIf() on it, then setAccessResult().

From Slack:

wimleers (he/him) [9:41 PM]
I was thinking about the CKE + Media Library problem space some more during my evening run.
- CKEditor is one of many possible text editors.
- Text editors are used where text formats are used.
- Text formats can only be used in `@FieldType=text` and `@FieldType=text_long`.
- Therefore CKEditor can only be used in the context of a particular field.
- So … isn’t `\Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget::getOpenerFieldId()` already sufficient? In fact, this seems to say we don’t even necessarily need an “opener ID” concept that is independent from fields, although that surely is prudent and forward-looking.

So … what am I missing here?
If I’m not missing something:
```quickedit.field_form:
  path: '/quickedit/form/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
  defaults:
    _controller: '\Drupal\quickedit\QuickEditController::fieldForm'
  options:
    parameters:
      entity:
        type: entity:{entity_type}
  requirements:
    _permission: 'access in-place editing'
    _access_quickedit_entity_field: 'TRUE'```
This is super close to what we want/need:
- a route to access the media library, that receives parameters describing which entity type and field the media library is being opened for
- a high-level permission
- plus entity field access checking: `\Drupal\quickedit\Access\QuickEditEntityFieldAccessCheck::access()`
… which then gets awfully close to what @phenaproxima (he/him) wrote in https://www.drupal.org/project/drupal/issues/3038254#comment-13040807

seanb [9:45 PM]
Text fields are not necessarily tied to entities, and can also be used in custom form if I’m not mistaken.
The beauty of the opener ID and the events is that the media library no longer “cares” about that stuff
We could decide that we don’t want to make this an API and “hardcode” the entity reference and CKEditor handling into the media library.

wimleers (he/him) [9:50 PM]
> Text fields are not necessarily tied to entities, and can also be used in custom form if I’m not mistaken.
Ahhh yes, `\Drupal\filter\Element\TextFormat` == `@RenderElement("text_format")`, ofc

seanb [9:50 PM]
But I personally like the idea that the media library does not have a strong opinion on how it’s used.

wimleers (he/him) [9:50 PM]
no of course, I agree with that intent
But I still had an obligation to question whether we truly needed _another_ abstraction :slightly_smiling_face:
Looks like we do.

seanb [9:51 PM]
Specially since anyone can create a new field type.
Who knows what awesome things people will come up with.

wimleers (he/him) [9:52 PM]
So the potential _openers_ are:
- Media field
- CKEditor on a `#type=text_format` on a `@FieldType=text_*` entity field
- CKEditor on a `#type=text_format` _without_ an entity field
(in core)
(contrib/custom can do w/e)

seanb [9:52 PM]
Yes

wimleers (he/him) [9:52 PM]
:thumbsup:

#12 + #13: interesting connection to #3038350: Deny access to all media library View Displays if there is no valid state object, I was completely unaware that those would need the same access checking. That is the case right? To properly check access to those, it needs the exact same inputs to decide whether access should be allowed or not? I know that you wrote in #13 that you think it's better left to #3038350, but it's still valuable to know whether I correctly understand the next steps you describe. :)

1. +++ b/core/modules/media_library/src/Event/MediaLibraryCheckAccessEvent.php
   @@ -0,0 +1,94 @@
   + * This event allows modules to control access to the media library based on
   + * the media library state and the user account. Example use cases that
   + * require different types of access checks could be the use of the media
   + * library in an entity reference field widget or the use of the library in a
   + * CKEditor field.
   + */
   

   ✅ Based on our chat in Slack, I think this can be refined. Done.

2. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -0,0 +1,85 @@
   +    // If the user is allowed to edit the field, we also allow access to the
   +    // media library.
   +    $field_access = $this->entityTypeManager
   

   👍 Per my explanation, this would also determine access in case of using CKEditor on a body field: we'd then check edit access on the body field of the given entity.

3. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -0,0 +1,85 @@
   +      ->fieldAccess('edit', $field_definitions[$field_name], $event->getAccount(), NULL, TRUE);
   

   🤔 The 4th parameter here is NULL. That is FieldItemListInterface $items = NULL, for which \Drupal\Core\Entity\EntityAccessControlHandlerInterface::fieldAccess()'s docs say:

      * @param \Drupal\Core\Field\FieldItemListInterface $items
      *   (optional) The field values for which to check access, or NULL if access
      *    is checked for the field definition, without any specific value
      *    available. Defaults to NULL.
   

   So this is not wrong. But it does mean that some field access checks will deny access. For example, \Drupal\user\UserAccessControlHandler::checkFieldAccess() allows editing certain fields if it's the owner making the change. Due to this NULL, it's impossible to determine which entity's field is being edited. So it's feasible to run into a scenario where I'm the owner and on this community site I want am able to pick one of the fifty existing forum post banners (this used to be a thing 🤓) from the media library. The code concludes that it's not the owner editing this field, and denies access. Even though I am the owner.

   I am fine with marking this out of scope and delegating it to a follow-up, because supporting this also requires modifying the opener ID.

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -397,9 +407,11 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -    // The opener ID is used by the select form and the upload form to add the
   -    // selected/uploaded media items to the widget.
   -    $opener_id = static::$openerIdPrefix . $field_name . $id_suffix;
   +    // Create a unique opener ID with a fixed prefix for entity reference field
   +    // widgets. The opener ID could be used to check access to the media
   +    // library, and used by the media library select/add forms to add the
   +    // selected/added media items to the widget.
   +    $opener_id = static::$openerIdPrefix . $field_widget_id;
   

   🤔 Is this a pure refactor or is this changing the opener ID?

5. Fixed nits.

As the patch now clearly documents, this blocks #2801307: [META] Support WYSIWYG embedding of media entities.

1. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -59,17 +59,31 @@ public function onMediaLibraryCheckAccess(MediaLibraryCheckAccessEvent $event) {
   -    list($entity_type, $bundle, $field_name) = explode('.', $field_id);
   ...
   +    list($entity_type_id, $bundle, $field_name) = explode('.', $field_id);
   

   Good rename 👍

2. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -59,17 +59,31 @@ public function onMediaLibraryCheckAccess(MediaLibraryCheckAccessEvent $event) {
   +    // When we don't have an ID for the parent entity, create a new entity.
   +    else {
   +      $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
   +      $default_values = [];
   +      if ($entity_type->hasKey('bundle')) {
   +        $default_values[$entity_type->getKey('bundle')] = $bundle;
   +      }
   +      $entity = $this->entityTypeManager->getStorage($entity_type_id)->create($default_values);
   +    }
   ...
   +      ->fieldAccess('edit', $field_definitions[$field_name], $event->getAccount(), $entity->get($field_name), TRUE);
   

   👍 🤔 In this case, $entity->get($field_name) won't be set to the particular value that it currently has, which the field access check logic might use. Oh wait, if there's no entity yet, it must be a new one, so it can't have a current value. Oops! :D

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -279,7 +310,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $entity_id = $items->getEntity()->id() ?: 0;
   

   👎 We can't assume numerical IDs.

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -440,6 +472,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#access' => $this->uiBuilder->checkAccess($this->currentUser, $state),
   

   👍 Woah! That's a big change. AFAICT this is not essential, although it results a better UX: rather than having a "Add media" button that doesn't do anything, this ensures the "Add media" button just doesn't appear.

   Ideally, we'd have explicit test coverage for this, but I don't think that's a blocker for this; this is a pure UX improvement.

If it weren't for point 3, this would be RTBC!

1. +++ b/core/modules/media_library/src/Event/MediaLibraryEvents.php
   @@ -0,0 +1,30 @@
   +  const CHECK_ACCESS = 'media_library.check_access';
   

   I'm wondering if this should be CHECK_ACCESS, or just ACCESS. The latter is less verbose, but probably just as clear from a DX standpoint.

2. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -0,0 +1,99 @@
   +/**
   + * Event subscriber for media library field widgets.
   + */
   +class MediaLibraryWidgetEventSubscriber implements EventSubscriberInterface {
   

   I think we should rename this class to MediaLibraryAccessSubscriber, and have it only deal with access-related things (field widget and otherwise). "WidgetEventSubscriber" is both too long and too vague for my taste.

   Also, I'm not sure we need to put this in an EventSubscriber sub-namespace. Do other core modules do that? If so, we can leave it; otherwise, let's follow their lead.

3. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -0,0 +1,99 @@
   +  public function onMediaLibraryCheckAccess(MediaLibraryCheckAccessEvent $event) {
   

   Can this be renamed to 'checkAccess'? 'onMediaLibrary' seems too verbose.

4. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -0,0 +1,99 @@
   +    if ($parent_entity_id = MediaLibraryWidget::getOpenerParentEntityId($state->getOpenerId())) {
   

   I'm not sure we should add a new public static method to MediaLibraryWidget just for this event subscriber to use. For now, let's make it a protected method of the event subscriber.

5. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetEventSubscriber.php
   @@ -0,0 +1,99 @@
   +      $entity = $this->entityTypeManager->getStorage($entity_type_id)->load($parent_entity_id);
   +    }
   +    // When we don't have an ID for the parent entity, create a new entity.
   +    else {
   +      $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
   +      $default_values = [];
   +      if ($entity_type->hasKey('bundle')) {
   +        $default_values[$entity_type->getKey('bundle')] = $bundle;
   +      }
   +      $entity = $this->entityTypeManager->getStorage($entity_type_id)->create($default_values);
   +    }
   

   In fact, let's move all of this to a protected method of the event subscriber. Something like getFieldItems() or similar.

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -247,7 +247,7 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   -    $parents = $form['#parents'];
   +    $parents = isset($form['#parents']) ? $form['#parents'] : [];
   

   Why did this change?

7. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -53,6 +56,20 @@ class MediaLibraryUiBuilder {
   +  /**
   +   * The current user.
   +   *
   +   * @var \Drupal\Core\Session\AccountInterface
   +   */
   +  protected $currentUser;
   +
   +  /**
   +   * The event dispatcher.
   +   *
   +   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
   +   */
   +  protected $eventDispatcher;
   +
   

   I'm hoping we will eventually move this stuff into its own access checker class, which means these will be moved out of here. However, that won't break BC, because we can _remove_ parameters from a constructor with no ill effects. No need to change anything, just pointing it out.

8. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -189,8 +215,20 @@ public function checkAccess(AccountInterface $account = NULL, MediaLibraryState
   +    $access_result = AccessResult::allowedIfHasPermission($account, 'view media')
   

   This really should be moved to the route requirements, i.e., _permission: 'view media'. However, that's out of scope here and there's no need to change it in this patch.

9. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -55,6 +70,13 @@ class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInt
   +  /**
   +   * The separator to use for widget IDs separating the field ID and the suffix.
   +   *
   +   * @var string
   +   */
   +  protected static $widgetIdSeparator = ':';
   

   I was going to suggest that this be a constant, but then I saw that we have a similar thing already in this class. I don't like that either, but eh...at least it's protected.

10. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -756,8 +801,52 @@ protected static function setFieldState(array $element, FormStateInterface $form
    +  /**
    +   * Gets the parent entity ID from an opener ID.
    +   *
    +   * @param string $opener_id
    +   *   The opener ID of the media library.
    +   *
    +   * @return string|null
    +   *   The parent entity ID or NULL if the parent entity is new.
    +   *
    +   * @see \Drupal\media_library\MediaLibraryState
    +   */
    +  public static function getOpenerParentEntityId($opener_id) {
    

    We really need a @see here to show how the entity ID is added to the opener ID.

11. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -756,8 +801,52 @@ protected static function setFieldState(array $element, FormStateInterface $form
    +    list($field_id, $entity_id) = explode(static::$widgetIdSeparator, $widget_id);
    

    This could be list(, $entity_id), since we never actually use $field_id.

12. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -756,8 +801,52 @@ protected static function setFieldState(array $element, FormStateInterface $form
         // Media library widget opener IDs are always prefixed with 'field:' in .
    -    if (preg_match('/^' . static::$openerIdPrefix . '([a-z0-9_-]+)$/', $opener_id, $matches)) {
    +    if (preg_match('/^' . static::$openerIdPrefix . '([a-z0-9\.:_-]+)$/', $opener_id, $matches)) {
    

    I think the inline comment could use an update to account for what this regex is now doing. It should mention what kind of things expects to find after the 'field:' prefix.

13. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -88,43 +89,77 @@ public function testMediaLibraryAccess() {
    +   * @param array $reason
    +   *   (optional) The expected reason for the access result.
    

    $reason is a string.

14. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -88,43 +89,77 @@ public function testMediaLibraryAccess() {
    +  protected function assertMediaLibraryAccess(AccountInterface $account, MediaLibraryState $state, $allowed, $cache_tags, $cache_contexts, $reason = NULL) {
    

    $cache_tags and $cache_contexts should be type hinted as arrays.

Partial review. This is shaping up really clearly. I'd like a framework manager to look at a particular point, but I'm running out of things to complain about.

1. +++ b/core/modules/media_library/src/Event/MediaLibraryAccessEvent.php
   @@ -0,0 +1,98 @@
   +   * @param \Drupal\media_library\MediaLibraryState $state
   +   *   The current state of the media library, derived from the current request.
   

   I think we should rephrase the description of this parameter, since it's not certain that the state of the media library will be derived from the request. Perhaps "The current state of the media library, usually derived from the current request."

2. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -0,0 +1,124 @@
   +   * @throws \Exception
   +   */
   +  public function onMediaLibraryAccess(MediaLibraryAccessEvent $event) {
   

   This method doesn't throw an exception, so it should not have a @throws annotation.

3. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -0,0 +1,124 @@
   +    $field_name = MediaLibraryWidget::getOpenerFieldName($state->getOpenerId());
   

   I think we should rename this to getFieldNameFromOpenerId(). That is more verbose, but it's also a lot more clear as to what it does. Given that the whole idea of an "opener ID" is somewhat obscure anyway, anything we can do to make this clearer is a good thing.

4. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -0,0 +1,124 @@
   +    $entity_type = $state->getEntityType();
   

   I know we discussed this, but I'm still wondering if it's necessary to have entity-specific stuff on MediaLibraryState. It feels like the wrong place for it. Could we maybe put that on the event class? Perhaps we could subclass MediaLibraryAccessEvent with some entity-specific methods (EntityAwareMediaLibraryAccessEvent or something), and use those?

   Although, that would require additional query string hashing and validation, which is inconvenient and verbose (and potentially impossible...although we could potentially confine it to this class, which might be the most appropriate course of action).

   Sorry for thinking out loud there...I think I need to defer to a framework manager on this one.

5. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -0,0 +1,124 @@
   +    if ($this->entityTypeManager->getDefinition($entity_type) instanceof ContentEntityInterface) {
   

   This is the wrong type check. The entity type would be an instance of ContentEntityTypeInterface, not ContentEntityInterface. The fact that tests did not catch this means that we probably need a unit or kernel test of it. :)

6. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -0,0 +1,124 @@
   +    else {
   +      $entity_type = $this->entityTypeManager->getDefinition($state->getEntityType());
   +      $default_values = [];
   +      if ($entity_type->hasKey('bundle')) {
   +        $default_values[$entity_type->getKey('bundle')] = $state->getEntityBundle();
   +      }
   +      $entity = $this->entityTypeManager->getStorage($entity_type)->create($default_values);
   

   I think core has a "sample entity generator" mechanism now, which we should probably use instead. :)

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -726,7 +726,7 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
   +    if ($widget_id = MediaLibraryWidget::getOpenerWidgetId($opener_id)) {
   

   Likewise, I think we should rename this getFieldWidgetIdFromOpenerId(), so that it's clearer. We should probably also add a @see so that one can easily look up what a "widget ID" is in this context.

8. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -98,6 +99,10 @@ public static function fromRequest(Request $request) {
   +    // Restore the parameters from the request since there might be additional
   +    // values.
   +    $state->replace($query->all());
   

   Why was this moved? If it needs to come before the validation, we should probably expand the comment to explain why.

9. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -190,6 +195,34 @@ public function isValidHash($hash) {
   +    $this->set('media_library_entity_id', $entity->id() ?: 0);
   

   I don't think we can default the entity ID to 0. For all we know, that could be a valid ID for that entity type. This is also assuming that the entity has a numeric ID. We should default it to an empty string, I think, since that is what NULL would become if parsed from a query string.

10. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -244,4 +277,37 @@ public function getAvailableSlots() {
    +   * @return string|int|null
    

    Coming from a parsed query string, the return value is always going to be a string.

11. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -244,4 +277,37 @@ public function getAvailableSlots() {
    +    return $this->get('media_library_entity_id') ?: NULL;
    

    We don't really need the ?: NULL here.

12. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -220,15 +259,25 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
    +    $link_state = clone $state;
    +    $link_state->remove(MainContentViewSubscriber::WRAPPER_FORMAT);
    +    $link_state->remove(FormBuilderInterface::AJAX_FORM_REQUEST);
    +    $link_state->remove('_media_library_form_rebuild');
    

    Rather than do this here, let's implement __clone() on MediaLibraryState, so that these parameters are always removed. :) (And let's add a unit test to confirm that this actually happens as expected.)

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -70,10 +92,24 @@ class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInt
   +    if (!$ui_builder) {
   +      @trigger_error('The media_library.ui_builder service must be passed to MediaLibraryWidget::__construct(), it is required before Drupal 9.0.0.', E_USER_DEPRECATED);
   +      $ui_builder = \Drupal::service('media_library.ui_builder');
   +    }
   +    $this->uiBuilder = $ui_builder;
   +    if (!$current_user) {
   +      @trigger_error('The current_user service must be passed to MediaLibraryWidget::__construct(), it is required before Drupal 9.0.0.', E_USER_DEPRECATED);
   +      $current_user = \Drupal::currentUser();
   +    }
   +    $this->currentUser = $current_user;
   

   We might need a change record for these deprecations.

   Also, let's make the $ui_builder the final argument. I say this because in the future we might want to refactor the access-checking to work another way, rather than directly calling methods of the UI builder -- in which case we'll likely remove the argument. If that comes to pass, having it as the final argument will make the deprecation easier.

2. +++ b/core/modules/media_library/tests/modules/media_library_test/media_library_test.module
   @@ -0,0 +1,23 @@
   +    return AccessResult::forbiddenIf(!$account->hasPermission('access administration pages'));
   

   Can we use AccessResult::allowedIfHasPermission() here, for readability?

3. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
   @@ -88,43 +89,77 @@ public function testMediaLibraryAccess() {
   +  /**
   +   * Checks the media library access for the test opener.
   +   *
   +   * @param \Drupal\media_library\Event\MediaLibraryAccessEvent $event
   +   *   The media library check access event.
   +   */
   +  public function onMediaLibraryAccess(MediaLibraryAccessEvent $event) {
   +    // Always allow access for the test opener.
   +    if ($event->getState()->getOpenerId() === 'test') {
   +      $event->setAccessResult(AccessResult::allowed());
   +    }
   

   Why is this method in the test class? Doesn't seem to be used anywhere...?

Did a thorough review. Every single time I thought something looked questionable, I searched the issue for it and found @phenaproxima asking the exact question I wanted to ask! (And a few times I did so for things I already asked about earlier and there was a good reason to do it the current way.)

Thanks to all prior reviews — those by me, but especially those by @phenaproxima — this review is at least three five times shorter than it otherwise would've been :D

1. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -190,6 +198,34 @@ public function isValidHash($hash) {
   +  public function setSelectedTypeId($selected_type_id) {
   ...
   +  public function setEntity(EntityInterface $entity) {
   

   These are the only two setters, the only two mutators on this value object. I was tempted it to withSelectedTypeId(…) and have it return a new MediaLibraryState. But then I realized that MediaLibraryState extends ParameterBag, which means all data is overridable anyway. So that's pointless.

   Keeping this exactly the same, but moved setSelectedTypeId() after getSelectedTypeId() and fixed a comment nit.

2. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -220,15 +259,20 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +      // Set the selected type ID in the link state to get a new valid hash.
   +      $link_state->setSelectedTypeId($allowed_type_id);
   

   This comment is explaining implementation details of the method; renaming the method instead for clarity.

3. A few comments left me wondering what was actually happening, so I edited those for clarity. Since I didn't author the patch, I don't know enough about the internals and nuances, so hopefully this makes it more understandable to other non-experts in Media Library implementation details :)

   Also fixed a bunch of übernits.

Evidently very little to complain about. I think this is ready :) Great work!

1. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -0,0 +1,123 @@
   +    if ($entity_id = $state->getEntityId()) {
   +      $entity = $this->entityTypeManager->getStorage($state->getEntityType())->load($entity_id);
   

   should this handle things like forward revisions and translations?

   we have an api for that since #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing

2. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -0,0 +1,123 @@
   +      $entity = $this->entityTypeManager->getStorage($entity_type_id)->create($default_values);
   

   should we use \Drupal\Core\Entity\ContentEntityStorageInterface::createWithSampleValues here?

3. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -220,6 +243,19 @@ public function getSelectedTypeId() {
   +  public function setSelectedTypeIdAndRehash($selected_type_id) {
   
   +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -220,15 +260,19 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +      $link_state->setSelectedTypeIdAndRehash($allowed_type_id);
   

   are we sure we want to mutate state on the object here - i.e should this return a new instance with the new type and the new hash?

   these kind of mutations can cause weird edge cases and can be hard to track down

4. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -244,4 +280,48 @@ public function getAvailableSlots() {
   +   * Magic method: Remove properties from the state when it is cloned.
   

   we could just use inheritdoc here?

5. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -189,8 +216,21 @@ public function checkAccess(AccountInterface $account = NULL, MediaLibraryState
   +    // Dispatch an event to allow openers to grant access to the media library.
   +    $access_event = new MediaLibraryAccessEvent($account, $state);
   +    $this->eventDispatcher->dispatch(MediaLibraryEvents::ACCESS, $access_event);
   +
   +    // Both media's own access checking and the media library access event must
   +    // allow access.
   +    return $access_result->andIf($access_event->getAccessResult());
   

   So because this doesn't use the normal entity access api, there is no other extension point (such as hook_entity_access{_alter}) ?

+++ b/core/modules/media_library/src/MediaLibraryState.php
@@ -190,6 +198,21 @@ public function isValidHash($hash) {
+    $this->set('media_library_entity_id', $entity->id() ?: '');
+    $this->set('media_library_entity_type', $entity->getEntityTypeId());
+    $this->set('media_library_entity_bundle', $entity->bundle());

I just noticed the class-level docblock hasn't been updated yet to document these.

It's not just the methods though. It's also the entity-related state parameters, and adding them to the hash validation.

What I think might be cleaner is adding a MediaLibraryOpener class, and having that be the only new thing added to MediaLibraryState. Then, we can have different subclasses of MediaLibraryOpener, so for example, we could have a EntityFieldWidgetMediaLibraryOpener, and that could have methods on it related to the widget's host entity and field.

It might take more time to flesh this out though, and there's DrupalDevDays coming up next week, so if there are other things being worked on that are currently blocked on some of what's in this patch that's unrelated to any of the MediaLibraryState changes, it would be great to unblock that while we figure out the MediaLibraryState changes in parallel. For example, it doesn't look to me like #3044649: Delegate media library selection handling to the "thing" that opened the library needs any changes to MediaLibraryState. Is that correct? If so, then could the other parts of this patch that are needed by that one be moved into that one?

Here's a partial, no-tests-yet rewrite of this patch on top of the new architecture we're intending to land in #3044649: Delegate media library selection handling to the "thing" that opened the library.

We found a way to get this to work which involves no nasty code smells or entity-specific changes to MediaLibraryState. Thanks to this extremely pleasant turn of events, this is far slimmer and does not specifically need framework manager review. So I'm removing the tag.

This new architecture doesn't involve events (or even tagged services!), but it's quite different from our previous work in here and therefore necessitates rethinking the tests. I'll get on that next.

This patch combines the access stuff with the work we're doing in #3044649: Delegate media library selection handling to the "thing" that opened the library, just to make sure we're not introducing any problems once we land that one.

This one changes MediaLibraryUiBuilder so that it actually delegates the access check to the opener service. Let's see how testbot likes it.

Another pass, after some changes in the other issue. Including the combined patch and the "real" patch for comparison.

Forgot an (important) thing in the previous patch. :)

Dang. Killed the previous test, because I forgot to update the media_library.ui_builder service definition...let's see what this one does.

OK. Now that #3044649: Delegate media library selection handling to the "thing" that opened the library is in, here's a new patch which fixes most of the tests on my local machine.

Tagging for backport to 8.7.x. Access hardening is, in general, a Good Thing, and since #3044649: Delegate media library selection handling to the "thing" that opened the library was backported as well, it makes sense to have this in 8.7.x too, since it makes use of those new internals.

Fixed the test.

1. +++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
   @@ -11,11 +15,80 @@
   +      return AccessResult::forbidden("The media library can only be opened by fieldable entities.");
   

   This error message doesn't really make sense, since on lines 52–57 we already are requiring field_name anyway.

2. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -189,8 +219,20 @@ public function checkAccess(AccountInterface $account = NULL, MediaLibraryState
   +    $view_access = AccessResult::allowedIfHasPermission($account, 'view media')
   

   $view_access is confusing in this context, because there's the view media permission that is being checked, but there's also $view which is a "View".

   I suggest $can_view_media.

3. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -189,8 +219,20 @@ public function checkAccess(AccountInterface $account = NULL, MediaLibraryState
   +    // If the user can view media, delegate any further access checking to the
   +    // opener service nominated by the media library state.
   +    if ($view_access->isAllowed()) {
   +      return $this->openerResolver->get($state)
   +        ->checkAccess($state, $account)
   +        ->andIf($view_access);
   +    }
   +    else {
   +      return $view_access;
   +    }
   

   Nit: flipping the order would make this slightly easier to read:

   // Return early if the user is not even allowed to view media.
   if (!$view_access->isAllowed() {
     return $view_access;
   }
   
   // Otherwise, also check access for the media library opener's specific circumstances.
   $can_open_media_library = $this->openerResolver->get($state)
     ->checkAccess(…);
   return $view_access->andIf($can_open_media_library);
   

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -461,9 +461,17 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      // The entity ID needs to be a string to ensure that the media library
   +      // state generates its tamper-proof hash in a consistent way.
   +      'entity_id' => (string) $entity->id(),
   

   Ugh, nothing we can do about this, but so annoying to once again have the DB layer be loading ints as strings.

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -286,7 +286,13 @@ public function testWidgetAccess() {
   +    // The opener parameters are not relevant to the test, but the opener
   +    // expects them to be there or it will deny access.
   +    $state = MediaLibraryState::create('media_library.opener.field_widget', $allowed_types, 'type_three', 2, [
   +      'entity_type_id' => 'media',
   +      'bundle' => 'type_one',
   +      'field_name' => 'field_media_test',
   +    ]);
   
   +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
   @@ -78,14 +81,22 @@ public function testMediaLibraryAccess() {
   +    // Create a media library state to test access. The opener parameters are
   +    // not relevant in this test, but the opener expects them to be there and
   +    // will deny access if they aren't.
   +    $state = MediaLibraryState::create('media_library.opener.field_widget', [$media_type], $media_type, 2, [
   +      'entity_type_id' => 'media',
   +      'bundle' => $media_type,
   +      'field_name' => 'field_media_test',
   +    ]);
   

   Why isn't entity_id also expected?

   Ah … because entity_id isn't required: it is only used when editing an entity whose media field widget is triggering the media library.

   👍

WRT #50.1:

+++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
@@ -11,11 +15,80 @@
+    if (!$entity_type->entityClassImplements(FieldableEntityInterface::class)) {
+      return AccessResult::forbidden("The media library can only be opened by fieldable entities.");
+    }

Okay so actually there is a convoluted way to reach this point: it’d require somebody constructing a MediaLibraryState for a non-fieldable entity type intentionally.

But that's plainly wrong code, not forbidden access.

So rather than this edge case doing return AccessResult, it should throw new HttpException(500, 'Internal Server Error');

Addressed Wim's feedback and added an explicit test of the access delegation, per @effulgentsia's request.

Re-reviewed the entire patch. No remaining concerns.

+++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
@@ -128,6 +131,37 @@
+    $this->assertSame('Denied by test module', $access_result->getReason());

+++ b/core/modules/media_library/tests/modules/media_library_test/media_library_test.module
@@ -0,0 +1,13 @@
+  return AccessResult::forbiddenIf($field_definition->getName() === 'field_media_no_access', 'Denied by test module');

👏

1. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -168,9 +196,11 @@ protected function buildLibraryContent(MediaLibraryState $state) {
      public function checkAccess(AccountInterface $account = NULL, MediaLibraryState $state = NULL) {
   +    $account = $account ?: $this->currentUser;
   

   The $account argument is always supplied here. There's no point in it being optional - see \Drupal\Core\Access\CustomAccessCheck::access. And therefore there's not point in injecting the current user service as far as I can see. See also \Drupal\migrate_drupal_ui\MigrateAccessCheck::checkAccess

2. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -189,8 +219,20 @@ public function checkAccess(AccountInterface $account = NULL, MediaLibraryState
   +    // If the user cannot even view media, there's no need for further access
   +    // checking.
   +    if ($can_view_media->isForbidden()) {
   +      return $can_view_media;
   +    }
   +
   +    // Delegate any further access checking to the opener service nominated by
   +    // the media library state.
   +    return $this->openerResolver->get($state)->checkAccess($state, $account)
   +      ->andIf($can_view_media);
   

   I think the shortcut for checking $can_view_media makes sense but that means I think we need a comment saying we are doing ->andIf($can_view_media) do add the cacheability data in. Or perhaps even better we want to use inheritCacheability() instead of andIf as this makes it clearer.

Fixed #54.1, but I after closer examination of #54.2, I decided to leave it as-is.

Why? Because inheritCacheability() is implemented on AccessResult, but not defined on AccessResultInterface -- and MediaLibraryOpenerInterface::checkAccess() returns AccessResultInterface. So, calling inheritCacheability() on the return value of checkAccess() would be assuming implementation details of the opener service in question, which strikes me as a bad idea since MediaLibraryOpenerInterface is clearly an API. andIf() is safer from that standpoint, and the default implementation will inherit cacheability anyway.

On second thought, I decided to go ahead with the suggestion in #54.2, wrapping it in an instanceof check. I figured that AccessResultInterface::andIf() does not guarantee the merging of cache metadata (its interface documentation makes no mention of that); the cache metadata inheritance is a beneficial detail of AccessResult::andIf()'s implementation. So why not take advantage of it? :)

+++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
@@ -189,8 +203,23 @@ public function checkAccess(AccountInterface $account = NULL, MediaLibraryState
+    $can_view_media = AccessResult::allowedIfHasPermission($account, 'view media')
       ->addCacheableDependency($view);
+
+    // If the user cannot even view media, there's no need for further access
+    // checking.
+    if ($can_view_media->isForbidden()) {
+      return $can_view_media;
+    }

This if statement is never entered, because allowedIfHasPermission() returns either allowed or neutral, never forbidden. If the intention is to prevent further unnecessary access checking, then I think if (!$can_view_media->isAllowed()) is what's intended. That would be fine from a cacheability standpoint.

Or perhaps even better we want to use inheritCacheability() instead of andIf as this makes it clearer.

I disagree with that. I think ->andIf() is clearer, is by far the more common/idiomatic pattern in the rest of core's codebase, and also has the advantage of being more optimized. For example, if the left side is neutral, then it does not need to (and doesn't) merge the cacheability of the argument.

I figured that AccessResultInterface::andIf() does not guarantee the merging of cache metadata

Yeah, but if the implementor of andIf() returns an object that doesn't implement CacheableDependencyInterface, then we don't cache the result, so that's fine. And if the implementor returns an object that does implement CacheableDependencyInterface, then it's responsible for returning one that does so accurately.

1. +++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
   @@ -11,11 +15,80 @@
   +    // Ensure the field definition actually exists.
   +    $field_definitions = $this->entityFieldManager->getFieldDefinitions($entity_type_id, $bundle);
   +    if (!isset($field_definitions[$field_name])) {
   +      return AccessResult::forbidden("Unknown field '$field_name'.");
   +    }
   +
   +    // Since we defer to a field to determine access, ensure we are dealing with
   +    // a fieldable entity type.
   +    $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
   +    if (!$entity_type->entityClassImplements(FieldableEntityInterface::class)) {
   +      throw new \LogicException("The media library can only be opened by fieldable entities.");
   +    }
   

   If we're validating that $field_name corresponds to a field that exists, should we also validate that $entity_type_id corresponds to an entity type that exists prior to invoking a function (entityClassImplements) on it?

2. +++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
   @@ -11,11 +15,80 @@
   +    return $this->entityTypeManager->getAccessControlHandler($entity_type_id)
   +      ->fieldAccess('edit', $field_definitions[$field_name], $account, $entity->get($field_name), TRUE);
   

   Don't we also need to check edit access on the entity itself as well? If not, why not?

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -330,7 +330,13 @@ public function testWidgetAccess() {
   +    $state = MediaLibraryState::create('media_library.opener.field_widget', $allowed_types, 'type_three', 2, [
   +      'entity_type_id' => 'media',
   +      'bundle' => 'type_one',
   +      'field_name' => 'field_media_test',
   +    ]);
   

   This is mocking a situation where a media reference field is placed on a media bundle. Which, while a possible scenario, is a confusing one, because you have to mentally keep straight what's the host and what's the target. I think it would make future reading of this easier if we use one of our test entity types (e.g., entity_test or whichever one most easily supports fields) as the host instead.

4. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
   @@ -78,14 +84,22 @@ public function testMediaLibraryAccess() {
   +    $state = MediaLibraryState::create('media_library.opener.field_widget', [$media_type], $media_type, 2, [
   +      'entity_type_id' => 'media',
   +      'bundle' => $media_type,
   +      'field_name' => 'field_media_test',
   +    ]);
   

   Same here.

+++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
@@ -11,11 +15,80 @@
   public function checkAccess(MediaLibraryState $state, AccountInterface $account) {
-    throw new \Exception('Not yet implemented, see https://www.drupal.org/project/drupal/issues/3038254.');
+    $parameters = $state->getOpenerParameters() + ['entity_id' => NULL];

Since everything after here depends on $parameters, we should probably add url.query_args (or at a minimum url.query_args:media_library_opener_parameters and url.query_args:hash) to the cache context of every returned access result. Maybe it makes sense to encapsulate that into making MediaLibraryState implement CacheableDependencyInterface, which we could punt to a separate issue if desired.

The lack of this cache context isn't currently surfacing into a bug, because when the media library is opened via /admin/content/media-widget, that's an admin route which isn't cached by dynamic_page_cache. And when it's opened via /media-library, then I think the View that's rendered is adding the url.query_args cache context, and something is also setting max-age to 0, but it's fragile to rely on those incidental things.

+++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
@@ -11,11 +15,80 @@
+    // Ensure the field definition actually exists.
+    $field_definitions = $this->entityFieldManager->getFieldDefinitions($entity_type_id, $bundle);
+    if (!isset($field_definitions[$field_name])) {
+      return AccessResult::forbidden("Unknown field '$field_name'.");
+    }

Perhaps we should also check that the field is a media field? Even further, do we want to check that it's configured to use the media library widget? Though that would require us knowing which form display to check for that.

#54.2 + #56: I don't like using inheritCacheability(). We're AND'ing two access results together. It doesn't matter that there's an early return in some case. We use andIf() just about everywhere in core. Finally, the early return could also be considered a premature optimization that should be removed.
EDIT: looks like @effulgentsia essentially states the same in #58.

#58:

This if statement is never entered

So great that you spotted this! 👏 So bad I did not notice 😩😨 It looked like @phenaproxima had copied what I proposed in #50.3, but he did not.

#59.2: great question! I thought about this too while reviewing. I concluded the current code is correct. My thought process:

1. This access control is not about editing any data, it's about the ability to access a selection dialog, which contains data that the user may not be allowed to see (photos of yet-to-be announced products for example).
2. Note that the data inside the selection dialog is itself already protected by the view media permission: MediaAccessControlHandler::access() is called for every Media entity listed by the view in the media library selection dialog. Hence there is already no risk of access bypass or information disclosure. If I apply
   

   diff --git a/core/modules/media/src/MediaAccessControlHandler.php b/core/modules/media/src/MediaAccessControlHandler.php
   index a8fdea2f63..b8ebb0181f 100644
   --- a/core/modules/media/src/MediaAccessControlHandler.php
   +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -16,6 +16,7 @@ class MediaAccessControlHandler extends EntityAccessControlHandler {
       * {@inheritdoc}
       */
      protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
   +    return AccessResult::forbidden();
        if ($account->hasPermission('administer media')) {
          return AccessResult::allowed()->cachePerPermissions();
        }
   

   and try to access my media library (which contains 2 images), I see this:
   
   No media to be seen.

   (Although there's markup there that shouldn't be there, but that's out of scope here. Created #3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label for that.)

3. So the $can_view_media access result being talked about in #58 and in other comments is solely for usability, not for security: we don't want users to be able to access the MLSD (Media Library Selection Dialog) if they're not going to see anything in there. Just like we don't render menu links that we know result in a 403 for the current user.
4. We're working on this issue as part of #2983179: [META] Implement stricter access checking for the media library, and that issue's summary provides a reason for doing this even despite media access control already being respected:
   

   a new View is added for all users with the "View media" permission. There's nothing implicitly dangerous or special about this view, but as anonymous users often have the "View media" permission sites may prefer to only allow access to the view if it's opened from the widget.

   IOW: to prevent anonymous users from browsing the entire media library, despite them having the view media permission. #3038241: Implement a tamper-proof hash for the media library state already achieves much of this, but the tamper-proof hash that it uses is based on contents, not on session/user. In other words: if an anonymous user manages to snatch a URL used by an authenticated user, then that URL would contain a hash that also works for the anonymous user — that hash works for any user.

5. So the incremental step of locking down that we're adding here is to take into account if the current user is able to access the MLSD opener — whether that's a media field, a CKEditor button on a text field, or something else entirely.
   Which finally brings me to answering your concrete question: whether to also check entity access and not only field access. fieldAccess() receives an $entity, so it can choose to provide different behavior for new vs existing entities. (\Drupal\Core\Entity\EntityAccessControlHandler::fieldAccess() does this for example.) And to allow field access logic to on entity newness, the current patch does:

   +++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
   @@ -11,11 +15,80 @@
   +    // Load the entity if we have an ID, or create it in memory otherwise.
   +    if ($parameters['entity_id']) {
   +      $entity = $storage->load($parameters['entity_id']);
   +    }
   +    else {
   +      $values = [];
   +      if ($bundle_key = $entity_type->getKey('bundle')) {
   +        $values[$bundle_key] = $bundle;
   +      }
   +      $entity = $storage->create($values);
   +    }
   

   To be honest, this is the code I was least certain about. Because \Drupal\Core\Entity\EntityAccessControlHandlerInterface::fieldAccess() is so quite ill-defined. We struggled with this in the past too, for example for Quick Edit, but also REST (to this day it is unclear whether the $items parameter should contain the existing value or the new value, for example).

   However, in this case, I felt it didn't matter as much since the access checking is not related to editing the referencing entity but to browsing the media library if the current user is able to edit this data model field in general (and not about being able to edit this data model field on this particular entity instance).

   I could be convinced either way, precisely because the API is ambiguous. But the current implementation seems correct for my browsing the media library if the current user is able to edit this data model field in general statement, since \Drupal\Core\Entity\EntityAccessControlHandlerInterface::fieldAccess()'s docs say:

      * This method does not determine whether access is granted to the entity
      * itself, only the specific field. Callers are responsible for ensuring that
   

#59.3: +1, that'd be clearer.

Also, having written #62, I think it's clear that the current issue summary is inadequately explaining what problem this is aiming to solve, and how big of a problem that really is in the first place.

Created #3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label for #62 but forgot to add it to the related issues here.

+1 to removing the premature optimisation. For me it helps but there is less complexity to review in the access method which always seems a good thing.

+++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
@@ -11,11 +15,80 @@
   public function checkAccess(MediaLibraryState $state, AccountInterface $account) {

+++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
@@ -167,10 +181,10 @@ protected function buildLibraryContent(MediaLibraryState $state) {
+  public function checkAccess(AccountInterface $account, MediaLibraryState $state = NULL) {

This is a little unfortunate. The arguments being the other way around... we changed them on the \Drupal\media_library\MediaLibraryOpenerInterface so that checkAccess matched getSelectionResponse - at my request :(

Not sure the best way out of this other than to change the order of both methods on MediaLibraryOpenerInterface since there's nothing that impacts the order of getSelectionResponse in the same way. I think this is acceptable because the interface is so new.

OK, this patch addresses most of the feedback since #56. It should also pass the tests.

Let's go comment-by-comment:

#58: I removed the early return and went back to andIf().

#59:

1. There's no need to explicitly validate that; calling $this->entityTypeManager->getDefinition() will throw an exception of the entity type does not exist. There is also no need to explicitly validate the existence of the field itself, since FieldableEntityInterface::get() will throw an exception if you try to get the field items for a field that doesn't exist. This allowed me to remove the entity field manager from MediaLibraryFieldWidgetOpener.
2. Left this as-is, per Wim's reasoning in #62.
3. Fixed.
4. Fixed.

#60: I added the url.query_args cache context to all returned access results. +1 on adding CacheableDependencyInterface to MediaLibraryState, and I will open that issue.

#61: I added checks to ensure that the field is an entity reference field, and that it references media items. Checking the entity form display feels like overkill to me, especially since it is a straight-up error condition if MediaLibraryFieldWidgetOpener::checkAccess() is ever run with a field that isn't a media-targeted entity reference field. (Which is why it throws \LogicException in those cases, rather than deny access. It should not be possible, under normal use, to cause this error.)

#62: Thanks for this very well-written and expertly thought-out comment, Wim! I had to read it three times before I got it, but I agree with your reasoning.

#65: I'd like a second opinion before I change the interface. I say this because I'd ultimately like to move MediaLibraryUiBuilder::checkAccess() into its own dedicated route-level access checker, and that would probably obviate the refactoring you propose. So, for now, I left it as it is.

Opened #3063343: Make MediaLibraryState implement CacheableDependencyInterface to remove the need for hardcoding a cache context to implement CacheableDependencyInterface on MediaLibraryState.

I could be convinced either way, precisely because the API is ambiguous. But the current implementation seems correct for my browsing the media library if the current user is able to edit this data model field in general statement

What do we define as the "data model field in general"? Suppose the only entity bundle that contains a media field is Article nodes. And suppose that the anonymous user does not have permission to edit any article nodes. By default, without a module that implements hook_entity_field_access(), ->fieldAccess() returns allowed for all fields. It's only an entity access check that would determine that the user isn't allowed to edit articles at all.

I'd like a second opinion before I change the interface. I say this because I'd ultimately like to move MediaLibraryUiBuilder::checkAccess() into its own dedicated route-level access checker, and that would probably obviate the refactoring you propose.

I think it's rare for us to have access checking methods that have more than one parameter and where $account is the first one. So, from a consistency standpoint, I think it would be best to change MediaLibraryUiBuilder::checkAccess() to put $state first. If we want to wait until we move the entire method out to a separate class, I think that's fine.

I discussed the "should we check entity access" question with @effulgentsia and we decided that, because core's out of the box field permissions are so permissive, it makes sense to check entity access as well (or create access if the entity does not exist). Kicking back for that, plus explicit test coverage.

discussed the "should we check entity access" question with @effulgentsia and we decided that, because core's out of the box field permissions are so permissive

I thought this was the the argument that would be made :) That's why I tried to explain in #62 why that is okay. In a nutshell: if a user is able to edit a field that can reference media, then the user can also access the media library.

Now fast forward to #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library, where a second consumer of the API being added by this issue will be added. A prototype of that can be found at #2998005-28: [PP-1] Support Drupal core's Media Library (which was built on top of an earlier iteration of this patch). Let me quote the relevant code:

 public function onMediaLibraryAccess(MediaLibraryAccessEvent $event) {
   $opener_id = $event->getState()->getOpenerId();
   if (!static::isEntityEmbedMediaLibraryOpenerId($opener_id)) {
     return;
   }

   $filter_format_id = mb_substr($opener_id, 20);
   $format = FilterFormat::load($filter_format_id);
   $event->setAccessResult(AccessResult::allowedIfHasPermission(
     $event->getAccount(),
     $format->getPermissionName()
   ));
 }

This checks only the text format's permission! Because in the context of embedding something from the media library in CKEditor, that is happening without any knowledge of a host entity. In fact, it may be something completely else than an entity form that is using \Drupal\filter\Element\TextFormat.

In a nutshell: if a user is able to use a text format, the user can also access the media library.

Both are about

take into account if the current user is able to access the MLSD opener

In the former, we could also check entity access. In the latter, we don't get that choice. Are we okay with that difference?

If the answer is "yes": okay, then let's continue this way, although I still am concerned this is making things very confusing. If the answer is "no": please elaborate :)

I think the text format case and the field case are different. In the case of text format, it's clear what permission to a text format means. But in the case of a field, what does "if a user is able to edit a field that can reference media" mean? Fields can only exist on entity types / bundles. And if a user does not have create/edit permission to any entity types / bundles on which a field exists, then that user does not in fact have permission to edit the field, regardless of what ->fieldAccess() says. That's why the docs for fieldAccess() says that the caller is also responsible for checking entity-level access.

For whatever it's worth, I agree with @effulgentsia on that. If the docs for fieldAccess() explicitly state that the calling code is responsible for checking entity-level access, then that's what we should do. There's no harm in it.

Rerolled, so no interdiff this time.

I added entity-level access checking and expanded the tests. MediaLibraryAccessTest now distinctly tests that entity creation permissions, entity edit permissions, and field-level permissions are respected, in addition to the stuff related to the media_library view.

This should fix it.

+++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
@@ -11,11 +15,92 @@
+    return $process_result($result);

I think we want to merge in $entity_access here. Because we want all the cacheability stuff merge. So this should be $process_result($entity_access->addIf($field_access))

Addressed comments in #78 & resolved some coding standard issues.

Thanks, @yogeshmpawar! This should bring things back to green...

Addressing #83:

1. Rephrased slightly. I don't want to explicitly mention the field widget opener in the error message, because that information will be readily available in the exception backtrace.
2. Well spotted! Fixed.
3. Fixed the doc comments and renamed the methods for extra clarity.
4. Added a helper method, and asserted that users with the proper permissions are granted access.
5. Why the hell not? Done.
6. Done.
7. Good point. Coverage added.

#72:

That's why the docs for fieldAccess() says that the caller is also responsible for checking entity-level access.

This is what convinced me 👍Thanks for your patience! 😊

#83.7++ — I'd like to see explicit test coverage for this edge case too.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -334,31 +330,14 @@
   -      'entity_type_id' => 'entity_test',
   -      'bundle' => 'test_type',
   -      'field_name' => 'field_test_media',
   +      'entity_type_id' => 'node',
   +      'bundle' => 'basic_page',
   +      'field_name' => 'field_unlimited_media',
   
   @@ -371,11 +350,10 @@
   -      'create test_type entity_test_with_bundle entities',
   +      'create basic_page content',
   

   This addressed #83.2, but I'm not sure we really want this. Generally speaking we always use the entity_test entity type in our tests, except when we are testing entity type-specific behavior. This is to ensure we don't couple to node-specific assumptions.

   Then again, node assumptions were a problem in the past, but not anymore today.

   So I think this is actually okay now. Especially because this is just the functional JS test, the kernel test is still testing with entity_test.

2. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
   @@ -175,48 +206,44 @@
   +    // Test that access is denied even without an entity to work with.
   +    $state = MediaLibraryState::create('media_library.opener.field_widget', ['file', 'image'], 'file', 2, [
   +      'entity_type_id' => 'entity_test',
   +      'bundle' => 'test',
   +      'field_name' => $field_storage->getName(),
   +    ]);
   

   I'm missing test coverage for the inverse: access granted even without an entity to work with (in case the user has the necessary permissions of course).

I'm missing test coverage for the inverse: access granted even without an entity to work with (in case the user has the necessary permissions of course).

I think we have coverage for this in testFieldWidgetEntityCreateAccess(). If no entity exists, it uses creation permissions.

It looks like one part of #85 still needs to be addressed.

#83.7++ — I'd like to see explicit test coverage for this edge case too.

Putting back to needs review just to check that this is outstanding.

Heh, thanks for your vigilance :)

I was agreeing with @seanB's request for explicit tests in #83.7, and @phenaproxima already addressed that in #84.

#86: Ah, yes, I missed that. The structure of the tests makes it hard to see what is and what isn't tested. But you are right, that edge case indeed does have test coverage.
(Ideally there'd be a single test method testing all permutations in a data provider — that's 2^3 permutations: entity access allowed/forbidden + field access allowed/forbidden + entity new/existing. If core committers think this is sufficiently clear, I won't hold this up.)

RE-RTBC'ing because #88 has been confirmed to be addressed.

Committed and pushed 94361f3394 to 8.8.x and 7b11e5f44a to 8.7.x. Thanks!

Backported to 8.7.x as the media library is experimental.

I've committed this pre the release as I believe this is advantageous but it would be great if someone can do the issue summary update as requested by @Wim Leers in #63.

Yay! Updated the #2994699 core patch to use this newly added API: #2994699-22: Create a CKEditor plugin to select and embed a media item from the Media Library.

Just as a follow-up to this, I've noticed that there's something new happening with the arguments passed to the access checkers for non-bundleable entities on the new check.

Quoted below from \Drupal\Core\Entity\EntityAccessControlHandler:

* @param string|null $entity_bundle
* (optional) The bundle of the entity. Required if the entity supports
* bundles, defaults to NULL otherwise.
*
* @return \Drupal\Core\Access\AccessResultInterface
* The access result.
*/
protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {

The $entity_bundle argument seems like it's being passed the entity type id instead of NULL for entities like Users.

To reproduce:

1) Install Drupal 8.7.7
2) Enable the media library module
3) Create an image media type
4) Add an image.
5) Create an entity reference field on the user entity to a media item
6) Go to the user add form and open the media library widget and select a media item, but do not insert it yet.
7) Set a breakpoint at line 284 in EntityAccessControlHandler.php
8) Click insert to pause at the breakpoint in the XHR request.
9) Observe that $context['entity_type_id'] is 'user' and that $entity_bundle is also 'user'.

Going by the documentation on the checkCreateAccess method, I would have expected that NULL would be passed in $entity_bundle for something like a User.

If this is agreeable, then I'd be more than willing to make a new issue and submit a patch to fix it.

Thanks