bug #9485 [Acl] Fix for issue #9433 (guilro)

This PR was squashed before being merged into the 2.2 branch (closes #9485).

Discussion
----------

[Acl] Fix for issue #9433

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9433
| License       | MIT
| Doc PR        |

Two new test for issue #9433 :
`testUpdateAclInsertingMultipleObjectFieldAcesThrowsDBConstraintViolations()`
`testUpdateAclDeletingObjectFieldAcesThrowsDBConstraintViolations()`

The change to `updateAces()` line 857 is enough to make the first test succeed. When changing the `order` field value to a higher value, we must first change the value of the next entry (and all the next entries recursively) to preserve uniqueness of the `order` field in the database.

All the other changes are for the second test. In the former `updateAcl()` method, we commit the changes of the existing ACEs to the database before deleting or adding the new ones. We must delete the old ACEs before changing the existing ACEs in order to preserve uniqueness of the `order` field in the database.

Commits
-------

a38fab9 [Acl] Fix for issue #9433

2 files changed, 145 insertions, 26 deletions

diff --git a/Acl/Dbal/MutableAclProvider.php b/Acl/Dbal/MutableAclProvider.php
index 0ac4fa7..f24a580 100644
--- a/Acl/Dbal/MutableAclProvider.php
+++ b/Acl/Dbal/MutableAclProvider.php
@@ -252,6 +252,22 @@ class MutableAclProvider extends AclProvider implements MutableAclProviderInterf
                 }
             }
 
+            // check properties for deleted, and created ACEs, and perform deletions
+            // we need to perfom deletions before updating existing ACEs, in order to
+            // preserve uniqueness of the order field
+            if (isset($propertyChanges['classAces'])) {
+                $this->updateOldAceProperty('classAces', $propertyChanges['classAces']);
+            }
+            if (isset($propertyChanges['classFieldAces'])) {
+                $this->updateOldFieldAceProperty('classFieldAces', $propertyChanges['classFieldAces']);
+            }
+            if (isset($propertyChanges['objectAces'])) {
+                $this->updateOldAceProperty('objectAces', $propertyChanges['objectAces']);
+            }
+            if (isset($propertyChanges['objectFieldAces'])) {
+                $this->updateOldFieldAceProperty('objectFieldAces', $propertyChanges['objectFieldAces']);
+            }
+
             // this includes only updates of existing ACEs, but neither the creation, nor
             // the deletion of ACEs; these are tracked by changes to the ACL's respective
             // properties (classAces, classFieldAces, objectAces, objectFieldAces)
@@ -259,20 +275,20 @@ class MutableAclProvider extends AclProvider implements MutableAclProviderInterf
                 $this->updateAces($propertyChanges['aces']);
             }
 
-            // check properties for deleted, and created ACEs
+            // check properties for deleted, and created ACEs, and perform creations
             if (isset($propertyChanges['classAces'])) {
-                $this->updateAceProperty('classAces', $propertyChanges['classAces']);
+                $this->updateNewAceProperty('classAces', $propertyChanges['classAces']);
                 $sharedPropertyChanges['classAces'] = $propertyChanges['classAces'];
             }
             if (isset($propertyChanges['classFieldAces'])) {
-                $this->updateFieldAceProperty('classFieldAces', $propertyChanges['classFieldAces']);
+                $this->updateNewFieldAceProperty('classFieldAces', $propertyChanges['classFieldAces']);
                 $sharedPropertyChanges['classFieldAces'] = $propertyChanges['classFieldAces'];
             }
             if (isset($propertyChanges['objectAces'])) {
-                $this->updateAceProperty('objectAces', $propertyChanges['objectAces']);
+                $this->updateNewAceProperty('objectAces', $propertyChanges['objectAces']);
             }
             if (isset($propertyChanges['objectFieldAces'])) {
-                $this->updateFieldAceProperty('objectFieldAces', $propertyChanges['objectFieldAces']);
+                $this->updateNewFieldAceProperty('objectFieldAces', $propertyChanges['objectFieldAces']);
             }
 
             // if there have been changes to shared properties, we need to synchronize other
@@ -740,12 +756,12 @@ QUERY;
     }
 
     /**
-     * This processes changes on an ACE related property (classFieldAces, or objectFieldAces).
+     * This processes new entries changes on an ACE related property (classFieldAces, or objectFieldAces).
      *
      * @param string $name
      * @param array  $changes
      */
-    private function updateFieldAceProperty($name, array $changes)
+    private function updateNewFieldAceProperty($name, array $changes)
     {
         $sids = new \SplObjectStorage();
         $classIds = new \SplObjectStorage();
@@ -782,6 +798,26 @@ QUERY;
                 }
             }
         }
+    }
+
+    /**
+     * This process old entries changes on an ACE related property (classFieldAces, or objectFieldAces).
+     *
+     * @param string $name
+     * @param array $changes
+     */
+    private function updateOldFieldAceProperty($ane, array $changes)
+    {
+        $currentIds = array();
+        foreach ($changes[1] as $field => $new) {
+            for ($i=0,$c=count($new); $i<$c; $i++) {
+                $ace = $new[$i];
+
+                if (null != $ace->getId()) {
+                    $currentIds[$ace->getId()] = true;
+                }
+            }
+        }
 
         foreach ($changes[0] as $old) {
             for ($i=0,$c=count($old); $i<$c; $i++) {
@@ -796,12 +832,12 @@ QUERY;
     }
 
     /**
-     * This processes changes on an ACE related property (classAces, or objectAces).
+     * This processes new entries changes on an ACE related property (classAces, or objectAces).
      *
      * @param string $name
      * @param array  $changes
      */
-    private function updateAceProperty($name, array $changes)
+    private function updateNewAceProperty($name, array $changes)
     {
         list($old, $new) = $changes;
 
@@ -838,6 +874,26 @@ QUERY;
                 $currentIds[$ace->getId()] = true;
             }
         }
+    }
+
+    /**
+     * This processes old entries changes on an ACE related property (classAces, or objectAces).
+     *
+     * @param string $name
+     * @param array  $changes
+     */
+    private function updateOldAceProperty($name, array $changes)
+    {
+        list($old, $new) = $changes;
+        $currentIds = array();
+
+        for ($i=0,$c=count($new); $i<$c; $i++) {
+            $ace = $new[$i];
+
+            if (null != $ace->getId()) {
+                $currentIds[$ace->getId()] = true;
+            }
+        }
 
         for ($i=0,$c=count($old); $i<$c; $i++) {
             $ace = $old[$i];
@@ -857,26 +913,41 @@ QUERY;
     private function updateAces(\SplObjectStorage $aces)
     {
         foreach ($aces as $ace) {
-            $propertyChanges = $aces->offsetGet($ace);
-            $sets = array();
+            $this->updateAce($aces, $ace);
+        }
+    }
 
-            if (isset($propertyChanges['mask'])) {
-                $sets[] = sprintf('mask = %d', $propertyChanges['mask'][1]);
-            }
-            if (isset($propertyChanges['strategy'])) {
-                $sets[] = sprintf('granting_strategy = %s', $this->connection->quote($propertyChanges['strategy']));
-            }
-            if (isset($propertyChanges['aceOrder'])) {
-                $sets[] = sprintf('ace_order = %d', $propertyChanges['aceOrder'][1]);
-            }
-            if (isset($propertyChanges['auditSuccess'])) {
-                $sets[] = sprintf('audit_success = %s', $this->connection->getDatabasePlatform()->convertBooleans($propertyChanges['auditSuccess'][1]));
-            }
-            if (isset($propertyChanges['auditFailure'])) {
-                $sets[] = sprintf('audit_failure = %s', $this->connection->getDatabasePlatform()->convertBooleans($propertyChanges['auditFailure'][1]));
+    private function updateAce(\SplObjectStorage $aces, $ace)
+    {
+        $propertyChanges = $aces->offsetGet($ace);
+        $sets = array();
+
+        if (isset($propertyChanges['aceOrder'])
+            && $propertyChanges['aceOrder'][1] > $propertyChanges['aceOrder'][0]
+            && $propertyChanges == $aces->offsetGet($ace)) {
+                $aces->next();
+                if ($aces->valid()) {
+                    $this->updateAce($aces, $aces->current());
+                }
             }
 
-            $this->connection->executeQuery($this->getUpdateAccessControlEntrySql($ace->getId(), $sets));
+        if (isset($propertyChanges['mask'])) {
+            $sets[] = sprintf('mask = %d', $propertyChanges['mask'][1]);
+        }
+        if (isset($propertyChanges['strategy'])) {
+            $sets[] = sprintf('granting_strategy = %s', $this->connection->quote($propertyChanges['strategy']));
+        }
+        if (isset($propertyChanges['aceOrder'])) {
+            $sets[] = sprintf('ace_order = %d', $propertyChanges['aceOrder'][1]);
+        }
+        if (isset($propertyChanges['auditSuccess'])) {
+            $sets[] = sprintf('audit_success = %s', $this->connection->getDatabasePlatform()->convertBooleans($propertyChanges['auditSuccess'][1]));
         }
+        if (isset($propertyChanges['auditFailure'])) {
+            $sets[] = sprintf('audit_failure = %s', $this->connection->getDatabasePlatform()->convertBooleans($propertyChanges['auditFailure'][1]));
+        }
+
+        $this->connection->executeQuery($this->getUpdateAccessControlEntrySql($ace->getId(), $sets));
     }
+
 }
diff --git a/Tests/Acl/Dbal/MutableAclProviderTest.php b/Tests/Acl/Dbal/MutableAclProviderTest.php
index edcdd4d..00a2228 100644
--- a/Tests/Acl/Dbal/MutableAclProviderTest.php
+++ b/Tests/Acl/Dbal/MutableAclProviderTest.php
@@ -359,6 +359,54 @@ class MutableAclProviderTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals($newParentParentAcl->getId(), $reloadedAcl->getParentAcl()->getParentAcl()->getId());
     }
 
+    public function testUpdateAclInsertingMultipleObjectFieldAcesThrowsDBConstraintViolations()
+    {
+        $provider = $this->getProvider();
+        $oid = new ObjectIdentity(1, 'Foo');
+        $sid1 = new UserSecurityIdentity('johannes', 'FooClass');
+        $sid2 = new UserSecurityIdentity('guilro', 'FooClass');
+        $sid3 = new UserSecurityIdentity('bmaz', 'FooClass');
+        $fieldName = 'fieldName';
+
+        $acl = $provider->createAcl($oid);
+        $acl->insertObjectFieldAce($fieldName, $sid1, 4);
+        $provider->updateAcl($acl);
+
+        $acl = $provider->findAcl($oid);
+        $acl->insertObjectFieldAce($fieldName, $sid2, 4);
+        $provider->updateAcl($acl);
+
+        $acl = $provider->findAcl($oid);
+        $acl->insertObjectFieldAce($fieldName, $sid3, 4);
+        $provider->updateAcl($acl);
+    }
+
+    public function testUpdateAclDeletingObjectFieldAcesThrowsDBConstraintViolations()
+    {
+        $provider = $this->getProvider();
+        $oid = new ObjectIdentity(1, 'Foo');
+        $sid1 = new UserSecurityIdentity('johannes', 'FooClass');
+        $sid2 = new UserSecurityIdentity('guilro', 'FooClass');
+        $sid3 = new UserSecurityIdentity('bmaz', 'FooClass');
+        $fieldName = 'fieldName';
+
+        $acl = $provider->createAcl($oid);
+        $acl->insertObjectFieldAce($fieldName, $sid1, 4);
+        $provider->updateAcl($acl);
+
+        $acl = $provider->findAcl($oid);
+        $acl->insertObjectFieldAce($fieldName, $sid2, 4);
+        $provider->updateAcl($acl);
+
+        $index = 0;
+        $acl->deleteObjectFieldAce($index, $fieldName);
+        $provider->updateAcl($acl);
+
+        $acl = $provider->findAcl($oid);
+        $acl->insertObjectFieldAce($fieldName, $sid3, 4);
+        $provider->updateAcl($acl);
+    }
+
     /**
      * Data must have the following format:
      * array(