Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Correctly check the roles in the TokenChecker (see #1082)
Description
-----------

This fixes the issue that protected elements will be shown direct after successful login but required and not yet fullfilled 2FA authentication.

**To-Do:**

- [x] Adjust tests

Commits
-------

4cbef67 Check if token is instance of TwoFactorTokenInterface
a181726 Test method if token is a TwoFactorToken
2617628 Check roles instead of token
549220c Use the RoleVoter service
ef01e25 Fix test
2fe8a6e Use interface instead of explicit class
b70c2ba Check properly against VoterInterface
498cbe1 Merge branch '4.8' into bugfix/2fa-token-checker
cf1ab57 Rename method
  • Loading branch information
bytehead committed Feb 11, 2020
1 parent 4f2ebec commit 7824079
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 26 deletions.
1 change: 1 addition & 0 deletions core-bundle/src/Resources/config/services.yml
Expand Up @@ -576,6 +576,7 @@ services:
- '@security.token_storage'
- '@session'
- '@security.authentication.trust_resolver'
- '@security.access.simple_role_voter'
- '%contao.preview_script%'
public: true

Expand Down
13 changes: 10 additions & 3 deletions core-bundle/src/Security/Authentication/Token/TokenChecker.php
Expand Up @@ -21,6 +21,7 @@
use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
use Symfony\Component\Security\Http\FirewallMapInterface;

class TokenChecker
Expand Down Expand Up @@ -53,18 +54,24 @@ class TokenChecker
*/
private $trustResolver;

/**
* @var VoterInterface
*/
private $roleVoter;

/**
* @var string
*/
private $previewScript;

public function __construct(RequestStack $requestStack, FirewallMapInterface $firewallMap, TokenStorageInterface $tokenStorage, SessionInterface $session, AuthenticationTrustResolverInterface $trustResolver, string $previewScript = '')
public function __construct(RequestStack $requestStack, FirewallMapInterface $firewallMap, TokenStorageInterface $tokenStorage, SessionInterface $session, AuthenticationTrustResolverInterface $trustResolver, VoterInterface $roleVoter, string $previewScript = '')
{
$this->requestStack = $requestStack;
$this->firewallMap = $firewallMap;
$this->tokenStorage = $tokenStorage;
$this->session = $session;
$this->trustResolver = $trustResolver;
$this->roleVoter = $roleVoter;
$this->previewScript = $previewScript;
}

Expand All @@ -75,7 +82,7 @@ public function hasFrontendUser(): bool
{
$token = $this->getToken(self::FRONTEND_FIREWALL);

return null !== $token && $token->getUser() instanceof FrontendUser;
return null !== $token && VoterInterface::ACCESS_GRANTED === $this->roleVoter->vote($token, null, ['ROLE_MEMBER']);
}

/**
Expand All @@ -85,7 +92,7 @@ public function hasBackendUser(): bool
{
$token = $this->getToken(self::BACKEND_FIREWALL);

return null !== $token && $token->getUser() instanceof BackendUser;
return null !== $token && VoterInterface::ACCESS_GRANTED === $this->roleVoter->vote($token, null, ['ROLE_USER']);
}

/**
Expand Down
Expand Up @@ -1697,7 +1697,8 @@ public function testRegistersTheSecurityTokenChecker(): void
$this->assertSame('security.token_storage', (string) $definition->getArgument(2));
$this->assertSame('session', (string) $definition->getArgument(3));
$this->assertSame('security.authentication.trust_resolver', (string) $definition->getArgument(4));
$this->assertSame('%contao.preview_script%', (string) $definition->getArgument(5));
$this->assertSame('security.access.simple_role_voter', (string) $definition->getArgument(5));
$this->assertSame('%contao.preview_script%', (string) $definition->getArgument(6));
}

public function testRegistersTheSecurityTwoFactorAuthenticator(): void
Expand Down
Expand Up @@ -30,6 +30,7 @@
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
use Symfony\Component\Security\Core\Authorization\Voter\RoleVoter;

class TokenCheckerTest extends TestCase
{
Expand All @@ -46,10 +47,10 @@ protected function setUp(): void
/**
* @dataProvider getUserInTokenStorageData
*/
public function testChecksForUserInTokenStorageIfFirewallContextMatches(string $class, string $firewallContext): void
public function testChecksForUserInTokenStorageIfFirewallContextMatches(string $class, string $firewallContext, array $roles): void
{
$user = $this->mockUser($class);
$token = new UsernamePasswordToken($user, 'password', 'provider', ['ROLE_USER']);
$token = new UsernamePasswordToken($user, 'password', 'provider', $roles);

$session = $this->createMock(SessionInterface::class);
$session
Expand All @@ -68,36 +69,58 @@ public function testChecksForUserInTokenStorageIfFirewallContextMatches(string $
$this->mockFirewallMapWithConfigContext($firewallContext),
$tokenStorage,
$session,
$this->trustResolver
$this->trustResolver,
$this->getRoleVoter()
);

if (FrontendUser::class === $class) {
$this->assertTrue($tokenChecker->hasFrontendUser());
} else {
if (\count($roles)) {
$this->assertTrue($tokenChecker->hasFrontendUser());
} else {
$this->assertFalse($tokenChecker->hasFrontendUser());
}
} elseif (\count($roles)) {
$this->assertTrue($tokenChecker->hasBackendUser());
} else {
$this->assertFalse($tokenChecker->hasBackendUser());
}
}

public function getUserInTokenStorageData(): \Generator
{
yield [FrontendUser::class, 'contao_frontend'];
yield [BackendUser::class, 'contao_backend'];
yield [FrontendUser::class, 'contao_frontend', []];
yield [FrontendUser::class, 'contao_frontend', ['ROLE_MEMBER']];
yield [BackendUser::class, 'contao_backend', []];
yield [BackendUser::class, 'contao_backend', ['ROLE_USER']];
}

/**
* @dataProvider getUserInSessionData
*/
public function testChecksForUserInSessionIfFirewallContextDoesNotMatch(string $class, string $firewallContext): void
public function testChecksForUserInSessionIfFirewallContextDoesNotMatch(string $class, string $firewallContext, array $roles): void
{
$user = $this->mockUser($class);
$token = new UsernamePasswordToken($user, 'password', 'provider', ['ROLE_USER']);
$token = new UsernamePasswordToken($user, 'password', 'provider', $roles);

$session = $this->createMock(SessionInterface::class);
$session
->expects($this->never())
->method('isStarted')
;

$tokenStorage = $this->createMock(TokenStorageInterface::class);
$tokenStorage
->method('getToken')
->willReturn($token)
;

$tokenChecker = new TokenChecker(
$this->mockRequestStack(),
$this->mockFirewallMapWithConfigContext($firewallContext),
$this->mockTokenStorage($class),
$this->mockSessionWithToken($token),
$this->trustResolver
$tokenStorage,
$session,
$this->trustResolver,
$this->getRoleVoter()
);

if (FrontendUser::class === $class) {
Expand All @@ -109,21 +132,22 @@ public function testChecksForUserInSessionIfFirewallContextDoesNotMatch(string $

public function getUserInSessionData(): \Generator
{
yield [FrontendUser::class, 'contao_backend'];
yield [BackendUser::class, 'contao_frontend'];
yield [BackendUser::class, 'contao_backend', ['ROLE_USER']];
yield [FrontendUser::class, 'contao_frontend', ['ROLE_MEMBER']];
}

public function testReturnsTheFrontendUsername(): void
{
$user = $this->mockUser(FrontendUser::class);
$token = new UsernamePasswordToken($user, 'password', 'provider', ['ROLE_USER']);
$token = new UsernamePasswordToken($user, 'password', 'provider', ['ROLE_MEMBER']);

$tokenChecker = new TokenChecker(
$this->mockRequestStack(),
$this->mockFirewallMapWithConfigContext('contao_backend'),
$this->mockTokenStorage(FrontendUser::class),
$this->mockSessionWithToken($token),
$this->trustResolver
$this->trustResolver,
$this->getRoleVoter()
);

$this->assertSame('foobar', $tokenChecker->getFrontendUsername());
Expand All @@ -139,7 +163,8 @@ public function testReturnsTheBackendUsername(): void
$this->mockFirewallMapWithConfigContext('contao_frontend'),
$this->mockTokenStorage(BackendUser::class),
$this->mockSessionWithToken($token),
$this->trustResolver
$this->trustResolver,
$this->getRoleVoter()
);

$this->assertSame('foobar', $tokenChecker->getBackendUsername());
Expand Down Expand Up @@ -169,6 +194,7 @@ public function testChecksIfThePreviewModeIsActive(TokenInterface $token, string
$this->mockTokenStorage(BackendUser::class),
$session,
$this->trustResolver,
$this->getRoleVoter(),
'/preview.php'
);

Expand Down Expand Up @@ -202,7 +228,8 @@ public function testDoesNotReturnATokenIfTheSessionIsNotStarted(): void
$this->mockFirewallMapWithConfigContext('contao_backend'),
$this->mockTokenStorage(BackendUser::class),
$session,
$this->trustResolver
$this->trustResolver,
$this->getRoleVoter()
);

$this->assertFalse($tokenChecker->hasFrontendUser());
Expand All @@ -228,7 +255,8 @@ public function testDoesNotReturnATokenIfTheSessionKeyIsNotSet(): void
$this->mockFirewallMapWithConfigContext('contao_frontend'),
$this->mockTokenStorage(FrontendUser::class),
$session,
$this->trustResolver
$this->trustResolver,
$this->getRoleVoter()
);

$this->assertFalse($tokenChecker->hasBackendUser());
Expand Down Expand Up @@ -260,7 +288,8 @@ public function testDoesNotReturnATokenIfTheSerializedObjectIsNotAToken(): void
$this->mockFirewallMapWithConfigContext('contao_backend'),
$this->mockTokenStorage(BackendUser::class),
$session,
$this->trustResolver
$this->trustResolver,
$this->getRoleVoter()
);

$this->assertNull($tokenChecker->getFrontendUsername());
Expand All @@ -275,7 +304,8 @@ public function testDoesNotReturnATokenIfTheTokenIsNotAuthenticated(): void
$this->mockFirewallMapWithConfigContext('contao_frontend'),
$this->mockTokenStorage(FrontendUser::class),
$this->mockSessionWithToken($token),
$this->trustResolver
$this->trustResolver,
$this->getRoleVoter()
);

$this->assertNull($tokenChecker->getBackendUsername());
Expand All @@ -290,7 +320,8 @@ public function testDoesNotReturnATokenIfTheTokenIsAnonymous(): void
$this->mockFirewallMapWithConfigContext('contao_backend'),
$this->mockTokenStorage(BackendUser::class),
$this->mockSessionWithToken($token),
$this->trustResolver
$this->trustResolver,
$this->getRoleVoter()
);

$this->assertNull($tokenChecker->getFrontendUsername());
Expand Down Expand Up @@ -368,4 +399,9 @@ private function mockSessionWithToken(TokenInterface $token): SessionInterface

return $session;
}

private function getRoleVoter(): RoleVoter
{
return new RoleVoter('ROLE_');
}
}

0 comments on commit 7824079

Please sign in to comment.