From 505afcecfa63ff481f5f116779a83eb5c0ec5b8c Mon Sep 17 00:00:00 2001 From: Don Beckham Date: Sun, 26 Jul 2026 14:31:41 -0500 Subject: [PATCH] Refuse tokens that carry no groups claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workspace is gated in two places on purpose: Authentik binds the application to the dropbox group, and the app re-checks the groups claim. The second check existed to not depend on the first — but when the claim was absent it fell back to trusting the first, which is the one case where the backstop was the only thing left to do its job. Absence of the claim does not mean the user belongs to nothing. It means the provider's configuration has drifted, and the honest answer to "is this user a member" is that we cannot tell. Checked the live provider before changing the default, rather than assuming: - a "Beckham Share - OIDC groups" scope mapping is bound to the provider and returns {"groups": [g.name for g in user.ak_groups.all()]} - evaluated against both accounts it yields a populated list containing dropbox - the app requests the groups scope in the authorization request - the current log window covers four sign-ins and no fallback warnings So nobody is relying on the fallback and nobody gets locked out. ALLOW_MISSING_GROUPS_CLAIM (default false) keeps a way back in if that mapping ever breaks, since locking the only two members out of their own files while repairing an identity provider is its own kind of outage. It re-opens the gap it exists to close, so the app warns about it at startup for as long as it is set. --- .env.example | 4 ++++ CHANGELOG.md | 9 +++++++++ app/auth.py | 27 +++++++++++++++++++++------ app/config.py | 5 +++++ app/main.py | 3 +++ docs/CONFIGURATION.md | 1 + docs/SECURITY.md | 21 +++++++++++++++------ tests/backend/conftest.py | 6 ++++++ tests/backend/test_auth.py | 22 ++++++++++++++++++++++ 9 files changed, 86 insertions(+), 12 deletions(-) diff --git a/.env.example b/.env.example index 83e4b94..c85a932 100644 --- a/.env.example +++ b/.env.example @@ -19,6 +19,10 @@ OIDC_CLIENT_ID= OIDC_CLIENT_SECRET= OIDC_SCOPES=openid profile email groups REQUIRED_GROUP=dropbox +# Admit tokens with no groups claim at all. Off by default — a missing claim +# means the provider's scope mapping has drifted, not that the user has no +# groups. Turn on only while repairing that mapping. +ALLOW_MISSING_GROUPS_CLAIM=false # ── Upload limits / abuse controls ────────────────────────────────────────── MAX_UPLOAD_BYTES=5368709120 diff --git a/CHANGELOG.md b/CHANGELOG.md index 90ecdb0..7585856 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ pre-1.0 scheme; dates are when the change reached `main`. ## Unreleased +### Security +- An OIDC token that carries no `groups` claim is now refused instead of being + admitted on the strength of Authentik's application binding. The provider is + configured to send the claim, so its absence means the configuration has + drifted — and falling back reduced a deliberately two-layer gate to the one + layer the in-app check exists not to depend on. `ALLOW_MISSING_GROUPS_CLAIM` + (default `false`) restores the old behaviour for repairing a broken scope + mapping, and warns at startup for as long as it is set. + ### Added - Full reference documentation under `docs/`: architecture, configuration, operations runbook, security model, and API reference, with a docs index and diff --git a/app/auth.py b/app/auth.py index e003409..98c018a 100644 --- a/app/auth.py +++ b/app/auth.py @@ -38,13 +38,28 @@ def __init__(self, data: dict): @property def in_required_group(self) -> bool: - # If Authentik returns no groups claim (scope mapping not configured), - # trust Authentik's application-level group binding instead of locking - # everyone out — but log it so the gap is visible. - if not self.groups: - log.warning("OIDC token carried no groups claim for %s; relying on Authentik app binding", self.sub) + if self.groups: + return settings.required_group in self.groups + # No groups claim at all. Authentik is configured to send one, so its + # absence means the provider's scope mapping has drifted, not that this + # user happens to belong to nothing. Authentik's own binding on the + # application should still have kept non-members out, but that is the + # layer we don't control — and this check exists precisely to not depend + # on it. So the answer is no, unless an operator has deliberately said + # otherwise while repairing the mapping. + if settings.allow_missing_groups_claim: + log.warning( + "OIDC token carried no groups claim for %s; admitting anyway because " + "ALLOW_MISSING_GROUPS_CLAIM is set. Fix the provider's groups scope mapping.", + self.sub, + ) return True - return settings.required_group in self.groups + log.warning( + "OIDC token carried no groups claim for %s; refusing. The provider's groups " + "scope mapping is missing or not bound to this application.", + self.sub, + ) + return False def get_current_user(request: Request) -> CurrentUser | None: diff --git a/app/config.py b/app/config.py index 838b4dd..bd99813 100644 --- a/app/config.py +++ b/app/config.py @@ -39,6 +39,11 @@ class Settings(BaseSettings): # Only members of this group may use the authenticated interface. Authentik # also gates the application by this group; this is the in-app backstop. required_group: str = "dropbox" + # A token with no `groups` claim leaves the in-app check with nothing to + # check, so it is refused. Turn this on only as a temporary measure while a + # broken scope mapping is repaired: it collapses a deliberately two-layer + # gate down to Authentik's binding alone. The app says so on every startup. + allow_missing_groups_claim: bool = False # ── Upload limits & abuse controls ─────────────────────────────────── # Authenticated members get a generous cap; anonymous (landing page) is tight. diff --git a/app/main.py b/app/main.py index 2b1b97c..4a94649 100644 --- a/app/main.py +++ b/app/main.py @@ -39,6 +39,9 @@ async def lifespan(app: FastAPI): init_db() log.info("%s started (oidc_configured=%s, email_enabled=%s)", settings.app_name, settings.oidc_configured, settings.email_enabled) + if settings.allow_missing_groups_claim: + log.warning("ALLOW_MISSING_GROUPS_CLAIM is set: tokens with no groups claim will be " + "admitted to the workspace. Clear it once the scope mapping is fixed.") yield diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index b34c943..4a1f584 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -42,6 +42,7 @@ ignored. Booleans accept `true`/`false`/`1`/`0`. | `OIDC_CLIENT_SECRET` | *(empty)* | OAuth2 client secret. | | `OIDC_SCOPES` | `openid profile email groups` | Requested scopes. The `groups` scope is required for the in-app group check. | | `REQUIRED_GROUP` | `dropbox` | Group required for the authenticated workspace. Enforced at Authentik **and** in-app. | +| `ALLOW_MISSING_GROUPS_CLAIM` | `false` | Admit tokens that carry no `groups` claim at all. Off by default: without the claim the in-app check has nothing to check, and admitting the user silently reduces the gate to Authentik's binding alone. Turn on only while repairing a broken scope mapping; the app logs a warning at startup for as long as it is set. | > Sign-in stays disabled until all three of discovery URL, client ID, and client > secret are set (`settings.oidc_configured`). The public landing page, uploads, diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 75e5940..465b594 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -19,10 +19,17 @@ abuse controls, the data handled, and the threat model. can't complete the flow at the IdP. 2. **In-app backstop** — `/app` and every management route re-check the `groups` claim (`CurrentUser.in_required_group`). -- **No-groups fallback.** If a token arrives with no `groups` claim (scope - mapping missing), the app trusts Authentik's application-level binding rather - than locking everyone out, and logs a warning so the gap is visible. Keep the - `groups` scope configured so the in-app check is authoritative. +- **A missing `groups` claim is refused.** Authentik is configured to send the + claim (a `groups` scope mapping is bound to the provider), so a token without + one means the provider's configuration has drifted — not that the user belongs + to nothing. Admitting them would quietly reduce a deliberately two-layer gate + to Authentik's binding alone, which is the layer this check exists not to + depend on. The request is refused and the reason is logged. +- **Escape hatch.** `ALLOW_MISSING_GROUPS_CLAIM` (default `false`) restores the + old behaviour so a broken scope mapping can be repaired without locking the + members out of their own files. It re-opens the gap it exists to close, so the + app logs a warning at startup for as long as it is set. Clear it once the + mapping is fixed. ## 2. Anonymous-upload abuse controls @@ -112,7 +119,9 @@ Every upload writes an `upload_events` row for review — see below. - Set a strong, unique `SECRET_KEY` in production; rotating it invalidates all sessions. -- Keep the `groups` scope mapping configured so the in-app group check is - authoritative rather than relying on the fallback. +- Keep the `groups` scope mapping bound to the provider. Without it nobody + reaches the workspace, which is the intended failure direction — check + `docker compose logs app` for the "no groups claim" warning if sign-in starts + ending in a 403. - Review the `upload_events` table periodically for anomalous IPs/fingerprints. - Tune `ANON_*` limits to the host's tolerance; they are all environment-driven. diff --git a/tests/backend/conftest.py b/tests/backend/conftest.py index 906f63d..1d5a6ce 100644 --- a/tests/backend/conftest.py +++ b/tests/backend/conftest.py @@ -68,6 +68,12 @@ def nonmember(monkeypatch): return _as_user(monkeypatch, ["other"]) +@pytest.fixture +def groupless(monkeypatch): + """Authenticated user whose token carried no `groups` claim at all.""" + return _as_user(monkeypatch, []) + + @pytest.fixture def db_session(): s = appdb.SessionLocal() diff --git a/tests/backend/test_auth.py b/tests/backend/test_auth.py index 0e96e16..8524426 100644 --- a/tests/backend/test_auth.py +++ b/tests/backend/test_auth.py @@ -27,6 +27,28 @@ def test_member_sees_source_link(client, member): assert settings.source_url in r.text +def test_missing_groups_claim_is_refused(client, groupless): + # A token with no groups claim leaves the in-app check with nothing to + # check. Absence of evidence is not evidence of membership. + r = client.get("/app") + assert r.status_code == 403 + assert "Access required" in r.text + + +def test_missing_groups_claim_also_blocks_the_management_api(client, groupless): + r = client.post("/api/files", files={"file": ("x.txt", b"x", "text/plain")}, data={"expiry_hours": "24"}) + assert r.status_code == 403 + + +def test_missing_groups_claim_can_be_admitted_deliberately(client, groupless, monkeypatch): + # The escape hatch for repairing a broken scope mapping without locking the + # only two members out of their own files. + from app.config import settings + + monkeypatch.setattr(settings, "allow_missing_groups_claim", True) + assert client.get("/app").status_code == 200 + + def test_api_upload_forbidden_for_anonymous(client): r = client.post("/api/files", files={"file": ("x.txt", b"x", "text/plain")}, data={"expiry_hours": "24"}) assert r.status_code == 403