feat(security): per-tenant OIDC clients and tenant-scoped Authentik config (#434 stage 2) - #448
Merged
Merged
Conversation
…nfig [skip ci] Stage 2 of #434. SOURCE COMPLETE, TESTS NOT YET UPDATED — see below. Pushed to avoid stranding the work; not ready for review. Removes the last process.env.AUTHENTIK_* reads outside the registry: - oidc.ts: OIDCService now takes an AuthentikTenant instead of reading env at import time. One instance per tenant, keyed by tenant id (several hosts can map to one tenant and must share a discovery cache and init/backoff state). The former `oidcService` singleton is replaced by getOidcService() / getOidcServiceFor(tenant), plus initializeAllTenants() which warms every tenant at boot and starts each one's self-heal loop independently — one tenant's Authentik being down must not block or reset another's. - The in-cluster endpoint rewrite now uses tenant.baseUrl, so each tenant's server-side token/userinfo/jwks calls land on ITS OWN authentik Service. - authentikPassword.ts, accountApi.ts, machine-identity.ts, AuthentikIdentityProvider.ts, routes/auth.ts, routes/invitations.ts: base URL, issuer, redirect URI, admin token, client credentials and enrollment flow slug all resolve from the tenant. AUTHENTIK_AUTH_FLOW_SLUG and AUTHENTIK_FLOW_TIMEOUT_MS stay on the environment deliberately — the former is authentik's own built-in slug, identical in every instance; the latter is a tuning knob. - AuthentikIdentityProvider binds `oidc` through a lazy getter rather than at construction: the provider is built once, outside any request, so capturing an instance would pin it to whichever tenant was ambient at construction. tsc --noEmit is clean (one pre-existing unrelated eventPublisher error). KNOWN BROKEN: 7 suites mock the module as `jest.mock('../src/services/oidc', () => ({ oidcService: {...} }))`. That export no longer exists, so the mocks resolve undefined and the suites fail with "Cannot read properties of undefined". 12 failed / 56 passed against a baseline of 2 failed / 96 passed. This is a real contract change that must be carried through those mocks — NOT a flake, and not safe to merge until done. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session-Id: 55a394fa-d5ba-4da4-b41f-b0ef56fa6ddf
Completes the stage-2 contract change. The former `oidcService` singleton no
longer exists, so six suites needed updating:
- Four mock the module. Their factories now build the object once and expose
both `oidcService` and `getOidcService: () => oidcService`, backed by the
SAME object, so every existing assertion still addresses exactly what the
code under test receives — the mock shape is unchanged below that line.
- Two use the REAL module (oidc-lazy-reinit re-requires it per test after
resetModules; oidc-google-signin imports it) and now call getOidcService().
The per-test isolation lazy-reinit relies on is preserved: a fresh module
means a fresh per-tenant instance Map, hence a fresh client.
Result: 124 passed / 2 failed across the tenant registry plus the 8
Authentik/OIDC suites. That is exactly the 96/2 baseline plus the 28 new
registry tests — the same two pre-existing failures
(google-brokered-signin, authentik-provider) that reproduce on clean master,
and no regression. tsc --noEmit clean.
No `process.env.AUTHENTIK_*` read remains outside the registry, which closes
that acceptance criterion of #434.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session-Id: 55a394fa-d5ba-4da4-b41f-b0ef56fa6ddf
Contributor
Automated code review (gate-code-review)Credit balance is too low Report-only — this check never blocks merge. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| invitation.role as 'owner' | 'admin' | 'member' | 'viewer' | ||
| ) | ||
| } catch (permitErr) { | ||
| console.error( |
#455 ("un-break sign-in — right-size the timeouts, 503 for outages") rewrote authentikPassword.ts and its test heavily enough that git could not align them at all — both came through as whole-file conflicts (line 1 to EOF). Rather than hand-merge two ~900-line files, I took master's versions wholesale, so ALL of #455's login fix is preserved verbatim, then re-applied my tenant edits on top: - authentikBaseUrl(), redirectUri(), enrollmentFlowSlug() and the admin token resolve from the current tenant instead of process.env - oidcService -> getOidcService() - the test's oidc mock exposes getOidcService() backed by the same object #455's three NEW timeout knobs (AUTHENTIK_LOGIN_DEADLINE_MS, AUTHENTIK_SLOW_HOP_WARN_MS, AUTHENTIK_LOGIN_WARN_MS) are deliberately left on the environment — like AUTHENTIK_FLOW_TIMEOUT_MS they are tuning values, not tenant-identifying configuration. Verified after the merge: tsc --noEmit clean; 129 passed / 2 failed across the tenant registry plus the 8 Authentik/OIDC suites. The 2 are the same pre-existing failures that reproduce on clean master. The count rose from 124 to 129 because #455 added 5 tests to the password-login suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session-Id: 55a394fa-d5ba-4da4-b41f-b0ef56fa6ddf
izzywdev
pushed a commit
that referenced
this pull request
Jul 29, 2026
…r-menu-clplz8 Third merge from master. Two conflicts, both resolved in master's favour, plus one latent defect of my own that the merge brought into view. **`services/chat-service/src/rag/index-docs.ts`** — master fixed the SAME `Awaited<ReturnType<typeof fs.readdir>>` bug I had just fixed, with the same root-cause diagnosis (ReturnType resolves to readdir's Buffer overload under @types/node 24) and an equivalent remedy: infer from the call rather than annotate. Took master's version byte-for-byte and dropped my now-unneeded `type Dirent` import. It is their file and their fix; divergence here buys nothing. **`package-lock.json`** — master removed the stale nested `services/chat-service/node_modules/@types/node@18.19.0`, which is exactly the lockfile reconciliation my previous commit flagged as needed but out of scope. Resolved by taking master's lock wholesale and re-running `npm install` to re-derive this branch's `services/notification-service` subtree, rather than hand-editing lock JSON. Net effect: there is now exactly ONE `@types/node` in the tree, the root `24.13.3` — every nested Node-18 copy is gone, so notification- service resolves the version its manifest asks for and the class of failure that broke CI cannot recur silently in these workspaces. **`frontend/e2e/post-prod/live-smoke.spec.ts`** — not a conflict; a spec my earlier sweep missed because it lives under `e2e/post-prod/` rather than `tests/`. It seeded the bare `authToken` key before app boot. That still boots today, but only because the account vault's one-time legacy migration sweeps it — the same "upgrade path used as a write channel" trap already fixed in WhiteLabelLoginCard. This one matters more: it is the POST-PRODUCTION synthetic that verifies the live deployment, so it would have gone quietly red against prod the day that migration is retired. Now routed through the shared `seedMockSession` helper, whose docstring is generalised since it now seeds a real prod token as well as fixtures. Verified after the merge: - frontend `tsc --noEmit` clean; vitest 136/136 across 19 files - backend 91/91 (app-installations, notification-proxy, apps, provisioning, root-org-admin) against real Postgres, `--runInBand` - chat-service 131 passed / 2 skipped across 24 suites; `tsc` clean - notification-service `tsc` clean, 35/35, now resolving root 24.13.3 - master's new `@fuzefront/auth-ui` 17/17 - no duplicate migration ordinals: 014, 015, 016, 017 - `packages/auth-ui` touches no storage, so #458 introduces no bare-token write Note for anyone running the backend suites locally: master's Permit ReBAC work makes `PERMIT_API_KEY` mandatory at import time, so they need `PERMIT_API_KEY=ci-no-real-permit-calls` (what CI passes) or every suite fails in setup before a single test runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014KynhzG6wKxUnR8KWwK8un
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage 2 of #434, on top of #441. Closes the acceptance criterion "no
process.env.AUTHENTIK_*read remains outside the tenant registry."What changed
oidc.ts—OIDCServicenow takes anAuthentikTenantinstead of reading env at import time. One instance per tenant, keyed by tenant id rather than host, because several hosts can map to one tenant (live./marketplace.mendysrobotics.com) and must share a single discovery cache and init/backoff state instead of racing into duplicate discovery calls. TheoidcServicesingleton is replaced bygetOidcService()/getOidcServiceFor(tenant).initializeAllTenants()— warms every configured tenant at boot and starts each one's self-heal loop. Failures are logged, not thrown: one tenant's Authentik being down must not stop the others coming up, nor reset their state.isssubtlety) now usestenant.baseUrl, so each tenant's server-side token/userinfo/jwks calls land on its own authentik Service.authentikPassword.ts,accountApi.ts,machine-identity.ts,AuthentikIdentityProvider.ts,routes/auth.ts,routes/invitations.ts— base URL, issuer, redirect URI, admin token, client credentials and enrollment flow slug all resolve from the tenant.Two judgement calls worth reviewing
Left on the environment deliberately:
AUTHENTIK_AUTH_FLOW_SLUG(authentik's own built-in flow slug, identical in every instance) andAUTHENTIK_FLOW_TIMEOUT_MS(a tuning knob). Neither is tenant-identifying. Everything that is now comes from the registry.AuthentikIdentityProviderbindsoidcthrough a lazy getter, not at construction. The provider is built once, outside any request, so capturing an instance would permanently pin it to whichever tenant was ambient at construction time. An injected override still wins, so test injection is unaffected.Test changes, and why they were needed
The removed singleton was part of six suites' mocking contract, so this is a real API change, not incidental churn:
oidcServiceandgetOidcService: () => oidcServicebacked by the same object, so every existing assertion still addresses exactly what the code under test receives. Nothing below the mock line changed.oidc-lazy-reinitre-requires it per test afterresetModules;oidc-google-signinimports it) and now callgetOidcService(). The per-test isolationlazy-reinitdepends on is preserved — a fresh module means a fresh per-tenant instance map, hence a fresh client.Verification
master) for the same 8 suitestsc --noEmiteventPublishererror)process.env.AUTHENTIK_*outside the registryThe 2 failures are
google-brokered-signinandauthentik-provider, which reproduce on cleanmasterand are pre-existing — I confirmed that earlier by running the same set against a stashed tree. They are real failures onmastertoday and worth their own look, but not from this work.Still open on #434
Mounting
tenantContexton the auth routers; tenant-scoped session claims at mint time; the chart wiring (per-tenant env replacing the flatAUTHENTIK_*block intemplates/security.yaml, plus routing/api/v1/security/*and/api/auth/*from the Mendys hosts); and the per-tenant Google client decision (shared client vs one each — a consent-screen branding call).Related: #428, #433, #439, #441, izzywdev/FuzeInfra#421, izzywdev/MendysRobotics#253.
🤖 Generated with Claude Code