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

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';

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);
       }
     }
 

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<string, ClientInfo> = new Map();
   timestamps: Map<string, ClientTimestampTracker> = new Map();
 
@@ -168,7 +167,7 @@ class ServerController extends EventEmitter implements ServerEventsListener {
   onConnectionCreated(
     clientQuery: SecureClientQuery,
     clientConnection: ClientConnection,
-    downgrade: boolean,
+    downgrade?: boolean,
   ): Promise<ClientDescription> {
     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}`;
 }

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(),
 });

desktop/flipper-server-core/src/devices/ios/iOSContainerUtility.tsx

@@ -210,17 +210,15 @@ async function push(
 ): Promise<void> {
   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<void> {
   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<T>(p: Promise<T>): Promise<T> {
-  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<boolean> {
   return exec('xcode-select -p')
     .then(({stdout}) => {