Skip to content
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

The logoutFilter request matcher is hardcode to POST method #10311

Closed
3rojka opened this issue Sep 22, 2021 · 9 comments · Fixed by #10339
Closed

The logoutFilter request matcher is hardcode to POST method #10311

3rojka opened this issue Sep 22, 2021 · 9 comments · Fixed by #10339
Assignees
Labels
in: saml2 An issue in SAML2 modules status: feedback-provided Feedback has been provided type: enhancement A general enhancement

Comments

@3rojka
Copy link

3rojka commented Sep 22, 2021

I have troubles with this hardcoded POST method, as in my application we do not post logout url and only use get, possibility to configure the matcher would be nice. At least I did not found a way to configure that. I had to copy the whole configurer and reimplement just this piece of code.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 22, 2021
@sjohnr sjohnr added in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 22, 2021
@gourav
Copy link
Contributor

gourav commented Sep 23, 2021

Hello @sjohnr @jzheaux .
This seems like a beginner friendly issue where configurer needs to take an additional parameter for required request method.

If this analysis is correct, may I work on this ?

@3rojka
Copy link
Author

3rojka commented Sep 23, 2021

There is deeper question about all the configurers, I understand that final classes ar hard to break, but why cannot be configurers overwritten is a mystery to me. I would like to propose considering not to make these classes final and open the for customization. But maybe I am just missing some aspect of possibility to customize it.
I have a decade old application that evolved from application servers to spring boot application, and for its nature I need to be able to do more customization to some of the filters created, while I would like to use the defaults from spring. Then I end up copy pasting whole configurer into my code and overwrite what I need, but I admit that this is really ugly.

@jzheaux
Copy link
Contributor

jzheaux commented Sep 23, 2021

I think we definitely want to encourage POST /logout since GET /logout is vulnerable to CSRF Logout. I'd prefer that the methods exposed in saml2Logout be for the most secure cases.

That said, I think that we can make this a little easier by allowing the filter to be post-processed as most other filters can already be.

What's needed to do this is change Saml2LogoutConfigurer to wrap the filter in the postProcess method made available by the configurer API like so:

// ...
LogoutFilter logoutFilter = new LogoutFilter(logoutRequestSuccessHandler, logoutHandlers);
logoutFilter.setLogoutRequestMatcher(createLogoutMatcher());
return postProcess(logoutFilter);

Then, an application can do:

http
	.saml2Logout((saml2) -> saml2
		.addObjectPostProcessor(new ObjectPostProcessor<LogoutFilter>() {
				@Override public<O extends LogoutFilter> O postProcess(O object) {
					object.setLogoutRequestMatcher(myRequestMatcher);
					return object;
				}
		})
	);

to override the default to whatever request matching is needed.

Would you be able to submit a PR to allow the logout filter to be post-processed? If you are able, it would also be nice to do it for the two other filters that are added by the DSL.

@jzheaux jzheaux added type: enhancement A general enhancement status: waiting-for-feedback We need additional information before we can continue labels Sep 23, 2021
@3rojka
Copy link
Author

3rojka commented Sep 23, 2021

yes postprocessing would solve my problem

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 23, 2021
@gourav
Copy link
Contributor

gourav commented Sep 24, 2021

Hi @3rojka.
I am sorry but are willing to work on this or may I take this up ?

@3rojka
Copy link
Author

3rojka commented Sep 26, 2021

Take it.

@jzheaux jzheaux assigned jzheaux and gourav and unassigned jzheaux Sep 27, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Oct 1, 2021

Hi, @Erised. Is this something that you are still able to contribute?

@gourav
Copy link
Contributor

gourav commented Oct 2, 2021

Hello @jzheaux. Please review changes in #10339 for this issue.
Kindly let me know the further changes required, I will push those straightaway.

@EkaLinMan
Copy link

EkaLinMan commented May 24, 2023

Hi @jzheaux, I use Spring security 5.8.3 and have a similar issue when using <saml2-logout> in XML configuration,
is there any way to solve it with XML way?
https://github.com/spring-projects/spring-security/blob/5.8.x/config/src/main/java/org/springframework/security/config/http/Saml2LogoutBeanDefinitionParser.java#L166
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants