From 5e5cf061fc3dc30caa07b10557376de61fd5f2bb Mon Sep 17 00:00:00 2001 From: Steven Gum <14935595+stevengum@users.noreply.github.com> Date: Wed, 23 Oct 2019 10:38:01 -0700 Subject: [PATCH] [Streaming] Auth and browser compatibility fixes, linting, refactoring (#1338) * patch auth in streaming to send token to channel * apply PR feedback for BotFrameworkAdapter * move ws server construction to wsNodeWebSocket constructor for browser compat * cleanup and cache serviceUrl in authenticateConnection --- .../botbuilder/src/botFrameworkAdapter.ts | 117 ++++++++++++------ .../tests/botFrameworkAdapter.test.js | 1 - .../src/webSocket/nodeWebSocket.ts | 10 +- .../src/webSocket/wsNodeWebSocket.ts | 8 +- 4 files changed, 87 insertions(+), 49 deletions(-) diff --git a/libraries/botbuilder/src/botFrameworkAdapter.ts b/libraries/botbuilder/src/botFrameworkAdapter.ts index 76025a59f8..ee436c33cb 100644 --- a/libraries/botbuilder/src/botFrameworkAdapter.ts +++ b/libraries/botbuilder/src/botFrameworkAdapter.ts @@ -186,10 +186,10 @@ const US_GOV_OAUTH_ENDPOINT = 'https://api.botframework.azure.us'; // This key is exported internally so that the TeamsActivityHandler will not overwrite any already set InvokeResponses. export const INVOKE_RESPONSE_KEY: symbol = Symbol('invokeResponse'); const defaultPipeName = 'bfv4.pipes'; -const VERSION_PATH:string = '/api/version'; -const MESSAGES_PATH:string = '/api/messages'; -const GET:string = 'GET'; -const POST:string = 'POST'; +const VERSION_PATH: string = '/api/version'; +const MESSAGES_PATH: string = '/api/messages'; +const GET: string = 'GET'; +const POST: string = 'POST'; /** * A [BotAdapter](xref:botbuilder-core.BotAdapter) that can connect a bot to a service endpoint. @@ -1000,28 +1000,25 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide response.statusCode = StatusCodes.BAD_REQUEST; response.setBody(`Request missing verb and/or path. Verb: ${ request.verb }. Path: ${ request.path }`); return response; - } + } - if (request.verb.toLocaleUpperCase() === GET && request.path.toLocaleLowerCase() === VERSION_PATH) { - response.statusCode = StatusCodes.OK; - response.setBody({UserAgent: USER_AGENT}); + if (request.verb.toLocaleUpperCase() !== POST && request.verb.toLocaleUpperCase() !== GET) { + response.statusCode = StatusCodes.METHOD_NOT_ALLOWED; + response.setBody(`Invalid verb received. Only GET and POST are accepted. Verb: ${ request.verb }`); + } - return response; + if (request.path.toLocaleLowerCase() === VERSION_PATH) { + return await this.handleVersionRequest(request, response); } + // Convert the StreamingRequest into an activity the Adapter can understand. let body: Activity; try { body = await this.readRequestBodyAsString(request); } catch (error) { response.statusCode = StatusCodes.BAD_REQUEST; - response.setBody(`Unable to read request body. Error: ${ error }`); - return response; - } - - if (request.verb.toLocaleUpperCase() !== POST) { - response.statusCode = StatusCodes.METHOD_NOT_ALLOWED; - response.setBody(`Method ${ request.verb.toLocaleUpperCase() } not allowed. Expected POST.`); + response.setBody(`Request body missing or malformed: ${ error }`); return response; } @@ -1031,7 +1028,13 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide return response; } - try { + if (request.verb.toLocaleUpperCase() !== POST) { + response.statusCode = StatusCodes.METHOD_NOT_ALLOWED; + response.setBody(`Invalid verb received for ${ request.verb.toLocaleLowerCase() }. Only GET and POST are accepted. Verb: ${ request.verb }`); + return response; + } + + try { let context = new TurnContext(this, body); await this.runMiddleware(context, this.logic); @@ -1057,6 +1060,39 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide return response; } + private async handleVersionRequest(request: IReceiveRequest, response: StreamingResponse): Promise { + if (request.verb.toLocaleUpperCase() === GET) { + response.statusCode = StatusCodes.OK; + + if (!this.credentials.appId) { + response.setBody({ UserAgent: USER_AGENT }); + return response; + } + + let token = ''; + try { + token = await this.credentials.getToken(); + + } catch (err) { + /** + * In reality a missing BotToken will cause the channel to close the connection, + * but we still send the response and allow the channel to make that decision + * instead of proactively disconnecting. This allows the channel to know why + * the connection has been closed and make the choice not to make endless reconnection + * attempts that will end up right back here. + */ + console.error(err.message); + } + response.setBody({ UserAgent: USER_AGENT, BotToken: token }); + + } else { + response.statusCode = StatusCodes.METHOD_NOT_ALLOWED; + response.setBody(`Invalid verb received for path: ${ request.path }. Only GET is accepted. Verb: ${ request.verb }`); + } + + return response; + } + /** * Creates an OAuth API client. * @@ -1114,7 +1150,7 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide protected checkEmulatingOAuthCards(context: TurnContext): void { if (!this.isEmulatingOAuthCards && context.activity.channelId === 'emulator' && - (!this.credentials.appId || !this.credentials.appPassword)) { + (!this.credentials.appId)) { this.isEmulatingOAuthCards = true; } } @@ -1153,19 +1189,22 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide return serviceUrl && !serviceUrl.toLowerCase().startsWith('http'); } - private async authenticateConnection(req: WebRequest, appId?: string, appPassword?: string, channelService?: string): Promise { - if (!appId || !appPassword) { + private async authenticateConnection(req: WebRequest, channelService?: string): Promise { + if (!this.credentials.appId) { // auth is disabled - return true; + return; } - let authHeader: string = req.headers.authorization || req.headers.Authorization || ''; - let channelIdHeader: string = req.headers.channelid || req.headers.ChannelId || req.headers.ChannelID || ''; - let credentials = new MicrosoftAppCredentials(appId, appPassword); - let credentialProvider = new SimpleCredentialProvider(credentials.appId, credentials.appPassword); - let claims = await JwtTokenValidation.validateAuthHeader(authHeader, credentialProvider, channelService, channelIdHeader); + const authHeader: string = req.headers.authorization || req.headers.Authorization || ''; + const channelIdHeader: string = req.headers.channelid || req.headers.ChannelId || req.headers.ChannelID || ''; + // Validate the received Upgrade request from the channel. + const claims = await JwtTokenValidation.validateAuthHeader(authHeader, this.credentialsProvider, channelService, channelIdHeader); + + // Add serviceUrl from claim to static cache to trigger token refreshes. + const serviceUrl = claims.getClaimValue(AuthenticationConstants.ServiceUrlClaim); + MicrosoftAppCredentials.trustServiceUrl(serviceUrl); - return claims.isAuthenticated; + if (!claims.isAuthenticated) { throw new Error('Unauthorized Access. Request is not authorized'); } } /** @@ -1203,15 +1242,19 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide // Restify-specific check. if (typeof((res as any).claimUpgrade) !== 'function') { - throw new Error("ClaimUpgrade is required for creating WebSocket connection."); + throw new Error('ClaimUpgrade is required for creating WebSocket connection.'); } - const authenticated = await this.authenticateConnection(req, this.settings.appId, this.settings.appPassword, this.settings.channelService); - if (!authenticated) { + try { + await this.authenticateConnection(req, this.settings.channelService); + } catch (err) { + // Set the correct status code for the socket to send back to the channel. res.status(StatusCodes.UNAUTHORIZED); - return Promise.resolve(); + res.send(err.message); + // Re-throw the error so the developer will know what occurred. + throw err; } - + const upgrade = (res as any).claimUpgrade(); const socket = this.webSocketFactory.createWebSocket(req as IncomingMessage, upgrade.socket, upgrade.head); @@ -1227,13 +1270,9 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide await this.streamingServer.start(); } - private async readRequestBodyAsString(request: IReceiveRequest): Promise { - try { - let contentStream = request.streams[0]; - return await contentStream.readAsJson(); - } catch (error) { - return Promise.reject(error); - } + private async readRequestBodyAsString(request: IReceiveRequest): Promise { + const contentStream = request.streams[0]; + return await contentStream.readAsJson(); } } diff --git a/libraries/botbuilder/tests/botFrameworkAdapter.test.js b/libraries/botbuilder/tests/botFrameworkAdapter.test.js index ac15d42bdb..f951dda9da 100644 --- a/libraries/botbuilder/tests/botFrameworkAdapter.test.js +++ b/libraries/botbuilder/tests/botFrameworkAdapter.test.js @@ -2,7 +2,6 @@ const assert = require('assert'); const { TurnContext } = require('botbuilder-core'); const connector = require('botframework-connector'); const { BotFrameworkAdapter } = require('../'); -const os = require('os'); const reference = { activityId: '1234', diff --git a/libraries/botframework-streaming/src/webSocket/nodeWebSocket.ts b/libraries/botframework-streaming/src/webSocket/nodeWebSocket.ts index 96c5f5c800..b68571c388 100644 --- a/libraries/botframework-streaming/src/webSocket/nodeWebSocket.ts +++ b/libraries/botframework-streaming/src/webSocket/nodeWebSocket.ts @@ -11,11 +11,10 @@ import { Socket } from 'net'; import { Watershed } from 'watershed'; import { ISocket } from '../interfaces/ISocket'; -const SHED = new Watershed(); - export class NodeWebSocket implements ISocket { private waterShedSocket: any; private connected: boolean; + protected watershedShed: Watershed; /** * Creates a new instance of the [NodeWebSocket](xref:botframework-streaming.NodeWebSocket) class. @@ -25,6 +24,7 @@ export class NodeWebSocket implements ISocket { public constructor(waterShedSocket?) { this.waterShedSocket = waterShedSocket; this.connected = !!waterShedSocket; + this.watershedShed = new Watershed(); } /** @@ -34,7 +34,7 @@ export class NodeWebSocket implements ISocket { * @param head Buffer */ public create(req: IncomingMessage, socket: Socket, head: Buffer): void { - this.waterShedSocket = SHED.accept(req, socket, head); + this.waterShedSocket = this.watershedShed.accept(req, socket, head); this.connected = true; } @@ -62,7 +62,7 @@ export class NodeWebSocket implements ISocket { */ public async connect(serverAddress, port = 8082): Promise { // Following template from https://github.com/joyent/node-watershed#readme - const wskey = SHED.generateKey(); + const wskey = this.watershedShed.generateKey(); const options = { port: port, hostname: serverAddress, @@ -75,7 +75,7 @@ export class NodeWebSocket implements ISocket { const req = request(options); req.end(); req.on('upgrade', function(res, socket, head): void { - SHED.connect(res, socket, head, wskey); + this.watershedShed.connect(res, socket, head, wskey); }); this.connected = true; diff --git a/libraries/botframework-streaming/src/webSocket/wsNodeWebSocket.ts b/libraries/botframework-streaming/src/webSocket/wsNodeWebSocket.ts index c251a928dc..854cee833e 100644 --- a/libraries/botframework-streaming/src/webSocket/wsNodeWebSocket.ts +++ b/libraries/botframework-streaming/src/webSocket/wsNodeWebSocket.ts @@ -12,14 +12,13 @@ import { Socket } from 'net'; import * as WebSocket from 'ws'; import * as crypto from 'crypto'; -const WS_SERVER = new WebSocket.Server({ noServer: true }); - // Taken from watershed, these needs to be investigated. const NONCE_LENGTH = 16; export class WsNodeWebSocket implements ISocket { private wsSocket: WebSocket; private connected: boolean; + protected wsServer: WebSocket.Server; /** * Creates a new instance of the [WsNodeWebSocket](xref:botframework-streaming.WsNodeWebSocket) class. @@ -29,6 +28,7 @@ export class WsNodeWebSocket implements ISocket { public constructor(wsSocket?: WebSocket) { this.wsSocket = wsSocket; this.connected = !!wsSocket; + this.wsServer = new WebSocket.Server({ noServer: true }); } /** @@ -40,7 +40,7 @@ export class WsNodeWebSocket implements ISocket { public async create(req: IncomingMessage, socket: Socket, head: Buffer): Promise { return new Promise((resolve, reject) => { try { - WS_SERVER.handleUpgrade(req, socket, head, (websocket) => { + this.wsServer.handleUpgrade(req, socket, head, (websocket) => { this.wsSocket = websocket; this.connected = true; resolve(); @@ -90,7 +90,7 @@ export class WsNodeWebSocket implements ISocket { req.on('upgrade', (res, socket, head): void => { // @types/ws does not contain the signature for completeUpgrade // https://github.com/websockets/ws/blob/0a612364e69fc07624b8010c6873f7766743a8e3/lib/websocket-server.js#L269 - (WS_SERVER as any).completeUpgrade(wskey, undefined, res, socket, head, (websocket): void => { + (this.wsServer as any).completeUpgrade(wskey, undefined, res, socket, head, (websocket): void => { this.wsSocket = websocket; this.connected = true; });