From 597f679ed3c39ee802b33b30e9f95ec296592dc9 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 9 Feb 2022 04:21:47 -0800 Subject: [PATCH] Propagate errors properly when cert exchange fails Summary: Certificate exchange errors should be communicated back to the user, as they are often actionable, and otherwise leave users in a stuck state. Also removed the ServerController interface, upgraded the listener types to get at the necessary internal apis typewise that were already available. Removed that error wrapping utility complaining about idb installation, this is rarely ever the problem anymore, and it hides the underlying error. Reviewed By: nikoant Differential Revision: D34105452 fbshipit-source-id: 3b3cd0b99cecbda26dfd0744a90690fe568a5ea5 --- .../src/FlipperServerImpl.tsx | 2 +- .../src/comms/ServerAdapter.tsx | 9 +-- .../src/comms/ServerController.tsx | 33 ++++++++--- .../src/comms/__tests__/utils.tsx | 1 + .../src/devices/ios/iOSContainerUtility.tsx | 55 +++++++------------ 5 files changed, 50 insertions(+), 50 deletions(-) diff --git a/desktop/flipper-server-core/src/FlipperServerImpl.tsx b/desktop/flipper-server-core/src/FlipperServerImpl.tsx index 43eccbcd8..616098144 100644 --- a/desktop/flipper-server-core/src/FlipperServerImpl.tsx +++ b/desktop/flipper-server-core/src/FlipperServerImpl.tsx @@ -8,7 +8,7 @@ */ import EventEmitter from 'events'; -import ServerController from './comms/ServerController'; +import {ServerController} from './comms/ServerController'; import {CertificateExchangeMedium} from './utils/CertificateProvider'; import {AndroidDeviceManager} from './devices/android/androidDeviceManager'; import {IOSDeviceManager} from './devices/ios/iOSDeviceManager'; diff --git a/desktop/flipper-server-core/src/comms/ServerAdapter.tsx b/desktop/flipper-server-core/src/comms/ServerAdapter.tsx index 57892896e..2192d5a75 100644 --- a/desktop/flipper-server-core/src/comms/ServerAdapter.tsx +++ b/desktop/flipper-server-core/src/comms/ServerAdapter.tsx @@ -105,6 +105,8 @@ export interface ServerEventsListener { * // TODO: payload should become JSON */ onClientMessage(clientId: string, payload: string): void; + + onClientSetupError(clientQuery: ClientQuery, error: any): void; } /** @@ -178,12 +180,7 @@ abstract class ServerAdapter { }); return response; } catch (e) { - console.warn( - `[conn] Failed to exchange certificate with ${clientQuery.app} on ${ - clientQuery.device || clientQuery.device_id - }`, - e, - ); + this.listener.onClientSetupError(clientQuery, e); } } diff --git a/desktop/flipper-server-core/src/comms/ServerController.tsx b/desktop/flipper-server-core/src/comms/ServerController.tsx index 7db2a9cfe..3368b9ec6 100644 --- a/desktop/flipper-server-core/src/comms/ServerController.tsx +++ b/desktop/flipper-server-core/src/comms/ServerController.tsx @@ -64,10 +64,6 @@ type ClientCsrQuery = { csr_path?: string | undefined; }; -interface ServerController { - on(event: 'error', callback: (err: Error) => void): this; -} - /** * Responsible of creating and managing the actual underlying servers: * - Insecure (used for certificate exchange) @@ -76,7 +72,10 @@ interface ServerController { * * Additionally, it manages client connections. */ -class ServerController extends EventEmitter implements ServerEventsListener { +export class ServerController + extends EventEmitter + implements ServerEventsListener +{ connections: Map = new Map(); timestamps: Map = new Map(); @@ -168,7 +167,7 @@ class ServerController extends EventEmitter implements ServerEventsListener { onConnectionCreated( clientQuery: SecureClientQuery, clientConnection: ClientConnection, - downgrade: boolean, + downgrade?: boolean, ): Promise { const {app, os, device, device_id, sdk_version, csr, csr_path, medium} = clientQuery; @@ -361,6 +360,26 @@ class ServerController extends EventEmitter implements ServerEventsListener { return null; } + onClientSetupError(clientQuery: ClientQuery, e: any) { + console.warn( + `[conn] Failed to exchange certificate with ${clientQuery.app} on ${ + clientQuery.device || clientQuery.device_id + }`, + e, + ); + const client: UninitializedClient = { + os: clientQuery.os, + deviceName: clientQuery.device, + appName: appNameWithUpdateHint(clientQuery), + }; + this.emit('client-setup-error', { + client, + error: `[conn] Failed to exchange certificate with ${ + clientQuery.app + } on ${clientQuery.device || clientQuery.device_id}: ${e}`, + }); + } + /** * Creates a Client and sets the underlying connection. * @param connection A client connection to communicate between server and client. @@ -527,8 +546,6 @@ class ConnectionTracker { } } -export default ServerController; - function clientQueryToKey(clientQuery: ClientQuery): string { return `${clientQuery.app}/${clientQuery.os}/${clientQuery.device}/${clientQuery.device_id}`; } diff --git a/desktop/flipper-server-core/src/comms/__tests__/utils.tsx b/desktop/flipper-server-core/src/comms/__tests__/utils.tsx index bb97b5fee..864bab419 100644 --- a/desktop/flipper-server-core/src/comms/__tests__/utils.tsx +++ b/desktop/flipper-server-core/src/comms/__tests__/utils.tsx @@ -62,4 +62,5 @@ export const createMockSEListener = (): ServerEventsListener => ({ onConnectionClosed: jest.fn(), onError: jest.fn(), onClientMessage: jest.fn(), + onClientSetupError: jest.fn(), }); diff --git a/desktop/flipper-server-core/src/devices/ios/iOSContainerUtility.tsx b/desktop/flipper-server-core/src/devices/ios/iOSContainerUtility.tsx index b77b09608..c4fd8a1c6 100644 --- a/desktop/flipper-server-core/src/devices/ios/iOSContainerUtility.tsx +++ b/desktop/flipper-server-core/src/devices/ios/iOSContainerUtility.tsx @@ -210,17 +210,15 @@ async function push( ): Promise { await memoize(checkIdbIsInstalled)(idbPath); - return wrapWithErrorMessage( - reportPlatformFailures( - safeExec( - `${idbPath} file push --log ${idbLogLevel} --udid ${udid} --bundle-id ${bundleId} '${src}' '${dst}'`, - ) - .then(() => { - return; - }) - .catch((e) => handleMissingIdb(e, idbPath)), - `${operationPrefix}:push`, - ), + return reportPlatformFailures( + safeExec( + `${idbPath} file push --log ${idbLogLevel} --udid ${udid} --bundle-id ${bundleId} '${src}' '${dst}'`, + ) + .then(() => { + return; + }) + .catch((e) => handleMissingIdb(e, idbPath)), + `${operationPrefix}:push`, ); } @@ -233,18 +231,16 @@ async function pull( ): Promise { await memoize(checkIdbIsInstalled)(idbPath); - return wrapWithErrorMessage( - reportPlatformFailures( - safeExec( - `${idbPath} file pull --log ${idbLogLevel} --udid ${udid} --bundle-id ${bundleId} '${src}' '${dst}'`, - ) - .then(() => { - return; - }) - .catch((e) => handleMissingIdb(e, idbPath)) - .catch((e) => handleMissingPermissions(e)), - `${operationPrefix}:pull`, - ), + return reportPlatformFailures( + safeExec( + `${idbPath} file pull --log ${idbLogLevel} --udid ${udid} --bundle-id ${bundleId} '${src}' '${dst}'`, + ) + .then(() => { + return; + }) + .catch((e) => handleMissingIdb(e, idbPath)) + .catch((e) => handleMissingPermissions(e)), + `${operationPrefix}:pull`, ); } @@ -282,23 +278,12 @@ function handleMissingPermissions(e: Error): void { console.warn(e); throw new Error( 'Cannot connect to iOS application. idb_certificate_pull_failed' + - 'Idb lacks permissions to exchange certificates. Did you install a source build ([FB] or enable certificate exchange)? ' + - e, + 'Idb lacks permissions to exchange certificates. Did you install a source build ([FB] or enable certificate exchange)? See console logs for more details.', ); } throw e; } -function wrapWithErrorMessage(p: Promise): Promise { - return p.catch((e: Error) => { - console.warn(e); - // Give the user instructions. Don't embed the error because it's unique per invocation so won't be deduped. - throw new Error( - "A problem with idb has ocurred. Please run `sudo rm -rf /tmp/idb*` and `sudo yum install -y fb-idb` to update it, if that doesn't fix it, post in Flipper Support.", - ); - }); -} - async function isXcodeDetected(): Promise { return exec('xcode-select -p') .then(({stdout}) => {