[Security] Improve DX of recent additions

Author: nicolas-grekas

src/Symfony/Bridge/Twig/Extension/SecurityExtension.php

@@ -31,7 +31,6 @@ final class SecurityExtension extends AbstractExtension
     public function __construct(
         private ?AuthorizationCheckerInterface $securityChecker = null,
         private ?ImpersonateUrlGenerator $impersonateUrlGenerator = null,
-        private ?UserAuthorizationCheckerInterface $userSecurityChecker = null,
     ) {
     }
 
@@ -58,8 +57,12 @@ public function isGranted(mixed $role, mixed $object = null, ?string $field = nu
 
     public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?string $field = null, ?AccessDecision $accessDecision = null): bool
     {
-        if (!$this->userSecurityChecker) {
-            throw new \LogicException(\sprintf('An instance of "%s" must be provided to use "%s()".', UserAuthorizationCheckerInterface::class, __METHOD__));
+        if (null === $this->securityChecker) {
+            return false;
+        }
+
+        if (!$this->securityChecker instanceof UserAuthorizationCheckerInterface) {
+            throw new \LogicException(\sprintf('You cannot use "%s()" if the authorization checker doesn\'t implement "%s".%s', __METHOD__, UserAuthorizationCheckerInterface::class, interface_exists(UserAuthorizationCheckerInterface::class) ? ' Try upgrading the "symfony/security-core" package to v7.3 minimum.' : ''));
         }
 
         if (null !== $field) {
@@ -71,7 +74,7 @@ public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $s
         }
 
         try {
-            return $this->userSecurityChecker->isGrantedForUser($user, $attribute, $subject, $accessDecision);
+            return $this->securityChecker->isGrantedForUser($user, $attribute, $subject, $accessDecision);
         } catch (AuthenticationCredentialsNotFoundException) {
             return false;
         }
@@ -123,7 +126,7 @@ public function getFunctions(): array
             new TwigFunction('impersonation_path', $this->getImpersonatePath(...)),
         ];
 
-        if ($this->userSecurityChecker) {
+        if ($this->securityChecker instanceof UserAuthorizationCheckerInterface) {
             $functions[] = new TwigFunction('is_granted_for_user', $this->isGrantedForUser(...));
         }
 

src/Symfony/Bridge/Twig/Tests/Extension/SecurityExtensionTest.php

@@ -15,12 +15,23 @@
 use Symfony\Bridge\PhpUnit\ClassExistsMock;
 use Symfony\Bridge\Twig\Extension\SecurityExtension;
 use Symfony\Component\Security\Acl\Voter\FieldVote;
+use Symfony\Component\Security\Core\Authorization\AccessDecision;
 use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
 use Symfony\Component\Security\Core\Authorization\UserAuthorizationCheckerInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 
 class SecurityExtensionTest extends TestCase
 {
+    public static function setUpBeforeClass(): void
+    {
+        ClassExistsMock::register(SecurityExtension::class);
+    }
+
+    protected function tearDown(): void
+    {
+        ClassExistsMock::withMockedClasses([FieldVote::class => true]);
+    }
+
     /**
      * @dataProvider provideObjectFieldAclCases
      */
@@ -39,17 +50,16 @@ public function testIsGrantedCreatesFieldVoteObjectWhenFieldNotNull($object, $fi
 
     public function testIsGrantedThrowsWhenFieldNotNullAndFieldVoteClassDoesNotExist()
     {
-        if (!class_exists(UserAuthorizationCheckerInterface::class)) {
+        if (!interface_exists(UserAuthorizationCheckerInterface::class)) {
             $this->markTestSkipped('This test requires symfony/security-core 7.3 or superior.');
         }
 
         $securityChecker = $this->createMock(AuthorizationCheckerInterface::class);
 
-        ClassExistsMock::register(SecurityExtension::class);
         ClassExistsMock::withMockedClasses([FieldVote::class => false]);
 
         $this->expectException(\LogicException::class);
-        $this->expectExceptionMessageMatches('Passing a $field to the "is_granted()" function requires symfony/acl.');
+        $this->expectExceptionMessage('Passing a $field to the "is_granted()" function requires symfony/acl.');
 
         $securityExtension = new SecurityExtension($securityChecker);
         $securityExtension->isGranted('ROLE', 'object', 'bar');
@@ -60,49 +70,74 @@ public function testIsGrantedThrowsWhenFieldNotNullAndFieldVoteClassDoesNotExist
      */
     public function testIsGrantedForUserCreatesFieldVoteObjectWhenFieldNotNull($object, $field, $expectedSubject)
     {
-        if (!class_exists(UserAuthorizationCheckerInterface::class)) {
+        if (!interface_exists(UserAuthorizationCheckerInterface::class)) {
             $this->markTestSkipped('This test requires symfony/security-core 7.3 or superior.');
         }
 
         $user = $this->createMock(UserInterface::class);
-        $userSecurityChecker = $this->createMock(UserAuthorizationCheckerInterface::class);
-        $userSecurityChecker
-            ->expects($this->once())
-            ->method('isGrantedForUser')
-            ->with($user, 'ROLE', $expectedSubject)
-            ->willReturn(true);
+        $securityChecker = $this->createMockAuthorizationChecker();
 
-        $securityExtension = new SecurityExtension(null, null, $userSecurityChecker);
+        $securityExtension = new SecurityExtension($securityChecker);
         $this->assertTrue($securityExtension->isGrantedForUser($user, 'ROLE', $object, $field));
+        $this->assertSame($user, $securityChecker->user);
+        $this->assertSame('ROLE', $securityChecker->attribute);
+
+        if (null === $field) {
+            $this->assertSame($object, $securityChecker->subject);
+        } else {
+            $this->assertEquals($expectedSubject, $securityChecker->subject);
+        }
+    }
+
+    public static function provideObjectFieldAclCases()
+    {
+        return [
+            [null, null, null],
+            ['object', null, 'object'],
+            ['object', false, new FieldVote('object', false)],
+            ['object', 0, new FieldVote('object', 0)],
+            ['object', '', new FieldVote('object', '')],
+            ['object', 'field', new FieldVote('object', 'field')],
+        ];
     }
 
     public function testIsGrantedForUserThrowsWhenFieldNotNullAndFieldVoteClassDoesNotExist()
     {
-        if (!class_exists(UserAuthorizationCheckerInterface::class)) {
+        if (!interface_exists(UserAuthorizationCheckerInterface::class)) {
             $this->markTestSkipped('This test requires symfony/security-core 7.3 or superior.');
         }
 
-        $securityChecker = $this->createMock(UserAuthorizationCheckerInterface::class);
+        $securityChecker = $this->createMockAuthorizationChecker();
 
-        ClassExistsMock::register(SecurityExtension::class);
         ClassExistsMock::withMockedClasses([FieldVote::class => false]);
 
         $this->expectException(\LogicException::class);
-        $this->expectExceptionMessageMatches('Passing a $field to the "is_granted_for_user()" function requires symfony/acl.');
+        $this->expectExceptionMessage('Passing a $field to the "is_granted_for_user()" function requires symfony/acl.');
 
-        $securityExtension = new SecurityExtension(null, null, $securityChecker);
-        $securityExtension->isGrantedForUser($this->createMock(UserInterface::class), 'object', 'bar');
+        $securityExtension = new SecurityExtension($securityChecker);
+        $securityExtension->isGrantedForUser($this->createMock(UserInterface::class), 'ROLE', 'object', 'bar');
     }
 
-    public static function provideObjectFieldAclCases()
+    private function createMockAuthorizationChecker(): AuthorizationCheckerInterface&UserAuthorizationCheckerInterface
     {
-        return [
-            [null, null, null],
-            ['object', null, 'object'],
-            ['object', false, new FieldVote('object', false)],
-            ['object', 0, new FieldVote('object', 0)],
-            ['object', '', new FieldVote('object', '')],
-            ['object', 'field', new FieldVote('object', 'field')],
-        ];
+        return new class implements AuthorizationCheckerInterface, UserAuthorizationCheckerInterface {
+            public UserInterface $user;
+            public mixed $attribute;
+            public mixed $subject;
+
+            public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
+            {
+                throw new \BadMethodCallException('This method should not be called.');
+            }
+
+            public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
+            {
+                $this->user = $user;
+                $this->attribute = $attribute;
+                $this->subject = $subject;
+
+                return true;
+            }
+        };
     }
 }

src/Symfony/Bundle/SecurityBundle/Resources/config/security.php

@@ -31,7 +31,6 @@
 use Symfony\Component\Security\Core\Authorization\AuthorizationChecker;
 use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
 use Symfony\Component\Security\Core\Authorization\ExpressionLanguage;
-use Symfony\Component\Security\Core\Authorization\UserAuthorizationChecker;
 use Symfony\Component\Security\Core\Authorization\UserAuthorizationCheckerInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter;
 use Symfony\Component\Security\Core\Authorization\Voter\ClosureVoter;
@@ -69,12 +68,7 @@
                 service('security.access.decision_manager'),
             ])
         ->alias(AuthorizationCheckerInterface::class, 'security.authorization_checker')
-
-        ->set('security.user_authorization_checker', UserAuthorizationChecker::class)
-            ->args([
-                service('security.access.decision_manager'),
-            ])
-        ->alias(UserAuthorizationCheckerInterface::class, 'security.user_authorization_checker')
+        ->alias(UserAuthorizationCheckerInterface::class, 'security.authorization_checker')
 
         ->set('security.token_storage', UsageTrackingTokenStorage::class)
             ->args([
@@ -94,7 +88,7 @@
                 service_locator([
                     'security.token_storage' => service('security.token_storage'),
                     'security.authorization_checker' => service('security.authorization_checker'),
-                    'security.user_authorization_checker' => service('security.user_authorization_checker'),
+                    'security.user_authorization_checker' => service('security.authorization_checker'),
                     'security.authenticator.managers_locator' => service('security.authenticator.managers_locator')->ignoreOnInvalid(),
                     'request_stack' => service('request_stack'),
                     'security.firewall.map' => service('security.firewall.map'),
@@ -174,8 +168,7 @@
 
         ->set('security.access.closure_voter', ClosureVoter::class)
             ->args([
-                service('security.access.decision_manager'),
-                service('security.authentication.trust_resolver'),
+                service('security.authorization_checker'),
             ])
             ->tag('security.voter', ['priority' => 245])
 

src/Symfony/Bundle/SecurityBundle/Resources/config/templating_twig.php

@@ -26,7 +26,6 @@
             ->args([
                 service('security.authorization_checker')->ignoreOnInvalid(),
                 service('security.impersonate_url_generator')->ignoreOnInvalid(),
-                service('security.user_authorization_checker')->ignoreOnInvalid(),
             ])
             ->tag('twig.extension')
     ;

src/Symfony/Bundle/SecurityBundle/Security.php

@@ -65,6 +65,17 @@ public function isGranted(mixed $attributes, mixed $subject = null, ?AccessDecis
             ->isGranted($attributes, $subject, $accessDecision);
     }
 
+    /**
+     * Checks if the attribute is granted against the user and optionally supplied subject.
+     *
+     * This should be used over isGranted() when checking permissions against a user that is not currently logged in or while in a CLI context.
+     */
+    public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
+    {
+        return $this->container->get('security.user_authorization_checker')
+            ->isGrantedForUser($user, $attribute, $subject, $accessDecision);
+    }
+
     public function getToken(): ?TokenInterface
     {
         return $this->container->get('security.token_storage')->getToken();
@@ -150,17 +161,6 @@ public function logout(bool $validateCsrfToken = true): ?Response
         return $logoutEvent->getResponse();
     }
 
-    /**
-     * Checks if the attribute is granted against the user and optionally supplied subject.
-     *
-     * This should be used over isGranted() when checking permissions against a user that is not currently logged in or while in a CLI context.
-     */
-    public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
-    {
-        return $this->container->get('security.user_authorization_checker')
-            ->isGrantedForUser($user, $attribute, $subject, $accessDecision);
-    }
-
     private function getAuthenticator(?string $authenticatorName, string $firewallName): AuthenticatorInterface
     {
         if (!isset($this->authenticators[$firewallName])) {

src/Symfony/Component/Security/Core/Authentication/Token/UserAuthorizationCheckerToken.php

@@ -11,8 +11,11 @@
 
 namespace Symfony\Component\Security\Core\Authorization;
 
+use Symfony\Component\Security\Core\Authentication\Token\AbstractToken;
 use Symfony\Component\Security\Core\Authentication\Token\NullToken;
+use Symfony\Component\Security\Core\Authentication\Token\OfflineTokenInterface;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
+use Symfony\Component\Security\Core\User\UserInterface;
 
 /**
  * AuthorizationChecker is the main authorization point of the Security component.
@@ -22,8 +25,9 @@
  * @author Fabien Potencier <[email protected]>
  * @author Johannes M. Schmitt <[email protected]>
  */
-class AuthorizationChecker implements AuthorizationCheckerInterface
+class AuthorizationChecker implements AuthorizationCheckerInterface, UserAuthorizationCheckerInterface
 {
+    private array $tokenStack = [];
     private array $accessDecisionStack = [];
 
     public function __construct(
@@ -34,7 +38,7 @@ public function __construct(
 
     final public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
     {
-        $token = $this->tokenStorage->getToken();
+        $token = end($this->tokenStack) ?: $this->tokenStorage->getToken();
 
         if (!$token || !$token->getUser()) {
             $token = new NullToken();
@@ -48,4 +52,17 @@ final public function isGranted(mixed $attribute, mixed $subject = null, ?Access
             array_pop($this->accessDecisionStack);
         }
     }
+
+    final public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
+    {
+        $token = new class($user->getRoles()) extends AbstractToken implements OfflineTokenInterface {};
+        $token->setUser($user);
+        $this->tokenStack[] = $token;
+
+        try {
+            return $this->isGranted($attribute, $subject, $accessDecision);
+        } finally {
+            array_pop($this->tokenStack);
+        }
+    }
 }