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/19 08:12:08 UTC
[PR] [KYUUBI #5475] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
AngersZhuuuu opened a new pull request, #5476:
URL: https://github.com/apache/kyuubi/pull/5476
### _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.
### _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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1774720539
The existing UT seems insufficient.
Let's update the test at least with `create view v1 as select id, name, max(age) as max_age from xxxx....` to test
--
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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn closed pull request #5476: [KYUUBI #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege
URL: https://github.com/apache/kyuubi/pull/5476
--
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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1774356805
Any suggestion?
--
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 #5475] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1770488056
Incomplete image link in PR description.
--
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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1775257825
Thanks, 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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1774659535
Can we add some tests for it?
--
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 #5475] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1770294305
ping @yaooqinn
--
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 #5475] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1772950612
ping @yaooqinn @bowenliang123 could you table a look?
--
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 #5475] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1770497641
> Incomplete image link in PR description.
Yea, changed
--
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 #5475] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1770463591
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/5476?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#5476](https://app.codecov.io/gh/apache/kyuubi/pull/5476?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f7585a4) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/48bdc7d4cb9023a5de6bef2a10f3ea7512f1940d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (48bdc7d) will **not change** coverage.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #5476 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 588 588
Lines 33466 33479 +13
Branches 4401 4402 +1
======================================
- Misses 33466 33479 +13
```
| [Files](https://app.codecov.io/gh/apache/kyuubi/pull/5476?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/5476?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%> (ø)` | |
| [.../plugin/spark/authz/util/PermanentViewMarker.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3V0aWwvUGVybWFuZW50Vmlld01hcmtlci5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...rk/authz/ranger/RuleApplyPermanentViewMarker.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3Jhbmdlci9SdWxlQXBwbHlQZXJtYW5lbnRWaWV3TWFya2VyLnNjYWxh) | `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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1774663966
> Can we add some tests for it?
Existing ut already verified this? Since we skip check the subquery expression's privilege check
--
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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#discussion_r1368421937
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -42,9 +42,16 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
// TODO: Currently, we do not do an auth check in the subquery
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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#issuecomment-1774845816
> The existing UT seems insufficient as it only has one col.
>
> Let's update the test at least with `create view v1 as select id, name, max(age) as max_age from xxxx....` to test
How about current?
--
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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#discussion_r1368701301
##########
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,
Review Comment:
nit: it's not resolvedSubquery but a view?
--
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 #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #5476:
URL: https://github.com/apache/kyuubi/pull/5476#discussion_r1368324559
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -42,9 +42,16 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
// TODO: Currently, we do not do an auth check in the subquery
Review Comment:
Comments overdue？
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala:
##########
@@ -42,9 +42,16 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
// TODO: Currently, we do not do an auth check in the subquery
Review Comment:
Comments overdue？
--
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