Merged
Conversation
Closed
1 task
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughApple OAuth 클라이언트·모델·설정과 Apple/Kakao 로그인 서비스가 추가되었고, User 도메인이 provider+socialId 기반으로 변경되었습니다. 인증 컨트롤러에 Apple 로그인·로그아웃·탈퇴 엔드포인트가 추가되었으며 jjwt 의존성과 RestTemplate 빈이 도입되고 관련 리포지토리·매퍼·테스트가 갱신되었습니다. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 분 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🧪 테스트 결과158 tests 158 ✅ 26s ⏱️ Results for commit ff43504. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In
`@ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/common/LogoutService.kt`:
- Around line 19-29: The logout flow sets updatedAt on the domain via
LogoutService.logout (creating loggedOutUser) but the value isn't persisted
because UserMapper.toEntity ignores domain.updatedAt; update UserMapper.toEntity
to map the domain.updatedAt into the UserEntity constructor (handle
nullability/defaults as appropriate) so the timestamp persists, and audit other
mappers/usages (e.g., CreateAppleUserService, CreateKakaoUserService) to ensure
they also pass domain.updatedAt through to their entity conversions.
In
`@ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/common/WithdrawService.kt`:
- Around line 21-35: In WithdrawService.withdraw the external
kakaoOAuthClient.unlink call occurs inside the `@Transactional` scope (after
userQueryRepository.findById and before userCommandRepository.delete); refactor
so the DB delete happens inside the transaction (call
userCommandRepository.delete within the transactional method) and then perform
kakaoOAuthClient.unlink outside that transaction (e.g., return from withdraw and
invoke an externalUnlink method, publish a domain event, or call an
async/retry-enabled unlink handler) and ensure unlink failures are logged and
retried or recorded for later reconciliation.
In
`@ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/login/AppleOAuthService.kt`:
- Line 45: The log statement in AppleOAuthService (log.info("애플 로그인 - socialId:
{}, email: {}, nickname: {}", socialId, email, nickname)) exposes PII; change it
to avoid plaintext socialId/email at INFO level by either removing them entirely
or logging only masked/hashed versions (e.g., SHA-256 or show only last 4 chars)
and drop to debug level (log.debug) for any non-essential details; ensure any
nickname is either non-identifying or similarly masked before logging and update
the log call accordingly.
In
`@ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/login/CreateAppleUserService.kt`:
- Around line 45-55: The current findOrCreateUser uses an email fallback which
can create duplicate accounts for Apple Sign In (email is only provided on first
login); change findOrCreateUser to always lookup by provider+socialId via
userQueryRepository.findByProviderAndSocialId(AuthProvider.APPLE, socialId) and
remove the email-based fallback (userQueryRepository.findByEmail) from the
lookup path; instead, when calling createNewUser(email, nickname, profileImage,
socialId) ensure that createNewUser persists the AuthProvider and socialId with
the new User and only stores the email as an attribute (not as a lookup key), or
limit email-based association to an explicit first-login flow if you have a
reliable flag—this ensures socialId is the sole trust anchor for Apple users and
prevents duplicate accounts.
In
`@ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/login/CreateKakaoUserService.kt`:
- Around line 48-51: 현재 CreateKakaoUserService의 사용자 조회
로직(userQueryRepository.findByProviderAndSocialId(...) ?:
userQueryRepository.findByEmail(email))은 이메일 기반 폴백으로 인해 다른 소셜 프로바이더 계정과 잘못 연결될 수
있으니, 이메일 폴백을 제거하고 provider+socialId 조회만 사용하도록 수정하거나(즉 existingUser =
userQueryRepository.findByProviderAndSocialId(AuthProvider.KAKAO, socialId)만 사용)
이메일이 이미 다른 프로바이더로 존재하는 경우에는 createNewUser(...)를 바로 호출하지 말고 계정 연동 확인 또는 예외를 발생시키는
흐름을 추가하라; 참조 심볼: CreateKakaoUserService,
userQueryRepository.findByProviderAndSocialId, userQueryRepository.findByEmail,
createNewUser.
In
`@ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/login/KakaoLoginService.kt`:
- Around line 30-36: KakaoLoginService currently maps a null Kakao email to an
empty string which breaks the UserEntity unique constraint; change the null
handling so that when kakaoProfile.kakao_account.email is null you generate a
unique placeholder (e.g., UUID-based string like "<uuid>@kakao.placeholder") and
pass that into createKakaoUserService.saveUserAndGenerateTokens instead of "",
ensure the placeholder format is deterministic/unique per user and acceptable to
your email validation rules, and update createNewUser /
userQueryRepository.findByEmail assumptions if they expect real emails (or
alternatively make email nullable in UserEntity and adjust uniqueness logic to
use provider+socialId in find/create flows).
In `@ssolv-infrastructure/build.gradle.kts`:
- Around line 48-52: Upgrade the hardcoded io.jsonwebtoken dependencies
(io.jsonwebtoken:jjwt-api, jjwt-impl, jjwt-jackson) from 0.12.6 to 0.13.0 in
both modules to mitigate the Bouncy Castle CVE; then centralize the version by
adding a single jjwt entry in the Version Catalog (gradle/libs.versions.toml) or
buildSrc and replace the literal "0.12.6" usages with the catalog/buildSrc
reference in both build.gradle.kts files so all modules consume the same
version.
In
`@ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/auth/client/AppleOAuthClient.kt`:
- Around line 30-54: getAllowedRedirectUris() currently only merges
hardcodedUris with appleProperties.redirectUris (a collection), missing the
single-value appleProperties.redirectUri, which can cause valid redirects to be
rejected in requestToken(); update getAllowedRedirectUris() to also include
appleProperties.redirectUri (if non-null/non-blank) so the returned Set contains
that single configured URI as well as redirectUris, referencing the
getAllowedRedirectUris(), appleProperties.redirectUri,
appleProperties.redirectUris, and requestToken() symbols when making the change.
- Around line 122-138: The parseIdToken function currently only Base64-decodes
the payload; update parseIdToken( idToken: String ) to validate the JWT
signature using Apple's JWKS (https://appleid.apple.com/auth/keys) and then
verify standard claims before converting to AppleResponse.IdTokenPayload: fetch
and cache JWKS, select the key by the token's header.kid, verify the token
signature, ensure iss == "https://appleid.apple.com", aud equals your configured
clientId, and exp is in the future; only after successful verification
deserialize the payload with objectMapper.readValue and propagate AuthException
on any verification failure (preserve existing exception handling behavior).
- Around line 59-83: The code in AppleOAuthClient currently creates a new
RestTemplate per request (variable restTemplate) without timeouts; inject a
RestTemplateBuilder into the AppleOAuthClient (constructor) and use it to build
a reusable RestTemplate bean with explicit connect and read timeouts, replace
the local new RestTemplate() usage with the injected/built RestTemplate, and
ensure AppleOAuthClient uses that instance when calling
restTemplate.exchange(...) to avoid indefinite blocking on Apple API calls.
🧹 Nitpick comments (4)
ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/auth/properties/AppleProperties.kt (1)
6-16: Spring Boot 권장 패턴 및 설정 검증을 고려해 주세요.현재
@Component+@ConfigurationProperties조합을 사용하고 있는데, Spring Boot 2.2+ 에서는@ConfigurationPropertiesScan또는@EnableConfigurationProperties를 권장합니다. 또한 필수 설정값에 대한 검증이 없어 애플리케이션 시작 시 빈 값으로 실행될 수 있습니다.
privateKey는 민감 정보이므로 로깅에 노출되지 않도록 주의가 필요합니다.♻️ 검증 추가 제안
+import jakarta.validation.constraints.NotBlank +import org.springframework.validation.annotation.Validated `@Component` +@Validated `@ConfigurationProperties`(prefix = "apple") data class AppleProperties( - var teamId: String = "", - var keyId: String = "", - var clientId: String = "", + `@field`:NotBlank var teamId: String = "", + `@field`:NotBlank var keyId: String = "", + `@field`:NotBlank var clientId: String = "", var redirectUri: String = "", var redirectUris: List<String> = emptyList(), - var privateKey: String = "", + `@field`:NotBlank var privateKey: String = "", var tokenUri: String = "https://appleid.apple.com/auth/token" )ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/login/CreateKakaoUserService.kt (1)
74-89: 토큰 생성 및 프로필 업데이트 로직이 적절합니다.프로필 이미지 변경 여부를 확인하고 조건부로 업데이트하는 로직이 명확합니다.
user.id!!는createNewUser에서 저장된 사용자를 반환하므로 안전하지만, 방어적으로requireNotNull(user.id)를 사용하면 더 명확한 에러 메시지를 제공할 수 있습니다.ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/auth/properties/KakaoProperties.kt (1)
13-14: adminKey 누락을 조기 검증하세요.
Line 13-14에서 adminKey 기본값이 빈 문자열이라 설정 누락 시 런타임에서만 실패할 가능성이 큽니다. unlink 기능을 쓴다면@Validated+@NotBlank등으로 fail-fast 검증을 두는 것이 안전합니다.✅ 예시
+import org.springframework.validation.annotation.Validated +import jakarta.validation.constraints.NotBlank `@Component` `@ConfigurationProperties`(prefix = "kakao") +@Validated data class KakaoProperties( var clientId: String = "", var redirectUri: String = "", var redirectUris: List<String> = emptyList(), var tokenUri: String = "https://kauth.kakao.com/oauth/token", var userInfoUri: String = "https://kapi.kakao.com/v2/user/me", - var adminKey: String = "" + `@field`:NotBlank + var adminKey: String = "" )ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/auth/client/KakaoOAuthClient.kt (1)
187-197: 예외를 삼키면 탈퇴 실패 원인 파악이 어려워질 수 있습니다.현재 구현은 카카오 연결 끊기 실패 시 로그만 남기고 진행하는데, 이는 의도된 설계로 이해됩니다. 하지만 운영 환경에서 탈퇴 관련 이슈 발생 시 원인 파악이 어려울 수 있습니다.
성공/실패 여부를 반환하거나, 실패 시 메트릭을 수집하는 방안을 고려해 보세요.
♻️ 개선 제안
- fun unlink(socialId: String) { + fun unlink(socialId: String): Boolean { val restTemplate = RestTemplate() val headers = HttpHeaders().apply { add("Content-Type", "application/x-www-form-urlencoded") add("Authorization", "KakaoAK ${kakaoProperties.adminKey}") } val params: MultiValueMap<String, String> = LinkedMultiValueMap<String, String>().apply { add("target_id_type", "user_id") add("target_id", socialId) } val request = HttpEntity(params, headers) - try { + return try { restTemplate.exchange( "https://kapi.kakao.com/v1/user/unlink", HttpMethod.POST, request, String::class.java ) + true } catch (e: Exception) { log.error("카카오 연결 끊기 실패 - socialId: {}, error: {}", socialId, e.message) - // 탈퇴 과정이므로 에러가 발생해도 로컬 데이터 삭제는 진행할 수 있도록 예외를 던지지 않거나 로그만 남김 + false } }
ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/common/LogoutService.kt
Show resolved
Hide resolved
ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/common/WithdrawService.kt
Outdated
Show resolved
Hide resolved
ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/login/AppleOAuthService.kt
Outdated
Show resolved
Hide resolved
...pi-core/src/main/kotlin/org/depromeet/team3/auth/application/login/CreateAppleUserService.kt
Show resolved
Hide resolved
...pi-core/src/main/kotlin/org/depromeet/team3/auth/application/login/CreateKakaoUserService.kt
Show resolved
Hide resolved
ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/login/KakaoLoginService.kt
Show resolved
Hide resolved
ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/auth/client/AppleOAuthClient.kt
Show resolved
Hide resolved
ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/auth/client/AppleOAuthClient.kt
Outdated
Show resolved
Hide resolved
ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/auth/client/AppleOAuthClient.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@ssolv-api-common/build.gradle.kts`:
- Around line 19-21: The build references the version-catalog accessor
libs.jjwt.api but no version catalog is configured; either add a
gradle/libs.versions.toml that defines JJWT entries (e.g., jjwt-api/impl/jackson
coordinates and version) and enable the catalog and TYPESAFE_PROJECT_ACCESSORS
in settings.gradle.kts (call enableFeaturePreview("TYPESAFE_PROJECT_ACCESSORS")
and configureVersionCatalogs if needed), or replace the libs.* usages in
build.gradle.kts with hard-coded dependency strings like
"io.jsonwebtoken:jjwt-api:0.12.3" / "io.jsonwebtoken:jjwt-impl:0.12.3" /
"io.jsonwebtoken:jjwt-jackson:0.12.3" so the build no longer relies on a missing
libs accessor (update references to libs.jjwt.api, libs.jjwt.impl,
libs.jjwt.jackson accordingly).
In `@ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/auth/UserEntity.kt`:
- Around line 39-43: The UserEntity.meetings mapping currently uses cascade =
[CascadeType.ALL] and orphanRemoval = true which causes MeetingEntity rows (and
thus dangling MeetingAttendeeEntity records) to be deleted when a host user is
removed; change the mapping to remove CascadeType.ALL and orphanRemoval on
meetings, and instead handle host-user deletion explicitly in the user deletion
flow: in the service that removes a UserEntity, check MeetingEntity.attendees
for each hosted meeting and either transfer hosting (assign hostUser to another
attendee), prevent deletion if attendees exist, or perform a controlled delete
that first removes MeetingAttendeeEntity rows then the MeetingEntity, ensuring
referential integrity (reference symbols: UserEntity.meetings, MeetingEntity,
MeetingAttendeeEntity, MeetingEntity.attendees).
In
`@ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/common/BaseTimeEntity.kt`:
- Around line 13-18: BaseTimeEntity exposes mutable timestamp fields which
allows external code to overwrite createdAt/updatedAt; change declarations to
make setters non-public (e.g., keep properties as var createdAt: LocalDateTime =
LocalDateTime.now() private set and var updatedAt: LocalDateTime? = null private
set) so only the entity itself (e.g., its updateTimestamp() method) can modify
them, leaving getters public for reads and preserving JPA mapping on the same
properties.
In
`@ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/mapper/UserMapper.kt`:
- Around line 33-36: The mapping sets entity.createdAt = domain.createdAt
directly in the apply block, which can violate BaseTimeEntity's non-null DB
constraint if domain.createdAt is null; update the mapper (the apply block in
UserMapper.kt where createdAt and updatedAt are assigned) to guard against null
by using a null-check or default (e.g., use domain.createdAt ?: Instant.now() or
only set when non-null) so createdAt is never assigned null, and do the same
consideration for updatedAt as appropriate.
🧹 Nitpick comments (2)
ssolv-api-core/build.gradle.kts (1)
15-17: ssolv-api-core에서 불필요한 jjwt 의존성 선언 제거 권장
ssolv-api-core는 jjwt 클래스를 직접 사용하지 않습니다.UpdateTokenService,CreateAppleUserService등의 클래스들은 모두 내부 추상화 계층인JwtTokenProvider를 통해서만 토큰 처리를 수행합니다.
ssolv-api-common에서 이미api()스코프로 jjwt를 선언하고 있으며,ssolv-api-core는 이 모듈에 의존하므로 jjwt를 transitively 사용할 수 있습니다. 따라서 lines 15-17의 jjwt 의존성 선언은 불필요합니다.제거 권장 코드
implementation(libs.jjwt.api) runtimeOnly(libs.jjwt.impl) runtimeOnly(libs.jjwt.jackson)ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/login/CreateKakaoUserService.kt (1)
43-51: 동일 이메일의 타 프로바이더 계정 처리 정책 검토 권장현재
UserEntity의다만 보안 및 사용자 경험 관점에서 다른 프로바이더의 동일 이메일 계정이 있는 경우, 자동 병합 없이 사용자에게 명확한 안내를 제공하는 것이 권장됩니다.
🔧 선택적 개선 예시
private fun findOrCreateUser( email: String, nickname: String, profileImage: String?, socialId: String ): User { val existingUser = userQueryRepository.findByProviderAndSocialId(AuthProvider.KAKAO, socialId) + if (existingUser == null) { + val emailOwner = userQueryRepository.findByEmail(email) + if (emailOwner != null && emailOwner.provider != AuthProvider.KAKAO) { + throw IllegalStateException("EMAIL_IN_USE_BY_OTHER_PROVIDER") + } + } return existingUser ?: createNewUser(email, nickname, profileImage, socialId) }
ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/auth/UserEntity.kt
Show resolved
Hide resolved
ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/common/BaseTimeEntity.kt
Show resolved
Hide resolved
ssolv-infrastructure/src/main/kotlin/org/depromeet/team3/mapper/UserMapper.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/common/WithdrawService.kt`:
- Around line 79-86: deleteLocalUser() is annotated with `@Transactional` but is
invoked via self-invocation from withdraw(), so Spring AOP proxying is skipped;
move the transactional deletion into a separate `@Service` bean (e.g.,
UserDeletionService) with a method deleteLocalUser(User) annotated with
`@Transactional`, inject that service into WithdrawService and replace the
internal call to deleteLocalUser(user) with
userDeletionService.deleteLocalUser(user) so the proxy applies and cascade
deletions run inside a real transaction; alternatively, if you prefer
programmatic transactions, use a TransactionTemplate in WithdrawService to
execute the deletion logic inside a transaction (or register a
TransactionSynchronizationManager callback to run unlink logic after commit).
🧹 Nitpick comments (2)
ssolv-global-utils/src/main/kotlin/org/depromeet/team3/common/exception/ErrorCode.kt (1)
67-76: 주석 범위와 실제 코드 불일치주석에 "O011
O020"이라고 되어 있지만 실제 에러 코드는 O019까지만 정의되어 있습니다. 주석을 "O011O019"로 수정하거나, O020을 예약해둔 것이라면 그 의도를 명시해주세요.- // Apple OAuth 관련 에러 (O011~O020) + // Apple OAuth 관련 에러 (O011~O019)ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/common/WithdrawService.kt (1)
62-77: N+1 쿼리 문제 가능성모임 목록을 조회한 후 각 모임별로 참석자를 개별 쿼리하면 모임 수만큼 추가 쿼리가 발생합니다. 모임 수가 많은 사용자의 탈퇴 시 성능 저하가 발생할 수 있습니다.
리포지토리에 다른 참석자가 있는 호스팅 모임을 한 번에 확인하는 메서드를 추가하는 것을 권장합니다.
♻️ 단일 쿼리로 최적화하는 예시
// MeetingRepository에 추가 fun hasHostedMeetingsWithOtherAttendees(userId: Long): Boolean // WithdrawService에서 사용 private fun validateHostedMeetings(userId: Long) { if (meetingRepository.hasHostedMeetingsWithOtherAttendees(userId)) { log.warn("탈퇴 시도 실패 - userId: {}, 다른 참석자가 있는 모임 호스팅 중", userId) throw AuthException(ErrorCode.CANNOT_WITHDRAW_WITH_ACTIVE_MEETINGS) } }
ssolv-api-core/src/main/kotlin/org/depromeet/team3/auth/application/common/WithdrawService.kt
Outdated
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎋 이슈 및 작업중인 브랜치
🔑 주요 내용
Check List
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.