diff --git a/CHANGELOG.md b/CHANGELOG.md index 7eeea2fd4..549354faa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Fixed - [client/main] Auto update is now opt-in by default and changed UX to reflect this in PR [1575](https://github.com/microsoft/BotFramework-Emulator/pull/1575) +- [client/main] Fixed OAuth card sign-in flow when not using magic code in PR [1660](https://github.com/microsoft/BotFramework-Emulator/pull/1660) ## v4.4.2 - 2019 - 05 - 28 ## Fixed diff --git a/packages/app/main/src/appUpdater.spec.ts b/packages/app/main/src/appUpdater.spec.ts index 1563c1a8f..9072f7af9 100644 --- a/packages/app/main/src/appUpdater.spec.ts +++ b/packages/app/main/src/appUpdater.spec.ts @@ -111,6 +111,18 @@ jest.mock('./utils/sendNotificationToClient', () => ({ }, })); +jest.mock('./emulator', () => ({ + Emulator: { + getInstance: () => ({ + ngrok: { + ngrokEmitter: { + on: () => null, + }, + }, + }), + }, +})); + describe('AppUpdater', () => { let mockTrackEvent; const trackEventBackup = TelemetryService.trackEvent; diff --git a/packages/app/main/src/commands/ngrokCommands.ts b/packages/app/main/src/commands/ngrokCommands.ts index 8ae164736..c7db41be3 100644 --- a/packages/app/main/src/commands/ngrokCommands.ts +++ b/packages/app/main/src/commands/ngrokCommands.ts @@ -35,7 +35,6 @@ import { SharedConstants } from '@bfemulator/app-shared'; import { Command } from '@bfemulator/sdk-shared'; import { Emulator } from '../emulator'; -import { kill } from '../ngrok'; const Commands = SharedConstants.Commands.Ngrok; @@ -56,6 +55,6 @@ export class NgrokCommands { @Command(Commands.KillProcess) protected killNgrokProcess() { - return kill(); + Emulator.getInstance().ngrok.kill(); } } diff --git a/packages/app/main/src/main.ts b/packages/app/main/src/main.ts index e0626bc5f..cae5808ea 100644 --- a/packages/app/main/src/main.ts +++ b/packages/app/main/src/main.ts @@ -45,7 +45,6 @@ import * as commandLine from './commandLine'; import { Protocol } from './constants'; import { Emulator } from './emulator'; import './fetchProxy'; -import { ngrokEmitter } from './ngrok'; import { Window } from './platform/window'; import { azureLoggedInUserChanged } from './settingsData/actions/azureAuthActions'; import { rememberBounds, rememberTheme } from './settingsData/actions/windowStateActions'; @@ -157,7 +156,7 @@ class EmulatorApplication { } private initializeNgrokListeners() { - ngrokEmitter.on('expired', this.onNgrokSessionExpired); + Emulator.getInstance().ngrok.ngrokEmitter.on('expired', this.onNgrokSessionExpired); } private initializeAppListeners() { diff --git a/packages/app/main/src/ngrok.spec.ts b/packages/app/main/src/ngrok.spec.ts index 01713c898..10ffca944 100644 --- a/packages/app/main/src/ngrok.spec.ts +++ b/packages/app/main/src/ngrok.spec.ts @@ -31,8 +31,7 @@ // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. // import './fetchProxy'; -import { ngrokEmitter } from './ngrok'; -import * as ngrok from './ngrok'; +import { intervals, NgrokInstance } from './ngrok'; const mockSpawn = { on: () => {}, @@ -76,6 +75,8 @@ jest.mock('node-fetch', () => { }); describe('the ngrok ', () => { + const ngrok = new NgrokInstance(); + afterEach(() => { ngrok.kill(); }); @@ -106,9 +107,9 @@ describe('the ngrok ', () => { it('should emit when the ngrok session is expired', async () => { mockOk = 0; - ngrok.intervals.retry = 100; - ngrok.intervals.expirationPoll = 1; - ngrok.intervals.expirationTime = -1; + intervals.retry = 100; + intervals.expirationPoll = 1; + intervals.expirationTime = -1; let emitted = false; ngrok.ngrokEmitter.on('expired', () => { emitted = true; @@ -127,7 +128,7 @@ describe('the ngrok ', () => { it('should disconnect', async () => { let disconnected = false; - ngrokEmitter.on('disconnect', url => { + ngrok.ngrokEmitter.on('disconnect', url => { disconnected = true; }); @@ -144,7 +145,7 @@ describe('the ngrok ', () => { it('should throw when the number of reties to retrieve the ngrok url are exhausted', async () => { mockOk = -101; let threw = false; - ngrok.intervals.retry = 1; + intervals.retry = 1; try { await ngrok.connect({ addr: 61914, diff --git a/packages/app/main/src/ngrok.ts b/packages/app/main/src/ngrok.ts index 1c0d64775..846359f95 100644 --- a/packages/app/main/src/ngrok.ts +++ b/packages/app/main/src/ngrok.ts @@ -68,173 +68,175 @@ const defaultOptions: Partial = { const bin = 'ngrok' + (platform() === 'win32' ? '.exe' : ''); const addrRegExp = /starting web service.*addr=(\d+\.\d+\.\d+\.\d+:\d+)/; -let ngrokStartTime: number; -let ngrokExpirationTimer: NodeJS.Timer; -let pendingConnection: Promise<{ url; inspectUrl }>; export const intervals = { retry: 200, expirationPoll: 1000 * 60 * 5, expirationTime: 1000 * 60 * 60 * 8 }; -// Errors should result in the immediate termination of ngrok -// since we have no visibility into the internal state of -// ngrok after the error is received. -export const ngrokEmitter = new EventEmitter().on('error', kill); -let ngrok: ChildProcess; -let tunnels = {}; -let inspectUrl = ''; - -export function running(): boolean { - return ngrok && !!ngrok.pid; -} - -export async function connect(opts: Partial): Promise<{ url; inspectUrl }> { - const options = { ...defaultOptions, ...opts } as NgrokOptions; - if (pendingConnection) { - return pendingConnection; +export class NgrokInstance { + // Errors should result in the immediate termination of ngrok + // since we have no visibility into the internal state of + // ngrok after the error is received. + public ngrokEmitter = new EventEmitter().on('error', this.kill); + private pendingConnection: Promise<{ url; inspectUrl }>; + private ngrokProcess: ChildProcess; + private tunnels = {}; + private inspectUrl = ''; + private ngrokStartTime: number; + private ngrokExpirationTimer: NodeJS.Timer; + + public running(): boolean { + return this.ngrokProcess && !!this.ngrokProcess.pid; } - await getNgrokInspectUrl(options); - return runTunnel(options); -} -async function getNgrokInspectUrl(opts: NgrokOptions): Promise<{ inspectUrl: string }> { - if (running()) { - return { inspectUrl }; + public async connect(opts: Partial): Promise<{ url; inspectUrl }> { + const options = { ...defaultOptions, ...opts } as NgrokOptions; + if (this.pendingConnection) { + return this.pendingConnection; + } + await this.getNgrokInspectUrl(options); + return this.runTunnel(options); } - ngrok = spawnNgrok(opts); - // If we do not receive a ready state from ngrok within 3 seconds, emit and reject - inspectUrl = await new Promise((resolve, reject) => { - const timeout = setTimeout(() => { - const message = 'Failed to receive a ready state from ngrok within 3 seconds.'; - ngrokEmitter.emit('error', message); - reject(message); - }, 3000); - - /** - * Look for an address in the many messages - * sent by ngrok or fail if one does not arrive - * in 3 seconds. - */ - const onNgrokData = (data: Buffer) => { - const addr = data.toString().match(addrRegExp); - if (!addr) { + + public async disconnect(url?: string) { + const tunnelsToDisconnect = url ? [this.tunnels[url]] : Object.keys(this.tunnels); + const requests = tunnelsToDisconnect.map(tunnel => fetch(tunnel, { method: 'DELETE' })); + const responses: Response[] = await Promise.all(requests); + responses.forEach(response => { + if (!response.ok || response.status === 204) { + // Not sure why a 204 is a failure return; } - clearTimeout(timeout); - ngrok.stdout.removeListener('data', onNgrokData); - resolve(`http://${addr[1]}`); - }; - - ngrok.stdout.on('data', onNgrokData); - process.on('exit', kill); - }); - return { inspectUrl }; -} - -/** Checks if the ngrok tunnel is due for expiration */ -function checkForNgrokExpiration(): void { - const currentTime = Date.now(); - const timeElapsed = currentTime - ngrokStartTime; - if (timeElapsed >= intervals.expirationTime) { - cleanUpNgrokExpirationTimer(); - ngrokEmitter.emit('expired'); - } else { - ngrokExpirationTimer = setTimeout(checkForNgrokExpiration, intervals.expirationPoll); + delete this.tunnels[response.url]; + this.ngrokEmitter.emit('disconnect', response.url); + }); } -} -/** Clears the ngrok expiration timer and resets the tunnel start time */ -function cleanUpNgrokExpirationTimer(): void { - ngrokStartTime = null; - clearTimeout(ngrokExpirationTimer); -} + public kill() { + if (!this.ngrokProcess) { + return; + } + this.ngrokProcess.stdout.pause(); + this.ngrokProcess.stderr.pause(); -async function runTunnel(opts: NgrokOptions): Promise<{ url; inspectUrl }> { - let retries = 100; - const url = `${inspectUrl}/api/tunnels`; - const body = JSON.stringify(opts); - // eslint-disable-next-line no-constant-condition - while (true) { - const resp = await fetch(url, { - method: 'POST', - body, - headers: { - 'Content-Type': 'application/json', - }, - }); + this.ngrokProcess.kill(); + this.ngrokProcess = null; + this.tunnels = {}; + this.cleanUpNgrokExpirationTimer(); + } - if (!resp.ok) { - const error = await resp.text(); - await new Promise(resolve => setTimeout(resolve, ~~intervals.retry)); - if (!retries) { - throw new Error(error); - } - retries--; - continue; + private async getNgrokInspectUrl(opts: NgrokOptions): Promise<{ inspectUrl: string }> { + if (this.running()) { + return { inspectUrl: this.inspectUrl }; } + this.ngrokProcess = this.spawnNgrok(opts); + // If we do not receive a ready state from ngrok within 3 seconds, emit and reject + this.inspectUrl = await new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + const message = 'Failed to receive a ready state from ngrok within 3 seconds.'; + this.ngrokEmitter.emit('error', message); + reject(message); + }, 3000); + + /** + * Look for an address in the many messages + * sent by ngrok or fail if one does not arrive + * in 3 seconds. + */ + const onNgrokData = (data: Buffer) => { + const addr = data.toString().match(addrRegExp); + if (!addr) { + return; + } + clearTimeout(timeout); + this.ngrokProcess.stdout.removeListener('data', onNgrokData); + resolve(`http://${addr[1]}`); + }; + + this.ngrokProcess.stdout.on('data', onNgrokData); + process.on('exit', this.kill); + }); + return { inspectUrl: this.inspectUrl }; + } - const result = await resp.json(); - const { public_url: publicUrl, uri, msg } = result; - if (!publicUrl) { - throw Object.assign(new Error(msg || 'failed to start tunnel'), result); - } - tunnels[publicUrl] = uri; - if (opts.proto === 'http' && opts.bind_tls) { - tunnels[publicUrl.replace('https', 'http')] = uri + ' (http)'; + /** Checks if the ngrok tunnel is due for expiration */ + private checkForNgrokExpiration(): void { + const currentTime = Date.now(); + const timeElapsed = currentTime - this.ngrokStartTime; + if (timeElapsed >= intervals.expirationTime) { + this.cleanUpNgrokExpirationTimer(); + this.ngrokEmitter.emit('expired'); + } else { + this.ngrokExpirationTimer = setTimeout(this.checkForNgrokExpiration.bind(this), intervals.expirationPoll); } - ngrokStartTime = Date.now(); - ngrokExpirationTimer = setTimeout(checkForNgrokExpiration, intervals.expirationPoll); + } - ngrokEmitter.emit('connect', publicUrl, inspectUrl); - pendingConnection = null; - return { url: publicUrl, inspectUrl }; + /** Clears the ngrok expiration timer and resets the tunnel start time */ + private cleanUpNgrokExpirationTimer(): void { + this.ngrokStartTime = null; + clearTimeout(this.ngrokExpirationTimer); } -} -export async function disconnect(url?: string) { - const tunnelsToDisconnect = url ? [tunnels[url]] : Object.keys(tunnels); - const requests = tunnelsToDisconnect.map(tunnel => fetch(tunnel, { method: 'DELETE' })); - const responses: Response[] = await Promise.all(requests); - responses.forEach(response => { - if (!response.ok || response.status === 204) { - // Not sure why a 204 is a failure - return; - } - delete tunnels[response.url]; - ngrokEmitter.emit('disconnect', response.url); - }); -} + private async runTunnel(opts: NgrokOptions): Promise<{ url; inspectUrl }> { + let retries = 100; + const url = `${this.inspectUrl}/api/tunnels`; + const body = JSON.stringify(opts); + // eslint-disable-next-line no-constant-condition + while (true) { + const resp = await fetch(url, { + method: 'POST', + body, + headers: { + 'Content-Type': 'application/json', + }, + }); + + if (!resp.ok) { + const error = await resp.text(); + await new Promise(resolve => setTimeout(resolve, ~~intervals.retry)); + if (!retries) { + throw new Error(error); + } + retries--; + continue; + } -export function kill() { - if (!ngrok) { - return; + const result = await resp.json(); + const { public_url: publicUrl, uri, msg } = result; + if (!publicUrl) { + throw Object.assign(new Error(msg || 'failed to start tunnel'), result); + } + this.tunnels[publicUrl] = uri; + if (opts.proto === 'http' && opts.bind_tls) { + this.tunnels[publicUrl.replace('https', 'http')] = uri + ' (http)'; + } + this.ngrokStartTime = Date.now(); + this.ngrokExpirationTimer = setTimeout(this.checkForNgrokExpiration.bind(this), intervals.expirationPoll); + + this.ngrokEmitter.emit('connect', publicUrl, this.inspectUrl); + this.pendingConnection = null; + return { url: publicUrl, inspectUrl: this.inspectUrl }; + } } - ngrok.stdout.pause(); - ngrok.stderr.pause(); - ngrok.kill(); - ngrok = null; - tunnels = {}; - cleanUpNgrokExpirationTimer(); -} + private spawnNgrok(opts: NgrokOptions): ChildProcess { + const filename = `${opts.path ? path.basename(opts.path) : bin}`; + const folder = opts.path ? path.dirname(opts.path) : path.join(__dirname, 'bin'); + const args = ['start', '--none', '--log=stdout', `--region=${opts.region}`]; + const ngrokPath = path.join(folder, filename); + const ngrok = spawn(ngrokPath, args, { cwd: folder }); + // Errors are emitted instead of throwing since ngrok is a long running process + ngrok.on('error', e => this.ngrokEmitter.emit('error', e)); + + ngrok.on('exit', () => { + this.tunnels = {}; + this.cleanUpNgrokExpirationTimer(); + this.ngrokEmitter.emit('disconnect'); + }); + + ngrok.on('close', () => { + this.cleanUpNgrokExpirationTimer(); + this.ngrokEmitter.emit('close'); + }); -function spawnNgrok(opts: NgrokOptions): ChildProcess { - const filename = `${opts.path ? path.basename(opts.path) : bin}`; - const folder = opts.path ? path.dirname(opts.path) : path.join(__dirname, 'bin'); - const args = ['start', '--none', '--log=stdout', `--region=${opts.region}`]; - const ngrokPath = path.join(folder, filename); - const ngrok = spawn(ngrokPath, args, { cwd: folder }); - // Errors are emitted instead of throwing since ngrok is a long running process - ngrok.on('error', e => ngrokEmitter.emit('error', e)); - - ngrok.on('exit', () => { - tunnels = {}; - cleanUpNgrokExpirationTimer(); - ngrokEmitter.emit('disconnect'); - }); - - ngrok.on('close', () => { - cleanUpNgrokExpirationTimer(); - ngrokEmitter.emit('close'); - }); - - ngrok.stderr.on('data', (data: Buffer) => ngrokEmitter.emit('error', data.toString())); - return ngrok; + ngrok.stderr.on('data', (data: Buffer) => this.ngrokEmitter.emit('error', data.toString())); + return ngrok; + } } diff --git a/packages/app/main/src/ngrokService.spec.ts b/packages/app/main/src/ngrokService.spec.ts index c8eb775a9..3892a7620 100644 --- a/packages/app/main/src/ngrokService.spec.ts +++ b/packages/app/main/src/ngrokService.spec.ts @@ -102,12 +102,19 @@ jest.mock('./main', () => ({ }, })); +const mockRunning = jest.fn(() => false); +const mockConnect = jest + .fn() + .mockResolvedValue({ url: 'http://fdsfds.ngrok.io', inspectUrl: 'http://fdsfds.ngrok.io' }); jest.mock('./ngrok', () => { - const connected = false; return { - running: () => connected, - connect: async opts => ({ url: 'http://fdsfds.ngrok.io', inspectUrl: 'http://fdsfds.ngrok.io' }), - kill: () => true, + NgrokInstance: jest.fn().mockImplementation(() => { + return { + running: () => mockRunning(), + connect: mockConnect, + kill: () => true, + }; + }), }; }); @@ -117,6 +124,8 @@ describe('The ngrokService', () => { beforeEach(() => { getStore().dispatch(setFramework(Emulator.getInstance().framework as any)); mockCallsToLog.length = 0; + mockRunning.mockClear(); + mockConnect.mockClear(); }); it('should be a singleton', () => { @@ -153,4 +162,47 @@ describe('The ngrokService', () => { 'ngrok not configured (only needed when connecting to remotely hosted bots)' ); }); + + it('should use the current ngrok instance for an oauth postback url if already running', async () => { + (ngrokService as any).serviceUrl = 'someServiceUrl'; + (ngrokService as any).pendingRecycle = new Promise(resolve => resolve()); + mockRunning.mockReturnValueOnce(true); + const serviceUrl = await ngrokService.getServiceUrlForOAuth(); + + expect(serviceUrl).toBe('someServiceUrl'); + + (ngrokService as any).serviceUrl = undefined; + (ngrokService as any).pendingRecycle = null; + }); + + it('should start up a new ngrok process for an oauth postback url', async () => { + mockRunning.mockReturnValueOnce(false); + mockConnect.mockResolvedValueOnce({ url: 'someNgrokServiceUrl' }); + const serviceUrl = await ngrokService.getServiceUrlForOAuth(); + + expect(serviceUrl).toBe('someNgrokServiceUrl'); + }); + + it('should throw if failed to start up a new ngrok process for an oauth postback url', async () => { + mockRunning.mockReturnValueOnce(false); + mockConnect.mockRejectedValueOnce(new Error('Failed to start ngrok.')); + try { + await ngrokService.getServiceUrlForOAuth(); + expect(false); // fail test + } catch (e) { + expect(e).toEqual( + new Error(`Failed to connect to ngrok instance for OAuth postback URL: ${new Error('Failed to start ngrok.')}`) + ); + } + }); + + it('should shut down the ngrok oauth instance if it is running', () => { + const mockKill = jest.fn(() => null); + (ngrokService as any).oauthNgrokInstance = { kill: mockKill }; + ngrokService.shutDownOAuthNgrokInstance(); + + expect(mockKill).toHaveBeenCalled(); + + (ngrokService as any).oauthNgrokInstance = undefined; + }); }); diff --git a/packages/app/main/src/ngrokService.ts b/packages/app/main/src/ngrokService.ts index 2bac64e44..1162d46e9 100644 --- a/packages/app/main/src/ngrokService.ts +++ b/packages/app/main/src/ngrokService.ts @@ -31,6 +31,8 @@ // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. // +import { EventEmitter } from 'events'; + import { FrameworkSettings } from '@bfemulator/app-shared'; import { appSettingsItem, @@ -45,12 +47,13 @@ import { import { Emulator } from './emulator'; import { emulatorApplication } from './main'; -import * as ngrok from './ngrok'; +import { NgrokInstance } from './ngrok'; import { getStore } from './settingsData/store'; -let ngrokInstance: NgrokService; +let ngrokServiceInstance: NgrokService; export class NgrokService { + private ngrok = new NgrokInstance(); private ngrokPath: string; private serviceUrl: string; private inspectUrl: string; @@ -58,9 +61,10 @@ export class NgrokService { private localhost = 'localhost'; private triedToSpawn: boolean; private pendingRecycle: Promise; + private oauthNgrokInstance: NgrokInstance; constructor() { - return ngrokInstance || (ngrokInstance = this); // Singleton + return ngrokServiceInstance || (ngrokServiceInstance = this); // Singleton } public async getServiceUrl(botUrl: string): Promise { @@ -71,14 +75,14 @@ export class NgrokService { if (this.pendingRecycle) { await this.pendingRecycle; } - if (ngrok.running()) { + if (this.ngrok.running()) { return this.serviceUrl; } const { bypassNgrokLocalhost, runNgrokAtStartup } = getStore().getState().framework; // Use ngrok const local = !botUrl || isLocalHostUrl(botUrl); if (runNgrokAtStartup || !local || (local && !bypassNgrokLocalhost)) { - if (!ngrok.running()) { + if (!this.ngrok.running()) { await this.startup(); } @@ -88,6 +92,41 @@ export class NgrokService { return `http://${this.localhost}:${Emulator.getInstance().framework.serverPort}`; } + // OAuth sign-in flow must always use an ngrok url so that the BF token + // service can deliver the token to the Emulator + public async getServiceUrlForOAuth(): Promise { + // grab the service url from the current ngrok instance if it's running + if (this.pendingRecycle) { + await this.pendingRecycle; + } + if (this.ngrok.running()) { + return this.serviceUrl; + } + // otherwise, we need to spin up an auxillary ngrok instance that we can tear down when the token response comes back + this.oauthNgrokInstance = new NgrokInstance(); + const port = Emulator.getInstance().framework.serverPort; + const ngrokPath = getStore().getState().framework.ngrokPath; + const inspectUrl = new Promise(async (resolve, reject) => { + try { + const { url } = await this.oauthNgrokInstance.connect({ + addr: port, + path: ngrokPath, + }); + resolve(url); + } catch (e) { + reject(new Error(`Failed to connect to ngrok instance for OAuth postback URL: ${e}`)); + } + }); + + return inspectUrl; + } + + public shutDownOAuthNgrokInstance(): void { + if (this.oauthNgrokInstance) { + this.oauthNgrokInstance.kill(); + } + } + public getSpawnStatus = (): { triedToSpawn: boolean; err: any } => ({ triedToSpawn: this.triedToSpawn, err: this.spawnErr, @@ -95,7 +134,7 @@ export class NgrokService { public async updateNgrokFromSettings(framework: FrameworkSettings) { this.cacheSettings(); - if (this.ngrokPath !== framework.ngrokPath && ngrok.running()) { + if (this.ngrokPath !== framework.ngrokPath && this.ngrok.running()) { return this.recycle(); } } @@ -105,7 +144,7 @@ export class NgrokService { return this.pendingRecycle; } - ngrok.kill(); + this.ngrok.kill(); const port = Emulator.getInstance().framework.serverPort; this.ngrokPath = getStore().getState().framework.ngrokPath; @@ -118,7 +157,7 @@ export class NgrokService { return (this.pendingRecycle = new Promise(async resolve => { try { this.triedToSpawn = true; - const { inspectUrl, url } = await ngrok.connect({ + const { inspectUrl, url } = await this.ngrok.connect({ addr: port, path: this.ngrokPath, }); @@ -137,6 +176,20 @@ export class NgrokService { return Promise.resolve(); } + public kill(): void { + if (this.ngrok) { + this.ngrok.kill(); + } + } + + public get ngrokEmitter(): EventEmitter { + return this.ngrok.ngrokEmitter || undefined; + } + + public get running(): boolean { + return this.ngrok.running() || false; + } + /** Logs a message in all active conversations that ngrok has expired */ public broadcastNgrokExpired(): void { this.broadcast(ngrokExpirationItem('ngrok tunnel has expired.')); @@ -177,7 +230,7 @@ export class NgrokService { ); } else if (!this.ngrokPath) { this.reportNotConfigured(conversationId); - } else if (ngrok.running()) { + } else if (this.ngrok.running()) { this.reportRunning(conversationId); } else { emulatorApplication.mainWindow.logService.logToChat( diff --git a/packages/app/main/src/protocolHandler.spec.ts b/packages/app/main/src/protocolHandler.spec.ts index 62e3af829..3a7fa7528 100644 --- a/packages/app/main/src/protocolHandler.spec.ts +++ b/packages/app/main/src/protocolHandler.spec.ts @@ -81,12 +81,17 @@ jest.mock('./settingsData/store', () => ({ })); let mockGetSpawnStatus: any = jest.fn(() => ({ triedToSpawn: true })); +let mockRunningStatus; const mockRecycle = jest.fn(() => null); const mockEmulator = { framework: { serverUrl: 'http://[::]:8090' }, ngrok: { getSpawnStatus: () => mockGetSpawnStatus(), + ngrokEmitter: { + once: (_eventName, cb) => cb(), + }, recycle: () => mockRecycle(), + running: () => mockRunningStatus, }, }; jest.mock('./emulator', () => ({ @@ -95,14 +100,6 @@ jest.mock('./emulator', () => ({ }, })); -let mockRunningStatus; -jest.mock('./ngrok', () => ({ - ngrokEmitter: { - once: (_eventName, cb) => cb(), - }, - running: () => mockRunningStatus, -})); - let mockSendNotificationToClient; jest.mock('./utils/sendNotificationToClient', () => ({ sendNotificationToClient: () => mockSendNotificationToClient(), diff --git a/packages/app/main/src/protocolHandler.ts b/packages/app/main/src/protocolHandler.ts index 4736f7a08..05674e2bd 100644 --- a/packages/app/main/src/protocolHandler.ts +++ b/packages/app/main/src/protocolHandler.ts @@ -47,7 +47,6 @@ import { import { Protocol } from './constants'; import { Emulator } from './emulator'; -import { ngrokEmitter, running } from './ngrok'; import { getSettings } from './settingsData/store'; import { sendNotificationToClient } from './utils/sendNotificationToClient'; import { TelemetryService } from './telemetry'; @@ -258,17 +257,18 @@ class ProtocolHandlerImpl implements ProtocolHandler { const appSettings: FrameworkSettings = getSettings().framework; if (appSettings.ngrokPath) { - let ngrokSpawnStatus = Emulator.getInstance().ngrok.getSpawnStatus(); + const ngrok = Emulator.getInstance().ngrok; + let ngrokSpawnStatus = ngrok.getSpawnStatus(); // if ngrok hasn't spawned yet, we need to start it up if (!ngrokSpawnStatus.triedToSpawn) { - await Emulator.getInstance().ngrok.recycle(); + await ngrok.recycle(); } - ngrokSpawnStatus = Emulator.getInstance().ngrok.getSpawnStatus(); + ngrokSpawnStatus = ngrok.getSpawnStatus(); if (ngrokSpawnStatus.triedToSpawn && ngrokSpawnStatus.err) { throw new Error(`Error while trying to spawn ngrok instance: ${ngrokSpawnStatus.err || ''}`); } - if (running()) { + if (ngrok.running) { try { await this.commandService.call(SharedConstants.Commands.Bot.SetActive, bot); await this.commandService.remoteCall(SharedConstants.Commands.Bot.Load, bot); @@ -277,7 +277,7 @@ class ProtocolHandlerImpl implements ProtocolHandler { } } else { // if ngrok hasn't connected yet, wait for it to connect and load the bot - ngrokEmitter.once( + ngrok.ngrokEmitter.once( 'connect', async (...args: any[]): Promise => { try { diff --git a/packages/app/main/src/restServer.ts b/packages/app/main/src/restServer.ts index a4bbd244d..74c00c685 100644 --- a/packages/app/main/src/restServer.ts +++ b/packages/app/main/src/restServer.ts @@ -56,10 +56,15 @@ export class RestServer { private _botEmulator: BotEmulator; public get botEmulator(): BotEmulator { if (!this._botEmulator) { - this._botEmulator = new BotEmulator(botUrl => Emulator.getInstance().ngrok.getServiceUrl(botUrl), { - fetch, - loggerOrLogService: emulatorApplication.mainWindow.logService, - }); + this._botEmulator = new BotEmulator( + botUrl => Emulator.getInstance().ngrok.getServiceUrl(botUrl), + () => Emulator.getInstance().ngrok.getServiceUrlForOAuth(), + () => Emulator.getInstance().ngrok.shutDownOAuthNgrokInstance(), + { + fetch, + loggerOrLogService: emulatorApplication.mainWindow.logService, + } + ); this._botEmulator.facilities.conversations.on('new', this.onNewConversation); } return this._botEmulator; diff --git a/packages/emulator/cli/src/index.ts b/packages/emulator/cli/src/index.ts index dd5afb632..4f3dfc95b 100644 --- a/packages/emulator/cli/src/index.ts +++ b/packages/emulator/cli/src/index.ts @@ -104,9 +104,14 @@ async function main() { const port = program.port || (await getPort(5000)); // Create a bot entry - const bot = new BotEmulator(async () => program.serviceUrl || `http://localhost:${port}`, { - loggerOrLogService: new NpmLogger(), - }); + const bot = new BotEmulator( + async () => program.serviceUrl, + () => program.serviceUrl, + () => null || `http://localhost:${port}`, + { + loggerOrLogService: new NpmLogger(), + } + ); if (program.file) { const botsJSON = await new Promise((resolve, reject) => { diff --git a/packages/emulator/core/src/botEmulator.spec.ts b/packages/emulator/core/src/botEmulator.spec.ts index 874db008f..073937c8b 100644 --- a/packages/emulator/core/src/botEmulator.spec.ts +++ b/packages/emulator/core/src/botEmulator.spec.ts @@ -73,6 +73,8 @@ jest.mock('restify', () => ({ describe('BotEmulator', () => { it('should instantiate itself properly', async () => { const getServiceUrl = _url => Promise.resolve('serviceUrl'); + const getServiceUrlForOAuth = () => Promise.resolve('serviceUrlForOAuth'); + const shutDownOAuthNgrokInstance = jest.fn(() => null); const customFetch = (_url, _options) => Promise.resolve(); const customLogger = { logActivity: (_conversationId: string, _activity: GenericActivity, _role: string) => 'activityLogged', @@ -86,10 +88,14 @@ describe('BotEmulator', () => { fetch: customFetch, loggerOrLogService: customLogger, }; - const botEmulator1 = new BotEmulator(getServiceUrl, options1); + const botEmulator1 = new BotEmulator(getServiceUrl, getServiceUrlForOAuth, shutDownOAuthNgrokInstance, options1); const serviceUrl = await botEmulator1.getServiceUrl(''); + const serviceUrlForOAuth = await botEmulator1.getServiceUrlForOAuth(); + botEmulator1.shutDownOAuthNgrokInstance(); expect(serviceUrl).toBe('serviceUrl'); + expect(serviceUrlForOAuth).toBe('serviceUrlForOAuth'); + expect(shutDownOAuthNgrokInstance).toHaveBeenCalled(); expect(botEmulator1.options).toEqual({ ...options1, stateSizeLimitKB: 64 }); expect(botEmulator1.facilities.attachments).not.toBeFalsy(); expect(botEmulator1.facilities.botState).not.toBeFalsy(); diff --git a/packages/emulator/core/src/botEmulator.ts b/packages/emulator/core/src/botEmulator.ts index a8326913d..063eb933b 100644 --- a/packages/emulator/core/src/botEmulator.ts +++ b/packages/emulator/core/src/botEmulator.ts @@ -71,11 +71,20 @@ export class BotEmulator { // TODO: Instead of providing a getter for serviceUrl, we should let the upstream to set the serviceUrl // Currently, the upstreamer doesn't really know when the serviceUrl change (ngrok), they need to do their job public getServiceUrl: (botUrl: string) => Promise; + public getServiceUrlForOAuth: () => Promise; + public shutDownOAuthNgrokInstance: () => void; public options: BotEmulatorOptions; public facilities: Facilities; - constructor(getServiceUrl: (botUrl: string) => Promise, options: BotEmulatorOptions = DEFAULT_OPTIONS) { + constructor( + getServiceUrl: (botUrl: string) => Promise, + getServiceUrlForOAuth: () => Promise, + shutDownOAuthNgrokInstance: () => void, + options: BotEmulatorOptions = DEFAULT_OPTIONS + ) { this.getServiceUrl = getServiceUrl; + this.getServiceUrlForOAuth = getServiceUrlForOAuth; + this.shutDownOAuthNgrokInstance = shutDownOAuthNgrokInstance; this.options = { ...DEFAULT_OPTIONS, ...options }; diff --git a/packages/emulator/core/src/userToken/middleware/tokenResponse.ts b/packages/emulator/core/src/userToken/middleware/tokenResponse.ts index 6fb127e0a..df7f2c7a1 100644 --- a/packages/emulator/core/src/userToken/middleware/tokenResponse.ts +++ b/packages/emulator/core/src/userToken/middleware/tokenResponse.ts @@ -48,6 +48,8 @@ export default function tokenResponse(botEmulator: BotEmulator) { const conversation = botEmulator.facilities.conversations.conversationById(conversationId); conversation.sendTokenResponse(body.connectionName, body.token, false).then(response => { + // shut down the oauth ngrok instance + botEmulator.shutDownOAuthNgrokInstance(); if (response.statusCode === HttpStatus.OK) { res.send(HttpStatus.OK, body); } else { diff --git a/packages/emulator/core/src/utils/oauthLinkEncoder.spec.ts b/packages/emulator/core/src/utils/oauthLinkEncoder.spec.ts index 500a25963..f50277064 100644 --- a/packages/emulator/core/src/utils/oauthLinkEncoder.spec.ts +++ b/packages/emulator/core/src/utils/oauthLinkEncoder.spec.ts @@ -66,6 +66,7 @@ describe('The OauthLinkEncoder', () => { mockArgsSentToFetch.length = 0; const emulator = { getServiceUrl: async () => 'http://localhost', + getServiceUrlForOAuth: async () => 'https://ngrok.io/emulator', facilities: { conversations: { conversationById: () => ({ diff --git a/packages/emulator/core/src/utils/oauthLinkEncoder.ts b/packages/emulator/core/src/utils/oauthLinkEncoder.ts index 27871531c..823117b9a 100644 --- a/packages/emulator/core/src/utils/oauthLinkEncoder.ts +++ b/packages/emulator/core/src/utils/oauthLinkEncoder.ts @@ -112,17 +112,17 @@ export default class OAuthLinkEncoder { const headers = { Authorization: this.authorizationHeader, }; - const conversation = this.botEmulator.facilities.conversations.conversationById(this.conversationId); - const emulatorUrl = await this.botEmulator.getServiceUrl(conversation.botEndpoint.botUrl); - const url = - 'https://api.botframework.com/api/botsignin/GetSignInUrl?state=' + - state + - '&emulatorUrl=' + - emulatorUrl + - '&code_challenge=' + - codeChallenge; let errorMessage: string; try { + // we need to make sure that the postback url is accessible from the token server (ngrok) + const emulatorUrl = await this.botEmulator.getServiceUrlForOAuth(); + const url = + 'https://api.botframework.com/api/botsignin/GetSignInUrl?state=' + + state + + '&emulatorUrl=' + + emulatorUrl + + '&code_challenge=' + + codeChallenge; const response = await fetch(url, { headers, method: 'GET', diff --git a/packages/extensions/luis/client/src/Luis/Client.spec.ts b/packages/extensions/luis/client/src/Luis/Client.spec.ts index f26499618..a14672c99 100644 --- a/packages/extensions/luis/client/src/Luis/Client.spec.ts +++ b/packages/extensions/luis/client/src/Luis/Client.spec.ts @@ -240,7 +240,7 @@ describe('LUIS API client', () => { } }); - fit('should throw if training fails', async () => { + it('should throw if training fails', async () => { mockGetStatus.mockResolvedValue({ _response: { status: 201 } }); try { await client.train({} as any);