[Security] Use the is_granted expression to keep consistency with php example above

[tyx]

Q	A
Doc fix?	yes
New docs?	no
Applies to	2.4
Fixed tickets	-

As has_role expression only check for roles (simple in_array) without calling the AccessDecisionManager, it does not do the same job that
$this->get('security.context')->isGranted('ROLE_ADMIN') used in the php example just above.

It is quiete confusing.

[stof]

Nope, it does not a a simple in_array check. It still supports the role hierarchy. So the vote for roles is still the same.

However, it is true that is_granted should be used in case you want to trigger your custom voters for other attributes

[tyx]

I guess you're right. I read the code on branch 2.5:
https://github.com/symfony/symfony/blob/2.5/src/Symfony/Component/Security/Core/Authorization/ExpressionLanguage.php

I just see on master, it looks to be changed.

Indeed my point was about the custom voters not called.

[wouterj]

I'm 👍 thanks @tyx !

[weaverryan]

is_granted comes from SensioFrameworkExtraBundle, but since that's shipped with the SE, I'm totally ok with using it - but I wanted to make that point.

If we are going to use is_granted (which I also like more, if only because we already understand what it means), then we should use it everyone in this doc - we have has_role in 2 other places in this doc. We should also add is_granted to the function reference near the bottom of this section: https://github.com/symfony/symfony-docs/blob/master/book/security.rst#complex-access-controls-with-expressions

So:

1. Question: Are we cool with using is_granted even though it's in SensioFWEBundle? I am ok with this

2. If so, @tyx can you change the other 2 has_role references to is_granted in this chapter and also add a reference line (above has_role) for is_granted in the section I linked above?

Thanks!

[stof]

Question: Are we cool with using is_granted even though it's in SensioFWEBundle? I am ok with this

Given that this place is documenting the FrameworkExtraBundle annotation, it is fine

f so, @tyx can you change the other 2 has_role references to is_granted in this chapter and also add a reference line (above has_role) for is_granted in the section I linked above?

This is wrong. is_granted is available only in the @Security annotation, not in expressions handled by the SecurityVoter. So these other usages cannot be changed

[weaverryan]

@stof Ah, thanks for those details! Could we make is_granted available for all? I want to increase the consistency - from an outsider's perspective (i.e. someone not thinking about the implementation), it seems confusing that there is a difference between using security expressions in @Security versus using them in another security context.

[stof]

@weaverryan no we cannot. A voter cannot have access to the full authentication system. It is a circular dependency.

[tyx]

So what about the changes @weaverryan asked ? Should I only add mention about is_granted in the complex-access-controls-with-expressions part ?

[weaverryan]

Yes, I think we should, and I think we should use it where we can.

I don't understand yet why it can't also be made to work under access_control, but I admit I'm totally ignorant to the implementation :). I have homework to look into that further.

@tyx if I'm being unclear or if you have any questions, let me know!

[stof]

@weaverryan this is because the access_control uses the security system. Expression used there at evaluated by a voter, which then cannot have access to the full AuthorizationManager given that it would create a circular dependency.
On the other hand, the expression of the @Security annotation is not evaluated by the ExpressionVoter but by the SecurityListener of the bundle. Given that it is evaluated outside the authorization system, it can depend on this system

[tyx]

I updated the PR with is_granted mention.

Let me know if there is another modification needed.

[xabbuh]

You'll have to indent this line by two spaces to make it part of the list item.

[tyx]

done !

[wouterj]

I would add something like "(this can ony be used in the @Security annotation)" at the end of this item, to warn users and avoid confusion.

[wouterj]

As this article has been revisited in #4606 , removing this section and code example, I'm going to close this PR.

However, I would still like to thank @tyx for creating this PR as this was missing from this article.