From 29faa5ec6cfc41f72ca0aea58a3871f762ecb849 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Thu, 19 Nov 2020 10:21:17 +0100 Subject: [PATCH] Fix review findings --- .../repository/hooks/DefaultHookHandler.java | 4 + .../scm/repository/hooks/HookServer.java | 116 +++++++++------- .../hooks/DefaultHookHandlerTest.java | 28 ++++ .../scm/security/JwtAccessTokenBuilder.java | 13 +- .../security/JwtAccessTokenBuilderTest.java | 131 +++++++++++++----- 5 files changed, 199 insertions(+), 93 deletions(-) diff --git a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/DefaultHookHandler.java b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/DefaultHookHandler.java index 2b2e4132c2..2d035bf32f 100644 --- a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/DefaultHookHandler.java +++ b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/DefaultHookHandler.java @@ -32,6 +32,7 @@ import org.apache.shiro.authc.AuthenticationException; import org.apache.shiro.subject.Subject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import sonia.scm.ExceptionWithContext; import sonia.scm.NotFoundException; import sonia.scm.repository.RepositoryHookType; import sonia.scm.repository.api.HgHookMessage; @@ -110,6 +111,9 @@ class DefaultHookHandler implements HookHandler { } catch (NotFoundException ex) { LOG.warn("could not find repository with id {}", request.getRepositoryId(), ex); return error("repository not found"); + } catch (ExceptionWithContext ex) { + LOG.debug("scm exception on hook occurred", ex); + return error(context, ex); } catch (Exception ex) { LOG.warn("unknown error on hook occurred", ex); return error(context, ex); diff --git a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/HookServer.java b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/HookServer.java index ac27323018..90a15c6fb4 100644 --- a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/HookServer.java +++ b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/HookServer.java @@ -52,12 +52,63 @@ public class HookServer implements AutoCloseable { private ExecutorService acceptor; private ExecutorService workerPool; private ServerSocket serverSocket; + private SecurityManager securityManager; @Inject public HookServer(HookHandlerFactory handlerFactory) { this.handlerFactory = handlerFactory; } + public int start() throws IOException { + securityManager = SecurityUtils.getSecurityManager(); + + acceptor = createAcceptor(); + workerPool = createWorkerPool(); + serverSocket = createServerSocket(); + // set timeout to 2 min, to avoid blocking clients + serverSocket.setSoTimeout(2 * 60 * 1000); + + accept(); + + int port = serverSocket.getLocalPort(); + LOG.info("open hg hook server on port {}", port); + return port; + } + + private void accept() { + acceptor.submit(() -> { + while (!serverSocket.isClosed()) { + try { + LOG.trace("wait for next hook connection"); + Socket clientSocket = serverSocket.accept(); + LOG.trace("accept incoming hook client from {}", clientSocket.getInetAddress()); + HookHandler hookHandler = handlerFactory.create(clientSocket); + workerPool.submit(associateSecurityManager(hookHandler)); + } catch (IOException ex) { + LOG.debug("failed to accept socket, possible closed", ex); + } + } + LOG.warn("ServerSocket is closed"); + }); + } + + private Runnable associateSecurityManager(HookHandler hookHandler) { + return () -> { + ThreadContext.bind(securityManager); + try { + hookHandler.run(); + } finally { + ThreadContext.unbindSubject(); + ThreadContext.unbindSecurityManager(); + } + }; + } + + @Nonnull + private ServerSocket createServerSocket() throws IOException { + return new ServerSocket(0, 0, InetAddress.getLoopbackAddress()); + } + private ExecutorService createAcceptor() { return Executors.newSingleThreadExecutor( createThreadFactory("HgHookAcceptor") @@ -77,64 +128,23 @@ public class HookServer implements AutoCloseable { .build(); } - public int start() throws IOException { - acceptor = createAcceptor(); - workerPool = createWorkerPool(); - serverSocket = createServerSocket(); - // set timeout to 2 min, to avoid blocking clients - serverSocket.setSoTimeout(2 * 60 * 1000); - - accept(); - - int port = serverSocket.getLocalPort(); - LOG.info("open hg hook server on port {}", port); - return port; - } - - private void accept() { - SecurityManager securityManager = SecurityUtils.getSecurityManager(); - acceptor.submit(() -> { - while (!serverSocket.isClosed()) { - try { - LOG.trace("wait for next hook connection"); - Socket clientSocket = serverSocket.accept(); - LOG.trace("accept incoming hook client from {}", clientSocket.getInetAddress()); - HookHandler hookHandler = handlerFactory.create(clientSocket); - workerPool.submit(associateSecurityManager(securityManager, hookHandler)); - } catch (IOException ex) { - LOG.debug("failed to accept socket, possible closed", ex); - } - } - LOG.warn("ServerSocket is closed"); - }); - } - - private Runnable associateSecurityManager(SecurityManager securityManager, HookHandler hookHandler) { - return () -> { - ThreadContext.bind(securityManager); - try { - hookHandler.run(); - } finally { - ThreadContext.unbindSubject(); - ThreadContext.unbindSecurityManager(); - } - }; - } - - @Nonnull - private ServerSocket createServerSocket() throws IOException { - return new ServerSocket(0, 0, InetAddress.getLoopbackAddress()); - } - @Override - public void close() throws IOException { - if (serverSocket != null) { - serverSocket.close(); - } + public void close() { + closeSocket(); shutdown(acceptor); shutdown(workerPool); } + private void closeSocket() { + if (serverSocket != null) { + try { + serverSocket.close(); + } catch (IOException ex) { + LOG.warn("failed to close server socket", ex); + } + } + } + private void shutdown(ExecutorService acceptor) { if (acceptor != null) { acceptor.shutdown(); diff --git a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/DefaultHookHandlerTest.java b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/DefaultHookHandlerTest.java index 0a430727d6..fcdbc4792c 100644 --- a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/DefaultHookHandlerTest.java +++ b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/DefaultHookHandlerTest.java @@ -34,6 +34,7 @@ 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.ExceptionWithContext; import sonia.scm.NotFoundException; import sonia.scm.repository.RepositoryHookType; import sonia.scm.repository.api.HgHookMessage; @@ -47,6 +48,7 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.net.Socket; +import java.util.Collections; import java.util.List; import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; @@ -150,6 +152,20 @@ class DefaultHookHandlerTest { assertError(response, "Something went wrong"); } + @Test + void shouldHandleExceptionWithContext() throws IOException { + mockMessageProvider(); + + doThrow(new TestingException("Exception with Context")) + .when(hookEventFacade) + .handle("42"); + + DefaultHookHandler.Request request = createRequest(RepositoryHookType.POST_RECEIVE); + DefaultHookHandler.Response response = send(request); + + assertError(response, "Exception with Context"); + } + @Test void shouldSendMessagesOnException() throws IOException { HgHookMessageProvider messageProvider = new HgHookMessageProvider(); @@ -245,4 +261,16 @@ class DefaultHookHandlerTest { return Sockets.read(new ByteArrayInputStream(output.toByteArray()), DefaultHookHandler.Response.class); } + private static class TestingException extends ExceptionWithContext { + + private TestingException(String message) { + super(Collections.emptyList(), message); + } + + @Override + public String getCode() { + return "42"; + } + } + } diff --git a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java index c0d8521220..4e43f45e68 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java +++ b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java @@ -24,6 +24,7 @@ package sonia.scm.security; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.Maps; @@ -57,6 +58,12 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder { */ private static final Logger LOG = LoggerFactory.getLogger(JwtAccessTokenBuilder.class); + @VisibleForTesting + static final long DEFAULT_REFRESHABLE = 12L; + + @VisibleForTesting + static final TimeUnit DEFAULT_REFRESHABLE_UNIT = TimeUnit.HOURS; + private final KeyGenerator keyGenerator; private final SecureKeyResolver keyResolver; private final Clock clock; @@ -65,8 +72,8 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder { private String issuer; private long expiresIn = 1; private TimeUnit expiresInUnit = TimeUnit.HOURS; - private long refreshableFor = 12; - private TimeUnit refreshableForUnit = TimeUnit.HOURS; + private long refreshableFor = DEFAULT_REFRESHABLE; + private TimeUnit refreshableForUnit = DEFAULT_REFRESHABLE_UNIT; private Instant refreshExpiration; private String parentKeyId; private Scope scope = Scope.empty(); @@ -183,7 +190,7 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder { if (refreshableFor > 0) { long re = refreshableForUnit.toMillis(refreshableFor); - claims.put(JwtAccessToken.REFRESHABLE_UNTIL_CLAIM_KEY, new Date(now.toEpochMilli() + re).getTime()); + claims.put(JwtAccessToken.REFRESHABLE_UNTIL_CLAIM_KEY, Date.from(now.plusMillis(re))); } else if (refreshExpiration != null) { claims.put(JwtAccessToken.REFRESHABLE_UNTIL_CLAIM_KEY, Date.from(refreshExpiration)); } diff --git a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java b/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java index 3f36a4e341..66ec8a0a6e 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java @@ -39,15 +39,17 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import java.time.Clock; +import java.time.Instant; import java.util.Collection; +import java.util.Date; import java.util.Set; import java.util.concurrent.TimeUnit; import static java.util.Collections.singleton; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertThrows; -import static org.mockito.Mockito.anyString; -import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.*; import static sonia.scm.security.SecureKeyTestUtil.createSecureKey; /** @@ -85,52 +87,107 @@ class JwtAccessTokenBuilderTest { ThreadContext.unbindSubject(); } - /** - * Prepare mocks and set up object under test. - */ @BeforeEach - void setUpObjectUnderTest() { + void setUpDependencies() { lenient().when(keyGenerator.createKey()).thenReturn("42"); lenient().when(secureKeyResolver.getSecureKey(anyString())).thenReturn(createSecureKey()); enrichers = Sets.newHashSet(); factory = new JwtAccessTokenBuilderFactory(keyGenerator, secureKeyResolver, enrichers); } - /** - * Tests {@link JwtAccessTokenBuilder#build()}. - */ - @Test - void testBuild() { - JwtAccessToken token = factory.create().subject("dent") - .issuer("https://www.scm-manager.org") - .expiresIn(1, TimeUnit.MINUTES) - .custom("a", "b") - .scope(Scope.valueOf("repo:*")) - .build(); + @Nested + class SimpleTests { - // assert claims - assertClaims(token); + /** + * Prepare mocks and set up object under test. + */ + @BeforeEach + void setUpObjectUnderTest() { + factory = new JwtAccessTokenBuilderFactory(keyGenerator, secureKeyResolver, enrichers); + } + + /** + * Tests {@link JwtAccessTokenBuilder#build()}. + */ + @Test + void testBuild() { + JwtAccessToken token = factory.create().subject("dent") + .issuer("https://www.scm-manager.org") + .expiresIn(1, TimeUnit.MINUTES) + .custom("a", "b") + .scope(Scope.valueOf("repo:*")) + .build(); + + // assert claims + assertClaims(token); + + // reparse and assert again + String compact = token.compact(); + assertThat(compact).isNotEmpty(); + Claims claims = Jwts.parser() + .setSigningKey(secureKeyResolver.getSecureKey("dent").getBytes()) + .parseClaimsJws(compact) + .getBody(); + assertClaims(new JwtAccessToken(claims, compact)); + } + + private void assertClaims(JwtAccessToken token) { + assertThat(token.getId()).isNotEmpty(); + assertThat(token.getIssuedAt()).isNotNull(); + assertThat(token.getExpiration()).isNotNull(); + assertThat(token.getExpiration().getTime() > token.getIssuedAt().getTime()).isTrue(); + assertThat(token.getSubject()).isEqualTo("dent"); + assertThat(token.getIssuer()).isNotEmpty(); + assertThat(token.getIssuer()).get().isEqualTo("https://www.scm-manager.org"); + assertThat(token.getCustom("a")).get().isEqualTo("b"); + assertThat(token.getScope()).hasToString("[\"repo:*\"]"); + } - // reparse and assert again - String compact = token.compact(); - assertThat(compact).isNotEmpty(); - Claims claims = Jwts.parser() - .setSigningKey(secureKeyResolver.getSecureKey("dent").getBytes()) - .parseClaimsJws(compact) - .getBody(); - assertClaims(new JwtAccessToken(claims, compact)); } - private void assertClaims(JwtAccessToken token) { - assertThat(token.getId()).isNotEmpty(); - assertThat(token.getIssuedAt()).isNotNull(); - assertThat(token.getExpiration()).isNotNull(); - assertThat(token.getExpiration().getTime() > token.getIssuedAt().getTime()).isTrue(); - assertThat(token.getSubject()).isEqualTo("dent"); - assertThat(token.getIssuer()).isNotEmpty(); - assertThat(token.getIssuer()).get().isEqualTo("https://www.scm-manager.org"); - assertThat(token.getCustom("a")).get().isEqualTo("b"); - assertThat(token.getScope()).hasToString("[\"repo:*\"]"); + @Nested + class ClockTests { + + @Mock + private Clock clock; + + @BeforeEach + void setUpObjectUnderTest() { + factory = new JwtAccessTokenBuilderFactory(keyGenerator, secureKeyResolver, enrichers, clock); + } + + @Test + void shouldSetRefreshExpiration() { + Instant now = Instant.now(); + when(clock.instant()).thenReturn(now); + + JwtAccessToken token = factory.create() + .subject("dent") + .refreshableFor(2, TimeUnit.SECONDS) + .build(); + + assertThat(token.getRefreshExpiration()).isPresent(); + Date date = token.getRefreshExpiration().get(); + + assertThat(date).hasSameTimeAs(Date.from(now.plusSeconds(2L))); + } + + @Test + void shouldSetDefaultRefreshExpiration() { + Instant now = Instant.now(); + when(clock.instant()).thenReturn(now); + + JwtAccessToken token = factory.create() + .subject("dent") + .build(); + + assertThat(token.getRefreshExpiration()).isPresent(); + Date date = token.getRefreshExpiration().get(); + + long defaultRefresh = JwtAccessTokenBuilder.DEFAULT_REFRESHABLE_UNIT.toMillis(JwtAccessTokenBuilder.DEFAULT_REFRESHABLE); + assertThat(date).hasSameTimeAs(Date.from(now.plusMillis(defaultRefresh))); + } + } @Nested