morrySnow commented on code in PR #62995:
URL: https://github.com/apache/doris/pull/62995#discussion_r3569680057
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowGrantsCommand.java:
##########
@@ -82,13 +89,31 @@ public ShowResultSet doRun(ConnectContext ctx, StmtExecutor
executor) throws Exc
}
}
boolean isSelf = userIdent != null &&
ConnectContext.get().getCurrentUserIdentity().equals(userIdent);
- Preconditions.checkState(isAll || userIdent != null);
- // if show all grants, or show other user's grants, need global GRANT
priv.
+ boolean hasGlobalGrantPriv = Env.getCurrentEnv().getAccessManager()
+ .checkGlobalPriv(ConnectContext.get(), PrivPredicate.GRANT);
+ Preconditions.checkState(isAll || userIdent != null || roleName !=
null);
+ // if show all grants, or show other user's grants, or show role's
grants, need global GRANT priv.
if (isAll || !isSelf) {
Review Comment:
**Bug (privilege check ordering):** When `roleName != null` and `userIdent
== null` (from the `ShowGrantsCommand(String)` constructor), `isSelf` evaluates
to `false`, so `!isSelf` is `true`, and this `if (isAll || !isSelf)` always
triggers for role queries. A user who belongs to the role but lacks GRANT
privilege is denied here before reaching the role-membership bypass check below
(line 112).
**Fix suggestion:** Change this condition to:
```java
if (userIdent != null && (isAll || !isSelf)) {
```
so role-based queries skip this general check and are handled by the
dedicated role privilege logic.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowGrantsCommand.java:
##########
@@ -82,13 +89,31 @@ public ShowResultSet doRun(ConnectContext ctx, StmtExecutor
executor) throws Exc
}
}
boolean isSelf = userIdent != null &&
ConnectContext.get().getCurrentUserIdentity().equals(userIdent);
- Preconditions.checkState(isAll || userIdent != null);
- // if show all grants, or show other user's grants, need global GRANT
priv.
+ boolean hasGlobalGrantPriv = Env.getCurrentEnv().getAccessManager()
+ .checkGlobalPriv(ConnectContext.get(), PrivPredicate.GRANT);
+ Preconditions.checkState(isAll || userIdent != null || roleName !=
null);
+ // if show all grants, or show other user's grants, or show role's
grants, need global GRANT priv.
if (isAll || !isSelf) {
- if
(!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ConnectContext.get(),
PrivPredicate.GRANT)) {
+ if (!hasGlobalGrantPriv) {
ErrorReport.reportAnalysisException(ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR,
"GRANT");
}
}
+ if (roleName != null) {
+ // check role if exists
+ if
(!Env.getCurrentEnv().getAccessManager().getAuth().doesRoleExist(roleName)) {
+ throw new AnalysisException(String.format("Role: %s does not
exist", roleName));
+ }
+
+ // check current user if belongs to this role
+ Set<UserIdentity> roleUsers =
Env.getCurrentEnv().getAccessManager().getAuth().getRoleUsers(roleName);
+ boolean hasRole =
roleUsers.contains(ConnectContext.get().getCurrentUserIdentity());
+
+ // only users has admin priv or users belong to this role, that
have show priv
+ if (!hasGlobalGrantPriv && !hasRole) {
Review Comment:
**Dead code path:** The `!hasGlobalGrantPriv && hasRole` case (user is a
role member but lacks GRANT priv) can never reach here — those users are
already denied at line 96-98. Only users WITH GRANT privilege survive the
earlier check, so `!hasGlobalGrantPriv` is always `false` here. After fixing
the check at line 96 to exclude role queries, this condition will correctly
handle both cases.
##########
regression-test/suites/query_p0/show/test_nereids_show_grants_for_role.groovy:
##########
@@ -0,0 +1,128 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_show_grants_for_role", "p0,auth") {
+ def currentTimeMillis = System.currentTimeMillis()
+ String roleName = "test_show_grants_for_role_${currentTimeMillis}_role"
+ String noneExistRoleName =
"test_show_grants_for_role_${currentTimeMillis}_none_exist_role"
+ String userName1WithRole =
"'test_show_grants_for_role_${currentTimeMillis}_user1_with_role'@'%'"
+ String userName2WithRole =
"'test_show_grants_for_role_${currentTimeMillis}_user2_with_role'@'%'"
+ String pwd = "123456"
+ String dbName = "test_show_grants_for_role_${currentTimeMillis}_db"
+ String tableName = "test_show_grants_for_role_${currentTimeMillis}_table"
+ String userWithGrantPriv =
"test_show_grants_for_role_${currentTimeMillis}_user_with_grant_priv"
+
+ try_sql("DROP ROLE IF EXISTS ${roleName}")
+ try_sql("DROP USER IF EXISTS ${userName1WithRole}")
+ try_sql("DROP USER IF EXISTS ${userName2WithRole}")
+
+ // Create role and user
+ sql """CREATE ROLE ${roleName}"""
+ sql """CREATE USER ${userName1WithRole} IDENTIFIED BY '${pwd}' default
role '${roleName}'"""
+ sql """CREATE USER ${userName2WithRole} IDENTIFIED BY '${pwd}' default
role '${roleName}'"""
+ sql """CREATE USER ${userWithGrantPriv} IDENTIFIED BY '${pwd}'"""
+ sql """create database if not exists ${dbName}"""
+ sql """
+ CREATE TABLE if not exists ${dbName}.${tableName}(
+ id bigint NOT NULL,
+ name varchar
+ ) ENGINE=OLAP
+ unique KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1"
+ );
+ """
+ sql """insert into
${dbName}.${tableName}(id,name)values(1,'a'),(2,'b'),(3,'c'),(4,'d'),(5,'e'),(6,'f');"""
+
+ // Grant privileges to role
+ sql """GRANT SELECT_PRIV ON ${dbName}.${tableName} TO ROLE '${roleName}'"""
+
+ // Test show grants for role exists
+ def roleGrants = sql """SHOW GRANTS FOR ROLE '${roleName}'"""
+ assertTrue(roleGrants.size() == 2)
+ logger.info("roleGrants: ${roleGrants}")
+
+ def userTablePrivs = "internal.${dbName}.${tableName}: Select_priv"
+ for (int i = 0; i < roleGrants.size(); i++) {
+ def grant = roleGrants.get(i)
+ logger.info("grant: ${grant}")
+ def userIdentity = grant.get(0)
+ def roles = grant.get(4)
+ def tablePrivs = grant.get(8)
+ logger.info("Roles: ${roles}")
+ logger.info("UserIdentity: ${userIdentity}")
+ logger.info("TablePrivs: ${tablePrivs}")
+ assertTrue(userName1WithRole == userIdentity || userName2WithRole ==
userIdentity)
+ assertTrue(roles == roleName)
+ assertTrue(tablePrivs == userTablePrivs)
+ }
+
+ // test show grants for none exist role
+ test {
+ sql """SHOW GRANTS FOR ROLE '${noneExistRoleName}'"""
+ exception "Role: ${noneExistRoleName} does not exist"
+ }
+
+ // test show grants for root role
+ def rootUserGrants = sql """show grants for 'root'"""
+ def rootRoleGrants = sql """show grants for role 'operator'"""
+ assertTrue(rootUserGrants.size() == rootRoleGrants.size())
+ for (int i = 0; i < rootUserGrants.size(); i++) {
+ def rootUserGrant = rootUserGrants.get(0)
Review Comment:
**Bug:** `get(0)` should be `get(i)`. The loop variable `i` is never used,
so every iteration compares only the first row of each list. If
`rootUserGrants` and `rootRoleGrants` have the same size but differ at any row
beyond index 0, this test passes incorrectly.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java:
##########
@@ -2188,4 +2188,27 @@ public List<List<String>> getAllUserInfo() {
}
return userInfos;
}
+
+ public List<List<String>> getRoleAuthInfo(String roleName) {
+ List<List<String>> results = Lists.newArrayList();
+ Role role = roleManager.getRole(roleName);
+ if (role == null) {
+ return results;
+ }
+ // get all users belong to this role
+ Set<UserIdentity> users = getRoleUsers(roleName);
+ for (UserIdentity user : users) {
+ List<List<String>> userGrants = getAuthInfo(user);
+ results.addAll(userGrants);
+ }
+ return results;
+ }
+
+ public List<List<String>> getRoleAuthInfo(String roleName, UserIdentity
currentUser) {
Review Comment:
**Issue:** The `roleName` parameter is accepted but never referenced in the
method body. This method always returns `getAuthInfo(currentUser)` — the
complete grant set for the user, including grants from ALL roles and direct
grants — with no filtering by the specified `roleName`. Consider either using
`roleName` to filter results (if the intent is to show only role-specific
grants), or removing the unused parameter and renaming the method to reflect
what it actually does (e.g., `getCurrentUserAuthInfo`).
##########
regression-test/suites/query_p0/show/test_nereids_show_grants_for_role.groovy:
##########
@@ -0,0 +1,128 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_show_grants_for_role", "p0,auth") {
+ def currentTimeMillis = System.currentTimeMillis()
+ String roleName = "test_show_grants_for_role_${currentTimeMillis}_role"
+ String noneExistRoleName =
"test_show_grants_for_role_${currentTimeMillis}_none_exist_role"
+ String userName1WithRole =
"'test_show_grants_for_role_${currentTimeMillis}_user1_with_role'@'%'"
+ String userName2WithRole =
"'test_show_grants_for_role_${currentTimeMillis}_user2_with_role'@'%'"
+ String pwd = "123456"
+ String dbName = "test_show_grants_for_role_${currentTimeMillis}_db"
+ String tableName = "test_show_grants_for_role_${currentTimeMillis}_table"
+ String userWithGrantPriv =
"test_show_grants_for_role_${currentTimeMillis}_user_with_grant_priv"
+
+ try_sql("DROP ROLE IF EXISTS ${roleName}")
+ try_sql("DROP USER IF EXISTS ${userName1WithRole}")
+ try_sql("DROP USER IF EXISTS ${userName2WithRole}")
+
+ // Create role and user
+ sql """CREATE ROLE ${roleName}"""
+ sql """CREATE USER ${userName1WithRole} IDENTIFIED BY '${pwd}' default
role '${roleName}'"""
+ sql """CREATE USER ${userName2WithRole} IDENTIFIED BY '${pwd}' default
role '${roleName}'"""
+ sql """CREATE USER ${userWithGrantPriv} IDENTIFIED BY '${pwd}'"""
+ sql """create database if not exists ${dbName}"""
+ sql """
+ CREATE TABLE if not exists ${dbName}.${tableName}(
+ id bigint NOT NULL,
+ name varchar
+ ) ENGINE=OLAP
+ unique KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1"
+ );
+ """
+ sql """insert into
${dbName}.${tableName}(id,name)values(1,'a'),(2,'b'),(3,'c'),(4,'d'),(5,'e'),(6,'f');"""
+
+ // Grant privileges to role
+ sql """GRANT SELECT_PRIV ON ${dbName}.${tableName} TO ROLE '${roleName}'"""
+
+ // Test show grants for role exists
+ def roleGrants = sql """SHOW GRANTS FOR ROLE '${roleName}'"""
+ assertTrue(roleGrants.size() == 2)
+ logger.info("roleGrants: ${roleGrants}")
+
+ def userTablePrivs = "internal.${dbName}.${tableName}: Select_priv"
+ for (int i = 0; i < roleGrants.size(); i++) {
+ def grant = roleGrants.get(i)
+ logger.info("grant: ${grant}")
+ def userIdentity = grant.get(0)
+ def roles = grant.get(4)
+ def tablePrivs = grant.get(8)
+ logger.info("Roles: ${roles}")
+ logger.info("UserIdentity: ${userIdentity}")
+ logger.info("TablePrivs: ${tablePrivs}")
+ assertTrue(userName1WithRole == userIdentity || userName2WithRole ==
userIdentity)
+ assertTrue(roles == roleName)
+ assertTrue(tablePrivs == userTablePrivs)
+ }
+
+ // test show grants for none exist role
+ test {
+ sql """SHOW GRANTS FOR ROLE '${noneExistRoleName}'"""
+ exception "Role: ${noneExistRoleName} does not exist"
+ }
+
+ // test show grants for root role
+ def rootUserGrants = sql """show grants for 'root'"""
+ def rootRoleGrants = sql """show grants for role 'operator'"""
+ assertTrue(rootUserGrants.size() == rootRoleGrants.size())
+ for (int i = 0; i < rootUserGrants.size(); i++) {
+ def rootUserGrant = rootUserGrants.get(0)
+ def rootRoleGrant = rootRoleGrants.get(0)
+ for (int j = 0; j < rootUserGrant.size(); j++) {
+ assertTrue(rootUserGrant.get(j) == rootRoleGrant.get(j))
+ }
+ }
+
+ // test show grants for admin role
+ def adminUserGrants = sql """show grants for 'admin'"""
+ def adminRoleGrants = sql """show grants for role 'admin'"""
+ assertTrue(adminUserGrants.size() == adminRoleGrants.size())
+ for (int i = 0; i < adminUserGrants.size(); i++) {
+ def adminUserGrant = adminUserGrants.get(0)
+ def adminRoleGrant = adminRoleGrants.get(0)
Review Comment:
**Bug:** Same issue as line 86 — `get(0)` should be `get(i)`. Only the first
admin grant row is compared on every loop iteration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
