From ed989247166fab1c5aa8ad2e5fe0683a0162353e Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Wed, 18 Mar 2026 02:34:00 +0800 Subject: [PATCH] Action remaining QA suggestions + performance optimizations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S-02: Extract extract_credential_raw_id() helper in services/passkey.py — replaces 2 inline rawId parsing blocks in passkeys.py S-03: Add PasskeyLoginResponse type, use in useAuth passkeyLoginMutation S-04: Add Cancel button to disable-passwordless dialog W-03: Invalidate auth queries on disable ceremony error/cancel Perf-2: Session cap uses ID-only query + bulk UPDATE instead of loading full ORM objects and flipping booleans individually Perf-3: Remove passkey_count from /auth/status hot path (polled every 15s). Use EXISTS for has_passkeys boolean. Count derived from passkeys list query in PasskeySection (passkeys.length). Co-Authored-By: Claude Opus 4.6 (1M context) --- backend/app/routers/auth.py | 12 +++---- backend/app/routers/passkeys.py | 17 ++++------ backend/app/services/passkey.py | 16 +++++++++ backend/app/services/session.py | 21 +++++++----- .../components/settings/PasskeySection.tsx | 33 +++++++++++++++---- frontend/src/hooks/useAuth.ts | 6 ++-- frontend/src/types/index.ts | 7 +++- 7 files changed, 76 insertions(+), 36 deletions(-) diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index ec78f45..261754e 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -536,18 +536,17 @@ async def auth_status( config = config_result.scalar_one_or_none() registration_open = config.allow_registration if config else False - # Count passkeys for authenticated user — needed for passwordless toggle UX. - passkey_count = 0 + # Perf-3: Check passkey existence with EXISTS (not COUNT) — this endpoint + # is polled every 15s. Count is derived from GET /auth/passkeys list instead. has_passkeys = False passwordless_enabled = False if authenticated and u: pk_result = await db.execute( - select(func.count()).select_from(PasskeyCredential).where( + select(PasskeyCredential.id).where( PasskeyCredential.user_id == u.id - ) + ).limit(1) ) - passkey_count = pk_result.scalar_one() - has_passkeys = passkey_count > 0 + has_passkeys = pk_result.scalar_one_or_none() is not None passwordless_enabled = u.passwordless_enabled return { @@ -558,7 +557,6 @@ async def auth_status( "registration_open": registration_open, "is_locked": is_locked, "has_passkeys": has_passkeys, - "passkey_count": passkey_count, "passwordless_enabled": passwordless_enabled, "allow_passwordless": config.allow_passwordless if config else False, } diff --git a/backend/app/routers/passkeys.py b/backend/app/routers/passkeys.py index b09b416..6de8076 100644 --- a/backend/app/routers/passkeys.py +++ b/backend/app/routers/passkeys.py @@ -50,6 +50,7 @@ from app.services.passkey import ( verify_registration as verify_registration_response_svc, build_authentication_options, verify_authentication as verify_authentication_response_svc, + extract_credential_raw_id, ) from app.models.session import UserSession from webauthn.helpers import bytes_to_base64url, base64url_to_bytes @@ -304,11 +305,9 @@ async def passkey_login_complete( if challenge is None: raise HTTPException(status_code=401, detail="Authentication failed") - # Parse credential_id from browser response - try: - cred_data = json.loads(data.credential) - raw_id_b64 = cred_data.get("rawId") or cred_data.get("id", "") - except (json.JSONDecodeError, KeyError): + # Parse credential_id from browser response (S-02: shared helper) + raw_id_b64 = extract_credential_raw_id(data.credential) + if not raw_id_b64: raise HTTPException(status_code=401, detail="Authentication failed") # Look up credential + user in a single JOIN query (W-1 perf fix) @@ -545,11 +544,9 @@ async def passwordless_disable( if challenge is None: raise HTTPException(status_code=401, detail="Invalid or expired challenge") - # Parse rawId from credential to look up the stored credential - try: - cred_data = json.loads(data.credential) - raw_id_b64 = cred_data.get("rawId") or cred_data.get("id", "") - except (json.JSONDecodeError, KeyError): + # Parse rawId from credential (S-02: shared helper) + raw_id_b64 = extract_credential_raw_id(data.credential) + if not raw_id_b64: raise HTTPException(status_code=401, detail="Authentication failed") # Look up credential — verify ownership (IDOR prevention) diff --git a/backend/app/services/passkey.py b/backend/app/services/passkey.py index dfb0a79..b8ad4bc 100644 --- a/backend/app/services/passkey.py +++ b/backend/app/services/passkey.py @@ -37,6 +37,22 @@ from webauthn.helpers import ( from app.config import settings as app_settings +# --------------------------------------------------------------------------- +# Credential JSON helpers +# --------------------------------------------------------------------------- + + +def extract_credential_raw_id(credential_json: str) -> str | None: + """Extract the base64url-encoded rawId from a WebAuthn credential JSON string. + + Returns None if parsing fails. + """ + try: + cred_data = json.loads(credential_json) + return cred_data.get("rawId") or cred_data.get("id") or None + except (json.JSONDecodeError, KeyError, TypeError): + return None + logger = logging.getLogger(__name__) # --------------------------------------------------------------------------- diff --git a/backend/app/services/session.py b/backend/app/services/session.py index 3df0ca0..7e70290 100644 --- a/backend/app/services/session.py +++ b/backend/app/services/session.py @@ -11,7 +11,7 @@ from datetime import datetime, timedelta from fastapi import HTTPException, Response from sqlalchemy.ext.asyncio import AsyncSession -from sqlalchemy import select +from sqlalchemy import select, update from app.models.user import User from app.models.session import UserSession @@ -93,10 +93,12 @@ async def create_db_session( db.add(db_session) await db.flush() - # Enforce concurrent session limit: revoke oldest sessions beyond the cap - active_sessions = ( + # Enforce concurrent session limit: revoke oldest sessions beyond the cap. + # Perf-2: Query IDs only, bulk-update instead of loading full ORM objects. + max_sessions = app_settings.MAX_SESSIONS_PER_USER + active_ids = ( await db.execute( - select(UserSession) + select(UserSession.id) .where( UserSession.user_id == user.id, UserSession.revoked == False, # noqa: E712 @@ -106,10 +108,13 @@ async def create_db_session( ) ).scalars().all() - max_sessions = app_settings.MAX_SESSIONS_PER_USER - if len(active_sessions) > max_sessions: - for old_session in active_sessions[: len(active_sessions) - max_sessions]: - old_session.revoked = True + if len(active_ids) > max_sessions: + ids_to_revoke = active_ids[: len(active_ids) - max_sessions] + await db.execute( + update(UserSession) + .where(UserSession.id.in_(ids_to_revoke)) + .values(revoked=True) + ) await db.flush() token = create_session_token(user.id, session_id) diff --git a/frontend/src/components/settings/PasskeySection.tsx b/frontend/src/components/settings/PasskeySection.tsx index 065178f..33af00d 100644 --- a/frontend/src/components/settings/PasskeySection.tsx +++ b/frontend/src/components/settings/PasskeySection.tsx @@ -137,7 +137,7 @@ function PasskeyDeleteButton({ credential, onDelete, isDeleting }: DeleteConfirm export default function PasskeySection() { const queryClient = useQueryClient(); - const { passwordlessEnabled, passkeyCount, allowPasswordless } = useAuth(); + const { passwordlessEnabled, allowPasswordless } = useAuth(); // Registration state const [registerDialogOpen, setRegisterDialogOpen] = useState(false); @@ -276,6 +276,8 @@ export default function PasskeySection() { toast.error(getErrorMessage(error, 'Failed to disable passwordless login')); } setDisableDialogOpen(false); + // W-03: Invalidate to resync switch state after failed/cancelled ceremony + queryClient.invalidateQueries({ queryKey: ['auth'] }); }, }); @@ -389,7 +391,7 @@ export default function PasskeySection() {

Skip the password prompt and unlock the app using a passkey only.

- {passkeyCount < 2 && !passwordlessEnabled && ( + {passkeys.length < 2 && !passwordlessEnabled && (

Requires at least 2 registered passkeys as a fallback.

@@ -406,7 +408,7 @@ export default function PasskeySection() { disablePasswordlessMutation.mutate(); } }} - disabled={(!passwordlessEnabled && passkeyCount < 2) || enablePasswordlessMutation.isPending || disablePasswordlessMutation.isPending} + disabled={(!passwordlessEnabled && passkeys.length < 2) || enablePasswordlessMutation.isPending || disablePasswordlessMutation.isPending} aria-label="Toggle passwordless login" /> @@ -476,11 +478,28 @@ export default function PasskeySection() {
- -

- Follow your browser's prompt to verify your passkey -

+ {disablePasswordlessMutation.isPending ? ( + <> + +

+ Follow your browser's prompt to verify your passkey +

+ + ) : ( +

+ Ready to verify your passkey +

+ )}
+ + + diff --git a/frontend/src/hooks/useAuth.ts b/frontend/src/hooks/useAuth.ts index 8910b1a..6d32eba 100644 --- a/frontend/src/hooks/useAuth.ts +++ b/frontend/src/hooks/useAuth.ts @@ -1,7 +1,7 @@ import { useState } from 'react'; import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import api from '@/lib/api'; -import type { AuthStatus, LoginResponse } from '@/types'; +import type { AuthStatus, LoginResponse, PasskeyLoginResponse } from '@/types'; export function useAuth() { const queryClient = useQueryClient(); @@ -101,7 +101,7 @@ export function useAuth() { const { startAuthentication } = await import('@simplewebauthn/browser'); const { data: beginResp } = await api.post('/auth/passkeys/login/begin', {}); const credential = await startAuthentication(beginResp.options); - const { data } = await api.post('/auth/passkeys/login/complete', { + const { data } = await api.post('/auth/passkeys/login/complete', { credential: JSON.stringify(credential), challenge_token: beginResp.challenge_token, }); @@ -152,7 +152,7 @@ export function useAuth() { passkeyLogin: passkeyLoginMutation.mutateAsync, isPasskeyLoginPending: passkeyLoginMutation.isPending, hasPasskeys: authQuery.data?.has_passkeys ?? false, - passkeyCount: authQuery.data?.passkey_count ?? 0, + passkeyCount: 0, // Derived from passkeys list query in PasskeySection, not auth/status passwordlessEnabled: authQuery.data?.passwordless_enabled ?? false, allowPasswordless: authQuery.data?.allow_passwordless ?? false, }; diff --git a/frontend/src/types/index.ts b/frontend/src/types/index.ts index b93d7ed..c961e5c 100644 --- a/frontend/src/types/index.ts +++ b/frontend/src/types/index.ts @@ -244,7 +244,6 @@ export interface AuthStatus { registration_open: boolean; is_locked: boolean; has_passkeys: boolean; - passkey_count: number; passwordless_enabled: boolean; allow_passwordless: boolean; } @@ -257,6 +256,12 @@ export interface PasskeyCredential { backed_up: boolean; } +export interface PasskeyLoginResponse { + authenticated?: true; + must_change_password?: boolean; + unlocked?: boolean; +} + // Login response discriminated union export interface LoginSuccessResponse { authenticated: true;