From 30822ee7d1d36cafc04996c7971bf4b869b7ca11 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Thu, 29 Aug 2024 08:50:21 -0700 Subject: [PATCH 01/11] 17289 add password validation --- netbox/netbox/settings.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/netbox/netbox/settings.py b/netbox/netbox/settings.py index eadac566f3..a264253b79 100644 --- a/netbox/netbox/settings.py +++ b/netbox/netbox/settings.py @@ -21,7 +21,6 @@ from utilities.release import load_release_data from utilities.string import trailing_slash - # # Environment setup # @@ -63,7 +62,23 @@ ADMINS = getattr(configuration, 'ADMINS', []) ALLOW_TOKEN_RETRIEVAL = getattr(configuration, 'ALLOW_TOKEN_RETRIEVAL', True) ALLOWED_HOSTS = getattr(configuration, 'ALLOWED_HOSTS') # Required -AUTH_PASSWORD_VALIDATORS = getattr(configuration, 'AUTH_PASSWORD_VALIDATORS', []) +AUTH_PASSWORD_VALIDATORS = getattr(configuration, 'AUTH_PASSWORD_VALIDATORS', [ + { + "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator", + }, + { + "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator", + "OPTIONS": { + "min_length": 12, + }, + }, + { + "NAME": "django.contrib.auth.password_validation.CommonPasswordValidator", + }, + { + "NAME": "utilities.password_validation.NumericAlphaPasswordValidator", + }, +]) BASE_PATH = trailing_slash(getattr(configuration, 'BASE_PATH', '')) CHANGELOG_SKIP_EMPTY_CHANGES = getattr(configuration, 'CHANGELOG_SKIP_EMPTY_CHANGES', True) CENSUS_REPORTING_ENABLED = getattr(configuration, 'CENSUS_REPORTING_ENABLED', True) From 85229faad0029a3770ca3b31036856e0b3bbe49b Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Thu, 29 Aug 2024 08:50:50 -0700 Subject: [PATCH 02/11] 17289 add password validation --- netbox/utilities/password_validation.py | 27 +++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 netbox/utilities/password_validation.py diff --git a/netbox/utilities/password_validation.py b/netbox/utilities/password_validation.py new file mode 100644 index 0000000000..ef5da076eb --- /dev/null +++ b/netbox/utilities/password_validation.py @@ -0,0 +1,27 @@ +from django.core.exceptions import ValidationError +from django.utils.translation import gettext as _ + + +class NumericAlphaPasswordValidator: + """ + Validate that the password has at least one numeral, one uppercase letter and one lowercase letter. + """ + + def validate(self, password, user=None): + if not any(char.isdigit() for char in password): + raise ValidationError( + _("Password should have at least one numeral"), + ) + + if not any(char.isupper() for char in password): + raise ValidationError( + _("Password should have at least one uppercase letter"), + ) + + if not any(char.islower() for char in password): + raise ValidationError( + _("Password should have at least one lowercase letter"), + ) + + def get_help_text(self): + return _("Your password must contain at least one numeral, one uppercase letter and one lowercase letter.") From d6f477599dd1a2304a1dc4bbed852f9621fada12 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Thu, 29 Aug 2024 09:05:58 -0700 Subject: [PATCH 03/11] 17289 fix tests --- netbox/users/tests/test_views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/netbox/users/tests/test_views.py b/netbox/users/tests/test_views.py index 3dabc9dae5..435d7579c0 100644 --- a/netbox/users/tests/test_views.py +++ b/netbox/users/tests/test_views.py @@ -38,8 +38,8 @@ def setUpTestData(cls): 'first_name': 'firstx', 'last_name': 'lastx', 'email': 'userx@foo.com', - 'password': 'pass1xxx', - 'confirm_password': 'pass1xxx', + 'password': 'pass1xxxABCD', + 'confirm_password': 'pass1xxxABCD', } cls.csv_data = ( @@ -84,8 +84,8 @@ def test_password_validation_enforced(self): self.assertHttpStatus(response, 200) # Password long enough - data['password'] = 'foobar123' - data['confirm_password'] = 'foobar123' + data['password'] = 'foobar123AD' + data['confirm_password'] = 'foobar123AD' self.assertHttpStatus(self.client.post(**request), 302) From d0b2f0c5a160f5fcd21c69d2e653c2acd62a116b Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Thu, 29 Aug 2024 09:46:48 -0700 Subject: [PATCH 04/11] 17289 fix tests --- netbox/users/tests/test_api.py | 61 ++++++++++++++++++++++++-------- netbox/users/tests/test_views.py | 49 ++++++++++++++++++++----- 2 files changed, 88 insertions(+), 22 deletions(-) diff --git a/netbox/users/tests/test_api.py b/netbox/users/tests/test_api.py index f37eaf764d..63d3e1ff79 100644 --- a/netbox/users/tests/test_api.py +++ b/netbox/users/tests/test_api.py @@ -38,26 +38,26 @@ def setUpTestData(cls): permissions[2].object_types.add(ObjectType.objects.get_by_natural_key('dcim', 'rack')) users = ( - User(username='User1', password='password1'), - User(username='User2', password='password2'), - User(username='User3', password='password3'), + User(username='User1', password='FooBarFooBar1'), + User(username='User2', password='FooBarFooBar2'), + User(username='User3', password='FooBarFooBar3'), ) User.objects.bulk_create(users) cls.create_data = [ { 'username': 'User4', - 'password': 'password4', + 'password': 'FooBarFooBar4', 'permissions': [permissions[0].pk], }, { 'username': 'User5', - 'password': 'password5', + 'password': 'FooBarFooBar5', 'permissions': [permissions[1].pk], }, { 'username': 'User6', - 'password': 'password6', + 'password': 'FooBarFooBar6', 'permissions': [permissions[2].pk], }, ] @@ -77,12 +77,12 @@ def test_that_password_is_changed(self): user_credentials = { 'username': 'newuser', - 'password': 'abc123', + 'password': 'abc123FOO', } user = User.objects.create_user(**user_credentials) data = { - 'password': 'newpassword' + 'password': 'FooBarFooBar1' } url = reverse('users-api:user-detail', kwargs={'pk': user.id}) response = self.client.patch(url, data, format='json', **self.header) @@ -90,10 +90,23 @@ def test_that_password_is_changed(self): user.refresh_from_db() self.assertTrue(user.check_password(data['password'])) - @override_settings(AUTH_PASSWORD_VALIDATORS=[{ - 'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', - 'OPTIONS': {'min_length': 8} - }]) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + { + "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator", + }, + { + "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator", + "OPTIONS": { + "min_length": 8, + }, + }, + { + "NAME": "django.contrib.auth.password_validation.CommonPasswordValidator", + }, + { + "NAME": "utilities.password_validation.NumericAlphaPasswordValidator", + }, + ]) def test_password_validation_enforced(self): """ Test that any configured password validation rules (AUTH_PASSWORD_VALIDATORS) are enforced. @@ -102,7 +115,7 @@ def test_password_validation_enforced(self): data = { 'username': 'new_user', - 'password': 'foo', + 'password': 'f1A', } url = reverse('users-api:user-list') @@ -111,10 +124,30 @@ def test_password_validation_enforced(self): self.assertEqual(response.status_code, 400) # Password long enough - data['password'] = 'foobar123' + data['password'] = 'FooBar123' response = self.client.post(url, data, format='json', **self.header) self.assertEqual(response.status_code, 201) + # Password no number + data['password'] = 'foobarFoo' + response = self.client.post(url, data, format='json', **self.header) + self.assertEqual(response.status_code, 400) + + # Password no letter + data['password'] = '123456789012' + response = self.client.post(url, data, format='json', **self.header) + self.assertEqual(response.status_code, 400) + + # Password no uppercase + data['password'] = 'foobarfoo1' + response = self.client.post(url, data, format='json', **self.header) + self.assertEqual(response.status_code, 400) + + # Password no lowercase + data['password'] = 'FOOBARFOO1' + response = self.client.post(url, data, format='json', **self.header) + self.assertEqual(response.status_code, 400) + class GroupTest(APIViewTestCases.APIViewTestCase): model = Group diff --git a/netbox/users/tests/test_views.py b/netbox/users/tests/test_views.py index 435d7579c0..1b4d821f8d 100644 --- a/netbox/users/tests/test_views.py +++ b/netbox/users/tests/test_views.py @@ -60,10 +60,23 @@ def setUpTestData(cls): 'last_name': 'newlastname', } - @override_settings(AUTH_PASSWORD_VALIDATORS=[{ - 'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', - 'OPTIONS': {'min_length': 8} - }]) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + { + "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator", + }, + { + "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator", + "OPTIONS": { + "min_length": 8, + }, + }, + { + "NAME": "django.contrib.auth.password_validation.CommonPasswordValidator", + }, + { + "NAME": "utilities.password_validation.NumericAlphaPasswordValidator", + }, + ]) def test_password_validation_enforced(self): """ Test that any configured password validation rules (AUTH_PASSWORD_VALIDATORS) are enforced. @@ -71,8 +84,8 @@ def test_password_validation_enforced(self): self.add_permissions('users.add_user') data = { 'username': 'new_user', - 'password': 'foo', - 'confirm_password': 'foo', + 'password': 'F1a', + 'confirm_password': 'F1a', } # Password too short @@ -84,10 +97,30 @@ def test_password_validation_enforced(self): self.assertHttpStatus(response, 200) # Password long enough - data['password'] = 'foobar123AD' - data['confirm_password'] = 'foobar123AD' + data['password'] = 'fooBar12' + data['confirm_password'] = 'fooBar12' self.assertHttpStatus(self.client.post(**request), 302) + # Password no number + data['password'] = 'FooBarFooBar' + data['confirm_password'] = 'FooBarFooBar' + self.assertHttpStatus(self.client.post(**request), 200) + + # Password no letter + data['password'] = '123456789123' + data['confirm_password'] = '123456789123' + self.assertHttpStatus(self.client.post(**request), 200) + + # Password no uppercase + data['password'] = 'foobar123abc' + data['confirm_password'] = 'foobar123abc' + self.assertHttpStatus(self.client.post(**request), 200) + + # Password no lowercase + data['password'] = 'FOOBAR123ABC' + data['confirm_password'] = 'FOOBAR123ABC' + self.assertHttpStatus(self.client.post(**request), 200) + class GroupTestCase( ViewTestCases.GetObjectViewTestCase, From 37b724c6bc3c94aed4a69d9aa19fd8438e724eb0 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Fri, 30 Aug 2024 10:36:20 -0700 Subject: [PATCH 05/11] Update netbox/utilities/password_validation.py Co-authored-by: Jeremy Stretch --- netbox/utilities/password_validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netbox/utilities/password_validation.py b/netbox/utilities/password_validation.py index ef5da076eb..4d744a2dd1 100644 --- a/netbox/utilities/password_validation.py +++ b/netbox/utilities/password_validation.py @@ -20,7 +20,7 @@ def validate(self, password, user=None): if not any(char.islower() for char in password): raise ValidationError( - _("Password should have at least one lowercase letter"), + _("Password must have at least one lowercase letter."), ) def get_help_text(self): From 329f7c32950eb64a2c76f24ba057f6669ec63fa1 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Fri, 30 Aug 2024 10:36:29 -0700 Subject: [PATCH 06/11] Update netbox/utilities/password_validation.py Co-authored-by: Jeremy Stretch --- netbox/utilities/password_validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netbox/utilities/password_validation.py b/netbox/utilities/password_validation.py index 4d744a2dd1..ca198cfa9a 100644 --- a/netbox/utilities/password_validation.py +++ b/netbox/utilities/password_validation.py @@ -10,7 +10,7 @@ class NumericAlphaPasswordValidator: def validate(self, password, user=None): if not any(char.isdigit() for char in password): raise ValidationError( - _("Password should have at least one numeral"), + _("Password must have at least one numeral."), ) if not any(char.isupper() for char in password): From b57de257b52756df5cedc8b87c906509cd7fcc83 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Fri, 30 Aug 2024 10:36:42 -0700 Subject: [PATCH 07/11] Update netbox/utilities/password_validation.py Co-authored-by: Jeremy Stretch --- netbox/utilities/password_validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netbox/utilities/password_validation.py b/netbox/utilities/password_validation.py index ca198cfa9a..ff34e79320 100644 --- a/netbox/utilities/password_validation.py +++ b/netbox/utilities/password_validation.py @@ -15,7 +15,7 @@ def validate(self, password, user=None): if not any(char.isupper() for char in password): raise ValidationError( - _("Password should have at least one uppercase letter"), + _("Password must have at least one uppercase letter."), ) if not any(char.islower() for char in password): From 44c37ecdc522275ce6f75b002a3821e19d91fc53 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Fri, 30 Aug 2024 10:49:37 -0700 Subject: [PATCH 08/11] 17289 update tests --- netbox/netbox/settings.py | 2 +- netbox/users/tests/test_api.py | 21 ++++----------------- netbox/users/tests/test_views.py | 21 ++------------------- netbox/utilities/password_validation.py | 2 +- 4 files changed, 8 insertions(+), 38 deletions(-) diff --git a/netbox/netbox/settings.py b/netbox/netbox/settings.py index a264253b79..e08516d3f6 100644 --- a/netbox/netbox/settings.py +++ b/netbox/netbox/settings.py @@ -76,7 +76,7 @@ "NAME": "django.contrib.auth.password_validation.CommonPasswordValidator", }, { - "NAME": "utilities.password_validation.NumericAlphaPasswordValidator", + "NAME": "utilities.password_validation.AlphanumericPasswordValidator", }, ]) BASE_PATH = trailing_slash(getattr(configuration, 'BASE_PATH', '')) diff --git a/netbox/users/tests/test_api.py b/netbox/users/tests/test_api.py index 63d3e1ff79..71496f0077 100644 --- a/netbox/users/tests/test_api.py +++ b/netbox/users/tests/test_api.py @@ -90,23 +90,10 @@ def test_that_password_is_changed(self): user.refresh_from_db() self.assertTrue(user.check_password(data['password'])) - @override_settings(AUTH_PASSWORD_VALIDATORS=[ - { - "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator", - }, - { - "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator", - "OPTIONS": { - "min_length": 8, - }, - }, - { - "NAME": "django.contrib.auth.password_validation.CommonPasswordValidator", - }, - { - "NAME": "utilities.password_validation.NumericAlphaPasswordValidator", - }, - ]) + @override_settings(AUTH_PASSWORD_VALIDATORS=[{ + 'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', + 'OPTIONS': {'min_length': 8} + }]) def test_password_validation_enforced(self): """ Test that any configured password validation rules (AUTH_PASSWORD_VALIDATORS) are enforced. diff --git a/netbox/users/tests/test_views.py b/netbox/users/tests/test_views.py index 1b4d821f8d..86da7dda23 100644 --- a/netbox/users/tests/test_views.py +++ b/netbox/users/tests/test_views.py @@ -60,23 +60,6 @@ def setUpTestData(cls): 'last_name': 'newlastname', } - @override_settings(AUTH_PASSWORD_VALIDATORS=[ - { - "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator", - }, - { - "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator", - "OPTIONS": { - "min_length": 8, - }, - }, - { - "NAME": "django.contrib.auth.password_validation.CommonPasswordValidator", - }, - { - "NAME": "utilities.password_validation.NumericAlphaPasswordValidator", - }, - ]) def test_password_validation_enforced(self): """ Test that any configured password validation rules (AUTH_PASSWORD_VALIDATORS) are enforced. @@ -97,8 +80,8 @@ def test_password_validation_enforced(self): self.assertHttpStatus(response, 200) # Password long enough - data['password'] = 'fooBar12' - data['confirm_password'] = 'fooBar12' + data['password'] = 'fooBarFoo123' + data['confirm_password'] = 'fooBarFoo123' self.assertHttpStatus(self.client.post(**request), 302) # Password no number diff --git a/netbox/utilities/password_validation.py b/netbox/utilities/password_validation.py index ff34e79320..9094c4c7e6 100644 --- a/netbox/utilities/password_validation.py +++ b/netbox/utilities/password_validation.py @@ -2,7 +2,7 @@ from django.utils.translation import gettext as _ -class NumericAlphaPasswordValidator: +class AlphanumericPasswordValidator: """ Validate that the password has at least one numeral, one uppercase letter and one lowercase letter. """ From 4b5dad85555381bb08c31a50997208dbd1047073 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Fri, 30 Aug 2024 13:08:51 -0700 Subject: [PATCH 09/11] 17289 remove common password check --- netbox/netbox/settings.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/netbox/netbox/settings.py b/netbox/netbox/settings.py index e08516d3f6..a759dcfd3e 100644 --- a/netbox/netbox/settings.py +++ b/netbox/netbox/settings.py @@ -72,9 +72,6 @@ "min_length": 12, }, }, - { - "NAME": "django.contrib.auth.password_validation.CommonPasswordValidator", - }, { "NAME": "utilities.password_validation.AlphanumericPasswordValidator", }, From 372bcbacc776faf7b58212b8e2fd675894020786 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Fri, 30 Aug 2024 13:40:09 -0700 Subject: [PATCH 10/11] 17289 fix user create --- netbox/users/forms/model_forms.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/netbox/users/forms/model_forms.py b/netbox/users/forms/model_forms.py index 639b9f7264..ad3f50d2c7 100644 --- a/netbox/users/forms/model_forms.py +++ b/netbox/users/forms/model_forms.py @@ -2,7 +2,7 @@ from django.conf import settings from django.contrib.auth import password_validation from django.contrib.postgres.forms import SimpleArrayField -from django.core.exceptions import FieldError +from django.core.exceptions import FieldError, ValidationError from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ @@ -221,16 +221,21 @@ def save(self, *args, **kwargs): return instance + def _post_clean(self): + super()._post_clean() + # Validate the password after self.instance is updated with form data + if self.cleaned_data['password']: + try: + password_validation.validate_password(self.cleaned_data['password'], self.instance) + except ValidationError as error: + self.add_error('password', error) + def clean(self): # Check that password confirmation matches if password is set if self.cleaned_data['password'] and self.cleaned_data['password'] != self.cleaned_data['confirm_password']: raise forms.ValidationError(_("Passwords do not match! Please check your input and try again.")) - # Enforce password validation rules (if configured) - if self.cleaned_data['password']: - password_validation.validate_password(self.cleaned_data['password'], self.instance) - class GroupForm(forms.ModelForm): users = DynamicModelMultipleChoiceField( From e2b0516b309d06052dc03356e2d03068415d32e2 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Fri, 30 Aug 2024 13:54:20 -0700 Subject: [PATCH 11/11] 17289 revert _post_clean --- netbox/netbox/settings.py | 3 --- netbox/users/forms/model_forms.py | 15 +++++---------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/netbox/netbox/settings.py b/netbox/netbox/settings.py index a759dcfd3e..2c01302754 100644 --- a/netbox/netbox/settings.py +++ b/netbox/netbox/settings.py @@ -63,9 +63,6 @@ ALLOW_TOKEN_RETRIEVAL = getattr(configuration, 'ALLOW_TOKEN_RETRIEVAL', True) ALLOWED_HOSTS = getattr(configuration, 'ALLOWED_HOSTS') # Required AUTH_PASSWORD_VALIDATORS = getattr(configuration, 'AUTH_PASSWORD_VALIDATORS', [ - { - "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator", - }, { "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator", "OPTIONS": { diff --git a/netbox/users/forms/model_forms.py b/netbox/users/forms/model_forms.py index ad3f50d2c7..639b9f7264 100644 --- a/netbox/users/forms/model_forms.py +++ b/netbox/users/forms/model_forms.py @@ -2,7 +2,7 @@ from django.conf import settings from django.contrib.auth import password_validation from django.contrib.postgres.forms import SimpleArrayField -from django.core.exceptions import FieldError, ValidationError +from django.core.exceptions import FieldError from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ @@ -221,21 +221,16 @@ def save(self, *args, **kwargs): return instance - def _post_clean(self): - super()._post_clean() - # Validate the password after self.instance is updated with form data - if self.cleaned_data['password']: - try: - password_validation.validate_password(self.cleaned_data['password'], self.instance) - except ValidationError as error: - self.add_error('password', error) - def clean(self): # Check that password confirmation matches if password is set if self.cleaned_data['password'] and self.cleaned_data['password'] != self.cleaned_data['confirm_password']: raise forms.ValidationError(_("Passwords do not match! Please check your input and try again.")) + # Enforce password validation rules (if configured) + if self.cleaned_data['password']: + password_validation.validate_password(self.cleaned_data['password'], self.instance) + class GroupForm(forms.ModelForm): users = DynamicModelMultipleChoiceField(