HUE-7989 [useradmin] Provide better UI message and message in logs when ldap server down

Review Request #12532 - Created Feb. 14, 2018 and submitted

Ying Chen
enricoberti, jgauthier, johan, ranade, romain, weixia
commit 757b3091ee7ea5dfc0bbf47c03cbeeed5f6b5fdd (HEAD -> HUE-7989-ldapDown)
Author: Ying Chen <>
Date:   Wed Feb 14 14:23:15 2018 -0800

    HUE-7989 [useradmin] Provide better UI message and message in logs when ldap server down

:100644 100644 ff50bb3d8a... 5e53932d4e... M    apps/useradmin/src/useradmin/
:100644 100644 d39cdbc33b... 2c5b59c162... M    desktop/core/src/desktop/auth/

  • 0
  • 0
  • 4
  • 5
  • 9
Description From Last Updated
  1. Put a few hints, the idea would be to contain it to the LdapBackend or LdapBackend form?

  2. desktop/core/ext-py/Django-1.6.10/django/contrib/auth/ (Diff revision 1)

    We should avoid to update the core libs

  3. desktop/core/src/desktop/auth/ (Diff revision 1)

    Here, couldn't we do a custom LDAP health check call?

    If the call fails, we raise a custom exception: LDAP... might be down or not configured

    If it is fine, we raise the usal 'invalid login' message?

    Might even be cleaner in desktop.auth.backends LdapBackend if it is possible there

    1. Without raise LDAPError from django_auth_ldap.backend (core libs) for server down message, desktop.auth.backends LdapBackend won't fail and will always get None for returned user object, so it's hard to find out whether server down or invalid login.
  4. desktop/core/src/desktop/templates/login.mako (Diff revision 1)

    Ideally we should not need to change anything, and have the same logic as the 'invalid password'

    1. Currently, invalid password seems broken, it redirected to "next", error message got washed. Need to debug why.

  1. Nice!

  2. desktop/core/src/desktop/auth/ (Diff revision 3)

    raise ValidationError(_("LDAP %s Error: %s" % (server_key, e.message.get(desc) or e.message)))

    would work?

    1. If Exception is other than LDAPError like LdapBindException, the e.message is a string and it will cause exception.
  1. Last 2 things to be sure

  2. Add a comment that this is for bypassing? (as the name of the class is very similar to the official LDAPError)

  3. apps/useradmin/src/useradmin/ (Diff revision 4)

    Are we still going to allow to go in the 'except:'?

    If we just re-raise e do we still need the LdapError error class?

  2. nit: no need of 'Exception, e'

    1. I want to log exception in handle_bind_exception(self, exception, bind_user=None) for supportability.
  2. Indeed, I missed the variable e somehow

Review request changed

Status: Closed (submitted)