You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by ji...@apache.org on 2018/01/18 16:17:57 UTC
[geode] branch develop updated: GEODE-4310: allow
ResourcePermission to take Strings as arguments for Resource and Operation.
(#1300)
This is an automated email from the ASF dual-hosted git repository.
jinmeiliao pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new fccdd15 GEODE-4310: allow ResourcePermission to take Strings as arguments for Resource and Operation. (#1300)
fccdd15 is described below
commit fccdd15d767db103c9ae8a0b66afe28ca3f3d439
Author: jinmeiliao <ji...@pivotal.io>
AuthorDate: Thu Jan 18 08:17:51 2018 -0800
GEODE-4310: allow ResourcePermission to take Strings as arguments for Resource and Operation. (#1300)
* still checks validity of the resource/operation except wildcards.
* update tests
---
.../internal/security/ResourcePermissions.java | 6 +
.../apache/geode/security/ResourcePermission.java | 126 +++++++++-----
.../internal/security/ResourcePermissionTest.java | 185 +++++++++++++++------
3 files changed, 223 insertions(+), 94 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/security/ResourcePermissions.java b/geode-core/src/main/java/org/apache/geode/management/internal/security/ResourcePermissions.java
index a0db662..5565a46 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/security/ResourcePermissions.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/security/ResourcePermissions.java
@@ -22,8 +22,14 @@ import static org.apache.geode.security.ResourcePermission.Resource.CLUSTER;
import static org.apache.geode.security.ResourcePermission.Resource.DATA;
import org.apache.geode.security.ResourcePermission;
+import org.apache.geode.security.ResourcePermission.Operation;
+import org.apache.geode.security.ResourcePermission.Resource;
public final class ResourcePermissions {
+ public static final ResourcePermission ALL = new ResourcePermission(Resource.ALL, Operation.ALL);
+ public static final ResourcePermission DATA_ALL = new ResourcePermission(DATA, Operation.ALL);
+ public static final ResourcePermission CLUSTER_ALL =
+ new ResourcePermission(CLUSTER, Operation.ALL);
public static final ResourcePermission DATA_READ = new ResourcePermission(DATA, READ);
public static final ResourcePermission DATA_WRITE = new ResourcePermission(DATA, WRITE);
public static final ResourcePermission DATA_MANAGE = new ResourcePermission(DATA, MANAGE);
diff --git a/geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java b/geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java
index 7cd7763..74db186 100644
--- a/geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java
+++ b/geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java
@@ -14,6 +14,8 @@
*/
package org.apache.geode.security;
+import java.util.function.UnaryOperator;
+
import org.apache.commons.lang.StringUtils;
import org.apache.shiro.authz.permission.WildcardPermission;
@@ -29,6 +31,7 @@ import org.apache.geode.cache.Region;
public class ResourcePermission extends WildcardPermission {
public static String ALL = "*";
+ public static String NULL = "NULL";
/**
* @deprecated use ALL
@@ -40,81 +43,81 @@ public class ResourcePermission extends WildcardPermission {
public static String ALL_KEYS = "*";
public enum Resource {
- NULL, CLUSTER, DATA
+ ALL, NULL, CLUSTER, DATA;
+
+ public String getName() {
+ if (this == ALL) {
+ return ResourcePermission.ALL;
+ }
+ return name();
+ }
}
public enum Operation {
- NULL, MANAGE, WRITE, READ
+ ALL, NULL, MANAGE, WRITE, READ;
+ public String getName() {
+ if (this == ALL) {
+ return ResourcePermission.ALL;
+ }
+ return name();
+ }
}
- // when ALL is specified, we need it to convert to string "*" instead of "ALL".
public enum Target {
- ALL(ResourcePermission.ALL), DISK, GATEWAY, QUERY, DEPLOY;
-
- private String name;
-
- Target() {}
-
- Target(String name) {
- this.name = name;
- }
-
+ ALL, DISK, GATEWAY, QUERY, DEPLOY;
public String getName() {
- if (name != null) {
- return name;
+ if (this == ALL) {
+ return ResourcePermission.ALL;
}
return name();
}
}
- // these default values are used when creating a lock around an operation
- private Resource resource = Resource.NULL;
- private Operation operation = Operation.NULL;
+ // these default values are used when creating an allow-all lock around an operation
+ private String resource = NULL;
+ private String operation = NULL;
private String target = ALL;
private String key = ALL;
- public ResourcePermission() {
- this(Resource.NULL, Operation.NULL, ALL, ALL);
- }
+ public ResourcePermission() {}
- public ResourcePermission(String resource, String operation) {
+ public ResourcePermission(Resource resource, Operation operation) {
this(resource, operation, ALL, ALL);
}
- public ResourcePermission(String resource, String operation, String target) {
+ public ResourcePermission(Resource resource, Operation operation, String target) {
this(resource, operation, target, ALL);
}
- public ResourcePermission(String resource, String operation, String target, String key) {
- this((resource == null) ? Resource.NULL : Resource.valueOf(resource.toUpperCase()),
- (operation == null) ? Operation.NULL : Operation.valueOf(operation.toUpperCase()), target,
+ public ResourcePermission(Resource resource, Operation operation, Target target) {
+ this(resource, operation, target, ALL);
+ }
+
+ public ResourcePermission(Resource resource, Operation operation, Target target, String key) {
+ this(resource == null ? null : resource.getName(),
+ operation == null ? null : operation.getName(), target == null ? null : target.getName(),
key);
}
- public ResourcePermission(Resource resource, Operation operation) {
- this(resource, operation, ALL, ALL);
+ public ResourcePermission(Resource resource, Operation operation, String target, String key) {
+ this(resource == null ? null : resource.getName(),
+ operation == null ? null : operation.getName(), target, key);
}
- public ResourcePermission(Resource resource, Operation operation, String target) {
- this(resource, operation, target, ALL);
+ public ResourcePermission(String resource, String operation) {
+ this(resource, operation, ALL, ALL);
}
- public ResourcePermission(Resource resource, Operation operation, Target target) {
+ public ResourcePermission(String resource, String operation, String target) {
this(resource, operation, target, ALL);
}
- public ResourcePermission(Resource resource, Operation operation, Target target,
- String targetKey) {
- this(resource, operation, target.getName(), targetKey);
- }
+ public ResourcePermission(String resource, String operation, String target, String key) {
+ // what's eventually stored are either "*", "NULL" or a valid enum except ALL.
+ // Fields are never null.
+ this.resource = parsePart(resource, r -> Resource.valueOf(r).getName());
+ this.operation = parsePart(operation, o -> Operation.valueOf(o).getName());
- public ResourcePermission(Resource resource, Operation operation, String target, String key) {
- if (resource != null) {
- this.resource = resource;
- }
- if (operation != null) {
- this.operation = operation;
- }
if (target != null) {
this.target = StringUtils.stripStart(target, Region.SEPARATOR);
}
@@ -125,17 +128,48 @@ public class ResourcePermission extends WildcardPermission {
setParts(this.resource + ":" + this.operation + ":" + this.target + ":" + this.key, true);
}
+ private String parsePart(String part, UnaryOperator<String> operator) {
+ if (part == null) {
+ return NULL;
+ }
+ if (part.equals(ALL)) {
+ return ALL;
+ }
+ return operator.apply(part.toUpperCase());
+ }
+
/**
- * Returns the resource, could be either DATA or CLUSTER
+ * Returns the resource, could be either ALL, NULL, DATA or CLUSTER
*/
public Resource getResource() {
- return resource;
+ if (ALL.equals(resource)) {
+ return Resource.ALL;
+ }
+
+ return Resource.valueOf(resource);
}
/**
- * Returns the operation, could be either MANAGE, WRITE or READ
+ * Returns the operation, could be either ALL, NULL, MANAGE, WRITE or READ
*/
public Operation getOperation() {
+ if (ALL.equals(operation))
+ return Operation.ALL;
+ return Operation.valueOf(operation);
+ }
+
+
+ /**
+ * could be either "*", "NULL", "DATA", "CLUSTER"
+ */
+ public String getResourceString() {
+ return resource;
+ }
+
+ /**
+ * Returns the operation, could be either "*", "NULL", "MANAGE", "WRITE" or "READ"
+ */
+ public String getOperationString() {
return operation;
}
@@ -163,7 +197,7 @@ public class ResourcePermission extends WildcardPermission {
@Override
public String toString() {
if (ALL.equals(target)) {
- return getResource() + ":" + getOperation();
+ return resource + ":" + operation;
} else if (ALL.equals(key)) {
return resource + ":" + operation + ":" + target;
} else {
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/security/ResourcePermissionTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/security/ResourcePermissionTest.java
index 48f93d8..4b15407 100644
--- a/geode-core/src/test/java/org/apache/geode/management/internal/security/ResourcePermissionTest.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/security/ResourcePermissionTest.java
@@ -15,7 +15,7 @@
package org.apache.geode.management.internal.security;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.assertEquals;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.Assert.assertTrue;
import org.apache.shiro.authz.permission.WildcardPermission;
@@ -34,9 +34,9 @@ public class ResourcePermissionTest {
@Test
public void testEmptyConstructor() {
ResourcePermission context = new ResourcePermission();
- assertEquals(Resource.NULL, context.getResource());
- assertEquals(Operation.NULL, context.getOperation());
- assertEquals(ResourcePermission.ALL, context.getTarget());
+ assertThat(Resource.NULL).isEqualTo(context.getResource());
+ assertThat(Operation.NULL).isEqualTo(context.getOperation());
+ assertThat(ResourcePermission.ALL).isEqualTo(context.getTarget());
}
@Test
@@ -47,68 +47,157 @@ public class ResourcePermissionTest {
@Test
public void testConstructor() {
- ResourcePermission context = new ResourcePermission();
- assertEquals(Resource.NULL, context.getResource());
- assertEquals(Operation.NULL, context.getOperation());
- assertEquals(ResourcePermission.ALL, context.getTarget());
-
- context = new ResourcePermission();
- assertEquals(Resource.NULL, context.getResource());
- assertEquals(Operation.NULL, context.getOperation());
- assertEquals(ResourcePermission.ALL, context.getTarget());
-
- context = new ResourcePermission(Resource.DATA, null);
- assertEquals(Resource.DATA, context.getResource());
- assertEquals(Operation.NULL, context.getOperation());
- assertEquals(ResourcePermission.ALL, context.getTarget());
-
- context = new ResourcePermission(Resource.CLUSTER, null);
- assertEquals(Resource.CLUSTER, context.getResource());
- assertEquals(Operation.NULL, context.getOperation());
- assertEquals(ResourcePermission.ALL, context.getTarget());
-
- context = new ResourcePermission(null, Operation.MANAGE, "REGIONA");
- assertEquals(Resource.NULL, context.getResource());
- assertEquals(Operation.MANAGE, context.getOperation());
- assertEquals("REGIONA", context.getTarget());
+ ResourcePermission permission = new ResourcePermission();
+ assertThat(Resource.NULL).isEqualTo(permission.getResource());
+ assertThat(Operation.NULL).isEqualTo(permission.getOperation());
+ assertThat(ResourcePermission.ALL).isEqualTo(permission.getTarget());
+
+ permission = new ResourcePermission();
+ assertThat(Resource.NULL).isEqualTo(permission.getResource());
+ assertThat(Operation.NULL).isEqualTo(permission.getOperation());
+ assertThat(ResourcePermission.ALL).isEqualTo(permission.getTarget());
+
+ permission = new ResourcePermission(Resource.DATA, null);
+ assertThat(Resource.DATA).isEqualTo(permission.getResource());
+ assertThat(Operation.NULL).isEqualTo(permission.getOperation());
+ assertThat(ResourcePermission.ALL).isEqualTo(permission.getTarget());
+
+ permission = new ResourcePermission(Resource.CLUSTER, null);
+ assertThat(Resource.CLUSTER).isEqualTo(permission.getResource());
+ assertThat(Operation.NULL).isEqualTo(permission.getOperation());
+ assertThat(ResourcePermission.ALL).isEqualTo(permission.getTarget());
+
+ permission = new ResourcePermission(null, Operation.MANAGE, "REGIONA");
+ assertThat(Resource.NULL).isEqualTo(permission.getResource());
+ assertThat(Operation.MANAGE).isEqualTo(permission.getOperation());
+ assertThat("REGIONA").isEqualTo(permission.getTarget());
+
+ permission = new ResourcePermission(Resource.DATA, Operation.MANAGE, "REGIONA");
+ assertThat(Resource.DATA).isEqualTo(permission.getResource());
+ assertThat(Operation.MANAGE).isEqualTo(permission.getOperation());
+ assertThat("REGIONA").isEqualTo(permission.getTarget());
+
+ permission = new ResourcePermission(Resource.CLUSTER, Operation.MANAGE);
+ assertThat(Resource.CLUSTER).isEqualTo(permission.getResource());
+ assertThat(Operation.MANAGE).isEqualTo(permission.getOperation());
+ assertThat(ResourcePermission.ALL).isEqualTo(permission.getTarget());
- context = new ResourcePermission(Resource.DATA, Operation.MANAGE, "REGIONA");
- assertEquals(Resource.DATA, context.getResource());
- assertEquals(Operation.MANAGE, context.getOperation());
- assertEquals("REGIONA", context.getTarget());
+ // make sure "ALL" in the resource "DATA" means regionName won't be converted to *
+ permission = new ResourcePermission(Resource.DATA, Operation.READ, "ALL");
+ assertThat(Resource.DATA).isEqualTo(permission.getResource());
+ assertThat(Operation.READ).isEqualTo(permission.getOperation());
+ assertThat("ALL").isEqualTo(permission.getTarget());
+
+ permission = new ResourcePermission(Resource.CLUSTER, Operation.READ, Target.ALL);
+ assertThat(permission.getTarget()).isEqualTo(ResourcePermission.ALL);
+
+ permission = new ResourcePermission((String) null, (String) null);
+ assertThat(permission.getResourceString()).isEqualTo(ResourcePermission.NULL);
+ assertThat(permission.getOperationString()).isEqualTo(ResourcePermission.NULL);
+ assertThat(permission.getTarget()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getKey()).isEqualTo(ResourcePermission.ALL);
+
+ permission = new ResourcePermission((Resource) null, (Operation) null);
+ assertThat(permission.getResource()).isEqualTo(Resource.NULL);
+ assertThat(permission.getOperation()).isEqualTo(Operation.NULL);
+ assertThat(permission.getResourceString()).isEqualTo(ResourcePermission.NULL);
+ assertThat(permission.getOperationString()).isEqualTo(ResourcePermission.NULL);
+ assertThat(permission.getTarget()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getKey()).isEqualTo(ResourcePermission.ALL);
+
+ permission = new ResourcePermission("*", "*");
+ assertThat(permission.getResource()).isEqualTo(Resource.ALL);
+ assertThat(permission.getOperation()).isEqualTo(Operation.ALL);
+ assertThat(permission.getResourceString()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getOperationString()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getTarget()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getKey()).isEqualTo(ResourcePermission.ALL);
+
+ permission = new ResourcePermission("ALL", "ALL");
+ assertThat(permission.getResource()).isEqualTo(Resource.ALL);
+ assertThat(permission.getOperation()).isEqualTo(Operation.ALL);
+ assertThat(permission.getResourceString()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getOperationString()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getTarget()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getKey()).isEqualTo(ResourcePermission.ALL);
+
+ permission = new ResourcePermission("NULL", "NULL");
+ assertThat(permission.getResource()).isEqualTo(Resource.NULL);
+ assertThat(permission.getOperation()).isEqualTo(Operation.NULL);
+ assertThat(permission.getResourceString()).isEqualTo(ResourcePermission.NULL);
+ assertThat(permission.getOperationString()).isEqualTo(ResourcePermission.NULL);
+ assertThat(permission.getTarget()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getKey()).isEqualTo(ResourcePermission.ALL);
+
+ permission = new ResourcePermission("*", "READ");
+ assertThat(permission.getResource()).isEqualTo(Resource.ALL);
+ assertThat(permission.getOperation()).isEqualTo(Operation.READ);
+ assertThat(permission.getResourceString()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getOperationString()).isEqualTo("READ");
+ assertThat(permission.getTarget()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.getKey()).isEqualTo(ResourcePermission.ALL);
+ assertThat(permission.toString()).isEqualTo("*:READ");
+ }
- context = new ResourcePermission(Resource.CLUSTER, Operation.MANAGE);
- assertEquals(Resource.CLUSTER, context.getResource());
- assertEquals(Operation.MANAGE, context.getOperation());
- assertEquals(ResourcePermission.ALL, context.getTarget());
+ @Test
+ public void invalidResourceOperation() {
+ assertThatThrownBy(() -> new ResourcePermission("invalid", "invalid"))
+ .isInstanceOf(java.lang.IllegalArgumentException.class);
+ }
- // make sure "ALL" in the resource "DATA" means regionName won't be converted to *
- context = new ResourcePermission(Resource.DATA, Operation.READ, "ALL");
- assertEquals(Resource.DATA, context.getResource());
- assertEquals(Operation.READ, context.getOperation());
- assertEquals("ALL", context.getTarget());
+ @Test
+ public void regionNameIsStripped() {
+ ResourcePermission permission = new ResourcePermission("DATA", "READ", "/regionA");
+ assertThat(permission.getResource()).isEqualTo(Resource.DATA);
+ assertThat(permission.getOperation()).isEqualTo(Operation.READ);
+ assertThat(permission.getTarget()).isEqualTo("regionA");
+ assertThat(permission.getKey()).isEqualTo(ResourcePermission.ALL);
+ }
- context = new ResourcePermission(Resource.CLUSTER, Operation.READ, Target.ALL);
- assertEquals(context.getTarget(), ResourcePermission.ALL);
+ @Test
+ public void allImplies() {
+ ResourcePermission permission = ResourcePermissions.ALL;
+ assertThat(permission.implies(new ResourcePermission("DATA", "READ"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("DATA", "WRITE"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("DATA", "MANAGE"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("CLUSTER", "READ"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("CLUSTER", "WRITE"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("CLUSTER", "MANAGE"))).isTrue();
+
+ permission = ResourcePermissions.DATA_ALL;
+ assertThat(permission.implies(new ResourcePermission("DATA", "READ"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("DATA", "WRITE"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("DATA", "MANAGE"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("CLUSTER", "READ"))).isFalse();
+ assertThat(permission.implies(new ResourcePermission("CLUSTER", "WRITE"))).isFalse();
+ assertThat(permission.implies(new ResourcePermission("CLUSTER", "MANAGE"))).isFalse();
+
+ permission = ResourcePermissions.CLUSTER_ALL;
+ assertThat(permission.implies(new ResourcePermission("DATA", "READ"))).isFalse();
+ assertThat(permission.implies(new ResourcePermission("DATA", "WRITE"))).isFalse();
+ assertThat(permission.implies(new ResourcePermission("DATA", "MANAGE"))).isFalse();
+ assertThat(permission.implies(new ResourcePermission("CLUSTER", "READ"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("CLUSTER", "WRITE"))).isTrue();
+ assertThat(permission.implies(new ResourcePermission("CLUSTER", "MANAGE"))).isTrue();
}
@Test
public void testToString() {
ResourcePermission context = new ResourcePermission();
- assertEquals("NULL:NULL", context.toString());
+ assertThat("NULL:NULL").isEqualTo(context.toString());
context = new ResourcePermission(Resource.DATA, Operation.MANAGE);
- assertEquals("DATA:MANAGE", context.toString());
+ assertThat("DATA:MANAGE").isEqualTo(context.toString());
context = new ResourcePermission(Resource.DATA, Operation.MANAGE, "REGIONA");
- assertEquals("DATA:MANAGE:REGIONA", context.toString());
+ assertThat("DATA:MANAGE:REGIONA").isEqualTo(context.toString());
context = new ResourcePermission(Resource.DATA, Operation.MANAGE);
- assertEquals("DATA:MANAGE", context.toString());
+ assertThat("DATA:MANAGE").isEqualTo(context.toString());
}
@Test
- public void testImplies() {
+ public void impliesWithWildCardPermission() {
// If caseSensitive=false, the permission string becomes lower-case, which will cause failures
// when testing implication against our (case sensitive) resources, e.g., DATA
--
To stop receiving notification emails like this one, please contact
['"commits@geode.apache.org" <co...@geode.apache.org>'].