Do not resolve external groups for system accounts (#1541)

This change modifies the behaviour of the DefaultGroupCollector.
The collector does not longer resolve external groups for the anonymous user and it does not resolve internal nor external groups for the account which is used by the AdministrationContext.
This should reduce the requests which are send to external systems like ldap servers.

gradle/changelog/fix_group_collector.yaml

@@ -0,0 +1,2 @@
+- type: fixed
+  description: Do not resolve external groups for system accounts ([#1541](https://github.com/scm-manager/scm-manager/pull/1541))

scm-core/src/main/java/sonia/scm/SCMContext.java

@@ -48,9 +48,11 @@ public final class SCMContext
    * the anonymous user
    * @since 1.21
    */
-  public static final User ANONYMOUS = new User(USER_ANONYMOUS,
+  public static final User ANONYMOUS = new User(
+    USER_ANONYMOUS,
     "SCM Anonymous",
-                                         "scm-anonymous@scm-manager.org");
+    "scm-anonymous@scm-manager.org"
+  );
 
   /** Singleton instance of {@link SCMContextProvider} */
   private static volatile SCMContextProvider provider;

scm-core/src/main/java/sonia/scm/security/Authentications.java

@@ -29,6 +29,18 @@ import sonia.scm.SCMContext;
 
 public class Authentications {
 
+  /**
+   * Username of the system account.
+   * @since 2.14.0
+   */
+  public static final String PRINCIPAL_SYSTEM = "_scmsystem";
+
+  /**
+   * Username of the anonymous account.
+   * @since 2.14.0
+   */
+  public static final String PRINCIPAL_ANONYMOUS = SCMContext.USER_ANONYMOUS;
+
   private Authentications() {}
 
   public static boolean isAuthenticatedSubjectAnonymous() {
@@ -36,6 +48,17 @@ public class Authentications {
   }
 
   public static boolean isSubjectAnonymous(String principal) {
-    return SCMContext.USER_ANONYMOUS.equals(principal);
+    return PRINCIPAL_ANONYMOUS.equals(principal);
+  }
+
+  /**
+   * Returns true if the given principal is equal to the one from the system account.
+   *
+   * @param principal principal
+   * @return {@code true}
+   * @since 2.14.0
+   */
+  public static boolean isSubjectSystemAccount(String principal) {
+    return PRINCIPAL_SYSTEM.equals(principal);
   }
 }

scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java

@@ -63,12 +63,15 @@ public class DefaultGroupCollector implements GroupCollector {
   public Set<String> collect(String principal) {
     ImmutableSet.Builder<String> builder = ImmutableSet.builder();
 
-    if (!Authentications.isSubjectAnonymous(principal)) {
+    if (Authentications.isSubjectAnonymous(principal)) {
+      appendInternalGroups(principal, builder);
+    } else if (Authentications.isSubjectSystemAccount(principal)) {
+      builder.add(AUTHENTICATED);
+    } else {
       builder.add(AUTHENTICATED);
-    }
-
       builder.addAll(resolveExternalGroups(principal));
       appendInternalGroups(principal, builder);
+    }
 
     Set<String> groups = builder.build();
     LOG.debug("collected following groups for principal {}: {}", principal, groups);

scm-webapp/src/main/java/sonia/scm/web/security/AdministrationContextRealm.java

@@ -25,7 +25,6 @@
 package sonia.scm.web.security;
 
 import com.google.common.collect.Sets;
-import org.apache.shiro.authc.AuthenticationException;
 import org.apache.shiro.authc.AuthenticationInfo;
 import org.apache.shiro.authc.AuthenticationToken;
 import org.apache.shiro.authz.AuthorizationInfo;

scm-webapp/src/main/java/sonia/scm/web/security/DefaultAdministrationContext.java

@@ -39,13 +39,11 @@ import org.apache.shiro.util.ThreadState;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import sonia.scm.SCMContext;
+import sonia.scm.security.Authentications;
 import sonia.scm.security.Role;
 import sonia.scm.user.User;
 import sonia.scm.util.AssertUtil;
 
-import javax.xml.bind.JAXB;
-import java.net.URL;
-
 //~--- JDK imports ------------------------------------------------------------
 
 /**
@@ -57,8 +55,13 @@ public class DefaultAdministrationContext implements AdministrationContext
 {
 
   /** Field description */
-  public static final String SYSTEM_ACCOUNT =
+  private static final User SYSTEM_ACCOUNT = new User(
-    "/sonia/scm/web/security/system-account.xml";
+    Authentications.PRINCIPAL_SYSTEM,
+    "SCM-Manager System Account",
+    null
+  );
+
+
 
   /** Field description */
   static final String REALM = "AdminRealm";
@@ -83,16 +86,7 @@ public class DefaultAdministrationContext implements AdministrationContext
     this.injector = injector;
     this.securityManager = securityManager;
 
-    URL url = DefaultAdministrationContext.class.getResource(SYSTEM_ACCOUNT);
+    principalCollection = createAdminCollection(SYSTEM_ACCOUNT);
-
-    if (url == null)
-    {
-      throw new RuntimeException("could not find resource for system account");
-    }
-
-    User adminUser = JAXB.unmarshal(url, User.class);
-
-    principalCollection = createAdminCollection(adminUser);
   }
 
   //~--- methods --------------------------------------------------------------

scm-webapp/src/main/resources/sonia/scm/web/security/system-account.xml

@@ -1,42 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>
-<!--
-
-    MIT License
-
-    Copyright (c) 2020-present Cloudogu GmbH and Contributors
-
-    Permission is hereby granted, free of charge, to any person obtaining a copy
-    of this software and associated documentation files (the "Software"), to deal
-    in the Software without restriction, including without limitation the rights
-    to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-    copies of the Software, and to permit persons to whom the Software is
-    furnished to do so, subject to the following conditions:
-
-    The above copyright notice and this permission notice shall be included in all
-    copies or substantial portions of the Software.
-
-    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-    AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-    OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
-    SOFTWARE.
-
--->
-<!--
-    Document   : system-user.xml
-    Created on : July 30, 2011, 10:49 AM
-    Author     : sdorra
-    Description:
-        Purpose of the document follows.
--->
-
-<users>
-  <name>scmsystem</name>
-  <displayName>SCM System</displayName>
-  <mail>scm-sytem@scm-manager.com</mail>
-  <password>*</password>
-  <admin>true</admin>
-  <type>xml</type>
-</users>

scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java

@@ -32,8 +32,10 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import sonia.scm.SCMContext;
 import sonia.scm.cache.MapCache;
 import sonia.scm.cache.MapCacheManager;
+import sonia.scm.security.Authentications;
 
 import java.util.HashSet;
 import java.util.List;
@@ -43,6 +45,8 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
+import static sonia.scm.security.Authentications.PRINCIPAL_ANONYMOUS;
+import static sonia.scm.security.Authentications.PRINCIPAL_SYSTEM;
 
 @ExtendWith(MockitoExtension.class)
 class DefaultGroupCollectorTest {
@@ -94,6 +98,28 @@ class DefaultGroupCollectorTest {
     verify(groupResolver, never()).resolve("trillian");
   }
 
+  @Test
+  void shouldNotCallResolverForAnonymous() {
+    groupResolvers.add(groupResolver);
+    Set<String> groups = collector.collect(PRINCIPAL_ANONYMOUS);
+    verify(groupResolver, never()).resolve(PRINCIPAL_ANONYMOUS);
+  }
+
+  @Test
+  void shouldNotCallResolverForSystemAccount() {
+    groupResolvers.add(groupResolver);
+    Set<String> groups = collector.collect(PRINCIPAL_SYSTEM);
+    verify(groupResolver, never()).resolve(PRINCIPAL_SYSTEM);
+  }
+
+  @Test
+  void shouldNotResolveInternalGroupsForSystemAccount() {
+    Set<String> groups = collector.collect(PRINCIPAL_SYSTEM);
+    verify(groupDAO, never()).getAll();
+  }
+
+
+
   @Nested
   class WithGroupsFromDao {
 

scm-webapp/src/test/java/sonia/scm/web/security/DefaultAdministrationContextTest.java

@@ -36,9 +36,9 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import sonia.scm.security.Authentications;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.Mockito.when;
 
 @ExtendWith(MockitoExtension.class)
@@ -61,7 +61,7 @@ class DefaultAdministrationContextTest {
   void shouldBindSubject() {
     context.runAsAdmin(() -> {
       Subject adminSubject = SecurityUtils.getSubject();
-      assertThat(adminSubject.getPrincipal()).isEqualTo("scmsystem");
+      assertThat(adminSubject.getPrincipal()).isEqualTo(Authentications.PRINCIPAL_SYSTEM);
     });
   }
 
@@ -72,7 +72,7 @@ class DefaultAdministrationContextTest {
 
       context.runAsAdmin(() -> {
         Subject adminSubject = SecurityUtils.getSubject();
-        assertThat(adminSubject.getPrincipal()).isEqualTo("scmsystem");
+        assertThat(adminSubject.getPrincipal()).isEqualTo(Authentications.PRINCIPAL_SYSTEM);
       });
 
     } finally {