From 3480d7f21e705421f452c77c6e341aea7b94edf3 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 | 15 +- assets/typescript/authentication.ts | 6 +- assets/typescript/registration.ts | 10 +- ci/qa/phpunit.xml | 2 +- composer.json | 1 - composer.lock | 136 +----------------- dev/Command/AuthenticationCommand.php | 1 - src/Controller/AuthenticationController.php | 6 +- src/Controller/AuthenticationQrController.php | 1 - src/Controller/RegistrationController.php | 3 + ...RequiresActiveSessionAttributeListener.php | 2 +- templates/default/authentication.html.twig | 3 +- templates/default/registration.html.twig | 3 +- ...iresActiveSessionAttributeListenerTest.php | 1 - .../SessionStateListenerTest.php | 1 - 15 files changed, 36 insertions(+), 155 deletions(-) diff --git a/assets/typescript/Client/StatusClient.ts b/assets/typescript/Client/StatusClient.ts index a9babc3e..d2a90fdf 100644 --- a/assets/typescript/Client/StatusClient.ts +++ b/assets/typescript/Client/StatusClient.ts @@ -5,14 +5,23 @@ 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); + let url; + if(this.correlationLoggingId !== ''){ + url = this.apiUrl + (this.apiUrl.includes('?') ? '&' : '?') + 'correlation-id=' + this.correlationLoggingId; + }else{ + url = this.apiUrl; + } + + 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/ci/qa/phpunit.xml b/ci/qa/phpunit.xml index 8d17dbb5..0f05cc07 100644 --- a/ci/qa/phpunit.xml +++ b/ci/qa/phpunit.xml @@ -17,7 +17,7 @@ - + diff --git a/composer.json b/composer.json index e269211f..862bbeb0 100644 --- a/composer.json +++ b/composer.json @@ -48,7 +48,6 @@ "khanamiryan/qrcode-detector-decoder": "^2.0", "league/csv": "^9.13", "malukenho/docheader": "^1", - "mockery/mockery": "^1.6", "overtrue/phplint": "*", "phpmd/phpmd": "^2.15", "phpstan/phpstan": "^1.10", diff --git a/composer.lock b/composer.lock index c94b49b7..dbc64042 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "be9d520404343ee4e12d3071c5bf78e1", + "content-hash": "364d7351b9f4930e8d29773e7330e973", "packages": [ { "name": "beberlei/assert", @@ -8400,57 +8400,6 @@ "abandoned": "guzzlehttp/guzzle", "time": "2014-01-28T22:29:15+00:00" }, - { - "name": "hamcrest/hamcrest-php", - "version": "v2.0.1", - "source": { - "type": "git", - "url": "https://github.com/hamcrest/hamcrest-php.git", - "reference": "8c3d0a3f6af734494ad8f6fbbee0ba92422859f3" - }, - "dist": { - "type": "zip", - "url": "https://api.github.com/repos/hamcrest/hamcrest-php/zipball/8c3d0a3f6af734494ad8f6fbbee0ba92422859f3", - "reference": "8c3d0a3f6af734494ad8f6fbbee0ba92422859f3", - "shasum": "" - }, - "require": { - "php": "^5.3|^7.0|^8.0" - }, - "replace": { - "cordoval/hamcrest-php": "*", - "davedevelopment/hamcrest-php": "*", - "kodova/hamcrest-php": "*" - }, - "require-dev": { - "phpunit/php-file-iterator": "^1.4 || ^2.0", - "phpunit/phpunit": "^4.8.36 || ^5.7 || ^6.5 || ^7.0" - }, - "type": "library", - "extra": { - "branch-alias": { - "dev-master": "2.1-dev" - } - }, - "autoload": { - "classmap": [ - "hamcrest" - ] - }, - "notification-url": "https://packagist.org/downloads/", - "license": [ - "BSD-3-Clause" - ], - "description": "This is the PHP port of Hamcrest Matchers", - "keywords": [ - "test" - ], - "support": { - "issues": "https://github.com/hamcrest/hamcrest-php/issues", - "source": "https://github.com/hamcrest/hamcrest-php/tree/v2.0.1" - }, - "time": "2020-07-09T08:09:16+00:00" - }, { "name": "instaclick/php-webdriver", "version": "1.4.19", @@ -8850,89 +8799,6 @@ }, "time": "2024-03-31T07:05:07+00:00" }, - { - "name": "mockery/mockery", - "version": "1.6.12", - "source": { - "type": "git", - "url": "https://github.com/mockery/mockery.git", - "reference": "1f4efdd7d3beafe9807b08156dfcb176d18f1699" - }, - "dist": { - "type": "zip", - "url": "https://api.github.com/repos/mockery/mockery/zipball/1f4efdd7d3beafe9807b08156dfcb176d18f1699", - "reference": "1f4efdd7d3beafe9807b08156dfcb176d18f1699", - "shasum": "" - }, - "require": { - "hamcrest/hamcrest-php": "^2.0.1", - "lib-pcre": ">=7.0", - "php": ">=7.3" - }, - "conflict": { - "phpunit/phpunit": "<8.0" - }, - "require-dev": { - "phpunit/phpunit": "^8.5 || ^9.6.17", - "symplify/easy-coding-standard": "^12.1.14" - }, - "type": "library", - "autoload": { - "files": [ - "library/helpers.php", - "library/Mockery.php" - ], - "psr-4": { - "Mockery\\": "library/Mockery" - } - }, - "notification-url": "https://packagist.org/downloads/", - "license": [ - "BSD-3-Clause" - ], - "authors": [ - { - "name": "Pádraic Brady", - "email": "padraic.brady@gmail.com", - "homepage": "https://github.com/padraic", - "role": "Author" - }, - { - "name": "Dave Marshall", - "email": "dave.marshall@atstsolutions.co.uk", - "homepage": "https://davedevelopment.co.uk", - "role": "Developer" - }, - { - "name": "Nathanael Esayeas", - "email": "nathanael.esayeas@protonmail.com", - "homepage": "https://github.com/ghostwriter", - "role": "Lead Developer" - } - ], - "description": "Mockery is a simple yet flexible PHP mock object framework", - "homepage": "https://github.com/mockery/mockery", - "keywords": [ - "BDD", - "TDD", - "library", - "mock", - "mock objects", - "mockery", - "stub", - "test", - "test double", - "testing" - ], - "support": { - "docs": "https://docs.mockery.io/", - "issues": "https://github.com/mockery/mockery/issues", - "rss": "https://github.com/mockery/mockery/releases.atom", - "security": "https://github.com/mockery/mockery/security/advisories", - "source": "https://github.com/mockery/mockery" - }, - "time": "2024-05-16T03:13:13+00:00" - }, { "name": "myclabs/deep-copy", "version": "1.12.0", diff --git a/dev/Command/AuthenticationCommand.php b/dev/Command/AuthenticationCommand.php index a981d8e7..64dbaf5f 100644 --- a/dev/Command/AuthenticationCommand.php +++ b/dev/Command/AuthenticationCommand.php @@ -174,7 +174,6 @@ protected function decorateResult(string $text): string protected function readAuthenticationLinkFromFile(string $file, OutputInterface $output): string { $qrcode = new QrReader(file_get_contents($file), QrReader::SOURCE_TYPE_BLOB); - /** @phpstan-var mixed $link */ $link = $qrcode->text(); if (!is_string($link)) { diff --git a/src/Controller/AuthenticationController.php b/src/Controller/AuthenticationController.php index 0fadc7bc..635bf2b2 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 ) { } @@ -72,7 +74,6 @@ public function __invoke(Request $request): Response $logger->info('Verifying if there is a pending authentication request from SP'); - // Do have a valid sample AuthnRequest? if (!$this->authenticationService->authenticationRequired()) { $logger->error('There is no pending authentication request from SP'); @@ -145,7 +146,8 @@ public function __invoke(Request $request): Response $logger->info('Return authentication page with QR code'); return $this->render('default/authentication.html.twig', [ - 'authenticateUrl' => $this->tiqrService->authenticationUrl() + 'authenticateUrl' => $this->tiqrService->authenticationUrl(), + 'correlationLoggingId' => $this->correlationIdService->generateCorrelationId(), ]); } diff --git a/src/Controller/AuthenticationQrController.php b/src/Controller/AuthenticationQrController.php index b7260e03..78fc71a6 100644 --- a/src/Controller/AuthenticationQrController.php +++ b/src/Controller/AuthenticationQrController.php @@ -53,7 +53,6 @@ public function __invoke(): Response $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]); $logger->info('Client request QR image'); - // Do have a valid sample AuthnRequest?. if (!$this->authenticationService->authenticationRequired()) { $logger->error('There is no pending authentication request from SP'); 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 1c802961..4ddbbd78 100644 --- a/src/EventSubscriber/RequiresActiveSessionAttributeListener.php +++ b/src/EventSubscriber/RequiresActiveSessionAttributeListener.php @@ -44,7 +44,7 @@ * @param array $sessionOptions */ public function __construct( - private LoggerInterface $logger, + private LoggerInterface $logger, private SessionCorrelationIdService $sessionCorrelationIdService, private array $sessionOptions, ) { 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 55f5ea5f..378707e4 100644 --- a/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php +++ b/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php @@ -17,7 +17,6 @@ namespace Unit\EventSubscriber; -use Mockery; use Psr\Log\LoggerInterface; use Psr\Log\LogLevel; use Surfnet\Tiqr\Attribute\RequiresActiveSession; diff --git a/tests/Unit/EventSubscriber/SessionStateListenerTest.php b/tests/Unit/EventSubscriber/SessionStateListenerTest.php index 03e28235..03b506b4 100644 --- a/tests/Unit/EventSubscriber/SessionStateListenerTest.php +++ b/tests/Unit/EventSubscriber/SessionStateListenerTest.php @@ -17,7 +17,6 @@ namespace Unit\EventSubscriber; -use Mockery; use Psr\Log\LoggerInterface; use Psr\Log\LogLevel; use Surfnet\Tiqr\EventSubscriber\SessionStateListener;