From 3b42aa777ba219463802206a912b512b1ef4e4b1 Mon Sep 17 00:00:00 2001 From: Tony Anziano Date: Tue, 20 Aug 2019 14:45:21 -0700 Subject: [PATCH] Added error logging to OAuth signin link generation flow. (#1745) --- CHANGELOG.md | 1 + packages/app/main/src/ngrok.spec.ts | 30 ++++ packages/app/main/src/ngrok.ts | 8 + .../middleware/replyToActivity.spec.ts | 139 ++++++++++++++++++ .../middleware/replyToActivity.ts | 37 +++-- 5 files changed, 195 insertions(+), 20 deletions(-) create mode 100644 packages/emulator/core/src/conversations/middleware/replyToActivity.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index df9545a67..5356af70b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [main] Added End-to-End tests using Spectron in PR [1696](https://github.com/microsoft/BotFramework-Emulator/pull/1696) - [main] New Conversation: send a single conversation update activity including bot and user as members added [1709](https://github.com/microsoft/BotFramework-Emulator/pull/1709) - [app] Consolidated application state store and removed the need for explicit state synchronization between the main and renderer processes in PR [1721](https://github.com/microsoft/BotFramework-Emulator/pull/1721) +- [main] Added logging to OAuth signin link generation flow in PR [1745](https://github.com/microsoft/BotFramework-Emulator/pull/1745) ## Fixed - [main] Fixed bug where opening a chat via URL was sending two conversation updates in PR [1735](https://github.com/microsoft/BotFramework-Emulator/pull/1735) diff --git a/packages/app/main/src/ngrok.spec.ts b/packages/app/main/src/ngrok.spec.ts index 10ffca944..7b5413d7c 100644 --- a/packages/app/main/src/ngrok.spec.ts +++ b/packages/app/main/src/ngrok.spec.ts @@ -30,6 +30,9 @@ // OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. // + +import { join } from 'path'; + import './fetchProxy'; import { intervals, NgrokInstance } from './ngrok'; @@ -74,12 +77,18 @@ jest.mock('node-fetch', () => { }; }); +const mockExistsSync = jest.fn(() => true); +jest.mock('fs', () => ({ + existsSync: () => mockExistsSync(), +})); + describe('the ngrok ', () => { const ngrok = new NgrokInstance(); afterEach(() => { ngrok.kill(); }); + it('should spawn ngrok successfully when the happy path is followed', async () => { const result = await ngrok.connect({ addr: 61914, @@ -158,4 +167,25 @@ describe('the ngrok ', () => { } expect(threw.toString()).toBe('Error: oh noes!'); }); + + it('should throw if it failed to find an ngrok executable at the specified path', async () => { + mockExistsSync.mockReturnValueOnce(false); + + const path = join('Applications', 'ngrok'); + let thrown; + try { + await ngrok.connect({ + addr: 61914, + path, + name: 'c87d3e60-266e-11e9-9528-5798e92fee89', + proto: 'http', + }); + } catch (e) { + thrown = e; + } + + expect(thrown.toString()).toBe( + `Error: Could not find ngrok executable at path: ${path}. Make sure that the correct path to ngrok is configured in the Emulator app settings. Ngrok is required to receive a token from the Bot Framework token service.` + ); + }); }); diff --git a/packages/app/main/src/ngrok.ts b/packages/app/main/src/ngrok.ts index 846359f95..a9618da6f 100644 --- a/packages/app/main/src/ngrok.ts +++ b/packages/app/main/src/ngrok.ts @@ -34,6 +34,7 @@ import { ChildProcess, spawn } from 'child_process'; import { EventEmitter } from 'events'; import { platform } from 'os'; import * as path from 'path'; +import { existsSync } from 'fs'; import { clearTimeout, setTimeout } from 'timers'; import { uniqueId } from '@bfemulator/sdk-shared'; @@ -221,6 +222,13 @@ export class NgrokInstance { 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); + if (!existsSync(ngrokPath)) { + throw new Error( + `Could not find ngrok executable at path: ${ngrokPath}. ` + + `Make sure that the correct path to ngrok is configured in the Emulator app settings. ` + + `Ngrok is required to receive a token from the Bot Framework token service.` + ); + } 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)); diff --git a/packages/emulator/core/src/conversations/middleware/replyToActivity.spec.ts b/packages/emulator/core/src/conversations/middleware/replyToActivity.spec.ts new file mode 100644 index 000000000..d40967562 --- /dev/null +++ b/packages/emulator/core/src/conversations/middleware/replyToActivity.spec.ts @@ -0,0 +1,139 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. +// +// Microsoft Bot Framework: http://botframework.com +// +// Bot Framework Emulator Github: +// https://github.com/Microsoft/BotFramwork-Emulator +// +// Copyright (c) Microsoft Corporation +// All rights reserved. +// +// MIT License: +// Permission is hereby granted, free of charge, to any person obtaining +// a copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to +// permit persons to whom the Software is furnished to do so, subject to +// the following conditions: +// +// The above copyright notice and this permission notice shall be +// included in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED ""AS IS"", WITHOUT WARRANTY OF ANY KIND, +// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE +// LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION +// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION +// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +// + +import { OK } from 'http-status-codes'; + +import replyToActivity from './replyToActivity'; + +const mockResolveOAuthCards = jest.fn().mockResolvedValue(true); +jest.mock('../../utils/oauthLinkEncoder', () => { + return jest.fn().mockImplementation(() => { + return { resolveOAuthCards: mockResolveOAuthCards }; + }); +}); + +describe('replyToActivity route middleware', () => { + const mockReq: any = { + body: { + id: 'someActivityId', + }, + conversation: { + postActivityToUser: jest.fn(() => 'post activity to user response'), + }, + headers: { + authorization: 'Bearer ', + }, + params: { + activityId: 'someOtherActivityId', + conversationId: 'someConversationId', + }, + }; + const mockRes: any = { + end: jest.fn(() => null), + send: jest.fn(() => null), + }; + const mockNext: any = jest.fn(() => null); + const mockBotEmulator: any = { + facilities: { + logger: { + logException: jest.fn(() => null), + }, + }, + }; + + beforeEach(() => { + mockResolveOAuthCards.mockClear(); + mockReq.conversation.postActivityToUser.mockClear(); + mockRes.end.mockClear(); + mockRes.send.mockClear(); + mockNext.mockClear(); + mockBotEmulator.facilities.logger.logException.mockClear(); + }); + + it('should resolve any OAuth cards, post the activity to the user, and send an OK response', async () => { + replyToActivity(mockBotEmulator)(mockReq, mockRes, mockNext); + + // since the middleware is not an async function, wait for the async operations to complete + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(mockReq.conversation.postActivityToUser).toHaveBeenCalledWith({ + ...mockReq.body, + replyToId: mockReq.params.activityId, + }); + expect(mockRes.send).toHaveBeenCalledWith(OK, 'post activity to user response'); + expect(mockRes.end).toHaveBeenCalled(); + expect(mockNext).toHaveBeenCalled(); + }); + + it('should resolve any OAuth cards, post the activity (with a null id) to the user, and send an OK response', async () => { + mockReq.body.id = undefined; + + replyToActivity(mockBotEmulator)(mockReq, mockRes, mockNext); + + // since the middleware is not an async function, wait for the async operations to complete + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(mockReq.conversation.postActivityToUser).toHaveBeenCalledWith({ + ...mockReq.body, + replyToId: mockReq.params.activityId, + id: null, + }); + expect(mockRes.send).toHaveBeenCalledWith(OK, 'post activity to user response'); + expect(mockRes.end).toHaveBeenCalled(); + expect(mockNext).toHaveBeenCalled(); + + mockReq.body.id = 'someActivityId'; + }); + + it('should log any exceptions from OAuth signin in generation before posting the activity to the user', async () => { + const ngrokError = new Error('Failed to spawn ngrok'); + mockResolveOAuthCards.mockRejectedValueOnce(ngrokError); + replyToActivity(mockBotEmulator)(mockReq, mockRes, mockNext); + + // since the middleware is not an async function, wait for the async operations to complete + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(mockBotEmulator.facilities.logger.logException).toHaveBeenCalledWith('someConversationId', ngrokError); + expect(mockBotEmulator.facilities.logger.logException).toHaveBeenCalledWith( + 'someConversationId', + new Error('Falling back to emulated OAuth token.') + ); + expect(mockReq.conversation.postActivityToUser).toHaveBeenCalledWith({ + ...mockReq.body, + replyToId: mockReq.params.activityId, + }); + expect(mockRes.send).toHaveBeenCalledWith(OK, 'post activity to user response'); + expect(mockRes.end).toHaveBeenCalled(); + expect(mockNext).toHaveBeenCalled(); + }); +}); diff --git a/packages/emulator/core/src/conversations/middleware/replyToActivity.ts b/packages/emulator/core/src/conversations/middleware/replyToActivity.ts index 0d5ca919d..0a9b4901c 100644 --- a/packages/emulator/core/src/conversations/middleware/replyToActivity.ts +++ b/packages/emulator/core/src/conversations/middleware/replyToActivity.ts @@ -47,16 +47,9 @@ export default function replyToActivity(botEmulator: BotEmulator) { const conversationParameters: ConversationAPIPathParameters = req.params; try { - // TODO: Need to re-enable - // VersionManager.checkVersion(req.header("User-agent")); - activity.id = activity.id || null; activity.replyToId = req.params.activityId; - // if we found the activity to reply to - // if (!conversation.activities.find((existingActivity, index, obj) => existingActivity.id == activity.replyToId)) - // throw createAPIException(HttpStatus.NOT_FOUND, ErrorCodes.BadArgument, "replyToId is not a known activity id"); - const continuation = function(): void { const response: ResourceResponse = (req as any).conversation.postActivityToUser(activity); @@ -64,20 +57,24 @@ export default function replyToActivity(botEmulator: BotEmulator) { res.end(); }; - const visitor = new OAuthLinkEncoder( - botEmulator, - req.headers.authorization as string, - activity, - conversationParameters.conversationId - ); - visitor.resolveOAuthCards(activity).then( - (value?: any) => { - continuation(); - }, - (_reason: any) => { + const { conversationId } = conversationParameters; + const visitor = new OAuthLinkEncoder(botEmulator, req.headers.authorization as string, activity, conversationId); + visitor + .resolveOAuthCards(activity) + .then(_ => { continuation(); - } - ); + }) + .catch( + // failed to generate an OAuth signin link + (reason: any) => { + botEmulator.facilities.logger.logException(conversationId, reason); + botEmulator.facilities.logger.logException( + conversationId, + new Error('Falling back to emulated OAuth token.') + ); + continuation(); + } + ); } catch (err) { sendErrorResponse(req, res, next, err); }