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

@@ -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.
      *

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,
+                'parsed_resulting_group_names' => $formattedGroups,
             ]);
         }
 
         return $allGroups;
     }
 
+    protected function extractGroupNamesFromLdapGroupDns(array $groupDNs): array
+    {
+        $names = [];
+
+        foreach ($groupDNs as $groupDN) {
+            $exploded = $this->ldap->explodeDn($groupDN, 1);
+            if ($exploded !== false && count($exploded) > 0) {
+                $names[] = $exploded[0];
+            }
+        }
+
+        return array_unique($names);
+    }
+
     /**
-     * Get the parent groups of an array of groups.
+     * Build an array of all relevant groups DNs after recursively scanning
+     * across parents of the groups given.
      *
      * @throws LdapException
      */
-    private function getGroupsRecursive(array $groupsArray, array $checked): array
+    protected function getGroupsRecursive(array $groupDNs, array $checked): array
     {
         $groupsToAdd = [];
-        foreach ($groupsArray as $groupName) {
+        foreach ($groupDNs as $groupDN) {
-            if (in_array($groupName, $checked)) {
+            if (in_array($groupDN, $checked)) {
                 continue;
             }
 
-            $parentGroups = $this->getGroupGroups($groupName);
+            $parentGroups = $this->getParentsOfGroup($groupDN);
             $groupsToAdd = array_merge($groupsToAdd, $parentGroups);
-            $checked[] = $groupName;
+            $checked[] = $groupDN;
         }
 
-        $groupsArray = array_unique(array_merge($groupsArray, $groupsToAdd), SORT_REGULAR);
+        $uniqueDNs = array_unique(array_merge($groupDNs, $groupsToAdd), SORT_REGULAR);
 
         if (empty($groupsToAdd)) {
-            return $groupsArray;
+            return $uniqueDNs;
         }
 
-        return $this->getGroupsRecursive($groupsArray, $checked);
+        return $this->getGroupsRecursive($uniqueDNs, $checked);
     }
 
     /**
-     * Get the parent groups of a single group.
-     *
      * @throws LdapException
      */
-    private function getGroupGroups(string $groupName): array
+    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);
-
+        $read = $this->ldap->read($ldapConnection, $groupDN, '(objectClass=*)', [$groupsAttr]);
-        $baseDn = $this->config['base_dn'];
+        $results = $this->ldap->getEntries($ldapConnection, $read);
-        $groupsAttr = strtolower($this->config['group_attribute']);
+        if ($results['count'] === 0) {
-
-        $groupFilter = 'CN=' . $this->ldap->escape($groupName);
-        $groups = $this->ldap->searchAndGetEntries($ldapConnection, $baseDn, $groupFilter, [$groupsAttr]);
-        if ($groups['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.
+     * Extract an array of group DN values from the given LDAP search response entry
-     * Gets the base CN (common name) of the string.
      */
-    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'])) {
+        if (isset($ldapEntry[$groupsAttr]['count'])) {
-            $count = (int) $userGroupSearchResponse[$groupsAttr]['count'];
+            $count = (int) $ldapEntry[$groupsAttr]['count'];
         }
 
         for ($i = 0; $i < $count; $i++) {
-            $dnComponents = $this->ldap->explodeDn($userGroupSearchResponse[$groupsAttr][$i], 1);
+            $dn = $ldapEntry[$groupsAttr][$i];
-            if (!in_array($dnComponents[0], $ldapGroups)) {
+            if (!in_array($dn, $groupDNs)) {
-                $ldapGroups[] = $dnComponents[0];
+                $groupDNs[] = $dn;
             }
         }
 
-        return $ldapGroups;
+        return $groupDNs;
     }
 
     /**

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']);