audit follow-ups: password schema cleanup, hook isolation, polish#22
Merged
Conversation
- hashed_password is nullable on UserMixin (both SA + SM). OAuth users get NULL instead of a fake random hash. - has_usable_password column dropped. hashed_password IS NOT NULL is the single signal. - /auth/set-password route + flows/set_password.py removed. /change-password now accepts optional current_password — only skipped when the stored hash is NULL. - AbstractUserAdapter.create_user: hashed_password is str | None. - flows.oauth.link_or_create_user and oauth_callback no longer take hash_algorithm (no password to hash). - Docs, agent skill notes, and CHANGELOG updated. - Tests added: first-time set via /change-password without current_password, and the regression case where current_password is still required when a hash exists.
Wrap each hook call in try/except inside EventHooks.emit. A raising hook is logged via fastapi_fullauth.hooks and the next hook still runs. Previously a single failing hook aborted the chain and surfaced as a 500 even though the primary side effect (user created, password reset, etc.) had already committed. Hooks fire after that commit, so a notification failure shouldn't undo the operation the response reports. Added test: three hooks registered, middle one raises, the first and third both still fire and the HTTP request returns its normal status.
Add _user_query() helper to SQLAlchemyAdapter that calls selectinload(user_model.roles) when the user model has a roles attribute. Used by get_user_by_id, get_user_by_field, update_user, create_user, and get_user_roles. Previously the bare select(user_model) relied on the app to declare lazy="selectin" on the relationship. If left default, _to_schema's [r.name for r in user.roles] triggered an async lazy-load outside the session and raised MissingGreenlet. The SQLModel adapter already had this helper; the two now behave the same. Added test: a User model with default-lazy roles relationship, run through create_user -> assign_role -> get_user_by_email, asserts roles populate without error.
Stale text the previous two commits missed: - skill/hooks.md said a raising hook becomes a 500 and propagates. Flipped to describe the new behavior: caught, logged via fastapi_fullauth.hooks, next hook still runs, route returns normally. - docs/auth/hooks.md and llms-full.txt got a matching one-paragraph note in the "Multiple hooks per event" section. - skill/adapters.md and skill/troubleshooting.md previously said a custom user model with default-lazy roles would throw MissingGreenlet. The adapter now handles roles via selectinload itself; only other custom relationships need lazy="selectin" or selectinload at the call site.
…64 padding Four small follow-ups from the audit. - Passkey router: broad except now uses logger.exception so the traceback lands in fastapi_fullauth.routers.passkey instead of just the str(e). Affects /passkeys/register/complete and /passkeys/authenticate/complete. - SA UserMixin.email is String(320), matching the SQLModel mixin's max_length and the OAuthAccountMixin.provider_email column. Avoids silent VARCHAR truncation on MySQL/MSSQL default lengths. - AuthRateLimiter.check() now sets X-RateLimit-Limit / X-RateLimit-Remaining / X-RateLimit-Reset / Retry-After on its 429 responses, matching what the global RateLimitMiddleware does. - flows.passkey._b64_decode: padding is now (-len(data)) % 4 instead of 4 - len(data) % 4 so multiples of 4 don't get four trailing '=' bytes appended. Test added: extended test_login_rate_limited to assert the X-RateLimit-* and Retry-After headers on the 429.
ChallengeStore (memory + redis backends + factory) lives in fastapi_fullauth/protection/challenges.py now. It's a stateful anti-replay defence for WebAuthn — closer in nature to lockout and ratelimit than to TokenEngine or the password-hashing primitives. Old imports `from fastapi_fullauth.core.challenges import ...` need to update to `fastapi_fullauth.protection.challenges`. Also re-exported from the fastapi_fullauth.protection package for symmetry with the other defensive stores. Cleaned up a stale section in the agent-skill api-reference: the Core block referenced core.redis_blacklist (gone in 0.10) and listed the challenge imports under "Core". Now lists the real core imports (TokenEngine, crypto, TokenBlacklist) and moves the challenge imports into the Protection block.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements major breaking changes for version 0.10.0 centered on OAuth-first user handling. It makes password hashes optional for OAuth-only users, consolidates password-setting into a single flow, adds graceful hook error handling, improves SQLAlchemy async safety, and relocates challenge store exports to the protection module. ChangesOAuth-First User Model & Password Unification
Runtime Behavior: Hooks, Storage, Rate Limiting
Infrastructure: Challenge Store & Passkey Fixes
Documentation Updates: Schema, Flows & Troubleshooting
Test Coverage: New Scenarios & Updated Assertions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
- change_password.py: SIM102 — merged the nested `if hashed is not None` + `if not current_password or not verify_password(...)` into a single `and` condition. - routers/passkey.py: I001 — moved `protection.challenges` import after `dependencies.current_user` so the alphabetical order is correct.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fastapi_fullauth/routers/passkey.py (1)
8-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix import ordering to unblock lint CI.
The import block is not sorted/formatted per Ruff I001, which is currently failing CI.
Suggested fix
from fastapi_fullauth.adapters.base import PasskeyAdapterMixin -from fastapi_fullauth.protection.challenges import ChallengeStore from fastapi_fullauth.dependencies.current_user import CurrentUser, get_fullauth +from fastapi_fullauth.protection.challenges import ChallengeStore from fastapi_fullauth.routers._schemas import build_login_response_model from fastapi_fullauth.types import TokenPair, UserSchema, UserSchemaType from fastapi_fullauth.utils import get_client_ip🤖 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/routers/passkey.py` around lines 8 - 13, Reorder the import block to satisfy Ruff I001 by grouping and alphabetizing imports: place standard-library imports first (none here), then third-party packages, then local package imports; within each group sort alphabetically. Update the import lines referencing PasskeyAdapterMixin, ChallengeStore, CurrentUser and get_fullauth, build_login_response_model, TokenPair/UserSchema/UserSchemaType, and get_client_ip so they follow that ordering and are alphabetized to unblock lint CI.
🧹 Nitpick comments (1)
fastapi_fullauth/adapters/sqlalchemy.py (1)
144-148: 💤 Low valueConsider replacing assert with explicit exception.
The
assert user is not Noneon line 147 is defensive programming for a should-never-happen scenario, but asserts can be disabled with Python's-Oflag. For production robustness, consider:♻️ Proposed refactor
# Re-fetch with roles eager-loaded so _to_schema works outside the session. result = await session.execute(self._user_query().where(self._user_model.id == user.id)) user = result.scalars().first() -assert user is not None +if user is None: + raise RuntimeError(f"User {user.id} not found immediately after creation") return self._to_schema(user)🤖 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/adapters/sqlalchemy.py` around lines 144 - 148, The code uses assert user is not None after re-fetching the user (in the block calling self._user_query(), session.execute(... where self._user_model.id == user.id)) which can be disabled with Python -O; replace the assert with an explicit exception (e.g., raise RuntimeError or a custom UserNotFoundError) that includes context (user id and that eager-load fetch failed) before returning self._to_schema(user) so the error cannot be skipped in production.
🤖 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/flows/change_password.py`:
- Around line 24-27: Collapse the nested guards in the change password check by
merging the two ifs into one: replace the nested "if hashed is not None: if not
current_password or not verify_password(current_password, hashed):" with a
single conditional that checks both hashed is not None and the combined failure
condition, then call logger.warning(...) and raise AuthenticationError("Current
password is incorrect"); refer to the variables and functions hashed,
current_password, verify_password, logger, and AuthenticationError in the
change_password flow.
---
Outside diff comments:
In `@fastapi_fullauth/routers/passkey.py`:
- Around line 8-13: Reorder the import block to satisfy Ruff I001 by grouping
and alphabetizing imports: place standard-library imports first (none here),
then third-party packages, then local package imports; within each group sort
alphabetically. Update the import lines referencing PasskeyAdapterMixin,
ChallengeStore, CurrentUser and get_fullauth, build_login_response_model,
TokenPair/UserSchema/UserSchemaType, and get_client_ip so they follow that
ordering and are alphabetized to unblock lint CI.
---
Nitpick comments:
In `@fastapi_fullauth/adapters/sqlalchemy.py`:
- Around line 144-148: The code uses assert user is not None after re-fetching
the user (in the block calling self._user_query(), session.execute(... where
self._user_model.id == user.id)) which can be disabled with Python -O; replace
the assert with an explicit exception (e.g., raise RuntimeError or a custom
UserNotFoundError) that includes context (user id and that eager-load fetch
failed) before returning self._to_schema(user) so the error cannot be skipped in
production.
🪄 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: 71d74988-6c02-4dde-9792-605ee9eaff59
📒 Files selected for processing (36)
CHANGELOG.mddocs/adapters/sqlalchemy.mddocs/adapters/sqlmodel.mddocs/auth/hooks.mddocs/getting-started.mddocs/llms-full.txtfastapi_fullauth/.agents/skills/fastapi-fullauth/references/adapters.mdfastapi_fullauth/.agents/skills/fastapi-fullauth/references/api-reference.mdfastapi_fullauth/.agents/skills/fastapi-fullauth/references/composable-design.mdfastapi_fullauth/.agents/skills/fastapi-fullauth/references/hooks.mdfastapi_fullauth/.agents/skills/fastapi-fullauth/references/oauth.mdfastapi_fullauth/.agents/skills/fastapi-fullauth/references/troubleshooting.mdfastapi_fullauth/adapters/base.pyfastapi_fullauth/adapters/sqlalchemy.pyfastapi_fullauth/adapters/sqlmodel.pyfastapi_fullauth/flows/change_password.pyfastapi_fullauth/flows/oauth.pyfastapi_fullauth/flows/passkey.pyfastapi_fullauth/flows/set_password.pyfastapi_fullauth/fullauth.pyfastapi_fullauth/hooks.pyfastapi_fullauth/models/sqlalchemy/base.pyfastapi_fullauth/models/sqlmodel/base.pyfastapi_fullauth/protection/__init__.pyfastapi_fullauth/protection/challenges.pyfastapi_fullauth/protection/ratelimit.pyfastapi_fullauth/routers/_schemas.pyfastapi_fullauth/routers/oauth.pyfastapi_fullauth/routers/passkey.pyfastapi_fullauth/routers/profile.pyfastapi_fullauth/types.pytests/test_auth.pytests/test_hooks.pytests/test_passkey.pytests/test_profile.pytests/test_sqlalchemy_adapter.py
💤 Files with no reviewable changes (3)
- fastapi_fullauth/types.py
- fastapi_fullauth/flows/set_password.py
- fastapi_fullauth/routers/oauth.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six commits addressing audit findings on the 0.10.0 branch.
Breaking
hashed_passwordis nullable onUserMixin. OAuth-only users getNULLinstead of a fake random hash.has_usable_passwordcolumn dropped —hashed_password IS NOT NULLis the single signal./auth/set-passwordroute andflows.set_passwordremoved./change-passwordaccepts optionalcurrent_password; only skipped when the stored hash isNULL.AbstractUserAdapter.create_user:hashed_passwordis nowstr | None.flows.oauth.link_or_create_userandoauth_callbackno longer takehash_algorithm(no password to hash).ChallengeStoremoved fromcore.challengestoprotection.challenges. Also re-exported fromfastapi_fullauth.protection.Fixed
fastapi_fullauth.hooksand the next hook still runs. Previously one failing hook 500'd the request.SQLAlchemyAdaptereager-loads roles via a new_user_query()helper mirroring the SQLModel adapter. User models with default-lazy roles no longer raiseMissingGreenlet.logger.exceptionso tracebacks land in the logger instead of juststr(e).SA UserMixin.emailis nowString(320)matching the SQLModel side; no more silent VARCHAR truncation on MySQL/MSSQL.Changed
AuthRateLimiter.check()setsX-RateLimit-Limit/Remaining/Reset+Retry-Afteron its 429s, matching the globalRateLimitMiddleware.flows.passkey._b64_decodepadding idiom ((-len(data)) % 4).Tests
All 202 pass, mypy --strict clean. New tests:
/change-passwordwithoutcurrent_passwordsucceeds for users with no stored hash, still rejected when a hash exists.SQLAlchemyAdapteruser-model with default-lazy roles loads roles correctly.Summary by CodeRabbit
New Features
X-RateLimit-Limit,X-RateLimit-Remaining, andRetry-Afterheaders for better client guidance.Bug Fixes
Breaking Changes
/set-passwordendpoint removed and functionality merged into/change-password.change_passwordflow now optional for first-time password setup on OAuth-only accounts.