Make Javadoc comments consistent and validate provider ID in CreateRequest. (#418)

This makes the formatting of the Javadoc comments more consistent, and it's now more clear when/why IllegalArgumentExceptions get thrown. This also adds validation to the provider ID in the CreateRequest to ensure that it has the correct prefix.'

Improvements were also made to the tests. The JSON string is now more readable, the parameters are now in the correct order for assertEquals calls, and the IllegalArgumentException tests have been properly fleshed out.

Author: micahstairs

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

@@ -48,7 +48,7 @@ public String getIssuer() {
    * Returns a new {@link UpdateRequest}, which can be used to update the attributes of this
    * provider config.
    *
-   * @return a non-null {@link UpdateRequest} instance.
+   * @return A non-null {@link UpdateRequest} instance.
    */
   public UpdateRequest updateRequest() {
     return new UpdateRequest(getProviderId());
@@ -74,7 +74,8 @@ public CreateRequest() { }
     /**
      * Sets the client ID for the new provider.
      *
-     * @param clientId a non-null, non-empty client ID string.
+     * @param clientId A non-null, non-empty client ID string.
+     * @throws IllegalArgumentException If the client ID is null or empty.
      */
     public CreateRequest setClientId(String clientId) {
       checkArgument(!Strings.isNullOrEmpty(clientId), "Client ID must not be null or empty.");
@@ -85,7 +86,9 @@ public CreateRequest setClientId(String clientId) {
     /**
      * Sets the issuer for the new provider.
      *
-     * @param issuer a non-null, non-empty issuer URL string.
+     * @param issuer A non-null, non-empty issuer URL string.
+     * @throws IllegalArgumentException If the issuer URL is null or empty, or if the format is
+     *     invalid.
      */
     public CreateRequest setIssuer(String issuer) {
       checkArgument(!Strings.isNullOrEmpty(issuer), "Issuer must not be null or empty.");
@@ -97,6 +100,10 @@ public CreateRequest setIssuer(String issuer) {
     CreateRequest getThis() {
       return this;
     }
+
+    void assertValidProviderIdFormat(String providerId) {
+      checkArgument(providerId.startsWith("oidc."), "Invalid OIDC provider ID: " + providerId);
+    }
   }
 
   /**
@@ -116,9 +123,9 @@ public static final class UpdateRequest extends AbstractUpdateRequest<UpdateRequ
      * {@link AbstractFirebaseAuth#updateOidcProviderConfig(CreateRequest)} to update the provider
      * information persistently.
      *
-     * @param tenantId a non-null, non-empty provider ID string.
-     * @throws IllegalArgumentException If the provider ID is null or empty, or if it's an invalid
-     *     format
+     * @param tenantId A non-null, non-empty provider ID string.
+     * @throws IllegalArgumentException If the provider ID is null or empty, or is not prefixed with
+     *     "oidc.".
      */
     public UpdateRequest(String providerId) {
       super(providerId);
@@ -128,7 +135,8 @@ public UpdateRequest(String providerId) {
     /**
      * Sets the client ID for the exsting provider.
      *
-     * @param clientId a non-null, non-empty client ID string.
+     * @param clientId A non-null, non-empty client ID string.
+     * @throws IllegalArgumentException If the client ID is null or empty.
      */
     public UpdateRequest setClientId(String clientId) {
       checkArgument(!Strings.isNullOrEmpty(clientId), "Client ID must not be null or empty.");
@@ -139,7 +147,9 @@ public UpdateRequest setClientId(String clientId) {
     /**
      * Sets the issuer for the existing provider.
      *
-     * @param issuer a non-null, non-empty issuer URL string.
+     * @param issuer A non-null, non-empty issuer URL string.
+     * @throws IllegalArgumentException If the issuer URL is null or empty, or if the format is
+     *     invalid.
      */
     public UpdateRequest setIssuer(String issuer) {
       checkArgument(!Strings.isNullOrEmpty(issuer), "Issuer must not be null or empty.");

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

@@ -74,11 +74,14 @@ public abstract static class AbstractCreateRequest<T extends AbstractCreateReque
     /**
      * Sets the ID for the new provider.
      *
-     * @param providerId a non-null, non-empty provider ID string.
+     * @param providerId A non-null, non-empty provider ID string.
+     * @throws IllegalArgumentException If the provider ID is null or empty, or if the format is
+     *     invalid.
      */
     public T setProviderId(String providerId) {
       checkArgument(
           !Strings.isNullOrEmpty(providerId), "Provider ID name must not be null or empty.");
+      assertValidProviderIdFormat(providerId);
       this.providerId = providerId;
       return getThis();
     }
@@ -90,7 +93,8 @@ String getProviderId() {
     /**
      * Sets the display name for the new provider.
      *
-     * @param displayName a non-null, non-empty display name string.
+     * @param displayName A non-null, non-empty display name string.
+     * @throws IllegalArgumentException If the display name is null or empty.
      */
     public T setDisplayName(String displayName) {
       checkArgument(!Strings.isNullOrEmpty(displayName), "Display name must not be null or empty.");
@@ -101,7 +105,7 @@ public T setDisplayName(String displayName) {
     /**
      * Sets whether to allow the user to sign in with the provider.
      *
-     * @param enabled a boolean indicating whether the user can sign in with the provider
+     * @param enabled A boolean indicating whether the user can sign in with the provider.
      */
     public T setEnabled(boolean enabled) {
       properties.put("enabled", enabled);
@@ -113,6 +117,8 @@ Map<String, Object> getProperties() {
     }
 
     abstract T getThis();
+
+    abstract void assertValidProviderIdFormat(String providerId);
   }
 
   /**
@@ -135,7 +141,8 @@ String getProviderId() {
     /**
      * Sets the display name for the existing provider.
      *
-     * @param displayName a non-null, non-empty display name string.
+     * @param displayName A non-null, non-empty display name string.
+     * @throws IllegalArgumentException If the display name is null or empty.
      */
     public T setDisplayName(String displayName) {
       checkArgument(!Strings.isNullOrEmpty(displayName), "Display name must not be null or empty.");
@@ -146,7 +153,7 @@ public T setDisplayName(String displayName) {
     /**
      * Sets whether to allow the user to sign in with the provider.
      *
-     * @param enabled a boolean indicating whether the user can sign in with the provider
+     * @param enabled A boolean indicating whether the user can sign in with the provider.
      */
     public T setEnabled(boolean enabled) {
       properties.put("enabled", enabled);

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

@@ -32,23 +32,23 @@ public class OidcProviderConfigTest {
   private static final JsonFactory jsonFactory = Utils.getDefaultJsonFactory();
 
   private static final String OIDC_JSON_STRING =
-      "{"
-        + "\"name\":\"projects/projectId/oauthIdpConfigs/oidc.provider-id\","
-        + "\"displayName\":\"DISPLAY_NAME\","
-        + "\"enabled\":true,"
-        + "\"clientId\":\"CLIENT_ID\","
-        + "\"issuer\":\"https://oidc.com/issuer\""
-        + "}";
+      ("{"
+        + "  'name':        'projects/projectId/oauthIdpConfigs/oidc.provider-id',"
+        + "  'displayName': 'DISPLAY_NAME',"
+        + "  'enabled':      true,"
+        + "  'clientId':    'CLIENT_ID',"
+        + "  'issuer':      'https://oidc.com/issuer'"
+        + "}").replace("'", "\"");
 
   @Test
   public void testJsonSerialization() throws IOException {
     OidcProviderConfig config = jsonFactory.fromString(OIDC_JSON_STRING, OidcProviderConfig.class);
 
-    assertEquals(config.getProviderId(), "oidc.provider-id");
-    assertEquals(config.getDisplayName(), "DISPLAY_NAME");
+    assertEquals("oidc.provider-id", config.getProviderId());
+    assertEquals("DISPLAY_NAME", config.getDisplayName());
     assertTrue(config.isEnabled());
-    assertEquals(config.getClientId(), "CLIENT_ID");
-    assertEquals(config.getIssuer(), "https://oidc.com/issuer");
+    assertEquals("CLIENT_ID", config.getClientId());
+    assertEquals("https://oidc.com/issuer", config.getIssuer());
   }
 
   @Test
@@ -70,11 +70,31 @@ public void testCreateRequest() throws IOException {
     assertEquals("https://oidc.com/issuer", (String) properties.get("issuer"));
   }
 
+  @Test(expected = IllegalArgumentException.class)
+  public void testCreateRequestMissingProviderId() {
+    new OidcProviderConfig.CreateRequest().setProviderId(null);
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void testCreateRequestInvalidProviderId() {
+    new OidcProviderConfig.CreateRequest().setProviderId("saml.provider-id");
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void testCreateRequestMissingDisplayName() {
+    new OidcProviderConfig.CreateRequest().setDisplayName(null);
+  }
+
   @Test(expected = IllegalArgumentException.class)
   public void testCreateRequestMissingClientId() {
     new OidcProviderConfig.CreateRequest().setClientId(null);
   }
 
+  @Test(expected = IllegalArgumentException.class)
+  public void testCreateRequestMissingIssuer() {
+    new OidcProviderConfig.CreateRequest().setIssuer(null);
+  }
+
   @Test(expected = IllegalArgumentException.class)
   public void testCreateRequestInvalidIssuerUrl() {
     new OidcProviderConfig.CreateRequest().setIssuer("not a valid url");
@@ -119,11 +139,21 @@ public void testUpdateRequestInvalidProviderId() {
     new OidcProviderConfig.UpdateRequest("saml.provider-id");
   }
 
+  @Test(expected = IllegalArgumentException.class)
+  public void testUpdateRequestMissingDisplayName() {
+    new OidcProviderConfig.UpdateRequest("oidc.provider-id").setDisplayName(null);
+  }
+
   @Test(expected = IllegalArgumentException.class)
   public void testUpdateRequestMissingClientId() {
     new OidcProviderConfig.UpdateRequest("oidc.provider-id").setClientId(null);
   }
 
+  @Test(expected = IllegalArgumentException.class)
+  public void testUpdateRequestMissingIssuer() {
+    new OidcProviderConfig.UpdateRequest("oidc.provider-id").setIssuer(null);
+  }
+
   @Test(expected = IllegalArgumentException.class)
   public void testUpdateRequestInvalidIssuerUrl() {
     new OidcProviderConfig.UpdateRequest("oidc.provider-id").setIssuer("not a valid url");