Additional changes to address lazy firewalls and the missing token wh…

…en the supports method is called
scheb · Dec 2, 2020 · 5149d40 · 5149d40
1 parent a22a41a
commit 5149d40
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 47 deletions.
diff --git a/src/bundle/Security/Http/Authenticator/TwoFactorAuthenticator.php b/src/bundle/Security/Http/Authenticator/TwoFactorAuthenticator.php
@@ -20,6 +20,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
+use Symfony\Component\Security\Core\Exception\AuthenticationServiceException;
 use Symfony\Component\Security\Http\Authentication\AuthenticationFailureHandlerInterface;
 use Symfony\Component\Security\Http\Authentication\AuthenticationSuccessHandlerInterface;
 use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface;
@@ -87,19 +88,18 @@ public function __construct(
 
     public function supports(Request $request): ?bool
     {
-        if (!$this->twoFactorFirewallConfig->isCheckPathRequest($request)) {
-            return false;
-        }
-
-        $currentToken = $this->tokenStorage->getToken();
-
-        return $currentToken instanceof TwoFactorTokenInterface;
+        return $this->twoFactorFirewallConfig->isCheckPathRequest($request);
     }
 
     public function authenticate(Request $request): PassportInterface
     {
-        /** @var TwoFactorTokenInterface $currentToken */
+        // When the firewall is lazy, the token is not initialized in the "supports" stage, so this check does only work
+        // within the "authenticate" stage.
         $currentToken = $this->tokenStorage->getToken();
+        if (!($currentToken instanceof TwoFactorTokenInterface)) {
+            // This should only happen when the check path is called outside of a 2fa process and not protected via access_control
+            throw new AuthenticationServiceException('Tried to perform two-factor authentication, but two-factor authentication is not in progress.');
+        }
 
         $this->dispatchTwoFactorAuthenticationEvent(TwoFactorAuthenticationEvents::ATTEMPT, $request, $currentToken);
 

diff --git a/src/bundle/Security/Http/Firewall/TwoFactorListener.php b/src/bundle/Security/Http/Firewall/TwoFactorListener.php
@@ -21,6 +21,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
+use Symfony\Component\Security\Core\Exception\AuthenticationServiceException;
 use Symfony\Component\Security\Core\Exception\InvalidCsrfTokenException;
 use Symfony\Component\Security\Csrf\CsrfToken;
 use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
@@ -113,17 +114,20 @@ public function __construct(
 
     public function supports(Request $request): ?bool
     {
-        $token = $this->tokenStorage->getToken();
-
-        return $token instanceof TwoFactorTokenInterface
-            && $token->getProviderKey() === $this->twoFactorFirewallConfig->getFirewallName()
-            && $this->twoFactorFirewallConfig->isCheckPathRequest($request);
+        return $this->twoFactorFirewallConfig->isCheckPathRequest($request);
     }
 
     public function authenticate(RequestEvent $event): void
     {
-        /** @var TwoFactorTokenInterface $token */
+        // When the firewall is lazy, the token is not initialized in the "supports" stage, so this check does only work
+        // within the "authenticate" stage.
         $token = $this->tokenStorage->getToken();
+        if (!($token instanceof TwoFactorTokenInterface) || $token->getProviderKey() !== $this->twoFactorFirewallConfig->getFirewallName()) {
+            // This should only happen when the check path is called outside of a 2fa process and not protected via access_control
+            // or when the firewall is configured in an odd way (different firewall name)
+            throw new AuthenticationServiceException('Tried to perform two-factor authentication, but two-factor authentication is not in progress.');
+        }
+
         $response = $this->attemptAuthentication($event->getRequest(), $token);
         $event->setResponse($response);
     }

diff --git a/tests/Security/Http/Authenticator/TwoFactorAuthenticatorTest.php b/tests/Security/Http/Authenticator/TwoFactorAuthenticatorTest.php
@@ -22,6 +22,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
+use Symfony\Component\Security\Core\Exception\AuthenticationServiceException;
 use Symfony\Component\Security\Http\Authentication\AuthenticationFailureHandlerInterface;
 use Symfony\Component\Security\Http\Authentication\AuthenticationSuccessHandlerInterface;
 use Symfony\Component\Security\Http\Authenticator\Passport\Badge\CsrfTokenBadge;
@@ -216,37 +217,29 @@ private function expect2faCompleteFlagSet(MockObject $authenticatedToken): void
     /**
      * @test
      */
-    public function supports_notTwoFactorToken_returnFalse(): void
+    public function supports_notCheckPath_returnFalse(): void
     {
-        $this->stubIsCheckPath(true);
-        $this->stubTokenStorageHasToken($this->createMock(TokenInterface::class));
-
-        $returnValue = $this->authenticator->supports($this->request);
-        $this->assertFalse($returnValue);
+        $this->stubIsCheckPath(false);
+        $this->assertFalse($this->authenticator->supports($this->request));
     }
 
     /**
      * @test
      */
-    public function supports_notCheckPath_returnFalse(): void
+    public function supports_isCheckPath_returnTrue(): void
     {
-        $this->stubIsCheckPath(false);
-        $this->stubTokenStorageHasTwoFactorToken();
-
-        $returnValue = $this->authenticator->supports($this->request);
-        $this->assertFalse($returnValue);
+        $this->stubIsCheckPath(true);
+        $this->assertTrue($this->authenticator->supports($this->request));
     }
 
     /**
      * @test
      */
-    public function supports_isCheckPathAndTwoFactorToken_returnTrue(): void
+    public function authenticate_notTwoFactorToken_throwAuthenticationServiceException(): void
     {
-        $this->stubIsCheckPath(true);
-        $this->stubTokenStorageHasTwoFactorToken();
-
-        $returnValue = $this->authenticator->supports($this->request);
-        $this->assertTrue($returnValue);
+        $this->stubTokenStorageHasToken($this->createMock(TokenInterface::class));
+        $this->expectException(AuthenticationServiceException::class);
+        $this->authenticator->authenticate($this->request);
     }
 
     /**

diff --git a/tests/Security/Http/Firewall/TwoFactorListenerTest.php b/tests/Security/Http/Firewall/TwoFactorListenerTest.php
@@ -26,6 +26,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
+use Symfony\Component\Security\Core\Exception\AuthenticationServiceException;
 use Symfony\Component\Security\Csrf\CsrfToken;
 use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
 use Symfony\Component\Security\Http\Authentication\AuthenticationFailureHandlerInterface;
@@ -308,45 +309,45 @@ private function assertEventsDispatched(array $eventTypes): void
     /**
      * @test
      */
-    public function supports_noTwoFactorToken_returnFalse(): void
+    public function supports_notCheckPath_returnFalse(): void
     {
-        $this->stubTokenStorageHasToken($this->createMock(TokenInterface::class));
-        $this->stubRequestIsCheckPath(true);
-
+        $this->stubRequestIsCheckPath(false);
         $this->assertFalse($this->listener->supports($this->request));
     }
 
     /**
      * @test
      */
-    public function supports_differentFirewallName_returnFalse(): void
+    public function supports_isCheckPath_returnTrue(): void
     {
-        $this->stubTokenStorageHasToken($this->createTwoFactorToken('otherFirewallName'));
         $this->stubRequestIsCheckPath(true);
-
-        $this->assertFalse($this->listener->supports($this->request));
+        $this->assertTrue($this->listener->supports($this->request));
     }
 
     /**
      * @test
      */
-    public function supports_notCheckPath_returnFalse(): void
+    public function authenticate_noTwoFactorToken_throwAuthenticationServiceException(): void
     {
-        $this->stubTokenStorageHasToken($this->createTwoFactorToken());
-        $this->stubRequestIsCheckPath(false);
+        $this->stubTokenStorageHasToken($this->createMock(TokenInterface::class));
+        $this->stubRequestIsCheckPath(true);
 
-        $this->assertFalse($this->listener->supports($this->request));
+        $this->expectException(AuthenticationServiceException::class);
+
+        $this->listener->authenticate($this->requestEvent);
     }
 
     /**
      * @test
      */
-    public function supports_requirementsFulfilled_returnTrue(): void
+    public function authenticate_differentFirewallName_throwAuthenticationServiceException(): void
     {
-        $this->stubTokenStorageHasToken($this->createTwoFactorToken());
+        $this->stubTokenStorageHasToken($this->createTwoFactorToken('otherFirewallName'));
         $this->stubRequestIsCheckPath(true);
 
-        $this->assertTrue($this->listener->supports($this->request));
+        $this->expectException(AuthenticationServiceException::class);
+
+        $this->listener->authenticate($this->requestEvent);
     }
 
     /**