From e11170e3ffd59678b84d41015ccf83a1ef90d4e2 Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Wed, 11 Dec 2024 07:59:33 +0100 Subject: [PATCH] f --- .../__test__/AuthenticationPageService.test.ts | 10 +++++++++- ci/qa/phpcs.xml | 2 +- ci/qa/phpunit | 2 +- config/packages/framework.yaml | 4 ---- config/services_test.yaml | 8 ++------ .../AuthenticationNotificationController.php | 8 ++++++++ src/Features/Context/TiqrContext.php | 17 +++++++++++++++-- .../Framework}/FileValueStore.php | 8 ++++---- src/Features/mfaFatigueMitigation.feature | 10 ++++++++++ 9 files changed, 50 insertions(+), 19 deletions(-) rename src/{Service => Features/Framework}/FileValueStore.php (89%) diff --git a/assets/typescript/__test__/AuthenticationPageService.test.ts b/assets/typescript/__test__/AuthenticationPageService.test.ts index dee3bd4e..110b87c3 100644 --- a/assets/typescript/__test__/AuthenticationPageService.test.ts +++ b/assets/typescript/__test__/AuthenticationPageService.test.ts @@ -227,11 +227,19 @@ describe('AuthenticationPageService', () => { throw new Error('Should have started notification request'); } const spy = jest.spyOn(context.authenticationPageService, 'switchToNoDevice'); - // @TODO add testcase for no-trusted-device successCallback('no-device'); expect(spy).toBeCalled(); }); + it('Should show qr when there is no trusted-device cookie', () => { + if (!successCallback || !errorCallback) { + throw new Error('Should have started notification request'); + } + const spy = jest.spyOn(context.authenticationPageService, 'switchToNoDevice'); + successCallback('no-trusted-device'); + expect(spy).toBeCalled(); + }); + it('Should handle connection errors', () => { if (!successCallback || !errorCallback) { throw new Error('Should have started notification request'); diff --git a/ci/qa/phpcs.xml b/ci/qa/phpcs.xml index 7ebc3158..123f46cd 100644 --- a/ci/qa/phpcs.xml +++ b/ci/qa/phpcs.xml @@ -4,7 +4,7 @@ xsi:noNamespaceSchemaLocation="../../vendor/squizlabs/php_codesniffer/phpcs.xsd"> - + diff --git a/ci/qa/phpunit b/ci/qa/phpunit index 18bab109..8a85f031 100755 --- a/ci/qa/phpunit +++ b/ci/qa/phpunit @@ -3,4 +3,4 @@ cd $(dirname $0)/../../ printf "\nStart PHPUnit tests\n" -XDEBUG_MODE=coverage ./vendor/bin/phpunit --configuration=ci/qa/phpunit.xml --cache-result-file=../../var/cache/.phpunit-cache --coverage-text +XDEBUG_MODE=coverage ./vendor/bin/phpunit --configuration=ci/qa/phpunit.xml --coverage-text diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index 6a0acf93..8d92b795 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -40,10 +40,6 @@ when@test: test: true session: storage_factory_id: session.storage.factory.mock_file - handler_id: null - cookie_secure: true - cookie_samesite: none name: MOCKSESSID - cookie_httponly: true profiler: collect: false diff --git a/config/services_test.yaml b/config/services_test.yaml index fae4130d..63a27b78 100644 --- a/config/services_test.yaml +++ b/config/services_test.yaml @@ -16,12 +16,8 @@ services: arguments: - '/^Behat UA$/' -# surfnet_gssp.value_store.service: -# class: Surfnet\GsspBundle\Service\ValueStore\InMemoryValueStore -# public: true - surfnet_gssp.value_store.service: - class: Surfnet\Tiqr\Service\FileValueStore + class: Surfnet\Tiqr\Features\Framework\FileValueStore public: true arguments: - $filePath: '/var/www/html/var/store.json' + $filePath: '/var/www/html/var/gssp_store.json' diff --git a/src/Controller/AuthenticationNotificationController.php b/src/Controller/AuthenticationNotificationController.php index 6495cbf8..9830b3a4 100644 --- a/src/Controller/AuthenticationNotificationController.php +++ b/src/Controller/AuthenticationNotificationController.php @@ -109,6 +109,14 @@ public function __invoke(Request $request): Response } if (!$this->trustedCookieService->isTrustedDevice($cookie, $nameId, $notificationAddress)) { + $this->logger->notice( + sprintf( + 'A trusted device cookie is found for notification address "%s" and user "%s", but has signature mismatch', + $notificationAddress, + $nameId + ) + ); + return $this->generateNotificationResponse('no-trusted-device'); } diff --git a/src/Features/Context/TiqrContext.php b/src/Features/Context/TiqrContext.php index 807c8571..fb29d056 100644 --- a/src/Features/Context/TiqrContext.php +++ b/src/Features/Context/TiqrContext.php @@ -592,15 +592,17 @@ public function aPushNotificationIsSent(): void /** * @When /^push notification is sent with a trusted\-device cookie with address "([^"]*)"$/ + * @When /^push notification is sent with a trusted\-device cookie with address "([^"]*)" and cookie userId "([^"]*)"$/ */ - public function aPushNotificationIsSentWithATrustedDevice(string $notificationAddress): void + public function aPushNotificationIsSentWithATrustedDevice(string $notificationAddress, string $cookieUserId = null): void { $userId = $this->metadata->identity->identifier; + $cookieUserId = $cookieUserId ?? $userId; $config = new Configuration('tiqr-trusted-device', 3600, '000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f', 'none'); $cryptoHelper = new HaliteCryptoHelper($config); - $cookieValue = CookieValue::from($userId, $notificationAddress); + $cookieValue = CookieValue::from($cookieUserId, $notificationAddress); $helper = new CookieHelper($config, $cryptoHelper, new NullLogger()); $cookieName = $helper->buildCookieName($userId, $notificationAddress); @@ -677,6 +679,17 @@ public function theLogsShouldSayNoTrustedCookie(string $address): void ); } + /** + * @Then /^the logs should mention a signature mismatch for address "([^"]*)"$/ + */ + public function theLogsShouldMentionSignatureMismatch(string $address): void + { + $userId = $this->metadata->identity->identifier; + $this->logsContain( + 'A trusted device cookie is found for notification address "'.$address.'" and user "'.$userId.'", but has signature mismatch' + ); + } + /** * @Then /^I dump the page$/ */ diff --git a/src/Service/FileValueStore.php b/src/Features/Framework/FileValueStore.php similarity index 89% rename from src/Service/FileValueStore.php rename to src/Features/Framework/FileValueStore.php index bc8d080f..7615f460 100644 --- a/src/Service/FileValueStore.php +++ b/src/Features/Framework/FileValueStore.php @@ -18,7 +18,7 @@ * limitations under the License. */ -namespace Surfnet\Tiqr\Service; +namespace Surfnet\Tiqr\Features\Framework; use InvalidArgumentException; use Surfnet\GsspBundle\Exception\NotFound; @@ -32,7 +32,7 @@ public function __construct(string $filePath) { $this->filePath = $filePath; if (!file_exists($this->filePath)) { - file_put_contents($this->filePath, json_encode([])); + file_put_contents($this->filePath, json_encode([], JSON_THROW_ON_ERROR)); chmod($this->filePath, 0666); } } @@ -47,7 +47,7 @@ private function readValues(): array throw new InvalidArgumentException(sprintf('Could not read FileValueStore storage file. %s', $this->filePath)); } - $result = json_decode($content, true); + $result = json_decode($content, true, 512, JSON_THROW_ON_ERROR); return is_array($result) ? $result : []; } @@ -57,7 +57,7 @@ private function readValues(): array */ private function writeValues(array $values): void { - file_put_contents($this->filePath, json_encode($values)); + file_put_contents($this->filePath, json_encode($values, JSON_THROW_ON_ERROR)); } public function set(string $key, mixed $value): self diff --git a/src/Features/mfaFatigueMitigation.feature b/src/Features/mfaFatigueMitigation.feature index 2bedc57b..6bf78f81 100644 --- a/src/Features/mfaFatigueMitigation.feature +++ b/src/Features/mfaFatigueMitigation.feature @@ -42,6 +42,16 @@ Feature: When an user needs to authenticate When push notification is sent with a trusted-device cookie with address "0000000000111111111122222222223333333333" Then it should send a notification for the user with type "APNS" and address "0000000000111111111122222222223333333333" + Scenario: When a user tries to authenticates with a trusted cookie, but changes the address, a notification should not be sent + Given I am on "/demo/sp" + And I fill in "NameID" with my identifier + When I press "authenticate" + Then I should see "Log in with tiqr" + And I should be on "/authentication" + + When push notification is sent with a trusted-device cookie with address "0000000000111111111122222222223333333333" and cookie userId "abc-1234" + Then the logs should mention a signature mismatch for address "0000000000111111111122222222223333333333" + And it should fail with "no-trusted-device" # @TODO Add a test somewhere, maybe not here, that tests the cookie get overwritten properly (or appended) if a new scan occurs?