LDAP authentication should return a meaningful error when the LDAP server is unavailable #44896
Conversation
- No longer catch ConnectionException so that it will result in a 500 ISE - Catch the InvalidCredentialsException so we can still throw a BadCredentialsException if the provided credentials are known to be invalid ref: symfony#44089
- No longer catch ConnectionException so that it will result in a 500 ISE - Expect a ConnectionException instead of a UserNotFoundException if the LDAP server is unreachable ref: symfony#44089
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
|
||
| throw $e; | ||
| } | ||
| $this->ldap->bind($this->searchDn, $this->searchPassword); |
There was a problem hiding this comment.
We could opt to catch InvalidCredentialsException here in order to throw something more-specific that would indicate it is the search bind credentials (used during enumeration) that are invalid, as opposed to the credentials that the user has provided.
|
Thanks for your investigation on this issue and potential fix, @Jayfrown! I'll try to set-up a test LDAP docker container to experiment with the changes in this PR (probably on Wednesday). For now, to answer your question:
Generally, we want to be very stable with released versions. Expected wrong behavior is often better than unexpected errors. So when in doubt, we tend to lean on the safe side and call "distinguishing connection errors from invalid password errors" a feature (thus 6.1). Generally, you should be able to use |
Return a 500 ISE if the LDAP server is unreachable.
Not sure if this is seen as a "bug fix" or a "new feature", or neither. It's not a bug per se, but it feels like wrong behavior to state that credentials are invalid if the LDAP server is unreachable.
I have tested the following scenarios:
LDAP server unreachable
Adapter/ExtLdap/Connection.phpthrows aConnectionExceptionwhich 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.LDAP server reachable, with invalid search bind credentials
The
LdapUserProvidertries to search the LDAP directory for the provided user. To search, it tries to authenticate using the (invalid) bind credentials, resulting inAdapter/ExtLdap/Connection.phpthrowing anInvalidCredentialsExceptionwhich 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
InvalidCredentialsExceptioninside theLdapUserProviderand 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.LDAP server reachable, with valid search bind credentials, authenticating as an unknown user
As the search bind credentials are valid, the
LdapUserProvidersearches the LDAP directory, finds no entries, and throws aUserNotFoundExceptionwhich results in the authenticator returning{"code":401,"message":"Invalid credentials."}. I believe this is good / expected behavior.LDAP server reachable, with valid search bind credentials, using known user with invalid password
The
ldap/Adapter/ExtLdap/Connection.phpthrows anInvalidCredentialsException. Theldap/Security/CheckLdapCredentialsListener.phpnow catches theInvalidCredentialsException(instead of theConnectionException) and throws aBadCredentialsExceptionsuch that the authenticator returns{"code":401,"message":"Invalid credentials."}LDAP server reachable, with valid search bind credentials, using known user with valid password
The credentials are validated and the user is logged in, everything works as expected