> On Oct. 9, 2015, 10:24 p.m., Madhan Neethiraj wrote: > > Followig two places seem to be inconsistent in assigning to > > XXUserPermission.userId: the first one assigns XXUser.id and the second one > > assigns XXPortalUser.id. Please review. > > > > XUserMgr.java: #272 > > vXUserPermission.setUserId(xUser.getId()); > > > > XUserPermissionServiceBase.java: #53 > > vObj.setUserId(portalUser.getId()); > > Gautam Borad wrote: > XUserMgr.java: #272 > vXUserPermission.setUserId(xUser.getId()); > >>> From here object is getting created using createResource method of > service class, so as per the implementation while creating object, > client/UI/internal code will send xUserId and at service layer it will be > mapped to xPortalUserId. > So this seems correct. > > XUserPermissionServiceBase.java: #53 > vObj.setUserId(portalUser.getId()); > >>> This is the place where the mapping from xUserId to xPortalUserId > happens. As I mentioned above while creating/updating, UI/client will send > xUserId which will be mapped here to xPortalUserId.
VXUserPermission.userId is assigned two different values: XUser.Id in XUserMgr.java: #273 and XPortalUserId.id in XUserPermissionServiceBase.java: #53. Hence this concern. It looks like the value assigned in XUserPermissionServiceBase.mapViewToEntityBean() (line #53) may not be necessary. This method is to populate the entity object, XXUserPermission, from the contents of the view object, VXUserPermission, - right? Not assigning "VXUserPermission.userId" with "portalUser.id" will help reduce the confusion - and also make sure that subsequent references to VXUserPermission.userId, like in validateXUserPermForCreate()/validateXUserPermForUpdate() will expect "user.Id". If the assignment (line #53) is for optimization (to avoid query to covert userId ==> portaUserId), I would suggest going with one of the following: - adding a field "VXUserPermission.portalUserId" and have it populated from the userId field at the point of entry - use a query that finds XXUserPermission from the given user.id and moduleId (instead of portalUser.id and moduleId) - Madhan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39122/#review102121 ----------------------------------------------------------- On Oct. 9, 2015, 9:15 a.m., Gautam Borad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39122/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2015, 9:15 a.m.) > > > Review request for ranger, Alok Lal, Don Bosco Durai, Abhay Kulkarni, Madhan > Neethiraj, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy. > > > Bugs: RANGER-688 > https://issues.apache.org/jira/browse/RANGER-688 > > > Repository: ranger > > > Description > ------- > > *Current Implementation:* > > x_user_module_perm.userId refers to x_portal_user.id but while > assigning/revoking permissions using permission model, UI sends userId of > x_user table. > So this is a bug in current implementation because UI sends x_user.id and > server assumes that it's x_portal_user.id and will create permission. > When IDs of x_user and x_portal_user will not be in sync at that time this > may cause a serious issue. > > *Fix provided in patch:* > > UI will always have x_user/s but on server side x_portal_user/s required. So > now we can't expect UI to send x_portal_user.id in userPermission object so > in patch what we have done is: > Let UI send x_user.id in userPermission object but on server side it will be > mapped to x_portal_user.id and same when UI is reading permissions, let > database return x_portal_user.id that will be mapped to x_user.id. > Due to this UI remains as it is and also, we don't need to change table > structure of x_user_module_perm. > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java 5f43bc0 > security-admin/src/main/java/org/apache/ranger/common/UserSessionBase.java > 59e55f3 > security-admin/src/main/java/org/apache/ranger/db/XXPortalUserDao.java > d3467f8 > security-admin/src/main/java/org/apache/ranger/db/XXUserDao.java 0887594 > security-admin/src/main/java/org/apache/ranger/db/XXUserPermissionDao.java > e10dc14 > > security-admin/src/main/java/org/apache/ranger/patch/PatchPersmissionModel_J10003.java > f0aa938 > > security-admin/src/main/java/org/apache/ranger/service/XUserPermissionService.java > 3ff9c8d > > security-admin/src/main/java/org/apache/ranger/service/XUserPermissionServiceBase.java > 59c082d > security-admin/src/main/resources/META-INF/jpa_named_queries.xml 0370e9a > > Diff: https://reviews.apache.org/r/39122/diff/ > > > Testing > ------- > > Tested by creating a XPOrtalUser user using curl command and then for a new > user, assigned permissions to check if permissions are not messed up. > > > Thanks, > > Gautam Borad > >
