From 18e73382551b938bb54d48a0aa8fc370c13a55f0 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Sat, 7 Jan 2023 14:01:42 +1300 Subject: [PATCH 1/6] prune client infromation events from old devices --- .../views/settings/devices/useOwnDevices.ts | 6 ++++- src/utils/device/clientInformation.ts | 27 +++++++++++++++---- .../tabs/user/SessionManagerTab-test.tsx | 2 ++ 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/components/views/settings/devices/useOwnDevices.ts b/src/components/views/settings/devices/useOwnDevices.ts index 58f6cef43d2..601c1bc6332 100644 --- a/src/components/views/settings/devices/useOwnDevices.ts +++ b/src/components/views/settings/devices/useOwnDevices.ts @@ -35,7 +35,7 @@ import { CryptoEvent } from "matrix-js-sdk/src/crypto"; import MatrixClientContext from "../../../../contexts/MatrixClientContext"; import { _t } from "../../../../languageHandler"; -import { getDeviceClientInformation } from "../../../../utils/device/clientInformation"; +import { getDeviceClientInformation, pruneClientInformation } from "../../../../utils/device/clientInformation"; import { DevicesDictionary, ExtendedDevice, ExtendedDeviceAppInfo } from "./types"; import { useEventEmitter } from "../../../../hooks/useEventEmitter"; import { parseUserAgent } from "../../../../utils/device/parseUserAgent"; @@ -176,6 +176,10 @@ export const useOwnDevices = (): DevicesState => { refreshDevices(); }, [refreshDevices]); + useEffect(() => { + pruneClientInformation(Object.keys(devices), matrixClient); + }, [devices]); + useEventEmitter(matrixClient, CryptoEvent.DevicesUpdated, (users: string[]): void => { if (users.includes(userId)) { refreshDevices(); diff --git a/src/utils/device/clientInformation.ts b/src/utils/device/clientInformation.ts index e97135ab1f8..c96fc3feb5c 100644 --- a/src/utils/device/clientInformation.ts +++ b/src/utils/device/clientInformation.ts @@ -40,8 +40,8 @@ const formatUrl = (): string | undefined => { ].join(""); }; -export const getClientInformationEventType = (deviceId: string): string => - `io.element.matrix_client_information.${deviceId}`; +const clientInformationEventPrefix = "io.element.matrix_client_information."; +export const getClientInformationEventType = (deviceId: string): string => `${clientInformationEventPrefix}${deviceId}`; /** * Record extra client information for the current device @@ -66,9 +66,26 @@ export const recordClientInformation = async ( }; /** - * Remove extra client information - * @todo(kerrya) revisit after MSC3391: account data deletion is done - * (PSBE-12) + * Remove client information events for devices that no longer exist + * @param validDeviceIds - ids of current devices, + * client information for devices NOT in this list will be removed + */ +export const pruneClientInformation = async (validDeviceIds: string[], matrixClient: MatrixClient): Promise => { + const orphanClientInformationEvents = Object.values(matrixClient.store.accountData).filter((event) => { + if (event.getType().startsWith(clientInformationEventPrefix)) { + const [, deviceId] = event.getType().split(clientInformationEventPrefix); + return deviceId && !validDeviceIds.includes(deviceId); + } + return false; + }); + + orphanClientInformationEvents.forEach((event) => { + matrixClient.deleteAccountData(event.getType()); + }); +}; + +/** + * Remove extra client information for current device */ export const removeClientInformation = async (matrixClient: MatrixClient): Promise => { const deviceId = matrixClient.getDeviceId(); diff --git a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx index a90c37c3883..15c18b1ea69 100644 --- a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -93,6 +93,8 @@ describe("", () => { setLocalNotificationSettings: jest.fn(), getVersions: jest.fn().mockResolvedValue({}), }); + // @ts-ignore mock + mockClient.store = { accountData: {} }; const defaultProps = {}; const getComponent = (props = {}): React.ReactElement => ( From 160527e5d77eef7f952680717f02a7c84003c962 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 9 Jan 2023 13:34:27 +1300 Subject: [PATCH 2/6] test client data pruning --- .../views/settings/devices/useOwnDevices.ts | 9 +++- src/utils/device/clientInformation.ts | 1 + .../tabs/user/SessionManagerTab-test.tsx | 46 ++++++++++++++++++- 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/components/views/settings/devices/useOwnDevices.ts b/src/components/views/settings/devices/useOwnDevices.ts index 601c1bc6332..14cdfc34a98 100644 --- a/src/components/views/settings/devices/useOwnDevices.ts +++ b/src/components/views/settings/devices/useOwnDevices.ts @@ -177,8 +177,13 @@ export const useOwnDevices = (): DevicesState => { }, [refreshDevices]); useEffect(() => { - pruneClientInformation(Object.keys(devices), matrixClient); - }, [devices]); + const deviceIds = Object.keys(devices); + // empty devices means devices have not been fetched yet + // as there is always at least the current device + if (deviceIds.length) { + pruneClientInformation(Object.keys(devices), matrixClient); + } + }, [devices, matrixClient]); useEventEmitter(matrixClient, CryptoEvent.DevicesUpdated, (users: string[]): void => { if (users.includes(userId)) { diff --git a/src/utils/device/clientInformation.ts b/src/utils/device/clientInformation.ts index c96fc3feb5c..827f7206e5c 100644 --- a/src/utils/device/clientInformation.ts +++ b/src/utils/device/clientInformation.ts @@ -74,6 +74,7 @@ export const pruneClientInformation = async (validDeviceIds: string[], matrixCli const orphanClientInformationEvents = Object.values(matrixClient.store.accountData).filter((event) => { if (event.getType().startsWith(clientInformationEventPrefix)) { const [, deviceId] = event.getType().split(clientInformationEventPrefix); + console.log('YO', deviceId, validDeviceIds) return deviceId && !validDeviceIds.includes(deviceId); } return false; diff --git a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx index 15c18b1ea69..9e15cecb02e 100644 --- a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -46,6 +46,7 @@ import LogoutDialog from "../../../../../../src/components/views/dialogs/LogoutD import { DeviceSecurityVariation, ExtendedDevice } from "../../../../../../src/components/views/settings/devices/types"; import { INACTIVE_DEVICE_AGE_MS } from "../../../../../../src/components/views/settings/devices/filter"; import SettingsStore from "../../../../../../src/settings/SettingsStore"; +import { getClientInformationEventType } from "../../../../../../src/utils/device/clientInformation"; mockPlatformPeg(); @@ -87,14 +88,13 @@ describe("", () => { generateClientSecret: jest.fn(), setDeviceDetails: jest.fn(), getAccountData: jest.fn(), + deleteAccountData: jest.fn(), doesServerSupportUnstableFeature: jest.fn().mockResolvedValue(true), getPushers: jest.fn(), setPusher: jest.fn(), setLocalNotificationSettings: jest.fn(), getVersions: jest.fn().mockResolvedValue({}), }); - // @ts-ignore mock - mockClient.store = { accountData: {} }; const defaultProps = {}; const getComponent = (props = {}): React.ReactElement => ( @@ -184,6 +184,9 @@ describe("", () => { ], }); + // @ts-ignore mock + mockClient.store = { accountData: {} }; + mockClient.getAccountData.mockReset().mockImplementation((eventType) => { if (eventType.startsWith(LOCAL_NOTIFICATION_SETTINGS_PREFIX.name)) { return new MatrixEvent({ @@ -669,6 +672,45 @@ describe("", () => { ); }); + it("removes account data events for devices after sign out", async () => { + const mobileDeviceClientInfo = new MatrixEvent({ + type: getClientInformationEventType(alicesMobileDevice.device_id), + content: { + name: 'test' + } + }) + // @ts-ignore setup mock + mockClient.store = { + // @ts-ignore setup mock + accountData: { + [mobileDeviceClientInfo.getType()]: mobileDeviceClientInfo + } + } + + mockClient.getDevices.mockResolvedValueOnce({ + devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice], + }).mockResolvedValueOnce({ + // refreshed devices after sign out + devices: [alicesDevice], + }); + + const { getByTestId, getByLabelText } = render(getComponent()); + + await act(async () => { + await flushPromises(); + }); + + expect(mockClient.deleteAccountData).not.toHaveBeenCalled(); + + fireEvent.click(getByTestId("current-session-menu")); + fireEvent.click(getByLabelText("Sign out of all other sessions (2)")); + await confirmSignout(getByTestId); + + // only called once for signed out device with account data event + expect(mockClient.deleteAccountData).toHaveBeenCalledTimes(1); + expect(mockClient.deleteAccountData).toHaveBeenCalledWith(mobileDeviceClientInfo.getType()); + }); + describe("other devices", () => { const interactiveAuthError = { httpStatus: 401, data: { flows: [{ stages: ["m.login.password"] }] } }; From fbe08e7d7a29fd8585f2b446d64393aca3743cda Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 9 Jan 2023 13:39:46 +1300 Subject: [PATCH 3/6] remove debug --- src/utils/device/clientInformation.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/utils/device/clientInformation.ts b/src/utils/device/clientInformation.ts index 827f7206e5c..c96fc3feb5c 100644 --- a/src/utils/device/clientInformation.ts +++ b/src/utils/device/clientInformation.ts @@ -74,7 +74,6 @@ export const pruneClientInformation = async (validDeviceIds: string[], matrixCli const orphanClientInformationEvents = Object.values(matrixClient.store.accountData).filter((event) => { if (event.getType().startsWith(clientInformationEventPrefix)) { const [, deviceId] = event.getType().split(clientInformationEventPrefix); - console.log('YO', deviceId, validDeviceIds) return deviceId && !validDeviceIds.includes(deviceId); } return false; From 1403b81206ff14f00e7221611afec89aae99d458 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 9 Jan 2023 14:37:00 +1300 Subject: [PATCH 4/6] strict --- .../views/settings/devices/useOwnDevices.ts | 9 ++------- src/utils/device/clientInformation.ts | 4 ++-- .../tabs/user/SessionManagerTab-test.tsx | 18 ++++++++++-------- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/components/views/settings/devices/useOwnDevices.ts b/src/components/views/settings/devices/useOwnDevices.ts index 14cdfc34a98..674639913da 100644 --- a/src/components/views/settings/devices/useOwnDevices.ts +++ b/src/components/views/settings/devices/useOwnDevices.ts @@ -116,8 +116,8 @@ export type DevicesState = { export const useOwnDevices = (): DevicesState => { const matrixClient = useContext(MatrixClientContext); - const currentDeviceId = matrixClient.getDeviceId(); - const userId = matrixClient.getUserId(); + const currentDeviceId = matrixClient.getDeviceId()!; + const userId = matrixClient.getSafeUserId(); const [devices, setDevices] = useState({}); const [pushers, setPushers] = useState([]); @@ -138,11 +138,6 @@ export const useOwnDevices = (): DevicesState => { const refreshDevices = useCallback(async () => { setIsLoadingDeviceList(true); try { - // realistically we should never hit this - // but it satisfies types - if (!userId) { - throw new Error("Cannot fetch devices without user id"); - } const devices = await fetchDevicesWithVerification(matrixClient, userId); setDevices(devices); diff --git a/src/utils/device/clientInformation.ts b/src/utils/device/clientInformation.ts index c96fc3feb5c..95f9832cc1e 100644 --- a/src/utils/device/clientInformation.ts +++ b/src/utils/device/clientInformation.ts @@ -52,7 +52,7 @@ export const recordClientInformation = async ( sdkConfig: IConfigOptions, platform: BasePlatform, ): Promise => { - const deviceId = matrixClient.getDeviceId(); + const deviceId = matrixClient.getDeviceId()!; const { brand } = sdkConfig; const version = await platform.getAppVersion(); const type = getClientInformationEventType(deviceId); @@ -88,7 +88,7 @@ export const pruneClientInformation = async (validDeviceIds: string[], matrixCli * Remove extra client information for current device */ export const removeClientInformation = async (matrixClient: MatrixClient): Promise => { - const deviceId = matrixClient.getDeviceId(); + const deviceId = matrixClient.getDeviceId()!; const type = getClientInformationEventType(deviceId); const clientInformation = getDeviceClientInformation(matrixClient, deviceId); diff --git a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx index 9e15cecb02e..95ec76129b7 100644 --- a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -676,20 +676,22 @@ describe("", () => { const mobileDeviceClientInfo = new MatrixEvent({ type: getClientInformationEventType(alicesMobileDevice.device_id), content: { - name: 'test' - } - }) + name: "test", + }, + }); // @ts-ignore setup mock mockClient.store = { // @ts-ignore setup mock accountData: { - [mobileDeviceClientInfo.getType()]: mobileDeviceClientInfo - } - } + [mobileDeviceClientInfo.getType()]: mobileDeviceClientInfo, + }, + }; - mockClient.getDevices.mockResolvedValueOnce({ + mockClient.getDevices + .mockResolvedValueOnce({ devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice], - }).mockResolvedValueOnce({ + }) + .mockResolvedValueOnce({ // refreshed devices after sign out devices: [alicesDevice], }); From a2f52d25fe5af8293e25b18026d8381c7c62323b Mon Sep 17 00:00:00 2001 From: Kerry Date: Tue, 10 Jan 2023 08:45:20 +1300 Subject: [PATCH 5/6] Update src/components/views/settings/devices/useOwnDevices.ts Co-authored-by: Michael Weimann --- src/components/views/settings/devices/useOwnDevices.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/settings/devices/useOwnDevices.ts b/src/components/views/settings/devices/useOwnDevices.ts index 674639913da..027da7d47b9 100644 --- a/src/components/views/settings/devices/useOwnDevices.ts +++ b/src/components/views/settings/devices/useOwnDevices.ts @@ -176,7 +176,7 @@ export const useOwnDevices = (): DevicesState => { // empty devices means devices have not been fetched yet // as there is always at least the current device if (deviceIds.length) { - pruneClientInformation(Object.keys(devices), matrixClient); + pruneClientInformation(deviceIds, matrixClient); } }, [devices, matrixClient]); From 77dd30a6e607b2451f7ba78ac10b2fe1145eeb7f Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 10 Jan 2023 08:56:49 +1300 Subject: [PATCH 6/6] improvements from review --- src/utils/device/clientInformation.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/utils/device/clientInformation.ts b/src/utils/device/clientInformation.ts index 95f9832cc1e..de247a57436 100644 --- a/src/utils/device/clientInformation.ts +++ b/src/utils/device/clientInformation.ts @@ -70,17 +70,15 @@ export const recordClientInformation = async ( * @param validDeviceIds - ids of current devices, * client information for devices NOT in this list will be removed */ -export const pruneClientInformation = async (validDeviceIds: string[], matrixClient: MatrixClient): Promise => { - const orphanClientInformationEvents = Object.values(matrixClient.store.accountData).filter((event) => { - if (event.getType().startsWith(clientInformationEventPrefix)) { - const [, deviceId] = event.getType().split(clientInformationEventPrefix); - return deviceId && !validDeviceIds.includes(deviceId); +export const pruneClientInformation = (validDeviceIds: string[], matrixClient: MatrixClient): void => { + Object.values(matrixClient.store.accountData).forEach((event) => { + if (!event.getType().startsWith(clientInformationEventPrefix)) { + return; + } + const [, deviceId] = event.getType().split(clientInformationEventPrefix); + if (deviceId && !validDeviceIds.includes(deviceId)) { + matrixClient.deleteAccountData(event.getType()); } - return false; - }); - - orphanClientInformationEvents.forEach((event) => { - matrixClient.deleteAccountData(event.getType()); }); };