[Security] Add ability for voters to explain their vote #59771
Conversation
d664683 to
fcc69a6
Compare
There was a problem hiding this comment.
Just a thought at first read: Vote and AccessDecision really look like value objects.
Should we make:
AccessDecision::isGrantedreadonly and set with a constructor? The decision may not change when the object is created, and if so, a new instance may be created instead because that's another decision actually ;- The same thing for
Vote::$resultandVote::$voter? Here again, if one of these property has to change after instantiation, then it may be more correct to create a new instance because this is not the "same" vote anymore.
src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php
Outdated
Show resolved
Hide resolved
|
@alexandre-daubois making the properties readonly would break the FC layer: setting the results multiple times is how an updated implementation can still give proper feedback when given a non-updated one (voters/access decision managers/strategies.) |
src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php
Outdated
Show resolved
Hide resolved
fae982d to
20e7b35
Compare
| try { | ||
| return $this->userSecurityChecker->isGrantedForUser($user, $attribute, $subject, $accessDecision); | ||
| } catch (AuthenticationCredentialsNotFoundException) { | ||
| return false; |
There was a problem hiding this comment.
added for parity with isGranted() above
69264d6 to
2f4b88e
Compare
|
This PR should be ready for review. Status: needs review |
src/Symfony/Component/Security/Core/Authorization/AccessDecision.php
Outdated
Show resolved
Hide resolved
| 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 |
There was a problem hiding this comment.
Note that we could decide to not expose AccessDecision objects when using (User)AuthorizationCheckerInterface.
Any opinions on this?
Having the AccessDecision available to isGranted[ForUser]() methods would allow userland to get the audit log more easily. It's also used right now in AbstractController::denyUnlessGranted(), to compute a better error message. Using the AccessDecisionManager instead of the (User)AuthorizationChecker would be the alternative.
There was a problem hiding this comment.
Having it in (User)AuthorizationCheckerInterface looks useful enough
2f4b88e to
be530b6
Compare
src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig
Outdated
Show resolved
Hide resolved
be530b6 to
532bc37
Compare
532bc37 to
c6eb9ae
Compare
|
What a ride for those PRs :) |
|
Thank you @nicolas-grekas. |
|
The method |
|
We'd need benchmarks to consider any perf/mem issue here. To me, this should be negligible. |
This is our primary/only reason why we wanted this feature. |
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:
Internally, this provides a new access audit log infrastructure that relies on new
AccessDecisionandVoteobjects.Note that I didn't add (nor fix) tests for now. I'll borrow (and credit) from #58107 / #46493 as much as possible.