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 eea6c5f9..3b719608 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,13 @@ 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 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. + - `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`. @@ -77,6 +84,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 @@ -97,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/AllSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs index b2e24859..590c13b9 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 — AI Teammate agents automatically use OBO via agent user identity."); + context.ExitCode = 1; + return; + } + // Generate correlation ID at workflow entry point var correlationId = HttpClientFactory.GenerateCorrelationId(); logger.LogDebug("Starting setup all (CorrelationId: {CorrelationId})", correlationId); @@ -176,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", @@ -257,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) @@ -267,7 +301,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 +337,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/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/NonDwBlueprintSetupOrchestrator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs index ab19e1f2..98283f7f 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,26 +96,28 @@ 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 — only applicable to AI Teammate (DW) agents; always skipped here. + 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. Permission Grants - var blueprintIdForCmd = config.AgentBlueprintId ?? ""; - logger.LogInformation(SetupHelpers.DryRunRow(4, "Permission Grants") + "admin approval required — a365 setup admin --blueprint-id {BlueprintId}", blueprintIdForCmd); - - // 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)) @@ -134,7 +136,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 @@ -274,18 +276,20 @@ 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 (permissions set directly on agent identity)."); - // 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 +400,18 @@ 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); + 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); + + // 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 +638,87 @@ 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; + } + + // 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("Agent identity SP object ID is missing. App role assignments must be granted manually."); + 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', 'Directory.Read.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..ff0fcefd 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` only. + +| 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/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 c573d915..2c29ab06 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 = 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 10ad017d..7dc3df99 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, @@ -275,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) @@ -322,11 +402,16 @@ 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); + { + if (!string.IsNullOrWhiteSpace(agentIdentityDisplayName)) + logger.LogInformation(" $ai = Get-MgServicePrincipal -ServicePrincipalId '{AgentIdentitySpObjectId}' # {DisplayName}", agentIdentitySpObjectId, agentIdentityDisplayName); + else + 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(); @@ -334,11 +419,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); } } @@ -421,8 +503,22 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo logger.LogInformation("Setup Summary"); logger.LogInformation(""); - var pendingAdminAction = !results.AdminConsentGranted && results.BatchPermissionsPhase2Completed; - var pendingS2SAction = results.S2SAppRoleGranted == false; + // 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 + ? s2sOk && (!isBothMode || delegatedOk) + : delegatedOk; + 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. @@ -460,35 +556,46 @@ 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) + // 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 && 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(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); @@ -512,7 +619,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( @@ -591,7 +698,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) @@ -988,7 +1095,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/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/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/Models/Agent365Config.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/Agent365Config.cs index 456d62d5..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 @@ -150,6 +165,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 +688,7 @@ public Agent365Config WithCustomBlueprintPermissions(List> 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 55b93b0b..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. @@ -628,7 +627,56 @@ 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); + 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); + 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, @@ -771,6 +819,15 @@ private async Task 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..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 @@ -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("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] - 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("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/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs index 6db90a23..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 @@ -785,4 +785,168 @@ 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 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_AgentSpIdMissing_SetsGrantedFalse_NoGrantCalls() + { + var (ctx, _, blueprintService) = BuildS2SGrantTestContext(); + ctx.Config.AgenticAppId = null; + + await NonDwBlueprintSetupOrchestrator.GrantOrInstructAgentIdentityAppPermissionsAsync(ctx, OneS2SSpec()); + + ctx.Results.S2SAppRoleGranted.Should().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()); + } + + /// + /// 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"); + } } 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..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 @@ -539,4 +539,108 @@ 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"); + _mockLogger.Received().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("not supported with --aiteammate")), + 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 (step 5) and must not show S2S app permission grants. + /// + [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("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>()); + } } 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/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/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)), 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