From d1de7bf214d6b4e5b226bf4db671e4ab3b730ba5 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Wed, 6 Oct 2021 14:34:10 +0200 Subject: [PATCH] Clear external group cache on explicit logout or user deletion (#1819) Clears the external group cache whenever a user gets logged out by the logout rest method or the user gets deleted. Co-authored-by: Eduard Heimbuch --- .../changelog/clear_external_group_cache.yaml | 2 + .../java/sonia/scm/security/LogoutEvent.java | 41 +++++++++++++++++++ .../v2/resources/AuthenticationResource.java | 17 ++++++-- .../scm/group/DefaultGroupCollector.java | 17 ++++++++ .../sonia/scm/security/SecurityRequests.java | 8 +++- .../resources/AuthenticationResourceTest.java | 35 ++++++++++++++-- .../scm/group/DefaultGroupCollectorTest.java | 27 +++++++++++- .../scm/security/SecurityRequestsTest.java | 38 ++++++++++------- 8 files changed, 160 insertions(+), 25 deletions(-) create mode 100644 gradle/changelog/clear_external_group_cache.yaml create mode 100644 scm-core/src/main/java/sonia/scm/security/LogoutEvent.java diff --git a/gradle/changelog/clear_external_group_cache.yaml b/gradle/changelog/clear_external_group_cache.yaml new file mode 100644 index 0000000000..b3c0c40920 --- /dev/null +++ b/gradle/changelog/clear_external_group_cache.yaml @@ -0,0 +1,2 @@ +- type: Changed + description: Clear external group cache on explicit user logout ([#1819](https://github.com/scm-manager/scm-manager/pull/1819)) diff --git a/scm-core/src/main/java/sonia/scm/security/LogoutEvent.java b/scm-core/src/main/java/sonia/scm/security/LogoutEvent.java new file mode 100644 index 0000000000..c9dfeff451 --- /dev/null +++ b/scm-core/src/main/java/sonia/scm/security/LogoutEvent.java @@ -0,0 +1,41 @@ +/* + * 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. + */ + +package sonia.scm.security; + +import lombok.Value; +import sonia.scm.event.Event; + +/** + * Event is fired whenever a user explicitly logs out. + * The event is not fired if a session expires or a token is blacklisted, + * the event is only fired if the user calls the rest method for the logout. + * + * @since 2.24.0 + */ +@Value +@Event +public class LogoutEvent { + String primaryPrincipal; +} diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java index 3c4a92cdd1..b8ba0d1921 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java @@ -40,15 +40,18 @@ import io.swagger.v3.oas.annotations.security.SecuritySchemes; import io.swagger.v3.oas.annotations.tags.Tag; import org.apache.shiro.SecurityUtils; import org.apache.shiro.authc.AuthenticationException; +import org.apache.shiro.subject.PrincipalCollection; import org.apache.shiro.subject.Subject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import sonia.scm.event.ScmEventBus; import sonia.scm.metrics.AuthenticationMetrics; import sonia.scm.security.AccessToken; import sonia.scm.security.AccessTokenBuilder; import sonia.scm.security.AccessTokenBuilderFactory; import sonia.scm.security.AccessTokenCookieIssuer; import sonia.scm.security.AllowAnonymousAccess; +import sonia.scm.security.LogoutEvent; import sonia.scm.security.Scope; import sonia.scm.security.Tokens; import sonia.scm.web.VndMediaType; @@ -86,7 +89,6 @@ import java.util.Optional; @Tag(name = "Authentication", description = "Authentication related endpoints") }) @Path(AuthenticationResource.PATH) -@AllowAnonymousAccess public class AuthenticationResource { private static final Logger LOG = LoggerFactory.getLogger(AuthenticationResource.class); @@ -99,17 +101,19 @@ public class AuthenticationResource { private final Counter loginAttemptsCounter; private final Counter loginFailedCounter; private final Counter logoutCounter; + private final ScmEventBus eventBus; @Inject(optional = true) private LogoutRedirection logoutRedirection; @Inject - public AuthenticationResource(AccessTokenBuilderFactory tokenBuilderFactory, AccessTokenCookieIssuer cookieIssuer, MeterRegistry meterRegistry) { + public AuthenticationResource(AccessTokenBuilderFactory tokenBuilderFactory, AccessTokenCookieIssuer cookieIssuer, MeterRegistry meterRegistry, ScmEventBus eventBus) { this.tokenBuilderFactory = tokenBuilderFactory; this.cookieIssuer = cookieIssuer; this.loginAttemptsCounter = AuthenticationMetrics.loginAttempts(meterRegistry, AUTH_METRIC_TYPE); this.loginFailedCounter = AuthenticationMetrics.loginFailed(meterRegistry, AUTH_METRIC_TYPE); this.logoutCounter = AuthenticationMetrics.logout(meterRegistry, AUTH_METRIC_TYPE); + this.eventBus = eventBus; } @POST @@ -134,6 +138,7 @@ public class AuthenticationResource { schema = @Schema(implementation = ErrorDto.class) ) ) + @AllowAnonymousAccess public Response authenticateViaForm( @Context HttpServletRequest request, @Context HttpServletResponse response, @@ -174,6 +179,7 @@ public class AuthenticationResource { schema = @Schema(implementation = ErrorDto.class) ) ) + @AllowAnonymousAccess public Response authenticateViaJSONBody( @Context HttpServletRequest request, @Context HttpServletResponse response, @@ -238,7 +244,12 @@ public class AuthenticationResource { public Response logout(@Context HttpServletRequest request, @Context HttpServletResponse response) { logoutCounter.increment(); - SecurityUtils.getSubject().logout(); + Subject subject = SecurityUtils.getSubject(); + String primaryPrincipal = subject.getPrincipals().getPrimaryPrincipal().toString(); + subject.logout(); + + eventBus.post(new LogoutEvent(primaryPrincipal)); + // remove authentication cookie cookieIssuer.invalidate(request, response); 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 ccf659257c..91e6b58944 100644 --- a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java +++ b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java @@ -25,12 +25,16 @@ package sonia.scm.group; import com.cronutils.utils.VisibleForTesting; +import com.github.legman.Subscribe; import com.google.common.collect.ImmutableSet; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import sonia.scm.HandlerEventType; import sonia.scm.cache.Cache; import sonia.scm.cache.CacheManager; import sonia.scm.security.Authentications; +import sonia.scm.security.LogoutEvent; +import sonia.scm.user.UserEvent; import javax.inject.Inject; import javax.inject.Singleton; @@ -78,6 +82,19 @@ public class DefaultGroupCollector implements GroupCollector { return groups; } + @Subscribe(async = false) + public void clearCacheOnLogOut(LogoutEvent event) { + String principal = event.getPrimaryPrincipal(); + cache.remove(principal); + } + + @Subscribe(async = false) + public void clearCacheOnUserDeletion(UserEvent event) { + if (event.getEventType().equals(HandlerEventType.DELETE)) { + cache.remove(event.getItem().getName()); + } + } + private void appendInternalGroups(String principal, ImmutableSet.Builder builder) { for (Group group : groupDAO.getAll()) { if (group.isMember(principal)) { diff --git a/scm-webapp/src/main/java/sonia/scm/security/SecurityRequests.java b/scm-webapp/src/main/java/sonia/scm/security/SecurityRequests.java index 7e52d9f230..319218c4d1 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/SecurityRequests.java +++ b/scm-webapp/src/main/java/sonia/scm/security/SecurityRequests.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 javax.servlet.http.HttpServletRequest; @@ -41,7 +41,11 @@ public final class SecurityRequests { public static boolean isAuthenticationRequest(HttpServletRequest request) { String uri = request.getRequestURI().substring(request.getContextPath().length()); - return isAuthenticationRequest(uri); + return isAuthenticationRequest(uri) && !isLogoutMethod(request); + } + + private static boolean isLogoutMethod(HttpServletRequest request) { + return "DELETE".equals(request.getMethod()); } public static boolean isAuthenticationRequest(String uri) { diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/AuthenticationResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/AuthenticationResourceTest.java index 059242beb5..cb49007704 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/AuthenticationResourceTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/AuthenticationResourceTest.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.api.v2.resources; import com.github.sdorra.shiro.ShiroRule; @@ -35,14 +35,17 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import sonia.scm.config.ScmConfiguration; +import sonia.scm.event.ScmEventBus; import sonia.scm.security.AccessToken; import sonia.scm.security.AccessTokenBuilder; import sonia.scm.security.AccessTokenBuilderFactory; import sonia.scm.security.AccessTokenCookieIssuer; import sonia.scm.security.DefaultAccessTokenCookieIssuer; +import sonia.scm.security.LogoutEvent; import sonia.scm.web.RestDispatcher; import javax.servlet.http.HttpServletRequest; @@ -52,14 +55,14 @@ import java.io.UnsupportedEncodingException; import java.net.URISyntaxException; import java.util.List; import java.util.Optional; -import java.util.stream.Stream; import static java.net.URI.create; import static java.util.Optional.of; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @SubjectAware( @@ -79,6 +82,9 @@ public class AuthenticationResourceTest { @Mock private AccessTokenBuilder accessTokenBuilder; + @Mock + private ScmEventBus eventBus; + private MeterRegistry meterRegistry; private final AccessTokenCookieIssuer cookieIssuer = new DefaultAccessTokenCookieIssuer(mock(ScmConfiguration.class)); @@ -142,7 +148,7 @@ public class AuthenticationResourceTest { @Before public void prepareEnvironment() { meterRegistry = new SimpleMeterRegistry(); - authenticationResource = new AuthenticationResource(accessTokenBuilderFactory, cookieIssuer, meterRegistry); + authenticationResource = new AuthenticationResource(accessTokenBuilderFactory, cookieIssuer, meterRegistry, eventBus); dispatcher.addSingletonResource(authenticationResource); AccessToken accessToken = mock(AccessToken.class); @@ -248,9 +254,29 @@ public class AuthenticationResourceTest { Optional logoutMeter = meters.stream().filter(m -> m.getId().getName().equals("scm.auth.logout")).findFirst(); assertThat(logoutMeter).isPresent(); assertThat(logoutMeter.get().measure().iterator().next().getValue()).isEqualTo(1); + + verify(eventBus).post(any(LogoutEvent.class)); } @Test + @SubjectAware(username = "trillian", password = "secret") + public void shouldFireLogoutEvent() throws URISyntaxException { + MockHttpRequest request = MockHttpRequest.delete("/" + AuthenticationResource.PATH + "/access_token"); + + dispatcher.invoke(request, response); + assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus()); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Object.class); + verify(eventBus).post(captor.capture()); + + Object event = captor.getValue(); + assertThat(event).isInstanceOfSatisfying(LogoutEvent.class, logoutEvent -> { + assertThat(logoutEvent.getPrimaryPrincipal()).isEqualTo("trillian"); + }); + } + + @Test + @SubjectAware(username = "trillian", password = "secret") public void shouldHandleLogoutRedirection() throws URISyntaxException, UnsupportedEncodingException { authenticationResource.setLogoutRedirection(() -> of(create("http://example.com/cas/logout"))); @@ -263,6 +289,7 @@ public class AuthenticationResourceTest { } @Test + @SubjectAware(username = "trillian", password = "secret") public void shouldHandleDisabledLogoutRedirection() throws URISyntaxException { authenticationResource.setLogoutRedirection(Optional::empty); 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 ac5feefefd..c23cce618a 100644 --- a/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java @@ -32,10 +32,12 @@ 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.HandlerEventType; import sonia.scm.cache.MapCache; import sonia.scm.cache.MapCacheManager; -import sonia.scm.security.Authentications; +import sonia.scm.security.LogoutEvent; +import sonia.scm.user.User; +import sonia.scm.user.UserEvent; import java.util.HashSet; import java.util.List; @@ -98,6 +100,27 @@ class DefaultGroupCollectorTest { verify(groupResolver, never()).resolve("trillian"); } + @Test + void shouldClearCacheOnLogout() { + MapCache> cache = mapCacheManager.getCache(DefaultGroupCollector.CACHE_NAME); + cache.put("trillian", ImmutableSet.of("awesome", "incredible")); + + collector.clearCacheOnLogOut(new LogoutEvent("trillian")); + + assertThat(cache.get("trillian")).isNull(); + } + + + @Test + void shouldClearCacheOnUserDeletion() { + MapCache> cache = mapCacheManager.getCache(DefaultGroupCollector.CACHE_NAME); + cache.put("trillian", ImmutableSet.of("awesome", "incredible")); + + collector.clearCacheOnUserDeletion(new UserEvent(HandlerEventType.DELETE, new User("trillian"))); + + assertThat(cache.get("trillian")).isNull(); + } + @Test void shouldNotCallResolverForAnonymous() { groupResolvers.add(groupResolver); diff --git a/scm-webapp/src/test/java/sonia/scm/security/SecurityRequestsTest.java b/scm-webapp/src/test/java/sonia/scm/security/SecurityRequestsTest.java index 3d2ce9618a..517d22f2ab 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/SecurityRequestsTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/SecurityRequestsTest.java @@ -21,41 +21,51 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.security; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.junit.jupiter.MockitoExtension; import javax.servlet.http.HttpServletRequest; -import static org.junit.Assert.*; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.when; /** * Created by masuewer on 04.07.18. */ -@RunWith(MockitoJUnitRunner.class) -public class SecurityRequestsTest { +@ExtendWith(MockitoExtension.class) +class SecurityRequestsTest { @Mock private HttpServletRequest request; @Test - public void testIsAuthenticationRequestWithContextPath() { + void shouldReturnTrueWithContextPath() { when(request.getRequestURI()).thenReturn("/scm/api/auth/access_token"); when(request.getContextPath()).thenReturn("/scm"); - assertTrue(SecurityRequests.isAuthenticationRequest(request)); + assertThat(SecurityRequests.isAuthenticationRequest(request)).isTrue(); } @Test - public void testIsAuthenticationRequest() throws Exception { - assertTrue(SecurityRequests.isAuthenticationRequest("/api/auth/access_token")); - assertTrue(SecurityRequests.isAuthenticationRequest("/api/v2/auth/access_token")); - assertFalse(SecurityRequests.isAuthenticationRequest("/api/repositories")); - assertFalse(SecurityRequests.isAuthenticationRequest("/api/v2/repositories")); + void shouldDetectAuthenticationResource() { + assertThat(SecurityRequests.isAuthenticationRequest("/api/auth/access_token")).isTrue(); + assertThat(SecurityRequests.isAuthenticationRequest("/api/v2/auth/access_token")).isTrue(); + assertThat(SecurityRequests.isAuthenticationRequest("/api/repositories")).isFalse(); + assertThat(SecurityRequests.isAuthenticationRequest("/api/v2/repositories")).isFalse(); } + + @Test + void shouldReturnFalseForLogout() { + when(request.getRequestURI()).thenReturn("/scm/api/auth/access_token"); + when(request.getContextPath()).thenReturn("/scm"); + when(request.getMethod()).thenReturn("DELETE"); + + assertThat(SecurityRequests.isAuthenticationRequest(request)).isFalse(); + } + }