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

Shouldn't do a unique check on auth.user.email #1

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

dasmith2
Copy link

Hey Pinda, thanks for socialregistration here. It's saving me a lot of work. I believe we shouldn't do a unique check on auth.user.email. First, there is no unique constraint on the email field. Second, we are adding this stuff to an existing site, so a common workflow will be for existing users to want to start using Facebook or whatever to log in. They should be able to use the same e-mail.

What do you think? Thanks again!
Dave

@dasmith2
Copy link
Author

I just committed another change that implements OpenIDStore.removeAssociation

In the process of testing my setup I was getting responses from google to remove associations which was causing a NotImplementedError on removeAssociation.

@dasmith2
Copy link
Author

Here's the message from Google:

Received "invalidate_handle" from server https://www.google.com/accounts/o8/ud

@pinda
Copy link
Owner

pinda commented Dec 14, 2010

Hi Dave,

Good point. At first I didn't know why I put it there in the first place, but now I remember.
The user who is already logged in and user of the site can connect with facebook and doesn't have to fill in his/her username and email since the user is already logged in. After that they are able to log in with their socialprofile.

Users who are not registered and login with facebook etc. are presented the form in which they are not able to fill in the email of an already existing user.

Please correct me if i'm wrong, in the case above the unique check would be nice to have.

Greets

Joeri

@dasmith2
Copy link
Author

Oh, that's a good idea. OK, here's the corner case I ran into. What if users already have multiple logins with the same e-mail? Then forms.py breaks with a MultipleObjectsReturned error here:

def clean_username(self):
    username = self.cleaned_data.get('username')
    try:
        user = User.objects.get(username=username)

You could fix the break with

def clean_username(self):
    username = self.cleaned_data.get('username')
    if User.objects.filter(username=username):
        raise forms.ValidationError(_('This username is already in use.'))
    return username

If we go that direction, we should probably have a better error message that explains how you can combine accounts.

        raise forms.ValidationError(_('It looks like you already have a login for this email. If you want to use this new login on your existing account, first log in to your old account. Then, use this new login and we will alter your existing account.'))

@pinda
Copy link
Owner

pinda commented Dec 14, 2010

I totally agree. Everytime I use socialregistration for a website. I present the user with the option to connect his profile to social media once the user is logged in. In that case the error message should lead them in that direction.

    raise forms.ValidationError(_('It looks like you already have a login for this email. Login with your username and password. Once logged in you can connect your profile to your social media.'))

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

Successfully merging this pull request may close these issues.

3 participants