From ebe5e7f9ff5b4082a635e4c0a6bbf20d4623ae3f Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 19 Aug 2021 04:07:10 -0700 Subject: [PATCH] Make CertificateProvider side effect free in test (#2706) Summary: Pull Request resolved: https://github.com/facebook/flipper/pull/2706 Creating a CertificateProvider in test had the side effect of generating certificate files, which fails in windows CI. This change makes sure the files aren't generated in test. See https://github.com/facebook/flipper/runs/3366318523. Since it is not possible to start the flipper server 'physically' without writing file (for the secure server), figured to remove the test entirely, since there is high impact but little risk captured by it; if the server doesn't start, *any* manual / exploratory test will fail. Reviewed By: lblasa Differential Revision: D30423173 fbshipit-source-id: e411cc61df04120a7132983e9f8d3d140e4ca048 --- .../app/src/server/comms/ServerController.tsx | 4 ++ .../src/server/utils/CertificateProvider.tsx | 56 ++++++++++--------- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/desktop/app/src/server/comms/ServerController.tsx b/desktop/app/src/server/comms/ServerController.tsx index 5dd68e7db..ef54aa556 100644 --- a/desktop/app/src/server/comms/ServerController.tsx +++ b/desktop/app/src/server/comms/ServerController.tsx @@ -35,6 +35,7 @@ import ServerAdapter, { } from './ServerAdapter'; import {createBrowserServer, createServer} from './ServerFactory'; import {FlipperServer} from '../FlipperServer'; +import {isTest} from '../../utils/isProduction'; type ClientInfo = { connection: ClientConnection | null | undefined; @@ -109,6 +110,9 @@ class ServerController extends EventEmitter implements ServerEventsListener { * which point Flipper is accepting connections. */ init() { + if (isTest()) { + throw new Error('Spawing new server is not supported in test'); + } const {insecure, secure} = this.store.getState().application.serverPorts; this.initialized = this.certificateProvider .loadSecureServerConfig() diff --git a/desktop/app/src/server/utils/CertificateProvider.tsx b/desktop/app/src/server/utils/CertificateProvider.tsx index fb48d708f..01999804d 100644 --- a/desktop/app/src/server/utils/CertificateProvider.tsx +++ b/desktop/app/src/server/utils/CertificateProvider.tsx @@ -102,15 +102,24 @@ export default class CertificateProvider { ) { this.logger = logger; this.adb = getAdbClient(config); - this.certificateSetup = reportPlatformFailures( - this.ensureServerCertExists(), - 'ensureServerCertExists', - ); + if (isTest()) { + this.certificateSetup = Promise.reject( + new Error('Server certificates not available in test'), + ); + } else { + this.certificateSetup = reportPlatformFailures( + this.ensureServerCertExists(), + 'ensureServerCertExists', + ); + } this.config = config; this.server = server; } - uploadFiles = async (zipPath: string, deviceID: string): Promise => { + private uploadFiles = async ( + zipPath: string, + deviceID: string, + ): Promise => { const buff = await fsExtra.readFile(zipPath); const file = new File([buff], 'certs.zip'); return reportPlatformFailures( @@ -223,7 +232,7 @@ export default class CertificateProvider { return Promise.resolve('unknown'); } - ensureOpenSSLIsAvailable(): void { + private ensureOpenSSLIsAvailable(): void { if (!opensslInstalled()) { const e = Error( "It looks like you don't have OpenSSL installed. Please install it to continue.", @@ -232,7 +241,7 @@ export default class CertificateProvider { } } - getCACertificate(): Promise { + private getCACertificate(): Promise { return new Promise((resolve, reject) => { fs.readFile(caCert, (err, data) => { if (err) { @@ -244,7 +253,7 @@ export default class CertificateProvider { }); } - generateClientCertificate(csr: string): Promise { + private generateClientCertificate(csr: string): Promise { console.debug('Creating new client cert', logTag); return this.writeToTempFile(csr).then((path) => { @@ -259,7 +268,7 @@ export default class CertificateProvider { }); } - getRelativePathInAppContainer(absolutePath: string) { + private getRelativePathInAppContainer(absolutePath: string) { const matches = /Application\/[^/]+\/(.*)/.exec(absolutePath); if (matches && matches.length === 2) { return matches[1]; @@ -267,7 +276,7 @@ export default class CertificateProvider { throw new Error("Path didn't match expected pattern: " + absolutePath); } - async deployOrStageFileForMobileApp( + private async deployOrStageFileForMobileApp( destination: string, filename: string, contents: string, @@ -338,7 +347,7 @@ export default class CertificateProvider { return Promise.reject(new Error(`Unsupported device os: ${os}`)); } - pushFileToiOSDevice( + private pushFileToiOSDevice( udid: string, bundleId: string, destination: string, @@ -359,7 +368,7 @@ export default class CertificateProvider { }); } - getTargetAndroidDeviceId( + private getTargetAndroidDeviceId( appName: string, deviceCsrFilePath: string, csr: string, @@ -418,7 +427,7 @@ export default class CertificateProvider { }); } - getTargetiOSDeviceId( + private getTargetiOSDeviceId( appName: string, deviceCsrFilePath: string, csr: string, @@ -454,7 +463,7 @@ export default class CertificateProvider { }); } - androidDeviceHasMatchingCSR( + private androidDeviceHasMatchingCSR( directory: string, deviceId: string, processName: string, @@ -481,7 +490,7 @@ export default class CertificateProvider { }); } - iOSDeviceHasMatchingCSR( + private iOSDeviceHasMatchingCSR( directory: string, deviceId: string, bundleId: string, @@ -523,7 +532,7 @@ export default class CertificateProvider { .then((csrFromDevice) => csrFromDevice === this.santitizeString(csr)); } - santitizeString(csrString: string): string { + private santitizeString(csrString: string): string { return csrString.replace(/\r/g, '').trim(); } @@ -581,9 +590,6 @@ export default class CertificateProvider { } async ensureCertificateAuthorityExists(): Promise { - if (isTest()) { - return; - } if (!fs.existsSync(caKey)) { return this.generateCertificateAuthority(); } @@ -592,7 +598,7 @@ export default class CertificateProvider { ); } - checkCertIsValid(filename: string): Promise { + private checkCertIsValid(filename: string): Promise { if (!fs.existsSync(filename)) { return Promise.reject(new Error(`${filename} does not exist`)); } @@ -634,7 +640,7 @@ export default class CertificateProvider { }); } - verifyServerCertWasIssuedByCA() { + private verifyServerCertWasIssuedByCA() { const options: { [key: string]: any; } = {CAfile: caCert}; @@ -649,7 +655,7 @@ export default class CertificateProvider { }); } - generateCertificateAuthority(): Promise { + private generateCertificateAuthority(): Promise { if (!fs.existsSync(getFilePath(''))) { fs.mkdirSync(getFilePath('')); } @@ -667,7 +673,7 @@ export default class CertificateProvider { .then((_) => undefined); } - ensureServerCertExists(): Promise { + private async ensureServerCertExists(): Promise { if ( !( fs.existsSync(serverKey) && @@ -683,7 +689,7 @@ export default class CertificateProvider { .catch(() => this.generateServerCertificate()); } - generateServerCertificate(): Promise { + private generateServerCertificate(): Promise { return this.ensureCertificateAuthorityExists() .then((_) => { console.warn('Creating new server cert', logTag); @@ -711,7 +717,7 @@ export default class CertificateProvider { .then((_) => undefined); } - writeToTempFile(content: string): Promise { + private writeToTempFile(content: string): Promise { return tmpFile().then((path) => promisify(fs.writeFile)(path, content).then((_) => path), );