docs(auth): record the OAuth 2.1 decision for MCP authentication

[mforce]

Records the design for MCP authentication. Design only — no code, nothing shipped.

#788 began as a question: an MCP client could not authenticate at all, because the access token lives 15 minutes and rotation happens through an HttpOnly refresh cookie, which is a browser mechanism. The answer is to build an OAuth 2.1 authorization server with OpenIddict, running alongside the existing login and serving third-party clients only.

Why this is worth a document rather than just issues

Three things a reader cannot reconstruct from the code that eventually lands:

The rejected alternative, in full. A GitHub-style Personal Access Token design was worked through to completion — self-service on the Account page, cw_pat_ prefix with CRC32 checksum for secret scanning, GitHub-matching revocation semantics, Owner-wide visibility. It was workable and cheaper by roughly half. It lost on one argument: a PAT is a workaround for MCP clients not being able to do OAuth, and OAuth is the better answer for delegated, revocable access to someone else's data. The design is kept rather than discarded, so if OAuth proves too heavy nobody rediscovers it.

Why OpenIddict, and what it cost to rule out the others. Microsoft documents that ASP.NET Core has no PAT or token-server primitive — the Identity API's bearer tokens are "not intended to be a full-featured identity service provider or token server". So the options were OpenIddict or hand-rolling OAuth. Duende was priced ($5,750–$24,900/yr) and rejected.

Four accepted risks, stated rather than glossed — including one-maintainer bus factor on a security-critical dependency, and two authentication paths coexisting.

The constraint that matters most

An OAuth-authenticated request does not pass through CredentialEpochMiddleware, which is where the app does its per-request fail-closed checks. So disabled user, suspended farm, must-change-password and flock scoping are not inherited — every one must be re-established on the OAuth path. That is the single most likely thing to be under-scoped, because it produces nothing visible, and it is why #796 exists as its own slice.

Findings that overturned earlier assumptions

04-verified-findings.md records four corrections made during the design, each verified against source or the restored assemblies rather than recalled:

• TOTP is in-box, not greenfield — AuthenticatorTokenProvider, key generation and recovery codes all ship in Identity 10. An earlier round of this design advised against it on the opposite assumption.
• Passkeys ship in .NET 10 but as a primary factor, so Add TOTP and WebAuthn/passkey as step-up factors (follow-up to #308) #320's step-up slice is more work than its body implies, not less.
• Adding 4 tables breaks exactly one guard, and it is the correct one — TenantBypassDiscoveryTests.DiscoveredSurface_Floor asserts exact set equality, so it must be edited deliberately. The rest of the tenancy stack skips entities without AccountId structurally, so the boot is unaffected.
• Data Protection has no persisted key ring — a latent defect found in passing, filed independently as Data Protection has no persisted key ring, so Identity's token providers break across restarts and replicas #794 and now a prerequisite for OAuth slice 1: stand up OpenIddict — tables, migration, token issuance #795.

Why docs/plans/ rather than docs/decisions/

A decision record here is defined as "the relocated rationale for a rule that also appears — in one compressed paragraph — in AGENTS.md". Nothing has shipped, so there is no rule to compress; filing there would mean writing an AGENTS.md bullet for code that does not exist. docs/plans/ is explicitly the paper trail of a feature's design captured before the work.

A decision record is owed when the implementation lands, and the plan's README names both the rule it should carry and who owes it (#796).

Verification

Docs-only. TenancyDocsFreshnessTests passes; checked against both tracked-file guards (SchemaDocsTests' postgres pin, the tenancy staleness sweep) before committing, since #508 records a plan document tripping one of those.

Issue linkage — deliberately none

This PR uses no closing keyword. Issue #788 is the parent of six implementation slices (#795–#800) and must stay open until they land.

Worth recording the near-miss, because it is precisely the failure this repo's convention warns about. The first draft of this body wrote the close-keyword followed by the word "no" and a sentence explaining it did not apply. GitHub parses the keyword and ignores the prose disclaiming it — so #788 would have auto-closed on merge, taking the parent of six open slices with it. A second draft then quoted that phrase while explaining the mistake, and re-armed it.

Caught both times by checking closingIssuesReferences through the GraphQL API rather than re-reading the body, which is exactly why the convention says to verify against the API and never by reading.

Summary by CodeRabbit

• Documentation
  • Added design documentation for MCP OAuth 2.1 authentication using OpenIddict.
  • Documented the proposed authorization flow, consent and security requirements, permission model, token handling, and redirect protections.
  • Recorded library evaluations, verified platform findings, implementation considerations, risks, and rejected alternatives.
  • Added the MCP OAuth design record to the planning index.
  • No implementation changes are included; this work is design-only.

[mforce]

@coderabbitai review

Docs-only, but the prose makes falsifiable claims about this repo and about third-party libraries. Please weight correctness over style — a wrong claim here misleads whoever implements six slices.

Worth checking specifically:

1. The tenancy analysis in 04-verified-findings.md §3. It claims the AccountId must be a non-nullable Guid: the write guard and the #562 token walk are both fail-open for any other shape #673 AccountId model walk, TenantStampInterceptor and the Add discovery guard for flock-scoped EF query filters #613 flock walk all skip entities lacking the relevant property, so adding 4 OpenIddict tables does not break the boot — and that TenantBypassDiscoveryTests.DiscoveredSurface_Floor is the single test that fails, because it asserts exact set equality. If any of that is wrong, the whole cost estimate is wrong.
2. §4's Data Protection claim — that there is no AddDataProtection/PersistKeysTo* anywhere, that AddDefaultTokenProviders() is nonetheless registered, and that GeneratePasswordResetTokenAsync is called today.
3. §2's passkey claim — that Identity 10's entry points are on SignInManager, and that this repo uses AddIdentityCore without AddSignInManager(), so SignInManager is never registered.
4. 02-design.md's claim that scopes compose as two independent gates rather than a computed intersection. If that framing is wrong, the guard implications change.

No source changes. The PR deliberately does not close #788, which stays open as the parent of #795-#800.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: e0f88388-9d46-4485-9eb0-04d17dce9595

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4bd211fa-e69e-45c0-9721-65d9f983b417

📥 Commits

Reviewing files that changed from the base of the PR and between c1cc58c and 40ae474.

📒 Files selected for processing (6)

• docs/plans/788-mcp-oauth/00-README.md
• docs/plans/788-mcp-oauth/01-decision.md
• docs/plans/788-mcp-oauth/02-design.md
• docs/plans/788-mcp-oauth/03-libraries.md
• docs/plans/788-mcp-oauth/04-verified-findings.md
• docs/plans/README.md

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The PR adds design-only documentation for MCP OAuth 2.1 using OpenIddict. It records the decision, planned authorization flow, security requirements, library evaluation, verified framework findings, and implementation slices. No application code is added.

Changes

MCP OAuth design

Layer / File(s)	Summary
Decision and planning scope docs/plans/788-mcp-oauth/00-README.md, docs/plans/788-mcp-oauth/01-decision.md, docs/plans/README.md	Defines OpenIddict as the OAuth 2.1 approach, records rejected alternatives and accepted risks, and lists the planned implementation slices.
OAuth flow and security rules docs/plans/788-mcp-oauth/02-design.md	Describes registration, consent, authorization, reference-token exchange, scope enforcement, fail-closed checks, consent behavior, and redirect validation.
Library evaluation and verified findings docs/plans/788-mcp-oauth/03-libraries.md, docs/plans/788-mcp-oauth/04-verified-findings.md	Records library evaluations, available ASP.NET Core facilities, rejected PAT details, and verified findings about TOTP, passkeys, tenancy, Data Protection, bearer tokens, and revocation.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Other · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 40ae4

This documentation-only change introduces no runtime or deployment risk and is ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The documents correctly record the selected OAuth 2.1 and OpenIddict design for #788. However, #788 now tracks the OAuth implementation. The PR ships no OpenIddict foundation, token issuance, OAuth-pa…	Implement the required slices in #795–#800, including automated tests, or keep this change attached to a separate design-only issue and do not treat it as completion of #788.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise, uses a conventional docs scope, and accurately identifies the OAuth 2.1 MCP authentication decision documented by the pull request.
Description check	✅ Passed	The description clearly explains the change, rationale, rejected alternatives, risks, verification, issue linkage, and documentation location. It does not reproduce the template headings or checklist,…
Out of Scope Changes check	✅ Passed	The changes are limited to documentation in docs/plans/788-mcp-oauth/ and its planning-record index entry. The decision, design, library evaluation, verified findings, security checks, and implementat…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…

Full details: Linked Issues check

Explanation

The documents correctly record the selected OAuth 2.1 and OpenIddict design for #788. However, #788 now tracks the OAuth implementation. The PR ships no OpenIddict foundation, token issuance, OAuth-path checks, client registration, consent flow, connected-apps screens, audit provenance, or automated tests. The documents explicitly state that no code has shipped, so the coding requirements remain unmet.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch docs/788-oauth-design

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Relevant documentation files ==='
fd -t f '04-verified-findings.md|02-design.md|README.md' docs 2>/dev/null || true

printf '%s\n' '=== Document claims and context ==='
for f in $(fd -t f '04-verified-findings.md|02-design.md' docs 2>/dev/null); do
  echo "--- $f"
  rg -n -C 8 'AccountId|TenantStampInterceptor|TenantBypassDiscoveryTests|Data Protection|DataProtection|AddDataProtection|PersistKeys|AddDefaultTokenProviders|GeneratePasswordResetTokenAsync|passkey|Passkey|SignInManager|AddIdentityCore|AddSignInManager|scope|Scope|intersection|gate' "$f" || true
done

printf '%s\n' '=== Candidate source and test files ==='
fd -t f . | rg -i '(TenantStampInterceptor|TenantBypassDiscoveryTests|Program|ServiceCollection|Identity|Flock|Tenancy|OpenIddict|AccountId)'

printf '%s\n' '=== Identity, data-protection, and reset-token registrations/usages ==='
rg -n -i -C 5 'AddDataProtection\s*\(|PersistKeysTo|AddDefaultTokenProviders\s*\(|GeneratePasswordResetTokenAsync\s*\(|AddIdentityCore\s*(<|\()|AddSignInManager\s*(<|\()|SignInManager\s*<' -g '*.cs' -g '*.csproj' .

printf '%s\n' '=== Tenancy discovery/interceptor/flock patterns ==='
rg -n -i -C 8 'TenantStampInterceptor|TenantBypassDiscoveryTests|DiscoveredSurface_Floor|AccountId|flock' -g '*.cs' .

printf '%s\n' '=== Project package/framework versions relevant to Identity ==='
rg -n -C 3 'TargetFramework|Microsoft\.AspNetCore\.Identity|OpenIddict' -g '*.csproj' -g 'Directory.Packages.props' -g 'global.json' . || true

printf '%s\n' '=== Scope authorization implementation ==='
rg -n -i -C 8 'scope|Scope|require.*claim|RequireClaim|authorization|Authorization' -g '*.cs' . | head -n 800

Length of output: 202074

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

docs=$(fd -t f -a '04-verified-findings.md|02-design.md' docs)
printf '%s\n' '=== Documents ==='
printf '%s\n' "$docs"

printf '%s\n' '=== Complete relevant document sections ==='
while IFS= read -r f; do
  echo "--- $f"
  nl -ba "$f" | sed -n '/^ *[0-9]*.*§\? *2\|^ *[0-9]*.*Passkey\|^ *[0-9]*.*3\|^ *[0-9]*.*4\|^ *[0-9]*.*Data Protection\|scope/,+100p' | head -n 500
done <<< "$docs"

printf '%s\n' '=== Exact tenancy model configuration ==='
rg -n -l 'TenantStampInterceptor|HasQueryFilter|FindProperty\(.*AccountId|nameof\(.*AccountId|FlockScope' src/Cluckwork.Infrastructure tests/Cluckwork.Application.Tests/TenantBypass tests/Cluckwork.Application.Tests/FlockScope | while IFS= read -r f; do
  echo "--- $f"
  rg -n -C 12 'TenantStampInterceptor|HasQueryFilter|FindProperty\(.*AccountId|nameof\(.*AccountId|FlockScope|DiscoveredSurface_Floor|Assert\.Equal' "$f"
done

printf '%s\n' '=== Exact identity/data protection registrations and token call sites ==='
rg -n -l 'AddIdentityCore|AddSignInManager|AddDefaultTokenProviders|AddDataProtection|PersistKeysTo|GeneratePasswordResetTokenAsync' src tests | while IFS= read -r f; do
  echo "--- $f"
  rg -n -C 10 'AddIdentityCore|AddSignInManager|AddDefaultTokenProviders|AddDataProtection|PersistKeysTo|GeneratePasswordResetTokenAsync' "$f"
done

printf '%s\n' '=== Scope filter construction ==='
rg -n -l 'HasQueryFilter|IsUnrestricted|AssignedFlockIds' src/Cluckwork.Infrastructure | while IFS= read -r f; do
  echo "--- $f"
  rg -n -C 14 'HasQueryFilter|IsUnrestricted|AssignedFlockIds|AccountId' "$f"
done

printf '%s\n' '=== All DataProtection persistent-key APIs (source only) ==='
rg -n -i 'AddDataProtection|PersistKeysTo|DataProtectionProvider|IXmlRepository|KeyRing' src -g '*.cs' || true

Length of output: 133688

---

🤖 web_search executed:

Microsoft ASP.NET Core Identity 10 passkeys SignInManager APIs AddSignInManager documentation

Length of output: 12973

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Exact asserted model/discovery surfaces ==='
rg -n -i -C 5 'GetEntityTypes\(\)|GetDeclaredQueryFilters\(\)|Assert\.Equal\(\s*\[|expected.*(entity|filter|surface|model)|IdentityUserClaim|IdentityUserLogin|IdentityUserToken|IdentityUserRole|ApplicationUser|ApplicationRole' tests -g '*.cs' | head -n 1800

printf '%s\n' '=== Identity service registration, including possible indirect registration ==='
rg -n -C 6 'AddIdentityCore|AddSignInManager|AddIdentity\(|SignInManager\s*<' . -g '*.cs' -g '*.csproj' -g '*.props' -g '*.targets'

printf '%s\n' '=== Data Protection API and reset-token calls across tracked files ==='
git grep -n -i -E 'AddDataProtection|PersistKeysTo|DataProtectionProvider|IXmlRepository|KeyRing|AddDefaultTokenProviders|GeneratePasswordResetTokenAsync' || true

printf '%s\n' '=== Relevant part of discovery test (all expected names) ==='
nl -ba tests/Cluckwork.Application.Tests/TenantBypass/TenantBypassDiscoveryTests.cs | sed -n '32,90p'

printf '%s\n' '=== Role authorization definitions and route/tool authorization attributes ==='
rg -n -C 7 'RequireRole|Roles\s*=|RequireClaim|RequireAssertion|AddPolicy|Authorize' src/Cluckwork.Api src/Cluckwork.Infrastructure -g '*.cs' | head -n 1600

Length of output: 215667

---

@mforce, I found no correctness findings in the checked claims.

• The tenancy analysis is correct. The AccountId model walk and TenantStampInterceptor both skip entities with no AccountId. The flock discovery walk only selects Flock and entities with FlockId. The current filter-free-surface test uses Assert.Equal(expected, filterFree). Adding the four filter-free OpenIddict entities would therefore require an explicit update to TenantBypassDiscoveryTests.DiscoveredSurface_Floor. I found no other exact model-surface test that this change would trigger.

• The Data Protection claim is correct for tracked repository files. The repository has no explicit AddDataProtection(), PersistKeysTo*, DataProtectionProvider, IXmlRepository, or KeyRing configuration. AddDefaultTokenProviders() is registered. IdentityProvider calls GeneratePasswordResetTokenAsync before ResetPasswordAsync.

• The passkey claim is correct. The repository targets Identity 10 and registers AddIdentityCore<ApplicationUser>() without AddSignInManager(). No SignInManager registration or use exists. Microsoft documents the passkey APIs on SignInManager<TUser> and documents AddSignInManager() as the registration method.

• The scope wording in 02-design.md is correct as a design requirement. The proposed result is an intersection because role authorization and scope authorization are separate required checks. No computed role-and-scope intersection is necessary. The current repository does not yet implement OAuth scopes, so this is not a claim about existing runtime behavior.

I also confirmed that the document does not state that this PR closes #788.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.