[GitHub] [fluo] keith-turner commented on a diff in pull request #1120: First pass at adding scan time authorizations to transactions/snapshots

[GitBox <gi...@apache.org>]

keith-turner commented on code in PR #1120:
URL: https://github.com/apache/fluo/pull/1120#discussion_r892872914


##########
modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java:
##########
@@ -1571,6 +1579,20 @@ private void beginCommitAsyncTest(CommitData cd) {
   }
 
   public SnapshotScanner newSnapshotScanner(Span span, Collection<Column> columns) {
-    return new SnapshotScanner(env, new SnapshotScanner.Opts(span, columns, false), startTs, stats);
+    return newSnapshotScanner(span, columns, env.getAuthorizations());
+  }
+
+  public SnapshotScanner newSnapshotScanner(Span span, Collection<Column> columns,

Review Comment:
   There is also a parallel snapshots scanner used by this class that is a wrapper around the batch scanner. I am not sure if its gettings the correct auths passed to it.



##########
modules/api/src/main/java/org/apache/fluo/api/client/Snapshot.java:
##########
@@ -29,4 +31,8 @@ public interface Snapshot extends SnapshotBase, AutoCloseable {
    */
   @Override
   void close();
+
+  default Snapshot useScanTimeAuthz(Collection<String> authz) {
+    return this;

Review Comment:
   ```suggestion
     /**
      * Sets the authorizations to use when reading data. This overrides anything that may have been set in the configuration used to create a FluoClient.
      *
      * @since 1.2.0
      */
     default Snapshot useScanTimeAuthorizations(Collection<String> authz) {
       throw new UnsupportedOperationException();
   ```



##########
modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java:
##########
@@ -1571,6 +1579,20 @@ private void beginCommitAsyncTest(CommitData cd) {
   }
 
   public SnapshotScanner newSnapshotScanner(Span span, Collection<Column> columns) {
-    return new SnapshotScanner(env, new SnapshotScanner.Opts(span, columns, false), startTs, stats);
+    return newSnapshotScanner(span, columns, env.getAuthorizations());

Review Comment:
   Do we want to use env.getAuthorizations()? What about auths set on the snapshot?



##########
modules/api/src/main/java/org/apache/fluo/api/client/FluoClient.java:
##########
@@ -75,6 +77,10 @@ public interface FluoClient extends AutoCloseable {
    */
   MetricsReporter getMetricsReporter();
 
+  default FluoClient useAuthorizations(Collection<String> authLabels) {
+    return this;
+  }
+

Review Comment:
   If authorizations can be set in the FluoConfiguration, then this method is probably not needed.  FluoClients are always created from configuration and if the configuration has auths then the client can get them that way.



##########
modules/integration-tests/src/main/java/org/apache/fluo/integration/client/FluoClientIT.java:
##########
@@ -83,4 +97,72 @@ public void testFailures() {
     Logger.getLogger(FluoClientImpl.class).setLevel(clientLevel);
     Logger.getLogger(FluoFactory.class).setLevel(factoryLevel);
   }
+
+  @Test(expected = CommitException.class)
+  public void testWriteWithDefaultAuths() throws Throwable {
+    Column labeledColumn = new Column("data", "private_column", "PRIVATE");
+    try (FluoClient client = FluoFactory.newClient(config);
+        Transaction txn = client.newTransaction()) {
+      txn.set("bill", labeledColumn, "a value");
+      txn.commit();
+    }
+  }
+
+  @Test
+  public void testScanTimeAuths() throws Throwable {
+    try (AccumuloClient accumulo = AccumuloUtil.getClient(config)) {
+      accumulo.securityOperations().changeUserAuthorizations(config.getAccumuloUser(),
+          new Authorizations("PRIVATE", "PUBLIC"));
+    }
+
+    Column ssn = new Column("", "ssn", "PRIVATE");
+    Column name = new Column("", "name", "PUBLIC");
+    try (FluoClientImpl client = (FluoClientImpl) FluoFactory.newClient(config)) {
+      client.useAuthorizations(ImmutableList.of("PRIVATE", "PUBLIC"));

Review Comment:
   In addition to setting auths on client, would be nice to also test setting them on snapshot.  
   
   



##########
modules/integration-tests/src/main/java/org/apache/fluo/integration/client/FluoClientIT.java:
##########
@@ -83,4 +97,72 @@ public void testFailures() {
     Logger.getLogger(FluoClientImpl.class).setLevel(clientLevel);
     Logger.getLogger(FluoFactory.class).setLevel(factoryLevel);
   }
+
+  @Test(expected = CommitException.class)
+  public void testWriteWithDefaultAuths() throws Throwable {
+    Column labeledColumn = new Column("data", "private_column", "PRIVATE");
+    try (FluoClient client = FluoFactory.newClient(config);
+        Transaction txn = client.newTransaction()) {
+      txn.set("bill", labeledColumn, "a value");
+      txn.commit();
+    }
+  }
+
+  @Test
+  public void testScanTimeAuths() throws Throwable {
+    try (AccumuloClient accumulo = AccumuloUtil.getClient(config)) {
+      accumulo.securityOperations().changeUserAuthorizations(config.getAccumuloUser(),
+          new Authorizations("PRIVATE", "PUBLIC"));
+    }
+
+    Column ssn = new Column("", "ssn", "PRIVATE");
+    Column name = new Column("", "name", "PUBLIC");
+    try (FluoClientImpl client = (FluoClientImpl) FluoFactory.newClient(config)) {
+      client.useAuthorizations(ImmutableList.of("PRIVATE", "PUBLIC"));
+      try (Transaction txn = client.newTransaction()) {
+        txn.set("bill", ssn, "1");
+        txn.set("bill", name, "william");
+        txn.set("bob", ssn, "2");
+        txn.set("bob", name, "robert");
+        txn.commit();
+      }
+
+      System.out.println("***********************************");
+      System.out.println("Default auths");
+      System.out.println("***********************************");
+      try (Snapshot snapshot = client.newSnapshot()) {
+        System.out.println(snapshot.get(Bytes.of("bill")));

Review Comment:
   snapshot has multiple get methods, some use scanner and some use batch scanner.  Would be nice to test them all.



##########
modules/api/src/main/java/org/apache/fluo/api/config/FluoConfiguration.java:
##########
@@ -510,6 +514,19 @@ public String getAccumuloPassword() {
     throw new NoSuchElementException(ACCUMULO_PASSWORD_PROP + " is not set!");
   }
 
+  public FluoConfiguration setAccumuloAuthorizations(String authsCsv) {
+    setProperty(ACCUMULO_AUTH_PROP, authsCsv);
+    return this;
+  }
+
+  public String getAccumuloAuthorizations() {

Review Comment:
   This could return a list.
   
   ```suggestion
     /**
       *  Returns the authorizations configured for reading data or an empty list if none are configured.
       *
       * @since 1.2.0
       */
     public List<String> getAccumuloAuthorizations() {
   ```



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

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