Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: SocialLoginSerializer.validate() checks if email exists, however complete_social_login from allauth already created the user, always ending up in error state #650

Open
Meess opened this issue Aug 22, 2024 · 2 comments

Comments

@Meess
Copy link

Meess commented Aug 22, 2024

Config

Using all-auth social login with Google

# Pipfile
dj-rest-auth = "==6.0.0"
django-allauth = {extras = ["socialaccount"], version = "==0.63.6"}

Using the following settings:

# base_settings.py
ACCOUNT_LOGOUT_ON_GET = True
SOCIALACCOUNT_AUTO_SIGNUP = True
ACCOUNT_USERNAME_REQUIRED = False
ACCOUNT_USER_MODEL_USERNAME_FIELD = 'username'
SOCIAL_ACCOUNT_USER_MODEL_USERNAME_FIELD = 'username'
ACCOUNT_EMAIL_REQUIRED = True
SOCIALACCOUNT_EMAIL_REQUIRED = True
ACCOUNT_AUTHENTICATION_METHOD = 'email'
SOCIALACCOUNT_AUTHENTICATION_METHOD = 'email'

Functional Issue

Not signed up user has to login twice before actually logged in.

The first time a user logs in always results in a ValidationError with message 'User is already registered with this e-mail address.'. However the user is actually created in de db, even though the error occurs. If the user logs in again, it recognises the user in the database and logs you in with status 200 (i.e. user is actually logged in).

Issue

SocialLoginSerializer.validate(..) itself checks if the e-mail adress is already in use on signup, however prior to that is call the all-auth function complete_social_login which already checks this, and saves the user:

SocialLoginSerializer.validate(...) calls allauth's complete_social_login

# /dj_rest_auth/registration/serializers.py
class SocialLoginSerializer(serializers.Serializer):
    def validate(self, attrs):
            ...
            ret = complete_social_login(request, login)

########### START callstack into allauth ###########
# /allauth/socialaccount/helpers.py
def complete_social_login(request, sociallogin):
    ...
    flows.login.complete_login(request, sociallogin)

# /allauth/socialaccount/internal/flows/login.py
def complete_login(request, sociallogin, raises=False):
    ...
    return _authenticate(request, sociallogin)

def _authenticate(request, sociallogin):
    ...
    ret = process_signup(request, sociallogin)

# /allauth/socialaccount/internal/flows/signup.py
def process_signup(request, sociallogin):
    ...
    # Here the user get's saved by allauth, and returns to where dj-rest-auth's SocialLoginSerializer.validate
    # called complete_social_login (where it goes wrong.)
    get_adapter().save_user(request, sociallogin, form=None)
########### END callstack into allauth ###########

# /dj_rest_auth/registration/serializers.py
class SocialLoginSerializer(serializers.Serializer):
    def validate(self, attrs):
            ...
            # Back in dj-rest-auth validation, it tries to validate if the email adress is unique, however
            # the user was just created by all-auth with exactly this email adress, so it will throw an error
            # that the adres is already in use. As the user is already created, a new login follows the login flow
            # instead of the signup flow, and therefore the user can login without issue on the second login
            account_exists = get_user_model().objects.filter(
                    email=login.user.email,
                ).exists()

Possible solution

The returned login variable login.is_existing is checked to see if a user already existed, if not it goes and checks the email adress. However login.user can also be checked, as allauth fills login.user with the created user object on signup, dj-rest-auth could check this and see if it already has an ID. If it has an ID it knows the user is already created (and then assume allauth did proper validation) and hence skip the email check.

Workaround (hacky)

Create a custom SocialLoginSerializer, copy only yhe validate function from the original code and comment out the email check.

# custom_social_login_serializer.py
class CustomSocialLoginSerializer(SocialLoginSerializer):
    def validate(self, attrs):
            ...
            # if allauth_account_settings.UNIQUE_EMAIL:
            #     # Do we have an account already with this email address?
            #     account_exists = get_user_model().objects.filter(
            #         email=login.user.email,
            #     ).exists()
            #     if account_exists:
            #         raise serializers.ValidationError(
            #             _('User is already registered with this e-mail address.'),
            #         )

# social_providers.py
from dj_rest_auth.registration.views import SocialLoginView
from allauth.socialaccount.providers.google.views import GoogleOAuth2Adapter
from allauth.socialaccount.providers.oauth2.client import OAuth2Client
from .custom_social_login_serializer import CustomSocialLoginSerializer

class GoolgeAuth(SocialLoginView):
    adapter_class = GoogleOAuth2Adapter
    client_class = OAuth2Client
    serializer_class = CustomSocialLoginSerializer
@benshaji-sequoiaat
Copy link

benshaji-sequoiaat commented Oct 28, 2024

Hi, it seems this is a similar issue: #658

@Meess
Copy link
Author

Meess commented Nov 22, 2024

Hi, it seems this is a similar issue: #658

@benshaji-sequoiaat yes look like a similar issue. As I mentioned in this issue there is a hacky workaround.

I have little trust this will be solved as the README.md especially mentions:

This project has optional and very narrow support for Django-AllAuth. As the maintainer, I have no interest in making this package support all use cases in Django-AllAuth. I would rather focus on improving the quality of the base functionality or focus on OIDC support instead. Pull requests that extend or add more support for Django-AllAuth will most likely be declined. Do you disagree? Feel free to fork this repo!

I'm looking into all-auth headless to replace this library: https://docs.allauth.org/en/latest/headless/index.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants