Github Emails are not correctly populated on social login when user doesn't have a public email address

[czue]

Library Version: 64.0.0

Github auth does not populate the "email" field. It instead returns a list of email addresses. This causes a problem if you are using the email address as the username, as you will get an empty string for every github user who signs up (which causes unique constraint crashes every time after the first one).

I have been able to workaround this by adding this custom social account adapter:

class EmailAsUsernameSocialAdapter(DefaultSocialAccountAdapter):
    """
    Adapter that sets the user email to one of the email addresses if not set.
    """

    def populate_user(self, request, sociallogin, data):
        user = super().populate_user(request, sociallogin, data)
        if not user.email and sociallogin.email_addresses:
            primary_emails = [em for em in sociallogin.email_addresses if em.primary and em.verified]
            if len(primary_emails) == 1:
                user.email = primary_emails[0].email

        return user

However, I'm guessing it would be better if this were fixed at the Github provider level? Making the following change to extract_common_fields in the github provider class seems to work:

def extract_common_fields(self, data):
        email = data.get('email')
        if not email:
            primary_emails = [em for em in data.get("emails", []) if em["primary"] and em["verified"]]
            if len(primary_emails) == 1:
                email = primary_emails[0]["email"]

        return dict(
            email=email,
            username=data.get("login"),
            name=data.get("name"),
        )

Let me know if this is a suitable change for the library and I can submit a pull request.

[pennersr]

This causes a problem if you are using the email address as the username, as you will get an empty string for every github user who signs up (which causes unique constraint crashes every time after the first one).

Storing "email address as the username" is often a hack/workaround to have the username field populated with some form of content. That is not needed with allauth, it will automatically populate the username if needed. So don't do ACCOUNT_USER_MODEL_EMAIL_FIELD = "username" -- that is not going to work properly. Users can change their email address, and different users might even share the same unverified email address. You cannot store all of that as a username.

[czue]

Maybe I focused too much on the issue related to usernames. Is it not expected that the user.email would get populated? Currently it sets email addresses but ignores that field on the user model.

Also this appears to be a regression (or at least change of behavior) in a recent version, as this was working until I upgraded.

[pennersr]

Yes, allauth keep user.email in sync... but, that field does not have a unique constraint, so you won't run into the issue you mentioned.

[pennersr]

Also this appears to be a regression (or at least change of behavior) in a recent version, as this was working until I upgraded.

Earlier versions did not obtain all the email addresses from GitHub. So that is indeed a change in behavior, but it shouldn't be a breaking one, unless somehow your project is storing the email address in the username. That was not safe with earlier versions either because of above mentioned reasons...

[czue]

Hmm, it still feels like we're not in sync. Let me try again. Let's ignore usernames.

Based on my observed experience (and testing on two different Github accounts), previous versions of allauth would set user.email to the primary email address of the authed user, and the current version always sets it to an empty string.

This line in extract_common_fields appears to be attempting to set the email, but the source (data["email"]) does not appear to ever be populated - at least on the accounts I have seen/tested with.

On the other hand, data.emails is correctly populated, by these lines in extract_extra_data. But these are only used to attach EmailAddresses to the user. user.email is, best I can tell, always blank.

Are you sure that's expected behavior and not a regression? It certainly feels wrong to me. Hence the proposal to set user.email based on the primary, verified email address coming from emails.

Does that match your understanding of the issue and are you still saying that's the intended behavior?

[czue]

(Separately, and I hope I'm not overcomplicating things, it also creates email addresses of the form 12345+username@users.noreply.github.com, which according to you may not be a bug, but is certainly weird behavior. Let me know if I should make separate issue for that)

[pennersr]

Hmm, it still feels like we're not in sync. Let me try again. Let's ignore usernames.

Indeed. I think that is due to you mentioning this:

which causes unique constraint crashes every time after the first one

You will not have those crashes on user.email, unless you are using a non standard user.email field that adds a unique constraint.

But, if you are reporting that, in case of multiple emails, user.email is not properly kept in sync, that definitely is an issue. Let's reopen.

[czue]

Ok, cool.

But, if you are reporting that, in case of multiple emails, user.email is not properly kept in sync, that definitely is an issue. Let's reopen.

Yes, that is what I am claiming appears to happen based on my observations and testing.

[pennersr]

Are you sure that this issue is not due to something specific in your project? The case of multiple emails and populating user.email is covered here:

https://github.com/pennersr/django-allauth/blob/main/allauth/socialaccount/providers/github/tests.py#L104

Can you try a little bit of debugging to see what is going on on your end?

[pennersr]

How did you configure ACCOUNT_USER_MODEL_USERNAME_FIELD and ACCOUNT_USER_MODEL_EMAIL_FIELD ?

[czue]

@pennersr I left a comment on the diff that I think caused it here. I think the tests are passing because they don't actually exercise the logic of pulling the email from the secondary emails api, which my guess is the only reason it was working previously. So probably the tests should be updated as well?

Caveat: this is from a cursory reading of the code, I don't know the library well enough to know if this is actually right.

[czue]

How did you configure ACCOUNT_USER_MODEL_USERNAME_FIELD and ACCOUNT_USER_MODEL_EMAIL_FIELD ?

I haven't changed either of these from their defaults. Again, this just broke out from under me after upgrading (from 0.60.1) to 64.0. That would be timed right with it being introduced in https://github.com/pennersr/django-allauth/commit/bc9fa026d155cf6fb912be9128eaaf9b21daa3bd#r145989498

[czue]

I haven't fully bisected to confirm that's the exact commit that broke it, I'm just going by intuition. I can try downgrading to 63.0 and report if that also fixes it if that would be helpful.

[czue]

Ok, sorry for the barrage of updates. I think I figured it out. I was partly right. So this only an issue if the Github API returns null for email (which is quite common - it will be null if the user does not specify a public email even when getting the user's own profile. more here)

Previously - either by design or by accident - this was fine, because allauth would go through the process described in that SO post (loop through the user's private emails and get the right one). With the commit I linked above that behavior was removed. So now when someone auth's with github but does not have a public email, they will not have their email specified.

To me the pragmatic thing to do is keep the old behavior. If I auth with Github I expect it to use my primary email even if that is private. However I could see how a strict interpretation of the Github API behavior might lead one to conclude this is "expected" behavior.

Your thoughts?

[czue]

(btw if you agree this should get updated, I'd be happy to take a crack at a PR with tests. but don't want to go through that effort if it will be rejected)

[pennersr]

I am still not following exactly what is happening. What do you have configured for:

• SOCIALACCOUNT_QUERY_EMAIL
• ACCOUNT_EMAIL_REQUIRED

Because if these are True, https://api.github.com/user/emails is being queried, which should return private email addresses as well. In case of False, that does not happen, and the email field from the user payload is used, which can be null indeed.

[czue]

ACCOUNT_EMAIL_REQUIRED is True but it's not setting email.

Are you looking at the links I am putting in the comments? the emails API is getting called but it's not populating user.email anymore.

[czue]

To reproduce:

1. Set ACCOUNT_EMAIL_REQUIRED to True
2. Auth as a Github user without a public email.

The user.email field will not be populated, but EmailAddress objects will be created for the user.

[pennersr]

I think the tests are passing because they don't actually exercise the logic of pulling the email from the secondary emails api

They do. In that test case, 2 email addresses are retrieved from a mocked response, one of which gets assigned to user.email.

[pennersr]

Just to be sure, this...

which causes unique constraint crashes every time after the first one)

... is still a bit odd to me. That points to something project specific. Or is that no longer what is happening?

[czue]

They do. In that test case, 2 email addresses are retrieved from a mocked response, one of which gets assigned to user.email.

Huh, yeah that is odd. I can try and see if I can get spun up on the tests and figure out what's different between the two cases. From what I saw in the code I don't really understand how that test is passing (assuming there's no default for email in the mock request? The only difference i can see between the test and api response is that email is null not missing in the actual response from Github).

That points to something project specific.

Yeah that bit is project specific and you can ignore. This project is using emails as usernames.

[pennersr]

The only difference i can see between the test and api response is that email is null

I tried that, and that does not make any difference. Is there anything else at play? Do you have specific adapter code, dj-rest-auth involved, or are you saving the socialaccount/user instances manually? In the overall flow, at the time populate_user() gets called email is (with the new version) not present, but the allauth machinery does populate it before saving, unless it somehow gets bypassed because you save yourself.

[czue]

In the overall flow, at the time populate_user() gets called email is (with the new version) not present, but the allauth machinery does populate it before saving, unless it somehow gets bypassed because you save yourself.

Ah! This is what's going on. Where does that happen? I didn't realize there was further post-processing happening after this, but I can see now that that is in fact what's happening.

This is the account adapter that sets the username to the email which was what was raising the problem:

class EmailAsUsernameAdapter(DefaultAccountAdapter):
    """
    Adapter that always sets the username equal to the user's email address.
    """

    def populate_username(self, request, user):
        # override the username population to always use the email
        user_field(user, app_settings.USER_MODEL_USERNAME_FIELD, user_email(user))

I know you don't endorse emails-as-usernames, but if I want to require my users to login with an email address, is there a library-sanctioned way to achieve that? The above seemed to work smoothly until I ran into this issue.

I appreciate you working this through with me, and sorry about misleading you on the details of what was going on. (feel free to close this if you aren't planning on addressing this change in behavior, though it would be nice to know if there's a workaround, or if the socialadapter in the original comment is the right thing to do).

[pennersr]

See the documentation for populate_user():

        Note that the user instance being populated represents a
        suggested User instance that represents the social user that is
        in the process of being logged in.

        The User instance need not be completely valid and conflict
        free. For example, verifying whether or not the username
        already exists, is not a responsibility.

So, it's just populating the user fields with some suggestions that may or may not end up in the final user field. So, that's not a reliable place to make sure that username = email. If you truly want that (I would advise against it, as users can have multiple email addresses, change their email, etc.), the better place to do so would be in

    def save_user(self, request, sociallogin, form=None):

[pennersr]

if you aren't planning on addressing this change in behavior

I do think we need to look into this. Reopening.

[pennersr]

See: #4082

[czue]

Thank you for being patient through all the back and forth and speedy fix! Appreciate it.