HBASE-18437 Revoke access permissions of a user from a table does not work as expected
Signed-off-by: Andrew Purtell <apurt...@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java Project: http://git-wip-us.apache.org/repos/asf/hbase/repo Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/5b27f625 Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/5b27f625 Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/5b27f625 Branch: refs/heads/branch-1.4 Commit: 5b27f6253ad7e561ee4a3b3491ec647be7c726b0 Parents: 1220124 Author: Ashish Singhi <ashishsin...@apache.org> Authored: Fri Aug 11 12:48:32 2017 +0530 Committer: Andrew Purtell <apurt...@apache.org> Committed: Tue Aug 15 19:00:44 2017 -0700 ---------------------------------------------------------------------- .../hbase/security/access/Permission.java | 6 ++ .../security/access/AccessControlLists.java | 53 +++++++++-- .../security/access/TestAccessController.java | 96 ++++++++++++++------ 3 files changed, 118 insertions(+), 37 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hbase/blob/5b27f625/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java ---------------------------------------------------------------------- diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java index f4538a6..3a01ace 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java @@ -112,6 +112,12 @@ public class Permission extends VersionedWritable { return false; } + public void setActions(Action[] assigned) { + if (assigned != null && assigned.length > 0) { + actions = Arrays.copyOf(assigned, assigned.length); + } + } + @Override public boolean equals(Object obj) { if (!(obj instanceof Permission)) { http://git-wip-us.apache.org/repos/asf/hbase/blob/5b27f625/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java index 7197526..a2ac927 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java @@ -185,10 +185,18 @@ public class AccessControlLists { LOG.debug("Writing permission with rowKey " + Bytes.toString(rowKey) + " " + Bytes.toString(key) + ": " + Bytes.toStringBinary(value)); } - try { - t.put(p); - } finally { - t.close(); + if (t == null) { + try (Connection connection = ConnectionFactory.createConnection(conf)) { + try (Table table = connection.getTable(ACL_TABLE_NAME)) { + table.put(p); + } + } + } else { + try { + t.put(p); + } finally { + t.close(); + } } } @@ -220,13 +228,40 @@ public class AccessControlLists { */ static void removeUserPermission(Configuration conf, UserPermission userPerm, Table t) throws IOException { - Delete d = new Delete(userPermissionRowKey(userPerm)); - byte[] key = userPermissionKey(userPerm); - + if (null == userPerm.getActions()) { + removePermissionRecord(conf, userPerm, t); + } else { + // Get all the global user permissions from the acl table + List<UserPermission> permsList = getUserPermissions(conf, userPermissionRowKey(userPerm)); + List<Permission.Action> remainingActions = new ArrayList<>(); + List<Permission.Action> dropActions = Arrays.asList(userPerm.getActions()); + for (UserPermission perm : permsList) { + // Find the user and remove only the requested permissions + if (Bytes.toString(perm.getUser()).equals(Bytes.toString(userPerm.getUser()))) { + for (Permission.Action oldAction : perm.getActions()) { + if (!dropActions.contains(oldAction)) { + remainingActions.add(oldAction); + } + } + if (!remainingActions.isEmpty()) { + perm.setActions(remainingActions.toArray(new Permission.Action[remainingActions.size()])); + addUserPermission(conf, perm, t); + } else { + removePermissionRecord(conf, userPerm, t); + } + break; + } + } + } if (LOG.isDebugEnabled()) { - LOG.debug("Removing permission "+ userPerm.toString()); + LOG.debug("Removed permission "+ userPerm.toString()); } - d.addColumns(ACL_LIST_FAMILY, key); + } + + private static void removePermissionRecord(Configuration conf, UserPermission userPerm, Table t) + throws IOException { + Delete d = new Delete(userPermissionRowKey(userPerm)); + d.addColumns(ACL_LIST_FAMILY, userPermissionKey(userPerm)); if (t == null) { try (Connection connection = ConnectionFactory.createConnection(conf)) { try (Table table = connection.getTable(ACL_TABLE_NAME)) { http://git-wip-us.apache.org/repos/asf/hbase/blob/5b27f625/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java index 51aeff8..c5c0470 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java @@ -19,6 +19,7 @@ package org.apache.hadoop.hbase.security.access; import static org.apache.hadoop.hbase.AuthUtil.toGroupEntry; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -2380,28 +2381,30 @@ public class TestAccessController extends SecureTestUtil { // Grant table READ permissions to testGlobalGrantRevoke. String userName = testGlobalGrantRevoke.getShortName(); try { - grantGlobalUsingAccessControlClient(TEST_UTIL, systemUserConnection, - userName, Permission.Action.READ); + grantGlobalUsingAccessControlClient(TEST_UTIL, systemUserConnection, userName, + Permission.Action.READ); } catch (Throwable e) { LOG.error("error during call of AccessControlClient.grant. ", e); } try { // Now testGlobalGrantRevoke should be able to read also verifyAllowed(getAction, testGlobalGrantRevoke); - - // Revoke table READ permission to testGlobalGrantRevoke. - try { - revokeGlobalUsingAccessControlClient(TEST_UTIL, systemUserConnection, - userName, Permission.Action.READ); - } catch (Throwable e) { - LOG.error("error during call of AccessControlClient.revoke ", e); - } - - // Now testGlobalGrantRevoke shouldn't be able read - verifyDenied(getAction, testGlobalGrantRevoke); - } finally { + } catch (Exception e) { revokeGlobal(TEST_UTIL, userName, Permission.Action.READ); + throw e; } + + // Revoke table READ permission to testGlobalGrantRevoke. + try { + revokeGlobalUsingAccessControlClient(TEST_UTIL, systemUserConnection, userName, + Permission.Action.READ); + } catch (Throwable e) { + LOG.error("error during call of AccessControlClient.revoke ", e); + } + + // Now testGlobalGrantRevoke shouldn't be able read + verifyDenied(getAction, testGlobalGrantRevoke); + } @Test(timeout = 180000) @@ -2546,28 +2549,29 @@ public class TestAccessController extends SecureTestUtil { String namespace = TEST_TABLE.getNamespaceAsString(); // Grant namespace READ to testNS, this should supersede any table permissions try { - grantOnNamespaceUsingAccessControlClient(TEST_UTIL, systemUserConnection, userName, - namespace, Permission.Action.READ); + grantOnNamespaceUsingAccessControlClient(TEST_UTIL, systemUserConnection, userName, namespace, + Permission.Action.READ); } catch (Throwable e) { LOG.error("error during call of AccessControlClient.grant. ", e); } try { // Now testNS should be able to read also verifyAllowed(getAction, testNS); - - // Revoke namespace READ to testNS, this should supersede any table permissions - try { - revokeFromNamespaceUsingAccessControlClient(TEST_UTIL, systemUserConnection, userName, - namespace, Permission.Action.READ); - } catch (Throwable e) { - LOG.error("error during call of AccessControlClient.revoke ", e); - } - - // Now testNS shouldn't be able read - verifyDenied(getAction, testNS); - } finally { + } catch (Exception e) { revokeFromNamespace(TEST_UTIL, userName, namespace, Permission.Action.READ); + throw e; + } + + // Revoke namespace READ to testNS, this should supersede any table permissions + try { + revokeFromNamespaceUsingAccessControlClient(TEST_UTIL, systemUserConnection, userName, + namespace, Permission.Action.READ); + } catch (Throwable e) { + LOG.error("error during call of AccessControlClient.revoke ", e); } + + // Now testNS shouldn't be able read + verifyDenied(getAction, testNS); } @@ -2925,4 +2929,40 @@ public class TestAccessController extends SecureTestUtil { verifyDenied(replicateLogEntriesAction, USER_CREATE, USER_RW, USER_RO, USER_NONE, USER_OWNER, USER_GROUP_READ, USER_GROUP_ADMIN, USER_GROUP_CREATE); } + + @Test + public void testAccessControlRevokeOnlyFewPermission() throws Throwable { + TableName tname = TableName.valueOf("revoke"); + try { + TEST_UTIL.createTable(tname, TEST_FAMILY); + User testUserPerms = User.createUserForTesting(conf, "revokePerms", new String[0]); + Permission.Action[] actions = { Action.READ, Action.WRITE }; + AccessControlClient.grant(TEST_UTIL.getConnection(), tname, testUserPerms.getShortName(), + null, null, actions); + + List<UserPermission> userPermissions = AccessControlClient + .getUserPermissions(TEST_UTIL.getConnection(), tname.getNameAsString()); + assertEquals(2, userPermissions.size()); + + AccessControlClient.revoke(TEST_UTIL.getConnection(), tname, testUserPerms.getShortName(), + null, null, Action.WRITE); + + userPermissions = AccessControlClient.getUserPermissions(TEST_UTIL.getConnection(), + tname.getNameAsString()); + assertEquals(2, userPermissions.size()); + + Permission.Action[] expectedAction = { Action.READ }; + boolean userFound = false; + for (UserPermission p : userPermissions) { + if (testUserPerms.getShortName().equals(Bytes.toString(p.getUser()))) { + assertArrayEquals(expectedAction, p.getActions()); + userFound = true; + break; + } + } + assertTrue(userFound); + } finally { + TEST_UTIL.deleteTable(tname); + } + } }