> On March 22, 2023, 7:35 p.m., Madhan Neethiraj wrote: > > security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java > > Line 832 (original), 833 (patched) > > <https://reviews.apache.org/r/74349/diff/1/?file=2274970#file2274970line833> > > > > Instead of introducing RangerUsersAndGroups, I suggest to use existing > > RangerRole. This class also enables: > > - adding roles to a role > > - adding both admin and non-admin role members > > > > > > Also, I would recommend not changing signature of existing public > > methods - to avoid breaking clients using these methods. Instead, consider > > adding new methods. > > Ramachandran Krishnan wrote: > Hi Madhan, > > Do we need to keep both old and new behaviour like this > Or something different ? > > In PublicAPIsv2.java > > @PUT > @Path("/api/roles/{id}/addUsersAndGroups") > @Consumes({ "application/json" }) > @Produces({ "application/json" }) > public RangerRole addUsersAndGroupsByRoleId(@PathParam("id") Long > roleId, RangerRole rangerRole, @DefaultValue("false") @QueryParam("isAdmin") > Boolean isAdmin, @Context HttpServletRequest request) { > if (logger.isDebugEnabled()) { > logger.debug("==> PublicAPIsv2.addUsersAndGroups(id=" + > roleId + ", rangerRole=" + rangerRole + ", isAdmin=" + isAdmin + ")"); > } > > RangerRole rangerRole = > roleREST.addUsersAndGroupsByRoleId(roleId, rangerUsersAndGroups, isAdmin); > > if (logger.isDebugEnabled()) { > logger.debug("<== PublicAPIsv2.addUsersAndGroups(id=" + > roleId + ", rangerUsersAndGroups=" + rangerUsersAndGroups + ", isAdmin=" + > isAdmin + ")"); > } > return rangerRole; > } > > > public RangerRole addUsersAndGroups(Long roleId, List<String> users, > List<String> groups, Boolean isAdmin) { > return roleREST.addUsersAndGroups(roleId, users, groups, > isAdmin); > } > > > In RoleREST.java > > > @PUT > @Path("/roles/{id}/addUsersAndGroups") > @Consumes({ "application/json" }) > @Produces({ "application/json" }) > public RangerRole addUsersAndGroupsByRoleId(@PathParam("id") Long > roleId, RangerRole rangerRole, @DefaultValue("false") @QueryParam("isAdmin") > Boolean isAdmin) { > if (LOG.isDebugEnabled()) { > LOG.debug("==> addUsersAndGroups(id=" + roleId + ", > rangerRole=" + rangerRole + ", isAdmin=" + isAdmin + ")"); > } > > //Fetch the groups and users from rangerRole and pass > > List<String> users = new ArrayList<>(); > List<String> groups = new ArrayList<>(); > if (CollectionUtils.isEmpty(rangerRole.getUsers())) { > users = rangerRole.getUsers().stream().map(roleMember -> > roleMember.getName()).collect(Collectors.toList()); > } > > if (CollectionUtils.isEmpty(rangerRole.getGroups())) { > groups = rangerRole.getGroups().stream().map(roleMember -> > roleMember.getName()).collect(Collectors.toList()); > } > > RangerRole rangerRole = addUsersAndGroups(roleId, users, groups, > isAdmin); > > if (LOG.isDebugEnabled()) { > LOG.debug("<== addUsersAndGroups(id=" + roleId + ", > rangerRole=" + rangerRole + ", isAdmin=" + isAdmin + ")"); > } > > return rangerRole; > } > > public RangerRole addUsersAndGroups(Long roleId, List<String> users, > List<String> groups, Boolean isAdmin) { > if (LOG.isDebugEnabled()) { > LOG.debug("==> addUsersAndGroups(id=" + roleId + ", users=" + > Arrays.toString(users.toArray()) + ", groups=" + > Arrays.toString(groups.toArray()) + ", isAdmin=" + isAdmin + ")"); > }
A reminder to review this Ranger Roles Rest API's enhancement. Thanks! - Ramachandran ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74349/#review225293 ----------------------------------------------------------- On April 10, 2023, 2:01 p.m., Ramachandran Krishnan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74349/ > ----------------------------------------------------------- > > (Updated April 10, 2023, 2:01 p.m.) > > > Review request for ranger, Don Bosco Durai, Abhay Kulkarni, Madhan Neethiraj, > Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, > Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan Periasamy. > > > Bugs: RANGER-4133 > https://issues.apache.org/jira/browse/RANGER-4133 > > > Repository: ranger > > > Description > ------- > > Apis: > > API :addUsersAndGroups > > URI:/roles/roles/{id}/addUsersAndGroups > > In the API we mentioned a couple of fields (List<String> users, List<String> > groups, Boolean isAdmin). > How the caller will pass the fields > Proposed Solution: > Via query Parameter (Limitation: we can not pass more characters as part of > URI ) > > Existing RangerRole accommodate users, groups > > { > "groups": [ > { > "name": "testGroup1" > }, > { > "name": "testGroup2" > } > ], > "users": [ > { > "name": "testUser1" > }, > { > "name": "testUser2" > } > ] > } > > So we can use RangerRole as the Request payload > > Existing RangerRole accommodate users, groups > > > API :removeUsersAndGroups > > URI:/roles/roles/{id}/removeUsersAndGroups > > > In the API we mentioned a couple of fields (List<String> users, List<String> > groups) > How the caller will pass the fields > Proposed Solution: > Existing RangerRole accommodate users, groups > So we can use RangerRole as the Request payload > > > API :removeAdminFromUsersAndGroups > > URI:/roles/roles/{id}/removeUsersAndGroups > > > In the API we mentioned a couple of fields (List<String> users, List<String> > groups) > How the caller will pass the fields > Proposed Solution: > Existing RangerRole accommodate users, groups > So we can use RangerRole as the Request payload > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java > 85cd7dd67 > security-admin/src/main/java/org/apache/ranger/rest/RoleREST.java 4f0edd2b0 > security-admin/src/test/java/org/apache/ranger/rest/TestPublicAPIsv2.java > 73a593e9f > security-admin/src/test/java/org/apache/ranger/rest/TestRoleREST.java > 217c1bba3 > > > Diff: https://reviews.apache.org/r/74349/diff/2/ > > > Testing > ------- > > > Thanks, > > Ramachandran Krishnan > >
