이메일 인증 마이그레이션 / 비밀번호 찾기 api 변경 #248
Conversation
asp345
requested review from
davin111 and
Hank-Choi
and removed request for
a team
asp345
force-pushed
the
feature/password-reset
branch
3 times, most recently
from
a499c1b to
b1a1012
Compare
| class MailClient( | ||
| private val authService: AuthService | ||
| ) { |
There was a problem hiding this comment.
제가 느끼기엔 MailClient라고 하면 최하위 component 같은데요
authService를 의존성으로 갖고 있는게 어글리한 것 같아요
아예 util object를 만들어서 해당 util을 AuthService, MailClient가 의존하는 편이 더 깔끔할듯요
| if (!sesClient.sendEmail(request).join().sdkHttpResponse().isSuccessful) { | ||
| throw SendEmailFailedException | ||
| } |
There was a problem hiding this comment.
sesClient.sendEmail은 CompletableFuture를 반환하는데
여기서 join을 걸어버리면 blocking call이 일어나요
webflux쓰는 프로젝트라 blocking call은 절대 일어나면 안돼서
Mono.fromFuture(sesClient.sendEmail(request)).awaitSingle()
import kotlinx.coroutines.future.await
sesClient.sendEmail(request).await()
두가지 방식으로 처리하는게 좋아보입니다.
coroutine쓰는김에 kotlinx-coroutines-jdk8 라이브러리 갖고와서 후자로 처리하는게 좋아보입니다.
|
|
||
| override suspend fun verifyEmail(user: User, code: String) { | ||
| val key = "verification-code-${user.id}" | ||
| val value = mapper.treeToValue(mapper.reader().readTree(redisTemplate.opsForValue().getAndAwait(key)), RedisVerificationValue::class.java) |
There was a problem hiding this comment.
| val value = mapper.treeToValue(mapper.reader().readTree(redisTemplate.opsForValue().getAndAwait(key)), RedisVerificationValue::class.java) | |
| val value = redisTemplate.opsForValue().getAndAwait(key)?.let { mapper.readValue<RedisVerificationValue>(it) } |
| "<b>아래의 인증번호 6자리를 진행 중인 화면에 입력하여 3분내에 인증을 완료해주세요.</b><br/><br/>" + | ||
| "<h3>인증번호</h3><h3>$code</h3><br/><br/>" + | ||
| "인증번호는 이메일 발송 시점으로부터 3분 동안 유효합니다." | ||
| redisTemplate.opsForValue().set(key, mapper.writeValueAsString(value), Duration.ofMinutes(3)).subscribe() |
There was a problem hiding this comment.
이런거 value만 따로 빼서 두줄로 만들면 가독성 더 높아질 것 같아요
| user.credential.localId = localCredential.localId | ||
| user.credential.localPw = localCredential.localPw | ||
| user.credentialHash = authService.generateCredentialHash(user.credential) |
There was a problem hiding this comment.
kotlin apply 사용해서 데이터 수정이 어디서 일어나는지 한눈에 알아볼 수 있으면 좋을듯요
| user.credential.localId = localCredential.localId | |
| user.credential.localPw = localCredential.localPw | |
| user.credentialHash = authService.generateCredentialHash(user.credential) | |
| user.apply { | |
| credential.localId = localCredential.localId | |
| credential.localPw = localCredential.localPw | |
| credentialHash = authService.generateCredentialHash(user.credential) | |
| } |
| private suspend fun getVerificationValue(email: String, code: String, key: String): RedisVerificationValue { | ||
| val existing = when (val existingRedisString = redisTemplate.opsForValue().getAndAwait(key)) { | ||
| null -> null | ||
| else -> mapper.readValue(existingRedisString, RedisVerificationValue::class.java) |
There was a problem hiding this comment.
코틀린 확장함수 쓰면 더 깔끔해 질 것 같아요
| else -> mapper.readValue(existingRedisString, RedisVerificationValue::class.java) | |
| else -> mapper.readValue<RedisVerificationValue>(existingRedisString) |
| override suspend fun getMaskedEmail(localId: String): String { | ||
| val user = userRepository.findByCredentialLocalIdAndActiveTrue(localId) ?: throw UserNotFoundException | ||
| val email = user.email ?: throw UserNotFoundException | ||
| val maskedEmail = email.replace(Regex("(?<=.{3}).(?=.*@)"), "*") |
There was a problem hiding this comment.
Regex 매번 생성하기보다
static 메소드로 들고 있어도 괜찮을 듯 합니다.
| if (user.isEmailVerified == true) throw EmailAlreadyVerifiedException | ||
| if (!authService.isValidEmail(email)) throw InvalidEmailException | ||
| if (userRepository.existsByEmailAndIsEmailVerifiedTrueAndActiveTrue(email)) throw DuplicateEmailException | ||
| val key = "verification-code-${user.id}" |
There was a problem hiding this comment.
key를 공통적으로 쓰는 부분이 많은데
각 함수에서 따로 만드는 부분에서 실수가 나올 가능성이 있어보입니다.
private 함수로 빼면 좋을 것 같아요
또 prefix 같은걸 static 변수로 둬도 좋을 것 같고요
| val emailSubject = "[SNUTT] 인증번호 [$code] 를 입력해주세요" | ||
| val emailBody = "<h2>인증번호 안내</h2><br/>" + | ||
| "안녕하세요. SNUTT입니다. <br/> " + | ||
| "<b>아래의 인증번호 6자리를 진행 중인 화면에 입력하여 3분내에 인증을 완료해주세요.</b><br/><br/>" + | ||
| "<h3>인증번호</h3><h3>$code</h3><br/><br/>" + | ||
| "인증번호는 이메일 발송 시점으로부터 3분 동안 유효합니다." |
There was a problem hiding this comment.
이런 이메일 템플릿이 코드 안에 들어가는게 조금 어글리하네요
템플릿 파일을 resource 폴더에 두고 런타임에 읽어오는 건 어떨까요?
제안이고 이건 굳이 반영안해도 됩니다.
| val emailSubject = "[SNUTT] 아이디를 찾았습니다" | ||
| val emailBody = "<h2>아이디 찾기 안내</h2><br/>" + | ||
| "안녕하세요. SNUTT입니다. <br/> " + | ||
| "<b>${user.email}로 가입된 아이디는 다음과 같습니다.</b><br/><br/>" + | ||
| "<h3>아이디</h3><h3>${user.credential.localId}</h3><br/><br/>" | ||
| mailClient.sendMail(email, emailSubject, emailBody) | ||
| } | ||
|
|
||
| override suspend fun sendResetPasswordCode(email: String) { | ||
| if (!authService.isValidEmail(email)) throw InvalidEmailException | ||
| val user = userRepository.findByEmailAndActiveTrue(email) ?: throw UserNotFoundException | ||
| val key = "reset-password-code-${user.id}" | ||
| val code = Base64.getUrlEncoder().encodeToString(Random.nextBytes(6)) | ||
| val value = getVerificationValue(email, code, key) | ||
| val emailSubject = "[SNUTT] 인증코드 [$code] 를 입력해주세요" | ||
| val emailBody = "<h2>비밀번호 재설정 안내</h2><br/>" + | ||
| "안녕하세요. SNUTT입니다. <br/> " + | ||
| "<b>아래의 인증코드를 진행 중인 화면에 입력하여 비밀번호 재설정을 완료해주세요.</b><br/><br/>" + | ||
| "<h3>인증코드</h3><h3>$code</h3><br/><br/>" + | ||
| "인증코드는 이메일 발송 시점으로부터 3분 동안 유효합니다." |
| user.email = value.email | ||
| user.isEmailVerified = true |
asp345
force-pushed
the
feature/password-reset
branch
from
390544f to
10ae68e
Compare
|
리뷰해주신거 수정했는데 mail 쪽은 client만 common으로 빼고 util object에서 비동기 함수 실행이 안되는거 같아 MailService 만들어서 사용하는걸로 바꾸었습니다. |
|
@asp345 요거 email 테스트 이제 해본 건가요 |
|
이메일 테스트 완료했어요 |
|
@asp345 넹 참고로 가끔씩 머지한 브랜치 삭제해주면 좋습니다! 글구 클라까지 연동한 테스트를 위해서면 dev 는 원하는대로 작업된 건 라우팅 바꿔놔도 될 듯! |
아래 api들을 snu4t로 옮기고, 비밀번호 찾기에서 문제가 될 수 있는 부분 찾아서 수정했습니다 (https://wafflestudio.slack.com/archives/C0PAVPS5T/p1719648852739009)
변경사항 안 적은 부분들은 기존 api 스펙 유지했고 하다 보니까 양이 좀 많아졌는데 리뷰 부탁드려요
POST /auth/password/reset/email/check - 마스킹된 이메일 반환하도록 변경
POST /auth/password/reset/email/send
POST /auth/password/reset/verification/code
POST /auth/password/reset - 보안을 위해 verification에 사용한 코드를 client에서 다시 한번 보내도록 변경
POST /auth/id/find
DELETE /user/email/verification
POST /user/email/verification
POST /user/email/verification/code
GET /user/email/verification
POST /user/password
PUT /user/password