Add ability to match Endpoint requests by http method

[onobc]

What

This proposal adds the ability to match Actuator endpoint requests by http method.

Why

This provides a finer grained matching of requests. One example use is allowing unrestricted access to an endpoint for GET requests but restricting access to the endpoint for POST requests.

How

• Support has been added to both servlet and reactive flavors of EndpointRequest

[onobc]

Re: @Nullable... You will notice I did not use it in the reactive counterpart - on purpose. I wanted to see what the recommendation from the team is in this area. I do not see it widely used in the SB codebase other than mostly on a few params to @Endpoint methods (selectors, params).

Whatever direction we land I will make both imperative and reactive impls consistent.

[philwebb]

We currently don't use them in our own codebase so we may as well strip them out for now. We've got #10712 open to add them consistently.

[onobc]

Will you handle that in your polish commit @philwebb ? I don't mind doing it - just let me know.

[onobc]

This is net-new and is based on its servlet counterpart. I then added the coverage for the new matching on http method.

[mbhave]

I think these tests could inherit from AbstractEndpointRequestIntegrationTests too. I think that's why there was an abstract class and looks like an oversight on our part that there wasn't a reactive implementation before.

[onobc]

I think these tests could inherit from AbstractEndpointRequestIntegrationTests too.

I believe AERIT has servlet specific things in it though.

I think that's why there was an abstract class

I believe it was there to test the previous Jersey-flavored RequestMatcherProvider impl that has since been removed in 2.6.3.

[onobc]

I am not super-excited about this change. However, this seemed the least intrusive from a breakage standpoint. This leaves the other API in tact but it would break any previous lambda usage as the functional interface method now takes (pattern, method) vs. the previous (pattern).

[philwebb]

I've been playing with this locally and I'll try to push something soon. I think this API is really in wrong place so we could deprecate it here and move it to org.springframework.boot.actuate.autoconfigure.security.servlet where it's closer to the actual thing that uses it.

[onobc]

We would also like to backport this to 2.6.x branch but am unsure if the changes to RequestMatcherProvider would prevent that.

[snicoll]

Thanks for the PR Chris.

We would also like to backport this to 2.6.x branch

We don't backport enhancement and this feels like one to me.

[onobc]

Thanks for the PR Chris.

We would also like to backport this to 2.6.x branch

We don't backport enhancement and this feels like one to me.

TIL. I respect that sane policy.

And yep, this is an enhancement which is can easily be functionally worked around via composition w/ a guard on http method such as:

/**
 * Returns a matcher that includes the specified {@link Endpoint actuator endpoints} and http method.
 * @param httpMethod the http method to include
 * @param endpoints the endpoints to include
 * @return the configured {@link RequestMatcher}
 */
RequestMatcher to(HttpMethod httpMethod, String... endpoints) {
    final EndpointRequest.EndpointRequestMatcher matcher = EndpointRequest.to(endpoints);
    return (request) -> {
        if (httpMethod != null && !httpMethod.name().equals(request.getMethod())) {
            return false;
        }
        return matcher.matches(request);
    };
}

[philwebb]

I've pushed a polish commit to https://github.com/philwebb/spring-boot/tree/gh-29596.

I'd like someone else from the team to review. I'm wondering if we should include HttpMethod in the API or not. It's in spring-web and I wonder if it will always be available. Interestingly, the AntPathRequestMatcher uses Strings in the API but internally does rely on HttpMethod. I suspect that means we're fine to have it in out API.

[snicoll]

Given how the endpoint abstraction is designed at the moment, wouldn't it make more sense to provide the kind of operation that is requested on the endpoint? EndpointRequest we use an endpoint concept there so it feels natural to me to extend it to customize the "kind of operation" (read/write/delete).

[wilkinsona]

I agree that's natural for standard endpoints but I think it may be confusing for @ControllerEndpoint where you deal with HTTP methods directly.

[onobc]

I'm wondering if we should include HttpMethod in the API or not. It's in spring-web and I wonder if it will always be available. Interestingly, the AntPathRequestMatcher uses Strings in the API but internally does rely on HttpMethod. I suspect that means we're fine to have it in out API.

Yep, I saw this as well @philwebb . I ended up keeping HttpMethod until we go down into the AntPathRequestMatcher. It is also used in org.springframework.security.config.annotation.web.builders.HttpSecurity.RequestMatcherConfigurer#mvcMatchers(org.springframework.http.HttpMethod, java.lang.String...) which is where these matchers will be primarily used.

[onobc]

Given how the endpoint abstraction is designed at the moment, wouldn't it make more sense to provide the kind of operation that is requested on the endpoint? EndpointRequest we use an endpoint concept there so it feels natural to me to extend it to customize the "kind of operation" (read/write/delete).

I thought about this as well @snicoll . I ended up putting on the EndpointRequestMatcher directly to keep the functionality isolated to this particular case where the matchers are being used in the security mappings and where http methods are used.

[csterwa]

@philwebb was this merged in a different PR?

[philwebb]

@csterwa No, I don't believe this has been merged in any form as yet.