[CRASH + DISCUSSION] Should we request Twitter email transparently?

[SUPERCILEX]

While working on another PR, I came across this crash:

12-03 12:05:15.217 16688-16688/com.firebase.uidemo E/AndroidRuntime: FATAL EXCEPTION: main
                                                                     Process: com.firebase.uidemo, PID: 16688
                                                                     java.lang.IllegalArgumentException: Given String is empty or null
                                                                         at com.google.android.gms.common.internal.zzac.zzdv(Unknown Source)
                                                                         at com.google.firebase.auth.FirebaseAuth.fetchProvidersForEmail(Unknown Source)
                                                                         at com.firebase.ui.auth.ui.idp.CredentialSignInHandler.onComplete(CredentialSignInHandler.java:73)
                                                                         at com.google.android.gms.tasks.zzc$1.run(Unknown Source)
                                                                         at android.os.Handler.handleCallback(Handler.java:751)
                                                                         at android.os.Handler.dispatchMessage(Handler.java:95)
                                                                         at android.os.Looper.loop(Looper.java:154)
                                                                         at android.app.ActivityThread.main(ActivityThread.java:6088)
                                                                         at java.lang.reflect.Method.invoke(Native Method)
                                                                         at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
                                                                         at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

It's caused by this line of code and occurs if a user has an existing account and there is a FirebaseAuthUserCollisionException.

To solve this problem, there are two solutions:

1. Check for a null email in CredentialSignInHandler and fail the sign in if the email is null.
2. Use TwitterAuthClient#requestEmail to show the request email activity.

I prefer solution 2 because we are being more transparent about requesting a user's email, but it requires more steps and a Twitter user might not have an email: https://docs.fabric.io/android/twitter/log-in-with-twitter.html#request-user-email-address

@samtstern @amandle What should we do?

[SUPERCILEX]

I also just realized that with method 2, devs must follow the steps described in #433.

[samtstern]

@SUPERCILEX does this crash happen currently or only in the context of the new PR you're working on?

[SUPERCILEX]

It exists in master: create an email account using your twitter email -> log out -> log in using twitter to trigger the account linking.

[bobshao]

Hi @SUPERCILEX, I face the same issue. Do we have any solution or plan?

[SUPERCILEX]

Hi @bobshao, I created this issue because I was unsure how to proceed. @samtstern What do you think we should do?

[samtstern]

We should probably go with option #2. Does it require any additional developer configuration or just work on our side to integrate the flow?

[SUPERCILEX]

@samtstern It would require developers to follow the steps described in #433 or we would have to cancel the sign in. Other than that, it should be fairly easy to integrate.

[samtstern]

Aren't those steps already required to get Twitter sign in working in general? Or am I missing a distinction?

[SUPERCILEX]

No, without this change you could sign in with twitter and you just wouldn't receive an email. Now we would be forcing devs to request the email.

[samtstern]

That seems reasonable to me, as it stands Firebase Auth requires email as the unique key so we need to enforce that data from IDPs. Same reason we make Facebook request the email scope. This change would just be formalizing the requirement.

[SUPERCILEX]

@samtstern Sounds good! I'll see if I can a PR in today.

[samtstern]

@SUPERCILEX I just created two Twitter test accounts, one with phone number only and one with email only. I then used the quickstart-android project to try signing in with each one. This code doesn't have any special flow to ask Twitter for the email.

In each case sign in succeeded, the only different being the result of FirebaseUser.getEmail() after sign in. For one account it was null and for the other account it was the email as expected.

So I'm curious: in what situation will a Twitter account that does have an email associated with it not return an email during the normal sign in flow? That's the case that's relevant for our purposes.

[SUPERCILEX]

@samtstern I'm pretty sure that will only ever happen if the dev doesn't request the email permission in the Twitter console. However, I still think we should be concerned with Twitter users not having an email since we will still have to fail the sign in even after they press "Authorize". Because of that, I'm more concerned about the latter case... What should we do when a user doesn't have an email?

[samtstern]

I guess the question is do we really have to fail the sign in? Firebase Auth without FirebaseUI doesn't, so could we make FirebaseUI more lenient here? Yes we'd have to skip some features in the flow (like SmartLock) but we should still be able to get the user signed in.

As for the developer not requesting email permission in the console, I think compensating for that with additional UI or reverse-engineered HTTP requests could be overcompensating. After all, it's a developer error.

[SUPERCILEX]

The only place we really need an email is for account linking: a Twitter user who tries to log in using another provider will not have their Twitter account linked with said other provider. @samtstern If you think that is a reasonable trade-off, then no, we don't have to fail the sign in. What should we do?

[samtstern]

@SUPERCILEX can we attempt account linking only when the twitter user has an email and otherwise skip the linking check/attempt? That seems very reasonable to me, do you agree?

[SUPERCILEX]

Sounds good, we'll just have to check for null emails in CredentialSignInHandler.

[cutiko]

I'm not sure if it is appropriate to interrupt here but I got this problem now and I want to tell you about my experience in case it could help.

In my case the problem happened when the email was requested for Twitter but the email was already in use. Initially the login was done using google and then later using Twitter, the email is the same in both. The problem was fixed by deleting the google user.

Personally I think that forcing us (developers) to ask for Twitter email is ok, actually even better that ok, you should do it. You should force us to ask for Twitter email permission. This way the accounts created by any provider are always consistent. For adding Google login we must get the SHA1s (debug and release), there is nothing wrong into forcing us to do any step as long it create a quality login experience for the end user and sustainable for devs.

And if the email is already used, the user should be warn about it and redirected to the corresponding option. By example, if email/password login is enabled, and the user previously signed with that email, using Facebook or Google, the user is remind about and redirected. It should be the same behaviour.

Really sorry if I'm out of place, just trying to help.

[SUPERCILEX]

@cutiko You're totally fine, that's why FirebaseUI has public issues! 😄

In my case the problem happened when the email was requested for Twitter but the email was already in use. Initially the login was done using google and then later using Twitter, the email is the same in both.

@samtstern Hopefully my comment and the above will help you understand why we need the email. Adding the null check in CredentialSignInHandler fixes the crash for Twitter users without an email, but we still need the email before the user is signed to perform linking for Twitter users with an email.

@cutiko Yes, the plan is to force you guys to request the email permission (or at least if you don't neither account linking nor Smart Lock will work).

The fix should be out by FirebaseUI v1.1.0! 🎉

[samtstern]

A fix for this issue has been released in version 1.1.0