Re: [PR] Modified VisibilityFilter to handle multiple Authorizations objects [accumulo]

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #6402:
URL: https://github.com/apache/accumulo/pull/6402#discussion_r3555031299


##########
core/src/main/java/org/apache/accumulo/core/iterators/user/VisibilityFilter.java:
##########
@@ -136,14 +157,25 @@ public IteratorOptions describeOptions() {
     io.addNamedOption(FILTER_INVALID_ONLY,
         "if 'true', the iterator is instructed to ignore the authorizations and"
             + " only filter invalid visibility labels (default: false)");
-    io.addNamedOption(AUTHS,
-        "the serialized set of authorizations to filter against (default: empty"
-            + " string, accepts only entries visible by all)");

Review Comment:
   Can't remove this option. The configuration parameter is the main API for this class. Removing or renaming it breaks it.



##########
core/src/main/java/org/apache/accumulo/core/iterators/user/VisibilityFilter.java:
##########
@@ -136,14 +157,25 @@ public IteratorOptions describeOptions() {
     io.addNamedOption(FILTER_INVALID_ONLY,
         "if 'true', the iterator is instructed to ignore the authorizations and"
             + " only filter invalid visibility labels (default: false)");
-    io.addNamedOption(AUTHS,
-        "the serialized set of authorizations to filter against (default: empty"
-            + " string, accepts only entries visible by all)");
+    io.addNamedOption(NUM_AUTHS,
+        "The number of serialized authorizations to filter against (default 0)");
+    io.addUnnamedOption(AUTH_PREFIX
+        + "N, where the value is a serialized set of authorizations. N must be between zero and NUM_AUTHS.");

Review Comment:
   I'm not sure these descriptions really communicate how these are used. Is the idea that you specify some config like:
   
   ```
   numAuths = 3
   auth_0 = a,b,c
   auth_1 = c,b,d
   auth_2 = a,c,z
   ```
   
   And then it does something like:
   
   ```
   if (entry.evaluateUsing(auth_0) && entry..evaluateUsing(auth_1) && entry..evaluateUsing(auth_2)) {
     return entry;
   }
   ```
   
   Is the idea to replace multiple executions of the iterator with a single one?
   
   Instead, I think it might make sense to have a config that preserves the existing behavior, and extends the existing config:
   
   ```
   auths = a,b,c;c,b,d;a,c,z
   ```
   
   This is simpler, and backwards compatible. The description can be extended to say: "if multiple serialized sets are configured, separated by semi-colons, then entries are accepted only if they would be accepted by each of the sets independently"
   



##########
core/src/main/java/org/apache/accumulo/core/clientImpl/access/BytesAccess.java:
##########
@@ -93,6 +95,19 @@ public boolean canAccess(byte[] expression) {
     }
   }
 
+  public static BytesEvaluator newEvaluator(Collection<Authorizations> authsSet) {
+    Collection<Set<String>> convertedAuths = new ArrayList<>();
+    for (Authorizations auths : authsSet) {
+      List<byte[]> bytesAuths = auths.getAuthorizations();
+      Set<String> stringAuths = new HashSet<>(bytesAuths.size());
+      for (var auth : bytesAuths) {
+        stringAuths.add(new String(auth, ISO_8859_1));

Review Comment:
   A comment is warranted here to explain why ISO-8859-1 is used.



-- 
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-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org