Merge pull request #64 from SURFnet/34-prevent-false-positives-when-s…

…yncing 34 Prevent groups from being marked for update on every sync by only comparing properties that exist in LDAP and thus can be synced.
SURFnet · Dec 7, 2016 · 25667b0 · 25667b0
2 parents 751a96c + 504e554
commit 25667b0
Show file tree

Hide file tree

Showing 9 changed files with 133 additions and 27 deletions.
diff --git a/app/config/services.yml b/app/config/services.yml
@@ -23,11 +23,12 @@ services:
 
     app.grouphub_ldap_client:
         class: AppBundle\Ldap\GrouphubClient
-        arguments: ["@app.ldap_read_client", "@app.ldap_write_client", "@app.ldap_fallback_clients", "@app.ldap_normalizer", "%users_dn%", "%groups_dn%", "%grouphub_dn%", "%formal_dn%", "%adhoc_dn%", "%admin_groups_dn%", "%user_query%", "%group_query%"]
+        arguments: ["@app.ldap_read_client", "@app.ldap_write_client", "@app.ldap_fallback_clients", "@app.ldap_normalizer", "%users_dn%", "%groups_dn%", "%grouphub_dn%", "%formal_dn%", "%adhoc_dn%", "%admin_groups_dn%", "%user_query%", "%group_query%", "%ldap.mapping%"]
         private: true
 
     app.api_normalizer:
         class: AppBundle\Api\Normalizer
+        arguments: ["%ldap.mapping%"]
         private: true
 
     app.api_client:

diff --git a/src/AppBundle/Api/Normalizer.php b/src/AppBundle/Api/Normalizer.php
@@ -17,6 +17,17 @@
  */
 class Normalizer
 {
+    /**
+     * @var array
+     */
+    private $mapping;
+
+    public function __construct(array $mapping)
+    {
+        $this->mapping = $mapping;
+    }
+
+
     /**
      * @param User $user
      *
@@ -133,7 +144,7 @@ public function denormalizeGroups(array $groups)
             $result[] = $this->denormalizeGroup($group);
         }
 
-        return new Collection($result, $groups['count']);
+        return new Collection($result, $groups['count'], $this->mapping['group']);
     }
 
     /**
@@ -146,7 +157,7 @@ public function denormalizeGroup(array $group)
         return new Group(
             $group['id'],
             isset($group['reference']) ? $group['reference'] : '',
-            isset($group['name']) ? $group['name'] : '',
+            isset($group['name']) ? sprintf('%s_%s', $group['name'], $group['id']) : '',
             isset($group['description']) ? $group['description'] : '',
             isset($group['type']) ? $group['type'] : '',
             isset($group['owner']) ? $this->denormalizeUser($group['owner']) : null,

diff --git a/src/AppBundle/Ldap/GrouphubClient.php b/src/AppBundle/Ldap/GrouphubClient.php
@@ -77,6 +77,11 @@ class GrouphubClient
      */
     private $groupQuery;
 
+    /**
+     * @var array
+     */
+    private $mapping;
+
     /**
      * @param LdapClient  $readLdap
      * @param LdapClient  $writeLdap
@@ -90,6 +95,7 @@ class GrouphubClient
      * @param string      $adminGroupsDn
      * @param string      $userQuery
      * @param string      $groupQuery
+     * @param array $mapping
      */
     public function __construct(
         LdapClient $readLdap,
@@ -103,7 +109,8 @@ public function __construct(
         $adhocDn,
         $adminGroupsDn = '',
         $userQuery = 'cn=*',
-        $groupQuery = 'cn=*'
+        $groupQuery = 'cn=*',
+        array $mapping = []
     ) {
         $this->readLdap = $readLdap;
         $this->writeLdap = $writeLdap;
@@ -119,6 +126,7 @@ public function __construct(
         $this->adminGroupsDn = $adminGroupsDn;
         $this->userQuery = $userQuery;
         $this->groupQuery = $groupQuery;
+        $this->mapping = $mapping;
     }
 
     /**
@@ -210,7 +218,9 @@ function (Comparable $a, Comparable $b) {
 
         $entities = array_slice($entities, $offset, $limit);
 
-        return new SynchronizableSequence($entities);
+        $mapping = reset($entities) instanceof Group ? $this->mapping['group'] : [];
+
+        return new SynchronizableSequence($entities, $mapping);
     }
 
     /**

diff --git a/src/AppBundle/Ldap/Normalizer.php b/src/AppBundle/Ldap/Normalizer.php
@@ -182,11 +182,9 @@ public function normalizeGroupForUpdate(Group $group)
     {
         $mapping = $this->mapping['group'];
 
-        $data = array_filter(
-            [
-                $mapping['description'] => $group->getDescription(),
-            ]
-        );
+        if (!empty($mapping['description'])) {
+            $data[$mapping['description']] = $group->getDescription();
+        }
 
         if (!empty($mapping['owner'])) {
             $data[$mapping['owner']] = $group->getOwner()->getReference();

diff --git a/src/AppBundle/Model/Collection.php b/src/AppBundle/Model/Collection.php
@@ -16,11 +16,12 @@ class Collection extends SynchronizableSequence
 
     /**
      * @param array $elements
-     * @param int   $totalCount
+     * @param int $totalCount
+     * @param array $mapping
      */
-    public function __construct(array $elements = [], $totalCount = 0)
+    public function __construct(array $elements = [], $totalCount = 0, array $mapping = [])
     {
-        parent::__construct($elements);
+        parent::__construct($elements, $mapping);
 
         $this->totalCount = $totalCount;
     }

diff --git a/src/AppBundle/Model/Group.php b/src/AppBundle/Model/Group.php
@@ -228,19 +228,20 @@ public function compareTo($other)
     /**
      * @param Group $other
      *
+     * @param array $mapping
      * @return bool
      */
-    public function equals($other)
+    public function equals($other, array $mapping)
     {
         if ($this->compareTo($other) !== 0) {
             return false;
         }
 
-        if ($other->getName() !== $this->name) {
+        if (isset($mapping['name']) && $other->getName() !== $this->name) {
             return false;
         }
 
-        if ($other->getDescription() !== $this->description) {
+        if (isset($mapping['description']) && $other->getDescription() !== $this->description) {
             return false;
         }
 

diff --git a/src/AppBundle/SynchronizableSequence.php b/src/AppBundle/SynchronizableSequence.php
@@ -31,6 +31,16 @@ class SynchronizableSequence extends Sequence
      * @var array
      */
     private $equalElements = [];
+    /**
+     * @var array
+     */
+    private $mapping;
+
+    public function __construct(array $elements, array $mapping)
+    {
+        parent::__construct($elements);
+        $this->mapping = $mapping;
+    }
 
     /**
      * @param Traversable $sourceSequence
@@ -161,7 +171,7 @@ private function elementsAreEqual($sourceElement, $destinationElement)
                 throw new RuntimeException('Elements are not the same');
             }
 
-            return $sourceElement->equals($destinationElement);
+            return $sourceElement->equals($destinationElement, $this->mapping);
         }
 
         return $sourceElement == $destinationElement;

diff --git a/tests/AppBundle/Model/GroupTest.php b/tests/AppBundle/Model/GroupTest.php
@@ -0,0 +1,74 @@
+<?php
+
+namespace Tests\AppBundle\Model;
+
+
+use AppBundle\Model\Group;
+use PHPUnit_Framework_TestCase;
+
+class GroupTest extends PHPUnit_Framework_TestCase
+{
+    private $defaultMapping = [
+        'name' => 'name',
+        'description' => 'description'
+    ];
+
+    /**
+     * @test
+     */
+    public function shouldNotBeConsideredEqualToGroupWithDifferentCn()
+    {
+        $fooGroup = new Group(1, 'cn=foo,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'Description');
+        $barGroup = new Group(1, 'cn=bar,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'Description');
+        $this->assertFalse($fooGroup->equals($barGroup, $this->defaultMapping));
+    }
+
+    /**
+     * @test
+     */
+    public function shouldNotBeConsideredEqualToGroupWithDifferentName()
+    {
+        $fooGroup = new Group(1, 'cn=bar,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'Description');
+        $barGroup = new Group(1, 'cn=bar,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'OtherName', 'Description');
+        $this->assertFalse($fooGroup->equals($barGroup, $this->defaultMapping));
+    }
+
+    /**
+     * @test
+     */
+    public function shouldNotBeConsideredEqualToGroupWithDifferentDescription()
+    {
+        $fooGroup = new Group(1, 'cn=bar,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'Description');
+        $barGroup = new Group(1, 'cn=bar,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'OtherDescription');
+        $this->assertFalse($fooGroup->equals($barGroup, $this->defaultMapping));
+    }
+
+    /**
+     * @test
+     */
+    public function shouldBeConsideredEqualToGroupWithDifferentNameWhenNameIsNotMapped()
+    {
+        $fooGroup = new Group(1, 'cn=foo,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'Description');
+        $barGroup = new Group(1, 'cn=foo,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'OtherName', 'Description');
+        $this->assertTrue($fooGroup->equals($barGroup, array_merge($this->defaultMapping, ['name' => null])));
+    }
+    /**
+     * @test
+     */
+    public function shouldBeConsideredEqualToGroupWithDifferentDescriptionWhenDescriptionIsNotMapped()
+    {
+        $fooGroup = new Group(1, 'cn=foo,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'Description');
+        $barGroup = new Group(1, 'cn=foo,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'OtherDescription');
+        $this->assertTrue($fooGroup->equals($barGroup, array_merge($this->defaultMapping, ['description' => null])));
+    }
+
+    /**
+     * @test
+     */
+    public function shouldBeConsideredEqualToGroup()
+    {
+        $fooGroup = new Group(1, 'cn=foo,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'Description');
+        $barGroup = new Group(1, 'cn=foo,ou=AdHoc,ou=Grouphub,dc=surfuni,dc=org', 'Name', 'Description');
+        $this->assertTrue($fooGroup->equals($barGroup, $this->defaultMapping));
+    }
+}
diff --git a/tests/AppBundle/SynchronizableSequenceTest.php b/tests/AppBundle/SynchronizableSequenceTest.php
@@ -10,7 +10,7 @@ class SynchronizableSequenceTest extends \PHPUnit_Framework_TestCase
     public function testSyncForEqualSequences()
     {
         $seq1 = new Sequence([1, 2, 3, 4, 5]);
-        $seq2 = new SynchronizableSequence([1, 2, 3, 4, 5]);
+        $seq2 = new SynchronizableSequence([1, 2, 3, 4, 5], []);
 
         $lastIndex = $seq2->synchronize($seq1);
 
@@ -26,7 +26,7 @@ public function testSyncForEqualSequences()
     public function testSyncForRemovedElement()
     {
         $seq1 = new Sequence([1, 3, 4, 5]);
-        $seq2 = new SynchronizableSequence([1, 2, 3, 4, 5]);
+        $seq2 = new SynchronizableSequence([1, 2, 3, 4, 5], []);
 
         $lastIndex = $seq2->synchronize($seq1);
 
@@ -42,7 +42,7 @@ public function testSyncForRemovedElement()
     public function testSyncForAddedElement()
     {
         $seq1 = new Sequence([1, 2, 3, 4, 5, 6]);
-        $seq2 = new SynchronizableSequence([1, 2, 3, 4, 5]);
+        $seq2 = new SynchronizableSequence([1, 2, 3, 4, 5], []);
 
         $lastIndex = $seq2->synchronize($seq1);
 
@@ -58,7 +58,7 @@ public function testSyncForAddedElement()
     public function testSyncWithoutOutOfBoundCheck()
     {
         $seq1 = new Sequence([1, 4, 6]);
-        $seq2 = new SynchronizableSequence([1, 2, 3, 4]);
+        $seq2 = new SynchronizableSequence([1, 2, 3, 4], []);
 
         $lastIndex = $seq2->synchronize($seq1);
 
@@ -74,7 +74,7 @@ public function testSyncWithoutOutOfBoundCheck()
     public function testSyncWithCheckAndOutOfBoundElement()
     {
         $seq1 = new Sequence([1, 4, 6]);
-        $seq2 = new SynchronizableSequence([1, 2, 3, 4]);
+        $seq2 = new SynchronizableSequence([1, 2, 3, 4], []);
 
         $lastIndex = $seq2->synchronize($seq1, true);
 
@@ -90,7 +90,7 @@ public function testSyncWithCheckAndOutOfBoundElement()
     public function testSyncWithoutCheckAndRemovalAtEnd()
     {
         $seq1 = new Sequence([1, 3, 4]);
-        $seq2 = new SynchronizableSequence([1, 2, 4, 7]);
+        $seq2 = new SynchronizableSequence([1, 2, 4, 7], []);
 
         $lastIndex = $seq2->synchronize($seq1);
 
@@ -106,7 +106,7 @@ public function testSyncWithoutCheckAndRemovalAtEnd()
     public function testSyncWithCheckAndRemovalAtEnd()
     {
         $seq1 = new Sequence([1, 3, 4]);
-        $seq2 = new SynchronizableSequence([1, 2, 4, 7]);
+        $seq2 = new SynchronizableSequence([1, 2, 4, 7], []);
 
         $lastIndex = $seq2->synchronize($seq1, true);
 
@@ -122,7 +122,7 @@ public function testSyncWithCheckAndRemovalAtEnd()
     public function testSyncWithCheckAndOutOfBoundAtEnd()
     {
         $seq1 = new Sequence([1, 3, 4]);
-        $seq2 = new SynchronizableSequence([1, 5, 6]);
+        $seq2 = new SynchronizableSequence([1, 5, 6], []);
 
         $lastIndex = $seq2->synchronize($seq1, true);
 
@@ -138,7 +138,7 @@ public function testSyncWithCheckAndOutOfBoundAtEnd()
     public function testSyncWithCheckAndFullAddition()
     {
         $seq1 = new Sequence([1, 3, 4]);
-        $seq2 = new SynchronizableSequence([]);
+        $seq2 = new SynchronizableSequence([], []);
 
         $lastIndex = $seq2->synchronize($seq1, true);
 
@@ -154,7 +154,7 @@ public function testSyncWithCheckAndFullAddition()
     public function testSyncWithCheckAndFullRemoval()
     {
         $seq1 = new Sequence([]);
-        $seq2 = new SynchronizableSequence([1, 3, 4]);
+        $seq2 = new SynchronizableSequence([1, 3, 4], []);
 
         $lastIndex = $seq2->synchronize($seq1, true);