Fix review findings

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);

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();

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";
+    }
+  }
+
 }

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));
     }

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