Need to have more granularity on access to redirect entity

I ran into this today too. In my use case I wanted to be able to access the list of Redirect entities via JSON API as an anonymous user. The patch in #2 makes it work to get to the actual entities, but not the listing of entities at a route like /jsonapi/redirect/redirect. The attached patch adds a collection_permission entry to the entity annotation to allow for accessing the listing of entities if you have the 'view redirect entities' permission.

Hmm. This should fix the tests I think ...

Looks like we need to add some code to make sure we handle the admin_permission set in the redirect Entity's annotation. This new patch checks for the "administer redirects" permission and grants view/update/delete (or any other $operation) to anyone with that permission.

1. +++ b/redirect.permissions.yml
   @@ -1,4 +1,23 @@
    administer redirects:
      title: 'Administer individual URL redirections'
   +  restrict access: true
    administer redirect settings:
      title: 'Administer global URL redirection settings'
   +  restrict access: true
   +
   +view redirect entities:
   +  title: 'View redirect entity'
   +  description: 'Allow to view redirect entity'
   +
   +create redirect entities:
   +  title: 'Create redirect entity'
   +  description: 'Allow to create redirect entity'
   +
   +edit redirect entities:
   +  title: 'Edit redirect entity'
   +  description: 'Allow to edit redirect entity'
   +
   +delete redirect entities:
   +  title: 'Delete redirect entity'
   +  description: 'Allow to delete redirect entity'
   

   No reason to include "entity" in the title/description, maybe we can reuse the "individual redirects" terminology from the administer permission?

   Also the descriptions seem useless, if anything, then I'd suggest to add one on the administer permission to make it clear that it allows create, edit and update?

   And last, adding the restrict access flag here seems unrelated and I'm not convinced that it is necessary. That's reserved for permissions that are relevant for security.. anything that requires such a permission is excluded from security issues for example. I don't think that is the case here, you can do funky stuff with redirects but actually, the same is true for the create permission.

   And last, tests...

2. +++ b/src/Entity/RedirectAccessControlHandler.php
   @@ -0,0 +1,50 @@
   +  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
   +    $admin_permission = $this->entityType->getAdminPermission();
   +    if ($account->hasPermission($admin_permission)) {
   +      return AccessResult::allowed()->cachePerPermissions();
   +    }
   +
   +    switch ($operation) {
   +      case 'view':
   +        return AccessResult::allowedIfHasPermission($account, 'view redirect entities');
   +
   +      case 'update':
   +        return AccessResult::allowedIfHasPermission($account, 'edit redirect entities');
   +
   +      case 'delete':
   +        return AccessResult::allowedIfHasPermission($account, 'delete redirect entities');
   +    }
   

   An alternative way to implement this would be to use allowedIfHasPermissions() in each case below with the specific + the admin permission, might be slightly easier, not sure.

Thanks for the feedback @Berdir. Here's an updated patch which does the following:

• Removes the word "entities" from each of the permissions
• Removes the extraneous descriptions and use of the "restrict access" flag
• Refactors the code in RedirectAccessControlHandler to use AccessResult::allowedIfHasPermissions(), which I agree is a little bit clearer to read
• Adds tests for the new CRUD permissions

Great work @eojthebrave, we have the requirement on our project to allow roles to only edit/delete their own redirects, therefore I've extended the patch to include those permissions and altered the access control handler to suit. The access control logic is based off core's MediaAccessControlHandler.

We need to cache per user on the netural return so we still vary by user when access is denied.

I wasn't aware that redirects even have an author o.0. Probably copied that from node, also visible in the wrong description.

That said, I'm not convinced that this use case is common enough to be supported by default. own-permissions tend to be tricky with caching and so on.

You should be able to implement that in a custom module with hook_redirect_access()?

own-permissions tend to be tricky with caching and so on.

You're not wrong there! #2998824: MediaAccessControlHandler update/delete access caching is not correct shows that it can mess caching pretty heavily, however since these are only for edit/delete operations it shouldn't matter too much? I'm happy to move this into a custom module if you don't want to support it in the main one, understand it's an edgey use case.

Can this be redirect sub module? If not @acbramley we can make a new contrib module out of it.

I wasn't aware that redirects even have an author o.0. Probably copied that from node, also visible in the wrong description.

I think its safe to say author != owner.

The previous patch and issues such as #2220061: Add permission to restrict access to a user's own redirects only want to merge the two.

@Berdir how do you feel about progressing from patch @ #10?

Could you add your opinion / close out #2220061: Add permission to restrict access to a user's own redirects only too?

Looks good to me, working well with Entity Share!

This also needs hook_jsonapi_entity_filter_access() to be implemented so that the users with the permission can see JSONAPI listings of redirects.

Because redirects don't have a published status, JSONAPI_FILTER_AMONG_ALL needs to be used.

I was using Patch#12 for months with Redirect:1.10 and was working fine.

However, it's not working with Redirect:1.12 so reverting back to Redirect:1.10.

On a clean install the patch from #12 works on 1.12 but fails on Redirect 1.13.

composer cc
composer dump-autoload
composer install

Note that the patch will still apply successfully on 1.13, it just doesn't work.

Here's an version that works with 1.13

I can confirm that the patch from #12 applies to 1.13 but no longer works. Unfortunately the patch from #24 doesn't seem to apply to 1.13.

I can confirm that patch#24 doesn't apply with 1.13.

Opened MR 221 based off of patch #12, plus JSON:API filtering. Tests are passing locally from Drupal 10.3 to 11.4.

Since the MR is based on the branch head and doesn't apply to 1.13 cleanly, attached is a patch based off 1.13. It's identical to the MR except the JSON:API hook is in the .module file since the hook class doesn't exist yet.