Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Trigger a deprecation when a voter is missing the VoterInterface#22629
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
| } | ||
| foreach ($votersas$voter) { | ||
| $class =$container->getDefinition($voter->__toString())->getClass(); |
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.
use a string cast instead.
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.
Are you sure? That's harder to trace back (I'm not sure why toString is used either way)
| if (!method_exists($class,'vote')) { | ||
| // in case the vote method is completely missing, to prevent exceptions when voting | ||
| thrownewLogicException(sprintf('%s should implement the %s class when used as voter.',$class, VoterInterface::class)); |
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.
it is an interface, not a class
| if (!is_a($class, VoterInterface::class,true)) { | ||
| @trigger_error(sprintf('Using a security.voter tag on a class without implementing the %1$s is deprecated as of 3.4 and will be removed in 4.0. Implement the %1$s instead.', VoterInterface::class),E_USER_DEPRECATED); | ||
| $container->log($this,sprintf('Detected usage of security.voter for class "%s" without implementing the %s.',$class, VoterInterface::class)); |
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 log should be removed, as the deprecation is logged too already
vamsiikrishna commentedMay 4, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I have got some Voters which are tagged as voters but don't implement |
UPGRADE-3.4.md Outdated
| Security | ||
| -------- | ||
| * Using voters in the`AccessDecisionManager` without`VoterInterface` is now |
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.
[...] that do not implement theVoterInterface [...]
UPGRADE-3.4.md Outdated
| Security | ||
| -------- | ||
| * Using voters that do not implement the`VoterInterface`, are now deprecated |
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.
"is" and you can remove the comma
| return$voter->vote($token,$subject,$attributes); | ||
| } | ||
| thrownew \BadMethodCallException(sprintf('%s should implement the %s interface when used as voter.',get_class($voter), VoterInterface::class)); |
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 would still qualify it a\LogicException :) it's only possible by violating the contract. So technically this is a safeguard.. do we need it? Given the validation during compilation. 👍 for that.
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 would like to have it because it's a component that can be used without the DIC
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.
It's still a safeguard :) sure it adds some DX value, but nowhere near the value added by the pass (validation at compile time). Practically for the component this is just creating cosmetic errors, which we dont do elsewhere really.
| * TokenInterface vote proxy method. | ||
| * | ||
| * Acts as a BC layer when the VoterInterface is not implemented on the voter. | ||
| */ |
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 would add a note to remove this method in 4.0 (to avoid forgetting about it).
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 could make the method itself deprecated with a full-fledged warning, even though it's private. Would make it easier to detect.
| if (method_exists($this,'expectException')) { | ||
| $this->expectException($exception); | ||
| $this->expectExceptionMessage($message); |
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.
What about using the annotations?
linaori commentedJun 15, 2017
@fabpot while you approved it, you also closed it but I don't see another commit in the list for 3.4 (as it usually does), I think something went wrong |
fabpot commentedJun 15, 2017
@iltar Weird, not sure what happened. Merging now! |
fabpot commentedJun 15, 2017
Thank you@iltar. |
…ng the VoterInterface (iltar)This PR was squashed before being merged into the 3.4 branch (closes#22629).Discussion----------[Security] Trigger a deprecation when a voter is missing the VoterInterface| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | ~| License | MIT| Doc PR | ~Right now it's possible to add voters to the access decision manager that do not have a `VoterInterface`. - No Interface, no `vote()` method, and it will give a PHP error. - No Interface, but `vote()` method, it will still work. - If I don't implement the interface _and_ have no `vote()` method, I will get weird exception that's not meaningful: `Attempted to call an undefined method named "vote" of class "App\Voter\MyVoter".`This PR will deprecate the ability to use voters without the interface, it will also throw a proper exception when missing the interface _and_ the `vote()` method. Why when using and not when setting? Due to the fact that the voters can be set lazily via the `IteratorArgument`. The SecurityBundle will trigger a deprecation if the interface is not implemented and an exception if there's not even a `vote()` method present (to prevent exceptions at run-time).This should have full backwards compatibility with 3.3, but give more meaningful errors. The only behavioral difference, might be that the container will throw an exception instead of maybe succeeding in voting when 1 voter would be broken at the end of the list (based on strategy). This case however, will be detected during development and deployment, rather than run-time.Commits-------9c253e1 [Security] Trigger a deprecation when a voter is missing the VoterInterface
Uh oh!
There was an error while loading.Please reload this page.
Right now it's possible to add voters to the access decision manager that do not have a
VoterInterface.vote()method, and it will give a PHP error.vote()method, it will still work.vote()method, I will get weird exception that's not meaningful:Attempted to call an undefined method named "vote" of class "App\Voter\MyVoter".This PR will deprecate the ability to use voters without the interface, it will also throw a proper exception when missing the interfaceand the
vote()method. Why when using and not when setting? Due to the fact that the voters can be set lazily via theIteratorArgument. The SecurityBundle will trigger a deprecation if the interface is not implemented and an exception if there's not even avote()method present (to prevent exceptions at run-time).This should have full backwards compatibility with 3.3, but give more meaningful errors. The only behavioral difference, might be that the container will throw an exception instead of maybe succeeding in voting when 1 voter would be broken at the end of the list (based on strategy). This case however, will be detected during development and deployment, rather than run-time.