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.

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

3. Improve handling of qr timeout & state errors when polling #210

Merged
merged 7 commits into from
Nov 29, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ export class AuthenticationPageService {
case 'needs-refresh':
this.reloadPage();
break;
default:
this.switchToStatusRequestError();
break;
}
};

Expand Down
6 changes: 6 additions & 0 deletions assets/typescript/Component/RegistrationStatusComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
8 changes: 8 additions & 0 deletions assets/typescript/RegistrationStateMachine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
pmeulen marked this conversation as resolved.
Show resolved Hide resolved
break;
default:
this.unknownError();
return;
Expand Down
8 changes: 8 additions & 0 deletions assets/typescript/__test__/AuthenticationPageService.test.ts
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, 'switchToStatusRequestError');
successCallback('invalid-request');
expect(spy).toBeCalled();
});

it('Should handle challenge expired', () => {
if (!successCallback || !errorCallback) {
Expand Down
23 changes: 23 additions & 0 deletions assets/typescript/__test__/RegistrationPageService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -227,6 +249,7 @@ describe('RegistrationPageService', () => {
showAccountActivationHelp:jest.fn(),
showOneMomentPlease: jest.fn(),
showFinalized: jest.fn(),
showTimeoutHappened: jest.fn(),
showUnknownErrorHappened: jest.fn(),
};

Expand Down
195 changes: 195 additions & 0 deletions ci/qa/phpstan-baseline.neon
Original file line number Diff line number Diff line change
@@ -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\\>\\|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\\<float\\|int\\|string\\|Stringable\\|null\\>, array\\<int, mixed\\> 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
Expand Down
1 change: 1 addition & 0 deletions ci/qa/phpstan.neon
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ parameters:
level: 9
paths:
- ../../src
- ../../dev
21 changes: 13 additions & 8 deletions dev/Command/AuthenticationCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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);
Expand All @@ -126,7 +126,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$service['id'] = $serviceId;
$output->writeln([
'<comment>Generate OCRA for service:</comment>',
$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, '');
Expand All @@ -143,16 +143,16 @@ 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([
sprintf(
'<comment>Send authentication data to "%s" with body:</comment>',
$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, [
Expand All @@ -166,22 +166,27 @@ protected function execute(InputInterface $input, OutputInterface $output): int
return Command::SUCCESS;
}

protected function decorateResult($text): string
protected function decorateResult(string $text): string
{
return "<options=bold>$text</>";
}

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;
}

/**
Expand Down
Loading