ctubbsii commented on code in PR #5900:
URL: https://github.com/apache/accumulo/pull/5900#discussion_r2364554332
##########
core/src/test/java/org/apache/accumulo/core/security/NamespacePermissionsTest.java:
##########
@@ -31,18 +32,30 @@ public void testEnsureEquivalencies() {
EnumSet<NamespacePermission> set =
EnumSet.allOf(NamespacePermission.class);
for (TablePermission permission : TablePermission.values()) {
- set.remove(NamespacePermission.getEquivalent(permission));
+ var equivalent = NamespacePermission.getEquivalent(permission);
+ if (equivalent != null) {
+ assertTrue(set.remove(equivalent));
+ assertEquals(permission.name(), equivalent.name());
+ }
}
+ assertEquals(EnumSet.of(NamespacePermission.ALTER_NAMESPACE,
NamespacePermission.DROP_NAMESPACE,
Review Comment:
A comment to explain that these do not have an equivalent would be good.
##########
core/src/test/java/org/apache/accumulo/core/security/NamespacePermissionsTest.java:
##########
@@ -31,18 +32,30 @@ public void testEnsureEquivalencies() {
EnumSet<NamespacePermission> set =
EnumSet.allOf(NamespacePermission.class);
for (TablePermission permission : TablePermission.values()) {
- set.remove(NamespacePermission.getEquivalent(permission));
+ var equivalent = NamespacePermission.getEquivalent(permission);
+ if (equivalent != null) {
+ assertTrue(set.remove(equivalent));
+ assertEquals(permission.name(), equivalent.name());
+ }
}
+ assertEquals(EnumSet.of(NamespacePermission.ALTER_NAMESPACE,
NamespacePermission.DROP_NAMESPACE,
+ NamespacePermission.CREATE_TABLE), set);
+
+ set = EnumSet.allOf(NamespacePermission.class);
for (SystemPermission permission : SystemPermission.values()) {
if (permission == SystemPermission.GRANT) {
assertThrows(IllegalArgumentException.class,
() -> NamespacePermission.getEquivalent(permission), "GRANT has no
equivalent");
} else {
- set.remove(NamespacePermission.getEquivalent(permission));
+ var equivalent = NamespacePermission.getEquivalent(permission);
+ if (equivalent != null) {
+ assertTrue(set.remove(equivalent));
+ assertEquals(permission.name(), equivalent.name());
+ }
+
}
}
-
- assertTrue(set.isEmpty(),
- "All namespace permissions should have equivalent table or system
permissions.");
+ assertEquals(EnumSet.of(NamespacePermission.READ,
NamespacePermission.WRITE,
Review Comment:
A comment to explain that these do not have an equivalent would be good.
##########
core/src/test/java/org/apache/accumulo/core/security/NamespacePermissionsTest.java:
##########
@@ -31,18 +32,30 @@ public void testEnsureEquivalencies() {
EnumSet<NamespacePermission> set =
EnumSet.allOf(NamespacePermission.class);
for (TablePermission permission : TablePermission.values()) {
- set.remove(NamespacePermission.getEquivalent(permission));
+ var equivalent = NamespacePermission.getEquivalent(permission);
+ if (equivalent != null) {
+ assertTrue(set.remove(equivalent));
+ assertEquals(permission.name(), equivalent.name());
Review Comment:
This might hold coincidentally, but I don't think it's necessarily true.
Part of the reason for this method was that there may not actually be a direct
equivalent for something with the same name. For example, consider "ALTER"...
the ability to alter the nature of a namespace is not the same as being able to
alter the tables within that namespace, although one might imply the other.
More likely, though, `NamespacePermission.WRITE` could hypothetically be used
to imply `TablePermission.ALTER`. In this case, though, we explicitly have
`ALTER_NAMESPACE` and `ALTER_TABLE` to distinguish between these. So, my
example isn't perfect.
I think this is probably okay to assert here, but it raises the question of
why we're bothering to look up the equivalency, rather than just use the other
permission type directly. I suppose that question can be answered in the
future, with additional improvements. This is fine for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
