audit cleanup + mypy strict pass#20
Merged
Merged
Conversation
Rate-limit /auth/refresh (default 30 req/min) so a stolen refresh token can't be ground for fresh access tokens, and pass user.id through to RefreshToken instead of stringifying it. Replace the dynamic LoginResponse model with a real subclass of TokenPair so static checkers see the user kwarg. Switch to dynamic versioning via hatch, reorder pyproject sections, move pytest config to [tool.pytest], bump all dependency floors to current stable, and bump the Development Status classifier from Alpha to Beta. Adds Operating System :: OS Independent and pulls in pytest-cov for contributors.
Adds the missing parameter and return annotations, replaces bare `dict`
with `dict[str, Any]`, tightens `hash_password(algorithm=...)` at the
flow call sites to `Literal["argon2id", "bcrypt"]`, and types the ASGI
middleware constructors with `ASGIApp` / `RequestResponseEndpoint`.
Mixin methods accessed through `AbstractUserAdapter` (`assign_role`,
`get_oauth_account`, etc.) are now narrowed via `cast()` to the right
mixin at each call site in admin, oauth, and require_role. Passkey
routes drop the inline `assert`s in favour of `cast()` over the values
already enforced by FullAuthConfig at construction time, so `python -O`
doesn't quietly remove them.
Also fixes a real bug: `DELETE /oauth/accounts/{provider}` was calling
`delete_oauth_account(provider, user.id)`, passing the local user UUID
where the provider's `provider_user_id` was expected. The query never
matched, so unlinking silently no-op'd. The route now resolves the
right account first and returns 404 when the user doesn't have one.
SQLAlchemy 2.0 / SQLModel column-comparison stub limitations
(`where(self.user_model.id == user_id)` types as `bool`, not
`ColumnElement[bool]`) are scoped to the two adapter modules via a
focused `[[tool.mypy.overrides]]` block so the rest of the codebase
stays strict.
`uv run mypy --strict fastapi_fullauth` now passes 0/0 and CI runs it
on every push and PR. 189 tests still pass.
📝 WalkthroughWalkthroughThis PR implements version 0.9.2 with comprehensive type safety improvements across the codebase, adds a new refresh-token rate-limiting feature, refactors adapter patterns for type correctness, and introduces a LoginResponse model with optional user inclusion. The changes also standardize refresh-token user_id storage and enforce strict mypy type-checking in CI. ChangesType Safety and Refresh Rate Limiting (0.9.2)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🧹 Nitpick comments (2)
tests/test_auth.py (1)
910-915: ⚡ Quick winAssert login success before reading
refresh_tokenin the new test.Adding a status assertion here makes failures explicit and easier to diagnose.
Suggested fix
r = await client.post( "/api/v1/auth/login", json={"email": "rl@t.com", "password": "securepass123"}, ) + assert r.status_code == 200 refresh_token = r.json()["refresh_token"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_auth.py` around lines 910 - 915, The test reads r.json()["refresh_token"] without asserting the login response succeeded; update the test around the client.post("/api/v1/auth/login", json=...) call so you first assert the response status (e.g., assert r.status_code == 200 or assert r.ok) before accessing refresh_token, then extract refresh_token from r.json(); reference the existing variables r and refresh_token to locate the change.pyproject.toml (1)
108-124: 🏗️ Heavy liftDocument the necessity of each disabled mypy code, or narrow to only those truly required by SQLAlchemy/SQLModel stubs.
The override disables 10 error codes for the adapter modules. While the commit message acknowledges SQLAlchemy 2.0 / SQLModel stub limitations, the scope includes codes like
no-untyped-def,no-untyped-call, andno-any-returnthat appear tied to general adapter code patterns (untyped user parameters in_to_schema(), dynamic attribute access, nullable query results) rather than stub-specific issues alone. Consider documenting in a comment which specific stub limitation each code addresses, or refactoring the adapter code itself to minimize suppressions and preserve strict guarantees in these critical modules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 108 - 124, The mypy override for the adapter modules currently disables many error codes; either annotate why each is disabled with inline comments referencing the specific stub limitation or reduce the list to only the truly required codes by fixing/adapting the adapter code: add precise typing to functions like _to_schema(), annotate dynamic attribute access with getattr casts, narrow nullable query results with Optional/typing.cast or explicit checks, and replace untyped parameters/returns with typed signatures or overloads so you can remove broad flags like "no-untyped-def", "no-untyped-call", and "no-any-return"; document remaining disabled codes next to the module entries indicating which SQLAlchemy/SQLModel stub limitation or specific adapter function (e.g., fastapi_fullauth.adapters.sqlmodel.adapter._to_schema) necessitates each suppression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fastapi_fullauth/__init__.py`:
- Line 1: Update the package version constant __version__ in
fastapi_fullauth/__init__.py from "0.9.2" to the intended release "0.10.0" so
dynamic versioning and builds reflect the correct release target.
In `@fastapi_fullauth/flows/passkey.py`:
- Around line 149-151: The current conversion of stored transport strings in
pk.transports to AuthenticatorTransport enums (the list comprehension building
transports=([AuthenticatorTransport(t) for t in pk.transports] if pk.transports
else None)) can raise ValueError for unknown values; update this to first filter
pk.transports to only known/valid transport values (e.g., compare each t against
the set of valid values from the AuthenticatorTransport enum or wrap individual
conversions in a try/except), then construct the list of AuthenticatorTransport
instances from the filtered values so begin_authentication won’t fail on invalid
persisted strings.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 108-124: The mypy override for the adapter modules currently
disables many error codes; either annotate why each is disabled with inline
comments referencing the specific stub limitation or reduce the list to only the
truly required codes by fixing/adapting the adapter code: add precise typing to
functions like _to_schema(), annotate dynamic attribute access with getattr
casts, narrow nullable query results with Optional/typing.cast or explicit
checks, and replace untyped parameters/returns with typed signatures or
overloads so you can remove broad flags like "no-untyped-def",
"no-untyped-call", and "no-any-return"; document remaining disabled codes next
to the module entries indicating which SQLAlchemy/SQLModel stub limitation or
specific adapter function (e.g.,
fastapi_fullauth.adapters.sqlmodel.adapter._to_schema) necessitates each
suppression.
In `@tests/test_auth.py`:
- Around line 910-915: The test reads r.json()["refresh_token"] without
asserting the login response succeeded; update the test around the
client.post("/api/v1/auth/login", json=...) call so you first assert the
response status (e.g., assert r.status_code == 200 or assert r.ok) before
accessing refresh_token, then extract refresh_token from r.json(); reference the
existing variables r and refresh_token to locate the change.
🪄 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: 0a46a278-50e7-4822-b1c6-007e92a65a45
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/workflows/ci.ymlCHANGELOG.mdfastapi_fullauth/.agents/skills/fastapi-fullauth/references/api-reference.mdfastapi_fullauth/__init__.pyfastapi_fullauth/adapters/__init__.pyfastapi_fullauth/adapters/sqlalchemy/models/__init__.pyfastapi_fullauth/adapters/sqlmodel/models/__init__.pyfastapi_fullauth/backends/cookie.pyfastapi_fullauth/config.pyfastapi_fullauth/core/challenges.pyfastapi_fullauth/core/crypto.pyfastapi_fullauth/core/redis_blacklist.pyfastapi_fullauth/core/tokens.pyfastapi_fullauth/dependencies/current_user.pyfastapi_fullauth/dependencies/require_role.pyfastapi_fullauth/flows/change_password.pyfastapi_fullauth/flows/login.pyfastapi_fullauth/flows/oauth.pyfastapi_fullauth/flows/passkey.pyfastapi_fullauth/flows/password_reset.pyfastapi_fullauth/flows/register.pyfastapi_fullauth/flows/set_password.pyfastapi_fullauth/fullauth.pyfastapi_fullauth/middleware/csrf.pyfastapi_fullauth/middleware/security_headers.pyfastapi_fullauth/oauth/base.pyfastapi_fullauth/oauth/github.pyfastapi_fullauth/oauth/google.pyfastapi_fullauth/protection/lockout.pyfastapi_fullauth/protection/ratelimit.pyfastapi_fullauth/router/_models.pyfastapi_fullauth/router/admin.pyfastapi_fullauth/router/auth.pyfastapi_fullauth/router/oauth.pyfastapi_fullauth/router/passkey.pyfastapi_fullauth/router/profile.pyfastapi_fullauth/utils.pypyproject.tomltests/test_auth.py
| @@ -1,4 +1,4 @@ | |||
| __version__ = "0.9.1" | |||
| __version__ = "0.9.2" | |||
There was a problem hiding this comment.
Align package version with the intended release target.
Given dynamic versioning now reads from this file, keeping 0.9.2 will stamp builds as 0.9.2. PR objectives indicate this should roll into 0.10.0, so this should be updated before merge to avoid accidental mis-versioned publish.
Suggested fix
-__version__ = "0.9.2"
+__version__ = "0.10.0"📝 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.
Suggested change
| __version__ = "0.9.2" | |
| __version__ = "0.10.0" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fastapi_fullauth/__init__.py` at line 1, Update the package version constant
__version__ in fastapi_fullauth/__init__.py from "0.9.2" to the intended release
"0.10.0" so dynamic versioning and builds reflect the correct release target.
Comment on lines
+149
to
+151
| transports=( | ||
| [AuthenticatorTransport(t) for t in pk.transports] if pk.transports else None | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where transports are written and where enum conversion occurs.
rg -n -C3 'transports' fastapi_fullauth/flows/passkey.pyRepository: mdfarhankc/fastapi-fullauth
Length of output: 1008
🏁 Script executed:
# Find PasskeyCredential model definition and its transports field type
rg -n -A5 "class PasskeyCredential" fastapi_fullauth/Repository: mdfarhankc/fastapi-fullauth
Length of output: 385
🏁 Script executed:
# Check imports and understand AuthenticatorTransport enum
rg -n "AuthenticatorTransport\|from webauthn" fastapi_fullauth/flows/passkey.pyRepository: mdfarhankc/fastapi-fullauth
Length of output: 53
🏁 Script executed:
# Look for any validation or type hints on transports field
rg -n -B2 -A2 "transports.*:" fastapi_fullauth/Repository: mdfarhankc/fastapi-fullauth
Length of output: 1849
🏁 Script executed:
# Check imports and find where AuthenticatorTransport comes from
rg -n "from webauthn" fastapi_fullauth/flows/passkey.py | head -20Repository: mdfarhankc/fastapi-fullauth
Length of output: 486
🏁 Script executed:
# Search for any validation of transports during credential creation
rg -n -B5 -A5 "def create_passkey\|def register_passkey" fastapi_fullauth/Repository: mdfarhankc/fastapi-fullauth
Length of output: 53
🏁 Script executed:
# Look at the registration flow to see if webauthn validates transports
rg -n -B3 -A8 "credential_id.*=" fastapi_fullauth/flows/passkey.py | head -40Repository: mdfarhankc/fastapi-fullauth
Length of output: 1198
🌐 Web query:
webauthn python library AuthenticatorTransport enum valid values
💡 Result:
The most popular and actively maintained Python WebAuthn library is py_webauthn (also published as the 'webauthn' package on PyPI) from Duo Labs. Its AuthenticatorTransport enum, defined in webauthn/helpers/structs.py, has the following valid values (as lowercase strings): "usb", "nfc", "ble", "smart-card", "internal", "cable", "hybrid" [1][2][3][4]. Other libraries have slightly different sets: - webauthn-rp: "usb", "nfc", "ble", "internal" [5]. - python-fido2: Defines AuthenticatorTransport as a string enum, but specific values not listed in docs; accepts list of strings [6][7]. These match the WebAuthn spec transports, with py_webauthn including the most complete set per recent spec drafts (e.g., smart-card, hybrid added later) [1][8]. Use py_webauthn for comprehensive support.
Citations:
- 1: https://github.com/duo-labs/py_webauthn/blob/master/webauthn/helpers/structs.py
- 2: Loosen restriction on acceptable transports? duo-labs/py_webauthn#217
- 3: https://github.com/duo-labs/py_webauthn/
- 4: https://pypi.org/project/webauthn/
- 5: https://webauthn-rp.readthedocs.io/en/latest/webauthn_rp.html
- 6: https://developers.yubico.com/python-fido2/API_Documentation/autoapi/fido2/webauthn/index.html
- 7: https://github.com/Yubico/python-fido2/blob/main/fido2/server.py
- 8: https://w3.org/TR/webauthn-3
🏁 Script executed:
# Try to understand what values AuthenticatorTransport accepts
# First, let's see if there's any documentation or type hints in the codebase
rg -n "AuthenticatorTransport" fastapi_fullauth/ --type pyRepository: mdfarhankc/fastapi-fullauth
Length of output: 340
🏁 Script executed:
# Check if there's any version pinning or requirements file for webauthn
find . -name "requirements*.txt" -o -name "pyproject.toml" -o -name "poetry.lock" -o -name "setup.py" | head -5Repository: mdfarhankc/fastapi-fullauth
Length of output: 88
🏁 Script executed:
# Check webauthn dependency and version
cat pyproject.toml | grep -A20 -B2 "webauthn\|dependencies"Repository: mdfarhankc/fastapi-fullauth
Length of output: 1634
🏁 Script executed:
# Verify if transports are validated during the registration flow
# Look at the credential["response"].get("transports", []) and how it's handled
rg -n -B10 -A10 'credential\["response"\]\.get\("transports"' fastapi_fullauth/flows/passkey.pyRepository: mdfarhankc/fastapi-fullauth
Length of output: 955
Guard invalid WebAuthn transport values before enum conversion.
Line 150 converts stored transport strings to AuthenticatorTransport enum without validation. If any persisted transport value is invalid or unsupported (due to data migration, schema changes, or library updates), this raises ValueError and breaks begin_authentication for that user. Filter unknown values before enum construction.
🔧 Proposed fix
from webauthn import generate_authentication_options, options_to_json
from webauthn.helpers.structs import AuthenticatorTransport, PublicKeyCredentialDescriptor
+ valid_transports = {t.value for t in AuthenticatorTransport}
+
allow_credentials = None
if user_id is not None and adapter is not None:
existing = await adapter.get_user_passkeys(user_id)
allow_credentials = [
PublicKeyCredentialDescriptor(
id=_b64_decode(pk.credential_id),
transports=(
- [AuthenticatorTransport(t) for t in pk.transports] if pk.transports else None
+ [AuthenticatorTransport(t) for t in pk.transports if t in valid_transports]
+ if pk.transports
+ else None
),
)
for pk in existing
]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fastapi_fullauth/flows/passkey.py` around lines 149 - 151, The current
conversion of stored transport strings in pk.transports to
AuthenticatorTransport enums (the list comprehension building
transports=([AuthenticatorTransport(t) for t in pk.transports] if pk.transports
else None)) can raise ValueError for unknown values; update this to first filter
pk.transports to only known/valid transport values (e.g., compare each t against
the set of valid values from the AuthenticatorTransport enum or wrap individual
conversions in a try/except), then construct the list of AuthenticatorTransport
instances from the filtered values so begin_authentication won’t fail on invalid
persisted strings.
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.
Summary
LoginResponseclass, alpha→beta classifier, dynamic versioning via hatch, dep floors bumped,[tool.pytest]flat config, pytest-cov added.router/oauth.py— unlink was passinguser.id(UUID) where the provider'sprovider_user_id(string) is expected, so it would never match in practice.Folds into the v0.10.0 release (no v0.9.2 release on PyPI).
Test plan
uv run ruff format --check .— cleanuv run ruff check .— cleanuv run pytest tests/— 189 passed, 1 skipped (bcrypt)Summary by CodeRabbit
Release Notes v0.9.2
New Features
Bug Fixes
Improvements
Documentation