From 2f9e633fad93b66d7b925c7cd3d8e8246d760e29 Mon Sep 17 00:00:00 2001 From: Lorenzo Blasa Date: Fri, 2 Jun 2023 03:59:15 -0700 Subject: [PATCH] Medium refactoring Summary: Simplifies medium usage. Clients report this value as an integer. Internally, we transform this integer as type (a set of valid strings). Instead of transform this value in different places, do it once when the client query is received. Reviewed By: antonk52 Differential Revision: D46358024 fbshipit-source-id: ecd2b6c6ccbe7c38787a89d4e2f81930c7b91864 --- desktop/flipper-common/src/server-types.tsx | 2 ++ .../src/FlipperServerImpl.tsx | 2 +- .../src/comms/BrowserServerWebSocket.tsx | 2 +- .../src/comms/ServerAdapter.tsx | 9 ++--- .../src/comms/ServerController.tsx | 33 +++++++++---------- .../src/comms/ServerWebSocket.tsx | 2 +- .../src/comms/Utilities.tsx | 30 +++++++++++++++-- .../__tests__/BrowserServerWebSocket.node.tsx | 4 +-- .../__tests__/SecureServerWebSocket.node.tsx | 3 +- .../comms/__tests__/ServerWebSocket.node.tsx | 3 +- .../src/utils/CertificateProvider.tsx | 3 +- .../flipper-server-core/src/utils/tracker.tsx | 4 +-- .../createMockFlipperWithPlugin.node.tsx.snap | 1 + .../src/__tests__/test-utils/MockFlipper.tsx | 1 + .../src/utils/__tests__/exportData.node.tsx | 26 +++++++++++++-- 15 files changed, 84 insertions(+), 41 deletions(-) diff --git a/desktop/flipper-common/src/server-types.tsx b/desktop/flipper-common/src/server-types.tsx index 291d66ce5..027418632 100644 --- a/desktop/flipper-common/src/server-types.tsx +++ b/desktop/flipper-common/src/server-types.tsx @@ -30,6 +30,7 @@ import {LoggerInfo} from './utils/Logger'; // Since flipper-plugin however is currently shared among server, client and defines a lot of base types, leaving it here for now. export type FlipperServerType = 'embedded' | 'external'; +export type CertificateExchangeMedium = 'FS_ACCESS' | 'WWW' | 'NONE'; export type FlipperServerState = | 'pending' @@ -87,6 +88,7 @@ export type ClientQuery = { readonly device: string; readonly device_id: string; readonly sdk_version?: number; + medium: CertificateExchangeMedium; rsocket?: boolean; }; diff --git a/desktop/flipper-server-core/src/FlipperServerImpl.tsx b/desktop/flipper-server-core/src/FlipperServerImpl.tsx index 176c5ab3d..6aca92a11 100644 --- a/desktop/flipper-server-core/src/FlipperServerImpl.tsx +++ b/desktop/flipper-server-core/src/FlipperServerImpl.tsx @@ -11,7 +11,6 @@ import './utils/macCa'; import './utils/fetch-polyfill'; import EventEmitter from 'events'; import {ServerController} from './comms/ServerController'; -import {CertificateExchangeMedium} from './utils/CertificateProvider'; import {AndroidDeviceManager} from './devices/android/androidDeviceManager'; import {IOSDeviceManager} from './devices/ios/iOSDeviceManager'; import metroDevice from './devices/metro/metroDeviceManager'; @@ -26,6 +25,7 @@ import { Logger, FlipperServerExecOptions, DeviceDebugData, + CertificateExchangeMedium, } from 'flipper-common'; import {ServerDevice} from './devices/ServerDevice'; import {Base64} from 'js-base64'; diff --git a/desktop/flipper-server-core/src/comms/BrowserServerWebSocket.tsx b/desktop/flipper-server-core/src/comms/BrowserServerWebSocket.tsx index 60a6d2fcc..ad2f1a5b9 100644 --- a/desktop/flipper-server-core/src/comms/BrowserServerWebSocket.tsx +++ b/desktop/flipper-server-core/src/comms/BrowserServerWebSocket.tsx @@ -144,7 +144,7 @@ class BrowserServerWebSocket extends SecureServerWebSocket { if (!parsedBaseQuery) { return; } - return {...parsedBaseQuery, medium: 3}; + return {...parsedBaseQuery, medium: 'NONE'}; } protected verifyClient(): ws.VerifyClientCallbackSync { diff --git a/desktop/flipper-server-core/src/comms/ServerAdapter.tsx b/desktop/flipper-server-core/src/comms/ServerAdapter.tsx index bcecdb99d..1f86db3cc 100644 --- a/desktop/flipper-server-core/src/comms/ServerAdapter.tsx +++ b/desktop/flipper-server-core/src/comms/ServerAdapter.tsx @@ -7,9 +7,7 @@ * @format */ -import {CertificateExchangeMedium} from '../utils/CertificateProvider'; import {ClientConnection} from './ClientConnection'; -import {transformCertificateExchangeMediumToType} from './Utilities'; import { ClientDescription, ClientQuery, @@ -30,8 +28,7 @@ export type ClientCsrQuery = { * SecureClientQuery combines a ClientQuery with * ClientCsrQuery. It also adds medium information. */ -export type SecureClientQuery = ClientQuery & - ClientCsrQuery & {medium: 1 /*FS*/ | 2 /*WWW*/ | 3 /*NONE*/ | undefined}; +export type SecureClientQuery = ClientQuery & ClientCsrQuery; /** * Defines an interface for events triggered by a running server interacting @@ -75,7 +72,6 @@ export interface ServerEventsListener { unsanitizedCSR: string, clientQuery: ClientQuery, appDirectory: string, - medium: CertificateExchangeMedium, ): Promise<{deviceId: string}>; /** * A secure connection has been established with a validated client. @@ -161,7 +157,7 @@ abstract class ServerAdapter { if (message.method === 'signCertificate') { console.debug('CSR received from device', 'server'); - const {csr, destination, medium} = message; + const {csr, destination} = message; console.info( `[conn] Starting certificate exchange: ${clientQuery.app} on ${clientQuery.device}`, @@ -171,7 +167,6 @@ abstract class ServerAdapter { csr, clientQuery, destination, - transformCertificateExchangeMediumToType(medium), ); console.info( diff --git a/desktop/flipper-server-core/src/comms/ServerController.tsx b/desktop/flipper-server-core/src/comms/ServerController.tsx index 4d04fc376..63541dd54 100644 --- a/desktop/flipper-server-core/src/comms/ServerController.tsx +++ b/desktop/flipper-server-core/src/comms/ServerController.tsx @@ -7,7 +7,6 @@ * @format */ -import {CertificateExchangeMedium} from '../utils/CertificateProvider'; import { ClientDescription, ClientQuery, @@ -27,7 +26,6 @@ import { appNameWithUpdateHint, assertNotNull, cloneClientQuerySafeForLogging, - transformCertificateExchangeMediumToType, } from './Utilities'; import ServerAdapter, { SecureClientQuery, @@ -182,7 +180,7 @@ export class ServerController medium, rsocket, } = clientQuery; - const transformedMedium = transformCertificateExchangeMediumToType(medium); + console.info( `[conn] Connection established: ${app} on ${device_id}. Medium ${medium}. CSR: ${csr_path}`, cloneClientQuerySafeForLogging(clientQuery), @@ -204,7 +202,7 @@ export class ServerController device, device_id, sdk_version, - medium: transformedMedium, + medium, rsocket, }, {csr, csr_path}, @@ -233,10 +231,13 @@ export class ServerController ...timestamp, }); - tracker.track( - 'app-connection-secure-attempt', - (({csr, ...o}) => o)(clientQuery), - ); + tracker.track('app-connection-secure-attempt', { + app: clientQuery.app, + os: clientQuery.os, + device: clientQuery.device, + device_id: clientQuery.device_id, + medium: clientQuery.medium, + }); const {os, app, device_id} = clientQuery; // without these checks, the user might see a connection timeout error instead, which would be much harder to track down @@ -260,16 +261,13 @@ export class ServerController clearTimeout(timeout); } - const transformedMedium = transformCertificateExchangeMediumToType( - clientQuery.medium, - ); - if (transformedMedium === 'WWW' || transformedMedium === 'NONE') { + if (clientQuery.medium === 'WWW' || clientQuery.medium === 'NONE') { this.flipperServer.registerDevice( new DummyDevice( this.flipperServer, clientQuery.device_id, clientQuery.app + - (transformedMedium === 'WWW' ? ' Server Exchanged' : ''), + (clientQuery.medium === 'WWW' ? ' Server Exchanged' : ''), clientQuery.os, ), ); @@ -299,7 +297,6 @@ export class ServerController unsanitizedCSR: string, clientQuery: ClientQuery, appDirectory: string, - medium: CertificateExchangeMedium, ): Promise<{deviceId: string}> { let certificateProvider: CertificateProvider; switch (clientQuery.os) { @@ -318,7 +315,7 @@ export class ServerController ); certificateProvider = this.flipperServer.ios.certificateProvider; - if (medium === 'WWW') { + if (clientQuery.medium === 'WWW') { certificateProvider = new WWWCertificateProvider( this.flipperServer.keytarManager, ); @@ -339,7 +336,7 @@ export class ServerController } } - certificateProvider.verifyMedium(medium); + certificateProvider.verifyMedium(clientQuery.medium); return new Promise((resolve, reject) => { reportPlatformFailures( @@ -367,7 +364,7 @@ export class ServerController setTimeout(() => { this.emit('client-unresponsive-error', { client, - medium, + medium: clientQuery.medium, deviceID: response.deviceId, }); }, 30 * 1000), @@ -428,7 +425,7 @@ export class ServerController */ async addConnection( connection: ClientConnection, - query: ClientQuery & {medium: CertificateExchangeMedium}, + query: ClientQuery, csrQuery: ClientCsrQuery, silentReplace?: boolean, ): Promise { diff --git a/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx b/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx index 6960191eb..ce2fa6694 100644 --- a/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx +++ b/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx @@ -99,7 +99,7 @@ class ServerWebSocket extends ServerAdapter { }); try { - this.onConnection(ws, request); + this.onConnection(ws, request); // insecure connection, with medium. } catch (error) { // TODO: Investigate if we need to close the socket in the `error` listener // DRI: @aigoncharov diff --git a/desktop/flipper-server-core/src/comms/Utilities.tsx b/desktop/flipper-server-core/src/comms/Utilities.tsx index b83ce0e98..3c035ccff 100644 --- a/desktop/flipper-server-core/src/comms/Utilities.tsx +++ b/desktop/flipper-server-core/src/comms/Utilities.tsx @@ -7,9 +7,13 @@ * @format */ -import {ClientQuery, DeviceOS, ResponseMessage} from 'flipper-common'; +import { + CertificateExchangeMedium, + ClientQuery, + DeviceOS, + ResponseMessage, +} from 'flipper-common'; import {ParsedUrlQuery} from 'querystring'; -import {CertificateExchangeMedium} from '../utils/CertificateProvider'; import {SecureClientQuery} from './ServerAdapter'; /** @@ -126,11 +130,23 @@ export function parseClientQuery( return; } + let medium: number | undefined; + if (typeof query.medium === 'string') { + medium = parseInt(query.medium, 10); + } else if (typeof query.medium === 'number') { + medium = query.medium; + } + + if (medium !== undefined && (medium < 1 || medium > 3)) { + throw new Error('Unsupported exchange medium: ' + medium); + } + const clientQuery: ClientQuery = { device_id, device, app, os, + medium: transformCertificateExchangeMediumToType(medium), }; if (typeof query.sdk_version === 'string') { @@ -178,11 +194,19 @@ export function parseSecureClientQuery( let medium: number | undefined; if (typeof query.medium === 'string') { medium = parseInt(query.medium, 10); + } else if (typeof query.medium === 'number') { + medium = query.medium; } + if (medium !== undefined && (medium < 1 || medium > 3)) { throw new Error('Unsupported exchange medium: ' + medium); } - return {...clientQuery, csr, csr_path, medium: medium as any}; + return { + ...clientQuery, + csr, + csr_path, + medium: transformCertificateExchangeMediumToType(medium), + }; } export function cloneClientQuerySafeForLogging(clientQuery: SecureClientQuery) { diff --git a/desktop/flipper-server-core/src/comms/__tests__/BrowserServerWebSocket.node.tsx b/desktop/flipper-server-core/src/comms/__tests__/BrowserServerWebSocket.node.tsx index 6dbc602e6..6a99765c9 100644 --- a/desktop/flipper-server-core/src/comms/__tests__/BrowserServerWebSocket.node.tsx +++ b/desktop/flipper-server-core/src/comms/__tests__/BrowserServerWebSocket.node.tsx @@ -81,7 +81,7 @@ describe('BrowserServerWebSocket', () => { os, app, sdk_version: sdkVersion, - medium: 3, + medium: 'NONE', }; expect(mockSEListener.onConnectionAttempt).toBeCalledWith( expectedClientQuery, @@ -183,7 +183,7 @@ describe('BrowserServerWebSocket', () => { os: 'MacOS', app: device, sdk_version: 4, - medium: 3, + medium: 'NONE', }; expect(mockSEListener.onConnectionAttempt).toBeCalledWith( expectedClientQuery, diff --git a/desktop/flipper-server-core/src/comms/__tests__/SecureServerWebSocket.node.tsx b/desktop/flipper-server-core/src/comms/__tests__/SecureServerWebSocket.node.tsx index 1e4c309c7..b09b33bfa 100644 --- a/desktop/flipper-server-core/src/comms/__tests__/SecureServerWebSocket.node.tsx +++ b/desktop/flipper-server-core/src/comms/__tests__/SecureServerWebSocket.node.tsx @@ -18,6 +18,7 @@ import WebSocket from 'ws'; import SecureServerWebSocket from '../SecureServerWebSocket'; import {SecureClientQuery} from '../ServerAdapter'; +import {transformCertificateExchangeMediumToType} from '../Utilities'; import WebSocketClientConnection from '../WebSocketClientConnection'; import {createMockSEListener, WSMessageAccumulator} from './utils'; @@ -80,7 +81,7 @@ describe('SecureServerWebSocket', () => { sdk_version: sdkVersion, csr, csr_path: csrPath, - medium, + medium: transformCertificateExchangeMediumToType(medium), }; expect(mockSEListener.onSecureConnectionAttempt).toBeCalledWith( expectedClientQuery, diff --git a/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx b/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx index 8b2c57a6e..2eaac0f74 100644 --- a/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx +++ b/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx @@ -45,7 +45,7 @@ describe('ServerWebSocket', () => { expect(mockSEListener.onConnectionAttempt).toBeCalledTimes(0); wsClient = new WebSocket( - `ws://localhost:${port}?device_id=${deviceId}&device=${device}&app=${app}&os=${os}&sdk_version=${sdkVersion}`, + `ws://localhost:${port}?device_id=${deviceId}&device=${device}&app=${app}&os=${os}&sdk_version=${sdkVersion}&medium=2`, ); const receivedMessages = new WSMessageAccumulator(); await new Promise((resolve, reject) => { @@ -60,6 +60,7 @@ describe('ServerWebSocket', () => { os, app, sdk_version: sdkVersion, + medium: 'WWW', }; expect(mockSEListener.onConnectionAttempt).toBeCalledWith( expectedClientQuery, diff --git a/desktop/flipper-server-core/src/utils/CertificateProvider.tsx b/desktop/flipper-server-core/src/utils/CertificateProvider.tsx index 627f12960..e5f6fdff3 100644 --- a/desktop/flipper-server-core/src/utils/CertificateProvider.tsx +++ b/desktop/flipper-server-core/src/utils/CertificateProvider.tsx @@ -7,6 +7,7 @@ * @format */ +import {CertificateExchangeMedium} from 'flipper-common'; import { deviceCAcertFile, deviceClientCertFile, @@ -16,8 +17,6 @@ import { getCACertificate, } from './certificateUtils'; -export type CertificateExchangeMedium = 'FS_ACCESS' | 'WWW' | 'NONE'; - export default abstract class CertificateProvider { abstract medium: CertificateExchangeMedium; abstract name: string; diff --git a/desktop/flipper-server-core/src/utils/tracker.tsx b/desktop/flipper-server-core/src/utils/tracker.tsx index 57f49ee0f..8d69cb527 100644 --- a/desktop/flipper-server-core/src/utils/tracker.tsx +++ b/desktop/flipper-server-core/src/utils/tracker.tsx @@ -7,14 +7,14 @@ * @format */ -import {getLogger} from 'flipper-common'; +import {getLogger, CertificateExchangeMedium} from 'flipper-common'; type AppConnectionPayload = { app: string; os: string; device: string; device_id: string; - medium?: number | undefined; + medium?: CertificateExchangeMedium; }; type AppConnectionCertificateExchangePayload = AppConnectionPayload & { diff --git a/desktop/flipper-ui-core/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap b/desktop/flipper-ui-core/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap index 54f472fd1..6557297c7 100644 --- a/desktop/flipper-ui-core/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap +++ b/desktop/flipper-ui-core/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap @@ -9,6 +9,7 @@ exports[`can create a Fake flipper with legacy wrapper 1`] = ` "app": "TestApp", "device": "MockAndroidDevice", "device_id": "serial", + "medium": "NONE", "os": "Android", "sdk_version": 4, }, diff --git a/desktop/flipper-ui-core/src/__tests__/test-utils/MockFlipper.tsx b/desktop/flipper-ui-core/src/__tests__/test-utils/MockFlipper.tsx index decc96ce7..ebd2cc340 100644 --- a/desktop/flipper-ui-core/src/__tests__/test-utils/MockFlipper.tsx +++ b/desktop/flipper-ui-core/src/__tests__/test-utils/MockFlipper.tsx @@ -180,6 +180,7 @@ export default class MockFlipper { device: device.title, device_id: device.serial, sdk_version: 4, + medium: 'NONE', }; const id = buildClientId({ app: query.app, diff --git a/desktop/flipper-ui-core/src/utils/__tests__/exportData.node.tsx b/desktop/flipper-ui-core/src/utils/__tests__/exportData.node.tsx index db72eac5f..d1529ff43 100644 --- a/desktop/flipper-ui-core/src/utils/__tests__/exportData.node.tsx +++ b/desktop/flipper-ui-core/src/utils/__tests__/exportData.node.tsx @@ -105,7 +105,13 @@ function generateClientFromClientWithSalt( const identifier = generateClientIdentifierWithSalt(client.id, salt); return { id: identifier, - query: {app, os, device, device_id: salt + '-' + device_id}, + query: { + app, + os, + device, + device_id: salt + '-' + device_id, + medium: client.query.medium, + }, }; } @@ -114,7 +120,7 @@ function generateClientFromDevice(device: Device, app: string): ClientExport { const identifier = generateClientIdentifier(device, app); return { id: identifier, - query: {app, os, device: deviceType, device_id: serial}, + query: {app, os, device: deviceType, device_id: serial, medium: 'NONE'}, }; } @@ -149,6 +155,7 @@ test('test generateClientFromClientWithSalt helper function', () => { os: 'iOS', device: 'emulator', device_id: 'salt-serial', + medium: 'NONE', }, }); expect(client).toEqual({ @@ -158,6 +165,7 @@ test('test generateClientFromClientWithSalt helper function', () => { os: 'iOS', device: 'emulator', device_id: 'serial', + medium: 'NONE', }, }); }); @@ -178,6 +186,7 @@ test('test generateClientFromDevice helper function', () => { os: 'iOS', device: 'emulator', device_id: 'serial', + medium: 'NONE', }, }); }); @@ -727,6 +736,7 @@ test('test determinePluginsToProcess for mutilple clients having plugins present os: 'iOS', device: 'TestiPhone', device_id: 'serial1', + medium: 'NONE', }, null, logger, @@ -742,6 +752,7 @@ test('test determinePluginsToProcess for mutilple clients having plugins present os: 'iOS', device: 'TestiPhone', device_id: 'serial1', + medium: 'NONE', }, null, logger, @@ -757,6 +768,7 @@ test('test determinePluginsToProcess for mutilple clients having plugins present os: 'iOS', device: 'TestiPhone', device_id: 'serial1', + medium: 'NONE', }, null, logger, @@ -816,6 +828,7 @@ test('test determinePluginsToProcess for no selected plugin present in any clien os: 'iOS', device: 'TestiPhone', device_id: 'serial1', + medium: 'NONE', }, null, logger, @@ -831,6 +844,7 @@ test('test determinePluginsToProcess for no selected plugin present in any clien os: 'iOS', device: 'TestiPhone', device_id: 'serial1', + medium: 'NONE', }, null, logger, @@ -873,6 +887,7 @@ test('test determinePluginsToProcess for multiple clients on same device', async os: 'iOS', device: 'TestiPhone', device_id: 'serial1', + medium: 'NONE', }, null, logger, @@ -888,6 +903,7 @@ test('test determinePluginsToProcess for multiple clients on same device', async os: 'iOS', device: 'TestiPhone', device_id: 'serial1', + medium: 'NONE', }, null, logger, @@ -935,6 +951,7 @@ test('test determinePluginsToProcess for multiple clients on different device', os: 'iOS', device: 'TestiPhone', device_id: 'serial1', + medium: 'NONE', }, null, logger, @@ -950,6 +967,7 @@ test('test determinePluginsToProcess for multiple clients on different device', os: 'iOS', device: 'TestiPhone', device_id: 'serial1', + medium: 'NONE', }, null, logger, @@ -965,6 +983,7 @@ test('test determinePluginsToProcess for multiple clients on different device', os: 'iOS', device: 'TestiPhone', device_id: 'serial2', + medium: 'NONE', }, null, logger, @@ -980,6 +999,7 @@ test('test determinePluginsToProcess for multiple clients on different device', os: 'iOS', device: 'TestiPhone', device_id: 'serial2', + medium: 'NONE', }, null, logger, @@ -1051,6 +1071,7 @@ test('test determinePluginsToProcess to ignore archived clients', async () => { os: 'iOS', device: 'TestiPhone', device_id: 'serial', + medium: 'NONE', }, null, logger, @@ -1066,6 +1087,7 @@ test('test determinePluginsToProcess to ignore archived clients', async () => { os: 'iOS', device: 'TestiPhone', device_id: 'serial-archived', + medium: 'NONE', }, null, logger,