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

clean up activityhandler invoke path #1872

Merged
merged 7 commits into from
Mar 9, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
48 changes: 19 additions & 29 deletions libraries/botbuilder-core/src/activityHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,22 +252,6 @@ export class ActivityHandler extends ActivityHandlerBase {
return this.on('Event', handler);
}

/**
* Registers an activity event handler for the _invoke_ event, emitted for every incoming event activity.
*
* @param handler The event handler.
*
* @remarks
* Returns a reference to the [ActivityHandler](xref:botbuilder-core.ActivityHandler) object.
*
* To handle a `signin/verifyState` invoke event or `signin/tokenExchange`, use the
* [onInvokeActivity](xref:botbuilder-core.ActivityHandler.onInvokeActivity) sub-type
* event handler. To handle other named events, add logic to this handler.
*/
public onInvoke(handler: BotHandler): this {
return this.on('Invoke', handler);
}

/**
* Registers an activity event handler for the _end of conversation_ activity.
*
Expand Down Expand Up @@ -416,38 +400,44 @@ export class ActivityHandler extends ActivityHandlerBase {
}

/*
* Runs all registered _invoke_ handlers and then continues the event emission process.
* Provides default behavior for invoke activities.
*
* @param context The context object for the current turn.
*
* @remarks
* Overwrite this method to support channel-specific behavior across multiple channels.
* The default logic is to call any handlers registered via
* [onInvoke](xref:botbuilder-core.ActivityHandler.onInvoke),
* The default logic is to check for a signIn invoke and handle that
* and then continue by calling [defaultNextEvent](xref:botbuilder-core.ActivityHandler.defaultNextEvent).
*/
protected async onInvokeActivity(context: TurnContext): Promise<void|InvokeResponse> {
if(context.activity.name && (context.activity.name === verifyStateOperationName || context.activity.name === tokenExchangeOperationName)) {
await this.onSignInInvoke(context);
protected async onInvokeActivity(context: TurnContext): Promise<InvokeResponse> {
try {
if (context.activity.name && (context.activity.name === verifyStateOperationName || context.activity.name === tokenExchangeOperationName)) {
await this.onSignInInvoke(context);
return { status: 200 };
johnataylor marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return { status: 200 }; [](start = 15, length = 24)

if there is an oauthprompt in the dialog stack, that would have set an invokeResponse object in the turnstate dictionary for the Invoke_Response_key key, so returning an invokeresponse object here would either throw a key collision exception or worse, override the oauthprompt's invokeresponse with a 200 OK always.
Consider either restoring this back to return a void or checking if the Invoke_Response_key has been set to an invokeresponse. If already set, return that object otherwise return 200

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code in ActivityHandlerBase should handle that. Refer to line 70 in that file. The check you are looking for is there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swagatmishra2007, the scenario you mention is handled by this section of code:

// ActivityHandlerBase
case ActivityTypes.Invoke:
    const invokeResponse = await this.onInvokeActivity(context);
    // If onInvokeActivity has already sent an InvokeResponse, do not send another one.
    if (invokeResponse && !context.turnState.get(INVOKE_RESPONSE_KEY)) {
        await context.sendActivity({ value: invokeResponse, type: 'invokeResponse' });
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swagatmishra2007 if you think this is still a problem, please open an issue, and highlight (or add) a test case that illustrates the problem, thanks.

}
throw new Error('NotImplemented');
}
else {
await this.handle(context, 'Invoke', this.defaultNextEvent(context));
catch (err) {
if (err.message === 'NotImplemented') {
return { status: 501 };
}
throw err;
}
finally {
this.defaultNextEvent(context)();
}
}

/*
* Runs all registered _signin invoke activity type_ handlers and then continues the event emission process.
* Handle _signin invoke activity type_.
*
* @param context The context object for the current turn.
*
* @remarks
* Overwrite this method to support channel-specific behavior across multiple channels.
* The default logic is to call any handlers registered via
* [onSignInInvoke](xref:botbuilder-core.ActivityHandler.onSignInInvoke),
* and then continue by calling [defaultNextEvent](xref:botbuilder-core.ActivityHandler.defaultNextEvent).
*/
protected async onSignInInvoke(context: TurnContext): Promise<void> {
await this.handle(context, 'Invoke', this.defaultNextEvent(context));
throw new Error('NotImplemented');
}

/**
Expand Down
21 changes: 18 additions & 3 deletions libraries/botbuilder-core/src/activityHandlerBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import {
ChannelAccount,
MessageReaction,
TurnContext } from '.';
import { InvokeResponse } from './invokeResponse';

export const INVOKE_RESPONSE_KEY: symbol = Symbol('invokeResponse');

/**
* Defines the core behavior for event-emitting activity handlers for bots.
Expand Down Expand Up @@ -62,7 +65,11 @@ export class ActivityHandlerBase {
await this.onEventActivity(context);
break;
case ActivityTypes.Invoke:
await this.onInvokeActivity(context);
const invokeResponse = await this.onInvokeActivity(context);
// If onInvokeActivity has already sent an InvokeResponse, do not send another one.
if (invokeResponse && !context.turnState.get(INVOKE_RESPONSE_KEY)) {
await context.sendActivity({ value: invokeResponse, type: 'invokeResponse' });
}
break;
case ActivityTypes.EndOfConversation:
await this.onEndOfConversationActivity(context);
Expand Down Expand Up @@ -153,8 +160,16 @@ export class ActivityHandlerBase {
return;
}

protected async onInvokeActivity(context: TurnContext): Promise<void|any> {
return;
/**
* Provides a hook for invoke calls.
*
* @param context The context object for the current turn.
*
* @remarks
* Overwrite this method to handle particular invoke calls.
*/
protected async onInvokeActivity(context: TurnContext): Promise<InvokeResponse> {
return { status: 501 };
}

/**
Expand Down
12 changes: 0 additions & 12 deletions libraries/botbuilder-core/tests/ActivityHandler.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,18 +197,6 @@ describe('ActivityHandler', function() {
processActivity({type: ActivityTypes.Event}, bot, done);
});

it(`should fire onInvoke`, async function (done) {
const bot = new ActivityHandler();

bot.onInvoke(async(context, next) => {
assert(true, 'onInvoke not called');
done();
await next();
});

processActivity({type: ActivityTypes.Invoke}, bot, done);
});

it(`should fire onEndOfConversation`, async function(done) {

const bot = new ActivityHandler();
Expand Down
11 changes: 6 additions & 5 deletions libraries/botbuilder/src/botFrameworkAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import { STATUS_CODES } from 'http';
import * as os from 'os';

import { Activity, ActivityTypes, BotAdapter, BotCallbackHandlerKey, ChannelAccount, ConversationAccount, ConversationParameters, ConversationReference, ConversationsResult, DeliveryModes, ExpectedReplies, InvokeResponse, ExtendedUserTokenProvider, ResourceResponse, StatusCodes, TokenResponse, TurnContext } from 'botbuilder-core';
import { Activity, ActivityTypes, BotAdapter, BotCallbackHandlerKey, 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 { INodeBuffer, INodeSocket, IReceiveRequest, ISocket, IStreamingTransportServer, NamedPipeServer, NodeWebSocketFactory, NodeWebSocketFactoryBase, RequestHandler, StreamingResponse, WebSocketServer } from 'botframework-streaming';
Expand Down Expand Up @@ -86,7 +86,7 @@ const OAUTH_ENDPOINT = 'https://api.botframework.com';
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');
//export const INVOKE_RESPONSE_KEY: symbol = Symbol('invokeResponse');
johnataylor marked this conversation as resolved.
Show resolved Hide resolved

/**
* A [BotAdapter](xref:botbuilder-core.BotAdapter) that can connect a bot to a service endpoint.
Expand Down Expand Up @@ -938,7 +938,7 @@ export class BotFrameworkAdapter extends BotAdapter implements ExtendedUserToken
responses.push({} as ResourceResponse);
break;
case 'invokeResponse':
// Cache response to context object. This will be retrieved when turn completes.
// Cache response to context object. This will be retrieved when turn completes.
context.turnState.set(INVOKE_RESPONSE_KEY, activity);
responses.push({} as ResourceResponse);
break;
Expand Down Expand Up @@ -1264,11 +1264,12 @@ export class BotFrameworkAdapter extends BotAdapter implements ExtendedUserToken

if (body.type === ActivityTypes.Invoke) {
let invokeResponse: any = context.turnState.get(INVOKE_RESPONSE_KEY);

if (invokeResponse && invokeResponse.value) {
const value: InvokeResponse = invokeResponse.value;
response.statusCode = value.status;
response.setBody(value.body);
if (value.body) {
response.setBody(value.body);
}
} else {
response.statusCode = StatusCodes.NOT_IMPLEMENTED;
}
Expand Down
3 changes: 1 addition & 2 deletions libraries/botbuilder/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@

export {
BotFrameworkAdapter,
BotFrameworkAdapterSettings,
INVOKE_RESPONSE_KEY
BotFrameworkAdapterSettings
} from './botFrameworkAdapter';
export { BotFrameworkHttpClient } from './botFrameworkHttpClient';
export { ChannelServiceHandler } from './channelServiceHandler';
Expand Down
50 changes: 19 additions & 31 deletions libraries/botbuilder/src/teamsActivityHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
* Licensed under the MIT License.
*/

import { INVOKE_RESPONSE_KEY } from './botFrameworkAdapter';

import {
ActivityHandler,
ActivityTypes,
Expand All @@ -34,44 +32,17 @@ import { TeamsInfo } from './teamsInfo';

export class TeamsActivityHandler extends ActivityHandler {

/**
*
* @param context
*/
protected async onTurnActivity(context: TurnContext): Promise<void> {
switch (context.activity.type) {
case ActivityTypes.Invoke:
const invokeResponse = await this.onInvokeActivity(context);
// If onInvokeActivity has already sent an InvokeResponse, do not send another one.
if (invokeResponse && !context.turnState.get(INVOKE_RESPONSE_KEY)) {
await context.sendActivity({ value: invokeResponse, type: 'invokeResponse' });
}
await this.defaultNextEvent(context)();
break;
default:
await super.onTurnActivity(context);
break;
}
}

/**
*
* @param context
*/
protected async onInvokeActivity(context: TurnContext): Promise<InvokeResponse> {
let runEvents = true;
try {
if (!context.activity.name && context.activity.channelId === 'msteams') {
return await this.handleTeamsCardActionInvoke(context);
} else {
switch (context.activity.name) {
case verifyStateOperationName:
await this.handleTeamsSigninVerifyState(context, context.activity.value);
return TeamsActivityHandler.createInvokeResponse();

case tokenExchangeOperationName:
await this.handleTeamsSigninTokenExchange(context, context.activity.value);
return TeamsActivityHandler.createInvokeResponse();

case 'fileConsent/invoke':
return TeamsActivityHandler.createInvokeResponse(await this.handleTeamsFileConsent(context, context.activity.value));

Expand Down Expand Up @@ -112,7 +83,8 @@ export class TeamsActivityHandler extends ActivityHandler {
return TeamsActivityHandler.createInvokeResponse(await this.handleTeamsTaskModuleSubmit(context, context.activity.value));

default:
throw new Error('NotImplemented');
runEvents = false;
return super.onInvokeActivity(context);
}
}
} catch (err) {
Expand All @@ -122,6 +94,10 @@ export class TeamsActivityHandler extends ActivityHandler {
return { status: 400 };
}
throw err;
} finally {
if (runEvents) {
this.defaultNextEvent(context)();
}
}
}

Expand Down Expand Up @@ -181,6 +157,18 @@ export class TeamsActivityHandler extends ActivityHandler {
throw new Error('NotImplemented');
}

/*
* override default because to call teams specific events
*/
protected async onSignInInvoke(context: TurnContext): Promise<void> {
switch (context.activity.name) {
case verifyStateOperationName:
await this.handleTeamsSigninVerifyState(context, context.activity.value);
case tokenExchangeOperationName:
await this.handleTeamsSigninTokenExchange(context, context.activity.value);
}
}

/**
* Receives invoke activities with Activity name of 'signin/verifyState'
* @param context
Expand Down
2 changes: 2 additions & 0 deletions libraries/botbuilder/tests/teamsActivityHandler.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,8 @@ describe('TeamsActivityHandler', () => {
await bot.run(context);
});

console.log('>>>>>>>>>>>>> test');

await adapter.send(activity);
assert(onDialogCalled, 'onDialog handler not called');
assert(handleTeamsSigninVerifyStateCalled, 'handleTeamsSigninVerifyState handler not called');
Expand Down