[IsGranted] allow IsGranted put on a method to override one put on class

[c33s]

Description

if you put the IsGranted attribute on a class requireing ROLE_API for example and you want to only make some exceptions on method level requireing only PUBLIC_ACCESS this is currently not possible. you have to remove the attribute from the class and add to each method.

it would be nice to have IsGranted behave more like Route where putting it on a class cooperates with putting it on a method.

i assume the procesing order of the method attributes has to go before the processing of the class attribute to honor the "first match firewall principle".

the code below leads currently to "Full authentication is required to access this resource."

sidenote: also overriding a permission defined in security.yaml would be useful. so it would be possible to define global rules in security.yaml and override them on controller method level.

    access_control:
         - { path: ^/api, roles: ROLE_API }

Example

#[Route(path: '/api', name: 'api_')]
#[IsGranted('ROLE_API')]
class ApiController extends AbstractController
{
    #[Route(path: '/secured_by_class', name: 'secured_by_class')]
    public function secured(): Response
    {
        //...
    }
    
    #[Route(path: '/public_by_method_override', name: 'status')]
    #[IsGranted('PUBLIC_ACCESS')]
    public function status(): Response
    {
        //...
    }
}

[stof]

This would be a huge BC break as projects might rely on the fact that all IsGranted are checked instead of silently ignoring the class-level checks. And this would potentially create security issues in such projects by removing security checks.

Controllers cannot override access_control either because those are totally unrelated places running security checks (and it is totally valid to have multiple security checks guarding a request, having to satisfy them all). The AccessControlListener is totally unaware of the IsGrantedListener, and vice-versa.

[c33s]

@stof yes you have very valid concerns, what do you think about adding a dedicated attribute like IsGrantedOverrideable?

[derrabus]

I'm not very fond of that idea either, mostly because of the complexity it adds to our codebase.

I mean, you could also move the one action that needs a different behavior to a separate controller, right?

[c33s]

thanks for your comment @derrabus, yes you are right, moving the few methods which need different config out to their own controller works fine and yes i can imagine the codecomplexity this feature would add (if it would be so easy i might even had made a pull request for it)

my perspective on this feature is more security related: it's about not to have to add an IS_GRANTED to each method in a controller which can easily be forgotten. this would be catched by a class level IS_GRANTED which is overrideable if needed.
that are just my thoughts which came up on my journey from moving the grant rules from security.yaml to the controller and thereby noticing the risk of forgetting something which would lead to an "open door".
so why to move from security.yaml to controller: it has the benefit of not forgetting to change the url in secuity.yaml after changing a route path in the controller (like shifting from api/v1 to api/v2 but still have api/v1 as rule in security.yaml

feel free to close as wont-fix, i fully understand that.

[mvhirsch]

so why to move from security.yaml to controller: it has the benefit of not forgetting to change the url in secuity.yaml after changing a route path in the controller (like shifting from api/v1 to api/v2 but still have api/v1 as rule in security.yaml

We had a similar use-case where some devs forgot to add annotations/attributes to classes. For ADRs like this, we used to have a CI job running in place. You may take a look at PHPArkitect (or simple shell scripts).

And as always: tests should cover unauthorized access; that way you can't forget a route.

[chalasr]

Closing then (#54676 (comment)) :)