-
Notifications
You must be signed in to change notification settings - Fork 354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Finish password expiration #224
Changes from 18 commits
d4d0939
23edd12
c9c73d8
d20a523
c2b3316
ac43310
527b9cc
f69f798
06c4309
2954278
127613f
3108ddb
40d419d
83c9efc
b557fd7
1f44952
55ae160
1e0d672
d16099b
32b7623
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
__version__ = "2.0.0.dev1" | ||
__version__ = "2.0.0.dev2" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
from django.contrib.auth import get_user_model | ||
from django.core.management.base import LabelCommand | ||
|
||
from account.conf import settings | ||
from account.models import PasswordExpiry | ||
|
||
|
||
class Command(LabelCommand): | ||
|
||
help = "Create user-specific password expiration period." | ||
label = "username" | ||
|
||
def add_arguments(self, parser): | ||
super(Command, self).add_arguments(parser) | ||
parser.add_argument( | ||
"-e", "--expire", | ||
type=int, | ||
nargs="?", | ||
default=settings.ACCOUNT_PASSWORD_EXPIRY, | ||
help="number of seconds until password expires" | ||
) | ||
|
||
def handle_label(self, username, **options): | ||
User = get_user_model() | ||
try: | ||
user = User.objects.get(username=username) | ||
except User.DoesNotExist: | ||
return "User \"{}\" not found".format(username) | ||
|
||
expire = options["expire"] | ||
|
||
# Modify existing PasswordExpiry or create new if needed. | ||
if not hasattr(user, "password_expiry"): | ||
PasswordExpiry.objects.create(user=user, expiry=expire) | ||
else: | ||
user.password_expiry.expiry = expire | ||
user.password_expiry.save() | ||
|
||
return "User \"{}\" password expiration set to {} seconds".format(username, expire) | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import datetime | ||
import pytz | ||
|
||
from django.contrib.auth import get_user_model | ||
from django.core.management.base import BaseCommand | ||
|
||
from account.models import PasswordHistory | ||
|
||
|
||
class Command(BaseCommand): | ||
|
||
help = "Create password history for all users without existing history." | ||
|
||
def add_arguments(self, parser): | ||
parser.add_argument( | ||
"-d", "--days", | ||
type=int, | ||
nargs="?", | ||
default=10, | ||
help="age of current password (in days)" | ||
) | ||
parser.add_argument( | ||
"-f", "--force", | ||
action="store_true", | ||
help="create new password history for all users, regardless of existing history" | ||
) | ||
|
||
def handle(self, *args, **options): | ||
User = get_user_model() | ||
users = User.objects.all() | ||
if not options["force"]: | ||
users = users.filter(password_history=None) | ||
|
||
if not users: | ||
return "No users found without password history" | ||
|
||
days = options["days"] | ||
timestamp = datetime.datetime.now(tz=pytz.UTC) - datetime.timedelta(days=days) | ||
|
||
# Create new PasswordHistory on `timestamp` | ||
PasswordHistory.objects.bulk_create( | ||
[PasswordHistory(user=user, timestamp=timestamp) for user in users] | ||
) | ||
|
||
return "Password history set to {} for {} users".format(timestamp, len(users)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,21 @@ | ||
from __future__ import unicode_literals | ||
|
||
try: | ||
from urllib.parse import urlparse, urlunparse | ||
except ImportError: # python 2 | ||
from urlparse import urlparse, urlunparse | ||
|
||
from django.contrib import messages | ||
from django.core.urlresolvers import resolve, reverse | ||
from django.http import HttpResponseRedirect, QueryDict | ||
from django.utils import translation, timezone | ||
from django.utils.cache import patch_vary_headers | ||
from django.utils.translation import ugettext_lazy as _ | ||
|
||
from account import signals | ||
from account.conf import settings | ||
from account.models import Account | ||
from account.utils import check_password_expired | ||
|
||
|
||
class LocaleMiddleware(object): | ||
|
@@ -51,3 +62,32 @@ def process_request(self, request): | |
if account: | ||
tz = settings.TIME_ZONE if not account.timezone else account.timezone | ||
timezone.activate(tz) | ||
|
||
|
||
class ExpiredPasswordMiddleware(object): | ||
|
||
def process_request(self, request): | ||
if request.user.is_authenticated() and not request.user.is_staff: | ||
next_url = resolve(request.path).url_name | ||
# Authenticated users must be allowed to access | ||
# "change password" page and "log out" page. | ||
# even if password is expired. | ||
if next_url not in [settings.ACCOUNT_PASSWORD_CHANGE_REDIRECT_URL, | ||
settings.ACCOUNT_LOGOUT_URL, | ||
]: | ||
if check_password_expired(request.user): | ||
signals.password_expired.send(sender=self, user=request.user) | ||
messages.add_message( | ||
request, | ||
messages.WARNING, | ||
_("Your password has expired. Please save a new password.") | ||
) | ||
redirect_field_name = "next" # fragile! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 To resolve this fragility I suggest using an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with removing fragility and standardizing that field name. We might consider relying on / aliasing from So |
||
|
||
change_password_url = reverse(settings.ACCOUNT_PASSWORD_CHANGE_REDIRECT_URL) | ||
url_bits = list(urlparse(change_password_url)) | ||
querystring = QueryDict(url_bits[4], mutable=True) | ||
querystring[redirect_field_name] = next_url | ||
url_bits[4] = querystring.urlencode(safe="/") | ||
|
||
return HttpResponseRedirect(urlunparse(url_bits)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
# -*- coding: utf-8 -*- | ||
# Generated by Django 1.9.7 on 2016-09-01 17:50 | ||
# Generated by Django 1.10.1 on 2016-09-13 08:55 | ||
from __future__ import unicode_literals | ||
|
||
from django.conf import settings | ||
from django.db import migrations, models | ||
import django.db.models.deletion | ||
import django.utils.timezone | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
@@ -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)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='password_history', to=settings.AUTH_USER_MODEL)), | ||
], | ||
), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
# empty for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.