From 8feed1a22520be748cce469713898546d92eed05 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Thu, 16 Feb 2023 14:17:43 +0100 Subject: [PATCH] Try to resolve emails before creating a DM (#10164) --- src/components/views/dialogs/InviteDialog.tsx | 6 +- src/utils/direct-messages.ts | 16 +- src/utils/threepids.ts | 106 ++++++++++++ test/test-utils/test-utils.ts | 1 + test/utils/direct-messages-test.ts | 40 ++++- test/utils/threepids-test.ts | 157 ++++++++++++++++++ 6 files changed, 319 insertions(+), 7 deletions(-) create mode 100644 src/utils/threepids.ts create mode 100644 test/utils/threepids-test.ts diff --git a/src/components/views/dialogs/InviteDialog.tsx b/src/components/views/dialogs/InviteDialog.tsx index 91c4e01e25..f1c8913da8 100644 --- a/src/components/views/dialogs/InviteDialog.tsx +++ b/src/components/views/dialogs/InviteDialog.tsx @@ -469,10 +469,14 @@ export default class InviteDialog extends React.PureComponent => { + this.setState({ + busy: true, + }); + try { const cli = MatrixClientPeg.get(); const targets = this.convertFilter(); - startDmOnFirstMessage(cli, targets); + await startDmOnFirstMessage(cli, targets); this.props.onFinished(true); } catch (err) { logger.error(err); diff --git a/src/utils/direct-messages.ts b/src/utils/direct-messages.ts index b1fd4fa844..5e5a5d9c19 100644 --- a/src/utils/direct-messages.ts +++ b/src/utils/direct-messages.ts @@ -28,9 +28,19 @@ import { findDMRoom } from "./dm/findDMRoom"; import { privateShouldBeEncrypted } from "./rooms"; import { createDmLocalRoom } from "./dm/createDmLocalRoom"; import { startDm } from "./dm/startDm"; +import { resolveThreePids } from "./threepids"; export async function startDmOnFirstMessage(client: MatrixClient, targets: Member[]): Promise { - const existingRoom = findDMRoom(client, targets); + let resolvedTargets = targets; + + try { + resolvedTargets = await resolveThreePids(targets, client); + } catch (e) { + logger.warn("Error resolving 3rd-party members", e); + } + + const existingRoom = findDMRoom(client, resolvedTargets); + if (existingRoom) { dis.dispatch({ action: Action.ViewRoom, @@ -42,12 +52,12 @@ export async function startDmOnFirstMessage(client: MatrixClient, targets: Membe return existingRoom; } - const room = await createDmLocalRoom(client, targets); + const room = await createDmLocalRoom(client, resolvedTargets); dis.dispatch({ action: Action.ViewRoom, room_id: room.roomId, joining: false, - targets, + targets: resolvedTargets, }); return room; } diff --git a/src/utils/threepids.ts b/src/utils/threepids.ts new file mode 100644 index 0000000000..57d42b5875 --- /dev/null +++ b/src/utils/threepids.ts @@ -0,0 +1,106 @@ +/* +Copyright 2023 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 { MatrixClient } from "matrix-js-sdk/src/matrix"; + +import { DirectoryMember, Member, ThreepidMember } from "./direct-messages"; + +/** + * Tries to resolve the ThreepidMembers to DirectoryMembers. + * + * @param members - List of members to resolve + * @returns {Promise} Same list with ThreepidMembers replaced by DirectoryMembers if succesfully resolved + */ +export const resolveThreePids = async (members: Member[], client: MatrixClient): Promise => { + const threePidMembers = members.filter((m) => m instanceof ThreepidMember) as ThreepidMember[]; + + // Nothing to do here + if (threePidMembers.length === 0) return members; + + const lookedUpProfiles = await lookupThreePidProfiles(threePidMembers, client); + + return members.map((member: Member) => { + if (!(member instanceof ThreepidMember)) return member; + + const lookedUpProfile = lookedUpProfiles.find((r) => r.threePidId === member.userId); + + // No profile found for this member; use the ThreepidMember. + if (!lookedUpProfile) return member; + + return new DirectoryMember({ + user_id: lookedUpProfile.mxid, + avatar_url: lookedUpProfile?.profile?.avatar_url, + display_name: lookedUpProfile?.profile?.displayname, + }); + }); +}; + +/** + * Tries to look up the ThreepidMembers. + * + * @param threePids - List of 3rd-party members to look up + * @returns List of resolved 3rd-party IDs with their MXIDs + */ +export const lookupThreePids = async ( + threePids: ThreepidMember[], + client: MatrixClient, +): Promise<{ threePidId: string; mxid: string }[]> => { + // No identity server configured. Unable to resolve any 3rd party member. + if (!client.identityServer) return []; + + // Nothing we can search, return null + if (threePids.length === 0) return []; + + const token = await client.identityServer.getAccessToken(); + const lookedUp = await client.bulkLookupThreePids( + threePids.map((t) => [t.isEmail ? "email" : "msisdn", t.userId]), + token, + ); + + return lookedUp.threepids.map(([_threePidType, threePidId, mxid]: [string, string, string]) => ({ + threePidId, + mxid, + })); +}; + +/** + * Tries to look up the MXIDs and profiles of the ThreepidMembers. + * + * @param threePids - List of 3rd-prty members to look up + * @returns List of resolved 3rd-party members with their MXIDs and profile (if found) + */ +export const lookupThreePidProfiles = async ( + threePids: ThreepidMember[], + client: MatrixClient, +): Promise<{ threePidId: string; mxid: string; profile: null | { avatar_url?: string; displayname?: string } }[]> => { + const lookedUpThreePids = await lookupThreePids(threePids, client); + const promises = lookedUpThreePids.map(async (t) => { + let profile: null | { avatar_url?: string; display_name?: string } = null; + + try { + profile = await client.getProfileInfo(t.mxid); + } catch { + // ignore any lookup error + } + + return { + threePidId: t.threePidId, + mxid: t.mxid, + profile, + }; + }); + return Promise.all(promises); +}; diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index abe232de2d..8b7d839aae 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -180,6 +180,7 @@ export function createTestClient(): MatrixClient { getPushRules: jest.fn().mockResolvedValue(undefined), getPushers: jest.fn().mockResolvedValue({ pushers: [] }), getThreePids: jest.fn().mockResolvedValue({ threepids: [] }), + bulkLookupThreePids: jest.fn().mockResolvedValue({ threepids: [] }), setPusher: jest.fn().mockResolvedValue(undefined), setPushRuleEnabled: jest.fn().mockResolvedValue(undefined), setPushRuleActions: jest.fn().mockResolvedValue(undefined), diff --git a/test/utils/direct-messages-test.ts b/test/utils/direct-messages-test.ts index b803b9b23d..80d9f9becc 100644 --- a/test/utils/direct-messages-test.ts +++ b/test/utils/direct-messages-test.ts @@ -16,6 +16,7 @@ limitations under the License. import { mocked } from "jest-mock"; import { ClientEvent, MatrixClient, Room } from "matrix-js-sdk/src/matrix"; +import { logger } from "matrix-js-sdk/src/logger"; import DMRoomMap from "../../src/utils/DMRoomMap"; import { createTestClient } from "../test-utils"; @@ -28,6 +29,8 @@ import { waitForRoomReadyAndApplyAfterCreateCallbacks } from "../../src/utils/lo import { findDMRoom } from "../../src/utils/dm/findDMRoom"; import { createDmLocalRoom } from "../../src/utils/dm/createDmLocalRoom"; import { startDm } from "../../src/utils/dm/startDm"; +import { Member } from "../../src/utils/direct-messages"; +import { resolveThreePids } from "../../src/utils/threepids"; jest.mock("../../src/utils/rooms", () => ({ ...(jest.requireActual("../../src/utils/rooms") as object), @@ -59,6 +62,12 @@ jest.mock("../../src/utils/dm/startDm", () => ({ startDm: jest.fn(), })); +jest.mock("../../src/utils/threepids", () => ({ + resolveThreePids: jest.fn().mockImplementation(async (members: Member[]) => { + return members; + }), +})); + describe("direct-messages", () => { const userId1 = "@user1:example.com"; const member1 = new dmModule.DirectoryMember({ user_id: userId1 }); @@ -69,8 +78,6 @@ describe("direct-messages", () => { let roomEvents: Room[]; beforeEach(() => { - jest.restoreAllMocks(); - mockClient = createTestClient(); jest.spyOn(MatrixClientPeg, "get").mockReturnValue(mockClient); roomEvents = []; @@ -89,10 +96,17 @@ describe("direct-messages", () => { } as unknown as DMRoomMap; jest.spyOn(DMRoomMap, "shared").mockReturnValue(dmRoomMap); jest.spyOn(dis, "dispatch"); + jest.spyOn(logger, "warn"); + jest.useFakeTimers(); jest.setSystemTime(new Date(2022, 7, 4, 11, 12, 30, 42)); }); + afterEach(() => { + jest.restoreAllMocks(); + jest.useRealTimers(); + }); + describe("startDmOnFirstMessage", () => { describe("if no room exists", () => { beforeEach(() => { @@ -101,7 +115,8 @@ describe("direct-messages", () => { it("should create a local room and dispatch a view room event", async () => { mocked(createDmLocalRoom).mockResolvedValue(localRoom); - const room = await dmModule.startDmOnFirstMessage(mockClient, [member1]); + const members = [member1]; + const room = await dmModule.startDmOnFirstMessage(mockClient, members); expect(room).toBe(localRoom); expect(dis.dispatch).toHaveBeenCalledWith({ action: Action.ViewRoom, @@ -109,6 +124,25 @@ describe("direct-messages", () => { joining: false, targets: [member1], }); + + // assert, that startDmOnFirstMessage tries to resolve 3rd-party IDs + expect(resolveThreePids).toHaveBeenCalledWith(members, mockClient); + }); + + it("should work when resolveThreePids raises an error", async () => { + const error = new Error("error 4711"); + mocked(resolveThreePids).mockRejectedValue(error); + + mocked(createDmLocalRoom).mockResolvedValue(localRoom); + const members = [member1]; + const room = await dmModule.startDmOnFirstMessage(mockClient, members); + expect(room).toBe(localRoom); + + // ensure that startDmOnFirstMessage tries to resolve 3rd-party IDs + expect(resolveThreePids).toHaveBeenCalledWith(members, mockClient); + + // ensure that the error is logged + expect(logger.warn).toHaveBeenCalledWith("Error resolving 3rd-party members", error); }); }); diff --git a/test/utils/threepids-test.ts b/test/utils/threepids-test.ts new file mode 100644 index 0000000000..3f8ac6242e --- /dev/null +++ b/test/utils/threepids-test.ts @@ -0,0 +1,157 @@ +/* +Copyright 2023 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 { IIdentityServerProvider } from "matrix-js-sdk/src/@types/IIdentityServerProvider"; +import { MatrixClient } from "matrix-js-sdk/src/matrix"; + +import { DirectoryMember, ThreepidMember } from "../../src/utils/direct-messages"; +import { lookupThreePids, resolveThreePids } from "../../src/utils/threepids"; +import { stubClient } from "../test-utils"; + +describe("threepids", () => { + let client: Mocked; + const accessToken = "s3cr3t"; + let identityServer: Mocked; + + beforeEach(() => { + client = stubClient() as Mocked; + identityServer = { + getAccessToken: jest.fn().mockResolvedValue(accessToken), + } as unknown as Mocked; + }); + + describe("resolveThreePids", () => { + const userId = "@user1:example.com"; + const directoryMember = new DirectoryMember({ + user_id: userId, + }); + + const threePid1Id = "three1@example.com"; + const threePid1MXID = "@three1:example.com"; + const threePid1Member = new ThreepidMember(threePid1Id); + const threePid1Displayname = "Three Pid 1"; + const threePid2Id = "three2@example.com"; + const threePid2MXID = "@three2:example.com"; + const threePid2Member = new ThreepidMember(threePid2Id); + const threePid3Id = "three3@example.com"; + const threePid3Member = new ThreepidMember(threePid3Id); + const threePidPhoneId = "8801500121121"; + const threePidPhoneMember = new ThreepidMember(threePidPhoneId); + + it("should return an empty list for an empty input", async () => { + expect(await resolveThreePids([], client)).toEqual([]); + }); + + it("should return the same list for non-3rd-party members", async () => { + expect(await resolveThreePids([directoryMember], client)).toEqual([directoryMember]); + }); + + it("should return the same list for if no identity server is configured", async () => { + expect(await resolveThreePids([directoryMember, threePid1Member], client)).toEqual([ + directoryMember, + threePid1Member, + ]); + }); + + describe("when an identity server is configured", () => { + beforeEach(() => { + client.identityServer = identityServer; + }); + + it("should return the same list if the lookup doesn't return any results", async () => { + expect( + await resolveThreePids( + [directoryMember, threePid1Member, threePid2Member, threePidPhoneMember], + client, + ), + ).toEqual([directoryMember, threePid1Member, threePid2Member, threePidPhoneMember]); + expect(client.bulkLookupThreePids).toHaveBeenCalledWith( + [ + ["email", threePid1Id], + ["email", threePid2Id], + ["msisdn", threePidPhoneId], + ], + accessToken, + ); + }); + + describe("and some 3-rd party members can be resolved", () => { + beforeEach(() => { + client.bulkLookupThreePids.mockResolvedValue({ + threepids: [ + ["email", threePid1Id, threePid1MXID], + ["email", threePid2Id, threePid2MXID], + ], + }); + }); + + it("should return the resolved members", async () => { + expect( + await resolveThreePids( + [directoryMember, threePid1Member, threePid2Member, threePid3Member], + client, + ), + ).toEqual([ + directoryMember, + new DirectoryMember({ user_id: threePid1MXID }), + new DirectoryMember({ user_id: threePid2MXID }), + threePid3Member, + ]); + expect(client.bulkLookupThreePids).toHaveBeenCalledWith( + [ + ["email", threePid1Id], + ["email", threePid2Id], + ["email", threePid3Id], + ], + accessToken, + ); + }); + + describe("and some 3rd-party members have a profile", () => { + beforeEach(() => { + client.getProfileInfo.mockImplementation((matrixId: string) => { + if (matrixId === threePid1MXID) + return Promise.resolve({ displayname: threePid1Displayname }); + throw new Error("Profile not found"); + }); + }); + + it("should resolve the profiles", async () => { + expect( + await resolveThreePids( + [directoryMember, threePid1Member, threePid2Member, threePid3Member], + client, + ), + ).toEqual([ + directoryMember, + new DirectoryMember({ user_id: threePid1MXID, display_name: threePid1Displayname }), + new DirectoryMember({ user_id: threePid2MXID }), + threePid3Member, + ]); + }); + }); + }); + }); + }); + + describe("lookupThreePids", () => { + it("should return an empty list for an empty list", async () => { + client.identityServer = identityServer; + expect(await lookupThreePids([], client)).toEqual([]); + }); + }); +});