Make OIDC configuration optional so a deployment can start without it #180

Merged

rob merged 1 commit from feature/task-238-optional-oidc into main

2026-08-16 17:33:29 +00:00

rob commented

2026-08-16 17:22:39 +00:00

Owner

Summary

AddPlaceMarkOidc now treats the Oidc section as optional: absent entirely (IConfigurationSection.Exists() false) turns the flow off cleanly — app starts, local sign-in works, OIDC and step-up routes are not mapped. Any key present, however incomplete, is validated exactly as before and is fatal at start-up on the first bad or missing field.
Step-up re-authentication (ADR-0072) is gated off with OIDC: its redeem leg shares OIDC's handback store and ExternalIdentityRepository, and RequestDelegateFactory fails at endpoint-mapping time (not per request) for an unresolvable handler parameter — mapping it unconditionally was tried and broke start-up outright, not just the two OIDC-only routes.
ExternalIdentityRepository moved to an unconditional registration in Program.cs: GET /api/users/me/linked-providers needs it regardless of whether OIDC is configured today.
New unauthenticated GET /api/auth/oidc/availability, mapped unconditionally, so the WebUI knows whether to offer "Sign in with SSO" / "Link an external sign-in" / "Unlink" at all rather than risking a 404.
docker-compose.yml, user-secrets.example.json, .env.example and the README no longer carry the oidc.invalid placeholders; Compose passes Oidc__* through only when set in .env, never with a default, so an unset value is genuinely absent rather than an empty string (which would still count as "present").
ADR-0154 records the decision, in particular the absent-vs-incomplete line and why it's drawn at "does the section carry any key" rather than a new Oidc:Enabled flag.

Test plan

OidcStartupTests covers all three states: absent starts cleanly and doesn't map OIDC/step-up routes; complete validates and starts as before; a section with a single key still throws.
The absent/partial guards were watched fail (inverting the Exists() check, and separately gutting the field validation) before being trusted, then restored.
WebUI tests cover the SSO button, "Link an external sign-in" form, and "Unlink" button all being hidden/disabled when OIDC is unavailable, each mutation-tested the same way.
Full solution build and test suite green (Domain, Contracts, Architecture, WebUI, Api, Infrastructure).
Full Compose full profile stack brought up and torn down on a fresh clone with no .env: API started healthy with no Oidc__* in its environment, OIDC routes 404, availability reports false.

## Summary - `AddPlaceMarkOidc` now treats the `Oidc` section as optional: absent entirely (`IConfigurationSection.Exists()` false) turns the flow off cleanly — app starts, local sign-in works, OIDC and step-up routes are not mapped. Any key present, however incomplete, is validated exactly as before and is fatal at start-up on the first bad or missing field. - Step-up re-authentication (ADR-0072) is gated off with OIDC: its redeem leg shares OIDC's handback store and `ExternalIdentityRepository`, and `RequestDelegateFactory` fails at endpoint-mapping time (not per request) for an unresolvable handler parameter — mapping it unconditionally was tried and broke start-up outright, not just the two OIDC-only routes. - `ExternalIdentityRepository` moved to an unconditional registration in `Program.cs`: `GET /api/users/me/linked-providers` needs it regardless of whether OIDC is configured today. - New unauthenticated `GET /api/auth/oidc/availability`, mapped unconditionally, so the WebUI knows whether to offer "Sign in with SSO" / "Link an external sign-in" / "Unlink" at all rather than risking a 404. - `docker-compose.yml`, `user-secrets.example.json`, `.env.example` and the README no longer carry the `oidc.invalid` placeholders; Compose passes `Oidc__*` through only when set in `.env`, never with a default, so an unset value is genuinely absent rather than an empty string (which would still count as "present"). - ADR-0154 records the decision, in particular the absent-vs-incomplete line and why it's drawn at "does the section carry any key" rather than a new `Oidc:Enabled` flag. ## Test plan - [x] `OidcStartupTests` covers all three states: absent starts cleanly and doesn't map OIDC/step-up routes; complete validates and starts as before; a section with a single key still throws. - [x] The absent/partial guards were watched fail (inverting the `Exists()` check, and separately gutting the field validation) before being trusted, then restored. - [x] WebUI tests cover the SSO button, "Link an external sign-in" form, and "Unlink" button all being hidden/disabled when OIDC is unavailable, each mutation-tested the same way. - [x] Full solution build and test suite green (Domain, Contracts, Architecture, WebUI, Api, Infrastructure). - [x] Full Compose `full` profile stack brought up and torn down on a fresh clone with no `.env`: API started healthy with no `Oidc__*` in its environment, OIDC routes 404, availability reports `false`.

rob added 1 commit

2026-08-16 17:22:39 +00:00

Make OIDC configuration optional, off when the section is absent

CI / build (pull_request) Successful in 3m17s

Details

CI / container-images (pull_request) Successful in 3s

Details

CI / e2e (pull_request) Successful in 2m46s

Details

57b3593745

Step-up re-authentication is gated off with it: RequestDelegateFactory
fails at endpoint-mapping time, not per request, for a handler whose
DI dependencies AddPlaceMarkOidc never registered.

rob reviewed

2026-08-16 17:33:09 +00:00

rob left a comment

Verdict: mergeable

Attacked the absent-vs-incomplete line specifically and couldn't break it:

Exists() correctly returns false only when the section has neither a value nor any child — one key set (any of the five), an empty string, or a whitespace value all land in Exists()==true and go through RequireAbsoluteUrl/RequireNonBlank, which correctly reject blank/whitespace. CreateApi_OidcSectionCarriesOnlyOneKey_StillFailsToStart proves the single-key case; UseTestJwtConfigurationWithoutOidc's own remarks correctly note that setting a value to null via UseSetting still leaves the key present (Exists() true), which is why a separate builder method was needed for the true-absent case — good catch on their part.
Case-insensitive config binding makes the "differently-cased env var" scenario moot; an Oidc:Enabled flag was deliberately rejected in ADR-0154 so there's no separate flag to desync from the five real values.
Fatal branch runs inside AddPlaceMarkOidc, called before builder.Build(), so it's still register-time, not deferred to first request.
docker-compose.yml's bare-key pass-through (Oidc__Authority: with no value/default) is the right mechanism to keep an unset .env var genuinely absent from the container rather than an empty string — confirmed this is documented Compose behaviour, distinct from ${VAR} interpolation, and the PR says it was verified directly against a container besides.
Mutation claims for the partial-config guard check out by reading the assertions, not just names: CreateApi_OidcSectionAbsentEntirely_StartsCleanlyAndDoesNotMapOidcEndpoints asserts on the actual 404/200 responses, and CreateApi_OidcSectionCarriesOnlyOneKey_StillFailsToStart would genuinely redden if the Exists() gate were inverted or deleted.
WebUI: OidcAvailabilityState fails closed (Enabled stays false on load failure, retries next visit). SSO/Link are hidden (not disabled) when unavailable, matching the ADR's stated rationale that a disabled control implies "just switch it on" which isn't true here. The previously-linked-identity case is handled correctly and is the part I expected to find broken: Account.razor keeps Unlink visible but disabled with an explanatory message when OIDC is off but a provider is still linked, rather than either hiding it or offering a dead link — Account_OidcUnavailableWithALinkedProvider_DisablesUnlinkAndExplainsWhy asserts on both the disabled attribute and the hint text.
ADR-0107 condition 3 checked correctly: ADR-0002/0017 don't pose "should OIDC be optional" as an explicitly deferred question (ADR-0002's own deferred question is unrelated — token exchange, already answered by ADR-0017), so no Answered by was added and ADR-0154 cites them one-directionally instead. docs/adr/README.md diff is a clean single-row addition.

No findings to act on.

Verdict: mergeable Attacked the absent-vs-incomplete line specifically and couldn't break it: - `Exists()` correctly returns false only when the section has neither a value nor any child — one key set (any of the five), an empty string, or a whitespace value all land in `Exists()==true` and go through `RequireAbsoluteUrl`/`RequireNonBlank`, which correctly reject blank/whitespace. `CreateApi_OidcSectionCarriesOnlyOneKey_StillFailsToStart` proves the single-key case; `UseTestJwtConfigurationWithoutOidc`'s own remarks correctly note that setting a value to `null` via `UseSetting` still leaves the key present (`Exists()` true), which is why a separate builder method was needed for the true-absent case — good catch on their part. - Case-insensitive config binding makes the "differently-cased env var" scenario moot; an `Oidc:Enabled` flag was deliberately rejected in ADR-0154 so there's no separate flag to desync from the five real values. - Fatal branch runs inside `AddPlaceMarkOidc`, called before `builder.Build()`, so it's still register-time, not deferred to first request. - `docker-compose.yml`'s bare-key pass-through (`Oidc__Authority:` with no value/default) is the right mechanism to keep an unset `.env` var genuinely absent from the container rather than an empty string — confirmed this is documented Compose behaviour, distinct from `${VAR}` interpolation, and the PR says it was verified directly against a container besides. - Mutation claims for the partial-config guard check out by reading the assertions, not just names: `CreateApi_OidcSectionAbsentEntirely_StartsCleanlyAndDoesNotMapOidcEndpoints` asserts on the actual 404/200 responses, and `CreateApi_OidcSectionCarriesOnlyOneKey_StillFailsToStart` would genuinely redden if the `Exists()` gate were inverted or deleted. - WebUI: `OidcAvailabilityState` fails closed (`Enabled` stays `false` on load failure, retries next visit). SSO/Link are hidden (not disabled) when unavailable, matching the ADR's stated rationale that a disabled control implies "just switch it on" which isn't true here. The previously-linked-identity case is handled correctly and is the part I expected to find broken: `Account.razor` keeps Unlink visible but disabled with an explanatory message when OIDC is off but a provider is still linked, rather than either hiding it or offering a dead link — `Account_OidcUnavailableWithALinkedProvider_DisablesUnlinkAndExplainsWhy` asserts on both the `disabled` attribute and the hint text. - ADR-0107 condition 3 checked correctly: ADR-0002/0017 don't pose "should OIDC be optional" as an explicitly deferred question (ADR-0002's own deferred question is unrelated — token exchange, already answered by ADR-0017), so no `Answered by` was added and ADR-0154 cites them one-directionally instead. `docs/adr/README.md` diff is a clean single-row addition. No findings to act on.

rob merged commit 84610c871e into main

2026-08-16 17:33:29 +00:00

rob deleted branch feature/task-238-optional-oidc

2026-08-16 17:33:30 +00:00

rob referenced this pull request from a commit

2026-08-16 17:33:31 +00:00

Make OIDC configuration optional so a deployment can start without it (#180)

rob referenced this pull request

2026-08-16 17:39:17 +00:00

Joint image: derive connection strings and provisioning from plain env vars #179