[refactor] Redis 기반 JWT 토큰 블랙리스트 도입 (#59)#60
Merged
Conversation
로그아웃 시 JWT 토큰 블랙리스트 관리를 위한 Redis 도입 준비 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
seokjun01
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Redis-backed token blacklist: new Redis config and TokenBlacklistService, blacklist checks in the JWT filter, blacklisting during auth deletion, an error code for blacklisted tokens, build/test dependency updates, and corresponding unit + integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Filter as JwtAuthenticationFilter
participant Provider as JwtTokenProvider
participant Blacklist as TokenBlacklistService
participant Redis as Redis
participant Security as SecurityContext
Client->>Filter: HTTP request with Authorization: Bearer <token>
Filter->>Provider: validateToken(token)
Provider-->>Filter: valid
Filter->>Blacklist: isBlacklisted(token)
Blacklist->>Redis: GET "blacklist:<token>"
Redis-->>Blacklist: exists / not exists
alt token is blacklisted
Blacklist-->>Filter: true
Filter-->>Client: throw JwtAuthenticationException (TOKEN_BLACKLISTED)
else token not blacklisted
Blacklist-->>Filter: false
Filter->>Provider: getUserIdFromToken(token)
Provider-->>Filter: userId
Filter->>Security: set Authentication(userId)
Filter-->>Client: proceed (request allowed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
src/main/java/tave/crezipsa/crezipsa/global/config/RedisConfig.java (1)
15-21: Hash-operation serializers are not configured.Only
keySerializerandvalueSerializerare set toStringRedisSerializer. The hash key/value serializers fall back toJdkSerializationRedisSerializer. If any code later addsopsForHash()calls, keys and values will be stored as binary JDK-serialized blobs, making them hard to inspect in Redis CLI and incompatible with non-Java clients.Consider adding
template.setHashKeySerializer/template.setHashValueSerializer(orsetDefaultSerializer) now for consistency.♻️ Proposed addition
template.setKeySerializer(new StringRedisSerializer()); template.setValueSerializer(new StringRedisSerializer()); + template.setHashKeySerializer(new StringRedisSerializer()); + template.setHashValueSerializer(new StringRedisSerializer()); return template;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/global/config/RedisConfig.java` around lines 15 - 21, The RedisTemplate in RedisConfig.redisTemplate currently sets only keySerializer and valueSerializer, leaving hash serializers to JDK binary serialization; update RedisConfig.redisTemplate to also configure hash serializers by calling template.setHashKeySerializer and template.setHashValueSerializer (or setDefaultSerializer) using StringRedisSerializer (or the same serializer instance used for keys/values) so opsForHash() stores human-readable strings and is compatible with non-Java clients.build.gradle (1)
59-60: Testcontainers versions are hardcoded; prefer Spring Boot's managed version.Since Spring Boot 3.1, Testcontainers is part of the automatic dependency management in
spring-boot-dependencies, so the BOM is imported automatically. Hardcoding1.20.4can diverge from the version the Spring Boot 3.5.x BOM expects and may cause subtle incompatibilities.♻️ Drop explicit version pins
- testImplementation 'org.testcontainers:testcontainers:1.20.4' - testImplementation 'org.testcontainers:junit-jupiter:1.20.4' + testImplementation 'org.testcontainers:testcontainers' + testImplementation 'org.testcontainers:junit-jupiter'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle` around lines 59 - 60, Remove the explicit Testcontainers version pins in the Gradle dependencies so Spring Boot's dependency management can control the version: update the two dependency declarations referencing org.testcontainers (the testImplementation lines for 'org.testcontainers:testcontainers:1.20.4' and 'org.testcontainers:junit-jupiter:1.20.4') to omit the “:1.20.4” version suffix so they rely on the Spring Boot BOM-managed version.src/test/java/tave/crezipsa/crezipsa/global/config/TestRedisConfig.java (1)
11-20:@ConditionalOnMissingBeanin a non-auto-configuration class may have ordering ambiguities.Spring Boot's official guidance is to use
@ConditionalOnMissingBean(and@ConditionalOnBean) only within auto-configuration classes, because conditions are evaluated against the beans registered so far, and bean registration order is not guaranteed in regular@Configurationclasses. In practice this works here because the integration test activates a realRedisConnectionFactoryfirst, but it is fragile as a contract.A more deterministic alternative is to keep the mock registration in a
@TestConfigurationand import it explicitly in the tests that need it, rather than relying on the conditional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/tave/crezipsa/crezipsa/global/config/TestRedisConfig.java` around lines 11 - 20, The TestRedisConfig class uses `@ConditionalOnMissingBean` inside a regular `@Configuration` which can produce ordering ambiguities; instead, move the mock bean into a dedicated `@TestConfiguration` class (e.g., rename TestRedisConfig to TestRedisTestConfig or create a new class annotated with `@TestConfiguration`) and define the mock RedisTemplate bean in that class via the redisTemplate() `@Bean` (remove the `@ConditionalOnMissingBean` there), then explicitly import or register that test configuration in the tests that need it (via `@Import`(TestRedisTestConfig.class) or by declaring it on the test slice), ensuring deterministic registration of the mocked RedisTemplate.src/test/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistServiceIntegrationTest.java (2)
17-17: Narrow the Spring Boot test context to avoid loading the web layer
@SpringBootTestdefaults toWebEnvironment.MOCK, spinning up the full application context including the security filter chain. Since this test exercises onlyTokenBlacklistServiceand its Redis backing,WebEnvironment.NONEreduces startup time and removes unnecessary dependencies.♻️ Proposed change
-@SpringBootTest +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistServiceIntegrationTest.java` at line 17, Update the test annotation on TokenBlacklistServiceIntegrationTest to narrow the Spring Boot context by specifying webEnvironment = SpringBootTest.WebEnvironment.NONE so the full web/security filter chain isn't loaded; keep the class-level `@SpringBootTest` but add the webEnvironment attribute to target only the service and Redis infrastructure.
48-50:Thread.sleepis slow and fragile; prefer AwaitilityA 2500 ms hard sleep adds wall-clock time to every CI run and has only a 500 ms margin over the 2000 ms TTL — enough to be flaky on slow runners. Awaitility lets you poll until the condition is met with a configurable timeout.
♻️ Proposed refactor using Awaitility
+import static org.awaitility.Awaitility.await; +import java.time.Duration; // Then - TTL 만료 후 조회 - Thread.sleep(2500L); - assertThat(tokenBlacklistService.isBlacklisted(token)).isFalse(); + await().atMost(Duration.ofSeconds(4)) + .pollInterval(Duration.ofMillis(200)) + .untilAsserted(() -> + assertThat(tokenBlacklistService.isBlacklisted(token)).isFalse());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistServiceIntegrationTest.java` around lines 48 - 50, Replace the brittle Thread.sleep-based wait in TokenBlacklistServiceIntegrationTest with an Awaitility-based poll: remove Thread.sleep(2500L) and instead use Awaitility.await().atMost(...) to repeatedly call tokenBlacklistService.isBlacklisted(token) until it becomes false, e.g. await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat(tokenBlacklistService.isBlacklisted(token)).isFalse()); add the necessary Awaitility imports and choose a sensible timeout (e.g. 5s) to avoid flakiness while keeping the test fast.src/test/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImplTest.java (1)
40-64: Consider asserting operation order in the "with Auth" test
verify()calls on separate mocks don't guarantee execution order. Since blacklisting before deleting from the DB is semantically important (avoids a window where the DB record is gone but the token is still valid and not yet blacklisted), usingInOrderwould make the intent explicit.♻️ Proposed addition
+import org.mockito.InOrder; +import static org.mockito.Mockito.inOrder; // Then - verify(tokenBlacklistService).blacklist(accessToken, remainingMs); - verify(authRepository).deleteByUserId(userId); + InOrder inOrder = inOrder(tokenBlacklistService, authRepository); + inOrder.verify(tokenBlacklistService).blacklist(accessToken, remainingMs); + inOrder.verify(authRepository).deleteByUserId(userId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImplTest.java` around lines 40 - 64, Update the test deleteToken_withAuth_blacklistsAndDeletes in AuthUsecaseImplTest to assert call order: create a Mockito InOrder for tokenBlacklistService and authRepository and use inOrder.verify(...) to assert tokenBlacklistService.blacklist(accessToken, remainingMs) is called before authRepository.deleteByUserId(userId), keeping the existing stubs (when(...).thenReturn(...)) and verifications for arguments intact.src/main/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistService.java (1)
14-14: Consider hashing the token before using it as a Redis keyThe raw JWT string (typically 200–500 characters) is stored verbatim as the key suffix. Any Redis
SCAN/KEYSoperation exposes the actual token values. Using a SHA-256 hex digest as the key suffix shrinks key size and removes any sensitive payload exposure from the Redis keyspace.♻️ Proposed refactor — hash token in key operations
+import org.springframework.util.DigestUtils; - private static final String BLACKLIST_PREFIX = "blacklist:"; + private static final String BLACKLIST_PREFIX = "blacklist:"; + private String keyFor(String token) { + return BLACKLIST_PREFIX + DigestUtils.md5DigestAsHex(token.getBytes()); + // or: BLACKLIST_PREFIX + Hashing.sha256().hashString(token, StandardCharsets.UTF_8) + } public void blacklist(String token, long remainingMs) { if (remainingMs > 0) { redisTemplate.opsForValue() - .set(BLACKLIST_PREFIX + token, "logout", remainingMs, TimeUnit.MILLISECONDS); + .set(keyFor(token), "logout", remainingMs, TimeUnit.MILLISECONDS); } } public boolean isBlacklisted(String token) { - return Boolean.TRUE.equals(redisTemplate.hasKey(BLACKLIST_PREFIX + token)); + return Boolean.TRUE.equals(redisTemplate.hasKey(keyFor(token))); }Also applies to: 20-21, 26-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistService.java` at line 14, TokenBlacklistService currently appends the raw JWT to BLACKLIST_PREFIX when building Redis keys; change key construction to use a SHA-256 hex digest of the token instead. Implement a private helper (e.g., computeTokenKey(String token)) that computes MessageDigest.getInstance("SHA-256") over token.getBytes(StandardCharsets.UTF_8") and returns BLACKLIST_PREFIX + hexDigest, and update all places in TokenBlacklistService that check, add, or remove tokens (methods that build keys for Redis operations) to call this helper so the raw token is never used as a Redis key suffix.
🤖 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/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImpl.java`:
- Around line 42-47: The blacklist call in AuthUsecaseImpl
(tokenBlacklistService.blacklist(...)) can throw and prevent
authRepository.deleteByUserId(userId) from running; wrap the blacklist logic
(including tokenProvider.getRemainingExpiration(...)) in a try/catch that
catches RuntimeException/checked transport errors, log a warning with context
(userId and accessToken) and proceed to call
authRepository.deleteByUserId(userId) regardless; adjust the Optional.ifPresent
lambda to either perform a guarded try/catch inside it or extract the Optional
first and handle blacklist in a safe block so failures in
tokenBlacklistService.blacklist do not stop deletion.
- Around line 42-47: The logout flow in AuthUsecaseImpl (the
authRepository.findByUserId(...) block that calls
tokenProvider.getRemainingExpiration and tokenBlacklistService.blacklist) can
throw ExpiredJwtException and prevent authRepository.deleteByUserId(userId) from
running; wrap the call to tokenProvider.getRemainingExpiration/access-token
processing in a try/catch that catches ExpiredJwtException (and any parsing
exceptions), and in the catch either skip blacklisting or call
tokenBlacklistService.blacklist with a zero/short TTL, but always ensure
authRepository.deleteByUserId(userId) executes afterwards; also add a brief
comment referencing JwtTokenProvider.getRemainingExpiration to remind to harden
that method too.
In `@src/main/java/tave/crezipsa/crezipsa/global/exception/code/ErrorCode.java`:
- Around line 22-25: Reorder the enum entries in ErrorCode so the declaration
order matches numeric progression: move TOKEN_BLACKLISTED(401, "A40107", "로그아웃된
토큰입니다.") to appear after KAKAO_HEADER_NOT_FOUND(400, "A40106", "카카오 토큰 헤더가
누락되었습니다.") (i.e., sequence TOKEN_EXPIRED → KAKAO_USERINFO_FAILED →
KAKAO_HEADER_NOT_FOUND → TOKEN_BLACKLISTED) ensuring commas and formatting
remain correct for the ErrorCode enum.
In `@src/main/java/tave/crezipsa/crezipsa/global/security/JwtTokenProvider.java`:
- Around line 67-74: The getRemainingExpiration method in JwtTokenProvider
currently calls Jwts.parser().parseClaimsJws(token) which throws
ExpiredJwtException for expired tokens; modify getRemainingExpiration to catch
ExpiredJwtException (and optionally other JwtException subclasses) around the
parseClaimsJws call and return 0 when caught so callers (e.g.,
TokenBlacklistService.blacklist()) receive a non-exceptional ≤0 value instead of
an unchecked exception.
In
`@src/main/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistService.java`:
- Around line 18-27: Catch Redis DataAccessException (e.g.,
RedisConnectionFailureException) inside both
TokenBlacklistService.blacklist(String token, long remainingMs) and
TokenBlacklistService.isBlacklisted(String token): in blacklist(...) wrap
redisTemplate.opsForValue().set(...) in a try/catch, log the exception
(including token and remainingMs) and swallow it so failures are silent but
recorded; in isBlacklisted(...) wrap redisTemplate.hasKey(...) in a try/catch,
log the exception and implement a fail-open behavior by returning false when a
DataAccessException occurs (do not rethrow). This keeps JwtAuthenticationFilter
flows safe (so JwtAuthenticationEntryPoint still handles auth errors) and
records Redis failures for debugging.
In
`@src/test/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImplTest.java`:
- Around line 66-78: In the test deleteToken_withoutAuth_onlyDeletes, add an
assertion that tokenBlacklistService.blacklist(...) is never invoked when
authRepository.findByUserId(userId) returns Optional.empty(): after verifying
authRepository.deleteByUserId(userId), add a Mockito verify call using
verify(tokenBlacklistService, never()).blacklist(any()) (or the appropriate
signature) to ensure AuthUsecaseImpl.deleteToken does not call
tokenBlacklistService.blacklist when no Auth exists.
---
Nitpick comments:
In `@build.gradle`:
- Around line 59-60: Remove the explicit Testcontainers version pins in the
Gradle dependencies so Spring Boot's dependency management can control the
version: update the two dependency declarations referencing org.testcontainers
(the testImplementation lines for 'org.testcontainers:testcontainers:1.20.4' and
'org.testcontainers:junit-jupiter:1.20.4') to omit the “:1.20.4” version suffix
so they rely on the Spring Boot BOM-managed version.
In `@src/main/java/tave/crezipsa/crezipsa/global/config/RedisConfig.java`:
- Around line 15-21: The RedisTemplate in RedisConfig.redisTemplate currently
sets only keySerializer and valueSerializer, leaving hash serializers to JDK
binary serialization; update RedisConfig.redisTemplate to also configure hash
serializers by calling template.setHashKeySerializer and
template.setHashValueSerializer (or setDefaultSerializer) using
StringRedisSerializer (or the same serializer instance used for keys/values) so
opsForHash() stores human-readable strings and is compatible with non-Java
clients.
In
`@src/main/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistService.java`:
- Line 14: TokenBlacklistService currently appends the raw JWT to
BLACKLIST_PREFIX when building Redis keys; change key construction to use a
SHA-256 hex digest of the token instead. Implement a private helper (e.g.,
computeTokenKey(String token)) that computes
MessageDigest.getInstance("SHA-256") over
token.getBytes(StandardCharsets.UTF_8") and returns BLACKLIST_PREFIX +
hexDigest, and update all places in TokenBlacklistService that check, add, or
remove tokens (methods that build keys for Redis operations) to call this helper
so the raw token is never used as a Redis key suffix.
In
`@src/test/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImplTest.java`:
- Around line 40-64: Update the test deleteToken_withAuth_blacklistsAndDeletes
in AuthUsecaseImplTest to assert call order: create a Mockito InOrder for
tokenBlacklistService and authRepository and use inOrder.verify(...) to assert
tokenBlacklistService.blacklist(accessToken, remainingMs) is called before
authRepository.deleteByUserId(userId), keeping the existing stubs
(when(...).thenReturn(...)) and verifications for arguments intact.
In `@src/test/java/tave/crezipsa/crezipsa/global/config/TestRedisConfig.java`:
- Around line 11-20: The TestRedisConfig class uses `@ConditionalOnMissingBean`
inside a regular `@Configuration` which can produce ordering ambiguities; instead,
move the mock bean into a dedicated `@TestConfiguration` class (e.g., rename
TestRedisConfig to TestRedisTestConfig or create a new class annotated with
`@TestConfiguration`) and define the mock RedisTemplate bean in that class via the
redisTemplate() `@Bean` (remove the `@ConditionalOnMissingBean` there), then
explicitly import or register that test configuration in the tests that need it
(via `@Import`(TestRedisTestConfig.class) or by declaring it on the test slice),
ensuring deterministic registration of the mocked RedisTemplate.
In
`@src/test/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistServiceIntegrationTest.java`:
- Line 17: Update the test annotation on TokenBlacklistServiceIntegrationTest to
narrow the Spring Boot context by specifying webEnvironment =
SpringBootTest.WebEnvironment.NONE so the full web/security filter chain isn't
loaded; keep the class-level `@SpringBootTest` but add the webEnvironment
attribute to target only the service and Redis infrastructure.
- Around line 48-50: Replace the brittle Thread.sleep-based wait in
TokenBlacklistServiceIntegrationTest with an Awaitility-based poll: remove
Thread.sleep(2500L) and instead use Awaitility.await().atMost(...) to repeatedly
call tokenBlacklistService.isBlacklisted(token) until it becomes false, e.g.
await().atMost(5, TimeUnit.SECONDS).untilAsserted(() ->
assertThat(tokenBlacklistService.isBlacklisted(token)).isFalse()); add the
necessary Awaitility imports and choose a sensible timeout (e.g. 5s) to avoid
flakiness while keeping the test fast.
Comment on lines
+42
to
47
| authRepository.findByUserId(userId).ifPresent(auth -> { | ||
| String accessToken = auth.getAccessToken(); | ||
| long remainingMs = tokenProvider.getRemainingExpiration(accessToken); | ||
| tokenBlacklistService.blacklist(accessToken, remainingMs); | ||
| }); | ||
| authRepository.deleteByUserId(userId); |
There was a problem hiding this comment.
No Redis-failure fallback — logout is entirely blocked when Redis is unavailable.
If tokenBlacklistService.blacklist() throws (connection refused, timeout, etc.), the exception propagates out of the lambda and authRepository.deleteByUserId(userId) is never called, leaving the auth record intact. The PR author flagged this as a known gap. The access token's natural expiry is a built-in safety net, so failing open (skip blacklist, still delete the DB record) is acceptable here.
🛡️ Proposed resilience fix
authRepository.findByUserId(userId).ifPresent(auth -> {
String accessToken = auth.getAccessToken();
try {
long remainingMs = tokenProvider.getRemainingExpiration(accessToken);
tokenBlacklistService.blacklist(accessToken, remainingMs);
+ } catch (Exception e) {
+ log.warn("Failed to blacklist token during logout for userId={}: {}", userId, e.getMessage());
}
});
authRepository.deleteByUserId(userId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImpl.java`
around lines 42 - 47, The blacklist call in AuthUsecaseImpl
(tokenBlacklistService.blacklist(...)) can throw and prevent
authRepository.deleteByUserId(userId) from running; wrap the blacklist logic
(including tokenProvider.getRemainingExpiration(...)) in a try/catch that
catches RuntimeException/checked transport errors, log a warning with context
(userId and accessToken) and proceed to call
authRepository.deleteByUserId(userId) regardless; adjust the Optional.ifPresent
lambda to either perform a guarded try/catch inside it or extract the Optional
first and handle blacklist in a safe block so failures in
tokenBlacklistService.blacklist do not stop deletion.
Logout fails with an unhandled ExpiredJwtException when the stored access token is already expired.
tokenProvider.getRemainingExpiration(accessToken) calls Jwts.parser().parseClaimsJws(token) internally, which throws ExpiredJwtException for expired tokens. That exception is not caught here, so authRepository.deleteByUserId(userId) is never reached and the auth record stays in the DB — the user cannot log out.
The fix belongs in JwtTokenProvider.getRemainingExpiration (see that file's comment), but deleteToken should also be resilient to it:
🐛 Proposed fix in deleteToken
authRepository.findByUserId(userId).ifPresent(auth -> {
String accessToken = auth.getAccessToken();
- long remainingMs = tokenProvider.getRemainingExpiration(accessToken);
- tokenBlacklistService.blacklist(accessToken, remainingMs);
+ try {
+ long remainingMs = tokenProvider.getRemainingExpiration(accessToken);
+ tokenBlacklistService.blacklist(accessToken, remainingMs);
+ } catch (JwtAuthenticationException e) {
+ // Token already expired — no need to blacklist; proceed with DB deletion
+ log.debug("Access token already expired during logout for userId={}", userId);
+ }
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImpl.java`
around lines 42 - 47, The logout flow in AuthUsecaseImpl (the
authRepository.findByUserId(...) block that calls
tokenProvider.getRemainingExpiration and tokenBlacklistService.blacklist) can
throw ExpiredJwtException and prevent authRepository.deleteByUserId(userId) from
running; wrap the call to tokenProvider.getRemainingExpiration/access-token
processing in a try/catch that catches ExpiredJwtException (and any parsing
exceptions), and in the catch either skip blacklisting or call
tokenBlacklistService.blacklist with a zero/short TTL, but always ensure
authRepository.deleteByUserId(userId) executes afterwards; also add a brief
comment referencing JwtTokenProvider.getRemainingExpiration to remind to harden
that method too.
Comment on lines
22
to
25
| TOKEN_EXPIRED(401,"A40104","토큰 유효시간이 만료되었습니다."), | ||
| TOKEN_BLACKLISTED(401, "A40107", "로그아웃된 토큰입니다."), | ||
| KAKAO_USERINFO_FAILED(400, "A40105", "카카오 사용자 정보를 가져오지 못했습니다."), | ||
| KAKAO_HEADER_NOT_FOUND(400, "A40106", "카카오 토큰 헤더가 누락되었습니다."), |
There was a problem hiding this comment.
TOKEN_BLACKLISTED (A40107) is declared before A40105/A40106 in the enum body.
The code value A40107 is numerically correct (after A40106), but the enum declaration order breaks the ascending sequence: A40104 → A40107 → A40105 → A40106. Moving TOKEN_BLACKLISTED after KAKAO_HEADER_NOT_FOUND (A40106) keeps the section readable. Based on learnings, codes and status should remain consistent — the value itself is fine, this is purely a declaration-order nit.
🔧 Suggested reorder
TOKEN_EXPIRED(401,"A40104","토큰 유효시간이 만료되었습니다."),
- TOKEN_BLACKLISTED(401, "A40107", "로그아웃된 토큰입니다."),
KAKAO_USERINFO_FAILED(400, "A40105", "카카오 사용자 정보를 가져오지 못했습니다."),
KAKAO_HEADER_NOT_FOUND(400, "A40106", "카카오 토큰 헤더가 누락되었습니다."),
+ TOKEN_BLACKLISTED(401, "A40107", "로그아웃된 토큰입니다."),📝 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
| TOKEN_EXPIRED(401,"A40104","토큰 유효시간이 만료되었습니다."), | |
| TOKEN_BLACKLISTED(401, "A40107", "로그아웃된 토큰입니다."), | |
| KAKAO_USERINFO_FAILED(400, "A40105", "카카오 사용자 정보를 가져오지 못했습니다."), | |
| KAKAO_HEADER_NOT_FOUND(400, "A40106", "카카오 토큰 헤더가 누락되었습니다."), | |
| TOKEN_EXPIRED(401,"A40104","토큰 유효시간이 만료되었습니다."), | |
| KAKAO_USERINFO_FAILED(400, "A40105", "카카오 사용자 정보를 가져오지 못했습니다."), | |
| KAKAO_HEADER_NOT_FOUND(400, "A40106", "카카오 토큰 헤더가 누락되었습니다."), | |
| TOKEN_BLACKLISTED(401, "A40107", "로그아웃된 토큰입니다."), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/tave/crezipsa/crezipsa/global/exception/code/ErrorCode.java`
around lines 22 - 25, Reorder the enum entries in ErrorCode so the declaration
order matches numeric progression: move TOKEN_BLACKLISTED(401, "A40107", "로그아웃된
토큰입니다.") to appear after KAKAO_HEADER_NOT_FOUND(400, "A40106", "카카오 토큰 헤더가
누락되었습니다.") (i.e., sequence TOKEN_EXPIRED → KAKAO_USERINFO_FAILED →
KAKAO_HEADER_NOT_FOUND → TOKEN_BLACKLISTED) ensuring commas and formatting
remain correct for the ErrorCode enum.
Comment on lines
+66
to
+78
| @Test | ||
| @DisplayName("Auth가 존재하지 않으면 블랙리스트 등록 없이 삭제만 수행한다") | ||
| void deleteToken_withoutAuth_onlyDeletes() { | ||
| // Given | ||
| Long userId = 1L; | ||
| when(authRepository.findByUserId(userId)).thenReturn(Optional.empty()); | ||
|
|
||
| // When | ||
| authUsecase.deleteToken(userId); | ||
|
|
||
| // Then | ||
| verify(authRepository).deleteByUserId(userId); | ||
| } |
There was a problem hiding this comment.
Missing assertion that tokenBlacklistService.blacklist() is never called when Auth is absent
deleteToken_withoutAuth_onlyDeletes currently only verifies deleteByUserId() is called, but never asserts that blacklist() is NOT called. If AuthUsecaseImpl incorrectly invokes blacklist() even for a missing auth, this test silently passes because Mockito void mocks are no-ops.
🛠️ Proposed fix
+import static org.mockito.Mockito.never;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.anyString;
// Then
+ verify(tokenBlacklistService, never()).blacklist(anyString(), anyLong());
verify(authRepository).deleteByUserId(userId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImplTest.java`
around lines 66 - 78, In the test deleteToken_withoutAuth_onlyDeletes, add an
assertion that tokenBlacklistService.blacklist(...) is never invoked when
authRepository.findByUserId(userId) returns Optional.empty(): after verifying
authRepository.deleteByUserId(userId), add a Mockito verify call using
verify(tokenBlacklistService, never()).blacklist(any()) (or the appropriate
signature) to ensure AuthUsecaseImpl.deleteToken does not call
tokenBlacklistService.blacklist when no Auth exists.
- JwtTokenProvider.getRemainingExpiration: 만료된 토큰 ExpiredJwtException catch하여 0 반환 - TokenBlacklistService.blacklist: Redis 연결 실패 시 warn 로그 후 정상 진행 - TokenBlacklistService.isBlacklisted: Redis 연결 실패 시 false 반환하여 요청 허용 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/tave/crezipsa/crezipsa/global/security/JwtTokenProvider.java (1)
66-78: LGTM — past concern fully addressed.The
ExpiredJwtExceptioncatch withreturn 0and theMath.max(0, …)guard both resolve the previous issue. One optional defensive improvement: otherJwtExceptionsubtypes (MalformedJwtException,SignatureException, etc.) are currently uncaught. In practice this is fine because all callers receive tokens that have already passedvalidateToken, but making the contract explicit is low-cost.🛡️ Optional: catch all JwtException subtypes
public long getRemainingExpiration(String token) { try { Date expiration = Jwts.parser() .setSigningKey(secretKey) .parseClaimsJws(token) .getBody() .getExpiration(); return Math.max(0, expiration.getTime() - System.currentTimeMillis()); } catch (ExpiredJwtException e) { return 0; + } catch (JwtException e) { + return 0; // malformed / tampered — treat as non-blacklistable } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/global/security/JwtTokenProvider.java` around lines 66 - 78, In getRemainingExpiration in JwtTokenProvider, add a catch for general JwtException (e.g., catch (JwtException e)) after the existing ExpiredJwtException handler and return 0 there; this ensures malformed/signature/other JwtException subtypes are handled consistently when parsing fails, keeping the method contract of returning 0 for invalid/expired tokens.src/main/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistService.java (1)
20-39: PreferDataAccessExceptionoverExceptionin both catch blocks.The past concern about missing exception handling is now resolved. However,
catch (Exception e)is overly broad and will silently swallow unrelated runtime exceptions (e.g.,NullPointerException,ClassCastException) that should surface as bugs rather than be suppressed with a warn log.Spring Data Redis performs "exception translation to Spring's portable Data Access exception hierarchy", and
RedisConnectionFailureExceptionextendsDataAccessResourceFailureException, which itself extendsDataAccessException— so all Redis I/O failures are reliably caught byDataAccessExceptionwithout over-catching.♻️ Proposed fix — narrow catch to DataAccessException
+import org.springframework.dao.DataAccessException; import org.springframework.data.redis.core.RedisTemplate; public void blacklist(String token, long remainingMs) { if (remainingMs <= 0) { return; } try { redisTemplate.opsForValue() .set(BLACKLIST_PREFIX + token, "logout", remainingMs, TimeUnit.MILLISECONDS); - } catch (Exception e) { + } catch (DataAccessException e) { log.warn("Redis 블랙리스트 등록 실패 — 토큰은 자연 만료까지 유효합니다: {}", e.getMessage()); } } public boolean isBlacklisted(String token) { try { return Boolean.TRUE.equals(redisTemplate.hasKey(BLACKLIST_PREFIX + token)); - } catch (Exception e) { + } catch (DataAccessException e) { log.warn("Redis 블랙리스트 조회 실패 — 요청을 허용합니다: {}", e.getMessage()); return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistService.java` around lines 20 - 39, The catch clauses in blacklist(...) and isBlacklisted(...) are too broad; replace both catch (Exception e) with catch (org.springframework.dao.DataAccessException e) so only Spring Data access errors from redisTemplate are swallowed and logged, keep the same log messages and return behavior, and add the import for org.springframework.dao.DataAccessException if missing; target the methods blacklist, isBlacklisted and the redisTemplate/BLACKLIST_PREFIX usages when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/tave/crezipsa/crezipsa/global/security/JwtTokenProvider.java`:
- Around line 66-78: In getRemainingExpiration in JwtTokenProvider, add a catch
for general JwtException (e.g., catch (JwtException e)) after the existing
ExpiredJwtException handler and return 0 there; this ensures
malformed/signature/other JwtException subtypes are handled consistently when
parsing fails, keeping the method contract of returning 0 for invalid/expired
tokens.
In
`@src/main/java/tave/crezipsa/crezipsa/global/security/TokenBlacklistService.java`:
- Around line 20-39: The catch clauses in blacklist(...) and isBlacklisted(...)
are too broad; replace both catch (Exception e) with catch
(org.springframework.dao.DataAccessException e) so only Spring Data access
errors from redisTemplate are swallowed and logged, keep the same log messages
and return behavior, and add the import for
org.springframework.dao.DataAccessException if missing; target the methods
blacklist, isBlacklisted and the redisTemplate/BLACKLIST_PREFIX usages when
making this change.
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.
개요
로그아웃 시 DB의 Auth 레코드를 삭제하지만, 이미 발급된 JWT Access Token은 만료 전까지 여전히 유효한 보안 이슈를 해결합니다.
Redis에 로그아웃된 토큰을 블랙리스트로 등록하여, JWT 필터에서 매 요청마다 블랙리스트 여부를 체크합니다. 블랙리스트에 등록된 토큰은 남은 만료시간(TTL)만큼만 Redis에 저장되어, 만료 후 자동으로 삭제됩니다.
변경 사항
1. 인프라 환경 세팅
build.gradlespring-boot-starter-data-redis,testcontainers의존성 추가docker/docker-compose.ymldocker/docker-compose.dev.ymldocker/docker-compose.prod.ymlapplication.ymlspring.data.redis.host/port환경변수 설정 추가application-test.ymlapplication-redis-test.yml2. 핵심 코드
RedisConfig.javaRedisTemplate<String, String>Bean 정의.@ConditionalOnBean(RedisConnectionFactory.class)로 테스트 환경에서 자동 skipTokenBlacklistService.javablacklist(token, remainingMs): 토큰을 남은 TTL만큼 Redis에 저장.isBlacklisted(token): 블랙리스트 등록 여부 확인ErrorCode.javaTOKEN_BLACKLISTED(401, "A40107", "로그아웃된 토큰입니다.")추가JwtTokenProvider.javagetRemainingExpiration(token)메서드 추가 — 토큰의 남은 만료시간(ms) 계산JwtAuthenticationFilter.javatokenBlacklistService.isBlacklisted(token)체크 추가. 블랙리스트 토큰이면TOKEN_BLACKLISTED예외 발생AuthUsecaseImpl.javadeleteToken()수정 — Auth 조회 → accessToken 추출 → 남은 TTL 계산 → Redis 블랙리스트 등록 → DB 삭제3. 테스트
TokenBlacklistServiceTest.javaTokenBlacklistServiceIntegrationTest.javaJwtAuthenticationFilterTest.javaAuthUsecaseImplTest.javaTestRedisConfig.java@Profile("test")환경에서 mockRedisTemplate제공흐름도
배포 시 필요한 작업
DEV_ENV값에REDIS_HOST=redis,REDIS_PORT=6379추가PROD_ENV값에REDIS_HOST=redis,REDIS_PORT=6379추가docker compose up -d로 Redis 서비스 시작 확인셀프 피드백
잘한 점
test/redis-test로 분리하여 단위 테스트와 통합 테스트 간 context 간섭 방지@ConditionalOnBean(RedisConnectionFactory.class)적용으로 Redis 미연결 환경에서도 빌드 안정성 확보개선 고려 사항
deleteToken()의 accessToken 만료 예외 처리: 현재getRemainingExpiration()은 이미 만료된 토큰에 대해ExpiredJwtException을 던질 수 있음. 로그아웃 시점에 토큰이 이미 만료된 경우 블랙리스트 등록을 skip하는 try-catch가 필요할 수 있음Redis 장애 시 Fallback: 현재 Redis 연결 실패 시
isBlacklisted()가 예외를 던져 모든 요청이 실패할 수 있음. Redis 장애 시 false를 반환하는 방어 로직 추가를 고려할 수 있음 (보안 vs 가용성 트레이드오프)Refresh Token 블랙리스트: 현재 Access Token만 블랙리스트에 등록하지만, Refresh Token도 함께 블랙리스트에 등록하면 토큰 재발급까지 완전 차단 가능
docker-compose,application.yml등.gitignore파일: 현재 git 추적 대상이 아니어서 팀원에게 로컬 설정 업데이트 공유가 필요함리뷰 반영 (d91f43a)
getRemainingExpiration()이 만료된 토큰에서ExpiredJwtException을 던져 로그아웃이 실패하는 문제catch (ExpiredJwtException)추가, 0 반환하여 블랙리스트 skip + DB 삭제 정상 진행
Redis 장애 시→blacklist()예외 전파로deleteByUserId()미실행try-catch로 감싸고 warn 로그. 예외 전파 차단하여 DB 삭제항상 실행
Redis 장애 시→isBlacklisted()예외로 모든 인증 요청 HTTP 500try-catch로 감싸고false반환. Redis 장애 시 요청 허용(가용성 우선, 토큰 자연 만료가 안전망)
테스트 결과
Test plan
TokenBlacklistService단위 테스트 통과 (blacklist/isBlacklisted)TokenBlacklistServiceTestcontainers Redis 연동 테스트 통과 (set → 조회 → TTL 만료)JwtAuthenticationFilter블랙리스트 토큰 인증 실패 테스트 통과AuthUsecaseImpl로그아웃 시 블랙리스트 등록 테스트 통과CrezipsaApplicationTestscontext load 테스트 통과 (Redis mock 환경)./gradlew test전체 테스트 통과Summary by CodeRabbit
New Features
Tests
Chores