From c4e484aa85198c77080dc8d627721506f9e25146 Mon Sep 17 00:00:00 2001 From: Pieter van der Meulen Date: Mon, 2 Sep 2024 11:13:00 +0200 Subject: [PATCH 1/5] First idea's for better handling of timeouts and sending a session identifier along with the polls --- src/Controller/RegistrationController.php | 10 +- src/Tiqr/Legacy/TiqrService.php | 28 +++-- yarn.lock | 139 ++++++++++++++++++---- 3 files changed, 142 insertions(+), 35 deletions(-) diff --git a/src/Controller/RegistrationController.php b/src/Controller/RegistrationController.php index 6209e2f5..80468c3c 100644 --- a/src/Controller/RegistrationController.php +++ b/src/Controller/RegistrationController.php @@ -50,7 +50,7 @@ public function registration(Request $request): Response { $this->logger->info('Verifying if there is a pending registration from SP'); - // Do have a valid sample AuthnRequest?. + // Do we have a valid GSSP registration AuthnRequest in this session? if (!$this->registrationService->registrationRequired()) { $this->logger->warning('Registration is not required'); throw new NoActiveAuthenrequestException(); @@ -89,17 +89,22 @@ public function registration(Request $request): Response * * * @throws \InvalidArgumentException + * + * Requires session cookie to be set to a valid session. */ #[Route(path: '/registration/status', name: 'app_identity_registration_status', methods: ['GET'])] public function registrationStatus() : Response { $this->logger->info('Request for registration status'); - // Do have a valid sample AuthnRequest?. + // Do we have a valid GSSP registration AuthnRequest in this session? if (!$this->registrationService->registrationRequired()) { $this->logger->error('There is no pending registration request'); return new Response('No registration required', Response::HTTP_BAD_REQUEST); } + + // TODO: Check whether enrollment is expired here? + $status = $this->tiqrService->getEnrollmentStatus(); $this->logger->info(sprintf('Send json response status "%s"', $status)); return new Response((string) $this->tiqrService->getEnrollmentStatus()); @@ -118,6 +123,7 @@ public function registrationQr(Request $request, string $enrollmentKey): Respons { $this->logger->info('Request for registration QR img'); + // Do we have a valid GSSP registration AuthnRequest in this session? if (!$this->registrationService->registrationRequired()) { $this->logger->error('There is no pending registration request'); diff --git a/src/Tiqr/Legacy/TiqrService.php b/src/Tiqr/Legacy/TiqrService.php index 0da05dc3..a058f728 100644 --- a/src/Tiqr/Legacy/TiqrService.php +++ b/src/Tiqr/Legacy/TiqrService.php @@ -95,16 +95,22 @@ public function generateEnrollmentKey(string $sari): string { $this->initSession(); - $this->logger->debug('Generating userId'); + // We use a randomly generated user ID + $this->logger->debug('Generating tiqr userId'); $userId = $this->generateId(); - $this->logger->debug('Storing the userId to session state'); + $this->logger->debug('Storing the userId=' . $userId . ' to session state'); $this->session->set('userId', $userId); + + // The session ID is used to link the tiqr library's enrollment session to the user's browser session $sessionId = $this->session->getId(); $this->logger->debug('Clearing the previous enrollment state(s)'); try { $this->clearPreviousEnrollmentState(); - $this->logger->debug('Starting the new enrollment session'); + $this->logger->notice('Starting new enrollment session with sessionId ' . $sessionId . + ' and userId ' . $userId); + // accountName is the display name of the account that is shown in the tiqr app + // However, we set it to the name of the tiqr service. $enrollmentKey = $this->tiqrService->startEnrollmentSession($userId, $this->accountName, $sessionId); $this->logger->debug('Storing the enrollment key for future reference'); $this->storeEnrollmentKey($enrollmentKey); @@ -373,12 +379,10 @@ public function getSariForSessionIdentifier(string $identifier): string * (3 for enrollment, 1 for authentication). This allows us to correlate the actions of the * user's browser with those of the user's phone. * - * Because the enrollment identifier are sensitive two measures are implemented: + * Because the enrollment identifier is sensitive two measures are implemented: * - Only the first 8 characters of the identifiers are logged * - The identifiers are stored as a stable hash of the identifier to hide the rest of the identifier. * - * Note that tiqr currently stores these identifiers in plain in its state storage - * * @param string $identifier Session identifier: enrollment key or session key * @param string $sari The to associate with $identifier * @throws TiqrServerRuntimeException @@ -392,7 +396,7 @@ private function setSariForSessionIdentifier(string $identifier, string $sari): $hashedIdentifier = $this->getHashedIdentifier($identifier); $this->tiqrStateStorage->setValue('sari_' . $hashedIdentifier, $sari, 60 * 60); } catch (Exception $e) { - // Catch errors from the tiqr-server and up-cycle them to exceptions that are meaningful to our domain + // Catch errors from the tiqr-server and up-cycle them to exceptions that are meaningful to our domain throw TiqrServerRuntimeException::fromOriginalException($e); } } @@ -437,4 +441,14 @@ public function stateStorageHealthCheck(): HealthCheckResultDto return HealthCheckResultDto::fromHealthCheckInterface($this->tiqrStateStorage); } + + protected function getAuthenticationTimeout(): int + { + return Tiqr_Service::CHALLENGE_EXPIRE; + } + + protected function getEnrollmentTimeout(): int + { + return Tiqr_Service::ENROLLMENT_EXPIRE; + } } diff --git a/yarn.lock b/yarn.lock index f2c6425c..e46446bc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2349,10 +2349,10 @@ blamer@^1.0.4: execa "^4.0.0" which "^2.0.2" -body-parser@1.20.2: - version "1.20.2" - resolved "https://registry.yarnpkg.com/body-parser/-/body-parser-1.20.2.tgz#6feb0e21c4724d06de7ff38da36dad4f57a747fd" - integrity sha512-ml9pReCu3M61kGlqoTm2umSXTlRTuGTx0bfYj+uIUKKYycG5NtSbeetV3faSU6R7ajOPw0g/J1PvK4qNy7s5bA== +body-parser@1.20.3: + version "1.20.3" + resolved "https://registry.yarnpkg.com/body-parser/-/body-parser-1.20.3.tgz#1953431221c6fb5cd63c4b36d53fab0928e548c6" + integrity sha512-7rAxByjUMqQ3/bHJy7D6OGXvx/MMc4IqBn/X0fcM1QUcAItpZrBEYhWGem+tzXH90c+G01ypMcYJBO9Y30203g== dependencies: bytes "3.1.2" content-type "~1.0.5" @@ -2362,7 +2362,7 @@ body-parser@1.20.2: http-errors "2.0.0" iconv-lite "0.4.24" on-finished "2.4.1" - qs "6.11.0" + qs "6.13.0" raw-body "2.5.2" type-is "~1.6.18" unpipe "1.0.0" @@ -2461,6 +2461,17 @@ call-bind@^1.0.2, call-bind@^1.0.5, call-bind@^1.0.6: get-intrinsic "^1.2.3" set-function-length "^1.2.0" +call-bind@^1.0.7: + version "1.0.7" + resolved "https://registry.yarnpkg.com/call-bind/-/call-bind-1.0.7.tgz#06016599c40c56498c18769d2730be242b6fa3b9" + integrity sha512-GHTSNSYICQ7scH7sZ+M2rFopRoLh8t2bLSW6BbgrtLsahOIB5iyAVJf9GjWK3cYTDaMj4XdBpM1cA6pIS0Kv2w== + dependencies: + es-define-property "^1.0.0" + es-errors "^1.3.0" + function-bind "^1.1.2" + get-intrinsic "^1.2.4" + set-function-length "^1.2.1" + callsites@^3.0.0: version "3.1.0" resolved "https://registry.yarnpkg.com/callsites/-/callsites-3.1.0.tgz#b3630abd8943432f54b3f0519238e33cd7df2f73" @@ -3005,6 +3016,15 @@ define-data-property@^1.0.1, define-data-property@^1.1.2: gopd "^1.0.1" has-property-descriptors "^1.0.1" +define-data-property@^1.1.4: + version "1.1.4" + resolved "https://registry.yarnpkg.com/define-data-property/-/define-data-property-1.1.4.tgz#894dc141bb7d3060ae4366f6a0107e68fbe48c5e" + integrity sha512-rBMvIzlpA8v6E+SJZoo++HAYqsLrkg7MSfIinMPFhmkorw7X+dOXVJQs+QT69zGkzMyfDnIMN2Wid1+NbL3T+A== + dependencies: + es-define-property "^1.0.0" + es-errors "^1.3.0" + gopd "^1.0.1" + define-lazy-prop@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/define-lazy-prop/-/define-lazy-prop-2.0.0.tgz#3f7ae421129bcaaac9bc74905c98a0009ec9ee7f" @@ -3199,6 +3219,11 @@ encodeurl@~1.0.2: resolved "https://registry.yarnpkg.com/encodeurl/-/encodeurl-1.0.2.tgz#ad3ff4c86ec2d029322f5a02c3a9a606c95b3f59" integrity sha512-TPJXq8JqFaVYm2CWmPvnP2Iyo4ZSM7/QKcSmuMLDObfpH5fi7RUGmd/rTDf+rut/saiDiQEeVTNgAmJEdAOx0w== +encodeurl@~2.0.0: + version "2.0.0" + resolved "https://registry.yarnpkg.com/encodeurl/-/encodeurl-2.0.0.tgz#7b8ea898077d7e409d3ac45474ea38eaf0857a58" + integrity sha512-Q0n9HRi4m6JuGIV1eFlmvJB7ZEVxu93IrMyiMsGC0lrMJMWzRgx6WGquyfQgZVb31vhGgXnfmPNNXmxnOkRBrg== + end-of-stream@^1.1.0: version "1.4.4" resolved "https://registry.yarnpkg.com/end-of-stream/-/end-of-stream-1.4.4.tgz#5ae64a5f45057baf3626ec14da0ca5e4b2431eb0" @@ -3293,6 +3318,13 @@ es-array-method-boxes-properly@^1.0.0: resolved "https://registry.yarnpkg.com/es-array-method-boxes-properly/-/es-array-method-boxes-properly-1.0.0.tgz#873f3e84418de4ee19c5be752990b2e44718d09e" integrity sha512-wd6JXUmyHmt8T5a2xreUwKcGPq6f1f+WwIJkijUqiGcJz1qqnZgP6XIK+QyIWU5lT7imeNxUll48bziG+TSYcA== +es-define-property@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/es-define-property/-/es-define-property-1.0.0.tgz#c7faefbdff8b2696cf5f46921edfb77cc4ba3845" + integrity sha512-jxayLKShrEqqzJ0eumQbVhTYQM27CfT1T35+gCgDFoL82JLsXqTJ76zv6A0YLOgEnLUMvLzsDsGIrl8NFpT2gQ== + dependencies: + get-intrinsic "^1.2.4" + es-errors@^1.0.0, es-errors@^1.2.1, es-errors@^1.3.0: version "1.3.0" resolved "https://registry.yarnpkg.com/es-errors/-/es-errors-1.3.0.tgz#05f75a25dab98e4fb1dcd5e1472c0546d5057c8f" @@ -3602,36 +3634,36 @@ expect@^27.5.1: jest-message-util "^27.5.1" express@^4.17.3: - version "4.19.2" - resolved "https://registry.yarnpkg.com/express/-/express-4.19.2.tgz#e25437827a3aa7f2a827bc8171bbbb664a356465" - integrity sha512-5T6nhjsT+EOMzuck8JjBHARTHfMht0POzlA60WV2pMD3gyXw2LZnZ+ueGdNxG+0calOJcWKbpFcuzLZ91YWq9Q== + version "4.20.0" + resolved "https://registry.yarnpkg.com/express/-/express-4.20.0.tgz#f1d08e591fcec770c07be4767af8eb9bcfd67c48" + integrity sha512-pLdae7I6QqShF5PnNTCVn4hI91Dx0Grkn2+IAsMTgMIKuQVte2dN9PeGSSAME2FR8anOhVA62QDIUaWVfEXVLw== dependencies: accepts "~1.3.8" array-flatten "1.1.1" - body-parser "1.20.2" + body-parser "1.20.3" content-disposition "0.5.4" content-type "~1.0.4" cookie "0.6.0" cookie-signature "1.0.6" debug "2.6.9" depd "2.0.0" - encodeurl "~1.0.2" + encodeurl "~2.0.0" escape-html "~1.0.3" etag "~1.8.1" finalhandler "1.2.0" fresh "0.5.2" http-errors "2.0.0" - merge-descriptors "1.0.1" + merge-descriptors "1.0.3" methods "~1.1.2" on-finished "2.4.1" parseurl "~1.3.3" - path-to-regexp "0.1.7" + path-to-regexp "0.1.10" proxy-addr "~2.0.7" qs "6.11.0" range-parser "~1.2.1" safe-buffer "5.2.1" - send "0.18.0" - serve-static "1.15.0" + send "0.19.0" + serve-static "1.16.0" setprototypeof "1.2.0" statuses "2.0.1" type-is "~1.6.18" @@ -4050,6 +4082,13 @@ has-property-descriptors@^1.0.0, has-property-descriptors@^1.0.1: dependencies: get-intrinsic "^1.2.2" +has-property-descriptors@^1.0.2: + version "1.0.2" + resolved "https://registry.yarnpkg.com/has-property-descriptors/-/has-property-descriptors-1.0.2.tgz#963ed7d071dc7bf5f084c5bfbe0d1b6222586854" + integrity sha512-55JNKuIW+vq4Ke1BjOTjM2YctQIvCT7GFzHwmfZPGo5wnrgkid0YQtnAleFSqumZm4az3n2BS+erby5ipJdgrg== + dependencies: + es-define-property "^1.0.0" + has-proto@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/has-proto/-/has-proto-1.0.1.tgz#1885c1305538958aff469fef37937c22795408e0" @@ -5303,10 +5342,10 @@ memfs@^3.4.3: dependencies: fs-monkey "^1.0.4" -merge-descriptors@1.0.1: - version "1.0.1" - resolved "https://registry.yarnpkg.com/merge-descriptors/-/merge-descriptors-1.0.1.tgz#b00aaa556dd8b44568150ec9d1b953f3f90cbb61" - integrity sha512-cCi6g3/Zr1iqQi6ySbseM1Xvooa98N0w31jzUYrXPX2xqObmFGHJ0tQ5u74H3mVh7wLouTseZyYIq39g8cNp1w== +merge-descriptors@1.0.3: + version "1.0.3" + resolved "https://registry.yarnpkg.com/merge-descriptors/-/merge-descriptors-1.0.3.tgz#d80319a65f3c7935351e5cfdac8f9318504dbed5" + integrity sha512-gaNvAS7TZ897/rVaZ0nMtAyxNyi/pdbjbAwUpFQpN70GqnVfOiXpeUUMKRBmzXaSQ8DdTX4/0ms62r2K+hE6mQ== merge-stream@^2.0.0: version "2.0.0" @@ -5711,10 +5750,10 @@ path-parse@^1.0.7: resolved "https://registry.yarnpkg.com/path-parse/-/path-parse-1.0.7.tgz#fbc114b60ca42b30d9daf5858e4bd68bbedb6735" integrity sha512-LDJzPVEEEPR+y48z93A0Ed0yXb8pAByGWo/k5YYdYgpY2/2EsOsksJrq7lOHxryrVOn1ejG6oAp8ahvOIQD8sw== -path-to-regexp@0.1.7: - version "0.1.7" - resolved "https://registry.yarnpkg.com/path-to-regexp/-/path-to-regexp-0.1.7.tgz#df604178005f522f15eb4490e7247a1bfaa67f8c" - integrity sha512-5DFkuoqlv1uYQKxy8omFBeJPQcdoE07Kv2sferDCrAq1ohOU+MSDswDIbnx3YAM60qIOnYa53wBhXW0EbMonrQ== +path-to-regexp@0.1.10: + version "0.1.10" + resolved "https://registry.yarnpkg.com/path-to-regexp/-/path-to-regexp-0.1.10.tgz#67e9108c5c0551b9e5326064387de4763c4d5f8b" + integrity sha512-7lf7qcQidTku0Gu3YDPc8DJ1q7OOucfa/BSsIwjuh56VU7katFvuM8hULfkwB3Fns/rsVF7PwPKVw1sl5KQS9w== path-type@^4.0.0: version "4.0.0" @@ -6208,6 +6247,13 @@ qs@6.11.0: dependencies: side-channel "^1.0.4" +qs@6.13.0: + version "6.13.0" + resolved "https://registry.yarnpkg.com/qs/-/qs-6.13.0.tgz#6ca3bd58439f7e245655798997787b0d88a51906" + integrity sha512-+38qI9SOr8tfZ4QmJNplMUxqjbe7LKvvZgWdExBOmd+egZTtjLB67Gu0HRX3u/XOq7UU2Nx6nsjvS16Z9uwfpg== + dependencies: + side-channel "^1.0.6" + querystringify@^2.1.1: version "2.2.0" resolved "https://registry.yarnpkg.com/querystringify/-/querystringify-2.2.0.tgz#3345941b4153cb9d082d8eee4cda2016a9aef7f6" @@ -6572,6 +6618,25 @@ send@0.18.0: range-parser "~1.2.1" statuses "2.0.1" +send@0.19.0: + version "0.19.0" + resolved "https://registry.yarnpkg.com/send/-/send-0.19.0.tgz#bbc5a388c8ea6c048967049dbeac0e4a3f09d7f8" + integrity sha512-dW41u5VfLXu8SJh5bwRmyYUbAoSB3c9uQh6L8h/KtsFREPWpbX1lrljJo186Jc4nmci/sGUZ9a0a0J2zgfq2hw== + dependencies: + debug "2.6.9" + depd "2.0.0" + destroy "1.2.0" + encodeurl "~1.0.2" + escape-html "~1.0.3" + etag "~1.8.1" + fresh "0.5.2" + http-errors "2.0.0" + mime "1.6.0" + ms "2.1.3" + on-finished "2.4.1" + range-parser "~1.2.1" + statuses "2.0.1" + serialize-javascript@^6.0.1: version "6.0.2" resolved "https://registry.yarnpkg.com/serialize-javascript/-/serialize-javascript-6.0.2.tgz#defa1e055c83bf6d59ea805d8da862254eb6a6c2" @@ -6592,10 +6657,10 @@ serve-index@^1.9.1: mime-types "~2.1.17" parseurl "~1.3.2" -serve-static@1.15.0: - version "1.15.0" - resolved "https://registry.yarnpkg.com/serve-static/-/serve-static-1.15.0.tgz#faaef08cffe0a1a62f60cad0c4e513cff0ac9540" - integrity sha512-XGuRDNjXUijsUL0vl6nSD7cwURuzEgglbOaFuZM9g3kwDXOWVTck0jLzjPzGD+TazWbboZYu52/9/XPdUgne9g== +serve-static@1.16.0: + version "1.16.0" + resolved "https://registry.yarnpkg.com/serve-static/-/serve-static-1.16.0.tgz#2bf4ed49f8af311b519c46f272bf6ac3baf38a92" + integrity sha512-pDLK8zwl2eKaYrs8mrPZBJua4hMplRWJ1tIFksVC3FtBEBnl8dxgeHtsaMS8DhS9i4fLObaon6ABoc4/hQGdPA== dependencies: encodeurl "~1.0.2" escape-html "~1.0.3" @@ -6614,6 +6679,18 @@ set-function-length@^1.2.0: gopd "^1.0.1" has-property-descriptors "^1.0.1" +set-function-length@^1.2.1: + version "1.2.2" + resolved "https://registry.yarnpkg.com/set-function-length/-/set-function-length-1.2.2.tgz#aac72314198eaed975cf77b2c3b6b880695e5449" + integrity sha512-pgRc4hJ4/sNjWCSS9AmnS40x3bNMDTknHgL5UaMBTMyJnU90EgWh1Rz+MC9eFu4BuN/UwZjKQuY/1v3rM7HMfg== + dependencies: + define-data-property "^1.1.4" + es-errors "^1.3.0" + function-bind "^1.1.2" + get-intrinsic "^1.2.4" + gopd "^1.0.1" + has-property-descriptors "^1.0.2" + set-function-name@^2.0.0: version "2.0.1" resolved "https://registry.yarnpkg.com/set-function-name/-/set-function-name-2.0.1.tgz#12ce38b7954310b9f61faa12701620a0c882793a" @@ -6667,6 +6744,16 @@ side-channel@^1.0.4: get-intrinsic "^1.2.4" object-inspect "^1.13.1" +side-channel@^1.0.6: + version "1.0.6" + resolved "https://registry.yarnpkg.com/side-channel/-/side-channel-1.0.6.tgz#abd25fb7cd24baf45466406b1096b7831c9215f2" + integrity sha512-fDW/EZ6Q9RiO8eFG8Hj+7u/oW+XrPTIChwCOM2+th2A6OblDtYYIpve9m+KvI9Z4C9qSEXlaGR6bTEYHReuglA== + dependencies: + call-bind "^1.0.7" + es-errors "^1.3.0" + get-intrinsic "^1.2.4" + object-inspect "^1.13.1" + signal-exit@^3.0.2, signal-exit@^3.0.3: version "3.0.7" resolved "https://registry.yarnpkg.com/signal-exit/-/signal-exit-3.0.7.tgz#a9a1767f8af84155114eaabd73f99273c8f59ad9" From f352f79cd82fc5c19225e2f790caf7847bd150b0 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 Sep 2024 15:43:34 +0200 Subject: [PATCH 2/5] Improve handling of state errors when polling Prior to this change, when the code expired, the user was shown the error page. This change shows the user a friendly message and a retry link that serves a fresh qr code. When a state error occurs, an invalid-request message is sent to the status page, that will then show a 'something went wrong, refresh to try again' message. See https://www.pivotaltracker.com/n/projects/1163646/stories/188205289 --- .../typescript/AuthenticationPageService.ts | 3 + .../AuthenticationPageService.test.ts | 8 +++ .../AuthenticationStatusController.php | 62 +++++++++++-------- 3 files changed, 48 insertions(+), 25 deletions(-) diff --git a/assets/typescript/AuthenticationPageService.ts b/assets/typescript/AuthenticationPageService.ts index c141b46d..daf17f7b 100644 --- a/assets/typescript/AuthenticationPageService.ts +++ b/assets/typescript/AuthenticationPageService.ts @@ -106,6 +106,9 @@ export class AuthenticationPageService { case 'challenge-expired': this.switchToChallengeHasExpired(); break; + case 'invalid-request': + this.switchToNotificationFailed(); + break; case 'needs-refresh': this.reloadPage(); break; diff --git a/assets/typescript/__test__/AuthenticationPageService.test.ts b/assets/typescript/__test__/AuthenticationPageService.test.ts index 801694a5..7d3d9db7 100644 --- a/assets/typescript/__test__/AuthenticationPageService.test.ts +++ b/assets/typescript/__test__/AuthenticationPageService.test.ts @@ -157,6 +157,14 @@ describe('AuthenticationPageService', () => { successCallback('challenge-expired'); expect(spy).toBeCalled(); }); + it('Should handle authn error (invalid request)', () => { + if (!successCallback || !errorCallback) { + throw new Error('Should have started status request'); + } + const spy = jest.spyOn(context.authenticationPageService, 'switchToNotificationFailed'); + successCallback('invalid-request'); + expect(spy).toBeCalled(); + }); it('Should handle challenge expired', () => { if (!successCallback || !errorCallback) { diff --git a/src/Controller/AuthenticationStatusController.php b/src/Controller/AuthenticationStatusController.php index 84182151..d3f52bef 100644 --- a/src/Controller/AuthenticationStatusController.php +++ b/src/Controller/AuthenticationStatusController.php @@ -46,41 +46,53 @@ public function __construct( #[Route(path: '/authentication/status', name: 'app_identity_authentication_status', methods: ['GET'])] public function __invoke(): JsonResponse { - $nameId = $this->authenticationService->getNameId(); - $sari = $this->stateHandler->getRequestId(); - $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]); - $logger->info('Request for authentication status'); - - if (!$this->authenticationService->authenticationRequired()) { - $logger->error('There is no pending authentication request from SP'); - return $this->refreshAuthenticationPage(); - } - - $isAuthenticated = $this->tiqrService->isAuthenticated(); - - if ($isAuthenticated) { - $logger->info('Send json response is authenticated'); - - return $this->refreshAuthenticationPage(); - } - - if ($this->authenticationChallengeIsExpired()) { - return $this->timeoutNeedsManualRetry(); + try { + $nameId = $this->authenticationService->getNameId(); + $sari = $this->stateHandler->getRequestId(); + $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]); + $logger->info('Request for authentication status'); + + if (!$this->authenticationService->authenticationRequired()) { + $logger->error('There is no pending authentication request from SP'); + return $this->refreshAuthenticationPage(); + } + + $isAuthenticated = $this->tiqrService->isAuthenticated(); + + if ($isAuthenticated) { + $logger->info('Send json response is authenticated'); + + return $this->refreshAuthenticationPage(); + } + + if ($this->authenticationChallengeIsExpired()) { + return $this->timeoutNeedsManualRetry(); + } + + $logger->info('Send json response is not authenticated'); + + return $this->scheduleNextPollOnAuthenticationPage(); + } catch (Exception $e) { + $this->logger->error( + sprintf( + 'An unexpected authentication error occurred. Responding with "invalid-request", '. + 'original exception message: "%s"', + $e->getMessage() + ) + ); + return $this->generateAuthenticationStatusResponse('invalid-request'); } - - $logger->info('Send json response is not authenticated'); - - return $this->scheduleNextPollOnAuthenticationPage(); } /** * Generate a status response for authentication.html. * - * The javascript in the authentication page expects one of three statuses: + * The javascript in the authentication page expects one of four statuses: * * - pending: waiting for user action, schedule next poll * - needs-refresh: refresh the page (the /authentication page will handle the success or error) * - challenge-expired: Message user challenge is expired, let the user give the option to retry. + * - invalid-request: There was a state issue, or another reason why authentication failed * * @return JsonResponse */ From 0e18752026136ffa0e73b98a0b72aee96b17282c Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 11 Sep 2024 09:37:33 +0200 Subject: [PATCH 3/5] Improve handling of qr timeout Prior to this change, the application would not check if the qr code was expired. If the user scanned an expired qr code, it would try to see if it was invalid. This resulted in errors in the logs, because it should not even try to see if the scanned qr code is valid if it is expired. This change checks if the qr code is expired before attempting to check its validity. See https://www.pivotaltracker.com/n/projects/1163646/stories/188205272 --- .../typescript/AuthenticationPageService.ts | 6 +- .../Component/RegistrationStatusComponent.ts | 6 + assets/typescript/RegistrationStateMachine.ts | 8 + .../AuthenticationPageService.test.ts | 2 +- .../__test__/RegistrationPageService.test.ts | 23 +++ ci/qa/phpstan-baseline.neon | 195 ++++++++++++++++++ ci/qa/phpstan.neon | 1 + dev/Command/AuthenticationCommand.php | 21 +- dev/Command/RegistrationCommand.php | 6 +- dev/FileLogger.php | 4 + dev/Twig/GsspExtension.php | 2 +- .../AuthenticationStatusController.php | 26 +-- src/Controller/RegistrationController.php | 7 +- src/Service/TimeoutHelper.php | 34 +++ src/Tiqr/Legacy/TiqrService.php | 62 +++++- src/Tiqr/TiqrServiceInterface.php | 4 + templates/default/registration.html.twig | 4 + tests/Unit/TimeoutHelperTest.php | 57 +++++ translations/messages.en.yml | 1 + translations/messages.nl.yml | 1 + 20 files changed, 428 insertions(+), 42 deletions(-) create mode 100644 src/Service/TimeoutHelper.php create mode 100644 tests/Unit/TimeoutHelperTest.php diff --git a/assets/typescript/AuthenticationPageService.ts b/assets/typescript/AuthenticationPageService.ts index daf17f7b..78297cc5 100644 --- a/assets/typescript/AuthenticationPageService.ts +++ b/assets/typescript/AuthenticationPageService.ts @@ -106,12 +106,12 @@ export class AuthenticationPageService { case 'challenge-expired': this.switchToChallengeHasExpired(); break; - case 'invalid-request': - this.switchToNotificationFailed(); - break; case 'needs-refresh': this.reloadPage(); break; + default: + this.switchToStatusRequestError(); + break; } }; diff --git a/assets/typescript/Component/RegistrationStatusComponent.ts b/assets/typescript/Component/RegistrationStatusComponent.ts index f23ec0ac..4d8a3a32 100644 --- a/assets/typescript/Component/RegistrationStatusComponent.ts +++ b/assets/typescript/Component/RegistrationStatusComponent.ts @@ -49,6 +49,12 @@ export class RegistrationStatusComponent { public showUnknownErrorHappened() { this.show('div.status.error'); } + /** + * Unknown error happened. Please try again by refreshing your browser. + */ + public showTimeoutHappened() { + this.show('div.status.timeout'); + } private hideAll() { jQuery('.status-container >').hide(); diff --git a/assets/typescript/RegistrationStateMachine.ts b/assets/typescript/RegistrationStateMachine.ts index 2f32cd69..9e170b59 100644 --- a/assets/typescript/RegistrationStateMachine.ts +++ b/assets/typescript/RegistrationStateMachine.ts @@ -12,6 +12,8 @@ export class RegistrationStateMachine { * Client-side only status. */ public static readonly ERROR = 'ERROR'; + public static readonly TIMEOUT = 'TIMEOUT'; + private previousStatus = RegistrationStateMachine.IDLE; constructor(private statusPollingService: StatusPollService, @@ -62,6 +64,12 @@ export class RegistrationStateMachine { this.qrCode.hide(); document.location.replace(this.finalizedUrl); break; + case RegistrationStateMachine.TIMEOUT: + this.qrCode.hide(); + this.statusUi.showTimeoutHappened(); + this.statusPollingService.stop(); + this.previousStatus = RegistrationStateMachine.ERROR; + break; default: this.unknownError(); return; diff --git a/assets/typescript/__test__/AuthenticationPageService.test.ts b/assets/typescript/__test__/AuthenticationPageService.test.ts index 7d3d9db7..245c3ab2 100644 --- a/assets/typescript/__test__/AuthenticationPageService.test.ts +++ b/assets/typescript/__test__/AuthenticationPageService.test.ts @@ -161,7 +161,7 @@ describe('AuthenticationPageService', () => { if (!successCallback || !errorCallback) { throw new Error('Should have started status request'); } - const spy = jest.spyOn(context.authenticationPageService, 'switchToNotificationFailed'); + const spy = jest.spyOn(context.authenticationPageService, 'switchToStatusRequestError'); successCallback('invalid-request'); expect(spy).toBeCalled(); }); diff --git a/assets/typescript/__test__/RegistrationPageService.test.ts b/assets/typescript/__test__/RegistrationPageService.test.ts index a010c881..4866aa2b 100644 --- a/assets/typescript/__test__/RegistrationPageService.test.ts +++ b/assets/typescript/__test__/RegistrationPageService.test.ts @@ -141,6 +141,28 @@ describe('RegistrationPageService', () => { }); }); + describe('When timeout', () => { + beforeEach(() => { + context.authenticationPageService.start(); + if (!statusCallback || !errorCallback) { + throw new Error('Should have started status request'); + } + statusCallback(RegistrationStateMachine.TIMEOUT); + }); + + it('The qr code should be hidden', () => { + expect(context.qrComponent.isVisible()).toBeFalsy(); + }); + + it('Polling should be disabled', () => { + expect(context.pollingService.enabled).toBeFalsy(); + }); + + it('Show finalized', () => { + expect(context.statusUi.showTimeoutHappened).toBeCalled(); + }); + }); + describe('When connection error occurred', () => { beforeEach(() => { context.authenticationPageService.start(); @@ -227,6 +249,7 @@ describe('RegistrationPageService', () => { showAccountActivationHelp:jest.fn(), showOneMomentPlease: jest.fn(), showFinalized: jest.fn(), + showTimeoutHappened: jest.fn(), showUnknownErrorHappened: jest.fn(), }; diff --git a/ci/qa/phpstan-baseline.neon b/ci/qa/phpstan-baseline.neon index 67a87265..f01b6f4f 100644 --- a/ci/qa/phpstan-baseline.neon +++ b/ci/qa/phpstan-baseline.neon @@ -1,5 +1,200 @@ parameters: ignoreErrors: + - + message: "#^Cannot access offset 'authenticationUrl' on mixed\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Cannot access offset 'id' on mixed\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Cannot access offset 'identities' on mixed\\.$#" + count: 2 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Cannot access offset 'ocraSuite' on mixed\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Cannot access offset 'secret' on mixed\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Cannot access offset int\\|string\\|false on mixed\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Cannot access offset string on mixed\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Parameter \\#1 \\$array of function array_keys expects array, mixed given\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Parameter \\#1 \\$file of method Surfnet\\\\Tiqr\\\\Dev\\\\Command\\\\AuthenticationCommand\\:\\:readAuthenticationLinkFromFile\\(\\) expects string, mixed given\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Parameter \\#1 \\$json of function json_decode expects string, string\\|false given\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Parameter \\#1 \\$ocraSuite of static method OCRA\\:\\:generateOCRA\\(\\) expects string, mixed given\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Parameter \\#1 \\$uri of method GuzzleHttp\\\\Client\\:\\:post\\(\\) expects Psr\\\\Http\\\\Message\\\\UriInterface\\|string, mixed given\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Parameter \\#2 \\$key of static method OCRA\\:\\:generateOCRA\\(\\) expects string, mixed given\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Parameter \\#2 \\.\\.\\.\\$values of function sprintf expects bool\\|float\\|int\\|string\\|null, mixed given\\.$#" + count: 1 + path: ../../dev/Command/AuthenticationCommand.php + + - + message: "#^Cannot access offset 'authenticationUrl' on mixed\\.$#" + count: 1 + path: ../../dev/Command/RegistrationCommand.php + + - + message: "#^Cannot access offset 'identities' on mixed\\.$#" + count: 2 + path: ../../dev/Command/RegistrationCommand.php + + - + message: "#^Cannot access offset 'ocraSuite' on mixed\\.$#" + count: 1 + path: ../../dev/Command/RegistrationCommand.php + + - + message: "#^Cannot access offset mixed on mixed\\.$#" + count: 5 + path: ../../dev/Command/RegistrationCommand.php + + - + message: "#^Cannot access property \\$service on mixed\\.$#" + count: 2 + path: ../../dev/Command/RegistrationCommand.php + + - + message: "#^Method Surfnet\\\\Tiqr\\\\Dev\\\\Command\\\\RegistrationCommand\\:\\:storeIdentity\\(\\) has parameter \\$metadata with no type specified\\.$#" + count: 1 + path: ../../dev/Command/RegistrationCommand.php + + - + message: "#^Method Surfnet\\\\Tiqr\\\\Dev\\\\Command\\\\RegistrationCommand\\:\\:storeIdentity\\(\\) has parameter \\$secret with no type specified\\.$#" + count: 1 + path: ../../dev/Command/RegistrationCommand.php + + - + message: "#^Parameter \\#1 \\$file of method Surfnet\\\\Tiqr\\\\Dev\\\\Command\\\\RegistrationCommand\\:\\:readRegistrationUrlFromFile\\(\\) expects string, mixed given\\.$#" + count: 1 + path: ../../dev/Command/RegistrationCommand.php + + - + message: "#^Parameter \\#1 \\$json of function json_decode expects string, string\\|false given\\.$#" + count: 1 + path: ../../dev/Command/RegistrationCommand.php + + - + message: "#^Call to an undefined method SAML2\\\\Message\\:\\:getStatus\\(\\)\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Cannot call method getValue\\(\\) on SAML2\\\\XML\\\\saml\\\\Issuer\\|null\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Cannot cast mixed to string\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Method Surfnet\\\\Tiqr\\\\Dev\\\\Controller\\\\SPController\\:\\:signRequestQuery\\(\\) has parameter \\$queryParams with no value type specified in iterable type array\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^PHPDoc tag @var has invalid value \\(\\$securityKey\\)\\: Unexpected token \"\\$securityKey\", expected type at offset 10$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Parameter \\#1 \\$key of method SAML2\\\\Certificate\\\\PrivateKeyLoader\\:\\:loadPrivateKey\\(\\) expects SAML2\\\\Configuration\\\\PrivateKey, mixed given\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Parameter \\#1 \\$nameId of method Surfnet\\\\SamlBundle\\\\SAML2\\\\AuthnRequest\\:\\:setSubject\\(\\) expects string, mixed given\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Parameter \\#1 \\$source of method DOMDocument\\:\\:loadXML\\(\\) expects string, bool\\|string given\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Parameter \\#1 \\$string of function base64_decode expects string, bool\\|float\\|int\\|string\\|null given\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Parameter \\#1 \\$string of function base64_encode expects string, string\\|false given\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Parameter \\#1 \\$xml of static method SAML2\\\\Message\\:\\:fromXML\\(\\) expects DOMElement, DOMElement\\|null given\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Parameter \\#2 \\$values of method Symfony\\\\Component\\\\HttpFoundation\\\\ResponseHeaderBag\\:\\:set\\(\\) expects array\\\\|string\\|null, mixed given\\.$#" + count: 1 + path: ../../dev/Controller/SPController.php + + - + message: "#^Method Surfnet\\\\Tiqr\\\\Dev\\\\FileLogger\\:\\:getLogs\\(\\) return type has no value type specified in iterable type array\\.$#" + count: 1 + path: ../../dev/FileLogger.php + + - + message: "#^Method Surfnet\\\\Tiqr\\\\Dev\\\\FileLogger\\:\\:log\\(\\) has parameter \\$context with no value type specified in iterable type array\\.$#" + count: 1 + path: ../../dev/FileLogger.php + + - + message: "#^Parameter \\#1 \\$record of method League\\\\Csv\\\\Writer\\:\\:insertOne\\(\\) expects array\\, array\\ given\\.$#" + count: 1 + path: ../../dev/FileLogger.php + + - + message: "#^Parameter \\#1 \\$stream of static method League\\\\Csv\\\\AbstractCsv\\:\\:createFromStream\\(\\) expects resource, resource\\|false given\\.$#" + count: 1 + path: ../../dev/FileLogger.php + - message: "#^Parameter \\#3 \\$response of method Surfnet\\\\Tiqr\\\\Tiqr\\\\AuthenticationRateLimitServiceInterface\\:\\:authenticate\\(\\) expects string, mixed given\\.$#" count: 1 diff --git a/ci/qa/phpstan.neon b/ci/qa/phpstan.neon index 6ddf4542..784dc393 100644 --- a/ci/qa/phpstan.neon +++ b/ci/qa/phpstan.neon @@ -5,3 +5,4 @@ parameters: level: 9 paths: - ../../src + - ../../dev diff --git a/dev/Command/AuthenticationCommand.php b/dev/Command/AuthenticationCommand.php index aaf89a22..a981d8e7 100644 --- a/dev/Command/AuthenticationCommand.php +++ b/dev/Command/AuthenticationCommand.php @@ -88,7 +88,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int [$serviceId, $session, $challenge, $sp, $version] = explode('/', $authn); $userId = null; - if (strpos($serviceId, '@') >= 0) { + if (str_contains($serviceId, '@')) { [$userId, $serviceId] = explode('@', $serviceId); } @@ -101,7 +101,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int 'sp' => $sp, 'version' => $version, 'userId' => $userId, - ], JSON_PRETTY_PRINT)), + ], JSON_PRETTY_PRINT | JSON_THROW_ON_ERROR)), ]); $service = $this->getService($serviceId); @@ -126,7 +126,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $service['id'] = $serviceId; $output->writeln([ 'Generate OCRA for service:', - $this->decorateResult(json_encode($service, JSON_PRETTY_PRINT)), + $this->decorateResult(json_encode($service, JSON_PRETTY_PRINT | JSON_THROW_ON_ERROR)), ]); $response = OCRA::generateOCRA($ocraSuite, $secret, '', $challenge, '', $session, ''); @@ -143,8 +143,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int 'sessionKey' => $session, 'userId' => $userId, 'response' => $response, - 'notificationType' => $input->getOption('notificationType', ''), - 'notificationAddress' => $input->getOption('notificationAddress', ''), + 'notificationType' => $input->getOption('notificationType'), + 'notificationAddress' => $input->getOption('notificationAddress'), ]; $output->writeln([ @@ -152,7 +152,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int 'Send authentication data to "%s" with body:', $authenticationUrl ), - $this->decorateResult(json_encode($authenticationBody, JSON_PRETTY_PRINT)), + $this->decorateResult(json_encode($authenticationBody, JSON_PRETTY_PRINT | JSON_THROW_ON_ERROR)), ]); $result = $this->client->post($authenticationUrl, [ @@ -166,7 +166,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int return Command::SUCCESS; } - protected function decorateResult($text): string + protected function decorateResult(string $text): string { return "$text"; } @@ -174,14 +174,19 @@ protected function decorateResult($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)) { + throw new RuntimeException('Unable to read a link from the QR code'); + } + $output->writeln([ 'Registration link result: ', $this->decorateResult($link), ]); - return $link->toString(); + return $link; } /** diff --git a/dev/Command/RegistrationCommand.php b/dev/Command/RegistrationCommand.php index b6b1516e..47df043b 100644 --- a/dev/Command/RegistrationCommand.php +++ b/dev/Command/RegistrationCommand.php @@ -78,7 +78,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $metadata = json_decode($metadataBody); $output->writeln([ 'Metadata result:', - $this->decorateResult(json_encode($metadata, JSON_PRETTY_PRINT)), + $this->decorateResult(json_encode($metadata, JSON_PRETTY_PRINT | JSON_THROW_ON_ERROR)), ]); if ($metadata === false) { $output->writeln('Metadata has expired'); @@ -99,7 +99,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int 'Send registration data to enrollmentUrl "%s" with body:', $metadata->service->enrollmentUrl ), - $this->decorateResult(json_encode($registrationBody, JSON_PRETTY_PRINT)), + $this->decorateResult(json_encode($registrationBody, JSON_PRETTY_PRINT | JSON_THROW_ON_ERROR)), ]); $result = $this->client->post($metadata->service->enrollmentUrl, ['form_params' => $registrationBody]); @@ -121,7 +121,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int return Command::SUCCESS; } - protected function decorateResult($text): string + protected function decorateResult(string $text): string { return "$text"; } diff --git a/dev/FileLogger.php b/dev/FileLogger.php index 001702de..372a4a04 100644 --- a/dev/FileLogger.php +++ b/dev/FileLogger.php @@ -36,6 +36,10 @@ public function log($level, $message, array $context = []): void return; } $file = fopen($this->getCSVFile(), 'ab+'); + if (!$file) { + return; + } + $csv = Writer::createFromStream($file); $csv->setDelimiter(';'); $csv->insertOne([$level, $message, json_encode($context)]); diff --git a/dev/Twig/GsspExtension.php b/dev/Twig/GsspExtension.php index d4041163..0b2d7ac2 100644 --- a/dev/Twig/GsspExtension.php +++ b/dev/Twig/GsspExtension.php @@ -38,7 +38,7 @@ public function generateDemoSPUrl(): string { return sprintf( 'https://pieter.aai.surfnet.nl/simplesamlphp/sp.php?idp=%s', - urlencode($this->hostedEntities->getIdentityProvider()->getEntityId()) + urlencode($this->hostedEntities->getIdentityProvider()?->getEntityId() ?? '') ); } } diff --git a/src/Controller/AuthenticationStatusController.php b/src/Controller/AuthenticationStatusController.php index d3f52bef..ce5e71c3 100644 --- a/src/Controller/AuthenticationStatusController.php +++ b/src/Controller/AuthenticationStatusController.php @@ -57,6 +57,7 @@ public function __invoke(): JsonResponse return $this->refreshAuthenticationPage(); } + $isAuthenticated = $this->tiqrService->isAuthenticated(); if ($isAuthenticated) { @@ -65,7 +66,8 @@ public function __invoke(): JsonResponse return $this->refreshAuthenticationPage(); } - if ($this->authenticationChallengeIsExpired()) { + if ($this->tiqrService->isAuthenticationTimedOut()) { + $this->logger->info('The authentication timed out'); return $this->timeoutNeedsManualRetry(); } @@ -111,28 +113,6 @@ private function refreshAuthenticationPage(): JsonResponse return $this->generateAuthenticationStatusResponse('needs-refresh'); } - /** - * Check if the authentication challenge is expired. - * - * If the challenge is expired, the page should be refreshed so a new - * challenge and QR code is generated. - * - * @return bool - */ - private function authenticationChallengeIsExpired(): bool - { - // The use of authenticationUrl() here is a hack, because it depends on an implementation detail - // of this function. - // Effectively this does a $this->_stateStorage->getValue(self::PREFIX_CHALLENGE . $sessionKey); - // To check that the session key still exists in the Tiqr_Service's state storage - try { - $this->tiqrService->authenticationUrl(); - } catch (Exception) { - return true; - } - return false; - } - /** * Generate a response for authentication.html: Ask the user to retry. * diff --git a/src/Controller/RegistrationController.php b/src/Controller/RegistrationController.php index 80468c3c..5ba1c942 100644 --- a/src/Controller/RegistrationController.php +++ b/src/Controller/RegistrationController.php @@ -24,6 +24,7 @@ use Surfnet\GsspBundle\Service\RegistrationService; use Surfnet\GsspBundle\Service\StateHandlerInterface; use Surfnet\Tiqr\Exception\NoActiveAuthenrequestException; +use Surfnet\Tiqr\Tiqr\Legacy\TiqrService; use Surfnet\Tiqr\Tiqr\TiqrServiceInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\Request; @@ -96,6 +97,7 @@ public function registration(Request $request): Response public function registrationStatus() : Response { $this->logger->info('Request for registration status'); + // Do we have a valid GSSP registration AuthnRequest in this session? if (!$this->registrationService->registrationRequired()) { $this->logger->error('There is no pending registration request'); @@ -103,7 +105,10 @@ public function registrationStatus() : Response return new Response('No registration required', Response::HTTP_BAD_REQUEST); } - // TODO: Check whether enrollment is expired here? + if ($this->tiqrService->isEnrollmentTimedOut()) { + $this->logger->info('The registration timed out'); + return new Response(TiqrService::ENROLLMENT_TIMEOUT_STATUS); + } $status = $this->tiqrService->getEnrollmentStatus(); $this->logger->info(sprintf('Send json response status "%s"', $status)); diff --git a/src/Service/TimeoutHelper.php b/src/Service/TimeoutHelper.php new file mode 100644 index 00000000..cbe79f20 --- /dev/null +++ b/src/Service/TimeoutHelper.php @@ -0,0 +1,34 @@ += $timeoutInSeconds; + } +} diff --git a/src/Tiqr/Legacy/TiqrService.php b/src/Tiqr/Legacy/TiqrService.php index a058f728..3e77d905 100644 --- a/src/Tiqr/Legacy/TiqrService.php +++ b/src/Tiqr/Legacy/TiqrService.php @@ -24,6 +24,7 @@ use Psr\Log\LoggerInterface; use Surfnet\Tiqr\Exception\TiqrServerRuntimeException; use Surfnet\Tiqr\HealthCheck\HealthCheckResultDto; +use Surfnet\Tiqr\Service\TimeoutHelper; use Surfnet\Tiqr\Tiqr\Response\AuthenticationErrorResponse; use Surfnet\Tiqr\Tiqr\Response\AuthenticationResponse; use Surfnet\Tiqr\Tiqr\Response\RejectedAuthenticationResponse; @@ -37,9 +38,11 @@ use Tiqr_Service; use Tiqr_StateStorage_StateStorageInterface; + /** * Wrapper around the legacy Tiqr service. * + * @SuppressWarnings(PHPMD.ExcessiveClassComplexity) * @SuppressWarnings(PHPMD.TooManyPublicMethods) * @SuppressWarnings(PHPMD.CouplingBetweenObjects) * It's Legacy. @@ -47,6 +50,26 @@ final class TiqrService implements TiqrServiceInterface { public const ENROLL_KEYS_SESSION_NAME = 'enrollment-session-keys'; + + public const ENROLLMENT_TIMEOUT_STATUS = 'TIMEOUT'; + + /** + * Unix timestamp when the enrollment started + */ + private const ENROLLMENT_STARTED_AT = 'enrollment-started-at'; + + /** + * Unix timestamp when the authentication started + */ + private const AUTHENTICATION_STARTED_AT = 'authentication-started-at'; + + /** + * The time (in seconds) that is extracted from the timeout + * to prevent timeout issues right before the hard timeout + * time is reached. + */ + private const TIMEOUT_OFFSET = 2; + private SessionInterface $session; public function __construct( @@ -94,13 +117,13 @@ public function getEnrollmentSecret(string $enrollmentKey, string $sari): string public function generateEnrollmentKey(string $sari): string { $this->initSession(); - // We use a randomly generated user ID $this->logger->debug('Generating tiqr userId'); $userId = $this->generateId(); $this->logger->debug('Storing the userId=' . $userId . ' to session state'); $this->session->set('userId', $userId); + $this->recordStartTime(self::ENROLLMENT_STARTED_AT); // The session ID is used to link the tiqr library's enrollment session to the user's browser session $sessionId = $this->session->getId(); $this->logger->debug('Clearing the previous enrollment state(s)'); @@ -190,7 +213,7 @@ public function startAuthentication(string $userId, string $sari): string $this->session->set('sessionKey', $sessionKey); $this->setSariForSessionIdentifier($sessionKey, $sari); - + $this->recordStartTime(self::AUTHENTICATION_STARTED_AT); return $this->tiqrService->generateAuthURL($sessionKey); } catch (Exception $e) { // Catch errors from the tiqr-server and up-cycle them to exceptions that are meaningful to our domain @@ -451,4 +474,39 @@ protected function getEnrollmentTimeout(): int { return Tiqr_Service::ENROLLMENT_EXPIRE; } + + public function isAuthenticationTimedOut(): bool + { + $this->initSession(); + $this->logger->debug('Checking if authentication timeout is reached'); + $startedAt = $this->session->get(self::AUTHENTICATION_STARTED_AT); + assert(is_int($startedAt)); + return TimeoutHelper::isTimedOut( + time(), + $startedAt, + $this->getAuthenticationTimeout(), + self::TIMEOUT_OFFSET + ); + } + + public function isEnrollmentTimedOut(): bool + { + $this->initSession(); + $this->logger->debug('Checking if enrollment timeout is reached'); + $startedAt = $this->session->get(self::ENROLLMENT_STARTED_AT); + assert(is_int($startedAt)); + return TimeoutHelper::isTimedOut( + time(), + $startedAt, + $this->getEnrollmentTimeout(), + self::TIMEOUT_OFFSET + ); + } + + private function recordStartTime(string $sessionFieldIdentifier): void + { + $startedAt = time(); + $this->logger->debug(sprintf('Storing the %s = %s', $sessionFieldIdentifier, $startedAt)); + $this->session->set($sessionFieldIdentifier, $startedAt); + } } diff --git a/src/Tiqr/TiqrServiceInterface.php b/src/Tiqr/TiqrServiceInterface.php index 2ae689f6..d8bfd54c 100644 --- a/src/Tiqr/TiqrServiceInterface.php +++ b/src/Tiqr/TiqrServiceInterface.php @@ -242,4 +242,8 @@ public function sendNotification(string $notificationType, string $notificationA public function getSariForSessionIdentifier(string $identifier): string; public function stateStorageHealthCheck(): HealthCheckResultDto; + + public function isEnrollmentTimedOut(): bool; + + public function isAuthenticationTimedOut(): bool; } diff --git a/templates/default/registration.html.twig b/templates/default/registration.html.twig index 021ad15a..4bef79de 100644 --- a/templates/default/registration.html.twig +++ b/templates/default/registration.html.twig @@ -45,6 +45,10 @@ {{ 'enrol.status.error' | trans }} {{ 'enrol.retry' | trans }}. +
+ {{ 'enrol.status.timeout' | trans }} + {{ 'enrol.retry' | trans }}. +
diff --git a/tests/Unit/TimeoutHelperTest.php b/tests/Unit/TimeoutHelperTest.php new file mode 100644 index 00000000..feb22e86 --- /dev/null +++ b/tests/Unit/TimeoutHelperTest.php @@ -0,0 +1,57 @@ + [true, 1000, 0, 300, 2], // 100 seconds ago the user started the clock + 'just over time' => [true, 303, 0, 300, 2], // 5 seconds over timeout time + 'timeout due to offset - 1' => [true, 298, 0, 300, 2], // the offset is reached + 'timeout due to offset - 2' => [true, 299, 0, 300, 2], // the offset is reached + // In time expectations + 'very much in time' => [false, 0, 0, 300, 2], // 298 seconds to go before reaching timeout + 'just in time' => [false, 297, 0, 300, 2], // last second before reaching timeout + ]; + } +} diff --git a/translations/messages.en.yml b/translations/messages.en.yml index 25d602b6..e53609f4 100644 --- a/translations/messages.en.yml +++ b/translations/messages.en.yml @@ -51,6 +51,7 @@ enrol: processed: One moment please... finalized: Your account is ready for use. error: Unknown error occurred. Please try again by refreshing your browser. + timeout: Registration timeout. Try again or refresh this page. download: Download tiqr for iOS/Android cancel: Cancel diff --git a/translations/messages.nl.yml b/translations/messages.nl.yml index e1d10312..28f62d57 100644 --- a/translations/messages.nl.yml +++ b/translations/messages.nl.yml @@ -55,6 +55,7 @@ enrol: processed: Een ogenblik geduld a.u.b. finalized: Je account is gereed voor gebruik. error: Er is een onbekende fout opgetreden. Probeer het opnieuw door uw browser te vernieuwen. + timeout: Registratie timeout. Ververs de pagina om het nogmaals te proberen. download: Download de tiqr-app voor iOS/Android cancel: Annuleren From 76473dba531ee272bf41a2070e895984092925ec Mon Sep 17 00:00:00 2001 From: Martijn Harte Date: Thu, 12 Sep 2024 08:59:30 +0200 Subject: [PATCH 4/5] Improve logging on session related errors Prior to this change there we weren't able to keep track of sessions that got lost. This change allows us to see every time a session is created and distinguish them by their correlation id. Log an error on a route that requires an active session when there is none Prior to this change all routes were able to called, even though the user might not have had an active session This change will start logging errors when the session wasn't found, or is in an unexpected state Prior to this change session information got lost. We had no way of tracking down what happened to user sessions in the logs. This change logs whether a session existed and if it's in a valid state. Log information is enriched with a correlation id to be able to distinguish them. See https://www.pivotaltracker.com/n/projects/1163646/stories/188205214 --- config/openconext/parameters.yaml.dist | 3 + config/packages/framework.yaml | 4 +- config/services.yaml | 2 + src/Attribute/RequiresActiveSession.php | 28 ++ .../AuthenticationNotificationController.php | 2 + src/Controller/AuthenticationQrController.php | 2 + .../AuthenticationStatusController.php | 3 +- src/Controller/RegistrationController.php | 5 +- ...RequiresActiveSessionAttributeListener.php | 96 +++++++ src/EventSubscriber/SessionStateListener.php | 90 +++++++ src/Service/SessionCorrelationIdService.php | 61 +++++ src/Session/LoggingSessionFactory.php | 70 +++++ src/Tiqr/Legacy/TiqrService.php | 1 - ...iresActiveSessionAttributeListenerTest.php | 245 ++++++++++++++++++ .../SessionStateListenerTest.php | 221 ++++++++++++++++ .../SessionCorrelationIdServiceTest.php | 74 ++++++ .../Session/LoggingSessionFactoryTest.php | 56 ++++ 17 files changed, 958 insertions(+), 5 deletions(-) create mode 100644 src/Attribute/RequiresActiveSession.php create mode 100644 src/EventSubscriber/RequiresActiveSessionAttributeListener.php create mode 100644 src/EventSubscriber/SessionStateListener.php create mode 100644 src/Service/SessionCorrelationIdService.php create mode 100644 src/Session/LoggingSessionFactory.php create mode 100644 tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php create mode 100644 tests/Unit/EventSubscriber/SessionStateListenerTest.php create mode 100644 tests/Unit/Service/SessionCorrelationIdServiceTest.php create mode 100644 tests/Unit/Session/LoggingSessionFactoryTest.php diff --git a/config/openconext/parameters.yaml.dist b/config/openconext/parameters.yaml.dist index 5705b9f8..555177c9 100644 --- a/config/openconext/parameters.yaml.dist +++ b/config/openconext/parameters.yaml.dist @@ -7,6 +7,9 @@ parameters: # A secret key that's used to generate certain security-related tokens app_secret: ThisTokenIsNotSoSecretChangeIt + # A secret salt used to hash the correlationId for logging based on the session_id + correlation_id_salt: 'changeMeToAtLeast16CharsOfRandomString' + # All locales supported by the application default_locale: en_GB locales: diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index 16aae45d..f04eec90 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -14,8 +14,10 @@ framework: # Remove or comment this section to explicitly disable session support. session: handler_id: null - cookie_secure: auto + cookie_secure: true cookie_samesite: none + name: sess_tiqr + cookie_httponly: true router: strict_requirements: null utf8: true diff --git a/config/services.yaml b/config/services.yaml index d1b7ac86..8e2963ef 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -19,6 +19,8 @@ services: $locales: '%locales%' $tiqrConfiguration: '%tiqr_library_options%' $appSecret: '%app_secret%' + $sessionOptions: '%session.storage.options%' + $correlationIdSalt: '%correlation_id_salt%' # makes classes in src/ available to be used as services # this creates a service per class whose id is the fully-qualified class name diff --git a/src/Attribute/RequiresActiveSession.php b/src/Attribute/RequiresActiveSession.php new file mode 100644 index 00000000..98d87bfb --- /dev/null +++ b/src/Attribute/RequiresActiveSession.php @@ -0,0 +1,28 @@ +authenticationService->getNameId(); diff --git a/src/Controller/AuthenticationQrController.php b/src/Controller/AuthenticationQrController.php index ed3bd456..b7260e03 100644 --- a/src/Controller/AuthenticationQrController.php +++ b/src/Controller/AuthenticationQrController.php @@ -24,6 +24,7 @@ use Psr\Log\LoggerInterface; use Surfnet\GsspBundle\Service\AuthenticationService; use Surfnet\GsspBundle\Service\StateHandlerInterface; +use Surfnet\Tiqr\Attribute\RequiresActiveSession; use Surfnet\Tiqr\Tiqr\TiqrServiceInterface; use Surfnet\Tiqr\WithContextLogger; use Symfony\Component\HttpFoundation\Response; @@ -44,6 +45,7 @@ public function __construct( * @throws InvalidArgumentException */ #[Route(path: '/authentication/qr', name: 'app_identity_authentication_qr', methods: ['GET'])] + #[RequiresActiveSession] public function __invoke(): Response { $nameId = $this->authenticationService->getNameId(); diff --git a/src/Controller/AuthenticationStatusController.php b/src/Controller/AuthenticationStatusController.php index ce5e71c3..5a130c9c 100644 --- a/src/Controller/AuthenticationStatusController.php +++ b/src/Controller/AuthenticationStatusController.php @@ -25,6 +25,7 @@ use Psr\Log\LoggerInterface; use Surfnet\GsspBundle\Service\AuthenticationService; use Surfnet\GsspBundle\Service\StateHandlerInterface; +use Surfnet\Tiqr\Attribute\RequiresActiveSession; use Surfnet\Tiqr\Tiqr\TiqrServiceInterface; use Surfnet\Tiqr\WithContextLogger; use Symfony\Component\HttpFoundation\JsonResponse; @@ -44,6 +45,7 @@ public function __construct( * @throws InvalidArgumentException */ #[Route(path: '/authentication/status', name: 'app_identity_authentication_status', methods: ['GET'])] + #[RequiresActiveSession] public function __invoke(): JsonResponse { try { @@ -57,7 +59,6 @@ public function __invoke(): JsonResponse return $this->refreshAuthenticationPage(); } - $isAuthenticated = $this->tiqrService->isAuthenticated(); if ($isAuthenticated) { diff --git a/src/Controller/RegistrationController.php b/src/Controller/RegistrationController.php index 5ba1c942..67f0a3ba 100644 --- a/src/Controller/RegistrationController.php +++ b/src/Controller/RegistrationController.php @@ -23,6 +23,7 @@ use Psr\Log\LoggerInterface; use Surfnet\GsspBundle\Service\RegistrationService; use Surfnet\GsspBundle\Service\StateHandlerInterface; +use Surfnet\Tiqr\Attribute\RequiresActiveSession; use Surfnet\Tiqr\Exception\NoActiveAuthenrequestException; use Surfnet\Tiqr\Tiqr\Legacy\TiqrService; use Surfnet\Tiqr\Tiqr\TiqrServiceInterface; @@ -90,9 +91,8 @@ public function registration(Request $request): Response * * * @throws \InvalidArgumentException - * - * Requires session cookie to be set to a valid session. */ + #[RequiresActiveSession] #[Route(path: '/registration/status', name: 'app_identity_registration_status', methods: ['GET'])] public function registrationStatus() : Response { @@ -123,6 +123,7 @@ public function registrationStatus() : Response * * @throws \InvalidArgumentException */ + #[RequiresActiveSession] #[Route(path: '/registration/qr/{enrollmentKey}', name: 'app_identity_registration_qr', methods: ['GET'])] public function registrationQr(Request $request, string $enrollmentKey): Response { diff --git a/src/EventSubscriber/RequiresActiveSessionAttributeListener.php b/src/EventSubscriber/RequiresActiveSessionAttributeListener.php new file mode 100644 index 00000000..4ddbbd78 --- /dev/null +++ b/src/EventSubscriber/RequiresActiveSessionAttributeListener.php @@ -0,0 +1,96 @@ + $sessionOptions + */ + public function __construct( + private LoggerInterface $logger, + private SessionCorrelationIdService $sessionCorrelationIdService, + private array $sessionOptions, + ) { + if (!array_key_exists('name', $this->sessionOptions)) { + throw new RuntimeException( + 'The session name (PHP session cookie identifier) could not be found in the session configuration.' + ); + } + $this->sessionName = $this->sessionOptions['name']; + } + + public function onKernelControllerArguments(ControllerArgumentsEvent $event): void + { + if (!is_array($event->getAttributes()[RequiresActiveSession::class] ?? null)) { + return; + } + + $logger = WithContextLogger::from($this->logger, [ + 'correlationId' => $this->sessionCorrelationIdService->generateCorrelationId() ?? '', + 'route' => $event->getRequest()->getRequestUri(), + ]); + + try { + $sessionId = $event->getRequest()->getSession()->getId(); + $sessionCookieId = $event->getRequest()->cookies->get($this->sessionName); + + if (!$sessionCookieId) { + $logger->error('Route requires active session. Active session wasn\'t found. No session cookie was set.'); + + throw new AccessDeniedException(); + } + + if ($sessionId !== $sessionCookieId) { + $logger->error('Route requires active session. Session does not match session cookie.'); + + throw new AccessDeniedException(); + } + } catch (SessionNotFoundException) { + $logger->error('Route requires active session. Active session wasn\'t found.'); + + throw new AccessDeniedException(); + } + } + + public static function getSubscribedEvents(): array + { + return [KernelEvents::CONTROLLER_ARGUMENTS => ['onKernelControllerArguments', 20]]; + } +} diff --git a/src/EventSubscriber/SessionStateListener.php b/src/EventSubscriber/SessionStateListener.php new file mode 100644 index 00000000..33179505 --- /dev/null +++ b/src/EventSubscriber/SessionStateListener.php @@ -0,0 +1,90 @@ + $sessionOptions + */ + public function __construct( + private LoggerInterface $logger, + private SessionCorrelationIdService $sessionCorrelationIdService, + private array $sessionOptions, + ) { + if (!array_key_exists('name', $this->sessionOptions)) { + throw new RuntimeException( + 'The session name (PHP session cookie identifier) could not be found in the session configuration.' + ); + } + $this->sessionName = $this->sessionOptions['name']; + } + + public function onKernelRequest(RequestEvent $event): void + { + $logger = WithContextLogger::from($this->logger, [ + 'correlationId' => $this->sessionCorrelationIdService->generateCorrelationId() ?? '', + 'route' => $event->getRequest()->getRequestUri(), + ]); + + $sessionCookieId = $event->getRequest()->cookies->get($this->sessionName); + if ($sessionCookieId === null) { + $logger->info('User made a request without a session cookie.'); + return; + } + + $logger->info('User made a request with a session cookie.'); + + try { + $sessionId = $event->getRequest()->getSession()->getId(); + $logger->info('User has a session.'); + + if ($sessionId !== $sessionCookieId) { + $logger->error('The session cookie does not match the session id.'); + return; + } + } catch (SessionNotFoundException) { + $logger->info('Session not found.'); + return; + } + + $logger->info('User session matches the session cookie.'); + } + + public static function getSubscribedEvents(): array + { + return [KernelEvents::REQUEST => ['onKernelRequest', 20]]; + } +} diff --git a/src/Service/SessionCorrelationIdService.php b/src/Service/SessionCorrelationIdService.php new file mode 100644 index 00000000..46f529c2 --- /dev/null +++ b/src/Service/SessionCorrelationIdService.php @@ -0,0 +1,61 @@ + $sessionOptions + */ + public function __construct( + private RequestStack $requestStack, + array $sessionOptions, + ?string $correlationIdSalt = null, + ) { + if (!array_key_exists('name', $sessionOptions)) { + throw new RuntimeException( + 'The session name (PHP session cookie identifier) could not be found in the session configuration.' + ); + } + $this->correlationIdSalt = is_null($correlationIdSalt) || strlen($correlationIdSalt) < 16 ? null : $correlationIdSalt; + $this->sessionName = $sessionOptions['name']; + } + + public function generateCorrelationId(): ?string + { + if ($this->correlationIdSalt === null) { + return null; + } + + $sessionCookie = $this->requestStack->getMainRequest()?->cookies->get($this->sessionName); + + if ($sessionCookie === null) { + return null; + } + + return substr(hash('sha256', $sessionCookie.$this->correlationIdSalt), 0, 8); + } +} diff --git a/src/Session/LoggingSessionFactory.php b/src/Session/LoggingSessionFactory.php new file mode 100644 index 00000000..8491197f --- /dev/null +++ b/src/Session/LoggingSessionFactory.php @@ -0,0 +1,70 @@ +logger = WithContextLogger::from( + $monologLogger, + ['correlationId' => $sessionCorrelationIdService->generateCorrelationId() ?? ''], + ); + + parent::__construct($requestStack, $storageFactory, $usageReporter); + } + + public function createSession(): SessionInterface + { + $this->logger->info('Created new session.'); + + return parent::createSession(); + } +} 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/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php b/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php new file mode 100644 index 00000000..55f5ea5f --- /dev/null +++ b/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php @@ -0,0 +1,245 @@ + '/route']); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $stubControllerFactory = fn() => new class extends AbstractController { + }; + + $event = new ControllerArgumentsEvent( + self::$kernel, + $stubControllerFactory, + [], $request, + HttpKernelInterface::MAIN_REQUEST + ); + + $dispatcher = new EventDispatcher(); + + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->never())->method('log'); + + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService( + $requestStack, + ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ' + ), + ['name' => 'PHPSESSID'], + ); + + $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); + $dispatcher->dispatch($event, KernelEvents::REQUEST); + } + + public function testItDeniesAccessWhenThereIsNoActiveSession(): void + { + $this->expectException(AccessDeniedException::class); + $this->expectExceptionMessage('Access Denied.'); + + self::bootKernel(); + + $request = new Request(server: ['REQUEST_URI' => '/route']); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $stubControllerFactory = fn() => new class extends AbstractController { + }; + + $requestType = HttpKernelInterface::MAIN_REQUEST; + $controllerEvent = new ControllerEvent(self::$kernel, $stubControllerFactory, $request, $requestType); + $controllerEvent->setController($stubControllerFactory, [RequiresActiveSession::class => [null]]); + $event = new ControllerArgumentsEvent(self::$kernel, $controllerEvent, [], $request, $requestType); + + $dispatcher = new EventDispatcher(); + + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') + ->with( + LogLevel::ERROR, + 'Route requires active session. Active session wasn\'t found.', + ['correlationId' => '', 'route' => '/route'] + ); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); + + $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); + $dispatcher->dispatch($event, KernelEvents::REQUEST); + } + + public function testItDeniesAccessWhenThereIsNoSessionCookie(): void + { + $this->expectException(AccessDeniedException::class); + $this->expectExceptionMessage('Access Denied.'); + + self::bootKernel(); + + $session = new Session(new MockArraySessionStorage()); + $session->setId(self::SESSION_ID); + + $request = new Request(server: ['REQUEST_URI' => '/route']); + $request->setSession($session); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $stubControllerFactory = fn() => new class extends AbstractController { + }; + + $requestType = HttpKernelInterface::MAIN_REQUEST; + $controllerEvent = new ControllerEvent(self::$kernel, $stubControllerFactory, $request, $requestType); + $controllerEvent->setController($stubControllerFactory, [RequiresActiveSession::class => [null]]); + $event = new ControllerArgumentsEvent(self::$kernel, $controllerEvent, [], $request, $requestType); + + $dispatcher = new EventDispatcher(); + + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') + ->with( + LogLevel::ERROR, + 'Route requires active session. Active session wasn\'t found. No session cookie was set.', + ['correlationId' => '', 'route' => '/route'] + ); + + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); + + $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); + $dispatcher->dispatch($event, KernelEvents::REQUEST); + } + + public function testItDeniesAccessWhenTheActiveSessionDoesNotMatchTheSessionCookie(): void + { + $this->expectException(AccessDeniedException::class); + $this->expectExceptionMessage('Access Denied.'); + + self::bootKernel(); + + $session = new Session(new MockArraySessionStorage()); + $session->setId('erroneous-session-id'); + + $request = new Request(server: ['REQUEST_URI' => '/route'], cookies: ['PHPSESSID' => self::SESSION_ID]); + $request->setSession($session); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $stubControllerFactory = fn() => new class extends AbstractController { + }; + + $requestType = HttpKernelInterface::MAIN_REQUEST; + $controllerEvent = new ControllerEvent(self::$kernel, $stubControllerFactory, $request, $requestType); + $controllerEvent->setController($stubControllerFactory, [RequiresActiveSession::class => [null]]); + $event = new ControllerArgumentsEvent(self::$kernel, $controllerEvent, [], $request, $requestType); + + $dispatcher = new EventDispatcher(); + + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') + ->with( + LogLevel::ERROR, + 'Route requires active session. Session does not match session cookie.', + ['correlationId' => 'f02614d0', 'route' => '/route'] + ); + + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); + + $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); + $dispatcher->dispatch($event, KernelEvents::REQUEST); + } + + public function testItDoesNotThrowWhenTheActiveSessionMatchesTheSessionCookie(): void + { + self::bootKernel(); + + $session = new Session(new MockArraySessionStorage()); + $session->setId(self::SESSION_ID); + + $request = new Request(server: ['REQUEST_URI' => '/route'], cookies: ['PHPSESSID' => self::SESSION_ID]); + $request->setSession($session); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $stubControllerFactory = fn() => new class extends AbstractController { + }; + + $requestType = HttpKernelInterface::MAIN_REQUEST; + $controllerEvent = new ControllerEvent(self::$kernel, $stubControllerFactory, $request, $requestType); + $controllerEvent->setController($stubControllerFactory, [RequiresActiveSession::class => [null]]); + $event = new ControllerArgumentsEvent(self::$kernel, $controllerEvent, [], $request, $requestType); + + $dispatcher = new EventDispatcher(); + + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->never())->method('log'); + + $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 new file mode 100644 index 00000000..03e28235 --- /dev/null +++ b/tests/Unit/EventSubscriber/SessionStateListenerTest.php @@ -0,0 +1,221 @@ + '/route']); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $event = new RequestEvent(self::$kernel, $request, HttpKernelInterface::MAIN_REQUEST); + + $dispatcher = new EventDispatcher(); + + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') + ->with( + LogLevel::INFO, + 'User made a request without a session cookie.', + ['correlationId' => '', 'route' => '/route'] + ); + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSIONID'], + ); + + $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); + $dispatcher->dispatch($event, KernelEvents::REQUEST); + } + + public function testItLogsWhenUserHasNoSession(): void + { + self::bootKernel(); + + $request = new Request(server: ['REQUEST_URI' => '/route'], cookies: ['PHPSESSID' => self::SESSION_ID]); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $event = new RequestEvent(self::$kernel, $request, HttpKernelInterface::MAIN_REQUEST); + + $dispatcher = new EventDispatcher(); + + $mockLogger = $this->createMock(LoggerInterface::class); + + $mockLogger->expects($this->exactly(2)) + ->method('log') + ->willReturnCallback(function ($level, $message, $context) { + static $calledMessages = []; + + if (isset($calledMessages[$message])) { + $this->fail('Log message "' . $message . '" was called more than once.'); + } + $calledMessages[$message] = true; + + switch($message) { + case 'Session not found.': + case 'User made a request with a session cookie.': + $this->assertSame(LogLevel::INFO, $level); + $this->assertSame('f02614d0', $context['correlationId']); + $this->assertSame('/route', $context['route']); + break; + default: + $this->fail('Unexpected log message'); + } + + }) + ; + + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); + + $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); + $dispatcher->dispatch($event, KernelEvents::REQUEST); + } + + public function testItLogsAnErrorWhenTheSessionIdDoesNotMatchTheSessionCookie(): void + { + self::bootKernel(); + + $session = new Session(new MockArraySessionStorage()); + $session->setId('erroneous-session-id'); + + $request = new Request(server: ['REQUEST_URI' => '/route'], cookies: ['PHPSESSID' => self::SESSION_ID]); + $request->setSession($session); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $event = new RequestEvent(self::$kernel, $request, HttpKernelInterface::MAIN_REQUEST); + + $dispatcher = new EventDispatcher(); + + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->exactly(3)) + ->method('log') + ->willReturnCallback(function ($level, $message, $context) { + static $calledMessages = []; + + if (isset($calledMessages[$message])) { + $this->fail('Log message "' . $message . '" was called more than once.'); + } + $calledMessages[$message] = true; + + switch($message) { + case 'User made a request with a session cookie.': + case 'User has a session.': + case 'The session cookie does not match the session id.': + $this->assertSame($level === LogLevel::ERROR ? LogLevel::ERROR : LogLevel::INFO, $level); + $this->assertSame('f02614d0', $context['correlationId']); + $this->assertSame('/route', $context['route']); + break; + default: + $this->fail('Unexpected log message'); + } + }); + + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); + + $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); + $dispatcher->dispatch($event, KernelEvents::REQUEST); + } + + public function testTheUserSessionMatchesTheSessionCookie(): void + { + self::bootKernel(); + + $session = new Session(new MockArraySessionStorage()); + $session->setId(self::SESSION_ID); + + $request = new Request(server: ['REQUEST_URI' => '/route'], cookies: ['PHPSESSID' => self::SESSION_ID]); + $request->setSession($session); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + + $event = new RequestEvent(self::$kernel, $request, HttpKernelInterface::MAIN_REQUEST); + + $dispatcher = new EventDispatcher(); + + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->exactly(3)) + ->method('log') + ->willReturnCallback(function ($level, $message, $context) { + static $calledMessages = []; + + if (isset($calledMessages[$message])) { + $this->fail('Log message "' . $message . '" was called more than once.'); + } + $calledMessages[$message] = true; + + switch($message) { + case 'User made a request with a session cookie.': + case 'User has a session.': + case 'User session matches the session cookie.': + $this->assertSame(LogLevel::INFO, $level); + $this->assertSame('f02614d0', $context['correlationId']); + $this->assertSame('/route', $context['route']); + break; + default: + $this->fail('Unexpected log message'); + } + }); + + $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 new file mode 100644 index 00000000..c457f183 --- /dev/null +++ b/tests/Unit/Service/SessionCorrelationIdServiceTest.php @@ -0,0 +1,74 @@ +push($request); + + $service = new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'); + + $this->assertNull($service->generateCorrelationId()); + } + + public function testItGeneratesACorrelationIdBasedOnTheSessionCookie(): void + { + $request = new Request(cookies: ['PHPSESSID' => 'session-id']); + $requestStack = new RequestStack(); + $requestStack->push($request); + + $service = new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ' + ); + + $this->assertSame('f02614d0', $service->generateCorrelationId()); + } + + + /** + * @dataProvider saltProvider + */ + public function testItWillNotGenerateACorrelationIdWithoutAdequateSalt(?string $salt): void + { + $request = new Request(cookies: ['PHPSESSID' => 'session-id']); + $requestStack = new RequestStack(); + $requestStack->push($request); + + $service = new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], $salt); + + $this->assertNull($service->generateCorrelationId()); + } + + public function saltProvider(): array + { + return [ + 'empty salt' => [''], + 'null salt' => [null], + 'short salt' => ['abc'], + 'almost_long_enough salt' => ['1234567890ABCDE'], + ]; + } +} diff --git a/tests/Unit/Session/LoggingSessionFactoryTest.php b/tests/Unit/Session/LoggingSessionFactoryTest.php new file mode 100644 index 00000000..dde3e5d5 --- /dev/null +++ b/tests/Unit/Session/LoggingSessionFactoryTest.php @@ -0,0 +1,56 @@ + 'session-id']); + $requestStack = new RequestStack(); + $requestStack->push($request); + + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') + ->with( + LogLevel::INFO, + 'Created new session.', + ['correlationId' => 'f02614d0'] + ); + + $sessionFactory = new LoggingSessionFactory( + $requestStack, + $this->createStub(SessionStorageFactoryInterface::class), + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ); + + $this->assertInstanceOf(SessionInterface::class, $sessionFactory->createSession()); + } +} From b30b7b4dd8844e5c205a8d32af7223318338e4a2 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 17 Sep 2024 13:46:34 +0200 Subject: [PATCH 5/5] Add the session id derived id to polling requests Prior to this change, when a polling action from the browser hit the server, and the server returned a 500, or some other error, it was not possible to trace which session the browser belonged to. This change adds a correlation id to the polling mechanism, so that id can be used to search te logs for the origin of the session. This should help retrace errors. See: https://www.pivotaltracker.com/story/show/188205232 --- assets/typescript/Client/StatusClient.ts | 15 +- assets/typescript/authentication.ts | 6 +- assets/typescript/registration.ts | 10 +- composer.json | 1 - composer.lock | 136 +----------------- src/Controller/AuthenticationController.php | 6 +- src/Controller/RegistrationController.php | 3 + templates/default/authentication.html.twig | 3 +- templates/default/registration.html.twig | 3 +- ...iresActiveSessionAttributeListenerTest.php | 1 - .../SessionStateListenerTest.php | 1 - 11 files changed, 34 insertions(+), 151 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/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/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/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/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;