Skip to content

fix: add MCP V2 audience support and prevent removal by setup blueprint - #373

Merged
Pujarini Mohapatra (biswapm) merged 7 commits into
mainfrom
pmohapatra-MCPV2-fix
Apr 22, 2026
Merged

Pujarini Mohapatra (biswapm) merged 7 commits into
mainfrom
pmohapatra-MCPV2-fix

Conversation

@biswapm

@biswapm Pujarini Mohapatra (biswapm) commented Apr 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Root cause: RemoveStaleCustomPermissionsAsync had a hard-coded protectedIds list that only covered V1/known resource app IDs. V2 MCP audience GUIDs (set by setup permissions mcp / setup all) were treated as stale custom permissions and deleted whenever setup blueprint re-ran.
  • develop add-permissions: only issued a single AddRequiredResourceAccessAsync call against the shared ATG app ID, ignoring per-server V2 audience GUIDs entirely.

Changes

  • McpConstants: add V2ScopeValue, IsV1Scope(), ResolveAudienceOrAtgFallback() — centralizes audience normalization (null / api://-prefixed / "default" → ATG AppId; everything else is a V2 GUID)
  • ManifestHelper: add GetScopesByAudienceAsync() returning Dictionary<string, string[]> (resourceAppId → scopes); V1 entries resolve to ATG, V2 GUID entries get their own key
  • PermissionsSubcommand.ConfigureMcpPermissionsAsync: replace single ATG ResourcePermissionSpec with per-audience specs from GetScopesByAudienceAsync, fixing V2 inheritable-permission configuration in setup permissions mcp
  • PermissionsSubcommand.RemoveStaleCustomPermissionsAsync: extend protectedIds with all audience IDs from the manifest — re-running setup blueprint no longer removes V2 inheritable permissions
  • AddPermissionsSubcommand: when reading from manifest, use GetScopesByAudienceAsync and call AddRequiredResourceAccessAsync per audience (V1 + V2); explicit --scopes path unchanged

Backward compatibility

  • V1-only manifests produce the same single ATG spec as before — no behavior change
  • Existing agents: RemoveStaleCustomPermissionsAsync is idempotent; BatchPermissionsOrchestrator skips already-configured permissions
  • setup blueprint flow is unchanged — no new steps, no new auth prompts

Test plan

  • Build succeeds with 0 warnings
  • All 1306 unit tests pass
  • Manual: run setup permissions mcp with a V2 manifest, verify V2 audiences are configured as inheritable permissions
  • Manual: re-run setup blueprint, verify V2 inheritable permissions are preserved
  • Manual: run develop add-permissions with a V2 manifest, verify per-audience requiredResourceAccess entries are added

- McpConstants: add V2ScopeValue, IsV1Scope(), ResolveAudienceOrAtgFallback()
  to centralize audience normalization (null/api://-prefix/"default" → ATG AppId)
- ManifestHelper: add GetScopesByAudienceAsync() returning scopes grouped by
  resolved audience (resourceAppId) — V1 entries map to ATG, V2 GUID entries
  get their own key
- PermissionsSubcommand.ConfigureMcpPermissionsAsync: replace single ATG
  ResourcePermissionSpec with per-audience specs from GetScopesByAudienceAsync,
  fixing V2 inheritable-permission configuration in 'setup permissions mcp'
- PermissionsSubcommand.RemoveStaleCustomPermissionsAsync: extend protectedIds
  with all audience IDs from the manifest so re-running 'setup blueprint'
  no longer removes V2 inheritable permissions set by 'setup permissions mcp'
- AddPermissionsSubcommand: when reading from manifest, use GetScopesByAudienceAsync
  and call AddRequiredResourceAccessAsync per audience (V1 + V2); explicit
  --scopes path unchanged (still targets ATG)
Copilot AI review requested due to automatic review settings April 17, 2026 08:28

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

Adds MCP V2 (per-audience AppId) support across manifest parsing and permission configuration so V2 inheritable permissions aren’t removed when setup blueprint is re-run, and so develop add-permissions can add requiredResourceAccess per audience.

Changes:

  • Introduces manifest scope grouping by resolved audience (resourceAppId) via ManifestHelper.GetScopesByAudienceAsync.
  • Updates setup permissions mcp and develop add-permissions to apply permissions per audience (V1 + V2), and protects manifest audiences from stale-permission cleanup.
  • Adds MCP audience normalization helpers/constants in McpConstants.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
src/Microsoft.Agents.A365.DevTools.Cli/Helpers/ManifestHelper.cs Adds GetScopesByAudienceAsync to group manifest scopes by resolved audience.
src/Microsoft.Agents.A365.DevTools.Cli/Constants/McpConstants.cs Adds V2 scope constant and audience/scope helpers used by manifest parsing.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs Configures MCP permissions per audience and protects manifest audiences from stale custom-permission removal.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopSubcommands/AddPermissionsSubcommand.cs Adds requiredResourceAccess per audience when reading from manifest; retains single-resource behavior for explicit --scopes.

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Helpers/ManifestHelper.cs
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Helpers/ManifestHelper.cs Outdated
Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Constants/McpConstants.cs Outdated
Resolved conflicts in PermissionsSubcommand.cs (kept both: migration branch
--remove-legacy-scopes feature AND V2 audience protection in
RemoveStaleCustomPermissionsAsync). Removed duplicate declarations of
V2ScopeValue/IsV1Scope/ResolveAudienceOrAtgFallback in McpConstants.cs and
duplicate GetScopesByAudienceAsync in ManifestHelper.cs introduced by auto-merge.
@github-actions

github-actions Bot commented Apr 17, 2026 •

Copy link
Copy Markdown

⚠️ Deprecation Warning: The deny-licenses option is deprecated for possible removal in the next major release. For more information, see issue 997.

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

…Async

GetScopesByAudienceAsync always seeds McpServersMetadata.Read.All under the
ATG key when excludeLegacyAtg=false, so Count==0 was never reachable.
Copilot AI review requested due to automatic review settings April 17, 2026 09:00

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 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs:447

  • GetScopesByAudienceAsync() normalizes legacy/unset audiences to McpConstants.WorkIQToolsProdAppId, but this method now uses the returned keys directly as ResourcePermissionSpec.ResourceAppId. If the CLI is configured to use a non-default Agent365 Tools AppId via ConfigConstants.GetAgent365ToolsResourceAppId(setupConfig.Environment) (e.g., env var override), V1/legacy manifest entries would be configured against the wrong resource app id. Consider re-keying/merging the ATG entry from WorkIQToolsProdAppId to the environment-resolved ATG app id before building specs.
            var manifestPath = Path.Combine(setupConfig.DeploymentProjectPath ?? string.Empty, McpConstants.ToolingManifestFileName);

            var scopesByAudience = await ManifestHelper.GetScopesByAudienceAsync(
                manifestPath, excludeLegacyAtg: removeLegacyAtgScopes);

            // Validate all scopes are known: V1 pattern, V2 value, or metadata scope
            var unknownScopes = scopesByAudience.Values
                .SelectMany(s => s)
                .Where(s =>
                    !McpConstants.IsV1Scope(s) &&
                    !string.Equals(s, McpConstants.V2ScopeValue, StringComparison.OrdinalIgnoreCase) &&
                    !string.Equals(s, "McpServersMetadata.Read.All", StringComparison.OrdinalIgnoreCase))
                .Distinct(StringComparer.OrdinalIgnoreCase)
                .ToList();

            if (unknownScopes.Count > 0)
            {
                foreach (var unknownScope in unknownScopes)
                    logger.LogError("Unknown scope '{Scope}'. Re-run: a365 develop add-mcp-servers.", unknownScope);
                return false;
            }

            var specs = scopesByAudience
                .Select(kvp => new ResourcePermissionSpec(
                    kvp.Key, "Agent 365 Tools", kvp.Value, SetInheritable: true))
                .ToList();

…ation

ResolveAudienceOrAtgFallback and GetScopesByAudienceAsync hard-coded
WorkIQToolsProdAppId, causing permissions to target the wrong resource app
when A365_MCP_APP_ID_* env var overrides are active.

- Add ResolveAudienceOrAtgFallback(audience, atgAppId) overload; existing
  no-arg overload delegates to it (backward compatible).
- Add resolvedAtgAppId param to GetScopesByAudienceAsync and
  GetServerNamesByAudienceAsync (default null = prod constant).
- All callers with setupConfig.Environment now pass
  ConfigConstants.GetAgent365ToolsResourceAppId(environment) so the
  resolved value flows through manifest parsing and audience grouping.
Copilot AI review requested due to automatic review settings April 17, 2026 13:42

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 8 out of 8 changed files in this pull request and generated 2 comments.

Comment thread scripts/cli/Auth/New-Agent365ToolsServicePrincipalProdPublic.ps1
…s new non-DW setup path

Resolved conflicts in 3 files:
- AddPermissionsSubcommand: kept V2 per-audience GetScopesByAudienceAsync logic; applied main's
  cosmetic log message style (removed [SUCCESS]/[FAILED]/[ERROR] prefixes)
- AdminSubcommand: took main's --blueprint-id config-free path + agent instance registration;
  removed old inline spec-building (now delegated to BuildConfiguredPermissionSpecsAsync)
- AllSubcommand: took main's refactored step-method structure; preserved resolvedAtgAppId fix
  in BuildPermissionSpecsAsync

Also applied the resolvedAtgAppId fix to SetupHelpers.BuildConfiguredPermissionSpecsAsync
so AdminSubcommand's config-dir path uses the correct environment-resolved ATG app ID.
@biswapm

Copy link
Copy Markdown
Contributor Author

Copilot resolve the merge conflicts in this pull request

Copilot AI commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Copilot resolve the merge conflicts in this pull request

Merge conflicts are resolved on this PR branch in commit 5b47eac. I merged latest origin/main and reconciled conflicts in AddPermissionsSubcommand, AdminSubcommand, and AllSubcommand, then validated with a Release build and targeted tests. No UI changes were made in this update, so there is no UI screenshot to provide.

@biswapm
Pujarini Mohapatra (biswapm) merged commit 462e0c2 into main Apr 22, 2026
8 checks passed
@biswapm
Pujarini Mohapatra (biswapm) deleted the pmohapatra-MCPV2-fix branch April 22, 2026 02:37
Sellakumaran Kanagarathnam (sellakumaran) added a commit that referenced this pull request Apr 23, 2026
Reconciles the Teams Graph migration with main's Azure App Service infra
removal (#379), non-DW --agent-name flow (#365), managerApplications
attribute (#372), MCP V2 audience support (#373), and related changes.

Key resolutions:
- BotConfigurator.cs modify/delete: kept the delete; main's changes to
  it (making Location optional in the ABS payload) don't apply to Teams
  Graph.
- --m365 opt-in flag, TeamsGraphBackendConfigurator, SkippedDueToRollout,
  and rollout-cutoff date preserved intact.
- ExecuteAllCleanupAsync no longer takes a configurator; --agent-name
  /--tenant-id/--yes flags from main and their bootstrap config path
  preserved.
- UpdateEndpointAsync kept in simplified clear-then-set form; obsolete
  ABS regression test (Step 1/Step 1.5 pattern) removed.
- SetupHelpers.RegisterBlueprintMessagingEndpointAsync kept in its
  simplified form returning EndpointRegistrationResult; endpoint-name
  length check dropped.
- SetupContext renamed to use backendConfigurator; matching test-only
  callers updated.

Tests: 1316 passed, 0 failed, 13 skipped.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.

5 participants