You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Madhan Neethiraj <ma...@apache.org> on 2015/10/06 07:54:25 UTC
Re: Review Request 38848: RANGER-671 : Add support to retrieve
permissions
for the logged in user from UserSession rather going to database every time
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38848/#review101601
-----------------------------------------------------------
security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java (line 411)
<https://reviews.apache.org/r/38848/#comment159033>
There could be multiple active sessions for the an user, right? In that case, this method (and the next method) should return list of sessions.
security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java (line 425)
<https://reviews.apache.org/r/38848/#comment159031>
This loop will result in one call per each session that exists in the server. Is this necessary? Also, the object returned by the DB call, xUser, is used only to check if the user exists or not. Is that relevant here?
security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java (line 260)
<https://reviews.apache.org/r/38848/#comment159032>
To be clearer and avoid confusion, rename "userId" ==> "portalUserId".
security-admin/src/main/java/org/apache/ranger/common/UserSessionBase.java (line 133)
<https://reviews.apache.org/r/38848/#comment159034>
Many variables are named as *List, though their type is a Set. Consider renaming the variables (and accessors) to use names like "userPermissions" (i.e. without using List or Set in the name).
security-admin/src/main/java/org/apache/ranger/db/XXGroupPermissionDao.java (line 107)
<https://reviews.apache.org/r/38848/#comment159035>
Query name says ModuleIdAndUserId; but the parameter passed is groupId. Please review and fix.
security-admin/src/main/java/org/apache/ranger/db/XXGroupPermissionDao.java (line 108)
<https://reviews.apache.org/r/38848/#comment159036>
getSingleResult() - ensure that DB constraint is in place to avoid duplicate records. If no constraint exists, it will be safe to get the resultList and use only the first entry.
Please review ALL other such queries. Any failure here might result in no REST calls to go through!!
security-admin/src/main/java/org/apache/ranger/security/listener/RangerHttpSessionListener.java (line 39)
<https://reviews.apache.org/r/38848/#comment159039>
When does sessionDestroyed() get called? If user does not logout, would this get called after the session timeout? What is the default session timeout? Would this result in Ranger Admin to store a large number of HttpSession objects in memory? Especially, for sessions from REST clients..
security-admin/src/main/java/org/apache/ranger/security/listener/RangerHttpSessionListener.java (line 45)
<https://reviews.apache.org/r/38848/#comment159037>
will listOfSession ever be null?
security-admin/src/main/resources/META-INF/jpa_named_queries.xml (line 499)
<https://reviews.apache.org/r/38848/#comment159038>
query name is "ModuleIdAndUserId"; but the parameter is "groupId". Please review and update.
- Madhan Neethiraj
On Sept. 29, 2015, 4:41 p.m., Gautam Borad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38848/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 4:41 p.m.)
>
>
> Review request for ranger, Alok Lal, Don Bosco Durai, Abhay Kulkarni, Madhan Neethiraj, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-671
> https://issues.apache.org/jira/browse/RANGER-671
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Currently to fetch permissions for the user, every time a DB call is made to get list of tabs to which a user has permissions.
> Patch contains changes to fetch list of permissions from UserSession.
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java ccb18550dccb754ca718566f67cc47b92727c9c0
> security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java 41bc6f84a8a73353975536d19358882dfedfcf7e
> security-admin/src/main/java/org/apache/ranger/common/UserSessionBase.java 37b2049afe4234ce08fb6a87113f6995109484f1
> security-admin/src/main/java/org/apache/ranger/db/XXGroupPermissionDao.java 3121e7a5912c18b668c1f597953716c89bbbec39
> security-admin/src/main/java/org/apache/ranger/db/XXGroupUserDao.java 104e1884234de023566fd741e72bd429d3981d06
> security-admin/src/main/java/org/apache/ranger/db/XXModuleDefDao.java fa2b3d99b754973b4bd37cd09cc4f2d7696ed12e
> security-admin/src/main/java/org/apache/ranger/db/XXUserPermissionDao.java 1956b30abefda3e6380475a971de8a2ea8c746db
> security-admin/src/main/java/org/apache/ranger/patch/PatchPersmissionModel_J10003.java 841e386408454ef5fafe464e20f5aa3fe53d4d8c
> security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java 472dad6bb3753c32805be9401bb5b9c2e6178280
> security-admin/src/main/java/org/apache/ranger/security/context/RangerPreAuthSecurityHandler.java 6d132e6c7515cf2300b01cfaf514fe18e2900876
> security-admin/src/main/java/org/apache/ranger/security/listener/RangerHttpSessionListener.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/service/XGroupPermissionService.java d5168eb54df50e03e2842d9722736f00742d31be
> security-admin/src/main/java/org/apache/ranger/service/XUserPermissionService.java 92b69515513e5dce939e88420f1a74a567c19ad3
> security-admin/src/main/resources/META-INF/jpa_named_queries.xml ac4c753853bc6b16617c99daf978c825deb20a2e
> security-admin/src/main/webapp/WEB-INF/web.xml cbd57243c027f06e14a9b0a034717cf29bca97bb
> security-admin/src/test/java/org/apache/ranger/biz/TestXUserMgr.java ab149ad1b53c9e8e57dd3981c2aad89d4cdd1faf
>
> Diff: https://reviews.apache.org/r/38848/diff/
>
>
> Testing
> -------
>
> Tested REST calls using curl commands on a cluster having Ranger. Also done manual testing of Permission Model to work as expected.
>
>
> Thanks,
>
> Gautam Borad
>
>
Re: Review Request 38848: RANGER-671 : Add support to retrieve
permissions
for the logged in user from UserSession rather going to database every time
Posted by Gautam Borad <gb...@gmail.com>.
- Gautam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38848/#review101601
-----------------------------------------------------------
On Oct. 6, 2015, 10:14 a.m., Gautam Borad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38848/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2015, 10:14 a.m.)
>
>
> Review request for ranger, Alok Lal, Don Bosco Durai, Abhay Kulkarni, Madhan Neethiraj, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-671
> https://issues.apache.org/jira/browse/RANGER-671
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Currently to fetch permissions for the user, every time a DB call is made to get list of tabs to which a user has permissions.
> Patch contains changes to fetch list of permissions from UserSession.
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java ccb1855
> security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java 41bc6f8
> security-admin/src/main/java/org/apache/ranger/common/UserSessionBase.java 37b2049
> security-admin/src/main/java/org/apache/ranger/db/XXGroupPermissionDao.java 3121e7a
> security-admin/src/main/java/org/apache/ranger/db/XXGroupUserDao.java 104e188
> security-admin/src/main/java/org/apache/ranger/db/XXModuleDefDao.java fa2b3d9
> security-admin/src/main/java/org/apache/ranger/db/XXUserPermissionDao.java 1956b30
> security-admin/src/main/java/org/apache/ranger/patch/PatchPersmissionModel_J10003.java 841e386
> security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java 472dad6
> security-admin/src/main/java/org/apache/ranger/security/context/RangerPreAuthSecurityHandler.java 6d132e6
> security-admin/src/main/java/org/apache/ranger/security/listener/RangerHttpSessionListener.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/service/XGroupPermissionService.java d5168eb
> security-admin/src/main/java/org/apache/ranger/service/XUserPermissionService.java 92b6951
> security-admin/src/main/resources/META-INF/jpa_named_queries.xml ac4c753
> security-admin/src/test/java/org/apache/ranger/biz/TestXUserMgr.java ab149ad
>
> Diff: https://reviews.apache.org/r/38848/diff/
>
>
> Testing
> -------
>
> Tested REST calls using curl commands on a cluster having Ranger. Also done manual testing of Permission Model to work as expected.
>
>
> Thanks,
>
> Gautam Borad
>
>
Re: Review Request 38848: RANGER-671 : Add support to retrieve
permissions
for the logged in user from UserSession rather going to database every time
Posted by Gautam Borad <gb...@gmail.com>.
> On Oct. 6, 2015, 5:54 a.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java, line 425
> > <https://reviews.apache.org/r/38848/diff/1/?file=1086805#file1086805line425>
> >
> > This loop will result in one call per each session that exists in the server. Is this necessary? Also, the object returned by the DB call, xUser, is used only to check if the user exists or not. Is that relevant here?
This will be required right, because in UserSession there won't be XUser Id, but there would be loginId/username and for that we have to get XXUser object using loginId/username.
> On Oct. 6, 2015, 5:54 a.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/db/XXGroupPermissionDao.java, line 108
> > <https://reviews.apache.org/r/38848/diff/1/?file=1086808#file1086808line108>
> >
> > getSingleResult() - ensure that DB constraint is in place to avoid duplicate records. If no constraint exists, it will be safe to get the resultList and use only the first entry.
> >
> > Please review ALL other such queries. Any failure here might result in no REST calls to go through!!
Here constraint is in place. Is it good to use resultList?
If we are expecting only single record in database then Isn't it good to use getSingleResult()?
If duplicate records are there then, getSingleResult will definitely break but at least we will get to know that something went wrong and due to that there is duplicate entries in db.
> On Oct. 6, 2015, 5:54 a.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/security/listener/RangerHttpSessionListener.java, line 39
> > <https://reviews.apache.org/r/38848/diff/1/?file=1086816#file1086816line39>
> >
> > When does sessionDestroyed() get called? If user does not logout, would this get called after the session timeout? What is the default session timeout? Would this result in Ranger Admin to store a large number of HttpSession objects in memory? Especially, for sessions from REST clients..
1) When does sessionDestroyed() get called?
>>> When user logs out and even if user does not logout this will get called at the time of session timeout.
2) What is the default session timeout?
>>> 60 minutes
3) Would this result in Ranger Admin to store a large number of HttpSession objects in memory? Especially, for sessions from REST clients..
>>> When we call APIs using CURL that is basic authentication, it creates session but will not destroy immediately, but of course after 60mins (session timeout duration) it will get destroyed
> On Oct. 6, 2015, 5:54 a.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/security/listener/RangerHttpSessionListener.java, line 45
> > <https://reviews.apache.org/r/38848/diff/1/?file=1086816#file1086816line45>
> >
> > will listOfSession ever be null?
In our case it won't be null, so removing null check
- Gautam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38848/#review101601
-----------------------------------------------------------
On Oct. 6, 2015, 10:14 a.m., Gautam Borad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38848/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2015, 10:14 a.m.)
>
>
> Review request for ranger, Alok Lal, Don Bosco Durai, Abhay Kulkarni, Madhan Neethiraj, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-671
> https://issues.apache.org/jira/browse/RANGER-671
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Currently to fetch permissions for the user, every time a DB call is made to get list of tabs to which a user has permissions.
> Patch contains changes to fetch list of permissions from UserSession.
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java ccb1855
> security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java 41bc6f8
> security-admin/src/main/java/org/apache/ranger/common/UserSessionBase.java 37b2049
> security-admin/src/main/java/org/apache/ranger/db/XXGroupPermissionDao.java 3121e7a
> security-admin/src/main/java/org/apache/ranger/db/XXGroupUserDao.java 104e188
> security-admin/src/main/java/org/apache/ranger/db/XXModuleDefDao.java fa2b3d9
> security-admin/src/main/java/org/apache/ranger/db/XXUserPermissionDao.java 1956b30
> security-admin/src/main/java/org/apache/ranger/patch/PatchPersmissionModel_J10003.java 841e386
> security-admin/src/main/java/org/apache/ranger/rest/XUserREST.java 472dad6
> security-admin/src/main/java/org/apache/ranger/security/context/RangerPreAuthSecurityHandler.java 6d132e6
> security-admin/src/main/java/org/apache/ranger/security/listener/RangerHttpSessionListener.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/service/XGroupPermissionService.java d5168eb
> security-admin/src/main/java/org/apache/ranger/service/XUserPermissionService.java 92b6951
> security-admin/src/main/resources/META-INF/jpa_named_queries.xml ac4c753
> security-admin/src/test/java/org/apache/ranger/biz/TestXUserMgr.java ab149ad
>
> Diff: https://reviews.apache.org/r/38848/diff/
>
>
> Testing
> -------
>
> Tested REST calls using curl commands on a cluster having Ranger. Also done manual testing of Permission Model to work as expected.
>
>
> Thanks,
>
> Gautam Borad
>
>