[Security/Acl] small voter refactoring

symfony · May 18, 2011 · 53f5c23 · 53f5c23
1 parent fa0ef50
commit 53f5c23
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 71 deletions.
diff --git a/src/Symfony/Component/Security/Acl/Permission/BasicPermissionMap.php b/src/Symfony/Component/Security/Acl/Permission/BasicPermissionMap.php
@@ -84,7 +84,7 @@ class BasicPermissionMap implements PermissionMapInterface
     /**
      * {@inheritDoc}
      */
-    public function getMasks($permission)
+    public function getMasks($permission, $object)
     {
         if (!isset($this->map[$permission])) {
             throw new \InvalidArgumentException(sprintf('The permission "%s" is not supported by this implementation.', $permission));

diff --git a/src/Symfony/Component/Security/Acl/Permission/PermissionMapInterface.php b/src/Symfony/Component/Security/Acl/Permission/PermissionMapInterface.php
@@ -25,9 +25,10 @@ interface PermissionMapInterface
      * these bitmasks.
      *
      * @param string $permission
-     * @return array
+     * @param object $object
+     * @return array may return null if permission/object combination is not supported
      */
-    function getMasks($permission);
+    function getMasks($permission, $object);
 
     /**
      * Whether this map contains the given permission

diff --git a/src/Symfony/Component/Security/Acl/Voter/AclVoter.php b/src/Symfony/Component/Security/Acl/Voter/AclVoter.php
@@ -57,58 +57,45 @@ public function supportsAttribute($attribute)
 
     public function vote(TokenInterface $token, $object, array $attributes)
     {
-        $firstCall = true;
         foreach ($attributes as $attribute) {
-            if (!$this->supportsAttribute($attribute)) {
+            if (null === $masks = $this->permissionMap->getMasks($attribute, $object)) {
                 continue;
             }
 
-            if ($firstCall) {
-                $firstCall = false;
-
-                if (null === $object) {
-                    if (null !== $this->logger) {
-                        $this->logger->debug(sprintf('Object identity unavailable. Voting to %s', $this->allowIfObjectIdentityUnavailable? 'grant access' : 'abstain'));
-                    }
-
-                    return $this->allowIfObjectIdentityUnavailable ? self::ACCESS_GRANTED : self::ACCESS_ABSTAIN;
-                } else if ($object instanceof FieldVote) {
-                    $field = $object->getField();
-                    $object = $object->getDomainObject();
-                } else {
-                    $field = null;
+            if (null === $object) {
+                if (null !== $this->logger) {
+                    $this->logger->debug(sprintf('Object identity unavailable. Voting to %s', $this->allowIfObjectIdentityUnavailable? 'grant access' : 'abstain'));
                 }
 
-                if ($object instanceof ObjectIdentityInterface) {
-                    $oid = $object;
-                } else if (null === $oid = $this->objectIdentityRetrievalStrategy->getObjectIdentity($object)) {
-                    if (null !== $this->logger) {
-                        $this->logger->debug(sprintf('Object identity unavailable. Voting to %s', $this->allowIfObjectIdentityUnavailable? 'grant access' : 'abstain'));
-                    }
+                return $this->allowIfObjectIdentityUnavailable ? self::ACCESS_GRANTED : self::ACCESS_ABSTAIN;
+            } else if ($object instanceof FieldVote) {
+                $field = $object->getField();
+                $object = $object->getDomainObject();
+            } else {
+                $field = null;
+            }
 
-                    return $this->allowIfObjectIdentityUnavailable ? self::ACCESS_GRANTED : self::ACCESS_ABSTAIN;
+            if ($object instanceof ObjectIdentityInterface) {
+                $oid = $object;
+            } else if (null === $oid = $this->objectIdentityRetrievalStrategy->getObjectIdentity($object)) {
+                if (null !== $this->logger) {
+                    $this->logger->debug(sprintf('Object identity unavailable. Voting to %s', $this->allowIfObjectIdentityUnavailable? 'grant access' : 'abstain'));
                 }
-                $sids = $this->securityIdentityRetrievalStrategy->getSecurityIdentities($token);
 
-                try {
-                    $acl = $this->aclProvider->findAcl($oid, $sids);
-                } catch (AclNotFoundException $noAcl) {
-                    if (null !== $this->logger) {
-                        $this->logger->debug('No ACL found for the object identity. Voting to deny access.');
-                    }
-
-                    return self::ACCESS_DENIED;
-                }
+                return $this->allowIfObjectIdentityUnavailable ? self::ACCESS_GRANTED : self::ACCESS_ABSTAIN;
             }
+            $sids = $this->securityIdentityRetrievalStrategy->getSecurityIdentities($token);
 
             try {
-                if (null === $field && $acl->isGranted($this->permissionMap->getMasks($attribute), $sids, false)) {
+                $acl = $this->aclProvider->findAcl($oid, $sids);
+
+                if (null === $field && $acl->isGranted($masks, $sids, false)) {
                     if (null !== $this->logger) {
                         $this->logger->debug('ACL found, permission granted. Voting to grant access');
                     }
 
                     return self::ACCESS_GRANTED;
-                } else if (null !== $field && $acl->isFieldGranted($field, $this->permissionMap->getMasks($attribute), $sids, false)) {
+                } else if (null !== $field && $acl->isFieldGranted($field, $masks, $sids, false)) {
                     if (null !== $this->logger) {
                         $this->logger->debug('ACL found, permission granted. Voting to grant access');
                     }
@@ -120,6 +107,12 @@ public function vote(TokenInterface $token, $object, array $attributes)
                     $this->logger->debug('ACL found, insufficient permissions. Voting to deny access.');
                 }
 
+                return self::ACCESS_DENIED;
+            } catch (AclNotFoundException $noAcl) {
+                if (null !== $this->logger) {
+                    $this->logger->debug('No ACL found for the object identity. Voting to deny access.');
+                }
+
                 return self::ACCESS_DENIED;
             } catch (NoAceFoundException $noAce) {
                 if (null !== $this->logger) {

diff --git a/tests/Symfony/Tests/Component/Security/Acl/Voter/AclVoterTest.php b/tests/Symfony/Tests/Component/Security/Acl/Voter/AclVoterTest.php
@@ -71,8 +71,8 @@ public function testVote()
         list($voter,, $permissionMap,,) = $this->getVoter();
         $permissionMap
             ->expects($this->atLeastOnce())
-            ->method('contains')
-            ->will($this->returnValue(false))
+            ->method('getMasks')
+            ->will($this->returnValue(null))
         ;
 
         $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $voter->vote($this->getToken(), null, array('VIEW', 'EDIT', 'DELETE')));
@@ -86,8 +86,8 @@ public function testVoteWhenNoObjectIsPassed($allowIfObjectIdentityUnavailable)
         list($voter,, $permissionMap,,) = $this->getVoter($allowIfObjectIdentityUnavailable);
         $permissionMap
             ->expects($this->once())
-            ->method('contains')
-            ->will($this->returnValue(true))
+            ->method('getMasks')
+            ->will($this->returnValue(array()))
         ;
 
         if ($allowIfObjectIdentityUnavailable) {
@@ -107,8 +107,8 @@ public function testVoteWhenOidStrategyReturnsNull($allowIfUnavailable)
         list($voter,, $permissionMap, $oidStrategy,) = $this->getVoter($allowIfUnavailable);
         $permissionMap
             ->expects($this->once())
-            ->method('contains')
-            ->will($this->returnValue(true))
+            ->method('getMasks')
+            ->will($this->returnValue(array()))
         ;
 
         $oidStrategy
@@ -137,8 +137,8 @@ public function testVoteNoAclFound()
 
         $permissionMap
             ->expects($this->once())
-            ->method('contains')
-            ->will($this->returnValue(true))
+            ->method('getMasks')
+            ->will($this->returnValue(array()))
         ;
 
         $oidStrategy
@@ -170,11 +170,6 @@ public function testVoteGrantsAccess($grant)
     {
         list($voter, $provider, $permissionMap, $oidStrategy, $sidStrategy) = $this->getVoter();
 
-        $permissionMap
-            ->expects($this->once())
-            ->method('contains')
-            ->will($this->returnValue(true))
-        ;
         $permissionMap
             ->expects($this->once())
             ->method('getMasks')
@@ -221,11 +216,6 @@ public function testVoteNoAceFound()
     {
         list($voter, $provider, $permissionMap, $oidStrategy, $sidStrategy) = $this->getVoter();
 
-        $permissionMap
-            ->expects($this->once())
-            ->method('contains')
-            ->will($this->returnValue(true))
-        ;
         $permissionMap
             ->expects($this->once())
             ->method('getMasks')
@@ -269,11 +259,6 @@ public function testVoteGrantsFieldAccess($grant)
     {
         list($voter, $provider, $permissionMap, $oidStrategy, $sidStrategy) = $this->getVoter();
 
-        $permissionMap
-            ->expects($this->once())
-            ->method('contains')
-            ->will($this->returnValue(true))
-        ;
         $permissionMap
             ->expects($this->once())
             ->method('getMasks')
@@ -320,11 +305,6 @@ public function testVoteNoFieldAceFound()
     {
         list($voter, $provider, $permissionMap, $oidStrategy, $sidStrategy) = $this->getVoter();
 
-        $permissionMap
-            ->expects($this->once())
-            ->method('contains')
-            ->will($this->returnValue(true))
-        ;
         $permissionMap
             ->expects($this->once())
             ->method('getMasks')
@@ -367,11 +347,6 @@ public function testWhenReceivingAnObjectIdentityInterfaceWeDontRetrieveANewObje
 
         $oid = new ObjectIdentity('someID','someType');
 
-        $permissionMap
-            ->expects($this->once())
-            ->method('contains')
-            ->will($this->returnValue(true))
-        ;
         $permissionMap
             ->expects($this->once())
             ->method('getMasks')