From 9d1455e4dd939bce46ab4b1c9302b03cf680ca61 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 17 Jun 2025 11:31:08 +0100 Subject: [PATCH] Prevent skipping forced verification after logging in with OIDC (#30141) Pass the freshLogin parameter along to doSetLoggedIn when restoring a session, instead of hard-coding it to always be false. --- playwright/e2e/oidc/index.ts | 17 ++ playwright/e2e/oidc/oidc-native.spec.ts | 156 +++++++++++++++++- src/Lifecycle.ts | 2 +- .../components/structures/MatrixChat-test.tsx | 47 +++++- 4 files changed, 218 insertions(+), 4 deletions(-) diff --git a/playwright/e2e/oidc/index.ts b/playwright/e2e/oidc/index.ts index 02de6e2f03..602a4368c7 100644 --- a/playwright/e2e/oidc/index.ts +++ b/playwright/e2e/oidc/index.ts @@ -11,6 +11,9 @@ import { type Page } from "@playwright/test"; import { expect } from "../../element-web-test"; +/** + * Click through registering a new user in the MAS UI. + */ export async function registerAccountMas( page: Page, mailpit: MailpitClient, @@ -42,3 +45,17 @@ export async function registerAccountMas( await expect(page.getByText("Allow access to your account?")).toBeVisible(); await page.getByRole("button", { name: "Continue" }).click(); } + +/** + * Click through entering username and password into the MAS login prompt. + */ +export async function logInAccountMas(page: Page, username: string, password: string): Promise { + await expect(page.getByText("Please sign in to continue:")).toBeVisible(); + + await page.getByRole("textbox", { name: "Username" }).fill(username); + await page.getByRole("textbox", { name: "Password", exact: true }).fill(password); + await page.getByRole("button", { name: "Continue" }).click(); + + await expect(page.getByText("Allow access to your account?")).toBeVisible(); + await page.getByRole("button", { name: "Continue" }).click(); +} diff --git a/playwright/e2e/oidc/oidc-native.spec.ts b/playwright/e2e/oidc/oidc-native.spec.ts index a6fbf231ce..3268c2b65a 100644 --- a/playwright/e2e/oidc/oidc-native.spec.ts +++ b/playwright/e2e/oidc/oidc-native.spec.ts @@ -6,8 +6,12 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com Please see LICENSE files in the repository root for full details. */ +import { type Config, CONFIG_JSON } from "@element-hq/element-web-playwright-common"; +import { type Browser, type Page } from "@playwright/test"; +import { type StartedHomeserverContainer } from "@element-hq/element-web-playwright-common/lib/testcontainers/HomeserverContainer"; + import { test, expect } from "../../element-web-test.ts"; -import { registerAccountMas } from "."; +import { logInAccountMas, registerAccountMas } from "."; import { ElementAppPage } from "../../pages/ElementAppPage.ts"; import { masHomeserver } from "../../plugins/homeserver/synapse/masHomeserver.ts"; @@ -101,4 +105,154 @@ test.describe("OIDC Native", { tag: ["@no-firefox", "@no-webkit"] }, () => { expect(localStorageKeys).toHaveLength(0); }, ); + + test("can log in to an existing MAS account", { tag: "@screenshot" }, async ({ page, mailpitClient }, testInfo) => { + // Register an account with MAS + await page.goto("/#/login"); + await page.getByRole("button", { name: "Continue" }).click(); + + const userId = `alice_${testInfo.testId}`; + await registerAccountMas(page, mailpitClient, userId, `${userId}@email.com`, "Pa$sW0rD!"); + await expect(page.getByText("Welcome")).toBeVisible(); + + // Log out + await page.getByRole("button", { name: "User menu" }).click(); + await expect(page.getByText(userId, { exact: true })).toBeVisible(); + + // Allow the outstanding requests queue to settle before logging out + await page.waitForTimeout(2000); + await page.locator(".mx_UserMenu_contextMenu").getByRole("menuitem", { name: "Sign out" }).click(); + await expect(page).toHaveURL(/\/#\/login$/); + + // Log in again + await page.goto("/#/login"); + await page.getByRole("button", { name: "Continue" }).click(); + await page.getByRole("button", { name: "Continue" }).click(); + + // We should be in (we see an error because we have no recovery key). + await expect(page.getByText("Unable to verify this device")).toBeVisible(); + }); + + test.describe("with force_verification on", () => { + test.use({ + config: { + force_verification: true, + }, + }); + + test("verify dialog cannot be dismissed", { tag: "@screenshot" }, async ({ page, mailpitClient }, testInfo) => { + // Register an account with MAS + await page.goto("/#/login"); + await page.getByRole("button", { name: "Continue" }).click(); + + const userId = `alice_${testInfo.testId}`; + await registerAccountMas(page, mailpitClient, userId, `${userId}@email.com`, "Pa$sW0rD!"); + await expect(page.getByText("Welcome")).toBeVisible(); + + // Log out + await page.getByRole("button", { name: "User menu" }).click(); + await expect(page.getByText(userId, { exact: true })).toBeVisible(); + await page.waitForTimeout(2000); + await page.locator(".mx_UserMenu_contextMenu").getByRole("menuitem", { name: "Sign out" }).click(); + await expect(page).toHaveURL(/\/#\/login$/); + + // Log in again + await page.goto("/#/login"); + await page.getByRole("button", { name: "Continue" }).click(); + await page.getByRole("button", { name: "Continue" }).click(); + + // We should be being warned that we need to verify (but we can't) + await expect(page.getByText("Unable to verify this device")).toBeVisible(); + + // And there should be no way to close this prompt + await expect(page.getByRole("button", { name: "Skip verification for now" })).not.toBeVisible(); + }); + + test( + "continues to show verification prompt after cancelling device verification", + { tag: "@screenshot" }, + async ({ browser, config, homeserver, page, mailpitClient }, testInfo) => { + // Register an account with MAS + await page.goto("/#/login"); + await page.getByRole("button", { name: "Continue" }).click(); + + const userId = `alice_${testInfo.testId}`; + const password = "Pa$sW0rD!"; + await registerAccountMas(page, mailpitClient, userId, `${userId}@email.com`, password); + await expect(page.getByText("Welcome")).toBeVisible(); + + // Log in an additional account, and verify it. + // + // This means that when we log out and in again, we are offered + // to verify using another device. + const otherContext = await newContext(browser, config, homeserver); + const otherDevicePage = await otherContext.newPage(); + await otherDevicePage.goto("/#/login"); + await otherDevicePage.getByRole("button", { name: "Continue" }).click(); + await logInAccountMas(otherDevicePage, userId, password); + await verifyUsingOtherDevice(otherDevicePage, page); + await otherDevicePage.close(); + + // Log out + await page.getByRole("button", { name: "User menu" }).click(); + await expect(page.getByText(userId, { exact: true })).toBeVisible(); + await page.waitForTimeout(2000); + await page.locator(".mx_UserMenu_contextMenu").getByRole("menuitem", { name: "Sign out" }).click(); + await expect(page).toHaveURL(/\/#\/login$/); + + // Log in again + await page.goto("/#/login"); + await page.getByRole("button", { name: "Continue" }).click(); + await page.getByRole("button", { name: "Continue" }).click(); + + // We should be in, and not able to dismiss the verify dialog + await expect(page.getByText("Verify this device")).toBeVisible(); + await expect(page.getByRole("button", { name: "Skip verification for now" })).not.toBeVisible(); + + // When we start verifying with another device + await page.getByRole("button", { name: "Verify with another device" }).click(); + + // And then cancel it + await page.getByRole("button", { name: "Close dialog" }).click(); + + // Then we should still be at the unskippable verify prompt + await expect(page.getByText("Verify this device")).toBeVisible(); + await expect(page.getByRole("button", { name: "Skip verification for now" })).not.toBeVisible(); + }, + ); + }); }); + +/** + * Perform interactive emoji verification for a new device. + */ +async function verifyUsingOtherDevice(deviceToVerifyPage: Page, alreadyVerifiedDevicePage: Page) { + await deviceToVerifyPage.getByRole("button", { name: "Verify with another device" }).click(); + await alreadyVerifiedDevicePage.getByRole("button", { name: "Verify session" }).click(); + await alreadyVerifiedDevicePage.getByRole("button", { name: "Start" }).click(); + await alreadyVerifiedDevicePage.getByRole("button", { name: "They match" }).click(); + await deviceToVerifyPage.getByRole("button", { name: "They match" }).click(); + await alreadyVerifiedDevicePage.getByRole("button", { name: "Got it" }).click(); + await deviceToVerifyPage.getByRole("button", { name: "Got it" }).click(); +} + +/** + * Create a new browser context which serves up the default config plus what you supplied, and sets m.homeserver to the + * supplied homeserver's URL. + */ +async function newContext(browser: Browser, config: Partial>, homeserver: StartedHomeserverContainer) { + const otherContext = await browser.newContext(); + await otherContext.route(`http://localhost:8080/config.json*`, async (route) => { + const json = { + ...CONFIG_JSON, + ...config, + default_server_config: { + "m.homeserver": { + base_url: homeserver.baseUrl, + }, + }, + }; + await route.fulfill({ json }); + }); + return otherContext; +} diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 5641f936ae..20e7435599 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -657,7 +657,7 @@ export async function restoreSessionFromStorage(opts?: { ignoreGuest?: boolean } freshLogin: freshLogin, }, false, - false, + freshLogin, ); return true; } else { diff --git a/test/unit-tests/components/structures/MatrixChat-test.tsx b/test/unit-tests/components/structures/MatrixChat-test.tsx index 94fc9a925a..1fb705d98c 100644 --- a/test/unit-tests/components/structures/MatrixChat-test.tsx +++ b/test/unit-tests/components/structures/MatrixChat-test.tsx @@ -1040,7 +1040,7 @@ describe("", () => { localStorage.removeItem("must_verify_device"); }); - it("should show the complete security screen if unskippable verification is enabled", async () => { + it("should show the Complete Security screen if unskippable verification is enabled", async () => { // Given we have force verification on, and an existing logged-in session // that is not verified (see beforeEach()) @@ -1053,7 +1053,6 @@ describe("", () => { // Sanity: we are not racing with another screen update, so this heading stays visible await screen.findByRole("heading", { name: "Verify this device", level: 1 }); }); - it("should not open app after cancelling device verify if unskippable verification is on", async () => { // See https://github.com/element-hq/element-web/issues/29230 // We used to allow bypassing force verification by choosing "Verify with @@ -1081,6 +1080,50 @@ describe("", () => { await screen.findByRole("heading", { name: "Verify this device", level: 1 }); }); + describe("when query params have a loginToken", () => { + const loginToken = "test-login-token"; + const realQueryParams = { + loginToken, + }; + + let loginClient!: ReturnType; + const deviceId = "test-device-id"; + const accessToken = "test-access-token"; + const clientLoginResponse = { + user_id: userId, + device_id: deviceId, + access_token: accessToken, + }; + + beforeEach(() => { + localStorage.setItem("mx_sso_hs_url", serverConfig.hsUrl); + localStorage.setItem("mx_sso_is_url", serverConfig.isUrl); + loginClient = getMockClientWithEventEmitter(getMockClientMethods()); + // this is used to create a temporary client during login + jest.spyOn(MatrixJs, "createClient").mockReturnValue(loginClient); + + loginClient.login.mockClear().mockResolvedValue(clientLoginResponse); + }); + + it("should show the Complete Security screen after OIDC login if unskippable ver. is on", async () => { + // Given force_verification is on (outer describe) + // And we just logged in via OIDC (inner describe) + + // When we load the page + getComponent({ realQueryParams }); + + defaultDispatcher.dispatch({ + action: "will_start_client", + }); + await waitFor(() => + expect(defaultDispatcher.dispatch).toHaveBeenCalledWith({ action: "client_started" }), + ); + + // Then we are not allowed in - we are being asked to verify + await screen.findByRole("heading", { name: "Verify this device", level: 1 }); + }); + }); + function createMockCrypto(): CryptoApi { return { getVersion: jest.fn().mockReturnValue("Version 0"),