Skip to content

Add CsrfTokenRepository.loadDeferredToken #11924

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add CsrfTokenRepository.loadDeferredToken #11924

wants to merge 1 commit into from

Conversation

sjohnr
Copy link
Member

@sjohnr sjohnr commented Sep 29, 2022 •
edited
Loading

  • Move RepositoryDeferredCsrfToken to top-level and make package-private
  • Add CsrfTokenRepository.loadToken(HttpServletRequest, HttpServletResponse)
  • Update CsrfFilter
  • Rename CsrfTokenRepositoryRequestHandler to CsrfTokenRequestAttributeHandler

Issue gh-11892
Closes gh-11918

@sjohnr sjohnr added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement labels Sep 29, 2022
@sjohnr sjohnr added this to the 5.8.0-RC1 milestone Sep 29, 2022
@sjohnr sjohnr self-assigned this Sep 29, 2022
rwinch
rwinch requested changes Sep 29, 2022
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

I've provided feedback inline.

Additionally, I'm curious how this fits into the big picture of the breach protection

@sjohnr
Copy link
Member Author

sjohnr commented Sep 29, 2022

Thanks for the feedback, @rwinch!

Additionally, I'm curious how this fits into the big picture of the breach protection

The goal was to make a simpler API for customizing the request handling. The alternative we discussed would be to add a post-processor inside the CsrfTokenRepositoryRequestHandler. After experimenting with this a bit, I felt that I could simplify the API and wanted to give it a try and get some feedback.

With the change in this PR, support for breach fits back into the paradigm of a separate implementation of CsrfTokenRequestHandler. It is given the Supplier<CsrfToken>, which it can modify, and then pass to the delegate (or super) implementation from this PR. If we go this route, I won't need to make very many changes to the branch from the draft PR.

@sjohnr sjohnr force-pushed the gh-11918-load-token-supplier branch from 9a99987 to fe61b7c Compare October 3, 2022 17:18
@sjohnr sjohnr requested a review from rwinch October 3, 2022 17:19
@sjohnr
Copy link
Member Author

sjohnr commented Oct 3, 2022

Thanks @rwinch. I've updated the PR based on your feedback.

@sjohnr sjohnr changed the title Add deferred CsrfTokenRepository.loadToken Add deferred CsrfTokenRepository.loadDeferredToken Oct 3, 2022
Add deferred CsrfTokenRepository.loadDeferredToken
969c848 
* Move DeferredCsrfToken to top-level and implement Supplier<CsrfToken>
* Move RepositoryDeferredCsrfToken to top-level and make package-private
* Add CsrfTokenRepository.loadToken(HttpServletRequest, HttpServletResponse)
* Update CsrfFilter
* Rename CsrfTokenRepositoryRequestHandler to CsrfTokenRequestAttributeHandler

Issue spring-projectsgh-11892
Closes spring-projectsgh-11918
@sjohnr sjohnr force-pushed the gh-11918-load-token-supplier branch from fe61b7c to 969c848 Compare October 3, 2022 21:02
@sjohnr sjohnr changed the title Add deferred CsrfTokenRepository.loadDeferredToken Add CsrfTokenRepository.loadDeferredToken Oct 4, 2022
@sjohnr sjohnr added the status: duplicate A duplicate of another issue label Oct 4, 2022
@sjohnr
Copy link
Member Author

sjohnr commented Oct 4, 2022

Merged via 475b3bb

@sjohnr sjohnr closed this Oct 4, 2022
@sjohnr sjohnr deleted the gh-11918-load-token-supplier branch October 4, 2022 17:56
rwinch
rwinch approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Thanks @sjohnr This looks great. I think it has moved things along in a better direction than the work that I had done.

@sjohnr
Copy link
Member Author

sjohnr commented Oct 5, 2022

Appreciate the kind words @rwinch! Thanks for all the help, support and guidance too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch approved these changes

Assignees

@sjohnr sjohnr

Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.8.0-RC1
Development

Successfully merging this pull request may close these issues.
2 participants
@sjohnr @rwinch