Skip to content

Allow configuration of added SessionAuthenticationStrategy for CsrfConfigurer #5300

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
mvitz opened this issue May 4, 2018 · 11 comments
Closed
Assignees
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Milestone

Comments

@mvitz
Copy link
Contributor

mvitz commented May 4, 2018

Summary

CsrfConfigurer automatically adds an instance of CsrfAuthenticationStrategy as SessionAuthenticationStrategy. It would be helpful to allow another strategy to be added.

Actual Behavior

An instance of CsrfAuthenticationStrategy is configured automatically.

Expected Behavior

The CsrfConfigurer contains a method to configure the used SessionAuthenticationStrategy.

Version

5.0.4.RELEASE

@mvitz
Copy link
Contributor Author

mvitz commented May 7, 2018

If someone could confirm that extending the CsrfConfigurer to accept an SessionAuthenticationStrategy to override the default behaviour would be accepted I would create a MR.

@samiconductor
Copy link
Contributor

This would solve my problem in #5299. Thanks @mvitz!

@samiconductor
Copy link
Contributor

Any thoughts on this @rwinch?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 7, 2019
@serhanozbey
Copy link

serhanozbey commented Jun 11, 2019

@mvitz & @samiconductor, having the same problem here with a Spring MVC stateless application with JWT tokens. After authentication, CsrfAuthenticationStrategy is generating CSRF tokens for each resource, but saves the initial html csrf token as thymeleaf hidden field. Therefore, any POST requests are failing.

@rwinch
Copy link
Member

rwinch commented Jun 12, 2019

CsrfConfigurer automatically adds an instance of CsrfAuthenticationStrategy as SessionAuthenticationStrategy. It would be helpful to allow another strategy to be added.

First, why do you want to change the authentication strategy? Second, can you clarify why you want to do that vs

http
    .sessionManagement()
        .sessionAuthenticationStrategy(someStrategy);

@samiconductor
Copy link
Contributor

samiconductor commented Jun 12, 2019

@rwinch the problem is when you configure CSRF the configurer automatically adds CsrfAuthenticationStrategy. Setting the sessionAuthenticationStrategy does not override the CSRF strategy, it just adds to the list of strategies. And the problem with the CSRF strategy is it generates a new token every request.

With a STATELESS app, we want to store the CSRF token on the client on login and pass it back each time similar to a JWT token. So we would add the CSRF token to the client as a cookie that is hidden from JS and pass the CSRF token to the JS as a HTTP header and store it in session storage for example. Each request would ensure the cookie and passed token match.

However, when the CSRF token is recreated on each request, the cookie and passed token can get out of sync when making concurrent requests.

Anyway, that was my idea of how CSRF would work in a STATELESS app. Maybe there's another approach? I've just disabled CSRF in the mean time. But the above cookie/header approach could work if we can override the default strategy like @mvitz setup in this PR.

@rwinch
Copy link
Member

rwinch commented Jun 13, 2019

@samiconductor Thanks for the clarification. This makes more sense to me now. I'd love for you to submit a PR that allows configuring the strategy as you suggested. Once you have confirmed you are still able to work on it, I will mark this issue as such. If you need any help, don't hesitate to reach out

@rwinch rwinch added in: config An issue in spring-security-config type: enhancement A general enhancement status: ideal-for-contribution An issue that we actively are looking for someone to help us with and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 13, 2019
@samiconductor
Copy link
Contributor

@rwinch Sure no problem. Oh right, this isn't a PR. @mvitz had started it here a while ago but we couldn't figure out how to pass the tests. Will look into it again soon.

@rwinch
Copy link
Member

rwinch commented Jun 17, 2019

If you have specific questions, feel free to ask here.

@samiconductor
Copy link
Contributor

Used your commit @mvitz, thanks! Just moved the tests from groovy to the new java ones.

@rwinch integration tests failed for me (mostly ldap) but units passed.

@rwinch rwinch self-assigned this Jul 16, 2019
@rwinch rwinch removed the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Jul 16, 2019
@rwinch rwinch added this to the 5.2.0.RC1 milestone Jul 16, 2019
@rwinch rwinch closed this as completed in 09e8ae4 Jul 16, 2019
@rwinch
Copy link
Member

rwinch commented Jul 16, 2019

@samiconductor Thanks for the PR!

@rwinch integration tests failed for me (mostly ldap) but units passed.

Strange as they worked fine for me. If you continue to see errors, can you please create a ticket with what happens and details on how to reproduce?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants