Use enum for legacy roles, add input checks

gcloud-java-core/src/main/java/com/google/gcloud/IamPolicy.java

@@ -23,8 +23,11 @@
 
 import java.io.Serializable;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -65,8 +68,14 @@ protected Builder() {}
 
     /**
      * Replaces the builder's map of bindings with the given map of bindings.
+     *
+     * @throws IllegalArgumentException if the provided map is null or contain any null values
      */
     public final B bindings(Map<R, Set<Identity>> bindings) {
+      checkArgument(bindings != null, "The provided map of bindings cannot be null.");
+      for (Map.Entry<R, Set<Identity>> binding : bindings.entrySet()) {
+        verifyBinding(binding.getKey(), binding.getValue());
+      }
       this.bindings.clear();
       for (Map.Entry<R, Set<Identity>> binding : bindings.entrySet()) {
         this.bindings.put(binding.getKey(), new HashSet<Identity>(binding.getValue()));
@@ -78,10 +87,12 @@ public final B bindings(Map<R, Set<Identity>> bindings) {
      * Adds a binding to the policy.
      *
      * @throws IllegalArgumentException if the policy already contains a binding with the same role
+     *     or if the role or any identities are null
      */
     public final B addBinding(R role, Set<Identity> identities) {
+      verifyBinding(role, identities);
       checkArgument(!bindings.containsKey(role),
-          "The policy already contains a binding with the role " + role.toString());
+          "The policy already contains a binding with the role " + role.toString() + ".");
       bindings.put(role, new HashSet<Identity>(identities));
       return self();
     }
@@ -90,15 +101,23 @@ public final B addBinding(R role, Set<Identity> identities) {
      * Adds a binding to the policy.
      *
      * @throws IllegalArgumentException if the policy already contains a binding with the same role
+     *     or if the role or any identities are null
      */
     public final B addBinding(R role, Identity first, Identity... others) {
-      checkArgument(!bindings.containsKey(role),
-          "The policy already contains a binding with the role " + role.toString());
       HashSet<Identity> identities = new HashSet<>();
       identities.add(first);
       identities.addAll(Arrays.asList(others));
-      bindings.put(role, identities);
-      return self();
+      return addBinding(role, identities);
+    }
+
+    private void verifyBinding(R role, Collection<Identity> identities) {
+      checkArgument(role != null, "The role cannot be null.");
+      verifyIdentities(identities);
+    }
+
+    private void verifyIdentities(Collection<Identity> identities) {
+      checkArgument(identities != null, "A role cannot be assigned to a null set of identities.");
+      checkArgument(!identities.contains(null), "Null identities are not permitted.");
     }
 
     /**
@@ -113,13 +132,16 @@ public final B removeBinding(R role) {
      * Adds one or more identities to an existing binding.
      *
      * @throws IllegalArgumentException if the policy doesn't contain a binding with the specified
-     *     role
+     *     role or any identities are null
      */
     public final B addIdentity(R role, Identity first, Identity... others) {
-      checkArgument(bindings.containsKey(role), "The policy doesn't contain the specified role.");
-      Set<Identity> identities = bindings.get(role);
-      identities.add(first);
-      identities.addAll(Arrays.asList(others));
+      checkArgument(bindings.containsKey(role),
+          "The policy doesn't contain the role " + role.toString() + ".");
+      List<Identity> toAdd = new LinkedList<>();
+      toAdd.add(first);
+      toAdd.addAll(Arrays.asList(others));
+      verifyIdentities(toAdd);
+      bindings.get(role).addAll(toAdd);
       return self();
     }
 
@@ -130,7 +152,8 @@ public final B addIdentity(R role, Identity first, Identity... others) {
      *     role
      */
     public final B removeIdentity(R role, Identity first, Identity... others) {
-      checkArgument(bindings.containsKey(role), "The policy doesn't contain the specified role.");
+      checkArgument(bindings.containsKey(role),
+          "The policy doesn't contain the role " + role.toString() + ".");
       bindings.get(role).remove(first);
       bindings.get(role).removeAll(Arrays.asList(others));
       return self();
@@ -179,6 +202,11 @@ protected IamPolicy(Builder<R, ? extends Builder<R, ?>> builder) {
     this.version = builder.version;
   }
 
+  /**
+   * Returns a builder containing the properties of this IAM Policy.
+   */
+  public abstract Builder<R, ? extends Builder<R, ?>> toBuilder();
+
   /**
    * The map of bindings that comprises the policy.
    */

gcloud-java-core/src/main/java/com/google/gcloud/Identity.java

@@ -18,11 +18,26 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.common.base.CaseFormat;
+
 import java.io.Serializable;
 import java.util.Objects;
 
 /**
- * An identity in an {@link IamPolicy}.
+ * An identity in an {@link IamPolicy}. The following types of identities are permitted in IAM
+ * policies:
+ * <ul>
+ * <li>Google account
+ * <li>Service account
+ * <li>Google group
+ * <li>Google Apps domain
+ * </ul>
+ *
+ * <p>There are also two special identities that represent all users and all Google-authenticated
+ * accounts.
+ *
+ * @see <a href="https://cloud.google.com/iam/docs/overview#concepts_related_to_identity">Concepts
+ *     related to identity</a>
  */
 public final class Identity implements Serializable {
 
@@ -82,7 +97,8 @@ public Type type() {
    *   <li>email address (for identities of type {@code USER}, {@code SERVICE_ACCOUNT}, and
    *       {@code GROUP})
    *   <li>domain (for identities of type {@code DOMAIN})
-   *   <li>null (for identities of type {@code ALL_USERS} and {@code ALL_AUTHENTICATED_USERS})
+   *   <li>{@code null} (for identities of type {@code ALL_USERS} and
+   *       {@code ALL_AUTHENTICATED_USERS})
    * </ul>
    */
   public String id() {
@@ -178,7 +194,7 @@ public String strValue() {
       case DOMAIN:
         return "domain:" + id;
       default:
-        throw new IllegalArgumentException("Unexpected identity type: " + type);
+        throw new IllegalStateException("Unexpected identity type: " + type);
     }
   }
 
@@ -188,21 +204,22 @@ public String strValue() {
    */
   public static Identity valueOf(String identityStr) {
     String[] info = identityStr.split(":");
-    switch (info[0]) {
-      case "allUsers":
+    Type type = Type.valueOf(CaseFormat.LOWER_CAMEL.to(CaseFormat.UPPER_UNDERSCORE, info[0]));
+    switch (type) {
+      case ALL_USERS:
         return Identity.allUsers();
-      case "allAuthenticatedUsers":
+      case ALL_AUTHENTICATED_USERS:
         return Identity.allAuthenticatedUsers();
-      case "user":
+      case USER:
         return Identity.user(info[1]);
-      case "serviceAccount":
+      case SERVICE_ACCOUNT:
         return Identity.serviceAccount(info[1]);
-      case "group":
+      case GROUP:
         return Identity.group(info[1]);
-      case "domain":
+      case DOMAIN:
         return Identity.domain(info[1]);
       default:
-        throw new IllegalArgumentException("Unexpected identity type: " + info[0]);
+        throw new IllegalStateException("Unexpected identity type " + type);
     }
   }
 }

gcloud-java-core/src/test/java/com/google/gcloud/IamPolicyTest.java

@@ -18,6 +18,7 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -71,7 +72,8 @@ public PolicyImpl build() {
       super(builder);
     }
 
-    Builder toBuilder() {
+    @Override
+    public Builder toBuilder() {
       return new Builder(bindings(), etag(), version());
     }
 
@@ -93,38 +95,38 @@ public void testBuilder() {
     assertEquals(1, policy.version().intValue());
     policy = SIMPLE_POLICY.toBuilder().removeBinding("editor").build();
     assertEquals(ImmutableMap.of("viewer", BINDINGS.get("viewer")), policy.bindings());
-    assertEquals(null, policy.etag());
-    assertEquals(null, policy.version());
+    assertNull(policy.etag());
+    assertNull(policy.version());
     policy = policy.toBuilder()
         .removeIdentity("viewer", USER, ALL_USERS)
         .addIdentity("viewer", DOMAIN, GROUP)
         .build();
     assertEquals(ImmutableMap.of("viewer", ImmutableSet.of(SERVICE_ACCOUNT, DOMAIN, GROUP)),
         policy.bindings());
-    assertEquals(null, policy.etag());
-    assertEquals(null, policy.version());
+    assertNull(policy.etag());
+    assertNull(policy.version());
     policy = PolicyImpl.builder().addBinding("owner", USER, SERVICE_ACCOUNT).build();
     assertEquals(
         ImmutableMap.of("owner", ImmutableSet.of(USER, SERVICE_ACCOUNT)), policy.bindings());
-    assertEquals(null, policy.etag());
-    assertEquals(null, policy.version());
+    assertNull(policy.etag());
+    assertNull(policy.version());
     try {
       SIMPLE_POLICY.toBuilder().addBinding("viewer", USER);
       fail("Should have failed due to duplicate role.");
     } catch (IllegalArgumentException e) {
-      assertEquals("The policy already contains a binding with the role viewer", e.getMessage());
+      assertEquals("The policy already contains a binding with the role viewer.", e.getMessage());
     }
     try {
       SIMPLE_POLICY.toBuilder().addBinding("editor", ImmutableSet.of(USER));
       fail("Should have failed due to duplicate role.");
     } catch (IllegalArgumentException e) {
-      assertEquals("The policy already contains a binding with the role editor", e.getMessage());
+      assertEquals("The policy already contains a binding with the role editor.", e.getMessage());
     }
   }
 
   @Test
   public void testEqualsHashCode() {
-    assertNotEquals(FULL_POLICY, null);
+    assertNotNull(FULL_POLICY);
     PolicyImpl emptyPolicy = PolicyImpl.builder().build();
     AnotherPolicyImpl anotherPolicy = new AnotherPolicyImpl.Builder().build();
     assertNotEquals(emptyPolicy, anotherPolicy);
@@ -151,7 +153,7 @@ public void testEtag() {
   @Test
   public void testVersion() {
     assertNull(SIMPLE_POLICY.version());
-    assertEquals(null, SIMPLE_POLICY.version());
+    assertEquals(1, FULL_POLICY.version().intValue());
   }
 
   static class AnotherPolicyImpl extends IamPolicy<String> {
@@ -169,5 +171,10 @@ public AnotherPolicyImpl build() {
     AnotherPolicyImpl(Builder builder) {
       super(builder);
     }
+
+    @Override
+    public Builder toBuilder() {
+      return new Builder();
+    }
   }
 }

gcloud-java-resourcemanager/src/main/java/com/google/gcloud/resourcemanager/Policy.java

@@ -17,6 +17,7 @@
 package com.google.gcloud.resourcemanager;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.CaseFormat;
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
@@ -40,19 +41,59 @@
  *
  * @see <a href="https://cloud.google.com/iam/reference/rest/v1/Policy">Policy</a>
  */
-public class Policy extends IamPolicy<String> {
+public class Policy extends IamPolicy<Policy.Role> {
 
   private static final long serialVersionUID = -5573557282693961850L;
 
+  /**
+   * Represents legacy roles in an IAM Policy.
+   */
+  public enum Role {
+
+    /**
+     * Permissions for read-only actions that preserve state.
+     */
+    VIEWER("roles/viewer"),
+
+    /**
+     * All viewer permissions and permissions for actions that modify state.
+     */
+    EDITOR("roles/editor"),
+
+    /**
+     * All editor permissions and permissions for the following actions:
+     * <ul>
+     * <li>Manage access control for a resource.
+     * <li>Set up billing (for a project).
+     * </ul>
+     */
+    OWNER("roles/owner");
+
+    private String strValue;
+
+    private Role(String strValue) {
+      this.strValue = strValue;
+    }
+
+    String strValue() {
+      return strValue;
+    }
+
+    static Role fromStr(String roleStr) {
+      return Role.valueOf(CaseFormat.LOWER_CAMEL.to(
+          CaseFormat.UPPER_UNDERSCORE, roleStr.substring("roles/".length())));
+    }
+  }
+
   /**
    * Builder for an IAM Policy.
    */
-  public static class Builder extends IamPolicy.Builder<String, Builder> {
+  public static class Builder extends IamPolicy.Builder<Role, Builder> {
 
     private Builder() {}
 
     @VisibleForTesting
-    Builder(Map<String, Set<Identity>> bindings, String etag, Integer version) {
+    Builder(Map<Role, Set<Identity>> bindings, String etag, Integer version) {
       bindings(bindings).etag(etag).version(version);
     }
 
@@ -70,6 +111,7 @@ public static Builder builder() {
     return new Builder();
   }
 
+  @Override
   public Builder toBuilder() {
     return new Builder(bindings(), etag(), version());
   }
@@ -79,10 +121,10 @@ com.google.api.services.cloudresourcemanager.model.Policy toPb() {
         new com.google.api.services.cloudresourcemanager.model.Policy();
     List<com.google.api.services.cloudresourcemanager.model.Binding> bindingPbList =
         new LinkedList<>();
-    for (Map.Entry<String, Set<Identity>> binding : bindings().entrySet()) {
+    for (Map.Entry<Role, Set<Identity>> binding : bindings().entrySet()) {
       com.google.api.services.cloudresourcemanager.model.Binding bindingPb =
           new com.google.api.services.cloudresourcemanager.model.Binding();
-      bindingPb.setRole("roles/" + binding.getKey());
+      bindingPb.setRole(binding.getKey().strValue());
       bindingPb.setMembers(
           Lists.transform(
               new ArrayList<>(binding.getValue()),
@@ -102,11 +144,11 @@ public String apply(Identity identity) {
 
   static Policy fromPb(
       com.google.api.services.cloudresourcemanager.model.Policy policyPb) {
-    Map<String, Set<Identity>> bindings = new HashMap<>();
+    Map<Role, Set<Identity>> bindings = new HashMap<>();
     for (com.google.api.services.cloudresourcemanager.model.Binding bindingPb :
         policyPb.getBindings()) {
       bindings.put(
-          bindingPb.getRole().substring("roles/".length()),
+          Role.fromStr(bindingPb.getRole()),
           ImmutableSet.copyOf(
               Lists.transform(
                   bindingPb.getMembers(),

gcloud-java-resourcemanager/src/test/java/com/google/gcloud/resourcemanager/PolicyTest.java

@@ -33,8 +33,8 @@ public class PolicyTest {
   private static final Identity GROUP = Identity.group("[email protected]");
   private static final Identity DOMAIN = Identity.domain("google.com");
   private static final Policy SIMPLE_POLICY = Policy.builder()
-      .addBinding("viewer", ImmutableSet.of(USER, SERVICE_ACCOUNT, ALL_USERS))
-      .addBinding("editor", ImmutableSet.of(ALL_AUTH_USERS, GROUP, DOMAIN))
+      .addBinding(Policy.Role.VIEWER, ImmutableSet.of(USER, SERVICE_ACCOUNT, ALL_USERS))
+      .addBinding(Policy.Role.EDITOR, ImmutableSet.of(ALL_AUTH_USERS, GROUP, DOMAIN))
       .build();
   private static final Policy FULL_POLICY =
       new Policy.Builder(SIMPLE_POLICY.bindings(), "etag", 1).build();