From 04dd20fb212f6ca178f79ab2f20b417d825ee9b5 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Mon, 13 Apr 2026 17:44:35 -0700 Subject: [PATCH 1/3] Add S2S app role assignment support to setup process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enables automatic S2S (service-to-service) app role assignments during agent blueprint setup, specifically for the Observability API OtelWrite scope. Adds AppRoleScopes to ResourcePermissionSpec, a new GrantAppRoleAssignmentAsync method in AgentBlueprintService, and orchestration logic in BatchPermissionsOrchestrator to grant S2S roles after delegated grants. Separates AppRoleAssignment.ReadWrite.All into a dedicated RequiredS2SGrantScopes constant to prevent the admin-only scope from being requested on non-admin paths (a365 deploy, setup permissions). Fixes the admin consent URL for the Observability API — replaces the non-existent Maven.ReadWrite.All scope with Agent365.Observability.OtelWrite, matching the actual delegated and application permissions on the resource. Updates setup summaries, consent URL builders, and error reporting for clarity. Includes unit tests for all S2S assignment scenarios. --- .../Commands/SetupCommand.cs | 2 +- .../SetupSubcommands/AdminSubcommand.cs | 6 +- .../BatchPermissionsOrchestrator.cs | 93 +++++++++- .../ResourcePermissionSpec.cs | 3 +- .../Commands/SetupSubcommands/SetupHelpers.cs | 122 ++++++++----- .../Commands/SetupSubcommands/SetupResults.cs | 6 + .../Constants/AuthenticationConstants.cs | 13 +- .../Constants/ConfigConstants.cs | 7 - .../Services/AgentBlueprintService.cs | 138 ++++++++++++++ .../Helpers/SetupHelpersConsentUrlTests.cs | 4 +- .../Services/AgentBlueprintServiceTests.cs | 170 +++++++++++++++++- 11 files changed, 496 insertions(+), 68 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupCommand.cs index f88b9e12..6824f52f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupCommand.cs @@ -74,7 +74,7 @@ public static Command CreateCommand( logger, configService, executor, botConfigurator, authValidator, platformDetector, graphApiService, blueprintService, clientAppValidator, blueprintLookupService, federatedCredentialService, armApiService)); command.AddCommand(AdminSubcommand.CreateCommand( - logger, configService, authValidator, graphApiService, confirmationProvider)); + logger, configService, authValidator, graphApiService, confirmationProvider, blueprintService)); return command; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs index 8d2c257e..bb62d35d 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs @@ -38,7 +38,8 @@ public static Command CreateCommand( IConfigService configService, AzureAuthValidator authValidator, GraphApiService graphApiService, - IConfirmationProvider confirmationProvider) + IConfirmationProvider confirmationProvider, + AgentBlueprintService? blueprintService = null) { var command = new Command( "admin", @@ -215,7 +216,8 @@ await BatchPermissionsOrchestrator.GrantAdminPermissionsAsync( graphApiService, setupConfig, setupConfig.AgentBlueprintId!, setupConfig.TenantId, specs, logger, setupResults, ct, - knownBlueprintSpObjectId: setupConfig.AgentBlueprintServicePrincipalObjectId); + knownBlueprintSpObjectId: setupConfig.AgentBlueprintServicePrincipalObjectId, + blueprintService: blueprintService); setupResults.AdminConsentGranted = grantsConfigured; 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 c200713b..4e78ce97 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs @@ -171,6 +171,13 @@ internal static class BatchPermissionsOrchestrator var grantsOk = await ConfigureOauth2GrantsAsync( graph, blueprintAppId, tenantId, specs, phase1Result, permScopes, logger, ct); + // S2S runs after delegated grants, regardless of their success, using elevated scopes. + var s2sScopes = permScopes.Concat(AuthenticationConstants.RequiredS2SGrantScopes).ToArray(); + if (!string.IsNullOrWhiteSpace(phase1Result?.BlueprintSpObjectId)) + await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result.BlueprintSpObjectId, specs, s2sScopes, logger, setupResults, ct); + else if (setupResults is not null) + setupResults.S2SAppRoleGranted = true; + logger.LogInformation(""); if (grantsOk) { @@ -389,6 +396,7 @@ private static async Task UpdateBlueprintPermissions /// Phase 2b: Creates AllPrincipals (tenant-wide) OAuth2 permission grants for all specs. /// Requires Global Administrator. Only called when the current user is confirmed GA. /// Returns true if all grants succeeded, false if any grant failed. + /// S2S app role assignments are handled separately by . /// private static async Task ConfigureOauth2GrantsAsync( GraphApiService graph, @@ -443,6 +451,60 @@ private static async Task ConfigureOauth2GrantsAsync( return allGrantsOk; } + /// + /// Grants S2S app role assignments for all specs that carry . + /// Idempotent: skips roles already assigned. Sets on completion. + /// Requires Global Administrator and . + /// + private static async Task PerformS2SGrantsAsync( + AgentBlueprintService blueprintService, + string tenantId, + string blueprintSpObjectId, + IEnumerable specs, + string[] s2sScopes, + ILogger logger, + SetupResults? setupResults, + CancellationToken ct) + { + var s2sSpecs = specs.Where(s => s.AppRoleScopes is { Length: > 0 }).ToList(); + if (s2sSpecs.Count == 0) + { + if (setupResults is not null) setupResults.S2SAppRoleGranted = true; + return; + } + + logger.LogInformation(""); + logger.LogInformation("Configuring S2S app role assignments..."); + + var allS2SOk = true; + foreach (var spec in s2sSpecs) + { + logger.LogDebug( + " - App role assignment: blueprint -> {ResourceName} [{AppRoles}]", + spec.ResourceName, string.Join(' ', spec.AppRoleScopes!)); + + var s2sOk = await blueprintService.GrantAppRoleAssignmentAsync( + tenantId, + blueprintSpObjectId, + spec.ResourceAppId, + spec.AppRoleScopes!, + requiredScopes: s2sScopes, + ct: ct); + + if (s2sOk) + logger.LogInformation(" - S2S app role assigned for {ResourceName}", spec.ResourceName); + else + { + logger.LogWarning(" - Failed to assign S2S app role for {ResourceName}.", spec.ResourceName); + setupResults?.Warnings.Add($"S2S app role assignment failed for {spec.ResourceName}. Re-run 'a365 setup admin' to retry."); + allS2SOk = false; + } + } + + if (setupResults is not null) + setupResults.S2SAppRoleGranted = allS2SOk; + } + /// /// Phase 3: Checks for existing consent (skips browser if found), then either opens the /// browser for admins or returns a consolidated consent URL for non-admins. @@ -535,6 +597,9 @@ private static async Task ConfigureOauth2GrantsAsync( // we performed a role check and found the user lacks the GA role. if (adminCheck == Models.RoleCheckResult.DoesNotHaveRole) { + var s2sSpecs = specs.Where(s => s.AppRoleScopes is { Length: > 0 }).ToList(); + if (s2sSpecs.Count > 0 && setupResults is not null) + setupResults.S2SAppRoleGranted = false; return (false, consentUrl); } @@ -678,7 +743,8 @@ private record BlueprintPermissionsResult( ILogger logger, SetupResults setupResults, CancellationToken ct, - string? knownBlueprintSpObjectId = null) + string? knownBlueprintSpObjectId = null, + AgentBlueprintService? blueprintService = null) { if (specs.Count == 0) { @@ -689,8 +755,14 @@ private record BlueprintPermissionsResult( var effectiveSpecs = specs.Where(s => s.Scopes.Length > 0).ToList(); if (effectiveSpecs.Count == 0) { - logger.LogInformation("All permission specs have empty scope lists — nothing to grant."); - return (true, null); + var hasS2SSpecs = specs.Any(s => s.AppRoleScopes is { Length: > 0 }); + if (!hasS2SSpecs) + { + logger.LogInformation("All permission specs have empty scope and app role lists — nothing to grant."); + setupResults.S2SAppRoleGranted = true; + return (true, null); + } + logger.LogDebug("No delegated scopes to grant — proceeding with S2S app role assignments."); } var permScopes = AuthenticationConstants.RequiredPermissionGrantScopes; @@ -699,11 +771,17 @@ private record BlueprintPermissionsResult( logger.LogInformation(""); logger.LogInformation("Resolving service principals..."); + // When delegated specs are empty but S2S specs exist, resolve SPs from S2S specs instead + // so the blueprint SP object ID is available for app role assignment. + var specsForPhase1 = effectiveSpecs.Count > 0 + ? (IReadOnlyList)effectiveSpecs + : specs.Where(s => s.AppRoleScopes is { Length: > 0 }).ToList(); + BlueprintPermissionsResult? phase1Result = null; try { phase1Result = await UpdateBlueprintPermissionsAsync( - graph, blueprintAppId, tenantId, effectiveSpecs, permScopes, logger, ct, + graph, blueprintAppId, tenantId, specsForPhase1, permScopes, logger, ct, knownBlueprintSpObjectId); } catch (Exception ex) @@ -758,6 +836,13 @@ 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); + else + setupResults.S2SAppRoleGranted = true; // M-003: no service or unresolved SP — mark not applicable + return (allGrantsOk, phase1Result.BlueprintSpObjectId); } } 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 dbe2695c..5ca5d19e 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/ResourcePermissionSpec.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/ResourcePermissionSpec.cs @@ -18,4 +18,5 @@ internal record ResourcePermissionSpec( string ResourceAppId, string ResourceName, string[] Scopes, - bool SetInheritable); + bool SetInheritable, + string[]? AppRoleScopes = null); 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 3d967ffc..b9e8e14f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -16,6 +16,10 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; /// internal static class SetupHelpers { + internal const int DryRunValCol = 30; + internal static string DryRunRow(string label) => (" " + label).PadRight(DryRunValCol); + internal static string DryRunRow(int step, string label) => $" {step}. {label}".PadRight(DryRunValCol); + /// /// Returns the fixed-scope ResourcePermissionSpecs for the three platform APIs that every /// agent blueprint requires: Messaging Bot API, Observability API, and Power Platform API. @@ -31,8 +35,9 @@ internal static ResourcePermissionSpec[] GetFixedApiPermissionSpecs(bool setInhe new ResourcePermissionSpec( ConfigConstants.ObservabilityApiAppId, "Observability API", - new[] { "user_impersonation", ConfigConstants.ObservabilityApiOtelWriteScope }, - setInheritable), + new[] { ConfigConstants.ObservabilityApiOtelWriteScope }, + setInheritable, + AppRoleScopes: new[] { ConfigConstants.ObservabilityApiOtelWriteScope }), new ResourcePermissionSpec( PowerPlatformConstants.PowerPlatformApiResourceAppId, "Power Platform API", @@ -105,61 +110,90 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger) { logger.LogInformation(""); logger.LogInformation("Setup Summary"); + logger.LogInformation(""); var pendingAdminAction = !results.AdminConsentGranted && results.BatchPermissionsPhase2Completed; - // Completed steps — [OK] only - logger.LogInformation("Completed Steps:"); + // Numbered step rows — az CLI style: label padded to fixed column, then status word + var step = 0; + if (results.InfrastructureCreated) { - var status = results.InfrastructureAlreadyExisted ? "(already exists)" : "created"; - logger.LogInformation(" [OK] Infrastructure {Status}", status); + step++; + logger.LogInformation(DryRunRow(step, "Azure hosting") + (results.InfrastructureAlreadyExisted ? "reused" : "provisioned")); } + if (results.BlueprintCreated) { - var status = results.BlueprintAlreadyExisted ? "(already exists)" : "created"; - logger.LogInformation(" [OK] Agent blueprint {Status} ID: {BlueprintId}", status, results.BlueprintId ?? "unknown"); + step++; + var bpStatus = results.BlueprintAlreadyExisted ? "reused" : "created"; + logger.LogInformation(DryRunRow(step, "Blueprint") + "{Status} ID: {Id}", bpStatus, results.BlueprintId ?? "unknown"); } + if (results.BatchPermissionsPhase2Completed) { - logger.LogInformation(" [OK] Inheritable permissions configured and verified"); - if (results.AdminConsentGranted) - logger.LogInformation(" [OK] OAuth2 grants and admin consent configured"); + step++; + var grantStatus = results.AdminConsentGranted ? "ok" : "PENDING"; + logger.LogInformation(DryRunRow(step, "Permissions") + grantStatus); } + if (results.MessagingEndpointRegistered) { - var status = results.EndpointAlreadyExisted ? "(already exists)" : "created"; - logger.LogInformation(" [OK] Messaging endpoint {Status}", status); + step++; + logger.LogInformation(DryRunRow(step, "Messaging endpoint") + (results.EndpointAlreadyExisted ? "reused" : "registered")); } - // Action required — shown as its own section so it isn't conflated with completed work - var hasActionRequired = pendingAdminAction || results.ClientSecretManualActionRequired; + // Action required — one numbered section with inline instructions + var pendingS2SAction = results.S2SAppRoleGranted == false; + var hasActionRequired = pendingAdminAction || results.ClientSecretManualActionRequired || pendingS2SAction; if (hasActionRequired) { + var blueprintAppId = results.BlueprintId ?? ""; + var consentUrl = results.CombinedConsentUrl ?? results.AdminConsentUrl; + var itemNum = 0; + logger.LogInformation(""); logger.LogInformation("Action Required:"); + if (results.ClientSecretManualActionRequired) - logger.LogInformation(" Client secret - must be created manually in Entra ID and added to a365.generated.config.json (see instructions above)"); - if (pendingAdminAction) - logger.LogInformation(" OAuth2 grants — Global Administrator must grant consent (see Next Steps)"); + { + itemNum++; + logger.LogInformation(" {N}. Client secret — create manually in Entra ID and add to a365.generated.config.json (see instructions above)", itemNum); + } + + if (pendingAdminAction && !string.IsNullOrWhiteSpace(consentUrl)) + { + itemNum++; + logger.LogInformation(" {N}. OAuth2 permission grants — share this URL with your Global Administrator:", itemNum); + logger.LogInformation(" {ConsentUrl}", consentUrl); + } + + if (pendingS2SAction) + { + itemNum++; + logger.LogInformation(" {N}. Observability API S2S app role — run as Global Administrator (PowerShell):", itemNum); + logger.LogInformation(" Connect-MgGraph -Scopes 'AppRoleAssignment.ReadWrite.All'"); + logger.LogInformation(" $bp = Get-MgServicePrincipal -Filter \"appId eq '{BlueprintAppId}'\"", blueprintAppId); + logger.LogInformation(" $obs = Get-MgServicePrincipal -Filter \"appId eq '{ObsApiAppId}'\"", ConfigConstants.ObservabilityApiAppId); + logger.LogInformation(" $rid = ($obs.AppRoles | Where-Object {{ $_.Value -eq '{ObsScope}' }}).Id", ConfigConstants.ObservabilityApiOtelWriteScope); + logger.LogInformation(" New-MgServicePrincipalAppRoleAssignment -ServicePrincipalId $bp.Id -PrincipalId $bp.Id -ResourceId $obs.Id -AppRoleId $rid"); + } } - // Failed steps if (results.Errors.Count > 0) { logger.LogInformation(""); - logger.LogInformation("Failed Steps:"); + logger.LogInformation("Errors:"); foreach (var error in results.Errors) - logger.LogError(" [FAILED] {Error}", error); + logger.LogError(" {Error}", error); } - // Warnings if (results.Warnings.Count > 0) { logger.LogInformation(""); logger.LogInformation("Warnings:"); foreach (var warning in results.Warnings) - logger.LogInformation(" [WARN] {Warning}", warning); + logger.LogInformation(" {Warning}", warning); } logger.LogInformation(""); @@ -178,25 +212,6 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger) } } - if (pendingAdminAction) - { - logger.LogInformation(""); - logger.LogInformation("Next Steps — Global Administrator action required:"); - logger.LogInformation(" OAuth2 permission grants require a Global Administrator."); - logger.LogInformation(" Option 1 — Run the CLI as a Global Administrator:"); - logger.LogInformation(" a365 setup admin --config-dir \"\""); - if (!string.IsNullOrWhiteSpace(results.CombinedConsentUrl)) - { - logger.LogInformation(" Option 2 — Share a single consent URL with your Global Administrator:"); - logger.LogInformation(" {ConsentUrl}", results.CombinedConsentUrl); - } - else if (!string.IsNullOrWhiteSpace(results.AdminConsentUrl)) - { - logger.LogInformation(" Alternatively, a Global Administrator can grant Graph consent at:"); - logger.LogInformation(" {ConsentUrl}", results.AdminConsentUrl); - } - } - if (!results.HasErrors && !hasActionRequired) { if (results.HasWarnings) @@ -315,7 +330,7 @@ static string Build(string tenant, string client, string resourceUri, IEnumerabl urls.Add(("Agent 365 Tools", Build(tenantId, blueprintClientId, McpConstants.Agent365ToolsIdentifierUri, mcpScopeList))); urls.Add(("Messaging Bot API", Build(tenantId, blueprintClientId, ConfigConstants.MessagingBotApiIdentifierUri, new[] { ConfigConstants.MessagingBotApiAdminConsentScope }))); - urls.Add(("Observability API", Build(tenantId, blueprintClientId, ConfigConstants.ObservabilityApiIdentifierUri, new[] { ConfigConstants.ObservabilityApiAdminConsentScope }))); + urls.Add(("Observability API", Build(tenantId, blueprintClientId, ConfigConstants.ObservabilityApiIdentifierUri, new[] { ConfigConstants.ObservabilityApiOtelWriteScope }))); urls.Add(("Power Platform API", Build(tenantId, blueprintClientId, PowerPlatformConstants.PowerPlatformApiIdentifierUri, new[] { PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead }))); return urls; @@ -338,7 +353,7 @@ internal static string BuildCombinedConsentUrl( foreach (var s in mcpScopes) allScopes.Add($"{McpConstants.Agent365ToolsIdentifierUri}/{s}"); allScopes.Add($"{ConfigConstants.MessagingBotApiIdentifierUri}/{ConfigConstants.MessagingBotApiAdminConsentScope}"); - allScopes.Add($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiAdminConsentScope}"); + allScopes.Add($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiOtelWriteScope}"); allScopes.Add($"{PowerPlatformConstants.PowerPlatformApiIdentifierUri}/{PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead}"); return BuildAdminConsentUrl(tenantId, blueprintClientId, allScopes); } @@ -354,19 +369,28 @@ public static void DisplayAdminSetupSummary( { logger.LogInformation(""); logger.LogInformation("Admin Setup Summary"); - logger.LogInformation("Completed Steps:"); + logger.LogInformation(""); + + var adminStep = 0; if (results.AdminConsentGranted) { - logger.LogInformation(" [OK] OAuth2 grants configured (tenant-wide)"); + adminStep++; + logger.LogInformation(DryRunRow(adminStep, "Permission grants") + "ok (tenant-wide)"); + } + + if (results.S2SAppRoleGranted == true) + { + adminStep++; + logger.LogInformation(DryRunRow(adminStep, "S2S app role") + "ok ({Scope})", ConfigConstants.ObservabilityApiOtelWriteScope); } if (results.Errors.Count > 0) { logger.LogInformation(""); - logger.LogInformation("Failed Steps:"); + logger.LogInformation("Errors:"); foreach (var error in results.Errors) - logger.LogError(" [FAILED] {Error}", error); + logger.LogError(" {Error}", error); } if (results.Warnings.Count > 0) @@ -374,7 +398,7 @@ public static void DisplayAdminSetupSummary( logger.LogInformation(""); logger.LogInformation("Warnings:"); foreach (var warning in results.Warnings) - logger.LogInformation(" [WARN] {Warning}", warning); + logger.LogInformation(" {Warning}", warning); } logger.LogInformation(""); 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 b5175f70..b29df131 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs @@ -35,6 +35,12 @@ public class SetupResults /// public bool AdminConsentGranted { get; set; } + /// + /// True when all S2S app role assignments were successfully granted. False means pending + /// (non-admin user; Global Administrator action required). Null means not attempted. + /// + public bool? S2SAppRoleGranted { get; set; } + /// /// Error message when Microsoft Graph inheritable permissions fail to configure. /// Non-null indicates failure. This is critical for agent token exchange functionality. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs index 363fecd1..749834bb 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs @@ -201,7 +201,18 @@ public static string[] GetRequiredRedirectUris(string clientAppId) { "Application.ReadWrite.All", "DelegatedPermissionGrant.ReadWrite.All", - "AgentIdentityBlueprint.UpdateAuthProperties.All" + "AgentIdentityBlueprint.UpdateAuthProperties.All", + }; + + /// + /// Additional scopes required only on Global Administrator paths that grant S2S app role + /// assignments. Kept separate from so that + /// non-admin flows (deploy, setup permissions) do not request an admin-only scope and + /// trigger unexpected consent prompts. + /// + public static readonly string[] RequiredS2SGrantScopes = new[] + { + "AppRoleAssignment.ReadWrite.All", }; /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs index f28eecc6..eba8131e 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs @@ -76,13 +76,6 @@ public static class ConfigConstants /// public const string MessagingBotApiAdminConsentScope = "AgentData.ReadWrite"; - /// - /// Observability API scope used for admin consent URL construction. - /// Note: the orchestrator grants "user_impersonation" + ObservabilityApiOtelWriteScope via OAuth2 - /// permission grants; this scope is the consent-URL-facing name for the same resource. - /// - public const string ObservabilityApiAdminConsentScope = "Maven.ReadWrite.All"; - /// /// Observability API scope for writing OpenTelemetry data. /// Granted alongside "user_impersonation" to all provisioned agent identities. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs index e79243a8..726792f8 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs @@ -948,6 +948,144 @@ private async Task ResolveBlueprintObjectIdAsync( return blueprintAppId; } + /// + /// Grants application role assignments (appRoleAssignments) on the blueprint's service principal + /// for each named app role on the given resource. This enables S2S (service-to-service) access + /// where the blueprint identity calls the resource using a client-credentials token with no user + /// context. Idempotent: existing assignments for the same role are skipped. + /// Requires Global Administrator. + /// + /// Tenant ID. + /// Object ID of the blueprint's service principal. + /// Application ID of the resource (e.g. Observability API). + /// Names of the app roles to assign (e.g. "Agent365.Observability.OtelWrite"). + /// Graph scopes required to perform the operation. + /// Cancellation token. + /// True when all assignments succeeded or already existed; false if any failed. + public virtual async Task GrantAppRoleAssignmentAsync( + string tenantId, + string blueprintSpObjectId, + string resourceAppId, + IEnumerable appRoleNames, + IEnumerable? requiredScopes = null, + CancellationToken ct = default) + { + var roleNames = appRoleNames?.ToList() ?? new List(); + if (roleNames.Count == 0) return true; + + try + { + // Resolve the resource service principal. + var 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); + return false; + } + + // Fetch the resource SP's app roles to map names -> IDs. + using var resourceSpDoc = await _graphApiService.GraphGetAsync( + tenantId, $"/v1.0/servicePrincipals/{resourceSpId}?$select=appRoles", ct, + scopes: requiredScopes); + if (resourceSpDoc == null) + { + _logger.LogError("Failed to retrieve app roles for resource SP {ResourceSpId}.", resourceSpId); + return false; + } + + // Build a name -> id map from the resource SP's appRoles array. + var roleIdByName = new Dictionary(StringComparer.OrdinalIgnoreCase); + if (resourceSpDoc.RootElement.TryGetProperty("appRoles", out var appRolesEl) && + appRolesEl.ValueKind == JsonValueKind.Array) + { + foreach (var role in appRolesEl.EnumerateArray()) + { + if (role.TryGetProperty("value", out var valEl) && + role.TryGetProperty("id", out var idEl)) + { + var name = valEl.GetString(); + var id = idEl.GetString(); + if (!string.IsNullOrWhiteSpace(name) && !string.IsNullOrWhiteSpace(id)) + roleIdByName[name] = id; + } + } + } + + // Fetch existing assignments on the blueprint SP to avoid duplicates. + var existingRoleIds = new HashSet(StringComparer.OrdinalIgnoreCase); + using var existingDoc = await _graphApiService.GraphGetAsync( + tenantId, + $"/v1.0/servicePrincipals/{blueprintSpObjectId}/appRoleAssignments", + ct, scopes: requiredScopes); + if (existingDoc != null && + existingDoc.RootElement.TryGetProperty("value", out var existingArr) && + existingArr.ValueKind == JsonValueKind.Array) + { + foreach (var assignment in existingArr.EnumerateArray()) + { + if (assignment.TryGetProperty("resourceId", out var resId) && + resId.GetString()?.Equals(resourceSpId, StringComparison.OrdinalIgnoreCase) == true && + assignment.TryGetProperty("appRoleId", out var roleIdEl)) + { + var id = roleIdEl.GetString(); + if (!string.IsNullOrWhiteSpace(id)) existingRoleIds.Add(id); + } + } + } + + var allOk = true; + foreach (var roleName in roleNames) + { + if (!roleIdByName.TryGetValue(roleName, out var appRoleId)) + { + _logger.LogWarning("App role '{RoleName}' not found on resource {ResourceAppId} — assignment skipped.", roleName, resourceAppId); + allOk = false; + continue; + } + + if (existingRoleIds.Contains(appRoleId)) + { + _logger.LogDebug("App role '{RoleName}' already assigned on blueprint SP {BpSpId}.", roleName, blueprintSpObjectId); + continue; + } + + var payload = new + { + principalId = blueprintSpObjectId, + resourceId = resourceSpId, + appRoleId = appRoleId + }; + + var resp = await _graphApiService.GraphPostWithResponseAsync( + tenantId, + $"/v1.0/servicePrincipals/{blueprintSpObjectId}/appRoleAssignments", + payload, + ct, + requiredScopes); + + if (resp.IsSuccess) + { + _logger.LogDebug("App role '{RoleName}' assigned to blueprint SP {BpSpId}.", roleName, blueprintSpObjectId); + } + else + { + _logger.LogWarning( + "Failed to assign app role '{RoleName}': HTTP {Status} {Reason} — {Body}", + roleName, (int)resp.StatusCode, resp.ReasonPhrase, resp.Body); + allOk = false; + } + } + + return allOk; + } + catch (Exception ex) + { + _logger.LogError(ex, "Exception granting app role assignments on {ResourceAppId}: {Message}", resourceAppId, ex.Message); + return false; + } + } + private static List ParseInheritableScopesFromJson(JsonElement entry) { var scopesList = new List(); 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 f23e76de..28b14346 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 @@ -68,7 +68,7 @@ public void BuildAdminConsentUrls_ObservabilityApi_UsesCorrectScopeConstant() var urls = SetupHelpers.BuildAdminConsentUrls(TenantId, BlueprintClientId, new[] { "Mail.Send" }, new[] { "scope" }); var obsUrl = urls.First(u => u.ResourceName == "Observability API").ConsentUrl; - obsUrl.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiAdminConsentScope}"), + obsUrl.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiOtelWriteScope}"), because: "scope URIs are Uri.EscapeDataString-encoded in the query string — required by AAD for adminconsent"); } @@ -220,7 +220,7 @@ public void BuildCombinedConsentUrl_AlwaysIncludesAllThreeFixedResources() url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.MessagingBotApiIdentifierUri}/{ConfigConstants.MessagingBotApiAdminConsentScope}"), because: "scope URIs are Uri.EscapeDataString-encoded in the query string — required by AAD for adminconsent"); - url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiAdminConsentScope}")); + url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiOtelWriteScope}")); url.Should().Contain(Uri.EscapeDataString($"{PowerPlatformConstants.PowerPlatformApiIdentifierUri}/{PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead}")); } 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 d85e33af..4bed3255 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 @@ -464,10 +464,178 @@ private static IAuthenticationService FakeAuth() // Pass a no-op login hint resolver to skip the real 'az account show' process spawned by // AzCliHelper.ResolveLoginHintAsync — that static call bypasses the mocked CommandExecutor // and causes each test to wait several seconds for the real az CLI. - var graphService = new GraphApiService(_mockGraphLogger, executor, Substitute.For(), handler, _mockTokenProvider, + var graphService = new GraphApiService(_mockGraphLogger, executor, FakeAuth(), handler, _mockTokenProvider, loginHintResolver: () => Task.FromResult(null)); return (new AgentBlueprintService(_mockLogger, graphService), handler); } + + // ── GrantAppRoleAssignmentAsync tests ────────────────────────────────────── + + [Fact] + public async Task GrantAppRoleAssignmentAsync_WhenResourceSpNotFound_ReturnsFalse() + { + // Arrange + var (service, handler) = CreateServiceWithFakeHandler(); + // SP lookup returns empty value array + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); + + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); + + // Assert + result.Should().BeFalse(); + } + + [Fact] + public async Task GrantAppRoleAssignmentAsync_WhenRoleNotFoundOnResourceSp_ReturnsFalse() + { + // Arrange + var (service, handler) = CreateServiceWithFakeHandler(); + // SP lookup succeeds + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") + }); + // Resource SP has no matching app roles + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[]}") + }); + // Existing assignments + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); + + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); + + // Assert + result.Should().BeFalse(); + } + + [Fact] + public async Task GrantAppRoleAssignmentAsync_WhenRoleAlreadyAssigned_ReturnsTrueWithoutPost() + { + // Arrange + var (service, handler) = CreateServiceWithFakeHandler(); + // SP lookup + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") + }); + // Resource SP app roles + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}") + }); + // Existing assignments — role already assigned + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"resourceId\":\"resource-sp-id\",\"appRoleId\":\"role-id-1\"}]}") + }); + // No POST should be queued — if the handler is called a 4th time it returns 404 + + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); + + // Assert + result.Should().BeTrue(); + } + + [Fact] + public async Task GrantAppRoleAssignmentAsync_WhenPostSucceeds_ReturnsTrue() + { + // Arrange + var (service, handler) = CreateServiceWithFakeHandler(); + // SP lookup + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") + }); + // Resource SP app roles + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}") + }); + // Existing assignments — none + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); + // POST succeeds + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Created) + { + Content = new StringContent("{\"id\":\"assignment-id\"}") + }); + + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); + + // Assert + result.Should().BeTrue(); + } + + [Fact] + public async Task GrantAppRoleAssignmentAsync_WhenPostFails_ReturnsFalse() + { + // Arrange + var (service, handler) = CreateServiceWithFakeHandler(); + // SP lookup + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") + }); + // Resource SP app roles + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}") + }); + // Existing assignments — none + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); + // POST fails + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Forbidden) + { + Content = new StringContent("{\"error\":{\"code\":\"Authorization_RequestDenied\"}}") + }); + + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); + + // Assert + result.Should().BeFalse(); + } + + [Fact] + public async Task GrantAppRoleAssignmentAsync_WithEmptyRoleNames_ReturnsTrue() + { + // Arrange + var (service, handler) = CreateServiceWithFakeHandler(); + + // Act — no HTTP calls should be made for an empty role list + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + Array.Empty()); + + // Assert + result.Should().BeTrue(); + } } // Simple fake handler that returns queued responses sequentially From f59b001bda1e3a56bf04d4053f4de55da48cd8c8 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Mon, 13 Apr 2026 17:52:34 -0700 Subject: [PATCH 2/3] Update changelog: new features, S2S perms, and bug fixes - Re-enable a365 create-instance with User.ReadWrite.All requirement - Grant Agent365.Observability.OtelWrite as delegated & app perm - Add S2S app role assignment support to setup commands - Expand default Graph scopes (ChannelMessage, Files) - Add server-driven notice system for advisories/upgrades - Add --dry-run to a365 cleanup azure - Add AppServiceAuthRequirementCheck before deploy - Require sponsor for A365CreateInstanceRunner - Fix TLS proxy issues by using direct MSAL.NET token acquisition - Allow blueprint deletion by Global Admins regardless of creator - Use correct Observability API admin consent scope - Restrict AppRoleAssignment.ReadWrite.All to admin-only paths - Improve consent and authentication fallback flows --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f625256..e8573d66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added - Re-enabled `a365 create-instance` command (previously deprecated) — creates agent identity, agent user, and assigns licenses in a single command. The custom client app now requires the `User.ReadWrite.All` delegated permission for user creation and license assignment; existing users may need to update admin consent on their client app. -- `Agent365.Observability.OtelWrite` scope now granted to all provisioned agent identities on the Observability API alongside `user_impersonation`, enabling agents to write OpenTelemetry data to the Agent 365 observability service +- `Agent365.Observability.OtelWrite` granted to all provisioned agent identities on the Observability API as both a **delegated** permission (OAuth2 grant) and an **application** permission (S2S app role assignment), enabling agents to write OpenTelemetry data to the Agent 365 observability service +- S2S app role assignment support in `a365 setup permissions` and `a365 setup admin` — the CLI now automatically grants application-type (`appRoleAssignments`) permissions on the blueprint service principal when a `ResourcePermissionSpec` defines `AppRoleScopes`. Global Administrator is required for S2S grants; non-admin users receive actionable PowerShell fallback instructions - `ChannelMessage.Read.All` and `ChannelMessage.Send` added to default blueprint Microsoft Graph delegated scopes (`agentIdentityScopes`) - `Files.ReadWrite.All`, `ChannelMessage.Read.All`, and `ChannelMessage.Send` added to default blueprint Microsoft Graph application scopes (`agentApplicationScopes`) - Server-driven notice system: security advisories and critical upgrade prompts are displayed at startup when a maintainer updates `notices.json`. Notices are suppressed once the user upgrades past the specified `minimumVersion`. Results are cached locally for 4 hours to avoid network calls on every invocation. @@ -22,6 +23,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `A365CreateInstanceRunner` sponsor handling: sponsor is now required (Graph API rejects requests without one) — removed fallback that silently stripped the sponsor on retry, which caused `BadRequest` errors - Intermittent `ConnectionResetError (10054)` failures on corporate networks with TLS inspection proxies (Zscaler, Netskope) — Graph and ARM API calls now use direct MSAL.NET token acquisition instead of `az account get-access-token` subprocesses, bypassing the Python HTTP stack that triggered proxy resets (#321) - `a365 cleanup` blueprint deletion now succeeds for Global Administrators even when the blueprint was created by a different user +- Admin consent URL for the Observability API used the non-existent scope `Maven.ReadWrite.All` (AADSTS650053) — replaced with the correct delegated scope `Agent365.Observability.OtelWrite` +- `AppRoleAssignment.ReadWrite.All` (admin-only) was incorrectly included in `RequiredPermissionGrantScopes`, causing it to be requested on non-admin paths (`a365 deploy`, `setup permissions`) — moved to a dedicated `RequiredS2SGrantScopes` constant used only on Global Administrator paths - `a365 setup all` no longer times out for non-admin users — the CLI immediately surfaces a consent URL to share with an administrator instead of waiting for a browser prompt - `a365 setup all` requests admin consent once for all resources instead of prompting once per resource - Browser and WAM authentication blocked by Conditional Access Policy (AADSTS53003, AADSTS53000) now automatically falls back to device code flow (#294) From 96cfb9dcf7ce533df17b054a3bd47895eed93fbc Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Mon, 13 Apr 2026 18:10:09 -0700 Subject: [PATCH 3/3] Refine app role assignment logic and improve test clarity - Leave S2SAppRoleGranted as null if blueprint SP is unresolved or service is unavailable, clarifying "not attempted" vs "not applicable" - Deduplicate and sanitize app role names before assignment to prevent redundant POSTs - Track assigned role IDs within a single operation to avoid duplicate assignments - Add test assertions and resource disposal for improved test reliability and clarity - Enhance consent URL test with explicit reasoning for scope encoding --- .../BatchPermissionsOrchestrator.cs | 6 +- .../Services/AgentBlueprintService.cs | 7 +- .../Helpers/SetupHelpersConsentUrlTests.cs | 3 +- .../Services/AgentBlueprintServiceTests.cs | 233 ++++++++++-------- 4 files changed, 135 insertions(+), 114 deletions(-) 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 4e78ce97..ed711d47 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BatchPermissionsOrchestrator.cs @@ -175,8 +175,7 @@ internal static class BatchPermissionsOrchestrator var s2sScopes = permScopes.Concat(AuthenticationConstants.RequiredS2SGrantScopes).ToArray(); if (!string.IsNullOrWhiteSpace(phase1Result?.BlueprintSpObjectId)) await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result.BlueprintSpObjectId, specs, s2sScopes, logger, setupResults, ct); - else if (setupResults is not null) - setupResults.S2SAppRoleGranted = true; + // else: blueprint SP was not resolved — leave S2SAppRoleGranted = null (not attempted) logger.LogInformation(""); if (grantsOk) @@ -840,8 +839,7 @@ private record BlueprintPermissionsResult( 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); - else - setupResults.S2SAppRoleGranted = true; // M-003: no service or unresolved SP — mark not applicable + // else: blueprint service unavailable or SP not resolved — leave S2SAppRoleGranted = null (not attempted) return (allGrantsOk, phase1Result.BlueprintSpObjectId); } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs index 726792f8..1631f7bd 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs @@ -970,7 +970,11 @@ public virtual async Task GrantAppRoleAssignmentAsync( IEnumerable? requiredScopes = null, CancellationToken ct = default) { - var roleNames = appRoleNames?.ToList() ?? new List(); + // De-dup upfront: duplicate names map to the same role ID and would cause a redundant POST. + var roleNames = appRoleNames? + .Where(n => !string.IsNullOrWhiteSpace(n)) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToList() ?? new List(); if (roleNames.Count == 0) return true; try @@ -1067,6 +1071,7 @@ public virtual async Task GrantAppRoleAssignmentAsync( if (resp.IsSuccess) { _logger.LogDebug("App role '{RoleName}' assigned to blueprint SP {BpSpId}.", roleName, blueprintSpObjectId); + existingRoleIds.Add(appRoleId); // prevent duplicate POST if same ID appears again } else { 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 28b14346..d099434e 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 @@ -220,7 +220,8 @@ public void BuildCombinedConsentUrl_AlwaysIncludesAllThreeFixedResources() url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.MessagingBotApiIdentifierUri}/{ConfigConstants.MessagingBotApiAdminConsentScope}"), 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}")); + url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiOtelWriteScope}"), + because: "scope URIs are Uri.EscapeDataString-encoded in the query string — required by AAD for adminconsent"); url.Should().Contain(Uri.EscapeDataString($"{PowerPlatformConstants.PowerPlatformApiIdentifierUri}/{PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead}")); } 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 4bed3255..d702f2ce 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 @@ -476,19 +476,22 @@ public async Task GrantAppRoleAssignmentAsync_WhenResourceSpNotFound_ReturnsFals { // Arrange var (service, handler) = CreateServiceWithFakeHandler(); - // SP lookup returns empty value array - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + using (handler) { - Content = new StringContent("{\"value\":[]}") - }); + // SP lookup returns empty value array + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); - // Act - var result = await service.GrantAppRoleAssignmentAsync( - "tenant-id", "blueprint-sp-id", "resource-app-id", - new[] { "Agent365.Observability.OtelWrite" }); + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); - // Assert - result.Should().BeFalse(); + // Assert + result.Should().BeFalse(); + } } [Fact] @@ -496,29 +499,32 @@ public async Task GrantAppRoleAssignmentAsync_WhenRoleNotFoundOnResourceSp_Retur { // Arrange var (service, handler) = CreateServiceWithFakeHandler(); - // SP lookup succeeds - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") - }); - // Resource SP has no matching app roles - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"appRoles\":[]}") - }); - // Existing assignments - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + using (handler) { - Content = new StringContent("{\"value\":[]}") - }); + // SP lookup succeeds + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") + }); + // Resource SP has no matching app roles + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[]}") + }); + // Existing assignments + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); - // Act - var result = await service.GrantAppRoleAssignmentAsync( - "tenant-id", "blueprint-sp-id", "resource-app-id", - new[] { "Agent365.Observability.OtelWrite" }); + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); - // Assert - result.Should().BeFalse(); + // Assert + result.Should().BeFalse(); + } } [Fact] @@ -526,30 +532,33 @@ public async Task GrantAppRoleAssignmentAsync_WhenRoleAlreadyAssigned_ReturnsTru { // Arrange var (service, handler) = CreateServiceWithFakeHandler(); - // SP lookup - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") - }); - // Resource SP app roles - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}") - }); - // Existing assignments — role already assigned - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + using (handler) { - Content = new StringContent("{\"value\":[{\"resourceId\":\"resource-sp-id\",\"appRoleId\":\"role-id-1\"}]}") - }); - // No POST should be queued — if the handler is called a 4th time it returns 404 + // SP lookup + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") + }); + // Resource SP app roles + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}") + }); + // Existing assignments — role already assigned + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"resourceId\":\"resource-sp-id\",\"appRoleId\":\"role-id-1\"}]}") + }); + // No POST should be queued — if the handler is called a 4th time it returns 404 - // Act - var result = await service.GrantAppRoleAssignmentAsync( - "tenant-id", "blueprint-sp-id", "resource-app-id", - new[] { "Agent365.Observability.OtelWrite" }); + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); - // Assert - result.Should().BeTrue(); + // Assert + result.Should().BeTrue(); + } } [Fact] @@ -557,34 +566,37 @@ public async Task GrantAppRoleAssignmentAsync_WhenPostSucceeds_ReturnsTrue() { // Arrange var (service, handler) = CreateServiceWithFakeHandler(); - // SP lookup - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") - }); - // Resource SP app roles - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}") - }); - // Existing assignments — none - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"value\":[]}") - }); - // POST succeeds - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Created) + using (handler) { - Content = new StringContent("{\"id\":\"assignment-id\"}") - }); + // SP lookup + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") + }); + // Resource SP app roles + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}") + }); + // Existing assignments — none + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); + // POST succeeds + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Created) + { + Content = new StringContent("{\"id\":\"assignment-id\"}") + }); - // Act - var result = await service.GrantAppRoleAssignmentAsync( - "tenant-id", "blueprint-sp-id", "resource-app-id", - new[] { "Agent365.Observability.OtelWrite" }); + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); - // Assert - result.Should().BeTrue(); + // Assert + result.Should().BeTrue(); + } } [Fact] @@ -592,34 +604,37 @@ public async Task GrantAppRoleAssignmentAsync_WhenPostFails_ReturnsFalse() { // Arrange var (service, handler) = CreateServiceWithFakeHandler(); - // SP lookup - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") - }); - // Resource SP app roles - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}") - }); - // Existing assignments — none - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent("{\"value\":[]}") - }); - // POST fails - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Forbidden) + using (handler) { - Content = new StringContent("{\"error\":{\"code\":\"Authorization_RequestDenied\"}}") - }); + // SP lookup + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}") + }); + // Resource SP app roles + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}") + }); + // Existing assignments — none + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"value\":[]}") + }); + // POST fails + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Forbidden) + { + Content = new StringContent("{\"error\":{\"code\":\"Authorization_RequestDenied\"}}") + }); - // Act - var result = await service.GrantAppRoleAssignmentAsync( - "tenant-id", "blueprint-sp-id", "resource-app-id", - new[] { "Agent365.Observability.OtelWrite" }); + // Act + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + new[] { "Agent365.Observability.OtelWrite" }); - // Assert - result.Should().BeFalse(); + // Assert + result.Should().BeFalse(); + } } [Fact] @@ -627,14 +642,16 @@ public async Task GrantAppRoleAssignmentAsync_WithEmptyRoleNames_ReturnsTrue() { // Arrange var (service, handler) = CreateServiceWithFakeHandler(); + using (handler) + { + // Act — no HTTP calls should be made for an empty role list + var result = await service.GrantAppRoleAssignmentAsync( + "tenant-id", "blueprint-sp-id", "resource-app-id", + Array.Empty()); - // Act — no HTTP calls should be made for an empty role list - var result = await service.GrantAppRoleAssignmentAsync( - "tenant-id", "blueprint-sp-id", "resource-app-id", - Array.Empty()); - - // Assert - result.Should().BeTrue(); + // Assert + result.Should().BeTrue(); + } } }