Allow entities to specify a "collection permission"

I'd like to bump this issue again, I see it's been silent for a long time now and yet this issue persists in Drupal 9.5.x and I'm guessing 10.x.x as well.

Not being able to simply allow editors/site builders to see the media collection is a *huge* issue in my opinion, granting them "administer media"-permissions is far from ideal and in fact a completely ridiculous idea in my opinion.

I've "solved" this in one of our projects by simply altering the entity.media.collection route by subscribing to the routes alter event as shown below.

Yet, this is not the solution. I refuse to apply this to all our projects, this is something so trivial that simply needs to land in Core.

Can we make any progress with this issue and hopefully come up with a solution that works for everyone?

<?php

namespace Drupal\quintessence_general\Routing;

use Drupal\Core\Routing\RouteSubscriberBase;
use Symfony\Component\Routing\RouteCollection;

/**
 * Subscribes to route alter events.
 */
class RouteSubscriber extends RouteSubscriberBase {

  /**
   * {@inheritDoc}
   */
  protected function alterRoutes(RouteCollection $collection) {
    if ($route = $collection->get('entity.media.collection')) {
      $route->setRequirement('_permission', $route->getRequirement('_permission') . '+access media overview');
    }
  }

}

Also want to bump this. The semantics arguments around viewing one vs many entities was a complete departure from the actual goal of this issue: to provide a permission for granting access to the route that displays the view provided by the entity's list_builder handler. This really had nothing to do with json api or general entity access.

@mglaman had a perfect solution for all of this in #29

list_builder_permission would mitigate the semantics issue around the route being "collection" and permissions being named "overview". The use case we're working against is the list builder, not an API resource with a collection of entities.

If I remember, I'll try to roll a new patch this week.

Alright, I've created an MR that includes the patch from #56 and a fix for the documentation issue noted in #57. After reading through things again today, I think that @AaronMcHale was spot on in #57, so I've left the permission name as-is instead of changing it to list_builder_permission or whatever else. I agree that followups are probably the place to handle additional core entity types. If tests pass then I will mark this RTBC in the hopes that we can move things forward.

Alright, given that tests are passing, and reiterating that I think #57 perfectly sums up why this approach is okay, I'm marking this RTBC.

✨ Add more granular block content permissions Fixed introduce a access block library permission for block_content in 10.1.x, so we could also add that to the block content entity type annotation.

So I went ahead and rebased this for the 11.x branch. I initially pushed this to the existing MR, but then realized that I cannot change the target branch there, so I then created a new MR. Sorry if this caused any confusion. I also added the collection permission to content blocks per #72.

Since this was tagged "Needs documentation" I wanted to go ahead and take a stab at that, but I have to disagree with #36 ("This should be documented in entity.api.php, just like admin_permission is already documented there." @WimLeers). The only mention of the admin permission in entity.api.php is the following:

most entities can just use the 'admin_permission' annotation property instead [of declaring a custom access control handler]

That sentence is still valid with this issue and I contend that that counts as actual documentation of that property. I agree, however, that we should have proper documentation for the admin permission, so I opened 📌 Add proper documentation for entity type's admin_permission and collection_permission Needs review . Depending how that goes, we should add documentation for the collection permission here. I'm removing the "Needs documentation" tag for that reason.

Finally, I re-read the discussion above about whether or not this should impact JSON:API and I still strongly believe that it should not and I think the fact that JSON:API is now in core makes this an easier case to make than when we had the discussion before. Before we were discussing whether or not adding the concept of a collection permission in core would lead to the expectation that JSON:API in contrib should be affected by that in any way. Now that JSON:API is in core we no longer have this ambiguity of expectations: by not changing anything in JSON:API we make it very explicit that we do not consider exposing an empty list for a user without the collection permission a security issue. Another decoupled module may or not make the same choice that JSON:API did, but in any case it cannot be considered a security vulnerability because core sets this precedent.

Furthermore, one thing that @gabesullice brought up above (#31) that I did not pick up completely in the discussion previous that I think is the nail in the coffin of any reservations against my argument is the fact that JSON:API already grants access to the JSON:API collection for users without the admin permission. While the collection permission is useful for editorial entities, we will always have lots of entity types, in particular config entity types, which only declare an admin permission and not a collection permission. And since their JSON:API collection routes will always be available (albeit empty) for anyone regardless of any permissions, it would be horribly inconsistent for JSON:API to start denying access to its collection as soon as a collection route is declared.

To sum up, I think this is finally ready to land.

Can the change record be updated please.

What exactly do you think needs updating?

Was last updated 5 years ago to be introduced in 8.6

Ahh OK, missed that. Done.

Thanks! Will try and review this evening or tomorrow if no one beats me to it.

One nit picky I see is if the new get function could be type hinted. Probably can be handled during commit but if you get time.

Great, looking forward to it @smustgrave, thanks!

Added the return typehint.

Hiding patches as the fix is in MR 4201

Test coverage appears to cover the different combos of collection and admin permission.

I don't see any lose for those using just admin permission either.

Think this is ready for committer review.

Just had a quick look at the MR and it also looks fine to me.

Committed/pushed to 11.x, thanks!

This is very API focused but tagging for 10.2 release highlights in case there's something to group it with.

Thank you so much @tstoeckler for your detailed write-up in #76! I was wondering how this impacted JSON:API and was surprised to see no mention at all in either the diff or the change record → . I do think it'd be valuable to update the change record with how exactly this does or does not impact "the API use case" — no matter whether it's REST, Views REST export displays, JSON:API or GraphQL.

Would it be possible to still add that? 🙏

I added a note to the change record about JSON:API, fair enough. Not sure what to say about the other examples, though:

• Rest doesn't have anything like a collection, so not sure where people could possibly expect the collection permission to be relevant.
• Views Rest export displays can be a collection, but you can configure the access control, how you want, so you can just configure whatever permission you want to be checked for your export and in fact you could use "access content overview" or any other collection permission for the display. So not exactly sure what we could add to the change notice in regards to that
• Not really sure about GraphQL as I have never properly used it so couldn't really say if or in how far the introduction of the collection permission might need clarification there.

So yeah, totally fine with making the change notice more explicit and ideally more helpful for people, but not really sure what to add, in particular.

I think explicitly documenting that they are not affected would be very valuable! People reading this will wonder "how does this affect me?", and seeing JSON:API + routes-for-your-entity-type information is great, but I think the first 2 bullets you listed in #88 would make excellent additions to the change record! 🤩

OK, added some more info about Rest and Views. To be perfectly honest, I personally find the change record less clear after the last diff, but if you find this clarifying then I guess there's a decent chance others will, as well, so let's go with it.

if you have issues with the permission this maybe can help:

entity.ENTITY_NAME.collection:
  path: 'PATH_TO_LIST_FROM_ENTITY_ANNOTATION'
  defaults:
    _entity_list:  'ENTITY_ID'
    _title: 'TITLE'
  requirements:
    _permission: 'YOUR CUSTOM PERMISSION'
    _role: YOUR ROLE

it's very like to the one shared by vlad.dribas but with you don't pass the role sometimes don't work correctly.

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

tstoeckler → closed merge request !4201