feat: 소셜 로그인 연동 #133
feat: 소셜 로그인 연동 #133
Conversation
WalkthroughAdds account-linking to social authentication: extends SocialLoginRequest with Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Controller as Oauth2AuthController
participant Service as UserService
participant Repo as Repositories
Client->>Controller: POST /oauth2/login {idToken, nickname, authToken, accountLinking, linkToken}
Controller->>Service: upsertSocialUser(payload, accountLinking, linkToken)
alt Social account exists
Service->>Repo: findSocialAccount(provider, socialId)
Repo-->>Service: socialAccount -> user
Service-->>Controller: return LoginUser (200)
Controller-->>Client: 200 OK {user}
else Email exists & linking disabled
Service->>Repo: findUserByEmail(email)
Repo-->>Service: user found
Service-->>Controller: throw CoreException(ALREADY_REGISTERED_EMAIL, providerInfo)
Controller-->>Client: 409 Conflict {detail: providerInfo}
else Email exists & linking enabled
Service->>Repo: findUserByEmail(email)
Repo-->>Service: user found
Service->>Repo: save UserSocialAccount (with linkToken)
Repo-->>Service: saved social account
Service-->>Controller: return LoginUser (200)
Controller-->>Client: 200 OK {user}
else New email
Service->>Repo: save new User
Repo-->>Service: user created
Service->>Repo: save UserSocialAccount (with linkToken)
Repo-->>Service: saved social account
Service-->>Controller: return LoginUser (200)
Controller-->>Client: 200 OK {user}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (2)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
capturecat-core/src/docs/asciidoc/auth.adoc(1 hunks)capturecat-core/src/main/java/com/capturecat/core/api/auth/Oauth2AuthController.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/api/auth/dto/SocialLoginRequest.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java(4 hunks)capturecat-core/src/main/java/com/capturecat/core/support/error/CoreException.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorCode.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/support/handler/CoreExceptionHandler.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/support/response/ApiResponse.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/api/auth/Oauth2AuthControllerSliceTest.java(7 hunks)capturecat-core/src/test/java/com/capturecat/core/api/auth/Oauth2AuthControllerTest.java(3 hunks)capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/service/auth/SocialLoginLinkingTest.java(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). (2)
- GitHub Check: build
- GitHub Check: deploy-dev
🔇 Additional comments (22)
capturecat-core/src/docs/asciidoc/auth.adoc (2)
5-5: LGTM! Clear documentation update.The section title now accurately reflects that this endpoint handles both login and signup flows.
11-11: LGTM! Well-documented error behavior.The documentation clearly explains the 409 Conflict response for duplicate email scenarios, which aligns with the account linking feature.
capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java (1)
70-74: LGTM! Test coverage for new error code.The test correctly includes ALREADY_REGISTERED_EMAIL in the social login error documentation.
capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorCode.java (1)
45-46: LGTM! Proper error code addition.The new ALREADY_REGISTERED_EMAIL error code is correctly added with a clear Korean message, consistent with the existing error codes in the enum.
capturecat-core/src/main/java/com/capturecat/core/api/auth/dto/SocialLoginRequest.java (1)
3-4: LGTM! Record extended for account linking.The new
accountLinkingandlinkTokenfields are appropriately added. Note thatlinkTokencan be null (handled correctly in UserService at line 193), which is expected when account linking is not being performed.capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java (3)
71-71: LGTM! Method signature appropriately extended.The method now accepts
accountLinkingandlinkTokenparameters to support the account linking feature.
187-196: LGTM! Well-encapsulated account persistence.The method correctly handles the
unlinkKeyassignment, usinglinkTokenwhen provided (for linking scenarios) and falling back topayload.unlinkKey()for initial signups.
235-236: LGTM! Clean error detail carrier.The
SocialLinkingInforecord provides a structured way to convey provider and link token information in error responses.capturecat-core/src/main/java/com/capturecat/core/support/response/ApiResponse.java (1)
27-29: LGTM! Flexible error response with detail support.The signature change from
StringtoObjectenables structured error details (likeSocialLinkingInfo) to be returned in the response data field, improving error context for clients.capturecat-core/src/main/java/com/capturecat/core/support/handler/CoreExceptionHandler.java (1)
26-30: LGTM! Consistent error detail handling.The handler correctly logs and propagates the exception detail object, aligning with the refactor from
rawResponseBodytodetail.capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java (1)
47-47: LGTM! Appropriate error type for duplicate email.The new
ALREADY_REGISTERED_EMAILerror type uses the correct HTTP 409 Conflict status and WARN log level for this client-side conflict scenario.capturecat-core/src/main/java/com/capturecat/core/api/auth/Oauth2AuthController.java (1)
41-41: LGTM!The controller correctly forwards the new account linking parameters to the service layer. The implementation is clean and aligns with the feature requirements.
capturecat-core/src/test/java/com/capturecat/core/service/auth/SocialLoginLinkingTest.java (4)
63-86: LGTM!The test correctly verifies that existing linked accounts are returned without unnecessary database operations. Good use of
never()to ensure no saves occur.
88-115: LGTM!The test correctly validates that attempting to sign up with an existing email without account linking enabled throws
ALREADY_REGISTERED_EMAILexception. The test properly verifies that no database modifications occur in this error scenario.
117-148: LGTM!The test comprehensively validates the account linking flow, correctly verifying that a new social account is linked to an existing user without creating a duplicate user. Good use of
ArgumentCaptorto verify the saved entity's fields.
150-184: LGTM!The test correctly validates the new user creation flow, ensuring both the user and social account are properly saved and linked. The test comprehensively verifies all required fields are set correctly.
capturecat-core/src/main/java/com/capturecat/core/support/error/CoreException.java (1)
11-21: LGTM!The change from
String rawResponseBodytoObject detailprovides better flexibility for carrying structured error information (like theSocialLinkingInforecord used in tests). The API surface change appears to be consistently handled across the codebase.capturecat-core/src/test/java/com/capturecat/core/api/auth/Oauth2AuthControllerTest.java (1)
54-54: LGTM!The test correctly constructs the request with the new account linking parameters and updates the mock to match the new service signature.
Also applies to: 64-64
capturecat-core/src/test/java/com/capturecat/core/api/auth/Oauth2AuthControllerSliceTest.java (4)
61-61: LGTM!The existing tests are correctly updated to include the new
accountLinkingandlinkTokenparameters with appropriate default values.Also applies to: 76-76, 100-100, 115-115, 139-139
153-179: LGTM!The test comprehensively validates the conflict scenario when attempting to sign up with an already-registered email. It correctly expects HTTP 409 status and validates the structured error response including the provider information for account linking.
181-219: LGTM!The test thoroughly validates the account linking flow, correctly setting
accountLinking=trueand providing alinkToken. It properly verifies that the login succeeds and returns the expected JWT tokens when account linking is enabled.
231-232: LGTM!The
SocialLinkingInforecord provides a clean, type-safe way to carry structured error details for the account linking conflict scenario. Good use of Java records for immutable test data.
| // 이메일로 가입된 계정을 불러오거나 신규 계정 생성 | ||
| private User generateOrFetchUser(OidcUserPayload payload, boolean accountLinking) { | ||
| User user = null; | ||
| Optional<User> byUsername = userRepository.findByUsername(payload.email()); | ||
| // 해당 이메일로 가입된 유저가 있을 경우 | ||
| if (byUsername.isPresent()) { | ||
| user = byUsername.get(); | ||
| // 연동이 아닐 경우 에러 응답 | ||
| if (!accountLinking) { | ||
| String provider = userSocialAccountRepository.findByUser(user).getFirst().getProvider(); | ||
| throw new CoreException(ErrorType.ALREADY_REGISTERED_EMAIL, | ||
| new SocialLinkingInfo(provider, payload.unlinkKey())); | ||
| } | ||
| } else { // 없을 경우 신규 생성 | ||
| user = userRepository.save(buildUser(payload)); | ||
| } | ||
| return user; | ||
| } |
There was a problem hiding this comment.
Risk: Potential NoSuchElementException on empty social accounts.
At line 207, getFirst() assumes the user has at least one associated UserSocialAccount. If a user exists in the database but has no social accounts (due to data inconsistency, migration issues, or manual operations), this will throw a NoSuchElementException.
Consider defensive handling:
private User generateOrFetchUser(OidcUserPayload payload, boolean accountLinking) {
User user = null;
Optional<User> byUsername = userRepository.findByUsername(payload.email());
// 해당 이메일로 가입된 유저가 있을 경우
if (byUsername.isPresent()) {
user = byUsername.get();
// 연동이 아닐 경우 에러 응답
if (!accountLinking) {
- String provider = userSocialAccountRepository.findByUser(user).getFirst().getProvider();
+ List<UserSocialAccount> socialAccounts = userSocialAccountRepository.findByUser(user);
+ String provider = socialAccounts.isEmpty()
+ ? "UNKNOWN"
+ : socialAccounts.getFirst().getProvider();
throw new CoreException(ErrorType.ALREADY_REGISTERED_EMAIL,
new SocialLinkingInfo(provider, payload.unlinkKey()));
}
} else { // 없을 경우 신규 생성
user = userRepository.save(buildUser(payload));
}
return user;
}Alternatively, if users without social accounts represent invalid state, throw a more specific error explaining the data inconsistency.
📝 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.
| // 이메일로 가입된 계정을 불러오거나 신규 계정 생성 | |
| private User generateOrFetchUser(OidcUserPayload payload, boolean accountLinking) { | |
| User user = null; | |
| Optional<User> byUsername = userRepository.findByUsername(payload.email()); | |
| // 해당 이메일로 가입된 유저가 있을 경우 | |
| if (byUsername.isPresent()) { | |
| user = byUsername.get(); | |
| // 연동이 아닐 경우 에러 응답 | |
| if (!accountLinking) { | |
| String provider = userSocialAccountRepository.findByUser(user).getFirst().getProvider(); | |
| throw new CoreException(ErrorType.ALREADY_REGISTERED_EMAIL, | |
| new SocialLinkingInfo(provider, payload.unlinkKey())); | |
| } | |
| } else { // 없을 경우 신규 생성 | |
| user = userRepository.save(buildUser(payload)); | |
| } | |
| return user; | |
| } | |
| // 이메일로 가입된 계정을 불러오거나 신규 계정 생성 | |
| private User generateOrFetchUser(OidcUserPayload payload, boolean accountLinking) { | |
| User user = null; | |
| Optional<User> byUsername = userRepository.findByUsername(payload.email()); | |
| // 해당 이메일로 가입된 유저가 있을 경우 | |
| if (byUsername.isPresent()) { | |
| user = byUsername.get(); | |
| // 연동이 아닐 경우 에러 응답 | |
| if (!accountLinking) { | |
| List<UserSocialAccount> socialAccounts = userSocialAccountRepository.findByUser(user); | |
| String provider = socialAccounts.isEmpty() | |
| ? "UNKNOWN" | |
| : socialAccounts.getFirst().getProvider(); | |
| throw new CoreException(ErrorType.ALREADY_REGISTERED_EMAIL, | |
| new SocialLinkingInfo(provider, payload.unlinkKey())); | |
| } | |
| } else { // 없을 경우 신규 생성 | |
| user = userRepository.save(buildUser(payload)); | |
| } | |
| return user; | |
| } |
🤖 Prompt for AI Agents
In
capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java
around lines 198 to 215, the code calls findByUser(user).getFirst() which can
throw NoSuchElementException if the user has no UserSocialAccount entries;
defensively handle that case by retrieving the social account result safely
(check if present/empty) before calling getFirst(), and if empty throw a clear
CoreException indicating data inconsistency (or a specific error type) with
context (user id/email) or otherwise handle it explicitly so the code never
calls getFirst() on an empty collection.
capturecat-core/src/test/java/com/capturecat/core/api/auth/Oauth2AuthControllerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java (1)
204-221: Don’t call getFirst() on possibly empty list; handle missing social accounts.Calling getFirst() can throw NoSuchElementException when the user exists but has no social accounts (data drift, migrations). Handle empty safely or throw a clear CoreException.
- String provider = userSocialAccountRepository.findByUser(user).getFirst().getProvider(); - throw new CoreException(ErrorType.ALREADY_REGISTERED_EMAIL, - new SocialLinkingInfo(provider, payload.unlinkKey())); + List<UserSocialAccount> accounts = userSocialAccountRepository.findByUser(user); + String provider = accounts.isEmpty() ? "UNKNOWN" : accounts.get(0).getProvider(); + throw new CoreException( + ErrorType.ALREADY_REGISTERED_EMAIL, + new SocialLinkingInfo(provider, /* ensure this is a safe, non-secret token */ payload.unlinkKey()) + );If users without social accounts are invalid state, throw a dedicated error describing the inconsistency.
🧹 Nitpick comments (3)
capturecat-core/src/main/java/com/capturecat/core/domain/user/UserTagRepository.java (1)
20-22: Bulk delete: clear/flush persistence context and return affected rows.Add clearAutomatically/flushAutomatically to avoid stale state in the persistence context after JPQL delete, and return int for observability/tests.
- @Modifying - @Query("delete from UserTag ut where ut.user.id = :userId") - void deleteAllByUserId(@Param("userId") Long userId); + @Modifying(flushAutomatically = true, clearAutomatically = true) + @Query("delete from UserTag ut where ut.user.id = :userId") + int deleteAllByUserId(@Param("userId") Long userId);If this is ever called outside a service-level @transactional method, ensure transactional boundaries are present on the caller.
capturecat-core/src/test/java/com/capturecat/core/service/user/UserServiceTest.java (1)
125-127: Remove duplicate verification; optionally assert call order.You verify userSettingsRepository.deleteById(savedUser.getId()) here and again on Line 132. The second verify is redundant; remove it or assert invocation count explicitly. Consider enforcing deletion order with InOrder.
@@ - verify(userSettingsRepository).deleteById(savedUser.getId()); - verify(userTagRepository).deleteAllByUserId(savedUser.getId()); + verify(userSettingsRepository).deleteById(savedUser.getId()); + verify(userTagRepository).deleteAllByUserId(savedUser.getId()); @@ - verify(userSettingsRepository).deleteById(savedUser.getId()); + // (optional) Already verified above; remove duplicate verifyOptional ordering check:
InOrder inOrder = inOrder(bookmarkRepository, imageTagRepository, imageRepository, userSettingsRepository, userTagRepository, userRepository); inOrder.verify(bookmarkRepository).deleteByUserId(savedUser.getId()); inOrder.verify(imageTagRepository).deleteAllTagsByUserId(savedUser.getId()); inOrder.verify(imageRepository).deleteAllImagesByUserId(savedUser.getId()); inOrder.verify(userSettingsRepository).deleteById(savedUser.getId()); inOrder.verify(userTagRepository).deleteAllByUserId(savedUser.getId()); inOrder.verify(userRepository).deleteById(savedUser.getId());capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java (1)
154-171: Deletion strategy: prefer model/DB cascade to avoid brittle test coupling.The TODO calls it out: adding repositories per child entity scales poorly and forces test churn. Prefer:
- DB FKs with ON DELETE CASCADE for join tables (e.g., user_tag).
- JPA relationships with cascade + orphanRemoval where appropriate.
- One top-level userRepository.deleteById(userId) with reliable cascades.
If bulk deletes remain, document ordering and keep them idempotent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
capturecat-core/src/main/java/com/capturecat/core/domain/user/UserTagRepository.java(2 hunks)capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java(7 hunks)capturecat-core/src/test/java/com/capturecat/core/service/user/UserServiceTest.java(3 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). (2)
- GitHub Check: build
- GitHub Check: deploy-dev
🔇 Additional comments (3)
capturecat-core/src/test/java/com/capturecat/core/service/user/UserServiceTest.java (1)
64-66: Added mock for UserTagRepository: LGTM.Mock wiring is correct and enables verification of tag cleanup in withdraw.
capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java (2)
41-41: Wiring UserTagRepository: LGTM.Field injection aligns with new cleanup path in withdraw.
73-86: upsert flow reads well; verify linking token usage matches unlink key semantics.Overall control flow is clear. Ensure the token passed here isn’t later persisted as an unlinkKey. See comment on saveSocialAccount below.
| record SocialLinkingInfo(String provider, String linkToken) { | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid exposing secrets in SocialLinkingInfo.
If the second field is currently carrying an unlinkKey, rename it and ensure it contains a safe, client-scoped link token instead. Never return a provider unlinkKey to clients.
-record SocialLinkingInfo(String provider, String linkToken) {
+record SocialLinkingInfo(String provider, String linkToken /* must NOT be unlinkKey */) {
}I'll verify this security concern by examining the codebase.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the SocialLinkingInfo record definition and its usages
rg -n 'SocialLinkingInfo' capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java -B2 -A5
Length of output: 444
🏁 Script executed:
#!/bin/bash
# Search for where SocialLinkingInfo is instantiated
rg -n 'new SocialLinkingInfo|SocialLinkingInfo\s*\(' --type=java -B3 -A3Length of output: 3287
🏁 Script executed:
#!/bin/bash
# Search for unlinkKey references in the codebase
rg -n 'unlinkKey' --type=java -B2 -A2Length of output: 10812
🏁 Script executed:
#!/bin/bash
# Search for linkToken assignments and usage patterns
rg -n 'linkToken' --type=java -B2 -A2Length of output: 7024
I need to verify how the exception data is handled and whether it's exposed to clients.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check CoreException class and its handling
rg -n 'class CoreException' --type=java -A10
Length of output: 2907
🏁 Script executed:
#!/bin/bash
# Find exception handlers that might serialize SocialLinkingInfo
rg -n 'CoreException|@ExceptionHandler|@RestControllerAdvice' --type=java -B2 -A5Length of output: 167251
🏁 Script executed:
#!/bin/bash
# Check if CoreException details are returned in responses
rg -n 'ErrorResponse|error.*response|exception.*response' --type=java -B2 -A5 | head -100Length of output: 1953
🏁 Script executed:
#!/bin/bash
# Search for where the second parameter of CoreException is used
rg -n 'new CoreException\(' --type=java -B1 -A1Length of output: 35423
Rename linkToken field or change the value—do not store unlinkKey here.
The SocialLinkingInfo.linkToken field receives payload.unlinkKey() at line 215, which is semantically incorrect. While this detail is currently logged server-side only (not exposed to clients), the field name contradicts its content and creates a code smell that could lead to accidental secret exposure if the error handling changes. Either rename the field to reflect it carries an unlinkKey, or ensure it carries only a safe, client-scoped link token as the current name implies.
📌 관련 이슈 (필수)
📝 작업 내용 (필수)
💬 리뷰 참고 사항 (선택)
Summary by CodeRabbit
New Features
Bug Fixes / Cleanup
Documentation