[
https://issues.apache.org/jira/browse/GEODE-4310?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16330705#comment-16330705
]
ASF GitHub Bot commented on GEODE-4310:
---------------------------------------
jinmeiliao closed pull request #1300: GEODE-4310: allow ResourcePermission to
take Strings as arguments for…
URL: https://github.com/apache/geode/pull/1300
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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 a0db6624f7..5565a460b9 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.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 7cd77634db..74db186fc4 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 @@
public class ResourcePermission extends WildcardPermission {
public static String ALL = "*";
+ public static String NULL = "NULL";
/**
* @deprecated use ALL
@@ -40,81 +43,81 @@
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 ResourcePermission(Resource resource, Operation
operation, String target,
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 String getKey() {
@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 48f93d8274..4b15407ffe 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 @@
@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 void testIsPermission() {
@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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> improve ResourcePermission to take strings as argument for resource and
> operation
> ---------------------------------------------------------------------------------
>
> Key: GEODE-4310
> URL: https://issues.apache.org/jira/browse/GEODE-4310
> Project: Geode
> Issue Type: Improvement
> Components: security
> Reporter: Jinmei Liao
> Priority: Major
> Labels: pull-request-available
>
> would be nice to be able to create ResourcePermission("*", "*") that would
> mean that this operation requires all permissions
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
