-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
User manager in meta #400
User manager in meta #400
Conversation
Can one of the admins verify this patch? |
6a91327
to
208dd1a
Compare
Thanks for your response @darionyaphet @dangleptr , I have two questions to discuss as below : |
Address some comments as below: |
3a52188
to
dbf79e2
Compare
About SpaceID and UserID exists multiple places, I've actually thought about it,Because these two structures are used for user management often. So can use these names directly . I think convert |
It make sense for userId, but not for spaceID. We'd better use spaceId to join different entries instead of spaceName. |
By the way, please refer #483 . Put the user related processors under a separate dir. |
|
1,Refactor user processors code structure. |
Ready to review. Please review again. Thanks. |
#ifndef GRAPH_PERMISSIONMANAGER_H | ||
#define GRAPH_PERMISSIONMANAGER_H | ||
|
||
// Operation and permission define: |
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.
Well done
@@ -162,6 +162,18 @@ Status BaseProcessor<RESP>::spaceExist(GraphSpaceID spaceId) { | |||
return Status::SpaceNotFound(); | |||
} | |||
|
|||
template<typename RESP> | |||
Status BaseProcessor<RESP>::userExist(UserID spaceId) { |
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.
Could we unify the userExist with getUserId ?
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.
userExist
is judged by UserID , But getUserId
is judged by userName. And they query for different tables userIndex and userData. They are same with spaceExist and getSpaceId. I think it's best not to unify them. WDYT?
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.
Fair enough.
@@ -41,6 +41,16 @@ void DropSpaceProcessor::process(const cpp2::DropSpaceReq& req) { | |||
deleteKeys.emplace_back(MetaServiceUtils::indexSpaceKey(req.get_space_name())); | |||
deleteKeys.emplace_back(MetaServiceUtils::spaceKey(spaceId)); | |||
|
|||
// delete related role data. |
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.
Now we just delete the parts information when deleting space. We could delete the useless user and schema when compaction.
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.
Yeah, that makes sense. Let me modify it later.
} | ||
|
||
|
||
void AlterUserProcessor::process(const cpp2::AlterUserReq& req) { |
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 seems the alter is to change the whole value.
I want to know what's the language for alter 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.
No, Only change properties that require alter.
Alter user sentences:
ALTER USER account WITH with_user_opt_list
with_user_opt_item
: ACCOUNT LOCK
| ACCOUNT UNLOCK
| MAX_QUERIES_PER_HOUR INTEGER
| MAX_UPDATES_PER_HOUR INTEGER
| MAX_CONNECTIONS_PER_HOUR INTEGER
| MAX_USER_CONNECTIONS INTEGER
;
with_user_opt_list
: with_user_opt_item
| with_user_opt_list COMMA with_user_opt_item
;
|
||
|
||
void ChangePasswordProcessor::process(const cpp2::ChangePasswordReq& req) { | ||
folly::SharedMutex::WriteHolder wHolder(LockUtils::userLock()); |
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.
Change pwd is a special sentence?
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.
Yes, Change password is a separate sentence. No need to verify the password if it is a GOD
role. else the password must be validated. And GOD
can change anyone's password. Other roles can only change their own password.
change_password_sentence
: CHANGE PASSWORD account TO new_password
| CHANGE PASSWORD account FROM old_password TO new_password
;
return; | ||
} | ||
|
||
if (!checkPassword(userRet.value(), req.get_encoded_pwd())) { |
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 want to know when to check the password?
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.
This is a reserved interface,Used for client or terminal connections.
Address laura-ding's comment |
…moved user property email and phone. 3, Added new user property lock status
…irst name and last name. 3, Add new user properties of resource limit. 4, Changed related thrift structure from 'name' to 'id', 5,Add some comments for role.
…authentication_test module
Jenkins go |
Unit testing passed. |
@@ -171,3 +171,33 @@ nebula_link_libraries( | |||
gtest | |||
) | |||
nebula_add_test(meta_http_test) | |||
add_executable( |
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 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.
A stupid mistake,corrected.
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.
Well done
Jenkins go |
Jenkins go |
Unit testing passed. |
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.
Well done
* User manager of Meta Server * 1, Changed user data transport protocol from dataman to thrift. 2, Removed user property email and phone. 3, Added new user property lock status * 1,Refactor user processors code structure. 2,Remove user properties first name and last name. 3, Add new user properties of resource limit. 4, Changed related thrift structure from 'name' to 'id', 5,Add some comments for role. * To improve the drop space , on't delete useless role data * Rebased and added (raftex_obj raftex_thrift_obj wal_obj time_obj) to authentication_test module * Rebased and resolved some conflicts * Address laura-ding's comment * Resolved code alignment
* User manager of Meta Server * 1, Changed user data transport protocol from dataman to thrift. 2, Removed user property email and phone. 3, Added new user property lock status * 1,Refactor user processors code structure. 2,Remove user properties first name and last name. 3, Add new user properties of resource limit. 4, Changed related thrift structure from 'name' to 'id', 5,Add some comments for role. * To improve the drop space , on't delete useless role data * Rebased and added (raftex_obj raftex_thrift_obj wal_obj time_obj) to authentication_test module * Rebased and resolved some conflicts * Address laura-ding's comment * Resolved code alignment
#### What type of PR is this? - [ ] bug - [ ] feature - [x] enhancement #### What does this PR do? The original br tools require password-free ssh to each machine which add extra work for users. This pr's main goal is to remove ssh dependency. To do this, we introduce an agent in each machine. Then br can handle machines' data through agent's service. Agent communicate with the meta service through heartbeat. By heartbeat, agent register itself to meta service and pull the services it should supervise in its host. The agent: vesoft-inc/nebula-agent#1 The br: vesoft-inc/nebula-br#22 This pr includes: 1. refactor br related code, including renaming and adjust code structure. 2. batch the snapshot rpc by spaces 3. add agent heartbeat 4. report data/root path in the storaged/graphd heartbeat #### Which issue(s)/PR(s) this PR relates to? The agent: vesoft-inc/nebula-agent#1 The br: vesoft-inc/nebula-br#22 #### Special notes for your reviewer, ex. impact of this fix, etc: #### Additional context: #### Checklist: - [ ] Documentation affected (Please add the label if documentation needs to be modified.) - [ ] Incompatible (If it is incompatible, please describe it and add corresponding label.) - [ ] Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).) - [ ] Performance impacted: Consumes more CPU/Memory #### Release notes: Please confirm whether to reflect in release notes and how to describe: > ` Migrated from vesoft-inc#3469 Co-authored-by: pengwei.song <90180021+pengweisong@users.noreply.github.com>
Instead of PR #257 .