diff --git a/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java b/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java index 60c8cca5fa..50d914a75a 100644 --- a/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java +++ b/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java @@ -24,6 +24,7 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import io.micrometer.common.KeyValue; import io.micrometer.common.KeyValues; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationConvention; @@ -38,6 +39,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.core.log.LogMessage; +import org.springframework.util.StringUtils; /** * A {@link org.springframework.security.web.FilterChainProxy.FilterChainDecorator} that @@ -568,13 +570,11 @@ public final class ObservationFilterChainDecorator implements FilterChainProxy.F @Override public KeyValues getLowCardinalityKeyValues(FilterChainObservationContext context) { - KeyValues kv = KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) + return KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) .and(CHAIN_POSITION_NAME, String.valueOf(context.getChainPosition())) - .and(FILTER_SECTION_NAME, context.getFilterSection()); - if (context.getFilterName() != null) { - kv = kv.and(FILTER_NAME, context.getFilterName()); - } - return kv; + .and(FILTER_SECTION_NAME, context.getFilterSection()) + .and(FILTER_NAME, (StringUtils.hasText(context.getFilterName())) ? context.getFilterName() + : KeyValue.NONE_VALUE); } @Override diff --git a/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java b/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java index 5d6f5f528b..7d94772240 100644 --- a/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java +++ b/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java @@ -34,6 +34,7 @@ import reactor.core.publisher.Mono; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.StringUtils; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebFilter; import org.springframework.web.server.WebFilterChain; @@ -686,13 +687,11 @@ public final class ObservationWebFilterChainDecorator implements WebFilterChainP @Override public KeyValues getLowCardinalityKeyValues(WebFilterChainObservationContext context) { - KeyValues kv = KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) + return KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) .and(CHAIN_POSITION_NAME, String.valueOf(context.getChainPosition())) - .and(FILTER_SECTION_NAME, context.getFilterSection()); - if (context.getFilterName() != null) { - kv = kv.and(FILTER_NAME, context.getFilterName()); - } - return kv; + .and(FILTER_SECTION_NAME, context.getFilterSection()) + .and(FILTER_NAME, (StringUtils.hasText(context.getFilterName())) ? context.getFilterName() + : KeyValue.NONE_VALUE); } @Override diff --git a/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java b/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java index a8a1655c43..9271f4ddd6 100644 --- a/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java +++ b/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java @@ -18,6 +18,7 @@ package org.springframework.security.web; import java.io.IOException; import java.util.List; +import java.util.stream.Stream; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationHandler; @@ -28,6 +29,9 @@ import jakarta.servlet.ServletException; import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletResponse; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.ArgumentCaptor; import org.springframework.mock.web.MockHttpServletRequest; @@ -128,6 +132,40 @@ public class ObservationFilterChainDecoratorTests { verify(handler).onScopeClosed(any()); } + @ParameterizedTest + @MethodSource("decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag") + void decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag(Filter filter, + String expectedFilterNameTag) throws Exception { + ObservationHandler handler = mock(ObservationHandler.class); + given(handler.supportsContext(any())).willReturn(true); + ObservationRegistry registry = ObservationRegistry.create(); + registry.observationConfig().observationHandler(handler); + ObservationFilterChainDecorator decorator = new ObservationFilterChainDecorator(registry); + FilterChain chain = mock(FilterChain.class); + FilterChain decorated = decorator.decorate(chain, List.of(filter)); + decorated.doFilter(new MockHttpServletRequest("GET", "/"), new MockHttpServletResponse()); + ArgumentCaptor context = ArgumentCaptor.forClass(Observation.Context.class); + verify(handler, times(3)).onScopeClosed(context.capture()); + assertThat(context.getValue().getLowCardinalityKeyValue("spring.security.reached.filter.name").getValue()) + .isEqualTo(expectedFilterNameTag); + } + + static Stream decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag() { + Filter filterWithName = new BasicAuthenticationFilter(); + + // Anonymous class leads to an empty filter-name + Filter filterWithoutName = new Filter() { + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + chain.doFilter(request, response); + } + }; + + return Stream.of(Arguments.of(filterWithName, "BasicAuthenticationFilter"), + Arguments.of(filterWithoutName, "none")); + } + private static class BasicAuthenticationFilter implements Filter { @Override diff --git a/web/src/test/java/org/springframework/security/web/server/ObservationWebFilterChainDecoratorTests.java b/web/src/test/java/org/springframework/security/web/server/ObservationWebFilterChainDecoratorTests.java index d4a6e702ad..fc5379118b 100644 --- a/web/src/test/java/org/springframework/security/web/server/ObservationWebFilterChainDecoratorTests.java +++ b/web/src/test/java/org/springframework/security/web/server/ObservationWebFilterChainDecoratorTests.java @@ -18,16 +18,22 @@ package org.springframework.security.web.server; import java.util.ArrayList; import java.util.List; +import java.util.stream.Stream; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationHandler; import io.micrometer.observation.ObservationRegistry; import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.ArgumentCaptor; import reactor.core.publisher.Mono; import org.springframework.mock.http.server.reactive.MockServerHttpRequest; import org.springframework.mock.web.server.MockServerWebExchange; +import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebFilter; import org.springframework.web.server.WebFilterChain; @@ -35,6 +41,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -113,6 +120,51 @@ public class ObservationWebFilterChainDecoratorTests { handler.assertSpanStop(9, "http"); } + @ParameterizedTest + @MethodSource("decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTagArguments") + void decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag(WebFilter filter, + String expectedFilterNameTag) { + ObservationHandler handler = mock(ObservationHandler.class); + given(handler.supportsContext(any())).willReturn(true); + ObservationRegistry registry = ObservationRegistry.create(); + registry.observationConfig().observationHandler(handler); + ObservationWebFilterChainDecorator decorator = new ObservationWebFilterChainDecorator(registry); + WebFilterChain chain = mock(WebFilterChain.class); + given(chain.filter(any())).willReturn(Mono.empty()); + WebFilterChain decorated = decorator.decorate(chain, List.of(filter)); + decorated.filter(MockServerWebExchange.from(MockServerHttpRequest.get("/").build())).block(); + + ArgumentCaptor context = ArgumentCaptor.forClass(Observation.Context.class); + verify(handler, times(3)).onStop(context.capture()); + + assertThat(context.getValue().getLowCardinalityKeyValue("spring.security.reached.filter.name").getValue()) + .isEqualTo(expectedFilterNameTag); + } + + static Stream decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTagArguments() { + WebFilter filterWithName = new BasicAuthenticationFilter(); + + // Anonymous class leads to an empty filter-name + WebFilter filterWithoutName = new WebFilter() { + @Override + public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { + return chain.filter(exchange); + } + }; + + return Stream.of(Arguments.of(filterWithName, "BasicAuthenticationFilter"), + Arguments.of(filterWithoutName, "none")); + } + + static class BasicAuthenticationFilter implements WebFilter { + + @Override + public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { + return chain.filter(exchange); + } + + } + static class AccumulatingObservationHandler implements ObservationHandler { List contexts = new ArrayList<>();