8000 feature #44898 [Ldap] LDAP authentication should return a meaningful … · symfony/symfony@a4592e1 · GitHub
[go: up one dir, main page]
Skip to content

Commit a4592e1

Browse files
fabpotfabpot
committed
feature #44898 [Ldap] LDAP authentication should return a meaningful error when the LDAP server is unavailable (Jayfrown)
This PR was squashed before being merged into the 6.1 branch. Discussion ---------- [Ldap] LDAP authentication should return a meaningful error when the LDAP server is unavailable | Q | A | ------------- | --- | Branch? | 6.1 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #44089 | License | MIT Return a 500 ISE if the LDAP server is unreachable. I have tested the following scenarios: <details> <summary>LDAP server unreachable</summary> `Adapter/ExtLdap/Connection.php` throws a `ConnectionException` which is not caught, and this results in a 500 ISE shown to the user. In development mode, you see a relevant trace and the correct error hinting to an unreachable LDAP server, and in production mode this wouldn't be shown to the user, and would be logged. </details> <details> <summary>LDAP server reachable, with invalid search bind credentials</summary> The `LdapUserProvider` tries to search the LDAP directory for the provided user. To search, it tries to authenticate using the (invalid) bind credentials, resulting in `Adapter/ExtLdap/Connection.php` throwing an `InvalidCredentialsException` which is not caught. As this is indicative of a misconfiguration within the application (ie. the app's search bind credentials are wrong, and searching for any user will always fail) I believe a 500 ISE is appropriate. We could opt to catch the `InvalidCredentialsException` inside the `LdapUserProvider` and throw something more specific, to indicate it is the applications search bind credentials that are invalid, as opposed to the credentials the user provided during authentication. </details> <details> <summary>LDAP server reachable, with valid search bind credentials, authenticating as an unknown user</summary> As the search bind credentials are valid, the `LdapUserProvider` searches the LDAP directory, finds no entries, and throws a `UserNotFoundException` which results in the authenticator returning `{"code":401,"message":"Invalid credentials."}`. I believe this is good / expected behavior. </details> <details> <summary>LDAP server reachable, with valid search bind credentials, using known user with invalid password</summary> The `ldap/Adapter/ExtLdap/Connection.php` throws an `InvalidCredentialsException`. The `ldap/Security/CheckLdapCredentialsListener.php` now catches the `InvalidCredentialsException` (instead of the `ConnectionException`) and throws a `BadCredentialsException` such that the authenticator returns `{"code":401,"message":"Invalid credentials."}` </details> <details> <summary>LDAP server reachable, with valid search bind credentials, using known user with valid password</summary> The credentials are validated and the user is logged in, everything works as expected </details> Commits ------- ad2cc0e [Ldap] LDAP authentication should return a meaningful error when the LDAP server is unavailable
2 parents 1db1443 + ad2cc0e commit a4592e1

File tree

6 files changed

+47
-15
lines changed

‎src/Symfony/Component/Ldap/CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
CHANGELOG
22
=========
33

4+
6.1
5+
---
6+
7+
* Return a 500 Internal Server Error if LDAP server in unavailable during user enumeration / authentication
8+
* Introduce `InvalidSearchCredentialsException` to differentiate between cases where user-provided credentials are invalid and cases where the configured search credentials are invalid
9+
410
6.0
511
---
612

‎src/Symfony/Component/Ldap/Exception/InvalidSearchCredentialsException.php

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\Ldap\Exception;
13+
14+
/**
15+
* InvalidSearchCredentialsException is thrown if binding to ldap fails when
16+
* using the configured search_dn and search_password.
17+
*
18+
* @author Jeroen de Boer <info@jayfrown.nl>
19+
*/
20+
class InvalidSearchCredentialsException extends InvalidCredentialsException
21+
{
22+
}

‎src/Symfony/Component/Ldap/Security/CheckLdapCredentialsListener.php

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@
1313

1414
use Psr\Container\ContainerInterface;
1515
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
16-
use Symfony\Component\Ldap\Exception\ConnectionException;
16+
use Symfony\Component\Ldap\Exception\InvalidCredentialsException;
17+
use Symfony\Component\Ldap\Exception\InvalidSearchCredentialsException;
1718
use Symfony\Component\Ldap\LdapInterface;
1819
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
1920
use Symfony\Component\Security\Core\Exception\LogicException;
@@ -74,7 +75,11 @@ public function onCheckPassport(CheckPassportEvent $event)
7475
try {
7576
if ($ldapBadge->getQueryString()) {
7677
if ('' !== $ldapBadge->getSearchDn() && '' !== $ldapBadge->getSearchPassword()) {
77-
$ldap->bind($ldapBadge->getSearchDn(), $ldapBadge->getSearchPassword());
78+
try {
79+
$ldap->bind($ldapBadge->getSearchDn(), $ldapBadge->getSearchPassword());
80+
} catch (InvalidCredentialsException $e) {
81+
throw new InvalidSearchCredentialsException();
82+
}
7883
} else {
7984
throw new LogicException('Using the "query_string" config without using a "search_dn" and a "search_password" is not supported.');
8085
}
@@ -92,7 +97,7 @@ public function onCheckPassport(CheckPassportEvent $event)
9297
}
9398

9499
$ldap->bind($dn, $presentedPassword);
95-
} catch (ConnectionException $e) {
100+
} catch (InvalidCredentialsException $e) {
96101
throw new BadCredentialsException('The presented password is invalid.');
97102
}
98103

‎src/Symfony/Component/Ldap/Security/LdapUserProvider.php

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,9 @@
1212
namespace Symfony\Component\Ldap\Security;
1313

1414
use Symfony\Component\Ldap\Entry;
15-
use Symfony\Component\Ldap\Exception\ConnectionException;
1615
use Symfony\Component\Ldap\Exception\ExceptionInterface;
16+
use Symfony\Component\Ldap\Exception\InvalidCredentialsException;
17+
use Symfony\Component\Ldap\Exception\InvalidSearchCredentialsException;
1718
use Symfony\Component\Ldap\LdapInterface;
1819
use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
1920
use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
@@ -75,16 +76,14 @@ public function loadUserByIdentifier(string $identifier): UserInterface
7576
{
7677
try {
7778
$this->ldap->bind($this->searchDn, $this->searchPassword);
78-
$identifier = $this->ldap->escape($identifier, '', LdapInterface::ESCAPE_FILTER);
79-
$query = str_replace(['{username}', '{user_identifier}'], $identifier, $this->defaultSearch);
80-
$search = $this->ldap->query($this->baseDn, $query, ['filter' => 0 == \count($this->extraFields) ? '*' : $this->extraFields]);
81-
} catch (ConnectionException $e) {
82-
$e = new UserNotFoundException(sprintf('User "%s" not found.', $identifier), 0, $e);
83-
$e->setUserIdentifier($identifier);
84-
85-
throw $e;
79+
} catch (InvalidCredentialsException $e) {
80+
throw new InvalidSearchCredentialsException();
8681
}
8782

83+
$identifier = $this->ldap->escape($identifier, '', LdapInterface::ESCAPE_FILTER);
84+
$query = str_replace(['{username}', '{user_identifier}'], $identifier, $this->defaultSearch);
85+
$search = $this->ldap->query($this->baseDn, $query, ['filter' => 0 == \count($this->extraFields) ? '*' : $this->extraFields]);
86+
8887
$entries = $search->execute();
8988
$count = \count($entries);
9089

‎src/Symfony/Component/Ldap/Tests/Security/CheckLdapCredentialsListenerTest.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
use Symfony\Component\Ldap\Adapter\CollectionInterface;
1919
use Symfony\Component\Ldap\Adapter\QueryInterface;
2020
use Symfony\Component\Ldap\Entry;
21-
use Symfony\Component\Ldap\Exception\ConnectionException;
21+
use Symfony\Component\Ldap\Exception\InvalidCredentialsException;
2222
use Symfony\Component< CD89 /span>\Ldap\LdapInterface;
2323
use Symfony\Component\Ldap\Security\CheckLdapCredentialsListener;
2424
use Symfony\Component\Ldap\Security\LdapBadge;
@@ -133,7 +133,7 @@ public function testBindFailureShouldThrowAnException()
133133
$this->expectExceptionMessage('The presented password is invalid.');
134134

135135
$this->ldap->method('escape')->willReturnArgument(0);
136-
$this->ldap->expects($this->any())->method('bind')->willThrowException(new ConnectionException());
136+
$this->ldap->expects($this->any())->method('bind')->willThrowException(new InvalidCredentialsException());
137137

138138
$listener = $this->createListener();
139139
$listener->onCheckPassport($this->createEvent());

‎src/Symfony/Component/Ldap/Tests/Security/LdapUserProviderTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ class LdapUserProviderTest extends TestCase
2929
{
3030
public function testLoadUserByUsernameFailsIfCantConnectToLdap()
3131
{
32-
$this->expectException(UserNotFoundException::class);
32+
$this->expectException(ConnectionException::class);
3333

3434
$ldap = $this->createMock(LdapInterface::class);
3535
$ldap

0 commit comments

Comments
 (0)
0