From 10df9228e8614846d886a8158b45c15d0c4edc11 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 10 Oct 2022 16:57:31 +0200 Subject: [PATCH 1/2] remove client information events when disabling setting --- src/DeviceListener.ts | 26 +++++++++++--------- src/utils/device/clientInformation.ts | 18 ++++++++++++++ test/DeviceListener-test.ts | 34 ++++++++++++++++++++------- 3 files changed, 59 insertions(+), 19 deletions(-) diff --git a/src/DeviceListener.ts b/src/DeviceListener.ts index be440333e27..9e06091172c 100644 --- a/src/DeviceListener.ts +++ b/src/DeviceListener.ts @@ -42,7 +42,10 @@ import { Action } from "./dispatcher/actions"; import { isLoggedIn } from "./utils/login"; import SdkConfig from "./SdkConfig"; import PlatformPeg from "./PlatformPeg"; -import { recordClientInformation } from "./utils/device/clientInformation"; +import { + recordClientInformation, + removeClientInformation, +} from "./utils/device/clientInformation"; import SettingsStore, { CallbackFn } from "./settings/SettingsStore"; const KEY_BACKUP_POLL_INTERVAL = 5 * 60 * 1000; @@ -368,25 +371,26 @@ export default class DeviceListener { this.shouldRecordClientInformation = !!newValue; - if (this.shouldRecordClientInformation && !prevValue) { + if (this.shouldRecordClientInformation !== prevValue) { this.recordClientInformation(); } }; private recordClientInformation = async () => { - if (!this.shouldRecordClientInformation) { - return; - } try { - await recordClientInformation( - MatrixClientPeg.get(), - SdkConfig.get(), - PlatformPeg.get(), - ); + if (!this.shouldRecordClientInformation) { + await removeClientInformation(MatrixClientPeg.get()); + } else { + await recordClientInformation( + MatrixClientPeg.get(), + SdkConfig.get(), + PlatformPeg.get(), + ); + } } catch (error) { // this is a best effort operation // log the error without rethrowing - logger.error('Failed to record client information', error); + logger.error('Failed to update client information', error); } }; } diff --git a/src/utils/device/clientInformation.ts b/src/utils/device/clientInformation.ts index c31d1c690ea..5c9b65b54bc 100644 --- a/src/utils/device/clientInformation.ts +++ b/src/utils/device/clientInformation.ts @@ -65,6 +65,24 @@ export const recordClientInformation = async ( }); }; +/** + * Remove extra client information + * @todo(kerrya) revisit after MSC3391: account data deletion is done + * (PSBE-12) + */ +export const removeClientInformation = async ( + matrixClient: MatrixClient, +): Promise => { + const deviceId = matrixClient.getDeviceId(); + const type = getClientInformationEventType(deviceId); + const clientInformation = getDeviceClientInformation(matrixClient, deviceId); + + // if a non-empty client info event exists, overwrite to remove the content + if (clientInformation.name || clientInformation.version || clientInformation.url) { + await matrixClient.setAccountData(type, {}); + } +}; + const sanitizeContentString = (value: unknown): string | undefined => value && typeof value === 'string' ? value : undefined; diff --git a/test/DeviceListener-test.ts b/test/DeviceListener-test.ts index 21a0614d4aa..8d0dd48570e 100644 --- a/test/DeviceListener-test.ts +++ b/test/DeviceListener-test.ts @@ -17,7 +17,7 @@ limitations under the License. import { EventEmitter } from "events"; import { mocked } from "jest-mock"; -import { Room } from "matrix-js-sdk/src/matrix"; +import { MatrixEvent, Room } from "matrix-js-sdk/src/matrix"; import { logger } from "matrix-js-sdk/src/logger"; import DeviceListener from "../src/DeviceListener"; @@ -66,6 +66,7 @@ class MockClient extends EventEmitter { getClientWellKnown = jest.fn(); getDeviceId = jest.fn().mockReturnValue(deviceId); setAccountData = jest.fn(); + getAccountData = jest.fn(); } const mockDispatcher = mocked(dis); const flushPromises = async () => await new Promise(process.nextTick); @@ -138,7 +139,7 @@ describe('DeviceListener', () => { await createAndStart(); expect(errorLogSpy).toHaveBeenCalledWith( - 'Failed to record client information', + 'Failed to update client information', error, ); }); @@ -161,19 +162,39 @@ describe('DeviceListener', () => { }); describe('when device client information feature is disabled', () => { + const clientInfoEvent = new MatrixEvent({ type: `io.element.matrix_client_information.${deviceId}`, + content: { name: 'hello' }, + }); + const emptyClientInfoEvent = new MatrixEvent({ type: `io.element.matrix_client_information.${deviceId}` }); beforeEach(() => { jest.spyOn(SettingsStore, 'getValue').mockReturnValue(false); + + mockClient.getAccountData.mockReturnValue(undefined); }); it('does not save client information on start', async () => { await createAndStart(); - expect(mockClient.setAccountData).not.toHaveBeenCalledWith( + expect(mockClient.setAccountData).not.toHaveBeenCalled(); + }); + + it('removes client information on start if it exists', async () => { + mockClient.getAccountData.mockReturnValue(clientInfoEvent); + await createAndStart(); + + expect(mockClient.setAccountData).toHaveBeenCalledWith( `io.element.matrix_client_information.${deviceId}`, - { name: 'Element', url: 'localhost', version: '1.2.3' }, + {}, ); }); + it('does not try to remove client info event that are already empty', async () => { + mockClient.getAccountData.mockReturnValue(emptyClientInfoEvent); + await createAndStart(); + + expect(mockClient.setAccountData).not.toHaveBeenCalled(); + }); + it('does not save client information on logged in action', async () => { const instance = await createAndStart(); @@ -182,10 +203,7 @@ describe('DeviceListener', () => { await flushPromises(); - expect(mockClient.setAccountData).not.toHaveBeenCalledWith( - `io.element.matrix_client_information.${deviceId}`, - { name: 'Element', url: 'localhost', version: '1.2.3' }, - ); + expect(mockClient.setAccountData).not.toHaveBeenCalled(); }); it('saves client information after setting is enabled', async () => { From d53fdc73b6f1123ca587e7131115295451dcb2b2 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 10 Oct 2022 16:59:42 +0200 Subject: [PATCH 2/2] tweak naming --- src/DeviceListener.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/DeviceListener.ts b/src/DeviceListener.ts index 9e06091172c..1f49c3b34d6 100644 --- a/src/DeviceListener.ts +++ b/src/DeviceListener.ts @@ -93,7 +93,7 @@ export default class DeviceListener { ); this.dispatcherRef = dis.register(this.onAction); this.recheck(); - this.recordClientInformation(); + this.updateClientInformation(); } public stop() { @@ -219,7 +219,7 @@ export default class DeviceListener { private onAction = ({ action }: ActionPayload) => { if (action !== Action.OnLoggedIn) return; this.recheck(); - this.recordClientInformation(); + this.updateClientInformation(); }; // The server doesn't tell us when key backup is set up, so we poll @@ -372,20 +372,20 @@ export default class DeviceListener { this.shouldRecordClientInformation = !!newValue; if (this.shouldRecordClientInformation !== prevValue) { - this.recordClientInformation(); + this.updateClientInformation(); } }; - private recordClientInformation = async () => { + private updateClientInformation = async () => { try { - if (!this.shouldRecordClientInformation) { - await removeClientInformation(MatrixClientPeg.get()); - } else { + if (this.shouldRecordClientInformation) { await recordClientInformation( MatrixClientPeg.get(), SdkConfig.get(), PlatformPeg.get(), ); + } else { + await removeClientInformation(MatrixClientPeg.get()); } } catch (error) { // this is a best effort operation