[JS] Add support for MSI in the CQA sample#439
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for authenticating the custom Q&A Bot using either an Endpoint Key or a User Managed Identity. The codebase, configuration, and documentation are updated to accept and handle a managed identity client ID, with the bot's main dialog and adapter logic modified accordingly. The README is expanded with detailed managed identity setup instructions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bot (WebApp)
participant Azure Language Resource
User->>Bot (WebApp): Sends message
Bot (WebApp)->>Bot (WebApp): Reads .env for EndpointKey or ManagedIdentityClientId
alt Using Endpoint Key
Bot (WebApp)->>Azure Language Resource: Authenticates with Endpoint Key
else Using Managed Identity
Bot (WebApp)->>Azure AD: Obtains token using ManagedIdentityClientId
Bot (WebApp)->>Azure Language Resource: Authenticates with AAD token
end
Azure Language Resource-->>Bot (WebApp): Returns answer
Bot (WebApp)-->>User: Responds with answer
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
samples/javascript_nodejs/48.customQABot-all-features/.env (1)
7-8: Consider adding a comment to explain the new MSI configuration variable.To improve clarity for users configuring the bot, consider adding a comment explaining when to use this variable.
LanguageEndpointKey= +# LanguageManagedIdentityClientId - Client ID of User Managed Identity for authentication (alternative to LanguageEndpointKey) LanguageManagedIdentityClientId=samples/javascript_nodejs/48.customQABot-all-features/README.md (1)
32-62: Excellent documentation for MSI authentication! Fix minor indentation issues.The documentation comprehensively covers both authentication methods. However, there are some indentation inconsistencies that should be fixed.
Fix the indentation to maintain consistency:
2. Using a `User Managed Identity` resource: _provides a more complex configuration by using a User Managed Identity resource. Great way to authenticate without the need of a secret_. - Create a [User Managed Identity][create-msi] resource in the same region as the Language resource. - - Copy the `ClientId` as value of `LanguageManagedIdentityClientId` in [.env file](.env). + - Copy the `ClientId` as value of `LanguageManagedIdentityClientId` in [.env file](.env). - In the [Azure Portal][Azure], go to the WebApp resource, where the bot is hosted. - Go to `Identity` under Settings and select `User assigned`. More information on Identity assignment can be found [here][webapp-msi]. - Click on `Add` and select the User Managed Identity created in the previous step. - Click `Save` to assign the User Managed Identity to the WebApp resource. - - This will allow the WebApp to communicate with the Language resource using the User Managed Identity. + - This will allow the WebApp to communicate with the Language resource using the User Managed Identity. - In the [Azure Portal][Azure], go to the Language resource. - Assign the following role in the `Access Control (IAM)` section. More information on role assignment can be found [here][language-custom-role]. - - `Cognitive Services User`: _this role is required so the Managed Identity can access the keys of the Cognitive Service resource_. + - `Cognitive Services User`: _this role is required so the Managed Identity can access the keys of the Cognitive Service resource_.🧰 Tools
🪛 LanguageTool
[uncategorized] ~54-~54: Loose punctuation mark.
Context: ...m-role]. -Cognitive Services User: _this role is required so the Managed I...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
46-46: Unordered list indentation
Expected: 2; Actual: 3(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
samples/javascript_nodejs/48.customQABot-all-features/index.js (1)
72-75: Consider improving the error message for better clarity.The validation logic is correct, but the error message could be more descriptive to help users understand their configuration options.
if (!managedIdentityClientId?.trim() && !endpointKey?.trim()) { - throw new Error('Either LanguageManagedIdentityClientId or LanguageEndpointKey should be set.'); + throw new Error('Authentication configuration missing: Please set either LanguageManagedIdentityClientId (for MSI authentication) or LanguageEndpointKey (for key-based authentication) in your .env file.'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
samples/javascript_nodejs/48.customQABot-all-features/.env(1 hunks)samples/javascript_nodejs/48.customQABot-all-features/README.md(2 hunks)samples/javascript_nodejs/48.customQABot-all-features/dialogs/rootDialog.js(3 hunks)samples/javascript_nodejs/48.customQABot-all-features/index.js(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
samples/javascript_nodejs/48.customQABot-all-features/dialogs/rootDialog.js (2)
samples/javascript_nodejs/48.customQABot-all-features/index.js (2)
endpointKey(70-70)managedIdentityClientId(67-67)samples/csharp_dotnetcore/48.customQABot-all-features/Dialogs/RootDialog.cs (1)
QnAMakerDialog(48-93)
🪛 LanguageTool
samples/javascript_nodejs/48.customQABot-all-features/README.md
[uncategorized] ~54-~54: Loose punctuation mark.
Context: ...m-role]. - Cognitive Services User: _this role is required so the Managed I...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
samples/javascript_nodejs/48.customQABot-all-features/README.md
46-46: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (7)
samples/javascript_nodejs/48.customQABot-all-features/index.js (2)
19-28: Good migration to CloudAdapter for MSI support.The transition from
BotFrameworkAdaptertoCloudAdapterwithConfigurationBotFrameworkAuthenticationis the correct approach for supporting managed identity authentication.
92-104: Correct server configuration for CloudAdapter.The addition of body parser middleware and the use of
adapter.processmethod are the proper patterns for CloudAdapter.samples/javascript_nodejs/48.customQABot-all-features/dialogs/rootDialog.js (5)
31-31: LGTM! Function signature correctly updated for MSI support.The addition of the
managedIdentityClientIdparameter in the appropriate position maintains parameter order consistency.
72-73: LGTM! JSDoc documentation accurately reflects the optional parameters.The documentation correctly marks both authentication parameters as optional and provides clear descriptions of their purpose.
80-80: LGTM! Constructor signature properly updated.The constructor signature correctly accepts the new
managedIdentityClientIdparameter in the appropriate position.
87-87: LGTM! Function call correctly passes the new parameter.The call to
createQnAMakerDialogproperly includes the newmanagedIdentityClientIdparameter.
39-39:Details
❓ Verification inconclusive
Verify that passing
undefinedfor endpointKey is handled correctly by QnAMakerDialog.The change to pass
undefinedinstead ofendpointKeyto the constructor is logical since authentication will be configured later, but we should ensure the QnAMakerDialog constructor properly handlesundefinedfor this parameter.
🏁 Script executed:
#!/bin/bash # Description: Verify QnAMakerDialog constructor handling of undefined endpointKey # Expected: Constructor should accept undefined endpointKey parameter rg -A 10 -B 5 "class QnAMakerDialog" --type js rg -A 20 "constructor.*endpointKey" --type jsLength of output: 5210
Validate undefined endpointKey handling in QnAMakerDialog
The
createQnAMakerDialoghelper wraps the externalQnAMakerDialogconstructor (from@microsoft/botbuilder-ai), so its logic isn’t in our repo. Please confirm that passingundefinedfor theendpointKeyargument is supported (when using a managed identity) by reviewing theQnAMakerDialogsource in your dependencies or the official SDK documentation.• File: samples/javascript_nodejs/48.customQABot-all-features/dialogs/rootDialog.js
Line 39:undefined,
| if (managedIdentityClientId) { | ||
| qnaMakerDialog.withManagedIdentityClientId(managedIdentityClientId); | ||
| } else { | ||
| qnaMakerDialog.withEndpointKey(endpointKey); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation to ensure at least one authentication method is provided.
The authentication selection logic is well-implemented, but there's a potential issue: if both managedIdentityClientId and endpointKey are undefined, the code will call withEndpointKey(undefined) which may cause runtime errors.
Consider adding validation to ensure at least one authentication method is provided:
+ if (!managedIdentityClientId && !endpointKey) {
+ throw new Error('Either managedIdentityClientId or endpointKey must be provided for QnA Maker authentication.');
+ }
+
if (managedIdentityClientId) {
qnaMakerDialog.withManagedIdentityClientId(managedIdentityClientId);
} else {
qnaMakerDialog.withEndpointKey(endpointKey);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (managedIdentityClientId) { | |
| qnaMakerDialog.withManagedIdentityClientId(managedIdentityClientId); | |
| } else { | |
| qnaMakerDialog.withEndpointKey(endpointKey); | |
| } | |
| if (!managedIdentityClientId && !endpointKey) { | |
| throw new Error('Either managedIdentityClientId or endpointKey must be provided for QnA Maker authentication.'); | |
| } | |
| if (managedIdentityClientId) { | |
| qnaMakerDialog.withManagedIdentityClientId(managedIdentityClientId); | |
| } else { | |
| qnaMakerDialog.withEndpointKey(endpointKey); | |
| } |
🤖 Prompt for AI Agents
In samples/javascript_nodejs/48.customQABot-all-features/dialogs/rootDialog.js
around lines 51 to 55, add validation before the authentication logic to check
that either managedIdentityClientId or endpointKey is defined. If neither is
provided, throw an error or handle the case gracefully to prevent calling
withEndpointKey with an undefined value, which could cause runtime errors. This
ensures that the code only proceeds when a valid authentication method is
available.
|
/promoted 4049 |
2 participants
Important
This PR requires the feature introduced in [# 4879](http s://github.com/microsoft/botbuilder- js/pull/4879)
Description
This PR adds support for User Managed Identity to the CQA sample. Providing a way to choose between using the EndpointKey or the MSI resource to authenticate to the Language service.
Proposed Changes
index.jsto receive the new LanguageManagedIdentityClientId property. Additionally, the deprecated BotFrameworkAdapter was replaced by CloudAdapter.RootDialogto support both EndpointKey and MSI auth type for CQA resource.README.mdto include the new MSI option and the required configuration..envfile to include the new LanguageManagedIdentityClientId property.Testing
The following image shows both types of authentication working.
