----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71999/#review219360 -----------------------------------------------------------
I have reviewed changes in agents-common and security-admin module. agents-common/src/main/java/org/apache/ranger/plugin/model/UserInfo.java Lines 92 (patched) <https://reviews.apache.org/r/71999/#comment307499> Please include groups here for printing. agents-common/src/main/java/org/apache/ranger/plugin/model/UserInfo.java Lines 95 (patched) <https://reviews.apache.org/r/71999/#comment307500> Please consider removing duplicate code by using the function in the GroupInfo. agents-common/src/main/java/org/apache/ranger/plugin/util/RangerUserStore.java Lines 41 (patched) <https://reviews.apache.org/r/71999/#comment307501> Please consider implementing toString() for this class for debugging help. agents-common/src/main/java/org/apache/ranger/plugin/util/RangerUserStoreUtil.java Lines 57 (patched) <https://reviews.apache.org/r/71999/#comment307502> This code block appears to be the same as the block at 42. Please see if code duplication can be eliminated. Please review implementations of other functions (getGroupAttrVal, etc.) for the same. security-admin/src/main/java/org/apache/ranger/common/RangerUserStoreCache.java Lines 41 (patched) <https://reviews.apache.org/r/71999/#comment307503> Please review formatting of all variable declarations. security-admin/src/main/java/org/apache/ranger/common/RangerUserStoreCache.java Lines 67 (patched) <https://reviews.apache.org/r/71999/#comment307504> Is having a wrapper necessary here? Please review to see if RangerUserCacheWrapper class can be folded into RangerUserStoreCache class. security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java Lines 95 (patched) <https://reviews.apache.org/r/71999/#comment307495> Please consider using a different config parameter to control downloading of user-store. security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java Lines 1266 (patched) <https://reviews.apache.org/r/71999/#comment307498> Are we supporting only secure version of the download userStore API? security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java Lines 1289 (patched) <https://reviews.apache.org/r/71999/#comment307497> Please review if this API is appropriate for checking validity in this case, as the implementation seems to be too tightly coupled with "downloadPolicy" functionality. security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java Lines 1303 (patched) <https://reviews.apache.org/r/71999/#comment307496> If service is found to be valid, will this error condition ever occur? Please review. security-admin/src/main/java/org/apache/ranger/service/XUserServiceBase.java Lines 100 (patched) <https://reviews.apache.org/r/71999/#comment307505> Please review to see if we need to create a new Gson object in this function. It will be good if that can be avoided. - Abhay Kulkarni On Jan. 22, 2020, 7:02 a.m., Sailaja Polavarapu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71999/ > ----------------------------------------------------------- > > (Updated Jan. 22, 2020, 7:02 a.m.) > > > Review request for ranger. > > > Bugs: RANGER-2697 > https://issues.apache.org/jira/browse/RANGER-2697 > > > Repository: ranger > > > Description > ------- > > Modified Usersync code to retrieve configurable user and group attributes > from AD/LDAP. Also added rest API support in ranger admin to dowmload user > and group information including these additional attributes. > > > Diffs > ----- > > agents-common/src/main/java/org/apache/ranger/plugin/model/GroupInfo.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPluginInfo.java > 90367fe04 > agents-common/src/main/java/org/apache/ranger/plugin/model/UserInfo.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerUserStore.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerUserStoreUtil.java > PRE-CREATION > security-admin/src/main/java/org/apache/ranger/biz/AssetMgr.java 0f4488861 > security-admin/src/main/java/org/apache/ranger/biz/RoleDBStore.java > 04596dc65 > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > ccda6abb0 > security-admin/src/main/java/org/apache/ranger/biz/UserMgr.java 3045eaf9a > security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java 037c591e8 > > security-admin/src/main/java/org/apache/ranger/common/RangerUserStoreCache.java > PRE-CREATION > security-admin/src/main/java/org/apache/ranger/db/XXGlobalStateDao.java > 2e462bd3a > security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java > 1e8a09379 > > security-admin/src/main/java/org/apache/ranger/service/XGroupServiceBase.java > 1a701bbfb > > security-admin/src/main/java/org/apache/ranger/service/XUserServiceBase.java > 1004952a9 > > ugsync/src/main/java/org/apache/ranger/ldapusersync/process/LdapDeltaUserGroupBuilder.java > ca50f098a > > ugsync/src/main/java/org/apache/ranger/ldapusersync/process/LdapPolicyMgrUserGroupBuilder.java > b469e9245 > > ugsync/src/main/java/org/apache/ranger/ldapusersync/process/LdapUserGroupBuilder.java > 8efa1613a > > ugsync/src/main/java/org/apache/ranger/unixusersync/config/UserGroupSyncConfig.java > 1d4e37fcf > ugsync/src/main/java/org/apache/ranger/unixusersync/model/MUserInfo.java > 841bac64a > ugsync/src/main/java/org/apache/ranger/unixusersync/model/XGroupInfo.java > cbe0eb02b > ugsync/src/main/java/org/apache/ranger/unixusersync/model/XUserInfo.java > e2f36b2c5 > > ugsync/src/main/java/org/apache/ranger/unixusersync/process/PolicyMgrUserGroupBuilder.java > f08c5117b > ugsync/src/main/java/org/apache/ranger/usergroupsync/UserGroupSink.java > 77bd016fd > > ugsync/src/test/java/org/apache/ranger/usergroupsync/LdapPolicyMgrUserGroupBuilderTest.java > 99bc2b44e > > ugsync/src/test/java/org/apache/ranger/usergroupsync/PolicyMgrUserGroupBuilderTest.java > 2bc395180 > > > Diff: https://reviews.apache.org/r/71999/diff/2/ > > > Testing > ------- > > 1. Verified all the existing unit tests are run successfully. > 2. Patched a cluster and ran tests to sync various user/group attributes from > test AD > 3. Also verified new rest API to download user and group information with > addition attributes. > > > Thanks, > > Sailaja Polavarapu > >
