From 0e18752026136ffa0e73b98a0b72aee96b17282c Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 11 Sep 2024 09:37:33 +0200 Subject: [PATCH] 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