Merge pull request #23 from MEITREX/schurpl-testing

Fix 200 status error response for refreshing access tokens

Author: Morphenoed

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;
     }

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<String> 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<String> 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()))

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));