From c8294c7e94bc2a8fa1d57cf9f7d80478ab9967ff Mon Sep 17 00:00:00 2001 From: Christian Scheb Date: Sat, 16 Oct 2021 21:23:50 +0200 Subject: [PATCH] Add option to automatically set trusted device cookie when remember-me is activated (#66) --- doc/configuration.rst | 1 + .../Factory/Security/TwoFactorFactory.php | 2 + .../Authenticator/TwoFactorAuthenticator.php | 14 +++- .../Http/Firewall/TwoFactorListener.php | 13 +++- .../TwoFactor/TwoFactorFirewallConfig.php | 5 ++ .../Factory/Security/TwoFactorFactoryTest.php | 3 + .../TwoFactorAuthenticatorTest.php | 29 ++++++++ .../Http/Firewall/TwoFactorListenerTest.php | 71 +++++++++++++++++-- .../TwoFactor/TwoFactorFirewallConfigTest.php | 28 ++++++++ 9 files changed, 154 insertions(+), 12 deletions(-) diff --git a/doc/configuration.rst b/doc/configuration.rst index 5c929ca9..78554a2a 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -118,6 +118,7 @@ Firewall Configuration # (supports symfony/property-access notation for nested values) trusted_parameter_name: _trusted # Name of the parameter for the trusted device option # (supports symfony/property-access notation for nested values) + remember_me_sets_trusted: false # If remember-me option should also set the trusted device cookie multi_factor: false # If ALL active two-factor methods need to be fulfilled # (multi-factor authentication) success_handler: acme.custom_success_handler # Use a custom success handler instead of the default one diff --git a/src/bundle/DependencyInjection/Factory/Security/TwoFactorFactory.php b/src/bundle/DependencyInjection/Factory/Security/TwoFactorFactory.php index 9c7290d2..d8e212c5 100644 --- a/src/bundle/DependencyInjection/Factory/Security/TwoFactorFactory.php +++ b/src/bundle/DependencyInjection/Factory/Security/TwoFactorFactory.php @@ -26,6 +26,7 @@ class TwoFactorFactory implements SecurityFactoryInterface, FirewallListenerFact public const DEFAULT_TARGET_PATH = '/'; public const DEFAULT_AUTH_CODE_PARAMETER_NAME = '_auth_code'; public const DEFAULT_TRUSTED_PARAMETER_NAME = '_trusted'; + public const DEFAULT_REMEMBER_ME_SETS_TRUSTED = false; public const DEFAULT_MULTI_FACTOR = false; public const DEFAULT_PREPARE_ON_LOGIN = false; public const DEFAULT_PREPARE_ON_ACCESS_DENIED = false; @@ -88,6 +89,7 @@ public function addConfiguration(NodeDefinition $builder): void ->scalarNode('authentication_required_handler')->defaultNull()->end() ->scalarNode('auth_code_parameter_name')->defaultValue(self::DEFAULT_AUTH_CODE_PARAMETER_NAME)->end() ->scalarNode('trusted_parameter_name')->defaultValue(self::DEFAULT_TRUSTED_PARAMETER_NAME)->end() + ->scalarNode('remember_me_sets_trusted')->defaultValue(self::DEFAULT_REMEMBER_ME_SETS_TRUSTED)->end() ->booleanNode('multi_factor')->defaultValue(self::DEFAULT_MULTI_FACTOR)->end() ->booleanNode('prepare_on_login')->defaultValue(self::DEFAULT_PREPARE_ON_LOGIN)->end() ->booleanNode('prepare_on_access_denied')->defaultValue(self::DEFAULT_PREPARE_ON_ACCESS_DENIED)->end() diff --git a/src/bundle/Security/Http/Authenticator/TwoFactorAuthenticator.php b/src/bundle/Security/Http/Authenticator/TwoFactorAuthenticator.php index 1d827392..3e4e10e4 100644 --- a/src/bundle/Security/Http/Authenticator/TwoFactorAuthenticator.php +++ b/src/bundle/Security/Http/Authenticator/TwoFactorAuthenticator.php @@ -122,15 +122,23 @@ public function authenticate(Request $request): PassportInterface $passport->addBadge(new CsrfTokenBadge($tokenId, $tokenValue)); } - if ($this->twoFactorFirewallConfig->hasTrustedDeviceParameterInRequest($request) - && class_exists(TrustedDeviceBadge::class) // Make sure the package is installed - ) { + // Make sure the trusted device package is installed + if (class_exists(TrustedDeviceBadge::class) && $this->shouldSetTrustedDevice($request, $passport)) { $passport->addBadge(new TrustedDeviceBadge()); } return $passport; } + private function shouldSetTrustedDevice(Request $request, TwoFactorPassport $passport): bool + { + return $this->twoFactorFirewallConfig->hasTrustedDeviceParameterInRequest($request) + || ( + $this->twoFactorFirewallConfig->isRememberMeSetsTrusted() + && $passport->hasBadge(RememberMeBadge::class) + ); + } + public function createAuthenticatedToken(PassportInterface $passport, string $firewallName): TokenInterface { /** @var TwoFactorPassport $passport */ diff --git a/src/bundle/Security/Http/Firewall/TwoFactorListener.php b/src/bundle/Security/Http/Firewall/TwoFactorListener.php index 939786f1..c6988905 100644 --- a/src/bundle/Security/Http/Firewall/TwoFactorListener.php +++ b/src/bundle/Security/Http/Firewall/TwoFactorListener.php @@ -188,8 +188,8 @@ private function onSuccess(Request $request, TokenInterface $token, TwoFactorTok $this->dispatchTwoFactorAuthenticationEvent(TwoFactorAuthenticationEvents::COMPLETE, $request, $token); $firewallName = $previousTwoFactorToken->getProviderKey(true); - if ($this->trustedDeviceManager - && $this->twoFactorFirewallConfig->hasTrustedDeviceParameterInRequest($request) + if ($this->trustedDeviceManager // Make sure the trusted device package is installed + && $this->shouldSetTrustedDevice($request, $previousTwoFactorToken) && $this->trustedDeviceManager->canSetTrustedDevice($token->getUser(), $request, $firewallName) ) { $this->trustedDeviceManager->addTrustedDevice($token->getUser(), $firewallName); @@ -201,6 +201,15 @@ private function onSuccess(Request $request, TokenInterface $token, TwoFactorTok return $response; } + private function shouldSetTrustedDevice(Request $request, TwoFactorTokenInterface $previousTwoFactorToken): bool + { + return $this->twoFactorFirewallConfig->hasTrustedDeviceParameterInRequest($request) + || ( + $this->twoFactorFirewallConfig->isRememberMeSetsTrusted() + && $previousTwoFactorToken->hasAttribute(TwoFactorTokenInterface::ATTRIBUTE_NAME_REMEMBER_ME_COOKIE) + ); + } + private function dispatchTwoFactorAuthenticationEvent(string $eventType, Request $request, TokenInterface $token): void { $event = new TwoFactorAuthenticationEvent($request, $token); diff --git a/src/bundle/Security/TwoFactor/TwoFactorFirewallConfig.php b/src/bundle/Security/TwoFactor/TwoFactorFirewallConfig.php index de608711..af61fdc7 100644 --- a/src/bundle/Security/TwoFactor/TwoFactorFirewallConfig.php +++ b/src/bundle/Security/TwoFactor/TwoFactorFirewallConfig.php @@ -67,6 +67,11 @@ public function getTrustedParameterName(): string return $this->options['trusted_parameter_name'] ?? TwoFactorFactory::DEFAULT_TRUSTED_PARAMETER_NAME; } + public function isRememberMeSetsTrusted(): bool + { + return $this->options['remember_me_sets_trusted'] ?? TwoFactorFactory::DEFAULT_REMEMBER_ME_SETS_TRUSTED; + } + public function isCsrfProtectionEnabled(): bool { return $this->options['enable_csrf'] ?? TwoFactorFactory::DEFAULT_ENABLE_CSRF; diff --git a/tests/DependencyInjection/Factory/Security/TwoFactorFactoryTest.php b/tests/DependencyInjection/Factory/Security/TwoFactorFactoryTest.php index ddf14df9..70ac5868 100644 --- a/tests/DependencyInjection/Factory/Security/TwoFactorFactoryTest.php +++ b/tests/DependencyInjection/Factory/Security/TwoFactorFactoryTest.php @@ -73,6 +73,7 @@ private function getFullConfig(): array authentication_required_handler: my_authentication_required_handler auth_code_parameter_name: auth_code_param_name trusted_parameter_name: trusted_param_name + remember_me_sets_trusted: true multi_factor: true prepare_on_login: true prepare_on_access_denied: true @@ -220,6 +221,7 @@ public function addConfiguration_emptyConfig_setDefaultValues(): void $this->assertNull($processedConfiguration['authentication_required_handler']); $this->assertEquals(TwoFactorFactory::DEFAULT_AUTH_CODE_PARAMETER_NAME, $processedConfiguration['auth_code_parameter_name']); $this->assertEquals(TwoFactorFactory::DEFAULT_TRUSTED_PARAMETER_NAME, $processedConfiguration['trusted_parameter_name']); + $this->assertEquals(TwoFactorFactory::DEFAULT_REMEMBER_ME_SETS_TRUSTED, $processedConfiguration['remember_me_sets_trusted']); $this->assertEquals(TwoFactorFactory::DEFAULT_MULTI_FACTOR, $processedConfiguration['multi_factor']); $this->assertEquals(TwoFactorFactory::DEFAULT_PREPARE_ON_LOGIN, $processedConfiguration['prepare_on_login']); $this->assertEquals(TwoFactorFactory::DEFAULT_PREPARE_ON_ACCESS_DENIED, $processedConfiguration['prepare_on_access_denied']); @@ -246,6 +248,7 @@ public function addConfiguration_fullConfig_setConfigValues(): void $this->assertEquals('my_authentication_required_handler', $processedConfiguration['authentication_required_handler']); $this->assertEquals('auth_code_param_name', $processedConfiguration['auth_code_parameter_name']); $this->assertEquals('trusted_param_name', $processedConfiguration['trusted_parameter_name']); + $this->assertEquals(true, $processedConfiguration['remember_me_sets_trusted']); $this->assertTrue($processedConfiguration['multi_factor']); $this->assertTrue($processedConfiguration['prepare_on_login']); $this->assertTrue($processedConfiguration['prepare_on_access_denied']); diff --git a/tests/Security/Http/Authenticator/TwoFactorAuthenticatorTest.php b/tests/Security/Http/Authenticator/TwoFactorAuthenticatorTest.php index 747eb438..081c41bd 100644 --- a/tests/Security/Http/Authenticator/TwoFactorAuthenticatorTest.php +++ b/tests/Security/Http/Authenticator/TwoFactorAuthenticatorTest.php @@ -140,6 +140,14 @@ private function stubRequestHasTrustedDeviceParameter(bool $hasTrustedDevicePara ->willReturn($hasTrustedDeviceParam); } + private function stubRememberMeSetsTrusted(bool $rememberMeSetsTrusted): void + { + $this->twoFactorFirewallConfig + ->expects($this->any()) + ->method('isRememberMeSetsTrusted') + ->willReturn($rememberMeSetsTrusted); + } + /** * @return MockObject|TwoFactorTokenInterface */ @@ -331,6 +339,7 @@ public function authenticate_csrfEnabled_csrfBadgeAdded(): void public function authenticate_trustedDeviceParameterNotSet_noTrustedDeviceBadge(): void { $this->stubRequestHasTrustedDeviceParameter(false); + $this->stubRememberMeSetsTrusted(false); $this->stubTokenStorageHasTwoFactorToken(); $returnValue = $this->authenticator->authenticate($this->request); @@ -344,6 +353,7 @@ public function authenticate_trustedDeviceParameterNotSet_noTrustedDeviceBadge() public function authenticate_trustedDeviceParameterSet_addTrustedDeviceBadge(): void { $this->stubRequestHasTrustedDeviceParameter(true); + $this->stubRememberMeSetsTrusted(false); $this->stubTokenStorageHasTwoFactorToken(); $returnValue = $this->authenticator->authenticate($this->request); @@ -351,6 +361,25 @@ public function authenticate_trustedDeviceParameterSet_addTrustedDeviceBadge(): $this->assertTrue($returnValue->hasBadge(TrustedDeviceBadge::class)); } + /** + * @test + */ + public function authenticate_rememberMeSetsTrustedWithRememberMeEnabled_addTrustedDeviceBadge(): void + { + $this->stubRequestHasTrustedDeviceParameter(false); + $this->stubRememberMeSetsTrusted(true); + $twoFactorToken = $this->stubTokenStorageHasTwoFactorToken(); + $twoFactorToken + ->expects($this->any()) + ->method('hasAttribute') + ->with(TwoFactorTokenInterface::ATTRIBUTE_NAME_USE_REMEMBER_ME) + ->willReturn(true); + + $returnValue = $this->authenticator->authenticate($this->request); + $this->assertInstanceOf(TwoFactorPassport::class, $returnValue); + $this->assertTrue($returnValue->hasBadge(TrustedDeviceBadge::class)); + } + /** * @test */ diff --git a/tests/Security/Http/Firewall/TwoFactorListenerTest.php b/tests/Security/Http/Firewall/TwoFactorListenerTest.php index 64cd772f..1e637270 100644 --- a/tests/Security/Http/Firewall/TwoFactorListenerTest.php +++ b/tests/Security/Http/Firewall/TwoFactorListenerTest.php @@ -201,6 +201,30 @@ private function createTwoFactorToken($firewallName = self::FIREWALL_NAME, $auth return $twoFactorToken; } + /** + * @return MockObject|TokenInterface + */ + private function createTwoFactorTokenWithRememberMeCookie(Cookie $rememberMeCookie): MockObject + { + $attributes = [TwoFactorTokenInterface::ATTRIBUTE_NAME_REMEMBER_ME_COOKIE => [$rememberMeCookie]]; + + return $this->createTwoFactorToken(self::FIREWALL_NAME, null, $attributes); + } + + /** + * @return MockObject|TokenInterface + */ + private function createAuthenticatedToken(string $user): MockObject + { + $authenticatedToken = $this->createMock(TokenInterface::class); + $authenticatedToken + ->expects($this->any()) + ->method('getUser') + ->willReturn($user); + + return $authenticatedToken; + } + private function stubTokenStorageHasToken(TokenInterface $token): void { $this->tokenStorage @@ -227,6 +251,14 @@ private function stubRequestHasTrustedParameter(bool $hasTrustedParam): void ->willReturn($hasTrustedParam); } + private function stubRememberMeSetsTrusted(bool $rememberMeSetsTrusted): void + { + $this->twoFactorFirewallConfig + ->expects($this->any()) + ->method('isRememberMeSetsTrusted') + ->willReturn($rememberMeSetsTrusted); + } + private function stubHandlersReturnResponse(): Response { $response = new Response(); @@ -537,15 +569,38 @@ public function authenticate_twoFactorProcessComplete_dispatchCompleteEvent(): v */ public function authenticate_twoFactorProcessCompleteWithTrustedEnabled_setTrustedDevice(): void { - $authenticatedToken = $this->createMock(TokenInterface::class); - $authenticatedToken - ->expects($this->any()) - ->method('getUser') - ->willReturn('user'); + $authenticatedToken = $this->createAuthenticatedToken('user'); $this->stubTokenStorageHasToken($this->createTwoFactorToken()); $this->stubCsrfTokenIsValid(); $this->stubRequestHasTrustedParameter(true); + $this->stubRememberMeSetsTrusted(false); + $this->stubCanSetTrustedDevice(true); + $this->stubAuthenticationManagerReturnsToken($authenticatedToken); // Not a TwoFactorToken + $this->stubHandlersReturnResponse(); + + $this->trustedDeviceManager + ->expects($this->once()) + ->method('addTrustedDevice') + ->with('user', 'firewallName'); + + $this->listener->authenticate($this->requestEvent); + } + + /** + * @test + */ + public function authenticate_twoFactorProcessCompleteWithRememberMeSetsTrusted_setTrustedDevice(): void + { + $authenticatedToken = $this->createAuthenticatedToken('user'); + + $rememberMeCookie = new Cookie('remember_me', 'value'); + $twoFactorToken = $this->createTwoFactorTokenWithRememberMeCookie($rememberMeCookie); + + $this->stubTokenStorageHasToken($twoFactorToken); + $this->stubCsrfTokenIsValid(); + $this->stubRequestHasTrustedParameter(false); + $this->stubRememberMeSetsTrusted(true); $this->stubCanSetTrustedDevice(true); $this->stubAuthenticationManagerReturnsToken($authenticatedToken); // Not a TwoFactorToken $this->stubHandlersReturnResponse(); @@ -572,6 +627,7 @@ public function authenticate_twoFactorProcessCompleteTrustedDeviceNotAllowed_not $this->stubTokenStorageHasToken($this->createTwoFactorToken()); $this->stubCsrfTokenIsValid(); $this->stubRequestHasTrustedParameter(true); + $this->stubRememberMeSetsTrusted(false); $this->stubCanSetTrustedDevice(false); $this->stubAuthenticationManagerReturnsToken($authenticatedToken); // Not a TwoFactorToken $this->stubHandlersReturnResponse(); @@ -591,6 +647,8 @@ public function authenticate_twoFactorProcessCompleteWithTrustedDisabled_notSetT $this->stubTokenStorageHasToken($this->createTwoFactorToken()); $this->stubCsrfTokenIsValid(); $this->stubRequestHasTrustedParameter(false); + $this->stubRememberMeSetsTrusted(false); + $this->stubCanSetTrustedDevice(true); $this->stubAuthenticationManagerReturnsToken($this->createMock(TokenInterface::class)); // Not a TwoFactorToken $this->stubHandlersReturnResponse(); @@ -613,8 +671,7 @@ public function authenticate_twoFactorProcessCompleteWithRememberMeEnabled_setRe ->willReturn('user'); $rememberMeCookie = new Cookie('remember_me', 'value'); - $attributes = [TwoFactorTokenInterface::ATTRIBUTE_NAME_REMEMBER_ME_COOKIE => [$rememberMeCookie]]; - $twoFactorToken = $this->createTwoFactorToken(self::FIREWALL_NAME, null, $attributes); + $twoFactorToken = $this->createTwoFactorTokenWithRememberMeCookie($rememberMeCookie); $this->stubTokenStorageHasToken($twoFactorToken); $this->stubCsrfTokenIsValid(); diff --git a/tests/Security/TwoFactor/TwoFactorFirewallConfigTest.php b/tests/Security/TwoFactor/TwoFactorFirewallConfigTest.php index 875cbd07..162bbf0e 100644 --- a/tests/Security/TwoFactor/TwoFactorFirewallConfigTest.php +++ b/tests/Security/TwoFactor/TwoFactorFirewallConfigTest.php @@ -21,6 +21,7 @@ class TwoFactorFirewallConfigTest extends TestCase 'multi_factor' => true, 'auth_code_parameter_name' => 'auth_code_param', 'trusted_parameter_name' => 'trusted_param', + 'remember_me_sets_trusted' => true, 'enable_csrf' => true, 'csrf_parameter' => 'parameter_name', 'csrf_token_id' => 'token_id', @@ -112,6 +113,33 @@ public function getTrustedParameterName_optionSet_returnThatValue(): void $this->assertEquals('trusted_param', $returnValue); } + /** + * @test + */ + public function isRememberMeSetsTrusted_optionIsNotSet_returnFalse(): void + { + $returnValue = $this->createConfig([])->isRememberMeSetsTrusted(); + $this->assertFalse($returnValue); + } + + /** + * @test + */ + public function isRememberMeSetsTrusted_optionDisabled_returnTrue(): void + { + $returnValue = $this->createConfig(['remember_me_sets_trusted' => false])->isRememberMeSetsTrusted(); + $this->assertFalse($returnValue); + } + + /** + * @test + */ + public function isRememberMeSetsTrusted_optionEnabled_returnTrue(): void + { + $returnValue = $this->createConfig(self::FULL_OPTIONS)->isRememberMeSetsTrusted(); + $this->assertTrue($returnValue); + } + /** * @test */