The browser should only have one trusted device cookie.

When a user is using two IDP's, it should only have to scan once.
OpenConext · Dec 19, 2024 · c820e66 · c820e66
1 parent 3f72b8d
commit c820e66
Show file tree

Hide file tree

Showing 10 changed files with 28 additions and 44 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -42,28 +42,25 @@ 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);
     }
 
     /**
      * 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,15 +186,15 @@ 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));
     }
 
     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'));
     }