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.

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 privilege manager in graph layer. #502

Closed
wants to merge 4 commits into from

Conversation

boshengchen
Copy link
Contributor

This one depends on PR #400 and PR #480 .
Please hold on this one .

@nebula-community-bot
Copy link
Member

Can one of the admins verify this patch?

Copy link
Contributor

@dutor dutor left a comment

Choose a reason for hiding this comment

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

What a huge diff, but nice code.

I haven't completely gone through this PR. Will back soon.

#define ACL_ALTERUSER (1<<28)
#define ACL_GRANT (1<<29)
#define ACL_REVOKE (1<<30)
#define ACL_CHANGEPASSWORD (1<<31)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend to use enums.

Besides, you have used up all of the 32 bits, while there are definitely new statements in future. So, You should use a 64bits unsigned integer to represent ACLs instead. BTW. 1 ought to be 1UL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enum is better. That makes sense 👍
32 bits is really not enough if a new operation is added,I will change it to 64 bits.

case RoleType::GOD:
// God has all privileges
acl |= (ACL_ADDHOSTS
|ACL_REMOVEHOSTS
Copy link
Contributor

Choose a reason for hiding this comment

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

There ought to be spaces before and after an operator.

}, true);
}

folly::Future<StatusOr<bool>>
Copy link
Contributor

Choose a reason for hiding this comment

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

StatusOr<bool> is identical to Status

cpp2::ListUsersReq req;
return getResponse(std::move(req), [] (auto client, auto request) {
return client->future_listUsers(request);
}, [] (cpp2::ListUsersResp&& resp) -> decltype(auto) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do a research on the difference between auto versus decltype(auto), don't use the latter one unless it's necessary.

@boshengchen boshengchen added in progress do not review PR: not ready for the code review yet labels Jun 26, 2019
@bright-starry-sky
Copy link
Contributor

Close this one . Will track this with #1873 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not review PR: not ready for the code review yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants