From 66a9636ec5a94107be074b48bff85bd0d1afa211 Mon Sep 17 00:00:00 2001 From: Kerry Date: Mon, 10 Oct 2022 21:00:46 +0200 Subject: [PATCH 1/2] Device manager - remove client information events when disabling setting (#9384) * remove client information events when disabling setting * tweak naming Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> --- src/DeviceListener.ts | 34 +++++++++++++++------------ src/utils/device/clientInformation.ts | 18 ++++++++++++++ test/DeviceListener-test.ts | 34 ++++++++++++++++++++------- 3 files changed, 63 insertions(+), 23 deletions(-) diff --git a/src/DeviceListener.ts b/src/DeviceListener.ts index be440333e2..1f49c3b34d 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; @@ -90,7 +93,7 @@ export default class DeviceListener { ); this.dispatcherRef = dis.register(this.onAction); this.recheck(); - this.recordClientInformation(); + this.updateClientInformation(); } public stop() { @@ -216,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 @@ -368,25 +371,26 @@ export default class DeviceListener { this.shouldRecordClientInformation = !!newValue; - if (this.shouldRecordClientInformation && !prevValue) { - this.recordClientInformation(); + if (this.shouldRecordClientInformation !== prevValue) { + this.updateClientInformation(); } }; - private recordClientInformation = async () => { - if (!this.shouldRecordClientInformation) { - return; - } + private updateClientInformation = async () => { try { - await recordClientInformation( - MatrixClientPeg.get(), - SdkConfig.get(), - PlatformPeg.get(), - ); + if (this.shouldRecordClientInformation) { + await recordClientInformation( + MatrixClientPeg.get(), + SdkConfig.get(), + PlatformPeg.get(), + ); + } else { + await removeClientInformation(MatrixClientPeg.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 c31d1c690e..5c9b65b54b 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 21a0614d4a..8d0dd48570 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 4a98e26c4a1f4294e52c7135bb99f5aa51095326 Mon Sep 17 00:00:00 2001 From: Kerry Date: Tue, 11 Oct 2022 09:29:19 +0200 Subject: [PATCH 2/2] Device manager - UA parsing tweaks (#9382) * parseUA - use full client version * dont check for custom element model and os when device type is unknown * ignore OS version in browser based sessions --- src/utils/device/parseUserAgent.ts | 29 ++++++++++++++++-------- test/utils/device/parseUserAgent-test.ts | 25 +++++++++++--------- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/src/utils/device/parseUserAgent.ts b/src/utils/device/parseUserAgent.ts index 9d9d71e6f7..3eee617765 100644 --- a/src/utils/device/parseUserAgent.ts +++ b/src/utils/device/parseUserAgent.ts @@ -58,15 +58,16 @@ const getDeviceType = ( return DeviceType.Unknown; }; +interface CustomValues { + customDeviceModel?: string; + customDeviceOS?: string; +} /** * Some mobile model and OS strings are not recognised * by the UA parsing library * check they exist by hand */ -const checkForCustomValues = (userAgent: string): { - customDeviceModel?: string; - customDeviceOS?: string; -} => { +const checkForCustomValues = (userAgent: string): CustomValues => { if (userAgent.includes(BROWSER_KEYWORD)) { return {}; } @@ -97,13 +98,23 @@ export const parseUserAgent = (userAgent?: string): ExtendedDeviceInformation => const device = parser.getDevice(); const operatingSystem = parser.getOS(); - const deviceOperatingSystem = concatenateNameAndVersion(operatingSystem.name, operatingSystem.version); - const deviceModel = concatenateNameAndVersion(device.vendor, device.model); - const client = concatenateNameAndVersion(browser.name, browser.major || browser.version); - - const { customDeviceModel, customDeviceOS } = checkForCustomValues(userAgent); const deviceType = getDeviceType(userAgent, device, browser, operatingSystem); + // OSX versions are frozen at 10.15.17 in UA strings https://chromestatus.com/feature/5452592194781184 + // ignore OS version in browser based sessions + const shouldIgnoreOSVersion = deviceType === DeviceType.Web || deviceType === DeviceType.Desktop; + const deviceOperatingSystem = concatenateNameAndVersion( + operatingSystem.name, + shouldIgnoreOSVersion ? undefined : operatingSystem.version, + ); + const deviceModel = concatenateNameAndVersion(device.vendor, device.model); + const client = concatenateNameAndVersion(browser.name, browser.version); + + // only try to parse custom model and OS when device type is known + const { customDeviceModel, customDeviceOS } = deviceType !== DeviceType.Unknown + ? checkForCustomValues(userAgent) + : {} as CustomValues; + return { deviceType, deviceModel: deviceModel || customDeviceModel, diff --git a/test/utils/device/parseUserAgent-test.ts b/test/utils/device/parseUserAgent-test.ts index 9171a2a755..e4ae89d2e0 100644 --- a/test/utils/device/parseUserAgent-test.ts +++ b/test/utils/device/parseUserAgent-test.ts @@ -70,8 +70,8 @@ const DESKTOP_UA = [ "Mozilla/5.0 (Windows NT 10.0) AppleWebKit/537.36 (KHTML, like Gecko) ElementNightly/2022091301 Chrome/104.0.5112.102 Electron/20.1.1 Safari/537.36", ]; const DESKTOP_EXPECTED_RESULT = [ - makeDeviceExtendedInfo(DeviceType.Desktop, undefined, "Mac OS 10.15.7", "Electron", "20"), - makeDeviceExtendedInfo(DeviceType.Desktop, undefined, "Windows 10", "Electron", "20"), + makeDeviceExtendedInfo(DeviceType.Desktop, undefined, "Mac OS", "Electron", "20.1.1"), + makeDeviceExtendedInfo(DeviceType.Desktop, undefined, "Windows", "Electron", "20.1.1"), ]; const WEB_UA = [ @@ -88,16 +88,16 @@ const WEB_UA = [ ]; const WEB_EXPECTED_RESULT = [ - makeDeviceExtendedInfo(DeviceType.Web, undefined, "Mac OS 10.15.7", "Chrome", "104"), - makeDeviceExtendedInfo(DeviceType.Web, undefined, "Windows 10", "Chrome", "104"), - makeDeviceExtendedInfo(DeviceType.Web, undefined, "Mac OS 10.10", "Firefox", "39"), - makeDeviceExtendedInfo(DeviceType.Web, undefined, "Mac OS 10.10.2", "Safari", "8"), - makeDeviceExtendedInfo(DeviceType.Web, undefined, "Windows Vista", "Firefox", "40"), - makeDeviceExtendedInfo(DeviceType.Web, undefined, "Windows 10", "Edge", "12"), + makeDeviceExtendedInfo(DeviceType.Web, undefined, "Mac OS", "Chrome", "104.0.5112.102"), + makeDeviceExtendedInfo(DeviceType.Web, undefined, "Windows", "Chrome", "104.0.5112.102"), + makeDeviceExtendedInfo(DeviceType.Web, undefined, "Mac OS", "Firefox", "39.0"), + makeDeviceExtendedInfo(DeviceType.Web, undefined, "Mac OS", "Safari", "8.0.3"), + makeDeviceExtendedInfo(DeviceType.Web, undefined, "Windows", "Firefox", "40.0"), + makeDeviceExtendedInfo(DeviceType.Web, undefined, "Windows", "Edge", "12.246"), // using mobile browser - makeDeviceExtendedInfo(DeviceType.Web, "Apple iPad", "iOS 8.4.1", "Mobile Safari", "8"), - makeDeviceExtendedInfo(DeviceType.Web, "Apple iPhone", "iOS 8.4.1", "Mobile Safari", "8"), - makeDeviceExtendedInfo(DeviceType.Web, "Samsung SM-G973U", "Android 9", "Chrome", "69"), + makeDeviceExtendedInfo(DeviceType.Web, "Apple iPad", "iOS", "Mobile Safari", "8.0"), + makeDeviceExtendedInfo(DeviceType.Web, "Apple iPhone", "iOS", "Mobile Safari", "8.0"), + makeDeviceExtendedInfo(DeviceType.Web, "Samsung SM-G973U", "Android", "Chrome", "69.0.3497.100"), ]; @@ -106,6 +106,8 @@ const MISC_UA = [ "Curl Client/1.0", "banana", "", + // fluffy chat ios + "Dart/2.18 (dart:io)", ]; const MISC_EXPECTED_RESULT = [ @@ -113,6 +115,7 @@ const MISC_EXPECTED_RESULT = [ makeDeviceExtendedInfo(DeviceType.Unknown, undefined, undefined, undefined, undefined), makeDeviceExtendedInfo(DeviceType.Unknown, undefined, undefined, undefined, undefined), makeDeviceExtendedInfo(DeviceType.Unknown, undefined, undefined, undefined, undefined), + makeDeviceExtendedInfo(DeviceType.Unknown, undefined, undefined, undefined, undefined), ]; /* eslint-disable max-len */