nikrozman/element-web
1
0
Fork 0
Code Issues Actions 3 Packages Projects Releases Wiki Activity

Prevent skipping forced verification after logging in with OIDC (#30141)

Browse Source 
Pass the freshLogin parameter along to doSetLoggedIn when restoring a session,
instead of hard-coding it to always be false.
This commit is contained in:
Andy Balaam
2025-06-17 11:31:08 +01:00
committed by GitHub
parent 28a232eea8
commit 9d1455e4dd
4 changed files with 218 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
playwright/e2e/oidc/index.ts
View File

@@ -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<void> {
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();
}

156
playwright/e2e/oidc/oidc-native.spec.ts
View File

@@ -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<Partial<Config>>, 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;
}

2
src/Lifecycle.ts
View File

@@ -657,7 +657,7 @@ export async function restoreSessionFromStorage(opts?: { ignoreGuest?: boolean }
freshLogin: freshLogin,
},
false,
false,
freshLogin,
);
return true;
} else {

47
test/unit-tests/components/structures/MatrixChat-test.tsx
View File

@@ -1040,7 +1040,7 @@ describe("<MatrixChat />", () => {
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("<MatrixChat />", () => {
// 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("<MatrixChat />", () => {
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<typeof getMockClientWithEventEmitter>;
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"),