Conversation
- QR 코드 기반 웹 로그인 세션 관리 (Redis) - 웹용 Access Token 발급 (3일 만료) - CORS에 web.toduck.app 도메인 추가 - 웹 로그인 인증 엔드포인트 보안 설정 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
의존성 및 보안 설정 build.gradle, src/main/java/im/toduck/global/config/security/CorsConfig.java, src/main/java/im/toduck/global/config/security/SecurityConfig.java |
QR 코드 생성을 위한 ZXing 라이브러리 추가, CORS 허용 도메인에 https://web.toduck.app 추가, /v1/auth/web/authorize 엔드포인트에 인증 요구사항 추가 |
JWT 토큰 생성 src/main/java/im/toduck/domain/auth/domain/service/JwtService.java, src/main/java/im/toduck/global/security/jwt/access/AccessTokenProvider.java |
createWebAccessToken 메서드 추가로 3일 만료 시간의 웹 접근 토큰 생성, generateTokenWithCustomExpiry 메서드 추가로 커스텀 만료 시간 지원 |
웹 로그인 API 및 컨트롤러 src/main/java/im/toduck/domain/auth/presentation/api/WebLoginApi.java, src/main/java/im/toduck/domain/auth/presentation/controller/WebLoginController.java |
3개 엔드포인트 정의: 세션 생성(POST /sessions), 세션 승인(POST /authorize), 상태 조회(GET /sessions/{sessionToken}); 인증 및 권한 검증 포함 |
웹 로그인 요청/응답 DTO src/main/java/im/toduck/domain/auth/presentation/dto/request/WebLoginAuthorizeRequest.java, src/main/java/im/toduck/domain/auth/presentation/dto/response/WebSessionCreateResponse.java, src/main/java/im/toduck/domain/auth/presentation/dto/response/WebSessionStatusResponse.java |
세션 토큰 기반 승인 요청, 세션 토큰 및 Base64 QR 코드 응답, 상태(PENDING/APPROVED/EXPIRED) 및 토큰 응답 DTO 정의 |
웹 로그인 비즈니스 로직 src/main/java/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.java |
세션 생성 및 QR 코드 생성, 세션 승인, 상태 조회 및 토큰 발급 로직 구현; ZXing으로 300x300 PNG QR 코드를 Data URL로 생성 |
Redis 기반 세션 관리 src/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionStatus.java, src/main/java/im/toduck/infra/redis/weblogin/WebLoginSession.java, src/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionRepository.java, src/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionService.java, src/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionServiceImpl.java |
PENDING/APPROVED/EXPIRED 상태를 가진 Redis 세션 엔티티, 5분 TTL 기본값, CRUD 리포지토리, 서비스 인터페이스 및 구현체(SecureRandom 기반 토큰 생성) |
Sequence Diagram(s)
sequenceDiagram
participant Client as 클라이언트
participant Controller as WebLoginController
participant UseCase as WebLoginUseCase
participant Redis as Redis(Session)
participant JWT as JwtService
rect rgba(100, 150, 200, 0.5)
Note over Client,JWT: 1. 세션 생성 및 QR 코드 획득
Client->>Controller: POST /v1/auth/web/sessions
Controller->>UseCase: createWebSession()
UseCase->>Redis: 새 PENDING 세션 저장 (sessionToken, TTL=300s)
UseCase->>UseCase: QR 코드 생성 (sessionToken을 콘텐츠로)
UseCase-->>Controller: WebSessionCreateResponse(token, qrImageBase64)
Controller-->>Client: 200 OK {sessionToken, qrImageBase64}
end
rect rgba(150, 200, 150, 0.5)
Note over Client,JWT: 2. 세션 승인 (인증된 사용자)
Client->>Controller: POST /v1/auth/web/authorize {sessionToken}
Controller->>UseCase: authorizeWebSession(userId, role, request)
UseCase->>Redis: sessionToken으로 세션 조회
UseCase->>Redis: 세션 상태를 APPROVED로 변경, userId/role 저장
UseCase-->>Controller: true (성공)
Controller-->>Client: 200 OK {authorized: true}
end
rect rgba(200, 150, 100, 0.5)
Note over Client,JWT: 3. 세션 상태 조회 및 토큰 발급
Client->>Controller: GET /v1/auth/web/sessions/{sessionToken}
Controller->>UseCase: getWebSessionStatus(sessionToken)
UseCase->>Redis: sessionToken으로 세션 조회
alt 세션이 APPROVED 상태인 경우
UseCase->>JWT: createWebAccessToken(userId, role)
JWT-->>UseCase: accessToken (3일 만료)
UseCase->>Redis: 세션 삭제
UseCase-->>Controller: WebSessionStatusResponse(APPROVED, accessToken, userId)
else 세션이 PENDING 상태인 경우
UseCase-->>Controller: WebSessionStatusResponse(PENDING)
else 세션을 찾을 수 없는 경우
UseCase-->>Controller: WebSessionStatusResponse(EXPIRED)
end
Controller-->>Client: 200 OK {status, accessToken?, userId?}
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~30 minutes
Suggested labels
✨ Feature
Suggested reviewers
- kang20
- jungbk0808
- Junad-Park
Poem
🐰 QR 코드가 반짝반짝, 웹 로그인 춤을 추네요
Redis에 세션을 몽실몽실 담아두고
JWT 토큰을 팡팡 쏟아내니
승인과 조회가 우아하게 흘러가요
보안과 편의가 손잡고 함께 뛴다 🎉
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 10.81% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | PR 제목이 웹 로그인 기능 추가에 대한 주요 변경사항을 명확하게 요약하고 있으며, 전체 changeset의 핵심을 잘 반영하고 있습니다. |
| Description check | ✅ Passed | PR 설명이 리포지토리의 템플릿 구조를 따르고 있으며, QR 코드 세션 관리, 토큰 발급, 보안 설정 등 주요 작업 내용을 구조화되어 있고 충분히 상세하게 설명하고 있습니다. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feature/web-login
Tip
Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
📝 Jacoco Test Coverage
|
![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 (6)
src/main/java/im/toduck/domain/auth/domain/service/JwtService.java (1)
74-79: 만료 기간 3일 하드코딩 — 설정값으로 추출 권장.
Duration.ofDays(3)가 메서드 내부에 직접 하드코딩되어 있습니다. 다른 토큰의 만료 시간은@Value를 통해 외부 설정(application.properties)에서 주입받고 있으므로, 웹 액세스 토큰 만료 기간도 동일한 방식으로 관리하는 것이 일관성 및 운영 유연성 측면에서 좋습니다.♻️ 개선 제안
AccessTokenProvider또는JwtService에 설정값을 주입하는 방식:+ private static final Duration WEB_ACCESS_TOKEN_EXPIRY = Duration.ofDays(3); + // 또는 `@Value`("${jwt.expiration-time.web-access-token}") Duration webTokenExpiry; + public String createWebAccessToken(final Long userId, final String role) { return accessTokenProvider.generateTokenWithCustomExpiry( AccessTokenClaim.of(userId, role), - Duration.ofDays(3) + WEB_ACCESS_TOKEN_EXPIRY ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/im/toduck/domain/auth/domain/service/JwtService.java` around lines 74 - 79, The createWebAccessToken method in JwtService currently hardcodes Duration.ofDays(3); replace this with a configurable expiry injected from external configuration (e.g., `@Value`("${jwt.web-access.expiry-days}") or a Duration-based property) so the method uses that injected value when calling accessTokenProvider.generateTokenWithCustomExpiry(AccessTokenClaim.of(userId, role), ...); update JwtService to add the new field (or constructor parameter) for the configured expiry and ensure the application.properties includes the corresponding jwt.web-access.expiry-days (or a Duration property) so the token lifetime is configurable at runtime.src/main/java/im/toduck/global/config/security/CorsConfig.java (1)
22-32:https://web.toduck.appCORS origin 추가 확인.웹 로그인 기능에 필요한 origin이 적절히 추가되었습니다.
한 가지 참고:
http://localhost:5173이 허용 목록에 포함되어 있는데, 프로덕션 환경에서는 프로파일 기반으로 분리하는 것이 보안상 더 바람직합니다. 다만 이 PR에서 새로 도입된 것이 아니므로 별도 이슈로 트래킹하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/im/toduck/global/config/security/CorsConfig.java` around lines 22 - 32, The CORS whitelist in CorsConfig currently includes a development origin ("http://localhost:5173") alongside production origins; update CorsConfig to load environment/profile-specific origins instead of hardcoding localhost into the List literal (the code.building the origins List in class CorsConfig), e.g., move the localhost entry out of the static List and conditionally add it when active Spring profile is "dev" or via a config property (read from application-{profile}.yaml or a spring property like cors.allowed-origins) so production builds do not include "http://localhost:5173" while keeping the existing production origins unchanged.src/main/java/im/toduck/global/security/jwt/access/AccessTokenProvider.java (1)
38-61:generateToken과generateTokenWithCustomExpiry간 코드 중복 제거 가능.두 메서드가 만료 기간만 다르고 나머지 로직이 동일합니다.
generateToken이generateTokenWithCustomExpiry에 위임하도록 리팩터링하면 중복을 제거할 수 있습니다.♻️ 개선 제안
`@Override` public String generateToken(JwtClaims claims) { - Date now = new Date(); - - return Jwts.builder() - .header().add(createHeader()) - .and() - .claims(claims.getClaims()) - .signWith(secretKey) - .expiration(createExpireDate(now, tokenExpiration.toMillis())) - .compact(); + return generateTokenWithCustomExpiry(claims, tokenExpiration); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/im/toduck/global/security/jwt/access/AccessTokenProvider.java` around lines 38 - 61, The two methods generateToken and generateTokenWithCustomExpiry duplicate the same JWT build logic; refactor so generateToken delegates to generateTokenWithCustomExpiry (or both delegate to a single private helper) by passing tokenExpiration (tokenExpiration.toMillis()) as the custom expiry, keeping header creation via createHeader(), claims from JwtClaims.getClaims(), signWith(secretKey) and expiration via createExpireDate(now, ...); this removes duplication while preserving createHeader, secretKey and createExpireDate usage.src/main/java/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.java (1)
100-103:RuntimeException대신 프로젝트 표준 예외를 사용하세요.프로젝트 내에서
CommonException.from(ExceptionCode.XXX)패턴을 사용하고 있으므로(JwtService참고), QR 코드 생성 실패 시에도 동일한 패턴을 적용하는 것이 일관성을 유지합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.java` around lines 100 - 103, In WebLoginUseCase replace the RuntimeException thrown in the QR generation catch block (catching WriterException | IOException) with the project-standard exception pattern used elsewhere (see JwtService): call CommonException.from(...) with the appropriate ExceptionCode for QR generation failure (e.g., ExceptionCode.QR_CODE_GENERATION or a similarly named constant) and pass the caught exception as the cause so the error is logged and propagated consistently.src/main/java/im/toduck/domain/auth/presentation/controller/WebLoginController.java (2)
55-60:extractRoleFromAuthorities에서 첫 번째 authority만 사용하는 것은 취약할 수 있습니다.
getAuthorities()가 빈 컬렉션을 반환하면 기본값"USER"가 사용되며, 여러 authority가 있을 경우 첫 번째 것만 취합니다. 인증된 사용자의 authority가 비어있는 경우는 예외 상황일 수 있으므로, 기본값으로 조용히 넘어가는 대신 명시적으로 처리하는 것이 안전합니다.제안하는 수정 방안
private String extractRoleFromAuthorities(final CustomUserDetails userDetails) { return userDetails.getAuthorities().stream() .findFirst() .map(authority -> authority.getAuthority().replace("ROLE_", "")) - .orElse("USER"); + .orElseThrow(() -> new IllegalStateException("인증된 사용자에 authority가 없습니다.")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/im/toduck/domain/auth/presentation/controller/WebLoginController.java` around lines 55 - 60, In extractRoleFromAuthorities in WebLoginController, don't silently default to "USER" when getAuthorities() is empty or ignore multiple roles; instead explicitly handle the empty-authorities case (e.g., throw an IllegalStateException or an AuthenticationException with a clear message) and implement a deterministic selection strategy for multiple authorities (for example, prefer a higher-privilege role like "ADMIN" if present, otherwise pick the first sorted/filtered role and strip the "ROLE_" prefix). Update extractRoleFromAuthorities to validate userDetails.getAuthorities() is non-empty, throw or fail fast on empty, and choose roles deterministically (reference the method extractRoleFromAuthorities and CustomUserDetails#getAuthorities).
62-70: 세션 토큰은 이미 암호학적으로 안전하게 생성되고 있습니다.
WebLoginSessionServiceImpl에서SecureRandom을 사용하여 36바이트(288비트)의 난수를 생성하고 Base64 URL 인코딩하므로, 토큰 엔트로피는 충분합니다. 또한 Redis에 5분의 TTL이 설정되어 있어 토큰 노출 시간이 제한됩니다.선택사항으로,
createWebSession엔드포인트(@PreAuthorize("isAnonymous()"))에 rate limiting을 추가하면 Redis 저장소 남용을 방지할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/im/toduck/domain/auth/presentation/controller/WebLoginController.java` around lines 62 - 70, Session tokens are already cryptographically secure, but you should protect the createWebSession endpoint from abuse by adding rate limiting on the controller method (the createWebSession handler in WebLoginController, currently annotated with `@PreAuthorize`("isAnonymous()")). Implement a per-client rate limit (e.g., requests per minute) using your app's chosen mechanism (Spring Cloud RateLimiter, Bucket4j with Redis backing, or a custom filter) and apply it to createWebSession so excessive anonymous calls are rejected or throttled; ensure limits are configurable and consider scoping by client IP or username/session fingerprint.
🤖 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/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.java`:
- Around line 67-87: getWebSessionStatus has a TOCTOU race: two concurrent
pollers can both read an APPROVED session, each call
jwtService.createWebAccessToken and then deleteSession, producing duplicate
tokens; fix by making read-and-delete atomic — e.g., change the flow in
getWebSessionStatus to first attempt an atomic remove-and-return (use a Redis
DEL that returns count or a Lua script), call jwtService.createWebAccessToken
only when the delete result indicates the session was removed (delete returned
1), and otherwise return expired; alternatively implement a distributed lock
around webLoginSessionService.findBySessionToken/deleteSession (using Redisson)
so only the lock-holder issues the token; ensure you modify references in
getWebSessionStatus, webLoginSessionService.findBySessionToken,
webLoginSessionService.deleteSession, and the jwtService.createWebAccessToken
call sites accordingly.
- Around line 52-65: authorizeWebSession currently calls
WebLoginSession.approve() unconditionally which allows a different user to
re-approve an already APPROVED session; modify the logic so you first check the
session state and identity before mutating: in authorizeWebSession (the
webLoginSessionService.findBySessionToken flow) or inside
WebLoginSession.approve(), verify that session.getStatus() is not already
APPROVED or, if APPROVED, that the stored approvedUserId/approvedUserRole match
the incoming userId/role and return idempotently without overwriting; only call
webLoginSessionService.save(session) when you actually change state, and log and
return false when an attempt to re-approve by a different user/role is detected.
- Around line 38-39: The `@Transactional` on WebLoginUseCase.createWebSession() is
misleading because WebLoginSessionRepository uses Spring Data Redis
CrudRepository which doesn’t participate in Spring transactions by default;
either remove the `@Transactional` annotation from the createWebSession method in
class WebLoginUseCase or enable Redis transaction support (e.g., configure
RedisConnectionFactory.setEnableTransactionSupport(true) and ensure
RedisTemplate is set up for transactions) so that WebLoginSessionRepository
operations actually run in a transaction; choose one approach and update the
code/comments accordingly to avoid misleading transactional semantics.
In `@src/main/java/im/toduck/infra/redis/weblogin/WebLoginSession.java`:
- Around line 53-57: The approve() method in WebLoginSession must validate
current state before transitioning: check that this.status ==
WebLoginSessionStatus.PENDING and if not, throw an IllegalStateException (or a
domain-specific exception) to prevent invalid transitions; only after the check
set this.status = WebLoginSessionStatus.APPROVED and assign this.approvedUserId
and this.approvedUserRole. Ensure the guard uses the WebLoginSessionStatus enum
(PENDING → APPROVED) and update any unit tests that exercise approve() to expect
the exception for non-PENDING sessions.
In `@src/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionStatus.java`:
- Around line 3-7: The enum WebLoginSessionStatus uses APPROVED but the PR
description refers to AUTHORIZED—make these consistent by either renaming the
enum constant APPROVED to AUTHORIZED in WebLoginSessionStatus or updating the
PR/docs to use APPROVED; if you rename, update every reference/comparison/usages
of WebLoginSessionStatus.APPROVED across the codebase (tests, serialization,
switch statements, DB mappings, e.g. any code that checks
WebLoginSessionStatus.APPROVED or persists that value) to
WebLoginSessionStatus.AUTHORIZED and run tests to ensure no enum name collisions
or serialization breaks.
---
Nitpick comments:
In `@src/main/java/im/toduck/domain/auth/domain/service/JwtService.java`:
- Around line 74-79: The createWebAccessToken method in JwtService currently
hardcodes Duration.ofDays(3); replace this with a configurable expiry injected
from external configuration (e.g., `@Value`("${jwt.web-access.expiry-days}") or a
Duration-based property) so the method uses that injected value when calling
accessTokenProvider.generateTokenWithCustomExpiry(AccessTokenClaim.of(userId,
role), ...); update JwtService to add the new field (or constructor parameter)
for the configured expiry and ensure the application.properties includes the
corresponding jwt.web-access.expiry-days (or a Duration property) so the token
lifetime is configurable at runtime.
In `@src/main/java/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.java`:
- Around line 100-103: In WebLoginUseCase replace the RuntimeException thrown in
the QR generation catch block (catching WriterException | IOException) with the
project-standard exception pattern used elsewhere (see JwtService): call
CommonException.from(...) with the appropriate ExceptionCode for QR generation
failure (e.g., ExceptionCode.QR_CODE_GENERATION or a similarly named constant)
and pass the caught exception as the cause so the error is logged and propagated
consistently.
In
`@src/main/java/im/toduck/domain/auth/presentation/controller/WebLoginController.java`:
- Around line 55-60: In extractRoleFromAuthorities in WebLoginController, don't
silently default to "USER" when getAuthorities() is empty or ignore multiple
roles; instead explicitly handle the empty-authorities case (e.g., throw an
IllegalStateException or an AuthenticationException with a clear message) and
implement a deterministic selection strategy for multiple authorities (for
example, prefer a higher-privilege role like "ADMIN" if present, otherwise pick
the first sorted/filtered role and strip the "ROLE_" prefix). Update
extractRoleFromAuthorities to validate userDetails.getAuthorities() is
non-empty, throw or fail fast on empty, and choose roles deterministically
(reference the method extractRoleFromAuthorities and
CustomUserDetails#getAuthorities).
- Around line 62-70: Session tokens are already cryptographically secure, but
you should protect the createWebSession endpoint from abuse by adding rate
limiting on the controller method (the createWebSession handler in
WebLoginController, currently annotated with `@PreAuthorize`("isAnonymous()")).
Implement a per-client rate limit (e.g., requests per minute) using your app's
chosen mechanism (Spring Cloud RateLimiter, Bucket4j with Redis backing, or a
custom filter) and apply it to createWebSession so excessive anonymous calls are
rejected or throttled; ensure limits are configurable and consider scoping by
client IP or username/session fingerprint.
In `@src/main/java/im/toduck/global/config/security/CorsConfig.java`:
- Around line 22-32: The CORS whitelist in CorsConfig currently includes a
development origin ("http://localhost:5173") alongside production origins;
update CorsConfig to load environment/profile-specific origins instead of
hardcoding localhost into the List literal (the code.building the origins List
in class CorsConfig), e.g., move the localhost entry out of the static List and
conditionally add it when active Spring profile is "dev" or via a config
property (read from application-{profile}.yaml or a spring property like
cors.allowed-origins) so production builds do not include
"http://localhost:5173" while keeping the existing production origins unchanged.
In `@src/main/java/im/toduck/global/security/jwt/access/AccessTokenProvider.java`:
- Around line 38-61: The two methods generateToken and
generateTokenWithCustomExpiry duplicate the same JWT build logic; refactor so
generateToken delegates to generateTokenWithCustomExpiry (or both delegate to a
single private helper) by passing tokenExpiration (tokenExpiration.toMillis())
as the custom expiry, keeping header creation via createHeader(), claims from
JwtClaims.getClaims(), signWith(secretKey) and expiration via
createExpireDate(now, ...); this removes duplication while preserving
createHeader, secretKey and createExpireDate usage.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
build.gradlesrc/main/java/im/toduck/domain/auth/domain/service/JwtService.javasrc/main/java/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.javasrc/main/java/im/toduck/domain/auth/presentation/api/WebLoginApi.javasrc/main/java/im/toduck/domain/auth/presentation/controller/WebLoginController.javasrc/main/java/im/toduck/domain/auth/presentation/dto/request/WebLoginAuthorizeRequest.javasrc/main/java/im/toduck/domain/auth/presentation/dto/response/WebSessionCreateResponse.javasrc/main/java/im/toduck/domain/auth/presentation/dto/response/WebSessionStatusResponse.javasrc/main/java/im/toduck/global/config/security/CorsConfig.javasrc/main/java/im/toduck/global/config/security/SecurityConfig.javasrc/main/java/im/toduck/global/security/jwt/access/AccessTokenProvider.javasrc/main/java/im/toduck/infra/redis/weblogin/WebLoginSession.javasrc/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionRepository.javasrc/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionService.javasrc/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionServiceImpl.javasrc/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionStatus.java
Comment on lines
+38
to
+39
| @Transactional | ||
| public WebSessionCreateResponse createWebSession() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the directory structure
find . -type f -name "WebLoginUseCase.java" | head -5Repository: toduck-App/toduck-backend
Length of output: 142
🏁 Script executed:
# Check if RedisTemplate transaction support is enabled anywhere in the project
rg -rn "setEnableTransactionSupport|enableTransactionSupport" --type=javaRepository: toduck-App/toduck-backend
Length of output: 51
🏁 Script executed:
# Check for Redis configuration and RedisTemplate usage
rg -rn "RedisTemplate" --type=java -A 2 -B 2Repository: toduck-App/toduck-backend
Length of output: 3046
🏁 Script executed:
# Look for Redis configuration classes
fd -e java | xargs grep -l "RedisConnectionFactory\|RedisTemplate" | head -20Repository: toduck-App/toduck-backend
Length of output: 392
🏁 Script executed:
cat -n src/main/java/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.javaRepository: toduck-App/toduck-backend
Length of output: 4703
🏁 Script executed:
cat -n src/main/java/im/toduck/global/config/cache/CacheConfig.javaRepository: toduck-App/toduck-backend
Length of output: 1788
🏁 Script executed:
find . -name "WebLoginSessionService*" -type fRepository: toduck-App/toduck-backend
Length of output: 222
🏁 Script executed:
find . -name "WebLoginSession.java" -type fRepository: toduck-App/toduck-backend
Length of output: 136
🏁 Script executed:
cat -n src/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionService.javaRepository: toduck-App/toduck-backend
Length of output: 949
🏁 Script executed:
cat -n src/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionServiceImpl.javaRepository: toduck-App/toduck-backend
Length of output: 1757
🏁 Script executed:
cat -n src/main/java/im/toduck/infra/redis/weblogin/WebLoginSession.javaRepository: toduck-App/toduck-backend
Length of output: 2235
🏁 Script executed:
find . -name "WebLoginSessionRepository*" -type fRepository: toduck-App/toduck-backend
Length of output: 146
🏁 Script executed:
cat -n src/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionRepository.javaRepository: toduck-App/toduck-backend
Length of output: 306
Redis CrudRepository 연산에서 @Transactional은 실질적 효과가 없습니다.
WebLoginSessionRepository는 Spring Data Redis의 CrudRepository를 상속하며, 이 저장소는 기본적으로 Spring의 @Transactional 트랜잭션에 참여하지 않습니다. 프로젝트에 Redis 트랜잭션 지원 설정(예: setEnableTransactionSupport(true))이 없으므로, 현재 세 메서드의 @Transactional 어노테이션은 오해의 소지가 있습니다. 명확성을 위해 제거하거나 실제 트랜잭션 지원을 설정하는 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.java`
around lines 38 - 39, The `@Transactional` on WebLoginUseCase.createWebSession()
is misleading because WebLoginSessionRepository uses Spring Data Redis
CrudRepository which doesn’t participate in Spring transactions by default;
either remove the `@Transactional` annotation from the createWebSession method in
class WebLoginUseCase or enable Redis transaction support (e.g., configure
RedisConnectionFactory.setEnableTransactionSupport(true) and ensure
RedisTemplate is set up for transactions) so that WebLoginSessionRepository
operations actually run in a transaction; choose one approach and update the
code/comments accordingly to avoid misleading transactional semantics.
Comment on lines
+52
to
+65
| @Transactional | ||
| public boolean authorizeWebSession(final Long userId, final String role, final WebLoginAuthorizeRequest request) { | ||
| return webLoginSessionService.findBySessionToken(request.sessionToken()) | ||
| .map(session -> { | ||
| session.approve(userId, role); | ||
| webLoginSessionService.save(session); | ||
| log.info("웹 로그인 세션 승인 - UserId: {}, SessionToken: {}", userId, request.sessionToken()); | ||
| return true; | ||
| }) | ||
| .orElseGet(() -> { | ||
| log.info("웹 로그인 세션 없음/만료 - UserId: {}, SessionToken: {}", userId, request.sessionToken()); | ||
| return false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
authorizeWebSession에서 이미 승인된 세션을 다른 사용자가 재승인할 수 있는 보안 취약점.
현재 approve() 호출 전에 세션 상태를 확인하지 않으므로, 이미 APPROVED 상태인 세션에 대해 다른 사용자가 approve()를 호출하면 approvedUserId와 approvedUserRole이 덮어씌워집니다. API 문서에는 "idempotent"라고 명시되어 있지만, 실제로는 다른 사용자의 인증 정보로 세션이 갈아치워질 수 있습니다.
제안하는 수정 방안
WebLoginSession.approve() 또는 authorizeWebSession()에서 현재 상태를 검증하세요:
public void approve(final Long userId, final String role) {
+ if (this.status != WebLoginSessionStatus.PENDING) {
+ throw new IllegalStateException("이미 승인되었거나 만료된 세션입니다.");
+ }
this.status = WebLoginSessionStatus.APPROVED;
this.approvedUserId = userId;
this.approvedUserRole = role;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.java`
around lines 52 - 65, authorizeWebSession currently calls
WebLoginSession.approve() unconditionally which allows a different user to
re-approve an already APPROVED session; modify the logic so you first check the
session state and identity before mutating: in authorizeWebSession (the
webLoginSessionService.findBySessionToken flow) or inside
WebLoginSession.approve(), verify that session.getStatus() is not already
APPROVED or, if APPROVED, that the stored approvedUserId/approvedUserRole match
the incoming userId/role and return idempotently without overwriting; only call
webLoginSessionService.save(session) when you actually change state, and log and
return false when an attempt to re-approve by a different user/role is detected.
Comment on lines
+67
to
+87
| @Transactional | ||
| public WebSessionStatusResponse getWebSessionStatus(final String sessionToken) { | ||
| return webLoginSessionService.findBySessionToken(sessionToken) | ||
| .map(session -> { | ||
| if (session.isApproved()) { | ||
| String accessToken = jwtService.createWebAccessToken( | ||
| session.getApprovedUserId(), | ||
| session.getApprovedUserRole() | ||
| ); | ||
| webLoginSessionService.deleteSession(sessionToken); | ||
| log.info("웹 로그인 토큰 발급 완료 - UserId: {}, SessionToken: {}", | ||
| session.getApprovedUserId(), sessionToken); | ||
| return WebSessionStatusResponse.approved(accessToken, session.getApprovedUserId()); | ||
| } | ||
| return WebSessionStatusResponse.pending(); | ||
| }) | ||
| .orElseGet(() -> { | ||
| log.info("웹 로그인 세션 만료 또는 미존재 - SessionToken: {}", sessionToken); | ||
| return WebSessionStatusResponse.expired(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
getWebSessionStatus에서 토큰 이중 발급 가능한 TOCTOU 레이스 컨디션.
동일한 APPROVED 세션에 대해 두 개의 동시 폴링 요청이 들어오면, 두 요청 모두 세션을 찾고 각각 access token을 발급한 뒤 삭제를 시도합니다. 결과적으로 하나의 세션에서 두 개의 유효한 access token이 발급될 수 있습니다.
@Transactional은 Spring Data Redis의 CrudRepository 기반 연산에 대해 기본적으로 트랜잭션 보장을 제공하지 않으므로, 이 어노테이션만으로는 원자성을 보장할 수 없습니다.
해결 방법:
- Redis의
DELETE명령이 성공한 경우에만(반환값 1) 토큰을 발급하는 방식으로 변경하거나, - Lua 스크립트를 통해 조회-삭제를 원자적으로 수행하거나,
- Redisson 등의 분산 락을 사용해 동시 접근을 제어하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/im/toduck/domain/auth/domain/usecase/WebLoginUseCase.java`
around lines 67 - 87, getWebSessionStatus has a TOCTOU race: two concurrent
pollers can both read an APPROVED session, each call
jwtService.createWebAccessToken and then deleteSession, producing duplicate
tokens; fix by making read-and-delete atomic — e.g., change the flow in
getWebSessionStatus to first attempt an atomic remove-and-return (use a Redis
DEL that returns count or a Lua script), call jwtService.createWebAccessToken
only when the delete result indicates the session was removed (delete returned
1), and otherwise return expired; alternatively implement a distributed lock
around webLoginSessionService.findBySessionToken/deleteSession (using Redisson)
so only the lock-holder issues the token; ensure you modify references in
getWebSessionStatus, webLoginSessionService.findBySessionToken,
webLoginSessionService.deleteSession, and the jwtService.createWebAccessToken
call sites accordingly.
Comment on lines
+53
to
+57
| public void approve(final Long userId, final String role) { | ||
| this.status = WebLoginSessionStatus.APPROVED; | ||
| this.approvedUserId = userId; | ||
| this.approvedUserRole = role; | ||
| } |
There was a problem hiding this comment.
approve() 메서드에서 현재 상태 검증이 필요합니다.
WebLoginUseCase 리뷰에서 언급한 것과 같이, PENDING 상태가 아닌 세션에 대한 approve() 호출을 방어해야 합니다. 상태 전이(PENDING → APPROVED)의 유효성 검증은 도메인 엔티티 레벨에서 수행하는 것이 가장 적절합니다.
제안하는 수정 방안
public void approve(final Long userId, final String role) {
+ if (this.status != WebLoginSessionStatus.PENDING) {
+ throw new IllegalStateException("PENDING 상태의 세션만 승인할 수 있습니다.");
+ }
this.status = WebLoginSessionStatus.APPROVED;
this.approvedUserId = userId;
this.approvedUserRole = role;
}📝 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 approve(final Long userId, final String role) { | |
| this.status = WebLoginSessionStatus.APPROVED; | |
| this.approvedUserId = userId; | |
| this.approvedUserRole = role; | |
| } | |
| public void approve(final Long userId, final String role) { | |
| if (this.status != WebLoginSessionStatus.PENDING) { | |
| throw new IllegalStateException("PENDING 상태의 세션만 승인할 수 있습니다."); | |
| } | |
| this.status = WebLoginSessionStatus.APPROVED; | |
| this.approvedUserId = userId; | |
| this.approvedUserRole = role; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/im/toduck/infra/redis/weblogin/WebLoginSession.java` around
lines 53 - 57, The approve() method in WebLoginSession must validate current
state before transitioning: check that this.status ==
WebLoginSessionStatus.PENDING and if not, throw an IllegalStateException (or a
domain-specific exception) to prevent invalid transitions; only after the check
set this.status = WebLoginSessionStatus.APPROVED and assign this.approvedUserId
and this.approvedUserRole. Ensure the guard uses the WebLoginSessionStatus enum
(PENDING → APPROVED) and update any unit tests that exercise approve() to expect
the exception for non-PENDING sessions.
Comment on lines
+3
to
+7
| public enum WebLoginSessionStatus { | ||
| PENDING, | ||
| APPROVED, | ||
| EXPIRED | ||
| } |
There was a problem hiding this comment.
PR 설명과 enum 값 불일치 확인 필요.
PR 설명에는 상태 전환이 PENDING → AUTHORIZED로 기술되어 있으나, 실제 코드에서는 APPROVED를 사용합니다. 코드 내부적으로는 일관성이 있어 보이지만, PR 설명 또는 관련 문서의 업데이트가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/im/toduck/infra/redis/weblogin/WebLoginSessionStatus.java`
around lines 3 - 7, The enum WebLoginSessionStatus uses APPROVED but the PR
description refers to AUTHORIZED—make these consistent by either renaming the
enum constant APPROVED to AUTHORIZED in WebLoginSessionStatus or updating the
PR/docs to use APPROVED; if you rename, update every reference/comparison/usages
of WebLoginSessionStatus.APPROVED across the codebase (tests, serialization,
switch statements, DB mappings, e.g. any code that checks
WebLoginSessionStatus.APPROVED or persists that value) to
WebLoginSessionStatus.AUTHORIZED and run tests to ensure no enum name collisions
or serialization breaks.
Merged
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.
✨ 작업 내용
1️⃣ QR 코드 기반 웹 로그인 세션 관리
2️⃣ 웹용 Access Token 발급
3️⃣ 보안 및 인프라 설정
web.toduck.app도메인 추가/v1/auth/web/authorize엔드포인트 인증 필수 설정Summary by CodeRabbit
릴리스 노트
새 기능
설정