Improve error handling in some more classes (#3278)

* Improve error handling around downloading and installing

* Improve error handling around downloading and installing

* Improve error handling in some more classes

* Address Manny's feedback

Author: lfkellogg

firebase-app-distribution/src/main/java/com/google/firebase/app/distribution/FirebaseAppDistributionNotificationsManager.java

@@ -14,13 +14,13 @@
 
 package com.google.firebase.app.distribution;
 
-import android.app.Notification;
 import android.app.NotificationChannel;
 import android.app.NotificationManager;
 import android.app.PendingIntent;
 import android.content.Context;
 import android.content.Intent;
 import android.os.Build;
+import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 import androidx.core.app.NotificationCompat;
 import com.google.firebase.app.distribution.internal.LogWrapper;
@@ -48,18 +48,20 @@ class FirebaseAppDistributionNotificationsManager {
 
   void updateNotification(long totalBytes, long downloadedBytes, UpdateStatus status) {
     NotificationManager notificationManager = createNotificationManager(context);
-    Notification notification =
+    NotificationCompat.Builder notificationBuilder =
         new NotificationCompat.Builder(context, NOTIFICATION_CHANNEL_ID)
             .setOnlyAlertOnce(true)
             .setSmallIcon(appIconSource.getNonAdaptiveIconOrDefault(context))
-            .setContentIntent(createPendingIntent())
             .setContentTitle(context.getString(getNotificationContentTitleId(status)))
             .setProgress(
                 100,
                 (int) (((float) downloadedBytes / (float) totalBytes) * 100),
-                /*indeterminate = */ false)
-            .build();
-    notificationManager.notify(NOTIFICATION_TAG, /*id =*/ 0, notification);
+                /*indeterminate = */ false);
+    PendingIntent appLaunchIntent = createAppLaunchIntent();
+    if (appLaunchIntent != null) {
+      notificationBuilder.setContentIntent(appLaunchIntent);
+    }
+    notificationManager.notify(NOTIFICATION_TAG, /*id =*/ 0, notificationBuilder.build());
   }
 
   int getNotificationContentTitleId(UpdateStatus status) {
@@ -100,11 +102,13 @@ private NotificationManager createNotificationManager(Context context) {
     }
   }
 
-  private PendingIntent createPendingIntent() {
+  @Nullable
+  private PendingIntent createAppLaunchIntent() {
     // Query the package manager for the best launch intent for the app
     Intent intent = context.getPackageManager().getLaunchIntentForPackage(context.getPackageName());
     if (intent == null) {
       LogWrapper.getInstance().w(TAG + "No activity found to launch app");
+      return null;
     }
     return PendingIntent.getActivity(context, 0, intent, PendingIntent.FLAG_ONE_SHOT);
   }

firebase-app-distribution/src/main/java/com/google/firebase/app/distribution/FirebaseAppDistributionRegistrar.java

@@ -15,6 +15,7 @@
 package com.google.firebase.app.distribution;
 
 import android.app.Application;
+import android.content.Context;
 import androidx.annotation.Keep;
 import androidx.annotation.NonNull;
 import com.google.firebase.FirebaseApp;
@@ -57,14 +58,18 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(ComponentContainer
     FirebaseAppDistributionLifecycleNotifier lifecycleNotifier =
         FirebaseAppDistributionLifecycleNotifier.getInstance();
 
-    if (firebaseApp.getApplicationContext() instanceof Application) {
-      Application firebaseApplication = (Application) firebaseApp.getApplicationContext();
+    Context context = firebaseApp.getApplicationContext();
+    if (context instanceof Application) {
+      Application firebaseApplication = (Application) context;
       firebaseApplication.registerActivityLifecycleCallbacks(lifecycleNotifier);
     } else {
       LogWrapper.getInstance()
           .e(
               TAG
-                  + "Error registering app to ActivityLifecycleCallbacks. SDK might not function correctly.");
+                  + "Context "
+                  + context
+                  + " was not an Application, can't register for lifecycle callbacks. SDK might not"
+                  + " function correctly.");
     }
 
     return appDistribution;

firebase-app-distribution/src/main/java/com/google/firebase/app/distribution/FirebaseAppDistributionTesterApiClient.java

@@ -18,6 +18,7 @@
 import android.content.pm.PackageManager;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
 import com.google.android.gms.common.util.AndroidUtilsLight;
 import com.google.android.gms.common.util.Hex;
 import com.google.firebase.app.distribution.Constants.ErrorMessages;
@@ -27,9 +28,6 @@
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.net.MalformedURLException;
-import java.net.ProtocolException;
-import java.net.URL;
 import javax.net.ssl.HttpsURLConnection;
 import org.json.JSONArray;
 import org.json.JSONException;
@@ -58,31 +56,69 @@ class FirebaseAppDistributionTesterApiClient {
 
   public static final int DEFAULT_BUFFER_SIZE = 8192;
 
+  private final HttpsUrlConnectionFactory httpsUrlConnectionFactory;
+
+  FirebaseAppDistributionTesterApiClient() {
+    this(new HttpsUrlConnectionFactory());
+  }
+
+  @VisibleForTesting
+  FirebaseAppDistributionTesterApiClient(
+      @NonNull HttpsUrlConnectionFactory httpsUrlConnectionFactory) {
+    this.httpsUrlConnectionFactory = httpsUrlConnectionFactory;
+  }
+
   /**
    * Fetches and returns the lastest release for the app that the tester has access to, or null if
    * the tester doesn't have access to any releases.
    */
   @Nullable
-  public AppDistributionReleaseInternal fetchNewRelease(
+  AppDistributionReleaseInternal fetchNewRelease(
       @NonNull String fid,
       @NonNull String appId,
       @NonNull String apiKey,
       @NonNull String authToken,
       @NonNull Context context)
       throws FirebaseAppDistributionException {
-    HttpsURLConnection connection = openHttpsUrlConnection(appId, fid, apiKey, authToken, context);
+    HttpsURLConnection connection = null;
+    int responseCode;
     String responseBody;
-    try (BufferedInputStream inputStream = new BufferedInputStream(connection.getInputStream())) {
-      responseBody = convertInputStreamToString(inputStream);
+    try {
+      connection = openHttpsUrlConnection(appId, fid, apiKey, authToken, context);
+      responseCode = connection.getResponseCode();
+      responseBody = readResponseBody(connection);
     } catch (IOException e) {
-      throw getExceptionForHttpResponse(connection, e);
+      throw new FirebaseAppDistributionException(
+          ErrorMessages.NETWORK_ERROR, Status.NETWORK_FAILURE, e);
     } finally {
-      connection.disconnect();
+      if (connection != null) {
+        connection.disconnect();
+      }
     }
+
+    if (!isResponseSuccess(responseCode)) {
+      throw getExceptionForHttpResponse(responseCode);
+    }
+
     return parseNewRelease(responseBody);
   }
 
-  AppDistributionReleaseInternal parseNewRelease(String responseBody)
+  private String readResponseBody(HttpsURLConnection connection) throws IOException {
+    boolean isSuccess = isResponseSuccess(connection.getResponseCode());
+    InputStream inputStream = isSuccess ? connection.getInputStream() : connection.getErrorStream();
+    if (inputStream == null && !isSuccess) {
+      // If the server returns a response with an error code and no response body, getErrorStream
+      // returns null. We return an empty string to reflect the empty body.
+      return "";
+    }
+    return convertInputStreamToString(new BufferedInputStream(inputStream));
+  }
+
+  private static boolean isResponseSuccess(int responseCode) {
+    return responseCode >= 200 && responseCode < 300;
+  }
+
+  private AppDistributionReleaseInternal parseNewRelease(String responseBody)
       throws FirebaseAppDistributionException {
     try {
       JSONObject responseJson = new JSONObject(responseBody);
@@ -128,40 +164,27 @@ AppDistributionReleaseInternal parseNewRelease(String responseBody)
     }
   }
 
-  private FirebaseAppDistributionException getExceptionForHttpResponse(
-      HttpsURLConnection connection, Exception cause) {
-    // TODO(lkellogg): this try-catch should be unnecessary because it will only throw an
-    // IOException here if we couldn't connect to the server, in which case getInputStream() would
-    // have already failed with the same exception. We also weirdly have to choose one of the two
-    // thrown exceptions to set as the cause. We can avoid this by checking the response code
-    // first, and then catching any unexpected exceptions when reading the input stream, essentially
-    // combining the "default" case below with this try-catch.
-    int responseCode;
-    try {
-      responseCode = connection.getResponseCode();
-    } catch (IOException e) {
-      return new FirebaseAppDistributionException(
-          ErrorMessages.NETWORK_ERROR, Status.NETWORK_FAILURE, e);
-    }
-    LogWrapper.getInstance().e(TAG + "Failed due to " + responseCode);
+  private FirebaseAppDistributionException getExceptionForHttpResponse(int responseCode) {
     switch (responseCode) {
+      case 400:
+        return new FirebaseAppDistributionException(
+            "Bad request when fetching new release", Status.UNKNOWN);
       case 401:
         return new FirebaseAppDistributionException(
-            ErrorMessages.AUTHENTICATION_ERROR, Status.AUTHENTICATION_FAILURE, cause);
+            ErrorMessages.AUTHENTICATION_ERROR, Status.AUTHENTICATION_FAILURE);
       case 403:
-      case 400:
         return new FirebaseAppDistributionException(
-            ErrorMessages.AUTHORIZATION_ERROR, Status.AUTHENTICATION_FAILURE, cause);
+            ErrorMessages.AUTHORIZATION_ERROR, Status.AUTHENTICATION_FAILURE);
       case 404:
         return new FirebaseAppDistributionException(
-            ErrorMessages.NOT_FOUND_ERROR, Status.AUTHENTICATION_FAILURE, cause);
+            "App or tester not found when fetching new release", Status.AUTHENTICATION_FAILURE);
       case 408:
       case 504:
         return new FirebaseAppDistributionException(
-            ErrorMessages.TIMEOUT_ERROR, Status.NETWORK_FAILURE, cause);
+            ErrorMessages.TIMEOUT_ERROR, Status.NETWORK_FAILURE);
       default:
         return new FirebaseAppDistributionException(
-            ErrorMessages.UNKNOWN_ERROR, Status.UNKNOWN, cause);
+            "Received error status when fetching new release: " + responseCode, Status.UNKNOWN);
     }
   }
 
@@ -173,22 +196,13 @@ private String tryGetValue(JSONObject jsonObject, String key) {
     }
   }
 
-  HttpsURLConnection openHttpsUrlConnection(
+  private HttpsURLConnection openHttpsUrlConnection(
       String appId, String fid, String apiKey, String authToken, Context context)
-      throws FirebaseAppDistributionException {
+      throws IOException {
     HttpsURLConnection httpsURLConnection;
-    URL url = getReleasesEndpointUrl(appId, fid);
-    try {
-      httpsURLConnection = (HttpsURLConnection) url.openConnection();
-    } catch (IOException e) {
-      throw new FirebaseAppDistributionException(
-          ErrorMessages.NETWORK_ERROR, Status.NETWORK_FAILURE, e);
-    }
-    try {
-      httpsURLConnection.setRequestMethod(REQUEST_METHOD_GET);
-    } catch (ProtocolException e) {
-      throw new FirebaseAppDistributionException(ErrorMessages.UNKNOWN_ERROR, Status.UNKNOWN, e);
-    }
+    String url = String.format(RELEASE_ENDPOINT_URL_FORMAT, appId, fid);
+    httpsURLConnection = httpsUrlConnectionFactory.openConnection(url);
+    httpsURLConnection.setRequestMethod(REQUEST_METHOD_GET);
     httpsURLConnection.setRequestProperty(API_KEY_HEADER, apiKey);
     httpsURLConnection.setRequestProperty(INSTALLATION_AUTH_HEADER, authToken);
     httpsURLConnection.addRequestProperty(X_ANDROID_PACKAGE_HEADER_KEY, context.getPackageName());
@@ -197,16 +211,6 @@ HttpsURLConnection openHttpsUrlConnection(
     return httpsURLConnection;
   }
 
-  private URL getReleasesEndpointUrl(String appId, String fid)
-      throws FirebaseAppDistributionException {
-    try {
-      return new URL(String.format(RELEASE_ENDPOINT_URL_FORMAT, appId, fid));
-    } catch (MalformedURLException e) {
-      throw new FirebaseAppDistributionException(
-          ErrorMessages.UNKNOWN_ERROR, FirebaseAppDistributionException.Status.UNKNOWN, e);
-    }
-  }
-
   private static String convertInputStreamToString(InputStream is) throws IOException {
     ByteArrayOutputStream result = new ByteArrayOutputStream();
     byte[] buffer = new byte[DEFAULT_BUFFER_SIZE];
@@ -225,12 +229,22 @@ private String getFingerprintHashForPackage(Context context) {
       hash = AndroidUtilsLight.getPackageCertificateHashBytes(context, context.getPackageName());
 
       if (hash == null) {
+        LogWrapper.getInstance()
+            .e(
+                TAG
+                    + "Could not get fingerprint hash for X-Android-Cert header. Package is not signed: "
+                    + context.getPackageName());
         return null;
       } else {
         return Hex.bytesToStringUppercase(hash, /* zeroTerminated= */ false);
       }
     } catch (PackageManager.NameNotFoundException e) {
-      LogWrapper.getInstance().e(TAG + "No such package: " + context.getPackageName(), e);
+      LogWrapper.getInstance()
+          .e(
+              TAG
+                  + "Could not get fingerprint hash for X-Android-Cert header. No such package: "
+                  + context.getPackageName(),
+              e);
       return null;
     }
   }

firebase-app-distribution/src/main/java/com/google/firebase/app/distribution/NewReleaseFetcher.java

@@ -179,6 +179,8 @@ boolean isSameAsInstalledRelease(AppDistributionReleaseInternal newRelease)
       return hasSameHashAsInstalledRelease(newRelease);
     }
 
+    // TODO(lkellogg): getIasArtifactId() will likely never be null since it's set to the empty
+    //  string if not present in the response
     if (newRelease.getIasArtifactId() == null) {
       return false;
     }

firebase-app-distribution/src/test/java/com/google/firebase/app/distribution/AabUpdaterTest.java

@@ -66,7 +66,7 @@ public class AabUpdaterTest {
   static class TestActivity extends Activity {}
 
   @Before
-  public void setup() throws IOException {
+  public void setup() throws IOException, FirebaseAppDistributionException {
     MockitoAnnotations.initMocks(this);
 
     FirebaseApp.clearInstancesForTest();
@@ -113,7 +113,7 @@ public void updateAppTask_isNotRedirectResponse_setsDownloadFailure()
 
   @Test
   public void updateAppTask_missingLocationHeader_setsDownloadFailure()
-      throws IOException, InterruptedException {
+      throws InterruptedException {
     when(mockHttpsUrlConnection.getHeaderField("Location")).thenReturn(null);
 
     UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
@@ -123,8 +123,7 @@ public void updateAppTask_missingLocationHeader_setsDownloadFailure()
   }
 
   @Test
-  public void updateAppTask_emptyLocationHeader_setsDownloadFailure()
-      throws IOException, InterruptedException {
+  public void updateAppTask_emptyLocationHeader_setsDownloadFailure() throws InterruptedException {
     when(mockHttpsUrlConnection.getHeaderField("Location")).thenReturn("");
 
     UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
@@ -154,7 +153,7 @@ public void updateAppTask_whenAabReleaseAvailable_redirectsToPlay() throws Inter
   }
 
   @Test
-  public void updateAppTask_onAppResume_setsUpdateCancelled() throws IOException {
+  public void updateAppTask_onAppResume_setsUpdateCancelled() {
     TestOnCompleteListener<Void> onCompleteListener = new TestOnCompleteListener<>();
     UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
     updateTask.addOnCompleteListener(testExecutor, onCompleteListener);

firebase-app-distribution/src/test/java/com/google/firebase/app/distribution/ApkUpdaterTest.java

@@ -73,10 +73,10 @@ public class ApkUpdaterTest {
   @Mock private ApkInstaller mockApkInstaller;
   @Mock private FirebaseAppDistributionNotificationsManager mockNotificationsManager;
 
-  private Executor testExecutor = Executors.newSingleThreadExecutor();
+  private final Executor testExecutor = Executors.newSingleThreadExecutor();
 
   @Before
-  public void setup() throws IOException {
+  public void setup() throws IOException, FirebaseAppDistributionException {
     MockitoAnnotations.initMocks(this);
 
     FirebaseApp.clearInstancesForTest();