Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements token prefix normalization and enhanced logging for Kafka-based user verification. The main purpose is to handle incoming verification tokens from Kafka that may include the prefix (e.g., "메모노기_"), removing it before database lookup since tokens are stored without prefixes in the database.
Changes:
- Added
normalizeTokenValue()method to strip the verification prefix from incoming tokens before database lookup - Added comprehensive logging via
logVerificationResult()for both successful and failed verification attempts - Wrapped the main verification logic in a try-catch block to handle and log exceptions
- Updated
saveFailureHistory()to accept the original token value for accurate logging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tokenValue, | ||
| serverName, | ||
| characterName | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The logging in logVerificationResult may expose sensitive information. The tokenValue is being logged in plain text, which could be a security concern if logs are stored insecurely or accessed by unauthorized personnel. Consider logging only a hash or partial value of the token (e.g., first 4 and last 4 characters) instead of the full token value, similar to how credit card numbers are typically masked.
| tokenValue, | |
| serverName, | |
| characterName | |
| ); | |
| } | |
| maskTokenValue(tokenValue), | |
| serverName, | |
| characterName | |
| ); | |
| } | |
| private String maskTokenValue(String tokenValue) { | |
| if (tokenValue == null) { | |
| return null; | |
| } | |
| int length = tokenValue.length(); | |
| if (length <= 8) { | |
| return "****"; | |
| } | |
| String prefix = tokenValue.substring(0, 4); | |
| String suffix = tokenValue.substring(length - 4); | |
| return prefix + "****" + suffix; | |
| } |
| throw e; | ||
| } catch (Exception e) { | ||
| log.error("Unexpected error while processing user verification payload. payload={}", payload, e); | ||
| throw new CustomException(GlobalExceptionCode.USER_VERIFICATION_INVALID); |
There was a problem hiding this comment.
The new try-catch block wraps all exception handling, but if an exception occurs during parseMessage, it returns null and the outer catch won't be triggered. However, if an exception occurs in saveFailureHistory (called within parseMessage), it will be caught by the outer catch block and re-thrown as USER_VERIFICATION_INVALID, which may not be the appropriate error code. Consider whether exceptions from saveFailureHistory should be allowed to propagate or should be handled differently.
| throw new CustomException(GlobalExceptionCode.USER_VERIFICATION_INVALID); | |
| if (e instanceof CustomException) { | |
| throw (CustomException) e; | |
| } | |
| throw e; |
| private String normalizeTokenValue(String tokenValue) { | ||
| if (tokenValue == null) { | ||
| return null; | ||
| } | ||
| return tokenValue.startsWith(verificationPrefix) | ||
| ? tokenValue.substring(verificationPrefix.length()) | ||
| : tokenValue; | ||
| } |
There was a problem hiding this comment.
The new normalizeTokenValue method and the updated token lookup logic are critical changes that should have test coverage. Since the repository has comprehensive test coverage for AuthService (src/test/java/until/the/eternity/das/auth/application/AuthServiceTest.java), similar test coverage should be added for UserVerificationService to verify that: 1) tokens with the prefix are correctly normalized and found, 2) tokens without the prefix are handled correctly, 3) the prefix is properly configured and used, and 4) edge cases like null or empty token values are handled.
| } | ||
| try { | ||
| UserVerificationVerifyMessage event = parseMessage(payload); | ||
| if (event == null) { |
There was a problem hiding this comment.
There's an inconsistency in error handling: when parseMessage returns null (due to invalid JSON), the method returns early without throwing an exception. However, for all other errors, exceptions are thrown. This means that invalid JSON payloads will be silently consumed and marked as successfully processed by Kafka, while other errors will cause message reprocessing. Consider either: 1) throwing an exception after saving failure history for invalid JSON to ensure consistent error handling, or 2) documenting why invalid JSON should not trigger Kafka reprocessing while other errors should.
| if (event == null) { | |
| if (event == null) { | |
| // Invalid JSON or otherwise unparsable payload. We deliberately do not throw an exception | |
| // here to avoid Kafka reprocessing of malformed messages. The message is consumed and | |
| // skipped after logging for observability. | |
| log.warn("Skipping unverifiable Kafka payload due to invalid JSON or parse failure: {}", payload); |
| } catch (CustomException e) { | ||
| log.error("User verification processing failed. payload={}", payload, e); | ||
| throw e; | ||
| } catch (Exception e) { | ||
| log.error("Unexpected error while processing user verification payload. payload={}", payload, e); | ||
| throw new CustomException(GlobalExceptionCode.USER_VERIFICATION_INVALID); | ||
| } |
There was a problem hiding this comment.
Re-throwing CustomException in the catch block will cause duplicate logging. The GlobalExceptionHandler already logs CustomExceptions at line 18. This means every CustomException thrown from verifyFromKafkaPayload will be logged twice: once here at line 235, and again in the GlobalExceptionHandler. Consider removing the log statement here and only re-throwing the exception, or catch CustomException in the inner exception handler (GlobalExceptionHandler) without logging if it's already been logged.
📋 상세 설명
📊 체크리스트