ranger-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Madhan Neethiraj" <mad...@apache.org>
Subject Re: Review Request 40343: RANGER-685 : Add Ability to Authenticate with Knox SSO provider
Date Tue, 17 Nov 2015 08:37:26 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40343/#review106808
-----------------------------------------------------------



security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java
(line 89)
<https://reviews.apache.org/r/40343/#comment165601>

    Consider splitting the if/else blocks for better readabilty. After rearranging the code,
it is clear some branches are not handled. Please review all branches and update.
    
    if(ssoEnabled && isWebUserAgent(userAgent)) {
      if (!isAuthenticated) {
        if(jwtProperties != null {
          // do SSO stuff
        }
      } else {
        if(((HttpServletRequest) servletRequest).getRequestURI().contains(LOCAL_LOGIN_URL))
{
          String url = ((HttpServletRequest) servletRequest).getRequestURI().replace(LOCAL_LOGIN_URL+"/",
"");				
          url = url.replace(LOCAL_LOGIN_URL, "");
          LOG.warn("There is an active session and if you want local login to ranger, try
this on a separate browser");
    	  ((HttpServletResponse)servletResponse).sendRedirect(url);
        }
      }
    } else {
      filterChain.doFilter(servletRequest, servletResponse);
    }



security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java
(line 90)
<https://reviews.apache.org/r/40343/#comment165598>

    Shouldn't this be "! isAuthenticated()"?



security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java
(line 136)
<https://reviews.apache.org/r/40343/#comment165599>

    Looks like "!isAuthenticated()" should be "isAuthenticated()" - please review.



security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java
(line 193)
<https://reviews.apache.org/r/40343/#comment165602>

    The return value should be the reverse of what it was before renaming the method. Please
review.



security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerSSOAuthenticationFilter.java
(line 113)
<https://reviews.apache.org/r/40343/#comment165603>

    Where does the groups/roles for the user are set? For example, if admin login using SSO,
would ROLE_SYS_ADMIN be set? or only the default role, ROLE_USER be set in the session?


- Madhan Neethiraj


On Nov. 17, 2015, 8:13 a.m., Gautam Borad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40343/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2015, 8:13 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message