From 689af0dd09695e5d25d55c267ecb37082a55a547 Mon Sep 17 00:00:00 2001 From: Florent Morselli Date: Sun, 5 Jan 2025 20:49:23 +0100 Subject: [PATCH 01/23] [Security] OAuth2 Introspection Endpoint (RFC7662) In addition to the excellent work of @vincentchalamon #48272, this PR allows getting the data from the OAuth2 Introspection Endpoint. This endpoint is defined in the [RFC7662](https://datatracker.ietf.org/doc/html/rfc7662). It returns the following information that is used to retrieve the user: * If the access token is active * A set of claims that are similar to the OIDC one, including the `sub` or the `username`. --- AccessToken/Oidc/OidcTokenHandler.php | 144 +++++++++++++++++++------- CHANGELOG.md | 5 + 2 files changed, 114 insertions(+), 35 deletions(-) diff --git a/AccessToken/Oidc/OidcTokenHandler.php b/AccessToken/Oidc/OidcTokenHandler.php index 69e739d2..8260470c 100644 --- a/AccessToken/Oidc/OidcTokenHandler.php +++ b/AccessToken/Oidc/OidcTokenHandler.php @@ -17,9 +17,13 @@ use Jose\Component\Core\AlgorithmManager; use Jose\Component\Core\JWK; use Jose\Component\Core\JWKSet; +use Jose\Component\Encryption\JWEDecrypter; +use Jose\Component\Encryption\JWETokenSupport; +use Jose\Component\Encryption\Serializer\CompactSerializer as JweCompactSerializer; +use Jose\Component\Encryption\Serializer\JWESerializerManager; use Jose\Component\Signature\JWSTokenSupport; use Jose\Component\Signature\JWSVerifier; -use Jose\Component\Signature\Serializer\CompactSerializer; +use Jose\Component\Signature\Serializer\CompactSerializer as JwsCompactSerializer; use Jose\Component\Signature\Serializer\JWSSerializerManager; use Psr\Clock\ClockInterface; use Psr\Log\LoggerInterface; @@ -37,10 +41,13 @@ final class OidcTokenHandler implements AccessTokenHandlerInterface { use OidcTrait; + private ?JWKSet $decryptionKeyset = null; + private ?AlgorithmManager $decryptionAlgorithms = null; + private bool $enforceEncryption = false; public function __construct( private Algorithm|AlgorithmManager $signatureAlgorithm, - private JWK|JWKSet $jwkset, + private JWK|JWKSet $signatureKeyset, private string $audience, private array $issuers, private string $claim = 'sub', @@ -51,12 +58,19 @@ public function __construct( trigger_deprecation('symfony/security-http', '7.1', 'First argument must be instance of %s, %s given.', AlgorithmManager::class, Algorithm::class); $this->signatureAlgorithm = new AlgorithmManager([$signatureAlgorithm]); } - if ($jwkset instanceof JWK) { + if ($signatureKeyset instanceof JWK) { trigger_deprecation('symfony/security-http', '7.1', 'Second argument must be instance of %s, %s given.', JWKSet::class, JWK::class); - $this->jwkset = new JWKSet([$jwkset]); + $this->signatureKeyset = new JWKSet([$signatureKeyset]); } } + public function enabledJweSupport(JWKSet $decryptionKeyset, AlgorithmManager $decryptionAlgorithms, bool $enforceEncryption): void + { + $this->decryptionKeyset = $decryptionKeyset; + $this->decryptionAlgorithms = $decryptionAlgorithms; + $this->enforceEncryption = $enforceEncryption; + } + public function getUserBadgeFrom(string $accessToken): UserBadge { if (!class_exists(JWSVerifier::class) || !class_exists(Checker\HeaderCheckerManager::class)) { @@ -64,37 +78,9 @@ public function getUserBadgeFrom(string $accessToken): UserBadge } try { - // Decode the token - $jwsVerifier = new JWSVerifier($this->signatureAlgorithm); - $serializerManager = new JWSSerializerManager([new CompactSerializer()]); - $jws = $serializerManager->unserialize($accessToken); - $claims = json_decode($jws->getPayload(), true); - - // Verify the signature - if (!$jwsVerifier->verifyWithKeySet($jws, $this->jwkset, 0)) { - throw new InvalidSignatureException(); - } - - // Verify the headers - $headerCheckerManager = new Checker\HeaderCheckerManager([ - new Checker\AlgorithmChecker($this->signatureAlgorithm->list()), - ], [ - new JWSTokenSupport(), - ]); - // if this check fails, an InvalidHeaderException is thrown - $headerCheckerManager->check($jws, 0); - - // Verify the claims - $checkers = [ - new Checker\IssuedAtChecker(clock: $this->clock, allowedTimeDrift: 0, protectedHeaderOnly: false), - new Checker\NotBeforeChecker(clock: $this->clock, allowedTimeDrift: 0, protectedHeaderOnly: false), - new Checker\ExpirationTimeChecker(clock: $this->clock, allowedTimeDrift: 0, protectedHeaderOnly: false), - new Checker\AudienceChecker($this->audience), - new Checker\IssuerChecker($this->issuers), - ]; - $claimCheckerManager = new ClaimCheckerManager($checkers); - // if this check fails, an InvalidClaimException is thrown - $claimCheckerManager->check($claims); + $accessToken = $this->decryptIfNeeded($accessToken); + $claims = $this->loadAndVerifyJws($accessToken); + $this->verifyClaims($claims); if (empty($claims[$this->claim])) { throw new MissingClaimException(\sprintf('"%s" claim not found.', $this->claim)); @@ -111,4 +97,92 @@ public function getUserBadgeFrom(string $accessToken): UserBadge throw new BadCredentialsException('Invalid credentials.', $e->getCode(), $e); } } + + private function loadAndVerifyJws(string $accessToken): array + { + // Decode the token + $jwsVerifier = new JWSVerifier($this->signatureAlgorithm); + $serializerManager = new JWSSerializerManager([new JwsCompactSerializer()]); + $jws = $serializerManager->unserialize($accessToken); + + // Verify the signature + if (!$jwsVerifier->verifyWithKeySet($jws, $this->signatureKeyset, 0)) { + throw new InvalidSignatureException(); + } + + $headerCheckerManager = new Checker\HeaderCheckerManager([ + new Checker\AlgorithmChecker($this->signatureAlgorithm->list()), + ], [ + new JWSTokenSupport(), + ]); + // if this check fails, an InvalidHeaderException is thrown + $headerCheckerManager->check($jws, 0); + + return json_decode($jws->getPayload(), true); + } + + private function verifyClaims(array $claims): array + { + // Verify the claims + $checkers = [ + new Checker\IssuedAtChecker(clock: $this->clock, allowedTimeDrift: 0, protectedHeaderOnly: true), + new Checker\NotBeforeChecker(clock: $this->clock, allowedTimeDrift: 0, protectedHeaderOnly: true), + new Checker\ExpirationTimeChecker(clock: $this->clock, allowedTimeDrift: 0, protectedHeaderOnly: true), + new Checker\AudienceChecker($this->audience), + new Checker\IssuerChecker($this->issuers), + ]; + $claimCheckerManager = new ClaimCheckerManager($checkers); + + // if this check fails, an InvalidClaimException is thrown + return $claimCheckerManager->check($claims); + } + + private function decryptIfNeeded(string $accessToken): string + { + if (null === $this->decryptionKeyset || null === $this->decryptionAlgorithms) { + $this->logger?->debug('The encrypted tokens (JWE) are not supported. Skipping.'); + + return $accessToken; + } + + $jweHeaderChecker = new Checker\HeaderCheckerManager( + [ + new Checker\AlgorithmChecker($this->decryptionAlgorithms->list()), + new Checker\CallableChecker('enc', fn ($value) => \in_array($value, $this->decryptionAlgorithms->list())), + new Checker\CallableChecker('cty', fn ($value) => 'JWT' === $value), + new Checker\IssuedAtChecker(clock: $this->clock, allowedTimeDrift: 0, protectedHeaderOnly: true), + new Checker\NotBeforeChecker(clock: $this->clock, allowedTimeDrift: 0, protectedHeaderOnly: true), + new Checker\ExpirationTimeChecker(clock: $this->clock, allowedTimeDrift: 0, protectedHeaderOnly: true), + ], + [new JWETokenSupport()] + ); + $jweDecrypter = new JWEDecrypter($this->decryptionAlgorithms, null); + $serializerManager = new JWESerializerManager([new JweCompactSerializer()]); + try { + $jwe = $serializerManager->unserialize($accessToken); + $jweHeaderChecker->check($jwe, 0); + $result = $jweDecrypter->decryptUsingKeySet($jwe, $this->decryptionKeyset, 0); + if (false === $result) { + throw new \RuntimeException('The JWE could not be decrypted.'); + } + + $payload = $jwe->getPayload(); + if (null === $payload) { + throw new \RuntimeException('The JWE payload is empty.'); + } + + return $payload; + } catch (\InvalidArgumentException|\RuntimeException $e) { + if ($this->enforceEncryption) { + $this->logger?->error('An error occurred while decrypting the token.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); + throw new BadCredentialsException('Encrypted token is required.', 0, $e); + } + $this->logger?->debug('The token decryption failed. Skipping as not mandatory.'); + + return $accessToken; + } + } } diff --git a/CHANGELOG.md b/CHANGELOG.md index e9985ad1..8f6902f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,11 @@ CHANGELOG ========= +7.3 +--- + + * Add encryption support to `OidcTokenHandler` (JWE) + 7.2 --- From 09507738031ae00380d719561c27c61ac6879525 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 20 Jan 2025 10:01:54 +0100 Subject: [PATCH 02/23] [Security] Unset token roles when serializing it and user implements EquatableInterface --- Firewall/ContextListener.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Firewall/ContextListener.php b/Firewall/ContextListener.php index 11523b5e..11a1132f 100644 --- a/Firewall/ContextListener.php +++ b/Firewall/ContextListener.php @@ -301,11 +301,12 @@ private static function hasUserChanged(UserInterface $originalUser, TokenInterfa } } - $userRoles = array_map('strval', $refreshedUser->getRoles()); + $refreshedRoles = array_map('strval', $refreshedUser->getRoles()); + $originalRoles = $refreshedToken->getRoleNames(); // This comes from cloning the original token, so it still contains the roles of the original user if ( - \count($userRoles) !== \count($refreshedToken->getRoleNames()) - || \count($userRoles) !== \count(array_intersect($userRoles, $refreshedToken->getRoleNames())) + \count($refreshedRoles) !== \count($originalRoles) + || \count($refreshedRoles) !== \count(array_intersect($refreshedRoles, $originalRoles)) ) { return true; } From 21449b8b6caec800c878b10a032122873bd993cf Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Fri, 17 Jan 2025 16:58:49 +0100 Subject: [PATCH 03/23] [Security] Don't invalidate the user when the password was not stored in the session --- Firewall/ContextListener.php | 7 ++-- Tests/Firewall/AccessListenerTest.php | 6 +--- Tests/Firewall/ContextListenerTest.php | 24 ++++++++++--- Tests/Fixtures/CustomUser.php | 49 ++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 11 deletions(-) create mode 100644 Tests/Fixtures/CustomUser.php diff --git a/Firewall/ContextListener.php b/Firewall/ContextListener.php index d008363d..4a08ad0d 100644 --- a/Firewall/ContextListener.php +++ b/Firewall/ContextListener.php @@ -191,7 +191,7 @@ public function onKernelResponse(ResponseEvent $event): void * * @throws \RuntimeException */ - protected function refreshUser(TokenInterface $token): ?TokenInterface + private function refreshUser(TokenInterface $token): ?TokenInterface { $user = $token->getUser(); @@ -292,7 +292,10 @@ private static function hasUserChanged(UserInterface $originalUser, TokenInterfa } if ($originalUser instanceof PasswordAuthenticatedUserInterface || $refreshedUser instanceof PasswordAuthenticatedUserInterface) { - if (!$originalUser instanceof PasswordAuthenticatedUserInterface || !$refreshedUser instanceof PasswordAuthenticatedUserInterface || $originalUser->getPassword() !== $refreshedUser->getPassword()) { + if (!$originalUser instanceof PasswordAuthenticatedUserInterface + || !$refreshedUser instanceof PasswordAuthenticatedUserInterface + || $refreshedUser->getPassword() !== ($originalUser->getPassword() ?? $refreshedUser->getPassword()) + ) { return true; } diff --git a/Tests/Firewall/AccessListenerTest.php b/Tests/Firewall/AccessListenerTest.php index f8c0b62e..5decf414 100644 --- a/Tests/Firewall/AccessListenerTest.php +++ b/Tests/Firewall/AccessListenerTest.php @@ -42,11 +42,7 @@ public function testHandleWhenTheAccessDecisionManagerDecidesToRefuseAccess() ->willReturn([['foo' => 'bar'], null]) ; - $token = new class extends AbstractToken { - public function getCredentials(): mixed - { - } - }; + $token = new class extends AbstractToken {}; $tokenStorage = $this->createMock(TokenStorageInterface::class); $tokenStorage diff --git a/Tests/Firewall/ContextListenerTest.php b/Tests/Firewall/ContextListenerTest.php index b86d8260..b354af73 100644 --- a/Tests/Firewall/ContextListenerTest.php +++ b/Tests/Firewall/ContextListenerTest.php @@ -36,6 +36,7 @@ use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Core\User\UserProviderInterface; use Symfony\Component\Security\Http\Firewall\ContextListener; +use Symfony\Component\Security\Http\Tests\Fixtures\CustomUser; use Symfony\Component\Security\Http\Tests\Fixtures\NullUserToken; use Symfony\Contracts\Service\ServiceLocatorTrait; @@ -376,6 +377,25 @@ public function testOnKernelResponseRemoveListener() $this->assertEmpty($dispatcher->getListeners()); } + public function testRemovingPasswordFromSessionDoesntInvalidateTheToken() + { + $user = new CustomUser('user', ['ROLE_USER'], 'pass'); + + $userProvider = $this->createMock(UserProviderInterface::class); + $userProvider->expects($this->once()) + ->method('supportsClass') + ->with(CustomUser::class) + ->willReturn(true); + $userProvider->expects($this->once()) + ->method('refreshUser') + ->willReturn($user); + + $tokenStorage = $this->handleEventWithPreviousSession([$userProvider], $user); + + $this->assertInstanceOf(UsernamePasswordToken::class, $tokenStorage->getToken()); + $this->assertSame($user, $tokenStorage->getToken()->getUser()); + } + protected function runSessionOnKernelResponse($newToken, $original = null) { $session = new Session(new MockArraySessionStorage()); @@ -568,10 +588,6 @@ public function getRoleNames(): array return $this->roles; } - public function getCredentials() - { - } - public function getUser(): UserInterface { return $this->user; diff --git a/Tests/Fixtures/CustomUser.php b/Tests/Fixtures/CustomUser.php new file mode 100644 index 00000000..16afc539 --- /dev/null +++ b/Tests/Fixtures/CustomUser.php @@ -0,0 +1,49 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Tests\Fixtures; + +use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface; +use Symfony\Component\Security\Core\User\UserInterface; + +final class CustomUser implements UserInterface, PasswordAuthenticatedUserInterface +{ + public function __construct( + private string $username, + private array $roles, + private ?string $password = null, + ) { + } + + public function getUserIdentifier(): string + { + return $this->username; + } + + public function getRoles(): array + { + return $this->roles; + } + + public function getPassword(): ?string + { + return $this->password ?? null; + } + + public function eraseCredentials(): void + { + } + + public function __serialize(): array + { + return [\sprintf("\0%s\0username", self::class) => $this->username]; + } +} From ca54e553829d30f1764d7fa3bbbb73eb790f6bd8 Mon Sep 17 00:00:00 2001 From: core23 Date: Wed, 18 Sep 2024 08:00:28 +0200 Subject: [PATCH 04/23] [Security][SecurityBundle] Show user account status errors --- Authentication/AuthenticatorManager.php | 27 +- Authentication/ExposeSecurityLevel.php | 22 + CHANGELOG.md | 1 + .../AuthenticatorManagerBCTest.php | 488 ++++++++++++++++++ .../AuthenticatorManagerTest.php | 72 ++- 5 files changed, 591 insertions(+), 19 deletions(-) create mode 100644 Authentication/ExposeSecurityLevel.php create mode 100644 Tests/Authentication/AuthenticatorManagerBCTest.php diff --git a/Authentication/AuthenticatorManager.php b/Authentication/AuthenticatorManager.php index 6db5a63f..6a5f365e 100644 --- a/Authentication/AuthenticatorManager.php +++ b/Authentication/AuthenticatorManager.php @@ -46,6 +46,8 @@ */ class AuthenticatorManager implements AuthenticatorManagerInterface, UserAuthenticatorInterface { + private ExposeSecurityLevel $exposeSecurityErrors; + /** * @param iterable $authenticators */ @@ -56,9 +58,17 @@ public function __construct( private string $firewallName, private ?LoggerInterface $logger = null, private bool $eraseCredentials = true, - private bool $hideUserNotFoundExceptions = true, + ExposeSecurityLevel|bool $exposeSecurityErrors = ExposeSecurityLevel::None, private array $requiredBadges = [], ) { + if (\is_bool($exposeSecurityErrors)) { + trigger_deprecation('symfony/security-http', '7.3', 'Passing a boolean as "exposeSecurityErrors" parameter is deprecated, use %s value instead.', ExposeSecurityLevel::class); + + // The old parameter had an inverted meaning ($hideUserNotFoundExceptions), for that reason the current name does not reflect the behavior + $exposeSecurityErrors = $exposeSecurityErrors ? ExposeSecurityLevel::None : ExposeSecurityLevel::All; + } + + $this->exposeSecurityErrors = $exposeSecurityErrors; } /** @@ -250,7 +260,7 @@ private function handleAuthenticationFailure(AuthenticationException $authentica // Avoid leaking error details in case of invalid user (e.g. user not found or invalid account status) // to prevent user enumeration via response content comparison - if ($this->hideUserNotFoundExceptions && ($authenticationException instanceof UserNotFoundException || ($authenticationException instanceof AccountStatusException && !$authenticationException instanceof CustomUserMessageAccountStatusException))) { + if ($this->isSensitiveException($authenticationException)) { $authenticationException = new BadCredentialsException('Bad credentials.', 0, $authenticationException); } @@ -264,4 +274,17 @@ private function handleAuthenticationFailure(AuthenticationException $authentica // returning null is ok, it means they want the request to continue return $loginFailureEvent->getResponse(); } + + private function isSensitiveException(AuthenticationException $exception): bool + { + if (ExposeSecurityLevel::All !== $this->exposeSecurityErrors && $exception instanceof UserNotFoundException) { + return true; + } + + if (ExposeSecurityLevel::None === $this->exposeSecurityErrors && $exception instanceof AccountStatusException && !$exception instanceof CustomUserMessageAccountStatusException) { + return true; + } + + return false; + } } diff --git a/Authentication/ExposeSecurityLevel.php b/Authentication/ExposeSecurityLevel.php new file mode 100644 index 00000000..c80fd496 --- /dev/null +++ b/Authentication/ExposeSecurityLevel.php @@ -0,0 +1,22 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Authentication; + +/** + * @author Christian Gripp + */ +enum ExposeSecurityLevel: string +{ + case None = 'none'; + case AccountStatus = 'account_status'; + case All = 'all'; +} diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f6902f2..8dde7c24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ CHANGELOG --- * Add encryption support to `OidcTokenHandler` (JWE) + * Replace `$hideAccountStatusExceptions` argument with `$exposeSecurityErrors` in `AuthenticatorManager` constructor 7.2 --- diff --git a/Tests/Authentication/AuthenticatorManagerBCTest.php b/Tests/Authentication/AuthenticatorManagerBCTest.php new file mode 100644 index 00000000..9775e5a1 --- /dev/null +++ b/Tests/Authentication/AuthenticatorManagerBCTest.php @@ -0,0 +1,488 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Tests\Authentication; + +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Log\AbstractLogger; +use Psr\Log\LoggerInterface; +use Symfony\Bridge\PhpUnit\ExpectUserDeprecationMessageTrait; +use Symfony\Component\EventDispatcher\EventDispatcher; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Response; +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\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\Exception\LockedException; +use Symfony\Component\Security\Core\Exception\UserNotFoundException; +use Symfony\Component\Security\Core\User\InMemoryUser; +use Symfony\Component\Security\Http\Authentication\AuthenticatorManager; +use Symfony\Component\Security\Http\Authenticator\Debug\TraceableAuthenticator; +use Symfony\Component\Security\Http\Authenticator\InteractiveAuthenticatorInterface; +use Symfony\Component\Security\Http\Authenticator\Passport\Badge\CsrfTokenBadge; +use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; +use Symfony\Component\Security\Http\Authenticator\Passport\Credentials\PasswordCredentials; +use Symfony\Component\Security\Http\Authenticator\Passport\Passport; +use Symfony\Component\Security\Http\Authenticator\Passport\SelfValidatingPassport; +use Symfony\Component\Security\Http\Event\AuthenticationTokenCreatedEvent; +use Symfony\Component\Security\Http\Event\CheckPassportEvent; +use Symfony\Component\Security\Http\Tests\Fixtures\DummySupportsAuthenticator; + +class AuthenticatorManagerBCTest extends TestCase +{ + use ExpectUserDeprecationMessageTrait; + + private MockObject&TokenStorageInterface $tokenStorage; + private EventDispatcher $eventDispatcher; + private Request $request; + private InMemoryUser $user; + private MockObject&TokenInterface $token; + private Response $response; + + protected function setUp(): void + { + $this->tokenStorage = $this->createMock(TokenStorageInterface::class); + $this->eventDispatcher = new EventDispatcher(); + $this->request = new Request(); + $this->user = new InMemoryUser('wouter', null); + $this->token = $this->createMock(TokenInterface::class); + $this->token->expects($this->any())->method('getUser')->willReturn($this->user); + $this->response = $this->createMock(Response::class); + } + + /** + * @dataProvider provideSupportsData + * + * @group legacy + */ + public function testSupports($authenticators, $result) + { + $manager = $this->createManager($authenticators, hideUserNotFoundExceptions: true); + + $this->assertEquals($result, $manager->supports($this->request)); + } + + public static function provideSupportsData() + { + yield [[self::createDummySupportsAuthenticator(null), self::createDummySupportsAuthenticator(null)], null]; + yield [[self::createDummySupportsAuthenticator(null), self::createDummySupportsAuthenticator(false)], null]; + + yield [[self::createDummySupportsAuthenticator(null), self::createDummySupportsAuthenticator(true)], true]; + yield [[self::createDummySupportsAuthenticator(true), self::createDummySupportsAuthenticator(false)], true]; + + yield [[self::createDummySupportsAuthenticator(false), self::createDummySupportsAuthenticator(false)], false]; + yield [[], false]; + } + + /** + * @group legacy + */ + public function testSupportsInvalidAuthenticator() + { + $manager = $this->createManager([new \stdClass()], hideUserNotFoundExceptions: true); + + $this->expectExceptionObject( + new \InvalidArgumentException('Authenticator "stdClass" must implement "Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface".') + ); + + $manager->supports($this->request); + } + + /** + * @group legacy + */ + public function testSupportCheckedUponRequestAuthentication() + { + // the attribute stores the supported authenticators, returning false now + // means support changed between calling supports() and authenticateRequest() + // (which is the case with lazy firewalls) + $authenticator = $this->createAuthenticator(false); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->never())->method('authenticate'); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: true); + $manager->authenticateRequest($this->request); + } + + /** + * @dataProvider provideMatchingAuthenticatorIndex + * + * @group legacy + */ + public function testAuthenticateRequest($matchingAuthenticatorIndex) + { + $authenticators = [$this->createAuthenticator(0 === $matchingAuthenticatorIndex), $this->createAuthenticator(1 === $matchingAuthenticatorIndex)]; + $this->request->attributes->set('_security_authenticators', $authenticators); + $matchingAuthenticator = $authenticators[$matchingAuthenticatorIndex]; + + $authenticators[($matchingAuthenticatorIndex + 1) % 2]->expects($this->never())->method('authenticate'); + + $matchingAuthenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter', fn () => $this->user))); + + $listenerCalled = false; + $this->eventDispatcher->addListener(CheckPassportEvent::class, function (CheckPassportEvent $event) use (&$listenerCalled, $matchingAuthenticator) { + if ($event->getAuthenticator() === $matchingAuthenticator && $event->getPassport()->getUser() === $this->user) { + $listenerCalled = true; + } + }); + $matchingAuthenticator->expects($this->any())->method('createToken')->willReturn($this->token); + + $this->tokenStorage->expects($this->once())->method('setToken')->with($this->token); + + $manager = $this->createManager($authenticators, hideUserNotFoundExceptions: true); + $this->assertNull($manager->authenticateRequest($this->request)); + $this->assertTrue($listenerCalled, 'The CheckPassportEvent listener is not called'); + } + + public static function provideMatchingAuthenticatorIndex() + { + yield [0]; + yield [1]; + } + + /** + * @group legacy + */ + public function testNoCredentialsValidated() + { + $authenticator = $this->createAuthenticator(); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willReturn(new Passport(new UserBadge('wouter', fn () => $this->user), new PasswordCredentials('pass'))); + + $authenticator->expects($this->once()) + ->method('onAuthenticationFailure') + ->with($this->request, $this->isInstanceOf(BadCredentialsException::class)); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: true); + $manager->authenticateRequest($this->request); + } + + /** + * @group legacy + */ + public function testRequiredBadgeMissing() + { + $authenticator = $this->createAuthenticator(); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter'))); + + $authenticator->expects($this->once())->method('onAuthenticationFailure')->with($this->anything(), $this->callback(fn ($exception) => 'Authentication failed; Some badges marked as required by the firewall config are not available on the passport: "'.CsrfTokenBadge::class.'".' === $exception->getMessage())); + + $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], hideUserNotFoundExceptions: true); + $manager->authenticateRequest($this->request); + } + + /** + * @group legacy + */ + public function testAllRequiredBadgesPresent() + { + $authenticator = $this->createAuthenticator(); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $csrfBadge = new CsrfTokenBadge('csrfid', 'csrftoken'); + $csrfBadge->markResolved(); + $authenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter'), [$csrfBadge])); + $authenticator->expects($this->any())->method('createToken')->willReturn(new UsernamePasswordToken($this->user, 'main')); + + $authenticator->expects($this->once())->method('onAuthenticationSuccess'); + + $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], hideUserNotFoundExceptions: true); + $manager->authenticateRequest($this->request); + } + + /** + * @dataProvider provideEraseCredentialsData + * + * @group legacy + */ + public function testEraseCredentials($eraseCredentials) + { + $authenticator = $this->createAuthenticator(); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter', fn () => $this->user))); + + $authenticator->expects($this->any())->method('createToken')->willReturn($this->token); + + $this->token->expects($eraseCredentials ? $this->once() : $this->never())->method('eraseCredentials'); + + $manager = $this->createManager([$authenticator], 'main', $eraseCredentials, hideUserNotFoundExceptions: true); + $manager->authenticateRequest($this->request); + } + + public static function provideEraseCredentialsData() + { + yield [true]; + yield [false]; + } + + /** + * @group legacy + */ + public function testAuthenticateRequestCanModifyTokenFromEvent() + { + $authenticator = $this->createAuthenticator(); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter', fn () => $this->user))); + + $authenticator->expects($this->any())->method('createToken')->willReturn($this->token); + + $modifiedToken = $this->createMock(TokenInterface::class); + $modifiedToken->expects($this->any())->method('getUser')->willReturn($this->user); + $listenerCalled = false; + $this->eventDispatcher->addListener(AuthenticationTokenCreatedEvent::class, function (AuthenticationTokenCreatedEvent $event) use (&$listenerCalled, $modifiedToken) { + $event->setAuthenticatedToken($modifiedToken); + $listenerCalled = true; + }); + + $this->tokenStorage->expects($this->once())->method('setToken')->with($this->identicalTo($modifiedToken)); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: true); + $this->assertNull($manager->authenticateRequest($this->request)); + $this->assertTrue($listenerCalled, 'The AuthenticationTokenCreatedEvent listener is not called'); + } + + /** + * @group legacy + */ + public function testAuthenticateUser() + { + $authenticator = $this->createAuthenticator(); + $authenticator->expects($this->any())->method('onAuthenticationSuccess')->willReturn($this->response); + + $badge = new UserBadge('alex'); + + $authenticator + ->expects($this->any()) + ->method('createToken') + ->willReturnCallback(function (Passport $passport) use ($badge) { + $this->assertSame(['attr' => 'foo', 'attr2' => 'bar'], $passport->getAttributes()); + $this->assertSame([UserBadge::class => $badge], $passport->getBadges()); + + return $this->token; + }); + + $this->tokenStorage->expects($this->once())->method('setToken')->with($this->token); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: true); + $manager->authenticateUser($this->user, $authenticator, $this->request, [$badge], ['attr' => 'foo', 'attr2' => 'bar']); + } + + /** + * @group legacy + */ + public function testAuthenticateUserCanModifyTokenFromEvent() + { + $authenticator = $this->createAuthenticator(); + $authenticator->expects($this->any())->method('createToken')->willReturn($this->token); + $authenticator->expects($this->any())->method('onAuthenticationSuccess')->willReturn($this->response); + + $modifiedToken = $this->createMock(TokenInterface::class); + $modifiedToken->expects($this->any())->method('getUser')->willReturn($this->user); + $listenerCalled = false; + $this->eventDispatcher->addListener(AuthenticationTokenCreatedEvent::class, function (AuthenticationTokenCreatedEvent $event) use (&$listenerCalled, $modifiedToken) { + $event->setAuthenticatedToken($modifiedToken); + $listenerCalled = true; + }); + + $this->tokenStorage->expects($this->once())->method('setToken')->with($this->identicalTo($modifiedToken)); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: true); + $manager->authenticateUser($this->user, $authenticator, $this->request); + $this->assertTrue($listenerCalled, 'The AuthenticationTokenCreatedEvent listener is not called'); + } + + /** + * @group legacy + */ + public function testInteractiveAuthenticator() + { + $authenticator = $this->createMock(TestInteractiveBCAuthenticator::class); + $authenticator->expects($this->any())->method('isInteractive')->willReturn(true); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter', fn () => $this->user))); + $authenticator->expects($this->any())->method('createToken')->willReturn($this->token); + + $this->tokenStorage->expects($this->once())->method('setToken')->with($this->token); + + $authenticator->expects($this->any()) + ->method('onAuthenticationSuccess') + ->with($this->anything(), $this->token, 'main') + ->willReturn($this->response); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: true); + $response = $manager->authenticateRequest($this->request); + $this->assertSame($this->response, $response); + } + + /** + * @group legacy + */ + public function testLegacyInteractiveAuthenticator() + { + $authenticator = $this->createMock(InteractiveAuthenticatorInterface::class); + $authenticator->expects($this->any())->method('isInteractive')->willReturn(true); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter', fn () => $this->user))); + $authenticator->expects($this->any())->method('createToken')->willReturn($this->token); + + $this->tokenStorage->expects($this->once())->method('setToken')->with($this->token); + + $authenticator->expects($this->any()) + ->method('onAuthenticationSuccess') + ->with($this->anything(), $this->token, 'main') + ->willReturn($this->response); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: true); + $response = $manager->authenticateRequest($this->request); + $this->assertSame($this->response, $response); + } + + /** + * @group legacy + */ + public function testAuthenticateRequestHidesInvalidUserExceptions() + { + $invalidUserException = new UserNotFoundException(); + $authenticator = $this->createMock(TestInteractiveBCAuthenticator::class); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willThrowException($invalidUserException); + + $authenticator->expects($this->any()) + ->method('onAuthenticationFailure') + ->with($this->equalTo($this->request), $this->callback(fn ($e) => $e instanceof BadCredentialsException && $invalidUserException === $e->getPrevious())) + ->willReturn($this->response); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: true); + $response = $manager->authenticateRequest($this->request); + $this->assertSame($this->response, $response); + } + + /** + * @group legacy + */ + public function testAuthenticateRequestShowsAccountStatusException() + { + $invalidUserException = new LockedException(); + $authenticator = $this->createMock(TestInteractiveBCAuthenticator::class); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willThrowException($invalidUserException); + + $authenticator->expects($this->any()) + ->method('onAuthenticationFailure') + ->with($this->equalTo($this->request), $this->callback(fn ($e) => $e === $invalidUserException)) + ->willReturn($this->response); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: false); + $response = $manager->authenticateRequest($this->request); + $this->assertSame($this->response, $response); + } + + /** + * @group legacy + */ + public function testAuthenticateRequestHidesInvalidAccountStatusException() + { + $invalidUserException = new LockedException(); + $authenticator = $this->createMock(TestInteractiveBCAuthenticator::class); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willThrowException($invalidUserException); + + $authenticator->expects($this->any()) + ->method('onAuthenticationFailure') + ->with($this->equalTo($this->request), $this->callback(fn ($e) => $e instanceof BadCredentialsException && $invalidUserException === $e->getPrevious())) + ->willReturn($this->response); + + $manager = $this->createManager([$authenticator], hideUserNotFoundExceptions: true); + $response = $manager->authenticateRequest($this->request); + $this->assertSame($this->response, $response); + } + + /** + * @group legacy + */ + public function testLogsUseTheDecoratedAuthenticatorWhenItIsTraceable() + { + $authenticator = $this->createMock(TestInteractiveBCAuthenticator::class); + $authenticator->expects($this->any())->method('isInteractive')->willReturn(true); + $this->request->attributes->set('_security_authenticators', [new TraceableAuthenticator($authenticator)]); + + $authenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter', fn () => $this->user))); + $authenticator->expects($this->any())->method('createToken')->willReturn($this->token); + + $this->tokenStorage->expects($this->once())->method('setToken')->with($this->token); + + $authenticator->expects($this->any()) + ->method('onAuthenticationSuccess') + ->with($this->anything(), $this->token, 'main') + ->willReturn($this->response); + + $authenticator->expects($this->any()) + ->method('onAuthenticationSuccess') + ->with($this->anything(), $this->token, 'main') + ->willReturn($this->response); + + $logger = new class extends AbstractLogger { + public array $logContexts = []; + + public function log($level, $message, array $context = []): void + { + if ($context['authenticator'] ?? false) { + $this->logContexts[] = $context; + } + } + }; + + $manager = $this->createManager([$authenticator], 'main', true, [], $logger, hideUserNotFoundExceptions: true); + $response = $manager->authenticateRequest($this->request); + $this->assertSame($this->response, $response); + $this->assertStringContainsString($authenticator::class, $logger->logContexts[0]['authenticator']); + } + + private function createAuthenticator(?bool $supports = true) + { + $authenticator = $this->createMock(TestInteractiveBCAuthenticator::class); + $authenticator->expects($this->any())->method('supports')->willReturn($supports); + + return $authenticator; + } + + private static function createDummySupportsAuthenticator(?bool $supports = true) + { + return new DummySupportsAuthenticator($supports); + } + + private function createManager($authenticators, $firewallName = 'main', $eraseCredentials = true, array $requiredBadges = [], ?LoggerInterface $logger = null, bool $hideUserNotFoundExceptions = true) + { + $this->expectUserDeprecationMessage('Since symfony/security-http 7.3: Passing a boolean as "exposeSecurityErrors" parameter is deprecated, use Symfony\Component\Security\Http\Authentication\ExposeSecurityLevel value instead.'); + + return new AuthenticatorManager($authenticators, $this->tokenStorage, $this->eventDispatcher, $firewallName, $logger, $eraseCredentials, $hideUserNotFoundExceptions, $requiredBadges); + } +} + +abstract class TestInteractiveBCAuthenticator implements InteractiveAuthenticatorInterface +{ + public function createToken(Passport $passport, string $firewallName): TokenInterface + { + } +} diff --git a/Tests/Authentication/AuthenticatorManagerTest.php b/Tests/Authentication/AuthenticatorManagerTest.php index a6d09076..395a43b8 100644 --- a/Tests/Authentication/AuthenticatorManagerTest.php +++ b/Tests/Authentication/AuthenticatorManagerTest.php @@ -22,9 +22,11 @@ use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; use Symfony\Component\Security\Core\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\Exception\LockedException; use Symfony\Component\Security\Core\Exception\UserNotFoundException; use Symfony\Component\Security\Core\User\InMemoryUser; use Symfony\Component\Security\Http\Authentication\AuthenticatorManager; +use Symfony\Component\Security\Http\Authentication\ExposeSecurityLevel; use Symfony\Component\Security\Http\Authenticator\Debug\TraceableAuthenticator; use Symfony\Component\Security\Http\Authenticator\InteractiveAuthenticatorInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\CsrfTokenBadge; @@ -61,7 +63,7 @@ protected function setUp(): void */ public function testSupports($authenticators, $result) { - $manager = $this->createManager($authenticators); + $manager = $this->createManager($authenticators, exposeSecurityErrors: ExposeSecurityLevel::None); $this->assertEquals($result, $manager->supports($this->request)); } @@ -80,7 +82,7 @@ public static function provideSupportsData() public function testSupportsInvalidAuthenticator() { - $manager = $this->createManager([new \stdClass()]); + $manager = $this->createManager([new \stdClass()], exposeSecurityErrors: ExposeSecurityLevel::None); $this->expectExceptionObject( new \InvalidArgumentException('Authenticator "stdClass" must implement "Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface".') @@ -99,7 +101,7 @@ public function testSupportCheckedUponRequestAuthentication() $authenticator->expects($this->never())->method('authenticate'); - $manager = $this->createManager([$authenticator]); + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } @@ -126,7 +128,7 @@ public function testAuthenticateRequest($matchingAuthenticatorIndex) $this->tokenStorage->expects($this->once())->method('setToken')->with($this->token); - $manager = $this->createManager($authenticators); + $manager = $this->createManager($authenticators, exposeSecurityErrors: ExposeSecurityLevel::None); $this->assertNull($manager->authenticateRequest($this->request)); $this->assertTrue($listenerCalled, 'The CheckPassportEvent listener is not called'); } @@ -148,7 +150,7 @@ public function testNoCredentialsValidated() ->method('onAuthenticationFailure') ->with($this->request, $this->isInstanceOf(BadCredentialsException::class)); - $manager = $this->createManager([$authenticator]); + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } @@ -161,7 +163,7 @@ public function testRequiredBadgeMissing() $authenticator->expects($this->once())->method('onAuthenticationFailure')->with($this->anything(), $this->callback(fn ($exception) => 'Authentication failed; Some badges marked as required by the firewall config are not available on the passport: "'.CsrfTokenBadge::class.'".' === $exception->getMessage())); - $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class]); + $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } @@ -177,7 +179,7 @@ public function testAllRequiredBadgesPresent() $authenticator->expects($this->once())->method('onAuthenticationSuccess'); - $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class]); + $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } @@ -195,7 +197,7 @@ public function testEraseCredentials($eraseCredentials) $this->token->expects($eraseCredentials ? $this->once() : $this->never())->method('eraseCredentials'); - $manager = $this->createManager([$authenticator], 'main', $eraseCredentials); + $manager = $this->createManager([$authenticator], 'main', $eraseCredentials, exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } @@ -224,7 +226,7 @@ public function testAuthenticateRequestCanModifyTokenFromEvent() $this->tokenStorage->expects($this->once())->method('setToken')->with($this->identicalTo($modifiedToken)); - $manager = $this->createManager([$authenticator]); + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::None); $this->assertNull($manager->authenticateRequest($this->request)); $this->assertTrue($listenerCalled, 'The AuthenticationTokenCreatedEvent listener is not called'); } @@ -248,7 +250,7 @@ public function testAuthenticateUser() $this->tokenStorage->expects($this->once())->method('setToken')->with($this->token); - $manager = $this->createManager([$authenticator]); + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateUser($this->user, $authenticator, $this->request, [$badge], ['attr' => 'foo', 'attr2' => 'bar']); } @@ -268,7 +270,7 @@ public function testAuthenticateUserCanModifyTokenFromEvent() $this->tokenStorage->expects($this->once())->method('setToken')->with($this->identicalTo($modifiedToken)); - $manager = $this->createManager([$authenticator]); + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateUser($this->user, $authenticator, $this->request); $this->assertTrue($listenerCalled, 'The AuthenticationTokenCreatedEvent listener is not called'); } @@ -289,7 +291,7 @@ public function testInteractiveAuthenticator() ->with($this->anything(), $this->token, 'main') ->willReturn($this->response); - $manager = $this->createManager([$authenticator]); + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::None); $response = $manager->authenticateRequest($this->request); $this->assertSame($this->response, $response); } @@ -310,7 +312,7 @@ public function testLegacyInteractiveAuthenticator() ->with($this->anything(), $this->token, 'main') ->willReturn($this->response); - $manager = $this->createManager([$authenticator]); + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::None); $response = $manager->authenticateRequest($this->request); $this->assertSame($this->response, $response); } @@ -328,7 +330,43 @@ public function testAuthenticateRequestHidesInvalidUserExceptions() ->with($this->equalTo($this->request), $this->callback(fn ($e) => $e instanceof BadCredentialsException && $invalidUserException === $e->getPrevious())) ->willReturn($this->response); - $manager = $this->createManager([$authenticator]); + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::None); + $response = $manager->authenticateRequest($this->request); + $this->assertSame($this->response, $response); + } + + public function testAuthenticateRequestShowsAccountStatusException() + { + $invalidUserException = new LockedException(); + $authenticator = $this->createMock(TestInteractiveAuthenticator::class); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willThrowException($invalidUserException); + + $authenticator->expects($this->any()) + ->method('onAuthenticationFailure') + ->with($this->equalTo($this->request), $this->callback(fn ($e) => $e === $invalidUserException)) + ->willReturn($this->response); + + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::AccountStatus); + $response = $manager->authenticateRequest($this->request); + $this->assertSame($this->response, $response); + } + + public function testAuthenticateRequestHidesInvalidAccountStatusException() + { + $invalidUserException = new LockedException(); + $authenticator = $this->createMock(TestInteractiveAuthenticator::class); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willThrowException($invalidUserException); + + $authenticator->expects($this->any()) + ->method('onAuthenticationFailure') + ->with($this->equalTo($this->request), $this->callback(fn ($e) => $e instanceof BadCredentialsException && $invalidUserException === $e->getPrevious())) + ->willReturn($this->response); + + $manager = $this->createManager([$authenticator], exposeSecurityErrors: ExposeSecurityLevel::None); $response = $manager->authenticateRequest($this->request); $this->assertSame($this->response, $response); } @@ -365,7 +403,7 @@ public function log($level, $message, array $context = []): void } }; - $manager = $this->createManager([$authenticator], 'main', true, [], $logger); + $manager = $this->createManager([$authenticator], 'main', true, [], $logger, exposeSecurityErrors: ExposeSecurityLevel::None); $response = $manager->authenticateRequest($this->request); $this->assertSame($this->response, $response); $this->assertStringContainsString($authenticator::class, $logger->logContexts[0]['authenticator']); @@ -384,9 +422,9 @@ private static function createDummySupportsAuthenticator(?bool $supports = true) return new DummySupportsAuthenticator($supports); } - private function createManager($authenticators, $firewallName = 'main', $eraseCredentials = true, array $requiredBadges = [], ?LoggerInterface $logger = null) + private function createManager($authenticators, $firewallName = 'main', $eraseCredentials = true, array $requiredBadges = [], ?LoggerInterface $logger = null, ExposeSecurityLevel $exposeSecurityErrors = ExposeSecurityLevel::AccountStatus) { - return new AuthenticatorManager($authenticators, $this->tokenStorage, $this->eventDispatcher, $firewallName, $logger, $eraseCredentials, true, $requiredBadges); + return new AuthenticatorManager($authenticators, $this->tokenStorage, $this->eventDispatcher, $firewallName, $logger, $eraseCredentials, $exposeSecurityErrors, $requiredBadges); } } From 71c434b0104edfff9740607644fe6d45946e9bc5 Mon Sep 17 00:00:00 2001 From: Robin Chalas Date: Fri, 6 Dec 2024 11:20:22 +0100 Subject: [PATCH 05/23] [Security] Deprecate `UserInterface` & `TokenInterface`'s `eraseCredentials()` --- Authentication/AuthenticatorManager.php | 5 +++++ CHANGELOG.md | 2 ++ .../Authentication/AuthenticatorManagerTest.php | 16 ++++++++++++---- composer.json | 2 +- 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/Authentication/AuthenticatorManager.php b/Authentication/AuthenticatorManager.php index 6a5f365e..fa513a94 100644 --- a/Authentication/AuthenticatorManager.php +++ b/Authentication/AuthenticatorManager.php @@ -69,6 +69,10 @@ public function __construct( } $this->exposeSecurityErrors = $exposeSecurityErrors; + + if ($eraseCredentials) { + trigger_deprecation('symfony/security-http', '7.3', sprintf('Passing true as "$eraseCredentials" argument to "%s::__construct()" is deprecated and won\'t have any effect in 8.0, pass "false" instead and use your own erasing logic if needed.', __CLASS__)); + } } /** @@ -208,6 +212,7 @@ private function executeAuthenticator(AuthenticatorInterface $authenticator, Req // announce the authentication token $authenticatedToken = $this->eventDispatcher->dispatch(new AuthenticationTokenCreatedEvent($authenticatedToken, $passport))->getAuthenticatedToken(); + // @deprecated since Symfony 7.3, remove the if statement in 8.0 if (true === $this->eraseCredentials) { $authenticatedToken->eraseCredentials(); } diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dde7c24..fc55cbc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ CHANGELOG * Add encryption support to `OidcTokenHandler` (JWE) * Replace `$hideAccountStatusExceptions` argument with `$exposeSecurityErrors` in `AuthenticatorManager` constructor + * Deprecate passing `true` for the `$eraseCredentials` parameter of `AuthenticatorManager::__construct()`, erase credentials + on your own e.g. upon `AuthenticationTokenCreatedEvent` instead. Passing it won't thave any effect in 8.0. 7.2 --- diff --git a/Tests/Authentication/AuthenticatorManagerTest.php b/Tests/Authentication/AuthenticatorManagerTest.php index 395a43b8..78da50a8 100644 --- a/Tests/Authentication/AuthenticatorManagerTest.php +++ b/Tests/Authentication/AuthenticatorManagerTest.php @@ -15,6 +15,7 @@ use PHPUnit\Framework\TestCase; use Psr\Log\AbstractLogger; use Psr\Log\LoggerInterface; +use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -40,6 +41,8 @@ class AuthenticatorManagerTest extends TestCase { + use ExpectDeprecationTrait; + private MockObject&TokenStorageInterface $tokenStorage; private EventDispatcher $eventDispatcher; private Request $request; @@ -163,7 +166,7 @@ public function testRequiredBadgeMissing() $authenticator->expects($this->once())->method('onAuthenticationFailure')->with($this->anything(), $this->callback(fn ($exception) => 'Authentication failed; Some badges marked as required by the firewall config are not available on the passport: "'.CsrfTokenBadge::class.'".' === $exception->getMessage())); - $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); + $manager = $this->createManager([$authenticator], 'main', false, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } @@ -179,11 +182,12 @@ public function testAllRequiredBadgesPresent() $authenticator->expects($this->once())->method('onAuthenticationSuccess'); - $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); + $manager = $this->createManager([$authenticator], 'main', false, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } /** + * @group legacy * @dataProvider provideEraseCredentialsData */ public function testEraseCredentials($eraseCredentials) @@ -197,6 +201,10 @@ public function testEraseCredentials($eraseCredentials) $this->token->expects($eraseCredentials ? $this->once() : $this->never())->method('eraseCredentials'); + if ($eraseCredentials) { + $this->expectDeprecation('Since symfony/security-http 7.3: Passing true as "$eraseCredentials" argument to "Symfony\Component\Security\Http\Authentication\AuthenticatorManager::__construct()" is deprecated and won\'t have any effect in 8.0, pass "false" instead and use your own erasing logic if needed.'); + } + $manager = $this->createManager([$authenticator], 'main', $eraseCredentials, exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } @@ -403,7 +411,7 @@ public function log($level, $message, array $context = []): void } }; - $manager = $this->createManager([$authenticator], 'main', true, [], $logger, exposeSecurityErrors: ExposeSecurityLevel::None); + $manager = $this->createManager([$authenticator], 'main', false, [], $logger, exposeSecurityErrors: ExposeSecurityLevel::None); $response = $manager->authenticateRequest($this->request); $this->assertSame($this->response, $response); $this->assertStringContainsString($authenticator::class, $logger->logContexts[0]['authenticator']); @@ -422,7 +430,7 @@ private static function createDummySupportsAuthenticator(?bool $supports = true) return new DummySupportsAuthenticator($supports); } - private function createManager($authenticators, $firewallName = 'main', $eraseCredentials = true, array $requiredBadges = [], ?LoggerInterface $logger = null, ExposeSecurityLevel $exposeSecurityErrors = ExposeSecurityLevel::AccountStatus) + private function createManager($authenticators, $firewallName = 'main', $eraseCredentials = false, array $requiredBadges = [], ?LoggerInterface $logger = null, ExposeSecurityLevel $exposeSecurityErrors = ExposeSecurityLevel::AccountStatus) { return new AuthenticatorManager($authenticators, $this->tokenStorage, $this->eventDispatcher, $firewallName, $logger, $eraseCredentials, $exposeSecurityErrors, $requiredBadges); } diff --git a/composer.json b/composer.json index 9a443fe8..77f6af87 100644 --- a/composer.json +++ b/composer.json @@ -22,7 +22,7 @@ "symfony/http-kernel": "^6.4|^7.0", "symfony/polyfill-mbstring": "~1.0", "symfony/property-access": "^6.4|^7.0", - "symfony/security-core": "^7.2", + "symfony/security-core": "^7.3", "symfony/service-contracts": "^2.5|^3" }, "require-dev": { From 8b0dda3efa2d9cb8d0e9f709140d14175d138513 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 3 Feb 2025 18:35:30 +0100 Subject: [PATCH 06/23] [Security] Improve BC-layer to deprecate eraseCredentials methods --- Authentication/AuthenticatorManager.php | 47 ++++++++++++++++--- CHANGELOG.md | 2 - .../AuthenticatorManagerTest.php | 21 +++++++-- .../PasswordMigratingListenerTest.php | 8 +--- Tests/Firewall/ContextListenerTest.php | 11 +---- Tests/LoginLink/LoginLinkHandlerTest.php | 6 +-- 6 files changed, 59 insertions(+), 36 deletions(-) diff --git a/Authentication/AuthenticatorManager.php b/Authentication/AuthenticatorManager.php index fa513a94..856a5bfa 100644 --- a/Authentication/AuthenticatorManager.php +++ b/Authentication/AuthenticatorManager.php @@ -14,6 +14,7 @@ use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\Security\Core\Authentication\Token\AbstractToken; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\AuthenticationEvents; @@ -69,10 +70,6 @@ public function __construct( } $this->exposeSecurityErrors = $exposeSecurityErrors; - - if ($eraseCredentials) { - trigger_deprecation('symfony/security-http', '7.3', sprintf('Passing true as "$eraseCredentials" argument to "%s::__construct()" is deprecated and won\'t have any effect in 8.0, pass "false" instead and use your own erasing logic if needed.', __CLASS__)); - } } /** @@ -212,9 +209,8 @@ private function executeAuthenticator(AuthenticatorInterface $authenticator, Req // announce the authentication token $authenticatedToken = $this->eventDispatcher->dispatch(new AuthenticationTokenCreatedEvent($authenticatedToken, $passport))->getAuthenticatedToken(); - // @deprecated since Symfony 7.3, remove the if statement in 8.0 - if (true === $this->eraseCredentials) { - $authenticatedToken->eraseCredentials(); + if ($this->eraseCredentials) { + self::checkEraseCredentials($authenticatedToken)?->eraseCredentials(); } $this->eventDispatcher->dispatch(new AuthenticationSuccessEvent($authenticatedToken), AuthenticationEvents::AUTHENTICATION_SUCCESS); @@ -292,4 +288,41 @@ private function isSensitiveException(AuthenticationException $exception): bool return false; } + + /** + * @deprecated since Symfony 7.3 + */ + private static function checkEraseCredentials(TokenInterface|UserInterface|null $token): TokenInterface|UserInterface|null + { + if (!$token || !method_exists($token, 'eraseCredentials')) { + return null; + } + + static $genericImplementations = []; + $m = null; + + if (!isset($genericImplementations[$token::class])) { + $m = new \ReflectionMethod($token, 'eraseCredentials'); + $genericImplementations[$token::class] = AbstractToken::class === $m->class; + } + + if ($genericImplementations[$token::class]) { + return self::checkEraseCredentials($token->getUser()); + } + + static $deprecatedImplementations = []; + + if (!isset($deprecatedImplementations[$token::class])) { + $m ??= new \ReflectionMethod($token, 'eraseCredentials'); + $deprecatedImplementations[$token::class] = !$m->getAttributes(\Deprecated::class); + } + + if ($deprecatedImplementations[$token::class]) { + trigger_deprecation('symfony/security-http', '7.3', 'Implementing "%s::eraseCredentials()" is deprecated since Symfony 7.3; add the #[\Deprecated] attribute on the method to signal its either empty or that you moved the logic elsewhere, typically to the "__serialize()" method.', get_debug_type($token)); + + return $token; + } + + return null; + } } diff --git a/CHANGELOG.md b/CHANGELOG.md index fc55cbc3..8dde7c24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,6 @@ CHANGELOG * Add encryption support to `OidcTokenHandler` (JWE) * Replace `$hideAccountStatusExceptions` argument with `$exposeSecurityErrors` in `AuthenticatorManager` constructor - * Deprecate passing `true` for the `$eraseCredentials` parameter of `AuthenticatorManager::__construct()`, erase credentials - on your own e.g. upon `AuthenticationTokenCreatedEvent` instead. Passing it won't thave any effect in 8.0. 7.2 --- diff --git a/Tests/Authentication/AuthenticatorManagerTest.php b/Tests/Authentication/AuthenticatorManagerTest.php index 78da50a8..a88b3ba5 100644 --- a/Tests/Authentication/AuthenticatorManagerTest.php +++ b/Tests/Authentication/AuthenticatorManagerTest.php @@ -19,6 +19,7 @@ use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\Security\Core\Authentication\Token\AbstractToken; 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; @@ -166,7 +167,7 @@ public function testRequiredBadgeMissing() $authenticator->expects($this->once())->method('onAuthenticationFailure')->with($this->anything(), $this->callback(fn ($exception) => 'Authentication failed; Some badges marked as required by the firewall config are not available on the passport: "'.CsrfTokenBadge::class.'".' === $exception->getMessage())); - $manager = $this->createManager([$authenticator], 'main', false, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); + $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } @@ -182,12 +183,13 @@ public function testAllRequiredBadgesPresent() $authenticator->expects($this->once())->method('onAuthenticationSuccess'); - $manager = $this->createManager([$authenticator], 'main', false, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); + $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); } /** * @group legacy + * * @dataProvider provideEraseCredentialsData */ public function testEraseCredentials($eraseCredentials) @@ -197,16 +199,25 @@ public function testEraseCredentials($eraseCredentials) $authenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter', fn () => $this->user))); - $authenticator->expects($this->any())->method('createToken')->willReturn($this->token); + $token = new class extends AbstractToken { + public $erased = false; + + public function eraseCredentials(): void + { + $this->erased = true; + } + }; - $this->token->expects($eraseCredentials ? $this->once() : $this->never())->method('eraseCredentials'); + $authenticator->expects($this->any())->method('createToken')->willReturn($token); if ($eraseCredentials) { - $this->expectDeprecation('Since symfony/security-http 7.3: Passing true as "$eraseCredentials" argument to "Symfony\Component\Security\Http\Authentication\AuthenticatorManager::__construct()" is deprecated and won\'t have any effect in 8.0, pass "false" instead and use your own erasing logic if needed.'); + $this->expectDeprecation(\sprintf('Since symfony/security-http 7.3: Implementing "%s@anonymous::eraseCredentials()" is deprecated since Symfony 7.3; add the #[\Deprecated] attribute on the method to signal its either empty or that you moved the logic elsewhere, typically to the "__serialize()" method.', AbstractToken::class)); } $manager = $this->createManager([$authenticator], 'main', $eraseCredentials, exposeSecurityErrors: ExposeSecurityLevel::None); $manager->authenticateRequest($this->request); + + $this->assertSame($eraseCredentials, $token->erased); } public static function provideEraseCredentialsData() diff --git a/Tests/EventListener/PasswordMigratingListenerTest.php b/Tests/EventListener/PasswordMigratingListenerTest.php index 8cba5287..15671691 100644 --- a/Tests/EventListener/PasswordMigratingListenerTest.php +++ b/Tests/EventListener/PasswordMigratingListenerTest.php @@ -150,8 +150,6 @@ public function loadUserByUsername(string $username): UserInterface abstract class TestPasswordAuthenticatedUser implements UserInterface, PasswordAuthenticatedUserInterface { abstract public function getPassword(): ?string; - - abstract public function getSalt(): ?string; } class DummyTestPasswordAuthenticatedUser extends TestPasswordAuthenticatedUser @@ -161,16 +159,12 @@ public function getPassword(): ?string return null; } - public function getSalt(): ?string - { - return null; - } - public function getRoles(): array { return []; } + #[\Deprecated] public function eraseCredentials(): void { } diff --git a/Tests/Firewall/ContextListenerTest.php b/Tests/Firewall/ContextListenerTest.php index b354af73..11de7d54 100644 --- a/Tests/Firewall/ContextListenerTest.php +++ b/Tests/Firewall/ContextListenerTest.php @@ -563,21 +563,11 @@ public function __serialize(): array return [$this->user, $this->roles]; } - public function serialize(): string - { - return serialize($this->__serialize()); - } - public function __unserialize(array $data): void { [$this->user, $this->roles] = $data; } - public function unserialize($serialized) - { - $this->__unserialize(\is_array($serialized) ? $serialized : unserialize($serialized)); - } - public function __toString(): string { return $this->user->getUserIdentifier(); @@ -603,6 +593,7 @@ public function getUserIdentifier(): string return $this->getUserIdentifier(); } + #[\Deprecated] public function eraseCredentials(): void { } diff --git a/Tests/LoginLink/LoginLinkHandlerTest.php b/Tests/LoginLink/LoginLinkHandlerTest.php index 52ee5fab..e48c6bd2 100644 --- a/Tests/LoginLink/LoginLinkHandlerTest.php +++ b/Tests/LoginLink/LoginLinkHandlerTest.php @@ -284,16 +284,12 @@ public function getPassword(): string return $this->passwordProperty; } - public function getSalt(): string - { - return ''; - } - public function getUserIdentifier(): string { return $this->username; } + #[\Deprecated] public function eraseCredentials(): void { } From 251a5089cfcfb7c0201aa3877de860de9992f28f Mon Sep 17 00:00:00 2001 From: Florent Morselli Date: Sat, 18 Jan 2025 08:20:05 +0100 Subject: [PATCH 07/23] Add a normalization step for the user-identifier in firewalls --- Authenticator/Passport/Badge/UserBadge.php | 16 +++++++-- CHANGELOG.md | 1 + .../Passport/Badge/UserBadgeTest.php | 33 +++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/Authenticator/Passport/Badge/UserBadge.php b/Authenticator/Passport/Badge/UserBadge.php index 6833f081..d936ff62 100644 --- a/Authenticator/Passport/Badge/UserBadge.php +++ b/Authenticator/Passport/Badge/UserBadge.php @@ -33,6 +33,7 @@ class UserBadge implements BadgeInterface /** @var callable|null */ private $userLoader; private UserInterface $user; + private ?\Closure $identifierNormalizer = null; /** * Initializes the user badge. @@ -51,6 +52,7 @@ public function __construct( private string $userIdentifier, ?callable $userLoader = null, private ?array $attributes = null, + ?\Closure $identifierNormalizer = null, ) { if ('' === $userIdentifier) { trigger_deprecation('symfony/security-http', '7.2', 'Using an empty string as user identifier is deprecated and will throw an exception in Symfony 8.0.'); @@ -60,12 +62,20 @@ public function __construct( if (\strlen($userIdentifier) > self::MAX_USERNAME_LENGTH) { throw new BadCredentialsException('Username too long.'); } + if ($identifierNormalizer) { + $this->identifierNormalizer = static fn () => $identifierNormalizer($userIdentifier); + } $this->userLoader = $userLoader; } public function getUserIdentifier(): string { + if (isset($this->identifierNormalizer)) { + $this->userIdentifier = ($this->identifierNormalizer)(); + $this->identifierNormalizer = null; + } + return $this->userIdentifier; } @@ -88,15 +98,15 @@ public function getUser(): UserInterface } if (null === $this->getAttributes()) { - $user = ($this->userLoader)($this->userIdentifier); + $user = ($this->userLoader)($this->getUserIdentifier()); } else { - $user = ($this->userLoader)($this->userIdentifier, $this->getAttributes()); + $user = ($this->userLoader)($this->getUserIdentifier(), $this->getAttributes()); } // No user has been found via the $this->userLoader callback if (null === $user) { $exception = new UserNotFoundException(); - $exception->setUserIdentifier($this->userIdentifier); + $exception->setUserIdentifier($this->getUserIdentifier()); throw $exception; } diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dde7c24..568df817 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ CHANGELOG * Add encryption support to `OidcTokenHandler` (JWE) * Replace `$hideAccountStatusExceptions` argument with `$exposeSecurityErrors` in `AuthenticatorManager` constructor + * Add argument `$identifierNormalizer` to `UserBadge::__construct()` to allow normalizing the identifier 7.2 --- diff --git a/Tests/Authenticator/Passport/Badge/UserBadgeTest.php b/Tests/Authenticator/Passport/Badge/UserBadgeTest.php index e2a4dc8e..f648d048 100644 --- a/Tests/Authenticator/Passport/Badge/UserBadgeTest.php +++ b/Tests/Authenticator/Passport/Badge/UserBadgeTest.php @@ -15,6 +15,10 @@ use Symfony\Bridge\PhpUnit\ExpectUserDeprecationMessageTrait; use Symfony\Component\Security\Core\Exception\UserNotFoundException; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; +use Symfony\Component\String\Slugger\AsciiSlugger; +use Symfony\Component\String\UnicodeString; + +use function Symfony\Component\String\u; class UserBadgeTest extends TestCase { @@ -36,4 +40,33 @@ public function testEmptyUserIdentifier() // $this->expectException(BadCredentialsException::class) new UserBadge('', fn () => null); } + + /** + * @dataProvider provideUserIdentifierNormalizationData + */ + public function testUserIdentifierNormalization(string $identifier, string $expectedNormalizedIdentifier, callable $normalizer) + { + $badge = new UserBadge($identifier, fn () => null, identifierNormalizer: $normalizer); + + static::assertSame($expectedNormalizedIdentifier, $badge->getUserIdentifier()); + } + + public static function provideUserIdentifierNormalizationData(): iterable + { + $lowerAndNFKC = static fn (string $identifier) => u($identifier)->normalize(UnicodeString::NFKC)->lower()->toString(); + yield 'Simple lower conversion' => ['SmiTh', 'smith', $lowerAndNFKC]; + yield 'Normalize fi to fi. Other unicode characters are preserved (р, с, ѕ and а)' => ['рrinсeѕѕ.fionа', 'рrinсeѕѕ.fionа', $lowerAndNFKC]; + yield 'Greek characters' => ['ΝιΚόΛΑος', 'νικόλαος', $lowerAndNFKC]; + + $slugger = new AsciiSlugger('en'); + $asciiWithPrefix = static fn (string $identifier) => u($slugger->slug($identifier))->ascii()->lower()->prepend('USERID--')->toString(); + yield 'Username with prefix' => ['John Doe 1', 'USERID--john-doe-1', $asciiWithPrefix]; + + if (!\extension_loaded('intl')) { + return; + } + $upperAndAscii = fn (string $identifier) => u($identifier)->ascii()->upper()->toString(); + yield 'Greek to ASCII' => ['ΝιΚόΛΑος', 'NIKOLAOS', $upperAndAscii]; + yield 'Katakana to ASCII' => ['たなかそういち', 'TANAKASOUICHI', $upperAndAscii]; + } } From 042728257b9cce1aa3e0afd92c383551edd9a4bf Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 20 Jan 2025 16:22:38 +0100 Subject: [PATCH 08/23] [Security] Support hashing the hashed password using crc32c when putting the user in the session --- CHANGELOG.md | 1 + Firewall/ContextListener.php | 15 +++++++++++---- Tests/Firewall/ContextListenerTest.php | 9 +++++++-- Tests/Fixtures/CustomUser.php | 14 ++++++++++++-- 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 568df817..3a4f694a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ CHANGELOG * Add encryption support to `OidcTokenHandler` (JWE) * Replace `$hideAccountStatusExceptions` argument with `$exposeSecurityErrors` in `AuthenticatorManager` constructor * Add argument `$identifierNormalizer` to `UserBadge::__construct()` to allow normalizing the identifier + * Support hashing the hashed password using crc32c when putting the user in the session 7.2 --- diff --git a/Firewall/ContextListener.php b/Firewall/ContextListener.php index bbdbd4b7..05a4a84b 100644 --- a/Firewall/ContextListener.php +++ b/Firewall/ContextListener.php @@ -292,9 +292,16 @@ private static function hasUserChanged(UserInterface $originalUser, TokenInterfa } if ($originalUser instanceof PasswordAuthenticatedUserInterface || $refreshedUser instanceof PasswordAuthenticatedUserInterface) { - if (!$originalUser instanceof PasswordAuthenticatedUserInterface - || !$refreshedUser instanceof PasswordAuthenticatedUserInterface - || $refreshedUser->getPassword() !== ($originalUser->getPassword() ?? $refreshedUser->getPassword()) + if (!$originalUser instanceof PasswordAuthenticatedUserInterface || !$refreshedUser instanceof PasswordAuthenticatedUserInterface) { + return true; + } + + $originalPassword = $originalUser->getPassword(); + $refreshedPassword = $refreshedUser->getPassword(); + + if (null !== $originalPassword + && $refreshedPassword !== $originalPassword + && (8 !== \strlen($originalPassword) || hash('crc32c', $refreshedPassword ?? $originalPassword) !== $originalPassword) ) { return true; } @@ -303,7 +310,7 @@ private static function hasUserChanged(UserInterface $originalUser, TokenInterfa return true; } - if ($originalUser instanceof LegacyPasswordAuthenticatedUserInterface && $refreshedUser instanceof LegacyPasswordAuthenticatedUserInterface && $originalUser->getSalt() !== $refreshedUser->getSalt()) { + if ($originalUser instanceof LegacyPasswordAuthenticatedUserInterface && $originalUser->getSalt() !== $refreshedUser->getSalt()) { return true; } } diff --git a/Tests/Firewall/ContextListenerTest.php b/Tests/Firewall/ContextListenerTest.php index 11de7d54..7c3248c7 100644 --- a/Tests/Firewall/ContextListenerTest.php +++ b/Tests/Firewall/ContextListenerTest.php @@ -377,9 +377,14 @@ public function testOnKernelResponseRemoveListener() $this->assertEmpty($dispatcher->getListeners()); } - public function testRemovingPasswordFromSessionDoesntInvalidateTheToken() + /** + * @testWith [true] + * [false] + * [null] + */ + public function testNullOrHashedPasswordInSessionDoesntInvalidateTheToken(?bool $hashPassword) { - $user = new CustomUser('user', ['ROLE_USER'], 'pass'); + $user = new CustomUser('user', ['ROLE_USER'], 'pass', $hashPassword); $userProvider = $this->createMock(UserProviderInterface::class); $userProvider->expects($this->once()) diff --git a/Tests/Fixtures/CustomUser.php b/Tests/Fixtures/CustomUser.php index 16afc539..9d6e29e6 100644 --- a/Tests/Fixtures/CustomUser.php +++ b/Tests/Fixtures/CustomUser.php @@ -19,7 +19,8 @@ final class CustomUser implements UserInterface, PasswordAuthenticatedUserInterf public function __construct( private string $username, private array $roles, - private ?string $password = null, + private ?string $password, + private ?bool $hashPassword, ) { } @@ -44,6 +45,15 @@ public function eraseCredentials(): void public function __serialize(): array { - return [\sprintf("\0%s\0username", self::class) => $this->username]; + $data = (array) $this; + $passwordKey = \sprintf("\0%s\0password", self::class); + + if ($this->hashPassword) { + $data[$passwordKey] = hash('crc32c', $this->password); + } elseif (null !== $this->hashPassword) { + unset($data[$passwordKey]); + } + + return $data; } } From 5d6d6ee4ab3ead2aca57f3350521a8681d8ee1b3 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 27 Aug 2024 19:49:01 +0200 Subject: [PATCH 09/23] [Security] Add ability for voters to explain their vote --- EventListener/IsGrantedAttributeListener.php | 34 ++++----------- Firewall/AccessListener.php | 20 ++++----- Firewall/SwitchUserListener.php | 11 +++-- .../IsGrantedAttributeListenerTest.php | 43 ++++++++++++++----- Tests/Firewall/AccessListenerTest.php | 3 +- 5 files changed, 59 insertions(+), 52 deletions(-) diff --git a/EventListener/IsGrantedAttributeListener.php b/EventListener/IsGrantedAttributeListener.php index c511cf04..5ac76c2b 100644 --- a/EventListener/IsGrantedAttributeListener.php +++ b/EventListener/IsGrantedAttributeListener.php @@ -18,6 +18,7 @@ use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent; use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\HttpKernel\KernelEvents; +use Symfony\Component\Security\Core\Authorization\AccessDecision; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; use Symfony\Component\Security\Core\Exception\AccessDeniedException; use Symfony\Component\Security\Core\Exception\RuntimeException; @@ -58,19 +59,21 @@ public function onKernelControllerArguments(ControllerArgumentsEvent $event): vo $subject = $this->getIsGrantedSubject($subjectRef, $request, $arguments); } } + $accessDecision = new AccessDecision(); - if (!$this->authChecker->isGranted($attribute->attribute, $subject)) { - $message = $attribute->message ?: \sprintf('Access Denied by #[IsGranted(%s)] on controller', $this->getIsGrantedString($attribute)); + if (!$accessDecision->isGranted = $this->authChecker->isGranted($attribute->attribute, $subject, $accessDecision)) { + $message = $attribute->message ?: $accessDecision->getMessage(); if ($statusCode = $attribute->statusCode) { throw new HttpException($statusCode, $message, code: $attribute->exceptionCode ?? 0); } - $accessDeniedException = new AccessDeniedException($message, code: $attribute->exceptionCode ?? 403); - $accessDeniedException->setAttributes($attribute->attribute); - $accessDeniedException->setSubject($subject); + $e = new AccessDeniedException($message, code: $attribute->exceptionCode ?? 403); + $e->setAttributes($attribute->attribute); + $e->setSubject($subject); + $e->setAccessDecision($accessDecision); - throw $accessDeniedException; + throw $e; } } } @@ -97,23 +100,4 @@ private function getIsGrantedSubject(string|Expression $subjectRef, Request $req return $arguments[$subjectRef]; } - - private function getIsGrantedString(IsGranted $isGranted): string - { - $processValue = fn ($value) => \sprintf($value instanceof Expression ? 'new Expression("%s")' : '"%s"', $value); - - $argsString = $processValue($isGranted->attribute); - - if (null !== $subject = $isGranted->subject) { - $subject = !\is_array($subject) ? $processValue($subject) : array_map(function ($key, $value) use ($processValue) { - $value = $processValue($value); - - return \is_string($key) ? \sprintf('"%s" => %s', $key, $value) : $value; - }, array_keys($subject), $subject); - - $argsString .= ', '.(!\is_array($subject) ? $subject : '['.implode(', ', $subject).']'); - } - - return $argsString; - } } diff --git a/Firewall/AccessListener.php b/Firewall/AccessListener.php index f04de5a9..8bfcb674 100644 --- a/Firewall/AccessListener.php +++ b/Firewall/AccessListener.php @@ -15,6 +15,7 @@ use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\Security\Core\Authentication\Token\NullToken; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecision; use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter; use Symfony\Component\Security\Core\Exception\AccessDeniedException; @@ -72,19 +73,16 @@ public function authenticate(RequestEvent $event): void } $token = $this->tokenStorage->getToken() ?? new NullToken(); + $accessDecision = new AccessDecision(); - if (!$this->accessDecisionManager->decide($token, $attributes, $request, true)) { - throw $this->createAccessDeniedException($request, $attributes); - } - } + if (!$accessDecision->isGranted = $this->accessDecisionManager->decide($token, $attributes, $request, $accessDecision, true)) { + $e = new AccessDeniedException($accessDecision->getMessage()); + $e->setAttributes($attributes); + $e->setSubject($request); + $e->setAccessDecision($accessDecision); - private function createAccessDeniedException(Request $request, array $attributes): AccessDeniedException - { - $exception = new AccessDeniedException(); - $exception->setAttributes($attributes); - $exception->setSubject($request); - - return $exception; + throw $e; + } } public static function getPriority(): int diff --git a/Firewall/SwitchUserListener.php b/Firewall/SwitchUserListener.php index ffdad52e..8c03e856 100644 --- a/Firewall/SwitchUserListener.php +++ b/Firewall/SwitchUserListener.php @@ -19,6 +19,7 @@ use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecision; use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; use Symfony\Component\Security\Core\Exception\AccessDeniedException; use Symfony\Component\Security\Core\Exception\AuthenticationCredentialsNotFoundException; @@ -153,12 +154,14 @@ private function attemptSwitchUser(Request $request, string $username): ?TokenIn throw $e; } + $accessDecision = new AccessDecision(); - if (false === $this->accessDecisionManager->decide($token, [$this->role], $user)) { - $exception = new AccessDeniedException(); - $exception->setAttributes($this->role); + if (!$accessDecision->isGranted = $this->accessDecisionManager->decide($token, [$this->role], $user, $accessDecision)) { + $e = new AccessDeniedException($accessDecision->getMessage()); + $e->setAttributes($this->role); + $e->setAccessDecision($accessDecision); - throw $exception; + throw $e; } $this->logger?->info('Attempting to switch to user.', ['username' => $username]); diff --git a/Tests/EventListener/IsGrantedAttributeListenerTest.php b/Tests/EventListener/IsGrantedAttributeListenerTest.php index 2d03b7ac..81ca1cb3 100644 --- a/Tests/EventListener/IsGrantedAttributeListenerTest.php +++ b/Tests/EventListener/IsGrantedAttributeListenerTest.php @@ -13,12 +13,20 @@ use PHPUnit\Framework\TestCase; use Symfony\Component\ExpressionLanguage\Expression; -use Symfony\Component\ExpressionLanguage\ExpressionLanguage; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent; use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecisionManager; +use Symfony\Component\Security\Core\Authorization\AuthorizationChecker; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Authorization\ExpressionLanguage; +use Symfony\Component\Security\Core\Authorization\Voter\ExpressionVoter; +use Symfony\Component\Security\Core\Authorization\Voter\RoleVoter; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; use Symfony\Component\Security\Core\Exception\AccessDeniedException; use Symfony\Component\Security\Http\EventListener\IsGrantedAttributeListener; use Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeController; @@ -213,10 +221,23 @@ public function testExceptionWhenMissingSubjectAttribute() */ public function testAccessDeniedMessages(string|Expression $attribute, string|array|null $subject, string $method, int $numOfArguments, string $expectedMessage) { - $authChecker = $this->createMock(AuthorizationCheckerInterface::class); - $authChecker->expects($this->any()) - ->method('isGranted') - ->willReturn(false); + $authChecker = new AuthorizationChecker(new TokenStorage(), new AccessDecisionManager((function () use (&$authChecker) { + yield new ExpressionVoter(new ExpressionLanguage(), null, $authChecker); + yield new RoleVoter(); + yield new class() extends Voter { + protected function supports(string $attribute, mixed $subject): bool + { + return 'POST_VIEW' === $attribute; + } + + protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token, ?Vote $vote = null): bool + { + $vote->reasons[] = 'Because I can 😈.'; + + return false; + } + }; + })())); $expressionLanguage = $this->createMock(ExpressionLanguage::class); $expressionLanguage->expects($this->any()) @@ -252,12 +273,12 @@ public function testAccessDeniedMessages(string|Expression $attribute, string|ar public static function getAccessDeniedMessageTests() { - yield ['ROLE_ADMIN', null, 'admin', 0, 'Access Denied by #[IsGranted("ROLE_ADMIN")] on controller']; - yield ['ROLE_ADMIN', 'bar', 'withSubject', 2, 'Access Denied by #[IsGranted("ROLE_ADMIN", "arg2Name")] on controller']; - yield ['ROLE_ADMIN', ['arg1Name' => 'bar', 'arg2Name' => 'bar'], 'withSubjectArray', 2, 'Access Denied by #[IsGranted("ROLE_ADMIN", ["arg1Name", "arg2Name"])] on controller']; - yield [new Expression('"ROLE_ADMIN" in role_names or is_granted("POST_VIEW", subject)'), 'bar', 'withExpressionInAttribute', 1, 'Access Denied by #[IsGranted(new Expression(""ROLE_ADMIN" in role_names or is_granted("POST_VIEW", subject)"), "post")] on controller']; - yield [new Expression('user === subject'), 'bar', 'withExpressionInSubject', 1, 'Access Denied by #[IsGranted(new Expression("user === subject"), new Expression("args["post"].getAuthor()"))] on controller']; - yield [new Expression('user === subject["author"]'), ['author' => 'bar', 'alias' => 'bar'], 'withNestedExpressionInSubject', 2, 'Access Denied by #[IsGranted(new Expression("user === subject["author"]"), ["author" => new Expression("args["post"].getAuthor()"), "alias" => "arg2Name"])] on controller']; + yield ['ROLE_ADMIN', null, 'admin', 0, 'Access Denied. The user doesn\'t have ROLE_ADMIN.']; + yield ['ROLE_ADMIN', 'bar', 'withSubject', 2, 'Access Denied. The user doesn\'t have ROLE_ADMIN.']; + yield ['ROLE_ADMIN', ['arg1Name' => 'bar', 'arg2Name' => 'bar'], 'withSubjectArray', 2, 'Access Denied. The user doesn\'t have ROLE_ADMIN.']; + yield [new Expression('"ROLE_ADMIN" in role_names or is_granted("POST_VIEW", subject)'), 'bar', 'withExpressionInAttribute', 1, 'Access Denied. Because I can 😈. Expression ("ROLE_ADMIN" in role_names or is_granted("POST_VIEW", subject)) is false.']; + yield [new Expression('user === subject'), 'bar', 'withExpressionInSubject', 1, 'Access Denied. Expression (user === subject) is false.']; + yield [new Expression('user === subject["author"]'), ['author' => 'bar', 'alias' => 'bar'], 'withNestedExpressionInSubject', 2, 'Access Denied. Expression (user === subject["author"]) is false.']; } public function testNotFoundHttpException() diff --git a/Tests/Firewall/AccessListenerTest.php b/Tests/Firewall/AccessListenerTest.php index 5decf414..83df93d3 100644 --- a/Tests/Firewall/AccessListenerTest.php +++ b/Tests/Firewall/AccessListenerTest.php @@ -20,6 +20,7 @@ use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; +use Symfony\Component\Security\Core\Authorization\AccessDecision; use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter; use Symfony\Component\Security\Core\Exception\AccessDeniedException; @@ -235,7 +236,7 @@ public function testHandleMWithultipleAttributesShouldBeHandledAsAnd() $accessDecisionManager ->expects($this->once()) ->method('decide') - ->with($this->equalTo($authenticatedToken), $this->equalTo(['foo' => 'bar', 'bar' => 'baz']), $this->equalTo($request), true) + ->with($this->equalTo($authenticatedToken), $this->equalTo(['foo' => 'bar', 'bar' => 'baz']), $this->equalTo($request), new AccessDecision(), true) ->willReturn(true) ; From 7c7d2de02f2875e822b4e0135f550a175c66a859 Mon Sep 17 00:00:00 2001 From: Alexandre Daubois Date: Mon, 9 Dec 2024 08:56:13 +0100 Subject: [PATCH 10/23] [Security] Allow using a callable with `#[IsGranted]` --- Attribute/IsGranted.php | 18 +- CHANGELOG.md | 1 + EventListener/IsGrantedAttributeListener.php | 4 +- ...antedAttributeWithCallableListenerTest.php | 371 ++++++++++++++++++ ...AttributeMethodsWithCallableController.php | 95 +++++ ...GrantedAttributeWithCallableController.php | 31 ++ 6 files changed, 512 insertions(+), 8 deletions(-) create mode 100644 Tests/EventListener/IsGrantedAttributeWithCallableListenerTest.php create mode 100644 Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php create mode 100644 Tests/Fixtures/IsGrantedAttributeWithCallableController.php diff --git a/Attribute/IsGranted.php b/Attribute/IsGranted.php index c69ab017..546f293b 100644 --- a/Attribute/IsGranted.php +++ b/Attribute/IsGranted.php @@ -12,6 +12,10 @@ namespace Symfony\Component\Security\Http\Attribute; use Symfony\Component\ExpressionLanguage\Expression; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface; +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; /** * Checks if user has permission to access to some resource using security roles and voters. @@ -24,15 +28,15 @@ final class IsGranted { /** - * @param string|Expression $attribute The attribute that will be checked against a given authentication token and optional subject - * @param array|string|Expression|null $subject An optional subject - e.g. the current object being voted on - * @param string|null $message A custom message when access is not granted - * @param int|null $statusCode If set, will throw HttpKernel's HttpException with the given $statusCode; if null, Security\Core's AccessDeniedException will be used - * @param int|null $exceptionCode If set, will add the exception code to thrown exception + * @param string|Expression|(\Closure(TokenInterface $token, mixed $subject, AccessDecisionManagerInterface $accessDecisionManager, AuthenticationTrustResolverInterface $trustResolver): bool) $attribute The attribute that will be checked against a given authentication token and optional subject + * @param array|string|Expression|(\Closure(array, Request): mixed)|null $subject An optional subject - e.g. the current object being voted on + * @param string|null $message A custom message when access is not granted + * @param int|null $statusCode If set, will throw HttpKernel's HttpException with the given $statusCode; if null, Security\Core's AccessDeniedException will be used + * @param int|null $exceptionCode If set, will add the exception code to thrown exception */ public function __construct( - public string|Expression $attribute, - public array|string|Expression|null $subject = null, + public string|Expression|\Closure $attribute, + public array|string|Expression|\Closure|null $subject = null, public ?string $message = null, public ?int $statusCode = null, public ?int $exceptionCode = null, diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a4f694a..22fe59f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ CHANGELOG * Replace `$hideAccountStatusExceptions` argument with `$exposeSecurityErrors` in `AuthenticatorManager` constructor * Add argument `$identifierNormalizer` to `UserBadge::__construct()` to allow normalizing the identifier * Support hashing the hashed password using crc32c when putting the user in the session + * Add support for closures in `#[IsGranted]` 7.2 --- diff --git a/EventListener/IsGrantedAttributeListener.php b/EventListener/IsGrantedAttributeListener.php index 5ac76c2b..e79a2e94 100644 --- a/EventListener/IsGrantedAttributeListener.php +++ b/EventListener/IsGrantedAttributeListener.php @@ -55,6 +55,8 @@ public function onKernelControllerArguments(ControllerArgumentsEvent $event): vo foreach ($subjectRef as $refKey => $ref) { $subject[\is_string($refKey) ? $refKey : (string) $ref] = $this->getIsGrantedSubject($ref, $request, $arguments); } + } elseif ($subjectRef instanceof \Closure) { + $subject = $subjectRef($arguments, $request); } else { $subject = $this->getIsGrantedSubject($subjectRef, $request, $arguments); } @@ -69,7 +71,7 @@ public function onKernelControllerArguments(ControllerArgumentsEvent $event): vo } $e = new AccessDeniedException($message, code: $attribute->exceptionCode ?? 403); - $e->setAttributes($attribute->attribute); + $e->setAttributes([$attribute->attribute]); $e->setSubject($subject); $e->setAccessDecision($accessDecision); diff --git a/Tests/EventListener/IsGrantedAttributeWithCallableListenerTest.php b/Tests/EventListener/IsGrantedAttributeWithCallableListenerTest.php new file mode 100644 index 00000000..8b80736e --- /dev/null +++ b/Tests/EventListener/IsGrantedAttributeWithCallableListenerTest.php @@ -0,0 +1,371 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Tests\EventListener; + +use PHPUnit\Framework\TestCase; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent; +use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Exception\AccessDeniedException; +use Symfony\Component\Security\Http\EventListener\IsGrantedAttributeListener; +use Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController; +use Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeWithCallableController; + +/** + * @requires PHP 8.5 + */ +class IsGrantedAttributeWithCallableListenerTest extends TestCase +{ + public function testAttribute() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->exactly(2)) + ->method('isGranted') + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeWithCallableController(), 'foo'], + [], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeWithCallableController(), 'bar'], + [], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testNothingHappensWithNoConfig() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->never()) + ->method('isGranted'); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'noAttribute'], + [], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testIsGrantedCalledCorrectly() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + ->with($this->isInstanceOf(\Closure::class), null) + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'admin'], + [], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testIsGrantedSubjectFromArguments() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + // the subject => arg2name will eventually resolve to the 2nd argument, which has this value + ->with($this->isInstanceOf(\Closure::class), 'arg2Value') + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'withSubject'], + ['arg1Value', 'arg2Value'], + new Request(), + null + ); + + // create metadata for 2 named args for the controller + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testIsGrantedSubjectFromArgumentsWithArray() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + // the subject => arg2name will eventually resolve to the 2nd argument, which has this value + ->with($this->isInstanceOf(\Closure::class), [ + 'arg1Name' => 'arg1Value', + 'arg2Name' => 'arg2Value', + ]) + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'withSubjectArray'], + ['arg1Value', 'arg2Value'], + new Request(), + null + ); + + // create metadata for 2 named args for the controller + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testIsGrantedNullSubjectFromArguments() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + ->with($this->isInstanceOf(\Closure::class), null) + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'withSubject'], + ['arg1Value', null], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testIsGrantedArrayWithNullValueSubjectFromArguments() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + ->with($this->isInstanceOf(\Closure::class), [ + 'arg1Name' => 'arg1Value', + 'arg2Name' => null, + ]) + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'withSubjectArray'], + ['arg1Value', null], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testExceptionWhenMissingSubjectAttribute() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'withMissingSubject'], + [], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + + $this->expectException(\RuntimeException::class); + + $listener->onKernelControllerArguments($event); + } + + /** + * @dataProvider getAccessDeniedMessageTests + */ + public function testAccessDeniedMessages(string|array|null $subject, string $method, int $numOfArguments, string $expectedMessage) + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->any()) + ->method('isGranted') + ->willReturn(false); + + // avoid the error of the subject not being found in the request attributes + $arguments = array_fill(0, $numOfArguments, 'bar'); + $listener = new IsGrantedAttributeListener($authChecker); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), $method], + $arguments, + new Request(), + null + ); + + try { + $listener->onKernelControllerArguments($event); + $this->fail(); + } catch (AccessDeniedException $e) { + $this->assertSame($expectedMessage, $e->getMessage()); + $this->assertIsCallable($e->getAttributes()[0]); + if (null !== $subject) { + $this->assertSame($subject, $e->getSubject()); + } else { + $this->assertNull($e->getSubject()); + } + } + } + + public static function getAccessDeniedMessageTests() + { + yield [null, 'admin', 0, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::admin():23})] on controller']; + yield ['bar', 'withSubject', 2, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withSubject():30}, "arg2Name")] on controller']; + yield [['arg1Name' => 'bar', 'arg2Name' => 'bar'], 'withSubjectArray', 2, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withSubjectArray():37}, ["arg1Name", "arg2Name"])] on controller']; + yield ['bar', 'withCallableAsSubject', 1, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withCallableAsSubject():73}, {closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withCallableAsSubject():76})] on controller']; + yield [['author' => 'bar', 'alias' => 'bar'], 'withNestArgsInSubject', 2, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withNestArgsInSubject():84}, {closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withNestArgsInSubject():86})] on controller']; + } + + public function testNotFoundHttpException() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->any()) + ->method('isGranted') + ->willReturn(false); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'notFound'], + [], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + + $this->expectException(HttpException::class); + $this->expectExceptionMessage('Not found'); + + $listener->onKernelControllerArguments($event); + } + + public function testIsGrantedWithCallableAsSubject() + { + $request = new Request(); + + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + ->with($this->isInstanceOf(\Closure::class), 'postVal') + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'withCallableAsSubject'], + ['postVal'], + $request, + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testIsGrantedWithNestedExpressionInSubject() + { + $request = new Request(); + + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + ->with($this->isInstanceOf(\Closure::class), ['author' => 'postVal', 'alias' => 'bar']) + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'withNestArgsInSubject'], + ['postVal', 'bar'], + $request, + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testHttpExceptionWithExceptionCode() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->any()) + ->method('isGranted') + ->willReturn(false); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'exceptionCodeInHttpException'], + [], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + + $this->expectException(HttpException::class); + $this->expectExceptionMessage('Exception Code'); + $this->expectExceptionCode(10010); + + $listener->onKernelControllerArguments($event); + } + + public function testAccessDeniedExceptionWithExceptionCode() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->any()) + ->method('isGranted') + ->willReturn(false); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsWithCallableController(), 'exceptionCodeInAccessDeniedException'], + [], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + + $this->expectException(AccessDeniedException::class); + $this->expectExceptionMessage('Exception Code'); + $this->expectExceptionCode(10010); + + $listener->onKernelControllerArguments($event); + } +} diff --git a/Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php b/Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php new file mode 100644 index 00000000..8fab789c --- /dev/null +++ b/Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php @@ -0,0 +1,95 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Tests\Fixtures; + +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Http\Attribute\IsGranted; + +class IsGrantedAttributeMethodsWithCallableController +{ + public function noAttribute() + { + } + + #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { + return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); + })] + public function admin() + { + } + + #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { + return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); + }, subject: 'arg2Name')] + public function withSubject($arg1Name, $arg2Name) + { + } + + #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { + return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); + }, subject: ['arg1Name', 'arg2Name'])] + public function withSubjectArray($arg1Name, $arg2Name) + { + } + + #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { + return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); + }, subject: 'non_existent')] + public function withMissingSubject() + { + } + + #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { + return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); + }, message: 'Not found', statusCode: 404)] + public function notFound() + { + } + + #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { + return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); + }, message: 'Exception Code Http', statusCode: 404, exceptionCode: 10010)] + public function exceptionCodeInHttpException() + { + } + + #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { + return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); + }, message: 'Exception Code Access Denied', exceptionCode: 10010)] + public function exceptionCodeInAccessDeniedException() + { + } + + #[IsGranted( + static function (TokenInterface $token, $subject, ...$vars) { + return $token->getUser() === $subject; + }, + subject: static function (array $args) { + return $args['post']; + } + )] + public function withCallableAsSubject($post) + { + } + + #[IsGranted(static function ($token, $subject, ...$vars) { + return $token->getUser() === $subject['author']; + }, subject: static function (array $args) { + return [ + 'author' => $args['post'], + 'alias' => 'bar', + ]; + })] + public function withNestArgsInSubject($post, $arg2Name) + { + } +} diff --git a/Tests/Fixtures/IsGrantedAttributeWithCallableController.php b/Tests/Fixtures/IsGrantedAttributeWithCallableController.php new file mode 100644 index 00000000..72c585d1 --- /dev/null +++ b/Tests/Fixtures/IsGrantedAttributeWithCallableController.php @@ -0,0 +1,31 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Tests\Fixtures; + +use Symfony\Component\Security\Http\Attribute\IsGranted; + +#[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { + return $accessDecisionManager->decide($token, ['ROLE_USER']); +})] +class IsGrantedAttributeWithCallableController +{ + #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { + return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); + })] + public function foo() + { + } + + public function bar() + { + } +} From 3728d69104d79b51257f2a5de3348d74da709f4b Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 17 Feb 2025 16:31:27 +0100 Subject: [PATCH 11/23] [Security] Improve DX of recent additions --- Attribute/IsGranted.php | 7 +- Attribute/IsGrantedContext.php | 48 +++++++++ EventListener/IsGrantedAttributeListener.php | 8 +- ...antedAttributeWithClosureListenerTest.php} | 61 ++++++------ ...AttributeMethodsWithCallableController.php | 95 ------------------ ...dAttributeMethodsWithClosureController.php | 98 +++++++++++++++++++ ...GrantedAttributeWithClosureController.php} | 11 ++- 7 files changed, 191 insertions(+), 137 deletions(-) create mode 100644 Attribute/IsGrantedContext.php rename Tests/EventListener/{IsGrantedAttributeWithCallableListenerTest.php => IsGrantedAttributeWithClosureListenerTest.php} (78%) delete mode 100644 Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php create mode 100644 Tests/Fixtures/IsGrantedAttributeMethodsWithClosureController.php rename Tests/Fixtures/{IsGrantedAttributeWithCallableController.php => IsGrantedAttributeWithClosureController.php} (57%) diff --git a/Attribute/IsGranted.php b/Attribute/IsGranted.php index 546f293b..7f3fef69 100644 --- a/Attribute/IsGranted.php +++ b/Attribute/IsGranted.php @@ -13,9 +13,6 @@ use Symfony\Component\ExpressionLanguage\Expression; use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface; -use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; -use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; /** * Checks if user has permission to access to some resource using security roles and voters. @@ -28,8 +25,8 @@ final class IsGranted { /** - * @param string|Expression|(\Closure(TokenInterface $token, mixed $subject, AccessDecisionManagerInterface $accessDecisionManager, AuthenticationTrustResolverInterface $trustResolver): bool) $attribute The attribute that will be checked against a given authentication token and optional subject - * @param array|string|Expression|(\Closure(array, Request): mixed)|null $subject An optional subject - e.g. the current object being voted on + * @param string|Expression|\Closure(IsGrantedContext, mixed $subject):bool $attribute The attribute that will be checked against a given authentication token and optional subject + * @param array|string|Expression|\Closure(array, Request):mixed|null $subject An optional subject - e.g. the current object being voted on * @param string|null $message A custom message when access is not granted * @param int|null $statusCode If set, will throw HttpKernel's HttpException with the given $statusCode; if null, Security\Core's AccessDeniedException will be used * @param int|null $exceptionCode If set, will add the exception code to thrown exception diff --git a/Attribute/IsGrantedContext.php b/Attribute/IsGrantedContext.php new file mode 100644 index 00000000..fa2ce4a0 --- /dev/null +++ b/Attribute/IsGrantedContext.php @@ -0,0 +1,48 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Attribute; + +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecision; +use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter; +use Symfony\Component\Security\Core\User\UserInterface; + +readonly class IsGrantedContext implements AuthorizationCheckerInterface +{ + public function __construct( + public TokenInterface $token, + public ?UserInterface $user, + private AuthorizationCheckerInterface $authorizationChecker, + ) { + } + + public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool + { + return $this->authorizationChecker->isGranted($attribute, $subject, $accessDecision); + } + + public function isAuthenticated(): bool + { + return $this->authorizationChecker->isGranted(AuthenticatedVoter::IS_AUTHENTICATED); + } + + public function isAuthenticatedFully(): bool + { + return $this->authorizationChecker->isGranted(AuthenticatedVoter::IS_AUTHENTICATED_FULLY); + } + + public function isImpersonator(): bool + { + return $this->authorizationChecker->isGranted(AuthenticatedVoter::IS_IMPERSONATOR); + } +} diff --git a/EventListener/IsGrantedAttributeListener.php b/EventListener/IsGrantedAttributeListener.php index e79a2e94..607643ce 100644 --- a/EventListener/IsGrantedAttributeListener.php +++ b/EventListener/IsGrantedAttributeListener.php @@ -55,8 +55,6 @@ public function onKernelControllerArguments(ControllerArgumentsEvent $event): vo foreach ($subjectRef as $refKey => $ref) { $subject[\is_string($refKey) ? $refKey : (string) $ref] = $this->getIsGrantedSubject($ref, $request, $arguments); } - } elseif ($subjectRef instanceof \Closure) { - $subject = $subjectRef($arguments, $request); } else { $subject = $this->getIsGrantedSubject($subjectRef, $request, $arguments); } @@ -85,8 +83,12 @@ public static function getSubscribedEvents(): array return [KernelEvents::CONTROLLER_ARGUMENTS => ['onKernelControllerArguments', 20]]; } - private function getIsGrantedSubject(string|Expression $subjectRef, Request $request, array $arguments): mixed + private function getIsGrantedSubject(string|Expression|\Closure $subjectRef, Request $request, array $arguments): mixed { + if ($subjectRef instanceof \Closure) { + return $subjectRef($arguments, $request); + } + if ($subjectRef instanceof Expression) { $this->expressionLanguage ??= new ExpressionLanguage(); diff --git a/Tests/EventListener/IsGrantedAttributeWithCallableListenerTest.php b/Tests/EventListener/IsGrantedAttributeWithClosureListenerTest.php similarity index 78% rename from Tests/EventListener/IsGrantedAttributeWithCallableListenerTest.php rename to Tests/EventListener/IsGrantedAttributeWithClosureListenerTest.php index 8b80736e..2ea375ae 100644 --- a/Tests/EventListener/IsGrantedAttributeWithCallableListenerTest.php +++ b/Tests/EventListener/IsGrantedAttributeWithClosureListenerTest.php @@ -16,16 +16,20 @@ use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent; use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; +use Symfony\Component\Security\Core\Authorization\AccessDecisionManager; +use Symfony\Component\Security\Core\Authorization\AuthorizationChecker; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Authorization\Voter\ClosureVoter; use Symfony\Component\Security\Core\Exception\AccessDeniedException; use Symfony\Component\Security\Http\EventListener\IsGrantedAttributeListener; -use Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController; -use Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeWithCallableController; +use Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithClosureController; +use Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeWithClosureController; /** * @requires PHP 8.5 */ -class IsGrantedAttributeWithCallableListenerTest extends TestCase +class IsGrantedAttributeWithClosureListenerTest extends TestCase { public function testAttribute() { @@ -36,7 +40,7 @@ public function testAttribute() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeWithCallableController(), 'foo'], + [new IsGrantedAttributeWithClosureController(), 'foo'], [], new Request(), null @@ -52,7 +56,7 @@ public function testAttribute() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeWithCallableController(), 'bar'], + [new IsGrantedAttributeWithClosureController(), 'bar'], [], new Request(), null @@ -70,7 +74,7 @@ public function testNothingHappensWithNoConfig() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'noAttribute'], + [new IsGrantedAttributeMethodsWithClosureController(), 'noAttribute'], [], new Request(), null @@ -90,7 +94,7 @@ public function testIsGrantedCalledCorrectly() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'admin'], + [new IsGrantedAttributeMethodsWithClosureController(), 'admin'], [], new Request(), null @@ -111,7 +115,7 @@ public function testIsGrantedSubjectFromArguments() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'withSubject'], + [new IsGrantedAttributeMethodsWithClosureController(), 'withSubject'], ['arg1Value', 'arg2Value'], new Request(), null @@ -136,7 +140,7 @@ public function testIsGrantedSubjectFromArgumentsWithArray() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'withSubjectArray'], + [new IsGrantedAttributeMethodsWithClosureController(), 'withSubjectArray'], ['arg1Value', 'arg2Value'], new Request(), null @@ -157,7 +161,7 @@ public function testIsGrantedNullSubjectFromArguments() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'withSubject'], + [new IsGrantedAttributeMethodsWithClosureController(), 'withSubject'], ['arg1Value', null], new Request(), null @@ -180,7 +184,7 @@ public function testIsGrantedArrayWithNullValueSubjectFromArguments() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'withSubjectArray'], + [new IsGrantedAttributeMethodsWithClosureController(), 'withSubjectArray'], ['arg1Value', null], new Request(), null @@ -196,7 +200,7 @@ public function testExceptionWhenMissingSubjectAttribute() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'withMissingSubject'], + [new IsGrantedAttributeMethodsWithClosureController(), 'withMissingSubject'], [], new Request(), null @@ -214,10 +218,9 @@ public function testExceptionWhenMissingSubjectAttribute() */ public function testAccessDeniedMessages(string|array|null $subject, string $method, int $numOfArguments, string $expectedMessage) { - $authChecker = $this->createMock(AuthorizationCheckerInterface::class); - $authChecker->expects($this->any()) - ->method('isGranted') - ->willReturn(false); + $authChecker = new AuthorizationChecker(new TokenStorage(), new AccessDecisionManager((function () use (&$authChecker) { + yield new ClosureVoter($authChecker); + })())); // avoid the error of the subject not being found in the request attributes $arguments = array_fill(0, $numOfArguments, 'bar'); @@ -225,7 +228,7 @@ public function testAccessDeniedMessages(string|array|null $subject, string $met $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), $method], + [new IsGrantedAttributeMethodsWithClosureController(), $method], $arguments, new Request(), null @@ -236,7 +239,7 @@ public function testAccessDeniedMessages(string|array|null $subject, string $met $this->fail(); } catch (AccessDeniedException $e) { $this->assertSame($expectedMessage, $e->getMessage()); - $this->assertIsCallable($e->getAttributes()[0]); + $this->assertInstanceOf(\Closure::class, $e->getAttributes()[0]); if (null !== $subject) { $this->assertSame($subject, $e->getSubject()); } else { @@ -247,11 +250,11 @@ public function testAccessDeniedMessages(string|array|null $subject, string $met public static function getAccessDeniedMessageTests() { - yield [null, 'admin', 0, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::admin():23})] on controller']; - yield ['bar', 'withSubject', 2, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withSubject():30}, "arg2Name")] on controller']; - yield [['arg1Name' => 'bar', 'arg2Name' => 'bar'], 'withSubjectArray', 2, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withSubjectArray():37}, ["arg1Name", "arg2Name"])] on controller']; - yield ['bar', 'withCallableAsSubject', 1, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withCallableAsSubject():73}, {closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withCallableAsSubject():76})] on controller']; - yield [['author' => 'bar', 'alias' => 'bar'], 'withNestArgsInSubject', 2, 'Access Denied by #[IsGranted({closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withNestArgsInSubject():84}, {closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithCallableController::withNestArgsInSubject():86})] on controller']; + yield [null, 'admin', 0, 'Access Denied. Closure {closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithClosureController::admin():23} returned false.']; + yield ['bar', 'withSubject', 2, 'Access Denied. Closure {closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithClosureController::withSubject():30} returned false.']; + yield [['arg1Name' => 'bar', 'arg2Name' => 'bar'], 'withSubjectArray', 2, 'Access Denied. Closure {closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithClosureController::withSubjectArray():37} returned false.']; + yield ['bar', 'withClosureAsSubject', 1, 'Access Denied. Closure {closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithClosureController::withClosureAsSubject():73} returned false.']; + yield [['author' => 'bar', 'alias' => 'bar'], 'withNestArgsInSubject', 2, 'Access Denied. Closure {closure:Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeMethodsWithClosureController::withNestArgsInSubject():85} returned false.']; } public function testNotFoundHttpException() @@ -263,7 +266,7 @@ public function testNotFoundHttpException() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'notFound'], + [new IsGrantedAttributeMethodsWithClosureController(), 'notFound'], [], new Request(), null @@ -277,7 +280,7 @@ public function testNotFoundHttpException() $listener->onKernelControllerArguments($event); } - public function testIsGrantedWithCallableAsSubject() + public function testIsGrantedWithClosureAsSubject() { $request = new Request(); @@ -289,7 +292,7 @@ public function testIsGrantedWithCallableAsSubject() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'withCallableAsSubject'], + [new IsGrantedAttributeMethodsWithClosureController(), 'withClosureAsSubject'], ['postVal'], $request, null @@ -311,7 +314,7 @@ public function testIsGrantedWithNestedExpressionInSubject() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'withNestArgsInSubject'], + [new IsGrantedAttributeMethodsWithClosureController(), 'withNestArgsInSubject'], ['postVal', 'bar'], $request, null @@ -330,7 +333,7 @@ public function testHttpExceptionWithExceptionCode() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'exceptionCodeInHttpException'], + [new IsGrantedAttributeMethodsWithClosureController(), 'exceptionCodeInHttpException'], [], new Request(), null @@ -354,7 +357,7 @@ public function testAccessDeniedExceptionWithExceptionCode() $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), - [new IsGrantedAttributeMethodsWithCallableController(), 'exceptionCodeInAccessDeniedException'], + [new IsGrantedAttributeMethodsWithClosureController(), 'exceptionCodeInAccessDeniedException'], [], new Request(), null diff --git a/Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php b/Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php deleted file mode 100644 index 8fab789c..00000000 --- a/Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php +++ /dev/null @@ -1,95 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\Security\Http\Tests\Fixtures; - -use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; -use Symfony\Component\Security\Http\Attribute\IsGranted; - -class IsGrantedAttributeMethodsWithCallableController -{ - public function noAttribute() - { - } - - #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { - return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); - })] - public function admin() - { - } - - #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { - return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); - }, subject: 'arg2Name')] - public function withSubject($arg1Name, $arg2Name) - { - } - - #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { - return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); - }, subject: ['arg1Name', 'arg2Name'])] - public function withSubjectArray($arg1Name, $arg2Name) - { - } - - #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { - return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); - }, subject: 'non_existent')] - public function withMissingSubject() - { - } - - #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { - return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); - }, message: 'Not found', statusCode: 404)] - public function notFound() - { - } - - #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { - return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); - }, message: 'Exception Code Http', statusCode: 404, exceptionCode: 10010)] - public function exceptionCodeInHttpException() - { - } - - #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { - return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); - }, message: 'Exception Code Access Denied', exceptionCode: 10010)] - public function exceptionCodeInAccessDeniedException() - { - } - - #[IsGranted( - static function (TokenInterface $token, $subject, ...$vars) { - return $token->getUser() === $subject; - }, - subject: static function (array $args) { - return $args['post']; - } - )] - public function withCallableAsSubject($post) - { - } - - #[IsGranted(static function ($token, $subject, ...$vars) { - return $token->getUser() === $subject['author']; - }, subject: static function (array $args) { - return [ - 'author' => $args['post'], - 'alias' => 'bar', - ]; - })] - public function withNestArgsInSubject($post, $arg2Name) - { - } -} diff --git a/Tests/Fixtures/IsGrantedAttributeMethodsWithClosureController.php b/Tests/Fixtures/IsGrantedAttributeMethodsWithClosureController.php new file mode 100644 index 00000000..bf001c53 --- /dev/null +++ b/Tests/Fixtures/IsGrantedAttributeMethodsWithClosureController.php @@ -0,0 +1,98 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Tests\Fixtures; + +use Symfony\Component\Security\Http\Attribute\IsGranted; +use Symfony\Component\Security\Http\Attribute\IsGrantedContext; + +class IsGrantedAttributeMethodsWithClosureController +{ + public function noAttribute() + { + } + + #[IsGranted(static function (IsGrantedContext $context) { + return $context->isGranted('ROLE_ADMIN'); + })] + public function admin() + { + } + + #[IsGranted(static function (IsGrantedContext $context) { + return $context->isGranted('ROLE_ADMIN'); + }, subject: 'arg2Name')] + public function withSubject($arg1Name, $arg2Name) + { + } + + #[IsGranted(static function (IsGrantedContext $context) { + return $context->isGranted('ROLE_ADMIN'); + }, subject: ['arg1Name', 'arg2Name'])] + public function withSubjectArray($arg1Name, $arg2Name) + { + } + + #[IsGranted(static function (IsGrantedContext $context) { + return $context->isGranted('ROLE_ADMIN'); + }, subject: 'non_existent')] + public function withMissingSubject() + { + } + + #[IsGranted(static function (IsGrantedContext $context) { + return $context->isGranted('ROLE_ADMIN'); + }, message: 'Not found', statusCode: 404)] + public function notFound() + { + } + + #[IsGranted(static function (IsGrantedContext $context) { + return $context->isGranted('ROLE_ADMIN'); + }, message: 'Exception Code Http', statusCode: 404, exceptionCode: 10010)] + public function exceptionCodeInHttpException() + { + } + + #[IsGranted(static function (IsGrantedContext $context) { + return $context->isGranted('ROLE_ADMIN'); + }, message: 'Exception Code Access Denied', exceptionCode: 10010)] + public function exceptionCodeInAccessDeniedException() + { + } + + #[IsGranted( + static function (IsGrantedContext $context, mixed $subject) { + return $context->user === $subject; + }, + subject: static function (array $args) { + return $args['post']; + } + )] + public function withClosureAsSubject($post) + { + } + + #[IsGranted( + static function (IsGrantedContext $context, array $subject) { + return $context->user === $subject['author']; + }, + subject: static function (array $args) { + return [ + 'author' => $args['post'], + 'alias' => 'bar', + ]; + } + )] + public function withNestArgsInSubject($post, $arg2Name) + { + } +} diff --git a/Tests/Fixtures/IsGrantedAttributeWithCallableController.php b/Tests/Fixtures/IsGrantedAttributeWithClosureController.php similarity index 57% rename from Tests/Fixtures/IsGrantedAttributeWithCallableController.php rename to Tests/Fixtures/IsGrantedAttributeWithClosureController.php index 72c585d1..61a1ddbc 100644 --- a/Tests/Fixtures/IsGrantedAttributeWithCallableController.php +++ b/Tests/Fixtures/IsGrantedAttributeWithClosureController.php @@ -12,14 +12,15 @@ namespace Symfony\Component\Security\Http\Tests\Fixtures; use Symfony\Component\Security\Http\Attribute\IsGranted; +use Symfony\Component\Security\Http\Attribute\IsGrantedContext; -#[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { - return $accessDecisionManager->decide($token, ['ROLE_USER']); +#[IsGranted(static function (IsGrantedContext $context) { + return $context->isGranted('ROLE_USER'); })] -class IsGrantedAttributeWithCallableController +class IsGrantedAttributeWithClosureController { - #[IsGranted(static function ($token, $accessDecisionManager, ...$vars) { - return $accessDecisionManager->decide($token, ['ROLE_ADMIN']); + #[IsGranted(static function (IsGrantedContext $context) { + return $context->isGranted('ROLE_ADMIN'); })] public function foo() { From 0af71b67cb85cdbbde159713c0e13025ab6fc211 Mon Sep 17 00:00:00 2001 From: Florent Morselli Date: Thu, 13 Feb 2025 09:05:26 +0100 Subject: [PATCH 12/23] [Security] OAuth2 Introspection Endpoint (RFC7662) In addition to the excellent work of @vincentchalamon #48272, this PR allows getting the data from the OAuth2 Introspection Endpoint. This endpoint is defined in the [RFC7662](https://datatracker.ietf.org/doc/html/rfc7662). It returns the following information that is used to retrieve the user: * If the access token is active * A set of claims that are similar to the OIDC one, including the `sub` or the `username`. --- AccessToken/OAuth2/Oauth2TokenHandler.php | 100 ++++++++++++++++++ CHANGELOG.md | 1 + .../OAuth2/OAuth2TokenHandlerTest.php | 52 +++++++++ 3 files changed, 153 insertions(+) create mode 100644 AccessToken/OAuth2/Oauth2TokenHandler.php create mode 100644 Tests/AccessToken/OAuth2/OAuth2TokenHandlerTest.php diff --git a/AccessToken/OAuth2/Oauth2TokenHandler.php b/AccessToken/OAuth2/Oauth2TokenHandler.php new file mode 100644 index 00000000..05bda02f --- /dev/null +++ b/AccessToken/OAuth2/Oauth2TokenHandler.php @@ -0,0 +1,100 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\AccessToken\OAuth2; + +use Psr\Log\LoggerInterface; +use Symfony\Component\Security\Core\Exception\AuthenticationException; +use Symfony\Component\Security\Core\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\User\OAuth2User; +use Symfony\Component\Security\Http\AccessToken\AccessTokenHandlerInterface; +use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; +use Symfony\Contracts\HttpClient\HttpClientInterface; + +use function Symfony\Component\String\u; + +/** + * The token handler validates the token on the authorization server and the Introspection Endpoint. + * + * @see https://tools.ietf.org/html/rfc7662 + * + * @internal + */ +final class Oauth2TokenHandler implements AccessTokenHandlerInterface +{ + public function __construct( + private readonly HttpClientInterface $client, + private readonly ?LoggerInterface $logger = null, + ) { + } + + public function getUserBadgeFrom(string $accessToken): UserBadge + { + try { + // Call the Authorization server to retrieve the resource owner details + // If the token is invalid or expired, the Authorization server will return an error + $claims = $this->client->request('POST', '', [ + 'body' => [ + 'token' => $accessToken, + 'token_type_hint' => 'access_token', + ], + ])->toArray(); + + $sub = $claims['sub'] ?? null; + $username = $claims['username'] ?? null; + if (!$sub && !$username) { + throw new BadCredentialsException('"sub" and "username" claims not found on the authorization server response. At least one is required.'); + } + $active = $claims['active'] ?? false; + if (!$active) { + throw new BadCredentialsException('The claim "active" was not found on the authorization server response or is set to false.'); + } + + return new UserBadge($sub ?? $username, fn () => $this->createUser($claims), $claims); + } catch (AuthenticationException $e) { + $this->logger?->error('An error occurred on the authorization server.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); + + throw new BadCredentialsException('Invalid credentials.', $e->getCode(), $e); + } + } + + private function createUser(array $claims): OAuth2User + { + if (!\function_exists(\Symfony\Component\String\u::class)) { + throw new \LogicException('You cannot use the "OAuth2TokenHandler" since the String component is not installed. Try running "composer require symfony/string".'); + } + + foreach ($claims as $claim => $value) { + unset($claims[$claim]); + if ('' === $value || null === $value) { + continue; + } + $claims[u($claim)->camel()->toString()] = $value; + } + + if ('' !== ($claims['updatedAt'] ?? '')) { + $claims['updatedAt'] = (new \DateTimeImmutable())->setTimestamp($claims['updatedAt']); + } + + if ('' !== ($claims['emailVerified'] ?? '')) { + $claims['emailVerified'] = (bool) $claims['emailVerified']; + } + + if ('' !== ($claims['phoneNumberVerified'] ?? '')) { + $claims['phoneNumberVerified'] = (bool) $claims['phoneNumberVerified']; + } + + return new OAuth2User(...$claims); + } +} diff --git a/CHANGELOG.md b/CHANGELOG.md index 22fe59f6..5fe39838 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ CHANGELOG * Add argument `$identifierNormalizer` to `UserBadge::__construct()` to allow normalizing the identifier * Support hashing the hashed password using crc32c when putting the user in the session * Add support for closures in `#[IsGranted]` + * Add `OAuth2TokenHandler` with OAuth2 Token Introspection support for `AccessTokenAuthenticator` 7.2 --- diff --git a/Tests/AccessToken/OAuth2/OAuth2TokenHandlerTest.php b/Tests/AccessToken/OAuth2/OAuth2TokenHandlerTest.php new file mode 100644 index 00000000..c6538ff7 --- /dev/null +++ b/Tests/AccessToken/OAuth2/OAuth2TokenHandlerTest.php @@ -0,0 +1,52 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Tests\AccessToken\OAuth2; + +use PHPUnit\Framework\TestCase; +use Symfony\Component\HttpClient\MockHttpClient; +use Symfony\Component\HttpClient\Response\MockResponse; +use Symfony\Component\Security\Core\User\OAuth2User; +use Symfony\Component\Security\Http\AccessToken\OAuth2\Oauth2TokenHandler; +use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; + +class OAuth2TokenHandlerTest extends TestCase +{ + public static function testGetsUserIdentifierFromOAuth2ServerResponse(): void + { + $accessToken = 'a-secret-token'; + $claims = [ + 'active' => true, + 'client_id' => 'l238j323ds-23ij4', + 'username' => 'jdoe', + 'scope' => 'read write dolphin', + 'sub' => 'Z5O3upPC88QrAjx00dis', + 'aud' => 'https://protected.example.net/resource', + 'iss' => 'https://server.example.com/', + 'exp' => 1419356238, + 'iat' => 1419350238, + 'extension_field' => 'twenty-seven', + ]; + $expectedUser = new OAuth2User(...$claims); + + $client = new MockHttpClient([ + new MockResponse(json_encode($claims, \JSON_THROW_ON_ERROR)), + ]); + + $userBadge = (new Oauth2TokenHandler($client))->getUserBadgeFrom($accessToken); + $actualUser = $userBadge->getUserLoader()(); + + self::assertEquals(new UserBadge('Z5O3upPC88QrAjx00dis', fn () => $expectedUser, $claims), $userBadge); + self::assertInstanceOf(OAuth2User::class, $actualUser); + self::assertSame($claims, $userBadge->getAttributes()); + self::assertSame($claims['sub'], $actualUser->getUserIdentifier()); + } +} From e4e86018888d2b522e18882bb1ed109d7f68ab40 Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Tue, 14 May 2024 13:34:06 +0200 Subject: [PATCH 13/23] feat(security): OIDC discovery --- AccessToken/Oidc/OidcTokenHandler.php | 64 +++++++++++++++++-- AccessToken/Oidc/OidcUserInfoTokenHandler.php | 31 ++++++++- CHANGELOG.md | 1 + 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/AccessToken/Oidc/OidcTokenHandler.php b/AccessToken/Oidc/OidcTokenHandler.php index 8260470c..b656cbc8 100644 --- a/AccessToken/Oidc/OidcTokenHandler.php +++ b/AccessToken/Oidc/OidcTokenHandler.php @@ -34,6 +34,8 @@ use Symfony\Component\Security\Http\AccessToken\Oidc\Exception\MissingClaimException; use Symfony\Component\Security\Http\Authenticator\FallbackUserLoader; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; +use Symfony\Contracts\Cache\CacheInterface; +use Symfony\Contracts\HttpClient\HttpClientInterface; /** * The token handler decodes and validates the token, and retrieves the user identifier from it. @@ -45,9 +47,14 @@ final class OidcTokenHandler implements AccessTokenHandlerInterface private ?AlgorithmManager $decryptionAlgorithms = null; private bool $enforceEncryption = false; + private ?CacheInterface $discoveryCache = null; + private ?HttpClientInterface $discoveryClient = null; + private ?string $oidcConfigurationCacheKey = null; + private ?string $oidcJWKSetCacheKey = null; + public function __construct( private Algorithm|AlgorithmManager $signatureAlgorithm, - private JWK|JWKSet $signatureKeyset, + private JWK|JWKSet|null $signatureKeyset, private string $audience, private array $issuers, private string $claim = 'sub', @@ -71,15 +78,64 @@ public function enabledJweSupport(JWKSet $decryptionKeyset, AlgorithmManager $de $this->enforceEncryption = $enforceEncryption; } + public function enabledDiscovery(CacheInterface $cache, HttpClientInterface $client, string $oidcConfigurationCacheKey, string $oidcJWKSetCacheKey): void + { + $this->discoveryCache = $cache; + $this->discoveryClient = $client; + $this->oidcConfigurationCacheKey = $oidcConfigurationCacheKey; + $this->oidcJWKSetCacheKey = $oidcJWKSetCacheKey; + } + public function getUserBadgeFrom(string $accessToken): UserBadge { if (!class_exists(JWSVerifier::class) || !class_exists(Checker\HeaderCheckerManager::class)) { throw new \LogicException('You cannot use the "oidc" token handler since "web-token/jwt-signature" and "web-token/jwt-checker" are not installed. Try running "composer require web-token/jwt-signature web-token/jwt-checker".'); } + if (!$this->discoveryCache && !$this->signatureKeyset) { + throw new \LogicException('You cannot use the "oidc" token handler without JWKSet nor "discovery". Please configure JWKSet in the constructor, or call "enableDiscovery" method.'); + } + + $jwkset = $this->signatureKeyset; + if ($this->discoveryCache) { + try { + $oidcConfiguration = json_decode($this->discoveryCache->get($this->oidcConfigurationCacheKey, function (): string { + $response = $this->discoveryClient->request('GET', '.well-known/openid-configuration'); + + return $response->getContent(); + }), true, 512, \JSON_THROW_ON_ERROR); + } catch (\Throwable $e) { + $this->logger?->error('An error occurred while requesting OIDC configuration.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); + + throw new BadCredentialsException('Invalid credentials.', $e->getCode(), $e); + } + + try { + $jwkset = JWKSet::createFromJson( + $this->discoveryCache->get($this->oidcJWKSetCacheKey, function () use ($oidcConfiguration): string { + $response = $this->discoveryClient->request('GET', $oidcConfiguration['jwks_uri']); + // we only need signature key + $keys = array_filter($response->toArray()['keys'], static fn (array $key) => 'sig' === $key['use']); + + return json_encode(['keys' => $keys]); + }) + ); + } catch (\Throwable $e) { + $this->logger?->error('An error occurred while requesting OIDC certs.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); + + throw new BadCredentialsException('Invalid credentials.', $e->getCode(), $e); + } + } + try { $accessToken = $this->decryptIfNeeded($accessToken); - $claims = $this->loadAndVerifyJws($accessToken); + $claims = $this->loadAndVerifyJws($accessToken, $jwkset); $this->verifyClaims($claims); if (empty($claims[$this->claim])) { @@ -98,7 +154,7 @@ public function getUserBadgeFrom(string $accessToken): UserBadge } } - private function loadAndVerifyJws(string $accessToken): array + private function loadAndVerifyJws(string $accessToken, JWKSet $jwkset): array { // Decode the token $jwsVerifier = new JWSVerifier($this->signatureAlgorithm); @@ -106,7 +162,7 @@ private function loadAndVerifyJws(string $accessToken): array $jws = $serializerManager->unserialize($accessToken); // Verify the signature - if (!$jwsVerifier->verifyWithKeySet($jws, $this->signatureKeyset, 0)) { + if (!$jwsVerifier->verifyWithKeySet($jws, $jwkset, 0)) { throw new InvalidSignatureException(); } diff --git a/AccessToken/Oidc/OidcUserInfoTokenHandler.php b/AccessToken/Oidc/OidcUserInfoTokenHandler.php index 855d2955..2ab7c8a5 100644 --- a/AccessToken/Oidc/OidcUserInfoTokenHandler.php +++ b/AccessToken/Oidc/OidcUserInfoTokenHandler.php @@ -17,6 +17,7 @@ use Symfony\Component\Security\Http\AccessToken\Oidc\Exception\MissingClaimException; use Symfony\Component\Security\Http\Authenticator\FallbackUserLoader; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; +use Symfony\Contracts\Cache\CacheInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; /** @@ -26,6 +27,9 @@ final class OidcUserInfoTokenHandler implements AccessTokenHandlerInterface { use OidcTrait; + private ?CacheInterface $discoveryCache = null; + private ?string $oidcConfigurationCacheKey = null; + public function __construct( private HttpClientInterface $client, private ?LoggerInterface $logger = null, @@ -33,12 +37,37 @@ public function __construct( ) { } + public function enabledDiscovery(CacheInterface $cache, string $oidcConfigurationCacheKey): void + { + $this->discoveryCache = $cache; + $this->oidcConfigurationCacheKey = $oidcConfigurationCacheKey; + } + public function getUserBadgeFrom(string $accessToken): UserBadge { + if (null !== $this->discoveryCache) { + try { + // Call OIDC discovery to retrieve userinfo endpoint + // OIDC configuration is stored in cache + $oidcConfiguration = json_decode($this->discoveryCache->get($this->oidcConfigurationCacheKey, function (): string { + $response = $this->client->request('GET', '.well-known/openid-configuration'); + + return $response->getContent(); + }), true, 512, \JSON_THROW_ON_ERROR); + } catch (\Throwable $e) { + $this->logger?->error('An error occurred while requesting OIDC configuration.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); + + throw new BadCredentialsException('Invalid credentials.', $e->getCode(), $e); + } + } + try { // Call the OIDC server to retrieve the user info // If the token is invalid or expired, the OIDC server will return an error - $claims = $this->client->request('GET', '', [ + $claims = $this->client->request('GET', $this->discoveryCache ? $oidcConfiguration['userinfo_endpoint'] : '', [ 'auth_bearer' => $accessToken, ])->toArray(); diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fe39838..275180ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ CHANGELOG * Support hashing the hashed password using crc32c when putting the user in the session * Add support for closures in `#[IsGranted]` * Add `OAuth2TokenHandler` with OAuth2 Token Introspection support for `AccessTokenAuthenticator` + * Add discovery support to `OidcTokenHandler` and `OidcUserInfoTokenHandler` 7.2 --- From edaeca7383a96d1d618df4a136bc6351f14acd53 Mon Sep 17 00:00:00 2001 From: Christian Flothmann Date: Sun, 2 Mar 2025 16:03:52 +0100 Subject: [PATCH 14/23] replace assertEmpty() with stricter assertions --- Tests/Firewall/ContextListenerTest.php | 4 ++-- Tests/Firewall/ExceptionListenerTest.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/Firewall/ContextListenerTest.php b/Tests/Firewall/ContextListenerTest.php index 7c3248c7..585fca8a 100644 --- a/Tests/Firewall/ContextListenerTest.php +++ b/Tests/Firewall/ContextListenerTest.php @@ -368,13 +368,13 @@ public function testOnKernelResponseRemoveListener() $httpKernel = $this->createMock(HttpKernelInterface::class); $listener = new ContextListener($tokenStorage, [], 'session', null, $dispatcher, null, $tokenStorage->getToken(...)); - $this->assertEmpty($dispatcher->getListeners()); + $this->assertSame([], $dispatcher->getListeners()); $listener(new RequestEvent($httpKernel, $request, HttpKernelInterface::MAIN_REQUEST)); $this->assertNotEmpty($dispatcher->getListeners()); $listener->onKernelResponse(new ResponseEvent($httpKernel, $request, HttpKernelInterface::MAIN_REQUEST, new Response())); - $this->assertEmpty($dispatcher->getListeners()); + $this->assertSame([], $dispatcher->getListeners()); } /** diff --git a/Tests/Firewall/ExceptionListenerTest.php b/Tests/Firewall/ExceptionListenerTest.php index 8bcc9588..07978379 100644 --- a/Tests/Firewall/ExceptionListenerTest.php +++ b/Tests/Firewall/ExceptionListenerTest.php @@ -166,7 +166,7 @@ public function testUnregister() $this->assertNotEmpty($dispatcher->getListeners()); $listener->unregister($dispatcher); - $this->assertEmpty($dispatcher->getListeners()); + $this->assertSame([], $dispatcher->getListeners()); } public static function getAccessDeniedExceptionProvider() From cf7a7ec8b6a91fca38f355a2e47eb0b992447f33 Mon Sep 17 00:00:00 2001 From: Dariusz Ruminski Date: Sat, 8 Mar 2025 00:16:33 +0100 Subject: [PATCH 15/23] chore: PHP CS Fixer fixes --- AccessToken/OAuth2/Oauth2TokenHandler.php | 1 - AccessToken/Oidc/OidcTrait.php | 1 - Tests/Authenticator/Passport/Badge/UserBadgeTest.php | 1 - 3 files changed, 3 deletions(-) diff --git a/AccessToken/OAuth2/Oauth2TokenHandler.php b/AccessToken/OAuth2/Oauth2TokenHandler.php index 05bda02f..37f36d4d 100644 --- a/AccessToken/OAuth2/Oauth2TokenHandler.php +++ b/AccessToken/OAuth2/Oauth2TokenHandler.php @@ -18,7 +18,6 @@ use Symfony\Component\Security\Http\AccessToken\AccessTokenHandlerInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; use Symfony\Contracts\HttpClient\HttpClientInterface; - use function Symfony\Component\String\u; /** diff --git a/AccessToken/Oidc/OidcTrait.php b/AccessToken/Oidc/OidcTrait.php index d5ab61ff..be797711 100644 --- a/AccessToken/Oidc/OidcTrait.php +++ b/AccessToken/Oidc/OidcTrait.php @@ -12,7 +12,6 @@ namespace Symfony\Component\Security\Http\AccessToken\Oidc; use Symfony\Component\Security\Core\User\OidcUser; - use function Symfony\Component\String\u; /** diff --git a/Tests/Authenticator/Passport/Badge/UserBadgeTest.php b/Tests/Authenticator/Passport/Badge/UserBadgeTest.php index f648d048..c910e454 100644 --- a/Tests/Authenticator/Passport/Badge/UserBadgeTest.php +++ b/Tests/Authenticator/Passport/Badge/UserBadgeTest.php @@ -17,7 +17,6 @@ use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; use Symfony\Component\String\Slugger\AsciiSlugger; use Symfony\Component\String\UnicodeString; - use function Symfony\Component\String\u; class UserBadgeTest extends TestCase From 174fce615d8f396fdb792bf27e77e5bfbbe2330d Mon Sep 17 00:00:00 2001 From: valtzu Date: Sat, 15 Mar 2025 16:10:13 +0200 Subject: [PATCH 16/23] Fix typos in OIDC methods --- AccessToken/Oidc/OidcTokenHandler.php | 4 ++-- AccessToken/Oidc/OidcUserInfoTokenHandler.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/AccessToken/Oidc/OidcTokenHandler.php b/AccessToken/Oidc/OidcTokenHandler.php index b656cbc8..322b2a18 100644 --- a/AccessToken/Oidc/OidcTokenHandler.php +++ b/AccessToken/Oidc/OidcTokenHandler.php @@ -71,14 +71,14 @@ public function __construct( } } - public function enabledJweSupport(JWKSet $decryptionKeyset, AlgorithmManager $decryptionAlgorithms, bool $enforceEncryption): void + public function enableJweSupport(JWKSet $decryptionKeyset, AlgorithmManager $decryptionAlgorithms, bool $enforceEncryption): void { $this->decryptionKeyset = $decryptionKeyset; $this->decryptionAlgorithms = $decryptionAlgorithms; $this->enforceEncryption = $enforceEncryption; } - public function enabledDiscovery(CacheInterface $cache, HttpClientInterface $client, string $oidcConfigurationCacheKey, string $oidcJWKSetCacheKey): void + public function enableDiscovery(CacheInterface $cache, HttpClientInterface $client, string $oidcConfigurationCacheKey, string $oidcJWKSetCacheKey): void { $this->discoveryCache = $cache; $this->discoveryClient = $client; diff --git a/AccessToken/Oidc/OidcUserInfoTokenHandler.php b/AccessToken/Oidc/OidcUserInfoTokenHandler.php index 2ab7c8a5..1593018b 100644 --- a/AccessToken/Oidc/OidcUserInfoTokenHandler.php +++ b/AccessToken/Oidc/OidcUserInfoTokenHandler.php @@ -37,7 +37,7 @@ public function __construct( ) { } - public function enabledDiscovery(CacheInterface $cache, string $oidcConfigurationCacheKey): void + public function enableDiscovery(CacheInterface $cache, string $oidcConfigurationCacheKey): void { $this->discoveryCache = $cache; $this->oidcConfigurationCacheKey = $oidcConfigurationCacheKey; From aad723a3fe0e93389b402fc27f09a0c6a193f38c Mon Sep 17 00:00:00 2001 From: Dariusz Ruminski Date: Thu, 13 Mar 2025 23:58:12 +0100 Subject: [PATCH 17/23] chore: PHP CS Fixer fixes --- AccessToken/OAuth2/Oauth2TokenHandler.php | 1 + AccessToken/Oidc/OidcTrait.php | 1 + Tests/Authenticator/Passport/Badge/UserBadgeTest.php | 1 + Tests/EventListener/IsGrantedAttributeListenerTest.php | 2 +- 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/AccessToken/OAuth2/Oauth2TokenHandler.php b/AccessToken/OAuth2/Oauth2TokenHandler.php index 37f36d4d..05bda02f 100644 --- a/AccessToken/OAuth2/Oauth2TokenHandler.php +++ b/AccessToken/OAuth2/Oauth2TokenHandler.php @@ -18,6 +18,7 @@ use Symfony\Component\Security\Http\AccessToken\AccessTokenHandlerInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; use Symfony\Contracts\HttpClient\HttpClientInterface; + use function Symfony\Component\String\u; /** diff --git a/AccessToken/Oidc/OidcTrait.php b/AccessToken/Oidc/OidcTrait.php index be797711..d5ab61ff 100644 --- a/AccessToken/Oidc/OidcTrait.php +++ b/AccessToken/Oidc/OidcTrait.php @@ -12,6 +12,7 @@ namespace Symfony\Component\Security\Http\AccessToken\Oidc; use Symfony\Component\Security\Core\User\OidcUser; + use function Symfony\Component\String\u; /** diff --git a/Tests/Authenticator/Passport/Badge/UserBadgeTest.php b/Tests/Authenticator/Passport/Badge/UserBadgeTest.php index c910e454..f648d048 100644 --- a/Tests/Authenticator/Passport/Badge/UserBadgeTest.php +++ b/Tests/Authenticator/Passport/Badge/UserBadgeTest.php @@ -17,6 +17,7 @@ use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; use Symfony\Component\String\Slugger\AsciiSlugger; use Symfony\Component\String\UnicodeString; + use function Symfony\Component\String\u; class UserBadgeTest extends TestCase diff --git a/Tests/EventListener/IsGrantedAttributeListenerTest.php b/Tests/EventListener/IsGrantedAttributeListenerTest.php index 81ca1cb3..73494f40 100644 --- a/Tests/EventListener/IsGrantedAttributeListenerTest.php +++ b/Tests/EventListener/IsGrantedAttributeListenerTest.php @@ -224,7 +224,7 @@ public function testAccessDeniedMessages(string|Expression $attribute, string|ar $authChecker = new AuthorizationChecker(new TokenStorage(), new AccessDecisionManager((function () use (&$authChecker) { yield new ExpressionVoter(new ExpressionLanguage(), null, $authChecker); yield new RoleVoter(); - yield new class() extends Voter { + yield new class extends Voter { protected function supports(string $attribute, mixed $subject): bool { return 'POST_VIEW' === $attribute; From 9fefa7a419a141343e576985ba364722945e51e3 Mon Sep 17 00:00:00 2001 From: Oviglo Date: Thu, 20 Mar 2025 09:12:34 +0100 Subject: [PATCH 18/23] [Security] Add methods param in IsCsrfTokenValid attribute --- Attribute/IsCsrfTokenValid.php | 6 + .../IsCsrfTokenValidAttributeListener.php | 5 + .../IsCsrfTokenValidAttributeListenerTest.php | 137 ++++++++++++++++++ ...rfTokenValidAttributeMethodsController.php | 15 ++ 4 files changed, 163 insertions(+) diff --git a/Attribute/IsCsrfTokenValid.php b/Attribute/IsCsrfTokenValid.php index ef598df2..6226fb60 100644 --- a/Attribute/IsCsrfTokenValid.php +++ b/Attribute/IsCsrfTokenValid.php @@ -26,6 +26,12 @@ public function __construct( * Sets the key of the request that contains the actual token value that should be validated. */ public ?string $tokenKey = '_token', + + /** + * Sets the available http methods that can be used to validate the token. + * If not set, the token will be validated for all methods. + */ + public array|string $methods = [], ) { } } diff --git a/EventListener/IsCsrfTokenValidAttributeListener.php b/EventListener/IsCsrfTokenValidAttributeListener.php index 3e05c71d..3075e3d0 100644 --- a/EventListener/IsCsrfTokenValidAttributeListener.php +++ b/EventListener/IsCsrfTokenValidAttributeListener.php @@ -45,6 +45,11 @@ public function onKernelControllerArguments(ControllerArgumentsEvent $event): vo foreach ($attributes as $attribute) { $id = $this->getTokenId($attribute->id, $request, $arguments); + $methods = \array_map('strtoupper', (array) $attribute->methods); + + if ($methods && !\in_array($request->getMethod(), $methods, true)) { + continue; + } if (!$this->csrfTokenManager->isTokenValid(new CsrfToken($id, $request->getPayload()->getString($attribute->tokenKey)))) { throw new InvalidCsrfTokenException('Invalid CSRF token.'); diff --git a/Tests/EventListener/IsCsrfTokenValidAttributeListenerTest.php b/Tests/EventListener/IsCsrfTokenValidAttributeListenerTest.php index 00d464a6..6ce136ff 100644 --- a/Tests/EventListener/IsCsrfTokenValidAttributeListenerTest.php +++ b/Tests/EventListener/IsCsrfTokenValidAttributeListenerTest.php @@ -206,4 +206,141 @@ public function testExceptionWhenInvalidToken() $listener = new IsCsrfTokenValidAttributeListener($csrfTokenManager); $listener->onKernelControllerArguments($event); } + + public function testIsCsrfTokenValidCalledCorrectlyWithDeleteMethod() + { + $request = new Request(request: ['_token' => 'bar']); + $request->setMethod('DELETE'); + + $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class); + $csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foo', 'bar')) + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsCsrfTokenValidAttributeMethodsController(), 'withDeleteMethod'], + [], + $request, + null + ); + + $listener = new IsCsrfTokenValidAttributeListener($csrfTokenManager); + $listener->onKernelControllerArguments($event); + } + + public function testIsCsrfTokenValidIgnoredWithNonMatchingMethod() + { + $request = new Request(request: ['_token' => 'bar']); + $request->setMethod('POST'); + + $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class); + $csrfTokenManager->expects($this->never()) + ->method('isTokenValid') + ->with(new CsrfToken('foo', 'bar')); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsCsrfTokenValidAttributeMethodsController(), 'withDeleteMethod'], + [], + $request, + null + ); + + $listener = new IsCsrfTokenValidAttributeListener($csrfTokenManager); + $listener->onKernelControllerArguments($event); + } + + public function testIsCsrfTokenValidCalledCorrectlyWithGetOrPostMethodWithGetMethod() + { + $request = new Request(request: ['_token' => 'bar']); + $request->setMethod('GET'); + + $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class); + $csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foo', 'bar')) + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsCsrfTokenValidAttributeMethodsController(), 'withGetOrPostMethod'], + [], + $request, + null + ); + + $listener = new IsCsrfTokenValidAttributeListener($csrfTokenManager); + $listener->onKernelControllerArguments($event); + } + + public function testIsCsrfTokenValidNoIgnoredWithGetOrPostMethodWithPutMethod() + { + $request = new Request(request: ['_token' => 'bar']); + $request->setMethod('PUT'); + + $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class); + $csrfTokenManager->expects($this->never()) + ->method('isTokenValid') + ->with(new CsrfToken('foo', 'bar')); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsCsrfTokenValidAttributeMethodsController(), 'withGetOrPostMethod'], + [], + $request, + null + ); + + $listener = new IsCsrfTokenValidAttributeListener($csrfTokenManager); + $listener->onKernelControllerArguments($event); + } + + public function testIsCsrfTokenValidCalledCorrectlyWithInvalidTokenKeyAndPostMethod() + { + $this->expectException(InvalidCsrfTokenException::class); + + $request = new Request(request: ['_token' => 'bar']); + $request->setMethod('POST'); + + $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class); + $csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->withAnyParameters() + ->willReturn(false); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsCsrfTokenValidAttributeMethodsController(), 'withPostMethodAndInvalidTokenKey'], + [], + $request, + null + ); + + $listener = new IsCsrfTokenValidAttributeListener($csrfTokenManager); + $listener->onKernelControllerArguments($event); + } + + public function testIsCsrfTokenValidIgnoredWithInvalidTokenKeyAndUnavailableMethod() + { + $request = new Request(request: ['_token' => 'bar']); + $request->setMethod('PUT'); + + $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class); + $csrfTokenManager->expects($this->never()) + ->method('isTokenValid') + ->withAnyParameters(); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsCsrfTokenValidAttributeMethodsController(), 'withPostMethodAndInvalidTokenKey'], + [], + $request, + null + ); + + $listener = new IsCsrfTokenValidAttributeListener($csrfTokenManager); + $listener->onKernelControllerArguments($event); + } } diff --git a/Tests/Fixtures/IsCsrfTokenValidAttributeMethodsController.php b/Tests/Fixtures/IsCsrfTokenValidAttributeMethodsController.php index baf45d77..8555a43b 100644 --- a/Tests/Fixtures/IsCsrfTokenValidAttributeMethodsController.php +++ b/Tests/Fixtures/IsCsrfTokenValidAttributeMethodsController.php @@ -44,4 +44,19 @@ public function withCustomTokenKey() public function withInvalidTokenKey() { } + + #[IsCsrfTokenValid('foo', methods: 'DELETE')] + public function withDeleteMethod() + { + } + + #[IsCsrfTokenValid('foo', methods: ['GET', 'POST'])] + public function withGetOrPostMethod() + { + } + + #[IsCsrfTokenValid('foo', tokenKey: 'invalid_token_key', methods: ['POST'])] + public function withPostMethodAndInvalidTokenKey() + { + } } From 6298c3c8524e2e9141473c8d2f09ef8da5cf8fbb Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Fri, 4 Apr 2025 14:20:35 +0200 Subject: [PATCH 19/23] Remove non-final readonly classes --- Attribute/IsGrantedContext.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Attribute/IsGrantedContext.php b/Attribute/IsGrantedContext.php index fa2ce4a0..87776452 100644 --- a/Attribute/IsGrantedContext.php +++ b/Attribute/IsGrantedContext.php @@ -17,12 +17,12 @@ use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter; use Symfony\Component\Security\Core\User\UserInterface; -readonly class IsGrantedContext implements AuthorizationCheckerInterface +class IsGrantedContext implements AuthorizationCheckerInterface { public function __construct( - public TokenInterface $token, - public ?UserInterface $user, - private AuthorizationCheckerInterface $authorizationChecker, + public readonly TokenInterface $token, + public readonly ?UserInterface $user, + private readonly AuthorizationCheckerInterface $authorizationChecker, ) { } From 626eec0e495ea2e3ae850638741ff0e0b1ddefdd Mon Sep 17 00:00:00 2001 From: Christian Flothmann Date: Fri, 4 Apr 2025 15:02:15 +0200 Subject: [PATCH 20/23] replace expectDeprecation() with expectUserDeprecationMessage() --- Tests/Authentication/AuthenticatorManagerTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/Authentication/AuthenticatorManagerTest.php b/Tests/Authentication/AuthenticatorManagerTest.php index a88b3ba5..67f7247f 100644 --- a/Tests/Authentication/AuthenticatorManagerTest.php +++ b/Tests/Authentication/AuthenticatorManagerTest.php @@ -15,7 +15,7 @@ use PHPUnit\Framework\TestCase; use Psr\Log\AbstractLogger; use Psr\Log\LoggerInterface; -use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; +use Symfony\Bridge\PhpUnit\ExpectUserDeprecationMessageTrait; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -42,7 +42,7 @@ class AuthenticatorManagerTest extends TestCase { - use ExpectDeprecationTrait; + use ExpectUserDeprecationMessageTrait; private MockObject&TokenStorageInterface $tokenStorage; private EventDispatcher $eventDispatcher; @@ -211,7 +211,7 @@ public function eraseCredentials(): void $authenticator->expects($this->any())->method('createToken')->willReturn($token); if ($eraseCredentials) { - $this->expectDeprecation(\sprintf('Since symfony/security-http 7.3: Implementing "%s@anonymous::eraseCredentials()" is deprecated since Symfony 7.3; add the #[\Deprecated] attribute on the method to signal its either empty or that you moved the logic elsewhere, typically to the "__serialize()" method.', AbstractToken::class)); + $this->expectUserDeprecationMessage(\sprintf('Since symfony/security-http 7.3: Implementing "%s@anonymous::eraseCredentials()" is deprecated since Symfony 7.3; add the #[\Deprecated] attribute on the method to signal its either empty or that you moved the logic elsewhere, typically to the "__serialize()" method.', AbstractToken::class)); } $manager = $this->createManager([$authenticator], 'main', $eraseCredentials, exposeSecurityErrors: ExposeSecurityLevel::None); From 0a29c89e67ff0a153ab14fc16880ec164f4379e1 Mon Sep 17 00:00:00 2001 From: Benjamin Morel Date: Mon, 21 Apr 2025 01:34:08 +0200 Subject: [PATCH 21/23] Add callable type to CustomCredentials --- Authenticator/Passport/Credentials/CustomCredentials.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Authenticator/Passport/Credentials/CustomCredentials.php b/Authenticator/Passport/Credentials/CustomCredentials.php index 4543e174..23ac8c7f 100644 --- a/Authenticator/Passport/Credentials/CustomCredentials.php +++ b/Authenticator/Passport/Credentials/CustomCredentials.php @@ -27,9 +27,9 @@ class CustomCredentials implements CredentialsInterface private bool $resolved = false; /** - * @param callable $customCredentialsChecker the check function. If this function does not return `true`, a - * BadCredentialsException is thrown. You may also throw a more - * specific exception in the function. + * @param callable(mixed, UserInterface) $customCredentialsChecker If the callable does not return `true`, a + * BadCredentialsException is thrown. You may + * also throw a more specific exception. */ public function __construct( callable $customCredentialsChecker, From 8658634cc002096210c6227ec87a300dc647f88f Mon Sep 17 00:00:00 2001 From: Christian Flothmann Date: Mon, 26 May 2025 11:36:54 +0200 Subject: [PATCH 22/23] do not construct Vote instances inside vote() The so constructed objects will never be seen from the outside. Thus, adding reasons to them doesn't have an effect. --- Tests/EventListener/IsGrantedAttributeListenerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/EventListener/IsGrantedAttributeListenerTest.php b/Tests/EventListener/IsGrantedAttributeListenerTest.php index 73494f40..d34b31f2 100644 --- a/Tests/EventListener/IsGrantedAttributeListenerTest.php +++ b/Tests/EventListener/IsGrantedAttributeListenerTest.php @@ -232,7 +232,7 @@ protected function supports(string $attribute, mixed $subject): bool protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token, ?Vote $vote = null): bool { - $vote->reasons[] = 'Because I can 😈.'; + $vote?->addReason('Because I can 😈.'); return false; } From 2b244a049677e833064ae4ad5eda34853a65c62e Mon Sep 17 00:00:00 2001 From: matlec Date: Fri, 13 Jun 2025 17:02:47 +0200 Subject: [PATCH 23/23] [Security] Handle non-callable implementations of `FirewallListenerInterface` --- Firewall.php | 6 ++++- Tests/FirewallTest.php | 57 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/Firewall.php b/Firewall.php index f2f86a5d..354181c8 100644 --- a/Firewall.php +++ b/Firewall.php @@ -125,7 +125,11 @@ public static function getSubscribedEvents() protected function callListeners(RequestEvent $event, iterable $listeners) { foreach ($listeners as $listener) { - $listener($event); + if (!$listener instanceof FirewallListenerInterface) { + $listener($event); + } elseif (false !== $listener->supports($event->getRequest())) { + $listener->authenticate($event); + } if ($event->hasResponse()) { break; diff --git a/Tests/FirewallTest.php b/Tests/FirewallTest.php index f9417d23..89040f38 100644 --- a/Tests/FirewallTest.php +++ b/Tests/FirewallTest.php @@ -18,7 +18,9 @@ use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\Security\Http\Firewall; +use Symfony\Component\Security\Http\Firewall\AbstractListener; use Symfony\Component\Security\Http\Firewall\ExceptionListener; +use Symfony\Component\Security\Http\Firewall\FirewallListenerInterface; use Symfony\Component\Security\Http\FirewallMapInterface; class FirewallTest extends TestCase @@ -97,4 +99,59 @@ public function testOnKernelRequestWithSubRequest() $this->assertFalse($event->hasResponse()); } + + public function testListenersAreCalled() + { + $calledListeners = []; + + $callableListener = static function() use(&$calledListeners) { $calledListeners[] = 'callableListener'; }; + $firewallListener = new class($calledListeners) implements FirewallListenerInterface { + public function __construct(private array &$calledListeners) {} + + public function supports(Request $request): ?bool + { + return true; + } + + public function authenticate(RequestEvent $event): void + { + $this->calledListeners[] = 'firewallListener'; + } + + public static function getPriority(): int + { + return 0; + } + }; + $callableFirewallListener = new class($calledListeners) extends AbstractListener { + public function __construct(private array &$calledListeners) {} + + public function supports(Request $request): ?bool + { + return true; + } + + public function authenticate(RequestEvent $event): void + { + $this->calledListeners[] = 'callableFirewallListener'; + } + }; + + $request = $this->createMock(Request::class); + + $map = $this->createMock(FirewallMapInterface::class); + $map + ->expects($this->once()) + ->method('getListeners') + ->with($this->equalTo($request)) + ->willReturn([[$callableListener, $firewallListener, $callableFirewallListener], null, null]) + ; + + $event = new RequestEvent($this->createMock(HttpKernelInterface::class), $request, HttpKernelInterface::MAIN_REQUEST); + + $firewall = new Firewall($map, $this->createMock(EventDispatcherInterface::class)); + $firewall->onKernelRequest($event); + + $this->assertSame(['callableListener', 'firewallListener', 'callableFirewallListener'], $calledListeners); + } }