Conversation
Fixed line separator issue
Fixed line separator issue
…nto issue/466/handle-auth-details # Conflicts: # core/src/main/java/org/springframework/security/saml/SAMLAuthenticationProvider.java # core/src/main/java/org/springframework/security/saml/SAMLProcessingFilter.java # core/src/test/java/org/springframework/security/saml/SAMLAuthenticationProviderTest.java # core/src/test/java/org/springframework/security/saml/SAMLProcessingFilterTest.java
jzheaux
left a comment
jzheaux
left a comment
There was a problem hiding this comment.
Thanks, @dellekappa, for the PR! I've left some feedback inline.
| * @param authRequest the authentication request object that should have its details | ||
| * set | ||
| */ | ||
| protected void setDetails(HttpServletRequest request, |
There was a problem hiding this comment.
We don't want to introduce a new API in a maintenance release. Since no further minor releases are planned, let's leave this out.
Truthfully, it's likely not needed anyway since the user already gets full control by wiring an AuthenticationDetailsSource.
| SAMLAuthenticationToken token = new SAMLAuthenticationToken(context); | ||
|
|
||
| // Allow subclasses to set the "details" property | ||
| setDetails(request, token); |
There was a problem hiding this comment.
In lieu of calling a new method here, I'd recommend it simply call this.token.setDetails(details) directly.
| SAMLCredential authenticationCredential = excludeCredential ? null : credential; | ||
| ExpiringUsernameAuthenticationToken result = new ExpiringUsernameAuthenticationToken(expiration, principal, authenticationCredential, entitlements); | ||
| result.setDetails(userDetails); | ||
| result.setDetails(authentication.getDetails()); |
There was a problem hiding this comment.
Sadly, I don't think that we can main this change inside of a maintenance release since it's possible that there are applications that are calling getDetails and expecting the userDetails object to come back. By changing it to the result of AuthenticationDetailsSource, we'd break those applications.
Instead, what an application would probably need to do is create a custom authentication provider like so:
public class MyAuthenticationProvider implements AuthenticationProvider {
private final SAMLAuthenticationProvider delegate;
// .. constructor
@Override
public Authentication authenticate(Authentication authentication) {
ExpiringUsernameAuthenticationToken result = (ExpiringUsernameAuthenticationToken)
this.delegate.authenticate(authentication);
result.setDetails(authentication.getDetails());
return result;
}
}| expect(request.getRequestURL()).andReturn(new StringBuffer("http://localhost:8081/spring-security-saml2-webapp/saml/SSO")); | ||
| expect(request.getQueryString()).andReturn(null); | ||
| expect(request.getRemoteAddr()).andReturn(null); | ||
| expect(request.getSession(false)).andReturn(null); |
There was a problem hiding this comment.
Please check the whitespacing here.
jzheaux
added
status: waiting-for-feedback
2 participants
No description provided.