You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Nixon Rodrigues <ni...@freestoneinfotech.com> on 2016/11/03 12:56:58 UTC
Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/
-----------------------------------------------------------
Review request for atlas, Ankita Sinha, keval bhatt, Madhan Neethiraj, Shwetha GS, and Suma Shivaprasad.
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
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/#review156010
-----------------------------------------------------------
Ship it!
Ship It!
- Madhan Neethiraj
On Nov. 11, 2016, 3:07 p.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53426/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2016, 3:07 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/AtlasKnoxSSOAuthenticationFilter.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
>
>
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/
-----------------------------------------------------------
(Updated Nov. 11, 2016, 3:07 p.m.)
Review request for atlas, Ankita Sinha, keval bhatt, Madhan Neethiraj, Shwetha GS, and Suma Shivaprasad.
Changes
-------
This patch handles review comments / code optimizations suggested by Madhan.
Renamed Filter classed to AtlasKnoxSSOAuthenticationFilter.
Ignore the 4th version since all did'nt go in it.
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 (updated)
-----
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/AtlasKnoxSSOAuthenticationFilter.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
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/
-----------------------------------------------------------
(Updated Nov. 11, 2016, 2:39 p.m.)
Review request for atlas, Ankita Sinha, keval bhatt, Madhan Neethiraj, Shwetha GS, and Suma Shivaprasad.
Changes
-------
This patch handles review comments / code optimizations suggested by Madhan.
Renamed Filter classed to AtlasKnoxSSOAuthenticationFilter
Thanks
Nixon
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 (updated)
-----
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/AtlasKnoxSSOAuthenticationFilter.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
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
-----------------------------------------------------------
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.
Changes
-------
This patch handles review comments from Madhan.
changed property name to add knox in it.
########### KNOX Configs ################
atlas.sso.knox.browser.useragent=Mozilla,Chrome,Opera
atlas.sso.knox.enabled=true
atlas.sso.knox.providerurl=https://<knox gateway ip>:8443/gateway/knoxsso/api/v1/websso
atlas.sso.knox.publicKey=
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 (updated)
-----
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
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
> On Nov. 7, 2016, 6:44 p.m., Madhan Neethiraj wrote:
> > webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java, line 352
> > <https://reviews.apache.org/r/53426/diff/2/?file=1555423#file1555423line352>
> >
> > getJwtProperties() - the method seems to be initializing this.jwtProperties. Is this intentional? Perhaps this should go into loadJwtProperties() and have getJwtProperties() simply return this.jwtProperties.
method loadJwtProperties() is responsible of loading SSO configs from atlasPropertyConfigs to SSOAuthenticationProperties bean and method setJwtProperties() is responsible for setting values to member variable authenticationProviderUrl,publicKey etc.
- Nixon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/#review155171
-----------------------------------------------------------
On Nov. 7, 2016, 12:59 p.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53426/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 12:59 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
>
>
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/#review155171
-----------------------------------------------------------
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 112)
<https://reviews.apache.org/r/53426/#comment225009>
as a good coding practice, enclose 'if' body within "{}"
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 127)
<https://reviews.apache.org/r/53426/#comment225012>
as a good coding practice, enclose 'if' body within "{}"
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 162)
<https://reviews.apache.org/r/53426/#comment225013>
as a good coding practice, enclose 'if' body within "{}"
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 171)
<https://reviews.apache.org/r/53426/#comment225014>
as a good coding practice, enclose 'if' body within "{}"
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 190)
<https://reviews.apache.org/r/53426/#comment225015>
Consider replacing with: StringUtils.startsWithIgnoreCase(userAgent, ua)
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 233)
<https://reviews.apache.org/r/53426/#comment225017>
Consider moving "cookieName != null" from here to line #231.
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 234)
<https://reviews.apache.org/r/53426/#comment225016>
as a good coding practice, enclose 'if' body within "{}"
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 352)
<https://reviews.apache.org/r/53426/#comment225019>
getJwtProperties() - the method seems to be initializing this.jwtProperties. Is this intentional? Perhaps this should go into loadJwtProperties() and have getJwtProperties() simply return this.jwtProperties.
- Madhan Neethiraj
On Nov. 7, 2016, 12:59 p.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53426/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 12:59 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
>
>
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/
-----------------------------------------------------------
(Updated Nov. 7, 2016, 12:59 p.m.)
Review request for atlas, Ankita Sinha, keval bhatt, Madhan Neethiraj, Shwetha GS, and Suma Shivaprasad.
Changes
-------
This patch includes fix review comments on earlier version
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 (updated)
-----
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
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Apoorv Naik <na...@gmail.com>.
> On Nov. 3, 2016, 5:33 p.m., Apoorv Naik wrote:
> > webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java, line 125
> > <https://reviews.apache.org/r/53426/diff/1/?file=1553045#file1553045line125>
> >
> > Parameterized string instead of concatenation.
> >
> > ```java
> > LOG.debug("Knox doFilter {}", httpRequest.getRequestURI());
> > ```
Also encapsulate within if (LOG.isDebugEnabled) {} check. Right now there's a mix of debug calls, some with this check and some without.
- Apoorv
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/#review154772
-----------------------------------------------------------
On Nov. 3, 2016, 1:09 p.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53426/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2016, 1:09 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
>
>
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
> On Nov. 3, 2016, 5:33 p.m., Apoorv Naik wrote:
> > webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java, line 404
> > <https://reviews.apache.org/r/53426/diff/1/?file=1553045#file1553045line404>
> >
> > Why not use standard library to read the key from the file directly.
> >
> > Would the following work in this case ?
> >
> > ```java
> > import java.io.*;
> > import java.nio.*;
> > import java.security.*;
> > import java.security.spec.*;
> >
> > public class PublicKeyReader {
> >
> > public static PublicKey get(String filename)
> > throws Exception {
> >
> > byte[] keyBytes = Files.readAllBytes(new File(filename).toPath());
> >
> > X509EncodedKeySpec spec =
> > new X509EncodedKeySpec(keyBytes);
> > KeyFactory kf = KeyFactory.getInstance("RSA");
> > return kf.generatePublic(spec);
> > }
> > }
> >
> > ```
Yes, we can use this approach of reading file also but adding public key from Ambari UI will be handy without requiring server access for copying the knox public key.
- Nixon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/#review154772
-----------------------------------------------------------
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
>
>
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Apoorv Naik <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/#review154772
-----------------------------------------------------------
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 125)
<https://reviews.apache.org/r/53426/#comment224453>
Parameterized string instead of concatenation.
```java
LOG.debug("Knox doFilter {}", httpRequest.getRequestURI());
```
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 263)
<https://reviews.apache.org/r/53426/#comment224454>
Maybe a StringBuilder or String.format will be a better option instead of multiple concatenations.
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 347)
<https://reviews.apache.org/r/53426/#comment224455>
No need of concatenation here.
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 379)
<https://reviews.apache.org/r/53426/#comment224456>
Looks confusing, the expected parameter is "String pem" and here the public key path is being passed.
webapp/src/main/java/org/apache/atlas/web/filters/AtlasSSOAuthenticationFilter.java (line 404)
<https://reviews.apache.org/r/53426/#comment224458>
Why not use standard library to read the key from the file directly.
Would the following work in this case ?
```java
import java.io.*;
import java.nio.*;
import java.security.*;
import java.security.spec.*;
public class PublicKeyReader {
public static PublicKey get(String filename)
throws Exception {
byte[] keyBytes = Files.readAllBytes(new File(filename).toPath());
X509EncodedKeySpec spec =
new X509EncodedKeySpec(keyBytes);
KeyFactory kf = KeyFactory.getInstance("RSA");
return kf.generatePublic(spec);
}
}
```
- Apoorv Naik
On Nov. 3, 2016, 1:09 p.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53426/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2016, 1:09 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
>
>
Re: Review Request 53426: ATLAS-1244 - Atlas to Support KnoxSSO
Authentication
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53426/
-----------------------------------------------------------
(Updated Nov. 3, 2016, 1:09 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