Merge pull request #344 from OpenConext/feature/341-fix-cookie

Fix: Actually apply the trusted device cookie to the browser

config/services_test.yaml

@@ -21,3 +21,9 @@ services:
         public: true
         arguments:
             $filePath: '/var/www/html/var/gssp_store.json'
+
+    Surfnet\Tiqr\Features\Context\TiqrContext:
+        autowire: true
+        autoconfigure: true
+        arguments:
+            $store: '@surfnet_gssp.value_store.service'

src/Controller/AuthenticationController.php

@@ -30,6 +30,7 @@
 use Surfnet\Tiqr\Exception\UserPermanentlyBlockedException;
 use Surfnet\Tiqr\Exception\UserTemporarilyBlockedException;
 use Surfnet\Tiqr\Service\SessionCorrelationIdService;
+use Surfnet\Tiqr\Service\TrustedDeviceHelper;
 use Surfnet\Tiqr\Tiqr\AuthenticationRateLimitServiceInterface;
 use Surfnet\Tiqr\Tiqr\Exception\UserNotExistsException;
 use Surfnet\Tiqr\Tiqr\Response\AuthenticationResponse;
@@ -42,6 +43,7 @@
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Routing\Attribute\Route;
+use Throwable;
 
 /**
  * @SuppressWarnings(PHPMD.CouplingBetweenObjects)
@@ -55,6 +57,7 @@ public function __construct(
         private readonly TiqrUserRepositoryInterface $userRepository,
         private readonly AuthenticationRateLimitServiceInterface $authenticationRateLimitService,
         private readonly SessionCorrelationIdService $correlationIdService,
+        private readonly TrustedDeviceHelper $trustedDeviceHelper,
         private readonly LoggerInterface $logger
     ) {
     }
@@ -122,7 +125,10 @@ public function __invoke(Request $request): Response
             $logger->info('Authentication is finalized, returning to SP');
             $this->authenticationService->authenticate();
 
-            return $this->authenticationService->replyToServiceProvider();
+            $response = $this->authenticationService->replyToServiceProvider();
+            $this->trustedDeviceHelper->handleRegisterTrustedDevice($user->getNotificationAddress(), $response);
+
+            return $response;
         }
 
         // Start authentication process.

src/Controller/RegistrationController.php

@@ -26,20 +26,28 @@
 use Surfnet\Tiqr\Attribute\RequiresActiveSession;
 use Surfnet\Tiqr\Exception\NoActiveAuthenrequestException;
 use Surfnet\Tiqr\Service\SessionCorrelationIdService;
+use Surfnet\Tiqr\Service\TrustedDeviceHelper;
 use Surfnet\Tiqr\Tiqr\Legacy\TiqrService;
 use Surfnet\Tiqr\Tiqr\TiqrServiceInterface;
+use Surfnet\Tiqr\Tiqr\TiqrUserRepositoryInterface;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Routing\Attribute\Route;
+use Throwable;
 
+/**
+ * @SuppressWarnings(PHPMD.CouplingBetweenObjects)
+ */
 class RegistrationController extends AbstractController
 {
     public function __construct(
         private readonly RegistrationService $registrationService,
+        private readonly TiqrUserRepositoryInterface $userRepository,
         private readonly StateHandlerInterface $stateHandler,
         private readonly TiqrServiceInterface $tiqrService,
         private readonly SessionCorrelationIdService $correlationIdService,
+        private readonly TrustedDeviceHelper $trustedDeviceHelper,
         private readonly LoggerInterface $logger
     ) {
     }
@@ -66,8 +74,19 @@ public function registration(Request $request): Response
 
         if ($this->tiqrService->enrollmentFinalized()) {
             $this->logger->info('Registration is finalized returning to service provider');
-            $this->registrationService->register($this->tiqrService->getUserId());
-            return $this->registrationService->replyToServiceProvider();
+            $userId = $this->tiqrService->getUserId();
+            $this->registrationService->register($userId);
+
+            $response = $this->registrationService->replyToServiceProvider();
+
+            try {
+                $user = $this->userRepository->getUser($userId);
+                $this->trustedDeviceHelper->handleRegisterTrustedDevice($user->getNotificationAddress(), $response);
+            } catch (Throwable $e) {
+                $this->logger->warning(sprintf('Could not fetch registered user "%s"', $userId), ['exception' => $e]);
+            }
+
+            return $response;
         }
 
         $this->logger->info('Registration is not finalized return QR code');

src/Controller/TiqrAppApiController.php

@@ -22,20 +22,17 @@
 
 use Exception;
 use Psr\Log\LoggerInterface;
-use Surfnet\Tiqr\Service\TrustedDevice\TrustedDeviceService;
 use Surfnet\Tiqr\Service\UserAgentMatcherInterface;
 use Surfnet\Tiqr\Tiqr\AuthenticationRateLimitServiceInterface;
 use Surfnet\Tiqr\Tiqr\Exception\UserNotExistsException;
 use Surfnet\Tiqr\Tiqr\TiqrServiceInterface;
-use Surfnet\Tiqr\Tiqr\TiqrUserInterface;
 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;
-use Throwable;
 
 /**
  * The api that connects to the Tiqr app.
@@ -52,7 +49,6 @@ public function __construct(
         private readonly TiqrUserRepositoryInterface $userRepository,
         private readonly AuthenticationRateLimitServiceInterface $authenticationRateLimitService,
         private readonly LoggerInterface $logger,
-        private readonly TrustedDeviceService $cookieService,
     ) {
     }
 
@@ -249,15 +245,7 @@ private function registerAction(
             $logger->warning('Error finalizing enrollment', ['exception' => $e]);
         }
 
-        $okResponse = new Response('OK', Response::HTTP_OK);
-
-        try {
-            $this->registerTrustedDevice($notificationAddress, $okResponse);
-        } catch (Throwable $e) {
-            $logger->warning('Could not register trusted device on registration', ['exception' => $e]);
-        }
-
-        return $okResponse;
+        return new Response('OK', Response::HTTP_OK);
     }
 
     /** Handle login operation from the app, returns response for the app
@@ -325,12 +313,6 @@ private function loginAction(Request $request, string $notificationType, string
                     // Continue
                 }
 
-                try {
-                    $this->registerTrustedDevice($notificationAddress, $responseObject);
-                } catch (Throwable $e) {
-                    $this->logger->warning('Could not create trusted device cookie.', ['exception' => $e]);
-                }
-
                 return $responseObject;
             }
 
@@ -342,16 +324,4 @@ private function loginAction(Request $request, string $notificationType, string
 
         return new Response('AUTHENTICATION_FAILED', Response::HTTP_FORBIDDEN);
     }
-
-    private function registerTrustedDevice(
-        string $notificationAddress,
-        Response $responseObject
-    ): void {
-        if (trim($notificationAddress) !== '') {
-            $this->cookieService->registerTrustedDevice(
-                $responseObject,
-                $notificationAddress
-            );
-        }
-    }
 }

src/Features/Context/TiqrContext.php

@@ -24,7 +24,6 @@
 use Assert\AssertionFailedException;
 use Behat\Behat\Context\Context;
 use Behat\Behat\Hook\Scope\BeforeScenarioScope;
-use Behat\Behat\Tester\Exception\PendingException;
 use Behat\Gherkin\Node\TableNode;
 use Behat\Mink\Driver\BrowserKitDriver;
 use Behat\MinkExtension\Context\MinkContext;
@@ -33,6 +32,7 @@
 use OCRA;
 use RuntimeException;
 use stdClass;
+use Surfnet\GsspBundle\Service\ValueStore;
 use Surfnet\SamlBundle\Exception\NotFound;
 use Surfnet\Tiqr\Dev\FileLogger;
 use Surfnet\Tiqr\Service\TrustedDevice\Crypto\HaliteCryptoHelper;
@@ -86,6 +86,7 @@ public function __construct(
         private readonly FileLogger $fileLogger,
         private readonly KernelInterface $kernel,
         private readonly TrustedDeviceService $trustedDeviceService,
+        private readonly ValueStore $store,
     ) {
     }
 
@@ -99,6 +100,7 @@ public function gatherContexts(BeforeScenarioScope $scope): void
         $environment = $scope->getEnvironment();
         $this->minkContext = $environment->getContext(MinkContext::class);
         $this->minkContext->getSession()->setCookie('stepup_locale', 'en');
+        $this->store->clear();
     }
 
     /**
@@ -745,4 +747,26 @@ public function theTrustedDeviceCookieIsCleared(): void
     {
         $this->minkContext->getSession()->getDriver()->getClient()->getCookieJar()->expire('tiqr-trusted-device');
     }
+
+    /**
+     * @Then /^I should see the trusted device cookie$/
+     * @Then /^I should see the trusted device cookie for address "([^"]*)"$/
+     */
+    public function iShouldSeeTheCookie(?string $notificationAddress = null): void
+    {
+        $session = $this->minkContext->getMink()->getSession();
+        $trustedDeviceCookie = $session->getCookie('tiqr-trusted-device');
+
+        if (!is_string($trustedDeviceCookie) || trim($trustedDeviceCookie) === '') {
+            throw new RuntimeException('The expected trusted device cookie is not present');
+        }
+
+        if ($notificationAddress !== null) {
+            $request = new Request();
+            $request->cookies->set('tiqr-trusted-device', $trustedDeviceCookie);
+            $cookieValue = $this->trustedDeviceService->read($request);
+            Assertion::isInstanceOf($cookieValue, CookieValue::class);
+            Assertion::true($this->trustedDeviceService->isTrustedDevice($cookieValue, $notificationAddress));
+        }
+    }
 }

src/Features/Context/WebContext.php

@@ -22,6 +22,7 @@
 
 use Behat\Behat\Context\Context;
 use Behat\Behat\Hook\Scope\BeforeScenarioScope;
+use Behat\Behat\Tester\Exception\PendingException;
 use Behat\MinkExtension\Context\MinkContext;
 use Exception;
 use RobRichards\XMLSecLibs\XMLSecurityKey;

src/Features/authentication.feature

@@ -4,7 +4,7 @@ Feature: When an user needs to authenticate
 
   Background:
     Given the registration QR code is scanned
-    When the user registers the service
+    When the user registers the service with notification type "APNS" address: "0000000000111111111122222222223333333333"
     Then we have a registered user
     And I clear the logs
 
@@ -32,6 +32,7 @@ Feature: When an user needs to authenticate
 
     # Service provider AuthNRequest response page
     Then I should see "urn:oasis:names:tc:SAML:2.0:status:Success"
+    And I should see the trusted device cookie for address "0000000000111111111122222222223333333333"
 
     And the logs are:
       | level  | message                                                                                                                                   | sari    |
@@ -91,6 +92,7 @@ Feature: When an user needs to authenticate
       | info   | Authentication is finalized, returning to SP                                                                                              | present |
       | notice | Application authenticates the user                                                                                                        | present |
       | notice | Created redirect response for sso return endpoint "/saml/sso_return"                                                                      | present |
+      | notice | /Writing a trusted-device cookie with fingerprint .*/                                                                                     | present |
       | info   | User made a request with a session cookie.                                                                                                | present |
       | info   | Created new session.                                                                                                                      |         |
       | info   | User has a session.                                                                                                                       | present |
@@ -105,7 +107,7 @@ Feature: When an user needs to authenticate
       | info   | User session matches the session cookie.                                                                                                  |         |
       | info   | /SAMLResponse with id ".*" was not signed at root level, not attempting to verify the signature of the reponse itself/                    |         |
       | info   | /Verifying signature of Assertion with id ".*"/                                                                                           |         |
-
+      | notice | /Reading a trusted-device cookie with fingerprint .*/                                                                                     |         |
 
   Scenario: When an user cancels it's authentication
     # Service provider demo page
@@ -200,6 +202,7 @@ Feature: When an user needs to authenticate
 
     # Service provider AuthNRequest response page
     Then I should see "urn:oasis:names:tc:SAML:2.0:status:Success"
+    And I should see the trusted device cookie
 
     And the logs are:
       | level  | message                                                                                                                                   | sari    |
@@ -256,6 +259,7 @@ Feature: When an user needs to authenticate
       | info   | Authentication is finalized, returning to SP                                                                                              | present |
       | notice | Application authenticates the user                                                                                                        | present |
       | notice | Created redirect response for sso return endpoint "/saml/sso_return"                                                                      | present |
+      | notice | /Writing a trusted-device cookie with fingerprint .*/                                                                                     | present |
       | info   | User made a request with a session cookie.                                                                                                | present |
       | info   | Created new session.                                                                                                                      |         |
       | info   | User has a session.                                                                                                                       | present |
@@ -271,3 +275,4 @@ Feature: When an user needs to authenticate
       | info   | /SAMLResponse with id ".*" was not signed at root level, not attempting to verify the signature of the reponse itself/                    |         |
       | info   | /Verifying signature of Assertion with id ".*"/                                                                                           |         |
 
+

src/Features/mfaFatigueMitigation.feature

@@ -6,22 +6,9 @@ Feature: When an user needs to authenticate
     Given the registration QR code is scanned
     And the user registers the service with notification type "APNS" address: "0000000000111111111122222222223333333333"
     Then we have a registered user
-    And the logs should mention: Writing a trusted-device cookie with fingerprint
     And I clear the logs
     And the trusted device cookie is cleared
 
-  Scenario: When a user authenticates using a qr code it should set a trusted cookie
-    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"
-
-    Then I scan the tiqr authentication qrcode
-    And the app authenticates to the service with notification type "APNS" address: "0000000000111111111122222222223333333333"
-    Then we have a authenticated app
-    And we have a trusted cookie for address: "0000000000111111111122222222223333333333"
-
   Scenario: When a user authenticates without a trusted cookie, a push notification should not be sent
     Given I am on "/demo/sp"
     And I fill in "NameID" with my identifier