Skip to content

Associate by mail doesn't return is_new flag #201

@gglentini

Description

@gglentini

Hi,
I'm using your package in my project, configured with Email and Facebook backends and with this auth pipeline

SOCIAL_AUTH_PIPELINE = (
    'social.pipeline.social_auth.social_details', 
    'social.pipeline.social_auth.social_uid',   
    'social.pipeline.social_auth.auth_allowed',     
    'social.pipeline.social_auth.social_user', 
    'social.pipeline.user.get_username', 
    'social.pipeline.social_auth.associate_by_email',
    'social.pipeline.user.create_user', 
    'profiles.pipeline.set_user_password',          
    'profiles.pipeline.mail_validation',
    'social.pipeline.social_auth.associate_user'
    'social.pipeline.social_auth.load_extra_data',
    'social.pipeline.user.user_details'
)

I am also using a custom User class where email field is unique too, and email = username.

I have found a problem with associate_by_email step.
Maybe it's done this way by design, but the method doesn't return the is_new flag when an user is found (and of course it should be set to False).

Let's suppose I have already created an user using the Email backend.
In my unit tests I was trying to signup again using the same email, and an exception was expected.
But since the user is found by associate_by_email, create_user step is properly skipped, while my set_user_password method inspired by the example in your documentation tries to reset user's password, because is_new value hasn't changed till the beginning.

I have found a workaround just creating a custom method adding the flag

def associate_by_email(strategy, details, user=None, *args, **kwargs):
    if user:
        return None

    email = details.get('email')
    if email:
        # Try to associate accounts registered with the same email address,
        # only if it's a single object. AuthException is raised if multiple
        # objects are returned.
        users = list(strategy.storage.user.get_users_by_email(email))
        if len(users) == 0:
            return None
        elif len(users) > 1:
            raise AuthException(
                strategy.backend,
                'The given email address is associated with multiple accounts'
            )
        else:
            return {'user': users[0],
                        'is_new': False}

This way I check for the password in set_user_password instead of trying to reset it, receiving an error (password not valid, even if it was not the one I was checking for, but seems more reasonable than resetting the password).

I could submit a pull request if you find this change acceptable.

Cheers.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions