Skip to content

Remove GET request support from Saml2AuthenticationTokenConverter #17108

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+23 −6
Open

Remove GET request support from Saml2AuthenticationTokenConverter #17108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions ...pringframework/security/saml2/provider/service/web/Saml2AuthenticationTokenConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2022 the original author or authors.
* Copyright 2002-2025 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -43,6 +43,8 @@ public final class Saml2AuthenticationTokenConverter implements AuthenticationCo

private Saml2AuthenticationRequestRepository<AbstractSaml2AuthenticationRequest> authenticationRequestRepository;

private Boolean shouldInflate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use boolean and give it an initial value of true. We typically reserve null in Boolean for "no opinion".


/**
* Constructs a {@link Saml2AuthenticationTokenConverter} given a strategy for
* resolving {@link RelyingPartyRegistration}s
Expand Down Expand Up @@ -86,16 +88,26 @@ public void setAuthenticationRequestRepository(
this.authenticationRequestRepository = authenticationRequestRepository;
}

/**
* Use the given {@code shouldInflate} to inflate request.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please indicate the default value in this JavaDoc

* @param shouldInflate the {@code shouldInflate} to use
* @since 7.0
*/
public void setShouldInflateResponse(boolean shouldInflate) {
this.shouldInflate = shouldInflate;
}

private String decode(HttpServletRequest request) {
// prevent to break passivity in Saml2LoginBeanDefinitionParserTests
if (this.shouldInflate == null) {
this.shouldInflate = HttpMethod.GET.matches(request.getMethod());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not thread-safe since multiple requests will use the same instance of this class. Please approach this in a way that does not involve updating a member variable.

}
String encoded = request.getParameter(Saml2ParameterNames.SAML_RESPONSE);
if (encoded == null) {
return null;
}
try {
return Saml2Utils.withEncoded(encoded)
.requireBase64(true)
.inflate(HttpMethod.GET.matches(request.getMethod()))
.decode();
return Saml2Utils.withEncoded(encoded).requireBase64(true).inflate(this.shouldInflate).decode();
}
catch (Exception ex) {
throw new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.INVALID_RESPONSE, ex.getMessage()),
Expand Down
7 changes: 6 additions & 1 deletion ...framework/security/saml2/provider/service/web/Saml2AuthenticationTokenConverterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2025 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -61,6 +61,7 @@ public class Saml2AuthenticationTokenConverterTests {
public void convertWhenSamlResponseThenToken() {
Saml2AuthenticationTokenConverter converter = new Saml2AuthenticationTokenConverter(
this.relyingPartyRegistrationResolver);
converter.setShouldInflateResponse(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't alter any unit tests. In this way, we can have high confidence that the change is a passive one.

That said, I believe adding a test would be a good idea. I think we should test when shouldInflate is false and the request is a GET.

Copy link
Contributor Author

@ngocnhan-tran1996 ngocnhan-tran1996 May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't alter any unit tests.

This is unclear to me. I've made changes on my local machine, and some tests will fail. We should either set the value to false to keep it passive, or update the expected results in the tests. Which option should we choose?

given(this.relyingPartyRegistrationResolver.resolve(any(HttpServletRequest.class), any()))
.willReturn(this.relyingPartyRegistration);
MockHttpServletRequest request = new MockHttpServletRequest();
Expand All @@ -76,6 +77,7 @@ public void convertWhenSamlResponseThenToken() {
public void convertWhenSamlResponseWithRelyingPartyRegistrationResolver(
@Mock RelyingPartyRegistrationResolver resolver) {
Saml2AuthenticationTokenConverter converter = new Saml2AuthenticationTokenConverter(resolver);
converter.setShouldInflateResponse(false);
given(resolver.resolve(any(HttpServletRequest.class), any())).willReturn(this.relyingPartyRegistration);
MockHttpServletRequest request = new MockHttpServletRequest();
request.setParameter(Saml2ParameterNames.SAML_RESPONSE,
Expand Down Expand Up @@ -161,6 +163,7 @@ public void convertWhenGetRequestInvalidDeflatedThenSaml2AuthenticationException
public void convertWhenUsingSamlUtilsBase64ThenXmlIsValid() throws Exception {
Saml2AuthenticationTokenConverter converter = new Saml2AuthenticationTokenConverter(
this.relyingPartyRegistrationResolver);
converter.setShouldInflateResponse(false);
given(this.relyingPartyRegistrationResolver.resolve(any(HttpServletRequest.class), any()))
.willReturn(this.relyingPartyRegistration);
MockHttpServletRequest request = new MockHttpServletRequest();
Expand All @@ -178,6 +181,7 @@ public void convertWhenSavedAuthenticationRequestThenToken() {
.willReturn(this.relyingPartyRegistration.getRegistrationId());
Saml2AuthenticationTokenConverter converter = new Saml2AuthenticationTokenConverter(
this.relyingPartyRegistrationResolver);
converter.setShouldInflateResponse(false);
converter.setAuthenticationRequestRepository(authenticationRequestRepository);
given(this.relyingPartyRegistrationResolver.resolve(any(HttpServletRequest.class), any()))
.willReturn(this.relyingPartyRegistration);
Expand All @@ -203,6 +207,7 @@ public void convertWhenSavedAuthenticationRequestThenTokenWithRelyingPartyRegist
.willReturn(this.relyingPartyRegistration.getRegistrationId());
Saml2AuthenticationTokenConverter converter = new Saml2AuthenticationTokenConverter(resolver);
converter.setAuthenticationRequestRepository(authenticationRequestRepository);
converter.setShouldInflateResponse(false);
given(resolver.resolve(any(HttpServletRequest.class), any())).willReturn(this.relyingPartyRegistration);
given(authenticationRequestRepository.loadAuthenticationRequest(any(HttpServletRequest.class)))
.willReturn(authenticationRequest);
Expand Down