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

Fix CsrfConfigurer default AccessDeniedHandler consistency #10154

Closed
wants to merge 1 commit into from
Closed

Fix CsrfConfigurer default AccessDeniedHandler consistency #10154

wants to merge 1 commit into from

Conversation

christophejan
Copy link
Contributor

@christophejan christophejan commented Aug 3, 2021

Fix when AccessDeniedHandler is specified per RequestMatcher on
ExceptionHandlingConfigurer.

This introduces evolutions on :

  • CsrfConfigurer#getDefaultAccessDeniedHandler,
    to retrieve an AccessDeniedHandler similar to the one used by
    ExceptionHandlingConfigurer.
  • OAuth2ResourceServerConfigurer#accessDeniedHandler, to continue to
    handle CsrfException with the default AccessDeniedHandler implementation

Fixes: gh-6511

I have submitted the CLA

@christophejan
Copy link
Contributor Author

christophejan commented Aug 3, 2021

This pull request raise much more questions than I was expecting.

Before considering to merge this pull request, I think the following points should be reviewed :

  1. First, regarding ExceptionHandlingConfigurer#defaultAccessDeniedHandlerFor visibility. Current visibility is public (and so usable in a WebSecurityConfigurerAdapter) but not well handled by CsrfConfigurer (cf CsrfConfigurer accessDeniedHandler logic #6511). On the other hand, with reactive applications, ServerHttpSecurity#defaultAccessDeniedHandlers is only used internally without any public method to interact with. Does servlet and reactive should be consistent ?
  2. CsrfConfigurer default AccessDeniedHandler is currently retrieved from ExceptionHandlingConfigurer (when available). With reactive applications, there is no such interaction between CsrfSpec and ExceptionHandlingSpec. Does exception handling should be able to take responsibility for all AccessDeniedException or not ? Does servlet and reactive should be consistent ?
  3. ExceptionHandlingConfigurer#createDefaultDeniedHandler(HttpSecurityBuilder) implementation feel wrong to me. Currently, if only a single default AccessDeniedHandler is specified, it will be what is used for the default AccessDeniedHandler so the associated RequestMatcher is ignored. When more entries are specified, all RequestMatcher are take in account and AccessDeniedHandlerImpl is used as default. From my point of view, it seems inconsistent ; either the default should be taken from the specified entries (like for entry points) or AccessDeniedHandlerImpl should be the default in any case.

I’m sorry to raise many points for a such simple issue.

Best regards

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 3, 2021
@christophejan christophejan marked this pull request as draft August 3, 2021 10:42
Fix when AccessDeniedHandler is specified per RequestMatcher on
ExceptionHandlingConfigurer.

This introduces evolutions on :
- CsrfConfigurer#getDefaultAccessDeniedHandler,
to retrieve an AccessDeniedHandler similar to the one used by
ExceptionHandlingConfigurer.
- OAuth2ResourceServerConfigurer#accessDeniedHandler, to continue to
handle CsrfException with the default AccessDeniedHandler implementation

Fixes: gh-6511
@christophejan christophejan marked this pull request as ready for review August 4, 2021 10:47
@eleftherias eleftherias added in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 10, 2021
@rwinch
Copy link
Member

rwinch commented Nov 16, 2021

Thanks for the PR This is merged via 4318a51

@rwinch rwinch closed this Nov 16, 2021
@rwinch rwinch added this to the 5.7.0-M1 milestone Nov 16, 2021
@rwinch rwinch added in: config An issue in spring-security-config type: enhancement A general enhancement and removed in: web An issue in web modules (web, webmvc) labels Nov 16, 2021
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

Successfully merging this pull request may close these issues.

CsrfConfigurer accessDeniedHandler logic
4 participants