From c820e6653278116ea892f9eb378fdc1c2789738f Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Thu, 19 Dec 2024 08:31:21 +0100 Subject: [PATCH] The browser should only have one trusted device cookie. When a user is using two IDP's, it should only have to scan once. --- config/openconext/parameters.yaml.dist | 2 +- config/services.yaml | 2 +- .../AuthenticationNotificationController.php | 2 +- src/Features/Context/TiqrContext.php | 6 ++--- .../TrustedDevice/Http/CookieHelper.php | 23 ++++++----------- .../Http/CookieHelperInterface.php | 4 +-- .../TrustedDevice/TrustedDeviceService.php | 4 +-- .../ValueObject/Configuration.php | 2 +- .../DateTime/ExpirationHelperTest.php | 2 +- .../TrustedCookieServiceTest.php | 25 ++++++++----------- 10 files changed, 28 insertions(+), 44 deletions(-) diff --git a/config/openconext/parameters.yaml.dist b/config/openconext/parameters.yaml.dist index 549e5ae6..8734eeae 100644 --- a/config/openconext/parameters.yaml.dist +++ b/config/openconext/parameters.yaml.dist @@ -49,7 +49,7 @@ parameters: trusted_device_cookie_lifetime: 2592000 # The prefix used for the trusted-device cookies - trusted_device_cookie_prefix: 'tiqr-trusted-device' + trusted_device_cookie_name: 'tiqr-trusted-device' # The same_site attribute of the trusted-device cookies # Should be one of the strings in: \Surfnet\Tiqr\Service\TrustedDevice\Http\CookieSameSite: 'none', 'lax', 'strict' diff --git a/config/services.yaml b/config/services.yaml index 410ec151..cbd06994 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -68,7 +68,7 @@ services: Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration: public: false arguments: - - "%trusted_device_cookie_prefix%" + - "%trusted_device_cookie_name%" - "%trusted_device_cookie_lifetime%" - "%trusted_device_encryption_key%" - "%trusted_device_cookie_same_site%" diff --git a/src/Controller/AuthenticationNotificationController.php b/src/Controller/AuthenticationNotificationController.php index a035d01a..dc31090c 100644 --- a/src/Controller/AuthenticationNotificationController.php +++ b/src/Controller/AuthenticationNotificationController.php @@ -96,7 +96,7 @@ public function __invoke(Request $request): Response return $this->generateNotificationResponse('no-device'); } - $cookie = $this->trustedDeviceService->read($request, $nameId, $notificationAddress); + $cookie = $this->trustedDeviceService->read($request); if ($cookie === null) { $this->logger->notice( sprintf( diff --git a/src/Features/Context/TiqrContext.php b/src/Features/Context/TiqrContext.php index f503d8c9..70af33a8 100644 --- a/src/Features/Context/TiqrContext.php +++ b/src/Features/Context/TiqrContext.php @@ -30,13 +30,11 @@ use Exception; use GuzzleHttp\Exception\GuzzleException; use OCRA; -use Psr\Log\NullLogger; use RuntimeException; use stdClass; use Surfnet\SamlBundle\Exception\NotFound; use Surfnet\Tiqr\Dev\FileLogger; use Surfnet\Tiqr\Service\TrustedDevice\Crypto\HaliteCryptoHelper; -use Surfnet\Tiqr\Service\TrustedDevice\Http\CookieHelper; use Surfnet\Tiqr\Service\TrustedDevice\TrustedDeviceService; use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration; use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue; @@ -359,7 +357,7 @@ public function weHaveATrustedDevice(string $notificationAddress): void foreach ($cookieJar as $cookie) { $request->cookies->set($cookie->getName(), $cookie->getValue()); } - $cookieValue = $this->trustedDeviceService->read($request, $userId, $notificationAddress); + $cookieValue = $this->trustedDeviceService->read($request); Assertion::isInstanceOf($cookieValue, CookieValue::class); Assertion::true($this->trustedDeviceService->isTrustedDevice($cookieValue, $userId, $notificationAddress)); } @@ -602,7 +600,7 @@ public function aPushNotificationIsSentWithATrustedDevice(string $notificationAd $cryptoHelper = new HaliteCryptoHelper($config); $cookieValue = CookieValue::from($cookieUserId, $notificationAddress); - $cookieName = 'tiqr-trusted-device' . hash('sha256', $userId . '_' . $notificationAddress); + $cookieName = 'tiqr-trusted-device'; $encryptedValue = $cryptoHelper->encrypt($cookieValue); diff --git a/src/Service/TrustedDevice/Http/CookieHelper.php b/src/Service/TrustedDevice/Http/CookieHelper.php index 75a876c6..f4b83bb3 100644 --- a/src/Service/TrustedDevice/Http/CookieHelper.php +++ b/src/Service/TrustedDevice/Http/CookieHelper.php @@ -42,14 +42,12 @@ public function __construct( public function write(Response $response, CookieValue $value): void { - $cookieName = $this->buildCookieName($value->getUserId(), $value->getNotificationAddress()); - // The CookieValue is encrypted $encryptedCookieValue = $this->encryptionHelper->encrypt($value); $fingerprint = $this->hashFingerprint($encryptedCookieValue); $this->logger->notice(sprintf('Writing a trusted-device cookie with fingerprint %s', $fingerprint)); // Create a Symfony HttpFoundation cookie object - $cookie = $this->createCookieWithValue($encryptedCookieValue, $cookieName); + $cookie = $this->createCookieWithValue($encryptedCookieValue, $this->configuration->cookieName); // Which is added to the response headers $response->headers->setCookie($cookie); } @@ -57,13 +55,12 @@ public function write(Response $response, CookieValue $value): void /** * Retrieve the current cookie from the Request if it exists. */ - public function read(Request $request, string $userId, string $notificationAddress): CookieValue + public function read(Request $request): CookieValue { - $cookieName = $this->buildCookieName($userId, $notificationAddress); - if (!$request->cookies->has($cookieName)) { + if (!$request->cookies->has($this->configuration->cookieName)) { throw new CookieNotFoundException(); } - $cookie = $request->cookies->get($cookieName); + $cookie = $request->cookies->get($this->configuration->cookieName); if (!is_string($cookie)) { throw new InvalidArgumentException('Cookie payload must be string.'); } @@ -72,13 +69,12 @@ public function read(Request $request, string $userId, string $notificationAddre return $this->encryptionHelper->decrypt($cookie); } - public function fingerprint(Request $request, string $userId, string $notificationAddress): string + public function fingerprint(Request $request): string { - $cookieName = $this->buildCookieName($userId, $notificationAddress); - if (!$request->cookies->has($cookieName)) { + if (!$request->cookies->has($this->configuration->cookieName)) { throw new CookieNotFoundException(); } - $cookie = $request->cookies->get($cookieName); + $cookie = $request->cookies->get($this->configuration->cookieName); if (!is_string($cookie)) { throw new InvalidArgumentException('Cookie payload must be string.'); } @@ -110,9 +106,4 @@ private function getTimestamp(int $expiresInSeconds): int $currentTimestamp = time(); return $currentTimestamp + $expiresInSeconds; } - - private function buildCookieName(string $userId, string $notificationAddress): string - { - return $this->configuration->prefix . hash('sha256', $userId . '_' . $notificationAddress); - } } diff --git a/src/Service/TrustedDevice/Http/CookieHelperInterface.php b/src/Service/TrustedDevice/Http/CookieHelperInterface.php index 5f248b85..d5cd1b77 100644 --- a/src/Service/TrustedDevice/Http/CookieHelperInterface.php +++ b/src/Service/TrustedDevice/Http/CookieHelperInterface.php @@ -28,7 +28,7 @@ interface CookieHelperInterface { public function write(Response $response, CookieValue $value): void; - public function read(Request $request, string $userId, string $notificationAddress): CookieValue; + public function read(Request $request): CookieValue; - public function fingerprint(Request $request, string $userId, string $notificationAddress): string; + public function fingerprint(Request $request): string; } diff --git a/src/Service/TrustedDevice/TrustedDeviceService.php b/src/Service/TrustedDevice/TrustedDeviceService.php index dd2e42e7..57c5d005 100644 --- a/src/Service/TrustedDevice/TrustedDeviceService.php +++ b/src/Service/TrustedDevice/TrustedDeviceService.php @@ -60,10 +60,10 @@ public function isTrustedDevice( return true; } - public function read(Request $request, string $userId, string $notificationAddress): ?CookieValue + public function read(Request $request): ?CookieValue { try { - return $this->cookieHelper->read($request, $userId, $notificationAddress); + return $this->cookieHelper->read($request); } catch (CookieNotFoundException $e) { $this->logger->notice('A trusted-device cookie is not found'); return null; diff --git a/src/Service/TrustedDevice/ValueObject/Configuration.php b/src/Service/TrustedDevice/ValueObject/Configuration.php index 4d227f4c..7e34c0da 100644 --- a/src/Service/TrustedDevice/ValueObject/Configuration.php +++ b/src/Service/TrustedDevice/ValueObject/Configuration.php @@ -35,7 +35,7 @@ public CookieSameSite $sameSite; public function __construct( - public string $prefix, + public string $cookieName, public int $lifetimeInSeconds, #[SensitiveParameter] string $encryptionKey, string $sameSite, diff --git a/tests/Unit/Service/TrustedCookie/DateTime/ExpirationHelperTest.php b/tests/Unit/Service/TrustedCookie/DateTime/ExpirationHelperTest.php index 8dc18841..394fbdfd 100644 --- a/tests/Unit/Service/TrustedCookie/DateTime/ExpirationHelperTest.php +++ b/tests/Unit/Service/TrustedCookie/DateTime/ExpirationHelperTest.php @@ -113,7 +113,7 @@ private function makeExpirationHelper(int $expirationTime, int $now) : Expiratio $time->setTimestamp($now); $config = new Configuration( - '', + 'trusted-device-cookie', $expirationTime, '000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f', 'none', diff --git a/tests/Unit/Service/TrustedCookie/TrustedCookieServiceTest.php b/tests/Unit/Service/TrustedCookie/TrustedCookieServiceTest.php index cd21010b..185177ef 100644 --- a/tests/Unit/Service/TrustedCookie/TrustedCookieServiceTest.php +++ b/tests/Unit/Service/TrustedCookie/TrustedCookieServiceTest.php @@ -65,7 +65,7 @@ public function test_storing_a_persistent_cookie(): void { $this->buildService( new Configuration( - 'tiqr-trusted-device-cookie_', + 'tiqr-trusted-device-cookie', 60, '0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f', CookieSameSite::SAMESITE_STRICT->value, @@ -79,7 +79,7 @@ public function test_storing_a_persistent_cookie(): void self::assertCount(1, $cookieJar); $cookie = reset($cookieJar); // The name and lifetime of the cookie should match the one we configured it to be - self::assertEquals($this->configuration->prefix . hash('sha256', 'userId#1_'.'01011001'), $cookie->getName()); + self::assertEquals($this->configuration->cookieName, $cookie->getName()); self::assertEquals(time() + $this->configuration->lifetimeInSeconds, $cookie->getExpiresTime()); // By default, we set same-site header to none self::assertEquals(Cookie::SAMESITE_STRICT, $cookie->getSameSite()); @@ -165,7 +165,7 @@ public function test_read_write_cookie(): void { $this->buildService( new Configuration( - 'tiqr-trusted-device-cookie_', + 'tiqr-trusted-device-cookie', 60, '0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f', CookieSameSite::SAMESITE_STRICT->value, @@ -186,7 +186,7 @@ public function test_read_write_cookie(): void $request->cookies->set($cookie->getName(), $cookie->getValue()); } - $readCookie = $this->service->read($request, $userId, $notificationAddress); + $readCookie = $this->service->read($request); $this->assertTrue($this->service->isTrustedDevice($readCookie, $userId, $notificationAddress)); } @@ -194,7 +194,7 @@ public function test_does_not_read_tampered_cookie(): void { $this->buildService( new Configuration( - 'tiqr-trusted-device-cookie_', + 'tiqr-trusted-device-cookie', 60, '0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f', CookieSameSite::SAMESITE_STRICT->value, @@ -215,14 +215,11 @@ public function test_does_not_read_tampered_cookie(): void $request->cookies->set($cookie->getName(), $cookie->getValue() . '1'); } - $readCookie = $this->service->read($request, $userId, $notificationAddress); + $readCookie = $this->service->read($request); $this->assertNull($readCookie); } - /** - * This test is to make sure multiple users and users with multiple devices can use the same browser without issues - */ - public function test_it_handles_all_valid_cookies_from_browser(): void + public function test_it_overwrites_existing_cookies(): void { $this->buildService( new Configuration( @@ -266,7 +263,7 @@ public function test_it_handles_all_valid_cookies_from_browser(): void } $cookieJar = $response->headers->getCookies(); - self::assertCount(5, $cookieJar); + self::assertCount(1, $cookieJar); $request = new Request(); foreach ($cookieJar as $cookie) { @@ -275,10 +272,8 @@ public function test_it_handles_all_valid_cookies_from_browser(): void shuffle($store); - foreach ($store as $storedDevice){ - $readCookie = $this->service->read($request, $storedDevice['userId'], $storedDevice['notificationAddress']); - $this->assertTrue($this->service->isTrustedDevice($readCookie, $storedDevice['userId'], $storedDevice['notificationAddress'])); - } + $readCookie = $this->service->read($request); + $this->assertTrue($this->service->isTrustedDevice($readCookie, 'userId#1', '1')); }