[Security] add an AbstractVoter implementation #11183

inoryy · 2014-06-20T11:25:25Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT
Doc PR	symfony/symfony-docs#4257

The idea is to reduce boilerplate required to create custom Voter, doing most of the work for the developer and guiding him on the path by providing simple requirements via abstract methods that will be called by the AbstractVoter.

P.S. This is meant to be a DX Initiative improvement.

rybakit · 2014-06-20T11:49:23Z

👍

How about going a bit further — replace voteOnAttribute() with isGranted() which will just return true or false? Then you don't need to bother about ACCESS_GRANTED and ACCESS_DENIED constants in a concrete voter implementation.

vote() could look like:

public function vote(TokenInterface $token, $object, array $attributes)
{
    $result = self::ACCESS_ABSTAIN;

    if (!$this->supportsClass(get_class($object))) {
        return $result;
    }

    $user = $token->getUser();

    foreach ($attributes as $attribute) {
        if (!$this->supportsAttribute($attribute)) {
            continue;
        }

        if ($this->isGranted($object, $attribute, $user)) {
            return self::ACCESS_GRANTED;
        }

        $result = self::ACCESS_DENIED;
    }

    return $result;
}

stof · 2014-06-20T12:17:39Z

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

+        foreach ($attributes as $attribute) {
+            if ($this->supportsAttribute($attribute)) {
+                $vote = $this->voteOnAttribute($attribute, $object, $user);
+                if (VoterInterface::ACCESS_ABSTAIN !== $vote) {


this implementation is not consistent with the way core voters work. If you pass 2 attributes (which is not common anyway), they will grant the access as soon as one of them is accepted. Your implementation however gives more power to the first attribute by allowing it to reject everything

good point! will try to replicate core voters behavior

weaverryan · 2014-06-20T12:49:58Z

It still needs a test obviously, but I really like the implementation!

About @rybakit's comment, I thought about this too, and I like it, but I'm not sure. I suppose the ACCESS_GRANTED constants are pretty explicit, which I like. And since they live in the parent interface, it doesn't involve needing to add a use statement to use them, which I also like. So, I think I'm ok with leaving it with returning ACCESS_GRANTED, but if a bunch of other people like @rybakit's suggestion, then we should consider it :).

Such a great win imo - I hope this gets accepted - I'd be very happy to see how much more clear the documentation articles about voters become.

Thanks!

lyrixx · 2014-06-20T22:11:53Z

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

+    public function supportsClass($class)
+    {
+        foreach ($this->getSupportedClasses() as $supportedClass) {
+            if ($supportedClass === $class || is_subclass_of($class, $supportedClass)) {


http://php.net/is_a is easier IMHO

I think is_a still requires the first argument to be an object (all we have here are class names).

But you don't have to use get_class on line 50 ;)

supportsClass expects a string in the VoterInterface

ah ... Did not know ;) I never like this interface. It should be tweaked in SF3.0 IMHO.

yeah, supportsClass and supportAttribute are actually never used in Symfony except by voters themselves. They should be removed from the interface in 3.0. But in the meantime, the interface should be respected

Just ran few tests:

php > var_dump(is_a('BadFunctionCallException', 'Exception', true)); bool(true) php > var_dump(is_a('Exception', 'Exception', true)); bool(true) php > var_dump(is_a('Foobar', 'Exception', true)); bool(false)

@weaverryan:

I think is_a still requires the first argument to be an object (all we have here are class names).

See the php doc:

allow_string

If this parameter set to FALSE, string class name as object is not allowed. This also prevents from calling autoloader if the class doesn't exist.

in 5.3.3 to 5.3.6, is_a does not trigger the autoloading when passing a string. This won't cause issue for the behavior in the voter (as the class name comes from get_class and so the class is already autoloaded) but it could for other usages of the method.

good point ;)

jenkoian · 2014-06-22T16:53:16Z

I've created something really similar to this recently and have tests more or less ready to go, so let me know if I can help out with the tests or if you just want to see them or anything. 👍

javiereguiluz · 2014-08-05T17:41:32Z

I really love this idea to simplify the voters. Thanks @inoryy.

My question is: does anyone know what is needed to finish this PR? How can we help with that?

inoryy · 2014-08-05T17:50:06Z

oops, for some reason I was sure I wrapped it up. My bad, I'll put this on top of my todo list.

/cc @javiereguiluz @weaverryan

francoispluchino · 2014-08-05T17:54:27Z

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

+     * @return int     either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
+     */
+    abstract protected function voteOnAttribute($attribute, $object, UserInterface $user = null);
+}


missing newline at end of file

inoryy · 2014-08-23T07:08:18Z

@weaverryan @javiereguiluz this should be good to go.

I've fixed inconsistencies, implemented the recommended ideas where they wouldn't cause conflicts with API & added test coverage.

The build failed, but I think it's unrelated to the PR (something about Stopwatch on PHP 5.6?)

inoryy · 2014-08-23T07:36:19Z

... and now the build passed as well :)

wouterj · 2014-08-23T07:39:49Z

👍 great job @inoryy !

javiereguiluz · 2014-08-23T09:01:05Z

src/Symfony/Component/Security/Tests/Core/Authentication/Voter/AbstractVoterTest.php

+
+class ObjectFixture {}
+
+class UnsupportedObjectFixture {}


(ver minor comment) It looks like you didn't add a new line at the end of the file. These are the things that fabbot.io doesn't like ;)

javiereguiluz · 2014-08-23T09:03:02Z

👍 Great work Roman! Thanks a lot for helping us improve Symfony.

weaverryan · 2014-08-23T14:43:52Z

👍 Awesome - one of my favorite features for 2.6!

yguedidi · 2014-08-23T20:16:00Z

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

+     * It is safe to assume that $attribute and $object's class pass supportsAttribute/supportsClass
+     * $user can be one of the following:
+     *   a UserInterface object (fully authenticated user)
+     *   a string               (anonymously authenticated user)


I think a string user doesn't necessary mean it is an anonymously authenticated user

That's technically true, but I've never seen a case in practice other than the anonymous user. I'd rather be a little technically incorrect here to cover the vast majority of the use-cases (and if you are authentication a user without an object, it's likely you are more advanced and understand what's going on).

javiereguiluz · 2014-09-01T13:05:32Z

I'd like to write a "New in Symfony 2.6" post about this great feature. @inoryy do you know what's left to finish this PR? I think that you've done a terrific job so far and probably we only need a final effort. Do you think we could help you with anything? Thanks!

inoryy · 2014-09-01T14:37:14Z

@javiereguiluz I see two minor blocks:

Minor changes/corrections (mainly in docs) that I'll wrap up today.
Some arguable spots that could probably use your opinion to resolve (I've pinged on them)

weaverryan · 2014-09-01T15:41:05Z

Fwiw, we've weighed in on the arguable spots - I think we're all set there. Thanks for the fast response on this Roman!

inoryy · 2014-09-01T17:02:42Z

@weaverryan @javiereguiluz I've made the necessary changes, so it should be good to go as far as I know.

However, the test suite seems to be failing now for some reason. The only difference since last build success are doc entries and added a newline, so it seems it's not relevant to this PR?
Here's the error: https://travis-ci.org/symfony/symfony/jobs/34116312#L612

Symfony\Component\Process\Test\SigchildDisabledProcessTest::testIdleTimeoutNotExceededWhenOutputIsSent

P.S. @javiereguiluz I'm flattered that you're planning to feature this in "New in Symfony 2.6", but I also want to give credit where it's due - @weaverryan came up with the idea originally, I just implemented it :)

stof · 2014-09-01T17:55:20Z

@inoryy the process timeout tests are volatile. They are failing from time to time depending on the load of the Travis servers

inoryy · 2014-09-22T14:40:04Z

ping @javiereguiluz @weaverryan

javiereguiluz · 2014-09-22T14:46:04Z

In my opinion this can be safely merged. As @stof mentioned, the error reported by Travis has nothing to do with this code and it's caused by a volatile test.

fabpot · 2014-09-22T14:50:30Z

@inoryy You can rebase on current master as the volatile tests have been fixed now. Also, we need at least a PR on the docs for this new feature.

…custom voter

…ove/update API based on PR comments

inoryy · 2014-09-23T05:02:33Z

@fabpot I've rebased, but it still fails due to failures on master itself (failures are unrelated to this PR).
I've also added a doc PR: symfony/symfony-docs#4257

/cc @weaverryan @javiereguiluz

fabpot · 2014-09-23T05:36:51Z

I've just read the new documentation and I think this implementation has one drawback: it's not possible anymore to return ACCESS_ABSTAIN from the user code. It's not a big deal but that means we are loosing some flexibility. As the constant values are 0, 1, and -1, they don't conflict with Booleans true and false. So, what about supporting "everything"? The developer can return a Boolean, but he can also return the constants if he wants to.

Another idea if you think the constants are too cumbersome: instead of returning a Boolean, the developer can just return 0, 1, and -1 instead of using the constant. The value are pretty self describing and everyone can understand their meaning.

What do you think?

fabpot · 2014-09-23T06:02:10Z

Thinking about it more, I think we should supports the constants/numbers and remove the Booleans. That way, the abstract voter is closer to custom voters and this is still as easy as Boolean. true, false vs 0, 1, -1. Using constants or numbers is up to the developer and the documentation can favor numbers over constants.

inoryy · 2014-09-23T06:30:20Z

@fabpot I'm not sure I see the benefit of the added flexibility?
Can you provide an example use case where isGranted is called with supported attribute & object, but would still return ACCESS_ABSTAIN?

fabpot · 2014-09-23T06:34:29Z

As I said, it's a small benefit, but more important, using numbers is not more "difficult" to understand and adds consistency across the board. So, I don't see any drawback here.

inoryy · 2014-09-23T06:47:18Z

@fabpot but how to implement this considering multiple attributes?
Currently as soon as attribute and object are supported, we default to ACCESS_DENIED (-1) on this line and return ACCESS_GRANTED (1) as soon as at least one attribute grants access.
By supporting the "flexibility" on isGranted level, we'll have to respect ACCESS_ABSTAIN (0) vote as well (i.e. it would take priority over default ACCESS_DENIED), but then another attribute might cause ACCESS_DENIED as well, which would.. take priority over previous vote?

fabpot · 2014-09-23T09:50:30Z

@inoryy Indeed.

👍

fabpot · 2014-09-23T09:51:37Z

Thank you @inoryy.

ByScripts · 2014-12-10T09:24:47Z

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

+    public function vote(TokenInterface $token, $object, array $attributes)
+    {
+        if (!$object || !$this->supportsClass(get_class($object))) {
+            return self::ACCESS_ABSTAIN;


Why returning ACCESS_ABSTAIN so early? This avoids using is_granted(MY_ATTRIBUTE) without passing object.

Take the role checking as example: is_granted('ROLE_ADMIN') ... no object is passed. Ans this kind of implementation is not possible here.

At first glance, I would do something like if(!!$object && !$this->supportsClass(...)

You should open an issue. At first glance, I agree that this is something we overlooked!

@javiereguiluz

…plementation (Inoryy, javiereguiluz) This PR was submitted for the 2.7 branch but it was merged into the 2.6 branch instead (closes #5423). Discussion ---------- [Security] add & update doc entries on AbstractVoter implementation | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#11183) | Applies to | 2.6+ | Fixed tickets | - This PR finishes #4257. Commits ------- 95537d3 Added a link to the AbstractVoter cookbook 73bd908 Fixed some typos 60643f0 Removed the abstract_voter.rst.inc file 59c60b1 add fixes to abstract_voter include file e9053c0 fix problems pointed out by @javiereguiluz and @cordoval 968cb65 add & update doc entries on AbstractVoter implementation

stof added the DX label Jun 20, 2014

stof reviewed Jun 20, 2014
View reviewed changes

lyrixx reviewed Jun 20, 2014
View reviewed changes

francoispluchino reviewed Aug 5, 2014
View reviewed changes

sstok mentioned this pull request Aug 17, 2014

Question: VoterInterface supportsClass #11686

Closed

javiereguiluz reviewed Aug 23, 2014
View reviewed changes

yguedidi reviewed Aug 23, 2014
View reviewed changes

inoryy added 2 commits September 23, 2014 07:11

add abstract voter implementation, reducing boilerplate required for …

4a7bfe2

…custom voter

update AbstractVoter: vote returns as soon as access is granted; impr…

8c774e8

…ove/update API based on PR comments

inoryy mentioned this pull request Sep 23, 2014

[Security] add & update doc entries on AbstractVoter implementation symfony/symfony-docs#4257

Closed

add AbstractVoter tests; add licensing information

33413fc

fabpot closed this in 1fb8f88 Sep 23, 2014

ByScripts reviewed Dec 10, 2014
View reviewed changes

javiereguiluz mentioned this pull request Jun 22, 2015

[Security] add & update doc entries on AbstractVoter implementation symfony/symfony-docs#5423

Closed

[Security] add an AbstractVoter implementation #11183

[Security] add an AbstractVoter implementation #11183

Conversation

inoryy commented Jun 20, 2014

rybakit commented Jun 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Jun 20, 2014

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

jenkoian commented Jun 22, 2014

javiereguiluz commented Aug 5, 2014

inoryy commented Aug 5, 2014

Choose a reason for hiding this comment

inoryy commented Aug 23, 2014

inoryy commented Aug 23, 2014

wouterj commented Aug 23, 2014

Choose a reason for hiding this comment

javiereguiluz commented Aug 23, 2014

weaverryan commented Aug 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Sep 1, 2014

inoryy commented Sep 1, 2014

weaverryan commented Sep 1, 2014

inoryy commented Sep 1, 2014

stof commented Sep 1, 2014

inoryy commented Sep 22, 2014

javiereguiluz commented Sep 22, 2014

fabpot commented Sep 22, 2014

inoryy commented Sep 23, 2014

fabpot commented Sep 23, 2014

fabpot commented Sep 23, 2014

inoryy commented Sep 23, 2014

fabpot commented Sep 23, 2014

inoryy commented Sep 23, 2014

fabpot commented Sep 23, 2014

fabpot commented Sep 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment