[NO-ISSUE] FCM 푸시 알림 기능 develop 이식 및 회원 탈퇴 시 토큰 정리#266
Merged
Conversation
롤백(SOU-638)으로 제외됐던 FCM 푸시 알림 모듈을 develop의 새 패키지 구조(shared/auth.application)에 맞춰 재이식합니다. 주요 변경 - FCM 토큰 등록/해제 API (FcmTokenController, FcmTokenCommandService) - Firebase Admin 연동 (shared.config.FirebaseConfig/Properties) - 단일/사용자/브로드캐스트 전송 (FcmNotificationService) - 관리자 브로드캐스트 API 및 발송 이력 (AdminPushApi, PushBroadcastHistory) - 회원 탈퇴 시 활성 FCM 토큰 일괄 비활성화 (UserService.withdraw) 인프라/매핑 - ErrorCode.FCM_SEND_FAILED 추가 - orm.xml 에 FcmToken, PushBroadcastHistory 엔티티 매핑 추가 - V16__create_push_broadcast_history 마이그레이션 추가 - build.gradle firebase-admin 의존성 추가 테스트/문서 - 도메인/애플리케이션/컨트롤러 테스트 추가 (RestDocs 포함) - app/notification.adoc, admin/push.adoc API 문서 추가 - 전체 테스트 404개 통과 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 29 minutes and 40 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughFirebase Cloud Messaging(FCM) 라이브러리를 통합하고, 사용자별 FCM 토큰 관리 및 전체 기기 대상 푸시 브로드캐스트 시스템을 추가합니다. Firebase 설정, FCM 토큰 및 브로드캐스트 히스토리 도메인 엔티티, 토큰 등록/갱신/조회 서비스, 푸시 발송 및 이력 기록 서비스, 사용자/관리자 REST API 엔드포인트, DB 스키마, 포괄적 테스트 및 API 문서를 한 번에 추가합니다. ChangesFirebase Cloud Messaging 푸시 알림 기능
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b503edc74b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+28
to
+29
| existing.linkUser(userId); | ||
| existing.syncDeviceIdentity(request.deviceType(), request.deviceId()); |
There was a problem hiding this comment.
medium: 같은 물리 기기에서 다른 계정으로 전환할 때 새 요청의 FCM token이 이미 다른 사용자 row에 있고 현재 사용자에게 동일 deviceId의 기존 row가 남아 있으면, 여기서 기존 token row를 현재 userId/deviceId로 바꾸는 순간 V14__create_fcm_token_tablel.sql의 (user_id, device_id) unique index와 충돌해 등록 API가 500으로 실패합니다. token 기준 row와 현재 user/device row를 모두 조회한 뒤 기존 device row를 비활성화/삭제하거나 merge한 후 저장하세요.
Useful? React with 👍 / 👎.
Comment on lines
+52
to
+54
| } catch (FirebaseMessagingException e) { | ||
| log.error("FCM 전송 실패 tokenPrefix={} error={}", maskToken(registrationToken), e.getMessagingErrorCode(), e); | ||
| throw new BusinessException(ErrorCode.FCM_SEND_FAILED, e.getMessage()); |
There was a problem hiding this comment.
medium: 앱 삭제/재설치 등으로 FCM이 UNREGISTERED 같은 영구 실패를 반환해도 여기서 모든 FirebaseMessagingException을 동일한 FCM_SEND_FAILED로 감싸 호출부는 failCount만 올립니다. 그 token은 active=true로 남아 findAllByActiveTrue()/findByUserIdAndActiveTrue()에 계속 포함되므로 이후 broadcast마다 반복 실패하고, 사용자별 알림은 stale token만 있을 때 계속 실패할 수 있습니다. MessagingErrorCode를 보존해 영구 실패 시 해당 FcmToken을 비활성화/삭제하도록 처리하세요.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/main/java/com/souzip/shared/config/FirebaseConfig.java (1)
23-36: 💤 Low value파일 존재 여부를 명시적으로 검증하면 더 명확한 에러 메시지를 제공할 수 있습니다.
현재 구현은
FileInputStream생성 시 파일이 없으면FileNotFoundException이 발생하지만, 명시적으로 파일 존재를 체크하면 사용자에게 더 명확한 에러 메시지를 제공할 수 있습니다.♻️ 제안하는 개선안
`@Bean` public FirebaseApp firebaseApp() throws IOException { if (!StringUtils.hasText(firebaseProperties.getCredentialsPath())) { throw new IllegalStateException("firebase.enabled=true 일 때 firebase.credentials-path 가 필요합니다."); } + + File credentialsFile = new File(firebaseProperties.getCredentialsPath()); + if (!credentialsFile.exists()) { + throw new IllegalStateException( + String.format("Firebase credentials 파일을 찾을 수 없습니다: %s", firebaseProperties.getCredentialsPath()) + ); + } + - try (FileInputStream serviceAccount = new FileInputStream(firebaseProperties.getCredentialsPath())) { + try (FileInputStream serviceAccount = new FileInputStream(credentialsFile)) { FirebaseOptions options = FirebaseOptions.builder() .setCredentials(GoogleCredentials.fromStream(serviceAccount)) .build();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/souzip/shared/config/FirebaseConfig.java` around lines 23 - 36, In FirebaseConfig.firebaseApp(), add an explicit file-existence check for the path returned by firebaseProperties.getCredentialsPath() before opening FileInputStream: verify the path is non-empty (you already check) and that new File(path).exists() and isFile(); if not, throw a clear IllegalStateException (e.g. "Firebase credentials file not found at: <path>") so callers get a descriptive error instead of a FileNotFoundException; keep the existing try-with-resources FileInputStream block and rest of initialization (FirebaseOptions.builder(), FirebaseApp.initializeApp/getInstance()) unchanged.src/main/java/com/souzip/domain/notification/FcmToken.java (1)
44-44: 타임존 기준 명확화(일관성은 유지, 범위 큰 전환은 신중히)
FcmToken.lastUsedAt가LocalDateTime.now()로 갱신되며(예:FcmToken.java의 각 메서드에서lastUsedAt = LocalDateTime.now()),BaseEntity의createdAt/updatedAt도 동일하게LocalDateTime이라 현재 도메인 타임스탬프 방식은 일관적입니다.- 다만 다중 타임존 환경을 고려하면
LocalDateTime저장의 기준 타임존(UTC vs Asia/Seoul 등)을 명확히(문서화/표준화)하고, 필요 시Instant/ZonedDateTime또는 공통DateTimeProvider기반의 점진적 전환을 검토하는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/souzip/domain/notification/FcmToken.java` at line 44, The review points out inconsistent timezone semantics: update FcmToken.lastUsedAt (and related timestamps like BaseEntity.createdAt/updatedAt) to use a clear timezone strategy rather than calling LocalDateTime.now() directly; pick and document a project-wide approach (e.g., use Instant.now() for UTC storage or ZonedDateTime with a defined zone) or introduce a centralized DateTimeProvider/Clock to obtain timestamps, then replace occurrences of LocalDateTime.now() in FcmToken (lastUsedAt assignments) and the BaseEntity timestamp setters to call that provider (or Instant.now()) so all domain timestamps share the same, explicit timezone source.src/main/java/com/souzip/application/notification/FcmTokenCommandService.java (1)
60-66: ⚡ Quick win반복 저장 대신 벌크 업데이트 고려.
현재 구현은 각 토큰을 개별적으로
deactivate()하고 저장합니다. 사용자가 다수의 활성 토큰을 보유한 경우 성능에 영향을 줄 수 있습니다. JPA의@Modifying쿼리를 사용한 벌크 업데이트를 고려해보세요.♻️ 벌크 업데이트 예시
FcmTokenRepository에 커스텀 쿼리 추가:
`@Modifying` `@Query`("UPDATE FcmToken f SET f.active = false WHERE f.userId = :userId AND f.active = true") int deactivateAllByUserId(`@Param`("userId") Long userId);서비스 메서드 변경:
public void deactivateAllByUserId(Long userId) { - List<FcmToken> tokens = fcmTokenRepository.findByUserIdAndActiveTrue(userId); - tokens.forEach(token -> { - token.deactivate(); - fcmTokenRepository.save(token); - }); + fcmTokenRepository.deactivateAllByUserId(userId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/souzip/application/notification/FcmTokenCommandService.java` around lines 60 - 66, The current deactivateAllByUserId in FcmTokenCommandService iterates and saves each FcmToken causing N updates; replace with a bulk update: add a method to FcmTokenRepository such as `@Modifying` `@Query`("UPDATE FcmToken f SET f.active = false WHERE f.userId = :userId AND f.active = true") int deactivateAllByUserId(`@Param`("userId") Long userId) and then change FcmTokenCommandService.deactivateAllByUserId to call that repository method (ensure the service method or repository call is executed within a `@Transactional` context and, if using Spring Data JPA, add `@Transactional` and `@Modifying` imports where needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.gradle`:
- Line 63: The build declares an outdated Firebase Admin dependency
("com.google.firebase:firebase-admin:9.4.3") which should be reviewed and
updated; change the version to 9.9.0 (or later) in the implementation
coordinate, then run a Gradle dependency report (e.g., ./gradlew dependencies
--configuration compileClasspath and runtimeClasspath or dependencyInsight) to
verify transitive vulnerabilities and compatibility, and if vulnerable
transitive artifacts are found add dependency constraints or
force/implementation overrides in build.gradle to pin safe patch versions and
run your test suite to confirm no regressions.
In
`@src/main/java/com/souzip/application/notification/FcmNotificationService.java`:
- Around line 59-86: The logs in sendToUser leak a user identifier (userId)
which is PII; remove or mask userId in all log.warn calls inside sendToUser and
instead log only the non-PII token id (token.getId()) and error info
(e.getErrorCode()), or move detailed user-linked logs to a debug-only message.
Concretely, update the catch block that calls log.warn("FCM 전송 실패... userId={},
fcmTokenId={}, errorCode={}") to omit or mask userId and keep fcmTokenId and
errorCode, and update the later log.warn("FCM 일부 토큰 실패 userId={}, 성공={}, 실패={}")
to remove or mask userId (e.g., log only success/fail counts or use a
debug-level message that includes a masked userId). Ensure these changes are
applied in the sendToUser method and that references to FcmToken.getId(),
sendToToken, and BusinessException handling remain intact.
- Around line 34-56: The catch in sendToToken currently passes e.getMessage()
into the BusinessException which may expose sensitive Firebase details; change
the catch to avoid forwarding raw exception text: keep the detailed log (using
maskToken(registrationToken) and include e for internal traces), but throw new
BusinessException(ErrorCode.FCM_SEND_FAILED, "FCM 전송 실패") or another
non-sensitive, localized message instead of e.getMessage(); do not include the
raw FirebaseMessagingException text in the BusinessException. Also ensure the
catch still logs e (as it does) so internal diagnostics remain available while
client-facing messages are sanitized.
In `@src/main/java/com/souzip/domain/notification/FcmTokenNotFoundException.java`:
- Around line 15-19: The FCM token is being included in the exception message in
FcmTokenNotFoundException.byToken(String token), which exposes sensitive
credentials; change byToken to return a generic message that does not include
the token (e.g., "FCM 토큰을 찾을 수 없습니다.") and remove any direct token interpolation
in the String.format call; if you need to retain the token for internal tracing,
store it in a private/non-logged field on the exception rather than embedding it
in the message, but do not log or expose the raw token.
- Around line 9-13: The exception factory FcmTokenNotFoundException.byDeviceId
currently embeds the raw deviceId in the message (risking PII leakage); change
it to return a non-sensitive message (for example "FCM token for the specified
device was not found.") and remove the raw deviceId from the exception text, or
if you must include an identifier, call a dedicated masker (e.g.,
maskDeviceId(deviceId)) to redact most characters before inserting; update
FcmTokenNotFoundException.byDeviceId and add or reuse a maskDeviceId utility
where needed, and ensure any logging that needs the full deviceId uses
secure/debug-only logs rather than the exception message.
In `@src/main/java/com/souzip/domain/notification/PushBroadcastHistory.java`:
- Around line 29-47: In PushBroadcastHistory.record, add defensive validation
that the integer count fields totalTargets, successCount, and failCount are
non-negative: if any is < 0 throw an IllegalArgumentException (or use
Objects.checkIndex-style validation) with a clear message (e.g., "totalTargets
must be non-negative"), then proceed to assign the fields as before; keep the
existing requireNonNull checks for adminId/title/body and update only the record
method to enforce these non-negative checks for totalTargets, successCount and
failCount.
---
Nitpick comments:
In
`@src/main/java/com/souzip/application/notification/FcmTokenCommandService.java`:
- Around line 60-66: The current deactivateAllByUserId in FcmTokenCommandService
iterates and saves each FcmToken causing N updates; replace with a bulk update:
add a method to FcmTokenRepository such as `@Modifying` `@Query`("UPDATE FcmToken f
SET f.active = false WHERE f.userId = :userId AND f.active = true") int
deactivateAllByUserId(`@Param`("userId") Long userId) and then change
FcmTokenCommandService.deactivateAllByUserId to call that repository method
(ensure the service method or repository call is executed within a
`@Transactional` context and, if using Spring Data JPA, add `@Transactional` and
`@Modifying` imports where needed).
In `@src/main/java/com/souzip/domain/notification/FcmToken.java`:
- Line 44: The review points out inconsistent timezone semantics: update
FcmToken.lastUsedAt (and related timestamps like BaseEntity.createdAt/updatedAt)
to use a clear timezone strategy rather than calling LocalDateTime.now()
directly; pick and document a project-wide approach (e.g., use Instant.now() for
UTC storage or ZonedDateTime with a defined zone) or introduce a centralized
DateTimeProvider/Clock to obtain timestamps, then replace occurrences of
LocalDateTime.now() in FcmToken (lastUsedAt assignments) and the BaseEntity
timestamp setters to call that provider (or Instant.now()) so all domain
timestamps share the same, explicit timezone source.
In `@src/main/java/com/souzip/shared/config/FirebaseConfig.java`:
- Around line 23-36: In FirebaseConfig.firebaseApp(), add an explicit
file-existence check for the path returned by
firebaseProperties.getCredentialsPath() before opening FileInputStream: verify
the path is non-empty (you already check) and that new File(path).exists() and
isFile(); if not, throw a clear IllegalStateException (e.g. "Firebase
credentials file not found at: <path>") so callers get a descriptive error
instead of a FileNotFoundException; keep the existing try-with-resources
FileInputStream block and rest of initialization (FirebaseOptions.builder(),
FirebaseApp.initializeApp/getInstance()) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9bdfb804-6f25-4164-9fe9-85f71998b46a
📒 Files selected for processing (39)
build.gradlesrc/docs/asciidoc/admin/index.adocsrc/docs/asciidoc/admin/push.adocsrc/docs/asciidoc/app/index.adocsrc/docs/asciidoc/app/notification.adocsrc/main/java/com/souzip/adapter/webapi/admin/AdminPushApi.javasrc/main/java/com/souzip/adapter/webapi/admin/dto/PushBroadcastRequest.javasrc/main/java/com/souzip/application/notification/FcmNotificationService.javasrc/main/java/com/souzip/application/notification/FcmTokenCommandService.javasrc/main/java/com/souzip/application/notification/FcmTokenQueryService.javasrc/main/java/com/souzip/application/notification/PushBroadcastHistoryCommandService.javasrc/main/java/com/souzip/application/notification/PushBroadcastHistoryQueryService.javasrc/main/java/com/souzip/application/notification/dto/PushBroadcastHistoryResponse.javasrc/main/java/com/souzip/application/notification/dto/PushBroadcastResult.javasrc/main/java/com/souzip/application/notification/provided/FcmTokenFinder.javasrc/main/java/com/souzip/application/notification/required/FcmTokenRepository.javasrc/main/java/com/souzip/application/notification/required/PushBroadcastHistoryRepository.javasrc/main/java/com/souzip/domain/notification/DeviceType.javasrc/main/java/com/souzip/domain/notification/FcmToken.javasrc/main/java/com/souzip/domain/notification/FcmTokenNotFoundException.javasrc/main/java/com/souzip/domain/notification/FcmTokenRegisterRequest.javasrc/main/java/com/souzip/domain/notification/PushBroadcastHistory.javasrc/main/java/com/souzip/domain/notification/controller/FcmTokenController.javasrc/main/java/com/souzip/domain/notification/dto/FcmTokenResponse.javasrc/main/java/com/souzip/domain/user/service/UserService.javasrc/main/java/com/souzip/shared/config/FirebaseConfig.javasrc/main/java/com/souzip/shared/config/FirebaseProperties.javasrc/main/java/com/souzip/shared/config/FirebasePropertiesConfiguration.javasrc/main/java/com/souzip/shared/exception/ErrorCode.javasrc/main/resources/META-INF/orm.xmlsrc/main/resources/db/migration/V16__create_push_broadcast_history.sqlsrc/test/java/com/souzip/adapter/webapi/admin/AdminPushApiTest.javasrc/test/java/com/souzip/application/notification/FcmNotificationServiceTest.javasrc/test/java/com/souzip/application/notification/FcmTokenCommandServiceTest.javasrc/test/java/com/souzip/domain/notification/FcmTokenRegisterRequestTest.javasrc/test/java/com/souzip/domain/notification/FcmTokenTest.javasrc/test/java/com/souzip/domain/notification/PushBroadcastHistoryTest.javasrc/test/java/com/souzip/domain/notification/controller/FcmTokenControllerTest.javasrc/test/java/com/souzip/domain/user/service/UserServiceTest.java
|
|
||
| implementation 'com.amazonaws:aws-java-sdk-s3:1.12.529' | ||
|
|
||
| implementation 'com.google.firebase:firebase-admin:9.4.3' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Firebase Admin SDK 최신 버전 확인
curl -s https://repo1.maven.org/maven2/com/google/firebase/firebase-admin/maven-metadata.xml | grep -o '<latest>[^<]*</latest>' | sed 's/<[^>]*>//g'
# GitHub Security Advisories 확인
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.google.firebase:firebase-admin") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: souzip/souzip-api
Length of output: 244
firebase-admin 9.4.3 버전(보안 권고/취약점) 확인 및 필요 시 업그레이드 권장 (build.gradle 63행)
- 현재 Gradle에
com.google.firebase:firebase-admin:9.4.3을 사용 중이며, Maven 메타데이터 기준 최신은9.9.0입니다. 취약점 스캔/호환성 검토 후9.9.0(또는 그 이후)로 업그레이드를 검토하세요. - GitHub Security Advisories는 API 호출이 인증 필요(HTTP 401)로 현재 상태에서 직접 확인이 어렵습니다. 대신 Gradle 의존성 트리(compileClasspath/runtimeClasspath) 기반 SCA로 실제 취약 컴포넌트(트랜지티브 포함) 유무를 확인하고, 필요 시 dependency constraints/override로 패치 버전을 적용하세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.gradle` at line 63, The build declares an outdated Firebase Admin
dependency ("com.google.firebase:firebase-admin:9.4.3") which should be reviewed
and updated; change the version to 9.9.0 (or later) in the implementation
coordinate, then run a Gradle dependency report (e.g., ./gradlew dependencies
--configuration compileClasspath and runtimeClasspath or dependencyInsight) to
verify transitive vulnerabilities and compatibility, and if vulnerable
transitive artifacts are found add dependency constraints or
force/implementation overrides in build.gradle to pin safe patch versions and
run your test suite to confirm no regressions.
Comment on lines
+59
to
+86
| public void sendToUser(Long userId, String title, String body) { | ||
| List<FcmToken> tokens = fcmTokenFinder.getActiveTokensByUserId(userId); | ||
| if (tokens.isEmpty()) { | ||
| return; | ||
| } | ||
| int successCount = 0; | ||
| int failCount = 0; | ||
| for (FcmToken token : tokens) { | ||
| try { | ||
| sendToToken(token.getToken(), title, body); | ||
| successCount++; | ||
| } catch (BusinessException e) { | ||
| failCount++; | ||
| log.warn( | ||
| "FCM 전송 실패(다음 토큰으로 계속) userId={}, fcmTokenId={}, errorCode={}", | ||
| userId, | ||
| token.getId(), | ||
| e.getErrorCode() | ||
| ); | ||
| } | ||
| } | ||
| if (failCount > 0 && successCount == 0) { | ||
| throw new BusinessException(ErrorCode.FCM_SEND_FAILED, "활성 토큰 전송이 모두 실패했습니다."); | ||
| } | ||
| if (failCount > 0) { | ||
| log.warn("FCM 일부 토큰 실패 userId={}, 성공={}, 실패={}", userId, successCount, failCount); | ||
| } | ||
| } |
There was a problem hiding this comment.
userId 로깅은 PII 유출에 해당할 수 있음.
Lines 72-77, 84에서 userId를 로그에 기록하고 있습니다. 코딩 가이드라인에 따르면 "Never log PII (email, phone, tokens, access keys)"이며, 사용자 식별자(userId)는 PII로 간주될 수 있습니다. 운영 환경에서 민감 정보 유출을 방지하기 위해 userId 로깅을 제거하거나 마스킹하는 것을 권장합니다.
디버깅 목적이라면 fcmTokenId만 로깅하거나, 필요시 별도의 디버그 레벨 로그로 분리하는 방안을 고려하세요.
🔒 PII 로깅 제거 제안
failCount++;
log.warn(
- "FCM 전송 실패(다음 토큰으로 계속) userId={}, fcmTokenId={}, errorCode={}",
- userId,
+ "FCM 전송 실패(다음 토큰으로 계속) fcmTokenId={}, errorCode={}",
token.getId(),
e.getErrorCode()
); if (failCount > 0) {
- log.warn("FCM 일부 토큰 실패 userId={}, 성공={}, 실패={}", userId, successCount, failCount);
+ log.warn("FCM 일부 토큰 실패 성공={}, 실패={}", successCount, failCount);
}📝 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.
Suggested change
| public void sendToUser(Long userId, String title, String body) { | |
| List<FcmToken> tokens = fcmTokenFinder.getActiveTokensByUserId(userId); | |
| if (tokens.isEmpty()) { | |
| return; | |
| } | |
| int successCount = 0; | |
| int failCount = 0; | |
| for (FcmToken token : tokens) { | |
| try { | |
| sendToToken(token.getToken(), title, body); | |
| successCount++; | |
| } catch (BusinessException e) { | |
| failCount++; | |
| log.warn( | |
| "FCM 전송 실패(다음 토큰으로 계속) userId={}, fcmTokenId={}, errorCode={}", | |
| userId, | |
| token.getId(), | |
| e.getErrorCode() | |
| ); | |
| } | |
| } | |
| if (failCount > 0 && successCount == 0) { | |
| throw new BusinessException(ErrorCode.FCM_SEND_FAILED, "활성 토큰 전송이 모두 실패했습니다."); | |
| } | |
| if (failCount > 0) { | |
| log.warn("FCM 일부 토큰 실패 userId={}, 성공={}, 실패={}", userId, successCount, failCount); | |
| } | |
| } | |
| public void sendToUser(Long userId, String title, String body) { | |
| List<FcmToken> tokens = fcmTokenFinder.getActiveTokensByUserId(userId); | |
| if (tokens.isEmpty()) { | |
| return; | |
| } | |
| int successCount = 0; | |
| int failCount = 0; | |
| for (FcmToken token : tokens) { | |
| try { | |
| sendToToken(token.getToken(), title, body); | |
| successCount++; | |
| } catch (BusinessException e) { | |
| failCount++; | |
| log.warn( | |
| "FCM 전송 실패(다음 토큰으로 계속) fcmTokenId={}, errorCode={}", | |
| token.getId(), | |
| e.getErrorCode() | |
| ); | |
| } | |
| } | |
| if (failCount > 0 && successCount == 0) { | |
| throw new BusinessException(ErrorCode.FCM_SEND_FAILED, "활성 토큰 전송이 모두 실패했습니다."); | |
| } | |
| if (failCount > 0) { | |
| log.warn("FCM 일부 토큰 실패 성공={}, 실패={}", successCount, failCount); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/souzip/application/notification/FcmNotificationService.java`
around lines 59 - 86, The logs in sendToUser leak a user identifier (userId)
which is PII; remove or mask userId in all log.warn calls inside sendToUser and
instead log only the non-PII token id (token.getId()) and error info
(e.getErrorCode()), or move detailed user-linked logs to a debug-only message.
Concretely, update the catch block that calls log.warn("FCM 전송 실패... userId={},
fcmTokenId={}, errorCode={}") to omit or mask userId and keep fcmTokenId and
errorCode, and update the later log.warn("FCM 일부 토큰 실패 userId={}, 성공={}, 실패={}")
to remove or mask userId (e.g., log only success/fail counts or use a
debug-level message that includes a masked userId). Ensure these changes are
applied in the sendToUser method and that references to FcmToken.getId(),
sendToToken, and BusinessException handling remain intact.
Source: Coding guidelines
Comment on lines
+29
to
+47
| public static PushBroadcastHistory record( | ||
| UUID adminId, | ||
| String title, | ||
| String body, | ||
| int totalTargets, | ||
| int successCount, | ||
| int failCount, | ||
| boolean firebaseConfigured | ||
| ) { | ||
| PushBroadcastHistory row = new PushBroadcastHistory(); | ||
| row.adminId = requireNonNull(adminId, "관리자 ID는 필수입니다."); | ||
| row.title = requireNonNull(title, "제목은 필수입니다."); | ||
| row.body = requireNonNull(body, "본문은 필수입니다."); | ||
| row.totalTargets = totalTargets; | ||
| row.successCount = successCount; | ||
| row.failCount = failCount; | ||
| row.firebaseConfigured = firebaseConfigured; | ||
| return row; | ||
| } |
There was a problem hiding this comment.
카운트 필드에 대한 음수 검증을 추가하세요.
totalTargets, successCount, failCount 필드는 음수가 될 수 없는 비즈니스 의미를 가지지만, 현재 record 메서드에서 이에 대한 검증이 없습니다. 음수 값이 저장되면 데이터 무결성 문제가 발생할 수 있습니다. 도메인 모델 레벨에서 방어적으로 검증하는 것이 좋습니다.
🛡️ 제안: 카운트 필드 음수 검증 추가
public static PushBroadcastHistory record(
UUID adminId,
String title,
String body,
int totalTargets,
int successCount,
int failCount,
boolean firebaseConfigured
) {
PushBroadcastHistory row = new PushBroadcastHistory();
row.adminId = requireNonNull(adminId, "관리자 ID는 필수입니다.");
row.title = requireNonNull(title, "제목은 필수입니다.");
row.body = requireNonNull(body, "본문은 필수입니다.");
+ if (totalTargets < 0) {
+ throw new IllegalArgumentException("전체 타겟 수는 0 이상이어야 합니다.");
+ }
+ if (successCount < 0) {
+ throw new IllegalArgumentException("성공 카운트는 0 이상이어야 합니다.");
+ }
+ if (failCount < 0) {
+ throw new IllegalArgumentException("실패 카운트는 0 이상이어야 합니다.");
+ }
row.totalTargets = totalTargets;
row.successCount = successCount;
row.failCount = failCount;
row.firebaseConfigured = firebaseConfigured;
return row;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/souzip/domain/notification/PushBroadcastHistory.java`
around lines 29 - 47, In PushBroadcastHistory.record, add defensive validation
that the integer count fields totalTargets, successCount, and failCount are
non-negative: if any is < 0 throw an IllegalArgumentException (or use
Objects.checkIndex-style validation) with a clear message (e.g., "totalTargets
must be non-negative"), then proceed to assign the fields as before; keep the
existing requireNonNull checks for adminId/title/body and update only the record
method to enforce these non-negative checks for totalTargets, successCount and
failCount.
Develop-KIM
changed the title
- FcmTokenNotFoundException: 예외 메시지에서 토큰/디바이스 ID 제거 (민감정보 노출 방지) - FcmNotificationService: FirebaseMessagingException 메시지를 그대로 전파하지 않고 FcmSendException(오류 코드만 보존)으로 감싸 민감정보 누출 차단 - 영구 실패(UNREGISTERED/INVALID_ARGUMENT/SENDER_ID_MISMATCH) 토큰은 전송 후 비활성화하여 이후 broadcast/사용자 알림에서 반복 실패하지 않도록 처리 - FcmTokenCommandService.deactivateByIds, FcmTokenRepository.findAllByIdIn 추가 - 관련 단위 테스트 추가 (전체 408개 통과) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
리뷰 반영 (7e5e5f0)✅ 적용
⏭️ 별도 PR로 분리 권장
🙅 미적용 (사유)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e5e5f0686
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| messaging.send(messageBuilder.build()); | ||
| } catch (FirebaseMessagingException e) { | ||
| log.error("FCM 전송 실패 tokenPrefix={} error={}", maskToken(registrationToken), e.getMessagingErrorCode(), e); |
There was a problem hiding this comment.
medium: 아래 주석처럼 FirebaseMessagingException 메시지에는 원본 registration token 같은 민감 정보가 섞일 수 있는데, log.error(..., e)로 throwable을 그대로 넘기면 stack trace의 Exception.toString()에 그 메시지가 다시 출력됩니다. FCM이 잘못된 토큰 값을 메시지에 포함하는 실패 케이스에서는 maskToken()과 무관하게 운영 로그에 토큰이 남을 수 있으니, error code/마스킹 값만 로깅하거나 throwable 메시지를 sanitize해 주세요.
Useful? React with 👍 / 👎.
Comment on lines
+23
to
+26
| public boolean isPermanentFailure() { | ||
| return messagingErrorCode == MessagingErrorCode.UNREGISTERED | ||
| || messagingErrorCode == MessagingErrorCode.INVALID_ARGUMENT | ||
| || messagingErrorCode == MessagingErrorCode.SENDER_ID_MISMATCH; |
There was a problem hiding this comment.
medium: INVALID_ARGUMENT는 stale token뿐 아니라 잘못된 message/data payload에도 반환될 수 있습니다. 예를 들어 sendToToken(..., data) 호출에서 지원하지 않는 data key/값 때문에 FCM이 INVALID_ARGUMENT를 반환하면, 사용자 전송이나 broadcast 루프가 유효한 토큰들을 모두 영구 실패로 분류해 active=false로 바꿀 수 있습니다. 토큰 자체가 무효인 경우만 비활성화하도록 오류 세부 원인이나 토큰 검증 경로를 분리해 주세요.
Useful? React with 👍 / 👎.
| @ViewerAccess | ||
| @GetMapping("/broadcast/history") | ||
| public SuccessResponse<PaginationResponse<PushBroadcastHistoryResponse>> broadcastHistory( | ||
| @ModelAttribute PaginationRequest paginationRequest |
There was a problem hiding this comment.
medium: 신규 history API의 RestDocs는 page/size를 노출하지만, 여기서 받는 PaginationRequest는 기존 컨벤션처럼 pageNo/pageSize 생성자 파라미터만 바인딩합니다. 문서대로 ?page=2&size=20을 보내는 클라이언트는 값이 무시되어 기본값(1, 10)으로만 조회하게 되므로, 문서/테스트를 pageNo/pageSize로 맞추거나 이 endpoint에서 @RequestParam page/size를 명시적으로 받아 변환해 주세요.
Useful? React with 👍 / 👎.
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.
개요
롤백(SOU-638)으로 develop에서 제외됐던 FCM 푸시 알림 기능을 develop의 새 패키지 구조(
shared,auth.application)에 맞춰 재이식합니다. 더불어 회원 탈퇴 시 FCM 토큰이 정리되지 않던 문제도 함께 해결합니다.주요 변경
기능
FcmTokenController,FcmTokenCommandService)shared.config.FirebaseConfig/FirebaseProperties)FcmNotificationService)AdminPushApi,PushBroadcastHistory)UserService.withdraw→deactivateAllByUserId)인프라/매핑
ErrorCode.FCM_SEND_FAILED추가orm.xml에FcmToken,PushBroadcastHistory엔티티 매핑 추가V16__create_push_broadcast_history마이그레이션 추가 (develop의 V14 fcm_tokens와 정합)build.gradle에firebase-admin의존성 추가테스트/문서
app/notification.adoc,admin/push.adocAPI 문서 추가 및 index include패키지 이식 매핑
global.exceptionshared.exceptionglobal.commonshared.commonglobal.config(Firebase)shared.configglobal.security.annotationauth.adapter.security.annotationdomain.shared.BaseEntityshared.domain.BaseEntity🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Dependencies
Documentation