> On Oct. 8, 2015, 4:49 p.m., Madhan Neethiraj wrote: > > security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java, line 270 > > <https://reviews.apache.org/r/39122/diff/1/?file=1092890#file1092890line270> > > > > In this case, vxUserPermission.userId will be null. Will this > > userpermission object be useful? Would this cause failure in persisting in > > the db?
Here we are passing username as x_user is null so in service class, we will find x_portal_user using username and in database create permission for that user. Though such permission will not cause an issue and also not make sense to have in database, that's why it would be better we dont create such permissions, updating code to ignore if x_user does not exist. > On Oct. 8, 2015, 4:49 p.m., Madhan Neethiraj wrote: > > security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java, line 272 > > <https://reviews.apache.org/r/39122/diff/1/?file=1092890#file1092890line272> > > > > XXUserPermission.userId now has XXUser.id, instead of XXPortalUser.id. > > Please review all uses of XXUserPermission.userId to make sure that > > XXUser.id is used to store/compare. For example in > > XXModuleDefDao.findAccessibleModulesByUserId(). > > > > Also, what is the plan to update existing entries in XXUserPermission > > table? XXUserPermission.userId has XXPortalUser.id only. XXUser.id will be used for UI only. For example when server returns object to UI at that time UI will get userId from XXUser table; and when UI will send object to server it will be considered as xUserId and will be mapped to xPortalUser.id and then persistence will be done. Also, what is the plan to update existing entries in XXUserPermission table? >>> As per discussion with Vel, we have created JIRA but right now it is not >>> set as priority task. > On Oct. 8, 2015, 4:49 p.m., Madhan Neethiraj wrote: > > security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java, line 283 > > <https://reviews.apache.org/r/39122/diff/1/?file=1092890#file1092890line283> > > > > What does 'isCreate' flag mean here? It seems the code updates existing > > object if this flag is set.. Here we check for existing object in database, if it does not exist then we'll create new. If object exist in db and isCreate flag is false then we ignore. However, if object exists in db and isCreate flag is set, then we update permission object to allow that user. It might be possible that userPermission object is there in db but `isAllowed` is set to `No` so in this case if isCreate flag is set then we are kind of assigning/creating permission to that user. - Gautam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39122/#review101928 ----------------------------------------------------------- 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 > >
