Skip to content

keycloak: stop hard-coding UserContext.tenant_id from the tenant_id JWT claim, and check register_setup_steps opt-out still holds #376

Description

@antosubash

Why

KeycloakAuthProvider._claims_to_user_context (modules/keycloak/keycloak/provider.py:89-95) sets UserContext.tenant_id=claims.get("tenant_id") directly from the IdP's JWT — this is exactly the legacy single-tenant claim path the tenancy design doc says TenantMiddleware's resolver now supersedes ("Without one, the legacy claim path"/"An authenticated user with no tenant could previously name any tenant with it"), so once the tenants module registers a real resolver this claim is ignored. Without it, the claim is trusted as-is: that is sound only while the realm alone controls tenant_id (a protocol mapper users cannot edit). A realm that maps it from an editable user attribute lets a user pick their tenant, and nothing in this module says so.

Tables

  • KeycloakUserCache (models.py:14) — adopt MultiTenantMixin: no. It's a keycloak_sub → framework-UUID identity cache, analogous to users.User — identity is platform-level, not per-tenant. keycloak_sub unique constraint (models.py:18) stays global: a Keycloak subject maps to exactly one framework identity regardless of which tenants that person later belongs to.

Code paths

  • provider.py:89-95 — UserContext(..., tenant_id=claims.get("tenant_id")) is the concrete instance called out in the task: this must be removed (or left for backward compat but never trusted) once tenants' resolver exists, since TenantMiddleware only falls back to the claim/header path "without" a registered resolver — the risk window is between multi_tenant being turned on and the tenants module actually being installed and its resolver registered, during which this claim is the only source of tenant and is fully attacker/IdP-controlled.
  • _resolve_bearer/resolve_user (provider.py:31-37,64-73) — the session path (UserContext.from_session_dict, provider.py:37) round-trips whatever tenant_id was stamped at login (including from the claim above) through the signed session cookie; even after tenants ships, confirm nothing re-reads this stale UserContext.tenant_id field for authorization instead of calling the live resolver each request (same concern raised in the users module issue for the local-auth path).
  • _upsert_user_cache (provider.py:97-134) opens its own session via request.app.state.sm.db.session_factory, not through get_db's request-scoped session — worth confirming (not fully verified here) that this session still goes through the app's sync_session_class/tenant-aware listener chain (framework/db/simple_module_db/listeners.py:99) rather than bypassing it the way background_tasks' separate sync engine does; KeycloakUserCache has no tenant column so it may not matter today, but any future tenant-scoped write through this same pattern would inherit whatever answer that check gives.
  • register_setup_steps opt-out: per the framework CLAUDE.md, keycloak deliberately registers no setup steps because its local users table is legitimately empty forever ("a host-level superuser count would lock those installs out permanently"). Confirm this still holds once tenants exist — specifically, that a fresh multi-tenant install using Keycloak as its only auth provider does not get stuck with no tenant and no local admin to create one from /setup, since the design doc's "No-tenant handling" (redirect to /tenants) assumes an authenticated user exists to be redirected.
  • No public/anonymous routes beyond the existing login/logout/callback paths (get_public_paths, provider.py:45-57), unaffected by tenancy.

Migration

  • No schema change to KeycloakUserCache.
  • Operationally: realms/clients issuing a tenant_id claim should stop doing so (or it should be documented as ignored) once the tenants resolver is live, to avoid operators believing that claim still does anything.

Tests

  • A JWT bearing an arbitrary/attacker-supplied tenant_id claim never becomes the request's active tenant once a tenant_resolver is registered — assert TenantMiddleware prefers the resolver's answer over UserContext.tenant_id unconditionally, not just when the claim is absent.
  • Session round-trip: a UserContext cached in the session from before a tenant switch does not resurrect the old tenant on a later request — the active tenant is re-resolved per request, not read from the cookie.
  • _upsert_user_cache's session write path is confirmed to go through (or explicitly documented as not needing) the app's tenant/soft-delete listener chain.
  • Regression: a Keycloak-only install with multi_tenant=True and zero local users still boots (no SM021-style lockout) and a first Keycloak login can reach /tenants to create an org.

Out of scope / open questions

  • Whether Keycloak should eventually map a role claim into tenant:<role> for consistency with the tenants module's principal augmentation is a design question for tenants/keycloak integration, not decided here.
  • The _upsert_user_cache session-factory bypass is flagged for verification, not confirmed broken — worth a quick follow-up check against simple_module_db/listeners.py, but not blocking this issue's core fix (removing the trusted tenant_id claim).

Context: part of the SaaS tenancy work in #370 (fail-closed isolation, tenant_context()/all_tenants(), the tenants module). Design: docs/plans/2026-09-27-saas-tenancy-design.md and docs/framework/multi-tenancy.md on that branch.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions