Conversation
There was a problem hiding this comment.
- 이메일 인증 Rate Limit 구현은 아주 잘 하셨습니다~
- 지금은 이메일 발송할 때 성공 / 작업 진행중인 내역도
log.info()로 로그에 남게 하고 있는데, 실패한 것만 로그에 남기면 어떨까 합니다. 성공한 것도 로깅하면 너무 과도하게 로그가 찍힐 것 같습니다. - 클래스 네이밍에서
Port는 저희 프로젝트에서 사용하지 않는 네이밍입니다. 기존 outbound 포트 관례에 맞게 아래와 같이 수정해 주세요.기존 패턴 domain/notification/port/outbound/NotificationRepository.java ← 영속성 domain/notification/port/outbound/NotificationQueryRepository.java ← 쿼리 domain/auth/port/outbound/KakaoAuthClient.java ← 외부 API 변경 요청 EmailSendLogPort → EmailSendLogRepository EmailVerificationTokenStorePort → EmailVerificationTokenStoreRepository EmailSenderPort → EmailClient (KakaoAuthClient, AppleAuthClient 패턴 참고) - 추가된 env 값은 노션 문서에 최신화 해주시고, DB 마이그레이션 DDL은 작업 문서에 남겨주세요
...om/dreamteam/alter/adapter/outbound/email/redis/RedisEmailVerificationTokenStoreAdapter.java
Outdated
Show resolved
Hide resolved
...n/java/com/dreamteam/alter/adapter/inbound/general/user/controller/UserPublicController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dreamteam/alter/adapter/inbound/general/user/dto/CreateUserRequestDto.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dreamteam/alter/adapter/outbound/aws/ses/properties/AwsProperties.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dreamteam/alter/adapter/outbound/aws/ses/SesEmailSenderAdapter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dreamteam/alter/application/email/usecase/SendEmailVerificationCode.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| @PostMapping("/email/send") |
There was a problem hiding this comment.
/email/send 엔드포인트는 인증코드 발송 이라는 의미를 표현하기엔 애매한 것 같아서
/email/verification/send 로 바꾸면 어떨까 합니다
| } | ||
|
|
||
| @Override | ||
| @PostMapping("/email/verify") |
There was a problem hiding this comment.
위와 마찬가지로 /email/verification/send 로 바꾸면 어떨까 합니다
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class RedisEmailVerificationSessionStoreAdapter implements EmailVerificationSessionStoreRepository { |
There was a problem hiding this comment.
*Adapter 네이밍 대신에 인터페이스랑 맞춰서 EmailVerificationSessionStoreRepositoryImpl 로 변경해주세요
| if (sessionStoreRepository.isSendFailed(email)) { | ||
| sessionStoreRepository.clearSendFailed(email); | ||
| throw new CustomException(ErrorCode.EMAIL_SEND_FAILED); | ||
| } |
There was a problem hiding this comment.
인증코드는 발급했으나 이메일 발송은 실패한 경우를 체크하는 것 같은데, 이메일 발송 실패 한 케이스까지 캐싱해둘 필욘 없을 것 같습니다
만일 사용자가 메일을 못 받았다면 자연스럽게 인증코드를 다시 요청할 것이므로 해당 이메일과 인증코드가 매칭되는지만 검증하면 될 듯 합니다
| if (!storedCode.equals(inputCode)) { | ||
| // 시도 횟수 증가 | ||
| long attempts = sessionStoreRepository.incrementAttempt(email, Duration.ofSeconds(codeTtlSeconds)); | ||
|
|
||
| if (attempts >= maxAttempts) { | ||
| sessionStoreRepository.deleteCode(email); | ||
| throw new CustomException(ErrorCode.ILLEGAL_ARGUMENT, "인증 시도 횟수를 초과했습니다."); | ||
| } | ||
| throw new CustomException(ErrorCode.ILLEGAL_ARGUMENT, "인증 코드가 일치하지 않습니다."); | ||
| } |
There was a problem hiding this comment.
사용자 입장에서 한 인증코드로 여러 번 확인 요청을 할 일이 있을까 싶네요
한 번 요청해서 성공했으면 바로 세션을 제거하는 쪽으로 하면 검증할 필요 없을 것 같습니다
| @Override | ||
| public void execute(AppActor actor) { | ||
|
|
||
| User user = userQueryRepository.findById(actor.getUserId()) |
There was a problem hiding this comment.
actor.getUser()만 해도 사용자 객체를 불러올 수 있습니다
ActorActionContext에서 사용자 정보 확인 차 이미 User 객체를 조회해오므로 또 조회할 필요 없습니다~
| .orElseThrow(() -> new CustomException(ErrorCode.ILLEGAL_ARGUMENT, "이메일 인증 세션이 유효하지 않거나 만료되었습니다.")); | ||
|
|
||
| // 사용자 조회 | ||
| User user = userQueryRepository.findById(actor.getUserId()) |
There was a problem hiding this comment.
actor.getUser()만 해도 사용자 객체를 불러올 수 있습니다
ActorActionContext에서 사용자 정보 확인 차 이미 User 객체를 조회해오므로 또 조회할 필요 없습니다~
| private String email; | ||
|
|
||
| @Column(name = "code", nullable = false, length = 6) | ||
| private String code; |
There was a problem hiding this comment.
이메일 발송은 인증코드 뿐만 아니라 여러 곳에도 적용될 수 있으므로 인증코드 발송에만 한정지어 code 필드를 둘 필욘 없을 것 같습니다. 또한 유효기간이 짧은 인증 코드를 DB에 지속적으로 저장시킬 필요도 없습니다.
There was a problem hiding this comment.
이전에도 코멘트 했던 내용이긴 한데
앞으로 자체 ErrorCode 사용은 지양하기 위해 새로운 코드 생성은 자제해주세요
TOO_MANY_REQUESTS 코드만 유지하고 EMAIL_NOT_REGISTERED, EMAIL_SEND_FAILED 는 제거하고 기존에 있는 코드로 활용해주세요
CustomException 발생시킬 때 두 번째 인자로 에러 메시지를 직접 지정할 수 있습니다
| @Async | ||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | ||
| public void handleEmailSendEvent(EmailSendEvent event) { | ||
| Long logId = event.getLogId(); | ||
|
|
||
| emailSendLogRepository.findById(logId).ifPresent(logItem -> { | ||
| try { | ||
| emailClient.sendVerificationCode(logItem.getEmail(), logItem.getCode()); | ||
| logItem.markSent(); | ||
| } catch (Exception e) { | ||
| log.error("Async failed to send email to: {}", logItem.getEmail(), e); | ||
| logItem.markFailed(); | ||
| // 발송 실패 시 인증 코드 삭제 및 실패 상태 기록 | ||
| sessionStoreRepository.deleteCode(logItem.getEmail()); | ||
| sessionStoreRepository.markSendFailed(logItem.getEmail()); | ||
| } | ||
| emailSendLogRepository.save(logItem); | ||
| }); | ||
| } |
There was a problem hiding this comment.
이메일 인증 시 비동기로 처리하는 건 좋은데 별도 스레드 풀 설정이 없는 것 같아 추가하면 좋을듯하고
메일 전송 실패 시 로깅 하기전에 재시도 먼저 수행하는건 어떤가 싶습니다
지금은 사용자 없으니 스레드풀 설정은 안해도 재시도는 로직은 추가되면 좋을거같습니다
관련 문서
https://www.notion.so/BE-2f6865531628807ca2efc9313180e45c?source=copy_link