First idea's for better handling of timeouts and sending a session id…

…entifier along with the polls

src/Controller/AuthenticationController.php

@@ -73,7 +73,7 @@ public function authentication(Request $request): Response
 
         $logger->info('Verifying if there is a pending authentication request from SP');
 
-        // Do have a valid sample AuthnRequest?
+        // Do we have a valid GSSP authentication AuthnRequest in this session?
         if (!$this->authenticationService->authenticationRequired()) {
             $logger->error('There is no pending authentication request from SP');
 
@@ -146,12 +146,15 @@ public function authentication(Request $request): Response
         $logger->info('Return authentication page with QR code');
 
         return $this->render('default/authentication.html.twig', [
+            // TODO: Add something identifying the authentication session to the authenticateUrl
             'authenticateUrl' => $this->tiqrService->authenticationUrl()
         ]);
     }
 
     /**
      * @throws InvalidArgumentException
+     *
+     * Requires session cookie to be set to a valid session.
      */
     #[Route(path: '/authentication/status', name: 'app_identity_authentication_status', methods: ['GET'])]
     public function authenticationStatus(): JsonResponse
@@ -161,11 +164,15 @@ public function authenticationStatus(): JsonResponse
         $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]);
         $logger->info('Request for authentication status');
 
+        // Do we have a valid GSSP authentication AuthnRequest in this session?
         if (!$this->authenticationService->authenticationRequired()) {
             $logger->error('There is no pending authentication request from SP');
             return $this->refreshAuthenticationPage();
         }
 
+        // Check if the user is authenticated. This check will work if the user has completed the authentication
+        // within the last Tiqr_Service::LOGIN_EXPIRE seconds (1 hour).
+        // Uses our SessionId.
         $isAuthenticated = $this->tiqrService->isAuthenticated();
 
         if ($isAuthenticated) {
@@ -197,6 +204,13 @@ private function authenticationChallengeIsExpired(): bool
         // of this function.
         // Effectively this does a $this->_stateStorage->getValue(self::PREFIX_CHALLENGE . $sessionKey);
         // To check that the session key still exists in the Tiqr_Service's state storage
+
+        // TODO: Refactor this so it does not depend on the implementation of the TiqrService
+        //       i.e. we should keep track of authentication timeout ourselves
+        //       Uses this method creates noise in the logs because it logs errors because keys are not found
+        //       in the StateStorage. But this is not an error here, it is expected bacause expired keys
+        //       cannot be found in the statestorage by definition.
+        //       If you were truly interested in the authenticationUrl(), then logging the errors is correct.
         try {
             $this->tiqrService->authenticationUrl();
         } catch (Exception) {

src/Controller/RegistrationController.php

@@ -50,7 +50,7 @@ public function registration(Request $request): Response
     {
         $this->logger->info('Verifying if there is a pending registration from SP');
 
-        // Do have a valid sample AuthnRequest?.
+        // Do we have a valid GSSP registration AuthnRequest in this session?
         if (!$this->registrationService->registrationRequired()) {
             $this->logger->warning('Registration is not required');
             throw new NoActiveAuthenrequestException();
@@ -89,17 +89,22 @@ public function registration(Request $request): Response
      *
      *
      * @throws \InvalidArgumentException
+     *
+     * Requires session cookie to be set to a valid session.
      */
     #[Route(path: '/registration/status', name: 'app_identity_registration_status', methods: ['GET'])]
     public function registrationStatus() : Response
     {
         $this->logger->info('Request for registration status');
-        // Do have a valid sample AuthnRequest?.
+        // Do we have a valid GSSP registration AuthnRequest in this session?
         if (!$this->registrationService->registrationRequired()) {
             $this->logger->error('There is no pending registration request');
 
             return new Response('No registration required', Response::HTTP_BAD_REQUEST);
         }
+
+        // TODO: Check whether enrollment is expired here?
+
         $status = $this->tiqrService->getEnrollmentStatus();
         $this->logger->info(sprintf('Send json response status "%s"', $status));
         return new Response((string) $this->tiqrService->getEnrollmentStatus());
@@ -118,6 +123,7 @@ public function registrationQr(Request $request, string $enrollmentKey): Respons
     {
         $this->logger->info('Request for registration QR img');
 
+        // Do we have a valid GSSP registration AuthnRequest in this session?
         if (!$this->registrationService->registrationRequired()) {
             $this->logger->error('There is no pending registration request');
 

src/Tiqr/Legacy/TiqrService.php

@@ -95,16 +95,22 @@ public function generateEnrollmentKey(string $sari): string
     {
         $this->initSession();
 
-        $this->logger->debug('Generating userId');
+        // We use a randomly generated user ID
+        $this->logger->debug('Generating tiqr userId');
         $userId = $this->generateId();
-        $this->logger->debug('Storing the userId to session state');
+        $this->logger->debug('Storing the userId=' . $userId . ' to session state');
         $this->session->set('userId', $userId);
+
+        // The session ID is used to link the tiqr library's enrollment session to the user's browser session
         $sessionId = $this->session->getId();
         $this->logger->debug('Clearing the previous enrollment state(s)');
 
         try {
             $this->clearPreviousEnrollmentState();
-            $this->logger->debug('Starting the new enrollment session');
+            $this->logger->notice('Starting new enrollment session with sessionId ' . $sessionId .
+                ' and userId ' . $userId);
+            // accountName is the display name of the account that is shown in the tiqr app
+            // However, we set it to the name of the tiqr service.
             $enrollmentKey = $this->tiqrService->startEnrollmentSession($userId, $this->accountName, $sessionId);
             $this->logger->debug('Storing the enrollment key for future reference');
             $this->storeEnrollmentKey($enrollmentKey);
@@ -373,12 +379,10 @@ public function getSariForSessionIdentifier(string $identifier): string
      * (3 for enrollment, 1 for authentication). This allows us to correlate the actions of the
      * user's browser with those of the user's phone.
      *
-     * Because the enrollment identifier are sensitive two measures are implemented:
+     * Because the enrollment identifier is sensitive two measures are implemented:
      * - Only the first 8 characters of the identifiers are logged
      * - The identifiers are stored as a stable hash of the identifier to hide the rest of the identifier.
      *
-     * Note that tiqr currently stores these identifiers in plain in its state storage
-     *
      * @param string $identifier Session identifier: enrollment key or session key
      * @param string $sari       The to associate with $identifier
      * @throws TiqrServerRuntimeException
@@ -392,7 +396,7 @@ private function setSariForSessionIdentifier(string $identifier, string $sari):
             $hashedIdentifier = $this->getHashedIdentifier($identifier);
             $this->tiqrStateStorage->setValue('sari_' . $hashedIdentifier, $sari, 60 * 60);
         } catch (Exception $e) {
-            // Catch errors from the tiqr-server and up-cycle them to  exceptions that are meaningful to our domain
+            // Catch errors from the tiqr-server and up-cycle them to exceptions that are meaningful to our domain
             throw TiqrServerRuntimeException::fromOriginalException($e);
         }
     }
@@ -437,4 +441,14 @@ public function stateStorageHealthCheck(): HealthCheckResultDto
 
         return HealthCheckResultDto::fromHealthCheckInterface($this->tiqrStateStorage);
     }
+
+    protected function getAuthenticationTimeout(): int
+    {
+        return Tiqr_Service::CHALLENGE_EXPIRE;
+    }
+
+    protected function getEnrollmentTimeout(): int
+    {
+        return Tiqr_Service::ENROLLMENT_EXPIRE;
+    }
 }