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

Favor PathPatternParser Over HandlerMappingIntrospector #16408

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented Jan 13, 2025

Closes #13562

Instead of needing a HandlerMappingIntrospector instance, applications can now do the following to simplify specifying the servlet path in the Java DSL:

@Bean 
SecurityFilterChain webSecurity(HttpSecurity http) throws Exception {
    RequestMatcherBuilder graphql = ServletRequestMatcherBuilders.servletPath("/graphql");
    RequestMatcherBuilder mvc = ServletRequestMatcherBuilders.servletPath("/mvc");
    http
        .authorizeHttpRequests((authorize) -> authorize
            .requestMatchers(graphql.anyRequest()).hasRole("GRAPHQL")
            .requestMatchers(mvc.pattern("/these/**", "/endpoints/**")).hasRole("USER")
            .requestMatchers(mvc.pattern("/admin/**")).hasRole("ADMIN")
        // ....

    return http.build();
}

To apply one across all DSL instances, do:

@Bean 
RequestMatcherBuilder mvcOnly() {
    return ServletRequestMatcherBuilders.servletPath("/mvc");
}

@Bean 
SecurityFilterChain webSecurity(HttpSecurity http) throws Exception {
    http
        .authorizeHttpRequests((authorize) -> authorize
            .requestMatchers(antPattern("/graphql/**")).hasRole("GRAPHQL")
            .requestMatchers("/these/**", "/endpoints/**").hasRole("USER")
            .requestMatchers("/admin/**").hasRole("ADMIN")
        // ....

    return http.build();
}

This second one is quite handy for when Spring MVC has a non-root servlet path. For example, there may be an option for Spring Boot to publish this bean since it knows when a servlet path has been specified in Boot properties

This PR also produces PathPatternRequestMatcher, which allows for specifying a PathPatternParser.

Questions:

  1. Should ServletRequestMatcherBuilders be left out for now? I quite like it since it allows the API to be intentionally servlet-specific (have methods like defaultServlet, servletPath, and servletExtension (should that be needed). I also feel it's simpler to know what I'm doing with ServletRequestMatcherBuilder.servletPath than PathPatterRequestMatcher.builder()#servletPath. Technically, though, ServletRequestMatcherBuilders is only for convenience.
  • Is RequestMatcherBuilder helpful? I find it to be quite helpful and clearly in line with how other components are designed in Spring Security. It is essentially present so that beans have an interface for publishing and to simplify adding convenience behaviors like anyRequest(). Once again, though, it can be added later. (Note that it would likely need to be added were it to be used by Spring Boot to automatically configure the DSL with the servlet path.)

Please note that there are a couple of cleanup commits in this PR as well. The commit of particular interest is the first one.

@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement labels Jan 13, 2025
@jzheaux jzheaux self-assigned this Jan 13, 2025
@jzheaux jzheaux changed the title Simply MVC Request Matcher Construction Simplify MVC Request Matcher Construction Jan 13, 2025
@jzheaux jzheaux changed the title Simplify MVC Request Matcher Construction Simplify Spring MVC Request Matcher Construction Jan 13, 2025
@jzheaux jzheaux changed the title Simplify Spring MVC Request Matcher Construction Favor PathPatternParser Over HandlerMappingIntrospector Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify MvcRequestMatcher construction
1 participant