-
Notifications
You must be signed in to change notification settings - Fork 752
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.
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
[ISSUE-2779] Add user in session #2986
Conversation
Thanks for the contribution! Please review the labels and make any necessary changes. |
@@ -30,6 +30,7 @@ pub struct MutableStatus { | |||
abort: AtomicBool, | |||
current_database: RwLock<String>, | |||
session_settings: RwLock<Settings>, | |||
current_user: RwLock<Option<String>>, |
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.
it will be the UserInfo in the future not only a user name?
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.
I'm taking UserInfo at first but reverted it back to a simple user name, in the current code base UserInfo is a fat object:
pub struct UserInfo {
pub name: String,
pub hostname: String,
pub password: Vec<u8>,
pub auth_type: AuthType,
pub privileges: UserPrivilege,
pub quota: UserQuota,
}
the fields inside UserInfo might get mutated on the fly (like quota, privileges, even password), or mutated by another query instance. a conservative usage of UserInfo might be always fetching UserInfo
from the metasrv, and make it transactional with the user-related operations.
in the case about session privilege, we can construct a unique identifier for the privilege grants by user_name + client_address.
Codecov Report
@@ Coverage Diff @@
## main #2986 +/- ##
=====================================
- Coverage 68% 68% -1%
=====================================
Files 649 649
Lines 34018 34022 +4
=====================================
Hits 23181 23181
- Misses 10837 10841 +4
Continue to review full report at Codecov.
|
@@ -72,6 +74,18 @@ impl MutableStatus { | |||
*lock = db | |||
} | |||
|
|||
// Set the current user after authentication |
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.
It would be nice to add a unit test in test crate we have some MutableStatus tests there
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.
@BohuTANG thank you for remind, i've added a test case on the getter/setter for current user
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.
to test the behaviour of session's current user, i'd split another PR to add a current_user() function
25bdfb4
to
eeeb8cd
Compare
/lgtm |
Wait for another reviewer approval |
I hereby agree to the terms of the CLA available at: https://databend.rs/policies/cla/
Summary
this PR adds a user record in the session, which recorded on the authenticate phase of mysql & clickhouse. it also adds a user field in the
system.processes
table.Changelog
Related Issues
Fixes #2779
Test Plan
Unit Tests
Stateless Tests