HBASE-30532 Revoke does not work as expected when a user has permissi…

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<Permission.Action> remainingActions = new ArrayList<>();
       List<Permission.Action> 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);

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

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)

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<UserPermission> 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<UserPermission> 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();