Skip to content

Bind MCP grants to stable account identity - #840

Merged
kody-bot merged 2 commits into
mainfrom
cursor/legacy-purge-b-data-migrations-f449
Jul 22, 2026
Merged

kody-bot merged 2 commits into
mainfrom
cursor/legacy-purge-b-data-migrations-f449

Conversation

@kentcdodds

@kentcdodds kentcdodds commented Jul 22, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • resolve MCP grant accounts and RBAC by authoritative users.stable_user_id, not potentially stale grant email
  • require email and stable ID to identify the same account during MCP email-verification checks
  • refresh caller email/profile fields from the stored account and fail closed to no elevated roles on D1 errors
  • add stale-email-reuse regression coverage and align affected worker fixtures with stable IDs

Why

After an account email change, an old grant email can later belong to another account. Email-only verification or role lookup could then combine one account's stable data identity with another account's verification/RBAC state.

Validation

  • npm run validate: passed on final rebased head
  • unit suite: 1,042 tests passed via pre-push
  • Playwright E2E: 17 tests passed via pre-push
System recap — extends existing primitives (medium risk)

Mode: recap · Base: main @ 0b824ebc · Head: c3ee1c9b

Classification: extends — tightens identity binding across MCP auth, email verification, and RBAC.

Primitives touched

Primitive Group Impact
app-sessions auth extends — verification binds email and stable identity
email assistant composes — affected email tests carry stable IDs
entitlements auth composes — shared test users expose indexed stable IDs
app-ui surfaces composes — verification regression coverage

System map

OAuth grant authentication resolves stable identity in D1 before attaching current account metadata or RBAC; verification checks bind both identity attributes.

Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).

flowchart LR
	appSessions["app-sessions<br/>Browser sessions"]:::extended
	email["email<br/>Email"]:::touched
	entitlements["entitlements<br/>Plans & entitlements"]:::touched
	appSessions -->|"stable_user_id account + RBAC lookup"| appSessions
	appSessions -->|"email + stable ID verification pair"| email
	entitlements -->|"stable-ID-aware test schema"| appSessions
	classDef touched fill:#1a7f37,color:#fff
	classDef extended fill:#9a6700,color:#fff
	classDef added fill:#cf222e,color:#fff
	classDef untouched fill:#57606a,color:#fff
Loading

Invariants

  • Grant userId remains the authoritative per-user data scope.
  • Stale grant email cannot attach another account's roles or verified state.
  • D1 lookup failures do not grant elevated permissions.
Open in Web Open in Cursor 

Summary by CodeRabbit

  • Bug Fixes

    • Email verification now confirms the correct account when both email and account identity are provided, preventing stale verification from being reused.
    • OAuth and MCP authentication now resolve profiles and permissions using a stable account identity, improving accuracy when email details change or are unavailable.
    • Failed or missing account lookups now safely fall back without granting roles or permissions.
  • Tests

    • Expanded coverage for verification, identity resolution, permissions, and fallback behavior.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Email verification and MCP authentication now use deterministic stable_user_id lookups, with stricter identity matching, refreshed profile resolution, updated test schemas, seeded fixtures, and database mocks.

Changes

Stable identity authentication

Layer / File(s) Summary
Tighten email verification lookups
packages/worker/src/app/email-verification.ts, packages/worker/src/app/email-verification.node.test.ts
Verification requires matching email and stable ID when both are provided, while preserving email-only and stable-only lookup paths with expanded SQL assertions.
Resolve MCP context by stable identifier
packages/worker/src/mcp-auth-user-context.ts, packages/worker/src/mcp-auth-user-context.node.test.ts
MCP contexts load profile data and permissions through stable_user_id, validate usernames, and fall back to grant-derived fields when lookup or permission loading fails.
Align test schemas and seeded users
packages/worker/src/entitlements/test-schema.ts, packages/worker/src/email/*, packages/worker/src/mcp/capabilities/email/email-usage-get.workers.test.ts
Test schemas and seeded accounts now include deterministic stable IDs and a partial unique index.
Update authentication test database flows
packages/worker/src/mcp-auth.workers.test.ts, packages/worker/src/oauth-handlers.workers.test.ts
Authentication mocks model stable-ID profile and verification queries, while OAuth fixtures include the new column and pass seeded databases to reset-client scenarios.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GrantProps
  participant MCPContext
  participant APP_DB
  participant PermissionsDB
  GrantProps->>MCPContext: Provide stable user ID
  MCPContext->>APP_DB: Load user by stable_user_id
  APP_DB-->>MCPContext: Return profile row
  MCPContext->>PermissionsDB: Load roles and permissions
  PermissionsDB-->>MCPContext: Return authorization data
  MCPContext-->>GrantProps: Return enriched user context
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: binding MCP grants to stable account identity.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/legacy-purge-b-data-migrations-f449

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

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

@cursor
cursor Bot force-pushed the cursor/legacy-purge-b-data-migrations-f449 branch from e79f794 to f960a9b Compare July 22, 2026 01:27
@cursor
cursor Bot force-pushed the cursor/legacy-purge-b-data-migrations-f449 branch from f960a9b to c3ee1c9 Compare July 22, 2026 01:33
@kody-bot
kody-bot marked this pull request as ready for review July 22, 2026 01:35
@github-actions

Copy link
Copy Markdown
Contributor

🔎 Preview deployed: https://kody-pr-840.kody-a99.workers.dev

Worker: kody-pr-840
D1: kody-pr-840-db
KV: kody-pr-840-oauth-kv

Mocks:

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/worker/src/mcp-auth-user-context.ts (1)

27-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fallback email isn't normalized like the DB-refreshed path.

buildBaseUserFromGrant only trims grantProps.email, while the successful lookup path lowercases it (row.email.trim().toLowerCase(), line 80). Low risk since the fallback carries no roles, but worth aligning for consistency.

♻️ Normalize email casing in the fallback path
 	const email =
-		typeof grantProps.email === 'string' ? grantProps.email.trim() : ''
+		typeof grantProps.email === 'string'
+			? grantProps.email.trim().toLowerCase()
+			: ''
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp-auth-user-context.ts` around lines 27 - 28, Update
buildBaseUserFromGrant’s fallback email normalization to trim and lowercase
grantProps.email, matching the row.email normalization used by the successful
lookup path while preserving the existing empty-string fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/worker/src/mcp-auth-user-context.ts`:
- Around line 27-28: Update buildBaseUserFromGrant’s fallback email
normalization to trim and lowercase grantProps.email, matching the row.email
normalization used by the successful lookup path while preserving the existing
empty-string fallback.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 28e04bcc-efc9-4109-9fce-8f4b46b53fca

📥 Commits

Reviewing files that changed from the base of the PR and between 0b824eb and c3ee1c9.

📒 Files selected for processing (10)
  • packages/worker/src/app/email-verification.node.test.ts
  • packages/worker/src/app/email-verification.ts
  • packages/worker/src/email/inbound.workers.test.ts
  • packages/worker/src/email/outbound.workers.test.ts
  • packages/worker/src/entitlements/test-schema.ts
  • packages/worker/src/mcp-auth-user-context.node.test.ts
  • packages/worker/src/mcp-auth-user-context.ts
  • packages/worker/src/mcp-auth.workers.test.ts
  • packages/worker/src/mcp/capabilities/email/email-usage-get.workers.test.ts
  • packages/worker/src/oauth-handlers.workers.test.ts

@kody-bot
kody-bot merged commit bc7c7d6 into main Jul 22, 2026
7 checks passed
@kody-bot
kody-bot deleted the cursor/legacy-purge-b-data-migrations-f449 branch July 22, 2026 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants