diff --git a/src/main/java/de/unistuttgart/iste/meitrex/user_service/service/AccessTokenService.java b/src/main/java/de/unistuttgart/iste/meitrex/user_service/service/AccessTokenService.java index 827527d..97a542b 100644 --- a/src/main/java/de/unistuttgart/iste/meitrex/user_service/service/AccessTokenService.java +++ b/src/main/java/de/unistuttgart/iste/meitrex/user_service/service/AccessTokenService.java @@ -44,7 +44,7 @@ public class AccessTokenService { private final ExternalOAuthClient externalOAuthClient; /** - * Checks if a valid access token or refresh token is available for a given user and provider. + * Checks if a valid access token is available for a given user and provider. * * @param currentUser the currently logged-in user. * @param providerDto the external service provider name. @@ -68,8 +68,11 @@ public boolean isAccessTokenAvailable(LoggedInUser currentUser, ExternalServiceP return true; } - // If access token expired, check if the refresh token is expired - return accessToken.getRefreshTokenExpiresAt().isAfter(now); + // If access token expired, check if the refresh token is expired and try to refresh it + // There's a bug in GH Api that returns 200 error response because it thinks there is a problem with the refresh token (there is not), so we check here if the refresh works. + // if it does, then refreshAccessToken returns true and a new access token is saved, so getAccessToken can return it (without needing to refresh). + // if it doesn't, then returns false and the user will be prompted to re-authorize GitHub. (using generateAccessToken) + return accessToken.getRefreshTokenExpiresAt().isAfter(now) && refreshAccessToken(accessToken, provider) != null; } /** @@ -94,10 +97,14 @@ public AccessToken getAccessToken(UUID currentUserId, ExternalServiceProviderDto AccessTokenEntity accessToken = accessTokenOptional.get(); OffsetDateTime now = OffsetDateTime.now(); + if (accessToken.getAccessTokenExpiresAt() == null || accessToken.getAccessTokenExpiresAt().isAfter(now)) { return new AccessToken(accessToken.getAccessToken(), accessToken.getExternalUserId()); } + // the lines below won't run in current setup, since if we call getAccessToken, we already refreshed the access token if needed in isAccessTokenAvailable. + // but if Github resolve their problem, the normal workflow should be used instead, i.e. in isAccessTokenAvailable, we only check if the access token is expired without refreshing. + // The refresh will be done below then (check the isAccessTokenAvailable method for more details) if (!accessToken.getRefreshTokenExpiresAt().isAfter(now)) { throw new EntityNotFoundException("Access token expired and refresh token expired for user " + currentUserInfo.getId() + " and provider " + provider); } @@ -123,7 +130,7 @@ private AccessToken refreshAccessToken(AccessTokenEntity accessToken, ExternalSe return new AccessToken(accessToken.getAccessToken(), accessToken.getExternalUserId()); } catch (IOException | InterruptedException e) { if (e instanceof InterruptedException) Thread.currentThread().interrupt(); - log.error("Failed to refresh access token for user {} and provider {}", accessToken.getUserId(), provider, e); + log.error("Failed to refresh access token for user {} and provider {}", accessToken.getUserId(), provider); } return null; } @@ -161,7 +168,7 @@ public boolean generateAccessToken(LoggedInUser currentUser, GenerateAccessToken } } catch (IOException | InterruptedException e) { if (e instanceof InterruptedException) Thread.currentThread().interrupt(); - log.error("Failed to generate access token for user {} and provider {}", currentUserInfo.getId(), provider, e); + log.error("Failed to generate access token for user {} and provider {}", currentUserInfo.getId(), provider); } return false; } diff --git a/src/main/java/de/unistuttgart/iste/meitrex/user_service/service/oauth/GitHubOAuthStrategy.java b/src/main/java/de/unistuttgart/iste/meitrex/user_service/service/oauth/GitHubOAuthStrategy.java index 2ce9348..828bded 100644 --- a/src/main/java/de/unistuttgart/iste/meitrex/user_service/service/oauth/GitHubOAuthStrategy.java +++ b/src/main/java/de/unistuttgart/iste/meitrex/user_service/service/oauth/GitHubOAuthStrategy.java @@ -84,12 +84,17 @@ public AccessTokenResponse exchangeCodeForAccessToken(String code) throws IOExce HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); - if (response.statusCode() == 200) { - return parseTokenResponse(response.body()); + if (response.statusCode() != 200) { + log.error("Failed to exchange code. HTTP {} Body: {}", response.statusCode(), response.body()); + return null; } - log.error("Failed to exchange code for access token. HTTP Status: {} Response: {}", response.statusCode(), response.body()); - return null; + try { + return parseTokenResponse(response.body()); + } catch (Exception ex) { + log.error("Non-JSON or malformed token response body: {}", response.body()); + return null; + } } @Override @@ -108,14 +113,25 @@ public AccessTokenResponse refreshAccessToken(String refreshToken) throws IOExce HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); - if (response.statusCode() == 200) { - return parseTokenResponse(response.body()); + if (response.statusCode() != 200) { + log.error("Failed to refresh token. HTTP {} Body: {}", response.statusCode(), response.body()); + return null; } - log.error("Failed to refresh access token. HTTP Status: {} Response: {}", response.statusCode(), response.body()); - return null; + try { + JsonObject json = JsonParser.parseString(response.body()).getAsJsonObject(); + if (json.has("error")) { + log.error("GitHub refresh error: {}", json); + return null; + } + return parseTokenResponse(response.body()); + } catch (Exception ex) { + log.error("Non-JSON or malformed refresh response body: {}", response.body()); + return null; + } } + public String fetchExternalUserId(String accessToken) throws IOException, InterruptedException { HttpRequest request = HttpRequest.newBuilder() .uri(URI.create(githubInfo().getExternalUserIdUrl())) diff --git a/src/test/java/de/unistuttgart/iste/meitrex/user_service/service/AccessTokenServiceTest.java b/src/test/java/de/unistuttgart/iste/meitrex/user_service/service/AccessTokenServiceTest.java index 5bd9f3a..b178bee 100644 --- a/src/test/java/de/unistuttgart/iste/meitrex/user_service/service/AccessTokenServiceTest.java +++ b/src/test/java/de/unistuttgart/iste/meitrex/user_service/service/AccessTokenServiceTest.java @@ -27,6 +27,7 @@ import static org.mockito.Mockito.*; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; class AccessTokenServiceTest { @@ -100,13 +101,18 @@ void testIsAccessTokenAvailable_ValidExpiringToken() { } @Test - void testIsAccessTokenAvailable_ExpiredToken_ValidRefreshToken() { + void testIsAccessTokenAvailable_ExpiredToken_ValidRefreshToken() throws Exception{ validAccessToken.setAccessToken("valid_access_token"); validAccessToken.setAccessTokenExpiresAt(OffsetDateTime.now().minusMinutes(5)); validAccessToken.setRefreshToken("refresh_token"); validAccessToken.setRefreshTokenExpiresAt(OffsetDateTime.now().plusMinutes(5)); when(accessTokenRepository.findByUserIdAndProvider(any(), any())).thenReturn(Optional.of(validAccessToken)); + AccessTokenResponse refreshed = new AccessTokenResponse( + "new_valid_access_token", /* expiresIn */ 3600, "new_refresh", /* refreshExpiresIn */ 7200); + when(externalOAuthClient.refreshAccessToken(eq("refresh_token"), any())) + .thenReturn(refreshed); + Boolean result = accessTokenService.isAccessTokenAvailable(loggedInUser, providerDto); assertThat(result, is(true));