Request Twitter email #452

SUPERCILEX · 2016-12-09T19:41:42Z

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2016-12-09T19:43:26Z

auth/src/main/java/com/firebase/ui/auth/IdpResponse.java

    private final String mProviderId;
-    @Nullable


I don't think this should be nullable: we always know the email address and provider id and since having it be nullable caused a crash, I made it @nonnull.

SUPERCILEX · 2016-12-09T19:47:29Z

auth/src/main/java/com/firebase/ui/auth/provider/TwitterProvider.java

+
+            @Override
+            public void failure(TwitterException exception) {
+                TwitterProvider.this.failure(exception);


This is a problem:

Even if the user grants access to her email address, it is not guaranteed you will get an email address. For example, if someone signed up for Twitter with a phone number instead of an email address, the email field may be empty. When this happens, the failure method will be called because there is no email address available.

It will be confusing to the user if they press authorize and the sign in just fails unexpectedly because they didn't give Twitter an email. The only place we need to display a message is in AuthMethodPickerActivity, but what should we say and how? A snackbar? An alert dialog?

This is a problem ... I need to look into this and ask someone who knows more than I do :-)

SUPERCILEX · 2016-12-09T20:04:20Z

@samtstern The twitter tests are failing because we are waiting for a callback that will never come (the ok from the user in twitter's request email activity). Because of that, I don't think it's possible to test the TwitterProvider, but it also doesn't look like we really need the tests: we manually call twitterProvider.success(result); and don't check anything after that:

@Override
public void onSuccess(IdpResponse idpResponse) {
    assertTrue(mAssertSuccess);
    mCountDownLatch.countDown();
}

Is it ok for me to remove the test? Or how could I get it to work with requestEmail?

Here's the test in question: https://github.com/firebase/FirebaseUI-Android/blob/master/auth/src/test/java/com/firebase/ui/auth/ui/provider/TwitterProviderTest.java#L68

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2016-12-10T22:21:55Z

auth/src/main/java/com/firebase/ui/auth/provider/TwitterProvider.java

@@ -60,8 +74,36 @@ public void startLogin(Activity activity) {
    }

    @Override
-    public void success(Result<TwitterSession> result) {
-        mCallbackObject.onSuccess(createIdpResponse(result.data));
+    public void success(final Result<TwitterSession> sessionResult) {


Twitter caused a leak by storing our callback in a result receiver:

In com.firebase.uidemo:1.0:1. * com.firebase.ui.auth.ui.idp.AuthMethodPickerActivity has leaked: * GC ROOT android.os.ResultReceiver$MyResultReceiver.this$0 * references com.twitter.sdk.android.core.identity.ShareEmailResultReceiver.callback * references com.firebase.ui.auth.provider.TwitterProvider$EmailCallback.this$0 * references com.firebase.ui.auth.provider.TwitterProvider.mCallbackObject * leaks com.firebase.ui.auth.ui.idp.AuthMethodPickerActivity instance * Retaining: 11 KB. * Reference Key: 1437fccd-e7e7-4aa3-9154-b8421f639999 * Device: Google google Pixel XL marlin * Android Version: 7.1 API: 25 LeakCanary: 1.5 00f37f5 * Durations: watch=5020ms, gc=196ms, heap dump=14858ms, analysis=156016ms

and their RequestEmail activity was bothering me because it's useless: devs already have access to the email since they requested it in the Twitter console. Sigh to twitter 😄

Anyway, I dug through the source code and ripped Twitter's GET request so that we can handle that stuff ourselves. Now if we didn't get an email, we know whether or not the dev hasn't requested the email permission in the twitter dashboard or if the Twitter user just doesn't have an email.

@samtstern Since we now have control over what's going on, we can decide what to do in the two failure cases.

@SUPERCILEX I appreciate all the investigation but I have a few reservations about this approach:

We are supposed to be implementing a best-practices UI layer on top of various auth SDKs, but this is clearly not what Twitter recommends as best practice.

This adds a lot of complexity by introducing Retrofit into our code (I know it was a dependency before, but now we are using it directly)

The benefit here is real, but it's not huge. I think I'd rather use the Twitter SDK as it stands and just deal with the ambiguity of their errors.

As for the leak, I think we can work around that ourselves by making the callback a static inner class with a WeakReference to the Activity.

@samtstern Whoa, you just blew my mind! The main reason I didn't want to use requestEmail was because of the leak, but now that you showed me how to use WeakReferences... Wow! This is why I love contributing to FirebaseUI so much: I've read a few Medium posts on weak references, but I need people like you to show me where I can apply the concepts. Anyway, thanks for the tip! (I had to go back and rewrite some code in my app to use weak references 😄)

SUPERCILEX · 2016-12-10T22:26:44Z

auth/build.gradle

@@ -50,6 +50,9 @@ dependencies {
    // library when updating com.android.support libraries:
    compile "com.android.support:cardview-v7:$support_library_version"

+    compile 'com.squareup.retrofit2:retrofit:2.1.0'
+    compile 'com.squareup.retrofit2:converter-gson:2.1.0'


No worries about these, Twitter's already includes them and I'm just upgrading it for them:

+--- com.twitter.sdk.android:twitter:2.3.0 | +--- com.twitter.sdk.android:tweet-composer:2.3.0 | | +--- com.twitter:twitter-text:1.13.0 | | +--- io.fabric.sdk.android:fabric:1.3.14 | | +--- com.twitter.sdk.android:twitter-core:2.3.0 | | | +--- com.google.code.gson:gson:2.6.1 -> 2.7 | | | +--- com.squareup.retrofit2:converter-gson:2.0.2 -> 2.1.0 <<<<<<<<<<<< | | | | +--- com.squareup.retrofit2:retrofit:2.1.0 | | | | | \--- com.squareup.okhttp3:okhttp:3.3.0 | | | | | \--- com.squareup.okio:okio:1.8.0 | | | | \--- com.google.code.gson:gson:2.7 | | | +--- com.squareup.okhttp3:okhttp:3.2.0 -> 3.3.0 (*) | | | +--- io.fabric.sdk.android:fabric:1.3.14 | | | \--- com.squareup.retrofit2:retrofit:2.0.2 -> 2.1.0 (*) | | \--- com.squareup.picasso:picasso:2.5.2 | +--- io.fabric.sdk.android:fabric:1.3.14 | +--- com.twitter.sdk.android:twitter-core:2.3.0 (*) | \--- com.twitter.sdk.android:tweet-ui:2.3.0 | +--- io.fabric.sdk.android:fabric:1.3.14 | +--- com.twitter.sdk.android:twitter-core:2.3.0 (*) | +--- com.android.support:support-v4:23.1.1 -> 25.0.1 (*) | \--- com.squareup.picasso:picasso:2.5.2

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2016-12-14T16:22:52Z

auth/src/main/java/com/firebase/ui/auth/provider/TwitterProvider.java

+
+        @Override
+        public void success(Result<String> emailResult) {
+            mCallbackObject.get().onSuccess(createIdpResponse(emailResult.data));


We need to check if mCallbackObject.get() is null here before calling onSuccess to avoid a NPE.

When it's null I guess we can just log a warning and do nothing, it means the activity waiting for the callback was dead so there's nowhere to deliver it.

@samtstern so does this mean that on a low memory device onSuccess won't be called?

samtstern · 2016-12-14T16:23:12Z

auth/src/main/java/com/firebase/ui/auth/provider/TwitterProvider.java

+            Log.e(TAG, "Failure retrieving Twitter email. " + exception.getMessage());
+            // If retrieving the email fails, we should still be able to sign in, but Smart Lock
+            // and account linking won't work.
+            mCallbackObject.get().onSuccess(createIdpResponse(null));


Same thing here.

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2016-12-14T20:09:40Z

OK I just checked out this PR at the latest commit and tried a number of situations:

Twitter account that has email
- When collision, it linked properly
- When I signed in and clicked "deny" on the Twitter email screen FirebaseUI still seemed to get my email. Deny and allow behaved the same for this account.
Twitter account that only has phone number
- The allow/deny email screen still showed up
- No matter which one I clicked, I obviously could not get an email from Twitter

So now I am wondering: when does this request email screen ever do anything? What sort of account setup would result in Firebase Auth only getting the email when that screen is presented.

SUPERCILEX · 2016-12-14T20:57:47Z

When I signed in and clicked "deny" on the Twitter email screen FirebaseUI still seemed to get my email. Deny and allow behaved the same for this account.

@samtstern do you mean the welcome back screen still showed up? The account linking/welcome back screen isn't showing up for me when I press deny on Twitter's email screen. If account linking didn't occur, but FirebaseUser#getEmail() still works, that's expected behavior because firebase auth is getting the Twitter email some other way.

The allow/deny email screen still shows up if the twitter user doesn't have an email

This is working according to Twitter's documentation: we have to show the email screen to know if the Twitter user has an email.

What sort of account setup would result in Firebase Auth only getting the email when that screen is presented.

That was one of the reasons I chose to use the retrofit hack, we can't. Firebase auth doesn't care what we do because they get the Twitter email some other way.

@samtstern Do you want to proceed with this PR or go back to the retrofit hack? Twitter does want us to use their request email activity, but it seems redundant in most cases because Firebase Auth gets the email anyway.

samtstern · 2016-12-14T21:17:46Z

@SUPERCILEX sorry maybe I am confused but I think my question is why do we need either the retrofit hack or the request email screen? It seems to me that Firebase Auth is already going to get us the email address whenever possible and all we need to do in FirebaseUI is handle the case where twitter email is null (which you've done)

SUPERCILEX · 2016-12-14T21:43:26Z

@samtstern Now I'm all confused too! 😄 From what I understand, we need the email in IdpResponse because of this code in CredentialSignInHandler:

if (!task.isSuccessful() && task.getException() instanceof FirebaseAuthUserCollisionException) {
    // At this point, the user isn't signed in right?
    mHelper.getFirebaseAuth()
            // If the user isn't signed in, this will always fail whether or not the Twitter user has an email because we can't use FirebaseUser#getEmail() yet.
            .fetchProvidersForEmail(mIdpResponse.getEmail())
            // ...
}

@samtstern For account linking, we need the email from IdpResponse before the user is logged in, right?

samtstern · 2016-12-14T22:03:39Z

@SUPERCILEX I think the account linking cases is not relevant here, right? Would our flow ever initiate account linking for an account with no email?

SUPERCILEX · 2016-12-14T22:39:30Z

@samtstern Well no, but this PR was originally created to handle the case where a Twitter user with an email tries to link their account: user creates email account -> user tries to sign into Twitter with the same email they created their email account with -> we need the email before the user is signed in to perform account linking.

samtstern · 2016-12-15T19:41:05Z

I finally get it! Your last comment turned on the lightbulb for me. I'm going to summarize below just in case I forget again:

User signs up with email [email protected] using either email/pw or some non-Twitter IDP
User tries to sign up with a Twitter account with that email.
We acquire a Twitter auth credential. This does not have the email address so we can't check for providers.
We launch into the sign in flow and get a FirebaseAuthUserCollisionException because the Firebase auth backend used our Twitter token to get the email address and saw a collision that we didn't see.

But with your change we have:

3a. Use the twitter SDK to request the email and check for collisions, launching the welcome back flow if necessary.

Now that I finally understand the edge case here I think the way you are doing it now (launching the twitter email screen) is the correct approach.

SUPERCILEX · 2016-12-15T23:09:56Z

@samtstern do you want to wait for your email refactor to go in before merging this PR or are there a few changes that still need to be addressed before merging?

samtstern · 2016-12-15T23:29:44Z

Nope this LGTM!

SUPERCILEX · 2016-12-15T23:36:24Z

Awesome!

Officially request Twitter email

c40fa42

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX commented Dec 9, 2016

View reviewed changes

SUPERCILEX added 6 commits December 9, 2016 15:50

Cleanup + remove extra Twitter tests

d81b5f9

Signed-off-by: Alex Saveau <[email protected]>

Cleanup

7af8e9b

Signed-off-by: Alex Saveau <[email protected]>

Merge remote-tracking branch 'origin/fix-NPE' into twitter

4dec62e

Cleanup

7d7c582

Signed-off-by: Alex Saveau <[email protected]>

Bypass twitter get email activity

ed4b11b

Signed-off-by: Alex Saveau <[email protected]>

Update Twitter dependency

bff53a5

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX commented Dec 10, 2016

View reviewed changes

SUPERCILEX added 4 commits December 13, 2016 16:43

Fix memory leak and use default Twitter request email activity

24a2628

Signed-off-by: Alex Saveau <[email protected]>

Make it okay to not receive an email

086e5dd

Signed-off-by: Alex Saveau <[email protected]>

Cleanup

639b9d1

Signed-off-by: Alex Saveau <[email protected]>

Merge remote-tracking branch 'firebase/version-1.1.0-dev' into twitter

f537d1b

samtstern reviewed Dec 14, 2016

View reviewed changes

Check for null callback

6695453

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX mentioned this pull request Dec 15, 2016

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

Closed

samtstern merged commit c52387e into firebase:version-1.1.0-dev Dec 15, 2016

SUPERCILEX deleted the twitter branch December 15, 2016 23:36

SUPERCILEX mentioned this pull request Dec 21, 2016

Twitter provider with same email issue #466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request Twitter email #452

Request Twitter email #452

SUPERCILEX commented Dec 9, 2016

SUPERCILEX Dec 9, 2016

SUPERCILEX Dec 9, 2016

samtstern Dec 10, 2016

SUPERCILEX commented Dec 9, 2016

SUPERCILEX Dec 10, 2016

samtstern Dec 13, 2016

SUPERCILEX Dec 14, 2016

SUPERCILEX Dec 10, 2016

samtstern Dec 14, 2016

SUPERCILEX Dec 14, 2016

samtstern Dec 14, 2016

samtstern commented Dec 14, 2016

SUPERCILEX commented Dec 14, 2016

samtstern commented Dec 14, 2016

SUPERCILEX commented Dec 14, 2016

samtstern commented Dec 14, 2016

SUPERCILEX commented Dec 14, 2016

samtstern commented Dec 15, 2016

SUPERCILEX commented Dec 15, 2016

samtstern commented Dec 15, 2016

SUPERCILEX commented Dec 15, 2016

Request Twitter email #452

Request Twitter email #452

Conversation

SUPERCILEX commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samtstern commented Dec 14, 2016

SUPERCILEX commented Dec 14, 2016

samtstern commented Dec 14, 2016

SUPERCILEX commented Dec 14, 2016

samtstern commented Dec 14, 2016

SUPERCILEX commented Dec 14, 2016

samtstern commented Dec 15, 2016

SUPERCILEX commented Dec 15, 2016

samtstern commented Dec 15, 2016

SUPERCILEX commented Dec 15, 2016