Skip to content

Commit 18f8469

Browse files
ssddanbrownssddanbrown
committed
LDAP: Fixed handling of invalid group values
Updated LDAP group handling to properly handle empty group values by checking the explode's count property instead of performing a general array count. Also updated logic with DN validation/filtering before LDAP calls are made. For #6088
1 parent 7539933 commit 18f8469

3 files changed

Lines changed: 71 additions & 16 deletions

File tree

‎app/Access/Ldap.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public function setVersion($ldapConnection, int $version): bool
4848
}
4949

5050
/**
51-
* Search LDAP tree using the provided filter.
51+
* Search the LDAP tree using the provided filter.
5252
*
5353
* @param resource|\LDAP\Connection $ldapConnection
5454
*
@@ -95,7 +95,7 @@ public function searchAndGetEntries($ldapConnection, string $baseDn, string $fil
9595
}
9696

9797
/**
98-
* Bind to LDAP directory.
98+
* Bind to the LDAP directory.
9999
*
100100
* @param resource|\LDAP\Connection $ldapConnection
101101
*/

‎app/Access/LdapService.php‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -346,13 +346,15 @@ public function getUserGroups(string $userName): array
346346
}
347347

348348
$userGroups = $this->extractGroupsFromSearchResponseEntry($user);
349-
$allGroups = $this->getGroupsRecursive($userGroups, []);
349+
$filteredGroups = $this->filterGroups($userGroups);
350+
$allGroups = $this->getGroupsRecursive($filteredGroups, []);
350351
$formattedGroups = $this->extractGroupNamesFromLdapGroupDns($allGroups);
351352

352353
if ($this->config['dump_user_groups']) {
353354
throw new JsonDebugException([
354355
'details_from_ldap' => $user,
355356
'parsed_direct_user_groups' => $userGroups,
357+
'parsed_filtered_user_groups' => $filteredGroups,
356358
'parsed_recursive_user_groups' => $allGroups,
357359
'parsed_resulting_group_names' => $formattedGroups,
358360
]);
@@ -367,7 +369,7 @@ protected function extractGroupNamesFromLdapGroupDns(array $groupDNs): array
367369

368370
foreach ($groupDNs as $groupDN) {
369371
$exploded = $this->ldap->explodeDn($groupDN, 1);
370-
if ($exploded !== false && count($exploded) > 0) {
372+
if ($exploded !== false && $exploded['count'] > 0) {
371373
$names[] = $exploded[0];
372374
}
373375
}
@@ -390,7 +392,8 @@ protected function getGroupsRecursive(array $groupDNs, array $checked): array
390392
}
391393

392394
$parentGroups = $this->getParentsOfGroup($groupDN);
393-
$groupsToAdd = array_merge($groupsToAdd, $parentGroups);
395+
$parentGroupsFiltered = $this->filterGroups($parentGroups);
396+
$groupsToAdd = array_merge($groupsToAdd, $parentGroupsFiltered);
394397
$checked[] = $groupDN;
395398
}
396399

@@ -403,6 +406,20 @@ protected function getGroupsRecursive(array $groupDNs, array $checked): array
403406
return $this->getGroupsRecursive($uniqueDNs, $checked);
404407
}
405408

409+
/**
410+
* @param string[] $groupDNs
411+
* @return string[]
412+
*/
413+
protected function filterGroups(array $groupDNs): array
414+
{
415+
$filtered = array_filter($groupDNs, function (string $groupDN) {
416+
$exploded = $this->ldap->explodeDn($groupDN, 1);
417+
return $exploded !== false && $exploded['count'] > 0;
418+
});
419+
420+
return array_values($filtered);
421+
}
422+
406423
/**
407424
* @throws LdapException
408425
*/

‎tests/Auth/LdapTest.php‎

Lines changed: 49 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,22 +43,22 @@ protected function setUp(): void
4343
$this->mockUser = User::factory()->make();
4444
}
4545

46-
protected function runFailedAuthLogin()
46+
protected function runFailedAuthLogin(): void
4747
{
4848
$this->commonLdapMocks(1, 1, 1, 1, 1);
4949
$this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)
5050
->andReturn(['count' => 0]);
5151
$this->post('/login', ['username' => 'timmyjenkins', 'password' => 'cattreedog']);
5252
}
5353

54-
protected function mockEscapes($times = 1)
54+
protected function mockEscapes($times = 1): void
5555
{
5656
$this->mockLdap->shouldReceive('escape')->times($times)->andReturnUsing(function ($val) {
5757
return ldap_escape($val);
5858
});
5959
}
6060

61-
protected function mockExplodes($times = 1)
61+
protected function mockExplodes($times = 1): void
6262
{
6363
$this->mockLdap->shouldReceive('explodeDn')->times($times)->andReturnUsing(function ($dn, $withAttrib) {
6464
return ldap_explode_dn($dn, $withAttrib);
@@ -76,7 +76,7 @@ protected function mockUserLogin(?string $email = null): TestResponse
7676
/**
7777
* Set LDAP method mocks for things we commonly call without altering.
7878
*/
79-
protected function commonLdapMocks(int $connects = 1, int $versions = 1, int $options = 2, int $binds = 4, int $escapes = 2, int $explodes = 0, int $groups = 0)
79+
protected function commonLdapMocks(int $connects = 1, int $versions = 1, int $options = 2, int $binds = 4, int $escapes = 2, int $explodes = 0, int $groups = 0): void
8080
{
8181
$this->mockLdap->shouldReceive('connect')->times($connects)->andReturn($this->resourceId);
8282
$this->mockLdap->shouldReceive('setVersion')->times($versions);
@@ -364,7 +364,7 @@ public function test_login_maps_roles_and_retains_existing_roles()
364364
'services.ldap.remove_from_groups' => false,
365365
]);
366366

367-
$this->commonLdapMocks(1, 1, 4, 5, 2, 2, 2);
367+
$this->commonLdapMocks(1, 1, 4, 5, 2, 4, 2);
368368
$this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
369369
->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
370370
->andReturn(['count' => 1, 0 => [
@@ -409,7 +409,7 @@ public function test_login_maps_roles_and_removes_old_roles_if_set()
409409
'services.ldap.remove_from_groups' => true,
410410
]);
411411

412-
$this->commonLdapMocks(1, 1, 3, 4, 2, 1, 1);
412+
$this->commonLdapMocks(1, 1, 3, 4, 2, 2, 1);
413413
$this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
414414
->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
415415
->andReturn(['count' => 1, 0 => [
@@ -451,7 +451,7 @@ public function test_dump_user_groups_shows_group_related_details_as_json()
451451
'dn' => 'dc=test,' . config('services.ldap.base_dn'),
452452
'mail' => [$this->mockUser->email],
453453
]];
454-
$this->commonLdapMocks(1, 1, 4, 5, 2, 2, 0);
454+
$this->commonLdapMocks(1, 1, 4, 5, 2, 4, 0);
455455
$this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
456456
->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
457457
->andReturn($userResp, ['count' => 1,
@@ -523,7 +523,7 @@ public function test_recursive_group_search_queries_via_full_dn()
523523
],
524524
];
525525

526-
$this->commonLdapMocks(1, 1, 3, 4, 2, 1);
526+
$this->commonLdapMocks(1, 1, 3, 4, 2, 2);
527527

528528
$escapedName = ldap_escape($this->mockUser->name);
529529
$this->mockLdap->shouldReceive('searchAndGetEntries')->twice()
@@ -559,7 +559,7 @@ public function test_login_maps_roles_using_external_auth_ids_if_set()
559559
'services.ldap.remove_from_groups' => true,
560560
]);
561561

562-
$this->commonLdapMocks(1, 1, 3, 4, 2, 1, 1);
562+
$this->commonLdapMocks(1, 1, 3, 4, 2, 2, 1);
563563
$this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
564564
->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
565565
->andReturn(['count' => 1, 0 => [
@@ -600,7 +600,7 @@ public function test_login_group_mapping_does_not_conflict_with_default_role()
600600
'services.ldap.remove_from_groups' => true,
601601
]);
602602

603-
$this->commonLdapMocks(1, 1, 4, 5, 2, 2, 2);
603+
$this->commonLdapMocks(1, 1, 4, 5, 2, 4, 2);
604604
$this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
605605
->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
606606
->andReturn(['count' => 1, 0 => [
@@ -628,6 +628,44 @@ public function test_login_group_mapping_does_not_conflict_with_default_role()
628628
]);
629629
}
630630

631+
public function test_login_group_mapping_gracefully_handles_utf8_and_invalid_group_dns()
632+
{
633+
$invalidRole = Role::factory()->create(['display_name' => 'LdapTester']);
634+
$invalidRole2 = Role::factory()->create(['display_name' => 'beans']);
635+
$roleToReceive2 = Role::factory()->create(['display_name' => 'ldapper']);
636+
$this->mockUser->forceFill(['external_auth_id' => $this->mockUser->name])->save();
637+
638+
app('config')->set([
639+
'services.ldap.user_to_groups' => true,
640+
'services.ldap.group_attribute' => 'memberOf',
641+
'services.ldap.remove_from_groups' => true,
642+
]);
643+
644+
$this->commonLdapMocks(1, 1, 4, 5, 2, 6, 2);
645+
$this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
646+
->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
647+
->andReturn(['count' => 1, 0 => [
648+
'uid' => [$this->mockUser->name],
649+
'cn' => [$this->mockUser->name],
650+
'dn' => 'dc=test' . config('services.ldap.base_dn'),
651+
'mail' => [$this->mockUser->email],
652+
'memberof' => [
653+
'count' => 4,
654+
0 => 'cn=ldaptëstër,ou=groups,dc=ëxamplë,dc=com',
655+
1 => 'cn=ldapper,ou=groups,dc=example,dc=com',
656+
2 => 'bëans',
657+
3 => '',
658+
],
659+
]]);
660+
661+
$this->mockUserLogin()->assertRedirect('/');
662+
663+
$user = User::query()->where('email', $this->mockUser->email)->first();
664+
$this->assertDatabaseMissing('role_user', ['user_id' => $user->id, 'role_id' => $invalidRole->id]);
665+
$this->assertDatabaseMissing('role_user', ['user_id' => $user->id, 'role_id' => $invalidRole2->id]);
666+
$this->assertDatabaseHas('role_user', ['user_id' => $user->id, 'role_id' => $roleToReceive2->id]);
667+
}
668+
631669
public function test_login_uses_specified_display_name_attribute()
632670
{
633671
app('config')->set([
@@ -867,7 +905,7 @@ public function test_login_with_email_confirmation_required_maps_groups_but_show
867905
'services.ldap.remove_from_groups' => true,
868906
]);
869907

870-
$this->commonLdapMocks(1, 1, 6, 8, 4, 2, 2);
908+
$this->commonLdapMocks(1, 1, 6, 8, 4, 4, 2);
871909
$this->mockLdap->shouldReceive('searchAndGetEntries')
872910
->times(4)
873911
->andReturn(['count' => 1, 0 => [

0 commit comments

Comments
 (0)