[Security] make TokenInterface::getUser() nullable to tell about unau…

…thenticated tokens
symfony · Aug 20, 2021 · d9cd41c · d9cd41c
1 parent 53215e2
commit d9cd41c
Show file tree

Hide file tree

Showing 18 changed files with 57 additions and 66 deletions.
diff --git a/UPGRADE-5.4.md b/UPGRADE-5.4.md
@@ -112,8 +112,8 @@ Security
    behavior when using `enable_authenticator_manager: true`)
  * Deprecate not setting the `$exceptionOnNoToken` argument of `AccessListener` to `false`
    (this is the default behavior when using `enable_authenticator_manager: true`)
- * Deprecate `TokenInterface:isAuthenticated()` and `setAuthenticated()` methods without replacement.
-   Security tokens won't have an "authenticated" flag anymore, so they will always be considered authenticated
+ * Deprecate `TokenInterface:isAuthenticated()` and `setAuthenticated()` methods,
+   return `null` from `getUser()` instead when a token is not authenticated
  * Deprecate `DeauthenticatedEvent`, use `TokenDeauthenticatedEvent` instead
  * Deprecate `CookieClearingLogoutHandler`, `SessionLogoutHandler` and `CsrfTokenClearingLogoutHandler`.
    Use `CookieClearingLogoutListener`, `SessionLogoutListener` and `CsrfTokenClearingLogoutListener` instead

diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md
@@ -359,8 +359,8 @@ Security
    `UsernamePasswordFormAuthenticationListener`, `UsernamePasswordJsonAuthenticationListener` and `X509AuthenticationListener`
    from security-http, use the new authenticator system instead
  * Remove the Guard component, use the new authenticator system instead
- * Remove `TokenInterface:isAuthenticated()` and `setAuthenticated()` methods without replacement.
-   Security tokens won't have an "authenticated" flag anymore, so they will always be considered authenticated
+ * Remove `TokenInterface:isAuthenticated()` and `setAuthenticated()`,
+   return `null` from `getUser()` instead when a token is not authenticated
  * Remove `DeauthenticatedEvent`, use `TokenDeauthenticatedEvent` instead
  * Remove `CookieClearingLogoutHandler`, `SessionLogoutHandler` and `CsrfTokenClearingLogoutHandler`.
    Use `CookieClearingLogoutListener`, `SessionLogoutListener` and `CsrfTokenClearingLogoutListener` instead

diff --git a/src/Symfony/Bridge/Monolog/Processor/AbstractTokenProcessor.php b/src/Symfony/Bridge/Monolog/Processor/AbstractTokenProcessor.php
@@ -42,7 +42,7 @@ public function __invoke(array $record): array
 
         if (null !== $token = $this->getToken()) {
             $record['extra'][$this->getKey()] = [
-                'authenticated' => method_exists($token, 'isAuthenticated') ? $token->isAuthenticated(false) : true, // @deprecated since Symfony 5.4, always true in 6.0
+                'authenticated' => method_exists($token, 'isAuthenticated') ? $token->isAuthenticated(false) : (bool) $token->getUser(),
                 'roles' => $token->getRoleNames(),
             ];
 

diff --git a/src/Symfony/Bridge/Twig/AppVariable.php b/src/Symfony/Bridge/Twig/AppVariable.php
@@ -16,6 +16,7 @@
 use Symfony\Component\HttpFoundation\Session\Session;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\User\UserInterface;
 
 /**
  * Exposes some Symfony parameters and services as an "app" global variable.
@@ -68,7 +69,7 @@ public function getToken()
     /**
      * Returns the current user.
      *
-     * @return object|null
+     * @return UserInterface|null
      *
      * @see TokenInterface::getUser()
      */
@@ -84,7 +85,7 @@ public function getUser()
 
         $user = $token->getUser();
 
-        // @deprecated since 5.4, $user will always be a UserInterface instance
+        // @deprecated since Symfony 5.4, $user will always be a UserInterface instance
         return \is_object($user) ? $user : null;
     }
 

diff --git a/src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php b/src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php
@@ -127,7 +127,7 @@ public function collect(Request $request, Response $response, \Throwable $except
 
             $this->data = [
                 'enabled' => true,
-                'authenticated' => method_exists($token, 'isAuthenticated') ? $token->isAuthenticated(false) : true,
+                'authenticated' => method_exists($token, 'isAuthenticated') ? $token->isAuthenticated(false) : (bool) $token->getUser(),
                 'impersonated' => null !== $impersonatorUser,
                 'impersonator_user' => $impersonatorUser,
                 'impersonation_exit_path' => null,

diff --git a/src/Symfony/Component/Security/Core/Authentication/AuthenticationProviderManager.php b/src/Symfony/Component/Security/Core/Authentication/AuthenticationProviderManager.php
@@ -111,7 +111,7 @@ public function authenticate(TokenInterface $token)
             }
 
             // @deprecated since Symfony 5.3
-            if ($user = $result->getUser() instanceof UserInterface && !method_exists($result->getUser(), 'getUserIdentifier')) {
+            if ($result->getUser() instanceof UserInterface && !method_exists($result->getUser(), 'getUserIdentifier')) {
                 trigger_deprecation('symfony/security-core', '5.3', 'Not implementing method "getUserIdentifier(): string" in user class "%s" is deprecated. This method will replace "getUsername()" in Symfony 6.0.', get_debug_type($result->getUser()));
             }
 

diff --git a/src/Symfony/Component/Security/Core/Authentication/AuthenticationTrustResolver.php b/src/Symfony/Component/Security/Core/Authentication/AuthenticationTrustResolver.php
@@ -12,7 +12,6 @@
 namespace Symfony\Component\Security\Core\Authentication;
 
 use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken;
-use Symfony\Component\Security\Core\Authentication\Token\NullToken;
 use Symfony\Component\Security\Core\Authentication\Token\RememberMeToken;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 
@@ -25,9 +24,9 @@ class AuthenticationTrustResolver implements AuthenticationTrustResolverInterfac
 {
     public function isAuthenticated(TokenInterface $token = null): bool
     {
-        return null !== $token && !$token instanceof NullToken
+        return $token && $token->getUser()
             // @deprecated since Symfony 5.4, TokenInterface::isAuthenticated() and AnonymousToken no longer exists in 6.0
-            && !$token instanceof AnonymousToken && $token->isAuthenticated(false);
+            && !$token instanceof AnonymousToken && (!method_exists($token, 'isAuthenticated') || $token->isAuthenticated(false));
     }
 
     /**
@@ -39,34 +38,22 @@ public function isAnonymous(TokenInterface $token = null/*, $deprecation = true*
             trigger_deprecation('symfony/security-core', '5.4', 'The "%s()" method is deprecated, use "isAuthenticated()" or "isFullFledged()" if you want to check if the request is (fully) authenticated.', __METHOD__);
         }
 
-        if (null === $token) {
-            return false;
-        }
-
-        return $token instanceof AnonymousToken || $token instanceof NullToken;
+        return $token && !$this->isAuthenticated($token);
     }
 
     /**
      * {@inheritdoc}
      */
     public function isRememberMe(TokenInterface $token = null)
     {
-        if (null === $token) {
-            return false;
-        }
-
-        return $token instanceof RememberMeToken;
+        return $token && $token instanceof RememberMeToken;
     }
 
     /**
      * {@inheritdoc}
      */
     public function isFullFledged(TokenInterface $token = null)
     {
-        if (null === $token) {
-            return false;
-        }
-
-        return !$this->isAnonymous($token, false) && !$this->isRememberMe($token);
+        return $token && !$this->isAnonymous($token, false) && !$this->isRememberMe($token);
     }
 }
diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php
@@ -141,7 +141,7 @@ public function setUser($user)
     public function isAuthenticated()
     {
         if (1 > \func_num_args() || func_get_arg(0)) {
-            trigger_deprecation('symfony/security-core', '5.4', 'Method "%s()" is deprecated. In version 6.0, security tokens won\'t have an "authenticated" flag anymore and will always be considered authenticated.', __METHOD__);
+            trigger_deprecation('symfony/security-core', '5.4', 'Method "%s()" is deprecated, return null from "getUser()" instead when a token is not authenticated.', __METHOD__);
         }
 
         return $this->authenticated;
@@ -153,7 +153,7 @@ public function isAuthenticated()
     public function setAuthenticated(bool $authenticated)
     {
         if (2 > \func_num_args() || func_get_arg(1)) {
-            trigger_deprecation('symfony/security-core', '5.4', 'Method "%s()" is deprecated. In version 6.0, security tokens won\'t have an "authenticated" state anymore and will always be considered as authenticated.', __METHOD__);
+            trigger_deprecation('symfony/security-core', '5.4', 'Method "%s()" is deprecated', __METHOD__);
         }
 
         $this->authenticated = $authenticated;

diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/NullToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/NullToken.php
@@ -59,7 +59,7 @@ public function getUserIdentifier(): string
     public function isAuthenticated()
     {
         if (0 === \func_num_args() || func_get_arg(0)) {
-            trigger_deprecation('symfony/security-core', '5.4', 'Method "%s()" is deprecated. In version 6.0, security tokens won\'t have an "authenticated" flag anymore and will always be considered authenticated.', __METHOD__);
+            trigger_deprecation('symfony/security-core', '5.4', 'Method "%s()" is deprecated, return null from "getUser()" instead when a token is not authenticated.', __METHOD__);
         }
 
         return true;

diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/TokenInterface.php b/src/Symfony/Component/Security/Core/Authentication/Token/TokenInterface.php
@@ -51,7 +51,7 @@ public function getCredentials();
     /**
      * Returns a user representation.
      *
-     * @return UserInterface
+     * @return UserInterface|null
      *
      * @see AbstractToken::setUser()
      */
@@ -71,14 +71,14 @@ public function setUser($user);
      *
      * @return bool true if the token has been authenticated, false otherwise
      *
-     * @deprecated since Symfony 5.4. In 6.0, security tokens will always be considered authenticated
+     * @deprecated since Symfony 5.4, return null from "getUser()" instead when a token is not authenticated
      */
     public function isAuthenticated();
 
     /**
      * Sets the authenticated flag.
      *
-     * @deprecated since Symfony 5.4. In 6.0, security tokens will always be considered authenticated
+     * @deprecated since Symfony 5.4
      */
     public function setAuthenticated(bool $isAuthenticated);
 

diff --git a/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php b/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php
@@ -67,7 +67,9 @@ public function __construct(TokenStorageInterface $tokenStorage, /*AccessDecisio
      */
     final public function isGranted($attribute, $subject = null): bool
     {
-        if (null === ($token = $this->tokenStorage->getToken())) {
+        $token = $this->tokenStorage->getToken();
+
+        if (!$token || !$token->getUser()) {
             if ($this->exceptionOnNoToken) {
                 throw new AuthenticationCredentialsNotFoundException('The token storage contains no authentication token. One possible reason may be that there is no firewall configured for this URL.');
             }
@@ -78,7 +80,7 @@ final public function isGranted($attribute, $subject = null): bool
             // @deprecated since Symfony 5.4
             if ($this->alwaysAuthenticate || !$authenticated = $token->isAuthenticated(false)) {
                 if (!($authenticated ?? true)) {
-                    trigger_deprecation('symfony/core', '5.4', 'Returning false from "%s()" is deprecated and won\'t have any effect in Symfony 6.0 as security tokens will always be considered authenticated.');
+                    trigger_deprecation('symfony/core', '5.4', 'Returning false from "%s()" is deprecated, return null from "getUser()" instead.');
                 }
                 $this->tokenStorage->setToken($token = $this->authenticationManager->authenticate($token));
             }

diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/AuthenticatedVoter.php b/src/Symfony/Component/Security/Core/Authorization/Voter/AuthenticatedVoter.php
@@ -12,7 +12,6 @@
 namespace Symfony\Component\Security\Core\Authorization\Voter;
 
 use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface;
-use Symfony\Component\Security\Core\Authentication\Token\NullToken;
 use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 
@@ -96,7 +95,7 @@ public function vote(TokenInterface $token, $subject, array $attributes)
             if (self::IS_AUTHENTICATED === $attribute
                 && (method_exists($this->authenticationTrustResolver, 'isAuthenticated')
                     ? $this->authenticationTrustResolver->isAuthenticated($token)
-                    : (null !== $token && !$token instanceof NullToken))) {
+                    : ($token && $token->getUser()))) {
                 return VoterInterface::ACCESS_GRANTED;
             }
 

diff --git a/src/Symfony/Component/Security/Core/CHANGELOG.md b/src/Symfony/Component/Security/Core/CHANGELOG.md
@@ -17,7 +17,7 @@ CHANGELOG
  * Deprecate setting the `$alwaysAuthenticate` argument to `true` and not setting the
    `$exceptionOnNoToken` argument to `false` of `AuthorizationChecker`
  * Deprecate methods `TokenInterface::isAuthenticated()` and `setAuthenticated`,
-   tokens will always be considered authenticated in 6.0
+   return null from "getUser()" instead when a token is not authenticated
 
 5.3
 ---

diff --git a/src/Symfony/Component/Security/Core/Security.php b/src/Symfony/Component/Security/Core/Security.php
@@ -42,12 +42,8 @@ public function getUser(): ?UserInterface
         }
 
         $user = $token->getUser();
-        // @deprecated since 5.4, $user will always be a UserInterface instance
-        if (!\is_object($user)) {
-            return null;
-        }
 
-        // @deprecated since 5.4, $user will always be a UserInterface instance
+        // @deprecated since Symfony 5.4, $user will always be a UserInterface instance
         if (!$user instanceof UserInterface) {
             return null;
         }

diff --git a/src/Symfony/Component/Security/Core/Tests/Authentication/AuthenticationTrustResolverTest.php b/src/Symfony/Component/Security/Core/Tests/Authentication/AuthenticationTrustResolverTest.php
@@ -17,7 +17,6 @@
 use Symfony\Component\Security\Core\Authentication\Token\RememberMeToken;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\User\InMemoryUser;
-use Symfony\Component\Security\Core\User\User;
 use Symfony\Component\Security\Core\User\UserInterface;
 
 class AuthenticationTrustResolverTest extends TestCase
@@ -29,7 +28,6 @@ public function testIsAnonymous()
     {
         $resolver = new AuthenticationTrustResolver();
         $this->assertFalse($resolver->isAnonymous(null));
-        $this->assertFalse($resolver->isAnonymous($this->getToken()));
         $this->assertFalse($resolver->isAnonymous($this->getRememberMeToken()));
         $this->assertFalse($resolver->isAnonymous(new FakeCustomToken()));
     }
@@ -39,7 +37,6 @@ public function testIsRememberMe()
         $resolver = new AuthenticationTrustResolver();
 
         $this->assertFalse($resolver->isRememberMe(null));
-        $this->assertFalse($resolver->isRememberMe($this->getToken()));
         $this->assertFalse($resolver->isRememberMe(new FakeCustomToken()));
         $this->assertTrue($resolver->isRememberMe(new RealCustomRememberMeToken()));
         $this->assertTrue($resolver->isRememberMe($this->getRememberMeToken()));
@@ -52,7 +49,6 @@ public function testisFullFledged()
         $this->assertFalse($resolver->isFullFledged(null));
         $this->assertFalse($resolver->isFullFledged($this->getRememberMeToken()));
         $this->assertFalse($resolver->isFullFledged(new RealCustomRememberMeToken()));
-        $this->assertTrue($resolver->isFullFledged($this->getToken()));
         $this->assertTrue($resolver->isFullFledged(new FakeCustomToken()));
     }
 
@@ -72,7 +68,7 @@ public function testIsAnonymousWithClassAsConstructorButStillExtending()
         $resolver = $this->getResolver();
 
         $this->assertFalse($resolver->isAnonymous(null));
-        $this->assertFalse($resolver->isAnonymous($this->getToken()));
+        $this->assertFalse($resolver->isAnonymous(new FakeCustomToken()));
         $this->assertFalse($resolver->isAnonymous($this->getRememberMeToken()));
     }
 
@@ -81,7 +77,7 @@ public function testIsRememberMeWithClassAsConstructorButStillExtending()
         $resolver = $this->getResolver();
 
         $this->assertFalse($resolver->isRememberMe(null));
-        $this->assertFalse($resolver->isRememberMe($this->getToken()));
+        $this->assertFalse($resolver->isRememberMe(new FakeCustomToken()));
         $this->assertTrue($resolver->isRememberMe($this->getRememberMeToken()));
         $this->assertTrue($resolver->isRememberMe(new RealCustomRememberMeToken()));
     }
@@ -93,7 +89,7 @@ public function testisFullFledgedWithClassAsConstructorButStillExtending()
         $this->assertFalse($resolver->isFullFledged(null));
         $this->assertFalse($resolver->isFullFledged($this->getRememberMeToken()));
         $this->assertFalse($resolver->isFullFledged(new RealCustomRememberMeToken()));
-        $this->assertTrue($resolver->isFullFledged($this->getToken()));
+        $this->assertTrue($resolver->isFullFledged(new FakeCustomToken()));
     }
 
     /**
@@ -112,11 +108,6 @@ public function testLegacy()
         $this->assertFalse($resolver->isFullFledged($this->getRealCustomAnonymousToken()));
     }
 
-    protected function getToken()
-    {
-        return $this->createMock(TokenInterface::class);
-    }
-
     protected function getAnonymousToken()
     {
         return new AnonymousToken('secret', 'anon.');
@@ -133,7 +124,7 @@ public function __construct()
 
     protected function getRememberMeToken()
     {
-        $user = class_exists(InMemoryUser::class) ? new InMemoryUser('wouter', '', ['ROLE_USER']) : new User('wouter', '', ['ROLE_USER']);
+        $user = new InMemoryUser('wouter', '', ['ROLE_USER']);
 
         return new RememberMeToken($user, 'main', 'secret');
     }
@@ -179,6 +170,7 @@ public function getCredentials()
 
     public function getUser(): UserInterface
     {
+        return new InMemoryUser('wouter', '', ['ROLE_USER']);
     }
 
     public function setUser($user)

diff --git a/src/Symfony/Component/Security/Core/Tests/Authorization/Voter/AuthenticatedVoterTest.php b/src/Symfony/Component/Security/Core/Tests/Authorization/Voter/AuthenticatedVoterTest.php
@@ -13,12 +13,13 @@
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolver;
-use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken;
+use Symfony\Component\Security\Core\Authentication\Token\AbstractToken;
+use Symfony\Component\Security\Core\Authentication\Token\NullToken;
 use Symfony\Component\Security\Core\Authentication\Token\RememberMeToken;
 use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
-use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter;
 use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
+use Symfony\Component\Security\Core\User\InMemoryUser;
 
 class AuthenticatedVoterTest extends TestCase
 {
@@ -84,14 +85,28 @@ public function getLegacyVoteTests()
 
     protected function getToken($authenticated)
     {
+        $user = new InMemoryUser('wouter', '', ['ROLE_USER']);
+
         if ('fully' === $authenticated) {
-            return $this->createMock(TokenInterface::class);
-        } elseif ('remembered' === $authenticated) {
-            return $this->getMockBuilder(RememberMeToken::class)->setMethods(['setPersistent'])->disableOriginalConstructor()->getMock();
-        } elseif ('impersonated' === $authenticated) {
+            $token = new class() extends AbstractToken {
+                public function getCredentials()
+                {
+                }
+            };
+            $token->setUser($user);
+            $token->setAuthenticated(true, false);
+
+            return $token;
+        }
+
+        if ('remembered' === $authenticated) {
+            return new RememberMeToken($user, 'foo', 'bar');
+        }
+
+        if ('impersonated' === $authenticated) {
             return $this->getMockBuilder(SwitchUserToken::class)->disableOriginalConstructor()->getMock();
-        } else {
-            return $this->getMockBuilder(AnonymousToken::class)->setConstructorArgs(['', ''])->getMock();
         }
+
+        return new NullToken();
     }
 }
diff --git a/src/Symfony/Component/Security/Http/EventListener/UserCheckerListener.php b/src/Symfony/Component/Security/Http/EventListener/UserCheckerListener.php
@@ -46,7 +46,6 @@ public function preCheckCredentials(CheckPassportEvent $event): void
     public function postCheckCredentials(AuthenticationSuccessEvent $event): void
     {
         $user = $event->getAuthenticationToken()->getUser();
-        // @deprecated since 5.4, $user will always be an UserInterface instance
         if (!$user instanceof UserInterface) {
             return;
         }