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

Please do not force email scope for Google Sign In #1899

Closed
u201701 opened this issue Jan 6, 2021 · 5 comments
Closed

Please do not force email scope for Google Sign In #1899

u201701 opened this issue Jan 6, 2021 · 5 comments

Comments

@u201701
Copy link

u201701 commented Jan 6, 2021

Please do not force the user to request email scope when signing in with Google, as mandated by the red-circled line of code in the image.

image

The backward-compatible solution would be to add a similar method with a different name, say setScopesOnly or setScopesExactly, and have that method not include the .requestEmail() call.

Google itself makes it optional to request email, so this option should naturally be exposed in this library since there is no additional cost.

The rationale is that many users would be more comfortable logging in with Google if they are assured that the app will not receive their email address.

Since this is trivial, please implement it soon -- the only decision is what to call the name of the function, and then I could submit a pull request.

Thank you

@samtstern
Copy link
Contributor

@u201701 actually we can't remove this. Firebase Auth uses the user's email address as the primary key (or phone number for Phone Auth) so when we do social login with Google, Twitter, etc we need to request the user's email. This is not a FirebaseUI issue it's just a principle of using Firebase Auth, you can see that the docs here recommend the same:
https://firebase.google.com/docs/auth/android/google-signin

@u201701
Copy link
Author

u201701 commented Jan 6, 2021

@samtstern I am quite sure you are mistaken, and I hope you could discuss with the larger engineering team. Here are 6 points that I offer as compelling evidence:

  • Per the Firebase docs, the email is not a primary key The primary key is the "unique ID". I believe you misread or misconstrued "primary email" as primary key, and the two are very different. The unique ID is the one field that is necessary, while the others, notably "photo URL" which has equal standing to "primary email" may be left unfilled (null).

Firebase users have a fixed set of basic properties—a unique ID, a primary email address, a name and a photo URL—stored in the project's user database, that can be updated by the user (iOS, Android, web).

  • Consider that anonymous login definitely does not have any email associated, but it is still stored in the Firebase user table

If you still don't believe me, let's look at what your team's code does for other providers that do offer the choice of email address:

  • Facebook has an Oauth scope for reading email (they call it "permission"), but it is optional and one can authenticate without requesting a user's email.
    image

  • Now let's look at the setPermissions code (setScopes equivalent) for the FacebookBuilder. Unlike for the GoogleBuilder you do not force your user to request the permission to read email (it's definitely optional and not included in the empty scope).
    image

  • If that's not enough, the exact same thing is true for the team's code for the GithubBuilder There is a specific permission:
    image

  • and you don't force the user to request it:
    image

  • I could go on with other providers, but I hope that's not necessary. Your code is only forcing the read-email for the GoogleBuilder and nothing else. The email is not the primary key to any table, and the Firebase system works perfectly fine without email, as I have documented above.

Please reopen and add this tiny fix. Requesting a user's email address is pretty significant and that is why even Google (both Google Oauth and Firebase) do not force it in their auth systems.

This is an oversight and I hope you allow the use of your valuable library without requiring library-users to not accommodate their end-users privacy preferences.

@samtstern
Copy link
Contributor

@u201701 thank you for all of the feedback! So if you have the "one account per email address" setting turned on in the Firebase console (which most people do) then we do require the email scope.

Due to #1621 we made this the default behavior. We're not going to reverse that decision now, but you can override it using setSignInOptions rather than using setScopes which gives you complete control over the GoogleSignInOptions:

public GoogleBuilder setSignInOptions(@NonNull GoogleSignInOptions options) {

@u201701
Copy link
Author

u201701 commented Jan 6, 2021

@samtstern Thank you for your receptivity and your suggestion to use setSignInOptions as a workaround.

But, not only does the workaround not work, but hmm..., there's also a puzzle on our hands. I went through all the discussion, but I can't see how #1755 fixes #1621 setSignInOptions too necessarily calls requestEmail on line 1107 shown below, and setScopes necessarily calls setSignInOptions so the fix in #1755 seems redundant.

builder.requestEmail().requestIdToken(getApplicationContext()

The build method too forces a call to requestEmail by calling setScopes and indirectly setSignInOptions if neither setScopes nor setSignInOptions was explicitly called. All of this code I mention existed prior to #1755

Unfortunately, I am still stuck. We need a setSignInOptionsMin that does not include the requestEmail call. (I tried subclassing and overriding in vain because it is heavily protected.)

I realize most people do not need this, but I'm just the first in a minority for whom it is reasonable to prioritize not requesting email to encourage users to signup with Google. Since there is a reasonable fix to add the setSignInOptionsMin method without disturbing anything else in the class hierarchy or existing methods, I'd appreciate it if this could be considered reasonable.

@samtstern
Copy link
Contributor

This was fixed and released in version 7.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants