Merged
Conversation
6 tasks
📝 WalkthroughWalkthroughAdds JWT-based authentication: configuration, DB models and session setup, password hashing and JWT utilities, Pydantic auth schemas, /auth endpoints (signup, login, refresh), router mounting, and startup DB table creation; plus dependency and packaging updates and .env/template edits. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant FastAPI as FastAPI
participant Security as Security Module
participant DB as Relational DB
Client->>FastAPI: POST /auth/login {email,password}
FastAPI->>DB: SELECT user WHERE email=...
DB-->>FastAPI: user (or none)
FastAPI->>Security: verify_password(plain, hashed)
Security-->>FastAPI: bool
alt valid
FastAPI->>Security: create_access_token(sub=email)
Security-->>FastAPI: access_token
FastAPI->>Security: create_refresh_token(sub=email)
Security-->>FastAPI: refresh_token
FastAPI-->>Client: 200 TokenResponse
else invalid
FastAPI-->>Client: 401 Unauthorized
end
sequenceDiagram
actor Client
participant FastAPI as FastAPI
participant Security as Security Module
participant DB as Relational DB
Client->>FastAPI: POST /auth/refresh {refresh_token}
FastAPI->>Security: decode_refresh_token(token)
alt token valid & type=refresh
Security-->>FastAPI: subject (email)
FastAPI->>DB: SELECT user WHERE email=subject
DB-->>FastAPI: user (or none)
alt user exists
FastAPI->>Security: create_access_token(...)
Security-->>FastAPI: new_access_token
FastAPI->>Security: create_refresh_token(...)
Security-->>FastAPI: new_refresh_token
FastAPI-->>Client: 200 TokenResponse
else
FastAPI-->>Client: 401 Unauthorized
end
else invalid
FastAPI-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 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: 5
🧹 Nitpick comments (2)
src/api/v1/endpoints/auth.py (2)
55-79: Consider anonymizing PII in logs.Line 78 logs the user's email address, which is personally identifiable information. Depending on your compliance requirements (GDPR/CCPA), consider logging a user ID or anonymized identifier instead.
♻️ Suggested change
- logger.info("New user registered: %s", user.email) + logger.info("New user registered: user_id=%s", user.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v1/endpoints/auth.py` around lines 55 - 79, The logger in signup currently emits PII (user.email); update the log to avoid storing email by logging a non-PII identifier instead — e.g., use the persisted User.id (available after db.commit()/db.refresh(user)) or an anonymized token (hash/UUID) and replace the logger.info("New user registered: %s", user.email) call with a message that includes only that ID/anonymized identifier (reference: signup function, logger.info and user.email/user.id).
98-116: Avoid shadowing the function name with a local variable.Line 114 uses
refreshas a variable name, which shadows the enclosingrefreshfunction. While Python allows this, it reduces readability and can cause confusion.♻️ Suggested change
access = create_access_token(subject=user.email) - refresh = create_refresh_token(subject=user.email) + refresh_token = create_refresh_token(subject=user.email) logger.info("Token refreshed for: %s", user.email) - return TokenResponse(access_token=access, refresh_token=refresh) + return TokenResponse(access_token=access, refresh_token=refresh_token)Additionally, the same PII logging consideration applies here (line 115).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v1/endpoints/auth.py` around lines 98 - 116, The refresh endpoint function `refresh` currently shadows its own name by assigning the local variable `refresh = create_refresh_token(...)`; rename that variable to something like `refresh_token` (or `new_refresh_token`) to avoid shadowing the `refresh` function, update the return to use the new variable (`TokenResponse(access_token=access, refresh_token=refresh_token)`), and adjust the PII logger call (`logger.info`) to avoid logging full email (e.g., log user id or masked email) instead of `user.email`; keep references to `create_refresh_token`, `create_access_token`, and the `refresh` function name intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Around line 22-24: The project uses EmailStr in src/models/auth_schema.py but
pyproject.toml's runtime dependencies list (the array containing
"sqlalchemy>=2.0", "python-jose[cryptography]>=3.3", "passlib[bcrypt]>=1.7")
does not include email-validator; add "email-validator" to the runtime
dependencies in pyproject.toml so Pydantic's EmailStr validation works at
runtime (ensure it's added alongside the existing dependency entries).
In `@src/api/v1/endpoints/auth.py`:
- Around line 82-95: The login endpoint's short-circuit "if not user or not
verify_password(...)" leaks timing for email enumeration; update the login
function to always perform a password hash comparison (use a configured
constant-time dummy verify when User is None) by calling verify_password with
either user.hashed_password or a dummy hash so timing is constant, then branch
on the result to raise HTTPException; also remove or redact direct PII from
logger.info (replace logging of user.email with a non-PII message or user.id
when available) and keep the rest of the flow (create_access_token,
create_refresh_token, TokenResponse) unchanged.
In `@src/core/config.py`:
- Line 12: The hardcoded fallback SECRET_KEY must be removed and made required
so the app fails fast if not provided: in src/core/config.py remove the default
value for SECRET_KEY and instead load it from environment/config where missing
will raise an explicit error (e.g., raise RuntimeError or let your settings
validation fail) so that SECRET_KEY cannot silently fall back to
"change-me-in-production"; update any settings class or loader that references
SECRET_KEY to validate presence at import/initialization time.
In `@src/db/database.py`:
- Around line 8-15: The module-level engine initialization using create_engine
with settings.DATABASE_URL can fail for SQLite if the parent directory doesn't
exist; before calling create_engine (i.e., before the engine variable is
created), detect when settings.DATABASE_URL startswith "sqlite" and ensure the
file's parent directory exists by resolving the DB file path and calling
os.makedirs(parent, exist_ok=True) (use pathlib or os.path to get the parent);
then proceed to call create_engine as currently written so the SQLite file can
be created without raising OperationalError.
In `@src/main.py`:
- Around line 42-45: Move the DB initialization call
Base.metadata.create_all(bind=engine) into the existing startup try block so
that any exceptions during table creation are caught by the same error handler
and logged via the structured critical path; locate the startup try/except
around application startup (where logger.info("Database tables initialized.") is
used), remove the standalone call before the try, and invoke
Base.metadata.create_all(bind=engine) inside the try just before the "Database
tables initialized." log so failures flow into the try's exception handling.
---
Nitpick comments:
In `@src/api/v1/endpoints/auth.py`:
- Around line 55-79: The logger in signup currently emits PII (user.email);
update the log to avoid storing email by logging a non-PII identifier instead —
e.g., use the persisted User.id (available after db.commit()/db.refresh(user))
or an anonymized token (hash/UUID) and replace the logger.info("New user
registered: %s", user.email) call with a message that includes only that
ID/anonymized identifier (reference: signup function, logger.info and
user.email/user.id).
- Around line 98-116: The refresh endpoint function `refresh` currently shadows
its own name by assigning the local variable `refresh =
create_refresh_token(...)`; rename that variable to something like
`refresh_token` (or `new_refresh_token`) to avoid shadowing the `refresh`
function, update the return to use the new variable
(`TokenResponse(access_token=access, refresh_token=refresh_token)`), and adjust
the PII logger call (`logger.info`) to avoid logging full email (e.g., log user
id or masked email) instead of `user.email`; keep references to
`create_refresh_token`, `create_access_token`, and the `refresh` function name
intact.
🪄 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
Run ID: 202e1d94-a5ac-4ced-a042-7809025332ac
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.env.examplepyproject.tomlsrc/api/v1/endpoints/auth.pysrc/api/v1/router.pysrc/core/config.pysrc/core/security.pysrc/db/__init__.pysrc/db/database.pysrc/db/models.pysrc/main.pysrc/models/auth_schema.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/api/v1/endpoints/auth.py (2)
101-119: Variablerefreshshadows the enclosing function name.On line 117,
refresh = create_refresh_token(...)shadows the functionasync def refresh(...). While this works, it's confusing and violates naming clarity. Consider renaming the local variable.♻️ Proposed fix
`@router.post`("/refresh", response_model=TokenResponse) async def refresh(request: RefreshRequest, db: Session = Depends(get_db)): """Issue a new access token using a valid refresh token.""" email = decode_refresh_token(request.refresh_token) if email is None: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid or expired refresh token.", ) user = db.query(User).filter(User.email == email).first() if user is None: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="User not found.", ) access = create_access_token(subject=user.email) - refresh = create_refresh_token(subject=user.email) + refresh_token = create_refresh_token(subject=user.email) logger.info("Token refreshed: id=%s", user.id) - return TokenResponse(access_token=access, refresh_token=refresh) + return TokenResponse(access_token=access, refresh_token=refresh_token)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v1/endpoints/auth.py` around lines 101 - 119, The local variable refresh in the async def refresh(...) function shadows the function name; rename the local variable (e.g., new_refresh_token or refresh_token) where create_refresh_token(...) is called and update its use in the return statement (TokenResponse) to avoid name collision while keeping create_access_token(...), decode_refresh_token(...), and the logger.info("Token refreshed: id=%s", user.id) references intact.
62-76: Consider handlingIntegrityErrorfor race-condition robustness.The email uniqueness check on line 62 has a TOCTOU gap before the commit on line 75. While the database's
unique=Trueconstraint onUser.email(insrc/db/models.py:15) prevents duplicate entries, the current code would surface a 500 error if two concurrent signups with the same email hit the race window.Wrapping the commit in a try/except for
IntegrityErrorwould provide a cleaner user experience:🛡️ Proposed improvement
+from sqlalchemy.exc import IntegrityError + `@router.post`( "/signup", response_model=UserResponse, status_code=status.HTTP_201_CREATED, ) async def signup(request: SignupRequest, db: Session = Depends(get_db)): """Register a new user.""" existing = db.query(User).filter(User.email == request.email).first() if existing: raise HTTPException( status_code=status.HTTP_409_CONFLICT, detail="Email already registered.", ) user = User( email=request.email, hashed_password=hash_password(request.password), nickname=request.nickname, ) db.add(user) - db.commit() - db.refresh(user) + try: + db.commit() + db.refresh(user) + except IntegrityError: + db.rollback() + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="Email already registered.", + ) logger.info("New user registered: id=%s", user.id) return user🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v1/endpoints/auth.py` around lines 62 - 76, There is a TOCTOU race between checking existing = db.query(User)... and db.commit() causing a 500 on unique constraint violation; wrap the transaction around db.add(user); db.commit() in a try/except that catches sqlalchemy.exc.IntegrityError, calls db.rollback(), and raises HTTPException(status_code=409, detail="Email already registered.") to surface a proper client error; ensure IntegrityError is imported and keep db.refresh(user) only on success (after the commit).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/api/v1/endpoints/auth.py`:
- Around line 101-119: The local variable refresh in the async def refresh(...)
function shadows the function name; rename the local variable (e.g.,
new_refresh_token or refresh_token) where create_refresh_token(...) is called
and update its use in the return statement (TokenResponse) to avoid name
collision while keeping create_access_token(...), decode_refresh_token(...), and
the logger.info("Token refreshed: id=%s", user.id) references intact.
- Around line 62-76: There is a TOCTOU race between checking existing =
db.query(User)... and db.commit() causing a 500 on unique constraint violation;
wrap the transaction around db.add(user); db.commit() in a try/except that
catches sqlalchemy.exc.IntegrityError, calls db.rollback(), and raises
HTTPException(status_code=409, detail="Email already registered.") to surface a
proper client error; ensure IntegrityError is imported and keep db.refresh(user)
only on success (after the commit).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c9066d8-e1c2-4db8-8379-cd0d300e4d82
📒 Files selected for processing (6)
.gitignorepyproject.tomlsrc/api/v1/endpoints/auth.pysrc/core/config.pysrc/db/database.pysrc/main.py
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- pyproject.toml
- src/core/config.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main.py
- src/db/database.py
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
1 participant
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.
어떤 변경사항인가요?
작업 상세 내용
src/db/database.py,src/db/models.py)src/core/security.py)POST /api/v1/auth/signup)POST /api/v1/auth/login) — access + refresh 토큰 발급POST /api/v1/auth/refresh)get_current_user의존성 주입 방식 인증 미들웨어체크리스트
관련 이슈
리뷰 포인트
type클레임으로 구분하여 토큰 혼용 방지SECRET_KEY기본값이 하드코딩되어 있으므로 배포 시 반드시 환경변수 설정 필요참고사항 및 스크린샷(선택)
없음
Summary by CodeRabbit
New Features
Chores