-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[Security] Add ability for voters to explain their vote #59771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d664683 to
fcc69a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| } | ||
|
|
||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
…e more possibilities to AccessDecicsionStrategy (eltharin) This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [Security] improve VoteObject adding extraData for give more possibilities to AccessDecicsionStrategy | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | License | MIT In continuation of #58107 and #59771, add ExtraData in VoteObject and pass it to AccessDecicsionStrategy Object to allow the decision to be refined. ExtraData can be an array or an object, and AccessDecicsionStrategy can get it to get the final decision. In 7.2, voter can only respond abstain / allow or deny, but what if we want more choices, per example a ponderable vote ? With this PR, it allow to put somme data in the new VoteObject as : ```php /** ScoreData.php */ /** MyVoter.php */ public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null) : int { $vote->result = 1; $vote->reasons[] = 'is Admin'; $vote->extraData['score'] = 10; return $vote->result; } ``` we need also a custom strategy to take this score into account : ```php /** MyStrategy.php */ public function decide(\Traversable $results, $accessDecision = null): bool { $score = 0; foreach ($results as $key => $result) { $vote = $accessDecision->votes[$key]; // <== if(array_key_exists('score', $vote->extraData)) { $score += $vote->extraData['score']; } else { $score += $vote->result; } } $accessDecision->result = $score; if ($score > 0) { return true; } if ($score< 0) { return false; } return $this->allowIfAllAbstainDecisions; } ``` AccessDecision contains Vote objects and we can read our score from it. Commits ------- bd24f84 [Security] improve VoteObject adding extraData for give more possibilities to AccessDecicsionStrategy
…e more possibilities to AccessDecicsionStrategy (eltharin) This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [Security] improve VoteObject adding extraData for give more possibilities to AccessDecicsionStrategy | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | License | MIT In continuation of symfony/symfony#58107 and symfony/symfony#59771, add ExtraData in VoteObject and pass it to AccessDecicsionStrategy Object to allow the decision to be refined. ExtraData can be an array or an object, and AccessDecicsionStrategy can get it to get the final decision. In 7.2, voter can only respond abstain / allow or deny, but what if we want more choices, per example a ponderable vote ? With this PR, it allow to put somme data in the new VoteObject as : ```php /** ScoreData.php */ /** MyVoter.php */ public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null) : int { $vote->result = 1; $vote->reasons[] = 'is Admin'; $vote->extraData['score'] = 10; return $vote->result; } ``` we need also a custom strategy to take this score into account : ```php /** MyStrategy.php */ public function decide(\Traversable $results, $accessDecision = null): bool { $score = 0; foreach ($results as $key => $result) { $vote = $accessDecision->votes[$key]; // <== if(array_key_exists('score', $vote->extraData)) { $score += $vote->extraData['score']; } else { $score += $vote->result; } } $accessDecision->result = $score; if ($score > 0) { return true; } if ($score< 0) { return false; } return $this->allowIfAllAbstainDecisions; } ``` AccessDecision contains Vote objects and we can read our score from it. Commits ------- bd24f84ab90 [Security] improve VoteObject adding extraData for give more possibilities to AccessDecicsionStrategy
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.