----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40343/#review106705 -----------------------------------------------------------
security-admin/scripts/install.properties (line 118) <https://reviews.apache.org/r/40343/#comment165458> sso_originalurl - does not convey that this about query param. Consider renaming to something like 'sso_query_param_originalurl'. Please review all usage of this configuration for such update. security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java (line 160) <https://reviews.apache.org/r/40343/#comment165457> Instead of adding a separate API to check 'if SSO is enabled', consider using RangerServerInfo discussed in RANGER-716 (https://reviews.apache.org/r/39900/). security-admin/src/main/java/org/apache/ranger/security/handler/RangerAuthenticationProvider.java (line 78) <https://reviews.apache.org/r/40343/#comment165439> Should ssoEnabled be a static member? And the visibility be public? RangerSSOAuthenticationFilter.doFilter() sets this member every time an instance of RangerAuthenticationProvider is created. Please review if this can be made an instance member? security-admin/src/main/java/org/apache/ranger/security/web/authentication/RangerAuthenticationEntryPoint.java (line 134) <https://reviews.apache.org/r/40343/#comment165438> RangerSSOAuthenticationFileter.ssoEnabled flag is a static member. This condition (url containing LOCAL_LOGIN_URL) sets this flag to "false" - for the rest of the process life. Is this the desired behaviour? Please review. For example: if someone accesses an URL containing LOCAL_LOGIN_URL - will it turn off SSO for Ranger Admin? security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 83) <https://reviews.apache.org/r/40343/#comment165483> Initialization of properties is already done in the constructor - why repeat here, in lines #83, #84? security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 103) <https://reviews.apache.org/r/40343/#comment165491> Would anyuser logged in via SSO have all 3 roles? ROLE_USER, ROLE_SYS_ADMIN, ROLE_KEY_ADMIN. If yes, this should be reviewed and fixed. security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 117) <https://reviews.apache.org/r/40343/#comment165488> surround the debug log with if(LOG.isDebugEnabled()) security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 125) <https://reviews.apache.org/r/40343/#comment165489> surround the debug log with if(LOG.isDebugEnabled()) security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 134) <https://reviews.apache.org/r/40343/#comment165461> This looks like a message to the end user, but it will only go to the log file. Please review the message content to be useful while looking at the log file. Also, it is not clean what this block (and LOCAL_LOGIN_URL) is about. A line of comment could be helpful. security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 144) <https://reviews.apache.org/r/40343/#comment165462> Are "Mozilla" and "Chorme" the only browsers supported for SSO? security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 144) <https://reviews.apache.org/r/40343/#comment165463> Are "Mozilla" and "Chorme" the only browsers supported for SSO? security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 182) <https://reviews.apache.org/r/40343/#comment165492> isAuthenticationRequired(): consider renaming to isAuthenticated(). security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 200) <https://reviews.apache.org/r/40343/#comment165496> Add a check for cookieName != null. security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 201) <https://reviews.apache.org/r/40343/#comment165493> Consider changing this to debug level. security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 240) <https://reviews.apache.org/r/40343/#comment165500> This log messages here do not contain much detail to be useful. Consider a production log file with hundreds/thousands of such logs - what more details can be added to the log messages to make them useful? security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 271) <https://reviews.apache.org/r/40343/#comment165501> This log message does not contain much detail to be useful. Consider a production log file with hundreds/thousands of such logs - what more details can be added to this log message to make it useful? security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java (line 298) <https://reviews.apache.org/r/40343/#comment165511> This log message does not contain much detail to be useful. Consider a production log file with hundreds/thousands of such logs - what more details can be added to this log message to make it useful? - Madhan Neethiraj On Nov. 16, 2015, 11:20 a.m., Gautam Borad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40343/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2015, 11:20 a.m.) > > > Review request for ranger, Alok Lal, Don Bosco Durai, Abhay Kulkarni, Larry > McCay, Madhan Neethiraj, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan > Periasamy. > > > Bugs: RANGER-685 > https://issues.apache.org/jira/browse/RANGER-685 > > > Repository: ranger > > > Description > ------- > > Add Ability to Authenticate users with SSO option provided by Knox. > > > Diffs > ----- > > security-admin/pom.xml 3c26837efdedbd4deb95a791418251751f4f17e9 > security-admin/scripts/install.properties > f3af716fef39bf0ac5821466803e6fb56b54fd96 > security-admin/scripts/setup.sh 36696a036cf9f27fd6e106abb5e5bf94e79190ad > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > 9173d6e2f9e89b2de5b93227230afd2bd91c9edc > > security-admin/src/main/java/org/apache/ranger/security/handler/RangerAuthenticationProvider.java > 40b08c414caa65e8d3a995d7713a14b6147af1d3 > > security-admin/src/main/java/org/apache/ranger/security/web/authentication/RangerAuthenticationEntryPoint.java > 52228ddc2ea824e2f3fa6bea383f81fee6cf4d7d > > security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java > PRE-CREATION > > security-admin/src/main/java/org/apache/ranger/security/web/filter/SSOAuthentication.java > PRE-CREATION > > security-admin/src/main/java/org/apache/ranger/security/web/filter/SSOAuthenticationProperties.java > PRE-CREATION > security-admin/src/main/resources/conf.dist/ranger-admin-site.xml > fe7320c79ee9c337acd2fc41fd34f610ca98dcd5 > security-admin/src/main/resources/conf.dist/security-applicationContext.xml > 162afc621c61d14b9089b79b2d6e9bb2b1d19850 > security-admin/src/main/webapp/scripts/utils/XAUtils.js > 8cb90e36df9fc08907b7aca74d510a4485f21ca1 > security-admin/src/main/webapp/scripts/views/common/ErrorView.js > a9d5739ac0b541e9d713fd234ce9d9ceade2e9d1 > security-admin/src/main/webapp/scripts/views/common/ProfileBar.js > 0f872708f31202fc9b3422b7672d88eae107dc2f > > Diff: https://reviews.apache.org/r/40343/diff/ > > > Testing > ------- > > Tested with local Ranger admin authentication against KNOX SSO and tried > various scenarios. > > > Thanks, > > Gautam Borad > >
