fix: use upstream OAuth consent UI instead of broken auth-service reimplementation#21
Open
fix: use upstream OAuth consent UI instead of broken auth-service reimplementation#21
Conversation
…mplementation
The auth-service consent page had hard-coded permissions ('Read and write
posts', 'Access your profile', 'Manage your follows') that ignored the
actual OAuth scopes requested by the client. This replaces it with the
stock @atproto/oauth-provider consent UI.
Changes:
- epds-callback now redirects through /oauth/authorize after account
creation and device session binding, letting the stock oauthMiddleware
handle consent with actual scopes via consent-view.tsx
- Remove auth-service consent page (consent.ts) and its tests
- Remove client_login table, hasClientLogin/recordClientLogin from db.ts
- Add v8 migration to drop client_logins table
- Simplify complete.ts: remove consent branching (step 5b)
- Add design doc (docs/design/consent-flow-fix.md) with research findings
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR removes custom consent handling from the auth service, deletes the consent route and its tests, simplifies Changes
Sequence DiagramsequenceDiagram
actor User
participant AuthService as auth-service
participant PDSCore as pds-core
participant OAuthMW as stock OAuth middleware
participant OAuthProvider as OAuth Provider
User->>AuthService: Initiate login / authenticate
AuthService->>PDSCore: Redirect to epds-callback with PAR request_uri
PDSCore->>OAuthProvider: requestManager.get(requestUri)
OAuthProvider-->>PDSCore: Return clientId & scope info
PDSCore->>PDSCore: Upsert device ↔ account association
PDSCore->>OAuthMW: Redirect user to /oauth/authorize (with request_uri/state)
OAuthMW->>OAuthMW: Evaluate prior consent / prompt params
alt Auto-approve
OAuthMW->>OAuthProvider: Issue authorization code
OAuthProvider-->>OAuthMW: Code issued
OAuthMW-->>User: Redirect to client with code
else Consent required
OAuthMW-->>User: Render consent UI
User->>OAuthMW: Approve scopes
OAuthMW->>OAuthProvider: Issue authorization code
OAuthProvider-->>OAuthMW: Code issued
OAuthMW-->>User: Redirect to client with code
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/pds-core/src/index.ts (1)
276-282: PreferdidforupsertDeviceAccountto reduce nullable coupling.
didis already resolved in successful paths; usingaccount.subadds avoidable runtime dependency onaccountshape.♻️ Suggested hardening
- await provider.accountManager.upsertDeviceAccount(deviceId, account.sub) + if (!did) { + res + .status(500) + .type('html') + .send(renderError('Account resolution failed. Please try again.')) + return + } + await provider.accountManager.upsertDeviceAccount(deviceId, did)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pds-core/src/index.ts` around lines 276 - 282, The call to provider.accountManager.upsertDeviceAccount currently uses account.sub which creates an unnecessary runtime dependency on the account shape; change this to use the already-resolved DID variable (did) instead so the call becomes provider.accountManager.upsertDeviceAccount(deviceId, did) and remove any reliance on account.sub in this binding path, ensuring did is defined/validated before invoking upsertDeviceAccount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/consent-flow-fix.md`:
- Around line 17-19: Update the documentation to use the exact table name
client_logins (plural) for consistency: replace the occurrence of client_login
with client_logins in the sentence that describes the separate consent tracking
and ensure the references to hasClientLogin and recordClientLogin remain correct
and aligned with the schema/migration naming to avoid confusion with the atproto
provider's authorizedClients tracking.
- Around line 49-53: The fenced code blocks containing the flow diagrams (the
OTP verify → ... sequences) are missing a language identifier which triggers
markdownlint MD040; update those backtick fences around the flow diagrams (the
two blocks showing the OTP/auth/consent → ... sequences) to include a language
specifier such as "text" (i.e., change ``` to ```text) so the blocks pass MD040;
ensure both occurrences referenced in the comment (the block around the OTP
verify → /auth/complete ... sequence and the block around the
/oauth/epds-callback → create account ... sequence) are updated.
---
Nitpick comments:
In `@packages/pds-core/src/index.ts`:
- Around line 276-282: The call to provider.accountManager.upsertDeviceAccount
currently uses account.sub which creates an unnecessary runtime dependency on
the account shape; change this to use the already-resolved DID variable (did)
instead so the call becomes
provider.accountManager.upsertDeviceAccount(deviceId, did) and remove any
reliance on account.sub in this binding path, ensuring did is defined/validated
before invoking upsertDeviceAccount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ab703ac-c387-46a9-8e8c-6392f14c5900
📒 Files selected for processing (7)
docs/design/consent-flow-fix.mdpackages/auth-service/src/__tests__/consent.test.tspackages/auth-service/src/index.tspackages/auth-service/src/routes/complete.tspackages/auth-service/src/routes/consent.tspackages/pds-core/src/index.tspackages/shared/src/db.ts
💤 Files with no reviewable changes (3)
- packages/auth-service/src/index.ts
- packages/auth-service/src/tests/consent.test.ts
- packages/auth-service/src/routes/consent.ts
Comment on lines
+17
to
+19
| 3. **Separate consent tracking** — uses its own `client_login` table | ||
| (`hasClientLogin` / `recordClientLogin`) instead of the atproto provider's | ||
| built-in `authorizedClients` tracking, which already handles scope-level |
There was a problem hiding this comment.
Use the exact table name client_logins for consistency.
The doc currently says client_login, but the schema/migration name is client_logins. Please align wording to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/consent-flow-fix.md` around lines 17 - 19, Update the
documentation to use the exact table name client_logins (plural) for
consistency: replace the occurrence of client_login with client_logins in the
sentence that describes the separate consent tracking and ensure the references
to hasClientLogin and recordClientLogin remain correct and aligned with the
schema/migration naming to avoid confusion with the atproto provider's
authorizedClients tracking.
Comment on lines
+49
to
+53
| ``` | ||
| OTP verify → /auth/complete → check consent (auth-service) → | ||
| → show broken consent page (auth-service) → | ||
| → /oauth/epds-callback → setAuthorized() → redirect to client | ||
| ``` |
There was a problem hiding this comment.
Specify a language for fenced code blocks (MD040).
These blocks are missing a language identifier and will keep markdownlint warnings active.
🛠️ Suggested doc fix
-```
+```text
OTP verify → /auth/complete → check consent (auth-service) →
→ show broken consent page (auth-service) →
→ /oauth/epds-callback → setAuthorized() → redirect to client- +text
OTP verify → /auth/complete →
→ /oauth/epds-callback → create account if needed, upsertDeviceAccount →
→ redirect to /oauth/authorize?request_uri=... (stock middleware) →
→ stock middleware calls provider.authorize() →
→ if consent needed: renders upstream consent-view.tsx with real scopes →
→ if no consent needed: auto-approves (SSO match) →
→ redirect to client
Also applies to: 57-65
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/consent-flow-fix.md` around lines 49 - 53, The fenced code blocks
containing the flow diagrams (the OTP verify → ... sequences) are missing a
language identifier which triggers markdownlint MD040; update those backtick
fences around the flow diagrams (the two blocks showing the OTP/auth/consent →
... sequences) to include a language specifier such as "text" (i.e., change ```
to ```text) so the blocks pass MD040; ensure both occurrences referenced in the
comment (the block around the OTP verify → /auth/complete ... sequence and the
block around the /oauth/epds-callback → create account ... sequence) are
updated.
- Add handle.test.ts with 13 tests for validateLocalPart() - Add migration v8 tests verifying client_logins table is dropped - Ratchet coverage thresholds up (statements: 21, branches: 15, functions: 35, lines: 20) to reflect new coverage floor - Document coverage ratcheting policy in AGENTS.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared/src/__tests__/db.test.ts (1)
229-234: Use an exact schema version assertion for this v8 migration test.At Line 233,
>= 8can silently pass after future migrations, even if this test is not updated. Since the migration runner sets a precise version per applied migration (packages/shared/src/db.tslines 186-189), this test should assert the exact expected version for this snapshot.Proposed test hardening
- it('schema version is at least 8 after migration', () => { + it('schema version is exactly 8 after migration', () => { const row = db['db'] .prepare('SELECT version FROM schema_version') .get() as { version: number } - expect(row.version).toBeGreaterThanOrEqual(8) + expect(row.version).toBe(8) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/__tests__/db.test.ts` around lines 229 - 234, The test "schema version is at least 8 after migration" currently uses expect(row.version).toBeGreaterThanOrEqual(8) which can mask future changes; update the assertion to require the exact schema version produced by the migration runner by replacing the >= check with expect(row.version).toBe(8) (i.e., assert the precise value returned by db['db'].prepare('SELECT version FROM schema_version').get()) so the test fails if the migration runner (the code that sets the precise version) changes without updating the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/shared/src/__tests__/db.test.ts`:
- Around line 229-234: The test "schema version is at least 8 after migration"
currently uses expect(row.version).toBeGreaterThanOrEqual(8) which can mask
future changes; update the assertion to require the exact schema version
produced by the migration runner by replacing the >= check with
expect(row.version).toBe(8) (i.e., assert the precise value returned by
db['db'].prepare('SELECT version FROM schema_version').get()) so the test fails
if the migration runner (the code that sets the precise version) changes without
updating the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc80c47e-15c6-421a-990e-1caa7b05109c
📒 Files selected for processing (4)
AGENTS.mdpackages/shared/src/__tests__/db.test.tspackages/shared/src/__tests__/handle.test.tsvitest.config.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@atproto/oauth-providerconsent UI (consent-view.tsx)epds-callbacknow redirects through/oauth/authorizeafter account creation, letting the upstream middleware handle consent with actual scopes/permissionSetsconsent.ts,consent.test.ts,hasClientLogin/recordClientLogin, and theclient_loginstable (v8 migration)complete.tsby removing consent branchingProblem
The auth-service consent page displayed "Read and write posts", "Access your profile", "Manage your follows" regardless of what OAuth scopes the client actually requested. It also used its own
client_logintable instead of the atproto provider's built-inauthorizedClientstracking.How it works now
After OTP verification and account creation (if new user),
epds-callbackcallsupsertDeviceAccount()to bind the device session, then redirects to the stock/oauth/authorizeendpoint. The upstreamoauthMiddleware:dev-idcookieprovider.authorize()which checkscheckConsentRequired()against actual scopespermissionSetsDesign doc
See
docs/design/consent-flow-fix.mdfor the full analysis, implementation plan, and research findings about the middleware's device session handling and auto-approve conditions.Net change
-411 lines (213 added, 624 removed)
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores