Check for connection strings and managed identity settings on deployment #4423
Conversation
| return undefined; | ||
| } | ||
|
|
||
| function checkForEmulatorSettings(property: { [propertyName: string]: string }): ConnectionSetting | undefined { |
There was a problem hiding this comment.
In the future, if we need to add more emulator strings, we'll have to remember to update them here as well. I'm wondering if it might be more foolproof to create a centralized list of emulator strings as part of our constants and then loop over that list instead to perform the checks. This way, we would only need to maintain the single list at the centralized location. What do you think?
There was a problem hiding this comment.
Yeah that's a good idea.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces checks for connection strings and managed identity settings during deployment, aiming to alert users when insecure configurations are detected.
- Updated verifyAppSettings to accept externally provided appSettings instead of fetching them from the client.
- Added new logic in getWarningsForConnectionSettings to inspect both local and remote settings and return warning messages.
- Enhanced deploy.ts to incorporate deployment warnings and adjusted local settings file retrieval for consistency.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/commands/deploy/verifyAppSettings.ts | Use provided appSettings via options instead of fetching them remotely. |
| src/commands/deploy/getWarningsForConnectionSettings.ts | New functions to analyze connection settings and generate warnings. |
| src/commands/deploy/deploy.ts | Integrates warning messages into the deploy confirmation flow. |
| src/commands/appSettings/localSettings/getLocalSettingsFile.ts | Renamed function for consistency in local settings retrieval. |
| src/commands/appSettings/localSettings/LocalSettingsClient.ts | Updated import to match the renamed local settings file function. |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (2)
src/commands/deploy/getWarningsForConnectionSettings.ts:47
- [nitpick] Consider renaming 'checkForConnectionSettings' to something like 'checkForConvertibleConnectionString' to more clearly indicate that the function targets connection string settings.
function checkForConnectionSettings(property: { [propertyName: string]: string }): ConnectionSetting | undefined {
src/commands/deploy/getWarningsForConnectionSettings.ts:75
- [nitpick] Extract the emulator connection string and event hub regular expression checks into clearly named constants or helper functions for improved readability and maintainability.
if (property.value.includes(localStorageEmulatorConnectionString) ||
This PR is checking the local settings of the project being deployed and the remote settings of the app being deployed to. It is classifying the setting type as 'ConnectionString' | 'ManagedIdentity' | 'Emulator'.
Based on the settings, it will warn the user on deploy about the following two scenarios:
The warnings are just to alert the user that they should use managed identities, but it doesn't offer a way to convert their local settings yet. After Megan's PR, we may be able to offer a way to add the settings for them though.
I originally was trying to implement this matrix:
But I ran into a few of issues with trying to do this:
First of all, I think it's somewhat difficult for us to know if a local setting is the equivalent of a remote setting. I think technically the connection string can be compared to something that is hooked up to a managed identity, but I'm not sure how accurate it is. Not to mention, there are potentially many connection strings to one app, and it seemed like a lot to try to get it working 100% correctly.
Secondly, even if we do set up the managed identity for the user, there's no guarantee that the roles will be configured correctly. This could leave the user thinking that once they assign the MI, their app is configured correctly which is not the case. This is something we should revisit once we are able to assign roles.
I think I'm going to go back and make the default behavior for deploying to include the local settings with it. In those cases, we will not deploy anything that is an emulator string.