PathPatternRequestMatcher Include Optional Servlet Path in the pattern

[rwinch]

I wonder if it would be better to change PathPatternRequestMatcher to include the servlet in the PathPattern that should be used instead of having a separate RequestMatcher for the servlet.

Then instead of using pathWithinApplication() it would be the entire PathContainer.

NOTE: Further investigation might be necessary, but this seems like it doesn't work since the contextPath is considered unless pathWithinApplication is used. I'm wondering why framework doesn't allow only ignoring the contextPath.

[jzheaux]

I think it's important, though perhaps not vital, that we match the same way that Framework does. I feel like since they treat the servlet path separately, it makes some sense to separate in the same way.

While I think this could be considered an implementation detail, it's also true that HttpServletRequest has a separate property for servlet path, so it could make sense independent of how Framework matches.

Independent of that, I personally like being able to declare a builder with a servlet path prefix and reusing that for several matchers. I'm inclined to lean further in that direction, for example:

PathPatternRequestMatcher.Builder rest = withDefaults().servletPath("/spring/data/rest");
PathPatternRequestMatcher.Builder ui = withDefaults().servletPath("/mvc/ui");

// ...

.requestMatchers(rest.matchers("/list/**", "/several/**", "/resources/**")) // ...
.requestMatchers(ui.matchers("/ui/**", "/endpoints/**")) // ...

This has the nice benefit of lowering the cognitive load when mentally matching the controller URI with the authorization URI which has the potential of being more secure.

Perhaps the needed change is to call the method servletMapping and match against HttpServletMapping instead. In this way it could also accommodate extensions:

PathPatternRequestMatcher dotDo = withDefaults().servletPattern("*.do").matcher("/**");

Just thinking out loud.

[rwinch]

I think it's important, though perhaps not vital, that we match the same way that Framework does. I feel like since they treat the servlet path separately, it makes some sense to separate in the same way.

While it is important to align the matching logic as much as possible, we cannot match exactly as Framework (Spring MVC) does because Spring MVC does not use the servlet path at all. This makes sense because when Spring MVC starts processing the request it has already been narrowed down to the correct servlet by Servlet Container. It is important to highlight that to be the same as Spring MVC we would have to replicate the logic by the Servlet Container which is likely done in a vendor specific way based upon the specification.

Spring Security provides authorization rules for every request (All Servlets and Filter's) so it must have enough information to distinguish /servletA/foo and /servletB/foo. This means that the servlet path needs to be considered.

While MVC does not need to use the servlet path and Spring Security does, I do think it is important that we receive buy in from the Framework team for how we match requests. I've reached out to @rstoyanchev and we have started discussing if/how servlet path could be used in the matching. He suggested that this would be a way to do it for now:

String requestUri = "/app/servlet/path1/path2";
MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri);
request.setContextPath("/app");
request.setServletPath("/servlet")
RequestPath path = ServletRequestPathUtils.parseAndCache(request);
PathContainer contextPath = path.contextPath();
PathContainer pathInContext = path.subPath(contextPath.elements().size());
assertThat(pathInContext.value()).isEqualTo("/servlet/path2/path2");

In the future, he mentioned that we might be able to provide some API within Spring Framework to make this easier.

Independent of that, I personally like being able to declare a builder with a servlet path prefix and reusing that for several matchers. I'm inclined to lean further in that direction, for example:

I think there is value in not having to duplicate the servlet path, but I think we can do this without the limitations of this approach.

As it stands I see three limitations to the current approach.

First, the servlet path comparison only supports equals based comparison. It does not support pattern based matching which can be used elsewhere.

Second, the servlet path currently does not support path variables (e.g. /{servletPath}/foo/bar) which could be used in authorization decisions.

Finally, treating the servlet path separately artificially limits the builder with prefixes that are the servlet path.

I think that we can support servlet path prefixes, patterns and path variables on the servlet paths if matching was performed using the servlet path + requestUri. For example, servlet path prefixes would be supported using something like:

 builder.basePath("/servletPath")

If an application had an admin controller that had request mappings that start with /admin, then the following could be used:

builder.basePath("/admin")

PathPatternRequestMatcher dotDo = withDefaults().servletPattern("*.do").matcher("/**");

I'm not sure I understand why we want to require a servletPattern here. Consider a related RequestMatcher for a servlet mapped to *.do:

PathPatternRequestMatcher dotDo = withDefaults().servletPattern("*.do").matcher("/admin/users.do");

The *.do seems unnecessary in this instance making the configuration more verbose than necessary. It also feels like it would be error prone to user's forgetting the servletPattern.

I wonder if this is being considered as a remnant of when we needed to know what Servlet was being used to ensure that the proper RequestMatcher was used. For example, previously we needed to ensure that only Spring MVC's Dispatcher Servlet was used with MvcRequestMatcher.

I don't think we need to know what Servlet is used anymore since PathPatternRequestMatcher is recommended for all URLs (any Servlet) now.

This adds to another consideration. If we don't need to validate the Servlet is a aligned with the RequestMatcher, then perhaps we can remove the validation of the Servlet. We don't validate that paths within the Servlet are valid (nor do I think we should). I don't think that we need validation of the Servlet either. Removing this validation would simplify things quite a bit.