- Download Diff

Summary:

HUE-2010 [core] Configure Hue to terminate users who has not logged in X days

Review Request #4223 — Created March 4, 2014 and submitted March 21, 2014, 5:20 p.m.

Information
Owner:	abec
Repository:	old-hue-rw
Branch:
Bugs:	HUE-2010
Depends On:
Reviewers
Groups:	hue
People:	enricoberti, romain

Description

commit 77d0c4949faa229e44c9e98f2f950e11c84ff22d
Author: Abraham Elmahrek <abraham@elmahrek.com>
Date:   Tue Mar 4 15:03:12 2014 -0800

    HUE-2010 [core] Configure Hue to terminate users who has not logged in X days
    
    Added logic to AuthenticationForm.
    Made it so that all backends return a User object even if it is inactive.
    This make it easier to see the correct error message.
    Even superuser accounts are subject to this policy.

:100644 100644 f3b6aba... 58aa316... M  desktop/conf.dist/hue.ini
:100644 100644 8132770... 4b9055f... M  desktop/conf/pseudo-distributed.ini.tmpl
:100644 100644 286ecb5... 712d794... M  desktop/core/src/desktop/auth/backend.py
:100644 100644 9901e4c... 3ccb98f... M  desktop/core/src/desktop/auth/forms.py
:100644 100644 88f6a6f... b9da19f... M  desktop/core/src/desktop/auth/views.py
:100644 100644 0c56c6c... b599c77... M  desktop/core/src/desktop/auth/views_test.py
:100644 100644 eb06e85... 3b8771f... M  desktop/core/src/desktop/conf.py
:100644 100644 2135aae... 8199d2c... M  desktop/core/src/desktop/templates/login.mako

Testing Done

Tested new users and existing users. Set "expires_after" to 0 and saw that after creating a user, it was immediately disabled.
Also added a test.

Files

Loading file attachments...

Issues

2
0
5
1
All Issues: 8

Description	From	Last Updated
line never used?	romain	March 7, 2014, 2:18 p.m.
No easy way to have a 301?	romain	March 18, 2014, 1:56 p.m.
this looks like a big hack could we set the error in the form and raise a validation exception? (prefered) ...	romain	March 7, 2014, 2:19 p.m.
Probably should add a: # Apply only to AllowFirstUserDjangoBackend, LdapBackend	romain	March 21, 2014, 5:19 p.m.
nice!	romain	March 19, 2014, 8:36 a.m.
Account deactivated as inactive. This is simpler. Even maybe raise this message for 'inactive'	romain	March 21, 2014, 1:13 p.m.
test to update after	romain	March 21, 2014, 1:13 p.m.
probably to remove as not used and not i18ned	romain	March 21, 2014, 5:18 p.m.

Fix it!

Nice!

Just two questions: the error poping seems hacky, can't we re-use the standard form error?
Are we ok for disabling super user too?

abec March 18, 2014, 2:21 p.m.

Added a switch for disabling superusers.

desktop/core/src/desktop/auth/forms.py (Diff revision 1)
Show all issues
```
line never used?
```
1. abec March 18, 2014, 2:21 p.m.
  It is! In the parent class!
desktop/core/src/desktop/auth/views_test.py (Diff revision 1)
Show all issues
```
No easy way to have a 301?
```
1. abec March 18, 2014, 2:21 p.m.
  Shouldn't it be 200 since the page we're accessing is the login page?

desktop/core/src/desktop/templates/login.mako (Diff revision 1)

Show all issues

this looks like a big hack

could we set the error in the form and raise a validation exception? (prefered)

or check user.is_active if it works

abec March 18, 2014, 2:21 p.m.

We are setting the exception in the form. It looks like we just need a better representation of errors.

Diff:

Revision 2 (+152 -14)

Show changes

	desktop/conf/pseudo-distributed.ini.tmpl
	desktop/conf.dist/hue.ini
	desktop/core/src/desktop/conf.py
	desktop/core/src/desktop/auth/backend.py
	desktop/core/src/desktop/auth/forms.py
	desktop/core/src/desktop/auth/views.py
	desktop/core/src/desktop/auth/views_test.py
	desktop/core/src/desktop/lib/python_util.py
	2 more

Added Files:

Ship it!

```
Ship It!
```

Ship it! Fix it!

Oups, clicked by mistake!

Pretty good, just a few comments!

desktop/conf/pseudo-distributed.ini.tmpl (Diff revision 2)

Show all issues

Probably should add a:

# Apply only to AllowFirstUserDjangoBackend, LdapBackend

abec March 19, 2014, 11:12 a.m.

This should apply to all backends. I think the Ldap and AllowFirstUser backend required changes to get this working though. Maybe more testing for other backends?

romain March 19, 2014, 12:38 p.m.
```
Ok, good as is so!
```

desktop/core/src/desktop/auth/forms.py (Diff revision 2)
Show all issues
```
nice!
```

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

Would it make sense to add a help like:

Please contact an <a>admin</a> (mailto link with first admin from settings.ADMINS?)

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

Show all issues

Account deactivated as inactive.

This is simpler.

Even maybe raise this message for 'inactive'

abec March 19, 2014, 11:12 a.m.

How about "Account deactivated. Please contact an administrator." Provide links to the administrator as defined above.

romain March 19, 2014, 12:38 p.m.
```
I like it!
```

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

Show all issues

test to update after

abec March 19, 2014, 11:12 a.m.

Is the user experience we're expecting here the following? If an account has been deactivated, then reactivate with a successful login? Shouldn't we have admins re-activate users? Or do you mean to test that the user is actually deactivated?

romain March 19, 2014, 12:38 p.m.

I like the  "Account deactivated. Please contact an administrator." + link. The user does not need to know why I would think

desktop/core/src/desktop/lib/python_util.py (Diff revision 2)
Show all issues
```
probably to remove as not used and not i18ned
```
1. abec March 19, 2014, 11:12 a.m.
  timedelta_to_string is used in auth/forms.py.
2. abec March 21, 2014, 5:19 p.m.
  I removed it :)

You have a pending review.

Review Board 3.0.18

HUE-2010 [core] Configure Hue to terminate users who has not logged in X days

Diff:

Added Files:

Status: Closed (submitted)