AAP-37381 Apply password validators from settings.AUTH_PASSWORD_VALIDATORS correctly. (#15897)

* 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

@@ -626,15 +626,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.
@@ -663,6 +689,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)
@@ -984,7 +1036,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.'))
 
@@ -1007,6 +1058,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": <error-message>``}
+            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):
         if new_password and new_password != '$encrypted$':
             obj.set_password(new_password)

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