[Security] Trigger a deprecation when a voter is missing the VoterInterface #22629

linaori · 2017-05-04T08:48:27Z

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.

stof · 2017-05-04T08:58:59Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSecurityVotersPass.php

@@ -40,6 +41,20 @@ public function process(ContainerBuilder $container)
            throw new LogicException('No security voters found. You need to tag at least one with "security.voter"');
        }

+        foreach ($voters as $voter) {
+            $class = $container->getDefinition($voter->__toString())->getClass();


use a string cast instead.

Are you sure? That's harder to trace back (I'm not sure why toString is used either way)

stof · 2017-05-04T08:59:45Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSecurityVotersPass.php

+
+            if (!method_exists($class, 'vote')) {
+                // in case the vote method is completely missing, to prevent exceptions when voting
+                throw new LogicException(sprintf('%s should implement the %s class when used as voter.', $class, VoterInterface::class));


it is an interface, not a class

stof · 2017-05-04T09:00:30Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSecurityVotersPass.php

+
+            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));


this log should be removed, as the deprecation is logged too already

vamsiikrishna · 2017-05-04T10:14:44Z

I have got some Voters which are tagged as voters but don't implement VoterInterface .
Thanks for addressing this and adhering to BC promise .

xabbuh · 2017-05-04T11:53:39Z

UPGRADE-3.4.md

+Security
+--------
+
+ * Using voters in the `AccessDecisionManager` without `VoterInterface` is now


[...] that do not implement the VoterInterface [...]

xabbuh · 2017-05-04T12:29:33Z

UPGRADE-3.4.md

+Security
+--------
+
+ * Using voters that do not implement the `VoterInterface`, are now deprecated


"is" and you can remove the comma

ro0NL · 2017-05-04T18:12:01Z

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php

+            return $voter->vote($token, $subject, $attributes);
+        }
+
+        throw new \BadMethodCallException(sprintf('%s should implement the %s interface when used as voter.', get_class($voter), VoterInterface::class));


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.

I would like to have it because it's a component that can be used without the DIC

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.

fabpot · 2017-05-05T14:12:57Z

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php

+     * TokenInterface vote proxy method.
+     *
+     * Acts as a BC layer when the VoterInterface is not implemented on the voter.
+     */


I would add a note to remove this method in 4.0 (to avoid forgetting about it).

I could make the method itself deprecated with a full-fledged warning, even though it's private. Would make it easier to detect.

ogizanagi · 2017-05-07T00:07:12Z

...mfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSecurityVotersPassTest.php

+
+        if (method_exists($this, 'expectException')) {
+            $this->expectException($exception);
+            $this->expectExceptionMessage($message);


What about using the annotations?

linaori · 2017-06-15T06:56:07Z

@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 · 2017-06-15T14:21:13Z

@iltar Weird, not sure what happened. Merging now!

fabpot · 2017-06-15T14:23:29Z

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

Trigger deprecation when missing VoterInterface

0df7c95

carsonbot added Status: Needs Review Security Bug Deprecation labels May 4, 2017

Fixed phpunit compatibility

c8b5f80

stof requested changes May 4, 2017

View reviewed changes

Fixed feedback

4a13ef5

xabbuh reviewed May 4, 2017

View reviewed changes

Rewording in the change- and upgrade-log

ab0ab44

ro0NL reviewed May 4, 2017

View reviewed changes

LogicException instead of BadMethodCallException

b187304

fabpot approved these changes May 5, 2017

View reviewed changes

Added a deprecation warning to AccessDecisionManager::vote()

40ac4ec

ogizanagi approved these changes May 7, 2017

View reviewed changes

fabpot added this to the 3.4 milestone May 11, 2017

nicolas-grekas changed the base branch from master to 3.4 May 23, 2017 16:55

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels May 23, 2017

fabpot approved these changes Jun 14, 2017

View reviewed changes

fabpot closed this Jun 14, 2017

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

linaori deleted the feature/voter-interface-restrictions branch February 8, 2019 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Trigger a deprecation when a voter is missing the VoterInterface #22629

[Security] Trigger a deprecation when a voter is missing the VoterInterface #22629

linaori commented May 4, 2017 •

edited

Loading

stof May 4, 2017

linaori May 4, 2017

stof May 4, 2017

stof May 4, 2017

vamsiikrishna commented May 4, 2017 •

edited

Loading

xabbuh May 4, 2017

xabbuh May 4, 2017

ro0NL May 4, 2017

linaori May 4, 2017

ro0NL May 5, 2017

fabpot May 5, 2017

linaori May 5, 2017

ogizanagi May 7, 2017

linaori commented Jun 15, 2017

fabpot commented Jun 15, 2017

fabpot commented Jun 15, 2017

[Security] Trigger a deprecation when a voter is missing the VoterInterface #22629

[Security] Trigger a deprecation when a voter is missing the VoterInterface #22629

Conversation

linaori commented May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vamsiikrishna commented May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Jun 15, 2017

fabpot commented Jun 15, 2017

fabpot commented Jun 15, 2017

linaori commented May 4, 2017 •

edited

Loading

vamsiikrishna commented May 4, 2017 •

edited

Loading