feat: apis,infra google social login (#141) #142
Conversation
minwoo1999
changed the title
WalkthroughGoogle μμ λ‘κ·ΈμΈ κΈ°λ₯μ ꡬνν©λλ€. OAuth ν ν° κ²μ¦, μ¬μ©μ μ 보 μ‘°ν, νμκ°μ /λ‘κ·ΈμΈμ κ°λ₯νκ² νλ 맀λμ , μ λ΅, REST ν΄λΌμ΄μΈνΈ μ»΄ν¬λνΈλ₯Ό μΆκ°νκ³ κΈ°μ‘΄ μΈμ¦ νλ¦μ Google μ§μμ ν΅ν©ν©λλ€. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as ν΄λΌμ΄μΈνΈ (Google SDK)
participant UI as kakao-login.html
participant Ctrl as SocialLoginController
participant Factory as SocialLoginRequest<br/>(Credentials Factory)
participant Strategy as GoogleSignInStrategy
participant Manager as GoogleApiManager
participant GoogleAPI as GoogleApi
participant RestClient as GoogleRestClient
participant GoogleServer as Google OAuth Server
Client->>UI: Google λ‘κ·ΈμΈ λ²νΌ ν΄λ¦
Client->>GoogleServer: Google μΈμ¦ μμ²
GoogleServer->>Client: id_token λ°ν
Client->>UI: onSignIn μ½λ°±
UI->>Ctrl: POST /api/v1/auth/signin<br/>(providerType: GOOGLE, token)
Ctrl->>Factory: SocialLoginRequest.createCredentials()
Factory->>Factory: ProviderType.GOOGLE νλ³
Factory->>Factory: GoogleAuthCredentials(token) μμ±
Factory-->>Ctrl: GoogleAuthCredentials λ°ν
Ctrl->>Strategy: authenticate(GoogleAuthCredentials)
Strategy->>Strategy: validateCredentials()
Strategy->>Manager: getUserInfo(accessToken)
Manager->>GoogleAPI: fetchUserInfo(token, userInfoUrl)
GoogleAPI->>RestClient: getUserInfo(bearerToken, url)
RestClient->>GoogleServer: GET /userinfo<br/>(Authorization: Bearer token)
GoogleServer-->>RestClient: { id, email, picture }
RestClient-->>GoogleAPI: GoogleUserInfo
GoogleAPI-->>Manager: Result<GoogleUserInfo>
alt μ±κ³΅
Manager-->>Strategy: GoogleUserInfo
Strategy->>Strategy: createUserInfo(googleUserInfo)
Strategy-->>Ctrl: UserCreateInfoResponse
Ctrl-->>UI: 200 OK (νμκ°μ
/λ‘κ·ΈμΈ μλ£)
else μ€ν¨ (ν ν° λ¬΄ν¨)
RestClient->>Manager: HttpClientErrorException
Manager->>Manager: μμΈ λ§€ν
Manager-->>Strategy: AuthException<br/>(INVALID_OAUTH_TOKEN)
Strategy-->>Ctrl: AuthException
Ctrl-->>UI: 401 μ€λ₯
end
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~20 minutes μ£Όμ κΉκ² κ²ν ν μμ:
Pre-merge checks and finishing touchesβ Failed checks (1 warning)
β Passed checks (4 passed)
β¨ Finishing touches
π§ͺ Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
π Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
π Files selected for processing (9)
apis/src/main/kotlin/org/yapp/apis/auth/dto/request/SocialLoginRequest.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/auth/manager/GoogleApiManager.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/GoogleAuthCredentials.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/GoogleSignInStrategy.kt(1 hunks)apis/src/main/resources/application.yml(1 hunks)apis/src/main/resources/static/kakao-login.html(3 hunks)domain/src/main/kotlin/org/yapp/domain/user/ProviderType.kt(1 hunks)infra/build.gradle.kts(1 hunks)infra/src/main/kotlin/org/yapp/infra/external/oauth/google/response/GoogleUserInfo.kt(1 hunks)
π§° Additional context used
πͺ detekt (1.23.8)
apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/GoogleSignInStrategy.kt
[warning] 27-27: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 27-27: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-validation
π Additional comments (5)
apis/src/main/resources/static/kakao-login.html (1)
56-95: Google ν/λ²νΌ UI ꡬ쑰λ μΌκ΄μ± μκ² μ μΆκ°λ¨Kakao/Apple κ³Ό λμΌν ν¨ν΄μΌλ‘ νκ³Ό λ²νΌμ μΆκ°ν΄μ UX/κ΅¬μ‘°κ° μμ°μ€λ½μ΅λλ€.
λ€λ§<meta name="google-signin-client_id">μcontentκ°μ΄ μ€μ νκ²½(λ‘컬/μ€ν μ΄μ§/μ΄μ)μμ μ΄λ»κ² μ£Όμ λλμ§(μ μ νμΌ μμ , ν νλ¦Ώ μΉν λ±)λ§ λͺ νν ν΄ λλ©΄ μ€μ λλ½μΌλ‘ μΈν λ‘κ·ΈμΈ μ€ν¨λ₯Ό μ€μΌ μ μμ κ² κ°μ΅λλ€.Also applies to: 99-99
domain/src/main/kotlin/org/yapp/domain/user/ProviderType.kt (1)
7-7: ProviderType.GOOGLE μΆκ° λ°©ν₯μ μ μ β λ€λ₯Έ λΆκΈ°λ¬Έμ exhaustiveness λ§ νμΈ
ProviderTypeμ GOOGLE μ μΆκ°ν κ²μ λλ©μΈ λͺ¨λΈ νμ₯ κ΄μ μμ μμ°μ€λ½μ΅λλ€.
λ€λ§ μ΄ enum μwhen (provider)λ‘ λΆκΈ°νλ©΄μelseμμ΄ exhaustive νκ² μ²λ¦¬νλ μ½λκ° λ€λ₯Έ κ³³μ μλ€λ©΄(μ: λ‘κ·ΈμΈ μ λ΅ λΌμ°ν , ν΅κ³ μ§κ³ λ±), GOOGLE μΌμ΄μ€κ° λλ½λμ§ μμλμ§ ν λ²λ§ μ 체 κ²μμΌλ‘ μ κ²ν΄ μ£ΌμΈμ.infra/src/main/kotlin/org/yapp/infra/external/oauth/google/response/GoogleUserInfo.kt (1)
5-12: GoogleUserInfo.email μ nullable λ‘ λ μ νμ νλΉνλ©°, μ΄ν μ μ± μ²λ¦¬κ° μ€μν©λλ€Google UserInfo μλ΅μ λμ λ²μλ κ³μ μ€μ μ λ°λΌ email μ΄ λΉ μ§ μ μμ΄μ
String?λ‘ λͺ¨λΈλ§ν κ²μ μ’μ μ νμ λλ€.
λ€λ§ μ΄ν νμκ°μ /λ‘κ·ΈμΈ νλ¦μμ:
- email μ΄
nullμΈ κ²½μ° μ΄λ€ κΈ°μ€μΌλ‘ μ¬μ©μλ₯Ό μλ³ν μ§(μ: provideridκΈ°λ° μλ³ + email μ μ ν νλ),- μλΉμ€ μ μ± μ email μ΄ νμλΌλ©΄,
nullμΌ λ λͺ μμ μΌλ‘AuthExceptionλ±μ λμ Έ "μ΄λ©μΌ μ 곡μ λμν΄ λ¬λΌ"λ μμ μλ¬λ₯Ό μ£Όλμ§,λ₯Ό λͺ νν μ λ¦¬ν΄ λλ κ² μ€μν©λλ€. μ§κΈ DTO λ 벨μμ nullable λ‘λ§ λλμ§ μλλ‘, SignInStrategy λ¨μ μ²λ¦¬ λ‘μ§κΉμ§ ν λ² λ μ κ²ν΄ μ£ΌμΈμ.
apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/GoogleSignInStrategy.kt (2)
13-20: LGTM! ν΄λμ€ κ΅¬μ‘° λ° μμ‘΄μ± μ£Όμ μ΄ μ¬λ°λ¦ λλ€.Spring μ»΄ν¬λνΈλ‘ μ μ ν ꡬμ±λμκ³ ,
SignInStrategyμΈν°νμ΄μ€ ꡬνμ΄ μ νν©λλ€. GoogleApiManager μμ‘΄μ± μ£Όμ λ° λ‘κ±° μ΄κΈ°νλ νμ€ ν¨ν΄μ λ°λ₯΄κ³ μμ΅λλ€.
36-42: LGTM! νμ μμ μ±μ΄ 보μ₯λ μ격 μ¦λͺ κ²μ¦μ λλ€.μμ ν μΊμ€ν (
as?)κ³Ό μλΉμ€ μ°μ°μλ₯Ό μ¬μ©νμ¬ μλͺ»λ μ격 μ¦λͺ νμ μ μ μ ν μ²λ¦¬νκ³ μμ΅λλ€. λͺ νν μλ¬ λ©μμ§λ λλ²κΉ μ λμμ΄ λ©λλ€.
| import org.yapp.apis.auth.exception.AuthErrorCode | ||
| import org.yapp.apis.auth.exception.AuthException | ||
| import org.yapp.apis.auth.strategy.signin.AppleAuthCredentials | ||
| import org.yapp.apis.auth.strategy.signin.GoogleAuthCredentials |
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
Google λΆκΈ° λ‘μ§μ κΉλνμ§λ§, ν ν° μλ―Έ(id_token vs access_token)λ₯Ό λͺ νν ν΄ λλ κ²μ΄ μ’μ΅λλ€
ProviderType.GOOGLE -> GoogleAuthCredentials(request.validOauthToken()) λ‘ Kakao μ λμΌν ν¨ν΄μΌλ‘ λΆμΈ μ μ μ½κΈ° μ’μ΅λλ€.
λ€λ§ νλ‘ νΈμμ νμ¬ id_token μ 보λ΄λ ꡬ쑰λΌλ©΄, μ¬κΈ°μ μμ±λλ GoogleAuthCredentials.accessToken μ μ€μ λ‘λ id_token μ κ°λ¦¬ν€κ² λκ³ , μ΄ν GoogleApiManager μμλ μ΄λ₯Ό UserInfo νΈμΆμ© access token μΌλ‘ μ¬μ©νκ² λ©λλ€.
μ/λ€λ¨ λͺ¨λμμ μ΄λ€ μ’ λ₯μ ν ν°μ μ£Όκ³ λ°λμ§(access_token / id_token)λ₯Ό μ£Όμμ΄λ λ¬Έμ, νΉμ νλλͺ λ³κ²½μΌλ‘ λͺ νν ν΄ λλ©΄, μ΄ν μ μ§λ³΄μ μ μ€ν΄λ λ²κ·Έλ₯Ό μ€μΌ μ μμ κ² κ°μ΅λλ€.
Also applies to: 55-67
π€ Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/auth/dto/request/SocialLoginRequest.kt
around lines 8 and 55-67, the code uses
GoogleAuthCredentials(request.validOauthToken()) without clarifying whether the
incoming token is an id_token or an access_token; update the DTO and usage to
make the token type explicit: annotate the SocialLoginRequest field with a
comment stating which token is expected, or better rename the parameter/field to
idToken if the frontend sends id_token, add a matching constructor or factory
for GoogleAuthCredentials that accepts an idToken (or separate properties
idToken/accessToken), and update GoogleApiManager calls to use the correct token
property (idToken for verification / JWT parsing, accessToken for userinfo
calls) so the intent is clear and prevents future misuse.
| data class GoogleAuthCredentials( | ||
| val accessToken: String, | ||
| ) : SignInCredentials() { | ||
| override fun getProviderType(): ProviderType { | ||
| return ProviderType.GOOGLE | ||
| } | ||
| } |
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
accessToken νλλͺ
μ΄ μ€μ Google ν ν° μ’
λ₯μ μΌμΉνλμ§ μ κ² νμ
GoogleAuthCredentials κ° ProviderType.GOOGLE μ λ°ννλ ꡬ쑰λ λͺ
ννκ³ μ’μ΅λλ€.
λ€λ§ νμ¬ νλ¦μ νλ‘ νΈμμ id_token μ μ λ¬νλ€λ©΄, μ΄ νλλͺ
(accessToken) μ΄ μ€μ κ°κ³Ό λ§μ§ μκ² λ©λλ€. λμ€μ Google μͺ½μμ access_token κ³Ό id_token μ λͺ¨λ μ¬μ©νλ μꡬμ¬νμ΄ μκΈ°λ©΄ νΌλ μ¬μ§κ° ν¬λ,
- μ€μ λ‘ access_token μ λ°μ κ³νμ΄λΌλ©΄ νλ‘ νΈ/λ°±μλ λͺ¨λ access_token μΌλ‘ μ λ ¬νκ±°λ,
- νμ¬ κ°μ΄ id_token μ΄λΌλ©΄ νλλͺ μ λ°κΎΈκ±°λ KDoc μΌλ‘ λͺ μ
νλ λ°©ν₯μ ν λ² κ²ν ν΄ λ³΄μλ κ²μ μΆμ²λ립λλ€.
| override fun authenticate(credentials: SignInCredentials): UserCreateInfoResponse { | ||
| return try { | ||
| val googleCredentials = validateCredentials(credentials) | ||
| val googleUser = googleApiManager.getUserInfo(googleCredentials.accessToken) | ||
| createUserInfo(googleUser) | ||
| } catch (exception: Exception) { | ||
| log.error("Google authentication failed", exception) | ||
| when (exception) { | ||
| is AuthException -> throw exception | ||
| else -> throw AuthException(AuthErrorCode.FAILED_TO_GET_USER_INFO, exception.message) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
μμΈ μ²λ¦¬ λ²μ κ²ν κΆμ₯
νμ¬ Exceptionμ catchνμ¬ λͺ¨λ μμΈλ₯Ό μ²λ¦¬νκ³ μμ΅λλ€. λ‘κΉ
κ³Ό μ¬throw λ‘μ§μ μ μ νμ§λ§, Google API νΈμΆ μ λ°μ κ°λ₯ν ꡬ체μ μΈ μμΈ νμ
(μ: HTTP ν΄λΌμ΄μΈνΈ μμΈ, λ€νΈμν¬ μμΈ λ±)μ catchνλ©΄ λ λͺ
νν μλ¬ νΈλ€λ§μ΄ κ°λ₯ν©λλ€.
νμ¬ κ΅¬νλ λ°©μ΄μ μ΄κ³ μ λ΅ ν¨ν΄ 컨ν μ€νΈμμ μμ© κ°λ₯νμ§λ§, νμμ ꡬ체μ μΈ μμΈ νμ μΌλ‘ μΈλΆννλ κ²μ κ³ λ €ν΄λ³΄μΈμ.
μ μ λΆμ λꡬμ κ²½κ³ λ₯Ό μ°Έκ³ νμ΅λλ€.
π§° Tools
πͺ detekt (1.23.8)
[warning] 27-27: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 27-27: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
π€ Prompt for AI Agents
In
apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/GoogleSignInStrategy.kt
around lines 22 to 34, the current code catches the broad Exception type; narrow
the exception handling by catching specific exceptions (e.g., AuthException
first, HTTP client exceptions such as HttpClientRequestException or Ktor's
ClientRequestException/ServerResponseException, IO/SocketTimeoutException, and
other network-related exceptions) and handle each appropriately: log the error,
rethrow AuthException as-is, and wrap other known exception types into
AuthException with the proper AuthErrorCode and original exception as cause;
keep a final generic catch only if necessary to preserve behavior but prefer
explicit catches to satisfy static analysis and provide clearer error semantics.
| private fun createUserInfo(googleUser: GoogleUserInfo): UserCreateInfoResponse { | ||
| return UserCreateInfoResponse.of( | ||
| email = googleUser.email ?: ("google_${googleUser.id}@google.com"), | ||
| nickname = NicknameGenerator.generate(), | ||
| profileImageUrl = googleUser.picture, | ||
| providerType = ProviderType.GOOGLE, | ||
| providerId = googleUser.id | ||
| ) | ||
| } |
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
𧩠Analysis chain
π Script executed:
find . -type f -name "GoogleSignInStrategy.kt"Repository: YAPP-Github/Reed-Server
Length of output: 148
π Script executed:
git ls-files | grep -i google | head -20Repository: YAPP-Github/Reed-Server
Length of output: 381
π Script executed:
git ls-files | grep -i auth | head -30Repository: YAPP-Github/Reed-Server
Length of output: 2278
π Script executed:
cat apis/src/main/kotlin/org/yapp/apis/auth/dto/response/UserCreateInfoResponse.ktRepository: YAPP-Github/Reed-Server
Length of output: 1651
π Script executed:
cat infra/src/main/kotlin/org/yapp/infra/external/oauth/google/response/GoogleUserInfo.ktRepository: YAPP-Github/Reed-Server
Length of output: 354
π Script executed:
cat apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/GoogleSignInStrategy.ktRepository: YAPP-Github/Reed-Server
Length of output: 2142
π Script executed:
cat apis/src/main/kotlin/org/yapp/apis/auth/service/UserSignInService.ktRepository: YAPP-Github/Reed-Server
Length of output: 1272
π Script executed:
rg "email" --type kt apis/src/main/kotlin/org/yapp/apis/auth/ -B 2 -A 2 | head -80Repository: YAPP-Github/Reed-Server
Length of output: 93
π Script executed:
cat apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/AppleSignInStrategy.ktRepository: YAPP-Github/Reed-Server
Length of output: 1852
π Script executed:
cat apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/KakaoSignInStrategy.ktRepository: YAPP-Github/Reed-Server
Length of output: 2136
π Script executed:
rg "FindOrCreateUserRequest" --type kotlin -A 20Repository: YAPP-Github/Reed-Server
Length of output: 16922
π Script executed:
rg "validate.*email|email.*validate" --type kotlin -i -B 2 -A 5Repository: YAPP-Github/Reed-Server
Length of output: 1144
π Script executed:
rg "sendEmail|send.*mail|mailService" --type kotlin -iRepository: YAPP-Github/Reed-Server
Length of output: 49
π Script executed:
rg "class User" --type kotlin | grep -v "Response\|Request\|Info\|Strategy"Repository: YAPP-Github/Reed-Server
Length of output: 2816
π Script executed:
git log --oneline --all -- apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/GoogleSignInStrategy.kt | head -5Repository: YAPP-Github/Reed-Server
Length of output: 129
π Script executed:
rg "email" --type kotlin apis/src/main/kotlin/org/yapp/apis/user/ -B 2 -A 2 | head -100Repository: YAPP-Github/Reed-Server
Length of output: 4728
π Script executed:
cat apis/src/main/kotlin/org/yapp/apis/user/dto/request/FindOrCreateUserRequest.ktRepository: YAPP-Github/Reed-Server
Length of output: 2399
π Script executed:
cat domain/src/main/kotlin/org/yapp/domain/user/User.ktRepository: YAPP-Github/Reed-Server
Length of output: 5536
π Script executed:
find . -type f -name "*Test.kt" | grep -i auth | head -10Repository: YAPP-Github/Reed-Server
Length of output: 49
π Script executed:
rg "getOrDefaultEmail" --type kotlin -B 5 -A 5Repository: YAPP-Github/Reed-Server
Length of output: 2616
π Script executed:
rg "EmailValidator" --type kotlin -B 3 -A 10Repository: YAPP-Github/Reed-Server
Length of output: 3437
π Script executed:
find . -type f \( -name "*Test.kt" -o -name "*Tests.kt" \) -exec grep -l "email\|signin\|sign-in" {} \; | head -10Repository: YAPP-Github/Reed-Server
Length of output: 49
π Script executed:
rg "isValidEmail" --type kotlin -B 5 -A 15Repository: YAPP-Github/Reed-Server
Length of output: 2687
π Script executed:
rg "EMAIL_PATTERN" --type kotlin -B 2 -A 2Repository: YAPP-Github/Reed-Server
Length of output: 1070
π Script executed:
find . -type f -name "RegexUtils.kt"Repository: YAPP-Github/Reed-Server
Length of output: 137
π Script executed:
cat global-utils/src/main/kotlin/org/yapp/globalutils/util/RegexUtils.ktRepository: YAPP-Github/Reed-Server
Length of output: 423
π Script executed:
rg "providerId.*unique\|unique.*providerId" --type kotlin -iRepository: YAPP-Github/Reed-Server
Length of output: 49
π Script executed:
rg "findUserByProviderTypeAndProviderId" --type kotlin -B 3 -A 10Repository: YAPP-Github/Reed-Server
Length of output: 4431
π Script executed:
cat apis/src/main/kotlin/org/yapp/apis/auth/exception/AuthErrorCode.kt | grep -i "email\|user"Repository: YAPP-Github/Reed-Server
Length of output: 413
Email fallback handling is working as designed; consider documenting the approach.
The synthetic email fallback is intentional and will not cause validation failures. The EmailValidator only performs regex validation (allows underscores and dots), with no MX record checks. Emails like google_123456@google.com pass validation successfully.
However, there are design consistency issues worth addressing:
-
Redundant fallback logic: GoogleSignInStrategy creates
google_<id>@google.com, butFindOrCreateUserRequest.getOrDefaultEmail()independently creates<id>@google.localif email is null. This layering is unnecessary. -
Inconsistent formats: Google/Kakao use
provider_<id>@provider.com, whileFindOrCreateUserRequestuses<id>@provider.local. The.localconvention is typically for local networks, not production systems. -
Design alignment: AppleSignInStrategy accepts nullable emails directly, while Google/Kakao generate fallbacksβthis variation should be intentional and documented.
The core design (allowing nullable emails with providerId as the true identifier) is sound. Consider:
- Centralizing email fallback logic (remove from GoogleSignInStrategy, keep only in FindOrCreateUserRequest)
- Using consistent fallback format across all providers
- Adding a flag to identify synthetic emails (useful if email features are added later)
- Documenting that providerId, not email, is the primary user identifier
π€ Prompt for AI Agents
In
apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/GoogleSignInStrategy.kt
around lines 44-52, remove the synthetic email fallback so createUserInfo
returns the raw googleUser.email (nullable) rather than generating
"google_<id>@google.com"; centralize fallback logic into
FindOrCreateUserRequest.getOrDefaultEmail() so all providers share one
consistent format, pick and apply a single fallback pattern (e.g.
provider_<id>@provider.local or provider_<id>@provider.com) in that central
method, add a boolean flag (e.g. isSyntheticEmail) to the request/response
objects and set it when the central fallback is used, and update related
callers, tests and documentation to reflect that providerId is the primary
identifier and synthetic email behavior is centralized and consistent.
|
|
||
| implementation(Dependencies.Spring.BOOT_STARTER_OAUTH2_CLIENT) | ||
|
|
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
Spring OAuth2 Client μμ‘΄μ± μ¬μ© μ¬λΆ νμΈ μ μ
νμ¬ PR μμ λ³Έ μ½λλ§ λ³΄λ©΄ Google μ°λμ RestClient λ‘ μ§μ UserInfo μλν¬μΈνΈλ₯Ό νΈμΆνκ³ μμ΄, infra λͺ¨λμ΄ BOOT_STARTER_OAUTH2_CLIENT μ κΈ°λ₯μ μ€μ λ‘ μ°μ§ μλ κ²μ²λΌ 보μ
λλ€.
ν₯ν Security OAuth2 Client λ₯Ό μ¬μ©ν κ³νμ΄ μλ€λ©΄, μ΄ μμ‘΄μ±μ μ κ±°ν΄μ ν΄λμ€ν¨μ€μ λΉλ μκ°μ μ€μ΄λ κ²λ κ³ λ €ν΄ μ£ΌμΈμ.
|
@minwoo1999 CIκ° κΉ¨μ Έμ νλ² λ΄μ£Όμκ³ , major μμ£Όμ 리뷰λ λ°μνλ©΄ μ’μ κ² κ°μ΅λλ€! |
| @Component | ||
| class GoogleApiManager( | ||
| @Value("\${oauth.google.url.user-info}") private val userInfoUrl: String, | ||
| private val restClient: RestClient, |
There was a problem hiding this comment.
restClient νΈμΆμ μΈλΆ api νΈμΆμ΄κΈ°μ infra λͺ¨λμμ μννλκ² μ’μ κ² κ°μμ!
μμ 첨λΆν©λλ€!
class KakaoApiManager(
private val kakaoApi: KakaoApi,
private val kakaoOauthProperties: KakaoOauthProperties
) {| data class GoogleAuthCredentials( | ||
| val accessToken: String, | ||
| ) : SignInCredentials() { | ||
| override fun getProviderType(): ProviderType { | ||
| return ProviderType.GOOGLE | ||
| } | ||
| } |
There was a problem hiding this comment.
μ΄κ±° SignInCredentials sealed class μλμ λ£μΌλ©΄ μ’μ κ² κ°μμ!
- ν μ€νΈ νλ‘νμ oauth μ€μ μΆκ° - λ‘컬μμ sonar task μ€ν΅λλλ‘ μμ
There was a problem hiding this comment.
Actionable comments posted: 6
β»οΈ Duplicate comments (1)
apis/src/main/kotlin/org/yapp/apis/auth/manager/GoogleApiManager.kt (1)
19-39: μλ¬ νΈλ€λ§μ΄ κ°μ λμμ΅λλ€.μ΄μ 리뷰μμ μ§μ λ μλ¬ νΈλ€λ§ λ¬Έμ κ° κ°μ λμμ΅λλ€:
Resultνμ μ νμ©ν λͺ μμ μλ¬ μ²λ¦¬- HTTP μλ¬μ μΌλ° μμΈλ₯Ό ꡬλΆνμ¬ μ μ ν
AuthErrorCodeλ‘ λ§€ν- μ±κ³΅/μ€ν¨ μ λ‘κΉ μΆκ°
π Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
π Files selected for processing (6)
apis/src/main/kotlin/org/yapp/apis/auth/manager/GoogleApiManager.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/SignInCredentials.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/config/GoogleOauthProperties.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/config/PropertiesConfig.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/external/oauth/google/GoogleApi.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/external/oauth/google/GoogleRestClient.kt(1 hunks)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-validation
π Additional comments (5)
apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/SignInCredentials.kt (1)
10-10: LGTM - νΈλ μΌλ§ μ½€λ§ μΆκ° μ λμμ΅λλ€.Kotlin 컨벀μ μ λ°λ₯΄λ μ’μ λ³κ²½μ¬νμ λλ€. ν₯ν νλΌλ―Έν° μΆκ° μ diffκ° κΉλν΄μ§λλ€.
Also applies to: 17-17
apis/src/main/kotlin/org/yapp/apis/config/PropertiesConfig.kt (1)
6-8: ꡬνμ΄ μ νν©λλ€.Spring Bootμ νμ€ μ€μ νλ‘νΌν° λ°μΈλ© ν¨ν΄μ μ¬λ°λ₯΄κ² λ°λ₯΄κ³ μμ΅λλ€.
infra/src/main/kotlin/org/yapp/infra/external/oauth/google/GoogleApi.kt (1)
10-12: μμ μ μκ° μ μ ν©λλ€.Bearer ν ν° ν리ν½μ€ ν¬λ§·μ΄ μ¬λ°λ₯΄λ©°, private μ κ·Ό μ μ΄μλ‘ μΊ‘μνλμ΄ μμ΅λλ€.
apis/src/main/kotlin/org/yapp/apis/config/GoogleOauthProperties.kt (1)
5-8: νλ‘νΌν° κ΅¬μ‘°κ° λͺ νν©λλ€.ConfigurationProperties μ΄λ Έν μ΄μ κ³Ό ν리ν½μ€ μ€μ μ΄ μ¬λ°λ₯΄κ² ꡬμ±λμ΄ μμ΅λλ€.
apis/src/main/kotlin/org/yapp/apis/auth/manager/GoogleApiManager.kt (1)
12-17: μμ‘΄μ± κ΅¬μ‘°κ° κ°μ λμμ΅λλ€.μ΄μ 리뷰 νΌλλ°±μ λ°μνμ¬ infra λͺ¨λμ
GoogleApiλ₯Ό μ¬μ©νλλ‘ κ΅¬μ‘°κ° κ°μ λμμ΅λλ€.
| ) { | ||
| private val log = KotlinLogging.logger {} | ||
|
|
||
| fun getUserInfo(accessToken: String): GoogleUserInfo { |
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
νλΌλ―Έν°μ λν λ¬Έμνλ₯Ό μΆκ°νλ κ²μ κ³ λ €νμΈμ.
μ΄μ 리뷰μμ μ κΈ°λ access_token vs id_token ꡬλΆμ λν λͺ
νμ± λ¬Έμ μ κ΄λ ¨νμ¬, νλΌλ―Έν° μ΄λ¦μ λͺ
ννμ§λ§ KDoc μ£Όμμ μΆκ°νλ©΄ λ μ’μ΅λλ€.
λ€μκ³Ό κ°μ΄ λ¬Έμνλ₯Ό μΆκ°ν μ μμ΅λλ€:
+/**
+ * Google OAuth access tokenμ μ¬μ©νμ¬ μ¬μ©μ μ 보λ₯Ό μ‘°νν©λλ€.
+ * @param accessToken Google OAuth 2.0 access token (id_tokenμ΄ μλ)
+ * @return μ‘°νλ Google μ¬μ©μ μ 보
+ * @throws AuthException ν ν°μ΄ μ ν¨νμ§ μκ±°λ Google μλ²μ ν΅μ μ€ν¨ μ
+ */
fun getUserInfo(accessToken: String): GoogleUserInfo {π€ Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/auth/manager/GoogleApiManager.kt around
line 19, the public function parameter lacks KDoc explaining whether the token
is an OAuth access_token or an id_token and what format/value is expected; add a
KDoc block for the function that documents the purpose of the function,
clarifies that the parameter accessToken is an OAuth access_token (not an
id_token), describes expected format/expiration and any required scopes, and
briefly states what the returned GoogleUserInfo contains and possible
exceptions/errors thrown.
| override fun getProviderType(): ProviderType { | ||
| return ProviderType.GOOGLE | ||
| } |
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
ννμ λ°λλ‘ ν΅μΌνλ κ²μ κΆμ₯ν©λλ€.
getProviderType() λ©μλκ° λμΌ νμΌ λ΄ λ€λ₯Έ ꡬν체(KakaoAuthCredentials, AppleAuthCredentials)μμλ ννμ λ°λ(= ProviderType.XXX)λ₯Ό μ¬μ©νλ λ°λ©΄, μ¬κΈ°μλ§ λΈλ‘ λ°λλ₯Ό μ¬μ©νκ³ μμ΄ μ€νμΌ μΌκ΄μ±μ΄ λ¨μ΄μ§λλ€.
λ€μ diffλ₯Ό μ μ©νμ¬ μΌκ΄μ±μ κ°μ ν μ μμ΅λλ€:
- override fun getProviderType(): ProviderType {
- return ProviderType.GOOGLE
- }
+ override fun getProviderType(): ProviderType = ProviderType.GOOGLEπ Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override fun getProviderType(): ProviderType { | |
| return ProviderType.GOOGLE | |
| } | |
| override fun getProviderType(): ProviderType = ProviderType.GOOGLE |
π€ Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/auth/strategy/signin/SignInCredentials.kt
around lines 25 to 27, the getProviderType() method uses a block body while
other credential implementations (KakaoAuthCredentials, AppleAuthCredentials)
use expression bodies; change the method from a block body to an expression body
so it reads as a single-line expression returning ProviderType.GOOGLE to match
project style and maintain consistency.
| data class Url( | ||
| val userInfo: String | ||
| ) |
There was a problem hiding this comment.
Url ν΄λμ€λ₯Ό GoogleOauthProperties λ΄λΆλ‘ μ€μ²©μν€μΈμ.
νμ¬ Url ν΄λμ€κ° ν¨ν€μ§ λ 벨μ μ μλμ΄ μμ΄ λ€μ λ¬Έμ κ° λ°μν©λλ€:
- λ²μ©μ μΈ μ΄λ¦μΌλ‘ μΈν λ€μμ€νμ΄μ€ μΆ©λ μν
GoogleOauthPropertiesμμ κ΄κ³κ° λͺ μμ μ΄μ§ μμ- μ½λ ꡬ쑰μ κ°λ μ± μ ν
Url ν΄λμ€λ₯Ό GoogleOauthProperties λ΄λΆ ν΄λμ€λ‘ μ€μ²©νμ¬ κ΄κ³λ₯Ό λͺ
νν νκ³ λ€μμ€νμ΄μ€λ₯Ό μ 리νμΈμ.
λ€μκ³Ό κ°μ΄ μμ ν μ μμ΅λλ€:
@ConfigurationProperties(prefix = "oauth.google")
data class GoogleOauthProperties(
val url: Url
-)
+) {
+ data class Url(
+ val userInfo: String
+ )
+}
-
-data class Url(
- val userInfo: String
-)π€ Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/config/GoogleOauthProperties.kt around
lines 10 to 12, the top-level data class Url should be moved inside the
GoogleOauthProperties class to avoid namespace collisions and clarify the
relationship; edit GoogleOauthProperties to declare Url as a nested (or inner if
it needs access to outer instance) data class (e.g., data class Url(val
userInfo: String)) placed within the GoogleOauthProperties body, remove the
top-level Url declaration, and update any references/imports elsewhere to use
GoogleOauthProperties.Url (or GoogleOauthProperties().Url if made inner)
accordingly.
| fun fetchUserInfo( | ||
| accessToken: String, | ||
| userInfoUrl: String, | ||
| ): Result<GoogleUserInfo> { | ||
| return runCatching { | ||
| googleRestClient.getUserInfo(BEARER_PREFIX + accessToken, userInfoUrl) | ||
| } | ||
| } |
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
μ‘μΈμ€ ν ν° κ²μ¦μ μΆκ°νμΈμ.
accessTokenμ BEARER_PREFIXμ μ§μ μ°κ²°νκΈ° μ μ λ€μμ νμΈν΄μΌ ν©λλ€:
- ν ν°μ΄ μ΄λ―Έ "Bearer " ν리ν½μ€λ₯Ό ν¬ν¨νκ³ μμ§ μμμ§
- ν ν°μ΄ λΉμ΄μμ§ μμμ§
μλͺ»λ Authorization ν€λλ‘ μΈν API νΈμΆ μ€ν¨λ₯Ό λ°©μ§ν μ μμ΅λλ€.
λ€μκ³Ό κ°μ΄ κ²μ¦ λ‘μ§μ μΆκ°ν μ μμ΅λλ€:
fun fetchUserInfo(
accessToken: String,
userInfoUrl: String,
): Result<GoogleUserInfo> {
+ require(accessToken.isNotBlank()) { "Access token must not be blank" }
+ val token = if (accessToken.startsWith("Bearer ", ignoreCase = true)) {
+ accessToken
+ } else {
+ BEARER_PREFIX + accessToken
+ }
return runCatching {
- googleRestClient.getUserInfo(BEARER_PREFIX + accessToken, userInfoUrl)
+ googleRestClient.getUserInfo(token, userInfoUrl)
}
}π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun fetchUserInfo( | |
| accessToken: String, | |
| userInfoUrl: String, | |
| ): Result<GoogleUserInfo> { | |
| return runCatching { | |
| googleRestClient.getUserInfo(BEARER_PREFIX + accessToken, userInfoUrl) | |
| } | |
| } | |
| fun fetchUserInfo( | |
| accessToken: String, | |
| userInfoUrl: String, | |
| ): Result<GoogleUserInfo> { | |
| require(accessToken.isNotBlank()) { "Access token must not be blank" } | |
| val token = if (accessToken.startsWith("Bearer ", ignoreCase = true)) { | |
| accessToken | |
| } else { | |
| BEARER_PREFIX + accessToken | |
| } | |
| return runCatching { | |
| googleRestClient.getUserInfo(token, userInfoUrl) | |
| } | |
| } |
π€ Prompt for AI Agents
In infra/src/main/kotlin/org/yapp/infra/external/oauth/google/GoogleApi.kt
around lines 14 to 21, add validation and normalization for accessToken before
concatenating with BEARER_PREFIX: first trim and ensure it's not blank (return a
failed Result or throw an IllegalArgumentException inside runCatching if blank),
then if it already starts with "Bearer " (case-insensitive) use it as-is,
otherwise prepend BEARER_PREFIX; finally pass the normalized authorization
header into googleRestClient.getUserInfo so we avoid empty or double-prefixed
Authorization headers.
| @Component | ||
| class GoogleRestClient( | ||
| builder: RestClient.Builder | ||
| ) { | ||
| private val client = builder.build() |
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
νμμμ μ€μ μ μΆκ°νμΈμ.
μΈλΆ API νΈμΆ μ νμμμμ΄ μ€μ λμ΄ μμ§ μμΌλ©΄ μλ΅μ΄ μλ κ²½μ° λ¬΄νμ λκΈ°ν μ μμ΅λλ€. μ°κ²° νμμμκ³Ό μ½κΈ° νμμμμ μ€μ νμ¬ μμ€ν μμ μ±μ ν₯μμν€μΈμ.
λ€μκ³Ό κ°μ΄ νμμμμ μ€μ ν μ μμ΅λλ€:
+import java.time.Duration
+
@Component
class GoogleRestClient(
builder: RestClient.Builder
) {
- private val client = builder.build()
+ private val client = builder
+ .requestFactory(
+ org.springframework.http.client.SimpleClientHttpRequestFactory().apply {
+ setConnectTimeout(Duration.ofSeconds(5))
+ setReadTimeout(Duration.ofSeconds(10))
+ }
+ )
+ .build()Committable suggestion skipped: line range outside the PR's diff.
π€ Prompt for AI Agents
In
infra/src/main/kotlin/org/yapp/infra/external/oauth/google/GoogleRestClient.kt
around lines 7 to 11, the RestClient is built without any timeout configuration;
update the builder to set sensible connection and read (socket) timeouts before
calling build (for example short connect timeout and a longer read timeout),
using the RestClient.Builder timeout methods available in your HTTP client
(e.g., setConnectTimeout / setReadTimeout or the Duration-based equivalents), so
the client fails fast on network issues and avoids indefinite waits.
| fun getUserInfo( | ||
| bearerToken: String, | ||
| url: String, | ||
| ): GoogleUserInfo { | ||
| return client.get() | ||
| .uri(url) | ||
| .header("Authorization", bearerToken) | ||
| .retrieve() | ||
| .body(GoogleUserInfo::class.java) | ||
| ?: throw IllegalStateException("Google API μλ΅μ΄ null μ λλ€.") | ||
| } |
There was a problem hiding this comment.
π§Ή Nitpick | π΅ Trivial
μμΈ λ©μμ§λ₯Ό μλ¬ΈμΌλ‘ μμ±νκ±°λ μλ¬ μ½λλ₯Ό μ¬μ©νμΈμ.
Line 22μ IllegalStateException λ©μμ§κ° νκΈλ‘ μμ±λμ΄ μμ΅λλ€. μΈνλΌ λ μ΄μ΄μ μμΈ λ©μμ§λ μλ¬ΈμΌλ‘ μμ±νκ±°λ, μμ λ μ΄μ΄μμ μ²λ¦¬ν μ μλλ‘ λλ©μΈλ³ μμΈλ₯Ό μ¬μ©νλ κ²μ΄ μ’μ΅λλ€.
λ€μκ³Ό κ°μ΄ μμ ν μ μμ΅λλ€:
return client.get()
.uri(url)
.header("Authorization", bearerToken)
.retrieve()
.body(GoogleUserInfo::class.java)
- ?: throw IllegalStateException("Google API μλ΅μ΄ null μ
λλ€.")
+ ?: throw IllegalStateException("Google API response body is null")
}π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun getUserInfo( | |
| bearerToken: String, | |
| url: String, | |
| ): GoogleUserInfo { | |
| return client.get() | |
| .uri(url) | |
| .header("Authorization", bearerToken) | |
| .retrieve() | |
| .body(GoogleUserInfo::class.java) | |
| ?: throw IllegalStateException("Google API μλ΅μ΄ null μ λλ€.") | |
| } | |
| fun getUserInfo( | |
| bearerToken: String, | |
| url: String, | |
| ): GoogleUserInfo { | |
| return client.get() | |
| .uri(url) | |
| .header("Authorization", bearerToken) | |
| .retrieve() | |
| .body(GoogleUserInfo::class.java) | |
| ?: throw IllegalStateException("Google API response body is null") | |
| } |
π€ Prompt for AI Agents
In
infra/src/main/kotlin/org/yapp/infra/external/oauth/google/GoogleRestClient.kt
around lines 13 to 23, replace the Korean Literal in the thrown
IllegalStateException with an English message or throw a domain-specific
exception so infra-layer errors are consistent and can be handled by upper
layers; update the throw to something like IllegalStateException("Google API
response is null.") or define and throw a specific exception (e.g.,
GoogleApiException("response is null")) and ensure callers can handle the new
exception type.
|
π κ΄λ ¨ μ΄μ
Close [BOOK-469/feat] google social login μ ꡬνν©λλ€.Β #141
π μμ μ ν
β¨ Feature (κΈ°λ₯ μΆκ°)
π Bugfix (λ²κ·Έ μμ )
π§ Refactor (μ½λ 리ν©ν λ§)
βοΈ Chore (νκ²½ μ€μ )
π Docs (λ¬Έμ μμ± λ° μμ )
β Test (κΈ°λ₯ ν μ€νΈ)
π¨ style (μ½λ μ€νμΌ μμ )
π μμ λ΄μ
Google OAuth κΈ°λ° λ‘κ·ΈμΈ κΈ°λ₯ μΆκ°
GoogleAuthCredentials,GoogleSignInStrategyμ κ· κ΅¬νGoogle UserInfo μ‘°νμ©
GoogleApiManagerμΆκ°ProviderTypeμGOOGLEenum μΆκ°Google UserInfo DTO(
GoogleUserInfo) μ κ· μμ±κ΅¬κΈ λ‘κ·ΈμΈ λ²νΌ λ° ν΄λΌμ΄μΈνΈ μ΄κΈ°ν μ€ν¬λ¦½νΈ μΆκ° (HTML μ λ°μ΄νΈ)
application.ymlμ Google UserInfo API URL μ€μ μΆκ°OAuth2 Client μμ‘΄μ± μΆκ°
π§ͺ ν μ€νΈ λ΄μ
λΈλΌμ°μ μμ Google λ‘κ·ΈμΈ μ μ λμ νμΈ
Google OAuth ν ν° β μλ² μμ² β μ¬μ©μ μ 보 μ μ μ‘°ν νμΈ
κΈ°μ‘΄ Kakao/Apple λ‘κ·ΈμΈ μν₯ μμ
μΈμ¦ μ€ν¨ λ° μμΈ μ²λ¦¬ λ‘κΉ νμΈ
β PR 체ν¬λ¦¬μ€νΈ
μ»€λ° λ©μμ§κ° λͺ νν©λλ€
PR μ λͺ©μ΄ 컨벀μ μ λ§μ΅λλ€
κ΄λ ¨ μ΄μ λ²νΈλ₯Ό μμ±νμ΅λλ€
κΈ°λ₯μ΄ μ μμ μΌλ‘ μλν©λλ€
λΆνμν μ½λλ₯Ό μ κ±°νμ΅λλ€
π¬ μΆκ° μ€λͺ or 리뷰 ν¬μΈνΈ
Google UserInfo μλ΅μ νλ(
email nullable) μ²λ¦¬μ λν μ견 λΆνλ립λλ€.Summary by CodeRabbit
λ¦΄λ¦¬μ€ λ ΈνΈ
βοΈ Tip: You can customize this high-level summary in your review settings.