AAP-37381 Password validators from settings.AUTH_PASSWORD_VALIDATORS were not applied.

[djulich]

AAP-37381

SUMMARY

Fixed application of Django password validators.

The Django password validator UserAttributeSimilarityValidator did not work because it didn't have access to the object's attributes. This is fixed now.

Also added some functional tests for this.

ISSUE TYPE

• Bug, Docs Fix or other nominal change

COMPONENT NAME

• API

AWX VERSION

devel

ADDITIONAL INFORMATION

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.29%. Comparing base (39cd09c) to head (298b09b).
Report is 4 commits behind head on devel.

✅ All tests successful. No failed tests found.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[AlanCoding]

this seems suspicious. Will this method be deleted for the final version of the patch?

[djulich]

It's the base method for serializers which need the model instance for cleaning the input fields. Admittedly it's only needed in UserSerializer for now, but its use-case should be sufficiently generic to justify its integration into BaseSerializer.
Also, the method needs to be defined (as an abstract method if you will) (<-- NO, please see my comment to @bcoca below) in BaseSerializer because only BaseSerializer.validate provides the hook where it must be called (between creating the model instance and before saving it to the database).

[bcoca]

looks like a use case for the @abstractmethod decorator

[djulich]

@bcoca, it was my fault to lead you into thinking that. I was simply wrong in my comment regarding BaseSerializer.full_clean could be seen as an abstract method:

Making BaseSerializer.full_clean an @abstractmethod would require every derived class to implement it. But it is supposed to be overwritten only if a class needs access to the respective model instance for cleaning its input fields, which is only UserSerializer at the moment. So making it abstract would generate a lot of (functional) unnecessary code changes all over the place.

[art-tapin]

I've tested this PR locally in the Controller environment and the fix works perfectly. I added a few Django password validators to my settings and verified that all validators now work correctly including the UserAttributeSimilarityValidator (which was previously failing)

Tests:

• Added validation config to defaults.py and confirmed it was loaded
• Tested creating users with various validation issues:
  • Username matching password (properly rejected!)
  • Too short passwords
  • Common passwords
  • Numeric-only passwords
  • Multiple validation errors at once

The implementation approach using full_clean() seems to be correct. This is the most sensible way to implement this within DRF's validation flow. If it's empty because of the OOP to follow an inheritance pattern, then it seems okay to me.

I'm leaving this as a comment rather than approval because of the "needs_investigation" label, but my I confirm this fix is LGTM 🎖️

[djulich]

@AlanCoding, @art-tapin, can one of you approve, since Alan removed the need_investigation label?

[AlanCoding]

Right now these don't have any visibility for a test failure. The message will say that response.status_code is not 201, but then you won't know which test case that is for. This is partially fixable, as a cheap fix, with:

            assert response.status_code == expected_status_code, (i, fixtures)

This will print the loop variables along with the AssertionError.

But really, the best would be to put these in @pytest.mark.parametrize decorator.

[djulich]

@AlanCoding, you are absolutely right. I added your "cheap fix" so we get at least some information which fixtures caused the failure.

[djulich]

Following the suggestion from @AlanCoding, I refactored test_user_create_with_django_password_validation_ext to utilize @pytest.mark.parametrize instead of looping over a fixtures list within the test function.

[AlanCoding]

If there is no option within the existing DRF and custom serializer methods, then I would agree that we must introduce a new utility method (I am trying to think of other past cases where the object initialization is needed, there are some).

However, I had skepticism of that claim. I'll narrow this discussion to where I expected the validation logic would go, which is either validate_password or validate, and for simplicity I'll go with validate. This is the comment you left about that case.

        # If we try to instantiate a User object in `validate` for passing it to
        # the validators, as recommended in the [Django REST Framework
        # documentation](https://www.django-rest-framework.org/community/3.0-announcement/#differences-between-modelserializer-validation-and-modelform),
        # ValidationError is raised, but the user is saved into the database
        # anyways!

I deleted the methods added in your PR here and added this method to the UserSerializer:

    def validate(self, attrs):
        password = attrs.get("password")
        if password and password != '$encrypted$':
            # Apply validators from settings.AUTH_PASSWORD_VALIDATORS.
            if self.instance:
                obj = self.instance
            else:
                obj = User()
                for field_name in ('username', 'email'):
                    setattr(obj, field_name, attrs.get(field_name))
            django_validate_password(password, user=obj)
        return super().validate(attrs)

Locally running:

pytest awx/main/tests/functional/api/test_user.py

That does pass the tests. So now the consideration is what you said

but the user is saved into the database

If that is the concern, it seems the test may be insufficient. In any case, whether this approach is adopted or not, the test should look for this scenario that your comment suggests, that should be accomplished by applying this diff:

diff --git a/awx/main/tests/functional/api/test_user.py b/awx/main/tests/functional/api/test_user.py
index 5042a087b0..a50d79c0bd 100644
--- a/awx/main/tests/functional/api/test_user.py
+++ b/awx/main/tests/functional/api/test_user.py
@@ -242,11 +242,14 @@ def test_user_create_with_django_password_validation_ext(post, delete, admin):
         print(f"Testing fixtures {i}: {expected_status_code=} {user_attrs=}")
         with override_settings(AUTH_PASSWORD_VALIDATORS=password_validators):
             response = post(reverse('api:user_list'), user_attrs, admin, middleware=SessionMiddleware(mock.Mock()))
-            assert response.status_code == expected_status_code
+            assert response.status_code == expected_status_code, (i, fixtures)
             # 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:
+                username = fixtures['user_attrs']['username']
+                assert not User.objects.filter(username=username)
 
 
 @pytest.mark.django_db

Even applying that, the tests seem to pass. Because of that, I would still prefer .validate. I agree this is, actually, more complicated logic due to not having the model built in the case of new objects. But I would rather accept this (checking self.instance Noneness) than introduce a new type of validation method globally and diverging the code base further from standard Django / DRF patterns as a whole.

---

You've written great test content 👏 , I really appreciate that. I applied your current tests to the devel branch and found a failure. The ability to do this is really helpful, and it is the thoroughness of your test content that gives me the confidence to suggest this change in the first place.

[djulich]

@AlanCoding, you are right about that we can do the django password validation in Serializer.validate. We do not need to add a new utility method Serializer.full_clean method for it. Your UserSerializer.validate draft put me on the right way.

When I tried to validate in UserSerializer.validate, I instantiated the user object by

    obj = User(**attrs)

which indeed does not work: it writes the object into the db before validation, and I didn't bother to find why it behaves like that because I choose the easy way out and added Serializer.full_clean.

When I create the user instance the way @AlanCoding did it, by:

    obj = User()
    for k, v in attrs.items():
        setattr(obj, k, v)

it creates a user object suitable for Django's {{validate_password}} function without any unwanted side effect.

I'll update the code accordingly...

[AlanCoding]

Thanks for bearing with me on this.

Kudos on the fantastic code comments.

[djulich]

Changing the hard-coded test password from 'newpassword' to a random one, say 'aEArV$5Vkdw', which passes all reasonable validators (see comment above this line) should be low risk. But since this issue is not really related to the ticket AAP-37381, I'm hesitant to change it in this PR.

[djulich]

Adding the utility method BaseSerializer.validate_with_obj allows for subclass validators to use the model object without duplicating the code for its creation.

[AlanCoding]

Requesting clarification in method docstring - does obj have updated attributes?

There are 2 cases:

• new object
• updating existing object

In the 1st case the answer is obvious. In the 2nd case, I don't know if I have the "old object" or the "new object".

[djulich]

You get the "old object" in the 2nd case. I refined the code comments accordingly.

[AlanCoding]

I understand the object materialization problem you are focusing on, and why you want an added utility.

My only last word here is that I'd like post-merge review of existing cases. There are many other validators that should be rewritten with this new pattern. This serializers module is over 6,000 lines long, and has an extremely heavily duplicated pattern of getting an attribute from attrs or self.instance depending on the case. You have bitten off a bigger problem than what you might have yet realized, but I welcome code improvements from this, as long as we can commit and carry it forward.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud