8000 [Security] Add ability for voters to explain their vote · symfony/symfony@be530b6 · GitHub
[go: up one dir, main page]
Skip to content

Commit be530b6

Browse files
nicolas-grekasnicolas-grekas
committed
[Security] Add ability for voters to explain their vote
1 parent 6a6ebac commit be530b6

37 files changed

+489
-189
lines changed

‎UPGRADE-7.3.md

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,19 @@ Ldap
1616
Security
1717
--------
1818

19-
* Deprecate `UserInterface::eraseCredentials()` and `TokenInterface::eraseCredentials()`,
19+
* Deprecate `UserInterface::eraseCredentials()` and `TokenInterface::eraseCredentials()`;
2020
erase credentials e.g. using `__serialize()` instead
2121

22-
*Before*
22+
Before:
23+
2324
```php
2425
public function eraseCredentials(): void
2526
{
2627
}
2728
```
2829

29-
*After*
30+
After:
31+
3032
```php
3133
#[\Deprecated]
3234
public function eraseCredentials(): void
@@ -43,19 +45,36 @@ Security
4345
}
4446
```
4547

48+
* Add argument `$vote` to `VoterInterface::vote()` and to `Voter::voteOnAttribute()`;
49+
it should be used to report the reason of a vote. E.g:
50+
51+
```php
52+
protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token, ?Vote $vote = null): bool
53+
{
54+
$vote ??= new Vote();
55+
56+
$vote->reasons[] = 'A brief explanation of why access is granted or denied, as appropriate.';
57+
}
58+
```
59+
60+
* Add argument `$accessDecision` to `AccessDecisionManagerInterface::decide()` and `AuthorizationCheckerInterface::isGranted()`;
61+
it should be used to report the reason of a decision, including all the related votes.
62+
4663
Console
4764
-------
4865

49-
* Omitting parameter types in callables configured via `Command::setCode` method is deprecated
66+
* Omitting parameter types in callables configured via `Command::setCode()` method is deprecated
67+
68+
Before:
5069

51-
*Before*
5270
```php
5371
$command->setCode(function ($input, $output) {
5472
// ...
5573
});
5674
```
5775

58-
*After*
76+
After:
77+
5978
```php
6079
use Symfony\Component\Console\Input\InputInterface;
6180
use Symfony\Component\Console\Output\OutputInterface;
@@ -119,6 +138,7 @@ Validator
119138
}
120139
}
121140
```
141+
122142
* Deprecate passing an array of options to the constructors of the constraint classes, pass each option as a dedicated argument instead
123143

124144
Before:

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

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Bridge\Twig\Extension;
1313

1414
use Symfony\Component\Security\Acl\Voter\FieldVote;
15+
use Symfony\Component\Security\Core\Authorization\AccessDecision;
1516
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
1617
use Symfony\Component\Security\Core\Authorization\UserAuthorizationCheckerInterface;
1718
use Symfony\Component\Security\Core\Exception\AuthenticationCredentialsNotFoundException;
@@ -34,7 +35,7 @@ public function __construct(
3435
) {
3536
}
3637

37-
public function isGranted(mixed $role, mixed $object = null, ?string $field = null): bool
38+
public function isGranted(mixed $role, mixed $object = null, ?string $field = null, ?AccessDecision $accessDecision = null): bool
3839
{
3940
if (null === $this->securityChecker) {
4041
return false;
@@ -49,13 +50,13 @@ public function isGranted(mixed $role, mixed $object = null, ?string $field = nu
4950
}
5051

5152
try {
52-
return $this->securityChecker->isGranted($role, $object);
53+
return $this->securityChecker->isGranted($role, $object, $accessDecision);
5354
} catch (AuthenticationCredentialsNotFoundException) {
5455
return false;
5556
}
5657
}
5758

58-
public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?string $field = null): bool
59+
public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?string $field = null, ?AccessDecision $accessDecision = null): bool
5960
{
6061
if (!$this->userSecurityChecker) {
6162
throw new \LogicException(\sprintf('An instance of "%s" must be provided to use "%s()".', UserAuthorizationCheckerInterface::class, __METHOD__));
@@ -69,7 +70,11 @@ public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $s
6970
$subject = new FieldVote($subject, $field);
7071
}
7172

72-
return $this->userSecurityChecker->isGrantedForUser($user, $attribute, $subject);
73+
try {
74+
return $this->userSecurityChecker->isGrantedForUser($user, $attribute, $subject, $accessDecision);
75+
} catch (AuthenticationCredentialsNotFoundException) {
76+
return false;
77+
}
7378
}
7479

7580
public function getImpersonateExitUrl(?string $exitTo = null): string

‎src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
3636
use Symfony\Component\Routing\RouterInterface;
3737
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
38+
use Symfony\Component\Security\Core\Authorization\AccessDecision;
3839
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
3940
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
4041
use Symfony\Component\Security\Core\User\UserInterface;
@@ -202,6 +203,21 @@ protected function isGranted(mixed $attribute, mixed $subject = null): bool
202203
return $this->container->get('security.authorization_checker')->isGranted($attribute, $subject);
203204
}
204205

206+
/**
207+
* Checks if the attribute is granted against the current authentication token and optionally supplied subject.
208+
*/
209+
protected function getAccessDecision(mixed $attribute, mixed $subject = null): AccessDecision
210+
{
211+
if (!$this->container->has('security.authorization_checker')) {
212+
throw new \LogicException('The SecurityBundle is not registered in your application. Try running "composer require symfony/security-bundle".');
213+
}
214+
215+
$accessDecision = new AccessDecision();
216+
$accessDecision->isGranted = $this->container->get('security.authorization_checker')->isGranted($attribute, $subject, $accessDecision);
217+
218+
return $accessDecision;
219+
}
220+
205221
/**
206222
* Throws an exception unless the attribute is granted against the current authentication token and optionally
207223
* supplied subject.
@@ -210,12 +226,24 @@ protected function isGranted(mixed $attribute, mixed $subject = null): bool
210226
*/
211227
protected function denyAccessUnlessGranted(mixed $attribute, mixed $subject = null, string $message = 'Access Denied.'): void
212228
{
213-
if (!$this->isGranted($attribute, $subject)) {
214-
$exception = $this->createAccessDeniedException($message);
215-
$exception->setAttributes([$attribute]);
216-
$exception->setSubject($subject);
229+
if (class_exists(AccessDecision::class)) {
230+
$accessDecision = $this->getAccessDecision($attribute, $subject);
231+
$isGranted = $accessDecision->isGranted;
232+
} else {
233+
$accessDecision = null;
234+
$isGranted = $this->isGranted($attribute, $subject);
235+
}
236+
237+
if (!$isGranted) {
238+
$e = $this->createAccessDeniedException(3 > \func_num_args() && $accessDecision ? $accessDecision->getMessage() : $message);
239+
$e->setAttributes([$attribute]);
240+
$e->setSubject($subject);
241+
242+
if ($accessDecision) {
243+
$e->setAccessDecision($accessDecision);
244+
}
217245

218-
throw $exception;
246+
throw $e;
219247
}
220248
}
221249

‎src/Symfony/Bundle/FrameworkBundle/Tests/Controller/AbstractControllerTest.php

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,10 @@
4040
use Symfony\Component\Routing\RouterInterface;
4141
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
4242
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
43+
use Symfony\Component\Security\Core\Authorization\AccessDecision;
4344
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
45+
use Symfony\Component\Security\Core\Authorization\Voter\Vote;
46+
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
4447
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
4548
use Symfony\Component\Security\Core\User\InMemoryUser;
4649
use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
@@ -352,7 +355,19 @@ public function testIsGranted()
352355
public function testdenyAccessUnlessGranted()
353356
{
354357
$authorizationChecker = $this->createMock(AuthorizationCheckerInterface::class);
355-
$authorizationChecker->expects($this->once())->method('isGranted')->willReturn(false);
358+
$authorizationChecker
359+
->expects($this->once())
360+
->method('isGranted')
361+
->willReturnCallback(function ($attribute, $subject, ?AccessDecision $accessDecision = null) {
362+
if (class_exists(AccessDecision::class)) {
363+
$this->assertInstanceOf(AccessDecision::class, $accessDecision);
364+
$accessDecision->votes[] = $vote = new Vote();
365+
$vote->result = VoterInterface::ACCESS_DENIED;
366+
$vote->reasons[] = 'Why should I.';
367+
}
368+
369+
return false;
370+
});
356371

357372
$container = new Container();
358373
$container->set('security.authorization_checker', $authorizationChecker);
@@ -361,8 +376,17 @@ public function testdenyAccessUnlessGranted()
361376
$controller->setContainer($container);
362377

363378
$this->expectException(AccessDeniedException::class);
379+
$this->expectExceptionMessage('Access Denied.'.(class_exists(AccessDecision::class) ? ' Why should I.' : ''));
364380

365-
$controller->denyAccessUnlessGranted('foo');
381+
try {
382+
$controller->denyAccessUnlessGranted('foo');
383+
} catch (AccessDeniedException $e) {
384+
if (class_exists(AccessDecision::class)) {
385+
$this->assertFalse($e->getAccessDecision()->isGranted);
386+
}
387+
388+
throw $e;
389+
}
366390
}
367391

368392
/**

‎src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ public function collect(Request $request, Response $response, ?\Throwable $excep
138138

139139
// collect voter details
140140
$decisionLog = $this->accessDecisionManager->getDecisionLog();
141+
141142
foreach ($decisionLog as $key => $log) {
142143
$decisionLog[$key]['voter_details'] = [];
143144
foreach ($log['voterDetails'] as $voterDetail) {
@@ -147,6 +148,7 @@ public function collect(Request $request, Response $response, ?\Throwable $excep
147148
'class' => $classData,
148149
'attributes' => $voterDetail['attributes'], // Only displayed for unanimous strategy
149150
'vote' => $voterDetail['vote'],
151+
'reasons' => $voterDetail['reasons'] ?? [],
150152
];
151153
}
152154
unset($decisionLog[$key]['voterDetails']);

‎src/Symfony/Bundle/SecurityBundle/EventListener/VoteListener.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public function __construct(
3131

3232
public function onVoterVote(VoteEvent $event): void
3333
{
34-
$this->traceableAccessDecisionManager->addVoterVote($event->getVoter(), $event->getAttributes(), $event->getVote());
34+
$this->traceableAccessDecisionManager->addVoterVote($event->getVoter(), $event->getAttributes(), $event->getVote(), $event->getReasons());
3535
}
3636

3737
public static function getSubscribedEvents(): array

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
->args([
2323
service('debug.security.access.decision_manager.inner'),
2424
])
25+
->tag('kernel.reset', ['method' => 'reset', 'on_invalid' => 'ignore'])
2526

2627
->set('debug.security.voter.vote_listener', VoteListener::class)
2728
->args([

‎src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -571,14 +571,17 @@
571571
{% endif %}
572572
<td class="font-normal text-small">
573573
{% if voter_detail['vote'] == constant('Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::ACCESS_GRANTED') %}
574-
ACCESS GRANTED
574+
GRANTED
575575
{% elseif voter_detail['vote'] == constant('Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::ACCESS_ABSTAIN') %}
576-
ACCESS ABSTAIN
576+
ABSTAIN
577577
{% elseif voter_detail['vote'] == constant('Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::ACCESS_DENIED') %}
578-
ACCESS DENIED
578+
DENIED
579579
{% else %}
580580
unknown ({{ voter_detail['vote'] }})
581581
{% endif %}
582+
{% if voter_detail['reasons'] is not empty %}
583+
<br>{{ voter_detail['reasons'] | join('<br>') }}
584+
{% endif %}
582585
10000 </td>
583586
</tr>
584587
{% endfor %}

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Symfony\Component\HttpFoundation\Response;
1818
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
1919
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
20+
use Symfony\Component\Security\Core\Authorization\AccessDecision;
2021
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
2122
use Symfony\Component\Security\Core\Authorization\UserAuthorizationCheckerInterface;
2223
use Symfony\Component\Security\Core\Exception\LogicException;
@@ -58,10 +59,10 @@ public function getUser(): ?UserInterface
5859
/**
5960
* Checks if the attributes are granted against the current authentication token and optionally supplied subject.
6061
*/
61-
public function isGranted(mixed $attributes, mixed $subject = null): bool
62+
public function isGranted(mixed $attributes, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
6263
{
6364
return $this->container->get('security.authorization_checker')
64-
->isGranted($attributes, $subject);
65+
->isGranted($attributes, $subject, $accessDecision);
6566
}
6667

6768
public function getToken(): ?TokenInterface
@@ -154,10 +155,10 @@ public function logout(bool $validateCsrfToken = true): ?Response
154155
*
155156
* This should be used over isGranted() when checking permissions against a user that is not currently logged in or while in a CLI context.
156157
*/
157-
public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null): bool
158+
public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
158159
{
159160
return $this->container->get('security.user_authorization_checker')
160-
->isGrantedForUser($user, $attribute, $subject);
161+
->isGrantedForUser($user, $attribute, $subject, $accessDecision);
161162
}
162163

163164
private function getAuthenticator(?string $authenticatorName, string $firewallName): AuthenticatorInterface

‎src/Symfony/Bundle/SecurityBundle/Tests/DataCollector/SecurityDataCollectorTest.php

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
2929
use Symfony\Component\Security\Core\Authorization\TraceableAccessDecisionManager;
3030
use Symfony\Component\Security\Core\Authorization\Voter\TraceableVoter;
31+
use Symfony\Component\Security\Core\Authorization\Voter\Vote;
3132
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
3233
use Symfony\Component\Security\Core\Role\RoleHierarchy;
3334
use Symfony\Component\Security\Core\User\InMemoryUser;
@@ -271,8 +272,8 @@ public function dispatch(object $event, ?string $eventName = null): object
271272
'object' => new \stdClass(),
272273
'result' => true,
273274
'voter_details' => [
274-
['class' => $voter1::class, 'attributes' => ['view'], 'vote' => VoterInterface::ACCESS_ABSTAIN],
275-
['class' => $voter2::class, 'attributes' => ['view'], 'vote' => VoterInterface::ACCESS_ABSTAIN],
275+
['class' => $voter1::class, 'attributes' => ['view'], 'vote' => VoterInterface::ACCESS_ABSTAIN, 'reasons' => []],
276+
['class' => $voter2::class, 'attributes' => ['view'], 'vote' => VoterInterface::ACCESS_ABSTAIN, 'reasons' => []],
276277
],
277278
]];
278279

@@ -360,19 +361,19 @@ public function dispatch(object $event, ?string $eventName = null): object
360361
'object' => new \stdClass(),
361362
'result' => false,
362363
'voter_details' => [
363-
['class' => $voter1::class, 'attributes' => ['view'], 'vote' => VoterInterface::ACCESS_DENIED],
364-
['class' => $voter1::class, 'attributes' => ['edit'], 'vote' => VoterInterface::ACCESS_DENIED],
365-
['class' => $voter2::class, 'attributes' => ['view'], 'vote' => VoterInterface::ACCESS_GRANTED],
366-
['class' => $voter2::class, 'attributes' => ['edit'], 'vote' => VoterInterface::ACCESS_GRANTED],
364+
['class' => $voter1::class, 'attributes' => ['view'], 'vote' => VoterInterface::ACCESS_DENIED, 'reasons' => []],
365+
['class' => $voter1::class, 'attributes' => ['edit'], 'vote' => VoterInterface::ACCESS_DENIED, 'reasons' => []],
366+
['class' => $voter2::class, 'attributes' => ['view'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []],
367+
['class' => $voter2::class, 'attributes' => ['edit'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []],
367368
],
368369
],
369370
[
370371
'attributes' => ['update'],
371372
'object' => new \stdClass(),
372373
'result' => true,
373374
'voter_details' => [
374-
['class' => $voter1::class, 'attributes' => ['update'], 'vote' => VoterInterface::ACCESS_GRANTED],
375-
['class' => $voter2::class, 'attributes' => ['update'], 'vote' => VoterInterface::ACCESS_GRANTED],
375+
['class' => $voter1::class, 'attributes' => ['update'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []],
376+
['class' => $voter2::class, 'attributes' => ['update'], 'vote' => VoterInterface::ACCESS_GRANTED, 'reasons' => []],
376377
],
377378
],
378379
];
@@ -461,7 +462,7 @@ private function getRoleHierarchy()
461462

462463
final class DummyVoter implements VoterInterface
463464
{
464-
public function vote(TokenInterface $token, mixed $subject, array $attributes): int
465+
public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int
465466
{
466467
}
467468
}

0 commit comments

Comments
 (0)
0