spring-security
spring-security copied to clipboard
spring-projects
Set LogoutRepository on Saml2 Logout Success Handler in LogoutConfigurer
The Saml2LogoutConfigurer.class will use the configured Saml2LogoutRequestRepository.class and set it on the Saml2RelyingPartyInitiatedLogoutSuccessHandler.class.
I added this change to make progress on gh-11363.
@mmoussa-mapfre Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Thanks, @mmoussa-mapfre. Will you please add a unit test that confirms the success handler will now use the custom logout request repository?
@jzheaux I'd be happy to but I am not quite sure how to access that block of private code for testing in Saml2LogoutConfigurer. I see Saml2LogoutBeanDefinitionParserTests and Saml2LogoutConfigurerTests as possible places to test this change. Any tips are appreciated.
Yes, please add a test to Saml2LogoutConfigurerTests.
saml2LogoutWhenDefaultsThenLogsOutAndSendsLogoutRequest and saml2LogoutWhenCustomLogoutRequestResolverThenUses look like good starting points. Does that help?
@jzheaux I spent some time on it after you gave the tip but was still a bit confused. I have not had time to work on this since and I am still working on other efforts.
Okay, thanks. I think something like the following would do the trick:
@Test
public void saml2LogoutWhenCustomLogoutRequestRepositoryThenUses() throws Exception {
this.spring.register(Saml2LogoutComponentsConfig.class).autowire();
RelyingPartyRegistration registration = this.repository.findByRegistrationId("registration-id");
Saml2LogoutRequest logoutRequest = Saml2LogoutRequest.withRelyingPartyRegistration(registration)
.samlRequest(this.rpLogoutRequest).id(this.rpLogoutRequestId).relayState(this.rpLogoutRequestRelayState)
.parameters((params) -> params.put("Signature", this.rpLogoutRequestSignature)).build();
given(getBean(Saml2LogoutRequestResolver.class).resolve(any(), any())).willReturn(logoutRequest);
this.mvc.perform(post("/logout").with(authentication(this.user)).with(csrf()));
verify(getBean(Saml2LogoutRequestRepository.class)).saveLogoutRequest(eq(logoutRequest), any(), any());
}
@jzheaux I appreciate the help with this. I cannot seem to get the tests running locally, I tried to follow the setup guidelines without success. Could you add this change separately from this PR and we can just close this?
That's fine, @mmoussa-mapfre. I'm also happy to help troubleshoot the setup guidelines. We want contributors to be able to follow them. Do you have time to troubleshoot that with me or would you just prefer to wrap up?
@jzheaux I would like to troubleshoot and know what I am doing wrong. What did you have in mind?
The tests should run automatically when you do
./gradlew format check
I'm curious what error or behavior you are seeing, specifically if there are any hints to why the tests aren't running.
@jzheaux I was trying to run the tests inside STS without success.
I was able to get the tests running with gradlew format check like you suggested and used this for my specific addition gradlew :spring-security-config:test --tests Saml2LogoutConfigurerTests.saml2LogoutWhenCustomLogoutRequestRepositoryThenUses.
I have added the commit to my branch.
@jzheaux I noticed this is slated for version 6.0.0, is there any way this can be moved to the version 5 bug patches? I am currently locked on JDK 8 and spring security v5.x.x. I read version 6 requires JDK 17+ but I would like to use this bug fix myself.
I see, @mmoussa-mapfre. Will you please log an issue about what's failing when you try and run the tests in STS and we can take a look?
Is there any way this can be moved to the version 5 bug patches?
Yes, I believe this change is passive with 5.8. Could you do one more thing for me then and rebase your PR off of 5.8.x? Then it would be available in with the 5.8.0 release, which is still JDK 8.
@jzheaux I have rebased my branch off 5.8.x and switched the base on this PR. Thanks for the help and I will try to get more details about the STS testing issue.
Thanks again for the PR, @mmoussa-mapfre! This is now merged into 5.8.x as well as main.
