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 1207b252e4..8e5de9c283 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java +++ b/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 ) { diff --git a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenRefresher.java b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenRefresher.java index cb26d1f010..16370209b6 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenRefresher.java +++ b/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 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()); } } diff --git a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenRefresherTest.java b/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenRefresherTest.java index 1464bfeade..7c7a8c68cf 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenRefresherTest.java +++ b/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 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 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 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 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 refreshedToken = refresher.refresh(oldToken); - Assertions.assertThat(refreshedToken).isNotEmpty(); + assertThat(refreshedToken).isNotEmpty(); } }