From 807862798479d013fdf9608ed6f2f2f24250c105 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 11 Nov 2024 15:14:01 -0500 Subject: [PATCH] apply changes from review --- .../views/rooms/UserIdentityWarning.tsx | 120 ++++++------ .../views/rooms/UserIdentityWarning-test.tsx | 175 ++++++++++++++---- 2 files changed, 195 insertions(+), 100 deletions(-) diff --git a/src/components/views/rooms/UserIdentityWarning.tsx b/src/components/views/rooms/UserIdentityWarning.tsx index 17807339cc..2c494ba420 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 52d082740e..03119d3336 100644 --- a/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx +++ b/test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx @@ -197,41 +197,20 @@ 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 leaves, so we no longer show her warning. - act(() => { + // Alice joins. Her identity needs approval, so we should show a warning. client.emit( RoomStateEvent.Events, new MatrixEvent({ @@ -239,7 +218,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 +226,130 @@ 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, 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({ + 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(), + ); + + // 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