Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[KYUUBI #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege #5476

Closed
wants to merge 5 commits into from
Closed

[KYUUBI #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege #5476

wants to merge 5 commits into from

Conversation

AngersZhuuuu
Copy link
Contributor

@AngersZhuuuu AngersZhuuuu commented Oct 19, 2023 •
edited by yaooqinn
Loading

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 locally before make a pull request

Was this patch authored or co-authored using generative AI tooling?

@AngersZhuuuu
Copy link
Contributor Author

ping @yaooqinn

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2023 •
edited
Loading

Codecov Report

Merging #5476 (f7585a4) into master (03d6223) will not change coverage.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

❗ Current head f7585a4 differs from pull request most recent head e1f7920. Consider uploading reports for the commit e1f7920 to get more accurate results

@@          Coverage Diff           @@
##           master   #5476   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         588     588           
  Lines       33480   33479    -1     
  Branches     4405    4402    -3     
======================================
+ Misses      33480   33479    -1
Files Coverage Δ
.../kyuubi/plugin/spark/authz/PrivilegesBuilder.scala 0.00% <0.00%> (ø)
.../plugin/spark/authz/util/PermanentViewMarker.scala 0.00% <0.00%> (ø)
...rk/authz/ranger/RuleApplyPermanentViewMarker.scala 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cxzl25
Copy link
Contributor

cxzl25 commented Oct 19, 2023

Incomplete image link in PR description.

@AngersZhuuuu
Copy link
Contributor Author

Incomplete image link in PR description.

Yea, changed

@AngersZhuuuu
Copy link
Contributor Author

ping @yaooqinn @bowenliang123 could you table a look?

@bowenliang123 bowenliang123 changed the title [KYUUBI #5475] Authz check permanent view's subquery should check view's correct privilege [KYUUBI #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege Oct 21, 2023
@AngersZhuuuu
Copy link
Contributor Author

Any suggestion?

@yaooqinn
Copy link
Member

Can we add some tests for it?

@AngersZhuuuu
Copy link
Contributor Author

Can we add some tests for it?

Existing ut already verified this? Since we skip check the subquery expression's privilege check

@yaooqinn
Copy link
Member

yaooqinn commented Oct 23, 2023 •
edited
Loading

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

@AngersZhuuuu
Copy link
Contributor Author

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?

bowenliang123
bowenliang123 approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

yaooqinn
yaooqinn reviewed Oct 23, 2023
View reviewed changes
...rc/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala
}
PermanentViewMarker(resolvedSubquery, resolvedSubquery.desc)
PermanentViewMarker(
resolvedSubquery,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's not resolvedSubquery but a view?

@yaooqinn yaooqinn added this to the v1.9.0 milestone Oct 23, 2023
@yaooqinn yaooqinn closed this in 9e2fcc3 Oct 23, 2023
@yaooqinn
Copy link
Member

Thanks, merged to master

davidyuan1223 pushed a commit to davidyuan1223/kyuubi that referenced this pull request Oct 26, 2023
@AngersZhuuuu @davidyuan1223
[KYUUBI apache#5475][FOLLOWUP] Authz check permanent view's subquery …
0c8be79 
…should check view's correct privilege

### _Why are the changes needed?_
To fix apache#5475

In issue apache#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 apache#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 apache#5476 from AngersZhuuuu/KYUUBI-5475.

Closes apache#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 apache#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaooqinn yaooqinn yaooqinn approved these changes

@bowenliang123 bowenliang123 bowenliang123 approved these changes

Assignees

@AngersZhuuuu AngersZhuuuu

Labels
module:authz module:extensions module:spark
Projects
None yet
Milestone
v1.9.0
Development

Successfully merging this pull request may close these issues.

[TASK][EASY] Authz check permanent view's subquery should check view's correct privilege
5 participants
@AngersZhuuuu @codecov-commenter @cxzl25 @yaooqinn @bowenliang123