Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security] add an AbstractVoter implementation #11183

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
@inoryy
Copy link
Contributor

commented Jun 20, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2014

👍

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 stof added the DX label Jun 20, 2014

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

This comment has been minimized.

Copy link
@stof

stof Jun 20, 2014

Member

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

This comment has been minimized.

Copy link
@inoryy

inoryy Jun 20, 2014

Author Contributor

good point! will try to replicate core voters behavior

{
if (!$this->supportsClass(get_class($object))) {
return VoterInterface::ACCESS_ABSTAIN;
}

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 20, 2014

Member

What if $object is null? I suppose we should only bother checking supportsClass if there is an object?

This comment has been minimized.

Copy link
@inoryy

inoryy Jun 20, 2014

Author Contributor

Hmm, I'm conflicted on this.
The VoterInterface API/doc doesn't actually support $object to be null: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php#L52

But then again I know for a fact that it's possible to have non-object voters..

I suppose adding as simple check that object isn't null won't hurt

This comment has been minimized.

Copy link
@stof

stof Jun 20, 2014

Member

it supports it (see the RoleVoter). It is a phpdoc issue.

@weaverryan

This comment has been minimized.

Copy link
Member

commented Jun 20, 2014

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!

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

This comment has been minimized.

Copy link
@lyrixx

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 21, 2014

Member

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

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jun 21, 2014

Member

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

This comment has been minimized.

Copy link
@stof

stof Jun 21, 2014

Member

supportsClass expects a string in the VoterInterface

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jun 21, 2014

Member

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

This comment has been minimized.

Copy link
@stof

stof Jun 21, 2014

Member

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

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jun 21, 2014

Member

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)

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jun 21, 2014

Member

@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.

This comment has been minimized.

Copy link
@stof

stof Jun 21, 2014

Member

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.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jun 21, 2014

Member

good point ;)

return VoterInterface::ACCESS_ABSTAIN;
}
$user = $token->getUser();

This comment has been minimized.

Copy link
@jenkoian

jenkoian Jun 22, 2014

Contributor

As $token->getUser() could return a string (e.g. username) you may want to add a check here to ensure $user is a UserInterface as you typehint for UserInterface in voteOnAttribute().

*/
public function supportsAttribute($attribute)
{
return in_array($attribute, $this->getSupportedAttributes());

This comment has been minimized.

Copy link
@jenkoian

jenkoian Jun 22, 2014

Contributor

Wondering if it's worth normalizing case here so it will work whether the user is doing is_granted('create', foo) or is_granted('CREATE', foo).

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 10, 2014

Member

That's an interesting idea. I might be +1 to this. But I think an even better solution is if the security system were able to throw a clear exception if no voters voted on an attribute. So if you did a typo (e.g. CREETE), you'd see a clear message, rather than nobody voting and access being granted/denied 100% of the time based on your voting strategy

This comment has been minimized.

Copy link
@jenkoian

jenkoian Jul 11, 2014

Contributor

So if all voters abstain, rather than denying access, you think an exception should be thrown? If I've understood correctly that'd be a change in the AccessDecisionManager, which I would say is outside of the scope of this..

This comment has been minimized.

Copy link
@stof

stof Jul 11, 2014

Member

that would be a BC break

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 13, 2014

Member

Yea, this is exactly what I would like, but Stof is right that it would be a BC break and I agree it's also outside of the scope of this :). But yes, in a perfect world, this seems like the right behavior to me.

@jenkoian

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2014

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

This comment has been minimized.

Copy link
Member

commented Aug 5, 2014

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2014

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

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

This comment has been minimized.

Copy link
@francoispluchino

francoispluchino Aug 5, 2014

Contributor

missing newline at end of file

@inoryy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2014

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2014

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

@wouterj

This comment has been minimized.

Copy link
Member

commented Aug 23, 2014

👍 great job @inoryy !

class ObjectFixture {}
class UnsupportedObjectFixture {}

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Aug 23, 2014

Member

(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

This comment has been minimized.

Copy link
Member

commented Aug 23, 2014

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

@weaverryan

This comment has been minimized.

Copy link
Member

commented Aug 23, 2014

👍 Awesome - one of my favorite features for 2.6!

* 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)

This comment has been minimized.

Copy link
@yguedidi

yguedidi Aug 23, 2014

Contributor

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 1, 2014

Member

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).

}
// as soon as at least one attribute is supported, default is to deny access
$vote = self::ACCESS_DENIED;

This comment has been minimized.

Copy link
@rybakit

rybakit Aug 23, 2014

Contributor

I would move this line below the next if check.

This comment has been minimized.

Copy link
@inoryy

inoryy Aug 24, 2014

Author Contributor

What's the benefit?

This comment has been minimized.

Copy link
@rybakit

rybakit Aug 24, 2014

Contributor

The benefit is to avoid needless assignment if the next if check evaluates to true.

This comment has been minimized.

Copy link
@inoryy

inoryy Aug 25, 2014

Author Contributor

I'd prefer increased readability over nano-optimization

This comment has been minimized.

Copy link
@rybakit

rybakit Aug 25, 2014

Contributor

It's a matter of taste. Personally, I don't see, how one would be more readable than the other.

This comment has been minimized.

Copy link
@inoryy

inoryy Sep 1, 2014

Author Contributor

@weaverryan or @javiereguiluz can you weight in here? I think our discussion with @rybakit has stalled due to personal preferences, so only way to move forward is via neutral position's opinion.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Sep 1, 2014

Member

Symfony project always prefer code readability over micro-optimizations, so maybe we should leave the original code as is.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 1, 2014

Member

FWIW, the flow here is identical to the existing RoleVoter. I think we all agree that the difference is minor if anything, so I think we should keep it as it is.

// as soon as at least one attribute is supported, default is to deny access
$vote = self::ACCESS_DENIED;
if ($this->isGranted($attribute, $object, $token->getUser())) {

This comment has been minimized.

Copy link
@rybakit

rybakit Aug 23, 2014

Contributor

Move $token->getUser() call out of the loop, as it was before?

This comment has been minimized.

Copy link
@inoryy

inoryy Aug 24, 2014

Author Contributor

What's the benefit of that?

This comment has been minimized.

Copy link
@rybakit

rybakit Aug 24, 2014

Contributor

As the result of $token->getUser() is not changed during the loop, I see no reason to call it on every iteration. Moreover it could be an expensive call depending on how one will implement this method.

This comment has been minimized.

Copy link
@rybakit

rybakit Aug 25, 2014

Contributor

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authentication/Token/TokenInterface.php#L53

Nothing prevents a user from creating a custom token, like:

FooToken implements TokenInterface
{
    ...

    public function getUser()
    {
        // do a remote call here 
    }
}

This comment has been minimized.

Copy link
@inoryy

inoryy Sep 1, 2014

Author Contributor

@weaverryan or @javiereguiluz can you weight in here? I think our discussion with @rybakit has stalled due to personal preferences, so only way to move forward is via neutral position's opinion.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Sep 1, 2014

Member

I'm really divided in this one :( Let's wait for @weaverryan to see if he has a preference for any of the two options.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 1, 2014

Member

I also don't have a preference, so let's leave it. The argument that @rybakit is making is valid. But someone else could make the argument that calling $token->getUser() before the if statement could in theory mean that you're calling this function even if you don't need the user (i.e. if supportsAttribute returns false for all attributes). Both are micro-optimizations, but you can't support both. Also, even though we're supporting it, you really shouldn't be calling the voter with an array of attributes - so that makes this even a little less common.

Thanks guys!

This comment has been minimized.

Copy link
@miklos-martin

miklos-martin Sep 23, 2014

I think you shouldn't call $token->getUser() at all. Passing the token itself would allow more flexibility for end users.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 23, 2014

Member

Most of the time you don't need the token, so we're catering for the most common use case (most developers don't really understand - or need to understand - what a token is). But you're certainly right, and if you need access to the token, you should just skip extending this AbstractVoter and implement the vote method on your own class with whatever logic you need.

if ($this->isGranted($attribute, $object, $token->getUser())) {
// grant access as soon as at least one voter returns a positive response
return self::ACCESS_GRANTED;

This comment has been minimized.

Copy link
@jenkoian

jenkoian Aug 25, 2014

Contributor

We are returning here as soon as we get a granted attribute. This will mean that any further attributes for this vote will be ignored. This could be an issue, if, for example, you have is_granted( ['VIEW', 'DELETE'], $obj) and the user has view permissions, but not delete permissions, as it will grant permission for both attributes.

May be better to negate the if and return as soon as a denied is returned.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 1, 2014

Member

I believe we had this conversation previously here: #11183 (diff)

The implementation here is consistent with how the core voters (e.g. RoleVoter) handles multiple attributes, which I think is the best we can do. Imo, people shouldn't pass an array of attributes, since it's not obvious how that will be handled. But if they do, we'll handle them in the same way as the core voters (and of course, you can always not use the Abstract class if you need to customize this detail).

This comment has been minimized.

Copy link
@jenkoian

jenkoian Sep 1, 2014

Contributor

people shouldn't pass an array of attributes

Yeah I was surprised it was supported when I looked at this before. It's quite little effort to make this support multiple attributes if we did want to, but yeah whatever really.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 1, 2014

Member

Haha, yea I was surprised too once upon a time :). The problem will always be whether you make the voter support "AND" logic or "OR" logic for the multiple attributes. We can't do both, but we have to do one.

This comment has been minimized.

Copy link
@stof

stof Sep 1, 2014

Member

all core voters are implementing OR

This comment has been minimized.

Copy link
@inoryy

inoryy Sep 1, 2014

Author Contributor

@weaverryan @stof so my implementation is good to go, right?

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 1, 2014

Member

Looks good to me as is: your code looks identical to RoleVoter.

* $user can be one of the following:
* a UserInterface object (fully authenticated user)
* a string (anonymously authenticated user)
* null (non-authenticated user)

This comment has been minimized.

Copy link
@stof

stof Aug 25, 2014

Member

I don't think it is actually possible to reach the vter without an authenticated token

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 1, 2014

Member

Yea, this would only be possible if there were no active firewall, right? In which case, calling isGranted on the SecurityContext would cause an exception before ever getting here. So I think Stof is right.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Sep 1, 2014

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2014

@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

This comment has been minimized.

Copy link
Member

commented Sep 1, 2014

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2014

@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

This comment has been minimized.

Copy link
Member

commented Sep 1, 2014

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

@inoryy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2014

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Sep 22, 2014

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

This comment has been minimized.

Copy link
Member

commented Sep 22, 2014

@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.

@inoryy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2014

@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

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

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

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2014

@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

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2014

@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

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

@inoryy Indeed.

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

Thank you @inoryy.

@fabpot fabpot closed this in 1fb8f88 Sep 23, 2014

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

This comment has been minimized.

Copy link
@ByScripts

ByScripts Dec 10, 2014

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(...)

This comment has been minimized.

Copy link
@weaverryan

weaverryan Dec 10, 2014

Member

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

weaverryan added a commit to symfony/symfony-docs that referenced this pull request Jun 30, 2015

feature #5423 [Security] add & update doc entries on AbstractVoter im…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.