symfony: Reduce the perceived complexity of the security component

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

I’m sure that the Security component is great … but most Symfony newcomers think that it’s an abhorrent monstrosity. Could we please discuss about solutions to reduce its perceived complexity?

Problem 1 - Low level calls

I think this is the worst problem for newcomers. Symfony forces you to make “low level” calls for all its operations.

Example: getting the current user (what’s a token storage?)

$user = $this->get('security.token_storage')->getToken()->getUser();

instead of:

$user = $this->get('security')->getUser();

// add this method too in case you need the token
// $user = $this->get('security')->getToken();

Example: checking permissions (who’s the authorization checker?)

$this->get('security.authorization_checker')->isGranted('ROLE_ADMIN');
// $this->get('security.authorization_checker')->isGranted('ROLE_ADMIN', $subject);

instead of:

$this->get('security')->isGranted('ROLE_ADMIN');
// $this->get('security')->isGranted('ROLE_ADMIN', $subject);

Problem 2 - Unintuitive behavior

Example: defining a role hierarchy simplifies the assignment of roles for users, but complicates the code a lot. You need to deal with the getReachableRoles() method, etc.

Example: the management of the user types is so confusing. Checking if a user is anonymous is usually done like this:

$this->get('security.authorization_checker')->isGranted('IS_AUTHENTICATED_ANONYMOUSLY');

First, it’s incredibly verbose. Second, it returns true also when the user is NOT strictly anonymous (for example when the user is logged in).

Why not add something like this that returns true ONLY when the user is really anonymous (not logged in, not remembered, etc.):

$this->get('security')->isAnonymous();

The anonymous handling in general is very confusing. For example, having to add the anonymous option to firewalls is always a big “WTF?” in workshops for Symfony newcomers.

Problem 3 - Common tasks verbosity

Example: encoding a password for the currently logged user

$user = $this->get('security.token_storage')->getToken()->getUser();
$password = $this->get('security.password_encoder')->encodePassword($user, 'foobar');

Instead of:

$password = $this->get('security')->encodePassword('foobar');

// in case you want to encode it for other user:
// $password = $this->get('security')->encodePassword('foobar', UserInterface $user);

Example: login a user.

$user = ...
// why does the token need the roles as the 4th arg if I pass the entire user as the 1st arg?
$token = new UsernamePasswordToken($user, $user->getPassword(), 'main', $user->getRoles());
$token->setAuthenticated(true);
$this->get('security.token_storage')->setToken($token);
$this->get('session')->set('_security_main', serialize($token));
$this->get('session')->save();

instead of:

$user = ...
$this->get('security')->login($user);
// support this also: $this->get('security')->login($user, $providerKey);

Problem 4 - Roles, attributes

The concept of “security attributes” is hard to understand for a lot of newcomers. If they were called “security permissions”, anyone could understand them even without an explanation.
Having roles separated from attributes is always confusing. Are roles attributes? Are a special type of attributes? Are something different but related? Is IS_AUTHENTICATED_ANONYMOUSLY a role or not?

We could make everything “security permissions” and forget about roles.

// Before:
@Security("has_role('ROLE_ADMIN')")
@Security("is_granted('EDIT_PRODUCT')")

// After:
@Security("is_granted('ROLE_ADMIN')")
@Security("is_granted('EDIT_PRODUCT')")
// Alternative
@Security("is_granted('ROLE_ADMIN') and is_granted('EDIT_PRODUCT')")

Maintaining the ROLE_ prefix would be a good idea for most apps, but if they were permissions, people could remove it if needed. Example: “does the user have the ADMIN permission?” instead of “does the user has the ROLE_ADMIN role?”.

Final remarks

I’m not proposing to remove the low level security calls. I’m not asking to hide Security features. I’m not asking to remove developer’s control of Security component. I’m just asking to:

Make people work less and learn less things to make the common security tasks.
Allow people to go deep into low level functions if they need to (exactly the same as today).

References

A while ago I published a proof-of-concept bundle with some of these ideas: EasySecurityBundle. Don’t use it in real applications! I don’t know if it’s safe and it’s not optimized for performance. It was just a proof of concept to see if this idea is doable.

About this issue

Original URL
State: closed
Created 8 years ago
Reactions: 73
Comments: 54 (48 by maintainers)

Most upvoted comments

One problem I see coming back, is a god-class with its dependencies. People will want to inject the security service because it can do everything they need to. This will bring back the issue of having the security context and authorization checker in 1 object, causing recursive dependencies when injected.

I would like to to add some remarks:

$user = $this->get('security.token_storage')->getToken()->getUser();

By doing this, you might get issues depending one when you call this. I see people do this in constructors and get wonky bugs that are hard to reproduce. I see people do this in classes registered as service, which then get called when no user is available.

Actually, getting the user is already easy. In your controller you can add it as argument for your method or call getUser(). Imo the user object is nothing more than holding an identifier and could easily be removed (@wouterj was working on a concept regarding this). I think that by “merging” the user and the token, it will already become slightly easier to understand what’s going on.

$this->get('security.authorization_checker')->isGranted('ROLE_ADMIN');

I think it’s a better idea stop hinting towards ROLE_*s in Symfony when it comes to security. If you really want know if a user has a role, $token->hasRole() could be a better alternative.

_People think everything they add is a role but is actually an attribute, see: https://stovepipe.systems/post/symfony-security-roles-vs-voters_

defining a role hierarchy simplifies the assignment of roles for users, but complicates the code a lot. You need to deal with the getReachableRoles() method, etc.

Role hierarchy should be flattened when creating the token: https://github.com/symfony/symfony/issues/20748#issuecomment-264787130. However, this could result in behavioural BC breaks when the hierarchy is updated when changed and the user is already logged in (?)

$this->get('security.authorization_checker')->isGranted('IS_AUTHENTICATED_ANONYMOUSLY');
$this->get('security')->isAnonymous();

There’s a subtle difference here. The first is to check whether the authorization level is at least anonymous and the latter check if the user is an anonymous user. This feature is currently not available within symfony with isGranted but can be done in another way:

// not available
$this->get('security.authorization_checker')->isGranted('ROLE_ANONYMOUS');

// available
$this->get('security.token_storage')->getToken() instanceof AnonymousToken;

Regarding the Common tasks verbosity those are really nice when done in controllers (though you shouldn’t).

This could be something to add in the Controller, mainly so people don’t start injecting this and creating a dozen of hidden dependency Hells.

linaori on Dec 23, 2016

It’s the same as in the current situations, so nothing would change:

security:
    access_control:
        - { path: ^/foo, roles: [VIEW_PRODUCT_LIST] }

But lets return to the actual topic 😉

linaori on Dec 28, 2016

I’d like to revive this discussion as 4.0 is nearing fast and that will be the end for another 2 years for invasive breaking changes. In my opinion the current authentication layer is broken and stuck in 2010 paradigms, see also https://github.com/symfony/symfony/issues/23081 and https://github.com/symfony/symfony/issues/21998. The security component as a whole isn’t terribly complex in itself, dropping ACLs was a good idea, and right now I think the access control and authorization layers are fine for the next few years.

It’s mainly the authentication itself, where Guard has already improved some things for custom authentication mechanisms, but the UserInterface and UserProviderInterface need to be dropped, refactored or upgraded to support these modern days where passwords are oft not even stored in the local application. Currently they are really too tightly coupled, yet on the other hand not flexible enough to cleanly plug in things like automatic registration on OAuth2 login.

curry684 on Jun 30, 2017

Perhaps you could introduce a very simplistic interface with only identity and active fields (debatable) and then you could add specific interfaces for specific use cases, like UserInterface, OAuthInterface on top of it? Then you would have specific providers rely on these dedicated interfaces.

szymach on Jun 30, 2017

@curry684 one quick win for the complexity would be to ditch the AdvancedUserInterface, see #23292, I’ll start working on this soon.

linaori on Jun 30, 2017

The hard thing in this discussion is that there are 2 different concepts named attribute in the Security component currently, and they have nothing in common (except being in this component):

tokens can store arbitrary attributes, which have nothing to do with the authorization layer (unless you write a custom voter relying on them). For instance, I’m using them to implement a feature similar to the SwitchUser one (and with a cleaner implementation as it does not rely on a special role object, allowing to keep roles as being strings in my project).
voters are voting on permission attributes, which are the permission you are asking on a subject. Roles are a special kind of such attributes.

I think you are not all talking about the same attributes (or maybe even mixing the concepts in the same reply).

Permission attributes are not something you want to enforce storing inside a token. ->isGranted('EDIT', $post) should not require storing EDIT in the token (which does not make any sense).

stof on Apr 30, 2017

Well the Token is only responsible for identification, nothing else. It’s just extremely confusing to have both “Attributes” - which is very ambiguous - and “Roles”, which imply some sort of groups but are abused to give access to when the token has this role. However, this does not work for the role hierarchy because it’s calculated runtime.

Regarding attributes, I think a rename is required, but not sure into what. Can be Security Attribute, Permission Attribute or even an Action Identifier (because often tightly coupled to an action in your application).

linaori on Apr 21, 2017

I took your idea, and extended a little upon it 👍 but I don’t like having to define expressions, I would rather use a class with actual PHP.

What you are talking about is a simple voter, that’s all 🤣

Pierstoval on Apr 21, 2017

@iltar

Lets get rid of roles completely. You have a token. The token identifies the user. The token can have groups it belongs to (maybe similar to oauth2 scopes?).

Groups? No, I don’t think so. Attributes is the best option to me, because attributes are the stuff that validates an action via voters. Roles are just attributes checked by a specific voter (that uses the role hierarchy), actually. Getting rid of roles is a seducing idea, but it can actually simplify a lot of common actions if you have a system where you can manipulate users’ roles.

It’s actually ‘can Token execute Permission’.

Totally agree, a Role should just continue to be an attribute.

Actually, the idea I have is that maybe roles & attributes could be merged in a specific Token::getPermissionAttributes() method.

In the current system, it would simply return the full list of user’s roles (using role hierarchy) and special attributes (like IS_AUTHENTICATED_* or ROLE_ALLOWED_TO_SWITCH for example).

This would make things much easier for voters when implementing the supports() method for example.

I’m also missing voters in your system, how will multiple parts of an application vote on a permission? I don’t really think the suggested permission system will work.

I also agree on @iltar’s point about voters.

@sstok: your proposal does not seem to keep consistency with current roles and tokens, as well as voters.

I like the idea of a Permission to be an immutable object, but in the best world, my wish (and the idea I would like to provide with my PermissionsBundle ) is to merge roles and permissions, for roles to just be a specific permission object (and if you don’t know it already, Role is an immutable object).

This would lead in a class hierarchy a bit cooler (I’m writing a diagram for this idea)

Pierstoval on Apr 21, 2017

@stof What do you think of this POC I just made? https://github.com/Orbitale/PermissionsBundle

One single voter, default variables as helpers, and an ExpressionFunctionProvider that can be customized to fit needs in terms of security (and could be replaced by core provider instead if needed, but for now I wanted to use a custom one just for the PoC in case of).

Pierstoval on Apr 12, 2017

@Pierstoval I’m aware that a single voter would implement it. My point is that you should start implementing this voter in a bundle to try the proposal and see whether the configuration-based voter is flexible enough or whether you end up writing custom voters again in your own real-world projects

stof on Apr 11, 2017

It doesn’t have request matching, it’s merely a definition of an access rule. This would be the same as writing a voter for it.

linaori on Dec 28, 2016