Skip to content

feat(oauth): add account selection dialog for existing sessions#35

Merged
sirily11 merged 7 commits into
mainfrom
copilot/show-confirmation-dialog-on-oauth
Mar 11, 2026
Merged

feat(oauth): add account selection dialog for existing sessions#35
sirily11 merged 7 commits into
mainfrom
copilot/show-confirmation-dialog-on-oauth

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 11, 2026

When a user is already signed in and an OAuth client initiates authorization, the flow now presents an account selection page before proceeding to consent/auto-approve.

Flow

/api/oauth/authorize → (user signed in, no account_confirmed param)
  → /oauth/account-select
    → "Continue as user@example.com" → /api/oauth/authorize?...&account_confirmed=true
      → sign-in permission check (none/whitelist rejection happens here)
      → normal flow (consent/auto-approve)
    → "Use a different account" → destroySession() → /login?redirect=...

Changes

  • app/api/oauth/authorize/route.ts — redirect signed-in users to /oauth/account-select unless account_confirmed is set; sign-in permission checks (none/whitelist) happen after account selection so users always see the account chooser first
  • app/oauth/account-select/page.tsx — server component: validates params, fetches user email + client name
  • components/auth/account-select-card.tsx — client component with two options: continue with current account or sign out and switch
  • actions/oauth/account-select.tsswitchAccount server action: destroys session, redirects to login preserving OAuth params
  • E2E tests — added handleAccountSelection helper to test-helpers.ts, updated all 5 affected OAuth test files (consent, public-client, refresh-token, wildcard-redirect, user-id-consistency) and all 4 sign-in-permission OAuth flow tests
  • Unit tests — covers switchAccount action (session destruction ordering, redirect URL construction)

Screenshot


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 11, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
rxlab-auth Ready Ready Preview, Comment Mar 11, 2026 10:08am

Request Review

- New page at /oauth/account-select showing current user email
- Options to continue with current account or switch to a different account
- Switching account destroys session and redirects to login
- Modified /api/oauth/authorize to redirect to account-select when user is signed in
- Added unit tests for switchAccount server action

Co-authored-by: sirily11 <32106111+sirily11@users.noreply.github.com>
Co-authored-by: sirily11 <32106111+sirily11@users.noreply.github.com>
… simplify test mocks

Co-authored-by: sirily11 <32106111+sirily11@users.noreply.github.com>
Copilot AI changed the title [WIP] Add confirmation dialog for OAuth account selection Add OAuth account selection dialog for existing sessions Mar 11, 2026
@sirily11 sirily11 marked this pull request as ready for review March 11, 2026 08:26
Copilot AI review requested due to automatic review settings March 11, 2026 08:26
@autopilot-project-manager autopilot-project-manager Bot changed the title Add OAuth account selection dialog for existing sessions feat(oauth): add account selection dialog for existing sessions Mar 11, 2026
@sirily11
Copy link
Copy Markdown
Contributor

@copilot screenshot for new account switcher screen

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an OAuth “account selection” step when an authorization request is initiated while a session exists, allowing the user to proceed with the current account or sign out and use another account before consent/auto-approval.

Changes:

  • Redirect /api/oauth/authorize to a new /oauth/account-select page unless account_confirmed is present.
  • Introduce account-select UI (server page + client card) and a switchAccount server action to sign out and route to login preserving OAuth params.
  • Update OAuth E2E specs with a shared handleAccountSelection helper, and add unit tests for switchAccount.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
app/api/oauth/authorize/route.ts Adds redirect to account selection step before continuing OAuth flow
app/oauth/account-select/page.tsx New server page to validate params, require session, and fetch user/client data
components/auth/account-select-card.tsx New client UI offering “continue” vs “use different account” actions
actions/oauth/account-select.ts New server action to destroy session and redirect to login with preserved OAuth params
actions/oauth/account-select.test.ts Unit tests for switchAccount behavior and ordering
e2e/fixtures/test-helpers.ts Adds handleAccountSelection helper used by multiple OAuth E2E specs
e2e/oauth/consent.spec.ts Updates tests to invoke account selection helper during flow
e2e/oauth/public-client.spec.ts Updates tests to invoke account selection helper during flow
e2e/oauth/refresh-token.spec.ts Updates tests to invoke account selection helper during flow
e2e/oauth/wildcard-redirect.spec.ts Updates tests to invoke account selection helper during flow
e2e/oauth/user-id-consistency.spec.ts Updates tests to invoke account selection helper during flow

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

await destroySession();

const searchParams = new URLSearchParams(oauthParams);
const loginUrl = `/login?redirect=/api/oauth/authorize?${searchParams.toString()}`;
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

switchAccount builds /login?redirect=/api/oauth/authorize?... without URL-encoding the nested redirect value. This will break parsing on /login (everything after the first & becomes separate top-level query params), causing OAuth params to be dropped/truncated. Build the login URL with URL/URLSearchParams (or encodeURIComponent the redirect value) so the entire /api/oauth/authorize?... string is preserved in the single redirect param.

Suggested change
const loginUrl = `/login?redirect=/api/oauth/authorize?${searchParams.toString()}`;
const redirectTarget = `/api/oauth/authorize?${searchParams.toString()}`;
const loginUrl = `/login?redirect=${encodeURIComponent(redirectTarget)}`;

Copilot uses AI. Check for mistakes.
Comment on lines +50 to +55
const redirectUrl = mockRedirect.mock.calls[0][0];
expect(redirectUrl).toContain("/login?redirect=");
expect(redirectUrl).toContain("/api/oauth/authorize");
expect(redirectUrl).toContain("client_id=test-client");
expect(redirectUrl).toContain("scope=openid+profile");
});
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

These tests only assert that the redirect URL string contains certain substrings, so they won’t catch the broken nested query-string issue. Add assertions that redirect is properly URL-encoded (e.g. redirect=%2Fapi%2Foauth%2Fauthorize%3F...) and that all OAuth params survive inside the decoded redirect value.

Copilot uses AI. Check for mistakes.
Comment on lines +111 to 129
// If user is already signed in and hasn't confirmed their account,
// redirect to account selection page
const accountConfirmed = searchParams.get("account_confirmed");
if (!accountConfirmed) {
const accountSelectUrl = new URL("/oauth/account-select", request.url);
accountSelectUrl.searchParams.set("client_id", client_id);
accountSelectUrl.searchParams.set("redirect_uri", redirect_uri);
accountSelectUrl.searchParams.set("response_type", "code");
accountSelectUrl.searchParams.set("scope", scope);
if (state) accountSelectUrl.searchParams.set("state", state);
accountSelectUrl.searchParams.set("code_challenge", code_challenge);
accountSelectUrl.searchParams.set("code_challenge_method", code_challenge_method);
if (nonce) accountSelectUrl.searchParams.set("nonce", nonce);
return NextResponse.redirect(accountSelectUrl);
}

// Check sign-in permission
if (client.signInPermission === "none") {
const redirectUrl = new URL(redirect_uri);
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

Account selection redirect happens before the signInPermission === "none" check. For clients where sign-in is disabled, users will be sent to account-select (and can even “switch accounts”) even though no account can ever authorize. Consider handling the none case before redirecting to account selection, since switching accounts cannot change the outcome.

Copilot uses AI. Check for mistakes.
Comment on lines +111 to +115
// If user is already signed in and hasn't confirmed their account,
// redirect to account selection page
const accountConfirmed = searchParams.get("account_confirmed");
if (!accountConfirmed) {
const accountSelectUrl = new URL("/oauth/account-select", request.url);
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

account_confirmed is read directly from the request query string, so an OAuth client can include it in the initial /api/oauth/authorize request to bypass the account selection step entirely. If account selection is meant to be enforced, store the confirmation server-side (e.g., in session) or require a server-generated/signed token tied to the account-select redirect rather than trusting a client-controlled query param.

Copilot uses AI. Check for mistakes.
Comment thread e2e/oauth/consent.spec.ts
);

// Should show consent page
// Should show account selection first, then consent page
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

This test comment says account selection should be shown first, but the test doesn’t assert that /oauth/account-select is actually reached (the helper is conditional). Consider asserting the account-select page is shown in at least one scenario where the user is already logged in, so a regression that skips account selection is caught.

Suggested change
// Should show account selection first, then consent page
// Should show account selection first, then consent page
await expect(page).toHaveURL(/\/oauth\/account-select/);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot screenshot for new account switcher screen

Here's the account switcher screen:

Account Select Screen

It shows the current signed-in user's email with a "Continue with this account" option, and a "Use a different account" option that signs out and redirects to login.

@blacksmith-sh

This comment has been minimized.

@sirily11
Copy link
Copy Markdown
Contributor

@copilot Found 4 test failures on Blacksmith runners:

Failures

Test View Logs
[chromium] › e2e/admin/
sign-in-permission.spec.ts:169:9 › Client Sign-in Permission › OAuth Flow Enforcement ›
should block sign-in when permission is set to none View Logs
[chromium] › e2e/admin/
sign-in-permission.spec.ts:217:9 › Client Sign-in Permission › OAuth Flow Enforcement ›
should block non-whitelisted user when permission is whitelist View Logs
[chromium] › e2e/admin/
sign-in-permission.spec.ts:265:9 › Client Sign-in Permission › OAuth Flow Enforcement ›
should allow whitelisted user when permission is whitelist View Logs
[chromium] › e2e/admin/
sign-in-permission.spec.ts:329:9 › Client Sign-in Permission › OAuth Flow Enforcement ›
should allow all users when permission is set to all View Logs
Fix in Cursor

These tests failed. Make sure you run e2e before commit

… and update E2E tests

Co-authored-by: sirily11 <32106111+sirily11@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot Found 4 test failures on Blacksmith runners:

Failures
...

Fixed in 2cb2a8e. The issue was that the account selection redirect happened before the sign-in permission checks, so tests expecting immediate access_denied redirects were instead being sent to the account selection page.

Changes:

  1. Moved signInPermission checks (none / whitelist) before the account selection redirect in app/api/oauth/authorize/route.ts — rejected users are now denied immediately without seeing the account selection page
  2. Added handleAccountSelection calls in the two sign-in-permission E2E tests where users ARE allowed to proceed (whitelist-allowed and all-users)

@sirily11
Copy link
Copy Markdown
Contributor

@copilot permission check should happened after account selection. May need to update tests accordingly

Co-authored-by: sirily11 <32106111+sirily11@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot permission check should happened after account selection. May need to update tests accordingly

Fixed in 5d6b122. Moved permission checks back to after account selection in the authorize route. The flow is now: authenticate → account selection → permission check → consent/auto-approve. Updated all 4 sign-in-permission E2E tests to handle the account selection step before the permission check occurs.

@sirily11 sirily11 merged commit a0064fa into main Mar 11, 2026
3 checks passed
@sirily11 sirily11 deleted the copilot/show-confirmation-dialog-on-oauth branch March 11, 2026 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants