From b5ec1e7b7da968c3896d2a15aa384aaf2fe5b36f Mon Sep 17 00:00:00 2001 From: 924060929 <924060929@qq.com> Date: Fri, 23 Feb 2024 13:05:39 +0800 Subject: [PATCH] [fix](Nereids) support check authorization for view but skip check in the view (#31289) move UserAuthentication in BindRelation, support check authorization view but skip check in the view relate pr: #23295 --- .../apache/doris/nereids/CascadesContext.java | 30 +++++-- .../doris/nereids/jobs/executor/Analyzer.java | 7 +- .../nereids/rules/analysis/BindRelation.java | 18 +++- .../rules/analysis/UserAuthentication.java | 33 ++------ .../doris/external/hms/HmsCatalogTest.java | 20 +++++ .../apache/doris/qe/HmsQueryCacheTest.java | 16 ++++ .../account_p0/test_nereids_row_policy.groovy | 9 +- .../authorization/view_authorization.groovy | 82 +++++++++++++++++++ 8 files changed, 171 insertions(+), 44 deletions(-) create mode 100644 regression-test/suites/nereids_p0/authorization/view_authorization.groovy diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/CascadesContext.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/CascadesContext.java index fecdbf650c3d73..7cd486b47f799b 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/CascadesContext.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/CascadesContext.java @@ -119,6 +119,7 @@ public class CascadesContext implements ScheduleContext { private boolean isLeadingJoin = false; private final Map hintMap = Maps.newLinkedHashMap(); + private final boolean shouldCheckRelationAuthentication; /** * Constructor of OptimizerContext. @@ -128,7 +129,7 @@ public class CascadesContext implements ScheduleContext { */ private CascadesContext(Optional parent, Optional currentTree, StatementContext statementContext, Plan plan, Memo memo, - CTEContext cteContext, PhysicalProperties requireProperties) { + CTEContext cteContext, PhysicalProperties requireProperties, boolean shouldCheckRelationAuthentication) { this.parent = Objects.requireNonNull(parent, "parent should not null"); this.currentTree = Objects.requireNonNull(currentTree, "currentTree should not null"); this.statementContext = Objects.requireNonNull(statementContext, "statementContext should not null"); @@ -142,6 +143,7 @@ private CascadesContext(Optional parent, Optional curren this.subqueryExprIsAnalyzed = new HashMap<>(); this.runtimeFilterContext = new RuntimeFilterContext(getConnectContext().getSessionVariable()); this.materializationContexts = new ArrayList<>(); + this.shouldCheckRelationAuthentication = shouldCheckRelationAuthentication; } /** @@ -150,7 +152,13 @@ private CascadesContext(Optional parent, Optional curren public static CascadesContext initContext(StatementContext statementContext, Plan initPlan, PhysicalProperties requireProperties) { return newContext(Optional.empty(), Optional.empty(), statementContext, - initPlan, new CTEContext(), requireProperties); + initPlan, new CTEContext(), requireProperties, true); + } + + public static CascadesContext initViewContext(StatementContext statementContext, + Plan initPlan, PhysicalProperties requireProperties) { + return newContext(Optional.empty(), Optional.empty(), statementContext, + initPlan, new CTEContext(), requireProperties, false); } /** @@ -159,13 +167,14 @@ public static CascadesContext initContext(StatementContext statementContext, public static CascadesContext newContextWithCteContext(CascadesContext cascadesContext, Plan initPlan, CTEContext cteContext) { return newContext(Optional.of(cascadesContext), Optional.empty(), - cascadesContext.getStatementContext(), initPlan, cteContext, PhysicalProperties.ANY); + cascadesContext.getStatementContext(), initPlan, cteContext, PhysicalProperties.ANY, + cascadesContext.shouldCheckRelationAuthentication); } public static CascadesContext newCurrentTreeContext(CascadesContext context) { return CascadesContext.newContext(context.getParent(), context.getCurrentTree(), context.getStatementContext(), context.getRewritePlan(), context.getCteContext(), - context.getCurrentJobContext().getRequiredProperties()); + context.getCurrentJobContext().getRequiredProperties(), context.shouldCheckRelationAuthentication); } /** @@ -174,13 +183,14 @@ public static CascadesContext newCurrentTreeContext(CascadesContext context) { public static CascadesContext newSubtreeContext(Optional subtree, CascadesContext context, Plan plan, PhysicalProperties requireProperties) { return CascadesContext.newContext(Optional.of(context), subtree, context.getStatementContext(), - plan, context.getCteContext(), requireProperties); + plan, context.getCteContext(), requireProperties, context.shouldCheckRelationAuthentication); } private static CascadesContext newContext(Optional parent, Optional subtree, - StatementContext statementContext, Plan initPlan, - CTEContext cteContext, PhysicalProperties requireProperties) { - return new CascadesContext(parent, subtree, statementContext, initPlan, null, cteContext, requireProperties); + StatementContext statementContext, Plan initPlan, CTEContext cteContext, + PhysicalProperties requireProperties, boolean shouldCheckRelationAuthentication) { + return new CascadesContext(parent, subtree, statementContext, initPlan, null, + cteContext, requireProperties, shouldCheckRelationAuthentication); } public CascadesContext getRoot() { @@ -636,6 +646,10 @@ public void setLeadingJoin(boolean leadingJoin) { isLeadingJoin = leadingJoin; } + public boolean shouldCheckRelationAuthentication() { + return shouldCheckRelationAuthentication; + } + public Map getHintMap() { return hintMap; } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Analyzer.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Analyzer.java index 2f3e0ccfe2f924..d1e38d014e6e8e 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Analyzer.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Analyzer.java @@ -44,7 +44,6 @@ import org.apache.doris.nereids.rules.analysis.ReplaceExpressionByChildOutput; import org.apache.doris.nereids.rules.analysis.ResolveOrdinalInOrderByAndGroupBy; import org.apache.doris.nereids.rules.analysis.SubqueryToApply; -import org.apache.doris.nereids.rules.analysis.UserAuthentication; import org.apache.doris.nereids.rules.rewrite.MergeProjects; import org.apache.doris.nereids.rules.rewrite.SemiJoinCommute; @@ -119,8 +118,7 @@ private static List buildAnalyzeViewJobs(Optional buildAnalyzeJobs(Optional c topDown(new EliminateLogicalSelectHint()), bottomUp( new BindRelation(customTableResolver), - new CheckPolicy(), - new UserAuthentication() + new CheckPolicy() ), bottomUp(new BindExpression()), bottomUp(new BindSlotWithPaths()), diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java index 9d9591e34b19ae..54fa965e0eba3a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java @@ -157,7 +157,7 @@ private LogicalPlan bindWithCurrentDb(CascadesContext cascadesContext, UnboundRe } // TODO: should generate different Scan sub class according to table's type - LogicalPlan scan = getLogicalPlan(table, unboundRelation, tableQualifier, cascadesContext); + LogicalPlan scan = getAndCheckLogicalPlan(table, unboundRelation, tableQualifier, cascadesContext); if (cascadesContext.isLeadingJoin()) { LeadingHint leading = (LeadingHint) cascadesContext.getHintMap().get("Leading"); leading.putRelationIdAndTableName(Pair.of(unboundRelation.getRelationId(), tableName)); @@ -178,7 +178,7 @@ private LogicalPlan bind(CascadesContext cascadesContext, UnboundRelation unboun if (table == null) { table = RelationUtil.getTable(tableQualifier, cascadesContext.getConnectContext().getEnv()); } - return getLogicalPlan(table, unboundRelation, tableQualifier, cascadesContext); + return getAndCheckLogicalPlan(table, unboundRelation, tableQualifier, cascadesContext); } private LogicalPlan makeOlapScan(TableIf table, UnboundRelation unboundRelation, List tableQualifier) { @@ -234,7 +234,17 @@ private LogicalPlan makeOlapScan(TableIf table, UnboundRelation unboundRelation, return scan; } - private LogicalPlan getLogicalPlan(TableIf table, UnboundRelation unboundRelation, List tableQualifier, + private LogicalPlan getAndCheckLogicalPlan(TableIf table, UnboundRelation unboundRelation, + List tableQualifier, CascadesContext cascadesContext) { + // if current context is in the view, we can skip check authentication because + // the view already checked authentication + if (cascadesContext.shouldCheckRelationAuthentication()) { + UserAuthentication.checkPermission(table, cascadesContext.getConnectContext()); + } + return doGetLogicalPlan(table, unboundRelation, tableQualifier, cascadesContext); + } + + private LogicalPlan doGetLogicalPlan(TableIf table, UnboundRelation unboundRelation, List tableQualifier, CascadesContext cascadesContext) { switch (table.getType()) { case OLAP: @@ -289,7 +299,7 @@ private Plan parseAndAnalyzeView(String ddlSql, CascadesContext parentContext) { if (parsedViewPlan instanceof UnboundResultSink) { parsedViewPlan = (LogicalPlan) ((UnboundResultSink) parsedViewPlan).child(); } - CascadesContext viewContext = CascadesContext.initContext( + CascadesContext viewContext = CascadesContext.initViewContext( parentContext.getStatementContext(), parsedViewPlan, PhysicalProperties.ANY); viewContext.newAnalyzer(true, customTableResolver).analyze(); // we should remove all group expression of the plan which in other memo, so the groupId would not conflict 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 86fb64d6e56eaf..ffed8d4ea93c80 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 @@ -23,44 +23,31 @@ 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; -import org.apache.doris.nereids.rules.RuleType; -import org.apache.doris.nereids.trees.plans.Plan; -import org.apache.doris.nereids.trees.plans.algebra.CatalogRelation; import org.apache.doris.qe.ConnectContext; /** * Check whether a user is permitted to scan specific tables. */ -public class UserAuthentication extends OneAnalysisRuleFactory { - - @Override - public Rule build() { - return logicalRelation() - .when(CatalogRelation.class::isInstance) - .thenApply(ctx -> checkPermission((CatalogRelation) ctx.root, ctx.connectContext)) - .toRule(RuleType.RELATION_AUTHENTICATION); - } - - private Plan checkPermission(CatalogRelation relation, ConnectContext connectContext) { +public class UserAuthentication { + /** checkPermission. */ + public static void checkPermission(TableIf table, ConnectContext connectContext) { + if (table == null) { + return; + } // do not check priv when replaying dump file if (connectContext.getSessionVariable().isPlayNereidsDump()) { - return null; - } - TableIf table = relation.getTable(); - if (table == null) { - return null; + return; } String tableName = table.getName(); DatabaseIf db = table.getDatabase(); // when table inatanceof FunctionGenTable,db will be null if (db == null) { - return null; + return; } String dbName = db.getFullName(); CatalogIf catalog = db.getCatalog(); if (catalog == null) { - return null; + return; } String ctlName = catalog.getName(); // TODO: 2023/7/19 checkColumnsPriv @@ -71,7 +58,5 @@ private Plan checkPermission(CatalogRelation relation, ConnectContext connectCon ctlName + ": " + dbName + ": " + tableName); throw new AnalysisException(message); } - return null; } - } diff --git a/fe/fe-core/src/test/java/org/apache/doris/external/hms/HmsCatalogTest.java b/fe/fe-core/src/test/java/org/apache/doris/external/hms/HmsCatalogTest.java index 220813a305c8c6..e90e1f8889f019 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/external/hms/HmsCatalogTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/external/hms/HmsCatalogTest.java @@ -127,6 +127,10 @@ private void createDbAndTableForHmsCatalog(HMSExternalCatalog hmsCatalog) { tbl.getType(); minTimes = 0; result = TableIf.TableType.HMS_EXTERNAL_TABLE; + + tbl.getDatabase(); + minTimes = 0; + result = db; } }; @@ -169,6 +173,10 @@ private void createDbAndTableForHmsCatalog(HMSExternalCatalog hmsCatalog) { view1.isSupportedHmsTable(); minTimes = 0; result = true; + + view1.getDatabase(); + minTimes = 0; + result = db; } }; @@ -211,6 +219,10 @@ private void createDbAndTableForHmsCatalog(HMSExternalCatalog hmsCatalog) { view2.isSupportedHmsTable(); minTimes = 0; result = true; + + view2.getDatabase(); + minTimes = 0; + result = db; } }; @@ -253,6 +265,10 @@ private void createDbAndTableForHmsCatalog(HMSExternalCatalog hmsCatalog) { view3.isSupportedHmsTable(); minTimes = 0; result = true; + + view3.getDatabase(); + minTimes = 0; + result = db; } }; @@ -295,6 +311,10 @@ private void createDbAndTableForHmsCatalog(HMSExternalCatalog hmsCatalog) { view4.isSupportedHmsTable(); minTimes = 0; result = true; + + view4.getDatabase(); + minTimes = 0; + result = db; } }; diff --git a/fe/fe-core/src/test/java/org/apache/doris/qe/HmsQueryCacheTest.java b/fe/fe-core/src/test/java/org/apache/doris/qe/HmsQueryCacheTest.java index 1c066706947478..14ba32d61be329 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/qe/HmsQueryCacheTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/qe/HmsQueryCacheTest.java @@ -159,6 +159,10 @@ private void init(HMSExternalCatalog hmsCatalog) { // mock initSchemaAndUpdateTime and do nothing tbl.initSchemaAndUpdateTime(); minTimes = 0; + + tbl.getDatabase(); + minTimes = 0; + result = db; } }; @@ -203,6 +207,10 @@ private void init(HMSExternalCatalog hmsCatalog) { // mock initSchemaAndUpdateTime and do nothing tbl2.initSchemaAndUpdateTime(); minTimes = 0; + + tbl2.getDatabase(); + minTimes = 0; + result = db; } }; @@ -254,6 +262,10 @@ private void init(HMSExternalCatalog hmsCatalog) { view1.getUpdateTime(); minTimes = 0; result = NOW; + + view1.getDatabase(); + minTimes = 0; + result = db; } }; @@ -304,6 +316,10 @@ private void init(HMSExternalCatalog hmsCatalog) { view2.getUpdateTime(); minTimes = 0; result = NOW; + + view2.getDatabase(); + minTimes = 0; + result = db; } }; diff --git a/regression-test/suites/account_p0/test_nereids_row_policy.groovy b/regression-test/suites/account_p0/test_nereids_row_policy.groovy index d12b11261d842f..a803e4bcda428e 100644 --- a/regression-test/suites/account_p0/test_nereids_row_policy.groovy +++ b/regression-test/suites/account_p0/test_nereids_row_policy.groovy @@ -32,14 +32,16 @@ suite("test_nereids_row_policy") { sql "set enable_fallback_to_original_planner = false" sql "SELECT * FROM ${tableName}" } - def result3 = connect(user=user, password='123abc!@#', url=url) { + connect(user=user, password='123abc!@#', url=url) { sql "set enable_nereids_planner = true" sql "set enable_fallback_to_original_planner = false" - sql "SELECT * FROM ${viewName}" + test { + sql "SELECT * FROM ${viewName}" + exception "SELECT command denied to user" + } } assertEquals(size, result1.size()) assertEquals(size, result2.size()) - assertEquals(size, result3.size()) } def createPolicy = { name, predicate, type -> @@ -79,6 +81,7 @@ suite("test_nereids_row_policy") { sql "CREATE USER ${user} IDENTIFIED BY '123abc!@#'" sql "GRANT SELECT_PRIV ON internal.${dbName}.${tableName} TO ${user}" + sql 'sync' // no policy assertQueryResult 3 diff --git a/regression-test/suites/nereids_p0/authorization/view_authorization.groovy b/regression-test/suites/nereids_p0/authorization/view_authorization.groovy new file mode 100644 index 00000000000000..672cd680ec8d4a --- /dev/null +++ b/regression-test/suites/nereids_p0/authorization/view_authorization.groovy @@ -0,0 +1,82 @@ +// 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("view_authorization") { + def db = context.config.getDbNameByFile(context.file) + def user1 = "test_view_auth_user1" + def baseTable = "test_view_auth_base_table" + def view1 = "test_view_auth_view1" + def view2 = "test_view_auth_view2" + def view3 = "test_view_auth_view3" + + + sql "drop table if exists ${baseTable}" + sql "drop view if exists ${view1}" + sql "drop view if exists ${view2}" + sql "drop view if exists ${view3}" + sql "drop user if exists ${user1}" + + sql """ + CREATE TABLE ${baseTable} (id INT, name TEXT) + DISTRIBUTED BY HASH(`id`) + PROPERTIES ( + "replication_allocation" = "tag.location.default: 1" + ); + """ + + sql "insert into ${baseTable} values(1, 'hello'), (2, 'world'), (3, 'doris');" + sql "create view ${view1} as select *, concat(name, '_', id) from ${db}.${baseTable} where id=1;" + sql "create view ${view2} as select *, concat(name, '_', id) as xxx from ${db}.${baseTable} where id != 1;" + sql "create view ${view3} as select xxx, 100 from ${db}.${view2} where id=3" + + sql "create user ${user1}" + sql "grant SELECT_PRIV on ${db}.${view1} to '${user1}'@'%';" + sql "grant SELECT_PRIV on ${db}.${view3} to '${user1}'@'%';" + + sql 'sync' + + def defaultDbUrl = context.config.jdbcUrl.substring(0, context.config.jdbcUrl.lastIndexOf("/")) + logger.info("connect to ${defaultDbUrl}".toString()) + connect(user = user1, password = null, url = defaultDbUrl) { + sql "set enable_fallback_to_original_planner=false" + + // no privilege to base table + test { + sql "select * from ${db}.${baseTable}" + exception "SELECT command denied to user" + } + + // has privilege to view1 + test { + sql "select * from ${db}.${view1}" + result([[1, 'hello', 'hello_1']]) + } + + // no privilege to view2 + test { + sql "select * from ${db}.${view2}" + exception "SELECT command denied to user" + } + + // nested view + // has privilege to view3 + test { + sql "select * from ${db}.${view3}" + result([['doris_3', 100]]) + } + } +}