refactor(auth): use existing OTP flow for password reset (OTP-based forgot/reset)#55
Merged
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds guest and auth session subsystems (models, repos, services), guest-session and session-aware API endpoints, WebSocket chat, Redis utilities (rate limit, events), pipeline DLQ/audit logging, config/env updates, Alembic migrations, and comprehensive tests. ChangesGuest/auth sessions, WebSocket chat, pipeline audit
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy password-reset token flow with the existing OTP infrastructure, and (in the same change set) introduces broader session/guest-session management plus real-time/WebSocket + pipeline observability components.
Changes:
- Switch password reset to OTP-based verification (
RESET_PASSWORDpurpose) and update schemas/tests accordingly. - Add per-device
auth_sessionsrefresh-token ownership and guest-session lifecycle/limits + migration hooks. - Add Redis-backed eventing + WebSocket chat, plus pipeline audit logging and resiliency changes (locks/DLQ/backoff/circuit breaker).
Reviewed changes
Copilot reviewed 86 out of 89 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Pin pyjwt version. |
| pyproject.toml | Pin pyjwt; add pytest marker; Ruff config. |
| README.md | Update high-level guest/auth flow docs (currently truncated). |
| EDGE_CASES.md | Document EventBus/ConnectionRegistry edge cases. |
| .gitignore | Adjust ignored paths (incl. .env.example). |
| .github/workflows/ci.yaml | CI security step change (pip-audit ignore). |
| .env.example | Refresh example env values; add guest/auth session settings. |
| tests/conftest.py | Add fake Redis for rate-limit; optimize DB setup. |
| tests/test_ws_chat_endpoint.py | New WebSocket endpoint integration tests. |
| tests/test_websocket_chat_service.py | New unit tests for websocket chat service logic. |
| tests/test_pipeline_audit_log.py | New tests for pipeline-log endpoint + transition guards. |
| tests/test_oauth_state.py | New tests for signed OAuth state tokens. |
| tests/test_medical_case_full.py | Update guest-session + wrong-user expectations. |
| tests/test_logout.py | Update logout tests for AuthSessionManager dep. |
| tests/test_lab_result_api.py | Update wrong-user expectation. |
| tests/test_guest_session.py | New guest session service tests. |
| tests/test_guest_session_api.py | New guest-session endpoint tests. |
| tests/test_guest_session_dedup.py | New tests for guest-session dedup behavior. |
| tests/test_guest_session_enforcement.py | New guest-session enforcement tests (upload/case/chat). |
| tests/test_guest_session_limits.py | New guest-session upload limit tests. |
| tests/test_guest_session_migration.py | New guest-session migration tests. |
| tests/test_guest_migrate_idempotent.py | New idempotency test under row lock. |
| tests/test_guest_cases_api.py | New guest-case listing endpoint tests. |
| tests/test_eventbus.py | New EventBus tests. |
| tests/test_connection_registry.py | New ConnectionRegistry tests. |
| tests/test_auth_sessions.py | New per-device auth-session + OTP reset coverage. |
| app/tasks/pipeline.py | Add redis lock, DLQ, retries/backoff, audit logging, event publish. |
| app/tasks/maintenance.py | Add scheduled purge of stale guest sessions. |
| app/services/websocket.py | Add connection registry for per-user socket fanout. |
| app/services/websocket_chat.py | Add token-budget trimming + streaming LLM chat logic. |
| app/services/ocr.py | Add fetch headers + redirects for OCR file download. |
| app/services/oauth_state.py | Add signed OAuth state token helpers. |
| app/services/medical_case.py | Enforce guest session resolution/touch; list guest cases. |
| app/services/mail_transport.py | Improve fallback error reporting. |
| app/services/llm.py | Add circuit breaker + streaming support and broaden fallback codes. |
| app/services/lab_result.py | Add guest-session enforcement for JSON upload path (partial). |
| app/services/guest.py | Add guest session helper facade over GuestSessionManager. |
| app/services/guest_sessions.py | Add GuestSessionManager lifecycle/limits/migration logic. |
| app/services/events.py | Add Redis-backed EventBus (publish/subscribe). |
| app/services/email_service.py | Allow clinsight: URL scheme. |
| app/services/chat.py | Enforce guest chat limits + ownership via get_case. |
| app/services/auth/account.py | Refactor auth to return user only (tokens moved to sessions). |
| app/services/auth/init.py | Export AuthSessionManager; drop rotate helper export. |
| app/services/auth_sessions.py | Add per-device refresh-token session manager. |
| app/schemas/ws_chat.py | Add WebSocket message schemas + wire helpers. |
| app/schemas/pipeline_audit_log.py | Add response schema for pipeline audit logs. |
| app/schemas/medical_case.py | Change guest_session_id type to UUID. |
| app/schemas/lab_result.py | Remove guest_session_id from UploadRequest. |
| app/schemas/guest.py | Add guest session response schema. |
| app/schemas/chat.py | Remove user_id from ChatCreate. |
| app/schemas/auth.py | Add device/platform fields + reset-password email field + session schema. |
| app/schemas/init.py | Export PipelineAuditLogResponse. |
| app/repositories/pipeline_audit_log.py | Add pipeline audit log repo + safe log helper. |
| app/repositories/medical_case.py | Add guest-session UUID handling + count helper. |
| app/repositories/guest_session.py | Add guest session repo with atomic counters + cleanup. |
| app/repositories/chat.py | Add migration helper + recent-history query. |
| app/repositories/auth_session.py | Add auth session repository. |
| app/repositories/init.py | Export new repositories. |
| app/models/user.py | Add relationship to migrated guest sessions. |
| app/models/pipeline_audit_log.py | Add pipeline audit log model. |
| app/models/otp.py | Add RESET_PASSWORD OTP purpose. |
| app/models/medical_case.py | Change guest session to FK UUID + relationship. |
| app/models/lab_result.py | Add audit log relationship. |
| app/models/guest_session.py | Add guest session model. |
| app/models/auth_session.py | Add auth session model. |
| app/models/init.py | Export new models. |
| app/main.py | Create/connect EventBus + ConnectionRegistry in lifespan. |
| app/db/session.py | Minor engine init formatting. |
| app/core/redis_client.py | Add shared async Redis client helpers. |
| app/core/rate_limit.py | Add Redis-backed rate limiting. |
| app/core/guest_session.py | Add IP hashing + device fingerprint helpers. |
| app/core/exceptions.py | Add GuestLimitExceeded + RateLimitExceeded. |
| app/core/config.py | Add guest/auth session + OAuth settings. |
| app/core/celery_app.py | Add DLQ queue + beat schedule + includes. |
| app/api/v1/router.py | Add guest-session/cases and ws_chat routers. |
| app/api/v1/endpoints/ws_chat.py | Add /ws/chat WebSocket endpoint. |
| app/api/v1/endpoints/medical_case.py | Add guest/session context usage + pipeline-log endpoint. |
| app/api/v1/endpoints/lab_result.py | Adjust auth/guest context usage (upload path mismatch). |
| app/api/v1/endpoints/guest_session.py | Add guest-session create/me endpoints + rate limit. |
| app/api/v1/endpoints/guest_cases.py | Add guest cases listing endpoint. |
| app/api/v1/endpoints/chat.py | Enforce guest session + limits for chat. |
| app/api/v1/endpoints/auth.py | OTP reset + auth sessions + OAuth state + session management endpoints. |
| app/api/v1/endpoints/ai_interpretation.py | Enforce guest session resolution for interpretation endpoints. |
| app/api/deps.py | Add guest/auth session dependencies (currently duplicated/broken). |
| alembic/env.py | Minor formatting. |
| alembic/versions/da75221c0906_final_table_for_mvp.py | New consolidated “final” migration including new tables. |
| alembic/versions/a8598ffe37d5_initial.py | Removed old migration. |
| alembic/versions/3b1e0f4a2d9c_add_pending_email_and_email_change_token_to_users.py | Removed old migration. |
| alembic/versions/e425a99e7480_add_contact_messages_table.py | Removed old migration. |
Comments suppressed due to low confidence (1)
app/api/deps.py:304
SessionContext,get_session_context, andSessionContextDepare defined twice in this module (see the second definitions starting here). The later definitions override the earlier async guest-session validating version, which breaks endpoints expectingctx.guest_sessionand reintroduces unvalidatedguest_session_idusage. Remove the duplicate block and keep a single, validated SessionContext shape used consistently across endpoints.
@dataclass
class SessionContext:
user: User | None
guest_session_id: str | None
def get_session_context(
user: OptionalUser,
guest_session_id: GuestSessionId,
) -> SessionContext:
return SessionContext(user=user, guest_session_id=guest_session_id)
SessionContextDep = Annotated[SessionContext, Depends(get_session_context)]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+55
| } | ||
| if file.content_type not in valid_media_types: | ||
| raise BadRequestError("Unsupported file type. Acceptable types are JPEG, PNG, GIF, WebP, or PDF.") | ||
| raise BadRequestError("Unsupported file type. Acceptable types are JPEG, PNG, WebP, or PDF.") |
Comment on lines
24
to
+33
| @asynccontextmanager | ||
| async def lifespan(app: FastAPI) -> None: # noqa: ARG001 | ||
| async def lifespan(app: FastAPI): | ||
| configure_celery() | ||
|
|
||
| event_bus = EventBus(settings.CELERY_BROKER_URL) | ||
| await event_bus.connect() | ||
| app.state.event_bus = event_bus | ||
|
|
||
| connection_registry = ConnectionRegistry() | ||
| app.state.connection_registry = connection_registry |
Comment on lines
+33
to
+36
| async def disconnect(self) -> None: | ||
| if self.redis: | ||
| await self.redis.close() | ||
| logger.info("EventBus disconnected from Redis") |
Comment on lines
+68
to
+102
| async def subscribe( | ||
| self, | ||
| user_id: UUID, | ||
| ) -> AsyncIterator[dict]: | ||
| if not self.redis: | ||
| logger.error("EventBus not connected") | ||
| return | ||
|
|
||
| channel = f"events:{user_id}" | ||
| pubsub = self.redis.pubsub() | ||
| await pubsub.subscribe(channel) | ||
| logger.debug(f"Subscribed to channel: {channel}") | ||
|
|
||
| idle_timeout = 30 | ||
|
|
||
| try: | ||
| while True: | ||
| message = await asyncio.wait_for( | ||
| pubsub.get_message(ignore_subscribe_messages=True), | ||
| timeout=idle_timeout, | ||
| ) | ||
|
|
||
| if message: | ||
| try: | ||
| data = json.loads(message["data"]) | ||
| yield data | ||
| except (json.JSONDecodeError, KeyError, TypeError): | ||
| logger.error("Failed to deserialize message") | ||
| continue | ||
| else: | ||
| yield None | ||
|
|
||
| except asyncio.TimeoutError: | ||
| yield None | ||
| except asyncio.CancelledError: |
| sa.Column('id', sa.UUID(), nullable=False), | ||
| sa.Column('user_id', sa.UUID(), nullable=False), | ||
| sa.Column('code_hash', sa.String(), nullable=False), | ||
| sa.Column('purpose', sa.Enum('email_verification', name='otppurpose'), nullable=False), |
Comment on lines
+18
to
+36
| def connect(self, user_id: UUID, websocket: WebSocket) -> None: | ||
| if user_id not in self.active_connections: | ||
| self.active_connections[user_id] = [] | ||
| self.active_connections[user_id].append(websocket) | ||
| logger.debug( | ||
| "WebSocket connected", | ||
| extra={ | ||
| "user_id": str(user_id), | ||
| "connection_count": len(self.active_connections[user_id]), | ||
| }, | ||
| ) | ||
|
|
||
| def disconnect(self, user_id: UUID, websocket: WebSocket) -> None: | ||
| if user_id in self.active_connections: | ||
| try: | ||
| self.active_connections[user_id].remove(websocket) | ||
| if not self.active_connections[user_id]: | ||
| del self.active_connections[user_id] | ||
| logger.debug( |
Comment on lines
345
to
360
| # Google OAuth | ||
| @router.get("/google") | ||
| async def google_login() -> RedirectResponse: | ||
| async def google_login( | ||
| guest_session_id: str | None = Query(None, description="Guest session to migrate after OAuth"), | ||
| device_id: str | None = Query(None, description="Client device identifier for per-device auth session"), | ||
| platform: str | None = Query("web", description="Client platform (web, ios, android)"), | ||
| ) -> RedirectResponse: | ||
| """Redirect to Google's OAuth consent screen.""" | ||
| settings = get_settings() | ||
| oauth_state = create_oauth_state( | ||
| guest_session_id=guest_session_id, | ||
| device_id=device_id, | ||
| platform=platform, | ||
| ) | ||
| query_params = urlencode( | ||
| { |
Comment on lines
33
to
38
| ## 5. High-Level Logic & Behaviour | ||
| - **Upload**: Accepted: JPG, PNG, PDF. Unreadable/unsupported file → error state + reupload. OCR timeout at 15s → error + reupload. | ||
| - **AI Engine**: Classifies each value as Normal / Caution / Abnormal against reference. Disclaimer appended automatically. AI error → fallback message + retry. | ||
| - **Guest Flow**: Upload-to-interpretation without account. The session expires in 1hr. Sign-up prompt on: After 3 messages; chat history migration is automatic on signup. | ||
| - **Auth & Session**: OTP valid 15 mins, single-use. Password reset link expires in 1hr. Google OAuth via OAuth 2.0. Tokens stored securely on device. | ||
| - **Guest flow**: Call `POST /api/v1/guest-session` with `X-Device-Fingerprint` to obtain or reuse a `guest_session_id` (stored in Postgres, default TTL 1 hour). Send `X-Guest-Session-Id` on upload, c | ||
| - **Auth & session**: JWT access token in `Authorization: Bearer`; refresh token in `refresh_token` HttpOnly cookie, backed by `auth_sessions` per device. `CurrentUser` requires verified email. `Sessi | ||
| - **Notifications**: Push sent on interpretation complete. Suppressed if the user is actively on the result screen. Users who opt out can still access results in-app. |
Comment on lines
60
to
62
| - name: Check for vulnerabilities in dependencies | ||
| run: uv run pip-audit | ||
| run: uv run pip-audit --ignore-vuln PYSEC-2025-183 | ||
|
|
Comment on lines
+124
to
+137
| # 1. Save user message | ||
| user_msg = await save_user_message(chat_repo, case_id, user_id, text) | ||
| logger.info("[ws_chat] user message saved id=%s case=%s", user_msg.id, case_id) | ||
|
|
||
| # 2. Load history (200 messages — trim_history handles token budget) | ||
| history = await chat_repo.list_by_case(case_id, limit=200) | ||
|
|
||
| # 3. Build system prompt | ||
| system_prompt = await build_system_prompt(case_id, interp_repo, lab_repo) | ||
|
|
||
| # 4. Stream AI response | ||
| collected: list[str] = [] | ||
| try: | ||
| async for token in generate_ai_response(system_prompt, history, text): |
…/hngprojects/clinical-api into fix/otp-password-reset-from-link
…king for thread safety fix(events): improve disconnect method to handle sync and async close for Redis connections fix(medical_case): change ForbiddenError to NotFoundError to hide case existence from unauthorized users
…thread safety fix(events): ensure proper async handling of Redis connection closure fix(medical_case): update error handling for guest session access to medical cases
… improved isolation
…cpg compatibility
Collaborator
Author
|
@Lftobs all is well now |
Lftobs
approved these changes
May 20, 2026
Merged
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.
Description
Replace the token password-reset flow with the existing OTP system used for signup/email verification.
Changes include:
RESET_PASSWORDtoOtpPurposeand reuse thecreate_otp_for_user/verify_otp_for_userflows.POST /auth/forgot-passwordnow issues an OTP (sent via the existing OTP email template) instead of creating a separate password-reset token row.POST /auth/reset-passwordnow acceptsemail,token(OTP), andnew_passwordand verifies the OTP before updating the user's password.ResetPasswordRequestschema to includeemailand a token length compatible with OTPs.tests/test_auth_sessions.py(removed the separatetest_password_reset_otp.py) so coverage lives with related auth tests.Related Issue (Link to issue ticket)
No linked issue — this is a feature refinement requested in-branch.
Motivation and Context
The app already sends OTPs for signup/email verification. Using the same OTP infrastructure for password reset:
This change modifies an existing feature (password reset) rather than adding a completely new endpoint.
How Has This Been Tested?
tests/test_auth_sessions.py:test_forgot_password_sends_six_digit_otpverifies an OTP is sent and is numeric/length 6.test_reset_password_accepts_otp_and_changes_passwordverifies the OTP can be used to change the password and the user can log in with the new password.Note: Changes were committed locally on branch
feature/otp-password-reset. The commit was created successfully but pushing to the remote failed due to a transient network/host resolution error; the local branch and commit exist.Screenshots (if appropriate - Postman, etc):
N/A
Types of changes
Checklist:
tests/test_auth_sessions.py).Summary by CodeRabbit
Release Notes
New Features