Fix review findings: transaction atomicity, perf, and UI polish
Backend fixes: - session.py: record_failed/successful_login use flush() not commit() — callers own transaction boundary (BUG-2 atomicity fix) - auth.py: Add explicit commits after record_failed_login where callers raise immediately; add commit before TOTP mfa_token return path - passkeys.py: JOIN credential+user lookup in login/complete (W-1 perf) - passkeys.py: Move mfa_enforce_pending clear before main commit (S-2) - passkeys.py: Add Path(ge=1, le=2147483647) on DELETE endpoint (BUG-3) - auth.py: Switch has_passkeys from COUNT to EXISTS with LIMIT 1 (W-2) - passkey.py: Add single-worker nonce cache comment (H-1) Frontend fixes: - PasskeySection: emerald→green badge colors (W-3 palette) - PasskeySection: text-[11px]/text-[10px]→text-xs (W-4 a11y minimum) - PasskeySection: Scope deleteMutation.isPending to per-item (W-5) - nginx.conf: Permissions-Policy publickey-credentials use (self) (H-2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
51d98173a6
commit
ab84c7bc53
@ -324,6 +324,7 @@ async def login(
|
|||||||
# If TOTP is enabled, issue a short-lived MFA challenge token
|
# If TOTP is enabled, issue a short-lived MFA challenge token
|
||||||
if user.totp_enabled:
|
if user.totp_enabled:
|
||||||
mfa_token = create_mfa_token(user.id)
|
mfa_token = create_mfa_token(user.id)
|
||||||
|
await db.commit()
|
||||||
return {
|
return {
|
||||||
"authenticated": False,
|
"authenticated": False,
|
||||||
"totp_required": True,
|
"totp_required": True,
|
||||||
@ -514,15 +515,15 @@ 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
|
||||||
|
|
||||||
# Check if authenticated user has passkeys registered (Q-04)
|
# Check if authenticated user has passkeys registered (Q-04, W-2: EXISTS not COUNT)
|
||||||
has_passkeys = False
|
has_passkeys = 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)
|
||||||
)
|
)
|
||||||
)
|
has_passkeys = pk_result.scalar_one_or_none() is not None
|
||||||
has_passkeys = pk_result.scalar_one() > 0
|
|
||||||
|
|
||||||
return {
|
return {
|
||||||
"authenticated": authenticated,
|
"authenticated": authenticated,
|
||||||
@ -565,6 +566,7 @@ async def verify_password_endpoint(
|
|||||||
valid, new_hash = await averify_password_with_upgrade(data.password, current_user.password_hash)
|
valid, new_hash = await averify_password_with_upgrade(data.password, current_user.password_hash)
|
||||||
if not valid:
|
if not valid:
|
||||||
await record_failed_login(db, current_user)
|
await record_failed_login(db, current_user)
|
||||||
|
await db.commit()
|
||||||
raise HTTPException(status_code=401, detail="Invalid password")
|
raise HTTPException(status_code=401, detail="Invalid password")
|
||||||
|
|
||||||
if new_hash:
|
if new_hash:
|
||||||
@ -591,6 +593,7 @@ async def change_password(
|
|||||||
valid, _ = await averify_password_with_upgrade(data.old_password, current_user.password_hash)
|
valid, _ = await averify_password_with_upgrade(data.old_password, current_user.password_hash)
|
||||||
if not valid:
|
if not valid:
|
||||||
await record_failed_login(db, current_user)
|
await record_failed_login(db, current_user)
|
||||||
|
await db.commit()
|
||||||
raise HTTPException(status_code=401, detail="Invalid current password")
|
raise HTTPException(status_code=401, detail="Invalid current password")
|
||||||
|
|
||||||
if data.new_password == data.old_password:
|
if data.new_password == data.old_password:
|
||||||
|
|||||||
@ -24,7 +24,7 @@ import json
|
|||||||
import logging
|
import logging
|
||||||
from datetime import datetime
|
from datetime import datetime
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends, HTTPException, Request, Response
|
from fastapi import APIRouter, Depends, HTTPException, Path, Request, Response
|
||||||
from pydantic import BaseModel, ConfigDict, Field
|
from pydantic import BaseModel, ConfigDict, Field
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
@ -287,23 +287,16 @@ async def passkey_login_complete(
|
|||||||
except (json.JSONDecodeError, KeyError):
|
except (json.JSONDecodeError, KeyError):
|
||||||
raise HTTPException(status_code=401, detail="Authentication failed")
|
raise HTTPException(status_code=401, detail="Authentication failed")
|
||||||
|
|
||||||
# Look up credential by ID
|
# Look up credential + user in a single JOIN query (W-1 perf fix)
|
||||||
result = await db.execute(
|
result = await db.execute(
|
||||||
select(PasskeyCredential).where(
|
select(PasskeyCredential, User)
|
||||||
PasskeyCredential.credential_id == raw_id_b64
|
.join(User, User.id == PasskeyCredential.user_id)
|
||||||
|
.where(PasskeyCredential.credential_id == raw_id_b64)
|
||||||
)
|
)
|
||||||
)
|
row = result.one_or_none()
|
||||||
credential = result.scalar_one_or_none()
|
if not row:
|
||||||
if not credential:
|
|
||||||
raise HTTPException(status_code=401, detail="Authentication failed")
|
|
||||||
|
|
||||||
# Load user
|
|
||||||
user_result = await db.execute(
|
|
||||||
select(User).where(User.id == credential.user_id)
|
|
||||||
)
|
|
||||||
user = user_result.scalar_one_or_none()
|
|
||||||
if not user:
|
|
||||||
raise HTTPException(status_code=401, detail="Authentication failed")
|
raise HTTPException(status_code=401, detail="Authentication failed")
|
||||||
|
credential, user = row.tuple()
|
||||||
|
|
||||||
# Check account lockout (C-03)
|
# Check account lockout (C-03)
|
||||||
await check_account_lockout(user)
|
await check_account_lockout(user)
|
||||||
@ -361,6 +354,14 @@ async def passkey_login_complete(
|
|||||||
_, token = await create_db_session(db, user, client_ip, user_agent)
|
_, token = await create_db_session(db, user, client_ip, user_agent)
|
||||||
set_session_cookie(response, token)
|
set_session_cookie(response, token)
|
||||||
|
|
||||||
|
# B-01: Handle must_change_password and mfa_enforce_pending flags
|
||||||
|
result_data: dict = {"authenticated": True}
|
||||||
|
if user.must_change_password:
|
||||||
|
result_data["must_change_password"] = True
|
||||||
|
# Passkey satisfies MFA — if mfa_enforce_pending, clear it (before commit)
|
||||||
|
if user.mfa_enforce_pending:
|
||||||
|
user.mfa_enforce_pending = False
|
||||||
|
|
||||||
await log_audit_event(
|
await log_audit_event(
|
||||||
db, action="passkey.login_success", actor_id=user.id,
|
db, action="passkey.login_success", actor_id=user.id,
|
||||||
detail={"credential_name": credential.name},
|
detail={"credential_name": credential.name},
|
||||||
@ -368,15 +369,6 @@ async def passkey_login_complete(
|
|||||||
)
|
)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
|
|
||||||
# B-01: Handle must_change_password and mfa_enforce_pending flags
|
|
||||||
result_data: dict = {"authenticated": True}
|
|
||||||
if user.must_change_password:
|
|
||||||
result_data["must_change_password"] = True
|
|
||||||
# Passkey satisfies MFA — if mfa_enforce_pending, clear it
|
|
||||||
if user.mfa_enforce_pending:
|
|
||||||
user.mfa_enforce_pending = False
|
|
||||||
await db.commit()
|
|
||||||
|
|
||||||
return result_data
|
return result_data
|
||||||
|
|
||||||
|
|
||||||
@ -411,8 +403,8 @@ async def list_passkeys(
|
|||||||
|
|
||||||
@router.delete("/{credential_id}")
|
@router.delete("/{credential_id}")
|
||||||
async def delete_passkey(
|
async def delete_passkey(
|
||||||
credential_id: int,
|
credential_id: int = Path(ge=1, le=2147483647),
|
||||||
data: PasskeyDeleteRequest,
|
data: PasskeyDeleteRequest = ...,
|
||||||
request: Request,
|
request: Request,
|
||||||
db: AsyncSession = Depends(get_db),
|
db: AsyncSession = Depends(get_db),
|
||||||
current_user: User = Depends(get_current_user),
|
current_user: User = Depends(get_current_user),
|
||||||
|
|||||||
@ -43,8 +43,11 @@ _challenge_serializer = URLSafeTimedSerializer(
|
|||||||
salt="webauthn-challenge-v1",
|
salt="webauthn-challenge-v1",
|
||||||
)
|
)
|
||||||
|
|
||||||
# Thread-safe nonce cache for single-use enforcement
|
# Thread-safe nonce cache for single-use enforcement.
|
||||||
# Keys: nonce string, Values: expiry timestamp
|
# Keys: nonce string, Values: expiry timestamp.
|
||||||
|
# NOTE: This is process-local. If scaling to multiple uvicorn workers,
|
||||||
|
# move nonce tracking to Redis or a DB table with unique constraint.
|
||||||
|
# Current deployment: single worker (Dockerfile --workers 1).
|
||||||
_used_nonces: dict[str, float] = {}
|
_used_nonces: dict[str, float] = {}
|
||||||
_nonce_lock = threading.Lock()
|
_nonce_lock = threading.Lock()
|
||||||
|
|
||||||
|
|||||||
@ -43,19 +43,25 @@ async def check_account_lockout(user: User) -> None:
|
|||||||
|
|
||||||
|
|
||||||
async def record_failed_login(db: AsyncSession, user: User) -> None:
|
async def record_failed_login(db: AsyncSession, user: User) -> None:
|
||||||
"""Increment failure counter; lock account after 10 failures."""
|
"""Increment failure counter; lock account after 10 failures.
|
||||||
|
|
||||||
|
Does NOT commit — caller owns the transaction boundary.
|
||||||
|
"""
|
||||||
user.failed_login_count += 1
|
user.failed_login_count += 1
|
||||||
if user.failed_login_count >= 10:
|
if user.failed_login_count >= 10:
|
||||||
user.locked_until = datetime.now() + timedelta(minutes=30)
|
user.locked_until = datetime.now() + timedelta(minutes=30)
|
||||||
await db.commit()
|
await db.flush()
|
||||||
|
|
||||||
|
|
||||||
async def record_successful_login(db: AsyncSession, user: User) -> None:
|
async def record_successful_login(db: AsyncSession, user: User) -> None:
|
||||||
"""Reset failure counter and update last_login_at."""
|
"""Reset failure counter and update last_login_at.
|
||||||
|
|
||||||
|
Does NOT commit — caller owns the transaction boundary.
|
||||||
|
"""
|
||||||
user.failed_login_count = 0
|
user.failed_login_count = 0
|
||||||
user.locked_until = None
|
user.locked_until = None
|
||||||
user.last_login_at = datetime.now()
|
user.last_login_at = datetime.now()
|
||||||
await db.commit()
|
await db.flush()
|
||||||
|
|
||||||
|
|
||||||
async def create_db_session(
|
async def create_db_session(
|
||||||
|
|||||||
@ -187,5 +187,5 @@ server {
|
|||||||
add_header Content-Security-Policy "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; img-src 'self' data:; font-src 'self' https://fonts.gstatic.com; connect-src 'self';" always;
|
add_header Content-Security-Policy "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; img-src 'self' data:; font-src 'self' https://fonts.gstatic.com; connect-src 'self';" always;
|
||||||
add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always;
|
add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always;
|
||||||
# PT-I03: Restrict unnecessary browser APIs
|
# PT-I03: Restrict unnecessary browser APIs
|
||||||
add_header Permissions-Policy "camera=(), microphone=(), geolocation=(), interest-cohort=(), publickey-credentials-get=*, publickey-credentials-create=*" always;
|
add_header Permissions-Policy "camera=(), microphone=(), geolocation=(), interest-cohort=(), publickey-credentials-get=(self), publickey-credentials-create=(self)" always;
|
||||||
}
|
}
|
||||||
|
|||||||
@ -90,7 +90,7 @@ function PasskeyDeleteButton({ credential, onDelete, isDeleting }: DeleteConfirm
|
|||||||
aria-label={`Remove passkey ${credential.name}`}
|
aria-label={`Remove passkey ${credential.name}`}
|
||||||
>
|
>
|
||||||
{confirming ? (
|
{confirming ? (
|
||||||
<span className="text-[10px] font-medium text-red-400">Sure?</span>
|
<span className="text-xs font-medium text-red-400">Sure?</span>
|
||||||
) : (
|
) : (
|
||||||
<Trash2 className="h-3.5 w-3.5" />
|
<Trash2 className="h-3.5 w-3.5" />
|
||||||
)}
|
)}
|
||||||
@ -269,7 +269,7 @@ export default function PasskeySection() {
|
|||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
{hasPasskeys && (
|
{hasPasskeys && (
|
||||||
<span className="inline-flex items-center rounded-full bg-emerald-500/10 px-2.5 py-0.5 text-xs font-semibold text-emerald-400">
|
<span className="inline-flex items-center rounded-full bg-green-500/10 px-2.5 py-0.5 text-xs font-semibold text-green-400">
|
||||||
{passkeys.length} registered
|
{passkeys.length} registered
|
||||||
</span>
|
</span>
|
||||||
)}
|
)}
|
||||||
@ -292,14 +292,14 @@ export default function PasskeySection() {
|
|||||||
<Cloud className="h-3 w-3 text-muted-foreground shrink-0" aria-label="Synced" />
|
<Cloud className="h-3 w-3 text-muted-foreground shrink-0" aria-label="Synced" />
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
<span className="text-[11px] text-muted-foreground">
|
<span className="text-xs text-muted-foreground">
|
||||||
Added {formatDate(pk.created_at)} · Last used {formatRelativeTime(pk.last_used_at)}
|
Added {formatDate(pk.created_at)} · Last used {formatRelativeTime(pk.last_used_at)}
|
||||||
</span>
|
</span>
|
||||||
</div>
|
</div>
|
||||||
<PasskeyDeleteButton
|
<PasskeyDeleteButton
|
||||||
credential={pk}
|
credential={pk}
|
||||||
onDelete={handleDelete}
|
onDelete={handleDelete}
|
||||||
isDeleting={deleteMutation.isPending}
|
isDeleting={deleteMutation.isPending && (deleteMutation.variables as { id: number } | undefined)?.id === pk.id}
|
||||||
/>
|
/>
|
||||||
</li>
|
</li>
|
||||||
))}
|
))}
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user