Problem/Motivation

This got spun off from #2993187-34: Add a media library widget setting to only choose existing media and subsequent discussion. The media library already delegates access control for the "add new media" form to the entity system, but it does not provide any additional context (e.g., the MediaLibraryState object). If it did, then contrib modules and custom code could support more complex access control, including the functionality requested in #2993187: Add a media library widget setting to only choose existing media.

Steps to reproduce

N/A

Proposed resolution

In \Drupal\media_library\MediaLibraryUiBuilder::buildMediaTypeAddForm(), pass the MediaLibraryState object to the access control handler, in its $context parameter.

Remaining tasks

Create a merge request with test coverage.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD.

Issue fork drupal-3224530

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Status: Active » Needs review
seanB’s picture

Status: Needs review » Reviewed & tested by the community

That looks good to me! Thanks.

phenaproxima’s picture

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

I think we should add something to the test that makes the access decision based on something that's in the library state (not just that some library state got passed). That would also help clarify what an example use-case for it is.

phenaproxima’s picture

Status: Needs work » Needs review

Good call, @effulgentsia. I changed it so that the hook actually tries to use the state object to make an access decision.

seanB’s picture

Status: Needs review » Reviewed & tested by the community
larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Couple of questions on the MR, looks good though, nice and simple

phenaproxima’s picture

I take it back. If you look at the code of MediaLibraryUiBuilder, you'll see that it $build['content'] always has a 'form' element (see the buildLibraryContent() method). But it is always going to be absolutely empty if access is denied, per this code at the top of buildMediaTypeAddForm():

    $selected_type_id = $state->getSelectedTypeId();

    $access_handler = $this->entityTypeManager->getAccessControlHandler('media');
    $context = [
      'media_library_state' => $state,
    ];
    if (!$access_handler->createAccess($selected_type_id, NULL, $context)) {
      return [];
    }

So I think that, to prevent further refactoring and scope creep, assertEmpty() and assertNotEmpty() are the correct assertions for us to use.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Replied to all threads; for various reasons, I'm not sure any changes are needed here. So, tentatively restoring RTBC.

larowlan’s picture

Ok, thanks for entertaining my questions.

As there are no changes since my review, restoring @seanB's RTBC

larowlan’s picture

Issue tags: +Needs change record

Can we get a change record here detailing that this is available in $context

phenaproxima’s picture

Issue tags: -Needs change record

Created https://www.drupal.org/node/3225885 to document the change.

larowlan’s picture

  • larowlan committed 16edf0a on 9.3.x
    Issue #3224530 by phenaproxima, effulgentsia: Pass the media library...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 16edf0a and pushed to 9.3.x. Thanks!

Published the change record

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.