Decouple CertificateProvider and adb/idb client intialization

Summary:
Previously CertificateProvider initialized ADB and provided config to IDB. As result, AndroidDeviceManager and iOSDeviceManager indirectly depended on CertificateProvider.

With this diff we:
1. Make idbConfig resemble adbClient.
2. Make AndroidDeviceManager and iOSDeviceManager initialize their own clients
3. Fix server crash when one of the clients couldn't be initialized. The reason for the crash is CertificateProvider.prototype.init which is no longer needed.

Reviewed By: passy

Differential Revision: D33711652

fbshipit-source-id: 055b5625ed993827b65396f4af5157808479242b

desktop/flipper-server-core/src/FlipperServerImpl.tsx

@@ -164,9 +164,18 @@ export class FlipperServerImpl implements FlipperServer {
   }
 
   async startDeviceListeners() {
+    const asyncDeviceListenersPromises: Array<Promise<void>> = [];
+    if (this.config.settings.enableAndroid) {
+      asyncDeviceListenersPromises.push(this.android.watchAndroidDevices());
+    }
+    if (this.config.settings.enableIOS) {
+      asyncDeviceListenersPromises.push(this.ios.watchIOSDevices());
+    }
+    const asyncDeviceListeners = await Promise.all(
+      asyncDeviceListenersPromises,
+    );
     this.disposers.push(
-      await this.android.watchAndroidDevices(),
+      ...asyncDeviceListeners,
-      await this.ios.watchIOSDevices(),
       metroDevice(this),
       desktopDevice(this),
     );

desktop/flipper-server-core/src/comms/ServerController.tsx

@@ -89,10 +89,7 @@ class ServerController extends EventEmitter implements ServerEventsListener {
   constructor(flipperServer: FlipperServerImpl) {
     super();
     this.flipperServer = flipperServer;
-    this.certificateProvider = new CertificateProvider(
+    this.certificateProvider = new CertificateProvider(this);
-      this,
-      getFlipperServerConfig().settings,
-    );
     this.connectionTracker = new ConnectionTracker(this.logger);
   }
 
@@ -116,7 +113,6 @@ class ServerController extends EventEmitter implements ServerEventsListener {
     if (isTest()) {
       throw new Error('Spawing new server is not supported in test');
     }
-    await this.certificateProvider.init();
     const {insecure, secure} = getServerPortsConfig().serverPorts;
 
     const options = await this.certificateProvider.loadSecureServerConfig();

desktop/flipper-server-core/src/devices/android/adbClient.tsx

@@ -13,19 +13,29 @@ import adbConfig from './adbConfig';
 import adbkit, {Client} from 'adbkit';
 import path from 'path';
 
-let instance: Client;
+let instance: Client | undefined;
 
 type Config = {
   androidHome: string;
 };
 
-export async function getAdbClient(config: Config): Promise<Client> {
+export function getAdbClient(): Client | undefined {
-  if (!instance) {
+  return instance;
-    instance = await reportPlatformFailures(
+}
-      createClient(config),
+
-      'createADBClient',
+export async function setAdbClient(
+  config: Config,
+): Promise<Client | undefined> {
+  instance = await reportPlatformFailures(
+    createClient(config),
+    'createADBClient',
+  ).catch((e) => {
+    console.warn(
+      'Failed to initialize ADB. Please disable Android support in settings, or configure a correct path.',
+      e,
     );
-  }
+    return undefined;
+  });
   return instance;
 }
 

desktop/flipper-server-core/src/devices/android/androidDeviceManager.tsx

@@ -10,9 +10,7 @@
 import AndroidDevice from './AndroidDevice';
 import KaiOSDevice from './KaiOSDevice';
 import child_process from 'child_process';
-import {getAdbClient} from './adbClient';
+import {setAdbClient} from './adbClient';
-import which from 'which';
-import {promisify} from 'util';
 import {Client as ADBClient, Device} from 'adbkit';
 import {join} from 'path';
 import {FlipperServerImpl} from '../../FlipperServerImpl';
@@ -172,7 +170,13 @@ export class AndroidDeviceManager {
 
   async watchAndroidDevices() {
     try {
-      const client = await getAdbClient(getFlipperServerConfig().settings);
+      const client = await setAdbClient(getFlipperServerConfig().settings);
+      if (!client) {
+        throw new Error(
+          'AndroidDeviceManager.watchAndroidDevices -> adb not initialized',
+        );
+      }
+
       client
         .trackDevices()
         .then((tracker) => {

desktop/flipper-server-core/src/devices/ios/__tests__/iOSDevice.node.tsx

@@ -72,6 +72,7 @@ test('test getAllPromisesForQueryingDevices when xcode detected', () => {
     getLogger(),
   );
   flipperServer.ios.iosBridge = {} as IOSBridge;
+  (flipperServer.ios as any).idbConfig = getFlipperServerConfig().settings;
   const promises = flipperServer.ios.getAllPromisesForQueryingDevices(
     true,
     false,
@@ -85,6 +86,7 @@ test('test getAllPromisesForQueryingDevices when xcode is not detected', () => {
     getLogger(),
   );
   flipperServer.ios.iosBridge = {} as IOSBridge;
+  (flipperServer.ios as any).idbConfig = getFlipperServerConfig().settings;
   const promises = flipperServer.ios.getAllPromisesForQueryingDevices(
     false,
     true,
@@ -98,6 +100,7 @@ test('test getAllPromisesForQueryingDevices when xcode and idb are both unavaila
     getLogger(),
   );
   flipperServer.ios.iosBridge = {} as IOSBridge;
+  (flipperServer.ios as any).idbConfig = getFlipperServerConfig().settings;
   const promises = flipperServer.ios.getAllPromisesForQueryingDevices(
     false,
     false,
@@ -111,6 +114,7 @@ test('test getAllPromisesForQueryingDevices when both idb and xcode are availabl
     getLogger(),
   );
   flipperServer.ios.iosBridge = {} as IOSBridge;
+  (flipperServer.ios as any).idbConfig = getFlipperServerConfig().settings;
   const promises = flipperServer.ios.getAllPromisesForQueryingDevices(
     true,
     true,

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

@@ -148,6 +148,7 @@ async function targets(
   if (process.platform !== 'darwin') {
     return [];
   }
+
   const isXcodeInstalled = await isXcodeDetected();
   if (!isXcodeInstalled) {
     if (!isPhysicalDeviceEnabled) {
@@ -203,6 +204,7 @@ async function push(
   idbPath: string,
 ): Promise<void> {
   await memoize(checkIdbIsInstalled)(idbPath);
+
   return wrapWithErrorMessage(
     reportPlatformFailures(
       safeExec(
@@ -225,6 +227,7 @@ async function pull(
   idbPath: string,
 ): Promise<void> {
   await memoize(checkIdbIsInstalled)(idbPath);
+
   return wrapWithErrorMessage(
     reportPlatformFailures(
       safeExec(

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

@@ -22,6 +22,8 @@ import {
 import {FlipperServerImpl} from '../../FlipperServerImpl';
 import {notNull} from '../../utils/typeUtils';
 import {getFlipperServerConfig} from '../../FlipperServerConfig';
+import {IdbConfig, setIdbConfig} from './idbConfig';
+import {assertNotNull} from 'flipper-server-core/src/comms/Utilities';
 
 type iOSSimulatorDevice = {
   state: 'Booted' | 'Shutdown' | 'Shutting Down';
@@ -44,6 +46,7 @@ function isAvailable(simulator: iOSSimulatorDevice): boolean {
 
 export class IOSDeviceManager {
   private portForwarders: Array<ChildProcess> = [];
+  private idbConfig?: IdbConfig;
 
   private portforwardingClient = path.join(
     getFlipperServerConfig().paths.staticPath,
@@ -107,14 +110,15 @@ export class IOSDeviceManager {
     isXcodeDetected: boolean,
     isIdbAvailable: boolean,
   ): Array<Promise<any>> {
-    const config = getFlipperServerConfig().settings;
+    assertNotNull(this.idbConfig);
     return [
       isIdbAvailable
-        ? getActiveDevices(config.idbPath, config.enablePhysicalIOS).then(
+        ? getActiveDevices(
-            (devices: IOSDeviceParams[]) => {
+            this.idbConfig.idbPath,
-              this.processDevices(devices);
+            this.idbConfig.enablePhysicalIOS,
-            },
+          ).then((devices: IOSDeviceParams[]) => {
-          )
+            this.processDevices(devices);
+          })
         : null,
       !isIdbAvailable && isXcodeDetected
         ? this.getSimulators(true).then((devices) =>
@@ -126,9 +130,9 @@ export class IOSDeviceManager {
   }
 
   private async queryDevices(): Promise<any> {
-    const config = getFlipperServerConfig().settings;
+    assertNotNull(this.idbConfig);
     const isXcodeInstalled = await iosUtil.isXcodeDetected();
-    const isIdbAvailable = await iosUtil.isAvailable(config.idbPath);
+    const isIdbAvailable = await iosUtil.isAvailable(this.idbConfig.idbPath);
     console.debug(
       `[conn] queryDevices. isXcodeInstalled ${isXcodeInstalled}, isIdbAvailable ${isIdbAvailable}`,
     );
@@ -177,11 +181,8 @@ export class IOSDeviceManager {
   }
 
   public async watchIOSDevices() {
-    // TODO: pull this condition up
     const settings = getFlipperServerConfig().settings;
-    if (!settings.enableIOS) {
+    this.idbConfig = setIdbConfig(settings);
-      return;
-    }
     try {
       const isDetected = await iosUtil.isXcodeDetected();
       this.xcodeCommandLineToolsDetected = isDetected;

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

@@ -0,0 +1,22 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @format
+ */
+
+export type IdbConfig = {
+  idbPath: string;
+  enablePhysicalIOS: boolean;
+};
+
+let idbConfig: IdbConfig | undefined;
+
+export const getIdbConfig = () => idbConfig;
+
+export const setIdbConfig = (newIdbConfig: IdbConfig) => {
+  idbConfig = newIdbConfig;
+  return idbConfig;
+};

desktop/flipper-server-core/src/utils/CertificateProvider.tsx

@@ -22,12 +22,13 @@ import {reportPlatformFailures} from 'flipper-common';
 import {getAdbClient} from '../devices/android/adbClient';
 import * as androidUtil from '../devices/android/androidContainerUtility';
 import os from 'os';
-import {Client as ADBClient} from 'adbkit';
 import archiver from 'archiver';
 import {timeout, isTest} from 'flipper-common';
 import {v4 as uuid} from 'uuid';
 import {internGraphPOSTAPIRequest} from '../fb-stubs/internRequests';
 import {SERVICE_FLIPPER} from '../FlipperServerImpl';
+import {getIdbConfig} from '../devices/ios/idbConfig';
+import {assertNotNull} from '../comms/Utilities';
 
 export type CertificateExchangeMedium = 'FS_ACCESS' | 'WWW' | 'NONE';
 
@@ -69,14 +70,6 @@ export type SecureServerConfig = {
   rejectUnauthorized: boolean;
 };
 
-type CertificateProviderConfig = {
-  idbPath: string;
-  enableAndroid: boolean;
-  enableIOS: boolean;
-  androidHome: string;
-  enablePhysicalIOS: boolean;
-};
-
 /*
  * This class is responsible for generating and deploying server and client
  * certificates to allow for secure communication between Flipper and apps.
@@ -89,38 +82,13 @@ type CertificateProviderConfig = {
  * Flipper CA.
  */
 export default class CertificateProvider {
-  private _adb: ADBClient | undefined;
+  private adb = getAdbClient();
+  private idbConfig = getIdbConfig();
   private didCertificateSetup = false;
-  private config: CertificateProviderConfig;
   private server: ServerController;
 
-  get adb(): ADBClient {
+  constructor(server: ServerController) {
-    if (this.config.enableAndroid) {
-      if (this._adb) {
-        return this._adb;
-      }
-      throw new Error(`ADB initialisation was not not successful`);
-    }
-    throw new Error('Android is not enabled in settings');
-  }
-
-  constructor(server: ServerController, config: CertificateProviderConfig) {
     this.server = server;
-    this.config = config;
-  }
-
-  async init() {
-    if (this.config.enableAndroid) {
-      try {
-        this._adb = await getAdbClient(this.config);
-      } catch (_e) {
-        // make sure initialization failure is already logged
-        throw new Error(
-          'Failed to initialize ADB. Please disable Android support in settings, or configure a correct path. ' +
-            _e,
-        );
-      }
-    }
   }
 
   private uploadFiles = async (
@@ -315,6 +283,8 @@ export default class CertificateProvider {
     const appName = await this.extractAppNameFromCSR(csr);
 
     if (os === 'Android') {
+      assertNotNull(this.adb);
+
       const deviceId = await this.getTargetAndroidDeviceId(
         appName,
         destination,
@@ -359,6 +329,8 @@ export default class CertificateProvider {
     filename: string,
     contents: string,
   ): Promise<void> {
+    assertNotNull(this.idbConfig);
+
     const dir = await tmpDir({unsafeCleanup: true});
     const filePath = path.resolve(dir, filename);
     await fs.writeFile(filePath, contents);
@@ -368,7 +340,7 @@ export default class CertificateProvider {
       filePath,
       bundleId,
       destination,
-      this.config.idbPath,
+      this.idbConfig.idbPath,
     );
   }
 
@@ -377,6 +349,8 @@ export default class CertificateProvider {
     deviceCsrFilePath: string,
     csr: string,
   ): Promise<string> {
+    assertNotNull(this.adb);
+
     const devicesInAdb = await this.adb.listDevices();
     if (devicesInAdb.length === 0) {
       throw new Error('No Android devices found');
@@ -432,14 +406,16 @@ export default class CertificateProvider {
     deviceCsrFilePath: string,
     csr: string,
   ): Promise<string> {
+    assertNotNull(this.idbConfig);
+
     const matches = /\/Devices\/([^/]+)\//.exec(deviceCsrFilePath);
     if (matches && matches.length == 2) {
       // It's a simulator, the deviceId is in the filepath.
       return matches[1];
     }
     const targets = await iosUtil.targets(
-      this.config.idbPath,
+      this.idbConfig.idbPath,
-      this.config.enablePhysicalIOS,
+      this.idbConfig.enablePhysicalIOS,
     );
     if (targets.length === 0) {
       throw new Error('No iOS devices found');
@@ -470,6 +446,8 @@ export default class CertificateProvider {
     processName: string,
     csr: string,
   ): Promise<{isMatch: boolean; foundCsr: string}> {
+    assertNotNull(this.adb);
+
     const deviceCsr = await androidUtil.pull(
       this.adb,
       deviceId,
@@ -492,6 +470,8 @@ export default class CertificateProvider {
     bundleId: string,
     csr: string,
   ): Promise<boolean> {
+    assertNotNull(this.idbConfig);
+
     const originalFile = this.getRelativePathInAppContainer(
       path.resolve(directory, csrFileName),
     );
@@ -525,7 +505,7 @@ export default class CertificateProvider {
         originalFile,
         bundleId,
         dir,
-        this.config.idbPath,
+        this.idbConfig.idbPath,
       );
     } catch (e) {
       console.warn(
@@ -537,7 +517,7 @@ export default class CertificateProvider {
         originalFile,
         bundleId,
         path.join(dir, csrFileName),
-        this.config.idbPath,
+        this.idbConfig.idbPath,
       );
       console.info(
         'Subsequent idb pull succeeded. Nevermind previous wranings.',