Repository navigation
이메일 인증 기능 추가 - #40
Merged
Merged
이메일 인증 기능 추가#40
Conversation
|
Warning Review limit reached
More reviews will be available in 32 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a complete email verification flow. It introduces request/response DTOs for email verification, a new ChangesEmail Verification Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Results106 tests 106 ✅ 2s ⏱️ Results for commit 7604238. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/chainsawman/gesture/service/AuthService.java`:
- Around line 232-237: The verification block in AuthService allows unlimited
OTP guesses; add attempt limiting by tracking and incrementing a per-code
attempt counter stored alongside the OTP (e.g., in the same Redis/kv store)
before validating. Define a MAX_ATTEMPTS constant (e.g., 5), increment the
counter key (e.g., verificationAttemptsKey for the same request identifier)
atomically, and if the count exceeds MAX_ATTEMPTS throw a new
TooManyAttemptsException and invalidate/delete the stored code; otherwise
proceed to compare stored.equals(request.getCode()) and reset/delete both the
code and attempts on successful verification or when expired. Ensure the new
counter respects the original TTL (set its expiry to the OTP TTL) and use the
AuthService method handling verification to locate and implement these changes.
- Around line 223-224: The code stores the verification code in Redis via
redisTemplate.opsForValue().set(key, code, VERIFICATION_TTL_MINUTES,
TimeUnit.MINUTES) before calling
emailService.sendVerificationCode(request.getEmail(), code), which can leave a
valid code in Redis if email delivery fails; change the flow so the email is
sent first and only on successful return persist the code, or if you must set
before send, wrap emailService.sendVerificationCode(...) in a try/catch and
remove the key (redisTemplate.delete(key)) on any failure (or propagate the
error) so a usable code is not left in Redis; update the logic around key, code,
VERIFICATION_TTL_MINUTES, and request.getEmail() accordingly.
In `@src/main/java/chainsawman/gesture/service/EmailService.java`:
- Line 22: The RestTemplate in EmailService is created without timeouts causing
potential thread hangs; replace the direct new RestTemplate() (field
restTemplate and any other plain RestTemplate uses around line 36) with a
RestTemplate configured via a ClientHttpRequestFactory that sets explicit
connect and read timeouts (e.g., using HttpComponentsClientHttpRequestFactory or
SimpleClientHttpRequestFactory), and initialize restTemplate with that factory
so Brevo HTTP calls have bounded connect/read timeouts (choose reasonable
timeout values like a few seconds).
- Around line 24-37: The sendVerificationCode method currently lets RestTemplate
transport and HTTP errors bubble up; wrap the call to restTemplate.postForEntity
inside a try/catch that catches HttpStatusCodeException and RestClientException
and rethrows a domain-specific unchecked exception (e.g., EmailSendException or
EmailServiceException) from EmailService with a clear message, the HTTP
status/body when available, and the original exception as the cause so API
clients receive a stable error contract; add the new exception class if missing
and update sendVerificationCode to throw it on failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 66f8f4ba-bef0-4ff1-937d-83e1e5e2db1e
📒 Files selected for processing (8)
src/main/java/chainsawman/gesture/controller/AuthController.javasrc/main/java/chainsawman/gesture/dto/user/request/EmailSendRequest.javasrc/main/java/chainsawman/gesture/dto/user/request/EmailVerificationRequest.javasrc/main/java/chainsawman/gesture/dto/user/response/EmailVerificationResponse.javasrc/main/java/chainsawman/gesture/service/AuthService.javasrc/main/java/chainsawman/gesture/service/EmailService.javasrc/main/resources/application-prod.propertiessrc/main/resources/application.properties
Comment on lines
+223
to
+224
| redisTemplate.opsForValue().set(key, code, VERIFICATION_TTL_MINUTES, TimeUnit.MINUTES); | ||
| emailService.sendVerificationCode(request.getEmail(), code); |
There was a problem hiding this comment.
Avoid leaving verification codes in Redis when email delivery fails.
At Line 223-224, code is persisted before the external send. If send fails, a valid code remains without user delivery.
Proposed fix
public void sendEmailVerification(EmailSendRequest request) {
String code = String.format("%06d", SECURE_RANDOM.nextInt(1_000_000));
String key = EMAIL_VERIFICATION_PREFIX + request.getEmail();
- redisTemplate.opsForValue().set(key, code, VERIFICATION_TTL_MINUTES, TimeUnit.MINUTES);
- emailService.sendVerificationCode(request.getEmail(), code);
+ redisTemplate.opsForValue().set(key, code, VERIFICATION_TTL_MINUTES, TimeUnit.MINUTES);
+ try {
+ emailService.sendVerificationCode(request.getEmail(), code);
+ } catch (RuntimeException e) {
+ redisTemplate.delete(key);
+ throw e;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/chainsawman/gesture/service/AuthService.java` around lines 223
- 224, The code stores the verification code in Redis via
redisTemplate.opsForValue().set(key, code, VERIFICATION_TTL_MINUTES,
TimeUnit.MINUTES) before calling
emailService.sendVerificationCode(request.getEmail(), code), which can leave a
valid code in Redis if email delivery fails; change the flow so the email is
sent first and only on successful return persist the code, or if you must set
before send, wrap emailService.sendVerificationCode(...) in a try/catch and
remove the key (redisTemplate.delete(key)) on any failure (or propagate the
error) so a usable code is not left in Redis; update the logic around key, code,
VERIFICATION_TTL_MINUTES, and request.getEmail() accordingly.
Comment on lines
+232
to
+237
| if (stored == null) { | ||
| throw new ExpiredVerificationCodeException(); | ||
| } | ||
| if (!stored.equals(request.getCode())) { | ||
| throw new InvalidVerificationCodeException(); | ||
| } |
There was a problem hiding this comment.
Add attempt limiting to prevent OTP brute force.
Line 232-237 allows unlimited verification attempts for a 6-digit code during TTL. This is a security gap on an unauthenticated endpoint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/chainsawman/gesture/service/AuthService.java` around lines 232
- 237, The verification block in AuthService allows unlimited OTP guesses; add
attempt limiting by tracking and incrementing a per-code attempt counter stored
alongside the OTP (e.g., in the same Redis/kv store) before validating. Define a
MAX_ATTEMPTS constant (e.g., 5), increment the counter key (e.g.,
verificationAttemptsKey for the same request identifier) atomically, and if the
count exceeds MAX_ATTEMPTS throw a new TooManyAttemptsException and
invalidate/delete the stored code; otherwise proceed to compare
stored.equals(request.getCode()) and reset/delete both the code and attempts on
successful verification or when expired. Ensure the new counter respects the
original TTL (set its expiry to the OTP TTL) and use the AuthService method
handling verification to locate and implement these changes.
| @Value("${brevo.template.verification-id}") | ||
| private int templateId; | ||
|
|
||
| private final RestTemplate restTemplate = new RestTemplate(); |
There was a problem hiding this comment.
Configure HTTP timeouts for Brevo calls to avoid request-thread hangs.
Line 22 and Line 36 currently use a default RestTemplate without explicit connect/read timeouts. A slow/unresponsive Brevo call can block auth requests for too long.
Proposed fix
`@Service`
public class EmailService {
@@
- private final RestTemplate restTemplate = new RestTemplate();
+ private final RestTemplate restTemplate;
+
+ public EmailService(RestTemplateBuilder restTemplateBuilder) {
+ this.restTemplate = restTemplateBuilder
+ .setConnectTimeout(Duration.ofSeconds(3))
+ .setReadTimeout(Duration.ofSeconds(5))
+ .build();
+ }
@@
- restTemplate.postForEntity(BREVO_API_URL, new HttpEntity<>(body, headers), String.class);
+ restTemplate.postForEntity(BREVO_API_URL, new HttpEntity<>(body, headers), String.class);Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/chainsawman/gesture/service/EmailService.java` at line 22, The
RestTemplate in EmailService is created without timeouts causing potential
thread hangs; replace the direct new RestTemplate() (field restTemplate and any
other plain RestTemplate uses around line 36) with a RestTemplate configured via
a ClientHttpRequestFactory that sets explicit connect and read timeouts (e.g.,
using HttpComponentsClientHttpRequestFactory or SimpleClientHttpRequestFactory),
and initialize restTemplate with that factory so Brevo HTTP calls have bounded
connect/read timeouts (choose reasonable timeout values like a few seconds).
Comment on lines
+24
to
+37
| public void sendVerificationCode(String toEmail, String code) { | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.set("api-key", apiKey); | ||
| headers.set("accept", "application/json"); | ||
| headers.setContentType(MediaType.APPLICATION_JSON); | ||
|
|
||
| Map<String, Object> body = Map.of( | ||
| "to", List.of(Map.of("email", toEmail)), | ||
| "templateId", templateId, | ||
| "params", Map.of("code", code) | ||
| ); | ||
|
|
||
| restTemplate.postForEntity(BREVO_API_URL, new HttpEntity<>(body, headers), String.class); | ||
| } |
There was a problem hiding this comment.
Wrap Brevo client failures into a domain exception.
Line 36 can throw transport/4xx/5xx exceptions directly. Convert them to a consistent domain error so API clients get stable error contracts.
Proposed fix
+import org.springframework.web.client.RestClientException;
@@
public void sendVerificationCode(String toEmail, String code) {
@@
- restTemplate.postForEntity(BREVO_API_URL, new HttpEntity<>(body, headers), String.class);
+ try {
+ restTemplate.postForEntity(BREVO_API_URL, new HttpEntity<>(body, headers), String.class);
+ } catch (RestClientException e) {
+ throw new RuntimeException("이메일 전송에 실패했습니다."); // replace with project DomainException
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/chainsawman/gesture/service/EmailService.java` around lines 24
- 37, The sendVerificationCode method currently lets RestTemplate transport and
HTTP errors bubble up; wrap the call to restTemplate.postForEntity inside a
try/catch that catches HttpStatusCodeException and RestClientException and
rethrows a domain-specific unchecked exception (e.g., EmailSendException or
EmailServiceException) from EmailService with a clear message, the HTTP
status/body when available, and the original exception as the cause so API
clients receive a stable error contract; add the new exception class if missing
and update sendVerificationCode to throw it on failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
추가된 기능
brevo smtp서비스를 이용해 구현함
도메인으로는 산학 회사에서 받은 도메인 사용
Summary by CodeRabbit
New Features