New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security] add 'is_granted' to default security ExpressionLanguage? #23084

Closed
dmaicher opened this Issue Jun 6, 2017 · 6 comments

Comments

Projects
None yet
6 participants
@dmaicher
Contributor

dmaicher commented Jun 6, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version 3.4

In one of my projects I would like to evaluate a complex expression involving checking other custom attribute voters.

Unfortunately this does not work out of the box:

$authChecker->isGranted(new Expression("is_granted('custom_attr') and/or ..."))

==> The function "is_granted" does not exist around position 1 for expression ...

My solution for now is to add a custom extension for the security ExpressionLanguage and tag it with security.expression_language_provider:

use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\ExpressionLanguage\ExpressionFunction;
use Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface;

class ExpressionLanguageProvider implements ExpressionFunctionProviderInterface
{
    /**
     * @var ContainerInterface
     */
    private $container;

    public function __construct(ContainerInterface $container)
    {
        $this->container = $container;
    }

    public function getFunctions()
    {
        return [
            new ExpressionFunction('is_granted', null, function (array $variables, $attributes, $object = null) {
                return $this->container->get('security.authorization_checker')->isGranted($attributes, $object);
            })
        ];
    }
}

As @stof pointed out here symfony/symfony-docs#4282 there is a circular dependency and that's why I get the security.authorization_checker directly from the container. It would also be possible to use a ServiceLocator for it in 3.4.

WDYT? Should we think about adding this to the core? To me it seems quite useful.

@Hanmac

This comment has been minimized.

Show comment
Hide comment
@Hanmac

Hanmac Jun 7, 2017

i think it's nice, but i don't know if you need the whole ContainerInterface when you can just use a AuthorizationCheckerInterface? Edit: oh okay i see with "circular dependency" why it doesn't work.

hm i don't know the deeper part, but can it be directly added to Symfony\Component\Security\Core\Authorization\ExpressionLanguageProvider? Hm i think it can't because it doesn't work with AuthenticationTrustResolverInterface?

need to think whats the better way would be to add this into core

Hanmac commented Jun 7, 2017

i think it's nice, but i don't know if you need the whole ContainerInterface when you can just use a AuthorizationCheckerInterface? Edit: oh okay i see with "circular dependency" why it doesn't work.

hm i don't know the deeper part, but can it be directly added to Symfony\Component\Security\Core\Authorization\ExpressionLanguageProvider? Hm i think it can't because it doesn't work with AuthenticationTrustResolverInterface?

need to think whats the better way would be to add this into core

@derrabus

This comment has been minimized.

Show comment
Hide comment
@derrabus

derrabus Jun 7, 2017

Contributor

I tried to use is_granted in an ACL expression a couple of weeks ago and was disappointed that it did not work as expected. I've managed to work around it, yet I would see a benefit in adding this feature.

Contributor

derrabus commented Jun 7, 2017

I tried to use is_granted in an ACL expression a couple of weeks ago and was disappointed that it did not work as expected. I've managed to work around it, yet I would see a benefit in adding this feature.

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Jun 7, 2017

Contributor
Contributor

dmaicher commented Jun 7, 2017

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Jun 7, 2017

Contributor

@derrabus good point 😊 This would of course mean we can also use is_granted(...) on ACL expressions 👍

Contributor

dmaicher commented Jun 7, 2017

@derrabus good point 😊 This would of course mean we can also use is_granted(...) on ACL expressions 👍

@jvasseur

This comment has been minimized.

Show comment
Hide comment
@jvasseur

jvasseur Jun 7, 2017

Contributor

The circular dependency should be resolved in Symfony 3.3 now that voters are lazy loaded.

Contributor

jvasseur commented Jun 7, 2017

The circular dependency should be resolved in Symfony 3.3 now that voters are lazy loaded.

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Jun 7, 2017

Contributor

@jvasseur ah good point 👍 I'm still on 2.8 with this particular app where I'm using it 😋

Contributor

dmaicher commented Jun 7, 2017

@jvasseur ah good point 👍 I'm still on 2.8 with this particular app where I'm using it 😋

@fabpot fabpot closed this May 21, 2018

fabpot added a commit that referenced this issue May 21, 2018

feature #27305 [Security/Core] Add "is_granted()" to security express…
…ions, deprecate "has_role()" (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Security/Core] Add "is_granted()" to security expressions, deprecate "has_role()"

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #23084
| License       | MIT
| Doc PR        | -

Because `has_role()` doesn't use the auth checker, it is confusing. Let's move `is_granted()` to core (it's provided by SensioFrameworkExtraBundle / ApiPlatform for now.

Commits
-------

9dbf399 [Security/Core] Add "is_granted()" to security expressions, deprecate "has_role()"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment