Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Error logic out of botbuilder into authentication layer #1985

Merged
merged 34 commits into from
Apr 22, 2020
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
cbf3858
removed accidental trycatch block in between unit tests
Zerryth Apr 1, 2020
451d670
temporarily commented out botFrameworkHttpClient.test.js and channelS…
Zerryth Apr 1, 2020
5570672
ChannelValidation now throws AuthenticationError instead of Error; re…
Zerryth Apr 1, 2020
f2fb3a8
newly created private abortWebSocketUpgrade in Adapter successfully p…
Zerryth Apr 2, 2020
789d43b
builds error to write to socket based on error's status code and mess…
Zerryth Apr 2, 2020
ce56d10
Merge branch 'master' into Zerryth/authErrors
Zerryth Apr 2, 2020
c3d6eb5
Moved status code helpers into AuthenticationError from BotFrameworkA…
Zerryth Apr 3, 2020
4ddaba9
throw AuthenticationError instead of Error in all auth layers that ac…
Zerryth Apr 3, 2020
82f2524
throw AuthenticationError in BF Adapter instead of vanilla Error
Zerryth Apr 7, 2020
cf34944
SkillValidation throws AuthenticationError instead of vanilla Error
Zerryth Apr 9, 2020
fbbd7bc
BFAdapter Streaming tests now check for error's status code
Zerryth Apr 9, 2020
1b22fb0
corrected error message in unit test
Zerryth Apr 9, 2020
e8f5e2d
Merge branch 'master' into Zerryth/authErrors
Zerryth Apr 9, 2020
8f12e5e
auth.tests now asserts errors have correct status code
Zerryth Apr 9, 2020
98e4676
Merge branch 'master' into Zerryth/authErrors
Zerryth Apr 9, 2020
f94716f
deprecate abortWebSocketUpgrade
Zerryth Apr 9, 2020
d3d15c2
added AuthenticationError unit tests
Zerryth Apr 9, 2020
f250f87
Merge branch 'Zerryth/authErrors' of https://github.com/microsoft/bot…
Zerryth Apr 9, 2020
7ad7811
removed old abortWebSocketUpgrade() code from BF Adapter
Zerryth Apr 9, 2020
0e1afad
Merge branch 'master' into Zerryth/authErrors
Zerryth Apr 9, 2020
adb7cc1
AuthenticationError no long redefines message property
Zerryth Apr 21, 2020
8c2ab17
Merge branch 'master' into Zerryth/authErrors
Zerryth Apr 21, 2020
7a70db2
readded CallerIdConstants import
Zerryth Apr 21, 2020
30fa0d9
determineStatusCodeAndBuildMessage() refactored to be more succinct
Zerryth Apr 21, 2020
4a48908
Merge branch 'master' into Zerryth/authErrors
Zerryth Apr 21, 2020
f955d31
Reassigned status code vals. per Carlos's suggestions
Zerryth Apr 22, 2020
c5b9cdb
Merge branch 'Zerryth/authErrors' of https://github.com/microsoft/bot…
Zerryth Apr 22, 2020
0701748
Moved AuthError tests to separate file per Steven Gum
Zerryth Apr 22, 2020
09a4682
Replaced unexplicit numbers in botbuilder tests with StatusCode enum …
Zerryth Apr 22, 2020
a2c5da4
Merge branch 'master' into Zerryth/authErrors
Zerryth Apr 22, 2020
fc1a4ad
Used StatusCodes instead of inexplicit numbers in AuthenticationError
Zerryth Apr 22, 2020
59b1752
Merge branch 'Zerryth/authErrors' of https://github.com/microsoft/bot…
Zerryth Apr 22, 2020
ad6b1a9
Merge branch 'master' into Zerryth/authErrors
Zerryth Apr 22, 2020
845ad8e
Merge branch 'master' into Zerryth/authErrors
stevengum Apr 22, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion libraries/botbuilder-core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ export {
SkillConversationReferenceKey,
SkillConversationIdFactoryOptions } from './skills';
export * from './skypeMentionNormalizeMiddleware';
export * from './statusCodes';
export * from './storage';
export * from './telemetryLoggerMiddleware';
export * from './testAdapter';
Expand Down
41 changes: 20 additions & 21 deletions libraries/botbuilder/src/botFrameworkAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
import { STATUS_CODES } from 'http';
import { arch, release, type } from 'os';

import { Activity, ActivityTypes, CoreAppCredentials, BotAdapter, BotCallbackHandlerKey, CallerIdConstants, ChannelAccount, ConversationAccount, ConversationParameters, ConversationReference, ConversationsResult, DeliveryModes, ExpectedReplies, InvokeResponse, ExtendedUserTokenProvider, ResourceResponse, StatusCodes, TokenResponse, TurnContext, INVOKE_RESPONSE_KEY } from 'botbuilder-core';
import { AuthenticationConfiguration, AuthenticationConstants, ChannelValidation, Claim, ClaimsIdentity, ConnectorClient, EmulatorApiClient, GovernmentConstants, GovernmentChannelValidation, JwtTokenValidation, MicrosoftAppCredentials, AppCredentials, CertificateAppCredentials, SimpleCredentialProvider, TokenApiClient, TokenStatus, TokenApiModels, SignInUrlResponse, SkillValidation, TokenExchangeRequest } from 'botframework-connector';
import { Activity, ActivityTypes, CallerIdConstants, CoreAppCredentials, BotAdapter, BotCallbackHandlerKey, ChannelAccount, ConversationAccount, ConversationParameters, ConversationReference, ConversationsResult, DeliveryModes, ExpectedReplies, ExtendedUserTokenProvider, InvokeResponse, INVOKE_RESPONSE_KEY, IStatusCodeError, ResourceResponse, StatusCodes, TokenResponse, TurnContext } from 'botbuilder-core';
import { AuthenticationConfiguration, AuthenticationConstants, ChannelValidation, Claim, ClaimsIdentity, ConnectorClient, EmulatorApiClient, GovernmentConstants, GovernmentChannelValidation, JwtTokenValidation, MicrosoftAppCredentials, AppCredentials, CertificateAppCredentials, SimpleCredentialProvider, TokenApiClient, TokenStatus, TokenApiModels, SignInUrlResponse, SkillValidation, TokenExchangeRequest, AuthenticationError } from 'botframework-connector';

import { INodeBuffer, INodeSocket, IReceiveRequest, ISocket, IStreamingTransportServer, NamedPipeServer, NodeWebSocketFactory, NodeWebSocketFactoryBase, RequestHandler, StreamingResponse, WebSocketServer } from 'botframework-streaming';

Expand Down Expand Up @@ -1378,21 +1378,7 @@ export class BotFrameworkAdapter extends BotAdapter implements ExtendedUserToken
try {
await this.authenticateConnection(req, this.settings.channelService);
} catch (err) {
// If the authenticateConnection call fails, send back the correct error code and close
// the connection.
if (typeof(err.message) === 'string') {
if (err.message.toLowerCase().startsWith('unauthorized')) {
abortWebSocketUpgrade(socket, 401, err.message);
} else if (err.message.toLowerCase().startsWith(`'authheader'`)) {
abortWebSocketUpgrade(socket, 400, err.message);
} else {
abortWebSocketUpgrade(socket, 500, err.message);
}
} else {
abortWebSocketUpgrade(socket, 500);
}

// Re-throw the error so the developer will know what occurred.
abortWebSocketUpgrade(socket, err);
throw err;
}

Expand All @@ -1416,7 +1402,9 @@ export class BotFrameworkAdapter extends BotAdapter implements ExtendedUserToken
const serviceUrl = claims.getClaimValue(AuthenticationConstants.ServiceUrlClaim);
AppCredentials.trustServiceUrl(serviceUrl);

if (!claims.isAuthenticated) { throw new Error('Unauthorized Access. Request is not authorized'); }
if (!claims.isAuthenticated) {
throw new AuthenticationError('Unauthorized Access. Request is not authorized', StatusCodes.UNAUTHORIZED);
}
}

/**
Expand Down Expand Up @@ -1516,11 +1504,22 @@ function delay(timeout: number): Promise<void> {
});
}

function abortWebSocketUpgrade(socket: INodeSocket, code: number, message?: string): void {
/**
* Creates an error message with status code to write to socket, then closes the connection.
*
* @param socket The raw socket connection between the bot (server) and channel/caller (client).
* @param err The error. If the error includes a status code, it will be included in the message written to the socket.
*/
function abortWebSocketUpgrade(socket: INodeSocket, err: any): void {
if (socket.writable) {
const connectionHeader = `Connection: 'close'\r\n`;
socket.write(`HTTP/1.1 ${ code } ${ STATUS_CODES[code] }\r\n${ message }\r\n${ connectionHeader }\r\n`);
}

let message = '';
AuthenticationError.isStatusCodeError(err) ?
message = `HTTP/1.1 ${ err.statusCode } ${ StatusCodes[err.statusCode] }\r\n${ err.message }\r\n${ connectionHeader }\r\n`
: message = AuthenticationError.determineStatusCodeAndBuildMessage(err);

socket.write(message);
}
socket.destroy();
}
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ describe('BotFrameworkAdapter Streaming tests', () => {
await bot.run(context);
});
} catch (err) {
expect(err.statusCode).to.equal(401);
expect(err.message).to.equal('Unauthorized. No valid identity.');
const socketResponse = MockNetSocket.createNonSuccessResponse(401, err.message);
expect(writeSpy.called).to.be.true;
Expand All @@ -147,19 +148,14 @@ describe('BotFrameworkAdapter Streaming tests', () => {
});
} catch (err) {
expect(err.message).to.equal("'authHeader' required.");
expect(err.statusCode).to.equal(400);
const socketResponse = MockNetSocket.createNonSuccessResponse(400, err.message);
expect(writeSpy.called).to.be.true;
expect(writeSpy.calledWithExactly(socketResponse)).to.be.true;
expect(destroySpy.calledOnceWithExactly()).to.be.true;
};
});

try {

} catch (error) {

}

it('returns status code 500 when request logic is not callable', async () => {
const adapter = new BotFrameworkAdapter(new TestAdapterSettings());
const request = new MockHttpRequest();
Expand Down
4 changes: 3 additions & 1 deletion libraries/botbuilder/tests/streaming/mockNetSocket.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const { STATUS_CODES } = require('http');
const { AuthenticationError, StatusCodes } = require('botbuilder-core');

class MockNetSocket {
constructor(readable = true, writable = true) {
Expand All @@ -12,7 +13,8 @@ class MockNetSocket {
}

MockNetSocket.createNonSuccessResponse = (code, message) => {
return `HTTP/1.1 ${code} ${STATUS_CODES[code]}\r\n${message}\r\nConnection: 'close'\r\n\r\n`;
return `HTTP/1.1 ${code} ${StatusCodes[code]}\r\n${message}\r\nConnection: 'close'\r\n\r\n`;
};


module.exports.MockNetSocket = MockNetSocket;
44 changes: 44 additions & 0 deletions libraries/botframework-connector/src/auth/authenticationError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License.
*/
import { IStatusCodeError, StatusCodes } from 'botframework-schema';

export type StatusCode = number;

export class AuthenticationError extends Error implements IStatusCodeError {
constructor(
message: string,
public readonly statusCode: StatusCode
) {
super(message);
}

public static isStatusCodeError(err: any): err is IStatusCodeError {
return !!(err && err.statusCode && typeof err.statusCode === "number");
Zerryth marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Used to determine a status code from the error message for non-`IStatusCodeError`'s.
* @param err The error thrown, used to determine an appropriate status code.
*/
public static determineStatusCodeAndBuildMessage(err: any): string {
let errMessage: string = (err && err.message) ? err.message : 'Internal Server Error';
let code: number = AuthenticationError.determineStatusCode(errMessage);
Zerryth marked this conversation as resolved.
Show resolved Hide resolved
const connectionHeader = `Connection: 'close'\r\n`;

return `HTTP/1.1 ${ code } ${ StatusCodes[code] }\r\n${ errMessage }\r\n${ connectionHeader }\r\n`;
}

private static determineStatusCode(message: string): StatusCode {
if (typeof(message) === 'string') {
Zerryth marked this conversation as resolved.
Show resolved Hide resolved
if (message.toLowerCase().startsWith('unauthorized')) {
return 401;
} else if (message.toLowerCase().startsWith(`'authheader'`)) {
return 400;
}
}
return 500;
}

}
10 changes: 6 additions & 4 deletions libraries/botframework-connector/src/auth/channelValidation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { AuthenticationConfiguration } from './authenticationConfiguration';
import { ClaimsIdentity } from './claimsIdentity';
import { ICredentialProvider } from './credentialProvider';
import { JwtTokenExtractor } from './jwtTokenExtractor';
import { AuthenticationError } from './authenticationError';
import { StatusCodes } from 'botframework-schema';

export namespace ChannelValidation {

Expand Down Expand Up @@ -49,7 +51,7 @@ export namespace ChannelValidation {
const serviceUrlClaim: string = identity.getClaimValue(AuthenticationConstants.ServiceUrlClaim);
if (serviceUrlClaim !== serviceUrl) {
// Claim must match. Not Authorized.
throw new Error('Unauthorized. ServiceUrl claim do not match.');
throw new AuthenticationError('Unauthorized. ServiceUrl claim do not match.', StatusCodes.UNAUTHORIZED);
}

return identity;
Expand Down Expand Up @@ -93,7 +95,7 @@ export namespace ChannelValidation {
): Promise<ClaimsIdentity> {
if (!identity || !identity.isAuthenticated) {
// The token is in some way invalid. Not Authorized.
throw new Error('Unauthorized. Is not authenticated');
throw new AuthenticationError('Unauthorized. Is not authenticated', StatusCodes.UNAUTHORIZED);
}

// Now check that the AppID in the claimset matches
Expand All @@ -104,15 +106,15 @@ export namespace ChannelValidation {
// Look for the "aud" claim, but only if issued from the Bot Framework
if (identity.getClaimValue(AuthenticationConstants.IssuerClaim) !== AuthenticationConstants.ToBotFromChannelTokenIssuer) {
// The relevant Audiance Claim MUST be present. Not Authorized.
throw new Error('Unauthorized. Issuer Claim MUST be present.');
throw new AuthenticationError('Unauthorized. Issuer Claim MUST be present.', StatusCodes.UNAUTHORIZED);
}

// The AppId from the claim in the token must match the AppId specified by the developer.
// In this case, the token is destined for the app, so we find the app ID in the audience claim.
const audClaim: string = identity.getClaimValue(AuthenticationConstants.AudienceClaim);
if (!(await credentials.isValidAppId(audClaim || ''))) {
// The AppId is not valid or not present. Not Authorized.
throw new Error(`Unauthorized. Invalid AppId passed on token: ${ audClaim }`);
throw new AuthenticationError(`Unauthorized. Invalid AppId passed on token: ${ audClaim }`, StatusCodes.UNAUTHORIZED);
}

return identity;
Expand Down
16 changes: 9 additions & 7 deletions libraries/botframework-connector/src/auth/emulatorValidation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { GovernmentConstants } from './governmentConstants';
import { ICredentialProvider } from './credentialProvider';
import { JwtTokenExtractor } from './jwtTokenExtractor';
import { JwtTokenValidation } from './jwtTokenValidation';
import { AuthenticationError } from './authenticationError';
import { StatusCodes } from 'botframework-schema';

/**
* Validates and Examines JWT tokens from the Bot Framework Emulator
Expand Down Expand Up @@ -119,12 +121,12 @@ export namespace EmulatorValidation {
const identity: ClaimsIdentity = await tokenExtractor.getIdentityFromAuthHeader(authHeader, channelId, authConfig.requiredEndorsements);
if (!identity) {
// No valid identity. Not Authorized.
throw new Error('Unauthorized. No valid identity.');
throw new AuthenticationError('Unauthorized. No valid identity.', StatusCodes.UNAUTHORIZED);
}

if (!identity.isAuthenticated) {
// The token is in some way invalid. Not Authorized.
throw new Error('Unauthorized. Is not authenticated');
throw new AuthenticationError('Unauthorized. Is not authenticated', StatusCodes.UNAUTHORIZED);
}

// Now check that the AppID in the claimset matches
Expand All @@ -133,7 +135,7 @@ export namespace EmulatorValidation {
// Async validation.
const versionClaim: string = identity.getClaimValue(AuthenticationConstants.VersionClaim);
if (versionClaim === null) {
throw new Error('Unauthorized. "ver" claim is required on Emulator Tokens.');
throw new AuthenticationError('Unauthorized. "ver" claim is required on Emulator Tokens.', StatusCodes.UNAUTHORIZED);
}

let appId = '';
Expand All @@ -146,7 +148,7 @@ export namespace EmulatorValidation {
const appIdClaim: string = identity.getClaimValue(AuthenticationConstants.AppIdClaim);
if (!appIdClaim) {
// No claim around AppID. Not Authorized.
throw new Error('Unauthorized. "appid" claim is required on Emulator Token version "1.0".');
throw new AuthenticationError('Unauthorized. "appid" claim is required on Emulator Token version "1.0".', StatusCodes.UNAUTHORIZED);
}

appId = appIdClaim;
Expand All @@ -155,17 +157,17 @@ export namespace EmulatorValidation {
const appZClaim: string = identity.getClaimValue(AuthenticationConstants.AuthorizedParty);
if (!appZClaim) {
// No claim around AppID. Not Authorized.
throw new Error('Unauthorized. "azp" claim is required on Emulator Token version "2.0".');
throw new AuthenticationError('Unauthorized. "azp" claim is required on Emulator Token version "2.0".', StatusCodes.UNAUTHORIZED);
}

appId = appZClaim;
} else {
// Unknown Version. Not Authorized.
throw new Error(`Unauthorized. Unknown Emulator Token version "${ versionClaim }".`);
throw new AuthenticationError(`Unauthorized. Unknown Emulator Token version "${ versionClaim }".`, StatusCodes.UNAUTHORIZED);
}

if (!await credentials.isValidAppId(appId)) {
throw new Error(`Unauthorized. Invalid AppId passed on token: ${ appId }`);
throw new AuthenticationError(`Unauthorized. Invalid AppId passed on token: ${ appId }`, StatusCodes.UNAUTHORIZED);
}

return identity;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/**
* @module botbuilder
*/

import { AuthenticationError } from "./authenticationError";
import { StatusCodes } from "botframework-schema";

/**
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License.
Expand All @@ -24,7 +28,7 @@ export class EndorsementsValidator {
}

if (endorsements === null) {
throw new Error('endorsements required');
throw new AuthenticationError('endorsements required', StatusCodes.BAD_REQUEST);
Zerryth marked this conversation as resolved.
Show resolved Hide resolved
}

// The Call path to get here is:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import { ChannelValidation } from './channelValidation';
import { ClaimsIdentity } from './claimsIdentity';
import { ICredentialProvider } from './credentialProvider';
import { JwtTokenExtractor } from './jwtTokenExtractor';
import { AuthenticationError } from './authenticationError';
import { StatusCodes } from 'botframework-schema';

export namespace EnterpriseChannelValidation {

Expand Down Expand Up @@ -46,7 +48,7 @@ export namespace EnterpriseChannelValidation {
const serviceUrlClaim: string = identity.getClaimValue(AuthenticationConstants.ServiceUrlClaim);
if (serviceUrlClaim !== serviceUrl) {
// Claim must match. Not Authorized.
throw new Error('Unauthorized. ServiceUrl claim do not match.');
throw new AuthenticationError('Unauthorized. ServiceUrl claim do not match.', StatusCodes.UNAUTHORIZED);
}

return identity;
Expand Down Expand Up @@ -92,12 +94,12 @@ export namespace EnterpriseChannelValidation {

if (!identity) {
// No valid identity. Not Authorized.
throw new Error('Unauthorized. No valid identity.');
throw new AuthenticationError('Unauthorized. No valid identity.', StatusCodes.UNAUTHORIZED);
}

if (!identity.isAuthenticated) {
// The token is in some way invalid. Not Authorized.
throw new Error('Unauthorized. Is not authenticated');
throw new AuthenticationError('Unauthorized. Is not authenticated', StatusCodes.UNAUTHORIZED);
}

// Now check that the AppID in the claimset matches
Expand All @@ -108,15 +110,15 @@ export namespace EnterpriseChannelValidation {
// Look for the "aud" claim, but only if issued from the Bot Framework
if (identity.getClaimValue(AuthenticationConstants.IssuerClaim) !== AuthenticationConstants.ToBotFromChannelTokenIssuer) {
// The relevant Audiance Claim MUST be present. Not Authorized.
throw new Error('Unauthorized. Issuer Claim MUST be present.');
throw new AuthenticationError('Unauthorized. Issuer Claim MUST be present.', StatusCodes.UNAUTHORIZED);
}

// The AppId from the claim in the token must match the AppId specified by the developer.
// In this case, the token is destined for the app, so we find the app ID in the audience claim.
const audClaim: string = identity.getClaimValue(AuthenticationConstants.AudienceClaim);
if (!(await credentials.isValidAppId(audClaim || ''))) {
// The AppId is not valid or not present. Not Authorized.
throw new Error(`Unauthorized. Invalid AppId passed on token: ${ audClaim }`);
throw new AuthenticationError(`Unauthorized. Invalid AppId passed on token: ${ audClaim }`, StatusCodes.UNAUTHORIZED);
}

return identity;
Expand Down
Loading