From 61274494ba64c1961daccc0b49168142f460c900 Mon Sep 17 00:00:00 2001 From: Ashish Singhi Date: Tue, 25 Jul 2017 20:23:07 +0530 Subject: [PATCH] HBASE-18437 Revoke access permissions of a user from a table does not work as expected --- .../hadoop/hbase/security/access/Permission.java | 6 ++ .../hbase/security/access/AccessControlLists.java | 35 +++++++-- .../security/access/TestAccessController.java | 90 ++++++++++++++++------ 3 files changed, 102 insertions(+), 29 deletions(-) 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 8476f61..ed5e8fc 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 @@ -98,6 +98,12 @@ public class Permission extends VersionedWritable { return actions; } + public void setActions(Action[] assigned) { + if (assigned != null && assigned.length > 0) { + actions = Arrays.copyOf(assigned, assigned.length); + } + } + public boolean implies(Action action) { if (this.actions != null) { for (Action a : this.actions) { 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 97ac1de..4c7ae10 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 @@ -226,13 +226,38 @@ 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 { + List permsList = getUserPermissions(conf, userPermissionRowKey(userPerm)); + List leftActions = new ArrayList<>(); + List dropActions = Arrays.asList(userPerm.getActions()); + for (UserPermission perm : permsList) { + if (Bytes.toString(perm.getUser()).equals(Bytes.toString(userPerm.getUser()))) { + for (Permission.Action oldAction : perm.getActions()) { + if (!dropActions.contains(oldAction)) { + leftActions.add(oldAction); + } + } + if (!leftActions.isEmpty()) { + perm.setActions(leftActions.toArray(new Permission.Action[leftActions.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)); try { t.delete(d); } finally { 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 67227a9..7cb5d85 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 @@ -18,6 +18,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; @@ -2387,20 +2388,22 @@ public class TestAccessController extends SecureTestUtil { 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) @@ -2554,20 +2557,21 @@ public class TestAccessController extends SecureTestUtil { 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); } @@ -3174,4 +3178,42 @@ public class TestAccessController extends SecureTestUtil { verifyAllowed(regionLockHeartbeatAction, SUPERUSER, USER_ADMIN, namespaceUser, tableACUser); verifyDenied(regionLockHeartbeatAction, globalRWXUser, tableRWXUser); } + + @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 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); + } + } } -- 2.9.0.windows.1