Feature/whitelisted urls ignore tokens no matter valid invalid or expired extra filter validation (#25)

AadAppRoleStatelessAuthenticationFilter tries to get a valid UserPrincipal on every request (no matter if it is whitelisted).

That's ok if token is not existing, if token is invalid, but if token is valid but expired, AadAppRoleStatelessAuthenticationFilter rejects request.

That's an issue, and sadly, AadAppRoleStatelessAuthenticationFilter does not offer an option to ignore that.

This MR will introduce a custom preFilter, ConditionalAadFilter, that will skip AadAppRoleStatelessAuthenticationFilter if request is whitelisted.

Author: sergio-soria-bi

src/main/java/org/opendevstack/component_catalog/config/SecurityConfiguration.java

@@ -2,21 +2,23 @@
 
 import com.azure.spring.cloud.autoconfigure.implementation.aad.filter.AadAppRoleStatelessAuthenticationFilter;
 import lombok.RequiredArgsConstructor;
+import org.opendevstack.component_catalog.config.azure.ConditionalAadFilter;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Primary;
 import org.springframework.core.annotation.Order;
 import org.springframework.http.HttpMethod;
 import org.springframework.security.config.Customizer;
 import org.springframework.security.config.annotation.web.builders.HttpSecurity;
-import org.springframework.security.config.annotation.web.configuration.WebSecurityCustomizer;
 import org.springframework.security.config.annotation.web.configurers.CsrfConfigurer;
 import org.springframework.security.config.http.SessionCreationPolicy;
 import org.springframework.security.config.web.PathPatternRequestMatcherBuilderFactoryBean;
 import org.springframework.security.web.SecurityFilterChain;
 import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter;
 import org.springframework.security.web.authentication.www.BasicAuthenticationEntryPoint;
 import org.springframework.security.web.servlet.util.matcher.PathPatternRequestMatcher;
+import org.springframework.security.web.util.matcher.OrRequestMatcher;
+import org.springframework.security.web.util.matcher.RequestMatcher;
 import org.springframework.web.cors.CorsConfiguration;
 
 @Configuration
@@ -35,19 +37,6 @@ PathPatternRequestMatcherBuilderFactoryBean pathPatternRequestMatcherBuilderFact
         return new PathPatternRequestMatcherBuilderFactoryBean();
     }
 
-    @Bean
-    public WebSecurityCustomizer webSecurityCustomizer() {
-        return web -> web.ignoring().requestMatchers(
-                "/swagger-ui/**",
-                "/v3/api-docs/**",
-                "/v1/catalog-items/*/user-actions/**",
-                "/v1/user-actions/**",
-                "/v1/provision/*/*",
-                "/v1/schema-validation/**",
-                "/actuator/health"
-        );
-    }
-
     /**
      * Chain #1: ONLY for DELETE /v1/provision/* -> HTTP Basic with role PROVISIONER
      */
@@ -79,20 +68,39 @@ SecurityFilterChain basicForProvisionDelete(HttpSecurity http) throws Exception
     @Order(2)
     public SecurityFilterChain aadForEverythingElse(HttpSecurity http) throws Exception {
 
+        RequestMatcher protectedEndpoints = new OrRequestMatcher(
+                PathPatternRequestMatcher.withDefaults().matcher("/v1/**"),
+                PathPatternRequestMatcher.withDefaults().matcher("/actuator/**")
+        );
+
+        RequestMatcher whitelistedEndpoints = new OrRequestMatcher(
+                PathPatternRequestMatcher.withDefaults().matcher("/v1/catalog-items/*/user-actions/**"),
+                PathPatternRequestMatcher.withDefaults().matcher("/v1/user-actions/**"),
+                PathPatternRequestMatcher.withDefaults().matcher("/v1/schema-validation/**"),
+                PathPatternRequestMatcher.withDefaults().matcher("/swagger-ui/**"),
+                PathPatternRequestMatcher.withDefaults().matcher("/v3/api-docs/**"),
+                PathPatternRequestMatcher.withDefaults().matcher("/v1/user-actions/**"),
+                PathPatternRequestMatcher.withDefaults().matcher("/v1/provision/*/*"),
+                PathPatternRequestMatcher.withDefaults().matcher("/actuator/health")
+        );
+
         http
-                .authorizeHttpRequests(request -> request
-                        .requestMatchers("/v1/**", "/actuator/**")
-                        .hasAuthority("ROLE_USER") // If required, change or add proper roles set by AAD
+                .authorizeHttpRequests(req -> req
+                        .requestMatchers(
+                                whitelistedEndpoints
+                        ).permitAll()
+                        .anyRequest().hasAuthority("ROLE_USER")
                 )
                 .csrf(CsrfConfigurer::disable) //NOSONAR required for /actuator endpoints, STATELESS prevents CSRF
                 .cors(c -> c.configurationSource(request ->
                         new CorsConfiguration().applyPermitDefaultValues()))
                 .sessionManagement(configurer ->
                         // Avoid session caching and validation e.g. via JSESSIONID cookie, as we are stateless
                         configurer.sessionCreationPolicy(SessionCreationPolicy.STATELESS))
-
-                // 2) Azure AD (bearer/JWT) for the rest
-                .addFilterBefore(aadAuthFilter, UsernamePasswordAuthenticationFilter.class);
+                .addFilterBefore(
+                        new ConditionalAadFilter(aadAuthFilter, protectedEndpoints, whitelistedEndpoints),
+                        UsernamePasswordAuthenticationFilter.class
+                );
 
         return http.build();
     }

src/main/java/org/opendevstack/component_catalog/config/azure/ConditionalAadFilter.java

@@ -0,0 +1,55 @@
+package org.opendevstack.component_catalog.config.azure;
+
+import com.azure.spring.cloud.autoconfigure.implementation.aad.filter.AadAppRoleStatelessAuthenticationFilter;
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import lombok.extern.slf4j.Slf4j;
+import org.jspecify.annotations.NonNull;
+import org.springframework.security.web.util.matcher.RequestMatcher;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import java.io.IOException;
+
+@Slf4j
+public class ConditionalAadFilter extends OncePerRequestFilter {
+
+    // Ideally, we should create different filters and order them in the security configuration.
+    // The thing is, AadAppRoleStatelessAuthenticationFilter is not ordered, so if we try to do so, spring will complain.
+    // The solution would be, wrap it and add an @order annotation, or wrap it in the conditional filter itself, as we do here.
+    private final AadAppRoleStatelessAuthenticationFilter delegate;
+    private final RequestMatcher protectedEndpoints;
+    private final RequestMatcher whitelistedEndpoints;
+
+    public ConditionalAadFilter(
+            AadAppRoleStatelessAuthenticationFilter delegate,
+            RequestMatcher protectedEndpoints,
+            RequestMatcher whitelistedEndpoints) {
+        this.delegate = delegate;
+        this.protectedEndpoints = protectedEndpoints;
+        this.whitelistedEndpoints = whitelistedEndpoints;
+    }
+
+    @Override
+    protected boolean shouldNotFilter(@NonNull HttpServletRequest request) {
+        var shouldNotFilter = whitelistedEndpoints.matches(request) || !protectedEndpoints.matches(request);
+
+        log.debug("Validating url {}: protectedEndpoints matches? {}, whitelistedEndpoints matches? {}, shouldNotFilter? {}",
+                request.getRequestURI(),
+                protectedEndpoints.matches(request),
+                whitelistedEndpoints.matches(request),
+                shouldNotFilter);
+
+        return shouldNotFilter;
+    }
+
+    @Override
+    protected void doFilterInternal(
+            @NonNull HttpServletRequest request,
+            @NonNull HttpServletResponse response,
+            @NonNull FilterChain filterChain) throws IOException, ServletException {
+
+        delegate.doFilter(request, response, filterChain);
+    }
+}

src/main/resources/application-local.yml

@@ -4,6 +4,7 @@ logging:
     org.springframework: INFO
     org.springframework.security: INFO
     com.azure.spring.cloud.autoconfigure: INFO
+    org.opendevstack.component_catalog.config.azure: INFO
     org.opendevstack.component_catalog.config: INFO
     org.opendevstack.component_catalog.server.controllers: INFO
     org.opendevstack.component_catalog.server.api: INFO

src/test/java/org/opendevstack/component_catalog/config/azure/ConditionalAadFilterTest.java

@@ -0,0 +1,85 @@
+package org.opendevstack.component_catalog.config.azure;
+
+import com.azure.spring.cloud.autoconfigure.implementation.aad.filter.AadAppRoleStatelessAuthenticationFilter;
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.security.web.util.matcher.RequestMatcher;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.*;
+
+class ConditionalAadFilterTest {
+
+    private AadAppRoleStatelessAuthenticationFilter delegate;
+    private RequestMatcher protectedEndpoints;
+    private RequestMatcher whitelistedEndpoints;
+
+    private ConditionalAadFilter filter;
+
+    @BeforeEach
+    void setup() {
+        delegate = mock(AadAppRoleStatelessAuthenticationFilter.class);
+        protectedEndpoints = mock(RequestMatcher.class);
+        whitelistedEndpoints = mock(RequestMatcher.class);
+
+        filter = new ConditionalAadFilter(delegate, protectedEndpoints, whitelistedEndpoints);
+    }
+
+    @Test
+    void givenWhitelistedPath_whenShouldNotFilter_thenTrue() {
+        // given
+        HttpServletRequest request = mock(HttpServletRequest.class);
+        when(whitelistedEndpoints.matches(request)).thenReturn(true);
+
+        // when
+        boolean result = filter.shouldNotFilter(request);
+
+        // then
+        assertThat(result).isTrue();
+    }
+
+    @Test
+    void givenProtectedAndNotWhitelisted_whenShouldNotFilter_thenFalse() {
+        // given
+        HttpServletRequest request = mock(HttpServletRequest.class);
+        when(whitelistedEndpoints.matches(request)).thenReturn(false);
+        when(protectedEndpoints.matches(request)).thenReturn(true);
+
+        // when
+        boolean result = filter.shouldNotFilter(request);
+
+        // then
+        assertThat(result).isFalse();
+    }
+
+    @Test
+    void givenNotProtectedAndNotWhitelisted_whenShouldNotFilter_thenTrue() {
+        // given
+        HttpServletRequest request = mock(HttpServletRequest.class);
+        when(whitelistedEndpoints.matches(request)).thenReturn(false);
+        when(protectedEndpoints.matches(request)).thenReturn(false);
+
+        // when
+        boolean result = filter.shouldNotFilter(request);
+
+        // then
+        assertThat(result).isTrue();
+    }
+
+    @Test
+    void givenAnyRequest_whenDoFilterInternal_thenDelegateIsCalled() throws Exception {
+        // given
+        HttpServletRequest request = mock(HttpServletRequest.class);
+        HttpServletResponse response = mock(HttpServletResponse.class);
+        FilterChain filterChain = mock(FilterChain.class);
+
+        // when
+        filter.doFilterInternal(request, response, filterChain);
+
+        // then
+        verify(delegate).doFilter(request, response, filterChain);
+    }
+}