----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68096/#review206724 -----------------------------------------------------------
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java Lines 4796 (patched) <https://reviews.apache.org/r/68096/#comment289810> - instead of following 2 DB calls: - 1) retrieve XXService from serviceName - 2) retrieve XXServiceConfigMap from serviceId and configName consider making one DB call that takes serviceName and configName: daoMgr.getXXServiceConfigMap().findByServiceNameAndConfigKey(serviceName, SERVICE_ADMIN_USERS); the query should internally join XXService and XXServiceConfigMap tables. security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java Lines 4802 (patched) <https://reviews.apache.org/r/68096/#comment289811> Consider avoiding creation of userList. public boolean isServiceAdminUser(String serviceName, String userName) { boolean ret = false; XXServiceConfigMap cfgSvcAdminUsers = daoMgr.getXXServiceConfigMap().findByServiceNameAndConfigKey(serviceName, SERVICE_ADMIN_USERS); String svcAdminUsers = cfgSvcAdminUsers != null ? cfgSvcAdminUsers.getConfigValue() : null; if (svcAdminUsers != null) { for (String svcAdminUser : svcAdminUsers.split(",")) { if (userName.equals(svcAdminUser)) { ret = true; break; } } } return ret; } security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java Line 3033 (original), 3033 (patched) <https://reviews.apache.org/r/68096/#comment289812> Please avoid white-space-only changes. This file has bunch of such instance - like #3030 - #3034, #3040 - #3041, #3055, #3061. security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java Lines 3081 (patched) <https://reviews.apache.org/r/68096/#comment289813> Instead of separate 'if' block here, consider adding 'isServiceAdminUser' to line #3065 - like: if (isAdmin || isServiceAdminUser) { security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java Line 3107 (original), 3110 (patched) <https://reviews.apache.org/r/68096/#comment289814> Consider the following alternate: boolean isAdmin = bizUtil.isAdmin(); boolean isKeyAdmin = bizUtil.isKeyAdmin(); String userName = bizUtil.getCurrentUserLoginId(); boolean isSvcAdmin = isAdmin || svcStore.isServiceAdminUser(policy.getService(), userName); if(!isAdmin && !isKeyAdmin && !isSvcAdmin) { Similar update in ensureAdminAndAuditAccess() as well. - Madhan Neethiraj On July 29, 2018, 12:07 p.m., Pradeep Agrawal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68096/ > ----------------------------------------------------------- > > (Updated July 29, 2018, 12:07 p.m.) > > > Review request for ranger, Ankita Sinha, deepak sharma, Gautam Borad, Abhay > Kulkarni, Madhan Neethiraj, Mehul Parikh, suja s, and Velmurugan Periasamy. > > > Bugs: RANGER-2168 > https://issues.apache.org/jira/browse/RANGER-2168 > > > Repository: ranger > > > Description > ------- > > **Problem Statement:** Currently only user with admin role or a delegated > admin user can create the policy. We can possibly have a service admin user > who can be allowed to create policy. Such users can be configured in the > service config itself and can be removed by admin anytime. > > **Proposed Solution:** > Allow admin/keyadmin role users to add a custom service config property > 'service.admin.users' through service page. > Users provided in 'service.admin.users' can be internal or external and can > have any role. > Users provided in 'service.admin.users' should able to > create/update/delete/view policies of that ranger service. > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > 8efc950ce > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > e4449df2e > > > Diff: https://reviews.apache.org/r/68096/diff/1/ > > > Testing > ------- > > **Steps Performed:** > Created an internal user testuser in the Ranger admin. > Added a hive service 'hivedev' in Ranger. > > **Action-1**: Logged in from 'testuser' and tried to create a policy > 'testpolicy' in 'hivedev' service. > **Expected Behaviour**: Policy creation should fail. > **Actual Behaviour**: Policy creation failed. > > **Action-2.1**: Logged in from ranger admin user and added a custom property > 'service.admin.users' in 'hivedev' service and provided value 'testuser' in > the given text box. Saved the 'hivedev' service. > **Action-2.2**: Logged in from 'testuser' and tried to create a policy > 'testpolicy' in 'hivedev' service. > **Expected Behaviour**: Policy creation should successful. > **Actual Behaviour**: Policy creation finished successfully. > > Tested Policy updation and deletion which also executed successfully. > > **Action-3.1**: Logged in from ranger admin user and removed custom property > 'service.admin.users' from 'hivedev' service. Saved the 'hivedev' service. > **Action-3.2**: Logged in from 'testuser' and tried to create a policy > 'testpolicy1' in 'hivedev' service. > **Expected Behaviour**: Policy creation should fail. > **Actual Behaviour**: Policy creation failed. > > > Thanks, > > Pradeep Agrawal > >
