Merge pull request #16 from georchestra/fix_global_acces_rules

Fix access rules order and add integration tests
georchestra · Apr 25, 2022 · 4450068 · 4450068
2 parents 0fe8ca4 + 6e12e8a
commit 4450068
Show file tree

Hide file tree

Showing 18 changed files with 506 additions and 60 deletions.
diff --git a/gateway/pom.xml b/gateway/pom.xml
@@ -14,6 +14,16 @@
     <imageTag>${project.version}</imageTag>
     <spring-boot.build-image.imageName>georchestra/gateway:${imageTag}</spring-boot.build-image.imageName>
   </properties>
+  <dependencyManagement>
+    <dependencies>
+      <dependency>
+        <groupId>com.github.tomakehurst</groupId>
+        <artifactId>wiremock-jre8</artifactId>
+        <!-- override provided version 2.26.3 -->
+        <version>2.33.1</version>
+      </dependency>
+    </dependencies>
+  </dependencyManagement>
   <dependencies>
     <dependency>
       <groupId>org.georchestra</groupId>
@@ -70,13 +80,33 @@
       <artifactId>spring-boot-starter-test</artifactId>
       <scope>test</scope>
     </dependency>
-    <!-- <dependency> -->
-    <!-- <groupId>org.springframework.security</groupId> -->
-    <!-- <artifactId>spring-security-test</artifactId> -->
-    <!-- <scope>test</scope> -->
-    <!-- </dependency> -->
+    <dependency>
+      <groupId>com.github.tomakehurst</groupId>
+      <artifactId>wiremock-jre8</artifactId>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.springframework.security</groupId>
+      <artifactId>spring-security-test</artifactId>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
   <build>
+    <pluginManagement>
+      <plugins>
+        <plugin>
+          <groupId>org.apache.maven.plugins</groupId>
+          <artifactId>maven-failsafe-plugin</artifactId>
+          <!-- override provided version 2.22.2 -->
+          <version>3.0.0-M6</version>
+        </plugin>
+        <plugin>
+          <groupId>org.jacoco</groupId>
+          <artifactId>jacoco-maven-plugin</artifactId>
+          <version>0.8.8</version>
+        </plugin>
+      </plugins>
+    </pluginManagement>
     <plugins>
       <plugin>
         <groupId>net.revelc.code.formatter</groupId>
@@ -174,6 +204,18 @@
           <flattenMode>oss</flattenMode>
         </configuration>
       </plugin>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-failsafe-plugin</artifactId>
+        <executions>
+          <execution>
+            <goals>
+              <goal>integration-test</goal>
+              <goal>verify</goal>
+            </goals>
+          </execution>
+        </executions>
+      </plugin>
       <plugin>
         <groupId>org.springframework.boot</groupId>
         <artifactId>spring-boot-maven-plugin</artifactId>
@@ -189,7 +231,6 @@
       <plugin>
         <groupId>org.jacoco</groupId>
         <artifactId>jacoco-maven-plugin</artifactId>
-        <version>0.8.8</version>
         <executions>
           <execution>
             <id>prepare-agent</id>

diff --git a/gateway/src/main/java/org/georchestra/gateway/filter/headers/HeaderContributor.java b/gateway/src/main/java/org/georchestra/gateway/filter/headers/HeaderContributor.java
@@ -31,6 +31,7 @@
 import org.springframework.security.config.web.server.ServerHttpSecurity;
 import org.springframework.web.server.ServerWebExchange;
 
+import lombok.NonNull;
 import lombok.extern.slf4j.Slf4j;
 
 /**
@@ -44,7 +45,7 @@
  * @see GeorchestraUserHeadersContributor
  * @see GeorchestraOrganizationHeadersContributor
  */
-@Slf4j
+@Slf4j(topic = "org.georchestra.gateway.filter.headers")
 public abstract class HeaderContributor implements Ordered {
 
     /**
@@ -69,25 +70,37 @@ public abstract class HeaderContributor implements Ordered {
         return 0;
     }
 
-    protected void add(HttpHeaders target, String header, Optional<Boolean> enabled, Optional<String> value) {
+    protected void add(@NonNull HttpHeaders target, @NonNull String header, @NonNull Optional<Boolean> enabled,
+            @NonNull Optional<String> value) {
         add(target, header, enabled, value.orElse(null));
     }
 
-    protected void add(HttpHeaders target, String header, Optional<Boolean> enabled, List<String> values) {
-        add(target, header, enabled, values.stream().collect(Collectors.joining(";")));
+    protected void add(@NonNull HttpHeaders target, @NonNull String header, @NonNull Optional<Boolean> enabled,
+            @NonNull List<String> values) {
+        String val = values.isEmpty() ? null : values.stream().collect(Collectors.joining(";"));
+        add(target, header, enabled, val);
     }
 
-    protected void add(HttpHeaders target, String header, Optional<Boolean> enabled, String value) {
+    protected void add(@NonNull HttpHeaders target, @NonNull String header, @NonNull Optional<Boolean> enabled,
+            String value) {
         if (enabled.orElse(Boolean.FALSE).booleanValue()) {
             if (null == value) {
-                log.debug("Value for header {} is not present", header);
+                log.trace("Value for header {} is not present", header);
             } else {
-                log.info("Appending header {}: {}", header, value);
+                log.debug("Appending header {}: {}", header, value);
                 target.add(header, value);
             }
         } else {
-            log.debug("Header {} is not enabled", header);
+            log.trace("Header {} is not enabled", header);
         }
     }
 
+    protected void add(@NonNull HttpHeaders target, @NonNull String header, String value) {
+        if (null == value) {
+            log.trace("Value for header {} is not present", header);
+        } else {
+            log.debug("Appending header {}: {}", header, value);
+            target.add(header, value);
+        }
+    }
 }
diff --git a/gateway/src/main/java/org/georchestra/gateway/filter/headers/HeaderFiltersConfiguration.java b/gateway/src/main/java/org/georchestra/gateway/filter/headers/HeaderFiltersConfiguration.java
@@ -19,11 +19,13 @@
 package org.georchestra.gateway.filter.headers;
 
 import java.util.List;
+import java.util.function.BooleanSupplier;
 
 import org.georchestra.gateway.filter.headers.providers.GeorchestraOrganizationHeadersContributor;
 import org.georchestra.gateway.filter.headers.providers.GeorchestraUserHeadersContributor;
 import org.georchestra.gateway.filter.headers.providers.JsonPayloadHeadersContributor;
 import org.georchestra.gateway.filter.headers.providers.SecProxyHeaderContributor;
+import org.georchestra.gateway.model.GatewayConfigProperties;
 import org.springframework.cloud.gateway.filter.factory.GatewayFilterFactory;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.annotation.Bean;
@@ -51,8 +53,9 @@ public class HeaderFiltersConfiguration {
         return new GeorchestraUserHeadersContributor();
     }
 
-    public @Bean SecProxyHeaderContributor secProxyHeaderProvider() {
-        return new SecProxyHeaderContributor();
+    public @Bean SecProxyHeaderContributor secProxyHeaderProvider(GatewayConfigProperties configProps) {
+        BooleanSupplier secProxyEnabledSupplier = () -> configProps.getDefaultHeaders().getProxy().orElse(false);
+        return new SecProxyHeaderContributor(secProxyEnabledSupplier);
     }
 
     public @Bean GeorchestraOrganizationHeadersContributor organizationSecurityHeadersProvider() {

diff --git a/...c/main/java/org/georchestra/gateway/filter/headers/RemoveHeadersGatewayFilterFactory.java b/...c/main/java/org/georchestra/gateway/filter/headers/RemoveHeadersGatewayFilterFactory.java
@@ -60,7 +60,7 @@
  * </pre>
  * 
  */
-@Slf4j(topic = "org.georchestra.gateway.headers")
+@Slf4j(topic = "org.georchestra.gateway.filter.headers")
 public class RemoveHeadersGatewayFilterFactory extends AbstractGatewayFilterFactory<RegExConfig> {
 
     public RemoveHeadersGatewayFilterFactory() {
@@ -119,7 +119,7 @@ boolean anyMatches(@NonNull HttpHeaders headers) {
         void removeMatching(@NonNull HttpHeaders headers) {
             new HashSet<>(headers.keySet()).stream()//
                     .filter(this::matches)//
-                    .peek(name -> log.debug("Removing header {}", name))//
+                    .peek(name -> log.trace("Removing header {}", name))//
                     .forEach(headers::remove);
         }
     }

diff --git a/...main/java/org/georchestra/gateway/filter/headers/providers/SecProxyHeaderContributor.java b/...main/java/org/georchestra/gateway/filter/headers/providers/SecProxyHeaderContributor.java
@@ -18,22 +18,30 @@
  */
 package org.georchestra.gateway.filter.headers.providers;
 
+import java.util.function.BooleanSupplier;
 import java.util.function.Consumer;
 
 import org.georchestra.gateway.filter.headers.HeaderContributor;
 import org.springframework.http.HttpHeaders;
 import org.springframework.web.server.ServerWebExchange;
 
+import lombok.NonNull;
+import lombok.RequiredArgsConstructor;
+
 /**
- * Unconditionally contributes the {@literal sec-proxy: true} request header,
- * which is required by all backend services as a flag indicating the request is
- * authenticated.
+ * Contributes the {@literal sec-proxy: true} request header based on the value
+ * returned by the provided {@link BooleanSupplier}, which is required by all
+ * backend services as a flag indicating the request is authenticated.
  */
+@RequiredArgsConstructor
 public class SecProxyHeaderContributor extends HeaderContributor {
 
+    private final @NonNull BooleanSupplier secProxyHeaderEnabled;
+
     public @Override Consumer<HttpHeaders> prepare(ServerWebExchange exchange) {
         return headers -> {
-            headers.add("sec-proxy", "true");
+            if (secProxyHeaderEnabled.getAsBoolean())
+                add(headers, "sec-proxy", "true");
         };
     }
 }
diff --git a/gateway/src/main/java/org/georchestra/gateway/model/RoleBasedAccessRule.java b/gateway/src/main/java/org/georchestra/gateway/model/RoleBasedAccessRule.java
@@ -45,6 +45,12 @@ public class RoleBasedAccessRule {
      */
     private List<String> interceptUrl = List.of();
 
+    /**
+     * Highest precedence rule, if {@code true}, forbids access to the intercepted
+     * URLs
+     */
+    private boolean forbidden = false;
+
     /**
      * Whether anonymous (unauthenticated) access is to be granted to the
      * intercepted URIs. If {@code true}, no further specification is applied to the

diff --git a/...ay/src/main/java/org/georchestra/gateway/security/ResolveGeorchestraUserGlobalFilter.java b/...ay/src/main/java/org/georchestra/gateway/security/ResolveGeorchestraUserGlobalFilter.java
@@ -47,7 +47,7 @@
  * @see GeorchestraUserMapper
  */
 @RequiredArgsConstructor
-@Slf4j
+@Slf4j(topic = "org.georchestra.gateway.security")
 public class ResolveGeorchestraUserGlobalFilter implements GlobalFilter, Ordered {
 
     public static final int ORDER = RouteToRequestUrlFilter.ROUTE_TO_URL_FILTER_ORDER + 1;

diff --git a/...way/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizer.java b/...way/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizer.java
@@ -61,18 +61,22 @@ public void customize(ServerHttpSecurity http) {
 
         AuthorizeExchangeSpec authorizeExchange = http.authorizeExchange();
 
-        log.info("Applying global access rules...");
-        apply(authorizeExchange, config.getGlobalAccessRules());
+        // apply service-specific rules before global rules, order matters, and
+        // otherwise global path matches would be applied before service ones.
 
         config.getServices().forEach((name, service) -> {
             log.info("Applying access rules for backend service '{}'", name);
-            apply(authorizeExchange, service.getAccessRules());
+            apply(name, authorizeExchange, service.getAccessRules());
         });
+
+        log.info("Applying global access rules...");
+        apply("global", authorizeExchange, config.getGlobalAccessRules());
     }
 
-    private void apply(AuthorizeExchangeSpec authorizeExchange, List<RoleBasedAccessRule> accessRules) {
+    private void apply(String serviceName, AuthorizeExchangeSpec authorizeExchange,
+            List<RoleBasedAccessRule> accessRules) {
         if (accessRules == null || accessRules.isEmpty()) {
-            log.info("No access rules found.");
+            log.debug("No {} access rules found.", serviceName);
             return;
         }
         for (RoleBasedAccessRule rule : accessRules) {
@@ -83,18 +87,22 @@ private void apply(AuthorizeExchangeSpec authorizeExchange, List<RoleBasedAccess
     @VisibleForTesting
     void apply(AuthorizeExchangeSpec authorizeExchange, RoleBasedAccessRule rule) {
         final List<String> antPatterns = resolveAntPatterns(rule);
+        final boolean forbidden = rule.isForbidden();
         final boolean anonymous = rule.isAnonymous();
         final List<String> allowedRoles = rule.getAllowedRoles() == null ? List.of() : rule.getAllowedRoles();
         Access access = authorizeExchange(authorizeExchange, antPatterns);
-        if (anonymous) {
+        if (forbidden) {
+            log.debug("Denying access to everyone for {}", antPatterns);
+            denyAll(access);
+        } else if (anonymous) {
             log.debug("Granting anonymous access for {}", antPatterns);
             permitAll(access);
         } else if (allowedRoles.isEmpty()) {
             log.debug("Granting access to any authenticated user for {}", antPatterns);
             requireAuthenticatedUser(access);
         } else {
             List<String> roles = resolveRoles(antPatterns, allowedRoles);
-            log.debug("Granting access to roles {} for {}", antPatterns);
+            log.debug("Granting access to roles {} for {}", roles, antPatterns);
             hasAnyAuthority(access, roles);
         }
     }
@@ -115,10 +123,7 @@ Access authorizeExchange(AuthorizeExchangeSpec authorizeExchange, List<String> a
     }
 
     private List<String> resolveRoles(List<String> antPatterns, List<String> allowedRoles) {
-        List<String> roles = allowedRoles.stream().map(this::ensureRolePrefix).collect(Collectors.toList());
-        if (log.isDebugEnabled())
-            log.debug("Access rule: {} has any role: {}", antPatterns, roles.stream().collect(Collectors.joining(",")));
-        return roles;
+        return allowedRoles.stream().map(this::ensureRolePrefix).collect(Collectors.toList());
     }
 
     @VisibleForTesting
@@ -136,6 +141,11 @@ void permitAll(Access access) {
         access.permitAll();
     }
 
+    @VisibleForTesting
+    void denyAll(Access access) {
+        access.denyAll();
+    }
+
     private String ensureRolePrefix(@NonNull String roleName) {
         return roleName.startsWith("ROLE_") ? roleName : ("ROLE_" + roleName);
     }

diff --git a/...va/org/georchestra/gateway/security/oauth2/OAuth2AuthenticationTokenOpenIDUserMapper.java b/...va/org/georchestra/gateway/security/oauth2/OAuth2AuthenticationTokenOpenIDUserMapper.java
@@ -39,7 +39,7 @@
  * @author groldan
  *
  */
-@Slf4j
+@Slf4j(topic = "org.georchestra.gateway.security.oauth2")
 public class OAuth2AuthenticationTokenOpenIDUserMapper extends OAuth2AuthenticationTokenUserMapper {
 
     @Override

diff --git a/...ain/java/org/georchestra/gateway/security/oauth2/OAuth2AuthenticationTokenUserMapper.java b/...ain/java/org/georchestra/gateway/security/oauth2/OAuth2AuthenticationTokenUserMapper.java
@@ -26,7 +26,6 @@
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
-import org.georchestra.gateway.model.GeorchestraUsers;
 import org.georchestra.gateway.security.GeorchestraUserMapperExtension;
 import org.georchestra.security.model.GeorchestraUser;
 import org.slf4j.Logger;
@@ -42,7 +41,7 @@
  * @author groldan
  *
  */
-@Slf4j
+@Slf4j(topic = "org.georchestra.gateway.security.oauth2")
 public class OAuth2AuthenticationTokenUserMapper implements GeorchestraUserMapperExtension {
 
     @Override

diff --git a/gateway/src/main/resources/application.yml b/gateway/src/main/resources/application.yml
@@ -191,8 +191,14 @@ logging:
     root: warn
     '[org.springframework]': info
     '[org.springframework.cloud.gateway]': info
-    '[org.springframework.security]': debug
+    '[org.springframework.security]': info
     '[org.springframework.security.oauth2]': debug
     '[reactor.netty.http ]': debug
-    '[org.georchestra.gateway]': debug
+    '[org.georchestra.gateway]': info
+    '[org.georchestra.gateway.filter.headers]': debug
+    '[org.georchestra.gateway.config.security]': debug
+    '[org.georchestra.gateway.config.security.accessrules]': debug
+    '[org.georchestra.gateway.security.ldap]': debug
+    '[org.georchestra.gateway.security.oauth2]': debug
+