Skip to content
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

Adding a new RefreshableRolesTokenInterface to refresh security roles #24331

Open
wants to merge 3 commits into
base: 3.4
from

Conversation

@weaverryan
Copy link
Member

commented Sep 26, 2017

Q A
Branch? 3.4
Bug fix? yes, probably
New feature? no
BC breaks? minor behavior change: roles are refreshed!
Deprecations? no
Tests pass? yes
Fixed tickets #12025
License MIT
Doc PR not needed

This could also be considered a bug fix.

tl;dr; If your roles change in the database, then on next refresh, your security token's roles will now also change. The current behavior is that the token's roles do not change until logout. This PR changes that behavior.

Adding a new RefreshableReolsTokenInterface to refresh security roles
This affect session-based authentication. If your token implements this
interface, then if the roles change on your User (after refresh), then
the token's roles are updated to match. The AbstractToken implements
this interface, so in practice, most systems will automatically take
advantage of this.
@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2017

Hmm, there is a problem. It's possible that an authentication mechanism could choose to create a token by using the roles from the User... plus some additional roles. In that case, on refresh, the roles are reset back to only the roles from the User. This actually happens in SwitchUserListener (

$roles = $user->getRoles();
$roles[] = new SwitchUserRole('ROLE_PREVIOUS_ADMIN', $this->tokenStorage->getToken());
).

So this is tricky. We could make the feature more "opt-in"... but this is truly the correct behavior: token roles should refresh if they are changed on the User. It's surprising that they are not.

I can only think of one solution: if the Token roles != the User roles when the original authentication occurs, then we add some flag (in the session) that indicates that the roles should not be refreshed. Basically, if you are adding any custom roles beyond User::getRoles(), then you effectively opt out of us refreshing any of your roles.

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2017

Here's a summarized update:

  1. If your token implements a new RefreshableRolesTokenInterface and its shouldUpdateRoles() method returns true, then when the refreshed User's role change, the tokens roles will also change.

  2. AbstractToken implements RefreshableRolesTokenInterface, but returns false for shouldUpdateRoles(). So, by default, this new behavior is not applied (but see next point).

  3. All core sub-classes of AbstractToken change the return value to its shouldUpdateRoles() so that they DO opt into the new behavior. However, there is some logic to it: they opt into this new "refresh roles" behavior ONLY if the original roles set on the token === the roles on User::getRoles(). This is to avoid an edge case: if an authentication mechanism stores extra roles on the token, beyond User::getRoles(). In this case, you do not want the token's roles refreshed to equal User::getRoles(), because you will lose your extra roles.

tl;dr

Most users will now have refreshed token roles when the refreshed User's roles change. A minority is security systems - where they purposely store extra roles in their token - will not benefit from this.

I believe I've safely added this behavior everywhere that I can.

Changing behavior so that roles are only refreshed if the token requests
it

By default, AbstractToken (and its sub-classes), have a mechanism to
check if any extra tokens were added, beyond the user tokens. If
there are none, then the roles are refreshed. If there are some,
then they are not.

@weaverryan weaverryan force-pushed the weaverryan:refresh-roles-session branch from 2675664 to 87d2826 Sep 26, 2017

if ($token !== $this->refreshedToken) {
$shouldUpdateRoles = $token instanceof RefreshableRolesTokenInterface && $token->shouldUpdateRoles();
$session->set($this->refreshableRolesSessionKey, $shouldUpdateRoles);
}

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 26, 2017

Author Member

This was a tricky part. The return value from shouldUpdateRoles() is not serialized to the session (i.e. it's not in AbstractToken::serialize(). This is done for BC: we can't change serialize() & unserialize() to include this.

So, we instead store this value in a different session key. This logic only sets it if the token has changed. Basically, we want to set this session key during the original request when authentication occurred (that's when we know that shouldUpdateRoles() is accurate). By checking to see if the token changed, we're effectively accomplishing that.

This was the weirdest / hackiest part of this PR. I'm open to suggestions.

This comment has been minimized.

Copy link
@sstok

sstok Sep 28, 2017

Contributor

If your upgrading there is a big chance you already need to clear existing session data. So this change should be acceptable.

@chalasr chalasr self-requested a review Sep 26, 2017

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2017

Dont we have a UserChecker for these kind of things?

@ptrm04

This comment has been minimized.

Copy link

commented Sep 28, 2017

@weaverryan does this mean that when you are not using Doctrine to lazy/eagerly load roles via getRoles() on User object and instead some other way i.e. direct DBAL then in your refreshUser (if you implement UserProviderInterface) you'll have to ensure getRoles() returns roles as otherwise (here comes interesting bit) your roles will be purged if it returns an empty array (or "downgraded" to only ROLE_USER if you safeguard for empty collections in getRoles() implementation)?

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2017

@ptrm04 Hmm, yes, that's correct. And an interesting edge-case that I hadn't thought of. If you're (for some reason) not serializing your User's roles to the session or not refreshing them, then your roles would be lost. That's a slight BC break I hadn't thought of :/

@@ -44,6 +46,8 @@ public function __construct($user, $credentials, $providerKey, array $roles = ar
$this->providerKey = $providerKey;
parent::setAuthenticated(count($roles) > 0);
$this->setShouldUpdateRoles($user instanceof UserInterface && $user->getRoles() === $roles);

This comment has been minimized.

Copy link
@ro0NL

ro0NL Sep 29, 2017

Contributor

Is this is expected... this affects all tokens today no? Should we optin thru config instead?

This comment has been minimized.

Copy link
@weaverryan

weaverryan Oct 3, 2017

Author Member

This was my way of trying to safely opt everyone in automatically... but I don't think that's possible anymore, thanks to #24331 (comment)

So yea, I think we'll need to have a way to opt into this somehow...

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 2, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 8, 2017

@weaverryan status?

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2017

I won't have time to finish this thoughtfully for inclusion in 3.4. So, I'm changing the milestone to 4.1.

@weaverryan weaverryan modified the milestones: 3.4, 4.1 Oct 11, 2017

@stof

This comment has been minimized.

Copy link
Member

commented Oct 11, 2017

@weaverryan a clean implementation of the impersonation should not be using roles to store the old token (which requires using a custom Role object and not a string).
I actually have a working implementation of a cleaner system. But I don't know how to write a BC layer for it.

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2017

@stof I hope we will be able to make some big moves like the one you mentioned for 4.1 (and yea, BC is especially hard with security). For this issue, there are still enough edge-cases that I will need to make this new behavior opt-in.

@markwatney2016

This comment has been minimized.

Copy link

commented Feb 19, 2018

Is there any progress in this matter?

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

@raziel057

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2018

I support the tentative.

For one of my applications I'm not able to migrate to newer version of Symfony because of the logout_on_user_change option as soon as the roles of a user can be modified after authentication.

Would be great to have a way to refresh the token easily without being logout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.