sunscrapers / djoser

REST implementation of Django authentication system.
MIT License
2.55k stars 458 forks source link

Login Error Messages for Unverified Email Addresses in Djoser #738

Open kotaamo opened 1 year ago

kotaamo commented 1 year ago

Dear Djoser Community,

I'm implementing login authentication in my application using Djoser. Currently, a user creates an account by inputting their email address and password, and then completes the verification process by clicking the verification link sent to their email address.

I would like to enhance the user experience by sending a specific error message, something like "Email address is not verified", when a user attempts to login without having completed email verification.

Looking at the djoser/serializers.py, I noticed that when a user tries to login with an unverified email address, the same error message is returned as if it were a simple login error.

def validate(self, attrs):
    password = attrs.get("password")
    params = {settings.LOGIN_FIELD: attrs.get(settings.LOGIN_FIELD)}
    self.user = authenticate(
        request=self.context.get("request"), **params, password=password
    )
    if not self.user:
        self.user = User.objects.filter(**params).first()
        if self.user and not self.user.check_password(password):
            self.fail("invalid_credentials")
    if self.user and self.user.is_active:
        return attrs
    self.fail("invalid_credentials")

With this current implementation, whether the user input incorrect account details or they have not completed the email verification, both cases will return the "invalid_credentials" error message, which is defined as "No active account found with the given credentials".

Here are my questions:

1. Is it acceptable to modify the above validate function to display to the user that the authentication has not been completed when an unverified email user tries to log in?

2. I believe I could modify the validate function as shown below to distinguish between the two error cases, but I'm curious if there are reasons for not implementing it this way or if there are any risks involved:

3. In TokenCreateSerializer, INACTIVE_ACCOUNT_ERROR is defined as the error message for the case where account authentication has not been completed. Is there any particular reason why this is not being invoked?

default_error_messages = { "invalid_credentials": settings.CONSTANTS.messages.INVALID_CREDENTIALS_ERROR, "inactive_account": settings.CONSTANTS.messages.INACTIVE_ACCOUNT_ERROR, }

Any guidance would be greatly appreciated. Thank you in advance for your help.

Best regards, Kota Amo

tomwojcik commented 7 months ago
  1. Is it acceptable to modify the above validate function to display to the user that the authentication has not been completed when an unverified email user tries to log in?

One reason for using a generic "invalid_credentials" error message is to avoid giving away too much information about the account's state, which could potentially be used maliciously. For example, confirming that an email address is registered but not activated could be seen as a privacy leak.

  1. Return "inactive_account" error instead of "invalid_credentials" error only when self.user.is_active is False. This way, I believe I could differentiate between the case where the user inputted wrong account details and the case where the user has not completed email verification.

    def validate(self, attrs):
        password = attrs.get("password")
        params = {settings.LOGIN_FIELD: attrs.get(settings.LOGIN_FIELD)}
        self.user = authenticate(request=self.context.get("request"), **params, password=password)

        if not self.user:
            self.user = User.objects.filter(**params).first()
            if self.user and not self.user.check_password(password):
                self.fail("invalid_credentials")

        if self.user and not self.user.is_active:
            self.fail("inactive_account")

        if self.user:
            return attrs

        self.fail("invalid_credentials")

That does make sense. I'd be willing to accept a PR with these changes, but by default inactive_account needs to fail with the same error message as invalid_credentials.

  1. In TokenCreateSerializer, INACTIVE_ACCOUNT_ERROR is defined as the error message for the case where account authentication has not been completed. Is there any particular reason why this is not being invoked?

It seems to be a leftover from the ancient times.

https://github.com/sunscrapers/djoser/blob/610c78c527d4d2c9d2ec834de4df275851200bfd/testproject/testapp/tests/test_token_create.py#L51-L54

EDIT: I removed it https://github.com/sunscrapers/djoser/pull/806/commits/b02c422151035a2616102fbb9b48af28067f9c5d