[Security/Core] Adds ImpersonatedUserTokenInterface to deal with cross-firewall impersonation

[alterphp]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes/no
Deprecations?	no
Tickets	Fix #36603
License	MIT
Doc PR	symfony/symfony-docs#...

Basic switch_user brought by security (and documented in https://symfony.com/doc/current/security/impersonating_user.html) is very useful and works well when backoffice and front share the same firewall (exactly, same firewall context).

If you don't want, or can't, make them share the same firewall, you have to implement your own switch user feature. And if you do, you cannot add an additional role like ROLE_PREVIOUS_ADMIN check IS_IMPERSONATOR with existing voter because it strictly requires SwitchUserToken instance that has a mandatory $originalToken property that is not suitable in a cross-firewall context..

That's why I suggest using a new ImpersonatedUserTokenInterface that allows not to provide the original token.

[xabbuh]

If I understand #36603 correctly, you could solve the whole problem by letting your User class implement the EquatableInterface and simply ignore that specific role, couldn't you?

[alterphp]

If I understand #36603 correctly, you could solve the whole problem by letting your User class implement the EquatableInterface and simply ignore that specific role, couldn't you?

My need is to add a specific role in order to inform the user she is under impersonation mode => display something visible enough to warn her. I don't understand I could achieve by comparing an AdminUser and a FrontUser for example.

[xabbuh]

My understanding from reading #36603 was that adding the custom role led to the user being logged out because it was seen has having changed. So if that's the case you could prevent the user from being treated as changed by implementing isEqualTo() and ignore your custom role there. So you can still check for the role being present in the rest of your application code.

[alterphp]

Ok. Authentication prevents from same user change (affected roles have changed, it has been disabled, ...). It does not check if we move from one user to another user.

Let's see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php#L255

[xabbuh]

I am sorry, but I do not understand what you mean with your last comment.

[ajgarlag]

@alterphp Can you check if your original issue #36603 is fixed applying PR #35944 which fixes issue #35941?

[wouterj]

Please don't use roles to check if impersonation is the case. The ROLE_PREVIOUS_ADMIN role is deprecated in 5.1.

You should create your own voter and check for a specific token instance. See for instance the implementation of IS_IMPERSONATOR:

symfony/src/Symfony/Component/Security/Core/Authorization/Voter/AuthenticatedVoter.php

Lines 87 to 89 in e9be741

	if (self::IS_IMPERSONATOR === $attribute && $token instanceof SwitchUserToken) {
	return VoterInterface::ACCESS_GRANTED;
	}

[alterphp]

@alterphp Can you check if your original issue #36603 is fixed applying PR #35944 which fixes issue #35941?

I'm pretty sure it will make my use case work, but, as I understant it, the PR simply removes the check that authenticator is supposed to do.

[alterphp]

Please don't use roles to check if impersonation is the case. The ROLE_PREVIOUS_ADMIN role is deprecated in 5.1.

You should create your own voter and check for a specific token instance. See for instance the implementation of IS_IMPERSONATOR:

symfony/src/Symfony/Component/Security/Core/Authorization/Voter/AuthenticatedVoter.php

Lines 87 to 89 in e9be741

	if (self::IS_IMPERSONATOR === $attribute && $token instanceof SwitchUserToken) {
	return VoterInterface::ACCESS_GRANTED;
	}

The additional role is not used for authentication nor authorization, only to detect impersonation at the view level. It is a bad use ? I'm just trying to move the constraint of strictly using SwitchUserToken to a more extensible SwitchUserTokenInterface (that would not strictly require the original token).

@wouterj In a more simplier way, I'd like to use the existing AuthenticatedVoter (so removing ROLE_PREVIOUS_ADMIN and checking IS_IMPERSONATOR instead) but it strictly requires SwitchUserToken that itself requires an original token that is not suitable for cross-firewall impersonation...

[alterphp]

I've removed the part using former ROLE_PREVIOUS_ADMIN usage.

[ajgarlag]

@alterphp Can you check if your original issue #36603 is fixed applying PR #35944 which fixes issue #35941?

I'm pretty sure it will make my use case work, but, as I understant it, the PR simply removes the check that authenticator is supposed to do.

IMO the authenticator is supposed to check if the user has changed, but it is currently comparing user roles with token roles (as described in #35941).

The problem is that if you if you create a $token with an instance of an $user, and you add an additional role to the $token (not to the original $user), when you call $token->setUser($user) with the same $user instance used in constructor, the method $token->hasUserChanged($user) returns false, although you are using the same $user instance without any modification. It's a BC break introduced in 4f4c30d

[xabbuh]

I don't understand why getOriginalToken() should be able to return null. How would you end the impersonation if you don't know the original token that you need to switch back to?

[alterphp]

No need for switch back when impersonating in a different firewall. AdminUser is still logged as itself in the admin context, but also logged as impersonated user in the other firewall.

AFAIK it's impossible to access the original token (of admin context) from the front context. And I think it's useless in a cross-firewall context. Switching back is relevant in a single firewall context because authentication only handles one token at a time.

I want to check the impersonation status to display a warning, not to provide a switch back link.

[xabbuh]

My point is that the SwitchUserListener relies on the SwitchUserToken to return the original token and not null here. As soon as we allow the original token to be null people will end up in situations where that's the case and thus break the built-in feature. We shouldn't make it easier than necessary to shoot yourself in the foot.

I would rather like to see two questions answered before considering introducing this interface: Do you really need two different firewalls? And if so, why is it an actual issue to store the original token?

[alterphp]

My point is that the SwitchUserListener relies on the SwitchUserToken to return the original token and not null here. As soon as we allow the original token to be null people will end up in situations where that's the case and thus break the built-in feature. We shouldn't make it easier than necessary to shoot yourself in the foot.

IMO, SwitchUserListener should only handle existing SwitchUserToken case, not all impersonation cases. So maybe the interface would benefit being renamed to ImpersonatedTokenInterface.

Do you really need two different firewalls?

I think it's possible to merge my firewalls into one. But, from a security point of view, why providing security splitted firewalls if we need to merge them into one ? I prefer having one firewall per application context instead of relying on roles to split contexts access. IMO, roles are great for fine access definitions inside a context, not really for granting admin/extranet/front contexts.

And if so, why is it an actual issue to store the original token?

(AFAIK) It's not always possible, and a non-sense across multiple firewalls. Original token is only required for the "switch back" feature, because only one token may be taken in account in a single firewall. Within multiple firewalls context, each firewall handles its dedicated token.

[alterphp]

Anyway, thanks for your time trying to decrypt my need/intention ;-)

[alterphp]

IMO the authenticator is supposed to check if the user has changed, but it is currently comparing user roles with token roles (as described in #35941).

The problem is that if you if you create a $token with an instance of an $user, and you add an additional role to the $token (not to the original $user), when you call $token->setUser($user) with the same $user instance used in constructor, the method $token->hasUserChanged($user) returns false, although you are using the same $user instance without any modification. It's a BC break introduced in 4f4c30d

I think the check of different roles is intentional, but this should be confirmed by the core team.

[ajgarlag]

I think the check of different roles is intentional, but this should be confirmed by the core team.

It could be intentional, but it's is a BC break, so that change should provide an upgrade path triggering a deprecation error.

[fabpot]

Re-reading the whole thread, I don't think this is something that should be part of core. Closing. Thanks for proposing.