----------------------------------------------------------- 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 > >
