diff --git a/modules/users/users/provider.py b/modules/users/users/provider.py index 018cb2b5..e1440d34 100644 --- a/modules/users/users/provider.py +++ b/modules/users/users/provider.py @@ -69,6 +69,16 @@ class UsersAuthProvider: _is_auth_provider = True async def resolve_user(self, request: Request) -> UserContext | None: + # An ``Authorization`` header is an explicit claim, and it decides the + # request: a bad token returns None rather than falling through to the + # session cookie. Falling through would make an invalid token + # indistinguishable from no token, so a client whose credential expired + # silently keeps working on whatever other identity it carries and its + # 401s depend on what else is in the request. It gains nothing either — + # it can only resolve the session's own identity, which the caller + # already had. Pinned by ``test_a_bad_bearer_is_not_rescued_by_a_valid + # _session``, which asserted the opposite for as long as it went + # uncollected. auth_header = request.headers.get("authorization", "") if auth_header.startswith("Bearer "): return await self._resolve_bearer(request.scope, auth_header[7:]) diff --git a/pyproject.toml b/pyproject.toml index 5d27fee1..369ae5cc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -117,7 +117,7 @@ invalid-assignment = "ignore" [tool.pytest.ini_options] asyncio_mode = "auto" -testpaths = ["framework/cli/tests", "framework/core/tests", "framework/db/tests", "framework/hosting/tests", "framework/testing/tests", "host/tests", "modules/auth/tests", "modules/dashboard/tests", "modules/users/tests", "modules/permissions/tests", "modules/background_tasks/tests", "modules/file_storage/tests", "modules/settings/tests", "modules/feature_flags/tests", "modules/keycloak/tests", "modules/audit_log/tests", "modules/branding/tests", "modules/site_lock/tests", "scripts/tests", "tests/integration", "tests/e2e", "tests/benchmarks", "tests/perf"] +testpaths = ["framework/cli/tests", "framework/core/tests", "framework/db/tests", "framework/hosting/tests", "framework/testing/tests", "host/tests", "modules/auth/tests", "modules/dashboard/tests", "modules/users/tests", "modules/permissions/tests", "modules/background_tasks/tests", "modules/file_storage/tests", "modules/settings/tests", "modules/feature_flags/tests", "modules/keycloak/tests", "modules/audit_log/tests", "modules/branding/tests", "modules/site_lock/tests", "scripts/tests", "tests"] markers = [ "e2e: end-to-end tests requiring a live browser", "perf: performance benchmarks (opt-in; run via `make bench`)", diff --git a/tests/test_bootstrap_settings.py b/tests/test_bootstrap_settings.py index e3055b30..2e83657b 100644 --- a/tests/test_bootstrap_settings.py +++ b/tests/test_bootstrap_settings.py @@ -45,11 +45,36 @@ def test_bootstrap_placeholder_secret_blocks_production(monkeypatch): BootstrapSettings() -def test_host_settings_ignores_env(monkeypatch): - # HostSettings must NOT read env — env-sprawl is what we're removing. +def test_host_settings_reads_its_own_prefixed_env(monkeypatch): + """Env beats the default. This test asserted the opposite and never ran. + + ``testpaths`` did not list ``tests/``, so nothing here was collected by a + bare ``pytest`` — the stale assertion sat green for as long as it took to + notice. The contract it claimed ("HostSettings must NOT read env") is not + the one the codebase has: precedence is env → DB → default, and env has to + keep winning or an upgrade silently changes a deployment's behaviour + (CLAUDE.md § Conventions). ``HostSettings`` declares ``env_prefix="SM_"`` + for exactly that reason. + """ monkeypatch.setenv("SM_MULTI_TENANT", "true") - hs = HostSettings() - assert hs.multi_tenant is False # default wins; env ignored + assert HostSettings().multi_tenant is True + + +def test_host_settings_ignores_unprefixed_env(monkeypatch): + """What the ``env_prefix`` is actually defending against. + + Without it a bare ``HostSettings()`` would read unprefixed names, and + ``LOG_LEVEL`` in particular is a common variable that has nothing to do + with this app. + """ + monkeypatch.delenv("SM_MULTI_TENANT", raising=False) + monkeypatch.setenv("MULTI_TENANT", "true") + assert HostSettings().multi_tenant is False + + +def test_host_settings_default_wins_when_env_is_unset(monkeypatch): + monkeypatch.delenv("SM_MULTI_TENANT", raising=False) + assert HostSettings().multi_tenant is False def test_host_settings_default_locale_must_be_supported(): diff --git a/tests/test_principal_resolver_integration.py b/tests/test_principal_resolver_integration.py index 2ffb2c1a..1ff89a14 100644 --- a/tests/test_principal_resolver_integration.py +++ b/tests/test_principal_resolver_integration.py @@ -78,14 +78,38 @@ async def test_no_auth_header_on_api_returns_401(pat_client): @pytest.mark.anyio -async def test_session_wins_over_bad_bearer(authenticated_client): - """A valid session cookie + Bearer bad -> 200 via session; resolver not consulted. - - The ``authenticated_client`` fixture already carries an admin session cookie; - here we additionally send a bad bearer to prove the session path wins. - Endpoint is any admin-readable, non-user-enumerating route.""" +async def test_a_bad_bearer_is_not_rescued_by_a_valid_session(authenticated_client): + """An explicitly presented credential that is invalid fails the request. + + This test previously asserted the opposite — that the session cookie wins + and "the resolver is not consulted" — and had never run: ``testpaths`` did + not list ``tests/``, so a bare ``pytest`` collected nothing here. The + behaviour it described is not what ``UsersAuthProvider.resolve_user`` does; + the ``Authorization`` header is checked first and a bad token returns + ``None`` without falling through. + + Keeping the code and correcting the test is the deliberate call. Falling + through would make an invalid token indistinguishable from no token at all, + so a client whose credential has expired or been revoked silently keeps + working on whatever other identity it happens to carry, and its 401s become + dependent on what else is in the request. Nothing is gained by the + fall-through either: it can only ever resolve the session's own identity, + which the caller already had. + + The narrow cost is a browser that attaches a stale ``Authorization`` header + to a page request. Nothing in this app does that — pages authenticate with + the session cookie. + """ resp = await authenticated_client.get( "/api/permissions/", headers={"Authorization": "Bearer bad"}, ) + assert resp.status_code == 401 + + +@pytest.mark.anyio +async def test_the_same_session_succeeds_without_the_bad_header(authenticated_client): + """The other half: the session itself is fine, so the 401 above is the + header's doing and not a broken fixture.""" + resp = await authenticated_client.get("/api/permissions/") assert resp.status_code == 200