8000 feature #59771 [Security] Add ability for voters to explain their vot… · symfony/symfony@2962567 · GitHub
[go: up one dir, main page]
Skip to content

Commit 2962567

Browse files
fabpotfabpot
committed
feature #59771 [Security] Add ability for voters to explain their vote (nicolas-grekas)
This PR was merged into the 7.3 branch. Discussion ---------- [Security] Add ability for voters to explain their vote | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | Fix #27995, #26343, #35592, #43147 | License | MIT This PR takes over #58107, which itself took over from #46493, etc. It takes the only approach I could think of that would preserve BC. The visible tip of what this achieves is: ![image](https://github.com/user-attachments/assets/67bd0678-868f-40ed-b2c6-3b1f10ffe101) ![image](https://github.com/user-attachments/assets/715814d8-e4de-47f0-aa0e-c412092389ff) Internally, this provides a new access audit log infrastructure that relies on new `AccessDecision` and `Vote` objects. Note that I didn't add (nor fix) tests for now. I'll borrow (and credit) from #58107 / #46493 as much as possible. Commits ------- c6eb9ae [Security] Add ability for voters to explain their vote
2 parents 6fe4cb5 + c6eb9ae commit 2962567
  • Http

    • 37 files changed

    +491
    -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: 8 additions & 3 deletions
    Original file line numberDiff line numberDiff line change
    @@ -571,14 +571,19 @@
    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+
    {% for voter_reason in voter_detail['reasons'] %}
    584+
    <br>{{ voter_reason }}
    585+
    {% endfor %}
    586+
    {% endif %}
    582587
    </td>
    583588
    </tr>
    584589
    {% 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' => C462 ['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