From f10e3abb6e6467970fb4a391116683853f47442e Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Mon, 1 Aug 2022 19:28:33 +0200 Subject: [PATCH] Exclude functional members from DM detection (#9124) * Exclude functional members from DM detection * Fix getRoomFunctionalMembers test * Simplify getFunctionalMembers * Remove unnecessary filter --- src/CallHandler.tsx | 3 +- src/utils/dm/findDMForUser.ts | 6 +- src/utils/room/getFunctionalMembers.ts | 27 +++++ .../room/getJoinedNonFunctionalMembers.ts | 27 +++++ test/CallHandler-test.ts | 22 +++- test/utils/dm/findDMForUser-test.ts | 104 ++++++++++++++---- .../getJoinedNonFunctionalMembers-test.ts | 97 ++++++++++++++++ .../room/getRoomFunctionalMembers-test.ts | 53 +++++++++ 8 files changed, 312 insertions(+), 27 deletions(-) create mode 100644 src/utils/room/getFunctionalMembers.ts create mode 100644 src/utils/room/getJoinedNonFunctionalMembers.ts create mode 100644 test/utils/room/getJoinedNonFunctionalMembers-test.ts create mode 100644 test/utils/room/getRoomFunctionalMembers-test.ts diff --git a/src/CallHandler.tsx b/src/CallHandler.tsx index 380f337cda..804b448549 100644 --- a/src/CallHandler.tsx +++ b/src/CallHandler.tsx @@ -61,6 +61,7 @@ import { ViewRoomPayload } from "./dispatcher/payloads/ViewRoomPayload"; import { KIND_CALL_TRANSFER } from "./components/views/dialogs/InviteDialogTypes"; import { OpenInviteDialogPayload } from "./dispatcher/payloads/OpenInviteDialogPayload"; import { findDMForUser } from './utils/dm/findDMForUser'; +import { getJoinedNonFunctionalMembers } from './utils/room/getJoinedNonFunctionalMembers'; export const PROTOCOL_PSTN = 'm.protocol.pstn'; export const PROTOCOL_PSTN_PREFIXED = 'im.vector.protocol.pstn'; @@ -861,7 +862,7 @@ export default class CallHandler extends EventEmitter { // We leave the check for whether there's already a call in this room until later, // otherwise it can race. - const members = room.getJoinedMembers(); + const members = getJoinedNonFunctionalMembers(room); if (members.length <= 1) { Modal.createDialog(ErrorDialog, { description: _t('You cannot place a call with yourself.'), diff --git a/src/utils/dm/findDMForUser.ts b/src/utils/dm/findDMForUser.ts index 7513de88ad..47e3c87a74 100644 --- a/src/utils/dm/findDMForUser.ts +++ b/src/utils/dm/findDMForUser.ts @@ -19,6 +19,7 @@ import { MatrixClient, Room } from "matrix-js-sdk/src/matrix"; import DMRoomMap from "../DMRoomMap"; import { isLocalRoom } from "../localRoom/isLocalRoom"; import { isJoinedOrNearlyJoined } from "../membership"; +import { getFunctionalMembers } from "../room/getFunctionalMembers"; /** * Tries to find a DM room with a specific user. @@ -39,8 +40,11 @@ export function findDMForUser(client: MatrixClient, userId: string): Room { if (r && r.getMyMembership() === "join") { if (isLocalRoom(r)) return false; + const functionalUsers = getFunctionalMembers(r); const members = r.currentState.getMembers(); - const joinedMembers = members.filter(m => isJoinedOrNearlyJoined(m.membership)); + const joinedMembers = members.filter( + m => !functionalUsers.includes(m.userId) && isJoinedOrNearlyJoined(m.membership), + ); const otherMember = joinedMembers.find(m => m.userId === userId); return otherMember && joinedMembers.length === 2; } diff --git a/src/utils/room/getFunctionalMembers.ts b/src/utils/room/getFunctionalMembers.ts new file mode 100644 index 0000000000..4c839b0377 --- /dev/null +++ b/src/utils/room/getFunctionalMembers.ts @@ -0,0 +1,27 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { Room, UNSTABLE_ELEMENT_FUNCTIONAL_USERS } from "matrix-js-sdk/src/matrix"; + +export const getFunctionalMembers = (room: Room): string[] => { + const [functionalUsersStateEvent] = room.currentState.getStateEvents(UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name); + + if (Array.isArray(functionalUsersStateEvent?.getContent().service_members)) { + return functionalUsersStateEvent.getContent().service_members; + } + + return []; +}; diff --git a/src/utils/room/getJoinedNonFunctionalMembers.ts b/src/utils/room/getJoinedNonFunctionalMembers.ts new file mode 100644 index 0000000000..912c4bf1f1 --- /dev/null +++ b/src/utils/room/getJoinedNonFunctionalMembers.ts @@ -0,0 +1,27 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { Room, RoomMember } from "matrix-js-sdk/src/matrix"; + +import { getFunctionalMembers } from "./getFunctionalMembers"; + +/** + * Returns all room members that are non-functional (bots etc.). + */ +export const getJoinedNonFunctionalMembers = (room: Room): RoomMember[] => { + const functionalMembers = getFunctionalMembers(room); + return room.getJoinedMembers().filter(m => !functionalMembers.includes(m.userId)); +}; diff --git a/test/CallHandler-test.ts b/test/CallHandler-test.ts index b7a047a9ef..72b105a160 100644 --- a/test/CallHandler-test.ts +++ b/test/CallHandler-test.ts @@ -17,6 +17,7 @@ limitations under the License. import { IProtocol } from 'matrix-js-sdk/src/matrix'; import { CallEvent, CallState, CallType } from 'matrix-js-sdk/src/webrtc/call'; import EventEmitter from 'events'; +import { mocked } from 'jest-mock'; import CallHandler, { CallHandlerEvent, PROTOCOL_PSTN, PROTOCOL_PSTN_PREFIXED, PROTOCOL_SIP_NATIVE, PROTOCOL_SIP_VIRTUAL, @@ -26,6 +27,11 @@ import { MatrixClientPeg } from '../src/MatrixClientPeg'; import DMRoomMap from '../src/utils/DMRoomMap'; import SdkConfig from '../src/SdkConfig'; import { Action } from "../src/dispatcher/actions"; +import { getFunctionalMembers } from "../src/utils/room/getFunctionalMembers"; + +jest.mock("../src/utils/room/getFunctionalMembers", () => ({ + getFunctionalMembers: jest.fn(), +})); // The Matrix IDs that the user sees when talking to Alice & Bob const NATIVE_ALICE = "@alice:example.org"; @@ -41,6 +47,8 @@ const NATIVE_ROOM_ALICE = "$alice_room:example.org"; const NATIVE_ROOM_BOB = "$bob_room:example.org"; const NATIVE_ROOM_CHARLIE = "$charlie_room:example.org"; +const FUNCTIONAL_USER = "@bot:example.com"; + // The room we use to talk to virtual Bob (but that the user does not see) // Bob has a virtual room, but Alice doesn't const VIRTUAL_ROOM_BOB = "$virtual_bob_room:example.org"; @@ -69,9 +77,17 @@ function mkStubDM(roomId, userId) { getAvatarUrl: () => 'mxc://avatar.url/image.png', getMxcAvatarUrl: () => 'mxc://avatar.url/image.png', }, + { + userId: FUNCTIONAL_USER, + name: 'Bot user', + rawDisplayName: 'Bot user', + roomId: roomId, + membership: 'join', + getAvatarUrl: () => 'mxc://avatar.url/image.png', + getMxcAvatarUrl: () => 'mxc://avatar.url/image.png', + }, ]); room.currentState.getMembers = room.getJoinedMembers; - return room; } @@ -132,6 +148,10 @@ describe('CallHandler', () => { callHandler = new CallHandler(); callHandler.start(); + mocked(getFunctionalMembers).mockReturnValue([ + FUNCTIONAL_USER, + ]); + const nativeRoomAlice = mkStubDM(NATIVE_ROOM_ALICE, NATIVE_ALICE); const nativeRoomBob = mkStubDM(NATIVE_ROOM_BOB, NATIVE_BOB); const nativeRoomCharie = mkStubDM(NATIVE_ROOM_CHARLIE, NATIVE_CHARLIE); diff --git a/test/utils/dm/findDMForUser-test.ts b/test/utils/dm/findDMForUser-test.ts index a3dda43c5c..c1acad95e7 100644 --- a/test/utils/dm/findDMForUser-test.ts +++ b/test/utils/dm/findDMForUser-test.ts @@ -17,56 +17,112 @@ limitations under the License. import { mocked } from "jest-mock"; import { MatrixClient, Room } from "matrix-js-sdk/src/matrix"; -import { DirectoryMember, ThreepidMember } from "../../../src/utils/direct-messages"; -import { MatrixClientPeg } from "../../../src/MatrixClientPeg"; import DMRoomMap from "../../../src/utils/DMRoomMap"; -import { createTestClient } from "../../test-utils"; -import { findDMRoom } from "../../../src/utils/dm/findDMRoom"; +import { createTestClient, makeMembershipEvent } from "../../test-utils"; +import { LocalRoom } from "../../../src/models/LocalRoom"; import { findDMForUser } from "../../../src/utils/dm/findDMForUser"; +import { getFunctionalMembers } from "../../../src/utils/room/getFunctionalMembers"; -jest.mock("../../../src/utils/dm/findDMForUser", () => ({ - findDMForUser: jest.fn(), +jest.mock("../../../src/utils/room/getFunctionalMembers", () => ({ + getFunctionalMembers: jest.fn(), })); -describe("findDMRoom", () => { +describe("findDMForUser", () => { const userId1 = "@user1:example.com"; - const member1 = new DirectoryMember({ user_id: userId1 }); - const member2 = new ThreepidMember("user2"); - let mockClient: MatrixClient; + const userId2 = "@user2:example.com"; + const botId = "@bot:example.com"; let room1: Room; + let room2: LocalRoom; + let room3: Room; + let room4: Room; + let room5: Room; let dmRoomMap: DMRoomMap; + let mockClient: MatrixClient; beforeEach(() => { mockClient = createTestClient(); - jest.spyOn(MatrixClientPeg, "get").mockReturnValue(mockClient); + + // always return the bot user as functional member + mocked(getFunctionalMembers).mockReturnValue([botId]); room1 = new Room("!room1:example.com", mockClient, userId1); room1.getMyMembership = () => "join"; + room1.currentState.setStateEvents([ + makeMembershipEvent(room1.roomId, userId1, "join"), + makeMembershipEvent(room1.roomId, userId2, "join"), + ]); + + // this should not be a DM room because it is a local room + room2 = new LocalRoom("!room2:example.com", mockClient, userId1); + room2.getMyMembership = () => "join"; + room2.getLastActiveTimestamp = () => 100; + + room3 = new Room("!room3:example.com", mockClient, userId1); + room3.getMyMembership = () => "join"; + room3.currentState.setStateEvents([ + makeMembershipEvent(room3.roomId, userId1, "join"), + makeMembershipEvent(room3.roomId, userId2, "join"), + // Adding the bot user here. Should be excluded when determining if the room is a DM. + makeMembershipEvent(room3.roomId, botId, "join"), + ]); + + // this should not be a DM room because it has only one joined user + room4 = new Room("!room4:example.com", mockClient, userId1); + room4.getMyMembership = () => "join"; + room4.currentState.setStateEvents([ + makeMembershipEvent(room4.roomId, userId1, "invite"), + makeMembershipEvent(room4.roomId, userId2, "join"), + ]); + + // this should not be a DM room because it has no users + room5 = new Room("!room5:example.com", mockClient, userId1); + room5.getLastActiveTimestamp = () => 100; + + mocked(mockClient.getRoom).mockImplementation((roomId: string) => { + return { + [room1.roomId]: room1, + [room2.roomId]: room2, + [room3.roomId]: room3, + [room4.roomId]: room4, + [room5.roomId]: room5, + }[roomId]; + }); dmRoomMap = { getDMRoomForIdentifiers: jest.fn(), getDMRoomsForUserId: jest.fn(), } as unknown as DMRoomMap; jest.spyOn(DMRoomMap, "shared").mockReturnValue(dmRoomMap); + mocked(dmRoomMap.getDMRoomsForUserId).mockReturnValue([ + room1.roomId, + room2.roomId, + room3.roomId, + room4.roomId, + room5.roomId, + ]); }); - it("should return the room for a single target with a room", () => { - mocked(findDMForUser).mockReturnValue(room1); - expect(findDMRoom(mockClient, [member1])).toBe(room1); + describe("for an empty DM room list", () => { + beforeEach(() => { + mocked(dmRoomMap.getDMRoomsForUserId).mockReturnValue([]); + }); + + it("should return undefined", () => { + expect(findDMForUser(mockClient, userId1)).toBeUndefined(); + }); }); - it("should return null for a single target without a room", () => { - mocked(findDMForUser).mockReturnValue(null); - expect(findDMRoom(mockClient, [member1])).toBeNull(); + it("should find a room ordered by last activity 1", () => { + room1.getLastActiveTimestamp = () => 2; + room3.getLastActiveTimestamp = () => 1; + + expect(findDMForUser(mockClient, userId1)).toBe(room1); }); - it("should return the room for 2 targets with a room", () => { - mocked(dmRoomMap.getDMRoomForIdentifiers).mockReturnValue(room1); - expect(findDMRoom(mockClient, [member1, member2])).toBe(room1); - }); + it("should find a room ordered by last activity 2", () => { + room1.getLastActiveTimestamp = () => 1; + room3.getLastActiveTimestamp = () => 2; - it("should return null for 2 targets without a room", () => { - mocked(dmRoomMap.getDMRoomForIdentifiers).mockReturnValue(null); - expect(findDMRoom(mockClient, [member1, member2])).toBeNull(); + expect(findDMForUser(mockClient, userId1)).toBe(room3); }); }); diff --git a/test/utils/room/getJoinedNonFunctionalMembers-test.ts b/test/utils/room/getJoinedNonFunctionalMembers-test.ts new file mode 100644 index 0000000000..7973f6f384 --- /dev/null +++ b/test/utils/room/getJoinedNonFunctionalMembers-test.ts @@ -0,0 +1,97 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { mocked } from "jest-mock"; +import { MatrixClient, Room, RoomMember } from "matrix-js-sdk/src/matrix"; + +import { getFunctionalMembers } from "../../../src/utils/room/getFunctionalMembers"; +import { getJoinedNonFunctionalMembers } from "../../../src/utils/room/getJoinedNonFunctionalMembers"; + +jest.mock("../../../src/utils/room/getFunctionalMembers", () => ({ + getFunctionalMembers: jest.fn(), +})); + +describe("getJoinedNonFunctionalMembers", () => { + let room: Room; + let roomMember1: RoomMember; + let roomMember2: RoomMember; + + beforeEach(() => { + room = new Room("!room:example.com", {} as unknown as MatrixClient, "@user:example.com"); + room.getJoinedMembers = jest.fn(); + + roomMember1 = new RoomMember(room.roomId, "@user1:example.com"); + roomMember2 = new RoomMember(room.roomId, "@user2:example.com"); + }); + + describe("if there are no members", () => { + beforeEach(() => { + mocked(room.getJoinedMembers).mockReturnValue([]); + mocked(getFunctionalMembers).mockReturnValue([]); + }); + + it("should return an empty list", () => { + expect(getJoinedNonFunctionalMembers(room)).toHaveLength(0); + }); + }); + + describe("if there are only regular room members", () => { + beforeEach(() => { + mocked(room.getJoinedMembers).mockReturnValue([ + roomMember1, + roomMember2, + ]); + mocked(getFunctionalMembers).mockReturnValue([]); + }); + + it("should return the room members", () => { + const members = getJoinedNonFunctionalMembers(room); + expect(members).toContain(roomMember1); + expect(members).toContain(roomMember2); + }); + }); + + describe("if there are only functional room members", () => { + beforeEach(() => { + mocked(room.getJoinedMembers).mockReturnValue([]); + mocked(getFunctionalMembers).mockReturnValue([ + "@functional:example.com", + ]); + }); + + it("should return an empty list", () => { + expect(getJoinedNonFunctionalMembers(room)).toHaveLength(0); + }); + }); + + describe("if there are some functional room members", () => { + beforeEach(() => { + mocked(room.getJoinedMembers).mockReturnValue([ + roomMember1, + roomMember2, + ]); + mocked(getFunctionalMembers).mockReturnValue([ + roomMember1.userId, + ]); + }); + + it("should only return the non-functional members", () => { + const members = getJoinedNonFunctionalMembers(room); + expect(members).not.toContain(roomMember1); + expect(members).toContain(roomMember2); + }); + }); +}); diff --git a/test/utils/room/getRoomFunctionalMembers-test.ts b/test/utils/room/getRoomFunctionalMembers-test.ts new file mode 100644 index 0000000000..bcbba0f226 --- /dev/null +++ b/test/utils/room/getRoomFunctionalMembers-test.ts @@ -0,0 +1,53 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { Room, UNSTABLE_ELEMENT_FUNCTIONAL_USERS } from "matrix-js-sdk/src/matrix"; + +import { getFunctionalMembers } from "../../../src/utils/room/getFunctionalMembers"; +import { createTestClient, mkEvent } from "../../test-utils"; + +describe("getRoomFunctionalMembers", () => { + const client = createTestClient(); + const room = new Room("!room:example.com", client, client.getUserId()); + + it("should return an empty array if no functional members state event exists", () => { + expect(getFunctionalMembers(room)).toHaveLength(0); + }); + + it("should return an empty array if functional members state event does not have a service_members field", () => { + room.currentState.setStateEvents([mkEvent({ + event: true, + type: UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name, + user: "@user:example.com)", + room: room.roomId, + skey: "", + content: {}, + })]); + expect(getFunctionalMembers(room)).toHaveLength(0); + }); + + it("should return service_members field of the functional users state event", () => { + room.currentState.setStateEvents([mkEvent({ + event: true, + type: UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name, + user: "@user:example.com)", + room: room.roomId, + skey: "", + content: { service_members: ["@user:example.com"] }, + })]); + expect(getFunctionalMembers(room)).toEqual(["@user:example.com"]); + }); +});