[Security] Deprecate "AbstractVoter" in favor of "Voter" #16601

lyrixx · 2015-11-19T14:23:21Z

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#16556, #16558, #16554
License	MIT
Doc PR	-

stof · 2015-11-19T14:27:27Z

src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php

@@ -53,10 +53,10 @@ public function supportsClass($class);
     * ACCESS_GRANTED, ACCESS_DENIED, or ACCESS_ABSTAIN.
     *
     * @param TokenInterface $token      A TokenInterface instance
-     * @param object|null    $object     The object to secure
+     * @param mixed          $subject    The subject to secure


this is a BC break, as explained in #16600

stof · 2015-11-19T14:29:03Z

I don't see any reason to deprecated AbstractVoter in favor of an abstract Voter class (which does not follow our naming conventions btw). This does not remove the BC break

lyrixx · 2015-11-19T14:30:53Z

I don't see any reason to deprecated AbstractVoter

I deprecated it because I added a new "base voter".
I added a new "base voter" in order to fix listed issue.

lyrixx · 2015-11-20T15:25:05Z

I reverted the change on the interface to ease integration of this PR.

nicolas-grekas · 2015-11-20T15:33:14Z

👍
Status: reviewed

It removes a limitation of the current AbstractVoter (the first if block of the vote method) that can't be fixed with a simple deprecation notice. Which means we need a new class name. Voter is fine to me (abstract, like Command is).
@weaverryan I'd be happy to have your vote here since you cared about the linked issue.

weaverryan · 2015-11-20T18:35:40Z

UPGRADE-2.8.md

@@ -475,6 +475,8 @@ Security
   }
   ```

+ * The `AbstractVoter` is deprecated. Upgrade to `Voter` instead.
+


If we're deprecating AbstractVoter entirely, then we don't need to talk about it at all, right? The upgrade path would be to switch to Voter and make the changes needed there, correct? If so, I think we should have one simple, before and after (before AbstractVoter after with Voter).

If people were using AbstractVoter in 2.7, they can read how to upgrade to 2.8;
Then they could upgrade with the new Voter. Like that it's done step by step.

But I can merge all steps together.

Or, we should maybe just revert the changes made on AbstractVoter for 2.8 entirely, but keep the deprecations. Ie remove the supports method & co. Don't you think ?

This is a real BC break

How that? SF 2.8 is not released, how can it be à BC break to revert the changes made on 2.8?

Because the implementation sticks on the implementation of the 2.7 version.

weaverryan · 2015-11-20T18:38:57Z

I left a few comments, but I'm "pro" this idea and I like the name.

weaverryan · 2015-11-22T23:50:03Z

But I also think this can wait until 3.1... It's great, but I'm not sure it's important enough to stick into the release at the last second: there's too much else going on. If we do save this for 3.1, then I can update the AbstractVoter docs (I can't work on that til this gets a decision).

weaverryan · 2015-11-22T23:50:29Z

Whoops - I meant wait until 3.1 - I updated my comment.

nicolas-grekas · 2015-11-24T14:03:49Z

We worked together with @lyrixx on this, the PR now has two commits:

the first one (d3c6d93) reverts all changes made between 2.7 and 2.8-beta on AbstractVoter and deprecates it instead, see diff below;
the second one moves all this work made between 2.7 and 2.8-beta to the new Voter class.

The new Voter class is now more generic, at least it is generic enough to be reused by all the concrete voters we provide by default (to be done in an other PR).

We can postpone this deprecation to 3.1, but it would be quite unfortunate to ask devs to update their app twice. It's late but maybe not too late to fix this limitation now. That would reduce the burden significantly on this topic.

git diff 2.7...d3c6d93 -- src/Symfony/Component/Security/Core/Authorization/Voter/

diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php b/src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php
index efa1562..5dcf787 100644
--- a/src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php
+++ b/src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php
@@ -11,6 +11,8 @@

 namespace Symfony\Component\Security\Core\Authorization\Voter;

+@trigger_error('The '.__NAMESPACE__.'\AbstractVoter class is deprecated since version 2.8, to be removed in 3.0. Upgrade to Symfony\Component\Security\Core\Authorization\Voter\Voter instead.', E_USER_DEPRECATED);
+
 use Symfony\Component\Security\Core\User\UserInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;

@@ -18,6 +20,8 @@ use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
  * Abstract Voter implementation that reduces boilerplate code required to create a custom Voter.
  *
  * @author Roman Marint<C5><A1>enko <inoryy@gmail.com>
+ *
+ * @deprecated since version 2.8, to be removed in 3.0. Upgrade to Symfony\Component\Security\Core\Authorization\Voter\Voter instead.
  */
 abstract class AbstractVoter implements VoterInterface
 {
diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php b/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php
index d00ff1c..7e243f9 100644
--- a/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php
+++ b/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php
@@ -30,6 +30,8 @@ interface VoterInterface
      * @param string $attribute An attribute
      *
      * @return bool true if this Voter supports the attribute, false otherwise
+     *
+     * @deprecated since version 2.8, to be removed in 3.0.
      */
     public function supportsAttribute($attribute);

@@ -39,6 +41,8 @@ interface VoterInterface
      * @param string $class A class name
      *
      * @return bool true if this Voter can process the class
+     *
+     * @deprecated since version 2.8, to be removed in 3.0.
      */
     public function supportsClass($class);

stof · 2015-11-24T15:09:26Z

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

+            $vote = self::ACCESS_DENIED;
+
+            if ($this->voteOnAttribute($attribute, $object, $token)) {
+                // grant access as soon as at least one voter returns a positive response


this should be at least one attribute. We are inside the voter here. We don't have multiple voters

stof · 2015-11-24T15:10:09Z

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

+     * Determines if the attribute and object are supported by this voter.
+     *
+     * @param string $attribute An attribute
+     * @param mixed  $object    The object to secure


this should be renamed if it is not always an object

Well, grammatically, it's an object. But a synonym would be less confusing I agree. Here are some, any preference?

I think subject could be used instead.

I just updated the PR to use "matter" instead, as suggested by thesaurus.com.

$matter is not a common term and it may be difficult to understand it. Could we call it $object and add a note in the PHPdocs saying that this can also be a non-object? By the way, how common is to pass a non-object to this argument?

@javiereguiluz frequent enough to raise all this discussion and the related issues :)

weaverryan · 2015-11-24T15:52:15Z

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

+     *
+     * @return bool True if the attribute and matter are supported, false otherwise
+     */
+    abstract protected function supports($attribute, $matter);


$matter is a weird word in English - usually it only refers to the scientific meaning of matter (https://en.wikipedia.org/wiki/Matter).

I liked $subject better, and I think we should hint in the docs that this is often an object:

/** * @param mixed $subject The subject to secure (e.g. an object the user wants to access) */

Lets go for subject, thanks for the feedback. I used The subject to secure, e.g. an object the user wants to access or any other PHP type

nicolas-grekas · 2015-11-24T16:52:35Z

Thank you all for the feedback, all comments are now resolved, votes pending :)

fabpot · 2015-11-25T12:49:27Z

So, IIUC, the AbstractVoter class is the same as in 2.7 with the deprecation notices added and the new Voter class only removed the if statement that was problematic. Looks like a no-brainer to me. 👍

weaverryan · 2015-11-25T13:58:47Z

👍 I agree - that bug was always something we wanted to fix - we should have just done it all at once (like this PR).

nicolas-grekas · 2015-11-26T14:48:56Z

Thank you @lyrixx.

…r" (nicolas-grekas, lyrixx) This PR was merged into the 2.8 branch. Discussion ---------- [Security] Deprecate "AbstractVoter" in favor of "Voter" | Q | A | ------------- | --- | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #16556, #16558, #16554 | License | MIT | Doc PR | - Commits ------- fd8b87c [Security] Deprecate "AbstractVoter" in favor of "Voter" d3c6d93 [Security] Revert changes made between 2.7 and 2.8-beta

carsonbot added the Status: Needs Review label Nov 19, 2015

lyrixx force-pushed the new-voter branch from 2fb4f3d to cf08e4c Compare November 19, 2015 14:23

stof reviewed Nov 19, 2015
View reviewed changes

lyrixx force-pushed the new-voter branch 6 times, most recently from 3c91b4d to eeb1815 Compare November 20, 2015 15:07

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 20, 2015

weaverryan reviewed Nov 20, 2015
View reviewed changes

[Security] Revert changes made between 2.7 and 2.8-beta

d3c6d93

nicolas-grekas force-pushed the new-voter branch from eeb1815 to b6ee425 Compare November 24, 2015 13:43

nicolas-grekas changed the title ~~[Security] Add an abstract voter~~ [Security] Deprecate "AbstractVoter" in favor of "Voter" Nov 24, 2015

nicolas-grekas force-pushed the new-voter branch from b6ee425 to 39ade26 Compare November 24, 2015 13:53

nicolas-grekas force-pushed the new-voter branch from 39ade26 to d3028c5 Compare November 24, 2015 14:07

nicolas-grekas added Status: Needs Review and removed Status: Reviewed labels Nov 24, 2015

stof reviewed Nov 24, 2015
View reviewed changes

nicolas-grekas force-pushed the new-voter branch from d3028c5 to fe48477 Compare November 24, 2015 15:34

weaverryan reviewed Nov 24, 2015
View reviewed changes

[Security] Deprecate "AbstractVoter" in favor of "Voter"

fd8b87c

nicolas-grekas force-pushed the new-voter branch from fe48477 to fd8b87c Compare November 24, 2015 15:57

nicolas-grekas merged commit fd8b87c into symfony:2.8 Nov 26, 2015

nicolas-grekas deleted the new-voter branch November 26, 2015 14:53

This was referenced Nov 26, 2015

[Security] Abstain vote for scalar values. #16558

Closed

Why can't (shouldn't) primitive types be passed to AuthorizationChecker::isGranted #16554

Closed

[Security] AbstractVoter should abstain string objects #16531

Closed

weaverryan mentioned this pull request Nov 27, 2015

Update voter documentation for supportsAttribute and supportsClass deprecation symfony/symfony-docs#5728

Closed

xabbuh mentioned this pull request Nov 30, 2015

[Security] VoterInterface::vote(TokenInterface $token, $subject, array $attributes) #16600

Closed

This was referenced Nov 30, 2015

Release v2.8.0 #16763

Merged

Release v3.0.0 #16766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Deprecate "AbstractVoter" in favor of "Voter" #16601

[Security] Deprecate "AbstractVoter" in favor of "Voter" #16601

lyrixx commented Nov 19, 2015

stof Nov 19, 2015

stof commented Nov 19, 2015

lyrixx commented Nov 19, 2015

lyrixx commented Nov 20, 2015

nicolas-grekas commented Nov 20, 2015

weaverryan Nov 20, 2015

lyrixx Nov 20, 2015

nicolas-grekas Nov 20, 2015

lyrixx Nov 20, 2015

nicolas-grekas Nov 20, 2015

lyrixx Nov 21, 2015

weaverryan commented Nov 20, 2015

weaverryan commented Nov 22, 2015

weaverryan commented Nov 22, 2015

nicolas-grekas commented Nov 24, 2015

stof Nov 24, 2015

nicolas-grekas Nov 24, 2015

stof Nov 24, 2015

nicolas-grekas Nov 24, 2015

jvasseur Nov 24, 2015

nicolas-grekas Nov 24, 2015

javiereguiluz Nov 24, 2015

nicolas-grekas Nov 24, 2015

weaverryan Nov 24, 2015

nicolas-grekas Nov 24, 2015

nicolas-grekas commented Nov 24, 2015

fabpot commented Nov 25, 2015

weaverryan commented Nov 25, 2015

nicolas-grekas commented Nov 26, 2015

[Security] Deprecate "AbstractVoter" in favor of "Voter" #16601

[Security] Deprecate "AbstractVoter" in favor of "Voter" #16601

Conversation

lyrixx commented Nov 19, 2015

Choose a reason for hiding this comment

stof commented Nov 19, 2015

lyrixx commented Nov 19, 2015

lyrixx commented Nov 20, 2015

nicolas-grekas commented Nov 20, 2015

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

weaverryan commented Nov 20, 2015

weaverryan commented Nov 22, 2015

weaverryan commented Nov 22, 2015

nicolas-grekas commented Nov 24, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Nov 24, 2015

fabpot commented Nov 25, 2015

weaverryan commented Nov 25, 2015

nicolas-grekas commented Nov 26, 2015