Conversation
|
환경 변수 관련 때문에 빌드 테스트 check 가 fail 나는 것 같은데 ci.yml 을 수정하는 게 좋을까요 아니면 test 디렉터리 밑에 application.yml 에 환경변수 임의로 넣어놓는 것이 좋을까요? 아니면 저번 PR 에 지민님이 말했던 것처럼 env 파일로 관리해도 좋을 듯 합니다! |
There was a problem hiding this comment.
P4: 코드 확인했습니다! 로직이 깔끔하게 잘 구현되어 있는 거 같아 따로 건들 부분은 없는 것 같아요! 말씀해주신 로그인 로직에 따로 이메일 인증 확인을 거치는 건 우선 구현 하지 않아도 괜찮을 것 같습니다.
P3: 다만 말씀해주신 것처럼 이메일 쪽에서 sender의 값을 민감 정보로 처리해야하는 상황인데, gitignore 안에서 임의로 넣어두었던 yml은 빼고 application.yml을 공개로 돌려 업로드하는 게 괜찮을 듯 합니다. 민감 정보는 env에서 읽도록, 프로젝트 루트에 env를 두고 gitignore에도 env를 추가하는 식이 될 것 같습니다. 추가로 형식만 알 수 있는 .env.example도 두고요! 자세한 형식은 팀 노션 > .env 환경 변수 (BE) 페이지 안에 추가해두겠습니다
이렇게 되면 yml 버전 관리도 편리하고 개발 시 ci에 걸리는 일이 생지 않을 것 같아요!
Walkthrough이메일 인증 기능을 추가하여 회원가입 전 이메일 검증을 요구합니다. 인증 코드는 Redis에 5분간 저장되며, 검증 완료 후 VERIFIED 플래그를 1시간 저장합니다. 회원가입 시 이 플래그를 확인하여 미검증 이메일을 차단합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserController
participant EmailService
participant UserRepository
participant MailServer
participant Redis
rect rgba(100, 200, 150, 0.5)
Note over Client,Redis: 이메일 인증 요청 흐름
Client->>UserController: POST /api/users/email-send<br/>(EmailRequest.Send)
UserController->>EmailService: sendEmail(toEmail)
EmailService->>UserRepository: existsByEmail(toEmail)
alt 이메일 중복
EmailService-->>Client: USER_EMAIL_DUPLICATE (400)
else 신규 이메일
alt test 이메일
EmailService->>Redis: setDataExpire(CODE:email, "123456", 300s)
else 실제 이메일
EmailService->>MailServer: send(toEmail, authCode)
alt 메일 발송 실패
EmailService-->>Client: USER_EMAIL_NOT_VALID (400)
else 성공
EmailService->>Redis: setDataExpire(CODE:email, "123456", 300s)
EmailService-->>Client: EmailSentResponse (200)
end
end
end
end
rect rgba(150, 150, 200, 0.5)
Note over Client,Redis: 인증 코드 검증 흐름
Client->>UserController: POST /api/users/email-verify<br/>(EmailRequest.Verify)
UserController->>EmailService: verifyEmailCode(email, inputCode)
EmailService->>Redis: getData(CODE:email)
alt 코드 불일치 또는 만료
EmailService-->>Client: USER_EMAIL_AUTH_INVALID (400)
else 코드 일치
EmailService->>Redis: deleteData(CODE:email)
EmailService->>Redis: setDataExpire(VERIFIED:email, "TRUE", 3600s)
EmailService-->>Client: success (200)
end
end
rect rgba(200, 150, 100, 0.5)
Note over Client,Redis: 회원가입 흐름 (이메일 검증 필수)
Client->>UserController: POST /api/users/signup<br/>(SignUpRequest)
UserController->>UserService: signUpUser(signUpRequest)
UserService->>Redis: getData(VERIFIED:email)
alt 이메일 미검증
UserService-->>Client: USER_EMAIL_NOT_VERIFIED (401)
else 검증됨
UserService->>UserService: createUser + set isEmailVerified=true
UserService-->>Client: signup success (200)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 👍 잘 짜진 부분1. 이메일 서비스 로직이 명확하게 분리됨 2. TTL 기반 만료 처리 3. 에러 코드 체계화
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 53.33% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Title check | ❓ Inconclusive | Title 'Feat/#10' is vague and generic, only showing issue number without describing the actual feature implementation (email verification APIs). | Use a more descriptive title like 'Add email verification APIs with Redis-backed authentication codes' to clearly communicate the core change. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description check | ✅ Passed | PR description follows the template with clear overview, detailed work items, comprehensive test evidence with screenshots, and honest review points raising thoughtful questions. |
| Linked Issues check | ✅ Passed | All three coding objectives from issue #10 are fully implemented: email sending logic with Spring Mail [build.gradle, EmailService], Redis-based code storage with TTL [RedisUtil, EmailService], and signup integration with email verification checks [UserService]. |
| Out of Scope Changes check | ✅ Passed | SecurityConfig change expanding unauthenticated access from signup-only to all /api/users/** endpoints is necessary to support new email verification endpoints and is directly related to issue #10 requirements. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
Comment @coderabbitai help to get the list of available commands and usage tips.
그러면 gitignore 에서 src/main/resources/application.yml 이 있는걸 지워서 해당 yml 파일을 올리되 .env 에서 값을 끌어오도록 하는 방식으로 하는게 맞을까요? |
|
네 맞습니다! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/whereyouad/WhereYouAd/global/security/SecurityConfig.java`:
- Around line 35-38: The current authorizeHttpRequests chain permits the broad
"/api/users/**" wildcard which exposes all future user endpoints; update the
SecurityConfig authorizeHttpRequests(...) call to remove the "/api/users/**"
wildcard and instead list only the explicit public endpoints (e.g., exact paths
like "/api/users/register", "/api/users/verify-email", etc.) in the
requestMatchers(...).permitAll() calls (keep "/api/auth/**" and swagger entries
as needed) so anyRequest().authenticated() continues to protect all other user
routes; adjust the requestMatchers invocations in the same authorizeHttpRequests
chain to use these explicit paths or more restrictive patterns.
🧹 Nitpick comments (9)
.env.example (1)
1-15: 환경 변수 템플릿이 잘 구성되어 있어요! 👏DB, Redis, Mail, JWT에 필요한 환경 변수들이 명확하게 정리되어 있어서 새로운 팀원이 프로젝트 셋업할 때 큰 도움이 될 것 같습니다.
한 가지 사소한 점만 말씀드리면, 파일 끝에 빈 줄(trailing newline)을 추가하시면 POSIX 표준을 따르게 되어 일부 도구에서 경고가 사라집니다.
💡 파일 끝 빈 줄 추가 (선택 사항)
# JWT JWT_SECRET=your_very_long_and_secret_random_string_for_signing_tokens_to_match_HS512 +src/main/resources/application.yml (1)
16-22: 프로덕션 환경 고려가 필요해요 🔧현재 설정이 개발 환경에서는 편리하지만, 프로덕션에서는 주의가 필요한 부분이 있습니다:
ddl-auto: update(Line 18): 프로덕션에서 예기치 않은 스키마 변경이 발생할 수 있어요. 예를 들어, 엔티티에서 필드를 삭제해도 DB 컬럼은 삭제되지 않지만, 컬럼 타입 변경 시 데이터 손실이 발생할 수 있습니다.
show_sql: true+format_sql: true(Line 21-22): 모든 SQL이 로그에 출력되어 프로덕션에서 로그 용량이 급격히 증가하고 성능에 영향을 줄 수 있어요.나중에 환경별 profile (
application-dev.yml,application-prod.yml)을 분리하시면 좋을 것 같아요. 지금 당장은 아니어도 괜찮지만 참고해 주세요!💡 프로덕션용 권장 설정 예시
# application-prod.yml 예시 spring: jpa: hibernate: ddl-auto: validate # 또는 none properties: hibernate: format_sql: false show_sql: falsesrc/main/java/com/whereyouad/WhereYouAd/global/utils/RedisUtil.java (1)
10-12:@Service보다@Component가 더 적절할 수 있어요.
RedisUtil은 비즈니스 로직을 담당하는 서비스 레이어가 아니라 인프라 유틸리티에 가까워요. 스테레오타입 어노테이션의 의미를 명확히 하려면@Component를 사용하는 것이 더 적절합니다.기능상 차이는 없지만, 코드를 읽는 사람이 역할을 더 명확히 이해할 수 있어요.
♻️ 권장 변경
-@Service +@Component `@RequiredArgsConstructor` public class RedisUtil {src/main/java/com/whereyouad/WhereYouAd/domains/user/domain/service/UserService.java (2)
53-58: 회원가입 완료 후 Redis 인증 키 삭제를 권장해요.현재
VERIFIED:<email>키가 TTL 만료까지 Redis에 남아있어요. 회원가입이 완료되면 해당 키를 명시적으로 삭제하는 것이 좋습니다:
- 메모리 정리: 더 이상 필요 없는 데이터 제거
- 보안 강화: 같은 인증으로 다른 악의적 시도 방지
♻️ Redis 키 삭제 추가
User savedUser = userRepository.save(user); + //회원가입 완료 후 인증 키 삭제 (메모리 정리) + redisUtil.deleteData("VERIFIED:" + request.email()); + //Response DTO 로 변환 및 반환 return UserConverter.toSignInResponse(savedUser);
34-34: Redis 키 프리픽스를 상수로 추출하면 좋아요.
"VERIFIED:"같은 매직 스트링이EmailService와UserService에서 함께 사용될 텐데요, 오타 방지와 유지보수를 위해 상수로 추출하는 것을 권장해요.예를 들어,
EmailService나 별도의 상수 클래스에:public static final String VERIFIED_KEY_PREFIX = "VERIFIED:";src/main/java/com/whereyouad/WhereYouAd/domains/user/application/dto/request/EmailVerifyRequest.java (1)
6-12: Java 17 Record를 잘 활용하셨네요! 👍DTO로서 불변성과 간결함을 잘 갖추고 있습니다.
@NotBlank,한 가지 사소한 부분인데,
authCode에는 없어서 일관성 측면에서 개선하면 좋을 것 같습니다.✨ 선택적 개선안: authCode에도 커스텀 메시지 추가
public record EmailVerifyRequest( `@NotBlank`(message = "이메일은 필수입니다.") `@Email`(message = "이메일 형식이 올바르지 않습니다.") String email, - `@NotBlank` + `@NotBlank`(message = "인증코드는 필수입니다.") String authCode ) { }src/main/java/com/whereyouad/WhereYouAd/domains/user/domain/service/EmailService.java (3)
17-17:@Transactional사용에 대해 검토가 필요합니다.이 서비스는 JPA를 통한 DB 쓰기 작업이 없고,
userRepository.existsByEmail()은 읽기 전용 쿼리입니다. Redis 작업은 Spring의@Transactional과 별도로 동작하므로, 클래스 레벨의@Transactional이 실질적인 효과가 없습니다.현재는 해를 끼치지 않지만, 불필요한 트랜잭션 컨텍스트 생성을 피하려면 제거하거나, 읽기 전용임을 명시하는 것이 좋습니다.
♻️ 개선안
-@Transactional `@Service` `@RequiredArgsConstructor` `@Slf4j` public class EmailService {또는 읽기 전용으로 명시:
-@Transactional +@Transactional(readOnly = true) `@Service`
103-106: 보안 강화를 위해SecureRandom사용을 권장합니다.
Math.random()은 내부적으로java.util.Random을 사용하는데, 이는 예측 가능한 난수를 생성합니다. 인증 코드는 보안에 민감한 값이므로SecureRandom을 사용하는 것이 더 안전합니다.예를 들어, 악의적인 사용자가 난수 패턴을 분석해서 다음 인증 코드를 예측할 가능성이 (매우 낮지만) 존재합니다.
🔒 SecureRandom 적용 예시
+import java.security.SecureRandom; + public class EmailService { + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); //무작위 인증코드 값 생성 private String createCode() { - return String.valueOf((int)(Math.random() * (900000)) + 100000); + return String.valueOf(SECURE_RANDOM.nextInt(900000) + 100000); } }
75-75: 매직 넘버를 상수로 추출하면 가독성이 좋아집니다.
60 * 5L이나60 * 60L같은 값들을 상수로 정의하면 의미가 더 명확해지고, 나중에 값을 변경할 때도 한 곳만 수정하면 됩니다.✨ 상수 추출 예시
public class EmailService { + private static final long AUTH_CODE_EXPIRE_SECONDS = 60 * 5L; // 5분 + private static final long VERIFIED_EXPIRE_SECONDS = 60 * 60L; // 1시간 // sendEmail 메서드 내 - redisUtil.setDataExpire("CODE:" + toEmail, authCode, 60 * 5L); + redisUtil.setDataExpire("CODE:" + toEmail, authCode, AUTH_CODE_EXPIRE_SECONDS); // verifyEmailCode 메서드 내 - redisUtil.setDataExpire("VERIFIED:" + email, "TRUE", 60 * 60L); + redisUtil.setDataExpire("VERIFIED:" + email, "TRUE", VERIFIED_EXPIRE_SECONDS);Also applies to: 95-95
src/main/java/com/whereyouad/WhereYouAd/global/security/SecurityConfig.java
Show resolved
Hide resolved
|
ci.yml 에서 선언한 환경변수명이랑 저희 application.yml 환경변수명이랑 달라서 나는 ci 빌드 오류 같은데 제가 ci.yml 수정해서 커밋 올려보겠습니다! |
ci.yml 환경변수명 수정하였고 jwt secret 값에서도 빌드 시 인코딩 값을 요구해서, 임의로 값 넣어서 처리해뒀습니다! (ci는 단순 빌드 테스트용이라 괜찮다고 하네요!) |
아 넵!! 보니까 제가 JWT 에서 사용한 알고리즘이 HS512 인데 이걸 쓰면 jwt secret 값이 64 글자 이상이여야 정상동작 한다고 하더라구요! 아마 이전 "test_secret_key_value_must_be_long_enough_for_security_tests" 이 문자열이 64글자 미만이라 오류가 났을수도 있을거 같은데 일단 지금 정상 동작되니 이대로 해도 좋을 것 같아요! |
|
마지막으로 민규님 리뷰 하시고 나면 머지 하겠습니다! |
kingmingyu
left a comment
kingmingyu
left a comment
There was a problem hiding this comment.
P4: 준영님 고생하셨습니다! 오늘까지 개인 사정으로 작업 못하신다고 하셔서 다른 거 하다가 리뷰를 좀 늦게 한 것 같습니다! 커밋 단위가 세세해서 보기 편했던 것 같습니다!
src/main/java/com/whereyouad/WhereYouAd/domains/user/domain/service/EmailService.java
Show resolved
Hide resolved
src/main/java/com/whereyouad/WhereYouAd/domains/user/application/dto/request/EmailRequest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/whereyouad/WhereYouAd/domains/user/exception/code/UserErrorCode.java
Outdated
Show resolved
Hide resolved
…mailRequest.class 로 통일하여 내부 계층구조로 리팩터링
|
마지막으로 민규님 피드백 반영해서 이메일 인증 Request DTO 를 하나의 클래스로 통일하여 내부 계층구조로 리팩터링 하고, UserErrorCode 에 USER_400_N 코드 숫자 정렬 하고 마무리했습니다. CI 빌드 완료되면 머지 하겠습니다. |
3 participants
📌 관련 이슈
🚀 개요
이메일 인증 관련 API 추가, 기존 회원가입 이메일 인증 가능한 상태에만 가입되도록 제한
📄 작업 내용
더미 이메일 값(ex. test@example.com) 을 넣어도 이메일 인증 할 수 있도록 로깅 로직을 추가했습니다.
📸 스크린샷 / 테스트 결과 (선택)
===더미 이메일 값 인증코드 발송 테스트===
===실제 이메일 발송 테스트===
===예외 처리===
✅ 체크리스트
🔍 리뷰 포인트 (Review Points)
테스트 등에 여러 회원을 만들어야 할 상황이 있을 것 같아 이렇게 설계해봤습니다.
Summary by CodeRabbit
릴리스 노트
새로운 기능
Chores
✏️ Tip: You can customize this high-level summary in your review settings.