Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1088e32
Adds SwitchUserTokenInterface
alterphp May 3, 2020
61ecb3a
Remove useless added role (Sf 5.1 compliance)
alterphp May 4, 2020
1f598c6
Renamed interface to differenciate impersonation case of specific swi…
alterphp May 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions 6 src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
use Symfony\Component\HttpKernel\DataCollector\DataCollector;
use Symfony\Component\HttpKernel\DataCollector\LateDataCollectorInterface;
use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken;
use Symfony\Component\Security\Core\Authentication\Token\ImpersonatedUserTokenInterface;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface;
use Symfony\Component\Security\Core\Authorization\TraceableAccessDecisionManager;
use Symfony\Component\Security\Core\Authorization\Voter\TraceableVoter;
Expand Down Expand Up @@ -96,8 +96,8 @@ public function collect(Request $request, Response $response, \Throwable $except
$assignedRoles = $token->getRoleNames();

$impersonatorUser = null;
if ($token instanceof SwitchUserToken) {
$impersonatorUser = $token->getOriginalToken()->getUsername();
if ($token instanceof ImpersonatedUserTokenInterface && null !== $originalToken = $token->getOriginalToken()) {
$impersonatorUser = $originalToken->getUsername();
}

if (null !== $this->roleHierarchy) {
Expand Down
25 changes: 25 additions & 0 deletions 25 src/Symfony/Component/Security/Core/Authentication/Token/ImpersonatedUserTokenInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Security\Core\Authentication\Token;

/**
* ImpersonatedUserTokenInterface is the interface for an impersonated user token.
*
* @author Pierre-Charles Bertineau <pc.bertineau@alterphp.com>
*/
interface ImpersonatedUserTokenInterface extends TokenInterface
{
/**
* Provides original token if available.
*/
public function getOriginalToken(): ?TokenInterface;
}
7 changes: 5 additions & 2 deletions 7 src/Symfony/Component/Security/Core/Authentication/Token/SwitchUserToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*
* @author Christian Flothmann <christian.flothmann@sensiolabs.de>
*/
class SwitchUserToken extends UsernamePasswordToken
class SwitchUserToken extends UsernamePasswordToken implements ImpersonatedUserTokenInterface
{
private $originalToken;

Expand All @@ -35,7 +35,10 @@ public function __construct($user, $credentials, string $providerKey, array $rol
$this->originalToken = $originalToken;
}

public function getOriginalToken(): TokenInterface
/**
* {@inheritdoc}
*/
public function getOriginalToken(): ?TokenInterface
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

{
return $this->originalToken;
}
Expand Down
4 changes: 2 additions & 2 deletions 4 src/Symfony/Component/Security/Core/Authorization/Voter/AuthenticatedVoter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
namespace Symfony\Component\Security\Core\Authorization\Voter;

use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface;
use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
use Symfony\Component\Security\Core\Authentication\Token\ImpersonatedUserTokenInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;

/**
Expand Down Expand Up @@ -84,7 +84,7 @@ public function vote(TokenInterface $token, $subject, array $attributes)
return VoterInterface::ACCESS_GRANTED;
}

if (self::IS_IMPERSONATOR === $attribute && $token instanceof SwitchUserToken) {
if (self::IS_IMPERSONATOR === $attribute && $token instanceof ImpersonatedUserTokenInterface) {
return VoterInterface::ACCESS_GRANTED;
}
}
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.