Finish password expiration #224

Merged
merged 20 commits into from Sep 19, 2016

Conversation

Projects
None yet
4 participants
@grahamu
Contributor

grahamu commented Sep 10, 2016

This pull request completes password expiration functionality.

  • Add management command for setting user-specific password expiry.
  • Add management command for setting user password history.
  • Add documentation for password expiration.
  • Add Django admin configuration for PasswordExpiry, PasswordHistory models.
  • Add "password expired" message using contrib.messages.
  • Add middleware which checks for expired password.
  • Comprehensive tests.
Expand tests, fix logic, add docs
Add mgmt command for setting user-specific password expiry.
Add documentation for password expiry.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 10, 2016

Coverage Status

Coverage decreased (-0.5%) to 68.791% when pulling d4d0939 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 10, 2016

Coverage Status

Coverage decreased (-0.5%) to 68.791% when pulling d4d0939 on expiry-docs into 1a5cab5 on master.

Add mgmt commands and tests
Revise model migration.
Add user_password_history command.
Add management command tests.
Improve password tests.
Add Django admin functionality for new PasswordExpiry
and PasswordHistory models.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+1.05%) to 70.36% when pulling 23edd12 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+1.05%) to 70.36% when pulling 23edd12 on expiry-docs into 1a5cab5 on master.

grahamu added some commits Sep 13, 2016

Add ExpiredPasswordMiddleware
Use middleware to detect password expiration and redirect to
password change page if expired.
Added to CHANGELOG.md.
Removed outdated docs/changelog.rst.
Added documentation for password middleware.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.532% when pulling c9c73d8 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.532% when pulling c9c73d8 on expiry-docs into 1a5cab5 on master.

grahamu added some commits Sep 13, 2016

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.4%) to 72.727% when pulling ac43310 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.4%) to 72.727% when pulling ac43310 on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.532% when pulling 527b9cc on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.532% when pulling 527b9cc on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.532% when pulling 527b9cc on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.532% when pulling 527b9cc on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.532% when pulling 527b9cc on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.532% when pulling 527b9cc on expiry-docs into 1a5cab5 on master.

grahamu added some commits Sep 13, 2016

Add messages and signals
Add "Password is expired" message.
Add `password_expired` signal.
Update signals documentation.
Send signal for password expired
Improve verbose plural model name.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.3%) to 72.611% when pulling f69f798 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.3%) to 72.611% when pulling f69f798 on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.4%) to 72.708% when pulling 06c4309 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.4%) to 72.708% when pulling 06c4309 on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.553% when pulling 2954278 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.2%) to 72.553% when pulling 2954278 on expiry-docs into 1a5cab5 on master.

grahamu added some commits Sep 13, 2016

Add "?next=" when expired password detected
If middleware detects expired password, add "?next="
to redirect URL with expected page.
Add ACCOUNT_LOGOUT_URL
Improve creation of redirect URL with "next" value.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 40d419d on expiry-docs into 1a5cab5 on master.

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 40d419d on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 40d419d on expiry-docs into 1a5cab5 on master.

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 40d419d on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 40d419d on expiry-docs into 1a5cab5 on master.

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 40d419d on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 40d419d on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 40d419d on expiry-docs into 1a5cab5 on master.

@grahamu grahamu changed the title from WIP Expand password expiration tests, fix logic, add docs to Finish password expiration Sep 13, 2016

@grahamu

This comment has been minimized.

Show comment
Hide comment
@grahamu

grahamu Sep 13, 2016

Contributor

@brosner this is complete and ready for final thorough review, thanks!

Contributor

grahamu commented Sep 13, 2016

@brosner this is complete and ready for final thorough review, thanks!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 83c9efc on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 83c9efc on expiry-docs into 1a5cab5 on master.

@grahamu

This comment has been minimized.

Show comment
Hide comment
@grahamu

grahamu Sep 14, 2016

Contributor

Hmm, seems like we need management command documentation.

Contributor

grahamu commented Sep 14, 2016

Hmm, seems like we need management command documentation.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 14, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling b557fd7 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling b557fd7 on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 14, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling b557fd7 on expiry-docs into 1a5cab5 on master.

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling b557fd7 on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 14, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling b557fd7 on expiry-docs into 1a5cab5 on master.

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling b557fd7 on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 14, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling b557fd7 on expiry-docs into 1a5cab5 on master.

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling b557fd7 on expiry-docs into 1a5cab5 on master.

@grahamu

This comment has been minimized.

Show comment
Hide comment
@grahamu

grahamu Sep 14, 2016

Contributor

@brosner besides code review you can test this for real by cloning the latest cloudspotting "dua-password-expiry" branch, pinax/cloudspotting#14, currently found at:

pinax/cloudspotting@c7e4e53

Install and run cloudspotting as per docs (specifying latest hash of that branch), create a couple users, play around with management commands user_password_history and user_password_expiry, change ACCOUNT_PASSWORD_EXPIRY in settings.py to 60 (one minute), etc. Try to bust the system. Thanks.

Contributor

grahamu commented Sep 14, 2016

@brosner besides code review you can test this for real by cloning the latest cloudspotting "dua-password-expiry" branch, pinax/cloudspotting#14, currently found at:

pinax/cloudspotting@c7e4e53

Install and run cloudspotting as per docs (specifying latest hash of that branch), create a couple users, play around with management commands user_password_history and user_password_expiry, change ACCOUNT_PASSWORD_EXPIRY in settings.py to 60 (one minute), etc. Try to bust the system. Thanks.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 15, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 1f44952 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 15, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 1f44952 on expiry-docs into 1a5cab5 on master.

grahamu added some commits Sep 15, 2016

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 15, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 55ae160 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 15, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 55ae160 on expiry-docs into 1a5cab5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 15, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 1e0d672 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 15, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling 1e0d672 on expiry-docs into 1a5cab5 on master.

@grahamu

These areas would appreciate thoughtful consideration, thanks.

+ user.password_expiry.expiry = expire
+ user.password_expiry.save()
+
+ return "User \"{}\" password expiration set to {} seconds".format(username, expire)

This comment has been minimized.

@grahamu

grahamu Sep 15, 2016

Contributor

This handle_label method works fine for one or a handful of users, but doesn't scale nicely for a large userbase or for all users in a system. Should we improve this mechanism or leave be until someone complains?

@grahamu

grahamu Sep 15, 2016

Contributor

This handle_label method works fine for one or a handful of users, but doesn't scale nicely for a large userbase or for all users in a system. Should we improve this mechanism or leave be until someone complains?

This comment has been minimized.

@jacobwegner

jacobwegner Sep 15, 2016

Contributor

I'm not sure of the particulars of the use case driving these changes, but I think it'd be useful to set an expiration for all users if

  • adding the middleware to an existing project using DUA and wanting to set a "baseline" expiration for all existing users
  • enforcing a reset for all users in response to some kind of security breach

I don't have a strong opinion on that functionality being required from the get-go, and perhaps a new issue to track that functionality is useful and it can be added later when required.

@jacobwegner

jacobwegner Sep 15, 2016

Contributor

I'm not sure of the particulars of the use case driving these changes, but I think it'd be useful to set an expiration for all users if

  • adding the middleware to an existing project using DUA and wanting to set a "baseline" expiration for all existing users
  • enforcing a reset for all users in response to some kind of security breach

I don't have a strong opinion on that functionality being required from the get-go, and perhaps a new issue to track that functionality is useful and it can be added later when required.

This comment has been minimized.

@grahamu

grahamu Sep 19, 2016

Contributor

@jwegner your first point is handled by the global setting ACCOUNT_PASSWORD_EXPIRY, which is used by default if password expiration is enabled and the user does not have their own PasswordExpiry instance. Good thought on the second case, but I'd prefer to track the idea separately.

@grahamu

grahamu Sep 19, 2016

Contributor

@jwegner your first point is handled by the global setting ACCOUNT_PASSWORD_EXPIRY, which is used by default if password expiration is enabled and the user does not have their own PasswordExpiry instance. Good thought on the second case, but I'd prefer to track the idea separately.

account/middleware.py
+ messages.WARNING,
+ _("Your password has expired. Please save a new password.")
+ )
+ redirect_field_name = "next" # fragile!

This comment has been minimized.

@grahamu

grahamu Sep 15, 2016

Contributor

This redirect field name reference ("next") is quite fragile because if the project uses a different string this will break. Note that the redirect field name in classes protected by LoginRequiredMixin and methods protected by login_required default to "next" but can be overridden by class attribute or by invocation parameter. However this code is middleware with no way to set/reset the redirect field name.

To resolve this fragility I suggest using an account global, something like ACCOUNT_REDIRECT_FIELD_NAME.

@grahamu

grahamu Sep 15, 2016

Contributor

This redirect field name reference ("next") is quite fragile because if the project uses a different string this will break. Note that the redirect field name in classes protected by LoginRequiredMixin and methods protected by login_required default to "next" but can be overridden by class attribute or by invocation parameter. However this code is middleware with no way to set/reset the redirect field name.

To resolve this fragility I suggest using an account global, something like ACCOUNT_REDIRECT_FIELD_NAME.

This comment has been minimized.

@jacobwegner

jacobwegner Sep 15, 2016

Contributor

I agree with removing fragility and standardizing that field name.

We might consider relying on / aliasing from django.contrib.auth.REDIRECT_FIELD_NAME, but still allowing the the overrides on the existing classes/methods above.

So REDIRECT_FIELD_NAME is the default unless overridden.

@jacobwegner

jacobwegner Sep 15, 2016

Contributor

I agree with removing fragility and standardizing that field name.

We might consider relying on / aliasing from django.contrib.auth.REDIRECT_FIELD_NAME, but still allowing the the overrides on the existing classes/methods above.

So REDIRECT_FIELD_NAME is the default unless overridden.

@@ -28,7 +29,7 @@ class Migration(migrations.Migration):
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('password', models.CharField(max_length=255)),
- ('timestamp', models.DateTimeField(auto_now=True)),
+ ('timestamp', models.DateTimeField(default=django.utils.timezone.now)),

This comment has been minimized.

@grahamu

grahamu Sep 15, 2016

Contributor

auto_now=True does not work for timestamp field because we need to set the timestamp explicitly when creating PasswordHistory via management command.

@grahamu

grahamu Sep 15, 2016

Contributor

auto_now=True does not work for timestamp field because we need to set the timestamp explicitly when creating PasswordHistory via management command.

----
-
-* initial release
-* if migrating from Pinax; see :ref:`migration`

This comment has been minimized.

@grahamu

grahamu Sep 15, 2016

Contributor

This file appeared to be an orphan, superseded by /CHANGELOG.md.

@grahamu

grahamu Sep 15, 2016

Contributor

This file appeared to be an orphan, superseded by /CHANGELOG.md.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 15, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling d16099b on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 15, 2016

Coverage Status

Coverage increased (+3.5%) to 72.766% when pulling d16099b on expiry-docs into 1a5cab5 on master.

Use django.contrib.auth.REDIRECT_FIELD_NAME
Allow customization, but use REDIRECT_FIELD_NAME as default.
@grahamu

This comment has been minimized.

Show comment
Hide comment
@grahamu

grahamu Sep 19, 2016

Contributor

@jacobwegner how does this look with regard to next page redirect?

Contributor

grahamu commented Sep 19, 2016

@jacobwegner how does this look with regard to next page redirect?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 19, 2016

Coverage Status

Coverage increased (+3.4%) to 72.753% when pulling 32b7623 on expiry-docs into 1a5cab5 on master.

coveralls commented Sep 19, 2016

Coverage Status

Coverage increased (+3.4%) to 72.753% when pulling 32b7623 on expiry-docs into 1a5cab5 on master.

@brosner

I am happy with the direction here. This will be an excellent addition to DUA 2.0.

@brosner brosner merged commit c4a40be into master Sep 19, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+3.4%) to 72.753%
Details

@brosner brosner deleted the expiry-docs branch Sep 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment