From ede3058ae78f1987dc57954f92b564ff7b75702a Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 22 Jan 2025 13:17:14 +0100 Subject: [PATCH 1/5] feat(crypto): Support verification violation composer banner --- res/css/views/rooms/_UserIdentityWarning.pcss | 6 + .../views/rooms/UserIdentityWarning.tsx | 164 +++++++++++++----- src/i18n/strings/en_EN.json | 5 +- .../views/rooms/UserIdentityWarning-test.tsx | 80 +++++++++ 4 files changed, 209 insertions(+), 46 deletions(-) diff --git a/res/css/views/rooms/_UserIdentityWarning.pcss b/res/css/views/rooms/_UserIdentityWarning.pcss index e5d14eb4726..cf87e47a24a 100644 --- a/res/css/views/rooms/_UserIdentityWarning.pcss +++ b/res/css/views/rooms/_UserIdentityWarning.pcss @@ -20,8 +20,14 @@ Please see LICENSE files in the repository root for full details. margin-left: var(--cpd-space-6x); flex-grow: 1; } + .mx_UserIdentityWarning_main.critical { + color: var(--cpd-color-text-critical-primary); + } } } +.mx_UserIdentityWarning.critical { + background: linear-gradient(180deg, var(--cpd-color-red-100) 0%, var(--cpd-color-theme-bg) 100%); +} .mx_MessageComposer.mx_MessageComposer--compact > .mx_UserIdentityWarning { margin-left: calc(-25px + var(--RoomView_MessageList-padding)); diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index 06586c26381..9c82b6efa99 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -28,12 +28,23 @@ interface UserIdentityWarningProps { key: string; } +type ViolationType = "PinViolation" | "VerificationViolation"; + /** - * Does the given user's identity need to be approved? + * Does the given user's identity has a status violation. */ -async function userNeedsApproval(crypto: CryptoApi, userId: string): Promise { +async function userNeedsApproval(crypto: CryptoApi, userId: string): Promise { const verificationStatus = await crypto.getUserVerificationStatus(userId); - return verificationStatus.needsUserApproval; + return mapToViolationType(verificationStatus); +} + +function mapToViolationType(verificationStatus: UserVerificationStatus): ViolationType | null { + if (verificationStatus.wasCrossSigningVerified() && !verificationStatus.isCrossSigningVerified()) { + return "VerificationViolation"; + } else if (verificationStatus.needsUserApproval) { + return "PinViolation"; + } + return null; } /** @@ -46,6 +57,11 @@ enum InitialisationStatus { Completed, } +type ViolationPrompt = { + member: RoomMember; + type: ViolationType; +}; + /** * Displays a banner warning when there is an issue with a user's identity. * @@ -58,13 +74,13 @@ export const UserIdentityWarning: React.FC = ({ room } // 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); + const [currentPrompt, setCurrentPrompt] = useState(undefined); // Whether or not we've already initialised the component by loading the // room membership. const initialisedRef = useRef(InitialisationStatus.Uninitialised); // Which room members need their identity approved. - const membersNeedingApprovalRef = useRef>(new Map()); + const membersNeedingApprovalRef = useRef>(new Map()); // For each user, we assign a sequence number to each verification status // that we get, or fetch. // @@ -100,7 +116,8 @@ export const UserIdentityWarning: React.FC = ({ room } 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 (currentPrompt && membersNeedingApproval.get(currentPrompt.member.userId)?.type === currentPrompt.type) + return currentPrompt; if (membersNeedingApproval.size === 0) { if (currentPrompt) { @@ -113,7 +130,10 @@ export const UserIdentityWarning: React.FC = ({ room } // 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]!); - logger.debug(`UserIdentityWarning: now warning about user ${selection?.userId}`); + logger.debug(`UserIdentityWarning: selection is ${JSON.stringify(selection)}`); + logger.debug( + `UserIdentityWarning: now warning about user ${selection?.member.userId} for a ${selection?.type} violation`, + ); return selection; }); }, []); @@ -123,15 +143,24 @@ 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): void => { + (userId: string, violation?: ViolationType, member?: RoomMember): void => { + logger.debug(`UserIdentityWarning: add member ${userId} for violation ${violation}`); + if (userId === cli.getUserId()) { // We always skip our own user, because we can't pin our own identity. return; } + if (violation === undefined) return; + + // Member might be already resolved depending on the context. When called after a + // CryptoEvent.UserTrustStatusChanged event emitted it will not yet be resolved. member = member ?? room.getMember(userId) ?? undefined; - if (!member) return; + if (!member) { + logger.debug(`UserIdentityWarning: user ${userId} not found in room members, ignoring violation`); + return; + } - membersNeedingApprovalRef.current.set(userId, member); + membersNeedingApprovalRef.current.set(userId, { member, type: violation }); // 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 @@ -159,12 +188,12 @@ export const UserIdentityWarning: React.FC = ({ room } const userId = member.userId; const sequenceNum = incrementVerificationStatusSequence(userId); promises.push( - userNeedsApproval(crypto!, userId).then((needsApproval) => { - if (needsApproval) { + userNeedsApproval(crypto!, userId).then((type) => { + if (type != null) { // Only actually update the list if we have the most // recent value. if (verificationStatusSequences.get(userId) === sequenceNum) { - addMemberNeedingApproval(userId, member); + addMemberNeedingApproval(userId, type, member); } } }), @@ -231,8 +260,10 @@ export const UserIdentityWarning: React.FC = ({ room } incrementVerificationStatusSequence(userId); - if (verificationStatus.needsUserApproval) { - addMemberNeedingApproval(userId); + const violation = mapToViolationType(verificationStatus); + + if (violation) { + addMemberNeedingApproval(userId, violation); } else { removeMemberNeedingApproval(userId); } @@ -296,39 +327,82 @@ export const UserIdentityWarning: React.FC = ({ room } if (!crypto || !currentPrompt) return null; const confirmIdentity = async (): Promise => { - await crypto.pinCurrentUserIdentity(currentPrompt.userId); + if (currentPrompt.type === "VerificationViolation") { + await crypto.withdrawVerificationRequirement(currentPrompt.member.userId); + } else if (currentPrompt.type === "PinViolation") { + await crypto.pinCurrentUserIdentity(currentPrompt.member.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, - }, - )} - - + if (currentPrompt.type === "VerificationViolation") { + return ( +
+ +
+ + + {currentPrompt.member.rawDisplayName === currentPrompt.member.userId + ? _t( + "encryption|verified_identity_changed_no_displayname", + { userId: currentPrompt.member.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + ) + : _t( + "encryption|verified_identity_changed", + { + displayName: currentPrompt.member.rawDisplayName, + userId: currentPrompt.member.userId, + }, + { + a: substituteATag, + b: substituteBTag, + }, + )} + + +
-
- ); + ); + } else { + return ( +
+ +
+ + + {currentPrompt.member.rawDisplayName === currentPrompt.member.userId + ? _t( + "encryption|pinned_identity_changed_no_displayname", + { userId: currentPrompt.member.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + ) + : _t( + "encryption|pinned_identity_changed", + { + displayName: currentPrompt.member.rawDisplayName, + userId: currentPrompt.member.userId, + }, + { + a: substituteATag, + b: substituteBTag, + }, + )} + + +
+
+ ); + } }; function substituteATag(sub: string): React.ReactNode { diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 3efbf4cfc52..7e284693627 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1058,8 +1058,11 @@ "waiting_other_user": "Waiting for %(displayName)s to verify…" }, "verification_requested_toast_title": "Verification requested", + "verified_identity_changed": "%(displayName)s's (%(userId)s) verified identity has changed. Learn more", + "verified_identity_changed_no_displayname": "%(userId)s's verified identity has changed. Learn more", "verify_toast_description": "Other users may not trust it", - "verify_toast_title": "Verify this session" + "verify_toast_title": "Verify this session", + "withdraw_verification_action": "Withdraw verification" }, "error": { "admin_contact": "Please contact your service administrator to continue using this service.", diff --git a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx index eef25269125..7c761b0ba4b 100644 --- a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -93,6 +93,9 @@ describe("UserIdentityWarning", () => { jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ mockRoomMember("@alice:example.org", "Alice"), ]); + // 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), @@ -109,6 +112,49 @@ describe("UserIdentityWarning", () => { await waitFor(() => expect(crypto.pinCurrentUserIdentity).toHaveBeenCalledWith("@alice:example.org")); }); + // This tests the basic functionality of the component. If we have a room + // member whose identity is in verification violation, we should display a warning. When + // the "Withdraw verification" button gets pressed, it should call `withdrawVerification`. + it("displays a warning when a user's identity is in verification violation", async () => { + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ + mockRoomMember("@alice:example.org", "Alice"), + ]); + const crypto = client.getCrypto()!; + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, true, false, true), + ); + crypto.withdrawVerificationRequirement = jest.fn(); + renderComponent(client, room); + + await waitFor(() => + expect(getWarningByText("Alice's (@alice:example.org) verified identity has changed.")).toBeInTheDocument(), + ); + + expect( + screen.getByRole("button", { + name: "Withdraw verification", + }), + ).toBeInTheDocument(); + await userEvent.click(screen.getByRole("button")!); + await waitFor(() => expect(crypto.withdrawVerificationRequirement).toHaveBeenCalledWith("@alice:example.org")); + }); + + it("Should not display a warning if the user was verified and is still verified", async () => { + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ + mockRoomMember("@alice:example.org", "Alice"), + ]); + const crypto = client.getCrypto()!; + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(true, true, 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(); + expect(() => getWarningByText("Alice's (@alice:example.org) verified identity has changed.")).toThrow(); + }); + // 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. @@ -472,6 +518,40 @@ describe("UserIdentityWarning", () => { ); }); + it("displays the next user when the verification requirement is withdrawn", async () => { + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ + mockRoomMember("@alice:example.org", "Alice"), + mockRoomMember("@bob:example.org"), + ]); + const crypto = client.getCrypto()!; + jest.spyOn(crypto, "getUserVerificationStatus").mockImplementation(async (userId) => { + if (userId == "@alice:example.org") { + return new UserVerificationStatus(false, true, false, true); + } else { + return new UserVerificationStatus(false, false, false, true); + } + }); + + renderComponent(client, room); + // We should warn about Alice's identity first. + await waitFor(() => + expect(getWarningByText("Alice's (@alice:example.org) verified identity has changed.")).toBeInTheDocument(), + ); + + // Simulate Alice's new identity having been approved, so now we warn + // about Bob's identity. + 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(), + ); + }); + // 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. From 3aa4a6efe20ffbec36508c847766ebcc84c21759 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 28 Jan 2025 11:14:03 +0100 Subject: [PATCH 2/5] refactor UserIdentityWarning by using now a ViewModel fixup: logger import fixup: test lint type problems fix test having an unexpected verification violation fixup sonarcubes warnings --- .../rooms/UserIdentityWarningViewModel.tsx | 164 +++++++ .../views/rooms/UserIdentityWarning.tsx | 450 ++++-------------- .../components/structures/RoomView-test.tsx | 2 +- .../views/rooms/UserIdentityWarning-test.tsx | 369 +++++++------- 4 files changed, 446 insertions(+), 539 deletions(-) create mode 100644 src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx diff --git a/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx b/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx new file mode 100644 index 00000000000..a08a63f3fbd --- /dev/null +++ b/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx @@ -0,0 +1,164 @@ +/* +Copyright 2025 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import { useCallback, useEffect, useMemo, useState } from "react"; +import { EventType, MatrixEvent, Room, RoomStateEvent, RoomMember } from "matrix-js-sdk/src/matrix"; +import { CryptoEvent, CryptoApi } from "matrix-js-sdk/src/crypto-api"; +import { throttle } from "lodash"; +import { logger } from "matrix-js-sdk/src/logger"; + +import { useMatrixClientContext } from "../../../contexts/MatrixClientContext.tsx"; +import { useTypedEventEmitter } from "../../../hooks/useEventEmitter.ts"; +import { ButtonEvent } from "../../views/elements/AccessibleButton.tsx"; + +export type ViolationType = "PinViolation" | "VerificationViolation"; + +export type ViolationPrompt = { + member: RoomMember; + type: ViolationType; +}; + +export interface UserIdentityWarningState { + currentPrompt?: ViolationPrompt; + onButtonClick: (ev: ButtonEvent) => void; +} + +async function mapToViolations(cryptoApi: CryptoApi, members: RoomMember[]): Promise { + const violationList = new Array(); + for (const member of members) { + const verificationStatus = await cryptoApi.getUserVerificationStatus(member.userId); + if (verificationStatus.wasCrossSigningVerified() && !verificationStatus.isCrossSigningVerified()) { + violationList.push({ member, type: "VerificationViolation" }); + } else if (verificationStatus.needsUserApproval) { + violationList.push({ member, type: "PinViolation" }); + } + } + return violationList; +} + +export function useUserIdentityWarningViewModel(room: Room, key: string): UserIdentityWarningState { + const cli = useMatrixClientContext(); + const crypto = cli.getCrypto(); + + const [members, setMembers] = useState([]); + const [currentPrompt, setCurrentPrompt] = useState(undefined); + + const loadViolations = useMemo( + () => + throttle(async (): Promise => { + const isEncrypted = crypto && (await crypto.isEncryptionEnabledInRoom(room.roomId)); + if (!isEncrypted) { + setMembers([]); + setCurrentPrompt(undefined); + return; + } + + const targetMembers = await room.getEncryptionTargetMembers(); + setMembers(targetMembers); + const violations = await mapToViolations(crypto, targetMembers); + + let candidatePrompt: ViolationPrompt | undefined; + if (violations.length > 0) { + // sort by user ID to ensure consistent ordering + const sortedViolations = violations.sort((a, b) => a.member.userId.localeCompare(b.member.userId)); + candidatePrompt = sortedViolations[0]; + } else { + candidatePrompt = undefined; + } + + // is the current prompt still valid? + setCurrentPrompt((existingPrompt): ViolationPrompt | undefined => { + if (existingPrompt && violations.includes(existingPrompt)) { + return existingPrompt; + } else if (candidatePrompt) { + return candidatePrompt; + } else { + return undefined; + } + }); + }), + [crypto, room], + ); + + // We need to listen for changes to the members list + const onRoomStateEvent = useCallback( + async (event: MatrixEvent): Promise => { + if (!crypto || event.getRoomId() !== room.roomId) { + return; + } + let shouldRefresh = false; + + const eventType = event.getType(); + + if (eventType === EventType.RoomEncryption && event.getStateKey() === "") { + // Room is now encrypted, so we can initialise the component. + shouldRefresh = true; + } else if (eventType == EventType.RoomMember) { + // We're processing an m.room.member event + // Something has changed in membership, someone joined or someone left or + // someone changed their display name. Anyhow let's refresh. + const userId = event.getStateKey(); + shouldRefresh = !!userId; + } + + if (shouldRefresh) { + loadViolations().catch((e) => { + logger.error("Error refreshing UserIdentityWarningViewModel:", e); + }); + } + }, + [crypto, room, loadViolations], + ); + useTypedEventEmitter(cli, RoomStateEvent.Events, onRoomStateEvent); + + // We need to listen for changes to the verification status of the members to refresh violations + const onUserVerificationStatusChanged = useCallback( + (userId: string): void => { + if (members.find((m) => m.userId == userId)) { + // This member is tracked, we need to refresh. + // refresh all for now? + // As a later optimisation we could store the current violations and only update the relevant one. + loadViolations().catch((e) => { + logger.error("Error refreshing UserIdentityWarning:", e); + }); + } + }, + [loadViolations, members], + ); + useTypedEventEmitter(cli, CryptoEvent.UserTrustStatusChanged, onUserVerificationStatusChanged); + + useEffect(() => { + loadViolations().catch((e) => { + logger.error("Error initialising UserIdentityWarning:", e); + }); + }, [loadViolations]); + + let onButtonClick: (ev: ButtonEvent) => void = () => {}; + if (currentPrompt) { + onButtonClick = (ev: ButtonEvent): void => { + // XXX do we want some posthog tracking? + if (!crypto) { + return; + } + ev.preventDefault(); + if (currentPrompt.type === "VerificationViolation") { + crypto.withdrawVerificationRequirement(currentPrompt.member.userId).catch((e) => { + logger.error("Error withdrawing verification requirement:", e); + }); + } else if (currentPrompt.type === "PinViolation") { + crypto.pinCurrentUserIdentity(currentPrompt.member.userId).catch((e) => { + logger.error("Error withdrawing verification requirement:", e); + }); + } + }; + } + + return { + currentPrompt, + onButtonClick, + }; +} diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index 9c82b6efa99..674e0d46692 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -5,16 +5,18 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com Please see LICENSE files in the repository root for full details. */ -import React, { useCallback, useEffect, useRef, useState } from "react"; -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 React from "react"; +import { Room, RoomMember } from "matrix-js-sdk/src/matrix"; import { Button, Separator } from "@vector-im/compound-web"; +import classNames from "classnames"; import { _t } from "../../../languageHandler"; import MemberAvatar from "../avatars/MemberAvatar"; -import { useMatrixClientContext } from "../../../contexts/MatrixClientContext"; -import { useTypedEventEmitter } from "../../../hooks/useEventEmitter"; +import { + useUserIdentityWarningViewModel, + ViolationPrompt, +} from "../../viewmodels/rooms/UserIdentityWarningViewModel.tsx"; +import { ButtonEvent } from "../elements/AccessibleButton.tsx"; interface UserIdentityWarningProps { /** @@ -28,40 +30,6 @@ interface UserIdentityWarningProps { key: string; } -type ViolationType = "PinViolation" | "VerificationViolation"; - -/** - * Does the given user's identity has a status violation. - */ -async function userNeedsApproval(crypto: CryptoApi, userId: string): Promise { - const verificationStatus = await crypto.getUserVerificationStatus(userId); - return mapToViolationType(verificationStatus); -} - -function mapToViolationType(verificationStatus: UserVerificationStatus): ViolationType | null { - if (verificationStatus.wasCrossSigningVerified() && !verificationStatus.isCrossSigningVerified()) { - return "VerificationViolation"; - } else if (verificationStatus.needsUserApproval) { - return "PinViolation"; - } - return null; -} - -/** - * Whether the component is uninitialised, is in the process of initialising, or - * has completed initialising. - */ -enum InitialisationStatus { - Uninitialised, - Initialising, - Completed, -} - -type ViolationPrompt = { - member: RoomMember; - type: ViolationType; -}; - /** * Displays a banner warning when there is an issue with a user's identity. * @@ -69,341 +37,93 @@ type ViolationPrompt = { * button to acknowledge the change. */ export const UserIdentityWarning: React.FC = ({ room }) => { - const cli = useMatrixClientContext(); - 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 - // room membership. - const initialisedRef = useRef(InitialisationStatus.Uninitialised); - // Which room members need their identity approved. - const membersNeedingApprovalRef = useRef>(new Map()); - // For each user, we assign a sequence number to each verification status - // that we get, or fetch. - // - // 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; - }; + const { currentPrompt, onButtonClick } = useUserIdentityWarningViewModel(room, room.roomId); - // 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; - // 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.get(currentPrompt.member.userId)?.type === currentPrompt.type) - return currentPrompt; + if (!currentPrompt) return null; - if (membersNeedingApproval.size === 0) { - if (currentPrompt) { - // If we were previously showing a warning, log that we've stopped doing so. - logger.debug("UserIdentityWarning: no users left that need approval"); - } - return undefined; - } + const [title, action] = getTitleAndAction(currentPrompt); - // 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]!); - logger.debug(`UserIdentityWarning: selection is ${JSON.stringify(selection)}`); - logger.debug( - `UserIdentityWarning: now warning about user ${selection?.member.userId} for a ${selection?.type} violation`, - ); - return selection; - }); - }, []); - - // Add a user to the membersNeedingApproval map, and update the current - // 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, violation?: ViolationType, member?: RoomMember): void => { - logger.debug(`UserIdentityWarning: add member ${userId} for violation ${violation}`); - - if (userId === cli.getUserId()) { - // We always skip our own user, because we can't pin our own identity. - return; - } - if (violation === undefined) return; - - // Member might be already resolved depending on the context. When called after a - // CryptoEvent.UserTrustStatusChanged event emitted it will not yet be resolved. - member = member ?? room.getMember(userId) ?? undefined; - if (!member) { - logger.debug(`UserIdentityWarning: user ${userId} not found in room members, ignoring violation`); - return; - } - - membersNeedingApprovalRef.current.set(userId, { member, type: violation }); - // 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) { - logger.debug( - `UserIdentityWarning: user ${userId} now needs approval; approval-pending list now [${Array.from(membersNeedingApprovalRef.current.keys())}]`, - ); - updateCurrentPrompt(); - } - }, - [cli, room, updateCurrentPrompt], - ); - - // 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. - const addMembersWhoNeedApproval = useCallback( - async (members: RoomMember[]): Promise => { - const verificationStatusSequences = verificationStatusSequencesRef.current; - - const promises: Promise[] = []; - - for (const member of members) { - const userId = member.userId; - const sequenceNum = incrementVerificationStatusSequence(userId); - promises.push( - userNeedsApproval(crypto!, userId).then((type) => { - if (type != null) { - // Only actually update the list if we have the most - // recent value. - if (verificationStatusSequences.get(userId) === sequenceNum) { - addMemberNeedingApproval(userId, type, member); - } - } - }), - ); - } - - await Promise.all(promises); - }, - [crypto, addMemberNeedingApproval], + return warningBanner( + currentPrompt.type === "VerificationViolation", + memberAvatar(currentPrompt.member), + title, + action, + onButtonClick, ); +}; - // Remove a user from the membersNeedingApproval map, and update the current - // prompt if necessary. - const removeMemberNeedingApproval = useCallback( - (userId: string): void => { - membersNeedingApprovalRef.current.delete(userId); - logger.debug( - `UserIdentityWarning: user ${userId} no longer needs approval; approval-pending list now [${Array.from(membersNeedingApprovalRef.current.keys())}]`, +function getTitleAndAction(prompt: ViolationPrompt): [title: React.ReactNode, action: string] { + let title: React.ReactNode; + let action: string; + if (prompt.type === "VerificationViolation") { + if (prompt.member.rawDisplayName === prompt.member.userId) { + title = _t( + "encryption|verified_identity_changed_no_displayname", + { userId: prompt.member.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + ); + } else { + title = _t( + "encryption|verified_identity_changed", + { displayName: prompt.member.rawDisplayName, userId: prompt.member.userId }, + { + a: substituteATag, + b: substituteBTag, + }, ); - updateCurrentPrompt(); - }, - [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) { - 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 = InitialisationStatus.Initialising; - - const members = await room.getEncryptionTargetMembers(); - await addMembersWhoNeedApproval(members); - - logger.info( - `Initialised UserIdentityWarning component for room ${room.roomId} with approval-pending list [${Array.from(membersNeedingApprovalRef.current.keys())}]`, - ); - updateCurrentPrompt(); - initialisedRef.current = InitialisationStatus.Completed; - }, [crypto, room, addMembersWhoNeedApproval, updateCurrentPrompt]); - - useEffect(() => { - loadMembers().catch((e) => { - logger.error("Error initialising UserIdentityWarning:", e); - }); - }, [loadMembers]); - - // 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 => { - // 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; - } - - incrementVerificationStatusSequence(userId); - - const violation = mapToViolationType(verificationStatus); - - if (violation) { - addMemberNeedingApproval(userId, violation); - } else { - removeMemberNeedingApproval(userId); - } - }, - [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 eventType = event.getType(); - if (eventType === EventType.RoomEncryption && event.getStateKey() === "") { - // Room is now encrypted, so we can initialise the component. - return loadMembers().catch((e) => { - logger.error("Error initialising UserIdentityWarning:", e); - }); - } else if (eventType !== EventType.RoomMember) { - return; - } - - // We're processing an m.room.member event - - if (initialisedRef.current === InitialisationStatus.Uninitialised) { - return; - } - - const userId = event.getStateKey(); - - if (!userId) return; - - if ( - event.getContent().membership === KnownMembership.Join || - (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. - 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); - incrementVerificationStatusSequence(userId); - } - }, - [crypto, room, addMembersWhoNeedApproval, removeMemberNeedingApproval, loadMembers], - ); - useTypedEventEmitter(cli, RoomStateEvent.Events, onRoomStateEvent); - - if (!crypto || !currentPrompt) return null; - - const confirmIdentity = async (): Promise => { - if (currentPrompt.type === "VerificationViolation") { - await crypto.withdrawVerificationRequirement(currentPrompt.member.userId); - } else if (currentPrompt.type === "PinViolation") { - await crypto.pinCurrentUserIdentity(currentPrompt.member.userId); + action = _t("encryption|withdraw_verification_action"); + } else { + if (prompt.member.rawDisplayName === prompt.member.userId) { + title = _t( + "encryption|pinned_identity_changed_no_displayname", + { userId: prompt.member.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + ); + } else { + title = _t( + "encryption|pinned_identity_changed", + { displayName: prompt.member.rawDisplayName, userId: prompt.member.userId }, + { + a: substituteATag, + b: substituteBTag, + }, + ); } - }; + action = _t("action|ok"); + } + return [title, action]; +} - if (currentPrompt.type === "VerificationViolation") { - return ( -
- -
- - - {currentPrompt.member.rawDisplayName === currentPrompt.member.userId - ? _t( - "encryption|verified_identity_changed_no_displayname", - { userId: currentPrompt.member.userId }, - { - a: substituteATag, - b: substituteBTag, - }, - ) - : _t( - "encryption|verified_identity_changed", - { - displayName: currentPrompt.member.rawDisplayName, - userId: currentPrompt.member.userId, - }, - { - a: substituteATag, - b: substituteBTag, - }, - )} - - -
-
- ); - } else { - return ( -
- -
- - - {currentPrompt.member.rawDisplayName === currentPrompt.member.userId - ? _t( - "encryption|pinned_identity_changed_no_displayname", - { userId: currentPrompt.member.userId }, - { - a: substituteATag, - b: substituteBTag, - }, - ) - : _t( - "encryption|pinned_identity_changed", - { - displayName: currentPrompt.member.rawDisplayName, - userId: currentPrompt.member.userId, - }, - { - a: substituteATag, - b: substituteBTag, - }, - )} - - -
+function warningBanner( + isCritical: boolean, + avatar: React.ReactNode, + title: React.ReactNode, + action: string, + onButtonClick: (ev: ButtonEvent) => void, +): React.ReactNode { + return ( +
+ +
+ {avatar} + {title} +
- ); - } -}; +
+ ); +} +function memberAvatar(member: RoomMember): React.ReactNode { + return ; +} function substituteATag(sub: string): React.ReactNode { return ( diff --git a/test/unit-tests/components/structures/RoomView-test.tsx b/test/unit-tests/components/structures/RoomView-test.tsx index eabfe0c85ce..67bcacf4079 100644 --- a/test/unit-tests/components/structures/RoomView-test.tsx +++ b/test/unit-tests/components/structures/RoomView-test.tsx @@ -424,7 +424,7 @@ describe("RoomView", () => { jest.spyOn(cli, "getCrypto").mockReturnValue(crypto); jest.spyOn(cli.getCrypto()!, "isEncryptionEnabledInRoom").mockResolvedValue(true); jest.spyOn(cli.getCrypto()!, "getUserVerificationStatus").mockResolvedValue( - new UserVerificationStatus(false, true, false), + new UserVerificationStatus(false, false, false), ); localRoom.encrypted = true; localRoom.currentState.setStateEvents([ diff --git a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx index 7c761b0ba4b..c210e9343e0 100644 --- a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -6,7 +6,7 @@ Please see LICENSE files in the repository root for full details. */ import React from "react"; -import { sleep } from "matrix-js-sdk/src/utils"; +import { sleep, defer } from "matrix-js-sdk/src/utils"; import { EventType, MatrixClient, @@ -37,6 +37,50 @@ function mockRoom(): Room { return room; } +function mockMembershipForRoom(room: Room, users: string[] | [string, "joined" | "invited"][]): void { + const encryptToInvited = room.shouldEncryptForInvitedMembers(); + const members = users + .filter((user) => { + if (Array.isArray(user)) { + return encryptToInvited || user[1] === "joined"; + } else { + return true; + } + }) + .map((id) => { + if (Array.isArray(id)) { + return mockRoomMember(id[0]); + } else { + return mockRoomMember(id); + } + }); + + jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue(members); + + jest.spyOn(room, "getMember").mockImplementation((userId) => { + return members.find((member) => member.userId === userId) ?? null; + }); +} + +function emitMembershipChange(client: MatrixClient, userId: string, membership: "join" | "leave" | "invite"): void { + const sender = membership === "invite" ? "@carol:example.org" : userId; + client.emit( + RoomStateEvent.Events, + new MatrixEvent({ + event_id: "$event_id", + type: EventType.RoomMember, + state_key: userId, + content: { + membership: membership, + }, + room_id: ROOM_ID, + sender: sender, + }), + dummyRoomState(), + null, + ); +} + function mockRoomMember(userId: string, name?: string): RoomMember { return { userId, @@ -93,14 +137,11 @@ describe("UserIdentityWarning", () => { jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ mockRoomMember("@alice:example.org", "Alice"), ]); - // 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), ); - crypto.pinCurrentUserIdentity = jest.fn(); + crypto.pinCurrentUserIdentity = jest.fn().mockResolvedValue(undefined); renderComponent(client, room); await waitFor(() => @@ -123,7 +164,7 @@ describe("UserIdentityWarning", () => { jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( new UserVerificationStatus(false, true, false, true), ); - crypto.withdrawVerificationRequirement = jest.fn(); + crypto.withdrawVerificationRequirement = jest.fn().mockResolvedValue(undefined); renderComponent(client, room); await waitFor(() => @@ -170,6 +211,7 @@ describe("UserIdentityWarning", () => { ); 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(); @@ -198,6 +240,57 @@ describe("UserIdentityWarning", () => { ); }); + describe("Warning are displayed in consistent order", () => { + it("Ensure lexicographic order for prompt", async () => { + // members are not returned lexicographic order + mockMembershipForRoom(room, ["@b:example.org", "@a:example.org"]); + + const crypto = client.getCrypto()!; + + // All identities needs approval + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); + + crypto.pinCurrentUserIdentity = jest.fn(); + renderComponent(client, room); + + await waitFor(() => + expect(getWarningByText("@a:example.org's identity appears to have changed.")).toBeInTheDocument(), + ); + }); + + it("Ensure existing prompt stays even if a new violation with lower lexicographic order detected", async () => { + mockMembershipForRoom(room, ["@b:example.org"]); + + const crypto = client.getCrypto()!; + + // All identities needs approval + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + new UserVerificationStatus(false, false, false, true), + ); + + crypto.pinCurrentUserIdentity = jest.fn(); + renderComponent(client, room); + + await waitFor(() => + expect(getWarningByText("@b:example.org's identity appears to have changed.")).toBeInTheDocument(), + ); + + // Simulate a new member joined with lower lexico order and also in violation + mockMembershipForRoom(room, ["@a:example.org", "@b:example.org"]); + + act(() => { + emitMembershipChange(client, "@a:example.org", "join"); + }); + + // We should still display the warning for @b:example.org + await waitFor(() => + expect(getWarningByText("@b:example.org's 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 () => { @@ -209,18 +302,20 @@ describe("UserIdentityWarning", () => { jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( new UserVerificationStatus(false, false, false, false), ); - renderComponent(client, room); - await sleep(10); // give it some time to finish initialising + await act(async () => { + renderComponent(client, room); + await sleep(50); + }); + expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(); // The user changes their identity, so we should show the warning. act(() => { - client.emit( - CryptoEvent.UserTrustStatusChanged, - "@alice:example.org", - new UserVerificationStatus(false, false, false, true), - ); + const newStatus = new UserVerificationStatus(false, false, false, true); + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue(newStatus); + client.emit(CryptoEvent.UserTrustStatusChanged, "@alice:example.org", newStatus); }); + await waitFor(() => expect( getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), @@ -230,11 +325,9 @@ describe("UserIdentityWarning", () => { // Simulate the user's new identity having been approved, so we no // longer show the warning. act(() => { - client.emit( - CryptoEvent.UserTrustStatusChanged, - "@alice:example.org", - new UserVerificationStatus(false, false, false, false), - ); + const newStatus = new UserVerificationStatus(false, false, false, false); + jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue(newStatus); + client.emit(CryptoEvent.UserTrustStatusChanged, "@alice:example.org", newStatus); }); await waitFor(() => expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow(), @@ -246,8 +339,11 @@ describe("UserIdentityWarning", () => { 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)); + mockMembershipForRoom(room, []); + // jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([]); + // jest.spyOn(room, "getMember").mockImplementation((userId) => { + // return null; + // }); jest.spyOn(room, "shouldEncryptForInvitedMembers").mockReturnValue(true); const crypto = client.getCrypto()!; jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( @@ -257,62 +353,29 @@ describe("UserIdentityWarning", () => { 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, - ); + act(() => { + mockMembershipForRoom(room, ["@alice:example.org"]); + emitMembershipChange(client, "@alice:example.org", "join"); + }); + await waitFor(() => expect(getWarningByText("@alice:example.org's identity appears to have changed.")).toBeInTheDocument(), ); // 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, - ); + act(() => { + mockMembershipForRoom(room, ["@alice:example.org", "@bob:example.org"]); + emitMembershipChange(client, "@bob:example.org", "invite"); + }); // 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, - ); + mockMembershipForRoom(room, ["@bob:example.org"]); + emitMembershipChange(client, "@alice:example.org", "leave"); }); + await waitFor(() => expect(() => getWarningByText("@alice:example.org's identity appears to have changed.")).toThrow(), ); @@ -323,8 +386,9 @@ describe("UserIdentityWarning", () => { 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)); + mockMembershipForRoom(room, []); + // 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( @@ -334,21 +398,10 @@ describe("UserIdentityWarning", () => { 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, - ); + act(() => { + mockMembershipForRoom(room, ["@alice:example.org"]); + emitMembershipChange(client, "@alice:example.org", "join"); + }); await waitFor(() => expect(getWarningByText("@alice:example.org's identity appears to have changed.")).toBeInTheDocument(), ); @@ -356,40 +409,19 @@ describe("UserIdentityWarning", () => { // 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, - ); + act(() => { + mockMembershipForRoom(room, [ + ["@alice:example.org", "joined"], + ["@bob:example.org", "invited"], + ]); + emitMembershipChange(client, "@bob:example.org", "invite"); + }); // 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, - ); + mockMembershipForRoom(room, [["@bob:example.org", "invited"]]); + emitMembershipChange(client, "@alice:example.org", "leave"); }); await waitFor(() => expect(() => getWarningByText("@alice:example.org's identity appears to have changed.")).toThrow(), @@ -400,37 +432,26 @@ describe("UserIdentityWarning", () => { }); it("when member leaves immediately after component is loaded", async () => { + let hasLeft = false; jest.spyOn(room, "getEncryptionTargetMembers").mockImplementation(async () => { + if (hasLeft) return []; 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, - ); + emitMembershipChange(client, "@alice:example.org", "leave"); + hasLeft = true; }); 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); + await act(async () => { + renderComponent(client, room); + await sleep(10); + }); expect(() => getWarningByText("@alice:example.org's identity appears to have changed.")).toThrow(); }); @@ -507,11 +528,15 @@ describe("UserIdentityWarning", () => { // Simulate Alice's new identity having been approved, so now we warn // about Bob's identity. act(() => { - client.emit( - CryptoEvent.UserTrustStatusChanged, - "@alice:example.org", - new UserVerificationStatus(false, false, false, false), - ); + const newStatus = new UserVerificationStatus(false, false, false, false); + jest.spyOn(crypto, "getUserVerificationStatus").mockImplementation(async (userId) => { + if (userId == "@alice:example.org") { + return newStatus; + } else { + return new UserVerificationStatus(false, false, false, true); + } + }); + client.emit(CryptoEvent.UserTrustStatusChanged, "@alice:example.org", newStatus); }); await waitFor(() => expect(getWarningByText("@bob:example.org's identity appears to have changed.")).toBeInTheDocument(), @@ -541,6 +566,13 @@ describe("UserIdentityWarning", () => { // Simulate Alice's new identity having been approved, so now we warn // about Bob's identity. act(() => { + jest.spyOn(crypto, "getUserVerificationStatus").mockImplementation(async (userId) => { + if (userId == "@alice:example.org") { + return new UserVerificationStatus(false, false, false, false); + } else { + return new UserVerificationStatus(false, false, false, true); + } + }); client.emit( CryptoEvent.UserTrustStatusChanged, "@alice:example.org", @@ -564,50 +596,41 @@ describe("UserIdentityWarning", () => { ]); jest.spyOn(room, "getMember").mockReturnValue(mockRoomMember("@alice:example.org", "Alice")); const crypto = client.getCrypto()!; + + const firstStatusPromise = defer(); + let callNumber = 0; 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)); + await firstStatusPromise.promise; + callNumber++; + if (callNumber == 1) { + await sleep(40); + return new UserVerificationStatus(false, false, false, false); + } else { + return new UserVerificationStatus(false, false, false, true); + } }); + renderComponent(client, room); 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 () => { - jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([ - mockRoomMember("@alice:example.org", "Alice"), - ]); - jest.spyOn(room, "getMember").mockReturnValue(mockRoomMember("@alice:example.org", "Alice")); - const crypto = client.getCrypto()!; - 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)); + // jest.spyOn(crypto, "getUserVerificationStatus").mockImplementation(async () => { + act(() => { + // jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( + // new UserVerificationStatus(false, false, false, true), + // ); + client.emit( + CryptoEvent.UserTrustStatusChanged, + "@alice:example.org", + new UserVerificationStatus(false, false, false, true), + ); + firstStatusPromise.resolve(undefined); }); - renderComponent(client, room); - await sleep(10); // give it some time to finish initialising + // return Promise.resolve(new UserVerificationStatus(false, false, false, false)); + // }); await waitFor(() => - expect(() => + expect( getWarningByText("Alice's (@alice:example.org) identity appears to have changed."), - ).toThrow(), + ).toBeInTheDocument(), ); }); }); From e1c395024aca0dbd44e260ca4f2db3b70d881705 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 29 Jan 2025 17:05:37 +0100 Subject: [PATCH 3/5] review: comments on types and inline some const --- .../rooms/UserIdentityWarningViewModel.tsx | 110 +++++++++++------- 1 file changed, 67 insertions(+), 43 deletions(-) diff --git a/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx b/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx index a08a63f3fbd..c52fc4ba608 100644 --- a/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx +++ b/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx @@ -6,8 +6,8 @@ Please see LICENSE files in the repository root for full details. */ import { useCallback, useEffect, useMemo, useState } from "react"; -import { EventType, MatrixEvent, Room, RoomStateEvent, RoomMember } from "matrix-js-sdk/src/matrix"; -import { CryptoEvent, CryptoApi } from "matrix-js-sdk/src/crypto-api"; +import { EventType, MatrixEvent, Room, RoomMember, RoomStateEvent } from "matrix-js-sdk/src/matrix"; +import { CryptoApi, CryptoEvent } from "matrix-js-sdk/src/crypto-api"; import { throttle } from "lodash"; import { logger } from "matrix-js-sdk/src/logger"; @@ -17,16 +17,34 @@ import { ButtonEvent } from "../../views/elements/AccessibleButton.tsx"; export type ViolationType = "PinViolation" | "VerificationViolation"; +/** + * Represents a prompt to the user about a violation in the room. + * The type of violation and the member it relates to are included. + * If the type is "VerificationViolation", the warning is critical and should be reported with more urgency. + */ export type ViolationPrompt = { member: RoomMember; type: ViolationType; }; +/** + * The state of the UserIdentityWarningViewModel. + * This includes the current prompt to show to the user and a callback to handle button clicks. + * If currentPrompt is undefined, there are no violations to show. + */ export interface UserIdentityWarningState { currentPrompt?: ViolationPrompt; onButtonClick: (ev: ButtonEvent) => void; } +/** + * Maps a list of room members to a list of violations. + * Checks for all members in the room to see if they have any violations. + * If no violations are found, an empty list is returned. + * + * @param cryptoApi + * @param members - The list of room members to check for violations. + */ async function mapToViolations(cryptoApi: CryptoApi, members: RoomMember[]): Promise { const violationList = new Array(); for (const member of members) { @@ -85,51 +103,57 @@ export function useUserIdentityWarningViewModel(room: Room, key: string): UserId ); // We need to listen for changes to the members list - const onRoomStateEvent = useCallback( - async (event: MatrixEvent): Promise => { - if (!crypto || event.getRoomId() !== room.roomId) { - return; - } - let shouldRefresh = false; - - const eventType = event.getType(); - - if (eventType === EventType.RoomEncryption && event.getStateKey() === "") { - // Room is now encrypted, so we can initialise the component. - shouldRefresh = true; - } else if (eventType == EventType.RoomMember) { - // We're processing an m.room.member event - // Something has changed in membership, someone joined or someone left or - // someone changed their display name. Anyhow let's refresh. - const userId = event.getStateKey(); - shouldRefresh = !!userId; - } + useTypedEventEmitter( + cli, + RoomStateEvent.Events, + useCallback( + async (event: MatrixEvent): Promise => { + if (!crypto || event.getRoomId() !== room.roomId) { + return; + } + let shouldRefresh = false; + + const eventType = event.getType(); + + if (eventType === EventType.RoomEncryption && event.getStateKey() === "") { + // Room is now encrypted, so we can initialise the component. + shouldRefresh = true; + } else if (eventType == EventType.RoomMember) { + // We're processing an m.room.member event + // Something has changed in membership, someone joined or someone left or + // someone changed their display name. Anyhow let's refresh. + const userId = event.getStateKey(); + shouldRefresh = !!userId; + } - if (shouldRefresh) { - loadViolations().catch((e) => { - logger.error("Error refreshing UserIdentityWarningViewModel:", e); - }); - } - }, - [crypto, room, loadViolations], + if (shouldRefresh) { + loadViolations().catch((e) => { + logger.error("Error refreshing UserIdentityWarningViewModel:", e); + }); + } + }, + [crypto, room, loadViolations], + ), ); - useTypedEventEmitter(cli, RoomStateEvent.Events, onRoomStateEvent); // We need to listen for changes to the verification status of the members to refresh violations - const onUserVerificationStatusChanged = useCallback( - (userId: string): void => { - if (members.find((m) => m.userId == userId)) { - // This member is tracked, we need to refresh. - // refresh all for now? - // As a later optimisation we could store the current violations and only update the relevant one. - loadViolations().catch((e) => { - logger.error("Error refreshing UserIdentityWarning:", e); - }); - } - }, - [loadViolations, members], + useTypedEventEmitter( + cli, + CryptoEvent.UserTrustStatusChanged, + useCallback( + (userId: string): void => { + if (members.find((m) => m.userId == userId)) { + // This member is tracked, we need to refresh. + // refresh all for now? + // As a later optimisation we could store the current violations and only update the relevant one. + loadViolations().catch((e) => { + logger.error("Error refreshing UserIdentityWarning:", e); + }); + } + }, + [loadViolations, members], + ), ); - useTypedEventEmitter(cli, CryptoEvent.UserTrustStatusChanged, onUserVerificationStatusChanged); useEffect(() => { loadViolations().catch((e) => { @@ -151,7 +175,7 @@ export function useUserIdentityWarningViewModel(room: Room, key: string): UserId }); } else if (currentPrompt.type === "PinViolation") { crypto.pinCurrentUserIdentity(currentPrompt.member.userId).catch((e) => { - logger.error("Error withdrawing verification requirement:", e); + logger.error("Error pinning user identity:", e); }); } }; From 61ca0eb2b1aac167945c0abae575b7ff2a3d7d1f Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 29 Jan 2025 17:50:45 +0100 Subject: [PATCH 4/5] review: Quick refactor, better handling of action on button click --- .../rooms/UserIdentityWarningViewModel.tsx | 36 ++++++++++--------- .../views/rooms/UserIdentityWarning.tsx | 10 +++++- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx b/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx index c52fc4ba608..5a5d2da2d13 100644 --- a/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx +++ b/src/components/viewmodels/rooms/UserIdentityWarningViewModel.tsx @@ -13,7 +13,6 @@ import { logger } from "matrix-js-sdk/src/logger"; import { useMatrixClientContext } from "../../../contexts/MatrixClientContext.tsx"; import { useTypedEventEmitter } from "../../../hooks/useEventEmitter.ts"; -import { ButtonEvent } from "../../views/elements/AccessibleButton.tsx"; export type ViolationType = "PinViolation" | "VerificationViolation"; @@ -34,9 +33,16 @@ export type ViolationPrompt = { */ export interface UserIdentityWarningState { currentPrompt?: ViolationPrompt; - onButtonClick: (ev: ButtonEvent) => void; + dispatchAction: (action: UserIdentityWarningViewModelAction) => void; } +/** + * List of actions that can be dispatched to the UserIdentityWarningViewModel. + */ +export type UserIdentityWarningViewModelAction = + | { type: "PinUserIdentity"; userId: string } + | { type: "WithdrawVerification"; userId: string }; + /** * Maps a list of room members to a list of violations. * Checks for all members in the room to see if they have any violations. @@ -161,28 +167,26 @@ export function useUserIdentityWarningViewModel(room: Room, key: string): UserId }); }, [loadViolations]); - let onButtonClick: (ev: ButtonEvent) => void = () => {}; - if (currentPrompt) { - onButtonClick = (ev: ButtonEvent): void => { - // XXX do we want some posthog tracking? + const dispatchAction = useCallback( + (action: UserIdentityWarningViewModelAction): void => { if (!crypto) { return; } - ev.preventDefault(); - if (currentPrompt.type === "VerificationViolation") { - crypto.withdrawVerificationRequirement(currentPrompt.member.userId).catch((e) => { - logger.error("Error withdrawing verification requirement:", e); - }); - } else if (currentPrompt.type === "PinViolation") { - crypto.pinCurrentUserIdentity(currentPrompt.member.userId).catch((e) => { + if (action.type === "PinUserIdentity") { + crypto.pinCurrentUserIdentity(action.userId).catch((e) => { logger.error("Error pinning user identity:", e); }); + } else if (action.type === "WithdrawVerification") { + crypto.withdrawVerificationRequirement(action.userId).catch((e) => { + logger.error("Error withdrawing verification requirement:", e); + }); } - }; - } + }, + [crypto], + ); return { currentPrompt, - onButtonClick, + dispatchAction, }; } diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index 674e0d46692..1933767778d 100644 --- a/src/components/views/rooms/UserIdentityWarning.tsx +++ b/src/components/views/rooms/UserIdentityWarning.tsx @@ -37,12 +37,20 @@ interface UserIdentityWarningProps { * button to acknowledge the change. */ export const UserIdentityWarning: React.FC = ({ room }) => { - const { currentPrompt, onButtonClick } = useUserIdentityWarningViewModel(room, room.roomId); + const { currentPrompt, dispatchAction } = useUserIdentityWarningViewModel(room, room.roomId); if (!currentPrompt) return null; const [title, action] = getTitleAndAction(currentPrompt); + const onButtonClick = (ev: ButtonEvent): void => { + ev.preventDefault(); + if (currentPrompt.type === "VerificationViolation") { + dispatchAction({ type: "WithdrawVerification", userId: currentPrompt.member.userId }); + } else { + dispatchAction({ type: "PinUserIdentity", userId: currentPrompt.member.userId }); + } + }; return warningBanner( currentPrompt.type === "VerificationViolation", memberAvatar(currentPrompt.member), From 673e0d431a7c5891c27f07d87b141ea76952b24e Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 31 Jan 2025 14:47:20 +0100 Subject: [PATCH 5/5] review: Small updates, remove commented code --- .../views/rooms/UserIdentityWarning-test.tsx | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx index c210e9343e0..7548854bafa 100644 --- a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -240,7 +240,7 @@ describe("UserIdentityWarning", () => { ); }); - describe("Warning are displayed in consistent order", () => { + describe("Warnings are displayed in consistent order", () => { it("Ensure lexicographic order for prompt", async () => { // members are not returned lexicographic order mockMembershipForRoom(room, ["@b:example.org", "@a:example.org"]); @@ -340,10 +340,6 @@ describe("UserIdentityWarning", () => { it("when invited users can see encrypted messages", async () => { // Nobody in the room yet mockMembershipForRoom(room, []); - // jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([]); - // jest.spyOn(room, "getMember").mockImplementation((userId) => { - // return null; - // }); jest.spyOn(room, "shouldEncryptForInvitedMembers").mockReturnValue(true); const crypto = client.getCrypto()!; jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( @@ -613,11 +609,7 @@ describe("UserIdentityWarning", () => { renderComponent(client, room); await sleep(10); // give it some time to finish initialising - // jest.spyOn(crypto, "getUserVerificationStatus").mockImplementation(async () => { act(() => { - // jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue( - // new UserVerificationStatus(false, false, false, true), - // ); client.emit( CryptoEvent.UserTrustStatusChanged, "@alice:example.org", @@ -625,8 +617,6 @@ describe("UserIdentityWarning", () => { ); firstStatusPromise.resolve(undefined); }); - // return Promise.resolve(new UserVerificationStatus(false, false, false, false)); - // }); await waitFor(() => expect( getWarningByText("Alice's (@alice:example.org) identity appears to have changed."),