Action remaining QA suggestions + performance optimizations
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) <noreply@anthropic.com>
This commit is contained in:
parent
0a8e163e47
commit
ed98924716
@ -536,18 +536,17 @@ async def auth_status(
|
|||||||
config = config_result.scalar_one_or_none()
|
config = config_result.scalar_one_or_none()
|
||||||
registration_open = config.allow_registration if config else False
|
registration_open = config.allow_registration if config else False
|
||||||
|
|
||||||
# Count passkeys for authenticated user — needed for passwordless toggle UX.
|
# Perf-3: Check passkey existence with EXISTS (not COUNT) — this endpoint
|
||||||
passkey_count = 0
|
# is polled every 15s. Count is derived from GET /auth/passkeys list instead.
|
||||||
has_passkeys = False
|
has_passkeys = False
|
||||||
passwordless_enabled = False
|
passwordless_enabled = False
|
||||||
if authenticated and u:
|
if authenticated and u:
|
||||||
pk_result = await db.execute(
|
pk_result = await db.execute(
|
||||||
select(func.count()).select_from(PasskeyCredential).where(
|
select(PasskeyCredential.id).where(
|
||||||
PasskeyCredential.user_id == u.id
|
PasskeyCredential.user_id == u.id
|
||||||
)
|
).limit(1)
|
||||||
)
|
)
|
||||||
passkey_count = pk_result.scalar_one()
|
has_passkeys = pk_result.scalar_one_or_none() is not None
|
||||||
has_passkeys = passkey_count > 0
|
|
||||||
passwordless_enabled = u.passwordless_enabled
|
passwordless_enabled = u.passwordless_enabled
|
||||||
|
|
||||||
return {
|
return {
|
||||||
@ -558,7 +557,6 @@ async def auth_status(
|
|||||||
"registration_open": registration_open,
|
"registration_open": registration_open,
|
||||||
"is_locked": is_locked,
|
"is_locked": is_locked,
|
||||||
"has_passkeys": has_passkeys,
|
"has_passkeys": has_passkeys,
|
||||||
"passkey_count": passkey_count,
|
|
||||||
"passwordless_enabled": passwordless_enabled,
|
"passwordless_enabled": passwordless_enabled,
|
||||||
"allow_passwordless": config.allow_passwordless if config else False,
|
"allow_passwordless": config.allow_passwordless if config else False,
|
||||||
}
|
}
|
||||||
|
|||||||
@ -50,6 +50,7 @@ from app.services.passkey import (
|
|||||||
verify_registration as verify_registration_response_svc,
|
verify_registration as verify_registration_response_svc,
|
||||||
build_authentication_options,
|
build_authentication_options,
|
||||||
verify_authentication as verify_authentication_response_svc,
|
verify_authentication as verify_authentication_response_svc,
|
||||||
|
extract_credential_raw_id,
|
||||||
)
|
)
|
||||||
from app.models.session import UserSession
|
from app.models.session import UserSession
|
||||||
from webauthn.helpers import bytes_to_base64url, base64url_to_bytes
|
from webauthn.helpers import bytes_to_base64url, base64url_to_bytes
|
||||||
@ -304,11 +305,9 @@ async def passkey_login_complete(
|
|||||||
if challenge is None:
|
if challenge is None:
|
||||||
raise HTTPException(status_code=401, detail="Authentication failed")
|
raise HTTPException(status_code=401, detail="Authentication failed")
|
||||||
|
|
||||||
# Parse credential_id from browser response
|
# Parse credential_id from browser response (S-02: shared helper)
|
||||||
try:
|
raw_id_b64 = extract_credential_raw_id(data.credential)
|
||||||
cred_data = json.loads(data.credential)
|
if not raw_id_b64:
|
||||||
raw_id_b64 = cred_data.get("rawId") or cred_data.get("id", "")
|
|
||||||
except (json.JSONDecodeError, KeyError):
|
|
||||||
raise HTTPException(status_code=401, detail="Authentication failed")
|
raise HTTPException(status_code=401, detail="Authentication failed")
|
||||||
|
|
||||||
# Look up credential + user in a single JOIN query (W-1 perf fix)
|
# 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:
|
if challenge is None:
|
||||||
raise HTTPException(status_code=401, detail="Invalid or expired challenge")
|
raise HTTPException(status_code=401, detail="Invalid or expired challenge")
|
||||||
|
|
||||||
# Parse rawId from credential to look up the stored credential
|
# Parse rawId from credential (S-02: shared helper)
|
||||||
try:
|
raw_id_b64 = extract_credential_raw_id(data.credential)
|
||||||
cred_data = json.loads(data.credential)
|
if not raw_id_b64:
|
||||||
raw_id_b64 = cred_data.get("rawId") or cred_data.get("id", "")
|
|
||||||
except (json.JSONDecodeError, KeyError):
|
|
||||||
raise HTTPException(status_code=401, detail="Authentication failed")
|
raise HTTPException(status_code=401, detail="Authentication failed")
|
||||||
|
|
||||||
# Look up credential — verify ownership (IDOR prevention)
|
# Look up credential — verify ownership (IDOR prevention)
|
||||||
|
|||||||
@ -37,6 +37,22 @@ from webauthn.helpers import (
|
|||||||
|
|
||||||
from app.config import settings as app_settings
|
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__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
@ -11,7 +11,7 @@ from datetime import datetime, timedelta
|
|||||||
|
|
||||||
from fastapi import HTTPException, Response
|
from fastapi import HTTPException, Response
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select, update
|
||||||
|
|
||||||
from app.models.user import User
|
from app.models.user import User
|
||||||
from app.models.session import UserSession
|
from app.models.session import UserSession
|
||||||
@ -93,10 +93,12 @@ async def create_db_session(
|
|||||||
db.add(db_session)
|
db.add(db_session)
|
||||||
await db.flush()
|
await db.flush()
|
||||||
|
|
||||||
# Enforce concurrent session limit: revoke oldest sessions beyond the cap
|
# Enforce concurrent session limit: revoke oldest sessions beyond the cap.
|
||||||
active_sessions = (
|
# 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(
|
await db.execute(
|
||||||
select(UserSession)
|
select(UserSession.id)
|
||||||
.where(
|
.where(
|
||||||
UserSession.user_id == user.id,
|
UserSession.user_id == user.id,
|
||||||
UserSession.revoked == False, # noqa: E712
|
UserSession.revoked == False, # noqa: E712
|
||||||
@ -106,10 +108,13 @@ async def create_db_session(
|
|||||||
)
|
)
|
||||||
).scalars().all()
|
).scalars().all()
|
||||||
|
|
||||||
max_sessions = app_settings.MAX_SESSIONS_PER_USER
|
if len(active_ids) > max_sessions:
|
||||||
if len(active_sessions) > max_sessions:
|
ids_to_revoke = active_ids[: len(active_ids) - max_sessions]
|
||||||
for old_session in active_sessions[: len(active_sessions) - max_sessions]:
|
await db.execute(
|
||||||
old_session.revoked = True
|
update(UserSession)
|
||||||
|
.where(UserSession.id.in_(ids_to_revoke))
|
||||||
|
.values(revoked=True)
|
||||||
|
)
|
||||||
await db.flush()
|
await db.flush()
|
||||||
|
|
||||||
token = create_session_token(user.id, session_id)
|
token = create_session_token(user.id, session_id)
|
||||||
|
|||||||
@ -137,7 +137,7 @@ function PasskeyDeleteButton({ credential, onDelete, isDeleting }: DeleteConfirm
|
|||||||
|
|
||||||
export default function PasskeySection() {
|
export default function PasskeySection() {
|
||||||
const queryClient = useQueryClient();
|
const queryClient = useQueryClient();
|
||||||
const { passwordlessEnabled, passkeyCount, allowPasswordless } = useAuth();
|
const { passwordlessEnabled, allowPasswordless } = useAuth();
|
||||||
|
|
||||||
// Registration state
|
// Registration state
|
||||||
const [registerDialogOpen, setRegisterDialogOpen] = useState(false);
|
const [registerDialogOpen, setRegisterDialogOpen] = useState(false);
|
||||||
@ -276,6 +276,8 @@ export default function PasskeySection() {
|
|||||||
toast.error(getErrorMessage(error, 'Failed to disable passwordless login'));
|
toast.error(getErrorMessage(error, 'Failed to disable passwordless login'));
|
||||||
}
|
}
|
||||||
setDisableDialogOpen(false);
|
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() {
|
|||||||
<p className="text-xs text-muted-foreground">
|
<p className="text-xs text-muted-foreground">
|
||||||
Skip the password prompt and unlock the app using a passkey only.
|
Skip the password prompt and unlock the app using a passkey only.
|
||||||
</p>
|
</p>
|
||||||
{passkeyCount < 2 && !passwordlessEnabled && (
|
{passkeys.length < 2 && !passwordlessEnabled && (
|
||||||
<p className="text-xs text-amber-400">
|
<p className="text-xs text-amber-400">
|
||||||
Requires at least 2 registered passkeys as a fallback.
|
Requires at least 2 registered passkeys as a fallback.
|
||||||
</p>
|
</p>
|
||||||
@ -406,7 +408,7 @@ export default function PasskeySection() {
|
|||||||
disablePasswordlessMutation.mutate();
|
disablePasswordlessMutation.mutate();
|
||||||
}
|
}
|
||||||
}}
|
}}
|
||||||
disabled={(!passwordlessEnabled && passkeyCount < 2) || enablePasswordlessMutation.isPending || disablePasswordlessMutation.isPending}
|
disabled={(!passwordlessEnabled && passkeys.length < 2) || enablePasswordlessMutation.isPending || disablePasswordlessMutation.isPending}
|
||||||
aria-label="Toggle passwordless login"
|
aria-label="Toggle passwordless login"
|
||||||
/>
|
/>
|
||||||
</div>
|
</div>
|
||||||
@ -476,11 +478,28 @@ export default function PasskeySection() {
|
|||||||
</DialogDescription>
|
</DialogDescription>
|
||||||
</DialogHeader>
|
</DialogHeader>
|
||||||
<div className="flex flex-col items-center gap-4 py-4">
|
<div className="flex flex-col items-center gap-4 py-4">
|
||||||
<Loader2 className="h-8 w-8 animate-spin text-accent" />
|
{disablePasswordlessMutation.isPending ? (
|
||||||
<p className="text-sm text-muted-foreground text-center">
|
<>
|
||||||
Follow your browser's prompt to verify your passkey
|
<Loader2 className="h-8 w-8 animate-spin text-accent" />
|
||||||
</p>
|
<p className="text-sm text-muted-foreground text-center">
|
||||||
|
Follow your browser's prompt to verify your passkey
|
||||||
|
</p>
|
||||||
|
</>
|
||||||
|
) : (
|
||||||
|
<p className="text-sm text-muted-foreground text-center">
|
||||||
|
Ready to verify your passkey
|
||||||
|
</p>
|
||||||
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
<DialogFooter>
|
||||||
|
<Button
|
||||||
|
variant="outline"
|
||||||
|
onClick={() => setDisableDialogOpen(false)}
|
||||||
|
disabled={disablePasswordlessMutation.isPending}
|
||||||
|
>
|
||||||
|
Cancel
|
||||||
|
</Button>
|
||||||
|
</DialogFooter>
|
||||||
</DialogContent>
|
</DialogContent>
|
||||||
</Dialog>
|
</Dialog>
|
||||||
|
|
||||||
|
|||||||
@ -1,7 +1,7 @@
|
|||||||
import { useState } from 'react';
|
import { useState } from 'react';
|
||||||
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query';
|
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query';
|
||||||
import api from '@/lib/api';
|
import api from '@/lib/api';
|
||||||
import type { AuthStatus, LoginResponse } from '@/types';
|
import type { AuthStatus, LoginResponse, PasskeyLoginResponse } from '@/types';
|
||||||
|
|
||||||
export function useAuth() {
|
export function useAuth() {
|
||||||
const queryClient = useQueryClient();
|
const queryClient = useQueryClient();
|
||||||
@ -101,7 +101,7 @@ export function useAuth() {
|
|||||||
const { startAuthentication } = await import('@simplewebauthn/browser');
|
const { startAuthentication } = await import('@simplewebauthn/browser');
|
||||||
const { data: beginResp } = await api.post('/auth/passkeys/login/begin', {});
|
const { data: beginResp } = await api.post('/auth/passkeys/login/begin', {});
|
||||||
const credential = await startAuthentication(beginResp.options);
|
const credential = await startAuthentication(beginResp.options);
|
||||||
const { data } = await api.post('/auth/passkeys/login/complete', {
|
const { data } = await api.post<PasskeyLoginResponse>('/auth/passkeys/login/complete', {
|
||||||
credential: JSON.stringify(credential),
|
credential: JSON.stringify(credential),
|
||||||
challenge_token: beginResp.challenge_token,
|
challenge_token: beginResp.challenge_token,
|
||||||
});
|
});
|
||||||
@ -152,7 +152,7 @@ export function useAuth() {
|
|||||||
passkeyLogin: passkeyLoginMutation.mutateAsync,
|
passkeyLogin: passkeyLoginMutation.mutateAsync,
|
||||||
isPasskeyLoginPending: passkeyLoginMutation.isPending,
|
isPasskeyLoginPending: passkeyLoginMutation.isPending,
|
||||||
hasPasskeys: authQuery.data?.has_passkeys ?? false,
|
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,
|
passwordlessEnabled: authQuery.data?.passwordless_enabled ?? false,
|
||||||
allowPasswordless: authQuery.data?.allow_passwordless ?? false,
|
allowPasswordless: authQuery.data?.allow_passwordless ?? false,
|
||||||
};
|
};
|
||||||
|
|||||||
@ -244,7 +244,6 @@ export interface AuthStatus {
|
|||||||
registration_open: boolean;
|
registration_open: boolean;
|
||||||
is_locked: boolean;
|
is_locked: boolean;
|
||||||
has_passkeys: boolean;
|
has_passkeys: boolean;
|
||||||
passkey_count: number;
|
|
||||||
passwordless_enabled: boolean;
|
passwordless_enabled: boolean;
|
||||||
allow_passwordless: boolean;
|
allow_passwordless: boolean;
|
||||||
}
|
}
|
||||||
@ -257,6 +256,12 @@ export interface PasskeyCredential {
|
|||||||
backed_up: boolean;
|
backed_up: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export interface PasskeyLoginResponse {
|
||||||
|
authenticated?: true;
|
||||||
|
must_change_password?: boolean;
|
||||||
|
unlocked?: boolean;
|
||||||
|
}
|
||||||
|
|
||||||
// Login response discriminated union
|
// Login response discriminated union
|
||||||
export interface LoginSuccessResponse {
|
export interface LoginSuccessResponse {
|
||||||
authenticated: true;
|
authenticated: true;
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user