From 312a726ac8b92bf1544355eefd2d290211d3119b Mon Sep 17 00:00:00 2001 From: Nate Wiebe Date: Mon, 13 Dec 2021 17:05:08 -0500 Subject: [PATCH 01/15] [Security][SecurityBundle] User authorization checker --- .../Token/OfflineTokenInterface.php | 21 ++++++ .../Token/UserAuthorizationCheckerToken.php | 31 ++++++++ Authorization/UserAuthorizationChecker.php | 31 ++++++++ .../UserAuthorizationCheckerInterface.php | 29 ++++++++ Authorization/Voter/AuthenticatedVoter.php | 6 ++ CHANGELOG.md | 7 ++ .../UserAuthorizationCheckerTokenTest.php | 26 +++++++ .../UserAuthorizationCheckerTest.php | 70 +++++++++++++++++++ .../Voter/AuthenticatedVoterTest.php | 43 ++++++++++++ 9 files changed, 264 insertions(+) create mode 100644 Authentication/Token/OfflineTokenInterface.php create mode 100644 Authentication/Token/UserAuthorizationCheckerToken.php create mode 100644 Authorization/UserAuthorizationChecker.php create mode 100644 Authorization/UserAuthorizationCheckerInterface.php create mode 100644 Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php create mode 100644 Tests/Authorization/UserAuthorizationCheckerTest.php diff --git a/Authentication/Token/OfflineTokenInterface.php b/Authentication/Token/OfflineTokenInterface.php new file mode 100644 index 0000000..894f0fd --- /dev/null +++ b/Authentication/Token/OfflineTokenInterface.php @@ -0,0 +1,21 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authentication\Token; + +/** + * Interface used for marking tokens that do not represent the currently logged-in user. + * + * @author Nate Wiebe + */ +interface OfflineTokenInterface extends TokenInterface +{ +} diff --git a/Authentication/Token/UserAuthorizationCheckerToken.php b/Authentication/Token/UserAuthorizationCheckerToken.php new file mode 100644 index 0000000..2e84ce7 --- /dev/null +++ b/Authentication/Token/UserAuthorizationCheckerToken.php @@ -0,0 +1,31 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authentication\Token; + +use Symfony\Component\Security\Core\User\UserInterface; + +/** + * UserAuthorizationCheckerToken implements a token used for checking authorization. + * + * @author Nate Wiebe + * + * @internal + */ +final class UserAuthorizationCheckerToken extends AbstractToken implements OfflineTokenInterface +{ + public function __construct(UserInterface $user) + { + parent::__construct($user->getRoles()); + + $this->setUser($user); + } +} diff --git a/Authorization/UserAuthorizationChecker.php b/Authorization/UserAuthorizationChecker.php new file mode 100644 index 0000000..e4d2eab --- /dev/null +++ b/Authorization/UserAuthorizationChecker.php @@ -0,0 +1,31 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization; + +use Symfony\Component\Security\Core\Authentication\Token\UserAuthorizationCheckerToken; +use Symfony\Component\Security\Core\User\UserInterface; + +/** + * @author Nate Wiebe + */ +final class UserAuthorizationChecker implements UserAuthorizationCheckerInterface +{ + public function __construct( + private readonly AccessDecisionManagerInterface $accessDecisionManager, + ) { + } + + public function userIsGranted(UserInterface $user, mixed $attribute, mixed $subject = null): bool + { + return $this->accessDecisionManager->decide(new UserAuthorizationCheckerToken($user), [$attribute], $subject); + } +} diff --git a/Authorization/UserAuthorizationCheckerInterface.php b/Authorization/UserAuthorizationCheckerInterface.php new file mode 100644 index 0000000..370cf61 --- /dev/null +++ b/Authorization/UserAuthorizationCheckerInterface.php @@ -0,0 +1,29 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization; + +use Symfony\Component\Security\Core\User\UserInterface; + +/** + * Interface is used to check user authorization without a session. + * + * @author Nate Wiebe + */ +interface UserAuthorizationCheckerInterface +{ + /** + * Checks if the attribute is granted against the user and optionally supplied subject. + * + * @param mixed $attribute A single attribute to vote on (can be of any type, string and instance of Expression are supported by the core) + */ + public function userIsGranted(UserInterface $user, mixed $attribute, mixed $subject = null): bool; +} diff --git a/Authorization/Voter/AuthenticatedVoter.php b/Authorization/Voter/AuthenticatedVoter.php index a001186..a073f61 100644 --- a/Authorization/Voter/AuthenticatedVoter.php +++ b/Authorization/Voter/AuthenticatedVoter.php @@ -12,8 +12,10 @@ namespace Symfony\Component\Security\Core\Authorization\Voter; use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface; +use Symfony\Component\Security\Core\Authentication\Token\OfflineTokenInterface; use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Exception\InvalidArgumentException; /** * AuthenticatedVoter votes if an attribute like IS_AUTHENTICATED_FULLY, @@ -54,6 +56,10 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes): continue; } + if ($token instanceof OfflineTokenInterface) { + throw new InvalidArgumentException('Cannot decide on authentication attributes when an offline token is used.'); + } + $result = VoterInterface::ACCESS_DENIED; if (self::IS_AUTHENTICATED_FULLY === $attribute diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cf09c7..2a54af9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ CHANGELOG ========= +7.3 +--- + + * Add `UserAuthorizationChecker::userIsGranted()` to test user authorization without relying on the session. + For example, users not currently logged in, or while processing a message from a message queue. + * Add `OfflineTokenInterface` to mark tokens that do not represent the currently logged-in user + 7.2 --- diff --git a/Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php b/Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php new file mode 100644 index 0000000..2e7e11b --- /dev/null +++ b/Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php @@ -0,0 +1,26 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Tests\Authentication\Token; + +use PHPUnit\Framework\TestCase; +use Symfony\Component\Security\Core\Authentication\Token\UserAuthorizationCheckerToken; +use Symfony\Component\Security\Core\User\InMemoryUser; + +class UserAuthorizationCheckerTokenTest extends TestCase +{ + public function testConstructor() + { + $token = new UserAuthorizationCheckerToken($user = new InMemoryUser('foo', 'bar', ['ROLE_FOO'])); + $this->assertSame(['ROLE_FOO'], $token->getRoleNames()); + $this->assertSame($user, $token->getUser()); + } +} diff --git a/Tests/Authorization/UserAuthorizationCheckerTest.php b/Tests/Authorization/UserAuthorizationCheckerTest.php new file mode 100644 index 0000000..e8b165a --- /dev/null +++ b/Tests/Authorization/UserAuthorizationCheckerTest.php @@ -0,0 +1,70 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Tests\Authorization; + +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Symfony\Component\Security\Core\Authentication\Token\UserAuthorizationCheckerToken; +use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; +use Symfony\Component\Security\Core\Authorization\UserAuthorizationChecker; +use Symfony\Component\Security\Core\User\InMemoryUser; + +class UserAuthorizationCheckerTest extends TestCase +{ + private AccessDecisionManagerInterface&MockObject $accessDecisionManager; + private UserAuthorizationChecker $authorizationChecker; + + protected function setUp(): void + { + $this->accessDecisionManager = $this->createMock(AccessDecisionManagerInterface::class); + + $this->authorizationChecker = new UserAuthorizationChecker($this->accessDecisionManager); + } + + /** + * @dataProvider isGrantedProvider + */ + public function testIsGranted(bool $decide, array $roles) + { + $user = new InMemoryUser('username', 'password', $roles); + + $this->accessDecisionManager + ->expects($this->once()) + ->method('decide') + ->with($this->callback(fn (UserAuthorizationCheckerToken $token): bool => $user === $token->getUser()), $this->identicalTo(['ROLE_FOO'])) + ->willReturn($decide); + + $this->assertSame($decide, $this->authorizationChecker->userIsGranted($user, 'ROLE_FOO')); + } + + public static function isGrantedProvider(): array + { + return [ + [false, ['ROLE_USER']], + [true, ['ROLE_USER', 'ROLE_FOO']], + ]; + } + + public function testIsGrantedWithObjectAttribute() + { + $attribute = new \stdClass(); + + $token = new UserAuthorizationCheckerToken(new InMemoryUser('username', 'password', ['ROLE_USER'])); + + $this->accessDecisionManager + ->expects($this->once()) + ->method('decide') + ->with($this->isInstanceOf($token::class), $this->identicalTo([$attribute])) + ->willReturn(true); + $this->assertTrue($this->authorizationChecker->userIsGranted($token->getUser(), $attribute)); + } +} diff --git a/Tests/Authorization/Voter/AuthenticatedVoterTest.php b/Tests/Authorization/Voter/AuthenticatedVoterTest.php index ed894b3..89f6c35 100644 --- a/Tests/Authorization/Voter/AuthenticatedVoterTest.php +++ b/Tests/Authorization/Voter/AuthenticatedVoterTest.php @@ -17,8 +17,10 @@ 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\UserAuthorizationCheckerToken; use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; +use Symfony\Component\Security\Core\Exception\InvalidArgumentException; use Symfony\Component\Security\Core\User\InMemoryUser; class AuthenticatedVoterTest extends TestCase @@ -85,6 +87,43 @@ public function testSupportsType() $this->assertTrue($voter->supportsType(get_debug_type(new \stdClass()))); } + /** + * @dataProvider provideOfflineAttributes + */ + public function testOfflineToken($attributes, $expected) + { + $voter = new AuthenticatedVoter(new AuthenticationTrustResolver()); + + $this->assertSame($expected, $voter->vote($this->getToken('offline'), null, $attributes)); + } + + public static function provideOfflineAttributes() + { + yield [[AuthenticatedVoter::PUBLIC_ACCESS], VoterInterface::ACCESS_GRANTED]; + yield [['ROLE_FOO'], VoterInterface::ACCESS_ABSTAIN]; + } + + /** + * @dataProvider provideUnsupportedOfflineAttributes + */ + public function testUnsupportedOfflineToken(string $attribute) + { + $voter = new AuthenticatedVoter(new AuthenticationTrustResolver()); + + $this->expectException(InvalidArgumentException::class); + + $voter->vote($this->getToken('offline'), null, [$attribute]); + } + + public static function provideUnsupportedOfflineAttributes() + { + yield [AuthenticatedVoter::IS_AUTHENTICATED_FULLY]; + yield [AuthenticatedVoter::IS_AUTHENTICATED_REMEMBERED]; + yield [AuthenticatedVoter::IS_AUTHENTICATED]; + yield [AuthenticatedVoter::IS_IMPERSONATOR]; + yield [AuthenticatedVoter::IS_REMEMBERED]; + } + protected function getToken($authenticated) { $user = new InMemoryUser('wouter', '', ['ROLE_USER']); @@ -108,6 +147,10 @@ public function getCredentials() return $this->getMockBuilder(SwitchUserToken::class)->disableOriginalConstructor()->getMock(); } + if ('offline' === $authenticated) { + return new UserAuthorizationCheckerToken($user); + } + return new NullToken(); } } From 9a04c4c5bf59a255f177b588bc2b3e1ab1683e57 Mon Sep 17 00:00:00 2001 From: Christian Flothmann Date: Sun, 15 Dec 2024 11:42:01 +0100 Subject: [PATCH 02/15] rename userIsGranted() to isGrantedForUser() --- Authorization/UserAuthorizationChecker.php | 2 +- Authorization/UserAuthorizationCheckerInterface.php | 2 +- CHANGELOG.md | 2 +- Tests/Authorization/UserAuthorizationCheckerTest.php | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Authorization/UserAuthorizationChecker.php b/Authorization/UserAuthorizationChecker.php index e4d2eab..f5ba7b8 100644 --- a/Authorization/UserAuthorizationChecker.php +++ b/Authorization/UserAuthorizationChecker.php @@ -24,7 +24,7 @@ public function __construct( ) { } - public function userIsGranted(UserInterface $user, mixed $attribute, mixed $subject = null): bool + public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null): bool { return $this->accessDecisionManager->decide(new UserAuthorizationCheckerToken($user), [$attribute], $subject); } diff --git a/Authorization/UserAuthorizationCheckerInterface.php b/Authorization/UserAuthorizationCheckerInterface.php index 370cf61..3335e6f 100644 --- a/Authorization/UserAuthorizationCheckerInterface.php +++ b/Authorization/UserAuthorizationCheckerInterface.php @@ -25,5 +25,5 @@ interface UserAuthorizationCheckerInterface * * @param mixed $attribute A single attribute to vote on (can be of any type, string and instance of Expression are supported by the core) */ - public function userIsGranted(UserInterface $user, mixed $attribute, mixed $subject = null): bool; + public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null): bool; } diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a54af9..3cc738c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ CHANGELOG 7.3 --- - * Add `UserAuthorizationChecker::userIsGranted()` to test user authorization without relying on the session. + * Add `UserAuthorizationChecker::isGrantedForUser()` to test user authorization without relying on the session. For example, users not currently logged in, or while processing a message from a message queue. * Add `OfflineTokenInterface` to mark tokens that do not represent the currently logged-in user diff --git a/Tests/Authorization/UserAuthorizationCheckerTest.php b/Tests/Authorization/UserAuthorizationCheckerTest.php index e8b165a..e9b6bb7 100644 --- a/Tests/Authorization/UserAuthorizationCheckerTest.php +++ b/Tests/Authorization/UserAuthorizationCheckerTest.php @@ -43,7 +43,7 @@ public function testIsGranted(bool $decide, array $roles) ->with($this->callback(fn (UserAuthorizationCheckerToken $token): bool => $user === $token->getUser()), $this->identicalTo(['ROLE_FOO'])) ->willReturn($decide); - $this->assertSame($decide, $this->authorizationChecker->userIsGranted($user, 'ROLE_FOO')); + $this->assertSame($decide, $this->authorizationChecker->isGrantedForUser($user, 'ROLE_FOO')); } public static function isGrantedProvider(): array @@ -65,6 +65,6 @@ public function testIsGrantedWithObjectAttribute() ->method('decide') ->with($this->isInstanceOf($token::class), $this->identicalTo([$attribute])) ->willReturn(true); - $this->assertTrue($this->authorizationChecker->userIsGranted($token->getUser(), $attribute)); + $this->assertTrue($this->authorizationChecker->isGrantedForUser($token->getUser(), $attribute)); } } From c83ae6df9b217e9d405f3a4ce69742d44961d340 Mon Sep 17 00:00:00 2001 From: kor3k Date: Sat, 21 Dec 2024 19:05:36 +0100 Subject: [PATCH 03/15] Sync Security\ExpressionLanguage constructor with parent change typehint array -> iterable --- Authorization/ExpressionLanguage.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Authorization/ExpressionLanguage.php b/Authorization/ExpressionLanguage.php index 846d2cf..35f32e4 100644 --- a/Authorization/ExpressionLanguage.php +++ b/Authorization/ExpressionLanguage.php @@ -29,8 +29,12 @@ class_exists(ExpressionLanguageProvider::class); */ class ExpressionLanguage extends BaseExpressionLanguage { - public function __construct(?CacheItemPoolInterface $cache = null, array $providers = []) + public function __construct(?CacheItemPoolInterface $cache = null, iterable $providers = []) { + if (!\is_array($providers)) { + $providers = iterator_to_array($providers, false); + } + // prepend the default provider to let users override it easily array_unshift($providers, new ExpressionLanguageProvider()); From 8f45e009dbb2af198a2cd851447ccddd7c608283 Mon Sep 17 00:00:00 2001 From: Dariusz Ruminski Date: Fri, 10 Jan 2025 15:17:09 +0100 Subject: [PATCH 04/15] chore: PHP CS Fixer fixes --- User/ChainUserChecker.php | 2 +- User/UserCheckerInterface.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/User/ChainUserChecker.php b/User/ChainUserChecker.php index 67fd76b..eb9ff33 100644 --- a/User/ChainUserChecker.php +++ b/User/ChainUserChecker.php @@ -29,7 +29,7 @@ public function checkPreAuth(UserInterface $user): void } } - public function checkPostAuth(UserInterface $user /*, TokenInterface $token*/): void + public function checkPostAuth(UserInterface $user /* , TokenInterface $token */): void { $token = 1 < \func_num_args() ? func_get_arg(1) : null; diff --git a/User/UserCheckerInterface.php b/User/UserCheckerInterface.php index 2dc748a..43f1651 100644 --- a/User/UserCheckerInterface.php +++ b/User/UserCheckerInterface.php @@ -35,5 +35,5 @@ public function checkPreAuth(UserInterface $user): void; * * @throws AccountStatusException */ - public function checkPostAuth(UserInterface $user /*, TokenInterface $token*/): void; + public function checkPostAuth(UserInterface $user /* , TokenInterface $token */): void; } From 3062fe62a6261d1e8a6bea0636d9c03f6b2ba684 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 20 Jan 2025 10:01:54 +0100 Subject: [PATCH 05/15] [Security] Unset token roles when serializing it and user implements EquatableInterface --- Authentication/Token/AbstractToken.php | 26 ++++++++++++++----- ...UserMessageAuthenticationExceptionTest.php | 2 ++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/Authentication/Token/AbstractToken.php b/Authentication/Token/AbstractToken.php index 67d992c..4a92b60 100644 --- a/Authentication/Token/AbstractToken.php +++ b/Authentication/Token/AbstractToken.php @@ -11,6 +11,7 @@ namespace Symfony\Component\Security\Core\Authentication\Token; +use Symfony\Component\Security\Core\User\EquatableInterface; use Symfony\Component\Security\Core\User\InMemoryUser; use Symfony\Component\Security\Core\User\UserInterface; @@ -23,24 +24,24 @@ abstract class AbstractToken implements TokenInterface, \Serializable { private ?UserInterface $user = null; - private array $roleNames = []; + private array $roleNames; private array $attributes = []; /** * @param string[] $roles An array of roles - * - * @throws \InvalidArgumentException */ public function __construct(array $roles = []) { + $this->roleNames = []; + foreach ($roles as $role) { - $this->roleNames[] = $role; + $this->roleNames[] = (string) $role; } } public function getRoleNames(): array { - return $this->roleNames; + return $this->roleNames ??= self::__construct($this->user->getRoles()) ?? $this->roleNames; } public function getUserIdentifier(): string @@ -82,7 +83,13 @@ public function eraseCredentials(): void */ public function __serialize(): array { - return [$this->user, true, null, $this->attributes, $this->roleNames]; + $data = [$this->user, true, null, $this->attributes]; + + if (!$this->user instanceof EquatableInterface) { + $data[] = $this->roleNames; + } + + return $data; } /** @@ -103,7 +110,12 @@ public function __serialize(): array */ public function __unserialize(array $data): void { - [$user, , , $this->attributes, $this->roleNames] = $data; + [$user, , , $this->attributes] = $data; + + if (\array_key_exists(4, $data)) { + $this->roleNames = $data[4]; + } + $this->user = \is_string($user) ? new InMemoryUser($user, '', $this->roleNames, false) : $user; } diff --git a/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php b/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php index 5555ce0..5a87429 100644 --- a/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php +++ b/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php @@ -53,6 +53,7 @@ public function testSharedSerializedData() $exception->setSafeMessage('message', ['token' => $token]); $processed = unserialize(serialize($exception)); + $this->assertSame($token->getRoleNames(), $processed->getToken()->getRoleNames()); $this->assertEquals($token, $processed->getToken()); $this->assertEquals($token, $processed->getMessageData()['token']); $this->assertSame($processed->getToken(), $processed->getMessageData()['token']); @@ -67,6 +68,7 @@ public function testSharedSerializedDataFromChild() $exception->setToken($token); $processed = unserialize(serialize($exception)); + $this->assertSame($token->getRoleNames(), $processed->getToken()->getRoleNames()); $this->assertEquals($token, $processed->childMember); $this->assertEquals($token, $processed->getToken()); $this->assertSame($processed->getToken(), $processed->childMember); From 658e5c856d04ea89cb8bf4963309eb803a3aa108 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Fri, 17 Jan 2025 16:58:49 +0100 Subject: [PATCH 06/15] [Security] Don't invalidate the user when the password was not stored in the session --- .../Token/Fixtures/CustomUser.php | 32 ++++++++----------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/Tests/Authentication/Token/Fixtures/CustomUser.php b/Tests/Authentication/Token/Fixtures/CustomUser.php index 9930203..c801cc2 100644 --- a/Tests/Authentication/Token/Fixtures/CustomUser.php +++ b/Tests/Authentication/Token/Fixtures/CustomUser.php @@ -1,20 +1,24 @@ + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + namespace Symfony\Component\Security\Core\Tests\Authentication\Token\Fixtures; use Symfony\Component\Security\Core\User\UserInterface; final class CustomUser implements UserInterface { - /** @var string */ - private $username; - /** @var array */ - private $roles; - - public function __construct(string $username, array $roles) - { - $this->username = $username; - $this->roles = $roles; + public function __construct( + private string $username, + private array $roles, + ) { } public function getUserIdentifier(): string @@ -27,16 +31,6 @@ public function getRoles(): array return $this->roles; } - public function getPassword(): ?string - { - return null; - } - - public function getSalt(): ?string - { - return null; - } - public function eraseCredentials(): void { } From 1aadc21b0338d67f506b7bfe1b95c095cbb808ad Mon Sep 17 00:00:00 2001 From: Robin Chalas Date: Fri, 6 Dec 2024 11:20:22 +0100 Subject: [PATCH 07/15] [Security] Deprecate `UserInterface` & `TokenInterface`'s `eraseCredentials()` --- Authentication/Token/AbstractToken.php | 7 +++++++ Authentication/Token/NullToken.php | 5 +++++ Authentication/Token/TokenInterface.php | 3 +++ CHANGELOG.md | 2 ++ Tests/Authentication/Token/AbstractTokenTest.php | 7 +++++++ Tests/User/InMemoryUserTest.php | 3 +++ User/InMemoryUser.php | 1 + User/UserInterface.php | 16 ++++++++++------ 8 files changed, 38 insertions(+), 6 deletions(-) diff --git a/Authentication/Token/AbstractToken.php b/Authentication/Token/AbstractToken.php index 4a92b60..382d92c 100644 --- a/Authentication/Token/AbstractToken.php +++ b/Authentication/Token/AbstractToken.php @@ -59,8 +59,15 @@ public function setUser(UserInterface $user): void $this->user = $user; } + /** + * Removes sensitive information from the token. + * + * @deprecated since Symfony 7.3 + */ public function eraseCredentials(): void { + trigger_deprecation('symfony/security-core', '7.3', sprintf('The "%s()" method is deprecated and will be removed in 8.0, use a DTO instead or implement your own erasing logic if needed.', __METHOD__)); + if ($this->getUser() instanceof UserInterface) { $this->getUser()->eraseCredentials(); } diff --git a/Authentication/Token/NullToken.php b/Authentication/Token/NullToken.php index 9c2e489..1c60357 100644 --- a/Authentication/Token/NullToken.php +++ b/Authentication/Token/NullToken.php @@ -43,6 +43,11 @@ public function getUserIdentifier(): string return ''; } + /** + * Removes sensitive information from the token. + * + * @deprecated since Symfony 7.3 + */ public function eraseCredentials(): void { } diff --git a/Authentication/Token/TokenInterface.php b/Authentication/Token/TokenInterface.php index 1e67b1e..8fb58c1 100644 --- a/Authentication/Token/TokenInterface.php +++ b/Authentication/Token/TokenInterface.php @@ -56,6 +56,9 @@ public function setUser(UserInterface $user): void; /** * Removes sensitive information from the token. + * + * @deprecated since Symfony 7.3, use a dedicated DTO instead or implement your + * own erasing logic instead */ public function eraseCredentials(): void; diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cc738c..b3d1337 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ CHANGELOG * Add `UserAuthorizationChecker::isGrantedForUser()` to test user authorization without relying on the session. For example, users not currently logged in, or while processing a message from a message queue. * Add `OfflineTokenInterface` to mark tokens that do not represent the currently logged-in user + * Deprecate `UserInterface::eraseCredentials()` and `TokenInterface::eraseCredentials()`, + use a dedicated DTO or erase credentials on your own e.g. upon `AuthenticationTokenCreatedEvent` instead 7.2 --- diff --git a/Tests/Authentication/Token/AbstractTokenTest.php b/Tests/Authentication/Token/AbstractTokenTest.php index cc1357a..33b6520 100644 --- a/Tests/Authentication/Token/AbstractTokenTest.php +++ b/Tests/Authentication/Token/AbstractTokenTest.php @@ -12,12 +12,15 @@ namespace Symfony\Component\Security\Core\Tests\Authentication\Token; use PHPUnit\Framework\TestCase; +use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; use Symfony\Component\Security\Core\Authentication\Token\AbstractToken; use Symfony\Component\Security\Core\User\InMemoryUser; use Symfony\Component\Security\Core\User\UserInterface; class AbstractTokenTest extends TestCase { + use ExpectDeprecationTrait; + /** * @dataProvider provideUsers */ @@ -33,6 +36,9 @@ public static function provideUsers() yield [new InMemoryUser('fabien', null), 'fabien']; } + /** + * @group legacy + */ public function testEraseCredentials() { $token = new ConcreteToken(['ROLE_FOO']); @@ -40,6 +46,7 @@ public function testEraseCredentials() $user = $this->createMock(UserInterface::class); $user->expects($this->once())->method('eraseCredentials'); $token->setUser($user); + $this->expectDeprecation('The Symfony\Component\Security\Core\User\UserInterface::eraseCredentials method is deprecated (since Symfony 7.3, use a dedicated DTO instead or implement your own erasing logic instead).'); $token->eraseCredentials(); } diff --git a/Tests/User/InMemoryUserTest.php b/Tests/User/InMemoryUserTest.php index 0e64bce..4537b1f 100644 --- a/Tests/User/InMemoryUserTest.php +++ b/Tests/User/InMemoryUserTest.php @@ -53,6 +53,9 @@ public function testIsEnabled() $this->assertFalse($user->isEnabled()); } + /** + * @group legacy + */ public function testEraseCredentials() { $user = new InMemoryUser('fabien', 'superpass'); diff --git a/User/InMemoryUser.php b/User/InMemoryUser.php index b14bc07..bfaae97 100644 --- a/User/InMemoryUser.php +++ b/User/InMemoryUser.php @@ -76,6 +76,7 @@ public function isEnabled(): bool public function eraseCredentials(): void { + trigger_deprecation('symfony/security-core', '7.3', sprintf('The "%s()" method is deprecated and will be removed in 8.0, use a DTO instead or implement your own erasing logic if needed.', __METHOD__)); } public function isEqualTo(UserInterface $user): bool diff --git a/User/UserInterface.php b/User/UserInterface.php index e607839..9bee95c 100644 --- a/User/UserInterface.php +++ b/User/UserInterface.php @@ -46,18 +46,22 @@ interface UserInterface */ public function getRoles(): array; + /** + * Returns the identifier for this user (e.g. username or email address). + * + * @return non-empty-string + */ + public function getUserIdentifier(): string; + /** * Removes sensitive data from the user. * * This is important if, at any given point, sensitive information like * the plain-text password is stored on this object. + * + * @deprecated since Symfony 7.3, use a dedicated DTO instead or implement your + * own erasing logic instead */ public function eraseCredentials(): void; - /** - * Returns the identifier for this user (e.g. username or email address). - * - * @return non-empty-string - */ - public function getUserIdentifier(): string; } From f5f6b03bafb27a15ac63f5a0eb25a15d535747d5 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 3 Feb 2025 18:35:30 +0100 Subject: [PATCH 08/15] [Security] Improve BC-layer to deprecate eraseCredentials methods --- Authentication/Token/AbstractToken.php | 4 ++-- Authentication/Token/NullToken.php | 6 ++++-- Authentication/Token/TokenInterface.php | 6 ++++-- CHANGELOG.md | 2 +- .../AuthenticationTrustResolverTest.php | 1 + .../Token/AbstractTokenTest.php | 4 +++- .../Token/Fixtures/CustomUser.php | 6 ++++++ Tests/User/InMemoryUserTest.php | 4 ++++ User/InMemoryUser.php | 8 +++++++- User/OidcUser.php | 7 +++++++ User/PasswordAuthenticatedUserInterface.php | 3 +++ User/UserInterface.php | 19 ++++++++++--------- 12 files changed, 52 insertions(+), 18 deletions(-) diff --git a/Authentication/Token/AbstractToken.php b/Authentication/Token/AbstractToken.php index 382d92c..b2e18a2 100644 --- a/Authentication/Token/AbstractToken.php +++ b/Authentication/Token/AbstractToken.php @@ -62,11 +62,11 @@ public function setUser(UserInterface $user): void /** * Removes sensitive information from the token. * - * @deprecated since Symfony 7.3 + * @deprecated since Symfony 7.3, erase credentials using the "__serialize()" method instead */ public function eraseCredentials(): void { - trigger_deprecation('symfony/security-core', '7.3', sprintf('The "%s()" method is deprecated and will be removed in 8.0, use a DTO instead or implement your own erasing logic if needed.', __METHOD__)); + trigger_deprecation('symfony/security-core', '7.3', \sprintf('The "%s::eraseCredentials()" method is deprecated and will be removed in 8.0, erase credentials using the "__serialize()" method instead.', TokenInterface::class)); if ($this->getUser() instanceof UserInterface) { $this->getUser()->eraseCredentials(); diff --git a/Authentication/Token/NullToken.php b/Authentication/Token/NullToken.php index 1c60357..cb2bc0f 100644 --- a/Authentication/Token/NullToken.php +++ b/Authentication/Token/NullToken.php @@ -44,12 +44,14 @@ public function getUserIdentifier(): string } /** - * Removes sensitive information from the token. - * * @deprecated since Symfony 7.3 */ + #[\Deprecated(since: 'symfony/security-core 7.3')] public function eraseCredentials(): void { + if (\PHP_VERSION_ID < 80400) { + @trigger_error(\sprintf('Method %s::eraseCredentials() is deprecated since symfony/security-core 7.3', self::class), \E_USER_DEPRECATED); + } } public function getAttributes(): array diff --git a/Authentication/Token/TokenInterface.php b/Authentication/Token/TokenInterface.php index 8fb58c1..c658e38 100644 --- a/Authentication/Token/TokenInterface.php +++ b/Authentication/Token/TokenInterface.php @@ -16,6 +16,9 @@ /** * TokenInterface is the interface for the user authentication information. * + * The __serialize/__unserialize() magic methods can be implemented on the token + * class to prevent sensitive credentials from being put in the session storage. + * * @author Fabien Potencier * @author Johannes M. Schmitt */ @@ -57,8 +60,7 @@ public function setUser(UserInterface $user): void; /** * Removes sensitive information from the token. * - * @deprecated since Symfony 7.3, use a dedicated DTO instead or implement your - * own erasing logic instead + * @deprecated since Symfony 7.3; erase credentials using the "__serialize()" method instead */ public function eraseCredentials(): void; diff --git a/CHANGELOG.md b/CHANGELOG.md index b3d1337..290aa8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ CHANGELOG For example, users not currently logged in, or while processing a message from a message queue. * Add `OfflineTokenInterface` to mark tokens that do not represent the currently logged-in user * Deprecate `UserInterface::eraseCredentials()` and `TokenInterface::eraseCredentials()`, - use a dedicated DTO or erase credentials on your own e.g. upon `AuthenticationTokenCreatedEvent` instead + erase credentials e.g. using `__serialize()` instead 7.2 --- diff --git a/Tests/Authentication/AuthenticationTrustResolverTest.php b/Tests/Authentication/AuthenticationTrustResolverTest.php index fc55998..c657b31 100644 --- a/Tests/Authentication/AuthenticationTrustResolverTest.php +++ b/Tests/Authentication/AuthenticationTrustResolverTest.php @@ -119,6 +119,7 @@ public function getUserIdentifier(): string { } + #[\Deprecated] public function eraseCredentials(): void { } diff --git a/Tests/Authentication/Token/AbstractTokenTest.php b/Tests/Authentication/Token/AbstractTokenTest.php index 33b6520..ef3d380 100644 --- a/Tests/Authentication/Token/AbstractTokenTest.php +++ b/Tests/Authentication/Token/AbstractTokenTest.php @@ -14,6 +14,7 @@ use PHPUnit\Framework\TestCase; use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; use Symfony\Component\Security\Core\Authentication\Token\AbstractToken; +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\User\InMemoryUser; use Symfony\Component\Security\Core\User\UserInterface; @@ -46,7 +47,8 @@ public function testEraseCredentials() $user = $this->createMock(UserInterface::class); $user->expects($this->once())->method('eraseCredentials'); $token->setUser($user); - $this->expectDeprecation('The Symfony\Component\Security\Core\User\UserInterface::eraseCredentials method is deprecated (since Symfony 7.3, use a dedicated DTO instead or implement your own erasing logic instead).'); + + $this->expectDeprecation(\sprintf('Since symfony/security-core 7.3: The "%s::eraseCredentials()" method is deprecated and will be removed in 8.0, erase credentials using the "__serialize()" method instead.', TokenInterface::class)); $token->eraseCredentials(); } diff --git a/Tests/Authentication/Token/Fixtures/CustomUser.php b/Tests/Authentication/Token/Fixtures/CustomUser.php index c801cc2..d4f91de 100644 --- a/Tests/Authentication/Token/Fixtures/CustomUser.php +++ b/Tests/Authentication/Token/Fixtures/CustomUser.php @@ -31,6 +31,12 @@ public function getRoles(): array return $this->roles; } + public function getPassword(): ?string + { + return null; + } + + #[\Deprecated] public function eraseCredentials(): void { } diff --git a/Tests/User/InMemoryUserTest.php b/Tests/User/InMemoryUserTest.php index 4537b1f..501bf74 100644 --- a/Tests/User/InMemoryUserTest.php +++ b/Tests/User/InMemoryUserTest.php @@ -12,11 +12,14 @@ namespace Symfony\Component\Security\Core\Tests\User; use PHPUnit\Framework\TestCase; +use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; use Symfony\Component\Security\Core\User\InMemoryUser; use Symfony\Component\Security\Core\User\UserInterface; class InMemoryUserTest extends TestCase { + use ExpectDeprecationTrait; + public function testConstructorException() { $this->expectException(\InvalidArgumentException::class); @@ -59,6 +62,7 @@ public function testIsEnabled() public function testEraseCredentials() { $user = new InMemoryUser('fabien', 'superpass'); + $this->expectDeprecation(\sprintf('%sMethod %s::eraseCredentials() is deprecated since symfony/security-core 7.3', \PHP_VERSION_ID >= 80400 ? 'Unsilenced deprecation: ' : '', InMemoryUser::class)); $user->eraseCredentials(); $this->assertEquals('superpass', $user->getPassword()); } diff --git a/User/InMemoryUser.php b/User/InMemoryUser.php index bfaae97..7bed183 100644 --- a/User/InMemoryUser.php +++ b/User/InMemoryUser.php @@ -74,9 +74,15 @@ public function isEnabled(): bool return $this->enabled; } + /** + * @deprecated since Symfony 7.3 + */ + #[\Deprecated(since: 'symfony/security-core 7.3')] public function eraseCredentials(): void { - trigger_deprecation('symfony/security-core', '7.3', sprintf('The "%s()" method is deprecated and will be removed in 8.0, use a DTO instead or implement your own erasing logic if needed.', __METHOD__)); + if (\PHP_VERSION_ID < 80400) { + @trigger_error(\sprintf('Method %s::eraseCredentials() is deprecated since symfony/security-core 7.3', self::class), \E_USER_DEPRECATED); + } } public function isEqualTo(UserInterface $user): bool diff --git a/User/OidcUser.php b/User/OidcUser.php index bcce363..df59c5f 100644 --- a/User/OidcUser.php +++ b/User/OidcUser.php @@ -71,8 +71,15 @@ public function getUserIdentifier(): string return (string) ($this->userIdentifier ?? $this->getSub()); } + /** + * @deprecated since Symfony 7.3 + */ + #[\Deprecated(since: 'symfony/security-core 7.3')] public function eraseCredentials(): void { + if (\PHP_VERSION_ID < 80400) { + @trigger_error(\sprintf('Method %s::eraseCredentials() is deprecated since symfony/security-core 7.3', self::class), \E_USER_DEPRECATED); + } } public function getSub(): ?string diff --git a/User/PasswordAuthenticatedUserInterface.php b/User/PasswordAuthenticatedUserInterface.php index 478c9e3..900963d 100644 --- a/User/PasswordAuthenticatedUserInterface.php +++ b/User/PasswordAuthenticatedUserInterface.php @@ -23,6 +23,9 @@ interface PasswordAuthenticatedUserInterface * Returns the hashed password used to authenticate the user. * * Usually on authentication, a plain-text password will be compared to this value. + * + * The __serialize/__unserialize() magic methods can be implemented on the user + * class to prevent hashed passwords from being put in the session storage. */ public function getPassword(): ?string; } diff --git a/User/UserInterface.php b/User/UserInterface.php index 9bee95c..1205212 100644 --- a/User/UserInterface.php +++ b/User/UserInterface.php @@ -24,6 +24,9 @@ * this interface. Objects that implement this interface are created and * loaded by different objects that implement UserProviderInterface. * + * The __serialize/__unserialize() magic methods can be implemented on the user + * class to prevent sensitive credentials from being put in the session storage. + * * @see UserProviderInterface * * @author Fabien Potencier @@ -46,22 +49,20 @@ interface UserInterface */ public function getRoles(): array; - /** - * Returns the identifier for this user (e.g. username or email address). - * - * @return non-empty-string - */ - public function getUserIdentifier(): string; - /** * Removes sensitive data from the user. * * This is important if, at any given point, sensitive information like * the plain-text password is stored on this object. * - * @deprecated since Symfony 7.3, use a dedicated DTO instead or implement your - * own erasing logic instead + * @deprecated since Symfony 7.3, erase credentials using the "__serialize()" method instead */ public function eraseCredentials(): void; + /** + * Returns the identifier for this user (e.g. username or email address). + * + * @return non-empty-string + */ + public function getUserIdentifier(): string; } From 32051818e61ff8b37bf8b2937fbbf00949c1e1c4 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 20 Jan 2025 16:22:38 +0100 Subject: [PATCH 09/15] [Security] Support hashing the hashed password using crc32c when putting the user in the session --- User/PasswordAuthenticatedUserInterface.php | 23 ++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/User/PasswordAuthenticatedUserInterface.php b/User/PasswordAuthenticatedUserInterface.php index 900963d..01613ec 100644 --- a/User/PasswordAuthenticatedUserInterface.php +++ b/User/PasswordAuthenticatedUserInterface.php @@ -14,6 +14,26 @@ /** * For users that can be authenticated using a password. * + * The __serialize/__unserialize() magic methods can be implemented on the user + * class to prevent hashed passwords from being put in the session storage. + * If the password is not stored at all in the session, getPassword() should + * return null after unserialization, and then, changing the user's password + * won't invalidate its sessions. + * In order to invalidate the user sessions while not storing the password hash + * in the session, it's also possible to hash the password hash before + * serializing it; crc32c is the only algorithm supported. + * For example: + * + * public function __serialize(): array + * { + * $data = (array) $this; + * $data["\0".self::class."\0password"] = hash('crc32c', $this->password); + * + * return $data; + * } + * + * Implement EquatableInteface if you need another logic. + * * @author Robin Chalas * @author Wouter de Jong */ @@ -23,9 +43,6 @@ interface PasswordAuthenticatedUserInterface * Returns the hashed password used to authenticate the user. * * Usually on authentication, a plain-text password will be compared to this value. - * - * The __serialize/__unserialize() magic methods can be implemented on the user - * class to prevent hashed passwords from being put in the session storage. */ public function getPassword(): ?string; } From 5e5c2181d0deae0e5b09e4fe8b42690618db8e64 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 27 Aug 2024 19:49:01 +0200 Subject: [PATCH 10/15] [Security] Add ability for voters to explain their vote --- Authorization/AccessDecision.php | 56 +++++++++++++ Authorization/AccessDecisionManager.php | 40 ++++++++-- .../AccessDecisionManagerInterface.php | 7 +- Authorization/AuthorizationChecker.php | 12 ++- .../AuthorizationCheckerInterface.php | 5 +- .../TraceableAccessDecisionManager.php | 79 +++++++++---------- Authorization/UserAuthorizationChecker.php | 4 +- .../UserAuthorizationCheckerInterface.php | 5 +- Authorization/Voter/AuthenticatedVoter.php | 33 ++++++-- Authorization/Voter/ExpressionVoter.php | 24 +++++- Authorization/Voter/RoleVoter.php | 17 +++- Authorization/Voter/TraceableVoter.php | 6 +- Authorization/Voter/Vote.php | 35 ++++++++ Authorization/Voter/Voter.php | 20 +++-- Authorization/Voter/VoterInterface.php | 7 +- CHANGELOG.md | 1 + Event/VoteEvent.php | 6 ++ Exception/AccessDeniedException.php | 12 +++ Test/AccessDecisionStrategyTestCase.php | 3 +- .../TraceableAccessDecisionManagerTest.php | 32 ++++---- Tests/Authorization/Voter/VoterTest.php | 7 +- Tests/Fixtures/DummyVoter.php | 3 +- 22 files changed, 311 insertions(+), 103 deletions(-) create mode 100644 Authorization/AccessDecision.php create mode 100644 Authorization/Voter/Vote.php diff --git a/Authorization/AccessDecision.php b/Authorization/AccessDecision.php new file mode 100644 index 0000000..d5465a1 --- /dev/null +++ b/Authorization/AccessDecision.php @@ -0,0 +1,56 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization; + +use Symfony\Component\Security\Core\Authorization\Voter\Vote; +use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; + +/** + * Contains the access verdict and all the related votes. + * + * @author Dany Maillard + * @author Roman JOLY + * @author Nicolas Grekas + */ +class AccessDecision +{ + /** + * @var class-string|string|null + */ + public ?string $strategy = null; + + public bool $isGranted; + + /** + * @var Vote[] + */ + public array $votes = []; + + public function getMessage(): string + { + $message = $this->isGranted ? 'Access Granted.' : 'Access Denied.'; + $access = $this->isGranted ? VoterInterface::ACCESS_GRANTED : VoterInterface::ACCESS_DENIED; + + if ($this->votes) { + foreach ($this->votes as $vote) { + if ($vote->result !== $access) { + continue; + } + foreach ($vote->reasons as $reason) { + $message .= ' '.$reason; + } + } + } + + return $message; + } +} diff --git a/Authorization/AccessDecisionManager.php b/Authorization/AccessDecisionManager.php index 3e42c4b..fd43538 100644 --- a/Authorization/AccessDecisionManager.php +++ b/Authorization/AccessDecisionManager.php @@ -15,6 +15,8 @@ use Symfony\Component\Security\Core\Authorization\Strategy\AccessDecisionStrategyInterface; use Symfony\Component\Security\Core\Authorization\Strategy\AffirmativeStrategy; use Symfony\Component\Security\Core\Authorization\Voter\CacheableVoterInterface; +use Symfony\Component\Security\Core\Authorization\Voter\TraceableVoter; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; use Symfony\Component\Security\Core\Exception\InvalidArgumentException; @@ -35,6 +37,7 @@ final class AccessDecisionManager implements AccessDecisionManagerInterface private array $votersCacheAttributes = []; private array $votersCacheObject = []; private AccessDecisionStrategyInterface $strategy; + private array $accessDecisionStack = []; /** * @param iterable $voters An array or an iterator of VoterInterface instances @@ -49,35 +52,56 @@ public function __construct( /** * @param bool $allowMultipleAttributes Whether to allow passing multiple values to the $attributes array */ - public function decide(TokenInterface $token, array $attributes, mixed $object = null, bool $allowMultipleAttributes = false): bool + public function decide(TokenInterface $token, array $attributes, mixed $object = null, bool|AccessDecision|null $accessDecision = null, bool $allowMultipleAttributes = false): bool { + if (\is_bool($accessDecision)) { + $allowMultipleAttributes = $accessDecision; + $accessDecision = null; + } + // Special case for AccessListener, do not remove the right side of the condition before 6.0 if (\count($attributes) > 1 && !$allowMultipleAttributes) { throw new InvalidArgumentException(\sprintf('Passing more than one Security attribute to "%s()" is not supported.', __METHOD__)); } - return $this->strategy->decide( - $this->collectResults($token, $attributes, $object) - ); + $accessDecision ??= end($this->accessDecisionStack) ?: new AccessDecision(); + $this->accessDecisionStack[] = $accessDecision; + + $accessDecision->strategy = $this->strategy instanceof \Stringable ? $this->strategy : get_debug_type($this->strategy); + + try { + return $accessDecision->isGranted = $this->strategy->decide( + $this->collectResults($token, $attributes, $object, $accessDecision) + ); + } finally { + array_pop($this->accessDecisionStack); + } } /** - * @return \Traversable + * @return \Traversable */ - private function collectResults(TokenInterface $token, array $attributes, mixed $object): \Traversable + private function collectResults(TokenInterface $token, array $attributes, mixed $object, AccessDecision $accessDecision): \Traversable { foreach ($this->getVoters($attributes, $object) as $voter) { - $result = $voter->vote($token, $object, $attributes); + $vote = new Vote(); + $result = $voter->vote($token, $object, $attributes, $vote); + if (!\is_int($result) || !(self::VALID_VOTES[$result] ?? false)) { throw new \LogicException(\sprintf('"%s::vote()" must return one of "%s" constants ("ACCESS_GRANTED", "ACCESS_DENIED" or "ACCESS_ABSTAIN"), "%s" returned.', get_debug_type($voter), VoterInterface::class, var_export($result, true))); } + $voter = $voter instanceof TraceableVoter ? $voter->getDecoratedVoter() : $voter; + $vote->voter = $voter instanceof \Stringable ? $voter : get_debug_type($voter); + $vote->result = $result; + $accessDecision->votes[] = $vote; + yield $result; } } /** - * @return iterable + * @return iterable */ private function getVoters(array $attributes, $object = null): iterable { diff --git a/Authorization/AccessDecisionManagerInterface.php b/Authorization/AccessDecisionManagerInterface.php index f25c7e1..cb4a331 100644 --- a/Authorization/AccessDecisionManagerInterface.php +++ b/Authorization/AccessDecisionManagerInterface.php @@ -23,8 +23,9 @@ interface AccessDecisionManagerInterface /** * Decides whether the access is possible or not. * - * @param array $attributes An array of attributes associated with the method being invoked - * @param mixed $object The object to secure + * @param array $attributes An array of attributes associated with the method being invoked + * @param mixed $object The object to secure + * @param AccessDecision|null $accessDecision Should be used to explain the decision */ - public function decide(TokenInterface $token, array $attributes, mixed $object = null): bool; + public function decide(TokenInterface $token, array $attributes, mixed $object = null/* , ?AccessDecision $accessDecision = null */): bool; } diff --git a/Authorization/AuthorizationChecker.php b/Authorization/AuthorizationChecker.php index c748697..3960f2b 100644 --- a/Authorization/AuthorizationChecker.php +++ b/Authorization/AuthorizationChecker.php @@ -24,20 +24,28 @@ */ class AuthorizationChecker implements AuthorizationCheckerInterface { + private array $accessDecisionStack = []; + public function __construct( private TokenStorageInterface $tokenStorage, private AccessDecisionManagerInterface $accessDecisionManager, ) { } - final public function isGranted(mixed $attribute, mixed $subject = null): bool + final public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool { $token = $this->tokenStorage->getToken(); if (!$token || !$token->getUser()) { $token = new NullToken(); } + $accessDecision ??= end($this->accessDecisionStack) ?: new AccessDecision(); + $this->accessDecisionStack[] = $accessDecision; - return $this->accessDecisionManager->decide($token, [$attribute], $subject); + try { + return $accessDecision->isGranted = $this->accessDecisionManager->decide($token, [$attribute], $subject, $accessDecision); + } finally { + array_pop($this->accessDecisionStack); + } } } diff --git a/Authorization/AuthorizationCheckerInterface.php b/Authorization/AuthorizationCheckerInterface.php index 6f5a602..7c673df 100644 --- a/Authorization/AuthorizationCheckerInterface.php +++ b/Authorization/AuthorizationCheckerInterface.php @@ -21,7 +21,8 @@ interface AuthorizationCheckerInterface /** * Checks if the attribute is granted against the current authentication token and optionally supplied subject. * - * @param mixed $attribute A single attribute to vote on (can be of any type, string and instance of Expression are supported by the core) + * @param mixed $attribute A single attribute to vote on (can be of any type, string and instance of Expression are supported by the core) + * @param AccessDecision|null $accessDecision Should be used to explain the decision */ - public function isGranted(mixed $attribute, mixed $subject = null): bool; + public function isGranted(mixed $attribute, mixed $subject = null/* , ?AccessDecision $accessDecision = null */): bool; } diff --git a/Authorization/TraceableAccessDecisionManager.php b/Authorization/TraceableAccessDecisionManager.php index 0b82eb3..a03e2d0 100644 --- a/Authorization/TraceableAccessDecisionManager.php +++ b/Authorization/TraceableAccessDecisionManager.php @@ -12,7 +12,6 @@ namespace Symfony\Component\Security\Core\Authorization; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; -use Symfony\Component\Security\Core\Authorization\Strategy\AccessDecisionStrategyInterface; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; /** @@ -25,77 +24,68 @@ */ class TraceableAccessDecisionManager implements AccessDecisionManagerInterface { - private ?AccessDecisionStrategyInterface $strategy = null; - /** @var iterable */ - private iterable $voters = []; + private ?string $strategy = null; + /** @var array */ + private array $voters = []; private array $decisionLog = []; // All decision logs private array $currentLog = []; // Logs being filled in + private array $accessDecisionStack = []; public function __construct( private AccessDecisionManagerInterface $manager, ) { - // The strategy and voters are stored in a private properties of the decorated service - if (property_exists($manager, 'strategy')) { - $reflection = new \ReflectionProperty($manager::class, 'strategy'); - $this->strategy = $reflection->getValue($manager); - } - if (property_exists($manager, 'voters')) { - $reflection = new \ReflectionProperty($manager::class, 'voters'); - $this->voters = $reflection->getValue($manager); - } } - public function decide(TokenInterface $token, array $attributes, mixed $object = null, bool $allowMultipleAttributes = false): bool + public function decide(TokenInterface $token, array $attributes, mixed $object = null, bool|AccessDecision|null $accessDecision = null, bool $allowMultipleAttributes = false): bool { - $currentDecisionLog = [ + if (\is_bool($accessDecision)) { + $allowMultipleAttributes = $accessDecision; + $accessDecision = null; + } + + // Using a stack since decide can be called by voters + $this->currentLog[] = [ 'attributes' => $attributes, 'object' => $object, 'voterDetails' => [], ]; - $this->currentLog[] = &$currentDecisionLog; - - $result = $this->manager->decide($token, $attributes, $object, $allowMultipleAttributes); - - $currentDecisionLog['result'] = $result; - - $this->decisionLog[] = array_pop($this->currentLog); // Using a stack since decide can be called by voters - - return $result; + $accessDecision ??= end($this->accessDecisionStack) ?: new AccessDecision(); + $this->accessDecisionStack[] = $accessDecision; + + try { + return $accessDecision->isGranted = $this->manager->decide($token, $attributes, $object, $accessDecision); + } finally { + $this->strategy = $accessDecision->strategy; + $currentLog = array_pop($this->currentLog); + if (isset($accessDecision->isGranted)) { + $currentLog['result'] = $accessDecision->isGranted; + } + $this->decisionLog[] = $currentLog; + } } - /** - * Adds voter vote and class to the voter details. - * - * @param array $attributes attributes used for the vote - * @param int $vote vote of the voter - */ - public function addVoterVote(VoterInterface $voter, array $attributes, int $vote): void + public function addVoterVote(VoterInterface $voter, array $attributes, int $vote, array $reasons = []): void { $currentLogIndex = \count($this->currentLog) - 1; $this->currentLog[$currentLogIndex]['voterDetails'][] = [ 'voter' => $voter, 'attributes' => $attributes, 'vote' => $vote, + 'reasons' => $reasons, ]; + $this->voters[$voter::class] = $voter; } public function getStrategy(): string { - if (null === $this->strategy) { - return '-'; - } - if ($this->strategy instanceof \Stringable) { - return (string) $this->strategy; - } - - return get_debug_type($this->strategy); + return $this->strategy ?? '-'; } /** - * @return iterable + * @return array */ - public function getVoters(): iterable + public function getVoters(): array { return $this->voters; } @@ -104,4 +94,11 @@ public function getDecisionLog(): array { return $this->decisionLog; } + + public function reset(): void + { + $this->strategy = null; + $this->voters = []; + $this->decisionLog = []; + } } diff --git a/Authorization/UserAuthorizationChecker.php b/Authorization/UserAuthorizationChecker.php index f5ba7b8..f515e5c 100644 --- a/Authorization/UserAuthorizationChecker.php +++ b/Authorization/UserAuthorizationChecker.php @@ -24,8 +24,8 @@ public function __construct( ) { } - public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null): bool + public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool { - return $this->accessDecisionManager->decide(new UserAuthorizationCheckerToken($user), [$attribute], $subject); + return $this->accessDecisionManager->decide(new UserAuthorizationCheckerToken($user), [$attribute], $subject, $accessDecision); } } diff --git a/Authorization/UserAuthorizationCheckerInterface.php b/Authorization/UserAuthorizationCheckerInterface.php index 3335e6f..15e5b4d 100644 --- a/Authorization/UserAuthorizationCheckerInterface.php +++ b/Authorization/UserAuthorizationCheckerInterface.php @@ -23,7 +23,8 @@ interface UserAuthorizationCheckerInterface /** * Checks if the attribute is granted against the user and optionally supplied subject. * - * @param mixed $attribute A single attribute to vote on (can be of any type, string and instance of Expression are supported by the core) + * @param mixed $attribute A single attribute to vote on (can be of any type, string and instance of Expression are supported by the core) + * @param AccessDecision|null $accessDecision Should be used to explain the decision */ - public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null): bool; + public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool; } diff --git a/Authorization/Voter/AuthenticatedVoter.php b/Authorization/Voter/AuthenticatedVoter.php index a073f61..1403aaa 100644 --- a/Authorization/Voter/AuthenticatedVoter.php +++ b/Authorization/Voter/AuthenticatedVoter.php @@ -40,9 +40,17 @@ public function __construct( ) { } - public function vote(TokenInterface $token, mixed $subject, array $attributes): int + /** + * @param Vote|null $vote Should be used to explain the vote + */ + public function vote(TokenInterface $token, mixed $subject, array $attributes/* , ?Vote $vote = null */): int { + $vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote(); + $vote ??= new Vote(); + if ($attributes === [self::PUBLIC_ACCESS]) { + $vote->reasons[] = 'Access is public.'; + return VoterInterface::ACCESS_GRANTED; } @@ -62,30 +70,45 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes): $result = VoterInterface::ACCESS_DENIED; - if (self::IS_AUTHENTICATED_FULLY === $attribute - && $this->authenticationTrustResolver->isFullFledged($token)) { + if ((self::IS_AUTHENTICATED_FULLY === $attribute || self::IS_AUTHENTICATED_REMEMBERED === $attribute) + && $this->authenticationTrustResolver->isFullFledged($token) + ) { + $vote->reasons[] = 'The user is fully authenticated.'; + return VoterInterface::ACCESS_GRANTED; } if (self::IS_AUTHENTICATED_REMEMBERED === $attribute - && ($this->authenticationTrustResolver->isRememberMe($token) - || $this->authenticationTrustResolver->isFullFledged($token))) { + && $this->authenticationTrustResolver->isRememberMe($token) + ) { + $vote->reasons[] = 'The user is remembered.'; + return VoterInterface::ACCESS_GRANTED; } if (self::IS_AUTHENTICATED === $attribute && $this->authenticationTrustResolver->isAuthenticated($token)) { + $vote->reasons[] = 'The user is authenticated.'; + return VoterInterface::ACCESS_GRANTED; } if (self::IS_REMEMBERED === $attribute && $this->authenticationTrustResolver->isRememberMe($token)) { + $vote->reasons[] = 'The user is remembered.'; + return VoterInterface::ACCESS_GRANTED; } if (self::IS_IMPERSONATOR === $attribute && $token instanceof SwitchUserToken) { + $vote->reasons[] = 'The user is impersonating another user.'; + return VoterInterface::ACCESS_GRANTED; } } + if (VoterInterface::ACCESS_DENIED === $result) { + $vote->reasons[] = 'The user is not appropriately authenticated.'; + } + return $result; } diff --git a/Authorization/Voter/ExpressionVoter.php b/Authorization/Voter/ExpressionVoter.php index bab3283..35d727a 100644 --- a/Authorization/Voter/ExpressionVoter.php +++ b/Authorization/Voter/ExpressionVoter.php @@ -28,7 +28,7 @@ class ExpressionVoter implements CacheableVoterInterface { public function __construct( private ExpressionLanguage $expressionLanguage, - private AuthenticationTrustResolverInterface $trustResolver, + private ?AuthenticationTrustResolverInterface $trustResolver, private AuthorizationCheckerInterface $authChecker, private ?RoleHierarchyInterface $roleHierarchy = null, ) { @@ -44,10 +44,16 @@ public function supportsType(string $subjectType): bool return true; } - public function vote(TokenInterface $token, mixed $subject, array $attributes): int + /** + * @param Vote|null $vote Should be used to explain the vote + */ + public function vote(TokenInterface $token, mixed $subject, array $attributes/* , ?Vote $vote = null */): int { + $vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote(); + $vote ??= new Vote(); $result = VoterInterface::ACCESS_ABSTAIN; $variables = null; + $failingExpressions = []; foreach ($attributes as $attribute) { if (!$attribute instanceof Expression) { continue; @@ -56,9 +62,18 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes): $variables ??= $this->getVariables($token, $subject); $result = VoterInterface::ACCESS_DENIED; + if ($this->expressionLanguage->evaluate($attribute, $variables)) { + $vote->reasons[] = \sprintf('Expression (%s) is true.', $attribute); + return VoterInterface::ACCESS_GRANTED; } + + $failingExpressions[] = $attribute; + } + + if ($failingExpressions) { + $vote->reasons[] = \sprintf('Expression (%s) is false.', implode(') || (', $failingExpressions)); } return $result; @@ -78,10 +93,13 @@ private function getVariables(TokenInterface $token, mixed $subject): array 'object' => $subject, 'subject' => $subject, 'role_names' => $roleNames, - 'trust_resolver' => $this->trustResolver, 'auth_checker' => $this->authChecker, ]; + if ($this->trustResolver) { + $variables['trust_resolver'] = $this->trustResolver; + } + // this is mainly to propose a better experience when the expression is used // in an access control rule, as the developer does not know that it's going // to be handled by this voter diff --git a/Authorization/Voter/RoleVoter.php b/Authorization/Voter/RoleVoter.php index 3c65fb6..46c08d1 100644 --- a/Authorization/Voter/RoleVoter.php +++ b/Authorization/Voter/RoleVoter.php @@ -25,10 +25,16 @@ public function __construct( ) { } - public function vote(TokenInterface $token, mixed $subject, array $attributes): int + /** + * @param Vote|null $vote Should be used to explain the vote + */ + public function vote(TokenInterface $token, mixed $subject, array $attributes/* , ?Vote $vote = null */): int { + $vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote(); + $vote ??= new Vote(); $result = VoterInterface::ACCESS_ABSTAIN; $roles = $this->extractRoles($token); + $missingRoles = []; foreach ($attributes as $attribute) { if (!\is_string($attribute) || !str_starts_with($attribute, $this->prefix)) { @@ -36,9 +42,18 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes): } $result = VoterInterface::ACCESS_DENIED; + if (\in_array($attribute, $roles, true)) { + $vote->reasons[] = \sprintf('The user has %s.', $attribute); + return VoterInterface::ACCESS_GRANTED; } + + $missingRoles[] = $attribute; + } + + if (VoterInterface::ACCESS_DENIED === $result) { + $vote->reasons[] = \sprintf('The user doesn\'t have%s %s.', 1 < \count($missingRoles) ? ' any of' : '', implode(', ', $missingRoles)); } return $result; diff --git a/Authorization/Voter/TraceableVoter.php b/Authorization/Voter/TraceableVoter.php index 1abc7c7..4757279 100644 --- a/Authorization/Voter/TraceableVoter.php +++ b/Authorization/Voter/TraceableVoter.php @@ -30,11 +30,11 @@ public function __construct( ) { } - public function vote(TokenInterface $token, mixed $subject, array $attributes): int + public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int { - $result = $this->voter->vote($token, $subject, $attributes); + $result = $this->voter->vote($token, $subject, $attributes, $vote ??= new Vote()); - $this->eventDispatcher->dispatch(new VoteEvent($this->voter, $subject, $attributes, $result), 'debug.security.authorization.vote'); + $this->eventDispatcher->dispatch(new VoteEvent($this->voter, $subject, $attributes, $result, $vote->reasons), 'debug.security.authorization.vote'); return $result; } diff --git a/Authorization/Voter/Vote.php b/Authorization/Voter/Vote.php new file mode 100644 index 0000000..e933c57 --- /dev/null +++ b/Authorization/Voter/Vote.php @@ -0,0 +1,35 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization\Voter; + +class Vote +{ + /** + * @var class-string|string + */ + public string $voter; + + /** + * @var VoterInterface::ACCESS_* + */ + public int $result; + + /** + * @var list + */ + public array $reasons = []; + + public function addReason(string $reason): void + { + $this->reasons[] = $reason; + } +} diff --git a/Authorization/Voter/Voter.php b/Authorization/Voter/Voter.php index 1f76a42..3d7fd9e 100644 --- a/Authorization/Voter/Voter.php +++ b/Authorization/Voter/Voter.php @@ -24,10 +24,15 @@ */ abstract class Voter implements VoterInterface, CacheableVoterInterface { - public function vote(TokenInterface $token, mixed $subject, array $attributes): int + /** + * @param Vote|null $vote Should be used to explain the vote + */ + public function vote(TokenInterface $token, mixed $subject, array $attributes/* , ?Vote $vote = null */): int { + $vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote(); + $vote ??= new Vote(); // abstain vote by default in case none of the attributes are supported - $vote = self::ACCESS_ABSTAIN; + $vote->result = self::ACCESS_ABSTAIN; foreach ($attributes as $attribute) { try { @@ -43,15 +48,15 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes): } // as soon as at least one attribute is supported, default is to deny access - $vote = self::ACCESS_DENIED; + $vote->result = self::ACCESS_DENIED; - if ($this->voteOnAttribute($attribute, $subject, $token)) { + if ($this->voteOnAttribute($attribute, $subject, $token, $vote)) { // grant access as soon as at least one attribute returns a positive response - return self::ACCESS_GRANTED; + return $vote->result = self::ACCESS_GRANTED; } } - return $vote; + return $vote->result; } /** @@ -90,6 +95,7 @@ abstract protected function supports(string $attribute, mixed $subject): bool; * * @param TAttribute $attribute * @param TSubject $subject + * @param Vote|null $vote Should be used to explain the vote */ - abstract protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool; + abstract protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token/* , ?Vote $vote = null */): bool; } diff --git a/Authorization/Voter/VoterInterface.php b/Authorization/Voter/VoterInterface.php index 5255c88..0902a94 100644 --- a/Authorization/Voter/VoterInterface.php +++ b/Authorization/Voter/VoterInterface.php @@ -30,10 +30,11 @@ interface VoterInterface * This method must return one of the following constants: * ACCESS_GRANTED, ACCESS_DENIED, or ACCESS_ABSTAIN. * - * @param mixed $subject The subject to secure - * @param array $attributes An array of attributes associated with the method being invoked + * @param mixed $subject The subject to secure + * @param array $attributes An array of attributes associated with the method being invoked + * @param Vote|null $vote Should be used to explain the vote * * @return self::ACCESS_* */ - public function vote(TokenInterface $token, mixed $subject, array $attributes): int; + public function vote(TokenInterface $token, mixed $subject, array $attributes/* , ?Vote $vote = null */): int; } diff --git a/CHANGELOG.md b/CHANGELOG.md index 290aa8b..331b204 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ CHANGELOG * Add `OfflineTokenInterface` to mark tokens that do not represent the currently logged-in user * Deprecate `UserInterface::eraseCredentials()` and `TokenInterface::eraseCredentials()`, erase credentials e.g. using `__serialize()` instead + * Add ability for voters to explain their vote 7.2 --- diff --git a/Event/VoteEvent.php b/Event/VoteEvent.php index edc66b3..5842c54 100644 --- a/Event/VoteEvent.php +++ b/Event/VoteEvent.php @@ -28,6 +28,7 @@ public function __construct( private mixed $subject, private array $attributes, private int $vote, + private array $reasons = [], ) { } @@ -50,4 +51,9 @@ public function getVote(): int { return $this->vote; } + + public function getReasons(): array + { + return $this->reasons; + } } diff --git a/Exception/AccessDeniedException.php b/Exception/AccessDeniedException.php index 93c3869..a3e5747 100644 --- a/Exception/AccessDeniedException.php +++ b/Exception/AccessDeniedException.php @@ -12,6 +12,7 @@ namespace Symfony\Component\Security\Core\Exception; use Symfony\Component\HttpKernel\Attribute\WithHttpStatus; +use Symfony\Component\Security\Core\Authorization\AccessDecision; /** * AccessDeniedException is thrown when the account has not the required role. @@ -23,6 +24,7 @@ class AccessDeniedException extends RuntimeException { private array $attributes = []; private mixed $subject = null; + private ?AccessDecision $accessDecision = null; public function __construct(string $message = 'Access Denied.', ?\Throwable $previous = null, int $code = 403) { @@ -48,4 +50,14 @@ public function setSubject(mixed $subject): void { $this->subject = $subject; } + + public function setAccessDecision(AccessDecision $accessDecision): void + { + $this->accessDecision = $accessDecision; + } + + public function getAccessDecision(): ?AccessDecision + { + return $this->accessDecision; + } } diff --git a/Test/AccessDecisionStrategyTestCase.php b/Test/AccessDecisionStrategyTestCase.php index 792e777..563a613 100644 --- a/Test/AccessDecisionStrategyTestCase.php +++ b/Test/AccessDecisionStrategyTestCase.php @@ -16,6 +16,7 @@ use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Authorization\AccessDecisionManager; use Symfony\Component\Security\Core\Authorization\Strategy\AccessDecisionStrategyInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; /** @@ -71,7 +72,7 @@ public function __construct( ) { } - public function vote(TokenInterface $token, $subject, array $attributes): int + public function vote(TokenInterface $token, $subject, array $attributes, ?Vote $vote = null): int { return $this->vote; } diff --git a/Tests/Authorization/TraceableAccessDecisionManagerTest.php b/Tests/Authorization/TraceableAccessDecisionManagerTest.php index 8797d74..f5313bb 100644 --- a/Tests/Authorization/TraceableAccessDecisionManagerTest.php +++ b/Tests/Authorization/TraceableAccessDecisionManagerTest.php @@ -61,8 +61,8 @@ public static function provideObjectsAndLogs(): \Generator 'object' => null, 'result' => true, 'voterDetails' => [ - ['voter' => $voter1, 'attributes' => ['ATTRIBUTE_1'], 'vote' => VoterInterface::ACCESS_GRANTED], - ['voter' => $voter2, 'attributes' => ['ATTRIBUTE_1'], 'vote' => VoterInterface::ACCESS_GRANTED], + ['voter' => $voter1, 'attributes' => ['ATTRIBUTE_1'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []], + ['voter' => $voter2, 'attributes' => ['ATTRIBUTE_1'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []], ], ]], ['ATTRIBUTE_1'], @@ -79,8 +79,8 @@ public static function provideObjectsAndLogs(): \Generator 'object' => true, 'result' => false, 'voterDetails' => [ - ['voter' => $voter1, 'attributes' => ['ATTRIBUTE_1', 'ATTRIBUTE_2'], 'vote' => VoterInterface::ACCESS_ABSTAIN], - ['voter' => $voter2, 'attributes' => ['ATTRIBUTE_1', 'ATTRIBUTE_2'], 'vote' => VoterInterface::ACCESS_GRANTED], + ['voter' => $voter1, 'attributes' => ['ATTRIBUTE_1', 'ATTRIBUTE_2'], 'vote' => VoterInterface::ACCESS_ABSTAIN, 'reasons' => []], + ['voter' => $voter2, 'attributes' => ['ATTRIBUTE_1', 'ATTRIBUTE_2'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []], ], ]], ['ATTRIBUTE_1', 'ATTRIBUTE_2'], @@ -97,8 +97,8 @@ public static function provideObjectsAndLogs(): \Generator 'object' => 'jolie string', 'result' => false, 'voterDetails' => [ - ['voter' => $voter1, 'attributes' => [null], 'vote' => VoterInterface::ACCESS_ABSTAIN], - ['voter' => $voter2, 'attributes' => [null], 'vote' => VoterInterface::ACCESS_DENIED], + ['voter' => $voter1, 'attributes' => [null], 'vote' => VoterInterface::ACCESS_ABSTAIN, 'reasons' => []], + ['voter' => $voter2, 'attributes' => [null], 'vote' => VoterInterface::ACCESS_DENIED, 'reasons' => []], ], ]], [null], @@ -139,8 +139,8 @@ public static function provideObjectsAndLogs(): \Generator 'object' => $x = [], 'result' => false, 'voterDetails' => [ - ['voter' => $voter1, 'attributes' => ['ATTRIBUTE_2'], 'vote' => VoterInterface::ACCESS_ABSTAIN], - ['voter' => $voter2, 'attributes' => ['ATTRIBUTE_2'], 'vote' => VoterInterface::ACCESS_ABSTAIN], + ['voter' => $voter1, 'attributes' => ['ATTRIBUTE_2'], 'vote' => VoterInterface::ACCESS_ABSTAIN, 'reasons' => []], + ['voter' => $voter2, 'attributes' => ['ATTRIBUTE_2'], 'vote' => VoterInterface::ACCESS_ABSTAIN, 'reasons' => []], ], ]], ['ATTRIBUTE_2'], @@ -157,8 +157,8 @@ public static function provideObjectsAndLogs(): \Generator 'object' => new \stdClass(), 'result' => false, 'voterDetails' => [ - ['voter' => $voter1, 'attributes' => [12.13], 'vote' => VoterInterface::ACCESS_DENIED], - ['voter' => $voter2, 'attributes' => [12.13], 'vote' => VoterInterface::ACCESS_DENIED], + ['voter' => $voter1, 'attributes' => [12.13], 'vote' => VoterInterface::ACCESS_DENIED, 'reasons' => []], + ['voter' => $voter2, 'attributes' => [12.13], 'vote' => VoterInterface::ACCESS_DENIED, 'reasons' => []], ], ]], [12.13], @@ -242,7 +242,7 @@ public function testAccessDecisionManagerCalledByVoter() 'attributes' => ['attr1'], 'object' => null, 'voterDetails' => [ - ['voter' => $voter1, 'attributes' => ['attr1'], 'vote' => VoterInterface::ACCESS_GRANTED], + ['voter' => $voter1, 'attributes' => ['attr1'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []], ], 'result' => true, ], @@ -250,8 +250,8 @@ public function testAccessDecisionManagerCalledByVoter() 'attributes' => ['attr2'], 'object' => null, 'voterDetails' => [ - ['voter' => $voter1, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_ABSTAIN], - ['voter' => $voter2, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_GRANTED], + ['voter' => $voter1, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_ABSTAIN, 'reasons' => []], + ['voter' => $voter2, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []], ], 'result' => true, ], @@ -259,9 +259,9 @@ public function testAccessDecisionManagerCalledByVoter() 'attributes' => ['attr2'], 'object' => $obj, 'voterDetails' => [ - ['voter' => $voter1, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_ABSTAIN], - ['voter' => $voter2, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_DENIED], - ['voter' => $voter3, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_GRANTED], + ['voter' => $voter1, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_ABSTAIN, 'reasons' => []], + ['voter' => $voter2, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_DENIED, 'reasons' => []], + ['voter' => $voter3, 'attributes' => ['attr2'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []], ], 'result' => true, ], diff --git a/Tests/Authorization/Voter/VoterTest.php b/Tests/Authorization/Voter/VoterTest.php index 602c61a..a8f87e0 100644 --- a/Tests/Authorization/Voter/VoterTest.php +++ b/Tests/Authorization/Voter/VoterTest.php @@ -13,6 +13,7 @@ use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; use Symfony\Component\Security\Core\Authorization\Voter\Voter; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; @@ -73,7 +74,7 @@ public function testVoteWithTypeError() class VoterTest_Voter extends Voter { - protected function voteOnAttribute(string $attribute, $object, TokenInterface $token): bool + protected function voteOnAttribute(string $attribute, $object, TokenInterface $token, ?Vote $vote = null): bool { return 'EDIT' === $attribute; } @@ -86,7 +87,7 @@ protected function supports(string $attribute, $object): bool class IntegerVoterTest_Voter extends Voter { - protected function voteOnAttribute($attribute, $object, TokenInterface $token): bool + protected function voteOnAttribute($attribute, $object, TokenInterface $token, ?Vote $vote = null): bool { return 42 === $attribute; } @@ -99,7 +100,7 @@ protected function supports($attribute, $object): bool class TypeErrorVoterTest_Voter extends Voter { - protected function voteOnAttribute($attribute, $object, TokenInterface $token): bool + protected function voteOnAttribute($attribute, $object, TokenInterface $token, ?Vote $vote = null): bool { return false; } diff --git a/Tests/Fixtures/DummyVoter.php b/Tests/Fixtures/DummyVoter.php index 1f92342..16ae8e8 100644 --- a/Tests/Fixtures/DummyVoter.php +++ b/Tests/Fixtures/DummyVoter.php @@ -12,11 +12,12 @@ namespace Symfony\Component\Security\Core\Tests\Fixtures; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; final class DummyVoter implements VoterInterface { - public function vote(TokenInterface $token, $subject, array $attributes): int + public function vote(TokenInterface $token, $subject, array $attributes, ?Vote $vote = null): int { } } From 4f5954498c035459e0ce0a5a37be83dc8adbcb09 Mon Sep 17 00:00:00 2001 From: Alexandre Daubois Date: Mon, 9 Dec 2024 08:56:13 +0100 Subject: [PATCH 11/15] [Security] Allow using a callable with `#[IsGranted]` --- .../AuthorizationCheckerInterface.php | 2 +- Authorization/Voter/ClosureVoter.php | 78 ++++++++++++++++ CHANGELOG.md | 1 + .../Authorization/Voter/ClosureVoterTest.php | 91 +++++++++++++++++++ 4 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 Authorization/Voter/ClosureVoter.php create mode 100644 Tests/Authorization/Voter/ClosureVoterTest.php diff --git a/Authorization/AuthorizationCheckerInterface.php b/Authorization/AuthorizationCheckerInterface.php index 7c673df..848b17e 100644 --- a/Authorization/AuthorizationCheckerInterface.php +++ b/Authorization/AuthorizationCheckerInterface.php @@ -21,7 +21,7 @@ interface AuthorizationCheckerInterface /** * Checks if the attribute is granted against the current authentication token and optionally supplied subject. * - * @param mixed $attribute A single attribute to vote on (can be of any type, string and instance of Expression are supported by the core) + * @param mixed $attribute A single attribute to vote on (can be of any type; strings, Expression and Closure instances are supported by the core) * @param AccessDecision|null $accessDecision Should be used to explain the decision */ public function isGranted(mixed $attribute, mixed $subject = null/* , ?AccessDecision $accessDecision = null */): bool; diff --git a/Authorization/Voter/ClosureVoter.php b/Authorization/Voter/ClosureVoter.php new file mode 100644 index 0000000..23140c8 --- /dev/null +++ b/Authorization/Voter/ClosureVoter.php @@ -0,0 +1,78 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization\Voter; + +use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface; +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; +use Symfony\Component\Security\Http\Attribute\IsGranted; + +/** + * This voter allows using a closure as the attribute being voted on. + * + * The following named arguments are passed to the closure: + * + * - `token`: The token being used for voting + * - `subject`: The subject of the vote + * - `accessDecisionManager`: The access decision manager + * - `trustResolver`: The trust resolver + * + * @see IsGranted doc for the complete closure signature. + * + * @author Alexandre Daubois + */ +final class ClosureVoter implements CacheableVoterInterface +{ + public function __construct( + private AccessDecisionManagerInterface $accessDecisionManager, + private AuthenticationTrustResolverInterface $trustResolver, + ) { + } + + public function supportsAttribute(string $attribute): bool + { + return false; + } + + public function supportsType(string $subjectType): bool + { + return true; + } + + public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int + { + $vote ??= new Vote(); + $failingClosures = []; + $result = VoterInterface::ACCESS_ABSTAIN; + foreach ($attributes as $attribute) { + if (!$attribute instanceof \Closure) { + continue; + } + + $name = (new \ReflectionFunction($attribute))->name; + $result = VoterInterface::ACCESS_DENIED; + if ($attribute(token: $token, subject: $subject, accessDecisionManager: $this->accessDecisionManager, trustResolver: $this->trustResolver)) { + $vote->reasons[] = \sprintf('Closure %s returned true.', $name); + + return VoterInterface::ACCESS_GRANTED; + } + + $failingClosures[] = $name; + } + + if ($failingClosures) { + $vote->reasons[] = \sprintf('Closure%s %s returned false.', 1 < \count($failingClosures) ? 's' : '', implode(', ', $failingClosures)); + } + + return $result; + } +} diff --git a/CHANGELOG.md b/CHANGELOG.md index 331b204..12a0c0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ CHANGELOG * Deprecate `UserInterface::eraseCredentials()` and `TokenInterface::eraseCredentials()`, erase credentials e.g. using `__serialize()` instead * Add ability for voters to explain their vote + * Add support for voting on closures 7.2 --- diff --git a/Tests/Authorization/Voter/ClosureVoterTest.php b/Tests/Authorization/Voter/ClosureVoterTest.php new file mode 100644 index 0000000..a919916 --- /dev/null +++ b/Tests/Authorization/Voter/ClosureVoterTest.php @@ -0,0 +1,91 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Tests\Authorization\Voter; + +use PHPUnit\Framework\TestCase; +use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface; +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; +use Symfony\Component\Security\Core\Authorization\Voter\ClosureVoter; +use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; +use Symfony\Component\Security\Core\User\UserInterface; + +class ClosureVoterTest extends TestCase +{ + private ClosureVoter $voter; + + protected function setUp(): void + { + $this->voter = new ClosureVoter( + $this->createMock(AccessDecisionManagerInterface::class), + $this->createMock(AuthenticationTrustResolverInterface::class), + ); + } + + public function testEmptyAttributeAbstains() + { + $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $this->voter->vote( + $this->createMock(TokenInterface::class), + null, + []) + ); + } + + public function testClosureReturningFalseDeniesAccess() + { + $token = $this->createMock(TokenInterface::class); + $token->method('getRoleNames')->willReturn([]); + $token->method('getUser')->willReturn($this->createMock(UserInterface::class)); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->voter->vote( + $token, + null, + [fn (...$vars) => false] + )); + } + + public function testClosureReturningTrueGrantsAccess() + { + $token = $this->createMock(TokenInterface::class); + $token->method('getRoleNames')->willReturn([]); + $token->method('getUser')->willReturn($this->createMock(UserInterface::class)); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->voter->vote( + $token, + null, + [fn (...$vars) => true] + )); + } + + public function testArgumentsContent() + { + $token = $this->createMock(TokenInterface::class); + $token->method('getRoleNames')->willReturn(['MY_ROLE', 'ANOTHER_ROLE']); + $token->method('getUser')->willReturn($this->createMock(UserInterface::class)); + + $outerSubject = new \stdClass(); + + $this->voter->vote( + $token, + $outerSubject, + [function (...$vars) use ($outerSubject) { + $this->assertInstanceOf(TokenInterface::class, $vars['token']); + $this->assertSame($outerSubject, $vars['subject']); + + $this->assertInstanceOf(AccessDecisionManagerInterface::class, $vars['accessDecisionManager']); + $this->assertInstanceOf(AuthenticationTrustResolverInterface::class, $vars['trustResolver']); + + return true; + }] + ); + } +} From 97778ec867f16b74baaec312a9ed0e70b8d5eaa6 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 17 Feb 2025 16:31:27 +0100 Subject: [PATCH 12/15] [Security] Improve DX of recent additions --- .../Token/UserAuthorizationCheckerToken.php | 31 -------- Authorization/AuthorizationChecker.php | 21 +++++- Authorization/UserAuthorizationChecker.php | 31 -------- Authorization/Voter/ClosureVoter.php | 17 ++--- CHANGELOG.md | 3 +- .../UserAuthorizationCheckerTokenTest.php | 26 ------- .../AuthorizationCheckerTest.php | 39 +++++++++++ .../UserAuthorizationCheckerTest.php | 70 ------------------- .../Voter/AuthenticatedVoterTest.php | 4 +- .../Authorization/Voter/ClosureVoterTest.php | 22 +++--- 10 files changed, 76 insertions(+), 188 deletions(-) delete mode 100644 Authentication/Token/UserAuthorizationCheckerToken.php delete mode 100644 Authorization/UserAuthorizationChecker.php delete mode 100644 Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php delete mode 100644 Tests/Authorization/UserAuthorizationCheckerTest.php diff --git a/Authentication/Token/UserAuthorizationCheckerToken.php b/Authentication/Token/UserAuthorizationCheckerToken.php deleted file mode 100644 index 2e84ce7..0000000 --- a/Authentication/Token/UserAuthorizationCheckerToken.php +++ /dev/null @@ -1,31 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\Security\Core\Authentication\Token; - -use Symfony\Component\Security\Core\User\UserInterface; - -/** - * UserAuthorizationCheckerToken implements a token used for checking authorization. - * - * @author Nate Wiebe - * - * @internal - */ -final class UserAuthorizationCheckerToken extends AbstractToken implements OfflineTokenInterface -{ - public function __construct(UserInterface $user) - { - parent::__construct($user->getRoles()); - - $this->setUser($user); - } -} diff --git a/Authorization/AuthorizationChecker.php b/Authorization/AuthorizationChecker.php index 3960f2b..3689bf5 100644 --- a/Authorization/AuthorizationChecker.php +++ b/Authorization/AuthorizationChecker.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 * @author Johannes M. Schmitt */ -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); + } + } } diff --git a/Authorization/UserAuthorizationChecker.php b/Authorization/UserAuthorizationChecker.php deleted file mode 100644 index f515e5c..0000000 --- a/Authorization/UserAuthorizationChecker.php +++ /dev/null @@ -1,31 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\Security\Core\Authorization; - -use Symfony\Component\Security\Core\Authentication\Token\UserAuthorizationCheckerToken; -use Symfony\Component\Security\Core\User\UserInterface; - -/** - * @author Nate Wiebe - */ -final class UserAuthorizationChecker implements UserAuthorizationCheckerInterface -{ - public function __construct( - private readonly AccessDecisionManagerInterface $accessDecisionManager, - ) { - } - - public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool - { - return $this->accessDecisionManager->decide(new UserAuthorizationCheckerToken($user), [$attribute], $subject, $accessDecision); - } -} diff --git a/Authorization/Voter/ClosureVoter.php b/Authorization/Voter/ClosureVoter.php index 23140c8..03a9f75 100644 --- a/Authorization/Voter/ClosureVoter.php +++ b/Authorization/Voter/ClosureVoter.php @@ -11,21 +11,14 @@ namespace Symfony\Component\Security\Core\Authorization\Voter; -use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; -use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; +use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; use Symfony\Component\Security\Http\Attribute\IsGranted; +use Symfony\Component\Security\Http\Attribute\IsGrantedContext; /** * This voter allows using a closure as the attribute being voted on. * - * The following named arguments are passed to the closure: - * - * - `token`: The token being used for voting - * - `subject`: The subject of the vote - * - `accessDecisionManager`: The access decision manager - * - `trustResolver`: The trust resolver - * * @see IsGranted doc for the complete closure signature. * * @author Alexandre Daubois @@ -33,8 +26,7 @@ final class ClosureVoter implements CacheableVoterInterface { public function __construct( - private AccessDecisionManagerInterface $accessDecisionManager, - private AuthenticationTrustResolverInterface $trustResolver, + private AuthorizationCheckerInterface $authorizationChecker, ) { } @@ -51,6 +43,7 @@ public function supportsType(string $subjectType): bool public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int { $vote ??= new Vote(); + $context = new IsGrantedContext($token, $token->getUser(), $this->authorizationChecker); $failingClosures = []; $result = VoterInterface::ACCESS_ABSTAIN; foreach ($attributes as $attribute) { @@ -60,7 +53,7 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes, ? $name = (new \ReflectionFunction($attribute))->name; $result = VoterInterface::ACCESS_DENIED; - if ($attribute(token: $token, subject: $subject, accessDecisionManager: $this->accessDecisionManager, trustResolver: $this->trustResolver)) { + if ($attribute($context, $subject)) { $vote->reasons[] = \sprintf('Closure %s returned true.', $name); return VoterInterface::ACCESS_GRANTED; diff --git a/CHANGELOG.md b/CHANGELOG.md index 12a0c0a..d3b3b65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,7 @@ CHANGELOG 7.3 --- - * Add `UserAuthorizationChecker::isGrantedForUser()` to test user authorization without relying on the session. - For example, users not currently logged in, or while processing a message from a message queue. + * Add `UserAuthorizationCheckerInterface` to test user authorization without relying on the session * Add `OfflineTokenInterface` to mark tokens that do not represent the currently logged-in user * Deprecate `UserInterface::eraseCredentials()` and `TokenInterface::eraseCredentials()`, erase credentials e.g. using `__serialize()` instead diff --git a/Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php b/Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php deleted file mode 100644 index 2e7e11b..0000000 --- a/Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php +++ /dev/null @@ -1,26 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\Security\Core\Tests\Authentication\Token; - -use PHPUnit\Framework\TestCase; -use Symfony\Component\Security\Core\Authentication\Token\UserAuthorizationCheckerToken; -use Symfony\Component\Security\Core\User\InMemoryUser; - -class UserAuthorizationCheckerTokenTest extends TestCase -{ - public function testConstructor() - { - $token = new UserAuthorizationCheckerToken($user = new InMemoryUser('foo', 'bar', ['ROLE_FOO'])); - $this->assertSame(['ROLE_FOO'], $token->getRoleNames()); - $this->assertSame($user, $token->getUser()); - } -} diff --git a/Tests/Authorization/AuthorizationCheckerTest.php b/Tests/Authorization/AuthorizationCheckerTest.php index 36b048c..00f0f50 100644 --- a/Tests/Authorization/AuthorizationCheckerTest.php +++ b/Tests/Authorization/AuthorizationCheckerTest.php @@ -14,6 +14,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Authentication\Token\NullToken; +use Symfony\Component\Security\Core\Authentication\Token\OfflineTokenInterface; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; @@ -77,4 +78,42 @@ public function testIsGrantedWithObjectAttribute() $this->tokenStorage->setToken($token); $this->assertTrue($this->authorizationChecker->isGranted($attribute)); } + + /** + * @dataProvider isGrantedForUserProvider + */ + public function testIsGrantedForUser(bool $decide, array $roles) + { + $user = new InMemoryUser('username', 'password', $roles); + + $this->accessDecisionManager + ->expects($this->once()) + ->method('decide') + ->with($this->callback(static fn (OfflineTokenInterface $token) => $token->getUser() === $user), ['ROLE_FOO']) + ->willReturn($decide); + + $this->assertSame($decide, $this->authorizationChecker->isGrantedForUser($user, 'ROLE_FOO')); + } + + public static function isGrantedForUserProvider(): array + { + return [ + [false, ['ROLE_USER']], + [true, ['ROLE_USER', 'ROLE_FOO']], + ]; + } + + public function testIsGrantedForUserWithObjectAttribute() + { + $attribute = new \stdClass(); + + $user = new InMemoryUser('username', 'password', ['ROLE_USER']); + + $this->accessDecisionManager + ->expects($this->once()) + ->method('decide') + ->with($this->isInstanceOf(OfflineTokenInterface::class), [$attribute]) + ->willReturn(true); + $this->assertTrue($this->authorizationChecker->isGrantedForUser($user, $attribute)); + } } diff --git a/Tests/Authorization/UserAuthorizationCheckerTest.php b/Tests/Authorization/UserAuthorizationCheckerTest.php deleted file mode 100644 index e9b6bb7..0000000 --- a/Tests/Authorization/UserAuthorizationCheckerTest.php +++ /dev/null @@ -1,70 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\Security\Core\Tests\Authorization; - -use PHPUnit\Framework\MockObject\MockObject; -use PHPUnit\Framework\TestCase; -use Symfony\Component\Security\Core\Authentication\Token\UserAuthorizationCheckerToken; -use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; -use Symfony\Component\Security\Core\Authorization\UserAuthorizationChecker; -use Symfony\Component\Security\Core\User\InMemoryUser; - -class UserAuthorizationCheckerTest extends TestCase -{ - private AccessDecisionManagerInterface&MockObject $accessDecisionManager; - private UserAuthorizationChecker $authorizationChecker; - - protected function setUp(): void - { - $this->accessDecisionManager = $this->createMock(AccessDecisionManagerInterface::class); - - $this->authorizationChecker = new UserAuthorizationChecker($this->accessDecisionManager); - } - - /** - * @dataProvider isGrantedProvider - */ - public function testIsGranted(bool $decide, array $roles) - { - $user = new InMemoryUser('username', 'password', $roles); - - $this->accessDecisionManager - ->expects($this->once()) - ->method('decide') - ->with($this->callback(fn (UserAuthorizationCheckerToken $token): bool => $user === $token->getUser()), $this->identicalTo(['ROLE_FOO'])) - ->willReturn($decide); - - $this->assertSame($decide, $this->authorizationChecker->isGrantedForUser($user, 'ROLE_FOO')); - } - - public static function isGrantedProvider(): array - { - return [ - [false, ['ROLE_USER']], - [true, ['ROLE_USER', 'ROLE_FOO']], - ]; - } - - public function testIsGrantedWithObjectAttribute() - { - $attribute = new \stdClass(); - - $token = new UserAuthorizationCheckerToken(new InMemoryUser('username', 'password', ['ROLE_USER'])); - - $this->accessDecisionManager - ->expects($this->once()) - ->method('decide') - ->with($this->isInstanceOf($token::class), $this->identicalTo([$attribute])) - ->willReturn(true); - $this->assertTrue($this->authorizationChecker->isGrantedForUser($token->getUser(), $attribute)); - } -} diff --git a/Tests/Authorization/Voter/AuthenticatedVoterTest.php b/Tests/Authorization/Voter/AuthenticatedVoterTest.php index 89f6c35..b5e0bf4 100644 --- a/Tests/Authorization/Voter/AuthenticatedVoterTest.php +++ b/Tests/Authorization/Voter/AuthenticatedVoterTest.php @@ -15,9 +15,9 @@ use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolver; 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\RememberMeToken; use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken; -use Symfony\Component\Security\Core\Authentication\Token\UserAuthorizationCheckerToken; use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; use Symfony\Component\Security\Core\Exception\InvalidArgumentException; @@ -148,7 +148,7 @@ public function getCredentials() } if ('offline' === $authenticated) { - return new UserAuthorizationCheckerToken($user); + return new class($user->getRoles()) extends AbstractToken implements OfflineTokenInterface {}; } return new NullToken(); diff --git a/Tests/Authorization/Voter/ClosureVoterTest.php b/Tests/Authorization/Voter/ClosureVoterTest.php index a919916..7a22f2d 100644 --- a/Tests/Authorization/Voter/ClosureVoterTest.php +++ b/Tests/Authorization/Voter/ClosureVoterTest.php @@ -12,13 +12,16 @@ namespace Symfony\Component\Security\Core\Tests\Authorization\Voter; use PHPUnit\Framework\TestCase; -use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; -use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; +use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; use Symfony\Component\Security\Core\Authorization\Voter\ClosureVoter; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; use Symfony\Component\Security\Core\User\UserInterface; +use Symfony\Component\Security\Http\Attribute\IsGrantedContext; +/** + * @requires function Symfony\Component\Security\Http\Attribute\IsGrantedContext::isGranted + */ class ClosureVoterTest extends TestCase { private ClosureVoter $voter; @@ -26,8 +29,7 @@ class ClosureVoterTest extends TestCase protected function setUp(): void { $this->voter = new ClosureVoter( - $this->createMock(AccessDecisionManagerInterface::class), - $this->createMock(AuthenticationTrustResolverInterface::class), + $this->createMock(AuthorizationCheckerInterface::class), ); } @@ -49,7 +51,7 @@ public function testClosureReturningFalseDeniesAccess() $this->assertSame(VoterInterface::ACCESS_DENIED, $this->voter->vote( $token, null, - [fn (...$vars) => false] + [fn () => false] )); } @@ -62,7 +64,7 @@ public function testClosureReturningTrueGrantsAccess() $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->voter->vote( $token, null, - [fn (...$vars) => true] + [fn () => true] )); } @@ -77,12 +79,8 @@ public function testArgumentsContent() $this->voter->vote( $token, $outerSubject, - [function (...$vars) use ($outerSubject) { - $this->assertInstanceOf(TokenInterface::class, $vars['token']); - $this->assertSame($outerSubject, $vars['subject']); - - $this->assertInstanceOf(AccessDecisionManagerInterface::class, $vars['accessDecisionManager']); - $this->assertInstanceOf(AuthenticationTrustResolverInterface::class, $vars['trustResolver']); + [function (IsGrantedContext $context, \stdClass $subject) use ($outerSubject) { + $this->assertSame($outerSubject, $subject); return true; }] From 41843c826b843ce843b09c37d6c837cc4d293a20 Mon Sep 17 00:00:00 2001 From: Florent Morselli Date: Thu, 13 Feb 2025 09:05:26 +0100 Subject: [PATCH 13/15] [Security] OAuth2 Introspection Endpoint (RFC7662) In addition to the excellent work of @vincentchalamon #48272, this PR allows getting the data from the OAuth2 Introspection Endpoint. This endpoint is defined in the [RFC7662](https://datatracker.ietf.org/doc/html/rfc7662). It returns the following information that is used to retrieve the user: * If the access token is active * A set of claims that are similar to the OIDC one, including the `sub` or the `username`. --- CHANGELOG.md | 1 + Tests/User/OAuth2UserTest.php | 51 +++++++++++++++++++++++++ User/OAuth2User.php | 70 +++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+) create mode 100644 Tests/User/OAuth2UserTest.php create mode 100644 User/OAuth2User.php diff --git a/CHANGELOG.md b/CHANGELOG.md index d3b3b65..1280641 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ CHANGELOG erase credentials e.g. using `__serialize()` instead * Add ability for voters to explain their vote * Add support for voting on closures + * Add `OAuth2User` with OAuth2 Access Token Introspection support for `OAuth2TokenHandler` 7.2 --- diff --git a/Tests/User/OAuth2UserTest.php b/Tests/User/OAuth2UserTest.php new file mode 100644 index 0000000..a53ed1b --- /dev/null +++ b/Tests/User/OAuth2UserTest.php @@ -0,0 +1,51 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Tests\User; + +use PHPUnit\Framework\TestCase; +use Symfony\Component\Security\Core\User\OAuth2User; + +class OAuth2UserTest extends TestCase +{ + public function testCannotCreateUserWithoutSubProperty() + { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('The claim "sub" or "username" must be provided.'); + + new OAuth2User(); + } + + public function testCreateFullUserWithAdditionalClaimsUsingPositionalParameters() + { + $this->assertEquals(new OAuth2User( + scope: 'read write dolphin', + username: 'jdoe', + exp: 1419356238, + iat: 1419350238, + sub: 'Z5O3upPC88QrAjx00dis', + aud: 'https://protected.example.net/resource', + iss: 'https://server.example.com/', + client_id: 'l238j323ds-23ij4', + extension_field: 'twenty-seven' + ), new OAuth2User(...[ + 'client_id' => 'l238j323ds-23ij4', + 'username' => 'jdoe', + 'scope' => 'read write dolphin', + 'sub' => 'Z5O3upPC88QrAjx00dis', + 'aud' => 'https://protected.example.net/resource', + 'iss' => 'https://server.example.com/', + 'exp' => 1419356238, + 'iat' => 1419350238, + 'extension_field' => 'twenty-seven', + ])); + } +} diff --git a/User/OAuth2User.php b/User/OAuth2User.php new file mode 100644 index 0000000..42c0550 --- /dev/null +++ b/User/OAuth2User.php @@ -0,0 +1,70 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\User; + +/** + * UserInterface implementation used by the access-token security workflow with an OIDC server. + */ +class OAuth2User implements UserInterface +{ + public readonly array $additionalClaims; + + public function __construct( + private array $roles = ['ROLE_USER'], + // Standard Claims (https://datatracker.ietf.org/doc/html/rfc7662#section-2.2) + public readonly ?string $scope = null, + public readonly ?string $clientId = null, + public readonly ?string $username = null, + public readonly ?string $tokenType = null, + public readonly ?int $exp = null, + public readonly ?int $iat = null, + public readonly ?int $nbf = null, + public readonly ?string $sub = null, + public readonly ?string $aud = null, + public readonly ?string $iss = null, + public readonly ?string $jti = null, + + // Additional Claims (" + // Specific implementations MAY extend this structure with + // their own service-specific response names as top-level members + // of this JSON object. + // ") + ...$additionalClaims, + ) { + if ((null === $sub || '' === $sub) && (null === $username || '' === $username)) { + throw new \InvalidArgumentException('The claim "sub" or "username" must be provided.'); + } + + $this->additionalClaims = $additionalClaims['additionalClaims'] ?? $additionalClaims; + } + + /** + * OIDC or OAuth specs don't have any "role" notion. + * + * If you want to implement "roles" from your OIDC server, + * send a "roles" constructor argument to this object + * (e.g.: using a custom UserProvider). + */ + public function getRoles(): array + { + return $this->roles; + } + + public function getUserIdentifier(): string + { + return (string) ($this->sub ?? $this->username); + } + + public function eraseCredentials(): void + { + } +} From 7929409caf9dc6071df627e8c19a57c222625fe5 Mon Sep 17 00:00:00 2001 From: Christophe Coevoet Date: Thu, 20 Mar 2025 12:22:14 +0100 Subject: [PATCH 14/15] Improve documentation of PasswordUpgraderInterface --- User/PasswordUpgraderInterface.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/User/PasswordUpgraderInterface.php b/User/PasswordUpgraderInterface.php index fd21f14..2f200cc 100644 --- a/User/PasswordUpgraderInterface.php +++ b/User/PasswordUpgraderInterface.php @@ -11,6 +11,8 @@ namespace Symfony\Component\Security\Core\User; +use Symfony\Component\Security\Core\Exception\UnsupportedUserException; + /** * @author Nicolas Grekas */ @@ -22,6 +24,8 @@ interface PasswordUpgraderInterface * This method should persist the new password in the user storage and update the $user object accordingly. * Because you don't want your users not being able to log in, this method should be opportunistic: * it's fine if it does nothing or if it fails without throwing any exception. + * + * @throws UnsupportedUserException if the implementation does not support that user */ public function upgradePassword(PasswordAuthenticatedUserInterface $user, string $newHashedPassword): void; } From 68ebebf88f688138c571f4853a2f68be49e8c159 Mon Sep 17 00:00:00 2001 From: Christian Flothmann Date: Fri, 4 Apr 2025 15:02:15 +0200 Subject: [PATCH 15/15] replace expectDeprecation() with expectUserDeprecationMessage() --- Tests/Authentication/Token/AbstractTokenTest.php | 6 +++--- Tests/User/InMemoryUserTest.php | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Tests/Authentication/Token/AbstractTokenTest.php b/Tests/Authentication/Token/AbstractTokenTest.php index ef3d380..3972b1c 100644 --- a/Tests/Authentication/Token/AbstractTokenTest.php +++ b/Tests/Authentication/Token/AbstractTokenTest.php @@ -12,7 +12,7 @@ namespace Symfony\Component\Security\Core\Tests\Authentication\Token; use PHPUnit\Framework\TestCase; -use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; +use Symfony\Bridge\PhpUnit\ExpectUserDeprecationMessageTrait; use Symfony\Component\Security\Core\Authentication\Token\AbstractToken; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\User\InMemoryUser; @@ -20,7 +20,7 @@ class AbstractTokenTest extends TestCase { - use ExpectDeprecationTrait; + use ExpectUserDeprecationMessageTrait; /** * @dataProvider provideUsers @@ -48,7 +48,7 @@ public function testEraseCredentials() $user->expects($this->once())->method('eraseCredentials'); $token->setUser($user); - $this->expectDeprecation(\sprintf('Since symfony/security-core 7.3: The "%s::eraseCredentials()" method is deprecated and will be removed in 8.0, erase credentials using the "__serialize()" method instead.', TokenInterface::class)); + $this->expectUserDeprecationMessage(\sprintf('Since symfony/security-core 7.3: The "%s::eraseCredentials()" method is deprecated and will be removed in 8.0, erase credentials using the "__serialize()" method instead.', TokenInterface::class)); $token->eraseCredentials(); } diff --git a/Tests/User/InMemoryUserTest.php b/Tests/User/InMemoryUserTest.php index 501bf74..f06e98c 100644 --- a/Tests/User/InMemoryUserTest.php +++ b/Tests/User/InMemoryUserTest.php @@ -12,13 +12,13 @@ namespace Symfony\Component\Security\Core\Tests\User; use PHPUnit\Framework\TestCase; -use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; +use Symfony\Bridge\PhpUnit\ExpectUserDeprecationMessageTrait; use Symfony\Component\Security\Core\User\InMemoryUser; use Symfony\Component\Security\Core\User\UserInterface; class InMemoryUserTest extends TestCase { - use ExpectDeprecationTrait; + use ExpectUserDeprecationMessageTrait; public function testConstructorException() { @@ -62,7 +62,7 @@ public function testIsEnabled() public function testEraseCredentials() { $user = new InMemoryUser('fabien', 'superpass'); - $this->expectDeprecation(\sprintf('%sMethod %s::eraseCredentials() is deprecated since symfony/security-core 7.3', \PHP_VERSION_ID >= 80400 ? 'Unsilenced deprecation: ' : '', InMemoryUser::class)); + $this->expectUserDeprecationMessage(\sprintf('%sMethod %s::eraseCredentials() is deprecated since symfony/security-core 7.3', \PHP_VERSION_ID >= 80400 ? 'Unsilenced deprecation: ' : '', InMemoryUser::class)); $user->eraseCredentials(); $this->assertEquals('superpass', $user->getPassword()); }