-
-
Notifications
You must be signed in to change notification settings - Fork 8
Improve error display for messages sent from insecure devices #93
Changes from 4 commits
bff059a
76b8950
8ad7a4f
70b592b
f4efe45
04d9110
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 |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/* | ||
Copyright 2024 New Vector Ltd. | ||
|
||
SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only | ||
Please see LICENSE files in the repository root for full details. | ||
*/ | ||
|
||
import { expect, test } from "../../element-web-test"; | ||
import { autoJoin, createSecondBotDevice, createSharedRoomWithUser, verify } from "./utils"; | ||
import { bootstrapCrossSigningForClient } from "../../pages/client.ts"; | ||
|
||
/** Tests for the "invisible crypto" behaviour -- i.e., when the "exclude insecure devices" setting is enabled */ | ||
test.describe("Invisible cryptography", () => { | ||
test.use({ | ||
displayName: "Alice", | ||
botCreateOpts: { displayName: "Bob" }, | ||
labsFlags: ["feature_exclude_insecure_devices"], | ||
}); | ||
|
||
test("Messages fail to decrypt when sender is previously verified", async ({ | ||
page, | ||
bot: bob, | ||
user: aliceCredentials, | ||
app, | ||
homeserver, | ||
}) => { | ||
await app.client.bootstrapCrossSigning(aliceCredentials); | ||
await autoJoin(bob); | ||
|
||
// create an encrypted room | ||
const testRoomId = await createSharedRoomWithUser(app, bob.credentials.userId, { | ||
name: "TestRoom", | ||
initial_state: [ | ||
{ | ||
type: "m.room.encryption", | ||
state_key: "", | ||
content: { | ||
algorithm: "m.megolm.v1.aes-sha2", | ||
}, | ||
}, | ||
], | ||
}); | ||
|
||
// Verify Bob | ||
await verify(app, bob); | ||
|
||
// Bob logs in a new device and resets cross-signing | ||
const bobSecondDevice = await createSecondBotDevice(page, homeserver, bob); | ||
await bootstrapCrossSigningForClient(await bobSecondDevice.prepareClient(), bob.credentials, true); | ||
|
||
/* should show an error for a message from a previously verified device */ | ||
await bobSecondDevice.sendMessage(testRoomId, "test encrypted from user that was previously verified"); | ||
const lastTile = page.locator(".mx_EventTile_last"); | ||
await expect(lastTile).toContainText("Verified identity has changed"); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,3 +10,24 @@ Please see LICENSE files in the repository root for full details. | |
color: $secondary-content; | ||
font-style: italic; | ||
} | ||
|
||
/* Formatting for the "Verified identity has changed" error */ | ||
.mx_DecryptionFailureVerifiedIdentityChanged > span { | ||
/* Show it in red */ | ||
color: $e2e-warning-color; | ||
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. Can we use instead directly a compound token: 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. done. Thanks for the tip! |
||
/* TODO: background colour? */ | ||
|
||
/* With a red border */ | ||
border: 1px solid $e2e-warning-color; | ||
border-radius: $font-16px; | ||
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. Compound spacing token! 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. ditto 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. |
||
|
||
/* Some space inside the border */ | ||
padding: 4px 12px 4px 8px; | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/* some space between the (!) icon and text */ | ||
display: inline-flex; | ||
gap: 8px; | ||
|
||
/* Center vertically */ | ||
align-items: center; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* | ||
Copyright 2024 New Vector Ltd. | ||
SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only | ||
Please see LICENSE files in the repository root for full details. | ||
*/ | ||
|
||
import { AllDevicesIsolationMode, OnlySignedDevicesIsolationMode } from "matrix-js-sdk/src/crypto-api"; | ||
import { MatrixClient } from "matrix-js-sdk/src/matrix"; | ||
|
||
import SettingController from "./SettingController"; | ||
import { MatrixClientPeg } from "../../MatrixClientPeg"; | ||
import { SettingLevel } from "../SettingLevel"; | ||
|
||
/** | ||
* A controller for the "exclude_insecure_devices" setting, which will | ||
* update the crypto stack's device isolation mode on change. | ||
*/ | ||
export default class DeviceIsolationModeController extends SettingController { | ||
public onChange(level: SettingLevel, roomId: string, newValue: any): void { | ||
setDeviceIsolationMode(MatrixClientPeg.safeGet(), newValue); | ||
} | ||
} | ||
|
||
/** | ||
* Set the crypto stack's device isolation mode based on the current value of the | ||
* "exclude_insecure_devices" setting. | ||
* | ||
* @param client - MatrixClient to update to the new setting. | ||
* @param settingValue - value of the "exclude_insecure_devices" setting. | ||
*/ | ||
export function setDeviceIsolationMode(client: MatrixClient, settingValue: boolean): void { | ||
client | ||
.getCrypto() | ||
?.setDeviceIsolationMode( | ||
settingValue ? new OnlySignedDevicesIsolationMode() : new AllDevicesIsolationMode(true), | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
Copyright 2024 New Vector Ltd. | ||
SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only | ||
Please see LICENSE files in the repository root for full details. | ||
*/ | ||
|
||
import { AllDevicesIsolationMode, OnlySignedDevicesIsolationMode } from "matrix-js-sdk/src/crypto-api"; | ||
|
||
import { stubClient } from "../../test-utils"; | ||
import DeviceIsolationModeController from "../../../src/settings/controllers/DeviceIsolationModeController.ts"; | ||
import { SettingLevel } from "../../../src/settings/SettingLevel"; | ||
|
||
describe("DeviceIsolationModeController", () => { | ||
afterEach(() => { | ||
jest.resetAllMocks(); | ||
}); | ||
|
||
describe("tracks enabling and disabling", () => { | ||
it("on sets signed device isolation mode", () => { | ||
const cli = stubClient(); | ||
const controller = new DeviceIsolationModeController(); | ||
controller.onChange(SettingLevel.DEVICE, "", true); | ||
expect(cli.getCrypto()?.setDeviceIsolationMode).toHaveBeenCalledWith(new OnlySignedDevicesIsolationMode()); | ||
}); | ||
|
||
it("off sets all device isolation mode", () => { | ||
const cli = stubClient(); | ||
const controller = new DeviceIsolationModeController(); | ||
controller.onChange(SettingLevel.DEVICE, "", false); | ||
expect(cli.getCrypto()?.setDeviceIsolationMode).toHaveBeenCalledWith(new AllDevicesIsolationMode(true)); | ||
}); | ||
}); | ||
}); |
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.
Playwright allow to add fixture which allows to encapsulate and easily share behaviour across the tests.
You can take a look at
pinned-messages.spec.ts
to see an example.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.
Frankly, I don't find our use of fixtures very helpful, and think we should use them less rather than more. They are too "magical", and not flexible enough, whilst a separate function is very explicit, and not much more code.
In this instance: there is a bunch of preparation which has to happen before the second device is created. I don't think I can do that with a fixture?