From af9d9ec8db5741abd1bb801dd176c3d7e622f5dd Mon Sep 17 00:00:00 2001 From: Alex <25013571+alexhb1@users.noreply.github.com> Date: Thu, 12 Feb 2026 17:47:10 +0000 Subject: [PATCH] Patch: Further multi-user fixes (#613) --- shelfmark/config/settings.py | 17 +- shelfmark/config/users_settings.py | 3 +- shelfmark/core/admin_routes.py | 28 +-- shelfmark/core/auth_modes.py | 25 +++ shelfmark/core/config.py | 1 - shelfmark/core/cwa_user_sync.py | 21 ++- shelfmark/download/orchestrator.py | 14 +- shelfmark/download/outputs/email.py | 5 +- shelfmark/main.py | 9 +- .../components/settings/SettingsContent.tsx | 11 +- .../src/components/settings/SettingsModal.tsx | 46 +++-- .../src/components/settings/UsersPanel.tsx | 163 +++++++++--------- .../settings/shared/SettingsSaveBar.tsx | 43 +++++ .../settings/shared/SettingsSubpage.tsx | 7 +- .../src/components/settings/shared/index.ts | 1 + .../components/settings/users/UserCard.tsx | 74 +++++--- .../settings/users/UserListView.tsx | 113 +++++------- .../settings/users/UserOverridesSection.tsx | 60 +++---- .../settings/users/UserOverridesView.tsx | 45 ++--- .../settings/users/settingsPayload.ts | 38 ++++ .../components/settings/users/useUserForm.ts | 23 ++- .../settings/users/useUserMutations.ts | 110 +++++++++--- .../settings/users/useUsersFetch.ts | 11 +- tests/config/test_download_settings.py | 48 ++++++ tests/core/test_admin_users_api.py | 52 +++++- tests/core/test_config_user_overrides.py | 17 ++ tests/core/test_cwa_user_sync.py | 40 ++++- tests/core/test_oidc_integration.py | 22 +++ .../test_orchestrator_user_output_mode.py | 79 +++++++++ tests/e2e/test_proxy_auth_middleware.py | 32 ++++ 30 files changed, 833 insertions(+), 325 deletions(-) create mode 100644 src/frontend/src/components/settings/shared/SettingsSaveBar.tsx create mode 100644 src/frontend/src/components/settings/users/settingsPayload.ts create mode 100644 tests/config/test_download_settings.py diff --git a/shelfmark/config/settings.py b/shelfmark/config/settings.py index 6379090c..cec11e46 100644 --- a/shelfmark/config/settings.py +++ b/shelfmark/config/settings.py @@ -636,14 +636,8 @@ def _on_save_downloads(values: dict[str, Any]) -> dict[str, Any]: # Preferred model: single recipient for global default and per-user override. raw_recipient = str(effective.get("EMAIL_RECIPIENT", "") or "").strip() - if not raw_recipient: - return { - "error": True, - "message": "Email recipient is required (Downloads -> Books -> Email Recipient).", - "values": values, - } - - if not _is_plain_email_address(raw_recipient): + # Optional global fallback: validate only when a default recipient is provided. + if raw_recipient and not _is_plain_email_address(raw_recipient): return { "error": True, "message": "Email recipient must be a valid plain email address.", @@ -909,8 +903,8 @@ def download_settings(): ), TextField( key="EMAIL_RECIPIENT", - label="Email Recipient", - description="Email address that should receive downloaded books when Output Mode is Email.", + label="Default Email Recipient", + description="Optional fallback email address when no per-user email recipient override is configured.", placeholder="reader@example.com", user_overridable=True, show_when={"field": "BOOKS_OUTPUT_MODE", "value": "email"}, @@ -1017,8 +1011,7 @@ def download_settings(): TextField( key="DESTINATION_AUDIOBOOK", label="Destination", - description="Leave empty to use Books destination. Use {User} for per-user folders (e.g. /audiobooks/{User}).", - placeholder="/audiobooks", + description="Directory where downloaded audiobook files are saved. Leave empty to use the Books destination.", user_overridable=True, universal_only=True, ), diff --git a/shelfmark/config/users_settings.py b/shelfmark/config/users_settings.py index a8f5dd17..5b2b5c22 100644 --- a/shelfmark/config/users_settings.py +++ b/shelfmark/config/users_settings.py @@ -25,7 +25,8 @@ def users_settings(): label="Restrict Settings and Onboarding to Admins", description=( "When enabled, only admin users can access Settings and Onboarding. " - "When disabled, any authenticated user can access them." + "When disabled, any authenticated user can access them. " + "Security and Users are always admin-only." ), default=True, env_supported=False, diff --git a/shelfmark/core/admin_routes.py b/shelfmark/core/admin_routes.py index a1b522e3..53581412 100644 --- a/shelfmark/core/admin_routes.py +++ b/shelfmark/core/admin_routes.py @@ -328,15 +328,9 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: "message": "Display name is managed by your identity provider.", }), 400 - # Prevent demoting the last admin - if role_changed and user_fields["role"] != "admin": - if user.get("role") == "admin": - other_admins = [ - u for u in user_db.list_users() - if u["role"] == "admin" and u["id"] != user_id - ] - if not other_admins: - return jsonify({"error": "Cannot remove admin role from the last admin account"}), 400 + # Allow demoting the last admin account. + # Auth mode resolution automatically falls back to "none" when no + # local password admin remains. # Avoid unnecessary writes for no-op field updates. for field in ("role", "email", "display_name"): @@ -401,7 +395,8 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: message = ( f"Synced {summary['total']} CWA users " - f"({summary['created']} created, {summary['updated']} updated)." + f"({summary['created']} created, {summary['updated']} updated, " + f"{summary.get('deleted', 0)} deleted)." ) logger.info(message) return jsonify({ @@ -429,20 +424,15 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: user.get("auth_source"), user.get("oidc_subject"), ) - if auth_source in {AUTH_SOURCE_PROXY, AUTH_SOURCE_CWA} and auth_source == auth_mode: + if auth_source == AUTH_SOURCE_CWA and auth_source == auth_mode: return jsonify({ "error": f"Cannot delete active {auth_source.upper()} users", "message": f"{auth_source.upper()} users are automatically re-provisioned on login.", }), 400 - # Prevent deleting the last local admin - if user.get("role") == "admin" and user.get("password_hash"): - local_admins = [ - u for u in user_db.list_users() - if u["role"] == "admin" and u.get("password_hash") and u["id"] != user_id - ] - if not local_admins: - return jsonify({"error": "Cannot delete the last local admin account"}), 400 + # Allow deleting the last local admin account. + # Auth mode resolution automatically falls back to "none" when no + # local password admin remains. user_db.delete_user(user_id) logger.info(f"Admin deleted user {user_id}: {user['username']}") diff --git a/shelfmark/core/auth_modes.py b/shelfmark/core/auth_modes.py index 586ffc1f..235fb2c0 100644 --- a/shelfmark/core/auth_modes.py +++ b/shelfmark/core/auth_modes.py @@ -14,6 +14,7 @@ AUTH_SOURCES = ( AUTH_SOURCE_CWA, ) AUTH_SOURCE_SET = frozenset(AUTH_SOURCES) +_ALWAYS_ADMIN_SETTINGS_TABS = frozenset({"security", "users"}) def has_local_password_admin(user_db: Any | None = None) -> bool: @@ -82,6 +83,18 @@ def is_settings_or_onboarding_path(path: str) -> bool: return path.startswith("/api/settings") or path.startswith("/api/onboarding") +def get_settings_tab_from_path(path: str) -> str | None: + """Extract tab name from /api/settings/[...] paths.""" + if not path.startswith("/api/settings/"): + return None + + suffix = path[len("/api/settings/"):] + if not suffix: + return None + + return suffix.split("/", 1)[0] or None + + def should_restrict_settings_to_admin( users_config: Mapping[str, Any], ) -> bool: @@ -89,6 +102,18 @@ def should_restrict_settings_to_admin( return bool(users_config.get("RESTRICT_SETTINGS_TO_ADMIN", True)) +def requires_admin_for_settings_access( + path: str, + users_config: Mapping[str, Any], +) -> bool: + """Return whether this settings/onboarding request requires admin privileges.""" + tab_name = get_settings_tab_from_path(path) + if tab_name in _ALWAYS_ADMIN_SETTINGS_TABS: + return True + + return should_restrict_settings_to_admin(users_config) + + def get_auth_check_admin_status( _auth_mode: str, users_config: Mapping[str, Any], diff --git a/shelfmark/core/config.py b/shelfmark/core/config.py index 387bea98..44e0a5a6 100644 --- a/shelfmark/core/config.py +++ b/shelfmark/core/config.py @@ -48,7 +48,6 @@ class Config: _instance: Optional['Config'] = None _lock = Lock() - def __new__(cls) -> 'Config': if cls._instance is None: with cls._lock: diff --git a/shelfmark/core/cwa_user_sync.py b/shelfmark/core/cwa_user_sync.py index c4fa853b..ed71aba3 100644 --- a/shelfmark/core/cwa_user_sync.py +++ b/shelfmark/core/cwa_user_sync.py @@ -4,6 +4,7 @@ from __future__ import annotations from typing import Any, Iterable +from shelfmark.core.auth_modes import AUTH_SOURCE_CWA, normalize_auth_source from shelfmark.core.external_user_linking import upsert_external_user from shelfmark.core.user_db import UserDB @@ -50,26 +51,44 @@ def sync_cwa_users_from_rows( """Sync CWA users from raw `(name, role_flags, email)` rows.""" created = 0 updated = 0 + active_cwa_user_ids: set[int] = set() for username, role_flags, email in rows: normalized_username = str(username or "").strip() if not normalized_username: continue role = "admin" if (int(role_flags or 0) & 1) == 1 else "user" - _, action = upsert_cwa_user( + user, action = upsert_cwa_user( user_db, cwa_username=normalized_username, cwa_email=_normalize_email(email), role=role, context="cwa_manual_sync", ) + active_cwa_user_ids.add(int(user["id"])) if action == "created": created += 1 else: updated += 1 + deleted = 0 + for existing_user in user_db.list_users(): + if normalize_auth_source( + existing_user.get("auth_source"), + existing_user.get("oidc_subject"), + ) != AUTH_SOURCE_CWA: + continue + + existing_id = int(existing_user.get("id") or 0) + if existing_id in active_cwa_user_ids: + continue + + user_db.delete_user(existing_id) + deleted += 1 + return { "created": created, "updated": updated, + "deleted": deleted, "total": created + updated, } diff --git a/shelfmark/download/orchestrator.py b/shelfmark/download/orchestrator.py index c19710af..09d5b452 100644 --- a/shelfmark/download/orchestrator.py +++ b/shelfmark/download/orchestrator.py @@ -95,7 +95,7 @@ def _resolve_email_destination( return configured_recipient, None return None, "Configured email recipient is invalid" - return None, "Email recipient is required" + return None, None def queue_book( @@ -126,10 +126,8 @@ def queue_book( email_to, email_error = _resolve_email_destination(user_id=user_id) if email_error: return False, email_error - if not email_to: - return False, "Email recipient is required" - - output_args = {"to": email_to} + if email_to: + output_args = {"to": email_to} # Create a source-agnostic download task task = DownloadTask( @@ -204,10 +202,8 @@ def queue_release( email_to, email_error = _resolve_email_destination(user_id=user_id) if email_error: return False, email_error - if not email_to: - return False, "Email recipient is required" - - output_args = {"to": email_to} + if email_to: + output_args = {"to": email_to} # Create a source-agnostic download task from release data task = DownloadTask( diff --git a/shelfmark/download/outputs/email.py b/shelfmark/download/outputs/email.py index cfeae274..6a4d3f1d 100644 --- a/shelfmark/download/outputs/email.py +++ b/shelfmark/download/outputs/email.py @@ -296,7 +296,10 @@ def _post_process_email( recipient = str(output_args.get("to", "") or "").strip() label = str(output_args.get("label", "") or "").strip() or recipient if not recipient: - status_callback("error", "No email recipient selected") + status_callback( + "error", + "No email recipient configured. Set a per-user email recipient or a default in Downloads -> Books.", + ) return None status_callback("resolving", "Preparing email") diff --git a/shelfmark/main.py b/shelfmark/main.py index 6f9a798e..295807d3 100644 --- a/shelfmark/main.py +++ b/shelfmark/main.py @@ -32,7 +32,7 @@ from shelfmark.core.auth_modes import ( get_auth_check_admin_status, has_local_password_admin, is_settings_or_onboarding_path, - should_restrict_settings_to_admin, + requires_admin_for_settings_access, ) from shelfmark.core.cwa_user_sync import upsert_cwa_user from shelfmark.core.external_user_linking import upsert_external_user @@ -403,9 +403,10 @@ def login_required(f): try: users_config = load_config_file("users") - restrict_to_admin = should_restrict_settings_to_admin(users_config) - - if restrict_to_admin and not session.get('is_admin', False): + if ( + requires_admin_for_settings_access(request.path, users_config) + and not session.get('is_admin', False) + ): return jsonify({"error": "Admin access required"}), 403 except Exception as e: diff --git a/src/frontend/src/components/settings/SettingsContent.tsx b/src/frontend/src/components/settings/SettingsContent.tsx index bffe3383..d3c3765c 100644 --- a/src/frontend/src/components/settings/SettingsContent.tsx +++ b/src/frontend/src/components/settings/SettingsContent.tsx @@ -17,7 +17,7 @@ import { ShowWhenCondition, TableFieldConfig, } from '../../types/settings'; -import { FieldWrapper } from './shared'; +import { FieldWrapper, SettingsSaveBar } from './shared'; import { TextField, PasswordField, @@ -351,14 +351,7 @@ export const SettingsContent = ({ {/* Save button - only visible when there are changes */} - {hasChanges && ( -
- {saveButton} -
- )} + {hasChanges && } ); }; diff --git a/src/frontend/src/components/settings/SettingsModal.tsx b/src/frontend/src/components/settings/SettingsModal.tsx index 21ecf88f..5888469d 100644 --- a/src/frontend/src/components/settings/SettingsModal.tsx +++ b/src/frontend/src/components/settings/SettingsModal.tsx @@ -1,7 +1,7 @@ import { useEffect, useState, useCallback, useRef, useMemo } from 'react'; import { useSettings } from '../../hooks/useSettings'; import { useSearchMode } from '../../contexts/SearchModeContext'; -import { getAdminSettingsOverridesSummary } from '../../services/api'; +import { getAdminSettingsOverridesSummary, getSettingsTab } from '../../services/api'; import { SettingsHeader } from './SettingsHeader'; import { SettingsSidebar } from './SettingsSidebar'; import { SettingsContent } from './SettingsContent'; @@ -37,7 +37,7 @@ export const SettingsModal = ({ isOpen, authMode, onClose, onShowToast, onSettin const [isMobile, setIsMobile] = useState(false); const [showMobileDetail, setShowMobileDetail] = useState(false); const [isClosing, setIsClosing] = useState(false); - const [usersSubpageState, setUsersSubpageState] = useState<{ title: string; onBack: () => void } | null>(null); + const [securityAccessError, setSecurityAccessError] = useState(null); const [tabOverrideSummaries, setTabOverrideSummaries] = useState< Record }>> >({}); @@ -97,16 +97,36 @@ export const SettingsModal = ({ isOpen, authMode, onClose, onShowToast, onSettin if (isOpen) { setShowMobileDetail(false); setIsClosing(false); - setUsersSubpageState(null); setTabOverrideSummaries({}); } }, [isOpen]); useEffect(() => { - if (selectedTab !== 'users') { - setUsersSubpageState(null); + if (!isOpen || selectedTab !== 'security') { + setSecurityAccessError(null); + return; } - }, [selectedTab]); + + let cancelled = false; + getSettingsTab("security") + .then(() => { + if (cancelled) return; + setSecurityAccessError(null); + }) + .catch((err) => { + if (cancelled) return; + const message = err instanceof Error ? err.message : 'Failed to load security settings'; + if (message.toLowerCase().includes('admin access required')) { + setSecurityAccessError(message); + return; + } + setSecurityAccessError(null); + }); + + return () => { + cancelled = true; + }; + }, [isOpen, selectedTab]); useEffect(() => { if (!isOpen || !selectedTab || selectedTab === 'users') { @@ -243,7 +263,6 @@ export const SettingsModal = ({ isOpen, authMode, onClose, onShowToast, onSettin const currentTab = tabs.find((t) => t.name === selectedTab); const currentTabDisplayName = currentTab?.displayName || 'Settings'; - const usersHeaderTitle = usersSubpageState ? `Settings / ${usersSubpageState.title}` : null; const selectedAuthMethod = values.security?.AUTH_METHOD; const usersAuthMode = typeof selectedAuthMethod === 'string' ? selectedAuthMethod : authMode; const currentTabContent = currentTab @@ -258,8 +277,11 @@ export const SettingsModal = ({ isOpen, authMode, onClose, onShowToast, onSettin isSaving={isSaving} hasChanges={currentTabHasChanges} onShowToast={onShowToast} - onSubpageStateChange={setUsersSubpageState} /> + ) : (selectedTab === 'security' && securityAccessError) ? ( +
+

{securityAccessError}

+
) : ( {currentTabContent} @@ -416,9 +438,7 @@ export const SettingsModal = ({ isOpen, authMode, onClose, onShowToast, onSettin aria-label="Settings" > diff --git a/src/frontend/src/components/settings/UsersPanel.tsx b/src/frontend/src/components/settings/UsersPanel.tsx index 7283b77d..a33f0622 100644 --- a/src/frontend/src/components/settings/UsersPanel.tsx +++ b/src/frontend/src/components/settings/UsersPanel.tsx @@ -11,7 +11,7 @@ import { useUsersPanelState, } from './users'; import { SettingsContent } from './SettingsContent'; -import { SettingsSubpage } from './shared'; +import { SettingsSaveBar } from './shared'; interface UsersPanelProps { authMode: string; @@ -23,7 +23,6 @@ interface UsersPanelProps { isSaving: boolean; hasChanges: boolean; onShowToast?: (message: string, type: 'success' | 'error' | 'info') => void; - onSubpageStateChange?: (state: { title: string; onBack: () => void } | null) => void; } export const UsersPanel = ({ @@ -36,7 +35,6 @@ export const UsersPanel = ({ isSaving, hasChanges, onShowToast, - onSubpageStateChange, }: UsersPanelProps) => { const { route, openCreate, openEdit, openEditOverrides, backToList } = useUsersPanelState(); @@ -63,6 +61,7 @@ export const UsersPanel = ({ isUserOverridable, userSettings, setUserSettings, + hasUserSettingsChanges, beginEditing, applyUserEditContext, resetEditContext, @@ -82,6 +81,7 @@ export const UsersPanel = ({ } = useUserMutations({ onShowToast, fetchUsers, + users, createForm, resetCreateForm, editingUser, @@ -145,36 +145,25 @@ export const UsersPanel = ({ backToList(); }; - useEffect(() => { - if (!onSubpageStateChange) { - return undefined; - } - - if (route.kind === 'edit-overrides') { - const username = editingUser && editingUser.id === route.userId - ? editingUser.username - : 'User'; - onSubpageStateChange({ - title: `Users / User Preferences: ${username}`, - onBack: handleBackToEdit, - }); - } else { - onSubpageStateChange(null); - } - - return () => { - onSubpageStateChange(null); - }; - }, [editingUser, handleBackToEdit, onSubpageStateChange, route]); - useEffect(() => { if (route.kind === 'create' && !canCreateLocalUsers) { backToList(); } }, [backToList, canCreateLocalUsers, route.kind]); - const handleSave = async () => { - const ok = await saveEditedUser(); + const handleSaveUserEdit = async () => { + const ok = await saveEditedUser({ includeSettings: false }); + if (ok) { + backToList(); + } + }; + + const handleSaveUserOverrides = async () => { + const ok = await saveEditedUser({ + includeProfile: false, + includePassword: false, + includeSettings: true, + }); if (ok) { backToList(); } @@ -213,65 +202,79 @@ export const UsersPanel = ({ } return ( - setUserSettings(updater)} - /> +
+ setUserSettings(updater)} + /> + + {hasUserSettingsChanges && ( + + )} +
); } return ( - -
- - -
- +
+
+ + +
+ +
- + + {hasChanges && ( + + )} +
); }; diff --git a/src/frontend/src/components/settings/shared/SettingsSaveBar.tsx b/src/frontend/src/components/settings/shared/SettingsSaveBar.tsx new file mode 100644 index 00000000..c9f93caa --- /dev/null +++ b/src/frontend/src/components/settings/shared/SettingsSaveBar.tsx @@ -0,0 +1,43 @@ +interface SettingsSaveBarProps { + onSave: () => void | Promise; + isSaving: boolean; +} + +export const SettingsSaveBar = ({ onSave, isSaving }: SettingsSaveBarProps) => ( +
+ +
+); diff --git a/src/frontend/src/components/settings/shared/SettingsSubpage.tsx b/src/frontend/src/components/settings/shared/SettingsSubpage.tsx index 5aa81221..e37276a2 100644 --- a/src/frontend/src/components/settings/shared/SettingsSubpage.tsx +++ b/src/frontend/src/components/settings/shared/SettingsSubpage.tsx @@ -2,13 +2,18 @@ import { ReactNode } from 'react'; interface SettingsSubpageProps { children: ReactNode; + hasBottomSaveBar?: boolean; } export const SettingsSubpage = ({ children, + hasBottomSaveBar = false, }: SettingsSubpageProps) => { return ( -
+
{children}
); diff --git a/src/frontend/src/components/settings/shared/index.ts b/src/frontend/src/components/settings/shared/index.ts index 1bb22f48..0ee53043 100644 --- a/src/frontend/src/components/settings/shared/index.ts +++ b/src/frontend/src/components/settings/shared/index.ts @@ -1,3 +1,4 @@ export { EnvLockBadge } from './EnvLockBadge'; export { FieldWrapper } from './FieldWrapper'; +export { SettingsSaveBar } from './SettingsSaveBar'; export { SettingsSubpage } from './SettingsSubpage'; diff --git a/src/frontend/src/components/settings/users/UserCard.tsx b/src/frontend/src/components/settings/users/UserCard.tsx index 1bb08266..d9bc5262 100644 --- a/src/frontend/src/components/settings/users/UserCard.tsx +++ b/src/frontend/src/components/settings/users/UserCard.tsx @@ -165,7 +165,11 @@ interface UserEditFieldsProps { editPasswordConfirm: string; onEditPasswordConfirmChange: (value: string) => void; downloadDefaults: DownloadDefaults | null; - onEditOverrides?: () => void; + onDelete?: () => void; + onConfirmDelete?: () => void; + onCancelDelete?: () => void; + isDeletePending?: boolean; + deleting?: boolean; } export const UserEditFields = ({ @@ -179,7 +183,11 @@ export const UserEditFields = ({ editPasswordConfirm, onEditPasswordConfirmChange, downloadDefaults, - onEditOverrides, + onDelete, + onConfirmDelete, + onCancelDelete, + isDeletePending = false, + deleting = false, }: UserEditFieldsProps) => { const capabilities = user.edit_capabilities; const { authSource, canSetPassword, canEditRole, canEditEmail, canEditDisplayName } = capabilities; @@ -246,29 +254,53 @@ export const UserEditFields = ({ )} -
- - - {onEditOverrides && ( +
+
+ +
+ {onDelete && ( +
+ {isDeletePending ? ( + <> + + + + ) : ( + + )} +
)}
diff --git a/src/frontend/src/components/settings/users/UserListView.tsx b/src/frontend/src/components/settings/users/UserListView.tsx index 7b028142..0a387c6d 100644 --- a/src/frontend/src/components/settings/users/UserListView.tsx +++ b/src/frontend/src/components/settings/users/UserListView.tsx @@ -74,6 +74,7 @@ export const UserListView = ({ const [confirmDelete, setConfirmDelete] = useState(null); const canCreateLocalUsers = canCreateLocalUsersForAuthMode(authMode); const isCwaMode = String(authMode || 'none').toLowerCase() === 'cwa'; + const toggleButtonClasses = 'p-2 rounded-full hover-action transition-colors text-gray-500 hover:text-gray-900 dark:text-gray-400 dark:hover:text-gray-100'; const usersHeading: HeadingFieldConfig = { key: 'users_heading', type: 'HeadingField', @@ -113,7 +114,9 @@ export const UserListView = ({ key={user.id} className={`rounded-lg border border-[var(--border-muted)] bg-[var(--bg-soft)] transition-colors ${active ? '' : 'opacity-60'}`} > -
+
- {!isEditingRow && ( - <> - - - {confirmDelete === user.id ? ( -
- - -
- ) : ( - - )} - + {isEditingRow && ( + )} + +
@@ -233,7 +208,11 @@ export const UserListView = ({ editPasswordConfirm={editPasswordConfirm} onEditPasswordConfirmChange={onEditPasswordConfirmChange} downloadDefaults={downloadDefaults} - onEditOverrides={onOpenOverrides} + onDelete={() => setConfirmDelete(user.id)} + onConfirmDelete={() => handleDelete(user.id)} + onCancelDelete={() => setConfirmDelete(null)} + isDeletePending={confirmDelete === user.id} + deleting={deletingUserId === user.id} /> ) : (
Loading user details...
diff --git a/src/frontend/src/components/settings/users/UserOverridesSection.tsx b/src/frontend/src/components/settings/users/UserOverridesSection.tsx index a96996cd..c1d92e30 100644 --- a/src/frontend/src/components/settings/users/UserOverridesSection.tsx +++ b/src/frontend/src/components/settings/users/UserOverridesSection.tsx @@ -44,9 +44,8 @@ const fallbackDestinationAudiobookField: TextFieldConfig = { type: 'TextField', key: 'DESTINATION_AUDIOBOOK', label: 'Destination', - description: 'Directory where downloaded audiobook files are saved. Use {User} for per-user folders and leave empty to use the books destination.', + description: 'Directory for this user\'s audiobook downloads.', value: '', - placeholder: '/audiobooks', }; const fallbackBookloreLibraryField: SelectFieldConfig = { @@ -141,7 +140,7 @@ const audiobooksHeading: HeadingFieldConfig = { type: 'HeadingField', key: 'delivery_preferences_audiobooks_heading', title: 'Audiobooks', - description: 'Audiobooks always use folder output. Set a user-specific destination or leave empty to inherit books.', + description: 'Audiobooks always use folder output. Use Reset to inherit the global audiobook destination.', }; const BOOK_PREFERENCE_KEYS: DeliverySettingKey[] = [ @@ -169,12 +168,26 @@ export const UserOverridesSection = ({ const destinationAudiobookField = getFieldByKey(fields, 'DESTINATION_AUDIOBOOK', fallbackDestinationAudiobookField); const bookloreLibraryField = getFieldByKey(fields, 'BOOKLORE_LIBRARY_ID', fallbackBookloreLibraryField); const booklorePathField = getFieldByKey(fields, 'BOOKLORE_PATH_ID', fallbackBooklorePathField); - const emailRecipientField = getFieldByKey(fields, 'EMAIL_RECIPIENT', fallbackEmailRecipientField); + const emailRecipientFieldSource = getFieldByKey(fields, 'EMAIL_RECIPIENT', fallbackEmailRecipientField); + const emailRecipientField: TextFieldConfig = { + ...emailRecipientFieldSource, + label: 'Email Recipient', + description: 'Email address used for this user in Email output mode.', + }; - const isOverridden = (key: DeliverySettingKey): boolean => - Object.prototype.hasOwnProperty.call(userSettings, key) && - userSettings[key] !== null && - userSettings[key] !== undefined; + const isOverridden = (key: DeliverySettingKey): boolean => { + if ( + !Object.prototype.hasOwnProperty.call(userSettings, key) || + userSettings[key] === null || + userSettings[key] === undefined + ) { + return false; + } + + const userValue = toStringValue(userSettings[key]); + const globalValue = toStringValue(globalValues[key as string]); + return userValue !== globalValue; + }; const resetKeys = (keys: DeliverySettingKey[]) => { setUserSettings((prev) => { @@ -242,24 +255,7 @@ export const UserOverridesSection = ({ { - setUserSettings((prev) => { - const next: PerUserSettings = { ...prev, BOOKS_OUTPUT_MODE: value }; - if (value === 'folder') { - delete next.BOOKLORE_LIBRARY_ID; - delete next.BOOKLORE_PATH_ID; - delete next.EMAIL_RECIPIENT; - } else if (value === 'booklore') { - delete next.DESTINATION; - delete next.EMAIL_RECIPIENT; - } else if (value === 'email') { - delete next.DESTINATION; - delete next.BOOKLORE_LIBRARY_ID; - delete next.BOOKLORE_PATH_ID; - } - return next; - }); - }} + onChange={(value) => setUserSettings((prev) => ({ ...prev, BOOKS_OUTPUT_MODE: value }))} disabled={Boolean(outputModeField.fromEnv)} /> @@ -370,12 +366,12 @@ export const UserOverridesSection = ({ ) : undefined } > - setUserSettings((prev) => ({ ...prev, DESTINATION_AUDIOBOOK: value }))} - disabled={Boolean(destinationAudiobookField.fromEnv)} - /> + setUserSettings((prev) => ({ ...prev, DESTINATION_AUDIOBOOK: value }))} + disabled={Boolean(destinationAudiobookField.fromEnv)} + /> )} diff --git a/src/frontend/src/components/settings/users/UserOverridesView.tsx b/src/frontend/src/components/settings/users/UserOverridesView.tsx index 4ba16e3a..c9a45134 100644 --- a/src/frontend/src/components/settings/users/UserOverridesView.tsx +++ b/src/frontend/src/components/settings/users/UserOverridesView.tsx @@ -4,8 +4,7 @@ import { SettingsSubpage } from '../shared'; import { UserOverridesSection } from './UserOverridesSection'; interface UserOverridesViewProps { - onSave: () => void; - saving: boolean; + hasChanges: boolean; onBack: () => void; deliveryPreferences: DeliveryPreferencesResponse | null; isUserOverridable: (key: keyof PerUserSettings) => boolean; @@ -14,39 +13,41 @@ interface UserOverridesViewProps { } export const UserOverridesView = ({ - onSave, - saving, + hasChanges, onBack, deliveryPreferences, isUserOverridable, userSettings, setUserSettings, }: UserOverridesViewProps) => ( - +
+
+ +
+ - -
- - -
); diff --git a/src/frontend/src/components/settings/users/settingsPayload.ts b/src/frontend/src/components/settings/users/settingsPayload.ts new file mode 100644 index 00000000..e4b238cd --- /dev/null +++ b/src/frontend/src/components/settings/users/settingsPayload.ts @@ -0,0 +1,38 @@ +import { DeliveryPreferencesResponse } from '../../../services/api'; +import { PerUserSettings } from './types'; + +const normalizeComparableValue = (value: unknown): string => { + if (value === null || value === undefined) { + return ''; + } + return String(value); +}; + +export const buildUserSettingsPayload = ( + userSettings: PerUserSettings, + userOverridableSettings: Set, + deliveryPreferences: DeliveryPreferencesResponse | null, +): Record => + (deliveryPreferences?.keys || [...userOverridableSettings]) + .map(String) + .sort() + .reduce>((payload, key) => { + const typedKey = key as keyof PerUserSettings; + const hasUserValue = Object.prototype.hasOwnProperty.call(userSettings, typedKey) + && userSettings[typedKey] !== null + && userSettings[typedKey] !== undefined; + + if (!hasUserValue) { + payload[key] = null; + return payload; + } + + const userValue = userSettings[typedKey]; + const globalValue = deliveryPreferences?.globalValues?.[key]; + const isDifferentFromGlobal = deliveryPreferences + ? normalizeComparableValue(userValue) !== normalizeComparableValue(globalValue) + : true; + + payload[key] = isDifferentFromGlobal ? userValue : null; + return payload; + }, {}); diff --git a/src/frontend/src/components/settings/users/useUserForm.ts b/src/frontend/src/components/settings/users/useUserForm.ts index 89e9d0b6..b4aefe2e 100644 --- a/src/frontend/src/components/settings/users/useUserForm.ts +++ b/src/frontend/src/components/settings/users/useUserForm.ts @@ -2,6 +2,19 @@ import { useState } from 'react'; import { AdminUser, DeliveryPreferencesResponse, DownloadDefaults } from '../../../services/api'; import { CreateUserFormState, INITIAL_CREATE_FORM, PerUserSettings } from './types'; import { UserEditContext } from './useUsersFetch'; +import { buildUserSettingsPayload } from './settingsPayload'; + +const normalizeUserSettings = (settings: PerUserSettings): PerUserSettings => { + const normalized: PerUserSettings = {}; + Object.keys(settings).sort().forEach((key) => { + const typedKey = key as keyof PerUserSettings; + const value = settings[typedKey]; + if (value !== null && value !== undefined) { + normalized[typedKey] = value; + } + }); + return normalized; +}; export const useUserForm = () => { const [createForm, setCreateForm] = useState({ ...INITIAL_CREATE_FORM }); @@ -11,6 +24,7 @@ export const useUserForm = () => { const [downloadDefaults, setDownloadDefaults] = useState(null); const [deliveryPreferences, setDeliveryPreferences] = useState(null); const [userSettings, setUserSettings] = useState({}); + const [originalUserSettings, setOriginalUserSettings] = useState({}); const [userOverridableSettings, setUserOverridableSettings] = useState>(new Set()); const resetCreateForm = () => setCreateForm({ ...INITIAL_CREATE_FORM }); @@ -19,6 +33,7 @@ export const useUserForm = () => { setDownloadDefaults(null); setDeliveryPreferences(null); setUserSettings({}); + setOriginalUserSettings({}); setUserOverridableSettings(new Set()); }; @@ -29,10 +44,12 @@ export const useUserForm = () => { }; const applyUserEditContext = (context: UserEditContext) => { + const normalizedSettings = normalizeUserSettings(context.userSettings); setEditingUser({ ...context.user }); setDownloadDefaults(context.downloadDefaults); setDeliveryPreferences(context.deliveryPreferences); - setUserSettings(context.userSettings); + setUserSettings(normalizedSettings); + setOriginalUserSettings(normalizedSettings); setUserOverridableSettings(new Set(context.userOverridableSettings)); }; @@ -44,6 +61,9 @@ export const useUserForm = () => { }; const isUserOverridable = (key: keyof PerUserSettings) => userOverridableSettings.has(String(key)); + const hasUserSettingsChanges = + JSON.stringify(buildUserSettingsPayload(userSettings, userOverridableSettings, deliveryPreferences)) + !== JSON.stringify(buildUserSettingsPayload(originalUserSettings, userOverridableSettings, deliveryPreferences)); return { createForm, @@ -63,6 +83,7 @@ export const useUserForm = () => { deliveryPreferences, userSettings, setUserSettings, + hasUserSettingsChanges, userOverridableSettings, isUserOverridable, }; diff --git a/src/frontend/src/components/settings/users/useUserMutations.ts b/src/frontend/src/components/settings/users/useUserMutations.ts index 4e207695..0b0db242 100644 --- a/src/frontend/src/components/settings/users/useUserMutations.ts +++ b/src/frontend/src/components/settings/users/useUserMutations.ts @@ -8,11 +8,13 @@ import { updateAdminUser, } from '../../../services/api'; import { CreateUserFormState, PerUserSettings } from './types'; +import { buildUserSettingsPayload } from './settingsPayload'; const MIN_PASSWORD_LENGTH = 4; interface UseUserMutationsParams { onShowToast?: (message: string, type: 'success' | 'error' | 'info') => void; - fetchUsers: () => Promise; + fetchUsers: () => Promise; + users: AdminUser[]; createForm: CreateUserFormState; resetCreateForm: () => void; editingUser: AdminUser | null; @@ -24,24 +26,32 @@ interface UseUserMutationsParams { onEditSaveSuccess?: () => void; } +interface SaveEditedUserOptions { + includeProfile?: boolean; + includePassword?: boolean; + includeSettings?: boolean; +} + const getPasswordError = (password: string, passwordConfirm: string) => { if (!password) return null; if (password.length < MIN_PASSWORD_LENGTH) return `Password must be at least ${MIN_PASSWORD_LENGTH} characters`; return password === passwordConfirm ? null : 'Passwords do not match'; }; -const buildSettingsPayload = (userSettings: PerUserSettings, userOverridableSettings: Set, deliveryPreferences: DeliveryPreferencesResponse | null) => - (deliveryPreferences?.keys || [...userOverridableSettings]).reduce>((payload, key) => { - const typedKey = key as keyof PerUserSettings; - payload[key] = Object.prototype.hasOwnProperty.call(userSettings, typedKey) && userSettings[typedKey] !== null && userSettings[typedKey] !== undefined - ? (userSettings[typedKey] ?? '') - : null; - return payload; - }, {}); +const countLocalPasswordAdmins = (users: AdminUser[]): number => + users.filter((user) => user.auth_source === 'builtin' && user.role === 'admin').length; + +const authSourceLabel: Record = { + builtin: 'Local', + oidc: 'OIDC', + proxy: 'Proxy', + cwa: 'CWA', +}; export const useUserMutations = ({ onShowToast, fetchUsers, + users, createForm, resetCreateForm, editingUser, @@ -82,43 +92,91 @@ export const useUserMutations = ({ } }; - const saveEditedUser = async () => { + const saveEditedUser = async ({ + includeProfile = true, + includePassword = true, + includeSettings = true, + }: SaveEditedUserOptions = {}) => { if (!editingUser) return false; - const passwordError = getPasswordError(editPassword, editPasswordConfirm); + const passwordError = includePassword ? getPasswordError(editPassword, editPasswordConfirm) : null; if (passwordError) return fail(passwordError); + const localAdminsBeforeSave = includeProfile ? countLocalPasswordAdmins(users) : 0; const caps = editingUser.edit_capabilities; - const settingsPayload = buildSettingsPayload(userSettings, userOverridableSettings, deliveryPreferences); + const settingsPayload = includeSettings + ? buildUserSettingsPayload(userSettings, userOverridableSettings, deliveryPreferences) + : null; + const updatePayload: Partial> & { + password?: string; + settings?: Record; + } = {}; + + if (includeProfile) { + if (caps.canEditEmail) { + updatePayload.email = editingUser.email; + } + if (caps.canEditDisplayName) { + updatePayload.display_name = editingUser.display_name; + } + if (caps.canEditRole) { + updatePayload.role = editingUser.role; + } + } + if (includePassword && caps.canSetPassword && editPassword) { + updatePayload.password = editPassword; + } + if (settingsPayload && Object.keys(settingsPayload).length > 0) { + updatePayload.settings = settingsPayload; + } setSaving(true); try { - await updateAdminUser(editingUser.id, { - ...(caps.canEditEmail ? { email: editingUser.email } : {}), - ...(caps.canEditDisplayName ? { display_name: editingUser.display_name } : {}), - ...(caps.canEditRole ? { role: editingUser.role } : {}), - ...(caps.canSetPassword && editPassword ? { password: editPassword } : {}), - ...(Object.keys(settingsPayload).length > 0 ? { settings: settingsPayload } : {}), - }); + await updateAdminUser(editingUser.id, updatePayload); onEditSaveSuccess?.(); - onShowToast?.('User updated', 'success'); - await fetchUsers(); + onShowToast?.( + includeSettings && !includeProfile && !includePassword ? 'User preferences updated' : 'User updated', + 'success', + ); + const refreshedUsers = await fetchUsers(); + if (includeProfile && localAdminsBeforeSave > 0 && countLocalPasswordAdmins(refreshedUsers) === 0) { + onShowToast?.( + "No local admin accounts remain. Authentication will fall back to 'No Authentication' until a local admin is created.", + 'info', + ); + } return true; - } catch { - return fail('Failed to update user'); + } catch (err) { + const message = err instanceof Error ? err.message : 'Failed to update user'; + return fail(`Failed to update user: ${message}`); } finally { setSaving(false); } }; const deleteUser = async (userId: number) => { + const deletedUser = users.find((user) => user.id === userId) || null; + const localAdminsBeforeDelete = countLocalPasswordAdmins(users); setDeletingUserId(userId); try { await deleteAdminUser(userId); onShowToast?.('User deleted', 'success'); - await fetchUsers(); + const refreshedUsers = await fetchUsers(); + if (deletedUser && deletedUser.auth_source !== 'builtin') { + onShowToast?.( + `${authSourceLabel[deletedUser.auth_source]} users may be re-provisioned by your authentication source on a future login or sync.`, + 'info', + ); + } + if (localAdminsBeforeDelete > 0 && countLocalPasswordAdmins(refreshedUsers) === 0) { + onShowToast?.( + "No local admin accounts remain. Authentication will fall back to 'No Authentication' until a local admin is created.", + 'info', + ); + } return true; - } catch { - return fail('Failed to delete user'); + } catch (err) { + const message = err instanceof Error ? err.message : 'Failed to delete user'; + return fail(`Failed to delete user: ${message}`); } finally { setDeletingUserId(null); } diff --git a/src/frontend/src/components/settings/users/useUsersFetch.ts b/src/frontend/src/components/settings/users/useUsersFetch.ts index 6e2c3ced..c6dc6894 100644 --- a/src/frontend/src/components/settings/users/useUsersFetch.ts +++ b/src/frontend/src/components/settings/users/useUsersFetch.ts @@ -27,16 +27,23 @@ export const useUsersFetch = ({ onShowToast }: UseUsersFetchParams) => { const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(null); - const fetchUsers = useCallback(async () => { + const shouldSuppressAccessToast = (message: string): boolean => + message.toLowerCase().includes('admin access required'); + + const fetchUsers = useCallback(async (): Promise => { try { setLoading(true); setLoadError(null); const data = await getAdminUsers(); setUsers(data); + return data; } catch (err) { const message = err instanceof Error ? err.message : 'Failed to load users'; setLoadError(message); - onShowToast?.(message, 'error'); + if (!shouldSuppressAccessToast(message)) { + onShowToast?.(message, 'error'); + } + return []; } finally { setLoading(false); } diff --git a/tests/config/test_download_settings.py b/tests/config/test_download_settings.py new file mode 100644 index 00000000..9c3421dd --- /dev/null +++ b/tests/config/test_download_settings.py @@ -0,0 +1,48 @@ +def _base_email_mode_values() -> dict[str, object]: + return { + "BOOKS_OUTPUT_MODE": "email", + "EMAIL_SMTP_HOST": "smtp.example.com", + "EMAIL_FROM": "Shelfmark ", + } + + +def test_on_save_downloads_allows_empty_default_email_recipient(monkeypatch): + from shelfmark.config.settings import _on_save_downloads + + monkeypatch.setattr("shelfmark.config.settings.load_config_file", lambda _tab: {}) + + values = { + **_base_email_mode_values(), + "EMAIL_RECIPIENT": "", + } + + result = _on_save_downloads(values) + + assert result["error"] is False + assert result["values"]["EMAIL_RECIPIENT"] == "" + + +def test_on_save_downloads_validates_default_email_recipient_format(monkeypatch): + from shelfmark.config.settings import _on_save_downloads + + monkeypatch.setattr("shelfmark.config.settings.load_config_file", lambda _tab: {}) + + values = { + **_base_email_mode_values(), + "EMAIL_RECIPIENT": "Reader ", + } + + result = _on_save_downloads(values) + + assert result["error"] is True + assert "valid plain email address" in result["message"] + + +def test_download_settings_email_recipient_field_uses_default_label(): + from shelfmark.config.settings import download_settings + + fields = download_settings() + email_field = next(field for field in fields if getattr(field, "key", None) == "EMAIL_RECIPIENT") + + assert email_field.label == "Default Email Recipient" + assert "Optional fallback" in email_field.description diff --git a/tests/core/test_admin_users_api.py b/tests/core/test_admin_users_api.py index 16dc4031..cefafa24 100644 --- a/tests/core/test_admin_users_api.py +++ b/tests/core/test_admin_users_api.py @@ -420,6 +420,23 @@ class TestAdminUserUpdateEndpoint: updated = user_db.get_user(user_id=user["id"]) assert updated["role"] == "admin" + def test_demote_last_admin_allowed(self, admin_client, user_db): + user = user_db.create_user( + username="onlyadmin", + role="admin", + password_hash="hashed_pw", + ) + + resp = admin_client.put( + f"/api/admin/users/{user['id']}", + json={"role": "user"}, + ) + + assert resp.status_code == 200 + updated = user_db.get_user(user_id=user["id"]) + assert updated is not None + assert updated["role"] == "user" + def test_update_user_email(self, admin_client, user_db): user = user_db.create_user(username="alice") @@ -744,6 +761,12 @@ class TestAdminSyncCwaUsersEndpoint: role="admin", auth_source="builtin", ) + stale_cwa = user_db.create_user( + username="stale__cwa", + email="stale@example.com", + role="user", + auth_source="cwa", + ) with patch("shelfmark.core.admin_routes._get_auth_mode", return_value="cwa"): with patch("shelfmark.core.admin_routes.CWA_DB_PATH", cwa_db_path): @@ -753,6 +776,7 @@ class TestAdminSyncCwaUsersEndpoint: assert resp.json["success"] is True assert resp.json["created"] == 1 assert resp.json["updated"] == 1 + assert resp.json["deleted"] == 1 assert resp.json["total"] == 2 alice_linked = user_db.get_user(user_id=local_email_match["id"]) @@ -775,6 +799,7 @@ class TestAdminSyncCwaUsersEndpoint: ) assert bob_cwa["username"].startswith("bob__cwa") assert bob_cwa["role"] == "user" + assert user_db.get_user(user_id=stale_cwa["id"]) is None def test_sync_cwa_users_rejected_when_not_in_cwa_mode(self, admin_client): with patch("shelfmark.core.admin_routes._get_auth_mode", return_value="builtin"): @@ -1129,14 +1154,23 @@ class TestAdminUserDeleteEndpoint: assert len(resp.json) == 1 assert resp.json[0]["username"] == "bob" - def test_delete_active_proxy_user_rejected(self, admin_client, user_db): + def test_delete_active_proxy_user_allowed(self, admin_client, user_db): user = user_db.create_user(username="proxyuser", auth_source="proxy") with patch("shelfmark.core.admin_routes._get_auth_mode", return_value="proxy"): resp = admin_client.delete(f"/api/admin/users/{user['id']}") + assert resp.status_code == 200 + assert resp.json["success"] is True + + def test_delete_active_cwa_user_rejected(self, admin_client, user_db): + user = user_db.create_user(username="cwauser", auth_source="cwa") + + with patch("shelfmark.core.admin_routes._get_auth_mode", return_value="cwa"): + resp = admin_client.delete(f"/api/admin/users/{user['id']}") + assert resp.status_code == 400 - assert "Cannot delete active PROXY users" in resp.json["error"] + assert "Cannot delete active CWA users" in resp.json["error"] def test_delete_inactive_proxy_user_allowed(self, admin_client, user_db): user = user_db.create_user(username="proxyuser", auth_source="proxy") @@ -1160,6 +1194,20 @@ class TestAdminUserDeleteEndpoint: assert resp.status_code == 200 assert resp.json["success"] is True + def test_delete_last_local_admin_allowed(self, admin_client, user_db): + user = user_db.create_user( + username="onlyadmin", + password_hash="hashed_pw", + role="admin", + ) + + with patch("shelfmark.core.admin_routes._get_auth_mode", return_value="builtin"): + resp = admin_client.delete(f"/api/admin/users/{user['id']}") + + assert resp.status_code == 200 + assert resp.json["success"] is True + assert user_db.get_user(user_id=user["id"]) is None + # --------------------------------------------------------------------------- # OIDC lockout prevention (security on_save handler) diff --git a/tests/core/test_config_user_overrides.py b/tests/core/test_config_user_overrides.py index 9ab1e42c..c79b4937 100644 --- a/tests/core/test_config_user_overrides.py +++ b/tests/core/test_config_user_overrides.py @@ -60,3 +60,20 @@ def test_get_ignores_user_override_for_non_overridable_field(monkeypatch): ) assert config.get("FILE_ORGANIZATION", "rename", user_id=10) == "rename" + + +def test_get_respects_empty_user_override_for_destination_audiobook(monkeypatch): + monkeypatch.setattr(config, "_ensure_loaded", lambda: None) + monkeypatch.setattr(config, "_cache", {"DESTINATION_AUDIOBOOK": "/global/audiobooks"}) + monkeypatch.setattr( + config, + "_field_map", + {"DESTINATION_AUDIOBOOK": (_DummyField(env_supported=True, user_overridable=True), "downloads")}, + ) + monkeypatch.setattr(config, "_get_user_override", lambda user_id, key: "") + monkeypatch.setattr( + "shelfmark.core.config._get_registry", + lambda: SimpleNamespace(is_value_from_env=lambda field: False), + ) + + assert config.get("DESTINATION_AUDIOBOOK", "/default", user_id=10) == "" diff --git a/tests/core/test_cwa_user_sync.py b/tests/core/test_cwa_user_sync.py index 2ca8cfa4..1e7c24c9 100644 --- a/tests/core/test_cwa_user_sync.py +++ b/tests/core/test_cwa_user_sync.py @@ -5,7 +5,7 @@ import tempfile import pytest -from shelfmark.core.cwa_user_sync import upsert_cwa_user +from shelfmark.core.cwa_user_sync import sync_cwa_users_from_rows, upsert_cwa_user from shelfmark.core.user_db import UserDB @@ -93,3 +93,41 @@ def test_upsert_updates_existing_cwa_user_by_username_before_email(user_db): assert user["auth_source"] == "cwa" assert user["email"] == "new@example.com" assert user["role"] == "admin" + + +def test_sync_prunes_cwa_users_missing_from_source(user_db): + active_cwa = user_db.create_user( + username="active_cwa", + email="active@example.com", + role="user", + auth_source="cwa", + ) + stale_cwa = user_db.create_user( + username="stale_cwa", + email="stale@example.com", + role="admin", + auth_source="cwa", + ) + local_builtin = user_db.create_user( + username="local_user", + email="local@example.com", + role="admin", + auth_source="builtin", + ) + + summary = sync_cwa_users_from_rows( + user_db, + rows=[("active_cwa", 1, "active@example.com")], + ) + + assert summary["created"] == 0 + assert summary["updated"] == 1 + assert summary["deleted"] == 1 + assert summary["total"] == 1 + + active_after = user_db.get_user(user_id=active_cwa["id"]) + assert active_after is not None + assert active_after["role"] == "admin" + + assert user_db.get_user(user_id=stale_cwa["id"]) is None + assert user_db.get_user(user_id=local_builtin["id"]) is not None diff --git a/tests/core/test_oidc_integration.py b/tests/core/test_oidc_integration.py index e9ed53f5..f2168ca6 100644 --- a/tests/core/test_oidc_integration.py +++ b/tests/core/test_oidc_integration.py @@ -2,8 +2,10 @@ from shelfmark.core.auth_modes import ( determine_auth_mode, + get_settings_tab_from_path, get_auth_check_admin_status, is_settings_or_onboarding_path, + requires_admin_for_settings_access, should_restrict_settings_to_admin, ) @@ -72,6 +74,26 @@ class TestSettingsRestrictionPolicy: assert should_restrict_settings_to_admin({"RESTRICT_SETTINGS_TO_ADMIN": True}) is True assert should_restrict_settings_to_admin({"RESTRICT_SETTINGS_TO_ADMIN": False}) is False + def test_extracts_settings_tab_from_path(self): + assert get_settings_tab_from_path("/api/settings/security") == "security" + assert get_settings_tab_from_path("/api/settings/users/action/open_users_tab") == "users" + assert get_settings_tab_from_path("/api/settings") is None + + def test_security_and_users_tabs_always_require_admin(self): + users_config = {"RESTRICT_SETTINGS_TO_ADMIN": False} + assert requires_admin_for_settings_access("/api/settings/security", users_config) is True + assert requires_admin_for_settings_access("/api/settings/users", users_config) is True + + def test_other_tabs_follow_global_toggle(self): + assert requires_admin_for_settings_access( + "/api/settings/general", + {"RESTRICT_SETTINGS_TO_ADMIN": False}, + ) is False + assert requires_admin_for_settings_access( + "/api/settings/general", + {"RESTRICT_SETTINGS_TO_ADMIN": True}, + ) is True + class TestAuthCheckAdminStatus: def test_authenticated_admin_when_restricted(self): diff --git a/tests/download/test_orchestrator_user_output_mode.py b/tests/download/test_orchestrator_user_output_mode.py index 5dacb52b..ca623525 100644 --- a/tests/download/test_orchestrator_user_output_mode.py +++ b/tests/download/test_orchestrator_user_output_mode.py @@ -84,3 +84,82 @@ def test_queue_release_uses_user_specific_books_output_mode(monkeypatch): assert task.output_mode == "email" assert task.output_args == {"to": "alice@example.com"} assert ("BOOKS_OUTPUT_MODE", 42) in config_calls + + +def test_queue_book_email_mode_without_recipient_is_queued(monkeypatch): + import shelfmark.download.orchestrator as orchestrator + + captured: dict[str, object] = {} + + def fake_get_book_info(_book_id, fetch_download_count=False): + assert fetch_download_count is False + return SimpleNamespace( + title="Test Book", + author="Tester", + format="epub", + size="1 MB", + preview=None, + content="book (fiction)", + ) + + def fake_config_get(key, default=None, user_id=None): + if key == "BOOKS_OUTPUT_MODE": + return "email" if user_id == 42 else "folder" + if key == "EMAIL_RECIPIENT": + return "" + return default + + def fake_add(task): + captured["task"] = task + return True + + monkeypatch.setattr(orchestrator.direct_download, "get_book_info", fake_get_book_info) + monkeypatch.setattr(orchestrator.config, "get", fake_config_get) + monkeypatch.setattr(orchestrator.book_queue, "add", fake_add) + monkeypatch.setattr(orchestrator, "ws_manager", None) + + success, error = orchestrator.queue_book("book-1", user_id=42, username="alice") + + assert success is True + assert error is None + task = captured["task"] + assert task.output_mode == "email" + assert task.output_args == {} + + +def test_queue_release_email_mode_without_recipient_is_queued(monkeypatch): + import shelfmark.download.orchestrator as orchestrator + + captured: dict[str, object] = {} + + def fake_config_get(key, default=None, user_id=None): + if key == "BOOKS_OUTPUT_MODE": + return "email" if user_id == 42 else "folder" + if key == "EMAIL_RECIPIENT": + return "" + return default + + def fake_add(task): + captured["task"] = task + return True + + monkeypatch.setattr(orchestrator.config, "get", fake_config_get) + monkeypatch.setattr(orchestrator.book_queue, "add", fake_add) + monkeypatch.setattr(orchestrator, "ws_manager", None) + + release_data = { + "source": "direct_download", + "source_id": "release-1", + "title": "Release Title", + "content_type": "book (fiction)", + "format": "epub", + "size": "1 MB", + } + + success, error = orchestrator.queue_release(release_data, user_id=42, username="alice") + + assert success is True + assert error is None + task = captured["task"] + assert task.output_mode == "email" + assert task.output_args == {} diff --git a/tests/e2e/test_proxy_auth_middleware.py b/tests/e2e/test_proxy_auth_middleware.py index e9345127..14282aef 100644 --- a/tests/e2e/test_proxy_auth_middleware.py +++ b/tests/e2e/test_proxy_auth_middleware.py @@ -205,6 +205,38 @@ class TestLoginRequiredDecorator: assert resp[0]["success"] is True + def test_security_tab_always_blocks_non_admin_even_when_toggle_off(self, main_module, view): + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch( + "shelfmark.core.settings_registry.load_config_file", + return_value={"RESTRICT_SETTINGS_TO_ADMIN": False}, + ): + with main_module.app.test_request_context("/api/settings/security"): + main_module.session["user_id"] = "user" + main_module.session["is_admin"] = False + decorated = main_module.login_required(view) + resp = _as_response(decorated()) + data = resp.get_json() + + assert resp.status_code == 403 + assert "Admin access required" in (data.get("error") or "") + + def test_users_tab_always_blocks_non_admin_even_when_toggle_off(self, main_module, view): + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch( + "shelfmark.core.settings_registry.load_config_file", + return_value={"RESTRICT_SETTINGS_TO_ADMIN": False}, + ): + with main_module.app.test_request_context("/api/settings/users"): + main_module.session["user_id"] = "user" + main_module.session["is_admin"] = False + decorated = main_module.login_required(view) + resp = _as_response(decorated()) + data = resp.get_json() + + assert resp.status_code == 403 + assert "Admin access required" in (data.get("error") or "") + def test_proxy_admin_restriction_blocks_non_admin(self, main_module, view): with patch.object(main_module, "get_auth_mode", return_value="proxy"): with patch(