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

fix: fix bot-sso race condition issue #929

Merged
merged 12 commits into from
Jul 12, 2023
Merged
11 changes: 10 additions & 1 deletion bot-sso/commands/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { BotCommand } from "../helpers/botCommand";
import { BotCommand, MatchTerm, SSOCommand } from "../helpers/botCommand";
import { LearnCommand } from "./learn";
import { ShowUserProfile } from "./showUserProfile";
import { WelcomeCommand } from "./welcome";
eriolchan marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -8,3 +8,12 @@ export const commands: BotCommand[] = [
new ShowUserProfile(),
new WelcomeCommand(),
];

export const SSOCommands: SSOCommand[] = [
new ShowUserProfile(),
];

export const SSOCommandMap: Map<MatchTerm[], any> = new Map();
SSOCommands.forEach((command) => {
SSOCommandMap.set(command.matchPatterns, command.operationWithSSOToken);
});
1 change: 0 additions & 1 deletion bot-sso/helpers/botCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ export class SSOCommand extends BotCommand {
async run(parameters: any): Promise<any> {
this.validateParameters(parameters);
const ssoDialog = parameters.ssoDialog;
ssoDialog.setSSOOperation(this.operationWithSSOToken);
await ssoDialog.run(parameters.context, parameters.dialogState);
}
}
63 changes: 45 additions & 18 deletions bot-sso/helpers/ssoDialog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
ComponentDialog,
} from "botbuilder-dialogs";
import {
Activity,
ActivityTypes,
Storage,
tokenExchangeOperationName,
Expand All @@ -15,6 +16,8 @@ import { TeamsBotSsoPrompt } from "@microsoft/teamsfx";
import "isomorphic-fetch";
import oboAuthConfig from "../authConfig";
import config from "../config";
import { SSOCommandMap } from "../commands";
import { MatchTerm } from "./botCommand";

const DIALOG_NAME = "SSODialog";
const MAIN_WATERFALL_DIALOG = "MainWaterfallDialog";
Expand All @@ -24,17 +27,13 @@ export class SSODialog extends ComponentDialog {
private requiredScopes: string[] = ["User.Read"]; // hard code the scopes for demo purpose only
private dedupStorage: Storage;
private dedupStorageKeys: string[];
private operationWithSSO: (
arg0: any,
ssoToken: string
) => Promise<any> | undefined;

// Developer controlls the lifecycle of credential provider, as well as the cache in it.
// In this sample the provider is shared in all conversations
constructor(dedupStorage: Storage) {
super(DIALOG_NAME);

const initialLoginEndpoint =`https://${config.botDomain}/auth-start.html` ;
const initialLoginEndpoint = `https://${config.botDomain}/auth-start.html`;

const dialog = new TeamsBotSsoPrompt(
oboAuthConfig,
Expand All @@ -60,16 +59,6 @@ export class SSODialog extends ComponentDialog {
this.dedupStorageKeys = [];
}

setSSOOperation(
handler: (arg0: any, arg1: string) => Promise<any> | undefined
) {
this.operationWithSSO = handler;
}

resetSSOOperation() {
this.operationWithSSO = undefined;
}

/**
* The run method handles the incoming activity (in the form of a DialogContext) and passes it through the dialog system.
* If no dialog is active, it will start the default dialog.
Expand All @@ -87,6 +76,9 @@ export class SSODialog extends ComponentDialog {
}

async ssoStep(stepContext: any) {
const turnContext = stepContext.context as TurnContext;
const text = this.getActivityText(turnContext.activity);
stepContext.options.commandMessage = text;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set user command in stepContext so that it can be retrived in later steps.

Copy link
Member

Choose a reason for hiding this comment

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

Local variable 'text' is redundant. Just use
stepContext.options.commandMessage = this.getActivityText(turnContext.activity);

return await stepContext.beginDialog(TEAMS_SSO_PROMPT_ID);
}

Expand All @@ -101,14 +93,21 @@ export class SSODialog extends ComponentDialog {

async executeOperationWithSSO(stepContext: any) {
const tokenResponse = stepContext.result;
const commandMessage = stepContext.options.commandMessage;
Copy link
Contributor

Choose a reason for hiding this comment

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

this line can be moved to else section (line 102)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated.

if (!tokenResponse || !tokenResponse.ssoToken) {
await stepContext.context.sendActivity(
eriolchan marked this conversation as resolved.
Show resolved Hide resolved
"There is an issue while trying to sign you in and retrieve your profile photo, please type \"show\" command to login and consent permissions again."
);
} else {
// Once got ssoToken, run operation that depends on ssoToken
if (this.operationWithSSO) {
await this.operationWithSSO(stepContext.context, tokenResponse.ssoToken);
if (commandMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we already save the command id in context, why can't we directly get command from it? Why do we need to for each SSO commands list?

for (const key of SSOCommandMap.keys()) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like some bad code smell here. Iterator in a map is a time-consuming action (cause some of the bucket in this map is empty, you know). Consider compute the key first, then get from the map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key cannot be calculated since the keys are arrays of string | regexp, so I need to iterate all keys and check if the commandMessage matches the key.

if (this.expressionMatchesText(key, commandMessage)) {
const operationWithSSO = SSOCommandMap.get(key);
await operationWithSSO(stepContext.context, tokenResponse.ssoToken);
return await stepContext.endDialog();
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already return end dialog in Line 114, it is better either to return early to reduce additional if...else layer, or remove Line 107 to keep Line 114 as the shared ending step.

}
}
}
}
return await stepContext.endDialog();
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like Line 114 is duplicated?

Expand All @@ -123,7 +122,6 @@ export class SSODialog extends ComponentDialog {
this.dedupStorageKeys = this.dedupStorageKeys.filter(
(key) => key.indexOf(conversationId) < 0
);
this.resetSSOOperation();
}

// If a user is signed into multiple Teams clients, the Bot might receive a "signin/tokenExchange" from each client.
Expand Down Expand Up @@ -173,4 +171,33 @@ export class SSODialog extends ComponentDialog {
}
return `${channelId}/${conversationId}/${value.id}`;
}

private getActivityText(activity: Activity): string {
let text = activity.text;
const removedMentionText = TurnContext.removeRecipientMention(activity);
if (removedMentionText) {
text = removedMentionText
.toLowerCase()
.replace(/\n|\r\n/g, "")
.trim();
}
return text;
}

private expressionMatchesText(matchPatterns: MatchTerm[], userInput: string): RegExpExecArray | boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a little complicated for user to understand and it is easy to be error prone. Is it better to use a map to connect command text with command function?

let matchResult: RegExpExecArray | boolean;
for (const pattern of matchPatterns) {
if (typeof pattern == "string") {
matchResult = new RegExp(pattern).exec(userInput);
} else if (pattern instanceof RegExp) {
matchResult = pattern.exec(userInput);
} else {
matchResult = pattern(userInput);
}
if (matchResult) {
return matchResult;
}
}
return false;
}
}
2 changes: 1 addition & 1 deletion bot-sso/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,4 @@
"nodemon": "^2.0.7",
"shx": "^0.3.3"
}
}
}