iMerica / dj-rest-auth

Authentication for Django Rest Framework
https://dj-rest-auth.readthedocs.io/en/latest/index.html
MIT License
1.67k stars 311 forks source link

New user creation not calling validate_password() on registration form submit #438

Closed edmundsj closed 2 years ago

edmundsj commented 2 years ago

I am using a custom user model in my project, as recommended by django. I am doing this so that I can use e-mail as the username and so that I can use my own password validators. If you know a more direct route to this goal than I am taking, that is my ultimate aim: username and password validation with my custom user model. I wrote a custom model manager for this, and in that custom model manager have a create_user() method I was previously using with my custom written views to create a user. In my settings.py, I have specified my customer user model:

AUTH_USER_MODEL = 'users.User'

When I run get_user_model() from django.contrib.auth it successfully returns my custom model, as expected. However, it looks like when I submit a POST request to create a new user with dj-rest-auth, create_user() of my custom model manager is never called. A user is written to the database, but not via the model manager's create_user() method. I can tell this because the password isn't validated, and I have a password validation line in the create_model() method.

What method is being called to create users in dj-rest-auth? Why is it not using get_user_model().objects.create_user() as is typical in django? How do I force it to use my create_user method?

edmundsj commented 2 years ago

It looks like internally dj-rest-auth uses django-allauth allauth.account.adapter.get_adapter as the analogue to the model manager, which has a new_user() method that gets called to create a new user, followed by a call to save_user() to save the user. The code for new_user is below:

    def new_user(self, request):
        """
        Instantiates a new User instance.
        """
        user = get_user_model()()
        return user

This tells me that allauth wants to be the model manager, and doesn't support using custom model managers. Is this correct? If so, it would be a little obnoxious. Looking at the save_user() method:

def save_user(self, request, user, form, commit=True):
        """
        Saves a new `User` instance using information provided in the
        signup form.
        """
        from .utils import user_email, user_field, user_username

        data = form.cleaned_data
        first_name = data.get("first_name")
        last_name = data.get("last_name")
        email = data.get("email")
        username = data.get("username")
        user_email(user, email)
        user_username(user, username)
        if first_name:
            user_field(user, "first_name", first_name)
        if last_name:
            user_field(user, "last_name", last_name)
        if "password1" in data:
            user.set_password(data["password1"])
        else:
            user.set_unusable_password()
        self.populate_username(request, user)
        if commit:
            # Ability not to commit makes it easier to derive from
            # this adapter by adding
            user.save()
        return user

It looks like the username/password validation is done at the form level with the form.cleaned_data object. This "form" appears to actually refer to the RegisterSerializer object in dj_rest_auth.registration. It looks like the RegisterSerializer cleaned_data property is where the adapter is getting the validated username/password. It's not clear to me from this class how the cleaned_data is being generated. I don't see a cleaned_data property anywhere in the RegisterSerializer class on in the rest_framework.serializers.Serializer class. How do I specify that this serializer uses my username/password validators?

Why is it not already using django's globally-specified password validators located in AUTH_PASSWORD_VALIDATORS. This seems like a bug. The RegisterSerializer class has a method called validate_password1(), which calls get_adapter().clean_password(),

    def validate_password1(self, password):
        return get_adapter().clean_password(password)

which itself calls validate_password(), which should use the password validators I specified. This tells me that validate_password1 is never getting called.

    def clean_password(self, password, user=None):
        """
        Validates a password. You can hook into this if you want to
        restric the allowed password choices.
        """
        min_length = app_settings.PASSWORD_MIN_LENGTH
        if min_length and len(password) < min_length:
            raise forms.ValidationError(
                _("Password must be a minimum of {0} characters.").format(min_length)
            )
        validate_password(password, user)
        return password

Smells like a bug.

edmundsj commented 2 years ago

This was actually a bug in my tests. The problem is that I was expecting validation to happen before database access in my test suite, but this is no longer the case. The database is first accessed with the call to the User() constructor, and then validation is performed, after model creation but before model saving. Seems weird that creating a User() object requires database access. Not a bug. Nothing to see here.