From 6955b99b52657578da24ba8def68d4f57606e6d9 Mon Sep 17 00:00:00 2001 From: Pascal Schur <36639504+schurpl@users.noreply.github.com> Date: Mon, 11 Aug 2025 22:29:09 +0200 Subject: [PATCH 1/6] Update GitHubOAuthStrategy.java Testing service with logs --- .../meitrex/user_service/service/oauth/GitHubOAuthStrategy.java | 1 + 1 file changed, 1 insertion(+) 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..7ec6703 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 @@ -110,6 +110,7 @@ public AccessTokenResponse refreshAccessToken(String refreshToken) throws IOExce if (response.statusCode() == 200) { return parseTokenResponse(response.body()); + log.error("Failed to refresh access token. HTTP Status: {} Response: {}", response.statusCode(), response.body()); } log.error("Failed to refresh access token. HTTP Status: {} Response: {}", response.statusCode(), response.body()); From 2cd8d1b7a33491a374dc4137bbd3dc84fc6da1ed Mon Sep 17 00:00:00 2001 From: Pascal Schur <36639504+schurpl@users.noreply.github.com> Date: Mon, 11 Aug 2025 22:32:22 +0200 Subject: [PATCH 2/6] Update GitHubOAuthStrategy.java Changed log position --- .../user_service/service/oauth/GitHubOAuthStrategy.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 7ec6703..ea16d36 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 @@ -109,8 +109,9 @@ public AccessTokenResponse refreshAccessToken(String refreshToken) throws IOExce HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); if (response.statusCode() == 200) { - return parseTokenResponse(response.body()); log.error("Failed to refresh access token. HTTP Status: {} Response: {}", response.statusCode(), response.body()); + return parseTokenResponse(response.body()); + } log.error("Failed to refresh access token. HTTP Status: {} Response: {}", response.statusCode(), response.body()); From 1b1846887b620cb8bb3af399130fd1b5945fb2db Mon Sep 17 00:00:00 2001 From: Pascal Schur <36639504+schurpl@users.noreply.github.com> Date: Mon, 11 Aug 2025 22:36:46 +0200 Subject: [PATCH 3/6] Update docker-build.yml Skip if for testing --- .github/workflows/docker-build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/docker-build.yml b/.github/workflows/docker-build.yml index 1f3a7b3..d0627e3 100644 --- a/.github/workflows/docker-build.yml +++ b/.github/workflows/docker-build.yml @@ -11,7 +11,7 @@ on: jobs: docker-build: runs-on: ubuntu-latest - if: github.ref == 'refs/heads/main' + #if: github.ref == 'refs/heads/main' steps: - name: Setup docker buildx uses: docker/setup-buildx-action@v2 From cb28c93edbccea66303c28433f8a03d5b1c2054d Mon Sep 17 00:00:00 2001 From: Mykhailo Zelia Date: Sat, 16 Aug 2025 21:41:57 +0200 Subject: [PATCH 4/6] Fix access tokens refresh --- .../service/AccessTokenService.java | 22 ++++++++++++------- .../service/oauth/GitHubOAuthStrategy.java | 5 ++++- 2 files changed, 18 insertions(+), 9 deletions(-) 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..3ea4422 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 @@ -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's a problem with the refresh token, 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,15 +97,18 @@ public AccessToken getAccessToken(UUID currentUserId, ExternalServiceProviderDto AccessTokenEntity accessToken = accessTokenOptional.get(); OffsetDateTime now = OffsetDateTime.now(); - if (accessToken.getAccessTokenExpiresAt() == null || accessToken.getAccessTokenExpiresAt().isAfter(now)) { + // commented out because the normal workflow didnt' work, where we had isAccessTokenAvailable check first, then getAccessToken with refresh if needed. + // instead we have isAccessTokenAvailable check and refreshAccessToken if needed, and then call getAccessToken that gets us the token. + // (check the isAccessTokenAvailable method for more details) +// if (accessToken.getAccessTokenExpiresAt() == null || accessToken.getAccessTokenExpiresAt().isAfter(now)) return new AccessToken(accessToken.getAccessToken(), accessToken.getExternalUserId()); - } - if (!accessToken.getRefreshTokenExpiresAt().isAfter(now)) { - throw new EntityNotFoundException("Access token expired and refresh token expired for user " + currentUserInfo.getId() + " and provider " + provider); - } - return refreshAccessToken(accessToken, provider); +// if (!accessToken.getRefreshTokenExpiresAt().isAfter(now)) { +// throw new EntityNotFoundException("Access token expired and refresh token expired for user " + currentUserInfo.getId() + " and provider " + provider); +// } +// +// return refreshAccessToken(accessToken, provider); } private AccessToken refreshAccessToken(AccessTokenEntity accessToken, ExternalServiceProvider provider) { 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 ea16d36..ad9239f 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,7 +84,9 @@ public AccessTokenResponse exchangeCodeForAccessToken(String code) throws IOExce HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); - if (response.statusCode() == 200) { + JsonObject json = JsonParser.parseString(response.body()).getAsJsonObject(); + + if (response.statusCode() == 200 && !json.has("error")) { return parseTokenResponse(response.body()); } @@ -107,6 +109,7 @@ public AccessTokenResponse refreshAccessToken(String refreshToken) throws IOExce .build(); HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); + JsonObject json = JsonParser.parseString(response.body()).getAsJsonObject(); if (response.statusCode() == 200) { log.error("Failed to refresh access token. HTTP Status: {} Response: {}", response.statusCode(), response.body()); From 73e245067d00588ec219202ad01a206dc8631ddf Mon Sep 17 00:00:00 2001 From: Mykhailo Zelia Date: Sat, 16 Aug 2025 22:31:34 +0200 Subject: [PATCH 5/6] Fix access tokens refresh --- .../service/AccessTokenService.java | 23 ++++++------ .../service/oauth/GitHubOAuthStrategy.java | 35 ++++++++++++------- .../service/AccessTokenServiceTest.java | 8 ++++- 3 files changed, 42 insertions(+), 24 deletions(-) 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 3ea4422..74601e2 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. @@ -69,7 +69,7 @@ public boolean isAccessTokenAvailable(LoggedInUser currentUser, ExternalServiceP } // 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's a problem with the refresh token, so we check here if the refresh works. + // 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; @@ -97,18 +97,19 @@ public AccessToken getAccessToken(UUID currentUserId, ExternalServiceProviderDto AccessTokenEntity accessToken = accessTokenOptional.get(); OffsetDateTime now = OffsetDateTime.now(); - // commented out because the normal workflow didnt' work, where we had isAccessTokenAvailable check first, then getAccessToken with refresh if needed. - // instead we have isAccessTokenAvailable check and refreshAccessToken if needed, and then call getAccessToken that gets us the token. - // (check the isAccessTokenAvailable method for more details) -// if (accessToken.getAccessTokenExpiresAt() == null || accessToken.getAccessTokenExpiresAt().isAfter(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); + } -// if (!accessToken.getRefreshTokenExpiresAt().isAfter(now)) { -// throw new EntityNotFoundException("Access token expired and refresh token expired for user " + currentUserInfo.getId() + " and provider " + provider); -// } -// -// return refreshAccessToken(accessToken, provider); + return refreshAccessToken(accessToken, provider); } private AccessToken refreshAccessToken(AccessTokenEntity accessToken, ExternalServiceProvider provider) { 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 ad9239f..055430f 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,14 +84,17 @@ public AccessTokenResponse exchangeCodeForAccessToken(String code) throws IOExce HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); - JsonObject json = JsonParser.parseString(response.body()).getAsJsonObject(); + if (response.statusCode() != 200) { + log.error("Failed to exchange code. HTTP {} Body: {}", response.statusCode(), response.body()); + return null; + } - if (response.statusCode() == 200 && !json.has("error")) { + try { return parseTokenResponse(response.body()); + } catch (Exception ex) { + log.error("Non-JSON or malformed token response body: {}", response.body(), ex); + return null; } - - log.error("Failed to exchange code for access token. HTTP Status: {} Response: {}", response.statusCode(), response.body()); - return null; } @Override @@ -109,18 +112,26 @@ public AccessTokenResponse refreshAccessToken(String refreshToken) throws IOExce .build(); HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); - JsonObject json = JsonParser.parseString(response.body()).getAsJsonObject(); - if (response.statusCode() == 200) { - log.error("Failed to refresh access token. HTTP Status: {} Response: {}", response.statusCode(), response.body()); - 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(), ex); + 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)); From b6c41ec0f4cde0f12504847b5a827eb28e00e06a Mon Sep 17 00:00:00 2001 From: Mykhailo Zelia Date: Sat, 23 Aug 2025 13:51:06 +0200 Subject: [PATCH 6/6] Fix stacktrace --- .../iste/meitrex/user_service/service/AccessTokenService.java | 4 ++-- .../user_service/service/oauth/GitHubOAuthStrategy.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) 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 74601e2..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 @@ -130,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; } @@ -168,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 055430f..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 @@ -92,7 +92,7 @@ public AccessTokenResponse exchangeCodeForAccessToken(String code) throws IOExce try { return parseTokenResponse(response.body()); } catch (Exception ex) { - log.error("Non-JSON or malformed token response body: {}", response.body(), ex); + log.error("Non-JSON or malformed token response body: {}", response.body()); return null; } } @@ -126,7 +126,7 @@ public AccessTokenResponse refreshAccessToken(String refreshToken) throws IOExce } return parseTokenResponse(response.body()); } catch (Exception ex) { - log.error("Non-JSON or malformed refresh response body: {}", response.body(), ex); + log.error("Non-JSON or malformed refresh response body: {}", response.body()); return null; } }