feat(auth): implement refresh token rotation and reuse detection#44
feat(auth): implement refresh token rotation and reuse detection#44aniebietafia merged 3 commits intomainfrom
Conversation
- Implemented `POST /api/v1/auth/refresh-token` endpoint for secure token rotation via HttpOnly cookies.
- Refactored `TokenStoreService` Redis architecture to use email-scoped keys (`refresh_token:{email}:{jti}`) to support efficient session revocation.
- Added `revoke_all_user_tokens` mechanism to instantly tear down an entire user session family if a stolen (already revoked) refresh token is replayed.
- Created `decode_refresh_token` wrapper returning a strictly typed `RefreshTokenClaims` schema to eliminate `Optional` type constraints from Mypy.
- Added 9 comprehensive integration tests in `test_auth_refresh.py` covering valid rotation, tampered tokens, hijacked sessions, and deactivated accounts.
- Resolved type-ignores in `aiokafka` integrations recognizing modern stubs.
Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 42 seconds. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (58)
📝 WalkthroughWalkthroughThis pull request implements a token rotation endpoint ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthEndpoint as Auth Endpoint
participant SecurityService
participant TokenStore
participant Database
participant Session
Client->>AuthEndpoint: POST /auth/refresh-token<br/>(refresh_token cookie)
AuthEndpoint->>AuthEndpoint: Extract refresh_token<br/>from HttpOnly cookie
alt Missing Cookie
AuthEndpoint-->>Client: 401 MISSING_REFRESH_TOKEN
else
AuthEndpoint->>SecurityService: decode_refresh_token(token)
SecurityService->>SecurityService: Verify JWT signature<br/>& expiry
alt Invalid/Expired
SecurityService-->>AuthEndpoint: ValueError
AuthEndpoint-->>Client: 401 INVALID_REFRESH_TOKEN
else
SecurityService-->>AuthEndpoint: RefreshTokenClaims<br/>(email, jti)
end
AuthEndpoint->>TokenStore: is_refresh_token_valid<br/>(email, jti)
TokenStore->>TokenStore: Check Redis<br/>refresh_token:{email}:{jti}
alt Token Not Found (Revoked)
TokenStore-->>AuthEndpoint: false
AuthEndpoint->>TokenStore: revoke_all_user_tokens(email)
TokenStore->>TokenStore: SCAN & DELETE all<br/>user tokens from Redis
AuthEndpoint-->>Client: 401 REFRESH_TOKEN_REUSE
else
TokenStore-->>AuthEndpoint: true
end
AuthEndpoint->>Database: Query User(email)
Database-->>AuthEndpoint: User record
alt Deactivated/Deleted Account
AuthEndpoint-->>Client: 403 ACCOUNT_DEACTIVATED
else
AuthEndpoint->>SecurityService: Generate new<br/>access_token + refresh_token
SecurityService-->>AuthEndpoint: New tokens<br/>with fresh JTIs
AuthEndpoint->>TokenStore: revoke_refresh_token<br/>(email, old_jti)
TokenStore->>TokenStore: DELETE old token<br/>from Redis
AuthEndpoint->>TokenStore: save_refresh_token<br/>(email, new_jti, ttl)
TokenStore->>TokenStore: SET new token<br/>in Redis with TTL
AuthEndpoint->>Client: 200 OK + new access_token<br/>+ new refresh_token cookie<br/>(HttpOnly, SameSite=Strict)
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from app.auth.models import ( | ||
| User, | ||
| VerificationToken, | ||
| ) # noqa: F401 — required for autogenerate |
Check notice
Code scanning / CodeQL
Unused import Note
Copilot Autofix
AI 27 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| from typing import Sequence, Union | ||
|
|
||
| from alembic import op | ||
| import sqlalchemy as sa |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 27 days ago
To fix the problem, remove the unused import so that every imported name is actually referenced in the file. This eliminates unnecessary dependencies and satisfies the static analysis rule.
Concretely, in alembic/versions/092f1a489931_add_meeting_model.py, delete the line import sqlalchemy as sa (line 12 in the snippet). No additional methods, imports, or definitions are required, since nothing in the file currently uses sa. If at a later time the upgrade or downgrade functions are filled in with SQLAlchemy constructs, the developer can re-add the import sqlalchemy as sa line as needed.
| @@ -9,7 +9,6 @@ | ||
| from typing import Sequence, Union | ||
|
|
||
| from alembic import op | ||
| import sqlalchemy as sa | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision: str = "092f1a489931" |
| import sqlalchemy as sa | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision: str = "092f1a489931" |
Check notice
Code scanning / CodeQL
Unused global variable Note
Copilot Autofix
AI 27 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision: str = "092f1a489931" | ||
| down_revision: Union[str, Sequence[str], None] = "7f066a8213a8" |
Check notice
Code scanning / CodeQL
Unused global variable Note
Copilot Autofix
AI 27 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| # revision identifiers, used by Alembic. | ||
| revision: str = "092f1a489931" | ||
| down_revision: Union[str, Sequence[str], None] = "7f066a8213a8" | ||
| branch_labels: Union[str, Sequence[str], None] = None |
Check notice
Code scanning / CodeQL
Unused global variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 27 days ago
To fix the problem, we should remove the unused global variable assignment if it has no side effects and is not required by external APIs. Here, branch_labels is assigned the literal None, which has no side effects, and in a typical Alembic migration this field is only necessary when branch labels are actually used. Removing the assignment does not change behavior when branch labels are not used.
Concretely, in alembic/versions/092f1a489931_add_meeting_model.py, delete the line that assigns branch_labels at the module level, keeping the other Alembic identifiers (revision, down_revision, depends_on) unchanged. No imports, methods, or additional definitions are needed.
| @@ -14,7 +14,6 @@ | ||
| # revision identifiers, used by Alembic. | ||
| revision: str = "092f1a489931" | ||
| down_revision: Union[str, Sequence[str], None] = "7f066a8213a8" | ||
| branch_labels: Union[str, Sequence[str], None] = None | ||
| depends_on: Union[str, Sequence[str], None] = None | ||
|
|
||
|
|
| revision: str = "092f1a489931" | ||
| down_revision: Union[str, Sequence[str], None] = "7f066a8213a8" | ||
| branch_labels: Union[str, Sequence[str], None] = None | ||
| depends_on: Union[str, Sequence[str], None] = None |
Check notice
Code scanning / CodeQL
Unused global variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 27 days ago
In general, unused global variables should either be removed (if they have no side effects and no external consumers) or renamed to clearly indicate that they are intentionally unused (for documentation or template purposes). Removing the left-hand side only is appropriate when the right-hand side has side effects that must be preserved; here the right-hand side is a literal None, so there are no side effects to worry about.
In this Alembic migration, depends_on is optional and is set to None, which is Alembic’s default value. To avoid changing existing functionality while keeping the template structure, the safest single change is to rename this variable to something that clearly indicates it is unused, such as _unused_depends_on. This preserves the information for future editors (they can see there is currently no dependency) and satisfies the static analysis rule that intentionally unused variables must have a special name. Concretely, in alembic/versions/092f1a489931_add_meeting_model.py, at line 18, change depends_on to _unused_depends_on, leaving the type annotation and value unchanged. No additional imports or helper methods are necessary.
| @@ -15,7 +15,7 @@ | ||
| revision: str = "092f1a489931" | ||
| down_revision: Union[str, Sequence[str], None] = "7f066a8213a8" | ||
| branch_labels: Union[str, Sequence[str], None] = None | ||
| depends_on: Union[str, Sequence[str], None] = None | ||
| _unused_depends_on: Union[str, Sequence[str], None] = None | ||
|
|
||
|
|
||
| def upgrade() -> None: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/main.py (1)
59-59:⚠️ Potential issue | 🔴 CriticalAPI base-path mismatch: configured as
/routers/v1but tests expect/api/v1.The routing chain is:
settings.API_V1_STR = "/routers/v1"(app/core/config.py:19)auth_routerhasprefix="/auth"(app/auth/router.py:26)api_routerincludesauth_routerwith no additional prefix, then mounted withsettings.API_V1_STRThis makes the effective route
/routers/v1/auth/*, but all 40+ tests intests/test_auth/request/api/v1/auth/*, causing 404 responses. Either the config setting must be changed to"/api/v1"or all tests must be updated to use/routers/v1paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/main.py` at line 59, The API base path is misconfigured: settings.API_V1_STR is set to "/routers/v1" while routes (auth_router included into api_router and mounted via app.include_router(api_router, prefix=settings.API_V1_STR)) are expected by tests at "/api/v1"; update the configuration by changing settings.API_V1_STR to "/api/v1" (so the effective route becomes "/api/v1/auth/*") and keep auth_router's prefix="/auth" and the app.include_router(api_router, prefix=settings.API_V1_STR) usage unchanged.tests/test_auth/test_auth_login.py (1)
221-223:⚠️ Potential issue | 🔴 CriticalUpdate test endpoints to use correct API route prefix.
The tests use
/api/v1/auth/loginbut the configured route prefix inapp/core/config.pyis/routers/v1. This mismatch causes all requests to return 404 errors.Update all test URLs from
/api/v1/auth/loginto/routers/v1/auth/loginat lines 221, 238, 252, 269, 288, 302, 316, 319, 336, 358, 386, 401–402, 407–408, 425–426, and 431–432.Example fix
- response = client.post( - "/api/v1/auth/login", + response = client.post( + "/routers/v1/auth/login",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth/test_auth_login.py` around lines 221 - 223, Tests are calling the wrong route prefix; update all test request URLs that use "/api/v1/auth/..." to the configured prefix "/routers/v1/auth/..." so requests hit the real routes; search for occurrences in tests/test_auth/test_auth_login.py (examples around the client.post calls shown with json={"email": "user@example.com", "password": "MyStr0ngP@ss!"}) and replace "/api/v1/" with "/routers/v1/" for every listed occurrence (lines referenced in the review: 221, 238, 252, 269, 288, 302, 316, 319, 336, 358, 386, 401–402, 407–408, 425–426, 431–432).
🧹 Nitpick comments (6)
pyproject.toml (1)
49-49: Avoid globally disablingB008; scope the suppression instead.Turning off
B008repo-wide can hide real default-argument bugs. Prefer keepingB008enabled and using targeted# noqa: B008(or per-file ignore) only where framework patterns require it.Suggested config change
[tool.ruff.lint] select = ["B", "E", "F", "I", "W", "C90", "UP", "ASYNC", "PT", "ARG", "PTH", "SIM", "PLE", "PLW", "RUF"] -ignore = ["B008"] +ignore = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 49, Remove the global ignore for B008 in pyproject.toml (the line ignore = ["B008"]) and instead re-enable the B008 check repository-wide; then add targeted suppressions (use per-file configuration or inline comments like "# noqa: B008") only in the specific modules or function definitions that legitimately require mutable default arguments (e.g., the files/functions following your framework patterns), so B008 remains active except where explicitly and locally suppressed.app/auth/router.py (1)
199-206: Move import to top of file.The lazy import of
UnauthorizedExceptioninside the function body is unconventional. Since this exception is used every time a cookie is missing, move it to the module-level imports for clarity.Proposed fix
At the top of the file (after line 22):
+from app.core.exceptions import UnauthorizedExceptionThen in the function:
if not raw_token: - from app.core.exceptions import UnauthorizedException - raise UnauthorizedException( code="MISSING_REFRESH_TOKEN", message="No refresh token provided.", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/auth/router.py` around lines 199 - 206, The code lazily imports UnauthorizedException inside the handler where raw_token = request.cookies.get("refresh_token") leading to unconventional in-function imports; move the import of UnauthorizedException to the module-level imports at the top of app/auth/router.py (alongside other imports) and remove the inner "from app.core.exceptions import UnauthorizedException" inside the function so the raise UnauthorizedException(...) uses the top-level symbol.app/auth/verification.py (1)
86-95: Consider bulk delete for efficiency.The current approach fetches all unexpired tokens into memory and deletes them one by one. For users with multiple tokens, a bulk delete would be more efficient:
Proposed optimization
- statement_del = select(VerificationToken).where( - VerificationToken.user_id == user.id, - VerificationToken.expires_at >= now, - ) - tokens = self.db.execute(statement_del).scalars().all() - for t in tokens: - self.db.delete(t) - if tokens: - self.db.commit() + from sqlalchemy import delete + + delete_stmt = delete(VerificationToken).where( + VerificationToken.user_id == user.id, + VerificationToken.expires_at >= now, + ) + self.db.execute(delete_stmt) + self.db.commit()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/auth/verification.py` around lines 86 - 95, Replace the fetch-and-loop deletion with a single bulk DELETE: instead of executing the SELECT stored in statement_del, build and execute a delete() against VerificationToken with the same filters (VerificationToken.user_id == user.id and VerificationToken.expires_at >= now), run it via self.db.execute(...) and call self.db.commit() if the delete affected rows (use the returned result.rowcount to decide). Update the code that currently uses tokens = self.db.execute(statement_del).scalars().all() and the for t in tokens loop to use this bulk delete approach.tests/test_auth/test_auth_login.py (1)
34-100: ExtractFakeRedisandFakePipelineto a shared test utility.These test doubles are duplicated verbatim in
test_auth_refresh.py. Consider moving them totests/conftest.pyor a dedicatedtests/fakes.pymodule to reduce duplication and ensure consistent behavior across test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth/test_auth_login.py` around lines 34 - 100, Move the FakeRedis and FakePipeline classes out of tests/test_auth/test_auth_login.py into a shared test utility (e.g., tests/conftest.py or tests/fakes.py), keeping all methods and signatures (FakeRedis: __init__, set, get, delete, exists, incr, scan, pipeline, reset; FakePipeline: __init__, delete, execute) intact; then update test_auth_login.py and the duplicate file (test_auth_refresh.py) to import the shared FakeRedis and FakePipeline instead of defining them inline so both tests use the same, centralized test doubles.tests/test_auth/test_auth_refresh.py (2)
312-335: Consider adding test for expired refresh token.The test suite covers garbage tokens and access tokens misused as refresh tokens, but doesn't explicitly test an expired (but otherwise valid) refresh token. This edge case is worth covering to ensure proper 401 response with
INVALID_REFRESH_TOKEN.Would you like me to generate a test case for expired refresh tokens?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth/test_auth_refresh.py` around lines 312 - 335, Add a new test in the TestRefreshTokenInvalid class that seeds a user with _seed_user(db_session), then creates a refresh token that is already expired (e.g., use _SECURITY.create_refresh_token with an expiry in the past or create then tamper the exp claim) and POSTs it to _URL as the "refresh_token" cookie; assert the response.status_code == 401 and response.json()["code"] == "INVALID_REFRESH_TOKEN". Reference TestRefreshTokenInvalid, _SECURITY.create_refresh_token (or the helper that builds tokens), _seed_user, and _URL when locating where to add the test.
234-238: Useasyncio.run()or pytest-asyncio for modern async test patterns.
asyncio.get_event_loop().run_until_complete()is not deprecated, butasyncio.run()is the recommended modern approach for running async code from synchronous contexts (Python 3.7+). For synchronous tests that need to call async methods, consider:
asyncio.run()(simpler, cleaner), or- Convert tests to async and use
pytest-asyncioExample using asyncio.run()
- import asyncio - - asyncio.get_event_loop().run_until_complete( - token_store.save_refresh_token(email=email, jti=jti, ttl_seconds=ttl) - ) + import asyncio + + asyncio.run( + token_store.save_refresh_token(email=email, jti=jti, ttl_seconds=ttl) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth/test_auth_refresh.py` around lines 234 - 238, Replace the legacy asyncio.get_event_loop().run_until_complete call with asyncio.run to execute the coroutine; specifically call asyncio.run(token_store.save_refresh_token(email=email, jti=jti, ttl_seconds=ttl)) in place of the current run_until_complete usage, or alternatively convert the test to an async test using pytest-asyncio and await token_store.save_refresh_token(email=email, jti=jti, ttl_seconds=ttl) inside the async test function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/issue-labeler.yml:
- Around line 36-37: The "api" label's regex was changed to
'(routers|endpoint|rest|controller)' and now no longer matches "api" or
"/api/v1"; update the api label pattern (the mapping keyed by api in the
issue-labeler config) to include both "api" and "routers" by adding "api" into
the alternation so it matches terms like "api" and "/api/v1" as well as
"routers" (e.g. change the pattern to include api alongside routers, endpoint,
rest, controller).
In `@alembic/versions/092f1a489931_add_meeting_model.py`:
- Around line 21-32: The revision currently does nothing: replace the pass in
upgrade() with a proper op.create_table call to create the meeting table (e.g.,
include at minimum an Integer primary key column "id", appropriate columns like
"title" (String), "start_time"/"end_time" (DateTime), audit timestamps and any
FK(s) if needed) and add the corresponding op.drop_table("meeting") in
downgrade(); ensure you import and use alembic.op and sqlalchemy types and match
your project's naming (refer to upgrade(), downgrade(), op.create_table and
op.drop_table to locate where to add these DDL changes).
In `@app/auth/service.py`:
- Around line 129-141: The login flow lacks the same active-account guard used
in refresh_token(), causing deactivated users to succeed at login but fail on
refresh; update the login function to check user.is_active after the existing
is_verified and deleted_at guards and raise a ForbiddenException (e.g.,
code="ACCOUNT_INACTIVE", message="This account is inactive.") when not active so
behavior matches refresh_token().
- Around line 170-177: The forgot_password flow is incomplete:
forgot_password(email, frontend_base_url) (which calls get_user_by_email)
generates a UUID token and embeds it in a link but never persists the token nor
provides a backend to redeem it; either implement token storage and a reset
endpoint or remove the method. Fix by persisting the token (e.g., in Redis or
DB) keyed to user.id with an expiry when generating the UUID in forgot_password,
send the same stored token in the reset link, and implement a POST
/reset-password handler (accepting user_id, token, new_password) that validates
the stored token (similar to verify-email token validation), updates the user
password, and invalidates/deletes the token; alternatively, if password reset is
out of scope, delete the forgot_password method and any email-sending that
references reset-password to avoid dangling links.
In `@app/auth/token_store.py`:
- Around line 95-99: The warning logs raw email when revoking tokens
(logger.warning in token_store.py using keys_to_delete and email); sanitize the
dynamic email field before logging to prevent log-injection and unsafe content —
implement or call a sanitizer (e.g., strip/control-char removal, newline
replacement, and optional masking/truncation) to produce sanitized_email and use
that in the logger.warning call instead of the raw email while keeping
len(keys_to_delete) unchanged; ensure the sanitizer is applied wherever email is
logged during revocation.
In `@app/core/config.py`:
- Line 19: The API prefix constant API_V1_STR in app/core/config.py is set to
"/routers/v1" which breaks the expected contract; change the value of API_V1_STR
to "/api/v1" so all route registration and auth cookie path scope use the
correct prefix (update any references to API_V1_STR if necessary to pick up the
new value).
In `@app/core/security.py`:
- Around line 135-136: Update the stale docstring reference that currently
points to app.schemas.user.RefreshTokenClaims: change it to
app.auth.schemas.RefreshTokenClaims in the docstring inside app/core/security.py
so the :class: cross-reference matches the new module layout and correctly links
the RefreshTokenClaims model.
In `@app/services/email_consumer.py`:
- Line 67: The Mailgun HTTP Basic Auth username is incorrect: replace the tuple
auth=("routers", settings.MAILGUN_API_KEY) with auth=("api",
settings.MAILGUN_API_KEY) where the Mailgun request is made (look for the
request in email_consumer.py — e.g., the function that posts to
/v3/{domain}/messages or any send_email/send_mail method) so the API uses the
required username "api" and the existing MAILGUN_API_KEY as the password.
In `@docker-compose.yml`:
- Line 42: The healthcheck command references a non-existent script name; update
the service healthcheck test that currently runs
"/opt/kafka/bin/kafka-broker-routers-versions.sh --bootstrap-server
localhost:9092" to use the correct Kafka script
"/opt/kafka/bin/kafka-broker-api-versions.sh --bootstrap-server localhost:9092"
(keep the CMD-SHELL wrapper and same bootstrap-server argument) so the Kafka
container can report healthy and allow dependent services like kafka-ui to
start.
In `@tests/test_auth/test_auth_refresh.py`:
- Line 185: The test constant _URL in tests/test_auth/test_auth_refresh.py is
set to "/api/v1/auth/refresh-token" but the test docstring and actual route
expect "/routers/v1/auth/refresh-token"; update the _URL value to
"/routers/v1/auth/refresh-token" so the test hits the correct endpoint (verify
other tests or fixtures don’t rely on the old path and update them if they do).
---
Outside diff comments:
In `@app/main.py`:
- Line 59: The API base path is misconfigured: settings.API_V1_STR is set to
"/routers/v1" while routes (auth_router included into api_router and mounted via
app.include_router(api_router, prefix=settings.API_V1_STR)) are expected by
tests at "/api/v1"; update the configuration by changing settings.API_V1_STR to
"/api/v1" (so the effective route becomes "/api/v1/auth/*") and keep
auth_router's prefix="/auth" and the app.include_router(api_router,
prefix=settings.API_V1_STR) usage unchanged.
In `@tests/test_auth/test_auth_login.py`:
- Around line 221-223: Tests are calling the wrong route prefix; update all test
request URLs that use "/api/v1/auth/..." to the configured prefix
"/routers/v1/auth/..." so requests hit the real routes; search for occurrences
in tests/test_auth/test_auth_login.py (examples around the client.post calls
shown with json={"email": "user@example.com", "password": "MyStr0ngP@ss!"}) and
replace "/api/v1/" with "/routers/v1/" for every listed occurrence (lines
referenced in the review: 221, 238, 252, 269, 288, 302, 316, 319, 336, 358, 386,
401–402, 407–408, 425–426, 431–432).
---
Nitpick comments:
In `@app/auth/router.py`:
- Around line 199-206: The code lazily imports UnauthorizedException inside the
handler where raw_token = request.cookies.get("refresh_token") leading to
unconventional in-function imports; move the import of UnauthorizedException to
the module-level imports at the top of app/auth/router.py (alongside other
imports) and remove the inner "from app.core.exceptions import
UnauthorizedException" inside the function so the raise
UnauthorizedException(...) uses the top-level symbol.
In `@app/auth/verification.py`:
- Around line 86-95: Replace the fetch-and-loop deletion with a single bulk
DELETE: instead of executing the SELECT stored in statement_del, build and
execute a delete() against VerificationToken with the same filters
(VerificationToken.user_id == user.id and VerificationToken.expires_at >= now),
run it via self.db.execute(...) and call self.db.commit() if the delete affected
rows (use the returned result.rowcount to decide). Update the code that
currently uses tokens = self.db.execute(statement_del).scalars().all() and the
for t in tokens loop to use this bulk delete approach.
In `@pyproject.toml`:
- Line 49: Remove the global ignore for B008 in pyproject.toml (the line ignore
= ["B008"]) and instead re-enable the B008 check repository-wide; then add
targeted suppressions (use per-file configuration or inline comments like "#
noqa: B008") only in the specific modules or function definitions that
legitimately require mutable default arguments (e.g., the files/functions
following your framework patterns), so B008 remains active except where
explicitly and locally suppressed.
In `@tests/test_auth/test_auth_login.py`:
- Around line 34-100: Move the FakeRedis and FakePipeline classes out of
tests/test_auth/test_auth_login.py into a shared test utility (e.g.,
tests/conftest.py or tests/fakes.py), keeping all methods and signatures
(FakeRedis: __init__, set, get, delete, exists, incr, scan, pipeline, reset;
FakePipeline: __init__, delete, execute) intact; then update test_auth_login.py
and the duplicate file (test_auth_refresh.py) to import the shared FakeRedis and
FakePipeline instead of defining them inline so both tests use the same,
centralized test doubles.
In `@tests/test_auth/test_auth_refresh.py`:
- Around line 312-335: Add a new test in the TestRefreshTokenInvalid class that
seeds a user with _seed_user(db_session), then creates a refresh token that is
already expired (e.g., use _SECURITY.create_refresh_token with an expiry in the
past or create then tamper the exp claim) and POSTs it to _URL as the
"refresh_token" cookie; assert the response.status_code == 401 and
response.json()["code"] == "INVALID_REFRESH_TOKEN". Reference
TestRefreshTokenInvalid, _SECURITY.create_refresh_token (or the helper that
builds tokens), _seed_user, and _URL when locating where to add the test.
- Around line 234-238: Replace the legacy
asyncio.get_event_loop().run_until_complete call with asyncio.run to execute the
coroutine; specifically call
asyncio.run(token_store.save_refresh_token(email=email, jti=jti,
ttl_seconds=ttl)) in place of the current run_until_complete usage, or
alternatively convert the test to an async test using pytest-asyncio and await
token_store.save_refresh_token(email=email, jti=jti, ttl_seconds=ttl) inside the
async test function.
🪄 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: 9b47e604-edeb-474b-a312-9991caf768a9
📒 Files selected for processing (62)
.github/issue-labeler.ymlREADME.mdalembic/env.pyalembic/versions/092f1a489931_add_meeting_model.pyalembic_output.txtapp/api/v1/__init__.pyapp/api/v1/endpoints/auth.pyapp/auth/__init__.pyapp/auth/account_lockout.pyapp/auth/constants.pyapp/auth/dependencies.pyapp/auth/models.pyapp/auth/router.pyapp/auth/schemas.pyapp/auth/service.pyapp/auth/token_store.pyapp/auth/utils.pyapp/auth/verification.pyapp/core/config.pyapp/core/security.pyapp/crud/__init__.pyapp/crud/auth_verification_api.mdapp/crud/user/user.pyapp/crud/verification_token.pyapp/external_services/__init__.pyapp/external_services/cloudinary/__init__.pyapp/external_services/mailgun/__init__.pyapp/main.pyapp/models/__init__.pyapp/models/base.pyapp/models/user.pyapp/models/verification_token.pyapp/routers/__init__.pyapp/routers/api.pyapp/schemas/auth.pyapp/services/email_consumer.pyapp/services/token_store.pyapp/user/__init__.pyapp/user/config.pyapp/user/constants.pyapp/user/dependencies.pyapp/user/exceptions.pyapp/user/models.pyapp/user/router.pyapp/user/schemas.pyapp/user/service.pyapp/user/utils.pyapp/utils/__init__.pyapp/utils/authentication.pyapp/utils/validation.pydocker-compose.ymlfail_log.txtpyproject.tomlrequirements/base.txtrequirements/dev.txtrequirements/prod.txttests/test_auth/test_auth_login.pytests/test_auth/test_auth_refresh.pytests/test_auth/test_auth_signup.pytests/test_auth/test_email_verification.pytests/test_auth/test_schemas_user.pytests/test_auth/test_verification_token_crud.py
💤 Files with no reviewable changes (12)
- app/crud/init.py
- alembic_output.txt
- app/api/v1/init.py
- app/crud/auth_verification_api.md
- fail_log.txt
- app/models/verification_token.py
- app/crud/user/user.py
- app/models/user.py
- app/crud/verification_token.py
- app/services/token_store.py
- app/schemas/auth.py
- app/api/v1/endpoints/auth.py
| def upgrade() -> None: | ||
| """Upgrade schema.""" | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| pass | ||
| # ### end Alembic commands ### | ||
|
|
||
|
|
||
| def downgrade() -> None: | ||
| """Downgrade schema.""" | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| pass | ||
| # ### end Alembic commands ### |
There was a problem hiding this comment.
Migration is a no-op despite claiming to add a model.
Line 21–Line 32 contains only pass, so this revision applies no schema change. If a meeting model is intended, add explicit DDL; if not, remove this revision to avoid misleading migration history.
Would you like me to draft the op.create_table(...) / op.drop_table(...) skeleton for this migration?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@alembic/versions/092f1a489931_add_meeting_model.py` around lines 21 - 32, The
revision currently does nothing: replace the pass in upgrade() with a proper
op.create_table call to create the meeting table (e.g., include at minimum an
Integer primary key column "id", appropriate columns like "title" (String),
"start_time"/"end_time" (DateTime), audit timestamps and any FK(s) if needed)
and add the corresponding op.drop_table("meeting") in downgrade(); ensure you
import and use alembic.op and sqlalchemy types and match your project's naming
(refer to upgrade(), downgrade(), op.create_table and op.drop_table to locate
where to add these DDL changes).
| # Guard: email verified? | ||
| if not user.is_verified: | ||
| raise ForbiddenException( | ||
| code="EMAIL_NOT_VERIFIED", | ||
| message="Please verify your email before logging in.", | ||
| ) | ||
|
|
||
| # Guard: soft-deleted? | ||
| if user.deleted_at is not None: | ||
| raise ForbiddenException( | ||
| code="ACCOUNT_DELETED", | ||
| message="This account has been deleted.", | ||
| ) |
There was a problem hiding this comment.
Missing is_active check creates inconsistency with refresh_token().
The login method checks is_verified and deleted_at but does not check is_active. However, refresh_token() at line 220 rejects users where not user.is_active. This inconsistency allows a deactivated user to log in successfully but fail on their first token refresh.
Consider adding the is_active check here for consistent behavior:
Proposed fix
# Guard: soft-deleted?
if user.deleted_at is not None:
raise ForbiddenException(
code="ACCOUNT_DELETED",
message="This account has been deleted.",
)
+
+ # Guard: deactivated?
+ if not user.is_active:
+ raise ForbiddenException(
+ code="ACCOUNT_DEACTIVATED",
+ message="This account has been deactivated.",
+ )
# Reset failed-login counter on success🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/auth/service.py` around lines 129 - 141, The login flow lacks the same
active-account guard used in refresh_token(), causing deactivated users to
succeed at login but fail on refresh; update the login function to check
user.is_active after the existing is_verified and deleted_at guards and raise a
ForbiddenException (e.g., code="ACCOUNT_INACTIVE", message="This account is
inactive.") when not active so behavior matches refresh_token().
| async def forgot_password(self, email: str, frontend_base_url: str) -> None: | ||
| user = self.get_user_by_email(email) | ||
| if not user: | ||
| return | ||
|
|
||
| reset_link = ( | ||
| f"{frontend_base_url}/reset-password?user={user.id}&token={uuid.uuid4()}" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a password reset endpoint that validates the token
rg -n "reset-password|reset_password" --type py -g '!*test*' -C 3
# Check if there's a password reset token storage mechanism
rg -n "reset.*token|password.*token" --type py -g '!*test*' -C 2Repository: Brints/FluentMeet
Length of output: 1520
🏁 Script executed:
# Read the complete forgot_password method
head -n 195 app/auth/service.py | tail -n 30Repository: Brints/FluentMeet
Length of output: 1042
🏁 Script executed:
# Search for password reset endpoint in router files
find app -name "*.py" -type f | xargs grep -l "reset.*password\|password.*reset" | grep -i routerRepository: Brints/FluentMeet
Length of output: 79
🏁 Script executed:
# Look for token validation or reset endpoint implementation
rg -n "@.*post|@.*put|@.*patch" app/auth/router.py -A 5 | head -n 100Repository: Brints/FluentMeet
Length of output: 1032
🏁 Script executed:
# Check for any Redis or database storage of reset tokens
rg -n "redis|cache|token.*store|store.*token" --type py | grep -i resetRepository: Brints/FluentMeet
Length of output: 43
🏁 Script executed:
# Look for all POST endpoints in auth router
rg -n "^@router\." app/auth/router.py -A 10 | grep -E "post|put|patch" -A 8Repository: Brints/FluentMeet
Length of output: 1911
🏁 Script executed:
# Search for any endpoint that might handle the actual password reset
rg -n "reset|password" app/auth/router.py -B 2 -A 8Repository: Brints/FluentMeet
Length of output: 1410
🏁 Script executed:
# Check the complete auth router file to see all endpoints
cat -n app/auth/router.py | head -n 250Repository: Brints/FluentMeet
Length of output: 8263
🏁 Script executed:
# Look for token validation in the codebase
rg -n "uuid.*valid|valid.*uuid|token.*valid" --type pyRepository: Brints/FluentMeet
Length of output: 1437
🏁 Script executed:
# Check if there's a reset-password endpoint in other routers
find app -name "*.py" -type f | xargs grep -l "reset.*password\|password.*reset" | xargs basename -aRepository: Brints/FluentMeet
Length of output: 91
🏁 Script executed:
# List all routers to ensure we haven't missed any
find app -name "*router*.py" -o -name "*route*.py" -type fRepository: Brints/FluentMeet
Length of output: 98
🏁 Script executed:
# Check user router for password reset endpoint
rg -n "reset|password" app/user/router.py -B 2 -A 8Repository: Brints/FluentMeet
Length of output: 43
🏁 Script executed:
# Double-check: search entire app for any endpoint that accepts reset token
rg -n "token.*query|Query.*token" --type py app/ -A 3Repository: Brints/FluentMeet
Length of output: 43
🏁 Script executed:
# List all endpoints in auth router to be absolutely certain
rg -n "^@router\." app/auth/router.py | cut -d: -f2-Repository: Brints/FluentMeet
Length of output: 143
🏁 Script executed:
# Check if there's any mention of password reset validation in the entire codebase
rg -n "reset.*valid|validate.*reset" --type pyRepository: Brints/FluentMeet
Length of output: 43
🏁 Script executed:
# Final check: verify all endpoints and their HTTP methods
cat -n app/auth/router.py | grep -E "@router\.|^[0-9]+-async def|^[0-9]+-def " | head -n 30Repository: Brints/FluentMeet
Length of output: 185
Password reset feature is non-functional: token is not stored and no reset endpoint exists.
The UUID reset token at line 176 is embedded in the email link but is never persisted (no Redis, database, or cache storage). More critically, there is no backend endpoint to handle password reset form submission—users can receive the email but have nowhere to submit their new password.
Compare with /verify-email (which validates stored tokens) or refresh token rotation (which uses Redis): password reset is incomplete. Either implement token storage and a reset endpoint (e.g., POST /reset-password accepting user_id, token, and new_password), or if password reset is out of scope, remove the forgot_password method entirely to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/auth/service.py` around lines 170 - 177, The forgot_password flow is
incomplete: forgot_password(email, frontend_base_url) (which calls
get_user_by_email) generates a UUID token and embeds it in a link but never
persists the token nor provides a backend to redeem it; either implement token
storage and a reset endpoint or remove the method. Fix by persisting the token
(e.g., in Redis or DB) keyed to user.id with an expiry when generating the UUID
in forgot_password, send the same stored token in the reset link, and implement
a POST /reset-password handler (accepting user_id, token, new_password) that
validates the stored token (similar to verify-email token validation), updates
the user password, and invalidates/deletes the token; alternatively, if password
reset is out of scope, delete the forgot_password method and any email-sending
that references reset-password to avoid dangling links.
Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_auth/test_auth_refresh.py (1)
185-185:⚠️ Potential issue | 🔴 CriticalFix stale refresh endpoint URL constant (currently causing suite-wide 404s).
Line 185 points tests to a route that is not mounted in the current app config, so the refresh handler never executes. This is the root cause of the 404 cascade (including the downstream “old JTI still valid” failure).
Proposed fix
-_URL = "/api/v1/auth/refresh-token" +_URL = "/routers/v1/auth/refresh-token"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth/test_auth_refresh.py` at line 185, The test constant _URL points at a stale route and must be updated to the app's actual refresh endpoint: check the live route mounted by the auth refresh handler (e.g., inspect the app/router or the auth route declarations) and replace the _URL value in tests/test_auth/test_auth_refresh.py with that exact path so the refresh handler is invoked (ensure the string matches the handler's registered path exactly).
🧹 Nitpick comments (1)
tests/test_auth/test_auth_refresh.py (1)
268-272: Strengthen cookie assertions to cover required security attributes.The test currently checks
HttpOnlyandSameSite, but it should also assertSecure,Path, andMax-Ageso regressions against the refresh-cookie contract are caught.Suggested test enhancement
set_cookie = response.headers.get("set-cookie", "") assert "refresh_token=" in set_cookie assert "HttpOnly" in set_cookie assert "SameSite=strict" in set_cookie + assert "Secure" in set_cookie + assert "Max-Age=604800" in set_cookie + expected_cookie_path = _URL.removesuffix("/refresh-token") + assert f"Path={expected_cookie_path}" in set_cookie🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth/test_auth_refresh.py` around lines 268 - 272, Extend the cookie assertions in tests/test_auth/test_auth_refresh.py where set_cookie is extracted (set_cookie = response.headers.get("set-cookie", "")) to also assert the required security attributes: check that "Secure" is present, that the cookie exposes a path (e.g. "Path=/"), and that it includes an expiration directive by asserting "Max-Age=" appears; use the existing set_cookie string (and consider case-insensitive matching if needed) rather than changing response access code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_auth/test_auth_refresh.py`:
- Line 185: The test constant _URL points at a stale route and must be updated
to the app's actual refresh endpoint: check the live route mounted by the auth
refresh handler (e.g., inspect the app/router or the auth route declarations)
and replace the _URL value in tests/test_auth/test_auth_refresh.py with that
exact path so the refresh handler is invoked (ensure the string matches the
handler's registered path exactly).
---
Nitpick comments:
In `@tests/test_auth/test_auth_refresh.py`:
- Around line 268-272: Extend the cookie assertions in
tests/test_auth/test_auth_refresh.py where set_cookie is extracted (set_cookie =
response.headers.get("set-cookie", "")) to also assert the required security
attributes: check that "Secure" is present, that the cookie exposes a path (e.g.
"Path=/"), and that it includes an expiration directive by asserting "Max-Age="
appears; use the existing set_cookie string (and consider case-insensitive
matching if needed) rather than changing response access code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb0d8f76-b4d1-4f67-b9ee-056a5c323b98
📒 Files selected for processing (3)
app/auth/token_store.pyapp/core/security.pytests/test_auth/test_auth_refresh.py
aniebietafia
force-pushed
the
feat/auth-refresh-token
branch
from
1ddeda3 to
19df2dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
app/services/email_consumer.py (1)
67-67:⚠️ Potential issue | 🔴 CriticalUse Mailgun’s required Basic Auth username (
api) on Line 67.
auth=("routers", settings.MAILGUN_API_KEY)will fail against Mailgun/v3/{domain}/messages; the username must beapi.Suggested fix
- auth=("routers", settings.MAILGUN_API_KEY), + auth=("api", settings.MAILGUN_API_KEY),For Mailgun API POST /v3/{domain}/messages, what HTTP Basic Auth username is required?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/email_consumer.py` at line 67, The Mailgun API call in email_consumer.py uses the wrong Basic Auth username; change the auth tuple used when sending messages (currently auth=("routers", settings.MAILGUN_API_KEY)) to use the required username "api" so it reads auth=("api", settings.MAILGUN_API_KEY); update the auth tuple wherever send/post to Mailgun is performed (look for the auth= argument in the Mailgun POST call) and run tests or a quick send to verify the request is accepted.app/auth/service.py (2)
136-145:⚠️ Potential issue | 🟠 MajorReject inactive users during login too.
refresh_token()already blocksnot user.is_active, butlogin()still issues fresh tokens after the deleted check. A deactivated account can therefore authenticate successfully and keep using the new access token until it expires.Suggested fix
if user.deleted_at is not None: raise ForbiddenException( code="ACCOUNT_DELETED", message="This account has been deleted.", ) + + if not user.is_active: + raise ForbiddenException( + code="ACCOUNT_DEACTIVATED", + message="This account has been deactivated.", + ) # Reset failed-login counter on success🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/auth/service.py` around lines 136 - 145, In login(), add the same active-account guard used in refresh_token(): after retrieving the user and before resetting attempts or issuing tokens, check if not user.is_active and raise a ForbiddenException (e.g., code "ACCOUNT_INACTIVE", message "This account is deactivated.") so deactivated accounts cannot obtain new tokens; ensure this check is placed alongside the existing deleted_at check and occurs before calling lockout_svc.reset_attempts(email) and token creation.
170-177:⚠️ Potential issue | 🔴 CriticalPersist the password-reset token before emailing it.
The UUID in
reset_linkis generated and discarded. Without storing it (or a hash of it) with an expiry, a later reset handler has nothing authoritative to validate, so this flow cannot be completed safely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/auth/service.py` around lines 170 - 177, In forgot_password (service.py) you currently generate a UUID for the reset_link but never persist it; change the flow to generate a cryptographically secure token (e.g., secrets.token_urlsafe or uuid4), store a hashed version plus an expiry timestamp on the user record or a dedicated PasswordResetToken entity (fields like reset_password_token_hash and reset_password_expires), persist the change (save/commit) before sending the email, and invalidate any previous tokens for that user; keep using the plain token in the emailed link but ensure the reset handler (e.g., the function that handles reset-password) validates against the stored hash and checks expiry.
🧹 Nitpick comments (1)
app/core/security.py (1)
14-23: Reasonable workaround for passlib/bcrypt compatibility.The shim for
bcrypt.__about__.__version__addresses a known issue with passlib 1.7.4 not recognizing bcrypt 4.0+ versions. The try/except pattern safely injects the stub only when needed.Consider adding a brief comment linking to the upstream issue or noting when this workaround can be removed (e.g., when passlib releases a fix).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/core/security.py` around lines 14 - 23, Add a short explanatory comment above the bcrypt shim that references the upstream Passlib/bcrypt compatibility issue and states the removal condition; specifically, mention passlib 1.7.4 and bcrypt 4.0+ incompatibility, include a URL or issue number to the upstream bugtracker, and note "remove when passlib fixed (>= version X or PR/issue closed)"; place the comment next to the try/except and reference the injected symbols bcrypt.__about__ and the _BcryptAbout stub so future maintainers know why the shim exists and when it can be removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@alembic/versions/7f066a8213a8_change_user_id_to_uuid.py`:
- Around line 56-72: The downgrade() is incomplete: before altering types back
to INTEGER you must drop the foreign-key constraint on
verification_tokens.user_id (the same FK removed in upgrade()), then alter
verification_tokens.user_id and users.id back to sa.INTEGER(), and restore the
integer sequence/default on users.id (recreate the users_id sequence if needed
and set users.id server_default to nextval('users_id_seq'::regclass) or
equivalent). After the column type changes, re-create the FK constraint linking
verification_tokens.user_id to users.id. Update the downgrade() function to
perform these steps in this order so it mirrors the upgrade() logic.
In `@app/api/v1/endpoints/auth.py`:
- Around line 489-491: The calls to token_store.is_refresh_token_valid(...) and
token_store.revoke_all_user_tokens(...) use a keyword argument email= but the
TokenStoreService methods likely don't accept that name; update the
TokenStoreService method signatures (is_refresh_token_valid and
revoke_all_user_tokens) to accept email as a keyword parameter (e.g., def
is_refresh_token_valid(self, *, email: str, jti: str) / def
revoke_all_user_tokens(self, *, email: str)) or modify the calls to pass email
as a positional argument (token_store.is_refresh_token_valid(email, old_jti) /
token_store.revoke_all_user_tokens(email)) so the parameter names and calling
style match between the caller and the TokenStoreService implementation.
- Around line 365-368: The import path and method signatures for the token store
are wrong: replace the import of TokenStoreService/get_token_store_service with
the correct module (app.auth.token_store) and then fix all calls that pass email
to token store methods (e.g., save_refresh_token, revoke_refresh_token,
is_refresh_token_valid) by removing the email argument to match their signatures
(save_refresh_token(jti, ttl_seconds), revoke_refresh_token(jti),
is_refresh_token_valid(jti)); also remove or implement any nonexistent method
calls such as revoke_all_user_tokens (either call an existing API or add a
correctly named method to TokenStoreService) and update usages at the identified
call sites (the token_store variable and affected lines like the
save_refresh_token and revoke calls).
In `@app/auth/router.py`:
- Around line 199-206: The inline import of UnauthorizedException inside the
function should be moved to module-level imports: add "from app.core.exceptions
import UnauthorizedException" to the top of app/auth/router.py and remove the
inline import from the body where raw_token =
request.cookies.get("refresh_token") is checked; then keep the existing raise
UnauthorizedException(...) call unchanged so the function uses the top-level
symbol instead of importing it each call.
In `@app/auth/service.py`:
- Around line 156-159: The calls in app/auth/service.py use an email-scoped
TokenStoreService API that doesn't exist; update the callers
(save_refresh_token, is_refresh_token_valid, revoke_refresh_token) to match the
actual TokenStoreService signatures by removing the email= keyword and passing
the expected positional/keyword args (e.g., pass jti and ttl_seconds or only jti
as the real API requires) and replace the non-existent revoke_all_user_tokens
call with either the service's provided method for bulk revocation or implement
a loop using the existing revoke_refresh_token(jti) API; locate usages by the
function names save_refresh_token, is_refresh_token_valid, revoke_refresh_token,
and revoke_all_user_tokens in app/auth/service.py (around the reported blocks)
and adapt each call to the real TokenStoreService method signatures.
- Around line 208-226: The refresh-token rotation is vulnerable because
is_refresh_token_valid(...) and revoke_refresh_token(...) are separate awaits;
replace them with a single atomic call on the token store (e.g., implement and
call consume_refresh_token(email, jti) on token_store) that performs the
check-and-delete in one Redis operation (Lua script/EVAL/EVALSHA) and returns
success/failure; update the service to call await
self.token_store.consume_refresh_token(email=email, jti=old_jti) and if it
returns False then call revoke_all_user_tokens(email) and raise the
REFRESH_TOKEN_REUSE UnauthorizedException, otherwise continue normal flow,
leaving get_user_by_email(...) and subsequent logic unchanged.
In `@tests/test_auth/test_auth_login.py`:
- Line 1: The module docstring currently says "POST /routers/v1/auth/login" but
the tests actually call "/api/v1/auth/login"; update the top-level docstring in
tests/test_auth/test_auth_login.py to state the correct endpoint
"/api/v1/auth/login" so it matches the requests used in the test functions
(e.g., the POST calls to "/api/v1/auth/login").
In `@tests/test_auth/test_auth_refresh.py`:
- Around line 312-334: Add a test method to TestRefreshTokenInvalid that seeds
the user, builds a validly signed refresh token whose exp is already past (e.g.
call _SECURITY.create_refresh_token(email="refresh@example.com",
expires_delta=timedelta(seconds=-1)) or otherwise inject an "exp" in the past),
send it as the "refresh_token" cookie to client.post(_URL), and assert
response.status_code == 401 and response.json()["code"] ==
"INVALID_REFRESH_TOKEN"; place the new test alongside
test_returns_401_for_garbage_token and
test_returns_401_for_access_token_used_as_refresh so it exercises the
expired-token decoder branch.
- Around line 248-272: The test test_sets_new_httponly_refresh_cookie only
asserts partial cookie attributes; update it to assert the full refresh cookie
contract by checking that response.headers.get("set-cookie") from
client.post(_URL, cookies={"refresh_token": raw_token}) contains
"refresh_token=", "HttpOnly", "SameSite=strict", "Secure", "Path=/api/v1/auth",
and "Max-Age=604800" (or the ttl variable if dynamic) so all required flags are
validated in a single assertion block.
---
Duplicate comments:
In `@app/auth/service.py`:
- Around line 136-145: In login(), add the same active-account guard used in
refresh_token(): after retrieving the user and before resetting attempts or
issuing tokens, check if not user.is_active and raise a ForbiddenException
(e.g., code "ACCOUNT_INACTIVE", message "This account is deactivated.") so
deactivated accounts cannot obtain new tokens; ensure this check is placed
alongside the existing deleted_at check and occurs before calling
lockout_svc.reset_attempts(email) and token creation.
- Around line 170-177: In forgot_password (service.py) you currently generate a
UUID for the reset_link but never persist it; change the flow to generate a
cryptographically secure token (e.g., secrets.token_urlsafe or uuid4), store a
hashed version plus an expiry timestamp on the user record or a dedicated
PasswordResetToken entity (fields like reset_password_token_hash and
reset_password_expires), persist the change (save/commit) before sending the
email, and invalidate any previous tokens for that user; keep using the plain
token in the emailed link but ensure the reset handler (e.g., the function that
handles reset-password) validates against the stored hash and checks expiry.
In `@app/services/email_consumer.py`:
- Line 67: The Mailgun API call in email_consumer.py uses the wrong Basic Auth
username; change the auth tuple used when sending messages (currently
auth=("routers", settings.MAILGUN_API_KEY)) to use the required username "api"
so it reads auth=("api", settings.MAILGUN_API_KEY); update the auth tuple
wherever send/post to Mailgun is performed (look for the auth= argument in the
Mailgun POST call) and run tests or a quick send to verify the request is
accepted.
---
Nitpick comments:
In `@app/core/security.py`:
- Around line 14-23: Add a short explanatory comment above the bcrypt shim that
references the upstream Passlib/bcrypt compatibility issue and states the
removal condition; specifically, mention passlib 1.7.4 and bcrypt 4.0+
incompatibility, include a URL or issue number to the upstream bugtracker, and
note "remove when passlib fixed (>= version X or PR/issue closed)"; place the
comment next to the try/except and reference the injected symbols
bcrypt.__about__ and the _BcryptAbout stub so future maintainers know why the
shim exists and when it can be removed.
🪄 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: 6db09f04-e81f-4df6-9ab8-1ba579cad085
📒 Files selected for processing (60)
README.mdalembic/env.pyalembic/versions/092f1a489931_add_meeting_model.pyalembic/versions/7f066a8213a8_change_user_id_to_uuid.pyalembic/versions/e1a664780dc6_add_meeting_model.pyalembic_output.txtapp/api/v1/__init__.pyapp/api/v1/endpoints/auth.pyapp/auth/__init__.pyapp/auth/account_lockout.pyapp/auth/constants.pyapp/auth/dependencies.pyapp/auth/models.pyapp/auth/router.pyapp/auth/schemas.pyapp/auth/service.pyapp/auth/token_store.pyapp/auth/utils.pyapp/auth/verification.pyapp/core/security.pyapp/crud/__init__.pyapp/crud/auth_verification_api.mdapp/crud/user/user.pyapp/crud/verification_token.pyapp/external_services/__init__.pyapp/external_services/cloudinary/__init__.pyapp/external_services/mailgun/__init__.pyapp/main.pyapp/models/__init__.pyapp/models/base.pyapp/models/user.pyapp/models/verification_token.pyapp/routers/__init__.pyapp/routers/api.pyapp/schemas/auth.pyapp/services/email_consumer.pyapp/user/__init__.pyapp/user/config.pyapp/user/constants.pyapp/user/dependencies.pyapp/user/exceptions.pyapp/user/models.pyapp/user/router.pyapp/user/schemas.pyapp/user/service.pyapp/user/utils.pyapp/utils/__init__.pyapp/utils/authentication.pyapp/utils/validation.pyfail_log.txtpyproject.tomlrequirements/base.txtrequirements/dev.txtrequirements/prod.txttests/test_auth/test_auth_login.pytests/test_auth/test_auth_refresh.pytests/test_auth/test_auth_signup.pytests/test_auth/test_email_verification.pytests/test_auth/test_schemas_user.pytests/test_auth/test_verification_token_crud.py
💤 Files with no reviewable changes (9)
- alembic_output.txt
- app/crud/init.py
- app/models/verification_token.py
- app/crud/auth_verification_api.md
- app/api/v1/init.py
- app/crud/user/user.py
- app/models/user.py
- app/crud/verification_token.py
- fail_log.txt
✅ Files skipped from review due to trivial changes (11)
- README.md
- app/models/base.py
- app/routers/api.py
- pyproject.toml
- tests/test_auth/test_schemas_user.py
- app/routers/init.py
- app/auth/account_lockout.py
- tests/test_auth/test_auth_signup.py
- alembic/versions/e1a664780dc6_add_meeting_model.py
- app/auth/schemas.py
- app/auth/dependencies.py
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/init.py
- tests/test_auth/test_email_verification.py
- app/auth/models.py
| def downgrade() -> None: | ||
| """Downgrade schema.""" | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.alter_column('verification_tokens', 'user_id', | ||
| existing_type=sa.Uuid(), | ||
| type_=sa.INTEGER(), | ||
| existing_nullable=False) | ||
| op.alter_column('users', 'id', | ||
| existing_type=sa.Uuid(), | ||
| type_=sa.INTEGER(), | ||
| existing_nullable=False) | ||
| op.alter_column( | ||
| "verification_tokens", | ||
| "user_id", | ||
| existing_type=sa.Uuid(), | ||
| type_=sa.INTEGER(), | ||
| existing_nullable=False, | ||
| ) | ||
| op.alter_column( | ||
| "users", | ||
| "id", | ||
| existing_type=sa.Uuid(), | ||
| type_=sa.INTEGER(), | ||
| existing_nullable=False, | ||
| ) |
There was a problem hiding this comment.
Incomplete downgrade: FK constraint and default not handled.
The upgrade() function drops the FK constraint before altering column types and removes the integer default. However, downgrade() attempts to alter the columns back to INTEGER without:
- Dropping the FK constraint first (will fail due to type mismatch)
- Restoring the integer sequence/default on
users.id
This will cause the downgrade to fail if ever executed.
🐛 Proposed fix to make downgrade symmetric with upgrade
def downgrade() -> None:
"""Downgrade schema."""
# ### commands auto generated by Alembic - please adjust! ###
+ # Drop foreign key constraint before altering column types
+ op.drop_constraint(
+ "verification_tokens_user_id_fkey", "verification_tokens", type_="foreignkey"
+ )
+
+ # Truncate to allow type conversion (UUIDs cannot be cast to integers)
+ op.execute("TRUNCATE TABLE users CASCADE")
+
op.alter_column(
"verification_tokens",
"user_id",
existing_type=sa.Uuid(),
type_=sa.INTEGER(),
existing_nullable=False,
)
op.alter_column(
"users",
"id",
existing_type=sa.Uuid(),
type_=sa.INTEGER(),
existing_nullable=False,
)
+
+ # Restore integer sequence default
+ op.execute("CREATE SEQUENCE IF NOT EXISTS users_id_seq OWNED BY users.id")
+ op.execute("ALTER TABLE users ALTER COLUMN id SET DEFAULT nextval('users_id_seq')")
+
+ # Re-add foreign key constraint
+ op.create_foreign_key(
+ "verification_tokens_user_id_fkey",
+ "verification_tokens",
+ "users",
+ ["user_id"],
+ ["id"],
+ )
# ### end Alembic commands ###📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def downgrade() -> None: | |
| """Downgrade schema.""" | |
| # ### commands auto generated by Alembic - please adjust! ### | |
| op.alter_column('verification_tokens', 'user_id', | |
| existing_type=sa.Uuid(), | |
| type_=sa.INTEGER(), | |
| existing_nullable=False) | |
| op.alter_column('users', 'id', | |
| existing_type=sa.Uuid(), | |
| type_=sa.INTEGER(), | |
| existing_nullable=False) | |
| op.alter_column( | |
| "verification_tokens", | |
| "user_id", | |
| existing_type=sa.Uuid(), | |
| type_=sa.INTEGER(), | |
| existing_nullable=False, | |
| ) | |
| op.alter_column( | |
| "users", | |
| "id", | |
| existing_type=sa.Uuid(), | |
| type_=sa.INTEGER(), | |
| existing_nullable=False, | |
| ) | |
| def downgrade() -> None: | |
| """Downgrade schema.""" | |
| # ### commands auto generated by Alembic - please adjust! ### | |
| # Drop foreign key constraint before altering column types | |
| op.drop_constraint( | |
| "verification_tokens_user_id_fkey", "verification_tokens", type_="foreignkey" | |
| ) | |
| # Truncate to allow type conversion (UUIDs cannot be cast to integers) | |
| op.execute("TRUNCATE TABLE users CASCADE") | |
| op.alter_column( | |
| "verification_tokens", | |
| "user_id", | |
| existing_type=sa.Uuid(), | |
| type_=sa.INTEGER(), | |
| existing_nullable=False, | |
| ) | |
| op.alter_column( | |
| "users", | |
| "id", | |
| existing_type=sa.Uuid(), | |
| type_=sa.INTEGER(), | |
| existing_nullable=False, | |
| ) | |
| # Restore integer sequence default | |
| op.execute("CREATE SEQUENCE IF NOT EXISTS users_id_seq OWNED BY users.id") | |
| op.execute("ALTER TABLE users ALTER COLUMN id SET DEFAULT nextval('users_id_seq')") | |
| # Re-add foreign key constraint | |
| op.create_foreign_key( | |
| "verification_tokens_user_id_fkey", | |
| "verification_tokens", | |
| "users", | |
| ["user_id"], | |
| ["id"], | |
| ) | |
| # ### end Alembic commands ### |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@alembic/versions/7f066a8213a8_change_user_id_to_uuid.py` around lines 56 -
72, The downgrade() is incomplete: before altering types back to INTEGER you
must drop the foreign-key constraint on verification_tokens.user_id (the same FK
removed in upgrade()), then alter verification_tokens.user_id and users.id back
to sa.INTEGER(), and restore the integer sequence/default on users.id (recreate
the users_id sequence if needed and set users.id server_default to
nextval('users_id_seq'::regclass) or equivalent). After the column type changes,
re-create the FK constraint linking verification_tokens.user_id to users.id.
Update the downgrade() function to perform these steps in this order so it
mirrors the upgrade() logic.
| if not await token_store.is_refresh_token_valid(email=email, jti=old_jti): | ||
| # Potential theft: tear down ALL sessions for this user. | ||
| await token_store.revoke_all_user_tokens(email=email) |
There was a problem hiding this comment.
Same signature mismatch for is_refresh_token_valid and revoke_all_user_tokens.
These calls also pass email= as a keyword argument, but if the methods in TokenStoreService weren't updated to accept this parameter, they will fail at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/v1/endpoints/auth.py` around lines 489 - 491, The calls to
token_store.is_refresh_token_valid(...) and
token_store.revoke_all_user_tokens(...) use a keyword argument email= but the
TokenStoreService methods likely don't accept that name; update the
TokenStoreService method signatures (is_refresh_token_valid and
revoke_all_user_tokens) to accept email as a keyword parameter (e.g., def
is_refresh_token_valid(self, *, email: str, jti: str) / def
revoke_all_user_tokens(self, *, email: str)) or modify the calls to pass email
as a positional argument (token_store.is_refresh_token_valid(email, old_jti) /
token_store.revoke_all_user_tokens(email)) so the parameter names and calling
style match between the caller and the TokenStoreService implementation.
| raw_token = request.cookies.get("refresh_token") | ||
| if not raw_token: | ||
| from app.core.exceptions import UnauthorizedException | ||
|
|
||
| raise UnauthorizedException( | ||
| code="MISSING_REFRESH_TOKEN", | ||
| message="No refresh token provided.", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move inline import to module level.
The UnauthorizedException import is placed inside the function body at line 201. This should be moved to the top-level imports for consistency and to avoid repeated import overhead.
♻️ Suggested fix
Add to the imports at the top of the file:
from app.core.exceptions import UnauthorizedExceptionThen simplify the function:
raw_token = request.cookies.get("refresh_token")
if not raw_token:
- from app.core.exceptions import UnauthorizedException
-
raise UnauthorizedException(
code="MISSING_REFRESH_TOKEN",
message="No refresh token provided.",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/auth/router.py` around lines 199 - 206, The inline import of
UnauthorizedException inside the function should be moved to module-level
imports: add "from app.core.exceptions import UnauthorizedException" to the top
of app/auth/router.py and remove the inline import from the body where raw_token
= request.cookies.get("refresh_token") is checked; then keep the existing raise
UnauthorizedException(...) call unchanged so the function uses the top-level
symbol instead of importing it each call.
| # Persist refresh JTI in Redis | ||
| await self.token_store.save_refresh_token( | ||
| email=email, jti=refresh_jti, ttl_seconds=refresh_ttl | ||
| ) |
There was a problem hiding this comment.
Align this caller with the actual TokenStoreService API.
CI is already failing here: the imported TokenStoreService does not accept email= on save_refresh_token(), is_refresh_token_valid(), or revoke_refresh_token(), and it does not define revoke_all_user_tokens(). Until app/auth/token_store.py is updated to the email-scoped contract—or these calls are switched to the real signature—login and refresh are blocked.
Also applies to: 208-209, 226-235
🧰 Tools
🪛 GitHub Actions: CI
[error] 157-157: mypy: Unexpected keyword argument "email" for "save_refresh_token" of "TokenStoreService" [call-arg]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/auth/service.py` around lines 156 - 159, The calls in app/auth/service.py
use an email-scoped TokenStoreService API that doesn't exist; update the callers
(save_refresh_token, is_refresh_token_valid, revoke_refresh_token) to match the
actual TokenStoreService signatures by removing the email= keyword and passing
the expected positional/keyword args (e.g., pass jti and ttl_seconds or only jti
as the real API requires) and replace the non-existent revoke_all_user_tokens
call with either the service's provided method for bulk revocation or implement
a loop using the existing revoke_refresh_token(jti) API; locate usages by the
function names save_refresh_token, is_refresh_token_valid, revoke_refresh_token,
and revoke_all_user_tokens in app/auth/service.py (around the reported blocks)
and adapt each call to the real TokenStoreService method signatures.
| if not await self.token_store.is_refresh_token_valid(email=email, jti=old_jti): | ||
| await self.token_store.revoke_all_user_tokens(email=email) | ||
| logger.warning( | ||
| "Refresh token reuse detected for %s — all sessions revoked.", | ||
| sanitize_log_args(email)[0], | ||
| ) | ||
| raise UnauthorizedException( | ||
| code="REFRESH_TOKEN_REUSE", | ||
| message="Session has been invalidated. Please log in again.", | ||
| ) | ||
|
|
||
| user = self.get_user_by_email(email) | ||
| if user is None or user.deleted_at is not None or not user.is_active: | ||
| raise ForbiddenException( | ||
| code="ACCOUNT_DEACTIVATED", | ||
| message="This account has been deactivated.", | ||
| ) | ||
|
|
||
| await self.token_store.revoke_refresh_token(email=email, jti=old_jti) |
There was a problem hiding this comment.
Consume the old refresh token atomically.
is_refresh_token_valid() and revoke_refresh_token() are separate awaits. Two concurrent refresh requests using the same live token can both pass the validity check before either delete runs, producing two valid child refresh tokens from one parent and defeating single-use rotation.
Suggested direction
- if not await self.token_store.is_refresh_token_valid(email=email, jti=old_jti):
+ if not await self.token_store.consume_refresh_token(email=email, jti=old_jti):
await self.token_store.revoke_all_user_tokens(email=email)
logger.warning(
"Refresh token reuse detected for %s — all sessions revoked.",
sanitize_log_args(email)[0],
)
raise UnauthorizedException(
code="REFRESH_TOKEN_REUSE",
message="Session has been invalidated. Please log in again.",
)
@@
- await self.token_store.revoke_refresh_token(email=email, jti=old_jti)consume_refresh_token() needs to be implemented as one atomic Redis operation.
🧰 Tools
🪛 GitHub Actions: CI
[error] 208-208: mypy: Unexpected keyword argument "email" for "is_refresh_token_valid" of "TokenStoreService" [call-arg]
[error] 209-209: mypy: "TokenStoreService" has no attribute "revoke_all_user_tokens" [attr-defined]
[error] 226-226: mypy: Unexpected keyword argument "email" for "revoke_refresh_token" of "TokenStoreService" [call-arg]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/auth/service.py` around lines 208 - 226, The refresh-token rotation is
vulnerable because is_refresh_token_valid(...) and revoke_refresh_token(...) are
separate awaits; replace them with a single atomic call on the token store
(e.g., implement and call consume_refresh_token(email, jti) on token_store) that
performs the check-and-delete in one Redis operation (Lua script/EVAL/EVALSHA)
and returns success/failure; update the service to call await
self.token_store.consume_refresh_token(email=email, jti=old_jti) and if it
returns False then call revoke_all_user_tokens(email) and raise the
REFRESH_TOKEN_REUSE UnauthorizedException, otherwise continue normal flow,
leaving get_user_by_email(...) and subsequent logic unchanged.
| @@ -1,4 +1,4 @@ | |||
| """Integration tests for ``POST /api/v1/auth/login``.""" | |||
| """Integration tests for ``POST /routers/v1/auth/login``.""" | |||
There was a problem hiding this comment.
Fix inconsistent docstring.
The docstring states POST /routers/v1/auth/login but the actual test requests use /api/v1/auth/login (e.g., line 221). Update to match the actual endpoint path.
📝 Suggested fix
-"""Integration tests for ``POST /routers/v1/auth/login``."""
+"""Integration tests for ``POST /api/v1/auth/login``."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Integration tests for ``POST /routers/v1/auth/login``.""" | |
| """Integration tests for ``POST /api/v1/auth/login``.""" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_auth/test_auth_login.py` at line 1, The module docstring currently
says "POST /routers/v1/auth/login" but the tests actually call
"/api/v1/auth/login"; update the top-level docstring in
tests/test_auth/test_auth_login.py to state the correct endpoint
"/api/v1/auth/login" so it matches the requests used in the test functions
(e.g., the POST calls to "/api/v1/auth/login").
| def test_sets_new_httponly_refresh_cookie( | ||
| self, | ||
| client: TestClient, | ||
| db_session: Session, | ||
| token_store: TokenStoreService, | ||
| ) -> None: | ||
| email = "refresh@example.com" | ||
| _seed_user(db_session, email=email) | ||
| raw_token, jti, ttl = _make_refresh_cookie(email) | ||
|
|
||
| import asyncio | ||
|
|
||
| asyncio.get_event_loop().run_until_complete( | ||
| token_store.save_refresh_token(email=email, jti=jti, ttl_seconds=ttl) | ||
| ) | ||
|
|
||
| response = client.post(_URL, cookies={"refresh_token": raw_token}) | ||
|
|
||
| assert response.status_code == 200 | ||
| # TestClient exposes set-cookie as a header | ||
| set_cookie = response.headers.get("set-cookie", "") | ||
| assert "refresh_token=" in set_cookie | ||
| assert "HttpOnly" in set_cookie | ||
| assert "SameSite=strict" in set_cookie | ||
|
|
There was a problem hiding this comment.
Assert the full refresh-cookie contract.
This test only checks HttpOnly and SameSite. The endpoint requirement also includes Secure, Path=/api/v1/auth, and Max-Age=604800, so a regression in any of those flags would still pass here.
Suggested fix
assert response.status_code == 200
# TestClient exposes set-cookie as a header
set_cookie = response.headers.get("set-cookie", "")
+ lowered = set_cookie.lower()
assert "refresh_token=" in set_cookie
- assert "HttpOnly" in set_cookie
- assert "SameSite=strict" in set_cookie
+ assert "httponly" in lowered
+ assert "secure" in lowered
+ assert "samesite=strict" in lowered
+ assert "path=/api/v1/auth" in lowered
+ assert "max-age=604800" in lowered📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_sets_new_httponly_refresh_cookie( | |
| self, | |
| client: TestClient, | |
| db_session: Session, | |
| token_store: TokenStoreService, | |
| ) -> None: | |
| email = "refresh@example.com" | |
| _seed_user(db_session, email=email) | |
| raw_token, jti, ttl = _make_refresh_cookie(email) | |
| import asyncio | |
| asyncio.get_event_loop().run_until_complete( | |
| token_store.save_refresh_token(email=email, jti=jti, ttl_seconds=ttl) | |
| ) | |
| response = client.post(_URL, cookies={"refresh_token": raw_token}) | |
| assert response.status_code == 200 | |
| # TestClient exposes set-cookie as a header | |
| set_cookie = response.headers.get("set-cookie", "") | |
| assert "refresh_token=" in set_cookie | |
| assert "HttpOnly" in set_cookie | |
| assert "SameSite=strict" in set_cookie | |
| def test_sets_new_httponly_refresh_cookie( | |
| self, | |
| client: TestClient, | |
| db_session: Session, | |
| token_store: TokenStoreService, | |
| ) -> None: | |
| email = "refresh@example.com" | |
| _seed_user(db_session, email=email) | |
| raw_token, jti, ttl = _make_refresh_cookie(email) | |
| import asyncio | |
| asyncio.get_event_loop().run_until_complete( | |
| token_store.save_refresh_token(email=email, jti=jti, ttl_seconds=ttl) | |
| ) | |
| response = client.post(_URL, cookies={"refresh_token": raw_token}) | |
| assert response.status_code == 200 | |
| # TestClient exposes set-cookie as a header | |
| set_cookie = response.headers.get("set-cookie", "") | |
| lowered = set_cookie.lower() | |
| assert "refresh_token=" in set_cookie | |
| assert "httponly" in lowered | |
| assert "secure" in lowered | |
| assert "samesite=strict" in lowered | |
| assert "path=/api/v1/auth" in lowered | |
| assert "max-age=604800" in lowered |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_auth/test_auth_refresh.py` around lines 248 - 272, The test
test_sets_new_httponly_refresh_cookie only asserts partial cookie attributes;
update it to assert the full refresh cookie contract by checking that
response.headers.get("set-cookie") from client.post(_URL,
cookies={"refresh_token": raw_token}) contains "refresh_token=", "HttpOnly",
"SameSite=strict", "Secure", "Path=/api/v1/auth", and "Max-Age=604800" (or the
ttl variable if dynamic) so all required flags are validated in a single
assertion block.
| class TestRefreshTokenInvalid: | ||
| """Tampered or expired tokens.""" | ||
|
|
||
| def test_returns_401_for_garbage_token( | ||
| self, client: TestClient, db_session: Session | ||
| ) -> None: | ||
| _seed_user(db_session) | ||
| response = client.post(_URL, cookies={"refresh_token": "this.is.garbage"}) | ||
|
|
||
| assert response.status_code == 401 | ||
| assert response.json()["code"] == "INVALID_REFRESH_TOKEN" | ||
|
|
||
| def test_returns_401_for_access_token_used_as_refresh( | ||
| self, client: TestClient, db_session: Session | ||
| ) -> None: | ||
| """An access token must not be accepted as a refresh token.""" | ||
| _seed_user(db_session) | ||
| access_token, _ = _SECURITY.create_access_token(email="refresh@example.com") | ||
|
|
||
| response = client.post(_URL, cookies={"refresh_token": access_token}) | ||
|
|
||
| assert response.status_code == 401 | ||
| assert response.json()["code"] == "INVALID_REFRESH_TOKEN" |
There was a problem hiding this comment.
Add the expired-refresh-token case.
The suite covers garbage JWTs and using an access token as a refresh token, but not a validly signed refresh token whose exp is already in the past. That was an explicit requirement and exercises a different decoder branch than the cases above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_auth/test_auth_refresh.py` around lines 312 - 334, Add a test
method to TestRefreshTokenInvalid that seeds the user, builds a validly signed
refresh token whose exp is already past (e.g. call
_SECURITY.create_refresh_token(email="refresh@example.com",
expires_delta=timedelta(seconds=-1)) or otherwise inject an "exp" in the past),
send it as the "refresh_token" cookie to client.post(_URL), and assert
response.status_code == 401 and response.json()["code"] ==
"INVALID_REFRESH_TOKEN"; place the new test alongside
test_returns_401_for_garbage_token and
test_returns_401_for_access_token_used_as_refresh so it exercises the
expired-token decoder branch.
19df2dc to
e1c5056
Compare
2 participants
POST /api/v1/auth/refresh-tokenendpoint for secure token rotation via HttpOnly cookies.TokenStoreServiceRedis architecture to use email-scoped keys (refresh_token:{email}:{jti}) to support efficient session revocation.revoke_all_user_tokensmechanism to instantly tear down an entire user session family if a stolen (already revoked) refresh token is replayed.decode_refresh_tokenwrapper returning a strictly typedRefreshTokenClaimsschema to eliminateOptionaltype constraints from Mypy.test_auth_refresh.pycovering valid rotation, tampered tokens, hijacked sessions, and deactivated accounts.aiokafkaintegrations recognizing modern stubs.Closes #25
Summary by CodeRabbit
Release Notes