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][DX] Simplify IS_AUTHENTICATED_* attributes #29848

Open
romaricdrigon opened this Issue Jan 11, 2019 · 14 comments

Comments

Projects
None yet
7 participants
@romaricdrigon
Copy link

romaricdrigon commented Jan 11, 2019

Hello,

I find IS_AUTHENTICATED_REMEMBERED / IS_AUTHENTICATED_FULLY attributes to poorly convey what they mean and what they do. This issue is here to discuss and propose a few ideas to make authentication checks more explicit and elegant.

Detailled reasoning

At the moment, to check if a User is logged to the application by any mean, we should check IS_AUTHENTICATED_REMEMBERED attribute. In my opinion the name is not intuitive, I see no reason for the name to relate to a "remember me" feature we may not have on the application. Also I don't see any reason why "fully authenticated" users inherits IS_AUTHENTICATED_REMEMBERED attribute, it makes finding users who use remember_me less elegant (see example 3).

In real-world scenarios, developers starting to develop a Symfony application are likely to use IS_AUTHENTICATED_FULLY. But it is making adding the "remember me" feature later harder, since all checks will have to be modified back to IS_AUTHENTICATED_REMEMBERED or users enabling remember me won't be able to access part of the application.

The same reasoning can be applied to IS_AUTHENTICATED_ANONYMOUSLY, I don't see any reason why fully-fledged users get this attribute.

Propositions

Here is a few ideas, some being independent from each other:

  1. add a IS_AUTHENTICATED attribute, which would be granted to any non-anonymously authenticated user, no matter how
  2. add IS_AUTHENTICATED_REMEMBER_ME attribute, which only authenticated through a remember me mechanism (cookie...) will get - and only them
  3. either keep IS_AUTHENTICATED_FULLY or rename it to IS_AUTHENTICATED_FRESH (to discuss, I find "fresh" to be more explicit)
  4. add IS_ANONYMOUS which will be given only to anonymous users
  5. deprecate IS_AUTHENTICATED_REMEMBERED and IS_AUTHENTICATED_ANONYMOUSLY

Deprecated attributes could be dropped (or not) in Symfony 5, for backward-compatibility.
Regarding code, AuthenticatedVoter & related tests should be modified, it looks to be relatively easy.

Code examples

Checking an user is connected:

// Before - not having "remember_me" enabled
$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED_FULLY`)

// Also before - eventually having "remember_me" in the app
$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED_REMEMBERED`)

// After
$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED`)

Checking an user connected during the same session:

// Before
$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED_FULLY`)

// After (tbd)
$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED_FULLY`)

Checking an user is connected thanks to "remember_me" feature (cookie...):

// Before
$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED_REMEMBERED`)
 && !$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED_FULLY`)

// After (tbd)
$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED_REMEMBER_ME`)

Checking for an anonymous user:

// Before
$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED_ANONYMOUSLY`) 
 && !$this->get('security.authorization_checker')->isGranted(`IS_AUTHENTICATED_REMEMBERED`)

// After
$this->get('security.authorization_checker')->isGranted(`IS_ANONYMOUS`)

Thanks for reading!

@romaricdrigon romaricdrigon changed the title [Security][DX] Add IS_AUTHENTICATED attribute [Security][DX] Simplify IS_AUTHENTICATED_* attributes Jan 11, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Jan 11, 2019

i tend to like IS_AUTHENTICATED, IS_ANONYMOUS, IS_REMEMBERED in conjunction with their IS_AUTHENTICATED_* counter parts. As such im not sure we should deprecated those (bullet 5)

Not sure about naming it IS_AUTHENTICATED (as it might imply ANONYMOUS is included), perhaps IS_NOT_ANONYMOUS is better.

👍 for giving this some thought. See also #21029 (comment) where this is actually described.

@javiereguiluz javiereguiluz added the DX label Jan 12, 2019

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Jan 13, 2019

The names correspond to their respective firewall counterparts. I am of opinion however, that this configuration is primarily duplicate of what you define in your firewall. If you define that a firewall supports anonymous authentication, it makes no sense to not have anonymous access control on the same URL. The same goes for the remember me feature. Additionally when none of those are defined, it makes no sense to have no IS_AUTHENTICATED_FULLY in your access control. Imo the "anonymous", "remember me" and "remaining authenticators" should define the access control level in the firewall, making the actual access control redundant.

Now when it comes to checking authentication outside of the access control and routes, you might be more interested in checking whether or not the user can perform a specific action, rather than checking the state of the user. If the user wants to edit a blog post, you want to check if the user can edit a blog post, not whether or not the remember me or full authentication is granted.

Another issue we have at the moment, is that this "hierarchy" is static. There's no way to change the level of authentication. The 2fa bundle for example uses a custom 4th level to indicate that a user is identified but has no access to any of the content until the token is entered, as the identification process is not yet finished. https://github.com/scheb/two-factor-bundle/blob/master/Resources/doc/index.md#the-authentication-process

I'm not sure how to solve this properly, but the fix you're proposing is merely fixing a symptom of an underlying problem in my opinion.

@raresserban

This comment has been minimized.

Copy link

raresserban commented Jan 14, 2019

Don't know about others, but I always use ROLE_USER to verify if a user is logged in (regardless if it is a remembered user or not) and IS_AUTHENTICATED_ANONYMOUSLY to allow anonymous users access to a route. For me the other IS_AUTHENTICATED_* roles seem to confusing when I just want to easily check if a user is logged in or not.

@romaricdrigon

This comment has been minimized.

Copy link
Author

romaricdrigon commented Jan 14, 2019

@raresserban I did the same a few times. It feels like a workaround, though.

@iltar if I understand correctly, part of your point is that we mix different concept (is the user authenticated, firewall level, roles, user-defined permissions...) behind the same attribute logic, right? I agree that it is not quite clean on a "conceptual" level, but I recognize it allows to simplify usage, we can use the same is_granted calls everywhere (Twig, access control, security annotations...) which is easier and handy. For complex applications developers may be better defining their own voters and rules, but for a lot of scenarii directly test IS_* or ROLE_* is enough.

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Jan 14, 2019

While it might be enough, it complicates a lot behind the scenes (dynamic role hierarchy for example). I rather see a solution that also solves this, for example:

security:
    authorization:
        EDIT:
            - { type: 'App\Entity\Post', condition: same_user(subject.user, token.user) or has_role(token, ROLE_ADMIN) }
            - { type: 'App\Entity\Reply', condition: same_user(subject.user, token.user) }
        NEW:
            - { type: 'App\Entity\Post', condition: has_role(token, ROLE_ADMIN) }
            - { type: 'App\Entity\Reply', condition: access_level('REMEMBERED') }
        VIEW: { condition: true }

Which can then be used as:

$authorizationChecker->isGranted('EDIT', $post);
$authorizationChecker->isGranted('NEW', $post);
$authorizationChecker->isGranted('VIEW', $post);

$authorizationChecker->isGranted('EDIT', $reply);
$authorizationChecker->isGranted('NEW', $reply);
$authorizationChecker->isGranted('VIEW', $reply);
@romaricdrigon

This comment has been minimized.

Copy link
Author

romaricdrigon commented Jan 14, 2019

I see, so what you propose is twofold:

  1. add an access_level mechanism to test for (firewall) access level, instead of using attributes
  2. add a security.authorization config for defining rules without going as far as implementing your own Voter

I have mixed feelings about both.
Though I agree with the idea behind 1., implementation-wise it is a lot of work (expression language to modify, Twig function, new AccessLevelChecker to use instead of AuthorizationChecker...) & a lot of broken backward-compatibility. What about keeping attributes, but adding the important nuance that is is access level attributes? Referring to my first post, we could have ACCESS_LEVEL_AUTHENTICATED (point 1), ACCESS_LEVEL_REMEMBER_ME (point 2), ACCESS_LEVEL_FRESHLY_AUTHENTICATED (point 3) and ACCESS_LEVEL_ANONYMOUS (point 4).

Regarding your second point, though configuration is easier for newcomers, I think implementing those rules in PHP, in a Voter, is better since code can be and should be unit tested.

@garak

This comment has been minimized.

Copy link
Contributor

garak commented Jan 14, 2019

@romaricdrigon it looks like you're making confusion between "logged" and "authenticated", where the latter is just telling you that user is passing for a firewall.
So, a user is considered "authenticated" (in Symfony) if passed by a firewall, no matter if successfully or not. On the other hand, a user is "not authenticated" if on an application area where no firewall is defined.

@romaricdrigon

This comment has been minimized.

Copy link
Author

romaricdrigon commented Jan 14, 2019

I get that, where did I used the wrong term? I will be happy to fix it.
I believe some of this confusion is baked "in" the Security component, since as @iltar noted, attributes can be used both to detect an user authentication (IS_) and user authorization (roles...).

@garak

This comment has been minimized.

Copy link
Contributor

garak commented Jan 14, 2019

Well, if you got that, it's not clear to me how you can propose "IS_AUTHENTICATED" for non-anonymous users (that are, indeed, authenticated)

@romaricdrigon

This comment has been minimized.

Copy link
Author

romaricdrigon commented Jan 14, 2019

I'm not sure to get you. IS_AUTHENTICATED will replace IS_AUTHENTICATED_REMEMBERED, an attribute which will be given to users who are authenticated through any firewall. So, actually, it does exactly what it says, no?

@garak

This comment has been minimized.

Copy link
Contributor

garak commented Jan 14, 2019

You proposed

add a IS_AUTHENTICATED attribute, which would be granted to any non-anonymously authenticated user, no matter how

and then

add IS_ANONYMOUS which will be given only to anonymous users

as if anonymous were not authenticated

@romaricdrigon

This comment has been minimized.

Copy link
Author

romaricdrigon commented Jan 14, 2019

Ok, now I get you. Sorry for being slow to understand.

Technically this is a correct, they get an AnonymousToken, but I wonder if exposing that implementation detail to developper is useful. I would prefer to have IS_AUTHENTICATED in line with the "common" meaning, which is non-anonymous users.
But I get you. Do you have another wording to suggest?

@garak

This comment has been minimized.

Copy link
Contributor

garak commented Jan 14, 2019

The problem here is that notion of "authenticated" used by Symfony is not the same as the common use.
Unfortunately, I'm afraid that changing that now could led to misunderstanding with old developers.
As already suggested above, the simpler way to highlight generic logged user is using "ROLE_USER" role.

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Jan 15, 2019

Being authenticated in Symfony is to have identified the user. In this particular case it means that the user is identified as Anonymous, which is an authentication level.

"As already suggested above, the simpler way to highlight generic logged user is using "ROLE_USER" role."

This is not the way to do it. You have to distinguish between being an anonymous user, a user that came in via a remember me (assuming the user is right, when displaying a name for example) or a user that is fully authenticated (the classic login).

Roles are an extension to the "identification" part and are by no means mandatory. In the Guard authentication process, the ROLE_USER is never assigned unless you specify it as such. Only the UsernamePasswordToken will require this role to consider the token to be valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment