Skip to content

Completely rewrite CredentialsApiHelper.java and hopefully fix memory leak #566

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

Merged
merged 11 commits into from
Feb 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
76 changes: 65 additions & 11 deletions auth/src/main/java/com/firebase/ui/auth/AuthUI.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,15 @@
import android.support.annotation.Nullable;
import android.support.annotation.StyleRes;
import android.support.annotation.VisibleForTesting;
import android.support.v4.app.FragmentActivity;

import com.facebook.login.LoginManager;
import com.firebase.ui.auth.ui.FlowParameters;
import com.firebase.ui.auth.ui.idp.AuthMethodPickerActivity;
import com.firebase.ui.auth.util.CredentialTaskApi;
import com.firebase.ui.auth.util.CredentialsApiHelper;
import com.firebase.ui.auth.util.GoogleApiClientTaskHelper;
import com.firebase.ui.auth.util.GoogleSignInHelper;
import com.firebase.ui.auth.util.Preconditions;
import com.firebase.ui.auth.util.signincontainer.SmartLockBase;
import com.google.android.gms.auth.api.Auth;
Expand Down Expand Up @@ -313,10 +316,12 @@ public static int getDefaultTheme() {
* Signs the current user out, if one is signed in.
*
* @param activity The activity requesting the user be signed out.
* @return a task which, upon completion, signals that the user has been signed out
* ({@code result.isSuccess()}, or that the sign-out attempt failed unexpectedly
* ({@code !result.isSuccess()}).
* @return a task which, upon completion, signals that the user has been signed out ({@code
* result.isSuccess()}, or that the sign-out attempt failed unexpectedly ({@code
* !result.isSuccess()}).
* @deprecated use {@link #signOut(FragmentActivity)} instead
*/
@Deprecated
public Task<Void> signOut(@NonNull Activity 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.

This makes me so happy: because our new signOut method uses FragmentActivity which extends Activity, people will be automatically upgraded. How's that for customer service! 😄

// Get helper for Google Sign In and Credentials API
GoogleApiClientTaskHelper taskHelper = GoogleApiClientTaskHelper.getInstance(activity);
Expand All @@ -325,7 +330,7 @@ public Task<Void> signOut(@NonNull Activity activity) {
.addApi(Auth.GOOGLE_SIGN_IN_API, GoogleSignInOptions.DEFAULT_SIGN_IN);

// Get Credentials Helper
CredentialsApiHelper credentialsHelper = CredentialsApiHelper.getInstance(taskHelper);
CredentialTaskApi credentialsHelper = CredentialsApiHelper.getInstance(taskHelper);

// Firebase Sign out
mAuth.signOut();
Expand Down Expand Up @@ -359,8 +364,62 @@ public Void then(@NonNull Task<GoogleApiClient> task) throws Exception {
* silently.
*
* @param activity the calling {@link Activity}.
* @deprecated use {@link #delete(FragmentActivity)} instead
*/
@Deprecated
public Task<Void> delete(@NonNull Activity activity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell the difference between delete(FragmentActivity) and delete(Activity) is just that the former uses CredentialApiHelper and the latter uses OldCredentialApiHelper.

Could this all be simplified by having both of the CredentialApiHelper classes implement a common interface? I think it would look like this:

interface CredentialApiHelper {

  Task<Status> delete(Credential credential);

  Task<Status> disableAuthSignIn();

}

And then these two delete() functions would just be a matter of constructing the proper Helper and then calling to a private Task<Void> delete(CredentialApiHelper helper)

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 To address this comment and this one, I didn't add an interface or a good name because my intention for us to throw this away in FBUI v2.0. Do you disagree with that? If so, then we should definitely clean things up and maybe undeprecate thoses classes. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

2.0 could be a while away, let's try and keep things clean in the meantime.

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 SGTM, I'll leave things deprecated so we know to remove that stuff if we go that route, but I keep things clean in the meantime.

// Initialize SmartLock helper
GoogleApiClientTaskHelper gacHelper = GoogleApiClientTaskHelper.getInstance(activity);
gacHelper.getBuilder().addApi(Auth.CREDENTIALS_API);
CredentialTaskApi credentialHelper = CredentialsApiHelper.getInstance(gacHelper);

return getDeleteTask(credentialHelper);
}

/**
* Signs the current user out, if one is signed in.
*
* @param activity the activity requesting the user be signed out
* @return A task which, upon completion, signals that the user has been signed out ({@link
* Task#isSuccessful()}, or that the sign-out attempt failed unexpectedly !{@link
* Task#isSuccessful()}).
*/
public Task<Void> signOut(@NonNull FragmentActivity activity) {
// Get Credentials Helper
GoogleSignInHelper credentialsHelper = GoogleSignInHelper.getInstance(activity);

// Firebase Sign out
mAuth.signOut();

// Disable credentials auto sign-in
Task<Status> disableCredentialsTask = credentialsHelper.disableAutoSignIn();

// Google sign out
Task<Status> signOutTask = credentialsHelper.signOut();

// Facebook sign out
LoginManager.getInstance().logOut();

// Wait for all tasks to complete
return Tasks.whenAll(disableCredentialsTask, signOutTask);
}

/**
* Delete the use from FirebaseAuth and delete any associated credentials from the Credentials
* API. Returns a {@link Task} that succeeds if the Firebase Auth user deletion succeeds and
* fails if the Firebase Auth deletion fails. Credentials deletion failures are handled
* silently.
*
* @param activity the calling {@link Activity}.
*/
public Task<Void> delete(@NonNull FragmentActivity activity) {
// Initialize SmartLock helper
CredentialTaskApi credentialHelper = GoogleSignInHelper.getInstance(activity);

return getDeleteTask(credentialHelper);
}

private Task<Void> getDeleteTask(CredentialTaskApi credentialHelper) {
FirebaseUser firebaseUser = FirebaseAuth.getInstance().getCurrentUser();
if (firebaseUser == null) {
// If the current user is null, return a failed task immediately
Expand All @@ -370,11 +429,6 @@ public Task<Void> delete(@NonNull Activity activity) {
// Delete the Firebase user
Task<Void> deleteUserTask = firebaseUser.delete();

// Initialize SmartLock helper
GoogleApiClientTaskHelper gacHelper = GoogleApiClientTaskHelper.getInstance(activity);
gacHelper.getBuilder().addApi(Auth.CREDENTIALS_API);
CredentialsApiHelper credentialHelper = CredentialsApiHelper.getInstance(gacHelper);

// Get all SmartLock credentials associated with the user
List<Credential> credentials = SmartLockBase.credentialsFromFirebaseUser(firebaseUser);

Expand Down Expand Up @@ -557,8 +611,8 @@ public SignInIntentBuilder setTosUrl(@Nullable String tosUrl) {
* <p>If no providers are explicitly specified by calling this method, then the email
* provider is the default supported provider.
*
* @param idpConfigs a list of {@link IdpConfig}s, where each {@link IdpConfig} contains
* the configuration parameters for the IDP.
* @param idpConfigs a list of {@link IdpConfig}s, where each {@link IdpConfig} contains the
* configuration parameters for the IDP.
* @see IdpConfig
*/
public SignInIntentBuilder setProviders(@NonNull List<IdpConfig> idpConfigs) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import com.firebase.ui.auth.AuthUI.IdpConfig;
import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.R;
import com.firebase.ui.auth.util.GoogleApiConstants;
import com.firebase.ui.auth.util.GoogleApiHelper;
import com.google.android.gms.auth.api.Auth;
import com.google.android.gms.auth.api.signin.GoogleSignInAccount;
import com.google.android.gms.auth.api.signin.GoogleSignInOptions;
Expand Down Expand Up @@ -80,7 +80,7 @@ public GoogleProvider(FragmentActivity activity, IdpConfig idpConfig, @Nullable
}

mGoogleApiClient = new GoogleApiClient.Builder(activity)
.enableAutoManage(activity, GoogleApiConstants.AUTO_MANAGE_ID0, this)
.enableAutoManage(activity, GoogleApiHelper.getSafeAutoManageId(), this)
.addApi(Auth.GOOGLE_SIGN_IN_API, builder.build())
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import com.firebase.ui.auth.ui.TaskFailureLogger;
import com.firebase.ui.auth.ui.User;
import com.firebase.ui.auth.ui.email.fieldvalidators.EmailFieldValidator;
import com.firebase.ui.auth.util.GoogleApiConstants;
import com.firebase.ui.auth.util.GoogleApiHelper;
import com.google.android.gms.auth.api.Auth;
import com.google.android.gms.auth.api.credentials.Credential;
import com.google.android.gms.auth.api.credentials.CredentialPickerConfig;
Expand Down Expand Up @@ -234,7 +234,7 @@ private void showEmailAutoCompleteHint() {
private PendingIntent getEmailHintIntent() {
GoogleApiClient client = new GoogleApiClient.Builder(getContext())
.addApi(Auth.CREDENTIALS_API)
.enableAutoManage(getActivity(), GoogleApiConstants.AUTO_MANAGE_ID3,
.enableAutoManage(getActivity(), GoogleApiHelper.getSafeAutoManageId(),
new GoogleApiClient.OnConnectionFailedListener() {
@Override
public void onConnectionFailed(@NonNull ConnectionResult connectionResult) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.firebase.ui.auth.util;

import com.google.android.gms.auth.api.credentials.Credential;
import com.google.android.gms.common.api.Status;
import com.google.android.gms.tasks.Task;

public interface CredentialTaskApi {
Task<Status> disableAutoSignIn();

Task<Status> delete(Credential credential);
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
* A {@link com.google.android.gms.tasks.Task Task} based wrapper for the Smart Lock for Passwords
* API.
*/
public class CredentialsApiHelper {
@Deprecated
public class CredentialsApiHelper implements CredentialTaskApi {
@NonNull
private final GoogleApiClientTaskHelper mClientHelper;

Expand All @@ -51,6 +52,7 @@ public static CredentialsApiHelper getInstance(GoogleApiClientTaskHelper taskHel
return new CredentialsApiHelper(taskHelper);
}

@Override
public Task<Status> delete(final Credential credential) {
return mClientHelper.getConnectedGoogleApiClient().continueWithTask(
new ExceptionForwardingContinuation<GoogleApiClient, Status>() {
Expand All @@ -64,6 +66,7 @@ protected void process(
});
}

@Override
public Task<Status> disableAutoSignIn() {
return mClientHelper.getConnectedGoogleApiClient().continueWithTask(
new ExceptionForwardingContinuation<GoogleApiClient, Status>() {
Expand All @@ -82,20 +85,20 @@ public void onResult(@NonNull Status status) {
});
}

private abstract static class ExceptionForwardingContinuation<InT, OutT>
implements Continuation<InT, Task<OutT>> {
private abstract static class ExceptionForwardingContinuation<TIn, TOut>
implements Continuation<TIn, Task<TOut>> {

@Override
public final Task<OutT> then(@NonNull Task<InT> task) throws Exception {
TaskCompletionSource<OutT> source = new TaskCompletionSource<>();
public final Task<TOut> then(@NonNull Task<TIn> task) throws Exception {
TaskCompletionSource<TOut> source = new TaskCompletionSource<>();
// calling task.getResult() will implicitly re-throw the exception of the original
// task, which will be returned as the result for the output task. Similarly,
// if process() throws an exception, this will be turned into the task result.
process(task.getResult(), source);
return source.getTask();
}

protected abstract void process(InT in, TaskCompletionSource<OutT> result);
protected abstract void process(TIn in, TaskCompletionSource<TOut> result);
}

private static final class TaskResultCaptor<R extends Result> implements ResultCallback<R> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
* {@link com.google.android.gms.common.api.GoogleApiClient} instance, which manages a single
* instance per activity.
*/
@Deprecated
public class GoogleApiClientTaskHelper {

private static final IdentityHashMap<Activity, GoogleApiClientTaskHelper> INSTANCES
Expand Down

This file was deleted.

90 changes: 90 additions & 0 deletions auth/src/main/java/com/firebase/ui/auth/util/GoogleApiHelper.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package com.firebase.ui.auth.util;

import android.os.Bundle;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.v4.app.FragmentActivity;

import com.google.android.gms.common.ConnectionResult;
import com.google.android.gms.common.api.GoogleApiClient;
import com.google.android.gms.common.api.Result;
import com.google.android.gms.common.api.ResultCallback;
import com.google.android.gms.tasks.OnCompleteListener;
import com.google.android.gms.tasks.OnSuccessListener;
import com.google.android.gms.tasks.Task;
import com.google.android.gms.tasks.TaskCompletionSource;

import java.net.ConnectException;
import java.util.concurrent.atomic.AtomicInteger;

/**
* A {@link Task} based wrapper to get a connect {@link GoogleApiClient}.
*/
public abstract class GoogleApiHelper implements GoogleApiClient.ConnectionCallbacks, GoogleApiClient.OnConnectionFailedListener {
private static final AtomicInteger SAFE_ID = new AtomicInteger(0);

protected GoogleApiClient mClient;
private TaskCompletionSource<Bundle> mGoogleApiConnectionTask = new TaskCompletionSource<>();

protected GoogleApiHelper(FragmentActivity activity, GoogleApiClient.Builder builder) {
builder.enableAutoManage(activity, getSafeAutoManageId(), this);
builder.addConnectionCallbacks(this);
mClient = builder.build();
}

public static int getSafeAutoManageId() {
return SAFE_ID.getAndIncrement();
}

public Task<Bundle> getConnectedApiTask() {
return mGoogleApiConnectionTask.getTask();
}

@Override
public void onConnected(@Nullable Bundle bundle) {
mGoogleApiConnectionTask.setResult(bundle);
}

@Override
public void onConnectionSuspended(int i) {
// Just wait
}

@Override
public void onConnectionFailed(@NonNull ConnectionResult result) {
mGoogleApiConnectionTask.setException(new ConnectException(result.toString()));
}

protected static final class TaskResultCaptor<R extends Result> implements ResultCallback<R> {
private TaskCompletionSource<R> mSource;

public TaskResultCaptor(TaskCompletionSource<R> source) {
mSource = source;
}

@Override
public void onResult(@NonNull R result) {
mSource.setResult(result);
}
}

protected static class ExceptionForwarder<TResult> implements OnCompleteListener<TResult> {
private TaskCompletionSource mSource;
private OnSuccessListener<TResult> mListener;

public ExceptionForwarder(TaskCompletionSource source,
OnSuccessListener<TResult> listener) {
mSource = source;
mListener = listener;
}

@Override
public void onComplete(@NonNull Task<TResult> task) {
if (task.isSuccessful()) {
mListener.onSuccess(task.getResult());
} else {
mSource.setException(task.getException());
}
}
}
}
Loading