From f5cee77e7faa90f493006f92f8a9ce4236f33709 Mon Sep 17 00:00:00 2001 From: HypeMC Date: Tue, 2 Aug 2022 10:26:19 +0200 Subject: [PATCH] [Security] Allow using expressions with the #[IsGranted] attribute --- .../AddExpressionLanguageProvidersPass.php | 1 + .../DependencyInjection/SecurityExtension.php | 1 + .../Resources/config/security.php | 13 ++- .../Security/Http/Attribute/IsGranted.php | 8 +- .../Component/Security/Http/CHANGELOG.md | 1 + .../IsGrantedAttributeListener.php | 47 +++++--- .../IsGrantedAttributeListenerTest.php | 108 ++++++++++++++++-- .../IsGrantedAttributeMethodsController.php | 19 +++ .../Component/Security/Http/composer.json | 3 +- 9 files changed, 172 insertions(+), 29 deletions(-) diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddExpressionLanguageProvidersPass.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddExpressionLanguageProvidersPass.php index 0393e6c616b79..3109cf27c5ab1 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddExpressionLanguageProvidersPass.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddExpressionLanguageProvidersPass.php @@ -36,6 +36,7 @@ public function process(ContainerBuilder $container) if (!$container->hasDefinition('cache.system')) { $container->removeDefinition('cache.security_expression_language'); + $container->removeDefinition('cache.security_is_granted_attribute_expression_language'); } } } diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php index b82046b15cdf8..9f46f23bab0c0 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php @@ -112,6 +112,7 @@ public function load(array $configs, ContainerBuilder $container) if (!$container::willBeAvailable('symfony/expression-language', ExpressionLanguage::class, ['symfony/security-bundle'])) { $container->removeDefinition('security.expression_language'); $container->removeDefinition('security.access.expression_voter'); + $container->removeDefinition('security.is_granted_attribute_expression_language'); } // set some global scalars diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security.php b/src/Symfony/Bundle/SecurityBundle/Resources/config/security.php index 781c782d4ccf7..024fed6f7d3b0 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security.php +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security.php @@ -18,6 +18,7 @@ use Symfony\Bundle\SecurityBundle\Security\FirewallMap; use Symfony\Bundle\SecurityBundle\Security\LazyFirewallContext; use Symfony\Bundle\SecurityBundle\Security\Security; +use Symfony\Component\ExpressionLanguage\ExpressionLanguage as BaseExpressionLanguage; use Symfony\Component\Ldap\Security\LdapUserProvider; use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolver; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; @@ -275,7 +276,17 @@ ->tag('kernel.cache_warmer') ->set('controller.is_granted_attribute_listener', IsGrantedAttributeListener::class) - ->args([service('security.authorization_checker')]) + ->args([ + service('security.authorization_checker'), + service('security.is_granted_attribute_expression_language')->nullOnInvalid(), + ]) ->tag('kernel.event_subscriber') + + ->set('security.is_granted_attribute_expression_language', BaseExpressionLanguage::class) + ->args([service('cache.security_is_granted_attribute_expression_language')->nullOnInvalid()]) + + ->set('cache.security_is_granted_attribute_expression_language') + ->parent('cache.system') + ->tag('cache.pool') ; }; diff --git a/src/Symfony/Component/Security/Http/Attribute/IsGranted.php b/src/Symfony/Component/Security/Http/Attribute/IsGranted.php index 787468a359cdc..c8066a256971d 100644 --- a/src/Symfony/Component/Security/Http/Attribute/IsGranted.php +++ b/src/Symfony/Component/Security/Http/Attribute/IsGranted.php @@ -11,6 +11,8 @@ namespace Symfony\Component\Security\Http\Attribute; +use Symfony\Component\ExpressionLanguage\Expression; + /** * @author Ryan Weaver */ @@ -21,12 +23,14 @@ public function __construct( /** * Sets the first argument that will be passed to isGranted(). */ - public string $attribute, + public string|Expression $attribute, /** * Sets the second argument passed to isGranted(). + * + * @var array|string|Expression|null */ - public array|string|null $subject = null, + public array|string|Expression|null $subject = null, /** * The message of the exception - has a nice default if not set. diff --git a/src/Symfony/Component/Security/Http/CHANGELOG.md b/src/Symfony/Component/Security/Http/CHANGELOG.md index e0b4bda2525ce..268baf2446bc6 100644 --- a/src/Symfony/Component/Security/Http/CHANGELOG.md +++ b/src/Symfony/Component/Security/Http/CHANGELOG.md @@ -9,6 +9,7 @@ CHANGELOG * Deprecate empty username or password when using when using `JsonLoginAuthenticator` * Set custom lifetime for login link * Add `$lifetime` parameter to `LoginLinkHandlerInterface::createLoginLink()` + * Allow using expressions as `#[IsGranted()]` attribute and subject 6.0 --- diff --git a/src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php b/src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php index dea889a3d81b3..a0400a032d46a 100644 --- a/src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php +++ b/src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php @@ -12,6 +12,8 @@ namespace Symfony\Component\Security\Http\EventListener; use Symfony\Component\EventDispatcher\EventSubscriberInterface; +use Symfony\Component\ExpressionLanguage\Expression; +use Symfony\Component\ExpressionLanguage\ExpressionLanguage; use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent; use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\HttpKernel\KernelEvents; @@ -28,7 +30,8 @@ class IsGrantedAttributeListener implements EventSubscriberInterface { public function __construct( - private AuthorizationCheckerInterface $authChecker, + private readonly AuthorizationCheckerInterface $authChecker, + private ?ExpressionLanguage $expressionLanguage = null, ) { } @@ -42,21 +45,15 @@ public function onKernelControllerArguments(ControllerArgumentsEvent $event) $arguments = $event->getNamedArguments(); foreach ($attributes as $attribute) { - $subjectRef = $attribute->subject; $subject = null; - if ($subjectRef) { + if ($subjectRef = $attribute->subject) { if (\is_array($subjectRef)) { - foreach ($subjectRef as $ref) { - if (!\array_key_exists($ref, $arguments)) { - throw new RuntimeException(sprintf('Could not find the subject "%s" for the #[IsGranted] attribute. Try adding a "$%s" argument to your controller method.', $ref, $ref)); - } - $subject[$ref] = $arguments[$ref]; + foreach ($subjectRef as $refKey => $ref) { + $subject[\is_string($refKey) ? $refKey : (string) $ref] = $this->getIsGrantedSubject($ref, $arguments); } - } elseif (!\array_key_exists($subjectRef, $arguments)) { - throw new RuntimeException(sprintf('Could not find the subject "%s" for the #[IsGranted] attribute. Try adding a "$%s" argument to your controller method.', $subjectRef, $subjectRef)); } else { - $subject = $arguments[$subjectRef]; + $subject = $this->getIsGrantedSubject($subjectRef, $arguments); } } @@ -81,15 +78,37 @@ public static function getSubscribedEvents(): array return [KernelEvents::CONTROLLER_ARGUMENTS => ['onKernelControllerArguments', 10]]; } + private function getIsGrantedSubject(string|Expression $subjectRef, array $arguments): mixed + { + if ($subjectRef instanceof Expression) { + $this->expressionLanguage ??= new ExpressionLanguage(); + + return $this->expressionLanguage->evaluate($subjectRef, [ + 'args' => $arguments, + ]); + } + + if (!\array_key_exists($subjectRef, $arguments)) { + throw new RuntimeException(sprintf('Could not find the subject "%s" for the #[IsGranted] attribute. Try adding a "$%s" argument to your controller method.', $subjectRef, $subjectRef)); + } + + return $arguments[$subjectRef]; + } + private function getIsGrantedString(IsGranted $isGranted): string { - $processValue = fn ($value) => sprintf('"%s"', $value); + $processValue = fn ($value) => sprintf($value instanceof Expression ? 'new Expression("%s")' : '"%s"', $value); $argsString = $processValue($isGranted->attribute); if (null !== $subject = $isGranted->subject) { - $subject = array_map($processValue, (array) $subject); - $argsString .= ', '.(1 === \count($subject) ? reset($subject) : '['.implode(', ', $subject).']'); + $subject = !\is_array($subject) ? $processValue($subject) : array_map(function ($key, $value) use ($processValue) { + $value = $processValue($value); + + return \is_string($key) ? sprintf('"%s" => %s', $key, $value) : $value; + }, array_keys($subject), $subject); + + $argsString .= ', '.(!\is_array($subject) ? $subject : '['.implode(', ', $subject).']'); } return $argsString; diff --git a/src/Symfony/Component/Security/Http/Tests/EventListener/IsGrantedAttributeListenerTest.php b/src/Symfony/Component/Security/Http/Tests/EventListener/IsGrantedAttributeListenerTest.php index 8e5a9470efe3e..8e165019a8ec4 100644 --- a/src/Symfony/Component/Security/Http/Tests/EventListener/IsGrantedAttributeListenerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/EventListener/IsGrantedAttributeListenerTest.php @@ -12,11 +12,13 @@ namespace Symfony\Component\Security\Http\Tests\EventListener; use PHPUnit\Framework\TestCase; +use Symfony\Component\ExpressionLanguage\Expression; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent; use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Authorization\ExpressionLanguage; use Symfony\Component\Security\Core\Exception\AccessDeniedException; use Symfony\Component\Security\Http\EventListener\IsGrantedAttributeListener; use Symfony\Component\Security\Http\Tests\Fixtures\IsGrantedAttributeController; @@ -42,7 +44,7 @@ public function testAttribute() $listener = new IsGrantedAttributeListener($authChecker); $listener->onKernelControllerArguments($event); - $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); $authChecker->expects($this->once()) ->method('isGranted') ->willReturn(true); @@ -61,7 +63,7 @@ public function testAttribute() public function testNothingHappensWithNoConfig() { - $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); $authChecker->expects($this->never()) ->method('isGranted'); @@ -79,7 +81,7 @@ public function testNothingHappensWithNoConfig() public function testIsGrantedCalledCorrectly() { - $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); $authChecker->expects($this->once()) ->method('isGranted') ->with('ROLE_ADMIN') @@ -99,7 +101,7 @@ public function testIsGrantedCalledCorrectly() public function testIsGrantedSubjectFromArguments() { - $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); $authChecker->expects($this->once()) ->method('isGranted') // the subject => arg2name will eventually resolve to the 2nd argument, which has this value @@ -146,7 +148,7 @@ public function testIsGrantedSubjectFromArgumentsWithArray() public function testIsGrantedNullSubjectFromArguments() { - $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); $authChecker->expects($this->once()) ->method('isGranted') ->with('ROLE_ADMIN', null) @@ -166,7 +168,7 @@ public function testIsGrantedNullSubjectFromArguments() public function testIsGrantedArrayWithNullValueSubjectFromArguments() { - $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); $authChecker->expects($this->once()) ->method('isGranted') ->with('ROLE_ADMIN', [ @@ -191,7 +193,7 @@ public function testExceptionWhenMissingSubjectAttribute() { $this->expectException(\RuntimeException::class); - $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), @@ -208,17 +210,22 @@ public function testExceptionWhenMissingSubjectAttribute() /** * @dataProvider getAccessDeniedMessageTests */ - public function testAccessDeniedMessages(string $attribute, string|array|null $subject, string $method, int $numOfArguments, string $expectedMessage) + public function testAccessDeniedMessages(string|Expression $attribute, string|array|null $subject, string $method, int $numOfArguments, string $expectedMessage) { - $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); $authChecker->expects($this->any()) ->method('isGranted') ->willReturn(false); + $expressionLanguage = $this->createMock(ExpressionLanguage::class); + $expressionLanguage->expects($this->any()) + ->method('evaluate') + ->willReturn('bar'); + // avoid the error of the subject not being found in the request attributes $arguments = array_fill(0, $numOfArguments, 'bar'); - $listener = new IsGrantedAttributeListener($authChecker); + $listener = new IsGrantedAttributeListener($authChecker, $expressionLanguage); $event = new ControllerArgumentsEvent( $this->createMock(HttpKernelInterface::class), @@ -233,7 +240,7 @@ public function testAccessDeniedMessages(string $attribute, string|array|null $s $this->fail(); } catch (AccessDeniedException $e) { $this->assertSame($expectedMessage, $e->getMessage()); - $this->assertSame([$attribute], $e->getAttributes()); + $this->assertEquals([$attribute], $e->getAttributes()); if (null !== $subject) { $this->assertSame($subject, $e->getSubject()); } else { @@ -247,6 +254,9 @@ public function getAccessDeniedMessageTests() yield ['ROLE_ADMIN', null, 'admin', 0, 'Access Denied by #[IsGranted("ROLE_ADMIN")] on controller']; yield ['ROLE_ADMIN', 'bar', 'withSubject', 2, 'Access Denied by #[IsGranted("ROLE_ADMIN", "arg2Name")] on controller']; yield ['ROLE_ADMIN', ['arg1Name' => 'bar', 'arg2Name' => 'bar'], 'withSubjectArray', 2, 'Access Denied by #[IsGranted("ROLE_ADMIN", ["arg1Name", "arg2Name"])] on controller']; + yield [new Expression('"ROLE_ADMIN" in role_names or is_granted("POST_VIEW", subject)'), 'bar', 'withExpressionInAttribute', 1, 'Access Denied by #[IsGranted(new Expression(""ROLE_ADMIN" in role_names or is_granted("POST_VIEW", subject)"), "post")] on controller']; + yield [new Expression('user === subject'), 'bar', 'withExpressionInSubject', 1, 'Access Denied by #[IsGranted(new Expression("user === subject"), new Expression("args["post"].getAuthor()"))] on controller']; + yield [new Expression('user === subject["author"]'), ['author' => 'bar', 'alias' => 'bar'], 'withNestedExpressionInSubject', 2, 'Access Denied by #[IsGranted(new Expression("user === subject["author"]"), ["author" => new Expression("args["post"].getAuthor()"), "alias" => "arg2Name"])] on controller']; } public function testNotFoundHttpException() @@ -270,4 +280,80 @@ public function testNotFoundHttpException() $listener = new IsGrantedAttributeListener($authChecker); $listener->onKernelControllerArguments($event); } + + public function testIsGrantedwithExpressionInAttribute() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + ->with(new Expression('"ROLE_ADMIN" in role_names or is_granted("POST_VIEW", subject)'), 'postVal') + ->willReturn(true); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsController(), 'withExpressionInAttribute'], + ['postVal'], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker); + $listener->onKernelControllerArguments($event); + } + + public function testIsGrantedwithExpressionInSubject() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + ->with(new Expression('user === subject'), 'author') + ->willReturn(true); + + $expressionLanguage = $this->createMock(ExpressionLanguage::class); + $expressionLanguage->expects($this->once()) + ->method('evaluate') + ->with(new Expression('args["post"].getAuthor()'), [ + 'args' => ['post' => 'postVal'], + ]) + ->willReturn('author'); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsController(), 'withExpressionInSubject'], + ['postVal'], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker, $expressionLanguage); + $listener->onKernelControllerArguments($event); + } + + public function testIsGrantedwithNestedExpressionInSubject() + { + $authChecker = $this->createMock(AuthorizationCheckerInterface::class); + $authChecker->expects($this->once()) + ->method('isGranted') + ->with(new Expression('user === subject["author"]'), ['author' => 'author', 'alias' => 'arg2Val']) + ->willReturn(true); + + $expressionLanguage = $this->createMock(ExpressionLanguage::class); + $expressionLanguage->expects($this->once()) + ->method('evaluate') + ->with(new Expression('args["post"].getAuthor()'), [ + 'args' => ['post' => 'postVal', 'arg2Name' => 'arg2Val'], + ]) + ->willReturn('author'); + + $event = new ControllerArgumentsEvent( + $this->createMock(HttpKernelInterface::class), + [new IsGrantedAttributeMethodsController(), 'withNestedExpressionInSubject'], + ['postVal', 'arg2Val'], + new Request(), + null + ); + + $listener = new IsGrantedAttributeListener($authChecker, $expressionLanguage); + $listener->onKernelControllerArguments($event); + } } diff --git a/src/Symfony/Component/Security/Http/Tests/Fixtures/IsGrantedAttributeMethodsController.php b/src/Symfony/Component/Security/Http/Tests/Fixtures/IsGrantedAttributeMethodsController.php index f9fd8c1c4eb43..63e73a7b23b93 100644 --- a/src/Symfony/Component/Security/Http/Tests/Fixtures/IsGrantedAttributeMethodsController.php +++ b/src/Symfony/Component/Security/Http/Tests/Fixtures/IsGrantedAttributeMethodsController.php @@ -11,6 +11,7 @@ namespace Symfony\Component\Security\Http\Tests\Fixtures; +use Symfony\Component\ExpressionLanguage\Expression; use Symfony\Component\Security\Http\Attribute\IsGranted; class IsGrantedAttributeMethodsController @@ -43,4 +44,22 @@ public function withMissingSubject() public function notFound() { } + + #[IsGranted(attribute: new Expression('"ROLE_ADMIN" in role_names or is_granted("POST_VIEW", subject)'), subject: 'post')] + public function withExpressionInAttribute($post) + { + } + + #[IsGranted(attribute: new Expression('user === subject'), subject: new Expression('args["post"].getAuthor()'))] + public function withExpressionInSubject($post) + { + } + + #[IsGranted(attribute: new Expression('user === subject["author"]'), subject: [ + 'author' => new Expression('args["post"].getAuthor()'), + 'alias' => 'arg2Name', + ])] + public function withNestedExpressionInSubject($post, $arg2Name) + { + } } diff --git a/src/Symfony/Component/Security/Http/composer.json b/src/Symfony/Component/Security/Http/composer.json index 907cfbaa5b942..4d7a35f724947 100644 --- a/src/Symfony/Component/Security/Http/composer.json +++ b/src/Symfony/Component/Security/Http/composer.json @@ -17,7 +17,7 @@ ], "require": { "php": ">=8.1", - "symfony/security-core": "^5.4.7|^6.0", + "symfony/security-core": "^6.0", "symfony/http-foundation": "^5.4|^6.0", "symfony/http-kernel": "^6.2", "symfony/polyfill-mbstring": "~1.0", @@ -25,6 +25,7 @@ }, "require-dev": { "symfony/cache": "^5.4|^6.0", + "symfony/expression-language": "^5.4|^6.0", "symfony/rate-limiter": "^5.4|^6.0", "symfony/routing": "^5.4|^6.0", "symfony/security-csrf": "^5.4|^6.0",