Fixed a bug where OAuth cards were opening the file explorer if ngrok… (

#2155) * Fixed a bug where OAuth cards were opening the file explorer if ngrok was not configured. * Lint fixes. * Fixed test.
microsoft · Jun 9, 2020 · c5937fc · c5937fc
1 parent a5be796
commit c5937fc
Show file tree

Hide file tree

Showing 9 changed files with 32 additions and 158 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## Fixed
 - [client] Added missing content to signed in view of Cosmos DB service dialog and fixed product page link in PR [2150](https://github.com/microsoft/BotFramework-Emulator/pull/2150)
 - [docs] Modified CONTRIBUTING.md to include updated information about global dependencies required to build from source in PR [2153](https://github.com/microsoft/BotFramework-Emulator/pull/2153)
+- [client] Fixed a bug where trying to open the sign-in link on an OAuth card would open the file explorer if ngrok was not configured in PR [2155](https://github.com/microsoft/BotFramework-Emulator/pull/2155)
 
 ## v4.9.0 - 2020 - 05 - 11
 ## Added

diff --git a/packages/app/client/src/hyperlinkHandler.ts b/packages/app/client/src/hyperlinkHandler.ts
@@ -47,9 +47,9 @@ export class HyperlinkHandler {
     const { TrackEvent } = SharedConstants.Commands.Telemetry;
     try {
       const parsed = URL.parse(url) || { protocol: '' };
-      if ((parsed.protocol || '').startsWith('oauth:')) {
+      if ((parsed.protocol || '').startsWith(SharedConstants.EmulatedOAuthUrlProtocol)) {
         this.navigateEmulatedOAuthUrl(url.substring(8));
-      } else if (parsed.protocol.startsWith('oauthlink:')) {
+      } else if (parsed.protocol.startsWith(SharedConstants.OAuthUrlProtocol)) {
         this.navigateOAuthUrl(url.substring(12));
       } else {
         this.commandService.remoteCall(TrackEvent, 'app_openLink', { url }).catch(_e => void 0);

diff --git a/packages/app/main/src/server/routes/channel/userToken/handlers/emulateOAuthCards.spec.ts b/packages/app/main/src/server/routes/channel/userToken/handlers/emulateOAuthCards.spec.ts
diff --git a/packages/app/main/src/server/routes/channel/userToken/handlers/emulateOAuthCards.ts b/packages/app/main/src/server/routes/channel/userToken/handlers/emulateOAuthCards.ts
diff --git a/packages/app/main/src/server/routes/channel/userToken/mountUserTokenRoutes.spec.ts b/packages/app/main/src/server/routes/channel/userToken/mountUserTokenRoutes.spec.ts
@@ -36,7 +36,6 @@ import { createBotFrameworkAuthenticationMiddleware } from '../../handlers/botFr
 import { createJsonBodyParserMiddleware } from '../../../utils/jsonBodyParser';
 
 import { mountUserTokenRoutes } from './mountUserTokenRoutes';
-import { emulateOAuthCards } from './handlers/emulateOAuthCards';
 import { getToken } from './handlers/getToken';
 import { signOut } from './handlers/signOut';
 import { createTokenResponseHandler } from './handlers/tokenResponse';
@@ -77,12 +76,6 @@ describe('mountUserTokenRoutes', () => {
       getToken
     );
 
-    expect(emulatorServer.server.post).toHaveBeenCalledWith(
-      '/api/usertoken/emulateOAuthCards',
-      botFrameworkAuthentication,
-      emulateOAuthCards
-    );
-
     expect(emulatorServer.server.del).toHaveBeenCalledWith(
       '/api/usertoken/SignOut',
       botFrameworkAuthentication,

diff --git a/packages/app/main/src/server/routes/channel/userToken/mountUserTokenRoutes.ts b/packages/app/main/src/server/routes/channel/userToken/mountUserTokenRoutes.ts
@@ -36,7 +36,6 @@ import { createBotFrameworkAuthenticationMiddleware } from '../../handlers/botFr
 import { createJsonBodyParserMiddleware } from '../../../utils/jsonBodyParser';
 import { EmulatorRestServer } from '../../../restServer';
 
-import { emulateOAuthCards } from './handlers/emulateOAuthCards';
 import { getToken } from './handlers/getToken';
 import { signOut } from './handlers/signOut';
 import { createTokenResponseHandler } from './handlers/tokenResponse';
@@ -49,8 +48,6 @@ export function mountUserTokenRoutes(emulatorServer: EmulatorRestServer) {
 
   server.get('/api/usertoken/GetToken', botFrameworkAuthentication, getBotEndpoint, getToken);
 
-  server.post('/api/usertoken/emulateOAuthCards', botFrameworkAuthentication, emulateOAuthCards);
-
   server.del('/api/usertoken/SignOut', botFrameworkAuthentication, getBotEndpoint, signOut);
 
   server.post('/api/usertoken/tokenResponse', jsonBodyParser, createTokenResponseHandler(emulatorServer));

diff --git a/packages/app/main/src/server/utils/oauthLinkEncoder.spec.ts b/packages/app/main/src/server/utils/oauthLinkEncoder.spec.ts
@@ -32,6 +32,7 @@
 //
 
 import { AttachmentContentTypes } from '@bfemulator/sdk-shared';
+import { SharedConstants } from '@bfemulator/app-shared';
 
 import { OAuthLinkEncoder } from './oauthLinkEncoder';
 
@@ -111,15 +112,16 @@ describe('The OauthLinkEncoder', () => {
     });
   });
 
-  it('should throw when an error occurs retrieving the link while calling resolveOAuthCards', async () => {
+  it('should throw when an error occurs retrieving the link while calling resolveOAuthCards, but should also generate an emulated oauth link', async () => {
     ok = false;
     statusText = 'oh noes!';
     const mockActivity = {
       attachments: [
         {
           contentType: AttachmentContentTypes.oAuthCard,
           content: {
-            buttons: [{ type: 'signin' }],
+            buttons: [{ type: 'signin', value: undefined }],
+            connectionName: 'oauth-connection',
           },
         },
       ],
@@ -129,7 +131,11 @@ describe('The OauthLinkEncoder', () => {
       await encoder.resolveOAuthCards(mockActivity);
       expect(false);
     } catch (e) {
-      expect(e.message).toEqual(statusText);
+      expect(e.message).toEqual(`Failed to generate an actual sign-in link: Error: ${statusText}`);
+      expect(mockActivity.attachments[0].content.buttons[0].type).toBe('openUrl');
+      expect(mockActivity.attachments[0].content.buttons[0].value).toBe(
+        SharedConstants.EmulatedOAuthUrlProtocol + '//' + 'oauth-connection' + '&&&' + 'testConversation'
+      );
     }
   });
 
@@ -150,7 +156,7 @@ describe('The OauthLinkEncoder', () => {
       await encoder.resolveOAuthCards(mockActivity);
       expect(false);
     } catch (e) {
-      expect(e.message).toEqual("I'm in your throw!");
+      expect(e.message).toEqual(`Failed to generate an actual sign-in link: Error: ${"I'm in your throw!"}`);
     }
   });
 

diff --git a/packages/app/main/src/server/utils/oauthLinkEncoder.ts b/packages/app/main/src/server/utils/oauthLinkEncoder.ts
@@ -34,6 +34,7 @@
 import * as crypto from 'crypto';
 
 import { Attachment, OAuthCard } from 'botframework-schema';
+import { SharedConstants } from '@bfemulator/app-shared';
 import { AttachmentContentTypes } from '@bfemulator/sdk-shared';
 import { Activity } from 'botframework-schema';
 
@@ -42,9 +43,6 @@ import { EmulatorRestServer } from '../restServer';
 import { uniqueId } from './uniqueId';
 
 export class OAuthLinkEncoder {
-  public static OAuthUrlProtocol: string = 'oauthlink:';
-  public static EmulateOAuthCards: boolean = false;
-
   private readonly authorizationHeader: string;
   private readonly conversationId: string;
   private activity: Activity;
@@ -75,10 +73,21 @@ export class OAuthLinkEncoder {
       const oauthCard: OAuthCard = attachment.content as OAuthCard;
       if (oauthCard.buttons && oauthCard.buttons.length === 1) {
         const cardAction = oauthCard.buttons[0];
-        if (cardAction.type === 'signin' && !cardAction.value && !OAuthLinkEncoder.EmulateOAuthCards) {
-          const link = await this.getSignInLink(oauthCard.connectionName, codeChallenge);
-          cardAction.value = link;
-          cardAction.type = 'openUrl';
+        if (cardAction.type === 'signin' && !cardAction.value) {
+          // generate a sign-in link for the oauth card and assign it to the button
+          try {
+            const link = await this.getSignInLink(oauthCard.connectionName, codeChallenge);
+            cardAction.value = link;
+            cardAction.type = 'openUrl';
+          } catch (e) {
+            // failed to generate a sign-in link, fall back to an emulated sign-in token
+            const link =
+              SharedConstants.EmulatedOAuthUrlProtocol + '//' + oauthCard.connectionName + '&&&' + this.conversationId;
+            cardAction.value = link;
+            cardAction.type = 'openUrl';
+
+            throw new Error(`Failed to generate an actual sign-in link: ${e}`);
+          }
         }
       }
     }
@@ -134,7 +143,7 @@ export class OAuthLinkEncoder {
       });
       if (response.ok) {
         const link = await response.text();
-        return OAuthLinkEncoder.OAuthUrlProtocol + '//' + link + '&&&' + this.conversationId;
+        return SharedConstants.OAuthUrlProtocol + '//' + link + '&&&' + this.conversationId;
       }
       errorMessage = response.statusText;
     } catch (e) {

diff --git a/packages/app/shared/src/constants/sharedConstants.ts b/packages/app/shared/src/constants/sharedConstants.ts
@@ -39,6 +39,8 @@ export const SharedConstants = {
   TEMP_BOT_IN_MEMORY_PATH: 'TEMP_BOT_IN_MEMORY',
   EDITOR_KEY_PRIMARY,
   EDITOR_KEY_SECONDARY,
+  EmulatedOAuthUrlProtocol: 'oauth:',
+  OAuthUrlProtocol: 'oauthlink:',
 
   /** Names of commands used in both main and client */
   Commands: {