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 6 commits
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
3 changes: 3 additions & 0 deletions auth/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


testCompile 'junit:junit:4.12'
testCompile 'org.mockito:mockito-core:2.2.28'
testCompile 'org.robolectric:robolectric:3.1.4'
Expand Down
47 changes: 22 additions & 25 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,39 +26,37 @@
* 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;
private final int mErrorCode;

public IdpResponse(int errorCode) {
private IdpResponse(int errorCode) {
this(null, null, null, null, errorCode);
}

public IdpResponse(String providerId, String email) {
this(providerId, email, null, null);
public IdpResponse(@NonNull 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(@NonNull 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 providerId,
@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));
}
}
2 changes: 1 addition & 1 deletion auth/src/main/java/com/firebase/ui/auth/provider/FacebookProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public void startLogin(Activity activity) {

@Override
public void setAuthenticationCallback(IdpCallback callback) {
this.mCallbackObject = callback;
mCallbackObject = callback;
}

@Override
Expand Down
78 changes: 69 additions & 9 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 @@ -10,15 +10,29 @@
import com.firebase.ui.auth.R;
import com.google.firebase.auth.AuthCredential;
import com.google.firebase.auth.TwitterAuthProvider;
import com.google.gson.GsonBuilder;
import com.twitter.sdk.android.Twitter;
import com.twitter.sdk.android.core.Callback;
import com.twitter.sdk.android.core.Result;
import com.twitter.sdk.android.core.TwitterAuthConfig;
import com.twitter.sdk.android.core.TwitterCore;
import com.twitter.sdk.android.core.TwitterException;
import com.twitter.sdk.android.core.TwitterSession;
import com.twitter.sdk.android.core.identity.TwitterAuthClient;
import com.twitter.sdk.android.core.internal.TwitterApi;
import com.twitter.sdk.android.core.internal.network.OkHttpClientHelper;
import com.twitter.sdk.android.core.models.BindingValues;
import com.twitter.sdk.android.core.models.BindingValuesAdapter;
import com.twitter.sdk.android.core.models.SafeListAdapter;
import com.twitter.sdk.android.core.models.SafeMapAdapter;
import com.twitter.sdk.android.core.models.User;

import io.fabric.sdk.android.Fabric;
import okhttp3.OkHttpClient;
import retrofit2.Call;
import retrofit2.Retrofit;
import retrofit2.converter.gson.GsonConverterFactory;
import retrofit2.http.GET;

public class TwitterProvider extends Callback<TwitterSession> implements IdpProvider {
private static final String TAG = "TwitterProvider";
Expand Down Expand Up @@ -46,7 +60,7 @@ public String getProviderId() {

@Override
public void setAuthenticationCallback(IdpCallback callback) {
this.mCallbackObject = callback;
mCallbackObject = callback;
}

@Override
Expand All @@ -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) {
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 😄)

new Retrofit.Builder()
.client(getClient(sessionResult))
.baseUrl(new TwitterApi().getBaseHostUrl())
.addConverterFactory(getFactory())
.build()
.create(EmailService.class)
.getEmail()
.enqueue(new Callback<User>() {
@Override
public void success(Result<User> result) {
String email = result.data.email;
if (email == null) {
TwitterProvider.this.failure(
new TwitterException("Your application may not have access to"
+ " email addresses or the user may not have an email address. To request"
+ " access, please visit https://support.twitter.com/forms/platform."));
} else if (email.equals("")) {
TwitterProvider.this.failure(
new TwitterException("This user does not have an email address."));
} else {
mCallbackObject.onSuccess(createIdpResponse(sessionResult.data, email));
}
}

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

@Override
Expand All @@ -70,21 +112,39 @@ 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);
}

private OkHttpClient getClient(Result<TwitterSession> sessionResult) {
return OkHttpClientHelper.getOkHttpClient(
sessionResult.data,
TwitterCore.getInstance().getAuthConfig(),
TwitterCore.getInstance().getSSLSocketFactory());
}

private GsonConverterFactory getFactory() {
return GsonConverterFactory.create(
new GsonBuilder()
.registerTypeAdapterFactory(new SafeListAdapter())
.registerTypeAdapterFactory(new SafeMapAdapter())
.registerTypeAdapter(BindingValues.class, new BindingValuesAdapter())
.create());
}

interface EmailService {
@GET("/1.1/account/verify_credentials.json?include_email=true?include_entities=true?skip_status=true")
Call<User> getEmail();
}

public static AuthCredential createAuthCredential(IdpResponse response) {
if (!response.getProviderType().equalsIgnoreCase(TwitterAuthProvider.PROVIDER_ID)){
if (!response.getProviderType().equalsIgnoreCase(TwitterAuthProvider.PROVIDER_ID)) {
return null;
}
return TwitterAuthProvider.getCredential(
response.getIdpToken(),
response.getIdpSecret());
return TwitterAuthProvider.getCredential(response.getIdpToken(), response.getIdpSecret());
}
}
4 changes: 1 addition & 3 deletions auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,7 @@ protected void onDestroy() {
}
}

public static Intent createIntent(
Context context,
FlowParameters flowParams) {
public static Intent createIntent(Context context, FlowParameters flowParams) {
return BaseHelper.createBaseIntent(context, AuthMethodPickerActivity.class, flowParams);
}
}
2 changes: 1 addition & 1 deletion auth/src/test/java/com/firebase/ui/auth/testhelpers/FakeAuthResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import com.google.firebase.auth.FirebaseUser;

public class FakeAuthResult implements AuthResult {
FirebaseUser mFirebaseUser;
private FirebaseUser mFirebaseUser;

public FakeAuthResult(FirebaseUser firebaseUser) {
mFirebaseUser = firebaseUser;
Expand Down
2 changes: 1 addition & 1 deletion auth/src/test/java/com/firebase/ui/auth/testhelpers/FakeProviderQueryResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import java.util.List;

public class FakeProviderQueryResult implements ProviderQueryResult {
List<String> mProviders;
private List<String> mProviders;

public FakeProviderQueryResult(List<String> providers) {
mProviders = providers;
Expand Down
106 changes: 0 additions & 106 deletions auth/src/test/java/com/firebase/ui/auth/ui/provider/TwitterProviderTest.java
View file
Open in desktop

This file was deleted.