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 <noreply@anthropic.com>
This commit is contained in:
Kyle 2026-02-26 19:19:04 +08:00
parent e5a7ce13e0
commit e57a5b00c9
12 changed files with 96 additions and 102 deletions

View File

@ -76,7 +76,7 @@ def upgrade() -> None:
sa.Column("ip_address", sa.String(45), nullable=True), sa.Column("ip_address", sa.String(45), nullable=True),
sa.Column("created_at", sa.DateTime(), nullable=False, server_default=sa.text("NOW()")), sa.Column("created_at", sa.DateTime(), nullable=False, server_default=sa.text("NOW()")),
sa.PrimaryKeyConstraint("id"), sa.PrimaryKeyConstraint("id"),
sa.ForeignKeyConstraint(["actor_user_id"], ["users.id"]), sa.ForeignKeyConstraint(["actor_user_id"], ["users.id"], ondelete="SET NULL"),
sa.ForeignKeyConstraint( sa.ForeignKeyConstraint(
["target_user_id"], ["users.id"], ondelete="SET NULL" ["target_user_id"], ["users.id"], ondelete="SET NULL"
), ),

View File

@ -40,15 +40,18 @@ UMBRA_URL = "http://10.0.69.35"
# ── Dedup helpers ───────────────────────────────────────────────────────────── # ── 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( 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 return result.scalar_one_or_none() is not None
async def _mark_sent(db: AsyncSession, key: str) -> None: async def _mark_sent(db: AsyncSession, key: str, user_id: int) -> None:
db.add(NtfySent(notification_key=key)) db.add(NtfySent(notification_key=key, user_id=user_id))
await db.commit() 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 includes user_id to prevent cross-user dedup collisions
key = f"reminder:{settings.user_id}:{reminder.id}:{reminder.remind_at.date()}" 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 continue
payload = build_reminder_notification( payload = build_reminder_notification(
@ -91,7 +94,7 @@ async def _dispatch_reminders(db: AsyncSession, settings: Settings, now: datetim
**payload, **payload,
) )
if sent: 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: 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: for event in events:
# Key includes user_id to prevent cross-user dedup collisions # 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')}" 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 continue
payload = build_event_notification( payload = build_event_notification(
@ -142,7 +145,7 @@ async def _dispatch_events(db: AsyncSession, settings: Settings, now: datetime)
**payload, **payload,
) )
if sent: 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: 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: for todo in todos:
# Key includes user_id to prevent cross-user dedup collisions # Key includes user_id to prevent cross-user dedup collisions
key = f"todo:{settings.user_id}:{todo.id}:{today}" 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 continue
payload = build_todo_notification( payload = build_todo_notification(
@ -181,7 +184,7 @@ async def _dispatch_todos(db: AsyncSession, settings: Settings, today) -> None:
**payload, **payload,
) )
if sent: 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: 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: for project in projects:
# Key includes user_id to prevent cross-user dedup collisions # Key includes user_id to prevent cross-user dedup collisions
key = f"project:{settings.user_id}:{project.id}:{today}" 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 continue
payload = build_project_notification( payload = build_project_notification(
@ -219,7 +222,7 @@ async def _dispatch_projects(db: AsyncSession, settings: Settings, today) -> Non
**payload, **payload,
) )
if sent: 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: async def _dispatch_for_user(db: AsyncSession, settings: Settings, now: datetime) -> None:

View File

@ -53,7 +53,7 @@ app.add_middleware(
allow_origins=[o.strip() for o in settings.CORS_ORIGINS.split(",") if o.strip()], allow_origins=[o.strip() for o in settings.CORS_ORIGINS.split(",") if o.strip()],
allow_credentials=True, allow_credentials=True,
allow_methods=["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"], 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 # Include routers with /api prefix

View File

@ -14,7 +14,7 @@ class AuditLog(Base):
id: Mapped[int] = mapped_column(primary_key=True) id: Mapped[int] = mapped_column(primary_key=True)
actor_user_id: Mapped[Optional[int]] = mapped_column( 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( target_user_id: Mapped[Optional[int]] = mapped_column(
Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True, index=True Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True, index=True

View File

@ -176,7 +176,6 @@ async def create_user(
await db.flush() # populate new_user.id await db.flush() # populate new_user.id
await _create_user_defaults(db, new_user.id) await _create_user_defaults(db, new_user.id)
await db.commit()
await log_audit_event( await log_audit_event(
db, db,
@ -582,8 +581,7 @@ async def admin_dashboard(
mfa_adoption = (totp_count / total_users) if total_users else 0.0 mfa_adoption = (totp_count / total_users) if total_users else 0.0
# 10 most recent logins — join to get username # 10 most recent logins
actor_alias = sa.alias(User.__table__, name="actor")
recent_logins_result = await db.execute( recent_logins_result = await db.execute(
sa.select(User.username, User.last_login_at) sa.select(User.username, User.last_login_at)
.where(User.last_login_at != None) .where(User.last_login_at != None)
@ -595,20 +593,28 @@ async def admin_dashboard(
for row in recent_logins_result 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( 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 = [ recent_audit_entries = [
{ {
"id": e.id, "action": row.AuditLog.action,
"action": e.action, "actor_username": row.actor_username,
"actor_user_id": e.actor_user_id, "target_username": row.target_username,
"target_user_id": e.target_user_id, "created_at": row.AuditLog.created_at,
"detail": e.detail,
"created_at": e.created_at,
} }
for e in recent_audit_result.scalars() for row in recent_audit_result
] ]
return AdminDashboardResponse( return AdminDashboardResponse(

View File

@ -22,7 +22,7 @@ from typing import Optional
from fastapi import APIRouter, Depends, HTTPException, Request, Response, Cookie from fastapi import APIRouter, Depends, HTTPException, Request, Response, Cookie
from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy import select from sqlalchemy import select, func
from app.database import get_db from app.database import get_db
from app.models.user import User from app.models.user import User
@ -249,7 +249,6 @@ async def setup(
await db.flush() await db.flush()
await _create_user_defaults(db, new_user.id) await _create_user_defaults(db, new_user.id)
await db.commit()
ip = request.client.host if request.client else "unknown" ip = request.client.host if request.client else "unknown"
user_agent = request.headers.get("user-agent") user_agent = request.headers.get("user-agent")
@ -376,7 +375,7 @@ async def register(
select(User).where(User.username == data.username) select(User).where(User.username == data.username)
) )
if existing.scalar_one_or_none(): 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) password_hash = hash_password(data.password)
# SEC-01: Explicit field assignment — never **data.model_dump() # SEC-01: Explicit field assignment — never **data.model_dump()
@ -395,7 +394,6 @@ async def register(
await db.flush() await db.flush()
await _create_user_defaults(db, new_user.id) await _create_user_defaults(db, new_user.id)
await db.commit()
ip = request.client.host if request.client else "unknown" ip = request.client.host if request.client else "unknown"
user_agent = request.headers.get("user-agent") 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. Check authentication status, role, and whether initial setup/registration is available.
""" """
user_result = await db.execute(select(User)) user_count_result = await db.execute(
existing_user = user_result.scalar_one_or_none() select(func.count()).select_from(User)
setup_required = existing_user is None )
setup_required = user_count_result.scalar_one() == 0
authenticated = False authenticated = False
role = None role = None

View File

@ -64,14 +64,17 @@ class CreateUserRequest(BaseModel):
class UpdateUserRoleRequest(BaseModel): class UpdateUserRoleRequest(BaseModel):
model_config = ConfigDict(extra="forbid")
role: Literal["admin", "standard", "public_event_manager"] role: Literal["admin", "standard", "public_event_manager"]
class ToggleActiveRequest(BaseModel): class ToggleActiveRequest(BaseModel):
model_config = ConfigDict(extra="forbid")
is_active: bool is_active: bool
class ToggleMfaEnforceRequest(BaseModel): class ToggleMfaEnforceRequest(BaseModel):
model_config = ConfigDict(extra="forbid")
enforce: bool enforce: bool
@ -87,6 +90,7 @@ class SystemConfigResponse(BaseModel):
class SystemConfigUpdate(BaseModel): class SystemConfigUpdate(BaseModel):
model_config = ConfigDict(extra="forbid")
allow_registration: Optional[bool] = None allow_registration: Optional[bool] = None
enforce_mfa_new_users: Optional[bool] = None enforce_mfa_new_users: Optional[bool] = None

View File

@ -135,9 +135,6 @@ export default function AdminDashboardPage() {
<th className="px-5 py-2.5 text-left text-[11px] uppercase tracking-wider text-muted-foreground font-medium"> <th className="px-5 py-2.5 text-left text-[11px] uppercase tracking-wider text-muted-foreground font-medium">
When When
</th> </th>
<th className="px-5 py-2.5 text-left text-[11px] uppercase tracking-wider text-muted-foreground font-medium">
IP
</th>
</tr> </tr>
</thead> </thead>
<tbody> <tbody>
@ -153,9 +150,6 @@ export default function AdminDashboardPage() {
<td className="px-5 py-2.5 text-xs text-muted-foreground"> <td className="px-5 py-2.5 text-xs text-muted-foreground">
{getRelativeTime(entry.last_login_at)} {getRelativeTime(entry.last_login_at)}
</td> </td>
<td className="px-5 py-2.5 text-xs text-muted-foreground font-mono">
{entry.ip_address ?? '—'}
</td>
</tr> </tr>
))} ))}
</tbody> </tbody>

View File

@ -75,11 +75,11 @@ export default function CreateUserDialog({ open, onOpenChange }: CreateUserDialo
type="password" type="password"
value={password} value={password}
onChange={(e) => setPassword(e.target.value)} onChange={(e) => setPassword(e.target.value)}
placeholder="Min. 8 characters" placeholder="Min. 12 characters"
required required
/> />
<p className="text-[11px] text-muted-foreground"> <p className="text-[11px] text-muted-foreground">
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.
</p> </p>
</div> </div>

View File

@ -41,8 +41,7 @@ const ROLES: { value: UserRole; label: string }[] = [
export default function UserActionsMenu({ user }: UserActionsMenuProps) { export default function UserActionsMenu({ user }: UserActionsMenuProps) {
const [open, setOpen] = useState(false); const [open, setOpen] = useState(false);
const [roleSubmenuOpen, setRoleSubmenuOpen] = useState(false); const [roleSubmenuOpen, setRoleSubmenuOpen] = useState(false);
const [showResetPassword, setShowResetPassword] = useState(false); const [tempPassword, setTempPassword] = useState<string | null>(null);
const [newPassword, setNewPassword] = useState('');
const menuRef = useRef<HTMLDivElement>(null); const menuRef = useRef<HTMLDivElement>(null);
const updateRole = useUpdateRole(); const updateRole = useUpdateRole();
@ -162,50 +161,38 @@ export default function UserActionsMenu({ user }: UserActionsMenuProps) {
</div> </div>
{/* Reset Password */} {/* Reset Password */}
{!showResetPassword ? ( {tempPassword ? (
<div className="px-3 py-2 space-y-1.5">
<p className="text-[11px] text-muted-foreground">Temporary password:</p>
<code className="block px-2 py-1.5 bg-card-elevated rounded text-xs font-mono text-accent select-all break-all">
{tempPassword}
</code>
<button
className="w-full rounded-md bg-muted px-2 py-1 text-xs text-muted-foreground hover:text-foreground transition-colors"
onClick={() => {
setTempPassword(null);
setOpen(false);
}}
>
Done
</button>
</div>
) : (
<button <button
className="flex w-full items-center gap-2 px-3 py-2 text-sm hover:bg-card-elevated transition-colors" className="flex w-full items-center gap-2 px-3 py-2 text-sm hover:bg-card-elevated transition-colors"
onClick={() => setShowResetPassword(true)} onClick={async () => {
try {
const result = await resetPassword.mutateAsync(user.id);
setTempPassword((result as { temporary_password: string }).temporary_password);
toast.success('Password reset — user must change on next login');
} catch (err) {
toast.error(getErrorMessage(err, 'Password reset failed'));
}
}}
> >
<KeyRound className="h-4 w-4 text-muted-foreground" /> <KeyRound className="h-4 w-4 text-muted-foreground" />
Reset Password Reset Password
</button> </button>
) : (
<div className="px-3 py-2 space-y-2">
<input
className="h-8 w-full rounded-md border border-input bg-background px-2 text-sm focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring"
type="password"
placeholder="New password"
value={newPassword}
onChange={(e) => setNewPassword(e.target.value)}
autoFocus
/>
<div className="flex gap-2">
<button
className="flex-1 rounded-md bg-accent/15 px-2 py-1 text-xs text-accent hover:bg-accent/25 transition-colors"
onClick={() => {
if (!newPassword.trim()) return;
handleAction(
() => resetPassword.mutateAsync({ userId: user.id, new_password: newPassword }),
'Password reset'
);
setNewPassword('');
setShowResetPassword(false);
}}
>
Set
</button>
<button
className="flex-1 rounded-md bg-muted px-2 py-1 text-xs text-muted-foreground hover:text-foreground transition-colors"
onClick={() => {
setShowResetPassword(false);
setNewPassword('');
}}
>
Cancel
</button>
</div>
</div>
)} )}
<div className="my-1 border-t border-border" /> <div className="my-1 border-t border-border" />

View File

@ -1,7 +1,6 @@
import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
import api, { getErrorMessage } from '@/lib/api'; import api, { getErrorMessage } from '@/lib/api';
import type { import type {
AdminUser,
AdminUserDetail, AdminUserDetail,
AdminDashboardData, AdminDashboardData,
SystemConfig, SystemConfig,
@ -9,11 +8,14 @@ import type {
UserRole, UserRole,
} from '@/types'; } from '@/types';
interface UserListResponse {
users: AdminUserDetail[];
total: number;
}
interface AuditLogResponse { interface AuditLogResponse {
entries: AuditLogEntry[]; entries: AuditLogEntry[];
total: number; total: number;
page: number;
per_page: number;
} }
interface CreateUserPayload { interface CreateUserPayload {
@ -27,9 +29,9 @@ interface UpdateRolePayload {
role: UserRole; role: UserRole;
} }
interface ResetPasswordPayload { interface ResetPasswordResult {
userId: number; message: string;
new_password: string; temporary_password: string;
} }
// ── Queries ────────────────────────────────────────────────────────────────── // ── Queries ──────────────────────────────────────────────────────────────────
@ -38,8 +40,8 @@ export function useAdminUsers() {
return useQuery<AdminUserDetail[]>({ return useQuery<AdminUserDetail[]>({
queryKey: ['admin', 'users'], queryKey: ['admin', 'users'],
queryFn: async () => { queryFn: async () => {
const { data } = await api.get<AdminUserDetail[]>('/admin/users'); const { data } = await api.get<UserListResponse>('/admin/users');
return data; return data.users;
}, },
}); });
} }
@ -84,12 +86,12 @@ export function useAuditLog(
// ── Mutations ───────────────────────────────────────────────────────────────── // ── Mutations ─────────────────────────────────────────────────────────────────
function useAdminMutation<TVariables>( function useAdminMutation<TVariables, TData = unknown>(
mutationFn: (vars: TVariables) => Promise<unknown>, mutationFn: (vars: TVariables) => Promise<TData>,
onSuccess?: () => void onSuccess?: () => void
) { ) {
const queryClient = useQueryClient(); const queryClient = useQueryClient();
return useMutation<unknown, Error, TVariables>({ return useMutation<TData, Error, TVariables>({
mutationFn, mutationFn,
onSuccess: () => { onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ['admin'] }); queryClient.invalidateQueries({ queryKey: ['admin'] });
@ -107,42 +109,42 @@ export function useCreateUser() {
export function useUpdateRole() { export function useUpdateRole() {
return useAdminMutation(async ({ userId, role }: UpdateRolePayload) => { 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; return data;
}); });
} }
export function useResetPassword() { export function useResetPassword() {
return useAdminMutation(async ({ userId, new_password }: ResetPasswordPayload) => { return useAdminMutation(async (userId: number) => {
const { data } = await api.post(`/admin/users/${userId}/reset-password`, { new_password }); const { data } = await api.post<ResetPasswordResult>(`/admin/users/${userId}/reset-password`);
return data; return data;
}); });
} }
export function useDisableMfa() { export function useDisableMfa() {
return useAdminMutation(async (userId: number) => { 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; return data;
}); });
} }
export function useEnforceMfa() { export function useEnforceMfa() {
return useAdminMutation(async (userId: number) => { 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; return data;
}); });
} }
export function useRemoveMfaEnforcement() { export function useRemoveMfaEnforcement() {
return useAdminMutation(async (userId: number) => { 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; return data;
}); });
} }
export function useToggleUserActive() { export function useToggleUserActive() {
return useAdminMutation(async ({ userId, active }: { userId: number; active: boolean }) => { 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; return data;
}); });
} }
@ -156,7 +158,7 @@ export function useRevokeSessions() {
export function useUpdateConfig() { export function useUpdateConfig() {
return useAdminMutation(async (config: Partial<SystemConfig>) => { return useAdminMutation(async (config: Partial<SystemConfig>) => {
const { data } = await api.patch('/admin/config', config); const { data } = await api.put('/admin/config', config);
return data; return data;
}); });
} }

View File

@ -258,7 +258,6 @@ export interface AdminDashboardData {
recent_logins: Array<{ recent_logins: Array<{
username: string; username: string;
last_login_at: string; last_login_at: string;
ip_address: string;
}>; }>;
recent_audit_entries: Array<{ recent_audit_entries: Array<{
action: string; action: string;