diff --git a/CHANGELOG.md b/CHANGELOG.md index 47a35d7..217465f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,11 +2,47 @@ ## Unreleased +### Added + +- **Link an OAuth provider while signed in.** Until now a provider could only be attached as a side effect of signing in, and only when the provider reported the email as verified and it already matched an account, which left out a work account under a different address, a provider that does not verify emails, and one that returns no email at all. `GET /oauth/{provider}/link/authorize` and `POST /oauth/{provider}/link/callback` (both authenticated, same `binding` contract as sign-in) now attach the identity to the caller's account and return it; no tokens are issued, since linking is not a sign-in. The provider email is stored for display only, so it does not have to match the account and does not verify it. Two conflicts answer `409`: the identity already belongs to another user (never moved, that would be account takeover), or the user already linked a different account from the same provider (unlinking is keyed by provider alone and could not tell them apart). Linking an account the user already has refreshes the stored provider tokens. New flow functions `link_oauth_account()`, `oauth_link_callback()`, and `build_link_authorization_url()`; new exceptions `OAuthAccountAlreadyLinkedError` and `OAuthProviderAlreadyLinkedError`. This closes the last `get_oauth_associate_router` gap in the [fastapi-users migration guide](https://mdfarhankc.github.io/fastapi-fullauth/migrating-from-fastapi-users/). +- **Refresh-token pruning.** `adapter.prune_expired_refresh_tokens(before=None)` deletes rows whose expiry has passed and returns how many went. Rotation writes a row per refresh, so the table only ever grew, and `GET /sessions` reads every row a user has ever had. Only expired rows are removed: a revoked but unexpired row is what a replayed token is matched against, and deleting it early would turn a stolen-token replay into an ordinary rejection and leave the family alive. Call it from your own scheduler; nothing runs it for you. Custom adapters inherit a no-op, so nothing breaks. +- **`PASSWORD_MAX_LENGTH` (default 4096).** Without an upper bound, a large request body became hashing work for the server. bcrypt already refused over 72 bytes; argon2id would hash whatever arrived. +- **`REAUTH_MAX_AGE_SECONDS` (default 300) and an `auth_time` claim.** Tokens now record when the credentials behind the session were actually checked, stamped at login, OAuth callback and passkey sign-in, and carried unchanged through refresh rotation. New flow helpers `verify_recent_auth()` and `remaining_login_methods()`. +- **`after_oauth_link` and `after_oauth_unlink` hooks**, both `async def(user, provider)`. Attaching or removing a sign-in method is an event applications need to audit-log, and unlinking had no hook at all. + +### Changed + +- **The Redis lockout manager fails open instead of raising.** It had no error handling at all, so with `LOCKOUT_BACKEND=redis` a Redis outage turned every login into a 500. Attempts are now neither counted nor blocked while Redis is unreachable, and the error is logged; brute-force protection is lost for the duration. The rate limiter already made this trade; the token blacklist still fails closed. +- **`refresh_tokens.user_id`, `refresh_tokens.expires_at` and `oauth_accounts.user_id` are indexed** in the SQLModel and SQLAlchemy mixins. Every session listing, revoke-all, and prune filters on them, and a foreign key does not imply an index on PostgreSQL. The Beanie documents already indexed them. **Migration:** existing deployments need to add these indexes; new tables get them automatically. For Alembic, autogenerate picks them up, or add them by hand: + + ```sql + CREATE INDEX ix_fullauth_refresh_tokens_user_id ON fullauth_refresh_tokens (user_id); + CREATE INDEX ix_fullauth_refresh_tokens_expires_at ON fullauth_refresh_tokens (expires_at); + CREATE INDEX ix_fullauth_oauth_accounts_user_id ON fullauth_oauth_accounts (user_id); + ``` + +- **`POST /verify-email/request` no longer mails an already-verified account.** `create_email_verification_token()` returns `None` for a verified user; the route's response is unchanged, so this leaks nothing. +- **Redis clients are shared per event loop, not process-wide.** A redis-py asyncio client binds its connections to the loop that first used them, so one client handed to a second loop fails at runtime. Acquiring outside a running loop, which is the usual case since `FullAuth` is built at import time, still shares one client per URL. + +### Security + +- **Deleting an account and setting a first password now require recent authentication.** A valid access token said a session existed, not that whoever held it had proved anything, so a token lifted from a logged-in machine could delete the account outright, or give an OAuth-only account a password and a second way in. Both now need either `current_password` or a session whose credentials were checked within `REAUTH_MAX_AGE_SECONDS`, and answer `403` otherwise. The age comes from `auth_time`, which rotation preserves, so refreshing a stolen token does not renew it. **Breaking:** `DELETE /me` and first-password sets from sessions older than five minutes now fail; clients should prompt for the password and send it in the body, or re-authenticate. Tokens issued before this release carry no `auth_time`, so the session counts as last authenticated when its current token was issued: an access token minted minutes ago still passes, and rotating an old refresh token carries that token's own issue time forward rather than resetting the clock. +- **A password-reset link is invalidated by a password change.** The link stayed usable for its whole TTL even after the password had been changed by another reset link or by `/change-password`, so an older mail in an inbox could undo a deliberate change. The token now carries a digest of the password hash it was issued against, and a reset whose digest no longer matches is rejected and burned. No new storage. **Migration:** reset links issued before the upgrade stop working, up to `PASSWORD_RESET_EXPIRE_MINUTES` (15 by default) after deploy. +- **Removing a sign-in method can no longer lock an account out.** `DELETE /oauth/accounts/{provider}` counted a password and other providers but not passkeys, so it told a passkey user to "set a password first"; `DELETE /passkeys/{id}` had no such check at all, so a passkey-only account could delete its last credential and never get back in. Both routes now ask one shared question, `remaining_login_methods()`, which counts a stored password, every linked provider, and every registered passkey, and the `400` names only the alternatives the deployment actually offers. +- **A link state is bound to the user it was issued for.** The state for a link flow carries that user's id and the callback rejects it unless it matches the authenticated caller, so an attacker who finishes their own provider flow cannot have a victim submit the result and end up attached to the victim's account. The id travels in a claim rather than the token subject, since the state passes through the address bar and the provider and has to stay useless as a session token. Sign-in states and link states carry different purposes and cannot be swapped. `generate_oauth_state()` and `build_authorization_url()` gained keyword-only `purpose` and `claims` parameters, both defaulted, so existing calls are unaffected. + ### Fixed +- **The admin permission routes answer 501 instead of 500 without permission models.** The admin router mounts as soon as the adapter supports roles, so `/admin/assign-permission`, `/admin/remove-permission` and `/admin/role-permissions/{role}` existed against adapters configured for roles alone, where the adapter's `RuntimeError` escaped as a 500. Role management on such an adapter is unaffected. +- **The Agent Skill listed eight admin routes that do not exist.** `references/rbac.md` and the route table in `references/api-reference.md` documented user CRUD (`/admin/users`, `/admin/users/{id}/roles`, `/admin/roles/{role}/permissions`) that the library has never shipped, and said admin routes accept `require_role("admin")` when they require `is_superuser`. Both now describe the five routes that exist, and say plainly that there is no user CRUD. +- **The in-memory passkey challenge store never evicted anything.** Challenges were removed only by completing the flow, so every abandoned one stayed for the life of the process, and `POST /passkeys/authenticate/begin` needs no account. It now sweeps expired entries on write, like the blacklist, lockout and rate-limit stores. +- **`fastapi_fullauth.routers` exports every router factory.** It listed four of the seven, so `create_oauth_router`, `create_passkey_router`, and `create_sessions_router` had to be imported from their modules even though the package documented the shorter path. +- **The OAuth router is built once, not twice.** It was the only router property without a cache, and the router builder reads it twice (once to test it, once to mount it), so every `init_app()` built two full routers with their dynamically created response models and discarded one. +- **Signing in no longer attaches a second identity from the same provider.** If two accounts at one provider verify the same email address, auto-link-by-email attached both to the same user. `DELETE /oauth/accounts/{provider}` resolves by provider alone, so unlinking deleted whichever row came first and left the other one attached, with no way to reach it: the user unlinked the provider and stayed linked to it. The second sign-in is now refused, like the link route refuses it, and answers the same generic `400 OAuth authentication failed` as every other sign-in failure. Reachable with custom or self-hosted providers; the major providers do not verify one address on two accounts. +- **An OAuth error that is not an `OAuthProviderError` no longer returns a 500.** Both OAuth callbacks caught `OAuthProviderError`, so any other `OAuthError` subclass escaped as an unhandled exception. They now catch `OAuthError`. - **`/register` no longer claims to have sent a verification email.** With `PREVENT_REGISTRATION_ENUMERATION` on (the default) it answered `202` with "a verification email has been sent", but registration never emits `send_verification_email`: that hook fires only from `POST /verify-email/request`, which requires the user to already be signed in. Unless the application sent its own mail from `after_register`, the person was told to wait for an email that was never sent. The response now reads "If this email isn't already registered, the account has been created.", which is true either way and gives an attacker no more than before. [Sending one at signup](https://mdfarhankc.github.io/fastapi-fullauth/auth/hooks/#verification-email-on-registration) is now documented as a hook recipe. **If you assert on this string in tests, update it.** -- **Corrected the guidance given when auto-link-by-email is refused.** When a provider returns an unverified email that already belongs to an account, the flow refused the link (correctly) but told the user to "link your OAuth account from account settings", which no endpoint supports. It now says to sign in with a password or verify the address with the provider. The message is internal to `OAuthProviderError`: the router still answers every OAuth failure with a generic `400 OAuth authentication failed`, so nothing about the response changes. -- **The Agent Skill described routes and flows that do not exist.** `references/rbac.md` and the route table in `references/api-reference.md` listed eight admin routes (user CRUD under `/admin/users`, role and permission routes under `/admin/roles`) that the library has never shipped, and said admin routes accept `require_role("admin")` when they require `is_superuser`. `references/oauth.md` said the OAuth callback links through the caller's session, which it does not. All three now describe what the library actually does. + +- **Corrected the guidance given when auto-link-by-email is refused.** When a provider returns an unverified email that already belongs to an account, the flow refused the link (correctly) but told the user to "link your OAuth account from account settings", which no endpoint supports. It now names the path that the link routes above make real: sign in with a password, then link the provider. The message is internal to `OAuthProviderError`: the router still answers every OAuth failure with a generic `400 OAuth authentication failed`, so nothing about the response changes. The Agent Skill's OAuth reference described the same impossible flow, claiming the callback links through the caller's session; it now documents what the callback actually does. ## 0.16.0 diff --git a/docs/api-reference.md b/docs/api-reference.md index 3165abf..5e21cc0 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -167,6 +167,8 @@ from fastapi_fullauth.exceptions import ( AccountLockedError, # too many failed attempts OAuthError, # OAuth base error OAuthProviderError, # provider-specific error + OAuthAccountAlreadyLinkedError, # identity belongs to another user + OAuthProviderAlreadyLinkedError,# user already linked this provider ) ``` diff --git a/docs/auth/hooks.md b/docs/auth/hooks.md index 29da790..0d27f86 100644 --- a/docs/auth/hooks.md +++ b/docs/auth/hooks.md @@ -48,6 +48,8 @@ fullauth.hooks.on("after_register", on_register) | `after_email_verify` | `async def(user: UserSchema)` | After email is verified | | `after_oauth_login` | `async def(user: UserSchema, provider: str, is_new_user: bool)` | After OAuth login succeeds | | `after_oauth_register` | `async def(user: UserSchema, user_info: OAuthUserInfo)` | After a new user is created via OAuth | +| `after_oauth_link` | `async def(user: UserSchema, provider: str)` | After a signed-in user links a provider | +| `after_oauth_unlink` | `async def(user: UserSchema, provider: str)` | After a signed-in user unlinks a provider | | `send_verification_email` | `async def(email: str, token: str)` | When email verification is requested | | `send_password_reset_email` | `async def(email: str, token: str)` | When password reset is requested | @@ -111,7 +113,7 @@ async def send_verification(user): token = await create_email_verification_token( fullauth.adapter, fullauth.token_engine, user.id ) - if token: # None when the user no longer exists + if token: # None when the account is already verified await mailer.send_verification(user.email, token) ``` @@ -182,6 +184,7 @@ The `hooks.on()` method uses `@overload` with Protocol types so your IDE can aut | `EmailHook` | Email events | `async def(email: str, token: str)` | | `AfterOAuthLoginHook` | `after_oauth_login` | `async def(user: UserSchema, provider: str, is_new_user: bool)` | | `AfterOAuthRegisterHook` | `after_oauth_register` | `async def(user: UserSchema, user_info: OAuthUserInfo)` | +| `AfterOAuthLinkHook` | `after_oauth_link`, `after_oauth_unlink` | `async def(user: UserSchema, provider: str)` | You can import these from `fastapi_fullauth.hooks` if you want to type-annotate your callbacks: diff --git a/docs/auth/sessions.md b/docs/auth/sessions.md index bdffd31..16dc191 100644 --- a/docs/auth/sessions.md +++ b/docs/auth/sessions.md @@ -51,6 +51,20 @@ Revoking a session signs that device out at once: its refresh token stops workin This relies on the token blacklist (`BLACKLIST_ENABLED`, on by default). Run it on Redis when you have more than one worker; an in-memory blacklist only knows about revocations made in the same process. +## Pruning old refresh tokens + +Rotation writes a row per refresh, so the table only grows. Delete the expired +rows periodically from your own scheduler: + +```python +removed = await fullauth.adapter.prune_expired_refresh_tokens() +``` + +It removes only rows whose expiry has passed. A revoked but unexpired row stays: +that row is what a replayed token is matched against, and removing it early would +turn a stolen-token replay into an ordinary rejection, leaving the family alive. +Custom adapters inherit a no-op, so override it if your storage needs pruning. + ## Using a custom adapter The built-in adapters implement session listing out of the box. A custom adapter opts in by inheriting `SessionAdapterMixin` and implementing three methods: diff --git a/docs/configuration.md b/docs/configuration.md index 040c32b..de91d81 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -148,12 +148,14 @@ If you want to be defensively explicit that no file is ever read, pass `FullAuth |--------|------|---------|-------------| | `PASSWORD_HASH_ALGORITHM` | `"argon2id" \| "bcrypt"` | `"argon2id"` | Hashing algorithm. | | `PASSWORD_MIN_LENGTH` | `int` | `8` | Minimum password length. | +| `PASSWORD_MAX_LENGTH` | `int` | `4096` | Maximum password length, so a huge body can't be turned into hashing work. bcrypt caps at 72 bytes regardless. | ### Login | Option | Type | Default | Description | |--------|------|---------|-------------| | `LOGIN_FIELD` | `str` | `"email"` | Field used for login (`"email"`, `"username"`, etc.). | +| `REAUTH_MAX_AGE_SECONDS` | `int` | `300` | How recently credentials must have been checked for a destructive action (deleting the account, setting a first password). Supplying the current password proves it too. `0` accepts only the password, which locks passwordless accounts out of those actions. | | `LOCKOUT_ENABLED` | `bool` | `True` | Enable account lockout after failed login attempts. | | `LOCKOUT_BACKEND` | `"memory" \| "redis"` | `"memory"` | Lockout storage backend. Use `"redis"` for multi-worker deployments. | | `MAX_LOGIN_ATTEMPTS` | `int` | `5` | Failed attempts before account lockout. | @@ -171,7 +173,7 @@ middleware (`RateLimitMiddleware`) is opt-in; import it from | `TRUSTED_PROXY_HEADERS` | `list[str]` | `[]` | Headers to read the real client IP from (e.g. `["X-Forwarded-For"]`). The IP is taken from the right of the chain, not the left; left-most entries are client-supplied and never trusted. | | `TRUSTED_PROXY_COUNT` | `int` | `1` | Number of trusted proxy hops in front of the app. The client IP is the entry this many positions from the right of the forwarded chain. Must be `>= 1`. Set it too high and an attacker can spoof their IP; see [Rate Limiting](security/rate-limiting.md#proxy-support). | | `AUTH_RATE_LIMIT_ENABLED` | `bool` | `True` | Enable per-route auth rate limits. | -| `AUTH_RATE_LIMITS` | `AuthRateLimits` | see below | Per-route request caps: `login=5`, `register=3`, `password_reset=3`, `passkey_auth=10`, `refresh=30`. | +| `AUTH_RATE_LIMITS` | `AuthRateLimits` | see below | Per-route request caps: `login=5`, `register=3`, `password_reset=3`, `passkey_auth=10`, `refresh=30`, `reauth=5`. `reauth` meters the `current_password` check on `DELETE /me` and `/change-password`, in its own bucket so failed attempts cannot exhaust an IP's sign-in budget. | | `AUTH_RATE_LIMIT_WINDOW_SECONDS` | `int` | `60` | Rate limit window in seconds. | Override individual routes without touching the others. In Python, pass an diff --git a/docs/migrating-from-fastapi-users.md b/docs/migrating-from-fastapi-users.md index 6fe7dd7..42f0da9 100644 --- a/docs/migrating-from-fastapi-users.md +++ b/docs/migrating-from-fastapi-users.md @@ -15,7 +15,7 @@ The good news first: **password hashes carry over unchanged, and so does your us | Passkeys (WebAuthn) | none | registration and passwordless sign-in | | Lockout, rate limiting, CSRF, security headers | none | built in | | User admin CRUD routes | `GET/PATCH/DELETE /{id}` | **not included**; write your own with `require_role`, or call the adapter | -| Link an OAuth account while signed in | `get_oauth_associate_router` | **not yet**; sign-in linking is automatic on a provider-verified email | +| Link an OAuth account while signed in | `get_oauth_associate_router` | `/oauth/{provider}/link/authorize` and `/link/callback`; sign-in linking on a provider-verified email is automatic on top | | MFA / TOTP | none | none | Read the [threat model](security/threat-model.md) before you commit: it states plainly what the library defends against and what stays your responsibility. @@ -58,7 +58,8 @@ Your fastapi-users prefixes were whatever you passed to `include_router`; the ta | `GET/PATCH/DELETE /users/{id}` | not included | build your own, see below | | `GET /auth/{provider}/authorize` | `GET /oauth/{provider}/authorize` | returns `authorization_url` **and a `binding`** your client must store | | `GET /auth/{provider}/callback` | `POST /oauth/{provider}/callback` | JSON body `{code, state, binding}` | -| `get_oauth_associate_router` | not yet | | +| `get_oauth_associate_router` `/associate/{provider}/authorize` | `GET /oauth/{provider}/link/authorize` | authenticated; returns `authorization_url` and a `binding` | +| `get_oauth_associate_router` `/associate/{provider}/callback` | `POST /oauth/{provider}/link/callback` | authenticated; JSON body `{code, state, binding}`; returns the linked account, not tokens | | none | `GET /sessions`, `DELETE /sessions/{family_id}`, `POST /sessions/revoke-others` | new | | none | `/passkeys/*`, `/admin/assign-role`, `/admin/assign-permission` | new | diff --git a/docs/oauth.md b/docs/oauth.md index e668969..f22630d 100644 --- a/docs/oauth.md +++ b/docs/oauth.md @@ -60,6 +60,8 @@ When OAuth providers are configured, these routes are registered automatically: | GET | `/auth/oauth/providers` | List configured providers | | GET | `/auth/oauth/{provider}/authorize` | Get authorization URL | | POST | `/auth/oauth/{provider}/callback` | Exchange code for tokens | +| GET | `/auth/oauth/{provider}/link/authorize` | Authorization URL for linking to the signed-in account | +| POST | `/auth/oauth/{provider}/link/callback` | Link that provider account to the signed-in account | | GET | `/auth/oauth/accounts` | List linked OAuth accounts | | DELETE | `/auth/oauth/accounts/{provider}` | Unlink a provider | @@ -134,7 +136,7 @@ From this point on, the session works exactly like email/password login. The use ## Auto-linking by email -By default, if a user registers with `user@example.com` via email/password, then later logs in with Google using the same verified email, the accounts are linked automatically. Disable this with: +By default, if a user registers with `user@example.com` via email/password, then later logs in with Google using the same verified email, the accounts are linked automatically. If the provider does *not* report the address as verified, the sign-in is refused rather than linked, and the user has to sign in with their password and [link the provider explicitly](#linking-a-provider-while-signed-in). A sign-in that would attach a *second* account from a provider the user already linked is refused for the same reason unlinking gives: one identity per provider per account. Disable auto-linking with: ```python config = FullAuthConfig( @@ -143,12 +145,79 @@ config = FullAuthConfig( ) ``` +## Linking a provider while signed in + +Auto-linking only fires when the provider reports the email as verified *and* it already matches +an account. That leaves out the ordinary cases: a work GitHub account under a different address, a +provider that does not verify emails, a provider that returns no email at all. For those, a +signed-in user links the provider explicitly. + +The flow mirrors signing in, with two differences: both requests carry the user's access token, and +the callback returns the linked account instead of a token pair. Linking is not a sign-in, so no +session is created and nothing about the current session changes. + +``` +GET /api/v1/auth/oauth/github/link/authorize?redirect_uri=https://app.example.com/settings/callback +Authorization: Bearer +``` + +```json +{ + "authorization_url": "https://github.com/login/oauth/authorize?...", + "binding": "kZ3..." +} +``` + +Store the `binding` exactly as you do when signing in, send the user to `authorization_url`, then +post what the provider sends back: + +``` +POST /api/v1/auth/oauth/github/link/callback +Authorization: Bearer + +{"code": "...", "state": "...", "binding": "kZ3..."} +``` + +```json +{"provider": "github", "provider_user_id": "1234567", "provider_email": "work@example.com"} +``` + +The provider's email is stored for display only. Authorization comes from the session, so the +address does not have to match the account, and linking never marks the account verified or edits +the user row. + +Two conflicts answer `409` with a message you can show the user: + +- the provider account is already linked to a **different** user. It is never moved: that would let + anyone attach a victim's identity and then sign in as them. +- the user already linked a **different** account from the same provider. Unlinking is keyed by + provider alone, so a second one would make `DELETE /auth/oauth/accounts/{provider}` ambiguous. + Unlink the first one, then link again. + +Linking an account the user already has just refreshes the stored provider tokens and returns +`200`. + +Two hooks cover the pair, for audit logging: + +```python +@fullauth.hooks.on("after_oauth_link") +async def on_link(user, provider): + audit.record(user.id, f"linked {provider}") + + +@fullauth.hooks.on("after_oauth_unlink") +async def on_unlink(user, provider): + audit.record(user.id, f"unlinked {provider}") +``` + ## Security model **State token**: the OAuth `state` parameter is a purpose-scoped JWT with a 5-minute TTL (`OAUTH_STATE_EXPIRE_SECONDS`). If it is missing, expired, or tampered with, the callback is rejected. With the token blacklist enabled (the default) it is single-use. **Binding to the browser (login CSRF)**: a signed state alone does not stop login CSRF. An attacker can start a login with their own provider account, then make a victim's browser submit the attacker's `code` and `state`, signing the victim into the attacker's account. RFC 9700 (OAuth 2.0 Security Best Current Practice) therefore requires the state to be bound to the user agent. The authorize endpoint returns a random `binding` and puts only its SHA-256 hash in the state; the callback requires the `binding` back and compares it in constant time before touching the code. The victim's browser never had the attacker's binding, so the forged callback fails. +**Linking is bound to the user who started it**: the state for a link flow carries the id of the user it was issued for, and the callback rejects it unless that matches the authenticated caller. Without this, an attacker who completed their own provider flow could have a victim submit the result and end up attached to the victim's account. The id travels in a claim, never as the token's subject: the state passes through the address bar and the provider, so it must stay useless as a session token. A sign-in state and a link state carry different purposes and cannot be swapped. + **Redirect URI validation**: the library validates the `redirect_uri` parameter against the provider's configured `redirect_uris` list. Mismatched URIs are rejected with a 400 error. **Token storage**: provider access and refresh tokens are stored in the `oauth_accounts` table and updated on each login. @@ -174,7 +243,9 @@ Users can unlink an OAuth provider: DELETE /api/v1/auth/oauth/accounts/google ``` -This is blocked if the OAuth account is the user's only login method (no password set, no other OAuth providers). The user must set a password first. +This is blocked if it would leave the account with no way to sign in. A stored password, any other linked provider, and any registered passkey all count, so a passwordless user with a passkey can unlink freely. + +To add one back, see [Linking a provider while signed in](#linking-a-provider-while-signed-in). ## Adding your own provider @@ -215,7 +286,7 @@ If your provider deviates from the standard flow entirely, subclass the bare `OA ## Event hooks -Two hooks fire during OAuth flows: +Four hooks fire during OAuth flows: - `after_oauth_login` fires on every OAuth login (new and returning users): @@ -234,6 +305,8 @@ async def on_oauth_register(user, user_info): print(f"New OAuth user: {user.email}, provider data: {user_info.raw}") ``` +- `after_oauth_link` and `after_oauth_unlink` fire when a signed-in user attaches or removes a provider, both with `(user, provider)`. See [Linking a provider while signed in](#linking-a-provider-while-signed-in). + The `after_register` hook also fires for new OAuth users. ## Provider setup guides diff --git a/docs/passkeys.md b/docs/passkeys.md index d4af80d..f11b572 100644 --- a/docs/passkeys.md +++ b/docs/passkeys.md @@ -155,6 +155,12 @@ The server verifies the assertion, checks the sign count (see [Clone detection]( !!! note When an email is provided to `authenticate/begin`, the response always includes an `allowCredentials` list (possibly empty). Callers can't distinguish unknown emails from known ones with no passkeys. This prevents user enumeration. +## Deleting a passkey + +`DELETE /auth/passkeys/{id}` refuses with `400` when that passkey is the account's +only way to sign in, the same rule [unlinking a provider](oauth.md#unlinking-providers) +follows. Set a password or register a second passkey first. + ## Frontend integration Here's a minimal JavaScript example for registration and authentication: diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 0da1b12..004b190 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -41,6 +41,16 @@ Some backends must choose between availability and strictness when Redis is unre - **Token blacklist fails closed.** If revocation cannot be checked, the token is rejected. A Redis outage signs users out rather than letting revoked tokens through. - **Rate limiter fails open.** Requests are allowed and the error is logged. A Redis outage must not lock every user out of login. +- **Account lockout fails open.** Attempts are neither counted nor blocked while Redis is unreachable, and the error is logged. Failing closed would lock every account out, and raising would turn every login into a 500; brute-force protection is lost for the duration. + +## Destructive actions require recent authentication + +Deleting an account and setting a first password on an account that has none both +require proof that the person is present, not just that a session exists: either +the current password, or credentials checked within `REAUTH_MAX_AGE_SECONDS` +(5 minutes by default). The age comes from an `auth_time` claim stamped when +credentials are checked and carried unchanged through refresh rotation, so +refreshing a stolen token does not renew it. ## Your application's responsibilities @@ -54,6 +64,6 @@ Some backends must choose between availability and strictness when Redis is unre These are tracked for future releases: - OAuth provider access and refresh tokens are stored in plaintext. -- Sensitive actions (account deletion, setting a first password on an OAuth-only account) do not require recent re-authentication. -- Expired and revoked refresh-token rows are not pruned automatically. - Tokens carry no `iss` or `aud` claim, so services sharing a `SECRET_KEY` accept each other's tokens. +- Pruning expired refresh tokens is available (`adapter.prune_expired_refresh_tokens()`) but has to be scheduled by the application; nothing runs it for you. +- A password-reset link is invalidated by a password change, but not by a later reset *request*: the newest link and any still-unused older one both work until one of them is used. diff --git a/fastapi_fullauth/adapters/_sqlalchemy_base.py b/fastapi_fullauth/adapters/_sqlalchemy_base.py index f24cf60..53bb985 100644 --- a/fastapi_fullauth/adapters/_sqlalchemy_base.py +++ b/fastapi_fullauth/adapters/_sqlalchemy_base.py @@ -16,7 +16,7 @@ from typing import Any, TypeVar, cast from uuid import UUID -from sqlalchemy import CursorResult, select, update +from sqlalchemy import CursorResult, delete, select, update from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from sqlalchemy.orm import selectinload @@ -441,6 +441,20 @@ async def revoke_all_user_refresh_tokens(self, user_id: UserID) -> None: ) await self._commit(session) + async def prune_expired_refresh_tokens(self, before: datetime | None = None) -> int: + cutoff = before or datetime.now(timezone.utc) + async with self._begin() as session: + result = cast( + "CursorResult[Any]", + await session.execute( + delete(self._refresh_token_model).where( + self._refresh_token_model.expires_at < cutoff + ) + ), + ) + await self._commit(session) + return int(result.rowcount) + async def list_user_sessions(self, user_id: UserID) -> list[SessionInfo]: model = self._refresh_token_model now = datetime.now(timezone.utc) diff --git a/fastapi_fullauth/adapters/base.py b/fastapi_fullauth/adapters/base.py index f76f98c..93a75eb 100644 --- a/fastapi_fullauth/adapters/base.py +++ b/fastapi_fullauth/adapters/base.py @@ -1,6 +1,7 @@ from abc import ABC, abstractmethod from collections.abc import AsyncIterator from contextlib import asynccontextmanager +from datetime import datetime from functools import cache from types import UnionType from typing import Any, Generic, Literal, Protocol, TypeVar, Union, cast, get_args, get_origin @@ -152,6 +153,20 @@ async def revoke_all_user_refresh_tokens(self, user_id: UserID) -> None: ... @abstractmethod async def set_user_verified(self, user_id: UserID) -> None: ... + async def prune_expired_refresh_tokens(self, before: "datetime | None" = None) -> int: + """Delete refresh tokens that expired before ``before`` (default: now). + + Rotation writes a row per refresh, so the table only grows; call this + periodically from your own scheduler. Only expired rows go: a revoked but + unexpired row is what reuse detection matches a replayed token against, + and deleting it early would turn a stolen-token replay into an ordinary + rejection, leaving the family alive. + + Returns the number of rows removed. The default removes nothing, so + custom adapters keep working; the bundled adapters override it. + """ + return 0 + async def get_user_roles(self, user_id: UserID) -> list[str]: """Get user's roles. Returns [] by default. Override or use RoleAdapterMixin.""" return [] diff --git a/fastapi_fullauth/adapters/beanie.py b/fastapi_fullauth/adapters/beanie.py index cc099c1..b6f7fa0 100644 --- a/fastapi_fullauth/adapters/beanie.py +++ b/fastapi_fullauth/adapters/beanie.py @@ -292,6 +292,12 @@ async def revoke_all_user_refresh_tokens(self, user_id: UserID) -> None: {"$set": {"revoked": True}} ) + async def prune_expired_refresh_tokens(self, before: datetime | None = None) -> int: + cutoff = before or datetime.now(timezone.utc) + model = self._refresh_token_model + result = await model.find(model.expires_at < cutoff).delete() + return int(getattr(result, "deleted_count", 0) or 0) + # ── Sessions ───────────────────────────────────────────────────── async def list_user_sessions(self, user_id: UserID) -> list[SessionInfo]: diff --git a/fastapi_fullauth/adapters/tortoise.py b/fastapi_fullauth/adapters/tortoise.py index 4b8d843..33fcf5a 100644 --- a/fastapi_fullauth/adapters/tortoise.py +++ b/fastapi_fullauth/adapters/tortoise.py @@ -292,6 +292,11 @@ async def revoke_refresh_token_family(self, family_id: str) -> None: async def revoke_all_user_refresh_tokens(self, user_id: UserID) -> None: await self._refresh_token_model.filter(user_id=user_id).update(revoked=True) + async def prune_expired_refresh_tokens(self, before: datetime | None = None) -> int: + cutoff = before or datetime.now(timezone.utc) + deleted = await self._refresh_token_model.filter(expires_at__lt=cutoff).delete() + return int(deleted or 0) + # ── Sessions ───────────────────────────────────────────────────── async def list_user_sessions(self, user_id: UserID) -> list[SessionInfo]: diff --git a/fastapi_fullauth/config.py b/fastapi_fullauth/config.py index 862316f..7fa75bc 100644 --- a/fastapi_fullauth/config.py +++ b/fastapi_fullauth/config.py @@ -35,6 +35,10 @@ class AuthRateLimits: password_reset: int = 3 passkey_auth: int = 10 refresh: int = 30 + # Checking `current_password` on a destructive action is a password + # oracle for whoever holds the access token, so it is metered. Its own + # bucket, so failed attempts cannot exhaust an IP's sign-in budget. + reauth: int = 5 class FullAuthConfig(BaseSettings): @@ -67,6 +71,9 @@ class FullAuthConfig(BaseSettings): PASSWORD_HASH_ALGORITHM: Literal["argon2id", "bcrypt"] = "argon2id" PASSWORD_MIN_LENGTH: int = 8 + # Upper bound so an attacker can't spend the server's CPU hashing a huge + # body. bcrypt caps at 72 bytes anyway; argon2 will hash whatever it is given. + PASSWORD_MAX_LENGTH: int = 4096 LOGIN_FIELD: str = "email" # When True, login runs a dummy password verify on unknown-user / no-password @@ -75,6 +82,12 @@ class FullAuthConfig(BaseSettings): # than hiding which emails are registered. PREVENT_LOGIN_TIMING_ATTACKS: bool = True + # How recently the account's credentials must have been checked for a + # destructive action (deleting the account, setting a first password). + # Supplying the current password proves it too. 0 accepts only the password, + # which locks passwordless accounts out of those actions. + REAUTH_MAX_AGE_SECONDS: int = 300 + LOCKOUT_ENABLED: bool = True LOCKOUT_BACKEND: Literal["memory", "redis"] = "memory" MAX_LOGIN_ATTEMPTS: int = 5 diff --git a/fastapi_fullauth/core/_redis.py b/fastapi_fullauth/core/_redis.py index f5df04e..9b8c237 100644 --- a/fastapi_fullauth/core/_redis.py +++ b/fastapi_fullauth/core/_redis.py @@ -2,22 +2,52 @@ Several backends can point at the same Redis server (token blacklist, lockout, rate limiters, challenge store); giving each its own client would open one -connection pool per backend. Instead, backends acquire a shared per-URL client -here and release it in ``aclose()``; the client closes when the last holder -releases it. redis-py connects lazily, so acquiring never touches the network. +connection pool per backend. Instead, backends acquire a shared client here and +release it in ``aclose()``; the client closes when the last holder releases it. +redis-py connects lazily, so acquiring never touches the network. + +Clients are shared per (url, event loop). A redis-py asyncio client binds its +connections to the loop that first used them, so handing one client to a second +loop fails at runtime. The key holds the loop *object*, never ``id(loop)``: ids +of closed loops are recycled almost immediately, so an id-keyed entry would +answer for an unrelated new loop with a client bound to the dead one. """ +import asyncio +from dataclasses import dataclass from typing import TYPE_CHECKING if TYPE_CHECKING: from redis.asyncio import Redis -_clients: dict[str, "Redis"] = {} -_refcounts: dict[str, int] = {} +# The loop is part of the key, which also keeps it alive for as long as an entry +# refers to it. Acquiring outside a running loop (the usual case, since FullAuth +# is built at import time) uses None and shares one client per url. +RedisClientKey = tuple[str, "asyncio.AbstractEventLoop | None"] + + +@dataclass +class _Holder: + client: "Redis" + count: int + +_clients: dict[RedisClientKey, _Holder] = {} + + +def _current_loop() -> "asyncio.AbstractEventLoop | None": + try: + return asyncio.get_running_loop() + except RuntimeError: + return None -def acquire_redis(redis_url: str, *, feature: str) -> "Redis": - """Return the shared client for ``redis_url``, creating it on first use. + +def acquire_redis(redis_url: str, *, feature: str) -> tuple["Redis", RedisClientKey]: + """Return the shared client for ``redis_url`` plus the key to release it with. + + Release with exactly the returned key: re-deriving it at close time would + pick the wrong entry when the releasing loop is not the acquiring one, and + could close a client another holder is still using. ``feature`` names the caller for the ImportError message when the optional redis dependency is missing. @@ -30,21 +60,31 @@ def acquire_redis(redis_url: str, *, feature: str) -> "Redis": "Install it with: pip install fastapi-fullauth[redis]" ) from None - client = _clients.get(redis_url) - if client is None: - client = aioredis.from_url(redis_url, decode_responses=True) - _clients[redis_url] = client - _refcounts[redis_url] = _refcounts.get(redis_url, 0) + 1 - return client + loop = _current_loop() + key: RedisClientKey = (redis_url, loop) + holder = _clients.get(key) + + if holder is not None and loop is not None and loop.is_closed(): + # Same loop object, but it has been closed since: its connections are + # unusable. Start again rather than hand back a dead client. + _clients.pop(key, None) + holder = None + if holder is None: + holder = _Holder(client=aioredis.from_url(redis_url, decode_responses=True), count=0) + _clients[key] = holder -async def release_redis(redis_url: str) -> None: - """Release one hold on the shared client; close it when none remain.""" - count = _refcounts.get(redis_url, 0) - if count > 1: - _refcounts[redis_url] = count - 1 + holder.count += 1 + return holder.client, key + + +async def release_redis(key: RedisClientKey) -> None: + """Release one hold on a shared client; close it when none remain.""" + holder = _clients.get(key) + if holder is None: + return + holder.count -= 1 + if holder.count > 0: return - _refcounts.pop(redis_url, None) - client = _clients.pop(redis_url, None) - if client is not None: - await client.aclose() + _clients.pop(key, None) + await holder.client.aclose() diff --git a/fastapi_fullauth/core/blacklist.py b/fastapi_fullauth/core/blacklist.py index a54b8d4..69f296d 100644 --- a/fastapi_fullauth/core/blacklist.py +++ b/fastapi_fullauth/core/blacklist.py @@ -76,10 +76,12 @@ async def is_blacklisted(self, jti: str) -> bool: class RedisTokenBlacklist(TokenBlacklist): def __init__(self, redis_url: str, default_ttl_seconds: int = 1800) -> None: - from fastapi_fullauth.core._redis import acquire_redis + from fastapi_fullauth.core._redis import RedisClientKey, acquire_redis - self._redis = acquire_redis(redis_url, feature="the Redis blacklist backend") - self._redis_url: str | None = redis_url + self._redis_key: RedisClientKey | None + self._redis, self._redis_key = acquire_redis( + redis_url, feature="the Redis blacklist backend" + ) self._default_ttl = default_ttl_seconds self._prefix = "fullauth:blacklist:" @@ -112,6 +114,8 @@ async def is_any_blacklisted(self, *keys: str) -> bool: async def aclose(self) -> None: from fastapi_fullauth.core._redis import release_redis - if self._redis_url is not None: - await release_redis(self._redis_url) - self._redis_url = None + # Released with the key acquire returned, never a re-derived one: + # the closing loop may not be the acquiring one. + if self._redis_key is not None: + await release_redis(self._redis_key) + self._redis_key = None diff --git a/fastapi_fullauth/core/tokens.py b/fastapi_fullauth/core/tokens.py index 79bc815..cbb8bd4 100644 --- a/fastapi_fullauth/core/tokens.py +++ b/fastapi_fullauth/core/tokens.py @@ -34,6 +34,7 @@ def create_access_token( extra: dict[str, Any] | None = None, expire_seconds: int | None = None, family_id: str | None = None, + auth_time: datetime | None = None, ) -> str: if self.config.SECRET_KEY is None: raise RuntimeError("SECRET_KEY must be set to create tokens") @@ -55,12 +56,15 @@ def create_access_token( # device the caller is currently on. Omitted when unknown. if family_id is not None: payload["family_id"] = family_id + if auth_time is not None: + payload["auth_time"] = int(auth_time.timestamp()) return jwt.encode(payload, self.config.SECRET_KEY, algorithm=self.config.ALGORITHM) def create_refresh_token( self, user_id: str, family_id: str | None = None, + auth_time: datetime | None = None, ) -> RefreshTokenMeta: if self.config.SECRET_KEY is None: raise RuntimeError("SECRET_KEY must be set to create tokens") @@ -75,6 +79,10 @@ def create_refresh_token( "type": "refresh", "family_id": resolved_family_id, } + # Carried on the refresh token too, so rotation can hand the original + # credential-check time to the next access token. + if auth_time is not None: + payload["auth_time"] = int(auth_time.timestamp()) token = jwt.encode(payload, self.config.SECRET_KEY, algorithm=self.config.ALGORITHM) return RefreshTokenMeta(token=token, expires_at=expires_at, family_id=resolved_family_id) @@ -112,6 +120,15 @@ async def decode_token( jti = data.get("jti", "") family_id = data.get("family_id") + auth_time: datetime | None = None + raw_auth_time = data.get("auth_time") + if isinstance(raw_auth_time, int | float): + try: + auth_time = datetime.fromtimestamp(raw_auth_time, tz=timezone.utc) + except (OverflowError, OSError, ValueError): + # Out of range for a timestamp. Treat it as absent rather than + # raising: the caller then asks for re-authentication. + logger.warning("Token carries an unusable auth_time claim") if self.config.BLACKLIST_ENABLED: # A token dies with its session: revoking the family blacklists every # token issued to it, not only the one presented at logout. @@ -145,6 +162,7 @@ async def decode_token( roles=data.get("roles", []), extra=extra, family_id=family_id, + auth_time=auth_time, ) async def blacklist_token(self, jti: str, ttl_seconds: int | None = None) -> None: @@ -181,11 +199,14 @@ def create_token_pair( roles: list[str] | None = None, extra: dict[str, Any] | None = None, family_id: str | None = None, + auth_time: datetime | None = None, ) -> tuple[str, RefreshTokenMeta]: # Create the refresh token first so its resolved family_id (freshly # minted when none was passed) can be stamped onto the access token too. - refresh = self.create_refresh_token(user_id, family_id) - access = self.create_access_token(user_id, roles, extra, family_id=refresh.family_id) + refresh = self.create_refresh_token(user_id, family_id, auth_time=auth_time) + access = self.create_access_token( + user_id, roles, extra, family_id=refresh.family_id, auth_time=auth_time + ) return access, refresh diff --git a/fastapi_fullauth/exceptions.py b/fastapi_fullauth/exceptions.py index c2b5276..61c8cc6 100644 --- a/fastapi_fullauth/exceptions.py +++ b/fastapi_fullauth/exceptions.py @@ -13,6 +13,7 @@ "NoValidFieldsError", "OAuthAccountAlreadyLinkedError", "OAuthError", + "OAuthProviderAlreadyLinkedError", "OAuthProviderError", "RefreshTokenReuseError", "TokenBlacklistedError", @@ -80,6 +81,10 @@ class OAuthAccountAlreadyLinkedError(OAuthError): """The provider identity is already linked to a different user.""" +class OAuthProviderAlreadyLinkedError(OAuthError): + """The user already linked a different account from the same provider.""" + + class NoValidFieldsError(FullAuthError): """A profile update contained no updatable fields.""" diff --git a/fastapi_fullauth/flows/__init__.py b/fastapi_fullauth/flows/__init__.py index 003df55..e3774fc 100644 --- a/fastapi_fullauth/flows/__init__.py +++ b/fastapi_fullauth/flows/__init__.py @@ -1,30 +1,41 @@ from fastapi_fullauth.flows.change_password import change_password +from fastapi_fullauth.flows.credentials import last_method_detail, remaining_login_methods from fastapi_fullauth.flows.email_verify import create_email_verification_token, verify_email from fastapi_fullauth.flows.login import login from fastapi_fullauth.flows.logout import logout, logout_with_refresh_token from fastapi_fullauth.flows.oauth import ( + build_link_authorization_url, exchange_oauth_code, issue_oauth_tokens, + link_oauth_account, link_or_create_user, + oauth_link_callback, ) from fastapi_fullauth.flows.password_reset import request_password_reset, reset_password from fastapi_fullauth.flows.profile import validate_profile_updates +from fastapi_fullauth.flows.reauth import verify_recent_auth from fastapi_fullauth.flows.refresh import refresh from fastapi_fullauth.flows.register import register __all__ = [ + "build_link_authorization_url", "change_password", "create_email_verification_token", "exchange_oauth_code", "issue_oauth_tokens", + "last_method_detail", + "link_oauth_account", "link_or_create_user", "login", "logout", "logout_with_refresh_token", + "oauth_link_callback", "refresh", "register", + "remaining_login_methods", "request_password_reset", "reset_password", "validate_profile_updates", + "verify_recent_auth", "verify_email", ] diff --git a/fastapi_fullauth/flows/credentials.py b/fastapi_fullauth/flows/credentials.py new file mode 100644 index 0000000..05dca62 --- /dev/null +++ b/fastapi_fullauth/flows/credentials.py @@ -0,0 +1,73 @@ +"""The rule for removing one of a user's sign-in methods. + +Unlinking an OAuth provider and deleting a passkey are the same question asked +twice: would this leave the account with no way back in? Both routes ask it here +so they cannot drift apart. +""" + +from collections.abc import Collection +from uuid import UUID + +from fastapi_fullauth.adapters.base import ( + AbstractUserAdapter, + OAuthAdapterMixin, + PasskeyAdapterMixin, +) +from fastapi_fullauth.types import UserID + + +async def remaining_login_methods( + adapter: AbstractUserAdapter, + user_id: UserID, + *, + usable_providers: Collection[str], + passkeys_usable: bool, + without_provider: str | None = None, + without_passkey_id: UUID | None = None, +) -> int: + """How many ways the user could still sign in once the named credential is gone. + + Counts only what this deployment can actually serve. A stored credential for + a disabled feature is not a way back in: a passkey row survives turning + ``PASSKEY_ENABLED`` off, and an OAuth account survives dropping its provider + from the configured list, but neither can be signed in with. Counting those + would allow exactly the lockout this check exists to prevent, so the caller + passes the providers it has configured and whether passkey sign-in is live. + """ + count = 0 + if await adapter.get_hashed_password(user_id) is not None: + count += 1 + + if ( + usable_providers + and adapter.supports_feature("oauth") + and isinstance(adapter, OAuthAdapterMixin) + ): + count += sum( + 1 + for account in await adapter.get_user_oauth_accounts(user_id) + if account.provider != without_provider and account.provider in usable_providers + ) + + if ( + passkeys_usable + and adapter.supports_feature("passkey") + and isinstance(adapter, PasskeyAdapterMixin) + ): + count += sum( + 1 + for passkey in await adapter.get_user_passkeys(user_id) + if passkey.id != without_passkey_id + ) + + return count + + +def last_method_detail(*, passkeys_available: bool) -> str: + """The 400 body for refusing to remove the last sign-in method. + + Names only the alternatives this deployment actually offers, so it never + tells someone to do something the app does not support. + """ + alternative = " or add a passkey" if passkeys_available else "" + return f"Cannot remove your only sign-in method. Set a password{alternative} first." diff --git a/fastapi_fullauth/flows/email_verify.py b/fastapi_fullauth/flows/email_verify.py index a34cd81..0e93a93 100644 --- a/fastapi_fullauth/flows/email_verify.py +++ b/fastapi_fullauth/flows/email_verify.py @@ -15,10 +15,17 @@ async def create_email_verification_token( token_engine: TokenEngine, user_id: UserID, ) -> str | None: - """Generate an email verification token. Returns None if user not found.""" + """Generate an email verification token. + + Returns None when there is nothing to verify: no such user, or the address is + already verified. Callers answer the same either way, so this leaks nothing. + """ user = await adapter.get_user_by_id(user_id) if user is None: return None + if user.is_verified: + logger.debug("Verification token not issued; already verified: user_id=%s", user_id) + return None token = token_engine.create_access_token( user_id=str(user.id), diff --git a/fastapi_fullauth/flows/login.py b/fastapi_fullauth/flows/login.py index 25f4e40..2fd06a0 100644 --- a/fastapi_fullauth/flows/login.py +++ b/fastapi_fullauth/flows/login.py @@ -43,6 +43,7 @@ async def login( prevent_timing_attacks: bool = False, user_agent: str | None = None, ip_address: str | None = None, + max_password_length: int = 0, ) -> TokenPair: # Adapters normalise emails before lookup; key the lockout on the same value # so case or whitespace variants of one address share a single counter. @@ -52,6 +53,15 @@ async def login( logger.warning("Login blocked; account locked: %s", identifier) raise AccountLockedError("Account is temporarily locked") + # Rejected before any hashing: this is the unauthenticated path, so an + # oversized body would otherwise be turned into hashing work on demand. It + # still counts as a failed attempt, so spraying long strings is not free. + if max_password_length and len(password) > max_password_length: + if lockout: + await lockout.record_failure(lockout_key) + logger.warning("Login failed; password exceeds the configured maximum length") + raise AuthenticationError("Invalid credentials") + if user is None: user = await adapter.get_user_by_field(login_field, identifier) diff --git a/fastapi_fullauth/flows/oauth.py b/fastapi_fullauth/flows/oauth.py index 89fb76b..28be2e0 100644 --- a/fastapi_fullauth/flows/oauth.py +++ b/fastapi_fullauth/flows/oauth.py @@ -7,13 +7,32 @@ from fastapi_fullauth.adapters.base import AbstractUserAdapter, OAuthAdapterMixin from fastapi_fullauth.core.tokens import TokenEngine -from fastapi_fullauth.exceptions import OAuthProviderError, UserAlreadyExistsError +from fastapi_fullauth.exceptions import ( + OAuthAccountAlreadyLinkedError, + OAuthProviderAlreadyLinkedError, + OAuthProviderError, + UserAlreadyExistsError, +) from fastapi_fullauth.flows.tokens import issue_token_pair from fastapi_fullauth.oauth.base import OAuthProvider -from fastapi_fullauth.types import OAuthAccount, OAuthUserInfo, TokenPair, TokenPayload, UserSchema +from fastapi_fullauth.types import ( + OAuthAccount, + OAuthUserInfo, + TokenPair, + TokenPayload, + UserID, + UserSchema, +) logger = logging.getLogger("fastapi_fullauth.oauth") +# Purposes keep the two flows apart: a sign-in state cannot be redeemed at the +# link callback, and a link state cannot sign anyone in. +STATE_PURPOSE = "oauth_state" +LINK_STATE_PURPOSE = "oauth_link" + +_RESERVED_STATE_CLAIMS = frozenset({"purpose", "nonce", "binding", "redirect_uri"}) + def _b64url(data: bytes) -> str: return base64.urlsafe_b64encode(data).rstrip(b"=").decode("ascii") @@ -59,12 +78,28 @@ def generate_oauth_state( nonce: str | None = None, *, binding: str, + purpose: str = STATE_PURPOSE, + claims: dict[str, Any] | None = None, ) -> str: + """Create the signed, user-agent-bound state token for an OAuth flow. + + ``claims`` are carried in the state and returned at the callback. The claims + the state's own security rests on cannot be set this way. + + The subject is always ``"oauth-state"``, never a real user id: the state + travels through the browser's address bar and the provider, so it must be + worthless as a session token. + """ extra: dict[str, Any] = { - "purpose": "oauth_state", - "nonce": nonce or secrets.token_hex(16), - "binding": _binding_digest(binding), + k: v for k, v in (claims or {}).items() if k not in _RESERVED_STATE_CLAIMS } + extra.update( + { + "purpose": purpose, + "nonce": nonce or secrets.token_hex(16), + "binding": _binding_digest(binding), + } + ) if redirect_uri: extra["redirect_uri"] = redirect_uri return token_engine.create_access_token( @@ -80,6 +115,8 @@ def build_authorization_url( pkce_enabled: bool = True, *, binding: str, + purpose: str = STATE_PURPOSE, + claims: dict[str, Any] | None = None, ) -> str: """Create a signed state token and return the provider authorization URL. @@ -92,7 +129,13 @@ def build_authorization_url( """ nonce = secrets.token_hex(16) state = generate_oauth_state( - token_engine, ttl_seconds, redirect_uri, nonce=nonce, binding=binding + token_engine, + ttl_seconds, + redirect_uri, + nonce=nonce, + binding=binding, + purpose=purpose, + claims=claims, ) secret = token_engine.config.SECRET_KEY @@ -102,9 +145,40 @@ def build_authorization_url( return provider.get_authorization_url(state, redirect_uri) -async def _decode_bound_state(token_engine: TokenEngine, state: str, binding: str) -> TokenPayload: +def build_link_authorization_url( + token_engine: TokenEngine, + provider: OAuthProvider, + redirect_uri: str, + user_id: UserID, + ttl_seconds: int = 300, + pkce_enabled: bool = True, + *, + binding: str, +) -> str: + """Authorization URL for attaching ``provider`` to an already-signed-in user. + + The state names the user it was issued for, which the callback checks against + the caller's session. Without that check, an attacker could run this flow with + their own provider account and have a victim submit the result, attaching the + attacker's identity to the victim's account. + """ + return build_authorization_url( + token_engine, + provider, + redirect_uri, + ttl_seconds, + pkce_enabled, + binding=binding, + purpose=LINK_STATE_PURPOSE, + claims={"link_user_id": str(user_id)}, + ) + + +async def _decode_bound_state( + token_engine: TokenEngine, state: str, binding: str, *, purpose: str = STATE_PURPOSE +) -> TokenPayload: payload = await token_engine.decode_token(state, expected_type="access") - if payload.extra.get("purpose") != "oauth_state": + if payload.extra.get("purpose") != purpose: logger.warning("Invalid OAuth state token (wrong purpose)") raise OAuthProviderError("Invalid OAuth state token") expected = payload.extra.get("binding") @@ -133,7 +207,17 @@ async def exchange_oauth_code( # Checked before the state is burned, so a mismatched attempt cannot use up # the legitimate client's state. payload = await _decode_bound_state(token_engine, state, binding) + return await _burn_state_and_exchange(provider, token_engine, payload, code, pkce_enabled) + +async def _burn_state_and_exchange( + provider: OAuthProvider, + token_engine: TokenEngine, + payload: TokenPayload, + code: str, + pkce_enabled: bool, +) -> tuple[dict[str, Any], OAuthUserInfo]: + """Consume an already-verified state, then exchange the code for user info.""" # Single-use: burn the state so a captured (code, state) pair can't be # replayed within the state's TTL. Decoding it again raises TokenBlacklisted. if token_engine.config.BLACKLIST_ENABLED: @@ -164,6 +248,30 @@ def _ensure_active(user: UserSchema) -> None: raise OAuthProviderError("User account is deactivated") +async def _ensure_provider_not_linked( + oauth_adapter: "OAuthAdapterMixin", user: UserSchema, info: OAuthUserInfo +) -> None: + """Refuse a second identity from the same provider on one account. + + Unlinking resolves by provider alone, so a duplicate row would leave the + provider attached after the user unlinked it, with no way to reach the + leftover. Reachable when two accounts at one provider verify the same email. + """ + for account in await oauth_adapter.get_user_oauth_accounts(user.id): + if account.provider == info.provider: + logger.warning( + "oauth auto-link refused: account already linked to %s (user_id=%s, " + "existing=%s, attempted=%s)", + info.provider, + user.id, + account.provider_user_id, + info.provider_user_id, + ) + raise OAuthProviderAlreadyLinkedError( + f"Your account is already linked to a different {info.provider} account." + ) + + async def link_or_create_user( adapter: AbstractUserAdapter, info: OAuthUserInfo, @@ -214,10 +322,11 @@ async def link_or_create_user( info.provider_user_id, ) raise OAuthProviderError( - "This email is already registered. Sign in with your password, or " - f"verify this email address with {info.provider} and try again." + "This email is already registered. Sign in with your password, " + f"then link your {info.provider} account." ) _ensure_active(existing) + await _ensure_provider_not_linked(oauth_adapter, existing, info) user = existing if user is None: @@ -317,3 +426,131 @@ async def oauth_callback( ) return token_pair, user, is_new_user, info + + +async def link_oauth_account( + adapter: AbstractUserAdapter, + user: UserSchema, + info: OAuthUserInfo, + provider_tokens: dict[str, Any], +) -> OAuthAccount: + """Attach a provider identity to an already-authenticated user. + + Authorization comes from the caller's session, so the provider email is + stored for display only: it is never matched against an account, and the user + row, including ``is_verified``, is left untouched. + + Raises :class:`OAuthAccountAlreadyLinkedError` rather than moving an identity + that belongs to another user, and :class:`OAuthProviderAlreadyLinkedError` + when the user already linked a different account from the same provider, + which unlinking (keyed by provider alone) could not tell apart. + """ + oauth_adapter = cast("OAuthAdapterMixin", adapter) + fields: dict[str, Any] = { + "access_token": provider_tokens.get("access_token"), + "refresh_token": provider_tokens.get("refresh_token"), + "provider_email": info.email, + } + + existing = await oauth_adapter.get_oauth_account(info.provider, info.provider_user_id) + if existing is not None: + if existing.user_id == user.id: + updated = await oauth_adapter.update_oauth_account( + info.provider, info.provider_user_id, fields + ) + logger.info("OAuth account re-linked: user_id=%s, provider=%s", user.id, info.provider) + return updated or existing.model_copy(update=fields) + + owner = await adapter.get_user_by_id(existing.user_id) + if owner is not None: + logger.warning( + "OAuth link refused: identity owned by another user " + "(provider=%s, provider_user_id=%s, requested_by=%s)", + info.provider, + info.provider_user_id, + user.id, + ) + raise OAuthAccountAlreadyLinkedError( + f"This {info.provider} account is already linked to another user." + ) + + # The owner is gone but the row outlived it (storage without cascading + # deletes). Leaving it would block the identity forever. + logger.warning( + "Replacing an orphaned OAuth link: provider=%s, provider_user_id=%s", + info.provider, + info.provider_user_id, + ) + await oauth_adapter.delete_oauth_account(info.provider, info.provider_user_id) + + for account in await oauth_adapter.get_user_oauth_accounts(user.id): + if account.provider == info.provider: + logger.info( + "OAuth link refused: provider already linked (user_id=%s, provider=%s)", + user.id, + info.provider, + ) + raise OAuthProviderAlreadyLinkedError( + f"Your account is already linked to a different {info.provider} account." + ) + + created = await oauth_adapter.create_oauth_account( + OAuthAccount( + provider=info.provider, + provider_user_id=info.provider_user_id, + user_id=user.id, + provider_email=info.email, + access_token=provider_tokens.get("access_token"), + refresh_token=provider_tokens.get("refresh_token"), + ) + ) + if created.user_id != user.id: + # The adapters treat a duplicate insert as success and return the row + # that won, which on a race is another account's. Reporting that as a + # successful link would tell the caller they own an identity they do not. + logger.warning( + "OAuth link lost a race to another account (provider=%s, provider_user_id=%s, " + "requested_by=%s)", + info.provider, + info.provider_user_id, + user.id, + ) + raise OAuthAccountAlreadyLinkedError( + f"This {info.provider} account is already linked to another user." + ) + logger.info("OAuth account linked: user_id=%s, provider=%s", user.id, info.provider) + return created + + +async def oauth_link_callback( + adapter: AbstractUserAdapter, + token_engine: TokenEngine, + provider: OAuthProvider, + code: str, + state: str, + user: UserSchema, + pkce_enabled: bool = True, + *, + binding: str, +) -> OAuthAccount: + """Finish a link flow: verify the state, exchange the code, attach the identity. + + Issues no tokens. The caller is already signed in, and linking a provider is + not a sign-in. + """ + # Both checks run before the state is burned and before the code is spent, so + # a rejected attempt cannot consume the legitimate client's state. + payload = await _decode_bound_state(token_engine, state, binding, purpose=LINK_STATE_PURPOSE) + issued_for = payload.extra.get("link_user_id") + if not isinstance(issued_for, str) or issued_for != str(user.id): + logger.warning( + "OAuth link state rejected: issued for another user (provider=%s, presented_by=%s)", + provider.name, + user.id, + ) + raise OAuthProviderError("Invalid OAuth state token") + + provider_tokens, info = await _burn_state_and_exchange( + provider, token_engine, payload, code, pkce_enabled + ) + return await link_oauth_account(adapter, user, info, provider_tokens) diff --git a/fastapi_fullauth/flows/password_reset.py b/fastapi_fullauth/flows/password_reset.py index 1a67382..e5d14f6 100644 --- a/fastapi_fullauth/flows/password_reset.py +++ b/fastapi_fullauth/flows/password_reset.py @@ -1,3 +1,5 @@ +import hashlib +import hmac import logging from typing import Literal @@ -14,6 +16,17 @@ logger = logging.getLogger("fastapi_fullauth.password_reset") +def _password_fingerprint(hashed_password: str | None) -> str: + """Digest of the password hash the reset token was issued against. + + Carried in the token so that changing the password - through another reset + link, /change-password, or using this one - invalidates every other + outstanding link, without a table of issued tokens to keep. It is a digest of + a digest, so the token reveals nothing about the password. + """ + return hashlib.sha256((hashed_password or "").encode()).hexdigest()[:32] + + async def request_password_reset( adapter: AbstractUserAdapter, token_engine: TokenEngine, @@ -26,9 +39,10 @@ async def request_password_reset( return None logger.info("Password reset requested: user_id=%s", user.id) + fingerprint = _password_fingerprint(await adapter.get_hashed_password(user.id)) token = token_engine.create_access_token( user_id=str(user.id), - extra={"purpose": "password_reset"}, + extra={"purpose": "password_reset", "pwd": fingerprint}, expire_seconds=token_engine.config.PASSWORD_RESET_EXPIRE_MINUTES * 60, ) return token @@ -66,6 +80,16 @@ async def reset_password( logger.warning("Password reset blocked; account deactivated: user_id=%s", user.id) raise TokenError("User account is deactivated") + # A token issued against a password that has since changed is stale: another + # reset link was used, or the user changed it themselves. Reject it rather + # than let an old link out of an inbox undo a deliberate change. + presented = payload.extra.get("pwd") + expected = _password_fingerprint(await adapter.get_hashed_password(user.id)) + if not isinstance(presented, str) or not hmac.compare_digest(presented, expected): + await token_engine.blacklist_payload(payload) + logger.warning("Password reset rejected; stale token: user_id=%s", user.id) + raise TokenError("Invalid password reset token") + hashed = await ahash_password(new_password, algorithm=hash_algorithm) await adapter.set_password(user.id, hashed) diff --git a/fastapi_fullauth/flows/reauth.py b/fastapi_fullauth/flows/reauth.py new file mode 100644 index 0000000..3af8e0a --- /dev/null +++ b/fastapi_fullauth/flows/reauth.py @@ -0,0 +1,57 @@ +"""Proof that the person behind a session is still there. + +A valid access token says a session exists, not that whoever holds it just +proved who they are. Actions that cannot be undone ask for more: either the +current password, or a session whose credentials were checked recently. +""" + +import logging +from datetime import datetime, timezone + +from fastapi_fullauth.core.crypto import averify_password +from fastapi_fullauth.exceptions import AuthenticationError +from fastapi_fullauth.types import TokenPayload + +logger = logging.getLogger("fastapi_fullauth.reauth") + + +async def verify_recent_auth( + payload: TokenPayload, + *, + hashed_password: str | None, + current_password: str | None = None, + max_age_seconds: int = 300, + max_password_length: int = 0, +) -> None: + """Raise :class:`AuthenticationError` unless the caller has proved presence. + + Either proof is enough: + + - ``current_password`` matches the stored hash, or + - the session's credentials were checked within ``max_age_seconds``. + + The age comes from the token's ``auth_time``, which is stamped when + credentials are checked and carried through refresh rotation, so refreshing + a stolen token does not renew it. Tokens issued before ``auth_time`` existed + fall back to ``iat``. + """ + # A blank field means the client did not ask to prove it this way; fall + # through to the session's age rather than failing the request. + if hashed_password is not None and current_password: + # Capped before hashing, for the same reason login caps it. + if max_password_length and len(current_password) > max_password_length: + logger.warning("Re-authentication failed; password too long: sub=%s", payload.sub) + raise AuthenticationError("Current password is incorrect") + if await averify_password(current_password, hashed_password): + return + logger.warning("Re-authentication failed; wrong password: sub=%s", payload.sub) + raise AuthenticationError("Current password is incorrect") + + if max_age_seconds > 0: + checked_at = payload.auth_time or payload.iat + age = (datetime.now(timezone.utc) - checked_at).total_seconds() + if age <= max_age_seconds: + return + + logger.info("Re-authentication required: sub=%s", payload.sub) + raise AuthenticationError("Re-authentication required") diff --git a/fastapi_fullauth/flows/refresh.py b/fastapi_fullauth/flows/refresh.py index 8a7d819..2588128 100644 --- a/fastapi_fullauth/flows/refresh.py +++ b/fastapi_fullauth/flows/refresh.py @@ -83,6 +83,9 @@ async def refresh( roles=roles, user_agent=user_agent, ip_address=ip_address, + # Never `or now`: a token from before this claim existed + # must not be upgraded to a fresh check by rotating it. + auth_time=payload.auth_time or payload.iat, ) if not won or tokens is None: @@ -110,6 +113,7 @@ async def refresh( roles=roles, extra=extra_claims, family_id=payload.family_id, + auth_time=payload.auth_time or payload.iat, ) return TokenPair( access_token=access, diff --git a/fastapi_fullauth/flows/tokens.py b/fastapi_fullauth/flows/tokens.py index bef3c14..6803b6a 100644 --- a/fastapi_fullauth/flows/tokens.py +++ b/fastapi_fullauth/flows/tokens.py @@ -1,4 +1,5 @@ from collections.abc import Awaitable, Callable +from datetime import datetime, timezone from typing import Any from fastapi_fullauth.adapters.base import AbstractUserAdapter @@ -21,23 +22,28 @@ async def issue_token_pair( roles: list[str] | None = None, user_agent: str | None = None, ip_address: str | None = None, + auth_time: datetime | None = None, ) -> TokenPair: """Create an access/refresh token pair for ``user`` and persist the refresh token. Shared by the login, OAuth, passkey, and refresh-rotation flows. Pass ``family_id`` to keep an existing refresh-token family (rotation); omit it - to start a new one. Pass ``roles`` to reuse an already-fetched list and skip + to start a new one. Pass ``auth_time`` when rotating, to carry the original + credential-check time forward. Pass ``roles`` to reuse an already-fetched list and skip the lookup. ``user_agent``/``ip_address`` are recorded on the refresh token so the session list can show the device and origin of each sign-in. """ if roles is None: roles = await adapter.get_user_roles(user.id) + # A new session means credentials were just checked; a rotation passes the + # original time through so refreshing never looks like re-authenticating. access, refresh_meta = token_engine.create_token_pair( user_id=str(user.id), roles=roles, extra=extra_claims, family_id=family_id, + auth_time=auth_time or datetime.now(timezone.utc), ) # Only the sha256 digest is persisted: a leaked database must not hand out diff --git a/fastapi_fullauth/fullauth.py b/fastapi_fullauth/fullauth.py index 14323f5..daa12c4 100644 --- a/fastapi_fullauth/fullauth.py +++ b/fastapi_fullauth/fullauth.py @@ -76,7 +76,8 @@ def __init__( self._warn_memory_backends(config) self.password_validator = password_validator or PasswordValidator( - min_length=config.PASSWORD_MIN_LENGTH + min_length=config.PASSWORD_MIN_LENGTH, + max_length=config.PASSWORD_MAX_LENGTH, ) self.on_create_token_claims = on_create_token_claims self.login_response_schema = login_response_schema or LoginResponse @@ -91,6 +92,7 @@ def __init__( self._profile_router: APIRouter | None = None self._verify_router: APIRouter | None = None self._admin_router: APIRouter | None = None + self._oauth_router: APIRouter | None = None self._passkey_router: APIRouter | None = None self._sessions_router: APIRouter | None = None self._router: APIRouter | None = None @@ -341,12 +343,14 @@ def admin_router(self) -> APIRouter: def oauth_router(self) -> APIRouter | None: if not self.oauth_providers: return None - from fastapi_fullauth.routers.oauth import create_oauth_router + if self._oauth_router is None: + from fastapi_fullauth.routers.oauth import create_oauth_router - return create_oauth_router( - user_schema=self.adapter._user_schema, - login_response_schema=self.login_response_schema, - ) + self._oauth_router = create_oauth_router( + user_schema=self.adapter._user_schema, + login_response_schema=self.login_response_schema, + ) + return self._oauth_router @property def passkey_router(self) -> APIRouter | None: diff --git a/fastapi_fullauth/hooks.py b/fastapi_fullauth/hooks.py index 8b11d3c..99847e8 100644 --- a/fastapi_fullauth/hooks.py +++ b/fastapi_fullauth/hooks.py @@ -30,6 +30,10 @@ class AfterOAuthRegisterHook(Protocol): async def __call__(self, user: UserSchema, user_info: OAuthUserInfo) -> Any: ... +class AfterOAuthLinkHook(Protocol): + async def __call__(self, user: UserSchema, provider: str) -> Any: ... + + EventHook = Callable[..., Awaitable[Any]] F = TypeVar("F", bound=EventHook) @@ -44,6 +48,8 @@ async def __call__(self, user: UserSchema, user_info: OAuthUserInfo) -> Any: ... "after_email_verify": ("user",), "after_oauth_login": ("user", "provider", "is_new_user"), "after_oauth_register": ("user", "user_info"), + "after_oauth_link": ("user", "provider"), + "after_oauth_unlink": ("user", "provider"), "send_verification_email": ("email", "token"), "send_password_reset_email": ("email", "token"), } @@ -73,6 +79,10 @@ def on( self, event: Literal["after_oauth_register"], callback: AfterOAuthRegisterHook ) -> None: ... @overload + def on(self, event: Literal["after_oauth_link"], callback: AfterOAuthLinkHook) -> None: ... + @overload + def on(self, event: Literal["after_oauth_unlink"], callback: AfterOAuthLinkHook) -> None: ... + @overload def on(self, event: Literal["send_verification_email"], callback: EmailHook) -> None: ... @overload def on(self, event: Literal["send_password_reset_email"], callback: EmailHook) -> None: ... diff --git a/fastapi_fullauth/models/sqlalchemy/base.py b/fastapi_fullauth/models/sqlalchemy/base.py index 0e4db6b..caa35fa 100644 --- a/fastapi_fullauth/models/sqlalchemy/base.py +++ b/fastapi_fullauth/models/sqlalchemy/base.py @@ -51,11 +51,15 @@ class RefreshToken(RefreshTokenMixin, Base): id: Mapped[UUID] = mapped_column(Uuid, primary_key=True, default=uuid7) # VARCHAR(512), not Text: MySQL can't build a unique index on a TEXT column. token: Mapped[str] = mapped_column(String(512), unique=True, index=True, nullable=False) + # Indexed: every session listing, revoke-all, and prune filters on it. A + # foreign key does not imply an index on PostgreSQL. user_id: Mapped[UUID] = mapped_column( - Uuid, ForeignKey("fullauth_users.id", ondelete="CASCADE"), nullable=False + Uuid, ForeignKey("fullauth_users.id", ondelete="CASCADE"), nullable=False, index=True ) family_id: Mapped[str] = mapped_column(String(36), index=True, nullable=False) - expires_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False) + expires_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), nullable=False, index=True + ) revoked: Mapped[bool] = mapped_column(Boolean, default=False) user_agent: Mapped[str | None] = mapped_column(String(512), nullable=True) ip_address: Mapped[str | None] = mapped_column(String(45), nullable=True) diff --git a/fastapi_fullauth/models/sqlalchemy/oauth.py b/fastapi_fullauth/models/sqlalchemy/oauth.py index 83e8f77..a694463 100644 --- a/fastapi_fullauth/models/sqlalchemy/oauth.py +++ b/fastapi_fullauth/models/sqlalchemy/oauth.py @@ -22,7 +22,7 @@ class OAuthAccount(OAuthAccountMixin, Base): provider: Mapped[str] = mapped_column(String(50), nullable=False) provider_user_id: Mapped[str] = mapped_column(String(320), nullable=False) user_id: Mapped[UUID] = mapped_column( - Uuid, ForeignKey("fullauth_users.id", ondelete="CASCADE"), nullable=False + Uuid, ForeignKey("fullauth_users.id", ondelete="CASCADE"), nullable=False, index=True ) provider_email: Mapped[str | None] = mapped_column(String(320), nullable=True) access_token: Mapped[str | None] = mapped_column(Text, nullable=True) diff --git a/fastapi_fullauth/models/sqlmodel/base.py b/fastapi_fullauth/models/sqlmodel/base.py index 850fc75..d6bc6d9 100644 --- a/fastapi_fullauth/models/sqlmodel/base.py +++ b/fastapi_fullauth/models/sqlmodel/base.py @@ -47,9 +47,13 @@ class RefreshToken(RefreshTokenMixin, table=True): id: UUID = Field(default_factory=uuid7, primary_key=True) # VARCHAR(512): the AutoString 255 default truncates a refresh JWT on MySQL. token: str = Field(unique=True, index=True, max_length=512) - user_id: UUID = Field(foreign_key="fullauth_users.id", ondelete="CASCADE") + # Indexed: every session listing, revoke-all, and prune filters on it. A + # foreign key does not imply an index on PostgreSQL. + user_id: UUID = Field(foreign_key="fullauth_users.id", ondelete="CASCADE", index=True) family_id: str = Field(index=True, max_length=36) - expires_at: datetime = Field(sa_column=Column(DateTime(timezone=True), nullable=False)) + expires_at: datetime = Field( + sa_column=Column(DateTime(timezone=True), nullable=False, index=True) + ) revoked: bool = Field(default=False) user_agent: str | None = Field(default=None, max_length=512) ip_address: str | None = Field(default=None, max_length=45) diff --git a/fastapi_fullauth/models/sqlmodel/oauth.py b/fastapi_fullauth/models/sqlmodel/oauth.py index 4e63c9f..b222436 100644 --- a/fastapi_fullauth/models/sqlmodel/oauth.py +++ b/fastapi_fullauth/models/sqlmodel/oauth.py @@ -21,7 +21,7 @@ class OAuthAccount(OAuthAccountMixin, table=True): id: UUID = Field(default_factory=uuid7, primary_key=True) provider: str = Field(max_length=50) provider_user_id: str = Field(max_length=320) - user_id: UUID = Field(foreign_key="fullauth_users.id", ondelete="CASCADE") + user_id: UUID = Field(foreign_key="fullauth_users.id", ondelete="CASCADE", index=True) provider_email: str | None = Field(default=None, max_length=320) # Text, not the default VARCHAR(255): provider access/refresh tokens # (opaque or JWT-shaped) routinely exceed 255 chars and would silently diff --git a/fastapi_fullauth/protection/challenges.py b/fastapi_fullauth/protection/challenges.py index e896527..ff2fec3 100644 --- a/fastapi_fullauth/protection/challenges.py +++ b/fastapi_fullauth/protection/challenges.py @@ -15,6 +15,8 @@ if TYPE_CHECKING: from fastapi_fullauth.config import FullAuthConfig +from fastapi_fullauth.core.blacklist import SWEEP_INTERVAL_SECONDS + logger = logging.getLogger("fastapi_fullauth.challenges") @@ -39,9 +41,22 @@ class InMemoryChallengeStore(ChallengeStore): def __init__(self) -> None: self._store: dict[str, tuple[str, float]] = {} self._lock = asyncio.Lock() + self._next_sweep = 0.0 async def store(self, key: str, challenge: str, ttl: int = 60) -> None: - self._store[key] = (challenge, time.monotonic() + ttl) + now = time.monotonic() + self._sweep_expired(now) + self._store[key] = (challenge, now + ttl) + + def _sweep_expired(self, now: float) -> None: + # pop() is the only other way an entry leaves, so a flow that is started + # and never completed would live for the whole process. Beginning an + # authentication needs no account, so those are attacker-reachable. + if now < self._next_sweep: + return + self._next_sweep = now + SWEEP_INTERVAL_SECONDS + for key in [k for k, (_, expires_at) in self._store.items() if expires_at <= now]: + del self._store[key] async def pop(self, key: str) -> str | None: # Lock the pop+expiry-check as one atomic section so two concurrent pops @@ -60,10 +75,10 @@ class RedisChallengeStore(ChallengeStore): """Redis-backed challenge store. Works across multiple workers.""" def __init__(self, redis_url: str) -> None: - from fastapi_fullauth.core._redis import acquire_redis + from fastapi_fullauth.core._redis import RedisClientKey, acquire_redis - self._redis = acquire_redis(redis_url, feature="the Redis challenge store") - self._redis_url: str | None = redis_url + self._redis_key: RedisClientKey | None + self._redis, self._redis_key = acquire_redis(redis_url, feature="the Redis challenge store") self._prefix = "fullauth:challenge:" async def store(self, key: str, challenge: str, ttl: int = 60) -> None: @@ -77,9 +92,11 @@ async def pop(self, key: str) -> str | None: async def aclose(self) -> None: from fastapi_fullauth.core._redis import release_redis - if self._redis_url is not None: - await release_redis(self._redis_url) - self._redis_url = None + # Released with the key acquire returned, never a re-derived one: + # the closing loop may not be the acquiring one. + if self._redis_key is not None: + await release_redis(self._redis_key) + self._redis_key = None _challenge_store_registry: dict[str, type[ChallengeStore]] = { diff --git a/fastapi_fullauth/protection/lockout.py b/fastapi_fullauth/protection/lockout.py index 17f3a50..b048847 100644 --- a/fastapi_fullauth/protection/lockout.py +++ b/fastapi_fullauth/protection/lockout.py @@ -95,33 +95,49 @@ def __init__( lockout_seconds: int = 900, ) -> None: super().__init__(max_attempts, lockout_seconds) - from fastapi_fullauth.core._redis import acquire_redis + from fastapi_fullauth.core._redis import RedisClientKey, acquire_redis - self._redis = acquire_redis(redis_url, feature="the Redis lockout manager") - self._redis_url: str | None = redis_url + self._redis_key: RedisClientKey | None + self._redis, self._redis_key = acquire_redis(redis_url, feature="the Redis lockout manager") self._prefix = "fullauth:lockout:" async def is_locked(self, key: str) -> bool: - locked = await self._redis.get(f"{self._prefix}locked:{key}") + try: + locked = await self._redis.get(f"{self._prefix}locked:{key}") + except Exception: + # Fail open, like the rate limiter: a Redis outage must not turn every + # login into a 500, and failing closed would lock every account out. + # Brute-force protection is lost for the outage; log it loudly. + # (The token blacklist fails closed instead - see RedisTokenBlacklist.) + logger.error("Lockout Redis error; treating account as unlocked", exc_info=True) + return False return locked is not None async def record_failure(self, key: str) -> None: attempts_key = f"{self._prefix}attempts:{key}" locked_key = f"{self._prefix}locked:{key}" - pipe = self._redis.pipeline() - pipe.incr(attempts_key) - pipe.expire(attempts_key, self.lockout_seconds) - results = await pipe.execute() + try: + pipe = self._redis.pipeline() + pipe.incr(attempts_key) + pipe.expire(attempts_key, self.lockout_seconds) + results = await pipe.execute() + except Exception: + logger.error("Lockout Redis error; failure not counted", exc_info=True) + return count = results[0] if count >= self.max_attempts: # Set the lock and clear the counter in one round-trip so a concurrent # burst can't observe a half-applied state. - lock_pipe = self._redis.pipeline() - lock_pipe.setex(locked_key, self.lockout_seconds, "1") - lock_pipe.delete(attempts_key) - await lock_pipe.execute() + try: + lock_pipe = self._redis.pipeline() + lock_pipe.setex(locked_key, self.lockout_seconds, "1") + lock_pipe.delete(attempts_key) + await lock_pipe.execute() + except Exception: + logger.error("Lockout Redis error; account not locked", exc_info=True) + return logger.warning( "Account locked after %d failed attempts: %s", self.max_attempts, @@ -129,17 +145,24 @@ async def record_failure(self, key: str) -> None: ) async def clear(self, key: str) -> None: - pipe = self._redis.pipeline() - pipe.delete(f"{self._prefix}attempts:{key}") - pipe.delete(f"{self._prefix}locked:{key}") - await pipe.execute() + try: + pipe = self._redis.pipeline() + pipe.delete(f"{self._prefix}attempts:{key}") + pipe.delete(f"{self._prefix}locked:{key}") + await pipe.execute() + except Exception: + # Best effort: a successful login that can't clear the counter is not + # worth failing, the entries expire on their own. + logger.error("Lockout Redis error; counters not cleared", exc_info=True) async def aclose(self) -> None: from fastapi_fullauth.core._redis import release_redis - if self._redis_url is not None: - await release_redis(self._redis_url) - self._redis_url = None + # Released with the key acquire returned, never a re-derived one: + # the closing loop may not be the acquiring one. + if self._redis_key is not None: + await release_redis(self._redis_key) + self._redis_key = None _lockout_registry: dict[str, type[LockoutManager]] = { diff --git a/fastapi_fullauth/protection/ratelimit.py b/fastapi_fullauth/protection/ratelimit.py index 524ab25..41b5a39 100644 --- a/fastapi_fullauth/protection/ratelimit.py +++ b/fastapi_fullauth/protection/ratelimit.py @@ -82,12 +82,12 @@ def __init__( max_requests: int = 60, window_seconds: int = 60, ) -> None: - from fastapi_fullauth.core._redis import acquire_redis + from fastapi_fullauth.core._redis import RedisClientKey, acquire_redis self.max_requests = max_requests self.window_seconds = window_seconds - self._redis = acquire_redis(redis_url, feature="the Redis rate limiter") - self._redis_url: str | None = redis_url + self._redis_key: RedisClientKey | None + self._redis, self._redis_key = acquire_redis(redis_url, feature="the Redis rate limiter") self._prefix = "fullauth:ratelimit:" async def is_allowed(self, key: str) -> bool: @@ -167,9 +167,11 @@ async def reset(self, key: str) -> None: async def aclose(self) -> None: from fastapi_fullauth.core._redis import release_redis - if self._redis_url is not None: - await release_redis(self._redis_url) - self._redis_url = None + # Released with the key acquire returned, never a re-derived one: + # the closing loop may not be the acquiring one. + if self._redis_key is not None: + await release_redis(self._redis_key) + self._redis_key = None _rate_limiter_registry: dict[str, type[RateLimiter] | type[RedisRateLimiter]] = { @@ -243,6 +245,7 @@ def __init__(self, config: "FullAuthConfig") -> None: config, limits.passkey_auth, window ) self._limiters["refresh"] = create_rate_limiter(config, limits.refresh, window) + self._limiters["reauth"] = create_rate_limiter(config, limits.reauth, window) async def aclose(self) -> None: for limiter in self._limiters.values(): diff --git a/fastapi_fullauth/routers/__init__.py b/fastapi_fullauth/routers/__init__.py index a909dcb..70da3f5 100644 --- a/fastapi_fullauth/routers/__init__.py +++ b/fastapi_fullauth/routers/__init__.py @@ -1,11 +1,17 @@ from fastapi_fullauth.routers.admin import create_admin_router from fastapi_fullauth.routers.auth import create_auth_router +from fastapi_fullauth.routers.oauth import create_oauth_router +from fastapi_fullauth.routers.passkey import create_passkey_router from fastapi_fullauth.routers.profile import create_profile_router +from fastapi_fullauth.routers.sessions import create_sessions_router from fastapi_fullauth.routers.verify import create_verify_router __all__ = [ "create_admin_router", "create_auth_router", + "create_oauth_router", + "create_passkey_router", "create_profile_router", + "create_sessions_router", "create_verify_router", ] diff --git a/fastapi_fullauth/routers/_schemas.py b/fastapi_fullauth/routers/_schemas.py index a26afa7..93b1587 100644 --- a/fastapi_fullauth/routers/_schemas.py +++ b/fastapi_fullauth/routers/_schemas.py @@ -90,6 +90,11 @@ class ChangePasswordRequest(BaseModel): current_password: str | None = None +class DeleteAccountRequest(BaseModel): + # Optional: a session whose credentials were checked recently is proof too. + current_password: str | None = None + + def build_role_assignment_model(user_id_type: Any = UUID) -> type[BaseModel]: """Build the admin role-assignment body with ``user_id`` typed to the user key. diff --git a/fastapi_fullauth/routers/admin.py b/fastapi_fullauth/routers/admin.py index 325086c..e3c1165 100644 --- a/fastapi_fullauth/routers/admin.py +++ b/fastapi_fullauth/routers/admin.py @@ -19,6 +19,14 @@ from fastapi_fullauth.fullauth import FullAuth +def _require_permissions(fullauth: "FullAuth") -> None: + """The admin router mounts on role support alone, so the permission routes + can exist against an adapter with no permission models. Say so, rather than + letting the adapter's RuntimeError surface as a 500.""" + if not fullauth.adapter.supports_feature("permission"): + raise HTTPException(status_code=501, detail="Adapter does not support permissions") + + def create_admin_router(user_id_type: Any = UUID) -> APIRouter: router = APIRouter() RoleAssignment = build_role_assignment_model(user_id_type) # noqa: N806 @@ -73,6 +81,7 @@ async def assign_permission_route( caller: SuperUser, fullauth: "FullAuth" = Depends(get_fullauth), ) -> MessageResponse: + _require_permissions(fullauth) await cast("PermissionAdapterMixin", fullauth.adapter).assign_permission_to_role( data.role, data.permission ) @@ -97,6 +106,7 @@ async def remove_permission_route( caller: SuperUser, fullauth: "FullAuth" = Depends(get_fullauth), ) -> MessageResponse: + _require_permissions(fullauth) await cast("PermissionAdapterMixin", fullauth.adapter).remove_permission_from_role( data.role, data.permission ) @@ -120,6 +130,7 @@ async def list_role_permissions_route( caller: SuperUser, fullauth: "FullAuth" = Depends(get_fullauth), ) -> list[str]: + _require_permissions(fullauth) return await cast("PermissionAdapterMixin", fullauth.adapter).get_role_permissions( role_name ) diff --git a/fastapi_fullauth/routers/auth.py b/fastapi_fullauth/routers/auth.py index 0112b09..3dbdb80 100644 --- a/fastapi_fullauth/routers/auth.py +++ b/fastapi_fullauth/routers/auth.py @@ -153,6 +153,7 @@ async def login_route( user=user, hash_algorithm=fullauth.config.PASSWORD_HASH_ALGORITHM, prevent_timing_attacks=fullauth.config.PREVENT_LOGIN_TIMING_ATTACKS, + max_password_length=fullauth.config.PASSWORD_MAX_LENGTH, user_agent=user_agent, ip_address=ip_address, ) diff --git a/fastapi_fullauth/routers/oauth.py b/fastapi_fullauth/routers/oauth.py index e5d8884..62762e2 100644 --- a/fastapi_fullauth/routers/oauth.py +++ b/fastapi_fullauth/routers/oauth.py @@ -8,14 +8,20 @@ from fastapi_fullauth.dependencies.current_user import CurrentUser, get_fullauth from fastapi_fullauth.exceptions import ( OAUTH_ERROR_EXCEPTION, - OAuthProviderError, + OAuthAccountAlreadyLinkedError, + OAuthError, + OAuthProviderAlreadyLinkedError, TokenError, ) +from fastapi_fullauth.flows.credentials import last_method_detail, remaining_login_methods from fastapi_fullauth.flows.oauth import ( build_authorization_url, + build_link_authorization_url, generate_oauth_binding, oauth_callback, + oauth_link_callback, ) +from fastapi_fullauth.oauth.base import OAuthProvider from fastapi_fullauth.routers._schemas import LoginResponse, build_login_response_model from fastapi_fullauth.routers._transport import write_tokens from fastapi_fullauth.types import TokenPair, UserSchema, UserSchemaType @@ -57,6 +63,21 @@ class OAuthAccountResponse(BaseModel): provider_email: str | None = None +def _require_oauth_adapter(fullauth: "FullAuth") -> None: + """The combined router only mounts these when the adapter supports OAuth, but + the routers are composable: mounted by hand, the adapter's RuntimeError would + surface as a 500.""" + if not fullauth.adapter.supports_feature("oauth"): + raise HTTPException(status_code=501, detail="Adapter does not support OAuth") + + +def _require_provider(fullauth: "FullAuth", provider: str) -> OAuthProvider: + oauth_provider = fullauth.oauth_providers.get(provider) + if oauth_provider is None: + raise HTTPException(status_code=404, detail=f"OAuth provider '{provider}' not configured") + return oauth_provider + + def create_oauth_router( user_schema: type[UserSchemaType] = UserSchema, # type: ignore[assignment] login_response_schema: type[LoginResponse] = LoginResponse, @@ -86,12 +107,7 @@ async def authorize( redirect_uri: str, fullauth: "FullAuth" = Depends(get_fullauth), ) -> OAuthAuthorizeResponse: - oauth_provider = fullauth.oauth_providers.get(provider) - if oauth_provider is None: - raise HTTPException( - status_code=404, detail=f"OAuth provider '{provider}' not configured" - ) - + oauth_provider = _require_provider(fullauth, provider) if redirect_uri not in oauth_provider.redirect_uris: raise HTTPException(status_code=400, detail="Invalid redirect URI") @@ -121,12 +137,7 @@ async def callback( ) -> TokenPair: await fullauth.enforce_rate_limit(request, "login") - oauth_provider = fullauth.oauth_providers.get(provider) - if oauth_provider is None: - raise HTTPException( - status_code=404, detail=f"OAuth provider '{provider}' not configured" - ) - + oauth_provider = _require_provider(fullauth, provider) user_agent, ip_address = request_session_metadata( request, fullauth.config.TRUSTED_PROXY_HEADERS, fullauth.config.TRUSTED_PROXY_COUNT ) @@ -144,7 +155,7 @@ async def callback( ip_address=ip_address, binding=data.binding, ) - except (OAuthProviderError, TokenError): + except (OAuthError, TokenError): raise OAUTH_ERROR_EXCEPTION token_pair = await write_tokens(response, fullauth, token_pair) @@ -167,6 +178,76 @@ async def callback( return token_pair + @router.get( + "/oauth/{provider}/link/authorize", + status_code=200, + response_model=OAuthAuthorizeResponse, + description="Get the authorization URL for linking a provider to the signed-in account.", + ) + async def link_authorize( + provider: str, + redirect_uri: str, + user: CurrentUser, + fullauth: "FullAuth" = Depends(get_fullauth), + ) -> OAuthAuthorizeResponse: + _require_oauth_adapter(fullauth) + oauth_provider = _require_provider(fullauth, provider) + if redirect_uri not in oauth_provider.redirect_uris: + raise HTTPException(status_code=400, detail="Invalid redirect URI") + + binding = generate_oauth_binding() + url = build_link_authorization_url( + fullauth.token_engine, + oauth_provider, + redirect_uri, + user.id, + ttl_seconds=fullauth.config.OAUTH_STATE_EXPIRE_SECONDS, + pkce_enabled=fullauth.config.OAUTH_PKCE_ENABLED, + binding=binding, + ) + return OAuthAuthorizeResponse(authorization_url=url, binding=binding) + + @router.post( + "/oauth/{provider}/link/callback", + status_code=200, + response_model=OAuthAccountResponse, + description="Link the provider account from this flow to the signed-in account.", + ) + async def link_callback( + provider: str, + data: OAuthCallbackRequest, + user: CurrentUser, + fullauth: "FullAuth" = Depends(get_fullauth), + ) -> OAuthAccountResponse: + _require_oauth_adapter(fullauth) + oauth_provider = _require_provider(fullauth, provider) + + # Unlike sign-in, this route answers with the specific reason: the caller + # is authenticated, so there is nothing to enumerate, and a settings + # screen has to tell the two conflicts apart. + try: + account = await oauth_link_callback( + adapter=fullauth.adapter, + token_engine=fullauth.token_engine, + provider=oauth_provider, + code=data.code, + state=data.state, + user=user, + pkce_enabled=fullauth.config.OAUTH_PKCE_ENABLED, + binding=data.binding, + ) + except (OAuthAccountAlreadyLinkedError, OAuthProviderAlreadyLinkedError) as e: + raise HTTPException(status_code=409, detail=str(e)) from e + except (OAuthError, TokenError): + raise OAUTH_ERROR_EXCEPTION + + await fullauth.hooks.emit("after_oauth_link", user=user, provider=provider) + return OAuthAccountResponse( + provider=account.provider, + provider_user_id=account.provider_user_id, + provider_email=account.provider_email, + ) + @router.get( "/oauth/accounts", status_code=200, @@ -177,6 +258,7 @@ async def list_oauth_accounts( user: CurrentUser, fullauth: "FullAuth" = Depends(get_fullauth), ) -> list[OAuthAccountResponse]: + _require_oauth_adapter(fullauth) adapter = cast("OAuthAdapterMixin", fullauth.adapter) accounts = await adapter.get_user_oauth_accounts(user.id) return [ @@ -198,22 +280,33 @@ async def unlink_oauth_account( user: CurrentUser, fullauth: "FullAuth" = Depends(get_fullauth), ) -> None: + _require_oauth_adapter(fullauth) oauth_adapter = cast("OAuthAdapterMixin", fullauth.adapter) accounts = await oauth_adapter.get_user_oauth_accounts(user.id) - has_password = await fullauth.adapter.get_hashed_password(user.id) is not None - other_oauth = [a for a in accounts if a.provider != provider] - - if not has_password and not other_oauth: - raise HTTPException( - status_code=400, - detail="Cannot unlink the only login method. Set a password first.", - ) target = next((a for a in accounts if a.provider == provider), None) if target is None: raise HTTPException(status_code=404, detail="OAuth account not found") + # Passkeys count as a sign-in method here, so a passwordless user with a + # passkey is not told to set a password they do not need. + passkeys_usable = fullauth.config.PASSKEY_ENABLED and fullauth.adapter.supports_feature( + "passkey" + ) + if not await remaining_login_methods( + fullauth.adapter, + user.id, + usable_providers=set(fullauth.oauth_providers), + passkeys_usable=passkeys_usable, + without_provider=provider, + ): + raise HTTPException( + status_code=400, + detail=last_method_detail(passkeys_available=passkeys_usable), + ) + await oauth_adapter.delete_oauth_account(provider, target.provider_user_id) logger.info("OAuth account unlinked: user_id=%s, provider=%s", user.id, provider) + await fullauth.hooks.emit("after_oauth_unlink", user=user, provider=provider) return router diff --git a/fastapi_fullauth/routers/passkey.py b/fastapi_fullauth/routers/passkey.py index fbfc7df..f92721f 100644 --- a/fastapi_fullauth/routers/passkey.py +++ b/fastapi_fullauth/routers/passkey.py @@ -7,6 +7,7 @@ from fastapi_fullauth.adapters.base import PasskeyAdapterMixin from fastapi_fullauth.dependencies.current_user import CurrentUser, get_fullauth +from fastapi_fullauth.flows.credentials import last_method_detail, remaining_login_methods from fastapi_fullauth.protection.challenges import ChallengeStore from fastapi_fullauth.routers._schemas import LoginResponse, build_login_response_model from fastapi_fullauth.routers._transport import write_tokens @@ -279,6 +280,23 @@ async def delete_passkey( if not any(pk.id == passkey_id for pk in passkeys): raise HTTPException(status_code=404, detail="Passkey not found") + # Same rule the OAuth unlink route applies: never leave an account with + # no way to sign in. A linked provider only counts while it is still one + # of the configured ones. + if not await remaining_login_methods( + fullauth.adapter, + user.id, + usable_providers=set(fullauth.oauth_providers), + passkeys_usable=True, + without_passkey_id=passkey_id, + ): + # This route only exists where passkeys are enabled, so registering a + # second one is a real alternative to setting a password. + raise HTTPException( + status_code=400, + detail=last_method_detail(passkeys_available=True), + ) + await fullauth.adapter.delete_passkey(passkey_id) logger.info("Passkey deleted: user_id=%s, passkey_id=%s", user.id, passkey_id) diff --git a/fastapi_fullauth/routers/profile.py b/fastapi_fullauth/routers/profile.py index 95d6921..ce438ab 100644 --- a/fastapi_fullauth/routers/profile.py +++ b/fastapi_fullauth/routers/profile.py @@ -1,11 +1,16 @@ import logging from typing import TYPE_CHECKING, cast -from fastapi import APIRouter, Body, Depends, HTTPException +from fastapi import APIRouter, Body, Depends, HTTPException, Request from fastapi.exceptions import RequestValidationError from pydantic import BaseModel, ValidationError -from fastapi_fullauth.dependencies.current_user import CurrentUser, VerifiedUser, get_fullauth +from fastapi_fullauth.dependencies.current_user import ( + CurrentUser, + VerifiedUser, + current_token_payload, + get_fullauth, +) from fastapi_fullauth.exceptions import ( AuthenticationError, InvalidPasswordError, @@ -14,12 +19,14 @@ ) from fastapi_fullauth.flows.change_password import change_password from fastapi_fullauth.flows.profile import validate_profile_updates +from fastapi_fullauth.flows.reauth import verify_recent_auth from fastapi_fullauth.routers._schemas import ( ChangePasswordRequest, + DeleteAccountRequest, MessageResponse, build_profile_update_model, ) -from fastapi_fullauth.types import UserSchema, UserSchemaType +from fastapi_fullauth.types import TokenPayload, UserSchema, UserSchemaType logger = logging.getLogger("fastapi_fullauth.routers") @@ -80,11 +87,39 @@ async def update_me_route( return await fullauth.adapter.update_user(user.id, updates) - @router.delete("/me", status_code=204, description="Delete your own account.") + @router.delete( + "/me", + status_code=204, + description=( + "Delete your own account. Needs proof of presence: either " + "`current_password` in the body, or a session whose credentials were " + "checked within `REAUTH_MAX_AGE_SECONDS`. Answers 403 otherwise." + ), + ) async def delete_me_route( + request: Request, user: CurrentUser, + payload: TokenPayload = Depends(current_token_payload), fullauth: "FullAuth" = Depends(get_fullauth), + data: DeleteAccountRequest | None = Body(None), ) -> None: + # The password check below is an oracle for whoever holds the token, and + # a correct guess deletes the account. Meter it like a sign-in. + await fullauth.enforce_rate_limit(request, "reauth") + + # Deleting an account cannot be undone, so a token someone found is not + # enough on its own. + try: + await verify_recent_auth( + payload, + hashed_password=await fullauth.adapter.get_hashed_password(user.id), + current_password=data.current_password if data else None, + max_age_seconds=fullauth.config.REAUTH_MAX_AGE_SECONDS, + max_password_length=fullauth.config.PASSWORD_MAX_LENGTH, + ) + except AuthenticationError as e: + raise HTTPException(status_code=403, detail=str(e)) from e + await fullauth.adapter.revoke_all_user_refresh_tokens(user.id) await fullauth.adapter.delete_user(user.id) logger.warning("Account deleted: user_id=%s, email=%s", user.id, user.email) @@ -95,14 +130,34 @@ async def delete_me_route( response_model=message_response_schema, description=( "Change password. `current_password` is required when the user already " - "has one; for OAuth-only users without a stored password it may be omitted." + "has one. An account without a stored password is setting its first, " + "which needs a session whose credentials were checked within " + "`REAUTH_MAX_AGE_SECONDS` instead." ), ) async def change_password_route( data: ChangePasswordRequest, + request: Request, user: CurrentUser, + payload: TokenPayload = Depends(current_token_payload), fullauth: "FullAuth" = Depends(get_fullauth), ) -> MessageResponse: + # `current_password` is the same oracle as above. + await fullauth.enforce_rate_limit(request, "reauth") + + # Setting the first password on an OAuth-only or passkey-only account has + # no current password to check, and it creates a new way in, so presence + # has to be proved some other way. + if await fullauth.adapter.get_hashed_password(user.id) is None: + try: + await verify_recent_auth( + payload, + hashed_password=None, + max_age_seconds=fullauth.config.REAUTH_MAX_AGE_SECONDS, + ) + except AuthenticationError as e: + raise HTTPException(status_code=403, detail=str(e)) from e + try: await change_password( adapter=fullauth.adapter, diff --git a/fastapi_fullauth/types.py b/fastapi_fullauth/types.py index 8c363cc..6d5c24c 100644 --- a/fastapi_fullauth/types.py +++ b/fastapi_fullauth/types.py @@ -106,6 +106,10 @@ class TokenPayload(BaseModel): roles: list[str] = Field(default_factory=list) extra: dict[str, Any] = Field(default_factory=dict) family_id: str | None = None + # When the credentials behind this session were last checked. Set at login, + # OAuth callback, and passkey sign-in, and carried unchanged through refresh + # rotation, so rotating a token is not mistaken for proving presence. + auth_time: datetime | None = None class PasskeyCredential(BaseModel): diff --git a/fastapi_fullauth/validators.py b/fastapi_fullauth/validators.py index 3a32b8e..cd20c4c 100644 --- a/fastapi_fullauth/validators.py +++ b/fastapi_fullauth/validators.py @@ -12,6 +12,7 @@ def __init__( require_digit: bool = False, require_special: bool = False, blocked_passwords: list[str] | None = None, + max_length: int = 4096, ) -> None: self.min_length = min_length self.require_uppercase = require_uppercase @@ -19,6 +20,7 @@ def __init__( self.require_digit = require_digit self.require_special = require_special self.blocked_passwords = set(p.lower() for p in (blocked_passwords or [])) + self.max_length = max_length def validate(self, password: str) -> None: errors: list[str] = [] @@ -26,6 +28,11 @@ def validate(self, password: str) -> None: if len(password) < self.min_length: errors.append(f"Password must be at least {self.min_length} characters") + # 0 disables the cap. The flows validate before hashing, so an oversized + # password never reaches argon2. + if self.max_length and len(password) > self.max_length: + errors.append(f"Password must be at most {self.max_length} characters") + if self.require_uppercase and not re.search(r"[A-Z]", password): errors.append("Password must contain at least one uppercase letter") diff --git a/skills/fastapi-fullauth/references/api-reference.md b/skills/fastapi-fullauth/references/api-reference.md index b946b82..64e1b3c 100644 --- a/skills/fastapi-fullauth/references/api-reference.md +++ b/skills/fastapi-fullauth/references/api-reference.md @@ -316,9 +316,11 @@ Grouped for readability. All read from env with `FULLAUTH_` prefix. ### Passwords - `PASSWORD_HASH_ALGORITHM: "argon2id" | "bcrypt" = "argon2id"` - `PASSWORD_MIN_LENGTH: int = 8` +- `PASSWORD_MAX_LENGTH: int = 4096` (caps hashing work from an oversized body) ### Login - `LOGIN_FIELD: str = "email"` +- `REAUTH_MAX_AGE_SECONDS: int = 300` (how recently credentials must have been checked for `DELETE /me` and setting a first password; the current password in the body proves it too) - `PREVENT_LOGIN_TIMING_ATTACKS: bool = True` (dummy verify on unknown-user path, matches configured algorithm) ### Lockout @@ -396,8 +398,8 @@ Default prefix `/api/v1/auth`: | GET | `/me` | profile | yes | | GET | `/me/verified` | profile | yes (verified)| | PATCH | `/me` | profile | yes | -| DELETE | `/me` | profile | yes | -| POST | `/change-password` | profile | yes | +| DELETE | `/me` | profile | yes + recent | +| POST | `/change-password` | profile | yes (+ recent, for a first password) | | POST | `/verify-email/request` | verify | yes | | POST | `/verify-email/confirm` | verify | no | | POST | `/password-reset/request` | verify | no | @@ -410,6 +412,8 @@ Default prefix `/api/v1/auth`: | GET | `/oauth/providers` | oauth | no | | GET | `/oauth/{provider}/authorize` | oauth | no | | POST | `/oauth/{provider}/callback` | oauth | no | +| GET | `/oauth/{provider}/link/authorize`| oauth | yes | +| POST | `/oauth/{provider}/link/callback` | oauth | yes | | GET | `/oauth/accounts` | oauth | yes | | DELETE | `/oauth/accounts/{provider}` | oauth | yes | | POST | `/passkeys/register/begin` | passkey | yes | diff --git a/skills/fastapi-fullauth/references/oauth.md b/skills/fastapi-fullauth/references/oauth.md index 10230f6..86866e5 100644 --- a/skills/fastapi-fullauth/references/oauth.md +++ b/skills/fastapi-fullauth/references/oauth.md @@ -56,6 +56,8 @@ The `redirect_uris` list is a whitelist. `/oauth/{provider}/authorize?redirect_u - `POST /api/v1/auth/oauth/{provider}/callback`: body `{code, state, binding}`, exchange code for tokens and log the user in - `GET /api/v1/auth/oauth/accounts`: list OAuth accounts linked to current user (auth required) - `DELETE /api/v1/auth/oauth/accounts/{provider}`: unlink a provider (auth required, only works if the user has another login method) +- `GET /api/v1/auth/oauth/{provider}/link/authorize`: authorization URL for linking to the signed-in account (auth required); same `binding` contract as sign-in +- `POST /api/v1/auth/oauth/{provider}/link/callback`: link that provider account to the signed-in account (auth required); body `{code, state, binding}`, returns the account, issues no tokens The SPA flow: @@ -83,7 +85,7 @@ PKCE (S256) is enabled by default for providers that support it (Google, GitHub, When the gate fires, the flow raises `OAuthProviderError`. The router collapses it, like every other OAuth failure, into a generic `400 {"detail": "OAuth authentication failed"}`, so the endpoint cannot be used to probe which emails are registered; the specific reason is only in the server log (`fastapi_fullauth.oauth`, "oauth auto-link refused"). -There is no authenticated "link this provider to my account" endpoint. Linking happens only as a side effect of signing in, on a provider-verified email, so a user whose provider email is unverified or differs from their account email cannot link that provider at all. Tell them to sign in with their password instead. `GET /oauth/accounts` and `DELETE /oauth/accounts/{provider}` list and remove links that already exist. +The user signs in with their password instead, then links the provider explicitly: `GET /oauth/{provider}/link/authorize` followed by `POST /oauth/{provider}/link/callback`, both authenticated (see below). That path ignores the provider email entirely, so it also covers a provider account under a different address. To disable auto-link entirely: `FULLAUTH_OAUTH_AUTO_LINK_BY_EMAIL=False`. Then every OAuth sign-in either finds an existing linked identity or creates a brand-new user, never cross-links. @@ -117,6 +119,8 @@ OAuth users have `hashed_password=NULL`. They can't log in with a password becau `POST /api/v1/auth/change-password` (authenticated, body: `{new_password}`, `current_password` may be omitted) sets the first password. The route accepts the missing `current_password` only when the stored hash is `NULL`; once a password exists, `current_password` is required like any other change. +Setting that first password also needs recent authentication, because there is no current password to check and it creates a new way into the account: the access token's `auth_time` must be within `FULLAUTH_REAUTH_MAX_AGE_SECONDS` (300 by default), otherwise the route answers `403 Re-authentication required`. Refreshing does not reset `auth_time`, so the user has to sign in again. Tell users to set their password soon after signing in, or raise the window. + There's no separate `set-password` route; `/change-password` handles both first-time set and subsequent changes. ## Writing a custom provider @@ -161,5 +165,5 @@ Instantiate with `client_id`, `client_secret`, `redirect_uris` and pass to `Full - **GitHub's `email_verified`**: fetch the authenticated user's primary email from `/user/emails` and trust only the one with `primary=true, verified=true`. The provider built-in does this; a custom provider needs to do the same. - **`redirect_uri` must match exactly** between authorize and callback; the provider enforces it, and the library passes it through to `exchange_code`. Query strings count. - **State token and access token use the same `SECRET_KEY`.** Don't add custom `aud` logic; the purpose claim plus short TTL is what keeps them distinct. -- **Unlinking the only login**: `DELETE /oauth/accounts/{provider}` refuses to unlink a provider if it's the user's only remaining login method (no password, no other provider). It returns 400 with "Set a password first." +- **Unlinking the only login**: `DELETE /oauth/accounts/{provider}` refuses with 400 when unlinking would leave the account with no way to sign in. A stored password, any other linked provider, and any registered passkey all count, so a passwordless user who has a passkey can unlink freely. `DELETE /passkeys/{id}` applies the same rule. Unlinking a provider the user does not have is 404. - **Composite unique** on `(provider, provider_user_id)` is enforced at the DB level since v0.8.0. If you upgrade from ≤ 0.7.0, autogenerate the Alembic migration before deploying. diff --git a/tests/adapter_conformance.py b/tests/adapter_conformance.py index 13497ec..c28eb9f 100644 --- a/tests/adapter_conformance.py +++ b/tests/adapter_conformance.py @@ -11,7 +11,7 @@ ``atomic_transactions = False`` in their subclass. """ -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone from uuid import uuid4 import pytest @@ -249,6 +249,36 @@ async def test_refresh_token_crud(self, adapter, make_user): # revoking an unknown token also returns False assert await adapter.revoke_refresh_token("does-not-exist") is False + async def test_prune_expired_refresh_tokens(self, adapter, make_user): + """Only expired rows go. A revoked but unexpired row is what a replayed + token is matched against, so removing it early would turn a stolen-token + replay into an ordinary rejection and leave the family alive.""" + user = await make_user("prune@test.com") + now = datetime.now(timezone.utc) + + rows = { + "prune-live": (now + timedelta(days=1), False), + "prune-revoked-live": (now + timedelta(days=1), True), + "prune-expired": (now - timedelta(days=1), False), + "prune-expired-revoked": (now - timedelta(days=1), True), + } + for token, (expires_at, revoked) in rows.items(): + await adapter.store_refresh_token( + RefreshToken( + token=token, + user_id=user.id, + expires_at=expires_at, + family_id="prune-family", + revoked=revoked, + ) + ) + + assert await adapter.prune_expired_refresh_tokens() == 2 + assert await adapter.get_refresh_token("prune-live") is not None + assert await adapter.get_refresh_token("prune-revoked-live") is not None + assert await adapter.get_refresh_token("prune-expired") is None + assert await adapter.get_refresh_token("prune-expired-revoked") is None + async def test_revoke_refresh_token_family(self, adapter, make_user): user = await make_user("rtf@test.com") for i in range(3): diff --git a/tests/test_account_safety.py b/tests/test_account_safety.py new file mode 100644 index 0000000..01dcb07 --- /dev/null +++ b/tests/test_account_safety.py @@ -0,0 +1,521 @@ +"""Keeping an account reachable and destructive actions deliberate. + +Covers the rules that decide whether a credential may be removed, whether a +caller has proved they are present, and the hygiene around long-lived tokens. +""" + +from datetime import datetime, timedelta, timezone +from unittest.mock import patch +from uuid import uuid4 + +import pytest +from fastapi import FastAPI +from httpx import ASGITransport, AsyncClient + +from fastapi_fullauth import FullAuth, FullAuthConfig +from fastapi_fullauth.oauth.base import OAuthProvider +from fastapi_fullauth.types import ( + CreateUserSchema, + OAuthAccount, + OAuthUserInfo, + PasskeyCredential, +) + +SECRET = "test-secret-key-that-is-long-enough-32b" + + +def _passkey(user_id, credential_id="cred-1") -> PasskeyCredential: + return PasskeyCredential( + id=uuid4(), + user_id=user_id, + credential_id=credential_id, + public_key="key", + device_name="Laptop", + ) + + +class _Provider(OAuthProvider): + """Present only so the OAuth routes mount; no flow runs through it here.""" + + name = "google" + + def __init__(self) -> None: + self.client_id = "id" + self.client_secret = "secret" + self.redirect_uris = ["http://localhost/callback"] + self.scopes = self.default_scopes + + @property + def default_scopes(self) -> list[str]: + return ["email"] + + def get_authorization_url(self, state: str, redirect_uri: str) -> str: + return "https://provider.test/auth" + + async def exchange_code(self, code: str, redirect_uri: str) -> dict: + raise NotImplementedError + + async def get_user_info(self, tokens: dict) -> OAuthUserInfo: + raise NotImplementedError + + +def _oauth(user_id, provider="google") -> OAuthAccount: + return OAuthAccount(provider=provider, provider_user_id="p-1", user_id=user_id) + + +@pytest.fixture +def passkey_config(): + return FullAuthConfig( + SECRET_KEY=SECRET, + PREVENT_REGISTRATION_ENUMERATION=False, + PREVENT_LOGIN_TIMING_ATTACKS=False, + PASSKEY_RP_ID="localhost", + PASSKEY_ORIGINS=["http://localhost"], + ) + + +def _app(config, adapter, *, providers=None): + fullauth = FullAuth(config=config, adapter=adapter, providers=providers) + app = FastAPI() + fullauth.init_app(app) + return app, fullauth + + +def _client(app): + return AsyncClient(transport=ASGITransport(app=app), base_url="http://test") + + +async def _passwordless_user(adapter, email="oauth-only@test.com"): + return await adapter.create_user( + CreateUserSchema(email=email, password="unused-securepass123"), + hashed_password=None, + ) + + +def _headers(fullauth, user, *, auth_age_seconds: int = 0): + """A real session token whose credentials were checked `auth_age_seconds` ago.""" + token = fullauth.token_engine.create_access_token( + user_id=str(user.id), + auth_time=datetime.now(timezone.utc) - timedelta(seconds=auth_age_seconds), + ) + return {"Authorization": f"Bearer {token}"} + + +# ── Removing a sign-in method ──────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_unlinking_oauth_counts_a_passkey_as_a_way_back_in(passkey_config, adapter): + """A passwordless user with a passkey does not need a password to unlink.""" + app, fullauth = _app(passkey_config, adapter, providers=[_Provider()]) + user = await _passwordless_user(adapter) + await adapter.create_oauth_account(_oauth(user.id)) + await adapter.store_passkey(_passkey(user.id)) + + async with _client(app) as client: + r = await client.delete( + "/api/v1/auth/oauth/accounts/google", headers=_headers(fullauth, user) + ) + assert r.status_code == 204 + assert await adapter.get_user_oauth_accounts(user.id) == [] + + +@pytest.mark.asyncio +async def test_unlinking_the_only_oauth_account_is_refused(passkey_config, adapter): + app, fullauth = _app(passkey_config, adapter, providers=[_Provider()]) + user = await _passwordless_user(adapter) + await adapter.create_oauth_account(_oauth(user.id)) + + async with _client(app) as client: + r = await client.delete( + "/api/v1/auth/oauth/accounts/google", headers=_headers(fullauth, user) + ) + assert r.status_code == 400 + assert "only sign-in method" in r.json()["detail"] + # The message names passkeys only where they are available. + assert "passkey" in r.json()["detail"] + assert len(await adapter.get_user_oauth_accounts(user.id)) == 1 + + +@pytest.mark.asyncio +async def test_deleting_the_only_passkey_is_refused(passkey_config, adapter): + """The mirror of the unlink rule: a passkey-only account must not be able to + delete its way out of existence.""" + app, fullauth = _app(passkey_config, adapter) + user = await _passwordless_user(adapter) + passkey = await adapter.store_passkey(_passkey(user.id)) + + async with _client(app) as client: + r = await client.delete( + f"/api/v1/auth/passkeys/{passkey.id}", headers=_headers(fullauth, user) + ) + assert r.status_code == 400 + assert "only sign-in method" in r.json()["detail"] + assert len(await adapter.get_user_passkeys(user.id)) == 1 + + +@pytest.mark.asyncio +async def test_deleting_a_passkey_is_allowed_when_a_password_remains(passkey_config, adapter): + app, fullauth = _app(passkey_config, adapter) + user = await adapter.create_user( + CreateUserSchema(email="haspw@test.com", password="securepass123"), + hashed_password="hashed", + ) + passkey = await adapter.store_passkey(_passkey(user.id)) + + async with _client(app) as client: + r = await client.delete( + f"/api/v1/auth/passkeys/{passkey.id}", headers=_headers(fullauth, user) + ) + assert r.status_code == 204 + assert await adapter.get_user_passkeys(user.id) == [] + + +@pytest.mark.asyncio +async def test_unlinking_a_provider_the_user_does_not_have_is_404(passkey_config, adapter): + app, fullauth = _app(passkey_config, adapter, providers=[_Provider()]) + user = await _passwordless_user(adapter) + await adapter.create_oauth_account(_oauth(user.id)) + + async with _client(app) as client: + r = await client.delete( + "/api/v1/auth/oauth/accounts/github", headers=_headers(fullauth, user) + ) + assert r.status_code == 404 + + +@pytest.mark.asyncio +async def test_a_passkey_does_not_count_when_passkey_sign_in_is_off(config, adapter): + """Turning PASSKEY_ENABLED off leaves the rows behind but removes the way to + sign in with them. Counting one would allow the lockout this rule prevents.""" + app, fullauth = _app(config, adapter, providers=[_Provider()]) # passkeys disabled + user = await _passwordless_user(adapter) + await adapter.create_oauth_account(_oauth(user.id)) + await adapter.store_passkey(_passkey(user.id)) + + async with _client(app) as client: + r = await client.delete( + "/api/v1/auth/oauth/accounts/google", headers=_headers(fullauth, user) + ) + assert r.status_code == 400 + # And the advice does not mention passkeys, since they are not available. + assert "passkey" not in r.json()["detail"] + assert len(await adapter.get_user_oauth_accounts(user.id)) == 1 + + +@pytest.mark.asyncio +async def test_a_link_to_an_unconfigured_provider_does_not_count(passkey_config, adapter): + """A provider dropped from the configured list cannot be signed in with, so + an old row for it is not a way back into the account.""" + # google is configured; the user's only link is to a provider that is not. + app, fullauth = _app(passkey_config, adapter, providers=[_Provider()]) + user = await _passwordless_user(adapter) + await adapter.create_oauth_account(_oauth(user.id, provider="retired-provider")) + passkey = await adapter.store_passkey(_passkey(user.id)) + + async with _client(app) as client: + r = await client.delete( + f"/api/v1/auth/passkeys/{passkey.id}", headers=_headers(fullauth, user) + ) + assert r.status_code == 400 + assert len(await adapter.get_user_passkeys(user.id)) == 1 + + +# ── Proving presence ───────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_deleting_an_account_needs_recent_authentication(config, adapter): + app, fullauth = _app(config, adapter) + user = await adapter.create_user( + CreateUserSchema(email="doomed@test.com", password="securepass123"), + hashed_password="$argon2id$v=19$m=65536,t=3,p=4$placeholder", + ) + + async with _client(app) as client: + stale = _headers(fullauth, user, auth_age_seconds=3600) + r = await client.request("DELETE", "/api/v1/auth/me", headers=stale) + assert r.status_code == 403 + assert r.json()["detail"] == "Re-authentication required" + assert await adapter.get_user_by_id(user.id) is not None + + fresh = _headers(fullauth, user, auth_age_seconds=10) + r = await client.request("DELETE", "/api/v1/auth/me", headers=fresh) + assert r.status_code == 204 + assert await adapter.get_user_by_id(user.id) is None + + +@pytest.mark.asyncio +async def test_the_current_password_also_proves_presence(config, adapter, client, auth_headers): + """A stale session can still delete by supplying the password.""" + fullauth = client._transport.app.state.fullauth + user = await fullauth.adapter.get_user_by_email("user@test.com") + stale = _headers(fullauth, user, auth_age_seconds=3600) + + r = await client.request( + "DELETE", "/api/v1/auth/me", headers=stale, json={"current_password": "wrong-password"} + ) + assert r.status_code == 403 + assert r.json()["detail"] == "Current password is incorrect" + + r = await client.request( + "DELETE", "/api/v1/auth/me", headers=stale, json={"current_password": "securepass123"} + ) + assert r.status_code == 204 + + +@pytest.mark.asyncio +async def test_setting_a_first_password_needs_recent_authentication(config, adapter): + """There is no current password to check, and it creates a new way in.""" + app, fullauth = _app(config, adapter) + user = await _passwordless_user(adapter) + body = {"new_password": "brand-new-pass-123"} + + async with _client(app) as client: + r = await client.post( + "/api/v1/auth/change-password", + json=body, + headers=_headers(fullauth, user, auth_age_seconds=3600), + ) + assert r.status_code == 403 + assert await adapter.get_hashed_password(user.id) is None + + r = await client.post( + "/api/v1/auth/change-password", + json=body, + headers=_headers(fullauth, user, auth_age_seconds=10), + ) + assert r.status_code == 200 + assert await adapter.get_hashed_password(user.id) is not None + + +@pytest.mark.asyncio +async def test_refreshing_does_not_renew_the_authentication_time(client, login_tokens, app): + """Otherwise anyone holding a refresh token could mint their way past the + re-authentication check.""" + fullauth = app.state.fullauth + original = ( + await fullauth.token_engine.decode_token( + login_tokens["access_token"], expected_type="access" + ) + ).auth_time + assert original is not None + + r = await client.post( + "/api/v1/auth/refresh", json={"refresh_token": login_tokens["refresh_token"]} + ) + assert r.status_code == 200 + rotated = await fullauth.token_engine.decode_token( + r.json()["access_token"], expected_type="access" + ) + assert rotated.auth_time == original + + +# ── Token and password hygiene ─────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_a_password_reset_link_dies_when_the_password_changes(config, adapter): + """Two links in an inbox, or a link plus a deliberate change: only the + newest may work.""" + from fastapi_fullauth.core.tokens import TokenEngine + from fastapi_fullauth.exceptions import TokenError + from fastapi_fullauth.flows.password_reset import request_password_reset, reset_password + + engine = TokenEngine(config=config) + user = await adapter.create_user( + CreateUserSchema(email="reset@test.com", password="securepass123"), + hashed_password="hashed-original", + ) + + first = await request_password_reset(adapter, engine, "reset@test.com") + second = await request_password_reset(adapter, engine, "reset@test.com") + assert first and second + + await reset_password(adapter, engine, second, "new-securepass-123") + + with pytest.raises(TokenError): + await reset_password(adapter, engine, first, "attacker-chosen-pass") + assert user is not None + + +@pytest.mark.asyncio +async def test_no_verification_token_for_an_already_verified_account(config, adapter): + from fastapi_fullauth.core.tokens import TokenEngine + from fastapi_fullauth.flows.email_verify import create_email_verification_token + + engine = TokenEngine(config=config) + user = await adapter.create_user( + CreateUserSchema(email="verified@test.com", password="securepass123"), + hashed_password="x", + ) + assert await create_email_verification_token(adapter, engine, user.id) is not None + + await adapter.set_user_verified(user.id) + assert await create_email_verification_token(adapter, engine, user.id) is None + + +@pytest.mark.asyncio +async def test_registering_rejects_an_oversized_password(client): + r = await client.post( + "/api/v1/auth/register", + json={"email": "huge@test.com", "password": "a" * 5000}, + ) + assert r.status_code == 422 + assert "at most" in r.json()["detail"] + + +@pytest.mark.asyncio +async def test_pruning_removes_expired_refresh_tokens_only(adapter): + """Rotation writes a row per refresh, so the table has to be prunable. A + revoked but unexpired row must survive: it is what a replayed token is + matched against.""" + from fastapi_fullauth.types import RefreshToken as RefreshTokenSchema + + user = await adapter.create_user( + CreateUserSchema(email="prune@test.com", password="securepass123"), + hashed_password="x", + ) + now = datetime.now(timezone.utc) + + async def store(token: str, expires_at: datetime, revoked: bool = False) -> None: + await adapter.store_refresh_token( + RefreshTokenSchema( + token=token, + user_id=user.id, + expires_at=expires_at, + family_id="fam", + revoked=revoked, + ) + ) + + await store("live", now + timedelta(days=1)) + await store("revoked-but-live", now + timedelta(days=1), revoked=True) + await store("expired", now - timedelta(days=1)) + await store("expired-revoked", now - timedelta(days=1), revoked=True) + + assert await adapter.prune_expired_refresh_tokens() == 2 + + assert await adapter.get_refresh_token("live") is not None + assert await adapter.get_refresh_token("revoked-but-live") is not None + assert await adapter.get_refresh_token("expired") is None + assert await adapter.get_refresh_token("expired-revoked") is None + + +@pytest.mark.asyncio +async def test_rotating_a_token_from_before_auth_time_does_not_refresh_it(config, adapter): + """Upgrade path: tokens minted before the claim existed carry no auth_time. + Rotating one must not stamp it with "now", or every such token in the wild + would become a re-authentication bypass for its whole 30-day life.""" + import jwt + + from fastapi_fullauth.core.crypto import hash_refresh_token + from fastapi_fullauth.core.tokens import TokenEngine + from fastapi_fullauth.flows.refresh import refresh + from fastapi_fullauth.types import RefreshToken as RefreshTokenSchema + + engine = TokenEngine(config=config) + user = await adapter.create_user( + CreateUserSchema(email="legacy@test.com", password="securepass123"), + hashed_password="x", + ) + + # Exactly what the previous release issued: no auth_time, and signed in days + # ago, which is the case that matters. + signed_in_at = datetime.now(timezone.utc) - timedelta(days=3) + legacy = jwt.encode( + { + "sub": str(user.id), + "exp": datetime.now(timezone.utc) + timedelta(days=27), + "iat": signed_in_at, + "jti": uuid4().hex, + "type": "refresh", + "family_id": "legacy-family", + }, + config.SECRET_KEY, + algorithm=config.ALGORITHM, + ) + await adapter.store_refresh_token( + RefreshTokenSchema( + token=hash_refresh_token(legacy), + user_id=user.id, + expires_at=datetime.now(timezone.utc) + timedelta(days=27), + family_id="legacy-family", + ) + ) + + pair = await refresh(adapter, engine, legacy) + rotated = await engine.decode_token(pair.access_token, expected_type="access") + + assert rotated.auth_time is not None + # Three days old, not seconds: rotation carried the original time through. + age = (datetime.now(timezone.utc) - rotated.auth_time).total_seconds() + assert age > 60 * 60 * 24 * 2 + + # And the session is therefore still too stale for a destructive action. + from fastapi_fullauth.exceptions import AuthenticationError + from fastapi_fullauth.flows.reauth import verify_recent_auth + + with pytest.raises(AuthenticationError): + await verify_recent_auth(rotated, hashed_password=None, max_age_seconds=300) + + +@pytest.mark.asyncio +async def test_the_password_check_on_delete_is_rate_limited(config, adapter): + """Whoever holds the token can guess the password here, and a correct guess + deletes the account, so the attempts are metered like a sign-in.""" + app, fullauth = _app(config, adapter) + user = await adapter.create_user( + CreateUserSchema(email="oracle@test.com", password="securepass123"), + hashed_password="$argon2id$v=19$m=65536,t=3,p=4$placeholder", + ) + stale = _headers(fullauth, user, auth_age_seconds=3600) + + statuses = [] + async with _client(app) as client: + for _ in range(fullauth.config.AUTH_RATE_LIMITS.reauth + 2): + r = await client.request( + "DELETE", + "/api/v1/auth/me", + headers=stale, + json={"current_password": "wrong-guess"}, + ) + statuses.append(r.status_code) + + assert 429 in statuses, statuses + assert await adapter.get_user_by_id(user.id) is not None + + +@pytest.mark.asyncio +async def test_login_rejects_an_oversized_password_without_hashing_it(config, adapter): + """The cap exists to stop a body being turned into hashing work, and login is + the path an attacker can reach without an account.""" + import importlib + from unittest.mock import AsyncMock + + from fastapi_fullauth.core.tokens import TokenEngine + from fastapi_fullauth.exceptions import AuthenticationError + + # Patched by object, not by name: on 3.10 "flows.login" resolves to the + # re-exported function rather than the module. + login_module = importlib.import_module("fastapi_fullauth.flows.login") + await adapter.create_user( + CreateUserSchema(email="long@test.com", password="securepass123"), + hashed_password="x", + ) + + verify = AsyncMock(return_value=True) + with ( + patch.object(login_module, "averify_password", verify), + pytest.raises(AuthenticationError), + ): + await login_module.login( + adapter=adapter, + token_engine=TokenEngine(config=config), + identifier="long@test.com", + password="a" * 5000, + prevent_timing_attacks=True, + max_password_length=config.PASSWORD_MAX_LENGTH, + ) + + verify.assert_not_awaited() diff --git a/tests/test_auth.py b/tests/test_auth.py index e441305..0156f61 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -1068,10 +1068,11 @@ async def test_verify_email_token_burned_when_already_verified(): CreateUserSchema(email="already@test.com", password="securepass123"), hashed_password="x", ) - await adapter.set_user_verified(user.id) - + # Issued while still unverified, then verified through another path - which + # is exactly how a live token ends up pointing at a verified account. token = await create_email_verification_token(adapter, token_engine, user.id) assert token is not None + await adapter.set_user_verified(user.id) # First use resolves the already-verified account and burns the token. await verify_email(adapter, token_engine, token) diff --git a/tests/test_oauth.py b/tests/test_oauth.py index 30b504e..2ee6cd4 100644 --- a/tests/test_oauth.py +++ b/tests/test_oauth.py @@ -1,5 +1,9 @@ """Tests for OAuth2 social login.""" +from unittest.mock import AsyncMock, patch +from urllib.parse import parse_qs, urlparse +from uuid import uuid4 + import pytest from fastapi import FastAPI from httpx import ASGITransport, AsyncClient @@ -8,9 +12,17 @@ from fastapi_fullauth import FullAuth, FullAuthConfig from fastapi_fullauth.adapters.sqlmodel import SQLModelAdapter -from fastapi_fullauth.flows.oauth import generate_oauth_state, oauth_callback, verify_oauth_state +from fastapi_fullauth.exceptions import OAuthAccountAlreadyLinkedError +from fastapi_fullauth.flows.oauth import ( + build_link_authorization_url, + generate_oauth_state, + link_oauth_account, + oauth_callback, + verify_oauth_state, +) from fastapi_fullauth.oauth.base import OAuthProvider -from fastapi_fullauth.types import OAuthUserInfo +from fastapi_fullauth.types import CreateUserSchema, OAuthUserInfo +from fastapi_fullauth.types import OAuthAccount as OAuthAccountSchema from tests.conftest import OAuthAccount, RefreshToken, Role, User, UserRole BINDING = "client-binding-secret" @@ -444,8 +456,6 @@ async def test_callback_rejects_a_state_issued_to_another_client(oauth_app): submit the attacker's code and state, signing the victim into the attacker's account. The state must be bound to the client that requested it, so the victim's own binding cannot redeem the attacker's state.""" - from urllib.parse import parse_qs, urlparse - transport = ASGITransport(app=oauth_app) async with AsyncClient(transport=transport, base_url="http://test") as client: params = {"redirect_uri": "http://localhost/callback"} @@ -1003,3 +1013,382 @@ def handler(request: httpx.Request) -> httpx.Response: ) assert r.status_code == 400 await provider.aclose() + + +# ── Linking a provider while signed in ─────────────────────────── + +REDIRECT = "http://localhost/callback" + + +def _identity(provider_user_id: str, email: str = "linked@example.com") -> OAuthUserInfo: + return OAuthUserInfo( + provider="mock", + provider_user_id=provider_user_id, + email=email, + email_verified=True, + name="Linked User", + ) + + +def _app_with(config, adapter, provider): + fullauth = FullAuth(config=config, adapter=adapter, providers=[provider]) + app = FastAPI() + fullauth.init_app(app) + return app, fullauth + + +def _state_of(authorization_url: str) -> str: + return parse_qs(urlparse(authorization_url).query)["state"][0] + + +def _client(app): + return AsyncClient(transport=ASGITransport(app=app), base_url="http://test") + + +async def _sign_in(client, email: str = "owner@example.com") -> dict: + body = {"email": email, "password": "securepass123"} + await client.post("/api/v1/auth/register", json=body) + login = await client.post("/api/v1/auth/login", json=body) + return {"Authorization": "Bearer " + login.json()["access_token"]} + + +async def _start_link(client, headers, provider: str = "mock"): + r = await client.get( + "/api/v1/auth/oauth/" + provider + "/link/authorize", + params={"redirect_uri": REDIRECT}, + headers=headers, + ) + assert r.status_code == 200, r.text + return r.json() + + +async def _link(client, headers, *, binding: str | None = None): + """Run the whole link round trip and return the callback response.""" + started = await _start_link(client, headers) + return await client.post( + "/api/v1/auth/oauth/mock/link/callback", + json={ + "code": "link-code", + "state": _state_of(started["authorization_url"]), + "binding": binding or started["binding"], + }, + headers=headers, + ) + + +async def _linked_ids(client, headers) -> list: + r = await client.get("/api/v1/auth/oauth/accounts", headers=headers) + return [a["provider_user_id"] for a in r.json()] + + +@pytest.mark.asyncio +async def test_link_attaches_the_provider_without_touching_the_user(config, adapter): + """Authorization comes from the session, so the provider email is stored for + display only: it neither has to match the account nor verifies it.""" + app, _ = _app_with(config, adapter, MockOAuthProvider(_identity("gh-1", "work@example.com"))) + async with _client(app) as client: + headers = await _sign_in(client) + + r = await _link(client, headers) + assert r.status_code == 200 + assert r.json() == { + "provider": "mock", + "provider_user_id": "gh-1", + "provider_email": "work@example.com", + } + assert await _linked_ids(client, headers) == ["gh-1"] + + me = (await client.get("/api/v1/auth/me", headers=headers)).json() + assert me["email"] == "owner@example.com" + assert me["is_verified"] is False + + +@pytest.mark.asyncio +async def test_link_routes_require_authentication(oauth_app): + async with _client(oauth_app) as client: + r = await client.get( + "/api/v1/auth/oauth/mock/link/authorize", params={"redirect_uri": REDIRECT} + ) + assert r.status_code == 401 + + r = await client.post( + "/api/v1/auth/oauth/mock/link/callback", + json={"code": "c", "state": "s", "binding": BINDING}, + ) + assert r.status_code == 401 + + +@pytest.mark.asyncio +async def test_link_is_idempotent(config, adapter): + app, _ = _app_with(config, adapter, MockOAuthProvider(_identity("gh-1"))) + async with _client(app) as client: + headers = await _sign_in(client) + + assert (await _link(client, headers)).status_code == 200 + assert (await _link(client, headers)).status_code == 200 + assert await _linked_ids(client, headers) == ["gh-1"] + + +@pytest.mark.asyncio +async def test_link_refuses_an_identity_owned_by_another_user(config, adapter): + """The takeover case: linking must never move an identity between accounts, + or anyone could attach a victim's provider account and then sign in as them.""" + app, _ = _app_with(config, adapter, MockOAuthProvider(_identity("gh-1"))) + async with _client(app) as client: + owner = await _sign_in(client, "owner@example.com") + assert (await _link(client, owner)).status_code == 200 + + thief = await _sign_in(client, "thief@example.com") + r = await _link(client, thief) + assert r.status_code == 409 + assert "another user" in r.json()["detail"] + + assert await _linked_ids(client, thief) == [] + assert await _linked_ids(client, owner) == ["gh-1"] + + +@pytest.mark.asyncio +async def test_link_refuses_a_second_account_from_the_same_provider(config, adapter): + """Unlinking resolves by provider alone, so two accounts for one provider + would make it ambiguous.""" + provider = MockOAuthProvider(_identity("gh-1")) + app, _ = _app_with(config, adapter, provider) + async with _client(app) as client: + headers = await _sign_in(client) + assert (await _link(client, headers)).status_code == 200 + + provider._user_info = _identity("gh-2", "second@example.com") + r = await _link(client, headers) + assert r.status_code == 409 + assert "already linked to a different" in r.json()["detail"] + assert await _linked_ids(client, headers) == ["gh-1"] + + +@pytest.mark.asyncio +async def test_link_and_sign_in_states_are_not_interchangeable(config, adapter): + app, fullauth = _app_with(config, adapter, MockOAuthProvider(_identity("gh-1"))) + async with _client(app) as client: + headers = await _sign_in(client) + + started = await _start_link(client, headers) + r = await client.post( + "/api/v1/auth/oauth/mock/callback", + json={ + "code": "c", + "state": _state_of(started["authorization_url"]), + "binding": started["binding"], + }, + ) + assert r.status_code == 400 + + sign_in_state = generate_oauth_state(fullauth.token_engine, binding=BINDING) + r = await client.post( + "/api/v1/auth/oauth/mock/link/callback", + json={"code": "c", "state": sign_in_state, "binding": BINDING}, + headers=headers, + ) + assert r.status_code == 400 + assert await _linked_ids(client, headers) == [] + + +@pytest.mark.asyncio +async def test_link_state_issued_for_another_user_is_rejected(config, adapter): + """Even holding the matching binding, a state naming someone else must not + attach that flow's identity to the caller.""" + app, _ = _app_with(config, adapter, MockOAuthProvider(_identity("gh-1"))) + async with _client(app) as client: + attacker = await _sign_in(client, "attacker@example.com") + started = await _start_link(client, attacker) + + victim = await _sign_in(client, "victim@example.com") + r = await client.post( + "/api/v1/auth/oauth/mock/link/callback", + json={ + "code": "c", + "state": _state_of(started["authorization_url"]), + "binding": started["binding"], + }, + headers=victim, + ) + assert r.status_code == 400 + assert await _linked_ids(client, victim) == [] + + +@pytest.mark.asyncio +async def test_link_callback_rejects_a_wrong_binding(config, adapter): + app, _ = _app_with(config, adapter, MockOAuthProvider(_identity("gh-1"))) + async with _client(app) as client: + headers = await _sign_in(client) + r = await _link(client, headers, binding="not-the-binding") + assert r.status_code == 400 + assert await _linked_ids(client, headers) == [] + + +@pytest.mark.asyncio +async def test_link_state_is_useless_as_a_session_token(config, adapter): + """The state rides in the address bar and reaches the provider, so it must + not authenticate anything, even though it is an access-typed JWT.""" + app, _ = _app_with(config, adapter, MockOAuthProvider(_identity("gh-1"))) + async with _client(app) as client: + headers = await _sign_in(client) + started = await _start_link(client, headers) + state = _state_of(started["authorization_url"]) + + r = await client.get("/api/v1/auth/me", headers={"Authorization": "Bearer " + state}) + assert r.status_code == 401 + + +@pytest.mark.asyncio +async def test_link_authorize_validates_provider_and_redirect_uri(config, adapter): + app, _ = _app_with(config, adapter, MockOAuthProvider(_identity("gh-1"))) + async with _client(app) as client: + headers = await _sign_in(client) + + r = await client.get( + "/api/v1/auth/oauth/unknown/link/authorize", + params={"redirect_uri": REDIRECT}, + headers=headers, + ) + assert r.status_code == 404 + + r = await client.get( + "/api/v1/auth/oauth/mock/link/authorize", + params={"redirect_uri": "http://evil.test/callback"}, + headers=headers, + ) + assert r.status_code == 400 + + +@pytest.mark.asyncio +async def test_link_replaces_a_row_whose_owner_no_longer_exists(adapter): + """Storage without cascading deletes can outlive the user; the orphan must + not block the identity forever.""" + user = await adapter.create_user( + CreateUserSchema(email="relink@example.com", password="securepass123"), + hashed_password="x", + ) + await adapter.create_oauth_account( + OAuthAccountSchema( + provider="mock", + provider_user_id="gh-1", + user_id=uuid4(), + provider_email="ghost@example.com", + ) + ) + + account = await link_oauth_account(adapter, user, _identity("gh-1"), {"access_token": "t"}) + assert account.user_id == user.id + assert [a.provider_user_id for a in await adapter.get_user_oauth_accounts(user.id)] == ["gh-1"] + + +@pytest.mark.asyncio +async def test_link_state_carries_the_user_id_outside_the_subject(config): + """A real user id in `sub` would make the state a usable session token.""" + from fastapi_fullauth.core.tokens import TokenEngine + + engine = TokenEngine(config) + user_id = uuid4() + url = build_link_authorization_url( + engine, MockOAuthProvider(), REDIRECT, user_id, binding=BINDING + ) + payload = await engine.decode_token(_state_of(url), expected_type="access") + + assert payload.sub == "oauth-state" + assert payload.extra["link_user_id"] == str(user_id) + assert payload.extra["purpose"] == "oauth_link" + + +@pytest.mark.asyncio +async def test_sign_in_refuses_a_second_identity_from_the_same_provider(config, adapter): + """Two accounts at one provider can verify the same email. Attaching both to + one user would leave `DELETE /oauth/accounts/{provider}` ambiguous: it + resolves by provider alone, so it would delete one and leave the other.""" + provider = MockOAuthProvider(_identity("google-1", "dup@example.com")) + app, fullauth = _app_with(config, adapter, provider) + async with _client(app) as client: + first = await client.post( + "/api/v1/auth/oauth/mock/callback", + json={ + "code": "c", + "state": generate_oauth_state(fullauth.token_engine, binding=BINDING), + "binding": BINDING, + }, + ) + assert first.status_code == 200 + headers = {"Authorization": "Bearer " + first.json()["access_token"]} + + provider._user_info = _identity("google-2", "dup@example.com") + second = await client.post( + "/api/v1/auth/oauth/mock/callback", + json={ + "code": "c", + "state": generate_oauth_state(fullauth.token_engine, binding=BINDING), + "binding": BINDING, + }, + ) + # Generic, like every other sign-in failure: never a 500, and nothing + # about the account is revealed to an unauthenticated caller. + assert second.status_code == 400 + assert second.json()["detail"] == "OAuth authentication failed" + assert await _linked_ids(client, headers) == ["google-1"] + + +@pytest.mark.asyncio +async def test_link_rejects_an_account_that_a_race_gave_to_someone_else(adapter): + """Every adapter treats a duplicate insert as success and returns the row + that won, so a lost race hands back another user's account. Reporting that + as a successful link would tell the caller they own an identity they do not.""" + user = await adapter.create_user( + CreateUserSchema(email="racer@example.com", password="securepass123"), + hashed_password="x", + ) + winner = await adapter.create_user( + CreateUserSchema(email="winner@example.com", password="securepass123"), + hashed_password="x", + ) + foreign = OAuthAccountSchema(provider="mock", provider_user_id="gh-1", user_id=winner.id) + + with ( + patch.object(type(adapter), "create_oauth_account", AsyncMock(return_value=foreign)), + pytest.raises(OAuthAccountAlreadyLinkedError), + ): + await link_oauth_account(adapter, user, _identity("gh-1"), {"access_token": "t"}) + + +@pytest.mark.asyncio +async def test_oauth_routes_answer_501_on_an_adapter_without_oauth(config): + """The combined router only mounts these when the adapter supports OAuth, but + the routers are composable: mounted by hand, the adapter's RuntimeError would + otherwise surface as a 500.""" + engine, session_maker = await _make_db() + adapter = SQLModelAdapter( + session_maker=session_maker, + user_model=User, + refresh_token_model=RefreshToken, + ) + fullauth = FullAuth(config=config, adapter=adapter, providers=[MockOAuthProvider()]) + app = FastAPI() + fullauth.bind(app) + app.include_router(fullauth.oauth_router) + + user = await adapter.create_user( + CreateUserSchema(email="no-oauth@test.com", password="securepass123"), + hashed_password="x", + ) + token = fullauth.token_engine.create_access_token(user_id=str(user.id)) + headers = {"Authorization": "Bearer " + token} + + async with _client(app) as client: + r = await client.get("/oauth/accounts", headers=headers) + assert r.status_code == 501 + assert r.json()["detail"] == "Adapter does not support OAuth" + + r = await client.get( + "/oauth/mock/link/authorize", params={"redirect_uri": REDIRECT}, headers=headers + ) + assert r.status_code == 501 + + r = await client.delete("/oauth/accounts/mock", headers=headers) + assert r.status_code == 501 + + await engine.dispose() diff --git a/tests/test_passkey.py b/tests/test_passkey.py index 4fd3b5f..6eae916 100644 --- a/tests/test_passkey.py +++ b/tests/test_passkey.py @@ -1,5 +1,7 @@ """Tests for passkey (WebAuthn) challenge store and adapter methods.""" +import time +from unittest.mock import patch from uuid import UUID import pytest @@ -257,3 +259,22 @@ async def test_registration_stores_only_known_transports(passkey_adapter): assert passkey.transports == ["internal", "hybrid"] stored = await passkey_adapter.get_passkey_by_credential_id(passkey.credential_id) assert stored is not None and stored.transports == ["internal", "hybrid"] + + +@pytest.mark.asyncio +async def test_challenge_store_evicts_challenges_that_were_never_used(): + """Beginning a passkey sign-in needs no account, so unclaimed challenges are + attacker-reachable: they have to expire out of memory on their own.""" + from fastapi_fullauth.protection import challenges + + store = InMemoryChallengeStore() + for i in range(50): + await store.store(f"abandoned-{i}", "c", ttl=1) + assert len(store._store) == 50 + + # Jump past both the entries' TTL and the sweep interval. + store._next_sweep = 0.0 + with patch.object(challenges.time, "monotonic", return_value=time.monotonic() + 3600): + await store.store("fresh", "c", ttl=60) + + assert list(store._store) == ["fresh"] diff --git a/tests/test_polish.py b/tests/test_polish.py index a67c17e..088b4f1 100644 --- a/tests/test_polish.py +++ b/tests/test_polish.py @@ -241,15 +241,74 @@ async def test_redis_backends_share_one_client_per_url(): blacklist = RedisTokenBlacklist(url) lockout = RedisLockoutManager(url) assert blacklist._redis is lockout._redis + key = blacklist._redis_key + assert key == lockout._redis_key await blacklist.aclose() - assert url in _clients # still held by the lockout manager + assert key in _clients # still held by the lockout manager await lockout.aclose() - assert url not in _clients + assert key not in _clients await lockout.aclose() # double-close must not raise or disturb the registry - assert url not in _clients + assert key not in _clients + + +def test_a_new_event_loop_never_inherits_a_dead_loop_client(): + """`id(loop)` is recycled almost immediately after a loop is closed, so an + id-keyed registry hands a fresh loop a client bound to the dead one. The key + holds the loop object, which cannot be recycled while the entry refers to it.""" + import asyncio + + from fastapi_fullauth.core._redis import _clients, acquire_redis, release_redis + + url = "redis://loop-isolation-test:6379/0" + clients: list[object] = [] + keys: list[tuple] = [] + + def acquire_in_a_fresh_loop() -> None: + async def inner() -> None: + client, key = acquire_redis(url, feature="the test") + clients.append(client) + keys.append(key) + # Deliberately not released: this is the abandoned-holder case that + # leaves an entry behind for the next loop to trip over. + + asyncio.run(inner()) + + for _ in range(5): + acquire_in_a_fresh_loop() + + assert len({id(c) for c in clients}) == 5, "a loop was handed another loop's client" + assert len({k[1] for k in keys}) == 5 + + async def cleanup() -> None: + for key in keys: + await release_redis(key) + + asyncio.run(cleanup()) + assert not [k for k in _clients if k[0] == url] + + +@pytest.mark.asyncio +async def test_lockout_fails_open_when_redis_is_unreachable(): + """Failing closed would lock every account out during an outage, and raising + would turn every login into a 500. The rate limiter makes the same trade.""" + from unittest.mock import AsyncMock, MagicMock + + from fastapi_fullauth.protection.lockout import RedisLockoutManager + + lockout = RedisLockoutManager("redis://lockout-outage-test:6379/0") + down = MagicMock() + down.get = AsyncMock(side_effect=ConnectionError("redis is down")) + down.pipeline = MagicMock(side_effect=ConnectionError("redis is down")) + lockout._redis = down + + assert await lockout.is_locked("victim@test.com") is False + await lockout.record_failure("victim@test.com") + await lockout.clear("victim@test.com") + + await lockout.aclose() # ── init_app composes aclose() with a custom lifespan ─────────────── diff --git a/tests/test_rbac.py b/tests/test_rbac.py index b614b8a..71d6565 100644 --- a/tests/test_rbac.py +++ b/tests/test_rbac.py @@ -501,3 +501,51 @@ async def test_require_permission_rejects_adapter_without_permission_support(): with pytest.raises(RuntimeError, match="PermissionAdapterMixin"): await _dep(user=fake_user, fullauth=fake_fullauth) + + +@pytest.mark.asyncio +async def test_permission_routes_answer_501_without_permission_models(): + """The admin router mounts on role support alone, so these routes exist even + when the adapter has no permission models. That must not be a 500.""" + engine, session_maker = await _make_db() + adapter = SQLModelAdapter( + session_maker=session_maker, + user_model=User, + refresh_token_model=RefreshToken, + role_model=Role, + user_role_model=UserRole, + user_schema=UserSchemaWithRoles, + ) + fullauth = FullAuth( + config=FullAuthConfig(SECRET_KEY="test-secret-key-that-is-long-enough-32b"), + adapter=adapter, + ) + app = FastAPI() + fullauth.init_app(app) + + transport = ASGITransport(app=app) + async with AsyncClient(transport=transport, base_url="http://test") as client: + tokens = await _make_superuser(client, adapter) + headers = {"Authorization": f"Bearer {tokens['access_token']}"} + + r = await client.post( + "/api/v1/auth/admin/assign-permission", + json={"role": "editor", "permission": "posts:create"}, + headers=headers, + ) + assert r.status_code == 501 + assert r.json()["detail"] == "Adapter does not support permissions" + + r = await client.get("/api/v1/auth/admin/role-permissions/editor", headers=headers) + assert r.status_code == 501 + + # Role management still works; only permissions are unavailable. + user = await adapter.get_user_by_email("admin@test.com") + r = await client.post( + "/api/v1/auth/admin/assign-role", + json={"user_id": str(user.id), "role": "editor"}, + headers=headers, + ) + assert r.status_code == 200 + + await engine.dispose() diff --git a/tests/test_tokens.py b/tests/test_tokens.py index 8239040..1c92051 100644 --- a/tests/test_tokens.py +++ b/tests/test_tokens.py @@ -158,12 +158,14 @@ async def test_redis_blacklist_zero_ttl_does_not_become_default(): bl._default_ttl = 1800 bl._prefix = "fullauth:blacklist:" + # No exception is itself part of the assertion: real setex rejects a 0 ttl. await bl.add("jti-zero", ttl_seconds=0) - # Assert in milliseconds: `ttl` (whole seconds) floors a just-set 1s key to 0, - # which is flaky. `pttl` right after setex is ~1000ms and unambiguously not the - # 1800s (1_800_000 ms) default. + + # Only an upper bound. The entry is given one second, so any lower bound is a + # race against the suite's own scheduling: asserting the key is still alive + # fails whenever this coroutine is descheduled for longer than that. pttl = await bl._redis.pttl("fullauth:blacklist:jti-zero") - assert 0 < pttl <= 1000 # floored to ~1s, not the 1800s default + assert pttl <= 1000 # never the 1800s (1_800_000 ms) default @pytest.mark.asyncio