From 00874ac9e7a6d290d192844e499816bca5e388ed Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Fri, 30 Aug 2024 14:23:01 -0700 Subject: [PATCH] 17289 enforce minimum password strength (#17299) * 17289 add password validation * 17289 add password validation * 17289 fix tests * 17289 fix tests * Update netbox/utilities/password_validation.py Co-authored-by: Jeremy Stretch * Update netbox/utilities/password_validation.py Co-authored-by: Jeremy Stretch * Update netbox/utilities/password_validation.py Co-authored-by: Jeremy Stretch * 17289 update tests * 17289 remove common password check * 17289 fix user create * 17289 revert _post_clean --------- Co-authored-by: Jeremy Stretch --- netbox/netbox/settings.py | 13 ++++++-- netbox/users/tests/test_api.py | 40 ++++++++++++++++++------- netbox/users/tests/test_views.py | 36 +++++++++++++++------- netbox/utilities/password_validation.py | 27 +++++++++++++++++ 4 files changed, 94 insertions(+), 22 deletions(-) create mode 100644 netbox/utilities/password_validation.py diff --git a/netbox/netbox/settings.py b/netbox/netbox/settings.py index eadac566f34..2c013027542 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,17 @@ 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.MinimumLengthValidator", + "OPTIONS": { + "min_length": 12, + }, + }, + { + "NAME": "utilities.password_validation.AlphanumericPasswordValidator", + }, +]) 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) diff --git a/netbox/users/tests/test_api.py b/netbox/users/tests/test_api.py index f37eaf764da..71496f00774 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) @@ -102,7 +102,7 @@ def test_password_validation_enforced(self): data = { 'username': 'new_user', - 'password': 'foo', + 'password': 'f1A', } url = reverse('users-api:user-list') @@ -111,10 +111,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 3dabc9dae55..86da7dda233 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 = ( @@ -60,10 +60,6 @@ def setUpTestData(cls): 'last_name': 'newlastname', } - @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. @@ -71,8 +67,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 +80,30 @@ def test_password_validation_enforced(self): self.assertHttpStatus(response, 200) # Password long enough - data['password'] = 'foobar123' - data['confirm_password'] = 'foobar123' + data['password'] = 'fooBarFoo123' + data['confirm_password'] = 'fooBarFoo123' 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, diff --git a/netbox/utilities/password_validation.py b/netbox/utilities/password_validation.py new file mode 100644 index 00000000000..9094c4c7e6a --- /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 AlphanumericPasswordValidator: + """ + 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 must have at least one numeral."), + ) + + if not any(char.isupper() for char in password): + raise ValidationError( + _("Password must have at least one uppercase letter."), + ) + + if not any(char.islower() for char in password): + raise ValidationError( + _("Password must 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.")