Skip to content

Conversation

@kwon204
Copy link
Contributor

@kwon204 kwon204 commented Feb 7, 2025

#️⃣ 연관된 이슈>

📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

  • 구글 access token 관리에 대한 기능 입니다.
    • refresh token을 이용해 재발급받는 기능
    • 재발급한 토큰을 저장하는 기능
  • 인증 기능이 추가되면서 유저 기능과 분리하였습니다.
  • Google 인증 기능을 UserService에서 분리하여 테스트가 편해졌습니다.
  • RestTemplate을 RestClient로 바꾸었습니다.
    • RestClient는 메소드 체이닝 방식으로 사용할 수 있어서 사용하기 편해졌습니다.

🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

Google api는 global.google 패키지에 관리하면 좋을 거 같은데 어떤가요?

Summary by CodeRabbit

  • New Features
    • Launched an enhanced Google authentication flow, offering a more seamless sign-in experience with dedicated endpoints for login and callback.
  • Refactor
    • Consolidated authentication logic into a dedicated module for clearer separation from user management.
    • Improved error handling for OAuth-related issues, ensuring users receive more informative feedback during authentication.
    • Updated client request configuration for enhanced performance and reliability.
  • Bug Fixes
    • Enhanced handling of OAuth exceptions, providing specific error messages for invalid codes and user information.

@kwon204 kwon204 added the 🛠️ BE Backend label Feb 7, 2025
@kwon204 kwon204 self-assigned this Feb 7, 2025
@kwon204 kwon204 requested a review from efdao as a code owner February 7, 2025 06:26
@coderabbitai
Copy link

coderabbitai bot commented Feb 7, 2025

Walkthrough

This pull request refactors the system’s handling of Google OAuth authentication. A new AuthController and AuthService are introduced to manage Google login URL generation and OAuth2 callback processing. The OAuth-related DTOs have been relocated from the user domain to the auth domain. Existing Google OAuth methods are removed from UserController and UserService, with token update logic now centralized. Additionally, a new GoogleOAuthService handles token exchanges and user info retrieval, the RestClient configuration is updated, OAuth-specific error handling is enhanced, and tests have been refactored to reflect these changes.

Changes

Files Change Summary
backend/src/main/java/endolphin/backend/domain/auth/AuthController.java
backend/src/main/java/endolphin/backend/domain/auth/AuthService.java
Added new authentication controller and service to handle Google login URL retrieval and OAuth2 callback processing.
backend/src/main/java/endolphin/backend/domain/auth/dto/OAuthResponse.java
backend/src/main/java/endolphin/backend/domain/auth/dto/UrlResponse.java
Updated package declarations for OAuthResponse and UrlResponse; moved from user domain to auth domain.
backend/src/main/java/endolphin/backend/domain/user/UserController.java Removed obsolete Google OAuth methods (loginUrl and oauthCallback) from the UserController.
backend/src/main/java/endolphin/backend/domain/user/UserService.java Removed Google OAuth related methods; added updateAccessToken method; updated imports and method signature of createUser.
backend/src/main/java/endolphin/backend/domain/user/entity/User.java Added @Setter for the accessToken field.
backend/src/main/java/endolphin/backend/global/config/RestClientConfig.java Refactored RestTemplateConfig to RestClientConfig; updated method to return a RestClient using a builder pattern with timeout settings.
backend/src/main/java/endolphin/backend/global/error/GlobalExceptionHandler.java Added handleOAuthException method to manage OAuthException and return a proper ErrorResponse.
backend/src/main/java/endolphin/backend/global/error/exception/ErrorCode.java
backend/src/main/java/endolphin/backend/global/error/exception/OAuthException.java
Introduced new OAuthException class and added OAuth-related enum constants (INVALID_OAUTH_CODE, INVALID_OAUTH_USER_INFO) to ErrorCode.
backend/src/main/java/endolphin/backend/global/google/GoogleOAuthService.java Added new service to handle Google token exchange, user info retrieval, and reissuing access tokens through a RestClient.
backend/src/main/java/endolphin/backend/global/google/dto/GoogleTokens.java
backend/src/main/java/endolphin/backend/global/google/dto/GoogleUserInfo.java
Updated package declarations for GoogleTokens and GoogleUserInfo; moved from the user dto to the global google dto package.
backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java Renamed and refactored tests to focus on the authentication service instead of the user service; updated dependencies and import paths accordingly.

Sequence Diagram(s)

sequenceDiagram
   participant C as Client
   participant AC as AuthController
   participant AS as AuthService
   C->>AC: GET /api/v1/google
   AC->>AS: loginUrl()
   AS-->>AC: UrlResponse
   AC-->>C: ResponseEntity<UrlResponse>
Loading
sequenceDiagram
   participant C as Client
   participant AC as AuthController
   participant AS as AuthService
   participant GS as GoogleOAuthService
   participant GO as Google OAuth Server
   C->>AC: GET /oauth2/callback?code=XYZ
   AC->>AS: oauthCallback(code)
   AS->>GS: getGoogleTokens(code)
   GS->>GO: Exchange code for tokens
   GO-->>GS: Tokens
   GS->>AS: Return tokens
   AS->>GS: getUserInfo(accessToken)
   GS->>GO: Fetch user info
   GO-->>GS: User info
   GS-->>AS: Return user info
   AS-->>AC: OAuthResponse (JWT)
   AC-->>C: ResponseEntity<OAuthResponse>
Loading

Suggested reviewers

  • efdao

Poem

I'm a happy little rabbit,
Hopping through lines of code so neat,
With OAuth and tokens,
Our changes can't be beat,
In fields of config and error delight,
I nibble on bugs till they're out of sight 🐰🌸!

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (8)
backend/src/main/java/endolphin/backend/domain/user/UserService.java (1)

33-46: Update existing users with new tokens.
Currently, if the user already exists, tokens may become outdated. As a follow-up, consider updating the user’s tokens in the same flow (instead of ignoring them) to keep the account information fully synced.

backend/src/main/java/endolphin/backend/global/google/GoogleOAuthService.java (2)

26-28: Remove or utilize the injected UserService.
UserService is injected but never used in this class. Consider removing it or leveraging it if needed (e.g., to handle user-specific logic when reissuing tokens).


81-86: Extend error handling for 5xx responses.
Right now, only 4xx client errors throw an OAuthException. Consider also handling 5xx server errors, as Google’s API could fail temporarily (e.g., service downtime). A fallback or retry mechanism could improve reliability.

backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (1)

66-95: Add negative-case unit tests.
These tests nicely verify the happy path. However, there are no tests covering negative scenarios (e.g., invalid code, null tokens). Expanding coverage for error handling would make the authentication flow more robust.

Do you want me to generate an additional negative-test suite that verifies appropriate error handling for invalid or expired credentials?

backend/src/main/java/endolphin/backend/global/error/exception/OAuthException.java (1)

7-21: Add documentation for the exception class.

The implementation looks good, but would benefit from class-level documentation explaining its purpose and usage.

Add JavaDoc comments:

+/**
+ * Custom exception for handling OAuth-related errors.
+ * Maps HTTP status codes to specific error codes for OAuth operations.
+ */
 @Getter
 public class OAuthException extends RuntimeException {
backend/src/main/java/endolphin/backend/domain/user/entity/User.java (1)

31-35: Consider security implications of token storage.

While adding @Setter for accessToken enables token management, there are security considerations to address:

  1. Direct setter exposure could lead to unauthorized token modifications
  2. Tokens are stored in plain text

Consider:

  1. Using a more controlled method for token updates
  2. Implementing encryption for token storage
  3. Adding audit logging for token modifications

Would you like me to provide an example implementation with these security enhancements?

backend/src/main/java/endolphin/backend/domain/auth/AuthController.java (1)

20-25: Consider adding rate limiting for the login URL endpoint.

While the implementation is correct, consider adding rate limiting to prevent potential abuse of the Google OAuth endpoint.

backend/src/main/java/endolphin/backend/domain/auth/AuthService.java (1)

24-29: Improve URL generation with UriComponentsBuilder.

Replace String.format with UriComponentsBuilder to properly handle URL encoding and improve maintainability.

-    public UrlResponse getGoogleLoginUrl() {
-        return new UrlResponse(String.format(
-            "%s?client_id=%s&redirect_uri=%s&response_type=code&scope=%s&access_type=offline&prompt=consent",
-            googleOAuthProperties.authUrl(), googleOAuthProperties.clientId(),
-            googleOAuthProperties.redirectUri(), googleOAuthProperties.scope()));
-    }
+    public UrlResponse getGoogleLoginUrl() {
+        String url = UriComponentsBuilder
+            .fromUriString(googleOAuthProperties.authUrl())
+            .queryParam("client_id", googleOAuthProperties.clientId())
+            .queryParam("redirect_uri", googleOAuthProperties.redirectUri())
+            .queryParam("response_type", "code")
+            .queryParam("scope", googleOAuthProperties.scope())
+            .queryParam("access_type", "offline")
+            .queryParam("prompt", "consent")
+            .build()
+            .toUriString();
+        return new UrlResponse(url);
+    }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 086f511 and 80c41e5407f8e3be87f0e5fb5183f047872321d5.

📒 Files selected for processing (15)
  • backend/src/main/java/endolphin/backend/domain/auth/AuthController.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/auth/AuthService.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/auth/dto/OAuthResponse.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/auth/dto/UrlResponse.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/user/UserController.java (0 hunks)
  • backend/src/main/java/endolphin/backend/domain/user/UserService.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/user/entity/User.java (2 hunks)
  • backend/src/main/java/endolphin/backend/global/config/RestClientConfig.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/error/GlobalExceptionHandler.java (2 hunks)
  • backend/src/main/java/endolphin/backend/global/error/exception/ErrorCode.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/error/exception/OAuthException.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/google/GoogleOAuthService.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/google/dto/GoogleTokens.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/google/dto/GoogleUserInfo.java (1 hunks)
  • backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (2 hunks)
💤 Files with no reviewable changes (1)
  • backend/src/main/java/endolphin/backend/domain/user/UserController.java
✅ Files skipped from review due to trivial changes (4)
  • backend/src/main/java/endolphin/backend/global/google/dto/GoogleTokens.java
  • backend/src/main/java/endolphin/backend/global/google/dto/GoogleUserInfo.java
  • backend/src/main/java/endolphin/backend/domain/auth/dto/UrlResponse.java
  • backend/src/main/java/endolphin/backend/domain/auth/dto/OAuthResponse.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (5)
backend/src/main/java/endolphin/backend/domain/user/UserService.java (1)

28-31: Handle potential null user object.
If there's any chance user is null (e.g., caller-supplied), consider adding a null check or validating user existence before setting the token, to prevent unintended NullPointerExceptions.

backend/src/main/java/endolphin/backend/global/config/RestClientConfig.java (1)

6-6: LGTM! Good transition to RestClient.

The change from RestTemplate to RestClient is a good modernization step. The configuration is properly set up with appropriate timeouts.

Let's verify that all RestTemplate usages have been updated:

Also applies to: 9-9, 12-12, 16-16

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Search for any remaining RestTemplate usages that might need updating
rg "RestTemplate" -l

Length of output: 20


Verification Successful: No Remaining RestTemplate Usages
The codebase does not contain any references to RestTemplate, which confirms that the migration to RestClient was applied consistently.

backend/src/main/java/endolphin/backend/domain/auth/AuthController.java (2)

13-16: LGTM! Well-structured controller with clear responsibilities.

The controller is properly annotated and follows best practices:


27-32: Consider adding request validation and error handling.

The callback endpoint should validate the code parameter and handle potential error responses from Google OAuth.

Run this script to check if error handling is implemented:

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Search for OAuth error handling in the codebase
rg -A 5 "OAuthException|GoogleOAuthService.*error"

Length of output: 4327


Error handling is indeed in place, though additional request validation for the "code" parameter isn’t explicitly performed.

• The OAuth service (in GoogleOAuthService.java) throws an OAuthException when an error is encountered, and a global exception handler (GlobalExceptionHandler.java) properly catches and processes these exceptions.
• The controller relies on Spring’s @RequestParam to mandate the presence of the "code" parameter, which covers missing parameter cases—but it does not enforce further checks (such as ensuring the parameter isn’t empty or malformed).

Based on the analysis and shell script output, error responses from Google OAuth are handled via the service and global handler. You may still consider adding explicit validation (e.g., using @validated or @notblank) if you want to provide more specific feedback on invalid values.

backend/src/main/java/endolphin/backend/global/error/GlobalExceptionHandler.java (1)

24-30: LGTM! Consistent error handling implementation.

The OAuthException handler follows the established pattern and includes proper error logging.

@efdao efdao added this to the 2차 스프린트 milestone Feb 7, 2025
@efdao efdao linked an issue Feb 7, 2025 that may be closed by this pull request
2 tasks
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
backend/src/main/java/endolphin/backend/global/error/exception/OAuthException.java (1)

12-21: Consider using a Map for status to error code mapping.

While the switch expression is clean and readable, as the number of status mappings grows, maintaining them in a switch statement could become cumbersome. Consider using a static Map for better maintainability.

Here's a suggested implementation:

+    private static final Map<HttpStatus, ErrorCode> STATUS_TO_ERROR_CODE = Map.of(
+        HttpStatus.UNAUTHORIZED, ErrorCode.OAUTH_UNAUTHORIZED_ERROR,
+        HttpStatus.FORBIDDEN, ErrorCode.OAUTH_FORBIDDEN_ERROR,
+        HttpStatus.BAD_REQUEST, ErrorCode.OAUTH_BAD_REQUEST_ERROR
+    );

     public OAuthException(HttpStatus status, String message) {
         super(message);
         this.status = status;
-        switch (status) {
-            case UNAUTHORIZED -> errorCode = ErrorCode.OAUTH_UNAUTHORIZED_ERROR;
-            case FORBIDDEN -> errorCode = ErrorCode.OAUTH_FORBIDDEN_ERROR;
-            case BAD_REQUEST -> errorCode = ErrorCode.OAUTH_BAD_REQUEST_ERROR;
-            default -> errorCode = ErrorCode.INTERNAL_ERROR;
-        }
+        this.errorCode = STATUS_TO_ERROR_CODE.getOrDefault(status, ErrorCode.INTERNAL_ERROR);
     }
backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (2)

51-61: Consider adding edge cases to URL generation test.

While the happy path is well tested, consider adding test cases for:

  • Empty or null scope values
  • Special characters in redirect URI
  • Multiple scope values

65-95: Add error scenario tests for OAuth callback.

While the successful flow is well tested, consider adding test cases for:

  • Invalid authorization code
  • Token service errors
  • User info service errors
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 80c41e5407f8e3be87f0e5fb5183f047872321d5 and d31b88d8b6d4bb9ba9d793a802358d8ce68a1488.

📒 Files selected for processing (6)
  • backend/src/main/java/endolphin/backend/domain/auth/AuthService.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/user/UserService.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/user/entity/User.java (2 hunks)
  • backend/src/main/java/endolphin/backend/global/error/exception/ErrorCode.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/error/exception/OAuthException.java (1 hunks)
  • backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • backend/src/main/java/endolphin/backend/domain/user/entity/User.java
  • backend/src/main/java/endolphin/backend/domain/auth/AuthService.java
  • backend/src/main/java/endolphin/backend/global/error/exception/ErrorCode.java
🔇 Additional comments (10)
backend/src/main/java/endolphin/backend/global/error/exception/OAuthException.java (4)

1-7: LGTM! Well-structured class declaration with appropriate inheritance.

The class is properly placed in the error handling package and extends RuntimeException, which is appropriate for OAuth-related exceptions that typically don't require explicit handling.


9-10: LGTM! Well-designed immutable fields.

The fields are correctly marked as final and private, with @Getter providing controlled access.


23-27: LGTM! Well-implemented constructor with direct error code mapping.

The constructor properly initializes both fields and the parent class using the error code's properties.


1-28: Verify error code enum values.

Let's verify that all required OAuth error scenarios are covered in the ErrorCode enum.

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Search for ErrorCode enum definition and its OAuth-related values
ast-grep --pattern 'enum ErrorCode {
  $$$
  OAUTH_$$$
  $$$
}'

Length of output: 65


I'm going to run additional shell commands to search for the ErrorCode enum definition and to list lines containing "OAUTH_" so we can verify that the required OAuth error scenarios are indeed covered.


🏁 Script executed:

#!/bin/bash
# List the ErrorCode enum definition context and filter for OAuth-related constants

# First, find the file containing the ErrorCode enum
rg -n "enum ErrorCode" .

# Now, list lines with OAuth-related error codes
rg -n "OAUTH_" .

Length of output: 2075


OAuth ErrorCode Enum Verification Complete
The ErrorCode enum contains the three OAuth-specific constants (OAUTH_UNAUTHORIZED_ERROR, OAUTH_BAD_REQUEST_ERROR, and OAUTH_FORBIDDEN_ERROR) that are mapped in the OAuthException switch. Additionally, there are separate constants for invalid OAuth scenarios (INVALID_OAUTH_CODE and INVALID_OAUTH_USER_INFO) used in AuthService. This confirms that all required OAuth error scenarios are covered.

backend/src/main/java/endolphin/backend/domain/user/UserService.java (4)

3-4: LGTM! Imports align with package organization goals.

The relocation of Google-related DTOs to the global.google package improves code organization.


22-27: LGTM! Good optimization with readOnly transaction.

The readOnly flag is appropriate for this query method and allows for potential performance optimizations.


29-32: LGTM! Clean implementation of token update logic.

The method follows single responsibility principle and properly handles the persistence of token updates.


34-47: LGTM! Token update logic properly implemented.

The method now correctly handles both access and refresh tokens during user upsert operations, addressing the previous feedback about missing token update logic.

backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (2)

1-17: LGTM! Package structure aligns with domain separation.

The move to auth package and updated imports properly reflect the separation of authentication concerns.


27-42: LGTM! Comprehensive test setup with appropriate mocks.

The test class properly sets up all necessary dependencies for testing the auth service functionality.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p2; status가 필요한가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 저장할 필요는 없네요. 수정하겠습니다.

RestTemplate와 RestClient는 같은 기능을 하지만 메소드 체이닝 방식으로 사용할 수 있어서 교체
# Conflicts:
#	backend/src/main/java/endolphin/backend/global/error/exception/ErrorCode.java
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (1)

63-95: Add error case tests for OAuth callback.

The test only covers the happy path. Consider adding test cases for:

  • Invalid authorization code
  • Token retrieval failure
  • User info retrieval failure
@Test
@DisplayName("Invalid authorization code should throw OAuthException")
void oauth2Callback_ShouldThrowOAuthException_WhenInvalidCode() {
    // Given
    String invalidCode = "invalid-code";
    given(googleOAuthService.getGoogleTokens(anyString()))
        .willThrow(new OAuthException(HttpStatus.BAD_REQUEST, "invalid_grant"));

    // When/Then
    assertThrows(OAuthException.class, () -> authService.oauth2Callback(invalidCode));
}
🧹 Nitpick comments (4)
backend/src/main/java/endolphin/backend/global/error/exception/ErrorCode.java (1)

21-26: Consider adding more specific error messages.

While the current messages are clear, they could be more specific to help with debugging. For example:

-    OAUTH_UNAUTHORIZED_ERROR(HttpStatus.UNAUTHORIZED, "O001", "OAuth Unauthorized Error"),
+    OAUTH_UNAUTHORIZED_ERROR(HttpStatus.UNAUTHORIZED, "O001", "Google OAuth authentication failed"),
-    OAUTH_BAD_REQUEST_ERROR(HttpStatus.BAD_REQUEST, "O002", "OAuth Bad Request Error"),
+    OAUTH_BAD_REQUEST_ERROR(HttpStatus.BAD_REQUEST, "O002", "Invalid OAuth request parameters"),
-    OAUTH_FORBIDDEN_ERROR(HttpStatus.FORBIDDEN, "O003", "OAuth Forbidden Error"),
+    OAUTH_FORBIDDEN_ERROR(HttpStatus.FORBIDDEN, "O003", "Access to Google resource denied"),
-    INVALID_OAUTH_CODE(HttpStatus.UNAUTHORIZED, "O004", "Invalid OAuth Code"),
+    INVALID_OAUTH_CODE(HttpStatus.UNAUTHORIZED, "O004", "Invalid or expired Google OAuth code"),
-    INVALID_OAUTH_USER_INFO(HttpStatus.UNAUTHORIZED, "O005", "Invalid OAuth User Info"),
+    INVALID_OAUTH_USER_INFO(HttpStatus.UNAUTHORIZED, "O005", "Failed to retrieve Google user information"),
backend/src/main/java/endolphin/backend/global/google/GoogleOAuthService.java (2)

56-71: Add logging for token reissuance events.

Consider adding debug logging to track token refresh operations for better observability.

 public String reissueAccessToken(String refreshToken) {
+    log.debug("Reissuing access token using refresh token");
     MultiValueMap<String, String> params = getStringStringMultiValueMap();
     params.add("grant_type", "refresh_token");
     params.add("refresh_token", refreshToken);

     return restClient.post()
         .uri(googleOAuthProperties.tokenUrl())
         .contentType(MediaType.APPLICATION_FORM_URLENCODED)
         .body(params)
         .exchange((request, response) -> {
             validateResponse(response);
             GoogleTokens tokens = response.bodyTo(GoogleTokens.class);
+            log.debug("Successfully reissued access token");
             return tokens.accessToken();
         });
 }

81-86: Enhance error handling to include 5xx errors.

The validation only handles client errors (4xx). Consider handling server errors (5xx) as well for more robust error handling.

 private void validateResponse(ConvertibleClientHttpResponse response) throws IOException {
-    if (response.getStatusCode().is4xxClientError()) {
+    if (response.getStatusCode().is4xxClientError() || response.getStatusCode().is5xxServerError()) {
         String error = response.bodyTo(String.class);
         throw new OAuthException((HttpStatus) response.getStatusCode(), error);
     }
 }
backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (1)

49-61: Add edge cases for URL generation test.

Consider adding test cases for:

  • Empty/null scope
  • Special characters in clientId/redirectUri
  • Missing optional parameters
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d31b88d8b6d4bb9ba9d793a802358d8ce68a1488 and 0874c90.

📒 Files selected for processing (15)
  • backend/src/main/java/endolphin/backend/domain/auth/AuthController.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/auth/AuthService.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/auth/dto/OAuthResponse.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/auth/dto/UrlResponse.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/user/UserController.java (0 hunks)
  • backend/src/main/java/endolphin/backend/domain/user/UserService.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/user/entity/User.java (2 hunks)
  • backend/src/main/java/endolphin/backend/global/config/RestClientConfig.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/error/GlobalExceptionHandler.java (2 hunks)
  • backend/src/main/java/endolphin/backend/global/error/exception/ErrorCode.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/error/exception/OAuthException.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/google/GoogleOAuthService.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/google/dto/GoogleTokens.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/google/dto/GoogleUserInfo.java (1 hunks)
  • backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (2 hunks)
💤 Files with no reviewable changes (1)
  • backend/src/main/java/endolphin/backend/domain/user/UserController.java
🚧 Files skipped from review as they are similar to previous changes (10)
  • backend/src/main/java/endolphin/backend/global/google/dto/GoogleTokens.java
  • backend/src/main/java/endolphin/backend/domain/auth/dto/OAuthResponse.java
  • backend/src/main/java/endolphin/backend/domain/auth/dto/UrlResponse.java
  • backend/src/main/java/endolphin/backend/domain/user/entity/User.java
  • backend/src/main/java/endolphin/backend/global/error/GlobalExceptionHandler.java
  • backend/src/main/java/endolphin/backend/global/config/RestClientConfig.java
  • backend/src/main/java/endolphin/backend/global/error/exception/OAuthException.java
  • backend/src/main/java/endolphin/backend/global/google/dto/GoogleUserInfo.java
  • backend/src/main/java/endolphin/backend/domain/auth/AuthController.java
  • backend/src/main/java/endolphin/backend/domain/auth/AuthService.java
🔇 Additional comments (3)
backend/src/main/java/endolphin/backend/global/error/exception/ErrorCode.java (1)

21-26: LGTM! Well-structured OAuth error codes.

The OAuth error codes are well-organized with appropriate HTTP status codes and follow a consistent naming pattern.

backend/src/main/java/endolphin/backend/domain/user/UserService.java (2)

34-47: LGTM! Clean implementation of user upsert logic.

The method effectively handles both new user creation and existing user updates, with proper token management.


29-32: 🌐 Web query:

What is the format of a Google OAuth access token?

💡 Result:

Google OAuth access tokens are **opaque strings** in a proprietary format[4][10]. These tokens do not follow a standardized structure like JSON Web Tokens (JWT), and applications cannot directly inspect their contents[4][8]. 

### Key characteristics:
1. **Opaque format**:  
   - Access tokens appear as random strings (e.g., `1/fFAGRNJru1FTz70BzhT3Zg`)[2][10].  
   - They contain no user-accessible claims or metadata[4][8].  

2. **Token inspection**:  
   To retrieve metadata (expiration time, scopes, etc.), send the token to Google’s `tokeninfo` endpoint:  
   ```bash
   curl "https://oauth2.googleapis.com/tokeninfo?access_token=ACCESS_TOKEN"
   ```
   This returns JSON with fields like `expires_in`, `scope`, and `token_type`[4].

3. **JWT exceptions**:  
   - Service accounts use **signed JWTs** to *request* access tokens[1][5], but the resulting access token remains opaque[4].  
   - Errors occur if third-party tools mistakenly treat Google’s opaque access tokens as structured JWTs[8].  

### Technical specifications:
| Property          | Details                                                                 |
|-------------------|-------------------------------------------------------------------------|
| **Max size**      | 2048 bytes[1]                                                          |
| **Token type**    | Always `Bearer`[2]                                                     |
| **Lifetime**      | Typically 1 hour (adjustable for service accounts)[4][7]                |
| **Refresh tokens**| Long-lived (until revoked) and used to obtain new access tokens[2][5] |

For authentication requiring structured tokens, use **Google ID tokens** instead—these are JWTs containing claims like `email` and `sub` (subject identifier)[4].

Citations:

- 1: https://developers.google.com/identity/protocols/oauth2
- 2: https://developers.google.com/identity/protocols/oauth2/web-server?sjid=9134356799554274800-NA
- 3: https://www.googlecloudcommunity.com/gc/Apigee/Does-an-OAuth-access-token-format-generated-by-Apigee-have-a/td-p/9134/jump-to/first-unread-message
- 4: https://cloud.google.com/docs/authentication/token-types
- 5: https://support.google.com/cloud/answer/6158849?hl=en&sjid=3654904411249844528-NA
- 6: https://community.mendix.com/link/space/marketplace/questions/90958
- 7: https://cloud.google.com/apigee/docs/api-platform/security/oauth/access-tokens
- 8: https://community.auth0.com/t/google-login-error-access-token-is-in-jwt-format-implement-opaque-symmetric-encrypted-access-token/106480
- 9: https://developers.google.com/identity/oauth2/web/guides/use-token-model
- 10: https://blog.postman.com/how-to-access-google-apis-using-oauth-in-postman/

Copy link
Collaborator

@efdao efdao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🛠️ BE Backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] 구글 토큰 처리

3 participants