[KYUUBI #5417] Authz should not check dependent subquery plan privilege

### _Why are the changes needed?_ Fix #5417 If there is is a view with subquery, authz will still request this subquery's interval privilege, it's not we want. For view ``` CREATE VIEW db.view1 AS WITH temp AS ( SELECT max(scope) max_scope FROM db1.table1) SELECT id as new_id FROM db1.table2 WHERE scope = (SELECT max_scope FROM temp) ``` When we query the view ``` SEELCT * FROM db.view1 ``` Before this pr, since spark will first execute subquery, it will first request `[default/table1/scope]` then request `[default/view1/new_id]` after this pr, it only request `[default/view1/new_id]` ### _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?_ No Closes #5418 from AngersZhuuuu/KYUUBI-5417. Closes #5417 e2669fa [Angerszhuuuu] Update tableExtractors.scala bc72cfc [Angerszhuuuu] Update RuleApplyPermanentViewMarker.scala 1731b93 [Angerszhuuuu] Update RuleEliminateViewMarker.scala 282999e [Angerszhuuuu] follow comment 6b37aaa [Angerszhuuuu] Update RuleApplyPermanentViewMarker.scala d03354d [Angerszhuuuu] Revert "update" 7a96627 [Angerszhuuuu] update 78a32b3 [Angerszhuuuu] follow comment 79e07ab [Angerszhuuuu] Update PrivilegesBuilder.scala 518c2b3 [Angerszhuuuu] update d033624 [Angerszhuuuu] update 54ff954 [Angerszhuuuu] update. 1119f78 [Angerszhuuuu] Update RangerSparkExtensionSuite.scala 1918381 [Angerszhuuuu] Add UT 7723f90 [Angerszhuuuu] [KYUUBI #5417]Authz will still check source table when persist view contains a subquery Authored-by: Angerszhuuuu <[email protected]> Signed-off-by: Kent Yao <[email protected]>
apache · Oct 17, 2023 · f75e4ac · f75e4ac
1 parent c60f5b7
commit f75e4ac
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 6 deletions.
diff --git a/...main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala b/...main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala
@@ -17,6 +17,7 @@
 
 package org.apache.kyuubi.plugin.spark.authz.ranger
 
+import org.apache.spark.sql.catalyst.expressions.ScalarSubquery
 import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, View}
 import org.apache.spark.sql.catalyst.rules.Rule
 
@@ -36,7 +37,14 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
     plan mapChildren {
       case p: PermanentViewMarker => p
       case permanentView: View if hasResolvedPermanentView(permanentView) =>
-        PermanentViewMarker(permanentView, permanentView.desc)
+        val resolvedSubquery = permanentView.transformAllExpressions {
+          case scalarSubquery: ScalarSubquery =>
+            // 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.
+            scalarSubquery.copy(plan = PermanentViewMarker(scalarSubquery.plan, null))
+        }
+        PermanentViewMarker(resolvedSubquery, resolvedSubquery.desc)
       case other => apply(other)
     }
   }

diff --git a/...ark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala b/...ark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala
@@ -91,10 +91,14 @@ class TableIdentifierTableExtractor extends TableExtractor {
  */
 class CatalogTableTableExtractor extends TableExtractor {
   override def apply(spark: SparkSession, v1: AnyRef): Option[Table] = {
-    val catalogTable = v1.asInstanceOf[CatalogTable]
-    val identifier = catalogTable.identifier
-    val owner = Option(catalogTable.owner).filter(_.nonEmpty)
-    Some(Table(None, identifier.database, identifier.table, owner))
+    if (null == v1) {
+      None
+    } else {
+      val catalogTable = v1.asInstanceOf[CatalogTable]
+      val identifier = catalogTable.identifier
+      val owner = Option(catalogTable.owner).filter(_.nonEmpty)
+      Some(Table(None, identifier.database, identifier.table, owner))
+    }
   }
 }
 

diff --git a/...hz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/RuleEliminateViewMarker.scala b/...hz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/RuleEliminateViewMarker.scala
@@ -17,6 +17,7 @@
 
 package org.apache.kyuubi.plugin.spark.authz.util
 
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
 import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
 import org.apache.spark.sql.catalyst.rules.Rule
 
@@ -25,6 +26,10 @@ import org.apache.spark.sql.catalyst.rules.Rule
  */
 class RuleEliminateViewMarker extends Rule[LogicalPlan] {
   override def apply(plan: LogicalPlan): LogicalPlan = {
-    plan.transformUp { case pvm: PermanentViewMarker => pvm.child }
+    plan.transformUp {
+      case pvm: PermanentViewMarker => pvm.child.transformAllExpressions {
+          case s: SubqueryExpression => s.withNewPlan(apply(s.plan))
+        }
+    }
   }
 }
diff --git a/...rc/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala b/...rc/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala
@@ -747,4 +747,48 @@ class HiveCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite {
       assert(e.getMessage.contains(s"does not have [select] privilege on [$db1/$table/id]"))
     }
   }
+
+  test("[KYUUBI #5417] should not check dependent subquery plan privilege") {
+    val db1 = defaultDb
+    val table1 = "table1"
+    val table2 = "table2"
+    val view1 = "view1"
+    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, scope int)"))
+
+      val e1 = intercept[AccessControlException] {
+        doAs(
+          someone,
+          sql(
+            s"""
+               |WITH temp AS (
+               |    SELECT max(scope) max_scope
+               |    FROM $db1.$table1)
+               |SELECT id as new_id FROM $db1.$table2
+               |WHERE scope = (SELECT max_scope FROM temp)
+               |""".stripMargin).show())
+      }
+      // Will first check subquery privilege.
+      assert(e1.getMessage.contains(s"does not have [select] privilege on [$db1/$table1/scope]"))
+
+      doAs(
+        admin,
+        sql(
+          s"""
+             |CREATE VIEW $db1.$view1
+             |AS
+             |WITH temp AS (
+             |    SELECT max(scope) max_scope
+             |    FROM $db1.$table1)
+             |SELECT id as new_id FROM $db1.$table2
+             |WHERE scope = (SELECT max_scope FROM temp)
+             |""".stripMargin))
+      // Will just check permanent view privilege.
+      val e2 = intercept[AccessControlException](
+        doAs(someone, sql(s"SELECT * FROM $db1.$view1".stripMargin).show()))
+      assert(e2.getMessage.contains(s"does not have [select] privilege on [$db1/$view1/new_id]"))
+    }
+  }
 }