From e060e44b050383382cbb77cb41cb8395190d51ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dirk=20J=C3=BClich?= Date: Wed, 2 Apr 2025 16:45:58 +0200 Subject: [PATCH] AAP-37381 Apply Django password validators correctly. (#6902) * Move call to django_validate_password to the correct method were the user object is available. * Added tests for the Django password validation functionality. --- awx/api/serializers.py | 111 ++++++++++++- awx/main/tests/functional/api/test_user.py | 173 +++++++++++++++++++++ 2 files changed, 276 insertions(+), 8 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 5b3e0b4144..f3f37a03a1 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -639,15 +639,41 @@ class BaseSerializer(serializers.ModelSerializer, metaclass=BaseSerializerMetacl return exclusions def validate(self, attrs): + """ + Apply serializer validation. Called by DRF. + + Can be extended by subclasses. Or consider overwriting + `validate_with_obj` in subclasses, which provides access to the model + object and exception handling for field validation. + + :param dict attrs: The names and values of the model form fields. + :raise rest_framework.exceptions.ValidationError: If the validation + fails. + + The exception must contain a dict with the names of the form fields + which failed validation as keys, and a list of error messages as + values. This ensures that the error messages are rendered near the + relevant fields. + :return: The names and values from the model form fields, possibly + modified by the validations. + :rtype: dict + """ attrs = super(BaseSerializer, self).validate(attrs) + # Create/update a model instance and run its full_clean() method to + # do any validation implemented on the model class. + exclusions = self.get_validation_exclusions(self.instance) + # Create a new model instance or take the existing one if it exists, + # and update its attributes with the respective field values from + # attrs. + obj = self.instance or self.Meta.model() + for k, v in attrs.items(): + if k not in exclusions and k != 'canonical_address_port': + setattr(obj, k, v) try: - # Create/update a model instance and run its full_clean() method to - # do any validation implemented on the model class. - exclusions = self.get_validation_exclusions(self.instance) - obj = self.instance or self.Meta.model() - for k, v in attrs.items(): - if k not in exclusions and k != 'canonical_address_port': - setattr(obj, k, v) + # Run serializer validators which need the model object for + # validation. + self.validate_with_obj(attrs, obj) + # Apply any validations implemented on the model class. obj.full_clean(exclude=exclusions) # full_clean may modify values on the instance; copy those changes # back to attrs so they are saved. @@ -676,6 +702,32 @@ class BaseSerializer(serializers.ModelSerializer, metaclass=BaseSerializerMetacl raise ValidationError(d) return attrs + def validate_with_obj(self, attrs, obj): + """ + Overwrite this if you need the model instance for your validation. + + :param dict attrs: The names and values of the model form fields. + :param obj: An instance of the class's meta model. + + If the serializer runs on a newly created object, obj contains only + the attrs from its serializer. If the serializer runs because an + object has been edited, obj is the existing model instance with all + attributes and values available. + :raise django.core.exceptionsValidationError: Raise this if your + validation fails. + + To make the error appear at the respective form field, instantiate + the Exception with a dict containing the field name as key and the + error message as value. + + Example: ``ValidationError({"password": "Not good enough!"})`` + + If the exception contains just a string, the message cannot be + related to a field and is rendered at the top of the model form. + :return: None + """ + return + def reverse(self, *args, **kwargs): kwargs['request'] = self.context.get('request') return reverse(*args, **kwargs) @@ -1026,7 +1078,6 @@ class UserSerializer(BaseSerializer): return ret def validate_password(self, value): - django_validate_password(value) if not self.instance and value in (None, ''): raise serializers.ValidationError(_('Password required for new User.')) @@ -1049,6 +1100,50 @@ class UserSerializer(BaseSerializer): return value + def validate_with_obj(self, attrs, obj): + """ + Validate the password with the Django password validators + + To enable the Django password validators, configure + `settings.AUTH_PASSWORD_VALIDATORS` as described in the [Django + docs](https://docs.djangoproject.com/en/5.1/topics/auth/passwords/#enabling-password-validation) + + :param dict attrs: The User form field names and their values as a dict. + Example:: + + { + 'username': 'TestUsername', 'first_name': 'FirstName', + 'last_name': 'LastName', 'email': 'First.Last@my.org', + 'is_superuser': False, 'is_system_auditor': False, + 'password': 'secret123' + } + + :param obj: The User model instance. + :raises django.core.exceptions.ValidationError: Raise this if at least + one Django password validator fails. + + The exception contains a dict ``{"password": ``} + which indicates that the password field has failed validation, and + the reason for failure. + :return: None. + """ + # We must do this here instead of in `validate_password` bacause some + # django password validators need access to other model instance fields, + # e.g. ``username`` for the ``UserAttributeSimilarityValidator``. + password = attrs.get("password") + # Skip validation if no password has been entered. This may happen when + # an existing User is edited. + if password and password != '$encrypted$': + # Apply validators from settings.AUTH_PASSWORD_VALIDATORS. This may + # raise ValidationError. + # + # If the validation fails, re-raise the exception with adjusted + # content to make the error appear near the password field. + try: + django_validate_password(password, user=obj) + except DjangoValidationError as exc: + raise DjangoValidationError({"password": exc.messages}) + def _update_password(self, obj, new_password): # For now we're not raising an error, just not saving password for # users managed by LDAP who already have an unusable password set. diff --git a/awx/main/tests/functional/api/test_user.py b/awx/main/tests/functional/api/test_user.py index 87234c18ba..5eae127263 100644 --- a/awx/main/tests/functional/api/test_user.py +++ b/awx/main/tests/functional/api/test_user.py @@ -56,6 +56,175 @@ def test_user_create(post, admin): assert not response.data['is_system_auditor'] +# Disable local password checks to ensure that any ValidationError originates from the Django validators. +@override_settings( + LOCAL_PASSWORD_MIN_LENGTH=1, + LOCAL_PASSWORD_MIN_DIGITS=0, + LOCAL_PASSWORD_MIN_UPPER=0, + LOCAL_PASSWORD_MIN_SPECIAL=0, +) +@pytest.mark.django_db +def test_user_create_with_django_password_validation_basic(post, admin): + """Test if the Django password validators are applied correctly.""" + with override_settings( + AUTH_PASSWORD_VALIDATORS=[ + { + 'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator', + }, + { + 'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', + 'OPTIONS': { + 'min_length': 3, + }, + }, + { + 'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator', + }, + { + 'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator', + }, + ], + ): + # This user should fail the UserAttrSimilarity, MinLength and CommonPassword validators. + user_attrs = ( + { + "password": "Password", # NOSONAR + "username": "Password", + "is_superuser": False, + }, + ) + print(f"Create user with invalid password {user_attrs=}") + response = post(reverse('api:user_list'), user_attrs, admin, middleware=SessionMiddleware(mock.Mock())) + assert response.status_code == 400 + # This user should pass all Django validators. + user_attrs = { + "password": "r$TyKiOCb#ED", # NOSONAR + "username": "TestUser", + "is_superuser": False, + } + print(f"Create user with valid password {user_attrs=}") + response = post(reverse('api:user_list'), user_attrs, admin, middleware=SessionMiddleware(mock.Mock())) + assert response.status_code == 201 + + +@pytest.mark.parametrize( + "user_attrs,validators,expected_status_code", + [ + # Test password similarity with username. + ( + {"password": "TestUser1", "username": "TestUser1", "is_superuser": False}, # NOSONAR + [ + {'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator'}, + ], + 400, + ), + ( + {"password": "abc", "username": "TestUser1", "is_superuser": False}, # NOSONAR + [ + {'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator'}, + ], + 201, + ), + # Test password min length criterion. + ( + {"password": "TooShort", "username": "TestUser1", "is_superuser": False}, # NOSONAR + [ + {'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', 'OPTIONS': {'min_length': 9}}, + ], + 400, + ), + ( + {"password": "LongEnough", "username": "TestUser1", "is_superuser": False}, # NOSONAR + [ + {'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', 'OPTIONS': {'min_length': 9}}, + ], + 201, + ), + # Test password is too common criterion. + ( + {"password": "Password", "username": "TestUser1", "is_superuser": False}, # NOSONAR + [ + {'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator'}, + ], + 400, + ), + ( + {"password": "aEArV$5Vkdw", "username": "TestUser1", "is_superuser": False}, # NOSONAR + [ + {'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator'}, + ], + 201, + ), + # Test if password is only numeric. + ( + {"password": "1234567890", "username": "TestUser1", "is_superuser": False}, # NOSONAR + [ + {'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator'}, + ], + 400, + ), + ( + {"password": "abc4567890", "username": "TestUser1", "is_superuser": False}, # NOSONAR + [ + {'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator'}, + ], + 201, + ), + ], +) +# Disable local password checks to ensure that any ValidationError originates from the Django validators. +@override_settings( + LOCAL_PASSWORD_MIN_LENGTH=1, + LOCAL_PASSWORD_MIN_DIGITS=0, + LOCAL_PASSWORD_MIN_UPPER=0, + LOCAL_PASSWORD_MIN_SPECIAL=0, +) +@pytest.mark.django_db +def test_user_create_with_django_password_validation_ext(post, delete, admin, user_attrs, validators, expected_status_code): + """Test the functionality of the single Django password validators.""" + # + default_parameters = { + # Default values for input parameters which are None. + "user_attrs": { + "password": "r$TyKiOCb#ED", # NOSONAR + "username": "DefaultUser", + "is_superuser": False, + }, + "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': 'django.contrib.auth.password_validation.NumericPasswordValidator', + }, + ], + } + user_attrs = user_attrs if user_attrs is not None else default_parameters["user_attrs"] + validators = validators if validators is not None else default_parameters["validators"] + with override_settings(AUTH_PASSWORD_VALIDATORS=validators): + response = post(reverse('api:user_list'), user_attrs, admin, middleware=SessionMiddleware(mock.Mock())) + assert response.status_code == expected_status_code + # Delete user if it was created succesfully. + if response.status_code == 201: + response = delete(reverse('api:user_detail', kwargs={'pk': response.data['id']}), admin, middleware=SessionMiddleware(mock.Mock())) + assert response.status_code == 204 + else: + # Catch the unexpected behavior that sometimes the user is written + # into the database before the validation fails. This actually can + # happen if UserSerializer.validate instantiates User(**attrs)! + username = user_attrs['username'] + assert not User.objects.filter(username=username) + + @pytest.mark.django_db def test_fail_double_create_user(post, admin): response = post(reverse('api:user_list'), EXAMPLE_USER_DATA, admin, middleware=SessionMiddleware(mock.Mock())) @@ -82,6 +251,10 @@ def test_updating_own_password_refreshes_session(patch, admin): Updating your own password should refresh the session id. ''' with mock.patch('awx.api.serializers.update_session_auth_hash') as update_session_auth_hash: + # Attention: If the Django password validator `CommonPasswordValidator` + # is active, this test case will fail because this validator raises on + # password 'newpassword'. Consider changing the hard-coded password to + # something uncommon. patch(reverse('api:user_detail', kwargs={'pk': admin.pk}), {'password': 'newpassword'}, admin, middleware=SessionMiddleware(mock.Mock())) assert update_session_auth_hash.called