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

[WIP] fix: remove email dependency for oauth providers #414

Closed
wants to merge 2 commits into from

Conversation

kangmingtay
Copy link
Member

What kind of change does this PR introduce?

  • Fixes Oauth fails when no email is linked to account  #214
  • TLDR: Some oauth providers (e.g. twitter) do not require their users to provide an email address when they sign up. Gotrue currently restricts oauth logins to user accounts with at least a verified email address associated to it.

To-be-addressed

  • Should gotrue auto-confirm users with no email addresses associated to their oauth account?
  • Should gotrue have a setting in the config for the above?

Details

We are changing the orange flow to allow new users to be created without having to provide a verified email.

image

Copy link

@lunandd lunandd left a comment

Choose a reason for hiding this comment

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

I don't know Go, but the code looks alright IMO

@elliottetzkorn
Copy link

Is this going to be merged?

Copy link
Contributor

@nwneisen nwneisen left a comment

Choose a reason for hiding this comment

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

I left a comment on this. I'm happy to try it out with the Snapchat OAuth if you would like some additional testing.

EmailVerified: true,
EmailVerified: emailVerified,
CustomClaims: map[string]interface{}{
"needs_phone_verification": u.NeedsPhoneVerification,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think requiring any kind of verification based on OAuth values might not be possible. In the case with snapchat, there is no phone or email information. Some might also say that being able to sign in to an OAuth provider counts as a form of verification.

I think email and phone verification will need to be thought of as a separate process and it should be easy to check if a user is verified or not. Someone using GoTrue could then require users to verify before granting additional permissions.

@nwneisen nwneisen mentioned this pull request Apr 23, 2022
6 tasks
@bnjmnt4n
Copy link
Contributor

I think adding a setting for confirming OAuth users without email would be a good choice.

@smndtrl
Copy link

smndtrl commented Jul 24, 2022

I wanted to chime in here as this whole situation stops me from going on using Sign in with Apple as well.

I see the goal of gotrue to establish the identity of an authenticating user.

What in the end identifies a user is a pair of provider,subject as in provider says the authentication user is subject. Not more and not less. Thus requiring additional claims made the the provider e.g. email or phone number is something that an application requiring that information may opt in to on an app by app basis.

The identity (aka the arbitrary value a provider assigns to the user inside his system) has been established with reading the subject from the providers token response. How a applications handles additional verification steps should not block this stream of enabling authentication regardless of what provider claims get send in addition to the subject

@nico-herrera
Copy link

when is this going to be merged?

@kangmingtay
Copy link
Member Author

kangmingtay commented Sep 6, 2022

Hey everyone, apologies for the late update but this change is more complex than it looks and will involve a breaking change on the database schema.

We can't remove the requirement for an email to be present for an oauth provider because of the default automatic linking behaviour [1] that gotrue supports. If we remove the automatic linking behaviour, the following scenario can happen:

user   | email           | oauth provider sign-in
User A | foo@example.com | google sign-in
User B | ""              | twitter sign-in

If user B updates their twitter account to use foo@example.com, we would have an inconsistent behaviour because we do not and cannot [2] update the users.email for user B to foo@example.com.

We have also discussed removing the unique constraint on the users.email column so the above scenario becomes possible. However, that will break the automatic linking behaviour because the following scenario can occur:

user   | email           | oauth provider sign-in
User A | foo@example.com | google sign-in
User B | foo@example.com | twitter sign-in

# new user signs-in with discord
User C | foo@example.com | discord sign-in

Gotrue will not be able to decide whether User C should link to User A or B because both users have the same email address. At this point, we are discussing how we can continue to support this automatic linking behaviour but making it configurable rather than a default (and only) option. Until then, the current version of gotrue will continue to restrict users signing in with an oauth provider to require an email address.

We are also open to future suggestions and ideas on how we can workaround this issue. If it doesn't involve a breaking change or compromise security, we will be happy to review it.

[1]: Automatic linking is a feature which allows you to login as the same user using multiple oauth providers. As long as the oauth provider account are all associated to the same primary and verified email address, it will be linked to the same user.

[2]: We can't update the users.email column because of the unique constraint on the column. Since User A's users.email is foo@example.com, there will be a violation of the unique constraint.

@johndpope
Copy link

PREFACE -
I'm totally ok if I can make a sql migration to my own instance and get this working with some hack code.

Background requirement
I have authorised users - and want to associate additional social logins so I can query that provider
eg. they sign up via email - now add facebook login (not same facebook email) - so we can get their friends -
or google - get youtube likes etc.

This user flow - doesn't need to impinge on existing login - it could be completely separate
eg.
/user/associateSocialLogin?provider=facebook
sign in flow
oauth return - associate account with authorized user

It could even be another table associated_identities [facebook - token etc]

I asked question here - and while I apprecciate response from @GaryAustin1 - I need to solve this problem to proceed with app.

supabase/supabase#8897

I'll keep digging -

@antoniusnaumann
Copy link

I am currently facing this problem as well, as accessing users' email addresses is unacceptable for my use case.

@kangmingtay At least to me, automatic linking is an undesired and surprising behavior in the first place. If I rely on trusted third parties for authentication, I would assume that I then also rely on their mechanisms for providing a stable user id instead of relying on e-mail address and guessing potential links to other accounts created via third-party providers.

How is the above example currently handled when a user signed in via Twitter updates an existing email address?

@nwneisen
Copy link
Contributor

A solution for this might be for someone to put up a diff that allows the email requirement to be removed based on a configuration option. The email can be required by default but those who need to remove it can make the change in their configuration file. Authentication methods that don't have an email can also check for this setting as requirement if they are enabled.

@johndpope
Copy link

johndpope commented Sep 19, 2022

one critical thing to keep in mind -
submitting ios apps to appstore these days with say facebook / google - they will ask to implement apple login too.
Apple login out of the box gives end user option to obfuscate their email address bob@bob.com -> 1234.43223434@apple.com - so the premise of just using email as primary key is flawed.

@kangmingtay
Copy link
Member Author

Hey everyone,

At least to me, automatic linking is an undesired and surprising behavior in the first place

@antoniusnaumann we plan to change this behaviour in v2. Ideally, there would be a sensible default (no automatic linking) and we would expose a custom webhook functionality to change the linking logic based on how you want it to be.

A solution for this might be for someone to put up a diff that allows the email requirement to be removed based on a configuration option.

@nwneisen This can be problematic because it's not feasible to toggle between automatic linking and manual linking. It's tricky to differentiate which accounts are manually linked and which are automatically linked and apply the linking logic based on looking at emails alone.

Btw, thanks for being involved in the discussion and for providing these suggestions. The replies have been slow as we're preoccupied with working on other features such as MFA, SAML and multi-tenancy. I'll be closing this PR for now since it won't be merged and it does not solve the problems mentioned in #414 (comment)

@johndpope
Copy link

you mention v2 - is that more than 6 months away? I need this resolved in days or weeks. Prepared to pay someone to fix this. I love supabase but this is a blocker.

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.

Oauth fails when no email is linked to account
9 participants