From ece6b8f341e2df8301900e6aa3f83f695cffeec3 Mon Sep 17 00:00:00 2001 From: CaliBrain Date: Fri, 25 Sep 2026 18:56:16 -0400 Subject: [PATCH] fix(auth): fail closed when auth prerequisites are missing (#1387) (#1397) Auth mode resolution fell back to "none" (anonymous full admin) whenever the configured method's prerequisites were missing: no local password admin for builtin/OIDC, no Calibre-Web database, a blank proxy header, an unrecognized AUTH_METHOD (including "OIDC" in uppercase), or any error while reading the config. Deleting or demoting the last local admin was allowed on purpose because of that fallback, which exposed OIDC instances publicly. - Only an explicit AUTH_METHOD=none disables authentication. A configured method stays active when its prerequisites are missing, so sign-in fails instead of opening up. - An unrecognized or unreadable AUTH_METHOD resolves to "unavailable", which still requires a session and accepts no login. Values are normalized, so AUTH_METHOD=OIDC works. - Restore the guard against deleting or demoting the last local password admin while builtin/OIDC is active (unless DISABLE_LOCAL_AUTH is set). - Require a local admin before enabling Local auth, as OIDC already did. - Log a recovery hint at startup when builtin/OIDC runs without a local admin, and document recovery via AUTH_METHOD=none. - Drop the "will fall back to No Authentication" UI toasts and hints. Fixes https://github.com/calibrain/shelfmark/issues/1387 --- docs/users-and-requests.md | 6 + shelfmark/config/security.py | 9 +- shelfmark/config/security_handlers.py | 4 + shelfmark/config/users_settings.py | 4 + shelfmark/core/admin_routes.py | 52 ++++++-- shelfmark/core/auth_modes.py | 115 +++++----------- shelfmark/core/self_user_routes.py | 3 +- shelfmark/main.py | 30 ++++- .../settings/users/useUserMutations.ts | 25 +--- tests/config/test_security.py | 29 +++- tests/core/test_admin_users_api.py | 82 ++++++++++-- tests/core/test_oidc_integration.py | 125 ++++++------------ tests/e2e/test_auth_endpoints.py | 43 +++--- 13 files changed, 290 insertions(+), 237 deletions(-) diff --git a/docs/users-and-requests.md b/docs/users-and-requests.md index c6a98170..6411c58b 100644 --- a/docs/users-and-requests.md +++ b/docs/users-and-requests.md @@ -26,6 +26,12 @@ User accounts are synced from your Calibre-Web `app.db`. If a local user with a Requires mounting your Calibre-Web `app.db` to `/auth/app.db`. +### If a method's requirements go missing + +Once a method is selected, Shelfmark keeps requiring sign-in even if that method's requirements go missing later (a deleted local admin, a missing `app.db`, incomplete OIDC settings, or an unrecognized `AUTH_METHOD` value). It never falls back to "No Authentication" on its own. While Local or OIDC authentication is active, the last local admin can't be deleted or demoted. + +If you're locked out, start the container once with `AUTH_METHOD=none`, create a local admin under **Settings → Users**, then remove the override. Keep Shelfmark off the public internet while the override is set. + ## Per-User Settings Admins can configure per-user settings by editing a user in the user management panel. Non-admin users can also edit their own settings through **My Account** (accessible from the user menu). Admins control which sections are visible in My Account via the **Visible Self-Settings Sections** option. diff --git a/shelfmark/config/security.py b/shelfmark/config/security.py index d3476553..69980859 100644 --- a/shelfmark/config/security.py +++ b/shelfmark/config/security.py @@ -102,10 +102,7 @@ def security_settings() -> list[SettingsField]: CustomComponentField( key="builtin_admin_requirement", component="oidc_admin_hint", - label=( - "Local authentication is inactive until a local admin account with a " - "password is created." - ), + label="A local admin account is required before Local authentication can be enabled.", show_when=_auth_condition("builtin"), ), *( @@ -129,8 +126,8 @@ def security_settings() -> list[SettingsField]: component="oidc_admin_hint", label=( "Calibre-Web database not detected. Mount your app.db to " - "/auth/app.db to enable this method. Authentication will fall " - "back to none until the database is available." + "/auth/app.db to enable this method. Sign-in fails until the " + "database is available." ), show_when=_auth_condition("cwa"), ), diff --git a/shelfmark/config/security_handlers.py b/shelfmark/config/security_handlers.py index c562aefe..97339681 100644 --- a/shelfmark/config/security_handlers.py +++ b/shelfmark/config/security_handlers.py @@ -13,6 +13,7 @@ if TYPE_CHECKING: from collections.abc import Callable _OIDC_LOCKOUT_MESSAGE = "A local admin account with a password is required before enabling OIDC. Use the 'Go to Users' button above to create one. This ensures you can still sign in if your identity provider is unavailable." +_BUILTIN_LOCKOUT_MESSAGE = "A local admin account with a password is required before enabling Local authentication. Use the 'Go to Users' button above to create one, otherwise nobody could sign in as an admin." _OIDC_REQUIRED_FIELDS = ( ("OIDC_DISCOVERY_URL", "Discovery URL"), ("OIDC_CLIENT_ID", "Client ID"), @@ -78,6 +79,9 @@ def on_save_security( effective_values = _load_effective_security_values(normalized_values) auth_method = str(effective_values.get("AUTH_METHOD", "") or "").strip().lower() + if auth_method == "builtin" and not DISABLE_LOCAL_AUTH and not _has_local_password_admin(): + return {"error": True, "message": _BUILTIN_LOCKOUT_MESSAGE, "values": normalized_values} + if auth_method == "oidc": if not DISABLE_LOCAL_AUTH and not _has_local_password_admin(): return {"error": True, "message": _OIDC_LOCKOUT_MESSAGE, "values": normalized_values} diff --git a/shelfmark/config/users_settings.py b/shelfmark/config/users_settings.py index 3e646275..8774ef3b 100644 --- a/shelfmark/config/users_settings.py +++ b/shelfmark/config/users_settings.py @@ -107,6 +107,10 @@ _USERS_HEADING_DESCRIPTION_BY_AUTH_MODE = { "accounts are created here when new CWA users are found." ), "none": "Authentication is disabled. Anyone can access Shelfmark without signing in.", + "unavailable": ( + "AUTH_METHOD is not a recognized authentication method, so sign-in is refused until it " + "is fixed." + ), "default": "Authentication is disabled. Anyone can access Shelfmark without signing in.", } diff --git a/shelfmark/core/admin_routes.py b/shelfmark/core/admin_routes.py index 5e60b50b..199f81d3 100644 --- a/shelfmark/core/admin_routes.py +++ b/shelfmark/core/admin_routes.py @@ -18,7 +18,7 @@ from shelfmark.config.booklore_settings import ( get_booklore_library_options, get_booklore_path_options, ) -from shelfmark.config.env import CWA_DB_PATH +from shelfmark.config.env import CWA_DB_PATH, DISABLE_LOCAL_AUTH from shelfmark.core.admin_settings_routes import ( register_admin_settings_routes, validate_user_settings, @@ -127,6 +127,27 @@ def _serialize_user( return payload +_LAST_LOCAL_ADMIN_MESSAGE = ( + "Local and OIDC authentication need a local admin account with a password. " + "Create another local admin first, or change the authentication method." +) + + +def _is_last_local_password_admin(user_db: UserDB, user: dict[str, Any]) -> bool: + """Return True when *user* is the only admin who can sign in with a password.""" + if user.get("role") != "admin" or not user.get("password_hash"): + return False + return not any( + other["id"] != user["id"] and other.get("role") == "admin" and other.get("password_hash") + for other in user_db.list_users() + ) + + +def _auth_mode_requires_local_admin(auth_mode: str) -> bool: + """Return whether the active auth mode relies on a local password admin.""" + return auth_mode in {AUTH_SOURCE_BUILTIN, AUTH_SOURCE_OIDC} and not DISABLE_LOCAL_AUTH + + def _sync_all_cwa_users(user_db: UserDB) -> dict[str, int]: """Sync all users from the Calibre-Web database into users.db.""" if not CWA_DB_PATH or not CWA_DB_PATH.exists(): @@ -161,7 +182,7 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: @wraps(f) def decorated(*args: P.args, **kwargs: P.kwargs) -> ResponseReturnValue: - auth_mode = load_active_auth_mode(CWA_DB_PATH, user_db=user_db) + auth_mode = load_active_auth_mode() g.auth_mode = auth_mode if auth_mode != "none": if "user_id" not in session: @@ -350,9 +371,18 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: } ), 400 - # Allow demoting the last admin account. - # Auth mode resolution automatically falls back to "none" when no - # local password admin remains. + if ( + role_changed + and user_fields["role"] != "admin" + and _auth_mode_requires_local_admin(g.auth_mode) + and _is_last_local_password_admin(user_db, user) + ): + return jsonify( + { + "error": "Cannot remove admin role from the last local admin account", + "message": _LAST_LOCAL_ADMIN_MESSAGE, + } + ), 400 # Avoid unnecessary writes for no-op field updates. for field in ("role", "email", "display_name"): @@ -472,9 +502,15 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: } ), 400 - # Allow deleting the last local admin account. - # Auth mode resolution automatically falls back to "none" when no - # local password admin remains. + if _auth_mode_requires_local_admin(g.auth_mode) and _is_last_local_password_admin( + user_db, user + ): + return jsonify( + { + "error": "Cannot delete the last local admin account", + "message": _LAST_LOCAL_ADMIN_MESSAGE, + } + ), 400 user_db.delete_user(user_id) logger.info("Admin deleted user %s: %s", user_id, user["username"]) diff --git a/shelfmark/core/auth_modes.py b/shelfmark/core/auth_modes.py index 81c81948..44e4b9d5 100644 --- a/shelfmark/core/auth_modes.py +++ b/shelfmark/core/auth_modes.py @@ -2,14 +2,15 @@ from __future__ import annotations -import os -import sqlite3 -from pathlib import Path -from typing import TYPE_CHECKING, Any, Protocol, TypeGuard +from typing import TYPE_CHECKING, Any + +from shelfmark.core.logger import setup_logger if TYPE_CHECKING: from collections.abc import Mapping +logger = setup_logger(__name__) + AUTH_SOURCE_BUILTIN = "builtin" AUTH_SOURCE_OIDC = "oidc" AUTH_SOURCE_PROXY = "proxy" @@ -21,38 +22,13 @@ AUTH_SOURCES = ( AUTH_SOURCE_CWA, ) AUTH_SOURCE_SET = frozenset(AUTH_SOURCES) +AUTH_MODE_NONE = "none" +# Resolved when AUTH_METHOD is unrecognized or unreadable. It is not "none", so +# every guard still requires a session, and no login flow accepts it. +AUTH_MODE_UNAVAILABLE = "unavailable" _ALWAYS_ADMIN_SETTINGS_TABS = frozenset({"security", "users"}) -class _UserDBWithAdminPassword(Protocol): - """Minimal user DB surface needed for local-admin checks.""" - - def has_admin_with_password(self) -> bool: ... - - -def _has_admin_password_api(candidate: object) -> TypeGuard[_UserDBWithAdminPassword]: - """Return True when *candidate* exposes the admin-password lookup we need.""" - return callable(getattr(candidate, "has_admin_with_password", None)) - - -def has_local_password_admin(user_db: object | None = None) -> bool: - """Return True when at least one local admin with a password exists.""" - try: - db = user_db - if db is None: - from shelfmark.core.user_db import UserDB - - config_root = os.environ.get("CONFIG_DIR", "/config") - db = UserDB(str(Path(config_root) / "users.db")) - db.initialize() - - if not _has_admin_password_api(db): - return False - return db.has_admin_with_password() - except AttributeError, ImportError, OSError, RuntimeError, TypeError, ValueError, sqlite3.Error: - return False - - def normalize_auth_source( source: object, oidc_subject: object = None, @@ -66,61 +42,38 @@ def normalize_auth_source( return AUTH_SOURCE_BUILTIN -def determine_auth_mode( - security_config: Mapping[str, Any], - cwa_db_path: object | None, - *, - has_local_admin: bool = True, - disable_local_auth: bool = False, -) -> str: - """Determine active auth mode from security config and runtime prerequisites.""" - auth_mode = security_config.get("AUTH_METHOD", "none") - local_admin_available = has_local_admin or disable_local_auth +def determine_auth_mode(auth_method: object) -> str: + """Resolve the active auth mode from the configured AUTH_METHOD. - if auth_mode == AUTH_SOURCE_CWA and cwa_db_path: - return AUTH_SOURCE_CWA - - if auth_mode == AUTH_SOURCE_BUILTIN and local_admin_available: - return AUTH_SOURCE_BUILTIN - - if auth_mode == AUTH_SOURCE_PROXY and security_config.get("PROXY_AUTH_USER_HEADER"): - return AUTH_SOURCE_PROXY - - if ( - auth_mode == AUTH_SOURCE_OIDC - and local_admin_available - and security_config.get("OIDC_DISCOVERY_URL") - and security_config.get("OIDC_CLIENT_ID") - ): - return AUTH_SOURCE_OIDC - - return "none" + Only an explicit "none" disables authentication. A configured method stays + active even when its prerequisites (a local admin, OIDC settings, the CWA + database, a proxy header) are missing, so sign-in fails instead of the + instance silently opening up to anonymous admin access. + """ + normalized = str(auth_method or "").strip().lower() or AUTH_MODE_NONE + if normalized == AUTH_MODE_NONE or normalized in AUTH_SOURCE_SET: + return normalized + return AUTH_MODE_UNAVAILABLE -def load_active_auth_mode( - cwa_db_path: object | None, - *, - user_db: object | None = None, -) -> str: - """Resolve active auth mode using current security config and runtime prerequisites.""" +def load_active_auth_mode() -> str: + """Resolve the active auth mode from the current security config.""" try: - from shelfmark.config.env import DISABLE_LOCAL_AUTH from shelfmark.core.config import config as app_config - security_config = { - "AUTH_METHOD": app_config.get("AUTH_METHOD", "none"), - "PROXY_AUTH_USER_HEADER": app_config.get("PROXY_AUTH_USER_HEADER", ""), - "OIDC_DISCOVERY_URL": app_config.get("OIDC_DISCOVERY_URL", ""), - "OIDC_CLIENT_ID": app_config.get("OIDC_CLIENT_ID", ""), - } - return determine_auth_mode( - security_config, - cwa_db_path, - has_local_admin=has_local_password_admin(user_db), - disable_local_auth=DISABLE_LOCAL_AUTH, + auth_method = app_config.get("AUTH_METHOD", AUTH_MODE_NONE) + except ImportError, OSError, RuntimeError, TypeError, ValueError: + logger.exception("Could not read AUTH_METHOD; denying access until it can be read") + return AUTH_MODE_UNAVAILABLE + + auth_mode = determine_auth_mode(auth_method) + if auth_mode == AUTH_MODE_UNAVAILABLE: + logger.error( + "Unrecognized AUTH_METHOD %r; denying access. Use one of: none, %s", + auth_method, + ", ".join(AUTH_SOURCES), ) - except ImportError, OSError, RuntimeError, TypeError, ValueError, sqlite3.Error: - return "none" + return auth_mode def is_user_active_for_auth_mode(user: Mapping[str, Any], auth_mode: str) -> bool: diff --git a/shelfmark/core/self_user_routes.py b/shelfmark/core/self_user_routes.py index aa100b01..83e0124c 100644 --- a/shelfmark/core/self_user_routes.py +++ b/shelfmark/core/self_user_routes.py @@ -7,7 +7,6 @@ from typing import TYPE_CHECKING, Any from flask import Flask, Response, g, jsonify, request, session from werkzeug.security import generate_password_hash -from shelfmark.config.env import CWA_DB_PATH from shelfmark.core.admin_settings_routes import ( build_user_notification_test_response, validate_user_settings, @@ -191,7 +190,7 @@ def register_self_user_routes(app: Flask, user_db: UserDB) -> None: @wraps(f) def decorated(*args: object, **kwargs: object) -> Response | tuple[Response, int]: - auth_mode = load_active_auth_mode(CWA_DB_PATH, user_db=user_db) + auth_mode = load_active_auth_mode() g.auth_mode = auth_mode if auth_mode != "none" and "user_id" not in session: return jsonify({"error": "Authentication required"}), 401 diff --git a/shelfmark/main.py b/shelfmark/main.py index e52cc1f8..a5a96279 100644 --- a/shelfmark/main.py +++ b/shelfmark/main.py @@ -206,6 +206,30 @@ except (sqlite3.OperationalError, OSError) as e: download_history_service = None activity_view_state_service = None + +def _warn_if_local_admin_missing() -> None: + """Log a recovery hint when builtin/OIDC auth is active without a local admin.""" + if user_db is None or DISABLE_LOCAL_AUTH: + return + auth_mode = load_active_auth_mode() + if auth_mode not in ("builtin", "oidc"): + return + try: + if user_db.has_admin_with_password(): + return + except sqlite3.Error: + return + logger.warning( + "AUTH_METHOD=%s is active but no local admin account with a password exists. If no " + "admin can sign in, start once with AUTH_METHOD=none (keep Shelfmark off the public " + "internet meanwhile), create a local admin under Settings > Users, then remove the " + "override.", + auth_mode, + ) + + +_warn_if_local_admin_missing() + # Start download coordinator backend.start() @@ -325,10 +349,10 @@ def get_client_ip() -> str: def get_auth_mode() -> str: """Determine which authentication mode is active. - Uses configured AUTH_METHOD plus runtime prerequisites. - Returns "none" when config is invalid or unavailable. + Returns "none" only when AUTH_METHOD is explicitly "none"; an unreadable or + unrecognized AUTH_METHOD fails closed. """ - return load_active_auth_mode(CWA_DB_PATH, user_db=user_db) + return load_active_auth_mode() _AUDIOBOOK_CATEGORY_RANGE = (3030, 3049) diff --git a/src/frontend/src/components/settings/users/useUserMutations.ts b/src/frontend/src/components/settings/users/useUserMutations.ts index bdb00c35..8e5b3c36 100644 --- a/src/frontend/src/components/settings/users/useUserMutations.ts +++ b/src/frontend/src/components/settings/users/useUserMutations.ts @@ -41,9 +41,6 @@ const getPasswordError = (password: string, passwordConfirm: string) => { return password === passwordConfirm ? null : 'Passwords do not match'; }; -const countLocalPasswordAdmins = (users: AdminUser[]): number => - users.filter((user) => user.auth_source === 'builtin' && user.role === 'admin').length; - const authSourceLabel: Record = { builtin: 'Local', oidc: 'OIDC', @@ -109,7 +106,6 @@ export const useUserMutations = ({ ? getPasswordError(editPassword, editPasswordConfirm) : null; if (passwordError) return fail(passwordError); - const localAdminsBeforeSave = includeProfile ? countLocalPasswordAdmins(users) : 0; const caps = editingUser.edit_capabilities; const settingsPayload = includeSettings @@ -152,17 +148,7 @@ export const useUserMutations = ({ : 'User updated', 'success', ); - const refreshedUsers = await fetchUsers({ force: true }); - 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', - ); - } + await fetchUsers({ force: true }); return true; } catch (err) { const message = err instanceof Error ? err.message : 'Failed to update user'; @@ -174,24 +160,17 @@ export const useUserMutations = ({ 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'); - const refreshedUsers = await fetchUsers({ force: true }); + await fetchUsers({ force: true }); 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 (err) { const message = err instanceof Error ? err.message : 'Failed to delete user'; diff --git a/tests/config/test_security.py b/tests/config/test_security.py index e2766ed0..329e6070 100644 --- a/tests/config/test_security.py +++ b/tests/config/test_security.py @@ -378,7 +378,7 @@ class TestSecuritySettings: assert hint is not None assert hint.component == "oidc_admin_hint" assert hint.show_when == {"field": "AUTH_METHOD", "value": "builtin"} - assert "inactive" in hint.label.lower() + assert "required" in hint.label.lower() assert "local admin" in hint.label.lower() def test_oidc_admin_requirement_hint_absent_when_local_auth_is_disabled(self): @@ -417,17 +417,40 @@ class TestSecuritySettings: class TestSecurityOnSave: """Tests for current security on-save guard behavior.""" - def test_on_save_passthrough_for_non_oidc(self, tmp_path, monkeypatch): + def test_on_save_passthrough_for_proxy(self, tmp_path, monkeypatch): from shelfmark.config.security import _on_save_security _set_config_dir(monkeypatch, tmp_path) - values = {"AUTH_METHOD": "builtin", "PROXY_AUTH_USER_HEADER": "X-Auth-User"} + values = {"AUTH_METHOD": "proxy", "PROXY_AUTH_USER_HEADER": "X-Auth-User"} result = _on_save_security(values.copy()) assert result["error"] is False assert result["values"] == values + def test_on_save_blocks_builtin_without_local_admin(self, tmp_path, monkeypatch): + from shelfmark.config.security import _on_save_security + + _set_config_dir(monkeypatch, tmp_path) + UserDB(str(tmp_path / "users.db")).initialize() + + result = _on_save_security({"AUTH_METHOD": "builtin"}) + + assert result["error"] is True + assert "local admin" in result["message"].lower() + + def test_on_save_allows_builtin_with_local_admin(self, tmp_path, monkeypatch): + from shelfmark.config.security import _on_save_security + + _set_config_dir(monkeypatch, tmp_path) + user_db = UserDB(str(tmp_path / "users.db")) + user_db.initialize() + user_db.create_user(username="admin", password_hash="hash", role="admin") + + result = _on_save_security({"AUTH_METHOD": "builtin"}) + + assert result["error"] is False + def test_on_save_blocks_oidc_without_local_admin(self, tmp_path, monkeypatch): from shelfmark.config.security import _on_save_security diff --git a/tests/core/test_admin_users_api.py b/tests/core/test_admin_users_api.py index 9fa8c253..cc20e5af 100644 --- a/tests/core/test_admin_users_api.py +++ b/tests/core/test_admin_users_api.py @@ -437,23 +437,43 @@ 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): + def test_demote_last_admin_allowed_without_auth(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"}, - ) + with patch("shelfmark.core.admin_routes.load_active_auth_mode", return_value="none"): + 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" + @pytest.mark.parametrize("auth_mode", ["builtin", "oidc"]) + def test_demote_last_local_admin_rejected(self, admin_client, user_db, auth_mode): + """Regression for #1387: demoting the last local admin used to disable auth.""" + user = user_db.create_user( + username="onlyadmin", + role="admin", + password_hash="hashed_pw", + ) + + with patch("shelfmark.core.admin_routes.load_active_auth_mode", return_value=auth_mode): + resp = admin_client.put( + f"/api/admin/users/{user['id']}", + json={"role": "user"}, + ) + + assert resp.status_code == 400 + assert resp.json["error"] == "Cannot remove admin role from the last local admin account" + assert user_db.get_user(user_id=user["id"])["role"] == "admin" + def test_update_user_email(self, admin_client, user_db): user = user_db.create_user(username="alice") @@ -1798,18 +1818,54 @@ 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): + @pytest.mark.parametrize("auth_mode", ["builtin", "oidc"]) + def test_delete_last_local_admin_rejected(self, admin_client, user_db, auth_mode): + """Regression for #1387: removing the last local admin used to disable auth.""" user = user_db.create_user( username="onlyadmin", password_hash="hashed_pw", role="admin", ) + with patch("shelfmark.core.admin_routes.load_active_auth_mode", return_value=auth_mode): + resp = admin_client.delete(f"/api/admin/users/{user['id']}") + + assert resp.status_code == 400 + assert resp.json["error"] == "Cannot delete the last local admin account" + assert user_db.get_user(user_id=user["id"]) is not None + + def test_delete_local_admin_allowed_when_another_remains(self, admin_client, user_db): + user = user_db.create_user(username="admin1", password_hash="hashed_pw", role="admin") + user_db.create_user(username="admin2", password_hash="hashed_pw", role="admin") + with patch("shelfmark.core.admin_routes.load_active_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 + + @pytest.mark.parametrize("auth_mode", ["none", "proxy"]) + def test_delete_last_local_admin_allowed_without_local_auth( + self, admin_client, user_db, auth_mode + ): + user = user_db.create_user(username="onlyadmin", password_hash="hashed_pw", role="admin") + + with patch("shelfmark.core.admin_routes.load_active_auth_mode", return_value=auth_mode): + resp = admin_client.delete(f"/api/admin/users/{user['id']}") + + assert resp.status_code == 200 + assert user_db.get_user(user_id=user["id"]) is None + + def test_delete_last_local_admin_allowed_when_local_auth_disabled(self, admin_client, user_db): + user = user_db.create_user(username="onlyadmin", password_hash="hashed_pw", role="admin") + + with ( + patch("shelfmark.core.admin_routes.load_active_auth_mode", return_value="oidc"), + patch("shelfmark.core.admin_routes.DISABLE_LOCAL_AUTH", True), + ): + resp = admin_client.delete(f"/api/admin/users/{user['id']}") + + assert resp.status_code == 200 assert user_db.get_user(user_id=user["id"]) is None def test_delete_own_account_rejected(self, admin_client, user_db): @@ -1897,12 +1953,18 @@ class TestOIDCLockoutPrevention: ) assert result["error"] is False - def test_non_oidc_methods_not_blocked(self): - """Other auth methods should not trigger the OIDC check.""" - for method in ("none", "builtin", "proxy", "cwa"): + def test_external_methods_not_blocked(self): + """Methods that do not rely on a local admin should not trigger the check.""" + for method in ("none", "proxy", "cwa"): result = self._call_on_save({"AUTH_METHOD": method}) assert result["error"] is False, f"AUTH_METHOD={method} should not be blocked" + def test_builtin_blocked_without_local_admin(self): + """Local auth without a local admin would lock every admin out.""" + result = self._call_on_save({"AUTH_METHOD": "builtin"}) + assert result["error"] is True + assert "local admin" in result["message"].lower() + def test_oidc_check_preserves_values(self): """When OIDC is blocked, the original values should be returned.""" values = {"AUTH_METHOD": "oidc", "OIDC_CLIENT_ID": "myapp"} diff --git a/tests/core/test_oidc_integration.py b/tests/core/test_oidc_integration.py index 8a26b901..76a0a93d 100644 --- a/tests/core/test_oidc_integration.py +++ b/tests/core/test_oidc_integration.py @@ -1,10 +1,9 @@ """Tests for auth mode and admin policy helpers used by OIDC integration.""" -import sqlite3 - import pytest from shelfmark.core.auth_modes import ( + AUTH_MODE_UNAVAILABLE, determine_auth_mode, get_auth_check_admin_status, get_settings_tab_from_path, @@ -16,95 +15,59 @@ from shelfmark.core.auth_modes import ( class TestDetermineAuthMode: - def test_returns_oidc_when_fully_configured(self): - config = { - "AUTH_METHOD": "oidc", - "OIDC_DISCOVERY_URL": "https://auth.example.com/.well-known/openid-configuration", - "OIDC_CLIENT_ID": "shelfmark", - } - assert determine_auth_mode(config, cwa_db_path=None) == "oidc" - - def test_returns_none_when_oidc_missing_client_id(self): - config = { - "AUTH_METHOD": "oidc", - "OIDC_DISCOVERY_URL": "https://auth.example.com/.well-known/openid-configuration", - } - assert determine_auth_mode(config, cwa_db_path=None) == "none" - - def test_returns_none_when_oidc_missing_discovery_url(self): - config = { - "AUTH_METHOD": "oidc", - "OIDC_CLIENT_ID": "shelfmark", - } - assert determine_auth_mode(config, cwa_db_path=None) == "none" - - def test_builtin_still_works(self): - config = { - "AUTH_METHOD": "builtin", - } - assert determine_auth_mode(config, cwa_db_path=None) == "builtin" - - def test_builtin_requires_local_admin(self): - config = { - "AUTH_METHOD": "builtin", - } - assert determine_auth_mode(config, cwa_db_path=None, has_local_admin=False) == "none" - - def test_proxy_still_works(self): - config = { - "AUTH_METHOD": "proxy", - "PROXY_AUTH_USER_HEADER": "X-Auth-User", - } - assert determine_auth_mode(config, cwa_db_path=None) == "proxy" - - def test_oidc_requires_local_admin(self): - config = { - "AUTH_METHOD": "oidc", - "OIDC_DISCOVERY_URL": "https://auth.example.com/.well-known/openid-configuration", - "OIDC_CLIENT_ID": "shelfmark", - } - assert determine_auth_mode(config, cwa_db_path=None, has_local_admin=False) == "none" + @pytest.mark.parametrize("auth_method", ["builtin", "oidc", "proxy", "cwa", "none"]) + def test_returns_configured_method(self, auth_method): + assert determine_auth_mode(auth_method) == auth_method @pytest.mark.parametrize( - ("auth_mode", "config"), - [ - ("builtin", {"AUTH_METHOD": "builtin"}), - ( - "oidc", - { - "AUTH_METHOD": "oidc", - "OIDC_DISCOVERY_URL": "https://auth.example.com/.well-known/openid-configuration", - "OIDC_CLIENT_ID": "shelfmark", - }, - ), - ], + ("auth_method", "expected"), [("OIDC", "oidc"), (" Builtin ", "builtin")] ) - def test_disable_local_auth_keeps_configured_mode_without_admin(self, auth_mode, config): - assert ( - determine_auth_mode( - config, - cwa_db_path=None, - has_local_admin=False, - disable_local_auth=True, - ) - == auth_mode - ) + def test_normalizes_case_and_whitespace(self, auth_method, expected): + assert determine_auth_mode(auth_method) == expected - def test_load_active_auth_mode_reads_env_backed_cwa_setting(self, monkeypatch, tmp_path): + @pytest.mark.parametrize("auth_method", [None, "", " "]) + def test_unset_method_means_none(self, auth_method): + assert determine_auth_mode(auth_method) == "none" + + @pytest.mark.parametrize("auth_method", ["local", "ldap", "openid", "no"]) + def test_unrecognized_method_fails_closed(self, auth_method): + assert determine_auth_mode(auth_method) == AUTH_MODE_UNAVAILABLE + + def test_load_active_auth_mode_fails_closed_when_config_unreadable(self, monkeypatch): + from shelfmark.core.config import config as app_config + + def _boom(*_args, **_kwargs): + raise RuntimeError("boom") + + monkeypatch.setattr(app_config, "get", _boom) + assert load_active_auth_mode() == AUTH_MODE_UNAVAILABLE + + @pytest.mark.parametrize("auth_method", ["builtin", "oidc"]) + def test_load_active_auth_mode_keeps_local_modes_without_local_admin( + self, monkeypatch, tmp_path, auth_method + ): + """Regression for #1387: no local admin must not turn authentication off.""" + from shelfmark.core.config import config as app_config + + monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) + monkeypatch.setenv("AUTH_METHOD", auth_method) + app_config.refresh(force=True) + + try: + assert load_active_auth_mode() == auth_method + finally: + monkeypatch.delenv("AUTH_METHOD", raising=False) + app_config.refresh(force=True) + + def test_load_active_auth_mode_keeps_cwa_without_database(self, monkeypatch, tmp_path): from shelfmark.core.config import config as app_config monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) monkeypatch.setenv("AUTH_METHOD", "cwa") app_config.refresh(force=True) - cwa_db_path = tmp_path / "app.db" - conn = sqlite3.connect(cwa_db_path) - conn.execute("create table user (name text)") - conn.commit() - conn.close() - try: - assert load_active_auth_mode(cwa_db_path) == "cwa" + assert load_active_auth_mode() == "cwa" finally: monkeypatch.delenv("AUTH_METHOD", raising=False) app_config.refresh(force=True) @@ -118,7 +81,7 @@ class TestDetermineAuthMode: app_config.refresh(force=True) try: - assert load_active_auth_mode(cwa_db_path=None) == "proxy" + assert load_active_auth_mode() == "proxy" finally: monkeypatch.delenv("AUTH_METHOD", raising=False) monkeypatch.delenv("PROXY_AUTH_USER_HEADER", raising=False) diff --git a/tests/e2e/test_auth_endpoints.py b/tests/e2e/test_auth_endpoints.py index ec0357f2..ddb7a2ca 100644 --- a/tests/e2e/test_auth_endpoints.py +++ b/tests/e2e/test_auth_endpoints.py @@ -52,27 +52,11 @@ class TestGetAuthMode: assert main_module.get_auth_mode() == "none" def test_get_auth_mode_builtin(self, main_module): - with ( - patch.object( - main_module.app_config, - "get", - side_effect=_config_getter({"AUTH_METHOD": "builtin"}), - ), - patch("shelfmark.core.auth_modes.has_local_password_admin", return_value=True), + with patch.object( + main_module.app_config, "get", side_effect=_config_getter({"AUTH_METHOD": "builtin"}) ): assert main_module.get_auth_mode() == "builtin" - def test_get_auth_mode_builtin_without_local_admin_falls_back_to_none(self, main_module): - with ( - patch.object( - main_module.app_config, - "get", - side_effect=_config_getter({"AUTH_METHOD": "builtin"}), - ), - patch("shelfmark.core.auth_modes.has_local_password_admin", return_value=False), - ): - assert main_module.get_auth_mode() == "none" - def test_get_auth_mode_proxy(self, main_module): with patch.object( main_module.app_config, @@ -92,12 +76,31 @@ class TestGetAuthMode: ): assert main_module.get_auth_mode() == "cwa" - def test_get_auth_mode_default_on_error(self, main_module): + def test_get_auth_mode_fails_closed_on_error(self, main_module): with patch.object(main_module.app_config, "get", side_effect=RuntimeError("boom")): - assert main_module.get_auth_mode() == "none" + assert main_module.get_auth_mode() == "unavailable" class TestAuthCheckEndpoint: + @pytest.mark.parametrize("auth_method", ["builtin", "oidc"]) + def test_auth_check_requires_login_without_local_admin(self, main_module, auth_method): + """Regression for #1387: with no local admin, visitors were treated as admins.""" + with ( + patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"AUTH_METHOD": auth_method}), + ), + patch.object(main_module.user_db, "has_admin_with_password", return_value=False), + main_module.app.test_request_context("/api/auth/check"), + ): + data = _as_response(main_module.api_auth_check()).get_json() + + assert data["auth_mode"] == auth_method + assert data["auth_required"] is True + assert data["authenticated"] is False + assert data["is_admin"] is False + def test_auth_check_no_auth(self, main_module): with ( patch.object(main_module, "get_auth_mode", return_value="none"),