sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Add possibility to take control over checkAccess and hasAccess methods. #7581

Closed mleko64 closed 2 years ago

mleko64 commented 2 years ago

I have described the problem in detail here: https://stackoverflow.com/questions/69687646/how-to-control-access-to-actions-without-override-checkaccess-and-hasaccess-meth

TL;DR The most important (in my opinion) "checkAccess" and "hasAccess" methods are marked as "final" and cannot be overwritten in Admin classes any more to handle access to actions on my own.

The problem concerns, for example, cases when we want to control access to actions based on the state of a given object.

It would be nice if additional dedicated methods were implemented (for example, preCheckAccess and preHasAccess) that could be overwritten in the Admin class and take control of the permission checking process.

VincentLanglet commented 2 years ago

Hi, I don't think this would be needed.

checkAccess and hasAccess rely on the isGranted method which rely on the $this->getSecurityHandler()->isGranted() method.

So you can use custom voters. And if you don't like the strategy of the RoleSecurityHandler that allow a Super Admin to bypass the voters, you can implement your own SecurityHandler.

mleko64 commented 2 years ago

I really like the startegy of the "RoleSecurityHandler" (this covers most part of cases) and I do not want to resign from the Super Admin role permission check. Sometimes it is necessary to deny (or allow) access to a given action no relay on permission (no relay on "isGranted"). The "checkAccess" and "hasAccess" methods use the action and the object (if any). SecurityHandler uses the ready permission to check and the object (if any). How can I implement my own access logic to a given action still based on the action and the object and not on the permission?

I believe that the "checkAccess" and "hasAccess" methods should not be strongly related to permissions and should give the freedom to implement any logic, where based on a specific action and object (if any) we allow access (or not).

The "isGranted" method does not contain information about the action (only about the permission).

VincentLanglet commented 2 years ago

I really like the startegy of the "RoleSecurityHandler" (this covers most part of cases) and I do not want to resign from the Super Admin role permission check.

You can decorate the role security handler to rely on him most of the time except some exceptions.

Sometimes it is necessary to deny (or allow) access to a given action no relay on permission (no relay on "isGranted"). The "checkAccess" and "hasAccess" methods use the action and the object (if any). SecurityHandler uses the ready permission to check and the object (if any). How can I implement my own access logic to a given action still based on the action and the object and not on the permission?

Most of the action have different permissions

Also there is the getAccessMapping to override the access permission, you might be able to use this to change the permissions if needed.

I believe that the "checkAccess" and "hasAccess" methods should not be strongly related to permissions and should give the freedom to implement any logic, where based on a specific action and object (if any) we allow access (or not).

There was too much logic in the admin and we're trying to promote people decorating the admin services.

mleko64 commented 2 years ago

You light me up and I found another solution. I thought all the time that I can have only one SecurityHandler (configured in configuration file under "sonata.security.handler" key, but I discovered in Sonata\AdminBundle\DependencyInjection\Compiler\AddDependencyCallsCompilerPass (near line 274) that is possibility to override the default SecurityHandler (configured in global configuration) to another (for specific Admin class) via "security_handler" tag. Am I right?

If yes, then please tell me if this approach is correct:

  1. In the global configuration, I leave the default RoleSecurityHandler

  2. For specific Admin class, I create new SecurityHandler:

    <?php
    
    namespace App\SecurityHandler;
    
    class SpecificAdminSecurityHandler implements SecurityHandlerInterface
    {
      private RoleSecurityHandler $defaultSecurityHandler;
    
      public function __construct(RoleSecurityHandler $defaultSecurityHandler)
      {
         $this->defaultSecurityHandler = $defaultSecurityHandler;
      }
    
      public function isGranted(AdminInterface $admin, $attributes, ?object $object = null): bool
      {
         // Custom logic for denied based on $attributes (for example "EDIT" and $object)
         if (...) {
            return false;
         }
    
         return $this->defaultSecurityHandler->isGranted($admin, $attributes, $object);
      }
    }
  3. Assign new SecurityHandler to specific Admin class:

app.admin.specific:
   class: App\Admin\SpecificAdmin
   tags:
      - { security_handler: App\SecurityHandler\SpecificAdminSecurityHandler }

Of course, some code elements are omitted so as not to obscure the example. Should it work?

BTW - This section of documentation should be corrected (it is no longer possible to override the "checkAccess" method): https://docs.sonata-project.org/projects/SonataAdminBundle/en/4.x/reference/advanced_configuration/#custom-action-access-management

VincentLanglet commented 2 years ago

Yes, it's exactly this. And it can work the same if you need to override some other admin services.

BTW - This section of documentation should be corrected (it is no longer possible to override the "checkAccess" method): https://docs.sonata-project.org/projects/SonataAdminBundle/en/4.x/reference/advanced_configuration/#custom-action-access-management

Could you open a PR to update the doc ? For instance with the example you just did ?

mleko64 commented 2 years ago

Sure, PR is ready to check.