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

fix: Always fetch emails when QUERY_EMAILS is set #4037

Merged

Conversation

mecampbellsoup
Copy link
Contributor

I encountered this situation by having the following context.

First, I have this social account configuration:

SOCIALACCOUNT_LOGIN_ON_GET = True
SOCIALACCOUNT_EMAIL_AUTHENTICATION = True
SOCIALACCOUNT_EMAIL_AUTHENTICATION_AUTO_CONNECT = True

SOCIALACCOUNT_PROVIDERS = {
    "google": {
        "APP": {
            "client_id": getenv("GOOGLE_OAUTH_CLIENT_ID"),
            "secret": getenv("GOOGLE_OAUTH_CLIENT_SECRET"),
        }
    },
    "github": {
        "APP": {
            "client_id": getenv("GITHUB_OAUTH_CLIENT_ID"),
            "secret": getenv("GITHUB_OAUTH_CLIENT_SECRET"),
        }
    },
}

Next, I took these steps:

  1. Register a new user w/ email and password, foo@bar.com.
  2. I configured a GH OAuth application and provider.
  3. I hit the github_login view to initiate the process of linking my GitHub user w/ email address foo@bar.com to my local user account.
  4. Instead of the existing user having the GH socialaccount data merged/associated w/ it, allauth was trying to register a fresh user with the same email address, which triggered the "account already exists" email flow.

I inspected the data I was getting back from GH, and because the fetched profile had an "email" key in it, the self.get_emails() call is never made. Since I don't have a provider setting on the "github" provider configuration above, allauth seems to begin new user registration process instead of e.g. querying the DB for the given email address to see if it exists yet or not.

This PR should be completely backwards compatible w/ current behavior, but in some cases it will result in an extra API call to fetch the user's email addresses. I will leave a comment w/ an alternative option in the diff.

Related to #3745.

@coveralls
Copy link

coveralls commented Aug 15, 2024

Coverage Status

Changes unknown
when pulling 10b6ee8 on mecampbellsoup:mc/github-provider-fetch-emails
into ** on pennersr:main**.

@pennersr
Copy link
Owner

Note that in your scenario allauth (correctly) starts a new registration because email authentication cannot be used. The provider did provide an email, but as that email is not marked as verified, that email cannot be trusted.

@mecampbellsoup
Copy link
Contributor Author

Note that in your scenario allauth (correctly) starts a new registration because email authentication cannot be used. The provider did provide an email, but as that email is not marked as verified, that email cannot be trusted.

The code makes it impossible. The profiles_url which makes a GH API call to the /user endpoint never contains anything except for a key called "email" which contains the email address. To get the other properties including verified and primary, the emails_url call must be made as GH's user/emails endpoint is what has that data.

@mecampbellsoup mecampbellsoup force-pushed the mc/github-provider-fetch-emails branch 2 times, most recently from 9d662cb to aef1128 Compare August 16, 2024 11:34
Copy link
Owner

@pennersr pennersr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, though it breaks the build on typing related constructs. Can you look into these? Thanks.

@mecampbellsoup mecampbellsoup force-pushed the mc/github-provider-fetch-emails branch from aef1128 to 5e8feba Compare August 16, 2024 12:23
@mecampbellsoup mecampbellsoup force-pushed the mc/github-provider-fetch-emails branch from 5e8feba to 10b6ee8 Compare August 16, 2024 12:42
@pennersr pennersr merged commit e78a188 into pennersr:main Aug 16, 2024
36 checks passed
@mecampbellsoup mecampbellsoup deleted the mc/github-provider-fetch-emails branch August 16, 2024 12:50
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