You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "AngersZhuuuu (via GitHub)" <gi...@apache.org> on 2023/10/13 11:20:37 UTC
[PR] [KYUUBI #5417]Authz will still check source table when persist view contains a subquery [kyuubi]
AngersZhuuuu opened a new pull request, #5418:
URL: https://github.com/apache/kyuubi/pull/5418
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
### _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?_
<!--
If a generative AI tooling has been used in the process of authoring this patch, please include
phrase 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417]Authz will still check source table when persist view contains a subquery [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1358160904
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -747,4 +747,29 @@ 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)"))
+ 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))
+ val e = intercept[AccessControlException](
+ doAs(someone, sql(s"SELECT * FROM $db1.$view1".stripMargin).show()))
+ assert(e.getMessage.contains(s"does not have [select] privilege on [$db1/$view1/new_id]"))
Review Comment:
Without this pr, such case will throw exception
```
"Permission denied: user [someone] does not have [select] privilege on [default/table1/scope]"
```
cc @bowenliang123 @yaooqinn @pan3793
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360177523
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
Do you know what's happening? Tagging the whole sub-tree is a bit hypercritical to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1358410548
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -747,4 +747,29 @@ 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)"))
+ 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))
+ val e = intercept[AccessControlException](
+ doAs(someone, sql(s"SELECT * FROM $db1.$view1".stripMargin).show()))
+ assert(e.getMessage.contains(s"does not have [select] privilege on [$db1/$view1/new_id]"))
Review Comment:
+1. Sounds like the difference is the CTE for the views.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360357986
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,7 +37,11 @@ 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 =>
Review Comment:
```scala
permanentView.transformAllExpressions {
case s: ScalarSubquery => s.withNewPlan(PermanentViewMarker(s.plan, permanentView.desc))
How about we use `PermanentViewMarker` to make it check view privilege? In this case,
- we do not need add a new code-branch in PrivilegesBuilder
- we can fail the subqueries before executing them when view privilege failed
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360087118
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
Since spark will optimize the scalarSubquery's plan, so here we add tag to each level like `KYUUBI_AUTHZ_TAG`.
For `InSubquery` I won't like to support in a separated pr. cc @yaooqinn @bowenliang123
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#issuecomment-1761531694
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/5418?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#5418](https://app.codecov.io/gh/apache/kyuubi/pull/5418?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1119f78) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/1b229b63a0f101d495057cdbee6f81ee8b426006?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1b229b6) will **not change** coverage.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #5418 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 588 588
Lines 33423 33425 +2
Branches 4391 4392 +1
======================================
- Misses 33423 33425 +2
```
| [Files](https://app.codecov.io/gh/apache/kyuubi/pull/5418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/plugin/spark/authz/PrivilegesBuilder.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L1ByaXZpbGVnZXNCdWlsZGVyLnNjYWxh) | `0.00% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360381449
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,7 +37,11 @@ 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 =>
Review Comment:
Can't pass `permanentView.desc`, will request a wrong column. Use `null` is ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#issuecomment-1764201025
Please fix the PR title and description according to the latest updates. Thanks.
LGTM overall.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360478480
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,7 +37,12 @@ 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 =>
+ // Pass null to PermanentViewMarker to avoid check view's ScalarSubquery.
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360098731
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleAuthorization.scala:
##########
@@ -42,6 +42,7 @@ class RuleAuthorization(spark: SparkSession) extends Rule[LogicalPlan] {
object RuleAuthorization {
val KYUUBI_AUTHZ_TAG = TreeNodeTag[Boolean]("__KYUUBI_AUTHZ_TAG")
+ val PERMANNENT_VIEW_SUBQUERY_TAG = TreeNodeTag[Boolean]("__PERMANNENT_VIEW_SUBQUERY_TAG")
Review Comment:
TreeNodeTag[Unit] might be better, using `__KYUUBI` prefix to avoid potential naming contention with spark
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
Is it efficient enough to tag the scalarSubquery.plan only?
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
> I won't like to support in a separated pr?
would like?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#issuecomment-1765596989
Thank you @AngersZhuuuu
Merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360155633
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
> > Is it efficient enough to tag the scalarSubquery.plan only?
>
> Yea, the plan here is not optimized.
What I mean here is `scalarSubquery.plan.setTagValue` only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360217670
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
PlanSubqueries seem to happen in the planner not in the optimizer. Do you mean the execution.ScalarSubquery is already in analyzer instead of a logical one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360381449
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,7 +37,11 @@ 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 =>
Review Comment:
Can't pass `permanentView.desc`, will still request a read privilege. Should use null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360474915
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,7 +37,12 @@ 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 =>
+ // Pass null to PermanentViewMarker to avoid check view's ScalarSubquery.
Review Comment:
Maybe a TODO here
```
// 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360193180
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleAuthorization.scala:
##########
@@ -42,6 +42,7 @@ class RuleAuthorization(spark: SparkSession) extends Rule[LogicalPlan] {
object RuleAuthorization {
val KYUUBI_AUTHZ_TAG = TreeNodeTag[Boolean]("__KYUUBI_AUTHZ_TAG")
+ val PERMANNENT_VIEW_SUBQUERY_TAG = TreeNodeTag[Boolean]("__PERMANNENT_VIEW_SUBQUERY_TAG")
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360087118
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
Since spark will optimize the scalarSubquery's plan, so here we add tag to each level like `KYUUBI_AUTHZ_TAG`.
For `InSubquery` I won't like to support in a separated pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360227095
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
> PlanSubqueries seem to happen in the planner not in the optimizer. Do you mean the execution.ScalarSubquery is already in analyzer instead of a logical one?
<img width="1094" alt="截屏2023-10-16 下午3 28 24" src="https://github.com/apache/kyuubi/assets/46485123/6f440d1c-1664-4e81-b782-b705b3ca48ab">
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
> PlanSubqueries seem to happen in the planner not in the optimizer. Do you mean the execution.ScalarSubquery is already in analyzer instead of a logical one?
<img width="1094" alt="截屏2023-10-16 下午3 28 24" src="https://github.com/apache/kyuubi/assets/46485123/6f440d1c-1664-4e81-b782-b705b3ca48ab">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn closed pull request #5418: [KYUUBI #5417] Authz should not check dependent subquery plan privilege
URL: https://github.com/apache/kyuubi/pull/5418
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360341871
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
@yaooqinn Change to add a new node to do this. Pls take a look again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360357986
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,7 +37,11 @@ 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 =>
Review Comment:
```scala
permanentView.transformAllExpressions {
case s: ScalarSubquery => s.withNewPlan(PermanentViewMarker(s.plan, permanentView.desc))
```
How about we use `PermanentViewMarker` to make it check view privilege? In this case,
- we do not need add a new code-branch in PrivilegesBuilder
- we can fail the subqueries before executing them when view privilege failed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360087118
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
Since spark will optimize the scalarSubquery's plan, so here we add tag to each level like `KYUUBI_AUTHZ_TAG`.
For `InSubquery` I would like to support in a separated pr. cc @yaooqinn @bowenliang123
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
> > I won't like to support in a separated pr?
>
> would like?
Yea
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360169628
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
> > > Is it efficient enough to tag the scalarSubquery.plan only?
> >
> >
> > Yea, the plan here is not optimized.
>
> What I mean here is `scalarSubquery.plan.setTagValue` only
I know, won't work
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360153720
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
> Is it efficient enough to tag the scalarSubquery.plan only?
Yea, the plan here is not optimized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1360196690
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
> Do you know what's happening? Tagging the whole sub-tree is a bit hypercritical to me
If we only tag ScalarSubquery.plan, after generate executedPlan, may loss the top level node, since the `ScalarSubquery.plan` is not optimized.
<img width="1236" alt="截屏2023-10-16 下午2 56 45" src="https://github.com/apache/kyuubi/assets/46485123/a93905ea-8340-4204-8cc5-ddf0844dd487">
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -36,6 +38,15 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
plan mapChildren {
case p: PermanentViewMarker => p
case permanentView: View if hasResolvedPermanentView(permanentView) =>
+ permanentView.transformAllExpressions {
+ case scalarSubquery: ScalarSubquery =>
+ scalarSubquery.plan.transformDown {
+ case p =>
+ p.setTagValue(PERMANNENT_VIEW_SUBQUERY_TAG, true)
+ p
+ }
+ scalarSubquery
+ }
Review Comment:
> Do you know what's happening? Tagging the whole sub-tree is a bit hypercritical to me
If we only tag ScalarSubquery.plan, after generate executedPlan, may loss the top level node, since the `ScalarSubquery.plan` is not optimized.
<img width="1236" alt="截屏2023-10-16 下午2 56 45" src="https://github.com/apache/kyuubi/assets/46485123/a93905ea-8340-4204-8cc5-ddf0844dd487">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#issuecomment-1761581167
I have done this PR https://github.com/apache/kyuubi/pull/3326 last year for skipping shadowed source tables of the permanent view. The `PermanentViewMarker` was introduced with the `RuleApplyPermanentViewMarker` rule to get it done. Proper tests "KYUUBI #3326] check persisted view and skip shadowed table" are testing cases with permanent views.
Could you have a deep look at this and tell the difference before further implementations?
Skipping all the subqueries may be risky and not robust.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5417] Authz should not check dependent subquery plan privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5418:
URL: https://github.com/apache/kyuubi/pull/5418#discussion_r1358345971
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -747,4 +747,29 @@ 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)"))
+ 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))
+ val e = intercept[AccessControlException](
+ doAs(someone, sql(s"SELECT * FROM $db1.$view1".stripMargin).show()))
+ assert(e.getMessage.contains(s"does not have [select] privilege on [$db1/$view1/new_id]"))
Review Comment:
Can we also add a test case for a solid table in a subquery? What's the behavior after this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org