From 0c8be79ab76608a12bc5cf9f0dee76aed16fb8fb Mon Sep 17 00:00:00 2001 From: Angerszhuuuu Date: Mon, 23 Oct 2023 21:53:17 +0800 Subject: [PATCH] [KYUUBI #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### _Why are the changes needed?_ To fix #5475 In issue #5417 we fixed the problem that AUTHZ will still check scalar-subquery/in-subquery in permanent will. But we just ignore the check, the subquery still will run, in this PR, we record the permanent view's visited column to check the permanent view's privilege to avoid extra execution effort. For the test `[KYUUBI #5417] should not check scalar-subquery in permanent view` I print all the plan that pass to privilege builder as below 截屏2023-10-19 下午4 05 46 before this pr 截屏2023-10-19 下午4 15 29 This two graph shows this pr deny the execution of subquery when we don't have the veiw's privilege ### _How was this patch tested?_ - [ ] 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 #5476 from AngersZhuuuu/KYUUBI-5475. Closes #5475 e1f7920f3 [Angerszhuuuu] Merge branch 'master' into KYUUBI-5475 3bfd9e677 [Angerszhuuuu] update 6b8c0e6e5 [Angerszhuuuu] Merge branch 'master' into KYUUBI-5475 f7585a451 [Angerszhuuuu] Update PrivilegesBuilder.scala faea9c699 [Angerszhuuuu] [KYUUBI #5475] Authz check permanent view's subquery should check view's correct privilege Authored-by: Angerszhuuuu Signed-off-by: Kent Yao --- .../spark/authz/PrivilegesBuilder.scala | 6 +++ .../ranger/RuleApplyPermanentViewMarker.scala | 14 +++--- .../authz/util/PermanentViewMarker.scala | 5 ++- .../ranger/RangerSparkExtensionSuite.scala | 45 +++++++++++++++++++ 4 files changed, 64 insertions(+), 6 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 5c496b8744b..a0ed5fb6a14 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 @@ -28,6 +28,7 @@ import org.apache.kyuubi.plugin.spark.authz.OperationType.OperationType import org.apache.kyuubi.plugin.spark.authz.PrivilegeObjectActionType._ import org.apache.kyuubi.plugin.spark.authz.serde._ import org.apache.kyuubi.plugin.spark.authz.util.AuthZUtils._ +import org.apache.kyuubi.plugin.spark.authz.util.PermanentViewMarker import org.apache.kyuubi.util.reflect.ReflectUtils._ object PrivilegesBuilder { @@ -102,6 +103,11 @@ 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 679b5d65dfe..909cd9e93d3 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 @@ -39,12 +39,16 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] { case permanentView: View if hasResolvedPermanentView(permanentView) => val resolvedSubquery = permanentView.transformAllExpressions { case subquery: SubqueryExpression => - // TODO: Currently, we do not do an auth check in the subquery - // as the main query part also secures it. But for performance consideration, - // we also pre-check it in subqueries and fail fast with negative privileges. - subquery.withNewPlan(plan = PermanentViewMarker(subquery.plan, null)) + subquery.withNewPlan(plan = + PermanentViewMarker( + subquery.plan, + permanentView.desc, + permanentView.output.map(_.name))) } - PermanentViewMarker(resolvedSubquery, resolvedSubquery.desc) + PermanentViewMarker( + resolvedSubquery, + resolvedSubquery.desc, + resolvedSubquery.output.map(_.name)) case other => apply(other) } } 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 69b55e0fc74..d19f7a92314 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 @@ -21,7 +21,10 @@ import org.apache.spark.sql.catalyst.catalog.CatalogTable import org.apache.spark.sql.catalyst.expressions.Attribute import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, UnaryNode} -case class PermanentViewMarker(child: LogicalPlan, catalogTable: CatalogTable) extends UnaryNode +case class PermanentViewMarker( + child: LogicalPlan, + catalogTable: CatalogTable, + visitColNames: Seq[String]) 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 e4e3014f50a..532a114360b 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 @@ -837,6 +837,51 @@ class HiveCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite { } } + test("[KYUUBI #5475] Check permanent view's subquery should check view's correct privilege") { + val db1 = defaultDb + val table1 = "table1" + val table2 = "table2" + val view1 = "view1" + withSingleCallEnabled { + withCleanTmpResources( + Seq((s"$db1.$table1", "table"), (s"$db1.$table2", "table"), (s"$db1.$view1", "view"))) { + doAs(admin, sql(s"CREATE TABLE IF NOT EXISTS $db1.$table1(id int, scope int)")) + doAs( + admin, + sql( + s""" + | CREATE TABLE IF NOT EXISTS $db1.$table2( + | id int, + | name string, + | age int, + | scope int) + | """.stripMargin)) + doAs( + admin, + sql( + s""" + |CREATE VIEW $db1.$view1 + |AS + |WITH temp AS ( + | SELECT max(scope) max_scope + | FROM $db1.$table1) + |SELECT id, name, max(scope) as max_scope, sum(age) sum_age + |FROM $db1.$table2 + |WHERE scope in (SELECT max_scope FROM temp) + |GROUP BY id, name + |""".stripMargin)) + // Will just check permanent view privilege. + val e2 = intercept[AccessControlException]( + doAs( + someone, + sql(s"SELECT id as new_id, name, max_scope FROM $db1.$view1".stripMargin).show())) + assert(e2.getMessage.contains( + s"does not have [select] privilege on " + + s"[$db1/$view1/id,$db1/$view1/name,$db1/$view1/max_scope,$db1/$view1/sum_age]")) + } + } + } + test("[KYUUBI #5492] saveAsTable create DataSource table miss db info") { val table1 = "table1" withSingleCallEnabled {