From 24719705e74b49162fa3413e0b7501dedfd807d9 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 11 May 2023 15:30:02 +0200 Subject: [PATCH 01/11] Implement a login listener to fill the Sentry User context --- composer.json | 1 + .../Compiler/AddLoginListenerTagPass.php | 29 ++ src/EventListener/LoginListener.php | 129 +++++++ src/Resources/config/services.xml | 6 + src/SentryBundle.php | 2 + .../Compiler/AddLoginListenerTagPassTest.php | 31 ++ tests/EventListener/LoginListenerTest.php | 354 ++++++++++++++++++ 7 files changed, 552 insertions(+) create mode 100644 src/DependencyInjection/Compiler/AddLoginListenerTagPass.php create mode 100644 src/EventListener/LoginListener.php create mode 100644 tests/DependencyInjection/Compiler/AddLoginListenerTagPassTest.php create mode 100644 tests/EventListener/LoginListenerTest.php diff --git a/composer.json b/composer.json index 4214b086..cad83e3d 100644 --- a/composer.json +++ b/composer.json @@ -61,6 +61,7 @@ "symfony/monolog-bundle": "^3.4", "symfony/phpunit-bridge": "^5.2.6||^6.0", "symfony/process": "^4.4.20||^5.0.11||^6.0", + "symfony/security-http": "^4.4.20||^5.0.11||^6.0", "symfony/twig-bundle": "^4.4.20||^5.0.11||^6.0", "symfony/yaml": "^4.4.20||^5.0.11||^6.0", "vimeo/psalm": "^4.3" diff --git a/src/DependencyInjection/Compiler/AddLoginListenerTagPass.php b/src/DependencyInjection/Compiler/AddLoginListenerTagPass.php new file mode 100644 index 00000000..24d3110c --- /dev/null +++ b/src/DependencyInjection/Compiler/AddLoginListenerTagPass.php @@ -0,0 +1,29 @@ +getDefinition(LoginListener::class); + + if (!class_exists(LoginSuccessEvent::class)) { + $listenerDefinition->addTag('kernel.event_listener', [ + 'event' => AuthenticationSuccessEvent::class, + 'method' => 'handleAuthenticationSuccessEvent', + ]); + } + } +} diff --git a/src/EventListener/LoginListener.php b/src/EventListener/LoginListener.php new file mode 100644 index 00000000..0e2810da --- /dev/null +++ b/src/EventListener/LoginListener.php @@ -0,0 +1,129 @@ +hub = $hub; + } + + /** + * This method is called after authentication was fully successful. It allows + * to set information like the username of the currently authenticated user + * and of the impersonator, if any, on the Sentry's context. + */ + public function handleLoginSuccessEvent(LoginSuccessEvent $event): void + { + $this->updateUserContext($event->getAuthenticatedToken()); + } + + /** + * This method is called when an authentication provider authenticates the + * user. It is the event closest to {@see LoginSuccessEvent} in versions of + * the framework where it doesn't exist. + */ + public function handleAuthenticationSuccessEvent(AuthenticationSuccessEvent $event): void + { + if (class_exists(LoginSuccessEvent::class)) { + return; + } + + $this->updateUserContext($event->getAuthenticationToken()); + } + + private function updateUserContext(TokenInterface $token): void + { + if (!$this->isTokenAuthenticated($token)) { + return; + } + + $client = $this->hub->getClient(); + + if (null === $client || !$client->getOptions()->shouldSendDefaultPii()) { + return; + } + + $this->hub->configureScope(function (Scope $scope) use ($token): void { + $user = $scope->getUser() ?? new UserDataBag(); + + if (null === $user->getId()) { + $user->setId($this->getUserIdentifier($token->getUser())); + } + + $impersonatorUser = $this->getImpersonatorUser($token); + + if (null !== $impersonatorUser) { + $user->setMetadata('impersonator_username', $impersonatorUser); + } + + $scope->setUser($user); + }); + } + + private function isTokenAuthenticated(TokenInterface $token): bool + { + if (method_exists($token, 'isAuthenticated') && !$token->isAuthenticated()) { + return false; + } + + return null !== $token->getUser(); + } + + /** + * @param UserInterface|\Stringable|string|null $user + */ + private function getUserIdentifier($user): ?string + { + if ($user instanceof UserInterface) { + if (method_exists($user, 'getUserIdentifier')) { + return $user->getUserIdentifier(); + } + + if (method_exists($user, 'getUsername')) { + return $user->getUsername(); + } + } + + if (\is_string($user)) { + return $user; + } + + if (\is_object($user) && method_exists($user, '__toString')) { + return (string) $user; + } + + return null; + } + + private function getImpersonatorUser(TokenInterface $token): ?string + { + if ($token instanceof SwitchUserToken) { + return $this->getUserIdentifier($token->getOriginalToken()->getUser()); + } + + return null; + } +} diff --git a/src/Resources/config/services.xml b/src/Resources/config/services.xml index fb6ec637..e6bb7d8d 100644 --- a/src/Resources/config/services.xml +++ b/src/Resources/config/services.xml @@ -87,6 +87,12 @@ + + + + + + diff --git a/src/SentryBundle.php b/src/SentryBundle.php index 4c909d3d..45e57eff 100644 --- a/src/SentryBundle.php +++ b/src/SentryBundle.php @@ -4,6 +4,7 @@ namespace Sentry\SentryBundle; +use Sentry\SentryBundle\DependencyInjection\Compiler\AddLoginListenerTagPass; use Sentry\SentryBundle\DependencyInjection\Compiler\CacheTracingPass; use Sentry\SentryBundle\DependencyInjection\Compiler\DbalTracingPass; use Sentry\SentryBundle\DependencyInjection\Compiler\HttpClientTracingPass; @@ -23,5 +24,6 @@ public function build(ContainerBuilder $container): void $container->addCompilerPass(new DbalTracingPass()); $container->addCompilerPass(new CacheTracingPass()); $container->addCompilerPass(new HttpClientTracingPass()); + $container->addCompilerPass(new AddLoginListenerTagPass()); } } diff --git a/tests/DependencyInjection/Compiler/AddLoginListenerTagPassTest.php b/tests/DependencyInjection/Compiler/AddLoginListenerTagPassTest.php new file mode 100644 index 00000000..cc91f8f7 --- /dev/null +++ b/tests/DependencyInjection/Compiler/AddLoginListenerTagPassTest.php @@ -0,0 +1,31 @@ +markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event exists.'); + } + + $container = new ContainerBuilder(); + $container->register(LoginListener::class)->setPublic(true); + $container->addCompilerPass(new AddLoginListenerTagPass()); + $container->compile(); + + $listenerDefinition = $container->getDefinition(LoginListener::class); + + $this->assertSame([['event' => AuthenticationSuccessEvent::class, 'method' => 'handleAuthenticationSuccessEvent']], $listenerDefinition->getTag('kernel.event_listener')); + } +} diff --git a/tests/EventListener/LoginListenerTest.php b/tests/EventListener/LoginListenerTest.php new file mode 100644 index 00000000..ade8cb6d --- /dev/null +++ b/tests/EventListener/LoginListenerTest.php @@ -0,0 +1,354 @@ +hub = $this->createMock(HubInterface::class); + $this->listener = new LoginListener($this->hub); + } + + /** + * @dataProvider handleLoginSuccessEventDataProvider + */ + public function testHandleLoginSuccessEvent(TokenInterface $token, ?UserDataBag $user, UserDataBag $expectedUser): void + { + if (!class_exists(LoginSuccessEvent::class)) { + $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event does not exist.'); + } + + $scope = new Scope(); + + $client = $this->createMock(ClientInterface::class); + $client->expects($this->once()) + ->method('getOptions') + ->willReturn(new Options(['send_default_pii' => true])); + + $this->hub->expects($this->once()) + ->method('getClient') + ->willReturn($client); + + $this->hub->expects($this->once()) + ->method('configureScope') + ->willReturnCallback(static function (callable $callback) use ($scope): void { + $callback($scope); + }); + + if (null !== $user) { + $scope->setUser($user); + } + + $this->listener->handleLoginSuccessEvent(new LoginSuccessEvent( + $this->createMock(AuthenticatorInterface::class), + new SelfValidatingPassport(new UserBadge('foo_passport_user')), + $token, + new Request(), + null, + 'main' + )); + + $event = $scope->applyToEvent(Event::createEvent()); + + $this->assertNotNull($event); + $this->assertEquals($expectedUser, $event->getUser()); + } + + /** + * @dataProvider handleLoginSuccessEventDataProvider + * @dataProvider handleLoginSuccessEventForSymfonyVersionLowerThan54DataProvider + */ + public function testHandleAuthenticationSuccessEvent(TokenInterface $token, ?UserDataBag $user, UserDataBag $expectedUser): void + { + if (class_exists(LoginSuccessEvent::class)) { + $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event exists.'); + } + + $scope = new Scope(); + + $client = $this->createMock(ClientInterface::class); + $client->expects($this->once()) + ->method('getOptions') + ->willReturn(new Options(['send_default_pii' => true])); + + $this->hub->expects($this->once()) + ->method('getClient') + ->willReturn($client); + + $this->hub->expects($this->once()) + ->method('configureScope') + ->willReturnCallback(static function (callable $callback) use ($scope): void { + $callback($scope); + }); + + if (null !== $user) { + $scope->setUser($user); + } + + $this->listener->handleAuthenticationSuccessEvent(new AuthenticationSuccessEvent($token)); + + $event = $scope->applyToEvent(Event::createEvent()); + + $this->assertNotNull($event); + $this->assertEquals($expectedUser, $event->getUser()); + } + + public function handleLoginSuccessEventDataProvider(): \Generator + { + yield 'If the username is already set on the User context, then it is not overridden' => [ + new UsernamePasswordToken(new UserWithIdentifierStub(), 'main'), + new UserDataBag('bar_user'), + new UserDataBag('bar_user'), + ]; + + yield 'If the username is not set on the User context, then it is retrieved from the token' => [ + new UsernamePasswordToken(new UserWithIdentifierStub(), 'main'), + null, + new UserDataBag('foo_user'), + ]; + + yield 'If the user is being impersonated, then the username of the impersonator is set on the User context' => [ + new SwitchUserToken(new UserWithIdentifierStub(), 'main', [], new AuthenticatedTokenStub(new UserWithIdentifierStub('bar_user'))), + null, + UserDataBag::createFromArray([ + 'id' => 'foo_user', + 'impersonator_username' => 'bar_user', + ]), + ]; + } + + public function handleLoginSuccessEventForSymfonyVersionLowerThan54DataProvider(): \Generator + { + if (version_compare(Kernel::VERSION, '5.4.0', '>=')) { + return; + } + + yield 'Given an authenticated token, if the user is not set, then the User context is not populated' => [ + new AuthenticatedTokenStub(null), + null, + null, + ]; + + yield 'Given an authenticated token, if the user is a string, then the User context is populated' => [ + new AuthenticatedTokenStub('foo_user'), + null, + new UserDataBag('foo_user'), + ]; + + yield 'Given an authenticated token, if the user is an instance of the UserInterface interface but the getUserIdentifier() method does not exist, then the User context is populated by calling the getUsername() method' => [ + new AuthenticatedTokenStub(new UserWithoutIdentifierStub()), + null, + new UserDataBag('foo_user'), + ]; + + yield 'Given an authenticated token, if the user is an object implementing the Stringable interface, then the User context is populated by calling the __toString() method' => [ + new AuthenticatedTokenStub(new class() implements \Stringable { + public function __toString(): string + { + return 'foo_user'; + } + }), + null, + new UserDataBag('foo_user'), + ]; + } + + public function testHandleLoginSuccessEventDoesNothingIfTokenIsNotAuthenticated(): void + { + if (!class_exists(LoginSuccessEvent::class)) { + $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event does not exist.'); + } + + $this->hub->expects($this->never()) + ->method('getClient'); + + $this->hub->expects($this->never()) + ->method('configureScope'); + + $this->listener->handleLoginSuccessEvent(new LoginSuccessEvent( + $this->createMock(AuthenticatorInterface::class), + new SelfValidatingPassport(new UserBadge('foo_passport_user')), + new UnauthenticatedTokenStub(), + new Request(), + null, + 'main' + )); + } + + public function testHandleLoginSuccessEventDoesNothingIfClientIsNotSetOnHub(): void + { + if (!class_exists(LoginSuccessEvent::class)) { + $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event does not exist.'); + } + + $this->hub->expects($this->once()) + ->method('getClient') + ->willReturn(null); + + $this->hub->expects($this->never()) + ->method('configureScope'); + + $this->listener->handleLoginSuccessEvent(new LoginSuccessEvent( + $this->createMock(AuthenticatorInterface::class), + new SelfValidatingPassport(new UserBadge('foo_passport_user')), + new AuthenticatedTokenStub(new UserWithIdentifierStub()), + new Request(), + null, + 'main' + )); + } + + public function testHandleLoginSuccessEventDoesNothingIfSendingDefaultPiiIsDisabled(): void + { + if (!class_exists(LoginSuccessEvent::class)) { + $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event does not exist.'); + } + + $client = $this->createMock(ClientInterface::class); + $client->expects($this->once()) + ->method('getOptions') + ->willReturn(new Options(['send_default_pii' => false])); + + $this->hub->expects($this->once()) + ->method('getClient') + ->willReturn($client); + + $this->hub->expects($this->never()) + ->method('configureScope'); + + $this->listener->handleLoginSuccessEvent(new LoginSuccessEvent( + $this->createMock(AuthenticatorInterface::class), + new SelfValidatingPassport(new UserBadge('foo_passport_user')), + new AuthenticatedTokenStub(new UserWithIdentifierStub()), + new Request(), + null, + 'main' + )); + } + + public function testHandleAuthenticationSuccessEventDoesNothingIfTokenIsNotAuthenticated(): void + { + if (class_exists(LoginSuccessEvent::class)) { + $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event exists.'); + } + + $this->hub->expects($this->never()) + ->method('getClient'); + + $this->hub->expects($this->never()) + ->method('configureScope'); + + $this->listener->handleAuthenticationSuccessEvent(new AuthenticationSuccessEvent(new UnauthenticatedTokenStub())); + } + + public function testHandleAuthenticationSuccessEventDoesNothingIfClientIsNotSetOnHub(): void + { + if (class_exists(LoginSuccessEvent::class)) { + $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event exists.'); + } + + $this->hub->expects($this->once()) + ->method('getClient') + ->willReturn(null); + + $this->hub->expects($this->never()) + ->method('configureScope'); + + $this->listener->handleAuthenticationSuccessEvent(new AuthenticationSuccessEvent(new AuthenticatedTokenStub(new UserWithIdentifierStub()))); + } + + public function testHandleAuthenticationSuccessEventDoesNothingIfSendingDefaultPiiIsDisabled(): void + { + if (class_exists(LoginSuccessEvent::class)) { + $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event exists.'); + } + + $client = $this->createMock(ClientInterface::class); + $client->expects($this->once()) + ->method('getOptions') + ->willReturn(new Options(['send_default_pii' => false])); + + $this->hub->expects($this->once()) + ->method('getClient') + ->willReturn($client); + + $this->hub->expects($this->never()) + ->method('configureScope'); + + $this->listener->handleAuthenticationSuccessEvent(new AuthenticationSuccessEvent(new AuthenticatedTokenStub(new UserWithIdentifierStub()))); + } +} + +final class UnauthenticatedTokenStub extends AbstractToken +{ + public function isAuthenticated(): bool + { + return false; + } + + public function getCredentials(): ?string + { + return null; + } +} + +final class AuthenticatedTokenStub extends AbstractToken +{ + /** + * @param UserInterface|\Stringable|string|null $user + */ + public function __construct($user) + { + parent::__construct(); + + if (null !== $user) { + $this->setUser($user); + } + + if (method_exists($this, 'setAuthenticated')) { + $this->setAuthenticated(true); + } + } + + public function getCredentials(): ?string + { + return null; + } +} From b3863a12d49a96bd67ff86443525c2f61ee9bed7 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 11 May 2023 15:34:31 +0200 Subject: [PATCH 02/11] Clean up no longer useful code from the `RequestListener` class --- src/EventListener/RequestListener.php | 84 +----- src/Resources/config/services.xml | 1 - tests/EventListener/RequestListenerTest.php | 267 +------------------- 3 files changed, 7 insertions(+), 345 deletions(-) diff --git a/src/EventListener/RequestListener.php b/src/EventListener/RequestListener.php index b566d615..e42388cf 100644 --- a/src/EventListener/RequestListener.php +++ b/src/EventListener/RequestListener.php @@ -9,10 +9,6 @@ use Sentry\UserDataBag; use Symfony\Component\HttpKernel\Event\ControllerEvent; use Symfony\Component\HttpKernel\Event\RequestEvent; -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\User\UserInterface; /** * This listener ensures that a new {@see \Sentry\State\Scope} is created for @@ -28,21 +24,14 @@ final class RequestListener */ private $hub; - /** - * @var TokenStorageInterface|null The token storage - */ - private $tokenStorage; - /** * Constructor. * - * @param HubInterface $hub The current hub - * @param TokenStorageInterface|null $tokenStorage The token storage + * @param HubInterface $hub The current hub */ - public function __construct(HubInterface $hub, ?TokenStorageInterface $tokenStorage) + public function __construct(HubInterface $hub/* , ?TokenStorageInterface $tokenStorage */) { $this->hub = $hub; - $this->tokenStorage = $tokenStorage; } /** @@ -63,14 +52,10 @@ public function handleKernelRequestEvent(RequestEvent $event): void return; } - $userData = new UserDataBag(); - $userData->setIpAddress($event->getRequest()->getClientIp()); - - if (null !== $this->tokenStorage) { - $this->setUserData($userData, $this->tokenStorage->getToken()); - } + $this->hub->configureScope(static function (Scope $scope) use ($event): void { + $userData = new UserDataBag(); + $userData->setIpAddress($event->getRequest()->getClientIp()); - $this->hub->configureScope(static function (Scope $scope) use ($userData): void { $scope->setUser($userData); }); } @@ -97,63 +82,4 @@ public function handleKernelControllerEvent(ControllerEvent $event): void $scope->setTag('route', $route); }); } - - /** - * @param UserInterface|object|string|null $user - */ - private function getUsername($user): ?string - { - if ($user instanceof UserInterface) { - if (method_exists($user, 'getUserIdentifier')) { - return $user->getUserIdentifier(); - } - - if (method_exists($user, 'getUsername')) { - return $user->getUsername(); - } - } - - if (\is_string($user)) { - return $user; - } - - if (\is_object($user) && method_exists($user, '__toString')) { - return (string) $user; - } - - return null; - } - - private function getImpersonatorUser(TokenInterface $token): ?string - { - if (!$token instanceof SwitchUserToken) { - return null; - } - - return $this->getUsername($token->getOriginalToken()->getUser()); - } - - private function setUserData(UserDataBag $userData, ?TokenInterface $token): void - { - if (null === $token || !$this->isTokenAuthenticated($token)) { - return; - } - - $userData->setUsername($this->getUsername($token->getUser())); - - $impersonatorUser = $this->getImpersonatorUser($token); - - if (null !== $impersonatorUser) { - $userData->setMetadata('impersonator_username', $impersonatorUser); - } - } - - private function isTokenAuthenticated(TokenInterface $token): bool - { - if (method_exists($token, 'isAuthenticated') && !$token->isAuthenticated(false)) { - return false; - } - - return null !== $token->getUser(); - } } diff --git a/src/Resources/config/services.xml b/src/Resources/config/services.xml index e6bb7d8d..79e3c319 100644 --- a/src/Resources/config/services.xml +++ b/src/Resources/config/services.xml @@ -43,7 +43,6 @@ - diff --git a/tests/EventListener/RequestListenerTest.php b/tests/EventListener/RequestListenerTest.php index 9ee2803a..c280e6f3 100644 --- a/tests/EventListener/RequestListenerTest.php +++ b/tests/EventListener/RequestListenerTest.php @@ -10,8 +10,6 @@ use Sentry\Event; use Sentry\Options; use Sentry\SentryBundle\EventListener\RequestListener; -use Sentry\SentryBundle\Tests\EventListener\Fixtures\UserWithIdentifierStub; -use Sentry\SentryBundle\Tests\EventListener\Fixtures\UserWithoutIdentifierStub; use Sentry\State\HubInterface; use Sentry\State\Scope; use Sentry\UserDataBag; @@ -19,12 +17,6 @@ use Symfony\Component\HttpKernel\Event\ControllerEvent; use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; -use Symfony\Component\HttpKernel\Kernel; -use Symfony\Component\Security\Core\Authentication\Token\AbstractToken; -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\User\UserInterface; final class RequestListenerTest extends TestCase { @@ -33,11 +25,6 @@ final class RequestListenerTest extends TestCase */ private $hub; - /** - * @var MockObject&TokenStorageInterface - */ - private $tokenStorage; - /** * @var RequestListener */ @@ -46,17 +33,13 @@ final class RequestListenerTest extends TestCase protected function setUp(): void { $this->hub = $this->createMock(HubInterface::class); - $this->tokenStorage = $this->createMock(TokenStorageInterface::class); - $this->listener = new RequestListener($this->hub, $this->tokenStorage); + $this->listener = new RequestListener($this->hub); } /** * @dataProvider handleKernelRequestEventDataProvider - * @dataProvider handleKernelRequestEventForSymfonyVersionLowerThan54DataProvider - * @dataProvider handleKernelRequestEventForSymfonyVersionEqualTo54DataProvider - * @dataProvider handleKernelRequestEventForSymfonyVersionGreaterThan54DataProvider */ - public function testHandleKernelRequestEvent(RequestEvent $requestEvent, ?ClientInterface $client, ?TokenInterface $token, ?UserDataBag $expectedUser): void + public function testHandleKernelRequestEvent(RequestEvent $requestEvent, ?ClientInterface $client, ?UserDataBag $expectedUser): void { $scope = new Scope(); @@ -64,10 +47,6 @@ public function testHandleKernelRequestEvent(RequestEvent $requestEvent, ?Client ->method('getClient') ->willReturn($client); - $this->tokenStorage->expects($this->any()) - ->method('getToken') - ->willReturn($token); - $this->hub->expects($this->any()) ->method('configureScope') ->willReturnCallback(static function (callable $callback) use ($scope): void { @@ -95,7 +74,6 @@ public function handleKernelRequestEventDataProvider(): \Generator ), $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), null, - null, ]; yield 'options.send_default_pii = FALSE' => [ @@ -106,7 +84,6 @@ public function handleKernelRequestEventDataProvider(): \Generator ), $this->getMockedClientWithOptions(new Options(['send_default_pii' => false])), null, - null, ]; yield 'token IS NULL' => [ @@ -116,7 +93,6 @@ public function handleKernelRequestEventDataProvider(): \Generator HttpKernelInterface::MASTER_REQUEST ), $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - null, UserDataBag::createFromUserIpAddress('127.0.0.1'), ]; @@ -127,217 +103,10 @@ public function handleKernelRequestEventDataProvider(): \Generator HttpKernelInterface::MASTER_REQUEST ), $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - null, new UserDataBag(), ]; } - /** - * @return \Generator - */ - public function handleKernelRequestEventForSymfonyVersionLowerThan54DataProvider(): \Generator - { - if (version_compare(Kernel::VERSION, '5.4.0', '>=')) { - return; - } - - yield 'token.authenticated = FALSE' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new UnauthenticatedTokenStub(), - UserDataBag::createFromUserIpAddress('127.0.0.1'), - ]; - - yield 'token.authenticated = TRUE && token.user IS NULL' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new AuthenticatedTokenStub(null), - UserDataBag::createFromUserIpAddress('127.0.0.1'), - ]; - - yield 'token.authenticated = TRUE && token.user INSTANCEOF string' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new AuthenticatedTokenStub('foo_user'), - new UserDataBag(null, null, '127.0.0.1', 'foo_user'), - ]; - - yield 'token.authenticated = TRUE && token.user INSTANCEOF UserInterface && getUserIdentifier() method DOES NOT EXISTS' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new AuthenticatedTokenStub(new UserWithoutIdentifierStub()), - new UserDataBag(null, null, '127.0.0.1', 'foo_user'), - ]; - - yield 'token.authenticated = TRUE && token.user INSTANCEOF object && __toString() method EXISTS' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new AuthenticatedTokenStub(new class() implements \Stringable { - public function __toString(): string - { - return 'foo_user'; - } - }), - new UserDataBag(null, null, '127.0.0.1', 'foo_user'), - ]; - - yield 'token.authenticated = TRUE && token INSTANCEOF SwitchUserToken' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new SwitchUserToken( - new UserWithIdentifierStub(), - '', - 'user_provider', - ['ROLE_USER'], - new AuthenticatedTokenStub(new UserWithIdentifierStub('foo_user_impersonator')) - ), - UserDataBag::createFromArray([ - 'ip_address' => '127.0.0.1', - 'username' => 'foo_user', - 'impersonator_username' => 'foo_user_impersonator', - ]), - ]; - } - - /** - * @return \Generator - */ - public function handleKernelRequestEventForSymfonyVersionEqualTo54DataProvider(): \Generator - { - if (version_compare(Kernel::VERSION, '5.4.0', '!=')) { - return; - } - - yield 'token.authenticated = FALSE' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new UnauthenticatedTokenStub(), - UserDataBag::createFromUserIpAddress('127.0.0.1'), - ]; - - yield 'token.authenticated = TRUE && token.user IS NULL' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new AuthenticatedTokenStub(null), - UserDataBag::createFromUserIpAddress('127.0.0.1'), - ]; - - yield 'token.authenticated = TRUE && token.user INSTANCEOF UserInterface && getUserIdentifier() method EXISTS' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new AuthenticatedTokenStub(new UserWithIdentifierStub()), - new UserDataBag(null, null, '127.0.0.1', 'foo_user'), - ]; - - yield 'token.authenticated = TRUE && token INSTANCEOF SwitchUserToken' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new SwitchUserToken( - new UserWithIdentifierStub(), - 'main', - ['ROLE_USER'], - new AuthenticatedTokenStub(new UserWithIdentifierStub('foo_user_impersonator')) - ), - UserDataBag::createFromArray([ - 'ip_address' => '127.0.0.1', - 'username' => 'foo_user', - 'impersonator_username' => 'foo_user_impersonator', - ]), - ]; - } - - /** - * @return \Generator - */ - public function handleKernelRequestEventForSymfonyVersionGreaterThan54DataProvider(): \Generator - { - if (version_compare(Kernel::VERSION, '5.4.0', '<')) { - return; - } - - yield 'token.user IS NULL' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new AuthenticatedTokenStub(null), - UserDataBag::createFromUserIpAddress('127.0.0.1'), - ]; - - yield 'token.user INSTANCEOF UserInterface' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new AuthenticatedTokenStub(new UserWithIdentifierStub()), - new UserDataBag(null, null, '127.0.0.1', 'foo_user'), - ]; - - yield 'token INSTANCEOF SwitchUserToken' => [ - new RequestEvent( - $this->createMock(HttpKernelInterface::class), - new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), - HttpKernelInterface::MASTER_REQUEST - ), - $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new SwitchUserToken( - new UserWithIdentifierStub(), - 'main', - ['ROLE_USER'], - new AuthenticatedTokenStub(new UserWithIdentifierStub('foo_user_impersonator')) - ), - UserDataBag::createFromArray([ - 'ip_address' => '127.0.0.1', - 'username' => 'foo_user', - 'impersonator_username' => 'foo_user_impersonator', - ]), - ]; - } - /** * @dataProvider handleKernelControllerEventDataProvider * @@ -412,35 +181,3 @@ private function getMockedClientWithOptions(Options $options): ClientInterface return $client; } } - -final class UnauthenticatedTokenStub extends AbstractToken -{ - public function getCredentials(): ?string - { - return null; - } -} - -final class AuthenticatedTokenStub extends AbstractToken -{ - /** - * @param UserInterface|\Stringable|string|null $user - */ - public function __construct($user) - { - parent::__construct(); - - if (method_exists($this, 'setAuthenticated')) { - $this->setAuthenticated(true); - } - - if (null !== $user) { - $this->setUser($user); - } - } - - public function getCredentials(): ?string - { - return null; - } -} From 9059f8ff13d6f365a27a03eec32476f2256b5737 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 11 May 2023 15:46:22 +0200 Subject: [PATCH 03/11] Update PHPStan baseline --- phpstan-baseline.neon | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 0dfd27df..6d132fc8 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -313,27 +313,12 @@ parameters: - message: "#^Call to function method_exists\\(\\) with \\$this\\(Sentry\\\\SentryBundle\\\\Tests\\\\EventListener\\\\AuthenticatedTokenStub\\) and 'setAuthenticated' will always evaluate to false\\.$#" count: 1 - path: tests/EventListener/RequestListenerTest.php + path: tests/EventListener/LoginListenerTest.php - message: "#^Parameter \\#1 \\$user of method Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\AbstractToken\\:\\:setUser\\(\\) expects Symfony\\\\Component\\\\Security\\\\Core\\\\User\\\\UserInterface, string\\|Stringable\\|Symfony\\\\Component\\\\Security\\\\Core\\\\User\\\\UserInterface given\\.$#" count: 1 - path: tests/EventListener/RequestListenerTest.php - - - - message: "#^Parameter \\#3 \\$roles of class Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\SwitchUserToken constructor expects array\\, string given\\.$#" - count: 1 - path: tests/EventListener/RequestListenerTest.php - - - - message: "#^Parameter \\#4 \\$originalToken of class Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\SwitchUserToken constructor expects Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\TokenInterface, array\\ given\\.$#" - count: 1 - path: tests/EventListener/RequestListenerTest.php - - - - message: "#^Parameter \\#5 \\$originatedFromUri of class Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\SwitchUserToken constructor expects string\\|null, Sentry\\\\SentryBundle\\\\Tests\\\\EventListener\\\\AuthenticatedTokenStub given\\.$#" - count: 1 - path: tests/EventListener/RequestListenerTest.php + path: tests/EventListener/LoginListenerTest.php - message: "#^Call to an undefined method Symfony\\\\Component\\\\HttpKernel\\\\Event\\\\KernelEvent\\:\\:isMasterRequest\\(\\)\\.$#" From b4c2bfce2d6efb3a45c350e009a69776c9db6f32 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 11 May 2023 15:58:51 +0200 Subject: [PATCH 04/11] Move `symfony/security-http` to `require` section --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index cad83e3d..55b77290 100644 --- a/composer.json +++ b/composer.json @@ -37,7 +37,8 @@ "symfony/http-kernel": "^4.4.20||^5.0.11||^6.0", "symfony/polyfill-php80": "^1.22", "symfony/psr-http-message-bridge": "^1.2||^2.0", - "symfony/security-core": "^4.4.20||^5.0.11||^6.0" + "symfony/security-core": "^4.4.20||^5.0.11||^6.0", + "symfony/security-http": "^4.4.20||^5.0.11||^6.0" }, "require-dev": { "doctrine/dbal": "^2.13||^3.0", @@ -61,7 +62,6 @@ "symfony/monolog-bundle": "^3.4", "symfony/phpunit-bridge": "^5.2.6||^6.0", "symfony/process": "^4.4.20||^5.0.11||^6.0", - "symfony/security-http": "^4.4.20||^5.0.11||^6.0", "symfony/twig-bundle": "^4.4.20||^5.0.11||^6.0", "symfony/yaml": "^4.4.20||^5.0.11||^6.0", "vimeo/psalm": "^4.3" From 788316456166166a082a2880e2d69ffcc6e8d66a Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Fri, 12 May 2023 00:21:06 +0200 Subject: [PATCH 05/11] Fix tests for the `LoginListener` class --- tests/EventListener/LoginListenerTest.php | 34 +++++++++++++++-------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/tests/EventListener/LoginListenerTest.php b/tests/EventListener/LoginListenerTest.php index ade8cb6d..81c96f7c 100644 --- a/tests/EventListener/LoginListenerTest.php +++ b/tests/EventListener/LoginListenerTest.php @@ -20,7 +20,6 @@ use Symfony\Component\Security\Core\Authentication\Token\AbstractToken; use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; -use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; use Symfony\Component\Security\Core\Event\AuthenticationSuccessEvent; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface; @@ -49,7 +48,7 @@ protected function setUp(): void /** * @dataProvider handleLoginSuccessEventDataProvider */ - public function testHandleLoginSuccessEvent(TokenInterface $token, ?UserDataBag $user, UserDataBag $expectedUser): void + public function testHandleLoginSuccessEvent(TokenInterface $token, ?UserDataBag $user, ?UserDataBag $expectedUser): void { if (!class_exists(LoginSuccessEvent::class)) { $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event does not exist.'); @@ -95,7 +94,7 @@ public function testHandleLoginSuccessEvent(TokenInterface $token, ?UserDataBag * @dataProvider handleLoginSuccessEventDataProvider * @dataProvider handleLoginSuccessEventForSymfonyVersionLowerThan54DataProvider */ - public function testHandleAuthenticationSuccessEvent(TokenInterface $token, ?UserDataBag $user, UserDataBag $expectedUser): void + public function testHandleAuthenticationSuccessEvent(TokenInterface $token, ?UserDataBag $user, ?UserDataBag $expectedUser): void { if (class_exists(LoginSuccessEvent::class)) { $this->markTestSkipped('This test is incompatible with versions of Symfony where the LoginSuccessEvent event exists.'); @@ -133,19 +132,36 @@ public function testHandleAuthenticationSuccessEvent(TokenInterface $token, ?Use public function handleLoginSuccessEventDataProvider(): \Generator { yield 'If the username is already set on the User context, then it is not overridden' => [ - new UsernamePasswordToken(new UserWithIdentifierStub(), 'main'), + new AuthenticatedTokenStub(new UserWithIdentifierStub()), new UserDataBag('bar_user'), new UserDataBag('bar_user'), ]; yield 'If the username is not set on the User context, then it is retrieved from the token' => [ - new UsernamePasswordToken(new UserWithIdentifierStub(), 'main'), + new AuthenticatedTokenStub(new UserWithIdentifierStub()), null, new UserDataBag('foo_user'), ]; yield 'If the user is being impersonated, then the username of the impersonator is set on the User context' => [ - new SwitchUserToken(new UserWithIdentifierStub(), 'main', [], new AuthenticatedTokenStub(new UserWithIdentifierStub('bar_user'))), + (static function (): SwitchUserToken { + if (version_compare(Kernel::VERSION, '5.0.0', '<')) { + return new SwitchUserToken( + new UserWithIdentifierStub(), + null, + 'foo_provider', + ['ROLE_USER'], + new AuthenticatedTokenStub(new UserWithIdentifierStub('bar_user')) + ); + } + + return new SwitchUserToken( + new UserWithIdentifierStub(), + 'main', + ['ROLE_USER'], + new AuthenticatedTokenStub(new UserWithIdentifierStub('bar_user')) + ); + })(), null, UserDataBag::createFromArray([ 'id' => 'foo_user', @@ -160,12 +176,6 @@ public function handleLoginSuccessEventForSymfonyVersionLowerThan54DataProvider( return; } - yield 'Given an authenticated token, if the user is not set, then the User context is not populated' => [ - new AuthenticatedTokenStub(null), - null, - null, - ]; - yield 'Given an authenticated token, if the user is a string, then the User context is populated' => [ new AuthenticatedTokenStub('foo_user'), null, From 847c4b375fff185e7101306b23714345d0b739d7 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Fri, 12 May 2023 00:29:06 +0200 Subject: [PATCH 06/11] Update PHPStan baseline (again) --- phpstan-baseline.neon | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 6d132fc8..856a0129 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -320,6 +320,26 @@ parameters: count: 1 path: tests/EventListener/LoginListenerTest.php + - + message: "#^Parameter \\#2 \\$firewallName of class Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\SwitchUserToken constructor expects string, null given\\.$#" + count: 1 + path: tests/EventListener/LoginListenerTest.php + + - + message: "#^Parameter \\#3 \\$roles of class Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\SwitchUserToken constructor expects array\\, string given\\.$#" + count: 1 + path: tests/EventListener/LoginListenerTest.php + + - + message: "#^Parameter \\#4 \\$originalToken of class Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\SwitchUserToken constructor expects Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\TokenInterface, array\\ given\\.$#" + count: 1 + path: tests/EventListener/LoginListenerTest.php + + - + message: "#^Parameter \\#5 \\$originatedFromUri of class Symfony\\\\Component\\\\Security\\\\Core\\\\Authentication\\\\Token\\\\SwitchUserToken constructor expects string\\|null, Sentry\\\\SentryBundle\\\\Tests\\\\EventListener\\\\AuthenticatedTokenStub given\\.$#" + count: 1 + path: tests/EventListener/LoginListenerTest.php + - message: "#^Call to an undefined method Symfony\\\\Component\\\\HttpKernel\\\\Event\\\\KernelEvent\\:\\:isMasterRequest\\(\\)\\.$#" count: 1 From ab593580e98a2391035d67944ff9a0f00fdc8938 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Sat, 17 Jun 2023 17:32:25 +0200 Subject: [PATCH 07/11] Remove unneeded check --- src/EventListener/LoginListener.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/EventListener/LoginListener.php b/src/EventListener/LoginListener.php index 0e2810da..5f0fa25e 100644 --- a/src/EventListener/LoginListener.php +++ b/src/EventListener/LoginListener.php @@ -47,10 +47,6 @@ public function handleLoginSuccessEvent(LoginSuccessEvent $event): void */ public function handleAuthenticationSuccessEvent(AuthenticationSuccessEvent $event): void { - if (class_exists(LoginSuccessEvent::class)) { - return; - } - $this->updateUserContext($event->getAuthenticationToken()); } From 57c64b7345b0e904ad2e8c2f9521c65651aceca6 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Tue, 18 Jul 2023 16:28:58 +0200 Subject: [PATCH 08/11] Avoid overwriting the user context on the scope in `RequestListener` class --- src/EventListener/RequestListener.php | 9 ++++-- tests/EventListener/RequestListenerTest.php | 32 +++++++++++++++------ 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/EventListener/RequestListener.php b/src/EventListener/RequestListener.php index e42388cf..9c4951fb 100644 --- a/src/EventListener/RequestListener.php +++ b/src/EventListener/RequestListener.php @@ -53,10 +53,13 @@ public function handleKernelRequestEvent(RequestEvent $event): void } $this->hub->configureScope(static function (Scope $scope) use ($event): void { - $userData = new UserDataBag(); - $userData->setIpAddress($event->getRequest()->getClientIp()); + $user = $scope->getUser() ?? new UserDataBag(); - $scope->setUser($userData); + if (null === $user->getIpAddress()) { + $user->setIpAddress($event->getRequest()->getClientIp()); + } + + $scope->setUser($user); }); } diff --git a/tests/EventListener/RequestListenerTest.php b/tests/EventListener/RequestListenerTest.php index c280e6f3..440db6e7 100644 --- a/tests/EventListener/RequestListenerTest.php +++ b/tests/EventListener/RequestListenerTest.php @@ -39,9 +39,10 @@ protected function setUp(): void /** * @dataProvider handleKernelRequestEventDataProvider */ - public function testHandleKernelRequestEvent(RequestEvent $requestEvent, ?ClientInterface $client, ?UserDataBag $expectedUser): void + public function testHandleKernelRequestEvent(RequestEvent $requestEvent, ClientInterface $client, UserDataBag $currentUser, UserDataBag $expectedUser): void { $scope = new Scope(); + $scope->setUser($currentUser); $this->hub->expects($this->any()) ->method('getClient') @@ -73,7 +74,8 @@ public function handleKernelRequestEventDataProvider(): \Generator HttpKernelInterface::SUB_REQUEST ), $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - null, + new UserDataBag(), + new UserDataBag(), ]; yield 'options.send_default_pii = FALSE' => [ @@ -83,27 +85,41 @@ public function handleKernelRequestEventDataProvider(): \Generator HttpKernelInterface::MASTER_REQUEST ), $this->getMockedClientWithOptions(new Options(['send_default_pii' => false])), - null, + new UserDataBag(), + new UserDataBag(), ]; - yield 'token IS NULL' => [ + yield 'request.clientIp IS NULL' => [ + new RequestEvent( + $this->createMock(HttpKernelInterface::class), + new Request(), + HttpKernelInterface::MASTER_REQUEST + ), + $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), + new UserDataBag(), + new UserDataBag(), + ]; + + yield 'user.ipAddress IS NULL && request.clientIp IS NOT NULL' => [ new RequestEvent( $this->createMock(HttpKernelInterface::class), new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), HttpKernelInterface::MASTER_REQUEST ), $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - UserDataBag::createFromUserIpAddress('127.0.0.1'), + new UserDataBag('foo_user'), + new UserDataBag('foo_user', null, '127.0.0.1'), ]; - yield 'request.clientIp IS NULL' => [ + yield 'user.ipAddress IS NOT NULL && request.clientIp IS NOT NULL' => [ new RequestEvent( $this->createMock(HttpKernelInterface::class), - new Request(), + new Request([], [], [], [], [], ['REMOTE_ADDR' => '127.0.0.1']), HttpKernelInterface::MASTER_REQUEST ), $this->getMockedClientWithOptions(new Options(['send_default_pii' => true])), - new UserDataBag(), + new UserDataBag('foo_user', null, '::1'), + new UserDataBag('foo_user', null, '::1'), ]; } From 05a2c3c0649cacdfde6f34196934e83bee65abd8 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Wed, 26 Jul 2023 11:41:50 +0200 Subject: [PATCH 09/11] Add back listener for `kernel.request` event --- src/EventListener/LoginListener.php | 32 ++++++++- src/Resources/config/services.xml | 2 + tests/EventListener/LoginListenerTest.php | 79 ++++++++++++++++++++++- 3 files changed, 110 insertions(+), 3 deletions(-) diff --git a/src/EventListener/LoginListener.php b/src/EventListener/LoginListener.php index 5f0fa25e..2429a6cc 100644 --- a/src/EventListener/LoginListener.php +++ b/src/EventListener/LoginListener.php @@ -7,6 +7,8 @@ use Sentry\State\HubInterface; use Sentry\State\Scope; use Sentry\UserDataBag; +use Symfony\Component\HttpKernel\Event\RequestEvent; +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\Event\AuthenticationSuccessEvent; @@ -15,19 +17,45 @@ final class LoginListener { + use KernelEventForwardCompatibilityTrait; + /** * @var HubInterface The current hub */ private $hub; + /** + * @var TokenStorageInterface The token storage + */ + private $tokenStorage; + /** * Constructor. * - * @param HubInterface $hub The current hub + * @param HubInterface $hub The current hub + * @param TokenStorageInterface|null $tokenStorage The token storage */ - public function __construct(HubInterface $hub) + public function __construct(HubInterface $hub, ?TokenStorageInterface $tokenStorage) { $this->hub = $hub; + $this->tokenStorage = $tokenStorage; + } + + /** + * This method is called for each request handled by the framework and + * fills the Sentry scope with information about the current user. + */ + public function handleKernelRequestEvent(RequestEvent $event): void + { + if (null === $this->tokenStorage || !$this->isMainRequest($event)) { + return; + } + + $token = $this->tokenStorage->getToken(); + + if (null !== $token) { + $this->updateUserContext($token); + } } /** diff --git a/src/Resources/config/services.xml b/src/Resources/config/services.xml index 79e3c319..3f67a790 100644 --- a/src/Resources/config/services.xml +++ b/src/Resources/config/services.xml @@ -88,7 +88,9 @@ + + diff --git a/tests/EventListener/LoginListenerTest.php b/tests/EventListener/LoginListenerTest.php index 81c96f7c..9ae944a5 100644 --- a/tests/EventListener/LoginListenerTest.php +++ b/tests/EventListener/LoginListenerTest.php @@ -16,8 +16,11 @@ use Sentry\State\Scope; use Sentry\UserDataBag; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\Event\RequestEvent; +use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\HttpKernel\Kernel; use Symfony\Component\Security\Core\Authentication\Token\AbstractToken; +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\Event\AuthenticationSuccessEvent; @@ -34,6 +37,11 @@ final class LoginListenerTest extends TestCase */ private $hub; + /** + * @var MockObject&TokenStorageInterface + */ + private $tokenStorage; + /** * @var LoginListener */ @@ -42,7 +50,51 @@ final class LoginListenerTest extends TestCase protected function setUp(): void { $this->hub = $this->createMock(HubInterface::class); - $this->listener = new LoginListener($this->hub); + $this->tokenStorage = $this->createMock(TokenStorageInterface::class); + $this->listener = new LoginListener($this->hub, $this->tokenStorage); + } + + /** + * @dataProvider handleLoginSuccessEventDataProvider + * @dataProvider handleLoginSuccessEventForSymfonyVersionLowerThan54DataProvider + */ + public function testHandleKernelRequestEvent(TokenInterface $token, ?UserDataBag $user, ?UserDataBag $expectedUser): void + { + $scope = new Scope(); + + $client = $this->createMock(ClientInterface::class); + $client->expects($this->once()) + ->method('getOptions') + ->willReturn(new Options(['send_default_pii' => true])); + + $this->hub->expects($this->once()) + ->method('getClient') + ->willReturn($client); + + $this->hub->expects($this->once()) + ->method('configureScope') + ->willReturnCallback(static function (callable $callback) use ($scope): void { + $callback($scope); + }); + + $this->tokenStorage->expects($this->once()) + ->method('getToken') + ->willReturn($token); + + if (null !== $user) { + $scope->setUser($user); + } + + $this->listener->handleKernelRequestEvent(new RequestEvent( + $this->createMock(HttpKernelInterface::class), + new Request(), + HttpKernelInterface::MASTER_REQUEST + )); + + $event = $scope->applyToEvent(Event::createEvent()); + + $this->assertNotNull($event); + $this->assertEquals($expectedUser, $event->getUser()); } /** @@ -200,6 +252,31 @@ public function __toString(): string ]; } + public function testHandleKernelRequestEventDoesNothingIfRequestIsNotMain(): void + { + $this->tokenStorage->expects($this->never()) + ->method('getToken'); + + $this->listener->handleKernelRequestEvent(new RequestEvent( + $this->createMock(HttpKernelInterface::class), + new Request(), + HttpKernelInterface::SUB_REQUEST + )); + } + + public function testHandleKernelRequestEventDoesNothingIfTokenIsNotSet(): void + { + $this->tokenStorage->expects($this->once()) + ->method('getToken') + ->willReturn(null); + + $this->listener->handleKernelRequestEvent(new RequestEvent( + $this->createMock(HttpKernelInterface::class), + new Request(), + HttpKernelInterface::MASTER_REQUEST + )); + } + public function testHandleLoginSuccessEventDoesNothingIfTokenIsNotAuthenticated(): void { if (!class_exists(LoginSuccessEvent::class)) { From 52673a723157509702f9c8071f7804eb360d7981 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Wed, 26 Jul 2023 16:12:45 +0200 Subject: [PATCH 10/11] Rename data providers and reword test case descriptions --- tests/EventListener/LoginListenerTest.php | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/EventListener/LoginListenerTest.php b/tests/EventListener/LoginListenerTest.php index 9ae944a5..44c1838b 100644 --- a/tests/EventListener/LoginListenerTest.php +++ b/tests/EventListener/LoginListenerTest.php @@ -55,8 +55,8 @@ protected function setUp(): void } /** - * @dataProvider handleLoginSuccessEventDataProvider - * @dataProvider handleLoginSuccessEventForSymfonyVersionLowerThan54DataProvider + * @dataProvider authenticationTokenDataProvider + * @dataProvider authenticationTokenForSymfonyVersionLowerThan54DataProvider */ public function testHandleKernelRequestEvent(TokenInterface $token, ?UserDataBag $user, ?UserDataBag $expectedUser): void { @@ -98,7 +98,7 @@ public function testHandleKernelRequestEvent(TokenInterface $token, ?UserDataBag } /** - * @dataProvider handleLoginSuccessEventDataProvider + * @dataProvider authenticationTokenDataProvider */ public function testHandleLoginSuccessEvent(TokenInterface $token, ?UserDataBag $user, ?UserDataBag $expectedUser): void { @@ -143,8 +143,8 @@ public function testHandleLoginSuccessEvent(TokenInterface $token, ?UserDataBag } /** - * @dataProvider handleLoginSuccessEventDataProvider - * @dataProvider handleLoginSuccessEventForSymfonyVersionLowerThan54DataProvider + * @dataProvider authenticationTokenDataProvider + * @dataProvider authenticationTokenForSymfonyVersionLowerThan54DataProvider */ public function testHandleAuthenticationSuccessEvent(TokenInterface $token, ?UserDataBag $user, ?UserDataBag $expectedUser): void { @@ -181,7 +181,7 @@ public function testHandleAuthenticationSuccessEvent(TokenInterface $token, ?Use $this->assertEquals($expectedUser, $event->getUser()); } - public function handleLoginSuccessEventDataProvider(): \Generator + public function authenticationTokenDataProvider(): \Generator { yield 'If the username is already set on the User context, then it is not overridden' => [ new AuthenticatedTokenStub(new UserWithIdentifierStub()), @@ -222,25 +222,25 @@ public function handleLoginSuccessEventDataProvider(): \Generator ]; } - public function handleLoginSuccessEventForSymfonyVersionLowerThan54DataProvider(): \Generator + public function authenticationTokenForSymfonyVersionLowerThan54DataProvider(): \Generator { if (version_compare(Kernel::VERSION, '5.4.0', '>=')) { return; } - yield 'Given an authenticated token, if the user is a string, then the User context is populated' => [ + yield 'If the user is a string, then the value is used as-is' => [ new AuthenticatedTokenStub('foo_user'), null, new UserDataBag('foo_user'), ]; - yield 'Given an authenticated token, if the user is an instance of the UserInterface interface but the getUserIdentifier() method does not exist, then the User context is populated by calling the getUsername() method' => [ + yield 'If the user is an instance of the UserInterface interface but the getUserIdentifier() method does not exist, then the getUsername() method is invoked' => [ new AuthenticatedTokenStub(new UserWithoutIdentifierStub()), null, new UserDataBag('foo_user'), ]; - yield 'Given an authenticated token, if the user is an object implementing the Stringable interface, then the User context is populated by calling the __toString() method' => [ + yield 'If the user is an object implementing the Stringable interface, then the __toString() method is invoked' => [ new AuthenticatedTokenStub(new class() implements \Stringable { public function __toString(): string { From ecf154b3f7de923dee1bc3ba851af2022cef2ca9 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Wed, 26 Jul 2023 22:52:15 +0200 Subject: [PATCH 11/11] Fix errors reported by PHPStan and update baseline --- phpstan-baseline.neon | 5 +++++ src/EventListener/LoginListener.php | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 856a0129..c04293a5 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -160,6 +160,11 @@ parameters: count: 1 path: src/EventListener/ConsoleCommandListener.php + - + message: "#^Call to an undefined method Symfony\\\\Component\\\\HttpKernel\\\\Event\\\\KernelEvent\\:\\:isMasterRequest\\(\\)\\.$#" + count: 1 + path: src/EventListener/LoginListener.php + - message: "#^Call to an undefined method Symfony\\\\Component\\\\HttpKernel\\\\Event\\\\KernelEvent\\:\\:isMasterRequest\\(\\)\\.$#" count: 1 diff --git a/src/EventListener/LoginListener.php b/src/EventListener/LoginListener.php index 2429a6cc..6e6d47a8 100644 --- a/src/EventListener/LoginListener.php +++ b/src/EventListener/LoginListener.php @@ -25,7 +25,7 @@ final class LoginListener private $hub; /** - * @var TokenStorageInterface The token storage + * @var TokenStorageInterface|null The token storage */ private $tokenStorage;