Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Security] Add setVoters() on AccessDecisionManager#14733
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -41,12 +41,8 @@ class AccessDecisionManager implements AccessDecisionManagerInterface | |||
| * | |||
| * @throws \InvalidArgumentException | |||
| */ | |||
| public function __construct(array $voters, $strategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true) | |||
| public function __construct(array $voters = array(), $strategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true) | |||
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.
I don't see how it would make a difference anyway, but we don't need to make this option - the "old" way would of course also allow an empty array.
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.
This is not required, but if setting no voters on instantiation is allowed, I thought it would be more consistent to make the argument optional also.
👍 I tested this - it allows for the For docs:symfony/symfony-docs#5306, I have an issue to doc this. |
@weaverryan wouldn't that lead to potential recursion? |
@iltar Recursion where? Do you mean a circular reference? If so, then no - the container is able to create the AccessDecisionManager, then create the voters, then finish the AccessDecisionManager by calling |
@weaverryan : I think what@iltar means isn't about the circular object graph, but in the case of a voter |
@ogizanagi OOOH, yes, of course - I don't know how I missed that. Yes, it's possible - but I agree with you - that would be an error on the developer, and one that they would suffer during development and fix. And this is already true today if you use a voter to go back through the AccessDecisionManager ("possible" if you inject the container into a voter - it's hacky, hence this PR). |
@weaverryan that's exactly what I meant. Maybe we could somehow make the AccesDecisionManager track what it's voting for, so when it tries to vote again on the same role, it will throw an exception. |
I don't think it's possible because itmay make sense to call a voter recursively - the attributes or token may be different, or the voter may be holding some state (it shouldn't) that makes it act different the second time. I like the thought - but I'm not sure it's possible |
Thank you@nicolas-grekas. |
…icolas-grekas)This PR was merged into the 2.8 branch.Discussion----------[Security] Add setVoters() on AccessDecisionManager| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | partially#12360| License | MIT| Doc PR | -Alternative for#14550Commits-------3fd7cea [Security] Add setVoters() on AccessDecisionManager
… TokenInterface (weaverryan)This PR was squashed before being merged into the 2.8 branch (closes#15870).Discussion----------Updating AbstractVoter so that the method receives the TokenInterface| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#12360| License | MIT| Doc PR | not yetThisfixes#12360, and along with already-merged#14733, this would make it possible to make calls back to the `AccessDecisionManager` inside a voter (e.g. you might check to see if `IS_AUTHENTICATED_FULLY` from inside your voter).We originally passed the User instead of the token to be nice, but it's a limitation, and since we never sanitized the User (i.e. a string may be passed to `AbstractToken::isGranted()`), it's not helpful anyways.Thanks!Commits-------948ccec Updating AbstractVoter so that the method receives the TokenInterface
Alternative for#14550