Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a user verification system that integrates with Kafka to process verification events. The system allows users to generate verification tokens, which are validated when Kafka messages are received from a game server, linking in-game character identities to user accounts.
Changes:
- Added database tables for user verification tokens, verification status, and verification history tracking
- Implemented Kafka consumer to process verification events from game servers
- Created REST API endpoints for token generation, retrieval, and verification status queries
- Extended User entity with verification status fields and game server information
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| V20260213110000__add_user_verification_tables.sql | Creates four new tables for user verification system with appropriate indexes and foreign key constraints |
| application-sample.yml | Adds Kafka consumer configuration and verification-related application properties |
| UserVerificationController.java | Provides REST endpoints for token management and verification status queries |
| UserVerificationVerifyMessage.java | Defines Kafka message structure for verification events |
| UserVerificationKafkaConsumer.java | Consumes and processes verification messages from Kafka topics |
| VerificationTokenStatus.java | Enum defining possible token states |
| VerificationFailureReason.java | Enum defining verification failure reasons |
| UserVerificationTokenRepository.java | JPA repository for token persistence with custom query methods |
| UserVerificationToken.java | Entity representing verification tokens with lifecycle methods |
| UserVerificationRepository.java | JPA repository for user verification summary data |
| UserVerificationHistoryRepository.java | JPA repository for verification attempt history |
| UserVerificationHistory.java | Entity tracking all verification attempts |
| UserVerification.java | Entity storing current verification status per user |
| UserVerificationTokenResponse.java | DTO for token information responses |
| UserVerificationTokenIssueResponse.java | DTO for token issuance responses |
| UserVerificationInfoResponse.java | DTO for user verification status information |
| UserVerificationHistoryResponse.java | DTO for individual verification history records |
| UserVerificationHistoryListResponse.java | DTO for paginated verification history |
| UserVerificationService.java | Core service implementing token generation, Kafka message processing, and verification logic |
| UserRepository.java | Adds nickname uniqueness check method |
| User.java | Extends entity with verification status and game profile fields |
| GlobalExceptionCode.java | Adds error codes for verification-related failures |
| UserUpdateConstant.java | Adds Kafka topic constant for verification events |
| KafkaListenerConfig.java | Enables Kafka listener functionality |
| docker-compose-local.yml | Adds local development Docker configuration with Kafka connectivity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public class UserUpdateConstant { | ||
| public static final String INFO = "USER_INFO_UPDATE_EVENT"; | ||
| public static final String PASSWORD = "PASSWORD_UPDATE_EVENT"; | ||
| public static final String USER_VERIFICATION_VERIFY = "USER_VERIFICATION_VERIFY_EVENT"; |
There was a problem hiding this comment.
The constant USER_VERIFICATION_VERIFY is defined but appears to duplicate the functionality of the kafka topic configuration. The constant name suggests it should match the topic name in application-sample.yml, but this creates a maintenance burden if the topic name changes. Consider either removing this constant and using the property directly, or using this constant consistently throughout the codebase instead of string literals in configuration.
| public static final String USER_VERIFICATION_VERIFY = "USER_VERIFICATION_VERIFY_EVENT"; |
| if ("oldest".equals(normalizedSort)) { | ||
| items = items.stream().collect(java.util.stream.Collectors.toList()); | ||
| Collections.reverse(items); | ||
| } |
There was a problem hiding this comment.
The conversion from mutable to immutable list on line 114 is unnecessary and inefficient. The stream().toList() on line 111 already returns an immutable list. If the intention is to reverse the list, use a different approach such as creating the list directly as ArrayList or use Collections.reverse directly on a mutable copy created only when needed.
| invalidatePreviousOwner(event.serverName(), event.characterName(), user.getId()); | ||
|
|
||
| UserVerification verification = userVerificationRepository.findByUserId(user.getId()) | ||
| .orElseGet(() -> UserVerification.builder().user(user).verificationCount(0).verified(false).build()); | ||
|
|
||
| verification.markVerified(event.serverName(), event.characterName(), now, token.getId()); | ||
| token.markVerified(now); | ||
|
|
||
| user.updateServerName(event.serverName()); | ||
| user.updateVerificationStatus(true, now); |
There was a problem hiding this comment.
Race condition potential: When a character identity changes ownership from one user to another (lines 180-189), the invalidation of the previous owner and verification of the new owner are not done atomically within a single transaction. Although the method has @transactional, if two verification messages for the same character arrive simultaneously, it could lead to inconsistent state. Consider adding database-level locking (pessimistic locking) on the UserVerification record for the server/character combination to prevent race conditions.
| public void updateGameProfile(String nickname, String serverName) { | ||
| this.nickname = nickname; | ||
| this.serverName = serverName; | ||
| } |
There was a problem hiding this comment.
The updateGameProfile method allows updating both nickname and serverName together, but there's no validation to ensure the nickname doesn't conflict with existing users. This could lead to duplicate nickname constraint violations. Consider adding validation or using the existsByNicknameAndIdNot method to check for conflicts before updating, or document that validation should be done by the caller.
| private String toHex(byte[] bytes) { | ||
| StringBuilder builder = new StringBuilder(bytes.length * 2); | ||
| for (byte b : bytes) { | ||
| builder.append(String.format("%02X", b)); | ||
| } | ||
| return builder.toString(); | ||
| } |
There was a problem hiding this comment.
The toHex method converts bytes to uppercase hex strings (line 385), but many hex encoding conventions use lowercase. While this is not inherently wrong, consider documenting this choice or using lowercase for consistency with common hex encoding standards. This affects token values that are stored in the database and returned to users.
| boolean existsByNicknameAndIdNot(String nickname, Long id); | ||
|
|
There was a problem hiding this comment.
The existsByNicknameAndIdNot method is added but not used anywhere in this PR. If this method is intended for future use or is part of another feature, it should be documented. Otherwise, consider removing it to keep the codebase clean.
| boolean existsByNicknameAndIdNot(String nickname, Long id); |
| DB_PASSWORD: ${DB_PASSWORD:-devnogi0529!} | ||
| KAFKA_BOOTSTRAP_SERVERS: ${KAFKA_BOOTSTRAP_SERVERS:-119.194.235.11:9102} |
There was a problem hiding this comment.
The docker-compose file contains hardcoded sensitive information in plain text including database password 'devnogi0529!' on line 21 and Kafka bootstrap server IP address on line 22. Even for local development, credentials should not be hardcoded. These values should be moved to environment variables or a separate .env file that's not committed to version control.
| DB_PASSWORD: ${DB_PASSWORD:-devnogi0529!} | |
| KAFKA_BOOTSTRAP_SERVERS: ${KAFKA_BOOTSTRAP_SERVERS:-119.194.235.11:9102} | |
| DB_PASSWORD: ${DB_PASSWORD} | |
| KAFKA_BOOTSTRAP_SERVERS: ${KAFKA_BOOTSTRAP_SERVERS} |
|
|
||
| @Column(name = "is_verified", nullable = false) | ||
| @Comment("사용자 인증 상태") | ||
| private boolean verified; |
There was a problem hiding this comment.
The new verified field does not have a default value defined in the entity. Since the database migration uses DEFAULT FALSE, the entity should match this with @Builder.Default annotation to ensure consistency when creating User objects via builder pattern. Without this, new User instances created with the builder will have null for primitive boolean, which may cause issues.
| private boolean verified; | |
| @Builder.Default | |
| private boolean verified = false; |
| user_id BIGINT NULL COMMENT '회원 ID', | ||
| server_name VARCHAR(20) NULL COMMENT '서버명', | ||
| character_name VARCHAR(100) NULL COMMENT '캐릭터명', | ||
| verified_at DATETIME NOT NULL COMMENT '검증 처리 시각', | ||
| verification_success BOOLEAN NOT NULL COMMENT '인증 성공 여부', | ||
| failure_reason VARCHAR(100) NULL COMMENT '실패 사유', | ||
| token_id BIGINT NULL COMMENT '토큰 ID', | ||
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP COMMENT '생성 시각', | ||
| CONSTRAINT fk_user_verification_history_user_id FOREIGN KEY (user_id) REFERENCES users(id), |
There was a problem hiding this comment.
The foreign key constraint on user_id in user_verification_history allows NULL values (line 38), but the constraint on line 46 references users(id). In MySQL, a foreign key constraint on a nullable column will allow NULL values, but it's unusual to have verification history without knowing which user it belongs to. Consider whether user_id should be NOT NULL, or document the use case where user_id can be NULL (e.g., when token lookup fails).
📋 상세 설명
📊 체크리스트