Skip to content

Commit c3b240d

Browse files
author
Ajay Kannan
committed
Use enum for legacy roles, add input checks
1 parent 08295ee commit c3b240d

File tree

6 files changed

+135
-43
lines changed

6 files changed

+135
-43
lines changed

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

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,11 @@
2323

2424
import java.io.Serializable;
2525
import java.util.Arrays;
26+
import java.util.Collection;
2627
import java.util.HashMap;
2728
import java.util.HashSet;
29+
import java.util.LinkedList;
30+
import java.util.List;
2831
import java.util.Map;
2932
import java.util.Objects;
3033
import java.util.Set;
@@ -65,8 +68,15 @@ protected Builder() {}
6568

6669
/**
6770
* Replaces the builder's map of bindings with the given map of bindings.
71+
*
72+
* @throws IllegalArgumentException if the provided map is null or contain any null values
6873
*/
6974
public final B bindings(Map<R, Set<Identity>> bindings) {
75+
checkArgument(bindings != null, "The provided map of bindings cannot be null.");
76+
for (R role : bindings.keySet()) {
77+
Set<Identity> identities = bindings.get(role);
78+
verifyBinding(role, identities);
79+
}
7080
this.bindings.clear();
7181
for (Map.Entry<R, Set<Identity>> binding : bindings.entrySet()) {
7282
this.bindings.put(binding.getKey(), new HashSet<Identity>(binding.getValue()));
@@ -78,10 +88,12 @@ public final B bindings(Map<R, Set<Identity>> bindings) {
7888
* Adds a binding to the policy.
7989
*
8090
* @throws IllegalArgumentException if the policy already contains a binding with the same role
91+
* or if the role or any identities are null
8192
*/
8293
public final B addBinding(R role, Set<Identity> identities) {
94+
verifyBinding(role, identities);
8395
checkArgument(!bindings.containsKey(role),
84-
"The policy already contains a binding with the role " + role.toString());
96+
"The policy already contains a binding with the role " + role.toString() + ".");
8597
bindings.put(role, new HashSet<Identity>(identities));
8698
return self();
8799
}
@@ -90,15 +102,23 @@ public final B addBinding(R role, Set<Identity> identities) {
90102
* Adds a binding to the policy.
91103
*
92104
* @throws IllegalArgumentException if the policy already contains a binding with the same role
105+
* or if the role or any identities are null
93106
*/
94107
public final B addBinding(R role, Identity first, Identity... others) {
95-
checkArgument(!bindings.containsKey(role),
96-
"The policy already contains a binding with the role " + role.toString());
97108
HashSet<Identity> identities = new HashSet<>();
98109
identities.add(first);
99110
identities.addAll(Arrays.asList(others));
100-
bindings.put(role, identities);
101-
return self();
111+
return addBinding(role, identities);
112+
}
113+
114+
private void verifyBinding(R role, Collection<Identity> identities) {
115+
checkArgument(role != null, "The role cannot be null.");
116+
checkArgument(identities != null, "A role cannot be assigned to a null set of identities.");
117+
verifyIdentities(identities);
118+
}
119+
120+
private void verifyIdentities(Collection<Identity> identities) {
121+
checkArgument(!identities.contains(null), "Null identities are not permitted.");
102122
}
103123

104124
/**
@@ -113,13 +133,16 @@ public final B removeBinding(R role) {
113133
* Adds one or more identities to an existing binding.
114134
*
115135
* @throws IllegalArgumentException if the policy doesn't contain a binding with the specified
116-
* role
136+
* role or any identities are null
117137
*/
118138
public final B addIdentity(R role, Identity first, Identity... others) {
119-
checkArgument(bindings.containsKey(role), "The policy doesn't contain the specified role.");
120-
Set<Identity> identities = bindings.get(role);
121-
identities.add(first);
122-
identities.addAll(Arrays.asList(others));
139+
checkArgument(bindings.containsKey(role),
140+
"The policy doesn't contain the role " + role.toString() + ".");
141+
List<Identity> toAdd = new LinkedList<>();
142+
toAdd.add(first);
143+
toAdd.addAll(Arrays.asList(others));
144+
verifyIdentities(toAdd);
145+
bindings.get(role).addAll(toAdd);
123146
return self();
124147
}
125148

@@ -130,7 +153,8 @@ public final B addIdentity(R role, Identity first, Identity... others) {
130153
* role
131154
*/
132155
public final B removeIdentity(R role, Identity first, Identity... others) {
133-
checkArgument(bindings.containsKey(role), "The policy doesn't contain the specified role.");
156+
checkArgument(bindings.containsKey(role),
157+
"The policy doesn't contain the role " + role.toString() + ".");
134158
bindings.get(role).remove(first);
135159
bindings.get(role).removeAll(Arrays.asList(others));
136160
return self();
@@ -179,6 +203,11 @@ protected IamPolicy(Builder<R, ? extends Builder<R, ?>> builder) {
179203
this.version = builder.version;
180204
}
181205

206+
/**
207+
* Returns a builder containing the properties of this IAM Policy.
208+
*/
209+
public abstract Builder<R, ? extends Builder<R, ?>> toBuilder();
210+
182211
/**
183212
* The map of bindings that comprises the policy.
184213
*/

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

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,26 @@
1818

1919
import static com.google.common.base.Preconditions.checkNotNull;
2020

21+
import com.google.common.base.CaseFormat;
22+
2123
import java.io.Serializable;
2224
import java.util.Objects;
2325

2426
/**
25-
* An identity in an {@link IamPolicy}.
27+
* An identity in an {@link IamPolicy}. The following types of identities are permitted in IAM
28+
* policies:
29+
* <ul>
30+
* <li>Google account
31+
* <li>Service account
32+
* <li>Google group
33+
* <li>Google Apps domain
34+
* </ul>
35+
*
36+
* <p>There are also two additional special identities that represent all users and all
37+
* Google-authenticated accounts.
38+
*
39+
* @see <a href="https://cloud.google.com/iam/docs/overview#concepts_related_to_identity">Concepts
40+
* related to identity</a>
2641
*/
2742
public final class Identity implements Serializable {
2843

@@ -82,7 +97,8 @@ public Type type() {
8297
* <li>email address (for identities of type {@code USER}, {@code SERVICE_ACCOUNT}, and
8398
* {@code GROUP})
8499
* <li>domain (for identities of type {@code DOMAIN})
85-
* <li>null (for identities of type {@code ALL_USERS} and {@code ALL_AUTHENTICATED_USERS})
100+
* <li>{@code null} (for identities of type {@code ALL_USERS} and
101+
* {@code ALL_AUTHENTICATED_USERS})
86102
* </ul>
87103
*/
88104
public String id() {
@@ -178,7 +194,7 @@ public String strValue() {
178194
case DOMAIN:
179195
return "domain:" + id;
180196
default:
181-
throw new IllegalArgumentException("Unexpected identity type: " + type);
197+
throw new IllegalStateException("Unexpected identity type: " + type);
182198
}
183199
}
184200

@@ -188,21 +204,22 @@ public String strValue() {
188204
*/
189205
public static Identity valueOf(String identityStr) {
190206
String[] info = identityStr.split(":");
191-
switch (info[0]) {
192-
case "allUsers":
207+
Type type = Type.valueOf(CaseFormat.LOWER_CAMEL.to(CaseFormat.UPPER_UNDERSCORE, info[0]));
208+
switch (type) {
209+
case ALL_USERS:
193210
return Identity.allUsers();
194-
case "allAuthenticatedUsers":
211+
case ALL_AUTHENTICATED_USERS:
195212
return Identity.allAuthenticatedUsers();
196-
case "user":
213+
case USER:
197214
return Identity.user(info[1]);
198-
case "serviceAccount":
215+
case SERVICE_ACCOUNT:
199216
return Identity.serviceAccount(info[1]);
200-
case "group":
217+
case GROUP:
201218
return Identity.group(info[1]);
202-
case "domain":
219+
case DOMAIN:
203220
return Identity.domain(info[1]);
204221
default:
205-
throw new IllegalArgumentException("Unexpected identity type: " + info[0]);
222+
throw new IllegalStateException("Unexpected identity type " + type);
206223
}
207224
}
208225
}

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

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import static org.junit.Assert.assertEquals;
2020
import static org.junit.Assert.assertNotEquals;
21+
import static org.junit.Assert.assertNotNull;
2122
import static org.junit.Assert.assertNull;
2223
import static org.junit.Assert.assertTrue;
2324
import static org.junit.Assert.fail;
@@ -71,7 +72,8 @@ public PolicyImpl build() {
7172
super(builder);
7273
}
7374

74-
Builder toBuilder() {
75+
@Override
76+
public Builder toBuilder() {
7577
return new Builder(bindings(), etag(), version());
7678
}
7779

@@ -93,38 +95,38 @@ public void testBuilder() {
9395
assertEquals(1, policy.version().intValue());
9496
policy = SIMPLE_POLICY.toBuilder().removeBinding("editor").build();
9597
assertEquals(ImmutableMap.of("viewer", BINDINGS.get("viewer")), policy.bindings());
96-
assertEquals(null, policy.etag());
97-
assertEquals(null, policy.version());
98+
assertNull(policy.etag());
99+
assertNull(policy.version());
98100
policy = policy.toBuilder()
99101
.removeIdentity("viewer", USER, ALL_USERS)
100102
.addIdentity("viewer", DOMAIN, GROUP)
101103
.build();
102104
assertEquals(ImmutableMap.of("viewer", ImmutableSet.of(SERVICE_ACCOUNT, DOMAIN, GROUP)),
103105
policy.bindings());
104-
assertEquals(null, policy.etag());
105-
assertEquals(null, policy.version());
106+
assertNull(policy.etag());
107+
assertNull(policy.version());
106108
policy = PolicyImpl.builder().addBinding("owner", USER, SERVICE_ACCOUNT).build();
107109
assertEquals(
108110
ImmutableMap.of("owner", ImmutableSet.of(USER, SERVICE_ACCOUNT)), policy.bindings());
109-
assertEquals(null, policy.etag());
110-
assertEquals(null, policy.version());
111+
assertNull(policy.etag());
112+
assertNull(policy.version());
111113
try {
112114
SIMPLE_POLICY.toBuilder().addBinding("viewer", USER);
113115
fail("Should have failed due to duplicate role.");
114116
} catch (IllegalArgumentException e) {
115-
assertEquals("The policy already contains a binding with the role viewer", e.getMessage());
117+
assertEquals("The policy already contains a binding with the role viewer.", e.getMessage());
116118
}
117119
try {
118120
SIMPLE_POLICY.toBuilder().addBinding("editor", ImmutableSet.of(USER));
119121
fail("Should have failed due to duplicate role.");
120122
} catch (IllegalArgumentException e) {
121-
assertEquals("The policy already contains a binding with the role editor", e.getMessage());
123+
assertEquals("The policy already contains a binding with the role editor.", e.getMessage());
122124
}
123125
}
124126

125127
@Test
126128
public void testEqualsHashCode() {
127-
assertNotEquals(FULL_POLICY, null);
129+
assertNotNull(FULL_POLICY);
128130
PolicyImpl emptyPolicy = PolicyImpl.builder().build();
129131
AnotherPolicyImpl anotherPolicy = new AnotherPolicyImpl.Builder().build();
130132
assertNotEquals(emptyPolicy, anotherPolicy);
@@ -151,7 +153,7 @@ public void testEtag() {
151153
@Test
152154
public void testVersion() {
153155
assertNull(SIMPLE_POLICY.version());
154-
assertEquals(null, SIMPLE_POLICY.version());
156+
assertEquals(1, FULL_POLICY.version().intValue());
155157
}
156158

157159
static class AnotherPolicyImpl extends IamPolicy<String> {
@@ -169,5 +171,10 @@ public AnotherPolicyImpl build() {
169171
AnotherPolicyImpl(Builder builder) {
170172
super(builder);
171173
}
174+
175+
@Override
176+
public Builder toBuilder() {
177+
return new Builder();
178+
}
172179
}
173180
}

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

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package com.google.gcloud.resourcemanager;
1818

1919
import com.google.common.annotations.VisibleForTesting;
20+
import com.google.common.base.CaseFormat;
2021
import com.google.common.base.Function;
2122
import com.google.common.collect.ImmutableSet;
2223
import com.google.common.collect.Lists;
@@ -40,19 +41,56 @@
4041
*
4142
* @see <a href="https://cloud.google.com/iam/reference/rest/v1/Policy">Policy</a>
4243
*/
43-
public class Policy extends IamPolicy<String> {
44+
public class Policy extends IamPolicy<Policy.Role> {
4445

4546
private static final long serialVersionUID = -5573557282693961850L;
4647

48+
public enum Role {
49+
50+
/**
51+
* Permissions for read-only actions that preserve state.
52+
*/
53+
VIEWER("roles/viewer"),
54+
55+
/**
56+
* All viewer permissions and permissions for actions that modify state.
57+
*/
58+
EDITOR("roles/editor"),
59+
60+
/**
61+
* All editor permissions and permissions for the following actions:
62+
* <ul>
63+
* <li>Manage access control for a resource.
64+
* <li>Set up billing (for a project).
65+
* </ul>
66+
*/
67+
OWNER("roles/owner");
68+
69+
private String strValue;
70+
71+
private Role(String strValue) {
72+
this.strValue = strValue;
73+
}
74+
75+
String strValue() {
76+
return strValue;
77+
}
78+
79+
static Role fromStr(String roleStr) {
80+
return Role.valueOf(CaseFormat.LOWER_CAMEL.to(
81+
CaseFormat.UPPER_UNDERSCORE, roleStr.substring("roles/".length())));
82+
}
83+
}
84+
4785
/**
4886
* Builder for an IAM Policy.
4987
*/
50-
public static class Builder extends IamPolicy.Builder<String, Builder> {
88+
public static class Builder extends IamPolicy.Builder<Role, Builder> {
5189

5290
private Builder() {}
5391

5492
@VisibleForTesting
55-
Builder(Map<String, Set<Identity>> bindings, String etag, Integer version) {
93+
Builder(Map<Role, Set<Identity>> bindings, String etag, Integer version) {
5694
bindings(bindings).etag(etag).version(version);
5795
}
5896

@@ -70,6 +108,7 @@ public static Builder builder() {
70108
return new Builder();
71109
}
72110

111+
@Override
73112
public Builder toBuilder() {
74113
return new Builder(bindings(), etag(), version());
75114
}
@@ -79,10 +118,10 @@ com.google.api.services.cloudresourcemanager.model.Policy toPb() {
79118
new com.google.api.services.cloudresourcemanager.model.Policy();
80119
List<com.google.api.services.cloudresourcemanager.model.Binding> bindingPbList =
81120
new LinkedList<>();
82-
for (Map.Entry<String, Set<Identity>> binding : bindings().entrySet()) {
121+
for (Map.Entry<Role, Set<Identity>> binding : bindings().entrySet()) {
83122
com.google.api.services.cloudresourcemanager.model.Binding bindingPb =
84123
new com.google.api.services.cloudresourcemanager.model.Binding();
85-
bindingPb.setRole("roles/" + binding.getKey());
124+
bindingPb.setRole(binding.getKey().strValue());
86125
bindingPb.setMembers(
87126
Lists.transform(
88127
new ArrayList<>(binding.getValue()),
@@ -102,11 +141,11 @@ public String apply(Identity identity) {
102141

103142
static Policy fromPb(
104143
com.google.api.services.cloudresourcemanager.model.Policy policyPb) {
105-
Map<String, Set<Identity>> bindings = new HashMap<>();
144+
Map<Role, Set<Identity>> bindings = new HashMap<>();
106145
for (com.google.api.services.cloudresourcemanager.model.Binding bindingPb :
107146
policyPb.getBindings()) {
108147
bindings.put(
109-
bindingPb.getRole().substring("roles/".length()),
148+
Role.fromStr(bindingPb.getRole()),
110149
ImmutableSet.copyOf(
111150
Lists.transform(
112151
bindingPb.getMembers(),

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ public class PolicyTest {
3333
private static final Identity GROUP = Identity.group("[email protected]");
3434
private static final Identity DOMAIN = Identity.domain("google.com");
3535
private static final Policy SIMPLE_POLICY = Policy.builder()
36-
.addBinding("viewer", ImmutableSet.of(USER, SERVICE_ACCOUNT, ALL_USERS))
37-
.addBinding("editor", ImmutableSet.of(ALL_AUTH_USERS, GROUP, DOMAIN))
36+
.addBinding(Policy.Role.VIEWER, ImmutableSet.of(USER, SERVICE_ACCOUNT, ALL_USERS))
37+
.addBinding(Policy.Role.EDITOR, ImmutableSet.of(ALL_AUTH_USERS, GROUP, DOMAIN))
3838
.build();
3939
private static final Policy FULL_POLICY =
4040
new Policy.Builder(SIMPLE_POLICY.bindings(), "etag", 1).build();

0 commit comments

Comments
 (0)