8000 [Security] add an AbstractVoter implementation by inoryy · Pull Request #11183 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Security] add an AbstractVoter implementation #11183

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

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
add abstract voter implementation, reducing boilerplate required for …
…custom voter
  • Loading branch information
inoryy committed Sep 23, 2014
commit 4a7bfe2e786335b9546ace72e4396d3785b3da1b
9E1E
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

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

use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;

/**
* Abstract Voter implementation that reduces boilerplate code required to create a custom Voter
*
* @author Roman Marintšenko <inoryy@gmail.com>
*/
abstract class AbstractVoter implements VoterInterface
{
/**
* {@inheritdoc}
*/
public function supportsAttribute($attribute)
{
return in_array($attribute, $this->getSupportedAttributes());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be a BC break

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
* {@inheritdoc}
*/
public function supportsClass($class)
{
foreach ($this->getSupportedClasses() as $supportedClass) {
if ($supportedClass === $class || is_subclass_of($class, $supportedClass)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supportsClass expects a string in the VoterInterface

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point ;)

return true;
}
}

return false;
}

/**
* Iteratively check all given attributes by calling voteOnAttribute
* This method terminates as soon as it is able to return either ACCESS_GRANTED or ACCESS_DENIED vote
* Otherwise it will return ACCESS_ABSTAIN
*
* @param TokenInterface $token A TokenInterface instance
* @param object $object The object to secure
* @param array $attributes An array of attributes associated with the method being invoked
*
* @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
*/
public function vote(TokenInterface $token, $object, array $attributes)
{
if (!$this->supportsClass(get_class($object))) {
return VoterInterface::ACCESS_ABSTAIN;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


$user = $token->getUser();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point! will try to replicate core voters behavior

return $vote;
}
}
}

return VoterInterface::ACCESS_ABSTAIN;
}

/**
* Return an array of supported classes. This will be called by supportsClass
*
* @return array an array of supported classes, i.e. ['\Acme\DemoBundle\Model\Product']
*/
abstract protected function getSupportedClasses();

/**
* Return an array of supported attributes. This will be called by supportsAttribute
*
* @return array an array of supported attributes, i.e. ['CREATE', 'READ']
*/
abstract protected function getSupportedAttributes();

/**
* Perform a single vote operation on a given attribute, object and (optionally) user
* It is safe to assume that $attribute and $object's class pass supportsAttribute/supportsClass
*
* @param string $attribute
* @param object $object
* @param UserInterface $user
*
* @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
*/
abstract protected function voteOnAttribute($attribute, $object, UserInterface $user = null);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline at end of file

0