Skip to content

feat(chat): membership-based access control (ChatRoomMember + group/personal room policies)#21

Merged
ca5tlechan merged 4 commits into
developfrom
feature/chat-room-member
May 17, 2026
Merged

feat(chat): membership-based access control (ChatRoomMember + group/personal room policies)#21
ca5tlechan merged 4 commits into
developfrom
feature/chat-room-member

Conversation

@ThonApple
Copy link
Copy Markdown
Collaborator

@ThonApple ThonApple commented May 16, 2026

Summary

  • 채팅 도메인에 첫 클래스 멤버십 (ChatRoomMember) 도입. "이 방에 누가 속해 있는가"가 메시지 히스토리에 암묵적으로 의존하지 않고 DB 레벨로 강제됨.
  • GET /rooms, getMessages, sendMessage, markAsRead 등 모든 채팅 작업이 멤버십 검증 (403 CH003) 통과해야 함.
  • 그룹 채팅방 = (spot_id) 당 활성 1개 (partial unique index) — 같은 spot으로 재호출 시 idempotent.
  • 1:1 채팅방 = 카카오톡 스타일 재사용 (A↔B 다시 시작하면 기존 방 반환).
  • Spot 매칭(OPEN → MATCHED) 시 author + 참가자가 자동으로 채팅 멤버로 가입.
  • PERSONAL 방의 title이 상대방 닉네임으로 자동 채워짐.

Changes

Domain

  • New ChatRoomMember entity (chat_room_members)
    • UNIQUE (chat_room_id, user_id) + INDEX (user_id)
  • New ChatRoomMemberRepository
    • 멤버 존재/조회 헬퍼
    • findChatRoomIdsByUserId — "내가 속한 방"
    • findPersonalRoomIdsBetween(a, b) — PERSONAL 재사용 정책 핵심 쿼리
  • ChatRoom + name, imageUrl, postId(String UUID), isDeleted
  • ChatRoomRepository + findFirstBySpotIdAndTypeAndIsDeletedFalse

Service

  • ChatService.createRoom GROUP 전용 + idempotent
  • ChatService.createPersonalRoom 신규 (KakaoTalk 재사용 정책)
  • ChatService.ensureGroupRoomForSpot 노출 — SpotService에서 호출
  • 전 메서드 멤버십 검증
  • ChatRoomResponse PERSONAL title을 partner nickname으로

Spot integration

  • SpotService.matchSpot이 매칭 직후 author + 참가자를 채팅 멤버로 union 가입

Migration

  • docs/migrations/2026-05-15_chat_room_member.sql
    • chat_room_members 신규
    • chat_rooms 컬럼 확장
    • (spot_id) WHERE type='GROUP' AND is_deleted=false partial unique index
    • 기존 chat_messages.sender_id로부터 멤버십 백필

Error codes

  • CH003 CHAT_ROOM_ACCESS_DENIED (403)
  • CH004 CHAT_PARTNER_NOT_FOUND (404)
  • CH005 CHAT_PERSONAL_SELF_NOT_ALLOWED (400)
  • CH006 CHAT_PERSONAL_REQUIRES_PARTNER (400)

Side fixes (작은 우회 hotfix)

  • 기존 JWTFilter가 SecurityContext에 String email 만 principal로 셋팅하는 버그 우회 — ChatController/SpotController에서 String이면 UserRepository.findByEmail로 진짜 userId로 풀어줌. (TODO 주석으로 별도 PR에서 JWTFilter 자체 리팩터링 예정)
  • 이전 마이그레이션 스크립트(2026-05-14_spot_vote_answer_multiselect.sql) idempotent화 (재실행 가능)

Test plan

  • 회원가입/로그인 → JWT 토큰 발급
  • POST /api/chat/rooms (GROUP) — 본인 자동 멤버 등록
  • GET /api/chat/rooms — 본인 방만 필터됨, currentUserId/currentUserName 정상
  • 같은 spotId로 POST /api/chat/rooms 재호출 — idempotent (같은 id 반환)
  • 비멤버가 다른 방 GET /rooms/{id} — 403 CH003
  • POST /api/chat/rooms/personal — partnerNickname/title 정상
  • PERSONAL 재호출 — 기존 방 재사용
  • PERSONAL 자기 자신 — 400 CH005
  • POST /api/spots/{id}/match → GROUP 방 자동 생성 + 본인 멤버 가입
  • 멤버 메시지 송수신 200
  • 비멤버 메시지 송신 403 CH003

Follow-ups (별도 PR)

  • PR B — 읽음 추적 + 실 unreadCount
  • PR C — 나가기
  • PR D — 차단
  • Auth hotfix — JWTFilter가 principal에 CustomUserDetails를 셋팅하도록 리팩터링 (현재 컨트롤러 단위 우회 제거)
  • Spot hotfix — createSpot 시 author를 spot_participants에 OWNER role로 자동 등록

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Personal 1:1 chats with partner info and display metadata (name, image, post link).
    • Membership-based chat access; only members see rooms/messages.
    • Idempotent group-room provisioning when spots are matched; automatic room creation for participants.
    • Soft-delete for chat rooms to avoid duplicates.
  • Bug Fixes

    • Prevent starting personal chats with yourself; ensure correct sender identity on messages.
  • Chores

    • Database migrations added/updated for chat membership and room metadata.

Review Change Stack

ThonApple and others added 3 commits May 15, 2026 16:45
Hibernate ddl-auto=update already creates uq_vote_user_option on app
boot, causing the migration's plain ADD CONSTRAINT to fail with
"already exists" on re-runs. Guard with pg_constraint lookup so the
script can be safely re-applied in any environment.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds first-class membership to the chat domain so that "who can see /
write in which room" is a hard, queryable contract rather than implicit
from message history.

## Domain

- New ChatRoomMember entity (chat_room_members)
  - UNIQUE (chat_room_id, user_id) — one membership row per (room, user)
  - INDEX (user_id) — fast "rooms I'm in" lookup
- ChatRoom gains name / imageUrl / postId (String UUID — Post.id is UUID)
  / isDeleted columns
- New ChatRoomMemberRepository with:
  - membership exists/find/count helpers
  - findChatRoomIdsByUserId for filtered list endpoints
  - findPersonalRoomIdsBetween(a, b) — PERSONAL reuse policy core query
- ChatRoomRepository: findFirstBySpotIdAndTypeAndIsDeletedFalse for the
  GROUP-per-spot idempotent reuse path

## Service

- ChatService.getRooms / getRoom / getMessages / sendMessage / markAsRead
  now hard-require membership; non-members get CH003 (FORBIDDEN)
- ChatService.createRoom restricted to GROUP and idempotent on (spot_id):
  returning the existing room when one is already open
- ChatService.createPersonalRoom: KakaoTalk-style reuse — if A↔B already
  have a PERSONAL room, return it; otherwise create with both members
- ChatService.ensureGroupRoomForSpot exposed so SpotService can auto-add
  participants on matchSpot without reaching into chat internals
- ChatRoomResponse: PERSONAL title/subtitle now resolves to the actual
  partner's nickname (was placeholder "대화방"). New partnerId /
  partnerNickname fields surface the resolved partner for the client.

## Spot integration

- SpotService.matchSpot now calls ensureGroupRoomForSpot with the spot's
  participants, so the group chat exists and is populated the moment a
  spot transitions OPEN → MATCHED.

## Migration

- docs/migrations/2026-05-15_chat_room_member.sql
  - CREATE chat_room_members + indices
  - ALTER chat_rooms ADD name / image_url / post_id / is_deleted
  - Partial unique index (spot_id) WHERE type='GROUP' AND is_deleted=false
    AND spot_id IS NOT NULL — enforces "one active group room per spot"
    at the DB level (JPA can't express partial unique declaratively)
  - Backfill: existing chat_messages.sender_id pairs become membership
    rows so pre-existing rooms remain reachable for prior participants

## Error codes

- CH003 CHAT_ROOM_ACCESS_DENIED (403)
- CH004 CHAT_PARTNER_NOT_FOUND (404)
- CH005 CHAT_PERSONAL_SELF_NOT_ALLOWED (400)
- CH006 CHAT_PERSONAL_REQUIRES_PARTNER (400)

PR B (unread tracking) and PR C (leave) build on this foundation.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…cipal

The existing JWTFilter (capstone-common) populates the SecurityContext
with a UsernamePasswordAuthenticationToken whose principal is the raw
email String, not a CustomUserDetails. Controllers that ask for
@AuthenticationPrincipal CustomUserDetails therefore receive null, and
controllers that fall back to `instanceof String userId` end up storing
the email where the userId should go (e.g. authorId == email).

Until JWTFilter itself is refactored to seat a CustomUserDetails, work
around the bug at the edge:

- ChatController#currentUserId now accepts Object and dispatches:
  CustomUserDetails -> getUserId(); String email -> UserRepository
  .findByEmail -> id; otherwise null. Injects UserRepository for the
  lookup.
- SpotController#resolveCurrentUserId applies the same dispatch so
  createSpot stops persisting email as authorId.

Also harden SpotService.matchSpot so the chat-room backfill survives
the existing data gap where createSpot does not insert the author into
spot_participants: we now union the spot's authorId with the
participant list before calling ChatService.ensureGroupRoomForSpot,
guaranteeing the author (and any future real participants) become
chat members the moment a spot transitions to MATCHED.

TODO (separate PR): refactor JWTFilter to seat CustomUserDetails and
register the author as an OWNER participant inside createSpot, then
remove these workarounds.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

Warning

Rate limit exceeded

@ThonApple has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 60 minutes before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b71f131e-8edc-432c-bcc4-f18b9f5e0ff6

📥 Commits

Reviewing files that changed from the base of the PR and between caa9419 and d7a675d.

📒 Files selected for processing (13)
  • capstone-api/src/main/java/backend/chat/controller/ChatController.java
  • capstone-api/src/main/java/backend/chat/dto/ChatRoomResponse.java
  • capstone-api/src/main/java/backend/chat/dto/CreatePersonalChatRoomRequest.java
  • capstone-api/src/main/java/backend/chat/service/ChatService.java
  • capstone-api/src/main/java/backend/spot/controller/SpotController.java
  • capstone-api/src/main/java/backend/spot/service/SpotService.java
  • capstone-common/src/main/java/backend/global/error/exception/ErrorCode.java
  • capstone-domain/src/main/java/backend/chat/entity/ChatRoom.java
  • capstone-domain/src/main/java/backend/chat/entity/ChatRoomMember.java
  • capstone-domain/src/main/java/backend/chat/repository/ChatRoomMemberRepository.java
  • capstone-domain/src/main/java/backend/chat/repository/ChatRoomRepository.java
  • docs/migrations/2026-05-14_spot_vote_answer_multiselect.sql
  • docs/migrations/2026-05-15_chat_room_member.sql
📝 Walkthrough

Walkthrough

Adds membership-based access control and personal (1:1) chats, resolves String principals via UserRepository lookup, enriches chat responses with partner metadata, provisions idempotent GROUP rooms for spots on match, and migrates/backfills membership data.

Changes

Chat Room Membership and Personal Chats

Layer / File(s) Summary
Chat error codes
capstone-common/src/main/java/backend/global/error/exception/ErrorCode.java
Four new error codes added: CHAT_ROOM_ACCESS_DENIED, CHAT_PARTNER_NOT_FOUND, CHAT_PERSONAL_SELF_NOT_ALLOWED, CHAT_PERSONAL_REQUIRES_PARTNER.
Chat room and membership domain model
capstone-domain/src/main/java/backend/chat/entity/ChatRoom.java, capstone-domain/src/main/java/backend/chat/entity/ChatRoomMember.java, capstone-domain/src/main/java/backend/chat/repository/ChatRoomRepository.java
ChatRoom extended with postId, name, imageUrl, and soft-delete isDeleted plus markDeleted(); new ChatRoomMember entity maps membership with joinedAt; repository query added for non-deleted GROUP lookup by spotId.
Membership query and lookup
capstone-domain/src/main/java/backend/chat/repository/ChatRoomMemberRepository.java
New repository with derived queries and custom JPQL for finding personal room IDs between two users, user/room lookups, and grouped member counts.
Database migration and backfill
docs/migrations/2026-05-15_chat_room_member.sql
Creates chat_room_members table, adds name, image_url, post_id, is_deleted to chat_rooms, creates partial unique index uq_chat_room_group_spot, and backfills members from chat_messages.
Idempotent vote-answer migration
docs/migrations/2026-05-14_spot_vote_answer_multiselect.sql
Changes unconditional unique-constraint addition to a conditional DO block that checks pg_constraint before adding the constraint.
Authentication principal resolution
capstone-api/src/main/java/backend/chat/controller/ChatController.java, capstone-api/src/main/java/backend/spot/controller/SpotController.java
Controllers now accept @AuthenticationPrincipal Object principal and resolve currentUserId by returning null for anonymous, extracting userId from CustomUserDetails, or treating String principals as emails and resolving via UserRepository.findByEmail(...).
ChatRoomResponse & personal-room DTOs
capstone-api/src/main/java/backend/chat/dto/ChatRoomResponse.java, capstone-api/src/main/java/backend/chat/dto/CreatePersonalChatRoomRequest.java
Adds postId, name, imageUrl, partnerId, partnerNickname, PERSONAL_DEFAULT_TITLE; updates from() and title/subtitle logic to use partner enrichment; new CreatePersonalChatRoomRequest with required partnerId.
Membership-based access control in ChatService
capstone-api/src/main/java/backend/chat/service/ChatService.java
Adds assertMembership(...), filters room listings to memberships, excludes deleted rooms, enforces membership on getRoom, getMessages, sendMessage, and markAsRead, and sets senderId from authenticated user.
Idempotent room creation and personal chats
capstone-api/src/main/java/backend/chat/service/ChatService.java
ensureMember(...) idempotently adds members; ensureGroupRoomForSpot(...) reuses or creates a single active GROUP room per spotId; createPersonalRoom(...) reuses or creates PERSONAL rooms and ensures both participants are members, soft-deleting duplicates on race.
Chat room endpoints with Object principal
capstone-api/src/main/java/backend/chat/controller/ChatController.java
All chat endpoints updated to accept @AuthenticationPrincipal Object principal and pass currentUserId(principal) to ChatService; adds /rooms/personal POST endpoint for personal chat creation.
Spot-chat provisioning on match
capstone-api/src/main/java/backend/spot/service/SpotService.java
SpotService injects ChatService and calls ensureGroupRoomForSpot(spotId, memberUserIds) when a spot is matched to provision the group chat.

Sequence Diagram(s)

sequenceDiagram
  participant User as Authenticated User
  participant ChatCtrl as ChatController
  participant ChatSvc as ChatService
  participant ChatRepo as ChatRoomRepository
  participant MemberRepo as ChatRoomMemberRepository
  participant DB as Database

  User->>ChatCtrl: POST /rooms/personal (partnerId)
  ChatCtrl->>ChatSvc: createPersonalRoom(partnerId, currentUserId)
  ChatSvc->>MemberRepo: findPersonalRoomIdsBetween(currentUserId, partnerId)
  MemberRepo->>DB: JPQL query for personal room ids
  DB-->>MemberRepo: roomId(s) or empty
  alt room exists
    ChatSvc->>MemberRepo: ensureMember(roomId, currentUserId)
    MemberRepo->>DB: insert if not exists
    ChatSvc->>MemberRepo: ensureMember(roomId, partnerId)
    MemberRepo->>DB: insert if not exists
  else room missing
    ChatSvc->>ChatRepo: save new ChatRoom(type=PERSONAL)
    ChatRepo->>DB: insert chat_room
    ChatSvc->>MemberRepo: ensureMember(newRoomId, currentUserId)
    MemberRepo->>DB: insert member
    ChatSvc->>MemberRepo: ensureMember(newRoomId, partnerId)
    MemberRepo->>DB: insert member
  end
  ChatSvc->>ChatCtrl: return ChatRoom
  ChatCtrl->>ChatSvc: buildEnrichments([room], currentUserId)
  ChatSvc->>MemberRepo: findUserIdsByChatRoomId(roomId)
  MemberRepo->>DB: query other member user id
  ChatSvc-->>ChatCtrl: ChatRoomResponse with partner metadata
  ChatCtrl-->>User: 200 OK + ChatRoomResponse
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • spot-platform/backend#19: Modifies chat message authentication and sender resolution overlapping with this PR's principal handling and sendMessage changes.
  • spot-platform/backend#14: Earlier chat enrichment and title computation changes that this PR extends with partner metadata and personal chat logic.
  • spot-platform/backend#8: Prior chat/spot implementation that this PR builds upon for membership and personal chat flows.

Suggested labels

feature

Suggested reviewers

  • ca5tlechan
  • hoTan35

Poem

🐰 I hopped through rooms both group and two,

Ensured each member had a view,
Resolved the stranger-principal by mail,
Soft-delete kept duplicates pale,
Now partners chat — a carrot tale!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: introducing membership-based access control for chat rooms with both group and personal room policies.
Description check ✅ Passed The description is comprehensive and follows the template structure with Summary, Changes (organized by subsystem), and Test plan sections; all required information is present and well-organized.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/chat-room-member

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Nitpick comments (1)
capstone-api/src/main/java/backend/chat/controller/ChatController.java (1)

180-193: ⚡ Quick win

Inconsistent fallback with SpotController.resolveCurrentUserId.

When findByEmail fails, this method returns null, but SpotController falls back to the original string (treating it as a potential userId). This inconsistency means the same authentication state may behave differently across controllers.

Additionally, both controllers duplicate nearly identical principal-resolution logic. Consider extracting to a shared utility (e.g., AuthPrincipalResolver) to ensure consistent behavior and simplify the planned JWTFilter refactor.

♻️ Suggested shared utility

Create a shared component that both controllers can inject:

`@Component`
`@RequiredArgsConstructor`
public class AuthPrincipalResolver {

    private final UserRepository userRepository;

    public String resolveUserId(Object principal, Authentication authentication) {
        if (authentication == null || !authentication.isAuthenticated()
            || authentication instanceof AnonymousAuthenticationToken) {
            return null;
        }
        if (principal == null || "anonymousUser".equals(principal)) {
            return null;
        }
        if (principal instanceof CustomUserDetails userDetails) {
            return userDetails.getUserId();
        }
        if (principal instanceof String email) {
            return userRepository.findByEmail(email)
                .map(UserEntity::getId)
                .orElse(null);  // or throw if auth is expected
        }
        return null;
    }
}
🤖 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 `@capstone-api/src/main/java/backend/chat/controller/ChatController.java`
around lines 180 - 193, Extract the duplicated principal-resolution logic from
ChatController.currentUserId and SpotController.resolveCurrentUserId into a new
injectable component AuthPrincipalResolver (constructor-inject UserRepository)
with a single method resolveUserId(Object principal, Authentication
authentication) that performs the shared checks (null/anonymous/authenticated,
CustomUserDetails -> getUserId, String principal -> attempt
userRepository.findByEmail(...)). Make the behavior consistent: if principal is
a String and findByEmail fails, return the original String (matching
SpotController) instead of null, and update both controllers to call
authPrincipalResolver.resolveUserId(principal, authentication) (or similar) so
they share identical fallback semantics and are ready for the JWTFilter
refactor.
🤖 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 `@capstone-api/src/main/java/backend/chat/service/ChatService.java`:
- Around line 74-75: The list endpoints currently populate "rooms" via roomIds
-> chatRoomRepository.findAllById(roomIds) without excluding soft-deleted rooms,
so update those list paths (the block that sets the rooms variable, and similar
logic around the other occurrences noted near the findRoomOrThrow use at the
other ranges) to filter out rooms where isDeleted == true; either call a
repository method that accepts roomIds and isDeleted=false (e.g., add/use
findAllByIdAndIsDeletedFalse or similar) or post-filter the results from
chatRoomRepository.findAllById(roomIds) to remove r.isDeleted(), ensuring the
same soft-delete rule as findRoomOrThrow is applied before returning room lists.
- Around line 138-152: The current create-or-reuse flow in ChatService (the
existing -> room selection using
chatRoomMemberRepository.findPersonalRoomIdsBetween,
chatRoomRepository.findById, and chatRoomRepository.save of a ChatRoom with
ChatRoomType.PERSONAL) is racy and can produce duplicate PERSONAL rooms; fix it
by enforcing a canonical uniqueness constraint at the DB level (a unique index
on a canonical user-pair key stored on ChatRoom or a separate unique
personal_room key derived from member IDs) and update the ChatService flow to
handle unique-key conflicts: attempt insert, on unique-constraint violation
re-query the existing room (using the same findPersonalRoomIdsBetween/findById
logic) and return that result (or alternatively acquire a DB row lock on a
canonical lock-row before creating); ensure the code references the same
ChatRoom builder/save path and retries/read-after-write on conflict to guarantee
deterministic reuse.
- Around line 99-107: Race condition: two concurrent callers can both miss
findFirstBySpotIdAndTypeAndIsDeletedFalse and attempt
chatRoomRepository.save(...) causing a unique constraint failure; make creation
idempotent by re-query-on-conflict. Change the create logic in ChatService
around findFirstBySpotIdAndTypeAndIsDeletedFalse / ChatRoom.builder(...) /
chatRoomRepository.save(...) to: first try to find, if absent attempt to save
inside a try block, catch the persistence exception that indicates a duplicate
key (e.g. DataIntegrityViolationException or DuplicateKeyException), and in the
catch simply re-run findFirstBySpotIdAndTypeAndIsDeletedFalse and return that
result; apply the same pattern to the other occurrence (the block at lines
253-261) so concurrent creates become race-safe and return the existing room on
conflict.
- Around line 237-245: The current ensureMember logic does a pre-check using
chatRoomMemberRepository.existsByChatRoomIdAndUserId(...) followed by
chatRoomMemberRepository.save(...), which is vulnerable to a TOCTOU race and
will throw on concurrent inserts; change ensureMember to attempt the insert
unconditionally and treat unique-key violations as a harmless no-op by catching
the appropriate persistence exception (e.g., DataIntegrityViolationException or
the underlying constraint/duplicate-key exception depending on your JPA/DB),
leaving other exceptions rethrown, and ensure you still construct the
ChatRoomMember via
ChatRoomMember.builder().chatRoomId(roomId).userId(userId).build() before
saving.

In `@capstone-api/src/main/java/backend/spot/controller/SpotController.java`:
- Around line 281-287: The current fallback in the principal String branch
returns the original emailOrId (in SpotController.principal handling), which can
pass an email into downstream code expecting a userId; change the branch that
calls
userRepository.findByEmail(emailOrId).map(UserEntity::getId).orElse(emailOrId)
to instead return null when the lookup is empty so it matches
ChatController.currentUserId behavior; update the return path inside the
principal instanceof String emailOrId block (leave the JWTFilter/TODO note
intact) and ensure callers handle a null userId consistently.

In `@capstone-api/src/main/java/backend/spot/service/SpotService.java`:
- Around line 159-162: The stream in SpotService that collects memberUserIds
from spotParticipantRepository.findBySpotId(spotId) currently includes all
participant rows; change it to only include participants with an active status
by adding a predicate before mapping (e.g., filter(p -> p.getStatus() ==
SpotParticipant.Status.ACTIVE) or filter(SpotParticipant::isActive) depending on
the model) and keep null-safety for userId (filter uid -> uid != null &&
!uid.isBlank()). Update the pipeline on the SpotService method that builds
memberUserIds so non-ACTIVE participants are excluded from auto-join.

In
`@capstone-domain/src/main/java/backend/chat/repository/ChatRoomMemberRepository.java`:
- Around line 33-45: The query in findPersonalRoomIdsBetween only checks
membership and can return non-personal or deleted rooms; update the JPQL to join
ChatRoom (e.g., join ChatRoom cr on m.chatRoomId = cr.id) and add filters
cr.type = PERSONAL and cr.isDeleted = false in both the outer query and the
inner subquery (or apply the join+filters once so both membership constraints
only consider rooms where ChatRoom.type == PERSONAL and ChatRoom.isDeleted ==
false) so the method returns only active personal rooms.

In `@docs/migrations/2026-05-15_chat_room_member.sql`:
- Around line 60-73: The backfill only inserts members from
chat_messages.sender_id, which misses historical room members who never sent
messages; update the migration to also insert membership rows from other sources
(e.g., chat_rooms.owner_id, any participants/metadata columns such as
chat_rooms.participants JSON/array) by adding additional INSERT ... SELECT
blocks or a single INSERT ... SELECT with UNIONs into chat_room_members
(chat_room_id, user_id, joined_at) for those sources, and/or add an explicit
reconciliation step (a transaction that deduplicates and ensures required rows
exist) before enabling strict membership enforcement so reads/sends won't be
denied; reference the existing INSERT INTO chat_room_members ... SELECT ... FROM
chat_messages m and extend it to include chat_rooms.owner_id and
participants-derived user_ids and use ON CONFLICT (chat_room_id, user_id) DO
NOTHING to avoid duplicates.

---

Nitpick comments:
In `@capstone-api/src/main/java/backend/chat/controller/ChatController.java`:
- Around line 180-193: Extract the duplicated principal-resolution logic from
ChatController.currentUserId and SpotController.resolveCurrentUserId into a new
injectable component AuthPrincipalResolver (constructor-inject UserRepository)
with a single method resolveUserId(Object principal, Authentication
authentication) that performs the shared checks (null/anonymous/authenticated,
CustomUserDetails -> getUserId, String principal -> attempt
userRepository.findByEmail(...)). Make the behavior consistent: if principal is
a String and findByEmail fails, return the original String (matching
SpotController) instead of null, and update both controllers to call
authPrincipalResolver.resolveUserId(principal, authentication) (or similar) so
they share identical fallback semantics and are ready for the JWTFilter
refactor.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0b941ef3-22d2-4a80-801e-ecc4874146f1

📥 Commits

Reviewing files that changed from the base of the PR and between caa9419 and 4adfd75.

📒 Files selected for processing (13)
  • capstone-api/src/main/java/backend/chat/controller/ChatController.java
  • capstone-api/src/main/java/backend/chat/dto/ChatRoomResponse.java
  • capstone-api/src/main/java/backend/chat/dto/CreatePersonalChatRoomRequest.java
  • capstone-api/src/main/java/backend/chat/service/ChatService.java
  • capstone-api/src/main/java/backend/spot/controller/SpotController.java
  • capstone-api/src/main/java/backend/spot/service/SpotService.java
  • capstone-common/src/main/java/backend/global/error/exception/ErrorCode.java
  • capstone-domain/src/main/java/backend/chat/entity/ChatRoom.java
  • capstone-domain/src/main/java/backend/chat/entity/ChatRoomMember.java
  • capstone-domain/src/main/java/backend/chat/repository/ChatRoomMemberRepository.java
  • capstone-domain/src/main/java/backend/chat/repository/ChatRoomRepository.java
  • docs/migrations/2026-05-14_spot_vote_answer_multiselect.sql
  • docs/migrations/2026-05-15_chat_room_member.sql

Comment thread capstone-api/src/main/java/backend/chat/service/ChatService.java Outdated
Comment thread capstone-api/src/main/java/backend/chat/service/ChatService.java Outdated
Comment thread capstone-api/src/main/java/backend/chat/service/ChatService.java Outdated
Comment thread capstone-api/src/main/java/backend/chat/service/ChatService.java Outdated
Comment thread capstone-api/src/main/java/backend/spot/service/SpotService.java
Comment on lines +60 to +73
-- 4) 백필: 기존 chat_messages 의 sender_id 를 해당 방 멤버로 등록
-- (방장/단일 참여자 등 메타데이터가 없어도 sender 는 사실상 멤버였음)
INSERT INTO chat_room_members (chat_room_id, user_id, joined_at)
SELECT m.room_id, m.sender_id, MIN(m.created_at)
FROM chat_messages m
WHERE m.sender_id IS NOT NULL
AND m.sender_id <> ''
AND NOT EXISTS (
SELECT 1 FROM chat_room_members crm
WHERE crm.chat_room_id = m.room_id
AND crm.user_id = m.sender_id
)
GROUP BY m.room_id, m.sender_id
ON CONFLICT (chat_room_id, user_id) DO NOTHING;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Backfill scope is too narrow and can revoke valid room access.

Line 62–73 only backfills members from chat_messages.sender_id, so historical users who belonged to a room but never sent a message won’t get membership rows. Since this PR enforces membership for reads/sends, that can produce false CH003 denials post-migration.

Please extend the migration with additional membership sources (e.g., room owner/participants metadata where available) or add an explicit reconciliation step before enabling strict enforcement in production.

🤖 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 `@docs/migrations/2026-05-15_chat_room_member.sql` around lines 60 - 73, The
backfill only inserts members from chat_messages.sender_id, which misses
historical room members who never sent messages; update the migration to also
insert membership rows from other sources (e.g., chat_rooms.owner_id, any
participants/metadata columns such as chat_rooms.participants JSON/array) by
adding additional INSERT ... SELECT blocks or a single INSERT ... SELECT with
UNIONs into chat_room_members (chat_room_id, user_id, joined_at) for those
sources, and/or add an explicit reconciliation step (a transaction that
deduplicates and ensures required rows exist) before enabling strict membership
enforcement so reads/sends won't be denied; reference the existing INSERT INTO
chat_room_members ... SELECT ... FROM chat_messages m and extend it to include
chat_rooms.owner_id and participants-derived user_ids and use ON CONFLICT
(chat_room_id, user_id) DO NOTHING to avoid duplicates.

Closes 8 Major findings from CodeRabbit review.

## Race safety

- ChatService now wraps both GROUP-room creation and ChatRoomMember
  inserts in DataIntegrityViolationException handlers, leveraging the
  partial unique index and (chat_room_id, user_id) UNIQUE constraint
  as the source of truth. Concurrent idempotent calls converge on the
  same row instead of 500-ing.
- createPersonalRoom does a second lookup after insert to detect a
  racing partner room and soft-deletes the newer duplicate, keeping
  the older room as the canonical one. Full canonical-pair uniqueness
  is left as a follow-up (requires a new column + index).
- ensureGroupRoomForSpot now goes through the same findOrCreate helper
  so SpotService.matchSpot inherits the race-safe path.

## Soft-delete leak

- getRooms / getRoomsBySpot / getRoomsByUser filter out isDeleted
  rooms locally, matching findRoomOrThrow's behaviour and preventing
  deleted-room re-emergence when stale membership rows still exist.

## Personal-room lookup correctness

- ChatRoomMemberRepository.findPersonalRoomIdsBetween now joins
  ChatRoom and requires type=PERSONAL AND isDeleted=false. Previously
  a (group room + same two users) edge case could be reused as if it
  were a PERSONAL room.

## Spot integration

- SpotService.matchSpot only joins ACTIVE participants into the chat
  room. LEFT / EXPELLED participants no longer receive auto-membership.

## Auth fallback

- SpotController.resolveCurrentUserId no longer falls through to
  emailOrId when findByEmail misses — it now returns null, matching
  ChatController policy. Prevents email strings from leaking into
  authorId / participant.userId.

## Migration

- Added explicit note about the backfill's scope limitation (silent
  members not represented in chat_messages) and the upgrade path for
  environments that have alternative membership sources.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@ThonApple
Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@ThonApple ThonApple requested review from ca5tlechan and hoTan35 May 16, 2026 15:34
@ThonApple
Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown
Collaborator

@hoTan35 hoTan35 left a comment

Choose a reason for hiding this comment

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

Good👀👍

@ca5tlechan ca5tlechan merged commit 20544a6 into develop May 17, 2026
1 check passed
ThonApple added a commit that referenced this pull request May 17, 2026
…ervice

JWTFilter previously placed a raw email String into the SecurityContext
principal. Controllers that asked for @AuthenticationPrincipal
CustomUserDetails therefore got null, and any "instanceof String"
fallbacks silently treated the email as a userId — which is how
authorId / senderId / participant.userId ended up storing emails in
several places.

This change moves email → user resolution into the filter itself,
using Spring Security's standard UserDetailsService contract. The
existing UserService already implements UserDetailsService and returns
CustomUserDetails, so no new code path is needed — JWTFilter simply
calls loadUserByUsername(email) and seats the returned UserDetails as
the principal.

- Adds UserDetailsService dependency.
- Splits the validation flow (extract → parse/validate → resolve
  principal → seat authentication) into small helpers so each step
  reads top-to-bottom in doFilterInternal.
- Uses an internal AuthenticationRejected signal to keep error paths
  out of the happy path; writeError centralises the JSON response.
- UsernameNotFoundException from UserDetailsService is surfaced as
  INVALID_TOKEN (a token whose subject doesn't resolve to a live user
  is no different from a tampered token).

- jwtFilter bean now receives UserDetailsService via Spring injection.

- Drops the legacy `Object principal + Authentication authentication`
  parameter pair on seven endpoints in favour of a single
  `@AuthenticationPrincipal CustomUserDetails userDetails`, matching
  ChatController.
- resolveCurrentUserId collapses to a one-liner null-guard. The
  Anonymous-token branch is no longer needed: Spring already injects
  null for @AuthenticationPrincipal on anonymous requests.

Once this merges, the per-controller workaround helpers added in
PR #21 (ChatController.currentUserId / SpotController.resolveCurrentUserId
with findByEmail fallback) become redundant and can be deleted in a
cleanup commit.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ThonApple added a commit that referenced this pull request May 17, 2026
PR #21 introduced an Object-principal + findByEmail fallback in
ChatController.currentUserId to compensate for JWTFilter seating a
raw email String as the SecurityContext principal. With JWTFilter now
seating CustomUserDetails directly (this PR), that workaround becomes
dead code: every authenticated request already has a
CustomUserDetails principal, so accepting Object and pattern-matching
to String never matches.

Collapse all nine endpoints back to
`@AuthenticationPrincipal CustomUserDetails userDetails` and reduce
currentUserId to a null-safe one-liner, matching the SpotController
shape this PR also restores.

Removes the no-longer-needed UserRepository / UserEntity imports and
the userRepository field injection.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@ThonApple ThonApple deleted the feature/chat-room-member branch May 17, 2026 10:26
ThonApple added a commit that referenced this pull request May 18, 2026
* docs(capstone): codify PR workflow — full CodeRabbit review + dual reviewer policy

Adds a new section "PR 워크플로우" to capstone.md so any AI worker (or
human) opening a PR follows the same pre-merge checklist:

- Always trigger @coderabbitai full review on PR creation, and report
  rate-limit timing back to the user when blocked.
- Always assign both ca5tlechan and hoTan35 as reviewers — one approval
  unblocks merge.
- Re-trigger after fix pushes, since CodeRabbit may have evaluated the
  pre-fix snapshot.

This makes the policy that we've been applying ad-hoc on PR #21..#27
explicit and discoverable for new contributors.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs(capstone): make reviewer policy relative to current user

Original wording hardcoded "ca5tlechan, hoTan35" as the two reviewers,
which only made sense from 김동현's seat. When ca5tlechan or hoTan35
themselves open PRs they would end up assigning themselves as their
own reviewer.

Re-frame the rule as "the two workers other than yourself" and add an
explicit name ↔ GitHub login mapping table so the AI worker can derive
the correct --reviewer arguments from General Rules #1 (current user
identification) without asking the user every time.

Examples now in the doc:
- 김동현 -> --reviewer "hoTan35,ca5tlechan"
- 이성찬 -> --reviewer "hoTan35,ThonApple"

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants