rob/PlaceMark

Watch

Fork 0

Add external OIDC sign-in and account linking #63

Merged

rob merged 3 commits from feat/oidc-login into main

2026-08-05 08:13:45 +00:00

rob commented

2026-08-05 07:54:29 +00:00

Owner

Implements task 121 against ADR-0032's flow: /api/auth/oidc/{challenge,callback,redeem,link/begin}. The API completes the authorisation code flow itself with PKCE against the provider, and separately with the redemption-verifier construction between the callback and the WebUI's redeem call; no provider identity ever reaches a PlaceMark token. Provider identity is resolved only via a server-side, in-process flow record (SingleUseStore<T>) — nothing persisted, per ADR-0032's single-instance constraint.

Linking follows ADR-0017/ADR-0032 exactly: the external_identities row is written only at redeem, from the caller's own bearer token, never from the redirect or the request body. An OIDC identity whose email matches an existing local account is refused (409), not linked or merged — covered directly by test.

First-time sign-in provisions the user, personal group, Owner membership and external_identities row in one transaction (RegistrationRepository.ProvisionFromOidcAsync, reusing RegisterAsync's shape). Display name resolution (name → preferred_username → local part of email → placeholder, never the full address) is ADR-0057.

No schema script: external_identities already existed in 0001-initial-schema.sql; the flow/handle/handback records are in-process only.

Decision needing no further action here, flagged for Rob: an OIDC-only account still has no self-service route through DELETE /api/users/me (the gap PR #60 documented) — fixing it properly needs step-up re-authentication, which ADR-0017 leaves open generally rather than just for this route. Left open deliberately; commented on task 121 recommending a dedicated follow-up ticket, since I have no task-creation tool.

Provider is Authentik at the deployment level only — nothing about it appears in code; integration tests run against a real, second, stubbed OIDC provider (StubOidcProvider, a second in-process Kestrel host) rather than a substitute.

AddPlaceMarkOidc is mandatory at start-up, refusing on a missing/malformed authority, redirect URIs, client id or secret, mirroring the JWT signing key. Every existing WebApplicationFactory-based test keeps working unchanged — TestJwtConfiguration now also carries well-formed-but-unreachable Oidc:* defaults.

Confirmed (not assumed) that ADR-0037's rate limiter covers the new routes: OidcRateLimitingTests.

Implements task 121 against ADR-0032's flow: `/api/auth/oidc/{challenge,callback,redeem,link/begin}`. The API completes the authorisation code flow itself with PKCE against the provider, and separately with the redemption-verifier construction between the callback and the WebUI's redeem call; no provider identity ever reaches a PlaceMark token. Provider identity is resolved only via a server-side, in-process flow record (`SingleUseStore<T>`) — nothing persisted, per ADR-0032's single-instance constraint. Linking follows ADR-0017/ADR-0032 exactly: the `external_identities` row is written only at redeem, from the caller's own bearer token, never from the redirect or the request body. An OIDC identity whose `email` matches an existing local account is refused (409), not linked or merged — covered directly by test. First-time sign-in provisions the user, personal group, Owner membership and `external_identities` row in one transaction (`RegistrationRepository.ProvisionFromOidcAsync`, reusing `RegisterAsync`'s shape). Display name resolution (`name` → `preferred_username` → local part of `email` → placeholder, never the full address) is ADR-0057. No schema script: `external_identities` already existed in `0001-initial-schema.sql`; the flow/handle/handback records are in-process only. **Decision needing no further action here, flagged for Rob:** an OIDC-only account still has no self-service route through `DELETE /api/users/me` (the gap PR #60 documented) — fixing it properly needs step-up re-authentication, which ADR-0017 leaves open generally rather than just for this route. Left open deliberately; commented on task 121 recommending a dedicated follow-up ticket, since I have no task-creation tool. Provider is Authentik at the deployment level only — nothing about it appears in code; integration tests run against a real, second, stubbed OIDC provider (`StubOidcProvider`, a second in-process Kestrel host) rather than a substitute. `AddPlaceMarkOidc` is mandatory at start-up, refusing on a missing/malformed authority, redirect URIs, client id or secret, mirroring the JWT signing key. Every existing `WebApplicationFactory`-based test keeps working unchanged — `TestJwtConfiguration` now also carries well-formed-but-unreachable `Oidc:*` defaults. Confirmed (not assumed) that ADR-0037's rate limiter covers the new routes: `OidcRateLimitingTests`.

rob added 1 commit

2026-08-05 07:54:30 +00:00

Add external OIDC sign-in and account linking

CI / build (pull_request) Successful in 2m23s

Details

9407d35aaf

rob reviewed

2026-08-05 08:03:59 +00:00

rob left a comment

Verdict: changes needed

Tested this by attack rather than by reading, in a scratch worktree with main merged in (two additive conflicts in docs/adr/README.md and OpenApiDocumentTests.cs, resolved trivially). Full suite green under the pinned SDK (10.0.100): 520 + 267 + others, including a fresh dotnet test --filter Oidc run against a real Postgres and the real StubOidcProvider host.

Account-takeover checklist — all held:

Colliding email on first sign-in: refused 409 (ProvisionAsync's FindByEmailAsync check), never links, never merges. email_verified plays no part anywhere, matching ADR-0017's "no address needs to be verified for a link to occur" — correct, since a true/false claim on that field must not change the outcome.
Linking takes user_id only from the bearer token (CompleteLinkAsync), compared against — never sourced from — the begin-link handle's recorded value. Tried redeeming a link handle minted for one account with a different account's token: 401, no row written (Redeem_LinkHandleWasMintedForADifferentAccount_RefusesTheLink).
Handback code without the verifier, and with a wrong verifier: both 401, and the code is burned either way (Redeem_HandbackCodePresentedWithTheWrongVerifier_...) — a leaked code genuinely redeems nothing.
SingleUseStore<T> is a ConcurrentDictionary.TryRemove, so replay of a consumed handle and reuse of an expired one both fail atomically; unredeemed records simply age out on the next Create's prune. No background reaping, honestly stated as such.
CSRF: the attacker-begins/victim-completes case is closed at step 4 by the token/recorded-id comparison, not by the cookie — confirmed by both reading ADR-0032's own argument and by the test that exercises it.
Start-up: malformed/missing Oidc:Authority, ClientSecret, RedirectUri all throw InvalidOperationException before Build() returns, proven against the real host (OidcStartupTests).
ProvisionFromOidcAsync is one transaction, four inserts, rollback via undisposed-uncommitted NpgsqlTransaction; RegistrationRepositoryOidcTests forces both the email-collision and identity-already-linked failure paths and asserts zero rows left behind.
OidcDisplayName.Resolve never returns a full address — tested with email-only input and with an addr-only local part (@example.com) falling through to the placeholder rather than emitting an empty string.
OidcRateLimitingTests proves the limit independently against GET .../challenge, not just cited by construction.
SingleUseStore<T>'s in-process/single-instance limitation is documented honestly, including the concrete failure mode (a flow that began on one instance and returns to another fails intermittently, silently) — no overstatement.

One thing to fix before merge: StubOidcProvider.cs line 19 names the provider — "nothing named Authentik or any other product appears anywhere in this class or in the code under test" — which puts the literal string in a comment, in code, on this branch. It's a disclaimer rather than a real dependency, but the ticket's acceptance criterion is "no provider is named in code" with no carve-out for comments, and it's the one thing a case-insensitive grep across the branch actually turns up. Reword to drop the name (e.g. "nothing names the real product this stubs").

Nothing else found worth acting on.

Verdict: changes needed Tested this by attack rather than by reading, in a scratch worktree with `main` merged in (two additive conflicts in `docs/adr/README.md` and `OpenApiDocumentTests.cs`, resolved trivially). Full suite green under the pinned SDK (10.0.100): 520 + 267 + others, including a fresh `dotnet test --filter Oidc` run against a real Postgres and the real `StubOidcProvider` host. Account-takeover checklist — all held: - Colliding `email` on first sign-in: refused 409 (`ProvisionAsync`'s `FindByEmailAsync` check), never links, never merges. `email_verified` plays no part anywhere, matching ADR-0017's "no address needs to be verified for a link to occur" — correct, since a true/false claim on that field must not change the outcome. - Linking takes `user_id` only from the bearer token (`CompleteLinkAsync`), compared against — never sourced from — the begin-link handle's recorded value. Tried redeeming a link handle minted for one account with a different account's token: 401, no row written (`Redeem_LinkHandleWasMintedForADifferentAccount_RefusesTheLink`). - Handback code without the verifier, and with a wrong verifier: both 401, and the code is burned either way (`Redeem_HandbackCodePresentedWithTheWrongVerifier_...`) — a leaked code genuinely redeems nothing. - `SingleUseStore<T>` is a `ConcurrentDictionary.TryRemove`, so replay of a consumed handle and reuse of an expired one both fail atomically; unredeemed records simply age out on the next `Create`'s prune. No background reaping, honestly stated as such. - CSRF: the attacker-begins/victim-completes case is closed at step 4 by the token/recorded-id comparison, not by the cookie — confirmed by both reading ADR-0032's own argument and by the test that exercises it. - Start-up: malformed/missing `Oidc:Authority`, `ClientSecret`, `RedirectUri` all throw `InvalidOperationException` before `Build()` returns, proven against the real host (`OidcStartupTests`). - `ProvisionFromOidcAsync` is one transaction, four inserts, rollback via undisposed-uncommitted `NpgsqlTransaction`; `RegistrationRepositoryOidcTests` forces both the email-collision and identity-already-linked failure paths and asserts zero rows left behind. - `OidcDisplayName.Resolve` never returns a full address — tested with email-only input and with an addr-only local part (`@example.com`) falling through to the placeholder rather than emitting an empty string. - `OidcRateLimitingTests` proves the limit independently against `GET .../challenge`, not just cited by construction. - `SingleUseStore<T>`'s in-process/single-instance limitation is documented honestly, including the concrete failure mode (a flow that began on one instance and returns to another fails intermittently, silently) — no overstatement. One thing to fix before merge: `StubOidcProvider.cs` line 19 names the provider — "nothing named Authentik or any other product appears anywhere in this class or in the code under test" — which puts the literal string in a comment, in code, on this branch. It's a disclaimer rather than a real dependency, but the ticket's acceptance criterion is "no provider is named in code" with no carve-out for comments, and it's the one thing a case-insensitive grep across the branch actually turns up. Reword to drop the name (e.g. "nothing names the real product this stubs"). Nothing else found worth acting on.

rob added 2 commits

2026-08-05 08:09:27 +00:00

Remove provider name from a test comment 7b11da5ad2

Merge main into feat/oidc-login

CI / build (pull_request) Successful in 2m31s

Details

0ddee5f8f9

# Conflicts:
#	docs/adr/README.md
#	tests/PlaceMark.Api.Tests/Documentation/OpenApiDocumentTests.cs

rob commented

2026-08-05 08:09:38 +00:00

Author

Owner

Fixed: reworded the StubOidcProvider comment to describe it generically rather than naming a product. Grepped the branch case-insensitively for the name — nothing else found.

Merged main in (PR #61, move-place-endpoint) — resolved the ADR index and OpenApiDocumentTests conflicts. Full CI sequence (build, dotnet format --verify-no-changes, full test suite) reran clean after the merge.

Fixed: reworded the `StubOidcProvider` comment to describe it generically rather than naming a product. Grepped the branch case-insensitively for the name — nothing else found. Merged `main` in (PR #61, move-place-endpoint) — resolved the ADR index and `OpenApiDocumentTests` conflicts. Full CI sequence (build, `dotnet format --verify-no-changes`, full test suite) reran clean after the merge.

rob reviewed

2026-08-05 08:13:20 +00:00

rob left a comment

Verdict: mergeable

Re-reviewed the delta only, at 0ddee5f, in a fresh scratch worktree.

Provider name: 7b11da5 touches only StubOidcProvider.cs's doc comment (6 insertions / 5 deletions, nothing else). Confirmed independently, not just diffed: git grep -ilE authentik at 0ddee5f across the whole tree, and a filesystem-level case-insensitive grep of both file contents and filenames outside .git/bin/obj — both zero hits.
main merge: docs/adr/README.md index runs 0001–0055, 0057, strictly ascending, both the 0055 and 0057 rows present with their "Open questions" entries; no 0056 exists on either parent, so its absence isn't a merge defect. OpenApiDocumentTests.cs's route list is the true 23-entry union — 18 shared + the 4 OIDC-only routes from this branch + /api/places/{id}/group from main — with /api/users/me appearing exactly once, not duplicated or dropped. Full diff from 7b11da5 to 0ddee5f is nine files, all pure additions (the move-place-endpoint feature) plus the 7-line and 1-line conflict resolutions in those two files — nothing interleaved wrongly, nothing silently lost.
Nothing else changed: git diff --stat of every OIDC-touching path (Authentication/Oidc/*, Infrastructure/Authentication/* and their test trees) between 9407d35 and 0ddee5f shows only the one comment file.
Clean build at the pinned SDK (10.0.100) and a full dotnet test run: 13 + 12 + 105 + 17 + 520 + 267 passed, 0 failed. The flagged flaky mutual-demotion test didn't fire this run; noting it wasn't seen, not that it can't be.

Nothing else found worth acting on.

Verdict: mergeable Re-reviewed the delta only, at `0ddee5f`, in a fresh scratch worktree. - **Provider name**: `7b11da5` touches only `StubOidcProvider.cs`'s doc comment (6 insertions / 5 deletions, nothing else). Confirmed independently, not just diffed: `git grep -ilE authentik` at `0ddee5f` across the whole tree, and a filesystem-level case-insensitive grep of both file contents and filenames outside `.git`/`bin`/`obj` — both zero hits. - **`main` merge**: `docs/adr/README.md` index runs 0001–0055, 0057, strictly ascending, both the 0055 and 0057 rows present with their "Open questions" entries; no 0056 exists on either parent, so its absence isn't a merge defect. `OpenApiDocumentTests.cs`'s route list is the true 23-entry union — 18 shared + the 4 OIDC-only routes from this branch + `/api/places/{id}/group` from main — with `/api/users/me` appearing exactly once, not duplicated or dropped. Full diff from `7b11da5` to `0ddee5f` is nine files, all pure additions (the move-place-endpoint feature) plus the 7-line and 1-line conflict resolutions in those two files — nothing interleaved wrongly, nothing silently lost. - **Nothing else changed**: `git diff --stat` of every OIDC-touching path (`Authentication/Oidc/*`, `Infrastructure/Authentication/*` and their test trees) between `9407d35` and `0ddee5f` shows only the one comment file. - Clean build at the pinned SDK (10.0.100) and a full `dotnet test` run: 13 + 12 + 105 + 17 + 520 + 267 passed, 0 failed. The flagged flaky mutual-demotion test didn't fire this run; noting it wasn't seen, not that it can't be. Nothing else found worth acting on.

rob merged commit df458e2ac6 into main

2026-08-05 08:13:45 +00:00

rob deleted branch feat/oidc-login

2026-08-05 08:13:45 +00:00

rob referenced this pull request from a commit

2026-08-05 08:13:46 +00:00

Add external OIDC sign-in and account linking (#63)