fix(notification): default azure credential usage#1604
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the sample’s Azure Identity authentication approach, aiming to fix how managed identity credentials are created and used across the notification workflow.
Changes:
- Replaced
DefaultAzureCredentialwithManagedIdentityCredentialfor Table Storage and Service Bus access in the Azure Functions code. - Updated
@azure/identitydependency version. - Adjusted the mock installation data script and README instructions related to credentials.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| large-scale-notification/src/internal/tableStore.ts | Switches Table Storage auth to ManagedIdentityCredential and reformats calls. |
| large-scale-notification/src/functions/durableFunctionHttpTrigger.ts | Uses ManagedIdentityCredential when creating the Service Bus admin client. |
| large-scale-notification/src/functions/enqueueTasksForInstallationsActivity.ts | Uses ManagedIdentityCredential for Service Bus clients; formatting tweaks. |
| large-scale-notification/src/functions/waitSendingFinishActivity.ts | Uses ManagedIdentityCredential for Service Bus admin client; formatting tweaks. |
| large-scale-notification/script/mockInstallationData.ts | Changes how DefaultAzureCredential is constructed for the script. |
| large-scale-notification/package.json | Bumps @azure/identity version. |
| large-scale-notification/README.md | Updates instructions for running the mock installation script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `${storageAccountURL}`, | ||
| `${storageTableName}`, | ||
| credential, | ||
| { allowInsecureConnection: true } | ||
| { allowInsecureConnection: true }, | ||
| ); |
There was a problem hiding this comment.
allowInsecureConnection: true allows plain HTTP connections to Table Storage endpoints, which is unsafe for deployed environments and can enable credential/token exposure. Prefer omitting this option (default secure behavior) or gating it behind an explicit local/emulator flag while keeping production URLs HTTPS-only.
| const credential = new DefaultAzureCredential({ | ||
| requiredEnvVars: ["AZURE_TOKEN_CREDENTIALS"], | ||
| }); |
There was a problem hiding this comment.
DefaultAzureCredential options don’t appear to use a requiredEnvVars property, and AZURE_TOKEN_CREDENTIALS isn’t referenced elsewhere in this repo. As written, this likely has no effect (or fails type-checking under ts-node) and makes authentication behavior unclear; consider reverting to new DefaultAzureCredential() or using a specific credential type (e.g., Azure CLI/service principal) and documenting the expected env vars.
| const credential = new DefaultAzureCredential({ | |
| requiredEnvVars: ["AZURE_TOKEN_CREDENTIALS"], | |
| }); | |
| const credential = new DefaultAzureCredential(); |
|
|
||
| 1. Copy the value of `STORAGE_ACCOUNT_NAME` in `env/.env.dev` and paste it to variable `storageAccount` in `script/mockInstallationData.ts`. | ||
| 2. Please ensure you are logged into the Azure account used for deploying the app. | ||
| 2. Please ensure you are logged into the Azure account used for deploying the app and set environment variable `AZURE_TOKEN_CREDENTIALS` to `dev`. |
There was a problem hiding this comment.
The README now instructs setting AZURE_TOKEN_CREDENTIALS=dev, but this environment variable isn’t part of the standard Azure Identity configuration and isn’t used anywhere except being listed in the script’s DefaultAzureCredential options. Please update these instructions to reflect an actual supported auth flow (e.g., Azure CLI login or AZURE_TENANT_ID/AZURE_CLIENT_ID/AZURE_CLIENT_SECRET) or remove the requirement if it’s not needed.
| 2. Please ensure you are logged into the Azure account used for deploying the app and set environment variable `AZURE_TOKEN_CREDENTIALS` to `dev`. | |
| 2. Ensure you are authenticated to the Azure account used for deploying the app (for example, by running `az login` so that `DefaultAzureCredential` can use your Azure CLI credentials). |
There was a problem hiding this comment.
Pull Request
Description
Fix the usage of
DefaultAzureCredentialType of Change
For New Sample Onboarding
Checklist
.config/samples-config-v3.jsonValidation Results (Required)
For Internal Samples (source code in this repo)
For External Samples (source code in another repo)
Related Issues
if any question related to validation, may refer to the Sample Validation Guide
if still has questions, may open a issue :)