Device manager - prune client information events after remote sign out (matrix-org#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 <michaelw@matrix.org>

* improvements from review

Co-authored-by: Michael Weimann <michaelw@matrix.org>

Author: 2 people

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<DevicesState["devices"]>({});
     const [pushers, setPushers] = useState<DevicesState["pushers"]>([]);
@@ -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();

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<void> => {
-    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<void> => {
-    const deviceId = matrixClient.getDeviceId();
+    const deviceId = matrixClient.getDeviceId()!;
     const type = getClientInformationEventType(deviceId);
     const clientInformation = getDeviceClientInformation(matrixClient, deviceId);
 

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("<SessionManagerTab />", () => {
         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("<SessionManagerTab />", () => {
             ],
         });
 
+        // @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("<SessionManagerTab />", () => {
             );
         });
 
+        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"] }] } };