Conversation
|
Hello @tuteng @eolivelli Please take a look, I will add test cases for same if changes good to you. |
|
This is an important change for us. Pulsar manager not integrating with our SSO is a huge roadblock for pulsar manager adoption. Great work @sourabhaggrawal . Lets sprint to merge this quickly :-) |
shiv4289
left a comment
shiv4289
left a comment
There was a problem hiding this comment.
Hi @sourabhaggrawal Left a few comments. Please check out.
| @RequestMapping(value = "/sso", method = RequestMethod.POST) | ||
| @ResponseBody | ||
| public void loginByIDP(HttpServletRequest request, HttpServletResponse response) throws Exception { | ||
| String samlResponse = request.getParameter("SAMLResponse"); |
There was a problem hiding this comment.
Suggest to check the call is a SAML response before using it.
Moreover, lets be neutral regarding the sso method here? The config and api endpoint are generic. The implementation assumes SAML. We should check the type of auth and switch to a branch of code (factory?) which gives us (authenticated / not authenticated) as a result.
| String samlResponse = request.getParameter("SAMLResponse"); | ||
| SAMLParser samlParser = new SAMLParser(certificate); | ||
| Map<String,String> userDetails = samlParser.getUserDetailsFromSAMLResponse(samlResponse); | ||
| Asserts.check(StringUtils.contains(userDetails.get("email"),whiteListedDomain),"Login with domain not supported"); |
There was a problem hiding this comment.
Suggest to check the email format in string(using InternetAddress()?) in addition to the domain.
There was a problem hiding this comment.
I am wondering what we will achieve with the email format validation , the email is extracted from SAML Response which is served through IDP directly.
| .excludePathPatterns("/doc.html") | ||
| // BKVM | ||
| .excludePathPatterns("/bkvm") | ||
| .excludePathPatterns("/pulsar-manager/saml/**") |
There was a problem hiding this comment.
our endpoint is /sso right? Should this be /pulsar-manager/sso/**
There was a problem hiding this comment.
Also why ** ? can we do with just /sso without ** ?
There was a problem hiding this comment.
our endpoint is /saml/sso @shiv4289
here /saml is mapped at controller level and ** to exclude all method in that controller from authentication.
This makes me rethink to change the excludePathPatterns as /pulsar-manager/saml/sso to make only one method accessible without authentication.
3 participants
Motivation
Idea is to support authentication from ThirdParty using SAML Approach. Changes are backward compatible and existing Login Approach using credentials will continue to work.
Provide Third Party Login Support Using SAML
Modifications
Changes to login with ThirdParty using SAML implementation.
Verifying this change
./gradlew buildchecks.