From 6fa98f868321b916cb9b464d3969cb896c1f1b76 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 16 Oct 2024 17:32:39 -0400 Subject: [PATCH 01/10] display a warning when an unverified user's identity changes --- res/css/_components.pcss | 1 + res/css/views/rooms/_UserIdentityWarning.pcss | 26 ++ .../views/rooms/MessageComposer.tsx | 2 + .../views/rooms/UserIdentityWarning.tsx | 308 ++++++++++++++++ src/i18n/strings/en_EN.json | 2 + .../views/rooms/UserIdentityWarning-test.tsx | 342 ++++++++++++++++++ 6 files changed, 681 insertions(+) create mode 100644 res/css/views/rooms/_UserIdentityWarning.pcss create mode 100644 src/components/views/rooms/UserIdentityWarning.tsx create mode 100644 test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx diff --git a/res/css/_components.pcss b/res/css/_components.pcss index c0dd2ee0b02..4ebfcec8de3 100644 --- a/res/css/_components.pcss +++ b/res/css/_components.pcss @@ -319,6 +319,7 @@ @import "./views/rooms/_ThirdPartyMemberInfo.pcss"; @import "./views/rooms/_ThreadSummary.pcss"; @import "./views/rooms/_TopUnreadMessagesBar.pcss"; +@import "./views/rooms/_UserIdentityWarning.pcss"; @import "./views/rooms/_VoiceRecordComposerTile.pcss"; @import "./views/rooms/_WhoIsTypingTile.pcss"; @import "./views/rooms/wysiwyg_composer/_EditWysiwygComposer.pcss"; diff --git a/res/css/views/rooms/_UserIdentityWarning.pcss b/res/css/views/rooms/_UserIdentityWarning.pcss new file mode 100644 index 00000000000..bac0825e260 --- /dev/null +++ b/res/css/views/rooms/_UserIdentityWarning.pcss @@ -0,0 +1,26 @@ +/* +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. +*/ + +.mx_UserIdentityWarning { + border-top: 1px solid $separator; + padding-top: 5px; + margin-left: calc(-42px + var(--RoomView_MessageList-padding)); + display: flex; + align-items: center; + + .mx_BaseAvatar { + margin-left: 8px; + } + .mx_UserIdentityWarning_main { + margin-left: 24px; + flex-grow: 1; + } +} + +.mx_MessageComposer.mx_MessageComposer--compact > .mx_UserIdentityWarning { + margin-left: calc(-25px + var(--RoomView_MessageList-padding)); +} diff --git a/src/components/views/rooms/MessageComposer.tsx b/src/components/views/rooms/MessageComposer.tsx index e44265b9479..3e68b963a49 100644 --- a/src/components/views/rooms/MessageComposer.tsx +++ b/src/components/views/rooms/MessageComposer.tsx @@ -30,6 +30,7 @@ import E2EIcon from "./E2EIcon"; import SettingsStore from "../../../settings/SettingsStore"; import { aboveLeftOf, MenuProps } from "../../structures/ContextMenu"; import ReplyPreview from "./ReplyPreview"; +import UserIdentityWarning from "./UserIdentityWarning"; import { UPDATE_EVENT } from "../../../stores/AsyncStore"; import VoiceRecordComposerTile from "./VoiceRecordComposerTile"; import { VoiceRecordingStore } from "../../../stores/VoiceRecordingStore"; @@ -671,6 +672,7 @@ export class MessageComposer extends React.Component {
{recordingTooltip}
+ { + const verificationStatus = await crypto.getUserVerificationStatus(userId); + return verificationStatus.needsUserApproval; +} + +/** + * Displays a banner warning when there is an issue with a user's identity. + * + * Warns when an unverified user's identity has changed, and gives the user a + * button to acknowledge the change. + */ +export default class UserIdentityWarning extends React.Component { + // Which room members need their identity approved. + private membersNeedingApproval: Map; + // Whether we got a verification status update while we were fetching a + // user's verification status. + // + // We set the entry for a user to `false` when we fetch a user's + // verification status, and remove the user's entry when we are done + // fetching. When we receive a verification status update, if the entry for + // the user is `false`, we set it to `true`. After we have finished + // fetching the user's verification status, if the entry for the user is + // `true`, rather than `false`, we know that we got an update, and so we + // discard the value that we fetched. We always use the value from the + // update and consider it as the most up-to-date version. If the fetched + // value is more up-to-date, then we should be getting a new update soon + // with the newer value, so it will fix itself in the end. + private gotVerificationStatusUpdate: Map; + private mounted: boolean; + private initialised: boolean; + public constructor(props: IProps, context: React.ContextType) { + super(props, context); + this.state = { + currentPrompt: undefined, + }; + this.membersNeedingApproval = new Map(); + this.gotVerificationStatusUpdate = new Map(); + this.mounted = true; + this.initialised = false; + this.addListeners(); + } + + public componentDidMount(): void { + if (!MatrixClientPeg.safeGet().getCrypto()) return; + if (this.props.room.hasEncryptionStateEvent()) { + this.initialise().catch((e) => { + logger.error("Error initialising UserIdentityWarning:", e); + }); + } + } + + public componentWillUnmount(): void { + this.mounted = false; + this.removeListeners(); + } + + // Select a new user to display a warning for. This is called after the + // current prompted user no longer needs their identity approved. + private selectCurrentPrompt(): void { + if (this.membersNeedingApproval.size === 0) { + this.setState({ + currentPrompt: undefined, + }); + return; + } + // We return the user with the smallest user ID. + const keys = Array.from(this.membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b)); + this.setState({ + currentPrompt: this.membersNeedingApproval.get(keys[0]!), + }); + } + + // Initialise the component. Get the room members, check which ones need + // their identity approved, and pick one to display. + public async initialise(): Promise { + if (!this.mounted || this.initialised) { + return; + } + this.initialised = true; + + const crypto = MatrixClientPeg.safeGet().getCrypto()!; + const members = await this.props.room.getEncryptionTargetMembers(); + if (!this.mounted) { + return; + } + + for (const member of members) { + const userId = member.userId; + if (this.gotVerificationStatusUpdate.has(userId)) { + // We're already checking their verification status, so we don't + // need to do anything here. + continue; + } + this.gotVerificationStatusUpdate.set(userId, false); + if (await userNeedsApproval(crypto, userId)) { + if ( + !this.membersNeedingApproval.has(userId) && + this.gotVerificationStatusUpdate.get(userId) === false + ) { + this.membersNeedingApproval.set(userId, member); + } + } + this.gotVerificationStatusUpdate.delete(userId); + } + if (!this.mounted) { + return; + } + + this.selectCurrentPrompt(); + } + + private addMemberNeedingApproval(userId: string): void { + if (userId === MatrixClientPeg.safeGet().getUserId()) { + // We always skip our own user, because we can't pin our own identity. + return; + } + const member = this.props.room.getMember(userId); + if (member) { + this.membersNeedingApproval.set(userId, member); + if (!this.state.currentPrompt) { + // If we're not currently displaying a prompt, then we should + // display a prompt for this user. + this.selectCurrentPrompt(); + } + } + } + + private removeMemberNeedingApproval(userId: string): void { + this.membersNeedingApproval.delete(userId); + + // If we removed the currently displayed user, we need to pick a new one + // to display. + if (this.state.currentPrompt?.userId === userId) { + this.selectCurrentPrompt(); + } + } + + private addListeners(): void { + const cli = MatrixClientPeg.safeGet(); + cli.on(CryptoEvent.UserTrustStatusChanged, this.onUserTrustStatusChanged); + cli.on(RoomStateEvent.Events, this.onRoomStateEvent); + } + + private removeListeners(): void { + const cli = MatrixClientPeg.get(); + if (cli) { + cli.removeListener(CryptoEvent.UserTrustStatusChanged, this.onUserTrustStatusChanged); + cli.removeListener(RoomStateEvent.Events, this.onRoomStateEvent); + } + } + + private onUserTrustStatusChanged = (userId: string, verificationStatus: UserVerificationStatus): void => { + // Handle a change in user trust. If the user's identity now needs + // approval, make sure that a warning is shown. If the user's identity + // doesn't need approval, remove the warning (if any). + + if (!this.initialised) { + return; + } + + if (this.gotVerificationStatusUpdate.has(userId)) { + this.gotVerificationStatusUpdate.set(userId, true); + } + + if (verificationStatus.needsUserApproval) { + this.addMemberNeedingApproval(userId); + } else { + this.removeMemberNeedingApproval(userId); + } + }; + + private onRoomStateEvent = async (event: MatrixEvent): Promise => { + if (event.getRoomId() !== this.props.room.roomId) { + return; + } + + const eventType = event.getType(); + if (eventType === EventType.RoomEncryption && event.getStateKey() === "") { + // Room is now encrypted, so we can initialise the component. + return this.initialise().catch((e) => { + logger.error("Error initialising UserIdentityWarning:", e); + }); + } else if (eventType !== EventType.RoomMember) { + return; + } + + if (!this.initialised) { + return; + } + + const userId = event.getStateKey(); + + if (!userId) return; + + if ( + event.getContent().membership === KnownMembership.Join || + (event.getContent().membership === KnownMembership.Join && this.props.room.shouldEncryptForInvitedMembers()) + ) { + // Someone's membership changed and we will now encrypt to them. If + // their identity needs approval, show a warning. + if (this.gotVerificationStatusUpdate.has(userId)) { + // We're already checking their verification status, so we don't + // need to do anything here. + return; + } + this.gotVerificationStatusUpdate.set(userId, false); + const crypto = MatrixClientPeg.safeGet().getCrypto()!; + if (await userNeedsApproval(crypto, userId)) { + if ( + !this.membersNeedingApproval.has(userId) && + this.gotVerificationStatusUpdate.get(userId) === false + ) { + this.addMemberNeedingApproval(userId); + } + } + this.gotVerificationStatusUpdate.delete(userId); + } else { + // Someone's membership changed and we no longer encrypt to them. + // If we're showing a warning about them, we don't need to any more. + this.removeMemberNeedingApproval(userId); + } + }; + + // Callback for when the user hits the "OK" button + public confirmIdentity = async (ev: ButtonEvent): Promise => { + if (this.state.currentPrompt) { + await MatrixClientPeg.safeGet().getCrypto()!.pinCurrentUserIdentity(this.state.currentPrompt.userId); + } + }; + + public render(): React.ReactNode { + const { currentPrompt } = this.state; + if (currentPrompt) { + const substituteATag = (sub: string): React.ReactNode => ( + + {sub} + + ); + const substituteBTag = (sub: string): React.ReactNode => {sub}; + return ( +
+ + + {currentPrompt.rawDisplayName === currentPrompt.userId + ? _t( + "encryption|pinned_identity_changed_no_displayname", + { userId: currentPrompt.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + ) + : _t( + "encryption|pinned_identity_changed", + { displayName: currentPrompt.rawDisplayName, userId: currentPrompt.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + )} + + + {_t("action|ok")} + +
+ ); + } else { + return null; + } + } +} diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 7e96be0589a..560397c9fb4 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -909,6 +909,8 @@ "warning": "If you didn't set the new recovery method, an attacker may be trying to access your account. Change your account password and set a new recovery method immediately in Settings." }, "not_supported": "", + "pinned_identity_changed": "%(displayName)s's (%(userId)s) identity appears to have changed. Learn more", + "pinned_identity_changed_no_displayname": "%(userId)s's identity appears to have changed. Learn more", "recovery_method_removed": { "description_1": "This session has detected that your Security Phrase and key for Secure Messages have been removed.", "description_2": "If you did this accidentally, you can setup Secure Messages on this session which will re-encrypt this session's message history with a new recovery method.", diff --git a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx new file mode 100644 index 00000000000..b3b8cb5e71a --- /dev/null +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -0,0 +1,342 @@ +/* +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 React from "react"; +import { sleep } from "matrix-js-sdk/src/utils"; +import { + CryptoEvent, + EventType, + MatrixClient, + MatrixEvent, + Room, + RoomState, + RoomStateEvent, + RoomMember, +} from "matrix-js-sdk/src/matrix"; +import { UserVerificationStatus } from "matrix-js-sdk/src/crypto-api"; +import { fireEvent, render, screen, waitFor } from "jest-matrix-react"; + +import { stubClient } from "../../../../test-utils"; +import UserIdentityWarning from "../../../../../src/components/views/rooms/UserIdentityWarning"; +import MatrixClientContext from "../../../../../src/contexts/MatrixClientContext"; + +const ROOM_ID = "!room:id"; + +function mockRoom(): Room { + const room = { + getEncryptionTargetMembers: jest.fn(async () => []), + getMember: jest.fn((userId) => {}), + roomId: ROOM_ID, + shouldEncryptForInvitedMembers: jest.fn(() => true), + hasEncryptionStateEvent: jest.fn(() => true), + } as unknown as Room; + + return room; +} + +function mockRoomMember(userId: string, name?: string): RoomMember { + return { + userId, + name: name ?? userId, + rawDisplayName: name ?? userId, + roomId: ROOM_ID, + getMxcAvatarUrl: jest.fn(), + } as unknown as RoomMember; +} + +function dummyRoomState(): RoomState { + return new RoomState(ROOM_ID); +} + +// Get the warning element, given the warning text (excluding the "Learn more" link). +function getWarningByText(text: string): Element { + return screen.getByText((content?: string, element?: Element | null): boolean => { + return ( + !!element && + element.classList.contains("mx_UserIdentityWarning_main") && + element.textContent === text + " Learn more" + ); + }); +} + +describe("UserIdentityWarning", () => { + let client: MatrixClient; + let room: Room; + + beforeEach(async () => { + client = stubClient(); + room = mockRoom(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + // This tests the basic functionality of the component. If we have a room + // member whose identity needs accepting, we should display a warning. When + // the "OK" button gets pressed, it should call `pinCurrentUserIdentity`. + it("displays a warning when a user's identity needs approval", async () => { + room.getEncryptionTargetMembers = jest.fn(async () => { + return [mockRoomMember("@alice:example.org", "Alice")]; + }); + const crypto = client.getCrypto()!; + crypto["getUserVerificationStatus"] = jest.fn(async () => { + return Promise.resolve(new UserVerificationStatus(false, false, false, true)); + }); + crypto.pinCurrentUserIdentity = jest.fn(); + const { container } = render(, { + wrapper: ({ ...rest }) => , + }); + + await waitFor(() => + expect( + getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), + ).toBeInTheDocument(), + ); + fireEvent.click(container.querySelector("[role=button]")!); + await waitFor(() => expect(crypto.pinCurrentUserIdentity).toHaveBeenCalledWith("@alice:example.org")); + }); + + // We don't display warnings in non-encrypted rooms, but if encryption is + // enabled, then we should display a warning if there are any users whose + // identity need accepting. + it("displays pending warnings when encryption is enabled", async () => { + room.getEncryptionTargetMembers = jest.fn(async () => { + return [mockRoomMember("@alice:example.org", "Alice")]; + }); + // Start the room off unencrypted. We shouldn't display anything. + room.hasEncryptionStateEvent = jest.fn(() => false); + const crypto = client.getCrypto()!; + crypto["getUserVerificationStatus"] = jest.fn(async () => { + return Promise.resolve(new UserVerificationStatus(false, false, false, true)); + }); + + render(, { + wrapper: ({ ...rest }) => , + }); + await sleep(10); // give it some time to finish initialising + expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(); + + // Encryption gets enabled in the room. We should now warn that Alice's + // identity changed. + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomEncryption, + state_key: "", + content: { + algorithm: "m.megolm.v1.aes-sha2", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + await waitFor(() => + expect( + getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), + ).toBeInTheDocument(), + ); + }); + + // When a user's identity needs approval, or has been approved, the display + // should update appropriately. + it("updates the display when identity changes", async () => { + room.getEncryptionTargetMembers = jest.fn(async () => { + return [mockRoomMember("@alice:example.org", "Alice")]; + }); + room.getMember = jest.fn(() => mockRoomMember("@alice:example.org", "Alice")); + const crypto = client.getCrypto()!; + crypto["getUserVerificationStatus"] = jest.fn(async () => { + return Promise.resolve(new UserVerificationStatus(false, false, false, false)); + }); + render(, { + wrapper: ({ ...rest }) => , + }); + await sleep(10); // give it some time to finish initialising + expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(); + + // The user changes their identity, so we should show the warning. + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, true), + ); + await waitFor(() => + expect( + getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), + ).toBeInTheDocument(), + ); + + // Simulate the user's new identity having been approved, so we no + // longer show the warning. + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, false), + ); + await waitFor(() => + expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(), + ); + }); + + // We only display warnings about users in the room. When someone + // joins/leaves, we should update the warning appropriately. + it("updates the display when a member joins/leaves", async () => { + // Nobody in the room yet + room.getEncryptionTargetMembers = jest.fn(async () => { + return []; + }); + room.getMember = jest.fn(() => mockRoomMember("@alice:example.org", "Alice")); + const crypto = client.getCrypto()!; + crypto["getUserVerificationStatus"] = jest.fn(async () => { + return Promise.resolve(new UserVerificationStatus(false, false, false, true)); + }); + render(, { + wrapper: ({ ...rest }) => , + }); + await sleep(10); // give it some time to finish initialising + + // Alice joins. Her identity needs approval, so we should show a warning. + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@alice:example.org", + content: { + membership: "join", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + await waitFor(() => + expect( + getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), + ).toBeInTheDocument(), + ); + + // Alice leaves, so we no longer show her warning. + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@alice:example.org", + content: { + membership: "leave", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + await waitFor(() => + expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(), + ); + }); + + // When we have multiple users whose identity needs approval, one user's + // identity no longer reeds approval (e.g. their identity was approved), + // then we show the next one. + it("displays the next user when the current user's identity is approved", async () => { + room.getEncryptionTargetMembers = jest.fn(async () => { + return [mockRoomMember("@alice:example.org", "Alice"), mockRoomMember("@bob:example.org")]; + }); + const crypto = client.getCrypto()!; + crypto["getUserVerificationStatus"] = jest.fn(async () => { + return Promise.resolve(new UserVerificationStatus(false, false, false, true)); + }); + + render(, { + wrapper: ({ ...rest }) => , + }); + // We should warn about Alice's identity first. + await waitFor(() => + expect( + getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), + ).toBeInTheDocument(), + ); + + // Simulate Alice's new identity having been approved, so now we warn + // about Bob's identity. + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, false), + ); + await waitFor(() => + expect(getWarningByText("@bob:example.org's identity appears to have changed.")).toBeInTheDocument(), + ); + }); + + // If we get an update for a user's verification status while we're fetching + // that user's verification status, we should display based on the updated + // value. + describe("handles races between fetching verification status and receiving updates", () => { + // First case: check that if the update says that the user identity + // needs approval, but the fetch says it doesn't, we show the warning. + it("update says identity needs approval", async () => { + room.getEncryptionTargetMembers = jest.fn(async () => { + return [mockRoomMember("@alice:example.org", "Alice")]; + }); + room.getMember = jest.fn(() => mockRoomMember("@alice:example.org", "Alice")); + const crypto = client.getCrypto()!; + crypto["getUserVerificationStatus"] = jest.fn(async () => { + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, true), + ); + return Promise.resolve(new UserVerificationStatus(false, false, false, false)); + }); + render(, { + wrapper: ({ ...rest }) => , + }); + await sleep(10); // give it some time to finish initialising + await waitFor(() => + expect( + getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), + ).toBeInTheDocument(), + ); + }); + + // Second case: check that if the update says that the user identity + // doesn't needs approval, but the fetch says it does, we don't show the + // warning. + it("update says identity doesn't need approval", async () => { + room.getEncryptionTargetMembers = jest.fn(async () => { + return [mockRoomMember("@alice:example.org", "Alice")]; + }); + room.getMember = jest.fn(() => mockRoomMember("@alice:example.org", "Alice")); + const crypto = client.getCrypto()!; + crypto["getUserVerificationStatus"] = jest.fn(async () => { + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, false), + ); + return Promise.resolve(new UserVerificationStatus(false, false, false, true)); + }); + render(, { + wrapper: ({ ...rest }) => , + }); + await sleep(10); // give it some time to finish initialising + await waitFor(() => + expect(() => + getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), + ).toThrow(), + ); + }); + }); +}); From 9f2c1a10989ef0e9609c05bae59ee34bae17e455 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 21 Oct 2024 16:47:58 -0400 Subject: [PATCH 02/10] use Compound and make comments into doc comments --- res/css/views/rooms/_UserIdentityWarning.pcss | 22 +-- .../views/rooms/UserIdentityWarning.tsx | 130 ++++++++++-------- 2 files changed, 87 insertions(+), 65 deletions(-) diff --git a/res/css/views/rooms/_UserIdentityWarning.pcss b/res/css/views/rooms/_UserIdentityWarning.pcss index bac0825e260..b294b3fc8cf 100644 --- a/res/css/views/rooms/_UserIdentityWarning.pcss +++ b/res/css/views/rooms/_UserIdentityWarning.pcss @@ -6,18 +6,20 @@ Please see LICENSE files in the repository root for full details. */ .mx_UserIdentityWarning { - border-top: 1px solid $separator; - padding-top: 5px; + /* 42px is the padding-left of .mx_MessageComposer_wrapper in res/css/views/rooms/_MessageComposer.pcss */ margin-left: calc(-42px + var(--RoomView_MessageList-padding)); - display: flex; - align-items: center; - .mx_BaseAvatar { - margin-left: 8px; - } - .mx_UserIdentityWarning_main { - margin-left: 24px; - flex-grow: 1; + .mx_UserIdentityWarning_row { + display: flex; + align-items: center; + + .mx_BaseAvatar { + margin-left: var(--cpd-space-2x); + } + .mx_UserIdentityWarning_main { + margin-left: var(--cpd-space-6x); + flex-grow: 1; + } } } diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index d9624276a6a..05af72f1154 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -16,25 +16,31 @@ import { RoomMember, } from "matrix-js-sdk/src/matrix"; import { logger } from "matrix-js-sdk/src/logger"; +import { Button, Separator } from "@vector-im/compound-web"; import type { CryptoApi, UserVerificationStatus } from "matrix-js-sdk/src/crypto-api"; import { _t } from "../../../languageHandler"; -import AccessibleButton, { ButtonEvent } from "../elements/AccessibleButton"; import MemberAvatar from "../avatars/MemberAvatar"; import { MatrixClientPeg } from "../../../MatrixClientPeg"; import { SDKContext } from "../../../contexts/SDKContext"; -interface IProps { - // The current room being viewed. +interface UserIdentityWarningProps { + /** + * The current room being viewed. + */ room: Room; } -interface IState { - // The current room member that we are prompting the user to approve. +interface UserIdentityWarningState { + /** + * The current room member that we are prompting the user to approve. + */ currentPrompt: RoomMember | undefined; } -// Does the given user's identity need to be approved? +/** + * Does the given user's identity need to be approved? + */ async function userNeedsApproval(crypto: CryptoApi, userId: string): Promise { const verificationStatus = await crypto.getUserVerificationStatus(userId); return verificationStatus.needsUserApproval; @@ -46,26 +52,30 @@ async function userNeedsApproval(crypto: CryptoApi, userId: string): Promise { - // Which room members need their identity approved. +export default class UserIdentityWarning extends React.Component { + /** + * Which room members need their identity approved. + */ private membersNeedingApproval: Map; - // Whether we got a verification status update while we were fetching a - // user's verification status. - // - // We set the entry for a user to `false` when we fetch a user's - // verification status, and remove the user's entry when we are done - // fetching. When we receive a verification status update, if the entry for - // the user is `false`, we set it to `true`. After we have finished - // fetching the user's verification status, if the entry for the user is - // `true`, rather than `false`, we know that we got an update, and so we - // discard the value that we fetched. We always use the value from the - // update and consider it as the most up-to-date version. If the fetched - // value is more up-to-date, then we should be getting a new update soon - // with the newer value, so it will fix itself in the end. + /** + * Whether we got a verification status update while we were fetching a + * user's verification status. + * + * We set the entry for a user to `false` when we fetch a user's + * verification status, and remove the user's entry when we are done + * fetching. When we receive a verification status update, if the entry for + * the user is `false`, we set it to `true`. After we have finished + * fetching the user's verification status, if the entry for the user is + * `true`, rather than `false`, we know that we got an update, and so we + * discard the value that we fetched. We always use the value from the + * update and consider it as the most up-to-date version. If the fetched + * value is more up-to-date, then we should be getting a new update soon + * with the newer value, so it will fix itself in the end. + */ private gotVerificationStatusUpdate: Map; private mounted: boolean; private initialised: boolean; - public constructor(props: IProps, context: React.ContextType) { + public constructor(props: UserIdentityWarningProps, context: React.ContextType) { super(props, context); this.state = { currentPrompt: undefined, @@ -91,8 +101,10 @@ export default class UserIdentityWarning extends React.Component this.removeListeners(); } - // Select a new user to display a warning for. This is called after the - // current prompted user no longer needs their identity approved. + /** + * Select a new user to display a warning for. This is called after the + * current prompted user no longer needs their identity approved. + */ private selectCurrentPrompt(): void { if (this.membersNeedingApproval.size === 0) { this.setState({ @@ -107,8 +119,10 @@ export default class UserIdentityWarning extends React.Component }); } - // Initialise the component. Get the room members, check which ones need - // their identity approved, and pick one to display. + /** + * Initialise the component. Get the room members, check which ones need + * their identity approved, and pick one to display. + */ public async initialise(): Promise { if (!this.mounted || this.initialised) { return; @@ -186,11 +200,12 @@ export default class UserIdentityWarning extends React.Component } } + /** + * Handle a change in user trust. If the user's identity now needs + * approval, make sure that a warning is shown. If the user's identity + * doesn't need approval, remove the warning (if any). + */ private onUserTrustStatusChanged = (userId: string, verificationStatus: UserVerificationStatus): void => { - // Handle a change in user trust. If the user's identity now needs - // approval, make sure that a warning is shown. If the user's identity - // doesn't need approval, remove the warning (if any). - if (!this.initialised) { return; } @@ -258,8 +273,10 @@ export default class UserIdentityWarning extends React.Component } }; - // Callback for when the user hits the "OK" button - public confirmIdentity = async (ev: ButtonEvent): Promise => { + /** + * Callback for when the user hits the "OK" button. + */ + public confirmIdentity = async (): Promise => { if (this.state.currentPrompt) { await MatrixClientPeg.safeGet().getCrypto()!.pinCurrentUserIdentity(this.state.currentPrompt.userId); } @@ -276,29 +293,32 @@ export default class UserIdentityWarning extends React.Component const substituteBTag = (sub: string): React.ReactNode => {sub}; return (
- - - {currentPrompt.rawDisplayName === currentPrompt.userId - ? _t( - "encryption|pinned_identity_changed_no_displayname", - { userId: currentPrompt.userId }, - { - a: substituteATag, - b: substituteBTag, - }, - ) - : _t( - "encryption|pinned_identity_changed", - { displayName: currentPrompt.rawDisplayName, userId: currentPrompt.userId }, - { - a: substituteATag, - b: substituteBTag, - }, - )} - - - {_t("action|ok")} - + +
+ + + {currentPrompt.rawDisplayName === currentPrompt.userId + ? _t( + "encryption|pinned_identity_changed_no_displayname", + { userId: currentPrompt.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + ) + : _t( + "encryption|pinned_identity_changed", + { displayName: currentPrompt.rawDisplayName, userId: currentPrompt.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + )} + + +
); } else { From 0d186dd3acd82f40e0d737b921b04a023c3ada66 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 22 Oct 2024 14:39:56 -0400 Subject: [PATCH 03/10] refactor to use functional component --- .../views/rooms/MessageComposer.tsx | 4 +- .../views/rooms/UserIdentityWarning.tsx | 447 ++++++++---------- .../views/rooms/UserIdentityWarning-test.tsx | 86 ++-- 3 files changed, 250 insertions(+), 287 deletions(-) diff --git a/src/components/views/rooms/MessageComposer.tsx b/src/components/views/rooms/MessageComposer.tsx index 3e68b963a49..73a048d3dda 100644 --- a/src/components/views/rooms/MessageComposer.tsx +++ b/src/components/views/rooms/MessageComposer.tsx @@ -30,7 +30,7 @@ import E2EIcon from "./E2EIcon"; import SettingsStore from "../../../settings/SettingsStore"; import { aboveLeftOf, MenuProps } from "../../structures/ContextMenu"; import ReplyPreview from "./ReplyPreview"; -import UserIdentityWarning from "./UserIdentityWarning"; +import { UserIdentityWarning } from "./UserIdentityWarning"; import { UPDATE_EVENT } from "../../../stores/AsyncStore"; import VoiceRecordComposerTile from "./VoiceRecordComposerTile"; import { VoiceRecordingStore } from "../../../stores/VoiceRecordingStore"; @@ -672,7 +672,7 @@ export class MessageComposer extends React.Component {
{recordingTooltip}
- + { - /** - * Which room members need their identity approved. - */ - private membersNeedingApproval: Map; - /** - * Whether we got a verification status update while we were fetching a - * user's verification status. - * - * We set the entry for a user to `false` when we fetch a user's - * verification status, and remove the user's entry when we are done - * fetching. When we receive a verification status update, if the entry for - * the user is `false`, we set it to `true`. After we have finished - * fetching the user's verification status, if the entry for the user is - * `true`, rather than `false`, we know that we got an update, and so we - * discard the value that we fetched. We always use the value from the - * update and consider it as the most up-to-date version. If the fetched - * value is more up-to-date, then we should be getting a new update soon - * with the newer value, so it will fix itself in the end. - */ - private gotVerificationStatusUpdate: Map; - private mounted: boolean; - private initialised: boolean; - public constructor(props: UserIdentityWarningProps, context: React.ContextType) { - super(props, context); - this.state = { - currentPrompt: undefined, - }; - this.membersNeedingApproval = new Map(); - this.gotVerificationStatusUpdate = new Map(); - this.mounted = true; - this.initialised = false; - this.addListeners(); - } +export const UserIdentityWarning: React.FC = ({ room }) => { + const cli = MatrixClientPeg.safeGet(); + const crypto = cli.getCrypto(); - public componentDidMount(): void { - if (!MatrixClientPeg.safeGet().getCrypto()) return; - if (this.props.room.hasEncryptionStateEvent()) { - this.initialise().catch((e) => { - logger.error("Error initialising UserIdentityWarning:", e); - }); - } - } + // The current room member that we are prompting the user to approve. + const [currentPrompt, setCurrentPrompt] = useState(undefined); - public componentWillUnmount(): void { - this.mounted = false; - this.removeListeners(); - } + // Whether or not we've already initialised the component by loading the + // room membership. + const initialisedRef = useRef(false); + // Which room members need their identity approved. + const membersNeedingApprovalRef = useRef>(new Map()); + // Whether we got a verification status update while we were fetching a + // user's verification status. + // + // We set the entry for a user to `false` when we fetch a user's + // verification status, and remove the user's entry when we are done + // fetching. When we receive a verification status update, if the entry for + // the user is `false`, we set it to `true`. After we have finished + // fetching the user's verification status, if the entry for the user is + // `true`, rather than `false`, we know that we got an update, and so we + // discard the value that we fetched. We always use the value from the + // update and consider it as the most up-to-date version. If the fetched + // value is more up-to-date, then we should be getting a new update soon + // with the newer value, so it will fix itself in the end. + const gotVerificationStatusUpdateRef = useRef>(new Map()); - /** - * Select a new user to display a warning for. This is called after the - * current prompted user no longer needs their identity approved. - */ - private selectCurrentPrompt(): void { - if (this.membersNeedingApproval.size === 0) { - this.setState({ - currentPrompt: undefined, - }); - return; - } - // We return the user with the smallest user ID. - const keys = Array.from(this.membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b)); - this.setState({ - currentPrompt: this.membersNeedingApproval.get(keys[0]!), - }); - } + useEffect(() => { + if (!crypto) return; - /** - * Initialise the component. Get the room members, check which ones need - * their identity approved, and pick one to display. - */ - public async initialise(): Promise { - if (!this.mounted || this.initialised) { - return; - } - this.initialised = true; + const membersNeedingApproval = membersNeedingApprovalRef.current; + const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; - const crypto = MatrixClientPeg.safeGet().getCrypto()!; - const members = await this.props.room.getEncryptionTargetMembers(); - if (!this.mounted) { - return; + /** + * Select a new user to display a warning for. This is called after the + * current prompted user no longer needs their identity approved. + */ + function selectCurrentPrompt(): void { + if (membersNeedingApproval.size === 0) { + setCurrentPrompt(undefined); + return; + } + + // We return the user with the smallest user ID. + const keys = Array.from(membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b)); + setCurrentPrompt(membersNeedingApproval.get(keys[0]!)); } - for (const member of members) { - const userId = member.userId; - if (this.gotVerificationStatusUpdate.has(userId)) { - // We're already checking their verification status, so we don't - // need to do anything here. - continue; + function addMemberNeedingApproval(userId: string): void { + if (userId === cli.getUserId()) { + // We always skip our own user, because we can't pin our own identity. + return; } - this.gotVerificationStatusUpdate.set(userId, false); - if (await userNeedsApproval(crypto, userId)) { - if ( - !this.membersNeedingApproval.has(userId) && - this.gotVerificationStatusUpdate.get(userId) === false - ) { - this.membersNeedingApproval.set(userId, member); + const member = room.getMember(userId); + if (member) { + membersNeedingApproval.set(userId, member); + if (!currentPrompt) { + // If we're not currently displaying a prompt, then we should + // display a prompt for this user. + selectCurrentPrompt(); } } - this.gotVerificationStatusUpdate.delete(userId); - } - if (!this.mounted) { - return; } - this.selectCurrentPrompt(); - } + function removeMemberNeedingApproval(userId: string): void { + membersNeedingApproval.delete(userId); - private addMemberNeedingApproval(userId: string): void { - if (userId === MatrixClientPeg.safeGet().getUserId()) { - // We always skip our own user, because we can't pin our own identity. - return; - } - const member = this.props.room.getMember(userId); - if (member) { - this.membersNeedingApproval.set(userId, member); - if (!this.state.currentPrompt) { - // If we're not currently displaying a prompt, then we should - // display a prompt for this user. - this.selectCurrentPrompt(); + // If we removed the currently displayed user, we need to pick a new one + // to display. + if (currentPrompt?.userId === userId) { + selectCurrentPrompt(); } } - } - private removeMemberNeedingApproval(userId: string): void { - this.membersNeedingApproval.delete(userId); + /** + * Initialise the component. Get the room members, check which ones need + * their identity approved, and pick one to display. + */ + async function initialise(): Promise { + if (initialisedRef.current) { + return; + } + initialisedRef.current = true; - // If we removed the currently displayed user, we need to pick a new one - // to display. - if (this.state.currentPrompt?.userId === userId) { - this.selectCurrentPrompt(); - } - } + const members = await room.getEncryptionTargetMembers(); - private addListeners(): void { - const cli = MatrixClientPeg.safeGet(); - cli.on(CryptoEvent.UserTrustStatusChanged, this.onUserTrustStatusChanged); - cli.on(RoomStateEvent.Events, this.onRoomStateEvent); - } + for (const member of members) { + const userId = member.userId; + if (gotVerificationStatusUpdate.has(userId)) { + // We're already checking their verification status, so we don't + // need to do anything here. + continue; + } + gotVerificationStatusUpdate.set(userId, false); + if (await userNeedsApproval(crypto!, userId)) { + if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { + membersNeedingApproval.set(userId, member); + } + } + gotVerificationStatusUpdate.delete(userId); + } - private removeListeners(): void { - const cli = MatrixClientPeg.get(); - if (cli) { - cli.removeListener(CryptoEvent.UserTrustStatusChanged, this.onUserTrustStatusChanged); - cli.removeListener(RoomStateEvent.Events, this.onRoomStateEvent); + selectCurrentPrompt(); } - } - /** - * Handle a change in user trust. If the user's identity now needs - * approval, make sure that a warning is shown. If the user's identity - * doesn't need approval, remove the warning (if any). - */ - private onUserTrustStatusChanged = (userId: string, verificationStatus: UserVerificationStatus): void => { - if (!this.initialised) { - return; - } + const onUserTrustStatusChanged = (userId: string, verificationStatus: UserVerificationStatus): void => { + if (!initialisedRef.current) { + return; + } - if (this.gotVerificationStatusUpdate.has(userId)) { - this.gotVerificationStatusUpdate.set(userId, true); - } + if (gotVerificationStatusUpdate.has(userId)) { + gotVerificationStatusUpdate.set(userId, true); + } - if (verificationStatus.needsUserApproval) { - this.addMemberNeedingApproval(userId); - } else { - this.removeMemberNeedingApproval(userId); - } - }; + if (verificationStatus.needsUserApproval) { + addMemberNeedingApproval(userId); + } else { + removeMemberNeedingApproval(userId); + } + }; - private onRoomStateEvent = async (event: MatrixEvent): Promise => { - if (event.getRoomId() !== this.props.room.roomId) { - return; - } + const onRoomStateEvent = async (event: MatrixEvent): Promise => { + if (event.getRoomId() !== room.roomId) { + return; + } - const eventType = event.getType(); - if (eventType === EventType.RoomEncryption && event.getStateKey() === "") { - // Room is now encrypted, so we can initialise the component. - return this.initialise().catch((e) => { - logger.error("Error initialising UserIdentityWarning:", e); - }); - } else if (eventType !== EventType.RoomMember) { - return; - } + const eventType = event.getType(); + if (eventType === EventType.RoomEncryption && event.getStateKey() === "") { + // Room is now encrypted, so we can initialise the component. + return initialise().catch((e) => { + logger.error("Error initialising UserIdentityWarning:", e); + }); + } else if (eventType !== EventType.RoomMember) { + return; + } - if (!this.initialised) { - return; - } + if (!initialisedRef.current) { + return; + } - const userId = event.getStateKey(); + const userId = event.getStateKey(); - if (!userId) return; + if (!userId) return; - if ( - event.getContent().membership === KnownMembership.Join || - (event.getContent().membership === KnownMembership.Join && this.props.room.shouldEncryptForInvitedMembers()) - ) { - // Someone's membership changed and we will now encrypt to them. If - // their identity needs approval, show a warning. - if (this.gotVerificationStatusUpdate.has(userId)) { - // We're already checking their verification status, so we don't - // need to do anything here. - return; - } - this.gotVerificationStatusUpdate.set(userId, false); - const crypto = MatrixClientPeg.safeGet().getCrypto()!; - if (await userNeedsApproval(crypto, userId)) { - if ( - !this.membersNeedingApproval.has(userId) && - this.gotVerificationStatusUpdate.get(userId) === false - ) { - this.addMemberNeedingApproval(userId); + if ( + event.getContent().membership === KnownMembership.Join || + (event.getContent().membership === KnownMembership.Join && room.shouldEncryptForInvitedMembers()) + ) { + // Someone's membership changed and we will now encrypt to them. If + // their identity needs approval, show a warning. + if (gotVerificationStatusUpdate.has(userId)) { + // We're already checking their verification status, so we don't + // need to do anything here. + return; } + gotVerificationStatusUpdate.set(userId, false); + const crypto = MatrixClientPeg.safeGet().getCrypto()!; + if (await userNeedsApproval(crypto!, userId)) { + if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { + addMemberNeedingApproval(userId); + } + } + gotVerificationStatusUpdate.delete(userId); + } else { + // Someone's membership changed and we no longer encrypt to them. + // If we're showing a warning about them, we don't need to any more. + removeMemberNeedingApproval(userId); } - this.gotVerificationStatusUpdate.delete(userId); - } else { - // Someone's membership changed and we no longer encrypt to them. - // If we're showing a warning about them, we don't need to any more. - this.removeMemberNeedingApproval(userId); - } - }; + }; - /** - * Callback for when the user hits the "OK" button. - */ - public confirmIdentity = async (): Promise => { - if (this.state.currentPrompt) { - await MatrixClientPeg.safeGet().getCrypto()!.pinCurrentUserIdentity(this.state.currentPrompt.userId); + cli.on(CryptoEvent.UserTrustStatusChanged, onUserTrustStatusChanged); + cli.on(RoomStateEvent.Events, onRoomStateEvent); + + if (room.hasEncryptionStateEvent()) { + initialise().catch((e) => { + logger.error("Error initialising UserIdentityWarning:", e); + }); } - }; - - public render(): React.ReactNode { - const { currentPrompt } = this.state; - if (currentPrompt) { - const substituteATag = (sub: string): React.ReactNode => ( - - {sub} - - ); - const substituteBTag = (sub: string): React.ReactNode => {sub}; - return ( -
- -
- - - {currentPrompt.rawDisplayName === currentPrompt.userId - ? _t( - "encryption|pinned_identity_changed_no_displayname", - { userId: currentPrompt.userId }, - { - a: substituteATag, - b: substituteBTag, - }, - ) - : _t( - "encryption|pinned_identity_changed", - { displayName: currentPrompt.rawDisplayName, userId: currentPrompt.userId }, - { - a: substituteATag, - b: substituteBTag, - }, - )} - - -
+ + return () => { + cli.removeListener(CryptoEvent.UserTrustStatusChanged, onUserTrustStatusChanged); + cli.removeListener(RoomStateEvent.Events, onRoomStateEvent); + }; + }, [currentPrompt, room, cli, crypto]); + + if (!crypto) return null; + + if (currentPrompt) { + const confirmIdentity = async (): Promise => { + await crypto.pinCurrentUserIdentity(currentPrompt.userId); + }; + + const substituteATag = (sub: string): React.ReactNode => ( + + {sub} + + ); + const substituteBTag = (sub: string): React.ReactNode => {sub}; + return ( +
+ +
+ + + {currentPrompt.rawDisplayName === currentPrompt.userId + ? _t( + "encryption|pinned_identity_changed_no_displayname", + { userId: currentPrompt.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + ) + : _t( + "encryption|pinned_identity_changed", + { displayName: currentPrompt.rawDisplayName, userId: currentPrompt.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + )} + +
- ); - } else { - return null; - } +
+ ); + } else { + return null; } -} +}; diff --git a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx index b3b8cb5e71a..94bbf982b11 100644 --- a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -18,10 +18,10 @@ import { RoomMember, } from "matrix-js-sdk/src/matrix"; import { UserVerificationStatus } from "matrix-js-sdk/src/crypto-api"; -import { fireEvent, render, screen, waitFor } from "jest-matrix-react"; +import { act, fireEvent, render, screen, waitFor } from "jest-matrix-react"; import { stubClient } from "../../../../test-utils"; -import UserIdentityWarning from "../../../../../src/components/views/rooms/UserIdentityWarning"; +import { UserIdentityWarning } from "../../../../../src/components/views/rooms/UserIdentityWarning"; import MatrixClientContext from "../../../../../src/contexts/MatrixClientContext"; const ROOM_ID = "!room:id"; @@ -88,7 +88,7 @@ describe("UserIdentityWarning", () => { return Promise.resolve(new UserVerificationStatus(false, false, false, true)); }); crypto.pinCurrentUserIdentity = jest.fn(); - const { container } = render(, { + const { container } = render(, { wrapper: ({ ...rest }) => , }); @@ -115,7 +115,7 @@ describe("UserIdentityWarning", () => { return Promise.resolve(new UserVerificationStatus(false, false, false, true)); }); - render(, { + render(, { wrapper: ({ ...rest }) => , }); await sleep(10); // give it some time to finish initialising @@ -156,18 +156,20 @@ describe("UserIdentityWarning", () => { crypto["getUserVerificationStatus"] = jest.fn(async () => { return Promise.resolve(new UserVerificationStatus(false, false, false, false)); }); - render(, { + render(, { wrapper: ({ ...rest }) => , }); await sleep(10); // give it some time to finish initialising expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(); // The user changes their identity, so we should show the warning. - client.emit( - CryptoEvent.UserTrustStatusChanged, - "@alice:example.org", - new UserVerificationStatus(false, false, false, true), - ); + act(() => { + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, true), + ); + }); await waitFor(() => expect( getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), @@ -176,11 +178,13 @@ describe("UserIdentityWarning", () => { // Simulate the user's new identity having been approved, so we no // longer show the warning. - client.emit( - CryptoEvent.UserTrustStatusChanged, - "@alice:example.org", - new UserVerificationStatus(false, false, false, false), - ); + act(() => { + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, false), + ); + }); await waitFor(() => expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(), ); @@ -198,7 +202,7 @@ describe("UserIdentityWarning", () => { crypto["getUserVerificationStatus"] = jest.fn(async () => { return Promise.resolve(new UserVerificationStatus(false, false, false, true)); }); - render(, { + render(, { wrapper: ({ ...rest }) => , }); await sleep(10); // give it some time to finish initialising @@ -226,21 +230,23 @@ describe("UserIdentityWarning", () => { ); // Alice leaves, so we no longer show her warning. - client.emit( - RoomStateEvent.Events, - new MatrixEvent({ - event_id: "$event_id", - type: EventType.RoomMember, - state_key: "@alice:example.org", - content: { - membership: "leave", - }, - room_id: ROOM_ID, - sender: "@alice:example.org", - }), - dummyRoomState(), - null, - ); + act(() => { + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@alice:example.org", + content: { + membership: "leave", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + }); await waitFor(() => expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(), ); @@ -258,7 +264,7 @@ describe("UserIdentityWarning", () => { return Promise.resolve(new UserVerificationStatus(false, false, false, true)); }); - render(, { + render(, { wrapper: ({ ...rest }) => , }); // We should warn about Alice's identity first. @@ -270,11 +276,13 @@ describe("UserIdentityWarning", () => { // Simulate Alice's new identity having been approved, so now we warn // about Bob's identity. - client.emit( - CryptoEvent.UserTrustStatusChanged, - "@alice:example.org", - new UserVerificationStatus(false, false, false, false), - ); + act(() => { + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, false), + ); + }); await waitFor(() => expect(getWarningByText("@bob:example.org's identity appears to have changed.")).toBeInTheDocument(), ); @@ -300,7 +308,7 @@ describe("UserIdentityWarning", () => { ); return Promise.resolve(new UserVerificationStatus(false, false, false, false)); }); - render(, { + render(, { wrapper: ({ ...rest }) => , }); await sleep(10); // give it some time to finish initialising @@ -328,7 +336,7 @@ describe("UserIdentityWarning", () => { ); return Promise.resolve(new UserVerificationStatus(false, false, false, true)); }); - render(, { + render(, { wrapper: ({ ...rest }) => , }); await sleep(10); // give it some time to finish initialising From 0b1e7adf91a1ff3033c3d0da9a5ee69a7d9aa365 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 23 Oct 2024 15:34:30 -0400 Subject: [PATCH 04/10] split into multiple hooks --- .../views/rooms/UserIdentityWarning.tsx | 268 ++++++++++-------- 1 file changed, 147 insertions(+), 121 deletions(-) diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index 39d37523f46..50da5dfd581 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only Please see LICENSE files in the repository root for full details. */ -import React, { useEffect, useRef, useState } from "react"; +import React, { useCallback, useRef, useState } from "react"; import { CryptoEvent, EventType, @@ -21,7 +21,8 @@ import { Button, Separator } from "@vector-im/compound-web"; import type { CryptoApi, UserVerificationStatus } from "matrix-js-sdk/src/crypto-api"; import { _t } from "../../../languageHandler"; import MemberAvatar from "../avatars/MemberAvatar"; -import { MatrixClientPeg } from "../../../MatrixClientPeg"; +import { useMatrixClientContext } from "../../../contexts/MatrixClientContext"; +import { useTypedEventEmitter } from "../../../hooks/useEventEmitter"; interface UserIdentityWarningProps { /** @@ -50,7 +51,7 @@ async function userNeedsApproval(crypto: CryptoApi, userId: string): Promise = ({ room }) => { - const cli = MatrixClientPeg.safeGet(); + const cli = useMatrixClientContext(); const crypto = cli.getCrypto(); // The current room member that we are prompting the user to approve. @@ -76,85 +77,103 @@ export const UserIdentityWarning: React.FC = ({ room } // with the newer value, so it will fix itself in the end. const gotVerificationStatusUpdateRef = useRef>(new Map()); - useEffect(() => { - if (!crypto) return; - + // Select a new user to display a warning for. This is called after the + // current prompted user no longer needs their identity approved. + const selectCurrentPrompt = useCallback((): void => { const membersNeedingApproval = membersNeedingApprovalRef.current; - const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; - - /** - * Select a new user to display a warning for. This is called after the - * current prompted user no longer needs their identity approved. - */ - function selectCurrentPrompt(): void { - if (membersNeedingApproval.size === 0) { - setCurrentPrompt(undefined); - return; - } - - // We return the user with the smallest user ID. - const keys = Array.from(membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b)); - setCurrentPrompt(membersNeedingApproval.get(keys[0]!)); + if (membersNeedingApproval.size === 0) { + setCurrentPrompt(undefined); + return; } - function addMemberNeedingApproval(userId: string): void { + // We return the user with the smallest user ID. + const keys = Array.from(membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b)); + setCurrentPrompt(membersNeedingApproval.get(keys[0]!)); + }, [membersNeedingApprovalRef]); + + // Add a user to the membersNeedingApproval map, and update the current + // prompt if necessary. + const addMemberNeedingApproval = useCallback( + (userId: string): void => { if (userId === cli.getUserId()) { // We always skip our own user, because we can't pin our own identity. return; } const member = room.getMember(userId); if (member) { - membersNeedingApproval.set(userId, member); + membersNeedingApprovalRef.current.set(userId, member); if (!currentPrompt) { // If we're not currently displaying a prompt, then we should // display a prompt for this user. selectCurrentPrompt(); } } - } + }, + [cli, room, membersNeedingApprovalRef, currentPrompt, selectCurrentPrompt], + ); - function removeMemberNeedingApproval(userId: string): void { - membersNeedingApproval.delete(userId); + // Add a user from the membersNeedingApproval map, and update the current + // prompt if necessary. + const removeMemberNeedingApproval = useCallback( + (userId: string): void => { + membersNeedingApprovalRef.current.delete(userId); // If we removed the currently displayed user, we need to pick a new one // to display. if (currentPrompt?.userId === userId) { selectCurrentPrompt(); } + }, + [membersNeedingApprovalRef, currentPrompt, selectCurrentPrompt], + ); + + // Initialise the component. Get the room members, check which ones need + // their identity approved, and pick one to display. + const loadMembers = useCallback(async (): Promise => { + if (!crypto || initialisedRef.current) { + return; } + initialisedRef.current = true; - /** - * Initialise the component. Get the room members, check which ones need - * their identity approved, and pick one to display. - */ - async function initialise(): Promise { - if (initialisedRef.current) { - return; - } - initialisedRef.current = true; + const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; + const membersNeedingApproval = membersNeedingApprovalRef.current; - const members = await room.getEncryptionTargetMembers(); + const members = await room.getEncryptionTargetMembers(); - for (const member of members) { - const userId = member.userId; - if (gotVerificationStatusUpdate.has(userId)) { - // We're already checking their verification status, so we don't - // need to do anything here. - continue; - } - gotVerificationStatusUpdate.set(userId, false); - if (await userNeedsApproval(crypto!, userId)) { - if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { - membersNeedingApproval.set(userId, member); - } + for (const member of members) { + const userId = member.userId; + if (gotVerificationStatusUpdate.has(userId)) { + // We're already checking their verification status, so we don't + // need to do anything here. + continue; + } + gotVerificationStatusUpdate.set(userId, false); + if (await userNeedsApproval(crypto, userId)) { + if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { + membersNeedingApproval.set(userId, member); } - gotVerificationStatusUpdate.delete(userId); } - - selectCurrentPrompt(); + gotVerificationStatusUpdate.delete(userId); } - const onUserTrustStatusChanged = (userId: string, verificationStatus: UserVerificationStatus): void => { + selectCurrentPrompt(); + }, [crypto, room, initialisedRef, gotVerificationStatusUpdateRef, membersNeedingApprovalRef, selectCurrentPrompt]); + + // If the room has encryption enabled, we load the room members right away. + // If not, we wait until encryption is enabled before loading the room + // members, since we don't need to display anything in unencrypted rooms. + if (crypto && room.hasEncryptionStateEvent()) { + loadMembers().catch((e) => { + logger.error("Error initialising UserIdentityWarning:", e); + }); + } + + // When a user's verification status changes, we check if they need to be + // added/removed from the set of members needing approval. + const onUserVerificationStatusChanged = useCallback( + (userId: string, verificationStatus: UserVerificationStatus): void => { + const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; + if (!initialisedRef.current) { return; } @@ -168,17 +187,27 @@ export const UserIdentityWarning: React.FC = ({ room } } else { removeMemberNeedingApproval(userId); } - }; - - const onRoomStateEvent = async (event: MatrixEvent): Promise => { - if (event.getRoomId() !== room.roomId) { + }, + [initialisedRef, gotVerificationStatusUpdateRef, addMemberNeedingApproval, removeMemberNeedingApproval], + ); + useTypedEventEmitter(cli, CryptoEvent.UserTrustStatusChanged, onUserVerificationStatusChanged); + + // We watch for encryption events (since we only display warnings in + // encrypted rooms), and for membership changes (since we only display + // warnings for users in the room). + const onRoomStateEvent = useCallback( + async (event: MatrixEvent): Promise => { + if (!crypto || event.getRoomId() !== room.roomId) { return; } + const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; + const membersNeedingApproval = membersNeedingApprovalRef.current; + const eventType = event.getType(); if (eventType === EventType.RoomEncryption && event.getStateKey() === "") { // Room is now encrypted, so we can initialise the component. - return initialise().catch((e) => { + return loadMembers().catch((e) => { logger.error("Error initialising UserIdentityWarning:", e); }); } else if (eventType !== EventType.RoomMember) { @@ -205,8 +234,7 @@ export const UserIdentityWarning: React.FC = ({ room } return; } gotVerificationStatusUpdate.set(userId, false); - const crypto = MatrixClientPeg.safeGet().getCrypto()!; - if (await userNeedsApproval(crypto!, userId)) { + if (await userNeedsApproval(crypto, userId)) { if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { addMemberNeedingApproval(userId); } @@ -217,67 +245,65 @@ export const UserIdentityWarning: React.FC = ({ room } // If we're showing a warning about them, we don't need to any more. removeMemberNeedingApproval(userId); } - }; - - cli.on(CryptoEvent.UserTrustStatusChanged, onUserTrustStatusChanged); - cli.on(RoomStateEvent.Events, onRoomStateEvent); - - if (room.hasEncryptionStateEvent()) { - initialise().catch((e) => { - logger.error("Error initialising UserIdentityWarning:", e); - }); - } - - return () => { - cli.removeListener(CryptoEvent.UserTrustStatusChanged, onUserTrustStatusChanged); - cli.removeListener(RoomStateEvent.Events, onRoomStateEvent); - }; - }, [currentPrompt, room, cli, crypto]); - - if (!crypto) return null; - - if (currentPrompt) { - const confirmIdentity = async (): Promise => { - await crypto.pinCurrentUserIdentity(currentPrompt.userId); - }; - - const substituteATag = (sub: string): React.ReactNode => ( - - {sub} - - ); - const substituteBTag = (sub: string): React.ReactNode => {sub}; - return ( -
- -
- - - {currentPrompt.rawDisplayName === currentPrompt.userId - ? _t( - "encryption|pinned_identity_changed_no_displayname", - { userId: currentPrompt.userId }, - { - a: substituteATag, - b: substituteBTag, - }, - ) - : _t( - "encryption|pinned_identity_changed", - { displayName: currentPrompt.rawDisplayName, userId: currentPrompt.userId }, - { - a: substituteATag, - b: substituteBTag, - }, - )} - - -
+ }, + [ + crypto, + room, + gotVerificationStatusUpdateRef, + membersNeedingApprovalRef, + addMemberNeedingApproval, + removeMemberNeedingApproval, + loadMembers, + ], + ); + useTypedEventEmitter(cli, RoomStateEvent.Events, onRoomStateEvent); + + if (!crypto || !currentPrompt) return null; + + const confirmIdentity = async (): Promise => { + await crypto.pinCurrentUserIdentity(currentPrompt.userId); + }; + + return ( +
+ +
+ + + {currentPrompt.rawDisplayName === currentPrompt.userId + ? _t( + "encryption|pinned_identity_changed_no_displayname", + { userId: currentPrompt.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + ) + : _t( + "encryption|pinned_identity_changed", + { displayName: currentPrompt.rawDisplayName, userId: currentPrompt.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + )} + +
- ); - } else { - return null; - } +
+ ); }; + +function substituteATag(sub: string): React.ReactNode { + return ( + + {sub} + + ); +} + +function substituteBTag(sub: string): React.ReactNode { + return {sub}; +} From f4b403d23669b1b2a7ab6668e9a520098b7dee92 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 24 Oct 2024 18:40:38 -0400 Subject: [PATCH 05/10] apply minor changes from review --- .../views/rooms/UserIdentityWarning.tsx | 12 +- .../views/rooms/UserIdentityWarning-test.tsx | 159 +++++++++--------- 2 files changed, 81 insertions(+), 90 deletions(-) diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index 50da5dfd581..6e674d7ef82 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -6,19 +6,11 @@ Please see LICENSE files in the repository root for full details. */ import React, { useCallback, useRef, useState } from "react"; -import { - CryptoEvent, - EventType, - KnownMembership, - MatrixEvent, - Room, - RoomStateEvent, - RoomMember, -} from "matrix-js-sdk/src/matrix"; +import { EventType, KnownMembership, MatrixEvent, Room, RoomStateEvent, RoomMember } from "matrix-js-sdk/src/matrix"; +import { CryptoApi, CryptoEvent, UserVerificationStatus } from "matrix-js-sdk/src/crypto-api"; import { logger } from "matrix-js-sdk/src/logger"; import { Button, Separator } from "@vector-im/compound-web"; -import type { CryptoApi, UserVerificationStatus } from "matrix-js-sdk/src/crypto-api"; import { _t } from "../../../languageHandler"; import MemberAvatar from "../avatars/MemberAvatar"; import { useMatrixClientContext } from "../../../contexts/MatrixClientContext"; diff --git a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx index 94bbf982b11..c02449a36d8 100644 --- a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -8,7 +8,6 @@ Please see LICENSE files in the repository root for full details. import React from "react"; import { sleep } from "matrix-js-sdk/src/utils"; import { - CryptoEvent, EventType, MatrixClient, MatrixEvent, @@ -17,8 +16,9 @@ import { RoomStateEvent, RoomMember, } from "matrix-js-sdk/src/matrix"; -import { UserVerificationStatus } from "matrix-js-sdk/src/crypto-api"; -import { act, fireEvent, render, screen, waitFor } from "jest-matrix-react"; +import { CryptoEvent, UserVerificationStatus } from "matrix-js-sdk/src/crypto-api"; +import { act, render, screen, waitFor } from "jest-matrix-react"; +import userEvent from "@testing-library/user-event"; import { stubClient } from "../../../../test-utils"; import { UserIdentityWarning } from "../../../../../src/components/views/rooms/UserIdentityWarning"; @@ -52,7 +52,11 @@ function dummyRoomState(): RoomState { return new RoomState(ROOM_ID); } -// Get the warning element, given the warning text (excluding the "Learn more" link). +/** + * Get the warning element, given the warning text (excluding the "Learn more" + * link). This is needed because the warning text contains a `` tag, so the + * normal `getByText` doesn't work. + */ function getWarningByText(text: string): Element { return screen.getByText((content?: string, element?: Element | null): boolean => { return ( @@ -63,6 +67,12 @@ function getWarningByText(text: string): Element { }); } +function renderComponent(client: MatrixClient, room: Room) { + return render(, { + wrapper: ({ ...rest }) => , + }); +} + describe("UserIdentityWarning", () => { let client: MatrixClient; let room: Room; @@ -80,24 +90,22 @@ describe("UserIdentityWarning", () => { // member whose identity needs accepting, we should display a warning. When // the "OK" button gets pressed, it should call `pinCurrentUserIdentity`. it("displays a warning when a user's identity needs approval", async () => { - room.getEncryptionTargetMembers = jest.fn(async () => { - return [mockRoomMember("@alice:example.org", "Alice")]; - }); + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ + mockRoomMember("@alice:example.org", "Alice"), + ]); const crypto = client.getCrypto()!; - crypto["getUserVerificationStatus"] = jest.fn(async () => { - return Promise.resolve(new UserVerificationStatus(false, false, false, true)); - }); + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); crypto.pinCurrentUserIdentity = jest.fn(); - const { container } = render(, { - wrapper: ({ ...rest }) => , - }); + renderComponent(client, room); await waitFor(() => expect( getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), ).toBeInTheDocument(), ); - fireEvent.click(container.querySelector("[role=button]")!); + await userEvent.click(screen.getByRole("button")!); await waitFor(() => expect(crypto.pinCurrentUserIdentity).toHaveBeenCalledWith("@alice:example.org")); }); @@ -105,19 +113,17 @@ describe("UserIdentityWarning", () => { // enabled, then we should display a warning if there are any users whose // identity need accepting. it("displays pending warnings when encryption is enabled", async () => { - room.getEncryptionTargetMembers = jest.fn(async () => { - return [mockRoomMember("@alice:example.org", "Alice")]; - }); + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ + mockRoomMember("@alice:example.org", "Alice"), + ]); // Start the room off unencrypted. We shouldn't display anything. - room.hasEncryptionStateEvent = jest.fn(() => false); + jest.spyOn(room, "hasEncryptionStateEvent").mockReturnValue(false); const crypto = client.getCrypto()!; - crypto["getUserVerificationStatus"] = jest.fn(async () => { - return Promise.resolve(new UserVerificationStatus(false, false, false, true)); - }); + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); - render(, { - wrapper: ({ ...rest }) => , - }); + renderComponent(client, room); await sleep(10); // give it some time to finish initialising expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(); @@ -148,17 +154,15 @@ describe("UserIdentityWarning", () => { // When a user's identity needs approval, or has been approved, the display // should update appropriately. it("updates the display when identity changes", async () => { - room.getEncryptionTargetMembers = jest.fn(async () => { - return [mockRoomMember("@alice:example.org", "Alice")]; - }); - room.getMember = jest.fn(() => mockRoomMember("@alice:example.org", "Alice")); + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ + mockRoomMember("@alice:example.org", "Alice"), + ]); + jest.spyOn(room, "getMember").mockReturnValue(mockRoomMember("@alice:example.org", "Alice")); const crypto = client.getCrypto()!; - crypto["getUserVerificationStatus"] = jest.fn(async () => { - return Promise.resolve(new UserVerificationStatus(false, false, false, false)); - }); - render(, { - wrapper: ({ ...rest }) => , - }); + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, false), + ); + renderComponent(client, room); await sleep(10); // give it some time to finish initialising expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(); @@ -194,17 +198,13 @@ describe("UserIdentityWarning", () => { // joins/leaves, we should update the warning appropriately. it("updates the display when a member joins/leaves", async () => { // Nobody in the room yet - room.getEncryptionTargetMembers = jest.fn(async () => { - return []; - }); - room.getMember = jest.fn(() => mockRoomMember("@alice:example.org", "Alice")); + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([]); + jest.spyOn(room, "getMember").mockReturnValue(mockRoomMember("@alice:example.org", "Alice")); const crypto = client.getCrypto()!; - crypto["getUserVerificationStatus"] = jest.fn(async () => { - return Promise.resolve(new UserVerificationStatus(false, false, false, true)); - }); - render(, { - wrapper: ({ ...rest }) => , - }); + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); + renderComponent(client, room); await sleep(10); // give it some time to finish initialising // Alice joins. Her identity needs approval, so we should show a warning. @@ -253,20 +253,19 @@ describe("UserIdentityWarning", () => { }); // When we have multiple users whose identity needs approval, one user's - // identity no longer reeds approval (e.g. their identity was approved), + // identity no longer needs approval (e.g. their identity was approved), // then we show the next one. it("displays the next user when the current user's identity is approved", async () => { - room.getEncryptionTargetMembers = jest.fn(async () => { - return [mockRoomMember("@alice:example.org", "Alice"), mockRoomMember("@bob:example.org")]; - }); + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ + mockRoomMember("@alice:example.org", "Alice"), + mockRoomMember("@bob:example.org"), + ]); const crypto = client.getCrypto()!; - crypto["getUserVerificationStatus"] = jest.fn(async () => { - return Promise.resolve(new UserVerificationStatus(false, false, false, true)); - }); + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); - render(, { - wrapper: ({ ...rest }) => , - }); + renderComponent(client, room); // We should warn about Alice's identity first. await waitFor(() => expect( @@ -295,22 +294,22 @@ describe("UserIdentityWarning", () => { // First case: check that if the update says that the user identity // needs approval, but the fetch says it doesn't, we show the warning. it("update says identity needs approval", async () => { - room.getEncryptionTargetMembers = jest.fn(async () => { - return [mockRoomMember("@alice:example.org", "Alice")]; - }); - room.getMember = jest.fn(() => mockRoomMember("@alice:example.org", "Alice")); + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ + mockRoomMember("@alice:example.org", "Alice"), + ]); + jest.spyOn(room, "getMember").mockReturnValue(mockRoomMember("@alice:example.org", "Alice")); const crypto = client.getCrypto()!; - crypto["getUserVerificationStatus"] = jest.fn(async () => { - client.emit( - CryptoEvent.UserTrustStatusChanged, - "@alice:example.org", - new UserVerificationStatus(false, false, false, true), - ); + jest.spyOn(crypto, "getUserVerificationStatus").mockImplementation(async () => { + act(() => { + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, true), + ); + }); return Promise.resolve(new UserVerificationStatus(false, false, false, false)); }); - render(, { - wrapper: ({ ...rest }) => , - }); + renderComponent(client, room); await sleep(10); // give it some time to finish initialising await waitFor(() => expect( @@ -323,22 +322,22 @@ describe("UserIdentityWarning", () => { // doesn't needs approval, but the fetch says it does, we don't show the // warning. it("update says identity doesn't need approval", async () => { - room.getEncryptionTargetMembers = jest.fn(async () => { - return [mockRoomMember("@alice:example.org", "Alice")]; - }); - room.getMember = jest.fn(() => mockRoomMember("@alice:example.org", "Alice")); + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ + mockRoomMember("@alice:example.org", "Alice"), + ]); + jest.spyOn(room, "getMember").mockReturnValue(mockRoomMember("@alice:example.org", "Alice")); const crypto = client.getCrypto()!; - crypto["getUserVerificationStatus"] = jest.fn(async () => { - client.emit( - CryptoEvent.UserTrustStatusChanged, - "@alice:example.org", - new UserVerificationStatus(false, false, false, false), - ); + jest.spyOn(crypto, "getUserVerificationStatus").mockImplementation(async () => { + act(() => { + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, false), + ); + }); return Promise.resolve(new UserVerificationStatus(false, false, false, true)); }); - render(, { - wrapper: ({ ...rest }) => , - }); + renderComponent(client, room); await sleep(10); // give it some time to finish initialising await waitFor(() => expect(() => From d2b244f9725fb1f9489931afbdb32da7effbd286 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 6 Nov 2024 20:02:35 -0500 Subject: [PATCH 06/10] use Crypto API to determine if room is encrypted --- .../views/rooms/UserIdentityWarning.tsx | 17 +++++++++-------- .../views/rooms/UserIdentityWarning-test.tsx | 5 +++-- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index 6e674d7ef82..17807339ccd 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -125,6 +125,12 @@ export const UserIdentityWarning: React.FC = ({ room } if (!crypto || initialisedRef.current) { return; } + // If encryption is not enabled in the room, we don't need to do + // anything. If encryption gets enabled later, we will retry, via + // onRoomStateEvent. + if (!(await crypto.isEncryptionEnabledInRoom(room.roomId))) { + return; + } initialisedRef.current = true; const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; @@ -151,14 +157,9 @@ export const UserIdentityWarning: React.FC = ({ room } selectCurrentPrompt(); }, [crypto, room, initialisedRef, gotVerificationStatusUpdateRef, membersNeedingApprovalRef, selectCurrentPrompt]); - // If the room has encryption enabled, we load the room members right away. - // If not, we wait until encryption is enabled before loading the room - // members, since we don't need to display anything in unencrypted rooms. - if (crypto && room.hasEncryptionStateEvent()) { - loadMembers().catch((e) => { - logger.error("Error initialising UserIdentityWarning:", e); - }); - } + loadMembers().catch((e) => { + logger.error("Error initialising UserIdentityWarning:", e); + }); // When a user's verification status changes, we check if they need to be // added/removed from the set of members needing approval. diff --git a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx index c02449a36d8..52d082740e7 100644 --- a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -32,7 +32,6 @@ function mockRoom(): Room { getMember: jest.fn((userId) => {}), roomId: ROOM_ID, shouldEncryptForInvitedMembers: jest.fn(() => true), - hasEncryptionStateEvent: jest.fn(() => true), } as unknown as Room; return room; @@ -80,6 +79,7 @@ describe("UserIdentityWarning", () => { beforeEach(async () => { client = stubClient(); room = mockRoom(); + jest.spyOn(client.getCrypto()!, "isEncryptionEnabledInRoom").mockResolvedValue(true); }); afterEach(() => { @@ -117,8 +117,8 @@ describe("UserIdentityWarning", () => { mockRoomMember("@alice:example.org", "Alice"), ]); // Start the room off unencrypted. We shouldn't display anything. - jest.spyOn(room, "hasEncryptionStateEvent").mockReturnValue(false); const crypto = client.getCrypto()!; + jest.spyOn(crypto, "isEncryptionEnabledInRoom").mockResolvedValue(false); jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( new UserVerificationStatus(false, false, false, true), ); @@ -129,6 +129,7 @@ describe("UserIdentityWarning", () => { // Encryption gets enabled in the room. We should now warn that Alice's // identity changed. + jest.spyOn(crypto, "isEncryptionEnabledInRoom").mockResolvedValue(true); client.emit( RoomStateEvent.Events, new MatrixEvent({ From 807862798479d013fdf9608ed6f2f2f24250c105 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 11 Nov 2024 15:14:01 -0500 Subject: [PATCH 07/10] apply changes from review --- .../views/rooms/UserIdentityWarning.tsx | 120 ++++++------ .../views/rooms/UserIdentityWarning-test.tsx | 173 ++++++++++++++---- 2 files changed, 194 insertions(+), 99 deletions(-) diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index 17807339ccd..2c494ba4208 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -47,6 +47,7 @@ export const UserIdentityWarning: React.FC = ({ room } const crypto = cli.getCrypto(); // The current room member that we are prompting the user to approve. + // `undefined` means we are not currently showing a prompt. const [currentPrompt, setCurrentPrompt] = useState(undefined); // Whether or not we've already initialised the component by loading the @@ -57,7 +58,7 @@ export const UserIdentityWarning: React.FC = ({ room } // Whether we got a verification status update while we were fetching a // user's verification status. // - // We set the entry for a user to `false` when we fetch a user's + // We set the entry for a user to `false` when we start fetching a user's // verification status, and remove the user's entry when we are done // fetching. When we receive a verification status update, if the entry for // the user is `false`, we set it to `true`. After we have finished @@ -71,40 +72,72 @@ export const UserIdentityWarning: React.FC = ({ room } // Select a new user to display a warning for. This is called after the // current prompted user no longer needs their identity approved. - const selectCurrentPrompt = useCallback((): void => { + const selectCurrentPrompt = useCallback((): RoomMember | undefined => { const membersNeedingApproval = membersNeedingApprovalRef.current; if (membersNeedingApproval.size === 0) { - setCurrentPrompt(undefined); - return; + return undefined; } - // We return the user with the smallest user ID. + // We pick the user with the smallest user ID. const keys = Array.from(membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b)); - setCurrentPrompt(membersNeedingApproval.get(keys[0]!)); - }, [membersNeedingApprovalRef]); + const selection = membersNeedingApproval.get(keys[0]!); + return selection; + }, []); // Add a user to the membersNeedingApproval map, and update the current - // prompt if necessary. + // prompt if necessary. The user will only be added if they are actually a + // member of the room. If they are not a member, this function will do + // nothing. const addMemberNeedingApproval = useCallback( - (userId: string): void => { + (userId: string, member?: RoomMember, updatePrompt: boolean = true): void => { if (userId === cli.getUserId()) { // We always skip our own user, because we can't pin our own identity. return; } - const member = room.getMember(userId); + member = member ?? room.getMember(userId) ?? undefined; if (member) { membersNeedingApprovalRef.current.set(userId, member); - if (!currentPrompt) { - // If we're not currently displaying a prompt, then we should - // display a prompt for this user. - selectCurrentPrompt(); + if (updatePrompt) { + setCurrentPrompt((currentPrompt) => { + // We have to do this in a callback to + // `setCurrentPrompt` because this function could have + // been called after an `await`, and the `currentPrompt` + // that this function would have may be outdated. + if (!currentPrompt) { + return selectCurrentPrompt(); + } + }); } } }, - [cli, room, membersNeedingApprovalRef, currentPrompt, selectCurrentPrompt], + [cli, room, selectCurrentPrompt], ); - // Add a user from the membersNeedingApproval map, and update the current + // Check if the user's identity needs approval, and if so, add them to the + // membersNeedingApproval map and update the prompt if needed. They will + // only be added if they are a member of the room. + const addMemberIfNeedsApproval = useCallback( + async (userId: string, member?: RoomMember, updatePrompt: boolean = true): Promise => { + const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; + const membersNeedingApproval = membersNeedingApprovalRef.current; + + if (gotVerificationStatusUpdate.has(userId)) { + // We're already checking their verification status, so we don't + // need to do anything here. + return; + } + gotVerificationStatusUpdate.set(userId, false); + if (await userNeedsApproval(crypto!, userId)) { + if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { + addMemberNeedingApproval(userId, member, updatePrompt); + } + } + gotVerificationStatusUpdate.delete(userId); + }, + [crypto, addMemberNeedingApproval], + ); + + // Remove a user from the membersNeedingApproval map, and update the current // prompt if necessary. const removeMemberNeedingApproval = useCallback( (userId: string): void => { @@ -113,10 +146,10 @@ export const UserIdentityWarning: React.FC = ({ room } // If we removed the currently displayed user, we need to pick a new one // to display. if (currentPrompt?.userId === userId) { - selectCurrentPrompt(); + setCurrentPrompt(selectCurrentPrompt()); } }, - [membersNeedingApprovalRef, currentPrompt, selectCurrentPrompt], + [currentPrompt, selectCurrentPrompt], ); // Initialise the component. Get the room members, check which ones need @@ -133,29 +166,14 @@ export const UserIdentityWarning: React.FC = ({ room } } initialisedRef.current = true; - const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; - const membersNeedingApproval = membersNeedingApprovalRef.current; - const members = await room.getEncryptionTargetMembers(); for (const member of members) { - const userId = member.userId; - if (gotVerificationStatusUpdate.has(userId)) { - // We're already checking their verification status, so we don't - // need to do anything here. - continue; - } - gotVerificationStatusUpdate.set(userId, false); - if (await userNeedsApproval(crypto, userId)) { - if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { - membersNeedingApproval.set(userId, member); - } - } - gotVerificationStatusUpdate.delete(userId); + await addMemberIfNeedsApproval(member.userId, member, false); } - selectCurrentPrompt(); - }, [crypto, room, initialisedRef, gotVerificationStatusUpdateRef, membersNeedingApprovalRef, selectCurrentPrompt]); + setCurrentPrompt(selectCurrentPrompt()); + }, [crypto, room, addMemberIfNeedsApproval, selectCurrentPrompt]); loadMembers().catch((e) => { logger.error("Error initialising UserIdentityWarning:", e); @@ -181,7 +199,7 @@ export const UserIdentityWarning: React.FC = ({ room } removeMemberNeedingApproval(userId); } }, - [initialisedRef, gotVerificationStatusUpdateRef, addMemberNeedingApproval, removeMemberNeedingApproval], + [addMemberNeedingApproval, removeMemberNeedingApproval], ); useTypedEventEmitter(cli, CryptoEvent.UserTrustStatusChanged, onUserVerificationStatusChanged); @@ -194,9 +212,6 @@ export const UserIdentityWarning: React.FC = ({ room } return; } - const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; - const membersNeedingApproval = membersNeedingApprovalRef.current; - const eventType = event.getType(); if (eventType === EventType.RoomEncryption && event.getStateKey() === "") { // Room is now encrypted, so we can initialise the component. @@ -217,37 +232,18 @@ export const UserIdentityWarning: React.FC = ({ room } if ( event.getContent().membership === KnownMembership.Join || - (event.getContent().membership === KnownMembership.Join && room.shouldEncryptForInvitedMembers()) + (event.getContent().membership === KnownMembership.Invite && room.shouldEncryptForInvitedMembers()) ) { // Someone's membership changed and we will now encrypt to them. If // their identity needs approval, show a warning. - if (gotVerificationStatusUpdate.has(userId)) { - // We're already checking their verification status, so we don't - // need to do anything here. - return; - } - gotVerificationStatusUpdate.set(userId, false); - if (await userNeedsApproval(crypto, userId)) { - if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { - addMemberNeedingApproval(userId); - } - } - gotVerificationStatusUpdate.delete(userId); + await addMemberIfNeedsApproval(userId); } else { // Someone's membership changed and we no longer encrypt to them. // If we're showing a warning about them, we don't need to any more. removeMemberNeedingApproval(userId); } }, - [ - crypto, - room, - gotVerificationStatusUpdateRef, - membersNeedingApprovalRef, - addMemberNeedingApproval, - removeMemberNeedingApproval, - loadMembers, - ], + [crypto, room, addMemberIfNeedsApproval, removeMemberNeedingApproval, loadMembers], ); useTypedEventEmitter(cli, RoomStateEvent.Events, onRoomStateEvent); diff --git a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx index 52d082740e7..03119d3336a 100644 --- a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -197,41 +197,95 @@ describe("UserIdentityWarning", () => { // We only display warnings about users in the room. When someone // joins/leaves, we should update the warning appropriately. - it("updates the display when a member joins/leaves", async () => { - // Nobody in the room yet - jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([]); - jest.spyOn(room, "getMember").mockReturnValue(mockRoomMember("@alice:example.org", "Alice")); - const crypto = client.getCrypto()!; - jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( - new UserVerificationStatus(false, false, false, true), - ); - renderComponent(client, room); - await sleep(10); // give it some time to finish initialising + describe("updates the display when a member joins/leaves", () => { + it("when invited users can see encrypted messages", async () => { + // Nobody in the room yet + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([]); + jest.spyOn(room, "getMember").mockImplementation((userId) => mockRoomMember(userId)); + jest.spyOn(room, "shouldEncryptForInvitedMembers").mockReturnValue(true); + const crypto = client.getCrypto()!; + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); + renderComponent(client, room); + await sleep(10); // give it some time to finish initialising - // Alice joins. Her identity needs approval, so we should show a warning. - client.emit( - RoomStateEvent.Events, - new MatrixEvent({ - event_id: "$event_id", - type: EventType.RoomMember, - state_key: "@alice:example.org", - content: { - membership: "join", - }, - room_id: ROOM_ID, - sender: "@alice:example.org", - }), - dummyRoomState(), - null, - ); - await waitFor(() => - expect( - getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), - ).toBeInTheDocument(), - ); + // Alice joins. Her identity needs approval, so we should show a warning. + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@alice:example.org", + content: { + membership: "join", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + await waitFor(() => + expect(getWarningByText("@alice:example.org's identity appears to have changed.")).toBeInTheDocument(), + ); - // Alice leaves, so we no longer show her warning. - act(() => { + // Bob is invited. His identity needs approval, so we should show a + // warning for him after Alice's warning is resolved by her leaving. + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@bob:example.org", + content: { + membership: "invite", + }, + room_id: ROOM_ID, + sender: "@carol:example.org", + }), + dummyRoomState(), + null, + ); + + // Alice leaves, so we no longer show her warning, but we will show + // a warning for Bob. + act(() => { + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@alice:example.org", + content: { + membership: "leave", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + }); + await waitFor(() => + expect(() => getWarningByText("@alice:example.org's identity appears to have changed.")).toThrow(), + ); + expect(getWarningByText("@bob:example.org's identity appears to have changed.")).toBeInTheDocument(); + }); + + it("when invited users cannot see encrypted messages", async () => { + // Nobody in the room yet + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([]); + jest.spyOn(room, "getMember").mockImplementation((userId) => mockRoomMember(userId)); + jest.spyOn(room, "shouldEncryptForInvitedMembers").mockReturnValue(false); + const crypto = client.getCrypto()!; + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); + renderComponent(client, room); + await sleep(10); // give it some time to finish initialising + + // Alice joins. Her identity needs approval, so we should show a warning. client.emit( RoomStateEvent.Events, new MatrixEvent({ @@ -239,7 +293,7 @@ describe("UserIdentityWarning", () => { type: EventType.RoomMember, state_key: "@alice:example.org", content: { - membership: "leave", + membership: "join", }, room_id: ROOM_ID, sender: "@alice:example.org", @@ -247,10 +301,55 @@ describe("UserIdentityWarning", () => { dummyRoomState(), null, ); + await waitFor(() => + expect(getWarningByText("@alice:example.org's identity appears to have changed.")).toBeInTheDocument(), + ); + + // Bob is invited. His identity needs approval, but we don't encrypt + // to him, so we won't show a warning. (When Alice leaves, the + // display won't be updated to show a warningfor Bob.) + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@bob:example.org", + content: { + membership: "invite", + }, + room_id: ROOM_ID, + sender: "@carol:example.org", + }), + dummyRoomState(), + null, + ); + + // Alice leaves, so we no longer show her warning, and we don't show + // a warning for Bob. + act(() => { + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@alice:example.org", + content: { + membership: "leave", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + }); + await waitFor(() => + expect(() => getWarningByText("@alice:example.org's identity appears to have changed.")).toThrow(), + ); + await waitFor(() => + expect(() => getWarningByText("@bob:example.org's identity appears to have changed.")).toThrow(), + ); }); - await waitFor(() => - expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(), - ); }); // When we have multiple users whose identity needs approval, one user's From 5615b9824af01f966c1e22e8fb8f17e06056fb85 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 12 Nov 2024 15:06:46 -0500 Subject: [PATCH 08/10] change initialisation status to a tri-state rather than a boolean --- .../views/rooms/UserIdentityWarning.tsx | 43 ++++++++++++++----- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index 2c494ba4208..b0b254e37aa 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -36,6 +36,16 @@ async function userNeedsApproval(crypto: CryptoApi, userId: string): Promise = ({ room } // Whether or not we've already initialised the component by loading the // room membership. - const initialisedRef = useRef(false); + const initialisedRef = useRef(InitialisationStatus.Uninitialised); // Which room members need their identity approved. const membersNeedingApprovalRef = useRef>(new Map()); // Whether we got a verification status update while we were fetching a @@ -89,7 +99,7 @@ export const UserIdentityWarning: React.FC = ({ room } // member of the room. If they are not a member, this function will do // nothing. const addMemberNeedingApproval = useCallback( - (userId: string, member?: RoomMember, updatePrompt: boolean = true): void => { + (userId: string, member?: RoomMember): void => { if (userId === cli.getUserId()) { // We always skip our own user, because we can't pin our own identity. return; @@ -97,14 +107,24 @@ export const UserIdentityWarning: React.FC = ({ room } member = member ?? room.getMember(userId) ?? undefined; if (member) { membersNeedingApprovalRef.current.set(userId, member); - if (updatePrompt) { + // We only select the prompt if we are done initialising, + // because we will select the prompt after we're done + // initialising, and we want to start by displaying a warning + // for the user with the smallest ID. + if (initialisedRef.current === InitialisationStatus.Completed) { setCurrentPrompt((currentPrompt) => { + // If we aren't currently displaying a warning, we pick + // a new user to show a warning for. If we are already + // displaying a warning, don't change the display. + // // We have to do this in a callback to // `setCurrentPrompt` because this function could have // been called after an `await`, and the `currentPrompt` // that this function would have may be outdated. if (!currentPrompt) { return selectCurrentPrompt(); + } else { + return currentPrompt; } }); } @@ -117,7 +137,7 @@ export const UserIdentityWarning: React.FC = ({ room } // membersNeedingApproval map and update the prompt if needed. They will // only be added if they are a member of the room. const addMemberIfNeedsApproval = useCallback( - async (userId: string, member?: RoomMember, updatePrompt: boolean = true): Promise => { + async (userId: string, member?: RoomMember): Promise => { const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; const membersNeedingApproval = membersNeedingApprovalRef.current; @@ -129,7 +149,7 @@ export const UserIdentityWarning: React.FC = ({ room } gotVerificationStatusUpdate.set(userId, false); if (await userNeedsApproval(crypto!, userId)) { if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { - addMemberNeedingApproval(userId, member, updatePrompt); + addMemberNeedingApproval(userId, member); } } gotVerificationStatusUpdate.delete(userId); @@ -155,7 +175,7 @@ export const UserIdentityWarning: React.FC = ({ room } // Initialise the component. Get the room members, check which ones need // their identity approved, and pick one to display. const loadMembers = useCallback(async (): Promise => { - if (!crypto || initialisedRef.current) { + if (!crypto || initialisedRef.current != InitialisationStatus.Uninitialised) { return; } // If encryption is not enabled in the room, we don't need to do @@ -164,15 +184,16 @@ export const UserIdentityWarning: React.FC = ({ room } if (!(await crypto.isEncryptionEnabledInRoom(room.roomId))) { return; } - initialisedRef.current = true; + initialisedRef.current = InitialisationStatus.Initialising; const members = await room.getEncryptionTargetMembers(); for (const member of members) { - await addMemberIfNeedsApproval(member.userId, member, false); + await addMemberIfNeedsApproval(member.userId, member); } setCurrentPrompt(selectCurrentPrompt()); + initialisedRef.current = InitialisationStatus.Completed; }, [crypto, room, addMemberIfNeedsApproval, selectCurrentPrompt]); loadMembers().catch((e) => { @@ -185,7 +206,9 @@ export const UserIdentityWarning: React.FC = ({ room } (userId: string, verificationStatus: UserVerificationStatus): void => { const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; - if (!initialisedRef.current) { + // If we haven't started initialising, that means that we're in a + // room where we don't need to display any warnings. + if (initialisedRef.current === InitialisationStatus.Uninitialised) { return; } @@ -222,7 +245,7 @@ export const UserIdentityWarning: React.FC = ({ room } return; } - if (!initialisedRef.current) { + if (initialisedRef.current === InitialisationStatus.Uninitialised) { return; } From 0ef342b9f082efdd3c802c4dde3ee21f6e4fa055 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 14 Nov 2024 22:48:38 -0500 Subject: [PATCH 09/10] fix more race conditions, and apply changes from review --- .../views/rooms/UserIdentityWarning.tsx | 142 ++++++++++-------- .../views/rooms/UserIdentityWarning-test.tsx | 83 ++++++++++ 2 files changed, 162 insertions(+), 63 deletions(-) diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index b0b254e37aa..a8ca8a48e55 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -80,18 +80,28 @@ export const UserIdentityWarning: React.FC = ({ room } // with the newer value, so it will fix itself in the end. const gotVerificationStatusUpdateRef = useRef>(new Map()); - // Select a new user to display a warning for. This is called after the - // current prompted user no longer needs their identity approved. - const selectCurrentPrompt = useCallback((): RoomMember | undefined => { + // Update the current prompt. Select a new user if needed, or hide the + // warning if we don't have anyone to warn about. + const updateCurrentPrompt = useCallback((): undefined => { const membersNeedingApproval = membersNeedingApprovalRef.current; - if (membersNeedingApproval.size === 0) { - return undefined; - } + // We have to do this in a callback to `setCurrentPrompt` + // because this function could have been called after an + // `await`, and the `currentPrompt` that this function would + // have may be outdated. + setCurrentPrompt((currentPrompt) => { + // If we're already displaying a warning, and that user still needs + // approval, continue showing that user. + if (currentPrompt && membersNeedingApproval.has(currentPrompt.userId)) return currentPrompt; + + if (membersNeedingApproval.size === 0) { + return undefined; + } - // We pick the user with the smallest user ID. - const keys = Array.from(membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b)); - const selection = membersNeedingApproval.get(keys[0]!); - return selection; + // We pick the user with the smallest user ID. + const keys = Array.from(membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b)); + const selection = membersNeedingApproval.get(keys[0]!); + return selection; + }); }, []); // Add a user to the membersNeedingApproval map, and update the current @@ -105,54 +115,52 @@ export const UserIdentityWarning: React.FC = ({ room } return; } member = member ?? room.getMember(userId) ?? undefined; - if (member) { - membersNeedingApprovalRef.current.set(userId, member); - // We only select the prompt if we are done initialising, - // because we will select the prompt after we're done - // initialising, and we want to start by displaying a warning - // for the user with the smallest ID. - if (initialisedRef.current === InitialisationStatus.Completed) { - setCurrentPrompt((currentPrompt) => { - // If we aren't currently displaying a warning, we pick - // a new user to show a warning for. If we are already - // displaying a warning, don't change the display. - // - // We have to do this in a callback to - // `setCurrentPrompt` because this function could have - // been called after an `await`, and the `currentPrompt` - // that this function would have may be outdated. - if (!currentPrompt) { - return selectCurrentPrompt(); - } else { - return currentPrompt; - } - }); - } + if (!member) return; + + membersNeedingApprovalRef.current.set(userId, member); + // We only select the prompt if we are done initialising, + // because we will select the prompt after we're done + // initialising, and we want to start by displaying a warning + // for the user with the smallest ID. + if (initialisedRef.current === InitialisationStatus.Completed) { + updateCurrentPrompt(); } }, - [cli, room, selectCurrentPrompt], + [cli, room, updateCurrentPrompt], ); // Check if the user's identity needs approval, and if so, add them to the // membersNeedingApproval map and update the prompt if needed. They will // only be added if they are a member of the room. - const addMemberIfNeedsApproval = useCallback( - async (userId: string, member?: RoomMember): Promise => { + const addMembersWhoNeedApproval = useCallback( + async (members: RoomMember[]): Promise => { const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; - const membersNeedingApproval = membersNeedingApprovalRef.current; - if (gotVerificationStatusUpdate.has(userId)) { - // We're already checking their verification status, so we don't - // need to do anything here. - return; - } - gotVerificationStatusUpdate.set(userId, false); - if (await userNeedsApproval(crypto!, userId)) { - if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { - addMemberNeedingApproval(userId, member); + const promises: Promise[] = []; + + for (const member of members) { + const userId = member.userId; + if (gotVerificationStatusUpdate.has(userId)) { + // We're already checking their verification status, so we don't + // need to do anything here. + continue; } + gotVerificationStatusUpdate.set(userId, false); + promises.push( + userNeedsApproval(crypto!, userId).then((needsApproval) => { + if (needsApproval) { + // Only actually update the list if we haven't received a + // `UserTrustStatusChanged` for this user in the meantime. + if (gotVerificationStatusUpdate.get(userId) === false) { + addMemberNeedingApproval(userId, member); + } + } + gotVerificationStatusUpdate.delete(userId); + }), + ); } - gotVerificationStatusUpdate.delete(userId); + + await Promise.all(promises); }, [crypto, addMemberNeedingApproval], ); @@ -162,20 +170,15 @@ export const UserIdentityWarning: React.FC = ({ room } const removeMemberNeedingApproval = useCallback( (userId: string): void => { membersNeedingApprovalRef.current.delete(userId); - - // If we removed the currently displayed user, we need to pick a new one - // to display. - if (currentPrompt?.userId === userId) { - setCurrentPrompt(selectCurrentPrompt()); - } + updateCurrentPrompt(); }, - [currentPrompt, selectCurrentPrompt], + [updateCurrentPrompt], ); // Initialise the component. Get the room members, check which ones need // their identity approved, and pick one to display. const loadMembers = useCallback(async (): Promise => { - if (!crypto || initialisedRef.current != InitialisationStatus.Uninitialised) { + if (!crypto || initialisedRef.current !== InitialisationStatus.Uninitialised) { return; } // If encryption is not enabled in the room, we don't need to do @@ -187,14 +190,11 @@ export const UserIdentityWarning: React.FC = ({ room } initialisedRef.current = InitialisationStatus.Initialising; const members = await room.getEncryptionTargetMembers(); + await addMembersWhoNeedApproval(members); - for (const member of members) { - await addMemberIfNeedsApproval(member.userId, member); - } - - setCurrentPrompt(selectCurrentPrompt()); + updateCurrentPrompt(); initialisedRef.current = InitialisationStatus.Completed; - }, [crypto, room, addMemberIfNeedsApproval, selectCurrentPrompt]); + }, [crypto, room, addMembersWhoNeedApproval, updateCurrentPrompt]); loadMembers().catch((e) => { logger.error("Error initialising UserIdentityWarning:", e); @@ -213,6 +213,9 @@ export const UserIdentityWarning: React.FC = ({ room } } if (gotVerificationStatusUpdate.has(userId)) { + // There is an ongoing call to `addMembersWhoNeedApproval`. Flag + // to it that we've got a more up-to-date result so it should + // discard its result. gotVerificationStatusUpdate.set(userId, true); } @@ -259,14 +262,27 @@ export const UserIdentityWarning: React.FC = ({ room } ) { // Someone's membership changed and we will now encrypt to them. If // their identity needs approval, show a warning. - await addMemberIfNeedsApproval(userId); + const member = room.getMember(userId); + if (member) { + await addMembersWhoNeedApproval([member]).catch((e) => { + logger.error("Error adding member in UserIdentityWarning:", e); + }); + } } else { // Someone's membership changed and we no longer encrypt to them. // If we're showing a warning about them, we don't need to any more. removeMemberNeedingApproval(userId); + const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; + if (gotVerificationStatusUpdate.has(userId)) { + // There is an ongoing call to `addMembersWhoNeedApproval`. + // Indicate that we've received a verification status update + // to prevent it from trying to add the user as needing + // verification. + gotVerificationStatusUpdate.set(userId, true); + } } }, - [crypto, room, addMemberIfNeedsApproval, removeMemberNeedingApproval, loadMembers], + [crypto, room, addMembersWhoNeedApproval, removeMemberNeedingApproval, loadMembers], ); useTypedEventEmitter(cli, RoomStateEvent.Events, onRoomStateEvent); diff --git a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx index 03119d3336a..1ad50988a30 100644 --- a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -350,6 +350,89 @@ describe("UserIdentityWarning", () => { expect(() => getWarningByText("@bob:example.org's identity appears to have changed.")).toThrow(), ); }); + + it("when member leaves immediately after component is loaded", async () => { + jest.spyOn(room, "getEncryptionTargetMembers").mockImplementation(async () => { + setTimeout(() => { + // Alice immediately leaves after we get the room + // membership, so we shouldn't show the warning any more + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@alice:example.org", + content: { + membership: "leave", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + }); + return [mockRoomMember("@alice:example.org")]; + }); + jest.spyOn(room, "getMember").mockImplementation((userId) => mockRoomMember(userId)); + jest.spyOn(room, "shouldEncryptForInvitedMembers").mockReturnValue(false); + const crypto = client.getCrypto()!; + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); + renderComponent(client, room); + + await sleep(10); + expect(() => getWarningByText("@alice:example.org's identity appears to have changed.")).toThrow(); + }); + + it("when member leaves immediately after joining", async () => { + // Nobody in the room yet + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([]); + jest.spyOn(room, "getMember").mockImplementation((userId) => mockRoomMember(userId)); + jest.spyOn(room, "shouldEncryptForInvitedMembers").mockReturnValue(false); + const crypto = client.getCrypto()!; + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); + renderComponent(client, room); + await sleep(10); // give it some time to finish initialising + + // Alice joins. Her identity needs approval, so we should show a warning. + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@alice:example.org", + content: { + membership: "join", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + // ... but she immediately leaves, so we shouldn't show the warning any more + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: "@alice:example.org", + content: { + membership: "leave", + }, + room_id: ROOM_ID, + sender: "@alice:example.org", + }), + dummyRoomState(), + null, + ); + await sleep(10); // give it some time to finish + expect(() => getWarningByText("@alice:example.org's identity appears to have changed.")).toThrow(); + }); }); // When we have multiple users whose identity needs approval, one user's From ccbfcea320379e9c75d53b3fb8d44c933be07dde Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 18 Nov 2024 17:27:16 -0500 Subject: [PATCH 10/10] apply changes from review and switch to using counter for detecting races --- .../views/rooms/UserIdentityWarning.tsx | 77 ++++++++----------- 1 file changed, 34 insertions(+), 43 deletions(-) diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index a8ca8a48e55..56ab13c21bf 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -65,20 +65,29 @@ export const UserIdentityWarning: React.FC = ({ room } const initialisedRef = useRef(InitialisationStatus.Uninitialised); // Which room members need their identity approved. const membersNeedingApprovalRef = useRef>(new Map()); - // Whether we got a verification status update while we were fetching a - // user's verification status. + // For each user, we assign a sequence number to each verification status + // that we get, or fetch. // - // We set the entry for a user to `false` when we start fetching a user's - // verification status, and remove the user's entry when we are done - // fetching. When we receive a verification status update, if the entry for - // the user is `false`, we set it to `true`. After we have finished - // fetching the user's verification status, if the entry for the user is - // `true`, rather than `false`, we know that we got an update, and so we - // discard the value that we fetched. We always use the value from the - // update and consider it as the most up-to-date version. If the fetched - // value is more up-to-date, then we should be getting a new update soon - // with the newer value, so it will fix itself in the end. - const gotVerificationStatusUpdateRef = useRef>(new Map()); + // Since fetching a verification status is asynchronous, we could get an + // update in the middle of fetching the verification status, which could + // mean that the status that we fetched is out of date. So if the current + // sequence number is not the same as the sequence number when we started + // the fetch, then we drop our fetched result, under the assumption that the + // update that we received is the most up-to-date version. If it is in fact + // not the most up-to-date version, then we should be receiving a new update + // soon with the newer value, so it will fix itself in the end. + // + // We also assign a sequence number when the user leaves the room, in order + // to prevent prompting about a user who leaves while we are fetching their + // verification status. + const verificationStatusSequencesRef = useRef>(new Map()); + const incrementVerificationStatusSequence = (userId: string): number => { + const verificationStatusSequences = verificationStatusSequencesRef.current; + const value = verificationStatusSequences.get(userId); + const newValue = value === undefined ? 1 : value + 1; + verificationStatusSequences.set(userId, newValue); + return newValue; + }; // Update the current prompt. Select a new user if needed, or hide the // warning if we don't have anyone to warn about. @@ -129,33 +138,27 @@ export const UserIdentityWarning: React.FC = ({ room } [cli, room, updateCurrentPrompt], ); - // Check if the user's identity needs approval, and if so, add them to the - // membersNeedingApproval map and update the prompt if needed. They will - // only be added if they are a member of the room. + // For each user in the list check if their identity needs approval, and if + // so, add them to the membersNeedingApproval map and update the prompt if + // needed. They will only be added if they are a member of the room. const addMembersWhoNeedApproval = useCallback( async (members: RoomMember[]): Promise => { - const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; + const verificationStatusSequences = verificationStatusSequencesRef.current; const promises: Promise[] = []; for (const member of members) { const userId = member.userId; - if (gotVerificationStatusUpdate.has(userId)) { - // We're already checking their verification status, so we don't - // need to do anything here. - continue; - } - gotVerificationStatusUpdate.set(userId, false); + const sequenceNum = incrementVerificationStatusSequence(userId); promises.push( userNeedsApproval(crypto!, userId).then((needsApproval) => { if (needsApproval) { - // Only actually update the list if we haven't received a - // `UserTrustStatusChanged` for this user in the meantime. - if (gotVerificationStatusUpdate.get(userId) === false) { + // Only actually update the list if we have the most + // recent value. + if (verificationStatusSequences.get(userId) === sequenceNum) { addMemberNeedingApproval(userId, member); } } - gotVerificationStatusUpdate.delete(userId); }), ); } @@ -204,20 +207,13 @@ export const UserIdentityWarning: React.FC = ({ room } // added/removed from the set of members needing approval. const onUserVerificationStatusChanged = useCallback( (userId: string, verificationStatus: UserVerificationStatus): void => { - const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; - // If we haven't started initialising, that means that we're in a // room where we don't need to display any warnings. if (initialisedRef.current === InitialisationStatus.Uninitialised) { return; } - if (gotVerificationStatusUpdate.has(userId)) { - // There is an ongoing call to `addMembersWhoNeedApproval`. Flag - // to it that we've got a more up-to-date result so it should - // discard its result. - gotVerificationStatusUpdate.set(userId, true); - } + incrementVerificationStatusSequence(userId); if (verificationStatus.needsUserApproval) { addMemberNeedingApproval(userId); @@ -248,6 +244,8 @@ export const UserIdentityWarning: React.FC = ({ room } return; } + // We're processing an m.room.member event + if (initialisedRef.current === InitialisationStatus.Uninitialised) { return; } @@ -272,14 +270,7 @@ export const UserIdentityWarning: React.FC = ({ room } // Someone's membership changed and we no longer encrypt to them. // If we're showing a warning about them, we don't need to any more. removeMemberNeedingApproval(userId); - const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current; - if (gotVerificationStatusUpdate.has(userId)) { - // There is an ongoing call to `addMembersWhoNeedApproval`. - // Indicate that we've received a verification status update - // to prevent it from trying to add the user as needing - // verification. - gotVerificationStatusUpdate.set(userId, true); - } + incrementVerificationStatusSequence(userId); } }, [crypto, room, addMembersWhoNeedApproval, removeMemberNeedingApproval, loadMembers],