[DX] Fix the documentation about the Symfony security

[javiereguiluz]

If you read this DX issue regarding the security roles, you can read one of the usual awesome replies by @stof: symfony/symfony#11728 (comment)

You'll see that the Security chapter and cookbooks need some serious improvements. Given that we are talking about Security, I think that this should be a high priority issue.

For now, this is the roadmap of changes to make:

• Never mention that IS_AUTHENTICATED_* are roles. They are not.
• Better explain the differences between roles, strings, expressions, ACL permissions maps, etc.
• ...

[stof]

Another reason why it is important to fix it: the expression-language security functions added in 2.4 (with the annotation in FrameworkExtraBundle 3.0) are defining has_role() which only checks for roles, and so will not grant access for IS_AUTHENTICATED_*. See https://groups.google.com/forum/#!topic/symfony2/cyNRtjfrI8Q for such a user support request

[weaverryan]

Hi guys!

A few thoughts/questions:

1. I was following the conversation on the linked issue, but I'm not convinced that I understand the problem (on a non-technical level) of calling both roles. This was originally done purely for simplicity. Explaining roles, attributes, etc seems a bit more complex.

2. I've always felt that the has_role() function seemed like a mistake. the isGranted() method gives us a nice interface that is able to handle true ROLE_ strings or attributes handled by any other voters. The has_role() is limited and breaks the simple, consistent API. There is an is_granted() function in the security expression-language, but it only exists with the SensioFrameworkExtraBundle. This also seems odd - why would this need to live outside of FrameworkBundle? I think I'm probably missing something here :).

Thanks!

[stof]

@weaverryan is_granted does not exist in the expression passed to isGranted directly, because it would be a circular reference: the voter depending on the full authentication manager.
SensioFrameworkExtraBundle adds the is_granted function, because it evaluates the expression outside the security voting system, so there is no circular reference. Note btw that using has_role to check for roles will thus be much faster than is_granted by calling directly the appropriate logic rather than all voters.

Regarding the distinction between roles and attributes, I think the fact that the doc does not explain this distinction is the source of many support requests about custom voters: the doc starts by saying that roles start with ROLE_, then talks about some special IS_AUTHENTICATED_* "roles" named differently (hint: not roles) and then says you can implement your own voter supporting other things (and even talking about ACLs later). After that, the logical question coming in is "what is a role ?". The first definition (the right one) does not apply anymore.
I already got someone asking me why the role hierarchy was not working for things checked by a custom voter. another time was someone being confused about $token->getRoles() not containing the IS_AUTHENTICATED_FULLY role. In both cases, the reason was a confusion between roles and permission attributes.

[weaverryan]

@stof Thanks for the explanation. But from a usability perspective, I still think it's not great. isGranted and is_granted (in Twig) are consistent ideas that we can teach. With the introduction of has_role (for technical reasons), things get shakier. And my guess is that the performance gain is a micro-optimization. I know this is outside of the scope of this docs ticket, but I think this sucks.

About your second paragraph, these are really clear reasons and it helps a lot. And now I agree that we should make a distinction between roles and attributes.

[stof]

@weaverryan the goal of has_role is to be able to perform a role check when passing an expression to isGranted. This is why it is there, not because of a performance. See the first sentence of my comment. the difference is that SensioFrameworkExtraBundle does not pass the expression to isGranted. It evaluates it directly.

[yguedidi]

Why not simply remove the role feature for Symfony 3.0 and then only speak about attributes?
First time I meet roles I thinked that they're "just" special attributes with a hierarchy.
I think it's possible to add the hierarchy to attributes, and then everything will be simple: just say that an attribute can be anything, a role, a scope, a state, ...

[stof]

@yguedidi roles are something coming from the user: there is $user->getRoles(). We cannot remove such feature (we would still have to give it a name).
The difference is that attributes can come from other sources:

• the OAuth scope would be set on login based on the access token being used, not based on the user associated to the access token
• the IS_AUTHENTICATED_* attributes are checked based on the token itself
• ACL attributes are checked based on the ACL system.

Removing the roles would make it much harder to use the security system (or would force people to reimplement it in their own code, which makes the removal a bad option for DX)

[yguedidi]

Thanks @stof for your awsome answer!
One more question, couldn't we considere the user as a source of attributes like ACL or OAuth? with a function $user->getAttributes()? So getRoles() can be a subset of getAttributes() that only returns attributes prefixed with ROLE_.

[stof]

@yguedidi but there is not $token->getAttributes() (there is $token->getRoles() though). There is no source of attributes in the code (my wording was confusing in my previous comment though). Permission attributes are not something which is listed in the token. It is something which is supported by a security voter. The way these attributes are checked depend on the implementation.

Attributes are not even checked for a user, but for a token and an (optional) object being checked.
For instance, the OAuth scopes don't depend on the user, but on the access token (a real verification will generally involve both a scope check and another permission check to know whether the user can perform the action). ACL permissions depend on both the user and the object. And IS_AUTHENTICATED_* attributes depend only on the token itself.
So it makes strictly no sense to rename $user-getRoles() to $user->getAttributes(). You would still have 2 different concepts, but it would become even worse as you would given them the same name for separate things

[yguedidi]

@stof sorry but there is a $token->getAttributes()
I don't say that we should remove the "roles feature", but we can make things more generic using attributes.
The token can "grab" attributes from the user, we then can have STATE_WORKING, STATE_VACATION (those are just exemples) attributes about a user, not only roles.

• Roles are attributes prefixed with ROLE_ (even the voter is ready for that (by extracting only attributes that are roles)
• IS_AUTHENTICATED_* can be attributes with a hierarchy (like the current one with roles) and always add *FULLY attribute to token somewhere except it's an anonymous or rememberme one.
• I don't know about ACL before but I made a quick look now to be able to answer you "Ô grand @stof", so I see security identities are also based on current roles (ref), so why not on attributes? As I said, I don't know about ACL before now, so I think it's sure there are things I forgot.

That's just an idea. Then we can talk about roles as special attributes. Maybe we could move roles feature to security bundle and have only attributes in the component? I say again that a talk about 3.0, even if I know that it's not a reason for big BC breaks

[stof]

@yguedidi but $token->getAttributes is actually a different thing

[yguedidi]

@stof ah, do I misunderstand something?

[yguedidi]

ping @stof

[stof]

@yguedidi $token->getAttributes() is related to other data you want to store in the token, which will be kept as long as you are logged in with this token (I'm not sure it is used much in core btw). They are not attributes checked by the authorization system.

[yguedidi]

Thank you @stof.
As the VoterInterface has a supportAttributes() function, I think it mean that it vote on token attributes.
The code itself is confusing by mixin roles and attributes, so the doc is confusing too, that's why I propose to treat roles as attributes (like the code does in some parts).

[stof]

@yguedidi this method is meant to check whether the voter supports the permission attribute being voted one. It has nothing to do with token attributes (the authorization part of the security component deals with permission attributes).
But to be exact, Symfony never calls the support* methods of the voters. They are left-overs of the initial implementation of the Security component, and we figured they were useless in the interface only far too late to remove them (because of our BC policy)

[yguedidi]

@stof I know that those methods are not used and will be removed, sorry I forgot to mension that.
So if I understand you correctly, token attributes are like "custom data" managed by the developer to add data to the current token and permission attributes are roles, IS_AUTHENTICATED_* and ACLs, used by authorization?

[stof]

@yguedidi yeah, right. Permission attributes are what you pass as first argument to isGranted()

[yguedidi]

@stof OK thank you.
So my proposal is to make things more generic by treating roles and others like attributes, and vote on attributes. I think this will make thing more clean and avoid the current confusion between roles and attributes. Is there any chance for this proposal to be voted by deciders? If yes I'll open a discussion issue in symfony's repo. Thanks again.
PS: about current token attributes, we still can add getData(), setData() methods for that feature.

[stof]

So my proposal is to make things more generic by treating roles and others like attributes, and vote on attributes.

I don't understand your proposal. We already vote on attributes. And this is precisely the reason of the confusion in the doc, because the doc considers that everything we vote on is a role, while we can vote on much more thinkgs than roles.
And if you just rename the existing roles to attributes, it will not solve the confusion at all. But instead of having confusion only in the doc, you will have it in the code, because not all permission attributes are things stored in the user/token, so permission attributes and roles simply cannot become the same thing without removing features from Symfony, making many supported use cases unsupported by the Security component (which will not happen).

[yguedidi]

@stof OK well. I know voters vote on attributes (isGranted() get an attribute as a parameters).
What I'm trying to explain (despite my lacks in english, sorry), is that I think the confusion comes from the code, in the token side.
I think it comes from the fact that roles have their dedicated classes and functions (Role class, getRoles(), etc..), but they shouldn't because we can vote on them (so they are attributes). In fact I think they are just a string starting with ROLE_.
That's why I propose to treat them as attributes in the token, removing their dedicated classes/functions and use the more generic ones (like $token->getAttributes()).
RoleVoter can for exemple extract attributes that start with ROLE_ to vote on with the given token, instead of using $token->getRoles().
About token custom data, I think it make more sense to have a $token->getData() or similar instead of $token->getAttributes() as you said, which is one more confusion.

because not all permission attributes are things stored in the user/token

I should miss something. Voters get a token as a parameter, so I think current session's permissions are stored as attributes in the token.

Then, voters vote on attributes. Roles are "just" attributes that start with ROLE_. Token has attributes that are the current session's permissions (which can be roles, scope, ACL, IS_* or anything else).
No more confusion.

Sorry to take you time answering to this @stof.

[stof]

@yguedidi using classes for the roles is something we are considering to remove for 3.0: symfony/symfony#8313 (comment)

And $token->getAttributes are not the authorization attributes.
And expecting authorization attributes to be stored in the token is wrong. It would require to compute all potential attributes used in the app when creating the token, and this would limit the power of voters a lot (especially for voters which are using the $object argument to vote on a specific object).
Please note that I'm always referring to these attributes as "permission attributes" or "authorization attributes" (I don't know which term would be the better) in this discussion, not as "token attributes". They are not a concept owned by the token.

[stof]

to be more precise, if we need to find an owner for the concept of attributes, it is the code performing an authorization check with isGranted. This is the place where you will write the attribute (for an expression for instance, it is the only place where the expression will appear in your code)

[yguedidi]

@stof I called them "token attributes" to differenciate them from the attributes given to isGranted() (which can be any string, even a non exist attribute, eg isGranted('FOO_ATTR')).
I agree with you that in fact they are "permission attributes", but as they are the current session's "rights", and are stored in the token, I called them "token attribute".

And expecting authorization attributes to be stored in the token is wrong. It would require to compute all potential attributes used in the app when creating the token

The token interface has a setAttribute($name, $value) so no need to compute all attributes on token creation.

[stof]

The token interface has a setAttribute($name, $value) so no need to compute all attributes on token creation.

Except that this setter is not related to permission attributes. It is related to what you suggest renaming to data in your previous comment.

I agree with you that in fact they are "permission attributes", but as they are the current session's "rights", and are stored in the token, I called them "token attribute".

Let me repeat it again, permission attributes are not stored in the token. Only roles are stored there. ACLs are stored in your database (based on the username stored in the token and the identity of the object being voted on), expressions are not stored anywhere (they are evaluating the permission dynamically), and IS_AUTHENTICATED_* attributes are related to the kind of token being passed to the voter (the checks are looking like $token instance of AnonymousToken). And for custom voters based on your business rules, the storage can be anywhere too (for instance, in a blog system, you would probably compare the current user to the author of the blog post being voted on).
The token is the storage of who you are (and how we authenticated you as this person). It is the result of the authentication system. It is not a storage of what you are allowed to do at all.

And any attempt to enforce having permission attributes being stored in the token would force removing features from the authorization system, as most of them cannot be implemented based on stuff stored in the token (to be honest, a few of them could be implemented this way if you accept them to be hundred times slower and affecting performance even when the check is not performed in the given request).

And if you expect attributes to be stored in the token but don't put them in it at creation time, when do you do it ? When voting on the attribute ? this makes the storage useless as it still mean that voters hold the complete logic they have now, and are expected to add the attribute in the token after that, thus leaking memory by storing stuff your app does not need anymore.

(which can be any string, even a non exist attribute, eg isGranted('FOO_ATTR')).

There is nothing like a non-existent attribute, given they can be anything (even something else than strings). The only concept you could have is unsupported attributes, i.e. an attribute for which all voters abstained voting. Currently, unsupported attributes are either always accepted or always rejected depending on your configuration. There is a proposal in symfony/symfony#11742 to throw an exception in this case instead, as it is generally a mistake.

I called them "token attributes" to differenciate them from the attributes given to isGranted()

I think the issue is that you are trying to introduce a third concept which does not exist in Symfony (while the current issue is related to the fact that the documentation already mixes the 2 existing concepts instead of differentiating them). These are the 2 concepts which exist:

• permission attributes (i.e. anything passed to isGranted and then voters inside it), which are coming from places using the authorization system (and then are expected to be supported by a voter)
• roles, which are attributes which can be granted to a user, and are represented by strings starting with ROLE_ (to be able to recognize them in the voter), for which a voter is provided in core. The role votter allows to support the case where the authorization only need to depend on the user being authenticated (i.e. not checking permission on a specific object and not changing the permission depending of how the user has authenticated). Roles are a special kind of permission attributes.

$token->getAttributes() is not related to the authorization system at all. It is a way for the authentication system to store attributes in the token. These are meant to be used for advanced authentication cases, not for authorization (they could be used to achieve something like the github sudo mode for instance, and may be useful to implement 2FA as well, even if I'm not sure as I haven't tried to implement 2FA in Symfony yet)

[yguedidi]

Wahou, thank you @stof, especially when you write that "token attributes" are about authentication.
I think this is the main thing that confuse me. (a token has attributes, voters vote on attributes, token is passed to voters...).
Thanks for the clarification about permission attributes storage.

Only roles are stored there
Roles are a special kind of permission attributes.

So back to my proposal, and as role classes will be deprecated in favor of a string representation, why limit things to roles?
Why not replace $token->getRoles() by for exemple $token->getPermissions() (I previously called it $token->getAttributes())?
I call a permission an attribute granted to the user/token and stored in the token. It can be a role (a string starting with ROLE_) or anything else, thanks to the recently merged AbstractVoter.
I think this will ease and open creation of application specific security voters, with custom kind of attributes.
Moreover, RoleHierarchy can become PermissionHierarchy.

[yguedidi]

@stof any thoughts about that proposal?

[stof]

@yguedidi renaming them to permissions would be confusing IMO, given that not all permissions will be stored there.
And most use cases for custom voters are not about voting on permissions based on the token roles (just use roles for that), but about voting based on business rules in your domain objects (for instance checking that the author of a blog post is allowed to edit it).
What is your use case for custom voters reproducing the behavior of the RoleVoter with a different naming convention ?

[linaori]

I ended up in here when trying to find out what the token methods did setAttributes, getAttributes and hasAttribute. As I can see, there are no default implementations within symfony and there's no documentation mentioning those specific methods.

@stof So what is their intention? What are they supposed to be used for? If they are not for Authorization, what's their Authentication intent? You mentioned they are used inside voters, but that's Authorization.

[javiereguiluz]

We agree that the Symfony Security docs need a lot of improvements. This is in our priority list ... but it's taking us a lot of time because of the massive complexity of the Security internals.

In any case, to focus all our discussions about this in one place, we've created a meta issue in #7496 and we've linked this issue from there. That's why we're closing your issue ... but only to avoid duplicated discussions. We won't forget about what you said here. Thanks!