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 {