Skip to content

Request Twitter email #452

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Dec 15, 2016
Merged

Request Twitter email #452

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c40fa42
Officially request Twitter email
SUPERCILEX Dec 9, 2016
d81b5f9
Cleanup + remove extra Twitter tests
SUPERCILEX Dec 9, 2016
7af8e9b
Cleanup
SUPERCILEX Dec 10, 2016
4dec62e
Merge remote-tracking branch 'origin/fix-NPE' into twitter
SUPERCILEX Dec 10, 2016
7d7c582
Cleanup
SUPERCILEX Dec 10, 2016
ed4b11b
Bypass twitter get email activity
SUPERCILEX Dec 10, 2016
bff53a5
Update Twitter dependency
SUPERCILEX Dec 10, 2016
24a2628
Fix memory leak and use default Twitter request email activity
SUPERCILEX Dec 14, 2016
086e5dd
Make it okay to not receive an email
SUPERCILEX Dec 14, 2016
639b9d1
Cleanup
SUPERCILEX Dec 14, 2016
f537d1b
Merge remote-tracking branch 'firebase/version-1.1.0-dev' into twitter
SUPERCILEX Dec 14, 2016
6695453
Check for null callback
SUPERCILEX Dec 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions auth/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ Twitter app as reported by the [Twitter application manager](https://apps.twitte
</resources>
```

In addition, if you are using Smart Lock or require a user's email, you must enable the
"Request email addresses from users" permission in the "Permissions" tab of your app.
In addition, you must enable the "Request email addresses from users" permission
in the "Permissions" tab of your Twitter app.

## Using FirebaseUI for Authentication

Expand Down
43 changes: 20 additions & 23 deletions auth/src/main/java/com/firebase/ui/auth/IdpResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import android.content.Intent;
import android.os.Parcel;
import android.os.Parcelable;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;

import com.firebase.ui.auth.ui.ExtraConstants;
Expand All @@ -25,9 +26,7 @@
* A container that encapsulates the result of authenticating with an Identity Provider.
*/
public class IdpResponse implements Parcelable {

private final String mProviderId;
@Nullable
private final String mEmail;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

private final String mToken;
private final String mSecret;
Expand All @@ -37,27 +36,27 @@ public IdpResponse(int errorCode) {
this(null, null, null, null, errorCode);
}

public IdpResponse(String providerId, String email) {
this(providerId, email, null, null);
public IdpResponse(String providerId, @NonNull String email) {
this(providerId, email, null, null, ResultCodes.OK);
}

public IdpResponse(String providerId, @Nullable String email, @Nullable String token) {
this(providerId, email, token, null);
public IdpResponse(String providerId, @NonNull String email, @NonNull String token) {
this(providerId, email, token, null, ResultCodes.OK);
}

public IdpResponse(
String providerId,
@Nullable String email,
@Nullable String token,
@Nullable String secret) {
@NonNull String email,
@NonNull String token,
@NonNull String secret) {
this(providerId, email, token, secret, ResultCodes.OK);
}

public IdpResponse(
private IdpResponse(
String providerId,
@Nullable String email,
@Nullable String token,
@Nullable String secret,
String email,
String token,
String secret,
int errorCode) {
mProviderId = providerId;
mEmail = email;
Expand Down Expand Up @@ -87,11 +86,17 @@ public IdpResponse[] newArray(int size) {
/**
* Get the type of provider. e.g. {@link AuthUI#GOOGLE_PROVIDER}
*/
@Nullable
public String getProviderType() {
return mProviderId;
}

/**
* Get the email used to sign in.
*/
public String getEmail() {
return mEmail;
}

/**
* Get the token received as a result of logging in with the specified IDP
*/
Expand All @@ -108,14 +113,6 @@ public String getIdpSecret() {
return mSecret;
}

/**
* Get the email used to sign in.
*/
@Nullable
public String getEmail() {
return mEmail;
}

/**
* Get the error code for a failed sign in
*/
Expand Down Expand Up @@ -157,6 +154,6 @@ public static Intent getIntent(IdpResponse response) {
}

public static Intent getErrorCodeIntent(int errorCode) {
return new Intent().putExtra(ExtraConstants.EXTRA_IDP_RESPONSE, new IdpResponse(errorCode));
return getIntent(new IdpResponse(errorCode));
}
}
18 changes: 14 additions & 4 deletions auth/src/main/java/com/firebase/ui/auth/provider/TwitterProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,18 @@ public void startLogin(Activity activity) {
}

@Override
public void success(Result<TwitterSession> result) {
mCallbackObject.onSuccess(createIdpResponse(result.data));
public void success(final Result<TwitterSession> sessionResult) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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 😄)

mTwitterAuthClient.requestEmail(sessionResult.data, new Callback<String>() {
@Override
public void success(Result<String> result) {
mCallbackObject.onSuccess(createIdpResponse(sessionResult.data, result.data));
}

@Override
public void failure(TwitterException exception) {
TwitterProvider.this.failure(exception);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
});
}

@Override
Expand All @@ -70,10 +80,10 @@ public void failure(TwitterException exception) {
mCallbackObject.onFailure(new Bundle());
}

private IdpResponse createIdpResponse(TwitterSession twitterSession) {
private IdpResponse createIdpResponse(TwitterSession twitterSession, String email) {
return new IdpResponse(
TwitterAuthProvider.PROVIDER_ID,
null,
email,
twitterSession.getAuthToken().token,
twitterSession.getAuthToken().secret);
}
Expand Down