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
Coverage Report for CI Build 24210964488Coverage increased (+1.8%) to 31.462%Details
Uncovered Changes
Coverage Regressions6 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - 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
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
|
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).
The previous scenarios toggled PDS_SIGNUP_ALLOW_CONSENT_SKIP and PDS_OAUTH_TRUSTED_CLIENTS as if they were per-scenario state, but those are PDS env vars read at startup — a single e2e run against a single deployment cannot exercise both on and off paths for them. The pr-base environment is now configured with the PDS flag on, both demo clients opted in via metadata, and only one of the two demo clients listed in PDS_OAUTH_TRUSTED_CLIENTS. That leaves the trusted/untrusted distinction as the one dimension that actually varies per scenario, so rewrite the scenarios around that: - trusted demo client + new user → consent skipped - untrusted demo client + new user → consent shown - consent skipped at sign-up does not carry over to a second client The env-var and metadata-flag off-paths belong in unit tests, not e2e.
@atproto/[email protected] renders the consent buttons as "Authorize" and "Deny access", not "Approve" and "Deny". The existing step definitions and feature file were still asserting the old labels from the custom auth-service consent page this PR is replacing, so scenarios that reach the consent screen could not pass end-to-end against the upstream UI. - Update step defs in auth.steps.ts and consent.steps.ts to click "Authorize" instead of "Approve" - Update features/consent-screen.feature button labels to match - Strengthen "a consent screen is displayed" to also assert the permissions-request preamble text and at least one <code>-rendered scope from the upstream consent-form view, not just the presence of the Authorize button Also plumb EPDS_CLIENT_NAME through packages/demo/client-metadata.json so trusted and untrusted demo instances can report distinct client_name values. This is needed for upcoming e2e scenarios that differentiate the two demo clients on the consent screen; without it both clients render as "ePDS Demo" and cannot be told apart.
The previous assertion only checked for the presence of the Authorize button and a single `atproto` substring, which would have accepted a consent page that was missing scopes entirely or was silently asking for more than intended. Since we fully control both demo clients and know they request exactly `atproto transition:generic`, the test can and should assert the exact set of rendered <code> scope items. This catches both under-asking (PDS dropping a requested scope) and over-asking (PDS adding unrequested scopes to the consent form).
…d CI Add a second demo-client URL to the e2e test configuration to support consent-skip scenarios that differentiate trusted vs. untrusted clients. The existing E2E_DEMO_URL continues to point at the trusted demo (and is exposed as both `demoUrl` and `demoTrustedUrl` on testEnv) so existing scenarios keep working without changes. - e2e/support/env.ts: add demoTrustedUrl and demoUntrustedUrl - e2e/.env.example: document the new variable - .github/workflows/e2e-pr.yml: derive the untrusted demo URL from the deployment's env slug using Railway's standard slug rule (strip @, strip /, replace spaces with -, lowercase), wait for it alongside the other services, and pass it to the e2e runner
The helper was defined privately inside auth.steps.ts but needs to be reusable from consent.steps.ts too, now that consent-screen scenarios distinguish trusted and untrusted demo clients and want to assert which demo the user landed on after a valid session. Lift it into e2e/support/utils.ts and add an optional expectedDemoUrl parameter that asserts the post-redirect URL's origin matches the given demo client. Omitting the parameter preserves the existing behaviour (any demo origin accepted), so the existing "redirected back to the demo client with a valid session" step keeps working unchanged.
Wire up the step definitions and browser flows for the three sign-up consent-skip scenarios added earlier: - "New user skips consent when signing up via a trusted client" drives the full sign-up via createAccountViaOAuth against the trusted demo and verifies the user lands on /welcome without a consent screen - "New user still sees consent when signing up via an untrusted client" uses a new startSignUpAwaitingConsent flow helper that drives sign-up up to (but not through) the consent screen, so the scenario can then assert the consent page's contents and click Authorize itself - "Sign-up consent skip does not carry over to a second client" signs up via the trusted demo (exercising the skip), resets the browser context to drop cookies, then re-authenticates via OTP through the untrusted demo and asserts consent is shown — verifying that the scope authorisation recorded for the trusted client does not leak to the untrusted client Also tightens the Gherkin scenarios: the strengthened "a consent screen is displayed" step already asserts scopes and preamble text, so the redundant "shows the OAuth scopes" lines are removed to keep each scenario focused on its distinct assertion. Follow-up TODO (separate PR): the "later initiates OAuth login via the untrusted demo client" step duplicates OTP-login boilerplate with "a returning user has already approved the demo client" in auth.steps.ts. Extract a shared driveOtpLogin(world, email, demoUrl) helper to eliminate the overlap.
Every test in client-metadata.test.ts was open-coding the same
vi.fn().mockResolvedValue({ ok, json: () => Promise.resolve(...) })
boilerplate, plus the `as unknown as typeof fetch` cast, plus a
manual globalThis.fetch assignment. Lift those three shapes into
mockFetchOk / mockFetchNotOk / mockFetchReject helpers so each test
only states what matters to it (the URL, the body, the assertion).
…etadata is discoverable' The step was misleadingly named: in atproto OAuth there is no client registration handshake, and the step definition only fetches the demo service's client-metadata document and checks it's reachable with a valid client_id field. A client is "known" to the PDS purely by virtue of its metadata being discoverable when the PDS dereferences the client_id URL during an authorize request. Rename to "the demo OAuth client's metadata is discoverable" across all five feature files, which accurately describes what the step checks without referring to the literal filename (feature files are read by non-technical reviewers too). Also introduce trusted/untrusted variants of the step for the consent-screen feature's Background: both demo services need to be reachable for the new consent-skip scenarios, but previously the Background only verified the trusted demo, leaving untrusted failures to surface deep inside a scenario rather than at startup. The three step definitions share a common assertClientMetadataDiscoverable helper so there's only one place to update if the metadata contract ever grows new required fields.
Before the PDS_SIGNUP_ALLOW_CONSENT_SKIP feature, sign-up via the trusted demo created the account but did not record an authorized client, so a returning user's first login to the same client would show the consent screen. The passwordless-authentication scenario "Returning user authenticates with email OTP" was written against that assumption and included "And the user approves the consent screen" as a step. With the consent-skip feature in this PR, sign-up via the trusted demo now calls setAuthorizedClient as part of the skip path — so the returning user's login finds the client already authorized and redirects straight to /welcome without a consent stop. The step then times out waiting for an Authorize button that never appears. Fix: - Remove the stale consent-approval step from the scenario. - Simplify "a returning user has already approved the demo client" helper: delete its Step 2 which manually drove a post-signup login to click Authorize and record the authorisation. That work is now done server-side during sign-up itself, so Step 2 was a no-op waiting on a consent button that wouldn't render, stalling until its 30 s timeout. - Update the helper docstrings to describe the new behaviour so future readers don't reintroduce approve-consent steps on top.
The E2E tests workflow normally fires itself off Railway's deployment_status webhook, so for everyday PR work nothing special is needed. But two situations require a manual trigger — e2e-only changes that don't cause a Railway rebuild, and re-running against an existing environment without a new commit — and the invocation has one non-obvious footgun: `gh workflow run` without `--ref` defaults to the repository default branch, so you silently test old feature files and step definitions against the right Railway environment, producing confusing results that look like real test failures. Document the correct invocation (--ref + -f env_name) in both e2e/README.md (primary developer-facing doc, with full context on env-name formats, URL derivation, and how to handle missing Railway domains) and AGENTS.md (short pointer so agents don't repeat the mistake, with a link to the detailed section).
The "Existing user sees consent screen for a new client" and "User denies consent" scenarios both signed up via the trusted demo (inside the `a returning user has a PDS account` Given) and then logged in to the same trusted demo, expecting to hit a consent screen on the return login. Before PDS_SIGNUP_ALLOW_CONSENT_SKIP that worked because sign-up didn't record authorisation; now sign-up auto-authorises the trusted client via setAuthorizedClient, so the return login finds the client already authorised and redirects straight to /welcome — no consent screen, scenarios fail waiting on an Authorize button that never appears. The scenario name explicitly says "for a new client", so the fix is to make the login genuinely hit a new client: switch the When step to the untrusted demo, which the user has never touched. Untrusted clients don't satisfy the three-condition consent-skip check, so the upstream oauth-provider renders the real consent screen on first login — exactly what the scenarios want to test. - Add `When the untrusted demo client initiates an OAuth login` step def (navigates to testEnv.demoUntrustedUrl). - Replace the old `it shows the demo client's name` with `it shows the untrusted demo client's name`, extracting a shared assertClientNameVisibleFromMetadata helper so the logic isn't duplicated. - Update the two affected consent-screen scenarios to use the untrusted demo for the login and the redirect-back assertion. - Add a short explanatory comment above the scenarios so future readers don't revert them to "the demo client" thinking it's more consistent with the rest of the suite.
The previous "a consent screen is displayed" step asserted the preamble text "This application is requesting the following list of technical permissions" and the exact raw scope strings (`atproto`, `transition:generic`) rendered as <code> list items. That failed at runtime because both live inside a collapsed <Admonition> panel with the HTML `hidden` attribute and aria-hidden="true" — the upstream oauth-provider-ui's DescriptionCard only reveals them when the user clicks a "Technical details" disclosure button. Playwright correctly reported them as hidden. Replace with a high-level, user-facing check: assert that the permission card titled "Authenticate" is visible. That card is rendered unconditionally (not inside a collapsed panel) by the RpcMethodsDetails component when the scope contains transition:generic, so it's the human-readable equivalent of the scope our demo clients request. This proves the scope was parsed and summarised to the user without depending on the details-panel implementation or requiring the test to enumerate every possible scope-card variant.
@atproto/oauth-provider-ui's <ClientName> component only renders the self-declared client_name for clients listed in the PDS's trusted allowlist. For untrusted clients it falls through to <UrlViewer> and renders the client_id URL's host, so users identify the app by its domain rather than a self-declared name that could be misleading. This is a deliberate security property of the upstream UI. The previous step "it shows the untrusted demo client's name" asserted the client_name value from the demo's client-metadata.json was visible on the consent page — which can never be true for an untrusted client. Replace with "it identifies the untrusted demo client by its URL host": derives the host from E2E_DEMO_UNTRUSTED_URL and asserts that string is visible. Asserting the host is present is also a security-relevant check: it proves the upstream "untrusted → show URL, not name" guard is working and that the PDS is correctly classifying the demo client as untrusted (not leaking it onto the trusted allowlist accidentally).
The "User denies consent" scenario asserted the browser was "redirected to the PDS with an access_denied error" and waited for a URL matching `**/oauth/authorize**error=access_denied**`. That's incorrect on two counts. First, per RFC 6749 §4.1.2.1, denying consent causes the authorization server to redirect back to the CLIENT's redirect_uri with `error=access_denied`, not to keep the user on the PDS's /oauth/authorize endpoint. The browser was never supposed to land at the asserted URL. Second, the demo client's callback route catches any `error` query parameter from the PDS, logs it generically, and redirects to its own landing page with `?error=auth_failed` (see packages/demo/src/app/api/oauth/callback/route.ts) — discarding the original OAuth error code in the process. So by the time Playwright's waitForURL fires, the final URL is the demo client's landing page, not the intermediate callback URL with access_denied. Rename the step to "redirected back to the untrusted demo client with an auth error" and assert against the actual final URL: <untrusted-demo-origin>/?error=auth_failed. Also update the step definition comment to explain the two-hop redirect so future readers don't try to "fix" it back to asserting access_denied.
…(HYPER-270) The "Returning user skips consent for a previously-approved client" scenario was using the trusted demo throughout, so both the sign-up (inside the Given) and the return login went through the trusted client. That path is covered by the sign-up consent-skip feature (PDS_SIGNUP_ALLOW_CONSENT_SKIP → setAuthorizedClient in pds-core/src/index.ts step 5), which auto-authorizes the client as a side-effect of sign-up. The scenario passed green purely because of that side-effect, not because the explicit click-Authorize → record-grant path actually works. HYPER-270 reports that for an untrusted client (where the consent skip does NOT fire, so the user really does see consent and click Authorize), the PDS does not persist the grant — so the return login shows consent again. The existing e2e scenarios did not exercise this path at all, so the green run did not cover HYPER-270. Rewrite scenario 43 to: - Sign up via the untrusted demo, land on the real consent screen, click Authorize explicitly, wait for /welcome, then reset the browser context. This is encapsulated in a new Given "a returning user has already approved the untrusted demo client" that's distinct from the trusted-demo-based Given still used by passwordless-authentication. - Initiate OAuth login via the untrusted demo again, enter email and OTP, and assert no consent screen is shown on return. If HYPER-270 is a live bug, this scenario will fail at "no consent screen is shown" on the second login — which is exactly the evidence needed to prove the previous green CI was a coverage gap, not real coverage.
Adds two protected GET endpoints guarded by both EPDS_INTERNAL_SECRET and EPDS_DEBUG_GRANTS=1: - /_internal/debug-grants?sub=<did> — dumps authorized_client and account_device rows for a given sub, letting us see whether a grant was persisted after a click-Authorize on an untrusted client and whether account_device was reattached on the return login. - /_internal/debug-recent-accounts?limit=<n> — lists the most recently-created actor rows so we can find the sub of the e2e test user without knowing its DID up front. These are temporary debugging aids for HYPER-270 and must be removed before merging PR #21. The double gating (internal secret + env var enable) means they don't ship to production-like environments by accident — the env var has to be explicitly set on whichever Railway environment we're investigating.
HYPER-270 reproduces reliably because @atproto/oauth-provider's request-manager.ts forcibly sets prompt=consent on every authorize request from a public client (token_endpoint_auth_method=none) that isn't in PDS_OAUTH_TRUSTED_CLIENTS and isn't first-party. As a deliberate security property of atproto OAuth, public untrusted clients cannot benefit from previously-stored consent grants — the stored grant is kept in authorized_client but checkConsentRequired always returns true because prompt=consent was injected. To make the demos exercise the "remember previous grants" path in e2e (and to give certified.app a working reference implementation for HYPER-270), switch the demo to confidential-client authentication when EPDS_CLIENT_PRIVATE_JWK is set in the environment: - New lib/client-jwk.ts (jose-based): parses the private JWK from env, derives the public JWK, derives a kid via calculateJwkThumbprint (RFC 7638), and signs client_assertion JWTs via SignJWT. Caches the imported signing key for the process lifetime. Falls back to returning null when the env var is unset, preserving public-client mode as the local-dev default. - New /jwks.json route: serves the public JWK wrapped in a JWKS envelope, 404s when the env var is unset so a public client doesn't accidentally advertise an empty keyset. - client-metadata.json/route.ts: conditionally declares token_endpoint_auth_method=private_key_jwt + ES256 + jwks_uri when the env var is set, falls back to token_endpoint_auth_method= none otherwise. - api/oauth/login/route.ts: signs a client_assertion and adds it to the PAR request body when the env var is set. The PDS's PAR endpoint enforces the same client-auth method as the token endpoint, so missing the assertion here produces "client authentication method private_key_jwt required a client_assertion". Regenerates the assertion on DPoP-nonce retry so its jti is fresh and the upstream replay store doesn't reject it as a duplicate. - api/oauth/callback/route.ts: same client_assertion addition to the token exchange POST, with matching jti-refresh on the DPoP-nonce retry path. Uses `jose` (newly added as a direct dep of packages/demo) rather than hand-rolling the JWT signing, for the same reason the upstream @atproto packages depend on it: getting JWS ES256 right (DER → raw r||s conversion, curve point padding, leading zero handling) is easy to get wrong and not something to reinvent. New client-jwk.test.ts (10 tests): covers parsing edge cases (unset, empty, invalid JSON, malformed JWK), public-JWK derivation, deterministic kid, spec-compliant JWT header and payload, a full sign-then-verify round-trip using jose's own jwtVerify against the derived public JWK, and fresh-jti-per-call behaviour.
Extends scripts/setup.sh with a generate_es256_private_jwk helper that calls Node's crypto module (not openssl, which doesn't emit JWK format directly) to produce a P-256 private JWK as a single-line JSON blob suitable for an env var. The helper runs once during setup_package_envs, only if EPDS_CLIENT_PRIVATE_JWK isn't already set in packages/demo/.env (so re-running setup preserves existing keypairs), and writes it with set_env_var in the same style as the existing secret generation. Also extends print_next_steps with a warning that each Railway demo service needs its OWN keypair — the .env value generated by setup.sh works for local dev and for ONE Railway service, but any additional demo services (e.g. the untrusted demo that exercises the consent-screen e2e scenarios) must generate an independent keypair or they could forge client_assertions claiming to be each other. Provides a copy-pasteable Node one-liner for generating further keypairs. Adds EPDS_CLIENT_PRIVATE_JWK documentation to packages/demo/.env.example with an explanatory comment linking back to HYPER-270 and noting the fall-back-to-public-client-mode behaviour when unset. At runtime, the private JWK is parsed and the signing key is derived via `jose` (see packages/demo/src/lib/client-jwk.ts) — the setup-time helper only needs to produce a syntactically valid JWK, not the full jose-blessed representation.
The first pass at confidential-client support passed the specific endpoint URL (parEndpoint / tokenUrl) as the `aud` claim on the client_assertion JWT. That fails PDS-side verification with "unexpected aud claim value" because upstream @atproto/oauth-provider explicitly checks `audience: this.issuer` — the authorization server's issuer identifier from its metadata, not the endpoint URL being called. See packages/oauth-provider/src/client/client.ts in atproto's repo. Fix: - Extend discoverOAuthEndpoints() in packages/demo/src/lib/auth.ts to return the `issuer` from /.well-known/oauth-authorization-server alongside the existing endpoint URLs. - Extend the OAuthSession cookie type with an `issuer` field so the callback route can use the same issuer value that login discovered. - For the email-based login path (no handle resolution), default the issuer to PDS_URL, which matches the atproto invariant that a PDS is its own authorization server. - Update the PAR client_assertion in login/route.ts and the token client_assertion in callback/route.ts (and both DPoP-nonce retry paths) to use `issuer` as the audience. Also adds docs/design/demo-oauth-library-refactor.md — a planning document for the follow-up PR that will replace all this hand-rolled client_assertion code (and the rest of the demo's hand-rolled OAuth primitives) with @atproto/oauth-client-node. This fix is a stepping stone: it unblocks HYPER-270's e2e coverage within PR #21's scope, and the refactor PR (tracked as ePDS#56) will clean it up.
|
This was referenced Apr 10, 2026
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