[HYPER-178] use upstream OAuth consent UI instead of broken auth-service reimplementation#21
Open
[HYPER-178] use upstream OAuth consent UI instead of broken auth-service reimplementation#21
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR restructures the ePDS OAuth consent flow by removing the auth-service's custom consent endpoint and redirecting authentication through the stock OAuth middleware. Key changes include deleting the consent router and related tests, simplifying the complete handler to redirect to Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AuthService as auth-service
participant PDSCore as pds-core
participant OAuthProvider as oauth-provider<br/>(middleware)
participant Client
Note over Browser,Client: Previous Flow (Being Removed)
Browser->>AuthService: POST /auth/complete (existing user)
AuthService->>AuthService: Check if new account
AuthService->>Browser: Redirect to /auth/consent
Browser->>AuthService: GET /auth/consent
AuthService->>AuthService: Query auth_flow by flow_id
AuthService->>Browser: Render consent form
Browser->>AuthService: POST /auth/consent (approve)
AuthService->>AuthService: recordClientLogin(email, clientId)
AuthService->>AuthService: Sign HMAC callback params
AuthService->>PDSCore: Redirect to /oauth/epds-callback<br/>(pre-approved)
PDSCore->>PDSCore: Issue authorization code
PDSCore->>Client: Redirect with code & state
Client->>OAuthProvider: Exchange code for token
OAuthProvider->>Client: Return access token
sequenceDiagram
participant Browser
participant AuthService as auth-service
participant PDSCore as pds-core
participant OAuthProvider as oauth-provider<br/>(middleware)
participant Client
Note over Browser,Client: New Flow (After PR)
Browser->>AuthService: POST /auth/complete (existing user)
AuthService->>AuthService: Check if new account
AuthService->>Browser: Redirect to /oauth/epds-callback<br/>(approved=1)
Browser->>PDSCore: GET /oauth/epds-callback
PDSCore->>PDSCore: Upsert device-account
PDSCore->>Browser: Redirect to /oauth/authorize<br/>(request_uri, client_id)
Browser->>OAuthProvider: GET /oauth/authorize
OAuthProvider->>OAuthProvider: Resolve PAR request
OAuthProvider->>OAuthProvider: Compute consent requirement
OAuthProvider->>Browser: Render consent form<br/>(if needed) or auto-approve
Browser->>OAuthProvider: Approve (if needed)
OAuthProvider->>OAuthProvider: Record consent & issue code
OAuthProvider->>Client: Redirect with code & state
Client->>OAuthProvider: Exchange code for token
OAuthProvider->>Client: Return access token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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)
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
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
aspiers
force-pushed
the
fix/consent-use-upstream-oauth-ui
branch
from
9a4c5c3 to
48c192c
Compare
Pull Request Test Coverage Report for Build 23526996486Details
💛 - Coveralls |
aspiers
force-pushed
the
fix/consent-use-upstream-oauth-ui
branch
from
48c192c to
5320798
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/design/consent-flow-fix.md (2)
99-99:⚠️ Potential issue | 🟡 MinorUse the exact table name
client_loginsfor consistency.This line says
client_login(singular), but the actual table name in the schema and migration isclient_logins(plural). Line 17 correctly uses the plural form.📝 Suggested fix
-- The `client_login` table (add a migration to drop it) +- The `client_logins` table (add a migration to drop it)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/consent-flow-fix.md` at line 99, The document references the wrong table name `client_login`; update the text to use the exact table name `client_logins` everywhere (e.g., the line that currently reads "The `client_login` table (add a migration to drop it)") so it matches the schema and migrations; ensure any related mention (migration note) and section headings use `client_logins` for consistency with the schema and other lines like line 17.
135-135:⚠️ Potential issue | 🟡 MinorUse the exact table name
client_loginsfor consistency.Same issue here—should be
client_logins(plural) to match the actual table name.📝 Suggested fix
-- No more separate `client_login` table +- No more separate `client_logins` table🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/consent-flow-fix.md` at line 135, The document currently refers to the table name as "client_login" but the actual table is named "client_logins"; update any occurrences of the singular identifier "client_login" to the exact plural "client_logins" (e.g., change the text snippet "- No more separate `client_login` table" and any other mentions) so all references match the real table name and remain consistent across the codebase and docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/design/consent-flow-fix.md`:
- Line 99: The document references the wrong table name `client_login`; update
the text to use the exact table name `client_logins` everywhere (e.g., the line
that currently reads "The `client_login` table (add a migration to drop it)") so
it matches the schema and migrations; ensure any related mention (migration
note) and section headings use `client_logins` for consistency with the schema
and other lines like line 17.
- Line 135: The document currently refers to the table name as "client_login"
but the actual table is named "client_logins"; update any occurrences of the
singular identifier "client_login" to the exact plural "client_logins" (e.g.,
change the text snippet "- No more separate `client_login` table" and any other
mentions) so all references match the real table name and remain consistent
across the codebase and docs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fcb81a9-bd01-40c4-b2ee-450231b2895c
📒 Files selected for processing (11)
AGENTS.mddocs/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/__tests__/db.test.tspackages/shared/src/__tests__/handle.test.tspackages/shared/src/db.tsvitest.config.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
✅ Files skipped from review due to trivial changes (2)
- vitest.config.ts
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/tests/db.test.ts
- packages/shared/src/tests/handle.test.ts
- packages/pds-core/src/index.ts
aspiers
force-pushed
the
fix/consent-use-upstream-oauth-ui
branch
from
92f6cbc to
47c89ae
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/design/consent-flow-fix.md (1)
99-99:⚠️ Potential issue | 🟡 MinorUse the exact table name
client_loginsfor consistency.Line 99 uses
client_login(singular), but the actual table name in the schema isclient_logins(plural), as correctly referenced on line 17.📝 Suggested fix
-- The `client_login` table (add a migration to drop it) +- The `client_logins` table (add a migration to drop it)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/consent-flow-fix.md` at line 99, Update the migration/remove-table reference to use the correct table name `client_logins` (plural) instead of `client_login` so it matches the existing schema; locate the mention in the consent flow docs and any migration or drop-table statements that reference `client_login` and change them to `client_logins` (verify references in the migration that drops the table and any code or doc strings that name the table).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/design/consent-flow-fix.md`:
- Line 99: Update the migration/remove-table reference to use the correct table
name `client_logins` (plural) instead of `client_login` so it matches the
existing schema; locate the mention in the consent flow docs and any migration
or drop-table statements that reference `client_login` and change them to
`client_logins` (verify references in the migration that drops the table and any
code or doc strings that name the table).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f05001bd-46f4-4ecd-9d49-1d76bb633a9d
📒 Files selected for processing (11)
AGENTS.mddocs/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/__tests__/db.test.tspackages/shared/src/__tests__/handle.test.tspackages/shared/src/db.tsvitest.config.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
✅ Files skipped from review due to trivial changes (5)
- AGENTS.md
- vitest.config.ts
- packages/shared/src/tests/db.test.ts
- packages/pds-core/src/index.ts
- packages/shared/src/tests/handle.test.ts
aspiers
changed the title
…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
- 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
aspiers
force-pushed
the
fix/consent-use-upstream-oauth-ui
branch
from
47c89ae to
1918adb
Compare
|
🚅 Deployed to the ePDS-pr-21 environment in ePDS
|
…OW_CONSENT_SKIP) Move resolveClientMetadata to shared package so pds-core can resolve custom ePDS metadata fields. Add triple-gated consent-skip: env var + trusted client + client metadata flag. When not skipping, new users get prompt=consent to bypass unnecessary account selection screen. Also adds AGENTS.md debugging guidance.
The stock /oauth/authorize middleware reads prompt from the stored PAR parameters, not the URL query string. Patching via store.updateRequest() so new accounts skip account selection and go straight to consent.
The stock authorize UI always shows account selection regardless of the prompt parameter. Removed the dead code. The consent-skip path for trusted clients (which issues the code directly) is the only way to bypass the stock UI.
The stock authorize UI auto-advances past account selection when a session has selected=true, which requires login_hint to match the account. For new accounts, patch the stored PAR parameters to set login_hint to the new account's DID so the UI goes straight to the consent screen.
Existing users also need login_hint to skip account selection.
Only includes epds_skip_consent_on_signup in client metadata when the env var is explicitly set to 'true'.
…hresholds Coverage increased from 29% to 33% (statements). New tests cover resolveClientMetadata, resolveClientName, caching, fallback behavior, and ePDS extension fields (epds_skip_consent_on_signup, brand_color).
|
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
2 participants
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
Refactor