-
Notifications
You must be signed in to change notification settings - Fork 1k
RANGER-4771: Remove the calls to ensureAdminAccess() in grantAccess()… #703
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
Conversation
boolean isAdmin = bizUtil.isAdmin(); | ||
boolean isKeyAdmin = bizUtil.isKeyAdmin(); | ||
String userName = bizUtil.getCurrentUserLoginId(); | ||
String userName = bizUtil.getCurrentUserLoginId() != null ? bizUtil.getCurrentUserLoginId() : grantor; |
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.
When grantor
is provided, that value should be used as userName
, instead of bizUtil.getCurrentUserLoginId()
. Also, shouldn't isAdmin
and isKeyAdmin
also be derived from grantor
?
final String userName;
final boolean isAdmin;
final boolean isKeyAdmin;
if (StringUtils.isEmpty(grantor)) { // use currently logged-in user
userName = bizUtil.getCurrentUserLoginId();
isAdmin = bizUtil.isAdmin();
isKeyAdmin = bizUtil.isKeyAdmin();
} else {
// find role of the given grantor; logic from SessionMgr.setUserRoles(userSession)
Collection<String> userRoles = userMgr.getRolesByLoginId(grantor); // add @Autowired UserMgr userMgr;
userName = grantor;
isAdmin = userRoles.contains(RangerConstants.ROLE_SYS_ADMIN);
isKeyAdmin = userRoles.contains(RangerConstants.ROLE_KEY_ADMIN);
}
validator.validate(policy, Action.UPDATE, bizUtil.isAdmin() || isServiceAdmin(policy.getService()) || isZoneAdmin(policy.getZoneName())); | ||
|
||
ensureAdminAccess(policy); | ||
ensureAdminAccess(policy, null); |
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 avoid updates to existing use of ensureAdminAccess(policy)
, I suggest adding following method:
void ensureAdminAccess(RangerPolicy policy) {
ensureAdminAccess(policy, null);
}
4aea2f8
to
290a61e
Compare
… and revokeAccess()
290a61e
to
82c1eb8
Compare
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.
Looks good to me.
Merged in master - c731ac3 |
… and revokeAccess()
What changes were proposed in this pull request?
Added parameter in ensureAdminAccess() method to pass grantor in case userName is null.
How was this patch tested?
Build passed and checked if grant and revoke are working.