Feature/api key issuance#4
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive API key management system, including the core entity, service logic, and a REST controller for CRUD operations. It also implements a custom security filter to authenticate requests via the X-API-KEY header and integrates Flyway for database schema migrations. The code review feedback highlights several areas for optimization and better design: specifically, improving performance by handling usage timestamp updates asynchronously, optimizing database queries by using count methods instead of loading full entity lists, and enhancing encapsulation by avoiding broad Lombok annotations. Additionally, the reviewer suggested more robust exception handling in the security filter and defensive checks for string operations to prevent potential runtime errors.
| ApiKey apiKey = apiKeyRepository.findByHashedKeyAndIsActiveTrue(hashedKey) | ||
| .orElseThrow(() -> new IllegalArgumentException("Invalid or inactive API Key")); | ||
|
|
||
| apiKey.setLastUsedAt(LocalDateTime.now()); |
There was a problem hiding this comment.
매 API 요청마다 lastUsedAt 필드를 업데이트하는 것은 데이터베이스에 상당한 쓰기 부하를 줍니다. 특히 트래픽이 많은 환경에서는 인증 과정이 성능 병목 지점이 될 수 있습니다. 이 필드 업데이트를 비동기(@Async)로 처리하거나, 일정 시간 간격으로 배치 업데이트하는 방식을 고려해 보시기 바랍니다.
| public ApiKeyCreateResponse createApiKey(Authentication authentication, ApiKeyCreateRequest request) { | ||
| Member member = getMember(authentication); | ||
|
|
||
| long currentKeysCount = apiKeyRepository.findByMemberId(member.getId()).size(); |
There was a problem hiding this comment.
API 키 개수를 확인할 때 전체 엔티티 리스트를 조회하여 메모리에서 크기를 체크하는 것은 비효율적입니다. countByMemberId 쿼리 메서드를 사용하여 데이터베이스 레벨에서 개수만 조회하도록 개선해 주세요.
| long currentKeysCount = apiKeyRepository.findByMemberId(member.getId()).size(); | |
| long currentKeysCount = apiKeyRepository.countByMemberId(member.getId()); |
| public interface ApiKeyRepository extends JpaRepository<ApiKey, Long> { | ||
| Optional<ApiKey> findByHashedKey(String hashedKey); | ||
| Optional<ApiKey> findByHashedKeyAndIsActiveTrue(String hashedKey); | ||
| List<ApiKey> findByMemberId(Long memberId); |
There was a problem hiding this comment.
API 키 개수 제한 확인을 위해 효율적인 count 쿼리 메서드를 추가하는 것이 좋습니다.
List<ApiKey> findByMemberId(Long memberId);
long countByMemberId(Long memberId);| @Entity | ||
| @Table(name = "api_keys") | ||
| @Getter | ||
| @Setter |
There was a problem hiding this comment.
클래스 레벨의 @Setter는 객체의 무분별한 상태 변경을 허용하여 캡슐화를 저해하고 유지보수성을 떨어뜨릴 수 있습니다. 필요한 필드에만 @Setter를 붙이거나, 의미 있는 비즈니스 메서드(예: deactivate())를 통해 상태를 변경하는 것을 권장합니다.
| } catch (Exception e) { | ||
| // Invalid API key - we can either throw error or just let it pass to be caught by security chain | ||
| // Here we just don't set the authentication | ||
| } |
There was a problem hiding this comment.
모든 예외(Exception)를 포괄적으로 잡아 아무런 처리도 하지 않는 것은 예상치 못한 시스템 오류를 숨길 수 있어 위험합니다. 인증 실패(예: IllegalArgumentException)와 시스템 오류를 구분하여 처리하고, 최소한 디버깅을 위한 로그를 남기는 것이 좋습니다.
} catch (IllegalArgumentException e) {
logger.debug("Invalid API key provided: " + e.getMessage());
} catch (Exception e) {
logger.error("Unexpected error during API key authentication", e);
}|
|
||
| String rawKey = "tk-" + UUID.randomUUID().toString().replace("-", ""); | ||
| String hashedKey = HashingUtil.hash(rawKey); | ||
| String displayKey = rawKey.substring(0, 7) + "..." + rawKey.substring(rawKey.length() - 4); |
There was a problem hiding this comment.
현재 키 생성 로직상 안전하지만, substring 인덱스 접근 시 문자열 길이를 먼저 체크하는 것이 방어적 프로그래밍 관점에서 더 안전합니다.
| String displayKey = rawKey.substring(0, 7) + "..." + rawKey.substring(rawKey.length() - 4); | |
| String displayKey = rawKey.length() > 11 ? rawKey.substring(0, 7) + "..." + rawKey.substring(rawKey.length() - 4) : rawKey; |
1 participant
🚀 개요
외부 서비스 연동 및 API 보안을 위한 API Key 발급 기능과 헤더 기반(X-API-KEY)
인증 시스템을 구현
🛠 작업 내용
추가
컨트롤러 구현
대해 X-API-KEY 헤더를 검증하는 필터 구현 및 SecurityConfig 등록
스크립트 작성 (V2)
시점에만 평문 키 노출