From 1a2dabeb664034dc02a2f2f1f84382564ab3b11e Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 15 Feb 2021 08:45:47 +0100 Subject: [PATCH] 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 | 2 + .../src/main/java/sonia/scm/SCMContext.java | 10 +++-- .../sonia/scm/security/Authentications.java | 27 +++++++++++- .../scm/group/DefaultGroupCollector.java | 13 +++--- .../security/AdministrationContextRealm.java | 3 +- .../DefaultAdministrationContext.java | 26 +++++------- .../sonia/scm/web/security/system-account.xml | 42 ------------------- .../scm/group/DefaultGroupCollectorTest.java | 28 ++++++++++++- .../DefaultAdministrationContextTest.java | 8 ++-- 9 files changed, 83 insertions(+), 76 deletions(-) create mode 100644 gradle/changelog/fix_group_collector.yaml delete mode 100644 scm-webapp/src/main/resources/sonia/scm/web/security/system-account.xml diff --git a/gradle/changelog/fix_group_collector.yaml b/gradle/changelog/fix_group_collector.yaml new file mode 100644 index 0000000000..d553c59e83 --- /dev/null +++ b/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)) diff --git a/scm-core/src/main/java/sonia/scm/SCMContext.java b/scm-core/src/main/java/sonia/scm/SCMContext.java index dec4997f48..1a383bc64e 100644 --- a/scm-core/src/main/java/sonia/scm/SCMContext.java +++ b/scm-core/src/main/java/sonia/scm/SCMContext.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm; //~--- non-JDK imports -------------------------------------------------------- @@ -48,9 +48,11 @@ public final class SCMContext * the anonymous user * @since 1.21 */ - public static final User ANONYMOUS = new User(USER_ANONYMOUS, - "SCM Anonymous", - "scm-anonymous@scm-manager.org"); + public static final User ANONYMOUS = new User( + USER_ANONYMOUS, + "SCM Anonymous", + "scm-anonymous@scm-manager.org" + ); /** Singleton instance of {@link SCMContextProvider} */ private static volatile SCMContextProvider provider; diff --git a/scm-core/src/main/java/sonia/scm/security/Authentications.java b/scm-core/src/main/java/sonia/scm/security/Authentications.java index d99643ee60..349ab7cfa1 100644 --- a/scm-core/src/main/java/sonia/scm/security/Authentications.java +++ b/scm-core/src/main/java/sonia/scm/security/Authentications.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.security; import org.apache.shiro.SecurityUtils; @@ -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); } } diff --git a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java index 2ad7cc7fc5..ccf659257c 100644 --- a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java +++ b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.group; import com.cronutils.utils.VisibleForTesting; @@ -63,13 +63,16 @@ public class DefaultGroupCollector implements GroupCollector { public Set collect(String principal) { ImmutableSet.Builder 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); } - builder.addAll(resolveExternalGroups(principal)); - appendInternalGroups(principal, builder); - Set groups = builder.build(); LOG.debug("collected following groups for principal {}: {}", principal, groups); return groups; diff --git a/scm-webapp/src/main/java/sonia/scm/web/security/AdministrationContextRealm.java b/scm-webapp/src/main/java/sonia/scm/web/security/AdministrationContextRealm.java index 8eed9d7165..c07f64c824 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/security/AdministrationContextRealm.java +++ b/scm-webapp/src/main/java/sonia/scm/web/security/AdministrationContextRealm.java @@ -21,11 +21,10 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + 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; diff --git a/scm-webapp/src/main/java/sonia/scm/web/security/DefaultAdministrationContext.java b/scm-webapp/src/main/java/sonia/scm/web/security/DefaultAdministrationContext.java index 97f9b2386f..864ecbe613 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/security/DefaultAdministrationContext.java +++ b/scm-webapp/src/main/java/sonia/scm/web/security/DefaultAdministrationContext.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.web.security; //~--- non-JDK imports -------------------------------------------------------- @@ -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 = - "/sonia/scm/web/security/system-account.xml"; + private static final User SYSTEM_ACCOUNT = new User( + 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); - - if (url == null) - { - throw new RuntimeException("could not find resource for system account"); - } - - User adminUser = JAXB.unmarshal(url, User.class); - - principalCollection = createAdminCollection(adminUser); + principalCollection = createAdminCollection(SYSTEM_ACCOUNT); } //~--- methods -------------------------------------------------------------- diff --git a/scm-webapp/src/main/resources/sonia/scm/web/security/system-account.xml b/scm-webapp/src/main/resources/sonia/scm/web/security/system-account.xml deleted file mode 100644 index 555e865559..0000000000 --- a/scm-webapp/src/main/resources/sonia/scm/web/security/system-account.xml +++ /dev/null @@ -1,42 +0,0 @@ - - - - - - scmsystem - SCM System - scm-sytem@scm-manager.com - * - true - xml - \ No newline at end of file diff --git a/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java b/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java index af9eea8d33..ac5feefefd 100644 --- a/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.group; import com.google.common.collect.ImmutableSet; @@ -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 groups = collector.collect(PRINCIPAL_ANONYMOUS); + verify(groupResolver, never()).resolve(PRINCIPAL_ANONYMOUS); + } + + @Test + void shouldNotCallResolverForSystemAccount() { + groupResolvers.add(groupResolver); + Set groups = collector.collect(PRINCIPAL_SYSTEM); + verify(groupResolver, never()).resolve(PRINCIPAL_SYSTEM); + } + + @Test + void shouldNotResolveInternalGroupsForSystemAccount() { + Set groups = collector.collect(PRINCIPAL_SYSTEM); + verify(groupDAO, never()).getAll(); + } + + + @Nested class WithGroupsFromDao { diff --git a/scm-webapp/src/test/java/sonia/scm/web/security/DefaultAdministrationContextTest.java b/scm-webapp/src/test/java/sonia/scm/web/security/DefaultAdministrationContextTest.java index 6a5dbc470e..b6a946e61e 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/security/DefaultAdministrationContextTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/security/DefaultAdministrationContextTest.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.web.security; import com.google.inject.Guice; @@ -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 {