From e57a5b00c9480e1cec5612dd678e242a2b999629 Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Thu, 26 Feb 2026 19:19:04 +0800 Subject: [PATCH] Fix QA review findings: C-01 through C-04, W-01 through W-07, S-01/S-04/S-05/S-06 Critical fixes: - C-01: Pass user_id to _mark_sent/_already_sent (ntfy crash) - C-02: Align frontend HTTP methods with backend routes (PATCH->PUT, DELETE->POST, fix reset-password/enforce-mfa/disable-mfa paths) - C-03: Add X-Requested-With to CORS allow_headers - C-04: Replace scalar_one_or_none with func.count for auth/status Warning fixes: - W-01: Batch audit log into same transaction in create_user, setup, register - W-02: Extract users array from UserListResponse wrapper in useAdminUsers - W-03: Update password hint from "8 chars" to "12 chars" in CreateUserDialog - W-04: Remove password input from reset flow, show returned temp password - W-06: Remove unused actor_alias variable in admin_dashboard - W-07: Resolve usernames in dashboard audit entries via JOIN, remove ip_address column from recent_logins (not tracked on User model) Suggestions applied: - S-01/S-06: Add extra="forbid" to all admin mutation schemas - S-04: Add ondelete="SET NULL" to audit_log.actor_user_id FK - S-05: Improve registration error message for better UX Co-Authored-By: Claude Opus 4.6 --- .../026_add_user_role_and_system_config.py | 2 +- backend/app/jobs/notifications.py | 27 ++++---- backend/app/main.py | 2 +- backend/app/models/audit_log.py | 2 +- backend/app/routers/admin.py | 30 +++++---- backend/app/routers/auth.py | 13 ++-- backend/app/schemas/admin.py | 4 ++ .../components/admin/AdminDashboardPage.tsx | 6 -- .../src/components/admin/CreateUserDialog.tsx | 4 +- .../src/components/admin/UserActionsMenu.tsx | 67 ++++++++----------- frontend/src/hooks/useAdmin.ts | 40 +++++------ frontend/src/types/index.ts | 1 - 12 files changed, 96 insertions(+), 102 deletions(-) diff --git a/backend/alembic/versions/026_add_user_role_and_system_config.py b/backend/alembic/versions/026_add_user_role_and_system_config.py index 9bb863a..63b11f4 100644 --- a/backend/alembic/versions/026_add_user_role_and_system_config.py +++ b/backend/alembic/versions/026_add_user_role_and_system_config.py @@ -76,7 +76,7 @@ def upgrade() -> None: sa.Column("ip_address", sa.String(45), nullable=True), sa.Column("created_at", sa.DateTime(), nullable=False, server_default=sa.text("NOW()")), sa.PrimaryKeyConstraint("id"), - sa.ForeignKeyConstraint(["actor_user_id"], ["users.id"]), + sa.ForeignKeyConstraint(["actor_user_id"], ["users.id"], ondelete="SET NULL"), sa.ForeignKeyConstraint( ["target_user_id"], ["users.id"], ondelete="SET NULL" ), diff --git a/backend/app/jobs/notifications.py b/backend/app/jobs/notifications.py index bcd0961..595191a 100644 --- a/backend/app/jobs/notifications.py +++ b/backend/app/jobs/notifications.py @@ -40,15 +40,18 @@ UMBRA_URL = "http://10.0.69.35" # ── Dedup helpers ───────────────────────────────────────────────────────────── -async def _already_sent(db: AsyncSession, key: str) -> bool: +async def _already_sent(db: AsyncSession, key: str, user_id: int) -> bool: result = await db.execute( - select(NtfySent).where(NtfySent.notification_key == key) + select(NtfySent).where( + NtfySent.user_id == user_id, + NtfySent.notification_key == key, + ) ) return result.scalar_one_or_none() is not None -async def _mark_sent(db: AsyncSession, key: str) -> None: - db.add(NtfySent(notification_key=key)) +async def _mark_sent(db: AsyncSession, key: str, user_id: int) -> None: + db.add(NtfySent(notification_key=key, user_id=user_id)) await db.commit() @@ -76,7 +79,7 @@ async def _dispatch_reminders(db: AsyncSession, settings: Settings, now: datetim # Key includes user_id to prevent cross-user dedup collisions key = f"reminder:{settings.user_id}:{reminder.id}:{reminder.remind_at.date()}" - if await _already_sent(db, key): + if await _already_sent(db, key, settings.user_id): continue payload = build_reminder_notification( @@ -91,7 +94,7 @@ async def _dispatch_reminders(db: AsyncSession, settings: Settings, now: datetim **payload, ) if sent: - await _mark_sent(db, key) + await _mark_sent(db, key, settings.user_id) async def _dispatch_events(db: AsyncSession, settings: Settings, now: datetime) -> None: @@ -124,7 +127,7 @@ async def _dispatch_events(db: AsyncSession, settings: Settings, now: datetime) for event in events: # Key includes user_id to prevent cross-user dedup collisions key = f"event:{settings.user_id}:{event.id}:{event.start_datetime.strftime('%Y-%m-%dT%H:%M')}" - if await _already_sent(db, key): + if await _already_sent(db, key, settings.user_id): continue payload = build_event_notification( @@ -142,7 +145,7 @@ async def _dispatch_events(db: AsyncSession, settings: Settings, now: datetime) **payload, ) if sent: - await _mark_sent(db, key) + await _mark_sent(db, key, settings.user_id) async def _dispatch_todos(db: AsyncSession, settings: Settings, today) -> None: @@ -165,7 +168,7 @@ async def _dispatch_todos(db: AsyncSession, settings: Settings, today) -> None: for todo in todos: # Key includes user_id to prevent cross-user dedup collisions key = f"todo:{settings.user_id}:{todo.id}:{today}" - if await _already_sent(db, key): + if await _already_sent(db, key, settings.user_id): continue payload = build_todo_notification( @@ -181,7 +184,7 @@ async def _dispatch_todos(db: AsyncSession, settings: Settings, today) -> None: **payload, ) if sent: - await _mark_sent(db, key) + await _mark_sent(db, key, settings.user_id) async def _dispatch_projects(db: AsyncSession, settings: Settings, today) -> None: @@ -204,7 +207,7 @@ async def _dispatch_projects(db: AsyncSession, settings: Settings, today) -> Non for project in projects: # Key includes user_id to prevent cross-user dedup collisions key = f"project:{settings.user_id}:{project.id}:{today}" - if await _already_sent(db, key): + if await _already_sent(db, key, settings.user_id): continue payload = build_project_notification( @@ -219,7 +222,7 @@ async def _dispatch_projects(db: AsyncSession, settings: Settings, today) -> Non **payload, ) if sent: - await _mark_sent(db, key) + await _mark_sent(db, key, settings.user_id) async def _dispatch_for_user(db: AsyncSession, settings: Settings, now: datetime) -> None: diff --git a/backend/app/main.py b/backend/app/main.py index 624a61f..c546d5b 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -53,7 +53,7 @@ app.add_middleware( allow_origins=[o.strip() for o in settings.CORS_ORIGINS.split(",") if o.strip()], allow_credentials=True, allow_methods=["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"], - allow_headers=["Content-Type", "Authorization", "Cookie"], + allow_headers=["Content-Type", "Authorization", "Cookie", "X-Requested-With"], ) # Include routers with /api prefix diff --git a/backend/app/models/audit_log.py b/backend/app/models/audit_log.py index a16f8a6..1ebef1d 100644 --- a/backend/app/models/audit_log.py +++ b/backend/app/models/audit_log.py @@ -14,7 +14,7 @@ class AuditLog(Base): id: Mapped[int] = mapped_column(primary_key=True) actor_user_id: Mapped[Optional[int]] = mapped_column( - Integer, ForeignKey("users.id"), nullable=True, index=True + Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True, index=True ) target_user_id: Mapped[Optional[int]] = mapped_column( Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True, index=True diff --git a/backend/app/routers/admin.py b/backend/app/routers/admin.py index 7c80b1a..1d1228d 100644 --- a/backend/app/routers/admin.py +++ b/backend/app/routers/admin.py @@ -176,7 +176,6 @@ async def create_user( await db.flush() # populate new_user.id await _create_user_defaults(db, new_user.id) - await db.commit() await log_audit_event( db, @@ -582,8 +581,7 @@ async def admin_dashboard( mfa_adoption = (totp_count / total_users) if total_users else 0.0 - # 10 most recent logins — join to get username - actor_alias = sa.alias(User.__table__, name="actor") + # 10 most recent logins recent_logins_result = await db.execute( sa.select(User.username, User.last_login_at) .where(User.last_login_at != None) @@ -595,20 +593,28 @@ async def admin_dashboard( for row in recent_logins_result ] - # 10 most recent audit entries + # 10 most recent audit entries — resolve usernames via JOINs + actor_user = sa.orm.aliased(User, name="actor_user") + target_user = sa.orm.aliased(User, name="target_user") recent_audit_result = await db.execute( - sa.select(AuditLog).order_by(AuditLog.created_at.desc()).limit(10) + sa.select( + AuditLog, + actor_user.username.label("actor_username"), + target_user.username.label("target_username"), + ) + .outerjoin(actor_user, AuditLog.actor_user_id == actor_user.id) + .outerjoin(target_user, AuditLog.target_user_id == target_user.id) + .order_by(AuditLog.created_at.desc()) + .limit(10) ) recent_audit_entries = [ { - "id": e.id, - "action": e.action, - "actor_user_id": e.actor_user_id, - "target_user_id": e.target_user_id, - "detail": e.detail, - "created_at": e.created_at, + "action": row.AuditLog.action, + "actor_username": row.actor_username, + "target_username": row.target_username, + "created_at": row.AuditLog.created_at, } - for e in recent_audit_result.scalars() + for row in recent_audit_result ] return AdminDashboardResponse( diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index 5dbeac1..eda49c0 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -22,7 +22,7 @@ from typing import Optional from fastapi import APIRouter, Depends, HTTPException, Request, Response, Cookie from sqlalchemy.ext.asyncio import AsyncSession -from sqlalchemy import select +from sqlalchemy import select, func from app.database import get_db from app.models.user import User @@ -249,7 +249,6 @@ async def setup( await db.flush() await _create_user_defaults(db, new_user.id) - await db.commit() ip = request.client.host if request.client else "unknown" user_agent = request.headers.get("user-agent") @@ -376,7 +375,7 @@ async def register( select(User).where(User.username == data.username) ) if existing.scalar_one_or_none(): - raise HTTPException(status_code=400, detail="Registration failed") + raise HTTPException(status_code=400, detail="Registration could not be completed. Please try a different username.") password_hash = hash_password(data.password) # SEC-01: Explicit field assignment — never **data.model_dump() @@ -395,7 +394,6 @@ async def register( await db.flush() await _create_user_defaults(db, new_user.id) - await db.commit() ip = request.client.host if request.client else "unknown" user_agent = request.headers.get("user-agent") @@ -458,9 +456,10 @@ async def auth_status( """ Check authentication status, role, and whether initial setup/registration is available. """ - user_result = await db.execute(select(User)) - existing_user = user_result.scalar_one_or_none() - setup_required = existing_user is None + user_count_result = await db.execute( + select(func.count()).select_from(User) + ) + setup_required = user_count_result.scalar_one() == 0 authenticated = False role = None diff --git a/backend/app/schemas/admin.py b/backend/app/schemas/admin.py index 075a916..958a26c 100644 --- a/backend/app/schemas/admin.py +++ b/backend/app/schemas/admin.py @@ -64,14 +64,17 @@ class CreateUserRequest(BaseModel): class UpdateUserRoleRequest(BaseModel): + model_config = ConfigDict(extra="forbid") role: Literal["admin", "standard", "public_event_manager"] class ToggleActiveRequest(BaseModel): + model_config = ConfigDict(extra="forbid") is_active: bool class ToggleMfaEnforceRequest(BaseModel): + model_config = ConfigDict(extra="forbid") enforce: bool @@ -87,6 +90,7 @@ class SystemConfigResponse(BaseModel): class SystemConfigUpdate(BaseModel): + model_config = ConfigDict(extra="forbid") allow_registration: Optional[bool] = None enforce_mfa_new_users: Optional[bool] = None diff --git a/frontend/src/components/admin/AdminDashboardPage.tsx b/frontend/src/components/admin/AdminDashboardPage.tsx index 33d95c4..7881759 100644 --- a/frontend/src/components/admin/AdminDashboardPage.tsx +++ b/frontend/src/components/admin/AdminDashboardPage.tsx @@ -135,9 +135,6 @@ export default function AdminDashboardPage() { When - - IP - @@ -153,9 +150,6 @@ export default function AdminDashboardPage() { {getRelativeTime(entry.last_login_at)} - - {entry.ip_address ?? '—'} - ))} diff --git a/frontend/src/components/admin/CreateUserDialog.tsx b/frontend/src/components/admin/CreateUserDialog.tsx index a44164e..e4e8023 100644 --- a/frontend/src/components/admin/CreateUserDialog.tsx +++ b/frontend/src/components/admin/CreateUserDialog.tsx @@ -75,11 +75,11 @@ export default function CreateUserDialog({ open, onOpenChange }: CreateUserDialo type="password" value={password} onChange={(e) => setPassword(e.target.value)} - placeholder="Min. 8 characters" + placeholder="Min. 12 characters" required />

- Must be at least 8 characters. The user will be prompted to change it on first login. + Min. 12 characters with at least one letter and one non-letter. User must change on first login.

diff --git a/frontend/src/components/admin/UserActionsMenu.tsx b/frontend/src/components/admin/UserActionsMenu.tsx index 3163b2f..d6b4a7b 100644 --- a/frontend/src/components/admin/UserActionsMenu.tsx +++ b/frontend/src/components/admin/UserActionsMenu.tsx @@ -41,8 +41,7 @@ const ROLES: { value: UserRole; label: string }[] = [ export default function UserActionsMenu({ user }: UserActionsMenuProps) { const [open, setOpen] = useState(false); const [roleSubmenuOpen, setRoleSubmenuOpen] = useState(false); - const [showResetPassword, setShowResetPassword] = useState(false); - const [newPassword, setNewPassword] = useState(''); + const [tempPassword, setTempPassword] = useState(null); const menuRef = useRef(null); const updateRole = useUpdateRole(); @@ -162,50 +161,38 @@ export default function UserActionsMenu({ user }: UserActionsMenuProps) { {/* Reset Password */} - {!showResetPassword ? ( + {tempPassword ? ( +
+

Temporary password:

+ + {tempPassword} + + +
+ ) : ( - ) : ( -
- setNewPassword(e.target.value)} - autoFocus - /> -
- - -
-
)}
diff --git a/frontend/src/hooks/useAdmin.ts b/frontend/src/hooks/useAdmin.ts index 3d9d484..5d73c33 100644 --- a/frontend/src/hooks/useAdmin.ts +++ b/frontend/src/hooks/useAdmin.ts @@ -1,7 +1,6 @@ import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import api, { getErrorMessage } from '@/lib/api'; import type { - AdminUser, AdminUserDetail, AdminDashboardData, SystemConfig, @@ -9,11 +8,14 @@ import type { UserRole, } from '@/types'; +interface UserListResponse { + users: AdminUserDetail[]; + total: number; +} + interface AuditLogResponse { entries: AuditLogEntry[]; total: number; - page: number; - per_page: number; } interface CreateUserPayload { @@ -27,9 +29,9 @@ interface UpdateRolePayload { role: UserRole; } -interface ResetPasswordPayload { - userId: number; - new_password: string; +interface ResetPasswordResult { + message: string; + temporary_password: string; } // ── Queries ────────────────────────────────────────────────────────────────── @@ -38,8 +40,8 @@ export function useAdminUsers() { return useQuery({ queryKey: ['admin', 'users'], queryFn: async () => { - const { data } = await api.get('/admin/users'); - return data; + const { data } = await api.get('/admin/users'); + return data.users; }, }); } @@ -84,12 +86,12 @@ export function useAuditLog( // ── Mutations ───────────────────────────────────────────────────────────────── -function useAdminMutation( - mutationFn: (vars: TVariables) => Promise, +function useAdminMutation( + mutationFn: (vars: TVariables) => Promise, onSuccess?: () => void ) { const queryClient = useQueryClient(); - return useMutation({ + return useMutation({ mutationFn, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ['admin'] }); @@ -107,42 +109,42 @@ export function useCreateUser() { export function useUpdateRole() { return useAdminMutation(async ({ userId, role }: UpdateRolePayload) => { - const { data } = await api.patch(`/admin/users/${userId}/role`, { role }); + const { data } = await api.put(`/admin/users/${userId}/role`, { role }); return data; }); } export function useResetPassword() { - return useAdminMutation(async ({ userId, new_password }: ResetPasswordPayload) => { - const { data } = await api.post(`/admin/users/${userId}/reset-password`, { new_password }); + return useAdminMutation(async (userId: number) => { + const { data } = await api.post(`/admin/users/${userId}/reset-password`); return data; }); } export function useDisableMfa() { return useAdminMutation(async (userId: number) => { - const { data } = await api.delete(`/admin/users/${userId}/totp`); + const { data } = await api.post(`/admin/users/${userId}/disable-mfa`); return data; }); } export function useEnforceMfa() { return useAdminMutation(async (userId: number) => { - const { data } = await api.post(`/admin/users/${userId}/enforce-mfa`); + const { data } = await api.put(`/admin/users/${userId}/enforce-mfa`, { enforce: true }); return data; }); } export function useRemoveMfaEnforcement() { return useAdminMutation(async (userId: number) => { - const { data } = await api.delete(`/admin/users/${userId}/enforce-mfa`); + const { data } = await api.put(`/admin/users/${userId}/enforce-mfa`, { enforce: false }); return data; }); } export function useToggleUserActive() { return useAdminMutation(async ({ userId, active }: { userId: number; active: boolean }) => { - const { data } = await api.patch(`/admin/users/${userId}/active`, { is_active: active }); + const { data } = await api.put(`/admin/users/${userId}/active`, { is_active: active }); return data; }); } @@ -156,7 +158,7 @@ export function useRevokeSessions() { export function useUpdateConfig() { return useAdminMutation(async (config: Partial) => { - const { data } = await api.patch('/admin/config', config); + const { data } = await api.put('/admin/config', config); return data; }); } diff --git a/frontend/src/types/index.ts b/frontend/src/types/index.ts index 0cb34c4..542e3d4 100644 --- a/frontend/src/types/index.ts +++ b/frontend/src/types/index.ts @@ -258,7 +258,6 @@ export interface AdminDashboardData { recent_logins: Array<{ username: string; last_login_at: string; - ip_address: string; }>; recent_audit_entries: Array<{ action: string;