From 3fefc7d487ae9dce5a4fb802ff10d8fe2271b76b Mon Sep 17 00:00:00 2001 From: Angerszhuuuu Date: Fri, 27 Oct 2023 10:03:50 +0800 Subject: [PATCH] [KYUUBI #5472][AUTHZ][FOLLOWUP] Check permanent view also need support merge projection ### _Why are the changes needed?_ To close #5472 . Check permanent view also need support merge projection For case `SELECT count(id) FROM $db1.$view1 WHERE id > 10` we only need to check `id`, before this pr, we need to check `id`, `scope`. ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [ ] [Run test](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request ### _Was this patch authored or co-authored using generative AI tooling?_ Closes #5542 from AngersZhuuuu/KYUUBI-5472-FOLLOWUP. Closes #5472 fd63c5ab2 [Bowen Liang] Update extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala 41e487c95 [Angerszhuuuu] Update extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala 4f688c4c1 [Angerszhuuuu] Update PrivilegesBuilder.scala cb9322ce3 [Angerszhuuuu] [KYUUBI #5472][AUTHZ][FOLLOWUP] Check permanent view also need support merge projection Lead-authored-by: Angerszhuuuu Co-authored-by: Bowen Liang Signed-off-by: Cheng Pan --- .../spark/authz/PrivilegesBuilder.scala | 17 +++--- .../ranger/RuleApplyPermanentViewMarker.scala | 3 +- .../authz/util/PermanentViewMarker.scala | 3 +- .../ranger/RangerSparkExtensionSuite.scala | 53 +++++++++++-------- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala b/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala index a0ed5fb6a..0d4b53a5c 100644 --- a/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala +++ b/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala @@ -63,9 +63,19 @@ object PrivilegesBuilder { conditionList: Seq[NamedExpression] = Nil, spark: SparkSession): Unit = { + def getOutputColumnNames(plan: LogicalPlan): Seq[String] = { + plan match { + case pvm: PermanentViewMarker + if pvm.isSubqueryExpressionPlaceHolder || pvm.output.isEmpty => + pvm.visitColNames + case _ => + plan.output.map(_.name) + } + } + def mergeProjection(table: Table, plan: LogicalPlan): Unit = { if (projectionList.isEmpty) { - privilegeObjects += PrivilegeObject(table, plan.output.map(_.name)) + privilegeObjects += PrivilegeObject(table, getOutputColumnNames(plan)) } else { val cols = (projectionList ++ conditionList).flatMap(collectLeaves) .filter(plan.outputSet.contains).map(_.name).distinct @@ -103,11 +113,6 @@ object PrivilegesBuilder { val cols = conditionList ++ aggCols buildQuery(a.child, privilegeObjects, projectionList, cols, spark) - case pvm: PermanentViewMarker => - getScanSpec(pvm).tables(pvm, spark).foreach { table => - privilegeObjects += PrivilegeObject(table, pvm.visitColNames) - } - case scan if isKnownScan(scan) && scan.resolved => getScanSpec(scan).tables(scan, spark).foreach(mergeProjection(_, scan)) diff --git a/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala b/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala index 909cd9e93..f12088f5f 100644 --- a/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala +++ b/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala @@ -43,7 +43,8 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] { PermanentViewMarker( subquery.plan, permanentView.desc, - permanentView.output.map(_.name))) + permanentView.output.map(_.name), + true)) } PermanentViewMarker( resolvedSubquery, diff --git a/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/PermanentViewMarker.scala b/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/PermanentViewMarker.scala index d19f7a923..e997e46f8 100644 --- a/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/PermanentViewMarker.scala +++ b/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/PermanentViewMarker.scala @@ -24,7 +24,8 @@ import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, UnaryNode} case class PermanentViewMarker( child: LogicalPlan, catalogTable: CatalogTable, - visitColNames: Seq[String]) extends UnaryNode + visitColNames: Seq[String], + isSubqueryExpressionPlaceHolder: Boolean = false) extends UnaryNode with WithInternalChild { override def output: Seq[Attribute] = child.output diff --git a/extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala b/extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala index c2e886f02..07c8efeda 100644 --- a/extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala +++ b/extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala @@ -913,31 +913,42 @@ class HiveCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite { |CREATE VIEW $db1.$view2 |AS |SELECT count(*) as cnt, sum(id) as sum_id FROM $db1.$table1 - """.stripMargin)) - val e1 = intercept[AccessControlException]( - doAs(someone, sql(s"SELECT count(*) FROM $db1.$table1").show())) - assert(e1.getMessage.contains( - s"does not have [select] privilege on [$db1/$table1/id,$db1/$table1/scope]")) + """.stripMargin)) + interceptContains[AccessControlException]( + doAs(someone, sql(s"SELECT count(*) FROM $db1.$table1").show()))( + s"does not have [select] privilege on [$db1/$table1/id,$db1/$table1/scope]") - val e2 = intercept[AccessControlException]( - doAs(someone, sql(s"SELECT count(*) FROM $db1.$view1").show())) - assert(e2.getMessage.contains( - s"does not have [select] privilege on [$db1/$view1/id,$db1/$view1/scope]")) + interceptContains[AccessControlException]( + doAs(someone, sql(s"SELECT count(*) FROM $db1.$view1").show()))( + s"does not have [select] privilege on [$db1/$view1/id,$db1/$view1/scope]") - val e3 = intercept[AccessControlException]( - doAs(someone, sql(s"SELECT count(*) FROM $db1.$view2").show())) - assert(e3.getMessage.contains( - s"does not have [select] privilege on [$db1/$view2/cnt,$db1/$view2/sum_id]")) + interceptContains[AccessControlException]( + doAs(someone, sql(s"SELECT count(*) FROM $db1.$view2").show()))( + s"does not have [select] privilege on [$db1/$view2/cnt,$db1/$view2/sum_id]") - val e4 = intercept[AccessControlException]( - doAs(someone, sql(s"SELECT count(*) FROM $db1.$view2 WHERE cnt > 10").show())) - assert(e4.getMessage.contains( - s"does not have [select] privilege on [$db1/$view2/cnt,$db1/$view2/sum_id]")) + interceptContains[AccessControlException]( + doAs(someone, sql(s"SELECT count(id) FROM $db1.$table1 WHERE id > 10").show()))( + s"does not have [select] privilege on [$db1/$table1/id]") - val e5 = intercept[AccessControlException]( - doAs(someone, sql(s"SELECT count(cnt) FROM $db1.$view2 WHERE cnt > 10").show())) - assert(e5.getMessage.contains( - s"does not have [select] privilege on [$db1/$view2/cnt,$db1/$view2/sum_id]")) + interceptContains[AccessControlException]( + doAs(someone, sql(s"SELECT count(id) FROM $db1.$view1 WHERE id > 10").show()))( + s"does not have [select] privilege on [$db1/$view1/id]") + + interceptContains[AccessControlException]( + doAs(someone, sql(s"SELECT count(sum_id) FROM $db1.$view2 WHERE sum_id > 10").show()))( + s"does not have [select] privilege on [$db1/$view2/sum_id]") + + interceptContains[AccessControlException]( + doAs(someone, sql(s"SELECT count(scope) FROM $db1.$table1 WHERE id > 10").show()))( + s"does not have [select] privilege on [$db1/$table1/scope,$db1/$table1/id]") + + interceptContains[AccessControlException]( + doAs(someone, sql(s"SELECT count(scope) FROM $db1.$view1 WHERE id > 10").show()))( + s"does not have [select] privilege on [$db1/$view1/scope,$db1/$view1/id]") + + interceptContains[AccessControlException]( + doAs(someone, sql(s"SELECT count(cnt) FROM $db1.$view2 WHERE sum_id > 10").show()))( + s"does not have [select] privilege on [$db1/$view2/cnt,$db1/$view2/sum_id]") } } }