micronaut-core icon indicating copy to clipboard operation
micronaut-core copied to clipboard

#7552 Allow adding a custom context for path management

Open viniciusxyz opened this issue 1 year ago • 14 comments

#7552 The idea of ​​this MR is to create the endpoints.all.context.path property that can be used to separate the management context-path from the server.context-path

With this change, the behavior of the paths is as follows:

config:

micronaut.server.context-path=/foo endpoints.all.context-path=/fff endpoints.all.path=/eee

Now:

Default app -> http://localhost:8080/foo Management -> http://localhost:8080/fff/eee

Before

Default app -> http://localhost:8080/foo Management -> http://localhost:8080/foo/eee

config:

micronaut.server.context-path=/foo endpoints.all.path=/eee

Now:

Default app -> http://localhost:8080/foo Management -> http://localhost:8080/foo/eee

Before

Default app -> http://localhost:8080/foo Management -> http://localhost:8080/foo/eee

With the addition of this property there should be no change in behavior in relation to the current properties.

viniciusxyz avatar Sep 02 '23 22:09 viniciusxyz

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 02 '23 22:09 CLAassistant

not sure this makes sense to be honest. There should be a separate context path setting for endpoints in which case it wouldn't be breaking

graemerocher avatar Oct 23 '23 09:10 graemerocher

To make this non-breaking IMO we should support endpoints.all.context-path

graemerocher avatar Nov 13 '23 15:11 graemerocher

To make this non-breaking IMO we should support endpoints.all.context-path

I will try to update MR over the weekend by adding this property.

viniciusxyz avatar Nov 14 '23 11:11 viniciusxyz

To make this non-breaking IMO we should support endpoints.all.context-path

I will try to update MR over the weekend by adding this property.

I apologize, but due to Black Friday I had to work on the weekend and was unable to make the necessary changes, I will try to make it as soon as possible

viniciusxyz avatar Nov 18 '23 19:11 viniciusxyz

@sdelamo I made the changes based on the comments made and instead of changing the behavior of endpoints.all.path the idea is to add the endpoints.all.context-path property, I am finishing the implementation, but I came across a difficulty that I still haven't been able to overcome, for some reason when adding the new property the EndpointsFilter filter is never executed, I'm investigating this and as soon as possible I'll add the documentation and remove the draft PR.

If you have any idea why a filter with the /** pattern is not filtering a request after adding a path, the tip would be very welcome.

viniciusxyz avatar Dec 10 '23 16:12 viniciusxyz

can you merge 4.3.x against your branch. I changed the base to be 4.3.x

sdelamo avatar Dec 12 '23 12:12 sdelamo

Can you add a controller to the test to verify the controller is still accessible in the expected path?

Moreover, can you modify your test to test an endpoint which does not return 401. The Unauthorized makes the test confusing.

As the test refers to the context path of the management endpoints and they are configured as sensitive by default, I have to guarantee that even with the change in context there is no change in the security model that the framework adds today, which is why I am following the applied testing model in the EndpointsBasePathSpec test which has a very similar objective, I still haven't been able to define why the security tests aren't passing, it's as if there was some trigger to activate the EndpointsFilter that I can't see

viniciusxyz avatar Dec 16 '23 15:12 viniciusxyz

@sdelamo The problem currently is that when defining micronaut.server.context-path and endpoints.all.context-path as in this test, EndpointsFilter -> doFilter is never called, this occurs because of this code snippet ServerFilterRouteBuilder.java.

As a mental model, let’s think about the following:

Let's assume we have an application configured with Prometheus and the following properties:

  • micronaut.server.context-path: /any
  • endpoints.all.context-path: /test
  • endpoints.prometheus.sensitive: true
  1. A new enhancement of AbstractEndpointRouteBuilder.java creates the path as /test/prometheus ignoring the context path as it is what is desired.

  2. A class ServerFilterRouteBuilder changes the default pattern /** of EndpointsFilter to /any/**

This causes calls to the prometheus point, even if they are defined as sensitive true, to still be displayed without security. This is due to the filter's behavior. Is there any filter that I can use that does not have this behavior of append the contextPath?

viniciusxyz avatar Dec 17 '23 18:12 viniciusxyz

@sdelamo Since I couldn't get the filter to ignore the ContextPath in the filters, I added a new property to the ServerFilter to establish whether the ContextPath should be added or not. With this and adding the option to not put ContextPath in the security filter now the solution works and all tests pass.

If you have any comments about the solution or need me to modify or test something, please let me know.

viniciusxyz avatar Feb 04 '24 16:02 viniciusxyz

Any news about this improvement?

altro3 avatar Apr 11 '24 08:04 altro3

Any news about this improvement?

I'm still waiting for approval :)

viniciusxyz avatar Apr 11 '24 10:04 viniciusxyz

@graemerocher @sdelamo ping

altro3 avatar Apr 11 '24 11:04 altro3

it is scheduled for 4.5

graemerocher avatar Apr 16 '24 20:04 graemerocher

Merged with https://github.com/micronaut-projects/micronaut-core/pull/10870

Thanks for the contribution

graemerocher avatar Jun 01 '24 10:06 graemerocher