From 31ed97310d14b113d04687d2813c0be1b1348816 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 17 Sep 2024 13:46:34 +0200 Subject: [PATCH] Add the session id derived id to polling requests The polling async requests should include the correlation id based on the session id of the user that is placing them. That way we can correlate between the access logs and the application logs. See: https://www.pivotaltracker.com/story/show/188205232 --- assets/typescript/Client/StatusClient.ts | 10 ++++-- assets/typescript/authentication.ts | 6 ++-- assets/typescript/registration.ts | 10 ++++-- src/Controller/AuthenticationController.php | 6 ++-- src/Controller/RegistrationController.php | 3 ++ ...RequiresActiveSessionAttributeListener.php | 3 +- src/EventSubscriber/SessionStateListener.php | 1 + src/Service/SessionCorrelationIdService.php | 1 + src/Tiqr/Legacy/TiqrService.php | 1 - templates/default/authentication.html.twig | 3 +- templates/default/registration.html.twig | 3 +- ...iresActiveSessionAttributeListenerTest.php | 33 ++++++++++++++++--- .../SessionStateListenerTest.php | 24 +++++++++++--- .../SessionCorrelationIdServiceTest.php | 4 +-- .../Session/LoggingSessionFactoryTest.php | 2 +- 15 files changed, 83 insertions(+), 27 deletions(-) diff --git a/assets/typescript/Client/StatusClient.ts b/assets/typescript/Client/StatusClient.ts index a9babc3e..047fa1f5 100644 --- a/assets/typescript/Client/StatusClient.ts +++ b/assets/typescript/Client/StatusClient.ts @@ -5,14 +5,18 @@ export interface PendingRequest { } export class StatusClient { - constructor(private apiUrl: string) { - + constructor( + private apiUrl: string, + private correlationLoggingId: string, + ) { } /** * Request status form the API. */ public request(callback: (status: string) => void, errorCallback: (error: unknown) => void): PendingRequest { - return jQuery.get(this.apiUrl, callback).fail(errorCallback); + const url = this.apiUrl + (this.apiUrl.includes('?') ? '&' : '?') + 'correlation-id=' + this.correlationLoggingId; + + return jQuery.get(url, callback).fail(errorCallback); } } diff --git a/assets/typescript/authentication.ts b/assets/typescript/authentication.ts index b38e13ed..e56e8751 100644 --- a/assets/typescript/authentication.ts +++ b/assets/typescript/authentication.ts @@ -8,12 +8,12 @@ import jQuery from 'jquery'; declare global { interface Window { - bootstrapAuthentication: (statusApiUrl: string, notificationApiUrl: string) => AuthenticationPageService; + bootstrapAuthentication: (statusApiUrl: string, notificationApiUrl: string, correlationLoggingId: string) => AuthenticationPageService; } } -window.bootstrapAuthentication = (statusApiUrl: string, notificationApiUrl: string) => { - const statusClient = new StatusClient(statusApiUrl); +window.bootstrapAuthentication = (statusApiUrl: string, notificationApiUrl: string, correlationLoggingId: string) => { + const statusClient = new StatusClient(statusApiUrl, correlationLoggingId); const notificationClient = new NotificationClient(notificationApiUrl); const pollingService = new StatusPollService(statusClient); diff --git a/assets/typescript/registration.ts b/assets/typescript/registration.ts index 98328d19..9588762c 100644 --- a/assets/typescript/registration.ts +++ b/assets/typescript/registration.ts @@ -7,12 +7,16 @@ import jQuery from 'jquery'; declare global { interface Window { - bootstrapRegistration: (statusApiUrl: string, notificationApiUrl: string) => RegistrationStateMachine; + bootstrapRegistration: ( + statusApiUrl: string, + notificationApiUrl: string, + correlationLoggingId: string + ) => RegistrationStateMachine; } } -window.bootstrapRegistration = (statusApiUrl: string, finalizedUrl: string) => { - const statusClient = new StatusClient(statusApiUrl); +window.bootstrapRegistration = (statusApiUrl: string, finalizedUrl: string, correlationLoggingId: string) => { + const statusClient = new StatusClient(statusApiUrl, correlationLoggingId); const pollingService = new StatusPollService(statusClient); const machine = new RegistrationStateMachine( pollingService, diff --git a/src/Controller/AuthenticationController.php b/src/Controller/AuthenticationController.php index 79bfd9f0..5eb2a5b8 100644 --- a/src/Controller/AuthenticationController.php +++ b/src/Controller/AuthenticationController.php @@ -29,6 +29,7 @@ use Surfnet\Tiqr\Exception\UserNotFoundException; use Surfnet\Tiqr\Exception\UserPermanentlyBlockedException; use Surfnet\Tiqr\Exception\UserTemporarilyBlockedException; +use Surfnet\Tiqr\Service\SessionCorrelationIdService; use Surfnet\Tiqr\Tiqr\AuthenticationRateLimitServiceInterface; use Surfnet\Tiqr\Tiqr\Exception\UserNotExistsException; use Surfnet\Tiqr\Tiqr\Response\AuthenticationResponse; @@ -53,6 +54,7 @@ public function __construct( private readonly TiqrServiceInterface $tiqrService, private readonly TiqrUserRepositoryInterface $userRepository, private readonly AuthenticationRateLimitServiceInterface $authenticationRateLimitService, + private readonly SessionCorrelationIdService $correlationIdService, private readonly LoggerInterface $logger ) { } @@ -145,8 +147,8 @@ public function __invoke(Request $request): Response $logger->info('Return authentication page with QR code'); return $this->render('default/authentication.html.twig', [ - // TODO: Add something identifying the authentication session to the authenticateUrl - 'authenticateUrl' => $this->tiqrService->authenticationUrl() + 'authenticateUrl' => $this->tiqrService->authenticationUrl(), + 'correlationLoggingId' => $this->correlationIdService->generateCorrelationId(), ]); } diff --git a/src/Controller/RegistrationController.php b/src/Controller/RegistrationController.php index 67f0a3ba..69f7b1ad 100644 --- a/src/Controller/RegistrationController.php +++ b/src/Controller/RegistrationController.php @@ -25,6 +25,7 @@ use Surfnet\GsspBundle\Service\StateHandlerInterface; use Surfnet\Tiqr\Attribute\RequiresActiveSession; use Surfnet\Tiqr\Exception\NoActiveAuthenrequestException; +use Surfnet\Tiqr\Service\SessionCorrelationIdService; use Surfnet\Tiqr\Tiqr\Legacy\TiqrService; use Surfnet\Tiqr\Tiqr\TiqrServiceInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; @@ -38,6 +39,7 @@ public function __construct( private readonly RegistrationService $registrationService, private readonly StateHandlerInterface $stateHandler, private readonly TiqrServiceInterface $tiqrService, + private readonly SessionCorrelationIdService $correlationIdService, private readonly LoggerInterface $logger ) { } @@ -81,6 +83,7 @@ public function registration(Request $request): Response 'default/registration.html.twig', [ 'metadataUrl' => sprintf("tiqrenroll://%s", $metadataUrl), + 'correlationLoggingId' => $this->correlationIdService->generateCorrelationId(), 'enrollmentKey' => $key ] ); diff --git a/src/EventSubscriber/RequiresActiveSessionAttributeListener.php b/src/EventSubscriber/RequiresActiveSessionAttributeListener.php index beb7a5a3..4c6c5af7 100644 --- a/src/EventSubscriber/RequiresActiveSessionAttributeListener.php +++ b/src/EventSubscriber/RequiresActiveSessionAttributeListener.php @@ -41,8 +41,9 @@ private string $sessionName; public function __construct( - private LoggerInterface $logger, + private LoggerInterface $logger, private SessionCorrelationIdService $sessionCorrelationIdService, + /** @var array */ private array $sessionOptions, ) { if (!array_key_exists('name', $this->sessionOptions)) { diff --git a/src/EventSubscriber/SessionStateListener.php b/src/EventSubscriber/SessionStateListener.php index a03a774b..1bc89e58 100644 --- a/src/EventSubscriber/SessionStateListener.php +++ b/src/EventSubscriber/SessionStateListener.php @@ -39,6 +39,7 @@ public function __construct( private LoggerInterface $logger, private SessionCorrelationIdService $sessionCorrelationIdService, + /** @var array */ private array $sessionOptions, ) { if (!array_key_exists('name', $this->sessionOptions)) { diff --git a/src/Service/SessionCorrelationIdService.php b/src/Service/SessionCorrelationIdService.php index 76e72698..1338f156 100644 --- a/src/Service/SessionCorrelationIdService.php +++ b/src/Service/SessionCorrelationIdService.php @@ -29,6 +29,7 @@ public function __construct( private RequestStack $requestStack, + /** @var array */ private array $sessionOptions, private string $sessionCorrelationSalt, ) { diff --git a/src/Tiqr/Legacy/TiqrService.php b/src/Tiqr/Legacy/TiqrService.php index 3e77d905..181bbb25 100644 --- a/src/Tiqr/Legacy/TiqrService.php +++ b/src/Tiqr/Legacy/TiqrService.php @@ -38,7 +38,6 @@ use Tiqr_Service; use Tiqr_StateStorage_StateStorageInterface; - /** * Wrapper around the legacy Tiqr service. * diff --git a/templates/default/authentication.html.twig b/templates/default/authentication.html.twig index 0cd8feac..04a64d68 100644 --- a/templates/default/authentication.html.twig +++ b/templates/default/authentication.html.twig @@ -16,7 +16,8 @@ */ var authenticationPageService = window.bootstrapAuthentication( "{{ path('app_identity_authentication_status') | escape('js') }}", - "{{ path('app_identity_authentication_notification') | escape('js') }}" + "{{ path('app_identity_authentication_notification') | escape('js') }}", + "{{ correlationLoggingId }}" ); {% endblock %} diff --git a/templates/default/registration.html.twig b/templates/default/registration.html.twig index 4bef79de..f04d8320 100644 --- a/templates/default/registration.html.twig +++ b/templates/default/registration.html.twig @@ -13,7 +13,8 @@ {% endblock %} diff --git a/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php b/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php index 0f310f0b..e73c23db 100644 --- a/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php +++ b/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php @@ -66,7 +66,14 @@ public function testControllersWithoutTheRequiresActiveSessionAttributeAreIgnore $mockLogger = Mockery::mock(LoggerInterface::class); $mockLogger->shouldNotReceive('log'); - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService( + $requestStack, + ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ' + ), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -103,7 +110,11 @@ public function testItDeniesAccessWhenThereIsNoActiveSession(): void ['correlationId' => '', 'route' => '/route'] ); - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -144,7 +155,11 @@ public function testItDeniesAccessWhenThereIsNoSessionCookie(): void ['correlationId' => '', 'route' => '/route'] ); - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -185,7 +200,11 @@ public function testItDeniesAccessWhenTheActiveSessionDoesNotMatchTheSessionCook ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route'] ); - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -219,7 +238,11 @@ public function testItDoesNotThrowWhenTheActiveSessionMatchesTheSessionCookie(): $mockLogger = Mockery::mock(LoggerInterface::class); $mockLogger->shouldNotReceive('log'); - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); diff --git a/tests/Unit/EventSubscriber/SessionStateListenerTest.php b/tests/Unit/EventSubscriber/SessionStateListenerTest.php index 580bddbc..d5367259 100644 --- a/tests/Unit/EventSubscriber/SessionStateListenerTest.php +++ b/tests/Unit/EventSubscriber/SessionStateListenerTest.php @@ -56,7 +56,11 @@ public function testItLogsWhenUserHasNoSessionCookie(): void ->once() ->with(LogLevel::INFO, 'User made a request without a session cookie.', ['correlationId' => '', 'route' => '/route']); - $listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSIONID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -85,7 +89,11 @@ public function testItLogsWhenUserHasNoSession(): void ->once() ->with(LogLevel::INFO, 'Session not found.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - $listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -121,7 +129,11 @@ public function testItLogsAnErrorWhenTheSessionIdDoesNotMatchTheSessionCookie(): ->once() ->with(LogLevel::ERROR, 'The session cookie does not match the session id.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - $listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -158,7 +170,11 @@ public function testTheUserSessionMatchesTheSessionCookie(): void ->once() ->with(LogLevel::INFO, 'User session matches the session cookie.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - $listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); $dispatcher->dispatch($event, KernelEvents::REQUEST); diff --git a/tests/Unit/Service/SessionCorrelationIdServiceTest.php b/tests/Unit/Service/SessionCorrelationIdServiceTest.php index 8ff8c178..996a83c0 100644 --- a/tests/Unit/Service/SessionCorrelationIdServiceTest.php +++ b/tests/Unit/Service/SessionCorrelationIdServiceTest.php @@ -30,7 +30,7 @@ public function testItHasNoCorrelationIdWhenThereIsNoSessionCookie(): void $requestStack = new RequestStack(); $requestStack->push($request); - $service = new SessionCorrelationIdService($requestStack); + $service = new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'); $this->assertNull($service->generateCorrelationId()); } @@ -41,7 +41,7 @@ public function testItGeneratesACorrelationIdBasedOnTheSessionCookie(): void $requestStack = new RequestStack(); $requestStack->push($request); - $service = new SessionCorrelationIdService($requestStack); + $service = new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'); $this->assertSame('f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', $service->generateCorrelationId()); } diff --git a/tests/Unit/Session/LoggingSessionFactoryTest.php b/tests/Unit/Session/LoggingSessionFactoryTest.php index 43d625f5..6399f95e 100644 --- a/tests/Unit/Session/LoggingSessionFactoryTest.php +++ b/tests/Unit/Session/LoggingSessionFactoryTest.php @@ -45,7 +45,7 @@ public function testItLogsWheneverASessionIsCreated(): void $requestStack, $this->createStub(SessionStorageFactoryInterface::class), $mockLogger, - new SessionCorrelationIdService($requestStack), + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), ); $this->assertInstanceOf(SessionInterface::class, $sessionFactory->createSession());