diff --git a/CHANGELOG.md b/CHANGELOG.md index 116c145a..5dca78c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,8 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g - `setup requirements` now detects and auto-repairs the **`wids` optional claim** on the CLI client app's access tokens. Without this claim, the CLI cannot read the signed-in user's directory roles from the token and silently skips the tenant-wide consent step on the agent blueprint — the user-visible symptom is a blueprint with `inheritablePermissions.kind=allAllowed` but no permissions actually granted on the blueprint service principal. When the running user has admin authority over the app registration, the CLI patches `optionalClaims.accessToken` to include `wids` and clears the local MSAL token cache so the next acquisition carries the new claim. The well-known CLI app is also now created with `wids` already configured. Non-admin users get explicit Azure portal click-path remediation. - `setup requirements` now auto-provisions the `Application.Read.All` consented permission when it is declared on the CLI client app but missing from the tenant's OAuth2 consent grant. Required by the wids check (which queries `/v1.0/applications`) and the existing redirect-URI / public-client validators. - `--secret-lifetime-months ` option (and matching `secretLifetimeMonths` field in the `--input-file` JSON) on `develop-mcp register-external-mcp-server` — controls the lifetime of the client secrets created on the A365Proxy and RemoteProxy Entra apps. Valid range `1-24`; omit to use the Graph default (~2 years). Calendar-aware (uses `DateTimeOffset.AddMonths`, so Jan 31 + 1 month → Feb 28/29). Added so tenants with an `appManagementPolicies` cap on client-secret lifetime — previously a hard failure inside `CreateEntraAppsAsync` with a generic "Failed to create secret" message — can fit registration inside their tenant's policy. When Graph rejects the requested (or default) lifetime with a tenant-policy error, the CLI now emits an actionable error naming the flag and the attempted value (e.g. `Tenant Entra ID policy rejected the requested 12-month lifetime ... Pass --secret-lifetime-months N with a smaller value (e.g. --secret-lifetime-months 3) that fits inside your tenant's appManagementPolicies cap.`) instead of the previous generic failure. +- `--publisher-name` / `-p` option on `develop-mcp publish` — sets the publisher name written into the published MCP server's package metadata. Required for custom (user-created) MCP servers; ignored for 1p Microsoft-owned servers (e.g. `msdyn_DataverseMCPServer`), which always publish as "Microsoft". Prompted interactively when omitted. +- `--yes` / `-y` option on `develop-mcp publish` — skips the interactive "Proceed with publish? (y/N)" confirmation. ### Fixed - `setup all` now exits silently on Ctrl+C instead of printing `ERROR: Setup failed: A task was canceled.` followed by a misleading partial summary. @@ -112,6 +114,9 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g - **`a365 config display` removed** — use `a365 query-entra blueprint-scopes` to inspect live blueprint permissions and consent state. - **`a365 config permissions` removed** — replace with `a365 setup permissions custom --resource-app-id --scopes `. - **`--config`/`-c` option removed from all commands** — config file is now always resolved from the current directory (`a365.config.json`). Scripts passing `--config ` will receive a parse error; change directory before running the CLI instead. +- **`--tenant-id` / `-t` removed from `a365 develop-mcp register-external-mcp-server`** — the tenant is now auto-detected from the current `az login` session. Scripts passing `-t ` / `--tenant-id ` will receive a System.CommandLine parse error; run `az login --tenant ` (or `az account set --subscription `) to target a specific tenant instead. +- **`a365 develop-mcp publish` now creates a `-PublicClients` Entra app registration in your tenant** — the publish orchestration runs CLI-side, so after each publish you will see a new app registration named `-PublicClients` in your tenant's Entra ID. These are created by the CLI; clean them up with the same name if you unpublish. +- **`a365 develop-mcp publish` now requires the `Application.ReadWrite.All` Microsoft Graph permission** — needed to create the Entra app registration above. Running publish with only read-only Graph permissions will fail. Grant `Application.ReadWrite.All` to the account (or app) running the CLI before publishing. - **`--agent-instance-only` renamed to `--agent-registration-only`** on `a365 setup all` — update any scripts using the old flag name. - **`setup permissions custom --resource-app-id --scopes` applies permissions directly to Entra ID** — unlike the former `a365 config permissions` which only wrote to `a365.config.json`, this inline mode immediately mutates the live blueprint in Entra and cannot be undone by editing a config file. - `a365 setup` now writes the `Agent365Observability` placeholder section (`AgentId`, `AgentBlueprintId`, `TenantId`, `AgentName`, `AgentDescription`) and `EnableAgent365Exporter: false` to `appsettings.json` (.NET) and `ENABLE_A365_OBSERVABILITY_EXPORTER=false` to `.env` (Python/Node.js), so observability configuration is pre-populated for all three platforms after running setup diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs index f7a0d892..1f05a65f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -35,7 +35,7 @@ public static Command CreateCommand( // Add subcommands developMcpCommand.AddCommand(CreateListEnvironmentsSubcommand(logger, toolingService)); developMcpCommand.AddCommand(CreateListServersSubcommand(logger, toolingService)); - developMcpCommand.AddCommand(CreatePublishSubcommand(logger, toolingService)); + developMcpCommand.AddCommand(CreatePublishSubcommand(logger, toolingService, graphApiService)); developMcpCommand.AddCommand(CreateUnpublishSubcommand(logger, toolingService)); developMcpCommand.AddCommand(CreateApproveSubcommand(logger, toolingService)); developMcpCommand.AddCommand(CreateBlockSubcommand(logger, toolingService)); @@ -279,180 +279,68 @@ private static Command CreateListServersSubcommand( /// Creates the publish subcommand /// private static Command CreatePublishSubcommand( - ILogger logger, - IAgent365ToolingService toolingService) + ILogger logger, + IAgent365ToolingService toolingService, + GraphApiService? graphApiService) { - var command = new Command("publish", "Publish an MCP server to a Dataverse environment"); + var command = new Command("publish", "Publish an MCP server to a Dataverse environment."); var envIdOption = new Option( ["--environment-id", "-e"], - description: "Dataverse environment ID" - ); + description: "Dataverse environment ID"); envIdOption.IsRequired = false; // Allow null so we can prompt command.AddOption(envIdOption); var serverNameOption = new Option( ["--server-name", "-s"], - description: "MCP server name to publish" - ); - serverNameOption.IsRequired = false; // Allow null so we can prompt + description: "MCP server name to publish"); + serverNameOption.IsRequired = false; command.AddOption(serverNameOption); var aliasOption = new Option( ["--alias", "-a"], - description: "Alias for the MCP server" - ); + description: "Alias for the MCP server"); command.AddOption(aliasOption); var displayNameOption = new Option( ["--display-name", "-d"], - description: "Display name for the MCP server" - ); + description: "Display name for the MCP server (max 30 chars)"); command.AddOption(displayNameOption); - var dryRunOption = new Option( - name: "--dry-run", - description: "Show what would be done without executing" - ); - command.AddOption(dryRunOption); - - var verboseOption = new Option( - ["--verbose", "-v"], - description: "Enable verbose logging" - ); - command.AddOption(verboseOption); - - command.SetHandler(async (envId, serverName, alias, displayName, dryRun, verbose) => - { - _ = verbose; - try - { - // Validate and prompt for missing required arguments with security checks - if (string.IsNullOrWhiteSpace(envId)) - { - envId = InputValidator.PromptAndValidateRequiredInput("Enter Dataverse environment ID: ", "Environment ID"); - if (string.IsNullOrWhiteSpace(envId)) - { - logger.LogError("Environment ID is required"); - return; - } - } - else - { - // Validate provided environment ID - envId = InputValidator.ValidateInput(envId, "Environment ID"); - if (envId == null) - { - logger.LogError("Invalid environment ID format"); - return; - } - } - - if (string.IsNullOrWhiteSpace(serverName)) - { - serverName = InputValidator.PromptAndValidateRequiredInput("Enter MCP server name to publish: ", "Server name", 100); - if (string.IsNullOrWhiteSpace(serverName)) - { - logger.LogError("Server name is required"); - return; - } - } - else - { - // Validate provided server name - serverName = InputValidator.ValidateInput(serverName, "Server name"); - if (serverName == null) - { - logger.LogError("Invalid server name format"); - return; - } - } - - logger.LogInformation("Starting publish operation for server {ServerName} in environment {EnvId}...", serverName, envId); + var publisherNameOption = new Option( + ["--publisher-name", "-p"], + description: "Publisher name for the MCP Server. Required for custom (user-created) MCP servers; ignored for 1p Microsoft-owned servers (e.g. msdyn_DataverseMCPServer) which always publish as 'Microsoft'."); + command.AddOption(publisherNameOption); - if (dryRun) - { - logger.LogInformation("[DRY RUN] Would read config from a365.config.json"); - logger.LogInformation("[DRY RUN] Would publish MCP server {ServerName} to environment {EnvId}", serverName, envId); - logger.LogInformation("[DRY RUN] Alias: {Alias}", alias ?? "[would prompt]"); - logger.LogInformation("[DRY RUN] Display Name: {DisplayName}", displayName ?? "[would prompt]"); - await Task.CompletedTask; - return; - } - - // Validate and prompt for missing optional values with security checks - if (string.IsNullOrWhiteSpace(alias)) - { - alias = InputValidator.PromptAndValidateRequiredInput("Enter alias for the MCP server: ", "Alias", 50); - if (string.IsNullOrWhiteSpace(alias)) - { - logger.LogError("Alias is required"); - return; - } - } - else - { - // Validate provided alias - alias = InputValidator.ValidateInput(alias, "Alias", maxLength: 50); - if (alias == null) - { - logger.LogError("Invalid alias format"); - return; - } - } + var yesOption = new Option( + ["--yes", "-y"], + description: "Skip the interactive 'Proceed with publish? (y/N)' confirmation."); + command.AddOption(yesOption); - if (string.IsNullOrWhiteSpace(displayName)) - { - displayName = InputValidator.PromptAndValidateRequiredInput("Enter display name for the MCP server: ", "Display name", 100); - if (string.IsNullOrWhiteSpace(displayName)) - { - logger.LogError("Display name is required"); - return; - } - } - else - { - // Validate provided display name - displayName = InputValidator.ValidateInput(displayName, "Display name", maxLength: 100); - if (displayName == null) - { - logger.LogError("Invalid display name format"); - return; - } - } - } - catch (ArgumentException ex) - { - logger.LogError("Input validation failed: {Message}", ex.Message); - return; - } + var dryRunOption = new Option("--dry-run", "Show what would be done without executing"); + command.AddOption(dryRunOption); - // Create request - var request = new PublishMcpServerRequest - { - Alias = alias, - DisplayName = displayName - }; + // Verbose is handled globally in Program.cs (sets LogLevel.Debug); declared here so the parser accepts -v. + command.AddOption(new Option(["--verbose", "-v"], description: "Enable verbose logging")); - // Call service - var response = await toolingService.PublishServerAsync(envId, serverName, request); + command.SetHandler(async (context) => + { + var args = new RawPublishArgs( + EnvironmentId: context.ParseResult.GetValueForOption(envIdOption), + ServerName: context.ParseResult.GetValueForOption(serverNameOption), + Alias: context.ParseResult.GetValueForOption(aliasOption), + DisplayName: context.ParseResult.GetValueForOption(displayNameOption), + PublisherName: context.ParseResult.GetValueForOption(publisherNameOption), + Yes: context.ParseResult.GetValueForOption(yesOption), + DryRun: context.ParseResult.GetValueForOption(dryRunOption)); - if (response == null || !response.IsSuccess) + var executor = new PublishCommandExecutor(logger, toolingService, graphApiService); + var success = await executor.ExecuteAsync(args, context.GetCancellationToken()); + if (!success) { - if (response?.Message != null) - { - logger.LogError("Failed to publish MCP server {ServerName} to environment {EnvId}: {ErrorMessage}", serverName, envId, response.Message); - } - else - { - logger.LogError("Failed to publish MCP server {ServerName} to environment {EnvId}: No response received", serverName, envId); - } - return; + context.ExitCode = 1; } - - logger.LogInformation("Successfully published MCP server {ServerName} to environment {EnvId}", serverName, envId); - - }, envIdOption, serverNameOption, aliasOption, displayNameOption, dryRunOption, verboseOption); + }); return command; } @@ -844,9 +732,6 @@ private static Command CreateRegisterExternalMcpServerSubcommand( var remoteScopesOption = new Option("--remote-scopes", description: "Scopes for the remote MCP server (e.g., 'api://{appId-guid}/{scopeName}' such as 'api://00000000-0000-0000-0000-000000000000/access_as_user')"); command.AddOption(remoteScopesOption); - var tenantIdOption = new Option(["--tenant-id", "-t"], description: "Entra tenant ID for app registration (defaults to current az login tenant)"); - command.AddOption(tenantIdOption); - var serviceTreeIdOption = new Option("--service-tree-id", description: "ServiceTree ID for Entra app registration (required in Microsoft corporate tenants)"); command.AddOption(serviceTreeIdOption); @@ -881,7 +766,7 @@ private static Command CreateRegisterExternalMcpServerSubcommand( ToolsInput: context.ParseResult.GetValueForOption(toolsOption), InputFile: context.ParseResult.GetValueForOption(inputFileOption), RemoteScopes: context.ParseResult.GetValueForOption(remoteScopesOption), - TenantId: context.ParseResult.GetValueForOption(tenantIdOption), + TenantId: null, ServiceTreeId: context.ParseResult.GetValueForOption(serviceTreeIdOption), SecretLifetimeMonths: context.ParseResult.GetValueForOption(secretLifetimeMonthsOption), PublisherName: context.ParseResult.GetValueForOption(publisherOption), diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs new file mode 100644 index 00000000..407fc31b --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs @@ -0,0 +1,499 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Helpers; +using Microsoft.Agents.A365.DevTools.Cli.Models; +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; +using Microsoft.Extensions.Logging; + +namespace Microsoft.Agents.A365.DevTools.Cli.Commands; + +/// +/// Raw CLI arguments passed to the develop-mcp publish command. +/// +internal record RawPublishArgs( + string? EnvironmentId, + string? ServerName, + string? Alias, + string? DisplayName, + string? PublisherName, + bool Yes, + bool DryRun); + +/// +/// Orchestrates first-party MCP server publish in one CLI command. The shape mirrors +/// for BYO register: the CLI creates the Public Clients +/// Entra app in the user's own tenant, calls the platform publish endpoint, and back-fills the +/// PPMI scope grant on it after the platform resolves the underlying server's PPMI identity. +/// +internal class PublishCommandExecutor +{ + // protected (instead of private) on the seam methods + non-sealed class so tests can stub + // out the parts that hit external systems (Azure CLI for tenant detection). The class is + // still internal — overrides only happen in the test assembly via InternalsVisibleTo. + private readonly ILogger _logger; + private readonly IAgent365ToolingService _toolingService; + private readonly GraphApiService? _graphApiService; + private readonly RetryHelper _retryHelper; + + internal PublishCommandExecutor( + ILogger logger, + IAgent365ToolingService toolingService, + GraphApiService? graphApiService) + { + _logger = logger; + _toolingService = toolingService; + _graphApiService = graphApiService; + _retryHelper = new RetryHelper(logger, maxRetries: 5, baseDelaySeconds: 3); + } + + private sealed record ResolvedInput + { + public required string EnvironmentId { get; init; } + public required string ServerName { get; init; } + public required string Alias { get; init; } + public required string DisplayName { get; init; } + public required bool DryRun { get; init; } + + // Null when the caller didn't supply --publisher-name and didn't enter one at the prompt. + // The platform's v2 publish validator rejects null/empty for custom (user-created) servers + // and ignores the value for 1p app-based servers (auto-fills "Microsoft"). The CLI can't + // classify ahead of time without knowing the server's mapping, so it leaves the value + // null when unspecified and lets the platform decide. + public string? PublisherName { get; init; } + + // When true, skip the interactive "Proceed with publish? (y/N)" confirmation. Set via + // --yes / -y. Required for non-interactive contexts (CI scripts, automation). + public required bool Yes { get; init; } + } + + internal sealed record EntraAppSet( + string? PublicClientsClientId, + string? PublicClientsObjectId, + string PublicClientsAppName); + + internal async Task ExecuteAsync(RawPublishArgs args, CancellationToken ct = default) + { + var input = ResolveInputs(args); + if (input is null) return false; + + DisplayPublishSummary(input); + + if (input.DryRun) + { + _logger.LogInformation("[DRY RUN] Would create Entra app '{PublicClients}' in tenant", $"{input.ServerName}-PublicClients"); + _logger.LogInformation("[DRY RUN] Would call publish endpoint and back-fill PPMI scope on the created app"); + return true; + } + + if (!input.Yes) + { + Console.Write("Proceed with publish? (y/N): "); + var confirmation = Console.ReadLine()?.Trim().ToLowerInvariant(); + if (confirmation != "y" && confirmation != "yes") + { + Console.WriteLine("Publish cancelled."); + // User cancellation is not a failure — exit 0. Matches the register command's same + // prompt-cancel path. + return true; + } + } + else + { + _logger.LogDebug("Skipping interactive confirmation (--yes was supplied)."); + } + + Console.WriteLine(); + ct.ThrowIfCancellationRequested(); + Console.WriteLine($"Publishing MCP server '{input.ServerName}' as '{input.Alias}' to environment {input.EnvironmentId}..."); + + var tenantId = await DetectTenantIdAsync(); + if (tenantId is null) return false; + + if (_graphApiService is null) + { + _logger.LogError("Graph API service is not available. Cannot create Entra applications."); + return false; + } + + var warnings = new List(); + var apps = await CreateEntraAppsAsync(input, tenantId, warnings, ct); + if (apps is null) return false; + + ct.ThrowIfCancellationRequested(); + + var request = new PublishMcpServerRequest + { + Alias = input.Alias, + DisplayName = input.DisplayName, + PublicClientsAppId = apps.PublicClientsClientId, + PublisherName = input.PublisherName, + }; + + PublishMcpServerResponse? publishResponse; + try + { + // Hits the platform's v2 publish endpoint via the tooling service, which performs the + // full elevation orchestration (PPMI provisioning, MOS upload). The platform's v1 + // endpoint remains for older CLI binaries. + publishResponse = await _toolingService.PublishServerAsync(input.EnvironmentId, input.ServerName, request, ct); + } + catch (Exception ex) + { + // Caller cancellation (Ctrl+C) should abort fast and predictably rather than be reported + // as a publish failure and trigger rollback work. Rethrow so the process exits quickly, + // matching the cancellation handling in the setup flows. + if (ex is OperationCanceledException && ct.IsCancellationRequested) + { + throw; + } + + _logger.LogError("Failed to publish MCP server '{ServerName}': {Error}", input.ServerName, ex.Message); + _logger.LogDebug("Exception details: {Exception}", ex.ToString()); + await RollbackEntraAppsAsync(apps, tenantId, ct); + return false; + } + + if (publishResponse is null || !publishResponse.IsSuccess) + { + var errorMsg = publishResponse?.Message ?? "No response received"; + _logger.LogError("Failed to publish MCP server {ServerName}: {Error}", input.ServerName, errorMsg); + await RollbackEntraAppsAsync(apps, tenantId, ct); + return false; + } + + _logger.LogDebug("Successfully published MCP server {ServerName}", input.ServerName); + + await ConfigureEntraAppsAsync(input, apps, publishResponse, tenantId, warnings, ct); + + DisplayResults(input, warnings); + return true; + } + + private ResolvedInput? ResolveInputs(RawPublishArgs args) + { + // Dry-run skips interactive prompts so the command stays scriptable / CI-friendly. + // Missing required values get a clearly-labeled placeholder for summary purposes; the + // executor short-circuits before any platform call, so the placeholders never leave + // this process. User-supplied values still go through normal validation. + const string DryRunPlaceholder = "(unspecified)"; + + try + { + var environmentId = args.EnvironmentId; + if (string.IsNullOrWhiteSpace(environmentId)) + { + environmentId = args.DryRun + ? DryRunPlaceholder + : DevelopMcpCommand.InputValidator.PromptAndValidateRequiredInput("Enter Dataverse environment ID: ", "Environment ID"); + if (string.IsNullOrWhiteSpace(environmentId)) { _logger.LogError("Environment ID is required"); return null; } + } + else + { + environmentId = DevelopMcpCommand.InputValidator.ValidateInput(environmentId, "Environment ID"); + if (environmentId == null) { _logger.LogError("Invalid environment ID format"); return null; } + } + + var serverName = args.ServerName; + if (string.IsNullOrWhiteSpace(serverName)) + { + serverName = args.DryRun + ? DryRunPlaceholder + : DevelopMcpCommand.InputValidator.PromptAndValidateRequiredInput("Enter MCP server name to publish: ", "Server name", 100); + if (string.IsNullOrWhiteSpace(serverName)) { _logger.LogError("Server name is required"); return null; } + } + else + { + serverName = DevelopMcpCommand.InputValidator.ValidateInput(serverName, "Server name"); + if (serverName == null) { _logger.LogError("Invalid server name format"); return null; } + } + + var alias = args.Alias; + if (string.IsNullOrWhiteSpace(alias)) + { + alias = args.DryRun + ? DryRunPlaceholder + : DevelopMcpCommand.InputValidator.PromptAndValidateRequiredInput("Enter alias for the MCP server: ", "Alias", 50); + if (string.IsNullOrWhiteSpace(alias)) { _logger.LogError("Alias is required"); return null; } + } + else + { + alias = DevelopMcpCommand.InputValidator.ValidateInput(alias, "Alias", maxLength: 50); + if (alias == null) { _logger.LogError("Invalid alias format"); return null; } + } + + var displayName = args.DisplayName; + if (string.IsNullOrWhiteSpace(displayName)) + { + displayName = args.DryRun + ? DryRunPlaceholder + : DevelopMcpCommand.InputValidator.PromptAndValidateRequiredInput("Enter display name for the MCP server: ", "Display name", 30); + if (string.IsNullOrWhiteSpace(displayName)) { _logger.LogError("Display name is required"); return null; } + } + else + { + displayName = DevelopMcpCommand.InputValidator.ValidateInput(displayName, "Display name", maxLength: 30); + if (displayName == null) { _logger.LogError("Invalid display name format"); return null; } + } + + // Publisher name: optional from the CLI's perspective. The platform's v2 validator + // requires a non-empty value for custom (user-created) servers and ignores it for + // 1p app-based servers. Prompt only when the option was omitted entirely (null) — a + // Microsoft developer publishing msdyn_DataverseMCPServer can just press Enter. An + // explicitly empty/whitespace value (e.g. --publisher-name "" from a script) is treated + // as "no publisher" without prompting, so non-interactive automation never hangs. If a + // custom server ends up with no value, the platform's error message says what's missing. + // Dry-run also skips the prompt. + var publisherName = args.PublisherName; + if (publisherName is null) + { + publisherName = args.DryRun + ? null + : DevelopMcpCommand.InputValidator.PromptAndValidateOptionalInput( + "Enter publisher name (optional for 1p Microsoft-owned servers, required otherwise): ", + "Publisher name", + maxLength: 100); + } + else if (string.IsNullOrWhiteSpace(publisherName)) + { + publisherName = null; + } + else + { + publisherName = DevelopMcpCommand.InputValidator.ValidateInput(publisherName, "Publisher name", maxLength: 100); + if (publisherName == null) { _logger.LogError("Invalid publisher name format"); return null; } + } + + return new ResolvedInput + { + EnvironmentId = environmentId, + ServerName = serverName, + Alias = alias, + DisplayName = displayName, + PublisherName = string.IsNullOrWhiteSpace(publisherName) ? null : publisherName, + Yes = args.Yes, + DryRun = args.DryRun, + }; + } + catch (ArgumentException ex) + { + _logger.LogError("Input validation failed: {Message}", ex.Message); + return null; + } + } + + private void DisplayPublishSummary(ResolvedInput input) + { + Console.WriteLine(); + var prevColor = Console.ForegroundColor; + Console.ForegroundColor = ConsoleColor.Cyan; + Console.WriteLine("Publish Summary"); + Console.WriteLine("==============="); + Console.ForegroundColor = prevColor; + DevelopMcpCommand.WriteLabel(" Environment: "); Console.WriteLine(input.EnvironmentId); + DevelopMcpCommand.WriteLabel(" Server Name: "); Console.WriteLine(input.ServerName); + DevelopMcpCommand.WriteLabel(" Alias: "); Console.WriteLine(input.Alias); + DevelopMcpCommand.WriteLabel(" Display Name: "); Console.WriteLine(input.DisplayName); + DevelopMcpCommand.WriteLabel(" Publisher: "); Console.WriteLine(input.PublisherName ?? "(none — platform will reject if this is a custom server)"); + Console.WriteLine(); + } + + protected virtual async Task DetectTenantIdAsync() + { + var tenantId = await TenantDetectionHelper.DetectTenantIdAsync(null, _logger); + + if (string.IsNullOrWhiteSpace(tenantId)) + { + _logger.LogError("Tenant ID could not be determined. Run 'az login' and try again."); + return null; + } + + return tenantId; + } + + private async Task CreateEntraAppsAsync(ResolvedInput input, string tenantId, List warnings, CancellationToken ct = default) + { + var provisioner = new EntraAppProvisioner(_logger, _graphApiService!, _retryHelper); + + var publicClients = await provisioner.CreatePublicClientsAppAsync( + input.ServerName, tenantId, serviceTreeId: null, warnings, ct); + + return new EntraAppSet( + PublicClientsClientId: publicClients.ClientId, + PublicClientsObjectId: publicClients.ObjectId, + PublicClientsAppName: publicClients.AppName); + } + + // Best-effort compensating delete for the Entra apps created in CreateEntraAppsAsync, run when + // the platform publish call fails after app creation. Each delete is wrapped independently so + // a failure on the first app doesn't skip the second. Failures are logged with both clientId + // and objectId so the user can clean up manually. + internal async Task RollbackEntraAppsAsync(EntraAppSet apps, string tenantId, CancellationToken ct = default) + { + if (_graphApiService is null) + { + _logger.LogWarning("Graph API service is unavailable; cannot roll back Entra app '{PublicClients}'. Delete it manually in the Azure portal.", apps.PublicClientsAppName); + return; + } + + _logger.LogInformation("Rolling back Entra app registrations created for failed publish..."); + + if (!string.IsNullOrWhiteSpace(apps.PublicClientsObjectId)) + { + await DeleteOneAsync(apps.PublicClientsObjectId, apps.PublicClientsClientId, apps.PublicClientsAppName, ct); + } + + async Task DeleteOneAsync(string objectId, string? clientId, string appName, CancellationToken cancellationToken) + { + try + { + var deleted = await _graphApiService!.DeleteEntraAppAsync(tenantId, objectId, cancellationToken); + if (deleted) + { + _logger.LogInformation("Rolled back Entra app '{AppName}' (objectId {ObjectId})", appName, objectId); + } + else + { + _logger.LogError( + "Failed to roll back Entra app '{AppName}' (clientId {ClientId}, objectId {ObjectId}). Delete it manually in the Azure portal.", + appName, clientId ?? "", objectId); + } + } + catch (Exception ex) + { + _logger.LogError( + ex, + "Exception rolling back Entra app '{AppName}' (clientId {ClientId}, objectId {ObjectId}). Delete it manually in the Azure portal.", + appName, clientId ?? "", objectId); + } + } + } + + private async Task ConfigureEntraAppsAsync( + ResolvedInput input, + EntraAppSet apps, + PublishMcpServerResponse response, + string tenantId, + List warnings, + CancellationToken ct = default) + { + var tasks = new List(); + var concurrentWarnings = new System.Collections.Concurrent.ConcurrentBag(); + + // Grant required-resource-access on the just-created Public Clients Entra app. + // The platform resolves the right resource per server type (Custom: managedidentityid; app-based + // / Dataverse MCP: 1p mappings; fallback: platform's own app id) and returns both the resource + // app id and the scope name. We look up the scope guid on the resource app, then add it as + // requiredResourceAccess on the Entra app we created. + var resourceAppId = response.McpServerAppId; + var resourceScopeName = response.McpServerScope; + Guid? resourceScopeId = null; + if (!string.IsNullOrWhiteSpace(resourceAppId) && !string.IsNullOrWhiteSpace(resourceScopeName)) + { + _logger.LogDebug("Resolving scope '{ScopeName}' on underlying server app {AppId}", resourceScopeName, resourceAppId); + try + { + resourceScopeId = await _retryHelper.ExecuteWithRetryAsync( + async retryCt => await _graphApiService!.GetOAuth2PermissionScopeIdAsync( + tenantId, resourceAppId, resourceScopeName, retryCt), + result => !result.HasValue, + cancellationToken: ct); + } + catch (Exception ex) + { + var msg = $"Could not find '{resourceScopeName}' scope on app {resourceAppId} after retries: {ex.Message}. API permissions not added."; + _logger.LogError(msg); + concurrentWarnings.Add(msg); + } + } + else + { + var msg = $"Underlying server app id or scope was not returned by publish (appId='{resourceAppId}', scope='{resourceScopeName}'). API permissions not added."; + _logger.LogWarning(msg); + concurrentWarnings.Add(msg); + } + + if (resourceScopeId.HasValue) + { + if (apps.PublicClientsObjectId != null) + { + tasks.Add(AddRequiredResourceAccessAsync(tenantId, apps.PublicClientsObjectId, apps.PublicClientsAppName, resourceAppId!, resourceScopeId.Value, concurrentWarnings, ct)); + } + } + else if (!string.IsNullOrWhiteSpace(resourceAppId) && !string.IsNullOrWhiteSpace(resourceScopeName)) + { + var msg = $"Could not find '{resourceScopeName}' scope on app {resourceAppId}. API permissions not added."; + _logger.LogError(msg); + concurrentWarnings.Add(msg); + } + + await Task.WhenAll(tasks); + + foreach (var w in concurrentWarnings) + warnings.Add(w); + } + + private async Task AddRequiredResourceAccessAsync( + string tenantId, + string appObjectId, + string appName, + string resourceAppId, + Guid resourceScopeId, + System.Collections.Concurrent.ConcurrentBag concurrentWarnings, + CancellationToken ct = default) + { + try + { + _logger.LogDebug("Adding required-resource-access for resource {ResourceAppId} on '{AppName}' ({ObjectId})", resourceAppId, appName, appObjectId); + var success = await _retryHelper.ExecuteWithRetryAsync( + async retryCt => await _graphApiService!.AddRequiredResourceAccessAsync( + tenantId, appObjectId, resourceAppId, resourceScopeId, retryCt), + result => !result, + cancellationToken: ct); + if (!success) + { + var msg = $"Failed to add required-resource-access on '{appName}' after retries."; + _logger.LogError(msg); + concurrentWarnings.Add(msg); + } + else + { + _logger.LogInformation("Added API permission on '{AppName}'", appName); + } + } + catch (Exception ex) + { + var msg = $"Failed to add required-resource-access on '{appName}': {ex.Message}"; + _logger.LogError(msg); + concurrentWarnings.Add(msg); + } + } + + private void DisplayResults(ResolvedInput input, List warnings) + { + Console.WriteLine(); + if (warnings.Count == 0) + { + var prevColor = Console.ForegroundColor; + Console.ForegroundColor = ConsoleColor.Green; + Console.WriteLine($"MCP server '{input.ServerName}' published as '{input.Alias}' to environment {input.EnvironmentId}."); + Console.ForegroundColor = prevColor; + } + else + { + var prevColor = Console.ForegroundColor; + Console.ForegroundColor = ConsoleColor.Yellow; + Console.WriteLine($"MCP server '{input.ServerName}' was published with {warnings.Count} warning(s):"); + Console.ForegroundColor = prevColor; + Console.WriteLine(); + foreach (var w in warnings) + { + _logger.LogWarning(" - {Warning}", w); + } + } + + Console.WriteLine(); + Console.WriteLine($"Please ask your tenant admin to approve MCP server '{input.ServerName}' in the Microsoft 365 Admin Center."); + } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs index 20f1c01b..622e23c0 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs @@ -582,127 +582,46 @@ private void DisplayRegistrationSummary(ResolvedInput input) private async Task CreateEntraAppsAsync( ResolvedInput input, string tenantId, List warnings) { - var a365AppName = $"{input.ServerName}-A365Proxy"; - var remoteProxyAppName = $"{input.ServerName}-RemoteProxy"; - var publicClientsAppName = $"{input.ServerName}-PublicClients"; - - _logger.LogDebug("Creating Entra application for A365 Proxy..."); - var a365App = await _graphApiService!.CreateEntraAppAsync(tenantId, a365AppName, serviceTreeId: input.ServiceTreeId); - if (a365App == null) - { - _logger.LogError("Failed to create Entra application '{AppName}'. Ensure you have Application.ReadWrite.All permission in the target tenant. Run with -v for details.", a365AppName); - return null; - } - _logger.LogInformation("Created Entra app '{AppName}' (clientId: {ClientId})", a365AppName, a365App.Value.ClientId); - - var a365Secret = await _graphApiService.AddAppPasswordAsync(tenantId, a365App.Value.ObjectId, lifetimeMonths: input.SecretLifetimeMonths); - if (string.IsNullOrWhiteSpace(a365Secret)) - { - _logger.LogError("Failed to create secret for '{AppName}'. Run with -v for details.", a365AppName); - return null; - } + var provisioner = new EntraAppProvisioner(_logger, _graphApiService!, _retryHelper); - if (string.IsNullOrWhiteSpace(a365App.Value.ClientId)) - { - _logger.LogError("A365 Proxy Entra application was created but returned an empty client ID"); - return null; - } - - _logger.LogDebug("Created A365 Proxy app: {ClientId}", a365App.Value.ClientId); + var a365ProxyApp = await provisioner.CreateProxyAppAsync( + input.ServerName, tenantId, suffix: "A365Proxy", roleDisplay: "A365 Proxy", + serviceTreeId: input.ServiceTreeId, lifetimeMonths: input.SecretLifetimeMonths); + if (a365ProxyApp == null) return null; string? remoteProxyClientId = null; string? remoteProxySecret = null; string? remoteProxyObjectId = null; + var remoteProxyAppName = $"{input.ServerName}-RemoteProxy"; if (input.IsEntra) { - _logger.LogDebug("Creating Entra application for Remote Proxy..."); - var remoteApp = await _graphApiService.CreateEntraAppAsync(tenantId, remoteProxyAppName, serviceTreeId: input.ServiceTreeId); - if (remoteApp == null) - { - _logger.LogError("Failed to create Entra application '{AppName}'. Ensure you have Application.ReadWrite.All permission in the target tenant. Run with -v for details.", remoteProxyAppName); - return null; - } - _logger.LogInformation("Created Entra app '{AppName}' (clientId: {ClientId})", remoteProxyAppName, remoteApp.Value.ClientId); - - remoteProxySecret = await _graphApiService.AddAppPasswordAsync(tenantId, remoteApp.Value.ObjectId, lifetimeMonths: input.SecretLifetimeMonths); - if (string.IsNullOrWhiteSpace(remoteProxySecret)) - { - _logger.LogError("Failed to create secret for '{AppName}'. Run with -v for details.", remoteProxyAppName); - return null; - } - - if (string.IsNullOrWhiteSpace(remoteApp.Value.ClientId)) - { - _logger.LogError("Remote Proxy Entra application was created but returned an empty client ID"); - return null; - } + var remoteProxyApp = await provisioner.CreateProxyAppAsync( + input.ServerName, tenantId, suffix: "RemoteProxy", roleDisplay: "Remote Proxy", + serviceTreeId: input.ServiceTreeId, lifetimeMonths: input.SecretLifetimeMonths); + if (remoteProxyApp == null) return null; - _logger.LogDebug("Created Remote Proxy app: {ClientId}", remoteApp.Value.ClientId); - remoteProxyClientId = remoteApp.Value.ClientId; - remoteProxyObjectId = remoteApp.Value.ObjectId; + remoteProxyClientId = remoteProxyApp.ClientId; + remoteProxySecret = remoteProxyApp.Secret; + remoteProxyObjectId = remoteProxyApp.ObjectId; + remoteProxyAppName = remoteProxyApp.AppName; } - string? publicClientsClientId = null; - string? publicClientsObjectId = null; - - _logger.LogDebug("Creating Entra application for Public Clients..."); - var copilotApp = await _graphApiService.CreateEntraAppAsync(tenantId, publicClientsAppName, serviceTreeId: input.ServiceTreeId); - if (copilotApp != null) - { - publicClientsClientId = copilotApp.Value.ClientId; - publicClientsObjectId = copilotApp.Value.ObjectId; - _logger.LogInformation("Created Entra app '{AppName}' (clientId: {ClientId})", publicClientsAppName, publicClientsClientId); - - var copilotRedirectUri = $"ms-appx-web://Microsoft.AAD.BrokerPlugin/{publicClientsClientId}"; - var publicClientUris = new[] { copilotRedirectUri, "http://localhost:8080/callback", "https://vscode.dev/redirect", "http://localhost" }; - try - { - var success = await _retryHelper.ExecuteWithRetryAsync( - async ct => await _graphApiService.UpdateAppPublicClientRedirectUrisAsync(tenantId, publicClientsObjectId, publicClientUris, ct), - result => !result); - if (!success) - { - var msg = $"Failed to set redirect URIs on Public Clients app '{publicClientsAppName}' after retries."; - _logger.LogError(msg); - warnings.Add(msg); - } - else - { - _logger.LogDebug( - "Set {RedirectUriCount} redirect URIs on '{AppName}' ({ObjectId}): {RedirectUris}", - publicClientUris.Length, - publicClientsAppName, - publicClientsObjectId, - string.Join(", ", publicClientUris)); - } - } - catch (Exception ex) - { - var msg = $"Failed to set redirect URIs on Public Clients app: {ex.Message}"; - _logger.LogError(msg); - warnings.Add(msg); - } - } - else - { - var msg = "Failed to create Public Clients Entra app. Continuing without it."; - _logger.LogWarning(msg); - warnings.Add(msg); - } + var publicClients = await provisioner.CreatePublicClientsAppAsync( + input.ServerName, tenantId, serviceTreeId: input.ServiceTreeId, warnings); return new EntraAppSet( - A365AppClientId: a365App.Value.ClientId, - A365AppSecret: a365Secret, - A365AppObjectId: a365App.Value.ObjectId, - A365AppName: a365AppName, + A365AppClientId: a365ProxyApp.ClientId, + A365AppSecret: a365ProxyApp.Secret, + A365AppObjectId: a365ProxyApp.ObjectId, + A365AppName: a365ProxyApp.AppName, RemoteProxyClientId: remoteProxyClientId, RemoteProxySecret: remoteProxySecret, RemoteProxyObjectId: remoteProxyObjectId, RemoteProxyAppName: remoteProxyAppName, - PublicClientsClientId: publicClientsClientId, - PublicClientsObjectId: publicClientsObjectId, - PublicClientsAppName: publicClientsAppName); + PublicClientsClientId: publicClients.ClientId, + PublicClientsObjectId: publicClients.ObjectId, + PublicClientsAppName: publicClients.AppName); } private static AddMcpServerRequest BuildRequest(ResolvedInput input, EntraAppSet apps) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerRequest.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerRequest.cs index 966def0b..ceb92080 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerRequest.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerRequest.cs @@ -1,23 +1,40 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. using System.Text.Json.Serialization; namespace Microsoft.Agents.A365.DevTools.Cli.Models; /// -/// Request model for publishing an MCP server to a Dataverse environment +/// Request model for publishing an MCP server to a Dataverse environment. /// public class PublishMcpServerRequest { /// - /// Alias for the MCP server + /// Alias for the MCP server. /// [JsonPropertyName("alias")] public required string Alias { get; set; } /// - /// Display name for the MCP server + /// Display name for the MCP server. /// [JsonPropertyName("DisplayName")] public required string DisplayName { get; set; } + + /// + /// Public Clients (VS Code / Copilot CLI) Entra app client id created CLI-side. Carried in the + /// request so the platform echoes it back in the response and the CLI can wire the PPMI + /// Tools.ListInvoke.All scope onto it after publish completes. + /// + [JsonPropertyName("publicClientsAppId")] + public string? PublicClientsAppId { get; set; } + + /// + /// Publisher / developer name written into the MOS package manifest. Required for Custom + /// (user-created) servers — the platform's v2 publish validator rejects empty values for those. + /// Ignored for 1p Microsoft-owned app-based servers (e.g. msdyn_DataverseMCPServer), + /// which always publish under "Microsoft" regardless. + /// + [JsonPropertyName("publisherName")] + public string? PublisherName { get; set; } } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerResponse.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerResponse.cs index 1b874682..2ea1e2b9 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerResponse.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerResponse.cs @@ -1,28 +1,54 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +using System; using System.Text.Json.Serialization; namespace Microsoft.Agents.A365.DevTools.Cli.Models; /// -/// Response model for MCP server publish operation +/// Response model for MCP server publish operation. /// public class PublishMcpServerResponse { /// - /// Status of the publish operation + /// Status of the publish operation. /// [JsonPropertyName("Status")] public string? Status { get; set; } /// - /// Message from the API response + /// Message from the API response. /// [JsonPropertyName("Message")] public string? Message { get; set; } /// - /// Whether the operation was successful + /// Resolved underlying MCP server's Entra app client id. The platform picks the right source per + /// server type — Custom servers from managedidentityid on the mcpserver row, app-based + /// / Dataverse MCP servers from 1p server-to-app mappings, fallback to the platform's own app id. + /// The CLI uses this together with to look up the scope id and grant + /// required-resource-access on the Public Clients Entra app. + /// + [JsonPropertyName("McpServerAppId")] + public string? McpServerAppId { get; set; } + + /// + /// Resolved OAuth scope name on the underlying MCP server's app. Paired with ; + /// the CLI calls Graph's GetOAuth2PermissionScopeId on (McpServerAppId, McpServerScope) to get the + /// scope guid for required-resource-access grants. + /// + [JsonPropertyName("McpServerScope")] + public string? McpServerScope { get; set; } + + /// + /// Public Clients Entra app client id, echoed back from the request so post-response + /// orchestration can grant the PPMI scope onto it. + /// + [JsonPropertyName("PublicClientsAppId")] + public string? PublicClientsAppId { get; set; } + + /// + /// Whether the operation was successful. /// [JsonIgnore] public bool IsSuccess => Status?.Equals("Success", StringComparison.OrdinalIgnoreCase) ?? false; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs index 374357e9..f57bfa76 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs @@ -251,7 +251,9 @@ private string BuildListMcpServersUrl(string environment, string environmentId) } /// - /// Builds URL for publishing an MCP server to a Dataverse environment + /// Builds URL for publishing an MCP server to a Dataverse environment. Hits the platform's v2 + /// publish endpoint, which performs the full elevation orchestration (PPMI provisioning and MOS + /// upload). /// /// Environment name /// Dataverse environment ID @@ -260,7 +262,7 @@ private string BuildListMcpServersUrl(string environment, string environmentId) private string BuildPublishMcpServerUrl(string environment, string environmentId, string serverName) { var baseUrl = BuildAgent365ToolsBaseUrl(environment); - return $"{baseUrl}/agents/dataverse/environments/{environmentId}/mcpServers/{serverName}/publish"; + return $"{baseUrl}/agents/dataverse/environments/{environmentId}/mcpServers/{serverName}/publish/v2"; } /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs new file mode 100644 index 00000000..14d705a0 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs @@ -0,0 +1,210 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; +using Microsoft.Extensions.Logging; + +namespace Microsoft.Agents.A365.DevTools.Cli.Services; + +/// +/// Creates the Entra app registrations needed by publish and register flows. Both flows produce +/// the same two app shapes (a confidential proxy app with a secret, and a public-client app with +/// canonical redirect URIs); register additionally produces a second proxy app for the remote +/// MCP server. This provisioner centralizes the creation steps and their error-handling so the +/// command executors can compose them without duplicating the per-step null checks and logging. +/// +internal class EntraAppProvisioner +{ + private static readonly string[] PublicClientCanonicalRedirectUris = + [ + "http://localhost:8080/callback", + "https://vscode.dev/redirect", + "http://localhost", + ]; + + private readonly ILogger _logger; + private readonly GraphApiService _graphApiService; + private readonly RetryHelper _retryHelper; + + internal EntraAppProvisioner(ILogger logger, GraphApiService graphApiService, RetryHelper retryHelper) + { + _logger = logger; + _graphApiService = graphApiService; + _retryHelper = retryHelper; + } + + /// + /// Result of creating a confidential proxy app (A365 Proxy or Remote Proxy). All fields are + /// guaranteed non-empty on success; a null return indicates failure with the cause already + /// logged. + /// + internal sealed record ProxyAppResult(string ClientId, string Secret, string ObjectId, string AppName); + + /// + /// Result of creating the Public Clients app. Failure here is non-fatal: the caller still + /// receives the resolved AppName and a warning is appended to the supplied list, but + /// ClientId/ObjectId may be null if the app or redirect-URI setup failed. + /// + internal sealed record PublicClientsAppResult(string? ClientId, string? ObjectId, string AppName); + + /// + /// Creates a confidential proxy Entra app named {serverName}-{suffix}, adds a client + /// secret, and validates that the returned client ID is non-empty. Used for both the A365 + /// Proxy (publish + register) and the Remote Proxy (register, when auth is EntraOAuth). + /// + /// Optional secret lifetime in months, passed through to Graph's + /// addPassword call. When null, Graph applies its default (~2 years). Register surfaces + /// this via --secret-lifetime-months so users in tenants whose appManagementPolicies + /// cap credential lifetimes below 2 years can opt into a shorter expiry; publish leaves it null. + internal virtual async Task CreateProxyAppAsync( + string serverName, + string tenantId, + string suffix, + string roleDisplay, + string? serviceTreeId, + int? lifetimeMonths = null, + CancellationToken ct = default) + { + var appName = $"{serverName}-{suffix}"; + + _logger.LogDebug("Creating Entra application for {Role}...", roleDisplay); + var app = await _graphApiService.CreateEntraAppAsync(tenantId, appName, serviceTreeId: serviceTreeId, ct); + if (app == null) + { + _logger.LogError("Failed to create Entra application '{AppName}'. Ensure you have Application.ReadWrite.All permission in the target tenant. Run with -v for details.", appName); + return null; + } + _logger.LogInformation("Created Entra app '{AppName}' (clientId: {ClientId})", appName, app.Value.ClientId); + + var secret = await _graphApiService.AddAppPasswordAsync(tenantId, app.Value.ObjectId, lifetimeMonths: lifetimeMonths, ct: ct); + if (string.IsNullOrWhiteSpace(secret)) + { + _logger.LogError("Failed to create secret for '{AppName}'. Run with -v for details.", appName); + await TryDeleteOrphanedAppAsync(tenantId, app.Value.ObjectId, appName, "secret-creation failed", ct); + return null; + } + + if (string.IsNullOrWhiteSpace(app.Value.ClientId)) + { + _logger.LogError("{Role} Entra application was created but returned an empty client ID", roleDisplay); + await TryDeleteOrphanedAppAsync(tenantId, app.Value.ObjectId, appName, "empty client ID returned", ct); + return null; + } + + _logger.LogDebug("Created {Role} app: {ClientId}", roleDisplay, app.Value.ClientId); + return new ProxyAppResult(app.Value.ClientId, secret, app.Value.ObjectId, appName); + } + + /// + /// Best-effort compensating delete for an Entra app that was successfully created but failed + /// a follow-up step (secret creation, post-create validation). Without this, partial failures + /// in leak orphan app registrations into the user's tenant + /// on a likely failure mode (Graph throttling, permission gaps). Delete failures are surfaced + /// as warnings — the caller has already returned null with a clear error, so we don't want a + /// secondary cleanup error to drown the root cause; the user can clean up manually using the + /// objectId we log. + /// + private async Task TryDeleteOrphanedAppAsync(string tenantId, string objectId, string appName, string reason, CancellationToken ct) + { + if (string.IsNullOrWhiteSpace(objectId)) + { + return; + } + + try + { + var deleted = await _graphApiService.DeleteEntraAppAsync(tenantId, objectId, ct); + if (deleted) + { + _logger.LogInformation( + "Rolled back orphan Entra app '{AppName}' (objectId {ObjectId}) after {Reason}.", + appName, objectId, reason); + } + else + { + _logger.LogWarning( + "Could not roll back Entra app '{AppName}' (objectId {ObjectId}) after {Reason}. Delete it manually in the Azure portal.", + appName, objectId, reason); + } + } + catch (Exception ex) + { + _logger.LogWarning( + ex, + "Exception rolling back Entra app '{AppName}' (objectId {ObjectId}) after {Reason}. Delete it manually in the Azure portal.", + appName, objectId, reason); + } + } + + /// + /// Creates the Public Clients Entra app named {serverName}-PublicClients and sets its + /// public-client redirect URIs (broker plugin + canonical localhost/vscode URIs). Best-effort: + /// any failure is converted to a warning rather than failing the caller; the returned record + /// carries null IDs to signal partial success while still surfacing the app name. + /// + internal virtual async Task CreatePublicClientsAppAsync( + string serverName, + string tenantId, + string? serviceTreeId, + List warnings, + CancellationToken ct = default) + { + var appName = $"{serverName}-PublicClients"; + + _logger.LogDebug("Creating Entra application for Public Clients..."); + var app = await _graphApiService.CreateEntraAppAsync(tenantId, appName, serviceTreeId: serviceTreeId, ct); + if (app == null) + { + var msg = "Failed to create Public Clients Entra app. Continuing without it."; + _logger.LogWarning(msg); + warnings.Add(msg); + return new PublicClientsAppResult(null, null, appName); + } + + var clientId = app.Value.ClientId; + var objectId = app.Value.ObjectId; + _logger.LogInformation("Created Entra app '{AppName}' (clientId: {ClientId})", appName, clientId); + + var brokerRedirectUri = $"ms-appx-web://Microsoft.AAD.BrokerPlugin/{clientId}"; + var publicClientUris = new[] { brokerRedirectUri }.Concat(PublicClientCanonicalRedirectUris).ToArray(); + + try + { + var success = await _retryHelper.ExecuteWithRetryAsync( + async retryCt => await _graphApiService.UpdateAppPublicClientRedirectUrisAsync(tenantId, objectId, publicClientUris, retryCt), + result => !result, + cancellationToken: ct); + if (!success) + { + var msg = $"Failed to set redirect URIs on Public Clients app '{appName}' after retries."; + _logger.LogError(msg); + warnings.Add(msg); + } + else + { + _logger.LogDebug( + "Set {RedirectUriCount} redirect URIs on '{AppName}' ({ObjectId}): {RedirectUris}", + publicClientUris.Length, + appName, + objectId, + string.Join(", ", publicClientUris)); + } + } + catch (Exception ex) + { + // Caller cancellation (Ctrl+C) should terminate the CLI promptly rather than be demoted + // to a warning and continue. Rethrow so cancellation propagates, matching the handling + // in the command executors and setup flows. + if (ex is OperationCanceledException && ct.IsCancellationRequested) + { + throw; + } + + var msg = $"Failed to set redirect URIs on Public Clients app: {ex.Message}"; + _logger.LogError(msg); + warnings.Add(msg); + } + + return new PublicClientsAppResult(clientId, objectId, appName); + } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs index dd110f90..0e5df40e 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs @@ -33,13 +33,14 @@ public interface IAgent365ToolingService CancellationToken cancellationToken = default); /// - /// Publishes an MCP server to a Dataverse environment + /// Publishes an MCP server to a Dataverse environment via the platform's v2 publish endpoint, + /// which performs the full elevation orchestration (PPMI provisioning and MOS upload). /// /// Dataverse environment ID /// MCP server name to publish - /// Publish request with alias, display name, and description + /// Publish request with alias, display name, the Public Clients Entra app id, and optional publisher name /// Cancellation token - /// Response from the publish operation + /// Response from the publish operation, including the underlying server's app id and OAuth scope (for the post-publish required-resource-access grant) and the echoed Public Clients app id Task PublishServerAsync( string environmentId, string serverName, diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs index 50a9c375..4b0308bd 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs @@ -94,46 +94,244 @@ public async Task AzureCliStyleParameters_AreAcceptedCorrectly(string command, p result.Should().Be(0, $"Azure CLI style command should be accepted: {string.Join(" ", fullCommand)}"); } - [Fact] - public async Task ServiceIntegration_PublishCommand_PassesCorrectParameters() + [Fact] + public async Task PublishCommand_AcceptsAllNamedParameters_InDryRun() { - // Core functionality test: Ensures publish command integration works correctly - + // Verifies the publish CLI parses every documented flag without error. Dry-run + // short-circuits before any platform call, so this is a pure CLI parsing test — + // it does NOT verify that the parsed values flow into PublishServerAsync. + // That contract is covered by PublishCommand_ForwardsParsedParametersToToolingService + // (which mocks Graph + tenant detection so the non-dry-run path can be exercised). + // Arrange var testEnvId = "test-environment-123"; var testServerName = "msdyn_TestServer"; var testAlias = "test-alias"; var testDisplayName = "Test Server Display Name"; - var mockResponse = new PublishMcpServerResponse + // Act — dry-run short-circuits the Graph + platform calls so this stays a pure CLI parsing test. + var result = await _command.InvokeAsync(new[] { - Status = "Success", - Message = "Server published successfully" - }; - - _mockToolingService.PublishServerAsync(testEnvId, testServerName, Arg.Any()) - .Returns(mockResponse); - - // Act - var result = await _command.InvokeAsync(new[] - { - "publish", + "publish", "--environment-id", testEnvId, "--server-name", testServerName, "--alias", testAlias, - "--display-name", testDisplayName + "--display-name", testDisplayName, + "--dry-run", }); + // Assert — successful parse + dispatch, no service calls. + result.Should().Be( + 0, + because: "dry-run should never trigger a non-zero exit code when all flags parse cleanly."); + await _mockToolingService.DidNotReceive().PublishServerAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + + /// + /// Strengthened contract test: verifies that every parsed publish flag flows into the actual + /// call. Exercises the non-dry-run + /// path by (a) using --yes to bypass the interactive confirmation prompt, (b) mocking + /// so Entra app creation succeeds without a real tenant, and + /// (c) subclassing to stub out tenant auto-detection so + /// the executor doesn't shell out to Azure CLI in CI. Catches breakage of the CLI-to-service + /// contract (alias/display-name/publisher-name mapping, request shaping, and selecting the + /// correct service method) that the dry-run-only test above can't catch. + /// + [Fact] + public async Task PublishCommand_ForwardsParsedParametersToToolingService() + { + // Arrange + const string TestTenantId = "test-tenant-99999"; + const string TestEnvironmentId = "test-env-forward"; + const string TestServerName = "msdyn_TestServer"; + const string TestAlias = "test-alias-forward"; + const string TestDisplayName = "Test Display Forward"; + const string TestPublisherName = "Contoso Forward"; + const string TestPublicClientsObjectId = "public-clients-object-id"; + const string TestPublicClientsClientId = "public-clients-client-id"; + + var logger = Substitute.For(); + var toolingService = Substitute.For(); + var graphApiService = Substitute.For(); + + // Mock Graph so CreateEntraAppsAsync → factory.CreatePublicClientsAppAsync succeeds. + graphApiService.CreateEntraAppAsync( + TestTenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>( + (TestPublicClientsObjectId, TestPublicClientsClientId))); + graphApiService.UpdateAppPublicClientRedirectUrisAsync( + TestTenantId, TestPublicClientsObjectId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); + + // Mock Graph for ConfigureEntraAppsAsync → required-resource-access grant on Public Clients. + graphApiService.GetOAuth2PermissionScopeIdAsync( + TestTenantId, Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(Guid.NewGuid())); + graphApiService.AddRequiredResourceAccessAsync( + TestTenantId, Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); + + // Capture what gets forwarded to PublishServerAsync. + string? capturedEnvId = null; + string? capturedServerName = null; + PublishMcpServerRequest? capturedRequest = null; + toolingService.PublishServerAsync( + Arg.Do(e => capturedEnvId = e), + Arg.Do(s => capturedServerName = s), + Arg.Do(r => capturedRequest = r), + Arg.Any()) + .Returns(new PublishMcpServerResponse + { + Status = "Success", + McpServerAppId = Guid.NewGuid().ToString(), + McpServerScope = "Tools.ListInvoke.All", + }); + + var executor = new TestablePublishCommandExecutor( + logger, toolingService, graphApiService, TestTenantId); + + var args = new RawPublishArgs( + EnvironmentId: TestEnvironmentId, + ServerName: TestServerName, + Alias: TestAlias, + DisplayName: TestDisplayName, + PublisherName: TestPublisherName, + Yes: true, + DryRun: false); + + // Act + var result = await executor.ExecuteAsync(args); + // Assert - result.Should().Be(0); - - await _mockToolingService.Received(1).PublishServerAsync( - testEnvId, - testServerName, - Arg.Is(req => - req.Alias == testAlias && - req.DisplayName == testDisplayName) - ); + result.Should().BeTrue( + because: "the happy path with all dependencies mocked must return true so the publish " + + "handler exits 0."); + capturedRequest.Should().NotBeNull( + because: "PublishServerAsync must be invoked once the prompt is skipped, the tenant " + + "is detected, and the Public Clients app is created."); + capturedEnvId.Should().Be( + TestEnvironmentId, + because: "--environment-id must flow unchanged into PublishServerAsync's first positional arg."); + capturedServerName.Should().Be( + TestServerName, + because: "--server-name must flow unchanged into PublishServerAsync's second positional arg."); + capturedRequest!.Alias.Should().Be( + TestAlias, + because: "--alias must populate request.Alias; this is the platform's `name` for the published row."); + capturedRequest.DisplayName.Should().Be( + TestDisplayName, + because: "--display-name must populate request.DisplayName; the platform's v2 validator " + + "requires it and surfaces it in MOS."); + capturedRequest.PublisherName.Should().Be( + TestPublisherName, + because: "--publisher-name must populate request.PublisherName so the MOS manifest's " + + "developer field is set for custom servers (the platform rejects empty values " + + "for non-1p servers)."); + capturedRequest.PublicClientsAppId.Should().Be( + TestPublicClientsClientId, + because: "the just-created Public Clients Entra app's clientId must be carried to the " + + "platform so it can be echoed back and the CLI can grant the PPMI scope on it " + + "post-publish."); + } + + /// + /// Regression test for the empty-publisher case: an explicit --publisher-name "" (empty / + /// whitespace) must be treated as "no publisher" without triggering the interactive prompt, so + /// non-interactive automation can't hang. Exercises the non-dry-run path with --yes; the + /// proof the prompt was skipped is that the executor reaches PublishServerAsync at all (a real + /// prompt would block on Console.ReadLine), and the forwarded request carries a null publisher. + /// + [Fact] + public async Task PublishCommand_ExplicitEmptyPublisherName_SkipsPromptAndForwardsNull() + { + // Arrange + const string TestTenantId = "test-tenant-99999"; + const string TestEnvironmentId = "test-env-empty-pub"; + const string TestServerName = "msdyn_TestServer"; + const string TestAlias = "test-alias-empty-pub"; + const string TestDisplayName = "Test Display Empty Pub"; + const string TestPublicClientsObjectId = "public-clients-object-id"; + const string TestPublicClientsClientId = "public-clients-client-id"; + + var logger = Substitute.For(); + var toolingService = Substitute.For(); + var graphApiService = Substitute.For(); + + graphApiService.CreateEntraAppAsync( + TestTenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>( + (TestPublicClientsObjectId, TestPublicClientsClientId))); + graphApiService.UpdateAppPublicClientRedirectUrisAsync( + TestTenantId, TestPublicClientsObjectId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); + graphApiService.GetOAuth2PermissionScopeIdAsync( + TestTenantId, Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(Guid.NewGuid())); + graphApiService.AddRequiredResourceAccessAsync( + TestTenantId, Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); + + PublishMcpServerRequest? capturedRequest = null; + toolingService.PublishServerAsync( + Arg.Any(), + Arg.Any(), + Arg.Do(r => capturedRequest = r), + Arg.Any()) + .Returns(new PublishMcpServerResponse + { + Status = "Success", + McpServerAppId = Guid.NewGuid().ToString(), + McpServerScope = "Tools.ListInvoke.All", + }); + + var executor = new TestablePublishCommandExecutor( + logger, toolingService, graphApiService, TestTenantId); + + var args = new RawPublishArgs( + EnvironmentId: TestEnvironmentId, + ServerName: TestServerName, + Alias: TestAlias, + DisplayName: TestDisplayName, + PublisherName: string.Empty, // explicit empty, e.g. --publisher-name "" from a script + Yes: true, + DryRun: false); + + // Act + var result = await executor.ExecuteAsync(args); + + // Assert + result.Should().BeTrue( + because: "an explicit empty publisher is valid (treated as no publisher), so the happy " + + "path with all dependencies mocked must return true."); + capturedRequest.Should().NotBeNull( + because: "the executor must proceed to PublishServerAsync without prompting; an explicit " + + "empty --publisher-name must not be treated as 'missing' and block on the prompt."); + capturedRequest!.PublisherName.Should().BeNull( + because: "an explicit empty/whitespace --publisher-name is normalized to null ('no " + + "publisher'); the platform decides whether that's acceptable for the server type."); + } + + /// + /// Test-only subclass of that stubs out + /// with a known value, so the + /// strengthened contract test doesn't need to shell out to az account show in CI. + /// + private sealed class TestablePublishCommandExecutor : PublishCommandExecutor + { + private readonly string? _tenantId; + + public TestablePublishCommandExecutor( + ILogger logger, + IAgent365ToolingService toolingService, + GraphApiService? graphApiService, + string? tenantId) + : base(logger, toolingService, graphApiService) + { + _tenantId = tenantId; + } + + protected override Task DetectTenantIdAsync() => Task.FromResult(_tenantId); } [Fact] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandTests.cs index d53e9161..b42bc20f 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandTests.cs @@ -115,29 +115,44 @@ public void PublishSubcommand_HasCorrectOptionsWithAliases() var command = DevelopMcpCommand.CreateCommand(_mockLogger, _mockToolingService); var subcommand = command.Subcommands.First(sc => sc.Name == "publish"); - // Assert - subcommand.Description.Should().Be("Publish an MCP server to a Dataverse environment"); - + // Assert — exact match. The description is intentionally terse (no internal-implementation + // details exposed to end users); a regression that re-adds back-fill / orchestration prose + // should fail this test until the user-facing copy is reviewed. + subcommand.Description.Should().Be( + "Publish an MCP server to a Dataverse environment.", + because: "publish description is user-facing and must stay terse; do not leak internal " + + "orchestration details (Entra apps, back-fill, etc.) into the help output."); + var options = subcommand.Options.ToList(); - - // Verify all expected options exist + + // Verify all expected options exist. Tenant ID is auto-detected from the current az login + // session, so publish does not expose --tenant-id; ServiceTree tagging is not required for + // publish since it targets Dataverse environments rather than Microsoft corp tenants. var optionNames = options.Select(o => o.Name).ToList(); optionNames.Should().Contain("environment-id"); optionNames.Should().Contain("server-name"); optionNames.Should().Contain("alias"); optionNames.Should().Contain("display-name"); + optionNames.Should().NotContain( + "tenant-id", + because: "tenant id is auto-detected from the current 'az login' session; exposing " + + "--tenant-id would imply per-publish tenant targeting that the executor does not support."); + optionNames.Should().NotContain( + "service-tree-id", + because: "publish targets a customer's Dataverse env, not a Microsoft corp tenant — " + + "the ServiceTree tagging that --service-tree-id provides is not applicable here."); optionNames.Should().Contain("dry-run"); // Verify critical aliases for Azure CLI compliance var envOption = options.FirstOrDefault(o => o.Name == "environment-id"); envOption!.Aliases.Should().Contain("-e"); - + var serverOption = options.FirstOrDefault(o => o.Name == "server-name"); serverOption!.Aliases.Should().Contain("-s"); - + var aliasOption = options.FirstOrDefault(o => o.Name == "alias"); aliasOption!.Aliases.Should().Contain("-a"); - + var displayNameOption = options.FirstOrDefault(o => o.Name == "display-name"); displayNameOption!.Aliases.Should().Contain("-d"); } @@ -275,7 +290,7 @@ public void RegisterExternalMcpServerSubcommand_HasAllExpectedOptions() optionNames.Should().Contain("tools"); optionNames.Should().Contain("input-file"); optionNames.Should().Contain("remote-scopes"); - optionNames.Should().Contain("tenant-id"); + optionNames.Should().NotContain("tenant-id", because: "tenant ID is auto-detected by RegisterCommandExecutor via TenantDetectionHelper"); optionNames.Should().Contain("service-tree-id"); optionNames.Should().Contain("secret-lifetime-months"); optionNames.Should().Contain("publisher"); @@ -298,9 +313,6 @@ public void RegisterExternalMcpServerSubcommand_HasAllExpectedOptions() var inputFileOption = options.First(o => o.Name == "input-file"); inputFileOption.Aliases.Should().Contain("-f"); - var tenantIdOption = options.First(o => o.Name == "tenant-id"); - tenantIdOption.Aliases.Should().Contain("-t"); - var verboseOption = options.First(o => o.Name == "verbose"); verboseOption.Aliases.Should().Contain("-v"); } @@ -318,7 +330,6 @@ public void RegisterExternalMcpServerSubcommand_HasAllExpectedOptions() [InlineData("register-external-mcp-server", "server-url", "-u")] [InlineData("register-external-mcp-server", "auth-type", "-a")] [InlineData("register-external-mcp-server", "input-file", "-f")] - [InlineData("register-external-mcp-server", "tenant-id", "-t")] [InlineData("register-external-mcp-server", "secret-lifetime-months", "-l")] public void CriticalOptions_HaveConsistentAliases(string subcommandName, string optionName, string expectedAlias) { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorDryRunTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorDryRunTests.cs new file mode 100644 index 00000000..1989ea12 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorDryRunTests.cs @@ -0,0 +1,104 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Commands; +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging; +using NSubstitute; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; + +/// +/// Tests for dry-run output. The dry-run log must mirror the +/// real Entra app naming scheme (derived from ServerName) so users can predict what will be +/// created — the {ServerName}-PublicClients app. +/// +public class PublishCommandExecutorDryRunTests +{ + /// + /// The dry-run log must (a) name only the Public Clients app — derived from ServerName, + /// not Alias — (b) describe a PPMI-scope-only back-fill (no redirect-URI back-fill), and + /// (c) skip the platform publish call entirely. + /// + [Fact] + public async Task ExecuteAsync_DryRun_NamesPublicClientsApp_AndBackfillsPpmiScopeOnly() + { + var logger = Substitute.For(); + var toolingService = Substitute.For(); + + // ServerName and Alias are intentionally distinct so a regression that reverts to Alias + // would surface in the negative assertion below. + const string serverName = "mcp_TestServer"; + const string alias = "myAlias"; + const string expectedPublicClientsAppName = "mcp_TestServer-PublicClients"; + + var args = new RawPublishArgs( + EnvironmentId: "00000000-0000-0000-0000-000000000000", + ServerName: serverName, + Alias: alias, + DisplayName: "Test Display", + PublisherName: null, + Yes: false, + DryRun: true); + + var executor = new PublishCommandExecutor(logger, toolingService, graphApiService: null); + + await executor.ExecuteAsync(args, CancellationToken.None); + + // The dry-run line names the Public Clients app (derived from ServerName, not Alias). + logger.Received(1).Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => + o.ToString()!.Contains("[DRY RUN] Would create Entra app") && + o.ToString()!.Contains(expectedPublicClientsAppName) && + !o.ToString()!.Contains($"{alias}-PublicClients")), + Arg.Any(), + Arg.Any>()); + + // The back-fill line now mentions only PPMI scope, not redirect URI. + logger.Received(1).Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => + o.ToString()!.Contains("back-fill PPMI scope") && + !o.ToString()!.Contains("redirect URI")), + Arg.Any(), + Arg.Any>()); + + await toolingService.DidNotReceiveWithAnyArgs().PublishServerAsync(default!, default!, default!, default); + } + + /// + /// Sanity check that --publisher-name threads through the executor without crashing the + /// dry-run flow. The platform's v2 validator rejects an empty publisher only for Custom servers; + /// at this CLI layer we don't classify, we just forward what was provided. This test pins that + /// the field is accepted on , no interactive prompt is invoked when + /// a value is supplied (the prompt would hang in xUnit without stdin), and the dry-run still + /// short-circuits before any platform call. + /// + [Fact] + public async Task ExecuteAsync_DryRun_AcceptsPublisherName_WithoutCallingPlatform() + { + var logger = Substitute.For(); + var toolingService = Substitute.For(); + + var args = new RawPublishArgs( + EnvironmentId: "00000000-0000-0000-0000-000000000000", + ServerName: "mcp_TestServer", + Alias: "myAlias", + DisplayName: "Test Display", + PublisherName: "Contoso", + Yes: false, + DryRun: true); + + var executor = new PublishCommandExecutor(logger, toolingService, graphApiService: null); + + await executor.ExecuteAsync(args, CancellationToken.None); + + // Dry-run short-circuits before the platform publish call regardless of publisher. + await toolingService.DidNotReceiveWithAnyArgs().PublishServerAsync(default!, default!, default!, default); + } +} diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/EntraAppProvisionerTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/EntraAppProvisionerTests.cs new file mode 100644 index 00000000..4b7c6624 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/EntraAppProvisionerTests.cs @@ -0,0 +1,231 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; +using Microsoft.Extensions.Logging; +using NSubstitute; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; + +/// +/// Tests for . The provisioner absorbed the per-app creation flows +/// that previously lived inline in PublishCommandExecutor and RegisterCommandExecutor. These tests +/// pin every branch (success + each failure mode) so the executors can compose the provisioner +/// without needing their own per-step coverage. +/// +public class EntraAppProvisionerTests +{ + private const string ServerName = "mcp_TestServer"; + private const string TenantId = "00000000-0000-0000-0000-0000000000aa"; + private const string AppObjectId = "11111111-1111-1111-1111-111111111111"; + private const string AppClientId = "22222222-2222-2222-2222-222222222222"; + private const string AppSecret = "fake-secret"; + + private readonly ILogger _logger; + private readonly GraphApiService _graph; + private readonly RetryHelper _retryHelper; + private readonly EntraAppProvisioner _provisioner; + + public EntraAppProvisionerTests() + { + _logger = Substitute.For(); + _graph = Substitute.For(); + _retryHelper = new RetryHelper(_logger, maxRetries: 1, baseDelaySeconds: 0); + _provisioner = new EntraAppProvisioner(_logger, _graph, _retryHelper); + } + + [Fact] + public async Task CreateProxyAppAsync_HappyPath_ReturnsAllFieldsPopulated() + { + _graph.CreateEntraAppAsync(TenantId, $"{ServerName}-A365Proxy", serviceTreeId: "svc-tree-7", Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.AddAppPasswordAsync(TenantId, AppObjectId).Returns(Task.FromResult(AppSecret)); + + var result = await _provisioner.CreateProxyAppAsync(ServerName, TenantId, suffix: "A365Proxy", roleDisplay: "A365 Proxy", serviceTreeId: "svc-tree-7"); + + result.Should().NotBeNull(); + result!.ClientId.Should().Be(AppClientId); + result.Secret.Should().Be(AppSecret); + result.ObjectId.Should().Be(AppObjectId); + result.AppName.Should().Be($"{ServerName}-A365Proxy"); + + await _graph.Received(1).CreateEntraAppAsync(TenantId, $"{ServerName}-A365Proxy", serviceTreeId: "svc-tree-7", Arg.Any()); + await _graph.Received(1).AddAppPasswordAsync(TenantId, AppObjectId); + } + + [Fact] + public async Task CreateProxyAppAsync_UsesSuffixInAppName() + { + _graph.CreateEntraAppAsync(TenantId, $"{ServerName}-RemoteProxy", serviceTreeId: null, Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.AddAppPasswordAsync(TenantId, AppObjectId).Returns(Task.FromResult(AppSecret)); + + var result = await _provisioner.CreateProxyAppAsync(ServerName, TenantId, suffix: "RemoteProxy", roleDisplay: "Remote Proxy", serviceTreeId: null); + + result.Should().NotBeNull(); + result!.AppName.Should().Be($"{ServerName}-RemoteProxy"); + } + + [Fact] + public async Task CreateProxyAppAsync_WhenCreateAppReturnsNull_ReturnsNullAndLogsError() + { + _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>(null)); + + var result = await _provisioner.CreateProxyAppAsync(ServerName, TenantId, suffix: "A365Proxy", roleDisplay: "A365 Proxy", serviceTreeId: null); + + result.Should().BeNull(); + await _graph.DidNotReceive().AddAppPasswordAsync(Arg.Any(), Arg.Any()); + _logger.Received(1).Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("Failed to create Entra application") && o.ToString()!.Contains($"{ServerName}-A365Proxy")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task CreateProxyAppAsync_WhenAddPasswordReturnsNull_ReturnsNullAndLogsError() + { + _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.AddAppPasswordAsync(TenantId, AppObjectId).Returns(Task.FromResult(null)); + + var result = await _provisioner.CreateProxyAppAsync(ServerName, TenantId, suffix: "A365Proxy", roleDisplay: "A365 Proxy", serviceTreeId: null); + + result.Should().BeNull(); + _logger.Received(1).Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("Failed to create secret") && o.ToString()!.Contains($"{ServerName}-A365Proxy")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task CreateProxyAppAsync_WhenAddPasswordReturnsWhitespace_ReturnsNullAndLogsError() + { + _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.AddAppPasswordAsync(TenantId, AppObjectId).Returns(Task.FromResult(" ")); + + var result = await _provisioner.CreateProxyAppAsync(ServerName, TenantId, suffix: "A365Proxy", roleDisplay: "A365 Proxy", serviceTreeId: null); + + result.Should().BeNull(); + } + + [Fact] + public async Task CreateProxyAppAsync_WhenClientIdIsEmpty_ReturnsNullAndLogsError() + { + _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, string.Empty))); + _graph.AddAppPasswordAsync(TenantId, AppObjectId).Returns(Task.FromResult(AppSecret)); + + var result = await _provisioner.CreateProxyAppAsync(ServerName, TenantId, suffix: "RemoteProxy", roleDisplay: "Remote Proxy", serviceTreeId: null); + + result.Should().BeNull(); + _logger.Received(1).Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("Remote Proxy") && o.ToString()!.Contains("empty client ID")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task CreatePublicClientsAppAsync_HappyPath_ReturnsIdsAndSetsBrokerAndCanonicalRedirectUris() + { + var capturedUris = new List(); + _graph.CreateEntraAppAsync(TenantId, $"{ServerName}-PublicClients", serviceTreeId: "svc-tree-9", Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.UpdateAppPublicClientRedirectUrisAsync( + TenantId, + AppObjectId, + Arg.Do(uris => capturedUris.Add(uris)), + Arg.Any()) + .Returns(Task.FromResult(true)); + + var warnings = new List(); + var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: "svc-tree-9", warnings); + + result.Should().NotBeNull(); + result.ClientId.Should().Be(AppClientId); + result.ObjectId.Should().Be(AppObjectId); + result.AppName.Should().Be($"{ServerName}-PublicClients"); + warnings.Should().BeEmpty(); + + capturedUris.Should().ContainSingle(); + var uris = capturedUris[0]; + uris.Should().BeEquivalentTo( + new[] + { + $"ms-appx-web://Microsoft.AAD.BrokerPlugin/{AppClientId}", + "http://localhost:8080/callback", + "https://vscode.dev/redirect", + "http://localhost", + }, + opt => opt.WithStrictOrdering(), + because: "Public Clients redirect URIs are part of the OAuth contract with VS Code / " + + "Copilot CLI and the Windows broker. The broker URI is required for WAM/SSO " + + "on Windows, the localhost callbacks support MSAL.NET and Copilot CLI flows, " + + "and vscode.dev/redirect supports the VS Code web client. Silently " + + "adding/removing/reordering entries breaks one of those flows."); + } + + [Fact] + public async Task CreatePublicClientsAppAsync_WhenCreateAppReturnsNull_ReturnsAppNameOnlyAndAppendsWarning() + { + _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>(null)); + + var warnings = new List(); + var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: null, warnings); + + result.ClientId.Should().BeNull(); + result.ObjectId.Should().BeNull(); + result.AppName.Should().Be($"{ServerName}-PublicClients"); + warnings.Should().ContainSingle().Which.Should().Be("Failed to create Public Clients Entra app. Continuing without it."); + + await _graph.DidNotReceive().UpdateAppPublicClientRedirectUrisAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task CreatePublicClientsAppAsync_WhenRedirectUriUpdateReturnsFalse_ReturnsIdsAndAppendsRetryWarning() + { + _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.UpdateAppPublicClientRedirectUrisAsync( + TenantId, AppObjectId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(false)); + + var warnings = new List(); + var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: null, warnings); + + result.ClientId.Should().Be(AppClientId); + result.ObjectId.Should().Be(AppObjectId); + result.AppName.Should().Be($"{ServerName}-PublicClients"); + warnings.Should().ContainSingle().Which.Should().Be($"Failed to set redirect URIs on Public Clients app '{ServerName}-PublicClients' after retries."); + } + + [Fact] + public async Task CreatePublicClientsAppAsync_WhenRedirectUriUpdateThrows_ReturnsIdsAndAppendsExceptionWarning() + { + _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.UpdateAppPublicClientRedirectUrisAsync( + TenantId, AppObjectId, Arg.Any(), Arg.Any()) + .Returns>(_ => throw new InvalidOperationException("Graph blew up")); + + var warnings = new List(); + var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: null, warnings); + + result.ClientId.Should().Be(AppClientId); + result.ObjectId.Should().Be(AppObjectId); + result.AppName.Should().Be($"{ServerName}-PublicClients"); + warnings.Should().ContainSingle().Which.Should().Be("Failed to set redirect URIs on Public Clients app: Graph blew up"); + } +}