Feature/auth password reset with OCI Email SDK#139
Merged
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 46 minutes and 39 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 Walkthrough워크스루이 PR은 비밀번호 재설정 기능을 구현합니다. 이메일 기반 검증 코드를 통해 사용자가 비밀번호를 재설정할 수 있도록 인증 시스템을 확장하며, 메일 서비스 인프라, 데이터베이스 모델, 리포지토리 메서드, 비즈니스 로직, API 엔드포인트 및 테스트를 추가합니다. 변경 사항비밀번호 재설정 플로우
🎯 평가🎯 3 (중간) | ⏱️ ~25분 관련된 가능성이 있는 PR
제안 검토자
🚥 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 |
![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
🧹 Nitpick comments (2)
wacruit/src/tests/auth/test_auth_password_reset_service.py (1)
66-82: ⚡ Quick win시도 횟수 상한(5회) 경계 테스트를 추가해 주세요.
현재는 오답 1회 증가만 검증해서, “최대 5회” 규칙의 경계(5회/6회) 회귀를 잡기 어렵습니다. 이 케이스를 추가하면 보안 규칙 검증 신뢰도가 크게 올라갑니다.
🤖 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 `@wacruit/src/tests/auth/test_auth_password_reset_service.py` around lines 66 - 82, Add a boundary test that simulates repeated wrong password-reset codes to exercise the 5-attempt limit: call auth_service._generate_password_reset_code = lambda: "123456" and auth_service.send_password_reset_email(EmailStr(user.email)) once, then call auth_service.verify_password_reset_code(EmailStr(user.email), "000000") five times asserting InvalidPasswordResetCodeException each time and that auth_repository.get_latest_password_reset_verification(EmailStr(user.email)).attempt_count increments up to 5, then call it a sixth time and assert the expected locked/blocked behavior (e.g., still raises InvalidPasswordResetCodeException or a specific lock exception and that attempt_count == 6 or a locked flag is set) to cover the 5/6 boundary for verify_password_reset_code and related repository state.wacruit/src/database/migrations/versions/2026_05_17_0000-2f9c8e0a1b34_add_password_reset_verification.py (1)
18-49: 운영 관점에서 만료 레코드 정리 정책을 함께 두는 것을 권장합니다.
password_reset_verification는 이메일(PII)을 저장하므로, 만료/사용 완료 레코드를 주기적으로 정리하는 배치(또는 TTL성 정책)와 관련 모니터링을 같이 정의해두면 장기 운영 리스크를 줄일 수 있습니다.🤖 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 `@wacruit/src/database/migrations/versions/2026_05_17_0000-2f9c8e0a1b34_add_password_reset_verification.py` around lines 18 - 49, Add an operational cleanup policy for the password_reset_verification table: in upgrade() create an index on expires_at (and optionally on verified_at/used_at) to make TTL deletions efficient, and add a migration comment or companion SQL to create a DB-side TTL/partitioning rule or background job that periodically deletes rows where expires_at < now() OR used_at IS NOT NULL OR verified_at IS NOT NULL; also add/update monitoring/alerting hooks (e.g., a metric name or housekeeping job entry) so the batch run success/failure and retention counts for password_reset_verification are tracked. Ensure references to the table and columns use the exact names password_reset_verification, expires_at, used_at, verified_at, and the migration functions upgrade()/downgrade() so reviewers can locate the changes.
🤖 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 `@wacruit/src/apps/auth/schemas.py`:
- Line 32: 스키마의 new_password Field(new_password: str = Field(...,
max_length=50))에 최소 길이 검증이 빠져있으니 Field에 min_length를 추가해 재설정 계약에 맞는 최소 길이(예:
min_length=8 또는 프로젝트 규정값)를 명시하세요; 해당 변경은 wacruit/src/apps/auth/schemas.py의
new_password 선언을 수정하여 적용하면 됩니다.
In `@wacruit/src/apps/auth/services.py`:
- Around line 114-122: The current flow in send_password_reset_code path uses
AuthService methods expire_unused_password_reset_verifications and
create_password_reset_verification separately which can race and invalidate the
just-sent code; introduce a new repository method (e.g.,
auth_repository.replace_active_password_reset_for_email) that, inside a DB
transaction, atomically expires any existing active PasswordResetVerification
rows for the email and inserts the new PasswordResetVerification (with code_hash
from PasswordService.hash_password and expires_at), then update the service to
call that single transactional method instead of calling
expire_unused_password_reset_verifications and
create_password_reset_verification separately after
self.email_service.send_password_reset_code.
- Around line 129-141: The reset_password flow can race because verification is
checked and consumed in separate steps; change reset_password to perform a
single atomic DB transaction or acquire a row lock when consuming the
verification so the check-and-mark-used and the user password update happen
together. Specifically, in reset_password replace the current pattern of calling
_get_valid_password_reset_verification then later setting verification.used_at
and calling auth_repository.update_password_reset_verification /
auth_repository.update_user with a repository method or transaction block that:
re-queries the verification FOR UPDATE (or equivalent), verifies it is unused,
sets used_at, updates the user's hashed password
(PasswordService.hash_password), and commits both updates in one transaction so
a concurrent request cannot pass the check.
In `@wacruit/src/apps/mail/services.py`:
- Around line 35-40: The SMTP client can hang because no socket timeout is set;
update the smtplib.SMTP call to pass a timeout (e.g., timeout =
getattr(mail_config, "timeout", 10)) so the connection and subsequent calls
(smtplib.SMTP(...), smtp.starttls(), smtp.login(...), smtp.send_message(...))
will fail fast; ensure mail_config exposes a configurable timeout or use a
sensible default and propagate that value into the smtplib.SMTP(...) constructor
call.
---
Nitpick comments:
In
`@wacruit/src/database/migrations/versions/2026_05_17_0000-2f9c8e0a1b34_add_password_reset_verification.py`:
- Around line 18-49: Add an operational cleanup policy for the
password_reset_verification table: in upgrade() create an index on expires_at
(and optionally on verified_at/used_at) to make TTL deletions efficient, and add
a migration comment or companion SQL to create a DB-side TTL/partitioning rule
or background job that periodically deletes rows where expires_at < now() OR
used_at IS NOT NULL OR verified_at IS NOT NULL; also add/update
monitoring/alerting hooks (e.g., a metric name or housekeeping job entry) so the
batch run success/failure and retention counts for password_reset_verification
are tracked. Ensure references to the table and columns use the exact names
password_reset_verification, expires_at, used_at, verified_at, and the migration
functions upgrade()/downgrade() so reviewers can locate the changes.
In `@wacruit/src/tests/auth/test_auth_password_reset_service.py`:
- Around line 66-82: Add a boundary test that simulates repeated wrong
password-reset codes to exercise the 5-attempt limit: call
auth_service._generate_password_reset_code = lambda: "123456" and
auth_service.send_password_reset_email(EmailStr(user.email)) once, then call
auth_service.verify_password_reset_code(EmailStr(user.email), "000000") five
times asserting InvalidPasswordResetCodeException each time and that
auth_repository.get_latest_password_reset_verification(EmailStr(user.email)).attempt_count
increments up to 5, then call it a sixth time and assert the expected
locked/blocked behavior (e.g., still raises InvalidPasswordResetCodeException or
a specific lock exception and that attempt_count == 6 or a locked flag is set)
to cover the 5/6 boundary for verify_password_reset_code and related repository
state.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a681001-909b-4a46-b313-fbe7e387bf17
📒 Files selected for processing (15)
wacruit/src/apps/auth/__init__.pywacruit/src/apps/auth/exceptions.pywacruit/src/apps/auth/models.pywacruit/src/apps/auth/repositories.pywacruit/src/apps/auth/schemas.pywacruit/src/apps/auth/services.pywacruit/src/apps/auth/views.pywacruit/src/apps/mail/__init__.pywacruit/src/apps/mail/config.pywacruit/src/apps/mail/exceptions.pywacruit/src/apps/mail/services.pywacruit/src/database/__init__.pywacruit/src/database/migrations/versions/2026_05_17_0000-2f9c8e0a1b34_add_password_reset_verification.pywacruit/src/tests/auth/conftest.pywacruit/src/tests/auth/test_auth_password_reset_service.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
wacruit/src/apps/auth/repositories.py (1)
42-61:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift동시 재발급이 아직 직렬화되지 않습니다.
이 메서드는 현재 unused row만 UPDATE한 뒤 새 row를 INSERT합니다. 같은 이메일로 두 요청이 동시에 들어오면 서로의 신규 row를 보지 못한 채 둘 다 active row를 남길 수 있고, 이후 검증은 최신 1건만 보므로 사용자에게 방금 도착한 코드가 즉시 무효화될 수 있습니다. 이메일 단위로
Userrow를FOR UPDATE로 잠그거나 advisory lock을 잡아서 expire+insert를 직렬화해 주세요.🔧 한 가지 가능한 방향
) -> PasswordResetVerification: with self.transaction: + ( + self.session.query(User) + .where(User.email == email) + .with_for_update() + .first() + ) ( self.session.query(PasswordResetVerification) .where( PasswordResetVerification.email == email, PasswordResetVerification.used_at.is_(None),🤖 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 `@wacruit/src/apps/auth/repositories.py` around lines 42 - 61, The current replace_active_password_reset_for_email function can race because it UPDATEs unused PasswordResetVerification rows then INSERTs a new one without serialization; wrap the update+insert in a transaction-scoped lock by either selecting the owning User row FOR UPDATE (e.g., query User where User.email==email and .with_for_update()) or acquiring a DB advisory lock keyed by the email (pg_advisory_xact_lock using a hash of email) at the start of replace_active_password_reset_for_email, then perform the existing .update(...) and self.session.add(verification) while the lock/transaction is held so expire+insert are serialized for that email (ensure to keep use of self.transaction and rollback behavior unchanged).wacruit/src/apps/auth/services.py (1)
139-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winreset 단계에서도 유효성 재검증을 락 안으로 넣어 주세요.
지금은
_get_valid_password_reset_verification()결과를 신뢰한 뒤 별도 consume 호출로 넘어가서, 그 사이에 재발급 요청이 기존 row의expires_at를 당기면 이미 무효화된 코드도 비밀번호 변경에 성공할 수 있습니다.consume_password_reset_verification()가used_at뿐 아니라verified_at is not null과expires_at >= now까지 함께 확인하도록 옮기는 편이 안전합니다.🤖 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 `@wacruit/src/apps/auth/services.py` around lines 139 - 146, 현재 서비스에서 _get_valid_password_reset_verification() 결과를 신뢰하고 별도 consume 호출로 넘어가면 race condition이 발생할 수 있으니, 검증 로직을 consume_password_reset_verification() 안으로 옮겨 DB 락(또는 트랜잭션 내 FOR UPDATE 같은 방식)으로 처리하세요: consume_password_reset_verification(verification_id, email, hashed_pw, now) 내부에서 used_at IS NULL, verified_at IS NOT NULL, expires_at >= now 조건을 함께 확인하고 업데이트(used_at/hashed password 저장)를 시도해 성공 여부를 불리언으로 반환하도록 변경하고, 서비스 코드는 이전의 _get_valid... 결과를 최종 결정으로 사용하지 않고 consume의 반환값만으로 성공/실패를 판단하도록 수정하세요.
🤖 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 `@wacruit/src/apps/auth/services.py`:
- Around line 114-123: Currently the code sends the reset email before
persisting the DB state, which can lead to an un-verifyable code if
replace_active_password_reset_for_email fails; update the sequence in the method
that calls send_password_reset_code and replace_active_password_reset_for_email
so you first create and persist PasswordResetVerification (using
PasswordService.hash_password(code) and
replace_active_password_reset_for_email(email, PasswordResetVerification(...),
now)) and only after that call
email_service.send_password_reset_code(str(email), code); ensure you still
compute expires_at and code beforehand and handle/report errors from
replace_active_password_reset_for_email before attempting to send the email.
In `@wacruit/src/apps/mail/services.py`:
- Around line 76-85: The try/except currently wraps both signer creation and
EmailDPClient initialization so any client creation errors (e.g. bad
settings.oci_region/endpoint) silently fall back to oci.config.from_file();
change it so only oci.auth.signers.InstancePrincipalsSecurityTokenSigner() is
inside the try and its Exception triggers the fallback to
oci.config.from_file(), while EmailDPClient(...) construction runs outside that
except block and is allowed to raise (fail-fast) if it errors; reference
InstancePrincipalsSecurityTokenSigner, EmailDPClient, settings.oci_region, and
oci.config.from_file when making the change.
---
Duplicate comments:
In `@wacruit/src/apps/auth/repositories.py`:
- Around line 42-61: The current replace_active_password_reset_for_email
function can race because it UPDATEs unused PasswordResetVerification rows then
INSERTs a new one without serialization; wrap the update+insert in a
transaction-scoped lock by either selecting the owning User row FOR UPDATE
(e.g., query User where User.email==email and .with_for_update()) or acquiring a
DB advisory lock keyed by the email (pg_advisory_xact_lock using a hash of
email) at the start of replace_active_password_reset_for_email, then perform the
existing .update(...) and self.session.add(verification) while the
lock/transaction is held so expire+insert are serialized for that email (ensure
to keep use of self.transaction and rollback behavior unchanged).
In `@wacruit/src/apps/auth/services.py`:
- Around line 139-146: 현재 서비스에서 _get_valid_password_reset_verification() 결과를
신뢰하고 별도 consume 호출로 넘어가면 race condition이 발생할 수 있으니, 검증 로직을
consume_password_reset_verification() 안으로 옮겨 DB 락(또는 트랜잭션 내 FOR UPDATE 같은 방식)으로
처리하세요: consume_password_reset_verification(verification_id, email, hashed_pw,
now) 내부에서 used_at IS NULL, verified_at IS NOT NULL, expires_at >= now 조건을 함께
확인하고 업데이트(used_at/hashed password 저장)를 시도해 성공 여부를 불리언으로 반환하도록 변경하고, 서비스 코드는 이전의
_get_valid... 결과를 최종 결정으로 사용하지 않고 consume의 반환값만으로 성공/실패를 판단하도록 수정하세요.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3d316a3-546d-4176-beef-005affec76f7
📒 Files selected for processing (9)
wacruit/src/apps/auth/repositories.pywacruit/src/apps/auth/schemas.pywacruit/src/apps/auth/services.pywacruit/src/apps/mail/config.pywacruit/src/apps/mail/services.pywacruit/src/settings.pywacruit/src/tests/auth/conftest.pywacruit/src/tests/auth/test_auth_password_reset_service.pywacruit/src/tests/mail/test_mail_service.py
✅ Files skipped from review due to trivial changes (1)
- wacruit/src/settings.py
snu-BuMinSeong
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacruit/src/apps/mail/services.py (1)
34-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
from_email형식 검증을 추가해 설정 오류를 조기에 차단하세요.Line 34-35는 빈 값만 검사하고 있어,
@가 없는 값이 들어오면 Line 90-92에서 잘못된message_id가 생성되고 502로 래핑될 수 있습니다. 설정 문제는MailConfigException으로 fail-fast 하는 편이 안전합니다.🔧 제안 수정
def send_email(self, to_email: str, subject: str, content: str) -> None: - if not mail_config.compartment_id or not mail_config.from_email: + if ( + not mail_config.compartment_id + or not mail_config.from_email + or "@" not in mail_config.from_email + or mail_config.from_email.rsplit("@", 1)[-1] == "" + ): raise MailConfigException()Also applies to: 90-92
🤖 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 `@wacruit/src/apps/mail/services.py` around lines 34 - 35, The code currently only checks for presence of mail_config.compartment_id and mail_config.from_email before raising MailConfigException; add a simple format validation for mail_config.from_email (e.g., ensure it contains a single '@' and a domain part) and raise MailConfigException with a clear message if invalid so configuration errors fail-fast; update the validation near the existing check referencing mail_config and MailConfigException and ensure this prevents invalid values reaching the message_id generation code paths around the logic that builds message_id (referenced in the file around lines handling message_id) which currently can produce malformed IDs when from_email lacks '@'.
🤖 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.
Outside diff comments:
In `@wacruit/src/apps/mail/services.py`:
- Around line 34-35: The code currently only checks for presence of
mail_config.compartment_id and mail_config.from_email before raising
MailConfigException; add a simple format validation for mail_config.from_email
(e.g., ensure it contains a single '@' and a domain part) and raise
MailConfigException with a clear message if invalid so configuration errors
fail-fast; update the validation near the existing check referencing mail_config
and MailConfigException and ensure this prevents invalid values reaching the
message_id generation code paths around the logic that builds message_id
(referenced in the file around lines handling message_id) which currently can
produce malformed IDs when from_email lacks '@'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d0647b7-15b8-45c1-8198-4faaa97b018f
📒 Files selected for processing (3)
wacruit/src/apps/auth/services.pywacruit/src/apps/mail/services.pywacruit/src/tests/mail/test_mail_service.py
mjy926
approved these changes
May 25, 2026
This was referenced Jun 7, 2026
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
2 participants
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.
변경 사항
POST /api/v3/auth/password-reset/emailPOST /api/v3/auth/password-reset/verifyPOST /api/v3/auth/password-resetpassword_reset_verification테이블 추가구현 상세
PasswordService로 해시하여 저장합니다.oci.email_data_plane.EmailDPClient.submit_email()을 사용합니다.oci.config.from_file())로 fallback합니다.필요한 운영 설정
OCI Email Delivery를 설정한 뒤 아래 secret/env 값을 등록해야 합니다.