[Security] Add setVoters() on AccessDecisionManager #14733
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.
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.
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.
ce36374 to
3fd7cea
Compare
|
👍 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 it may 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 #14550 Commits ------- 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 yet This fixes #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