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"),