Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error reporting #204

Merged
merged 5 commits into from
Sep 11, 2024
Merged

Improve error reporting #204

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
991d372
Make the Tiqr Configuration validation less cryptic
MKodde Sep 2, 2024
6e8410b
Move Authn Status to own controller
MKodde Sep 2, 2024
2ad5f77
Make the other Authentication controllers invokable
MKodde Sep 2, 2024
4160478
When authn error occurs, send 'invalid-request'
MKodde Sep 2, 2024
aba1285
Handle unknown statuses as an error
MKodde Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions assets/typescript/AuthenticationPageService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 8 additions & 0 deletions assets/typescript/__test__/AuthenticationPageService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
9 changes: 7 additions & 2 deletions config/services.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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'
248 changes: 1 addition & 247 deletions src/Controller/AuthenticationController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down Expand Up @@ -151,219 +150,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.
*
* 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 {
Expand Down Expand Up @@ -399,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;
}
}
Loading