From e685f8e8192c41b2ca28897cd9de1a9d3f76f5c8 Mon Sep 17 00:00:00 2001 From: Jason Estibeiro Date: Wed, 11 May 2016 17:08:31 +0530 Subject: [PATCH] Added basic Django password validations and updated .gitignore (#136) --- .gitignore | 2 + src/pretix/base/forms/auth.py | 28 +++++- src/pretix/base/forms/user.py | 12 +++ src/pretix/control/views/auth.py | 3 +- src/pretix/settings.py | 15 +++ src/tests/control/test_auth.py | 166 ++++++++++++++++++++++++++++--- src/tests/control/test_user.py | 26 ++++- 7 files changed, 232 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index 426adf1593..9719c0bcde 100644 --- a/.gitignore +++ b/.gitignore @@ -18,4 +18,6 @@ _static/ .secret atlassian-ide-plugin.xml pretixeu/ +.project +.pydevproject diff --git a/src/pretix/base/forms/auth.py b/src/pretix/base/forms/auth.py index d5ec586b55..ec94473f35 100644 --- a/src/pretix/base/forms/auth.py +++ b/src/pretix/base/forms/auth.py @@ -1,5 +1,8 @@ from django import forms from django.contrib.auth import authenticate +from django.contrib.auth.password_validation import ( + password_validators_help_texts, validate_password, +) from django.utils.translation import ugettext_lazy as _ from pretix.base.models import User @@ -84,7 +87,7 @@ class RegistrationForm(forms.Form): ) def clean(self): - password1 = self.cleaned_data.get('password') + password1 = self.cleaned_data.get('password', '') password2 = self.cleaned_data.get('password_repeat') if password1 and password1 != password2: @@ -93,6 +96,12 @@ class RegistrationForm(forms.Form): code='pw_mismatch' ) + user = User(email=self.cleaned_data.get('email')) + if validate_password(password1, user=user) is not None: + raise forms.ValidationError( + _(password_validators_help_texts()), + code='pw_invalid' + ) return self.cleaned_data def clean_email(self): @@ -107,7 +116,7 @@ class RegistrationForm(forms.Form): class PasswordRecoverForm(forms.Form): error_messages = { - 'pw_mismatch': _("Please enter the same password twice") + 'pw_mismatch': _("Please enter the same password twice"), } password = forms.CharField( label=_('Password'), @@ -119,11 +128,12 @@ class PasswordRecoverForm(forms.Form): widget=forms.PasswordInput ) - def __init__(self, *args, **kwargs): + def __init__(self, user_id=None, *args, **kwargs): + self.user_id = user_id super().__init__(*args, **kwargs) def clean(self): - password1 = self.cleaned_data.get('password') + password1 = self.cleaned_data.get('password', '') password2 = self.cleaned_data.get('password_repeat') if password1 and password1 != password2: @@ -132,6 +142,16 @@ class PasswordRecoverForm(forms.Form): code='pw_mismatch' ) + try: + user = User.objects.get(id=self.user_id) + except User.DoesNotExist: + user = None + if validate_password(password1, user=user) is not None: + raise forms.ValidationError( + _(password_validators_help_texts()), + code='pw_invalid' + ) + return self.cleaned_data diff --git a/src/pretix/base/forms/user.py b/src/pretix/base/forms/user.py index 0ff68895a3..c5d56dda6c 100644 --- a/src/pretix/base/forms/user.py +++ b/src/pretix/base/forms/user.py @@ -1,5 +1,8 @@ from django import forms from django.contrib.auth.hashers import check_password +from django.contrib.auth.password_validation import ( + password_validators_help_texts, validate_password, +) from django.db.models import Q from django.utils.translation import ugettext_lazy as _ @@ -66,6 +69,15 @@ class UserSettingsForm(forms.ModelForm): ) return email + def clean_new_pw(self): + password1 = self.cleaned_data.get('new_pw', '') + if password1 and validate_password(password1, user=self.user) is not None: + raise forms.ValidationError( + _(password_validators_help_texts()), + code='pw_invalid' + ) + return password1 + def clean_new_pw_repeat(self): password1 = self.cleaned_data.get('new_pw') password2 = self.cleaned_data.get('new_pw_repeat') diff --git a/src/pretix/control/views/auth.py b/src/pretix/control/views/auth.py index b83f0b1881..402c009fbb 100644 --- a/src/pretix/control/views/auth.py +++ b/src/pretix/control/views/auth.py @@ -161,7 +161,8 @@ class Recover(TemplateView): @cached_property def form(self): - return PasswordRecoverForm(data=self.request.POST if self.request.method == 'POST' else None) + return PasswordRecoverForm(data=self.request.POST if self.request.method == 'POST' else None, + user_id=self.request.GET.get('id')) def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) diff --git a/src/pretix/settings.py b/src/pretix/settings.py index 3ff04429f2..2235469fbb 100644 --- a/src/pretix/settings.py +++ b/src/pretix/settings.py @@ -388,3 +388,18 @@ CELERY_RESULT_SERIALIZER = 'pickle' BOOTSTRAP3 = { 'success_css_class': '' } + +AUTH_PASSWORD_VALIDATORS = [ + { + 'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator', + }, + { + 'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', + }, + { + 'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator', + }, + { + 'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator', + }, +] diff --git a/src/tests/control/test_auth.py b/src/tests/control/test_auth.py index dde5672fd8..64bd82d387 100644 --- a/src/tests/control/test_auth.py +++ b/src/tests/control/test_auth.py @@ -114,20 +114,88 @@ class RegistrationFormTest(TestCase): }) self.assertEqual(response.status_code, 200) + def test_user_attribute_similarity_passwords(self): + response = self.client.post('/control/register', { + 'email': 'dummy@dummy.dummy', + 'password': 'dummydummy', + 'password_repeat': 'dummydummy' + }) + self.assertEqual(response.status_code, 200) + + def test_short_passwords(self): + response = self.client.post('/control/register', { + 'email': 'dummy@dummy.dummy', + 'password': 'foobar', + 'password_repeat': 'foobar' + }) + self.assertEqual(response.status_code, 200) + + def test_common_passwords(self): + response = self.client.post('/control/register', { + 'email': 'dummy@dummy.dummy', + 'password': 'password', + 'password_repeat': 'password' + }) + self.assertEqual(response.status_code, 200) + + response = self.client.post('/control/register', { + 'email': 'dummy@dummy.dummy', + 'password': 'football', + 'password_repeat': 'football' + }) + self.assertEqual(response.status_code, 200) + + response = self.client.post('/control/register', { + 'email': 'dummy@dummy.dummy', + 'password': 'jennifer', + 'password_repeat': 'jennifer' + }) + self.assertEqual(response.status_code, 200) + + def test_numeric_passwords(self): + response = self.client.post('/control/register', { + 'email': 'dummy@dummy.dummy', + 'password': '12345678', + 'password_repeat': '12345678' + }) + self.assertEqual(response.status_code, 200) + + response = self.client.post('/control/register', { + 'email': 'dummy@dummy.dummy', + 'password': '23423523452345235', + 'password_repeat': '23423523452345235' + }) + self.assertEqual(response.status_code, 200) + + def test_empty_passwords(self): + response = self.client.post('/control/register', { + 'email': 'dummy@dummy.dummy', + 'password': '', + 'password_repeat': '' + }) + self.assertEqual(response.status_code, 200) + + response = self.client.post('/control/register', { + 'email': 'dummy@dummy.dummy', + 'password': 'foobarbar', + 'password_repeat': '' + }) + self.assertEqual(response.status_code, 200) + def test_email_duplicate(self): self.user = User.objects.create_user('dummy@dummy.dummy', 'dummy') response = self.client.post('/control/register', { 'email': 'dummy@dummy.dummy', - 'password': 'foo', - 'password_repeat': 'foo' + 'password': 'foobarbar', + 'password_repeat': 'foobarbar' }) self.assertEqual(response.status_code, 200) def test_success(self): response = self.client.post('/control/register', { 'email': 'dummy@dummy.dummy', - 'password': 'foo', - 'password_repeat': 'foo' + 'password': 'foobarbar', + 'password_repeat': 'foobarbar' }) self.assertEqual(response.status_code, 302) @@ -175,8 +243,8 @@ class PasswordRecoveryFormTest(TestCase): response = self.client.post( '/control/forgot/recover?id=%d&token=foo' % self.user.id, { - 'password': 'foobar', - 'password_repeat': 'foobar' + 'password': 'foobarbar', + 'password_repeat': 'foobarbar' } ) self.assertEqual(response.status_code, 302) @@ -197,8 +265,8 @@ class PasswordRecoveryFormTest(TestCase): response = self.client.post( '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token), { - 'password': 'foobar', - 'password_repeat': 'foobar' + 'password': 'foobarbar', + 'password_repeat': 'foobarbar' } ) self.assertEqual(response.status_code, 302) @@ -212,15 +280,29 @@ class PasswordRecoveryFormTest(TestCase): response = self.client.post( '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token), { - 'password': 'foobar', - 'password_repeat': 'foobar' + 'password': 'foobarbar', + 'password_repeat': 'foobarbar' } ) self.assertEqual(response.status_code, 302) self.user = User.objects.get(id=self.user.id) - self.assertTrue(self.user.check_password('foobar')) + self.assertTrue(self.user.check_password('foobarbar')) def test_recovery_valid_token_empty_passwords(self): + token = default_token_generator.make_token(self.user) + response = self.client.get('/control/forgot/recover?id=%d&token=%s' % (self.user.id, token)) + self.assertEqual(response.status_code, 200) + response = self.client.post( + '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token), + { + 'password': 'foobarbar', + 'password_repeat': '' + } + ) + self.assertEqual(response.status_code, 200) + self.user = User.objects.get(id=self.user.id) + self.assertTrue(self.user.check_password('demo')) + token = default_token_generator.make_token(self.user) response = self.client.get('/control/forgot/recover?id=%d&token=%s' % (self.user.id, token)) self.assertEqual(response.status_code, 200) @@ -228,7 +310,7 @@ class PasswordRecoveryFormTest(TestCase): '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token), { 'password': '', - 'password_repeat': 'foobar' + 'password_repeat': 'foobarbar' } ) self.assertEqual(response.status_code, 200) @@ -249,3 +331,63 @@ class PasswordRecoveryFormTest(TestCase): self.assertEqual(response.status_code, 200) self.user = User.objects.get(id=self.user.id) self.assertTrue(self.user.check_password('demo')) + + def test_recovery_valid_token_user_attribute_similarity_passwords(self): + token = default_token_generator.make_token(self.user) + response = self.client.get('/control/forgot/recover?id=%d&token=%s' % (self.user.id, token)) + self.assertEqual(response.status_code, 200) + response = self.client.post( + '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token), + { + 'password': 'dummydemo', + 'password_repeat': 'dummydemo' + } + ) + self.assertEqual(response.status_code, 200) + self.user = User.objects.get(id=self.user.id) + self.assertTrue(self.user.check_password('demo')) + + def test_recovery_valid_token_short_passwords(self): + token = default_token_generator.make_token(self.user) + response = self.client.get('/control/forgot/recover?id=%d&token=%s' % (self.user.id, token)) + self.assertEqual(response.status_code, 200) + response = self.client.post( + '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token), + { + 'password': 'foobar', + 'password_repeat': 'foobar' + } + ) + self.assertEqual(response.status_code, 200) + self.user = User.objects.get(id=self.user.id) + self.assertTrue(self.user.check_password('demo')) + + def test_recovery_valid_token_common_passwords(self): + token = default_token_generator.make_token(self.user) + response = self.client.get('/control/forgot/recover?id=%d&token=%s' % (self.user.id, token)) + self.assertEqual(response.status_code, 200) + response = self.client.post( + '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token), + { + 'password': 'football', + 'password_repeat': 'football' + } + ) + self.assertEqual(response.status_code, 200) + self.user = User.objects.get(id=self.user.id) + self.assertTrue(self.user.check_password('demo')) + + def test_recovery_valid_token_numeric_passwords(self): + token = default_token_generator.make_token(self.user) + response = self.client.get('/control/forgot/recover?id=%d&token=%s' % (self.user.id, token)) + self.assertEqual(response.status_code, 200) + response = self.client.post( + '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token), + { + 'password': '12345678', + 'password_repeat': '12345678' + } + ) + self.assertEqual(response.status_code, 200) + self.user = User.objects.get(id=self.user.id) + self.assertTrue(self.user.check_password('demo')) diff --git a/src/tests/control/test_user.py b/src/tests/control/test_user.py index 207516f22f..b990df31de 100644 --- a/src/tests/control/test_user.py +++ b/src/tests/control/test_user.py @@ -67,13 +67,33 @@ class UserSettingsTest(BrowserTest): assert self.user.password == pw def test_change_password_success(self): - self.driver.find_element_by_name("new_pw").send_keys("foo") - self.driver.find_element_by_name("new_pw_repeat").send_keys("foo") + self.driver.find_element_by_name("new_pw").send_keys("foobarbar") + self.driver.find_element_by_name("new_pw_repeat").send_keys("foobarbar") self.driver.find_element_by_name("old_pw").send_keys("dummy") self.scroll_and_click(self.driver.find_element_by_class_name('btn-save')) self.driver.find_element_by_class_name("alert-success") self.user = User.objects.get(pk=self.user.pk) - assert self.user.check_password("foo") + assert self.user.check_password("foobarbar") + + def test_change_password_short(self): + self.driver.find_element_by_name("new_pw").send_keys("foobar") + self.driver.find_element_by_name("new_pw_repeat").send_keys("foobar") + self.driver.find_element_by_name("old_pw").send_keys("dummy") + self.scroll_and_click(self.driver.find_element_by_class_name('btn-save')) + self.driver.find_element_by_class_name("alert-danger") + pw = self.user.password + self.user = User.objects.get(pk=self.user.pk) + assert self.user.password == pw + + def test_change_password_user_attribute_similarity(self): + self.driver.find_element_by_name("new_pw").send_keys("dummy123") + self.driver.find_element_by_name("new_pw_repeat").send_keys("dummy123") + self.driver.find_element_by_name("old_pw").send_keys("dummy") + self.scroll_and_click(self.driver.find_element_by_class_name('btn-save')) + self.driver.find_element_by_class_name("alert-danger") + pw = self.user.password + self.user = User.objects.get(pk=self.user.pk) + assert self.user.password == pw def test_change_password_require_repeat(self): self.driver.find_element_by_name("new_pw").send_keys("foo")