rob/PlaceMark

Watch

Fork 0

Let an OIDC-only account delete itself via step-up re-authentication #82

Merged

rob merged 2 commits from feat/step-up-reauthentication into main

2026-08-06 05:56:08 +00:00

rob commented

2026-08-06 05:34:17 +00:00

Owner

Closes Vikunja task #152. Recorded as ADR-0072 (next free number, checked against main and the one other open PR, #81, which adds 0071).

DELETE /api/users/me accepts a step-up proof in place of a password. A new PlaceMark.Api.Authentication.StepUp feature reuses ADR-0032's existing OIDC challenge/callback endpoints under a third OidcFlowMode, and adds POST /api/auth/step-up/begin (authenticated, records user_id + purpose behind a 60s handle) and POST /api/auth/step-up/redeem (turns the resulting handback code into a two-minute, single-use StepUpProof, requiring both that the redeeming caller is authenticated as the account the handle recorded and that the freshly verified provider identity resolves — through ExternalIdentityRepository — to that same account, which is what proves a live re-authentication with the account's own already-linked identity rather than merely "holds a valid token"). No grace window, unlike the refresh token: a step-up proof has no legitimate multi-tab replay case, so reuse is refused outright every time (see ADR-0072's discussion of why ADR-0042's reasoning doesn't carry over).

DeleteAccountRequest gains an optional StepUpToken, mutually exclusive with Password via IValidatableObject. POST /api/auth/step-up/begin also runs ADR-0019's sole-owner-of-a-shared-group check (advisory, via a new read-only AccountDeletionRepository.FindSoleOwnedSharedGroupsAsync) before minting a handle, so a caller who's already ineligible isn't sent through a whole provider round trip only to be told so at the end — the authoritative check remains DeleteAsync's own locked transaction, unchanged.

No WebUI surface: there's no account-deletion button in PlaceMark.WebUI yet, so nothing consumes the two-phase flow client-side today.

Full reasoning, alternatives considered and what's deliberately left open (step-up before linking/unlinking, a WebUI surface) are in ADR-0072.

Closes Vikunja task #152. Recorded as ADR-0072 (next free number, checked against `main` and the one other open PR, #81, which adds 0071). `DELETE /api/users/me` accepts a step-up proof in place of a password. A new `PlaceMark.Api.Authentication.StepUp` feature reuses ADR-0032's existing OIDC challenge/callback endpoints under a third `OidcFlowMode`, and adds `POST /api/auth/step-up/begin` (authenticated, records `user_id` + purpose behind a 60s handle) and `POST /api/auth/step-up/redeem` (turns the resulting handback code into a two-minute, single-use `StepUpProof`, requiring both that the redeeming caller is authenticated as the account the handle recorded *and* that the freshly verified provider identity resolves — through `ExternalIdentityRepository` — to that same account, which is what proves a live re-authentication with the account's own already-linked identity rather than merely "holds a valid token"). No grace window, unlike the refresh token: a step-up proof has no legitimate multi-tab replay case, so reuse is refused outright every time (see ADR-0072's discussion of why ADR-0042's reasoning doesn't carry over). `DeleteAccountRequest` gains an optional `StepUpToken`, mutually exclusive with `Password` via `IValidatableObject`. `POST /api/auth/step-up/begin` also runs ADR-0019's sole-owner-of-a-shared-group check (advisory, via a new read-only `AccountDeletionRepository.FindSoleOwnedSharedGroupsAsync`) before minting a handle, so a caller who's already ineligible isn't sent through a whole provider round trip only to be told so at the end — the authoritative check remains `DeleteAsync`'s own locked transaction, unchanged. No WebUI surface: there's no account-deletion button in `PlaceMark.WebUI` yet, so nothing consumes the two-phase flow client-side today. Full reasoning, alternatives considered and what's deliberately left open (step-up before linking/unlinking, a WebUI surface) are in ADR-0072.

rob added 1 commit

2026-08-06 05:34:17 +00:00

Let an OIDC-only account delete itself via step-up re-authentication

CI / build (pull_request) Successful in 2m30s

Details

439f129c3f

rob reviewed

2026-08-06 05:42:35 +00:00

rob left a comment

Verdict: changes needed

Test gap on the crux binding check. In StepUpEndpoints.PostRedeem, disabling the callerUserId != challenge.UserId check (the "redeeming caller is authenticated as the account the handle recorded" comparison — ADR-0072's CSRF closure) does not fail any test: I mutated it locally and all five StepUpEndpointsTests still pass. PostRedeem_StepUpHandleWasMintedForADifferentAccount_RefusesTheProof is the test that should catch this, but it passes for an unrelated reason — the provider identity driven through the callback in that test (subject-victim-again) was never linked to any account, so the later provenUserId is null check refuses the request regardless of whether the caller-binding check ran. The two checks aren't independently exercised: only the provenUserId check is proven by the suite (confirmed separately — disabling that one does fail PostRedeem_VerifiedIdentityResolvesToADifferentAccountThanTheChallenge_RefusesTheProof). Since this is named as the crux of the ticket, the caller-binding check needs its own test where the driven identity does resolve to an existing account (e.g. the victim's own already-linked identity), so removing that specific check is what causes the failure.

Branch doesn't merge cleanly. main has moved (PR #81 landed ADR-0071 after this branch was cut) and merging now conflicts in docs/adr/README.md — both PRs add adjacent rows to the index table. No real numbering collision (ADR-0072 is still free), just the table entry; needs a rebase before merge.

Everything else held up under review: token binding to account and purpose is enforced at redemption (not just recorded at issue), SingleUseStore.TryConsume's TryRemove-then-check is race-free for concurrent redeems/deletes, handles are 32 bytes from RandomNumberGenerator, expiry is checked server-side and fails closed, the in-process-store/single-instance tradeoff is stated in ADR-0072 rather than assumed. DeleteAccountRequest's mutual exclusivity correctly rejects neither/both via one XOR check, and a local-only account can't get a valid step-up token in the first place. ADR-0019's authoritative check still runs unchanged inside DeleteAsync's locked transaction regardless of PostBegin's advisory pre-check. Rate limiting is inherited correctly via the route-prefix match. The stub OIDC provider is a genuine second Kestrel host doing a real code exchange with PKCE verification, not a substitute. No schema changes, contracts stay off the wire, build and full test suite (including Testcontainers) are clean on SDK 10.0.100, CI is green on the head commit.

Verdict: changes needed **Test gap on the crux binding check.** In `StepUpEndpoints.PostRedeem`, disabling the `callerUserId != challenge.UserId` check (the "redeeming caller is authenticated as the account the handle recorded" comparison — ADR-0072's CSRF closure) does not fail any test: I mutated it locally and all five `StepUpEndpointsTests` still pass. `PostRedeem_StepUpHandleWasMintedForADifferentAccount_RefusesTheProof` is the test that should catch this, but it passes for an unrelated reason — the provider identity driven through the callback in that test (`subject-victim-again`) was never linked to any account, so the later `provenUserId is null` check refuses the request regardless of whether the caller-binding check ran. The two checks aren't independently exercised: only the `provenUserId` check is proven by the suite (confirmed separately — disabling *that* one does fail `PostRedeem_VerifiedIdentityResolvesToADifferentAccountThanTheChallenge_RefusesTheProof`). Since this is named as the crux of the ticket, the caller-binding check needs its own test where the driven identity does resolve to an existing account (e.g. the victim's own already-linked identity), so removing that specific check is what causes the failure. **Branch doesn't merge cleanly.** `main` has moved (PR #81 landed ADR-0071 after this branch was cut) and merging now conflicts in `docs/adr/README.md` — both PRs add adjacent rows to the index table. No real numbering collision (ADR-0072 is still free), just the table entry; needs a rebase before merge. Everything else held up under review: token binding to account and purpose is enforced at redemption (not just recorded at issue), `SingleUseStore.TryConsume`'s `TryRemove`-then-check is race-free for concurrent redeems/deletes, handles are 32 bytes from `RandomNumberGenerator`, expiry is checked server-side and fails closed, the in-process-store/single-instance tradeoff is stated in ADR-0072 rather than assumed. `DeleteAccountRequest`'s mutual exclusivity correctly rejects neither/both via one XOR check, and a local-only account can't get a valid step-up token in the first place. ADR-0019's authoritative check still runs unchanged inside `DeleteAsync`'s locked transaction regardless of `PostBegin`'s advisory pre-check. Rate limiting is inherited correctly via the route-prefix match. The stub OIDC provider is a genuine second Kestrel host doing a real code exchange with PKCE verification, not a substitute. No schema changes, contracts stay off the wire, build and full test suite (including Testcontainers) are clean on SDK 10.0.100, CI is green on the head commit.

rob added 1 commit

2026-08-06 05:50:33 +00:00

Merge remote-tracking branch 'origin/main' into feat/step-up-reauthentication

CI / build (pull_request) Successful in 2m31s

Details

3c35148533

# Conflicts:
#	docs/adr/README.md

rob commented

2026-08-06 05:50:46 +00:00

Author

Owner

Both addressed at 3c35148.

PostRedeem_StepUpHandleWasMintedForADifferentAccount_RefusesTheProof renamed to PostRedeem_CallerIsNotTheAccountTheStepUpHandleWasMintedFor_RefusesTheProof and rebuilt: the attacker now completes an entirely legitimate step-up round trip for their own account (so provenUserId genuinely equals challenge.UserId), and only the redeem call's bearer token is the victim's — isolating the caller-binding check. Verified by mutation: deleting callerUserId != challenge.UserId turned it red (200 instead of 401); restoring it turned it green again, with no diff left behind.

Audited the rest of the file for the same shadowing risk. PostRedeem_VerifiedIdentityResolvesToADifferentAccountThanTheChallenge_RefusesTheProof passes the caller-binding check on purpose (same account throughout) so only provenUserId is left to refuse it — confirmed by the same mutation exercise, red on removal, green on restore, with the fixed test above staying green throughout so there's no new cross-masking between the two.
Merged origin/main; the only conflict was the ADR index row, resolved keeping both — 0071 then 0072.

Full CI sequence (build, test — 1412 tests across all six projects, dotnet format --verify-no-changes) clean under the pinned SDK.

Both addressed at `3c35148`. 1. `PostRedeem_StepUpHandleWasMintedForADifferentAccount_RefusesTheProof` renamed to `PostRedeem_CallerIsNotTheAccountTheStepUpHandleWasMintedFor_RefusesTheProof` and rebuilt: the attacker now completes an entirely legitimate step-up round trip for their own account (so `provenUserId` genuinely equals `challenge.UserId`), and only the redeem call's bearer token is the victim's — isolating the caller-binding check. Verified by mutation: deleting `callerUserId != challenge.UserId` turned it red (200 instead of 401); restoring it turned it green again, with no diff left behind. Audited the rest of the file for the same shadowing risk. `PostRedeem_VerifiedIdentityResolvesToADifferentAccountThanTheChallenge_RefusesTheProof` passes the caller-binding check on purpose (same account throughout) so only `provenUserId` is left to refuse it — confirmed by the same mutation exercise, red on removal, green on restore, with the fixed test above staying green throughout so there's no new cross-masking between the two. 2. Merged `origin/main`; the only conflict was the ADR index row, resolved keeping both — 0071 then 0072. Full CI sequence (build, test — 1412 tests across all six projects, `dotnet format --verify-no-changes`) clean under the pinned SDK.

rob reviewed

2026-08-06 05:55:48 +00:00

rob left a comment

Verdict: mergeable

Both follow-ups from review 134 check out:

Reproduced the mutation directly: deleting the callerUserId != challenge.UserId check turns PostRedeem_CallerIsNotTheAccountTheStepUpHandleWasMintedFor_RefusesTheProof red (200 instead of 401) while the other four tests in the file stay green. Traced the guard order in PostRedeem for this scenario by hand: handback consumption and hash checks pass, IsAuthenticated is trivially true, and the attacker's own identity is genuinely linked to their own account, so provenUserId == challenge.UserId would also pass — the caller-binding check is the only thing that can fail the request. The isolation is real.
Audited the rest of the file: PostBegin_CallerIsSoleOwnerOfASharedGroup_RefusesWithoutMintingAHandle has only the one guard it's testing; PostRedeem_OidcRedeemGivenAStepUpHandbackCode_Refuses hits a deterministic switch arm with no earlier check that could produce the same 401 for a different reason. No masking found elsewhere.
docs/adr/README.md merge resolution is clean: 0071 and 0072 both present in numeric order, nothing else dropped (diffed the merge commit against its main-side parent).

Build and full test suite (542 + 284 including Testcontainers) clean on SDK 10.0.100. CI green on 3c35148 (run #392).

Verdict: mergeable Both follow-ups from review 134 check out: - Reproduced the mutation directly: deleting the `callerUserId != challenge.UserId` check turns `PostRedeem_CallerIsNotTheAccountTheStepUpHandleWasMintedFor_RefusesTheProof` red (200 instead of 401) while the other four tests in the file stay green. Traced the guard order in `PostRedeem` for this scenario by hand: handback consumption and hash checks pass, `IsAuthenticated` is trivially true, and the attacker's own identity is genuinely linked to their own account, so `provenUserId == challenge.UserId` would also pass — the caller-binding check is the only thing that can fail the request. The isolation is real. - Audited the rest of the file: `PostBegin_CallerIsSoleOwnerOfASharedGroup_RefusesWithoutMintingAHandle` has only the one guard it's testing; `PostRedeem_OidcRedeemGivenAStepUpHandbackCode_Refuses` hits a deterministic switch arm with no earlier check that could produce the same 401 for a different reason. No masking found elsewhere. - `docs/adr/README.md` merge resolution is clean: 0071 and 0072 both present in numeric order, nothing else dropped (diffed the merge commit against its main-side parent). Build and full test suite (542 + 284 including Testcontainers) clean on SDK 10.0.100. CI green on `3c35148` (run #392).

rob merged commit f925f654b2 into main

2026-08-06 05:56:08 +00:00

rob deleted branch feat/step-up-reauthentication

2026-08-06 05:56:09 +00:00

rob referenced this pull request from a commit

2026-08-06 05:56:10 +00:00

Let an OIDC-only account delete itself via step-up re-authentication (#82)