Improve Entra Connection Startup and Prevent Duplicate Consent Prompts - #22614
Improve Entra Connection Startup and Prevent Duplicate Consent Prompts#22614Chris Johnstone (cjohnsto-nz) wants to merge 4 commits into
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR optimizes startup and authentication for saved Entra-authenticated connections by avoiding expensive tenant enumeration when a tenant is already known, coalescing duplicate permission prompts, and adding connection-flow tracing to better pinpoint delays across the connection pipeline.
Changes:
- Avoids eager tenant enumeration during VS Code account/token resolution and adds trace-phase logging around token acquisition.
- Adds correlation-style tracing across connection dialog actions and Object Explorer session creation; loads saved connections/groups concurrently during startup.
- Coalesces concurrent connection-sharing permission prompts for the same extension to prevent duplicate user prompts.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/mssql/test/unit/vscodeEntraMfaUtils.test.ts | Adds unit coverage for account options, explicit-tenant behavior, and token trace logging. |
| extensions/mssql/test/unit/connectionSharingService.test.ts | Adds test coverage for coalescing in-flight permission prompts. |
| extensions/mssql/test/unit/connectionManager.test.ts | Adds test asserting connections and groups load concurrently during initialization. |
| extensions/mssql/test/unit/connectionDialogWebviewController.test.ts | Adds tests ensuring tenant enumeration isn’t done in background and validates correlation payload logging behavior. |
| extensions/mssql/test/unit/azureHelpers.test.ts | Updates expectations for getAccounts(false) usage. |
| extensions/mssql/src/webviews/pages/ConnectionDialog/connectionDialogStateProvider.tsx | Sends click correlation payload for connect action and logs click event. |
| extensions/mssql/src/sharedInterfaces/connectionDialog.ts | Extends connect reducer payload to optionally include click correlation fields. |
| extensions/mssql/src/objectExplorer/objectExplorerService.ts | Adds correlation trace capture/logging for Object Explorer expansion/session creation phases. |
| extensions/mssql/src/objectExplorer/objectExplorerProvider.ts | Exposes recordConnectionClick to pass correlation into Object Explorer service. |
| extensions/mssql/src/controllers/mainController.ts | Records Object Explorer selection “click” correlation IDs and wires them into the provider. |
| extensions/mssql/src/controllers/connectionManager.ts | Adds phased connection tracing, concurrent startup loading, and token-request tracing IDs. |
| extensions/mssql/src/connectionSharing/connectionSharingService.ts | Coalesces concurrent permission requests per extension to prevent duplicate prompts. |
| extensions/mssql/src/connectionconfig/connectionDialogWebviewController.ts | Accepts correlation payload for connect and adds tracing for submit/test/reveal phases; avoids background tenant preloading. |
| extensions/mssql/src/connectionconfig/azureHelpers.ts | Uses getAccounts(false) consistently to avoid unnecessary work. |
| extensions/mssql/src/azure/vscodeEntraMfaUtils.ts | Adds explicit-tenant fast path and trace-phase logging for account/tenant/session resolution. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (3)
extensions/mssql/src/connectionconfig/connectionDialogWebviewController.ts:403
transportDurationMsaccepts any finiteclickTimestampthat is <= now, including negative values. A forged/invalid negative timestamp would produce an arbitrarily large duration and pollute connection-trace logs. Clamp to epoch-style timestamps by requiringclickTimestamp >= 0(or another lower bound) before computing the duration.
const transportDurationMs =
typeof payload.clickTimestamp === "number" &&
Number.isFinite(payload.clickTimestamp) &&
payload.clickTimestamp <= now
? now - payload.clickTimestamp
: undefined;
extensions/mssql/src/controllers/mainController.ts:1270
- The
onDidChangeSelectionhandler logs "connection clicked", but this event fires for any selection change (including keyboard navigation or programmatic selection viareveal). This makes the trace message misleading and can create confusing correlation logs. Rename the log message to reflect selection rather than a click.
const correlationId = uuid();
this._logger.debug(
`[ConnectionTrace] Object Explorer connection clicked correlationId=${correlationId} durationMs=0`,
);
this._objectExplorerProvider.recordConnectionClick(node, correlationId);
extensions/mssql/src/azure/vscodeEntraMfaUtils.ts:147
- When an explicit
tenantIdis provided, tenant enumeration is skipped unconditionally. This means a malformed tenantId (not a GUID) will now be passed through togetSessionFromVSCode, which can cause token acquisition failures where previouslygetDefaultTenantId(...)would have been used. Consider validating the explicit tenantId shape and falling back to default tenant resolution when it’s clearly invalid.
let resolvedTenantId = tenantId;
if (!resolvedTenantId) {
resolvedTenantId = await traceTokenPhase(
trace,
"tenant resolution",
async () => {
const tenants = await VsCodeAzureHelper.getTenantsForAccount(account);
return getDefaultTenantId(account.id, tenants);
},
(selectedTenantId) => `tenantResolved=${selectedTenantId !== undefined}`,
);
} else {
trace?.logger.debug(
`[ConnectionTrace] VS Code accounts tenant resolution skipped requestId=${trace.requestId} reason=explicitTenantId`,
);
}
|
Thanks for working on these improvements. This PR combines several separate changes: Entra token resolution, connection startup and connection-sharing permission prompt handling. Since connection management is a core and sensitive area of the extension, I think these changes would be easier to review and validate as separate, focused PRs. Could you create tracking issues for the individual problems and address them one at a time? Adding Benjin Dubishar (@Benjin) for additional context since he is the primary maintainer for this area. |
|
Aasim Khan (@aasimkhan30) Benjin Dubishar (@Benjin) |
|
Aasim Khan (@aasimkhan30) Benjin Dubishar (@Benjin) |
Description
This PR improves startup performance and authentication reliability for saved Entra-authenticated SQL connections.
For multi account VS Code configurations with multiple tenants, this reduces the connection time from several minutes to a few seconds, and reduces excessive permission prompting for connections that have already been granted permission.
It addresses several sources of unnecessary startup work and duplicate prompts:
These changes reduce avoidable client-side delays while preserving authentication fallback, connection error handling, query connection behavior, and Object Explorer semantics.
No related issue is linked to this PR.
Code Changes Checklist
Validation
All repository extension test suites passed with coverage reporting disabled:
3377 passing,12 pending198 passing,6 pending22 passingAdditional validation:
npm run build -- --target mssqlgit diff --check main..HEADThe default
npm run testcommand runs the tests successfully but exits with a coverage-reporting error when the coverage writer encounters an external source-map path containinghttps:\. The test suites themselves pass;--coverage=falsewas used to obtain a clean exit code.Additional Notes
The larger SQL Tools Service Object Explorer session delay remains outside the extension's client-side control. This PR does not make Object Explorer appear ready before the general query connection is established, avoiding query-readiness races and preserving current connection semantics.
The changes are split into three logic commits:
Optimize Entra account and token resolutionTrace and optimize connection startupCoalesce connection sharing permission promptsReviewers
Please read the reviewer guidelines.