From 80f48b444c536b8aef68bc457a8955b0bba4aefa Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Fri, 10 Sep 2021 07:01:41 -0700 Subject: [PATCH] Fix pending connections for websocket browser connections Summary: Connections from VSCode and Kite would remain forever pending because they don't go through the secure connection handler. This diff fixes that. Also removed the separate event that existed for that, since registering a new client is already a 'success' signal, so it doesn't need a separate event. It turned out that the VSCode pending connection is actually correct, as it never handles the `getPlugins` event, so apparently the handling is broken. Added timeouts to guard against that as well. Applied several code simplications as well. Introduced an explicit cert exchange medium 'NONE' so that in code it is a bit clearer where CSR negotiation is supposed to happen. Changelog: Fixed an issue where Kite / Unity apps didn't connect anymore Reviewed By: timur-valiev Differential Revision: D30866301 fbshipit-source-id: 8bd214fd9eebcd9a7583f1b44ee283883002f62e --- desktop/app/src/Client.tsx | 24 +++++++++----- desktop/app/src/dispatcher/flipperServer.tsx | 2 +- desktop/app/src/reducers/connections.tsx | 33 +++---------------- .../sandy-chrome/appinspect/AppSelector.tsx | 4 +-- desktop/app/src/server/FlipperServer.tsx | 10 ------ .../app/src/server/comms/ServerAdapter.tsx | 2 +- .../app/src/server/comms/ServerController.tsx | 29 ++++++++-------- .../app/src/server/comms/ServerWebSocket.tsx | 6 ++-- .../server/comms/ServerWebSocketBrowser.tsx | 3 +- desktop/app/src/server/comms/Utilities.tsx | 16 +++++---- .../src/server/utils/CertificateProvider.tsx | 2 +- 11 files changed, 56 insertions(+), 75 deletions(-) diff --git a/desktop/app/src/Client.tsx b/desktop/app/src/Client.tsx index 80fb50cc2..aa210c0d4 100644 --- a/desktop/app/src/Client.tsx +++ b/desktop/app/src/Client.tsx @@ -26,7 +26,12 @@ import {processMessagesLater} from './utils/messageQueue'; import {emitBytesReceived} from './dispatcher/tracking'; import {debounce} from 'lodash'; import {batch} from 'react-redux'; -import {createState, _SandyPluginInstance, getFlipperLib} from 'flipper-plugin'; +import { + createState, + _SandyPluginInstance, + getFlipperLib, + timeout, +} from 'flipper-plugin'; import {freeze} from 'immer'; import GK from './fb-stubs/GK'; import {message} from 'antd'; @@ -230,9 +235,10 @@ export default class Client extends EventEmitter { // get the supported plugins async loadPlugins(): Promise { - const {plugins} = await this.rawCall<{plugins: Plugins}>( - 'getPlugins', - false, + const {plugins} = await timeout( + 30 * 1000, + this.rawCall<{plugins: Plugins}>('getPlugins', false), + 'Fetch plugin timeout for ' + this.id, ); this.plugins = new Set(plugins); return plugins; @@ -307,10 +313,12 @@ export default class Client extends EventEmitter { if (this.sdkVersion < 4) { return []; } - return this.rawCall<{plugins: PluginsArr}>( - 'getBackgroundPlugins', - false, - ).then((data) => data.plugins); + const data = await timeout( + 30 * 1000, + this.rawCall<{plugins: PluginsArr}>('getBackgroundPlugins', false), + 'Fetch background plugins timeout for ' + this.id, + ); + return data.plugins; } // get the plugins, and update the UI diff --git a/desktop/app/src/dispatcher/flipperServer.tsx b/desktop/app/src/dispatcher/flipperServer.tsx index a1297f846..7e4a01fdf 100644 --- a/desktop/app/src/dispatcher/flipperServer.tsx +++ b/desktop/app/src/dispatcher/flipperServer.tsx @@ -70,7 +70,7 @@ export default async (store: Store, logger: Logger) => { server.on('device-connected', (device) => { logger.track('usage', 'register-device', { - os: 'Android', + os: device.os, name: device.title, serial: device.serial, }); diff --git a/desktop/app/src/reducers/connections.tsx b/desktop/app/src/reducers/connections.tsx index 1493b6e1e..20549e6c3 100644 --- a/desktop/app/src/reducers/connections.tsx +++ b/desktop/app/src/reducers/connections.tsx @@ -14,7 +14,6 @@ import type BaseDevice from '../server/devices/BaseDevice'; import MacDevice from '../server/devices/desktop/MacDevice'; import type Client from '../Client'; import type {UninitializedClient} from '../server/UninitializedClient'; -import {isEqual} from 'lodash'; import {performance} from 'perf_hooks'; import type {Actions, Store} from '.'; import {WelcomeScreenStaticView} from '../sandy-chrome/WelcomeScreen'; @@ -73,11 +72,7 @@ type StateV2 = { enabledPlugins: {[client: string]: string[]}; enabledDevicePlugins: Set; clients: Array; - uninitializedClients: Array<{ - client: UninitializedClient; - deviceId?: string; - errorMessage?: string; - }>; + uninitializedClients: UninitializedClient[]; deepLinkPayload: unknown; staticView: StaticView; selectedAppPluginListRevision: number; @@ -137,10 +132,6 @@ export type Action = type: 'START_CLIENT_SETUP'; payload: UninitializedClient; } - | { - type: 'FINISH_CLIENT_SETUP'; - payload: {client: UninitializedClient; deviceId: string}; - } | { type: 'SET_STATIC_VIEW'; payload: StaticView; @@ -366,8 +357,8 @@ export default (state: State = INITAL_STATE, action: Actions): State => { clients: newClients, uninitializedClients: state.uninitializedClients.filter((c) => { return ( - c.deviceId !== payload.query.device_id || - c.client.appName !== payload.query.app + c.deviceName !== payload.query.device || + c.appName !== payload.query.app ); }), }); @@ -402,23 +393,7 @@ export default (state: State = INITAL_STATE, action: Actions): State => { const {payload} = action; return { ...state, - uninitializedClients: state.uninitializedClients - .filter((entry) => !isEqual(entry.client, payload)) - .concat([{client: payload}]) - .sort((a, b) => a.client.appName.localeCompare(b.client.appName)), - }; - } - case 'FINISH_CLIENT_SETUP': { - const {payload} = action; - return { - ...state, - uninitializedClients: state.uninitializedClients - .map((c) => - isEqual(c.client, payload.client) - ? {...c, deviceId: payload.deviceId} - : c, - ) - .sort((a, b) => a.client.appName.localeCompare(b.client.appName)), + uninitializedClients: [...state.uninitializedClients, payload], }; } case 'REGISTER_PLUGINS': { diff --git a/desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx b/desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx index a8a4fd481..71637fc2d 100644 --- a/desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx +++ b/desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx @@ -223,8 +223,8 @@ function computeEntries( Currently connecting... , ...uninitializedClients.map((client) => ( - - {client.client.appName} + + {`${client.appName} (${client.deviceName})`} )), ]); diff --git a/desktop/app/src/server/FlipperServer.tsx b/desktop/app/src/server/FlipperServer.tsx index a237c1214..aff1f3be3 100644 --- a/desktop/app/src/server/FlipperServer.tsx +++ b/desktop/app/src/server/FlipperServer.tsx @@ -113,16 +113,6 @@ export class FlipperServer { }); }); - server.addListener( - 'finish-client-setup', - (payload: {client: UninitializedClient; deviceId: string}) => { - this.store.dispatch({ - type: 'FINISH_CLIENT_SETUP', - payload: payload, - }); - }, - ); - server.addListener( 'client-setup-error', ({client, error}: {client: UninitializedClient; error: Error}) => { diff --git a/desktop/app/src/server/comms/ServerAdapter.tsx b/desktop/app/src/server/comms/ServerAdapter.tsx index 22821f939..ffbda1c29 100644 --- a/desktop/app/src/server/comms/ServerAdapter.tsx +++ b/desktop/app/src/server/comms/ServerAdapter.tsx @@ -29,7 +29,7 @@ export type ClientCsrQuery = { * ClientCsrQuery. It also adds medium information. */ export type SecureClientQuery = ClientQuery & - ClientCsrQuery & {medium: number | undefined}; + ClientCsrQuery & {medium: 1 /*FS*/ | 2 /*WWW*/ | 3 /*NONE*/ | undefined}; /** * Defines an interface for events triggered by a running server interacting diff --git a/desktop/app/src/server/comms/ServerController.tsx b/desktop/app/src/server/comms/ServerController.tsx index 36a94064e..72d5df65e 100644 --- a/desktop/app/src/server/comms/ServerController.tsx +++ b/desktop/app/src/server/comms/ServerController.tsx @@ -34,6 +34,7 @@ import ServerAdapter, { import {createBrowserServer, createServer} from './ServerFactory'; import {FlipperServer} from '../FlipperServer'; import {isTest} from '../../utils/isProduction'; +import {timeout} from 'flipper-plugin'; type ClientInfo = { connection: ClientConnection | null | undefined; @@ -187,15 +188,15 @@ class ServerController extends EventEmitter implements ServerEventsListener { const transformedMedium = transformCertificateExchangeMediumToType( clientQuery.medium, ); - if (transformedMedium === 'WWW') { - this.store.dispatch({ - type: 'REGISTER_DEVICE', - payload: new DummyDevice( + if (transformedMedium === 'WWW' || transformedMedium === 'NONE') { + this.flipperServer.registerDevice( + new DummyDevice( clientQuery.device_id, - clientQuery.app + ' Server Exchanged', + clientQuery.app + + (transformedMedium === 'WWW' ? ' Server Exchanged' : ''), clientQuery.os, ), - }); + ); } } @@ -247,11 +248,6 @@ class ServerController extends EventEmitter implements ServerEventsListener { }); }, 30 * 1000); - this.emit('finish-client-setup', { - client, - deviceId: response.deviceId, - }); - resolve(response); }) .catch((error) => { @@ -285,8 +281,8 @@ class ServerController extends EventEmitter implements ServerEventsListener { // otherwise, use given device_id const {csr_path, csr} = csrQuery; - // For iOS we do not need to confirm the device id, as it never changes unlike android. - if (csr_path && csr && query.os != 'iOS') { + // For Android, device id might change + if (csr_path && csr && query.os === 'Android') { const app_name = await this.certificateProvider.extractAppNameFromCSR( csr, ); @@ -312,6 +308,7 @@ class ServerController extends EventEmitter implements ServerEventsListener { console.log( `[conn] Matching device for ${query.app} on ${query.device_id}...`, ); + // TODO: grab device from flipperServer.devices instead of store const device = getDeviceBySerial(this.store.getState(), query.device_id) ?? (await findDeviceForConnection(this.store, query.app, query.device_id)); @@ -335,7 +332,11 @@ class ServerController extends EventEmitter implements ServerEventsListener { `[conn] Initializing client ${query.app} on ${query.device_id}...`, ); - await client.init(); + await timeout( + 30 * 1000, + client.init(), + `[conn] Failed to initialize client ${query.app} on ${query.device_id} in a timely manner`, + ); connection.subscribeToEvents((status: ConnectionStatus) => { if ( diff --git a/desktop/app/src/server/comms/ServerWebSocket.tsx b/desktop/app/src/server/comms/ServerWebSocket.tsx index b363e8fde..caaab1496 100644 --- a/desktop/app/src/server/comms/ServerWebSocket.tsx +++ b/desktop/app/src/server/comms/ServerWebSocket.tsx @@ -281,8 +281,10 @@ class ServerWebSocket extends ServerWebSocketBase { if (typeof query.medium === 'string') { medium = parseInt(query.medium, 10); } - - return {...clientQuery, csr, csr_path, medium}; + if (medium !== undefined && (medium < 1 || medium > 3)) { + throw new Error('Unsupported exchange medium: ' + medium); + } + return {...clientQuery, csr, csr_path, medium: medium as any}; } } diff --git a/desktop/app/src/server/comms/ServerWebSocketBrowser.tsx b/desktop/app/src/server/comms/ServerWebSocketBrowser.tsx index 85d7edc3e..f48918529 100644 --- a/desktop/app/src/server/comms/ServerWebSocketBrowser.tsx +++ b/desktop/app/src/server/comms/ServerWebSocketBrowser.tsx @@ -110,7 +110,7 @@ class ServerWebSocketBrowser extends ServerWebSocketBase { plugins, ); - const extendedClientQuery = {...clientQuery, medium: 1}; + const extendedClientQuery = {...clientQuery, medium: 3 as const}; extendedClientQuery.sdk_version = plugins == null ? 4 : 1; console.log( @@ -118,6 +118,7 @@ class ServerWebSocketBrowser extends ServerWebSocketBase { ); let resolvedClient: Client | null = null; + this.listener.onSecureConnectionAttempt(extendedClientQuery); const client: Promise = this.listener.onConnectionCreated( extendedClientQuery, clientConnection, diff --git a/desktop/app/src/server/comms/Utilities.tsx b/desktop/app/src/server/comms/Utilities.tsx index 1c825a346..99682091b 100644 --- a/desktop/app/src/server/comms/Utilities.tsx +++ b/desktop/app/src/server/comms/Utilities.tsx @@ -18,12 +18,16 @@ import {ClientQuery} from '../../Client'; export function transformCertificateExchangeMediumToType( medium: number | undefined, ): CertificateExchangeMedium { - if (medium == 1) { - return 'FS_ACCESS'; - } else if (medium == 2) { - return 'WWW'; - } else { - return 'FS_ACCESS'; + switch (medium) { + case undefined: + case 1: + return 'FS_ACCESS'; + case 2: + return 'WWW'; + case 3: + return 'NONE'; + default: + throw new Error('Unknown Certificate exchange medium: ' + medium); } } diff --git a/desktop/app/src/server/utils/CertificateProvider.tsx b/desktop/app/src/server/utils/CertificateProvider.tsx index ed7e8dbce..ea0a9fc3e 100644 --- a/desktop/app/src/server/utils/CertificateProvider.tsx +++ b/desktop/app/src/server/utils/CertificateProvider.tsx @@ -30,7 +30,7 @@ import {timeout} from 'flipper-plugin'; import {v4 as uuid} from 'uuid'; import {isTest} from '../../utils/isProduction'; -export type CertificateExchangeMedium = 'FS_ACCESS' | 'WWW'; +export type CertificateExchangeMedium = 'FS_ACCESS' | 'WWW' | 'NONE'; const tmpFile = promisify(tmp.file) as ( options?: FileOptions,