Skip to content

Commit c1f68c7

Browse files
authored
Fix refresh token behavior on EXPIRY_MARGIN (#122)
The previous behavior was resulting in the library using an expired token for a few minutes (for as long as EXPIRY_MARGIN_IN_MINUTES to be precise), and that is because we had the following timeline: - now-5min = 2022-12-23T09:57:00Z - now = 2022-12-23T10:02:00Z - expiresAt = 2022-12-23T10:05:00Z - now+5min = 2022-12-23T10:07:00Z So checking if expiresAt is before now-5min returns true until now = 2022-12-23T10:09:59Z, which means the calls will fail without the token being refreshed until that point.
1 parent d8ac35a commit c1f68c7

File tree

2 files changed

+28
-2
lines changed

2 files changed

+28
-2
lines changed

src/main/java/com/spotify/github/v3/clients/GitHubClient.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -701,8 +701,8 @@ private String getInstallationToken(final String jwtToken, final int installatio
701701
}
702702

703703
private boolean isExpired(final AccessToken token) {
704-
// Subtract a few minutes to avoid making calls with an expired token due to clock differences
705-
return token.expiresAt().isBefore(ZonedDateTime.now().minusMinutes(EXPIRY_MARGIN_IN_MINUTES));
704+
// Adds a few minutes to avoid making calls with an expired token due to clock differences
705+
return token.expiresAt().isBefore(ZonedDateTime.now().plusMinutes(EXPIRY_MARGIN_IN_MINUTES));
706706
}
707707

708708
private AccessToken generateInstallationToken(final String jwtToken, final int installationId)

src/test/java/com/spotify/github/v3/clients/GitHubAuthTest.java

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,13 @@ public class GitHubAuthTest {
7070
.toJson(
7171
ImmutableAccessToken.copyOf(getTestInstallationToken())
7272
.withExpiresAt(ZonedDateTime.now().minusHours(2))));
73+
private final MockResponse expiredTokenWithinExpiryMarginResponse =
74+
new MockResponse()
75+
.setBody(
76+
Json.create()
77+
.toJson(
78+
ImmutableAccessToken.copyOf(getTestInstallationToken())
79+
.withExpiresAt(ZonedDateTime.now().minusMinutes(3))));
7380
private final MockResponse checkRunResponse =
7481
new MockResponse().setBody(loadResource("com/spotify/github/v3/checks/checks-run-completed-response.json"));
7582

@@ -149,6 +156,25 @@ public void fetchesANewInstallationTokenIfExpired() throws Exception {
149156
assertThat(mockServer.takeRequest().getPath(), is("/repos/foo/bar/check-runs/123"));
150157
}
151158

159+
@Test
160+
public void fetchesANewInstallationTokenIfExpirationIsWithinExpiryMargin() throws Exception {
161+
mockServer.enqueue(expiredTokenWithinExpiryMarginResponse);
162+
mockServer.enqueue(checkRunResponse);
163+
mockServer.enqueue(validTokenResponse);
164+
mockServer.enqueue(checkRunResponse);
165+
166+
checksClient.getCheckRun(123).join();
167+
checksClient.getCheckRun(123).join();
168+
169+
// 2 to get the token, 2 checks
170+
assertThat(mockServer.getRequestCount(), is(4));
171+
172+
assertThat(mockServer.takeRequest().getPath(), is("/app/installations/1/access_tokens"));
173+
assertThat(mockServer.takeRequest().getPath(), is("/repos/foo/bar/check-runs/123"));
174+
assertThat(mockServer.takeRequest().getPath(), is("/app/installations/1/access_tokens"));
175+
assertThat(mockServer.takeRequest().getPath(), is("/repos/foo/bar/check-runs/123"));
176+
}
177+
152178
@Test
153179
public void throwsIfFetchingInstallationTokenRequestIsUnsuccessful() throws Exception {
154180
mockServer.enqueue(new MockResponse().setResponseCode(500));

0 commit comments

Comments
 (0)