-
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 permission manager #1873
User permission manager #1873
Conversation
Ready to review. come on teachers. thanks. |
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.
Just skim the pr. It looks good to me totally. Very nice~
Addressed dangleptr's comments as below : |
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. The pr looks good to me now.
case Sentence::Kind::kDescribeSpace : { | ||
/** | ||
* Use space and Describe space are special operations. | ||
* Permission checking needs to be done in their executor. |
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.
why? Miss the 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.
why? Miss the spaceId?
Yes, Usually we need to get the space id from the session when check permission.
We can't get the space ID form session when Use Space
or Describe space
, so we need get the space id in their executor.
src/graph/PermissionCheck.cpp
Outdated
case Sentence::Kind::kLimit : | ||
case Sentence::Kind::KGroupBy : | ||
case Sentence::Kind::kReturn : { | ||
return permission::PermissionManager::canReadSchemaData(session); |
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.
Maybe canReadSchemaOrData is a better 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.
Maybe canReadSchemaOrData is a better name.
Good idea. Changed method name from canReadSchemaData
to canReadSchemaOrData
Codecov Report
@@ Coverage Diff @@
## master #1873 +/- ##
==========================================
+ Coverage 86.90% 87.03% +0.13%
==========================================
Files 636 640 +4
Lines 59819 60632 +813
==========================================
+ Hits 51984 52772 +788
- Misses 7835 7860 +25
Continue to review full report at Codecov.
|
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.
} | ||
bool havePermission = false; | ||
switch (session->roleWithSpace(spaceId)) { | ||
case session::Role::GOD : |
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.
What is the difference between session->isGod()
and session::Role::GOD ?
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.
What is the difference between
session->isGod()
and session::Role::GOD ?
session->isGod
just check if the user is god.
session->roleWithSpace(spaceId)
check what role of in this space.
case session::Role::GOD
still there because the compilation requirement of switch block .
* Skip special operations check at here. they are : | ||
* kUse, kDescribeSpace, kRevoke and kGrant. | ||
*/ | ||
if (!PermissionCheck::permissionCheck(session, 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.
What if the query is "use space test; sth else", can we work? I guess it would fail, although this is a quite usual usage.
I suppose we don't need this code block, because we could check it in each executor just like you did.
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.
What if the query is "use space test; sth else", can we work? But this is a quite usual.
Good question. you are right . it's fail when "use space ; sth else" .
because our permission check is in the preparation phase, have not set the space id in the session yet in the preparation phase. so the second statement should be fail.
I originally intended to check permissions in each executor, but this would result in too much code clutter.
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.
ok.
Clean code! |
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.
Great work
src/common/session/Session.h
Outdated
std::string account_; | ||
time::Duration idleDuration_; | ||
/* | ||
* map<space name, role> |
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.
map<space Id, role> ?
} | ||
} | ||
return Role::INVALID_ROLE; | ||
} |
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.
move toRole
to Session.cpp
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.
Great work
…uthorize to PermissionManager;4,skip space check for kDefaultSpaceId when list roles
03a20e0
* user permission manager * Addressed dangleptr's comments * 1,cache user pwd; 2,check god by user name root;3,move FLAGS_enable_authorize to PermissionManager;4,skip space check for kDefaultSpaceId when list roles * Changed the method name from canReadSchemaData to canReadSchemaOrData * fixed comment typo error
1, moved
ClientSession
to common layer . and rename toSession
.2, Added Permission Manager .
3, Added Permission Check.
This PR depend on #1842 ,I will create more test cases after #1842 is done.