Re: [PR] improves ComprehensiveIT and adjust sunny ITs [accumulo]

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

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


##########
test/src/main/java/org/apache/accumulo/test/ComprehensiveIT.java:
##########
@@ -417,6 +426,299 @@ public void testOverwriteAndBatchDeleter() throws Exception {
     }
   }
 
+  @Test
+  public void invalidInstanceName() {
+    try (var client = Accumulo.newClient().to("fake_instance_name", getCluster().getZooKeepers())
+        .as(getAdminPrincipal(), getToken()).build()) {
+      assertThrows(RuntimeException.class, () -> client.instanceOperations().getTabletServers());
+    }
+  }
+
+  @Test
+  public void testMultiTableWrite() throws Exception {
+    String[] tables = getUniqueNames(2);
+    String table1 = tables[0];
+    String table2 = tables[1];
+
+    try (var client = Accumulo.newClient().from(getClientProps()).build()) {
+      client.tableOperations().create(table1);
+      client.tableOperations().create(table2);
+
+      try (var writer = client.createMultiTableBatchWriter()) {
+        writer.getBatchWriter(table1).addMutations(generateMutations(0, 100, tr -> true));
+        writer.getBatchWriter(table2).addMutations(generateMutations(100, 200, tr -> true));
+        writer.getBatchWriter(table1).addMutations(generateMutations(200, 300, tr -> true));
+        writer.getBatchWriter(table2).addMutations(generateMutations(300, 400, tr -> true));
+      }
+
+      TreeMap<Key,Value> expected1 = new TreeMap<>();
+      expected1.putAll(generateKeys(0, 100));
+      expected1.putAll(generateKeys(200, 300));
+
+      TreeMap<Key,Value> expected2 = new TreeMap<>();
+      expected2.putAll(generateKeys(100, 200));
+      expected2.putAll(generateKeys(300, 400));
+
+      verifyData(client, table1, AUTHORIZATIONS, expected1);
+      verifyData(client, table2, AUTHORIZATIONS, expected2);
+
+      try (var writer = client.createMultiTableBatchWriter()) {
+        writer.getBatchWriter(table1)
+            .addMutations(generateMutations(0, 100, 0x12345678, tr -> true));
+        writer.getBatchWriter(table2)
+            .addMutations(generateMutations(100, 200, 0x12345678, tr -> true));
+        writer.getBatchWriter(table1)
+            .addMutations(generateMutations(200, 300, 0x12345678, tr -> true));
+        writer.getBatchWriter(table2)
+            .addMutations(generateMutations(300, 400, 0x12345678, tr -> true));
+      }
+
+      expected1.putAll(generateKeys(0, 100, 0x12345678, tr -> true));
+      expected1.putAll(generateKeys(200, 300, 0x12345678, tr -> true));
+      expected2.putAll(generateKeys(100, 200, 0x12345678, tr -> true));
+      expected2.putAll(generateKeys(300, 400, 0x12345678, tr -> true));
+
+      verifyData(client, table1, AUTHORIZATIONS, expected1);
+      verifyData(client, table2, AUTHORIZATIONS, expected2);
+    }
+  }
+
+  @Test
+  public void testSecurity() throws Exception {
+    String[] tables = getUniqueNames(2);
+    String rootsTable = tables[0];
+    String usersTable = tables[0];
+
+    try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) {
+
+      client.tableOperations().create(rootsTable);
+      write(client, rootsTable, generateMutations(0, 100, tr -> true));
+      verifyData(client, rootsTable, AUTHORIZATIONS, generateKeys(0, 100));
+
+      var password = new PasswordToken("bestpass1234");
+      client.securityOperations().createLocalUser("user1", password);
+
+      try (var userClient =
+          Accumulo.newClient().from(getClientProps()).as("user1", password).build()) {
+
+        // user1 should not be able to read table
+        var ise = assertThrows(IllegalStateException.class,
+            () -> verifyData(userClient, rootsTable, AUTHORIZATIONS, generateKeys(0, 100)));
+        assertEquals(SecurityErrorCode.PERMISSION_DENIED,
+            ((AccumuloSecurityException) ise.getCause()).getSecurityErrorCode());
+
+        // user1 should not be able to grant theirself read access
+        var ase = assertThrows(AccumuloSecurityException.class, () -> userClient
+            .securityOperations().grantTablePermission("user1", rootsTable, TablePermission.READ));
+        assertEquals(SecurityErrorCode.PERMISSION_DENIED, ase.getSecurityErrorCode());
+
+        // grant user1 read access
+        client.securityOperations().grantTablePermission("user1", rootsTable, TablePermission.READ);
+
+        // security changes take time to propagate, should eventually be able to scan table
+        Wait.waitFor(() -> {
+          try {
+            verifyData(userClient, rootsTable, Authorizations.EMPTY,
+                generateKeys(0, 100, tr -> tr.vis.isEmpty()));
+            return true;
+          } catch (IllegalStateException e) {
+            return false;
+          }
+        });
+        verifyData(userClient, rootsTable, Authorizations.EMPTY,
+            generateKeys(0, 100, tr -> tr.vis.isEmpty()));
+
+        // should not be able to scan with authorizations the user does not have
+        ise = assertThrows(IllegalStateException.class,
+            () -> verifyData(userClient, rootsTable, AUTHORIZATIONS, generateKeys(0, 100)));
+        assertEquals(SecurityErrorCode.BAD_AUTHORIZATIONS,
+            ((AccumuloSecurityException) ise.getCause()).getSecurityErrorCode());
+
+        // scan w/o setting auths, should only return data w/o auths
+        try (Scanner scanner = userClient.createScanner(rootsTable)) {
+          assertEquals(generateKeys(0, 100, tr -> tr.vis.isEmpty()), scan(scanner));
+        }
+
+        // user should not have permission to write to table
+        var mre = assertThrows(MutationsRejectedException.class,
+            () -> write(userClient, rootsTable, generateMutations(100, 200, tr -> true)));
+        assertEquals(SecurityErrorCode.PERMISSION_DENIED, mre.getSecurityErrorCodes().values()
+            .stream().flatMap(Set::stream).collect(MoreCollectors.onlyElement()));
+        // ensure no new data was written
+        assertEquals(new Text(row(99)), client.tableOperations().getMaxRow(rootsTable,
+            AUTHORIZATIONS, new Text(row(98)), true, new Text(row(110)), true));
+
+        client.securityOperations().grantTablePermission("user1", rootsTable,
+            TablePermission.WRITE);
+        // security changes take time to propagate, should eventually be able to write
+        Wait.waitFor(() -> {
+          try {
+            write(userClient, rootsTable, generateMutations(100, 200, tr -> true));
+            return true;
+          } catch (MutationsRejectedException e) {
+            return false;
+          }
+        });
+
+        // ensure newly written data is visible
+        verifyData(client, rootsTable, AUTHORIZATIONS, generateKeys(0, 200));
+
+        // allow user to write to table and verify can write
+        client.securityOperations().changeUserAuthorizations("user1", AUTHORIZATIONS);
+        Wait.waitFor(() -> {
+          try {
+            verifyData(userClient, rootsTable, AUTHORIZATIONS, generateKeys(0, 200));
+            return true;
+          } catch (IllegalStateException e) {
+            return false;
+          }
+        });
+        verifyData(userClient, rootsTable, AUTHORIZATIONS, generateKeys(0, 200));
+        // should scan with all of users granted auths since no auths were specified
+        try (Scanner scanner = userClient.createScanner(rootsTable)) {
+          assertEquals(generateKeys(0, 200), scan(scanner));
+        }
+
+        var splits = new TreeSet<>(List.of(new Text(row(50))));
+
+        // should not have permission to alter the table
+        ase = assertThrows(AccumuloSecurityException.class,
+            () -> userClient.tableOperations().addSplits(rootsTable, splits));
+        assertEquals(SecurityErrorCode.PERMISSION_DENIED, ase.getSecurityErrorCode());
+        // ensure no splits were added
+        assertEquals(Set.of(), Set.copyOf(client.tableOperations().listSplits(rootsTable)));
+
+        client.securityOperations().grantTablePermission("user1", rootsTable,
+            TablePermission.ALTER_TABLE);
+        Wait.waitFor(() -> {
+          try {
+            userClient.tableOperations().addSplits(rootsTable, splits);
+            return true;
+          } catch (AccumuloSecurityException e) {
+            return false;
+          }
+        });
+        assertEquals(splits, Set.copyOf(userClient.tableOperations().listSplits(rootsTable)));
+
+        // user should not have permission to bulk import
+        ase = assertThrows(AccumuloSecurityException.class, () -> bulkImport(userClient, rootsTable,
+            List.of(generateKeys(200, 250, tr -> true), generateKeys(250, 300, tr -> true))));
+        assertEquals(SecurityErrorCode.PERMISSION_DENIED, ase.getSecurityErrorCode());
+        verifyData(userClient, rootsTable, AUTHORIZATIONS, generateKeys(0, 200));
+
+        // TODO open a bug about this, had to add this permission to get bulk import to work
+        client.securityOperations().grantSystemPermission("user1", SystemPermission.SYSTEM);

Review Comment:
   That seems strange to me. Might be a bug. Did you ever create a follow-on issue about this?



-- 
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