[Security] Fixed AbstractToken::hasUserChanged()

[wouterj]

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #36989
License	MIT
Doc PR	-

This PR completely reverts #35944.

That PR tried to fix a BC break (ref #35941, #35509) introduced by #31177. However, this broke many authentications (ref #36989), as the User is serialized in the session (as hinted by @stof). Many applications don't include the roles property in the serialization (at least, the MakerBundle doesn't include it).

In 5.2, we should probably deprecate having different roles in token and user, which fixes the BC breaks all together.

[wouterj]

Travis failures are unrelated (failing on PHP nightly, Cache and PropertyAccess components)

[javiereguiluz]

I've tested this in the Symfony Demo app and it fixed the reported issue (symfony/demo#1121). Thanks Wouter!

[chalasr]

Ok for better reintroducing on master.

[ajgarlag]

The problem reported in #36989 should have been appeared when #31177 was merged, but it came with a bug fixed in #35944. Without that bug, all that apps broken now, should have been broken with the feature introduced in #31177.

There are at least 3 options.

1. Revert [Security/Core] Fix wrong roles comparison #35944 fixing some broken implementations that rely on the bug introduced by #21571 Comparing roles to detected that users has changed #31177, but a the cost of breaking some other systems (like mine) that rely in the fact that user roles and token roles have been always (at least since Symfony 2.3) two different things and that has been never deprecated. We may discuss if it has to change in the future in other issue.
2. Revert #21571 Comparing roles to detected that users has changed #31177 (as proposed originally in this PR). Then whe should introduce a documentation advice about implementing EquatableInterface for those systems willing to compare roles to detect if user has changed.
3. Do not revert anything, forcing those broken implementation to add EquatableInterface and/or serialize user roles to fix them.

In my opinion, current behavior is the right one, and any implementation not serializing user roles should be fixed by implementing EquatableInterface. I'm against fixing a bug introducing another one already fixed.

Maybe this deserve a longer discussion. Please does not merge this PR on weekend, and let other developers to review it during next week.

[wouterj]

We all agree that #31177 introduced a BC break when custom roles are added to the token (that do not exist in the user), no need to discuss that.

The issue is that the proposed fix (that is reverted in this PR) breaks every application (as the user's roles aren't serialized, and thus $this->user->getRoles() === []).

The BC break in #31177 can be fixed by using EquatableInterface. It is choosen to rather focus on documenting the BC break from 4.3 to 4.4, than introducing another major BC break in patch releases 4.4.9 and 5.0.9 (meaning everyone updating from 4.4.8 to 4.4.9 will have a broken application).

I agree with you that the situation 4.4, 5.0 and 5.1 are in is not ideal. Let's focus on fixing the complete state in 5.2 instead (given that 5.1's stable release is only days away, I don't think there is enough time to think about and test a better fix).

[ajgarlag]

I do not agree with you that #35944 breaks every application. In symfony/symfony there is only one User class implementing UserInterface, but it implements EquatableInterface too, so it is not affected by those BC breaks.

I agree with you that #35944 breaks some applications (including symfony/demo), that decided to use a custom User class and not to serialize user roles, but reverting #35944 will break other applications too.

There is other related problem. If you revert #35944, any discussion about having different roles in user and token will be completely biased.

[wouterj]

In symfony/symfony there is only one User class implementing UserInterface, but it implements EquatableInterface too, so it is not affected by those BC breaks.

The same is true for the original BC break, applications using Symfony's User class aren't affected by any of the mentioned PRs in this PR, but very very very few applications use this class (it's mostly only used in the in_memory user provider, which isn't a production ready user provider).

I don't think it makes sense fixing a BC break introduced in a minor release affecting a few applications (applications that customize Security tokens) by introducing a BC break in a patch release affecting much more applications (applications that customize the User class).

Reverting both PRs (as originally suggested in the PR) is also scary: Applications that are build from 4.4 or 5.0 at the start are build with the premise "user's are logged out when you remove roles". If we revert both changes these applications silently loosen their security permissions without knowing it when upgrading to 4.4.9 or 5.0.9 (there is no Symfony error).

Anyway, I'll let the core team have the final call here. Your and mine arguments are clear now, I think.

[nicolas-grekas]

any discussion about having different roles in user and token will be completely biased.

This raises the question of the next steps. What would be the plan for 5.2? Any idea? Might they influence the decision here? My current understanding is that we should merge this PR as proposed, also because we know how to deprecate things so we won't put us in a dead end, IMHO.

[nicolas-grekas]

Thank you @wouterj.

[nicolas-grekas]

Follow up welcome :)

[faizanakram99]

Token roles and User roles have been two different things right from the start, not sure removing that behaviour is a good idea. Not every role is persisted in db and hence won't be available in User roles, some roles are applied on fly to auth token based on different conditions. As an example ROLE_PREVIOUS_ADMIN, we cannot make SwitchUserToken an exception while preventing the same behaviour for custom roles

If someone wants to trigger UserHasChanged on roles change, they can implement EquatableInterface

[stof]

AFAICT, ROLE_PREVIOUS_ADMIN is deprecated as a way to check if you are currently impersonating a user. IS_IMPERSONATOR is the new way.

[ajgarlag]

I gave this another try: if some implementations are not serializing user roles, we could detect if the unserialized user instance has an empty array of roles. Then we should compare the given user roles with token roles. But if the unserialized user instance has any role, then we should compare the given user roles with unserialized user roles.

But then I discovered that the new SerializableUser test class introduced in this PR shares with symfony/demo and friendsofsymfony/user-bundle a common strategy. They do not serialize user roles, but when you call getRoles on an unserialized instance, this method returns a fixed arbitrary role. Why? I do not know, but this behavior does not allow to check if the User implementation is serializing user roles.

I think this is a flawsy implementation: if we review UserInterface it provides 4 getters: getUsername, getRoles, getPassword and getSalt and a modifier eraseCredentials. getPassword and getSalt output can be modified by a call to eraseCredentials, but getUsername and getRoles output should no vary. IMO, the only way to successfully serialize any class implementing this interface is to ensure that getUsername and getRoles output is the same after unserializing it.

Well, we are here, with a known BC break in Symfony 4.4 that in fact deprecates having different roles is user and token instances, and a lot of different (IMO flawsy) UserInterface implementations that when unserialized returns an arbitrary role. How could we fix it?

I only see an easy way: deprecate any User class not implementing EquatableInterface. I will open new RFC issue to dicuss it.