Await foreground activity during sign in

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistributionException.java

@@ -17,6 +17,7 @@
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import com.google.firebase.FirebaseException;
+import com.google.firebase.appdistribution.Constants.ErrorMessages;
 
 /** Possible exceptions thrown in FirebaseAppDistribution */
 public class FirebaseAppDistributionException extends FirebaseException {
@@ -98,4 +99,13 @@ public AppDistributionRelease getRelease() {
   public Status getErrorCode() {
     return status;
   }
+
+  static FirebaseAppDistributionException wrap(Throwable t) {
+    // We never want to wrap a FirebaseAppDistributionException
+    if (t instanceof FirebaseAppDistributionException) {
+      return (FirebaseAppDistributionException) t;
+    }
+    return new FirebaseAppDistributionException(
+        String.format("%s: %s", ErrorMessages.UNKNOWN_ERROR, t.getMessage()), Status.UNKNOWN, t);
+  }
 }

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistributionLifecycleNotifier.java

@@ -83,6 +83,13 @@ interface OnActivityDestroyedListener {
     void onDestroyed(Activity activity);
   }
 
+  /**
+   * Get a {@link Task} that will succeed with a result of the app's foregrounded {@link Activity},
+   * when one is available.
+   *
+   * <p>The returned task will never fail. It will instead remain pending indefinitely until some
+   * activity comes to the foreground.
+   */
   Task<Activity> getForegroundActivity() {
     synchronized (lock) {
       if (currentActivity != null) {

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/TaskUtils.java

@@ -14,21 +14,31 @@
 
 package com.google.firebase.appdistribution;
 
+import com.google.android.gms.tasks.SuccessContinuation;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.android.gms.tasks.Tasks;
-import com.google.firebase.appdistribution.Constants.ErrorMessages;
+import com.google.auto.value.AutoValue;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
 import com.google.firebase.appdistribution.internal.LogWrapper;
 import java.util.concurrent.Executor;
 
 class TaskUtils {
   private static final String TAG = "TaskUtils:";
 
+  /**
+   * A functional interface to wrap a function that returns some result of a possibly long-running
+   * operation, and could potentially throw a {@link FirebaseAppDistributionException}.
+   */
   interface Operation<TResult> {
     TResult run() throws FirebaseAppDistributionException;
   }
 
+  /** A functional interface to wrap a function that produces a {@link Task}. */
+  interface TaskSource<TResult> {
+    Task<TResult> get();
+  }
+
   /**
    * Runs a long running operation inside a {@link Task}, wrapping any errors in {@link
    * FirebaseAppDistributionException}.
@@ -51,10 +61,8 @@ static <TResult> Task<TResult> runAsyncInTask(Executor executor, Operation<TResu
         () -> {
           try {
             taskCompletionSource.setResult(operation.run());
-          } catch (FirebaseAppDistributionException e) {
-            taskCompletionSource.setException(e);
           } catch (Throwable t) {
-            taskCompletionSource.setException(wrapException(t));
+            taskCompletionSource.setException(FirebaseAppDistributionException.wrap(t));
           }
         });
     return taskCompletionSource.getTask();
@@ -79,14 +87,61 @@ static <TResult> Task<TResult> handleTaskFailure(Task<TResult> task) {
       LogWrapper.getInstance().e(TAG + "Task failed to complete due to " + e.getMessage(), e);
       return e instanceof FirebaseAppDistributionException
           ? task
-          : Tasks.forException(wrapException(e));
+          : Tasks.forException(FirebaseAppDistributionException.wrap(e));
     }
     return task;
   }
 
-  private static FirebaseAppDistributionException wrapException(Throwable t) {
-    return new FirebaseAppDistributionException(
-        String.format("%s: %s", ErrorMessages.UNKNOWN_ERROR, t.getMessage()), Status.UNKNOWN, t);
+  /**
+   * An @{link AutoValue} class to hold the result of two Tasks, combined using {@link
+   * #combineWithResultOf}.
+   *
+   * @param <T1> The result type of the first task
+   * @param <T2> The result type of the second task
+   */
+  @AutoValue
+  abstract static class CombinedTaskResults<T1, T2> {
+    abstract T1 first();
+
+    abstract T2 second();
+
+    static <T1, T2> CombinedTaskResults<T1, T2> create(T1 first, T2 second) {
+      return new AutoValue_TaskUtils_CombinedTaskResults(first, second);
+    }
+  }
+
+  /**
+   * Returns a {@link SuccessContinuation} to be chained off of a {@link Task}, that will run
+   * another task in sequence and combine both results together.
+   *
+   * <p>This is useful when you want to run two tasks and use the results of each, but those tasks
+   * need to be run sequentially. If they can be run in parallel, use {@link Tasks#whenAll} or one
+   * of its variations.
+   *
+   * <p>Usage:
+   *
+   * <pre>{@code
+   * runFirstAsyncTask()
+   *   .onSuccessTask(combineWithResultOf(() -> startSecondAsyncTask())
+   *   .addOnSuccessListener(
+   *       results ->
+   *           doSomethingWithBothResults(results.result1(), results.result2()));
+   * }</pre>
+   *
+   * @param secondTaskSource A {@link TaskSource} providing the next task to run
+   * @param <T1> The result type of the first task
+   * @param <T2> The result type of the second task
+   * @return A {@link SuccessContinuation} that will return a new task with result type {@link
+   *     CombinedTaskResults}, combining the results of both tasks
+   */
+  static <T1, T2> SuccessContinuation<T1, CombinedTaskResults<T1, T2>> combineWithResultOf(
+      TaskSource<T2> secondTaskSource) {
+    return firstResult ->
+        secondTaskSource
+            .get()
+            .onSuccessTask(
+                secondResult ->
+                    Tasks.forResult(CombinedTaskResults.create(firstResult, secondResult)));
   }
 
   static void safeSetTaskException(TaskCompletionSource taskCompletionSource, Exception e) {

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/TesterSignInManager.java

@@ -15,7 +15,7 @@
 package com.google.firebase.appdistribution;
 
 import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_CANCELED;
-import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_FAILURE;
+import static com.google.firebase.appdistribution.TaskUtils.combineWithResultOf;
 import static com.google.firebase.appdistribution.TaskUtils.safeSetTaskException;
 import static com.google.firebase.appdistribution.TaskUtils.safeSetTaskResult;
 
@@ -28,12 +28,13 @@
 import androidx.annotation.NonNull;
 import androidx.annotation.VisibleForTesting;
 import androidx.browser.customtabs.CustomTabsIntent;
-import com.google.android.gms.tasks.OnSuccessListener;
+import com.google.android.gms.tasks.OnFailureListener;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.android.gms.tasks.Tasks;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.appdistribution.Constants.ErrorMessages;
+import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
 import com.google.firebase.appdistribution.internal.InstallActivity;
 import com.google.firebase.appdistribution.internal.LogWrapper;
 import com.google.firebase.appdistribution.internal.SignInResultActivity;
@@ -55,6 +56,9 @@ class TesterSignInManager {
 
   private final Object signInTaskLock = new Object();
 
+  @GuardedBy("signInTaskLock")
+  private boolean hasBeenSentToBrowserForCurrentTask = false;
+
   @GuardedBy("signInTaskLock")
   private TaskCompletionSource<Void> signInTaskCompletionSource = null;
 
@@ -87,8 +91,7 @@ class TesterSignInManager {
   @VisibleForTesting
   void onActivityCreated(Activity activity) {
     // We call finish() in the onCreate method of the SignInResultActivity, so we must set the
-    // result
-    // of the signIn Task in the onActivityCreated callback
+    // result of the signIn Task in the onActivityCreated callback
     if (activity instanceof SignInResultActivity) {
       LogWrapper.getInstance().v("Sign in completed");
       this.setSuccessfulSignInResult();
@@ -104,50 +107,74 @@ void onActivityStarted(Activity activity) {
       return;
     } else {
       // Throw error if app reentered during sign in
-      if (this.isCurrentlySigningIn()) {
-        LogWrapper.getInstance().e("App Resumed without sign in flow completing.");
-        this.setCanceledAuthenticationError();
+      synchronized (signInTaskLock) {
+        if (awaitingResultFromBrowser()) {
+          LogWrapper.getInstance().e("App Resumed without sign in flow completing.");
+          setSignInTaskCompletionError(
+              new FirebaseAppDistributionException(
+                  Constants.ErrorMessages.AUTHENTICATION_CANCELED, AUTHENTICATION_CANCELED));
+        }
       }
     }
   }
 
   @NonNull
   public Task<Void> signInTester() {
-
     if (signInStorage.getSignInStatus()) {
       LogWrapper.getInstance().v(TAG + "Tester is already signed in.");
       return Tasks.forResult(null);
     }
 
     synchronized (signInTaskLock) {
-      if (this.isCurrentlySigningIn()) {
+      if (signInTaskCompletionSource != null
+          && !signInTaskCompletionSource.getTask().isComplete()) {
         LogWrapper.getInstance()
             .v(TAG + "Detected In-Progress sign in task. Returning the same task.");
         return signInTaskCompletionSource.getTask();
       }
 
       signInTaskCompletionSource = new TaskCompletionSource<>();
+      hasBeenSentToBrowserForCurrentTask = false;
 
       firebaseInstallationsApiProvider
           .get()
           .getId()
-          .addOnSuccessListener(getFidGenerationOnSuccessListener())
           .addOnFailureListener(
-              e -> {
-                LogWrapper.getInstance().e(TAG + "Fid retrieval failed.", e);
-                setSignInTaskCompletionError(
-                    new FirebaseAppDistributionException(
-                        ErrorMessages.AUTHENTICATION_ERROR, AUTHENTICATION_FAILURE, e));
-              });
+              handleTaskFailure(ErrorMessages.AUTHENTICATION_ERROR, Status.AUTHENTICATION_FAILURE))
+          .onSuccessTask(combineWithResultOf(() -> lifecycleNotifier.getForegroundActivity()))
+          .addOnSuccessListener(
+              fidAndActivity -> {
+                // Launch the intent outside of the synchronized block because we don't need to wait
+                // for the lock, and we don't want to risk the activity leaving the foreground in
+                // the meantime.
+                openSignInFlowInBrowser(fidAndActivity.first(), fidAndActivity.second());
+                // This synchronized block is required by the @GuardedBy annotation, but is not
+                // practically required in this case because the only reads of this variable are on
+                // the main thread, which this callback is also running on.
+                synchronized (signInTaskLock) {
+                  hasBeenSentToBrowserForCurrentTask = true;
+                }
+              })
+          // No failures expected here, since getForegroundActivity() will wait indefinitely for a
+          // foreground activity, but catch any unexpected failures to be safe.
+          .addOnFailureListener(handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, Status.UNKNOWN));
 
       return signInTaskCompletionSource.getTask();
     }
   }
 
-  private boolean isCurrentlySigningIn() {
+  private OnFailureListener handleTaskFailure(String message, Status status) {
+    return e -> {
+      LogWrapper.getInstance().e(TAG + message, e);
+      setSignInTaskCompletionError(new FirebaseAppDistributionException(message, status, e));
+    };
+  }
+
+  private boolean awaitingResultFromBrowser() {
     synchronized (signInTaskLock) {
       return signInTaskCompletionSource != null
-          && !signInTaskCompletionSource.getTask().isComplete();
+          && !signInTaskCompletionSource.getTask().isComplete()
+          && hasBeenSentToBrowserForCurrentTask;
     }
   }
 
@@ -157,33 +184,12 @@ private void setSignInTaskCompletionError(FirebaseAppDistributionException e) {
     }
   }
 
-  private void setCanceledAuthenticationError() {
-    setSignInTaskCompletionError(
-        new FirebaseAppDistributionException(
-            Constants.ErrorMessages.AUTHENTICATION_CANCELED, AUTHENTICATION_CANCELED));
-  }
-
   private void setSuccessfulSignInResult() {
     synchronized (signInTaskLock) {
       safeSetTaskResult(signInTaskCompletionSource, null);
     }
   }
 
-  private OnSuccessListener<String> getFidGenerationOnSuccessListener() {
-    return fid -> {
-      Context context = firebaseApp.getApplicationContext();
-      Uri uri =
-          Uri.parse(
-              String.format(
-                  SIGNIN_REDIRECT_URL,
-                  firebaseApp.getOptions().getApplicationId(),
-                  fid,
-                  getApplicationName(context),
-                  context.getPackageName()));
-      openSignInFlowInBrowser(context, uri);
-    };
-  }
-
   private static String getApplicationName(Context context) {
     try {
       return context.getApplicationInfo().loadLabel(context.getPackageManager()).toString();
@@ -193,22 +199,30 @@ private static String getApplicationName(Context context) {
     }
   }
 
-  private void openSignInFlowInBrowser(Context applicationContext, Uri uri) {
+  private void openSignInFlowInBrowser(String fid, Activity activity) {
+    Context context = firebaseApp.getApplicationContext();
+    Uri uri =
+        Uri.parse(
+            String.format(
+                SIGNIN_REDIRECT_URL,
+                firebaseApp.getOptions().getApplicationId(),
+                fid,
+                getApplicationName(context),
+                context.getPackageName()));
     LogWrapper.getInstance().v(TAG + "Opening sign in flow in browser at " + uri);
-    if (supportsCustomTabs(applicationContext)) {
+    if (supportsCustomTabs(context)) {
       // If we can launch a chrome view, try that.
       CustomTabsIntent customTabsIntent = new CustomTabsIntent.Builder().build();
       Intent intent = customTabsIntent.intent;
       intent.addFlags(Intent.FLAG_ACTIVITY_NO_HISTORY);
       intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
-      customTabsIntent.launchUrl(applicationContext, uri);
-
+      customTabsIntent.launchUrl(activity, uri);
     } else {
       // If we can't launch a chrome view try to launch anything that can handle a URL.
       Intent browserIntent = new Intent(Intent.ACTION_VIEW, uri);
       browserIntent.addFlags(Intent.FLAG_ACTIVITY_NO_HISTORY);
       browserIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
-      applicationContext.startActivity(browserIntent);
+      activity.startActivity(browserIntent);
     }
   }
 

firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/TestUtils.java

@@ -31,8 +31,8 @@ static void assertTaskFailure(Task task, Status status, String messageSubstring)
   }
 
   static void assertTaskFailure(
-      UpdateTask updateTask, Status status, String messageSubstring, Throwable cause) {
-    assertTaskFailure(updateTask, status, messageSubstring);
-    assertThat(updateTask.getException()).hasCauseThat().isEqualTo(cause);
+      Task task, Status status, String messageSubstring, Throwable cause) {
+    assertTaskFailure(task, status, messageSubstring);
+    assertThat(task.getException()).hasCauseThat().isEqualTo(cause);
   }
 }