Add binding entries as necessary in IamPolicy.Builder

Author: Ajay Kannan

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

@@ -83,39 +83,8 @@ public final B bindings(Map<R, Set<Identity>> bindings) {
       return self();
     }
 
-    /**
-     * 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() + ".");
-      bindings.put(role, new HashSet<Identity>(identities));
-      return self();
-    }
-
-    /**
-     * 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) {
-      HashSet<Identity> identities = new HashSet<>();
-      identities.add(first);
-      identities.addAll(Arrays.asList(others));
-      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.");
     }
@@ -129,33 +98,33 @@ 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 or any identities are null
+     * Adds one or more identities to the policy under the role specified. Creates a new role
+     * binding if the binding corresponding to the given role did not previously exist.
      */
     public final B addIdentity(R role, Identity first, Identity... 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);
+      verifyBinding(role, toAdd);
+      Set<Identity> identities = bindings.get(role);
+      if (identities == null) {
+        identities = new HashSet<Identity>();
+        bindings.put(role, identities);
+      }
+      identities.addAll(toAdd);
       return self();
     }
 
     /**
-     * Removes one or more identities from an existing binding.
-     *
-     * @throws IllegalArgumentException if the policy doesn't contain a binding with the specified
-     *     role
+     * Removes one or more identities from an existing binding. Does nothing if the binding
+     * associated with the provided role doesn't exist.
      */
     public final B removeIdentity(R role, Identity first, Identity... others) {
-      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));
+      Set<Identity> identities = bindings.get(role);
+      if (identities != null) {
+        identities.remove(first);
+        identities.removeAll(Arrays.asList(others));
+      }
       return self();
     }
 

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

@@ -28,6 +28,7 @@
 
 import org.junit.Test;
 
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
 
@@ -46,8 +47,8 @@ public class IamPolicyTest {
       "editor",
       ImmutableSet.of(ALL_AUTH_USERS, GROUP, DOMAIN));
   private static final PolicyImpl SIMPLE_POLICY = PolicyImpl.builder()
-      .addBinding("viewer", ImmutableSet.of(USER, SERVICE_ACCOUNT, ALL_USERS))
-      .addBinding("editor", ImmutableSet.of(ALL_AUTH_USERS, GROUP, DOMAIN))
+      .addIdentity("viewer", USER, SERVICE_ACCOUNT, ALL_USERS)
+      .addIdentity("editor", ALL_AUTH_USERS, GROUP, DOMAIN)
       .build();
   private static final PolicyImpl FULL_POLICY =
       new PolicyImpl.Builder(SIMPLE_POLICY.bindings(), "etag", 1).build();
@@ -105,22 +106,43 @@ public void testBuilder() {
         policy.bindings());
     assertNull(policy.etag());
     assertNull(policy.version());
-    policy = PolicyImpl.builder().addBinding("owner", USER, SERVICE_ACCOUNT).build();
+    policy = PolicyImpl.builder()
+        .removeIdentity("viewer", USER)
+        .addIdentity("owner", USER, SERVICE_ACCOUNT)
+        .build();
     assertEquals(
         ImmutableMap.of("owner", ImmutableSet.of(USER, SERVICE_ACCOUNT)), policy.bindings());
     assertNull(policy.etag());
     assertNull(policy.version());
+  }
+
+  @Test
+  public void testIllegalPolicies() {
+    try {
+      PolicyImpl.builder().addIdentity(null, USER);
+      fail("Null role should cause exception.");
+    } catch (IllegalArgumentException ex) {
+      assertEquals("The role cannot be null.", ex.getMessage());
+    }
+    try {
+      PolicyImpl.builder().addIdentity("viewer", null, USER);
+      fail("Null identity should cause exception.");
+    } catch (IllegalArgumentException ex) {
+      assertEquals("Null identities are not permitted.", ex.getMessage());
+    }
     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());
+      PolicyImpl.builder().bindings(null);
+      fail("Null bindings map should cause exception.");
+    } catch (IllegalArgumentException ex) {
+      assertEquals("The provided map of bindings cannot be null.", ex.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());
+      Map<String, Set<Identity>> bindings = new HashMap<>();
+      bindings.put("viewer", null);
+      PolicyImpl.builder().bindings(bindings);
+      fail("Null set of identities should cause exception.");
+    } catch (IllegalArgumentException ex) {
+      assertEquals("A role cannot be assigned to a null set of identities.", ex.getMessage());
     }
   }
 

gcloud-java-examples/src/main/java/com/google/gcloud/examples/resourcemanager/snippets/ModifyPolicy.java

@@ -49,11 +49,7 @@ public static void main(String... args) {
     // Add a viewer
     Policy.Builder modifiedPolicy = policy.toBuilder();
     Identity newViewer = Identity.user("<insert user's email address here>");
-    if (policy.bindings().containsKey(Role.viewer())) {
-      modifiedPolicy.addIdentity(Role.viewer(), newViewer);
-    } else {
-      modifiedPolicy.addBinding(Role.viewer(), newViewer);
-    }
+    modifiedPolicy.addIdentity(Role.viewer(), newViewer);
 
     // Write policy
     Policy updatedPolicy = project.replacePolicy(modifiedPolicy.build());

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

@@ -202,10 +202,10 @@ public Project replace() {
   }
 
   /**
-   * Returns the IAM access control policy for the specified project. Returns {@code null} if the
-   * resource does not exist or if you do not have adequate permission to view the project or get
-   * the policy.
+   * Returns the IAM access control policy for this project. Returns {@code null} if the resource
+   * does not exist or if you do not have adequate permission to view the project or get the policy.
    *
+   * @return the IAM policy for the project
    * @throws ResourceManagerException upon failure
    * @see <a href=
    *     "https://cloud.google.com/resource-manager/reference/rest/v1beta1/projects/getIamPolicy">
@@ -216,12 +216,12 @@ public Policy getPolicy() {
   }
 
   /**
-   * Sets the IAM access control policy for the specified project. Replaces any existing policy.
-   * It is recommended that you use the read-modify-write pattern. See code samples and important
-   * details of replacing policies in the documentation for {@link ResourceManager#replacePolicy}.
+   * Sets the IAM access control policy for this project. Replaces any existing policy. It is
+   * recommended that you use the read-modify-write pattern. See code samples and important details
+   * of replacing policies in the documentation for {@link ResourceManager#replacePolicy}.
    *
+   * @return the newly set IAM policy for this project
    * @throws ResourceManagerException upon failure
-   * @see ResourceManager#replacePolicy
    * @see <a href=
    *     "https://cloud.google.com/resource-manager/reference/rest/v1beta1/projects/setIamPolicy">
    *     Resource Manager setIamPolicy</a>
@@ -237,7 +237,7 @@ public Policy replacePolicy(Policy newPolicy) {
    * For example, the Cloud Platform Console tests IAM permissions internally to determine which UI
    * should be available to the logged-in user.
    *
-   * @return A list of booleans representing whether the caller has the permissions specified (in
+   * @return a list of booleans representing whether the caller has the permissions specified (in
    *     the order of the given permissions)
    * @throws ResourceManagerException upon failure
    * @see <a href=
@@ -255,7 +255,7 @@ List<Boolean> testPermissions(List<Permission> permissions) {
    * For example, the Cloud Platform Console tests IAM permissions internally to determine which UI
    * should be available to the logged-in user.
    *
-   * @return A list of booleans representing whether the caller has the permissions specified (in
+   * @return a list of booleans representing whether the caller has the permissions specified (in
    *     the order of the given permissions)
    * @throws ResourceManagerException upon failure
    * @see <a href=

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

@@ -37,10 +37,10 @@ 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(Role.owner(), ImmutableSet.of(USER))
-      .addBinding(Role.viewer(), ImmutableSet.of(ALL_USERS))
-      .addBinding(Role.editor(), ImmutableSet.of(ALL_AUTH_USERS, DOMAIN))
-      .addBinding(Role.rawRole("some-role"), ImmutableSet.of(SERVICE_ACCOUNT, GROUP))
+      .addIdentity(Role.owner(), USER)
+      .addIdentity(Role.viewer(), ALL_USERS)
+      .addIdentity(Role.editor(), ALL_AUTH_USERS, DOMAIN)
+      .addIdentity(Role.rawRole("some-role"), SERVICE_ACCOUNT, GROUP)
       .build();
   private static final Policy FULL_POLICY =
       new Policy.Builder(SIMPLE_POLICY.bindings(), "etag", 1).build();
@@ -68,10 +68,10 @@ public void testRoleType() {
   @Test
   public void testEquals() {
     Policy copy = Policy.builder()
-        .addBinding(Role.owner(), ImmutableSet.of(USER))
-        .addBinding(Role.viewer(), ImmutableSet.of(ALL_USERS))
-        .addBinding(Role.editor(), ImmutableSet.of(ALL_AUTH_USERS, DOMAIN))
-        .addBinding(Role.rawRole("some-role"), ImmutableSet.of(SERVICE_ACCOUNT, GROUP))
+        .addIdentity(Role.owner(), USER)
+        .addIdentity(Role.viewer(), ALL_USERS)
+        .addIdentity(Role.editor(), ALL_AUTH_USERS, DOMAIN)
+        .addIdentity(Role.rawRole("some-role"), SERVICE_ACCOUNT, GROUP)
         .build();
     assertEquals(SIMPLE_POLICY, copy);
     assertNotEquals(SIMPLE_POLICY, FULL_POLICY);

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

@@ -58,8 +58,8 @@ public class ProjectTest {
   private static final Identity SERVICE_ACCOUNT =
       Identity.serviceAccount("[email protected]");
   private static final Policy POLICY = Policy.builder()
-      .addBinding(Role.owner(), ImmutableSet.of(USER))
-      .addBinding(Role.editor(), ImmutableSet.of(SERVICE_ACCOUNT))
+      .addIdentity(Role.owner(), USER)
+      .addIdentity(Role.editor(), SERVICE_ACCOUNT)
       .build();
 
   private ResourceManager serviceMockReturnsOptions = createStrictMock(ResourceManager.class);

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

@@ -72,8 +72,8 @@ public class ResourceManagerImplTest {
       .build();
   private static final Map<ResourceManagerRpc.Option, ?> EMPTY_RPC_OPTIONS = ImmutableMap.of();
   private static final Policy POLICY = Policy.builder()
-      .addBinding(Role.owner(), Identity.user("[email protected]"))
-      .addBinding(Role.editor(), Identity.serviceAccount("[email protected]"))
+      .addIdentity(Role.owner(), Identity.user("[email protected]"))
+      .addIdentity(Role.editor(), Identity.serviceAccount("[email protected]"))
       .build();
 
   @Rule

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

@@ -17,7 +17,6 @@
 package com.google.gcloud.resourcemanager;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.gcloud.BaseSerializationTest;
 import com.google.gcloud.Identity;
 import com.google.gcloud.PageImpl;
@@ -46,9 +45,8 @@ public class SerializationTest extends BaseSerializationTest {
       ResourceManager.ProjectGetOption.fields(ResourceManager.ProjectField.NAME);
   private static final ResourceManager.ProjectListOption PROJECT_LIST_OPTION =
       ResourceManager.ProjectListOption.filter("name:*");
-  private static final Policy POLICY = Policy.builder()
-      .addBinding(Policy.Role.viewer(), ImmutableSet.of(Identity.user("[email protected]")))
-      .build();
+  private static final Policy POLICY =
+      Policy.builder().addIdentity(Policy.Role.viewer(), Identity.user("[email protected]")).build();
   private static final ResourceManagerException RESOURCE_MANAGER_EXCEPTION =
       new ResourceManagerException(42, "message");