From e9e8fbf3f5acdf9090044de51a2e20f1dd6e1e86 Mon Sep 17 00:00:00 2001 From: tanyar09 Date: Fri, 5 Dec 2025 14:20:45 -0500 Subject: [PATCH] feat: Add is_active and role fields to AuthUser schema and update user management logic This commit introduces new fields `is_active` and `role` to the `AuthUserResponse` and `AuthUserUpdateRequest` schemas, enhancing user management capabilities. The `deleteUser` and `updateUser` functions are updated to handle user deactivation instead of deletion when linked data exists. Additionally, the ManageUsers component is enhanced with filtering options for active status and roles, improving user experience. Documentation has been updated to reflect these changes. --- frontend/src/api/authUsers.ts | 10 +- frontend/src/api/users.ts | 6 +- frontend/src/pages/ManageUsers.tsx | 168 +++++++++- frontend/src/pages/Search.tsx | 32 +- src/web/api/auth_users.py | 501 ++++++++++++++++++++++++----- src/web/api/users.py | 88 ++++- src/web/app.py | 62 +++- src/web/schemas/auth_users.py | 4 + 8 files changed, 737 insertions(+), 134 deletions(-) diff --git a/frontend/src/api/authUsers.ts b/frontend/src/api/authUsers.ts index e6b1627..9498d3f 100644 --- a/frontend/src/api/authUsers.ts +++ b/frontend/src/api/authUsers.ts @@ -6,6 +6,8 @@ export interface AuthUserResponse { email: string is_admin: boolean | null has_write_access: boolean | null + is_active: boolean | null + role: string | null created_at: string | null updated_at: string | null } @@ -23,6 +25,8 @@ export interface AuthUserUpdateRequest { name: string is_admin: boolean has_write_access: boolean + is_active?: boolean + role?: string } export interface AuthUsersListResponse { @@ -57,8 +61,10 @@ export const authUsersApi = { return data }, - deleteUser: async (userId: number): Promise => { - await apiClient.delete(`/api/v1/auth-users/${userId}`) + deleteUser: async (userId: number): Promise<{ message?: string; deactivated?: boolean }> => { + const response = await apiClient.delete(`/api/v1/auth-users/${userId}`) + // Return data if present (200 OK with deactivation message), otherwise empty object (204 No Content) + return response.data || {} }, } diff --git a/frontend/src/api/users.ts b/frontend/src/api/users.ts index 6cc1d5a..b553553 100644 --- a/frontend/src/api/users.ts +++ b/frontend/src/api/users.ts @@ -79,8 +79,10 @@ export const usersApi = { return data }, - deleteUser: async (userId: number): Promise => { - await apiClient.delete(`/api/v1/users/${userId}`) + deleteUser: async (userId: number): Promise<{ message?: string; deactivated?: boolean }> => { + const response = await apiClient.delete(`/api/v1/users/${userId}`) + // Return data if present (200 OK with deactivation message), otherwise empty object (204 No Content) + return response.data || {} }, } diff --git a/frontend/src/pages/ManageUsers.tsx b/frontend/src/pages/ManageUsers.tsx index 7bacc58..8cb304c 100644 --- a/frontend/src/pages/ManageUsers.tsx +++ b/frontend/src/pages/ManageUsers.tsx @@ -34,6 +34,7 @@ type AuthUserSortKey = | 'name' | 'is_admin' | 'has_write_access' + | 'is_active' | 'created_at' | 'updated_at' const DEFAULT_ADMIN_ROLE: UserRoleValue = 'admin' @@ -114,7 +115,7 @@ export default function ManageUsers() { const [error, setError] = useState(null) const [showCreateModal, setShowCreateModal] = useState(false) const [editingUser, setEditingUser] = useState(null) - const [filterActive, setFilterActive] = useState(null) + const [filterActive, setFilterActive] = useState(true) const [filterRole, setFilterRole] = useState(null) const [createForm, setCreateForm] = useState({ @@ -146,6 +147,8 @@ export default function ManageUsers() { const [authError, setAuthError] = useState(null) const [showAuthCreateModal, setShowAuthCreateModal] = useState(false) const [editingAuthUser, setEditingAuthUser] = useState(null) + const [authFilterActive, setAuthFilterActive] = useState(true) + const [authFilterRole, setAuthFilterRole] = useState(null) // 'Admin' or 'User' const [authCreateForm, setAuthCreateForm] = useState({ email: '', @@ -160,6 +163,8 @@ export default function ManageUsers() { name: '', is_admin: false, has_write_access: false, + is_active: true, + role: 'User', }) const [grantFrontendPermission, setGrantFrontendPermission] = useState(false) @@ -598,6 +603,8 @@ const getDisplayRoleLabel = (user: UserResponse): string => { return user.is_admin ? 1 : 0 case 'has_write_access': return user.has_write_access ? 1 : 0 + case 'is_active': + return user.is_active !== false ? 1 : 0 case 'created_at': return user.created_at ? new Date(user.created_at).getTime() : 0 case 'updated_at': @@ -634,8 +641,31 @@ const getDisplayRoleLabel = (user: UserResponse): string => { return cloned }, [filteredUsers, userSort]) + const filteredAuthUsers = useMemo(() => { + let filtered = [...authUsers] + + // Filter by active status + if (authFilterActive !== null) { + filtered = filtered.filter((user) => { + const isActive = user.is_active !== false // Default to true if null/undefined + return isActive === authFilterActive + }) + } + + // Filter by role (Admin/User) + if (authFilterRole !== null) { + filtered = filtered.filter((user) => { + // Use role field if available, otherwise derive from is_admin + const userRole = user.role || (user.is_admin === true ? 'Admin' : 'User') + return userRole === authFilterRole + }) + } + + return filtered + }, [authUsers, authFilterActive, authFilterRole]) + const sortedAuthUsers = useMemo(() => { - const cloned = [...authUsers] + const cloned = [...filteredAuthUsers] cloned.sort((a, b) => { const valueA = getAuthSortValue(a, authSort.key) const valueB = getAuthSortValue(b, authSort.key) @@ -646,7 +676,7 @@ const getDisplayRoleLabel = (user: UserResponse): string => { return authSort.direction === 'asc' ? comparison : -comparison }) return cloned - }, [authUsers, authSort]) + }, [filteredAuthUsers, authSort]) const getUserSortIndicator = (key: UserSortKey) => userSort.key === key ? (userSort.direction === 'asc' ? '▲' : '▼') : '↕' @@ -669,6 +699,8 @@ const getDisplayRoleLabel = (user: UserResponse): string => { name: authEditForm.name, is_admin: authEditForm.is_admin, has_write_access: authEditForm.has_write_access, + is_active: authEditForm.is_active, + role: authEditForm.role, } await authUsersApi.updateUser(editingAuthUser.id, updateData) setEditingAuthUser(null) @@ -677,6 +709,8 @@ const getDisplayRoleLabel = (user: UserResponse): string => { name: '', is_admin: false, has_write_access: false, + is_active: true, + role: 'User', }) loadAuthUsers() } catch (err: any) { @@ -690,17 +724,18 @@ const getDisplayRoleLabel = (user: UserResponse): string => { } try { setError(null) - await usersApi.deleteUser(userId) + const result = await usersApi.deleteUser(userId) loadUsers() + // If user was deactivated instead of deleted, show informational message + if (result.deactivated && result.message) { + // Show as info message, not error + setError(null) + // You might want to add a success/info message state here + // For now, we'll just reload and the user will see the status changed + alert(result.message) + } } catch (err: any) { const responseDetail = err.response?.data?.detail - if (err.response?.status === 409) { - setError( - responseDetail || - 'This user identified faces and cannot be deleted. Set them inactive instead.' - ) - return - } setError(responseDetail || 'Failed to delete user') } } @@ -711,8 +746,16 @@ const getDisplayRoleLabel = (user: UserResponse): string => { } try { setAuthError(null) - await authUsersApi.deleteUser(userId) + const result = await authUsersApi.deleteUser(userId) loadAuthUsers() + // If user was deactivated instead of deleted, show informational message + if (result.deactivated && result.message) { + // Show as info message, not error + setAuthError(null) + // You might want to add a success/info message state here + // For now, we'll just reload and the user will see the status changed + alert(result.message) + } } catch (err: any) { setAuthError(err.response?.data?.detail || 'Failed to delete auth user') } @@ -735,11 +778,16 @@ const getDisplayRoleLabel = (user: UserResponse): string => { const startAuthEdit = (user: AuthUserResponse) => { setEditingAuthUser(user) + // Determine role: if is_admin is true, role is 'Admin', otherwise 'User' + // Use user.role if available, otherwise derive from is_admin + const userRole = user.role || (user.is_admin === true ? 'Admin' : 'User') setAuthEditForm({ email: user.email || '', name: user.name || '', is_admin: user.is_admin === true, has_write_access: user.has_write_access === true, + is_active: user.is_active !== false, // Default to true if null/undefined + role: userRole, }) } @@ -1033,7 +1081,50 @@ const getDisplayRoleLabel = (user: UserResponse): string => { {/* Frontend Users Tab */} {activeTab === 'frontend' && ( -
+ <> + {/* Filters */} +
+
+ + + +
+
+ + {authError && ( +
+ {authError} +
+ )} + +
{authLoading ? (
Loading users...
) : sortedAuthUsers.length === 0 ? ( @@ -1084,6 +1175,16 @@ const getDisplayRoleLabel = (user: UserResponse): string => { + + +
+
+ )} {/* Manage Roles Tab */} @@ -1679,6 +1792,22 @@ const getDisplayRoleLabel = (user: UserResponse): string => { required /> +
+ + +
+
diff --git a/frontend/src/pages/Search.tsx b/frontend/src/pages/Search.tsx index 3c1d80e..aa2370e 100644 --- a/frontend/src/pages/Search.tsx +++ b/frontend/src/pages/Search.tsx @@ -167,8 +167,10 @@ export default function Search() { restoredSearchType === 'unprocessed' || restoredSearchType === 'favorites')) { // Use a small delay to ensure state is fully restored + // Don't show validation errors for auto-searches + // Pass the restored search type directly to avoid stale state issues setTimeout(() => { - performSearch() + performSearch(undefined, false, restoredSearchType ?? undefined) }, 150) } }, 100) @@ -211,24 +213,26 @@ export default function Search() { } }, [searchType, selectedTags, matchAll, dateFrom, dateTo, mediaType, selectedPeople, inputValue, tagsExpanded, filtersExpanded, configExpanded, sortColumn, sortDir, page, results, total]) - const performSearch = async (pageNum: number = page) => { + const performSearch = async (pageNum: number = page, showValidationErrors: boolean = true, searchTypeOverride?: SearchType) => { setLoading(true) try { + // Use override if provided (for state restoration), otherwise use current state + const currentSearchType = searchTypeOverride ?? searchType const params: any = { - search_type: searchType, + search_type: currentSearchType, page: pageNum, page_size: pageSize, } // For "Photos without faces" search, always exclude videos - if (searchType === 'no_faces') { + if (currentSearchType === 'no_faces') { params.media_type = 'image' } else if (mediaType && mediaType !== 'all') { // Add media type filter if not 'all' for other search types params.media_type = mediaType } - if (searchType === 'name') { + if (currentSearchType === 'name') { // Combine selected people names and free text input // For selected people, use last name (most unique) or first+last if last name is empty const selectedNames = selectedPeople.map(p => { @@ -243,22 +247,28 @@ export default function Search() { const allNames = [...selectedNames, freeText].filter(Boolean) if (allNames.length === 0) { - alert('Please enter at least one name or select a person to search.') + if (showValidationErrors) { + alert('Please enter at least one name or select a person to search.') + } setLoading(false) return } params.person_name = allNames.join(', ') - } else if (searchType === 'date') { + } else if (currentSearchType === 'date') { if (!dateFrom && !dateTo) { - alert('Please enter at least one date (from date or to date).') + if (showValidationErrors) { + alert('Please enter at least one date (from date or to date).') + } setLoading(false) return } params.date_from = dateFrom || undefined params.date_to = dateTo || undefined - } else if (searchType === 'tags') { + } else if (currentSearchType === 'tags') { if (selectedTags.length === 0) { - alert('Please select at least one tag to search for.') + if (showValidationErrors) { + alert('Please select at least one tag to search for.') + } setLoading(false) return } @@ -283,7 +293,7 @@ export default function Search() { const handleSearch = () => { setPage(1) setSelectedPhotos(new Set()) - performSearch(1) + performSearch(1, true) // Show validation errors when user explicitly clicks search } // Filter people for dropdown based on input diff --git a/src/web/api/auth_users.py b/src/web/api/auth_users.py index 4f656b1..b867151 100644 --- a/src/web/api/auth_users.py +++ b/src/web/api/auth_users.py @@ -2,9 +2,11 @@ from __future__ import annotations +import logging from typing import Annotated from fastapi import APIRouter, Depends, HTTPException, Query, Response, status +from fastapi.responses import JSONResponse from sqlalchemy import text from sqlalchemy.orm import Session @@ -20,6 +22,28 @@ from src.web.schemas.auth_users import ( from src.web.utils.password import hash_password router = APIRouter(prefix="/auth-users", tags=["auth-users"]) +logger = logging.getLogger(__name__) + + +def _check_column_exists(auth_db: Session, table_name: str, column_name: str) -> bool: + """Check if a column exists in a table.""" + try: + from sqlalchemy import inspect as sqlalchemy_inspect + inspector = sqlalchemy_inspect(auth_db.bind) + columns = {col["name"] for col in inspector.get_columns(table_name)} + return column_name in columns + except Exception: + return False + + +def _get_role_from_is_admin(auth_db: Session, is_admin: bool) -> str: + """Get role value from is_admin boolean. Returns 'Admin' if is_admin is True, 'User' otherwise.""" + return "Admin" if is_admin else "User" + + +def _get_is_admin_from_role(role: str | None) -> bool: + """Get is_admin boolean from role string. Returns True if role is 'Admin', False otherwise.""" + return role == "Admin" if role else False @router.get("", response_model=AuthUsersListResponse) @@ -29,53 +53,127 @@ def list_auth_users( ) -> AuthUsersListResponse: """List all users from auth database - admin only.""" try: + # Check if optional columns exist + has_role_column = _check_column_exists(auth_db, "users", "role") + has_is_active_column = _check_column_exists(auth_db, "users", "is_active") + # Query users from auth database with all columns from schema - result = auth_db.execute(text(""" - SELECT - id, - email, - name, - is_admin, - has_write_access, - created_at, - updated_at - FROM users - ORDER BY COALESCE(name, email) ASC - """)) + # Try to include is_active and role if columns exist + result = None + try: + # Build SELECT query based on which columns exist + select_fields = "id, email, name, is_admin, has_write_access" + if has_is_active_column: + select_fields += ", is_active" + if has_role_column: + select_fields += ", role" + select_fields += ", created_at, updated_at" + + result = auth_db.execute(text(f""" + SELECT {select_fields} + FROM users + ORDER BY COALESCE(name, email) ASC + """)) + except Exception: + # Rollback the failed transaction before trying again + auth_db.rollback() + try: + # Try with is_active only (no role) + select_fields = "id, email, name, is_admin, has_write_access" + if has_is_active_column: + select_fields += ", is_active" + select_fields += ", created_at, updated_at" + result = auth_db.execute(text(f""" + SELECT {select_fields} + FROM users + ORDER BY COALESCE(name, email) ASC + """)) + except Exception: + # Rollback again before final attempt + auth_db.rollback() + # Base columns only + result = auth_db.execute(text(""" + SELECT + id, + email, + name, + is_admin, + has_write_access, + created_at, + updated_at + FROM users + ORDER BY COALESCE(name, email) ASC + """)) + + if result is None: + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to query auth users", + ) rows = result.fetchall() users = [] for row in rows: - # Access row attributes directly - SQLAlchemy Row objects support attribute access - user_id = int(row.id) - email = str(row.email) - name = row.name if row.name is not None else None - - # Get boolean fields - convert to proper boolean - # These columns have defaults so they should always have values - is_admin = bool(row.is_admin) - has_write_access = bool(row.has_write_access) - - created_at = row.created_at - updated_at = row.updated_at - - users.append(AuthUserResponse( - id=user_id, - name=name, - email=email, - is_admin=is_admin, - has_write_access=has_write_access, - created_at=created_at, - updated_at=updated_at, - )) + try: + # Access row attributes directly - SQLAlchemy Row objects support attribute access + user_id = int(row.id) + email = str(row.email) + name = row.name if row.name is not None else None + + # Get boolean fields - convert to proper boolean + # These columns have defaults so they should always have values + is_admin = bool(row.is_admin) + has_write_access = bool(row.has_write_access) + + # Optional columns - try to get from row, default to None if not selected + is_active = None + try: + is_active = row.is_active + if is_active is not None: + is_active = bool(is_active) + else: + # NULL values should be treated as True (active) + is_active = True + except (AttributeError, KeyError): + # Column not selected or doesn't exist - default to True (active) + is_active = True + + # Get role - if column doesn't exist, derive from is_admin + if has_role_column: + role = getattr(row, 'role', None) + else: + role = _get_role_from_is_admin(auth_db, is_admin) + + created_at = getattr(row, 'created_at', None) + updated_at = getattr(row, 'updated_at', None) + + users.append(AuthUserResponse( + id=user_id, + name=name, + email=email, + is_admin=is_admin, + has_write_access=has_write_access, + is_active=is_active, + role=role, + created_at=created_at, + updated_at=updated_at, + )) + except Exception as row_error: + logger.warning(f"Error processing auth user row: {row_error}") + # Skip this row and continue + continue return AuthUsersListResponse(items=users, total=len(users)) + except HTTPException: + raise except Exception as e: + auth_db.rollback() import traceback error_detail = f"Failed to list auth users: {str(e)}\n{traceback.format_exc()}" + logger.error(f"Error listing auth users: {error_detail}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=error_detail, + detail=f"Failed to list auth users: {str(e)}", ) @@ -179,11 +277,48 @@ def get_auth_user( ) -> AuthUserResponse: """Get a specific auth user by ID - admin only.""" try: - result = auth_db.execute(text(""" - SELECT id, email, name, is_admin, has_write_access, created_at, updated_at - FROM users - WHERE id = :user_id - """), {"user_id": user_id}) + # Check if optional columns exist + has_role_column = _check_column_exists(auth_db, "users", "role") + has_is_active_column = _check_column_exists(auth_db, "users", "is_active") + + # Try to include is_active and role if columns exist + try: + # Build SELECT query based on which columns exist + select_fields = "id, email, name, is_admin, has_write_access" + if has_is_active_column: + select_fields += ", is_active" + if has_role_column: + select_fields += ", role" + select_fields += ", created_at, updated_at" + + result = auth_db.execute(text(f""" + SELECT {select_fields} + FROM users + WHERE id = :user_id + """), {"user_id": user_id}) + except Exception: + # Rollback the failed transaction before trying again + auth_db.rollback() + try: + # Try with is_active only (no role) + select_fields = "id, email, name, is_admin, has_write_access" + if has_is_active_column: + select_fields += ", is_active" + select_fields += ", created_at, updated_at" + result = auth_db.execute(text(f""" + SELECT {select_fields} + FROM users + WHERE id = :user_id + """), {"user_id": user_id}) + except Exception: + # Rollback again before final attempt + auth_db.rollback() + # Columns don't exist, select without them + result = auth_db.execute(text(""" + SELECT id, email, name, is_admin, has_write_access, created_at, updated_at + FROM users + WHERE id = :user_id + """), {"user_id": user_id}) row = result.first() if not row: @@ -194,6 +329,18 @@ def get_auth_user( is_admin = bool(row.is_admin) has_write_access = bool(row.has_write_access) + is_active = getattr(row, 'is_active', None) + if is_active is not None: + is_active = bool(is_active) + else: + # NULL values should be treated as True (active) + is_active = True + + # Get role - if column doesn't exist, derive from is_admin + if has_role_column: + role = getattr(row, 'role', None) + else: + role = _get_role_from_is_admin(auth_db, is_admin) return AuthUserResponse( id=row.id, @@ -201,12 +348,15 @@ def get_auth_user( email=row.email, is_admin=is_admin, has_write_access=has_write_access, + is_active=is_active, + role=role, created_at=getattr(row, 'created_at', None), updated_at=getattr(row, 'updated_at', None), ) except HTTPException: raise except Exception as e: + auth_db.rollback() raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=f"Failed to get auth user: {str(e)}", @@ -248,52 +398,94 @@ def update_auth_user( detail=f"User with email '{request.email}' already exists", ) - # Update all fields (all are required) + # Check if role column exists + has_role_column = _check_column_exists(auth_db, "users", "role") + + # Update all fields (all are required, is_active and role are optional) dialect = auth_db.bind.dialect.name if auth_db.bind else 'postgresql' supports_returning = dialect == 'postgresql' + # Determine is_admin value - if role is provided and role column doesn't exist, use role to set is_admin + is_admin_value = request.is_admin + if request.role is not None and not has_role_column: + # Role column doesn't exist, derive is_admin from role + is_admin_value = _get_is_admin_from_role(request.role) + + # Build UPDATE query - include is_active and role if provided and column exists + update_fields = ["email = :email", "name = :name", "is_admin = :is_admin", "has_write_access = :has_write_access"] + update_params = { + "user_id": user_id, + "email": request.email, + "name": request.name, + "is_admin": is_admin_value, + "has_write_access": request.has_write_access, + } + + if request.is_active is not None: + update_fields.append("is_active = :is_active") + update_params["is_active"] = request.is_active + + if request.role is not None and has_role_column: + # Only update role if the column exists + update_fields.append("role = :role") + update_params["role"] = request.role + + update_sql = f""" + UPDATE users + SET {', '.join(update_fields)} + WHERE id = :user_id + """ + if supports_returning: - result = auth_db.execute(text(""" - UPDATE users - SET email = :email, - name = :name, - is_admin = :is_admin, - has_write_access = :has_write_access - WHERE id = :user_id - RETURNING id, email, name, is_admin, has_write_access, created_at, updated_at - """), { - "user_id": user_id, - "email": request.email, - "name": request.name, - "is_admin": request.is_admin, - "has_write_access": request.has_write_access, - }) + # Build select fields - try to include optional columns + select_fields = "id, email, name, is_admin, has_write_access" + if request.is_active is not None or _check_column_exists(auth_db, "users", "is_active"): + select_fields += ", is_active" + if has_role_column: + select_fields += ", role" + select_fields += ", created_at, updated_at" + result = auth_db.execute(text(f""" + {update_sql} + RETURNING {select_fields} + """), update_params) auth_db.commit() row = result.first() else: # SQLite - update then select - auth_db.execute(text(""" - UPDATE users - SET email = :email, - name = :name, - is_admin = :is_admin, - has_write_access = :has_write_access - WHERE id = :user_id - """), { - "user_id": user_id, - "email": request.email, - "name": request.name, - "is_admin": request.is_admin, - "has_write_access": request.has_write_access, - }) + auth_db.execute(text(update_sql), update_params) auth_db.commit() - # Get the updated row - result = auth_db.execute(text(""" - SELECT id, email, name, is_admin, has_write_access, created_at, updated_at - FROM users - WHERE id = :user_id - """), {"user_id": user_id}) - row = result.first() + # Get the updated row - try to include optional columns + try: + if has_role_column: + result = auth_db.execute(text(""" + SELECT id, email, name, is_admin, has_write_access, is_active, role, created_at, updated_at + FROM users + WHERE id = :user_id + """), {"user_id": user_id}) + else: + result = auth_db.execute(text(""" + SELECT id, email, name, is_admin, has_write_access, is_active, created_at, updated_at + FROM users + WHERE id = :user_id + """), {"user_id": user_id}) + row = result.first() + except Exception: + auth_db.rollback() + try: + result = auth_db.execute(text(""" + SELECT id, email, name, is_admin, has_write_access, is_active, created_at, updated_at + FROM users + WHERE id = :user_id + """), {"user_id": user_id}) + row = result.first() + except Exception: + auth_db.rollback() + result = auth_db.execute(text(""" + SELECT id, email, name, is_admin, has_write_access, created_at, updated_at + FROM users + WHERE id = :user_id + """), {"user_id": user_id}) + row = result.first() if not row: raise HTTPException( @@ -303,6 +495,18 @@ def update_auth_user( is_admin = bool(row.is_admin) has_write_access = bool(row.has_write_access) + is_active = getattr(row, 'is_active', None) + if is_active is not None: + is_active = bool(is_active) + else: + # NULL values should be treated as True (active) + is_active = True + + # Get role - if column doesn't exist, derive from is_admin + if has_role_column: + role = getattr(row, 'role', None) + else: + role = _get_role_from_is_admin(auth_db, is_admin) return AuthUserResponse( id=row.id, @@ -310,6 +514,8 @@ def update_auth_user( email=row.email, is_admin=is_admin, has_write_access=has_write_access, + is_active=is_active, + role=role, created_at=getattr(row, 'created_at', None), updated_at=getattr(row, 'updated_at', None), ) @@ -329,27 +535,146 @@ def delete_auth_user( user_id: int, auth_db: Session = Depends(get_auth_db), ) -> Response: - """Delete an auth user - admin only.""" + """Delete an auth user - admin only. + + If the user has linked data (pending_photos, pending_identifications, + inappropriate_photo_reports), the user will be set to inactive instead + of deleted. Admins will be notified via logging. + """ try: - # Check if user exists - check_result = auth_db.execute(text(""" - SELECT id FROM users WHERE id = :user_id + # Check if user exists and get user info + user_result = auth_db.execute(text(""" + SELECT id, email, name FROM users WHERE id = :user_id """), {"user_id": user_id}) - if not check_result.first(): + user_row = user_result.first() + if not user_row: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail=f"Auth user with ID {user_id} not found", ) - # Delete user - auth_db.execute(text(""" - DELETE FROM users WHERE id = :user_id - """), {"user_id": user_id}) + user_email = user_row.email + user_name = user_row.name or user_email - auth_db.commit() + # Check for linked data in auth database + pending_photos_count = auth_db.execute(text(""" + SELECT COUNT(*) FROM pending_photos WHERE user_id = :user_id + """), {"user_id": user_id}).scalar() or 0 - return Response(status_code=status.HTTP_204_NO_CONTENT) + pending_identifications_count = auth_db.execute(text(""" + SELECT COUNT(*) FROM pending_identifications WHERE user_id = :user_id + """), {"user_id": user_id}).scalar() or 0 + + inappropriate_reports_count = auth_db.execute(text(""" + SELECT COUNT(*) FROM inappropriate_photo_reports WHERE user_id = :user_id + """), {"user_id": user_id}).scalar() or 0 + + has_linked_data = ( + pending_photos_count > 0 or + pending_identifications_count > 0 or + inappropriate_reports_count > 0 + ) + + if has_linked_data: + # Check if is_active column exists by trying to query it + dialect = auth_db.bind.dialect.name if auth_db.bind else "postgresql" + has_is_active_column = False + + try: + # Try to select is_active column to check if it exists + test_result = auth_db.execute(text(""" + SELECT is_active FROM users WHERE id = :user_id LIMIT 1 + """), {"user_id": user_id}) + test_result.first() + has_is_active_column = True + except Exception: + # Column doesn't exist - this should have been added at startup + # but if it wasn't, we can't proceed + error_msg = "is_active column does not exist in auth database users table" + logger.error( + f"Cannot deactivate auth user '{user_name}' (ID: {user_id}): {error_msg}" + ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=( + f"Cannot delete user '{user_name}' because they have linked data " + f"({pending_photos_count} pending photo(s), " + f"{pending_identifications_count} pending identification(s), " + f"{inappropriate_reports_count} inappropriate photo report(s)) " + f"and the is_active column does not exist in the auth database users table. " + f"Please restart the server to add the column automatically, or contact " + f"your database administrator to add it manually: " + f"ALTER TABLE users ADD COLUMN is_active BOOLEAN DEFAULT TRUE" + ), + ) + + # Set user inactive instead of deleting + if dialect == "postgresql": + auth_db.execute(text(""" + UPDATE users SET is_active = FALSE WHERE id = :user_id + """), {"user_id": user_id}) + else: + # SQLite uses 0 for FALSE + auth_db.execute(text(""" + UPDATE users SET is_active = 0 WHERE id = :user_id + """), {"user_id": user_id}) + auth_db.commit() + + # Notify admins via logging + logger.warning( + f"Auth user '{user_name}' (ID: {user_id}, email: {user_email}) was set to inactive " + f"instead of deleted because they have linked data: {pending_photos_count} pending " + f"photo(s), {pending_identifications_count} pending identification(s), " + f"{inappropriate_reports_count} inappropriate photo report(s). " + f"Action performed by admin: {current_admin['username']}", + extra={ + "user_id": user_id, + "user_email": user_email, + "user_name": user_name, + "pending_photos_count": pending_photos_count, + "pending_identifications_count": pending_identifications_count, + "inappropriate_reports_count": inappropriate_reports_count, + "admin_username": current_admin["username"], + } + ) + + # Return success but indicate user was deactivated + return JSONResponse( + status_code=status.HTTP_200_OK, + content={ + "message": ( + f"User '{user_name}' has been set to inactive because they have " + f"linked data ({pending_photos_count} pending photo(s), " + f"{pending_identifications_count} pending identification(s), " + f"{inappropriate_reports_count} inappropriate photo report(s))." + ), + "deactivated": True, + "pending_photos_count": pending_photos_count, + "pending_identifications_count": pending_identifications_count, + "inappropriate_reports_count": inappropriate_reports_count, + } + ) + else: + # No linked data - safe to delete + auth_db.execute(text(""" + DELETE FROM users WHERE id = :user_id + """), {"user_id": user_id}) + auth_db.commit() + + logger.info( + f"Auth user '{user_name}' (ID: {user_id}, email: {user_email}) was deleted. " + f"Action performed by admin: {current_admin['username']}", + extra={ + "user_id": user_id, + "user_email": user_email, + "user_name": user_name, + "admin_username": current_admin["username"], + } + ) + + return Response(status_code=status.HTTP_204_NO_CONTENT) + except HTTPException: raise except Exception as e: diff --git a/src/web/api/users.py b/src/web/api/users.py index 538cd06..f524d8e 100644 --- a/src/web/api/users.py +++ b/src/web/api/users.py @@ -6,6 +6,7 @@ import logging from typing import Annotated from fastapi import APIRouter, Depends, HTTPException, Query, Response, status +from fastapi.responses import JSONResponse from sqlalchemy import text from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session @@ -19,7 +20,7 @@ from src.web.constants.roles import ( is_admin_role, ) from src.web.db.session import get_auth_db, get_db -from src.web.db.models import User +from src.web.db.models import Face, PhotoFavorite, PhotoPersonLinkage, User from src.web.schemas.users import ( UserCreateRequest, UserResponse, @@ -442,6 +443,10 @@ def delete_user( ) -> Response: """Delete a user - admin only. + If the user has linked data (faces identified, video person linkages), + the user will be set to inactive instead of deleted, and favorites will + be removed. Admins will be notified via logging. + Prevents admin from deleting themselves. """ user = db.query(User).filter(User.id == user_id).first() @@ -458,21 +463,72 @@ def delete_user( detail="Cannot delete your own account", ) - try: + # Check for linked data (faces or photo_person_linkages identified by this user) + faces_count = db.query(Face).filter(Face.identified_by_user_id == user_id).count() + linkages_count = db.query(PhotoPersonLinkage).filter( + PhotoPersonLinkage.identified_by_user_id == user_id + ).count() + + has_linked_data = faces_count > 0 or linkages_count > 0 + + # Always delete favorites (they use username, not user_id) + favorites_deleted = db.query(PhotoFavorite).filter( + PhotoFavorite.username == user.username + ).delete() + + if has_linked_data: + # Set user inactive instead of deleting + user.is_active = False + db.add(user) + db.commit() + + # Notify admins via logging + logger.warning( + f"User '{user.username}' (ID: {user_id}) was set to inactive instead of deleted " + f"because they have linked data: {faces_count} face(s) and {linkages_count} " + f"video person linkage(s). {favorites_deleted} favorite(s) were deleted. " + f"Action performed by admin: {current_admin['username']}", + extra={ + "user_id": user_id, + "username": user.username, + "faces_count": faces_count, + "linkages_count": linkages_count, + "favorites_deleted": favorites_deleted, + "admin_username": current_admin["username"], + } + ) + + # Return success but indicate user was deactivated + return JSONResponse( + status_code=status.HTTP_200_OK, + content={ + "message": ( + f"User '{user.username}' has been set to inactive because they have " + f"linked data ({faces_count} face(s), {linkages_count} linkage(s)). " + f"{favorites_deleted} favorite(s) were deleted." + ), + "deactivated": True, + "faces_count": faces_count, + "linkages_count": linkages_count, + "favorites_deleted": favorites_deleted, + } + ) + else: + # No linked data - safe to delete db.delete(user) db.commit() - except IntegrityError as exc: - db.rollback() - constraint_name = "faces_identified_by_user_id_fkey" - if exc.orig and constraint_name in str(exc.orig): - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=( - "This user has identified faces and cannot be deleted. " - "Set the user inactive instead." - ), - ) from exc - raise - - return Response(status_code=status.HTTP_204_NO_CONTENT) + + logger.info( + f"User '{user.username}' (ID: {user_id}) was deleted. " + f"{favorites_deleted} favorite(s) were deleted. " + f"Action performed by admin: {current_admin['username']}", + extra={ + "user_id": user_id, + "username": user.username, + "favorites_deleted": favorites_deleted, + "admin_username": current_admin["username"], + } + ) + + return Response(status_code=status.HTTP_204_NO_CONTENT) diff --git a/src/web/app.py b/src/web/app.py index 824ee6a..f2abe6c 100644 --- a/src/web/app.py +++ b/src/web/app.py @@ -30,7 +30,7 @@ from src.web.api.version import router as version_router from src.web.settings import APP_TITLE, APP_VERSION from src.web.constants.roles import DEFAULT_ADMIN_ROLE, DEFAULT_USER_ROLE, ROLE_VALUES from src.web.db.base import Base, engine -from src.web.db.session import database_url +from src.web.db.session import auth_engine, database_url # Import models to ensure they're registered with Base.metadata from src.web.db import models # noqa: F401 from src.web.db.models import RolePermission @@ -475,6 +475,63 @@ def ensure_photo_person_linkage_table(inspector) -> None: print("✅ Created photo_person_linkage table") +def ensure_auth_user_is_active_column() -> None: + """Ensure auth database users table contains is_active column.""" + if auth_engine is None: + # Auth database not configured + return + + try: + from sqlalchemy import inspect as sqlalchemy_inspect + auth_inspector = sqlalchemy_inspect(auth_engine) + + if "users" not in auth_inspector.get_table_names(): + print("ℹ️ Auth database users table does not exist yet") + return + + columns = {column["name"] for column in auth_inspector.get_columns("users")} + if "is_active" in columns: + print("ℹ️ is_active column already exists in auth database users table") + return + + # Column doesn't exist - try to add it + print("🔄 Adding is_active column to auth database users table...") + + dialect = auth_engine.dialect.name + + try: + with auth_engine.connect() as connection: + with connection.begin(): + if dialect == "postgresql": + connection.execute( + text("ALTER TABLE users ADD COLUMN IF NOT EXISTS is_active BOOLEAN DEFAULT TRUE") + ) + else: + # SQLite + connection.execute( + text("ALTER TABLE users ADD COLUMN is_active BOOLEAN DEFAULT 1") + ) + print("✅ Added is_active column to auth database users table") + except Exception as alter_exc: + # Check if it's a permission error + error_str = str(alter_exc) + if "permission" in error_str.lower() or "insufficient" in error_str.lower() or "owner" in error_str.lower(): + print("⚠️ Cannot add is_active column: insufficient database privileges") + print(" The column will need to be added manually by a database administrator:") + if dialect == "postgresql": + print(" ALTER TABLE users ADD COLUMN is_active BOOLEAN DEFAULT TRUE;") + else: + print(" ALTER TABLE users ADD COLUMN is_active BOOLEAN DEFAULT 1;") + print(" Until then, users with linked data cannot be deleted.") + else: + # Some other error + print(f"⚠️ Failed to add is_active column to auth database users table: {alter_exc}") + except Exception as exc: + print(f"⚠️ Failed to check/add is_active column to auth database users table: {exc}") + # Don't raise - auth database might not be available or have permission issues + # The delete endpoint will handle this case gracefully + + def ensure_role_permissions_table(inspector) -> None: """Ensure the role_permissions table exists for permission matrix.""" if "role_permissions" in inspector.get_table_names(): @@ -528,6 +585,9 @@ async def lifespan(app: FastAPI): ensure_photo_person_linkage_table(inspector) ensure_face_excluded_column(inspector) ensure_role_permissions_table(inspector) + + # Ensure auth database schema + ensure_auth_user_is_active_column() except Exception as exc: print(f"❌ Database initialization failed: {exc}") raise diff --git a/src/web/schemas/auth_users.py b/src/web/schemas/auth_users.py index c7b0f36..148e25c 100644 --- a/src/web/schemas/auth_users.py +++ b/src/web/schemas/auth_users.py @@ -18,6 +18,8 @@ class AuthUserResponse(BaseModel): email: str is_admin: Optional[bool] = None has_write_access: Optional[bool] = None + is_active: Optional[bool] = None + role: Optional[str] = None created_at: Optional[datetime] = None updated_at: Optional[datetime] = None @@ -43,6 +45,8 @@ class AuthUserUpdateRequest(BaseModel): name: str = Field(..., min_length=1, max_length=200, description="Name (required)") is_admin: bool = Field(..., description="Admin role (required)") has_write_access: bool = Field(..., description="Write access (required)") + is_active: Optional[bool] = Field(None, description="Active status (optional)") + role: Optional[str] = Field(None, description="Role: 'Admin' or 'User' (optional)") class AuthUsersListResponse(BaseModel):