Skip to content

Commit 0c7c251

Browse files
chalasrchalasr
committed
feature #59805 [Security] Improve DX of recent additions (nicolas-grekas)
This PR was merged into the 7.3 branch. Discussion ---------- [Security] Improve DX of recent additions | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | not really | Deprecations? | no | Issues | - | License | MIT This is a follow up of #48142 and #59150 which were merged recently into 7.3. Summary of the changes: - `UserAuthorizationChecker`, the implementation of the corresponding interface is merged into the existing `AuthorizationChecker`. - `AuthorizationChecker::isGranted()` is made aware of token changed by its new `isGrantedForUser()` method, so that calls to `is_granted()` nested into `is_granted_for_user()` calls will check the provided user, not the logged in one. - Replace the many arguments passed to `IsGranted`'s closures by 1. a new `IsGrantedContext` and 2. the `$subject`. This makes everything simpler, easier to discover, and more extensible. Thanks to the previous item, IsGrantedContext only needs the auth-checker, not the access-decision-manager anymore. Simpler again. - Fix to the tests, those were broken in both PRs. Commits ------- aa38eb8 [Security] Improve DX of recent additions
2 parents aa53b94 + aa38eb8 commit 0c7c251

22 files changed

+350
-378
lines changed

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

+8-5
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ final class SecurityExtension extends AbstractExtension
3131
public function __construct(
3232
private ?AuthorizationCheckerInterface $securityChecker = null,
3333
private ?ImpersonateUrlGenerator $impersonateUrlGenerator = null,
34-
private ?UserAuthorizationCheckerInterface $userSecurityChecker = null,
3534
) {
3635
}
3736

@@ -58,8 +57,12 @@ public function isGranted(mixed $role, mixed $object = null, ?string $field = nu
5857

5958
public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?string $field = null, ?AccessDecision $accessDecision = null): bool
6059
{
61-
if (!$this->userSecurityChecker) {
62-
throw new \LogicException(\sprintf('An instance of "%s" must be provided to use "%s()".', UserAuthorizationCheckerInterface::class, __METHOD__));
60+
if (null === $this->securityChecker) {
61+
return false;
62+
}
63+
64+
if (!$this->securityChecker instanceof UserAuthorizationCheckerInterface) {
65+
throw new \LogicException(\sprintf('You cannot use "%s()" if the authorization checker doesn\'t implement "%s".%s', __METHOD__, UserAuthorizationCheckerInterface::class, interface_exists(UserAuthorizationCheckerInterface::class) ? ' Try upgrading the "symfony/security-core" package to v7.3 minimum.' : ''));
6366
}
6467

6568
if (null !== $field) {
@@ -71,7 +74,7 @@ public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $s
7174
}
7275

7376
try {
74-
return $this->userSecurityChecker->isGrantedForUser($user, $attribute, $subject, $accessDecision);
77+
return $this->securityChecker->isGrantedForUser($user, $attribute, $subject, $accessDecision);
7578
} catch (AuthenticationCredentialsNotFoundException) {
7679
return false;
7780
}
@@ -123,7 +126,7 @@ public function getFunctions(): array
123126
new TwigFunction('impersonation_path', $this->getImpersonatePath(...)),
124127
];
125128

126-
if ($this->userSecurityChecker) {
129+
if ($this->securityChecker instanceof UserAuthorizationCheckerInterface) {
127130
$functions[] = new TwigFunction('is_granted_for_user', $this->isGrantedForUser(...));
128131
}
129132

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

+61-26
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,23 @@
1515
use Symfony\Bridge\PhpUnit\ClassExistsMock;
1616
use Symfony\Bridge\Twig\Extension\SecurityExtension;
1717
use Symfony\Component\Security\Acl\Voter\FieldVote;
18+
use Symfony\Component\Security\Core\Authorization\AccessDecision;
1819
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
1920
use Symfony\Component\Security\Core\Authorization\UserAuthorizationCheckerInterface;
2021
use Symfony\Component\Security\Core\User\UserInterface;
2122

2223
class SecurityExtensionTest extends TestCase
2324
{
25+
public static function setUpBeforeClass(): void
26+
{
27+
ClassExistsMock::register(SecurityExtension::class);
28+
}
29+
30+
protected function tearDown(): void
31+
{
32+
ClassExistsMock::withMockedClasses([FieldVote::class => true]);
33+
}
34+
2435
/**
2536
* @dataProvider provideObjectFieldAclCases
2637
*/
@@ -39,17 +50,16 @@ public function testIsGrantedCreatesFieldVoteObjectWhenFieldNotNull($object, $fi
3950

4051
public function testIsGrantedThrowsWhenFieldNotNullAndFieldVoteClassDoesNotExist()
4152
{
42-
if (!class_exists(UserAuthorizationCheckerInterface::class)) {
53+
if (!interface_exists(UserAuthorizationCheckerInterface::class)) {
4354
$this->markTestSkipped('This test requires symfony/security-core 7.3 or superior.');
4455
}
4556

4657
$securityChecker = $this->createMock(AuthorizationCheckerInterface::class);
4758

48-
ClassExistsMock::register(SecurityExtension::class);
4959
ClassExistsMock::withMockedClasses([FieldVote::class => false]);
5060

5161
$this->expectException(\LogicException::class);
52-
$this->expectExceptionMessageMatches('Passing a $field to the "is_granted()" function requires symfony/acl.');
62+
$this->expectExceptionMessage('Passing a $field to the "is_granted()" function requires symfony/acl.');
5363

5464
$securityExtension = new SecurityExtension($securityChecker);
5565
$securityExtension->isGranted('ROLE', 'object', 'bar');
@@ -60,49 +70,74 @@ public function testIsGrantedThrowsWhenFieldNotNullAndFieldVoteClassDoesNotExist
6070
*/
6171
public function testIsGrantedForUserCreatesFieldVoteObjectWhenFieldNotNull($object, $field, $expectedSubject)
6272
{
63-
if (!class_exists(UserAuthorizationCheckerInterface::class)) {
73+
if (!interface_exists(UserAuthorizationCheckerInterface::class)) {
6474
$this->markTestSkipped('This test requires symfony/security-core 7.3 or superior.');
6575
}
6676

6777
$user = $this->createMock(UserInterface::class);
68-
$userSecurityChecker = $this->createMock(UserAuthorizationCheckerInterface::class);
69-
$userSecurityChecker
70-
->expects($this->once())
71-
->method('isGrantedForUser')
72-
->with($user, 'ROLE', $expectedSubject)
73-
->willReturn(true);
78+
$securityChecker = $this->createMockAuthorizationChecker();
7479

75-
$securityExtension = new SecurityExtension(null, null, $userSecurityChecker);
80+
$securityExtension = new SecurityExtension($securityChecker);
7681
$this->assertTrue($securityExtension->isGrantedForUser($user, 'ROLE', $object, $field));
82+
$this->assertSame($user, $securityChecker->user);
83+
$this->assertSame('ROLE', $securityChecker->attribute);
84+
85+
if (null === $field) {
86+
$this->assertSame($object, $securityChecker->subject);
87+
} else {
88+
$this->assertEquals($expectedSubject, $securityChecker->subject);
89+
}
90+
}
91+
92+
public static function provideObjectFieldAclCases()
93+
{
94+
return [
95+
[null, null, null],
96+
['object', null, 'object'],
97+
['object', false, new FieldVote('object', false)],
98+
['object', 0, new FieldVote('object', 0)],
99+
['object', '', new FieldVote('object', '')],
100+
['object', 'field', new FieldVote('object', 'field')],
101+
];
77102
}
78103

79104
public function testIsGrantedForUserThrowsWhenFieldNotNullAndFieldVoteClassDoesNotExist()
80105
{
81-
if (!class_exists(UserAuthorizationCheckerInterface::class)) {
106+
if (!interface_exists(UserAuthorizationCheckerInterface::class)) {
82107
$this->markTestSkipped('This test requires symfony/security-core 7.3 or superior.');
83108
}
84109

85-
$securityChecker = $this->createMock(UserAuthorizationCheckerInterface::class);
110+
$securityChecker = $this->createMockAuthorizationChecker();
86111

87-
ClassExistsMock::register(SecurityExtension::class);
88112
ClassExistsMock::withMockedClasses([FieldVote::class => false]);
89113

90114
$this->expectException(\LogicException::class);
91-
$this->expectExceptionMessageMatches('Passing a $field to the "is_granted_for_user()" function requires symfony/acl.');
115+
$this->expectExceptionMessage('Passing a $field to the "is_granted_for_user()" function requires symfony/acl.');
92116

93-
$securityExtension = new SecurityExtension(null, null, $securityChecker);
94-
$securityExtension->isGrantedForUser($this->createMock(UserInterface::class), 'object', 'bar');
117+
$securityExtension = new SecurityExtension($securityChecker);
118+
$securityExtension->isGrantedForUser($this->createMock(UserInterface::class), 'ROLE', 'object', 'bar');
95119
}
96120

97-
public static function provideObjectFieldAclCases()
121+
private function createMockAuthorizationChecker(): AuthorizationCheckerInterface&UserAuthorizationCheckerInterface
98122
{
99-
return [
100-
[null, null, null],
101-
['object', null, 'object'],
102-
['object', false, new FieldVote('object', false)],
103-
['object', 0, new FieldVote('object', 0)],
104-
['object', '', new FieldVote('object', '')],
105-
['object', 'field', new FieldVote('object', 'field')],
106-
];
123+
return new class implements AuthorizationCheckerInterface, UserAuthorizationCheckerInterface {
124+
public UserInterface $user;
125+
public mixed $attribute;
126+
public mixed $subject;
127+
128+
public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
129+
{
130+
throw new \BadMethodCallException('This method should not be called.');
131+
}
132+
133+
public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
134+
{
135+
$this->user = $user;
136+
$this->attribute = $attribute;
137+
$this->subject = $subject;
138+
139+
return true;
140+
}
141+
};
107142
}
108143
}

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

+3-10
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
use Symfony\Component\Security\Core\Authorization\AuthorizationChecker;
3232
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
3333
use Symfony\Component\Security\Core\Authorization\ExpressionLanguage;
34-
use Symfony\Component\Security\Core\Authorization\UserAuthorizationChecker;
3534
use Symfony\Component\Security\Core\Authorization\UserAuthorizationCheckerInterface;
3635
use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter;
3736
use Symfony\Component\Security\Core\Authorization\Voter\ClosureVoter;
@@ -69,12 +68,7 @@
6968
service('security.access.decision_manager'),
7069
])
7170
->alias(AuthorizationCheckerInterface::class, 'security.authorization_checker')
72-
73-
->set('security.user_authorization_checker', UserAuthorizationChecker::class)
74-
->args([
75-
service('security.access.decision_manager'),
76-
])
77-
->alias(UserAuthorizationCheckerInterface::class, 'security.user_authorization_checker')
71+
->alias(UserAuthorizationCheckerInterface::class, 'security.authorization_checker')
7872

7973
->set('security.token_storage', UsageTrackingTokenStorage::class)
8074
->args([
@@ -94,7 +88,7 @@
9488
service_locator([
9589
'security.token_storage' => service('security.token_storage'),
9690
'security.authorization_checker' => service('security.authorization_checker'),
97-
'security.user_authorization_checker' => service('security.user_authorization_checker'),
91+
'security.user_authorization_checker' => service('security.authorization_checker'),
9892
'security.authenticator.managers_locator' => service('security.authenticator.managers_locator')->ignoreOnInvalid(),
9993
'request_stack' => service('request_stack'),
10094
'security.firewall.map' => service('security.firewall.map'),
@@ -174,8 +168,7 @@
174168

175169
->set('security.access.closure_voter', ClosureVoter::class)
176170
->args([
177-
service('security.access.decision_manager'),
178-
service('security.authentication.trust_resolver'),
171+
service('security.authorization_checker'),
179172
])
180173
->tag('security.voter', ['priority' => 245])
181174

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

-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
->args([
2727
service('security.authorization_checker')->ignoreOnInvalid(),
2828
service('security.impersonate_url_generator')->ignoreOnInvalid(),
29-
service('security.user_authorization_checker')->ignoreOnInvalid(),
3029
])
3130
->tag('twig.extension')
3231
;

‎src/Symfony/Bundle/SecurityBundle/Security.php

+11-11
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,17 @@ public function isGranted(mixed $attributes, mixed $subject = null, ?AccessDecis
6565
->isGranted($attributes, $subject, $accessDecision);
6666
}
6767

68+
/**
69+
* Checks if the attribute is granted against the user and optionally supplied subject.
70+
*
71+
* This should be used over isGranted() when checking permissions against a user that is not currently logged in or while in a CLI context.
72+
*/
73+
public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
74+
{
75+
return $this->container->get('security.user_authorization_checker')
76+
->isGrantedForUser($user, $attribute, $subject, $accessDecision);
77+
}
78+
6879
public function getToken(): ?TokenInterface
6980
{
7081
return $this->container->get('security.token_storage')->getToken();
@@ -150,17 +161,6 @@ public function logout(bool $validateCsrfToken = true): ?Response
150161
return $logoutEvent->getResponse();
151162
}
152163

153-
/**
154-
* Checks if the attribute is granted against the user and optionally supplied subject.
155-
*
156-
* This should be used over isGranted() when checking permissions against a user that is not currently logged in or while in a CLI context.
157-
*/
158-
public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
159-
{
160-
return $this->container->get('security.user_authorization_checker')
161-
->isGrantedForUser($user, $attribute, $subject, $accessDecision);
162-
}
163-
164164
private function getAuthenticator(?string $authenticatorName, string $firewallName): AuthenticatorInterface
165165
{
166166
if (!isset($this->authenticators[$firewallName])) {

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

-31
This file was deleted.

‎src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php

+19-2
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,11 @@
1111

1212
namespace Symfony\Component\Security\Core\Authorization;
1313

14+
use Symfony\Component\Security\Core\Authentication\Token\AbstractToken;
1415
use Symfony\Component\Security\Core\Authentication\Token\NullToken;
16+
use Symfony\Component\Security\Core\Authentication\Token\OfflineTokenInterface;
1517
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
18+
use Symfony\Component\Security\Core\User\UserInterface;
1619

1720
/**
1821
* AuthorizationChecker is the main authorization point of the Security component.
@@ -22,8 +25,9 @@
2225
* @author Fabien Potencier <fabien@symfony.com>
2326
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
2427
*/
25-
class AuthorizationChecker implements AuthorizationCheckerInterface
28+
class AuthorizationChecker implements AuthorizationCheckerInterface, UserAuthorizationCheckerInterface
2629
{
30+
private array $tokenStack = [];
2731
private array $accessDecisionStack = [];
2832

2933
public function __construct(
@@ -34,7 +38,7 @@ public function __construct(
3438

3539
final public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
3640
{
37-
$token = $this->tokenStorage->getToken();
41+
$token = end($this->tokenStack) ?: $this->tokenStorage->getToken();
3842

3943
if (!$token || !$token->getUser()) {
4044
$token = new NullToken();
@@ -48,4 +52,17 @@ final public function isGranted(mixed $attribute, mixed $subject = null, ?Access
4852
array_pop($this->accessDecisionStack);
4953
}
5054
}
55+
56+
final public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
57+
{
58+
$token = new class($user->getRoles()) extends AbstractToken implements OfflineTokenInterface {};
59+
$token->setUser($user);
60+
$this->tokenStack[] = $token;
61+
62+
try {
63+
return $this->isGranted($attribute, $subject, $accessDecision);
64+
} finally {
65+
array_pop($this->tokenStack);
66+
}
67+
}
5168
}

‎src/Symfony/Component/Security/Core/Authorization/UserAuthorizationChecker.php

-31
This file was deleted.

0 commit comments

Comments
 (0)