atlas-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 53426: ATLAS-1244 - Atlas to Support KnoxSSO Authentication
Date Fri, 11 Nov 2016 03:10:32 GMT

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




webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line
110)
<https://reviews.apache.org/r/53426/#comment225762>

    Consider rearranging the code for early bailouts - for efficiency and better readability:
    
    if (!ssoEnabled) {
      filterChain.doFilter(servletRequest, servletResponse);
      return;
    }
    
    HttpServletRequest httpRequest = (HttpServletRequest) servletRequest;
    
    if (httpRequest.getSession() != null && httpRequest.getSession().getAttribute("locallogin")
!= null)) {
      servletRequest.setAttribute("ssoEnabled", false);
    
      filterChain.doFilter(servletRequest, servletResponse);
      return;
    }
    
    if (!isWebUserAgent(httpRequest.getHeader("User-Agent")) ||
        jwtProperties == null || !isAuthenticated()) {
      filterChain.doFilter(servletRequest, servletResponse);
      return;
    }
    
    HttpServletResponse httpServletResponse = (HttpServletResponse) servletResponse;
    String              serializedJWT       = getJWTFromCookie(httpRequest);
    
    if (serializedJWT != null) {



webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line
117)
<https://reviews.apache.org/r/53426/#comment225761>

    Since the value of  configuration.getBoolean("atlas.sso.knox.enabled") will not change,
consider avoiding this config lookup by initializing the value in a member variable in the
constructor.



webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line
282)
<https://reviews.apache.org/r/53426/#comment225763>

    validateExpiration() call does not seem necessary when validateSignature() returns false.
Consider rewritting as below:
    
            boolean ret = validateSignature(jwtToken);
    
            if (ret) {
              ret = validateExpiration(jwtToken);
    
              if (!ret) {
                LOG.warn("Expiration time validation of JWT token failed.");
              }
            } else
                LOG.warn("Signature of JWT token could not be verified. Please check the public
key");
            }
    
            return ret;



webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line
313)
<https://reviews.apache.org/r/53426/#comment225764>

    is it necessary to instantiate RSASSAVerifier() in every call? Since 'publicKey' only
changes in setJwtProperties(), consider instantiting RSASSAVerifier() in setJwtProperties()
and use that instance here.



webapp/src/main/java/org/apache/atlas/web/filters/SSOAuthentication.java (line 33)
<https://reviews.apache.org/r/53426/#comment225765>

    if 'token' is set only in the constructor, consider marking this as 'final'


- Madhan Neethiraj


On Nov. 9, 2016, 1:02 p.m., Nixon Rodrigues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53426/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2016, 1:02 p.m.)
> 
> 
> Review request for atlas, Ankita Sinha, keval bhatt, Madhan Neethiraj, Shwetha GS, and
Suma Shivaprasad.
> 
> 
> Bugs: ATLAS-1244
>     https://issues.apache.org/jira/browse/ATLAS-1244
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> This patch includes new filter for Atlas to Support KnoxSSO Authentication.
> 
> 
> Diffs
> -----
> 
>   distro/src/conf/atlas-application.properties 0349ccc 
>   webapp/pom.xml 6dbd484 
>   webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java 30200b5

>   webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java
PRE-CREATION 
>   webapp/src/main/java/org/apache/atlas/web/filters/SSOAuthentication.java PRE-CREATION

>   webapp/src/main/java/org/apache/atlas/web/filters/SSOAuthenticationProperties.java
PRE-CREATION 
>   webapp/src/main/java/org/apache/atlas/web/security/AtlasAbstractAuthenticationProvider.java
595387a 
>   webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationProvider.java
23d3d70 
>   webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
8654716 
>   webapp/src/main/resources/spring-security.xml ea9aa94 
>   webapp/src/main/webapp/WEB-INF/web.xml 7c6bc6d 
>   webapp/src/test/webapp/WEB-INF/web.xml 1b152ee 
> 
> Diff: https://reviews.apache.org/r/53426/diff/
> 
> 
> Testing
> -------
> 
> Verified Knox SSO authentication.
> Verified basic authentication process
> Verified form based authentication process
> 
> All existing test cases passing.
> 
> 
> Thanks,
> 
> Nixon Rodrigues
> 
>


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