From b6c0cc8868b6858b650ca78ec28dda6ff21eaab2 Mon Sep 17 00:00:00 2001 From: Steven Gum <14935595+stevengum@users.noreply.github.com> Date: Wed, 11 Dec 2019 22:04:44 -0800 Subject: [PATCH] fix auth bug, streaming dependencies, add tests (#1498) --- .../botbuilder/src/botFrameworkAdapter.ts | 5 +- .../botbuilder/src/botFrameworkHttpClient.ts | 5 +- .../botbuilder/src/skills/skillHandler.ts | 6 +-- .../src/streaming/streamingHttpClient.ts | 2 +- .../tests/botFrameworkAdapter.test.js | 48 ++++++++++++------- .../src/auth/appCredentials.ts | 17 +++++-- .../src/auth/certificateAppCredentials.ts | 4 +- .../src/auth/microsoftAppCredentials.ts | 6 +-- .../tests/appCredentials.test.js | 22 +++++++-- libraries/botframework-streaming/package.json | 2 +- 10 files changed, 75 insertions(+), 42 deletions(-) diff --git a/libraries/botbuilder/src/botFrameworkAdapter.ts b/libraries/botbuilder/src/botFrameworkAdapter.ts index 28db86e078..d234df8d47 100644 --- a/libraries/botbuilder/src/botFrameworkAdapter.ts +++ b/libraries/botbuilder/src/botFrameworkAdapter.ts @@ -1085,7 +1085,8 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide if (!credentials) throw new Error('invalid credentials'); let client: ConnectorClient = context.turnState.get(this.ConnectorClientKey); - if (!client) { + // Inspect the retrieved client to confirm that the serviceUrl is correct, if it isn't, create a new one. + if (!client || client['baseUri'] !== serviceUrl) { client = this.createConnectorClientInternal(serviceUrl, credentials); } @@ -1098,7 +1099,7 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide * @param appId * @param oAuthScope */ - protected async buildCredentials(appId: string, oAuthScope: string = ''): Promise { + protected async buildCredentials(appId: string, oAuthScope?: string): Promise { // There is no cache for AppCredentials in JS as opposed to C#. // Instead of retrieving an AppCredentials from the Adapter instance, generate a new one const appPassword = await this.credentialsProvider.getAppPassword(appId); diff --git a/libraries/botbuilder/src/botFrameworkHttpClient.ts b/libraries/botbuilder/src/botFrameworkHttpClient.ts index 5ed9830386..9948e640b4 100644 --- a/libraries/botbuilder/src/botFrameworkHttpClient.ts +++ b/libraries/botbuilder/src/botFrameworkHttpClient.ts @@ -110,12 +110,11 @@ export class BotFrameworkHttpClient { const appPassword = await this.credentialProvider.getAppPassword(appId); if (JwtTokenValidation.isGovernment(this.channelService)) { - appCredentials = new MicrosoftAppCredentials(appId, appPassword, this.channelService); + appCredentials = new MicrosoftAppCredentials(appId, appPassword, this.channelService, oAuthScope); appCredentials.oAuthEndpoint = GovernmentConstants.ToChannelFromBotLoginUrl; appCredentials.oAuthScope = GovernmentConstants.ToChannelFromBotOAuthScope; } else { - appCredentials = new MicrosoftAppCredentials(appId, appPassword, this.channelService); - appCredentials.oAuthScope = !oAuthScope ? AuthenticationConstants.ToChannelFromBotOAuthScope : oAuthScope; + appCredentials = new MicrosoftAppCredentials(appId, appPassword, this.channelService, oAuthScope); } // Cache the credentials for later use diff --git a/libraries/botbuilder/src/skills/skillHandler.ts b/libraries/botbuilder/src/skills/skillHandler.ts index 97110f442b..bf7efe5fb9 100644 --- a/libraries/botbuilder/src/skills/skillHandler.ts +++ b/libraries/botbuilder/src/skills/skillHandler.ts @@ -148,11 +148,11 @@ export class SkillHandler extends ChannelServiceHandler { const adapter: BotFrameworkAdapter = (context.adapter as BotFrameworkAdapter); // Cache the ClaimsIdentity and ConnectorClient on the context so that it's available inside of the bot's logic. context.turnState.set(adapter.BotIdentityKey, claimsIdentity); - const client = await adapter.createConnectorClientWithIdentity(activity.serviceUrl, claimsIdentity); - context.turnState.set(adapter.ConnectorClientKey, client); context.turnState.set(this.SkillConversationReferenceKey, skillConversationReference); + activity = TurnContext.applyConversationReference(activity, conversationReference) as Activity; + const client = adapter.createConnectorClient(activity.serviceUrl); + context.turnState.set(adapter.ConnectorClientKey, client); - TurnContext.applyConversationReference(activity, conversationReference); context.activity.id = replyToActivityId; switch (activity.type) { case ActivityTypes.EndOfConversation: diff --git a/libraries/botbuilder/src/streaming/streamingHttpClient.ts b/libraries/botbuilder/src/streaming/streamingHttpClient.ts index e289a7bfc3..6f799806e0 100644 --- a/libraries/botbuilder/src/streaming/streamingHttpClient.ts +++ b/libraries/botbuilder/src/streaming/streamingHttpClient.ts @@ -6,7 +6,7 @@ * Licensed under the MIT License. */ -import { WebResource, HttpOperationResponse, HttpClient } from 'botframework-connector/node_modules/@azure/ms-rest-js'; +import { WebResource, HttpOperationResponse, HttpClient } from '@azure/ms-rest-js'; import { IStreamingTransportServer, StreamingRequest } from 'botframework-streaming'; export class StreamingHttpClient implements HttpClient { diff --git a/libraries/botbuilder/tests/botFrameworkAdapter.test.js b/libraries/botbuilder/tests/botFrameworkAdapter.test.js index d579389df3..2e48f301e6 100644 --- a/libraries/botbuilder/tests/botFrameworkAdapter.test.js +++ b/libraries/botbuilder/tests/botFrameworkAdapter.test.js @@ -1,7 +1,7 @@ const assert = require('assert'); -const { TurnContext } = require('botbuilder-core'); +const { ActivityTypes, TurnContext } = require('botbuilder-core'); const connector = require('botframework-connector'); -const { AuthenticationConstants, CertificateAppCredentials } = require('botframework-connector'); +const { AuthenticationConstants, CertificateAppCredentials, ConnectorClient, MicrosoftAppCredentials } = require('botframework-connector'); const { BotFrameworkAdapter } = require('../'); const reference = { @@ -257,13 +257,25 @@ describe(`BotFrameworkAdapter`, function () { }); }); - it(`should createConnectorClient().`, function (done) { - const req = new MockRequest(incomingMessage); - const adapter = new AdapterUnderTest(); - const client = adapter.testCreateConnectorClient(reference.serviceUrl); - assert(client, `client not returned.`); - assert(client.conversations, `invalid client returned.`); - done(); + describe('get/create ConnectorClient methods', () => { + it(`should createConnectorClient().`, function (done) { + const req = new MockRequest(incomingMessage); + const adapter = new AdapterUnderTest(); + const client = adapter.testCreateConnectorClient(reference.serviceUrl); + assert(client, `client not returned.`); + assert(client.conversations, `invalid client returned.`); + done(); + }); + + it('getOrCreateConnectorClient should create a new client if the cached serviceUrl does not match the provided one', () => { + const adapter = new BotFrameworkAdapter(); + const context = new TurnContext(adapter, { type: ActivityTypes.Message, text: 'hello', serviceUrl: 'http://bing.com' }); + const cc = new ConnectorClient(new MicrosoftAppCredentials('', ''), {baseUri: 'http://bing.com'}); + context.turnState.set(adapter.ConnectorClientKey, cc); + + const client = adapter.getOrCreateConnectorClient(context, 'https://botframework.com', adapter.credentials); + assert.notEqual(client.baseUri, cc.baseUri); + }); }); it(`should processActivity().`, function (done) { @@ -732,15 +744,15 @@ describe(`BotFrameworkAdapter`, function () { }); // This unit test doesn't work anymore because client.UserAgentInfo was removed, so we can't inspect the user agent string - // it(`should create a User-Agent header with the same info as the host machine.`, function (done) { - // const adapter = new BotFrameworkAdapter(); - // const client = adapter.createConnectorClient('https://example.com'); - // //const userAgentHeader = client.userAgentInfo.value; - // const pjson = require('../package.json'); - // const userAgent = 'Microsoft-BotFramework/3.1 BotBuilder/' + pjson.version + ' (Node.js,Version=' + process.version + '; ' + os.type() + ' ' + os.release() + '; ' + os.arch() + ')'; - // // assert(userAgentHeader.includes(userAgent), `ConnectorClient doesn't have user-agent header created by BotFrameworkAdapter or header is incorrect.`); - // done(); - // }); + xit(`should create a User-Agent header with the same info as the host machine.`, function (done) { + const adapter = new BotFrameworkAdapter(); + const client = adapter.createConnectorClient('https://example.com'); + //const userAgentHeader = client.userAgentInfo.value; + const pjson = require('../package.json'); + const userAgent = 'Microsoft-BotFramework/3.1 BotBuilder/' + pjson.version + ' (Node.js,Version=' + process.version + '; ' + os.type() + ' ' + os.release() + '; ' + os.arch() + ')'; + // assert(userAgentHeader.includes(userAgent), `ConnectorClient doesn't have user-agent header created by BotFrameworkAdapter or header is incorrect.`); + done(); + }); it(`should set openIdMetadata property on ChannelValidation`, function (done) { const testEndpoint = "http://rainbows.com"; diff --git a/libraries/botframework-connector/src/auth/appCredentials.ts b/libraries/botframework-connector/src/auth/appCredentials.ts index e55867373b..74a8b03cfe 100644 --- a/libraries/botframework-connector/src/auth/appCredentials.ts +++ b/libraries/botframework-connector/src/auth/appCredentials.ts @@ -30,23 +30,32 @@ export abstract class AppCredentials implements msrest.ServiceClientCredentials public appId: string; public oAuthEndpoint: string; - public oAuthScope: string = AuthenticationConstants.ToBotFromChannelTokenIssuer; - public readonly tokenCacheKey: string; + private _oAuthScope: string; + public tokenCacheKey: string; protected refreshingToken: Promise | null = null; protected readonly authenticationContext: adal.AuthenticationContext; - constructor(appId: string, channelAuthTenant?: string) { + constructor(appId: string, channelAuthTenant?: string, oAuthScope: string = AuthenticationConstants.ToBotFromChannelTokenIssuer) { this.appId = appId; const tenant = channelAuthTenant && channelAuthTenant.length > 0 ? channelAuthTenant : AuthenticationConstants.DefaultChannelAuthTenant; this.oAuthEndpoint = AuthenticationConstants.ToChannelFromBotLoginUrlPrefix + tenant; - this.tokenCacheKey = `${ appId }${ this.oAuthScope }-cache`; + this.oAuthScope = oAuthScope; // aadApiVersion is set to '1.5' to avoid the "spn:" concatenation on the audience claim // For more info, see https://github.com/AzureAD/azure-activedirectory-library-for-nodejs/issues/128 this.authenticationContext = new adal.AuthenticationContext(this.oAuthEndpoint, true, undefined, '1.5'); } + public get oAuthScope(): string { + return this._oAuthScope + } + + public set oAuthScope(value: string) { + this._oAuthScope = value; + this.tokenCacheKey = `${ this.appId }${ this.oAuthScope }-cache`; + } + /** * Adds the host of service url to trusted hosts. * If expiration time is not provided, the expiration date will be current (utc) date + 1 day. diff --git a/libraries/botframework-connector/src/auth/certificateAppCredentials.ts b/libraries/botframework-connector/src/auth/certificateAppCredentials.ts index a88aa7e8d3..f3594034f3 100644 --- a/libraries/botframework-connector/src/auth/certificateAppCredentials.ts +++ b/libraries/botframework-connector/src/auth/certificateAppCredentials.ts @@ -16,8 +16,8 @@ export class CertificateAppCredentials extends AppCredentials { public certificateThumbprint: string; public certificatePrivateKey: string; - constructor(appId: string, certificateThumbprint: string, certificatePrivateKey: string, channelAuthTenant?: string) { - super(appId, channelAuthTenant); + constructor(appId: string, certificateThumbprint: string, certificatePrivateKey: string, channelAuthTenant?: string, oAuthScope?: string) { + super(appId, channelAuthTenant, oAuthScope); this.certificateThumbprint = certificateThumbprint; this.certificatePrivateKey = certificatePrivateKey; } diff --git a/libraries/botframework-connector/src/auth/microsoftAppCredentials.ts b/libraries/botframework-connector/src/auth/microsoftAppCredentials.ts index aed73f0457..d601a87210 100644 --- a/libraries/botframework-connector/src/auth/microsoftAppCredentials.ts +++ b/libraries/botframework-connector/src/auth/microsoftAppCredentials.ts @@ -16,12 +16,8 @@ export class MicrosoftAppCredentials extends AppCredentials { public appPassword: string; constructor(appId: string, appPassword: string, channelAuthTenant?: string, oAuthScope?: string) { - super(appId, channelAuthTenant); + super(appId, channelAuthTenant, oAuthScope); this.appPassword = appPassword; - - // AppCredentials.oAuthScope has an initial an initial value of AuthenticationConstants.ToBotFromChannelTokenIssuer, - // Only change it if there is an actual value provided in the constructor for MicrosoftAppCredentials. - this.oAuthScope = oAuthScope ? oAuthScope : this.oAuthScope; } protected async refreshToken(): Promise { diff --git a/libraries/botframework-connector/tests/appCredentials.test.js b/libraries/botframework-connector/tests/appCredentials.test.js index 57af1902fc..80a7757eb8 100644 --- a/libraries/botframework-connector/tests/appCredentials.test.js +++ b/libraries/botframework-connector/tests/appCredentials.test.js @@ -32,18 +32,34 @@ describe('AppCredentials', () => { strictEqual(certCreds.certificatePrivateKey, CERT_KEY); const msAppCreds = new MicrosoftAppCredentials(APP_ID, APP_PASSWORD); - strictEqual(msAppCreds.appId, '2cd87869-38a0-4182-9251-d056e8f0ac24'); + strictEqual(msAppCreds.appId, APP_ID); strictEqual(msAppCreds.appPassword, APP_PASSWORD); }); describe('MicrosoftAppCredentials', () => { it('should set oAuthScope when passed in the constructor', () => { const oAuthScope = 'oAuthScope'; - const tokenGenerator = new MicrosoftAppCredentials('2cd87869-38a0-4182-9251-d056e8f0ac24', undefined, undefined, oAuthScope); + const tokenGenerator = new MicrosoftAppCredentials(APP_ID, undefined, undefined, oAuthScope); strictEqual(tokenGenerator.oAuthScope, oAuthScope); }); + + it('should set update the tokenCacheKey when oAuthScope is set after construction', () => { + const tokenGenerator = new MicrosoftAppCredentials(APP_ID); + strictEqual(tokenGenerator.tokenCacheKey, `${APP_ID}${AuthenticationConstants.ToBotFromChannelTokenIssuer}-cache`); + + const oAuthScope = 'oAuthScope'; + tokenGenerator.oAuthScope = oAuthScope; + strictEqual(tokenGenerator.tokenCacheKey, `${APP_ID}${oAuthScope}-cache`); + + /* CertificateAppCredentials */ + const certCreds = new CertificateAppCredentials(APP_ID, CERT_THUMBPRINT, CERT_KEY); + strictEqual(certCreds.tokenCacheKey, `${APP_ID}${AuthenticationConstants.ToBotFromChannelTokenIssuer}-cache`); + certCreds.oAuthScope = oAuthScope; + strictEqual(certCreds.tokenCacheKey, `${APP_ID}${oAuthScope}-cache`); + }); + it('should fail to get a token with an appId and no appPassword', async () => { - const tokenGenerator = new MicrosoftAppCredentials('2cd87869-38a0-4182-9251-d056e8f0ac24'); + const tokenGenerator = new MicrosoftAppCredentials(APP_ID); try { await tokenGenerator.getToken(true); fail('Should not have successfully retrieved token.'); diff --git a/libraries/botframework-streaming/package.json b/libraries/botframework-streaming/package.json index 78d3a8e4d0..9471ca2275 100644 --- a/libraries/botframework-streaming/package.json +++ b/libraries/botframework-streaming/package.json @@ -22,13 +22,13 @@ "main": "lib/index.js", "typings": "lib/index.d.js", "dependencies": { + "@types/ws": "^6.0.3", "uuid": "^3.3.2", "ws": "^7.1.2" }, "devDependencies": { "@types/chai": "^4.1.7", "@types/node": "^10.12.18", - "@types/ws": "^6.0.3", "@typescript-eslint/eslint-plugin": "^1.10.2", "@typescript-eslint/parser": "^1.10.2", "chai": "^4.2.0",