-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(auth): stop transient failures from logging players out; retry + session-expiry UX #4440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4d87a4e
7bf2de4
766bf78
6f4a5fb
b8d9336
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,22 @@ export async function getAuthHeader(): Promise<string> { | |
| return `Bearer ${jwt}`; | ||
| } | ||
|
|
||
| export async function logOut(allSessions: boolean = false): Promise<boolean> { | ||
| export interface LogOutOptions { | ||
| /** Revoke every session (/auth/revoke) instead of just the current one. */ | ||
| allSessions?: boolean; | ||
| /** | ||
| * Set only for an explicit, user-initiated logout — the one case where we | ||
| * also wipe the local persistent identity + cosmetics. Error-path callers | ||
| * must leave this false, or a transient failure becomes a permanent new | ||
| * guest account. | ||
| */ | ||
| userInitiated?: boolean; | ||
| } | ||
|
|
||
| export async function logOut({ | ||
| allSessions = false, | ||
| userInitiated = false, | ||
| }: LogOutOptions = {}): Promise<boolean> { | ||
| try { | ||
| const response = await fetch( | ||
| getApiBase() + (allSessions ? "/auth/revoke" : "/auth/logout"), | ||
|
|
@@ -98,9 +113,14 @@ export async function logOut(allSessions: boolean = false): Promise<boolean> { | |
| return false; | ||
| } finally { | ||
| __jwt = null; | ||
| localStorage.removeItem(PERSISTENT_ID_KEY); | ||
| new UserSettings().clearFlag(); | ||
| new UserSettings().setSelectedPatternName(undefined); | ||
| // Only destroy the local persistent identity / cosmetics on an explicit | ||
| // user logout. Error-path callers must NOT wipe identity, or a transient | ||
| // failure turns into a permanent brand-new guest account. | ||
| if (userInitiated) { | ||
| localStorage.removeItem(PERSISTENT_ID_KEY); | ||
| new UserSettings().clearFlag(); | ||
| new UserSettings().setSelectedPatternName(undefined); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -188,29 +208,91 @@ async function refreshJwt(): Promise<void> { | |
| } | ||
| } | ||
|
|
||
| // Outcome of the most recent refresh attempt, so callers (e.g. ranked | ||
| // matchmaking) can tell "your session expired" apart from "we couldn't reach | ||
| // the auth server right now". | ||
| export type RefreshOutcome = "ok" | "expired" | "transient"; | ||
| let __lastRefreshOutcome: RefreshOutcome = "ok"; | ||
|
|
||
| export function getLastRefreshOutcome(): RefreshOutcome { | ||
| return __lastRefreshOutcome; | ||
| } | ||
|
|
||
| const REFRESH_MAX_ATTEMPTS = 3; | ||
|
|
||
| function refreshBackoffMs(attempt: number): number { | ||
| // Backoff between attempts: 500ms, then 1000ms. | ||
| return 500 * 2 ** (attempt - 1); | ||
| } | ||
|
|
||
| function delay(ms: number): Promise<void> { | ||
| return new Promise((resolve) => setTimeout(resolve, ms)); | ||
| } | ||
|
|
||
| function notifySessionExpired(): void { | ||
| if (typeof window === "undefined") return; | ||
| window.dispatchEvent(new CustomEvent("auth-session-expired")); | ||
| } | ||
|
|
||
| async function doRefreshJwt(): Promise<void> { | ||
| try { | ||
| console.log("Refreshing jwt"); | ||
| const response = await fetch(getApiBase() + "/auth/refresh", { | ||
| method: "POST", | ||
| credentials: "include", | ||
| }); | ||
| if (response.status !== 200) { | ||
| console.error("Refresh failed", response); | ||
| logOut(); | ||
| return; | ||
| // Whether an authenticated session was active before this attempt. Only a | ||
| // session that dies mid-use should surface the "signed out" UI. | ||
| const hadSession = __jwt !== null; | ||
|
|
||
| for (let attempt = 1; attempt <= REFRESH_MAX_ATTEMPTS; attempt++) { | ||
| try { | ||
| console.log( | ||
| `Refreshing jwt (attempt ${attempt}/${REFRESH_MAX_ATTEMPTS})`, | ||
| ); | ||
| const response = await fetch(getApiBase() + "/auth/refresh", { | ||
| method: "POST", | ||
| credentials: "include", | ||
| }); | ||
|
|
||
| if (response.status === 200) { | ||
| const json = await response.json(); | ||
| const { jwt, expiresIn } = json; | ||
| __expiresAt = Date.now() + expiresIn * 1000; | ||
| __jwt = jwt; | ||
| __lastRefreshOutcome = "ok"; | ||
| console.log("Refresh succeeded"); | ||
| return; | ||
| } | ||
|
|
||
| // 401/403 are definitive: the refresh token is genuinely invalid or | ||
| // expired, so retrying can't help. Do a "soft" logout — clear the | ||
| // in-memory JWT but preserve the session cookie + persistent identity — | ||
| // and let a previously-signed-in user be prompted to log in again. | ||
| if (response.status === 401 || response.status === 403) { | ||
| console.error("Refresh rejected — session expired", response.status); | ||
| __jwt = null; | ||
| __lastRefreshOutcome = "expired"; | ||
| if (hadSession) notifySessionExpired(); | ||
| return; | ||
|
Comment on lines
+266
to
+271
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Clear cached user data when the session expires. This branch clears Proposed fix export function invalidateUserMe() {
__userMe = null;
}
+
+if (typeof window !== "undefined") {
+ window.addEventListener("auth-session-expired", invalidateUserMe);
+}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| // Everything else (5xx, 429, ...) is transient — fall through to retry. | ||
| console.error( | ||
| `Refresh failed (status ${response.status}), attempt ${attempt}/${REFRESH_MAX_ATTEMPTS}`, | ||
| ); | ||
| } catch (e) { | ||
| // Network error / server unreachable — transient, fall through to retry. | ||
| console.error( | ||
| `Refresh failed (network), attempt ${attempt}/${REFRESH_MAX_ATTEMPTS}`, | ||
| e, | ||
| ); | ||
| } | ||
|
|
||
| if (attempt < REFRESH_MAX_ATTEMPTS) { | ||
| await delay(refreshBackoffMs(attempt)); | ||
| } | ||
| const json = await response.json(); | ||
| const { jwt, expiresIn } = json; | ||
| __expiresAt = Date.now() + expiresIn * 1000; | ||
| console.log("Refresh succeeded"); | ||
| __jwt = jwt; | ||
| } catch (e) { | ||
| console.error("Refresh failed", e); | ||
| // if server unreachable, just clear jwt | ||
| __jwt = null; | ||
| return; | ||
| } | ||
|
|
||
| // Transient failures exhausted. Clear the in-memory JWT only — keep the | ||
| // session cookie and persistent identity so the next refresh can recover. | ||
| console.error("Refresh failed after retries; staying recoverable"); | ||
| __jwt = null; | ||
| __lastRefreshOutcome = "transient"; | ||
| } | ||
|
|
||
| export async function sendMagicLink(email: string): Promise<boolean> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { html } from "lit"; | ||
| import { customElement } from "lit/decorators.js"; | ||
| import { wasLinkedAccount } from "./Api"; | ||
| import { BaseModal, ModalConfig } from "./components/BaseModal"; | ||
| import { translateText } from "./Utils"; | ||
|
|
||
| /** | ||
| * App-level warning shown when a previously-signed-in user's auth session | ||
| * expires (a definitive 401/403 on /auth/refresh). Auth.ts dispatches the | ||
| * "auth-session-expired" window event; we only surface it for users who were | ||
| * actually logged in to an account — guests get a fresh session silently. | ||
| */ | ||
| @customElement("session-expired-modal") | ||
| export class SessionExpiredModal extends BaseModal { | ||
| connectedCallback(): void { | ||
| super.connectedCallback(); | ||
| window.addEventListener("auth-session-expired", this.onSessionExpired); | ||
| } | ||
|
|
||
| disconnectedCallback(): void { | ||
| window.removeEventListener("auth-session-expired", this.onSessionExpired); | ||
| super.disconnectedCallback(); | ||
| } | ||
|
|
||
| private onSessionExpired = (): void => { | ||
| if (!wasLinkedAccount()) return; | ||
| if (this.isOpen()) return; | ||
| this.open(); | ||
| }; | ||
|
|
||
| protected modalConfig(): ModalConfig { | ||
| return { | ||
| title: translateText("session_expired.title"), | ||
| hideHeader: false, | ||
| hideCloseButton: false, | ||
| maxWidth: "420px", | ||
| }; | ||
| } | ||
|
|
||
| private logIn(): void { | ||
| this.close(); | ||
| window.showPage?.("page-account"); | ||
| } | ||
|
|
||
| protected renderBody() { | ||
| return html` | ||
| <div class="px-6 py-4 text-gray-800 dark:text-gray-200"> | ||
| <p class="mb-6">${translateText("session_expired.body")}</p> | ||
| <div class="flex justify-end gap-3"> | ||
| <button | ||
| class="px-4 py-2 rounded-md bg-gray-200 text-gray-900 dark:bg-gray-700 dark:text-gray-100" | ||
| @click=${() => this.close()} | ||
| > | ||
| ${translateText("session_expired.dismiss")} | ||
| </button> | ||
| <button | ||
| class="px-4 py-2 rounded-md bg-blue-600 text-white" | ||
| @click=${() => this.logIn()} | ||
| > | ||
| ${translateText("session_expired.log_in")} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| `; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not cache transient
falseuser lookups.This path now treats 401/non-200 as transient, but
__userMestill stores the promise that resolves tofalse. LatergetUserMe()calls can keep returning the cachedfalseand never retry until something callsinvalidateUserMe().Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents