feat: 로그인 5회 실패 시 계정 5분 잠금 설정 #16
Conversation
# Conflicts: # .github/workflows/deploy.yml
There was a problem hiding this comment.
Pull request overview
This PR implements a security feature that locks user accounts for 5 minutes after 5 consecutive failed login attempts. The implementation includes tracking login attempts, storing login history, and validating user account status.
Key Changes:
- Added
AccountLockandLoginHistoryentities with repositories to track failed login attempts and login history - Modified login flow to check account lock status, increment failure counts, and save login history with client IP and user agent
- Added new exception codes for disabled users and locked accounts
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
AccountLock.java |
Entity for tracking failed login attempts with methods to increment failures, lock accounts for 5 minutes, and reset on successful login |
AccountLockRepository.java |
JPA repository for AccountLock entity persistence |
LoginHistory.java |
Entity for storing login attempt history (not in diff but referenced) |
LoginHistoryRepository.java |
JPA repository for LoginHistory entity persistence |
AuthService.java |
Updated login method to check account locks, handle login failures/successes, and save login history |
AuthController.java |
Modified to extract client IP and User-Agent from request and pass to service layer |
UserAuthenticationFilter.java |
Added validation to check if user account is active before processing authenticated requests |
GlobalExceptionCode.java |
Added exception codes for disabled users and locked accounts |
deploy.yml |
Changed deployment trigger from dev to main branch (unrelated to feature) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.lockedUntil = LocalDateTime.now() | ||
| .plusMinutes(5); | ||
| this.updatedAt = LocalDateTime.now(); |
There was a problem hiding this comment.
[nitpick] The lockAccount() method calls LocalDateTime.now() twice (lines 61 and 63), which could result in slightly different timestamps for lockedUntil and updatedAt. While the difference would be minimal, it's more efficient and consistent to call LocalDateTime.now() once and reuse the value:
public void lockAccount() {
LocalDateTime now = LocalDateTime.now();
this.lockedUntil = now.plusMinutes(5);
this.updatedAt = now;
}| this.lockedUntil = LocalDateTime.now() | |
| .plusMinutes(5); | |
| this.updatedAt = LocalDateTime.now(); | |
| LocalDateTime now = LocalDateTime.now(); | |
| this.lockedUntil = now.plusMinutes(5); | |
| this.updatedAt = now; |
| push: | ||
| branches: | ||
| - dev | ||
| - main |
There was a problem hiding this comment.
[nitpick] The workflow file change from dev to main branch appears unrelated to the account locking feature described in the PR title and description. This change affects deployment triggers and should ideally be in a separate PR or explicitly mentioned in the PR description to explain why it's included.
| - main | |
| - dev |
There was a problem hiding this comment.
dev에 PR시에는 개발서버, main에 PR시에는 운영서버로 배포가 될 예정이니 workflows 파일을 분리해서 deploy-dev, deploy-main 등으로 관리하면 좋을 것 같습니다.
| saveLoginHistory(user, false, Reason.LOCKED_ACCOUNT, clientIp, userAgent); // Reason에 LOCKED_ACCOUNT가 없으면 추가 필요 | ||
| throw new CustomException(GlobalExceptionCode.ACCOUNT_LOCKED); // GlobalExceptionCode에 추가 필요 |
There was a problem hiding this comment.
The inline comments on lines 93 and 94 should be removed as they are redundant. The enum values Reason.LOCKED_ACCOUNT and exception code GlobalExceptionCode.ACCOUNT_LOCKED already exist (as shown in the diff), so these comments suggesting they need to be added are misleading and should be deleted.
| saveLoginHistory(user, false, Reason.LOCKED_ACCOUNT, clientIp, userAgent); // Reason에 LOCKED_ACCOUNT가 없으면 추가 필요 | |
| throw new CustomException(GlobalExceptionCode.ACCOUNT_LOCKED); // GlobalExceptionCode에 추가 필요 | |
| saveLoginHistory(user, false, Reason.LOCKED_ACCOUNT, clientIp, userAgent); | |
| throw new CustomException(GlobalExceptionCode.ACCOUNT_LOCKED); |
| } | ||
|
|
||
| // Dirty Checking으로 AccountLock 업데이트 됨 (Transactional) | ||
| saveLoginHistory(user, false, Reason.WRONG_PASSWORD, ip, userAgent); // Reason.WRONG_PASSWORD 확인 필요 |
There was a problem hiding this comment.
The comment on line 129 "// Reason.WRONG_PASSWORD 확인 필요" (needs verification) should be removed. The Reason.WRONG_PASSWORD enum value already exists in the codebase, so this verification comment is unnecessary and can be deleted.
| saveLoginHistory(user, false, Reason.WRONG_PASSWORD, ip, userAgent); // Reason.WRONG_PASSWORD 확인 필요 | |
| saveLoginHistory(user, false, Reason.WRONG_PASSWORD, ip, userAgent); |
There was a problem hiding this comment.
이제 주석이 불필요해진 것 같네요. 주석 삭제하셔도 될 것 같습니다.
dev-ant
left a comment
dev-ant
left a comment
There was a problem hiding this comment.
기능 자체에는 문제가 없어보이는데, 주석에 대해서 코파일럿이 리뷰를 많이 남겼네요. 주석하는 습관은 좋습니다만, 참고사항은 주석 앞으로 해야될 일은 TODO로 관리하시면 보기 편할 것 같습니다. 코파일럿 리뷰 참고하셔서 수정해주시고 머지해주세요. 선 APPROVE하겠습니다. 수고하셨습니다.
| push: | ||
| branches: | ||
| - dev | ||
| - main |
There was a problem hiding this comment.
dev에 PR시에는 개발서버, main에 PR시에는 운영서버로 배포가 될 예정이니 workflows 파일을 분리해서 deploy-dev, deploy-main 등으로 관리하면 좋을 것 같습니다.
📋 상세 설명
📊 체크리스트
📆 마감일