Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23815 )
Change subject: IMPALA-14085: Implement GRANT/REVOKE ROLES TO/FROM a user ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/23815/3/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/23815/3/common/thrift/Frontend.thrift@310 PS3, Line 310: // Parameters for SHOW [CURRENT] ROLES and SHOW ROLE GRANT GROUP <groupName> commands nit: need to mention SHOW ROLE GRANT USER <user_name> as well. http://gerrit.cloudera.org:8080/#/c/23815/1/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/23815/1/common/thrift/JniCatalog.thrift@739 PS1, Line 739: 2: required list<string> group_names > I guess it would be safer to add an additional required bool field to TGran Yeah, this is the problem of using required fields. We should always use optional in the future. Can we mention adding principal_is_group in the commit message? http://gerrit.cloudera.org:8080/#/c/23815/3/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/23815/3/common/thrift/JniCatalog.thrift@745 PS3, Line 745: 4: optional list<string> user_names I'm confused why we use list here and other fileds. Can we grant roles to multiple users in a single statement / request? http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java File fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java: http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java@34 PS3, Line 34: private final boolean principalIsGroup_; nit: Can we add a comment mentioning that if principal is not a group, it's a user name? http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java@79 PS3, Line 79: if (Strings.isNullOrEmpty(groupName_) && Strings.isNullOrEmpty(userName_)) { : throw new AnalysisException("Group name and user name in GRANT/REVOKE ROLE " + : "cannot both be empty."); nit: might be nice to use different messages based on isGrantStmt_ and principalIsGroup_, i.e. "Group name cannot be empty in GRANT ROLE TO GROUP", "User name cannot be empty in REVOKE ROLE FROM USER". http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java: http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java@44 PS3, Line 44: SHOW ROLES where both 'groupName' and 'userName' are null How do we check both are null? http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java@47 PS3, Line 47: !(groupName != null && userName != null) nit: this can be simplified as "groupName == null || userName == null" http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@139 PS3, Line 139: /* group */ null); Do we need to pass groups of the grant user? Just confused on the difference with L131. http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@627 PS3, Line 627: revoke_role_params.group_names != null ? Should we use grant_role_params.isPrincipal_is_group() like L615? http://gerrit.cloudera.org:8080/#/c/23815/3/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/23815/3/tests/authorization/test_ranger.py@854 PS3, Line 854: cols[0:len(cols) - 1] if omit_last_column else cols[0:len(cols)] nit: I think we can simplify this to cols[:-1] if omit_last_column else cols http://gerrit.cloudera.org:8080/#/c/23815/3/tests/authorization/test_ranger.py@2334 PS3, Line 2334: TestRanger._check_rows(result_3, [['r_1'], ['r_2']]) I'm curious that can we revoke role r_1 from the user? If so, can we test it as well? http://gerrit.cloudera.org:8080/#/c/23815/3/tests/authorization/test_ranger.py@2337 PS3, Line 2337: admin_client.execute("revoke role r_2 from user {}".format(user)) It'd be nice to verify the SHOW results after these. -- To view, visit http://gerrit.cloudera.org:8080/23815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie5a16aeb4bbf8637ad326a1ec3d5fce1b196d73f Gerrit-Change-Number: 23815 Gerrit-PatchSet: 3 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Sat, 03 Jan 2026 02:41:33 +0000 Gerrit-HasComments: Yes
