Rename OIDC delete operation and refactor integration tests. (#411)

I've renamed the delete operation since we need separate methods for deleting OIDC and SAML provider configs.

I've also created a ProviderConfigTestUtils class to house shared logic between TenantAwareFirebaseAuthIT and FirebaseAuthIT, and I've added a TemporaryProviderConfig class to make it easier for provider configs to be cleaned up automatically. I've structured this class in such a way that we can easily tack on logic to cleanup SAML provider configs as well.

I've also decided to remove testGetUserWithMultipleTenantIds. I initially wrote this test to understand how tenant-aware auths and tenant-agnostic auths interacted with one another. In its existing state, it appears to be leaking created users, and in order to resolve this with TemporaryUser, we would need to declare multiple TemporaryUser objects as rules. I think it's better to avoid this complexity and remove this test. After all, this integration test is mainly making assertions about the server's behavior and not our behavior.

Author: micahstairs

src/main/java/com/google/firebase/auth/AbstractFirebaseAuth.java

@@ -970,7 +970,7 @@ public ApiFuture<OidcProviderConfig> createOidcProviderConfigAsync(
   private CallableOperation<OidcProviderConfig, FirebaseAuthException>
       createOidcProviderConfigOp(final OidcProviderConfig.CreateRequest request) {
     checkNotDestroyed();
-    checkNotNull(request, "create request must not be null");
+    checkNotNull(request, "Create request must not be null.");
     final FirebaseUserManager userManager = getUserManager();
     return new CallableOperation<OidcProviderConfig, FirebaseAuthException>() {
       @Override
@@ -1010,7 +1010,7 @@ public ApiFuture<OidcProviderConfig> updateOidcProviderConfigAsync(
   private CallableOperation<OidcProviderConfig, FirebaseAuthException> updateOidcProviderConfigOp(
       final OidcProviderConfig.UpdateRequest request) {
     checkNotDestroyed();
-    checkNotNull(request, "update request must not be null");
+    checkNotNull(request, "Update request must not be null.");
     final FirebaseUserManager userManager = getUserManager();
     return new CallableOperation<OidcProviderConfig, FirebaseAuthException>() {
       @Override
@@ -1051,7 +1051,7 @@ public ApiFuture<OidcProviderConfig> getOidcProviderConfigAsync(@NonNull String
   private CallableOperation<OidcProviderConfig, FirebaseAuthException>
       getOidcProviderConfigOp(final String providerId) {
     checkNotDestroyed();
-    checkArgument(!Strings.isNullOrEmpty(providerId), "provider ID must not be null or empty");
+    checkArgument(!Strings.isNullOrEmpty(providerId), "Provider ID must not be null or empty.");
     final FirebaseUserManager userManager = getUserManager();
     return new CallableOperation<OidcProviderConfig, FirebaseAuthException>() {
       @Override
@@ -1149,38 +1149,38 @@ protected ListProviderConfigsPage<OidcProviderConfig> execute()
   }
 
   /**
-   * Deletes the provider config identified by the specified provider ID.
+   * Deletes the OIDC Auth provider config identified by the specified provider ID.
    *
    * @param providerId A provider ID string.
    * @throws IllegalArgumentException If the provider ID string is null or empty.
    * @throws FirebaseAuthException If an error occurs while deleting the provider config.
    */
-  public void deleteProviderConfig(@NonNull String providerId) throws FirebaseAuthException {
-    deleteProviderConfigOp(providerId).call();
+  public void deleteOidcProviderConfig(@NonNull String providerId) throws FirebaseAuthException {
+    deleteOidcProviderConfigOp(providerId).call();
   }
 
   /**
-   * Similar to {@link #deleteProviderConfig} but performs the operation asynchronously.
+   * Similar to {@link #deleteOidcProviderConfig} but performs the operation asynchronously.
    *
    * @param providerId A provider ID string.
    * @return An {@code ApiFuture} which will complete successfully when the specified provider
    *     config has been deleted. If an error occurs while deleting the provider config, the future
    *     throws a {@link FirebaseAuthException}.
    * @throws IllegalArgumentException If the provider ID string is null or empty.
    */
-  public ApiFuture<Void> deleteProviderConfigAsync(String providerId) {
-    return deleteProviderConfigOp(providerId).callAsync(firebaseApp);
+  public ApiFuture<Void> deleteOidcProviderConfigAsync(String providerId) {
+    return deleteOidcProviderConfigOp(providerId).callAsync(firebaseApp);
   }
 
-  private CallableOperation<Void, FirebaseAuthException> deleteProviderConfigOp(
+  private CallableOperation<Void, FirebaseAuthException> deleteOidcProviderConfigOp(
       final String providerId) {
     checkNotDestroyed();
-    checkArgument(!Strings.isNullOrEmpty(providerId), "provider ID must not be null or empty");
+    checkArgument(!Strings.isNullOrEmpty(providerId), "Provider ID must not be null or empty.");
     final FirebaseUserManager userManager = getUserManager();
     return new CallableOperation<Void, FirebaseAuthException>() {
       @Override
       protected Void execute() throws FirebaseAuthException {
-        userManager.deleteProviderConfig(providerId);
+        userManager.deleteOidcProviderConfig(providerId);
         return null;
       }
     };

src/main/java/com/google/firebase/auth/FirebaseUserManager.java

@@ -319,7 +319,7 @@ OidcProviderConfig createOidcProviderConfig(
       OidcProviderConfig.CreateRequest request) throws FirebaseAuthException {
     GenericUrl url = new GenericUrl(idpConfigMgtBaseUrl + "/oauthIdpConfigs");
     String providerId = request.getProviderId();
-    checkArgument(!Strings.isNullOrEmpty(providerId), "provider ID must not be null or empty");
+    checkArgument(!Strings.isNullOrEmpty(providerId), "Provider ID must not be null or empty.");
     url.set("oauthIdpConfigId", providerId);
     return sendRequest("POST", url, request.getProperties(), OidcProviderConfig.class);
   }
@@ -328,7 +328,7 @@ OidcProviderConfig updateOidcProviderConfig(OidcProviderConfig.UpdateRequest req
       throws FirebaseAuthException {
     Map<String, Object> properties = request.getProperties();
     checkArgument(!properties.isEmpty(),
-        "provider config update must have at least one property set");
+        "Provider config update must have at least one property set.");
     GenericUrl url =
         new GenericUrl(idpConfigMgtBaseUrl + getOidcUrlSuffix(request.getProviderId()));
     url.put("updateMask", generateMask(properties));
@@ -346,7 +346,7 @@ ListOidcProviderConfigsResponse listOidcProviderConfigs(int maxResults, String p
         ImmutableMap.<String, Object>builder().put("pageSize", maxResults);
     if (pageToken != null) {
       checkArgument(!pageToken.equals(
-          ListTenantsPage.END_OF_LIST), "invalid end of list page token");
+          ListTenantsPage.END_OF_LIST), "Invalid end of list page token.");
       builder.put("nextPageToken", pageToken);
     }
 
@@ -360,7 +360,7 @@ ListOidcProviderConfigsResponse listOidcProviderConfigs(int maxResults, String p
     return response;
   }
 
-  void deleteProviderConfig(String providerId) throws FirebaseAuthException {
+  void deleteOidcProviderConfig(String providerId) throws FirebaseAuthException {
     GenericUrl url = new GenericUrl(idpConfigMgtBaseUrl + getOidcUrlSuffix(providerId));
     sendRequest("DELETE", url, null, GenericJson.class);
   }
@@ -374,12 +374,12 @@ private static String generateMask(Map<String, Object> properties) {
   }
 
   private static String getTenantUrlSuffix(String tenantId) {
-    checkArgument(!Strings.isNullOrEmpty(tenantId), "tenant ID must not be null or empty");
+    checkArgument(!Strings.isNullOrEmpty(tenantId), "Tenant ID must not be null or empty.");
     return "/tenants/" + tenantId;
   }
 
   private static String getOidcUrlSuffix(String providerId) {
-    checkArgument(!Strings.isNullOrEmpty(providerId), "provider ID must not be null or empty");
+    checkArgument(!Strings.isNullOrEmpty(providerId), "Provider ID must not be null or empty.");
     return "/oauthIdpConfigs/" + providerId;
   }
 

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

@@ -46,6 +46,7 @@
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.FirebaseOptions;
 import com.google.firebase.ImplFirebaseTrampolines;
+import com.google.firebase.auth.ProviderConfigTestUtils.TemporaryProviderConfig;
 import com.google.firebase.auth.hash.Scrypt;
 import com.google.firebase.internal.Nullable;
 import com.google.firebase.testing.IntegrationTestUtils;
@@ -84,8 +85,9 @@ public class FirebaseAuthIT {
   private static final FirebaseAuth auth = FirebaseAuth.getInstance(
       IntegrationTestUtils.ensureDefaultApp());
 
-  @Rule
-  public final TemporaryUser temporaryUser = new TemporaryUser();
+  @Rule public final TemporaryUser temporaryUser = new TemporaryUser();
+  @Rule public final TemporaryProviderConfig temporaryProviderConfig =
+      new TemporaryProviderConfig(auth);
 
   @Test
   public void testGetNonExistingUser() throws Exception {
@@ -563,124 +565,113 @@ public void testGenerateSignInWithEmailLink() throws Exception {
   public void testOidcProviderConfigLifecycle() throws Exception {
     // Create config provider
     String providerId = "oidc.provider-id";
-    OidcProviderConfig.CreateRequest createRequest =
+    OidcProviderConfig config = temporaryProviderConfig.createOidcProviderConfig(
         new OidcProviderConfig.CreateRequest()
             .setProviderId(providerId)
             .setDisplayName("DisplayName")
             .setEnabled(true)
             .setClientId("ClientId")
-            .setIssuer("https://oidc.com/issuer");
-    OidcProviderConfig config = auth.createOidcProviderConfigAsync(createRequest).get();
+            .setIssuer("https://oidc.com/issuer"));
     assertEquals(providerId, config.getProviderId());
     assertEquals("DisplayName", config.getDisplayName());
     assertTrue(config.isEnabled());
     assertEquals("ClientId", config.getClientId());
     assertEquals("https://oidc.com/issuer", config.getIssuer());
 
-    try {
-      // Get config provider
-      config = auth.getOidcProviderConfigAsync(providerId).get();
-      assertEquals(providerId, config.getProviderId());
-      assertEquals("DisplayName", config.getDisplayName());
-      assertTrue(config.isEnabled());
-      assertEquals("ClientId", config.getClientId());
-      assertEquals("https://oidc.com/issuer", config.getIssuer());
+    // Get config provider
+    config = auth.getOidcProviderConfigAsync(providerId).get();
+    assertEquals(providerId, config.getProviderId());
+    assertEquals("DisplayName", config.getDisplayName());
+    assertTrue(config.isEnabled());
+    assertEquals("ClientId", config.getClientId());
+    assertEquals("https://oidc.com/issuer", config.getIssuer());
 
-      // Update config provider
-      OidcProviderConfig.UpdateRequest updateRequest =
-          new OidcProviderConfig.UpdateRequest(providerId)
-              .setDisplayName("NewDisplayName")
-              .setEnabled(false)
-              .setClientId("NewClientId")
-              .setIssuer("https://oidc.com/new-issuer");
-      config = auth.updateOidcProviderConfigAsync(updateRequest).get();
-      assertEquals(providerId, config.getProviderId());
-      assertEquals("NewDisplayName", config.getDisplayName());
-      assertFalse(config.isEnabled());
-      assertEquals("NewClientId", config.getClientId());
-      assertEquals("https://oidc.com/new-issuer", config.getIssuer());
-    } finally {
-      // Delete config provider
-      auth.deleteProviderConfigAsync(providerId).get();
-    }
+    // Update config provider
+    OidcProviderConfig.UpdateRequest updateRequest =
+        new OidcProviderConfig.UpdateRequest(providerId)
+            .setDisplayName("NewDisplayName")
+            .setEnabled(false)
+            .setClientId("NewClientId")
+            .setIssuer("https://oidc.com/new-issuer");
+    config = auth.updateOidcProviderConfigAsync(updateRequest).get();
+    assertEquals(providerId, config.getProviderId());
+    assertEquals("NewDisplayName", config.getDisplayName());
+    assertFalse(config.isEnabled());
+    assertEquals("NewClientId", config.getClientId());
+    assertEquals("https://oidc.com/new-issuer", config.getIssuer());
 
-    assertOidcProviderConfigDoesNotExist(auth, providerId);
+    // Delete config provider
+    auth.deleteOidcProviderConfigAsync(providerId).get();
+    ProviderConfigTestUtils.assertOidcProviderConfigDoesNotExist(auth, providerId);
   }
 
   @Test
   public void testListOidcProviderConfigs() throws Exception {
     final List<String> providerIds = new ArrayList<>();
 
-    try {
-      // Create provider configs
-      for (int i = 0; i < 3; i++) {
-        String providerId = "oidc.provider-id" + i;
-        providerIds.add(providerId);
-        OidcProviderConfig.CreateRequest createRequest = new OidcProviderConfig.CreateRequest()
+    // Create provider configs
+    for (int i = 0; i < 3; i++) {
+      String providerId = "oidc.provider-id" + i;
+      providerIds.add(providerId);
+      OidcProviderConfig config = temporaryProviderConfig.createOidcProviderConfig(
+          new OidcProviderConfig.CreateRequest()
             .setProviderId(providerId)
             .setClientId("CLIENT_ID")
-            .setIssuer("https://oidc.com/issuer");
-        auth.createOidcProviderConfig(createRequest);
-      }
-
-      // Test list by batches
-      final AtomicInteger collected = new AtomicInteger(0);
-      ListProviderConfigsPage<OidcProviderConfig> page =
-          auth.listOidcProviderConfigsAsync(null).get();
-      while (page != null) {
-        for (OidcProviderConfig providerConfig : page.getValues()) {
-          if (checkProviderConfig(providerIds, providerConfig)) {
-            collected.incrementAndGet();
-          }
-        }
-        page = page.getNextPage();
-      }
-      assertEquals(providerIds.size(), collected.get());
+            .setIssuer("https://oidc.com/issuer"));
+    }
 
-      // Test iterate all
-      collected.set(0);
-      page = auth.listOidcProviderConfigsAsync(null).get();
-      for (OidcProviderConfig providerConfig : page.iterateAll()) {
+    // Test list by batches
+    final AtomicInteger collected = new AtomicInteger(0);
+    ListProviderConfigsPage<OidcProviderConfig> page =
+        auth.listOidcProviderConfigsAsync(null).get();
+    while (page != null) {
+      for (OidcProviderConfig providerConfig : page.getValues()) {
         if (checkProviderConfig(providerIds, providerConfig)) {
           collected.incrementAndGet();
         }
       }
-      assertEquals(providerIds.size(), collected.get());
-
-      // Test iterate async
-      collected.set(0);
-      final Semaphore semaphore = new Semaphore(0);
-      final AtomicReference<Throwable> error = new AtomicReference<>();
-      ApiFuture<ListProviderConfigsPage<OidcProviderConfig>> pageFuture =
-          auth.listOidcProviderConfigsAsync(null);
-      ApiFutures.addCallback(
-          pageFuture,
-          new ApiFutureCallback<ListProviderConfigsPage<OidcProviderConfig>>() {
-            @Override
-            public void onFailure(Throwable t) {
-              error.set(t);
-              semaphore.release();
-            }
+      page = page.getNextPage();
+    }
+    assertEquals(providerIds.size(), collected.get());
 
-            @Override
-            public void onSuccess(ListProviderConfigsPage<OidcProviderConfig> result) {
-              for (OidcProviderConfig providerConfig : result.iterateAll()) {
-                if (checkProviderConfig(providerIds, providerConfig)) {
-                  collected.incrementAndGet();
-                }
-              }
-              semaphore.release();
-            }
-          }, MoreExecutors.directExecutor());
-      semaphore.acquire();
-      assertEquals(providerIds.size(), collected.get());
-      assertNull(error.get());
-    } finally {
-      // Delete provider configs
-      for (String providerId : providerIds) {
-        auth.deleteProviderConfigAsync(providerId).get();
+    // Test iterate all
+    collected.set(0);
+    page = auth.listOidcProviderConfigsAsync(null).get();
+    for (OidcProviderConfig providerConfig : page.iterateAll()) {
+      if (checkProviderConfig(providerIds, providerConfig)) {
+        collected.incrementAndGet();
       }
     }
+    assertEquals(providerIds.size(), collected.get());
+
+    // Test iterate async
+    collected.set(0);
+    final Semaphore semaphore = new Semaphore(0);
+    final AtomicReference<Throwable> error = new AtomicReference<>();
+    ApiFuture<ListProviderConfigsPage<OidcProviderConfig>> pageFuture =
+        auth.listOidcProviderConfigsAsync(null);
+    ApiFutures.addCallback(
+        pageFuture,
+        new ApiFutureCallback<ListProviderConfigsPage<OidcProviderConfig>>() {
+          @Override
+          public void onFailure(Throwable t) {
+            error.set(t);
+            semaphore.release();
+          }
+
+          @Override
+          public void onSuccess(ListProviderConfigsPage<OidcProviderConfig> result) {
+            for (OidcProviderConfig providerConfig : result.iterateAll()) {
+              if (checkProviderConfig(providerIds, providerConfig)) {
+                collected.incrementAndGet();
+              }
+            }
+            semaphore.release();
+          }
+        }, MoreExecutors.directExecutor());
+    semaphore.acquire();
+    assertEquals(providerIds.size(), collected.get());
+    assertNull(error.get());
   }
 
   private Map<String, String> parseLinkParameters(String link) throws Exception {
@@ -823,23 +814,11 @@ private boolean checkProviderConfig(List<String> providerIds, OidcProviderConfig
   }
 
 
-  private static void assertOidcProviderConfigDoesNotExist(
-      AbstractFirebaseAuth firebaseAuth, String providerId) throws Exception {
-    try {
-      firebaseAuth.getOidcProviderConfigAsync(providerId).get();
-      fail("No error thrown for getting a deleted provider config");
-    } catch (ExecutionException e) {
-      assertTrue(e.getCause() instanceof FirebaseAuthException);
-      assertEquals(FirebaseUserManager.CONFIGURATION_NOT_FOUND_ERROR,
-          ((FirebaseAuthException) e.getCause()).getErrorCode());
-    }
-  }
-
   private static void assertUserDoesNotExist(AbstractFirebaseAuth firebaseAuth, String uid)
       throws Exception {
     try {
       firebaseAuth.getUserAsync(uid).get();
-      fail("No error thrown for getting a user which was expected to be absent");
+      fail("No error thrown for getting a user which was expected to be absent.");
     } catch (ExecutionException e) {
       assertTrue(e.getCause() instanceof FirebaseAuthException);
       assertEquals(FirebaseUserManager.USER_NOT_FOUND_ERROR,