Skip to content

Commit edfd7b9

Browse files
1livvjzheaux
1livv
authored and
jzheaux
committed
Addressed review comments
Signed-off-by: Liviu Gheorghe <[email protected]>
1 parent 358f6c9 commit edfd7b9

File tree

8 files changed

+59
-84
lines changed

8 files changed

+59
-84
lines changed

‎config/src/test/java/org/springframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurerTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,7 @@ public void saml2LogoutRequestWhenLowercaseEncodingAndDifferentQueryParamOrderTh
371371
}
372372

373373
@Test
374-
public void saml2LogoutRequestWhenNoRegistrationThen401() throws Exception {
374+
public void saml2LogoutRequestWhenNoRegistrationThen400() throws Exception {
375375
this.spring.register(Saml2LogoutDefaultsConfig.class).autowire();
376376
DefaultSaml2AuthenticatedPrincipal principal = new DefaultSaml2AuthenticatedPrincipal("user",
377377
Collections.emptyMap());
@@ -384,7 +384,7 @@ public void saml2LogoutRequestWhenNoRegistrationThen401() throws Exception {
384384
.param("SigAlg", this.apLogoutRequestSigAlg)
385385
.param("Signature", this.apLogoutRequestSignature)
386386
.with(authentication(user)))
387-
.andExpect(status().isUnauthorized());
387+
.andExpect(status().isBadRequest());
388388
verifyNoInteractions(getBean(LogoutHandler.class));
389389
}
390390

‎config/src/test/java/org/springframework/security/config/http/Saml2LogoutBeanDefinitionParserTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ public void saml2LogoutRequestWhenCustomSecurityContextHolderStrategyThenUses()
271271
}
272272

273273
@Test
274-
public void saml2LogoutRequestWhenNoRegistrationThen401() throws Exception {
274+
public void saml2LogoutRequestWhenNoRegistrationThen400() throws Exception {
275275
this.spring.configLocations(this.xml("Default")).autowire();
276276
DefaultSaml2AuthenticatedPrincipal principal = new DefaultSaml2AuthenticatedPrincipal("user",
277277
Collections.emptyMap());
@@ -284,7 +284,7 @@ public void saml2LogoutRequestWhenNoRegistrationThen401() throws Exception {
284284
.param("SigAlg", this.apLogoutRequestSigAlg)
285285
.param("Signature", this.apLogoutRequestSignature)
286286
.with(authentication(user)))
287-
.andExpect(status().isUnauthorized());
287+
.andExpect(status().isBadRequest());
288288
}
289289

290290
@Test

‎saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/core/Saml2ErrorCodes.java

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -130,30 +130,6 @@ public final class Saml2ErrorCodes {
130130
*/
131131
public static final String INVALID_IN_RESPONSE_TO = "invalid_in_response_to";
132132

133-
/**
134-
* The RP registration does not have configured a logout request endpoint
135-
* @since 6.3
136-
*/
137-
public static final String MISSING_LOGOUT_REQUEST_ENDPOINT = "missing_logout_request_endpoint";
138-
139-
/**
140-
* The saml response or logout request was delivered via an invalid binding
141-
* @since 6.3
142-
*/
143-
public static final String INVALID_BINDING = "invalid_binding";
144-
145-
/**
146-
* The saml logout request failed validation
147-
* @since 6.3
148-
*/
149-
public static final String INVALID_LOGOUT_REQUEST = "invalid_logout_request";
150-
151-
/**
152-
* The saml logout response could not be generated
153-
* @since 6.3
154-
*/
155-
public static final String FAILED_TO_GENERATE_LOGOUT_RESPONSE = "failed_to_generate_logout_response";
156-
157133
private Saml2ErrorCodes() {
158134
}
159135

‎saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/web/authentication/logout/BaseOpenSamlLogoutResponseResolver.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -240,9 +240,8 @@ private String serialize(LogoutResponse logoutResponse) {
240240
private String getSamlStatus(Saml2AuthenticationException exception) {
241241
Saml2Error saml2Error = exception.getSaml2Error();
242242
return switch (saml2Error.getErrorCode()) {
243-
case Saml2ErrorCodes.MISSING_LOGOUT_REQUEST_ENDPOINT, Saml2ErrorCodes.INVALID_BINDING ->
244-
StatusCode.REQUEST_DENIED;
245-
case Saml2ErrorCodes.INVALID_LOGOUT_REQUEST -> StatusCode.REQUESTER;
243+
case Saml2ErrorCodes.INVALID_DESTINATION -> StatusCode.REQUEST_DENIED;
244+
case Saml2ErrorCodes.INVALID_REQUEST -> StatusCode.REQUESTER;
246245
default -> StatusCode.RESPONDER;
247246
};
248247
}

‎saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/web/authentication/logout/Saml2LogoutRequestFilter.java

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -113,27 +113,44 @@ public Saml2LogoutRequestFilter(RelyingPartyRegistrationResolver relyingPartyReg
113113
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain)
114114
throws ServletException, IOException {
115115
Authentication authentication = this.securityContextHolderStrategy.getContext().getAuthentication();
116+
Saml2LogoutRequestValidatorParameters parameters;
116117
try {
117-
Saml2LogoutRequestValidatorParameters parameters = this.logoutRequestResolver.resolve(request,
118-
authentication);
119-
if (parameters == null) {
120-
chain.doFilter(request, response);
121-
return;
122-
}
118+
parameters = this.logoutRequestResolver.resolve(request, authentication);
119+
}
120+
catch (Saml2AuthenticationException ex) {
121+
this.logger.trace("Did not process logout request since failed to find requested RelyingPartyRegistration");
122+
response.sendError(HttpServletResponse.SC_BAD_REQUEST);
123+
return;
124+
}
125+
if (parameters == null) {
126+
chain.doFilter(request, response);
127+
return;
128+
}
123129

124-
Saml2LogoutResponse logoutResponse = processLogoutRequest(request, response, authentication, parameters);
125-
sendLogoutResponse(request, response, logoutResponse);
130+
try {
131+
validateLogoutRequest(request, parameters);
126132
}
127133
catch (Saml2AuthenticationException ex) {
128134
Saml2LogoutResponse errorLogoutResponse = this.logoutResponseResolver.resolve(request, authentication, ex);
129135
if (errorLogoutResponse == null) {
130-
this.logger.trace("Returning error since no error logout response could be generated", ex);
136+
this.logger.trace(LogMessage.format(
137+
"Returning error since no error logout response could be generated: %s", ex.getSaml2Error()));
131138
response.sendError(HttpServletResponse.SC_UNAUTHORIZED);
132139
return;
133140
}
134141

135142
sendLogoutResponse(request, response, errorLogoutResponse);
143+
return;
136144
}
145+
146+
this.handler.logout(request, response, authentication);
147+
Saml2LogoutResponse logoutResponse = this.logoutResponseResolver.resolve(request, authentication);
148+
if (logoutResponse == null) {
149+
this.logger.trace("Returning error since no logout response generated");
150+
response.sendError(HttpServletResponse.SC_UNAUTHORIZED);
151+
return;
152+
}
153+
sendLogoutResponse(request, response, logoutResponse);
137154
}
138155

139156
public void setLogoutRequestMatcher(RequestMatcher logoutRequestMatcher) {
@@ -155,38 +172,28 @@ public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy secur
155172
this.securityContextHolderStrategy = securityContextHolderStrategy;
156173
}
157174

158-
private Saml2LogoutResponse processLogoutRequest(HttpServletRequest request, HttpServletResponse response,
159-
Authentication authentication, Saml2LogoutRequestValidatorParameters parameters) {
175+
private void validateLogoutRequest(HttpServletRequest request, Saml2LogoutRequestValidatorParameters parameters) {
160176
RelyingPartyRegistration registration = parameters.getRelyingPartyRegistration();
161177
if (registration.getSingleLogoutServiceLocation() == null) {
162178
this.logger.trace(
163179
"Did not process logout request since RelyingPartyRegistration has not been configured with a logout request endpoint");
164-
throw new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.MISSING_LOGOUT_REQUEST_ENDPOINT,
180+
throw new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.INVALID_DESTINATION,
165181
"RelyingPartyRegistration has not been configured with a logout request endpoint"));
166182
}
167183

168184
Saml2MessageBinding saml2MessageBinding = Saml2MessageBindingUtils.resolveBinding(request);
169185
if (!registration.getSingleLogoutServiceBindings().contains(saml2MessageBinding)) {
170186
this.logger.trace("Did not process logout request since used incorrect binding");
171187
throw new Saml2AuthenticationException(
172-
new Saml2Error(Saml2ErrorCodes.INVALID_BINDING, "Logout request used invalid binding"));
188+
new Saml2Error(Saml2ErrorCodes.INVALID_REQUEST, "Logout request used invalid binding"));
173189
}
174190

175191
Saml2LogoutValidatorResult result = this.logoutRequestValidator.validate(parameters);
176192
if (result.hasErrors()) {
177193
this.logger.debug(LogMessage.format("Failed to validate LogoutRequest: %s", result.getErrors()));
178194
throw new Saml2AuthenticationException(
179-
new Saml2Error(Saml2ErrorCodes.INVALID_LOGOUT_REQUEST, "Failed to validate the logout request"));
180-
}
181-
182-
this.handler.logout(request, response, authentication);
183-
Saml2LogoutResponse logoutResponse = this.logoutResponseResolver.resolve(request, authentication);
184-
if (logoutResponse == null) {
185-
this.logger.trace("Returning error since no logout response generated");
186-
throw new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.FAILED_TO_GENERATE_LOGOUT_RESPONSE,
187-
"Could not generated logout response"));
195+
new Saml2Error(Saml2ErrorCodes.INVALID_REQUEST, "Failed to validate the logout request"));
188196
}
189-
return logoutResponse;
190197
}
191198

192199
private void sendLogoutResponse(HttpServletRequest request, HttpServletResponse response,

‎saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/web/authentication/logout/Saml2LogoutResponseResolver.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,9 @@ public interface Saml2LogoutResponseResolver {
5353
* processed
5454
* @return a signed and serialized SAML 2.0 Logout Response
5555
*/
56-
Saml2LogoutResponse resolve(HttpServletRequest request, Authentication authentication,
57-
Saml2AuthenticationException authenticationException);
56+
default Saml2LogoutResponse resolve(HttpServletRequest request, Authentication authentication,
57+
Saml2AuthenticationException authenticationException) {
58+
return null;
59+
}
5860

5961
}

‎saml2/saml2-service-provider/src/opensaml4Test/java/org/springframework/security/saml2/provider/service/web/authentication/logout/OpenSaml4LogoutResponseResolverTests.java

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public void resolveWhenCustomParametersConsumerThenUses() {
6565
logoutResponseResolver.setParametersConsumer(parametersConsumer);
6666
MockHttpServletRequest request = new MockHttpServletRequest();
6767
RelyingPartyRegistration registration = TestRelyingPartyRegistrations.relyingPartyRegistration()
68-
.assertingPartyDetails(
68+
.assertingPartyMetadata(
6969
(party) -> party.singleLogoutServiceResponseLocation("https://ap.example.com/logout"))
7070
.build();
7171
Authentication authentication = new TestingAuthenticationToken("user", "password");
@@ -109,19 +109,10 @@ public void setParametersConsumerWhenNullThenIllegalArgument() {
109109

110110
private static Stream<Arguments> provideAuthExceptionAndExpectedSamlStatusCode() {
111111
return Stream.of(
112-
Arguments.of(
113-
new Saml2AuthenticationException(
114-
new Saml2Error(Saml2ErrorCodes.MISSING_LOGOUT_REQUEST_ENDPOINT, "")),
115-
StatusCode.REQUEST_DENIED),
116-
Arguments.of(new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.INVALID_BINDING, "")),
112+
Arguments.of(new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.INVALID_DESTINATION, "")),
117113
StatusCode.REQUEST_DENIED),
118-
Arguments.of(
119-
new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.INVALID_LOGOUT_REQUEST, "")),
120-
StatusCode.REQUESTER),
121-
Arguments.of(
122-
new Saml2AuthenticationException(
123-
new Saml2Error(Saml2ErrorCodes.FAILED_TO_GENERATE_LOGOUT_RESPONSE, "")),
124-
StatusCode.RESPONDER)
114+
Arguments.of(new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.INVALID_REQUEST, "")),
115+
StatusCode.REQUESTER)
125116

126117
);
127118
}

‎saml2/saml2-service-provider/src/test/java/org/springframework/security/saml2/provider/service/web/authentication/logout/Saml2LogoutRequestFilterTests.java

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ public void doFilterWhenSamlRequestThenRedirects() throws Exception {
9999
@Test
100100
public void doFilterWhenSamlRequestThenPosts() throws Exception {
101101
RelyingPartyRegistration registration = TestRelyingPartyRegistrations.full()
102-
.assertingPartyDetails((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
102+
.assertingPartyMetadata((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
103103
.build();
104104
Authentication authentication = new TestingAuthenticationToken("user", "password");
105105
given(this.securityContextHolderStrategy.getContext()).willReturn(new SecurityContextImpl(authentication));
@@ -149,7 +149,7 @@ public void doFilterWhenNoSamlRequestOrResponseThenNoLogout() throws Exception {
149149
@Test
150150
public void doFilterWhenValidationFailsErrorLogoutResponseIsPosted() throws Exception {
151151
RelyingPartyRegistration registration = TestRelyingPartyRegistrations.full()
152-
.assertingPartyDetails((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
152+
.assertingPartyMetadata((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
153153
.build();
154154
Authentication authentication = new TestingAuthenticationToken("user", "password");
155155
SecurityContextHolder.getContext().setAuthentication(authentication);
@@ -165,15 +165,15 @@ public void doFilterWhenValidationFailsErrorLogoutResponseIsPosted() throws Exce
165165
given(this.logoutRequestValidator.validate(any()))
166166
.willReturn(Saml2LogoutValidatorResult.withErrors(new Saml2Error("error", "description")).build());
167167
given(this.logoutResponseResolver.resolve(any(), any(),
168-
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_LOGOUT_REQUEST))))
168+
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_REQUEST))))
169169
.willReturn(logoutResponse);
170170

171171
this.logoutRequestProcessingFilter.doFilter(request, response, new MockFilterChain());
172172

173173
checkResponse(response.getContentAsString(), registration);
174174
verify(this.logoutRequestValidator).validate(any());
175175
verify(this.logoutResponseResolver).resolve(any(), any(),
176-
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_LOGOUT_REQUEST)));
176+
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_REQUEST)));
177177
verifyNoInteractions(this.logoutHandler);
178178
}
179179

@@ -186,23 +186,23 @@ public void doFilterWhenNoRelyingErrorLogoutResponseIsPosted() throws Exception
186186
request.setParameter(Saml2ParameterNames.SAML_REQUEST, "request");
187187
MockHttpServletResponse response = new MockHttpServletResponse();
188188
RelyingPartyRegistration registration = TestRelyingPartyRegistrations.full()
189-
.assertingPartyDetails((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
189+
.assertingPartyMetadata((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
190190
.singleLogoutServiceLocation(null)
191191
.build();
192192
Saml2LogoutResponse logoutResponse = Saml2LogoutResponse.withRelyingPartyRegistration(registration)
193193
.samlResponse("response")
194194
.build();
195195

196196
given(this.relyingPartyRegistrationResolver.resolve(any(), any())).willReturn(registration);
197-
given(this.logoutResponseResolver.resolve(any(), any(), argThat(
198-
(ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.MISSING_LOGOUT_REQUEST_ENDPOINT))))
197+
given(this.logoutResponseResolver.resolve(any(), any(),
198+
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_DESTINATION))))
199199
.willReturn(logoutResponse);
200200

201201
this.logoutRequestProcessingFilter.doFilterInternal(request, response, new MockFilterChain());
202202

203203
checkResponse(response.getContentAsString(), registration);
204-
verify(this.logoutResponseResolver).resolve(any(), any(), argThat(
205-
(ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.MISSING_LOGOUT_REQUEST_ENDPOINT)));
204+
verify(this.logoutResponseResolver).resolve(any(), any(),
205+
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_DESTINATION)));
206206
verifyNoInteractions(this.logoutHandler);
207207
}
208208

@@ -215,7 +215,7 @@ public void doFilterWhenInvalidBindingErrorLogoutResponseIsPosted() throws Excep
215215
request.setParameter(Saml2ParameterNames.SAML_REQUEST, "request");
216216
MockHttpServletResponse response = new MockHttpServletResponse();
217217
RelyingPartyRegistration registration = TestRelyingPartyRegistrations.full()
218-
.assertingPartyDetails((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
218+
.assertingPartyMetadata((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
219219
.singleLogoutServiceBindings((bindings) -> {
220220
bindings.clear();
221221
bindings.add(Saml2MessageBinding.REDIRECT);
@@ -227,14 +227,14 @@ public void doFilterWhenInvalidBindingErrorLogoutResponseIsPosted() throws Excep
227227

228228
given(this.relyingPartyRegistrationResolver.resolve(any(), any())).willReturn(registration);
229229
given(this.logoutResponseResolver.resolve(any(), any(),
230-
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_BINDING))))
230+
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_REQUEST))))
231231
.willReturn(logoutResponse);
232232

233233
this.logoutRequestProcessingFilter.doFilterInternal(request, response, new MockFilterChain());
234234

235235
checkResponse(response.getContentAsString(), registration);
236236
verify(this.logoutResponseResolver).resolve(any(), any(),
237-
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_BINDING)));
237+
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_REQUEST)));
238238
verifyNoInteractions(this.logoutHandler);
239239
}
240240

@@ -247,7 +247,7 @@ public void doFilterWhenNoErrorResponseCanBeGeneratedThen401() throws Exception
247247
request.setParameter(Saml2ParameterNames.SAML_REQUEST, "request");
248248
MockHttpServletResponse response = new MockHttpServletResponse();
249249
RelyingPartyRegistration registration = TestRelyingPartyRegistrations.full()
250-
.assertingPartyDetails((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
250+
.assertingPartyMetadata((party) -> party.singleLogoutServiceBinding(Saml2MessageBinding.POST))
251251
.singleLogoutServiceBindings((bindings) -> {
252252
bindings.clear();
253253
bindings.add(Saml2MessageBinding.REDIRECT);
@@ -256,14 +256,14 @@ public void doFilterWhenNoErrorResponseCanBeGeneratedThen401() throws Exception
256256

257257
given(this.relyingPartyRegistrationResolver.resolve(any(), any())).willReturn(registration);
258258
given(this.logoutResponseResolver.resolve(any(), any(),
259-
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_BINDING))))
259+
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_REQUEST))))
260260
.willReturn(null);
261261

262262
this.logoutRequestProcessingFilter.doFilterInternal(request, response, new MockFilterChain());
263263

264264
assertThat(response.getStatus()).isEqualTo(401);
265265
verify(this.logoutResponseResolver).resolve(any(), any(),
266-
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_BINDING)));
266+
argThat((ex) -> ex.getSaml2Error().getErrorCode().equals(Saml2ErrorCodes.INVALID_REQUEST)));
267267
verifyNoInteractions(this.logoutHandler);
268268
}
269269

0 commit comments

Comments
 (0)