Try simplify disambiguation logic

webapp/src/main/java/com/box/l10n/mojito/security/ServiceDisambiguator.java

@@ -1,6 +1,7 @@
 package com.box.l10n.mojito.security;
 
 import com.box.l10n.mojito.entity.security.user.User;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
 import org.slf4j.Logger;
@@ -25,7 +26,7 @@ public class ServiceDisambiguator {
    * @param services list of relevant services
    * @return the User or null if none match
    */
-  public User findServiceWithCommonAncestor(List<User> services, String servicePath) {
+  public User getServiceWithCommonAncestor(List<User> services, String servicePath) {
     if (services == null || servicePath == null || services.isEmpty() || servicePath.isEmpty()) {
       logger.debug("No services found or no service path given");
       return null;
@@ -46,34 +47,22 @@ public User findServiceWithCommonAncestor(List<User> services, String servicePat
 
       String[] currentPathElements =
           currentServicePath.split(headerSecurityConfig.serviceDelimiter);
-      StringBuilder commonAncestor = new StringBuilder();
 
-      int minLength = Math.min(servicePathElements.length, currentPathElements.length);
-
-      for (int i = 0; i < minLength; i++) {
-        if (servicePathElements[i].equals(currentPathElements[i])) {
-          commonAncestor.append(servicePathElements[i]);
-          if (i < minLength - 1) {
-            commonAncestor.append(headerSecurityConfig.serviceDelimiter);
-          }
-        }
-      }
-
-      logger.debug("Common ancestor between services found: {}", commonAncestor);
-
-      // Update the service which shares the shortest path
-      String mostCommonAncestor = commonAncestor.toString();
-      if (mostCommonAncestor.isEmpty()) {
+      String commonAncestor = getCommonAncestorPath(servicePathElements, currentPathElements);
+      if (commonAncestor.isEmpty()) {
         continue;
       }
 
-      if (mostCommonAncestor.length() != currentServicePath.length()) {
+      // We ignore this service because it is a sibling (i.e different child service)
+      // example case: service doing auth -> test.com/infra/jenkins/agent3
+      // currentServicePath -> test.com/infra/jenkins/agent1
+      // commonAncestor -> test.com/infra/jenkins
+      if (commonAncestor.length() < currentServicePath.length()) {
         continue;
       }
 
-      if (longestAncestorPath == null
-          || mostCommonAncestor.length() > longestAncestorPath.length()) {
-        longestAncestorPath = mostCommonAncestor;
+      if (longestAncestorPath == null || commonAncestor.length() > longestAncestorPath.length()) {
+        longestAncestorPath = commonAncestor;
         closestService = currentService;
       }
     }
@@ -83,4 +72,18 @@ public User findServiceWithCommonAncestor(List<User> services, String servicePat
         Optional.ofNullable(closestService).map(User::getUsername).orElse("null"));
     return closestService;
   }
+
+  private String getCommonAncestorPath(String[] servicePathElements, String[] currentPathElements) {
+    StringBuilder commonAncestor = new StringBuilder();
+    int minLength = Math.min(servicePathElements.length, currentPathElements.length);
+    List<String> matchedElements = new ArrayList<>(currentPathElements.length);
+    for (int i = 0; i < minLength; i++) {
+      if (servicePathElements[i].equals(currentPathElements[i])) {
+        matchedElements.add(servicePathElements[i]);
+      }
+    }
+
+    logger.debug("Common ancestor between services found: {}", commonAncestor);
+    return String.join(headerSecurityConfig.serviceDelimiter, matchedElements);
+  }
 }

webapp/src/main/java/com/box/l10n/mojito/service/security/user/UserService.java

@@ -409,9 +409,8 @@ public Page<User> findByUsernameOrName(String username, String search, Pageable
   /**
    * Gets a service by name and if it doesn't exist create a created user.
    *
-   * <p>All admin service accounts should already be created beforehand.
-   * Get the exact service account or any service account which is the
-   * ancestor service to the account
+   * <p>All admin service accounts should already be created beforehand. Get the exact service
+   * account or any service account which is the ancestor service to the account
    *
    * @param serviceName
    * @return
@@ -423,7 +422,8 @@ public User getServiceAccountUser(String serviceName) {
     }
 
     Optional<List<User>> users =
-        userRepository.findServicesByServiceNameAndPrefix(serviceName, headerSecurityConfig.getServicePrefix());
+        userRepository.findServicesByServiceNameAndPrefix(
+            serviceName, headerSecurityConfig.getServicePrefix());
     if (users.isEmpty()) {
       logger.debug("No matching services found as per DB query");
       sendPagerDutyNotification(serviceName);
@@ -433,8 +433,7 @@ public User getServiceAccountUser(String serviceName) {
       return null;
     }
 
-    User matchingUser =
-        serviceDisambiguator.findServiceWithCommonAncestor(users.get(), serviceName);
+    User matchingUser = serviceDisambiguator.getServiceWithCommonAncestor(users.get(), serviceName);
     if (matchingUser == null) {
       logger.debug("No matching services found as per ServiceDisambiguator");
       sendPagerDutyNotification(serviceName);

webapp/src/test/java/com/box/l10n/mojito/security/ServiceDisambiguatorTest.java

@@ -22,16 +22,16 @@ public ServiceDisambiguatorTest() {
   @Test
   public void testFindServiceWithShortestSharedAncestor_EdgeCases() {
     User sharedAncestor =
-        serviceDisambiguator.findServiceWithCommonAncestor(
+        serviceDisambiguator.getServiceWithCommonAncestor(
             new ArrayList<>(), "spiffe://test.com/container/service");
     assertNull(sharedAncestor);
-    sharedAncestor = serviceDisambiguator.findServiceWithCommonAncestor(new ArrayList<>(), "");
+    sharedAncestor = serviceDisambiguator.getServiceWithCommonAncestor(new ArrayList<>(), "");
     assertNull(sharedAncestor);
-    sharedAncestor = serviceDisambiguator.findServiceWithCommonAncestor(new ArrayList<>(), null);
+    sharedAncestor = serviceDisambiguator.getServiceWithCommonAncestor(new ArrayList<>(), null);
     assertNull(sharedAncestor);
-    sharedAncestor = serviceDisambiguator.findServiceWithCommonAncestor(null, "");
+    sharedAncestor = serviceDisambiguator.getServiceWithCommonAncestor(null, "");
     assertNull(sharedAncestor);
-    sharedAncestor = serviceDisambiguator.findServiceWithCommonAncestor(null, "test");
+    sharedAncestor = serviceDisambiguator.getServiceWithCommonAncestor(null, "test");
     assertNull(sharedAncestor);
   }
 
@@ -47,11 +47,11 @@ public void testFindServiceWithShortestSharedAncestor_ExactMatch() throws Except
                 "spiffe://test.com/container1/imageService",
                 "spiffe://test.com/container1/userService"));
     User result =
-        serviceDisambiguator.findServiceWithCommonAncestor(
+        serviceDisambiguator.getServiceWithCommonAncestor(
             services, "spiffe://test.com/container1/tagService");
     assertEquals(result, services.get(3));
     result =
-        serviceDisambiguator.findServiceWithCommonAncestor(
+        serviceDisambiguator.getServiceWithCommonAncestor(
             services, "spiffe://test.com/container1/imageService");
     assertEquals(result, services.get(4));
   }
@@ -65,14 +65,14 @@ public void testFindServiceWithShortestSharedAncestor_SpecificExamples() {
                 "spiffe://test.com/infra/jenkins/agent2",
                 "spiffe://test.com/infra/jenkins"));
     User result =
-        serviceDisambiguator.findServiceWithCommonAncestor(
+        serviceDisambiguator.getServiceWithCommonAncestor(
             services, "spiffe://test.com/infra/jenkins/agent1");
     assertEquals(result, services.getFirst());
     result =
-        serviceDisambiguator.findServiceWithCommonAncestor(
+        serviceDisambiguator.getServiceWithCommonAncestor(
             services, "spiffe://test.com/infra/jenkins/agent3");
     assertEquals(result, services.get(2));
-    result = serviceDisambiguator.findServiceWithCommonAncestor(services, "spiffe://test.com");
+    result = serviceDisambiguator.getServiceWithCommonAncestor(services, "spiffe://test.com");
     assertNull(result);
   }