10000 [Security/Core] Adds ImpersonatedUserTokenInterface to deal with cross-firewall impersonation by alterphp · Pull Request #36674 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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 1 commit
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
Prev Previous commit
Renamed interface to differenciate impersonation case of specific swi… 
…tch user case
  • Loading branch information
@alterphp
alterphp committed May 11, 2020
commit 1f598c642b9deefc880e08aa4bf220f1ba022b82
4 changes: 2 additions & 2 deletions 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\Componen 10000 t\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\SwitchUserTokenInterface;
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,7 +96,7 @@ public function collect(Request $request, Response $response, \Throwable $except
$assignedRoles = $token->getRoleNames();

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

Expand Down
7 changes: 5 additions & 2 deletions ...cation/Token/SwitchUserTokenInterface.php → .../Token/ImpersonatedUserTokenInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@
namespace Symfony\Component\Security\Core\Authentication\Token;

/**
* SwitchUserTokenInterface is the interface for a switch user token.
* ImpersonatedUserTokenInterface is the interface for an impersonated user token.
*
* @author Pierre-Charles Bertineau <pc.bertineau@alterphp.com>
*/
interface SwitchUserTokenInterface extends TokenInterface
interface ImpersonatedUserTokenInterface extends TokenInterface
{
/**
* Provides original token if available.
*/
public function getOriginalToken(): ?TokenInterface;
}
5 changes: 4 additions & 1 deletion 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 implements SwitchUserTokenInterface
class SwitchUserToken extends UsernamePasswordToken implements ImpersonatedUserTokenInterface
{
private $originalToken;

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

/**
* {@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 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\SwitchUserTokenInterface;
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 SwitchUserTokenInterface) {
if (self::IS_IMPERSONATOR === $attribute && $token instanceof ImpersonatedUserTokenInterface) {
return VoterInterface::ACCESS_GRANTED;
}
}
Expand Down
0