Skip to content

Auth: Time out the session when the timeout modals "Stay logged in" refresh fails (closes #22986) - #23079

Merged
leekelleher merged 3 commits into
v17/devfrom
v17/bugfix/22986-stay-logged-in-silent-failure
Jun 29, 2026
Merged

Auth: Time out the session when the timeout modals "Stay logged in" refresh fails (closes #22986)#23079
leekelleher merged 3 commits into
v17/devfrom
v17/bugfix/22986-stay-logged-in-silent-failure

Conversation

@iOvergaard

@iOvergaard iOvergaard commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Background

This began as an attempt to reproduce #22986 — "clicking Stay logged in logs you out anyway." We couldn't reproduce it literally: on a healthy session the button works, the token rotates, the user stays logged in. But the reported symptom is exactly what the code produces when the refresh behind that button fails or the session has already expired — and the modal's countdown could drift out of sync with the real server-side expiry, showing time the user no longer had. The reporter's config (TimeOut, load-balancing) is unknown, so this may not be their precise cause, but these are real bugs in the same path producing the same outcome.

What changed

  • A failed refresh now times the user out immediately. Previously the failed /token result was swallowed: the modal closed, the user looked logged in, then got bounced on their next action — matching the report.
  • The timeout flow is wall-clock based. The warning timer recomputes the remaining time from Date.now() when it fires, and the modal counts down against an absolute deadline — so neither drifts out of sync after system sleep or background-tab throttling. A 5s margin keeps the client slightly ahead of the server so a late click still lands in time.
  • /token is no longer retried after a definitive failure. UmbAuthClient now separates definitive OAuth rejections (invalid_grant-class) from transient failures (network/5xx); a definitive rejection latches the session as dead and stops further refresh attempts until a new session is established. Previously every pending request fired its own doomed /token call (18+ observed). Transient failures stay retryable.
  • Fixed import order in three auth modal files so ESLint can run on the package.

Testing

10 new unit tests (failed-refresh timeout, late-firing timers via patched Date.now, wall-clock countdown, the fatal/transient latch). Verified in the browser with a revoked refresh token: one /token call and an immediate "session timed out" screen, for both the modal click and a burst of requests on a dead session; happy path still extends the session. Full client suite passes, tsc clean, no new lint warnings. No public API changes.

If you still hit this after the fix

Please reopen with your Global:TimeOut and Security:KeepUserLoggedIn settings, whether the site is load-balanced / behind a proxy, and whether /token returns 200 but the next request 401s — that separates a swallowed refresh failure from an environmental cookie / Data Protection key-ring issue.

Fixes #22986

🤖 Generated with Claude Code

iOvergaard and others added 2 commits June 5, 2026 14:48
…andle failed refresh (#22986)

- Time the user out when the "Stay logged in" refresh fails (e.g. the
  refresh token expired while the modal was open), instead of silently
  swallowing the failure and logging the user out on their next action
- Recompute the remaining session time from the wall clock when the
  warning timer fires — after system sleep or background-tab throttling
  the timer fires late and the session may already be expired
- Drive the modal countdown from an absolute deadline instead of
  counting interval ticks, so it stays truthful across sleep/throttling
- Subtract a 5s safety margin from the client-computed expiry so a
  click near the end of the countdown still reaches the server in time

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The import/order rule crashes ESLint 10 (eslint-plugin-import fixer
incompatibility) whenever it needs to report a violation, which made
the whole auth package unlintable. Reordering the imports to the
configured group order (parent, sibling, external) avoids the crash.
Also removes an invalid @required JSDoc tag flagged by the linter.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 5, 2026 12:56
@claude

claude Bot commented Jun 5, 2026

Copy link
Copy Markdown

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


PR Review

Target: origin/v17/dev · Based on commit: 4f77db47

Fixes three compounding defects in the session-timeout flow: a failed /token refresh was silently swallowed (leaving a doomed session), the warning timer passed stale scheduling-time values to the modal, and the modal countdown decremented by tick count instead of against a wall-clock deadline. All three root causes are now addressed and covered by new unit tests.

  • Other changes: Session behavior is now honest — a failed "Stay logged in" refresh immediately triggers the re-auth screen instead of leaving the UI in a broken state; the countdown display now reflects real elapsed time after system sleep or background-tab throttling.

Suggestions

  • umb-auth-timeout-modal.element.ts:29: if (this.#interval) relies on setInterval never returning 0. While that holds in all current runtimes, this.#interval !== undefined matches the declared type (#interval?: any — absent state is undefined) and makes the intent explicit. Fix this →

  • auth-session-timeout.controller.test.ts:116: The keepUserLoggedIn=true auto-refresh-failure path has no dedicated test. The fix is in the shared #tryValidateToken() method, so it is indirectly covered by the onContinue tests, but an explicit test would make the branch coverage unambiguous. E.g. set context.keepUserLoggedIn = true, stub validateToken to return false, inject a nearly-expired access-token session, and assert timeOutCalls === 1. Fix this →


Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions.

Labels applied: area/frontend, category/ux

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

Improves the backoffice auth session-timeout flow so the “Stay logged in” action no longer silently succeeds when the refresh token has already expired, and makes the timeout countdown resilient to system sleep/background-tab timer throttling.

Changes:

  • Ensure failed token refresh triggers timeOut() (so re-auth starts immediately instead of leaving a stale in-memory session).
  • Recompute remaining session time from the wall clock when the warning timer fires (with a small safety margin) to avoid showing a timeout modal for an already-expired session.
  • Drive the timeout modal countdown from an absolute deadline (and add unit tests covering sleep/throttling scenarios); additionally fixes import ordering in auth modal files.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Umbraco.Web.UI.Client/src/packages/core/auth/modals/umb-auth-timeout-modal.element.ts Countdown now uses an absolute deadline; adds interval-guard on reconnect; expires via onExpired callback.
src/Umbraco.Web.UI.Client/src/packages/core/auth/modals/umb-auth-timeout-modal.element.test.ts New tests validating countdown tick behavior and wall-clock expiry after simulated sleep.
src/Umbraco.Web.UI.Client/src/packages/core/auth/modals/umb-app-auth-modal.token.ts Import order adjustment; removes misleading @required JSDoc on an optional value.
src/Umbraco.Web.UI.Client/src/packages/core/auth/modals/umb-app-auth-modal.element.ts Import order adjustment.
src/Umbraco.Web.UI.Client/src/packages/core/auth/modals/manifests.ts Import order adjustment.
src/Umbraco.Web.UI.Client/src/packages/core/auth/controllers/auth-session-timeout.controller.ts Recomputes remaining time at timer-fire; introduces expiry safety margin; #tryValidateToken() times out on refresh failure; getContext() moved into guarded try.
src/Umbraco.Web.UI.Client/src/packages/core/auth/controllers/auth-session-timeout.controller.test.ts New tests for warning-zone modal opening, refresh success/failure behavior, and late timer firing after simulated sleep.

@claude claude Bot added area/frontend category/ux User experience labels Jun 5, 2026
When the refresh token is rejected with invalid_grant, every queued or
subsequent API request would previously fire its own doomed /token call
(observed 18+ in a row) before the user was finally logged out.

- UmbAuthClient now distinguishes definitive OAuth rejections (400/401
  with an error body) from transient failures (network errors, 5xx)
- UmbAuthContext latches the session as dead on a definitive rejection,
  times the user out once, and short-circuits further refresh attempts
  until a new session is established (login, peer tab, re-auth)
- Transient failures do not latch, so a network blip can still recover

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@leekelleher
leekelleher self-requested a review June 29, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TimeOut setting for user not respected

3 participants