fix: fix bot-sso race condition issue #929

yukun-dong · 2023-06-21T06:59:40Z

https://msazure.visualstudio.com/Microsoft%20Teams%20Extensibility/_workitems/edit/17749280/

bot-sso/helpers/ssoDialog.ts

yukun-dong · 2023-07-10T02:00:30Z

bot-sso/helpers/ssoDialog.ts

@@ -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;


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

Siglud · 2023-07-10T02:42:24Z

bot-sso/helpers/ssoDialog.ts

@@ -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;


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

Siglud · 2023-07-10T02:46:57Z

bot-sso/helpers/ssoDialog.ts

-      if (this.operationWithSSO) {
-        await this.operationWithSSO(stepContext.context, tokenResponse.ssoToken);
+      if (commandMessage) {
+        for (const key of SSOCommandMap.keys()) {


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.

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.

eriolchan · 2023-07-10T03:32:50Z

bot-sso/teamsapp.yml

@@ -130,3 +131,4 @@ publish:
    # the specified environment variable(s).
    writeToEnvironmentFile:
      publishedAppId: TEAMS_APP_PUBLISHED_APP_ID
+projectId: 45b97f10-8aa9-4386-8175-60f02bc1d97b


why do we add project id here?

eriolchan · 2023-07-10T03:34:16Z

bot-sso/env/.env.local

-TEAMS_APP_ID=
-TEAMS_APP_TENANT_ID=
-AAD_APP_ACCESS_AS_USER_PERMISSION_ID=
+BOT_ID=25536365-03fc-4c2e-b608-2b195d86e3d6


why do we check in real information? Should it be generated by local run?

Yes they shouldn't be added here. They are removed now.

eriolchan · 2023-07-10T03:34:29Z

bot-sso/env/.env.local.user

Revert "refactor: update" This reverts commit a7cc9e8530b93be288f1d9b4874f4013403b5b09. update

bot-sso/commands/index.ts

eriolchan · 2023-07-10T05:49:03Z

bot-sso/helpers/ssoDialog.ts

@@ -101,14 +92,21 @@ export class SSODialog extends ComponentDialog {

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


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

Thanks, updated.

eriolchan · 2023-07-10T05:55:40Z

bot-sso/helpers/ssoDialog.ts

    if (!tokenResponse || !tokenResponse.ssoToken) {
      await stepContext.context.sendActivity(
        "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) {


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?

bot-sso/helpers/ssoDialog.ts

eriolchan · 2023-07-10T05:58:46Z

bot-sso/helpers/ssoDialog.ts

+          if (this.expressionMatchesText(SSOCommand.matchPatterns, commandMessage)) {
+            const operationWithSSO = SSOCommand.operationWithSSOToken;
+            await operationWithSSO(stepContext.context, tokenResponse.ssoToken);
+            return await stepContext.endDialog();


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.

eriolchan · 2023-07-10T06:04:50Z

bot-sso/helpers/ssoDialog.ts

+    return text;
+  }
+
+  private expressionMatchesText(matchPatterns: MatchTerm[], userInput: string): RegExpExecArray | boolean {


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?

eriolchan · 2023-07-11T08:31:15Z

bot-sso/helpers/ssoDialog.ts

+    const operationWithSSO = SSOCommandMap.get(stepContext.options.commandMessage);
+    if (operationWithSSO) {
+      await operationWithSSO(stepContext.context, tokenResponse.ssoToken);
+      return await stepContext.endDialog();
    }
    return await stepContext.endDialog();


It looks like Line 114 is duplicated?

fix: fix race condition

0c9d4f7

yukun-dong requested a review from eriolchan as a code owner June 21, 2023 06:59

refactor: format

950f896

JerryYangKai previously approved these changes Jun 21, 2023

View reviewed changes

hund030 reviewed Jun 21, 2023

View reviewed changes

bot-sso/helpers/ssoDialog.ts Outdated Show resolved Hide resolved

refactor: update

988db47

yukun-dong dismissed JerryYangKai’s stale review via 988db47 June 21, 2023 08:05

yukun-dong force-pushed the yukun/bot-sso branch from c62e1f9 to 07ebbcb Compare July 7, 2023 07:24

refactor: update

69b5fc2

yukun-dong force-pushed the yukun/bot-sso branch from fe5f37d to 69b5fc2 Compare July 7, 2023 07:46

refactor: format

ae84c3b

yukun-dong commented Jul 10, 2023

View reviewed changes

Siglud reviewed Jul 10, 2023

View reviewed changes

eriolchan reviewed Jul 10, 2023

View reviewed changes

refactor: update

08ca20e

Revert "refactor: update" This reverts commit a7cc9e8530b93be288f1d9b4874f4013403b5b09. update

yukun-dong force-pushed the yukun/bot-sso branch from a7cc9e8 to 08ca20e Compare July 10, 2023 04:21

refactor: update

1efd55e

Siglud previously approved these changes Jul 10, 2023

View reviewed changes

eriolchan reviewed Jul 10, 2023

View reviewed changes

refactor: update

4189b83

yukun-dong dismissed Siglud’s stale review via 4189b83 July 10, 2023 06:24

yukun-dong added 2 commits July 10, 2023 14:57

refactor: use string for command's text

fff928b

refactor: prune import

a646b6b

eriolchan reviewed Jul 11, 2023

View reviewed changes

refactor: update

1110da4

yukun-dong force-pushed the yukun/bot-sso branch from 43db036 to 1110da4 Compare July 11, 2023 09:04

eriolchan previously approved these changes Jul 12, 2023

View reviewed changes

refactor: update

8b17b44

yukun-dong dismissed eriolchan’s stale review via 8b17b44 July 12, 2023 01:46

eriolchan approved these changes Jul 12, 2023

View reviewed changes

yukun-dong merged commit 6bc7939 into dev Jul 12, 2023
4 checks passed

yukun-dong deleted the yukun/bot-sso branch July 12, 2023 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix bot-sso race condition issue #929

fix: fix bot-sso race condition issue #929

yukun-dong commented Jun 21, 2023

yukun-dong Jul 10, 2023

Siglud Jul 10, 2023

Siglud Jul 10, 2023

yukun-dong Jul 10, 2023

eriolchan Jul 10, 2023

eriolchan Jul 10, 2023

yukun-dong Jul 10, 2023

eriolchan Jul 10, 2023

eriolchan Jul 10, 2023

yukun-dong Jul 10, 2023

eriolchan Jul 10, 2023

eriolchan Jul 10, 2023

eriolchan Jul 10, 2023

eriolchan Jul 11, 2023

fix: fix bot-sso race condition issue #929

fix: fix bot-sso race condition issue #929

Conversation

yukun-dong commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment