From f376b02d37f41ec7c2a813cfaa74d152d5648c74 Mon Sep 17 00:00:00 2001 From: zhangdong <493738387@qq.com> Date: Mon, 21 Aug 2023 21:52:08 +0800 Subject: [PATCH 1/5] disable col auth --- .../java/org/apache/doris/common/Config.java | 5 ++ .../org/apache/doris/analysis/GrantStmt.java | 4 +- .../org/apache/doris/analysis/SelectStmt.java | 9 +++ .../rules/analysis/UserAuthentication.java | 29 ++++++-- .../apache/doris/planner/OriginalPlanner.java | 73 ------------------- .../doris/mysql/privilege/AuthTest.java | 2 + 6 files changed, 42 insertions(+), 80 deletions(-) diff --git a/fe/fe-common/src/main/java/org/apache/doris/common/Config.java b/fe/fe-common/src/main/java/org/apache/doris/common/Config.java index aa415da2121ff9..2a97e4d47477e5 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/common/Config.java +++ b/fe/fe-common/src/main/java/org/apache/doris/common/Config.java @@ -2111,4 +2111,9 @@ public class Config extends ConfigBase { "是否用 mysql 的 bigint 类型来返回 Doris 的 largeint 类型", "Whether to use mysql's bigint type to return Doris's largeint type"}) public static boolean use_mysql_bigint_for_largeint = false; + + @ConfField(description = { + "是否开启列权限", + "Whether to enable col auth"}) + public static boolean enable_col_auth = false; } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java index aa0c21f0f0a488..c0eb482ea92d36 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java @@ -21,6 +21,7 @@ import org.apache.doris.catalog.Env; import org.apache.doris.cluster.ClusterNamespace; import org.apache.doris.common.AnalysisException; +import org.apache.doris.common.Config; import org.apache.doris.common.ErrorCode; import org.apache.doris.common.ErrorReport; import org.apache.doris.common.FeNameFormat; @@ -181,7 +182,8 @@ public void analyze(Analyzer analyzer) throws UserException { public static void checkAccessPrivileges( List accessPrivileges) throws AnalysisException { for (AccessPrivilegeWithCols access : accessPrivileges) { - if (!access.getAccessPrivilege().canHasColPriv() && !CollectionUtils.isEmpty(access.getCols())) { + if ((!access.getAccessPrivilege().canHasColPriv() && !CollectionUtils.isEmpty(access.getCols())) + || !Config.enable_col_auth) { throw new AnalysisException( String.format("%s do not support col auth.", access.getAccessPrivilege().name())); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java index b69466ab51003c..0f03afde0afd24 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java @@ -24,6 +24,7 @@ import org.apache.doris.catalog.AggregateFunction; import org.apache.doris.catalog.Column; import org.apache.doris.catalog.DatabaseIf; +import org.apache.doris.catalog.Env; import org.apache.doris.catalog.FunctionSet; import org.apache.doris.catalog.OlapTable; import org.apache.doris.catalog.PrimitiveType; @@ -44,6 +45,7 @@ import org.apache.doris.common.UserException; import org.apache.doris.common.util.SqlUtils; import org.apache.doris.common.util.ToSqlContext; +import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.qe.ConnectContext; import org.apache.doris.rewrite.ExprRewriter; import org.apache.doris.rewrite.mvrewrite.MVSelectFailedException; @@ -392,6 +394,13 @@ public void getTables(Analyzer analyzer, boolean expandView, Map View view = (View) table; view.getQueryStmt().getTables(analyzer, expandView, tableMap, parentViewNameSet); } else { + // check auth + if (!Env.getCurrentEnv().getAccessManager() + .checkTblPriv(ConnectContext.get(), tblRef.getName(), PrivPredicate.SELECT)) { + ErrorReport.reportAnalysisException(ErrorCode.ERR_TABLEACCESS_DENIED_ERROR, "SELECT", + ConnectContext.get().getQualifiedUser(), ConnectContext.get().getRemoteIP(), + dbName + "." + tableName); + } tableMap.put(table.getId(), table); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/UserAuthentication.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/UserAuthentication.java index 3c97e904768b57..86fb64d6e56eaf 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/UserAuthentication.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/UserAuthentication.java @@ -17,7 +17,10 @@ package org.apache.doris.nereids.rules.analysis; +import org.apache.doris.catalog.DatabaseIf; +import org.apache.doris.catalog.TableIf; import org.apache.doris.common.ErrorCode; +import org.apache.doris.datasource.CatalogIf; import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.rules.Rule; @@ -44,17 +47,31 @@ private Plan checkPermission(CatalogRelation relation, ConnectContext connectCon if (connectContext.getSessionVariable().isPlayNereidsDump()) { return null; } - - String dbName = relation.getDatabase().getFullName(); - String tableName = relation.getTable().getName(); - if (!connectContext.getEnv().getAccessManager().checkTblPriv(connectContext, dbName, + TableIf table = relation.getTable(); + if (table == null) { + return null; + } + String tableName = table.getName(); + DatabaseIf db = table.getDatabase(); + // when table inatanceof FunctionGenTable,db will be null + if (db == null) { + return null; + } + String dbName = db.getFullName(); + CatalogIf catalog = db.getCatalog(); + if (catalog == null) { + return null; + } + String ctlName = catalog.getName(); + // TODO: 2023/7/19 checkColumnsPriv + if (!connectContext.getEnv().getAccessManager().checkTblPriv(connectContext, ctlName, dbName, tableName, PrivPredicate.SELECT)) { String message = ErrorCode.ERR_TABLEACCESS_DENIED_ERROR.formatErrorMsg("SELECT", ConnectContext.get().getQualifiedUser(), ConnectContext.get().getRemoteIP(), - dbName + ": " + tableName); + ctlName + ": " + dbName + ": " + tableName); throw new AnalysisException(message); } - return null; } + } diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/OriginalPlanner.java b/fe/fe-core/src/main/java/org/apache/doris/planner/OriginalPlanner.java index 08af81afbb19cd..7b0fa4512cc70f 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/planner/OriginalPlanner.java +++ b/fe/fe-core/src/main/java/org/apache/doris/planner/OriginalPlanner.java @@ -31,23 +31,15 @@ import org.apache.doris.analysis.SlotRef; import org.apache.doris.analysis.StatementBase; import org.apache.doris.analysis.StorageBackend; -import org.apache.doris.analysis.TableName; import org.apache.doris.analysis.TupleDescriptor; import org.apache.doris.catalog.Column; import org.apache.doris.catalog.Env; import org.apache.doris.catalog.OlapTable; import org.apache.doris.catalog.PrimitiveType; import org.apache.doris.catalog.ScalarType; -import org.apache.doris.catalog.Table; -import org.apache.doris.catalog.TableIf; import org.apache.doris.catalog.Type; -import org.apache.doris.catalog.external.ExternalTable; -import org.apache.doris.cluster.ClusterNamespace; -import org.apache.doris.common.AnalysisException; import org.apache.doris.common.Config; import org.apache.doris.common.UserException; -import org.apache.doris.datasource.InternalCatalog; -import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.qe.ConnectContext; import org.apache.doris.rewrite.mvrewrite.MVSelectFailedException; import org.apache.doris.statistics.query.StatsDelta; @@ -56,10 +48,7 @@ import org.apache.doris.thrift.TRuntimeFilterMode; import com.google.common.base.Preconditions; -import com.google.common.base.Strings; -import com.google.common.collect.HashMultimap; import com.google.common.collect.Lists; -import com.google.common.collect.Maps; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -173,8 +162,6 @@ public void createPlanFragments(StatementBase statement, Analyzer analyzer, TQue insertStmt.prepareExpressions(); } - checkColumnPrivileges(singleNodePlan); - // TODO chenhao16 , no used materialization work // compute referenced slots before calling computeMemLayout() //analyzer.markRefdSlots(analyzer, singleNodePlan, resultExprs, null); @@ -313,66 +300,6 @@ public void createPlanFragments(StatementBase statement, Analyzer analyzer, TQue } } - private void checkColumnPrivileges(PlanNode singleNodePlan) throws UserException { - if (ConnectContext.get() == null) { - return; - } - // 1. collect all columns from all scan nodes - List scanNodes = Lists.newArrayList(); - singleNodePlan.collect((PlanNode planNode) -> planNode instanceof ScanNode, scanNodes); - // catalog : - Map> ctlToTableColumnMap = Maps.newHashMap(); - for (ScanNode scanNode : scanNodes) { - if (!scanNode.needToCheckColumnPriv()) { - continue; - } - TupleDescriptor tupleDesc = scanNode.getTupleDesc(); - TableIf table = tupleDesc.getTable(); - if (table == null) { - continue; - } - TableName tableName = getFullQualifiedTableNameFromTable(table); - for (SlotDescriptor slotDesc : tupleDesc.getSlots()) { - if (!slotDesc.isMaterialized()) { - continue; - } - Column column = slotDesc.getColumn(); - if (column == null) { - continue; - } - HashMultimap tableColumnMap = ctlToTableColumnMap.get(tableName.getCtl()); - if (tableColumnMap == null) { - tableColumnMap = HashMultimap.create(); - ctlToTableColumnMap.put(tableName.getCtl(), tableColumnMap); - } - tableColumnMap.put(tableName, column.getName()); - LOG.debug("collect column {} in {}", column.getName(), tableName); - } - } - // 2. check privs - // TODO: only support SELECT_PRIV now - PrivPredicate wanted = PrivPredicate.SELECT; - for (Map.Entry> entry : ctlToTableColumnMap.entrySet()) { - Env.getCurrentEnv().getAccessManager().checkColumnsPriv(ConnectContext.get().getCurrentUserIdentity(), - entry.getKey(), entry.getValue(), wanted); - } - } - - private TableName getFullQualifiedTableNameFromTable(TableIf table) throws AnalysisException { - if (table instanceof Table) { - String dbName = ClusterNamespace.getNameFromFullName(((Table) table).getQualifiedDbName()); - if (Strings.isNullOrEmpty(dbName)) { - throw new AnalysisException("failed to get db name from table " + table.getName()); - } - return new TableName(InternalCatalog.INTERNAL_CATALOG_NAME, dbName, table.getName()); - } else if (table instanceof ExternalTable) { - ExternalTable extTable = (ExternalTable) table; - return new TableName(extTable.getCatalog().getName(), extTable.getDbName(), extTable.getName()); - } else { - throw new AnalysisException("table " + table.getName() + " is not internal or external table instance"); - } - } - /** * If there are unassigned conjuncts, returns a SelectNode on top of root that evaluate those conjuncts; otherwise * returns root unchanged. diff --git a/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java b/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java index 497830417726d1..b0afc8055d3cae 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java @@ -34,6 +34,7 @@ import org.apache.doris.catalog.DomainResolver; import org.apache.doris.catalog.Env; import org.apache.doris.common.AnalysisException; +import org.apache.doris.common.Config; import org.apache.doris.common.DdlException; import org.apache.doris.common.ExceptionChecker; import org.apache.doris.common.UserException; @@ -153,6 +154,7 @@ public void setUp() throws NoSuchMethodException, SecurityException { }; resolver = new MockDomainResolver(auth); + Config.enable_col_auth = true; } @Test From 8838a16525b4fc4b485da57e87f8bbf8851f30c6 Mon Sep 17 00:00:00 2001 From: zhangdong <493738387@qq.com> Date: Tue, 22 Aug 2023 10:51:46 +0800 Subject: [PATCH 2/5] disable col auth --- .../src/main/java/org/apache/doris/analysis/GrantStmt.java | 6 +++--- .../src/main/java/org/apache/doris/analysis/RevokeStmt.java | 2 +- .../main/java/org/apache/doris/analysis/ShowGrantsStmt.java | 3 +++ .../main/java/org/apache/doris/mysql/privilege/Auth.java | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java index c0eb482ea92d36..53c19add7edc9f 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java @@ -150,7 +150,7 @@ public void analyze(Analyzer analyzer) throws UserException { } else if (roles != null) { for (int i = 0; i < roles.size(); i++) { String originalRoleName = roles.get(i); - FeNameFormat.checkRoleName(originalRoleName, false /* can not be admin */, "Can not grant role"); + FeNameFormat.checkRoleName(originalRoleName, true /* can be admin */, "Can not grant role"); roles.set(i, ClusterNamespace.getFullName(analyzer.getClusterName(), originalRoleName)); } } @@ -182,8 +182,8 @@ public void analyze(Analyzer analyzer) throws UserException { public static void checkAccessPrivileges( List accessPrivileges) throws AnalysisException { for (AccessPrivilegeWithCols access : accessPrivileges) { - if ((!access.getAccessPrivilege().canHasColPriv() && !CollectionUtils.isEmpty(access.getCols())) - || !Config.enable_col_auth) { + if ((!access.getAccessPrivilege().canHasColPriv() || !Config.enable_col_auth) && !CollectionUtils + .isEmpty(access.getCols())) { throw new AnalysisException( String.format("%s do not support col auth.", access.getAccessPrivilege().name())); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/RevokeStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/RevokeStmt.java index 74e9a4de91f601..8e11c3a7d1b813 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/RevokeStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/RevokeStmt.java @@ -133,7 +133,7 @@ public void analyze(Analyzer analyzer) throws AnalysisException { } else if (roles != null) { for (int i = 0; i < roles.size(); i++) { String originalRoleName = roles.get(i); - FeNameFormat.checkRoleName(originalRoleName, false /* can not be admin */, "Can not revoke role"); + FeNameFormat.checkRoleName(originalRoleName, true /* can be admin */, "Can not revoke role"); roles.set(i, ClusterNamespace.getFullName(analyzer.getClusterName(), originalRoleName)); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowGrantsStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowGrantsStmt.java index a380b27451467c..e2c41eed0c9432 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowGrantsStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowGrantsStmt.java @@ -89,6 +89,9 @@ public void analyze(Analyzer analyzer) throws AnalysisException { ErrorReport.reportAnalysisException(ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR, "GRANT"); } } + if (userIdent != null && !Env.getCurrentEnv().getAccessManager().getAuth().doesUserExist(userIdent)) { + throw new AnalysisException(String.format("User: %s does not exist", userIdent)); + } } @Override 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 995a0164785157..636ac50f394f27 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 @@ -663,7 +663,7 @@ private void grantInternal(UserIdentity userIdent, List roles, boolean i // return true if user ident exist - private boolean doesUserExist(UserIdentity userIdent) { + public boolean doesUserExist(UserIdentity userIdent) { return userManager.userIdentityExist(userIdent, false); } From 655e879c701cde92e8dd6525234a5b8b6f50bb35 Mon Sep 17 00:00:00 2001 From: zhangdong <493738387@qq.com> Date: Tue, 22 Aug 2023 14:26:49 +0800 Subject: [PATCH 3/5] add p0 --- .../suites/account_p0/test_nereids_authentication.groovy | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/regression-test/suites/account_p0/test_nereids_authentication.groovy b/regression-test/suites/account_p0/test_nereids_authentication.groovy index 2d5d2cbb1382eb..46d60732d6b915 100644 --- a/regression-test/suites/account_p0/test_nereids_authentication.groovy +++ b/regression-test/suites/account_p0/test_nereids_authentication.groovy @@ -28,6 +28,7 @@ suite("test_nereids_authentication", "query") { } sql "set enable_nereids_planner = true" + sql "set enable_fallback_to_original_planner = false" def dbName = "nereids_authentication" sql "DROP DATABASE IF EXISTS ${dbName}" @@ -57,7 +58,7 @@ suite("test_nereids_authentication", "query") { fail() } catch (Exception e) { log.info(e.getMessage()) - assertTrue(e.getMessage().contains('Permission denied')) + assertTrue(e.getMessage().contains('denied to user')) } } @@ -67,7 +68,7 @@ suite("test_nereids_authentication", "query") { fail() } catch (Exception e) { log.info(e.getMessage()) - assertTrue(e.getMessage().contains('Permission denied')) + assertTrue(e.getMessage().contains('denied to user')) } } From bb83b99fcdbfc1a98e020b58c1df253ed271c709 Mon Sep 17 00:00:00 2001 From: zhangdong <493738387@qq.com> Date: Tue, 22 Aug 2023 15:26:35 +0800 Subject: [PATCH 4/5] fix ut --- .../main/java/org/apache/doris/datasource/ExternalCatalog.java | 3 +++ .../main/java/org/apache/doris/datasource/InternalCatalog.java | 3 +++ .../test/java/org/apache/doris/datasource/ColumnPrivTest.java | 3 +++ 3 files changed, 9 insertions(+) diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java index 301e2039ce6788..9dbc40ff8476ca 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java @@ -350,6 +350,9 @@ public String getResource() { @Nullable @Override public ExternalDatabase getDbNullable(String dbName) { + if (StringUtils.isEmpty(dbName)) { + return null; + } try { makeSureInitialized(); } catch (Exception e) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java index acaa0284c4fb4f..f658cccc546567 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java @@ -256,6 +256,9 @@ public List getDbIds() { @Nullable @Override public Database getDbNullable(String dbName) { + if (StringUtils.isEmpty(dbName)) { + return null; + } if (fullNameToDb.containsKey(dbName)) { return fullNameToDb.get(dbName); } else { diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/ColumnPrivTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/ColumnPrivTest.java index 151532aee7fe8e..b9726cf59875fd 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/ColumnPrivTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/ColumnPrivTest.java @@ -48,12 +48,15 @@ import com.google.common.collect.Maps; import org.junit.Assert; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import java.util.List; import java.util.Map; import java.util.Set; +// when `select` suppport `col auth`,will open ColumnPrivTest +@Disabled public class ColumnPrivTest extends TestWithFeService { private static Auth auth; private static Env env; From 416e5982ae0cb4c128eb700dea36178e9d1c34f3 Mon Sep 17 00:00:00 2001 From: zhangdong <493738387@qq.com> Date: Tue, 22 Aug 2023 16:24:21 +0800 Subject: [PATCH 5/5] fix p0 --- regression-test/data/account_p0/test_account.out | 4 ---- regression-test/suites/account_p0/test_account.groovy | 8 +++++++- 2 files changed, 7 insertions(+), 5 deletions(-) delete mode 100644 regression-test/data/account_p0/test_account.out diff --git a/regression-test/data/account_p0/test_account.out b/regression-test/data/account_p0/test_account.out deleted file mode 100644 index ad97022a09090b..00000000000000 --- a/regression-test/data/account_p0/test_account.out +++ /dev/null @@ -1,4 +0,0 @@ --- This file is automatically generated. You should know what you did if you want to edit this --- !show_grants1 -- -'default_cluster:non_existent_user_1'@'%' \N \N \N \N \N \N \N \N \N - diff --git a/regression-test/suites/account_p0/test_account.groovy b/regression-test/suites/account_p0/test_account.groovy index b38d72a80da619..1eb07e259cf493 100644 --- a/regression-test/suites/account_p0/test_account.groovy +++ b/regression-test/suites/account_p0/test_account.groovy @@ -18,5 +18,11 @@ suite("test_account") { // todo: test account management, such as role, user, grant, revoke ... sql "show roles" - qt_show_grants1 "show grants for 'non_existent_user_1'" + try { + sql "show grants for 'non_existent_user_1'" + fail() + } catch (Exception e) { + log.info(e.getMessage()) + assertTrue(e.getMessage().contains('not exist')) + } }