diff --git a/docs/en/developer.md b/docs/en/developer.md index 36ca040..808f705 100644 --- 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 index 27b8e2e..7bac541 100644 --- 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 index 822fb00..8185d4d 100644 --- 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 index 73cd82a..6cfc1cd 100644 --- 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' => 'joe@bloggs.com', 'samaccountname' => 'joe' + ], + '456' => [ + 'distinguishedname' => 'CN=Appleseed,DC=playpen,DC=local', + 'objectguid' => '456', + 'cn' => 'jappleseed', + 'useraccountcontrol' => '1', + 'givenname' => 'Johnny', + 'sn' => 'Appleseed', + 'mail' => 'john@appleseed.com', + 'password' => 'mockPassword1', + 'canonicalName' => 'mockCanonicalName2', + 'userprincipalname' => 'john@appleseed.com', + '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' => 'john@appleseed.com', + 'password' => 'mockPassword1', + 'canonicalName' => 'mockCanonicalName2', + 'userprincipalname' => 'john@appleseed.com', + '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 new file mode 100644 index 0000000..0ab4a41 --- /dev/null +++ b/tests/php/Model/LDAPFakeMember.php @@ -0,0 +1,26 @@ + Image::class + ]; + + /** + * We don't actually want/need to change anything + * + * @return int|void + */ + public function write(){ + // Noop + } +} diff --git a/tests/php/Services/LDAPServiceTest.php b/tests/php/Services/LDAPServiceTest.php index 56470b1..a0dff54 100644 --- a/tests/php/Services/LDAPServiceTest.php +++ b/tests/php/Services/LDAPServiceTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\LDAP\Tests\Services; +use SilverStripe\Assets\Image; use SilverStripe\Core\Config\Config; use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\SapphireTest; @@ -10,13 +11,24 @@ use SilverStripe\LDAP\Model\LDAPGateway; use SilverStripe\LDAP\Services\LDAPService; use SilverStripe\LDAP\Tests\Model\LDAPFakeGateway; +use SilverStripe\LDAP\Tests\Model\LDAPFakeMember; use SilverStripe\Security\Group; use SilverStripe\Security\Member; +use Silverstripe\Assets\Dev\TestAssetStore; +use SilverStripe\ORM\DataQuery; class LDAPServiceTest extends SapphireTest { protected $usesDatabase = true; + protected static $fixture_file = 'LDAPServiceTest.yml'; + + + /** + * @var LDAPService + */ + private $service; + protected function setUp() { parent::setUp(); @@ -95,4 +107,175 @@ public function testUpdateMemberFromLDAP() $this->assertEquals('Bloggs', $member->Surname, 'Surname updated from LDAP'); $this->assertEquals('joe@bloggs.com', $member->Email, 'Email updated from LDAP'); } + + /** + * LDAP should correctly assign a member to the groups, if there's a mapping between LDAPGroupMapping and Group + * and LDAP returns the mapping via 'memberof' + */ + public function testAssignGroupMember() + { + Config::modify()->set( + Member::class, + 'ldap_field_mappings', + [ + 'givenname' => 'FirstName', + 'sn' => 'Surname', + 'mail' => 'Email', + ] + ); + + $member = new Member(); + $member->GUID = '789'; + + $this->service->updateMemberFromLDAP($member); + $this->assertCount(4, $member->Groups()); + } + + /** + * LDAP should correctly assign a member to the groups, if there's a mapping between LDAPGroupMapping and Group + * and LDAP returns the mapping via 'memberof' + */ + public function testAssignRemovedGroupMember() + { + Config::modify()->set( + Member::class, + 'ldap_field_mappings', + [ + 'givenname' => 'FirstName', + 'sn' => 'Surname', + 'mail' => 'Email', + ] + ); + + $member = new Member(); + $member->GUID = '789'; + $member->write(); + + // Pretend we're the module, and generate some mappings. + Group::get()->each(function ($group) use ($member) { + $group->Members()->add($member, [ + 'IsImportedFromLDAP' => '1' + ]); + }); + + $this->assertCount(Group::get()->count(), $member->Groups()); + + // There should only be 4 groups assigned from LDAP for this user, two should be removed. + $this->service->updateMemberFromLDAP($member); + $this->assertCount(4, $member->Groups()); + } + + /** + * If the LDAPService setting reset_missing_attributes is true, reset fields if the attribute isn't present + * in the response information. + */ + public function testUpdateMemberResetAttributesFromLDAP() + { + Config::modify()->set( + Member::class, + 'ldap_field_mappings', + [ + 'givenname' => 'FirstName', + 'sn' => 'Surname', + 'mail' => 'Email', + 'specialattribute' => 'specialattribute' + ] + ); + + Config::modify()->set(LDAPService::class,'reset_missing_attributes', true); + + $member = new Member(); + $member->GUID = '123'; + $member->specialattribute = "I should be removed because LDAP said so"; + + $this->service->updateMemberFromLDAP($member); + + $this->assertTrue($member->ID > 0, 'updateMemberFromLDAP writes the member'); + $this->assertEquals('123', $member->GUID, 'GUID remains the same'); + $this->assertEquals('Joe', $member->FirstName, 'FirstName updated from LDAP'); + $this->assertEquals('Bloggs', $member->Surname, 'Surname updated from LDAP'); + $this->assertEquals('joe@bloggs.com', $member->Email, 'Email updated from LDAP'); + $this->assertNull($member->specialattribute); + } + + /** + * If the LDAPService setting reset_missing_attributes is true, delete the thumbnail (special case) + * if it's not present in the response information. + */ + public function testUpdateMemberResetThumbnailFromLDAP() + { + Config::modify()->set( + Member::class, + 'ldap_field_mappings', + [ + 'givenname' => 'FirstName', + 'sn' => 'Surname', + 'mail' => 'Email', + 'thumbnailphoto' => 'ProfileImage' + ] + ); + + Config::modify()->set(LDAPService::class,'reset_missing_attributes', true); + + // Create a test 'image' for this member. + /** @var File $file */ + TestAssetStore::activate('FileTest'); + $file = new Image(); + $file->setFromString(str_repeat('x', 1000000), "test.jpg"); + + $member = new LDAPFakeMember(); + $member->GUID = '123'; + $member->setComponent("ProfileImage", $file); + + // make sure our Profile image is there. + $this->assertNotNull($member->ProfileImage); + $this->assertTrue($member->ProfileImage->exists()); + + $this->service->updateMemberFromLDAP($member); + + // ensure the profile image was deleted, as it wasn't present in the attribute response from TestLDAP service + $this->assertFalse($member->ProfileImage->exists()); + } + + /** + * If the LDAPService setting reset_missing_attributes is true, delete the thumbnail (special case) + * if it's not present in the response information. + */ + public function testUpdateMemberLeaveThumbnailIfSameFromLDAP() + { + Config::modify()->set( + Member::class, + 'ldap_field_mappings', + [ + 'givenname' => 'FirstName', + 'sn' => 'Surname', + 'mail' => 'Email', + 'thumbnailphoto' => 'ProfileImage' + ] + ); + + Config::modify()->set(LDAPService::class,'reset_missing_attributes', true); + + // Create a test 'image' for this member. + /** @var File $file */ + TestAssetStore::activate('FileTest'); + + $member = new LDAPFakeMember(); + $member->GUID = '456'; + + // make sure our Profile image is there. + $this->assertNotNull($member->ProfileImage); + + $this->service->updateMemberFromLDAP($member); + + // We have an image from the service. + $this->assertTrue($member->ProfileImage->exists()); + + // now make sure it doesn't change. + $obj = $member->ProfileImage; + + $this->service->updateMemberFromLDAP($member); + + $this->assertSame($member->ProfileImage, $obj); + } } diff --git a/tests/php/Services/LDAPServiceTest.yml b/tests/php/Services/LDAPServiceTest.yml new file mode 100644 index 0000000..f452314 --- /dev/null +++ b/tests/php/Services/LDAPServiceTest.yml @@ -0,0 +1,38 @@ +SilverStripe\Security\Group: + group1: + Title: "Group 1" + GUID: "d339637c-d0c1-11e8-80b6-7831c1cc1572" + DN: 'CN=Group1,CN=Users,DC=playpen,DC=local' + group2: + Title: "Group 2" + GUID: "df05eb94-d0c1-11e8-80b6-7831c1cc1572" + DN: 'CN=Group2,CN=Users,DC=playpen,DC=local' + group3: + GUID: "e80dd03a-d0c1-11e8-80b6-7831c1cc1572" + Title: "Group 3" + DN: 'CN=Group3,CN=Users,DC=playpen,DC=local' + group4: + GUID: "ee36f90a-d0c1-11e8-80b6-7831c1cc1572" + Title: "Group 4" + DN: 'CN=Group4,CN=Users,DC=playpen,DC=local' + group5: + GUID: "f4e9b9ae-d0c1-11e8-80b6-7831c1cc1572" + Title: "Group 5" + DN: 'CN=Group5,CN=Users,DC=playpen,DC=local' + +SilverStripe\LDAP\Model\LDAPGroupMapping: + group1: + Group: =>SilverStripe\Security\Group.group1 + DN: 'CN=Group1,CN=Users,DC=playpen,DC=local' + group2: + Group: =>SilverStripe\Security\Group.group2 + DN: 'CN=Group2,CN=Users,DC=playpen,DC=local' + group3: + Group: =>SilverStripe\Security\Group.group3 + DN: 'CN=Group3,CN=Users,DC=playpen,DC=local' + group4: + Group: =>SilverStripe\Security\Group.group4 + DN: 'CN=Group4,CN=Users,DC=playpen,DC=local' + group5: + Group: =>SilverStripe\Security\Group.group5 + DN: 'CN=Group5,CN=Users,DC=playpen,DC=local'