From f39fa9384a7dd3bb9e52a9d90ccbc8badbd493e9 Mon Sep 17 00:00:00 2001 From: zhangdong Date: Wed, 2 Jul 2025 19:08:25 +0800 Subject: [PATCH 1/2] [fix](auth)fix when authentication, the permissions of multiple roles should be merged (#52349) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What problem does this PR solve? when user1 has two role: role1 and role2 role1 has priv1 role2 has priv2 when user1 needs both priv1 and priv2 for authorization **expect behavior**： Authentication successful **Current system behavior**： the system will throw a 'Permission Denied' error." **Fix Solution**： When validating permissions via the second role, retain awareness of privileges assigned to the first role # Conflicts: # fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Role.java # fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java # fe/fe-core/src/test/java/org/apache/doris/utframe/TestWithFeService.java --- .../apache/doris/mysql/privilege/Auth.java | 49 +++++++------------ .../apache/doris/mysql/privilege/Role.java | 46 +++++------------ .../doris/mysql/privilege/RoleManager.java | 2 +- .../suites/account_p0/test_grant_priv.groovy | 38 ++++++++++++++ 4 files changed, 70 insertions(+), 65 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java index ab5a9340f7d7bb..1ad3289803c2ad 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java +++ b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java @@ -270,8 +270,9 @@ protected boolean checkGlobalPriv(UserIdentity currentUser, PrivPredicate wanted readLock(); try { Set roles = getRolesByUserWithLdap(currentUser); + PrivBitSet savedPrivs = PrivBitSet.of(); for (Role role : roles) { - if (role.checkGlobalPriv(wanted)) { + if (role.checkGlobalPriv(wanted, savedPrivs)) { return true; } } @@ -293,8 +294,9 @@ protected boolean checkCtlPriv(UserIdentity currentUser, String ctl, PrivPredica readLock(); try { Set roles = getRolesByUserWithLdap(currentUser); + PrivBitSet savedPrivs = PrivBitSet.of(); for (Role role : roles) { - if (role.checkCtlPriv(ctl, wanted)) { + if (role.checkCtlPriv(ctl, wanted, savedPrivs)) { return true; } } @@ -316,8 +318,9 @@ protected boolean checkDbPriv(UserIdentity currentUser, String ctl, String db, P readLock(); try { Set roles = getRolesByUserWithLdap(currentUser); + PrivBitSet savedPrivs = PrivBitSet.of(); for (Role role : roles) { - if (role.checkDbPriv(ctl, db, wanted)) { + if (role.checkDbPriv(ctl, db, wanted, savedPrivs)) { return true; } } @@ -339,8 +342,9 @@ protected boolean checkTblPriv(UserIdentity currentUser, String ctl, String db, readLock(); try { Set roles = getRolesByUserWithLdap(currentUser); + PrivBitSet savedPrivs = PrivBitSet.of(); for (Role role : roles) { - if (role.checkTblPriv(ctl, db, tbl, wanted)) { + if (role.checkTblPriv(ctl, db, tbl, wanted, savedPrivs)) { return true; } } @@ -367,8 +371,9 @@ protected void checkColsPriv(UserIdentity currentUser, String ctl, String db, St private boolean checkColPriv(String ctl, String db, String tbl, String col, PrivPredicate wanted, Set roles) { + PrivBitSet savedPrivs = PrivBitSet.of(); for (Role role : roles) { - if (role.checkColPriv(ctl, db, tbl, col, wanted)) { + if (role.checkColPriv(ctl, db, tbl, col, wanted, savedPrivs)) { return true; } } @@ -380,8 +385,9 @@ protected boolean checkResourcePriv(UserIdentity currentUser, String resourceNam readLock(); try { Set roles = getRolesByUserWithLdap(currentUser); + PrivBitSet savedPrivs = PrivBitSet.of(); for (Role role : roles) { - if (role.checkResourcePriv(resourceName, wanted)) { + if (role.checkResourcePriv(resourceName, wanted, savedPrivs)) { return true; } } @@ -396,8 +402,9 @@ protected boolean checkStorageVaultPriv(UserIdentity currentUser, String storage readLock(); try { Set roles = getRolesByUserWithLdap(currentUser); + PrivBitSet savedPrivs = PrivBitSet.of(); for (Role role : roles) { - if (role.checkStorageVaultPriv(storageVaultName, wanted)) { + if (role.checkStorageVaultPriv(storageVaultName, wanted, savedPrivs)) { return true; } } @@ -418,8 +425,9 @@ protected boolean checkWorkloadGroupPriv(UserIdentity currentUser, String worklo } Set roles = getRolesByUserWithLdap(currentUser); + PrivBitSet savedPrivs = PrivBitSet.of(); for (Role role : roles) { - if (role.checkWorkloadGroupPriv(workloadGroupName, wanted)) { + if (role.checkWorkloadGroupPriv(workloadGroupName, wanted, savedPrivs)) { return true; } } @@ -440,29 +448,10 @@ protected boolean checkCloudPriv(UserIdentity currentUser, String cloudName, return true; } } - Set roles = userRoleManager.getRolesByUser(currentUser); - for (String roleName : roles) { - if (roleManager.getRole(roleName).checkCloudPriv(cloudName, wanted, type)) { - return true; - } - } - return false; - } finally { - readUnlock(); - } - } - - // ==== Other ==== - /* - * Check if current user has certain privilege. - * This method will check the given privilege levels - */ - public boolean checkHasPriv(ConnectContext ctx, PrivPredicate priv, PrivLevel... levels) { - readLock(); - try { - Set roles = getRolesByUserWithLdap(ctx.getCurrentUserIdentity()); + Set roles = getRolesByUserWithLdap(currentUser); + PrivBitSet savedPrivs = PrivBitSet.of(); for (Role role : roles) { - if (role.checkHasPriv(priv, levels)) { + if (role.checkCloudPriv(cloudName, wanted, type, savedPrivs)) { return true; } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Role.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Role.java index 4506afb3399b26..3d6f496d54b200 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Role.java +++ b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Role.java @@ -313,13 +313,11 @@ public void merge(Role other) throws DdlException { mergeNotCheck(other); } - public boolean checkGlobalPriv(PrivPredicate wanted) { - PrivBitSet savedPrivs = PrivBitSet.of(); + public boolean checkGlobalPriv(PrivPredicate wanted, PrivBitSet savedPrivs) { return checkGlobalInternal(wanted, savedPrivs); } - public boolean checkCtlPriv(String ctl, PrivPredicate wanted) { - PrivBitSet savedPrivs = PrivBitSet.of(); + public boolean checkCtlPriv(String ctl, PrivPredicate wanted, PrivBitSet savedPrivs) { if (checkGlobalInternal(wanted, savedPrivs) || checkCatalogInternal(ctl, wanted, savedPrivs)) { return true; @@ -363,8 +361,7 @@ private boolean checkCatalogInternal(String ctl, PrivPredicate wanted, PrivBitSe return false; } - public boolean checkDbPriv(String ctl, String db, PrivPredicate wanted) { - PrivBitSet savedPrivs = PrivBitSet.of(); + public boolean checkDbPriv(String ctl, String db, PrivPredicate wanted, PrivBitSet savedPrivs) { if (checkGlobalInternal(wanted, savedPrivs) || checkCatalogInternal(ctl, wanted, savedPrivs) || checkDbInternal(ctl, db, wanted, savedPrivs)) { @@ -435,8 +432,7 @@ private boolean checkDbInternal(String ctl, String db, PrivPredicate wanted, return false; } - public boolean checkTblPriv(String ctl, String db, String tbl, PrivPredicate wanted) { - PrivBitSet savedPrivs = PrivBitSet.of(); + public boolean checkTblPriv(String ctl, String db, String tbl, PrivPredicate wanted, PrivBitSet savedPrivs) { if (checkGlobalInternal(wanted, savedPrivs) || checkCatalogInternal(ctl, wanted, savedPrivs) || checkDbInternal(ctl, db, wanted, savedPrivs) @@ -457,14 +453,13 @@ public boolean checkTblPriv(String ctl, String db, String tbl, PrivPredicate wan } public boolean checkCloudPriv(String cloudName, - PrivPredicate wanted, ResourceTypeEnum type) { + PrivPredicate wanted, ResourceTypeEnum type, PrivBitSet savedPrivs) { ResourcePrivTable cloudPrivTable = getCloudPrivTable(type); if (cloudPrivTable == null) { LOG.warn("cloud resource type err: {}", type); return false; } - PrivBitSet savedPrivs = PrivBitSet.of(); if (checkGlobalInternal(wanted, savedPrivs) || checkCloudInternal(cloudName, wanted, savedPrivs, cloudPrivTable, type) || checkCloudVirtualComputeGroup(cloudName, wanted, savedPrivs, cloudPrivTable)) { @@ -475,12 +470,14 @@ public boolean checkCloudPriv(String cloudName, return false; } - public boolean checkColPriv(String ctl, String db, String tbl, String col, PrivPredicate wanted) { + public boolean checkColPriv(String ctl, String db, String tbl, String col, PrivPredicate wanted, + PrivBitSet savedPrivs) { Optional colPrivilege = wanted.getColPrivilege(); if (!colPrivilege.isPresent()) { throw new IllegalStateException("this privPredicate should not use checkColPriv:" + wanted); } - return checkTblPriv(ctl, db, tbl, wanted) || onlyCheckColPriv(ctl, db, tbl, col, colPrivilege.get()); + return checkTblPriv(ctl, db, tbl, wanted, savedPrivs) || onlyCheckColPriv(ctl, db, tbl, col, + colPrivilege.get()); } private boolean onlyCheckColPriv(String ctl, String db, String tbl, String col, @@ -497,8 +494,7 @@ private boolean checkTblInternal(String ctl, String db, String tbl, PrivPredicat return Privilege.satisfy(savedPrivs, wanted); } - public boolean checkResourcePriv(String resourceName, PrivPredicate wanted) { - PrivBitSet savedPrivs = PrivBitSet.of(); + public boolean checkResourcePriv(String resourceName, PrivPredicate wanted, PrivBitSet savedPrivs) { if (checkGlobalInternal(wanted, savedPrivs) || checkResourceInternal(resourceName, wanted, savedPrivs)) { return true; @@ -515,8 +511,7 @@ private boolean checkResourceInternal(String resourceName, PrivPredicate wanted, return Privilege.satisfy(savedPrivs, wanted); } - public boolean checkStorageVaultPriv(String storageVaultName, PrivPredicate wanted) { - PrivBitSet savedPrivs = PrivBitSet.of(); + public boolean checkStorageVaultPriv(String storageVaultName, PrivPredicate wanted, PrivBitSet savedPrivs) { if (checkGlobalInternal(wanted, savedPrivs) || checkStorageVaultInternal(storageVaultName, wanted, savedPrivs)) { return true; @@ -550,12 +545,11 @@ private boolean checkCloudVirtualComputeGroup(String cloudName, PrivPredicate wa return Privilege.satisfy(savedPrivs, wanted); } - public boolean checkWorkloadGroupPriv(String workloadGroupName, PrivPredicate wanted) { + public boolean checkWorkloadGroupPriv(String workloadGroupName, PrivPredicate wanted, PrivBitSet savedPrivs) { // For compatibility with older versions, it is not needed to check the privileges of the default group. if (WorkloadGroupMgr.DEFAULT_GROUP_NAME.equals(workloadGroupName)) { return true; } - PrivBitSet savedPrivs = PrivBitSet.of(); // usage priv not in global, but grant_priv may in global if (checkGlobalInternal(wanted, savedPrivs) || checkWorkloadGroupInternal(workloadGroupName, wanted, savedPrivs)) { @@ -646,22 +640,6 @@ public void setComment(String comment) { this.comment = comment; } - public boolean checkCanEnterCluster(String clusterName) { - if (checkGlobalPriv(PrivPredicate.ALL)) { - return true; - } - - if (dbPrivTable.hasClusterPriv(clusterName)) { - return true; - } - - if (tablePrivTable.hasClusterPriv(clusterName)) { - return true; - } - return false; - } - - private void grantPrivs(ResourcePattern resourcePattern, PrivBitSet privs) throws DdlException { if (privs.isEmpty()) { return; diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/RoleManager.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/RoleManager.java index 46949ef473953a..eba297bfd37148 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/RoleManager.java +++ b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/RoleManager.java @@ -224,7 +224,7 @@ public void getRoleWorkloadGroupPrivs(List> result, Set lim if (limitedRole != null && !limitedRole.contains(role.getRoleName())) { continue; } - String isGrantable = role.checkGlobalPriv(PrivPredicate.ADMIN) ? "YES" : "NO"; + String isGrantable = role.checkGlobalPriv(PrivPredicate.ADMIN, PrivBitSet.of()) ? "YES" : "NO"; for (Map.Entry entry : role.getWorkloadGroupPatternToPrivs().entrySet()) { List row = Lists.newArrayList(); diff --git a/regression-test/suites/account_p0/test_grant_priv.groovy b/regression-test/suites/account_p0/test_grant_priv.groovy index d2e04945905bee..21d26dd30d8330 100644 --- a/regression-test/suites/account_p0/test_grant_priv.groovy +++ b/regression-test/suites/account_p0/test_grant_priv.groovy @@ -21,6 +21,7 @@ suite("test_grant_priv") { def user1 = 'test_grant_priv_user1' def user2 = 'test_grant_priv_user2' def role1 = 'test_grant_priv_role1' + def role2 = 'test_grant_priv_role2' def pwd = '123456' def dbName = 'test_grant_priv_db' def tokens = context.config.jdbcUrl.split('/') @@ -29,6 +30,7 @@ suite("test_grant_priv") { sql """drop user if exists ${user1}""" sql """drop user if exists ${user2}""" sql """drop role if exists ${role1}""" + sql """drop role if exists ${role2}""" sql """DROP DATABASE IF EXISTS ${dbName}""" sql """CREATE DATABASE ${dbName}""" @@ -82,5 +84,41 @@ suite("test_grant_priv") { sql """drop user if exists ${user1}""" sql """drop user if exists ${user2}""" sql """drop role if exists ${role1}""" + sql """drop role if exists ${role2}""" + + sql """CREATE ROLE ${role1}""" + sql """CREATE ROLE ${role2}""" + sql """CREATE USER '${user1}' IDENTIFIED BY '${pwd}'""" + // for login + sql """grant select_priv on ${dbName}.* to ${user1}""" + sql """CREATE USER '${user2}' IDENTIFIED BY '${pwd}'""" + sql """grant grant_priv on *.*.* to role ${role1}""" + sql """grant select_priv on *.*.* to role ${role2}""" + sql """grant '${role1}' to ${user1}""" + // test only have role1 can not grant + connect(user1, "${pwd}", url) { + test { + sql """grant select_priv on *.*.* to ${user2}""" + exception "denied" + } + } + sql """revoke '${role1}' from ${user1}""" + sql """grant '${role2}' to ${user1}""" + // test only have role2 can not grant + connect(user1, "${pwd}", url) { + test { + sql """grant select_priv on *.*.* to ${user2}""" + exception "denied" + } + } + // test both have role1 and role2 can grant to other + sql """grant '${role1}' to ${user1}""" + connect(user1, "${pwd}", url) { + sql """grant select_priv on *.*.* to ${user2}""" + } + sql """drop user if exists ${user1}""" + sql """drop user if exists ${user2}""" + sql """drop role if exists ${role1}""" + sql """drop role if exists ${role2}""" sql """DROP DATABASE IF EXISTS ${dbName}""" } From 1ec30765bbb7c297d343ce14744aa76084930dd7 Mon Sep 17 00:00:00 2001 From: zhangdong Date: Wed, 9 Jul 2025 10:02:03 +0800 Subject: [PATCH 2/2] fix p0 --- regression-test/suites/account_p0/test_grant_priv.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/regression-test/suites/account_p0/test_grant_priv.groovy b/regression-test/suites/account_p0/test_grant_priv.groovy index 21d26dd30d8330..ef0dda3a07ad42 100644 --- a/regression-test/suites/account_p0/test_grant_priv.groovy +++ b/regression-test/suites/account_p0/test_grant_priv.groovy @@ -92,8 +92,8 @@ suite("test_grant_priv") { // for login sql """grant select_priv on ${dbName}.* to ${user1}""" sql """CREATE USER '${user2}' IDENTIFIED BY '${pwd}'""" - sql """grant grant_priv on *.*.* to role ${role1}""" - sql """grant select_priv on *.*.* to role ${role2}""" + sql """grant grant_priv on *.*.* to role '${role1}'""" + sql """grant select_priv on *.*.* to role '${role2}'""" sql """grant '${role1}' to ${user1}""" // test only have role1 can not grant connect(user1, "${pwd}", url) {