Skip to content

Improve Entra Connection Startup and Prevent Duplicate Consent Prompts - #22614

Closed
Chris Johnstone (cjohnsto-nz) wants to merge 4 commits into
microsoft:mainfrom
cjohnsto-nz:feature/startup-auth-performance
Closed

Improve Entra Connection Startup and Prevent Duplicate Consent Prompts#22614
Chris Johnstone (cjohnsto-nz) wants to merge 4 commits into
microsoft:mainfrom
cjohnsto-nz:feature/startup-auth-performance

Conversation

@cjohnsto-nz

Copy link
Copy Markdown
Contributor

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:

  • Avoids eager tenant enumeration for every VS Code account when a saved connection already contains an explicit tenant.
  • Preserves silent token acquisition followed by interactive sign-in fallback for non-happy paths.
  • Adds correlation-based logging across the connection dialog, Object Explorer, SQL Tools Service connection flow, and Entra token acquisition.
  • Loads saved connections and connection groups concurrently during startup.
  • Coalesces concurrent connection-sharing permission requests so multiple startup callers do not show duplicate approval 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

  • New or updated unit tests added
  • All existing tests pass
  • Code follows the contributing guidelines
  • Telemetry/logging updated if relevant
  • No regressions or UX breakage

Validation

All repository extension test suites passed with coverage reporting disabled:

  • MSSQL: 3377 passing, 12 pending
  • SQL Database Projects: 198 passing, 6 pending
  • Data Workspace: 22 passing

Additional validation:

  • npm run build -- --target mssql
  • git diff --check main..HEAD
  • Online MSSQL VSIX packaging completed successfully

The default npm run test command runs the tests successfully but exits with a coverage-reporting error when the coverage writer encounters an external source-map path containing https:\. The test suites themselves pass; --coverage=false was 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 resolution
  • Trace and optimize connection startup
  • Coalesce connection sharing permission prompts

Reviewers

Please read the reviewer guidelines.

@cjohnsto-nz

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread extensions/mssql/src/connectionconfig/connectionDialogWebviewController.ts Outdated
Comment thread extensions/mssql/test/unit/connectionSharingService.test.ts
Comment thread extensions/mssql/src/connectionconfig/connectionDialogWebviewController.ts Outdated
Copilot AI review requested due to automatic review settings August 5, 2026 02:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • transportDurationMs accepts any finite clickTimestamp that 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 requiring clickTimestamp >= 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 onDidChangeSelection handler logs "connection clicked", but this event fires for any selection change (including keyboard navigation or programmatic selection via reveal). 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 tenantId is provided, tenant enumeration is skipped unconditionally. This means a malformed tenantId (not a GUID) will now be passed through to getSessionFromVSCode, which can cause token acquisition failures where previously getDefaultTenantId(...) 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`,
        );
    }

@aasimkhan30

Copy link
Copy Markdown
Contributor

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.

@cjohnsto-nz

Copy link
Copy Markdown
Contributor Author

Aasim Khan (@aasimkhan30) Benjin Dubishar (@Benjin)
Happy to break this up. I will raise some smaller PRs 🙂

@cjohnsto-nz

Copy link
Copy Markdown
Contributor Author

Aasim Khan (@aasimkhan30) Benjin Dubishar (@Benjin)
It has been done!

#22615
#22616
#22617

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants