Reduce the perceived complexity of the security component #21029

javiereguiluz · 2016-12-23T11:19:57Z

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.

The text was updated successfully, but these errors were encountered:

Hanmac · 2016-12-23T11:29:37Z

i think that is an interesting idea to have a (delegated) security service.
for most part its just an redirection of parameters.

only the thing with login with using the Session might be a mit more complicated but i think that should be easy doable too.
should UsernamePasswordToken be the default way, or should it be done in other ways?

linaori · 2016-12-23T13:13:10Z

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: #20748 (comment). 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.

@iltar

@iltar thanks for your remarks. As always, very interesting. I just want to add something: we're not talking here about a god-class (like, for example, RubyOnRails' ActiveRecord). We're talking about a small utility class with 10 to 15 methods.

In fact, it's the same idea such as our Framework Base Controller or Console SymfonyStyle. They provide nice shortcuts for common operations and hide the low-level details.

linaori · 2016-12-23T14:15:53Z

@javiereguiluz it can work but only if that specific class has only lazy dependencies (get run-time from the container). Otherwise, if people decide to start injecting this service (which IMO should be avoided at all costs), they will start hitting issues like #11690 again due to the SecurityContext.

javiereguiluz · 2016-12-23T14:59:56Z

@iltar you are right. This is very important. For now I'm confident because the work made by @nicolas-grekas and others to make everything lazy in Symfony will help us a lot.

sstok · 2016-12-24T11:09:58Z

Basically this is Facade pattern 😃 (not like the Facade's used in Laravel).

When this is accepted the php-doc and end-user documentation should warn against the storing of method results in the constructor. I believe this is already done for some cases, but I can't remember where.

Pierstoval · 2016-12-25T18:29:32Z

If using such class is a dependency hell, I think we should go for controller traits and/or using the base Controller class in the FrameworkBundle.
This is really important, because even with documentation, we can't make sure that blog posts about this security "god" service will make sure that the service is not injected. And this would be allowing bad practices.
Plus, injecting the whole security system in a service might mean that you are not fully aware of how security works, so the good behavior would be to propose users to inject token storage or authorization checker, or directly make permission/authentication assertions in the controller itself.

IMO, security in the controller is the best option.

ro0NL · 2016-12-26T18:19:44Z

I think indeed adding a security god object will do more bad then good. It will only get bigger. With the base controller you'd already get this utilization. Without base controller, you probably know what your'e doing already.

That would make problem 1&3 more or less a non issue. Problem 3 actually still requires $user for the salt, and im not sure it's a common task for the base controller.

Now, i'd be all up for utilization or shortcut methods within a certain API, if truly convenient. Ie. TokenStorage::isAnonymousToken, TokenStorage::getUser sound really good?

Same for making things more intuitive, ie roles and attributes, or programmatically doing certain tasks like login($user).

lyrixx · 2016-12-26T18:55:45Z

Now, i'd be all up for utilization or shortcut methods within a certain API, if truly convenient. Ie. TokenStorage::isAnonymousToken, TokenStorage::getUser sound really good?

👍 with that anyway. But I don't know if it should be done in the Token class and / or in the TokenStorage class.

ro0NL · 2016-12-26T19:03:04Z

Practically it's the most logical imo. But it doesnt work out when typehinting against interfaces. However i dont have a real problem typehinting against concrete classes in controllers (without a base class). Making this work for everbody.

isTokenA(SomeUser::class) would also be very handy btw 👍

linaori · 2016-12-27T08:21:19Z

We should not forget that most of the stuff for new developers takes place in the controller from what I can tell, but I can only base this on personal experience from IRC.

@javiereguiluz Regarding the following:

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

This should simply be:

@Security("is_granted('EDIT_PRODUCT')")

If editing a product requires the user to be an admin, this should be integrated in the voter, otherwise you get inconsistent authorization rules. "Admin" is nothing more than a detail of the identification process on which you can base your authorization, but should imo not be used directly. If you were to strip the ROLE_ prefix, you'd end up with this:

@Security("is_granted('ADMIN')")

This does not define its intention at all.

Alternative

What about a rule definition using the expression engine? This would be the easy way of writing simple voters. Instead of having to write a voter manually, this could generate them for the developer, making authorization slightly easier as the concept of voters can be hidden for now.

security:
    access_rules:
        EDIT_PRODUCT: 'identify_as("admin") or owned_by(user)'
        DELETE_PRODUCT: 'identify_as("admin")'

Syntax and options just as example to illustrate the idea.

The original idea

I would like to give some short feedback (again) on each subject after sleeping a bit on it.

Problem 1 - Low level calls

Authorization checker & Token Storage: I'm still of opinion that it's a good idea to get rid of the UserInterface, but I don't see this happen any time soon. We already have shortcuts in controllers to get the user, perhaps add one for the token if it's needed?

Current features already supported:

Getting the user: UserInterface typehint in action arguments or $this->getUser();
- It's usually the User entity that people need here, show them how to pass this along via method arguments. While this concept is bad, it will still be possible to typehint a method signature against the entity while inserting the UserInterface. This should help them in the right direction.
$this->isGranted('...'); is already available in controllers.
- When doing authorization checks within other classes, the injection of the AuthorizationCheckerInterface is required anyway, thus type-hints are available.

Problem 2 - Unintuitive behavior

Role hierarchy: This should simply be flattened and inserted into the Token.

Authentication level: e.g. IS_AUTHENTICATED_ANONYMOUSLY is not really a permission, but an authentication type. What if we simply remove this completely from the Authorization level to make it less complex?

IS_AUTHENTICATED_* should mainly be used in access_control to define a minimal authentication type.
Anonymous authentication should IMO always be on for every firewall, but access control should define the minimal level of authentication required

Removing the role(s) option from the access_control is something I'd really like to have removed as it's really confusing. Instead I'd like to use a key named (e.g.) permissions to trigger the voters instead. Additionally I think a new option should be introduced to define the minimal authentication type for this resource. This would make the firewall config easier and give less problems with anonymous: ~ being forgotten

security:
    access_control:
        # anonymous: true is the default value
        - { path: ^/login }
        - { path: ^/dashboard, anonymous: false }
        # same as @Security("is_granted('ACCESS_ADMIN_CP')") on the controller
        # having configured the permissions option could imply that the default value
        # of anonymous becomes false instead of true for this item
        - { path: ^/admin, permissions: [ACCESS_ADMIN_CP] }
        # can be anonymous, but still trigger a voter, weird edge case that _could_ be supported
        - { path: ^/preview, permissions: [PREVIEW_DRAFT], anonymous: true }

        # no longer need the following item, reducing complexity and issues:
        #- { path: ^/, roles: IS_AUTHENTICATED_FULLY }

There would be 1 problem still, that would be inside a template, granting permissions based on (e.g.) IS_AUTHENTICATED_FULLY. However, this is an authorization check, thus should imo not be a call saying "at least this authentication type". I suggest to add an easier way to check this in a voter and expression on the above suggestion regarding the access_level. I don't think it's often required to use the authentication types for authorization based on my own experience.

Problem 3 - Common tasks verbosity

Encoding (hashing! rename?) a password / login a user manually: These methods could be added to the Controller and this is probably the only location where I think it's useful.

Vinorcola · 2016-12-28T09:21:00Z

I like the way of defining "access_rules" in the security.yml file. When your application grows, if you pretty much have to write a voter for each action (because each action requires a certain access authorization), it's over killing and the security logic is split in several classes. So to simplify, you end-up checking roles because it's faster.

With the configuration, you centralize security definition in one single file: maintenance get easier and no need to create voters, we can have a single voter that handle all the definition.

However, we need a way to check that the subject is of certain type. Maybe a simple

EDIT_PRODUCT: 'subject_is(AppBunel\Entity\Product) and ( user_identified_as("admin") or subject_owned_by(user) )'

would be enough? I would prefix thing by "subject_" or "user_" to distinguish on what the control is.

ro0NL · 2016-12-28T09:26:47Z

@iltar nice one. I really like your perspective here 👍

I like the way of defining "access_rules" in the security.yml file

Yes/no. It doesnt integrate well with routing at all.

So while we're at it, perhaps #20272 and #20274 can be taken into consideration.

linaori · 2016-12-28T09:32:02Z

The access_rules is a mere brainfart, but I think it would make it easier to add proper authorization while making it less complex to do so. Details have to be discussed ofc, but supports could be added like this:

security:
    access_rules:
        VIEW_PRODUCT_LIST: identified_as('admin')
        EDIT_PRODUCT:
            supports:   subject_type_is('App\Entity\Product')
            expression: identified_as('admin') or user_owns(subject)

ro0NL · 2016-12-28T09:36:22Z

So where does request matching comes in? Should it be a firewall only thingy?

edit: wait we have access_rules now. This plays with access_control i guess?

linaori · 2016-12-28T09:37:44Z

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.

Vinorcola · 2016-12-28T09:38:08Z

@ro0NL I don't think you should match request here. You define authorizations, and then you have to add the @Security annotation in front of your actions.

ro0NL · 2016-12-28T09:40:40Z

@Security annotation

Speaking about; i dont think it's a good idea to make this a dependency if you want specific controller/action matching. We shouldnt force annotations for that..

The feature is priceless and should (imo.) be fixed by the framework, hence #20272

linaori · 2016-12-28T09:45:34Z

@ro0NL nobody forces the annotation. In your controller you can still use the voter and in the security config you can still use the access_control. Given my previous example:

/** @Security("is_granted('EDIT_PRODUCT', product)") */
public function editProduct(Product $product);

$authorizationChecker->isGranted('EDIT_PRODUCT', $product);

# with my previous proposal
security:
    access_control:
        # no subject, already the case in 3.2 
        - { path: ^/foo, permissions: [VIEW_PRODUCT_LIST] }

ro0NL · 2016-12-28T09:52:55Z

Understand. And with simple /foo it's not a true issue. With complex routing/security it's totally error prone, in terms of duplicating paths, even regexes.

- { route: foo, permission: [X, Y] }

Priceless ;-)

linaori · 2016-12-28T09:55:47Z

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 😉

Vinorcola · 2016-12-29T08:43:40Z

I really'd like a shortcut for the "login a user" part. I use this code pretty much in each project (after user registration or password change). I think we should also bring a shorcut in the controller for that.

sashaaro · 2016-12-29T08:53:53Z

I think only chortcut login method make sense. I don't sure it will be good put login method in base controller. Mostly I need use login in test and particularly in Behat tests.

linaori · 2016-12-29T08:58:24Z

A manual login might be nice for cases after registration, where previously no authentication was known, such as a registration. However, I prefer to have a token based login with a redirect, but this is quite more complex than doing a simple login.

For @Vinorcola's case, the user is logged out because the user has changed. If changing a password should not log out the user, change that particular login in your user (implement the quatableInterface)

wouterj · 2016-12-29T09:26:43Z

Please note that the manual login has been requested before (in the DX-hype period). However, in a PR implementing this it was concluded that it is impossible to make such a feature working in all cases.

stof · 2017-04-11T14:35:09Z

For the proposal about a configuration-based voter, I suggest people interested in it to start experimenting in outside the core, to see how useful it is in real projects (rather than just in theory), as was done for the Guard component.
Such feature can totally be provided by a separate bundle (except th A3E2 at the configuration would not be in the security key configuring SecurityBundle but in the semantic config of your third-party bundle). It will make it easier to evaluate the usefulness of the feature (and then, integrating this in core will be an easy work once we have the working feature in a separate bundle and we decide it makes sense for core)

Pierstoval · 2017-04-11T14:52:14Z

@stof Security is complex to teach, and teaching about voters to newcomers make them look at the security component as the most complex to use, and most of the time they have problems in understanding how voters work.

This access_rules option would just need a single new voter as there is for roles already built-in Symfony

stof · 2017-04-11T15:52:39Z

@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

Hanmac · 2017-04-12T10:20:18Z

not 100% related to this issue but to this topic

when beginning a new Symfony project and thinking about security,
i was unsure about access rules with access_control and firewalls, or done with Specific Voters.
in one case i did both for the same stuff, and maybe did go overboard because it did check for the same things.

what would be helpful in the symfony docs where some comparation or list that shows when to use the one thing and when the other. (or both)

in that special case i did use a simple_preauth and did the logic there. (auth was connected to an external service to check if some ticket is still valid)

Pierstoval · 2017-04-12T14:17:08Z

@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).

sstok · 2017-04-21T09:58:15Z

Based on this discussion I came-up with the following concept, note that interfaces are not suffixed and more decoupled then usually done the Symfony code base. But I'm open to suggestions 😉

Permission system

Inspired on suggestions from @Pierstoval (Alex Rock Ancelet).

Legend

Role: A user, access group (Moderators) or system entity.
Permission: An immutable object, representing something a Role is
allowed perform. A Permission must be granted to Role to become active.

A Permission can be seen as a validation Constraint.
Attributes: Associative array, passed to the constructor of the
Permission object.
Service: An object loaded by a PSR-11 compatible container, the Service
is expected to have an __invoke() method that receives the Permission
object as only argument.

Execution

The system works with Permissions, a Permission answers a simple yes/no
answer: 'Can Role execute Permission'.

A Permission can be a marker like IsSuperUser which always returns
true when assigned, EditNewsArticle([id=20]) where [id=20] are Attributes
assigned to the granted Permission, or IsEnabled which uses a Service to
check the Permission.

The Permission interface has only one method getName(), which is expected
to return the unique name of the Permission, this can the FQCN (with dots)
or preferable a UUID to make class renaming easier.

A Permission can implement one of the following interfaces, all which derive
from the Permission interface:

InvokablePermission: Requires a __invoke(): bool method is defined
in the implementation.
ServiceDependentPermission: Uses a Service (referenced by service-id)
to check the Permission. This usable to check if Role is allowed to
create new objects, based on certain criteria (like a parent reference).

Requires a getService(): string method is defined in the implementation.
MarkerPermission: Abstract class that implements InvokablePermission
and always returns true.
AttributedPermission: Defines the Permission has one or more
Attributes, and can be granted multiple times (with different Attributes).

This also requires the getAttributes(): array method is defined in the
implementation.

Otherwise granting the permission is done only once.

Special interfaces (advanced features):

GrantingPermission: Defines this Permission can be granted to other
Role's. And checks whether granting is valid for the secondary Role.

Unlike other permissions this always uses a Service.

Method getGrantingValidator(): string returns the granting-validator
service-id.

The validator service receives the: Permission, (current user) granting Role, receiving (secondary user) Role.

linaori · 2017-04-21T11:24:48Z

Role: A user, access group (Moderators) or system entity.

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?).

Attributes: Associative array, passed to the constructor of the
Permission object.

Attributes is extremely ambiguous. I would love to see a better name for this.

The system works with Permissions, a Permission answers a simple yes/no
answer: 'Can Role execute Permission'.

It's actually 'can Token execute Permission'.

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 think that the "Role" concept and "User" object should be removed from the current system, leaving us with:

A token that identifies the user
A token that shows which groups (or scopes?) the current user belongs to

The current Authorization (permission) system can be confusing because of some limitations, such has having only 1 subject to vote on. Sometimes it's good to know if you can do a specific action for N subjects (same object). You can always create data wrappers, but this is more complex and won't work easily with @Security. Instead of rebuilding the authorization part, I'd like to see its DX improvement, especially for new (symfony) developers.

I think a lot of power lies in annotations (but should be optional):

class BarController
{
    /**
     * @isGranted()
     * same as
     * @isGranted("BarController::fooAction", {request, object, entity})
     *
     * @isGranted("VIEW", {object, entity})
     * same as
     * @isGranted("VIEW", entity)
     * @isGranted("VIEW", object)
     */
    public function fooAction(Request $request, SomeObject $object, SomeEntity $entity);
}

// would require a small adaption in voters
- public function voteOnAttribute($attribute, $subject, TokenInterface $token)
+ public function voteOnAttribute($attribute, array $subjects, TokenInterface $token)

The security annotation is quite powerful, but writing the is_granted for every single one is cumbersome and is error prone due to expressions in the annotation.

On top of that, I often see people needing to preview permissions for other users. This is impossible with the current system. If we only have a Token and no more User object, this could be done easily with a new kinda of method:

$token = // ... createTokenFor($identifier), security method
public function isGrantedFor($token, $attribute(?), $subject);

sstok · 2017-04-21T11:37:04Z

👍 Token, I couldn't come-up with a better name :) ROLE is whats used by PostgreSQL to define a User or group of users ;)

This is more a replacement for the ACL and current ROLE/attribute system, the current voting system is something we shouldn't discard as it's the most powerful.

Attributes is extremely ambiguous. I would love to see a better name for this.

This concept started out differently, as an DomainAction with attributes. So better a naming/concept is needed for this.

Pierstoval · 2017-04-21T11:39:06Z

@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)

sstok · 2017-04-21T11:57:44Z

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. Ask Can execute Permission, where permission is an object with some context information about what you want to do.

If Token was granted the Permission, they have access. A Permission can be an Attribute (IS_AUTHENTICATED_). Or something a little more advanced;

Say I want to create a FtpUser for the WebhostingAccount. I need to be either the owner of the WebhostingAccount (can be determined by a Voter) or be granted permission, currently I have no way to determine if I have permission because Roles don't have know about objects or and ACL only allows to reference existing objects. So I can't grant FtpUser::create because there is no object-id or way to determine there relationship.

By using a Permission I can grant CreateFtpUser(account=50) to the Token, so I have to check if this permission exists. Hmm, no need for a Service system? providing arguments (attributes whatever, extra information) should be enough 😄 🤦‍♂️

Pierstoval · 2017-04-21T12:01:30Z

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 🤣

linaori · 2017-04-21T12:18:16Z

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

@Pierstoval that naming is not correct. They are not permission attributes, they are an extend to identification. I mentioned groups or scopes, because they help identify. If you call them PermissionAttributes, it would imply that if I gave CAN_VIEW_CONTRACT as permission attribute, it can always view a contract, which is simply not true, this would be confusing.

sstok · 2017-04-21T12:23:23Z

@Pierstoval And it only took me 1 year to make it this simple... 🙂

Edit. Sorry for wrong mention 😇

Pierstoval · 2017-04-21T12:24:08Z

Yes, I'm probably misexplaining, but actually, token attributes would be the ones that would correspond to what we define as a permission.

If you take the example of roles, you have the app definition: role hierarchy, and the token definition: Token::getRoles().

I was more narrowing down the concept of role to a concept of permission 🙂

linaori · 2017-04-21T12:31:42Z

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).

Pierstoval · 2017-04-21T12:34:47Z

That's why merging attributes and role is necessary so roles could be just "special" attributes, managed by the Role voter 🙂

linaori · 2017-04-21T12:35:29Z

I think they should actually be completely separated 🤣

Pierstoval · 2017-04-21T12:40:44Z

When you are doing $checker->isGranted('...') , you check an attribute and a potential subject, I think this behavior should be kept as identical, but internal implementations (roles, etc.) should change.

IMO, roles are just "special attributes" managed by a voter.

In the case of my PermissionsBundle, permissions are just related to ExpressionLanguage, that's all.

Still, attributes are static properties related to a token/user, and the permission system relies on voters that are based on three things:

A token
A potential subject to secure
A list of attributes

As said, roles are just "special attributes", nothing more

stof · 2017-04-30T16:53:42Z

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).

curry684 · 2017-06-30T09:09:41Z

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 #23081 and #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.

linaori · 2017-06-30T09:23:26Z

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

curry684 · 2017-06-30T09:27:58Z

It's a good start but I personally have more issues with UserInterface defining 5 methods of which I have to leave 3 not-implemented in literally all of my applications ;) Another pet peeve of mine is that all classes and interface reference the concept of a "user" implicitly, while oft these days we're being logged in as API-keys, or microservices, or organizations, or whatever. Naming should become far more neutral in this respect, and adopt terminology like principal and identity. In fact most of my implementations of UserInterface are simply called Identity.

szymach · 2017-06-30T09:45:40Z

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.

curry684 · 2017-06-30T10:50:23Z

Yes, I think that would the ideal goal in the end. Something like an IdentityInterface exposing only basic authenticate and getDisplayName methods.

javiereguiluz · 2018-03-09T14:13:29Z

Closing because the scope of this issue is too broad and not actionable. Moreover, @iltar and others are making some improvements related to security lately, so we're already solving these problems. Thanks!

javiereguiluz added Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Security labels Dec 23, 2016

javiereguiluz mentioned this issue Dec 26, 2016

Revamped the main security article symfony/symfony-docs#7293

Closed

ro0NL mentioned this issue Dec 29, 2016

[RFC][HttpFoundation] Introduce RequestMatcher as a namespace #20274

Closed

Pierstoval mentioned this issue Jun 12, 2017

[RFC][DX][Security] Add basic access level control in firewall config #23139

Closed

javiereguiluz closed this as completed Mar 9, 2018

curry684 mentioned this issue Sep 4, 2018

UserInterface is thightly coupled with password authentication #28365

Closed

ro0NL mentioned this issue Jan 11, 2019

[Security][DX] Simplify IS_AUTHENTICATED_* attributes #29848

Closed

TomPradat mentioned this issue Apr 30, 2019

Confusing key roles in security.yaml > access_control #31328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the perceived complexity of the security component #21029

Reduce the perceived complexity of the security component #21029