HUE2533 [useradmin] Only show information updated when it is true

Review Request #9474 - Created Feb. 3, 2017 and submitted

Adrian Yavorskyy
hue
master
HUE-2533
hue
enricoberti, erickt, jennykim, krish, ranade, romain, weixia
commit 69a6c2f2f889ef2035d93eebfcf33e256d7ec150
Author: Adrian Yavorskyy <ayav@softserveinc.com>
Date:   Fri Feb 3 12:55:39 2017 +0200

    HUE2533 [useradmin] Do not accept blank password. Instead show warning message

:100644 100644 538a5ef... 8e85273... M	apps/useradmin/src/useradmin/views.py

Manual on Chrome

  • 6
  • 0
  • 3
  • 0
  • 9
Description From Last Updated
We should do the check only when we are doing a password change. Also the check should happen before the ... Romain Rigaux
One clean way would be to somehow reuse https://github.com/cloudera/hue/blob/master/apps/useradmin/src/useradmin/password_policy.py or do the check here. Romain Rigaux
One clean way would be to somehow reuse https://github.com/cloudera/hue/blob/master/apps/useradmin/src/useradmin/password_policy.py or do the check here. Romain Rigaux
Could we keep this true? (in 99% we want to create the home of the users) Romain Rigaux
Why would we need this one? Is it because we only offer it for the current user? https://github.com/cloudera/hue/blob/master/apps/useradmin/src/useradmin/templates/edit_user.mako#L80 Romain Rigaux
Is there a way to just have: if updated: request.info(_('User information updated')) here and move upt the logic into 'updated' ... Romain Rigaux
  1. 
      
  2. apps/useradmin/src/useradmin/views.py (Diff revision 1)
     
     

    Would it make sense to raise a PopupException instead of just a warning so that it is not accepted for real?

  3. 
      
  1. 
      
    1. I think that the problem is more general(not only about changing a password): when you are on user editing page and you don't change anything, but press "update user" - "User information message" will still appear. Maybe the right solution would be to track if at least one of user's properties is changed and then show info message. In case if nothing changed don't show the message or notify user that nothing was changed.

  2. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     

    We should do the check only when we are doing a password change.

    Also the check should happen before the form.save()

  3. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     

    One clean way would be to somehow reuse https://github.com/cloudera/hue/blob/master/apps/useradmin/src/useradmin/password_policy.py

    or do the check here.

  4. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     

    One clean way would be to somehow reuse https://github.com/cloudera/hue/blob/master/apps/useradmin/src/useradmin/password_policy.py

    or do the check here.

    1. Thanks for remark about doing the check only when we are doing a password change, I haven't considered that.
      I have read the comments above, but still having one question: how to know that for 100% that user is going to change the password ? My assumption is that if user fills in "current password" field in the form that means that he is going to change his password. So, based on this I can check passwords and raise exception in case new password is empty and current password is not empty. Is this a good idea?

    2. I have written comment above without knowing that for changing simple users'(not superuser) passwords you don't have enter current password. So, the way I thought to do will not work.

    3. Simplest is probably to have a password policy by default that forbids blank password:
      https://github.com/cloudera/hue/blob/master/apps/useradmin/src/useradmin/conf.py#L45

      (we can still keep the examples of regexp in the hue.ini as an example of set )

  5. 
      
  1. This is good and let me know if you want to change the default policy?

    1. By changing default policy you mean to keep it enabled by default or to change default regexp? I think it should be enabled by default.

  2. 
      
  1. I meant, we could update the conf.py to have these values, and also update hue.ini and https://github.com/cloudera/hue/blob/master/desktop/conf/pseudo-distributed.ini.tmpl#L1264 too to show something like this?

    [[password_policy]]
    # Set password policy to all users. The default policy requires password to be at least 8 characters long,
    # and contain both uppercase and lowercase letters, numbers, and special characters.

    # No blank password allowed
    is_enabled=true
    pwd_regex="^.+$"
    pwd_hint="The password is empty."
    pwd_error_message="The password must not be blank."
    
    # Complex password required
    ## is_enabled=false
    ## pwd_regex="^(?=.*?[A-Z])(?=(.*[a-z]){1,})(?=(.*[\d]){1,})(?=(.*[\W_]){1,}).{8,}$"
    ## pwd_hint="The password must be at least 8 characters long, and must contain both uppercase and lowercase letters, at least one number, and at least one special character."
    ## pwd_error_message="The password must be at least 8 characters long, and must contain both uppercase and lowercase letters, at least one number, and at least one special character."
    
  2. 
      
  1. Looks good! Could you attached a rebased patch or send a PR?

  2. 
      
  1. 
      
  2. apps/useradmin/src/useradmin/forms.py (Diff revision 6)
     
     

    Could we keep this true?

    (in 99% we want to create the home of the users)

    1. We could keep this true, but that means that almost everytime we press "update user"(without checking create home directory) we will have ensure_home_directory in form.changed_data, because it's initial value was true. As I understand we usually once create home directory, not a lot of times.

  3. apps/useradmin/src/useradmin/views.py (Diff revision 6)
     
     

    Why would we need this one?

    Is it because we only offer it for the current user?
    https://github.com/cloudera/hue/blob/master/apps/useradmin/src/useradmin/templates/edit_user.mako#L80

    1. If a superuser changes info about user, form.changed_data will also have 'language'.

  4. 
      
  1. 
      
  2. apps/useradmin/src/useradmin/views.py (Diff revision 7)
     
     

    To be clean, we should have only one printing like:

    if updated:
    request.info(_('User information updated'))

    and move up the 'updated' logic.

    1. I've updated the review. Please check to see if I understood you correctly.

  3. 
      
  1. 
      
  2. apps/useradmin/src/useradmin/views.py (Diff revision 7)
     
     
     
     
     
     
     
     
     

    Is there a way to just have:

    if updated:
    request.info(_('User information updated'))

    here and move upt the logic into 'updated' higher up?

    1. Something similar I was trying to do in https://review.cloudera.org/r/9474/diff/8/. Did you look into it?

  3. 
      
  1. 
      
  2. apps/useradmin/src/useradmin/views.py (Diff revision 8)
     
     

    Nit: bit shorter could be:

    updated = ...

  3. 
      
  1. Ship It!
    1. Finally ! :) A patch is attached to a jira !

  2. 
      
Review request changed

Status: Closed (submitted)

Loading...