Skip to content

Conversation

@RCNR
Copy link
Contributor

@RCNR RCNR commented Sep 8, 2025

📍 PR 타입 (하나 이상 선택)

  • 기능 추가
  • 버그 수정
  • 의존성, 환경 변수, 빌드 관련 코드 업데이트
  • 기타 사소한 수정

🏷️ 관련 이슈

Close #92

📌 개요

  • redis 적용
  • 리플레시 토큰 재발급 코드가 기존 DB에서 redis 사용으로 변경
  • 로그아웃도 마찬가지

🔁 변경 사항

📸 스크린샷

✅ 체크리스트

  • 코드가 정상적으로 동작하는지 확인
  • PR에 적절한 라벨을 선택
  • 관련 테스트 코드 작성
  • 문서(README, Swagger 등) 업데이트

💡 추가 사항 (리뷰어가 참고해야 할 것)

Summary by CodeRabbit

  • New Features

    • Token reissue now returns the new access-token payload in responses.
    • Refresh tokens are stored in Redis with expiration, affecting logout and reissue flows.
  • Improvements

    • Updated JWT handling for more robust parsing and validation.
  • Chores

    • Added Redis configuration and dependencies for Redis and Jackson JSR310.

RCNR added 2 commits September 8, 2025 15:33
- 리플레시 토큰 저장을 DB가 아닌 레디스 이용
- 레디스 사용으로 인한 코드 수정
@RCNR RCNR self-assigned this Sep 8, 2025
@RCNR RCNR added the refactor 코드 리팩토링 라벨 label Sep 8, 2025
@RCNR RCNR linked an issue Sep 8, 2025 that may be closed by this pull request
2 tasks
@coderabbitai
Copy link

coderabbitai bot commented Sep 8, 2025

Walkthrough

Switches refresh-token persistence from DB to Redis (Lettuce), adds Redis wiring and templates, updates AuthService/RefreshTokenService to use Redis and TTLs, adjusts JwtProvider to decode/verify with the newer API, and returns an access-token payload in the reissue response. Adds Redis and Jackson JSR310 dependencies.

Changes

Cohort / File(s) Summary
Build dependencies
build.gradle
Added org.springframework.boot:spring-boot-starter-data-redis and com.fasterxml.jackson.datatype:jackson-datatype-jsr310.
Redis configuration
src/main/java/naughty/tuzamate/global/config/RedisConfig.java
New Lettuce-based Redis config exposing LettuceConnectionFactory and RedisTemplate<String, String> with StringRedisSerializer.
Auth controller response
src/main/java/naughty/tuzamate/auth/controller/AuthController.java
reissueToken now returns success with an AccessTokenDto payload and still sets the refresh-token cookie/header.
Custom response API
src/main/java/naughty/tuzamate/global/apiPayload/CustomResponse.java
onSuccess signature updated to accept (AuthSuccessCode, TokenResponse.AccessTokenDto) (was single-arg).
JWT provider updates
src/main/java/naughty/tuzamate/auth/jwt/JwtProvider.java
Base64URL decode of secret (Decoders), updated verification/parsing calls (verifyWith, parseSignedClaims), and logs exception messages before throwing TOKEN_INVALID.
Auth service: Redis-backed refresh tokens
src/main/java/naughty/tuzamate/auth/service/AuthService.java
Replaced DB refresh-token flow with StringRedisTemplate lookups/sets under key refreshToken:{userId}; logout deletes Redis key; reissue validates against Redis and writes new refresh token with TTL; constructor now injects StringRedisTemplate.
RefreshToken service refactor
src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java
Replaced DB repository with RedisTemplate<String, String> and ValueOperations; added getRefreshToken(Long), renamed deleteTokendeleteRefreshToken, removed validateToken; saves tokens with TTL computed from expire.
DTO change
src/main/java/naughty/tuzamate/auth/dto/TokenResponse.java
Added public static nested AccessTokenDto holding accessToken and getter.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor C as Client
  participant AC as AuthController
  participant AS as AuthService
  participant JP as JwtProvider
  participant R as Redis

  rect rgba(230,245,255,0.6)
  note over C,AC: Access token reissue (Redis-backed refresh-token)
  C->>AC: POST /auth/token/reissue (Refresh-Token cookie)
  AC->>AS: reissueToken(refreshToken)
  AS->>JP: parseSignedClaims(refreshToken)
  JP-->>AS: claims (userId, tokenVersion, exp)
  AS->>R: GET refreshToken:{userId}
  alt token missing or mismatch
    AS-->>AC: throw LOGGED_OUT_USER
    AC-->>C: 401 response
  else token matches
    AS->>JP: create new access & refresh tokens
    AS->>R: SET refreshToken:{userId} = newRefresh (TTL=refreshExp)
    AS-->>AC: TokenDto(new tokens)
    AC-->>C: 200 success + AccessTokenDto payload + Set-Cookie
  end
  end
Loading
sequenceDiagram
  autonumber
  actor C as Client
  participant AC as AuthController
  participant AS as AuthService
  participant R as Redis

  rect rgba(240,255,240,0.6)
  note over C,AS: Logout (Redis delete)
  C->>AC: POST /auth/logout
  AC->>AS: logout(userId)
  AS->>R: DEL refreshToken:{userId}
  AS-->>AC: void
  AC-->>C: 200 success
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • weeast1521

Pre-merge checks (2 passed, 3 warnings)

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning This PR introduces changes beyond Redis integration, including adjusting the AuthController response payload, updating JwtProvider secret decoding and exception logging, and adding the Jackson JSR310 dependency, which fall outside the scope of applying Redis to refresh token management. Please isolate these unrelated modifications into their own PRs or clearly justify their inclusion by mapping them to the linked issue objectives.
Description Check ⚠️ Warning The description includes the PR type, related issue link, and overview but leaves the “🔁 변경 사항”, “📸 스크린샷”, and “💡 추가 사항” sections empty, which are required by the repository template to describe code changes, screenshots (or a note if none), and reviewer notes. Please populate the “🔁 변경 사항” section with a concise list of the key code changes (e.g., added RedisConfig, updated AuthService, JwtProvider, RefreshTokenService, etc.), add a note under “📸 스크린샷” if no UI changes, and provide any relevant details in the “💡 추가 사항” section for reviewer context.
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Refactor: 레디스 적용” clearly and concisely summarizes the primary change of applying Redis to the project’s refresh token management without extraneous details or ambiguity.
Linked Issues Check ✅ Passed The PR fully implements the objectives from issue #92 by adding the RedisConfig for connection setup and refactoring all refresh token operations (storage, retrieval, invalidation) to use Redis via StringRedisTemplate and RedisTemplate, satisfying both the configuration and token management requirements.

Poem

I cached a token in a burrow of red,
Lettuce leaves whisper where sessions are led.
Keys hop softly with TTL in tow,
Logouts vanish like tracks in snow.
New access lights dawn — the bunny says "go!" 🥕

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
		  - name: "Undocumented Breaking Changes"
			  mode: "warning"
			  instructions: |
				  Flag potential breaking changes that are not documented:
				  1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
				  2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
				  3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.

Please share your feedback with us on this Discord post.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/#92-redis-refreshToken

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

@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: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/java/naughty/tuzamate/auth/controller/AuthController.java (1)

52-58: Fix cookie maxAge unit (milliseconds vs seconds).

jwtProvider.getRefreshExpiration() appears to be milliseconds (see usages in AuthService and login). ResponseCookie.maxAge(long) expects seconds; current code sets a 1000x longer lifetime.

Apply this diff:

+import java.time.Duration;
@@
-                .maxAge(jwtProvider.getRefreshExpiration())
+                .maxAge(Duration.ofMillis(jwtProvider.getRefreshExpiration()))
                 .build();
🧹 Nitpick comments (11)
build.gradle (1)

35-37: Redis + JSR310 deps look good under Spring Boot BOM

Both dependencies are correctly version-managed by Boot 3.3.5. Minor: jackson-datatype-jsr310 is already pulled by spring-boot-starter-json (via web starters), so the explicit line may be redundant. Safe to keep or remove.

-	implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jsr310'
+	// Optional: Boot's JSON starter already brings jackson-datatype-jsr310 transitively
src/main/java/naughty/tuzamate/auth/jwt/JwtProvider.java (3)

51-58: Let JJWT set the alg header; avoid hardcoding "alg" and duplicate algorithm selection

Manually setting "alg": "HS256" risks drift if the key/algorithm changes. JJWT sets it from signWith(secret).

-                .setHeader(Map.of("alg", "HS256", "typ", "JWT")) // JWT header 설정
+                // Let JJWT populate 'alg' from the key; 'typ' is optional and can be omitted
                 .setSubject(member.getEmail()) // JWT의 Subject을 email로 설정
                 .claim("id", member.getId()) // id 를 Claim으로 추가
                 .claim("tokenVersion", member.getTokenVersion()) // tokenVersion을 Claim으로 추가
                 .setIssuedAt(Date.from(issuedAt)) // 만들어진 시간을 현재 시간으로 설정
                 .setExpiration(Date.from(expiredAt)) // 유효기간 설정
-                .signWith(secret, SignatureAlgorithm.HS256) // 암호화를 위한 sign 설정
+                .signWith(secret) // algorithm inferred from key

74-83: Logging on parse failure: prefer warn/debug and include cause; avoid noisy info logs

log.info(e.getMessage()) on exceptions can be noisy and drop stack context. Use warn (or debug in high-traffic) and keep the cause.

-        } catch (Exception e) { // parsing하는 과정에서 sign key가 틀리는 등의 이유로 일어나는 Exception
-            log.info(e.getMessage());
-            throw new AuthException(JwtErrorCode.TOKEN_INVALID);
+        } catch (Exception e) { // 서명 불일치, 만료 등
+            log.warn("JWT parse/verify failed: {}", e.getMessage());
+            throw new AuthException(JwtErrorCode.TOKEN_INVALID);

15-15: Remove unused import

StandardCharsets is currently unused unless you adopt the fallback above.

- import java.nio.charset.StandardCharsets;
+ // import java.nio.charset.StandardCharsets; // only needed if using raw-text fallback
src/main/java/naughty/tuzamate/global/config/RedisConfig.java (1)

26-44: Optional: Switch RefreshTokenService to use StringRedisTemplate and remove custom RedisTemplate bean

RefreshTokenService still injects RedisTemplate<String, String> (src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java:23), while AuthService already uses StringRedisTemplate. If you don’t require any custom serialization on RedisTemplate, refactor RefreshTokenService to inject StringRedisTemplate and use its opsForValue(), then delete the custom RedisTemplate<String,String> bean in RedisConfig.

src/main/java/naughty/tuzamate/auth/controller/AuthController.java (1)

52-57: Externalize cookie attributes; validate SameSite for your front-end topology.

  • SameSite=Lax blocks cross-site POSTs (good for CSRF), but if your FE is on a different site and needs to send the cookie cross-site, use SameSite=None and keep secure=true.
  • Consider moving secure/sameSite/domain to config (profiles/env vars) to avoid committing “테스트 시 false” toggles.

Would you like a small property-driven CookieFactory to centralize these flags per environment?

src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java (2)

33-33: Deduplicate key format with a constant.

Avoid hardcoding "refreshToken:" in multiple places.

Apply this diff:

@@
-    public void saveRefreshToken(Long userId, String token, LocalDateTime expire) {
+    private static final String KEY_PREFIX = "refreshToken:";
+    public void saveRefreshToken(Long userId, String token, LocalDateTime expire) {
@@
-        String key = "refreshToken:" + userId;
+        String key = KEY_PREFIX + userId;
@@
-       String key = "refreshToken:" + userId;
+       String key = KEY_PREFIX + userId;
@@
-        String key = "refreshToken:" + userId;
+        String key = KEY_PREFIX + userId;

Also applies to: 39-39, 47-48


6-7: Remove unused imports.

RefreshToken/RefreshTokenRepository imports are no longer used.

Apply this diff:

-import naughty.tuzamate.auth.entity.RefreshToken;
-import naughty.tuzamate.auth.repository.RefreshTokenRepository;
src/main/java/naughty/tuzamate/auth/service/AuthService.java (3)

49-50: Use the RefreshTokenService instead of accessing Redis directly.

Centralizing key format and TTL logic in RefreshTokenService avoids duplication and drift with controller/service code.

Apply this diff to route through the service:

@@
-        redisTemplate.delete("refreshToken:" + userId);
+        refreshTokenService.deleteRefreshToken(userId);
@@
-        String storedRefreshToken = redisTemplate.opsForValue().get("refreshToken:" + userId);
+        String storedRefreshToken = refreshTokenService.getRefreshToken(userId).orElse(null);
@@
-        redisTemplate.opsForValue().set(
-                "refreshToken:" + userId,
-                newRefreshToken,
-                jwtProvider.getRefreshExpiration(),
-                TimeUnit.MILLISECONDS
-        );
+        refreshTokenService.saveRefreshToken(
+                userId,
+                newRefreshToken,
+                java.time.LocalDateTime.now().plusNanos(java.util.concurrent.TimeUnit.MILLISECONDS.toNanos(jwtProvider.getRefreshExpiration()))
+        );

Add the dependency and remove unused ones:

@@
-import naughty.tuzamate.auth.entity.RefreshToken;
@@
-import naughty.tuzamate.auth.repository.RefreshTokenRepository;
+import naughty.tuzamate.auth.service.RefreshTokenService;
@@
-import org.springframework.data.redis.core.StringRedisTemplate;
@@
-import java.util.concurrent.TimeUnit;
+import java.time.LocalDateTime;
@@
-    private final RefreshTokenRepository refreshTokenRepository;
-    private final StringRedisTemplate redisTemplate;
+    private final RefreshTokenService refreshTokenService;

63-77: Make refresh rotation atomic to avoid double-issue race.

Two concurrent refresh requests with the same token can both pass the equality check, then overwrite each other; one client receives a token that’s immediately invalid. Use a compare-and-set in Redis (Lua or WATCH/MULTI/EXEC).

Example (Lua via StringRedisTemplate; returns OK only if the provided old token matches):

String key = "refreshToken:" + userId;
String lua = "if redis.call('GET', KEYS[1]) == ARGV[1] then " +
             "return redis.call('SET', KEYS[1], ARGV[2], 'PX', ARGV[3]) " +
             "else return nil end";
boolean updated = Boolean.TRUE.equals(redisTemplate.execute((connection) ->
        connection.scriptingCommands().eval(
            lua.getBytes(StandardCharsets.UTF_8),
            ReturnType.STATUS, 1,
            key.getBytes(StandardCharsets.UTF_8),
            token.getBytes(StandardCharsets.UTF_8),
            newRefreshToken.getBytes(StandardCharsets.UTF_8),
            String.valueOf(jwtProvider.getRefreshExpiration()).getBytes(StandardCharsets.UTF_8)
        ) != null
));
if (!updated) throw new CustomException(UserErrorCode.LOGGED_OUT_USER);

Alternatively, Redis transactions with WATCH on the key achieve similar atomicity.


25-26: Remove unused JPA repository dependency.

RefreshTokenRepository is not used after the Redis refactor.

Apply this diff:

-    private final RefreshTokenRepository refreshTokenRepository;

Also remove its import.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6436191 and 55992f7.

📒 Files selected for processing (6)
  • build.gradle (1 hunks)
  • src/main/java/naughty/tuzamate/auth/controller/AuthController.java (1 hunks)
  • src/main/java/naughty/tuzamate/auth/jwt/JwtProvider.java (3 hunks)
  • src/main/java/naughty/tuzamate/auth/service/AuthService.java (3 hunks)
  • src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java (2 hunks)
  • src/main/java/naughty/tuzamate/global/config/RedisConfig.java (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/naughty/tuzamate/auth/jwt/JwtProvider.java (3)
src/main/java/naughty/tuzamate/auth/jwt/error/exception/AuthException.java (1)
  • AuthException (7-13)
src/main/java/naughty/tuzamate/domain/user/service/UserServiceImpl.java (1)
  • UserServiceImpl (24-85)
src/main/java/naughty/tuzamate/auth/jwt/JwtFilter.java (1)
  • doFilterInternal (37-78)
src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java (2)
src/main/java/naughty/tuzamate/auth/repository/RefreshTokenRepository.java (2)
  • RefreshTokenRepository (8-15)
  • deleteByUserId (12-12)
src/main/java/naughty/tuzamate/auth/entity/RefreshToken.java (2)
  • RefreshToken (8-34)
  • update (30-32)
src/main/java/naughty/tuzamate/auth/service/AuthService.java (4)
src/main/java/naughty/tuzamate/auth/jwt/JwtFilter.java (1)
  • RequiredArgsConstructor (29-80)
src/main/java/naughty/tuzamate/domain/user/service/UserServiceImpl.java (1)
  • UserServiceImpl (24-85)
src/main/java/naughty/tuzamate/auth/repository/RefreshTokenRepository.java (1)
  • RefreshTokenRepository (8-15)
src/main/java/naughty/tuzamate/auth/entity/RefreshToken.java (2)
  • RefreshToken (8-34)
  • update (30-32)
src/main/java/naughty/tuzamate/auth/controller/AuthController.java (1)
src/main/java/naughty/tuzamate/auth/dto/TokenResponse.java (2)
  • TokenDto (7-17)
  • TokenResponse (5-19)
🔇 Additional comments (2)
src/main/java/naughty/tuzamate/auth/jwt/JwtProvider.java (1)

29-33: Add fallback chain for JWT_SECRET decoding
Attempt URL-safe Base64, then standard Base64, then raw UTF-8 for backward compatibility; consider throwing an exception in production if raw text is used for stricter enforcement:

-    public JwtProvider(@Value("${JWT_SECRET}") String secret, @Value("${ACCESS_EXPIRATION}") long accessExpiration, @Value("${REFRESH_EXPIRATION}") long refreshExpiration) {
-        this.secret = Keys.hmacShaKeyFor(Decoders.BASE64URL.decode(secret));
+    public JwtProvider(@Value("${JWT_SECRET}") String secret,
+                       @Value("${ACCESS_EXPIRATION}") long accessExpiration,
+                       @Value("${REFRESH_EXPIRATION}") long refreshExpiration) {
+        byte[] keyBytes;
+        try {
+            keyBytes = Decoders.BASE64URL.decode(secret);
+        } catch (IllegalArgumentException e1) {
+            try {
+                keyBytes = Decoders.BASE64.decode(secret);
+            } catch (IllegalArgumentException e2) {
+                // Fallback to raw text for backward-compat; consider disallowing in prod.
+                keyBytes = secret.getBytes(StandardCharsets.UTF_8);
+            }
+        }
+        this.secret = Keys.hmacShaKeyFor(keyBytes);
         this.accessExpiration = accessExpiration;
         this.refreshExpiration = refreshExpiration;
     }

Ensure all deployments inject JWT_SECRET as Base64URL, Base64, or raw text to prevent breakages.

src/main/java/naughty/tuzamate/auth/service/AuthService.java (1)

18-18: Confirm refresh expiration units are consistent across storage and cookies.

AuthService uses milliseconds for Redis TTL; ensure jwtProvider.getRefreshExpiration() is in ms and that the controller’s cookie maxAge uses Duration.ofMillis(...) (see controller comment).

I can add an automated test to assert Redis TTL and cookie Max-Age alignment.

Also applies to: 72-77

Comment on lines 34 to 36
Duration ttl = Duration.between(LocalDateTime.now(), expire);
valueOperations.set(key, token, ttl);
}
Copy link

@coderabbitai coderabbitai bot Sep 8, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Guard against non-positive TTL when saving tokens.

If expire <= now, Duration.between(...) is zero/negative. RedisTemplate.set with non-positive TTL will either error or instantly expire.

Apply this diff:

-        Duration ttl = Duration.between(LocalDateTime.now(), expire);
-        valueOperations.set(key, token, ttl);
+        Duration ttl = Duration.between(LocalDateTime.now(), expire);
+        if (ttl.isZero() || ttl.isNegative()) {
+            log.warn("Skip saving refresh token: non-positive TTL for userId={}, expire={}", userId, expire);
+            redisTemplate.delete(key);
+            return;
+        }
+        valueOperations.set(key, token, ttl);
📝 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
Duration ttl = Duration.between(LocalDateTime.now(), expire);
valueOperations.set(key, token, ttl);
}
Duration ttl = Duration.between(LocalDateTime.now(), expire);
if (ttl.isZero() || ttl.isNegative()) {
log.warn("Skip saving refresh token: non-positive TTL for userId={}, expire={}", userId, expire);
redisTemplate.delete(key);
return;
}
valueOperations.set(key, token, ttl);
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java around
lines 34–36, guard against non-positive TTL returned by Duration.between(...)
before calling RedisTemplate.set: compute the Duration, check ttl.isPositive(),
and only call valueOperations.set(key, token, ttl) when positive; if ttl is zero
or negative, do not call set with a TTL (either skip saving the token and
optionally delete any existing key and log/debug the event, or call set without
a TTL if semantics require persistence), ensuring you avoid passing a
non-positive TTL to Redis.

Copy link
Contributor Author

@RCNR RCNR Sep 9, 2025

Choose a reason for hiding this comment

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

@coderabbitai
redisTemplate.delete(key);
새로운 토큰 생성에 실패했다는 이유로 아직 유효할 수 있는 기존 토큰을 삭제하는 건 사용자를 강제 로그아웃시키게 되는 거잖아.

return;
같은 경우 예외를 던지는 게 아니라 메서드를 그냥 종료하는 건 문제가 되는 부분이잖아.

Copy link

Choose a reason for hiding this comment

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

@RCNR 맞습니다! 두 지적 모두 정확합니다.

  1. 기존 토큰 삭제 문제: 새로운 토큰 저장에 실패했다고 해서 아직 유효한 기존 토큰을 삭제하는 것은 사용자를 불필요하게 강제 로그아웃시키는 문제가 있습니다.

  2. Silent failure 문제: return으로 조용히 실패하면 호출하는 쪽에서 저장 실패를 알 수 없어 문제가 됩니다.

더 나은 해결책을 제안드립니다:

public void saveRefreshToken(Long userId, String token, LocalDateTime expire) {
    String key = "refreshToken:" + userId;
    Duration ttl = Duration.between(LocalDateTime.now(), expire);
    
    if (ttl.isZero() || ttl.isNegative()) {
        log.error("Invalid expire time for refresh token: userId={}, expire={}, current={}", 
                 userId, expire, LocalDateTime.now());
        throw new IllegalArgumentException("Refresh token expire time must be in the future");
    }
    
    valueOperations.set(key, token, ttl);
}

이 방식은:

  • 기존 토큰을 삭제하지 않음
  • 명확한 예외를 던져 호출자가 실패를 인지할 수 있음
  • 로그를 통해 디버깅 정보 제공

✏️ Learnings added
Learnt from: RCNR
PR: NaughtyComputer/Naughty-BE-Spring#93
File: src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java:34-36
Timestamp: 2025-09-09T04:45:15.920Z
Learning: When handling refresh token save failures in authentication systems, avoid deleting existing valid tokens as it would unnecessarily force user logout. Instead of silent failures, throw appropriate exceptions to signal the error to calling code.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link

@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: 1

🧹 Nitpick comments (7)
src/main/java/naughty/tuzamate/auth/dto/TokenResponse.java (2)

23-25: Nit: align constructor param with field (and make deserialization future-proof)

Rename the parameter to accessToken; optionally annotate for Jackson to enable constructor-based deserialization if ever needed.

-        public AccessTokenDto(String newAccessToken) {
-            this.accessToken = newAccessToken;
+        @JsonCreator
+        public AccessTokenDto(@JsonProperty("accessToken") String accessToken) {
+            this.accessToken = accessToken;
         }

Add imports (outside this hunk):

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;

21-21: Optional: enrich OpenAPI docs with @Schema

Adds clear docs/examples to Swagger UI.

+        @Schema(description = "JWT access token", example = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...")
         private final String accessToken;

Add import (outside this hunk):

import io.swagger.v3.oas.annotations.media.Schema;
src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java (5)

41-42: Use domain exception instead of IllegalArgumentException for consistency.

Throw your project’s CustomException with an appropriate JwtErrorCode to integrate with global handlers and uniform error responses.

Apply this diff:

-            throw new IllegalArgumentException("만료 시간이 이미 지났습니다.");
+            // Use your project's domain exception/error code (adjust import/package as needed)
+            throw new CustomException(JwtErrorCode.TOKEN_INVALID);

23-29: Avoid caching ValueOperations; fetch per call and drop @PostConstruct.

Removes a potential NPE in non-Spring tests and simplifies lifecycle. RedisTemplate.opsForValue() is cheap and thread-safe.

Apply this diff:

-    private ValueOperations<String, String> valueOperations;
-
-    @PostConstruct
-    private void init() {
-        valueOperations = redisTemplate.opsForValue();
-    }
+    // Fetch ops per call; no PostConstruct needed.
-        valueOperations.set(key, token, ttl);
+        redisTemplate.opsForValue().set(key, token, ttl);
-       String token = valueOperations.get(key);
+       String token = redisTemplate.opsForValue().get(key);

Also applies to: 44-44, 49-49


33-33: DRY the Redis key construction.

Centralize the prefix and format to avoid typos and ease future changes.

Apply these diffs:

-        String key = "refreshToken:" + userId;
+        String key = keyOf(userId);
-       String key = "refreshToken:" + userId;
+       String key = keyOf(userId);
-        String key = "refreshToken:" + userId;
+        String key = keyOf(userId);

Add this helper in the class (outside the shown ranges):

private static final String KEY_PREFIX = "refreshToken:";

private String keyOf(Long userId) {
    return KEY_PREFIX + userId;
}

Also applies to: 48-48, 56-56


34-34: Decouple time source for testability.

Inject a Clock and use LocalDateTime.now(clock) to make TTL computation deterministic in tests and resilient to time drift.

Example:

private final Clock clock;

// ...
Duration ttl = Duration.between(LocalDateTime.now(clock), expire);

51-51: Nit: simplify Optional wrapping.

Apply this diff:

-       return token != null ? Optional.of(token) : Optional.empty();
+       return Optional.ofNullable(token);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 55992f7 and 1014289.

📒 Files selected for processing (4)
  • src/main/java/naughty/tuzamate/auth/controller/AuthController.java (1 hunks)
  • src/main/java/naughty/tuzamate/auth/dto/TokenResponse.java (1 hunks)
  • src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java (2 hunks)
  • src/main/java/naughty/tuzamate/global/config/RedisConfig.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/java/naughty/tuzamate/global/config/RedisConfig.java
  • src/main/java/naughty/tuzamate/auth/controller/AuthController.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T04:45:15.920Z
Learnt from: RCNR
PR: NaughtyComputer/Naughty-BE-Spring#93
File: src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java:34-36
Timestamp: 2025-09-09T04:45:15.920Z
Learning: When handling refresh token save failures in authentication systems, avoid deleting existing valid tokens as it would unnecessarily force user logout. Instead of silent failures, throw appropriate exceptions to signal the error to calling code.

Applied to files:

  • src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java
🧬 Code graph analysis (2)
src/main/java/naughty/tuzamate/auth/dto/TokenResponse.java (3)
src/main/java/naughty/tuzamate/auth/hantu/dto/ResponseHantuAccessTokenDto.java (1)
  • ResponseHantuAccessTokenDto (7-23)
src/main/java/naughty/tuzamate/domain/user/dto/UserResponseDTO.java (2)
  • UserResponseDTO (8-23)
  • UserTokenDTO (10-21)
src/main/java/naughty/tuzamate/auth/dto/kakao/KakaoOAuth2DTO.java (2)
  • OAuth2TokenDTO (7-15)
  • KakaoOAuth2DTO (5-53)
src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java (3)
src/main/java/naughty/tuzamate/auth/entity/RefreshToken.java (2)
  • RefreshToken (8-34)
  • update (30-32)
src/main/java/naughty/tuzamate/auth/repository/RefreshTokenRepository.java (2)
  • RefreshTokenRepository (8-15)
  • deleteByUserId (12-12)
src/main/java/naughty/tuzamate/auth/service/AuthService.java (1)
  • AuthService (17-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: deploy
🔇 Additional comments (3)
src/main/java/naughty/tuzamate/auth/dto/TokenResponse.java (1)

19-26: LGTM: lean AccessTokenDto suits reissue responses

Immutable, minimal payload avoids exposing refreshToken. Naming aligns with existing TokenDto.

src/main/java/naughty/tuzamate/auth/service/RefreshTokenService.java (2)

36-40: Good: TTL guard prevents non‑positive expiry.

This avoids instant-expiry writes and aligns with the prior guidance to fail fast instead of silently overriding state.


44-44: No changes required: RedisTemplate is already configured with StringRedisSerializer for both keys and values.

Comment on lines +19 to +26
@Getter
public static class AccessTokenDto {
private final String accessToken;

public AccessTokenDto(String newAccessToken) {
this.accessToken = newAccessToken;
}
}
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify client/API contract for reissue endpoint

Reissue now returns AccessTokenDto (accessToken only). Ensure Swagger/specs and consumers aren’t expecting TokenDto with refreshToken.

Run to locate call sites and docs to update:

I can update the Swagger annotations and a brief migration note for clients if you’d like.


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find usages of TokenDto vs AccessTokenDto
rg -n --type=java -C2 'TokenResponse\.(TokenDto|AccessTokenDto)'

# Find controller responses using onSuccess(...) to check payload types
rg -nP --type=java -C2 '\bonSuccess\s*\('

# Scan OpenAPI/YAML for schema/field mentions
fd -t f -e yml -e yaml -e yml | xargs -I{} rg -n 'AccessTokenDto|TokenDto|accessToken|refreshToken' {}

Length of output: 26428


Update Swagger/OpenAPI contract for the reissue endpoint

  • In AuthController.java’s reissueToken method, update the @ApiResponse/@Schema (or OpenAPI YAML) to reference AccessTokenDto (accessToken only) instead of TokenDto (which includes refreshToken)
  • Remove refreshToken from the response schema/docs and add a brief migration note for clients about the changed payload
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/auth/dto/TokenResponse.java around lines 19 to
26, the API docs still expose TokenDto (which includes refreshToken) for the
reissue endpoint; update the OpenAPI/Swagger annotations in
AuthController.reissueToken to reference AccessTokenDto (only accessToken)
instead of TokenDto, remove refreshToken from the response schema/docs, and add
a short migration note in the API documentation (or YAML) stating that the
reissue response no longer returns a refreshToken and clients must obtain/retain
refresh tokens via the prior flow.

@RCNR RCNR merged commit 7854b82 into dev Sep 9, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor 코드 리팩토링 라벨

Projects

None yet

Development

Successfully merging this pull request may close these issues.

♻️ [Refactor] redis 적용

2 participants