Repository navigation
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
6 open issues
userPerm.getPermission().getActions()… · NewparseUserPermission(...)can return null whenactionCodesis null/empty (see method body), but… · NewloadAll(Connection):parseUserPermission(...)may return null… · NewPermissionStorage.getPermissions(Connection, entry)opens and closes the ACLTableinternally… · New
What changed in this PR
Refactors ACL permission reading/writing and filtering to use Connection-based APIs and moves “filterUser” logic out of PermissionStorage into a dedicated helper (PermissionUtils), updating related components and tests accordingly.
Changes:
- Replace
PermissionStorageAPIs that acceptedConfiguration/TablewithConnection-based methods returningList<UserPermission>. - Introduce
PermissionUtils.filter(...)for user/group + CF/CQ filtering, and update RPC/controller codepaths to use it. - Update multiple security/access tests to use the new APIs and adjust assertions/permission map building.
| File | Description |
|---|---|
| hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestWithDisabledAuthorization.java | Switches permission reads to Connection-based PermissionStorage APIs. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestTablePermissions.java | Updates tests to new add/remove/get permission APIs and rebuilds multimaps locally from returned lists. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestScanEarlyTermination.java | Uses Connection-based permission lookup in teardown. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestNamespaceCommands.java | Adds helper to build multimap from getNamespacePermissions(Connection, ...). |
| hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java | Uses Connection-based table permission lookup in teardown. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java | Uses Connection-based table permission lookup in teardown. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController3.java | Updates permission assertions to pass Connection into PermissionStorage. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController2.java | Updates permission assertions and replaces multimap lookups with list filtering. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java | Adds helper to rebuild multimap from list results; updates namespace permission call argument conversion. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsWithACL.java | Updates permission assertions to use Connection-based APIs. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java | Switches snapshot ACL extraction to Connection-based permission retrieval. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java | Stores Connection and updates permission reads to use it; simplifies read-action extraction to lists. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclController.java | Reworks permission lookups to use PermissionUtils.filter + new PermissionStorage list-based APIs; adjusts namespace entry encoding usage. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionUtils.java | New helper implementing filtering by user/group and optional CF/CQ. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/PermissionStorage.java | Major refactor: Connection-based read/write APIs, internal parsing helpers, and restricted test-only loadAll(Connection). |
| hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java | Updates ACL-to-ZK sync and grant/revoke hooks to use Connection-based PermissionStorage methods. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java | Updates grant/revoke/getUserPermissions logic to use new list-based APIs + PermissionUtils.filter. |
Review effort: Lite
Comment on lines
+229
to
249
| public static void addUserPermission(Connection conn, UserPermission userPerm, | ||
| boolean mergeExistingPermissions) throws IOException { | ||
| try (Table aclTable = conn.getTable(ACL_TABLE_NAME)) { | ||
| UserPermission toAdd; | ||
| if (mergeExistingPermissions) { | ||
| UserPermission oldUserPerm = getUserPermission(aclTable, userPerm); | ||
| if (oldUserPerm == null || ArrayUtils.isEmpty(oldUserPerm.getPermission().getActions())) { | ||
| toAdd = userPerm; | ||
| } else { | ||
| Set<Permission.Action> actionSet = new TreeSet<>(); | ||
| actionSet.addAll(Arrays.asList(oldUserPerm.getPermission().getActions())); | ||
| actionSet.addAll(Arrays.asList(userPerm.getPermission().getActions())); | ||
| oldUserPerm.getPermission().setActions(actionSet.toArray(Permission.Action[]::new)); | ||
| toAdd = oldUserPerm; | ||
| } | ||
| } else { | ||
| toAdd = userPerm; | ||
| } | ||
| put(aclTable, toAdd); | ||
| } | ||
| } |
| ListMultimap<String, UserPermission> perms = ArrayListMultimap.create(); | ||
| result.getFamilyMap(ACL_LIST_FAMILY).forEach((qual, actionCodes) -> { | ||
| UserPermission userPerm = parseUserPermission(entryName, qual, actionCodes); | ||
| perms.put(userPerm.getUser(), userPerm); |
| } | ||
| if (!result.isEmpty()) { | ||
| result.getFamilyMap(ACL_LIST_FAMILY).forEach((qual, actionCodes) -> { | ||
| UserPermission userPerms = parseUserPermission(entryName, qual, actionCodes); |
Comment on lines
+75
to
+85
| if (username != null) { | ||
| MutableObject<Set<String>> userGroupsHolder = new MutableObject<>(); | ||
| stream = stream.filter(up -> { | ||
| String u = up.getUser(); | ||
| if (u.charAt(0) == '@') { | ||
| // this is a group, check whether the given user belongs to this group | ||
| return getUserGroups(userGroupsHolder, username).contains(u.substring(1)); | ||
| } | ||
| return u.equals(username); | ||
| }); | ||
| } |
Comment on lines
+282
to
289
| try { | ||
| for (byte[] entry : entries) { | ||
| currentEntry = entry; | ||
| ListMultimap<String, UserPermission> perms = | ||
| PermissionStorage.getPermissions(conf, entry, t, null, null, null, false); | ||
| PermissionStorage.getPermissions(e.getConnection(), entry); | ||
| byte[] serialized = PermissionStorage.writePermissionsAsBytes(perms, conf); | ||
| zkPermissionWatcher.writeToZookeeper(entry, serialized); | ||
| } |
Comment on lines
+132
to
+133
| * Return the username for this table entry, and add family/qualifer to the given {@code builder} | ||
| * if available. |
Copilot stopped reviewing on behalf of
Apache9 due to an error
November 13, 2026 14:49
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…orage