From 9083dbd788f08e218199fc6378c63f162b1eb516 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Tue, 11 Apr 2023 18:08:07 -0600 Subject: [PATCH] Polish Observation Event Names Issue gh-12811 --- .../web/ObservationFilterChainDecorator.java | 202 +++++------------- .../ObservationFilterChainDecoratorTests.java | 49 +++-- 2 files changed, 82 insertions(+), 169 deletions(-) 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 b5ef2bd3d1f..33c2ff06747 100644 --- a/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java +++ b/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java @@ -38,36 +38,6 @@ import org.apache.commons.logging.LogFactory; import org.springframework.core.log.LogMessage; -import org.springframework.security.web.access.ExceptionTranslationFilter; -import org.springframework.security.web.access.channel.ChannelProcessingFilter; -import org.springframework.security.web.access.intercept.AuthorizationFilter; -import org.springframework.security.web.access.intercept.FilterSecurityInterceptor; -import org.springframework.security.web.authentication.AnonymousAuthenticationFilter; -import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter; -import org.springframework.security.web.authentication.logout.LogoutFilter; -import org.springframework.security.web.authentication.preauth.AbstractPreAuthenticatedProcessingFilter; -import org.springframework.security.web.authentication.preauth.x509.X509AuthenticationFilter; -import org.springframework.security.web.authentication.rememberme.RememberMeAuthenticationFilter; -import org.springframework.security.web.authentication.switchuser.SwitchUserFilter; -import org.springframework.security.web.authentication.ui.DefaultLoginPageGeneratingFilter; -import org.springframework.security.web.authentication.ui.DefaultLogoutPageGeneratingFilter; -import org.springframework.security.web.authentication.www.BasicAuthenticationFilter; -import org.springframework.security.web.authentication.www.DigestAuthenticationFilter; -import org.springframework.security.web.context.SecurityContextHolderFilter; -import org.springframework.security.web.context.SecurityContextPersistenceFilter; -import org.springframework.security.web.context.request.async.WebAsyncManagerIntegrationFilter; -import org.springframework.security.web.csrf.CsrfFilter; -import org.springframework.security.web.header.HeaderWriterFilter; -import org.springframework.security.web.jaasapi.JaasApiIntegrationFilter; -import org.springframework.security.web.savedrequest.RequestCacheAwareFilter; -import org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter; -import org.springframework.security.web.session.ConcurrentSessionFilter; -import org.springframework.security.web.session.DisableEncodeUrlFilter; -import org.springframework.security.web.session.ForceEagerSessionCreationFilter; -import org.springframework.security.web.session.SessionManagementFilter; -import org.springframework.util.Assert; -import org.springframework.util.StringUtils; -import org.springframework.web.filter.CorsFilter; /** * A {@link org.springframework.security.web.FilterChainProxy.FilterChainDecorator} that @@ -80,12 +50,6 @@ public final class ObservationFilterChainDecorator implements FilterChainProxy.F private static final Log logger = LogFactory.getLog(FilterChainProxy.class); - private static final int OPENTELEMETRY_MAX_NAME_LENGTH = 63; - - private static final int MAX_OBSERVATION_NAME_LENGTH = OPENTELEMETRY_MAX_NAME_LENGTH - ".before".length(); - - private static final Map OBSERVATION_NAMES = new HashMap<>(); - private static final String ATTRIBUTE = ObservationFilterChainDecorator.class + ".observation"; static final String UNSECURED_OBSERVATION_NAME = "spring.security.http.unsecured.requests"; @@ -136,83 +100,10 @@ private List wrap(List filters) { return observableFilters; } - static { - registerName(DisableEncodeUrlFilter.class, "session.encode-url.disable"); - registerName(ForceEagerSessionCreationFilter.class, "session.create"); - registerName(ChannelProcessingFilter.class, "web.request.delivery.ensure"); - registerName(WebAsyncManagerIntegrationFilter.class, "web-async-manager.join.security-context"); - registerName(SecurityContextHolderFilter.class, "security-context.hold"); - registerName(SecurityContextPersistenceFilter.class, "security-context.persist"); - registerName(HeaderWriterFilter.class, "web.response.header.set"); - registerName(CorsFilter.class, "cors.process"); - registerName(CsrfFilter.class, "csrf.protect"); - registerName(LogoutFilter.class, "principal.logout"); - registerName("org.springframework.security.oauth2.client.web.OAuth2AuthorizationRequestRedirectFilter", - "web.request.oauth2.redirect"); - registerName( - "org.springframework.security.saml2.provider.service.web." + "Saml2WebSsoAuthenticationRequestFilter", - "web.request.saml2.redirect"); - registerName(X509AuthenticationFilter.class, "web.request.x509.auth"); - registerName(AbstractPreAuthenticatedProcessingFilter.class, "web.request.pre-auth.base.process"); - registerName("org.springframework.security.cas.web.CasAuthenticationFilter", "web.request.sas.auth"); - registerName("org.springframework.security.oauth2.client.web.OAuth2LoginAuthenticationFilter", - "web.response.oauth2.process"); - registerName("org.springframework.security.saml2.provider.service.web.authentication" - + ".Saml2WebSsoAuthenticationFilter", "web.request.saml2.auth"); - registerName(UsernamePasswordAuthenticationFilter.class, "web.request.username-password.auth"); - registerName(DefaultLoginPageGeneratingFilter.class, "web.login-page.default.generate"); - registerName(DefaultLogoutPageGeneratingFilter.class, "web.logout-page.default.generate"); - registerName(ConcurrentSessionFilter.class, "session.refresh"); - registerName(DigestAuthenticationFilter.class, "web.request.digest.auth"); - registerName("org.springframework.security.oauth2.server.resource.web.authentication." - + "BearerTokenAuthenticationFilter", "web.request.bearer.auth"); - registerName(BasicAuthenticationFilter.class, "web.request.basic.auth"); - registerName(RequestCacheAwareFilter.class, "web.request.cache.extract"); - registerName(SecurityContextHolderAwareRequestFilter.class, "web.request.security.wrap"); - registerName(JaasApiIntegrationFilter.class, "web.request.jass.auth"); - registerName(RememberMeAuthenticationFilter.class, "web.request.remember-me.auth"); - registerName(AnonymousAuthenticationFilter.class, "web.request.anonymous.auth"); - registerName("org.springframework.security.oauth2.client.web.OAuth2AuthorizationCodeGrantFilter", - "web.response.oauth2.code-grant.process"); - registerName(SessionManagementFilter.class, "session.manage"); - registerName(ExceptionTranslationFilter.class, "exception.translate"); - registerName(FilterSecurityInterceptor.class, "web.response.security.intercept"); - registerName(AuthorizationFilter.class, "web.access.auth.restrict"); - registerName(SwitchUserFilter.class, "session.switch"); - } - - public static void registerName(Class clazz, String name) { - String keyName = clazz.getName(); - checkAlreadyRegistered(keyName); - OBSERVATION_NAMES.put(keyName, limitLength(name)); - } - - public static void registerName(String className, String name) { - checkAlreadyRegistered(className); - OBSERVATION_NAMES.put(className, name); - } - static AroundFilterObservation observation(HttpServletRequest request) { return (AroundFilterObservation) request.getAttribute(ATTRIBUTE); } - private static String getObservationName(String className) { - if (OBSERVATION_NAMES.containsKey(className)) { - return OBSERVATION_NAMES.get(className); - } - throw new IllegalArgumentException("Class not registered for observation: " + className); - } - - private static String limitLength(String s) { - Assert.isTrue(s.length() <= MAX_OBSERVATION_NAME_LENGTH, - "The name must be less than MAX_OBSERVATION_NAME_LENGTH=" + MAX_OBSERVATION_NAME_LENGTH); - return s; - } - - private static void checkAlreadyRegistered(String keyName) { - Assert.isTrue(!OBSERVATION_NAMES.containsKey(keyName), "Observation name is registered already: " + keyName); - } - private static final class VirtualFilterChain implements FilterChain { private final FilterChain originalChain; @@ -248,6 +139,49 @@ public void doFilter(ServletRequest request, ServletResponse response) throws IO static final class ObservationFilter implements Filter { + private static final Map OBSERVATION_NAMES = new HashMap<>(); + + static { + OBSERVATION_NAMES.put("DisableEncodeUrlFilter", "session.url-encoding"); + OBSERVATION_NAMES.put("ForceEagerSessionCreationFilter", "session.eager-create"); + OBSERVATION_NAMES.put("ChannelProcessingFilter", "access.channel"); + OBSERVATION_NAMES.put("WebAsyncManagerIntegrationFilter", "context.async"); + OBSERVATION_NAMES.put("SecurityContextHolderFilter", "context.holder"); + OBSERVATION_NAMES.put("SecurityContextPersistenceFilter", "context.management"); + OBSERVATION_NAMES.put("HeaderWriterFilter", "header"); + OBSERVATION_NAMES.put("CorsFilter", "cors"); + OBSERVATION_NAMES.put("CsrfFilter", "csrf"); + OBSERVATION_NAMES.put("LogoutFilter", "logout"); + OBSERVATION_NAMES.put("OAuth2AuthorizationRequestRedirectFilter", "oauth2.authnrequest"); + OBSERVATION_NAMES.put("Saml2WebSsoAuthenticationRequestFilter", "saml2.authnrequest"); + OBSERVATION_NAMES.put("X509AuthenticationFilter", "authentication.x509"); + OBSERVATION_NAMES.put("J2eePreAuthenticatedProcessingFilter", "preauthentication.j2ee"); + OBSERVATION_NAMES.put("RequestHeaderAuthenticationFilter", "preauthentication.header"); + OBSERVATION_NAMES.put("RequestAttributeAuthenticationFilter", "preauthentication.attribute"); + OBSERVATION_NAMES.put("WebSpherePreAuthenticatedProcessingFilter", "preauthentication.websphere"); + OBSERVATION_NAMES.put("CasAuthenticationFilter", "cas.authentication"); + OBSERVATION_NAMES.put("OAuth2LoginAuthenticationFilter", "oauth2.authentication"); + OBSERVATION_NAMES.put("Saml2WebSsoAuthenticationFilter", "saml2.authentication"); + OBSERVATION_NAMES.put("UsernamePasswordAuthenticationFilter", "authentication.form"); + OBSERVATION_NAMES.put("DefaultLoginPageGeneratingFilter", "page.login"); + OBSERVATION_NAMES.put("DefaultLogoutPageGeneratingFilter", "page.logout"); + OBSERVATION_NAMES.put("ConcurrentSessionFilter", "session.concurrent"); + OBSERVATION_NAMES.put("DigestAuthenticationFilter", "authentication.digest"); + OBSERVATION_NAMES.put("BearerTokenAuthenticationFilter", "authentication.bearer"); + OBSERVATION_NAMES.put("BasicAuthenticationFilter", "authentication.basic"); + OBSERVATION_NAMES.put("RequestCacheAwareFilter", "requestcache"); + OBSERVATION_NAMES.put("SecurityContextHolderAwareRequestFilter", "context.servlet"); + OBSERVATION_NAMES.put("JaasApiIntegrationFilter", "jaas"); + OBSERVATION_NAMES.put("RememberMeAuthenticationFilter", "authentication.rememberme"); + OBSERVATION_NAMES.put("AnonymousAuthenticationFilter", "authentication.anonymous"); + OBSERVATION_NAMES.put("OAuth2AuthorizationCodeGrantFilter", "oauth2.client.code"); + OBSERVATION_NAMES.put("SessionManagementFilter", "session.management"); + OBSERVATION_NAMES.put("ExceptionTranslationFilter", "access.exceptions"); + OBSERVATION_NAMES.put("FilterSecurityInterceptor", "access.request"); + OBSERVATION_NAMES.put("AuthorizationFilter", "authorization"); + OBSERVATION_NAMES.put("SwitchUserFilter", "authentication.switch"); + } + private final ObservationRegistry registry; private final FilterChainObservationConvention convention = new FilterChainObservationConvention(); @@ -256,7 +190,7 @@ static final class ObservationFilter implements Filter { private final String name; - private final String observationName; + private final String eventName; private final int position; @@ -268,28 +202,16 @@ static final class ObservationFilter implements Filter { this.name = filter.getClass().getSimpleName(); this.position = position; this.size = size; - String tempObservationName; - try { - tempObservationName = ObservationFilterChainDecorator.getObservationName(filter.getClass().getName()); - } - catch (IllegalArgumentException ex) { - tempObservationName = compressName(this.name); - logger.warn( - "Class " + filter.getClass().getName() - + " is not registered for observation and will have name " + tempObservationName - + ". Please consider of registering this class with " - + ObservationFilterChainDecorator.class.getSimpleName() + ".registerName(class, name).", - ex); - } - this.observationName = tempObservationName; + this.eventName = eventName(this.name); } - String getName() { - return this.name; + private String eventName(String className) { + String eventName = OBSERVATION_NAMES.get(className); + return (eventName != null) ? eventName : className; } - String getObservationName() { - return this.observationName; + String getName() { + return this.name; } @Override @@ -312,8 +234,7 @@ private void wrapFilter(ServletRequest request, ServletResponse response, Filter parentBefore.setFilterName(this.name); parentBefore.setChainPosition(this.position); } - parent.before().event(Observation.Event.of(this.observationName + ".before", - "before " + this.name)); + parent.before().event(Observation.Event.of(this.eventName + ".before", "before " + this.name)); this.filter.doFilter(request, response, chain); parent.start(); if (parent.after().getContext() instanceof FilterChainObservationContext parentAfter) { @@ -321,8 +242,7 @@ private void wrapFilter(ServletRequest request, ServletResponse response, Filter parentAfter.setFilterName(this.name); parentAfter.setChainPosition(this.size - this.position + 1); } - parent.after().event(Observation.Event.of(this.observationName + ".after", - "after " + this.name)); + parent.after().event(Observation.Event.of(this.eventName + ".after", "after " + this.name)); } private AroundFilterObservation parent(HttpServletRequest request) { @@ -335,24 +255,6 @@ private AroundFilterObservation parent(HttpServletRequest request) { return parent; } - private String compressName(String className) { - if (className.length() >= MAX_OBSERVATION_NAME_LENGTH) { - return maximalCompressClassName(className, MAX_OBSERVATION_NAME_LENGTH); - } - return className; - } - - private String maximalCompressClassName(String className, int maxLength) { - String[] names = className.split("(?=\\p{Lu})"); - for (int j = 0; j < names.length; j++) { - final int maxPortionLength = maxLength / names.length; - if (names[j].length() > maxPortionLength) { - names[j] = names[j].substring(0, maxPortionLength); - } - } - return StringUtils.arrayToDelimitedString(names, ""); - } - } interface AroundFilterObservation extends FilterObservation { 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 f519d11f53c..5e8c2a47d5f 100644 --- a/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java +++ b/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java @@ -16,8 +16,7 @@ package org.springframework.security.web; -import java.lang.reflect.Field; -import java.lang.reflect.Method; +import java.io.IOException; import java.util.List; import io.micrometer.observation.Observation; @@ -25,6 +24,9 @@ import io.micrometer.observation.ObservationRegistry; import jakarta.servlet.Filter; import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; @@ -80,7 +82,6 @@ void decorateFiltersWhenDefaultsThenObserves() throws Exception { FilterChain chain = mock(FilterChain.class); Filter filter = mock(Filter.class); FilterChain decorated = decorator.decorate(chain, List.of(filter)); - assertCompressedName(decorated); decorated.doFilter(new MockHttpServletRequest("GET", "/"), new MockHttpServletResponse()); verify(handler, times(2)).onStart(any()); ArgumentCaptor event = ArgumentCaptor.forClass(Observation.Event.class); @@ -90,22 +91,32 @@ void decorateFiltersWhenDefaultsThenObserves() throws Exception { assertThat(events.get(1).getName()).isEqualTo(filter.getClass().getSimpleName() + ".after"); } - void assertCompressedName(FilterChain filterChain) throws Exception { - assertThat(filterChain.getClass().getSimpleName()).isEqualTo("VirtualFilterChain"); - Field field = filterChain.getClass().getDeclaredField("additionalFilters"); - field.setAccessible(true); - List additionalFilters = - (List) field.get(filterChain); - assertThat(additionalFilters.size()).isEqualTo(1); - final ObservationFilterChainDecorator.ObservationFilter observationFilter = additionalFilters.get(0); - assertThat(observationFilter.getObservationName()).isEqualTo(observationFilter.getName()); - Method method = observationFilter.getClass().getDeclaredMethod("compressName", String.class); - method.setAccessible(true); - String compressed = (String) method.invoke(observationFilter, "ObservationFilterChainDecoratorTests"); - assertThat(compressed).isEqualTo("ObservationFilterChainDecoratorTests"); - String fakeCompressed = (String) method.invoke(observationFilter, - "ObservationFilterChainDecoratorTestsObservationFilterChainDecoratorTests"); - assertThat(fakeCompressed).isEqualTo("ObserFilteChainDecorTestsObserFilteChainDecorTests"); + @Test + void decorateFiltersWhenDefaultsThenUsesEventName() 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); + Filter filter = new BasicAuthenticationFilter(); + FilterChain decorated = decorator.decorate(chain, List.of(filter)); + decorated.doFilter(new MockHttpServletRequest("GET", "/"), new MockHttpServletResponse()); + ArgumentCaptor event = ArgumentCaptor.forClass(Observation.Event.class); + verify(handler, times(2)).onEvent(event.capture(), any()); + List events = event.getAllValues(); + assertThat(events.get(0).getName()).isEqualTo("authentication.basic.before"); + assertThat(events.get(1).getName()).isEqualTo("authentication.basic.after"); + } + + private static class BasicAuthenticationFilter implements Filter { + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + chain.doFilter(request, response); + } + } }