diff --git a/CHANGELOG.md b/CHANGELOG.md index afb6d9e2..158a32eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). **Option B — CLI**: `a365 setup admin` has been removed in this release. For blueprints that lack the inheritable-permission entry, including those created after this release, run `a365 setup permissions custom --resource-app-id --scopes Agent365.Observability.OtelWrite` to stamp it and request consent for the delegated permission. The CLI does not grant the application permission; use Option A for it. +#### Existing agents: grant Defender API permissions + +Agents provisioned before this release should have a Global Administrator re-run `a365 setup all --authmode ` for blueprint agents (delegated for `obo`, application for `s2s`, or both for `both`) or `a365 setup all` without `--authmode` for AI Teammates (both permission types) to stamp Defender inheritance and grant `RealtimeProtection.Evaluate.All` (#485). + ### Added + +- `a365 setup all` now grants the Defender API `RealtimeProtection.Evaluate.All` permission according to `--authmode` for blueprint agents (delegated for `obo`, application for `s2s`, both for `both`) and as both delegated and application for AI Teammates (#485). - `a365 develop-mcp grant-agents-access --agent-blueprint-id --mcp-server-name ` reports which agent instances of a blueprint are missing the permission to call a BYO MCP server and prompts you to select which ones to grant it to, requiring administrator consent on the first run in a tenant (#500). - When more than one Entra application shares the MCP server's name, `a365 develop-mcp grant-agents-access` now lists them all and asks which one to use instead of failing (#500). - `a365 develop-mcp grant-agents-access --help` now lists Microsoft's first-party agent blueprint names and IDs, and the same list is printed when `--agent-blueprint-id` is missing or not a GUID, so you can find the ID without looking it up elsewhere (#500). @@ -74,6 +80,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `a365 develop get-token --device-code` — forces device code auth for Microsoft Graph scopes the Windows WAM broker rejects (e.g. Exchange `MailboxSettings.ReadWrite`, `ExchangeMessageTrace.Read.All`). ### Fixed +- `setup all` now completes application permission grants after provisioning a missing resource service principal and lists manual provisioning before dependent permission grants (#485). +- Standalone `setup permissions` commands now list missing service principal provisioning and resource-specific consent before dependent permission grants (#485). +- Successful delegated consent recovery now persists verified grants and removes obsolete provisioning instructions while preserving pending application-role work (#485). +- AI Teammate and blueprint `--authmode both` setup summaries now report both application and delegated permissions without treating pending or unverified consent as granted (#485). - `a365 create-instance` now reports an ambiguous government-cloud environment as a configuration error with guidance to select a specific cloud (#478). - Setup now warns before replacing a stale stored blueprint ID with the sole application matching the configured display name (#478). - Messaging endpoint create and delete overrides now reject non-HTTPS URLs and URLs containing user information, query strings, or fragments (#478). diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/QueryEntraCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/QueryEntraCommand.cs index 6a5b0cb7..e77d7011 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/QueryEntraCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/QueryEntraCommand.cs @@ -670,6 +670,7 @@ private static Command CreateInstanceScopesSubcommand( null or "" => null, AuthenticationConstants.MicrosoftGraphResourceAppId => "Microsoft Graph", ConfigConstants.MessagingBotApiAppId => "Messaging Bot API", + ConfigConstants.DefenderApiAppId => "Defender API", PowerPlatformConstants.PowerPlatformApiResourceAppId => "Power Platform API", "00000002-0000-0000-c000-000000000000" => "Azure Active Directory Graph", "797f4846-ba00-4fd7-ba43-dac1f8f63013" => "Azure Service Management", diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupCommand.cs index 4414c619..9a460e75 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupCommand.cs @@ -52,7 +52,7 @@ public static Command CreateCommand( " a365 setup requirements Validate prerequisites\n" + " a365 setup blueprint Create the agent blueprint (Entra app)\n" + " a365 setup permissions mcp MCP server permissions (when using MCP tools)\n" + - " a365 setup permissions bot Bot API + Power Platform\n" + + " a365 setup permissions bot Bot API + Defender + Power Platform\n" + " a365 setup permissions custom Inline custom resource permission (--resource-app-id, --scopes)\n" + " a365 setup permissions copilotstudio Copilot Studio agent permissions\n\n" + "Roles required:\n" + 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 87f44dac..bbfb1c6a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs @@ -1022,7 +1022,14 @@ await PermissionsSubcommand.RemoveStaleCustomPermissionsAsync( // 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, serverNamesByAudience, - includeObservability: !ctx.SkipObservabilityPermissions); + includeObservability: !ctx.SkipObservabilityPermissions, + defenderPermissionMode: ctx.Results.IsNonDwBlueprintFlow + ? ctx.IsS2sMode + ? DefenderPermissionMode.Application + : ctx.IsBothMode + ? DefenderPermissionMode.Both + : DefenderPermissionMode.Delegated + : DefenderPermissionMode.Both); // Return the full scopesByAudience map alongside the V1-compat mcpScopes so V2 // callers (ApplyConsentUrlsIfNeeded) can route per-server audiences to the bare diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestS2SRunner.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestS2SRunner.cs index 2bc5be21..d0c5ce99 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestS2SRunner.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AzRestS2SRunner.cs @@ -54,7 +54,8 @@ internal static partial class AzRestS2SRunner IReadOnlyList specs, ILogger logger, CancellationToken ct, - string graphBaseUrl = GraphApiConstants.BaseUrl) + string graphBaseUrl = GraphApiConstants.BaseUrl, + IReadOnlyDictionary? resourceSpObjectIds = null) { if (!GuidPattern().IsMatch(blueprintSpObjectId)) { @@ -77,6 +78,12 @@ internal static partial class AzRestS2SRunner logger.LogWarning("az rest S2S runner: spec '{ResourceName}' has invalid ResourceAppId - skipping.", spec.ResourceName); return (false, false); } + if (resourceSpObjectIds?.TryGetValue(spec.ResourceAppId, out var knownSpId) == true + && !GuidPattern().IsMatch(knownSpId)) + { + logger.LogWarning("az rest S2S runner: spec '{ResourceName}' has invalid resource SP id - skipping.", spec.ResourceName); + return (false, false); + } foreach (var role in spec.AppRoleScopes!) { if (!SafeRolePattern().IsMatch(role)) @@ -110,7 +117,9 @@ internal static partial class AzRestS2SRunner ct.ThrowIfCancellationRequested(); try { - var ok = await AssignOneAsync(executor, blueprintSpObjectId, spec, existingAssignments, baseUrl, logger, ct); + string? resourceSpObjectId = null; + resourceSpObjectIds?.TryGetValue(spec.ResourceAppId, out resourceSpObjectId); + var ok = await AssignOneAsync(executor, blueprintSpObjectId, spec, existingAssignments, baseUrl, logger, ct, resourceSpObjectId); if (!ok) allOk = false; } catch (OperationCanceledException) @@ -140,11 +149,15 @@ private static async Task AssignOneAsync( HashSet<(string ResourceId, string AppRoleId)> existingAssignments, string graphBaseUrl, ILogger logger, - CancellationToken ct) + CancellationToken ct, + string? knownResourceSpObjectId) { + var resourcePath = knownResourceSpObjectId is not null + ? $"/v1.0/servicePrincipals/{knownResourceSpObjectId}?$select=id,appRoles" + : $"/v1.0/servicePrincipals?$filter=appId eq '{spec.ResourceAppId}'&$select=id,appRoles"; var spResult = await executor.ExecuteAsync( "az", - $"rest --method GET --url \"{graphBaseUrl}/v1.0/servicePrincipals?$filter=appId eq '{spec.ResourceAppId}'&$select=id,appRoles\"", + $"rest --method GET --url \"{graphBaseUrl}{resourcePath}\"", captureOutput: true, suppressErrorLogging: true, cancellationToken: ct); @@ -320,10 +333,15 @@ internal static (string? SpId, Dictionary AppRolesByValue) TryEx 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]; + if (doc.RootElement.ValueKind != JsonValueKind.Object) return (null, rolesByValue); + var first = doc.RootElement; + if (first.TryGetProperty("value", out var value)) + { + if (value.ValueKind != JsonValueKind.Array || value.GetArrayLength() == 0) + return (null, rolesByValue); + first = value[0]; + } + if (first.ValueKind != JsonValueKind.Object) return (null, rolesByValue); string? spId = first.TryGetProperty("id", out var idEl) && idEl.ValueKind == JsonValueKind.String ? idEl.GetString() : null; 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 c4a23c67..d20f7a47 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs @@ -84,9 +84,11 @@ internal static class BatchPermissionsOrchestrator return (true, true, true, null); } - // Filter out specs with no scopes — they would produce empty OAuth2 grants (HTTP 400). - // This can happen when the MCP manifest is missing or contains no required scopes. - var effectiveSpecs = specs.Where(s => s.Scopes.Length > 0).ToList(); + // Application-only specs must remain so S2S mode can assign app roles without creating + // an empty OAuth2 grant. + var effectiveSpecs = specs + .Where(s => s.Scopes.Length > 0 || s.AppRoleScopes is { Length: > 0 }) + .ToList(); if (setupResults is not null) { setupResults.ObservabilityResourceAppId = effectiveSpecs @@ -97,12 +99,12 @@ internal static class BatchPermissionsOrchestrator if (effectiveSpecs.Count < specs.Count) { var skipped = specs.Count - effectiveSpecs.Count; - logger.LogDebug("Skipping {Count} resource spec(s) with no scopes (manifest missing or empty).", skipped); + logger.LogDebug("Skipping {Count} resource spec(s) with no delegated scopes or application roles.", skipped); } if (effectiveSpecs.Count == 0) { - logger.LogInformation("All permission specs have empty scope lists — skipping batch permissions configuration."); + logger.LogInformation("All permission specs have empty delegated scope and application role lists — skipping batch permissions configuration."); return (true, true, true, null); } @@ -135,6 +137,27 @@ internal static class BatchPermissionsOrchestrator : Models.RoleCheckResult.DoesNotHaveRole; var isGlobalAdmin = adminCheck == Models.RoleCheckResult.HasRole; + // Recover missing resource service principals before inheritance and app-role grants. + // Successful az provisioning updates both the app-id set and object-id map so every + // downstream phase can use the newly created resource in this same run. + if (phase1Result != null) + { + var resolvedSpObjectIds = phase1Result.ResourceSpObjectIds + .ToDictionary(pair => pair.Key, pair => pair.Value, StringComparer.OrdinalIgnoreCase); + var resolvedSpAppIds = resolvedSpObjectIds.Keys.ToHashSet(StringComparer.OrdinalIgnoreCase); + var missingSpecs = FindMissingResourceSpSpecs(specs, resolvedSpAppIds); + await EnsureMissingResourceSpsAsync( + graph, tenantId, blueprintAppId, missingSpecs, resolvedSpAppIds, permScopes, + skipSpProvisioning, logger, setupResults, ct, + commandExecutor: commandExecutor, + confirmationProvider: confirmationProvider, + knownMcpAudienceAppIds: knownMcpAudienceAppIds, + resolvedSpObjectIds: resolvedSpObjectIds); + phase1Result = new BlueprintPermissionsResult( + phase1Result.BlueprintSpObjectId, + resolvedSpObjectIds); + } + // --- Phase 2a: Inheritable permissions (Agent ID Admin or GA) --- // --- Phase 2b: OAuth2 grants (Global Administrator only) --- logger.LogInformation("Configuring inheritable permissions..."); @@ -234,7 +257,7 @@ internal static class BatchPermissionsOrchestrator } else { - await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result!.BlueprintSpObjectId, specs, s2sScopes, logger, setupResults, ct); + await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result!.BlueprintSpObjectId, phase1Result.ResourceSpObjectIds, 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 @@ -248,7 +271,8 @@ internal static class BatchPermissionsOrchestrator { 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, graph.GraphBaseUrl); + commandExecutor, phase1Result.BlueprintSpObjectId, specs, logger, ct, graph.GraphBaseUrl, + phase1Result.ResourceSpObjectIds); if (attempted && succeeded) { logger.LogInformation("Application permissions granted."); @@ -280,7 +304,7 @@ internal static class BatchPermissionsOrchestrator // --- Admin consent --- var (consentGranted, consentUrl) = await GrantAdminConsentAsync( - graph, config, blueprintAppId, tenantId, specs, phase1Result, permScopes, logger, setupResults, ct, commandExecutor, adminCheck, confirmationProvider, skipSpProvisioning, knownMcpAudienceAppIds); + graph, config, blueprintAppId, tenantId, specs, phase1Result, permScopes, logger, setupResults, ct, commandExecutor, adminCheck, confirmationProvider, 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 @@ -415,12 +439,16 @@ private static async Task UpdateBlueprintPermissions continue; } + var permissionValues = spec.Scopes + .Concat(spec.AppRoleScopes ?? Array.Empty()) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToArray(); logger.LogDebug( - " - Configuring inheritable permissions: {ResourceName} [{Scopes}]", - spec.ResourceName, string.Join(' ', spec.Scopes)); + " - Configuring inheritable permissions: {ResourceName} [{Permissions}]", + spec.ResourceName, string.Join(' ', permissionValues)); var (ok, alreadyExists, err) = await blueprintService.SetInheritablePermissionsAsync( - tenantId, blueprintAppId, spec.ResourceAppId, spec.Scopes, + tenantId, blueprintAppId, spec.ResourceAppId, permissionValues, requiredScopes: permScopes, ct); if (alreadyExists || ok) @@ -502,6 +530,7 @@ private static async Task PerformS2SGrantsAsync( AgentBlueprintService blueprintService, string tenantId, string blueprintSpObjectId, + IReadOnlyDictionary resourceSpObjectIds, IEnumerable specs, string[] s2sScopes, ILogger logger, @@ -529,13 +558,15 @@ private static async Task PerformS2SGrantsAsync( "App role assignment: blueprint -> {ResourceName} [{AppRoles}]", spec.ResourceName, string.Join(' ', spec.AppRoleScopes!)); + resourceSpObjectIds.TryGetValue(spec.ResourceAppId, out var resourceSpObjectId); var grantResult = await blueprintService.GrantAppRoleAssignmentAsync( tenantId, blueprintSpObjectId, spec.ResourceAppId, spec.AppRoleScopes!, requiredScopes: s2sScopes, - ct: ct); + ct: ct, + knownResourceSpObjectId: resourceSpObjectId); if (grantResult.AllSucceeded) { @@ -604,7 +635,6 @@ private static void SetPendingBlueprintAppRoleSpecs(SetupResults setupResults, I CommandExecutor? commandExecutor = null, 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 @@ -658,36 +688,27 @@ private static void SetPendingBlueprintAppRoleSpecs(SetupResults setupResults, I ? 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 + var specsForUrl = phase1Result != null ? specs.Where(s => resolvedSpAppIds.Contains(s.ResourceAppId)).ToList() : specs.ToList(); + var hasUnresolvedDelegatedSpecs = HasUnresolvedDelegatedSpecs(specs, resolvedSpAppIds); + var consentRequirements = specsForUrl + .Where(spec => spec.Scopes is { Length: > 0 }) + .Select(spec => + { + string? resourceSpId = null; + phase1Result?.ResourceSpObjectIds.TryGetValue(spec.ResourceAppId, out resourceSpId); + return new AdminConsentRequirement( + spec.ResourceName, + spec.ResourceAppId, + spec.Scopes, + resourceSpId); + }) + .ToList(); var sharedMcpResourceAppId = ConfigConstants.GetAgent365ToolsResourceAppId(config.Environment); var allScopes = specsForUrl @@ -708,7 +729,7 @@ await EnsureMissingResourceSpsAsync( // the Action Required block from setupResults if S2S work remains. if (consentUrl == null) { - return (true, null); + return (granted: !hasUnresolvedDelegatedSpecs, consentUrl: null); } // Section header — mirrors PerformS2SGrantsAsync's "Configuring S2S app role assignments..." @@ -788,7 +809,7 @@ await EnsureMissingResourceSpsAsync( } } - if (allConsented) + if (allConsented && !hasUnresolvedDelegatedSpecs) { using (logger.Indent()) logger.LogInformation("Delegated admin consent already granted for all required scopes"); @@ -796,6 +817,8 @@ await EnsureMissingResourceSpsAsync( setupResults.TenantWideConsentAlreadyExisted = true; return (true, null); } + if (allConsented) + return (false, consentUrl); } } @@ -840,7 +863,8 @@ await EnsureMissingResourceSpsAsync( var found = await AdminConsentHelper.PollAdminConsentAsync( commandExecutor, logger, blueprintAppId, "All permissions", timeoutSeconds: 180, intervalSeconds: 5, ct, - graphBaseUrl: graph.GraphBaseUrl); + graphBaseUrl: graph.GraphBaseUrl, + requiredGrants: consentRequirements); consentVerified = found; // Browser was opened regardless — either the grant was directly observed (Verified) // or the timeout elapsed without observing it (AssumedComplete). Either way, setup @@ -855,7 +879,8 @@ await EnsureMissingResourceSpsAsync( var pollResult = await AdminConsentHelper.PollAdminConsentAsync( graph, logger, tenantId, phase1Result.BlueprintSpObjectId, "All permissions", timeoutSeconds: 180, intervalSeconds: 5, ct, - permScopes: AuthenticationConstants.BlueprintOperationScopes); + permScopes: AuthenticationConstants.BlueprintOperationScopes, + requiredGrants: consentRequirements); consentVerified = pollResult == ConsentPollResult.Verified; consentGranted = pollResult != ConsentPollResult.NotDetected; } @@ -921,12 +946,15 @@ await EnsureMissingResourceSpsAsync( 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; + var consentedResourceAppIds = originalSpecs + .Where(spec => spec.Scopes is { Length: > 0 }) + .Select(spec => spec.ResourceAppId) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + setupResults.MissingSpActions.RemoveAll(action => consentedResourceAppIds.Contains(action.ResourceAppId)); } + // All-resource fallback success supersedes the earlier SP-resolution snapshot. + hasUnresolvedDelegatedSpecs = false; consentGranted = true; consentVerified = true; } @@ -942,9 +970,28 @@ await EnsureMissingResourceSpsAsync( // 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. + if (hasUnresolvedDelegatedSpecs) + return (false, consentUrl); + return (consentGranted, consentVerified ? null : consentUrl); } + internal static List FindMissingResourceSpSpecs( + IReadOnlyList specs, + IReadOnlySet resolvedSpAppIds) => + specs + .Where(spec => + (spec.Scopes is { Length: > 0 } || spec.AppRoleScopes is { Length: > 0 }) + && !resolvedSpAppIds.Contains(spec.ResourceAppId)) + .ToList(); + + internal static bool HasUnresolvedDelegatedSpecs( + IReadOnlyList specs, + IReadOnlySet resolvedSpAppIds) => + specs.Any(spec => + spec.Scopes is { Length: > 0 } + && !resolvedSpAppIds.Contains(spec.ResourceAppId)); + /// /// Updates config.ResourceConsents in-memory for each spec based on phase results. /// The caller is responsible for persisting the config via configService.SaveStateAsync. @@ -954,7 +1001,10 @@ internal static void UpdateResourceConsents( IReadOnlyList specs, Dictionary inheritedResults) { - var configuredObservabilityAppIds = specs + var delegatedSpecs = specs + .Where(spec => spec.Scopes.Length > 0) + .ToList(); + var configuredObservabilityAppIds = delegatedSpecs .Where(spec => ConfigConstants.IsObservabilityApiAppId(spec.ResourceAppId)) .Select(spec => spec.ResourceAppId) .ToHashSet(StringComparer.OrdinalIgnoreCase); @@ -966,7 +1016,7 @@ internal static void UpdateResourceConsents( !configuredObservabilityAppIds.Contains(resourceConsent.ResourceAppId)); } - foreach (var spec in specs) + foreach (var spec in delegatedSpecs) { inheritedResults.TryGetValue(spec.ResourceAppId, out var inherited); @@ -1062,7 +1112,8 @@ internal static async Task EnsureMissingResourceSpsAsync( CancellationToken ct, CommandExecutor? commandExecutor = null, IConfirmationProvider? confirmationProvider = null, - IReadOnlyCollection? knownMcpAudienceAppIds = null) + IReadOnlyCollection? knownMcpAudienceAppIds = null, + IDictionary? resolvedSpObjectIds = null) { if (missingSpecs.Count == 0) return; @@ -1084,6 +1135,8 @@ internal static async Task EnsureMissingResourceSpsAsync( "Resource '{Name}' ({AppId}): service principal found in tenant — no provisioning needed.", spec.ResourceName, spec.ResourceAppId); resolvedSpAppIds.Add(spec.ResourceAppId); + if (resolvedSpObjectIds is not null) + resolvedSpObjectIds[spec.ResourceAppId] = spId; } else { @@ -1200,6 +1253,8 @@ internal static async Task EnsureMissingResourceSpsAsync( { logger.LogInformation("Done. Service principal created for '{Name}' (id: {SpId}).", spec.ResourceName, newSpId); resolvedSpAppIds.Add(spec.ResourceAppId); + if (resolvedSpObjectIds is not null) + resolvedSpObjectIds[spec.ResourceAppId] = newSpId; } else { @@ -1270,11 +1325,11 @@ internal static string BuildAzAdSpCreateCommand(string 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 + /// carries the 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 + /// For delegated scopes only, a 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. @@ -1299,14 +1354,19 @@ private static void RecordMissingSpAction( var azCommand = BuildAzAdSpCreateCommand(spec.ResourceAppId); var isMcpAudience = knownMcpAudienceAppIds?.Contains(spec.ResourceAppId) ?? false; - var perSpConsentUrl = BuildPerSpBlueprintConsentUrl(tenantId, blueprintAppId, spec, isMcpAudience, authorityHost); + var perSpConsentUrl = spec.Scopes is { Length: > 0 } + ? BuildPerSpBlueprintConsentUrl(tenantId, blueprintAppId, spec, isMcpAudience, authorityHost) + : null; setupResults?.MissingSpActions.Add(new MissingSpAction( ResourceName: spec.ResourceName, ResourceAppId: spec.ResourceAppId, Scopes: spec.Scopes?.ToArray() ?? Array.Empty(), AzCreateCommand: azCommand, - PerSpConsentUrl: perSpConsentUrl)); + PerSpConsentUrl: perSpConsentUrl) + { + AppRoleScopes = spec.AppRoleScopes?.ToArray() ?? Array.Empty(), + }); } /// @@ -1577,7 +1637,7 @@ private record BlueprintPermissionsResult( // S2S: Grant app role assignments for specs that carry AppRoleScopes. var s2sScopes = permScopes.Concat(AuthenticationConstants.RequiredS2SGrantScopes).ToArray(); if (blueprintService is not null && !string.IsNullOrWhiteSpace(phase1Result.BlueprintSpObjectId)) - await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result.BlueprintSpObjectId, specs, s2sScopes, logger, setupResults, ct); + await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result.BlueprintSpObjectId, phase1Result.ResourceSpObjectIds, specs, s2sScopes, logger, setupResults, ct); // else: blueprint service unavailable or SP not resolved — leave BlueprintS2SOutcome = NotApplicable (not attempted) return (allGrantsOk, phase1Result.BlueprintSpObjectId); 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 e5c52d01..7d6a1625 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs @@ -20,7 +20,7 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; /// 1. Requirements validation /// 2. Blueprint creation (shared with DW) /// 3. Batch permissions on the blueprint (shared with DW pipeline; non-DW spec set: -/// Power Platform API and custom; Observability API is not requested). MAC reads +/// Defender API, Power Platform API, custom, and optionally Observability API). MAC reads /// from the blueprint, so stamping here gives the same set visibility there. /// 4. Agent Identity creation via POST /beta/servicePrincipals/Microsoft.Graph.AgentIdentity /// 5. Agent Identity permission grants (same spec set as step 3) — OBO or S2S @@ -36,6 +36,16 @@ internal static class NonDwBlueprintSetupOrchestrator public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool isBootstrap = false, string[]? rawArgs = null, bool skipRequirements = false, bool isM365 = false, bool agentRegistrationOnly = false, string? authMode = null, string? messagingEndpointOverride = null, bool skipObservabilityPermissions = false) { var sub = new string(' ', SetupHelpers.DryRunValCol); + var selectedAuthMode = authMode ?? config.AuthMode; + var effectiveMode = string.IsNullOrWhiteSpace(selectedAuthMode) + ? "obo" + : selectedAuthMode.Trim().ToLowerInvariant(); + var defenderPermissionMode = effectiveMode switch + { + "s2s" => DefenderPermissionMode.Application, + "both" => DefenderPermissionMode.Both, + _ => DefenderPermissionMode.Delegated, + }; var observabilityPermissionsEffectivelySkipped = skipObservabilityPermissions && !SetupHelpers.CustomPermissionsRequestObservability(config); // Dry-run S2S work comes only from fixed specs today; MCP and custom specs carry delegated scopes. @@ -43,7 +53,8 @@ public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool i setInheritable: true, isM365, config.Environment, - includeObservability: !skipObservabilityPermissions) + includeObservability: !skipObservabilityPermissions, + defenderPermissionMode) .Any(s => s.AppRoleScopes is { Length: > 0 }); // --messaging-endpoint flag (if supplied) wins over the init-only config value for the plan. var plannedEndpoint = !string.IsNullOrWhiteSpace(messagingEndpointOverride) @@ -126,20 +137,17 @@ public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool i logger.LogInformation(sub + "create managed identity"); } - // 3. Inheritable Permissions — non-DW spec set (Power Platform API and custom; Observability API is - // not requested) stamped on the blueprint via SetInheritablePermissionsAsync so MAC and other - // dependent systems can see them. The same set is applied to the agent identity SP in step 5. - var selectedAuthMode = authMode ?? config.AuthMode; - var effectiveMode = string.IsNullOrWhiteSpace(selectedAuthMode) - ? "obo" - : selectedAuthMode.Trim().ToLowerInvariant(); + // 3. Inheritable Permissions — the non-DW spec set is stamped on the blueprint so MAC and + // dependent systems can see it. The same set is applied to the agent identity SP in step 5. logger.LogInformation(SetupHelpers.DryRunRow(3, "Inheritable Permissions") + "configure for {Resources} (Global Administrator required; consent URL printed if absent)", - skipObservabilityPermissions ? "Power Platform API and custom permissions" : "Observability API, Power Platform API, and custom permissions"); + skipObservabilityPermissions + ? "Defender API, Power Platform API, and custom permissions" + : "Observability API, Defender API, Power Platform API, and custom permissions"); if (observabilityPermissionsEffectivelySkipped) logger.LogInformation(sub + "Observability API not requested (registered agents export telemetry with an app-only token)"); - // 4. Blueprint Permission Grants — per authMode. The consent URL targets the blueprint - // app, and S2S app-role assignments are persisted as grants flowing from the blueprint; + // 4. Blueprint Permission Grants — Defender follows authMode; other permission specs retain + // their configured grant types. // grouping here keeps all blueprint-side rows (2 Blueprint, 3 Inheritable Permissions, // 4 Blueprint Permission Grants) contiguous. if (effectiveMode is "obo") @@ -282,6 +290,11 @@ await ctx.ClientAppValidator.GrantConsentForPermissionsAsync( public static async Task ExecuteAsync(SetupContext ctx) { ctx.Results.IsNonDwBlueprintFlow = true; + ctx.Results.EffectiveAuthMode = ctx.IsBothMode + ? Models.AuthMode.Both + : ctx.IsS2sMode + ? Models.AuthMode.S2s + : Models.AuthMode.Obo; ctx.Results.ObservabilityPermissionsSkipped = ctx.ObservabilityPermissionsEffectivelySkipped; ctx.Results.TenantId = ctx.Config.TenantId; // Bootstrap already printed the "Running..." banner before auth steps; skip here to avoid duplication. @@ -465,9 +478,14 @@ internal static async Task ExecuteAgentIdentityAndRegistrationAsync( // Skipped when --agent-registration-only: identity result flags are pre-set by the caller. if (!skipIdentityAndPermissions) { - // Record the auth mode and whether any S2S app role is requested before identity creation, - // so the summary stays accurate when the identity step fails. - ctx.Results.EffectiveAuthMode = ctx.IsBothMode ? Models.AuthMode.Both : ctx.IsS2sMode ? Models.AuthMode.S2s : Models.AuthMode.Obo; + // Keep direct callers of this phase aligned with the top-level orchestrator. + ctx.Results.EffectiveAuthMode = ctx.IsBothMode + ? Models.AuthMode.Both + : ctx.IsS2sMode + ? Models.AuthMode.S2s + : Models.AuthMode.Obo; + // Record whether the selected auth mode produced application permissions before + // identity creation so the summary stays accurate when that step fails. if (ctx.IsS2sMode || ctx.IsBothMode) ctx.Results.NoS2SAppRolesToGrant = !specs.Any(s => s.AppRoleScopes is { Length: > 0 }); @@ -722,8 +740,8 @@ internal static async Task GrantAgentIdentityS2SPermissionsAsync( List specs) { var hasS2sSpecs = specs.Any(s => s.AppRoleScopes is { Length: > 0 }); - // Blueprint agents no longer request OtelWrite, the only app role setup requested, so this - // step usually has nothing to grant. Record that so the summary does not report a delegated grant. + // Record whether the selected auth mode produced any application permissions so the + // summary can distinguish "no S2S work" from a failed grant. ctx.Results.NoS2SAppRolesToGrant = !hasS2sSpecs; if (hasS2sSpecs && AgentIdentityInheritsBlueprintAppRoles(ctx.Results)) { 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 98496fe1..1346260b 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs @@ -254,7 +254,7 @@ private static Command CreateBotSubcommand( IBootstrapConfigResolver? resolver = null) { var command = new Command("bot", - "Configure Messaging Bot API OAuth2 grants and inheritable permissions\n" + + "Configure Messaging Bot, Defender, and Power Platform API grants and inheritable permissions\n" + "Required role: Agent ID Developer; Global Administrator for tenant-wide OAuth2 consent\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" + @@ -296,7 +296,7 @@ private static Command CreateBotSubcommand( if (dryRunConfig is null) { logger.LogInformation("Dry run: a365 setup permissions bot --dry-run"); - logger.LogInformation(" Would configure Messaging Bot API OAuth2 grants and inheritable permissions."); + logger.LogInformation(" Would configure Messaging Bot, Defender, and Power Platform API grants and inheritable permissions."); logger.LogInformation("No changes made. Run without --dry-run to execute."); return; } @@ -305,6 +305,7 @@ private static Command CreateBotSubcommand( logger.LogInformation("Would configure Bot API permissions:"); logger.LogInformation(" - Blueprint: {BlueprintId}", dryRunConfig.AgentBlueprintId); logger.LogInformation(" - Messaging Bot API: {Scope}", ConfigConstants.MessagingBotApiAdminConsentScope); + logger.LogInformation(" - Defender API: {DefenderScope} (delegated + application)", ConfigConstants.DefenderApiRealtimeProtectionScope); logger.LogInformation(" - Power Platform API: Connectivity.Connections.Read"); logger.LogInformation("No changes made. Run without --dry-run to execute."); return; @@ -787,22 +788,32 @@ public static async Task ConfigureBotPermissionsAsync( await configService.SaveStateAsync(setupConfig); + // Defender still requires an application grant even when Observability permissions are skipped. + var s2sFailed = localResults.BlueprintS2SOutcome != Models.GrantOutcome.Granted; + logger.LogInformation(""); - if (consentGranted) + if (!s2sFailed && consentGranted) { logger.LogInformation("Bot API permissions configured successfully"); } - else + else if (!consentGranted) { logger.LogInformation("Bot API permissions configured; admin consent required"); LogAdminConsentNextSteps(logger, adminConsentUrl, localResults, specs); } + else + { + logger.LogWarning( + "Bot API permissions configured, but S2S app role assignment failed. " + + "Re-run 'a365 setup permissions bot' as {Roles} to retry.", + AuthenticationConstants.S2SGrantRequiredRoles); + } logger.LogInformation(""); if (!iSetupAll) { logger.LogInformation("Next step: Run 'a365 publish' to package your agent for upload to the Microsoft 365 Admin Center."); } - return consentGranted; + return consentGranted && !s2sFailed; } catch (Exception ex) { @@ -836,6 +847,7 @@ internal static async Task RemoveStaleCustomPermissionsAsync( envAtgAppId, ConfigConstants.MessagingBotApiAppId, observabilityAppId, + ConfigConstants.DefenderApiAppId, PowerPlatformConstants.PowerPlatformApiResourceAppId, AuthenticationConstants.MicrosoftGraphResourceAppId, }; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md index b83c08c6..1710b09f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md @@ -94,7 +94,7 @@ a365 setup all --authmode both ### Observability permissions -`setup all` does not request `Agent365.Observability.OtelWrite` for blueprint agents (in any auth mode) or AI Teammates, and `setup permissions bot` and `create-instance` do not request it for any agent. Agents export telemetry with an app-only token through the S2S endpoint, which authorizes a blueprint agent by its agent registration and an AI Teammate by its blueprint's registration, so no Observability admin consent is needed. For blueprint agents, registration is then the agent's only authorization, so a registration failure is reported as an error (exit code 1). OtelWrite was the only app role setup requested. Unless another permission adds an app role, `--authmode s2s` and `both` have nothing to assign for blueprint agents, and the setup summary reports the S2S grant as not required. +`setup all` does not request `Agent365.Observability.OtelWrite` for blueprint agents (in any auth mode) or AI Teammates, and `setup permissions bot` and `create-instance` do not request it for any agent. Agents export telemetry with an app-only token through the S2S endpoint, which authorizes a blueprint agent by its agent registration and an AI Teammate by its blueprint's registration, so no Observability admin consent is needed. For blueprint agents, registration is then the agent's only authorization, so a registration failure is reported as an error (exit code 1). Defender still contributes the `RealtimeProtection.Evaluate.All` application role in `--authmode s2s` and `both`, so those modes perform an S2S grant even when Observability permissions are omitted. Agents whose SDK still exports through the delegated (OBO) route need `OtelWrite`, as do agents whose app-only exports return 403 `insufficient_scope`; grant it manually (see the CHANGELOG upgrade note). Re-running setup does not revoke permissions granted earlier. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/ResourcePermissionSpec.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/ResourcePermissionSpec.cs index 5ca5d19e..1c363c4c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/ResourcePermissionSpec.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/ResourcePermissionSpec.cs @@ -3,6 +3,13 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; +internal enum DefenderPermissionMode +{ + Delegated, + Application, + Both, +} + /// /// Describes a single resource whose permissions should be configured on the agent blueprint. /// Used as input to . 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 cb05e47e..6cf20213 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs @@ -86,8 +86,8 @@ internal sealed class SetupContext /// (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. + /// so the setup summary's Action Required block renders them as numbered items with the + /// az ad sp create command and, for delegated specs, a per-SP consent URL. /// Set explicitly via --skip-sp-provisioning or implicitly when stdin is /// redirected (CI / coding-agent / pipe scenarios). /// @@ -179,7 +179,7 @@ public SetupContext( AgentInstanceOnly = agentInstanceOnly; IsBootstrap = isBootstrap; IsM365 = isM365; - AuthMode = string.IsNullOrWhiteSpace(authMode) ? null : authMode.ToLowerInvariant(); + AuthMode = string.IsNullOrWhiteSpace(authMode) ? null : authMode.Trim().ToLowerInvariant(); MessagingEndpointOverride = string.IsNullOrWhiteSpace(messagingEndpointOverride) ? null : messagingEndpointOverride.Trim(); SkipSpProvisioning = skipSpProvisioning; NonInteractive = nonInteractive; 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 f1894afe..135e64ac 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -49,18 +49,18 @@ internal static void PrintDryRunBlueprintReuseRows(ILogger logger, string bluepr /// Returns the fixed-scope ResourcePermissionSpecs for the platform APIs that every /// agent blueprint requires. /// - /// Power Platform API is always included. Observability API, using the app ID for + /// Defender API and Power Platform API are always included. Observability API, using the app ID for /// 's cloud, is included unless - /// is false. Messaging Bot API is - /// included only when is true — non-M365 (blueprint-only) agents - /// have no messaging surface so Bot scopes serve no purpose. + /// is false. Messaging Bot API is included only when is true — + /// non-M365 (blueprint-only) agents have no messaging surface so Bot scopes serve no purpose. /// /// internal static ResourcePermissionSpec[] GetFixedApiPermissionSpecs( bool setInheritable, bool isM365, string? environment = null, - bool includeObservability = true) + bool includeObservability = true, + DefenderPermissionMode defenderPermissionMode = DefenderPermissionMode.Both) { var specs = new List(); if (isM365) @@ -88,6 +88,18 @@ internal static ResourcePermissionSpec[] GetFixedApiPermissionSpecs( setInheritable, AppRoleScopes: new[] { ConfigConstants.ObservabilityApiOtelWriteScope })); } + var defenderDelegatedScopes = defenderPermissionMode is DefenderPermissionMode.Delegated or DefenderPermissionMode.Both + ? new[] { ConfigConstants.DefenderApiRealtimeProtectionScope } + : Array.Empty(); + var defenderAppRoleScopes = defenderPermissionMode is DefenderPermissionMode.Application or DefenderPermissionMode.Both + ? new[] { ConfigConstants.DefenderApiRealtimeProtectionScope } + : null; + specs.Add(new ResourcePermissionSpec( + ConfigConstants.DefenderApiAppId, + "Defender API", + defenderDelegatedScopes, + setInheritable, + AppRoleScopes: defenderAppRoleScopes)); specs.Add(new ResourcePermissionSpec( PowerPlatformConstants.PowerPlatformApiResourceAppId, "Power Platform API", @@ -147,7 +159,8 @@ internal static async Task> BuildConfiguredPermissi bool isM365 = true, Dictionary? scopesByAudience = null, Dictionary>? serverNamesByAudience = null, - bool includeObservability = true) + bool includeObservability = true, + DefenderPermissionMode defenderPermissionMode = DefenderPermissionMode.Both) { // Manifest read at most once, and only when scopesByAudience is not pre-supplied. // Callers that already have the manifest loaded (e.g. AllSubcommand.BuildPermissionSpecsAsync) @@ -186,7 +199,8 @@ internal static async Task> BuildConfiguredPermissi : "Agent 365 Tools", kvp.Value, SetInheritable: setInheritable))); - specs.AddRange(GetFixedApiPermissionSpecs(setInheritable, isM365, config.Environment, includeObservability)); + specs.AddRange(GetFixedApiPermissionSpecs( + setInheritable, isM365, config.Environment, includeObservability, defenderPermissionMode)); foreach (var customPerm in config.CustomBlueprintPermissions ?? new List()) { @@ -475,8 +489,8 @@ internal static async Task ResolveBootstrapEnvironmentAsync( /// /// Fixed permission specs for the non-DW admin consent flow. - /// Observability API requires both Application (app role for S2S) and Delegated (oauth2 grant for OBO). - /// Power Platform API requires Delegated only. + /// Observability API includes both permission types when requested. Defender follows the + /// selected auth mode. Power Platform API requires Delegated only. /// Extend this list or pass an override to /// when additional APIs are required (e.g. dynamic MCP scopes, custom permissions). /// @@ -484,25 +498,37 @@ internal static async Task ResolveBootstrapEnvironmentAsync( GetNonDwAdminConsentSpecs("prod"); internal static IReadOnlyList<(string ResourceName, string ResourceAppId, string Scope, string PermissionType)> GetNonDwAdminConsentSpecs( - string? environment) - => BuildNonDwAdminConsentSpecs(ConfigConstants.GetObservabilityApiAppId(environment)); + string? environment, + DefenderPermissionMode defenderPermissionMode = DefenderPermissionMode.Both) + => BuildNonDwAdminConsentSpecs( + ConfigConstants.GetObservabilityApiAppId(environment), + defenderPermissionMode); private static IReadOnlyList<(string ResourceName, string ResourceAppId, string Scope, string PermissionType)> BuildNonDwAdminConsentSpecs( - string observabilityAppId) + string observabilityAppId, + DefenderPermissionMode defenderPermissionMode = DefenderPermissionMode.Both) { - return - [ + var specs = new List<(string ResourceName, string ResourceAppId, string Scope, string PermissionType)> + { ("Observability API", observabilityAppId, ConfigConstants.ObservabilityApiOtelWriteScope, "Application"), ("Observability API", observabilityAppId, ConfigConstants.ObservabilityApiOtelWriteScope, "Delegated"), ("Power Platform API", PowerPlatformConstants.PowerPlatformApiResourceAppId, PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead, "Delegated"), - ]; + }; + + if (defenderPermissionMode is DefenderPermissionMode.Application or DefenderPermissionMode.Both) + specs.Insert(2, ("Defender API", ConfigConstants.DefenderApiAppId, ConfigConstants.DefenderApiRealtimeProtectionScope, "Application")); + if (defenderPermissionMode is DefenderPermissionMode.Delegated or DefenderPermissionMode.Both) + specs.Insert(defenderPermissionMode == DefenderPermissionMode.Both ? 3 : 2, + ("Defender API", ConfigConstants.DefenderApiAppId, ConfigConstants.DefenderApiRealtimeProtectionScope, "Delegated")); + + return specs; } /// /// Logs step-by-step instructions for a Global Administrator to grant admin consent /// for the blueprint app, with two options: Entra portal and PowerShell. /// - /// Defaults to (Observability API + Power Platform API). + /// Defaults to (Observability, Defender, and Power Platform APIs). /// Pass an explicit list to support dynamic or extended permission sets. /// /// @@ -640,6 +666,7 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str // Delegated success: either tenant-wide consent (DW or non-DW blueprint inheritable scopes) // or principal-scoped grants on the agent identity (non-DW path). var delegatedOk = tenantConsentGranted || tenantConsentUnverified || agentIdDelegatedGranted; + var reportTenantWideDelegatedConsent = !isNonDw || (isBothMode && !agentIdDelegatedGranted); var permissionGrantsCompleted = isS2SFlow ? s2sOk && (!isBothMode || delegatedOk) @@ -663,9 +690,8 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str // (e.g. admin already granted tenant consent but the per-principal call still failed). var pendingDelegatedAction = agentIdDelegatedFailed && !pendingAdminAction; var pendingS2SAction = permissionGrantsPending && isS2SFlow; - // Blueprint agents no longer request OtelWrite, the only app role setup requested, so an - // s2s/both run usually has no S2S grant at all. Say so explicitly: otherwise the row falls - // through to the delegated wording, which for s2s-only shows a PENDING with no action item. + // Some configurations can have no application-role specs. Say so explicitly; otherwise + // an s2s-only row can fall through to delegated wording with no matching action item. var noS2SAppRolesToGrant = isNonDw && (isS2sOnlyMode || isBothMode) && results.NoS2SAppRolesToGrant && !isS2SFlow; if (results.PermissionGrantsSkipped && isNonDw) @@ -757,35 +783,21 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str logger.LogInformation(DryRunRow(permGrantStep, "Blueprint Permission Grants") + "skipped (--agent-registration-only)"); else if (noS2SAppRolesToGrant && isS2sOnlyMode) logger.LogInformation(DryRunRow(permGrantStep, "Blueprint Permission Grants") + "not required (no S2S app roles to grant)"); - else if (isS2SFlow && s2sOk) + else if (isS2SFlow && s2sOk && (!reportTenantWideDelegatedConsent || tenantConsentGranted)) { - if (isBothMode && !delegatedOk) - logger.LogInformation(DryRunRow(permGrantStep, "Blueprint Permission Grants") + - (pendingDelegatedAction - ? "partial (S2S granted; delegated — see Action Required)" - : "partial (S2S granted; delegated — see warnings)")); - else - { - // "already granted" applies when this run made no changes — every S2S role was - // already in place. For "both" mode, also require that the per-principal OBO grant - // on the agent identity SP pre-existed; otherwise the row would say "already granted" - // when the delegated half of the bothMode result was actually written in this run. - // Note: AgentIdentityDelegatedAlreadyExisted has no writer in production today (the - // non-DW OBO grant path is satisfied by blueprint inheritance + tenant-wide consent, - // not a per-principal POST). The "(!isBothMode || !agentIdDelegatedGranted || ...)" - // shape stays defensive for when that design changes — the previous code used - // TenantWideConsentAlreadyExisted as a proxy, which was wrong because tenant-wide - // consent and per-principal OBO are different scopes. - var s2sVerb = results.BlueprintS2SAlreadyAssigned - && (!isBothMode || !agentIdDelegatedGranted || results.AgentIdentityDelegatedAlreadyExisted) - ? "already granted" - : "granted"; - var oboAlso = isBothMode && agentIdDelegatedGranted; - var label = oboAlso + var s2sVerb = results.BlueprintS2SAlreadyAssigned + && (reportTenantWideDelegatedConsent + ? results.TenantWideConsentAlreadyExisted + : !isBothMode || !agentIdDelegatedGranted || results.AgentIdentityDelegatedAlreadyExisted) + ? "already granted" + : "granted"; + var oboAlso = isBothMode && agentIdDelegatedGranted; + var label = reportTenantWideDelegatedConsent + ? $"{s2sVerb} S2S app roles + tenant-wide delegated" + : oboAlso ? $"{s2sVerb} S2S app roles + developer-scoped delegated on agent identity" : $"{s2sVerb} S2S app roles"; - logger.LogInformation(DryRunRow(permGrantStep, "Blueprint Permission Grants") + label); - } + logger.LogInformation(DryRunRow(permGrantStep, "Blueprint Permission Grants") + label); } else if (agentIdDelegatedGranted) logger.LogInformation(DryRunRow(permGrantStep, "Blueprint Permission Grants") + "granted developer-scoped delegated on agent identity"); @@ -934,7 +946,7 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str if (hasActionRequired) { var blueprintAppId = results.BlueprintId ?? ""; - var consentUrl = results.CombinedConsentUrl ?? results.AdminConsentUrl; + var consentUrl = results.AdminConsentUrl ?? results.CombinedConsentUrl; logger.LogInformation(""); logger.LogInformation("Action Required:"); @@ -946,6 +958,7 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str logger.LogInformation(" Add it to a365.generated.config.json as 'agentBlueprintClientSecret', then re-run setup."); logger.LogInformation(" See: https://learn.microsoft.com/en-us/entra/identity-platform/how-to-add-credentials"); } + LogMissingSpActions(logger, results.MissingSpActions, ref actionCount); if (pendingAdminAction) { actionCount++; @@ -957,7 +970,15 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str if (isNonDw && string.IsNullOrWhiteSpace(consentUrl)) { logger.LogInformation(" {N}. Permission Grants — must be granted by {Roles} in the Entra portal:", actionCount, AuthenticationConstants.DelegatedGrantRequiredRoles); - var consentSpecs = BuildNonDwAdminConsentSpecs(observabilityResourceAppId); + var defenderPermissionMode = results.EffectiveAuthMode switch + { + Models.AuthMode.S2s => DefenderPermissionMode.Application, + Models.AuthMode.Both => DefenderPermissionMode.Both, + _ => DefenderPermissionMode.Delegated, + }; + var consentSpecs = BuildNonDwAdminConsentSpecs( + observabilityResourceAppId, + defenderPermissionMode); if (results.ObservabilityPermissionsSkipped) consentSpecs = consentSpecs.Where(s => !ConfigConstants.IsObservabilityApiAppId(s.ResourceAppId)).ToList(); LogNonDwAdminConsentInstructions( @@ -997,7 +1018,6 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str s2sTargets.Add((true, results.PendingAgentIdentityAppRoleSpecs.Where(spec => spec.AppRoleScopes is { Length: > 0 }).Distinct().ToList())); if (blueprintS2sFailed) s2sTargets.Add((false, results.PendingBlueprintAppRoleSpecs.Where(spec => spec.AppRoleScopes is { Length: > 0 }).Distinct().ToList())); - foreach (var (isAgentIdentityTarget, pendingAppRoleSpecs) in s2sTargets) { actionCount++; @@ -1081,6 +1101,11 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str logger.LogInformation(" Invoke-MgGraphRequest -Method POST -Uri '{GraphBaseUrl}/v1.0/oauth2PermissionGrants' -Body $body -ContentType 'application/json'", resolvedGraphBaseUrl); logger.LogInformation(""); } + logger.LogInformation(" # Defender API"); + logger.LogInformation(" $defenderSp = Get-MgServicePrincipal -Filter \"appId eq '{DefenderAppId}'\"", ConfigConstants.DefenderApiAppId); + logger.LogInformation(" $body = @{{ clientId = $agentSpId; consentType = 'AllPrincipals'; resourceId = $defenderSp.Id; scope = '{DefenderScope}' }} | ConvertTo-Json", ConfigConstants.DefenderApiRealtimeProtectionScope); + logger.LogInformation(" Invoke-MgGraphRequest -Method POST -Uri '{GraphBaseUrl}/v1.0/oauth2PermissionGrants' -Body $body -ContentType 'application/json'", resolvedGraphBaseUrl); + logger.LogInformation(""); logger.LogInformation(" # Power Platform API"); logger.LogInformation(" $ppSp = Get-MgServicePrincipal -Filter \"appId eq '{PpAppId}'\"", PowerPlatformConstants.PowerPlatformApiResourceAppId); logger.LogInformation(" $body = @{{ clientId = $agentSpId; consentType = 'AllPrincipals'; resourceId = $ppSp.Id; scope = '{PpScope}' }} | ConvertTo-Json", PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead); @@ -1126,27 +1151,6 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str 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) @@ -1226,8 +1230,8 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str { nextStepLines.Add(() => logger.LogInformation(" 1. Run 'a365 setup permissions mcp' to configure MCP permissions")); nextStepLines.Add(() => logger.LogInformation(results.IsM365 - ? " 2. Run 'a365 setup permissions bot' to configure Bot API and Power Platform permissions" - : " 2. Run 'a365 setup permissions bot' to configure Power Platform permissions")); + ? " 2. Run 'a365 setup permissions bot' to configure Bot API, Defender, and Power Platform permissions" + : " 2. Run 'a365 setup permissions bot' to configure Defender and Power Platform permissions")); } if (nextStepLines.Count > 0) @@ -1245,10 +1249,9 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, str /// resources. Called when the current user lacks the Global Administrator role so that the URLs /// can be saved to a365.generated.config.json and shared with a tenant administrator. /// - /// Graph, Agent 365 Tools (MCP), and Power Platform API URLs are always generated; the Observability - /// API URL is generated unless is false. Messaging Bot API is included only - /// when is true — non-M365 tenants typically lack the Messaging Bot - /// resource SP and the consent endpoint returns AADSTS650053 otherwise. + /// Graph, Agent 365 Tools (MCP), and Power Platform API URLs are always generated. Observability + /// and delegated Defender URLs are generated only when their corresponding include flags are true. + /// Messaging Bot API is included only when is true. /// /// /// Display names of the resources for which URLs were saved. @@ -1259,7 +1262,8 @@ internal static List PopulateAdminConsentUrls( bool isM365 = true, IReadOnlyDictionary? mcpScopesByAudience = null, IReadOnlyDictionary>? mcpAudienceDisplayNames = null, - bool includeObservability = true) + bool includeObservability = true, + bool includeDefenderDelegated = true) { var graphBaseUrl = ConfigConstants.GetGraphBaseUrl(config.Environment, config.GraphBaseUrl); var graphResourceUri = graphBaseUrl; @@ -1269,11 +1273,13 @@ internal static List PopulateAdminConsentUrls( var urls = BuildAdminConsentUrls( config.TenantId, config.AgentBlueprintId!, config.AgentApplicationScopes, mcpScopes, isM365, mcpScopesByAudience, mcpAudienceDisplayNames, graphResourceUri, authorityHost, - mcpResourceAppId, observabilityResourceAppId, includeObservability); + mcpResourceAppId, observabilityResourceAppId, includeObservability, includeDefenderDelegated); // Clear an Observability consent URL saved by an earlier run so the admin is not asked for permissions this run skipped. if (!includeObservability) ClearSkippedObservabilityConsentUrl(config); + if (!includeDefenderDelegated) + ClearSkippedDefenderConsentUrl(config); // 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 @@ -1287,6 +1293,7 @@ internal static List PopulateAdminConsentUrls( ["Agent 365 Tools"] = mcpResourceAppId, ["Messaging Bot API"] = ConfigConstants.MessagingBotApiAppId, ["Observability API"] = observabilityResourceAppId, + ["Defender API"] = ConfigConstants.DefenderApiAppId, ["Power Platform API"] = PowerPlatformConstants.PowerPlatformApiResourceAppId, }; @@ -1394,6 +1401,8 @@ internal static string GetResourceIdentifierUri( return ConfigConstants.MessagingBotApiIdentifierUri; if (ConfigConstants.IsObservabilityApiAppId(resourceAppId)) return ConfigConstants.BuildObservabilityApiIdentifierUri(resourceAppId); + if (string.Equals(resourceAppId, ConfigConstants.DefenderApiAppId, StringComparison.OrdinalIgnoreCase)) + return ConfigConstants.DefenderApiIdentifierUri; if (string.Equals(resourceAppId, PowerPlatformConstants.PowerPlatformApiResourceAppId, StringComparison.OrdinalIgnoreCase)) return PowerPlatformConstants.PowerPlatformApiIdentifierUri; // WorkIQ Tools shared (issue #429): match by appId, not display name. V2 per-server @@ -1452,7 +1461,8 @@ internal static string BuildFullyQualifiedScope( /// (mirrors ): Microsoft Graph (when /// non-empty), Agent 365 Tools (when /// non-empty), Messaging Bot API (when is true), Observability API - /// (unless is false), and Power Platform API. + /// (unless is false), Defender API (when delegated + /// Defender consent is enabled), and Power Platform API. /// /// Messaging Bot is gated on because non-M365 tenants typically /// lack the Messaging Bot resource SP, in which case the /v2.0/adminconsent endpoint returns @@ -1472,7 +1482,8 @@ internal static string BuildFullyQualifiedScope( string? authorityHost = null, string? sharedMcpResourceAppId = null, string? observabilityResourceAppId = null, - bool includeObservability = true) + bool includeObservability = true, + bool includeDefenderDelegated = true) { var urls = new List<(string, string)>(); @@ -1541,6 +1552,8 @@ string Build(string tenant, string client, string resourceUri, IEnumerable /// Builds a single combined /v2.0/adminconsent URL covering every resource stamped on the /// blueprint: Graph, Agent 365 Tools (MCP), Observability API (unless - /// is false), Power Platform API, and - /// Messaging Bot API (only when is true). + /// is false), Defender API when delegated Defender + /// consent is enabled, Power Platform API, and Messaging Bot API (only when + /// is true). /// /// Messaging Bot is gated on because non-M365 tenants typically /// lack the Messaging Bot resource SP, which would cause the entire combined consent grant @@ -1569,7 +1583,8 @@ internal static string BuildCombinedConsentUrl( string? authorityHost = null, string? sharedMcpResourceAppId = null, string? observabilityResourceAppId = null, - bool includeObservability = true) + bool includeObservability = true, + bool includeDefenderDelegated = true) { var allScopes = new List(); foreach (var s in graphScopes) @@ -1605,6 +1620,8 @@ internal static string BuildCombinedConsentUrl( if (includeObservability) allScopes.Add( $"{ConfigConstants.BuildObservabilityApiIdentifierUri(observabilityResourceAppId ?? ConfigConstants.ObservabilityApiAppId)}/{ConfigConstants.ObservabilityApiOtelWriteScope}"); + if (includeDefenderDelegated) + allScopes.Add($"{ConfigConstants.DefenderApiIdentifierUri}/{ConfigConstants.DefenderApiRealtimeProtectionScope}"); allScopes.Add($"{PowerPlatformConstants.PowerPlatformApiIdentifierUri}/{PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead}"); return BuildAdminConsentUrl(tenantId, blueprintClientId, allScopes, authorityHost); } @@ -1614,11 +1631,9 @@ internal static string BuildCombinedConsentUrl( /// when the running account is not a Global Administrator. Called by both DW and non-DW setup paths /// after the batch permissions step. /// - /// Messaging Bot API URLs are included only when is true, and - /// Observability API URLs only when the context requests Observability permissions; the other - /// resources (Graph, MCP, Power Platform) are always included so a tenant admin - /// can complete the hand-off with a single URL. When Observability permissions are skipped, a - /// consent URL saved for them by an earlier run is cleared on every run, admin runs included. + /// Messaging Bot API URLs are included only when is true. + /// Observability and delegated Defender URLs follow the current setup request; stale URLs from + /// earlier runs are cleared when either permission is no longer requested. /// Otherwise this is a no-op if admin consent was already granted or the blueprint ID is absent. /// /// @@ -1631,26 +1646,32 @@ internal static void ApplyConsentUrlsIfNeeded( IReadOnlyDictionary? mcpScopesByAudience = null, IReadOnlyDictionary>? mcpAudienceDisplayNames = null) { - // Before the early return, so an admin run also clears a URL saved by an earlier non-admin run. + var includeDefenderDelegated = !ctx.Results.IsNonDwBlueprintFlow || !ctx.IsS2sMode; + + // Before the early return, so an admin run also clears URLs saved by an earlier non-admin run. if (ctx.ObservabilityPermissionsEffectivelySkipped) ClearSkippedObservabilityConsentUrl(ctx.Config); + if (!includeDefenderDelegated) + ClearSkippedDefenderConsentUrl(ctx.Config); if (ctx.Results.TenantWideConsentOutcome == Models.GrantOutcome.Granted || string.IsNullOrWhiteSpace(ctx.Config.AgentBlueprintId)) return; // A custom-permission opt-back-in still needs Observability in the non-admin consent URL. var includeObservability = !ctx.ObservabilityPermissionsEffectivelySkipped; - var consentResourceNames = PopulateAdminConsentUrls(ctx.Config, mcpResourceAppId, mcpScopes, isM365, mcpScopesByAudience, mcpAudienceDisplayNames, includeObservability); + var consentResourceNames = PopulateAdminConsentUrls( + ctx.Config, mcpResourceAppId, mcpScopes, isM365, mcpScopesByAudience, + mcpAudienceDisplayNames, includeObservability, includeDefenderDelegated); ctx.Results.ConsentUrlsSavedToPath = ctx.GeneratedConfigPath; ctx.Results.ConsentResourceNames.AddRange(consentResourceNames); var graphBaseUrl = ConfigConstants.GetGraphBaseUrl(ctx.Config.Environment, ctx.Config.GraphBaseUrl); var graphResourceUri = graphBaseUrl; var authorityHost = ConfigConstants.GetAuthorityHost(ctx.Config.Environment, ctx.Config.AuthorityHost); var observabilityResourceAppId = ConfigConstants.GetObservabilityApiAppId(ctx.Config.Environment); - ctx.Results.CombinedConsentUrl = BuildCombinedConsentUrl( + ctx.Results.CombinedConsentUrl = ctx.Results.AdminConsentUrl ?? BuildCombinedConsentUrl( ctx.Config.TenantId!, ctx.Config.AgentBlueprintId!, graphScopes, mcpScopes, isM365, mcpScopesByAudience, graphResourceUri, authorityHost, - mcpResourceAppId, observabilityResourceAppId, includeObservability); + mcpResourceAppId, observabilityResourceAppId, includeObservability, includeDefenderDelegated); } /// @@ -1664,6 +1685,15 @@ internal static void ClearSkippedObservabilityConsentUrl(Agent365Config config) consent.ConsentUrl = null; } + internal static void ClearSkippedDefenderConsentUrl(Agent365Config config) + { + foreach (var consent in config.ResourceConsents.Where( + rc => string.Equals(rc.ResourceAppId, ConfigConstants.DefenderApiAppId, StringComparison.OrdinalIgnoreCase))) + { + consent.ConsentUrl = null; + } + } + /// /// Prints the dry-run plan for the AI Teammate agent (--aiteammate true) path of setup all. /// @@ -1712,7 +1742,7 @@ internal static void PrintDwSetupAllDryRunPlan( } // 4. Inheritable Permissions - logger.LogInformation(DryRunRow(4, "Inheritable Permissions") + "configure for Microsoft Graph, Agent 365 Tools, Messaging Bot API, Power Platform API"); + logger.LogInformation(DryRunRow(4, "Inheritable Permissions") + "configure for Microsoft Graph, Agent 365 Tools, Messaging Bot API, Defender API, Power Platform API"); if (config is null || !CustomPermissionsRequestObservability(config)) logger.LogInformation(sub + "Observability API not requested (telemetry is authorized by the blueprint's registration)"); else @@ -2101,6 +2131,29 @@ public static async Task EnsureResourcePermissionsAsync( return (result, failureReason); } + private static void LogMissingSpActions( + ILogger logger, + IEnumerable actions, + ref int actionCount) + { + foreach (var action in actions) + { + actionCount++; + logger.LogInformation(" {N}. Missing service principal — '{Name}' ({AppId}) (Global Administrator required)", actionCount, action.ResourceName, action.ResourceAppId); + if (action.Scopes.Length > 0) + logger.LogInformation(" Delegated scopes pending: {Scopes}", string.Join(", ", action.Scopes)); + if (action.AppRoleScopes.Length > 0) + logger.LogInformation(" Application roles pending: {Roles}", string.Join(", ", action.AppRoleScopes)); + logger.LogInformation(" Step 1) Provision the SP:"); + logger.LogInformation(" {AzCommand}", action.AzCreateCommand); + if (!string.IsNullOrWhiteSpace(action.PerSpConsentUrl)) + { + logger.LogInformation(" Step 2) Grant the blueprint delegated consent for this resource (click Accept):"); + logger.LogInformation(" {Url}", action.PerSpConsentUrl); + } + } + } + /// /// Emits an "Action Required" block for a non-admin setup permissions * run that mirrors /// the corresponding section produced by the setup all summary writer. Surfaces: @@ -2137,7 +2190,7 @@ internal static void LogPermissionsActionRequired( var hasConsentUrl = !string.IsNullOrWhiteSpace(adminConsentUrl); var hasS2SPowerShell = appRoleSpecs.Count > 0 && results.BlueprintS2SOutcome != Models.GrantOutcome.Granted; - if (!hasConsentUrl && !hasS2SPowerShell) + if (!hasConsentUrl && !hasS2SPowerShell && results.MissingSpActions.Count == 0) { logger.LogInformation("Ask a tenant administrator to grant consent for the blueprint app's required permissions."); return; @@ -2147,6 +2200,8 @@ internal static void LogPermissionsActionRequired( logger.LogInformation("Action Required:"); var step = 0; + LogMissingSpActions(logger, results.MissingSpActions, ref step); + if (hasConsentUrl) { step++; 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 425fd971..3ad58e81 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs @@ -203,8 +203,8 @@ public class SetupResults /// /// The effective --authmode value used during the non-DW grant step. - /// Null when the non-DW grant step was not reached (e.g. agent identity creation failed) or - /// when the run is a DW (AI Teammate) flow — DW does not use --authmode. + /// Set at the start of a non-DW run so permission construction and failure summaries share the + /// same mode. Null for DW (AI Teammate) flows, which do not use --authmode. /// Used by DisplaySetupSummary to compute per-grant-type completion for the "both" mode and to /// derive which Action Required items apply. /// @@ -343,8 +343,8 @@ public void RecordProjectSettingsResult(ProjectSettingsSyncResult result) /// 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 + /// az ad sp create command to provision the SP plus, when delegated scopes are + /// requested, the per-SP unified-consent URL that grants blueprint consent. The setup /// summary's "Action Required" block renders these as numbered items so the operator /// can complete provisioning without re-running setup. /// @@ -356,19 +356,22 @@ public void RecordProjectSettingsResult(ProjectSettingsSyncResult result) /// /// One entry in . Resource identity plus the -/// two concrete commands/URLs the operator needs to complete provisioning manually: +/// 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. +/// (2) when delegated scopes are requested, the per-SP /v2.0/adminconsent URL. /// /// 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. +/// Per-SP delegated-consent URL, or null for application-only specs. public sealed record MissingSpAction( string ResourceName, string ResourceAppId, string[] Scopes, string AzCreateCommand, - string PerSpConsentUrl); + string? PerSpConsentUrl) +{ + /// Application roles awaiting assignment after the resource SP is provisioned. + public string[] AppRoleScopes { get; init; } = []; +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs index 6399c86f..813c005f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs @@ -108,6 +108,16 @@ public static class ConfigConstants /// public const string ObservabilityApiIdentifierUri = "api://9b975845-388f-4429-889e-eab1ef63949c"; + /// + /// Defender API App ID + /// + public const string DefenderApiAppId = "86a21212-634e-4553-b3d6-e477e4c9d9ec"; + + /// + /// Defender API identifier URI. + /// + public const string DefenderApiIdentifierUri = "api://86a21212-634e-4553-b3d6-e477e4c9d9ec"; + /// /// Single source of truth for the Messaging Bot API delegated scope. /// The resource SP (appId 5a807f24-c9de-44ee-a3a7-329e88a00ffc) exposes exactly @@ -125,6 +135,12 @@ public static class ConfigConstants /// public const string ObservabilityApiOtelWriteScope = "Agent365.Observability.OtelWrite"; + /// + /// Defender API app role and delegated scope for the Defender security integration. + /// Must match the value published on the resource SP. + /// + public const string DefenderApiRealtimeProtectionScope = "RealtimeProtection.Evaluate.All"; + /// /// Delegated scope value exposed on the blueprint app registration to enable /// OBO (On-Behalf-Of) callers to acquire tokens scoped to the agent. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs index 95d3484f..4561c3e2 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs @@ -1255,6 +1255,7 @@ private async Task ResolveBlueprintObjectIdAsync( /// Names of the app roles to assign (e.g. "Agent365.Observability.OtelWrite"). /// Graph scopes required to perform the operation. /// Cancellation token. + /// Resolved resource SP object ID, avoiding an app-ID lookup during replication. /// /// An describing the outcome. AllSucceeded is true when /// every role was either successfully assigned or already in place; AllAlreadyAssigned @@ -1267,7 +1268,8 @@ public virtual async Task GrantAppRoleAssignmentAsync( string resourceAppId, IEnumerable appRoleNames, IEnumerable? requiredScopes = null, - CancellationToken ct = default) + CancellationToken ct = default, + string? knownResourceSpObjectId = null) { // De-dup upfront: duplicate names map to the same role ID and would cause a redundant POST. var roleNames = appRoleNames? @@ -1278,9 +1280,11 @@ public virtual async Task GrantAppRoleAssignmentAsync( try { - // Resolve the resource service principal. - var resourceSpId = await _graphApiService.LookupServicePrincipalByAppIdAsync( - tenantId, resourceAppId, ct, requiredScopes); + // Reuse the authoritative ID when the app-ID index has not replicated yet (issue #485). + var resourceSpId = knownResourceSpObjectId; + if (string.IsNullOrWhiteSpace(resourceSpId)) + resourceSpId = await _graphApiService.LookupServicePrincipalByAppIdAsync( + tenantId, resourceAppId, ct, requiredScopes); if (string.IsNullOrWhiteSpace(resourceSpId)) { _logger.LogWarning("Resource SP not found for app ID {ResourceAppId} — S2S app role assignment skipped.", resourceAppId); 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 d827c5cc..8f237e2d 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AdminConsentHelper.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AdminConsentHelper.cs @@ -2,6 +2,8 @@ // Licensed under the MIT License. using System; +using System.Collections.Generic; +using System.Linq; using System.Text.Json; using System.Threading; using System.Threading.Tasks; @@ -18,8 +20,8 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; public enum ConsentPollResult { /// - /// An oauth2PermissionGrant for the client SP was observed in Graph. Safe to mark - /// consent as granted in persisted state. + /// Every requested AllPrincipals resource grant and scope was observed in Graph. Safe + /// to mark consent as granted in persisted state. /// Verified, @@ -38,6 +40,15 @@ public enum ConsentPollResult NotDetected } +/// +/// One resource grant that must be observed before admin-consent polling can report success. +/// +public sealed record AdminConsentRequirement( + string ResourceName, + string ResourceAppId, + IReadOnlyCollection Scopes, + string? ResourceSpObjectId = null); + /// /// Helper methods for admin consent flows that use az cli to poll Graph resources. /// Kept intentionally small and focused so it can be reused across commands/runners. @@ -67,8 +78,17 @@ internal static bool BypassConsentChecksForTests private static readonly AsyncLocal _bypassConsentChecks = new(); + /// Overrides polling so fallback tests need not wait for the consent timeout. + internal static bool? ConsentPollResultForTests + { + get => _consentPollResult.Value; + set => _consentPollResult.Value = value; + } + + private static readonly AsyncLocal _consentPollResult = new(); + /// - /// Polls Azure AD/Graph (via az rest) to detect an oauth2 permission grant for the provided appId. + /// Polls Azure AD/Graph (via az rest) until every requested resource grant is present. /// Mirrors the behavior previously implemented in A365SetupRunner.PollAdminConsentAsync. /// public static async Task PollAdminConsentAsync( @@ -79,14 +99,18 @@ public static async Task PollAdminConsentAsync( int timeoutSeconds, int intervalSeconds, CancellationToken ct, - string? graphBaseUrl = null) + string? graphBaseUrl = null, + IReadOnlyCollection? requiredGrants = null) { + if (ConsentPollResultForTests is bool pollResult) + return pollResult; if (BypassConsentChecksForTests) return true; var start = DateTime.UtcNow; var baseUrl = ConfigConstants.NormalizeGraphBaseUrl(graphBaseUrl); string? spId = null; + var resolvedRequirements = requiredGrants?.ToList(); int lastProgressReportSeconds = 0; logger.LogInformation( @@ -129,6 +153,21 @@ public static async Task PollAdminConsentAsync( if (spId != null) { + if (resolvedRequirements is { Count: > 0 }) + { + for (var i = 0; i < resolvedRequirements.Count; i++) + { + var requirement = resolvedRequirements[i]; + if (!string.IsNullOrWhiteSpace(requirement.ResourceSpObjectId)) + continue; + + var resourceSpId = await LookupSpObjectIdByAppIdAsync( + executor, requirement.ResourceAppId, baseUrl, ct); + if (!string.IsNullOrWhiteSpace(resourceSpId)) + resolvedRequirements[i] = requirement with { ResourceSpObjectId = resourceSpId }; + } + } + var grants = await executor.ExecuteAsync("az", $"rest --method GET --url \"{baseUrl}/v1.0/oauth2PermissionGrants?$filter=clientId eq '{spId}'\"", captureOutput: true, suppressErrorLogging: true, cancellationToken: ct); @@ -139,7 +178,10 @@ public static async Task PollAdminConsentAsync( { using var gdoc = JsonDocument.Parse(grants.StandardOutput); var arr = gdoc.RootElement.GetProperty("value"); - if (arr.GetArrayLength() > 0) + var consentComplete = resolvedRequirements is { Count: > 0 } + ? GrantsSatisfyRequirements(arr, resolvedRequirements) + : arr.GetArrayLength() > 0; + if (consentComplete) { logger.LogInformation("Consent granted ({ScopeDescriptor}).", scopeDescriptor); return true; @@ -194,7 +236,8 @@ public static async Task PollAdminConsentAsync( int timeoutSeconds, int intervalSeconds, CancellationToken ct, - IEnumerable? permScopes = null) + IEnumerable? permScopes = null, + IReadOnlyCollection? requiredGrants = null) { if (BypassConsentChecksForTests) { @@ -237,11 +280,16 @@ public static async Task PollAdminConsentAsync( permScopes); if (grantsDoc != null && - grantsDoc.RootElement.TryGetProperty("value", out var arr) && - arr.GetArrayLength() > 0) + grantsDoc.RootElement.TryGetProperty("value", out var arr)) { - logger.LogInformation("Consent granted ({ScopeDescriptor}).", scopeDescriptor); - return ConsentPollResult.Verified; + var consentComplete = requiredGrants is { Count: > 0 } + ? GrantsSatisfyRequirements(arr, requiredGrants) + : arr.GetArrayLength() > 0; + if (consentComplete) + { + logger.LogInformation("Consent granted ({ScopeDescriptor}).", scopeDescriptor); + return ConsentPollResult.Verified; + } } logger.LogDebug("No consent grants found for blueprint SP {ClientSpId} yet.", clientSpId); @@ -261,6 +309,54 @@ public static async Task PollAdminConsentAsync( } } + internal static bool GrantsSatisfyRequirements( + JsonElement grants, + IReadOnlyCollection requirements) + { + if (requirements.Count == 0) + return grants.GetArrayLength() > 0; + + var scopesByResourceSp = new Dictionary>(StringComparer.OrdinalIgnoreCase); + foreach (var grant in grants.EnumerateArray()) + { + if (!grant.TryGetProperty("consentType", out var consentType) + || !string.Equals(consentType.GetString(), "AllPrincipals", StringComparison.OrdinalIgnoreCase) + || !grant.TryGetProperty("resourceId", out var resourceIdElement) + || string.IsNullOrWhiteSpace(resourceIdElement.GetString())) + { + continue; + } + + var resourceId = resourceIdElement.GetString()!; + if (!scopesByResourceSp.TryGetValue(resourceId, out var grantedScopes)) + { + grantedScopes = new HashSet(StringComparer.OrdinalIgnoreCase); + scopesByResourceSp[resourceId] = grantedScopes; + } + + if (!grant.TryGetProperty("scope", out var scopeElement)) + continue; + + foreach (var scope in (scopeElement.GetString() ?? string.Empty) + .Split(' ', StringSplitOptions.RemoveEmptyEntries)) + { + grantedScopes.Add(scope); + } + } + + foreach (var requirement in requirements) + { + if (string.IsNullOrWhiteSpace(requirement.ResourceSpObjectId) + || !scopesByResourceSp.TryGetValue(requirement.ResourceSpObjectId, out var grantedScopes) + || !requirement.Scopes.All(grantedScopes.Contains)) + { + return false; + } + } + + return true; + } + /// /// Checks if admin consent already exists for specified scopes between client and resource service principals. /// Returns true if ALL required scopes are present in existing oauth2PermissionGrants. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/LogRedactionService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/LogRedactionService.cs index d8402d9d..b101f1d1 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/LogRedactionService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/LogRedactionService.cs @@ -60,6 +60,7 @@ public sealed class LogRedactionService : ILogRedactionService "00000003-0000-0000-c000-000000000000", // Microsoft Graph "5a807f24-c9de-44ee-a3a7-329e88a00ffc", // Agent 365 Messaging Bot API "9b975845-388f-4429-889e-eab1ef63949c", // Agent 365 Observability API + ConfigConstants.DefenderApiAppId, ConfigConstants.GccObservabilityApiAppId, ConfigConstants.GccHighObservabilityApiAppId, ConfigConstants.DodObservabilityApiAppId, diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/design.md b/src/Microsoft.Agents.A365.DevTools.Cli/design.md index 12771cb5..132cffff 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/design.md +++ b/src/Microsoft.Agents.A365.DevTools.Cli/design.md @@ -458,8 +458,8 @@ return await new CommandLineBuilder(rootCommand) | Agent Type | Flag | What it creates | |---|---|---| -| **AI Teammate agent** | `--aiteammate` | Azure infra + Agent Blueprint + batch permissions (4 resources) + messaging endpoint | -| **Custom Engine Agent / Blueprint** (default) | omit `--aiteammate` | Agent Blueprint + batch permissions (Graph + A365 Tools only) + Agent Instance (Graph API) | +| **AI Teammate agent** | `--aiteammate` | Azure infra + Agent Blueprint + batch permissions (5 resources) + messaging endpoint | +| **Custom Engine Agent / Blueprint** (default) | omit `--aiteammate` | Agent Blueprint + baseline permissions (4 resources) + Agent Identity + Agent Registration | Non-DW blueprint agents do not use Azure Bot Service, so there is no infrastructure step, no manifest zip, and no messaging endpoint registration. The final step is `POST /beta/agentRegistry/agentInstances` instead. @@ -475,14 +475,14 @@ AllSubcommand.ExecuteAsync ├── DW path: │ ExecuteInfrastructureStepAsync(ctx) ← DW only │ ExecuteBlueprintStepAsync(ctx) ← shared - │ ExecuteBatchPermissionsStepAsync(ctx, dwSpecs) ← shared (4 resources) + │ ExecuteBatchPermissionsStepAsync(ctx, dwSpecs) ← shared (5 resources) │ ExecuteMessagingEndpointStepAsync(ctx) ← DW only │ └── Non-DW path: NonDwBlueprintSetupOrchestrator.ExecuteAsync(ctx) ExecuteBlueprintStepAsync(ctx) ← reuses DW step - ExecuteBatchPermissionsStepAsync(ctx, nonDwSpecs) ← reuses, 2 resources only - RegisterAgentInstanceAsync(...) ← non-DW final step + ExecuteBatchPermissionsStepAsync(ctx, nonDwSpecs) ← reuses, 4 baseline resources + Create Agent Identity + Agent Registration ← non-DW final steps ``` `SetupContext.Config` is intentionally mutable — the blueprint step reloads configuration from disk after writing `AgentBlueprintId`, and the updated instance must be visible to all subsequent steps. @@ -493,9 +493,10 @@ The non-DW spec list is a strict subset of the DW list: | Resource | DW | Non-DW Blueprint | |---|---|---| -| Microsoft Graph (delegated) | ✓ | — | -| Agent 365 Tools (delegated) | ✓ | — | -| Messaging Bot API | ✓ | — | +| Microsoft Graph (delegated) | ✓ | ✓ | +| Agent 365 Tools (delegated) | ✓ | ✓ | +| Messaging Bot API (with `--m365`) | ✓ | ✓ | +| Defender API | delegated + application | follows `--authmode` | | Power Platform API | ✓ | ✓ | Neither list includes the Observability API: telemetry is exported with app-only tokens that the service authorizes by registration. Adding it to `customBlueprintPermissions` requests its delegated scope. diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AllSubcommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AllSubcommandTests.cs index efe88a04..daaabadc 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AllSubcommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AllSubcommandTests.cs @@ -358,7 +358,11 @@ public async Task ExecuteMessagingEndpointStepAsync_WhenOverrideProvidedAndConfi // Observability API permission wiring // ----------------------------------------------------------------------- - private SetupContext BuildPermissionsContext(bool skipObservabilityPermissions, List? customPermissions = null) + private SetupContext BuildPermissionsContext( + bool skipObservabilityPermissions, + List? customPermissions = null, + string? authMode = null, + bool isNonDwBlueprintFlow = true) { var executor = Substitute.For(Substitute.For>()); var graph = Substitute.For(); @@ -368,6 +372,8 @@ private SetupContext BuildPermissionsContext(bool skipObservabilityPermissions, Arg.Any(), Arg.Any(), Arg.Any?>(), Arg.Any()) .Returns(new List<(string ResourceAppId, bool ScopesAllAllowed, bool RolesAllAllowed)>()); + var results = new SetupResults { IsNonDwBlueprintFlow = isNonDwBlueprintFlow }; + return new SetupContext( config: new Agent365Config { @@ -378,7 +384,7 @@ private SetupContext BuildPermissionsContext(bool skipObservabilityPermissions, DeploymentProjectPath = _tempDir, CustomBlueprintPermissions = customPermissions, }, - results: new SetupResults(), + results: results, logger: NullLogger.Instance, configFile: new FileInfo(Path.Combine(_tempDir, "a365.config.json")), generatedConfigPath: Path.Combine(_tempDir, "a365.generated.config.json"), @@ -398,6 +404,7 @@ private SetupContext BuildPermissionsContext(bool skipObservabilityPermissions, federatedCredentialService: Substitute.ForPartsOf( Substitute.For>(), graph), clientAppValidator: Substitute.For(), + authMode: authMode, skipObservabilityPermissions: skipObservabilityPermissions); } @@ -412,12 +419,66 @@ public async Task BuildPermissionSpecsAsync_StampsObservabilityApiUnlessSkipped( specs.Any(s => s.ResourceAppId == ConfigConstants.ObservabilityApiAppId).Should().Be(!skipObservabilityPermissions, because: "the spec list drives inheritable permissions, app role grants, and admin consent, so skipping Observability permissions must remove Observability API from it"); - specs.Any(s => s.AppRoleScopes is { Length: > 0 }).Should().Be(!skipObservabilityPermissions, - because: "OtelWrite is the only app role setup requests, so skipping it must leave no app role grant that needs a Global Administrator"); + specs.Should().Contain(s => s.ResourceAppId == ConfigConstants.DefenderApiAppId, + because: "skipping Observability permissions must not remove the Defender permission required by the selected auth mode"); specs.Should().Contain(s => s.ResourceAppId == PowerPlatformConstants.PowerPlatformApiResourceAppId, because: "skipping Observability API must not drop the other required resources"); } + [Theory] + [InlineData(null, true, false)] + [InlineData("obo", true, false)] + [InlineData(" OBO ", true, false)] + [InlineData("s2s", false, true)] + [InlineData(" S2S ", false, true)] + [InlineData("both", true, true)] + [InlineData(" BOTH ", true, true)] + public async Task BuildPermissionSpecsAsync_NonDw_DefenderPermissionsMatchAuthMode( + string? authMode, + bool expectDelegated, + bool expectApplication) + { + var ctx = BuildPermissionsContext( + skipObservabilityPermissions: true, + authMode: authMode, + isNonDwBlueprintFlow: true); + + var (specs, _, _, _, _) = await AllSubcommand.BuildPermissionSpecsAsync(ctx); + + var defender = specs.Single(s => s.ResourceAppId == ConfigConstants.DefenderApiAppId); + defender.Scopes.Should().BeEquivalentTo( + expectDelegated ? [ConfigConstants.DefenderApiRealtimeProtectionScope] : [], + because: $"the '{authMode ?? "obo"}' auth mode must request delegated Defender consent only when OBO is enabled"); + if (expectApplication) + { + defender.AppRoleScopes.Should().BeEquivalentTo( + [ConfigConstants.DefenderApiRealtimeProtectionScope], + because: $"the '{authMode ?? "obo"}' auth mode enables S2S Defender evaluation"); + } + else + { + defender.AppRoleScopes.Should().BeNull( + because: $"the '{authMode ?? "obo"}' auth mode must not request a Defender application role"); + } + } + + [Fact] + public async Task BuildPermissionSpecsAsync_Dw_PreservesBothDefenderPermissionTypes() + { + var ctx = BuildPermissionsContext( + skipObservabilityPermissions: false, + authMode: "obo", + isNonDwBlueprintFlow: false); + + var (specs, _, _, _, _) = await AllSubcommand.BuildPermissionSpecsAsync(ctx); + + var defender = specs.Single(s => s.ResourceAppId == ConfigConstants.DefenderApiAppId); + defender.Scopes.Should().BeEquivalentTo([ConfigConstants.DefenderApiRealtimeProtectionScope], + because: "DW setup does not use blueprint-agent auth modes and must preserve its delegated Defender permission"); + defender.AppRoleScopes.Should().BeEquivalentTo([ConfigConstants.DefenderApiRealtimeProtectionScope], + because: "DW setup must preserve its existing Defender application permission"); + } + [Fact] public void ApplyConsentUrlsIfNeeded_WhenObservabilitySkipped_HandsOffOnlyTheRemainingResources() { @@ -426,14 +487,57 @@ public void ApplyConsentUrlsIfNeeded_WhenObservabilitySkipped_HandsOffOnlyTheRem SetupHelpers.ApplyConsentUrlsIfNeeded( ctx, McpConstants.WorkIQToolsProdAppId, ctx.Config.AgentApplicationScopes, new[] { "McpServers.Mail.All" }, isM365: false); - ctx.Results.ConsentResourceNames.Should().BeEquivalentTo(new[] { "Microsoft Graph", "Agent 365 Tools", "Power Platform API" }, - because: "a non-admin run must hand every stamped resource to an administrator, and Observability API is no longer stamped"); + ctx.Results.ConsentResourceNames.Should().BeEquivalentTo(new[] { "Microsoft Graph", "Agent 365 Tools", "Defender API", "Power Platform API" }, + because: "a non-admin run must hand every stamped resource to an administrator, including Defender, while Observability is no longer stamped"); ctx.Config.ResourceConsents.Should().NotContain(rc => rc.ResourceAppId == ConfigConstants.ObservabilityApiAppId, because: "no Observability API consent URL may be persisted when its permissions were skipped"); ctx.Results.CombinedConsentUrl.Should().NotContain(ConfigConstants.ObservabilityApiAppId, because: "the single hand-off URL must not request Observability API scopes that setup skipped"); } + [Fact] + public void ApplyConsentUrlsIfNeeded_S2s_OmitsDelegatedDefenderConsentAndClearsStaleUrl() + { + var ctx = BuildPermissionsContext( + skipObservabilityPermissions: true, + authMode: "s2s", + isNonDwBlueprintFlow: true); + ctx.Config.ResourceConsents.Add(new ResourceConsent + { + ResourceName = "Defender API", + ResourceAppId = ConfigConstants.DefenderApiAppId, + ConsentUrl = "https://login.microsoftonline.com/stale-defender-consent", + }); + + SetupHelpers.ApplyConsentUrlsIfNeeded( + ctx, McpConstants.WorkIQToolsProdAppId, ctx.Config.AgentApplicationScopes, + new[] { "McpServers.Mail.All" }, isM365: false); + + ctx.Results.ConsentResourceNames.Should().NotContain("Defender API", + because: "S2S-only mode requests the Defender application role, not its delegated scope"); + ctx.Results.CombinedConsentUrl.Should().NotContain( + Uri.EscapeDataString($"{ConfigConstants.DefenderApiIdentifierUri}/{ConfigConstants.DefenderApiRealtimeProtectionScope}"), + because: "the S2S handoff URL must not request delegated Defender admin consent"); + ctx.Config.ResourceConsents.Single(rc => rc.ResourceAppId == ConfigConstants.DefenderApiAppId) + .ConsentUrl.Should().BeNull( + because: "switching from OBO to S2S must clear a stale delegated Defender consent URL"); + } + + [Fact] + public void ApplyConsentUrlsIfNeeded_WhenOrchestratorFilteredMissingResource_PreservesFilteredUrl() + { + const string filteredUrl = "https://login.microsoftonline.com/tenant/v2.0/adminconsent?client_id=blueprint&scope=https%3A%2F%2Fgraph.microsoft.com%2FUser.Read"; + var ctx = BuildPermissionsContext(skipObservabilityPermissions: true); + ctx.Results.AdminConsentUrl = filteredUrl; + + SetupHelpers.ApplyConsentUrlsIfNeeded( + ctx, McpConstants.WorkIQToolsProdAppId, ctx.Config.AgentApplicationScopes, + new[] { "McpServers.Mail.All" }, isM365: false); + + ctx.Results.CombinedConsentUrl.Should().Be(filteredUrl, + because: "the orchestrator already excluded unresolved service principals, so rebuilding the URL would reintroduce a resource that poisons the entire consent request"); + } + [Fact] public void ApplyConsentUrlsIfNeeded_AdminRun_ClearsObservabilityConsentUrlSavedByAnEarlierRun() { 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 index 3afcbec9..8014c9cd 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AzRestS2SRunnerTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AzRestS2SRunnerTests.cs @@ -222,6 +222,81 @@ public async Task AzPostExitsNonZero_OverallFails() 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 async Task KnownResourceSpId_PreservesIdempotencyWithoutAppIdLookup() + { + StubExistingAssignmentsGet(existingResourceId: ResourceSpId, existingAppRoleId: OtelWriteRoleId); + StubResourceSpWithAppRolesLookup(returnsEmpty: true); + _executor.ExecuteAsync( + "az", Arg.Is(args => args.Contains($"https://graph.example/v1.0/servicePrincipals/{ResourceSpId}?$select=id,appRoles")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult + { + ExitCode = 0, + StandardOutput = $"{{\"id\":\"{ResourceSpId}\",\"appRoles\":[{{\"id\":\"{OtelWriteRoleId}\",\"value\":\"{OtelWriteRole}\"}}]}}", + })); + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, [ObsSpec()], _logger, ct: default, + graphBaseUrl: "https://graph.example", + resourceSpObjectIds: new Dictionary { [ObsAppId] = ResourceSpId }); + + attempted.Should().BeTrue(); + succeeded.Should().BeTrue( + because: "the fallback must reuse the known resource ID when app-ID lookup is still empty"); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(args => args.Contains("$filter=appId")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(args => args.Contains("--method POST")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task KnownResourceSpId_WhenReadFailsOrRoleIsMissing_ReturnsFailed(bool readFails) + { + StubExistingAssignmentsGet(emptyValue: true); + _executor.ExecuteAsync( + "az", Arg.Is(args => args.Contains($"/servicePrincipals/{ResourceSpId}?$select=id,appRoles")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult + { + ExitCode = readFails ? 1 : 0, + StandardOutput = $"{{\"id\":\"{ResourceSpId}\",\"appRoles\":[]}}", + StandardError = readFails ? "Resource unavailable" : string.Empty, + })); + + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, [ObsSpec()], _logger, ct: default, + resourceSpObjectIds: new Dictionary { [ObsAppId] = ResourceSpId }); + + attempted.Should().BeTrue(); + succeeded.Should().BeFalse( + because: "a recovered resource ID alone cannot count as a successful application-role assignment"); + await _executor.DidNotReceive().ExecuteAsync( + "az", Arg.Is(args => args.Contains("--method POST")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Theory] + [InlineData("")] + [InlineData("not-a-guid")] + [InlineData("22222222-2222-2222-2222-222222222222\" --method DELETE")] + public async Task InvalidKnownResourceSpId_ReturnsNotAttempted(string knownSpId) + { + var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync( + _executor, BlueprintSpId, [ObsSpec()], _logger, ct: default, + resourceSpObjectIds: new Dictionary { [ObsAppId] = knownSpId }); + + attempted.Should().BeFalse( + because: "a supplied resource object ID reaches a shell command and must pass GUID validation 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 void TryExtractFirstSpIdAndAppRoles_ValidPayload_ReturnsIdAndRoleMap() { @@ -259,6 +334,9 @@ public void TryExtractFirstSpIdAndAppRoles_RoleLookupIsCaseInsensitive() [InlineData("not json")] [InlineData("{\"value\":[]}")] [InlineData("{\"unrelated\":true}")] + [InlineData("{\"value\":null}")] + [InlineData("{\"value\":[null]}")] + [InlineData("[]")] public void TryExtractFirstSpIdAndAppRoles_InvalidOrEmpty_ReturnsNullId(string? azOutput) { var (spId, roles) = AzRestS2SRunner.TryExtractFirstSpIdAndAppRoles(azOutput); 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 index 84e4f13f..58763ff8 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorMissingSpTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorMissingSpTests.cs @@ -3,6 +3,7 @@ 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; @@ -23,9 +24,8 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; /// 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. +/// summary's Action Required block surfaces the az command and, for delegated specs, +/// the per-SP blueprint-as-client consent URL. /// /// /// @@ -82,6 +82,7 @@ public async Task PreflightFindsSp_AddsToResolvedSetAndDoesNotRunAz() .Returns(Task.FromResult(MailMcpSpObjectId)); var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var resolvedSpObjectIds = new Dictionary(StringComparer.OrdinalIgnoreCase); var setupResults = new SetupResults(); var missing = new[] { @@ -95,10 +96,13 @@ await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( _logger, setupResults: setupResults, ct: CancellationToken.None, - commandExecutor: _executor); + commandExecutor: _executor, + resolvedSpObjectIds: resolvedSpObjectIds); 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"); + resolvedSpObjectIds.Should().ContainKey(MailMcpAppId).WhoseValue.Should().Be(MailMcpSpObjectId, + because: "downstream consent and grant phases need the recovered resource service-principal object ID in the same run"); setupResults.MissingSpActions.Should().BeEmpty( because: "the resource was successfully resolved without any operator intervention — no Action Required entry needed"); await _executor.DidNotReceive().ExecuteAsync( @@ -147,6 +151,87 @@ await _executor.DidNotReceive().ExecuteAsync( 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 SkipSpProvisioning_ApplicationOnlySpec_RecordsCreateActionWithoutConsentUrl() + { + using var bypass = TemporarilyDisableSpProvisioningBypass(); + + _graph + .LookupServicePrincipalByAppIdAsync( + TenantId, + ConfigConstants.DefenderApiAppId, + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(null)); + + var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var setupResults = new SetupResults(); + var missing = new[] + { + new ResourcePermissionSpec( + ConfigConstants.DefenderApiAppId, + "Defender API", + [], + SetInheritable: true, + AppRoleScopes: [ConfigConstants.DefenderApiRealtimeProtectionScope]), + }; + + await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( + _graph, TenantId, BlueprintAppId, missing, resolvedSpAppIds, + permScopes: Array.Empty(), + skipSpProvisioning: true, + _logger, + setupResults: setupResults, + ct: CancellationToken.None, + commandExecutor: _executor); + + var action = setupResults.MissingSpActions.Should().ContainSingle( + because: "an application-only Defender spec still needs an actionable service-principal provisioning handoff").Subject; + action.AzCreateCommand.Should().Be($"az ad sp create --id {ConfigConstants.DefenderApiAppId}", + because: "the operator must be able to provision the missing Defender service principal before assigning its app role"); + action.Scopes.Should().BeEmpty( + because: "S2S-only Defender requests no delegated consent"); + action.AppRoleScopes.Should().BeEquivalentTo([ConfigConstants.DefenderApiRealtimeProtectionScope], + because: "the recovery action must identify the application role that remains pending"); + action.PerSpConsentUrl.Should().BeNull( + because: "application-only recovery must not emit an empty delegated-consent URL"); + } + + [Fact] + public void FindMissingResourceSpSpecs_IncludesApplicationOnlyDefender() + { + var defenderSpec = new ResourcePermissionSpec( + ConfigConstants.DefenderApiAppId, + "Defender API", + [], + SetInheritable: true, + AppRoleScopes: [ConfigConstants.DefenderApiRealtimeProtectionScope]); + + var missing = BatchPermissionsOrchestrator.FindMissingResourceSpSpecs( + [defenderSpec], + new HashSet(StringComparer.OrdinalIgnoreCase)); + + missing.Should().ContainSingle().Which.Should().Be(defenderSpec, + because: "S2S application-role assignment cannot succeed until the Defender service principal exists"); + } + + [Fact] + public void HasUnresolvedDelegatedSpecs_DetectsMissingDefenderConsent() + { + var defenderSpec = new ResourcePermissionSpec( + ConfigConstants.DefenderApiAppId, + "Defender API", + [ConfigConstants.DefenderApiRealtimeProtectionScope], + SetInheritable: true); + + var hasUnresolved = BatchPermissionsOrchestrator.HasUnresolvedDelegatedSpecs( + [defenderSpec], + new HashSet(StringComparer.OrdinalIgnoreCase)); + + hasUnresolved.Should().BeTrue( + because: "aggregate consent must remain incomplete until the Defender resource SP can receive its delegated grant"); + } + [Fact] public async Task NullCommandExecutor_FallsBackToWarningPathAndDoesNotPrompt() { @@ -253,6 +338,7 @@ public async Task ConfirmationProviderReturnsTrue_AzExitsZeroWithSpJson_AddsAppI accepting.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(true)); var resolvedSpAppIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var resolvedSpObjectIds = new Dictionary(StringComparer.OrdinalIgnoreCase); var setupResults = new SetupResults(); var missing = new[] { @@ -267,10 +353,13 @@ await BatchPermissionsOrchestrator.EnsureMissingResourceSpsAsync( setupResults: setupResults, ct: CancellationToken.None, commandExecutor: _executor, - confirmationProvider: accepting); + confirmationProvider: accepting, + resolvedSpObjectIds: resolvedSpObjectIds); 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"); + resolvedSpObjectIds.Should().ContainKey(TeamsMcpAppId).WhoseValue.Should().Be("d42a47bf-9727-444c-ae57-17bd588613cd", + because: "the same-run consent precheck and app-role assignment need the object ID returned by az"); 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 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 fa985611..7c18de5e 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 @@ -189,14 +189,29 @@ public async Task ConfigureAllPermissions_NonAdmin_BuildsUnifiedConsentUrlCoveri _graph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) .Returns(Task.FromResult(RoleCheckResult.DoesNotHaveRole)); - // Prevent real network calls: Phase 1 resource SP resolution and Phase 2a inheritable - // permission writes must not reach Azure endpoints in CI. Return null SPs (not found) - // and simulate an insufficient-privileges failure so both phases skip cleanly without - // making any real HTTP requests. + // Prevent real network calls while preserving the production invariant that consent URLs + // contain only resources whose service principals were resolved. _graph.EnsureServicePrincipalForAppIdAsync( Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>(), Arg.Any()) - .Returns(Task.FromResult(null)); + .Returns(call => Task.FromResult(call.ArgAt(1) switch + { + AuthenticationConstants.MicrosoftGraphResourceAppId => "graph-sp", + ConfigConstants.MessagingBotApiAppId => "bot-sp", + ConfigConstants.ObservabilityApiAppId => "observability-sp", + _ => null, + })); + _graph.GetAvailableScopeNamesAsync( + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(call => Task.FromResult(call.ArgAt(1) switch + { + "graph-sp" => new HashSet(["Mail.ReadWrite"], StringComparer.OrdinalIgnoreCase), + "bot-sp" => new HashSet(["BotApi.Scope"], StringComparer.OrdinalIgnoreCase), + "observability-sp" => new HashSet([ConfigConstants.ObservabilityApiOtelWriteScope], StringComparer.OrdinalIgnoreCase), + _ => [], + })); _blueprintService.SetInheritablePermissionsAsync( Arg.Any(), Arg.Any(), Arg.Any(), @@ -312,16 +327,411 @@ private void ArrangeS2SPhase1AndAdminCheck() .Returns(Task.FromResult((ok: false, alreadyExists: false, error: (string?)"Insufficient privileges"))); } - private static ResourcePermissionSpec[] S2SSpec() => + private static ResourcePermissionSpec[] S2SSpec(bool includeDelegatedScope = true) => [ new ResourcePermissionSpec( ConfigConstants.ObservabilityApiAppId, "Observability API", - new[] { ConfigConstants.ObservabilityApiOtelWriteScope }, + includeDelegatedScope ? new[] { ConfigConstants.ObservabilityApiOtelWriteScope } : [], SetInheritable: false, AppRoleScopes: new[] { ConfigConstants.ObservabilityApiOtelWriteScope }) ]; + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task ConfigureAllPermissions_WhenApplicationOnlySpRecovered_GrantsRoleInSameRun(bool useAzFallback) + { + var previousBypass = BatchPermissionsOrchestrator.BypassSpProvisioningForTests; + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = false; + try + { + _graph.GraphGetAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(JsonDocument.Parse("{\"id\":\"user-id\"}"))); + _graph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.HasRole)); + _graph.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), + ConfigConstants.DefenderApiAppId, + Arg.Any(), + Arg.Any?>(), + Arg.Any()) + .Returns(Task.FromResult(null)); + _graph.LookupServicePrincipalByAppIdAsync( + Arg.Any(), + ConfigConstants.DefenderApiAppId, + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(null)); + + _executor.ExecuteAsync( + "az", + Arg.Is(args => args.Contains($"ad sp create --id {ConfigConstants.DefenderApiAppId}")), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(Task.FromResult(new CommandResult + { + ExitCode = 0, + StandardOutput = $"{{\"id\":\"{S2SResourceSpId}\"}}", + })); + + _blueprintService.SetInheritablePermissionsAsync( + Arg.Any(), + Arg.Any(), + ConfigConstants.DefenderApiAppId, + Arg.Any>(), + Arg.Any?>(), + Arg.Any()) + .Returns(Task.FromResult((ok: true, alreadyExists: false, error: (string?)null))); + _blueprintService.VerifyInheritablePermissionsAsync( + Arg.Any(), + Arg.Any(), + ConfigConstants.DefenderApiAppId, + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult((exists: true, scopesAllAllowed: true, rolesAllAllowed: true, error: (string?)null))); + using var appRolesDocument = JsonDocument.Parse( + $"{{\"appRoles\":[{{\"id\":\"{S2SAppRoleId}\",\"value\":\"{ConfigConstants.DefenderApiRealtimeProtectionScope}\"}}]}}"); + using var assignmentsDocument = JsonDocument.Parse("{\"value\":[]}"); + _graph.GraphGetAsync( + S2STenantId, + $"/v1.0/servicePrincipals/{S2SResourceSpId}?$select=appRoles", + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(appRolesDocument)); + _graph.GraphGetAsync( + S2STenantId, + $"/v1.0/servicePrincipals/{S2SBlueprintSpObjectId}/appRoleAssignments", + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(assignmentsDocument)); + string? assignmentBody = null; + _graph.GraphPostWithResponseAsync( + S2STenantId, + $"/v1.0/servicePrincipals/{S2SBlueprintSpObjectId}/appRoleAssignments", + Arg.Any(), + Arg.Any(), + Arg.Any?>(), + Arg.Any(), + Arg.Any()) + .Returns(call => + { + assignmentBody = JsonSerializer.Serialize(call.ArgAt(2)); + return Task.FromResult(new GraphApiService.GraphResponse + { + IsSuccess = !useAzFallback, + StatusCode = useAzFallback ? 403 : 201, + }); + }); + if (useAzFallback) + { + _executor.ExecuteAsync( + "az", + Arg.Is(args => args.Contains("--method GET") && args.Contains($"/servicePrincipals/{S2SBlueprintSpObjectId}/appRoleAssignments")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "{\"value\":[]}" })); + _executor.ExecuteAsync( + "az", + Arg.Is(args => args.Contains($"/servicePrincipals/{S2SResourceSpId}?$select=id,appRoles")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new CommandResult + { + ExitCode = 0, + StandardOutput = $"{{\"id\":\"{S2SResourceSpId}\",\"appRoles\":[{{\"id\":\"{S2SAppRoleId}\",\"value\":\"{ConfigConstants.DefenderApiRealtimeProtectionScope}\"}}]}}", + })); + _executor.ExecuteAsync( + "az", + Arg.Is(args => args.Contains("--method POST") && args.Contains("/appRoleAssignments")), + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(call => + { + var match = System.Text.RegularExpressions.Regex.Match(call.ArgAt(1), "--body @\"([^\"]+)\""); + assignmentBody = File.ReadAllText(match.Groups[1].Value); + return Task.FromResult(new CommandResult { ExitCode = 0 }); + }); + } + + var confirmationProvider = Substitute.For(); + confirmationProvider.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(true)); + var setupResults = new SetupResults(); + var specs = new[] + { + new ResourcePermissionSpec( + ConfigConstants.DefenderApiAppId, + "Defender API", + [], + SetInheritable: true, + AppRoleScopes: [ConfigConstants.DefenderApiRealtimeProtectionScope]), + }; + + await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( + _graph, + _blueprintService, + new Agent365Config { TenantId = S2STenantId, AgentBlueprintId = S2SBlueprintAppId }, + S2SBlueprintAppId, + S2STenantId, + specs, + _logger, + setupResults, + ct: default, + knownBlueprintSpObjectId: S2SBlueprintSpObjectId, + confirmationProvider: confirmationProvider, + commandExecutor: _executor); + + await _graph.Received(1).GraphPostWithResponseAsync( + S2STenantId, + $"/v1.0/servicePrincipals/{S2SBlueprintSpObjectId}/appRoleAssignments", + Arg.Any(), + Arg.Any(), + Arg.Any?>(), + Arg.Any(), + Arg.Any()); + assignmentBody.Should().NotBeNull( + because: "same-run recovery must perform the real app-role assignment, not just report a mocked grant result"); + using var assignment = JsonDocument.Parse(assignmentBody!); + assignment.RootElement.GetProperty("resourceId").GetString().Should().Be(S2SResourceSpId, + because: "the assignment must use the authoritative object ID returned by provisioning even while app-ID lookups remain empty"); + assignment.RootElement.GetProperty("principalId").GetString().Should().Be(S2SBlueprintSpObjectId, + because: "the recovered resource role must be assigned to the blueprint service principal"); + assignment.RootElement.GetProperty("appRoleId").GetString().Should().Be(S2SAppRoleId, + because: "the assignment must grant the requested Defender role rather than another published role"); + setupResults.BlueprintS2SOutcome.Should().Be(GrantOutcome.Granted, + because: "successful missing-SP recovery must let the same setup run complete the Defender app-role assignment"); + setupResults.MissingSpActions.Should().BeEmpty( + because: "successful provisioning and assignment leave no manual recovery action"); + setupResults.PendingBlueprintAppRoleSpecs.Should().BeEmpty( + because: "a successful primary or fallback assignment must clear the pending application-role handoff"); + } + finally + { + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = previousBypass; + } + } + + [Fact] + public async Task ConfigureAllPermissions_WhenDelegatedSpUnresolved_DoesNotPersistAggregateConsent() + { + var previousBypass = BatchPermissionsOrchestrator.BypassSpProvisioningForTests; + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = false; + try + { + const string graphSpId = "00000000-0000-0000-0000-000000000006"; + _graph.GraphGetAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(JsonDocument.Parse("{\"id\":\"user-id\"}"))); + _graph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.DoesNotHaveRole)); + _graph.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any?>(), + Arg.Any()) + .Returns(call => Task.FromResult( + call.ArgAt(1) == AuthenticationConstants.MicrosoftGraphResourceAppId + ? graphSpId + : null)); + _graph.LookupServicePrincipalByAppIdAsync( + Arg.Any(), + ConfigConstants.DefenderApiAppId, + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(null)); + _graph.GetAvailableScopeNamesAsync( + Arg.Any(), + graphSpId, + Arg.Any()) + .Returns(new HashSet(["Mail.Read"], StringComparer.OrdinalIgnoreCase)); + + _blueprintService.SetInheritablePermissionsAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any>(), + Arg.Any?>(), + Arg.Any()) + .Returns(Task.FromResult((ok: true, alreadyExists: false, error: (string?)null))); + _blueprintService.VerifyInheritablePermissionsAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult((exists: true, scopesAllAllowed: true, rolesAllAllowed: true, error: (string?)null))); + + var config = new Agent365Config + { + TenantId = S2STenantId, + AgentBlueprintId = S2SBlueprintAppId, + }; + var setupResults = new SetupResults(); + var specs = new[] + { + new ResourcePermissionSpec( + AuthenticationConstants.MicrosoftGraphResourceAppId, + "Microsoft Graph", + ["Mail.Read"], + SetInheritable: true), + new ResourcePermissionSpec( + ConfigConstants.DefenderApiAppId, + "Defender API", + [ConfigConstants.DefenderApiRealtimeProtectionScope], + SetInheritable: true), + }; + + var result = await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( + _graph, + _blueprintService, + config, + S2SBlueprintAppId, + S2STenantId, + specs, + _logger, + setupResults, + ct: default, + knownBlueprintSpObjectId: S2SBlueprintSpObjectId, + commandExecutor: _executor, + skipSpProvisioning: true); + + result.adminConsentGranted.Should().BeFalse( + because: "an unresolved Defender resource cannot be treated as fully consented merely because older Graph grants already exist"); + result.adminConsentUrl.Should().NotBeNull( + because: "the filtered handoff URL must remain available while Defender recovery is pending"); + result.adminConsentUrl.Should().NotContain(ConfigConstants.DefenderApiAppId, + because: "the filtered URL must not reintroduce the missing Defender resource and poison consent for resolved resources"); + config.ResourceConsents.Should().NotContain( + consent => consent.ResourceAppId == ConfigConstants.DefenderApiAppId, + because: "Defender consent must not be persisted until its resource SP and delegated grant are directly verified"); + setupResults.MissingSpActions.Should().ContainSingle( + action => action.ResourceAppId == ConfigConstants.DefenderApiAppId, + because: "the operator still needs the explicit Defender service-principal recovery action"); + } + finally + { + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = previousBypass; + } + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task ConfigureAllPermissions_WhenMissingSpResolvesDuringConsentFallback_PersistsOnlyCompleteConsent(bool fallbackSucceeds) + { + const string graphSpId = "00000000-0000-0000-0000-000000000006"; + const string applicationOnlyAppId = "00000000-0000-0000-0000-000000000007"; + var previousConsentBypass = AdminConsentHelper.BypassConsentChecksForTests; + var previousPollResult = AdminConsentHelper.ConsentPollResultForTests; + var previousSpBypass = BatchPermissionsOrchestrator.BypassSpProvisioningForTests; + AdminConsentHelper.BypassConsentChecksForTests = false; + AdminConsentHelper.ConsentPollResultForTests = false; + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = false; + try + { + ArrangeS2SPhase1AndAdminCheck(); + _graph.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), AuthenticationConstants.MicrosoftGraphResourceAppId, + Arg.Any(), Arg.Any?>(), Arg.Any()) + .Returns(Task.FromResult(graphSpId)); + _graph.LookupServicePrincipalByAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(null)); + _graph.GetAvailableScopeNamesAsync(Arg.Any(), graphSpId, Arg.Any()) + .Returns(new HashSet(["Mail.Read"], StringComparer.OrdinalIgnoreCase)); + + var grants = new List<(string? ResourceId, string? Scope)>(); + _executor.ExecuteAsync( + "az", Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(call => + { + var args = call.ArgAt(1); + if (args.Contains("--method POST") && args.Contains("/oauth2PermissionGrants")) + { + var match = System.Text.RegularExpressions.Regex.Match(args, "--body @\"([^\"]+)\""); + using var body = JsonDocument.Parse(File.ReadAllText(match.Groups[1].Value)); + var resourceId = body.RootElement.GetProperty("resourceId").GetString(); + grants.Add((resourceId, body.RootElement.GetProperty("scope").GetString())); + return Task.FromResult(new CommandResult + { + ExitCode = resourceId == S2SResourceSpId && !fallbackSucceeds ? 1 : 0, + }); + } + if (args.Contains("/oauth2PermissionGrants")) + return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "{\"value\":[]}" }); + if (args.Contains($"/servicePrincipals?$filter=appId eq '{AuthenticationConstants.MicrosoftGraphResourceAppId}'")) + return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = $"{{\"value\":[{{\"id\":\"{graphSpId}\"}}]}}" }); + if (args.Contains($"/servicePrincipals?$filter=appId eq '{ConfigConstants.DefenderApiAppId}'")) + return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = $"{{\"value\":[{{\"id\":\"{S2SResourceSpId}\"}}]}}" }); + throw new InvalidOperationException($"Unexpected az arguments: {args}"); + }); + + var confirmationProvider = Substitute.For(); + confirmationProvider.ConfirmAsync(Arg.Any()) + .Returns(call => Task.FromResult(call.ArgAt(0).Contains("programmatically", StringComparison.Ordinal))); + var config = new Agent365Config { TenantId = S2STenantId, AgentBlueprintId = S2SBlueprintAppId }; + var results = new SetupResults(); + var specs = new[] + { + new ResourcePermissionSpec(AuthenticationConstants.MicrosoftGraphResourceAppId, "Microsoft Graph", ["Mail.Read"], false), + new ResourcePermissionSpec( + ConfigConstants.DefenderApiAppId, "Defender API", [ConfigConstants.DefenderApiRealtimeProtectionScope], + false, AppRoleScopes: [ConfigConstants.DefenderApiRealtimeProtectionScope]), + new ResourcePermissionSpec(applicationOnlyAppId, "Contoso API", [], false, AppRoleScopes: ["Contoso.Write"]), + }; + + var result = await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( + _graph, _blueprintService, config, S2SBlueprintAppId, S2STenantId, specs, _logger, results, + ct: default, knownBlueprintSpObjectId: S2SBlueprintSpObjectId, + confirmationProvider: confirmationProvider, commandExecutor: _executor, skipSpProvisioning: true); + + grants.Should().Contain(grant => grant.ResourceId == graphSpId && grant.Scope == "Mail.Read", + because: "the fallback must attempt every delegated resource rather than only the newly resolved one"); + grants.Should().Contain(grant => grant.ResourceId == S2SResourceSpId && grant.Scope == ConfigConstants.DefenderApiRealtimeProtectionScope, + because: "Defender must be resolved and its delegated grant attempted after the earlier recovery phase found no service principal"); + result.adminConsentGranted.Should().Be(fallbackSucceeds, + because: "only a successful all-resource fallback can replace the earlier unresolved-resource failure"); + if (fallbackSucceeds) + { + result.adminConsentUrl.Should().BeNull( + because: "verified fallback grants must permit the caller to persist consent without a stale handoff URL"); + config.ResourceConsents.Should().Contain( + consent => consent.ResourceAppId == ConfigConstants.DefenderApiAppId && consent.ConsentGranted, + because: "the successful fallback directly verified Defender consent even though the initial SP map was incomplete"); + results.MissingSpActions.Should().NotContain(action => action.ResourceAppId == ConfigConstants.DefenderApiAppId, + because: "a resource resolved and consented by the fallback no longer needs provisioning or delegated-consent instructions"); + } + else + { + result.adminConsentUrl.Should().NotBeNull( + because: "partial fallback success must retain the consent handoff"); + config.ResourceConsents.Should().BeEmpty( + because: "partial fallback success must not persist aggregate delegated consent"); + results.MissingSpActions.Should().Contain(action => action.ResourceAppId == ConfigConstants.DefenderApiAppId, + because: "a failed fallback must preserve the unresolved resource's recovery instructions"); + } + results.MissingSpActions.Should().Contain(action => action.ResourceAppId == applicationOnlyAppId, + because: "delegated consent success cannot resolve unrelated application-only service principal recovery"); + results.BlueprintS2SOutcome.Should().Be(GrantOutcome.Failed, + because: "delegated consent does not grant application roles that the operator declined"); + results.PendingBlueprintAppRoleSpecs.Should().BeEquivalentTo(specs.Skip(1), + because: "pending Defender and application-only roles must remain actionable after either fallback outcome"); + } + finally + { + AdminConsentHelper.BypassConsentChecksForTests = previousConsentBypass; + AdminConsentHelper.ConsentPollResultForTests = previousPollResult; + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = previousSpBypass; + } + } + /// /// When the programmatic Graph API path for S2S fails (e.g. token lacks /// AppRoleAssignment.ReadWrite.All even for a GA) and the az rest fallback completes @@ -501,8 +911,11 @@ private void ArrangeAzRestS2SCalls(bool blueprintAlreadyAssigned, int postExitCo /// 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() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task ConfigureAllPermissions_NonAdmin_WithS2SSpecs_SetsBlueprintS2SOutcomeFailed( + bool includeDelegatedScope) { // Arrange _graph.GraphGetAsync( @@ -534,13 +947,13 @@ await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( _graph, _blueprintService, new Agent365Config { TenantId = S2STenantId, AgentBlueprintId = S2SBlueprintAppId }, blueprintAppId: S2SBlueprintAppId, tenantId: S2STenantId, - specs: S2SSpec(), _logger, setupResults, ct: default); + specs: S2SSpec(includeDelegatedScope), _logger, setupResults, ct: default); // 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 hand-off block"); setupResults.PendingBlueprintAppRoleSpecs.Should().ContainSingle(s => s.ResourceAppId == ConfigConstants.ObservabilityApiAppId, - because: "a non-admin run leaves every requested app role for the summary's hand-off"); + because: "a non-admin run leaves every requested app role for the summary's hand-off, including application-only specs"); } // ────────────────────────────────────────────────────────────────────────────────────── @@ -903,6 +1316,30 @@ public void UpdateResourceConsents_ReplacesCommercialObservabilityEntryForGcc() because: "a GCC rerun must replace stale commercial Observability state"); } + [Fact] + public void UpdateResourceConsents_ApplicationOnlySpec_DoesNotRecordDelegatedConsent() + { + var config = new Agent365Config(); + var specs = new[] + { + new ResourcePermissionSpec( + ConfigConstants.DefenderApiAppId, + "Defender API", + [], + SetInheritable: true, + AppRoleScopes: [ConfigConstants.DefenderApiRealtimeProtectionScope]), + }; + var inheritedResults = new Dictionary + { + [ConfigConstants.DefenderApiAppId] = (true, false), + }; + + BatchPermissionsOrchestrator.UpdateResourceConsents(config, specs, inheritedResults); + + config.ResourceConsents.Should().BeEmpty( + because: "application-role outcomes are tracked separately and must not be persisted as verified delegated consent with an empty scope list"); + } + /// /// 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, diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorDryRunTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorDryRunTests.cs index 0f9765ad..f5341558 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorDryRunTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorDryRunTests.cs @@ -145,9 +145,7 @@ public void PrintDryRunPlan_IncludesAgentRegistrationStep() // ── authMode dry-run output ──────────────────────────────────────────────── /// - /// OBO mode must show delegated (principal-scoped) grants on the agent identity SP. - /// authMode controls only the agent-identity grant style; the blueprint step is independent - /// and always uses AllPrincipals grants on the blueprint (issue #417). + /// OBO mode must show delegated grants without a Defender application-role handoff. /// [Fact] public void PrintDryRunPlan_AuthModeObo_ShowsDelegatedGrantsOnAgentIdentity() @@ -157,10 +155,32 @@ public void PrintDryRunPlan_AuthModeObo_ShowsDelegatedGrantsOnAgentIdentity() AnyLogContains("delegated").Should().BeTrue(because: "OBO mode applies principal-scoped delegated grants to the agent identity SP"); } + /// + /// OBO mode requests the Defender delegated permission only. + /// + [Theory] + [InlineData("obo")] + [InlineData(null)] + public void PrintDryRunPlan_AuthModeObo_OmitsDefenderApplicationRoleGrant(string? authMode) + { + NonDwBlueprintSetupOrchestrator.PrintDryRunPlan( + BuildConfig(), + _logger, + authMode: authMode, + skipObservabilityPermissions: true); + + AnyLogContains("delegated grants").Should().BeTrue( + because: "OBO mode requires the Defender delegated permission"); + AnyLogContains("S2S app roles").Should().BeFalse( + because: "OBO mode must not request the Defender application role"); + AnyLogContains("Global Administrator required for S2S if 403").Should().BeFalse( + because: "OBO mode has no Defender application-role handoff"); + } + /// /// S2S mode must show application permissions on the agent identity SP and must not show /// delegated grants — there is no user context in S2S so delegated scopes don't apply. - /// authMode only affects the agent-identity step; the blueprint step is independent. + /// Defender follows authMode on both the blueprint and agent identity. /// [Fact] public void PrintDryRunPlan_AuthModeS2s_ShowsAppPermsOnAgentIdentity_NoDelegatedGrants() @@ -172,17 +192,17 @@ public void PrintDryRunPlan_AuthModeS2s_ShowsAppPermsOnAgentIdentity_NoDelegated } /// - /// Blueprint agents skip OtelWrite by default, which leaves the S2S half with no app roles to assign. + /// Skipping OtelWrite does not remove the Defender application role required for S2S evaluation. /// [Fact] - public void PrintDryRunPlan_AuthModeS2s_WhenObservabilitySkipped_ShowsNoAppRolesToGrant() + public void PrintDryRunPlan_AuthModeS2s_WhenObservabilitySkipped_ShowsDefenderAppRoleGrant() { NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger, authMode: "s2s", skipObservabilityPermissions: true); - AnyLogContains("not required (no S2S app roles to grant)").Should().BeTrue( - because: "the dry-run plan must match the real summary when the spec list has no app roles"); - AnyLogContains("Global Administrator required if 403").Should().BeFalse( - because: "there is no S2S grant to perform when blueprint agents skip OtelWrite"); + AnyLogContains("S2S app roles").Should().BeTrue( + because: "RealtimeProtection.Evaluate.All remains required when Observability permissions are skipped"); + AnyLogContains("Global Administrator required if 403").Should().BeTrue( + because: "assigning the Defender application role may require Global Administrator"); } [Fact] @@ -202,8 +222,8 @@ public void PrintDryRunPlan_CustomObservabilityPermission_DoesNotClaimObservabil AnyLogContains("Observability API not requested").Should().BeFalse( because: "a custom Observability permission explicitly opts back into requesting Observability permissions"); - AnyLogContains("not required (no S2S app roles to grant)").Should().BeTrue( - because: "custom permissions carry delegated scopes only, so there is still no S2S app role to grant"); + AnyLogContains("S2S app roles").Should().BeTrue( + because: "the fixed Defender permission still carries an application role when Observability is configured as a custom delegated permission"); } /// @@ -220,17 +240,17 @@ public void PrintDryRunPlan_AuthModeBoth_ShowsBothGrantRowsOnAgentIdentity() } /// - /// Both mode still has delegated consent work, but no S2S grant when no spec carries app roles. + /// Both mode retains delegated consent work and the Defender S2S app role when OtelWrite is skipped. /// [Fact] - public void PrintDryRunPlan_AuthModeBoth_WhenObservabilitySkipped_ShowsDelegatedOnly() + public void PrintDryRunPlan_AuthModeBoth_WhenObservabilitySkipped_ShowsDelegatedAndDefenderAppRole() { NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger, authMode: "both", skipObservabilityPermissions: true); - AnyLogContains("delegated grants for the signed-in principal; no S2S app roles to grant").Should().BeTrue( - because: "both mode must preserve the delegated half while saying the S2S half has no role to assign"); - AnyLogContains("Global Administrator required for S2S if 403").Should().BeFalse( - because: "there is no S2S grant fallback to describe when blueprint agents skip OtelWrite"); + AnyLogContains("delegated grants for the signed-in principal + S2S app roles").Should().BeTrue( + because: "both mode must preserve delegated grants and the Defender application role when OtelWrite is skipped"); + AnyLogContains("Global Administrator required for S2S if 403").Should().BeTrue( + because: "the Defender S2S app role still needs an administrative fallback"); } /// diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs index 3d1bc4a1..a3f60cfe 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs @@ -293,6 +293,42 @@ public void BotSubcommand_ShouldHaveDryRunOption() dryRunOption!.Aliases.Should().Contain("--dry-run"); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task BotDryRun_WithOrWithoutConfig_MentionsDefender(bool hasConfig) + { + if (hasConfig) + _mockConfigService.LoadAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new Agent365Config { TenantId = "tenant-id", AgentBlueprintId = "blueprint-id" })); + else + _mockConfigService.LoadAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromException(new FileNotFoundException("Config file not found."))); + var root = new RootCommand(); + root.AddCommand(PermissionsSubcommand.CreateCommand( + _mockLogger, + _mockAuthValidator, + _mockConfigService, + _mockExecutor, + _mockGraphApiService, + _mockBlueprintService, + _mockConfirmationProvider)); + + var result = await root.InvokeAsync("permissions bot --dry-run"); + + result.Should().Be(0, + because: "permissions bot must provide a usable preview whether or not a config file is available"); + var messages = _mockLogger.ReceivedCalls() + .Select(call => call.GetArguments()[2]?.ToString() ?? string.Empty) + .ToList(); + messages.Should().Contain( + message => message.Contains("Defender", StringComparison.OrdinalIgnoreCase), + because: "permissions bot configures Defender and the no-config preview must not omit that user-visible work"); + messages.Should().NotContain( + message => message.Contains("Observability", StringComparison.OrdinalIgnoreCase), + because: "permissions bot no longer requests Observability permissions in either preview path"); + } + [Fact] public void BotSubcommand_DescriptionShouldMentionPrerequisites() { @@ -308,6 +344,8 @@ public void BotSubcommand_DescriptionShouldMentionPrerequisites() // Assert botSubcommand.Description.Should().Contain("Prerequisites"); + botSubcommand.Description.Should().Contain("Defender", + because: "the command description must disclose every fixed platform API that permissions bot configures"); } [Fact] @@ -776,7 +814,7 @@ public async Task ConfigureCustomPermissionsAsync_WithEmptyList_SkipsGracefully( #region ConfigureBotPermissionsAsync — Issue 402 Regression Tests // Regression tests for the localResults fix: failures must be visible even when setupResults=null. - private Agent365Config ArrangeAdminPath() + private Agent365Config ArrangeAdminPath(bool s2sSucceeds = true) { var config = new Agent365Config { @@ -804,6 +842,12 @@ private Agent365Config ArrangeAdminPath() Arg.Any>(), Arg.Any?>(), Arg.Any()) .Returns(Task.FromResult<(bool ok, bool alreadyExists, string? error)>((true, false, null))); + _mockBlueprintService.GrantAppRoleAssignmentAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any?>(), Arg.Any(), + knownResourceSpObjectId: "resource-sp-id") + .Returns(Task.FromResult(new AppRoleGrantResult(AllSucceeded: s2sSucceeds, AllAlreadyAssigned: false))); + _mockGraphApiService.GraphGetAsync( Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>()) @@ -826,12 +870,56 @@ private Agent365Config ArrangeAdminPath() return config; } - /// - /// Telemetry is authorized by registration, so the bot step requests no Observability API permissions. That - /// removes the only app role it requested, so an app role grant that would fail is never attempted. - /// [Fact] - public async Task ConfigureBotPermissionsAsync_AdminPath_RequestsNoObservabilityApiOrAppRoles() + public async Task ConfigureBotPermissionsAsync_MissingDefenderSp_EmitsCompleteStandaloneHandoff() + { + var previousBypass = BatchPermissionsOrchestrator.BypassSpProvisioningForTests; + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = false; + try + { + var config = ArrangeAdminPath(); + _mockGraphApiService.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.DoesNotHaveRole)); + _mockGraphApiService.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), ConfigConstants.DefenderApiAppId, Arg.Any(), + Arg.Any?>(), Arg.Any()) + .Returns(Task.FromResult(null)); + _mockGraphApiService.LookupServicePrincipalByAppIdAsync( + Arg.Any(), ConfigConstants.DefenderApiAppId, Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(null)); + _mockConfirmationProvider.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(false)); + + var result = await PermissionsSubcommand.ConfigureBotPermissionsAsync( + "config.json", _mockLogger, _mockConfigService, _mockExecutor, config, + _mockGraphApiService, _mockBlueprintService, iSetupAll: false, + setupResults: null, confirmationProvider: _mockConfirmationProvider); + + var output = string.Join("\n", _mockLogger.ReceivedCalls() + .Where(call => call.GetMethodInfo().Name == nameof(ILogger.Log)) + .Select(call => call.GetArguments()[2]?.ToString() ?? string.Empty)); + var createCommand = $"az ad sp create --id {ConfigConstants.DefenderApiAppId}"; + result.Should().BeFalse( + because: "a standalone bot permission command cannot succeed while Defender provisioning and consent remain pending"); + output.Should().Contain(createCommand, + because: "standalone commands have no full setup summary to supply the missing Defender provisioning step"); + output.Should().Contain(Uri.EscapeDataString( + $"{ConfigConstants.DefenderApiIdentifierUri}/{ConfigConstants.DefenderApiRealtimeProtectionScope}"), + because: "Defender is excluded from the combined URL until its service principal exists, so its separate delegated-consent URL is required"); + output.Should().Contain("New-MgServicePrincipalAppRoleAssignment", + because: "provisioning and delegated consent do not grant the required Defender application role"); + output.IndexOf(createCommand, StringComparison.Ordinal).Should().BeLessThan( + output.IndexOf("New-MgServicePrincipalAppRoleAssignment", StringComparison.Ordinal), + because: "the administrator must provision the Defender service principal before assigning its application role"); + } + finally + { + BatchPermissionsOrchestrator.BypassSpProvisioningForTests = previousBypass; + } + } + + [Fact] + public async Task ConfigureBotPermissionsAsync_AdminPath_RequestsDefenderButNotObservability() { // Arrange — admin user, OAuth2 grants succeed. var config = ArrangeAdminPath(); @@ -850,7 +938,7 @@ public async Task ConfigureBotPermissionsAsync_AdminPath_RequestsNoObservability // Assert result.Should().BeTrue( - because: "with no app role requested there is no S2S grant left that could fail the step"); + because: "Defender permissions must succeed independently of the skipped Observability permissions"); await _mockBlueprintService.Received().SetInheritablePermissionsAsync( Arg.Any(), Arg.Any(), PowerPlatformConstants.PowerPlatformApiResourceAppId, Arg.Any>(), Arg.Any?>(), Arg.Any()); @@ -858,8 +946,14 @@ await _mockBlueprintService.DidNotReceive().SetInheritablePermissionsAsync( Arg.Any(), Arg.Any(), ConfigConstants.ObservabilityApiAppId, Arg.Any>(), Arg.Any?>(), Arg.Any()); await _mockBlueprintService.DidNotReceive().GrantAppRoleAssignmentAsync( - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any>(), Arg.Any?>(), Arg.Any()); + Arg.Any(), Arg.Any(), ConfigConstants.ObservabilityApiAppId, + Arg.Any>(), Arg.Any?>(), Arg.Any(), + knownResourceSpObjectId: Arg.Any()); + await _mockBlueprintService.Received().GrantAppRoleAssignmentAsync( + Arg.Any(), Arg.Any(), ConfigConstants.DefenderApiAppId, + Arg.Is>(roles => roles.SequenceEqual(new[] { ConfigConstants.DefenderApiRealtimeProtectionScope })), + Arg.Any?>(), Arg.Any(), + knownResourceSpObjectId: "resource-sp-id"); _mockLogger.DidNotReceive().Log( LogLevel.Warning, Arg.Any(), @@ -868,6 +962,38 @@ await _mockBlueprintService.DidNotReceive().GrantAppRoleAssignmentAsync( Arg.Any>()); } + [Fact] + public async Task ConfigureBotPermissionsAsync_AdminPath_WhenS2SFailsAndNoExternalSetupResults_ReturnsFalse() + { + var config = ArrangeAdminPath(s2sSucceeds: false); + + var result = await PermissionsSubcommand.ConfigureBotPermissionsAsync( + "config.json", + _mockLogger, + _mockConfigService, + _mockExecutor, + config, + _mockGraphApiService, + _mockBlueprintService, + iSetupAll: false, + setupResults: null); + + result.Should().BeFalse( + because: "a failed Defender application grant must fail the command even without external setup results"); + _mockLogger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? string.Empty).Contains("S2S app role assignment failed")), + Arg.Any(), + Arg.Any>()); + _mockLogger.DidNotReceive().Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? string.Empty).Contains("configured successfully")), + Arg.Any(), + Arg.Any>()); + } + [Fact] public async Task ConfigureBotPermissionsAsync_WhenGraphAuthFails_DoesNotLogSuccessMessage() { @@ -967,7 +1093,15 @@ public async Task ConfigureBotPermissionsAsync_AdminPath_WhenAllSucceeds_Returns setupResults: null); // Assert - result.Should().BeTrue(because: "every permission the bot step requests was granted, so the method must return true"); + result.Should().BeTrue(because: "all grants and S2S succeed — method must return true"); + await _mockBlueprintService.Received().GrantAppRoleAssignmentAsync( + config.TenantId, + config.AgentBlueprintServicePrincipalObjectId!, + ConfigConstants.DefenderApiAppId, + Arg.Is>(roles => roles.Contains(ConfigConstants.DefenderApiRealtimeProtectionScope)), + Arg.Any?>(), + Arg.Any(), + knownResourceSpObjectId: "resource-sp-id"); _mockLogger.Received().Log( LogLevel.Information, Arg.Any(), @@ -976,11 +1110,13 @@ public async Task ConfigureBotPermissionsAsync_AdminPath_WhenAllSucceeds_Returns Arg.Any>()); } - [Fact] - public async Task ConfigureBotPermissionsAsync_WithExternalSetupResults_RecordsNoS2SAttempt() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task ConfigureBotPermissionsAsync_WithExternalSetupResults_RecordsDefenderS2SOutcome(bool s2sSucceeds) { // Arrange — non-null SetupResults passed in (AllSubcommand path). - var config = ArrangeAdminPath(); + var config = ArrangeAdminPath(s2sSucceeds); var externalResults = new SetupResults(); // Act @@ -996,12 +1132,12 @@ public async Task ConfigureBotPermissionsAsync_WithExternalSetupResults_RecordsN setupResults: externalResults); // Assert - result.Should().BeTrue( - because: "the bot step requests no app role, so it cannot fail on an S2S grant"); - externalResults.BlueprintS2SOutcome.Should().Be(Cli.Models.GrantOutcome.NotApplicable, - because: "the caller's summary must not report an S2S grant that this step never requested"); + result.Should().Be(s2sSucceeds, + because: "the command must report whether the required Defender application grant succeeded"); + externalResults.BlueprintS2SOutcome.Should().Be( + s2sSucceeds ? Cli.Models.GrantOutcome.Granted : Cli.Models.GrantOutcome.Failed, + because: "the caller's summary must reflect the Defender application grant even when Observability is skipped"); } #endregion } - diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs index dcd49757..dca31aca 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs @@ -164,6 +164,19 @@ public void SetupCommand_HasRequiredSubcommands() subcommandNames.Should().Contain("all", "Setup should have all subcommand"); } + [Fact] + public async Task SetupHelp_BotPermissions_MentionsDefender() + { + var parser = new CommandLineBuilder(BuildSetupCommand()).UseDefaults().Build(); + var console = new TestConsole(); + + var result = await parser.InvokeAsync("--help", console); + + result.Should().Be(0); + console.Out.ToString().Should().Contain("Bot API + Defender + Power Platform", + because: "the top-level setup help must disclose Defender alongside the other permissions configured by permissions bot"); + } + [Fact] public void SetupCommand_PermissionsSubcommand_HasMcpAndBotSubcommands() { @@ -732,14 +745,16 @@ public async Task SetupAll_BlueprintAgent_DefaultPlan_OmitsObservabilityApi() } /// - /// The S2S endpoint authorizes registered agents without OtelWrite whatever the auth mode (validated live), so - /// s2s and both — from the flag or from a365.config.json — must not request Observability API permissions either. + /// The S2S endpoint authorizes registered agents without OtelWrite, while Defender still + /// contributes an application role for s2s and both modes. /// [Theory] [InlineData("--authmode s2s", null)] [InlineData("--authmode both", null)] [InlineData("", "both")] - public async Task SetupAll_BlueprintAgent_AppRoleAuthModes_OmitObservabilityApi(string args, string? configAuthMode) + public async Task SetupAll_BlueprintAgent_AppRoleAuthModes_OmitObservabilityAndKeepDefenderAppRole( + string args, + string? configAuthMode) { var config = new Agent365Config { @@ -772,7 +787,9 @@ public async Task SetupAll_BlueprintAgent_AppRoleAuthModes_OmitObservabilityApi( _mockLogger.Received().Log( LogLevel.Information, Arg.Any(), - Arg.Is(o => o.ToString()!.Contains("Blueprint Permission Grants") && o.ToString()!.Contains("no S2S app roles to grant")), + Arg.Is(o => o.ToString()!.Contains("Blueprint Permission Grants") && + o.ToString()!.Contains("S2S app roles") && + !o.ToString()!.Contains("no S2S app roles to grant")), 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 63b64cd5..8ca94b16 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 @@ -89,9 +89,10 @@ public async Task DwPath_NoManifest_NoCustom_ProducesBaselineSpecSet() AuthenticationConstants.MicrosoftGraphResourceAppId, ConfigConstants.MessagingBotApiAppId, ConfigConstants.ObservabilityApiAppId, + ConfigConstants.DefenderApiAppId, PowerPlatformConstants.PowerPlatformApiResourceAppId, McpConstants.WorkIQToolsProdAppId, - }, because: "the DW baseline spec set is the four fixed platform APIs plus the ATG AppId (seeded with McpServersMetadata.Read.All for V1 compatibility)"); + }, because: "the DW baseline spec set is the five fixed platform APIs (including the Defender API required by the security integration) plus the ATG AppId (seeded with McpServersMetadata.Read.All for V1 compatibility)"); // Assert: ATG entry carries only the seeded V1-compat scope when no manifest is present. SpecFor(specs, McpConstants.WorkIQToolsProdAppId).Scopes.Should().BeEquivalentTo(new[] { McpServersMetadataReadAll }, @@ -280,6 +281,7 @@ public async Task Unified_WithManifest_IsM365_StampsFullSet() AuthenticationConstants.MicrosoftGraphResourceAppId, ConfigConstants.MessagingBotApiAppId, ConfigConstants.ObservabilityApiAppId, + ConfigConstants.DefenderApiAppId, PowerPlatformConstants.PowerPlatformApiResourceAppId, McpConstants.WorkIQToolsProdAppId, }, because: "with a manifest present and isM365 true, blueprint agents must receive the same spec set as DW agents — this is the unified-pipeline contract"); @@ -306,6 +308,34 @@ 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 DefenderApi_CarriesBothDelegatedScopeAndAppRole() + { + // Arrange: smallest config that produces the Defender spec on either path. + var config = new Agent365Config { DeploymentProjectPath = _tempDir }; + + // Act + var specs = await SetupHelpers.BuildConfiguredPermissionSpecsAsync(config, setInheritable: true, isM365: true); + + // Assert + var defender = SpecFor(specs, ConfigConstants.DefenderApiAppId); + defender.Scopes.Should().BeEquivalentTo(new[] { ConfigConstants.DefenderApiRealtimeProtectionScope }, + because: "the Defender API delegated scope grants RealtimeProtection.Evaluate.All for the OBO path"); + defender.AppRoleScopes.Should().BeEquivalentTo(new[] { ConfigConstants.DefenderApiRealtimeProtectionScope }, + because: "the Defender API app role grants RealtimeProtection.Evaluate.All for the s2s path - the Defender webhook rejects tokens without the roles claim, so losing either side breaks one auth mode"); + defender.SetInheritable.Should().BeTrue( + because: "agent identities minted from the blueprint must inherit the Defender permission, exactly as they do for OtelWrite"); + } + + [Fact] + public void DefenderApi_ScopeValue_MatchesValuePublishedOnResource() + { + // A value the resource SP does not publish fails the combined consent URL for every + // resource in it (AADSTS650053), not just Defender. + ConfigConstants.DefenderApiRealtimeProtectionScope.Should().Be("RealtimeProtection.Evaluate.All", + because: "this is the app role and delegated scope value published on the Defender resource SP; changing it without a matching resource-side change fails admin consent tenant-wide"); + } + [Fact] public async Task MessagingBotApi_UsesScopeConstantSoSpecAndConsentUrlAgree() { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersAdminConsentInstructionsTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersAdminConsentInstructionsTests.cs index 03fcd851..fe5d1736 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersAdminConsentInstructionsTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersAdminConsentInstructionsTests.cs @@ -61,6 +61,22 @@ public void LogNonDwAdminConsentInstructions_OptionA_ShowsDelegatedPermissionsIn because: "only delegated grants are needed for OBO — no Application permissions"); } + [Fact] + public void LogNonDwAdminConsentInstructions_OptionA_ShowsDefenderDelegatedPermission() + { + var logger = new CapturingLogger(); + + SetupHelpers.LogNonDwAdminConsentInstructions(logger, BlueprintId); + + var defenderLines = logger.Messages + .Where(m => m.Contains("Defender API") && m.Contains(ConfigConstants.DefenderApiRealtimeProtectionScope)) + .ToList(); + defenderLines.Should().HaveCount(1, + because: "the Defender API delegated scope must appear exactly once so the admin grants it alongside the other platform APIs"); + defenderLines[0].Should().Contain("Delegated", + because: "only delegated grants are needed for OBO — no Application permissions"); + } + [Fact] public void LogNonDwAdminConsentInstructions_DoesNotEmitOptionBPowerShell() { @@ -146,6 +162,26 @@ public void GetNonDwAdminConsentSpecs_ForGcc_UsesGccObservabilityResource() because: "manual GCC consent instructions must target the GCC Observability resource"); } + [Theory] + [InlineData("Delegated", true, false)] + [InlineData("Application", false, true)] + [InlineData("Both", true, true)] + public void GetNonDwAdminConsentSpecs_DefenderPermissionsMatchAuthMode( + string modeName, + bool expectDelegated, + bool expectApplication) + { + var mode = Enum.Parse(modeName); + var specs = SetupHelpers.GetNonDwAdminConsentSpecs("prod", mode) + .Where(spec => spec.ResourceName == "Defender API") + .ToList(); + + specs.Any(spec => spec.PermissionType == "Delegated").Should().Be(expectDelegated, + because: $"{mode} must include delegated Defender consent exactly when OBO is enabled"); + specs.Any(spec => spec.PermissionType == "Application").Should().Be(expectApplication, + because: $"{mode} must include the Defender app role exactly when S2S is enabled"); + } + [Fact] public void NonDwAdminConsentSpecs_PowerPlatformApi_IsDelegatedOnly() { 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 cfa16533..7afca5c1 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 @@ -26,13 +26,14 @@ public void BuildAdminConsentUrls_WithGraphAndMcpScopes_ReturnsUrlForEachResourc var urls = SetupHelpers.BuildAdminConsentUrls(TenantId, BlueprintClientId, graphScopes, mcpScopes); - urls.Should().HaveCount(5); + urls.Should().HaveCount(6); urls.Select(u => u.ResourceName).Should().Contain(new[] { "Microsoft Graph", "Agent 365 Tools", "Messaging Bot API", "Observability API", + "Defender API", "Power Platform API" }); } @@ -72,6 +73,16 @@ public void BuildAdminConsentUrls_ObservabilityApi_UsesCorrectScopeConstant() because: "OtelWrite is the published delegated scope on the Observability API used for admin consent"); } + [Fact] + public void BuildAdminConsentUrls_DefenderApi_UsesAppIdIdentifierUri() + { + var urls = SetupHelpers.BuildAdminConsentUrls(TenantId, BlueprintClientId, new[] { "Mail.Send" }, new[] { "scope" }); + var defenderUrl = urls.First(u => u.ResourceName == "Defender API").ConsentUrl; + + defenderUrl.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.DefenderApiIdentifierUri}/{ConfigConstants.DefenderApiRealtimeProtectionScope}"), + because: "the Defender resource publishes its delegated permission under the api://{appId} audience"); + } + [Fact] public void BuildAdminConsentUrls_PowerPlatformApi_UsesCorrectScopeConstant() { @@ -215,9 +226,9 @@ public void BuildCombinedConsentUrl_IncludesAllMcpScopes() } [Fact] - public void BuildCombinedConsentUrl_AlwaysIncludesAllThreeFixedResources() + public void BuildCombinedConsentUrl_AlwaysIncludesAllFixedResources() { - // Even with empty graph and MCP scopes, the three fixed resources must be present + // Even with empty graph and MCP scopes, the fixed resources must be present var url = SetupHelpers.BuildCombinedConsentUrl( TenantId, BlueprintClientId, Array.Empty(), Array.Empty()); @@ -226,6 +237,8 @@ public void BuildCombinedConsentUrl_AlwaysIncludesAllThreeFixedResources() because: "scope URIs are Uri.EscapeDataString-encoded in the query string — required by AAD for adminconsent"); url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiOtelWriteScope}"), because: "OtelWrite is the published delegated scope on the Observability API used for admin consent"); + url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.DefenderApiIdentifierUri}/{ConfigConstants.DefenderApiRealtimeProtectionScope}"), + because: "RealtimeProtection.Evaluate.All is the published delegated scope on the Defender API - without it the agent cannot call the Defender security webhook"); url.Should().Contain(Uri.EscapeDataString($"{PowerPlatformConstants.PowerPlatformApiIdentifierUri}/{PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead}")); } @@ -288,8 +301,9 @@ public void BuildAdminConsentUrls_NonM365_ExcludesMessagingBotButKeepsAllOthers( "Microsoft Graph", "Agent 365 Tools", "Observability API", + "Defender API", "Power Platform API", - }, because: "non-M365 tenants lack the Messaging Bot resource SP — Bot would cause AADSTS650053 if included"); + }, because: "non-M365 tenants lack the Messaging Bot resource SP — Bot would cause AADSTS650053 if included; the Defender API is required for the security integration on every agent regardless of M365 surface"); } [Fact] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersDisplaySetupSummaryTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersDisplaySetupSummaryTests.cs index e80a6fe2..a0487e39 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersDisplaySetupSummaryTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersDisplaySetupSummaryTests.cs @@ -432,6 +432,151 @@ public void DisplaySetupSummary_NonDwAdminConsentPending_WithConsentUrl_ShowsUrl because: "the portal walkthrough is only a defensive fallback for the case where no consent URL was produced; when a URL is available the URL block must be used"); } + [Fact] + public void DisplaySetupSummary_NonDwAdminConsentPending_PrefersFilteredAdminConsentUrl() + { + var logger = new CapturingLogger(); + const string filteredUrl = "https://login.microsoftonline.com/tenant/v2.0/adminconsent?scope=filtered"; + const string reconstructedUrl = "https://login.microsoftonline.com/tenant/v2.0/adminconsent?scope=includes-missing-defender"; + var results = new SetupResults + { + IsNonDwBlueprintFlow = true, + BlueprintCreated = true, + BlueprintId = BlueprintId, + AgentIdentityCreated = true, + AgentIdentityId = AgentSpId, + TenantId = TenantId, + EffectiveAuthMode = Cli.Models.AuthMode.Obo, + TenantWideConsentOutcome = Cli.Models.GrantOutcome.Failed, + BatchPermissionsPhase1Completed = true, + BatchPermissionsPhase2Completed = true, + AdminConsentUrl = filteredUrl, + CombinedConsentUrl = reconstructedUrl, + }; + + SetupHelpers.DisplaySetupSummary(results, logger); + + logger.AllOutput.Should().Contain(filteredUrl, + because: "the orchestrator URL excludes unresolved service principals and is the only safe primary handoff"); + logger.AllOutput.Should().NotContain(reconstructedUrl, + because: "a reconstructed URL can reintroduce a missing resource and fail the entire consent request"); + } + + [Fact] + public void DisplaySetupSummary_NonDwGccS2sPending_UsesPendingCloudSpecificSpec() + { + var logger = new CapturingLogger(); + var results = new SetupResults + { + IsNonDwBlueprintFlow = true, + BlueprintCreated = true, + BlueprintId = BlueprintId, + AgentIdentityCreated = true, + AgentIdentityId = AgentSpId, + TenantId = TenantId, + EffectiveAuthMode = Cli.Models.AuthMode.S2s, + TenantWideConsentOutcome = Cli.Models.GrantOutcome.Granted, + BlueprintS2SOutcome = Cli.Models.GrantOutcome.Failed, + BatchPermissionsPhase1Completed = true, + BatchPermissionsPhase2Completed = true, + }; + results.PendingBlueprintAppRoleSpecs.Add(new ResourcePermissionSpec( + ConfigConstants.GccObservabilityApiAppId, + "Observability API", + [], + SetInheritable: true, + AppRoleScopes: [ConfigConstants.ObservabilityApiOtelWriteScope])); + + SetupHelpers.DisplaySetupSummary(results, logger); + + logger.AllOutput.Should().Contain(ConfigConstants.GccObservabilityApiAppId, + because: "manual S2S recovery must use the cloud-aware resource from the failed permission spec"); + logger.AllOutput.Should().NotContain(ConfigConstants.ObservabilityApiAppId, + because: "GCC recovery must not target the commercial Observability application"); + } + + [Fact] + public void DisplaySetupSummary_MissingSp_ProvisioningPrecedesDependentPermissionActions() + { + var logger = new CapturingLogger(); + var results = new SetupResults + { + BlueprintCreated = true, + BlueprintId = BlueprintId, + TenantId = TenantId, + BatchPermissionsPhase1Completed = true, + BatchPermissionsPhase2Completed = true, + BlueprintS2SOutcome = Cli.Models.GrantOutcome.Failed, + TenantWideConsentOutcome = Cli.Models.GrantOutcome.Failed, + AdminConsentUrl = NoAppRolesConsentUrl, + }; + results.PendingBlueprintAppRoleSpecs.Add(new ResourcePermissionSpec( + ConfigConstants.DefenderApiAppId, + "Defender API", + [], + SetInheritable: true, + AppRoleScopes: [ConfigConstants.DefenderApiRealtimeProtectionScope])); + var createCommand = $"az ad sp create --id {ConfigConstants.DefenderApiAppId}"; + results.MissingSpActions.Add(new MissingSpAction( + "Defender API", ConfigConstants.DefenderApiAppId, [], createCommand, null) + { + AppRoleScopes = [ConfigConstants.DefenderApiRealtimeProtectionScope], + }); + + SetupHelpers.DisplaySetupSummary(results, logger); + + logger.AllOutput.Should().Contain(createCommand, + because: "a missing Defender service principal must have a provisioning action"); + logger.AllOutput.Should().Contain("New-MgServicePrincipalAppRoleAssignment", + because: "provisioning alone does not grant the pending Defender application role"); + logger.AllOutput.Should().Contain("Permission Grants", + because: "provisioning must preserve the delegated consent handoff for other resources"); + var provisioningIndex = logger.AllOutput.IndexOf(createCommand, StringComparison.Ordinal); + provisioningIndex.Should().BeLessThan( + logger.AllOutput.IndexOf("New-MgServicePrincipalAppRoleAssignment", StringComparison.Ordinal), + because: "numbered recovery steps must provision the resource before assigning its application role"); + provisioningIndex.Should().BeLessThan( + logger.AllOutput.IndexOf("Permission Grants — forward", StringComparison.Ordinal), + because: "resource provisioning must precede any dependent delegated consent handoff"); + } + + [Fact] + public void DisplaySetupSummary_ApplicationOnlyMissingSp_ShowsProvisioningWithoutConsentUrl() + { + var logger = new CapturingLogger(); + var results = new SetupResults + { + IsNonDwBlueprintFlow = true, + BlueprintCreated = true, + BlueprintId = BlueprintId, + AgentIdentityCreated = true, + AgentIdentityId = AgentSpId, + TenantId = TenantId, + EffectiveAuthMode = Cli.Models.AuthMode.S2s, + TenantWideConsentOutcome = Cli.Models.GrantOutcome.Granted, + BatchPermissionsPhase1Completed = true, + BatchPermissionsPhase2Completed = true, + }; + results.MissingSpActions.Add(new MissingSpAction( + "Defender API", + ConfigConstants.DefenderApiAppId, + [], + $"az ad sp create --id {ConfigConstants.DefenderApiAppId}", + null) + { + AppRoleScopes = [ConfigConstants.DefenderApiRealtimeProtectionScope], + }); + + SetupHelpers.DisplaySetupSummary(results, logger); + + logger.AllOutput.Should().Contain($"az ad sp create --id {ConfigConstants.DefenderApiAppId}", + because: "application-only recovery must tell the administrator how to provision the missing Defender service principal"); + logger.AllOutput.Should().Contain($"Application roles pending: {ConfigConstants.DefenderApiRealtimeProtectionScope}", + because: "the handoff must identify the Defender app role that can be assigned after provisioning"); + logger.AllOutput.Should().NotContain("Step 2)", + because: "an application-only spec has no delegated scope and must not emit an empty admin-consent URL"); + } + [Fact] public void DisplaySetupSummary_NonDwAdminConsentPending_NoConsentUrl_FallsBackToPortalWalkthrough() { @@ -754,6 +899,7 @@ public void DisplaySetupSummary_DwAdmin_TenantConsentUnverified_PermissionGrants BatchPermissionsPhase1Completed = true, BatchPermissionsPhase2Completed = true, TenantWideConsentOutcome = Cli.Models.GrantOutcome.Unverified, + BlueprintS2SOutcome = Cli.Models.GrantOutcome.Granted, AdminConsentUrl = "https://login.microsoftonline.com/tenant/adminconsent?client_id=bp-id", }; @@ -767,6 +913,25 @@ public void DisplaySetupSummary_DwAdmin_TenantConsentUnverified_PermissionGrants because: "Unverified must not claim the grant succeeded — the consent was not directly observed"); grantsRow!.Should().NotContain("granted tenant-wide", because: "the Unverified row must not use the Granted row text"); + grantsRow.Should().NotContain("granted S2S app roles", + because: "a successful Defender application grant must not hide unverified tenant-wide delegated consent"); + } + + [Fact] + public void DisplaySetupSummary_DwAdmin_S2SGrantedAndTenantConsentFailed_PermissionGrantsRowShowsPending() + { + var logger = new CapturingLogger(); + var results = BuildS2SVerifiedResults(); + results.TenantWideConsentOutcome = Cli.Models.GrantOutcome.Failed; + + SetupHelpers.DisplaySetupSummary(results, logger); + + var grantsRow = System.Array.Find(logger.AllOutput.Split('\n'), line => line.Contains("Blueprint Permission Grants")); + grantsRow.Should().NotBeNull(); + grantsRow!.Should().Contain("PENDING", + because: "AI Teammates require delegated consent as well as the Defender application role"); + grantsRow.Should().NotContain("granted", + because: "the permission row must not claim completion while tenant-wide consent is still missing"); } [Fact] @@ -862,6 +1027,8 @@ public void DisplaySetupSummary_BlueprintOnly_EmitsPermissionsNextSteps() because: "the blueprint summary must point to the remaining MCP permissions step"); logger.AllOutput.Should().Contain("a365 setup permissions bot", because: "the blueprint summary must point to the bot permissions step"); + logger.AllOutput.Should().Contain("Defender", + because: "the permissions bot next step configures Defender and must disclose that work"); } [Fact] @@ -1028,21 +1195,53 @@ public void DisplaySetupSummary_TenantConsentNewlyGranted_RendersGranted() because: "the 'already granted' wording is reserved for runs that did no work — using it for first-time grants would mislead the user"); } - /// - /// S2S parity: when every S2S app role assignment was already in place - /// (BlueprintS2SAlreadyAssigned=true), the row must render "already granted S2S app roles". - /// - [Fact] - public void DisplaySetupSummary_S2SAllAlreadyAssigned_RendersAlreadyGranted() + [Theory] + [InlineData(false, false, false)] + [InlineData(false, false, true)] + [InlineData(false, true, false)] + [InlineData(false, true, true)] + [InlineData(true, false, false)] + [InlineData(true, false, true)] + [InlineData(true, true, false)] + [InlineData(true, true, true)] + public void DisplaySetupSummary_BothGrantsSucceeded_ReportsBothPermissionTypes( + bool isBlueprintBothMode, bool appRolesAlreadyAssigned, bool tenantConsentAlreadyExisted) { var logger = new CapturingLogger(); - var results = BuildS2SVerifiedResults(); + var results = BuildS2SVerifiedResults(isBlueprintBothMode); + results.BlueprintS2SAlreadyAssigned = appRolesAlreadyAssigned; + results.TenantWideConsentAlreadyExisted = tenantConsentAlreadyExisted; + + SetupHelpers.DisplaySetupSummary(results, logger); + + var grantsRow = System.Array.Find(logger.AllOutput.Split('\n'), line => line.Contains("Blueprint Permission Grants")); + grantsRow.Should().NotBeNull(); + grantsRow!.Should().Contain("granted S2S app roles + tenant-wide delegated", + because: "AI Teammates and blueprint both-mode agents require tenant-wide delegated consent alongside application roles"); + grantsRow!.Contains("already granted", StringComparison.Ordinal).Should().Be( + appRolesAlreadyAssigned && tenantConsentAlreadyExisted, + because: "combined grants are already in place only when both application roles and tenant-wide delegated consent predate this run"); + } + + [Theory] + [InlineData(Cli.Models.GrantOutcome.Unverified, "unverified")] + [InlineData(Cli.Models.GrantOutcome.Failed, "PENDING")] + public void DisplaySetupSummary_BlueprintBothMode_S2SGranted_ReportsUnfinishedTenantConsent( + Cli.Models.GrantOutcome tenantConsentOutcome, string expectedStatus) + { + var logger = new CapturingLogger(); + var results = BuildS2SVerifiedResults(isBlueprintBothMode: true); + results.TenantWideConsentOutcome = tenantConsentOutcome; results.BlueprintS2SAlreadyAssigned = true; SetupHelpers.DisplaySetupSummary(results, logger); - logger.AllOutput.Should().Contain("already granted S2S app roles", - because: "when every S2S role was already assigned the row must surface that — a future regression that drops the flag would silently report 'granted' on every idempotent re-run"); + var grantsRow = System.Array.Find(logger.AllOutput.Split('\n'), line => line.Contains("Blueprint Permission Grants")); + grantsRow.Should().NotBeNull(); + grantsRow!.Should().Contain(expectedStatus, + because: "blueprint both mode relies on tenant-wide delegated consent, not a per-principal grant recorded only by test fixtures"); + grantsRow.Should().NotContain("granted", + because: "a successful or pre-existing application role must not hide unverified or failed delegated consent"); } /// @@ -1272,20 +1471,18 @@ public void DisplaySetupSummary_InheritablePermissionsNewlyConfigured_RendersCon }; /// - /// Builds a DW-flow results object on the S2S-granted branch of the Blueprint Permission - /// Grants row. DW because the summary's s2sOk gate maps to blueprintS2sGranted - /// for DW and agentIdS2sGranted for non-DW — using DW keeps the flag's name aligned - /// with the field that drives s2sOk for this test, isolating the - /// toggle from non-DW row mechanics. + /// Builds a flow with verified application roles and tenant-wide consent, without a per-principal delegated grant. /// - private static SetupResults BuildS2SVerifiedResults() => new() + private static SetupResults BuildS2SVerifiedResults(bool isBlueprintBothMode = false) => new() { - // DW flow — no EffectiveAuthMode, no agent identity row. - IsNonDwBlueprintFlow = false, + IsNonDwBlueprintFlow = isBlueprintBothMode, + EffectiveAuthMode = isBlueprintBothMode ? Cli.Models.AuthMode.Both : null, + AgentIdentityS2SOutcome = isBlueprintBothMode ? Cli.Models.GrantOutcome.Granted : Cli.Models.GrantOutcome.NotApplicable, BlueprintCreated = true, BlueprintId = BlueprintId, TenantId = TenantId, BlueprintS2SOutcome = Cli.Models.GrantOutcome.Granted, + TenantWideConsentOutcome = Cli.Models.GrantOutcome.Granted, BatchPermissionsPhase1Completed = true, BatchPermissionsPhase2Completed = true, }; diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersLogPermissionsActionRequiredTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersLogPermissionsActionRequiredTests.cs index bcba65f3..85b9e46a 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersLogPermissionsActionRequiredTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersLogPermissionsActionRequiredTests.cs @@ -127,6 +127,29 @@ public void NoActions_EmitsGenericFallback() because: "the structured block must only appear when at least one concrete action is available"); } + [Fact] + public void MissingSpOnly_EmitsProvisioningAndResourceConsent() + { + var logger = new CapturingLogger(); + var results = BuildResults(); + var createCommand = $"az ad sp create --id {ConfigConstants.DefenderApiAppId}"; + var consentUrl = SetupHelpers.BuildAdminConsentUrl( + TenantId, BlueprintId, + [$"{ConfigConstants.DefenderApiIdentifierUri}/{ConfigConstants.DefenderApiRealtimeProtectionScope}"]); + results.MissingSpActions.Add(new MissingSpAction( + "Defender API", ConfigConstants.DefenderApiAppId, + [ConfigConstants.DefenderApiRealtimeProtectionScope], createCommand, consentUrl)); + + SetupHelpers.LogPermissionsActionRequired(logger, results, [], adminConsentUrl: null); + + logger.AllOutput.Should().Contain("Action Required", + because: "missing resource provisioning remains actionable even without a combined consent URL or application-role script"); + logger.AllOutput.Should().Contain(createCommand, + because: "the administrator must be given the command to provision the missing service principal"); + logger.AllOutput.Should().Contain(consentUrl, + because: "a resource excluded from combined consent still needs its own delegated-consent handoff"); + } + [Fact] public void AppRoleSpecs_EmitNewMgServicePrincipalAppRoleAssignment() { 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 c4eff2dc..5f9a89d7 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 @@ -41,6 +41,164 @@ await executor.Received(2).ExecuteAsync( Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } + [Fact] + public async Task PollAdminConsentAsync_DoesNotCompleteOnUnrelatedExistingGrant() + { + var executor = Substitute.For(Substitute.For>()); + var logger = Substitute.For(); + executor.ExecuteAsync( + "az", + Arg.Is(args => args.Contains("servicePrincipals")), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(Task.FromResult(new CommandResult + { + ExitCode = 0, + StandardOutput = """{"value":[{"id":"blueprint-sp"}]}""", + })); + + var unrelatedGrant = """ + { + "value": [ + { + "resourceId": "graph-sp", + "consentType": "AllPrincipals", + "scope": "User.Read" + } + ] + } + """; + var completeGrantSet = """ + { + "value": [ + { + "resourceId": "graph-sp", + "consentType": "AllPrincipals", + "scope": "User.Read" + }, + { + "resourceId": "defender-sp", + "consentType": "AllPrincipals", + "scope": "RealtimeProtection.Evaluate.All" + } + ] + } + """; + executor.ExecuteAsync( + "az", + Arg.Is(args => args.Contains("oauth2PermissionGrants")), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns( + Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = unrelatedGrant }), + Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = completeGrantSet })); + + var requirements = new[] + { + new AdminConsentRequirement( + "Defender API", + "86a21212-634e-4553-b3d6-e477e4c9d9ec", + ["RealtimeProtection.Evaluate.All"], + "defender-sp"), + }; + + var result = await AdminConsentHelper.PollAdminConsentAsync( + executor, + logger, + "11111111-1111-1111-1111-111111111111", + "All permissions", + timeoutSeconds: 10, + intervalSeconds: 0, + CancellationToken.None, + requiredGrants: requirements); + + result.Should().BeTrue( + because: "polling must wait until the requested Defender grant appears instead of completing on the pre-existing Graph grant"); + await executor.Received(2).ExecuteAsync( + "az", + Arg.Is(args => args.Contains("oauth2PermissionGrants")), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()); + } + + [Fact] + public void GrantsSatisfyRequirements_RejectsPrincipalAndUnrelatedGrants() + { + using var grants = JsonDocument.Parse(""" + { + "value": [ + { + "resourceId": "graph-sp", + "consentType": "AllPrincipals", + "scope": "RealtimeProtection.Evaluate.All" + }, + { + "resourceId": "defender-sp", + "consentType": "Principal", + "scope": "RealtimeProtection.Evaluate.All" + } + ] + } + """); + var requirements = new[] + { + new AdminConsentRequirement( + "Defender API", + "86a21212-634e-4553-b3d6-e477e4c9d9ec", + ["RealtimeProtection.Evaluate.All"], + "defender-sp"), + }; + + var result = AdminConsentHelper.GrantsSatisfyRequirements( + grants.RootElement.GetProperty("value"), + requirements); + + result.Should().BeFalse( + because: "only an AllPrincipals grant on the requested Defender resource may complete tenant-wide consent polling"); + } + + [Fact] + public void GrantsSatisfyRequirements_AggregatesScopesAcrossGrantRows() + { + using var grants = JsonDocument.Parse(""" + { + "value": [ + { + "resourceId": "resource-sp", + "consentType": "AllPrincipals", + "scope": "Scope.One" + }, + { + "resourceId": "resource-sp", + "consentType": "AllPrincipals", + "scope": "Scope.Two" + } + ] + } + """); + var requirements = new[] + { + new AdminConsentRequirement( + "Contoso API", + "22222222-2222-2222-2222-222222222222", + ["Scope.One", "Scope.Two"], + "resource-sp"), + }; + + var result = AdminConsentHelper.GrantsSatisfyRequirements( + grants.RootElement.GetProperty("value"), + requirements); + + result.Should().BeTrue( + because: "incremental Entra consent can split required scopes across multiple AllPrincipals grant rows"); + } + [Fact] public async Task PollAdminConsentAsync_PropagatesCancellation_WhenTokenCanceled() { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs index fb6957a7..b3b0d39f 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs @@ -953,6 +953,120 @@ private static IAuthenticationService FakeAuth() // ── GrantAppRoleAssignmentAsync tests ────────────────────────────────────── + [Fact] + public async Task GrantAppRoleAssignmentAsync_WithKnownResourceSpId_UsesItWithoutAppIdLookup() + { + var (service, handler) = CreateServiceWithFakeHandler(); + using (handler) + { + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[{\"value\":\"Defender.Evaluate\",\"id\":\"role-id\"}]}") + }); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Created) + { + Content = new StringContent("{\"id\":\"assignment-id\"}") + }); + + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", ["Defender.Evaluate"], + knownResourceSpObjectId: "recovered-resource-sp-id"); + + result.AllSucceeded.Should().BeTrue( + because: "a known resource SP must be usable even when its app-ID lookup has not replicated"); + result.AllAlreadyAssigned.Should().BeFalse( + because: "a newly created assignment must not be reported as an existing grant"); + handler.SentRequests.Should().NotContain(request => request.Url.Contains("$filter="), + because: "recovery's authoritative object ID must not be discarded in favor of another app-ID lookup"); + using var body = JsonDocument.Parse(handler.SentRequests.Single(request => request.Method == HttpMethod.Post).Body!); + body.RootElement.GetProperty("resourceId").GetString().Should().Be("recovered-resource-sp-id", + because: "the app-role assignment must target the recovered resource service principal"); + } + } + + [Fact] + public async Task GrantAppRoleAssignmentAsync_WithKnownResourceSpIdAndExistingRole_DoesNotPost() + { + var (service, handler) = CreateServiceWithFakeHandler(); + using (handler) + { + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[{\"value\":\"Defender.Evaluate\",\"id\":\"role-id\"}]}") + }); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"resourceId\":\"recovered-resource-sp-id\",\"appRoleId\":\"role-id\"}]}") + }); + + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", ["Defender.Evaluate"], + knownResourceSpObjectId: "recovered-resource-sp-id"); + + result.AllSucceeded.Should().BeTrue( + because: "using a known resource ID must preserve idempotent success for an existing assignment"); + result.AllAlreadyAssigned.Should().BeTrue( + because: "the summary must distinguish existing assignments from newly created grants"); + handler.SentRequests.Should().NotContain(request => request.Method == HttpMethod.Post, + because: "a recovered ID must not cause a duplicate application-role assignment"); + } + } + + [Theory] + [InlineData(HttpStatusCode.Forbidden)] + [InlineData(HttpStatusCode.NotFound)] + public async Task GrantAppRoleAssignmentAsync_WithKnownResourceSpId_WhenResourceReadFails_DoesNotGrant(HttpStatusCode status) + { + var (service, handler) = CreateServiceWithFakeHandler(); + using (handler) + { + handler.QueueResponse(new HttpResponseMessage(status) + { + Content = new StringContent("{\"error\":{\"code\":\"ResourceUnavailable\"}}") + }); + + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", ["Defender.Evaluate"], + knownResourceSpObjectId: "recovered-resource-sp-id"); + + result.AllSucceeded.Should().BeFalse( + because: "successful SP provisioning does not prove an application permission was granted"); + result.AllAlreadyAssigned.Should().BeFalse( + because: "an unreadable resource must not be reported as an existing grant"); + handler.SentRequests.Should().NotContain(request => request.Method == HttpMethod.Post, + because: "the app-role ID must be read successfully before any assignment is attempted"); + } + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public async Task GrantAppRoleAssignmentAsync_WithoutKnownResourceSpId_PreservesAppIdLookup(string? knownSpId) + { + var (service, handler) = CreateServiceWithFakeHandler(); + using (handler) + { + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); + + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", ["Defender.Evaluate"], + knownResourceSpObjectId: knownSpId); + + result.AllSucceeded.Should().BeFalse( + because: "an unresolved resource must still fail explicitly instead of reporting a successful grant"); + handler.SentRequests.Should().ContainSingle(request => request.Url.Contains("$filter="), + because: "callers without a known resource ID must retain the existing app-ID lookup behavior"); + } + } + [Fact] public async Task GrantAppRoleAssignmentAsync_WhenResourceSpNotFound_ReturnsFalse() { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/LogRedactionServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/LogRedactionServiceTests.cs index 78a76a99..98a3c98a 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/LogRedactionServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/LogRedactionServiceTests.cs @@ -107,6 +107,17 @@ public void Redact_ObservabilityResourceAppId_IsPreserved(string appId) because: "well-known first-party resource IDs do not identify a tenant or user"); } + [Fact] + public void Redact_DefenderResourceAppId_IsPreserved() + { + var result = _sut.Redact($"[INF] Defender resource: {ConfigConstants.DefenderApiAppId}", Source); + + result.RedactedContent.Should().Contain(ConfigConstants.DefenderApiAppId, + because: "the public Defender resource ID must remain visible for support diagnostics"); + result.IdsRedacted.Should().Be(0, + because: "a well-known first-party resource ID does not identify a tenant or user"); + } + [Fact] public void Redact_SameGuidAppearsMultipleTimes_SameAliasUsed() {