[KYUUBI #5475][FOLLOWUP] Authz check permanent view's subquery should…

… check view's correct privilege ### _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 <img width="1398" alt="截屏2023-10-19 下午4 05 46" src="https://github.com/apache/kyuubi/assets/46485123/b136bb47-816c-4066-aba7-a74cbe323f7d"> before this pr <img width="1310" alt="截屏2023-10-19 下午4 15 29" src="https://github.com/apache/kyuubi/assets/46485123/aa2e3cfe-bca7-493d-a364-b2c196c76c3a"> 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 e1f7920 [Angerszhuuuu] Merge branch 'master' into KYUUBI-5475 3bfd9e6 [Angerszhuuuu] update 6b8c0e6 [Angerszhuuuu] Merge branch 'master' into KYUUBI-5475 f7585a4 [Angerszhuuuu] Update PrivilegesBuilder.scala faea9c6 [Angerszhuuuu] [KYUUBI #5475] Authz check permanent view's subquery should check view's correct privilege Authored-by: Angerszhuuuu <angers.zhu@gmail.com> Signed-off-by: Kent Yao <yao@apache.org>
apache · Oct 26, 2023 · 0c8be79 · 0c8be79
1 parent 1293cf2
commit 0c8be79
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 6 deletions.
diff --git a/...i-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala b/...i-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/...main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala b/...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/...-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/PermanentViewMarker.scala b/...-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/...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
@@ -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 {