-
Notifications
You must be signed in to change notification settings - Fork 762
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add priv check #3710
Add priv check #3710
Conversation
Thanks for the contribution! Please review the labels and make any necessary changes. |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/databend/databend/Hd845vASwdywbeV498TapZB3WvpH [Deployment for 7ad46d1 canceled] |
@@ -205,40 +205,27 @@ impl UserGrantSet { | |||
&self.grants | |||
} | |||
|
|||
pub fn verify_global_privilege( | |||
pub fn verify_privilege( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common/meta/types mainly for type definiton.
Maybe better to put the verify_privilege
in other place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before adding roles, we might need a seperate PrivilegeManager to cache and verify the user privileges (it needs cache all the roles' privileges, and generate an effective grant set with the privileges merged from the roles), maybe the PrivilegeManager could be the better place to verify the privilege, and leaving the types user_grant mod as plain data objects. 🤔
this refactor requires some work, maybe we can make this refactor in a stand-alone PR? q.q
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's OK
a30ab5e
to
73191b8
Compare
73191b8
to
1a2031c
Compare
0d3b311
to
d611772
Compare
@@ -168,6 +170,28 @@ impl Session { | |||
self.mutable_state.set_current_user(user) | |||
} | |||
|
|||
pub fn validate_privilege( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_privilege is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there're some validate_
and check_
functions in our codebase, as the existing convention, the check_
functions often return a bool and used in an if condition, and the validate_
functions often returns an Result<()>, and mostly used as a short-circuit call with a ?
, which breaks the control flow if any error occurred, like:
validate_expression(&expr)?;
57 fn validate_function_arg(
58 name: &str,
59 args_len: usize,
60 variadic_arguments: Option<(usize, usize)>,
61 num_arguments: usize,
62 ) -> Result<()> {
in this case, the validate_privilege
function would return an Result<()> and used as a short-circuit call with a ?
, i guess naming it as validate_
follows the convension in the current codebase? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good 👍
/lgtm |
Codecov Report
@@ Coverage Diff @@
## main #3710 +/- ##
=====================================
Coverage 59% 59%
=====================================
Files 711 711
Lines 38305 38344 +39
=====================================
+ Hits 22878 22905 +27
- Misses 15427 15439 +12
Continue to review full report at Codecov.
|
Wait for another reviewer approval |
I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/
Summary
Summary about this PR
Changelog
Related Issues
Fixes #3170
Test Plan
Unit Tests
Stateless Tests