From 945a8eced941f1557af939718798eb3cd6732783 Mon Sep 17 00:00:00 2001 From: zhangdong <493738387@qq.com> Date: Thu, 24 Aug 2023 23:37:06 +0800 Subject: [PATCH] [fix](auth)Disable column auth temporarily (#23295) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - add config `enable_col_auth` to temporarily disable column permissions(because old/new planner has bug when select from view) - Restore the old optimizer to the previous authentication method - Support for new optimizer authentication（Legacy issue: When querying the view, the permissions of the base table will be authenticated. The view's own permissions should be authenticated and processed after the new optimizer is improved） - fix: show grants for non-existent users - fix: role:`admin` can not grant/revoke to/from user --- .../java/org/apache/doris/common/Config.java | 5 ++ .../org/apache/doris/analysis/GrantStmt.java | 6 +- .../org/apache/doris/analysis/RevokeStmt.java | 2 +- .../org/apache/doris/analysis/SelectStmt.java | 9 +++ .../apache/doris/analysis/ShowGrantsStmt.java | 3 + .../apache/doris/mysql/privilege/Auth.java | 2 +- .../rules/analysis/UserAuthentication.java | 1 + .../apache/doris/planner/OriginalPlanner.java | 73 ------------------- .../doris/datasource/ColumnPrivTest.java | 3 + .../doris/mysql/privilege/AuthTest.java | 2 + .../data/account_p0/test_account.out | 4 - .../suites/account_p0/test_account.groovy | 8 +- .../test_nereids_authentication.groovy | 5 +- 13 files changed, 39 insertions(+), 84 deletions(-) delete mode 100644 regression-test/data/account_p0/test_account.out 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 0f6671b0a02236..35cad6afe2715d 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 @@ -2093,6 +2093,11 @@ public class Config extends ConfigBase { "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; + @ConfField public static boolean forbid_running_alter_job = 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..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 @@ -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; @@ -149,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)); } } @@ -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() || !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/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/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); } 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 f82619fb739f1f..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 @@ -73,4 +73,5 @@ private Plan checkPermission(CatalogRelation relation, ConnectContext connectCon } 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/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; 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 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')) + } } 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')) } }