[Security] Add the ability for voter to return decision reason #46493
Conversation
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getDecision(TokenInterface $token, array $attributes, mixed $object = null, bool $allowMultipleAttributes = false): AccessDecision |
There was a problem hiding this comment.
$allowMultipleAttributes is in decide method (but not in interface). I think we can remove this arg and the check WDYT ?
Original comment:
Special case for AccessListener, do not remove the right side of the condition before 6.0
related to GHSA-g4m9-5hpf-hx72
src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php
Outdated
Show resolved
Hide resolved
| @@ -34,6 +36,8 @@ interface VoterInterface | |||
| * @param array $attributes An array of attributes associated with the method being invoked | |||
| * | |||
| * @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED | |||
| * | |||
| * @deprecated since Symfony 6.2, use {@see getVote()} instead. | |||
There was a problem hiding this comment.
Note sure if is the best way.
Also naming is probably not the best, maybe voteDecision is better ?
There was a problem hiding this comment.
what about doVote() ?
and when doing v7.0 we can revert to vote()
|
Hey! I think @NicoHaase has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php
Outdated
Show resolved
Hide resolved
| @@ -34,6 +36,8 @@ interface VoterInterface | |||
| * @param array $attributes An array of attributes associated with the method being invoked | |||
| * | |||
| * @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED | |||
| * | |||
| * @deprecated since Symfony 6.2, use {@see getVote()} instead. | |||
There was a problem hiding this comment.
what about doVote() ?
and when doing v7.0 we can revert to vote()
| */ | ||
| public function getDecision(TokenInterface $token, array $attributes, mixed $object = null, bool $allowMultipleAttributes = false): AccessDecision | ||
| { | ||
| // Special case for AccessListener, do not remove the right side of the condition before 6.0 |
There was a problem hiding this comment.
Related to #46493 (comment)
This was not removed on 6.x. I don't know why, and its impacts (CVE-2020-5275)
| return $this->messages; | ||
| } | ||
|
|
||
| public function getMessage(): string |
There was a problem hiding this comment.
what about a more specific name?
like getReasonMessage or something like that?
f2a827b to
28f4310
Compare
|
Hi all, I rebased on branch 6.3. This feature improve a lot DX and I hope it could be merge before 7.0. (6.3 ? 😃) Feedback/review are welcome 8000 d ! (@94noni Thanks for your first review, i'm waiting other review to see what are the best namming) |
28f4310 to
6cc3036
Compare
|
This PR is really cool and provides extremely valuable insights in the voting process. @nicolas-grekas given you modified the milestone from 6.3 to 6.4 in May, any idea on what it would still take to deliver it with 6.4 (aside of getting CI green)? |
|
I think this PR need to be rebases on 6.4 as 6.3 is live friendly ping @chalasr as « codeowner » on security related code |
14b1e9a to
58f1725
Compare
58f1725 to
a23e15b
Compare
|
I rebased on 6.4. Considering the number of upvote on issue, the feature is highly expected. Reviewer are welcomed ! 🙏 |
0ee4e7a to
068a117
Compare
f78bc07 to
fb81994
Compare
fb81994 to
5a3bf54
Compare
There was a problem hiding this comment.
I never opened this PR and I'm not expert in the domain, but now that I reviewed it, I'm wondering if all those deprecations aren't too much. It feels like we're throwing out a subsystem to replace it by a more capable one. That might be too costly for the community.
I'm looking for an approach that'd augment the existing voters instead of requiring everyone to rewrite theirs. This means that if a voter doesn't expose reasons, it would be fine writing them as today. And more capable ones could opt-in for exposing more detailed info.
Here is one idea: we allow voteOnAttribute to return int|string, string being meant as the reason to deny.
Then, we'd need another convention to allow the vote() method to give this info back to the caller. To do that, could we use an attribute on the token object? Another idea: we could document that voters may accept a last ?string &$reason argument, and if they do, they should populate it with the reason phrase. This would be in the interface as an @param so that we don't force implementing it.
The same reasoning would then propagate to access decisions.
WDYT?
|
thank for stopping by and for commenting here :) @nicolas-grekas obviously since the first idea/and PR of this idea of the security voting return reason, the codebase evolves as well as the langage and also implementations ideas/developpers involved this feature is, to me at least, still very relevant (think of workflow transition blocker message, etc) and can add lot of value i am not the code owner of this PR but have reviewed a lot this one, as well as all priors one, and I dont mind the implementation details as much as the feature is easily integrated/maintainable in the current codebase for core team and obviously for the upgrade path for developpers @alamirault i am opened for a discussion if you need so, perhaps we can find good compromise to make this advance! cheers :) |
|
It's been such a long time, and I was busy with personal work. That said, I think this is still relevant. And perhaps with the new direction we can pick it up again. There have been so many changes in the code base that merging does not seem like an option now. The way to move ahead is to probably start from scratch with the latest code base. |
|
HI, |
|
I took over in #59771, thanks for giving this a try! |
…e (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:   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
This PR continue the work started by @maidmaid and @noniagriconomie in #35592, continued by @yellow1912 in #43147.
It allow to have informations about AccessDecisionManager and Voters (And understand why we have an AccessDeniedException with clear infos).
I rebased on
6.26.36.47.1 and adapt code in some casesTODO: