Fix MCP UAMI persistence and dynamic-values identity threading#9265
Fix MCP UAMI persistence and dynamic-values identity threading#9265AbodeSaafan wants to merge 9 commits into
Conversation
…mption) Bug 1: After picking a UAMI in the MCP connection creation panel, the MCP server node on the canvas reverted to system-assigned identity. The connection-update thunk re-derived identity from the Logic App's identity config, which failed for hybrid 'SystemAssigned, UserAssigned' apps. Now reads the identity stored on the connection first. Bug 2: listMcpTools (and other dynamic-value calls) in the v1 designer ran without the selected identity. Now threads the identity from connectionReference.connectionProperties.authentication.identity through getListDynamicValues, and _buildMcpAuthentication prefers an explicit override over the Logic App identity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔒 AI Validation PendingThis PR is from an external contributor. AI validation will be performed after manual review by a maintainer. Maintainers: Add the |
There was a problem hiding this comment.
Pull request overview
This PR fixes managed identity (UAMI) propagation for MCP server connections and MCP dynamic-values in the v1 designer by preferring the identity stored on the connection (across both single-auth and multi-auth storage shapes) and threading the selected identity through dynamic-values calls and service-layer authentication building.
Changes:
- Add
getManagedIdentityFromConnectionhelper and use it to preserve a user-selected UAMI when re-hydrating connection properties. - Thread an optional
identitythroughgetDynamicValues→getListDynamicValues(query + cache key) → connector service calls. - Add/extend unit tests validating identity persistence, cache separation by identity, and service-layer MSI auth priority behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libs/logic-apps-shared/src/designer-client-services/lib/consumption/connector.ts | Adds identity override/effective identity selection so MCP MSI auth keeps the selected UAMI (including hybrid identity apps). |
| libs/logic-apps-shared/src/designer-client-services/lib/consumption/tests/connector.spec.ts | Adds tests covering identity propagation and MSI identity resolution priority for MCP auth. |
| libs/designer/src/lib/core/utils/parameters/dynamicdata.ts | Extracts selected identity from connectionReference and forwards it into dynamic-values fetching. |
| libs/designer/src/lib/core/utils/connectors/connections.ts | Introduces getManagedIdentityFromConnection helper to read identity from both connection parameter storage shapes. |
| libs/designer/src/lib/core/utils/connectors/test/connections.spec.ts | New unit tests validating the helper across multi-auth/single-auth shapes and precedence. |
| libs/designer/src/lib/core/queries/connector.ts | Extends getListDynamicValues to accept identity, include it in the cache key, and forward it to the connector service. |
| [ | ||
| 'listdynamicvalues', | ||
| (connectionId ?? '').toLowerCase(), | ||
| connectorId.toLowerCase(), | ||
| operationId.toLowerCase(), | ||
| dynamicState.operationId?.toLowerCase(), | ||
| getParametersKey({ ...dynamicState.parameters, ...parameters }), | ||
| identity ?? '', | ||
| ], |
There was a problem hiding this comment.
Good catch normalized to lowercase in the cache key (a85b8e5) so case-variant ARM IDs collapse to the same entry. The service call still passes the raw identity to preserve the canonical ID. Added a test in connector.spec.ts that asserts a case-variant second call is a cache hit.
📊 Coverage CheckThe following changed files need attention:
Please add tests for the uncovered files before merging. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three v2-specific gaps still left UAMI broken after publish: - mcpToolWizard.tsx: getListDynamicValues was missing the identity arg, so listMcpTools fetched tools without the selected UAMI. - consumption connector managed branch used bare identity instead of the effective identity resolved from parameter values/connection. - ParseReduxAction reconstructed connectionReferences from $connections but dropped connectionProperties, connectionRuntimeUrl, authentication, and impersonation. This explained the 'works only after publish' symptom: dynamic data could not see the chosen identity until a fresh load via the API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… in v2 Built-in MCP connections returned from Azure store their parameters in parameterValueSet.values.<key>.value when a non-default authentication set is selected (e.g. managedServiceIdentity). The consumption connector's getListDynamicValues branch only checked the flat parameterValues shape, so UAMI connections fell through to the managed-MCP path and listMcpTools was called without identity. Add a _flattenConnectionParameters helper that merges both shapes (parameterValueSet first, then parameterValues for any keys not already set) and use it for branch detection and authentication building. Updated tests exercise the real parameterValueSet-only Azure shape instead of the redundant dual-shape fixture that previously masked the bug. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Found another root cause for the v2 issue you hit at https://localhost:4200/v2 pushed as Root cause: Built-in MCP connections that use UAMI are returned by Azure in the Fix: Added No changes needed to the v2 |
Managed MCP connections persist UAMI at properties.parameterValues.authentication.identity, which the existing reader helpers (flat parameterValueSet / parameterValues paths only) missed. As a result, listMcpTools was posted with no identity even after the user had explicitly picked a UAMI on the existing connection. Extends both getManagedIdentityFromConnection (v1 + v2) and the consumption connector's listMcpTools identity resolution with a third fallback that reads parameterValues.authentication.identity, gated on authentication.type === 'ManagedServiceIdentity' to avoid misreading other auth shapes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pushed 313a51d with a fix for the v2 issue you reported (calling Root cause: Fix: added a third fallback that reads
Added 5 tests (positive nested-shape + negative type-guard in v1/v2 helpers, plus a connector test mirroring the existing managed-MCP UAMI test with the nested shape). All 40 tests in the 3 specs pass. Out of scope: the pre-publish 401 you saw when manually injecting the identity is a backend requirement (workflow must be published before |
…wizard Microsoft.Web/connections resources do not persist the UAMI ARM id for managed-identity parameter sets, so reading the resource is not enough to recover the user-chosen UAMI for a freshly created connection. Capture the identity from CreatedConnectionPayload (both legacy authentication.identity and the multi-auth connectionProperties.authentication.identity used by managed MCP connectors) into wizard state, fall back to the connection resource where it exists, and thread it through addOperation so the canvas node and listMcpTools both bind to the chosen identity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pushed c5f3243 with the actual fix after reproducing locally. Root cause (corrected from earlier guess in this PR): Fix:
Known limitation: picking an existing UAMI connection from the table for a managed MCP connector still cannot recover the UAMI from the resource alone. A wizard-level identity dropdown would be needed for full coverage ΓÇö calling that out as a follow-up rather than expanding scope here. Out of scope here: the "401 unless the workflow is published first" behavior is a save/binding concern on the run side, not designer state ΓÇö tracking separately. All 243 designer-v2 tests in the related suites pass. Comments from previous review are addressed in-scope. |
Custom MCP connectors (e.g. Stephen's MCP) declare connectionParameterSets with names like `oauthMI` instead of `ManagedServiceIdentity`, so the prior name-only gate skipped MSI connection-properties assembly and the UAMI never reached listMcpTools or the canvas node. Detect the identity parameter by inspecting the selected set's parameters for a managedIdentity type or identitypicker editor, falling back to the legacy `ManagedServiceIdentity` name check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
takyyon
left a comment
takyyon
left a comment
There was a problem hiding this comment.
Principal Architect review — generalized observations
Inline comments above flag the per-file concerns. Below are lower-priority items and observations that don't have a clean inline anchor.
CI is red on 4 checks
validate-prfailed in 3 seconds (configuration / approval gate, not a test)vscode-e2e (p43-customcode)failed after ~23 minvscode-e2e (p43-inlinejavascript)failed after ~22 minvscode-e2e-summaryfailed
These don't look like flakes given the run lengths. Worth investigating whether they're unrelated infra noise or actual regressions on the wiring path before merge — particularly given the touch on ParseReduxAction.ts (deserialization is in the e2e blast radius for Consumption workflows).
Coverage drops flagged by CI bot
libs/designer-v2/src/lib/core/actions/bjsworkflow/add.ts— 11%libs/designer-v2/src/lib/core/actions/bjsworkflow/connections.ts— 37%
The new connectionIdentity thread through addOperation → initializeOperationDetails → trySetDefaultConnectionForNode has no direct test — only the wizard-level mock in mcpToolWizard.spec.tsx. The identity-dispatch branch in trySetDefaultConnectionForNode is the most fragile new code in the PR and deserves a dedicated test in add.spec.ts exercising preferredConnectionIdentity set vs unset (would also cover the inline guard ask).
No E2E test added
Test plan has [ ] E2E tests added/updated unchecked. Four root causes across seven commits in a multi-layer wiring fix is exactly the shape that justifies one. The next regression in this area will be the second time this bug is fixed here. Strongly recommend at least one e2e covering: open MCP wizard → create new UAMI connection → save → verify the canvas node binds to the picked UAMI.
Stale state risk in mcpToolWizard
createdIdentity is reset on handleConnectionSelect (good), but not when the wizard re-opens with the same connection ID from a prior session. If a user creates a UAMI connection, saves, reopens the wizard pointed at the same connection, then proceeds — they could inherit the stale captured identity from the prior session. Safer to clear on a useEffect when the wizard step transitions back to the create step.
Pre-existing rough edges (not blocking)
_buildMcpAuthenticationmulti-UAMI ambiguity: when the Logic App has multiple UAMIs and no override/stored identity, the fallback picks the first key fromuserAssignedIdentitiesarbitrarily. Pre-existing behavior, but worth a follow-up —SystemAssigned, UserAssignedapps with 2+ UAMIs are a real customer config and they have no deterministic say in which one is chosen here.
Documentation gap
The actual root cause of the v2 issue (Microsoft.Web/connections resources do not persist the UAMI ARM id for managed-identity parameter sets like oauthMI) is documented only in this PR's discussion thread. Add an inline comment at the consumer sites (mcpToolWizard.tsx, createConnectionInternal.tsx, or wherever a future contributor lands first) noting:
Some
Microsoft.Web/connectionsparameter sets (e.g. multi-authoauthMI) do not persist the UAMI ARM id in the resource. The create-connection payload is the only authoritative source. Reading the connection back will produceundefinedfor these cases.
Without this, the next person debugging this will repeat the same 4-round-trip discovery process.
What's working well (worth preserving)
- New
getManagedIdentityFromConnectionis well-scoped, well-tested across all 3 storage shapes, and includes a negative guard for non-MSI auth types. - Editor/type-driven parameter detection in
createConnectionInternal.tsx(instead of name-matching on'ManagedServiceIdentity') is the right pattern and will pay forward for any future custom MSI connector. - The
_flattenConnectionParametershelper consolidates the dual-shape dance that was previously inlined — modulo the rename/JSDoc ask in the inline comment. - v1/v2 parity is maintained explicitly; helper is mirrored in both
libs/designerandlibs/designer-v2consistent with the rest of the repo. - Defense-in-depth (read helper → cache key → service call → wizard capture) means a single layer failing won't silently regress the whole path.
Verdict: Y-with-fixes. Strong fix for the named bugs; the 7-commit iteration shows real debugging rigor. Resolve CI before merge; address Finding #1 (silent behavior change in ParseReduxAction.ts) and Finding #4 (silent UX failure in mcpToolWizard.tsx) before merge or in a same-sprint follow-up; #2, #3, and #5 are good cleanups defensible to land separately.
…ng, e2e coverage - mcpToolWizard: useEffect resets createdIdentity on entry to CREATE_CONNECTION so a stale identity from a prior create attempt cannot leak into the next one - consumption/connector._buildMcpAuthentication: log a Warning when picking from multiple UAMIs without an explicit identity; SystemAssigned and the SystemAssigned+UserAssigned hybrid keep their existing fall-through (no identity => backend uses SAMI) - Add 4 connector.spec cases covering SAMI / hybrid / multi-UAMI paths - Fix stale assertions in buildConnectionReferencesFromConnectionsParameter.spec that still expected the dropped legacy spread - Add AgentWithMcpUami.json mock + LogicAppSelector entry + mcpSerialization.spec case covering UAMI round-trip from definition.parameters.\.value through state to serialized connectionReferences.connectionProperties Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the thorough review, @takyyon! Round-2 commit
E — CI. The four reds left on the PR are external/unrelated:
Inline threads on items 1–5 have individual replies confirming what was addressed (and what was deferred — the inline-dropdown re-pick UX from #4 is tracked as a follow-up). Ready for re-review when you have a moment. |
🤖 AI PR Validation ReportPR Review ResultsThank you for your submission! Here's detailed feedback on your PR title and body compliance:✅ PR Title
✅ Commit Type
✅ Risk Level
✅ What & Why
✅ Impact of Change
✅ Test Plan
|
| Section | Status | Recommendation |
|---|---|---|
| Title | ✅ | No change needed |
| Commit Type | ✅ | No change needed |
| Risk Level | ✅ | No change needed |
| What & Why | ✅ | No change needed |
| Impact of Change | ✅ | No change needed |
| Test Plan | ✅ | No change needed |
| Contributors | Add credits if applicable | |
| Screenshots/Videos | Optional, but useful for UI changes |
Your PR passes review for title and body compliance.
Note: I did not find any mismatch that would require raising the advised risk above the submitted Medium level.
Last updated: Tue, 16 Jun 2026 21:24:32 GMT
Commit Type
Risk Level
What & Why
Two MCP connection bugs in the Consumption designer (v1 and v2):
Bug 1: MCP server node reverts to system-assigned after saving a UAMI connection. The connection thunk re-derived the identity from the Logic App via
WorkflowService().getAppIdentity()with a strictequals(type, ''UserAssigned'')check, dropping the user''s UAMI selection (and failing entirely for hybridSystemAssigned, UserAssignedidentities), so the canvas node fell back to system-assigned.Bug 2:
listMcpToolswas called without the UAMI identity.getListDynamicValueshardcoded the identity toundefined, so both the cache key and the service call omitted the user''s selected identity, causing tool listings to use the wrong auth.Bug 3 (v2-only): Custom MCP connectors using non-standard parameter-set names (e.g.
oauthMI) bypassed the identity gate.createConnectionInternal.tsxonly built MSI connection properties when the parameter set was literally namedManagedServiceIdentity, so custom connectors lost the UAMI selection at creation time and the wizard had nothing to thread intolistMcpToolsor the canvas node.Fix
Connection persistence (v1 + v2):
getManagedIdentityFromConnectionhelper reads the stored identity from both connection-storage shapes (multi-authparameterValueSet.values.identity.valueand single-authparameterValues.identity).getConnectionPropertiesIfRequiredprefers the stored connection identity, falling back to the Logic App identity only when none is stored.Dynamic values threading (v1 + v2):
getListDynamicValuesaccepts an optionalidentityarg, includes it (lowercased) in the cache key, and forwards it to the service.dynamicdata.tsextracts the identity fromconnectionReference.connectionProperties.authentication.identityand threads it through.v2 MCP wizard:
mcpToolWizard.tsxcapturescreatedIdentityfrom the create-connection payload and threadsselectedIdentitythroughlistMcpToolsandaddOperation, so the canvas node binds to the picked UAMI.add.tsaccepts aconnectionIdentityand writes it to the operation''s connection reference.createConnectionInternal.tsxdetects the identity parameter dynamically (byConnectionParameterTypes.managedIdentitytype oridentitypickereditor) instead of matching only on the legacyManagedServiceIdentityparameter-set name, so custom connectors likeoauthMIwork.Service-layer defense-in-depth:
_buildMcpAuthenticationaccepts anidentityOverrideand the caller computes aneffectiveIdentitypriority chain that supports both flat (parameters.identity) and nested (connectionProperties.authentication.identity) payload shapes.Impact of Change
listMcpToolscalls use the right identity. Custom MCP connectors with non-standard parameter-set names now work end-to-end.getManagedIdentityFromConnectionhelper.getListDynamicValues,_buildMcpAuthentication, andaddOperationgained optional identity args. Identity parameter detection increateConnectionInternalis now editor/type-driven.Test Plan
13 new unit tests across both v1 and v2 (plus existing wizard and createConnection suites still green):
connections.spec.ts(v1 + v2) - 5 tests each for the helper across both storage shapesconnector.spec.ts(designer + designer-v2) - 3 tests for identity threading, cache-key separation, and case-insensitive cache reuseconnector.spec.ts(logic-apps-shared) - 5 tests for_buildMcpAuthenticationpriority and caller identity selectionmcpToolWizard.spec.tsx- existing tests updated for newselectedIdentityplumbingFull targeted runs green locally.
Contributors
Screenshots/Videos