----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42602/#review117913 -----------------------------------------------------------
security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1434) <https://reviews.apache.org/r/42602/#comment179223> Consider changing this log level to debug. security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1463) <https://reviews.apache.org/r/42602/#comment179222> "Removing user '" + xXUser.getName() + ' from group '" + groupUser.getName() + "'" security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1476) <https://reviews.apache.org/r/42602/#comment179221> "Deleting '" + AppConstants.getLabelFor_XAPermType(vXPermMap.getPermType()) + "' permission from policy ID='" + vXPermMap.getResourceId() + "' for group '" + vXPermMap.getGroupName() + "'" security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1487) <https://reviews.apache.org/r/42602/#comment179217> This log message may not be useful, as there is no simple co-relation of XXAuditMap objects to policy UI. Consider removing this log msg. security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1498) <https://reviews.apache.org/r/42602/#comment179216> "Removing group '" + xXGroupChild.getName() + ' from group '" + xXGroupParent.getName() + "'" security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1505) <https://reviews.apache.org/r/42602/#comment179214> Instead of updating RangerPolicy, it will be efficient to directly remove XXPolicyItemGroupPerm. That will also be help in deleting group permission entries across policyItem types - denyItems, allowExceptions, denyExceptions. security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1532) <https://reviews.apache.org/r/42602/#comment179215> Error in policy update should be propagaged up, so that the operation will be aborted. Not being able to remove group references in policies will result in not being able to delete the group object. This comment may not be relevant if XXPolicyItemGroupPerm objects are directory removed (instead of going through policy update) security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1537) <https://reviews.apache.org/r/42602/#comment179213> This block of code is repeated in else clause as well. Consider moving it outside of if/else, to avoid duplicate code. security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1585) <https://reviews.apache.org/r/42602/#comment179171> "No user found with id=" + id security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1593) <https://reviews.apache.org/r/42602/#comment179224> Consider changing this log level to debug. security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1624) <https://reviews.apache.org/r/42602/#comment179181> "Removing user '" + vXUser.getName() + ' from group '" + groupUser.getName() + "'" security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1634) <https://reviews.apache.org/r/42602/#comment179185> xXResource retrieval can be avoided, by logging only the policy-ID. Apply this for other such usage as well. security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1636) <https://reviews.apache.org/r/42602/#comment179183> "Deleting '" + AppConstants.getLabelFor_XAPermType(vXPermMap.getPermType()) + "' permission from policy ID='" + vXPermMap.getResourceId() + "' for user '" + vXPermMap.getUserName() + "'" security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1647) <https://reviews.apache.org/r/42602/#comment179192> This log message may not be useful, as there is no simple co-relation of XXAuditMap objects to policy UI. Consider removing this log msg. security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1656) <https://reviews.apache.org/r/42602/#comment179194> "Deleting " + xXAuthSessions.size() + " login session records for user '" + vXPortalUser.getLoginId() + "'" security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1665) <https://reviews.apache.org/r/42602/#comment179195> "Deleting '" + xXModuleDef.getModule() + "' module permission for user '" + vXPortalUser.getLoginId() + "'" security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1672) <https://reviews.apache.org/r/42602/#comment179196> "Deleting '" + xXPortalUserRole.getUserRole() + "' role for user '" + vXPortalUser.getLoginId() + "'" security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1678) <https://reviews.apache.org/r/42602/#comment179207> Why not remove XXPolicyItemUserPerm directly, similar to XXUserPermission objects above? That approach will be efficient and also will automatically address multiple policyItems in ranger-0.6 - denyItems, allowExceptions, denyException. security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1706) <https://reviews.apache.org/r/42602/#comment179210> Error in policy update should be propagaged up, so that the operation will be aborted. Not being able to remove user references in policies will result in not being able to delete the user object. This comment may not be relevant if XXPolicyItemUserPerm objects are directory removed (instead of going through policy update) security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 1711) <https://reviews.apache.org/r/42602/#comment179211> This block of code is repeated in else clause as well. Consider moving this out of if/else to avoid duplicate. security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java (line 271) <https://reviews.apache.org/r/42602/#comment179169> Instead of aborting with a NullPointerException, consider printing a WARN log and let the caller continue further. - Madhan Neethiraj On Feb. 2, 2016, 12:10 p.m., Gautam Borad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42602/ > ----------------------------------------------------------- > > (Updated Feb. 2, 2016, 12:10 p.m.) > > > Review request for ranger, Alok Lal, Don Bosco Durai, Abhay Kulkarni, Madhan > Neethiraj, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy. > > > Bugs: RANGER-204 > https://issues.apache.org/jira/browse/RANGER-204 > > > Repository: ranger > > > Description > ------- > > Problem Statement : Delete rest api of User not deleting user completely from > system and not able to delete user or group if user/group has any policy > defined. > > Proposed Solution: > Delete User/Group Rest will have 'forceDelete' queryParam, if 'forceDelete' > is true then User/group and their references shall be deleted permanently > from db, if 'forceDelete' is false and if there are any references of > provided User/Group then User/Group visibility shall be set to 'Hidden'. If > 'forceDelete' is false and there are no references then system will try to > permanently delete User/group from DB. > If 'forceDelete' is not passed in request then it will be set to false. > Delete User/Group related audit log will be logged in x_trx_log table and > shall be available in ranger logs also. > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java 3784439 > security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java > aaa4fa5 > security-admin/src/main/java/org/apache/ranger/db/XXAuthSessionDao.java > 4c9bdc5 > security-admin/src/main/java/org/apache/ranger/db/XXGroupGroupDao.java > df2796c > security-admin/src/main/java/org/apache/ranger/db/XXPolicyDao.java 006964c > security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java 448a60a > > security-admin/src/main/java/org/apache/ranger/service/XAuditMapService.java > 462b81a > > security-admin/src/main/java/org/apache/ranger/service/XPortalUserService.java > 41c4552 > security-admin/src/main/resources/META-INF/jpa_named_queries.xml f3aa431 > security-admin/src/test/java/org/apache/ranger/biz/TestXUserMgr.java > 8ace44b > > Diff: https://reviews.apache.org/r/42602/diff/ > > > Testing > ------- > > Tested following REST with forceDelete value as 'true'/'false'. > 1. service/xusers/users/userName/{userName} > 2. service/xusers/groups/groupName/{groupName} > 3. service/xusers/users/{id} > 4. service/xusers/groups/{id} > > Below are the observations: > 400/Bad Request if Group/User does'nt exist. > 204/No Content if Group/User is deleted or their status is changed to 'Hidden' > Delete/Update logs can be in Admin Audit log tab. > 'Warn' logs was observed if Group/User and their references were deleted. > > > Thanks, > > Gautam Borad > >
