Skip to content

test(fe): run the signing-in session scenarios in a browser - #4287

Open
sea-snake wants to merge 148 commits into
test/session-e2e-fixturesfrom
test/session-e2e-signing-in
Open

test(fe): run the signing-in session scenarios in a browser#4287
sea-snake wants to merge 148 commits into
test/session-e2e-fixturesfrom
test/session-e2e-signing-in

Conversation

@sea-snake

@sea-snake sea-snake commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

The "Signing in" scenarios of docs/ongoing/session-test-scenarios.md — FIRST-1 and FIRST-3 — plus the per-app account, which the designs promise and no scenario there names.

What a sign-in leaves behind: an account the app acts as, a delegation to act with straight away rather than at the next foreground, and one sign-in per browser rather than one per attempt.

FIRST-1 lets authorizePage perform the ceremony, so its body is the provider's side and the panel is read in afterEach. The other two sign in twice, which that fixture does not do, so they drive the app themselves.

@sea-snake
sea-snake marked this pull request as ready for review August 26, 2026 12:55
@sea-snake
sea-snake requested a review from a team as a code owner August 26, 2026 12:55
@sea-snake sea-snake added feature:revocable-app-sessions Design: revocable app sessions feature:silent-reauth-redirect Design: silent re-auth over the redirect transport labels Aug 26, 2026
@sea-snake
sea-snake requested a lite review from Copilot August 26, 2026 12:55
@zeropath-ai

zeropath-ai Bot commented Aug 26, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to 0d64d5f.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► src/frontend/tests/e2e-playwright/routes/authorize/app-sessions/signing-in.spec.ts
    Add end-to-end tests for signing-in scenarios and session handling

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a new Playwright E2E spec to exercise the “Signing in” app-session scenarios in a real browser, validating what state a successful sign-in leaves in the test app (session/account/delegation) and how repeated sign-ins behave within the same browser context.

Changes:

  • Introduces signing-in.spec.ts covering FIRST-1 and FIRST-3 behavior plus a per-app account check.
  • Verifies (a) first sign-in yields session + delegation, (b) repeated sign-in replaces the session, and (c) the same identity derives different app accounts on different origins.
Suppressed comments (1)

src/frontend/tests/e2e-playwright/routes/authorize/app-sessions/signing-in.spec.ts:61

  • The comparison against here ?? "" has the same issue as above: it can hide a missing/empty account value and reduce test signal. It’s clearer to assert both accounts are non-empty and compare against a concrete string.
    await testApp.signIn(authenticate);
    const here = await testApp.account.textContent();


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@sea-snake
sea-snake force-pushed the test/session-e2e-signing-in branch from 6425fe6 to 6b872d5 Compare August 26, 2026 13:07
@sea-snake

Copy link
Copy Markdown
Contributor Author

Fixed. textContent() ?? "" was masking exactly what you describe — an empty session key would have made not.toHaveText("") pass for the wrong reason. TestApp.sessionKeyPrincipal() now asserts the app has one and returns a string, and expectSessionKeyOtherThan(before) does the comparison, so there is no nullable value in the scenario at all. Same treatment for the account read on the other line.

@sea-snake
sea-snake force-pushed the test/session-e2e-signing-in branch 2 times, most recently from 578e1c4 to 6f4a991 Compare August 26, 2026 13:36
@sea-snake
sea-snake force-pushed the test/session-e2e-signing-in branch from 6f4a991 to f0da0f5 Compare August 26, 2026 13:53
@sea-snake
sea-snake removed this pull request from stack #4294 September 9, 2026 20:42
@sea-snake
sea-snake added this pull request to stack #4327 September 9, 2026 20:44
@sea-snake
sea-snake removed this pull request from stack #4327 September 11, 2026 14:36
@sea-snake
sea-snake added this pull request to stack #4330 September 11, 2026 14:36
sea-snake and others added 6 commits September 11, 2026 18:05
Signing in twice was asserted by the session key changing, which proves
only that it rotated — a second ceremony that had registered a second
browser would rotate one too. What the scenario claims is one entry for
that pairing, so that is what is counted.

The key is also no longer reported by the panel, so the check was resting
on a readout that has gone.

Read from a second browser, because a browser reading its own list finds
itself under no Sign out button: signing out the browser in front of you
goes through the identity's own sign-out instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
@sea-snake
sea-snake removed this pull request from stack #4330 September 11, 2026 20:47
@sea-snake
sea-snake added this pull request to stack #4332 September 11, 2026 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature:revocable-app-sessions Design: revocable app sessions feature:silent-reauth-redirect Design: silent re-auth over the redirect transport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants