From 6aba917e9968636f3d91592056ef15bab560c504 Mon Sep 17 00:00:00 2001 From: Duo Zhang Date: Tue, 10 Nov 2026 14:04:29 +0800 Subject: [PATCH] HBASE-30532 Revoke does not work as expected when a user has permissions on multiple families on the same table (#8862) Also fix TestSnapshotScannerHDFSAclController.testRevokeTable1 to not wait for acl cache changes on a no-op revoke The test grants table level READ and then revokes a family level READ. The revoke matches no stored permission record and is a no-op on the _acl_ table, so SecureTestUtil.revokeFromTable's updateACLs waits in vain for AuthManager mtime changes and times out. Use admin.revoke directly, like the second phase of the test already does. (cherry picked from commit 49f4cdf2617a62abdd0ffcf684ecae06d7895261) --- .../security/access/PermissionStorage.java | 5 +- .../access/TestAccessController2.java | 55 +++++++++++++- .../TestSnapshotScannerHDFSAclController.java | 6 +- .../security/access/TestTablePermissions.java | 73 +++++++++++++++++++ 4 files changed, 134 insertions(+), 5 deletions(-) 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();