Merge pull request silverstripe#3 from caffeineinc/bugfix/fix-remove-…

…group-mappings Fix group mappings for removed Groups
creative-commoners · Nov 4, 2018 · 6203f15 · 6203f15
2 parents c719e9e + 6fe2cae
commit 6203f15
Show file tree

Hide file tree

Showing 7 changed files with 371 additions and 27 deletions.
diff --git a/docs/en/developer.md b/docs/en/developer.md
@@ -493,6 +493,23 @@ SilverStripe\LDAP\Services\LDAPService:
 
 This will allow users to change their AD password via the regular CMS "forgot password" forms, etc.
 
+### Allow SilverStripe attributes to be reset (removed) by AD
+
+By default if attributes are present, and then missing in subsequent requests, they are ignored (non-destructive) by 
+this module. This can cause attributes to persist when they've been deliberately removed (attribute is no longer present)
+in the LDAP source data. 
+
+If you wish a full two way sync to occur, then set the attribute on `LDAPService` for `reset_missing_attributes` to 
+enable a full sync. 
+
+*Note*: This will mean syncs are destructive, and data or attributes will be reset if missing from the master LDAP source
+data. 
+
+```yaml
+SilverStripe\LDAP\Services\LDAPService:
+  reset_missing_attributes: true 
+```
+
 ### Writing LDAP data from SilverStripe
 
 A feature is available that allows data to be written back to LDAP based on the state of `SilverStripe\Security\Member` object fields.

diff --git a/src/Extensions/LDAPMemberExtension.php b/src/Extensions/LDAPMemberExtension.php
@@ -248,6 +248,7 @@ public function onAfterDelete()
         }
 
         $service->deleteLDAPMember($this->owner);
+
     }
 
     /**

diff --git a/src/Services/LDAPService.php b/src/Services/LDAPService.php
@@ -25,8 +25,10 @@
 use SilverStripe\Core\Injector\Injectable;
 use SilverStripe\ORM\DB;
 use SilverStripe\ORM\FieldType\DBDatetime;
+use SilverStripe\ORM\Relation;
 use SilverStripe\ORM\ValidationException;
 use SilverStripe\ORM\ValidationResult;
+use SilverStripe\ORM\DataQuery;
 use SilverStripe\Security\Group;
 use SilverStripe\Security\Member;
 use SilverStripe\Security\RandomGenerator;
@@ -546,17 +548,34 @@ public function updateMemberFromLDAP(Member $member, $data = null, $updateGroups
         $member->LastSynced = (string)DBDatetime::now();
 
         foreach ($member->config()->ldap_field_mappings as $attribute => $field) {
+            // Special handling required for attributes that don't exist in the response.
             if (!isset($data[$attribute])) {
-                $this->getLogger()->notice(
-                    sprintf(
-                        'Attribute %s configured in Member.ldap_field_mappings, ' .
-                                'but no available attribute in AD data (GUID: %s, Member ID: %s)',
-                        $attribute,
-                        $data['objectguid'],
-                        $member->ID
-                    )
-                );
+                // a attribute we're expecting is missing from the LDAP response
+                if ($this->config()->get("reset_missing_attributes")) {
+                    // (Destructive) Reset the corresponding attribute on our side if instructed to do so.
+                    if (method_exists($member->$field, "delete")
+                        && method_exists($member->$field, "deleteFile")
+                        && $member->$field->exists()
+                    ) {
+                        $member->$field->deleteFile();
+                        $member->$field->delete();
+                    } else {
+                        $member->$field = null;
+                    }
+                // or log the information.
+                } else {
+                    $this->getLogger()->debug(
+                        sprintf(
+                            'Attribute %s configured in Member.ldap_field_mappings, ' .
+                            'but no available attribute in AD data (GUID: %s, Member ID: %s)',
+                            $attribute,
+                            $data['objectguid'],
+                            $member->ID
+                        )
+                    );
+                }
 
+                // No further processing required.
                 continue;
             }
 
@@ -619,6 +638,8 @@ public function updateMemberFromLDAP(Member $member, $data = null, $updateGroups
      * @param array $data Array of all data returned about this user from LDAP
      * @param string $attributeName Name of the attribute in the $data array to get the binary blog from
      * @return boolean true on success, false on failure
+     *
+     * @throws ValidationException
      */
     private function processThumbnailPhoto(Member $member, $fieldName, $data, $attributeName)
     {
@@ -640,6 +661,12 @@ private function processThumbnailPhoto(Member $member, $fieldName, $data, $attri
         $existingObj = $member->getComponent($fieldName);
         if ($existingObj && $existingObj->exists()) {
             $file = $existingObj;
+
+            // If the file hashes match, and the file already exists, we don't need to update anything.
+            $hash = $existingObj->File->getHash();
+            if (hash_equals($hash, sha1($data[$attributeName]))) {
+                return true;
+            }
         } else {
             $file = new Image();
         }
@@ -649,6 +676,7 @@ private function processThumbnailPhoto(Member $member, $fieldName, $data, $attri
         $filename = sprintf('thumbnailphoto-%s.jpg', $data['objectguid']);
         $filePath = File::join_paths($thumbnailFolder->getFilename(), $filename);
         $fileCfg = [
+            // if there's a filename conflict we've got new content so overwrite it.
             'conflict' => AssetStore::CONFLICT_OVERWRITE,
             'visibility' => AssetStore::VISIBILITY_PUBLIC
         ];
@@ -664,6 +692,8 @@ private function processThumbnailPhoto(Member $member, $fieldName, $data, $attri
             $relationField = sprintf('%sID', $fieldName);
             $member->{$relationField} = $file->ID;
         }
+
+        return true;
     }
 
     /**
@@ -723,23 +753,23 @@ public function updateMemberGroups($data, Member $member)
             }
         }
 
-        // remove the user from any previously mapped groups, where the mapping has since been removed
-        $groupRecords = DB::query(
-            sprintf(
-                'SELECT "GroupID" FROM "Group_Members" WHERE "IsImportedFromLDAP" = 1 AND "MemberID" = %s',
-                $member->ID
-            )
-        );
-
-        if (!empty($mappedGroupIDs)) {
-            foreach ($groupRecords as $groupRecord) {
-                if (!in_array($groupRecord['GroupID'], $mappedGroupIDs)) {
-                    $group = Group::get()->byId($groupRecord['GroupID']);
-                    // Some groups may no longer exist. SilverStripe does not clean up join tables.
-                    if ($group) {
-                        $group->Members()->remove($member);
-                    }
-                }
+        // Lookup the previous mappings and see if there is any mappings no longer present.
+        $unmappedGroups = $member->Groups()->alterDataQuery(function (DataQuery $query) {
+            // join with the Group_Members table because we only want those group members assigned by this module.
+            $query->leftJoin("Group_Members", "Group_Members.GroupID = Group.ID");
+            $query->where("IsImportedFromLDAP = 1");
+        });
+
+        // Don't remove associations which have just been added and we know are already correct!
+        if(!empty($mappedGroupIDs)){
+            $unmappedGroups = $unmappedGroups->filter("GroupID:NOT", $mappedGroupIDs);
+        }
+
+        // Remove the member from any previously mapped groups, where the mapping
+        // has since been removed in the LDAP data source
+        if ($unmappedGroups->count()) {
+            foreach ($unmappedGroups as $group) {
+                $group->Members()->remove($member);
             }
         }
     }

diff --git a/tests/php/Model/LDAPFakeGateway.php b/tests/php/Model/LDAPFakeGateway.php
@@ -11,7 +11,9 @@ class LDAPFakeGateway extends LDAPGateway implements TestOnly
 {
     public function __construct()
     {
-        // do nothing
+        // thumbnail images are raw JPEG/JFIF files, but that's not important
+        // for this test, as long as the binary content are the same
+        self::$data['users']['456']['thumbnailphoto'] = base64_decode(self::$data['users']['456']['thumbnailphoto']);
     }
 
     private static $data = [
@@ -42,6 +44,42 @@ public function __construct()
                 'canonicalName'=>'mockCanonicalName',
                 'userprincipalname' => '[email protected]',
                 'samaccountname' => 'joe'
+            ],
+            '456' => [
+                'distinguishedname' => 'CN=Appleseed,DC=playpen,DC=local',
+                'objectguid' => '456',
+                'cn' => 'jappleseed',
+                'useraccountcontrol' => '1',
+                'givenname' => 'Johnny',
+                'sn' => 'Appleseed',
+                'mail' => '[email protected]',
+                'password' => 'mockPassword1',
+                'canonicalName' => 'mockCanonicalName2',
+                'userprincipalname' => '[email protected]',
+                'samaccountname' => 'john',
+                'thumbnailphoto' => 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==',
+                'displayname' => 'Johnny Appleseed'
+            ],
+            '789' => [
+                'distinguishedname' => 'CN=Appleseed,DC=playpen,DC=local',
+                'objectguid' => '456',
+                'cn' => 'jappleseed',
+                'useraccountcontrol' => '1',
+                'givenname' => 'Johnny',
+                'sn' => 'Appleseed',
+                'mail' => '[email protected]',
+                'password' => 'mockPassword1',
+                'canonicalName' => 'mockCanonicalName2',
+                'userprincipalname' => '[email protected]',
+                'samaccountname' => 'john',
+                'thumbnailphoto' => 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==',
+                'displayname' => 'Johnny Appleseed',
+                'memberof' => [
+                    'CN=Group1,CN=Users,DC=playpen,DC=local',
+                    'CN=Group2,CN=Users,DC=playpen,DC=local',
+                    'CN=Group3,CN=Users,DC=playpen,DC=local',
+                    'CN=Group4,CN=Users,DC=playpen,DC=local',
+                ]
             ]
         ]
     ];
@@ -77,8 +115,19 @@ public function getGroups($baseDn = null, $scope = Ldap::SEARCH_SCOPE_SUB, $attr
         }
     }
 
+    /**
+     * Return nested groups for a DN. Not currently implemented.
+     *
+     * @param string $dn
+     * @param null $baseDn
+     * @param int $scope
+     * @param array $attributes
+     *
+     * @return array
+     */
     public function getNestedGroups($dn, $baseDn = null, $scope = Ldap::SEARCH_SCOPE_SUB, $attributes = [])
     {
+        return [];
     }
 
     public function getGroupByGUID($guid, $baseDn = null, $scope = Ldap::SEARCH_SCOPE_SUB, $attributes = [])

diff --git a/tests/php/Model/LDAPFakeMember.php b/tests/php/Model/LDAPFakeMember.php
@@ -0,0 +1,26 @@
+<?php
+
+namespace SilverStripe\LDAP\Tests\Model;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\Security\Member;
+use SilverStripe\Assets\Image;
+
+class LDAPFakeMember extends Member implements TestOnly
+{
+    /**
+     * @var array
+     */
+    private static $has_one = [
+        'ProfileImage' => Image::class
+    ];
+
+    /**
+     * We don't actually want/need to change anything
+     *
+     * @return int|void
+     */
+    public function write(){
+        // Noop
+    }
+}