Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions modules/users/users/provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:])
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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`)",
Expand Down
33 changes: 29 additions & 4 deletions tests/test_bootstrap_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down
36 changes: 30 additions & 6 deletions tests/test_principal_resolver_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading