Skip to content

Cannot configure SecurityContextRepository in CasAuthenticationFilter #14529

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sammyhk opened this issue Feb 2, 2024 · 1 comment
Closed
Assignees
Labels
in: cas An issue in spring-security-cas type: bug A general bug
Milestone

Comments

@sammyhk
Copy link

sammyhk commented Feb 2, 2024

Describe the bug
CasAuthenticationFilter set a reference of SecurityContextRepository (

setSecurityContextRepository(this.securityContextRepository);
) in itself and use it in
this.securityContextRepository.saveContext(context, request, response);
which cause the setSecurityContextRepository(...) defined in parent class AbstractAuthenticationProcessingFilter not configurable anymore.
The securityContextRepository reference is just for the call of successfulAuthentication(...) (
this.logger.debug(
LogMessage.format("Authentication success. Updating SecurityContextHolder to contain: %s", authResult));
SecurityContext context = this.securityContextHolderStrategy.createEmptyContext();
context.setAuthentication(authResult);
this.securityContextHolderStrategy.setContext(context);
this.securityContextRepository.saveContext(context, request, response);
if (this.eventPublisher != null) {
this.eventPublisher.publishEvent(new InteractiveAuthenticationSuccessEvent(authResult, this.getClass()));
}
).
For me, seems like it is just duplicating the code defined in parent class AbstractAuthenticationProcessingFilter (
SecurityContext context = this.securityContextHolderStrategy.createEmptyContext();
context.setAuthentication(authResult);
this.securityContextHolderStrategy.setContext(context);
this.securityContextRepository.saveContext(context, request, response);
if (this.logger.isDebugEnabled()) {
this.logger.debug(LogMessage.format("Set SecurityContextHolder to %s", authResult));
}
this.rememberMeServices.loginSuccess(request, response, authResult);
if (this.eventPublisher != null) {
this.eventPublisher.publishEvent(new InteractiveAuthenticationSuccessEvent(authResult, this.getClass()));
}
this.successHandler.onAuthenticationSuccess(request, response, authResult);
) and can be rewritten to avoid the securityContextRepository reference defined in CasAuthenticationFilter.
Example:

@Override
protected final void successfulAuthentication(HttpServletRequest request, HttpServletResponse response,
		FilterChain chain, Authentication authResult) throws IOException, ServletException {
	boolean continueFilterChain = proxyTicketRequest(serviceTicketRequest(request, response), request);
	super.successfulAuthentication(request, response, chain, authResult);
	if (continueFilterChain) {
		chain.doFilter(request, response);
	}
}

Expected behavior
CasAuthenticationFilter should be able to configure different SecurityContextRepository by calling setSecurityContextRepository(...)

@sammyhk sammyhk added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Feb 2, 2024
kse-music added a commit to kse-music/spring-security that referenced this issue Feb 2, 2024
kse-music added a commit to kse-music/spring-security that referenced this issue Feb 2, 2024
@marcusdacoregio marcusdacoregio self-assigned this Feb 2, 2024
@marcusdacoregio marcusdacoregio added in: cas An issue in spring-security-cas and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 2, 2024
@marcusdacoregio marcusdacoregio added this to the 6.1.7 milestone Feb 2, 2024
@marcusdacoregio
Copy link
Contributor

Hi, @sammyhk. Thanks for the report.

If we apply the changes as you suggested, there is a test that stops passing where it expects the AuthenticationSuccessHandler to not be called. I don't think that we should apply a change that might break things for others.

The code duplication is not a problem here, we should probably override setSecurityContextRepository to call super and also set it into the CasAuthenticationFilter#securityContextRepository. The same goes for SecurityContextHolderStrategy.

@marcusdacoregio marcusdacoregio changed the title CasAuthenticationFilter cannot configured SecurityContextRepository Cannot configure SecurityContextRepository in CasAuthenticationFilter Feb 2, 2024
kse-music added a commit to kse-music/spring-security that referenced this issue Feb 3, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…n the parent class

Closes spring-projectsgh-14529
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: cas An issue in spring-security-cas type: bug A general bug
Projects
Status: No status
2 participants