Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label

I think we're going to need tests here :)

Views unfortunately does not check entity access on query or render time. We would probably need something like #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() to fix the views query, and if we hide the row on render we would probably break the views pager (if we have 10 rows per page and then hide 4 rows for example it would look pretty weird).

I think for we should at least fix the label disclosure in the checkbox, and wait for #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() so we can implement a better solution to hide inaccessible rows completely.

Here is a test for it and a fix. The standards views bulk form contains the same problem, so had to fix it there as well to make the test pass for the admin/content/media overview page.

Hmm, according to UserAccessControlHandler::checkAccess(), the anonymous user's profile can neither be viewed, updated nor deleted. Yet in the BulkFormTest we do exactly that, trying to block an anonymous user. Not sure what to do here. I think the BulkFormTest is doing something that shouldn't be possible so we need to change the test, but more opinions are very welcome.

    // The anonymous user's profile can neither be viewed, updated nor deleted.
    if ($entity->isAnonymous()) {
      return AccessResult::forbidden();
    }

In the best case, this will happen in a year. We can't wait for that. Let's add a work-around, even if it's hacky, and add a @todo pointing to that issue.

I agree, and I think the workaround to remove the checkbox should be good enough for now. Users should try to use views filters as much as possible to prevent inaccessible rows from being shown in the mean time.

#12: that seems out of scope here?

Not so sure about this. This issue is about the media library disclosing information it shouldn't. That happens via BulkForm and MediaLibrarySelectForm. If we fix the issue in BulkForm, we have to change the BulkFormTest in the user module as well, since the test is apparently doing something that should not be possible.

We could split the issue up, I'm perfectly fine with that if it helps, but both bulk forms need to be changed to fix the original information disclosure issue for the media library.

since the test is apparently doing something that should not be possible.

@seanB as posted on slack, it's not doing something that should not be possible, it is a bad test, it doesn't check that the anon user isn't blocked before trying to block it.
If you change the test to:

    // Attempt to block the anonymous user.
    $anonymous_account = $user_storage->load(0);
    $this->assertTrue($anonymous_account->isBlocked(), 'Ensure the anonymous user is blocked.');
    $edit = [
      'user_bulk_form[0]' => TRUE,
      'action' => 'user_block_user_action',
    ];
    $this->drupalPostForm(NULL, $edit, t('Apply to selected items'));
    $anonymous_account = $user_storage->load(0);
    $this->assertTrue($anonymous_account->isBlocked(), 'Ensure the anonymous user got blocked.');

it still passes

The problem I see with the proposed change here to the generic bulkform field is that the 'view' permission is used as a 'one permission to rule them all', if you don't have 'view' access, you cannot use the entity at all. The problem is that we don't know which action the user is going to attempt, so we have no idea which action to check the permission for. Using 'view' as a catch-all seems nice, but it means the setup where you have 'edit' rights but not 'view' rights is not possible. Might make sense for most entity types, but for ALL? Not so sure.

#7, I guess you can try https://www.drupal.org/project/entity/issues/2909970 ?
Need entity module though

As mentioned by Lendude on Slack, the view label operation could allow access to information in a views row, so that would be good to check that as well.

#15

@seanB as posted on slack, it's not doing something that should not be possible, it is a bad test, it doesn't check that the anon user isn't blocked before trying to block it.

You are right, I removed the test since it doesn't actually do anything.

#16

The problem I see with the proposed change here to the generic bulkform field is that the 'view' permission is used as a 'one permission to rule them all', if you don't have 'view' access, you cannot use the entity at all. The problem is that we don't know which action the user is going to attempt, so we have no idea which action to check the permission for. Using 'view' as a catch-all seems nice, but it means the setup where you have 'edit' rights but not 'view' rights is not possible. Might make sense for most entity types, but for ALL? Not so sure.

Currently you can perform actions on entities you are not allowed to see, which is exactly the "bug" we are trying to fix. How can you edit/delete information you don't have access too? Please correct me if I'm wrong, but that doesn't really seem to make sense.

#17

#7, I guess you can try https://www.drupal.org/project/entity/issues/2909970 ? Need entity module though

Unfortunately, we can't depend on contrib code in core.

I think @Lendude's concerns are valid - and if we want to fix them here then we need to postpose this issue on a views bulk form issue to implement this. I think this might not be an issue for nodes because node_access is checking on the query level.

Alternatively...

+++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
@@ -311,6 +311,7 @@ public function viewsForm(&$form, FormStateInterface $form_state) {
+          '#access' => $entity->access('view label') || $entity->access('view'),

Adding this feels tricky. Somethings like the awkward test added in core/modules/user/tests/src/Functional/Views/BulkFormTest.php depend on this not being there. How about we make this configurable?

@Wim Leers we can define a new obey_entity_view_access option (or whatever you want to call it) in \Drupal\views\Plugin\views\field\BulkForm::defineOptions() and make it configurable in \Drupal\views\Plugin\views\field\BulkForm::buildOptionsForm() and use it in \Drupal\views\Plugin\views\field\BulkForm::viewsForm() and then update the media library view to use this new option.

Created #3070237: Bulk form for Inaccessible entities still have rows generated for them in views, containing form wrapper markup *and* the label. Rerolled on top of that.

That was a debugging line I forgot to remove, sorry about that. Also rerolled on top of #3070237-7: Bulk form for Inaccessible entities still have rows generated for them in views, containing form wrapper markup *and* the label.

Sorry, I'm not sure which issue to comment in... my comment is actually about #3070237: Bulk form for Inaccessible entities still have rows generated for them in views, containing form wrapper markup *and* the label but I see the "backstory" in here.

@Lendude in #16 says:

"The problem I see with the proposed change here to the generic bulkform field is that the 'view' permission is used as a 'one permission to rule them all', if you don't have 'view' access, you cannot use the entity at all. The problem is that we don't know which action the user is going to attempt, so we have no idea which action to check the permission for. Using 'view' as a catch-all seems nice, but it means the setup where you have 'edit' rights but not 'view' rights is not possible. Might make sense for most entity types, but for ALL? Not so sure."

I basically agree 100% with @seanB in #18:

"Currently you can perform actions on entities you are not allowed to see, which is exactly the "bug" we are trying to fix. How can you edit/delete information you don't have access too? Please correct me if I'm wrong, but that doesn't really seem to make sense."

Indeed...

The initial premise seems invalid to me, because in order to use Views as the mechanism to edit or delete a bunch of entities, you must have access to at least view the entity label, if not anything else about the entity. Otherwise you get the very UI that we're trying to fix here, and subject yourself to random wanton data destruction by clicking things and seeing what happens. :P

@alexpott says in #19:

"I think @Lendude's concerns are valid - and if we want to fix them here then we need to postpose this issue on a views bulk form issue to implement this. I think this might not be an issue for nodes because node_access is checking on the query level."

I personally think the "if" we want to fix them, is no. If you're trying to set up some kind of highly restricted edit of entities you can't view, then custom code is just a few keystrokes away. :)

If I'm missing an obvious use case here, let me know. But at a glance this seems like a very straight-forward bug we just ought to fix, rather than adding more UI options to Views which is already a sea of UI options.

So, the main flag I wanted to raise was that we are effectively adding a form of hierarchical permission here, were we have one permission that overrules a second one (if you don't have view label you effectively don't get edit/delete entity).

So to do that properly we would need something like #1200572: Concept of a hierarchical permission system (I am NOT proposing we postpone on that :D)

To have that within media is fine I think, that is one specific entity type, but to have that for the generic entity bulk field handler in Views should be done with a little more caution I feel. Hierarchical permissions is not a pattern that is used in core that much I think and it's not something we can enforce elsewhere without the mentioned issue landing.

I'm not saying we should block on this, but I want to at least have it noted that we making this decision.

But my suggestion would be to just fix it in media. If we fix it in views that we would have one field handler that behaves differently then all the others in this particular set up. All other fields are still going to show (including the actual title field!), just not the bulk field. See sceenie from the other issue:

I think that is just going to lead to more questions, but that is just a guess obviously.

But at a glance this seems like a very straight-forward bug we just ought to fix

I fully agree with @seanB in #7 that the real bug/task is #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter(), and it is not that straight-forward :)
If we add a workaround for that, maybe add a @todo that makes it clear that it is a band-aid solution waiting on a proper fix?

I'm leaving this issue open for the questions around handling Media (and maybe other entity) access in general, but for the specific case of access to unpublished media, see #2969678-22: Integrate Media Library with Content Moderation.

Based on #31, this issue should probably have been reported privately first.

I discussed the issue with a few members of the Security Team (members of the Security Working Group and other committers on the security team), and we agreed to continue handling the issue in public. However, since there is an info disclosure/access bypass here, promoting to critical. The fact that it's a known security issue also makes it a blocker for Media Library to be stable, as per https://www.drupal.org/core/experimental#stabilizing.

@phenaproxima and @effulgentsia also asked why this issue needed to block Media Library being stable when any view can be affected by this issue and the most complete fix would be upstream in Views. However, the Media Library is itself a view we are shipping in core, and furthermore, it's a view that's accessible to content authors. (Most of our shipped views are only available to administrators, and there are additional hardenings already in place for nodes, taxonomy, and comments.)

In my opinion, the premise of the issue summary is wrong.

It says that forbidding access via MediaAccessControlHandler::checkAccess() should be respected by Views. But that's not how Views works anywhere else. For example, if you make the same kind of change to NodeAccessControlHandler::checkAccess(), it has no bearing on what's shown in the frontpage View; that View will happily show teasers of those "forbidden" nodes.

For nodes, we explicitly document this in hook_node_access():

note that this function isn't called for node listings (e.g., RSS feeds, the default home page at path 'node', a recent content block, etc.) See Node access rights for a full explanation.

We unfortunately do not document that for other entity types (e.g., in hook_entity_access() or hook_ENTITY_TYPE_access()). So one part of fixing this issue might be to add that to those docs.

So in that case, how should a module forbid access to media entities in a way that Views, including the Media Library View, will honor? For nodes, there's the grants API that's available, but that's not available for other entity types.

In my opinion, the best option for such a module is to use the query access API provided by the Entity API module. #18 says "Unfortunately, we can't depend on contrib code in core.", but we don't have to. It's not core that's wanting to implement additional media access logic. It's a hypothetical contrib/custom module that might want to, and that module can depend on the Entity API module.

Using that API is pretty straightforward. Here's an example of a query access subscriber that a contrib/custom module can implement if it wants to forbid access to Vimeo videos:

class QueryAccessSubscriber implements EventSubscriberInterface {

  public static function getSubscribedEvents() {
    return [
      'entity.query_access.media' => 'onQueryAccess',
    ];
  }

  public function onQueryAccess(QueryAccessEvent $event) {
    $conditions = $event->getConditions();
    $conditions->addCondition((new ConditionGroup('OR'))
      ->addCondition('bundle', 'remote_video', '<>')
      ->addCondition('field_media_oembed_video.value', ['https://vimeo.com/', 'https://vimeo.com/zzzz'], 'NOT BETWEEN')
    );
  }
}

This only works if the media entity type is assigned a query_access handler, which can be done with:

function test_entity_type_build(array &$entity_types) {
  if (!$entity_types['media']->hasHandlerClass('query_access')) {
    $entity_types['media']->setHandlerClass('query_access', \Drupal\entity\QueryAccess\QueryAccessHandler::class);
  }
}

Note that that might be the wrong handler class to assign (it might have logic in it that we don't want for media entities). I opened #3086409: Provide a default query_access handler for core (maybe all?) entity types to see if Entity API can provide a better default and auto-assign it, so that all a module would need to do is implement the subscriber. But even if Entity API maintainers say no to that, and even if the above hook_entity_type_build() implementation is incorrect, a real module needing to implement query access could implement a correct QueryAccessHandler class.

Note that the above doesn't require any changes at all to core to work, because Entity API hooks into Views via hook_views_query_alter(), which is invoked for all Views.

However, if a module wanting to implement query access control doesn't want to do so via Entity API module, then the currently available core API for it is to implement hook_query_media_access_alter(), which is just a special case of hook_query_TAG_alter() with 'media_access' as the tag. The main drawback of using that API is that you're dealing with a low-level database query object, so you have to deal with adding JOINs and table and field aliases and all that cumbersome stuff. It's really really not the ideal API, which is why Entity API added the query access API and why bringing that into core via #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() is a good idea. But, for now, it's the core API that's available.

Because it's the core API that's currently available, we should fully support it, which means making sure that Views actually tags its entity queries correctly. It currently does for some entity types, but media isn't one of them. Here's a patch that adds the tag for media and the other ones. Note that I got permission from the security team to post this patch to this issue.

In my opinion, this one line fix is all that is needed to mark this issue fixed. But I'm open to people disagreeing with that. In particular, we should decide if we also want to add test coverage for it (and where, since it's not a Media Library specific fix), and if we also want to add docs for hook_ENTITY_TYPE_access() that lets people know that it's not invoked for listings, similar to how we document hook_node_access() with that information. However, a challenge with writing those docs is that we'd need to decide what to point people to: the Entity API project, or hook_query_ENTITY_TYPE_access_alter(), or neither, or both.

Thanks for the write up @effulgentsia!

In my opinion, the premise of the issue summary is wrong.

It says that forbidding access via MediaAccessControlHandler::checkAccess() should be respected by Views. But that's not how Views works anywhere else.

💯

Everything else you write flows logically from this.

However, if a module wanting to implement query access control doesn't want to do so via Entity API module, then the currently available core API for it is to implement hook_query_media_access_alter(), which is just a special case of hook_query_TAG_alter() with 'media_access' as the tag.
...
Because it's the core API that's currently available, we should fully support it, which means making sure that Views actually tags its entity queries correctly.

💯

In my opinion, this one line fix is all that is needed to mark this issue fixed. But I'm open to people disagreeing with that.
...
In particular, we should decide if we also want to add test coverage for it (and where, since it's not a Media Library specific fix),
...
and if we also want to add docs for hook_ENTITY_TYPE_access() that lets people know that it's not invoked for listings, similar to how we document hook_node_access() with that information.

I think it would be good to have a test in /core/views/modules that implements hook_query_media_access_alter().

However, a challenge with writing those docs is that we'd need to decide what to point people to: the Entity API project, or hook_query_ENTITY_TYPE_access_alter(), or neither, or both.

I think that, for this issue, we should update the hook_ENTITY_TYPE_access() docs to point to hook_query_TAG_alter(), saying "see /core/views/modules/{test_to_be_written} for an example".

Then, let's open a follow-up issue for adding a hook_query_ENTITY_TYPE_access_alter() stub that we can document more thoroughly. We can add a @todo for this as well.

How does this look as an example of a hook_query_TAG_alter() that modules could implement? This has no test coverage yet, so no need to run testbot.

Moved the query alter into a formal test module, and made it generic enough to test any of core's content entity types. 🤓

Here we go, with proper, if simple, tests of Media and Block Content entities. :)

Sorry about that, dumb mistake.

1. 😃
2. ✅Yes. Here you go.
3. That seemed a tad verbose to me, so I settled on views_test_query_access, to make room for future test code related to query access checking.
4. 👍
5. ✅Done.
6. 👍🤓
7. 👎I prefer it this way, to be honest; I find it more readable. I don't like to nest function calls too much, especially when one of the arguments is a multi-line array.
8. 👍👍👍

#44 looks great to me as far as test coverage goes. #37 suggested having the test implementation also be reused for docs, but I don't think what's in #44 is good for that purpose, because it assumes a Views query (whereas 'media_access' is also applied to entity queries that aren't necessarily Views queries) and it doesn't handle relationships.

This patch adds what I think is a good docs example. It replaces the current hook_query_TAG_alter() example, which is an outdated example for node_access, which I don't think we need anymore, since anyone wanting to actually implement node access control should use the grants API.

1. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -436,34 +442,40 @@ function hook_query_alter(Drupal\Core\Database\Query\AlterableInterface $query)
   +  // This is an example of a possible hook_query_media_access_alter()
   +  // implementation. In other words, alter queries of media entities that
   +  // require access control (have the 'media_access' query tag).
   

   The example is really helpful and makes it clear how to do something similar when needed. Even though this is pretty complex stuff. It almost makes me want to do crazy access things :). I like it! 👍

2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -634,6 +634,10 @@
   + * Note that this hook is not called for listings (e.g., from entity queries
   + * and Views). For nodes, see @link node_access Node access rights @endlink for
   + * a full explanation. For other entity types, see hook_query_TAG_alter().
   
   @@ -654,6 +658,7 @@
   + * @see hook_query_TAG_alter()
   

   This will also make a big difference for people implementing access hooks!

I don't see anything to complain about. The docs look great. The change itself is small and covered by tests. RTBC!

Yay thanks all! Should this be backported?

Tagging for release notes as an important fix.

There's no release note nor any change record here. (Minor-only criticals are queried separately of the tag and listed in their own section if they don't have other disruptions.)

This is a change where an info disclosure was fixed. Sometimes those sorts of security hardenings are disruptive for sites that might have had improperly configured permissions and been relying on the info disclosure. Is there any such disruption here? If so, we should add a CR. A CR would also help us determine how disruptive the change was and whether it's significant enough to be in the release notes as a disruptive change (rather than just a critical fix).

Release note snippet can help explain the importance of the critical issues, but are not a hard requirement really unless there is a disruption from the change.

This is a change where an info disclosure was fixed.

I don't know if I agree with this assessment.

All this patch did is make Views, by default, add the {$entity_type_id}_access tag to its queries. That change, by itself, doesn't fix any info disclosure; it does, however, enable modules to harden their queries if desired.

In fact, this patch is not doing anything that custom code couldn't already do. (One could always implement hook_views_data_alter(), in which the same change could be made.) But it does add test coverage that the query access tag is properly added, and it adds documentation to cover the fact that hook_entity_access() does not run for queries.

So this is technically a "behavior change", in that Views will now tag its queries by default, and therefore does, IMHO, deserve a CR. But I suspect it is only a potential disruption for sites which were already implementing hook_query_TAG_alter() for {$entity_type_id}_access query tags. Since that is a possibility, although maybe not a super common case, this deserves a release note too.

Thanks @phenaproxima!

#31 explains why this is a (minor) information disclosure. But yeah, let's document whatever minor disruptions there may be and write a potential release note based on that.

The release note for this issue can probably be appended to the one for #2969678: Integrate Media Library with Content Moderation.

Wrote a release note.

The release note looks great. I think we still need the full change record for it.

Added CR https://www.drupal.org/node/3088475

(already committed, switching to Needs Review since the CR should be reviewed)

I think that CR is completely accurate and explains the background, motivation, and potential effects beautifully.

The change record looks fine to me, marking fixed again.

Publish the CR