From b1c32995c67976e73e66094ecf5b671820df52fc Mon Sep 17 00:00:00 2001 From: Kerry Date: Tue, 10 Jan 2023 19:01:50 +1300 Subject: [PATCH] Device manager - prune client information events after remote sign out (#9874) * prune client infromation events from old devices * test client data pruning * remove debug * strict * Update src/components/views/settings/devices/useOwnDevices.ts Co-authored-by: Michael Weimann * improvements from review Co-authored-by: Michael Weimann --- .../views/settings/devices/useOwnDevices.ts | 20 ++++---- src/utils/device/clientInformation.ts | 29 +++++++++--- .../tabs/user/SessionManagerTab-test.tsx | 46 +++++++++++++++++++ 3 files changed, 80 insertions(+), 15 deletions(-) diff --git a/src/components/views/settings/devices/useOwnDevices.ts b/src/components/views/settings/devices/useOwnDevices.ts index 58f6cef43d..027da7d47b 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"; @@ -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); @@ -176,6 +171,15 @@ export const useOwnDevices = (): DevicesState => { refreshDevices(); }, [refreshDevices]); + useEffect(() => { + 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(deviceIds, matrixClient); + } + }, [devices, matrixClient]); + 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 e97135ab1f..de247a5743 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 @@ -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); @@ -66,12 +66,27 @@ 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 = (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()); + } + }); +}; + +/** + * 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 a90c37c388..95ec76129b 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,6 +88,7 @@ describe("", () => { generateClientSecret: jest.fn(), setDeviceDetails: jest.fn(), getAccountData: jest.fn(), + deleteAccountData: jest.fn(), doesServerSupportUnstableFeature: jest.fn().mockResolvedValue(true), getPushers: jest.fn(), setPusher: jest.fn(), @@ -182,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({ @@ -667,6 +672,47 @@ 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"] }] } };