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

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
 

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"}

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)

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