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

Add ability to match Endpoint requests by http method #29596

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

Conversation

onobc
Copy link
Contributor

@onobc onobc commented Jan 29, 2022

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

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 29, 2022
@@ -191,38 +194,53 @@ protected RequestMatcherProvider getRequestMatcherProvider(WebApplicationContext

private final boolean includeLinks;

@Nullable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

import org.springframework.security.web.server.SecurityWebFilterChain;
import org.springframework.test.web.reactive.server.WebTestClient;

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

* @param httpMethod the http method
* @return a request matcher
*/
RequestMatcher getRequestMatcher(String pattern, HttpMethod httpMethod);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

onobc commented Jan 29, 2022

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

@snicoll
Copy link
Member

snicoll commented Jan 30, 2022

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
Copy link
Contributor Author

onobc commented Jan 30, 2022

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 philwebb self-assigned this Jan 31, 2022
@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Jan 31, 2022
@philwebb
Copy link
Member

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.

@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Jan 31, 2022
@snicoll
Copy link
Member

snicoll commented Jan 31, 2022

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
Copy link
Member

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
Copy link
Contributor Author

onobc commented Jan 31, 2022

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
Copy link
Contributor Author

onobc commented Jan 31, 2022

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.

@philwebb philwebb added type: enhancement A general enhancement for: merge-with-amendments Needs some changes when we merge and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged for: merge-with-amendments Needs some changes when we merge labels Aug 24, 2022
@philwebb philwebb added this to the 3.0.x milestone Aug 24, 2022
@philwebb philwebb modified the milestones: 3.0.x, 3.1.x Oct 17, 2022
@philwebb philwebb modified the milestones: 3.1.x, 3.2.x May 16, 2023
@philwebb philwebb modified the milestones: 3.2.x, 3.x Nov 3, 2023
@csterwa
Copy link

csterwa commented Mar 17, 2024

@philwebb was this merged in a different PR?

@philwebb
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants