[FEAT] 하트비트 및 인증로직 추가#246
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough사회자(Chairman) JWT 타입과 발급/검증 경로가 추가되었습니다. REST 엔드포인트로 사회자 토큰을 발급하고, WebSocket 인증 시 해당 토큰을 사용하도록 변경했습니다. STOMP/SockJS 하트비트 설정과 스케줄러 빈도 추가도 포함됩니다. Changes사회자 토큰 발급 및 검증 흐름
STOMP / SockJS 하트비트 및 스케줄러
Sequence Diagram(s)sequenceDiagram
actor Client
participant REST as SharingRestController
participant Service as CustomizeService
participant Repo as CustomizeTableDomainRepository
participant Auth as AuthManager
participant JWT as JwtTokenProvider
Client->>REST: GET /api/share/{tableId}/chairman-token
REST->>Service: findDebateTime(tableId, member)
Service->>Repo: getTotalTimeBoxTimes(tableId)
Repo-->>Service: debateTime
Service-->>REST: debateTime
REST->>Auth: issueChairmanToken(member, debateTime * 2)
Auth->>JWT: createChairmanToken(memberInfo, expiration)
JWT-->>Auth: chairmanToken
Auth-->>REST: chairmanToken
REST-->>Client: ChairmanTokenResponse {chairmanToken}
sequenceDiagram
actor Client
participant SockJS as SockJS/STOMP
participant Resolver as WebSocketAuthMemberResolver
participant Auth as AuthManager
participant JWTR as JwtTokenResolver
participant Handler as SharingWebSocketController
Client->>SockJS: CONNECT (Authorization: chairman token)
SockJS->>Resolver: resolveArgument(Authorization header)
Resolver->>Auth: resolveChairmanToken(token)
Auth->>JWTR: resolveChairmanToken(token)
JWTR-->>Auth: memberEmail
Auth-->>Resolver: memberEmail
Resolver->>Resolver: authService.getMember(email)
Resolver-->>SockJS: Member 객체
Client->>Handler: SEND /app/event/{roomId} (SharingRequest)
Handler->>Handler: share(member, roomId, request)
Handler-->>Client: SharingResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Test Results141 files 141 suites 18s ⏱️ Results for commit 5232baf. ♻️ This comment has been updated with latest results. |
📝 Test Coverage Report
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'Chairman Token' mechanism to manage permissions for debate sharing via WebSockets. It adds new endpoints for token issuance, updates JWT handling to support the new token type, and configures WebSocket heartbeats for improved connection stability. Review feedback highlights a bug in token resolution where the wrong resolver method was used and a potential null-pointer issue in a database query when summing debate times.
| } | ||
|
|
||
| public String resolveChairmanToken(String chairmanToken) { | ||
| return jwtTokenResolver.resolveAccessToken(chairmanToken); |
There was a problem hiding this comment.
resolveChairmanToken 메서드에서 jwtTokenResolver.resolveAccessToken을 호출하고 있습니다. JwtTokenResolver에는 CHAIRMAN_TOKEN 타입을 검증하는 전용 메서드인 resolveChairmanToken이 구현되어 있으므로, 이를 호출하도록 수정해야 합니다. 현재 상태로는 토큰 타입 불일치로 인해 인증에 실패하게 됩니다.
Suggested change
| return jwtTokenResolver.resolveAccessToken(chairmanToken); | |
| return jwtTokenResolver.resolveChairmanToken(chairmanToken); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/test/java/com/debatetimer/repository/customize/CustomizeTimeBoxRepositoryTest.java (1)
84-100: 엣지 케이스 테스트 추가 고려현재 테스트는 정상 케이스를 잘 커버하고 있습니다. 다만, 다음 엣지 케이스에 대한 테스트 추가를 고려해 보세요:
- 테이블에 타임박스가 없을 때 0을 반환하는지 확인
- 존재하지 않는 테이블 ID에 대한 동작 확인
💡 엣지 케이스 테스트 예시
`@Test` void 타임박스가_없는_테이블은_0을_반환한다() { Member chan = memberGenerator.generate("default@gmail.com"); CustomizeTableEntity emptyTable = customizeTableEntityGenerator.generate(chan); long summedTime = customizeTimeBoxRepository.sumTimeByTableId(emptyTable.getId()); assertThat(summedTime).isEqualTo(0L); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/debatetimer/repository/customize/CustomizeTimeBoxRepositoryTest.java` around lines 84 - 100, Add edge-case tests for CustomizeTimeBoxRepository.sumTimeByTableId: one test that creates a CustomizeTableEntity with no CustomizeTimeBoxEntity entries and asserts sumTimeByTableId(table.getId()) returns 0L, and another test that calls sumTimeByTableId with a non-existent table id (e.g., Long.MAX_VALUE or table.getId()+1) and asserts the expected behavior (0L or throws/handled result consistent with repository contract); add these new `@Test` methods in the SumTimeByTableId nested class (use memberGenerator and customizeTableEntityGenerator as in existing test) and name them clearly (e.g., 타임박스가_없는_테이블은_0을_반환한다 and 존재하지_않는_테이블_ID_처리_확인).src/test/java/com/debatetimer/fixture/HeaderGenerator.java (1)
26-27: 테스트용 사회자 토큰 만료시간(5초) 하드코딩은 플래키를 유발할 수 있습니다.Line 27의
5L은 CI 지연 시 만료를 일으킬 수 있습니다. 최소 하트비트 주기(10초)보다 충분히 큰 테스트 상수로 분리해 두는 편이 안전합니다.만료시간 상수화 예시
public class HeaderGenerator { + private static final long TEST_CHAIRMAN_TOKEN_EXPIRATION_SECONDS = 60L; private final JwtTokenProvider jwtTokenProvider; @@ public StompHeaders generateChairmanTokenHeader(String destination, Member member) { - String chairmanToken = jwtTokenProvider.createChairmanToken(new MemberInfo(member), 5L); + String chairmanToken = jwtTokenProvider.createChairmanToken( + new MemberInfo(member), TEST_CHAIRMAN_TOKEN_EXPIRATION_SECONDS + ); StompHeaders stompHeaders = new StompHeaders(); stompHeaders.setDestination(destination); stompHeaders.add(HttpHeaders.AUTHORIZATION, chairmanToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/debatetimer/fixture/HeaderGenerator.java` around lines 26 - 27, The 5L literal in generateChairmanTokenHeader (jwtTokenProvider.createChairmanToken(new MemberInfo(member), 5L)) hardcodes a short expiry that can cause flaky tests; replace it with a named test constant (e.g., TEST_CHAIRMAN_TOKEN_EXPIRY_SECONDS) defined in HeaderGenerator and set it to a value safely above the minimum heartbeat (≥10s, e.g., 30L), then call createChairmanToken(new MemberInfo(member), TEST_CHAIRMAN_TOKEN_EXPIRY_SECONDS).src/test/java/com/debatetimer/controller/sharing/SharingDocumentTest.java (1)
51-56: 성공 테스트에서 응답 바디 검증을 추가해 회귀 탐지력을 높여주세요.현재는 200만 확인해서
chairmanToken누락/빈 값 회귀를 놓칠 수 있습니다.응답 필드 검증 추가 예시
+import static org.hamcrest.Matchers.emptyOrNullString; +import static org.hamcrest.Matchers.not; @@ given(document) .contentType(ContentType.JSON) .headers(EXIST_MEMBER_HEADER) .pathParam("tableId", String.valueOf(requestTableId)) .when().get("/api/share/{tableId}/chairman-token") - .then().statusCode(200); + .then() + .statusCode(200) + .body("chairmanToken", not(emptyOrNullString()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/debatetimer/controller/sharing/SharingDocumentTest.java` around lines 51 - 56, The test currently only asserts HTTP 200 for the GET call built with given(document).headers(EXIST_MEMBER_HEADER).pathParam("tableId", String.valueOf(requestTableId)).when().get("/api/share/{tableId}/chairman-token") but doesn't validate the response body; update the test to assert the JSON response contains a non-null, non-empty "chairmanToken" field (e.g., add assertions after then() to check "chairmanToken" exists and is not an empty string) so regressions that drop or return an empty token are caught.src/test/java/com/debatetimer/service/sharing/SharingServiceTest.java (1)
31-32: 성공 케이스가 핵심 결과를 검증하지 않습니다.예외 미발생만 확인하면 잘못된 토큰(빈 문자열 등)도 통과합니다. 반환된
chairmanToken자체를 검증해 주세요.결과 검증 강화 예시
import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThat; @@ import com.debatetimer.dto.sharing.response.ChairmanTokenResponse; @@ - assertThatCode(() -> sharingService.issueChairmanToken(tableEntity.getId(), member)) - .doesNotThrowAnyException(); + ChairmanTokenResponse response = sharingService.issueChairmanToken(tableEntity.getId(), member); + assertThat(response.chairmanToken()).isNotBlank();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/debatetimer/service/sharing/SharingServiceTest.java` around lines 31 - 32, The test currently only asserts that sharingService.issueChairmanToken(tableEntity.getId(), member) does not throw, which misses validating the actual token; change the test in SharingServiceTest to capture the returned value (e.g., String chairmanToken = sharingService.issueChairmanToken(...)) and add assertions that chairmanToken is not null and not empty (and optionally matches the expected format such as a UUID or token regex) instead of using assertThatCode(...).doesNotThrowAnyException(); this ensures the method returns a valid chairmanToken rather than merely not throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/debatetimer/controller/tool/jwt/AuthManager.java`:
- Around line 41-43: The method resolveChairmanToken currently delegates to
jwtTokenResolver.resolveAccessToken which verifies ACCESS token type; change the
delegation to the chairman-specific resolver so CHAIRMAN_TOKEN validation is
used (e.g., call jwtTokenResolver.resolveChairmanToken or the generic resolver
that accepts a TokenType/enum and pass TokenType.CHAIRMAN) in
resolveChairmanToken to ensure correct token-type verification.
In
`@src/main/java/com/debatetimer/repository/customize/CustomizeTimeBoxRepository.java`:
- Around line 16-17: The SUM query can return null and cause a primitive long
unboxing NPE in sumTimeByTableId; modify the repository query to return zero
instead of null by using COALESCE (e.g. change the JPQL in
CustomizeTimeBoxRepository.sumTimeByTableId to "SELECT COALESCE(SUM(ctb.time),
0) FROM CustomizeTimeBoxEntity ctb WHERE ctb.customizeTable.id = :tableId" so
the method can safely remain returning long), and then verify
SharingService.issueChairmanToken() uses that value directly.
In `@src/main/java/com/debatetimer/service/sharing/SharingService.java`:
- Around line 34-39: The method issueChairmanToken currently multiplies
debateTime from customizeTableDomainRepository.getTotalTimeBoxTimes(...) by 2
and may pass 0 to jwtTokenProvider.createChairmanToken, creating an immediately
expired token; update issueChairmanToken to handle debateTime == 0 by either
enforcing a minimum expiry (e.g., compute expiry = Math.max(debateTime * 2,
MIN_EXPIRY_MS) and pass expiry to jwtTokenProvider.createChairmanToken) or throw
a domain/client exception (e.g., DTClientErrorException with a
NO_TIME_BOXES_CONFIGURED code) so that ChairmanTokenResponse is not created with
a zero TTL; locate this logic in issueChairmanToken and adjust the flow
accordingly.
In
`@src/test/java/com/debatetimer/controller/sharing/SharingWebSocketControllerTest.java`:
- Around line 61-81: The test method 사회자가_아니면_이벤트를_발행할_수_없다() contains an unused
local variable member and a misleading comment; remove the unused Member member
= memberGenerator.generate(...) declaration (or use it to authenticate if that
was intended) and update the inline comment on stompSession.send("/app/event/" +
roomId, request) to reflect that this is sending an unauthenticated/non-host
event (e.g., "인증되지 않은(또는 사회자가 아닌) 요청 전송") so the test intent matches the code
using handler, stompSession.subscribe, stompSession.send, SharingRequest and
handler.getCompletableFuture().
In `@src/test/java/com/debatetimer/service/sharing/SharingServiceTest.java`:
- Around line 36-41: Replace the current non-owner table test input (hardcoded
1L) with a real table owned by another member so the ownership check is
exercised: create a different Member via memberGenerator (e.g., other =
memberGenerator.generate(...)), create a Table/Room entity owned by that other
member and persist it, then call
sharingService.issueChairmanToken(table.getId(), member) using the original test
Member; assert the same DTClientErrorException and
ClientErrorCode.TABLE_NOT_FOUND.getMessage() to verify non-ownership is detected
(references: issueChairmanToken, memberGenerator, Member, sharingService,
ClientErrorCode.TABLE_NOT_FOUND).
---
Nitpick comments:
In `@src/test/java/com/debatetimer/controller/sharing/SharingDocumentTest.java`:
- Around line 51-56: The test currently only asserts HTTP 200 for the GET call
built with given(document).headers(EXIST_MEMBER_HEADER).pathParam("tableId",
String.valueOf(requestTableId)).when().get("/api/share/{tableId}/chairman-token")
but doesn't validate the response body; update the test to assert the JSON
response contains a non-null, non-empty "chairmanToken" field (e.g., add
assertions after then() to check "chairmanToken" exists and is not an empty
string) so regressions that drop or return an empty token are caught.
In `@src/test/java/com/debatetimer/fixture/HeaderGenerator.java`:
- Around line 26-27: The 5L literal in generateChairmanTokenHeader
(jwtTokenProvider.createChairmanToken(new MemberInfo(member), 5L)) hardcodes a
short expiry that can cause flaky tests; replace it with a named test constant
(e.g., TEST_CHAIRMAN_TOKEN_EXPIRY_SECONDS) defined in HeaderGenerator and set it
to a value safely above the minimum heartbeat (≥10s, e.g., 30L), then call
createChairmanToken(new MemberInfo(member), TEST_CHAIRMAN_TOKEN_EXPIRY_SECONDS).
In
`@src/test/java/com/debatetimer/repository/customize/CustomizeTimeBoxRepositoryTest.java`:
- Around line 84-100: Add edge-case tests for
CustomizeTimeBoxRepository.sumTimeByTableId: one test that creates a
CustomizeTableEntity with no CustomizeTimeBoxEntity entries and asserts
sumTimeByTableId(table.getId()) returns 0L, and another test that calls
sumTimeByTableId with a non-existent table id (e.g., Long.MAX_VALUE or
table.getId()+1) and asserts the expected behavior (0L or throws/handled result
consistent with repository contract); add these new `@Test` methods in the
SumTimeByTableId nested class (use memberGenerator and
customizeTableEntityGenerator as in existing test) and name them clearly (e.g.,
타임박스가_없는_테이블은_0을_반환한다 and 존재하지_않는_테이블_ID_처리_확인).
In `@src/test/java/com/debatetimer/service/sharing/SharingServiceTest.java`:
- Around line 31-32: The test currently only asserts that
sharingService.issueChairmanToken(tableEntity.getId(), member) does not throw,
which misses validating the actual token; change the test in SharingServiceTest
to capture the returned value (e.g., String chairmanToken =
sharingService.issueChairmanToken(...)) and add assertions that chairmanToken is
not null and not empty (and optionally matches the expected format such as a
UUID or token regex) instead of using
assertThatCode(...).doesNotThrowAnyException(); this ensures the method returns
a valid chairmanToken rather than merely not throwing.
🪄 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: 174ac2d0-e160-4c12-a7a2-6e9bdbc166b4
📒 Files selected for processing (19)
src/main/java/com/debatetimer/config/sharing/WebSocketAuthMemberResolver.javasrc/main/java/com/debatetimer/config/sharing/WebSocketConfig.javasrc/main/java/com/debatetimer/controller/sharing/SharingRestController.javasrc/main/java/com/debatetimer/controller/sharing/SharingWebSocketController.javasrc/main/java/com/debatetimer/controller/tool/jwt/AuthManager.javasrc/main/java/com/debatetimer/controller/tool/jwt/JwtTokenProvider.javasrc/main/java/com/debatetimer/controller/tool/jwt/JwtTokenResolver.javasrc/main/java/com/debatetimer/controller/tool/jwt/TokenType.javasrc/main/java/com/debatetimer/domainrepository/customize/CustomizeTableDomainRepository.javasrc/main/java/com/debatetimer/dto/sharing/response/ChairmanTokenResponse.javasrc/main/java/com/debatetimer/repository/customize/CustomizeTimeBoxRepository.javasrc/main/java/com/debatetimer/service/sharing/SharingService.javasrc/test/java/com/debatetimer/controller/BaseDocumentTest.javasrc/test/java/com/debatetimer/controller/Tag.javasrc/test/java/com/debatetimer/controller/sharing/SharingDocumentTest.javasrc/test/java/com/debatetimer/controller/sharing/SharingWebSocketControllerTest.javasrc/test/java/com/debatetimer/fixture/HeaderGenerator.javasrc/test/java/com/debatetimer/repository/customize/CustomizeTimeBoxRepositoryTest.javasrc/test/java/com/debatetimer/service/sharing/SharingServiceTest.java
unifolio0
reviewed
Apr 15, 2026
Contributor
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@coli-geonwoo 리뷰 몇개 남겼어요
| @RequiredArgsConstructor | ||
| public class SharingService { | ||
|
|
||
| private final JwtTokenProvider jwtTokenProvider; |
Contributor
There was a problem hiding this comment.
저희 이거 예전에 Oauth 로그인 할 때 AuthManager로 Controller 단에서 처리하기로 합의하지 않았었나요?
Contributor
Author
There was a problem hiding this comment.
일단 늦게 확인하고 답변 남겨 염치없지만 이부분은 의견이 달라 남겨둡니다.
1) 내가 기억하는 이전에 우리가 Controller 단에서 인증처리를 하려고 했던 이유
- 인증이 프로토콜 경계에서 일어나기 때문에
- Http 프로토콜에 종속되지 않고 서비스 확장가능성을 보장하기 위해
즉, 서비스 단에서는 '인증된 사용자'를 받으면 되지 토큰 해석이나 발급까지 서비스에서 할 필요가 없다는데 동의했기 때문에 그런 결정이 있었던 것으로 기억합니다.
그런데 해당 상황은 다음 부분에서 다르다고 생각합니다.
-
인증된 사용자를 대상으로 일정 시간 동안, 사회자용 권한 자격증을 발급해주는 로직임.
이 토큰 같은 경우에는누구인지 식별하는 인증 토큰보다 사회자용 이벤트 발행에 대한 권한이 있는지 검증하는 권한 토큰의 성격이 더 크다고 생각했어요. -
도메인이 많이 묻어있음
사회자용 인증 토큰을 발급하는 과정에서 도메인에 꽤 깊은 관여가 되어 있습니다. 사회자를 위해 유효기간이 전체 토론 시간 *2인 토큰을 발급해야 하는데, 도메인 레이어를 각각 호출해서 이를 controller에서 조립해 발급한다는 생각이 들었어요. -
만약 토큰 발급에 필요한 도메인이 많아진다면..?
예전 매쉬업 프로젝트에서도 컨트롤러에서 authService를 통해 토큰을 발급하는 로직을 고수해보았는데 점점 토큰 발급에 필요한 도메인이 많아지다보니까 low layer에서 필요한 정보들을 모아 컨트롤러에서 조합해 토큰 발급 로직에 넣어주어야 하는 경우가 생겼습니다.
사실 컨트롤러로 바꾸는 건 그리 어려운 일은 아닌데 비토 생각 듣고 싶어 한번 팅겨봅니다.
Contributor
Author
There was a problem hiding this comment.
claude 한테도 맥락 넘겨 리뷰받아보았는데 이런 의견을 주었어요.
claude 답변
사회자용 토큰 발급, 어디에 둘 것인가
좋은 질문이고, 사실 이 고민의 핵심은 "토큰이라는 것이 정말 HTTP/프로토콜에 종속된 개념인가, 아니면 도메인 개념인가" 라는 지점이라고 봐. 결론부터 말하면, 나는 이 경우에는 Service 레이어에서 발급하고, Controller는 단순히 그것을 HTTP 응답으로 감싸기만 하는 구조를 택할 거야. 이유를 차근차근 설명해줄게.
1. 이전 결정과 지금 상황의 본질적 차이
팀에서 OAuth/JWT 인증 토큰을 Controller에서 발급하기로 한 결정은 옳았어. 그 이유를 다시 분해해보면:
- 인증(Authentication)은 프로토콜 경계에서 일어나는 일이야. "이 HTTP 요청을 보낸 사람이 누구인가"를 식별하는 건 transport layer의 관심사지.
- JWT는 그 자체가 stateless 인증을 위한 HTTP 친화적 메커니즘이야. 쿠키, Authorization 헤더, CORS 같은 것과 한 묶음으로 다뤄지지.
- 그래서 Service는 "인증된 User"를 받는 것이지, 토큰을 알 필요가 없었어. 프로토콜이 gRPC로 바뀌어도 Service는 그대로 재사용 가능했지.
그런데 지금 발급하려는 "사회자용 토큰"은 성격이 완전히 달라:
- 이건 누구인지 식별하는 인증 토큰이 아니라, 특정 도메인 이벤트(토론 시작) 시점에, 특정 도메인 역할(사회자)에게, 특정 도메인 컨텍스트(이 토론, 이 시간 동안)에서 사용할 **권한 자격증(capability token)**이야.
- 발급 트리거가 HTTP 요청이 아니라 "토론이 시작된다"는 도메인 이벤트야. 토론 시작이 스케줄러로 자동화되거나, 다른 진입점(예: 관리자 콘솔, 배치 작업)이 생긴다면? Controller에 발급 로직이 있으면 그곳에서도 똑같은 코드를 또 작성하거나 Controller를 호출해야 해.
- 만료 시간이 **"토론 전체 시간의 2배"**라는 도메인 규칙으로 결정돼. 이건 명백히 비즈니스 규칙이야.
2. "토큰 발급 = Controller"는 규칙이 아니라 사례였다
여기서 중요한 포인트야. 팀의 이전 결정은 **"토큰은 무조건 Controller에서 발급한다"**가 아니라, **"프로토콜 종속적인 인증 토큰은 Controller에서 발급한다"**였어. 둘은 달라.
지금 토큰의 본질을 따져보면:
관점 | OAuth JWT | 사회자 토큰
-- | -- | --
발급 트리거 | 로그인 HTTP 요청 | 토론 시작 도메인 이벤트
만료 정책 결정 주체 | 보안 정책 (인프라/플랫폼) | 토론 길이 (도메인 규칙)
식별 대상 | "이 사용자가 누구인가" | "이 사람이 이 토론의 사회자인가"
다른 프로토콜에서 재사용? | 보통 안 함 (프로토콜별 인증) | 가능성 매우 높음 (WS, SSE, 추후 RPC 등)
검증 위치 | API Gateway / 미들웨어 | WebSocket 핸들러 비즈니스 로직 진입부
특히 마지막 두 줄을 보면, 사회자 토큰은 오히려 재사용성과 도메인 결합도 측면에서 Service에 있어야 할 이유가 더 강해.
3. Controller에 두면 생기는 실질적 문제
추상적인 "레이어드 아키텍처 위반"을 넘어서, 실제로 발생할 문제들:
(1) 만료 시간 규칙의 분산
"토론 시간의 2배"라는 규칙이 Controller에 있으면, Service의 Debate 엔티티가 getDuration()을 알고, Controller가 그걸 가져다 ×2 해서 토큰을 만들어. 도메인 규칙이 Controller에 흘러나가는 거지. 나중에 "프리미엄 토론은 3배" 같은 규칙이 추가되면? Controller에 if문이 박히기 시작해.
(2) 진입점이 늘어나면 중복 발생 지금은 사회자가 "토론 시작" 버튼을 누르는 HTTP 요청 하나뿐일 수 있어. 하지만 이런 게 추가될 가능성이 높아:
- 예약된 토론을 자동 시작하는 스케줄러
- 관리자가 강제로 시작시키는 어드민 API
- 다른 서비스가 호출하는 내부 API
이 모든 진입점에서 같은 토큰 발급 로직을 호출해야 해. Service에 있으면 한 줄, Controller에 있으면 복붙.
(3) 테스트 어려움 토큰 발급 로직을 테스트하려면 Controller 테스트를 돌려야 해. HTTP 목킹, 요청/응답 컨텍스트... Service에 있으면 순수 단위 테스트 한 방.
(4) "토큰" 단어에 속지 않기 이게 진짜 중요한데, "토큰"이라는 단어가 주는 인프라/보안 뉘앙스 때문에 자동으로 Controller에 두려는 관성이 생겨. 하지만 본질은 **"사회자에게 발급되는, 이 토론에서 유효한, 시간 제한 있는 권한 증서"**야. 이걸 그냥 도메인 객체로 보면 어디에 둘지 자명해져.
Contributor
There was a problem hiding this comment.
- 인증된 사용자를 대상으로 일정 시간 동안, 사회자용 권한 자격증을 발급해주는 로직임.
-> 그건 로직 전체를 기준으로 봐서 그런거지 결국에 JwtTokenProvider 가 하는 일은 토큰 발급인 것 같아요. 실제로 로그인과 같은 로직에서도 Oauth를 통해 인증을 받는다든지 하는 로직은 service단에서 하고 토큰 발급만 Controller에서 하는 형태인 것 같아요. - 만약 토큰 발급에 필요한 도메인이 많아진다면..?
-> 조금 더 자세히 설명해주세요. 제 생각에는 AuthManager에 토큰 유효시간도 받는 메소드를 만들고 Service에서 해당 정보도 같이 넘겨주는 것도 나쁘지 않다는 생각이 들어요.(너무 코드를 오랜만에 봐서 컨텍스트가 떨어져서 그럴 수도 있으니 의견 부탁드립니다.) - 테스트 어려움
-> 토큰 발급에 대한 것은 AuthManager나 JwtTokenProvider에서 하고 도메인으로 유효시간을 생성하거나 하는 부분만 Service테스트하면 되지 않나요? - "토큰" 단어에 속지 않기
->본질은 **"사회자에게 발급되는, 이 토론에서 유효한, 시간 제한 있는 권한 증서"**야.이게 더 이해가 안되는데 그렇게 보면 Member에게 발급되는 토큰도 해당 사용자에게 발급되는, 그 사용자에게 시간 제한 있는 권한 증서인데 결국 말장난 밖에 안된다는 생각이 들어요. 결국 토큰은 그냥 인증용도 아닌가요?
5. 가장 중요한 의견
지금 import를 보면 Service에서 controller 패키지를 알고 있게 됩니다.
Contributor
Author
There was a problem hiding this comment.
일단 Controller에 JwtTokenProvider가 있는 이상 순환의존이 생기는 문제를 피할 수 없어 Controller로 옮겨놓았습니다.
저는 인증과 인가는 구분되어야 한다고 생각하는데, 해당 케이스는 회원임을 검증하는 인증이 아닌 토론 공유 시 타이머 이벤트 발행에 대한 권한이 있는지 검증하는 인가라고 생각했습니다. 같은 회원이더라도 해당 권한을 지니지 못한 회원은 타이머 이벤트를 발행하지 못하므로 인가 상황에서 발급되는 자격증이라 생각했고, 그 방법론을 토큰으로 채택한거라고 생각했어요. 토큰은 인증용도다. 가 아니라 탑다운으로 인가 상황에 필요한 자격증 -> 자격증의 형태 == JWT 토큰으로 생각하는게 자연스럽다고 생각합니다.
도메인이 많아지는 상황에 대한 가정은 다음과 같아요.
인가 상황에서 권한 자격증 발급을 위해서 필요한 정보가 많아지거나, 검증되어야 하는 맥락이 많아질 수 있다고 생각했어요. 예를 들어 AService에서 a에 대한 정보와 BService에서 b라는 정보를 조합해 토큰을 만든다고 하면 이렇게 되겠죠..?
a= Aservice.getA();
b = Bservice.getB();
authManager.issueToken(a, b);이렇게 토큰 조합에 필요한 정보가 컨트롤러 단에 드러나있는게 개인적으로 불편했고, 만약 자격증 발급에 필요한 정보조합이 바뀐다면 이걸 Controller에서 수정해주어야 할 수 있다고 판단했습니다.(각 서비스를 호출해서 조합하는 역할을 해야하므로)
아니면 Facade로 새로운 계층을 만들어서
TokenInfo tokenInfo = FacadeService.getTokenInfo();
authManager.issueToken(tokenInfo);이렇게 말아서 올려야 하는데 이는 새로운 계층의 출현이라는 새로운 논의점으로 접어듭니다.
마지막으로 AuthManager는 Chairman이라는 도메인을 알게됩니다. 인증을 담당하는 서비스가 사회자라는 도메인을 깊게 알게 된다고 생각이 되었습니다.
또한, TokenType.ChairMan을 검증해야 하기 때문에 JwtTokenProvider에서 TokenType.Chairman을 주입한 issueChairmanToken이란 메서드도 생겼어요. 타협한 부분이긴 한데. issueAccessToken, issueRefreshToken으로 각 발급 로직 별로 메서드를 분리해놓아 지금은 issueChairmanToken으로 메서드를 만들어둔 상황이에요.
만약 AuthManager가 기존처럼 도메인에 독립적으로 토큰 발급만을 담당할 수 있는 방안이 있다면 지금처럼 Controller에 토큰 발급을 올려두는 것도 좋을 것 같슴다.
unifolio0
requested changes
Apr 15, 2026
leegwichan
approved these changes
Apr 20, 2026
Member
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
/noti 제 코멘트 이외 코멘트만 확인 부탁드립니다.
coli-geonwoo
commented
Apr 28, 2026
Contributor
Author
coli-geonwoo
left a comment
coli-geonwoo
left a comment
There was a problem hiding this comment.
/noti
요새 힘드네요.. 취준하랴 소마하랴.. 토큰 발급 계층에 대해 한번 비토 의견 듣고 싶어 생각남겨놓았으니 확인 부탁드립니다.
unifolio0
requested changes
May 6, 2026
Contributor
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@coli-geonwoo
의견 남겼어요
| @RequiredArgsConstructor | ||
| public class SharingService { | ||
|
|
||
| private final JwtTokenProvider jwtTokenProvider; |
Contributor
There was a problem hiding this comment.
- 인증된 사용자를 대상으로 일정 시간 동안, 사회자용 권한 자격증을 발급해주는 로직임.
-> 그건 로직 전체를 기준으로 봐서 그런거지 결국에 JwtTokenProvider 가 하는 일은 토큰 발급인 것 같아요. 실제로 로그인과 같은 로직에서도 Oauth를 통해 인증을 받는다든지 하는 로직은 service단에서 하고 토큰 발급만 Controller에서 하는 형태인 것 같아요. - 만약 토큰 발급에 필요한 도메인이 많아진다면..?
-> 조금 더 자세히 설명해주세요. 제 생각에는 AuthManager에 토큰 유효시간도 받는 메소드를 만들고 Service에서 해당 정보도 같이 넘겨주는 것도 나쁘지 않다는 생각이 들어요.(너무 코드를 오랜만에 봐서 컨텍스트가 떨어져서 그럴 수도 있으니 의견 부탁드립니다.) - 테스트 어려움
-> 토큰 발급에 대한 것은 AuthManager나 JwtTokenProvider에서 하고 도메인으로 유효시간을 생성하거나 하는 부분만 Service테스트하면 되지 않나요? - "토큰" 단어에 속지 않기
->본질은 **"사회자에게 발급되는, 이 토론에서 유효한, 시간 제한 있는 권한 증서"**야.이게 더 이해가 안되는데 그렇게 보면 Member에게 발급되는 토큰도 해당 사용자에게 발급되는, 그 사용자에게 시간 제한 있는 권한 증서인데 결국 말장난 밖에 안된다는 생각이 들어요. 결국 토큰은 그냥 인증용도 아닌가요?
5. 가장 중요한 의견
지금 import를 보면 Service에서 controller 패키지를 알고 있게 됩니다.
Comment on lines
28
to
31
| ) { | ||
|
|
||
| return sharingService.share(request); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| ) { | |
| return sharingService.share(request); | |
| } |
Contributor
Author
There was a problem hiding this comment.
이거 뭐지? ㅋㅋㅋ 반영 완료
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java (1)
102-128: ⚡ Quick win빈 타임박스(총합 0) 케이스도 테스트에 추가해 두는 걸 권장합니다.
현재는 정상 합계/권한 예외만 검증해서,
SUM + COALESCE회귀를 잡기 어렵습니다.테스트 추가 예시
`@Nested` class FindDebateTime { + `@Test` + void 타임박스가_없으면_총_토론_시간은_0이다() { + Member chan = memberGenerator.generate("default@gmail.com"); + CustomizeTableEntity chanTable = customizeTableEntityGenerator.generate(chan); + + long debateTime = customizeService.findDebateTime(chanTable.getId(), chan); + + assertThat(debateTime).isZero(); + } + `@Test` void 사용자_지정_토론_테이블의_총_토론_시간을_조회한다() {🤖 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/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java` around lines 102 - 128, Add a test in the FindDebateTime nested class to cover the "empty timebox sum is 0" case: create a Member (e.g., chan), generate a CustomizeTableEntity for that member with no calls to customizeTimeBoxEntityGenerator.generate (or generate boxes with 0 duration), call customizeService.findDebateTime(chanTable.getId(), chan) and assert the returned value is 0 to ensure SUM+COALESCE regressions are caught; place the new test alongside the existing tests in CustomizeServiceTest to exercise the same authorization/path but with zero total duration.
🤖 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 `@src/main/java/com/debatetimer/controller/sharing/SharingRestController.java`:
- Line 26: The multiplication debateTime * 2 in SharingRestController when
calling authManager.issueChairmanToken can overflow for large debateTime; change
the call to compute the TTL using Math.multiplyExact(debateTime, 2) (or catch
ArithmeticException) before passing to issueChairmanToken so you either
propagate/handle the error or clamp/validate the TTL—ensure you reference
debateTime, Math.multiplyExact, and authManager.issueChairmanToken when locating
and updating the code to prevent silent overflow.
---
Nitpick comments:
In `@src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java`:
- Around line 102-128: Add a test in the FindDebateTime nested class to cover
the "empty timebox sum is 0" case: create a Member (e.g., chan), generate a
CustomizeTableEntity for that member with no calls to
customizeTimeBoxEntityGenerator.generate (or generate boxes with 0 duration),
call customizeService.findDebateTime(chanTable.getId(), chan) and assert the
returned value is 0 to ensure SUM+COALESCE regressions are caught; place the new
test alongside the existing tests in CustomizeServiceTest to exercise the same
authorization/path but with zero total duration.
🪄 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: 88854ffe-5f20-43ce-a902-ebd7aabc0af3
📒 Files selected for processing (6)
src/main/java/com/debatetimer/controller/sharing/SharingRestController.javasrc/main/java/com/debatetimer/controller/tool/jwt/AuthManager.javasrc/main/java/com/debatetimer/service/customize/CustomizeService.javasrc/main/java/com/debatetimer/service/sharing/SharingService.javasrc/test/java/com/debatetimer/controller/sharing/SharingDocumentTest.javasrc/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/debatetimer/controller/tool/jwt/AuthManager.java
| @PathVariable("tableId") long tableId | ||
| ) { | ||
| long debateTime = customizeService.findDebateTime(tableId, member); | ||
| String chairmanToken = authManager.issueChairmanToken(member, debateTime * 2); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] AuthManager.issueChairmanToken 시그니처/만료 단위 확인"
fd -i 'AuthManager.java' src | xargs -r sed -n '1,260p'
rg -n --type=java -C2 'issueChairmanToken|expire|expiration|ttl' src
echo "[2] 토론 시간 입력값 상한 검증 확인"
fd -i 'CustomizeTimeBoxCreateRequest.java' src | xargs -r sed -n '1,260p'
rg -n --type=java -C2 '@Max|@Min|@Positive|@PositiveOrZero|time|duration' src/main/java/com/debatetimerRepository: debate-timer/debate-timer-be
Length of output: 50385
debateTime * 2 곱셈 오버플로우 방어 추가 권장
line 26의 long 타입 곱셈은 오버플로우 시 조용히 음수 또는 비정상 값을 발생시킵니다. debateTime이 매우 큰 값이면 곱셈 결과가 음수가 되어 issueChairmanToken에 부정확한 TTL이 전달될 수 있으므로 Math.multiplyExact()를 사용한 방어가 좋습니다.
제안 코드
- String chairmanToken = authManager.issueChairmanToken(member, debateTime * 2);
+ long chairmanTtl = Math.multiplyExact(debateTime, 2L);
+ String chairmanToken = authManager.issueChairmanToken(member, chairmanTtl);📝 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
| String chairmanToken = authManager.issueChairmanToken(member, debateTime * 2); | |
| long chairmanTtl = Math.multiplyExact(debateTime, 2L); | |
| String chairmanToken = authManager.issueChairmanToken(member, chairmanTtl); |
🤖 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/debatetimer/controller/sharing/SharingRestController.java`
at line 26, The multiplication debateTime * 2 in SharingRestController when
calling authManager.issueChairmanToken can overflow for large debateTime; change
the call to compute the TTL using Math.multiplyExact(debateTime, 2) (or catch
ArithmeticException) before passing to issueChairmanToken so you either
propagate/handle the error or clamp/validate the TTL—ensure you reference
debateTime, Math.multiplyExact, and authManager.issueChairmanToken when locating
and updating the code to prevent silent overflow.
Contributor
Author
|
/noti 서비스 순환의존이 생긴다는 무적논리를 감당하지 못해. Controller로 토큰 발급 로직 올렷습니다. 추가로 의견도 적어놓았어요. 일단 이 PR은 발효되었으니 머지하시죠. |
unifolio0
approved these changes
May 10, 2026
Contributor
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@coli-geonwoo
늦어서 죄송합니다. 일단 approve 하겠습니다. 그런데 지금 ide 포멧팅 다시 체크 부탁드려요. 제가 공백 관련해서 리뷰 단 것들이 하나도 수정 안된 것 같습니다.
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
3 participants
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.
🚩 연관 이슈
closed #245
🗣️ 리뷰 요구사항 (선택)
하트비트 문제
기존 문제
양쪽모두 하트비트 설정
두 하트비트의 목적이 다르고 신뢰성 있는 기능 개발을 위해 10초 간격으로 양방향 설정
STOMP가 하트비트를 통해 자동으로 처리해주는 것
해당 세션을 죽었다고 판단
WebSocket 연결 close
DISCONNECT이벤트 발행 → Spring의 내부 세션 정리해당 세션의 구독 정보 제거
SimpUserRegistry에서 사용자/세션 제거추후 처리할 에러 처리 문제 : Role 별 비정상종료 시 비즈니스 로직 처리 문제
⇒ 종료시 비즈니스 로직 처리를 위해서는 SessionDisconnectEvent 처리 필요
인증 로직
토큰이 3종류로 넘어가면서 중복 코드가 조금 생겼는데 이부분에 대한 의견 부탁합니다
Summary by CodeRabbit
새로운 기능
테스트