Ensure session is saved and closed when preparation is executed #257

Author: scheb

Security/TwoFactor/Provider/TwoFactorProviderPreparationListener.php

@@ -103,20 +103,22 @@ public function onKernelFinishRequest(FinishRequestEvent $event): void
         $providerName = $this->twoFactorToken->getCurrentTwoFactorProvider();
         $firewallName = $this->twoFactorToken->getProviderKey();
 
-        if ($this->preparationRecorder->isProviderPrepared($firewallName, $providerName)) {
+        try {
+            if ($this->preparationRecorder->isProviderPrepared($firewallName, $providerName)) {
+                if ($this->logger) {
+                    $this->logger->info(sprintf('Two-factor provider "%s" was already prepared.', $providerName));
+                }
+
+                return;
+            }
+            $user = $this->twoFactorToken->getUser();
+            $this->providerRegistry->getProvider($providerName)->prepareAuthentication($user);
+            $this->preparationRecorder->recordProviderIsPrepared($firewallName, $providerName);
             if ($this->logger) {
-                $this->logger->info(sprintf('Two-factor provider "%s" was already prepared.', $providerName));
+                $this->logger->info(sprintf('Two-factor provider "%s" prepared.', $providerName));
             }
-
-            return;
-        }
-
-        $user = $this->twoFactorToken->getUser();
-        $this->providerRegistry->getProvider($providerName)->prepareAuthentication($user);
-        $this->preparationRecorder->recordProviderIsPrepared($firewallName, $providerName);
-
-        if ($this->logger) {
-            $this->logger->info(sprintf('Two-factor provider "%s" prepared.', $providerName));
+        } finally {
+            $this->preparationRecorder->saveSession();
         }
     }
 

Security/TwoFactor/Provider/TwoFactorProviderPreparationRecorder.php

@@ -37,4 +37,11 @@ public function recordProviderIsPrepared(string $firewallName, string $providerN
         $calledProviders[$firewallName][] = $providerName;
         $this->session->set(self::CALLED_PROVIDERS_SESSION_KEY, $calledProviders);
     }
+
+    public function saveSession(): void
+    {
+        if ($this->session->isStarted()) {
+            $this->session->save();
+        }
+    }
 }

Tests/Security/TwoFactor/Provider/TwoFactorProviderPreparationListenerTest.php

@@ -131,6 +131,10 @@ private function expectPrepareCurrentProvider(): void
             ->expects($this->once())
             ->method('prepareAuthentication')
             ->with($this->identicalTo($this->user));
+
+        $this->preparationRecorder
+            ->expects($this->once())
+            ->method('saveSession');
     }
 
     private function expectNotPrepareCurrentProvider(): void
@@ -217,7 +221,7 @@ public function onTwoFactorForm_onEvent_twoFactorProviderIsPrepared(): void
     /**
      * @test
      */
-    public function onKernelFinishRequest_providerAlreadyPrepared_doNothing(): void
+    public function onKernelFinishRequest_providerAlreadyPrepared_saveSession(): void
     {
         $this->initTwoFactorProviderPreparationListener(true, true);
         $event = $this->createTwoFactorAuthenticationEvent();
@@ -228,6 +232,10 @@ public function onKernelFinishRequest_providerAlreadyPrepared_doNothing(): void
             ->with(self::FIREWALL_NAME, self::CURRENT_PROVIDER_NAME)
             ->willReturn(true);
 
+        $this->preparationRecorder
+            ->expects($this->once())
+            ->method('saveSession');
+
         $this->preparationRecorder
             ->expects($this->never())
             ->method('recordProviderIsPrepared');

Tests/Security/TwoFactor/Provider/TwoFactorProviderPreparationRecorderTest.php

@@ -84,4 +84,38 @@ public function onTwoFactorAuthenticationRequest_authenticationRequired_alreadyP
 
         $this->recorder->recordProviderIsPrepared(self::FIREWALL_NAME, self::CURRENT_PROVIDER_NAME);
     }
+
+    /**
+     * @test
+     */
+    public function saveSession_sessionIsStarted_save(): void
+    {
+        $this->session
+            ->expects($this->any())
+            ->method('isStarted')
+            ->willReturn(true);
+
+        $this->session
+            ->expects($this->once())
+            ->method('save');
+
+        $this->recorder->saveSession();
+    }
+
+    /**
+     * @test
+     */
+    public function saveSession_sessionNotStarted_doNotSave(): void
+    {
+        $this->session
+            ->expects($this->any())
+            ->method('isStarted')
+            ->willReturn(false);
+
+        $this->session
+            ->expects($this->never())
+            ->method('save');
+
+        $this->recorder->saveSession();
+    }
 }