Skip to content

Auth: Do not treat a boot-time token refresh rejection as a session timeout (closes #23462) - #23494

Merged
leekelleher merged 1 commit into
release/17.6from
v17/bugfix/23462-no-redirect-after-logout-login
Jul 28, 2026
Merged

Auth: Do not treat a boot-time token refresh rejection as a session timeout (closes #23462)#23494
leekelleher merged 1 commit into
release/17.6from
v17/bugfix/23462-no-redirect-after-logout-login

Conversation

@iOvergaard

Copy link
Copy Markdown
Contributor

What

Fixes #23462 — a 17.6.0-rc regression where signing out lands you on a "Your session has timed out" screen, and signing in from there never returns you to the backoffice.

Targeting release/17.6 because the regression is only on that line (introduced by #23079, which is in 17.6.0-rc but not in 17.5.3), so it would otherwise ship in 17.6.0.

Why it happens

Sign-out sends the browser to /umbraco/logout, a full page load, so the app boots and runs its normal session probe (setInitialState()/token, grant_type=refresh_token). There is no cookie any more, so OpenIddict answers 400 {"error":"invalid_grant"} — and since #23079, #performRefresh() treats any definitive rejection as an expired session and calls timeOut(), even when there was no session in the first place.

Both reported symptoms follow from that one call:

  1. timeoutSignal makes UmbAppAuthController open the login modal in timedOut state — hence the "Your session has timed out" wording on a deliberate sign-out.
  2. The timedOut state deliberately re-authenticates in a popup rather than redirecting, so a timed-out editor does not lose unsaved work, and on success it only closes the modal — navigation is the caller's job. Here the caller is the timeout observer, and the logout route has no auth guard, so nothing navigates. The popup closes and you are left on /umbraco/logout.

Two things kept this quiet:

  • An ordinary cold-boot login is unaffected, because on the wildcard route the auth guard fires its full-page login redirect immediately and wins the race against the 1s auditTime on timeoutSignal. Only the guard-less routes (logout, error, and the redirect-flow oauth_complete) are exposed.
  • BackOfficeLogout.spec.ts asserts the landing URL, then re-navigates to /umbraco and asserts the login page — it never signs in from the landing page.

The fix

#performRefresh() only times the user out when a session actually existed. A rejected refresh with nothing in hand means nobody is signed in, which the ordinary login redirect already handles. #23079's actual fix (live session, rejected refresh → time out) and the #sessionDead latch are untouched.

This restores the pre-17.6 behaviour on the sign-out landing page, and it is the same distinction the v19 cookie-auth work (#23484) makes explicitly: a cold boot has nothing to preserve and takes the full-page redirect, while a timed-out session gets the modal and the popup.

Testing

  • New unit test does not time the user out when there was no session to lose fails before the fix (expected 1 to equal 0) and passes after.
  • Corrected the existing times the user out on a definitive invalid_grant failure test: it asserted the timeout with no session established, which is exactly why it did not catch this. It now seeds a session via the sessionUpdate broadcast first, matching the Session timeout modal "Stay logged in" button does not prevent automatic logout #22986 scenario it exists for.
  • Full client suite green (2557 passed, 0 failed across 348 files). No new lint warnings.
  • Verified in a browser against a real backend with the production client build: with the pre-fix bundle the reported screen reproduces exactly; with the fix, sign-out lands on the ordinary login screen, sign-in does a full-page redirect and ends up in the backoffice. Cold-boot login unchanged.
  • Checked the genuine timeout still fires: revoked the refresh token in SQLite under a live session, then made an API call — the timeout modal still appears, over the backoffice as intended.

Follow-up

main (v18) carries the identical #performRefresh, so it needs this on the merge-up. v19's cookie-auth branch removes the machinery entirely, so nothing to do there.

🤖 Generated with Claude Code

A definitive /token rejection called timeOut() unconditionally, so the boot-time
refresh on the post-sign-out landing page (/umbraco/logout) raised a spurious
timeout. The app answered with the "session timed out" login modal, which
re-authenticates in a popup and leaves navigation to the caller — and the logout
route has no auth guard, so a successful sign-in went nowhere.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 28, 2026 11:01
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @iOvergaard's task in 2m 43s —— View job


PR Review

Target: origin/release/17.6 · Based on commit: 7d3895e4d876d88cea2b19e05c1451f8ecbe51ca

Fixes a v17.6 regression where #performRefresh() called timeOut() on any definitive invalid_grant rejection — including cold-boot refreshes with no existing session — by gating the timeout on whether a session was actually present before the rejection.

  • Other changes: Sign-out landing page (/umbraco/logout) now correctly shows the ordinary login screen instead of "Your session has timed out." Sign-in from that page performs a full-page redirect to the backoffice as expected.

No Critical or Important issues found. A few observations worth noting:

Suggestions

  • auth.context.ts:532: hadSession is captured synchronously before await this.#client.refreshToken(). This is the right place — the session could theoretically be set by a peer-tab sessionUpdate broadcast during the async call, but makeRefreshTokenRequest() already handles that case with an early-return guard before entering #performRefresh(), so there is no gap. Worth a brief word if the session-before-lock capture comment above is ever updated, just to tie the two together.

  • auth.context.test.ts:224: The new 'does not time the user out when there was no session to lose' test calls setInitialState(), which includes a 300 ms peer-session-request timeout before falling through to the refresh. This is intentional (it exercises the exact boot-time code path that triggered the regression), but reviewers should know the test is slower by design — roughly 300 ms per run.

  • auth.context.test.ts:197: The 'does not call /token again after a definitive invalid_grant failure' test intentionally omits a session seed, which is now correct: the #sessionDead latch is always set on a fatal rejection regardless of hadSession, so the test's assertion (fetchCalls length is 1) remains valid. No change needed.


Approved

The fix is minimal and precisely targeted, the logic is sound across all code paths (signed-out cold boot, live-session timeout, transient failure), and both tests are well-constructed — the corrected existing test seeds the session it needs, and the new test reproduces the exact regression scenario. No breaking changes.

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

Adjusts the backoffice auth refresh logic to avoid treating a boot-time refresh-token rejection (after explicit logout, when no session exists) as a session timeout, restoring the expected post-logout login/redirect flow on guard-less routes like /umbraco/logout.

Changes:

  • Only call timeOut() on fatal /token refresh rejections when a session was already established (hadSession).
  • Updates the existing invalid_grant timeout test to seed a session first (via sessionUpdate broadcast).
  • Adds a new unit test ensuring a boot-time invalid_grant with no session does not trigger timeout and leaves the context unauthorized.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Umbraco.Web.UI.Client/src/packages/core/auth/auth.context.ts Gate timeOut() behind a “session existed” check during fatal refresh rejection handling.
src/Umbraco.Web.UI.Client/src/packages/core/auth/auth.context.test.ts Aligns timeout behavior tests with real session establishment and adds coverage for the “no session to lose” boot path.

@iOvergaard iOvergaard added the status/regression A previously working feature that has broken or changed behavior unexpectedly label Jul 28, 2026
@sonarqubecloud

Copy link
Copy Markdown

@leekelleher
leekelleher self-requested a review July 28, 2026 13:15
@leekelleher
leekelleher merged commit 7c79cef into release/17.6 Jul 28, 2026
33 checks passed
@leekelleher
leekelleher deleted the v17/bugfix/23462-no-redirect-after-logout-login branch July 28, 2026 16:35
iOvergaard added a commit that referenced this pull request Jul 29, 2026
…imeout (closes #23462) (#23494)

fix(auth): only time out a session that actually existed

A definitive /token rejection called timeOut() unconditionally, so the boot-time
refresh on the post-sign-out landing page (/umbraco/logout) raised a spurious
timeout. The app answered with the "session timed out" login modal, which
re-authenticates in a popup and leaves navigation to the caller — and the logout
route has no auth guard, so a successful sign-in went nowhere.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/frontend category/ui User interface category/ux User experience release/17.6.0 release/18.1.0 status/regression A previously working feature that has broken or changed behavior unexpectedly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants