diff --git a/.claude/skills/review-staged/SKILL.md b/.claude/skills/review-staged/SKILL.md index a6f1725e..34001a54 100644 --- a/.claude/skills/review-staged/SKILL.md +++ b/.claude/skills/review-staged/SKILL.md @@ -92,7 +92,7 @@ The review does not assume docs will be updated later — a user-visible surface - **Not excuses**: "it's preview/opt-in/temporary", "Microsoft Learn will cover it", "you can see it in the diff". The CHANGELOG is the release's source of truth. ### Cross-Cutting Contract Checks -Five checks that static per-file analysis tends to miss — each requires tracing across multiple files or comparing code against docs/descriptions: +Nine checks that static per-file analysis tends to miss — each requires tracing across multiple files or comparing code against docs/descriptions: - **Return-value null semantics (Rule N)**: When a method documents that `null` (or a sentinel) carries a special meaning (e.g., "null = verified, safe to persist"), grep every call site and verify the producer returns null in exactly the documented cases. A path that returns non-null when the contract says "verified" silently breaks the caller's persistence gate. - **CHANGELOG vs code numeric consistency (Rule O)**: After reading CHANGELOG `[Unreleased]`, extract every numeric claim (interval, retry count, timeout). Grep production code for the corresponding literals. Flag any mismatch — the CHANGELOG and the code must agree. @@ -100,7 +100,41 @@ Five checks that static per-file analysis tends to miss — each requires tracin - **Test-only escape hatch declared `public` (Rule Q)**: When a property/field named `*ForTests*` or `*TestOverride*` is declared `public` in a production assembly, check the `.csproj` for `InternalsVisibleTo`. If present, `public` is unnecessary and widens the security surface — flag as MEDIUM and suggest `internal`. - **`--help` text accuracy (Rule R)**: When the diff changes how a command surfaces output (new URL handoff, removed PowerShell path, added fallback), read every description string in the same file and its parent command. If the description still names an output form that no longer matches the code, flag as MEDIUM. -Full detection rules and real examples are in `.claude/agents/pr-code-reviewer.md` Step 9, Rules N through R. +**Added 2026-05-29 after PR #432 Copilot review surfaced gaps Rules N–R didn't catch. See [.codereviews/why-review-staged-missed-copilot-findings-20260529T190832Z.md](../../codereviews/why-review-staged-missed-copilot-findings-20260529T190832Z.md) for the full miss analysis.** + +- **Branch-wide stale-mechanism sweep (Rule S)**: When the staged diff replaces mechanism X with mechanism Y (e.g., PowerShell fallback → az rest, `api://{appId}` → bare appId GUID, `per-app admin-consent URLs` → `az ad sp create`), enumerate the terms being replaced and `grep -rn` the **entire branch** (not just the staged delta) for each term. For every hit outside the staged delta, classify: + - Stale comment (FIX — code/comment lie about behavior). + - Stale user-facing log/warning/exception text (FIX — operators read these in production). + - Stale doc comment / XML doc (FIX — IDE surfaces these to consumers). + - Intentional historical reference (KEEP — e.g., commit message archaeology, CHANGELOG `[Released]` entries). + + This is the analog of "rename hygiene" applied to behavior changes. It catches the family of issues where the implementation moves but the surrounding documentation lies. **Treat each surviving reference as MEDIUM if user-facing (LogWarning, LogError, exception messages, Warnings collection entries), LOW if internal-only (private comments).** + + Six of the nine unique findings in PR #432's Copilot review were of this exact shape — all stale `"PowerShell fallback"` and `"api://{appId}"` references on lines outside the staged delta. + +- **Test-class parallelism safety (Rule T)**: For every test class in the diff (or anywhere on the branch), check whether the class reads or writes any static property or field whose name matches the pattern `*ForTests*` or `*TestOverride*` (these are the conventional escape hatches in this codebase). If yes, the class **must** carry either: + - `[Collection("Sequential")]` attribute, OR + - `[CollectionDefinition(..., DisableParallelization = true)]` for a class-specific collection it owns. + + Without one of these, xUnit may run the class in parallel with other classes that also mutate the same static state, producing flaky cross-class races. **Severity: MEDIUM.** The fix is one line. Two PR #432 findings (`BatchPermissionsOrchestratorTests`, `BatchPermissionsOrchestratorMissingSpTests`) were exactly this — both mutated `BypassSpProvisioningForTests` without `[Collection]` until Copilot flagged it. + + Implementation: `grep -rn "BypassConsentChecksForTests\|BypassSpProvisioningForTests\|OpenUrlOverrideForTests\|" `. For each hit, read the enclosing test class and verify the attribute is present. + +- **Branch-scope completeness checkpoint (Rule U)**: **The single biggest miss category in the PR #432 review.** Before declaring the review complete, list every file that appears in the **branch diff** (`git diff $(git merge-base HEAD origin/main)...HEAD --name-only`), not just the staged delta. The review must touch each file in that list at least to the extent of running the other rules against it. + + In practice this means: if the staged delta is 4 files but the branch diff is 22 files, the review surface is 22, not 4. Prior `/review-staged` runs do **not** absolve the current run of covering the rest of the branch — assume nothing has been covered until you've explicitly read it in this run. + +- **Hardening-bypass detection (Rule V)**: **Added 2026-05-29 after PR #432's second Copilot review caught a regression Rules N–U missed.** When the staged diff hardens an entry-point helper by making a parameter required (so the compiler forces it through one path), the lower-level function it delegates to typically still keeps the parameter optional — and **any direct caller of the lower-level function bypasses the hardening**. The hardening only protects the path that goes through the helper. + + Detection: when the diff includes a signature change of the shape "parameter X was optional with `= null` default, now required (no default)", identify the **lower-level function** the helper delegates to. If that function still has X optional, run `grep -rn "" --include="*.cs" src//` (production callers only) and verify each direct caller passes X. Any caller that doesn't is a recurrence of the exact bug the hardening was meant to prevent. + + Concrete PR #432 example: commit `7a1e317` made `mcpScopesByAudience` required on `ExecuteBatchPermissionsStepAsync` so the AllSubcommand and NonDwBlueprintSetupOrchestrator entry points couldn't forget it. But `ConfigureAllPermissionsAsync` (the lower-level orchestrator method) still had `knownMcpAudienceAppIds` optional, and `PermissionsSubcommand.ConfigureMcpPermissionsAsync` called it directly — bypassing the hardening and re-introducing the AADSTS500011 V2 routing regression. Copilot flagged it as HIGH; the skill missed it. + + **Severity: HIGH** when the bypassed call site is in production code (live regression risk). LOW when it's a test fixture (tests routinely use null defaults). Treat the parameter-becoming-required signature change in the diff as a trigger: the moment you see one, follow the call chain down and grep direct callers of the lower-level function. + + Implementation: at the start of the review, emit the list of branch-level files and treat them as the review surface. At the end, verify each file was read or explicitly justified as "no rule applies." + +Full detection rules and real examples are in `.claude/agents/pr-code-reviewer.md` Step 9, Rules N through V. ### Context Awareness The skill differentiates between: diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b011060..116c145a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g **Option B — CLI** (`a365 setup admin`) has been removed in this release. Use Option A above, or copy the PowerShell instructions printed in the `a365 setup all` summary output. ### Added +- `--skip-sp-provisioning` option on `setup all` — skips the interactive in-line provisioning of missing resource service principals (issue #429). Default: setup prompts per-resource and runs `az ad sp create --id ` using the operator's `az login`. With this flag, missing SPs are excluded from the consent URL and listed in the Action Required block with the `az` command and a per-SP consent URL. Implicitly enabled when stdin is redirected (CI / pipe scenarios). +- Action Required block in the `setup all` summary now lists missing service principals — each entry shows the resource, pending scopes, the `az ad sp create` command, and the per-SP consent URL needed to complete provisioning. - `logs export [command] [--output ]` — exports a redacted copy of a CLI diagnostic log safe to share with Microsoft support. Redacts JWT tokens, email addresses, OS-path usernames, and tenant-specific GUIDs; replaces identical values with consistent aliases so log correlation is preserved. Preserves diagnostic IDs that aren't sensitive but are useful for debugging — `TraceId`, `CorrelationId`, Microsoft Graph `request-id` and `client-request-id` values, and well-known public Microsoft / Agent 365 resource appIds (such as the Microsoft Graph appId `00000003-0000-0000-c000-000000000000`). Omit `[command]` to export all available logs at once. - `setup blueprint --show-secret` — displays the blueprint client secret stored in `a365.generated.config.json` in plaintext without re-running any setup steps. On Windows, decryption requires the same machine and user account that ran setup (DPAPI). When no secret is found, the command prints instructions to run `a365 setup blueprint --agent-name `. - Blueprint client secret is now printed to the terminal at creation time with a "copy this value now" warning. Use `a365 setup blueprint --show-secret` to retrieve it afterwards. @@ -49,6 +51,11 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g - `--secret-lifetime-months ` option (and matching `secretLifetimeMonths` field in the `--input-file` JSON) on `develop-mcp register-external-mcp-server` — controls the lifetime of the client secrets created on the A365Proxy and RemoteProxy Entra apps. Valid range `1-24`; omit to use the Graph default (~2 years). Calendar-aware (uses `DateTimeOffset.AddMonths`, so Jan 31 + 1 month → Feb 28/29). Added so tenants with an `appManagementPolicies` cap on client-secret lifetime — previously a hard failure inside `CreateEntraAppsAsync` with a generic "Failed to create secret" message — can fit registration inside their tenant's policy. When Graph rejects the requested (or default) lifetime with a tenant-policy error, the CLI now emits an actionable error naming the flag and the attempted value (e.g. `Tenant Entra ID policy rejected the requested 12-month lifetime ... Pass --secret-lifetime-months N with a smaller value (e.g. --secret-lifetime-months 3) that fits inside your tenant's appManagementPolicies cap.`) instead of the previous generic failure. ### Fixed +- `setup all` now exits silently on Ctrl+C instead of printing `ERROR: Setup failed: A task was canceled.` followed by a misleading partial summary. +- `setup all --m365` no longer fails with `AADSTS650053` because the Messaging Bot scope was hard-coded to scopes the resource SP does not publish (issue #429). +- `setup all` no longer fails with `AADSTS650053` for any drift between requested scopes and what a resource SP actually publishes (issue #429). Unpublished scopes are filtered out before building the consent URL; per-resource warnings surface what was dropped. +- `setup all` no longer fails with `AADSTS650052` ("organization lacks a service principal for ...") when a resource SP is missing in the tenant (issue #429). Setup now prompts per-resource and provisions the SP via `az ad sp create`. Declined / failed paths are surfaced in the Action Required block. +- `setup all` no longer fails with `AADSTS650053` or `AADSTS500011` for V2 MCP per-server audiences (issue #429). Scopes now route to the correct per-audience resource instead of collapsing onto the WorkIQ Tools URI. - `setup all` admin-consent pre-check no longer opens the browser unnecessarily when consent already exists for every required scope. The pre-check now uses `az rest` (mirroring the post-consent polling path) because the CLI's MSAL token cannot read `/v1.0/oauth2PermissionGrants` after the removal of `DelegatedPermissionGrant.Read.All` from the CLI client app — previously every re-run returned empty from the Graph check and opened a browser that waited up to 180 seconds for a no-op consent. The pre-check also filters `consentType eq 'AllPrincipals'` so a leftover `Principal`-scoped grant from an earlier `--authmode obo` run cannot falsely satisfy the tenant-wide check. App IDs are validated as GUIDs before being interpolated into the OData filter. - `setup all` summary row "Blueprint Permission Grants" no longer shows `granted` when admin consent could not be auto-verified. When the browser consent completed but the poll timed out, the row now shows "unverified" and an Action Required item with the re-grant URL is printed so the operator can verify or retry. - Admin consent polling now uses `az rest` (the Azure CLI token) to detect grants, the same path used by `a365 query-entra`. Previously the poll used an MSAL delegated token that lacked `DelegatedPermissionGrant.Read.All`, causing every poll to return empty and the timeout to expire even when consent had already been granted. @@ -93,9 +100,9 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g - `setup blueprint --m365` now prints a note when passed alone — the flag only takes effect with `--endpoint-only` or `--update-endpoint`; otherwise use `setup all --m365`. - Graph error bodies in `[DBG]` logs compressed to `{code}: {message}` instead of the full JSON envelope. - `a365.config.json` and `a365.generated.config.json` are no longer mirrored into the machine-global config folder (`%LocalAppData%\Microsoft.Agents.A365.DevTools.Cli` on Windows, `~/.config/a365` on Linux/macOS). Config is read from and written to the project directory only. Cleanup commands no longer fall back to `a365.generated.config.json` in the global folder when no project-local copy exists. MSAL token caches, CLI logs, and the version/notice caches continue to live in the global folder. -- `setup all` and `setup permissions` now automatically execute the S2S app role assignment PowerShell script when a Global Administrator runs the CLI and the programmatic Graph API path fails (the delegated token does not carry `AppRoleAssignment.ReadWrite.All`). Requires PowerShell 7+ (`pwsh`) and the `Microsoft.Graph.Authentication` / `Microsoft.Graph.Applications` modules; run `a365 setup requirements` to check and auto-install. On success, the "Action Required: S2S app role (PowerShell)" block is suppressed. If the modules are missing, the block is still printed with a note to run `a365 setup requirements`. +- `setup all` and `setup permissions` now issue the S2S app role assignment and delegated `AllPrincipals` OAuth2 consent via `az rest` against the operator's existing `az login` session, replacing the previous `Connect-MgGraph` PowerShell fallback (issue #429). `pwsh` and the Microsoft.Graph PowerShell modules are no longer required for this path. The per-prompt `[y/N]` confirmation is unchanged. - "Blueprint Permission Grants" row in the `setup all` summary now reports `already granted` (vs `granted`) when the run was fully idempotent — no new `oauth2PermissionGrant` was POSTed for delegated consent and no new `appRoleAssignment` was POSTed for S2S. Surfaces the distinction between "consent was captured in this run" and "consent existed before this run" so re-runs visually indicate no work was needed. -- PowerShell S2S fallback subprocess (`pwsh`) is now capped at 5 minutes and runs with `-ExecutionPolicy Bypass`. On Windows it also runs with `PSModulePath` / `DOTNET_ROOT` / `DOTNET_ROOT_X64` / `DOTNET_STARTUP_HOOKS` removed from the child environment to avoid `[Assembly with same name is already loaded]` failures when the parent dotnet host's paths collide with pwsh's own assembly resolution. The script pins the latest installed `Microsoft.Graph.Authentication` and `Microsoft.Graph.Applications` modules by absolute path and exits with code 2 when either module is missing. +- `setup all`: fallback consent prompt now explains the ambiguity (declined vs error) and offers to grant the permissions programmatically via `az login`; per-audience permission lists and consent URLs show per-server MCP names (e.g. `mcp_MailTools`) instead of a generic `Agent 365 Tools` for every audience. ### Removed - `a365 config` command family (`config init`, `config display`, `config permissions`) — replaced by `a365 setup all --agent-name` and `a365 setup permissions custom`. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs index 4cf45ef3..5aec500f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs @@ -146,6 +146,16 @@ public static Command CreateCommand( " both — delegated grants (OBO) and app permissions (S2S).\n" + "Not supported with --aiteammate true."); + var skipSpProvisioningOption = new Option( + "--skip-sp-provisioning", + description: "Skip the interactive in-line provisioning of missing resource service principals.\n" + + "Default: setup detects resources (e.g. V2 MCP per-server audiences) whose SP is missing\n" + + "from this tenant, prompts per-resource, and shells out to 'az ad sp create --id '\n" + + "using the operator's existing az login. With --skip-sp-provisioning, missing SPs are\n" + + "excluded from the unified admin-consent URL and surfaced as numbered items in the Action\n" + + "Required block, each with the az command and a per-SP consent URL.\n" + + "Implicitly enabled when stdin is redirected (CI / coding-agent / pipe scenarios)."); + command.AddOption(verboseOption); command.AddOption(dryRunOption); command.AddOption(skipInfrastructureOption); @@ -156,6 +166,7 @@ public static Command CreateCommand( command.AddOption(agentNameOption); command.AddOption(tenantIdOption); command.AddOption(authModeOption); + command.AddOption(skipSpProvisioningOption); command.SetHandler(async (System.CommandLine.Invocation.InvocationContext context) => { @@ -163,6 +174,11 @@ public static Command CreateCommand( var dryRun = context.ParseResult.GetValueForOption(dryRunOption); var skipInfrastructure = context.ParseResult.GetValueForOption(skipInfrastructureOption); var skipRequirements = context.ParseResult.GetValueForOption(skipRequirementsOption); + // --skip-sp-provisioning flag (off by default). Also auto-on when stdin is + // redirected so CI / coding-agent / pipe scenarios don't hang on the per-SP + // prompt loop. + var skipSpProvisioningFlag = context.ParseResult.GetValueForOption(skipSpProvisioningOption); + var skipSpProvisioning = skipSpProvisioningFlag || Console.IsInputRedirected; // Tri-state: null = not specified (respect config), true/false = explicit override. // Option means bare --aiteammate sets it to true without requiring "true" as a value. bool? aiTeammateFlag = context.ParseResult.CommandResult.FindResultFor(aiTeammateOption) != null @@ -383,7 +399,8 @@ effectiveAuthModeForValidation is not ("obo" or "s2s" or "both")) isBootstrap: isBootstrap, isM365: isM365, authMode: authMode ?? nonDwConfig.AuthMode, - confirmationProvider: confirmationProvider); + confirmationProvider: confirmationProvider, + skipSpProvisioning: skipSpProvisioning); context.ExitCode = await NonDwBlueprintSetupOrchestrator.ExecuteAsync(nonDwCtx); return; @@ -530,7 +547,8 @@ await RequirementsSubcommand.RunChecksOrExitAsync( blueprintLookupService: blueprintLookupService, federatedCredentialService: federatedCredentialService, clientAppValidator: clientAppValidator, - isM365: isM365); + isM365: isM365, + skipSpProvisioning: skipSpProvisioning); // Step 1: Infrastructure (optional, DW only) await ExecuteInfrastructureStepAsync(ctx); @@ -539,19 +557,25 @@ await RequirementsSubcommand.RunChecksOrExitAsync( await ExecuteBlueprintStepAsync(ctx); // Step 3: Configure all permissions in a batch. - var (specs, mcpResourceAppId, mcpScopes) = await BuildPermissionSpecsAsync(ctx); + var (specs, mcpResourceAppId, mcpScopes, mcpScopesByAudience, mcpServerNamesByAudience) = await BuildPermissionSpecsAsync(ctx); await ExecuteBatchPermissionsStepAsync( - ctx, specs, + ctx, specs, mcpScopesByAudience, knownBlueprintSpObjectId: ctx.Config.AgentBlueprintServicePrincipalObjectId); - SetupHelpers.ApplyConsentUrlsIfNeeded(ctx, mcpResourceAppId, ctx.Config.AgentApplicationScopes, mcpScopes, isM365: ctx.IsM365); + SetupHelpers.ApplyConsentUrlsIfNeeded( + ctx, mcpResourceAppId, ctx.Config.AgentApplicationScopes, mcpScopes, + isM365: ctx.IsM365, + mcpScopesByAudience: mcpScopesByAudience, + mcpAudienceDisplayNames: mcpServerNamesByAudience); await ctx.ConfigService.SaveStateAsync(ctx.Config, ctx.GeneratedConfigPath); // Step 4: Messaging endpoint registration — --m365 gated; no-op for non-M365 agents. await ExecuteMessagingEndpointStepAsync(ctx); + logger.LogInformation(""); + // Sync all settings (ServiceConnection, TokenValidation, Agent365Observability) to the app config file. setupResults.ProjectSettingsWritten = await ProjectSettingsSyncHelper.ExecuteAsync( ctx.ConfigFile.FullName, ctx.GeneratedConfigPath, @@ -579,6 +603,12 @@ await ExecuteBatchPermissionsStepAsync( SetupHelpers.DisplaySetupSummary(setupResults, logger); ExceptionHandler.ExitWithCleanup(1); } + catch (OperationCanceledException) + { + // Must sit before the catch-all below so Ctrl+C bypasses DisplaySetupSummary, + // which would render not-yet-attempted phases as "failed". + throw; + } catch (Exception ex) { logger.LogError(ex, "Setup failed: {Message}", ex.Message); @@ -743,8 +773,15 @@ internal static async Task ExecuteBlueprintStepAsync(SetupContext ctx) internal static async Task ExecuteBatchPermissionsStepAsync( SetupContext ctx, List specs, + IReadOnlyDictionary mcpScopesByAudience, string? knownBlueprintSpObjectId = null) { + // Required parameter — every caller must thread the loaded ToolingManifest + // audience map through. Forgetting it would route V2 MCP per-server audiences + // to api://{appId} and trigger AADSTS500011 (see commit 7a1e317's incomplete + // wiring of the non-DW path). + var knownMcpAudienceAppIds = mcpScopesByAudience.Keys.ToHashSet(StringComparer.OrdinalIgnoreCase); + try { var (blueprintPermissionsUpdated, inheritedPermissionsConfigured, consentGranted, adminConsentUrl) = @@ -754,7 +791,9 @@ await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( specs, ctx.Logger, ctx.Results, ctx.CancellationToken, knownBlueprintSpObjectId: knownBlueprintSpObjectId, confirmationProvider: ctx.ConfirmationProvider, - commandExecutor: ctx.Executor); + commandExecutor: ctx.Executor, + skipSpProvisioning: ctx.SkipSpProvisioning, + knownMcpAudienceAppIds: knownMcpAudienceAppIds); ctx.Results.BatchPermissionsPhase1Completed = blueprintPermissionsUpdated; ctx.Results.BatchPermissionsPhase2Completed = inheritedPermissionsConfigured; @@ -792,6 +831,10 @@ internal static async Task ExecuteMessagingEndpointStepAsync(SetupContext ctx) if (!ctx.IsM365) return; + // Phase separator: emit only after the non-M365 early-return so the non-M365 run + // does not get a stray blank line followed by silent no-op output. + ctx.Logger.LogInformation(""); + // Blueprint step failed; there is no blueprint to attach an endpoint to. Record this as // a distinct Failed + "BlueprintMissing" so the summary doesn't mislead the user with the // "non-M365 agent" wording reserved for null. @@ -845,7 +888,7 @@ internal static async Task ExecuteMessagingEndpointStepAsync(SetupContext ctx) /// Shared by both DW and non-DW flows so permissions are always consistent — the only difference /// is that non-M365 agents exclude Messaging Bot API. /// - internal static async Task<(List specs, string mcpResourceAppId, string[] mcpScopes)> BuildPermissionSpecsAsync(SetupContext ctx) + internal static async Task<(List specs, string mcpResourceAppId, string[] mcpScopes, Dictionary scopesByAudience, Dictionary> serverNamesByAudience)> BuildPermissionSpecsAsync(SetupContext ctx) { var desiredCustomIds = new HashSet( (ctx.Config.CustomBlueprintPermissions ?? new List()) @@ -859,16 +902,27 @@ await PermissionsSubcommand.RemoveStaleCustomPermissionsAsync( McpConstants.ToolingManifestFileName); var mcpResourceAppId = ConfigConstants.GetAgent365ToolsResourceAppId(ctx.Config.Environment); var scopesByAudience = await ManifestHelper.GetScopesByAudienceAsync(mcpManifestPath, excludeLegacyAtg: false, resolvedAtgAppId: mcpResourceAppId); + var serverNamesByAudience = await ManifestHelper.GetServerNamesByAudienceAsync(mcpManifestPath, mcpResourceAppId); // V1-compatible: extract ATG scopes for consent URL helpers (empty for V2-only manifests) var mcpScopes = scopesByAudience.TryGetValue(mcpResourceAppId, out var atgScopes) ? atgScopes : Array.Empty(); - // Pass the already-computed scopesByAudience to avoid reading the MCP manifest twice. - // BuildConfiguredPermissionSpecsAsync stamps Graph + manifest MCP audiences + fixed APIs - // (Bot only when isM365) + custom permissions for both DW and non-DW agents. + // Pass the already-computed scopesByAudience and serverNamesByAudience to avoid + // reading the MCP manifest a second time. BuildConfiguredPermissionSpecsAsync stamps + // Graph + manifest MCP audiences + fixed APIs (Bot only when isM365) + custom permissions + // for both DW and non-DW agents; serverNamesByAudience drives the per-server display + // names so V2 audiences read as e.g. "mcp_MailTools" rather than "Agent 365 Tools". var specs = await SetupHelpers.BuildConfiguredPermissionSpecsAsync( - ctx.Config, setInheritable: true, isM365: ctx.IsM365, scopesByAudience); - - return (specs, mcpResourceAppId, mcpScopes); + ctx.Config, setInheritable: true, isM365: ctx.IsM365, scopesByAudience, serverNamesByAudience); + + // Return the full scopesByAudience map alongside the V1-compat mcpScopes so V2 + // callers (ApplyConsentUrlsIfNeeded) can route per-server audiences to the bare + // appId GUID resource identifier instead of collapsing them onto the WorkIQ Tools + // URI (issue #429). api://{appId} is NOT used — per-server SPs have identifierUris + // null and only the bare appId GUID is in servicePrincipalNames, so api:// triggers + // AADSTS500011. serverNamesByAudience flows through to ApplyConsentUrlsIfNeeded so + // the Action Required block's per-audience consent URLs display the same per-server + // names the spec list uses. + return (specs, mcpResourceAppId, mcpScopes, scopesByAudience, serverNamesByAudience); } /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestConsentRunner.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestConsentRunner.cs new file mode 100644 index 00000000..5baa5e09 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestConsentRunner.cs @@ -0,0 +1,328 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging; +using System.Text.Json; +using System.Text.RegularExpressions; + +namespace Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; + +/// +/// Grants delegated admin consent on the agent blueprint service principal by shelling out +/// to az rest against the Graph oauth2PermissionGrants endpoint, using the +/// operator's existing az login session. Replaces PowerShellConsentRunner — +/// see CHANGELOG for the issue #429 motivation. Connect-MgGraph takes 5–10 seconds +/// to cold-boot the SDK and the MSAL/WAM browser negotiation is unreliable in practice +/// (operators observed 2-minute hangs); az rest is synchronous, fast, and reuses an +/// already-authenticated session. +/// +/// +/// Privilege model: a Global Administrator's az login token implicitly carries every +/// Graph application permission via the GA directory role, including +/// DelegatedPermissionGrant.ReadWrite.All — the scope POST /oauth2PermissionGrants +/// requires. No special consent is needed on the well-known az CLI app for this to work. +/// +/// +internal static partial class AzRestConsentRunner +{ + [GeneratedRegex(@"^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$")] + private static partial Regex GuidPattern(); + + // Delegated scope value names allow alphanumerics, dots, hyphens, underscores. Stricter + // than what Entra technically accepts so we never interpolate untrusted strings into the + // OData $filter parameter without an allowlist pass first. + [GeneratedRegex(@"^[A-Za-z0-9._-]+$")] + private static partial Regex SafeScopePattern(); + + /// + /// Executes the delegated admin-consent grants, one resource at a time, against the + /// operator's az session. Each grant is idempotent: if an + /// AllPrincipals grant already exists between the blueprint and the resource SP, + /// the existing scope set is union-merged with the requested set and PATCHed; otherwise + /// a new grant is POSTed. + /// + /// Command executor used to invoke az rest. + /// + /// Service-principal object id of the blueprint — used as the clientId on every + /// oauth2PermissionGrant row. Phase 1 resolves this and the orchestrator passes it in. + /// + /// + /// Permission specs whose Scopes become the grant scope set per resource. Specs + /// with empty Scopes are skipped. + /// + /// Logger. + /// Cancellation token. + /// + /// (Attempted, Succeeded): + /// - Attempted=false when prerequisites fail (bad GUID, unsafe scope value, no delegated specs). + /// - Succeeded=true only when every per-spec grant completed without error AND the + /// resource SP was resolvable. Partial failures across specs flag overall failure but + /// the loop continues so successful grants are still persisted on the wire. + /// + public static async Task<(bool Attempted, bool Succeeded)> TryRunAsync( + CommandExecutor executor, + string blueprintSpObjectId, + IReadOnlyList specs, + ILogger logger, + CancellationToken ct) + { + if (!GuidPattern().IsMatch(blueprintSpObjectId)) + { + logger.LogWarning("az rest consent runner: invalid blueprint SP id - skipping."); + return (false, false); + } + + var delegatedSpecs = specs + .Where(s => s.Scopes is { Length: > 0 }) + .ToList(); + + if (delegatedSpecs.Count == 0) return (false, false); + + // Validate appId / scope values upfront before any az calls. A bad value here is + // far cheaper to surface as a warning than to surface as a half-completed grant. + foreach (var spec in delegatedSpecs) + { + if (!GuidPattern().IsMatch(spec.ResourceAppId)) + { + logger.LogWarning("az rest consent runner: spec '{ResourceName}' has invalid ResourceAppId - skipping.", spec.ResourceName); + return (false, false); + } + + foreach (var scope in spec.Scopes) + { + if (!SafeScopePattern().IsMatch(scope)) + { + logger.LogWarning("az rest consent runner: spec '{ResourceName}' has unsafe scope value '{Scope}' - skipping.", spec.ResourceName, scope); + return (false, false); + } + } + } + + logger.LogInformation("Granting delegated admin consent..."); + + var allOk = true; + foreach (var spec in delegatedSpecs) + { + ct.ThrowIfCancellationRequested(); + try + { + var ok = await GrantOneAsync(executor, blueprintSpObjectId, spec, logger, ct); + if (!ok) allOk = false; + } + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) + { + logger.LogWarning(" '{Name}': unexpected exception while granting consent - {Message}", spec.ResourceName, ex.Message); + allOk = false; + } + } + + return (true, allOk); + } + + /// + /// Grants 's scopes for the given blueprint SP against a single + /// resource. Lookup, idempotency check, and write are three separate Graph round-trips. + /// + private static async Task GrantOneAsync( + CommandExecutor executor, + string blueprintSpObjectId, + ResourcePermissionSpec spec, + ILogger logger, + CancellationToken ct) + { + // 1. Resolve the resource SP object id. + var resourceSpResult = await executor.ExecuteAsync( + "az", + $"rest --method GET --url \"https://graph.microsoft.com/v1.0/servicePrincipals?$filter=appId eq '{spec.ResourceAppId}'&$select=id\"", + captureOutput: true, + suppressErrorLogging: true, + cancellationToken: ct); + + if (!resourceSpResult.Success) + { + logger.LogWarning( + " '{Name}': failed to look up resource service principal (exit {ExitCode}): {Stderr}", + spec.ResourceName, resourceSpResult.ExitCode, (resourceSpResult.StandardError ?? string.Empty).Trim()); + return false; + } + + var resourceSpId = TryExtractFirstId(resourceSpResult.StandardOutput); + if (string.IsNullOrWhiteSpace(resourceSpId)) + { + logger.LogWarning( + " '{Name}': resource service principal not found in tenant — cannot grant consent.", + spec.ResourceName); + return false; + } + + // 2. Check for an existing AllPrincipals grant for this (client, resource) pair. + // A leftover Principal-scoped grant (e.g. from an earlier --authmode obo run) + // must NOT satisfy this check — that would leave the tenant-wide grant + // un-created. Filter on consentType to be precise. + var grantQueryResult = await executor.ExecuteAsync( + "az", + $"rest --method GET --url \"https://graph.microsoft.com/v1.0/oauth2PermissionGrants?$filter=clientId eq '{blueprintSpObjectId}' and resourceId eq '{resourceSpId}' and consentType eq 'AllPrincipals'\"", + captureOutput: true, + suppressErrorLogging: true, + cancellationToken: ct); + + if (!grantQueryResult.Success) + { + logger.LogWarning( + " '{Name}': failed to query existing oauth2PermissionGrants (exit {ExitCode}): {Stderr}", + spec.ResourceName, grantQueryResult.ExitCode, (grantQueryResult.StandardError ?? string.Empty).Trim()); + return false; + } + + var (existingGrantId, existingScope) = TryExtractFirstGrantIdAndScope(grantQueryResult.StandardOutput); + var existingScopes = string.IsNullOrWhiteSpace(existingScope) + ? new HashSet(StringComparer.OrdinalIgnoreCase) + : existingScope.Split(' ', StringSplitOptions.RemoveEmptyEntries).ToHashSet(StringComparer.OrdinalIgnoreCase); + var mergedScopes = new HashSet(existingScopes, StringComparer.OrdinalIgnoreCase); + foreach (var s in spec.Scopes) mergedScopes.Add(s); + + if (existingGrantId is not null && mergedScopes.Count == existingScopes.Count) + { + logger.LogInformation(" '{Name}': delegated grant already includes the required scopes — no change needed.", spec.ResourceName); + return true; + } + + // 3. PATCH the existing grant (merged scope set) or POST a new one. + var scopeValue = string.Join(' ', mergedScopes.OrderBy(s => s, StringComparer.OrdinalIgnoreCase)); + if (existingGrantId is not null) + { + var patchBody = JsonSerializer.Serialize(new { scope = scopeValue }); + var patched = await ExecuteAzRestWithBodyAsync( + executor, + method: "PATCH", + url: $"https://graph.microsoft.com/v1.0/oauth2PermissionGrants/{existingGrantId}", + bodyJson: patchBody, + logger: logger, + ct: ct); + if (!patched) + { + logger.LogWarning(" '{Name}': PATCH of existing oauth2PermissionGrant failed.", spec.ResourceName); + return false; + } + logger.LogInformation(" '{Name}': delegated grant updated (merged {New} new scope(s) into existing grant).", + spec.ResourceName, mergedScopes.Count - existingScopes.Count); + return true; + } + + var createBody = JsonSerializer.Serialize(new + { + clientId = blueprintSpObjectId, + consentType = "AllPrincipals", + resourceId = resourceSpId, + scope = scopeValue, + }); + var created = await ExecuteAzRestWithBodyAsync( + executor, + method: "POST", + url: "https://graph.microsoft.com/v1.0/oauth2PermissionGrants", + bodyJson: createBody, + logger: logger, + ct: ct); + if (!created) + { + logger.LogWarning(" '{Name}': POST of new oauth2PermissionGrant failed.", spec.ResourceName); + return false; + } + + logger.LogInformation(" '{Name}': delegated grant created ({Count} scope(s)).", spec.ResourceName, mergedScopes.Count); + return true; + } + + /// + /// Writes to a temp file and shells out to + /// az rest --method <method> --url "<url>" --body @<tempfile> --headers Content-Type=application/json. + /// Temp file because passing JSON inline through Windows cmd.exe and az's own + /// argv parser requires double-escaping every internal quote, which is fragile across + /// shells. @<file> is the documented, robust path. + /// + private static async Task ExecuteAzRestWithBodyAsync( + CommandExecutor executor, + string method, + string url, + string bodyJson, + ILogger logger, + CancellationToken ct) + { + var tempFile = Path.Combine(Path.GetTempPath(), $"a365-azrest-{Guid.NewGuid():N}.json"); + try + { + await File.WriteAllTextAsync(tempFile, bodyJson, ct); + + var result = await executor.ExecuteAsync( + "az", + $"rest --method {method} --url \"{url}\" --body @\"{tempFile}\" --headers Content-Type=application/json", + captureOutput: true, + suppressErrorLogging: true, + cancellationToken: ct); + + if (!result.Success) + { + logger.LogWarning( + "az rest {Method} {Url} failed (exit {ExitCode}): {Stderr}", + method, url, result.ExitCode, (result.StandardError ?? string.Empty).Trim()); + return false; + } + return true; + } + finally + { + try { File.Delete(tempFile); } catch { /* best-effort cleanup */ } + } + } + + /// + /// Parses an OData collection response ({"value":[{...},...]}) and returns the + /// id of the first element, or null when the JSON is empty / unparseable / missing + /// the id property. Used to convert a $filter=appId eq '...' lookup into an SP + /// object id without needing System.Text.Json elsewhere. + /// + internal static string? TryExtractFirstId(string? azStandardOutput) + { + if (string.IsNullOrWhiteSpace(azStandardOutput)) return null; + try + { + using var doc = JsonDocument.Parse(azStandardOutput); + if (!doc.RootElement.TryGetProperty("value", out var value)) return null; + if (value.GetArrayLength() == 0) return null; + if (!value[0].TryGetProperty("id", out var id)) return null; + if (id.ValueKind != JsonValueKind.String) return null; + return id.GetString(); + } + catch (JsonException) { return null; } + } + + /// + /// Parses an oauth2PermissionGrants OData collection response and returns the first + /// element's id and scope (space-separated scope set). Both can be null + /// when the collection is empty. + /// + internal static (string? GrantId, string? Scope) TryExtractFirstGrantIdAndScope(string? azStandardOutput) + { + if (string.IsNullOrWhiteSpace(azStandardOutput)) return (null, null); + try + { + using var doc = JsonDocument.Parse(azStandardOutput); + if (!doc.RootElement.TryGetProperty("value", out var value)) return (null, null); + if (value.GetArrayLength() == 0) return (null, null); + var first = value[0]; + string? id = first.TryGetProperty("id", out var idEl) && idEl.ValueKind == JsonValueKind.String + ? idEl.GetString() + : null; + string? scope = first.TryGetProperty("scope", out var scEl) && scEl.ValueKind == JsonValueKind.String + ? scEl.GetString() + : null; + return (id, scope); + } + catch (JsonException) { return (null, null); } + } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestS2SRunner.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestS2SRunner.cs new file mode 100644 index 00000000..fe5134e9 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestS2SRunner.cs @@ -0,0 +1,347 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging; +using System.Text.Json; +using System.Text.RegularExpressions; + +namespace Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; + +/// +/// Assigns S2S app roles on the agent blueprint service principal by shelling out to +/// az rest against the Graph servicePrincipals/{id}/appRoleAssignments +/// endpoint, using the operator's existing az login session. Replaces +/// PowerShellS2SRunner — see CHANGELOG for the issue #429 motivation. Same +/// reasoning as : a GA's az token implicitly carries +/// AppRoleAssignment.ReadWrite.All via the directory role; az rest is +/// synchronous and fast; Connect-MgGraph module-load + MSAL/WAM is unreliable. +/// +internal static partial class AzRestS2SRunner +{ + [GeneratedRegex(@"^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$")] + private static partial Regex GuidPattern(); + + // App role value names follow the same allowlist as delegated scope names — Entra accepts + // a wider set, but we reject anything outside this character class so an attacker-controlled + // value never reaches the OData $filter or the request body without validation. + [GeneratedRegex(@"^[A-Za-z0-9._-]+$")] + private static partial Regex SafeRolePattern(); + + /// + /// Assigns every on every spec to + /// the blueprint SP. Each assignment is idempotent: an existing assignment with the + /// same (principalId, resourceId, appRoleId) tuple is skipped. + /// + /// Command executor used to invoke az rest. + /// + /// Service-principal object id of the blueprint — the assignment is created against + /// this SP and lists it as both principalId (the assignee) and the path target. + /// + /// Permission specs whose AppRoleScopes are the role values to assign. + /// Logger. + /// Cancellation token. + /// + /// (Attempted, Succeeded): + /// - Attempted=false when prerequisites fail (bad GUID, unsafe role value, no S2S specs). + /// - Succeeded=true only when every role on every spec was either already assigned or + /// newly POSTed without error. + /// + public static async Task<(bool Attempted, bool Succeeded)> TryRunAsync( + CommandExecutor executor, + string blueprintSpObjectId, + IReadOnlyList specs, + ILogger logger, + CancellationToken ct) + { + if (!GuidPattern().IsMatch(blueprintSpObjectId)) + { + logger.LogWarning("az rest S2S runner: invalid blueprint SP id - skipping."); + return (false, false); + } + + var s2sSpecs = specs + .Where(s => s.AppRoleScopes is { Length: > 0 }) + .ToList(); + + if (s2sSpecs.Count == 0) return (false, false); + + // Allowlist validation up front so we never interpolate untrusted values into the + // OData filter (resource SP lookup) or the request body (role id lookup). + foreach (var spec in s2sSpecs) + { + if (!GuidPattern().IsMatch(spec.ResourceAppId)) + { + logger.LogWarning("az rest S2S runner: spec '{ResourceName}' has invalid ResourceAppId - skipping.", spec.ResourceName); + return (false, false); + } + foreach (var role in spec.AppRoleScopes!) + { + if (!SafeRolePattern().IsMatch(role)) + { + logger.LogWarning("az rest S2S runner: spec '{ResourceName}' has unsafe role value '{Role}' - skipping.", spec.ResourceName, role); + return (false, false); + } + } + } + + logger.LogInformation("Assigning S2S app roles..."); + + var allOk = true; + + // Fetch the existing assignment list once at the top — every per-role idempotency + // check then compares against this in-memory set, avoiding N+1 Graph round-trips. + var existingAssignments = await GetExistingAssignmentsAsync(executor, blueprintSpObjectId, logger, ct); + if (existingAssignments is null) + { + // The GET itself failed; that's a hard stop because we can't reason about + // idempotency without it. + return (true, false); + } + + foreach (var spec in s2sSpecs) + { + ct.ThrowIfCancellationRequested(); + try + { + var ok = await AssignOneAsync(executor, blueprintSpObjectId, spec, existingAssignments, logger, ct); + if (!ok) allOk = false; + } + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) + { + logger.LogWarning(" '{Name}': unexpected exception while assigning app role - {Message}", spec.ResourceName, ex.Message); + allOk = false; + } + } + + return (true, allOk); + } + + /// + /// Looks up every role on and POSTs a new assignment per role + /// that isn't already present in . The resource + /// SP lookup is one Graph call (with $select=id,appRoles); the role id is + /// derived from the embedded appRoles array — no second GET needed. + /// + private static async Task AssignOneAsync( + CommandExecutor executor, + string blueprintSpObjectId, + ResourcePermissionSpec spec, + HashSet<(string ResourceId, string AppRoleId)> existingAssignments, + ILogger logger, + CancellationToken ct) + { + var spResult = await executor.ExecuteAsync( + "az", + $"rest --method GET --url \"https://graph.microsoft.com/v1.0/servicePrincipals?$filter=appId eq '{spec.ResourceAppId}'&$select=id,appRoles\"", + captureOutput: true, + suppressErrorLogging: true, + cancellationToken: ct); + + if (!spResult.Success) + { + logger.LogWarning( + " '{Name}': failed to look up resource service principal (exit {ExitCode}): {Stderr}", + spec.ResourceName, spResult.ExitCode, (spResult.StandardError ?? string.Empty).Trim()); + return false; + } + + var (resourceSpId, appRolesByValue) = TryExtractFirstSpIdAndAppRoles(spResult.StandardOutput); + if (string.IsNullOrWhiteSpace(resourceSpId)) + { + logger.LogWarning(" '{Name}': resource service principal not found in tenant — cannot assign app role.", spec.ResourceName); + return false; + } + + var allOk = true; + foreach (var role in spec.AppRoleScopes!) + { + if (!appRolesByValue.TryGetValue(role, out var appRoleId)) + { + logger.LogWarning(" '{Name}': app role '{Role}' is not published on the resource — skipping.", spec.ResourceName, role); + allOk = false; + continue; + } + + if (existingAssignments.Contains((resourceSpId, appRoleId))) + { + logger.LogInformation(" '{Name}': app role '{Role}' already assigned — no change needed.", spec.ResourceName, role); + continue; + } + + var createBody = JsonSerializer.Serialize(new + { + principalId = blueprintSpObjectId, + resourceId = resourceSpId, + appRoleId = appRoleId, + }); + var created = await ExecuteAzRestWithBodyAsync( + executor, + method: "POST", + url: $"https://graph.microsoft.com/v1.0/servicePrincipals/{blueprintSpObjectId}/appRoleAssignments", + bodyJson: createBody, + logger: logger, + ct: ct); + + if (!created) + { + logger.LogWarning(" '{Name}': POST of new appRoleAssignment for role '{Role}' failed.", spec.ResourceName, role); + allOk = false; + continue; + } + + // Update our in-memory cache so a later spec in the same run doesn't double-POST + // the same (resource, role) pair if it ever shows up twice in the spec list. + existingAssignments.Add((resourceSpId, appRoleId)); + logger.LogInformation(" '{Name}': app role '{Role}' assigned.", spec.ResourceName, role); + } + + return allOk; + } + + /// + /// Pulls the full appRoleAssignments collection for the blueprint SP. Returns a + /// HashSet<(resourceId, appRoleId)> for O(1) per-role idempotency checks + /// during the assignment loop. Returns null when the GET fails so the caller can + /// short-circuit. + /// + private static async Task?> GetExistingAssignmentsAsync( + CommandExecutor executor, + string blueprintSpObjectId, + ILogger logger, + CancellationToken ct) + { + var result = await executor.ExecuteAsync( + "az", + $"rest --method GET --url \"https://graph.microsoft.com/v1.0/servicePrincipals/{blueprintSpObjectId}/appRoleAssignments\"", + captureOutput: true, + suppressErrorLogging: true, + cancellationToken: ct); + + if (!result.Success) + { + logger.LogWarning( + "az rest GET appRoleAssignments failed (exit {ExitCode}): {Stderr}", + result.ExitCode, (result.StandardError ?? string.Empty).Trim()); + return null; + } + + var assignments = new HashSet<(string, string)>(); + if (string.IsNullOrWhiteSpace(result.StandardOutput)) return assignments; + try + { + using var doc = JsonDocument.Parse(result.StandardOutput); + if (doc.RootElement.TryGetProperty("value", out var arr)) + { + foreach (var item in arr.EnumerateArray()) + { + if (item.TryGetProperty("resourceId", out var resourceEl) && + item.TryGetProperty("appRoleId", out var roleEl) && + resourceEl.ValueKind == JsonValueKind.String && + roleEl.ValueKind == JsonValueKind.String) + { + var resourceId = resourceEl.GetString(); + var appRoleId = roleEl.GetString(); + if (!string.IsNullOrEmpty(resourceId) && !string.IsNullOrEmpty(appRoleId)) + assignments.Add((resourceId, appRoleId)); + } + } + } + } + catch (JsonException ex) + { + logger.LogWarning("Failed to parse appRoleAssignments response - assuming empty set: {Message}", ex.Message); + return assignments; + } + return assignments; + } + + /// + /// Same temp-file approach as — passes the JSON body + /// via --body @<tempfile> to avoid inline quote-escaping through cmd.exe. + /// + private static async Task ExecuteAzRestWithBodyAsync( + CommandExecutor executor, + string method, + string url, + string bodyJson, + ILogger logger, + CancellationToken ct) + { + var tempFile = Path.Combine(Path.GetTempPath(), $"a365-azrest-s2s-{Guid.NewGuid():N}.json"); + try + { + await File.WriteAllTextAsync(tempFile, bodyJson, ct); + + var result = await executor.ExecuteAsync( + "az", + $"rest --method {method} --url \"{url}\" --body @\"{tempFile}\" --headers Content-Type=application/json", + captureOutput: true, + suppressErrorLogging: true, + cancellationToken: ct); + + if (!result.Success) + { + logger.LogWarning( + "az rest {Method} {Url} failed (exit {ExitCode}): {Stderr}", + method, url, result.ExitCode, (result.StandardError ?? string.Empty).Trim()); + return false; + } + return true; + } + finally + { + try { File.Delete(tempFile); } catch { /* best-effort cleanup */ } + } + } + + /// + /// Parses the resource-SP-with-appRoles response and returns the SP id plus a + /// case-insensitive map from app role value (the user-facing name like + /// "Agent365.Observability.OtelWrite") to id (the GUID required by the + /// assignment body). Map is empty when the SP exposes no app roles. + /// + internal static (string? SpId, Dictionary AppRolesByValue) TryExtractFirstSpIdAndAppRoles(string? azStandardOutput) + { + var rolesByValue = new Dictionary(StringComparer.OrdinalIgnoreCase); + if (string.IsNullOrWhiteSpace(azStandardOutput)) return (null, rolesByValue); + try + { + using var doc = JsonDocument.Parse(azStandardOutput); + if (!doc.RootElement.TryGetProperty("value", out var value)) return (null, rolesByValue); + if (value.GetArrayLength() == 0) return (null, rolesByValue); + + var first = value[0]; + string? spId = first.TryGetProperty("id", out var idEl) && idEl.ValueKind == JsonValueKind.String + ? idEl.GetString() + : null; + + if (first.TryGetProperty("appRoles", out var rolesEl) && rolesEl.ValueKind == JsonValueKind.Array) + { + foreach (var role in rolesEl.EnumerateArray()) + { + if (role.TryGetProperty("value", out var roleValEl) && + role.TryGetProperty("id", out var roleIdEl) && + roleValEl.ValueKind == JsonValueKind.String && + roleIdEl.ValueKind == JsonValueKind.String) + { + var v = roleValEl.GetString(); + var i = roleIdEl.GetString(); + if (!string.IsNullOrEmpty(v) && !string.IsNullOrEmpty(i)) + rolesByValue[v] = i; + } + } + } + + return (spId, rolesByValue); + } + catch (JsonException) + { + return (null, rolesByValue); + } + } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs index b13ea4c7..ed0cbafc 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs @@ -74,7 +74,9 @@ internal static class BatchPermissionsOrchestrator CancellationToken ct, string? knownBlueprintSpObjectId = null, IConfirmationProvider? confirmationProvider = null, - CommandExecutor? commandExecutor = null) + CommandExecutor? commandExecutor = null, + bool skipSpProvisioning = false, + IReadOnlyCollection? knownMcpAudienceAppIds = null) { if (specs.Count == 0) { @@ -193,33 +195,58 @@ internal static class BatchPermissionsOrchestrator // in the token's scp claim — a privilege A365 tokens never carry — so the previous // programmatic path always failed in fresh tenants. See CHANGELOG for details. // - // confirmationProvider is intentionally unused: the /v2.0/adminconsent browser flow - // surfaces its own consent screen, which serves as the user-facing confirmation. - _ = confirmationProvider; if (isGlobalAdmin) { var s2sScopes = permScopes.Concat(AuthenticationConstants.RequiredS2SGrantScopes).ToArray(); - if (!string.IsNullOrWhiteSpace(phase1Result?.BlueprintSpObjectId)) - await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result.BlueprintSpObjectId, specs, s2sScopes, logger, setupResults, ct); - // else: blueprint SP was not resolved — leave BlueprintS2SOutcome = NotApplicable (not attempted) + var hasS2SWork = !string.IsNullOrWhiteSpace(phase1Result?.BlueprintSpObjectId) + && specs.Any(s => s.AppRoleScopes is { Length: > 0 }); + + // Single up-front prompt covering BOTH the primary Graph API path and the az + // rest fallback. Previously the prompt only existed in the fallback branch, so + // operators on tenants where the primary path succeeded never saw a confirmation + // for an admin-level write. The fallback path now reuses the same operator + // decision rather than re-asking. + var operatorConfirmedS2S = false; + if (hasS2SWork) + { + operatorConfirmedS2S = await PromptForBlueprintPermissionGrantAsync( + BlueprintPermissionKind.Application, specs, confirmationProvider, logger); + if (!operatorConfirmedS2S) + { + logger.LogInformation("Skipping S2S app role assignment per operator response. The setup summary lists the manual steps."); + if (setupResults is not null) + setupResults.BlueprintS2SOutcome = Models.GrantOutcome.Failed; + } + } - // When the programmatic Graph API path fails (e.g. token lacks AppRoleAssignment.ReadWrite.All - // even for GA), fall back to executing the same PowerShell script automatically. - if (setupResults?.BlueprintS2SOutcome == Models.GrantOutcome.Failed && commandExecutor != null) + if (operatorConfirmedS2S) { - logger.LogDebug("S2S app role assignments could not be completed via the Graph API."); - logger.LogDebug("Attempting via PowerShell (pwsh)..."); - var (attempted, succeeded) = await PowerShellS2SRunner.TryRunAsync( - commandExecutor, tenantId, blueprintAppId, specs, logger, ct); - if (attempted && succeeded) + await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result!.BlueprintSpObjectId, specs, s2sScopes, logger, setupResults, ct); + + // When the programmatic Graph API path fails (e.g. CLI token lacks + // AppRoleAssignment.ReadWrite.All even for a GA), fall back to issuing the + // same writes via `az rest` against the operator's existing az session. A + // GA's az token implicitly carries every Graph application permission via + // the directory role — including AppRoleAssignment.ReadWrite.All — so + // POST /appRoleAssignments succeeds without any additional consent. The + // operator already authorized the action at the single prompt above; no + // second prompt is required here. + if (setupResults?.BlueprintS2SOutcome == Models.GrantOutcome.Failed + && commandExecutor != null) { - logger.LogInformation("S2S app role assignments completed via PowerShell."); - setupResults.BlueprintS2SOutcome = Models.GrantOutcome.Granted; + logger.LogDebug("S2S app role assignments could not be completed via the Graph API; falling back to az rest."); + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + commandExecutor, phase1Result.BlueprintSpObjectId, specs, logger, ct); + if (attempted && succeeded) + { + logger.LogInformation("Application permissions granted."); + setupResults.BlueprintS2SOutcome = Models.GrantOutcome.Granted; + } + else if (attempted) + logger.LogWarning("Some app role assignments did not complete - see output above. Manual steps in summary."); + // else: validation rejected the input or no S2S specs were present. + // AzRestS2SRunner already logged an actionable warning; Action Required surfaces the rest. } - else if (attempted) - logger.LogWarning("PowerShell execution did not complete — see output above. Manual steps in summary."); - // else: pwsh missing / timeout / inputs invalid — PowerShellS2SRunner already - // logged an actionable warning. Manual steps appear in the setup summary. } } } @@ -235,7 +262,7 @@ internal static class BatchPermissionsOrchestrator // --- Admin consent --- var (consentGranted, consentUrl) = await GrantAdminConsentAsync( - graph, config, blueprintAppId, tenantId, specs, phase1Result, permScopes, logger, setupResults, ct, commandExecutor, adminCheck); + graph, config, blueprintAppId, tenantId, specs, phase1Result, permScopes, logger, setupResults, ct, commandExecutor, adminCheck, confirmationProvider, skipSpProvisioning, knownMcpAudienceAppIds); // Update in-memory ResourceConsents only when consent was directly verified (consentUrl == null). // AssumedComplete returns a non-null consentUrl — do not persist in that case since the grant @@ -441,8 +468,8 @@ private static async Task UpdateBlueprintPermissions /// emit identical scope identifiers (e.g. https://agent365.svc.cloud.microsoft/Tools.Execute, /// not api://{appId}/Tools.Execute). /// - private static string BuildFullyQualifiedScope(string resourceAppId, string scope, string? resourceName = null) - => SetupHelpers.BuildFullyQualifiedScope(resourceAppId, scope, resourceName); + private static string BuildFullyQualifiedScope(string resourceAppId, string scope, bool isMcpAudience = false) + => SetupHelpers.BuildFullyQualifiedScope(resourceAppId, scope, isMcpAudience); /// /// Grants S2S app role assignments for all specs that carry . @@ -540,21 +567,98 @@ private static async Task PerformS2SGrantsAsync( SetupResults? setupResults, CancellationToken ct, CommandExecutor? commandExecutor = null, - Models.RoleCheckResult adminCheck = Models.RoleCheckResult.Unknown) + Models.RoleCheckResult adminCheck = Models.RoleCheckResult.Unknown, + IConfirmationProvider? confirmationProvider = null, + bool skipSpProvisioning = false, + IReadOnlyCollection? knownMcpAudienceAppIds = null) { + // Hold onto the unfiltered spec list so the PowerShell consent fallback can attempt + // dropped scopes too — the programmatic oauth2PermissionGrants POST is lenient about + // scope existence and stamping intent is sometimes what the operator actually wants. + var originalSpecs = specs; + + // Issue #429: filter each spec's Scopes against what the resource SP actually exposes + // in this tenant. The /v2.0/adminconsent endpoint strictly validates every requested + // scope and rejects the entire URL with AADSTS650053 on the first unknown scope — + // a single drift between our spec list and the live resource SP (e.g. Bot Framework + // dropping "Authorization.ReadWrite" in favor of "AgentData.ReadWrite") blocks every + // other resource. Dropping unknown scopes here keeps the URL valid; the warnings tell + // the operator what we filtered out, and the PowerShell fallback (offered after the + // browser flow fails) can stamp them via the lenient programmatic path if needed. + IReadOnlyList droppedScopes = + Array.Empty(); + if (phase1Result is { ResourceSpObjectIds.Count: > 0 }) + { + var validation = await ScopeAvailabilityValidator.ValidateAsync( + graph, tenantId, specs, phase1Result.ResourceSpObjectIds, logger, ct); + specs = validation.EffectiveSpecs; + droppedScopes = validation.DroppedScopes; + + foreach (var d in droppedScopes) + { + logger.LogWarning( + "Resource '{ResourceName}' ({ResourceAppId}) does not publish delegated scope '{Scope}' — dropping from the unified admin-consent URL to avoid AADSTS650053. " + + "If you require this grant, opt into the az rest fallback when prompted; it uses the programmatic oauth2PermissionGrants POST which is lenient about scope existence.", + d.ResourceName, d.ResourceAppId, d.Scope); + setupResults?.Warnings.Add( + $"Dropped scope '{d.Scope}' from consent URL — not published on '{d.ResourceName}' ({d.ResourceAppId}). Use the az rest fallback to attempt it."); + } + } + // Build a single combined consent URL covering ALL delegated scopes across every // resource stamped on the blueprint (Graph, Agent 365 Tools, Messaging Bot, // Observability, Power Platform, ...). The /v2.0/adminconsent endpoint accepts - // fully-qualified scope URIs for any resource — Graph uses https://graph.microsoft.com/... - // and other resources use api://{appId}/... — so one URL grants everything at once. + // either a fully-qualified Application ID URI (e.g. https://graph.microsoft.com/...) + // or a bare Application ID GUID (for SPs without a published URI) — both flavors + // are produced by GetResourceIdentifierUri. // - // This replaces the previous "Graph-only URL + programmatic POST /oauth2PermissionGrants - // for everything else" model, which failed in fresh tenants because the Graph POST - // requires DelegatedPermissionGrant.ReadWrite.All in the token's scp claim (a privilege - // A365 tokens never carry). See CHANGELOG for details. - var allScopes = specs + // Issue #429: an unresolvable resource SP poisons the entire URL. AADSTS650052 is + // returned for the FIRST scope whose resource has no SP in the tenant + // ("organization lacks a service principal for ..."). Even when Phase 1 silently + // failed to create the SP (logWarningOnCreateFailure: false), the spec's scope + // still landed in the URL pre-fix. Filter to specs whose SP was actually resolved + // in Phase 1 before building the URL, and surface a warning for each excluded + // resource so the operator knows which scopes weren't consented. + var resolvedSpAppIds = phase1Result?.ResourceSpObjectIds is { } map + ? map.Keys.ToHashSet(StringComparer.OrdinalIgnoreCase) + : new HashSet(StringComparer.OrdinalIgnoreCase); + + // Find specs whose SP couldn't be resolved in Phase 1 and try to provision them in + // place by shelling out to 'az ad sp create --id {appId}' against the operator's + // existing az login (the per-app admin-consent URL pattern was removed because + // first-party MCP audiences fail it with AADSTS65003 — token-to-self consent). + // EnsureMissingResourceSpsAsync mutates the resolvedSpAppIds set on success and + // records MissingSpActions for the rest so the Action Required block renders the + // recovery steps (the az command + a per-SP /v2.0/adminconsent URL keyed to the + // blueprint as client). Skips entirely when skipSpProvisioning is true (flag or + // auto-detected from stdin) or when there is nothing missing. See helper for the + // full state machine. + if (resolvedSpAppIds.Count > 0) + { + var missingSpecs = specs + .Where(s => s.Scopes is { Length: > 0 } && !resolvedSpAppIds.Contains(s.ResourceAppId)) + .ToList(); + await EnsureMissingResourceSpsAsync( + graph, tenantId, blueprintAppId, missingSpecs, resolvedSpAppIds, permScopes, + skipSpProvisioning, logger, setupResults, ct, + commandExecutor: commandExecutor, + confirmationProvider: confirmationProvider, + knownMcpAudienceAppIds: knownMcpAudienceAppIds); + } + + // Apply the SP-resolution filter only when Phase 1 produced any results. When + // Phase 1 returned no resolved SPs at all (auth failure earlier), keep the legacy + // behavior of including every spec — that surfaces the auth failure path rather + // than silently dropping every scope here. + var specsForUrl = resolvedSpAppIds.Count > 0 + ? specs.Where(s => resolvedSpAppIds.Contains(s.ResourceAppId)).ToList() + : specs.ToList(); + + var allScopes = specsForUrl .Where(s => s.Scopes is { Length: > 0 }) - .SelectMany(s => s.Scopes.Select(scope => BuildFullyQualifiedScope(s.ResourceAppId, scope, s.ResourceName))) + .SelectMany(s => s.Scopes.Select(scope => BuildFullyQualifiedScope( + s.ResourceAppId, scope, + isMcpAudience: knownMcpAudienceAppIds?.Contains(s.ResourceAppId) ?? false))) .Distinct(StringComparer.OrdinalIgnoreCase) .ToList(); @@ -680,7 +784,7 @@ private static async Task PerformS2SGrantsAsync( // the freshly opened browser tab is the user-visible confirmation. If the browser // fails to launch, BrowserHelper.TryOpenUrl logs the URL itself, and if consent is // not detected within the timeout the Action Required block surfaces the URL again. - logger.LogInformation(" - Opening browser for admin consent (covers all required delegated permissions)..."); + logger.LogInformation(" - Opening browser for admin consent..."); BrowserHelper.TryOpenUrl(consentUrl!, logger); bool consentGranted; @@ -736,6 +840,58 @@ private static async Task PerformS2SGrantsAsync( setupResults?.Warnings.Add($"Admin consent not detected within timeout. Grant at: {consentUrl}"); } + // Issue #429: when the browser polling did not observe a verified grant — either the + // browser failed to open, the user closed the consent screen without granting, or Entra + // rejected the URL with an OAuth error (e.g. AADSTS650053) — offer to issue the + // oauth2PermissionGrants writes via `az rest` against the operator's existing az + // session. A GA's az token implicitly carries DelegatedPermissionGrant.ReadWrite.All + // via the directory role, which is what the writes need. Replaces the previous + // Connect-MgGraph PowerShell fallback (issue #429): pwsh module load + MSAL/WAM + // browser dance was 5s–2min and unreliable; az rest is synchronous and fast. + // + // Pass the *original* (unfiltered) specs to the runner: the programmatic + // oauth2PermissionGrants POST is lenient about scope existence, so the operator can + // record intent for scopes the resource SP does not currently publish. This is what + // the dropped-scope warnings above point them toward. + if (!consentVerified + && commandExecutor is not null + && phase1Result is { } p + && !string.IsNullOrWhiteSpace(p.BlueprintSpObjectId)) + { + var shouldRunConsent = await PromptForBlueprintPermissionGrantAsync( + BlueprintPermissionKind.Delegated, originalSpecs, confirmationProvider, logger); + if (!shouldRunConsent) + { + logger.LogInformation("Admin consent not granted. Re-run setup or grant via the URL above when ready."); + } + else + { + var (attempted, succeeded) = await AzRestConsentRunner.TryRunAsync( + commandExecutor, p.BlueprintSpObjectId, originalSpecs, logger, ct); + if (attempted && succeeded) + { + logger.LogInformation("Delegated admin consent granted."); + if (setupResults is not null) + { + // Mirror the post-browser-success bookkeeping: a successful run + // is just as good as a Verified browser poll for the purpose of "did we + // record consent." The caller's persistence gate is (granted && url==null), + // and returning verified=true below produces exactly that. + setupResults.TenantWideConsentAlreadyExisted = false; + } + consentGranted = true; + consentVerified = true; + } + else if (attempted) + { + logger.LogWarning("Admin consent did not complete - see output above. The consent URL remains in the setup summary for manual completion."); + } + // else: validation rejected the input or no delegated specs were present. + // AzRestConsentRunner already logged an actionable warning. The Action Required + // block surfaces the URL. + } + } + // Return URL when either polling failed outright OR consent was assumed-complete but not // verified. Caller uses (consentGranted && consentUrl == null) as the 'safe to persist' gate. return (consentGranted, consentVerified ? null : consentUrl); @@ -795,6 +951,397 @@ private static bool IsInsufficientPrivilegesError(string? err) || err.Contains("Authorization_RequestDenied", StringComparison.OrdinalIgnoreCase); } + /// + /// Issue #429: in-line provisioning of missing resource service principals before the + /// unified admin-consent URL is built. AADSTS650052 is returned when even one requested + /// resource lacks an SP in the tenant ("organization lacks a service principal for ..."); + /// the whole URL fails atomically. Phase 1's EnsureServicePrincipalForAppIdAsync + /// uses POST /servicePrincipals which requires Application.ReadWrite.All on + /// the CLI token (it does not carry it), so for some first-party multi-tenant apps the + /// silent SP-creation path fails. + /// + /// + /// The original approach used a per-app /v2.0/adminconsent browser URL with + /// {appId}/.default scope. That fails with AADSTS65003 for first-party MCP audiences + /// because (a) it is a "token-to-self" pattern (the app would consent to itself) which + /// requires preauthorization, and (b) the suggested workaround of using a URI identifier + /// is not available — the per-server SPs have identifierUris = null. So we shell + /// out to az ad sp create --id {appId} instead, which uses the operator's existing + /// az login token. A Global Administrator's az token carries + /// Application.ReadWrite.All implicitly via the GA directory role, which is exactly + /// the permission POST /servicePrincipals needs. + /// + /// + /// + /// For each spec whose ResourceAppId is missing from : + /// + /// + /// Re-queries Graph in case the SP appeared between Phase 1 and now + /// (operator consented in another window, slow replica caught up). Adds the appId to the + /// resolved set and skips ahead if found. + /// Honors : emits warnings with the + /// az ad sp create command for manual provisioning and returns. Set via the + /// --skip-sp-provisioning flag or implicitly when stdin is redirected + /// (CI / pipe). + /// Otherwise, serial loop: per-SP [y/N] confirmation, then + /// shells out to az ad sp create --id {appId} via . + /// On exit 0 plus Graph verification, adds to the resolved set; on failure, emits the + /// warning + manual command as next steps and continues with remaining specs. + /// + /// + internal static async Task EnsureMissingResourceSpsAsync( + GraphApiService graph, + string tenantId, + string blueprintAppId, + IReadOnlyList missingSpecs, + HashSet resolvedSpAppIds, + string[] permScopes, + bool skipSpProvisioning, + ILogger logger, + SetupResults? setupResults, + CancellationToken ct, + CommandExecutor? commandExecutor = null, + IConfirmationProvider? confirmationProvider = null, + IReadOnlyCollection? knownMcpAudienceAppIds = null) + { + if (missingSpecs.Count == 0) return; + + // Test bypass: short-circuits the entire helper so unit tests for the broader + // GrantAdminConsentAsync flow do not need to mock az / Graph. Tests that exercise + // this helper directly set this to false explicitly. + if (BypassSpProvisioningForTests) return; + + // Pre-flight: re-query each missing SP once. Cheap; eliminates the race where the + // operator already consented out-of-band between Phase 1 and now, or where a slow + // Graph replica needed one more probe to catch up. + var stillMissing = new List(); + foreach (var spec in missingSpecs) + { + var spId = await graph.LookupServicePrincipalByAppIdAsync(tenantId, spec.ResourceAppId, ct, permScopes); + if (!string.IsNullOrWhiteSpace(spId)) + { + logger.LogInformation( + "Resource '{Name}' ({AppId}): service principal found in tenant — no provisioning needed.", + spec.ResourceName, spec.ResourceAppId); + resolvedSpAppIds.Add(spec.ResourceAppId); + } + else + { + stillMissing.Add(spec); + } + } + + if (stillMissing.Count == 0) return; + + // Non-interactive path (--skip-sp-provisioning set, or stdin redirected, or CI/agent + // scenario, or no executor passed): emit per-resource warnings with the manual + // az command and return. The caller's existing exclusion + warning block handles + // the unified URL build with what's resolvable. + if (skipSpProvisioning || commandExecutor is null) + { + logger.LogInformation(""); + logger.LogInformation( + "{Count} resource(s) require service principal provisioning. Auto-provisioning is disabled; steps will be listed in the setup summary.", + stillMissing.Count); + foreach (var spec in stillMissing) + RecordMissingSpAction(spec, tenantId, blueprintAppId, logger, setupResults, knownMcpAudienceAppIds); + return; + } + + // Interactive path. Each iteration asks the operator, then shells out to + // 'az ad sp create --id {appId}'. The operator's az login token carries + // Application.ReadWrite.All implicitly via the Global Administrator directory role, + // which is what POST /servicePrincipals requires. + var pluralVerb = stillMissing.Count == 1 ? "is" : "are"; + var pluralNoun = stillMissing.Count == 1 ? "resource service principal" : "resource service principals"; + var maxNameWidth = stillMissing.Max(s => s.ResourceName.Length); + + logger.LogInformation(""); + logger.LogInformation("{Count} {Noun} {Verb} missing in your tenant.", stillMissing.Count, pluralNoun, pluralVerb); + logger.LogInformation("Provisioning will run 'az ad sp create' using your current az login."); + logger.LogInformation("You will be prompted before each is provisioned."); + logger.LogInformation(""); + + // Upfront list — name padded so the appId column lines up. Numbering uses "{i}." + // to match the per-prompt prefix below for visual correspondence. + for (int i = 0; i < stillMissing.Count; i++) + { + var spec = stillMissing[i]; + logger.LogInformation(" {Idx}. {Name} {AppId}", + i + 1, spec.ResourceName.PadRight(maxNameWidth), spec.ResourceAppId); + } + + for (int i = 0; i < stillMissing.Count; i++) + { + ct.ThrowIfCancellationRequested(); + var spec = stillMissing[i]; + + logger.LogInformation(""); + + // GUID guard: appId originates from manifest / typed config, but custom + // permissions are user-supplied and reach this loop too. Validate before + // interpolating into the shell command — defense in depth against injection. + if (!Guid.TryParse(spec.ResourceAppId, out _)) + { + logger.LogWarning( + " {Idx}. {Name} ({AppId}): skipping — resource app id is not a valid GUID.", + i + 1, spec.ResourceName, spec.ResourceAppId); + RecordMissingSpAction(spec, tenantId, blueprintAppId, logger, setupResults, knownMcpAudienceAppIds); + continue; + } + + // Per-SP confirmation. Default No (must type y). Null confirmationProvider + // preserves the legacy "auto-yes" behavior under test, mirroring the other + // PromptForBlueprintPermissionGrantAsync call sites. + var prompt = $" {i + 1}. {spec.ResourceName} - Provision via 'az ad sp create'? [y/N]: "; + var shouldProvision = confirmationProvider is null + || await confirmationProvider.ConfirmAsync(prompt); + if (!shouldProvision) + { + logger.LogInformation(" Skipped."); + RecordMissingSpAction(spec, tenantId, blueprintAppId, logger, setupResults, knownMcpAudienceAppIds); + continue; + } + + var azArgs = $"ad sp create --id {spec.ResourceAppId}"; + logger.LogInformation(" Running: az {AzArgs}", azArgs); + var azResult = await commandExecutor.ExecuteAsync( + "az", azArgs, + captureOutput: true, + suppressErrorLogging: true, + cancellationToken: ct); + + if (!azResult.Success) + { + var stderr = string.IsNullOrWhiteSpace(azResult.StandardError) ? azResult.StandardOutput : azResult.StandardError; + logger.LogWarning(" Failed: {Error}", (stderr ?? string.Empty).Trim()); + RecordMissingSpAction(spec, tenantId, blueprintAppId, logger, setupResults, knownMcpAudienceAppIds); + continue; + } + + // az exit 0 plus a parseable SP id in its JSON output is authoritative — the + // shell-out and the Graph backend are the same Entra tenant, so an SP id in the + // command output means the SP exists. The previous post-create Graph re-poll + // produced false "Graph still does not see the SP" warnings on slow replicas + // even when az clearly succeeded; trusting az output eliminates that. + string? newSpId = TryExtractSpIdFromAzOutput(azResult.StandardOutput); + if (!string.IsNullOrWhiteSpace(newSpId)) + { + logger.LogInformation(" Done. Service principal created for '{Name}' (id: {SpId}).", spec.ResourceName, newSpId); + resolvedSpAppIds.Add(spec.ResourceAppId); + } + else + { + // az exited 0 but its stdout did not parse — extremely unusual. Surface the + // raw output so the operator can diagnose, and record the action so the + // setup summary surfaces the recovery steps. + logger.LogWarning( + " az exited 0 but the output did not contain a service principal id. Output: {Output}", + (azResult.StandardOutput ?? string.Empty).Trim()); + RecordMissingSpAction(spec, tenantId, blueprintAppId, logger, setupResults, knownMcpAudienceAppIds); + } + } + + logger.LogInformation(""); + logger.LogInformation("Continuing with admin consent..."); + } + + /// + /// Parses the JSON returned by az ad sp create --id {appId} and extracts the SP + /// object id from the id property. Returns null when the input is null, empty, + /// not JSON, or missing the property. The presence of an id is sufficient evidence that + /// the SP was created — az returns the same JSON the Graph POST returned, in real time, + /// against the same backend. + /// + internal static string? TryExtractSpIdFromAzOutput(string? azStandardOutput) + { + if (string.IsNullOrWhiteSpace(azStandardOutput)) return null; + try + { + using var doc = System.Text.Json.JsonDocument.Parse(azStandardOutput); + if (doc.RootElement.TryGetProperty("id", out var idEl) && idEl.ValueKind == System.Text.Json.JsonValueKind.String) + return idEl.GetString(); + } + catch (System.Text.Json.JsonException) { /* not JSON; return null */ } + return null; + } + + /// + /// Test-only escape hatch — when true, + /// returns immediately without opening browsers, polling Graph, or consuming stdin. + /// Default false so the helper actually runs in production. Tests + /// for that do not want the helper firing + /// must set this to true in their setup. Tests that exercise the helper directly + /// leave it false. Pattern mirrors . + /// + internal static bool BypassSpProvisioningForTests { get; set; } = false; + + /// + /// Builds the az ad sp create command that provisions a missing resource SP in + /// the operator's tenant. The operator's az login (running as Global Administrator) + /// carries Application.ReadWrite.All implicitly via the GA directory role, which + /// is exactly the permission POST /servicePrincipals needs. Returned as a + /// ready-to-copy command string so the same form appears in the live "Running: ..." log + /// line and in the warning next-steps block. + /// + internal static string BuildAzAdSpCreateCommand(string resourceAppId) => + $"az ad sp create --id {resourceAppId}"; + + /// + /// Records a missing-SP action on so the + /// setup summary's "Action Required" block renders it as a numbered item. Each entry + /// carries the two concrete artifacts the operator needs to complete provisioning + /// without re-running setup: + /// + /// az ad sp create --id {appId} — provisions the SP in the tenant. + /// Per-SP /v2.0/adminconsent URL keyed to the blueprint as + /// client and this resource's scopes as the request. After step 1 succeeds, clicking + /// this URL grants the blueprint consent for this one resource additively (does not + /// wipe other resources' grants), avoiding any need to re-run a365 setup all. + /// + /// Used by every path that leaves a resource un-provisioned: declined per-SP prompt, + /// GUID guard rejection, az exiting non-zero, or --skip-sp-provisioning. + /// + private static void RecordMissingSpAction( + ResourcePermissionSpec spec, + string tenantId, + string blueprintAppId, + ILogger logger, + SetupResults? setupResults, + IReadOnlyCollection? knownMcpAudienceAppIds = null) + { + _ = logger; // intentionally unused — caller already emits a one-line inline marker + // ("Skipped." / "Failed: " / "...invalid GUID...") immediately + // before invoking this. The full recovery block (az command + per-SP + // consent URL) renders only in the Action Required section so the main + // output stays clean. See DisplaySetupSummary's MissingSpActions branch. + + var azCommand = BuildAzAdSpCreateCommand(spec.ResourceAppId); + var isMcpAudience = knownMcpAudienceAppIds?.Contains(spec.ResourceAppId) ?? false; + var perSpConsentUrl = BuildPerSpBlueprintConsentUrl(tenantId, blueprintAppId, spec, isMcpAudience); + + setupResults?.MissingSpActions.Add(new MissingSpAction( + ResourceName: spec.ResourceName, + ResourceAppId: spec.ResourceAppId, + Scopes: spec.Scopes?.ToArray() ?? Array.Empty(), + AzCreateCommand: azCommand, + PerSpConsentUrl: perSpConsentUrl)); + } + + /// + /// Builds the per-SP /v2.0/adminconsent URL the operator clicks AFTER manually + /// running az ad sp create --id {resourceAppId}. Unlike the broken + /// "consent the MCP app to itself" pattern (which fails with AADSTS65003 for first-party + /// token-to-self), this URL uses the BLUEPRINT as the client and the resource's actual + /// scopes as the request — a normal cross-app consent, additive to whatever the unified + /// admin-consent URL already granted in the same setup run. + /// + internal static string BuildPerSpBlueprintConsentUrl( + string tenantId, + string blueprintAppId, + ResourcePermissionSpec spec, + bool isMcpAudience = false) + { + var scopes = spec.Scopes ?? Array.Empty(); + var fullyQualified = scopes + .Select(s => $"{GetResourceUriForBlueprintConsent(spec.ResourceAppId, isMcpAudience)}/{s}"); + var scopeParam = string.Join("%20", fullyQualified.Select(Uri.EscapeDataString)); + var redirectEncoded = Uri.EscapeDataString(AuthenticationConstants.BlueprintConsentRedirectUri); + return $"https://login.microsoftonline.com/{tenantId}/v2.0/adminconsent" + + $"?client_id={blueprintAppId}" + + $"&scope={scopeParam}" + + $"&redirect_uri={redirectEncoded}" + + $"&state={Guid.NewGuid():N}"; + } + + /// + /// Resolves the resource identifier used in the per-SP unified-consent URL. Mirrors the + /// catch-all branches of : V2 MCP + /// per-server audiences (signaled via ) use the bare + /// appId GUID because their SPs have identifierUris=null; every other unknown + /// resource uses the standard api://{appId} Application ID URI form. Without + /// this split, a custom resource whose SP omits the bare GUID from + /// servicePrincipalNames would receive a recovery URL that still fails after + /// the operator provisions the SP. + /// + private static string GetResourceUriForBlueprintConsent(string resourceAppId, bool isMcpAudience) + => isMcpAudience ? resourceAppId : $"api://{resourceAppId}"; + + /// + /// Distinguishes the two flavors of blueprint permission grant for + /// . Picks which scopes on the + /// spec are surfaced to the operator (Scopes vs AppRoleScopes) and the + /// header noun ("delegated" vs "application"). + /// + private enum BlueprintPermissionKind { Delegated, Application } + + /// + /// Shared confirmation prompt for the two blueprint-permission grant fallbacks + /// (Phase 2b S2S app roles and Phase 3 delegated admin consent). Mirrors the clean + /// prompt shape used by + /// NonDwBlueprintSetupOrchestrator: list the resource:scopes that are about + /// to land on the blueprint, blank line, single [y/N] question. + /// + /// Returns true when the operator opts in (or when no confirmation provider + /// is supplied — preserves legacy "auto-yes" behavior under test). Returns + /// false when there is nothing to grant for the requested kind (caller + /// should treat this as a no-op rather than offering the runner an empty spec list). + /// + /// + private static async Task PromptForBlueprintPermissionGrantAsync( + BlueprintPermissionKind kind, + IReadOnlyList specs, + IConfirmationProvider? confirmationProvider, + ILogger logger) + { + // Per-kind wording: delegated permissions go through admin consent (tenant-wide + // OAuth2 grant); application permissions are a direct app role assignment on the + // blueprint SP. Calling the latter "admin consent" is technically incorrect and + // confused reviewers — keep the two prompts distinct. Delegated also carries a + // preamble: it is only reached as the post-browser-timeout fallback, where the + // operator needs context for why this prompt appeared without another browser open. + var (kindWord, scopesSelector) = kind switch + { + BlueprintPermissionKind.Delegated => + ("delegated", (Func?>)(s => s.Scopes)), + BlueprintPermissionKind.Application => + ("application", (Func?>)(s => s.AppRoleScopes)), + _ => throw new ArgumentOutOfRangeException(nameof(kind)) + }; + + var items = specs + .Where(s => scopesSelector(s) is { Count: > 0 }) + .Select(s => $" - {s.ResourceName}: {string.Join(", ", scopesSelector(s)!)}") + .ToList(); + + if (items.Count == 0) return false; + + // Singular vs plural is driven by total scope count, not row count: one resource row + // with two scopes is still "permissions" plural in the header. + var totalScopes = specs.Sum(s => scopesSelector(s)?.Count ?? 0); + var permissionWord = totalScopes == 1 ? "permission" : "permissions"; + var demonstrative = totalScopes == 1 ? "this" : "these"; + + var preamble = kind == BlueprintPermissionKind.Delegated + ? "Consent was not detected — unclear whether it was declined in the browser or an error occurred." + : null; + var header = $"The following {kindWord} {permissionWord} will be granted to the agent blueprint:"; + var confirmPrompt = kind == BlueprintPermissionKind.Delegated + ? $"Add {demonstrative} {permissionWord} to the blueprint programmatically? [y/N]: " + : $"Assign {demonstrative} {kindWord} {permissionWord} now? [y/N]: "; + + logger.LogInformation(""); + if (preamble is not null) + logger.LogInformation("{Preamble}", preamble); + logger.LogInformation("{Header}", header); + foreach (var item in items) + logger.LogInformation("{Item}", item); + logger.LogInformation(""); + + return confirmationProvider is null + || await confirmationProvider.ConfirmAsync(confirmPrompt); + } + /// /// Extracts the human-readable message from a Graph API JSON error response. /// Returns null if the input is not a parseable Graph error body. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs index 3586a93f..690cb1de 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs @@ -359,14 +359,21 @@ public static async Task ExecuteAsync(SetupContext ctx) // Non-fatal: a failure here (e.g. caller lacks Global Administrator) logs a warning // and continues so the agent-identity grants below still apply. await AllSubcommand.ExecuteBatchPermissionsStepAsync( - ctx, specs, + ctx, specs, buildResult.scopesByAudience, knownBlueprintSpObjectId: ctx.Config.AgentBlueprintServicePrincipalObjectId); // If admin consent wasn't granted (non-GA caller), persist per-resource consent URLs // and a combined URL so a Global Administrator can complete the hand-off out-of-band. // Messaging Bot is gated on isM365 to avoid AADSTS650053 in tenants without the Bot SP. + // V2 audience routing (issue #429): pass the full scopesByAudience map so per-server + // audiences land on the bare appId GUID resource identifier rather than collapsing + // onto the WorkIQ Tools URI. api:// is NOT used — per-server SPs have + // identifierUris null and the bare GUID is what's in servicePrincipalNames. SetupHelpers.ApplyConsentUrlsIfNeeded( - ctx, buildResult.mcpResourceAppId, ctx.Config.AgentApplicationScopes, buildResult.mcpScopes, isM365: ctx.IsM365); + ctx, buildResult.mcpResourceAppId, ctx.Config.AgentApplicationScopes, buildResult.mcpScopes, + isM365: ctx.IsM365, + mcpScopesByAudience: buildResult.scopesByAudience, + mcpAudienceDisplayNames: buildResult.serverNamesByAudience); // Save state before agent identity steps so progress (blueprint stamping outcomes, // consent URLs) is not lost on failure in the steps below. @@ -622,6 +629,8 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( // Step 6.5: Messaging endpoint registration — --m365 gated; no-op for non-M365 agents. // Skipped for --agent-registration-only (skipIdentityAndPermissions) — endpoint is already registered. + // Phase separator is emitted inside ExecuteMessagingEndpointStepAsync after the + // non-M365 early-return so non-M365 runs don't accumulate a stray blank line. if (!skipIdentityAndPermissions) await AllSubcommand.ExecuteMessagingEndpointStepAsync(ctx); @@ -629,6 +638,7 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( // to register the agent, not to regenerate appsettings files. if (!skipIdentityAndPermissions) { + ctx.Logger.LogInformation(""); ctx.Logger.LogInformation("Updating project settings..."); using (ctx.Logger.Indent()) { diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs index 1527db4b..a47b455a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs @@ -53,7 +53,7 @@ public static Command CreateCommand( var permissionsCommand = new Command("permissions", "Configure OAuth2 permission grants and inheritable permissions on the blueprint\n" + "Required role: Agent ID Developer for inheritable permissions; Global Administrator\n" + - "for tenant-wide OAuth2 consent. Non-admins get copy-paste PowerShell to forward.\n"); + "for tenant-wide OAuth2 consent. Non-admins get a unified /v2.0/adminconsent URL to forward.\n"); // Add subcommands permissionsCommand.AddCommand(CreateMcpSubcommand(logger, authValidator, configService, executor, graphApiService, blueprintService, confirmationProvider, resolver)); @@ -80,7 +80,7 @@ private static Command CreateMcpSubcommand( var command = new Command("mcp", "Configure MCP server OAuth2 grants and inheritable permissions\n" + "Required role: Agent ID Developer; Global Administrator for tenant-wide OAuth2 consent\n" + - "(non-admins receive copy-paste PowerShell to forward to an admin).\n\n"); + "(non-admins receive a unified /v2.0/adminconsent URL to forward to a Global Administrator).\n\n"); var agentNameOption = new Option( ["--agent-name", "-n"], @@ -248,7 +248,7 @@ private static Command CreateBotSubcommand( var command = new Command("bot", "Configure Messaging Bot API OAuth2 grants and inheritable permissions\n" + "Required role: Agent ID Developer; Global Administrator for tenant-wide OAuth2 consent\n" + - "(non-admins receive copy-paste PowerShell to forward to an admin).\n\n" + + "(non-admins receive a unified /v2.0/adminconsent URL to forward to a Global Administrator).\n\n" + "Prerequisites: Blueprint and MCP permissions (run 'a365 setup permissions mcp' first)\n" + "Next step: Run 'a365 publish' to package your agent for upload to the Microsoft 365 Admin Center"); @@ -296,7 +296,7 @@ private static Command CreateBotSubcommand( logger.LogInformation("Dry run: Configure Bot API Permissions"); logger.LogInformation("Would configure Bot API permissions:"); logger.LogInformation(" - Blueprint: {BlueprintId}", dryRunConfig.AgentBlueprintId); - logger.LogInformation(" - Messaging Bot API: Authorization.ReadWrite, user_impersonation"); + logger.LogInformation(" - Messaging Bot API: {Scope}", ConfigConstants.MessagingBotApiAdminConsentScope); logger.LogInformation(" - Observability API: {OtelScope} (delegated + application)", ConfigConstants.ObservabilityApiOtelWriteScope); logger.LogInformation(" - Power Platform API: Connectivity.Connections.Read"); logger.LogInformation("No changes made. Run without --dry-run to execute."); @@ -363,7 +363,7 @@ private static Command CreateCustomSubcommand( var command = new Command("custom", "Configure custom resource OAuth2 grants and inheritable permissions\n" + "Required role: Agent ID Developer; Global Administrator for tenant-wide OAuth2 consent\n" + - "(non-admins receive copy-paste PowerShell to forward to an admin).\n\n" + + "(non-admins receive a unified /v2.0/adminconsent URL to forward to a Global Administrator).\n\n" + "Prerequisites: Blueprint created (run 'a365 setup blueprint' first)\n"); var agentNameOption = new Option( @@ -679,13 +679,19 @@ public static async Task ConfigureMcpPermissionsAsync( spec.ResourceAppId, string.Join(", ", spec.Scopes)); var localResults = setupResults ?? new SetupResults(); + // Every spec built above came from scopesByAudience.Keys — they are all MCP + // per-server audiences. Passing the same set as knownMcpAudienceAppIds ensures + // the orchestrator's catch-all spec loop routes them through the bare-GUID + // branch of GetResourceIdentifierUri (api://{appId} would trigger AADSTS500011 + // because per-server SPs have identifierUris=null). var (_, _, consentGranted, adminConsentUrl) = await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( graphApiService, blueprintService, setupConfig, setupConfig.AgentBlueprintId!, setupConfig.TenantId, specs, logger, localResults, cancellationToken, knownBlueprintSpObjectId: setupConfig.AgentBlueprintServicePrincipalObjectId, confirmationProvider: confirmationProvider, - commandExecutor: executor); + commandExecutor: executor, + knownMcpAudienceAppIds: scopesByAudience.Keys.ToHashSet(StringComparer.OrdinalIgnoreCase)); // Ensure the Action Required block prints the blueprint and tenant context even when this // subcommand is run standalone (setup all populates these earlier; standalone runs don't). @@ -1066,13 +1072,25 @@ await RemoveStaleCustomPermissionsAsync( var localResults = setupResults ?? new SetupResults(); if (specList.Count > 0) { + // Operators can paste a ToolingManifest audience appId (e.g. "Windows 365 for + // Agents MCP", da81128c-...) into customPermissions. If we don't load the + // manifest here, those entries route through the api://{appId} branch in + // GetResourceIdentifierUri and trigger AADSTS500011 because per-server SPs + // have identifierUris=null. Loading the audience set lets the catch-all spec + // loop route them to the bare-GUID branch. + var customManifestPath = Path.Combine(setupConfig.DeploymentProjectPath ?? string.Empty, McpConstants.ToolingManifestFileName); + var customAtgAppId = ConfigConstants.GetAgent365ToolsResourceAppId(setupConfig.Environment); + var customManifestAudiences = await ManifestHelper.GetScopesByAudienceAsync( + customManifestPath, excludeLegacyAtg: false, resolvedAtgAppId: customAtgAppId); + var (_, _, consentGranted, adminConsentUrl) = await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( graphApiService, blueprintService, setupConfig, setupConfig.AgentBlueprintId!, setupConfig.TenantId, specList, logger, localResults, cancellationToken, knownBlueprintSpObjectId: setupConfig.AgentBlueprintServicePrincipalObjectId, confirmationProvider: confirmationProvider, - commandExecutor: executor); + commandExecutor: executor, + knownMcpAudienceAppIds: customManifestAudiences.Keys.ToHashSet(StringComparer.OrdinalIgnoreCase)); customAdminConsentUrl = adminConsentUrl; customConsentGranted = consentGranted; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PowerShellS2SRunner.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PowerShellS2SRunner.cs deleted file mode 100644 index 337f2d13..00000000 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PowerShellS2SRunner.cs +++ /dev/null @@ -1,209 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using Microsoft.Agents.A365.DevTools.Cli.Services; -using Microsoft.Extensions.Logging; -using System.Runtime.InteropServices; -using System.Text; -using System.Text.RegularExpressions; - -namespace Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; - -/// -/// Runs the S2S app role assignment PowerShell script automatically when the programmatic -/// Graph API path fails for a Global Administrator. Requires pwsh and the Microsoft.Graph -/// modules; run 'a365 setup requirements' to check prerequisites. -/// -internal static partial class PowerShellS2SRunner -{ - // Matches a standard GUID (8-4-4-4-12 hex, case-insensitive). - [GeneratedRegex(@"^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$")] - private static partial Regex GuidPattern(); - - // App role scope values must only contain alphanumeric characters, dots, hyphens, and underscores. - [GeneratedRegex(@"^[A-Za-z0-9._-]+$")] - private static partial Regex SafeScopePattern(); - - /// - /// Builds and executes the S2S app role assignment PowerShell script. - /// - /// - /// (Attempted, Succeeded): - /// - Attempted=false when prerequisites fail (bad GUID inputs, no S2S specs, pwsh not found, timeout). - /// - Succeeded=true only when the pwsh subprocess exits with code 0. - /// - public static async Task<(bool Attempted, bool Succeeded)> TryRunAsync( - CommandExecutor executor, - string tenantId, - string blueprintAppId, - IReadOnlyList specs, - ILogger logger, - CancellationToken ct) - { - if (!GuidPattern().IsMatch(tenantId) || !GuidPattern().IsMatch(blueprintAppId)) - { - logger.LogWarning("PowerShell S2S runner: invalid tenantId or blueprintAppId - skipping."); - return (false, false); - } - - var s2sSpecs = specs - .Where(s => s.AppRoleScopes is { Length: > 0 }) - .ToList(); - - if (s2sSpecs.Count == 0) - return (false, false); - - // Validate all resource app IDs and role values before building the script. - foreach (var spec in s2sSpecs) - { - if (!GuidPattern().IsMatch(spec.ResourceAppId)) - { - logger.LogWarning("PowerShell S2S runner: spec '{ResourceName}' has invalid ResourceAppId - skipping.", spec.ResourceName); - return (false, false); - } - - foreach (var role in spec.AppRoleScopes!) - { - if (!SafeScopePattern().IsMatch(role)) - { - logger.LogWarning("PowerShell S2S runner: spec '{ResourceName}' has unsafe role value '{Role}' - skipping.", spec.ResourceName, role); - return (false, false); - } - } - } - - var script = BuildScript(tenantId, blueprintAppId, s2sSpecs); - - logger.LogInformation("S2S app role assignment - connecting to Microsoft Graph. A browser window may open; sign in if prompted."); - - logger.LogDebug("Executing S2S PowerShell script via temp file..."); - logger.LogDebug("S2S PowerShell script:{NewLine}{Script}", Environment.NewLine, script); - - // Write the script to a temp file and invoke pwsh -File rather than piping - // via stdin (-Command -). When stdin carries the script, it reaches EOF as soon as the - // script is fully written; Connect-MgGraph -UseDeviceCode reads stdin as part of its - // device-code polling loop and exits immediately on EOF — auth never completes. - // With -File, stdin stays connected to the parent terminal so the device code wait works. - var tempFile = Path.Combine(Path.GetTempPath(), $"a365-s2s-{Guid.NewGuid():N}.ps1"); - CommandResult result; - - // Cap the pwsh subprocess at 5 minutes. If Connect-MgGraph hangs (e.g. on a - // headless machine where the browser launch never completes), we abandon the - // attempt rather than blocking the CLI forever. - using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - timeoutCts.CancelAfter(TimeSpan.FromMinutes(5)); - - try - { - await File.WriteAllTextAsync(tempFile, script, ct); - - // Remove environment variables that can cause assembly loading conflicts. - // This is Windows-only: the parent dotnet host injects PSModulePath / - // DOTNET_ROOT* values that collide with pwsh's own assembly resolution and - // produce "[Assembly with same name is already loaded]" failures. On - // Linux/Mac these vars are either unset or carry legitimate module search - // paths, so removing them would break module discovery instead of fixing it. - var envOverrides = new Dictionary(); - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - envOverrides["PSModulePath"] = null; - envOverrides["DOTNET_TOOLS"] = null; - envOverrides["DOTNET_ROOT"] = null; - envOverrides["DOTNET_ROOT_X64"] = null; - envOverrides["DOTNET_STARTUP_HOOKS"] = null; - envOverrides["DOTNETSTARTUPHOOKS"] = null; - } - - result = await executor.ExecuteWithStreamingAsync( - "pwsh", $"-NoProfile -ExecutionPolicy Bypass -File \"{tempFile}\"", - interactive: true, - suppressErrorLogging: true, - cancellationToken: timeoutCts.Token, - environmentOverrides: envOverrides, - redirectOutput: false); - } - catch (System.ComponentModel.Win32Exception ex) when (ex.NativeErrorCode == 2) - { - logger.LogWarning("PowerShell 7+ ('pwsh') is not installed or not on PATH. Install from https://aka.ms/powershell, then run 'a365 setup requirements' to verify."); - return (false, false); - } - catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !ct.IsCancellationRequested) - { - logger.LogWarning("PowerShell S2S runner timed out after 5 minutes. The 'Action Required' block at the end of setup contains manual steps you can run yourself."); - return (false, false); - } - finally - { - try { File.Delete(tempFile); } catch { /* best-effort cleanup */ } - } - - logger.LogDebug("pwsh exited with code {ExitCode}", result.ExitCode); - - // Note: stdout/stderr are not redirected (redirectOutput: false) so the child - // writes directly to the console. Success is determined by the exit code. - var succeeded = result.ExitCode == 0; - return (true, succeeded); - } - - private static string BuildScript( - string tenantId, - string blueprintAppId, - IReadOnlyList s2sSpecs) - { - var sb = new StringBuilder(); - - // Stop on any non-terminating error so the script's exit code accurately reflects success/failure. - sb.AppendLine("$ErrorActionPreference = 'Stop'"); - sb.AppendLine(""); - - // Force-load the Graph modules to avoid assembly loading conflicts. Pin the highest - // installed version of each module and import by absolute path so PowerShell does not - // silently pick up a different version through its standard probing. - // Authentication must be imported before Applications because Connect-MgGraph lives in - // Authentication and Applications transitively requires it. Exit code 2 is reserved for - // "modules missing" so callers can distinguish a missing-prereq from an auth failure. - sb.AppendLine("foreach ($name in @('Microsoft.Graph.Authentication','Microsoft.Graph.Applications')) {"); - sb.AppendLine(" $m = Get-Module $name -ListAvailable | Sort-Object Version -Descending | Select-Object -First 1"); - sb.AppendLine(" if (-not $m) { Write-Error \"Required PowerShell module '$name' is not installed. Run: Install-Module $name -Scope CurrentUser\"; exit 2 }"); - sb.AppendLine(" Import-Module $m.Path -Force"); - sb.AppendLine("}"); - sb.AppendLine(""); - - // Disconnect any stale cached session first; a reused DeviceCodeCredential from a prior - // run can leave the context in a broken state where Connect-MgGraph returns without error - // but subsequent cmdlets throw a NullReferenceException inside the credential object. - sb.AppendLine("try { Disconnect-MgGraph -Confirm:$false -ErrorAction SilentlyContinue } catch { }"); - // -ContextScope Process forces an in-memory-only connection, bypassing the persistent token - // cache. Without this, Connect-MgGraph reloads a stale DeviceCodeCredential from disk even - // after Disconnect-MgGraph, causing a NullReferenceException in subsequent cmdlets. - sb.AppendLine($"Connect-MgGraph -TenantId '{tenantId}' -Scopes 'AppRoleAssignment.ReadWrite.All','Application.Read.All' -NoWelcome -ContextScope Process"); - // Guard: Connect-MgGraph can return without throwing even when auth did not complete. - sb.AppendLine("$_ctx = Get-MgContext"); - sb.AppendLine("if (-not $_ctx -or [string]::IsNullOrEmpty($_ctx.Account)) { Write-Error 'Authentication did not complete - no account in context after Connect-MgGraph'; exit 1 }"); - sb.AppendLine($"$bp = Get-MgServicePrincipal -Filter \"appId eq '{blueprintAppId}'\""); - sb.AppendLine("if (-not $bp) { Write-Error 'Blueprint SP not found'; exit 1 }"); - - foreach (var spec in s2sSpecs) - { - // PowerShell escapes a single-quote inside a single-quoted string by doubling it. - var safeResourceName = spec.ResourceName.Replace("'", "''"); - foreach (var role in spec.AppRoleScopes!) - { - sb.AppendLine($"# {safeResourceName}: {role}"); - sb.AppendLine($"$res = Get-MgServicePrincipal -Filter \"appId eq '{spec.ResourceAppId}'\""); - sb.AppendLine($"if (-not $res) {{ Write-Error 'Resource SP not found for {safeResourceName}'; exit 1 }}"); - sb.AppendLine($"$rid = ($res.AppRoles | Where-Object {{ $_.Value -eq '{role}' }}).Id"); - sb.AppendLine($"if (-not $rid) {{ Write-Error 'App role not found: {role}'; exit 1 }}"); - // Idempotent: skip if already assigned (error code 'Request_MultipleObjectsWithSameKeyValue'). - sb.AppendLine("$existing = Get-MgServicePrincipalAppRoleAssignment -ServicePrincipalId $bp.Id | Where-Object { $_.AppRoleId -eq $rid -and $_.ResourceId -eq $res.Id }"); - sb.AppendLine("if (-not $existing) {"); - sb.AppendLine(" New-MgServicePrincipalAppRoleAssignment -ServicePrincipalId $bp.Id -PrincipalId $bp.Id -ResourceId $res.Id -AppRoleId $rid | Out-Null"); - sb.AppendLine("}"); - } - } - - sb.AppendLine("Write-Output 'A365-S2S-OK'"); - return sb.ToString(); - } - -} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs index 2c29ab06..74f48088 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs @@ -67,12 +67,26 @@ internal sealed class SetupContext /// Null or "obo" — principal-scoped delegated grants; no admin consent needed. public bool IsOboMode => AuthMode is null || string.Equals(AuthMode, "obo", StringComparison.OrdinalIgnoreCase); - /// "s2s" — app role assignments on agent identity; Global Admin needed or PowerShell fallback. + /// "s2s" — app role assignments on agent identity; Global Admin needed or az rest fallback. public bool IsS2sMode => string.Equals(AuthMode, "s2s", StringComparison.OrdinalIgnoreCase); /// "both" — delegated grants (OBO) and app role assignments (S2S). public bool IsBothMode => string.Equals(AuthMode, "both", StringComparison.OrdinalIgnoreCase); + /// + /// When true, skip the interactive in-line provisioning of missing resource service + /// principals (issue #429). The default flow prompts per-resource and shells out to + /// az ad sp create --id <appId> via the operator's existing az login + /// (Global Administrator's directory role carries the required + /// Application.ReadWrite.All). With this set, missing SPs are excluded from the + /// unified admin-consent URL and recorded on + /// so the setup summary's Action Required block renders them as numbered items, each + /// with the az ad sp create command and a per-SP /v2.0/adminconsent URL. + /// Set explicitly via --skip-sp-provisioning or implicitly when stdin is + /// redirected (CI / coding-agent / pipe scenarios). + /// + public bool SkipSpProvisioning { get; } + /// /// Overrides the az CLI login hint resolver used during blueprint creation. /// Null in production — injected as a no-op in tests to avoid spawning 'az account show'. @@ -124,7 +138,8 @@ public SetupContext( bool isM365 = false, string? authMode = null, Func>? loginHintResolver = null, - IConfirmationProvider? confirmationProvider = null) + IConfirmationProvider? confirmationProvider = null, + bool skipSpProvisioning = false) { Config = config; Results = results; @@ -139,6 +154,7 @@ public SetupContext( IsBootstrap = isBootstrap; IsM365 = isM365; AuthMode = string.IsNullOrWhiteSpace(authMode) ? null : authMode.ToLowerInvariant(); + SkipSpProvisioning = skipSpProvisioning; ConfigService = configService; Executor = executor; BackendConfigurator = backendConfigurator; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs index ff36f53e..8c495f9f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -59,10 +59,18 @@ internal static ResourcePermissionSpec[] GetFixedApiPermissionSpecs(bool setInhe var specs = new List(); if (isM365) { + // The Messaging Bot API resource SP exposes a single delegated scope: + // ConfigConstants.MessagingBotApiAdminConsentScope ("AgentData.ReadWrite"). Requesting + // any other scope name here (the previous "Authorization.ReadWrite" + "user_impersonation" + // pair did not exist on the resource) causes the unified /v2.0/adminconsent URL to be + // rejected with AADSTS650053. Pre-PR #424 the orchestrator went through a programmatic + // oauth2PermissionGrants POST that did not strictly validate scope existence; the unified + // URL endpoint does, so the spec must agree with the URL builders that already use this + // constant (BuildAdminConsentUrls, BuildCombinedConsentUrl). See issue #429. specs.Add(new ResourcePermissionSpec( ConfigConstants.MessagingBotApiAppId, "Messaging Bot API", - new[] { "Authorization.ReadWrite", "user_impersonation" }, + new[] { ConfigConstants.MessagingBotApiAdminConsentScope }, setInheritable)); } specs.Add(new ResourcePermissionSpec( @@ -98,8 +106,15 @@ internal static async Task> BuildConfiguredPermissi Agent365Config config, bool setInheritable, bool isM365 = true, - Dictionary? scopesByAudience = null) + Dictionary? scopesByAudience = null, + Dictionary>? serverNamesByAudience = null) { + // Manifest read at most once, and only when scopesByAudience is not pre-supplied. + // Callers that already have the manifest loaded (e.g. AllSubcommand.BuildPermissionSpecsAsync) + // pass both maps to avoid a second disk read; the BootstrapTests path supplies only + // scopesByAudience and accepts the generic "Agent 365 Tools" display fallback. This + // preserves the no-manifest-read contract documented in + // BuildConfiguredPermissionSpecsAsync_WithPreComputedScopes_DoesNotReadManifest. if (scopesByAudience is null) { var mcpManifestPath = Path.Combine( @@ -107,7 +122,9 @@ internal static async Task> BuildConfiguredPermissi McpConstants.ToolingManifestFileName); var atgAppId = ConfigConstants.GetAgent365ToolsResourceAppId(config.Environment); scopesByAudience = await ManifestHelper.GetScopesByAudienceAsync(mcpManifestPath, excludeLegacyAtg: false, resolvedAtgAppId: atgAppId); + serverNamesByAudience ??= await ManifestHelper.GetServerNamesByAudienceAsync(mcpManifestPath, atgAppId); } + serverNamesByAudience ??= new Dictionary>(StringComparer.OrdinalIgnoreCase); var specs = new List { @@ -118,8 +135,17 @@ internal static async Task> BuildConfiguredPermissi SetInheritable: setInheritable), }; + // Per-server V2 audiences read their manifest McpServerName (e.g. "mcp_MailTools") + // instead of the generic shared name. The legacy shared ATG audience is excluded by + // GetServerNamesByAudienceAsync and falls through to "Agent 365 Tools" here. specs.AddRange(scopesByAudience.Select(kvp => - new ResourcePermissionSpec(kvp.Key, "Agent 365 Tools", kvp.Value, SetInheritable: setInheritable))); + new ResourcePermissionSpec( + kvp.Key, + serverNamesByAudience.TryGetValue(kvp.Key, out var names) && names.Count > 0 + ? names[0] + : "Agent 365 Tools", + kvp.Value, + SetInheritable: setInheritable))); specs.AddRange(GetFixedApiPermissionSpecs(setInheritable, isM365)); foreach (var customPerm in config.CustomBlueprintPermissions ?? new List()) @@ -732,7 +758,8 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger) results.MessagingEndpointResult == Models.EndpointRegistrationResult.SkippedContractMismatch; var messagingEndpointFailureRequired = results.MessagingEndpointResult == Models.EndpointRegistrationResult.Failed; - var hasActionRequired = pendingAdminAction || tenantConsentUnverified || results.ClientSecretManualActionRequired || pendingS2SAction || pendingDelegatedAction || messagingEndpointManualRequired || messagingEndpointFailureRequired; + var hasMissingSpActions = results.MissingSpActions.Count > 0; + var hasActionRequired = pendingAdminAction || tenantConsentUnverified || results.ClientSecretManualActionRequired || pendingS2SAction || pendingDelegatedAction || messagingEndpointManualRequired || messagingEndpointFailureRequired || hasMissingSpActions; if (hasActionRequired) { var blueprintAppId = results.BlueprintId ?? ""; @@ -888,6 +915,27 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger) logger.LogInformation(" {Url}", ConfigConstants.TeamsDeveloperPortalConfigureEndpointUrl); } } + if (hasMissingSpActions) + { + // Issue #429: resources whose SP could not be provisioned in-line during + // setup. Each entry is a two-step recovery the operator can complete + // without re-running 'a365 setup all': (1) provision the SP via az, + // (2) click the per-SP unified-consent URL to grant the blueprint consent + // for this resource's scopes. Step 2 is keyed to the BLUEPRINT as client + // (not the resource as client — that pattern fails AADSTS65003 for + // first-party token-to-self), so it is a normal cross-app consent and + // additive to whatever the unified consent URL already granted. + foreach (var action in results.MissingSpActions) + { + actionCount++; + logger.LogInformation(" {N}. Missing service principal — '{Name}' ({AppId}) (Global Administrator required)", actionCount, action.ResourceName, action.ResourceAppId); + logger.LogInformation(" Scopes pending: {Scopes}", string.Join(", ", action.Scopes)); + logger.LogInformation(" Step 1) Provision the SP:"); + logger.LogInformation(" {AzCommand}", action.AzCreateCommand); + logger.LogInformation(" Step 2) Grant the blueprint consent for this resource (click Accept):"); + logger.LogInformation(" {Url}", action.PerSpConsentUrl); + } + } } if (results.Errors.Count > 0) @@ -973,11 +1021,18 @@ internal static List PopulateAdminConsentUrls( Agent365Config config, string mcpResourceAppId, IEnumerable mcpScopes, - bool isM365 = true) + bool isM365 = true, + IReadOnlyDictionary? mcpScopesByAudience = null, + IReadOnlyDictionary>? mcpAudienceDisplayNames = null) { - var urls = BuildAdminConsentUrls(config.TenantId, config.AgentBlueprintId!, config.AgentApplicationScopes, mcpScopes, isM365); - - // Map resource names to App IDs for upsert into ResourceConsents + var urls = BuildAdminConsentUrls(config.TenantId, config.AgentBlueprintId!, config.AgentApplicationScopes, mcpScopes, isM365, mcpScopesByAudience, mcpAudienceDisplayNames); + + // Map resource names to App IDs for upsert into ResourceConsents. The fixed-name + // entries cover Graph + Bot + Obs + PP + the WorkIQ shared MCP audience. V2 + // per-server audiences (issue #429) are emitted by BuildAdminConsentUrls with + // display names like "Agent 365 Tools (16b1878d-...)"; for those we extract the + // appId from the display name and create one ResourceConsent per audience so + // query-entra and the setup summary surface the right SP. var appIdByName = new Dictionary(StringComparer.OrdinalIgnoreCase) { ["Microsoft Graph"] = AuthenticationConstants.MicrosoftGraphResourceAppId, @@ -990,7 +1045,20 @@ internal static List PopulateAdminConsentUrls( var populated = new List(); foreach (var (resourceName, consentUrl) in urls) { - if (!appIdByName.TryGetValue(resourceName, out var appId)) continue; + string appId; + if (appIdByName.TryGetValue(resourceName, out var fixedAppId)) + { + appId = fixedAppId; + } + else if (TryExtractAudienceAppIdFromResourceName(resourceName, out var audienceAppId)) + { + // V2 per-server audience — appId is embedded in the display name we generated. + appId = audienceAppId; + } + else + { + continue; + } var existing = config.ResourceConsents.FirstOrDefault( rc => rc.ResourceAppId.Equals(appId, StringComparison.OrdinalIgnoreCase)); @@ -1013,6 +1081,27 @@ internal static List PopulateAdminConsentUrls( return populated; } + /// + /// Parses display names produced by for V2 + /// per-server audiences, e.g. "Agent 365 Tools (16b1878d-62c7-4009-aa25-68989d63bbad)" + /// or the per-server form "mcp_MailTools (16b1878d-...)". Prefix-agnostic so the + /// extractor keeps working after BuildAdminConsentUrls started threading manifest-derived + /// server names into the display label. Returns the embedded audience appId when the name + /// ends with a parenthesized GUID; false otherwise. + /// + private static bool TryExtractAudienceAppIdFromResourceName(string resourceName, out string audienceAppId) + { + audienceAppId = string.Empty; + if (string.IsNullOrWhiteSpace(resourceName)) return false; + if (!resourceName.EndsWith(')')) return false; + var openIdx = resourceName.LastIndexOf('('); + if (openIdx < 0 || openIdx >= resourceName.Length - 2) return false; + var inner = resourceName.Substring(openIdx + 1, resourceName.Length - openIdx - 2); + if (!Guid.TryParse(inner, out _)) return false; + audienceAppId = inner; + return true; + } + /// /// Builds a single /v2.0/adminconsent URL from fully-qualified scope URIs. /// All callers must pass fully-qualified scopes (e.g. "https://graph.microsoft.com/User.Read"). @@ -1027,26 +1116,23 @@ internal static string BuildAdminConsentUrl(string tenantId, string clientId, IE } /// - /// Returns the canonical identifier URI for a known platform resource app ID - /// (Graph, Agent 365 Tools, Messaging Bot, Observability, Power Platform). For any - /// unknown app ID, returns the universally-valid api://{appId} form. - /// - /// This is the single source of truth for building fully-qualified OAuth2 scope URIs - /// used in the /v2.0/adminconsent flow. Both the per-resource builder - /// () and the combined-URL builder used by - /// BatchPermissionsOrchestrator resolve resource URIs through this helper so - /// the user always sees the same scope identifiers regardless of which code path - /// produced the URL. - /// - /// - /// The Agent 365 Tools (MCP) resource app ID is tenant-discovered and not a static - /// constant, so callers that know they are building scopes for MCP must pass the - /// resource name ("Agent 365 Tools") to resolve the canonical URI; without it, the - /// method falls back to api://{appId} (functionally equivalent but visually - /// inconsistent with the per-resource URL). - /// + /// Returns the resource identifier the /v2.0/adminconsent endpoint expects as the + /// scope prefix for . + /// + /// Known platform resources (Graph, WorkIQ Tools, Messaging Bot, Observability, + /// Power Platform) → their canonical identifier URI. + /// =true (V2 MCP per-server audience) → bare + /// appId GUID. Those SPs have identifierUris=null; api://{appId} triggers + /// AADSTS500011. + /// Everything else → the standard api://{appId} Application ID URI form. + /// /// - internal static string GetResourceIdentifierUri(string resourceAppId, string? resourceName = null) + /// The application id of the resource. + /// True when the caller knows + /// is a V2 MCP per-server audience (e.g. it sits in the ToolingManifest audience set + /// or the call site is iterating mcpScopesByAudience). Default false preserves + /// the safe api://{appId} fallback for any caller that has not been updated. + internal static string GetResourceIdentifierUri(string resourceAppId, bool isMcpAudience = false) { if (string.Equals(resourceAppId, AuthenticationConstants.MicrosoftGraphResourceAppId, StringComparison.OrdinalIgnoreCase)) return AuthenticationConstants.MicrosoftGraphResourceUri; @@ -1056,18 +1142,59 @@ internal static string GetResourceIdentifierUri(string resourceAppId, string? re return ConfigConstants.ObservabilityApiIdentifierUri; if (string.Equals(resourceAppId, PowerPlatformConstants.PowerPlatformApiResourceAppId, StringComparison.OrdinalIgnoreCase)) return PowerPlatformConstants.PowerPlatformApiIdentifierUri; - if (string.Equals(resourceName, "Agent 365 Tools", StringComparison.OrdinalIgnoreCase)) + // WorkIQ Tools shared (issue #429): match by appId, not display name. V2 per-server + // audiences are also named "Agent 365 Tools" so the old name-based check collapsed + // them onto WorkIQ's URI and produced AADSTS650053. + if (IsAgent365ToolsResourceAppId(resourceAppId)) return McpConstants.Agent365ToolsIdentifierUri; + + // V2 MCP per-server audiences (identifierUris=null, only bare appId in + // servicePrincipalNames). Caller signals this via isMcpAudience=true — either + // because it is iterating mcpScopesByAudience, or because it checked the loaded + // ToolingManifest audience set for this appId. + if (isMcpAudience) + return resourceAppId; + return $"api://{resourceAppId}"; } + /// + /// Returns true when the supplied resource appId is the WorkIQ Tools (Agent 365 Tools) + /// shared resource — either the hard-coded prod appId or an env-overridden value + /// pinned via A365_MCP_APP_ID_<env>. Used by + /// to distinguish the WorkIQ shared audience + /// (returns canonical https URI) from V2 MCP per-server audiences (returns bare appId + /// GUID because per-server SPs have identifierUris = null and Entra rejects + /// api://{appId} for them with AADSTS500011). + /// + private static bool IsAgent365ToolsResourceAppId(string resourceAppId) + { + if (string.IsNullOrWhiteSpace(resourceAppId)) return false; + if (string.Equals(resourceAppId, McpConstants.WorkIQToolsProdAppId, StringComparison.OrdinalIgnoreCase)) + return true; + // Also accept any value the environment-aware resolver returns for known env keys. + // Cheaper than walking every possible env: only check the env on the running config + // when explicitly passed via env var. ConfigConstants.GetAgent365ToolsResourceAppId + // already short-circuits to the prod appId when no override is set. + foreach (var envKey in new[] { "prod", "preprod", "test", "dev" }) + { + var resolved = ConfigConstants.GetAgent365ToolsResourceAppId(envKey); + if (string.Equals(resourceAppId, resolved, StringComparison.OrdinalIgnoreCase)) + return true; + } + return false; + } + /// /// Builds a fully-qualified OAuth2 scope URI for use in the /v2.0/adminconsent URL. /// Resolves the resource URI via so the resulting /// scope identifier matches what the per-resource URL builder emits. /// - internal static string BuildFullyQualifiedScope(string resourceAppId, string scope, string? resourceName = null) - => $"{GetResourceIdentifierUri(resourceAppId, resourceName)}/{scope}"; + /// Forwarded to ; pass + /// true when the caller knows is a V2 MCP per-server + /// audience (e.g. found in the loaded ToolingManifest audience set). Default false. + internal static string BuildFullyQualifiedScope(string resourceAppId, string scope, bool isMcpAudience = false) + => $"{GetResourceIdentifierUri(resourceAppId, isMcpAudience)}/{scope}"; /// /// Builds per-resource admin consent URLs covering every resource stamped on the blueprint @@ -1087,7 +1214,9 @@ internal static string BuildFullyQualifiedScope(string resourceAppId, string sco string blueprintClientId, IEnumerable graphScopes, IEnumerable mcpScopes, - bool isM365 = true) + bool isM365 = true, + IReadOnlyDictionary? mcpScopesByAudience = null, + IReadOnlyDictionary>? mcpAudienceDisplayNames = null) { var urls = new List<(string, string)>(); @@ -1098,9 +1227,53 @@ static string Build(string tenant, string client, string resourceUri, IEnumerabl if (graphScopeList.Count > 0) urls.Add(("Microsoft Graph", Build(tenantId, blueprintClientId, AuthenticationConstants.MicrosoftGraphResourceUri, graphScopeList))); - var mcpScopeList = mcpScopes.ToList(); - if (mcpScopeList.Count > 0) - urls.Add(("Agent 365 Tools", Build(tenantId, blueprintClientId, McpConstants.Agent365ToolsIdentifierUri, mcpScopeList))); + // V2 per-server audiences (issue #429): when the caller passes a by-audience map, + // emit one URL fragment per audience whose resource identifier is resolved by + // GetResourceIdentifierUri (the WorkIQ shared audience keeps its canonical https + // URI; per-server audiences use the bare appId GUID — see GetResourceIdentifierUri + // for why api://{appId} fails with AADSTS500011 against per-server SPs). The legacy + // flat-list path is preserved unchanged below for V1 + // callers and existing tests that have no by-audience info to thread through. + if (mcpScopesByAudience is { Count: > 0 }) + { + foreach (var (audienceAppId, scopes) in mcpScopesByAudience) + { + if (scopes is null || scopes.Length == 0) continue; + // The loop iterates over manifest-derived MCP audiences; every key here is + // by definition an MCP per-server audience appId. + var resourceUri = GetResourceIdentifierUri(audienceAppId, isMcpAudience: true); + // Display name: WorkIQ shared audience keeps the legacy "Agent 365 Tools" + // label. Per-server audiences use the manifest McpServerName when supplied + // (e.g. "mcp_MailTools (16b1878d-...)") so the consent URL block matches the + // names shown in the spec list and grant log lines. Fall back to the generic + // "Agent 365 Tools ({appId})" form when no display-name map is passed in + // (legacy callers, tests). The parenthetical appId is required by + // TryExtractAudienceAppIdFromResourceName for the PopulateAdminConsentUrls + // upsert path. + string resourceName; + if (IsAgent365ToolsResourceAppId(audienceAppId)) + { + resourceName = "Agent 365 Tools"; + } + else if (mcpAudienceDisplayNames is not null + && mcpAudienceDisplayNames.TryGetValue(audienceAppId, out var names) + && names.Count > 0) + { + resourceName = $"{names[0]} ({audienceAppId})"; + } + else + { + resourceName = $"Agent 365 Tools ({audienceAppId})"; + } + urls.Add((resourceName, Build(tenantId, blueprintClientId, resourceUri, scopes))); + } + } + else + { + var mcpScopeList = mcpScopes.ToList(); + if (mcpScopeList.Count > 0) + urls.Add(("Agent 365 Tools", Build(tenantId, blueprintClientId, McpConstants.Agent365ToolsIdentifierUri, mcpScopeList))); + } if (isM365) urls.Add(("Messaging Bot API", Build(tenantId, blueprintClientId, ConfigConstants.MessagingBotApiIdentifierUri, new[] { ConfigConstants.MessagingBotApiAdminConsentScope }))); @@ -1127,13 +1300,37 @@ internal static string BuildCombinedConsentUrl( string blueprintClientId, IEnumerable graphScopes, IEnumerable mcpScopes, - bool isM365 = true) + bool isM365 = true, + IReadOnlyDictionary? mcpScopesByAudience = null) { var allScopes = new List(); foreach (var s in graphScopes) allScopes.Add($"{AuthenticationConstants.MicrosoftGraphResourceUri}/{s}"); - foreach (var s in mcpScopes) - allScopes.Add($"{McpConstants.Agent365ToolsIdentifierUri}/{s}"); + + // V2 per-server audiences (issue #429): when the caller passes a by-audience map, + // emit per-audience scope URIs using GetResourceIdentifierUri so the WorkIQ + // shared audience keeps its https URI and per-server audiences use the bare appId + // GUID (api://{appId} fails for per-server SPs with AADSTS500011 — see + // GetResourceIdentifierUri). Without this, every MCP scope landed on the WorkIQ URI + // and Entra returned AADSTS650053 for any scope the WorkIQ SP did not publish. + if (mcpScopesByAudience is { Count: > 0 }) + { + foreach (var (audienceAppId, scopes) in mcpScopesByAudience) + { + if (scopes is null) continue; + // The loop iterates over manifest-derived MCP audiences; every key here is + // by definition an MCP per-server audience appId. + var resourceUri = GetResourceIdentifierUri(audienceAppId, isMcpAudience: true); + foreach (var s in scopes) + allScopes.Add($"{resourceUri}/{s}"); + } + } + else + { + foreach (var s in mcpScopes) + allScopes.Add($"{McpConstants.Agent365ToolsIdentifierUri}/{s}"); + } + if (isM365) allScopes.Add($"{ConfigConstants.MessagingBotApiIdentifierUri}/{ConfigConstants.MessagingBotApiAdminConsentScope}"); allScopes.Add($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiOtelWriteScope}"); @@ -1157,17 +1354,19 @@ internal static void ApplyConsentUrlsIfNeeded( string mcpResourceAppId, IEnumerable graphScopes, IEnumerable mcpScopes, - bool isM365 = true) + bool isM365 = true, + IReadOnlyDictionary? mcpScopesByAudience = null, + IReadOnlyDictionary>? mcpAudienceDisplayNames = null) { if (ctx.Results.TenantWideConsentOutcome == Models.GrantOutcome.Granted || string.IsNullOrWhiteSpace(ctx.Config.AgentBlueprintId)) return; - var consentResourceNames = PopulateAdminConsentUrls(ctx.Config, mcpResourceAppId, mcpScopes, isM365); + var consentResourceNames = PopulateAdminConsentUrls(ctx.Config, mcpResourceAppId, mcpScopes, isM365, mcpScopesByAudience, mcpAudienceDisplayNames); ctx.Results.ConsentUrlsSavedToPath = ctx.GeneratedConfigPath; ctx.Results.ConsentResourceNames.AddRange(consentResourceNames); ctx.Results.CombinedConsentUrl = BuildCombinedConsentUrl( ctx.Config.TenantId!, ctx.Config.AgentBlueprintId!, - graphScopes, mcpScopes, isM365); + graphScopes, mcpScopes, isM365, mcpScopesByAudience); } /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs index b6f6349a..b67a9e80 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs @@ -277,6 +277,36 @@ public class SetupResults public List Errors { get; } = new(); public List Warnings { get; } = new(); + /// + /// Resources whose service principal could not be provisioned in-line during setup + /// (operator declined the per-SP prompt, az ad sp create failed, or + /// --skip-sp-provisioning was set). Each entry is a fully-actionable pair: the + /// az ad sp create command to provision the SP plus the per-SP unified-consent + /// URL that grants the blueprint consent for this resource's scopes. The setup + /// summary's "Action Required" block renders these as numbered items so the operator + /// can complete provisioning without re-running setup. + /// + public List MissingSpActions { get; } = new(); + public bool HasErrors => Errors.Count > 0; public bool HasWarnings => Warnings.Count > 0; } + +/// +/// One entry in . Resource identity plus the +/// two concrete commands/URLs the operator needs to complete provisioning manually: +/// (1) the az ad sp create command that creates the SP in the tenant, and +/// (2) the per-SP /v2.0/adminconsent URL that grants the blueprint consent for +/// this resource's delegated scopes once the SP exists. +/// +/// Human-readable display name (e.g. "Work IQ Teams MCP"). +/// Application ID of the resource (the GUID). +/// Delegated scopes the blueprint needs on this resource. +/// Copy-paste-able az ad sp create --id .... +/// Per-SP unified-consent URL keyed to the blueprint as client and the resource scopes as the request. +public sealed record MissingSpAction( + string ResourceName, + string ResourceAppId, + string[] Scopes, + string AzCreateCommand, + string PerSpConsentUrl); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs index ad72a98c..bf2fe665 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs @@ -81,10 +81,13 @@ public static class ConfigConstants public const string ObservabilityApiIdentifierUri = "api://9b975845-388f-4429-889e-eab1ef63949c"; /// - /// Messaging Bot API scope used for admin consent URL construction. - /// Note: the orchestrator grants "Authorization.ReadWrite" + "user_impersonation" via OAuth2 - /// permission grants; this scope name is what the /adminconsent endpoint accepts for the - /// same resource and maps to the same effective consent. + /// Single source of truth for the Messaging Bot API delegated scope. + /// The resource SP (appId 5a807f24-c9de-44ee-a3a7-329e88a00ffc) exposes exactly + /// one delegated scope, "AgentData.ReadWrite". Both the per-resource and combined + /// /v2.0/adminconsent URL builders and the spec list consumed by + /// BatchPermissionsOrchestrator must reference this constant — a mismatch causes + /// the strict /v2.0/adminconsent endpoint to reject the entire URL with + /// AADSTS650053 (issue #429). /// public const string MessagingBotApiAdminConsentScope = "AgentData.ReadWrite"; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AdminConsentHelper.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AdminConsentHelper.cs index ea2e5af0..d02b6e7e 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AdminConsentHelper.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AdminConsentHelper.cs @@ -24,10 +24,10 @@ public enum ConsentPollResult Verified, /// - /// The timeout elapsed without detecting a grant, or the user pressed Enter to skip - /// verification. The CLI did NOT observe the grant directly. Callers must NOT update - /// persisted consent state on this outcome and must keep the consent URL visible so the - /// user can verify manually (for example via 'a365 query-entra inheritance'). + /// The timeout elapsed without detecting a grant. The CLI did NOT observe the grant + /// directly. Callers must NOT update persisted consent state on this outcome and must + /// keep the consent URL visible so the user can verify manually (for example via + /// 'a365 query-entra inheritance'). /// AssumedComplete, @@ -67,32 +67,6 @@ internal static bool BypassConsentChecksForTests private static readonly AsyncLocal _bypassConsentChecks = new(); - /// - /// Non-blocking check for a buffered Enter keypress. Used by the consent polling loop - /// to let the operator skip waiting and continue without blocking on stdin. - /// Safe when stdin is redirected (e.g. test/CI): returns false and any other buffered keys - /// are consumed harmlessly. Returns true only when an Enter key was pressed and consumed. - /// - private static bool TryConsumeEnterKey() - { - try - { - while (Console.KeyAvailable) - { - var key = Console.ReadKey(intercept: true); - if (key.Key == ConsoleKey.Enter) - { - return true; - } - } - } - catch - { - // Console.KeyAvailable throws when stdin is redirected. Treat as "no Enter". - } - return false; - } - /// /// Polls Azure AD/Graph (via az rest) to detect an oauth2 permission grant for the provided appId. /// Mirrors the behavior previously implemented in A365SetupRunner.PollAdminConsentAsync. @@ -114,7 +88,7 @@ public static async Task PollAdminConsentAsync( int lastProgressReportSeconds = 0; logger.LogInformation( - "Waiting for admin consent to be granted. Open the URL above in a browser and complete the consent flow. The CLI will continue automatically (timeout: {TimeoutSeconds}s).", + " Sign in and Accept the permission(s). If the tab shows an error after Accept, consent likely succeeded — the CLI will still detect it (timeout: {TimeoutSeconds}s).", timeoutSeconds); try @@ -173,21 +147,21 @@ public static async Task PollAdminConsentAsync( } } - // Delay between polls. If cancellation is requested this will throw OperationCanceledException, - // which we catch below and treat as a graceful cancellation resulting in 'false'. await Task.Delay(TimeSpan.FromSeconds(intervalSeconds), ct); } - logger.LogWarning( - "Admin consent was not detected within {TimeoutSeconds}s. Continuing — you can re-run this command after granting consent.", + logger.LogDebug( + "Admin consent was not detected within {TimeoutSeconds}s. The browser flow may not have completed, or 'az login' may be signed into a different tenant than the consent target. Verify with 'az account show'.", timeoutSeconds); return false; } catch (OperationCanceledException) { - // Treat cancellation as a graceful timeout/no-consent scenario - logger.LogDebug("Polling for admin consent was cancelled or timed out for app {AppId} ({Scope}).", appId, scopeDescriptor); - return false; + // Propagate so Ctrl+C aborts setup cleanly via AllSubcommand's OCE handler, + // instead of falling into the az rest fallback prompt with a stale "permission(s)?" + // question. Mirrors the Graph overload below. + logger.LogDebug("Polling for admin consent was cancelled for app {AppId} ({Scope}).", appId, scopeDescriptor); + throw; } } @@ -202,9 +176,8 @@ public static async Task PollAdminConsentAsync( /// /// when a grant was observed in Graph. /// when the timeout elapsed without detecting - /// a grant, or when the user pressed Enter to skip verification — the grant was NOT directly - /// observed. Callers must NOT update persisted consent state on this outcome and must keep - /// the consent URL visible so the user can verify manually. + /// a grant — the grant was NOT directly observed. Callers must NOT update persisted consent + /// state on this outcome and must keep the consent URL visible so the user can verify manually. /// when the blueprint SP id is not available. /// Throws when is cancelled — /// callers must let the exception propagate so user Ctrl+C is honored consistently with the @@ -233,7 +206,7 @@ public static async Task PollAdminConsentAsync( } logger.LogInformation( - "Waiting for admin consent to be granted. Complete the consent flow in the browser. The CLI will continue automatically (timeout: {TimeoutSeconds}s).", + " Sign in and Accept the permission(s). If the tab shows an error after Accept, consent likely succeeded — the CLI will still detect it (timeout: {TimeoutSeconds}s).", timeoutSeconds); var start = DateTime.UtcNow; @@ -248,16 +221,10 @@ public static async Task PollAdminConsentAsync( { lastProgressReportSeconds = elapsedSeconds; logger.LogInformation( - "Still waiting for admin consent... ({ElapsedSeconds}s / {TimeoutSeconds}s). Press Enter to skip verification and continue.", + "Still waiting for admin consent... ({ElapsedSeconds}s / {TimeoutSeconds}s).", elapsedSeconds, timeoutSeconds); } - if (TryConsumeEnterKey()) - { - logger.LogInformation("Continuing. Run 'a365 query-entra inheritance' later to confirm permissions if needed."); - return ConsentPollResult.AssumedComplete; - } - // Use the caller's full permission scopes so the request uses the broad delegated // token (which includes Application.Read.All and other admin-level scopes) rather // than the default User.Read-only token, which is denied on oauth2PermissionGrants. @@ -277,22 +244,11 @@ public static async Task PollAdminConsentAsync( logger.LogDebug("No consent grants found for blueprint SP {ClientSpId} yet.", clientSpId); - // Short-poll loop so an Enter keypress is detected within 250 ms rather than - // waiting a full intervalSeconds before the next Graph check. - var pollEnd = DateTime.UtcNow.AddSeconds(intervalSeconds); - while (DateTime.UtcNow < pollEnd && !ct.IsCancellationRequested) - { - if (TryConsumeEnterKey()) - { - logger.LogInformation("Continuing. Run 'a365 query-entra inheritance' later to confirm permissions if needed."); - return ConsentPollResult.AssumedComplete; - } - await Task.Delay(250, ct); - } + await Task.Delay(TimeSpan.FromSeconds(intervalSeconds), ct); } - logger.LogWarning( - "Admin consent was not detected within {TimeoutSeconds}s. Continuing — run 'a365 query-entra inheritance' later to verify.", + logger.LogDebug( + "Admin consent was not detected within {TimeoutSeconds}s. The browser flow may not have completed, or 'az login' may be signed into a different tenant than the consent target. Verify with 'az account show'.", timeoutSeconds); return ConsentPollResult.AssumedComplete; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/ScopeAvailabilityValidator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/ScopeAvailabilityValidator.cs new file mode 100644 index 00000000..5569903c --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/ScopeAvailabilityValidator.cs @@ -0,0 +1,153 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; +using Microsoft.Extensions.Logging; + +namespace Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; + +/// +/// Pre-flight validator that filters a permission spec list against what each resource +/// service principal actually exposes in the tenant. Issue #429: the unified +/// /v2.0/adminconsent endpoint rejects the entire URL with AADSTS650053 when any +/// requested scope does not exist on its resource SP — a single bad scope blocks every +/// other resource. This helper queries Graph once per resource SP and drops missing scopes +/// from the spec list before builds the URL. +/// +/// The helper is intentionally pure (no console I/O, no PowerShell, no decisions about +/// "should we fall back" — those live in the orchestrator). The result reports both the +/// effective spec list to use for the URL and the dropped scopes so the orchestrator can +/// surface warnings and offer the PowerShell fallback for users who want to stamp them +/// anyway via the programmatic oauth2PermissionGrants path (which is lenient and +/// will create the grant row even for non-existent scopes — useful when a resource SP is +/// expected to expose the scope shortly, e.g. just-registered first-party SP). +/// +/// +internal static class ScopeAvailabilityValidator +{ + /// + /// Validates each spec's Scopes against the delegated scopes the resource SP + /// actually exposes. Returns the filtered specs and a per-resource breakdown of what + /// was dropped. Specs whose SP cannot be resolved are passed through unchanged — the + /// caller already handles missing SPs in + /// Phase 1 and not having a resolved SP id is not the same as "the SP exposes nothing." + /// + /// Graph API service used to query each resource SP's published scopes. + /// Tenant ID for the Graph calls. + /// Input specs to validate. + /// + /// Map from resource appId to resolved SP object id (Phase 1 output). Specs whose appId + /// is missing from this map are passed through unchanged. + /// + /// Logger for debug-level breadcrumbs only — the caller is responsible for user-facing warnings. + /// Cancellation token. + public static async Task ValidateAsync( + GraphApiService graph, + string tenantId, + IReadOnlyList specs, + IReadOnlyDictionary resourceSpObjectIds, + ILogger logger, + CancellationToken ct) + { + ArgumentNullException.ThrowIfNull(graph); + ArgumentNullException.ThrowIfNull(specs); + ArgumentNullException.ThrowIfNull(resourceSpObjectIds); + ArgumentNullException.ThrowIfNull(logger); + + var effective = new List(specs.Count); + var dropped = new List(); + + foreach (var spec in specs) + { + // No SP id means Phase 1 could not resolve it. Pass through — keeping the + // pre-existing behavior where a missing SP is logged in Phase 1 and the rest + // of the orchestrator decides what to do. Validating against an empty set + // would silently drop every scope on the resource, which is far worse than + // surfacing AADSTS650053 if it ever gets that far. + if (!resourceSpObjectIds.TryGetValue(spec.ResourceAppId, out var spObjectId) || + string.IsNullOrWhiteSpace(spObjectId)) + { + effective.Add(spec); + continue; + } + + // Nothing to filter — keep as-is. + if (spec.Scopes is not { Length: > 0 }) + { + effective.Add(spec); + continue; + } + + // GetAvailableScopeNamesAsync can throw (transient Graph failure, disposed + // JsonDocument in stubbed tests, network error). Treat any exception the same + // way as "returned no scopes" — pass through unchanged. Validation is a safety + // net; we'd rather miss a filter opportunity than block setup on a side-quest + // error inside the validator. + HashSet available; + try + { + available = await graph.GetAvailableScopeNamesAsync(tenantId, spObjectId, ct); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + catch (Exception ex) + { + logger.LogDebug(ex, + "Could not query published scopes for '{ResourceName}' ({AppId}); passing spec through unchanged.", + spec.ResourceName, spec.ResourceAppId); + effective.Add(spec); + continue; + } + + if (available.Count == 0) + { + // Graph call returned no scopes. Could be the SP genuinely exposes none, + // or the call failed silently (GetAvailableScopeNamesAsync swallows errors + // and returns an empty set). Pass through unchanged — dropping every scope + // here would block setup the same way AADSTS650053 does, just from a + // different cause; the orchestrator's existing handling is preferable. + logger.LogDebug( + "Resource '{ResourceName}' ({AppId}) returned no published delegated scopes — passing spec through unchanged.", + spec.ResourceName, spec.ResourceAppId); + effective.Add(spec); + continue; + } + + var validScopes = new List(spec.Scopes.Length); + foreach (var scope in spec.Scopes) + { + if (available.Contains(scope)) + validScopes.Add(scope); + else + dropped.Add(new DroppedScope(spec.ResourceName, spec.ResourceAppId, scope)); + } + + effective.Add(spec with { Scopes = validScopes.ToArray() }); + } + + return new ValidationResult(effective, dropped); + } + + /// + /// Outcome of . + /// + /// + /// Spec list with unavailable scopes removed. Caller passes this to the consent URL + /// builder. A spec with all scopes dropped is preserved with an empty Scopes + /// array; the URL builder already filters those out. + /// + /// + /// One entry per (resource, scope) pair that was filtered out. Used by the caller to + /// emit user-facing warnings and decide whether to offer the PowerShell fallback. + /// + public sealed record ValidationResult( + IReadOnlyList EffectiveSpecs, + IReadOnlyList DroppedScopes); + + /// + /// A single scope that was filtered out because the resource SP does not expose it. + /// + public sealed record DroppedScope(string ResourceName, string ResourceAppId, string Scope); +} diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AzRestConsentRunnerTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AzRestConsentRunnerTests.cs new file mode 100644 index 00000000..51d7ba68 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AzRestConsentRunnerTests.cs @@ -0,0 +1,267 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; +using Microsoft.Agents.A365.DevTools.Cli.Constants; +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; + +/// +/// Tests for — the az rest-based replacement for +/// the deprecated PowerShellConsentRunner. Mocks so we +/// can assert the exact az invocations (URL, method, headers) and verify idempotency +/// (existing AllPrincipals grant with the requested scopes already merged → no PATCH/POST). +/// +public class AzRestConsentRunnerTests +{ + private const string BlueprintSpId = "11111111-1111-1111-1111-111111111111"; + private const string ResourceSpId = "22222222-2222-2222-2222-222222222222"; + private const string ExistingGrantId = "33333333-3333-3333-3333-333333333333"; + private const string ObsAppId = ConfigConstants.ObservabilityApiAppId; + + private readonly CommandExecutor _executor = Substitute.For(Substitute.For>()); + private readonly ILogger _logger = NullLogger.Instance; + + [Fact] + public async Task InvalidBlueprintSpId_ReturnsNotAttempted() + { + var (attempted, succeeded) = await AzRestConsentRunner.TryRunAsync( + _executor, + blueprintSpObjectId: "not-a-guid", + specs: new[] { ObsSpec() }, + _logger, + ct: default); + + attempted.Should().BeFalse(because: "the GUID guard must reject invalid input before any az invocation"); + succeeded.Should().BeFalse(); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task NoDelegatedSpecs_ReturnsNotAttempted() + { + // Spec carries only S2S roles; the consent runner has no work to do. + var specs = new[] + { + new ResourcePermissionSpec(ObsAppId, "Observability API", + Scopes: Array.Empty(), + SetInheritable: false, + AppRoleScopes: new[] { "Agent365.Observability.OtelWrite" }) + }; + + var (attempted, succeeded) = await AzRestConsentRunner.TryRunAsync( + _executor, BlueprintSpId, specs, _logger, ct: default); + + attempted.Should().BeFalse(because: "no delegated specs means there's nothing for the consent runner to grant"); + succeeded.Should().BeFalse(); + } + + [Fact] + public async Task UnsafeScopeValue_ReturnsNotAttempted() + { + var specs = new[] + { + new ResourcePermissionSpec(ObsAppId, "Observability API", + Scopes: new[] { "Agent365.Observability.OtelWrite'; DROP TABLE --" }, + SetInheritable: false) + }; + + var (attempted, succeeded) = await AzRestConsentRunner.TryRunAsync( + _executor, BlueprintSpId, specs, _logger, ct: default); + + attempted.Should().BeFalse(because: "scope values are interpolated into the OData filter and request body; the allowlist must reject anything outside [A-Za-z0-9._-]"); + succeeded.Should().BeFalse(); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ResourceSpNotFoundInTenant_ReturnsAttemptedAndFailed() + { + // GET resource SP → empty value array. No write should be attempted. + StubResourceSpLookup(returnsEmpty: true); + + var (attempted, succeeded) = await AzRestConsentRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeFalse(because: "no resource SP means we cannot anchor the oauth2PermissionGrant; the operator must provision the SP first"); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method POST") || s.Contains("--method PATCH")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ExistingGrantAlreadyHasRequestedScopes_NoWriteIssued() + { + // GET resource SP → found. GET existing grant → already has the requested scope. + // Runner must not PATCH (idempotent) and must report success. + StubResourceSpLookup(returnsEmpty: false); + StubExistingGrantLookup(existingScope: ConfigConstants.ObservabilityApiOtelWriteScope); + + var (attempted, succeeded) = await AzRestConsentRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeTrue(because: "existing grant already covers the requested scope set — re-issuing the PATCH would be a no-op and waste a round-trip"); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method POST") || s.Contains("--method PATCH")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ExistingGrantWithSubsetOfScopes_PatchedWithMergedScopeSet() + { + // Existing grant covers a different scope. The runner must PATCH with the union. + StubResourceSpLookup(returnsEmpty: false); + StubExistingGrantLookup(existingScope: "SomeOtherScope"); + + // PATCH succeeds. + _executor + .ExecuteAsync("az", Arg.Is(s => s.Contains("--method PATCH") && s.Contains($"oauth2PermissionGrants/{ExistingGrantId}")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 0 })); + + var (attempted, succeeded) = await AzRestConsentRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeTrue(); + // PATCH on the existing grant id, not a fresh POST. + await _executor.Received().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method PATCH") && s.Contains($"oauth2PermissionGrants/{ExistingGrantId}")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method POST")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task NoExistingGrant_PostedWithNewGrantBody() + { + // GET existing grant returns an empty value array → fresh POST. + StubResourceSpLookup(returnsEmpty: false); + StubExistingGrantLookup(empty: true); + + _executor + .ExecuteAsync("az", Arg.Is(s => s.Contains("--method POST") && s.Contains("/oauth2PermissionGrants")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 0 })); + + var (attempted, succeeded) = await AzRestConsentRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeTrue(); + await _executor.Received().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method POST") && s.Contains("oauth2PermissionGrants") && !s.Contains($"/{ExistingGrantId}")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method PATCH")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task AzPostExitsNonZero_OverallFails() + { + // POST returns exit 1 with stderr. Runner reports failure and the caller's existing + // Action Required path surfaces the recovery URL. + StubResourceSpLookup(returnsEmpty: false); + StubExistingGrantLookup(empty: true); + + _executor + .ExecuteAsync("az", Arg.Is(s => s.Contains("--method POST")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 1, StandardError = "Insufficient privileges to complete the operation." })); + + var (attempted, succeeded) = await AzRestConsentRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeFalse(because: "az exited non-zero — the grant was not created, so we must surface a failure so the orchestrator's Action Required path takes over"); + } + + [Fact] + public void TryExtractFirstId_ValidOdataValueArray_ReturnsFirstId() + { + var json = "{\"value\":[{\"id\":\"" + ResourceSpId + "\"},{\"id\":\"another\"}]}"; + AzRestConsentRunner.TryExtractFirstId(json).Should().Be(ResourceSpId, + because: "the runner only needs the first match; az's $filter=appId eq '...' query is uniquely keyed"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("not json")] + [InlineData("{\"value\":[]}")] + [InlineData("{\"unrelated\":true}")] + public void TryExtractFirstId_InvalidOrEmpty_ReturnsNull(string? azOutput) + { + AzRestConsentRunner.TryExtractFirstId(azOutput).Should().BeNull( + because: "missing data must yield null so the caller takes the warning path instead of dereferencing"); + } + + [Fact] + public void TryExtractFirstGrantIdAndScope_ParsesIdAndScope() + { + var json = "{\"value\":[{\"id\":\"" + ExistingGrantId + "\",\"scope\":\"A B C\"}]}"; + var (id, scope) = AzRestConsentRunner.TryExtractFirstGrantIdAndScope(json); + id.Should().Be(ExistingGrantId); + scope.Should().Be("A B C"); + } + + [Fact] + public void TryExtractFirstGrantIdAndScope_EmptyArray_ReturnsBothNull() + { + var (id, scope) = AzRestConsentRunner.TryExtractFirstGrantIdAndScope("{\"value\":[]}"); + id.Should().BeNull(); + scope.Should().BeNull(); + } + + // ───────────────────────────────────────────────────────────────────────── + // Test scaffolding + // ───────────────────────────────────────────────────────────────────────── + + private static ResourcePermissionSpec ObsSpec() => + new(ObsAppId, + "Observability API", + new[] { ConfigConstants.ObservabilityApiOtelWriteScope }, + SetInheritable: false); + + private void StubResourceSpLookup(bool returnsEmpty) + { + var json = returnsEmpty + ? "{\"value\":[]}" + : $"{{\"value\":[{{\"id\":\"{ResourceSpId}\"}}]}}"; + _executor + .ExecuteAsync("az", + Arg.Is(s => s.Contains("/servicePrincipals?") && s.Contains($"appId eq '{ObsAppId}'")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = json })); + } + + private void StubExistingGrantLookup(string? existingScope = null, bool empty = false) + { + string json; + if (empty) + { + json = "{\"value\":[]}"; + } + else + { + json = "{\"value\":[{\"id\":\"" + ExistingGrantId + "\",\"scope\":\"" + (existingScope ?? string.Empty) + "\"}]}"; + } + _executor + .ExecuteAsync("az", + Arg.Is(s => s.Contains("oauth2PermissionGrants?") && s.Contains($"clientId eq '{BlueprintSpId}'")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = json })); + } +} diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AzRestS2SRunnerTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AzRestS2SRunnerTests.cs new file mode 100644 index 00000000..468a1f93 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AzRestS2SRunnerTests.cs @@ -0,0 +1,352 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; +using Microsoft.Agents.A365.DevTools.Cli.Constants; +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; + +/// +/// Tests for — the az rest-based replacement for +/// PowerShellS2SRunner. Mocks so we can assert the +/// exact az invocations and verify idempotency (existing (resourceId, appRoleId) +/// assignment on the blueprint SP → no fresh POST). +/// +public class AzRestS2SRunnerTests +{ + private const string BlueprintSpId = "11111111-1111-1111-1111-111111111111"; + private const string ResourceSpId = "22222222-2222-2222-2222-222222222222"; + private const string OtelWriteRoleId = "44444444-4444-4444-4444-444444444444"; + private const string ObsAppId = ConfigConstants.ObservabilityApiAppId; + private const string OtelWriteRole = ConfigConstants.ObservabilityApiOtelWriteScope; + + private readonly CommandExecutor _executor = Substitute.For(Substitute.For>()); + private readonly ILogger _logger = NullLogger.Instance; + + [Fact] + public async Task InvalidBlueprintSpId_ReturnsNotAttempted() + { + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, + blueprintSpObjectId: "not-a-guid", + specs: new[] { ObsSpec() }, + _logger, + ct: default); + + attempted.Should().BeFalse(because: "the GUID guard must reject invalid input before any az invocation"); + succeeded.Should().BeFalse(); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task NoS2SSpecs_ReturnsNotAttempted() + { + // Spec carries only delegated scopes; the S2S runner has no work to do. + var specs = new[] + { + new ResourcePermissionSpec(ObsAppId, "Observability API", + Scopes: new[] { OtelWriteRole }, + SetInheritable: false, + AppRoleScopes: null) + }; + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, specs, _logger, ct: default); + + attempted.Should().BeFalse(because: "no specs with AppRoleScopes means there's nothing for the S2S runner to assign"); + succeeded.Should().BeFalse(); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task UnsafeRoleValue_ReturnsNotAttempted() + { + var specs = new[] + { + new ResourcePermissionSpec(ObsAppId, "Observability API", + Scopes: Array.Empty(), + SetInheritable: false, + AppRoleScopes: new[] { "Agent365.Observability.OtelWrite'; DROP TABLE --" }) + }; + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, specs, _logger, ct: default); + + attempted.Should().BeFalse(because: "role values are interpolated into the resource SP lookup filter and the request body; the allowlist must reject anything outside [A-Za-z0-9._-]"); + succeeded.Should().BeFalse(); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task InvalidResourceAppId_ReturnsNotAttempted() + { + var specs = new[] + { + new ResourcePermissionSpec(ResourceAppId: "not-a-guid", + "Observability API", + Scopes: Array.Empty(), + SetInheritable: false, + AppRoleScopes: new[] { OtelWriteRole }) + }; + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, specs, _logger, ct: default); + + attempted.Should().BeFalse(because: "ResourceAppId reaches the OData $filter and must pass the GUID allowlist before any az invocation"); + succeeded.Should().BeFalse(); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ExistingAssignmentsGetFails_ReturnsAttemptedAndFailed() + { + // The very first GET (existing assignments on the blueprint SP) errors out. + // We cannot reason about idempotency without it, so the runner must short-circuit + // with Attempted=true so the orchestrator's Action Required path surfaces the failure. + StubExistingAssignmentsGet(failure: true); + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeFalse(because: "the initial GET drives every per-role idempotency decision; if it fails we cannot safely proceed"); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method POST")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ResourceSpNotFoundInTenant_ReturnsAttemptedAndFailed() + { + // GET existing assignments → empty (fine). + // GET resource SP → empty value array. + StubExistingAssignmentsGet(emptyValue: true); + StubResourceSpWithAppRolesLookup(returnsEmpty: true); + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeFalse(because: "no resource SP means we cannot resolve the appRoleId, and we must not attempt to POST against a non-existent resource"); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method POST")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ExistingAssignmentAlreadyPresent_NoPostIssued() + { + // The blueprint SP already has (ResourceSpId, OtelWriteRoleId) assigned. + // Runner must skip the POST and report success. + StubExistingAssignmentsGet(existingResourceId: ResourceSpId, existingAppRoleId: OtelWriteRoleId); + StubResourceSpWithAppRolesLookup(returnsEmpty: false); + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeTrue(because: "the requested role is already assigned — re-issuing the POST would return 4xx and waste a round-trip"); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method POST")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task NoExistingAssignment_PostedWithRoleBody() + { + // Blueprint SP has no existing assignments. Runner POSTs the new appRoleAssignment. + StubExistingAssignmentsGet(emptyValue: true); + StubResourceSpWithAppRolesLookup(returnsEmpty: false); + + _executor + .ExecuteAsync("az", Arg.Is(s => s.Contains("--method POST") && s.Contains($"/servicePrincipals/{BlueprintSpId}/appRoleAssignments")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 0 })); + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeTrue(); + await _executor.Received().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method POST") && s.Contains($"/servicePrincipals/{BlueprintSpId}/appRoleAssignments")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task RoleNotPublishedOnResource_ReturnsFailureForThatSpec() + { + // Resource SP exists but does not publish the requested role value. + StubExistingAssignmentsGet(emptyValue: true); + StubResourceSpWithAppRolesLookup(returnsEmpty: false, publishRole: false); + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeFalse(because: "if the resource SP doesn't publish the requested role we cannot assign it; this is the operator's misconfiguration, not a runner failure mode we can recover from"); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(s => s.Contains("--method POST")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task AzPostExitsNonZero_OverallFails() + { + // POST returns exit 1. Runner reports failure so the orchestrator's Action Required + // path surfaces the recovery URL. + StubExistingAssignmentsGet(emptyValue: true); + StubResourceSpWithAppRolesLookup(returnsEmpty: false); + + _executor + .ExecuteAsync("az", Arg.Is(s => s.Contains("--method POST")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 1, StandardError = "Insufficient privileges to complete the operation." })); + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, new[] { ObsSpec() }, _logger, ct: default); + + attempted.Should().BeTrue(); + succeeded.Should().BeFalse(because: "az exited non-zero — the assignment was not created, so we must surface a failure so the orchestrator's Action Required path takes over"); + } + + [Fact] + public void TryExtractFirstSpIdAndAppRoles_ValidPayload_ReturnsIdAndRoleMap() + { + var json = + "{\"value\":[{\"id\":\"" + ResourceSpId + "\",\"appRoles\":[" + + "{\"id\":\"" + OtelWriteRoleId + "\",\"value\":\"" + OtelWriteRole + "\"}," + + "{\"id\":\"55555555-5555-5555-5555-555555555555\",\"value\":\"SomeOther.Role\"}" + + "]}]}"; + + var (spId, roles) = AzRestS2SRunner.TryExtractFirstSpIdAndAppRoles(json); + spId.Should().Be(ResourceSpId); + roles.Should().ContainKey(OtelWriteRole).WhoseValue.Should().Be(OtelWriteRoleId); + roles.Should().ContainKey("SomeOther.Role"); + } + + [Fact] + public void TryExtractFirstSpIdAndAppRoles_RoleLookupIsCaseInsensitive() + { + // App role values are spelled with mixed case in Entra ("Agent365.Observability.OtelWrite"), + // but our spec strings should resolve regardless of case differences from the SP record. + var json = + "{\"value\":[{\"id\":\"" + ResourceSpId + "\",\"appRoles\":[" + + "{\"id\":\"" + OtelWriteRoleId + "\",\"value\":\"agent365.observability.otelwrite\"}" + + "]}]}"; + + var (_, roles) = AzRestS2SRunner.TryExtractFirstSpIdAndAppRoles(json); + roles.TryGetValue(OtelWriteRole, out var id).Should().BeTrue( + because: "Entra is case-insensitive for app role values; a case mismatch with the constant should not stop the assignment"); + id.Should().Be(OtelWriteRoleId); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("not json")] + [InlineData("{\"value\":[]}")] + [InlineData("{\"unrelated\":true}")] + public void TryExtractFirstSpIdAndAppRoles_InvalidOrEmpty_ReturnsNullId(string? azOutput) + { + var (spId, roles) = AzRestS2SRunner.TryExtractFirstSpIdAndAppRoles(azOutput); + spId.Should().BeNull( + because: "missing data must yield a null SP id so the caller takes the warning path instead of dereferencing"); + roles.Should().BeEmpty(); + } + + [Fact] + public void TryExtractFirstSpIdAndAppRoles_SpWithNoAppRoles_ReturnsIdAndEmptyMap() + { + // A resource SP that exists but has not published any app roles. SP id is still returned + // so the caller can log a precise "role not published" message rather than a generic miss. + var json = "{\"value\":[{\"id\":\"" + ResourceSpId + "\",\"appRoles\":[]}]}"; + var (spId, roles) = AzRestS2SRunner.TryExtractFirstSpIdAndAppRoles(json); + spId.Should().Be(ResourceSpId); + roles.Should().BeEmpty(); + } + + // ───────────────────────────────────────────────────────────────────────── + // Test scaffolding + // ───────────────────────────────────────────────────────────────────────── + + private static ResourcePermissionSpec ObsSpec() => + new(ObsAppId, + "Observability API", + Scopes: Array.Empty(), + SetInheritable: false, + AppRoleScopes: new[] { OtelWriteRole }); + + /// + /// Stubs the very first GET — /servicePrincipals/{blueprintSpId}/appRoleAssignments. + /// Pass =true to simulate a non-zero exit; pass + /// =true for an empty value array; otherwise the response + /// contains exactly one assignment with and + /// . + /// + private void StubExistingAssignmentsGet( + bool failure = false, + bool emptyValue = false, + string? existingResourceId = null, + string? existingAppRoleId = null) + { + CommandResult result; + if (failure) + { + result = new CommandResult { ExitCode = 1, StandardError = "Forbidden" }; + } + else if (emptyValue) + { + result = new CommandResult { ExitCode = 0, StandardOutput = "{\"value\":[]}" }; + } + else + { + var json = "{\"value\":[{\"resourceId\":\"" + existingResourceId + "\",\"appRoleId\":\"" + existingAppRoleId + "\"}]}"; + result = new CommandResult { ExitCode = 0, StandardOutput = json }; + } + + _executor + .ExecuteAsync("az", + Arg.Is(s => s.Contains($"/servicePrincipals/{BlueprintSpId}/appRoleAssignments") && s.Contains("--method GET")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(result)); + } + + /// + /// Stubs the per-spec resource SP lookup — /servicePrincipals?$filter=appId eq '...'&$select=id,appRoles. + /// When is true (default) the response includes the OtelWrite role; + /// false simulates a misconfigured resource that hasn't published the requested role. + /// + private void StubResourceSpWithAppRolesLookup(bool returnsEmpty, bool publishRole = true) + { + string json; + if (returnsEmpty) + { + json = "{\"value\":[]}"; + } + else if (publishRole) + { + json = "{\"value\":[{\"id\":\"" + ResourceSpId + "\",\"appRoles\":[{\"id\":\"" + OtelWriteRoleId + "\",\"value\":\"" + OtelWriteRole + "\"}]}]}"; + } + else + { + json = "{\"value\":[{\"id\":\"" + ResourceSpId + "\",\"appRoles\":[]}]}"; + } + + _executor + .ExecuteAsync("az", + Arg.Is(s => s.Contains("/servicePrincipals?") && s.Contains($"appId eq '{ObsAppId}'") && s.Contains("$select=id,appRoles")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = json })); + } +} diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorMissingSpTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorMissingSpTests.cs new file mode 100644 index 00000000..62531bba --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorMissingSpTests.cs @@ -0,0 +1,473 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; + +/// +/// Tests for and +/// the two URL/command builders the helper depends on. +/// +/// +/// Issue #429 history: the first attempt used per-app /v2.0/adminconsent with +/// {appId}/.default — fails with AADSTS65003 ("first party token-to-self") on the +/// MCP audiences. The helper now shells out to az ad sp create --id {appId} using +/// the operator's GA-privileged az login, parses the returned JSON for the SP id, and +/// trusts az when an id is present (no Graph re-poll). When the operator declines, az +/// fails, the GUID guard rejects, or --skip-sp-provisioning is set, the helper +/// records a on so the setup +/// summary's Action Required block surfaces both the az command AND the per-SP +/// blueprint-as-client consent URL — together they are a complete recovery without +/// re-running a365 setup all. +/// +/// +/// +/// Tests that exercise the helper set +/// to false via a try/finally so they do not leak state into other tests. The default +/// is false so the helper runs in production; tests for the broader +/// ConfigureAllPermissionsAsync flow flip it to true in their setup. +/// +/// +[Collection("Sequential")] +public class BatchPermissionsOrchestratorMissingSpTests +{ + private const string TenantId = "11111111-1111-1111-1111-111111111111"; + private const string BlueprintAppId = "22222222-2222-2222-2222-222222222222"; + private const string MailMcpAppId = "16b1878d-62c7-4009-aa25-68989d63bbad"; + private const string MailMcpSpObjectId = "96f7de40-d3bb-49e1-8358-37909ebb5bab"; + private const string TeamsMcpAppId = "ce5029ee-c1d3-45c0-bdcc-efb5a4245687"; + + private readonly GraphApiService _graph = Substitute.For(); + private readonly ILogger _logger = NullLogger.Instance; + private readonly CommandExecutor _executor = Substitute.For(Substitute.For>()); + + [Fact] + public async Task EmptyMissingSpecs_NoOpAndNoGraphOrExecutorCalls() + { + using var bypass = TemporarilyDisableSpProvisioningBypass(); + var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + + await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( + _graph, TenantId, BlueprintAppId, + missingSpecs: Array.Empty(), + resolvedSpAppIds: resolvedSpAppIds, + permScopes: Array.Empty(), + skipSpProvisioning: false, + _logger, + setupResults: null, + ct: CancellationToken.None, + commandExecutor: _executor); + + resolvedSpAppIds.Should().BeEmpty(); + await _graph.DidNotReceive().LookupServicePrincipalByAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>()); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task PreflightFindsSp_AddsToResolvedSetAndDoesNotRunAz() + { + using var bypass = TemporarilyDisableSpProvisioningBypass(); + + _graph + .LookupServicePrincipalByAppIdAsync(TenantId, MailMcpAppId, Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(MailMcpSpObjectId)); + + var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var setupResults = new SetupResults(); + var missing = new[] + { + new ResourcePermissionSpec(MailMcpAppId, "Work IQ Mail MCP", new[] { "Tools.ListInvoke.All" }, SetInheritable: true) + }; + + await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( + _graph, TenantId, BlueprintAppId, missing, resolvedSpAppIds, + permScopes: Array.Empty(), + skipSpProvisioning: false, + _logger, + setupResults: setupResults, + ct: CancellationToken.None, + commandExecutor: _executor); + + resolvedSpAppIds.Should().Contain(MailMcpAppId, + because: "the pre-flight Graph lookup found the SP — the operator must have consented to it between Phase 1 and now, so the helper records it and skips the az shell-out"); + setupResults.MissingSpActions.Should().BeEmpty( + because: "the resource was successfully resolved without any operator intervention — no Action Required entry needed"); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task SkipSpProvisioning_True_RecordsMissingSpActionAndDoesNotRunAz() + { + using var bypass = TemporarilyDisableSpProvisioningBypass(); + + _graph + .LookupServicePrincipalByAppIdAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(null)); + + var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var setupResults = new SetupResults(); + var missing = new[] + { + new ResourcePermissionSpec(TeamsMcpAppId, "Work IQ Teams MCP", new[] { "Tools.ListInvoke.All" }, SetInheritable: true) + }; + + await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( + _graph, TenantId, BlueprintAppId, missing, resolvedSpAppIds, + permScopes: Array.Empty(), + skipSpProvisioning: true, + _logger, + setupResults: setupResults, + ct: CancellationToken.None, + commandExecutor: _executor); + + resolvedSpAppIds.Should().BeEmpty( + because: "with --skip-sp-provisioning set the helper must not provision anything in-line"); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + setupResults.MissingSpActions.Should().ContainSingle(a => a.ResourceAppId == TeamsMcpAppId, + because: "the operator needs the recovery steps in the Action Required block — moving from Warnings to MissingSpActions is the whole point of the rework"); + var entry = setupResults.MissingSpActions.Single(); + entry.AzCreateCommand.Should().Be($"az ad sp create --id {TeamsMcpAppId}", + because: "the recovery's step 1 is the same az command the helper would have run interactively — single source of truth for the format"); + entry.PerSpConsentUrl.Should().Contain($"client_id={BlueprintAppId}", + because: "step 2 grants the BLUEPRINT consent for the resource scope — using the resource as client would hit AADSTS65003 'first party token-to-self' for these MCP apps"); + entry.PerSpConsentUrl.Should().Contain(Uri.EscapeDataString($"{TeamsMcpAppId}/Tools.ListInvoke.All"), + because: "the scope param targets the resource SP that step 1 just created"); + setupResults.Warnings.Should().BeEmpty( + because: "the rework moved missing-SP messaging out of the noisy main-output Warnings block and into the focused Action Required block at the end"); + } + + [Fact] + public async Task NullCommandExecutor_FallsBackToWarningPathAndDoesNotPrompt() + { + // Without an executor the helper has no way to shell out to az; it must behave the + // same as --skip-sp-provisioning: record the Action Required entry, no prompts, + // no provisioning. + using var bypass = TemporarilyDisableSpProvisioningBypass(); + + _graph + .LookupServicePrincipalByAppIdAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(null)); + + var confirmer = Substitute.For(); + var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var setupResults = new SetupResults(); + var missing = new[] + { + new ResourcePermissionSpec(TeamsMcpAppId, "Work IQ Teams MCP", new[] { "Tools.ListInvoke.All" }, SetInheritable: true) + }; + + await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( + _graph, TenantId, BlueprintAppId, missing, resolvedSpAppIds, + permScopes: Array.Empty(), + skipSpProvisioning: false, + _logger, + setupResults: setupResults, + ct: CancellationToken.None, + commandExecutor: null, + confirmationProvider: confirmer); + + resolvedSpAppIds.Should().BeEmpty(); + setupResults.MissingSpActions.Should().ContainSingle(a => a.ResourceAppId == TeamsMcpAppId); + // No prompt fires — there is no executor to provision anyway, so a confirmation + // would be misleading. (NSubstitute does not accept a 'because' on Received().) + await confirmer.DidNotReceive().ConfirmAsync(Arg.Any()); + } + + [Fact] + public async Task ConfirmationProviderReturnsFalse_RecordsActionAndDoesNotRunAz() + { + // Interactive path with a confirmation provider that declines the per-SP prompt. + // The helper must respect that: no az shell-out, MissingSpActions populated, appId + // NOT added to resolvedSpAppIds. Mirrors what an operator typing 'n' would produce. + using var bypass = TemporarilyDisableSpProvisioningBypass(); + + _graph + .LookupServicePrincipalByAppIdAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(null)); + + var declining = Substitute.For(); + declining.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(false)); + + var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var setupResults = new SetupResults(); + var missing = new[] + { + new ResourcePermissionSpec(TeamsMcpAppId, "Work IQ Teams MCP", new[] { "Tools.ListInvoke.All" }, SetInheritable: true) + }; + + await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( + _graph, TenantId, BlueprintAppId, missing, resolvedSpAppIds, + permScopes: Array.Empty(), + skipSpProvisioning: false, + _logger, + setupResults: setupResults, + ct: CancellationToken.None, + commandExecutor: _executor, + confirmationProvider: declining); + + resolvedSpAppIds.Should().BeEmpty( + because: "the operator declined — the helper must not stamp the appId into the resolved set without provisioning evidence"); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + setupResults.MissingSpActions.Should().ContainSingle(a => a.ResourceAppId == TeamsMcpAppId, + because: "declining still leaves the operator with an unresolved resource — the Action Required block must list the manual recovery steps"); + await declining.Received().ConfirmAsync(Arg.Is(s => s.Contains("Provision via 'az ad sp create'?") && s.Contains("[y/N]"))); + } + + [Fact] + public async Task ConfirmationProviderReturnsTrue_AzExitsZeroWithSpJson_AddsAppIdToResolvedSet() + { + // The happy path: operator accepts the prompt, az exits 0 with the SP JSON in + // stdout. The helper parses the id directly from az output (no Graph re-poll) and + // adds the appId to resolvedSpAppIds. MissingSpActions stays empty for this spec. + using var bypass = TemporarilyDisableSpProvisioningBypass(); + + _graph + .LookupServicePrincipalByAppIdAsync(TenantId, TeamsMcpAppId, Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(null)); + + // az returns the real shape we saw from the operator's manual run: a JSON object + // with "id" set to the new SP's object id. Anything else (oauth2PermissionScopes, + // servicePrincipalNames, etc.) is irrelevant to the helper's success check. + _executor + .ExecuteAsync("az", Arg.Is(s => s.Contains("ad sp create") && s.Contains(TeamsMcpAppId)), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult + { + ExitCode = 0, + StandardOutput = "{\"id\":\"d42a47bf-9727-444c-ae57-17bd588613cd\",\"appId\":\"" + TeamsMcpAppId + "\"}" + })); + + var accepting = Substitute.For(); + accepting.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(true)); + + var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var setupResults = new SetupResults(); + var missing = new[] + { + new ResourcePermissionSpec(TeamsMcpAppId, "Work IQ Teams MCP", new[] { "Tools.ListInvoke.All" }, SetInheritable: true) + }; + + await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( + _graph, TenantId, BlueprintAppId, missing, resolvedSpAppIds, + permScopes: Array.Empty(), + skipSpProvisioning: false, + _logger, + setupResults: setupResults, + ct: CancellationToken.None, + commandExecutor: _executor, + confirmationProvider: accepting); + + resolvedSpAppIds.Should().Contain(TeamsMcpAppId, + because: "az returned the SP JSON with an id — that is authoritative evidence the SP exists, so the caller's URL build must include this resource"); + setupResults.MissingSpActions.Should().BeEmpty( + because: "the SP was provisioned successfully — no recovery steps belong in Action Required for this resource"); + // The helper trusts az output and does NOT issue a follow-up Graph lookup for the + // newly created SP. Only the pre-flight lookup should have fired. + await _graph.Received(1).LookupServicePrincipalByAppIdAsync( + TenantId, TeamsMcpAppId, Arg.Any(), Arg.Any?>()); + } + + [Fact] + public async Task AzCommandFailsWithNonZeroExit_RecordsActionAndDoesNotAddAppIdToResolvedSet() + { + using var bypass = TemporarilyDisableSpProvisioningBypass(); + + _graph + .LookupServicePrincipalByAppIdAsync(TenantId, TeamsMcpAppId, Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(null)); + + _executor + .ExecuteAsync("az", Arg.Is(s => s.Contains("ad sp create")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 1, StandardError = "Insufficient privileges to complete the operation." })); + + var accepting = Substitute.For(); + accepting.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(true)); + + var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var setupResults = new SetupResults(); + var missing = new[] + { + new ResourcePermissionSpec(TeamsMcpAppId, "Work IQ Teams MCP", new[] { "Tools.ListInvoke.All" }, SetInheritable: true) + }; + + await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( + _graph, TenantId, BlueprintAppId, missing, resolvedSpAppIds, + permScopes: Array.Empty(), + skipSpProvisioning: false, + _logger, + setupResults: setupResults, + ct: CancellationToken.None, + commandExecutor: _executor, + confirmationProvider: accepting); + + resolvedSpAppIds.Should().BeEmpty( + because: "az failed — there is no SP to record; including the appId would poison the unified URL with the same AADSTS650052 we are trying to avoid"); + setupResults.MissingSpActions.Should().ContainSingle(a => a.ResourceAppId == TeamsMcpAppId, + because: "operator needs the recovery steps in the Action Required block so they can run the az command manually (potentially after fixing whatever blocked it: bad az login, tenant policy, etc.)"); + } + + [Fact] + public async Task NonGuidResourceAppId_SkippedWithMissingSpActionAndAzNotInvoked() + { + // Safety guard: a custom permission with a malformed appId reaching this helper must + // not be interpolated into a shell command. Guard rejects it, records the recovery + // entry, and continues with remaining specs. + using var bypass = TemporarilyDisableSpProvisioningBypass(); + + _graph + .LookupServicePrincipalByAppIdAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(null)); + + var accepting = Substitute.For(); + accepting.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(true)); + + var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var setupResults = new SetupResults(); + var missing = new[] + { + new ResourcePermissionSpec("not-a-guid; rm -rf /", "Malicious", new[] { "x" }, SetInheritable: true) + }; + + await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( + _graph, TenantId, BlueprintAppId, missing, resolvedSpAppIds, + permScopes: Array.Empty(), + skipSpProvisioning: false, + _logger, + setupResults: setupResults, + ct: CancellationToken.None, + commandExecutor: _executor, + confirmationProvider: accepting); + + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + // Guard must reject the spec BEFORE prompting; otherwise the operator could approve + // a shell injection by mistake. (NSubstitute does not accept 'because' on Received().) + await accepting.DidNotReceive().ConfirmAsync(Arg.Any()); + setupResults.MissingSpActions.Should().ContainSingle(a => a.ResourceName == "Malicious", + because: "the malformed spec is still missing — Action Required must list the recovery steps so the operator can fix the manifest or custom permission entry"); + } + + [Fact] + public void BuildAzAdSpCreateCommand_ProducesExpectedShape() + { + var cmd = BatchPermissionsOrchestrator.BuildAzAdSpCreateCommand(TeamsMcpAppId); + + cmd.Should().Be($"az ad sp create --id {TeamsMcpAppId}", + because: "the live 'Running: ...' log line and the Action Required step 1 both surface this exact command; one source of truth keeps them aligned"); + } + + [Fact] + public void BuildPerSpBlueprintConsentUrl_KeysClientIdOnBlueprintAndScopeOnResource() + { + // The previous "consent the MCP app to itself" pattern fails with AADSTS65003 + // (first party token-to-self). This URL has the blueprint as the CLIENT and the + // resource as the SCOPE target — a normal cross-app consent that Entra accepts. + var spec = new ResourcePermissionSpec(TeamsMcpAppId, "Work IQ Teams MCP", new[] { "Tools.ListInvoke.All" }, SetInheritable: true); + var url = BatchPermissionsOrchestrator.BuildPerSpBlueprintConsentUrl(TenantId, BlueprintAppId, spec); + + url.Should().StartWith($"https://login.microsoftonline.com/{TenantId}/v2.0/adminconsent", + because: "the per-SP recovery URL targets the v2 admin-consent endpoint scoped to the operator's tenant"); + url.Should().Contain($"client_id={BlueprintAppId}", + because: "the BLUEPRINT must be the client so this is a normal cross-app consent — using the resource as client would hit AADSTS65003 token-to-self"); + url.Should().Contain(Uri.EscapeDataString($"{TeamsMcpAppId}/Tools.ListInvoke.All"), + because: "the scope param must qualify the requested permission under the resource SP that step 1 (az ad sp create) just provisioned"); + } + + [Fact] + public void BuildPerSpBlueprintConsentUrl_DefaultIsMcpAudienceFalse_UsesApiSchemePrefix() + { + // Default catch-all (caller has not signaled MCP-ness): use the standard + // api://{appId} Application ID URI form. This matches the broader + // GetResourceIdentifierUri contract — bare GUID is only safe for V2 MCP + // per-server SPs (identifierUris=null); other unknown resources may have + // identifierUris set and need the api:// prefix. + var customAppId = "99999999-9999-9999-9999-999999999999"; + var spec = new ResourcePermissionSpec(customAppId, "Custom Resource", new[] { "Custom.Scope" }, SetInheritable: true); + + var url = BatchPermissionsOrchestrator.BuildPerSpBlueprintConsentUrl(TenantId, BlueprintAppId, spec); + + url.Should().ContainEquivalentOf($"api%3A%2F%2F{customAppId}%2FCustom.Scope", + because: "non-MCP unknown resources keep the standard api://{appId} form so the recovery URL works for any resource whose SP publishes api:// as an identifierUri"); + url.Should().NotContain($"={customAppId}%2FCustom.Scope", + because: "without the api:// prefix the URL would fail for custom resources whose SPs do not register the bare appId in servicePrincipalNames"); + } + + [Fact] + public void BuildPerSpBlueprintConsentUrl_McpAudience_UsesBareAppIdGuidNotApiScheme() + { + // When the caller signals the resource is a V2 MCP per-server audience (e.g. + // the appId is in the loaded ToolingManifest audience set), the URL must use + // the bare appId GUID — api://{appId} triggers AADSTS500011 for these SPs + // because identifierUris is null. + var spec = new ResourcePermissionSpec(TeamsMcpAppId, "Work IQ Teams MCP", new[] { "Tools.ListInvoke.All" }, SetInheritable: true); + + var url = BatchPermissionsOrchestrator.BuildPerSpBlueprintConsentUrl(TenantId, BlueprintAppId, spec, isMcpAudience: true); + + url.Should().Contain(Uri.EscapeDataString($"{TeamsMcpAppId}/Tools.ListInvoke.All"), + because: "V2 MCP per-server SPs publish only the bare appId GUID in servicePrincipalNames; api:// triggers AADSTS500011 on the recovery URL after the operator provisions the SP"); + url.Should().NotContain($"api%3A%2F%2F{TeamsMcpAppId}", + because: "the per-SP URL must not emit api://{appId} for MCP per-server audiences — that is exactly the AADSTS500011 path the operator just escaped by running az ad sp create"); + } + + [Fact] + public void TryExtractSpIdFromAzOutput_ValidJsonWithId_ReturnsId() + { + var json = "{\"id\":\"d42a47bf-9727-444c-ae57-17bd588613cd\",\"appId\":\"" + TeamsMcpAppId + "\"}"; + + var spId = BatchPermissionsOrchestrator.TryExtractSpIdFromAzOutput(json); + + spId.Should().Be("d42a47bf-9727-444c-ae57-17bd588613cd", + because: "az ad sp create returns the SP JSON in stdout; the 'id' property is the SP object id and is authoritative evidence the SP exists"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("not json at all")] + [InlineData("{\"unrelated\":\"value\"}")] + [InlineData("{\"id\": 42}")] // id present but not a string + public void TryExtractSpIdFromAzOutput_InvalidOrMissingId_ReturnsNull(string? azOutput) + { + var spId = BatchPermissionsOrchestrator.TryExtractSpIdFromAzOutput(azOutput); + + spId.Should().BeNull( + because: "the helper falls back to the warning path only when az output is unparseable or missing the id field — all of these cases must produce null so the caller does not mistakenly add the appId to the resolved set"); + } + + // ───────────────────────────────────────────────────────────────────────── + // Test scaffolding + // + // Flips BypassSpProvisioningForTests off for the duration of one test and + // restores it on Dispose. The default (false) is the production value; tests + // for the broader orchestrator flow flip it ON in their setup, so this scope + // ensures we're explicit about wanting the helper to actually run. + // ───────────────────────────────────────────────────────────────────────── + + private static IDisposable TemporarilyDisableSpProvisioningBypass() + { + var prior = BatchPermissionsOrchestrator.BypassSpProvisioningForTests; + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = false; + return new RestoreOnDispose(() => BatchPermissionsOrchestrator.BypassSpProvisioningForTests = prior); + } + + private sealed class RestoreOnDispose : IDisposable + { + private readonly Action _restore; + public RestoreOnDispose(Action restore) { _restore = restore; } + public void Dispose() => _restore(); + } +} diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorTests.cs index 979c77f7..26c0a65a 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorTests.cs @@ -21,6 +21,7 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; /// Focused on the non-fatal phase-independence contract: each phase failure /// must not prevent subsequent phases from running. /// +[Collection("Sequential")] public class BatchPermissionsOrchestratorTests : IDisposable { private readonly GraphApiService _graph; @@ -40,12 +41,18 @@ public BatchPermissionsOrchestratorTests() // Reset in Dispose so state does not leak into other test classes. BrowserHelper.OpenUrlOverrideForTests = (_, _) => { }; AdminConsentHelper.BypassConsentChecksForTests = true; + // Bypass the missing-SP provisioning helper for the broader ConfigureAllPermissionsAsync + // tests in this class so they do not need to mock 'az ad sp create' calls. Tests that + // exercise that helper directly live in BatchPermissionsOrchestratorMissingSpTests and + // flip the flag back to false themselves. + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = true; } public void Dispose() { BrowserHelper.OpenUrlOverrideForTests = null; AdminConsentHelper.BypassConsentChecksForTests = false; + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = false; GC.SuppressFinalize(this); } @@ -263,11 +270,14 @@ await _graph.DidNotReceive().CreateOrUpdateOauth2PermissionGrantWithDetailsAsync // for the Graph-only non-admin path. The admin (browser-launching) path is not unit-tested to // avoid invoking a real browser in CI. - // ---- PowerShell S2S fallback tests ---- - // Use valid GUIDs for tenantId/blueprintAppId so PowerShellS2SRunner GUID validation passes. + // ---- az rest S2S fallback tests ---- + // Use valid GUIDs for tenantId/blueprintAppId/blueprintSpObjectId so AzRestS2SRunner GUID + // validation passes. (The PowerShell fallback was replaced by an az rest path in issue #429.) private const string S2STenantId = "00000000-0000-0000-0000-000000000001"; private const string S2SBlueprintAppId = "00000000-0000-0000-0000-000000000002"; - private const string S2SBlueprintSpObjectId = "sp-object-id"; + private const string S2SBlueprintSpObjectId = "00000000-0000-0000-0000-000000000003"; + private const string S2SResourceSpId = "00000000-0000-0000-0000-000000000004"; + private const string S2SAppRoleId = "00000000-0000-0000-0000-000000000005"; private void ArrangeS2SPhase1AndAdminCheck() { @@ -314,20 +324,16 @@ private static ResourcePermissionSpec[] S2SSpec() => /// /// When the programmatic Graph API path for S2S fails (e.g. token lacks - /// AppRoleAssignment.ReadWrite.All even for a GA) and pwsh executes the - /// fallback script successfully, BlueprintS2SOutcome must be set to Granted - /// so the Action Required block is suppressed in the setup summary. + /// AppRoleAssignment.ReadWrite.All even for a GA) and the az rest fallback completes + /// every assignment, BlueprintS2SOutcome must be set to Granted so the Action Required + /// block is suppressed in the setup summary. /// [Fact] - public async Task ConfigureAllPermissions_WhenS2SFailsAndPwshSucceeds_SetsBlueprintS2SOutcomeGranted() + public async Task ConfigureAllPermissions_WhenS2SFailsAndAzRestSucceeds_SetsBlueprintS2SOutcomeGranted() { // Arrange ArrangeS2SPhase1AndAdminCheck(); - _executor.ExecuteWithStreamingAsync( - Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any?>(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()) - .Returns(new CommandResult { ExitCode = 0 }); + ArrangeAzRestS2SCalls(blueprintAlreadyAssigned: false, postExitCode: 0); var setupResults = new SetupResults(); @@ -342,25 +348,21 @@ await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( // Assert setupResults.BlueprintS2SOutcome.Should().Be(GrantOutcome.Granted, - because: "when pwsh executes the S2S script successfully the Action Required block must be suppressed"); + because: "when the az rest POST /appRoleAssignments succeeds the Action Required block must be suppressed"); } /// - /// When the programmatic path fails and pwsh exits non-zero (e.g. exit code 2 from the - /// in-script Microsoft.Graph module check, or any other script failure), BlueprintS2SOutcome - /// must remain Failed so the Action Required block still surfaces — the user needs to - /// install the modules / fix the underlying issue and re-run. + /// When the programmatic path fails and az rest's POST exits non-zero (e.g. transient + /// Graph 5xx, throttling, or an unexpected validation error), BlueprintS2SOutcome must + /// remain Failed so the Action Required block still surfaces — the user needs to retry + /// or fix the underlying issue. /// [Fact] - public async Task ConfigureAllPermissions_WhenS2SFailsAndPwshExitsNonZero_OutcomeRemainsFailed() + public async Task ConfigureAllPermissions_WhenS2SFailsAndAzRestExitsNonZero_OutcomeRemainsFailed() { // Arrange ArrangeS2SPhase1AndAdminCheck(); - _executor.ExecuteWithStreamingAsync( - Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any?>(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()) - .Returns(new CommandResult { ExitCode = 2 }); + ArrangeAzRestS2SCalls(blueprintAlreadyAssigned: false, postExitCode: 1); var setupResults = new SetupResults(); @@ -375,16 +377,59 @@ await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( // Assert setupResults.BlueprintS2SOutcome.Should().Be(GrantOutcome.Failed, - because: "a non-zero pwsh exit code means the fallback could not complete — Action Required must remain visible"); + because: "a non-zero az rest exit code means the assignment was not created — Action Required must remain visible"); } /// - /// Backward-compat contract: when no commandExecutor is supplied (e.g. callers that have not - /// been updated, or unattended/non-interactive runs), the PowerShell fallback is not attempted - /// and BlueprintS2SOutcome remains Failed exactly as before this feature was added. + /// Contract: when the operator declines the up-front S2S confirmation prompt, NEITHER + /// the primary Graph API path NOR the az rest fallback may run. BlueprintS2SOutcome is + /// set to Failed so DisplaySetupSummary surfaces the manual steps. This locks in the + /// fix for the regression where the prompt was only present in the fallback branch, + /// so operators on tenants where the primary path succeeded never saw a confirmation. /// [Fact] - public async Task ConfigureAllPermissions_WhenNoCommandExecutor_PwshFallbackNotAttempted() + public async Task ConfigureAllPermissions_WhenOperatorDeclinesS2SPrompt_NoGrantsAttempted() + { + // Arrange — Phase 1 + admin check OK, executor available, but operator says No. + ArrangeS2SPhase1AndAdminCheck(); + var confirmationProvider = Substitute.For(); + confirmationProvider.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(false)); + + var setupResults = new SetupResults(); + + // Act + await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( + _graph, _blueprintService, + new Agent365Config { TenantId = S2STenantId, AgentBlueprintId = S2SBlueprintAppId }, + blueprintAppId: S2SBlueprintAppId, tenantId: S2STenantId, + specs: S2SSpec(), _logger, setupResults, ct: default, + knownBlueprintSpObjectId: S2SBlueprintSpObjectId, + confirmationProvider: confirmationProvider, + commandExecutor: _executor); + + // Assert — outcome is Failed (Action Required surfaces manual steps). + setupResults.BlueprintS2SOutcome.Should().Be(GrantOutcome.Failed, + because: "operator declined the confirmation, so no S2S grants were attempted; Action Required must surface the manual steps"); + + // Primary path: no Graph API S2S call should have been made. + await _blueprintService.DidNotReceive().GrantAppRoleAssignmentAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any(), Arg.Any()); + + // Fallback path: no az rest call to the S2S appRoleAssignments endpoint either. + await _executor.DidNotReceive().ExecuteAsync( + "az", + Arg.Is(s => s.Contains("/appRoleAssignments")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + /// + /// Backward-compat contract: when no commandExecutor is supplied (unattended/non-interactive + /// runs, or callers that have not been updated), the az rest fallback is not attempted and + /// BlueprintS2SOutcome remains Failed exactly as before this feature was added. + /// + [Fact] + public async Task ConfigureAllPermissions_WhenNoCommandExecutor_AzRestFallbackNotAttempted() { // Arrange ArrangeS2SPhase1AndAdminCheck(); @@ -401,19 +446,54 @@ await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( // Assert setupResults.BlueprintS2SOutcome.Should().Be(GrantOutcome.Failed, - because: "without a commandExecutor the PowerShell fallback is not attempted and outcome stays Failed"); + because: "without a commandExecutor the az rest fallback is not attempted and outcome stays Failed"); - await _executor.DidNotReceive().ExecuteWithStreamingAsync( - Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any?>(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()); + await _executor.DidNotReceive().ExecuteAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + /// + /// Mocks the three az rest calls the AzRestS2SRunner issues per spec: + /// + /// GET /servicePrincipals/{blueprintSpId}/appRoleAssignments — existing-assignment lookup + /// GET /servicePrincipals?$filter=appId eq '...'&$select=id,appRoles — resource SP + role table + /// POST /servicePrincipals/{blueprintSpId}/appRoleAssignments — the create call whose exit code drives the outcome + /// + /// =true short-circuits the POST entirely (idempotent path). + /// Otherwise determines whether the runner reports success or failure. + /// + private void ArrangeAzRestS2SCalls(bool blueprintAlreadyAssigned, int postExitCode) + { + var assignmentsJson = blueprintAlreadyAssigned + ? "{\"value\":[{\"resourceId\":\"" + S2SResourceSpId + "\",\"appRoleId\":\"" + S2SAppRoleId + "\"}]}" + : "{\"value\":[]}"; + + _executor + .ExecuteAsync("az", + Arg.Is(s => s.Contains($"/servicePrincipals/{S2SBlueprintSpObjectId}/appRoleAssignments") && s.Contains("--method GET")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = assignmentsJson })); + + var roleValue = ConfigConstants.ObservabilityApiOtelWriteScope; + var spLookupJson = "{\"value\":[{\"id\":\"" + S2SResourceSpId + "\",\"appRoles\":[{\"id\":\"" + S2SAppRoleId + "\",\"value\":\"" + roleValue + "\"}]}]}"; + _executor + .ExecuteAsync("az", + Arg.Is(s => s.Contains("/servicePrincipals?") && s.Contains($"appId eq '{ConfigConstants.ObservabilityApiAppId}'") && s.Contains("$select=id,appRoles")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = spLookupJson })); + + _executor + .ExecuteAsync("az", + Arg.Is(s => s.Contains("--method POST") && s.Contains($"/servicePrincipals/{S2SBlueprintSpObjectId}/appRoleAssignments")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = postExitCode, StandardError = postExitCode == 0 ? string.Empty : "Insufficient privileges." })); } /// /// When the caller is a non-admin and the spec list includes S2S (AppRoleScopes) entries, /// ConfigureAllPermissionsAsync must set BlueprintS2SOutcome = Failed so that - /// DisplaySetupSummary surfaces the PowerShell S2S hand-off block in the Action Required - /// section — just like it does for a GA whose Graph API call returns 403. + /// DisplaySetupSummary surfaces the S2S hand-off block in the Action Required section — + /// just like it does for a GA whose Graph API call returns 403. /// [Fact] public async Task ConfigureAllPermissions_NonAdmin_WithS2SSpecs_SetsBlueprintS2SOutcomeFailed() @@ -452,7 +532,7 @@ await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( // Assert setupResults.BlueprintS2SOutcome.Should().Be(GrantOutcome.Failed, - because: "a non-admin user cannot complete S2S app role assignment directly — the outcome must be marked Failed so DisplaySetupSummary surfaces the PowerShell hand-off block"); + because: "a non-admin user cannot complete S2S app role assignment directly — the outcome must be marked Failed so DisplaySetupSummary surfaces the hand-off block"); } // ────────────────────────────────────────────────────────────────────────────────────── @@ -767,4 +847,114 @@ await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( setupResults.InheritablePermissionsAlreadyExisted.Should().BeFalse( because: "a failed inheritable spec breaks the 'all already existed' claim — the aggregation must require r.configured AND r.alreadyExisted for every spec, not just r.alreadyExisted"); } + + // ────────────────────────────────────────────────────────────────────────────────────── + // V2 audience routing through GrantAdminConsentAsync's catch-all spec loop. + // + // GetResourceIdentifierUri unit tests in SetupHelpersConsentUrlTests cover the helper's + // four branches in isolation. This test integrates through ConfigureAllPermissionsAsync + // and exercises the orchestrator's catch-all spec loop at GrantAdminConsentAsync — the + // layer where the original PR #432 bug lived (non-DW caller forgot to pass + // knownMcpAudienceAppIds, so V2 per-server audiences fell to api://{appId} → AADSTS500011). + // + // The compile-time fix (mcpScopesByAudience required on ExecuteBatchPermissionsStepAsync) + // makes the entry-point case unforgettable. This test additionally pins the contract at + // the deeper layer: that the URL-building loop honors knownMcpAudienceAppIds per spec. + // ────────────────────────────────────────────────────────────────────────────────────── + + /// + /// Non-admin path: GrantAdminConsentAsync builds the unified consent URL via the catch-all + /// spec loop and returns it for hand-off. When a spec's appId is in knownMcpAudienceAppIds, + /// the URL must contain its bare-GUID-prefixed scope (no api://). When the appId is NOT in + /// the set, the URL must contain the api:// form. Both must hold simultaneously — the + /// per-spec decision must use the audience set, not a global flag. + /// + [Fact] + public async Task ConfigureAllPermissions_NonAdmin_McpAudienceUsesBareGuid_NonMcpUsesApiScheme() + { + // Arrange — non-admin path so GrantAdminConsentAsync builds and returns the URL. + // BypassConsentChecksForTests is true by default (ctor); override to false here so + // the pre-check actually runs, finds "no grants" via the mocked Graph response, + // and the orchestrator falls through to URL building. Restored in the finally below. + var priorBypass = AdminConsentHelper.BypassConsentChecksForTests; + AdminConsentHelper.BypassConsentChecksForTests = false; + try + { + + const string mcpAudienceAppId = "16b1878d-62c7-4009-aa25-68989d63bbad"; // mcp_MailTools from ToolingManifest + const string mcpScope = "Tools.ListInvoke.All"; + const string customAppId = "99999999-9999-9999-9999-999999999999"; // not in manifest + const string customScope = "Custom.Scope"; + + // The /me probe in UpdateBlueprintPermissionsAsync gets the user JSON; every other + // GraphGetAsync (oauth2PermissionGrants pre-check) gets an empty value array so the + // pre-check returns "not consented" and the URL-building path runs. + _graph.GraphGetAsync( + Arg.Any(), Arg.Is(p => p.Contains("/me", StringComparison.Ordinal)), + Arg.Any(), Arg.Any?>()) + .Returns(JsonDocument.Parse("{\"id\":\"user-id\"}")); + _graph.GraphGetAsync( + Arg.Any(), Arg.Is(p => !p.Contains("/me", StringComparison.Ordinal)), + Arg.Any(), Arg.Any?>()) + .Returns(JsonDocument.Parse("{\"value\":[]}")); + + _graph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(RoleCheckResult.DoesNotHaveRole); + + // Phase 1 resource SP resolution succeeds for both appIds — the catch-all loop + // includes only specs whose SP was resolved (resolvedSpAppIds filter). + _graph.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any()) + .Returns(Task.FromResult("resolved-sp-id")); + + _blueprintService.SetInheritablePermissionsAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any?>(), Arg.Any()) + .Returns(Task.FromResult((ok: true, alreadyExists: true, error: (string?)null))); + + var specs = new[] + { + new ResourcePermissionSpec(mcpAudienceAppId, "Agent 365 Tools", + new[] { mcpScope }, SetInheritable: false), + new ResourcePermissionSpec(customAppId, "Custom Resource", + new[] { customScope }, SetInheritable: false), + }; + + var setupResults = new SetupResults(); + var mcpAudienceSet = new HashSet(StringComparer.OrdinalIgnoreCase) { mcpAudienceAppId }; + + // Act + var (_, _, _, adminConsentUrl) = await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( + _graph, _blueprintService, + new Agent365Config { TenantId = S2STenantId, AgentBlueprintId = S2SBlueprintAppId }, + blueprintAppId: S2SBlueprintAppId, tenantId: S2STenantId, + specs: specs, _logger, setupResults, ct: default, + knownBlueprintSpObjectId: S2SBlueprintSpObjectId, + commandExecutor: _executor, + knownMcpAudienceAppIds: mcpAudienceSet); + + // Assert — the URL was generated (non-admin hand-off) and routes the two specs + // through the correct branches of GetResourceIdentifierUri. + adminConsentUrl.Should().NotBeNull( + because: "non-admin path must return a consent URL the operator can hand off to a GA"); + + // MCP per-server audience → bare GUID. Uri.EscapeDataString turns '/' into %2F. + adminConsentUrl.Should().Contain($"{mcpAudienceAppId}%2F{mcpScope}", + because: "the manifest-known MCP appId must route through the bare-GUID branch in GetResourceIdentifierUri — this is the AADSTS500011 fix"); + adminConsentUrl.Should().NotContain($"api%3A%2F%2F{mcpAudienceAppId}", + because: "api://{appId} for a V2 MCP per-server audience produces AADSTS500011 because per-server SPs have identifierUris=null"); + + // Non-MCP custom resource → api://{appId}. EscapeDataString produces lowercase hex on + // some runtimes (api%3a%2f%2f) and uppercase on others (api%3A%2F%2F); FluentAssertions + // matches case-insensitively here to keep the test stable across .NET versions. + adminConsentUrl.Should().ContainEquivalentOf($"api%3A%2F%2F{customAppId}%2F{customScope}", + because: "non-MCP unknown resources keep the standard api://{appId} Application ID URI form (pre-PR-#432 behavior)"); + + } + finally + { + AdminConsentHelper.BypassConsentChecksForTests = priorBypass; + } + } } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PowerShellS2SRunnerTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PowerShellS2SRunnerTests.cs deleted file mode 100644 index 1086718e..00000000 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PowerShellS2SRunnerTests.cs +++ /dev/null @@ -1,278 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using FluentAssertions; -using Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; -using Microsoft.Agents.A365.DevTools.Cli.Constants; -using Microsoft.Agents.A365.DevTools.Cli.Services; -using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Logging.Abstractions; -using NSubstitute; -using Xunit; - -namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; - -public class PowerShellS2SRunnerTests -{ - private readonly CommandExecutor _executor; - private readonly ILogger _logger; - - public PowerShellS2SRunnerTests() - { - _logger = NullLogger.Instance; - _executor = Substitute.For(Substitute.For>()); - } - - /// - /// GUID validation rejects non-GUID tenantId/blueprintAppId without calling the executor. - /// This guards against script injection via malformed input. - /// - [Fact] - public async Task TryRunAsync_InvalidTenantIdGuid_ReturnsFalseWithoutCallingExecutor() - { - // Arrange - var specs = new[] - { - new ResourcePermissionSpec( - ConfigConstants.ObservabilityApiAppId, - "Observability API", - new[] { ConfigConstants.ObservabilityApiOtelWriteScope }, - SetInheritable: false, - AppRoleScopes: new[] { ConfigConstants.ObservabilityApiOtelWriteScope }) - }; - - // Act - var (attempted, succeeded) = await PowerShellS2SRunner.TryRunAsync( - _executor, - tenantId: "not-a-guid", - blueprintAppId: "00000000-0000-0000-0000-000000000002", - specs: specs, - _logger, - ct: default); - - // Assert - attempted.Should().BeFalse(because: "invalid GUID must be rejected before launching pwsh"); - succeeded.Should().BeFalse(); - - await _executor.DidNotReceive().ExecuteWithStreamingAsync( - Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any?>(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()); - } - - /// - /// When pwsh is not found on the system (ExecuteWithStreamingAsync throws Win32Exception - /// with NativeErrorCode 2 — ERROR_FILE_NOT_FOUND / ENOENT), - /// TryRunAsync returns (false, false, false) — the caller falls through to Action Required. - /// - [Fact] - public async Task TryRunAsync_PwshNotFound_ReturnsFalseWithoutAttempting() - { - // Arrange - _executor.ExecuteWithStreamingAsync( - Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any?>(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()) - .Returns(Task.FromException(new System.ComponentModel.Win32Exception(2))); - - var specs = new[] - { - new ResourcePermissionSpec( - ConfigConstants.ObservabilityApiAppId, - "Observability API", - new[] { ConfigConstants.ObservabilityApiOtelWriteScope }, - SetInheritable: false, - AppRoleScopes: new[] { ConfigConstants.ObservabilityApiOtelWriteScope }) - }; - - // Act - var (attempted, succeeded) = await PowerShellS2SRunner.TryRunAsync( - _executor, - tenantId: "00000000-0000-0000-0000-000000000001", - blueprintAppId: "00000000-0000-0000-0000-000000000002", - specs: specs, - _logger, - ct: default); - - // Assert - attempted.Should().BeFalse(because: "pwsh not found means no execution was possible"); - succeeded.Should().BeFalse(); - } - - /// - /// When pwsh is available and the script exits with code 0, TryRunAsync returns - /// (Attempted=true, Succeeded=true). The script passed to the executor must contain - /// the tenantId, blueprintAppId, and role values. - /// - [Fact] - public async Task TryRunAsync_ValidInputsAndPwshSucceeds_ScriptContainsExpectedValuesAndReturnsSucceeded() - { - // Arrange - var tenantId = "00000000-0000-0000-0000-000000000001"; - var blueprintAppId = "00000000-0000-0000-0000-000000000002"; - - string? capturedScript = null; - _executor.ExecuteWithStreamingAsync( - Arg.Any(), - Arg.Do(args => - { - var match = System.Text.RegularExpressions.Regex.Match(args, @"-File ""([^""]+)"""); - if (match.Success && System.IO.File.Exists(match.Groups[1].Value)) - capturedScript = System.IO.File.ReadAllText(match.Groups[1].Value); - }), - Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>(), - Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()) - .Returns(new CommandResult { ExitCode = 0 }); - - var specs = new[] - { - new ResourcePermissionSpec( - ConfigConstants.ObservabilityApiAppId, - "Observability API", - new[] { ConfigConstants.ObservabilityApiOtelWriteScope }, - SetInheritable: false, - AppRoleScopes: new[] { ConfigConstants.ObservabilityApiOtelWriteScope }) - }; - - // Act - var (attempted, succeeded) = await PowerShellS2SRunner.TryRunAsync( - _executor, - tenantId: tenantId, - blueprintAppId: blueprintAppId, - specs: specs, - _logger, - ct: default); - - // Assert - attempted.Should().BeTrue(because: "pwsh was found and the script was executed"); - succeeded.Should().BeTrue(because: "pwsh exited with code 0"); - - capturedScript.Should().NotBeNull(); - capturedScript.Should().Contain(tenantId, - because: "the script must be scoped to the correct tenant"); - capturedScript.Should().Contain(blueprintAppId, - because: "the script must target the blueprint application"); - capturedScript.Should().Contain(ConfigConstants.ObservabilityApiAppId, - because: "the script must reference the resource app ID for each spec"); - capturedScript.Should().Contain(ConfigConstants.ObservabilityApiOtelWriteScope, - because: "the script must include the app role value to look up"); - capturedScript.Should().Contain("-ContextScope Process", - because: "Connect-MgGraph must use process-scoped auth to bypass the persistent token cache and avoid DeviceCodeCredential NRE on repeat runs"); - capturedScript.Should().Contain("Microsoft.Graph.Authentication", - because: "Connect-MgGraph lives in Authentication, which must be pinned and imported before Applications to avoid version-mismatch assembly conflicts"); - } - - /// - /// When pwsh runs but exits with a non-zero exit code, TryRunAsync returns - /// (Attempted=true, Succeeded=false). Success is now determined purely by exit code - /// since stdout is no longer redirected back to the parent. - /// - [Fact] - public async Task TryRunAsync_PwshExitsNonZero_ReturnsAttemptedNotSucceeded() - { - // Arrange — pwsh ran but exited with code 1 (e.g. assignment failed) - _executor.ExecuteWithStreamingAsync( - Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any?>(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()) - .Returns(new CommandResult { ExitCode = 1 }); - - var specs = new[] - { - new ResourcePermissionSpec( - ConfigConstants.ObservabilityApiAppId, - "Observability API", - new[] { ConfigConstants.ObservabilityApiOtelWriteScope }, - SetInheritable: false, - AppRoleScopes: new[] { ConfigConstants.ObservabilityApiOtelWriteScope }) - }; - - // Act - var (attempted, succeeded) = await PowerShellS2SRunner.TryRunAsync( - _executor, - tenantId: "00000000-0000-0000-0000-000000000001", - blueprintAppId: "00000000-0000-0000-0000-000000000002", - specs: specs, - _logger, - ct: default); - - // Assert - attempted.Should().BeTrue(because: "pwsh was invoked and produced an exit code"); - succeeded.Should().BeFalse(because: "non-zero exit code indicates the script failed"); - } - - /// - /// GUID validation rejects an invalid blueprintAppId even when tenantId is valid, - /// without calling the executor. - /// - [Fact] - public async Task TryRunAsync_InvalidBlueprintAppIdGuid_ReturnsFalseWithoutCallingExecutor() - { - // Arrange - var specs = new[] - { - new ResourcePermissionSpec( - ConfigConstants.ObservabilityApiAppId, - "Observability API", - new[] { ConfigConstants.ObservabilityApiOtelWriteScope }, - SetInheritable: false, - AppRoleScopes: new[] { ConfigConstants.ObservabilityApiOtelWriteScope }) - }; - - // Act - var (attempted, succeeded) = await PowerShellS2SRunner.TryRunAsync( - _executor, - tenantId: "00000000-0000-0000-0000-000000000001", - blueprintAppId: "not-a-guid", - specs: specs, - _logger, - ct: default); - - // Assert - attempted.Should().BeFalse(because: "invalid blueprintAppId GUID must be rejected before launching pwsh"); - succeeded.Should().BeFalse(); - - await _executor.DidNotReceive().ExecuteWithStreamingAsync( - Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any?>(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()); - } - - /// - /// When all specs have no AppRoleScopes, TryRunAsync returns (false, false, false) - /// immediately without invoking the executor. - /// - [Fact] - public async Task TryRunAsync_SpecsWithNoAppRoleScopes_ReturnsFalseWithoutCallingExecutor() - { - // Arrange — spec with delegated-only scopes, no AppRoleScopes - var specs = new[] - { - new ResourcePermissionSpec( - ConfigConstants.ObservabilityApiAppId, - "Observability API", - new[] { ConfigConstants.ObservabilityApiOtelWriteScope }, - SetInheritable: false, - AppRoleScopes: Array.Empty()) - }; - - // Act - var (attempted, succeeded) = await PowerShellS2SRunner.TryRunAsync( - _executor, - tenantId: "00000000-0000-0000-0000-000000000001", - blueprintAppId: "00000000-0000-0000-0000-000000000002", - specs: specs, - _logger, - ct: default); - - // Assert - attempted.Should().BeFalse(because: "no S2S specs means there is nothing to assign via PowerShell"); - succeeded.Should().BeFalse(); - - await _executor.DidNotReceive().ExecuteWithStreamingAsync( - Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any?>(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()); - } -} diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupSubcommands/PermissionSpecsTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupSubcommands/PermissionSpecsTests.cs index c8b61e31..674d6a52 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupSubcommands/PermissionSpecsTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupSubcommands/PermissionSpecsTests.cs @@ -286,6 +286,31 @@ public async Task ObservabilityApi_CarriesBothDelegatedScopeAndAppRole() because: "Observability API app role grants OtelWrite for the s2s path — losing either side breaks one auth mode"); } + [Fact] + public async Task MessagingBotApi_UsesScopeConstantSoSpecAndConsentUrlAgree() + { + // Arrange: smallest config that produces the Messaging Bot spec. + var config = new Agent365Config { DeploymentProjectPath = _tempDir }; + + // Act + var specs = await SetupHelpers.BuildConfiguredPermissionSpecsAsync(config, setInheritable: true, isM365: true); + + // Assert: the Messaging Bot spec must request exactly the scopes that the resource SP + // exposes (a live query shows AgentData.ReadWrite is the only delegated scope on + // appId 5a807f24-c9de-44ee-a3a7-329e88a00ffc). Requesting any other scope here causes + // /v2.0/adminconsent to reject the entire combined-URL request with + // AADSTS650053 ("scope doesn't exist on the resource") — see issue #429. + // + // The single source of truth is ConfigConstants.MessagingBotApiAdminConsentScope. + // Asserting against the constant (rather than a string literal) means future scope-name + // changes propagate consistently to both the spec list and the per-resource URL builder + // — the same constant is used by SetupHelpers.BuildAdminConsentUrls / + // BuildCombinedConsentUrl, so a one-place update keeps both paths aligned. + var bot = SpecFor(specs, ConfigConstants.MessagingBotApiAppId); + bot.Scopes.Should().BeEquivalentTo(new[] { ConfigConstants.MessagingBotApiAdminConsentScope }, + because: "the spec must request only scopes the Messaging Bot resource SP exposes; the unified /v2.0/adminconsent URL strictly validates scope existence and rejects with AADSTS650053 otherwise (issue #429). Using the constant keeps the spec list and the URL builder pinned to a single source of truth."); + } + [Fact] public async Task SetInheritableFlag_PropagatesToEverySpec() { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersConsentUrlTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersConsentUrlTests.cs index 1bc7aab5..9607bbb4 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersConsentUrlTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersConsentUrlTests.cs @@ -306,4 +306,186 @@ public void PopulateAdminConsentUrls_NonM365_ResourceConsentsExcludeMessagingBot rc => rc.ResourceAppId == ConfigConstants.MessagingBotApiAppId, because: "no Messaging Bot consent URL is generated for non-M365 agents, so no resourceConsents entry should be persisted"); } + + // ── V2 per-server audience routing (issue #429) ────────────────────────── + // + // V2 manifest entries declare a per-server audience (a unique Entra appId) and the + // generic scope "Tools.ListInvoke.All". Each per-server SP exposes that scope on its + // OWN identifierUri; WorkIQ Tools (the V1 shared resource) does NOT publish it. + // + // Pre-fix behavior collapsed every "Agent 365 Tools"-named spec onto the shared + // WorkIQ URI (https://agent365.svc.cloud.microsoft), producing a URL that asked Entra + // for Tools.ListInvoke.All on WorkIQ — which fails with AADSTS650053. The URL builders + // must route per-server audiences through the bare appId GUID so each scope lands on + // its actual SP. api://{appId} is NOT used — per-server SPs have identifierUris null + // and the bare GUID is what's in servicePrincipalNames (api:// triggers AADSTS500011). + + private const string PerServerAudienceMail = "16b1878d-62c7-4009-aa25-68989d63bbad"; + private const string PerServerAudienceCalendar = "910333d2-47e9-43ca-981f-6df2f4531ef4"; + private const string V2Scope = "Tools.ListInvoke.All"; + + [Fact] + public void BuildCombinedConsentUrl_V2PerServerAudiences_EmitsBareAppIdResourcePerAudienceNotWorkIqUri() + { + var scopesByAudience = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + [PerServerAudienceMail] = new[] { V2Scope }, + [PerServerAudienceCalendar] = new[] { V2Scope }, + // WorkIQ Tools still carries its V1-compat seed scope; it must keep the canonical URI. + [McpConstants.WorkIQToolsProdAppId] = new[] { "McpServersMetadata.Read.All" }, + }; + + var url = SetupHelpers.BuildCombinedConsentUrl( + TenantId, BlueprintClientId, + graphScopes: Array.Empty(), + mcpScopes: Array.Empty(), + isM365: false, + mcpScopesByAudience: scopesByAudience); + + // Per-server V2 SPs (e.g. Work IQ Mail MCP, appId 16b1878d-...) have identifierUris + // unset; their only registered resource identifier is the bare appId GUID in + // servicePrincipalNames. The previous "api://{appId}" form caused AADSTS500011 + // ("resource principal not found"); the bare appId form is the canonical fallback + // Entra accepts for SPs without a published Application ID URI. + url.Should().Contain(Uri.EscapeDataString($"{PerServerAudienceMail}/{V2Scope}"), + because: "V2 per-server SPs publish only the bare appId GUID as their resource identifier; api://{appId} produces AADSTS500011 because that URI is not registered on the SP (issue #429)"); + url.Should().NotContain(Uri.EscapeDataString($"api://{PerServerAudienceMail}/{V2Scope}"), + because: "the api:// prefix must NOT be emitted for per-server audiences — that was the AADSTS500011 regression"); + url.Should().Contain(Uri.EscapeDataString($"{PerServerAudienceCalendar}/{V2Scope}"), + because: "every V2 audience routes through its own appId; collapsing them would produce a single URL fragment that Entra rejects"); + url.Should().Contain(Uri.EscapeDataString($"{McpConstants.Agent365ToolsIdentifierUri}/McpServersMetadata.Read.All"), + because: "the WorkIQ Tools (V1-shared) audience still uses the canonical https URI — the V2 fix must not regress V1 routing"); + } + + [Fact] + public void BuildAdminConsentUrls_V2PerServerAudiences_OneUrlPerAudience() + { + var scopesByAudience = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + [PerServerAudienceMail] = new[] { V2Scope }, + [PerServerAudienceCalendar] = new[] { V2Scope }, + }; + + var urls = SetupHelpers.BuildAdminConsentUrls( + TenantId, BlueprintClientId, + graphScopes: new[] { "Mail.Send" }, + mcpScopes: Array.Empty(), + isM365: false, + mcpScopesByAudience: scopesByAudience); + + urls.Should().Contain(u => u.ConsentUrl.Contains(Uri.EscapeDataString($"{PerServerAudienceMail}/{V2Scope}")), + because: "the per-resource URL list must surface a URL the operator can hand off for the Mail MCP audience — collapsing it onto WorkIQ would point the operator at an SP that does not publish the scope"); + urls.Should().Contain(u => u.ConsentUrl.Contains(Uri.EscapeDataString($"{PerServerAudienceCalendar}/{V2Scope}")), + because: "every per-server audience needs its own per-resource handoff URL"); + } + + [Fact] + public void PopulateAdminConsentUrls_V2PerServerAudiences_AddsResourceConsentPerAudience() + { + var config = new Agent365Config + { + TenantId = TenantId, + AgentBlueprintId = BlueprintClientId, + }; + + var scopesByAudience = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + [PerServerAudienceMail] = new[] { V2Scope }, + [PerServerAudienceCalendar] = new[] { V2Scope }, + }; + + var names = SetupHelpers.PopulateAdminConsentUrls( + config, McpConstants.WorkIQToolsProdAppId, + mcpScopes: Array.Empty(), + isM365: false, + mcpScopesByAudience: scopesByAudience); + + config.ResourceConsents.Should().Contain(rc => rc.ResourceAppId == PerServerAudienceMail, + because: "each V2 per-server audience needs its own ResourceConsent entry so query-entra and the setup summary surface the right SP for the operator to verify"); + config.ResourceConsents.Should().Contain(rc => rc.ResourceAppId == PerServerAudienceCalendar); + names.Should().Contain(n => n.Contains(PerServerAudienceMail) || n.Contains("Mail", StringComparison.OrdinalIgnoreCase)); + } + + // ───────────────────────────────────────────────────────────────────────── + // GetResourceIdentifierUri — three-branch contract. + // + // Branch 1: known platform resources (Graph, Bot, Obs, PP) → canonical URI. + // (Covered by the BuildAdminConsentUrls happy-path tests above.) + // Branch 2: WorkIQ Tools shared appId → canonical https URI; takes precedence + // even when isMcpAudience=true (because the WorkIQ SP publishes the + // canonical URI and Entra accepts it). + // Branch 3: isMcpAudience=true (V2 MCP per-server) → bare appId GUID. + // Reason: those SPs have identifierUris=null; api://{appId} → AADSTS500011. + // Branch 4 (default): every other unknown resource → api://{appId}. Restores + // the long-standing pre-PR-#432 behavior for custom resources so a custom + // appId whose SP omits the bare GUID from servicePrincipalNames doesn't + // silently break consent. + // + // These tests pin all four branches so a future refactor cannot generalize + // the bare-GUID-for-all-unknowns rule back without us noticing. + // ───────────────────────────────────────────────────────────────────────── + + private const string CustomResourceAppId = "abcdef01-2345-6789-abcd-ef0123456789"; + + [Fact] + public void GetResourceIdentifierUri_UnknownResource_DefaultsToApiScheme() + { + var uri = SetupHelpers.GetResourceIdentifierUri(CustomResourceAppId); + + uri.Should().Be($"api://{CustomResourceAppId}", + because: "custom resources default to api://{appId} — switching them to the bare GUID would risk AADSTS errors for custom apps whose SPs do not register the bare appId in servicePrincipalNames"); + } + + [Fact] + public void GetResourceIdentifierUri_UnknownResource_WithIsMcpAudienceFalse_DefaultsToApiScheme() + { + // Caller has examined the manifest, knows this appId is NOT an MCP audience. + var uri = SetupHelpers.GetResourceIdentifierUri(CustomResourceAppId, isMcpAudience: false); + + uri.Should().Be($"api://{CustomResourceAppId}", + because: "isMcpAudience=false signals 'caller checked the manifest; this is not an MCP per-server audience' — keep the safer api://{appId} form"); + } + + [Fact] + public void GetResourceIdentifierUri_PerServerMcpAudience_WithIsMcpAudienceTrue_ReturnsBareAppIdGuid() + { + // Caller is iterating the manifest audience set OR found this appId in it. + var perServerAppId = "16b1878d-62c7-4009-aa25-68989d63bbad"; // Mail MCP, from ToolingManifest.json + var uri = SetupHelpers.GetResourceIdentifierUri(perServerAppId, isMcpAudience: true); + + uri.Should().Be(perServerAppId, + because: "V2 MCP per-server SPs publish only the bare appId GUID in servicePrincipalNames — the api:// form is rejected with AADSTS500011 (issue #429)"); + } + + [Fact] + public void GetResourceIdentifierUri_WorkIqSharedAppId_ReturnsCanonicalHttpsUri_RegardlessOfIsMcpAudience() + { + // WorkIQ shared appId branch fires before the isMcpAudience check, so even if + // the caller mistakenly flags it as an MCP per-server audience, the result is + // the canonical https URI that the WorkIQ SP publishes. + var withFalse = SetupHelpers.GetResourceIdentifierUri(McpConstants.WorkIQToolsProdAppId, isMcpAudience: false); + var withTrue = SetupHelpers.GetResourceIdentifierUri(McpConstants.WorkIQToolsProdAppId, isMcpAudience: true); + + withFalse.Should().Be(McpConstants.Agent365ToolsIdentifierUri, + because: "the WorkIQ Tools shared audience publishes the canonical https URI; that must continue to flow through (the V2 fix must not regress V1 routing)"); + withTrue.Should().Be(McpConstants.Agent365ToolsIdentifierUri, + because: "WorkIQ shared check is by appId and takes precedence over the isMcpAudience signal — guards against a caller that erroneously flags WorkIQ as a per-server audience"); + } + + [Fact] + public void BuildFullyQualifiedScope_PassesIsMcpAudienceThrough() + { + // Spot-check that the convenience helper's isMcpAudience parameter is forwarded + // to GetResourceIdentifierUri (and the result is "{resource}/{scope}" concatenated). + var perServerAppId = "16b1878d-62c7-4009-aa25-68989d63bbad"; + var scope = "Tools.ListInvoke.All"; + + var withFalse = SetupHelpers.BuildFullyQualifiedScope(perServerAppId, scope, isMcpAudience: false); + var withTrue = SetupHelpers.BuildFullyQualifiedScope(perServerAppId, scope, isMcpAudience: true); + + withFalse.Should().Be($"api://{perServerAppId}/{scope}", + because: "default catch-all when caller has no MCP signal"); + withTrue.Should().Be($"{perServerAppId}/{scope}", + because: "V2 MCP per-server audience — bare appId GUID prefix"); + } } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AdminConsentHelperTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AdminConsentHelperTests.cs index 47808ff4..cd8a3393 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AdminConsentHelperTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AdminConsentHelperTests.cs @@ -36,20 +36,26 @@ public async Task PollAdminConsentAsync_ReturnsTrue_WhenGrantExists() } [Fact] - public async Task PollAdminConsentAsync_ReturnsFalse_WhenNoGrant() + public async Task PollAdminConsentAsync_PropagatesCancellation_WhenTokenCanceled() { + // Requirement: Ctrl+C during admin-consent polling must propagate the + // OperationCanceledException up to AllSubcommand's OCE handler so setup aborts + // cleanly. The previous implementation swallowed OCE and returned false, which + // then fell through to the az rest fallback prompt — confusing operators who + // had just pressed Ctrl+C. Mirrors the Graph overload contract. var executor = Substitute.For(Substitute.For>()); var logger = Substitute.For(); - // service principal not found + // No grant — so the loop will iterate and hit Task.Delay where the CT fires. executor.ExecuteAsync("az", Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult(new Microsoft.Agents.A365.DevTools.Cli.Services.CommandResult { ExitCode = 0, StandardOutput = "{\"value\":[]}" })); - // Use intervalSeconds=0 and a short CTS to avoid real waits — this is a mock-only test. var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(200)); - var result = await AdminConsentHelper.PollAdminConsentAsync(executor, logger, "appId-1", "Test", 1, 0, cts.Token); - result.Should().BeFalse(); + Func act = () => AdminConsentHelper.PollAdminConsentAsync(executor, logger, "appId-1", "Test", 10, 1, cts.Token); + + await act.Should().ThrowAsync( + because: "OCE must propagate so Ctrl+C aborts setup via AllSubcommand's OCE handler instead of falling into the az rest delegated-consent fallback prompt with a stale 'permission(s)?' question"); } [Fact] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/ScopeAvailabilityValidatorTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/ScopeAvailabilityValidatorTests.cs new file mode 100644 index 00000000..a1e24486 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/ScopeAvailabilityValidatorTests.cs @@ -0,0 +1,243 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; +using Microsoft.Extensions.Logging; +using NSubstitute; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services.Helpers; + +/// +/// Tests for . +/// +/// +/// The validator's contract (issue #429): given a list of permission specs and a map of +/// resolved resource SP object IDs, query each SP's published delegated scopes and drop +/// any requested scope the SP does not expose. The caller (BatchPermissionsOrchestrator) +/// uses the filtered list to build a /v2.0/adminconsent URL that Entra will accept, +/// and uses the dropped-scope list to emit a per-resource warning and offer a PowerShell +/// fallback for users who want to stamp those scopes via the lenient programmatic +/// oauth2PermissionGrants path. +/// +/// +public class ScopeAvailabilityValidatorTests +{ + private const string Tenant = "11111111-1111-1111-1111-111111111111"; + private const string GraphAppId = "00000003-0000-0000-c000-000000000000"; + private const string GraphSpId = "22222222-2222-2222-2222-222222222222"; + private const string BotAppId = "5a807f24-c9de-44ee-a3a7-329e88a00ffc"; + private const string BotSpId = "33333333-3333-3333-3333-333333333333"; + + private readonly GraphApiService _graph = Substitute.For(); + private readonly ILogger _logger = Substitute.For(); + + [Fact] + public async Task ValidScopes_PassThroughUnchangedAndDropNothing() + { + // Arrange — Graph SP exposes the exact scope set the spec requests. + _graph + .GetAvailableScopeNamesAsync(Tenant, GraphSpId, Arg.Any()) + .Returns(new HashSet(StringComparer.OrdinalIgnoreCase) { "Mail.Send", "User.Read" }); + + var specs = new[] + { + new ResourcePermissionSpec(GraphAppId, "Microsoft Graph", new[] { "Mail.Send", "User.Read" }, SetInheritable: true) + }; + var spMap = new Dictionary { [GraphAppId] = GraphSpId }; + + // Act + var result = await ScopeAvailabilityValidator.ValidateAsync(_graph, Tenant, specs, spMap, _logger, CancellationToken.None); + + // Assert + result.DroppedScopes.Should().BeEmpty( + because: "every requested scope is published on the resource SP — nothing should be filtered"); + result.EffectiveSpecs.Should().ContainSingle() + .Which.Scopes.Should().BeEquivalentTo(new[] { "Mail.Send", "User.Read" }, + because: "the spec passes through unchanged when all scopes are valid"); + } + + [Fact] + public async Task UnavailableScope_DroppedFromSpecAndRecordedInResult() + { + // Arrange — SP exposes one of the two requested scopes. + _graph + .GetAvailableScopeNamesAsync(Tenant, BotSpId, Arg.Any()) + .Returns(new HashSet(StringComparer.OrdinalIgnoreCase) { "AgentData.ReadWrite" }); + + var specs = new[] + { + // This is the exact pre-fix Messaging Bot spec from issue #429: + // "Authorization.ReadWrite" and "user_impersonation" do not exist on the + // Messaging Bot SP, which makes the entire /v2.0/adminconsent URL fail with + // AADSTS650053. The validator must keep AgentData.ReadWrite and drop the others. + new ResourcePermissionSpec(BotAppId, "Messaging Bot API", + new[] { "Authorization.ReadWrite", "AgentData.ReadWrite", "user_impersonation" }, + SetInheritable: true) + }; + var spMap = new Dictionary { [BotAppId] = BotSpId }; + + // Act + var result = await ScopeAvailabilityValidator.ValidateAsync(_graph, Tenant, specs, spMap, _logger, CancellationToken.None); + + // Assert + result.EffectiveSpecs.Should().ContainSingle() + .Which.Scopes.Should().BeEquivalentTo(new[] { "AgentData.ReadWrite" }, + because: "scopes the resource SP does not publish must be filtered out so the unified /v2.0/adminconsent URL does not blow up with AADSTS650053"); + + result.DroppedScopes.Should().BeEquivalentTo(new[] + { + new ScopeAvailabilityValidator.DroppedScope("Messaging Bot API", BotAppId, "Authorization.ReadWrite"), + new ScopeAvailabilityValidator.DroppedScope("Messaging Bot API", BotAppId, "user_impersonation"), + }, because: "the caller surfaces a warning per dropped (resource, scope) pair and offers a PowerShell fallback for users who want to stamp them anyway via the lenient programmatic OAuth2 grant path"); + } + + [Fact] + public async Task MissingSpObjectId_PassesSpecThroughUnchanged() + { + // Arrange — no SP id was resolved for the resource in Phase 1. + var specs = new[] + { + new ResourcePermissionSpec(GraphAppId, "Microsoft Graph", new[] { "Mail.Send" }, SetInheritable: true) + }; + var emptyMap = new Dictionary(); + + // Act + var result = await ScopeAvailabilityValidator.ValidateAsync(_graph, Tenant, specs, emptyMap, _logger, CancellationToken.None); + + // Assert + result.EffectiveSpecs.Should().ContainSingle() + .Which.Scopes.Should().BeEquivalentTo(new[] { "Mail.Send" }, + because: "specs whose SP could not be resolved in Phase 1 must pass through unchanged — dropping every scope on an unresolvable SP would silently empty the consent URL, which is worse than letting AADSTS650053 surface if it gets that far"); + result.DroppedScopes.Should().BeEmpty(); + + await _graph.DidNotReceive().GetAvailableScopeNamesAsync(Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task GraphReturnsEmptySet_PassesSpecThroughUnchanged() + { + // Arrange — Graph call swallows errors and returns an empty set when the SP + // cannot be read. Treat that as "we don't know," not "the SP exposes nothing." + _graph + .GetAvailableScopeNamesAsync(Tenant, GraphSpId, Arg.Any()) + .Returns(new HashSet(StringComparer.OrdinalIgnoreCase)); + + var specs = new[] + { + new ResourcePermissionSpec(GraphAppId, "Microsoft Graph", new[] { "Mail.Send" }, SetInheritable: true) + }; + var spMap = new Dictionary { [GraphAppId] = GraphSpId }; + + // Act + var result = await ScopeAvailabilityValidator.ValidateAsync(_graph, Tenant, specs, spMap, _logger, CancellationToken.None); + + // Assert + result.EffectiveSpecs.Should().ContainSingle() + .Which.Scopes.Should().BeEquivalentTo(new[] { "Mail.Send" }, + because: "an empty published-scopes set means the Graph call could not read the SP; dropping every requested scope on that basis would block setup even when the scopes are actually valid"); + result.DroppedScopes.Should().BeEmpty(); + } + + [Fact] + public async Task EmptySpecScopes_PassThroughWithoutGraphCall() + { + // Arrange — spec with no delegated scopes (e.g. an app-role-only spec). The + // validator must not waste a Graph round-trip on a no-op. + var specs = new[] + { + new ResourcePermissionSpec(GraphAppId, "Microsoft Graph", Array.Empty(), SetInheritable: false) + }; + var spMap = new Dictionary { [GraphAppId] = GraphSpId }; + + // Act + var result = await ScopeAvailabilityValidator.ValidateAsync(_graph, Tenant, specs, spMap, _logger, CancellationToken.None); + + // Assert + result.EffectiveSpecs.Should().ContainSingle() + .Which.Scopes.Should().BeEmpty(); + result.DroppedScopes.Should().BeEmpty(); + await _graph.DidNotReceive().GetAvailableScopeNamesAsync(Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task GraphCallThrows_PassesSpecThroughUnchangedAndDoesNotPropagate() + { + // Arrange — simulate a transient Graph failure (or, in practice, a stubbed test + // where the mocked JsonDocument is disposed across calls — this is exactly what + // broke existing BatchPermissionsOrchestratorTests when the validator was first + // wired in). The validator is a safety net; an internal failure must never block + // setup or surface as an unhandled exception. + _graph + .GetAvailableScopeNamesAsync(Tenant, GraphSpId, Arg.Any()) + .Returns>(_ => throw new ObjectDisposedException("JsonDocument")); + + var specs = new[] + { + new ResourcePermissionSpec(GraphAppId, "Microsoft Graph", new[] { "Mail.Send" }, SetInheritable: true) + }; + var spMap = new Dictionary { [GraphAppId] = GraphSpId }; + + // Act + var result = await ScopeAvailabilityValidator.ValidateAsync(_graph, Tenant, specs, spMap, _logger, CancellationToken.None); + + // Assert + result.EffectiveSpecs.Should().ContainSingle() + .Which.Scopes.Should().BeEquivalentTo(new[] { "Mail.Send" }, + because: "a Graph error while validating must not drop scopes — the validator is a defensive safety net, not a gatekeeper, and missing a filter opportunity is better than blocking setup on an internal validator failure"); + result.DroppedScopes.Should().BeEmpty(); + } + + [Fact] + public async Task CancellationDuringGraphCall_PropagatesOperationCanceled() + { + // Arrange — distinguish "Graph failed" (swallow) from "operator hit Ctrl+C" + // (must propagate so the rest of setup terminates promptly). + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + _graph + .GetAvailableScopeNamesAsync(Tenant, GraphSpId, Arg.Any()) + .Returns>(_ => throw new OperationCanceledException(cts.Token)); + + var specs = new[] + { + new ResourcePermissionSpec(GraphAppId, "Microsoft Graph", new[] { "Mail.Send" }, SetInheritable: true) + }; + var spMap = new Dictionary { [GraphAppId] = GraphSpId }; + + // Act + Func act = () => ScopeAvailabilityValidator.ValidateAsync(_graph, Tenant, specs, spMap, _logger, cts.Token); + + // Assert + await act.Should().ThrowAsync( + because: "Ctrl+C during validation must abort setup, not be silently swallowed like a transient Graph error"); + } + + [Fact] + public async Task CaseInsensitiveScopeMatch_KeepsDifferentCasing() + { + // Arrange — SP publishes "AgentData.ReadWrite", spec asks for "agentdata.readwrite". + // Case mismatch must not cause a false drop — Entra is case-insensitive on scope names. + _graph + .GetAvailableScopeNamesAsync(Tenant, BotSpId, Arg.Any()) + .Returns(new HashSet(StringComparer.OrdinalIgnoreCase) { "AgentData.ReadWrite" }); + + var specs = new[] + { + new ResourcePermissionSpec(BotAppId, "Messaging Bot API", new[] { "agentdata.readwrite" }, SetInheritable: true) + }; + var spMap = new Dictionary { [BotAppId] = BotSpId }; + + // Act + var result = await ScopeAvailabilityValidator.ValidateAsync(_graph, Tenant, specs, spMap, _logger, CancellationToken.None); + + // Assert + result.EffectiveSpecs.Single().Scopes.Should().BeEquivalentTo(new[] { "agentdata.readwrite" }, + because: "scope matching must be case-insensitive to mirror Entra's behavior — a casing-only difference is not a real mismatch"); + result.DroppedScopes.Should().BeEmpty(); + } +}