Fix time computations

scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java

@@ -162,7 +162,7 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder {
 
     if (refreshableFor > 0) {
       long refreshExpiration = refreshableForUnit.toMillis(refreshableFor);
-      claims.put("scm-manager.refreshableUntil", new Date(now.getTime() + refreshExpiration).getTime() / 1000);
+      claims.put("scm-manager.refreshableUntil", new Date(now.getTime() + refreshExpiration).getTime());
     }
 
     if ( issuer != null ) {

scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenRefresher.java

@@ -1,6 +1,7 @@
 package sonia.scm.security;
 
-import java.time.Instant;
+import javax.inject.Inject;
+import java.time.Clock;
 import java.util.Date;
 import java.util.Map;
 import java.util.Optional;
@@ -10,10 +11,17 @@ public class JwtAccessTokenRefresher {
 
   private final JwtAccessTokenBuilderFactory builderFactory;
   private final JwtAccessTokenRefreshStrategy refreshStrategy;
+  private final Clock clock;
 
+  @Inject
   public JwtAccessTokenRefresher(JwtAccessTokenBuilderFactory builderFactory, JwtAccessTokenRefreshStrategy refreshStrategy) {
+    this(builderFactory, refreshStrategy, Clock.systemDefaultZone());
+  }
+
+  JwtAccessTokenRefresher(JwtAccessTokenBuilderFactory builderFactory, JwtAccessTokenRefreshStrategy refreshStrategy, Clock clock) {
     this.builderFactory = builderFactory;
     this.refreshStrategy = refreshStrategy;
+    this.clock = clock;
   }
 
   public Optional<JwtAccessToken> refresh(JwtAccessToken oldToken) {
@@ -31,7 +39,7 @@ public class JwtAccessTokenRefresher {
   }
 
   private boolean canBeRefreshed(JwtAccessToken oldToken) {
-    return tokenIsValid(oldToken) || tokenCanBeRefreshed(oldToken);
+    return tokenIsValid(oldToken) && tokenCanBeRefreshed(oldToken);
   }
 
   private boolean shouldBeRefreshed(JwtAccessToken oldToken) {
@@ -40,14 +48,14 @@ public class JwtAccessTokenRefresher {
 
   private boolean tokenCanBeRefreshed(JwtAccessToken oldToken) {
     Date refreshExpiration = oldToken.getRefreshExpiration();
-    return refreshExpiration != null && isBeforeNow(refreshExpiration);
+    return refreshExpiration != null && isAfterNow(refreshExpiration);
   }
 
   private boolean tokenIsValid(JwtAccessToken oldToken) {
-    return isBeforeNow(oldToken.getExpiration());
+    return isAfterNow(oldToken.getExpiration());
   }
 
-  private boolean isBeforeNow(Date expiration) {
-    return expiration.toInstant().isBefore(Instant.now());
+  private boolean isAfterNow(Date expiration) {
+    return expiration.toInstant().isAfter(clock.instant());
   }
 }

scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenRefresherTest.java

@@ -2,7 +2,6 @@ package sonia.scm.security;
 
 import com.github.sdorra.shiro.ShiroRule;
 import com.github.sdorra.shiro.SubjectAware;
-import org.assertj.core.api.Assertions;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -10,11 +9,15 @@ import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
 
+import java.time.Clock;
+import java.time.Instant;
 import java.util.Collections;
 import java.util.Optional;
 import java.util.Random;
-import java.util.concurrent.TimeUnit;
 
+import static java.time.Duration.ofMinutes;
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.when;
 
@@ -33,7 +36,9 @@ public class JwtAccessTokenRefresherTest {
   private SecureKeyResolver keyResolver;
   @Mock
   private JwtAccessTokenRefreshStrategy refreshStrategy;
-  private JwtAccessTokenBuilderFactory builderFactory;
+  @Mock
+  private Clock clock;
+
   private JwtAccessTokenRefresher refresher;
   private JwtAccessTokenBuilder tokenBuilder;
 
@@ -44,43 +49,74 @@ public class JwtAccessTokenRefresherTest {
     SecureKey secureKey = new SecureKey(bytes, System.currentTimeMillis());
     when(keyResolver.getSecureKey(any())).thenReturn(secureKey);
 
-    builderFactory = new JwtAccessTokenBuilderFactory(new DefaultKeyGenerator(), keyResolver, Collections.emptySet());
-    refresher = new JwtAccessTokenRefresher(builderFactory, refreshStrategy);
+    JwtAccessTokenBuilderFactory builderFactory = new JwtAccessTokenBuilderFactory(new DefaultKeyGenerator(), keyResolver, Collections.emptySet());
+    refresher = new JwtAccessTokenRefresher(builderFactory, refreshStrategy, clock);
     tokenBuilder = builderFactory.create();
+    when(clock.instant()).thenAnswer(invocationOnMock -> Instant.now());
+    when(refreshStrategy.shouldBeRefreshed(any())).thenReturn(true);
   }
 
   @Test
   public void shouldNotRefreshTokenWithDisabledRefresh() {
     JwtAccessToken oldToken = tokenBuilder
-      .refreshableFor(0, TimeUnit.MINUTES)
+      .refreshableFor(0, MINUTES)
       .build();
 
     Optional<JwtAccessToken> refreshedToken = refresher.refresh(oldToken);
 
-    Assertions.assertThat(refreshedToken).isEmpty();
+    assertThat(refreshedToken).isEmpty();
+  }
+
+  @Test
+  public void shouldNotRefreshTokenWhenTokenExpired() {
+    Instant oneMinuteAgo = Instant.now().plus(ofMinutes(2));
+    when(clock.instant()).thenReturn(oneMinuteAgo);
+    JwtAccessToken oldToken = tokenBuilder
+      .expiresIn(1, MINUTES)
+      .refreshableFor(5, MINUTES)
+      .build();
+
+    Optional<JwtAccessToken> refreshedToken = refresher.refresh(oldToken);
+
+    assertThat(refreshedToken).isEmpty();
+  }
+
+  @Test
+  public void shouldNotRefreshTokenWhenRefreshExpired() {
+    Instant oneMinuteAgo = Instant.now().plus(ofMinutes(2));
+    when(clock.instant()).thenReturn(oneMinuteAgo);
+    JwtAccessToken oldToken = tokenBuilder
+      .expiresIn(5, MINUTES)
+      .refreshableFor(1, MINUTES)
+      .build();
+
+    Optional<JwtAccessToken> refreshedToken = refresher.refresh(oldToken);
+
+    assertThat(refreshedToken).isEmpty();
   }
 
   @Test
   public void shouldNotRefreshTokenWhenStrategyDoesNotSaySo() {
     JwtAccessToken oldToken = tokenBuilder
-      .refreshableFor(10, TimeUnit.MINUTES)
+      .refreshableFor(10, MINUTES)
       .build();
     when(refreshStrategy.shouldBeRefreshed(oldToken)).thenReturn(false);
 
     Optional<JwtAccessToken> refreshedToken = refresher.refresh(oldToken);
 
-    Assertions.assertThat(refreshedToken).isEmpty();
+    assertThat(refreshedToken).isEmpty();
   }
 
   @Test
   public void shouldRefreshTokenWithEnabledRefresh() {
     JwtAccessToken oldToken = tokenBuilder
-      .refreshableFor(1, TimeUnit.MINUTES)
+      .expiresIn(1, MINUTES)
+      .refreshableFor(1, MINUTES)
       .build();
     when(refreshStrategy.shouldBeRefreshed(oldToken)).thenReturn(true);
 
     Optional<JwtAccessToken> refreshedToken = refresher.refresh(oldToken);
 
-    Assertions.assertThat(refreshedToken).isNotEmpty();
+    assertThat(refreshedToken).isNotEmpty();
   }
 }