From 991d3720c5c5be28347d721b7adbe6a9a486322e Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 Sep 2024 14:49:23 +0200 Subject: [PATCH 1/5] Make the Tiqr Configuration validation less cryptic The Assert statements would yield unusable error messages without a path to know what to go and fix. This change at least tells us what config item is not right. And what is expected. --- src/Tiqr/TiqrConfiguration.php | 121 ++++++++++++++++++++++++++------- 1 file changed, 97 insertions(+), 24 deletions(-) diff --git a/src/Tiqr/TiqrConfiguration.php b/src/Tiqr/TiqrConfiguration.php index dc80d57e..92cc0e30 100644 --- a/src/Tiqr/TiqrConfiguration.php +++ b/src/Tiqr/TiqrConfiguration.php @@ -35,83 +35,156 @@ class TiqrConfiguration implements TiqrConfigurationInterface /** * @param array> $tiqrConfiguration * - * @throws \Assert\AssertionFailedException + * @throws \Assert\AssertionFailedException\ + * @SuppressWarnings(PHPMD.ExcessiveMethodLength) */ public function __construct(array $tiqrConfiguration) { - Assertion::string($tiqrConfiguration['general']['identifier']); + Assertion::string( + $tiqrConfiguration['general']['identifier'], + 'TiqrConfiguration: general -> identifier must be of type string' + ); $this->options['identifier'] = $tiqrConfiguration['general']['identifier']; - Assertion::string($tiqrConfiguration['general']['name']); + Assertion::string( + $tiqrConfiguration['general']['name'], + 'TiqrConfiguration: general -> name must be of type string' + ); $this->options['name'] = $tiqrConfiguration['general']['name']; - Assertion::string($tiqrConfiguration['general']['auth_protocol']); + Assertion::string( + $tiqrConfiguration['general']['auth_protocol'], + 'TiqrConfiguration: general -> auth_protocol must be of type string' + ); $this->options['auth.protocol'] = $tiqrConfiguration['general']['auth_protocol']; - Assertion::string($tiqrConfiguration['general']['enroll_protocol']); + Assertion::string( + $tiqrConfiguration['general']['enroll_protocol'], + 'TiqrConfiguration: general -> enroll_protocol must be of type string' + ); $this->options['enroll.protocol'] = $tiqrConfiguration['general']['enroll_protocol']; - Assertion::string($tiqrConfiguration['general']['ocra_suite']); + Assertion::string( + $tiqrConfiguration['general']['ocra_suite'], + 'TiqrConfiguration: general -> ocra_suite must be of type string' + ); $this->options['ocra.suite'] = $tiqrConfiguration['general']['ocra_suite']; - Assertion::string($tiqrConfiguration['general']['logoUrl']); + Assertion::string( + $tiqrConfiguration['general']['logoUrl'], + 'TiqrConfiguration: general -> logoUrl must be of type string' + ); $this->options['logoUrl'] = $tiqrConfiguration['general']['logoUrl']; - Assertion::string($tiqrConfiguration['general']['infoUrl']); + Assertion::string( + $tiqrConfiguration['general']['infoUrl'], + 'TiqrConfiguration: general -> infoUrl must be of type string' + ); $this->options['infoUrl'] = $tiqrConfiguration['general']['infoUrl']; if (isset($tiqrConfiguration['library']['apns'])) { - Assertion::string($tiqrConfiguration['library']['apns']['certificate']); + Assertion::string( + $tiqrConfiguration['library']['apns']['certificate'], + 'TiqrConfiguration: library -> apns -> certificate must be of type string' + ); $this->options['apns.certificate'] = $tiqrConfiguration['library']['apns']['certificate']; - Assertion::string($tiqrConfiguration['library']['apns']['environment']); + Assertion::string( + $tiqrConfiguration['library']['apns']['environment'], + 'TiqrConfiguration: library -> apns -> environment must be of type string' + ); $this->options['apns.environment'] = $tiqrConfiguration['library']['apns']['environment']; } if (isset($tiqrConfiguration['library']['firebase']) && is_array($tiqrConfiguration['library']['firebase'])) { - Assertion::string($tiqrConfiguration['library']['firebase']['projectId']); + Assertion::string( + $tiqrConfiguration['library']['firebase']['projectId'], + 'TiqrConfiguration: library -> firebase -> projectId must be of type string' + ); $this->options['firebase.projectId'] = $tiqrConfiguration['library']['firebase']['projectId']; - Assertion::string($tiqrConfiguration['library']['firebase']['credentialsFile']); + Assertion::string( + $tiqrConfiguration['library']['firebase']['credentialsFile'], + 'TiqrConfiguration: library -> firebase -> credentialsFile must be of type string' + ); $this->options['firebase.credentialsFile'] = $tiqrConfiguration['library']['firebase']['credentialsFile']; - Assertion::boolean($tiqrConfiguration['library']['firebase']['cacheTokens']); + Assertion::boolean( + $tiqrConfiguration['library']['firebase']['cacheTokens'], + 'TiqrConfiguration: library -> firebase -> cacheTokens must be of type string' + ); $this->options['firebase.cacheTokens'] = $tiqrConfiguration['library']['firebase']['cacheTokens']; - Assertion::string($tiqrConfiguration['library']['firebase']['tokenCacheDir']); + Assertion::string( + $tiqrConfiguration['library']['firebase']['tokenCacheDir'], + 'TiqrConfiguration: library -> firebase -> tokenCacheDir must be of type string' + ); $this->options['firebase.tokenCacheDir'] = $tiqrConfiguration['library']['firebase']['tokenCacheDir']; } if (isset($tiqrConfiguration['accountblocking'][self::MAX_ATTEMPTS])) { - Assertion::digit($tiqrConfiguration['accountblocking'][self::MAX_ATTEMPTS]); - Assertion::greaterThan($tiqrConfiguration['accountblocking'][self::MAX_ATTEMPTS], 0); + Assertion::digit( + $tiqrConfiguration['accountblocking'][self::MAX_ATTEMPTS], + 'TiqrConfiguration: accountblocking -> maxAttempts must be of type digit' + ); + Assertion::greaterThan( + $tiqrConfiguration['accountblocking'][self::MAX_ATTEMPTS], + 0, + 'TiqrConfiguration: accountblocking -> maxAttempts must be greater that 0' + ); $this->options[self::MAX_ATTEMPTS] = $tiqrConfiguration['accountblocking'][self::MAX_ATTEMPTS]; } if (isset($tiqrConfiguration['accountblocking'][self::TEMPORARILY_BLOCK_DURATION])) { - Assertion::digit($tiqrConfiguration['accountblocking'][self::TEMPORARILY_BLOCK_DURATION]); - Assertion::greaterThan($tiqrConfiguration['accountblocking'][self::TEMPORARILY_BLOCK_DURATION], 0); + Assertion::digit( + $tiqrConfiguration['accountblocking'][self::TEMPORARILY_BLOCK_DURATION], + 'TiqrConfiguration: accountblocking -> temporarilyBlockDuration must be of type digit' + ); + Assertion::greaterThan( + $tiqrConfiguration['accountblocking'][self::TEMPORARILY_BLOCK_DURATION], + 0, + 'TiqrConfiguration: accountblocking -> temporarilyBlockDuration must be greater that 0' + ); $this->options[self::TEMPORARILY_BLOCK_DURATION] = $tiqrConfiguration['accountblocking'][self::TEMPORARILY_BLOCK_DURATION]; } if (isset($tiqrConfiguration['accountblocking'][self::MAX_TEMPORARILY_BLOCKS])) { - Assertion::digit($tiqrConfiguration['accountblocking'][self::MAX_TEMPORARILY_BLOCKS]); - Assertion::greaterThan($tiqrConfiguration['accountblocking'][self::MAX_TEMPORARILY_BLOCKS], 0); + Assertion::digit( + $tiqrConfiguration['accountblocking'][self::MAX_TEMPORARILY_BLOCKS], + 'TiqrConfiguration: accountblocking -> maxTemporarilyBlocks must be of type digit' + ); + Assertion::greaterThan( + $tiqrConfiguration['accountblocking'][self::MAX_TEMPORARILY_BLOCKS], + 0, + 'TiqrConfiguration: accountblocking -> maxTemporarilyBlocks must be greater that 0' + ); $this->options[self::MAX_TEMPORARILY_BLOCKS] = $tiqrConfiguration['accountblocking'][self::MAX_TEMPORARILY_BLOCKS]; } $this->options['statestorage']['type'] = $tiqrConfiguration['storage']['statestorage']['type']; - Assertion::isArray($tiqrConfiguration['storage']['statestorage']['arguments']); + Assertion::isArray( + $tiqrConfiguration['storage']['statestorage']['arguments'], + 'TiqrConfiguration: storage -> statestorage -> arguments must be of type array' + ); $this->options['statestorage'] += $tiqrConfiguration['storage']['statestorage']['arguments']; $this->options['userstorage']['type'] = $tiqrConfiguration['storage']['userstorage']['type']; - Assertion::isArray($tiqrConfiguration['storage']['userstorage']['arguments']); + Assertion::isArray( + $tiqrConfiguration['storage']['userstorage']['arguments'], + 'TiqrConfiguration: storage -> userstorage -> arguments must be of type array' + ); $this->options['userstorage'] += $tiqrConfiguration['storage']['userstorage']['arguments']; $this->options['devicestorage']['type'] = $tiqrConfiguration['storage']['devicestorage']['type']; - Assertion::isArray($tiqrConfiguration['storage']['devicestorage']['arguments']); + Assertion::isArray( + $tiqrConfiguration['storage']['devicestorage']['arguments'], + 'TiqrConfiguration: storage -> devicestorage -> arguments must be of type array' + ); $this->options['devicestorage'] += $tiqrConfiguration['storage']['devicestorage']['arguments']; if (isset($tiqrConfiguration['storage']['usersecretstorage'])) { $this->options['usersecretstorage']['type'] = $tiqrConfiguration['storage']['usersecretstorage']['type']; - Assertion::isArray($tiqrConfiguration['storage']['usersecretstorage']['arguments']); + Assertion::isArray( + $tiqrConfiguration['storage']['usersecretstorage']['arguments'], + 'TiqrConfiguration: storage -> usersecretstorage -> arguments must be of type array' + ); $this->options['usersecretstorage'] += $tiqrConfiguration['storage']['usersecretstorage']['arguments']; } } From 6e8410bbb5c3c45ae87be621c74aeaa43ec96a6a Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 Sep 2024 14:52:02 +0200 Subject: [PATCH 2/5] Move Authn Status to own controller Some additional bootstrapping was required to allow for this (in services.yaml). --- config/services.yaml | 9 +- src/Controller/AuthenticationController.php | 114 -------------- .../AuthenticationStatusController.php | 143 ++++++++++++++++++ 3 files changed, 150 insertions(+), 116 deletions(-) create mode 100644 src/Controller/AuthenticationStatusController.php diff --git a/config/services.yaml b/config/services.yaml index 3edf3b34..d1b7ac86 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -26,7 +26,13 @@ services: resource: '../src/*' exclude: '../src/{DependencyInjection,Entity,Migrations,Tests,Kernel.php}' - # add more service definitions when explicit configuration is needed + Surfnet\Tiqr\Controller\: + tags: [ 'controller.service_arguments' ] + resource: '../src/Controller/' + autowire: true + autoconfigure: true + + # add more service definitions when explicit configuration is needed # please note that last definitions always *replace* previous ones Symfony\Component\DependencyInjection\Container: @@ -51,6 +57,5 @@ services: $pattern: '%mobile_app_user_agent_pattern%' Surfnet\Tiqr\Controller\ExceptionController: - tags: ['controller.service_arguments'] arguments: $errorPageHelper: '@Surfnet\Tiqr\Service\ErrorPageHelper' diff --git a/src/Controller/AuthenticationController.php b/src/Controller/AuthenticationController.php index ee66432b..e0cf98a4 100644 --- a/src/Controller/AuthenticationController.php +++ b/src/Controller/AuthenticationController.php @@ -151,120 +151,6 @@ public function authentication(Request $request): Response ]); } - /** - * @throws InvalidArgumentException - * - * Requires session cookie to be set to a valid session. - */ - #[Route(path: '/authentication/status', name: 'app_identity_authentication_status', methods: ['GET'])] - public function authenticationStatus(): JsonResponse - { - $nameId = $this->authenticationService->getNameId(); - $sari = $this->stateHandler->getRequestId(); - $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]); - $logger->info('Request for authentication status'); - - // Do we have a valid GSSP authentication AuthnRequest in this session? - if (!$this->authenticationService->authenticationRequired()) { - $logger->error('There is no pending authentication request from SP'); - return $this->refreshAuthenticationPage(); - } - - // Check if the user is authenticated. This check will work if the user has completed the authentication - // within the last Tiqr_Service::LOGIN_EXPIRE seconds (1 hour). - // Uses our SessionId. - $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(); - } - - /** - * 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 - - // TODO: Refactor this so it does not depend on the implementation of the TiqrService - // i.e. we should keep track of authentication timeout ourselves - // Uses this method creates noise in the logs because it logs errors because keys are not found - // in the StateStorage. But this is not an error here, it is expected bacause expired keys - // cannot be found in the statestorage by definition. - // If you were truly interested in the authenticationUrl(), then logging the errors is correct. - try { - $this->tiqrService->authenticationUrl(); - } catch (Exception) { - return true; - } - return false; - } - - /** - * Authentication is pending, schedule a new poll action. - * - * @return JsonResponse - */ - private function scheduleNextPollOnAuthenticationPage(): JsonResponse - { - return $this->generateAuthenticationStatusResponse('pending'); - } - - /** - * Generate a response for authentication.html: Ask the user to retry. - * - * @return JsonResponse - */ - private function timeoutNeedsManualRetry(): JsonResponse - { - return $this->generateAuthenticationStatusResponse('challenge-expired'); - } - - /** - * Generate a response for authentication.html: refresh the page. - * - * @return JsonResponse - */ - private function refreshAuthenticationPage(): JsonResponse - { - return $this->generateAuthenticationStatusResponse('needs-refresh'); - } - - /** - * Generate a status response for authentication.html. - * - * The javascript in the authentication page expects one of three 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. - * - * @return JsonResponse - */ - private function generateAuthenticationStatusResponse(string $status): JsonResponse - { - return new JsonResponse($status); - } - /** * Generate a notification response for authentication.html. * diff --git a/src/Controller/AuthenticationStatusController.php b/src/Controller/AuthenticationStatusController.php new file mode 100644 index 00000000..ade35c09 --- /dev/null +++ b/src/Controller/AuthenticationStatusController.php @@ -0,0 +1,143 @@ +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(); + } + + /** + * Generate a status response for authentication.html. + * + * The javascript in the authentication page expects one of three 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. + * + * @return JsonResponse + */ + private function generateAuthenticationStatusResponse(string $status): JsonResponse + { + return new JsonResponse($status); + } + + /** + * Generate a response for authentication.html: refresh the page. + * + * @return JsonResponse + */ + 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. + * + * @return JsonResponse + */ + private function timeoutNeedsManualRetry(): JsonResponse + { + return $this->generateAuthenticationStatusResponse('challenge-expired'); + } + + /** + * Authentication is pending, schedule a new poll action. + * + * @return JsonResponse + */ + private function scheduleNextPollOnAuthenticationPage(): JsonResponse + { + return $this->generateAuthenticationStatusResponse('pending'); + } +} From 2ad5f7716a2673429b179ac340470527c1d5249e Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 Sep 2024 14:57:03 +0200 Subject: [PATCH 3/5] Make the other Authentication controllers invokable --- src/Controller/AuthenticationController.php | 134 +-------------- .../AuthenticationNotificationController.php | 155 ++++++++++++++++++ src/Controller/AuthenticationQrController.php | 65 ++++++++ 3 files changed, 221 insertions(+), 133 deletions(-) create mode 100644 src/Controller/AuthenticationNotificationController.php create mode 100644 src/Controller/AuthenticationQrController.php diff --git a/src/Controller/AuthenticationController.php b/src/Controller/AuthenticationController.php index e0cf98a4..79bfd9f0 100644 --- a/src/Controller/AuthenticationController.php +++ b/src/Controller/AuthenticationController.php @@ -38,7 +38,6 @@ use Surfnet\Tiqr\Tiqr\TiqrUserRepositoryInterface; use Surfnet\Tiqr\WithContextLogger; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; -use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Attribute\Route; @@ -65,7 +64,7 @@ public function __construct( * @throws Exception */ #[Route(path: '/authentication', name: 'app_identity_authentication', methods: ['GET', 'POST'])] - public function authentication(Request $request): Response + public function __invoke(Request $request): Response { $nameId = $this->authenticationService->getNameId(); $sari = $this->stateHandler->getRequestId(); @@ -151,105 +150,6 @@ public function authentication(Request $request): Response ]); } - /** - * Generate a notification response for authentication.html. - * - * The javascript in the authentication page expects one of three statuses: - * - * - success: Notification send successfully - * - error: Notification was not send successfully - * - no-device: There was no device to send the notification - * - * @return JsonResponse - */ - private function generateNotificationResponse(string $status): JsonResponse - { - return new JsonResponse($status); - } - - /** - * - * @throws InvalidArgumentException - */ - #[Route(path: '/authentication/qr', name: 'app_identity_authentication_qr', methods: ['GET'])] - public function authenticationQr(): Response - { - $nameId = $this->authenticationService->getNameId(); - $sari = $this->stateHandler->getRequestId(); - $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]); - $logger->info('Client request QR image'); - - // Do have a valid sample AuthnRequest?. - if (!$this->authenticationService->authenticationRequired()) { - $logger->error('There is no pending authentication request from SP'); - - return new Response('No authentication required', Response::HTTP_BAD_REQUEST); - } - - $logger->info('Return QR image response'); - - return $this->tiqrService->createAuthenticationQRResponse(); - } - - /** - * - * @throws InvalidArgumentException - */ - #[Route(path: '/authentication/notification', name: 'app_identity_authentication_notification', methods: ['POST'])] - public function authenticationNotification(): Response - { - $nameId = $this->authenticationService->getNameId(); - $sari = $this->stateHandler->getRequestId(); - $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]); - $logger->info('Client requests sending push notification'); - - // Do have a valid sample AuthnRequest?. - if (!$this->authenticationService->authenticationRequired()) { - $logger->error('There is no pending authentication request from SP'); - - return new Response('No authentication required', Response::HTTP_BAD_REQUEST); - } - - $logger->info('Sending push notification'); - - // Get user. - try { - $user = $this->userRepository->getUser($nameId); - } catch (UserNotExistsException $exception) { - $logger->error(sprintf( - 'User with nameId "%s" not found, error "%s"', - $nameId, - $exception->getMessage() - )); - - return new Response(null, Response::HTTP_BAD_REQUEST); - } - - // Send notification. - $notificationType = $user->getNotificationType(); - $notificationAddress = $user->getNotificationAddress(); - - if ($notificationType && $notificationAddress) { - $this->logger->notice(sprintf( - 'Sending push notification for user "%s" with type "%s" and (untranslated) address "%s"', - $nameId, - $notificationType, - $notificationAddress - )); - - $result = $this->sendNotification($notificationType, $notificationAddress); - if ($result) { - return $this->generateNotificationResponse('success'); - } - return $this->generateNotificationResponse('error'); - } - - $this->logger->notice(sprintf('No notification address for user "%s", no notification was sent', $nameId)); - - return $this->generateNotificationResponse('no-device'); - } - - private function handleInvalidResponse(TiqrUserInterface $user, AuthenticationResponse $response, LoggerInterface $logger): Response { try { @@ -285,36 +185,4 @@ private function showUserIsBlockedErrorPage(bool $isBlockedPermanently): Respons ] ); } - - /** - * @return bool True when the notification was successfully sent, false otherwise - */ - private function sendNotification(string $notificationType, string $notificationAddress): bool - { - try { - $this->tiqrService->sendNotification($notificationType, $notificationAddress); - } catch (Exception $e) { - $this->logger->warning( - sprintf( - 'Failed to send push notification for type "%s" and address "%s"', - $notificationType, - $notificationAddress - ), - [ - 'exception' => $e, - ] - ); - return false; - } - - $this->logger->notice( - sprintf( - 'Successfully sent push notification for type "%s" and address "%s"', - $notificationType, - $notificationAddress - ) - ); - - return true; - } } diff --git a/src/Controller/AuthenticationNotificationController.php b/src/Controller/AuthenticationNotificationController.php new file mode 100644 index 00000000..3ebbce69 --- /dev/null +++ b/src/Controller/AuthenticationNotificationController.php @@ -0,0 +1,155 @@ +authenticationService->getNameId(); + $sari = $this->stateHandler->getRequestId(); + $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]); + $logger->info('Client requests sending push notification'); + + // Do have a valid sample AuthnRequest?. + if (!$this->authenticationService->authenticationRequired()) { + $logger->error('There is no pending authentication request from SP'); + + return new Response('No authentication required', Response::HTTP_BAD_REQUEST); + } + + $logger->info('Sending push notification'); + + // Get user. + try { + $user = $this->userRepository->getUser($nameId); + } catch (UserNotExistsException $exception) { + $logger->error(sprintf( + 'User with nameId "%s" not found, error "%s"', + $nameId, + $exception->getMessage() + )); + + return new Response(null, Response::HTTP_BAD_REQUEST); + } + + // Send notification. + $notificationType = $user->getNotificationType(); + $notificationAddress = $user->getNotificationAddress(); + + if ($notificationType && $notificationAddress) { + $this->logger->notice(sprintf( + 'Sending push notification for user "%s" with type "%s" and (untranslated) address "%s"', + $nameId, + $notificationType, + $notificationAddress + )); + + $result = $this->sendNotification($notificationType, $notificationAddress); + if ($result) { + return $this->generateNotificationResponse('success'); + } + return $this->generateNotificationResponse('error'); + } + + $this->logger->notice(sprintf('No notification address for user "%s", no notification was sent', $nameId)); + + return $this->generateNotificationResponse('no-device'); + } + + /** + * @return bool True when the notification was successfully sent, false otherwise + */ + private function sendNotification(string $notificationType, string $notificationAddress): bool + { + try { + $this->tiqrService->sendNotification($notificationType, $notificationAddress); + } catch (Exception $e) { + $this->logger->warning( + sprintf( + 'Failed to send push notification for type "%s" and address "%s"', + $notificationType, + $notificationAddress + ), + [ + 'exception' => $e, + ] + ); + return false; + } + + $this->logger->notice( + sprintf( + 'Successfully sent push notification for type "%s" and address "%s"', + $notificationType, + $notificationAddress + ) + ); + + return true; + } + + /** + * Generate a notification response for authentication.html. + * + * The javascript in the authentication page expects one of three statuses: + * + * - success: Notification send successfully + * - error: Notification was not send successfully + * - no-device: There was no device to send the notification + * + * @return JsonResponse + */ + private function generateNotificationResponse(string $status): JsonResponse + { + return new JsonResponse($status); + } +} diff --git a/src/Controller/AuthenticationQrController.php b/src/Controller/AuthenticationQrController.php new file mode 100644 index 00000000..809d4c36 --- /dev/null +++ b/src/Controller/AuthenticationQrController.php @@ -0,0 +1,65 @@ +authenticationService->getNameId(); + $sari = $this->stateHandler->getRequestId(); + $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]); + $logger->info('Client request QR image'); + + // Do have a valid sample AuthnRequest?. + if (!$this->authenticationService->authenticationRequired()) { + $logger->error('There is no pending authentication request from SP'); + + return new Response('No authentication required', Response::HTTP_BAD_REQUEST); + } + + $logger->info('Return QR image response'); + + return $this->tiqrService->createAuthenticationQRResponse(); + } +} From 416047899f2a39d5f084d1f5cd305b05f0143d80 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 Sep 2024 15:43:34 +0200 Subject: [PATCH 4/5] When authn error occurs, send 'invalid-request' This error status was previously not supported. It is now. The uncaught errors are caught, and the invalid-request is sent back to the JS app. That in turn displays the user facing error page. --- .../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 ade35c09..469981d0 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 aba1285407ec626a1715264a1692c17971fcc4a7 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 11 Sep 2024 09:37:33 +0200 Subject: [PATCH 5/5] Handle unknown statuses as an error At first I opted to handle the 'invalid-request' manually. But having a default switch-case to handle all unhandled stati as an error makes more sense. And before this commit, the invalid request was handled as a Push Notification failure. But that was not my intention. I wanted to render the error page, and for that, we need to call the switchToStatusRequestError method instead. --- assets/typescript/AuthenticationPageService.ts | 6 +++--- .../typescript/__test__/AuthenticationPageService.test.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) 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/__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(); });