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

Closed
wants to merge 3 commits into from

Conversation

weaverryan
Copy link
Member

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.

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
Copy link
Member Author

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
Copy link
Member Author

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.

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.
if ($token !== $this->refreshedToken) {
$shouldUpdateRoles = $token instanceof RefreshableRolesTokenInterface && $token->shouldUpdateRoles();
$session->set($this->refreshableRolesSessionKey, $shouldUpdateRoles);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 September 26, 2017 18:56
@ro0NL
Copy link
Contributor

ro0NL commented Sep 27, 2017

Dont we have a UserChecker for these kind of things?

@ThePeterMick
Copy link

ThePeterMick 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
Copy link
Member Author

@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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@weaverryan status?

@weaverryan
Copy link
Member Author

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
Copy link
Member

stof 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
Copy link
Member Author

@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
Copy link

Is there any progress in this matter?

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018
@raziel057
Copy link
Contributor

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.

@fabpot
Copy link
Member

fabpot commented Feb 3, 2020

@weaverryan Isn't it fixed by #31177?

@weaverryan
Copy link
Member Author

It’s complicated - so I’ll check in greater detail in a few hours :)

@weaverryan
Copy link
Member Author

Starting in Symfony 4.4 (thanks to #31177), if the roles on a user are modified (added or removed), the user will be logged out. That's not exactly what I was envisioning here, but that's the "safest" way for this to work.

Another way that the feature could work (and what I was attempting here) is that when the roles on a user are updated, on the next refresh, the roles in the token are updated. So, if ROLE_ADMIN were removed from a user in the database, on refresh, the user would no longer have that role (versus the current functionality where the user is logged out).

If someone is interested in that other way of making the system work, please open a pull request. I imagine that the idea behind this PR - and really the main code behind this PR - would still work. But there would need to be some way to opt-into the "refresh the roles" system... and make sure that the different roles on the 2 Users don't cause the user to be logged out. I'm not sure how to do that - but if you want the feature, you can create a PR and see if it can be figured out.

Cheers!

@weaverryan weaverryan closed this Feb 3, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@nathan-de-pachtere
Copy link

nathan-de-pachtere commented Dec 3, 2020

In case someone need to do it with Symfony 5.2 / ApiPlatform. In my case my app have 3 subscription plans with different roles. When the user subscribe to one plan I need to alter the subscription and so change the user roles. So the fact that Symfony properly logout after user changes is not good for user experience ... because you need to log in again after change.

Here is my workaround :
You manually set the isEqualTo function in order to not compare roles.

class User implements UserInterface, EquatableInterface
{
...
public function isEqualTo(UserInterface $user)
{
    if(!$user instanceof static){
        return false;
    }
    if($user->id !== $this->id){
        return false;
    }
    if($user->email != $this->email){
        return false;
    }
    if($user->password !== $this->password){
        return false;
    }
    if($user->getSalt() !== $this->getSalt()){
        return false;
    }
    return true;
}
...
}

@weaverryan Let me know if there is some security concerns about that.

@ThePeterMick
Copy link

@NathanDePachtere and with that, does it reload the roles for you? I'm using Symfony 5.1 now, with custom isEqualTo, however it never reloads updated roles.

@nathan-de-pachtere
Copy link

@ptrm04 The isEqualTo() function should be triggered when calling setRoles(['YOUR_NEW_ROLE']) on your user. So user's roles should already be updated, don't need to reload something.

It's because you change roles that Symfony trigger its logic and compare user from session with your current user.

@ThePeterMick
Copy link

ThePeterMick commented Dec 6, 2020

@NathanDePachtere the code you pasted won't reload roles automatically within symfony, it will only not log a user out when roles change. So you must be doing something else in your code if you are saying your roles get updated in the user object stored in the session. Afaik currently once the roles are set upon login they are "set in stone" until the user logs out and logs back in again.

I've created a documentation issue to update the docs to capture the recommended approach to reloading roles post-login: symfony/symfony-docs#14665

@nathan-de-pachtere
Copy link

nathan-de-pachtere commented Dec 11, 2020

@ptrm04 I'm saying that the PHP User object is up to date not the session. As you mention you need to log back in if you want your session to be recreated and update.

The only other thing I do is on my front :
As soon as the user access my front there is a check if the user is login on the API or not, so if the session is invalid he will be asked to log back in.
This check is only done when accessing so if my user do something related to his roles he is not logout for the time he stays on the page.

Don't know maybe I'm missing something but for now this is working fine for my project.

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

Successfully merging this pull request may close these issues.

None yet