From 04cf53e7aa6b444674f0257650bd97ecc6a6e12d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 2 Dec 2025 11:14:00 +0000 Subject: [PATCH] Fix bug which caused app not to load correctly when `force_verification` is enabled (#31265) * MatrixChat: add a load of logging for view transitions This stuff was essentially impossible to follow and debug. I think a load of logging will help. * Add more comments on `state.view` * Add a new state between LOADING/SOFT_LOGOUT and LOGGED_IN ... so that we can transition into COMPLETE_SECURITY without going via LOGGED_IN. * Remove redundant check for `force_verification` This check was previously necessary to keep the tests working, because: * onLoggedIn would call `onShowPostLoginScreen`, * which (without the check) would call `showScreenAfterLogin` * which would queue up an action `Action.ViewHomePage` * Then we would receive an already-queued `ClientStarted` action, which would transition us (correctly) to the `COMPLETE_SECURITY` view * Then we would receive the `ViewHomePage` action, taking us back to `LOGGED_IN`. I don't think the check was necessary in practice, because in practice there would be enough delay between the OnLoggedIn and ClientStarted actions that the race didn't happen. The *problem* with the check was that it meant that, whenever `force_verification` was enabled, we would get stuck in the LOADING state. The check is now unnecessary, because `onLoggedIn` no longer calls `onShowPostLoginScreen`. * `onShowPostLoginScreen` need no longer be `async` * Regression test for https://github.com/element-hq/element-web/issues/31203 --- playwright/e2e/login/login.spec.ts | 30 +++++++ src/PosthogTrackers.ts | 1 + src/Views.ts | 108 +++++++++++++---------- src/components/structures/MatrixChat.tsx | 104 ++++++++++++++++------ 4 files changed, 172 insertions(+), 71 deletions(-) create mode 100644 playwright/e2e/login/login.spec.ts diff --git a/playwright/e2e/login/login.spec.ts b/playwright/e2e/login/login.spec.ts new file mode 100644 index 0000000000..56025d9405 --- /dev/null +++ b/playwright/e2e/login/login.spec.ts @@ -0,0 +1,30 @@ +/* +Copyright 2025 Element Creations Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import { expect, test } from "../../element-web-test"; +import { logIntoElement } from "../crypto/utils.ts"; + +test.describe(`With force_verification: true`, () => { + test.use({ + config: { + force_verification: true, + }, + }); + + test("Can reload after login", async ({ page, credentials }) => { + // The page should reload fine when going to the base client URL + // Regression test for https://github.com/element-hq/element-web/issues/31203 + await logIntoElement(page, credentials); + + // We should auto-upload the E2EE keys, and show a welcome page + await expect(page.getByRole("heading", { name: `Welcome ${credentials.displayName}` })).toBeVisible(); + + await page.goto("/"); + + await expect(page.getByRole("heading", { name: `Welcome ${credentials.displayName}` })).toBeVisible(); + }); +}); diff --git a/src/PosthogTrackers.ts b/src/PosthogTrackers.ts index ae67b5f378..2650403d3b 100644 --- a/src/PosthogTrackers.ts +++ b/src/PosthogTrackers.ts @@ -27,6 +27,7 @@ const notLoggedInMap: Record, ScreenName> = { [Views.FORGOT_PASSWORD]: "ForgotPassword", [Views.COMPLETE_SECURITY]: "CompleteSecurity", [Views.E2E_SETUP]: "E2ESetup", + [Views.PENDING_CLIENT_START]: "Loading", [Views.SOFT_LOGOUT]: "SoftLogout", [Views.LOCK_STOLEN]: "SessionLockStolen", }; diff --git a/src/Views.ts b/src/Views.ts index 0757b8c9c8..ccb0d93e05 100644 --- a/src/Views.ts +++ b/src/Views.ts @@ -20,52 +20,62 @@ Please see LICENSE files in the repository root for full details. * │ LOADING │─────────────────────────────►│ CONFIRM_LOCK_ │ * │ │◄─────────────────────────────│ THEFT │ * └─────────────────┘ Lock theft confirmed └─────────────────┘ - * Session recovered │ │ │ - * ┌──────────────┘ │ └────────────────┐ - * │ ┌─────────────┘ │ No previous session - * │ │ Token/OIDC login succeeded │ - * │ │ ▼ - * │ │ ┌─────────────────┐ - * │ │ │ WELCOME │ (from all other states - * │ │ │ │ except LOCK_STOLEN) - * │ │ └─────────────────┘ │ - * │ │ "Create Account" │ │ "Sign in" │ Client logged out - * │ │ ┌────────────────────────┘ │ │ - * │ │ │ │ ┌────────────────────┘ - * │ │ │ │ │ - * │ │ ▼ "Create an ▼ ▼ "Forgot - * │ │ ┌─────────────────┐ account" ┌─────────────────┐ password" ┌─────────────────┐ - * │ │ │ REGISTER │◄───────────────│ LOGIN │───────────────►│ FORGOT_PASSWORD │ - * │ │ │ │───────────────►│ │◄───────────────│ │ - * │ │ └─────────────────┘ "Sign in here" └─────────────────┘ Complete / └─────────────────┘ - * │ │ │ │ "Sign in instead" ▲ - * │ │ └────────────────────────────────┐ │ │ - * │ └────────────────────────────────────────┐ │ │ │ - * │ ▼ ▼ ▼ │ - * │ ┌──────────────────┐ │ - * │ │ (postLoginSetup) │ │ - * │ └──────────────────┘ │ - * │ ┌────────────────────────────────────┘ │ │ │ - * │ │ E2EE not enabled ┌─────────────┘ └──────┐ │ - * │ │ │ Account has │ Account lacks │ - * │ │ │ cross-signing │ cross-signing │ - * │ │ │ keys │ keys │ - * │ │ Client started and ▼ ▼ │ - * │ │ force_verification ┌─────────────────┐ ┌─────────────────┐ │ - * │ │ pending │ COMPLETE_ │ │ E2E_SETUP │ │ - * │ │ ┌─────────────────►│ SECURITY │ │ │ │ - * │ │ │ └─────────────────┘ └─────────────────┘ │ "Forgotten - * │ │ │ ┌───────────────────────┘ │ │ your - * │ │ │ │ ┌───────────────────────────────────────────────┘ │ password?" - * │ │ │ │ │ │ - * │ │ │ │ │ (from all other states │ - * │ │ │ │ │ except LOCK_STOLEN) │ - * │ │ │ │ │ └──────────────┐ │ - * ▼ ▼ │ ▼ ▼ Soft logout error ▼ │ - * ┌─────────────────┐ ┌─────────────────┐ - * │ LOGGED_IN │ Re-authentication succeeded │ SOFT_LOGOUT │ - * │ │◄────────────────────────────────────────────────────────│ │ - * └─────────────────┘ └─────────────────┘ + * Session recovered │ │ │ Token/OIDC login succeeded + * ┌──────────────┘ │ └──────────────────────────────────────────────────────────────────┐ + * │ └───────────────────────────────────────────┐ │ + * │ │ No previous session │ + * │ ▼ │ + * │ (from all other states ┌─────────────────┐ │ + * │ except LOCK_STOLEN) │ WELCOME │ │ + * │ │ │ │ │ + * │ │ Client logged out └─────────────────┘ │ + * │ │ │ │ │ + * │ └──────────────────────────┐ "Sign in" │ │ "Create account" │ + * │ │ ┌────────────┘ └──────────────┐ │ + * │ │ │ │ │ + * │ "Forgot ▼ ▼ "Create an ▼ │ + * │ ┌─────────────────┐ password" ┌─────────────────┐ account" ┌─────────────────┐ │ + * │ │ FORGOT_PASSWORD │◄───────────────│ LOGIN │───────────────►│ REGISTER │ │ + * │ │ │───────────────►│ │◄───────────────│ │ │ + * │ └─────────────────┘ Complete / └─────────────────┘ "Sign in here" └─────────────────┘ │ + * │ ▲ "Sign in instead" │ │ │ + * │ │ └──────────────────────┐ ┌─────────┘ │ + * │ │"Forgotten your │ │ ┌──────────────────┘ + * │ │ password?" │ │ │ + * │ │ │ │ │ + * │ ┌─────────────────┐ Soft-logout error │ │ │ + * │ │ SOFT_LOGOUT │◄───────────── (from all other states │ │ │ + * │ │ │ except LOCK_STOLEN) │ │ │ + * │ └─────────────────┘ │ │ │ + * │ │ Re-authentication succeeded ▼ ▼ ▼ + * │ │ ┌──────────────────┐ + * ▼ ▼ │ (postLoginSetup) │ + * ┌─────────────────┐ └──────────────────┘ + * │ PENDING_CLIENT_ │ Account has │ │ │ Account lacks + * │ START │ cross-signing │ │ │ cross-signing + * └─────────────────┘ keys │ │ │ keys + * │ │ │ │ │ + * │ └───────────────────────────────┐ │ │ │ + * │ Client started, │ │ │ └──────┐ + * │ force_verification pending │ │ │ │ + * │ ▼ │ │ │ + * │ Client started, ┌─────────────────┐ │ │ │ + * │ force_verification │ COMPLETE_ │◄────────────┘ │ ▼ + * │ not needed │ SECURITY │ │ ┌─────────────────┐ + * │ └─────────────────┘ │ │ E2E_SETUP │ + * │ │ │ │ │ + * │ ┌─────────────────────────────────────┘ E2EE not enabled │ └─────────────────┘ + * │ │ ┌─────────────────────────────────────────────────────────────┘ │ + * │ │ │ ┌──────────────────────────────────────────────────────────────────────┘ + * │ │ │ │ + * │ │ │ │ + * │ │ │ │ + * │ │ │ │ + * ▼ ▼ ▼ ▼ + * ┌─────────────────┐ + * │ LOGGED_IN │ + * │ │ + * └─────────────────┘ * * (from all other states) * │ @@ -102,6 +112,12 @@ enum Views { // flow to setup SSSS / cross-signing on this account E2E_SETUP, + /** + * We have successfully recovered a session from localstorage, but the client + * has not yet been started. + */ + PENDING_CLIENT_START, + // we are logged in with an active matrix client. The logged_in state also // includes guests users as they too are logged in at the client level. LOGGED_IN, diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 41c4117164..21706e9f34 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -173,8 +173,18 @@ interface IProps { } interface IState { - // the master view we are showing. + /** + * The master view we are showing. + * + * This represents the state of a state machine: see the documentation on {@link Views} for a transition diagram. + * + * TODO: this doesn't work well, because updates to React state are not instantaneous, meaning that if several + * events or {@link Action}s happen in quick succession, we may end up following the wrong transition. + * We should probably move the view into a separate object (like a ViewModel) and have the React state subscribe + * to updates. + */ view: Views; + // What the LoggedInView would be showing if visible. // A member of the enum for standard pages or a string for those provided by // a module. @@ -473,6 +483,9 @@ export default class MatrixChat extends React.PureComponent { | (Pick | IState | null), callback?: () => void, ): void { + if (state && "view" in state) { + logger.debug(`MatrixChat: Queuing change of view from ${Views[this.state.view]} to ${Views[state.view]}`); + } if (this.shouldTrackPageChange(this.state, { ...this.state, ...state })) { this.startPageChangeTimer(); } @@ -648,9 +661,15 @@ export default class MatrixChat extends React.PureComponent { private onAction = (payload: ActionPayload): void => { // once the session lock has been stolen, don't try to do anything. if (this.state.view === Views.LOCK_STOLEN) { + logger.warn(`MatrixChat: ignoring action ${payload.action} as session lock has been stolen`); return; } + // Exclude some rather spammy actions from being logged. + if (payload.action != Action.UserActivity) { + logger.debug(`MatrixChat: handling action ${payload.action}`); + } + // Start the onboarding process for certain actions if ( MatrixClientPeg.get()?.isGuest() && @@ -1390,11 +1409,14 @@ export default class MatrixChat extends React.PureComponent { * * In other words, whenever we think we have completed the login and E2E setup tasks. */ - private async onShowPostLoginScreen(): Promise { + private onShowPostLoginScreen(): void { + logger.debug("onShowPostLoginScreen: Transitioning to logged in view."); + this.setStateForNewView({ view: Views.LOGGED_IN }); // If a specific screen is set to be shown after login, show that above // all else, as it probably means the user clicked on something already. if (this.screenAfterLogin?.screen) { + logger.debug(`onShowPostLoginScreen: showing screen ${this.screenAfterLogin.screen}`); this.showScreen(this.screenAfterLogin.screen, this.screenAfterLogin.params); this.screenAfterLogin = undefined; } else if (MatrixClientPeg.currentUserIsJustRegistered()) { @@ -1403,6 +1425,7 @@ export default class MatrixChat extends React.PureComponent { if (ThreepidInviteStore.instance.pickBestInvite()) { // The user has a 3pid invite pending - show them that const threepidInvite = ThreepidInviteStore.instance.pickBestInvite(); + logger.debug(`onShowPostLoginScreen: showing room ${threepidInvite.roomId} after registration`); // HACK: This is a pretty brutal way of threading the invite back through // our systems, but it's the safest we have for now. @@ -1411,9 +1434,11 @@ export default class MatrixChat extends React.PureComponent { } else { // The user has just logged in after registering, // so show the homepage. + logger.debug("onShowPostLoginScreen: Showing home page after registration"); dis.dispatch({ action: Action.ViewHomePage, justRegistered: true }); } - } else if (!(await this.shouldForceVerification())) { + } else { + logger.debug("onShowPostLoginScreen: showScreenAfterLogin"); this.showScreenAfterLogin(); } @@ -1477,15 +1502,19 @@ export default class MatrixChat extends React.PureComponent { // If screenAfterLogin is set, use that, then null it so that a second login will // result in view_home_page, _user_settings or _room_directory if (this.screenAfterLogin && this.screenAfterLogin.screen) { + logger.debug(`showScreenAfterLogin: showing screen ${this.screenAfterLogin.screen}`); this.showScreen(this.screenAfterLogin.screen, this.screenAfterLogin.params); this.screenAfterLogin = undefined; } else if (localStorage && localStorage.getItem("mx_last_room_id")) { // Before defaulting to directory, show the last viewed room + logger.debug("showScreenAfterLogin: showing last room"); this.viewLastRoom(); } else { if (MatrixClientPeg.safeGet().isGuest()) { + logger.debug("showScreenAfterLogin: showing guest welcome page"); dis.dispatch({ action: "view_welcome_page" }); } else { + logger.debug("showScreenAfterLogin: showing home page"); dis.dispatch({ action: Action.ViewHomePage }); } } @@ -1506,18 +1535,27 @@ export default class MatrixChat extends React.PureComponent { this.stores.client = MatrixClientPeg.safeGet(); StorageManager.tryPersistStorage(); - // If we're in the middle of a login/registration, we wait for it to complete before transitioning to the logged - // in view the login flow will call `postLoginSetup` when it's done, which will arrange for `onShowPostLoginScreen` - // to be called. + // If we're loading the app for the first time, we can now transition to a splash screen while we wait for the + // client to start. The exceptions are: + // + // - If there is a token login in flight: in that case we wait for the login to complete (which hits + // `postLoginSetup`). + // + // - Lifecycle emits an `Action.OnLoggedIn` event during startup even if the localstorage flag indicating a + // previous soft logout is set. In that situation we actually want to wait for the `Action.ClientNotViable` + // event, which will transition us into Views.SOFT_LOGOUT. We therefore have to check for !isSoftLogout(). + // There will be a subsequent `Action.OnLoggedIn` event once the reauthentication completes. + // + // XXX: fix this properly by having Lifecycle not emit OnLoggedIn when it knows it is about to emit a + // ClientNotViable. + // + // If we're already in the SOFT_LOGOUT view, that means that reauthentication has succeeded, and we can + // transition to the splash screen. if ( - !this.tokenLogin && - !Lifecycle.isSoftLogout() && - this.state.view !== Views.LOGIN && - this.state.view !== Views.REGISTER && - this.state.view !== Views.COMPLETE_SECURITY && - this.state.view !== Views.E2E_SETUP + (this.state.view === Views.LOADING && !Lifecycle.isSoftLogout() && !this.tokenLogin) || + this.state.view === Views.SOFT_LOGOUT ) { - this.onShowPostLoginScreen(); + this.setStateForNewView({ view: Views.PENDING_CLIENT_START }); } } @@ -1750,15 +1788,6 @@ export default class MatrixChat extends React.PureComponent { const cli = MatrixClientPeg.safeGet(); const shouldForceVerification = await this.shouldForceVerification(); - // XXX: Don't replace the screen if it's already one of these: postLoginSetup - // changes to these screens in certain circumstances so we shouldn't clobber it. - // We should probably have one place where we decide what the next screen is after - // login. - if (![Views.COMPLETE_SECURITY, Views.E2E_SETUP].includes(this.state.view)) { - if (shouldForceVerification) { - this.setStateForNewView({ view: Views.COMPLETE_SECURITY }); - } - } const crypto = cli.getCrypto(); if (crypto) { @@ -1775,13 +1804,31 @@ export default class MatrixChat extends React.PureComponent { this.setState({ ready: true, }); + + // If the view is PENDING_CLIENT_START, that means we recovered the session from localstorage, or from + // soft-logout: we can now transition to the logged-in view. + // + // If the view is something else, that probably means it's a login or registration view; we handle that in + // `postLoginSetup`. + if (this.state.view === Views.PENDING_CLIENT_START) { + if (shouldForceVerification) { + this.setStateForNewView({ view: Views.COMPLETE_SECURITY }); + } else { + this.onShowPostLoginScreen(); + } + } } public showScreen(screen: string, params?: { [key: string]: any }): void { + logger.debug(`showScreen ${screen}`); + const cli = MatrixClientPeg.get(); const isLoggedOutOrGuest = !cli || cli.isGuest(); if (!isLoggedOutOrGuest && AUTH_SCREENS.includes(screen)) { // user is logged in and landing on an auth page which will uproot their session, redirect them home instead + logger.info( + `showScreen: suppressing change to AuthScreen ${screen} for logged-in user, and going to home screen instead`, + ); dis.dispatch({ action: Action.ViewHomePage }); return; } @@ -2087,9 +2134,7 @@ export default class MatrixChat extends React.PureComponent { } } - await this.onShowPostLoginScreen().catch((e) => { - logger.error("Exception showing post-login screen", e); - }); + this.onShowPostLoginScreen(); }; private getFragmentAfterLogin(): string { @@ -2128,6 +2173,15 @@ export default class MatrixChat extends React.PureComponent { view = ; } else if (this.state.view === Views.E2E_SETUP) { view = ; + } else if (this.state.view === Views.PENDING_CLIENT_START) { + // we think we are logged in, but are still waiting for the /sync to complete + view = ( + + ); } else if (this.state.view === Views.LOGGED_IN) { // `ready` and `view==LOGGED_IN` may be set before `page_type` (because the // latter is set via the dispatcher). If we don't yet have a `page_type`,