Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/23815 )
Change subject: IMPALA-14085: Implement GRANT/REVOKE ROLE TO/FROM a user ...................................................................... Patch Set 4: (12 comments) I have tried to address Quanlong's comments on patch set 3. Let me know if there are additional suggestions. Thanks! 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, SHOW ROLE GRANT GROUP <groupName> and > nit: need to mention SHOW ROLE GRANT USER <user_name> as well. Done 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 > Yeah, this is the problem of using required fields. We should always use op Yes I will mention '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 > Can we grant roles to multiple users in a single statement / request? In theory we could, since in GrantRevokeRoleRequest (https://github.com/apache/ranger/blob/master/agents-common/src/main/java/org/apache/ranger/plugin/util/GrantRevokeRoleRequest.java), the fields that encode the grantee groups and grantee users are Set's and in RangerCatalogdAuthorizationManager#createGrantRevokeRoleRequest(), we populate the Set's using the input arguments 'groupNames' and 'userNames', each of which is a List of String. public class GrantRevokeRoleRequest implements Serializable { ... private Set<String> users; private Set<String> groups; ... } 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: // If 'principalIsGroup_' is false, then the grantee/revokee is a user. > nit: Can we add a comment mentioning that if principal is not a group, it's Yes. I will add a comment in the next patch. http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java@79 PS3, Line 79: } : if (principalIsGroup_ && Strings.isNullOrEmpty(groupName_)) { : throw new AnalysisException(S > nit: might be nice to use different messages based on isGrantStmt_ and prin Yes I will change the error message in the next patch. 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: e' should not both be set whether 'isShowCurrentRoles' is > How do we check both are null? I think I wanted to verify that if isShowCurrentRoles is false, then 'groupName' and 'userName' cannot both be non-empty. Taking a closer look at show_roles_stmt in https://gerrit.cloudera.org/c/23815/3/fe/src/main/cup/sql-parser.cup#1171, I think we could further simply the check to the following, since this should also be true even when isShowCurrentRoles is true. Preconditions.checkState(groupName == null || userName == null) I will make the corresponding change in the next patch. http://gerrit.cloudera.org:8080/#/c/23815/3/fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java@47 PS3, Line 47: n empty 'groupName' or 'userName' should > nit: this can be simplified as "groupName == null || userName == null" Thanks! I will make the corresponding change as described above. 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 differenc I guess we don't have to. I took a look at https://cwiki.apache.org/confluence/display/Hive/SQL+Standard+Based+Hive+Authorization where we could find an example of SHOW ROLE GRANT USER but it's not clear whether Apache Hive also lists the roles associated with groups the user belongs to. It looks like https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/show/rolegrant/ShowRoleGrantOperation.java is the implementation for the SHOW ROLE GRANT USER/GROUP command in Apache Hive. Specifically, in ShowRoleGrantOperation#execute(), authorizer.getRoleGrantInfoForPrincipal() is called. When Ranger is the authorization provider, RangerHiveAuthorizer#getRoleGrantInfoForPrincipal() implements the method, which in turn calls the following to retrieve the roles associated with the given principal. Set<RangerRole> roles = hivePlugin.getRangerRoleForPrincipal(principalName, type); The implementation of getRangerRoleForPrincipal() is in https://github.com/apache/ranger/blob/master/agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java, and it does not look like Ranger returns the roles associated with the groups the user belongs to when 'type' is "USER". switch (type) { case "USER": roleMapping = rangerRolesUtil.getUserRoleMapping(); break; case "GROUP": roleMapping = rangerRolesUtil.getGroupRoleMapping(); break; case "ROLE": roleMapping = rangerRolesUtil.getRoleRoleMapping(); break; } 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.isPrincipal_is_group() ? > Should we use grant_role_params.isPrincipal_is_group() like L615? Yes. It would be good to do so to make them consistent with each other. 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: format(access_type, cols_str, db, tbl, kw, principal_name)) > nit: I think we can simplify this to Done http://gerrit.cloudera.org:8080/#/c/23815/3/tests/authorization/test_ranger.py@2334 PS3, Line 2334: self.execute_query_expect_success(admin_client, > can we revoke role r_1 from the user? We have to revoke the role 'r_1' from the group 'getuser()' so that the user 'getuser()' is not associated with the role 'r_1'. I will add more test cases to demonstrate this in the next patch. http://gerrit.cloudera.org:8080/#/c/23815/3/tests/authorization/test_ranger.py@2337 PS3, Line 2337: self.execute_query_expect_success(admin_client, > It'd be nice to verify the SHOW results after these. Yes I will verify the SHOW results after these in the next patch. -- 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: 4 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, 10 Jan 2026 00:01:41 +0000 Gerrit-HasComments: Yes
