Skip to content

fix: reuse the popup for the interactive retry — no more blocked second window / 'Open new window' dialog#13

Open
jeswr wants to merge 2 commits into
mainfrom
fix/popup-retry-window-reuse
Open

fix: reuse the popup for the interactive retry — no more blocked second window / 'Open new window' dialog#13
jeswr wants to merge 2 commits into
mainfrom
fix/popup-retry-window-reuse

Conversation

@jeswr

@jeswr jeswr commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Problem

DPoPTokenProvider always tries prompt=none first, so a fresh login is: silent attempt → login_required → interactive retry. AuthorizationCodeFlow closed the popup on every callback message, so the retry had to open() a new window — outside the original click's (already consumed) user activation. Popup blockers stop that, stranding the user in the "User interaction needed to launch authorization code flow in new window → Open new window" dialog on every first login. This is the "unnecessary accepting of prompts to redirect" users report.

Change

Keep the popup open when the callback message carries one of the OIDC "user must interact" errors (login_required / interaction_required / consent_required) — exactly the errors token providers retry interactively right away. The retry's open(uri, sameWindowName) then merely navigates the existing named window, which browsers always allow without user activation.

Result: the user sees one popup that goes from the silent bounce straight to the IdP login page — no blocked window, no interstitial dialog, no extra click.

A code response or a terminal error (e.g. access_denied when the user denies consent) still closes the window exactly as before; cancel/abort paths are unchanged.

Verification

  • npm run build (tsc): clean.
  • The repo has no DOM test setup, so this is verified live: a Playwright-driven login (Chrome popup blocker enabled, --disable-popup-blocking removed) against a deployed pod manager + Solid-OIDC broker (https://app.solid-test.jeswr.org). Before: silent popup closes → second window needed. After: one popup, prompt=none bounce navigates in place to the Keycloak login, flow completes with zero interstitial dialogs.

Future work (out of scope here): running the prompt=none attempt in a hidden iframe would remove the popup flash for purely silent re-auth; that needs a callback.html contract change (parent vs opener), so it is left for a separate discussion.

🤖 Generated with Claude Code

The DPoP provider always tries `prompt=none` first, so a fresh login is:
silent attempt → `login_required` → interactive retry. The popup element
closed the window on EVERY callback message, so the retry had to open a
NEW window — outside the original click's (already consumed) user
activation. Popup blockers stop that, stranding the user in the "User
interaction needed … Open new window" dialog on every first login (and
on slower connections the first open can be blocked too).

Now the popup is kept open when the callback carries one of the OIDC
"user must interact" errors (login_required / interaction_required /
consent_required) — exactly the errors token providers retry right away.
The retry's `open()` then merely NAVIGATES the existing named window,
which browsers always allow. Users see one popup that goes from the
silent bounce straight to the login page, with no interstitial dialog.

A code response or a terminal error (e.g. access_denied when the user
denies consent) still closes the window as before, as do cancel/abort.

Verified live with a Playwright-driven login (Chrome popup blocker
enabled) against a deployed app + Solid-OIDC broker.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 11, 2026 20:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the AuthorizationCodeFlow popup lifecycle so that when a silent prompt=none attempt returns an OIDC “user must interact” error (login_required, interaction_required, consent_required), the popup is kept open. This enables the subsequent interactive retry to reuse (navigate) the same named popup window, avoiding popup-blocker failures that occur when trying to open a second window without user activation.

Changes:

  • Keep the authorization popup open on OIDC “interaction needed” error callbacks to allow interactive retry to navigate the existing named window.
  • Add a helper (needsInteraction) to detect the relevant OIDC error responses from the callback URL.

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

Comment on lines 189 to +193
const onMessage = (message: MessageEvent) => {
signal.removeEventListener("abort", onAbort)
this.#switchModal.close()
this.#authorizationWindow?.close()

// When the server answered a silent (`prompt=none`) attempt with a "user

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1ce81c1: the listener now ignores any message whose source is not the authorization popup, and removes itself on the matching message instead of using { once: true }.

Review follow-up: filter the window message listener by
message.source === the popup, so unrelated (or hostile) postMessage
traffic can neither resolve the flow early nor spoof an authorization
response. The listener now removes itself on the matching message
instead of using { once: true }.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
langsamu pushed a commit that referenced this pull request Jun 12, 2026
@langsamu

Copy link
Copy Markdown
Collaborator

@jeswr,

  1. I have taken the second commit to main in a3c618c because it's independently good.
  2. I like the improved, seamless UX. But I don't like that the UI element duplicates functionality from the token provider. Both now check for the error messages. I'd like to think a bit more about this.
  3. Is it correct to start with prompt=none? I see differences between the authorization servers: Some require interaction, others don't. Maybe I was doing something wrong in the journey?

@jeswr

jeswr commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@langsamu thanks — answering your three points, with behavioural evidence rather than guessing where it's a runtime question.

1. Taking the second commit to main (a3c618c). 👍 Agreed it stands alone; this PR is now just the popup-reuse change on top of it.

2. The UI element duplicating the token provider's error-message checks. This is a fair observation and I don't want to paper over it — it is genuine duplication. Both AuthorizationCodeFlow#needsInteraction (UI) and DPoPTokenProvider#authenticate (provider) independently classify the same three OIDC errors (login_required / interaction_required / consent_required). They do it for different reasons — the UI needs to decide whether to keep the popup open for the retry, the provider needs to decide whether to retry at all — but the predicate is the same, so a server that returned a fourth "interaction" error, or a change to the set, would need editing in two places. I agree it's worth thinking about before merge rather than rushing it; happy to factor the predicate into one shared, exported helper (e.g. in the provider, consumed by the UI) if you'd like that direction. I've left this unresolved deliberately — flagging it as a real design point, not closing it.

3. "Is it correct to start with prompt=none? Some servers require interaction, others don't."

Short answer: yes, starting with prompt=none is correct — it's the OIDC "silent authentication" pattern (OIDC Core §3.1.2.1). The differences you saw are expected and handled, and they come from session state at the AS, not from a server doing something wrong.

prompt=none means "authenticate me only if you can do it without any UI":

  • If the user already has an active session at that AS → the AS returns a code with no interaction (silent success).
  • If they don't → the AS returns one of login_required / interaction_required / consent_required, and the code retries interactively (dropping prompt, or sending prompt=consent when we're opting into offline_access). See DPoPTokenProvider.ts#authenticate (the validateAuthResponse catch block) and AuthorizationCodeFlow#needsInteraction.

So "some require interaction, others don't" is the same server behaving differently depending on whether you already had a session — which is exactly the win prompt=none buys us (no popup when one isn't needed).

Evidence (Playwright, on branch threads/evidence, runs credential-free):

I sent an unauthenticated prompt=none authorization request — same params the provider sends (PKCE/S256, scope=openid webid) — to three real servers and captured what each returns. Test: prompt-none.spec.ts; helper: oidc.ts.

Server cold prompt=none (no session) →
solidcommunity.net (CSS / oidc-provider) error=interaction_required"An account cookie is required."
login.inrupt.com (ESS) error=interaction_required"Interaction required to proceed"
idp.solid-test.jeswr.org (the broker) error=login_required"End-User authentication is required"

All three decline the silent attempt, and the only difference is which of the three error strings they use (CSS/ESS say interaction_required, the broker says login_required) — both already in the retry set, so the flow handles all three. Run output: 6 passed (3 skipped).

Two accuracy caveats I want to be straight about:

  • The "warm" half — an existing AS session making prompt=none return a code silently — I could not test autonomously, because it needs a real interactive login (credentials I don't have here). It's left as a documented skipped test in warm-session.spec.ts with the scaffolding in place, rather than faked. That's the half that produces the "doesn't require interaction" outcome you saw, so if your journey hit a server that didn't prompt, you most likely already had a session there.
  • One incidental finding: both CSS and the broker reject prompt=none without PKCE with invalid_request ("policy requires PKCE"), before getting to the interaction check — see probe-prompt-none.out vs probe-prompt-none-pkce.out. The real client always sends PKCE when code_challenge_methods_supported is present, so this doesn't affect us; just noting it so the error you observe is reproducible.

Also recorded the discovery metadata the provider keys its behaviour off (refresh_token grant, offline_access scope, PKCE, DPoP algs) for all three in discovery.spec.ts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants