From 40d2721cc4e0289ad872b03da8872d7ae83a167a Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Mon, 27 Apr 2026 18:56:02 -0700 Subject: [PATCH 1/6] Add --authmode option to setup; gate agent grants by mode Introduce --authmode (obo|s2s|both) to a365 setup all and related flows, allowing users to control how agent identity permissions are granted. For non-DW agents, inheritable permissions and AllPrincipals grants are always skipped to avoid requiring Global Admin. The orchestrator now applies delegated grants, app role assignments, or both based on authMode, with PowerShell fallback for S2S if needed. The dry-run plan and summary output reflect these changes. Agent365Config and docs updated to support authMode. Includes extensive tests for all authMode values and validation logic. GraphApiService now treats "Permission entry already exists" as success. --- CHANGELOG.md | 6 + .../SetupSubcommands/AllSubcommand.cs | 28 +++- .../NonDwBlueprintSetupOrchestrator.cs | 140 +++++++++++++++--- .../Commands/SetupSubcommands/README.md | 29 ++++ .../Commands/SetupSubcommands/SetupContext.cs | 17 +++ .../Commands/SetupSubcommands/SetupHelpers.cs | 5 +- .../Models/Agent365Config.cs | 12 ++ .../Models/README.md | 1 + .../Services/GraphApiService.cs | 9 ++ .../design.md | 3 +- ...DwBlueprintSetupOrchestratorDryRunTests.cs | 84 ++++++++++- .../Commands/SetupCommandTests.cs | 98 ++++++++++++ 12 files changed, 404 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eea6c5f9..559dad75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,12 @@ a365 setup admin --config-dir "" ``` ### Added +- `--authmode obo|s2s|both` option on `setup all` — controls how the agent identity service principal receives permissions: + - `obo` (default): principal-scoped delegated grants (`consentType: "Principal"`); no Global Administrator required. + - `s2s`: application role assignments on the agent identity SP; attempted programmatically, falls back to printed PowerShell instructions if the caller lacks Global Administrator. + - `both`: applies both OBO delegated grants and S2S app role assignments. + - Inheritable permissions (Phase 2a) and AllPrincipals grants (Phase 2b) are always skipped for non-DW agents regardless of `authMode`, to avoid requiring a Global Administrator role. + - `authMode` can be persisted in `a365.config.json` to apply on every run without the flag. - `--project-path ` option on `develop list-configured`, `develop add-mcp-servers`, and `develop remove-mcp-servers` — specify the manifest location without requiring `a365.config.json`. - `setup requirements` runs without `a365.config.json` — system checks (PowerShell modules, Frontier enrollment) always run; client app checks run when a config file or Azure CLI session is available. - `--agent-name` and `--tenant-id` options added to `setup blueprint`, `setup permissions` (all subcommands), `create-instance`, `publish`, and `query-entra` — all commands can now resolve configuration from Entra ID without requiring `a365.config.json`. 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 b2e24859..d86c329f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs @@ -134,6 +134,14 @@ public static Command CreateCommand( description: "Treat this agent as an M365 agent. When set, registers the messaging endpoint via MCP Platform. " + "Default is false (opt-in); omit this flag for non-M365 agents."); + var authModeOption = new Option( + "--authmode", + description: "Authentication pattern for the agent identity (blueprint agents only).\n" + + " obo — on-behalf-of (default); principal-scoped delegated grants; no admin consent needed.\n" + + " s2s — service-to-service; app permissions on agent identity; Global Admin needed or PowerShell fallback.\n" + + " both — delegated grants (OBO) and app permissions (S2S).\n" + + "Not supported with --aiteammate true."); + command.AddOption(verboseOption); command.AddOption(dryRunOption); command.AddOption(skipInfrastructureOption); @@ -143,6 +151,7 @@ public static Command CreateCommand( command.AddOption(m365Option); command.AddOption(agentNameOption); command.AddOption(tenantIdOption); + command.AddOption(authModeOption); command.SetHandler(async (System.CommandLine.Invocation.InvocationContext context) => { @@ -159,8 +168,23 @@ public static Command CreateCommand( var agentName = context.ParseResult.GetValueForOption(agentNameOption); var tenantIdFlag = context.ParseResult.GetValueForOption(tenantIdOption); bool isM365 = context.ParseResult.GetValueForOption(m365Option); + var authMode = context.ParseResult.GetValueForOption(authModeOption)?.ToLowerInvariant(); var ct = context.GetCancellationToken(); + // --authmode validation + if (authMode is not null && authMode is not ("obo" or "s2s" or "both")) + { + logger.LogError("Invalid --authmode value '{Value}'. Allowed values: obo, s2s, both.", authMode); + context.ExitCode = 1; + return; + } + if (authMode is not null && aiTeammateFlag == true) + { + logger.LogError("--authmode is not supported with --aiteammate true. It only applies to blueprint agents (--aiteammate false)."); + context.ExitCode = 1; + return; + } + // Generate correlation ID at workflow entry point var correlationId = HttpClientFactory.GenerateCorrelationId(); logger.LogDebug("Starting setup all (CorrelationId: {CorrelationId})", correlationId); @@ -267,7 +291,8 @@ public static Command CreateCommand( if (dryRun) { var rawArgs = context.ParseResult.Tokens.Select(t => t.Value).ToArray(); - NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(nonDwConfig, logger, isBootstrap, rawArgs, skipRequirements, isM365, agentRegistrationOnly); + var effectiveAuthMode = authMode ?? nonDwConfig.AuthMode; + NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(nonDwConfig, logger, isBootstrap, rawArgs, skipRequirements, isM365, agentRegistrationOnly, effectiveAuthMode); return; } @@ -302,6 +327,7 @@ public static Command CreateCommand( agentInstanceOnly: agentRegistrationOnly, isBootstrap: isBootstrap, isM365: isM365, + authMode: authMode ?? nonDwConfig.AuthMode, confirmationProvider: confirmationProvider); context.ExitCode = await NonDwBlueprintSetupOrchestrator.ExecuteAsync(nonDwCtx); 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 ab19e1f2..d81d82a1 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs @@ -30,7 +30,7 @@ internal static class NonDwBlueprintSetupOrchestrator /// Prints a dry-run plan showing all resources that would be created or configured, /// using actual names and values from the loaded config. Makes no API calls. /// - public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool isBootstrap = false, string[]? rawArgs = null, bool skipRequirements = false, bool isM365 = false, bool agentRegistrationOnly = false) + 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) { var sub = new string(' ', SetupHelpers.DryRunValCol); @@ -96,16 +96,15 @@ public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool i logger.LogInformation(sub + "create managed identity"); } - // 3. Inheritable Permissions - var permsList = new List { "Observability API", "Power Platform API" }; - if (config.CustomBlueprintPermissions?.Count > 0) - foreach (var custom in config.CustomBlueprintPermissions) - permsList.Add(custom.ResourceName ?? custom.ResourceAppId); - logger.LogInformation(SetupHelpers.DryRunRow(3, "Inheritable Permissions") + "configure for {Permissions}", string.Join(", ", permsList)); + // 3. Inheritable Permissions — skipped in all authMode values (admin involvement avoided) + var effectiveMode = (authMode ?? config.AuthMode)?.ToLowerInvariant() ?? "obo"; + logger.LogInformation(SetupHelpers.DryRunRow(3, "Inheritable Permissions") + "skipped (authmode: {AuthMode} — admin involvement avoided)", effectiveMode); - // 4. Permission Grants - var blueprintIdForCmd = config.AgentBlueprintId ?? ""; - logger.LogInformation(SetupHelpers.DryRunRow(4, "Permission Grants") + "admin approval required — a365 setup admin --blueprint-id {BlueprintId}", blueprintIdForCmd); + // 4. Permission Grants — per authMode + if (effectiveMode is "obo" or "both") + logger.LogInformation(SetupHelpers.DryRunRow(4, "Agent Identity Delegated") + "principal-scoped delegated grants (programmatic, no admin required)"); + if (effectiveMode is "s2s" or "both") + logger.LogInformation(SetupHelpers.DryRunRow(4, "Agent Identity App Perms") + "application permissions — attempted programmatically; PowerShell fallback if not Global Admin"); // 5. Agent identity var agentIdentityDisplayName = config.AgentIdentityDisplayName ?? "Agent"; @@ -274,18 +273,22 @@ public static async Task ExecuteAsync(SetupContext ctx) // Step 3: Blueprint creation (shared with DW) await AllSubcommand.ExecuteBlueprintStepAsync(ctx); - // Step 4: Batch permissions — non-DW path stamps only Observability API and Power Platform API. + // Step 4: Build permission specs — non-DW path stamps only Observability API and Power Platform API. // Microsoft Graph, Agent 365 Tools (MCP), and Messaging Bot API are excluded. var buildResult = await AllSubcommand.BuildPermissionSpecsAsync(ctx, isDw: false); specs = buildResult.specs; - var mcpResourceAppId = buildResult.mcpResourceAppId; - await AllSubcommand.ExecuteBatchPermissionsStepAsync(ctx, specs); - - SetupHelpers.ApplyConsentUrlsIfNeeded(ctx, mcpResourceAppId, graphScopes: [], mcpScopes: [], isDw: false); + // Phase 2a (inheritable perms) and Phase 2b (AllPrincipals grants + admin consent) + // are skipped for all authMode values — admin involvement is avoided by design. + // Delegated and/or app grants are applied to the agent identity SP below, gated by mode. + ctx.Results.BatchPermissionsPhase1Completed = true; + ctx.Results.BatchPermissionsPhase2Completed = true; + ctx.Results.AdminConsentGranted = false; + ctx.Logger.LogInformation( + "Inheritable perms and AllPrincipals grants skipped (authmode: {AuthMode}).", + ctx.AuthMode ?? "obo"); - // Save state after permissions (before agent identity creation, so progress - // is not lost if subsequent steps fail). + // Save state before agent identity steps so progress is not lost on failure. await ctx.ConfigService.SaveStateAsync(ctx.Config); // Steps 5-8: Agent identity creation, permission grants, registration, project settings. @@ -396,11 +399,16 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( } } - // Step 5a: Grant permissions to the agent identity (non-admin path only). - // If the batch permissions step already granted AllPrincipals admin consent, skip this. - if (!string.IsNullOrWhiteSpace(ctx.Config.AgenticAppId) && !ctx.Results.AdminConsentGranted) + // Step 5a: Grant permissions to the agent identity, gated by authMode. + if (!string.IsNullOrWhiteSpace(ctx.Config.AgenticAppId)) { - await GrantAgentIdentityPermissionsAsync(ctx, specs); + // OBO and Both: principal-scoped delegated grants (no admin required). + if (ctx.IsOboMode || ctx.IsBothMode) + await GrantAgentIdentityPermissionsAsync(ctx, specs); + + // S2S and Both: app role assignments (requires Global Admin; falls back to PowerShell instructions). + if (ctx.IsS2sMode || ctx.IsBothMode) + await GrantOrInstructAgentIdentityAppPermissionsAsync(ctx, specs); } // Step 6: Register agent via Graph API (copilot/agentRegistrations). @@ -627,4 +635,94 @@ internal static async Task GrantAgentIdentityPermissionsAsync( ctx.Results.AgentIdentityPermissionsGranted = true; } } + + /// + /// Attempts to grant app role assignments on the agent identity SP for S2S access. + /// Requires Global Administrator. When the signed-in user lacks that role, prints + /// PowerShell instructions covering only the app permission section (no delegated/OBO output). + /// + internal static async Task GrantOrInstructAgentIdentityAppPermissionsAsync( + SetupContext ctx, + List specs) + { + var s2sSpecs = specs.Where(s => s.AppRoleScopes is { Length: > 0 }).ToList(); + if (s2sSpecs.Count == 0) + { + ctx.Logger.LogDebug("No app role specs for agent identity S2S grants; skipping."); + return; + } + + // Resolve the agent identity service principal object ID. + var agentIdentitySpObjectId = await ctx.GraphApiService.EnsureServicePrincipalForAppIdAsync( + ctx.Config.TenantId!, + ctx.Config.AgenticAppId!, + ctx.CancellationToken, + Constants.AuthenticationConstants.RequiredPermissionGrantScopes); + + if (string.IsNullOrWhiteSpace(agentIdentitySpObjectId)) + { + ctx.Logger.LogWarning( + "Could not resolve service principal for agent identity ({AgentId}). " + + "App role assignments must be granted manually.", + ctx.Config.AgenticAppId); + ctx.Results.S2SAppRoleGranted = false; + return; + } + + ctx.Logger.LogDebug("Attempting S2S app role assignments on agent identity ({SpId})...", agentIdentitySpObjectId); + + var failedSpecs = new List(); + foreach (var spec in s2sSpecs) + { + var granted = await ctx.BlueprintService.GrantAppRoleAssignmentAsync( + ctx.Config.TenantId!, + agentIdentitySpObjectId, + spec.ResourceAppId, + spec.AppRoleScopes!, + Constants.AuthenticationConstants.RequiredPermissionGrantScopes, + ctx.CancellationToken); + + if (granted) + ctx.Logger.LogDebug("S2S app roles granted on {ResourceName} to agent identity.", spec.ResourceName); + else + { + ctx.Logger.LogDebug("S2S app role assignment failed for {ResourceName} — likely not Global Admin.", spec.ResourceName); + failedSpecs.Add(spec); + } + } + + if (failedSpecs.Count == 0) + { + using (ctx.Logger.Indent()) + ctx.Logger.LogInformation("S2S app role assignments granted to agent identity."); + ctx.Results.S2SAppRoleGranted = true; + return; + } + + // Non-admin fallback: print PowerShell instructions for only the failed resources. + ctx.Results.S2SAppRoleGranted = false; + ctx.Logger.LogInformation(""); + ctx.Logger.LogInformation("S2S app role assignments require Global Administrator. Run the following PowerShell as an admin:"); + ctx.Logger.LogInformation(""); + ctx.Logger.LogInformation(" # Connect to Microsoft Graph"); + ctx.Logger.LogInformation(" Connect-MgGraph -TenantId '{TenantId}' -Scopes 'AppRoleAssignment.ReadWrite.All'", ctx.Config.TenantId); + ctx.Logger.LogInformation(""); + ctx.Logger.LogInformation(" $agentSpId = '{AgentSpId}'", agentIdentitySpObjectId); + + foreach (var spec in failedSpecs) + { + ctx.Logger.LogInformation(""); + ctx.Logger.LogInformation(" # {ResourceName}", spec.ResourceName); + ctx.Logger.LogInformation(" $resourceSp = Get-MgServicePrincipal -Filter \"appId eq '{ResourceAppId}'\"", spec.ResourceAppId); + foreach (var role in spec.AppRoleScopes!) + { + ctx.Logger.LogInformation(" $roleId = ($resourceSp.AppRoles | Where-Object {{ $_.Value -eq '{Role}' }}).Id", role); + ctx.Logger.LogInformation(" New-MgServicePrincipalAppRoleAssignment -ServicePrincipalId $agentSpId -PrincipalId $agentSpId -ResourceId $resourceSp.Id -AppRoleId $roleId"); + } + } + + ctx.Logger.LogInformation(""); + ctx.Results.Warnings.Add( + "S2S app role assignments require Global Administrator. PowerShell instructions have been printed above."); + } } 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 873ee0e3..aeb09374 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md @@ -20,6 +20,8 @@ This folder contains the workflow components for the `a365 setup` command. The s | **RequirementsSubcommand** | `RequirementsSubcommand.cs` | Validates prerequisites (Azure CLI, permissions) | | **SetupHelpers** | `SetupHelpers.cs` | Shared helper methods; `EnsureResourcePermissionsAsync` used by standalone callers and `CopilotStudioSubcommand` | | **SetupResults** | `SetupResults.cs` | Result models for setup operations | +| **SetupContext** | `SetupContext.cs` | Context bundle threaded through orchestrator steps; exposes `AuthMode`, `IsOboMode`, `IsS2sMode`, `IsBothMode` | +| **NonDwBlueprintSetupOrchestrator** | `NonDwBlueprintSetupOrchestrator.cs` | Blueprint-based non-DW setup flow; skips Phase 2a/2b (inheritable permissions); gates agent identity grants by `authMode` | --- @@ -65,6 +67,33 @@ a365 setup infrastructure # Provision Azure only a365 setup permissions # Configure permissions only ``` +### Authentication mode (`--authmode`) + +The `--authmode` option controls how the agent identity service principal is granted permissions. It is available on `setup all`, `setup blueprint`, and `setup permissions mcp/bot/custom`. + +| Value | Behaviour | +|-------|-----------| +| `obo` (default) | Principal-scoped delegated grants (`consentType: "Principal"`) on the agent identity SP — no Global Admin required | +| `s2s` | Application role assignments on the agent identity SP — attempted programmatically; PowerShell instructions printed as fallback if the caller lacks Global Admin | +| `both` | Both OBO delegated grants and S2S app role assignments | + +`authMode` may also be persisted in `a365.config.json` so it takes effect on every run without the flag. + +Phase 2a (inheritable permissions on the blueprint) and Phase 2b (AllPrincipals grants) are **always skipped** for non-DW agents regardless of `authMode`, to avoid requiring a Global Admin role. + +```bash +# Use OBO grants (default) +a365 setup all + +# Use S2S app-role assignments +a365 setup all --authmode s2s + +# Use both +a365 setup all --authmode both +``` + +--- + ### Messaging endpoint (M365 agents) The `--m365` flag opts into registering the messaging endpoint with Teams Graph via MCP Platform. It is **off by default** — without it, `--endpoint-only`, `--update-endpoint`, and the messaging-endpoint step in `setup all` skip the API call and point you at the Teams Developer Portal for manual configuration. 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 c573d915..d63de698 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs @@ -58,6 +58,21 @@ internal sealed class SetupContext /// public bool IsM365 { get; } + /// + /// Authentication pattern for the agent identity. Resolved from --authmode flag or config. + /// Null and "obo" both resolve to OBO (the default). + /// + public string? AuthMode { get; } + + /// Null or "obo" — principal-scoped delegated grants; no admin consent needed. + public bool IsOboMode => AuthMode is null || string.Equals(AuthMode, "obo", StringComparison.OrdinalIgnoreCase); + + /// "s2s" — app role assignments on agent identity; Global Admin needed or PowerShell fallback. + public bool IsS2sMode => string.Equals(AuthMode, "s2s", StringComparison.OrdinalIgnoreCase); + + /// "both" — delegated grants (OBO) and app role assignments (S2S). + public bool IsBothMode => string.Equals(AuthMode, "both", StringComparison.OrdinalIgnoreCase); + /// /// Overrides the az CLI login hint resolver used during blueprint creation. /// Null in production — injected as a no-op in tests to avoid spawning 'az account show'. @@ -107,6 +122,7 @@ public SetupContext( bool agentInstanceOnly = false, bool isBootstrap = false, bool isM365 = false, + string? authMode = null, Func>? loginHintResolver = null, IConfirmationProvider? confirmationProvider = null) { @@ -122,6 +138,7 @@ public SetupContext( AgentInstanceOnly = agentInstanceOnly; IsBootstrap = isBootstrap; IsM365 = isM365; + AuthMode = authMode?.ToLowerInvariant(); ConfigService = configService; Executor = executor; BackendConfigurator = backendConfigurator; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs index 10ad017d..04e16dc3 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -421,7 +421,10 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo logger.LogInformation("Setup Summary"); logger.LogInformation(""); - var pendingAdminAction = !results.AdminConsentGranted && results.BatchPermissionsPhase2Completed; + // Developer-scoped grants (OBO authmode) are the intentional alternative to AllPrincipals admin consent. + // When AgentIdentityPermissionsGranted is true, no admin action is needed for permission grants. + var pendingAdminAction = !results.AdminConsentGranted && results.BatchPermissionsPhase2Completed + && !results.AgentIdentityPermissionsGranted; var pendingS2SAction = results.S2SAppRoleGranted == false; // ── Numbered step rows — mirrors the dry-run step list ───────────────── diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/Agent365Config.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/Agent365Config.cs index 456d62d5..6185231c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/Agent365Config.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/Agent365Config.cs @@ -150,6 +150,17 @@ private static void ValidateGuid(string value, string fieldName, List er [JsonPropertyName("clientAppId")] public string ClientAppId { get; init; } = string.Empty; + /// + /// Authentication pattern for the agent identity (blueprint agents only). + /// Accepted values: "obo" (default), "s2s", "both". + /// obo — on-behalf-of; principal-scoped delegated grants; no admin consent needed. + /// s2s — service-to-service; app role assignments on agent identity; Global Admin needed or PowerShell fallback. + /// both — delegated grants (OBO) and app permissions (S2S). + /// Persisted to a365.config.json. Not written to a365.generated.config.json. + /// + [JsonPropertyName("authMode")] + public string? AuthMode { get; init; } + #endregion #region Azure OpenAI Configuration @@ -662,6 +673,7 @@ public Agent365Config WithCustomBlueprintPermissions(List CreateOrUpdateOauth2PermissionGrantCoreAsync( if (grantResponse.IsSuccess) return true; + // "Permission entry already exists" means the grant is already in place — treat as success. + if (grantResponse.Body.Contains("Permission entry already exists", StringComparison.OrdinalIgnoreCase)) + { + _logger.LogDebug( + "OAuth2 permission grant already exists for resource {ResourceSpId} — treating as success (idempotent).", + resourceSpObjectId); + return true; + } + if (!grantResponse.Body.Contains("Directory_ObjectNotFound", StringComparison.OrdinalIgnoreCase)) { _logger.LogWarning( diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/design.md b/src/Microsoft.Agents.A365.DevTools.Cli/design.md index d2182e15..258c8182 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/design.md +++ b/src/Microsoft.Agents.A365.DevTools.Cli/design.md @@ -97,7 +97,7 @@ flowchart LR | File | Content | Editing | |------|---------|---------| -| `a365.config.json` | Tenant ID, subscription, resource names, project path | User edits | +| `a365.config.json` | Tenant ID, subscription, resource names, project path, `authMode` | User edits | | `a365.generated.config.json` | Agent blueprint ID, identity ID, consent status | CLI generates | ### Configuration File Storage and Portability @@ -128,6 +128,7 @@ public class Agent365Config public string ResourceGroup { get; init; } = string.Empty; public string WebAppName { get; init; } = string.Empty; public string DeploymentProjectPath { get; init; } = string.Empty; + public string? AuthMode { get; init; } // "obo" | "s2s" | "both"; default obo when null // DYNAMIC PROPERTIES (get/set) - from a365.generated.config.json // Modified at runtime by CLI operations 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 4cf0c861..8fef6ad4 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 @@ -92,19 +92,23 @@ public void PrintDryRunPlan_IncludesDisplayName() } [Fact] - public void PrintDryRunPlan_IncludesObservabilityApiPermissions() + public void PrintDryRunPlan_InheritablePermissions_AreSkipped() { NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger); - AnyLogContains("Observability API").Should().BeTrue(because: "Observability API is required for non-DW blueprints to write OpenTelemetry data"); + // Phase 2a (inheritable permissions) is skipped in all authMode values to avoid Global Admin + // involvement. The dry-run communicates this explicitly rather than listing specific APIs. + AnyLogContains("skipped").Should().BeTrue(because: "inheritable permissions are skipped in all authMode values to avoid Global Admin involvement"); } [Fact] - public void PrintDryRunPlan_IncludesPowerPlatformApiPermissions() + public void PrintDryRunPlan_DelegatedGrants_ArePlannedForAgentIdentity() { NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger); - AnyLogContains("Power Platform API").Should().BeTrue(because: "Power Platform API is required for non-DW blueprints"); + // Default (null/obo) authMode grants principal-scoped delegated permissions to the + // agent identity SP instead of using AllPrincipals grants on the blueprint. + AnyLogContains("delegated").Should().BeTrue(because: "default OBO authMode applies principal-scoped delegated grants to the agent identity SP"); } [Fact] @@ -130,4 +134,76 @@ public void PrintDryRunPlan_IncludesAgentRegistrationStep() AnyLogContains("Agent Registration").Should().BeTrue(because: "agent registration is a required setup step"); } + + // ── authMode dry-run output ──────────────────────────────────────────────── + + /// + /// OBO mode must show delegated grants and must not surface admin-consent or AllPrincipals + /// instructions — the whole point of OBO authMode is to avoid Global Admin involvement. + /// + [Fact] + public void PrintDryRunPlan_AuthModeObo_ShowsDelegatedGrants_NoAdminConsentOrAllPrincipals() + { + NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger, authMode: "obo"); + + AnyLogContains("delegated").Should().BeTrue(because: "OBO mode applies principal-scoped delegated grants"); + AnyLogContains("admin consent").Should().BeFalse(because: "OBO mode must not require admin consent"); + AnyLogContains("AllPrincipals").Should().BeFalse(because: "OBO mode must not create AllPrincipals grants"); + } + + /// + /// S2S mode must show application permissions and must not show delegated grants + /// — there is no user context in S2S so delegated scopes are not applicable. + /// + [Fact] + public void PrintDryRunPlan_AuthModeS2s_ShowsAppPerms_NoDelegatedGrants() + { + NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger, authMode: "s2s"); + + AnyLogContains("application permissions").Should().BeTrue(because: "S2S mode applies app role assignments"); + AnyLogContains("Delegated").Should().BeFalse(because: "S2S mode must not show delegated grants — no user context"); + AnyLogContains("admin consent").Should().BeFalse(because: "S2S mode falls back to PowerShell instructions; no interactive admin consent prompt"); + } + + /// + /// Both mode must surface both delegated grants (OBO) and application permissions (S2S). + /// + [Fact] + public void PrintDryRunPlan_AuthModeBoth_ShowsBothGrantRows() + { + NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger, authMode: "both"); + + AnyLogContains("delegated").Should().BeTrue(because: "Both mode includes OBO delegated grants"); + AnyLogContains("application permissions").Should().BeTrue(because: "Both mode includes S2S app permissions"); + AnyLogContains("admin consent").Should().BeFalse(because: "Both mode must not require interactive admin consent"); + } + + /// + /// Null authMode must default to OBO behaviour — the plan shows delegated grants + /// and must not imply admin consent is required. + /// + [Fact] + public void PrintDryRunPlan_NullAuthMode_TreatedAsObo() + { + NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger, authMode: null); + + AnyLogContains("delegated").Should().BeTrue(because: "null authMode defaults to OBO — delegated grants must appear"); + AnyLogContains("admin consent").Should().BeFalse(because: "null authMode defaults to OBO — admin consent must not be required"); + } + + /// + /// All authMode values must suppress the inheritable-permissions (Phase 2a) step + /// to avoid Global Admin involvement. + /// + [Theory] + [InlineData("obo")] + [InlineData("s2s")] + [InlineData("both")] + [InlineData(null)] + public void PrintDryRunPlan_AllAuthModes_SkipInheritablePermissions(string? authMode) + { + NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger, authMode: authMode); + + AnyLogContains("skipped").Should().BeTrue(because: $"authmode '{authMode ?? "null (obo)"}' must skip inheritable permissions to avoid Global Admin involvement"); + } } 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 ac4af342..3bbb7ac3 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 @@ -539,4 +539,102 @@ public async Task SetupAll_WithAiteammateFalse_ForcesBlueprintPlanRegardlessOfCo Arg.Any(), Arg.Any>()); } + + // ── --authmode parameter validation ─────────────────────────────────────── + + private Command BuildSetupCommand() => SetupCommand.CreateCommand( + _mockLogger, _mockConfigService, _mockExecutor, _mockBackendConfigurator, + _mockAuthValidator, _mockPlatformDetector, + _mockGraphApiService, _mockBlueprintService, _mockBlueprintLookupService, + _mockFederatedCredentialService, _mockClientAppValidator, _mockConfirmationProvider); + + private Agent365Config BlueprintConfig() => new() + { + TenantId = "tenant", + AgentIdentityDisplayName = "agent", + AgentBlueprintDisplayName = "TestBlueprint", + DeploymentProjectPath = ".", + AiTeammate = false, + UseBlueprint = true, + }; + + /// + /// An unrecognised --authmode value must be rejected before any Azure calls are made. + /// + [Fact] + public async Task SetupAll_AuthMode_InvalidValue_ExitsWithCode1() + { + _mockConfigService.LoadAsync(Arg.Any(), Arg.Any()).Returns(Task.FromResult(BlueprintConfig())); + var parser = new CommandLineBuilder(BuildSetupCommand()).Build(); + + var result = await parser.InvokeAsync("all --aiteammate false --authmode invalid", new TestConsole()); + + result.Should().Be(1, because: "an unrecognised --authmode value must abort before touching Azure"); + _mockLogger.Received().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("Invalid --authmode value")), + Arg.Any(), + Arg.Any>()); + } + + /// + /// --authmode is only meaningful for blueprint agents. Passing it with --aiteammate true + /// must be rejected because the combination is undefined. + /// + [Fact] + public async Task SetupAll_AuthMode_WithAiteammateTrue_ExitsWithCode1() + { + _mockConfigService.LoadAsync(Arg.Any(), Arg.Any()).Returns(Task.FromResult(BlueprintConfig())); + var parser = new CommandLineBuilder(BuildSetupCommand()).Build(); + + var result = await parser.InvokeAsync("all --aiteammate true --authmode obo", new TestConsole()); + + result.Should().Be(1, because: "--authmode is not supported with --aiteammate true"); + _mockLogger.Received().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("not supported with --aiteammate true")), + Arg.Any(), + Arg.Any>()); + } + + /// All three valid authMode values must be accepted without error. + [Theory] + [InlineData("obo")] + [InlineData("s2s")] + [InlineData("both")] + [InlineData("OBO")] // case-insensitive + [InlineData("S2S")] + [InlineData("BOTH")] + public async Task SetupAll_AuthMode_ValidValues_ExitWithCode0(string authModeValue) + { + _mockConfigService.LoadAsync(Arg.Any(), Arg.Any()).Returns(Task.FromResult(BlueprintConfig())); + var parser = new CommandLineBuilder(BuildSetupCommand()).Build(); + + var result = await parser.InvokeAsync($"all --aiteammate false --authmode {authModeValue} --dry-run", new TestConsole()); + + result.Should().Be(0, because: $"'{authModeValue}' is a valid --authmode value and --dry-run exits 0"); + } + + /// + /// Omitting --authmode defaults to OBO behaviour — the dry-run plan must show + /// delegated grants and must not mention AllPrincipals or admin consent. + /// + [Fact] + public async Task SetupAll_NoAuthMode_DefaultsToOboBehaviour() + { + _mockConfigService.LoadAsync(Arg.Any(), Arg.Any()).Returns(Task.FromResult(BlueprintConfig())); + var parser = new CommandLineBuilder(BuildSetupCommand()).Build(); + + var result = await parser.InvokeAsync("all --aiteammate false --dry-run", new TestConsole()); + + result.Should().Be(0, because: "omitting --authmode is valid and dry-run exits 0"); + _mockLogger.Received().Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("obo")), + Arg.Any(), + Arg.Any>()); + } } From 62eaa667e3793efe670f7f75655b7ab5c265cb8a Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Tue, 28 Apr 2026 11:51:42 -0700 Subject: [PATCH 2/6] Update setup all dry-run output and clarify error messages - Dry-run no longer detects tenant ID via az CLI when not needed. - "Inheritable Permissions" now shows "skipped (permissions set directly on agent identity)" for non-AI Teammate agents. - Messaging endpoint step output clarified to "skipped (non-M365 agent)". - Improved error message for --authmode with --aiteammate. - Tests updated for new output and error messages. - Changelog updated to reflect these changes. --- CHANGELOG.md | 2 ++ .../SetupSubcommands/AllSubcommand.cs | 10 +++---- .../NonDwBlueprintSetupOrchestrator.cs | 28 +++++++++---------- .../Commands/SetupSubcommands/SetupHelpers.cs | 6 ++-- .../Commands/SetupCommandTests.cs | 14 +++++++--- 5 files changed, 32 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 559dad75..b91cdd68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,8 @@ a365 setup admin --config-dir "" - `ToolingManifest.json` duplicate server detection now falls back to `mcpServerName` when `mcpServerUniqueName` is absent, preventing false duplicate errors for older manifest entries ### Fixed +- `setup all` dry-run with `--agent-name` no longer runs az CLI tenant detection — tenant ID is not shown in the plan, so the subprocess was unnecessary +- `setup all` live summary incorrectly showed `Inheritable Permissions: configured` for non-AI Teammate agents — now shows `skipped (permissions set directly on agent identity)` - `AgentBlueprintService.SetInheritablePermissionsAsync` no longer crashes when the Graph PATCH call throws a transient exception (#366) — the exception is caught, logged, and surfaced as a structured error result - `cleanup` now returns exit code 1 when no config file and no `--agent-name` are provided, instead of silently reporting success. - `AgentBlueprintService.SetInheritablePermissionsAsync` now correctly propagates `OperationCanceledException` when the user cancels (Ctrl+C), instead of masking cancellation as a generic error 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 d86c329f..4c7fa1a7 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs @@ -180,7 +180,7 @@ public static Command CreateCommand( } if (authMode is not null && aiTeammateFlag == true) { - logger.LogError("--authmode is not supported with --aiteammate true. It only applies to blueprint agents (--aiteammate false)."); + logger.LogError("--authmode is not supported with --aiteammate — AI Teammate agents automatically use OBO via agent user identity."); context.ExitCode = 1; return; } @@ -200,13 +200,11 @@ public static Command CreateCommand( { if (dryRun) { - // Dry-run: detect tenant only (no client app lookup needed for display) - var dryRunTenantId = tenantIdFlag; - if (string.IsNullOrWhiteSpace(dryRunTenantId)) - dryRunTenantId = await SetupHelpers.ResolveBootstrapTenantIdAsync(null, executor, logger); + // Dry-run: build config from flags only — no az CLI subprocess needed. + // TenantId is not shown in the plan so detection is skipped intentionally. nonDwConfig = new Agent365Config { - TenantId = dryRunTenantId ?? "(unknown — run 'az login' or pass --tenant-id)", + TenantId = tenantIdFlag ?? string.Empty, ClientAppId = string.Empty, AgentIdentityDisplayName = $"{agentName} Identity", AgentBlueprintDisplayName = $"{agentName} Blueprint", 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 d81d82a1..eb687732 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs @@ -96,25 +96,25 @@ public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool i logger.LogInformation(sub + "create managed identity"); } - // 3. Inheritable Permissions — skipped in all authMode values (admin involvement avoided) + // 3. Inheritable Permissions — only applicable to AI Teammate (DW) agents; always skipped here. var effectiveMode = (authMode ?? config.AuthMode)?.ToLowerInvariant() ?? "obo"; - logger.LogInformation(SetupHelpers.DryRunRow(3, "Inheritable Permissions") + "skipped (authmode: {AuthMode} — admin involvement avoided)", effectiveMode); + logger.LogInformation(SetupHelpers.DryRunRow(3, "Inheritable Permissions") + "skipped (permissions set directly on agent identity)"); - // 4. Permission Grants — per authMode - if (effectiveMode is "obo" or "both") - logger.LogInformation(SetupHelpers.DryRunRow(4, "Agent Identity Delegated") + "principal-scoped delegated grants (programmatic, no admin required)"); - if (effectiveMode is "s2s" or "both") - logger.LogInformation(SetupHelpers.DryRunRow(4, "Agent Identity App Perms") + "application permissions — attempted programmatically; PowerShell fallback if not Global Admin"); - - // 5. Agent identity + // 4. Agent identity (created before grants so the SP exists to receive them) var agentIdentityDisplayName = config.AgentIdentityDisplayName ?? "Agent"; var agentRegistrationDisplayName = agentIdentityDisplayName.EndsWith(" Identity", StringComparison.OrdinalIgnoreCase) ? agentIdentityDisplayName[..^" Identity".Length].TrimEnd() + " Agent" : agentIdentityDisplayName; if (!string.IsNullOrWhiteSpace(config.AgenticAppId)) - logger.LogInformation(SetupHelpers.DryRunRow(5, "Agent identity") + "reuse: {DisplayName} (ID: {AgentId})", agentIdentityDisplayName, config.AgenticAppId); + logger.LogInformation(SetupHelpers.DryRunRow(4, "Agent identity") + "reuse: {DisplayName} (ID: {AgentId})", agentIdentityDisplayName, config.AgenticAppId); else - logger.LogInformation(SetupHelpers.DryRunRow(5, "Agent identity") + "create: {DisplayName}", agentIdentityDisplayName); + logger.LogInformation(SetupHelpers.DryRunRow(4, "Agent identity") + "create: {DisplayName}", agentIdentityDisplayName); + + // 5. Permission Grants — per authMode, applied to the agent identity SP + if (effectiveMode is "obo" or "both") + logger.LogInformation(SetupHelpers.DryRunRow(5, "Agent Identity Delegated") + "principal-scoped delegated grants (programmatic, no admin required)"); + if (effectiveMode is "s2s" or "both") + logger.LogInformation(SetupHelpers.DryRunRow(5, "Agent Identity App Perms") + "application permissions — attempted programmatically; PowerShell fallback if not Global Admin"); // 6. Agent Registration if (!string.IsNullOrWhiteSpace(config.AgentRegistrationId)) @@ -133,7 +133,7 @@ public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool i } else { - logger.LogInformation(SetupHelpers.DryRunRow(7, "Messaging endpoint") + "skip (non-M365 agent; pass --m365 to enable)"); + logger.LogInformation(SetupHelpers.DryRunRow(7, "Messaging endpoint") + "skipped (non-M365 agent)"); } // 8. Project settings @@ -284,9 +284,7 @@ public static async Task ExecuteAsync(SetupContext ctx) ctx.Results.BatchPermissionsPhase1Completed = true; ctx.Results.BatchPermissionsPhase2Completed = true; ctx.Results.AdminConsentGranted = false; - ctx.Logger.LogInformation( - "Inheritable perms and AllPrincipals grants skipped (authmode: {AuthMode}).", - ctx.AuthMode ?? "obo"); + ctx.Logger.LogInformation("Inheritable perms and AllPrincipals grants skipped (permissions set directly on agent identity)."); // Save state before agent identity steps so progress is not lost on failure. await ctx.ConfigService.SaveStateAsync(ctx.Config); 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 04e16dc3..c21261b8 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -463,7 +463,7 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo if (results.BlueprintFailed) logger.LogInformation(DryRunRow(3 + s, "Inheritable Permissions") + notRun); else if (results.BatchPermissionsPhase1Completed) - logger.LogInformation(DryRunRow(3 + s, "Inheritable Permissions") + "configured"); + logger.LogInformation(DryRunRow(3 + s, "Inheritable Permissions") + (isNonDw ? "skipped (permissions set directly on agent identity)" : "configured")); // Permission Grants: step 4 (non-DW) or step 5 (DW) if (results.BlueprintFailed) @@ -515,7 +515,7 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo switch (results.MessagingEndpointResult) { case null: - logger.LogInformation(DryRunRow(endpointStep, "Messaging endpoint") + "skipped (non-M365 agent; use --m365 to enable)"); + logger.LogInformation(DryRunRow(endpointStep, "Messaging endpoint") + "skipped (non-M365 agent)"); break; case Models.EndpointRegistrationResult.Created: logger.LogInformation( @@ -991,7 +991,7 @@ internal static void PrintDwSetupAllDryRunPlan( } else { - logger.LogInformation(DryRunRow(6, "Messaging endpoint") + "skip (non-M365 agent; pass --m365 to enable)"); + logger.LogInformation(DryRunRow(6, "Messaging endpoint") + "skipped (non-M365 agent)"); } // 7. Project settings (DW has no Agent identity or Agent Registration steps) 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 3bbb7ac3..47c5de5e 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 @@ -590,11 +590,11 @@ public async Task SetupAll_AuthMode_WithAiteammateTrue_ExitsWithCode1() var result = await parser.InvokeAsync("all --aiteammate true --authmode obo", new TestConsole()); - result.Should().Be(1, because: "--authmode is not supported with --aiteammate true"); + result.Should().Be(1, because: "--authmode is not supported with --aiteammate"); _mockLogger.Received().Log( LogLevel.Error, Arg.Any(), - Arg.Is(o => o.ToString()!.Contains("not supported with --aiteammate true")), + Arg.Is(o => o.ToString()!.Contains("not supported with --aiteammate")), Arg.Any(), Arg.Any>()); } @@ -619,7 +619,7 @@ public async Task SetupAll_AuthMode_ValidValues_ExitWithCode0(string authModeVal /// /// Omitting --authmode defaults to OBO behaviour — the dry-run plan must show - /// delegated grants and must not mention AllPrincipals or admin consent. + /// delegated grants (step 5) and must not show S2S app permission grants. /// [Fact] public async Task SetupAll_NoAuthMode_DefaultsToOboBehaviour() @@ -633,7 +633,13 @@ public async Task SetupAll_NoAuthMode_DefaultsToOboBehaviour() _mockLogger.Received().Log( LogLevel.Information, Arg.Any(), - Arg.Is(o => o.ToString()!.Contains("obo")), + Arg.Is(o => o.ToString()!.Contains("Agent Identity Delegated")), + Arg.Any(), + Arg.Any>()); + _mockLogger.DidNotReceive().Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("Agent Identity App Perms")), Arg.Any(), Arg.Any>()); } From 2a8ea89e449bf06c5021ffab8f5ed638ebe197a4 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Tue, 28 Apr 2026 13:52:24 -0700 Subject: [PATCH 3/6] Improve authMode validation, Entra app bootstrap, and tests - Enforces allowed values for authMode in config and CLI, with normalization to prevent silent misconfigurations. - Adds admin prompt for missing CLI app: Global Admins can create and consent or enter an existing ID; non-admins prompted for ID only. - Implements CreateCliClientAppAsync for app/SP creation and admin consent grant, with isFallbackPublicClient and WAM redirect URI set at creation time. - Reverts WellKnownClientAppDisplayName to "Agent 365 CLI" (was left as personal test value "Agent 365 CLISellak"). - Prevents duplicate admin action rows in S2S summary output. - Expands test coverage for all new flows; stubs GraphPatchAsync to eliminate real subprocess call in CreateCliClientAppAsync test. --- .claude/agents/pr-code-reviewer.md | 60 ++++++ CHANGELOG.md | 1 + .../SetupSubcommands/AllSubcommand.cs | 12 ++ .../NonDwBlueprintSetupOrchestrator.cs | 5 +- .../Commands/SetupSubcommands/README.md | 2 +- .../RequirementsSubcommand.cs | 2 +- .../Commands/SetupSubcommands/SetupContext.cs | 2 +- .../Commands/SetupSubcommands/SetupHelpers.cs | 102 +++++++++- .../Models/Agent365Config.cs | 15 ++ .../Services/GraphApiService.cs | 46 ++++- ...DwBlueprintSetupOrchestratorDryRunTests.cs | 4 +- .../Helpers/SetupHelpersBootstrapTests.cs | 175 ++++++++++++++++++ .../Services/GraphApiServiceTests.cs | 86 +++++++++ 13 files changed, 501 insertions(+), 11 deletions(-) diff --git a/.claude/agents/pr-code-reviewer.md b/.claude/agents/pr-code-reviewer.md index 660d7edb..fa997c2a 100644 --- a/.claude/agents/pr-code-reviewer.md +++ b/.claude/agents/pr-code-reviewer.md @@ -186,6 +186,27 @@ For each changed file, analyze: - Based on the conditional logic, what specific test scenarios are needed? - Generate concrete test code examples +9. **Holistic Related-File Expansion** (for patterns that span unstaged files) + + Do not limit analysis to staged/diff lines. For the following patterns, actively fetch and read related unstaged files before generating findings: + + **A. New property on a model class → check validators (even if not staged)** + When the diff adds a new `public` property to any class in `Models/` or `SetupSubcommands/` (e.g., `Agent365Config`, `SetupContext`, `SetupResults`): + - Read the full `Validate()` and `ValidateNonDwMinimal()` methods of that class — they may not be in the diff + - Also `Grep` for `ValidateAsync` methods in `ConfigService.cs` that take the same model type (see Anti-Pattern #25) + - If the new property represents a discrete set of values (e.g., `"obo|s2s|both"`) and no validation exists in any of these methods, flag HIGH (see Anti-Pattern #27) + + **B. New CLI `Option<...>` → read the directory README (even if not staged)** + When the diff adds `new Option<...>(` to a command file under `Commands/`: + - Read `README.md` in that same directory + - If the new option is not documented, flag MEDIUM (missing README update) + - Also verify the README does not claim the option is available on commands where it is not wired — check the command class to confirm scope + + **C. Changed observable log message → grep tests for old string** + When the diff changes a quoted string in a `LogInformation`/`LogWarning`/`LogError` call: + - Run `Grep` for the old string in `src/Tests/**/*.cs` + - If test files assert `Contains("old string")` and the old string is gone, the test will silently pass with the wrong message — flag HIGH + ### Step 3: Generate Findings For each issue found, provide: @@ -820,6 +841,45 @@ When a required-field check is added, removed, or relaxed in a model's `Validate ``` - **Real example**: Removing `"messagingEndpoint is required when needDeployment is 'no'."` from `Agent365Config.Validate()` without removing the parallel `ValidateRequired(config.MessagingEndpoint, ...)` call in `ConfigService.ValidateAsync`. The fix appeared in `Agent365ConfigTests.cs` and `Agent365Config.cs` but not in `ConfigService.cs`, so `a365 cleanup` still failed with `MessagingEndpoint is required` on bootstrap-path projects. +### 27. New Nullable Config Property Without Validation in `Validate()` + +When a new nullable string property is added to a config model class (`Agent365Config`, `SetupContext`, or similar) to represent a discrete set of allowed values (e.g., `obo|s2s|both`), and no validation is added to `Validate()` / `ValidateNonDwMinimal()` / `ValidateAsync()`, the property becomes a silent misconfiguration path: a typo in `a365.config.json` passes through unnoticed and silently disables the feature or skips grant branches. + +- **Pattern to catch**: + - `[JsonPropertyName("someMode")] public string? SomeMode { get; init; }` added to a model class + - No corresponding discrete-value check in `Validate()` / `ValidateNonDwMinimal()` for that property name + - Often paired with Anti-Pattern #28 (assignment without whitespace normalization) +- **Severity**: `high` — incorrect config values silently skip grant/permission branches; user gets no error +- **Check**: For every new property in a model class diff, read the `Validate()` method of that class (even if not staged) and search for the property name. If absent, flag it. +- **Fix**: Add a private helper and call it from all `Validate()` overloads: + ```csharp + private static void ValidateSomeMode(string? value, List errors) + { + if (value is not null && value is not ("a" or "b" or "c")) + errors.Add($"someMode must be 'a', 'b', or 'c' when set (got '{value}')."); + } + ``` +- **Real example (PR #391, Comments 4 & 5)**: `Agent365Config.AuthMode` was added without validation. `a365.config.json` could contain `"authMode": "typo"` and the run would proceed, silently skipping all grant branches. + +### 28. Mode/Enum-Like String Property Normalized With `?.ToLowerInvariant()` Instead of `IsNullOrWhiteSpace` + +When a string property represents a discrete set of values (e.g., `"obo"`, `"s2s"`, `"both"`), the assignment `AuthMode = authMode?.ToLowerInvariant()` looks correct but lets empty string through: `""?.ToLowerInvariant()` returns `""`, not `null`. An empty string from `a365.config.json` then makes `IsOboMode`/`IsS2sMode`/`IsBothMode` all return `false` — silently skipping all grant branches with no error. + +- **Pattern to catch**: + - `SomeMode = someMode?.ToLowerInvariant();` where `SomeMode` is a nullable string used in discrete-value comparisons (e.g., `== "obo"`) + - Appears in `SetupContext` constructors, settings classes, or model property setters +- **Severity**: `high` — empty-string config value silently disables all mode-dependent branches +- **Check**: For every `?.ToLowerInvariant()` assignment in the diff where the left-hand side is compared against a finite set of literals elsewhere, verify the `IsNullOrWhiteSpace` guard is present. +- **Fix**: + ```csharp + // Wrong — empty string passes through as a real mode + AuthMode = authMode?.ToLowerInvariant(); + + // Correct — empty/whitespace becomes null (= use documented default) + AuthMode = string.IsNullOrWhiteSpace(authMode) ? null : authMode.Trim().ToLowerInvariant(); + ``` +- **Real example (PR #391, Comments 7 & 8)**: `SetupContext.AuthMode` and `NonDwBlueprintSetupOrchestrator.effectiveMode` both used `?.ToLowerInvariant()`, allowing `""` to disable OBO without any visible error. + ## Example Invocation When you receive a request like "Review PR #253", you should: diff --git a/CHANGELOG.md b/CHANGELOG.md index b91cdd68..608b3a22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ a365 setup admin --config-dir "" ``` ### Added +- `setup requirements` Global Administrator path: when the well-known CLI client app is not found in a new tenant, Global Admins are offered a three-choice menu to create the app and grant admin consent automatically. - `--authmode obo|s2s|both` option on `setup all` — controls how the agent identity service principal receives permissions: - `obo` (default): principal-scoped delegated grants (`consentType: "Principal"`); no Global Administrator required. - `s2s`: application role assignments on the agent identity SP; attempted programmatically, falls back to printed PowerShell instructions if the caller lacks Global Administrator. 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 4c7fa1a7..590c13b9 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs @@ -279,6 +279,18 @@ public static Command CreateCommand( } } + // Validate the effective authMode (flag OR config). The CLI flag was validated above; + // this re-check catches an invalid authMode persisted in a365.config.json that was not + // caught at load time (e.g. a user manually edited the file with a bad value). + var effectiveAuthModeForValidation = authMode ?? nonDwConfig?.AuthMode?.Trim().ToLowerInvariant(); + if (!string.IsNullOrWhiteSpace(effectiveAuthModeForValidation) && + effectiveAuthModeForValidation is not ("obo" or "s2s" or "both")) + { + logger.LogError("Invalid authMode value '{Value}' (from --authmode flag or a365.config.json). Allowed values: obo, s2s, both.", effectiveAuthModeForValidation); + context.ExitCode = 1; + return; + } + // AI Teammate (DW) agents are M365 agents by design — auto-enable messaging endpoint. // --m365 remains opt-in for blueprint agents (non-DW path). if (nonDwConfig is null) 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 eb687732..e0d09282 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs @@ -97,7 +97,10 @@ public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool i } // 3. Inheritable Permissions — only applicable to AI Teammate (DW) agents; always skipped here. - var effectiveMode = (authMode ?? config.AuthMode)?.ToLowerInvariant() ?? "obo"; + var selectedAuthMode = authMode ?? config.AuthMode; + var effectiveMode = string.IsNullOrWhiteSpace(selectedAuthMode) + ? "obo" + : selectedAuthMode.Trim().ToLowerInvariant(); logger.LogInformation(SetupHelpers.DryRunRow(3, "Inheritable Permissions") + "skipped (permissions set directly on agent identity)"); // 4. Agent identity (created before grants so the SP exists to receive them) 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 aeb09374..ff0fcefd 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md @@ -69,7 +69,7 @@ a365 setup permissions # Configure permissions only ### Authentication mode (`--authmode`) -The `--authmode` option controls how the agent identity service principal is granted permissions. It is available on `setup all`, `setup blueprint`, and `setup permissions mcp/bot/custom`. +The `--authmode` option controls how the agent identity service principal is granted permissions. It is available on `setup all` only. | Value | Behaviour | |-------|-----------| diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/RequirementsSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/RequirementsSubcommand.cs index cef2a168..178f285a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/RequirementsSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/RequirementsSubcommand.cs @@ -153,7 +153,7 @@ public static Command CreateCommand( return null; } - logger.LogInformation("No a365.config.json found. Resolving client app from Entra..."); + logger.LogDebug("No a365.config.json found. Resolving client app from Entra..."); var clientAppId = await SetupHelpers.ResolveBootstrapClientAppIdAsync(tenantId, graphApiService, logger, ct); if (string.IsNullOrWhiteSpace(clientAppId)) { 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 d63de698..2c29ab06 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs @@ -138,7 +138,7 @@ public SetupContext( AgentInstanceOnly = agentInstanceOnly; IsBootstrap = isBootstrap; IsM365 = isM365; - AuthMode = authMode?.ToLowerInvariant(); + AuthMode = string.IsNullOrWhiteSpace(authMode) ? null : authMode.ToLowerInvariant(); ConfigService = configService; Executor = executor; BackendConfigurator = backendConfigurator; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs index c21261b8..147b4587 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -193,10 +193,28 @@ internal static async Task> BuildConfiguredPermissi if (graphApiService == null) return null; - logger.LogError("Entra app \"{AppName}\" was not found in tenant {TenantId}.", + logger.LogInformation("App \"{AppName}\" was not found in tenant {TenantId}.", AuthenticationConstants.WellKnownClientAppDisplayName, tenantId); - Console.Write("Enter your client app ID (or press Enter to cancel): "); - var entered = Console.ReadLine()?.Trim(); + + logger.LogInformation("Checking tenant permissions..."); + var adminCheck = await graphApiService.IsCurrentUserAdminAsync(tenantId, ct); + var isAdmin = adminCheck == Models.RoleCheckResult.HasRole; + + string? entered; + if (isAdmin) + { + Console.Write("Enter a client app ID, or [C] to create one: "); + entered = Console.ReadLine()?.Trim(); + + if (string.Equals(entered, "C", StringComparison.OrdinalIgnoreCase)) + return await CreateAndConsentClientAppAsync(tenantId, graphApiService, logger, ct); + } + else + { + Console.Write("Enter the client app ID: "); + entered = Console.ReadLine()?.Trim(); + } + if (string.IsNullOrWhiteSpace(entered)) { logger.LogInformation("Client app ID entry cancelled."); @@ -217,6 +235,77 @@ internal static async Task> BuildConfiguredPermissi return clientAppId; } + private static async Task CreateAndConsentClientAppAsync( + string tenantId, + GraphApiService graphApiService, + ILogger logger, + CancellationToken ct) + { + logger.LogInformation("Creating app registration '{Name}'...", + AuthenticationConstants.WellKnownClientAppDisplayName); + + var (appId, spId) = await graphApiService.CreateCliClientAppAsync( + tenantId, AuthenticationConstants.WellKnownClientAppDisplayName, ct); + + if (string.IsNullOrWhiteSpace(appId)) + { + logger.LogError("App creation failed. Check errors above."); + return null; + } + + logger.LogInformation("App created: {AppId}", appId); + + // Show all required permissions and ask for consent confirmation. + // AgentRegistration.ReadWrite.All is excluded from RequiredClientAppPermissions because + // ClientAppValidator acquires it via .default to avoid AADSTS650053; here we grant it explicitly. + var consentScopes = AuthenticationConstants.RequiredClientAppPermissions + .Append("AgentRegistration.ReadWrite.All") + .ToArray(); + + logger.LogInformation("The following permissions will be granted on behalf of all users:"); + logger.LogInformation(" Microsoft Graph / Agent 365 API:"); + foreach (var scope in consentScopes) + logger.LogInformation(" - {Scope}", scope); + + Console.Write("Grant admin consent for these permissions? [y/N]: "); + var consentChoice = Console.ReadLine()?.Trim().ToUpperInvariant(); + + if (consentChoice != "Y") + { + logger.LogInformation("Admin consent skipped. Grant consent manually in the Entra portal and run 'a365 setup requirements' again."); + return appId; + } + + if (string.IsNullOrWhiteSpace(spId)) + { + logger.LogWarning("Service principal not found for the new app — cannot grant admin consent automatically."); + logger.LogWarning("Grant consent manually in the Entra portal after the SP propagates."); + return appId; + } + + logger.LogInformation("Granting admin consent..."); + var graphSpId = await graphApiService.LookupServicePrincipalByAppIdAsync( + tenantId, AuthenticationConstants.MicrosoftGraphResourceAppId, ct, + AuthenticationConstants.RequiredPermissionGrantScopes); + + if (string.IsNullOrWhiteSpace(graphSpId)) + { + logger.LogWarning("Microsoft Graph service principal not found in tenant {TenantId} — admin consent could not be granted.", tenantId); + return appId; + } + + var granted = await graphApiService.CreateOrUpdateOauth2PermissionGrantAsync( + tenantId, spId, graphSpId, consentScopes, ct, + AuthenticationConstants.RequiredPermissionGrantScopes); + + if (granted) + logger.LogInformation("Admin consent granted. Run 'a365 setup requirements' again to verify."); + else + logger.LogWarning("Admin consent could not be granted. Grant consent manually in the Entra portal and run 'a365 setup requirements' again."); + + return appId; + } + private static async Task TryGetLocalClientAppIdAsync( string tenantId, ILogger logger, @@ -423,8 +512,13 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo // Developer-scoped grants (OBO authmode) are the intentional alternative to AllPrincipals admin consent. // When AgentIdentityPermissionsGranted is true, no admin action is needed for permission grants. + // In S2S flows, admin action is signalled by pendingS2SAction (Global Admin needed for app role + // assignments). pendingAdminAction is reserved for the inheritable-permissions admin-consent path + // and must be suppressed here to avoid a duplicate row in the summary. + var isS2SFlow = results.S2SAppRoleGranted.HasValue; var pendingAdminAction = !results.AdminConsentGranted && results.BatchPermissionsPhase2Completed - && !results.AgentIdentityPermissionsGranted; + && !results.AgentIdentityPermissionsGranted + && !isS2SFlow; var pendingS2SAction = results.S2SAppRoleGranted == false; // ── Numbered step rows — mirrors the dry-run step list ───────────────── diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/Agent365Config.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/Agent365Config.cs index 6185231c..dfe30532 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/Agent365Config.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/Agent365Config.cs @@ -37,6 +37,7 @@ public List Validate() } if (string.IsNullOrWhiteSpace(AgentIdentityDisplayName)) errors.Add("agentIdentityDisplayName is required."); + ValidateAuthMode(AuthMode, errors); // Validate custom blueprint permissions if (CustomBlueprintPermissions != null && CustomBlueprintPermissions.Count > 0) @@ -81,6 +82,7 @@ public List ValidateNonDwMinimal() else ValidateGuid(ClientAppId, nameof(ClientAppId), errors); if (string.IsNullOrWhiteSpace(AgentIdentityDisplayName)) errors.Add("agentIdentityDisplayName is required."); + ValidateAuthMode(AuthMode, errors); return errors; } @@ -96,6 +98,19 @@ private static void ValidateGuid(string value, string fieldName, List er } } + /// + /// Validates AuthMode is either unset (null/whitespace) or one of "obo", "s2s", "both" (case-insensitive). + /// + private static void ValidateAuthMode(string? value, List errors) + { + if (string.IsNullOrWhiteSpace(value)) return; + var normalised = value.Trim().ToLowerInvariant(); + if (normalised is not ("obo" or "s2s" or "both")) + { + errors.Add($"authMode '{value}' is invalid. Allowed values: obo, s2s, both."); + } + } + // ======================================================================== // STATIC PROPERTIES (init-only) - from a365.config.json // Developer-managed, immutable after construction diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs index b996f302..2f37d54d 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs @@ -628,7 +628,51 @@ public virtual async Task ApplicationExistsByAppIdAsync( return idProp.GetString(); } - public async Task CreateOrUpdateOauth2PermissionGrantAsync( + /// + /// Creates a new Entra app registration (public client) and its service principal for the CLI client app. + /// Used by setup requirements when the well-known CLI client app is not found in a new tenant. + /// Returns (appId, spObjectId), or (null, null) on failure. + /// + public virtual async Task<(string? appId, string? spId)> CreateCliClientAppAsync( + string tenantId, string displayName, CancellationToken ct = default) + { + var body = new + { + displayName, + signInAudience = "AzureADMyOrg", + isFallbackPublicClient = true, + publicClient = new { redirectUris = AuthenticationConstants.RequiredRedirectUris } + }; + + using var appDoc = await GraphPostAsync(tenantId, "/v1.0/applications", body, ct); + if (appDoc == null + || !appDoc.RootElement.TryGetProperty("appId", out var appIdProp) + || !appDoc.RootElement.TryGetProperty("id", out var objIdProp)) + { + _logger.LogError("Failed to create app registration in tenant {TenantId}.", tenantId); + return (null, null); + } + + var appId = appIdProp.GetString(); + var objectId = objIdProp.GetString(); + if (string.IsNullOrWhiteSpace(appId)) return (null, null); + + // Patch in the WAM broker redirect URI — requires the appId to be known first. + if (!string.IsNullOrWhiteSpace(objectId)) + { + var allUris = AuthenticationConstants.GetRequiredRedirectUris(appId); + await GraphPatchAsync(tenantId, $"/v1.0/applications/{objectId}", + new { publicClient = new { redirectUris = allUris } }, ct); + } + + var spId = await EnsureServicePrincipalForAppIdAsync(tenantId, appId, ct); + if (string.IsNullOrWhiteSpace(spId)) + _logger.LogWarning("App created ({AppId}) but service principal creation failed — admin consent may fail until the SP exists.", appId); + + return (appId, spId); + } + + public virtual async Task CreateOrUpdateOauth2PermissionGrantAsync( string tenantId, string clientSpObjectId, string resourceSpObjectId, 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 8fef6ad4..40cec6a0 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 @@ -98,7 +98,7 @@ public void PrintDryRunPlan_InheritablePermissions_AreSkipped() // Phase 2a (inheritable permissions) is skipped in all authMode values to avoid Global Admin // involvement. The dry-run communicates this explicitly rather than listing specific APIs. - AnyLogContains("skipped").Should().BeTrue(because: "inheritable permissions are skipped in all authMode values to avoid Global Admin involvement"); + AnyLogContains("permissions set directly on agent identity").Should().BeTrue(because: "inheritable permissions row must explain that permissions are set directly on the agent identity (not just contain the word 'skipped' which can match unrelated rows)"); } [Fact] @@ -204,6 +204,6 @@ public void PrintDryRunPlan_AllAuthModes_SkipInheritablePermissions(string? auth { NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(BuildConfig(), _logger, authMode: authMode); - AnyLogContains("skipped").Should().BeTrue(because: $"authmode '{authMode ?? "null (obo)"}' must skip inheritable permissions to avoid Global Admin involvement"); + AnyLogContains("permissions set directly on agent identity").Should().BeTrue(because: $"authmode '{authMode ?? "null (obo)"}' must skip inheritable permissions and explain permissions are set directly on the agent identity"); } } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersBootstrapTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersBootstrapTests.cs index de25bc41..29ef669a 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersBootstrapTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersBootstrapTests.cs @@ -340,6 +340,8 @@ public async Task ResolveBootstrapClientAppIdAsync_WhenLookupFails_AndUserEnters _mockGraph.FindApplicationByDisplayNameAsync( Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult(null)); + _mockGraph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.DoesNotHaveRole)); _mockGraph.ApplicationExistsByAppIdAsync( Arg.Any(), enteredId, Arg.Any()) .Returns(Task.FromResult(true)); @@ -369,6 +371,8 @@ public async Task ResolveBootstrapClientAppIdAsync_WhenLookupFails_AndUserCancel _mockGraph.FindApplicationByDisplayNameAsync( Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult(null)); + _mockGraph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.DoesNotHaveRole)); var originalIn = Console.In; Console.SetIn(new StringReader("\n")); @@ -397,6 +401,8 @@ public async Task ResolveBootstrapClientAppIdAsync_WhenLookupFails_AndUserEnters _mockGraph.FindApplicationByDisplayNameAsync( Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult(null)); + _mockGraph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.DoesNotHaveRole)); _mockGraph.ApplicationExistsByAppIdAsync( Arg.Any(), badId, Arg.Any()) .Returns(Task.FromResult(false)); @@ -419,6 +425,175 @@ public async Task ResolveBootstrapClientAppIdAsync_WhenLookupFails_AndUserEnters } } + // ── Global Administrator menu (Create / Enter / Cancel) ─────────────────── + + [Fact] + public async Task ResolveBootstrapClientAppIdAsync_Admin_ChooseCreate_CreatesAppAndReturnsId() + { + // Arrange: well-known Entra app missing, current user is Global Admin, + // user picks (C)reate and confirms (Y)es to consent. + const string newAppId = "new-app-id"; + const string newSpId = "new-sp-id"; + const string graphSpId = "graph-sp-id"; + + _mockGraph.FindApplicationByDisplayNameAsync( + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(null)); + _mockGraph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.HasRole)); + _mockGraph.CreateCliClientAppAsync( + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string?, string?)>((newAppId, newSpId))); + _mockGraph.LookupServicePrincipalByAppIdAsync( + Arg.Any(), + Arg.Is(id => id == AuthenticationConstants.MicrosoftGraphResourceAppId), + Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(graphSpId)); + _mockGraph.CreateOrUpdateOauth2PermissionGrantAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(true)); + + var originalIn = Console.In; + Console.SetIn(new StringReader("C\nY\n")); + try + { + // Act + var result = await SetupHelpers.ResolveBootstrapClientAppIdAsync( + "tenant-id", _mockGraph, NullLogger.Instance, CancellationToken.None); + + // Assert: the newly created app ID is returned and the permission grant was attempted. + result.Should().Be(newAppId, + because: "when an admin chooses (C)reate, the helper must return the appId returned by CreateCliClientAppAsync"); + + await _mockGraph.Received(1).CreateCliClientAppAsync( + Arg.Any(), + Arg.Is(name => name == AuthenticationConstants.WellKnownClientAppDisplayName), + Arg.Any()); + + await _mockGraph.Received(1).CreateOrUpdateOauth2PermissionGrantAsync( + Arg.Any(), + Arg.Is(id => id == newSpId), + Arg.Is(id => id == graphSpId), + Arg.Any>(), + Arg.Any(), + Arg.Any?>()); + } + finally + { + Console.SetIn(originalIn); + } + } + + [Fact] + public async Task ResolveBootstrapClientAppIdAsync_Admin_EntersExistingId_ReturnsId() + { + // Arrange: admin user types an existing app ID directly at the single prompt. + const string existingId = "existing-app-id"; + + _mockGraph.FindApplicationByDisplayNameAsync( + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(null)); + _mockGraph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.HasRole)); + _mockGraph.ApplicationExistsByAppIdAsync( + Arg.Any(), existingId, Arg.Any()) + .Returns(Task.FromResult(true)); + + var originalIn = Console.In; + Console.SetIn(new StringReader($"{existingId}\n")); + try + { + // Act + var result = await SetupHelpers.ResolveBootstrapClientAppIdAsync( + "tenant-id", _mockGraph, NullLogger.Instance, CancellationToken.None); + + // Assert: the entered app ID is returned, app creation was NOT attempted. + result.Should().Be(existingId, + because: "an admin who types an existing app ID at the prompt receives that ID after Graph confirms it exists"); + + await _mockGraph.DidNotReceive().CreateCliClientAppAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + finally + { + Console.SetIn(originalIn); + } + } + + [Fact] + public async Task ResolveBootstrapClientAppIdAsync_Admin_EmptyInput_ReturnsNull() + { + // Arrange: admin presses Enter without typing — cancels the flow. + _mockGraph.FindApplicationByDisplayNameAsync( + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(null)); + _mockGraph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.HasRole)); + + var originalIn = Console.In; + Console.SetIn(new StringReader("\n")); + try + { + // Act + var result = await SetupHelpers.ResolveBootstrapClientAppIdAsync( + "tenant-id", _mockGraph, NullLogger.Instance, CancellationToken.None); + + // Assert + result.Should().BeNull( + because: "empty input at the admin prompt cancels and returns null without creating or verifying any app"); + + await _mockGraph.DidNotReceive().CreateCliClientAppAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + await _mockGraph.DidNotReceive().ApplicationExistsByAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + finally + { + Console.SetIn(originalIn); + } + } + + [Fact] + public async Task ResolveBootstrapClientAppIdAsync_NonAdmin_OnlyIdPrompt() + { + // Arrange: non-admin user — the menu must not be shown; only the existing + // 'Enter your client app ID' prompt runs. + const string existingId = "existing-app-id"; + + _mockGraph.FindApplicationByDisplayNameAsync( + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(null)); + _mockGraph.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.DoesNotHaveRole)); + _mockGraph.ApplicationExistsByAppIdAsync( + Arg.Any(), existingId, Arg.Any()) + .Returns(Task.FromResult(true)); + + var originalIn = Console.In; + // Single-line input — non-admin does not see the menu, so only the existing + // 'Enter your client app ID' prompt consumes input. + Console.SetIn(new StringReader($"{existingId}\n")); + try + { + // Act + var result = await SetupHelpers.ResolveBootstrapClientAppIdAsync( + "tenant-id", _mockGraph, NullLogger.Instance, CancellationToken.None); + + // Assert + result.Should().Be(existingId, + because: "non-admin users skip the create/enter/cancel menu and go straight to the app ID prompt"); + + await _mockGraph.DidNotReceive().CreateCliClientAppAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + finally + { + Console.SetIn(originalIn); + } + } + // ── GetJsonString ───────────────────────────────────────────────────────── [Theory] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs index cb797c56..b0a3306c 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs @@ -864,6 +864,92 @@ public async Task ServicePrincipalExistsAsync_WhenSpNotFound_ReturnsFalse() } #endregion + + #region CreateCliClientAppAsync + + [Fact] + public async Task CreateCliClientAppAsync_OnSuccess_ReturnsAppIdAndSpId() + { + // Arrange — substitute the virtual GraphPostAsync and EnsureServicePrincipalForAppIdAsync + // so we don't need to wire up an HTTP handler. ForPartsOf returns a real instance with + // virtual members substitutable. + var graph = Substitute.ForPartsOf(); + + const string expectedAppId = "11111111-2222-3333-4444-555555555555"; + const string expectedSpId = "sp-object-id-123"; + + var appResponseJson = JsonDocument.Parse($"{{\"id\":\"app-object-id\",\"appId\":\"{expectedAppId}\"}}"); + + graph.GraphPostAsync( + Arg.Any(), + Arg.Is(p => p == "/v1.0/applications"), + Arg.Any(), + Arg.Any(), + Arg.Any?>(), + Arg.Any()) + .Returns(Task.FromResult(appResponseJson)); + + // Stub GraphPatchAsync — called to add the WAM broker redirect URI after creation. + // Without this stub the real implementation spawns 'az account get-access-token'. + graph.GraphPatchAsync( + Arg.Any(), + Arg.Is(p => p.Contains("/v1.0/applications/")), + Arg.Any(), + Arg.Any(), + Arg.Any?>()) + .Returns(Task.FromResult(true)); + + graph.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), + Arg.Is(id => id == expectedAppId), + Arg.Any(), + Arg.Any?>(), + Arg.Any()) + .Returns(Task.FromResult(expectedSpId)); + + // Act + var (appId, spId) = await graph.CreateCliClientAppAsync( + "tenant-123", "Test CLI App", CancellationToken.None); + + // Assert + appId.Should().Be(expectedAppId, + because: "the appId returned by Graph POST /v1.0/applications must be surfaced to the caller"); + spId.Should().Be(expectedSpId, + because: "EnsureServicePrincipalForAppIdAsync result must be returned as the spId"); + } + + [Fact] + public async Task CreateCliClientAppAsync_WhenPostFails_ReturnsNullNull() + { + // Arrange — POST /v1.0/applications returns null (e.g. 4xx/5xx with logged failure) + var graph = Substitute.ForPartsOf(); + + graph.GraphPostAsync( + Arg.Any(), + Arg.Is(p => p == "/v1.0/applications"), + Arg.Any(), + Arg.Any(), + Arg.Any?>(), + Arg.Any()) + .Returns(Task.FromResult(null)); + + // Act + var (appId, spId) = await graph.CreateCliClientAppAsync( + "tenant-123", "Test CLI App", CancellationToken.None); + + // Assert + appId.Should().BeNull( + because: "a failed app creation must surface (null, null) so the caller does not proceed"); + spId.Should().BeNull( + because: "spId is meaningless when the app itself was not created"); + + // SP creation must NOT be attempted when the app POST failed. + await graph.DidNotReceive().EnsureServicePrincipalForAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any()); + } + + #endregion } // Simple test handler that returns queued responses sequentially From 57034217e7bde63fc88f48ece52e944ea2a23b33 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Tue, 28 Apr 2026 14:17:25 -0700 Subject: [PATCH 4/6] Improve S2S app role grant logic and setup summary output Refactor setup summary to accurately reflect S2S app role assignment and delegated grant status, preventing duplicate or misleading "Action Required" rows. Reorder non-DW agent steps to match dry-run order. Add error logging for failed redirect URI patching after app registration. Add unit tests for S2S app role assignment scenarios. Update changelog to clarify Global Admin flows and summarize these improvements. --- CHANGELOG.md | 2 +- .../Commands/SetupSubcommands/SetupHelpers.cs | 61 ++++--- .../Services/GraphApiService.cs | 7 +- ...wBlueprintSetupOrchestratorExecuteTests.cs | 167 ++++++++++++++++++ 4 files changed, 209 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 608b3a22..f75ecf43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,7 @@ a365 setup admin --config-dir "" ``` ### Added -- `setup requirements` Global Administrator path: when the well-known CLI client app is not found in a new tenant, Global Admins are offered a three-choice menu to create the app and grant admin consent automatically. +- `setup requirements` Global Administrator path: when the well-known CLI client app is not found in a new tenant, Global Admins are prompted to create the app and grant admin consent automatically (enter an app ID or type `C` to create). - `--authmode obo|s2s|both` option on `setup all` — controls how the agent identity service principal receives permissions: - `obo` (default): principal-scoped delegated grants (`consentType: "Principal"`); no Global Administrator required. - `s2s`: application role assignments on the agent identity SP; attempted programmatically, falls back to printed PowerShell instructions if the caller lacks Global Administrator. 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 147b4587..267094fb 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -510,16 +510,19 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo logger.LogInformation("Setup Summary"); logger.LogInformation(""); - // Developer-scoped grants (OBO authmode) are the intentional alternative to AllPrincipals admin consent. - // When AgentIdentityPermissionsGranted is true, no admin action is needed for permission grants. - // In S2S flows, admin action is signalled by pendingS2SAction (Global Admin needed for app role - // assignments). pendingAdminAction is reserved for the inheritable-permissions admin-consent path - // and must be suppressed here to avoid a duplicate row in the summary. + // In S2S flows, app role assignment is the relevant success signal for permission grants, + // so the summary must not fall back to AdminConsentGranted when determining whether that + // step is pending. pendingAdminAction remains reserved for the inheritable-permissions + // admin-consent path, while pendingS2SAction is reserved for the S2S app-role-assignment path. var isS2SFlow = results.S2SAppRoleGranted.HasValue; - var pendingAdminAction = !results.AdminConsentGranted && results.BatchPermissionsPhase2Completed - && !results.AgentIdentityPermissionsGranted - && !isS2SFlow; - var pendingS2SAction = results.S2SAppRoleGranted == false; + var permissionGrantsCompleted = isS2SFlow + ? results.S2SAppRoleGranted == true + : results.AdminConsentGranted || results.AgentIdentityPermissionsGranted; + var permissionGrantsPending = isS2SFlow + ? results.S2SAppRoleGranted == false + : !permissionGrantsCompleted && results.BatchPermissionsPhase2Completed; + var pendingAdminAction = permissionGrantsPending && !isS2SFlow; + var pendingS2SAction = permissionGrantsPending && isS2SFlow; // ── Numbered step rows — mirrors the dry-run step list ───────────────── // Non-DW omits the Azure hosting step, so all steps after 1 are shifted down by 1. @@ -559,33 +562,39 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo else if (results.BatchPermissionsPhase1Completed) logger.LogInformation(DryRunRow(3 + s, "Inheritable Permissions") + (isNonDw ? "skipped (permissions set directly on agent identity)" : "configured")); - // Permission Grants: step 4 (non-DW) or step 5 (DW) + // Non-DW only: Agent identity — step 4 (before Permission Grants, matching dry-run order) + if (isNonDw) + { + if (results.BlueprintFailed) + logger.LogInformation(DryRunRow(4, "Agent identity") + notRun); + else if (results.AgentIdentityCreated) + { + var identityVerb = (results.AgentIdentityAlreadyExisted ? "reused" : "created").PadRight(9); + logger.LogInformation(DryRunRow(4, "Agent identity") + identityVerb + " '{Name}' (ID: {Id})", + results.AgentIdentityDisplayName ?? "unknown", results.AgentIdentityId ?? "unknown"); + } + else if (results.AgentIdentityFailed) + logger.LogWarning(DryRunRow(4, "Agent identity") + "failed — see warnings"); + } + + // Permission Grants: step 5 (non-DW: 5, DW: 4+s=5 — same step for both) + var permGrantStep = isNonDw ? 5 : 4 + s; if (results.BlueprintFailed) - logger.LogInformation(DryRunRow(4 + s, "Permission Grants") + notRun); + logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + notRun); + else if (isS2SFlow && results.S2SAppRoleGranted == true) + logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + "ok (S2S app roles)"); else if (results.AgentIdentityPermissionsGranted) - logger.LogInformation(DryRunRow(4 + s, "Permission Grants") + "ok (developer-scoped)"); + logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + "ok (developer-scoped)"); else if (results.BatchPermissionsPhase2Completed) - logger.LogInformation(DryRunRow(4 + s, "Permission Grants") + (results.AdminConsentGranted ? "ok" : "PENDING")); + logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + (results.AdminConsentGranted ? "ok" : "PENDING")); - // Non-DW only: Agent identity (5) and Agent Registration (6) + // Non-DW only: Agent Registration — step 6 if (isNonDw) { if (results.BlueprintFailed) - { - logger.LogInformation(DryRunRow(5, "Agent identity") + notRun); logger.LogInformation(DryRunRow(6, "Agent Registration") + notRun); - } else { - if (results.AgentIdentityCreated) - { - var identityVerb = (results.AgentIdentityAlreadyExisted ? "reused" : "created").PadRight(9); - logger.LogInformation(DryRunRow(5, "Agent identity") + identityVerb + " '{Name}' (ID: {Id})", - results.AgentIdentityDisplayName ?? "unknown", results.AgentIdentityId ?? "unknown"); - } - else if (results.AgentIdentityFailed) - logger.LogWarning(DryRunRow(5, "Agent identity") + "failed — see warnings"); - if (results.AgentInstanceRegistered) { var registrationVerb = (results.AgentRegistrationAlreadyExisted ? "reused" : "registered").PadRight(12); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs index 2f37d54d..1a6c1379 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs @@ -661,8 +661,13 @@ public virtual async Task ApplicationExistsByAppIdAsync( if (!string.IsNullOrWhiteSpace(objectId)) { var allUris = AuthenticationConstants.GetRequiredRedirectUris(appId); - await GraphPatchAsync(tenantId, $"/v1.0/applications/{objectId}", + var redirectUrisPatched = await GraphPatchAsync(tenantId, $"/v1.0/applications/{objectId}", new { publicClient = new { redirectUris = allUris } }, ct); + if (!redirectUrisPatched) + _logger.LogError( + "App created ({AppId}) in tenant {TenantId}, but patching redirect URIs failed for application object {ObjectId}. " + + "The app registration may be missing required redirect URIs and authentication may fail until they are added.", + appId, tenantId, objectId); } var spId = await EnsureServicePrincipalForAppIdAsync(tenantId, appId, ct); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs index 6db90a23..0cba1455 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs @@ -785,4 +785,171 @@ await graph.DidNotReceive().RegisterAgentInstanceAsyncV2( Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } + + // ------------------------------------------------------------------------- + // GrantOrInstructAgentIdentityAppPermissionsAsync tests + // ------------------------------------------------------------------------- + + private static (SetupContext ctx, GraphApiService graph, AgentBlueprintService blueprintService) + BuildS2SGrantTestContext() + { + var graph = Substitute.ForPartsOf(); + + graph.GraphGetAsync( + Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()) + .Returns((System.Text.Json.JsonDocument?)null); + + var config = new Agent365Config + { + AiTeammate = false, + TenantId = "tenant-id", + AgentIdentityDisplayName = "Test Agent", + ClientAppId = "client-app-id", + AgenticAppId = "agentic-app-id", + AuthMode = "s2s", + }; + + var mockExecutor = BuildMockExecutor(); + var configService = Substitute.For(); + configService.SaveStateAsync(Arg.Any(), Arg.Any()) + .Returns(Task.CompletedTask); + + var blueprintService = Substitute.ForPartsOf( + Substitute.For>(), graph); + + var ctx = new SetupContext( + config: config, + results: new SetupResults(), + logger: Substitute.For(), + configFile: new FileInfo("a365.config.json"), + generatedConfigPath: "a365.generated.config.json", + correlationId: "test-correlation-id", + skipInfrastructure: true, + skipRequirements: true, + cancellationToken: CancellationToken.None, + configService: configService, + executor: mockExecutor, + backendConfigurator: Substitute.For(), + authValidator: Substitute.For( + NullLogger.Instance, mockExecutor), + platformDetector: Substitute.ForPartsOf( + Substitute.For>()), + graphApiService: graph, + blueprintService: blueprintService, + blueprintLookupService: Substitute.ForPartsOf( + Substitute.For>(), graph), + federatedCredentialService: Substitute.ForPartsOf( + Substitute.For>(), graph), + clientAppValidator: Substitute.For(), + authMode: "s2s", + loginHintResolver: () => Task.FromResult(null)); + + return (ctx, graph, blueprintService); + } + + private static List OneS2SSpec() => + [new ResourcePermissionSpec("resource-app-id", "Test Resource", [], false, AppRoleScopes: ["TestRole.ReadWrite"])]; + + /// + /// When no spec has AppRoleScopes, the method exits immediately — no SP lookup or grant calls made. + /// + [Fact] + public async Task GrantOrInstructAgentIdentityAppPermissions_NoS2SSpecs_NoCallsMade() + { + var (ctx, graph, blueprintService) = BuildS2SGrantTestContext(); + var specsWithNoAppRoles = new List + { + new("resource-app-id", "Test Resource", ["user_impersonation"], false) + }; + + await NonDwBlueprintSetupOrchestrator.GrantOrInstructAgentIdentityAppPermissionsAsync(ctx, specsWithNoAppRoles); + + await graph.DidNotReceive().EnsureServicePrincipalForAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any()); + await blueprintService.DidNotReceive().GrantAppRoleAssignmentAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any?>(), Arg.Any()); + } + + /// + /// When the agent identity SP cannot be resolved, S2SAppRoleGranted is set to false + /// and no grant calls are made — grants cannot proceed without a target SP. + /// + [Fact] + public async Task GrantOrInstructAgentIdentityAppPermissions_AgentSpNotFound_SetsGrantedFalse_NoGrantCalls() + { + var (ctx, graph, blueprintService) = BuildS2SGrantTestContext(); + + graph.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any()) + .Returns((string?)null); + + await NonDwBlueprintSetupOrchestrator.GrantOrInstructAgentIdentityAppPermissionsAsync(ctx, OneS2SSpec()); + + ctx.Results.S2SAppRoleGranted.Should().Be(false, + because: "when the agent identity SP cannot be resolved, grants cannot proceed and the result must be false"); + await blueprintService.DidNotReceive().GrantAppRoleAssignmentAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any?>(), Arg.Any()); + } + + /// + /// When all GrantAppRoleAssignmentAsync calls return true, S2SAppRoleGranted is true + /// and no warning is added — this is the Global Admin success path. + /// + [Fact] + public async Task GrantOrInstructAgentIdentityAppPermissions_AllGrantsSucceed_SetsGrantedTrue_NoWarnings() + { + var (ctx, graph, blueprintService) = BuildS2SGrantTestContext(); + + graph.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any()) + .Returns("agent-sp-object-id"); + + blueprintService.GrantAppRoleAssignmentAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any?>(), Arg.Any()) + .Returns(Task.FromResult(true)); + + await NonDwBlueprintSetupOrchestrator.GrantOrInstructAgentIdentityAppPermissionsAsync(ctx, OneS2SSpec()); + + ctx.Results.S2SAppRoleGranted.Should().Be(true, + because: "when all app role assignments succeed, S2SAppRoleGranted must be true"); + ctx.Results.HasWarnings.Should().BeFalse( + because: "a successful S2S grant must not add any warnings to setup results"); + } + + /// + /// When GrantAppRoleAssignmentAsync returns false (caller lacks Global Admin), + /// S2SAppRoleGranted is false, a warning is added, and PowerShell instructions are logged. + /// + [Fact] + public async Task GrantOrInstructAgentIdentityAppPermissions_GrantFails_SetsGrantedFalse_AddsWarning() + { + var (ctx, graph, blueprintService) = BuildS2SGrantTestContext(); + + graph.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any()) + .Returns("agent-sp-object-id"); + + blueprintService.GrantAppRoleAssignmentAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any?>(), Arg.Any()) + .Returns(Task.FromResult(false)); + + await NonDwBlueprintSetupOrchestrator.GrantOrInstructAgentIdentityAppPermissionsAsync(ctx, OneS2SSpec()); + + ctx.Results.S2SAppRoleGranted.Should().Be(false, + because: "when app role assignments fail (non-admin path), S2SAppRoleGranted must be false"); + ctx.Results.HasWarnings.Should().BeTrue( + because: "a failed S2S grant must add a warning so the setup summary shows Action Required"); + ctx.Results.Warnings.Should().ContainSingle() + .Which.Should().Contain("PowerShell", + because: "the warning must reference the printed PowerShell instructions so the user knows where to look"); + } } From bda271a1aa5d86f9060239b742edf123daecd5de Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Tue, 28 Apr 2026 18:15:14 -0700 Subject: [PATCH 5/6] Update admin consent flow and permissions for agent blueprints - Switch blueprint SP creation to /serviceprincipals/graph.agentIdentityBlueprintPrincipal per MSFT guidance - Revise admin consent instructions: Option A (Entra portal) now covers only delegated permissions; application permissions for agent identity must be granted via PowerShell (Option B) - Remove step 7 from Option A and add note redirecting to PowerShell - Update PowerShell instructions to use agent identity SP object ID and assign app roles only to agent identity in non-DW flows - Add AgentIdentity.Read.All to required permissions and clarify Application.ReadWrite.All is not needed for blueprint creation - Change agent registrations endpoint to /beta/copilot/agentRegistrations - Update unit tests to match new consent flow and permission requirements - Aligns with latest Agent ID team guidance for correct admin consent and permission assignment --- CHANGELOG.md | 3 ++ .../SetupSubcommands/BlueprintSubcommand.cs | 2 +- .../Commands/SetupSubcommands/SetupHelpers.cs | 46 +++++++------------ .../Constants/AuthenticationConstants.cs | 17 +++++-- .../Services/AgentBlueprintService.cs | 2 +- .../Services/GraphApiService.cs | 3 +- ...tupHelpersAdminConsentInstructionsTests.cs | 35 ++++++++------ .../Services/ClientAppValidatorTests.cs | 4 +- 8 files changed, 58 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f75ecf43..3b719608 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -106,6 +106,9 @@ a365 setup admin --config-dir "" - `a365 develop add-mcp-servers` no longer writes the literal string `"null"` as a scope value in `ToolingManifest.json` when the V2 catalog returns `"scope": "null"` — the field is omitted, allowing correct fallback to name-based scope mapping - `a365 develop get-token` no longer requests a token with scope `"null"` when a manifest entry has a null scope from the V2 catalog - `a365 setup permissions mcp` no longer passes a literal `"default"` string as an AAD resourceAppId — Dataverse custom servers (`McpServers.DataverseCustom.All`, `McpServers.Dataverse.All`) with `"audience": "default"` are now bucketed under the shared ATG AppId, the same as missing or `api://` legacy audiences +- `a365 setup blueprint` (non-DW) blueprint service principal creation no longer returns 403 — the CLI now uses `POST /v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal` (Agent ID-specific endpoint) instead of the generic `/v1.0/servicePrincipals`, which required `Application.ReadWrite.All` +- `a365 setup all` (non-DW) agent identity idempotency pre-check no longer returns 403 — uses `AgentIdentity.Read.All` scope for `GET /beta/servicePrincipals/microsoft.graph.agentIdentity?$filter=agentIdentityBlueprintId eq '...'` +- Agent registration endpoint promoted from `/stagingbeta/copilot/agentRegistrations` to `/beta/copilot/agentRegistrations` ### Removed - `a365 deploy` command (`deploy app`, `deploy mcp`) — Azure App Service hosting is no longer managed by the CLI. Provide a `messagingEndpoint` in `a365.config.json` pointing to your externally hosted agent. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs index 5779f93f..bfb9924a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs @@ -1215,7 +1215,7 @@ public static async Task EnsureDelegatedConsentWithRetriesAsync( ILogger logger, CancellationToken ct) { - var createSpUrl = $"{Constants.GraphApiConstants.BaseUrl}/v1.0/servicePrincipals"; + var createSpUrl = $"{Constants.GraphApiConstants.BaseUrl}/v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal"; var spManifestJson = new JsonObject { ["appId"] = appId }.ToJsonString(); int forbiddenRetries = 0; const int maxForbiddenRetries = 3; 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 267094fb..2615e2d0 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -364,38 +364,29 @@ internal static async Task> BuildConfiguredPermissi internal static void LogNonDwAdminConsentInstructions( ILogger logger, string blueprintId, - string? agentIdentityAppId = null, + string? agentIdentitySpObjectId = null, + string? agentIdentityDisplayName = null, IReadOnlyList<(string ResourceName, string ResourceAppId, string Scope, string PermissionType)>? specs = null) { specs ??= NonDwAdminConsentSpecs; var appSpecs = specs.Where(s => s.PermissionType == "Application").ToList(); var delegatedSpecs = specs.Where(s => s.PermissionType == "Delegated").ToList(); - var hasAgentIdentity = !string.IsNullOrWhiteSpace(agentIdentityAppId); + var hasAgentIdentity = !string.IsNullOrWhiteSpace(agentIdentitySpObjectId); - // Option A — Entra portal + // Option A — Entra portal (Delegated permissions on blueprint only) logger.LogInformation(" Option A — Entra portal:"); - logger.LogInformation(" 1. Open https://entra.microsoft.com"); - logger.LogInformation(" 2. Navigate to: Identity > Applications > App registrations"); - logger.LogInformation(" 3. Search for the blueprint app by ID: {BlueprintId}", blueprintId); - logger.LogInformation(" (switch to 'All applications' tab if not shown under 'Owned applications')"); - logger.LogInformation(" 4. Open the app, go to: API permissions"); - logger.LogInformation(" 5. Confirm the following permissions are listed:"); - foreach (var group in specs.GroupBy(s => (s.ResourceName, s.Scope))) - { - var types = string.Join(", ", group.Select(s => s.PermissionType)); - logger.LogInformation(" - {ResourceName,-20}: {Scope} ({PermTypes})", group.Key.ResourceName, group.Key.Scope, types); - } + logger.LogInformation(" 1. Open https://entra.microsoft.com and sign in as a Global Administrator"); + logger.LogInformation(" 2. Use the search bar at the top of the portal, search for: {BlueprintId}", blueprintId); + logger.LogInformation(" 3. Select the blueprint app under 'App registrations'"); + logger.LogInformation(" 4. Go to: API permissions"); + logger.LogInformation(" 5. Add the following permissions (click 'Add a permission' for each):"); + foreach (var group in delegatedSpecs.GroupBy(s => (s.ResourceName, s.Scope))) + logger.LogInformation(" - {ResourceName,-20}: {Scope} (Delegated)", group.Key.ResourceName, group.Key.Scope); logger.LogInformation(" 6. Click 'Grant admin consent for your organization' and confirm"); if (hasAgentIdentity && appSpecs.Count > 0) { - logger.LogInformation(" 7. Grant Application permissions to the agent identity (required for S2S token acquisition):"); - logger.LogInformation(" - Search for the agent identity app by ID: {AgentIdentityAppId}", agentIdentityAppId); - logger.LogInformation(" - Open the app, go to: API permissions"); - logger.LogInformation(" - Confirm the following Application permissions are listed:"); - foreach (var (resourceName, _, scope, _) in appSpecs) - logger.LogInformation(" - {ResourceName,-20}: {Scope} (Application)", resourceName, scope); - logger.LogInformation(" - Click 'Grant admin consent for your organization' and confirm"); + logger.LogInformation(" Note: Application permissions for the agent identity (step 7) must be granted via PowerShell (Option B below)."); } // Option B — PowerShell (Microsoft Graph SDK) @@ -411,11 +402,11 @@ internal static void LogNonDwAdminConsentInstructions( logger.LogInformation(" Connect-MgGraph -Scopes {Scopes}", string.Join(", ", mgScopes)); logger.LogInformation(" $bp = Get-MgServicePrincipal -Filter \"appId eq '{BlueprintId}'\"", blueprintId); if (hasAgentIdentity) - logger.LogInformation(" $ai = Get-MgServicePrincipal -Filter \"appId eq '{AgentIdentityAppId}'\"", agentIdentityAppId); + logger.LogInformation(" $ai = Get-MgServicePrincipal -ServicePrincipalId '{AgentIdentitySpObjectId}'", agentIdentitySpObjectId); - if (appSpecs.Count > 0) + if (hasAgentIdentity && appSpecs.Count > 0) { - logger.LogInformation(" # Application permissions (app role assignments — blueprint and agent identity)"); + logger.LogInformation(" # Application permissions (app role assignments — agent identity only for non-AI teammate)"); foreach (var (resourceName, resourceAppId, scope, _) in appSpecs) { var varName = "$" + resourceName.Replace(" ", "").Replace("API", "").ToLowerInvariant(); @@ -423,11 +414,8 @@ internal static void LogNonDwAdminConsentInstructions( varName, resourceAppId, resourceName); logger.LogInformation(" $rid = ({Var}.AppRoles | Where-Object {{ $_.Value -eq '{Scope}' }}).Id", varName, scope); - logger.LogInformation(" New-MgServicePrincipalAppRoleAssignment -ServicePrincipalId $bp.Id -PrincipalId $bp.Id -ResourceId {Var}.Id -AppRoleId $rid", + logger.LogInformation(" New-MgServicePrincipalAppRoleAssignment -ServicePrincipalId $ai.Id -PrincipalId $ai.Id -ResourceId {Var}.Id -AppRoleId $rid", varName); - if (hasAgentIdentity) - logger.LogInformation(" New-MgServicePrincipalAppRoleAssignment -ServicePrincipalId $ai.Id -PrincipalId $ai.Id -ResourceId {Var}.Id -AppRoleId $rid", - varName); } } @@ -697,7 +685,7 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo else { logger.LogInformation(" {N}. Permission Grants — a Global Administrator must grant admin consent in the Entra portal:", actionCount); - LogNonDwAdminConsentInstructions(logger, adminCmdBlueprintId, results.AgentIdentityId); + LogNonDwAdminConsentInstructions(logger, adminCmdBlueprintId, results.AgentIdentityId, results.AgentIdentityDisplayName); } } if (pendingS2SAction) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs index 2a838478..110c9dcd 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs @@ -198,8 +198,9 @@ public static string[] GetRequiredRedirectUris(string clientAppId) /// /// Delegated scope for full read/write access to Entra ID applications. - /// No longer in RequiredClientAppPermissions — replaced by AgentIdentityBlueprintPrincipal.Create - /// for blueprint SP creation per Agent ID team guidance. + /// Not required for blueprint SP creation — the correct endpoint is + /// POST /v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal which accepts + /// AgentIdentityBlueprintPrincipal.Create alone (confirmed by Kyle Marsh, Agent ID team). /// Retained as a named constant for reference and potential future use. /// public const string ApplicationReadWriteAllScope = "Application.ReadWrite.All"; @@ -214,7 +215,7 @@ public static string[] GetRequiredRedirectUris(string clientAppId) /// public static readonly string[] RequiredClientAppPermissions = new[] { - "AgentIdentityBlueprintPrincipal.Create", // Required for POST /v1.0/servicePrincipals (blueprint SP creation) — per Agent ID team (Kyle Marsh) + "AgentIdentityBlueprintPrincipal.Create", // Required for POST /v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal (blueprint SP creation) "AgentIdentityBlueprint.ReadWrite.All", "AgentIdentityBlueprint.UpdateAuthProperties.All", "AgentIdentityBlueprint.AddRemoveCreds.All", // Required for passwordCredentials and FICs during setup and cleanup @@ -222,7 +223,7 @@ public static string[] GetRequiredRedirectUris(string clientAppId) "Directory.Read.All", "AgentInstance.ReadWrite.All", // Required for POST /beta/agentRegistry/agentInstances (AdminSubcommand, PublishCommand) // AgentRegistration.ReadWrite.All (resource: 00000003-0000-0000-c000-000000000000, ID: 20f263bf-7d50-4e66-912c-16b4b4194fd4) - // is required for POST/DELETE /stagingbeta/copilot/agentRegistrations. It is acquired via .default + // is required for POST/DELETE /beta/copilot/agentRegistrations. It is acquired via .default // on the custom app token provider (not enumerated explicitly) to avoid AADSTS650053. // This permission must be configured on the custom app via the portal but is not validated here // because ClientAppValidator queries /v1.0/oauth2PermissionGrants which only returns consented @@ -232,6 +233,7 @@ public static string[] GetRequiredRedirectUris(string clientAppId) // AgentIdentity.Create.All is a delegated scope used by CreateAgentIdentityDelegatedAsync // (POST /beta/servicePrincipals/Microsoft.Graph.AgentIdentity). Requires Agent ID Developer role. "AgentIdentityBlueprint.DeleteRestore.All", // Required for 'a365 cleanup' to delete the Agent Blueprint application + "AgentIdentity.Read.All", // Required for GET /beta/servicePrincipals/microsoft.graph.agentIdentity?$filter=agentIdentityBlueprintId (idempotency check) "AgentIdentity.DeleteRestore.All", // Required for 'a365 cleanup' to delete the Agent Identity service principal "User.Read", // Required for /me endpoint to resolve the signed-in user's object ID for blueprint owner/sponsor assignment "User.ReadWrite.All", // Required for agent user creation, usage location update, and license assignment @@ -275,6 +277,13 @@ public static string[] GetRequiredRedirectUris(string clientAppId) $"{MicrosoftGraphResourceUri}/User.Read" }; + /// + /// Delegated scope for reading Agent Identities as another client (portal, CLI, management tool). + /// Required for GET /beta/servicePrincipals/microsoft.graph.agentIdentity?$filter=agentIdentityBlueprintId eq '...' + /// Per the Agent ID permissions reference (other-client read row): AgentIdentity.Read.All. + /// + public const string AgentIdentityReadAllScope = "AgentIdentity.Read.All"; + /// /// Delegated scope for creating an Agent Identity (service principal) from a blueprint. /// Used by POST /beta/servicePrincipals/Microsoft.Graph.AgentIdentity with agentIdentityBlueprintId. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs index c2a3a93b..baecd0a2 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs @@ -176,7 +176,7 @@ public virtual async Task> GetAgentInstancesFor string blueprintId, CancellationToken cancellationToken = default) { - var requiredScopes = new[] { AuthenticationConstants.AgentIdentityBlueprintReadWriteAllScope }; + var requiredScopes = new[] { AuthenticationConstants.AgentIdentityReadAllScope }; var encodedId = Uri.EscapeDataString(blueprintId); // Fetch agent identity SPs and agent users for this blueprint sequentially to avoid races on shared HTTP headers diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs index 1a6c1379..52388dd3 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs @@ -46,8 +46,7 @@ public class GraphApiService // Graph path for the copilot agent registrations endpoint. // Both RegisterAgentInstanceAsyncV2 and DeleteAgentRegistrationAsync use this path. - // TODO: change from stagingbeta to beta before merging to main. - private const string AgentRegistrationsPath = "/stagingbeta/copilot/agentRegistrations"; + private const string AgentRegistrationsPath = "/beta/copilot/agentRegistrations"; /// /// Optional custom client app ID to use for authentication with Microsoft Graph PowerShell. 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 9386e854..d38bab7e 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 @@ -32,16 +32,18 @@ public void Log(LogLevel logLevel, EventId eventId, TState state, Except } [Fact] - public void LogNonDwAdminConsentInstructions_WithAgentIdentityAppId_EmitsOptionAStep7() + public void LogNonDwAdminConsentInstructions_WithAgentIdentitySpObjectId_OptionAOmitsStep7AndShowsNote() { var logger = new CapturingLogger(); SetupHelpers.LogNonDwAdminConsentInstructions(logger, BlueprintId, AgentIdentityId); - logger.AllOutput.Should().Contain("7. Grant Application permissions to the agent identity", - because: "step 7 must appear when agentIdentityAppId is provided"); + logger.AllOutput.Should().NotContain("7. Grant Application permissions", + because: "agent identity Application permissions are granted via PowerShell (Option B), not the Entra portal; step 7 was intentionally removed from Option A"); + logger.AllOutput.Should().Contain("Application permissions for the agent identity", + because: "a note redirecting the admin to Option B PowerShell must appear when agent identity is provided"); logger.AllOutput.Should().Contain(AgentIdentityId, - because: "agent identity app ID must appear in step 7"); + because: "agent identity SP object ID must appear in the PowerShell $ai lookup in Option B"); } [Fact] @@ -82,14 +84,16 @@ public void LogNonDwAdminConsentInstructions_WithoutAgentIdentityAppId_OmitsAgen } [Fact] - public void LogNonDwAdminConsentInstructions_AlwaysEmitsBlueprintAppRoleAssignmentInOptionB() + public void LogNonDwAdminConsentInstructions_WithAgentIdentity_EmitsAgentIdentityAppRoleAssignmentInOptionB() { var logger = new CapturingLogger(); - SetupHelpers.LogNonDwAdminConsentInstructions(logger, BlueprintId); + SetupHelpers.LogNonDwAdminConsentInstructions(logger, BlueprintId, AgentIdentityId); - logger.AllOutput.Should().Contain("-ServicePrincipalId $bp.Id -PrincipalId $bp.Id", - because: "blueprint app role assignment must always be emitted regardless of agent identity"); + logger.AllOutput.Should().Contain("-ServicePrincipalId $ai.Id -PrincipalId $ai.Id", + because: "non-AI teammate S2S app role must be assigned to the agent identity SP — the blueprint is not the token-acquiring principal in the non-DW flow"); + logger.AllOutput.Should().NotContain("-ServicePrincipalId $bp.Id -PrincipalId $bp.Id", + because: "the blueprint must not receive the app role in the non-DW flow; only the agent identity acquires S2S tokens"); } [Fact] @@ -106,23 +110,24 @@ public void LogNonDwAdminConsentInstructions_AlwaysEmitsDelegatedGrantForBluepri } [Fact] - public void LogNonDwAdminConsentInstructions_OptionAStep5_GroupsPermissionTypesForSameScope() + public void LogNonDwAdminConsentInstructions_OptionAStep5_ShowsDelegatedPermissionsOnly() { var logger = new CapturingLogger(); SetupHelpers.LogNonDwAdminConsentInstructions(logger, BlueprintId); - // Observability API has both Application and Delegated for the same scope. - // Step 5 must list it once, not twice. + // Step 5 covers the blueprint's delegated grants only. Application (S2S) permissions + // go to the agent identity SP via Option B PowerShell — they do not appear in the + // blueprint's "API permissions" pane in the Entra portal. var obsLines = logger.Messages .Where(m => m.Contains("Observability API") && m.Contains(ConfigConstants.ObservabilityApiOtelWriteScope)) .ToList(); obsLines.Should().HaveCount(1, - because: "Observability API scope must appear once in step 5, grouped for both permission types"); - obsLines[0].Should().Contain("Application", - because: "Application permission type must be listed"); + because: "Observability API delegated scope must appear exactly once in step 5"); obsLines[0].Should().Contain("Delegated", - because: "Delegated permission type must be listed"); + because: "step 5 shows only delegated grants for the blueprint"); + obsLines[0].Should().NotContain("Application", + because: "Application permissions for the Observability API are granted to the agent identity, not the blueprint, and are handled by Option B PowerShell"); } [Fact] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs index 0c587b28..4319cc38 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs @@ -905,8 +905,8 @@ private void SetupConsentGrantForAgentIdentityCreate() Arg.Any?>()) .Returns(_ => Task.FromResult(JsonDocument.Parse(spJson))); - // Grants for ConsentSpObjId — contains all three no-GUID scopes so they are removed from missingPermissions. - var grantsJson = """{"value": [{"scope": "AgentIdentity.Create.All AgentIdentityBlueprint.DeleteRestore.All AgentIdentity.DeleteRestore.All"}]}"""; + // Grants for ConsentSpObjId — contains all four no-GUID scopes so they are removed from missingPermissions. + var grantsJson = """{"value": [{"scope": "AgentIdentity.Create.All AgentIdentityBlueprint.DeleteRestore.All AgentIdentity.DeleteRestore.All AgentIdentity.Read.All"}]}"""; _graphApiService.GraphGetAsync( Arg.Any(), Arg.Is(p => p.Contains("oauth2PermissionGrants") && p.Contains(ConsentSpObjId)), From c3061d569065941a367582a35a10586a2266eed7 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Tue, 28 Apr 2026 18:52:52 -0700 Subject: [PATCH 6/6] Improve agent identity SP handling and grant reporting - Treat AgenticAppId as the SP object ID directly, removing the need for appId-to-SP lookup in non-DW flows. - Track and report the effective auth mode ("obo", "s2s", "both") in setup results for more accurate permission grant summaries. - Enhance PowerShell instructions: add 'Directory.Read.All' scope and optionally display agent identity SP display name. - Refine summary logic to distinguish between full, partial, and pending permission grants, especially in "both" mode. - Add EffectiveAuthMode property to SetupResults. - Update tests to reflect new SP object ID handling and clarify test intent. - Improves correctness, clarity, and user feedback for S2S and delegated grant scenarios. --- .../NonDwBlueprintSetupOrchestrator.cs | 19 +++++------- .../Commands/SetupSubcommands/SetupHelpers.cs | 31 +++++++++++++------ .../Commands/SetupSubcommands/SetupResults.cs | 7 +++++ ...wBlueprintSetupOrchestratorExecuteTests.cs | 17 +++++----- 4 files changed, 43 insertions(+), 31 deletions(-) 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 e0d09282..98283f7f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs @@ -403,6 +403,8 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( // Step 5a: Grant permissions to the agent identity, gated by authMode. if (!string.IsNullOrWhiteSpace(ctx.Config.AgenticAppId)) { + ctx.Results.EffectiveAuthMode = ctx.IsBothMode ? "both" : ctx.IsS2sMode ? "s2s" : "obo"; + // OBO and Both: principal-scoped delegated grants (no admin required). if (ctx.IsOboMode || ctx.IsBothMode) await GrantAgentIdentityPermissionsAsync(ctx, specs); @@ -653,19 +655,12 @@ internal static async Task GrantOrInstructAgentIdentityAppPermissionsAsync( return; } - // Resolve the agent identity service principal object ID. - var agentIdentitySpObjectId = await ctx.GraphApiService.EnsureServicePrincipalForAppIdAsync( - ctx.Config.TenantId!, - ctx.Config.AgenticAppId!, - ctx.CancellationToken, - Constants.AuthenticationConstants.RequiredPermissionGrantScopes); - + // AgenticAppId is the SP object ID returned by CreateAgentIdentityDelegatedAsync / + // FindExistingAgentIdentityAsync — use it directly without an appId→SP lookup. + var agentIdentitySpObjectId = ctx.Config.AgenticAppId; if (string.IsNullOrWhiteSpace(agentIdentitySpObjectId)) { - ctx.Logger.LogWarning( - "Could not resolve service principal for agent identity ({AgentId}). " + - "App role assignments must be granted manually.", - ctx.Config.AgenticAppId); + ctx.Logger.LogWarning("Agent identity SP object ID is missing. App role assignments must be granted manually."); ctx.Results.S2SAppRoleGranted = false; return; } @@ -706,7 +701,7 @@ internal static async Task GrantOrInstructAgentIdentityAppPermissionsAsync( ctx.Logger.LogInformation("S2S app role assignments require Global Administrator. Run the following PowerShell as an admin:"); ctx.Logger.LogInformation(""); ctx.Logger.LogInformation(" # Connect to Microsoft Graph"); - ctx.Logger.LogInformation(" Connect-MgGraph -TenantId '{TenantId}' -Scopes 'AppRoleAssignment.ReadWrite.All'", ctx.Config.TenantId); + ctx.Logger.LogInformation(" Connect-MgGraph -TenantId '{TenantId}' -Scopes 'AppRoleAssignment.ReadWrite.All', 'Directory.Read.All'", ctx.Config.TenantId); ctx.Logger.LogInformation(""); ctx.Logger.LogInformation(" $agentSpId = '{AgentSpId}'", agentIdentitySpObjectId); 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 2615e2d0..7dc3df99 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -402,7 +402,12 @@ internal static void LogNonDwAdminConsentInstructions( logger.LogInformation(" Connect-MgGraph -Scopes {Scopes}", string.Join(", ", mgScopes)); logger.LogInformation(" $bp = Get-MgServicePrincipal -Filter \"appId eq '{BlueprintId}'\"", blueprintId); if (hasAgentIdentity) - logger.LogInformation(" $ai = Get-MgServicePrincipal -ServicePrincipalId '{AgentIdentitySpObjectId}'", agentIdentitySpObjectId); + { + if (!string.IsNullOrWhiteSpace(agentIdentityDisplayName)) + logger.LogInformation(" $ai = Get-MgServicePrincipal -ServicePrincipalId '{AgentIdentitySpObjectId}' # {DisplayName}", agentIdentitySpObjectId, agentIdentityDisplayName); + else + logger.LogInformation(" $ai = Get-MgServicePrincipal -ServicePrincipalId '{AgentIdentitySpObjectId}'", agentIdentitySpObjectId); + } if (hasAgentIdentity && appSpecs.Count > 0) { @@ -498,14 +503,17 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo logger.LogInformation("Setup Summary"); logger.LogInformation(""); - // In S2S flows, app role assignment is the relevant success signal for permission grants, - // so the summary must not fall back to AdminConsentGranted when determining whether that - // step is pending. pendingAdminAction remains reserved for the inheritable-permissions - // admin-consent path, while pendingS2SAction is reserved for the S2S app-role-assignment path. + // Derive per-grant-type completion so "both" mode surfaces partial results correctly. + // isS2SFlow: S2S was attempted (S2SAppRoleGranted is non-null). + // isBothMode: both S2S and delegated grants were attempted — must check each independently. var isS2SFlow = results.S2SAppRoleGranted.HasValue; + var isBothMode = string.Equals(results.EffectiveAuthMode, "both", StringComparison.OrdinalIgnoreCase); + var s2sOk = isS2SFlow && results.S2SAppRoleGranted == true; + var delegatedOk = results.AdminConsentGranted || results.AgentIdentityPermissionsGranted; + var permissionGrantsCompleted = isS2SFlow - ? results.S2SAppRoleGranted == true - : results.AdminConsentGranted || results.AgentIdentityPermissionsGranted; + ? s2sOk && (!isBothMode || delegatedOk) + : delegatedOk; var permissionGrantsPending = isS2SFlow ? results.S2SAppRoleGranted == false : !permissionGrantsCompleted && results.BatchPermissionsPhase2Completed; @@ -569,8 +577,13 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo var permGrantStep = isNonDw ? 5 : 4 + s; if (results.BlueprintFailed) logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + notRun); - else if (isS2SFlow && results.S2SAppRoleGranted == true) - logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + "ok (S2S app roles)"); + else if (isS2SFlow && s2sOk) + { + if (isBothMode && !delegatedOk) + logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + "partial (S2S ok; delegated — see warnings)"); + else + logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + "ok (S2S app roles)"); + } else if (results.AgentIdentityPermissionsGranted) logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + "ok (developer-scoped)"); else if (results.BatchPermissionsPhase2Completed) 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 f3d1b1a1..37dfafdb 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs @@ -134,6 +134,13 @@ public class SetupResults /// public bool AgentIdentityPermissionsGranted { get; set; } + /// + /// The effective --authmode value used during the non-DW grant step ("obo", "s2s", or "both"). + /// Null when the grant step was not reached (e.g. agent identity creation failed). + /// Used by DisplaySetupSummary to compute per-grant-type completion for the "both" mode. + /// + public string? EffectiveAuthMode { get; set; } + /// /// Whether the Agent Identity was successfully created via the Agent Identity Graph API. /// Populated by the non-DW blueprint setup flow only. diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs index 0cba1455..0e8c1c67 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs @@ -874,23 +874,20 @@ await blueprintService.DidNotReceive().GrantAppRoleAssignmentAsync( } /// - /// When the agent identity SP cannot be resolved, S2SAppRoleGranted is set to false - /// and no grant calls are made — grants cannot proceed without a target SP. + /// When AgenticAppId is missing (the SP object ID was never stored), S2SAppRoleGranted + /// is set to false and no grant calls are made — the SP ID is required as the grant target. + /// AgenticAppId holds the SP object ID directly (not an app ID); no lookup is performed. /// [Fact] - public async Task GrantOrInstructAgentIdentityAppPermissions_AgentSpNotFound_SetsGrantedFalse_NoGrantCalls() + public async Task GrantOrInstructAgentIdentityAppPermissions_AgentSpIdMissing_SetsGrantedFalse_NoGrantCalls() { - var (ctx, graph, blueprintService) = BuildS2SGrantTestContext(); - - graph.EnsureServicePrincipalForAppIdAsync( - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any()) - .Returns((string?)null); + var (ctx, _, blueprintService) = BuildS2SGrantTestContext(); + ctx.Config.AgenticAppId = null; await NonDwBlueprintSetupOrchestrator.GrantOrInstructAgentIdentityAppPermissionsAsync(ctx, OneS2SSpec()); ctx.Results.S2SAppRoleGranted.Should().Be(false, - because: "when the agent identity SP cannot be resolved, grants cannot proceed and the result must be false"); + because: "when AgenticAppId (the SP object ID) is absent, grants cannot proceed and the result must be false"); await blueprintService.DidNotReceive().GrantAppRoleAssignmentAsync( Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any>(), Arg.Any?>(), Arg.Any());