Skip to content
Closed
Prev Previous commit
Next Next commit
improvements according to the reviews
  • Loading branch information
Michael Klein committed Nov 5, 2013
commit b1923b709a901c1f8ff643e9bcd32b69e40823a9
139 changes: 96 additions & 43 deletions cookbook/security/voters_data_permission.rst
Original file line number Diff line number Diff line change
@@ -1,28 +1,29 @@
.. index::
Copy link
Member

Choose a reason for hiding this comment

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

This new document needs to be referenced in /cookbook/map.rst and /cookbook/security/index.rst.

Copy link
Author

Choose a reason for hiding this comment

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

I added them right under the voter article.

single: Security; Data Permission Voters

How to implement your own Voter to check user permissions for accessing a given object
How to implement your own Voter to check User Permissions for accessing a Given Object
Copy link
Member

Choose a reason for hiding this comment

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

How To Use Voters to Check User Permissions

======================================================================================

In Symfony2 you can check the permission to access data by the
:doc:`ACL module </cookbook/security/acl>`, which is a bit overwhelming
for many applications. A much easier solution is to work with custom voters
voters, which are like simple conditional statements. Voters can be
also used to check for permission as a part or even the whole
In Symfony2 you can check the permission to access data by the
Copy link
Contributor

Choose a reason for hiding this comment

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

by -> using or via

:doc:`ACL module </cookbook/security/acl>`, which is a bit overwhelming
for many applications. A much easier solution is to work with custom voters,
which are like simple conditional statements. Voters can be
also be used to check for permission as a part or even the whole
Copy link
Member

Choose a reason for hiding this comment

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

remove the first be: Voters can also be used [...]

You can then move also and be to the line above I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

[...] for permission to of the application or even [...]

application: :doc:`"/cookbook/security/voters"`.
Copy link
Member

Choose a reason for hiding this comment

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

The most important thing here is to give explain to them what we're going to do in this chapter.

Sometimes, you need to check more than simply if the user has a role or not. For example, suppose
you have a Post entity object, and you need to check to see if the current user can edit that object.
Perhaps your logic is as simple as checking to see that the Post.author property is equal to the
given user, or maybe it's more complicated and you'll check several conditions. This type of access
information can be handled by Symfony's ACL module, but it's quite complex. If you already have all
the data you need to check for access (e.g. checking to see if the current user is the Post author),
then using Voters is a much simpler solution. In this entry, we'll show you how to do exactly that.

Copy link
Member

Choose a reason for hiding this comment

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

You may even show really early on, how this is all going to look inside a controller. The usage in a controller is one thing that's missing in this entry.

Copy link
Author

Choose a reason for hiding this comment

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

good point, I am adding this


.. tip::

It is good to understand the basics about what and how
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it good? need to connect it

:doc:`authorization </components/security/authorization>` works.
:doc:`authorization </components/security/authorization>` works. // correct link in book?

How Symfony Uses Voters
How Symfony uses Voters
Copy link
Member

Choose a reason for hiding this comment

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

How Symfony Uses Voters

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 @weaverryan suggested not to capitalise verbs.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is a standard in the documentation now

-----------------------

In order to use voters, you have to understand how Symfony works with them.
In general, all registered custom voters will be called every time you ask
Symfony about permissions (ACL). In general there are three different
approaches on how to handle the feedback from all voters:
In general, all registered custom voters will be called every time you ask
Symfony about permissions (ACL). You can use one of three different
approaches on how to handle the feedback from all voters: affirmative,
consensus and unanimous. For more information have a look at
:ref:`"components-security-access-decision-manager"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

at least list them and reiterate the one item you are going to treat or how is it going to connect with what follows here

Copy link
Member

Choose a reason for hiding this comment

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

put quotes outside fo the :ref: role:

":ref:`components-security-access-decision-manager`"

The Voter Interface
Expand All @@ -32,13 +33,13 @@ A custom voter must implement
:class:`Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface`,
which has this structure:

.. code-block:: php
.. code-block:: php // :: shortcut? and put the snippet (to line 56) in a single file an reference ?

interface VoterInterface
{
public function supportsAttribute($attribute);
public function supportsClass($class);
public function vote(TokenInterface $token, $object, array $attributes);
public function vote(TokenInterface $token, $post, array $attributes);
}

The ``supportsAttribute()`` method is used to check if the voter supports
Copy link
Member

Choose a reason for hiding this comment

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

use :method: role

Copy link
Author

Choose a reason for hiding this comment

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

sorry I dont get it

Copy link
Member

Choose a reason for hiding this comment

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

Use:

The :method:`Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::supportsAttribute` method is used to check if the voter supports
Expand All @@ -60,7 +61,7 @@ object according to your custom conditions (e.g. he must be the owner of
the object). If the condition fails, you'll return
``VoterInterface::ACCESS_DENIED``, otherwise you'll return
Copy link
Member

Choose a reason for hiding this comment

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

you -> it?

``VoterInterface::ACCESS_GRANTED``. In case the responsibility for this decision
belongs not to this voter, it will return ``VoterInterface::ACCESS_ABSTAIN``.
does not belong to this voter, it will return ``VoterInterface::ACCESS_ABSTAIN``.

Creating the Custom Voter
-------------------------
Expand All @@ -72,81 +73,86 @@ You could store your Voter to check permission for the view and edit action like
// src/Acme/DemoBundle/Security/Authorization/Entity/PostVoter.php
namespace Acme\DemoBundle\Security\Authorization\Entity;
Copy link
Member

Choose a reason for hiding this comment

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

The voter should not live in the Entity namespace. Use the Acme\DemoBundle\Security\Authorization\Voter namespace instead.


Copy link
Member

Choose a reason for hiding this comment

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

use Acme\DemoBundle\Entity\Post

(see below)

use Symfony\Component\HttpKernel\Exception\PreconditionFailedHttpException;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\User\UserInterface;
use Doctrine\Common\Util\ClassUtils;

class PostVoter implements VoterInterface
{

public function supportsAttribute($attribute)
public function supportsAttribute($attribute)
{
return in_array($attribute, array(
Copy link
Member

Choose a reason for hiding this comment

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

I would have used constants like in built-in voters.

Copy link
Author

Choose a reason for hiding this comment

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

would you also want to use the constant within the controller to check for?

e.g.

if (false === $this->get('security.context')->isGranted(PostVoter::VIEW, $post)) { throw new AccessDeniedException('Unauthorised access!'); }
'view',
'edit',
));
}
public function supportsClass($class)

public function supportsClass($obj)
{
// could be "Acme\DemoBundle\Entity\Post" as well
$array = array("Acme\DemoBundle\Entity\Post");

$array = array('Acme\DemoBundle\Entity\Post');
Copy link
Member

Choose a reason for hiding this comment

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

I would simplify this method by just checking instanceof on the class. This is an example, not a generic class (Symfony developers will probably figure out by themselves how to support more than one class).


foreach ($array as $item) {
// check with stripos in case doctrine is using a proxy class for this object
Copy link
Member

Choose a reason for hiding this comment

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

This is not the right way to check for Doctrine proxies (you can have lots of false positive with your stripos detection).

  • You should be using Doctrine\Common\Util\ClassUtils::getRealClass($class) to resolve proxies (available since Common 2.2, so it is usable in all Symfony 2.1+ projects)
  • Your voter should support all child classes of Post according to the rules of OOP, not only the exact match on the class
Copy link
Author

Choose a reason for hiding this comment

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

Ah cool, I didn't know that. I will change that to be more proper.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think there is even a util in doctrine common for this, but i may be wrong

Copy link
Author

Choose a reason for hiding this comment

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

yes this will be replaced by a better solution, please wait till tomorrow

if (stripos($s, $item) !== false) {

// if (stripos($s, $item) !== false) {
if ($obj instanceof $item)) // check if this will also check for interfaces etc. like it should be in oop (inheritace)
// or return $targetClass === $class || is_subclass_of($class, $targetClass);
return true;
}
}

return false;
Copy link
Member

Choose a reason for hiding this comment

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

add new line before return statement

Copy link
Member

Choose a reason for hiding this comment

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

why not simplify this to?

return $obj instanceof Post;
Copy link
Author

Choose a reason for hiding this comment

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

good point

}

public function vote(TokenInterface $token, $object, array $attributes)

/** @var \Acme\DemoBundle\Entity\Post $post */
Copy link
Member

Choose a reason for hiding this comment

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

please use multiple lines for this

public function vote(TokenInterface $token, $post, array $attributes) // remove array
{
// always get the first attribute
$attribute = $attributes[0];

// get current logged in user
$user = $token->getUser();

// check if class of this object is supported by this voter
if (!($this->supportsClass(get_class($object)))) {
if (!($this->supportsClass($post))) { // maybe without ClassUtils::getRealClass(

Copy link
Member

Choose a reason for hiding this comment

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

remove this line

Copy link
Author

Choose a reason for hiding this comment

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

according to PSR-1 and PSR-2 (checked by http://cs.sensiolabs.org/) you need to have an empty line before each return, except there is no other code in the current method.

Copy link
Author

Choose a reason for hiding this comment

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

At least it is correcting this way as I saw...

Copy link
Member

Choose a reason for hiding this comment

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

It's not in the PSR, it's in the Symfony coding standards. The CS fixer is a bit buggy on this, from the standards page:

Add a blank line before return statements, unless the return is alone inside a statement-group (like an if statement);

In this case, we are talking about a return statement being alone inside a statement-group (like an if statement)

Copy link
Member

Choose a reason for hiding this comment

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

PSR-1 and 2 don't require anything like this.

The Symfony coding standard require a newline before the return statement, except when it is the only statement in its block, not in the method (a block being the body of a method or of a conditional structure)

return VoterInterface::ACCESS_ABSTAIN;
}

// check if the given attribute is covered by this voter
foreach ($attributes as $attribute) {
if (!$this->supportsAttribute($attribute)) {
if (!$this->supportsAttribute($attribute)) {

Copy link
Member

Choose a reason for hiding this comment

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

remove this line

return VoterInterface::ACCESS_ABSTAIN;
}
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.

this implementation looks weird to me: you are abstaining even if some of the attributes are supported. None of the core voters are behaving this way (most of the time, you are checking a single attribute so it is not a common case though)

Copy link
Author

Choose a reason for hiding this comment

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

You are right, I am going to allow one Attribute only.


// check if given user is instance of user interface
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment?

if (!($user instanceof UserInterface)) {
Copy link
Member

Choose a reason for hiding this comment

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

remove () around $user instanceof UserInterface


Copy link
Member

Choose a reason for hiding this comment

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

remove this line

return VoterInterface::ACCESS_DENIED;
}
switch($this->attributes[0]) {

switch($attribute) {
case 'view':
if ($object->isPrivate() === false) {
// the data object could have for e.g. a method isPrivate() which checks the the boolean attribute $private
Copy link
Member

Choose a reason for hiding this comment

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

line break the comment

Copy link
Contributor

Choose a reason for hiding this comment

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

remove the the

Copy link
Contributor

Choose a reason for hiding this comment

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

Boolean

if (!$post->isPrivate()) {

Copy link
Member

Choose a reason for hiding this comment

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

remove this line

return VoterInterface::ACCESS_GRANTED;
}
break;

case 'edit':
if ($user->getId() === $object->getOwner()->getId()) {
// we assume that our data object has a method getOwner() to get the current owner user entity for this data object
Copy link
Member

Choose a reason for hiding this comment

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

line break this comment

if ($user->getId() === $post->getOwner()->getId()) {

Copy link
Member

Choose a reason for hiding this comment

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

remove this line

return VoterInterface::ACCESS_GRANTED;
}
break;

default:
Copy link
Contributor

Choose a reason for hiding this comment

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

This case can be remove because we are already out with !$this->supportsAttribute($attribute) from https://github.com/symfony/symfony-docs/pull/3138/files#diff-70c7f0c16e0c51e7dc991ab9b801ff73R107

// otherwise denied access
return VoterInterface::ACCESS_DENIED;
// otherwise throw an exception
throw new PreconditionFailedHttpException('The Attribute "'.$attribute.'"" was not found.')
}

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line break and you did not even use the container 👎

}
Expand All @@ -170,6 +176,53 @@ and tag it as a "security.voter":
security.access.post_voter:
class: Acme\DemoBundle\Security\Authorization\Entity\PostVoter
Copy link
Member

Choose a reason for hiding this comment

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

should be Acme\DemoBundle\Security\Authorization\Voter\PostVoter

public: false
# the service gets tagged as a voter
tags:
- { name: security.voter }
- { name: security.voter }

.. code-block:: xml

<?xml version="1.0" encoding="UTF-8" ?>
Copy link
Member

Choose a reason for hiding this comment

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

add <!-- src/Acme/DemoBundle/Resources/config/services.xml --> above this line

<container xmlns="http://symfony.com/schema/dic/services">
Copy link
Member

Choose a reason for hiding this comment

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

The XSD location is missing. It should look like this:

<container xmlns="http://symfony.com/schema/dic/services" xsi:schemaLocation="http://symfony.com/schema/dic/services  http://symfony.com/schema/dic/services/services-1.0.xsd" >
<services>
<service id="security.access.post_document_voter"
class="Acme\DemoBundle\Security\Authorization\Document\PostVoter"
Copy link
Member

Choose a reason for hiding this comment

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

should be Acme\DemoBundle\Security\Authorization\Voter\PostVoter

public="false">
<tag name="security.voter" />
</service>
</services>
</container>

.. code-block:: php

$container
Copy link
Member

Choose a reason for hiding this comment

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

add // src/Acme/DemoBundle/Resources/config/services.php above this line

->register('security.access.post_document_voter', 'Acme\DemoBundle\Security\Authorization\Document\PostVoter')
Copy link
Member

Choose a reason for hiding this comment

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

line breaking too

->addTag('security.voter')
;

How to use the Voter in a Controller
Copy link
Member

Choose a reason for hiding this comment

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

How to Use the Voter in a Controller

------------------------------------

Copy link
Member

Choose a reason for hiding this comment

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

missing some text

.. code-block:: php

// src/Acme/DemoBundle/Controller/PostController.php
namespace Acme\DemoBundle\Controller;

use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;

class PostController
Copy link
Member

Choose a reason for hiding this comment

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

you have to extend the Controller class from the FrameworkBundle. Otherwise, you could not use this->get(...) to retrieve a service from the container.

{
public function showAction($id)
{
// keep in mind, this will call all registered security voters
if (false === $this->get('security.context')->isGranted('view')) {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the second argument be the $post?

throw new AccessDeniedException('Unauthorised access!');
}

$product = $this->getDoctrine()
->getRepository('AcmeStoreBundle:Post')
->find($id);
Copy link
Contributor

Choose a reason for hiding this comment

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

; goes in the next line

Copy link
Member

Choose a reason for hiding this comment

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

-1

find() is always the last method call here, as it returns an array.

Copy link

Choose a reason for hiding this comment

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

This should be moved just before the permissions check. Also, $post instead of $product.


return new Response('<html><body>Headline for Post: '.$post->getName().'</body></html>');
Copy link
Member

Choose a reason for hiding this comment

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

To keep it simple, I propose to remove the HTML from this

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah no need for distractions

Copy link
Member

Choose a reason for hiding this comment

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

Actually, use '<h1>'.$post->getName().'</h1>'

}
}