diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java index cdae5a45e398..fe5ea3fbd68b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java @@ -212,8 +212,9 @@ public static void removeUserPermission(Configuration conf, UserPermission userP List remainingActions = new ArrayList<>(); List dropActions = Arrays.asList(userPerm.getPermission().getActions()); for (UserPermission perm : permsList) { - // Find the user and remove only the requested permissions - if (perm.getUser().equals(userPerm.getUser())) { + // Find the exact permission record (same user and scope) and remove only the requested + // permissions + if (userPerm.equalsExceptActions(perm)) { for (Permission.Action oldAction : perm.getPermission().getActions()) { if (!dropActions.contains(oldAction)) { remainingActions.add(oldAction); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController2.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController2.java index 386eb964d2f4..f6974c214e0e 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController2.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController2.java @@ -18,9 +18,11 @@ package org.apache.hadoop.hbase.security.access; import static org.apache.hadoop.hbase.AuthUtil.toGroupEntry; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Arrays; @@ -38,6 +40,7 @@ import org.apache.hadoop.hbase.client.Admin; import org.apache.hadoop.hbase.client.Connection; import org.apache.hadoop.hbase.client.ConnectionFactory; +import org.apache.hadoop.hbase.client.Get; import org.apache.hadoop.hbase.client.Put; import org.apache.hadoop.hbase.client.Result; import org.apache.hadoop.hbase.client.ResultScanner; @@ -478,7 +481,57 @@ public Void run() throws Exception { // Verify user from a group whose column qualifier level access has been revoked can't read the // data having this column family and qualifier. revokeFromTable(TEST_UTIL, TESTGROUP_1_NAME, tableName, TEST_FAMILY, Q1); - verifyDenied(TESTGROUP1_USER1, scanTableActionForGroupWithQualifierLevelAccess); + verifyDenied(TESTGROUP1_USER1, scanQualifierActionForGroupWithQualifierLevelAccess); + } + + @Test + public void testRevokeFamilyActionKeepsTableLevelPermission() throws Throwable { + User user = User.createUserForTesting(conf, "revoke_family_user", new String[0]); + + AccessTestAction putFamilyAction = new AccessTestAction() { + @Override + public Object run() throws Exception { + try (Connection connection = ConnectionFactory.createConnection(conf); + Table table = connection.getTable(tableName)) { + table.put(new Put(TEST_ROW).addColumn(TEST_FAMILY, Q1, value1)); + } + return null; + } + }; + + AccessTestAction getFamilyAction = new AccessTestAction() { + @Override + public Object run() throws Exception { + try (Connection connection = ConnectionFactory.createConnection(conf); + Table table = connection.getTable(tableName)) { + table.get(new Get(TEST_ROW).addColumn(TEST_FAMILY, Q1)); + } + return null; + } + }; + + // Client side grants: table level READ plus family level WRITE on TEST_FAMILY + grantOnTable(TEST_UTIL, user.getShortName(), tableName, null, null, Action.READ); + grantOnTable(TEST_UTIL, user.getShortName(), tableName, TEST_FAMILY, null, Action.WRITE); + verifyAllowed(user, getFamilyAction); + verifyAllowed(user, putFamilyAction); + + // Client side revoke of the family level WRITE only + revokeFromTable(TEST_UTIL, user.getShortName(), tableName, TEST_FAMILY, null, Action.WRITE); + + // Only the table level READ should remain + List userPerms = AccessControlClient.getUserPermissions(systemUserConnection, + tableName.getNameAsString(), user.getShortName()); + assertEquals(1, userPerms.size(), + "family level WRITE should be revoked, only the table level READ should remain"); + TablePermission permission = (TablePermission) userPerms.get(0).getPermission(); + assertNull(permission.getFamily(), "table level permission should be kept untouched"); + assertArrayEquals(new Permission.Action[] { Action.READ }, permission.getActions(), + "table level READ should be kept untouched"); + + // Enforcement: reading still works, writing to the family must now be denied + verifyAllowed(user, getFamilyAction); + verifyDenied(user, putFamilyAction); } public static class MyAccessController extends AccessController { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java index ed8dbe05006f..c42da502aab3 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java @@ -538,8 +538,10 @@ public void testRevokeTable1(TestInfo testInfo) throws Exception { // grant T(R) -> revoke table family TestHDFSAclHelper.grantOnTable(TEST_UTIL, grantUserName, table, READ); - SecureTestUtil.revokeFromTable(TEST_UTIL, grantUserName, table, TestHDFSAclHelper.COLUMN1, null, - READ); + // the revoke below has no effect on the table level permission, so it does not update the acl + // table and must not wait for acl cache changes + admin.revoke(new UserPermission(grantUserName, Permission.newBuilder(table) + .withFamily(TestHDFSAclHelper.COLUMN1).withActions(READ).build())); TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot, 6); // grant T(R) -> revoke T(R) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestTablePermissions.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestTablePermissions.java index ee08e4f1e821..ffdd0f8a5907 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestTablePermissions.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestTablePermissions.java @@ -216,6 +216,79 @@ public void testBasicWrite() throws Exception { assertTrue(actions.contains(Permission.Action.WRITE)); } + @Test + public void testRevokeFamilyPermissionKeepsTableLevelPermission() throws Exception { + Configuration conf = UTIL.getConfiguration(); + try (Connection connection = ConnectionFactory.createConnection(conf)) { + // table level READ for userA + addUserPermission(conf, + new UserPermission("userA", + Permission.newBuilder(TEST_TABLE).withActions(Permission.Action.READ).build()), + connection.getTable(PermissionStorage.ACL_TABLE_NAME)); + // family level WRITE for userA on the same table + addUserPermission(conf, + new UserPermission("userA", + Permission.newBuilder(TEST_TABLE).withFamily(TEST_FAMILY) + .withActions(Permission.Action.WRITE).build()), + connection.getTable(PermissionStorage.ACL_TABLE_NAME)); + } + + // revoke only the family level WRITE + try (Connection connection = ConnectionFactory.createConnection(conf)) { + PermissionStorage.removeUserPermission(conf, + new UserPermission("userA", + Permission.newBuilder(TEST_TABLE).withFamily(TEST_FAMILY) + .withActions(Permission.Action.WRITE).build()), + connection.getTable(PermissionStorage.ACL_TABLE_NAME)); + } + + List userPerms = + PermissionStorage.getTablePermissions(conf, TEST_TABLE).get("userA"); + assertEquals(1, userPerms.size(), + "family level WRITE should be revoked, only the table level READ should remain"); + TablePermission permission = (TablePermission) userPerms.get(0).getPermission(); + assertNull(permission.getFamily(), "table level permission should be kept untouched"); + assertArrayEquals(new Permission.Action[] { Permission.Action.READ }, permission.getActions(), + "table level READ should be kept untouched"); + } + + @Test + public void testRevokeFamilyPermissionKeepsTableLevelActions() throws Exception { + Configuration conf = UTIL.getConfiguration(); + try (Connection connection = ConnectionFactory.createConnection(conf)) { + // table level READ,WRITE for userB + addUserPermission(conf, + new UserPermission("userB", + Permission.newBuilder(TEST_TABLE) + .withActions(Permission.Action.READ, Permission.Action.WRITE).build()), + connection.getTable(PermissionStorage.ACL_TABLE_NAME)); + // family level WRITE for userB on the same table + addUserPermission(conf, + new UserPermission("userB", + Permission.newBuilder(TEST_TABLE).withFamily(TEST_FAMILY) + .withActions(Permission.Action.WRITE).build()), + connection.getTable(PermissionStorage.ACL_TABLE_NAME)); + } + + // revoke only the family level WRITE + try (Connection connection = ConnectionFactory.createConnection(conf)) { + PermissionStorage.removeUserPermission(conf, + new UserPermission("userB", + Permission.newBuilder(TEST_TABLE).withFamily(TEST_FAMILY) + .withActions(Permission.Action.WRITE).build()), + connection.getTable(PermissionStorage.ACL_TABLE_NAME)); + } + + List userPerms = + PermissionStorage.getTablePermissions(conf, TEST_TABLE).get("userB"); + assertEquals(1, userPerms.size(), + "family level WRITE should be revoked, only the table level permission should remain"); + TablePermission permission = (TablePermission) userPerms.get(0).getPermission(); + assertNull(permission.getFamily(), "table level permission should be kept untouched"); + assertArrayEquals(new Permission.Action[] { Permission.Action.READ, Permission.Action.WRITE }, + permission.getActions(), "table level actions should be kept untouched"); + } + @Test public void testPersistence() throws Exception { Configuration conf = UTIL.getConfiguration();