Extract constants to app properties

pinterest · Nov 25, 2024 · e0aaa45 · e0aaa45
1 parent e3b75f5
commit e0aaa45
Show file tree

Hide file tree

Showing 7 changed files with 81 additions and 37 deletions.
diff --git a/webapp/src/main/java/com/box/l10n/mojito/security/HeaderSecurityConfig.java b/webapp/src/main/java/com/box/l10n/mojito/security/HeaderSecurityConfig.java
@@ -1,16 +1,20 @@
 package com.box.l10n.mojito.security;
 
 import org.springframework.beans.factory.annotation.Value;
-import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression;
 import org.springframework.context.annotation.Configuration;
 
 @Configuration()
-@ConditionalOnExpression("'${l10n.security.authenticationType:}'.toUpperCase().contains('HEADER')")
 public class HeaderSecurityConfig {
 
   @Value("${l10n.spring.security.header.user.identifyingHeader:}")
   public String userIdentifyingHeader;
 
   @Value("${l10n.spring.security.header.service.identifyingHeader:}")
   public String serviceIdentifyingHeader;
+
+  @Value("${l10n.spring.security.header.service.identifyingPrefix:}")
+  public String servicePrefix;
+
+  @Value("${l10n.spring.security.header.service.delimiter:/}")
+  public String serviceDelimiter;
 }
diff --git a/webapp/src/main/java/com/box/l10n/mojito/security/ServiceDisambiguator.java b/webapp/src/main/java/com/box/l10n/mojito/security/ServiceDisambiguator.java
@@ -2,12 +2,15 @@
 
 import com.box.l10n.mojito.entity.security.user.User;
 import java.util.List;
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
 import org.springframework.stereotype.Component;
 
 @Component
 @ConditionalOnProperty("l10n.spring.security.services.enableFuzzyMatch")
 public class ServiceDisambiguator {
+  @Autowired HeaderSecurityConfig headerSecurityConfig;
+
   /***
    * This method finds the service with the longest shared path (basically a parent in path directory)
    * The service uses its parents user if it exists. If the exact service exists, then that user is
@@ -21,7 +24,7 @@ public User findServiceWithCommonAncestor(List<User> services, String servicePat
       return null;
     }
 
-    String[] servicePathElements = servicePath.split("/");
+    String[] servicePathElements = servicePath.split(headerSecurityConfig.serviceDelimiter);
     String longestAncestorPath = null;
     User closestService = null;
 
@@ -32,7 +35,8 @@ public User findServiceWithCommonAncestor(List<User> services, String servicePat
         return currentService;
       }
 
-      String[] currentPathElements = currentServicePath.split("/");
+      String[] currentPathElements =
+          currentServicePath.split(headerSecurityConfig.serviceDelimiter);
       StringBuilder commonAncestor = new StringBuilder();
 
       int minLength = Math.min(servicePathElements.length, currentPathElements.length);
@@ -41,7 +45,7 @@ public User findServiceWithCommonAncestor(List<User> services, String servicePat
         if (servicePathElements[i].equals(currentPathElements[i])) {
           commonAncestor.append(servicePathElements[i]);
           if (i < minLength - 1) {
-            commonAncestor.append("/");
+            commonAncestor.append(headerSecurityConfig.serviceDelimiter);
           }
         }
       }

diff --git a/webapp/src/main/java/com/box/l10n/mojito/security/UserDetailServiceAuthWrapper.java b/webapp/src/main/java/com/box/l10n/mojito/security/UserDetailServiceAuthWrapper.java
@@ -9,16 +9,20 @@
 public class UserDetailServiceAuthWrapper
     implements AuthenticationUserDetailsService<PreAuthenticatedAuthenticationToken> {
   protected PrincipalDetailService userDetailsService = null;
+  protected HeaderSecurityConfig headerSecurityConfig;
 
-  public UserDetailServiceAuthWrapper(PrincipalDetailService userDetailsService) {
+  public UserDetailServiceAuthWrapper(
+      PrincipalDetailService userDetailsService, HeaderSecurityConfig headerSecurityConfig) {
     this.userDetailsService = userDetailsService;
+    this.headerSecurityConfig = headerSecurityConfig;
   }
 
   @Override
   public UserDetails loadUserDetails(PreAuthenticatedAuthenticationToken token)
       throws UsernameNotFoundException {
     String username = token.getName();
-    boolean isService = username.contains("spiffe://");
+    boolean isService =
+        headerSecurityConfig != null && username.contains(headerSecurityConfig.servicePrefix);
     if (isService) {
       return this.userDetailsService.loadServiceWithName(username);
     } else {

diff --git a/webapp/src/main/java/com/box/l10n/mojito/security/WebSecurityHeaderConfig.java b/webapp/src/main/java/com/box/l10n/mojito/security/WebSecurityHeaderConfig.java
@@ -1,5 +1,6 @@
 package com.box.l10n.mojito.security;
 
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
@@ -10,12 +11,15 @@
 @ConditionalOnExpression("'${l10n.security.authenticationType:}'.toUpperCase().contains('HEADER')")
 @Configuration
 class WebSecurityHeaderConfig {
+  @Autowired HeaderSecurityConfig headerSecurityConfig;
+
   @Bean
   PreAuthenticatedAuthenticationProvider preAuthenticatedAuthenticationProvider() {
     PreAuthenticatedAuthenticationProvider preAuthenticatedAuthenticationProvider =
         new PreAuthenticatedAuthenticationProvider();
     UserDetailServiceAuthWrapper userDetailsByNameServiceWrapper =
-        new UserDetailServiceAuthWrapper(getPrincipalDetailsServiceCreatePartial());
+        new UserDetailServiceAuthWrapper(
+            getPrincipalDetailsServiceCreatePartial(), headerSecurityConfig);
     preAuthenticatedAuthenticationProvider.setPreAuthenticatedUserDetailsService(
         userDetailsByNameServiceWrapper);
     return preAuthenticatedAuthenticationProvider;

diff --git a/webapp/src/main/java/com/box/l10n/mojito/service/security/user/UserRepository.java b/webapp/src/main/java/com/box/l10n/mojito/service/security/user/UserRepository.java
@@ -61,11 +61,12 @@ Page<User> findByUsernameOrName(
       value =
           """
     SELECT u FROM  User u
-      WHERE LOWER(u.username) LIKE 'spiffe://%'
-        AND u.username <> '' AND u.username != NULL
+      WHERE COALESCE(u.username, '') <> ''
+        AND LOWER(u.username) LIKE CONCAT(LOWER(:servicePrefix), '%')
         AND LOWER(:serviceName) LIKE CONCAT(LOWER(u.username), '%')
-        AND u.enabled = true
+        AND u.enabled IS TRUE
     """)
   @EntityGraph(value = "User.legacy", type = EntityGraphType.FETCH)
-  Optional<List<User>> findService(@Param("serviceName") String serviceName);
+  Optional<List<User>> findService(
+      @Param("serviceName") String serviceName, @Param("servicePrefix") String servicePrefix);
 }
diff --git a/webapp/src/main/java/com/box/l10n/mojito/service/security/user/UserService.java b/webapp/src/main/java/com/box/l10n/mojito/service/security/user/UserService.java
@@ -9,12 +9,18 @@
 import com.box.l10n.mojito.pagerduty.PagerDutyIntegrationService;
 import com.box.l10n.mojito.pagerduty.PagerDutyPayload;
 import com.box.l10n.mojito.security.AuditorAwareImpl;
+import com.box.l10n.mojito.security.HeaderSecurityConfig;
 import com.box.l10n.mojito.security.Role;
 import com.box.l10n.mojito.security.ServiceDisambiguator;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Sets;
-import java.util.*;
+import io.micrometer.core.instrument.MeterRegistry;
+import io.micrometer.core.instrument.Tags;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
 import org.apache.commons.lang3.RandomStringUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.hibernate.Hibernate;
@@ -23,6 +29,7 @@
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.Pageable;
 import org.springframework.security.access.AccessDeniedException;
+import org.springframework.security.core.userdetails.UsernameNotFoundException;
 import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
 import org.springframework.stereotype.Component;
 import org.springframework.transaction.annotation.Transactional;
@@ -44,17 +51,14 @@ public class UserService {
 
   @Autowired AuditorAwareImpl auditorAwareImpl;
 
-  PagerDutyClient pagerDutyClient;
+  @Autowired(required = false)
+  ServiceDisambiguator serviceDisambiguator;
 
-  @Autowired
-  public UserService(PagerDutyIntegrationService pagerDutyIntegrationService) {
-    Optional<PagerDutyClient> defaultPagerDutyClient =
-        pagerDutyIntegrationService.getDefaultPagerDutyClient();
-    if (defaultPagerDutyClient.isEmpty()) {
-      throw new RuntimeException("Default pager duty client is required");
-    }
-    this.pagerDutyClient = defaultPagerDutyClient.get();
-  }
+  @Autowired HeaderSecurityConfig headerSecurityConfig;
+
+  @Autowired PagerDutyIntegrationService pagerDutyIntegrationService;
+
+  @Autowired MeterRegistry meterRegistry;
 
   /**
    * Allow PMs and ADMINs to create / edit users. However, a PM user can not create / edit ADMIN
@@ -402,9 +406,6 @@ public Page<User> findByUsernameOrName(String username, String search, Pageable
     return users;
   }
 
-  @Autowired(required = false)
-  ServiceDisambiguator serviceDisambiguator;
-
   /**
    * Gets a service by name and if it doesn't exist create a created user.
    *
@@ -419,22 +420,50 @@ public User getServiceAccountUser(String serviceName) {
       return userRepository.findByUsername(serviceName);
     }
 
-    Optional<List<User>> users = userRepository.findService(serviceName);
+    Optional<List<User>> users =
+        userRepository.findService(serviceName, headerSecurityConfig.servicePrefix);
     if (users.isEmpty()) {
       sendPagerDutyNotification(serviceName);
+      logger.error(
+          "Service '{}' attempted and failed authentication. No matching services found.",
+          serviceName);
       return null;
     }
 
     User matchingUser =
         serviceDisambiguator.findServiceWithCommonAncestor(users.get(), serviceName);
     if (matchingUser == null) {
       sendPagerDutyNotification(serviceName);
+      logger.error(
+          "Service '{}' attempted and failed authentication. No services could be fuzzy matched",
+          serviceName);
+      throw new UsernameNotFoundException("Service with name '" + serviceName + "' was not found");
     }
     return matchingUser;
   }
 
+  private String normalizeServiceName(String serviceName) {
+    return serviceName
+        .replaceAll(headerSecurityConfig.servicePrefix, "")
+        .replaceAll(headerSecurityConfig.serviceDelimiter, ":");
+  }
+
   private void sendPagerDutyNotification(String serviceName) {
-    String dedupKey = "mojito:auth:unrecognized:service:account:" + serviceName;
+    Optional<PagerDutyClient> defaultPagerDutyClient =
+        pagerDutyIntegrationService.getDefaultPagerDutyClient();
+    if (defaultPagerDutyClient.isEmpty()) {
+      logger.error("No default Pager Duty client configured");
+      meterRegistry
+          .counter(
+              "UserService.sendPagerDutyNotification.defaultPagerDutyClientNotConfigured",
+              Tags.of("serviceName", serviceName))
+          .increment();
+      return;
+    }
+
+    PagerDutyClient pagerDutyClient = defaultPagerDutyClient.get();
+    String dedupKey =
+        "mojito:auth:unrecognized:service:account:" + normalizeServiceName(serviceName);
     try {
       pagerDutyClient.triggerIncident(
           dedupKey,

diff --git a/webapp/src/test/java/com/box/l10n/mojito/security/ServiceDisambiguatorTest.java b/webapp/src/test/java/com/box/l10n/mojito/security/ServiceDisambiguatorTest.java
@@ -8,18 +8,16 @@
 import java.util.List;
 import java.util.stream.IntStream;
 import org.junit.Test;
-import org.springframework.boot.test.context.SpringBootTest;
-import org.springframework.test.context.TestPropertySource;
 
-@SpringBootTest
-@TestPropertySource(
-    properties = {
-      "app.name=InlineTestApplication",
-      "l10n.spring.security.header.service.prefix=spiffe://"
-    })
 public class ServiceDisambiguatorTest {
+  ServiceDisambiguator serviceDisambiguator;
 
-  ServiceDisambiguator serviceDisambiguator = new ServiceDisambiguator();
+  public ServiceDisambiguatorTest() {
+    this.serviceDisambiguator = new ServiceDisambiguator();
+    this.serviceDisambiguator.headerSecurityConfig = new HeaderSecurityConfig();
+    this.serviceDisambiguator.headerSecurityConfig.servicePrefix = "spiffe://";
+    this.serviceDisambiguator.headerSecurityConfig.serviceDelimiter = "/";
+  }
 
   @Test
   public void testFindServiceWithShortestSharedAncestor_EdgeCases() {