From 1b4ed69f41955d27794718cd74b3f56fba9be63f Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 28 Aug 2024 15:39:05 +0100 Subject: [PATCH] LDAP: Updated recursive group search to query by DN Added test to cover, added pre-change. Need to test post-changes and fix tests. --- app/Access/Ldap.php | 14 ++++- app/Access/LdapService.php | 101 ++++++++++++++++++++----------------- tests/Auth/LdapTest.php | 38 ++++++++++++++ 3 files changed, 107 insertions(+), 46 deletions(-) diff --git a/app/Access/Ldap.php b/app/Access/Ldap.php index 12a3d1e71..702d629ce 100644 --- a/app/Access/Ldap.php +++ b/app/Access/Ldap.php @@ -52,13 +52,25 @@ class Ldap * * @param resource|\LDAP\Connection $ldapConnection * - * @return resource|\LDAP\Result + * @return \LDAP\Result|array|false */ public function search($ldapConnection, string $baseDn, string $filter, array $attributes = null) { return ldap_search($ldapConnection, $baseDn, $filter, $attributes); } + /** + * Read an entry from the LDAP tree. + * + * @param resource|\Ldap\Connection $ldapConnection + * + * @return \LDAP\Result|array|false + */ + public function read($ldapConnection, string $baseDn, string $filter, array $attributes = null) + { + return ldap_read($ldapConnection, $baseDn, $filter, $attributes); + } + /** * Get entries from an LDAP search result. * diff --git a/app/Access/LdapService.php b/app/Access/LdapService.php index e822b09a6..7c8d8b18f 100644 --- a/app/Access/LdapService.php +++ b/app/Access/LdapService.php @@ -321,94 +321,105 @@ class LdapService return []; } - $userGroups = $this->groupFilter($user); + $userGroups = $this->extractGroupsFromSearchResponseEntry($user); $allGroups = $this->getGroupsRecursive($userGroups, []); + $formattedGroups = $this->extractGroupNamesFromLdapGroupDns($allGroups); if ($this->config['dump_user_groups']) { throw new JsonDebugException([ - 'details_from_ldap' => $user, - 'parsed_direct_user_groups' => $userGroups, - 'parsed_recursive_user_groups' => $allGroups, + 'details_from_ldap' => $user, + 'parsed_direct_user_groups' => $userGroups, + 'parsed_recursive_user_groups' => $allGroups, + 'parsed_resulting_group_names' => $formattedGroups, ]); } return $allGroups; } - /** - * Get the parent groups of an array of groups. - * - * @throws LdapException - */ - private function getGroupsRecursive(array $groupsArray, array $checked): array + protected function extractGroupNamesFromLdapGroupDns(array $groupDNs): array { - $groupsToAdd = []; - foreach ($groupsArray as $groupName) { - if (in_array($groupName, $checked)) { - continue; + $names = []; + + foreach ($groupDNs as $groupDN) { + $exploded = $this->ldap->explodeDn($groupDN, 1); + if ($exploded !== false && count($exploded) > 0) { + $names[] = $exploded[0]; } - - $parentGroups = $this->getGroupGroups($groupName); - $groupsToAdd = array_merge($groupsToAdd, $parentGroups); - $checked[] = $groupName; } - $groupsArray = array_unique(array_merge($groupsArray, $groupsToAdd), SORT_REGULAR); - - if (empty($groupsToAdd)) { - return $groupsArray; - } - - return $this->getGroupsRecursive($groupsArray, $checked); + return array_unique($names); } /** - * Get the parent groups of a single group. + * Build an array of all relevant groups DNs after recursively scanning + * across parents of the groups given. * * @throws LdapException */ - private function getGroupGroups(string $groupName): array + protected function getGroupsRecursive(array $groupDNs, array $checked): array { + $groupsToAdd = []; + foreach ($groupDNs as $groupDN) { + if (in_array($groupDN, $checked)) { + continue; + } + + $parentGroups = $this->getParentsOfGroup($groupDN); + $groupsToAdd = array_merge($groupsToAdd, $parentGroups); + $checked[] = $groupDN; + } + + $uniqueDNs = array_unique(array_merge($groupDNs, $groupsToAdd), SORT_REGULAR); + + if (empty($groupsToAdd)) { + return $uniqueDNs; + } + + return $this->getGroupsRecursive($uniqueDNs, $checked); + } + + /** + * @throws LdapException + */ + protected function getParentsOfGroup(string $groupDN): array + { + $groupsAttr = strtolower($this->config['group_attribute']); $ldapConnection = $this->getConnection(); $this->bindSystemUser($ldapConnection); $followReferrals = $this->config['follow_referrals'] ? 1 : 0; $this->ldap->setOption($ldapConnection, LDAP_OPT_REFERRALS, $followReferrals); - - $baseDn = $this->config['base_dn']; - $groupsAttr = strtolower($this->config['group_attribute']); - - $groupFilter = 'CN=' . $this->ldap->escape($groupName); - $groups = $this->ldap->searchAndGetEntries($ldapConnection, $baseDn, $groupFilter, [$groupsAttr]); - if ($groups['count'] === 0) { + $read = $this->ldap->read($ldapConnection, $groupDN, '(objectClass=*)', [$groupsAttr]); + $results = $this->ldap->getEntries($ldapConnection, $read); + if ($results['count'] === 0) { return []; } - return $this->groupFilter($groups[0]); + return $this->extractGroupsFromSearchResponseEntry($results[0]); } /** - * Filter out LDAP CN and DN language in a ldap search return. - * Gets the base CN (common name) of the string. + * Extract an array of group DN values from the given LDAP search response entry */ - protected function groupFilter(array $userGroupSearchResponse): array + protected function extractGroupsFromSearchResponseEntry(array $ldapEntry): array { $groupsAttr = strtolower($this->config['group_attribute']); - $ldapGroups = []; + $groupDNs = []; $count = 0; - if (isset($userGroupSearchResponse[$groupsAttr]['count'])) { - $count = (int) $userGroupSearchResponse[$groupsAttr]['count']; + if (isset($ldapEntry[$groupsAttr]['count'])) { + $count = (int) $ldapEntry[$groupsAttr]['count']; } for ($i = 0; $i < $count; $i++) { - $dnComponents = $this->ldap->explodeDn($userGroupSearchResponse[$groupsAttr][$i], 1); - if (!in_array($dnComponents[0], $ldapGroups)) { - $ldapGroups[] = $dnComponents[0]; + $dn = $ldapEntry[$groupsAttr][$i]; + if (!in_array($dn, $groupDNs)) { + $groupDNs[] = $dn; } } - return $ldapGroups; + return $groupDNs; } /** diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 3f80f00f4..7c2510eb1 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -435,6 +435,44 @@ class LdapTest extends TestCase ]); } + public function test_recursive_group_search_queries_via_full_dn() + { + app('config')->set([ + 'services.ldap.user_to_groups' => true, + 'services.ldap.group_attribute' => 'memberOf', + ]); + + $userResp = ['count' => 1, 0 => [ + 'uid' => [$this->mockUser->name], + 'cn' => [$this->mockUser->name], + 'dn' => 'dc=test,' . config('services.ldap.base_dn'), + 'mail' => [$this->mockUser->email], + ]]; + $groupResp = ['count' => 1, + 0 => [ + 'dn' => 'dc=test,' . config('services.ldap.base_dn'), + 'memberof' => [ + 'count' => 1, + 0 => 'cn=ldaptester,ou=groups,dc=example,dc=com', + ], + ], + ]; + + $this->commonLdapMocks(1, 1, 3, 4, 3, 1); + + $escapedName = ldap_escape($this->mockUser->name); + $this->mockLdap->shouldReceive('searchAndGetEntries')->twice() + ->with($this->resourceId, config('services.ldap.base_dn'), "(&(uid={$escapedName}))", \Mockery::type('array')) + ->andReturn($userResp, $groupResp); + + $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) + ->with($this->resourceId, config('services.ldap.base_dn'), $groupResp[0]['dn'], ['memberof']) + ->andReturn(['count' => 0]); + + $resp = $this->mockUserLogin(); + $resp->assertRedirect('/'); + } + public function test_external_auth_id_visible_in_roles_page_when_ldap_active() { $role = Role::factory()->create(['display_name' => 'ldaptester', 'external_auth_id' => 'ex-auth-a, test-second-param']);