HUE-7624 [core] Support multi-authentication with AllowFirstUserDjangoBackend and LdapBackend

Review Request #12028 - Created Nov. 15, 2017 and submitted

Ying Chen
hue
master
HUE-7624
hue
enricoberti, jgauthier, johan, ranade, romain, weixia
commit 86bb58527d3009844ab0ceec2a91716b2856e7f1 
Author: Ying Chen <yingchen@cloudera.com>
Date:   Tue Nov 14 18:55:12 2017 -0800

    HUE-7624 [core] Support multi-authentication with AllowFirstUserDjangoBackend and LdapBackend

:100644 100644 d29cf06928... b1747a5faf... M    desktop/core/src/desktop/auth/forms.py
:100644 100644 0ba80148cf... 06659f224a... M    desktop/core/src/desktop/auth/views.py
:100644 100644 80c0318ee2... d38cfcf346... M    desktop/core/src/desktop/auth/views_test.py


  • 0
  • 0
  • 11
  • 0
  • 11
Description From Last Updated
  1. Better commit message?

    e.g.

    [core] Support multi-authentication with AllowFirstUserDjangoBackend and LdapBackend

  2. desktop/core/src/desktop/auth/views.py (Diff revision 2)
     
     

    Could we create a new variable to explain more what we are checking for?

    What are we trying to do here? (any change to only require a request.POST.get('server') == 'LDAP')

    e.g.

    is_ldap_multi_backend = 'server' not in request.POST or request.POST['server'] == 'LDAP'):

    What are the possible values for request.POST['server']?

    1. When the very first time loading login page 'server' is not in request.POST. And LDAP is selected by default.
  3. desktop/core/src/desktop/auth/views.py (Diff revision 2)
     
     
     

    Is there a way to move the logic to get_server_choices() ?

  4. 
      
  1. Nice, getting much cleaner!

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

    Don't we already have a get_backend_names() function cf. the other file?

    1. Can't import from auth.views because of mutual imports. Move it from auth.views to auth.forms
  3. desktop/core/src/desktop/auth/forms.py (Diff revision 3)
     
     

    list is a builtin Python, use 'auth_choice'?

  4. desktop/core/src/desktop/auth/forms.py (Diff revision 3)
     
     
     

    Create a helper methode so that we don't duplicate the logic in the other file?

  5. 
      
  1. Really nice!!

    Do you think we could add a new test class to test the multi auth?

    e.g. def test_login(self)
    with ('AllowFirstUserDjangoBackend', 'LdapBackend')
    https://github.com/cloudera/hue/blob/master/desktop/core/src/desktop/auth/views_test.py#L172

    That way we know if we regress later.

  2. desktop/core/src/desktop/auth/forms.py (Diff revision 5)
     
     

    nit: newline before?

  3. 
      
  1. Really nice!! Just some nits and one question

  2. Is this new tests or can be removed?

  3. 'mb' or just 'm' or 'multiple'? (clearer)

  4. desktop/core/src/desktop/auth/views_test.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
  5. 
      
  1. Really nice!

  2. Nit: space after ,

  3. 
      
Review request changed

Status: Closed (submitted)

Loading...