diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs index 941b1d10..cabaa04b 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -1,11 +1,13 @@ // 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.Evaluate; using Microsoft.Extensions.Logging; using System.CommandLine; +using System.Linq; namespace Microsoft.Agents.A365.DevTools.Cli.Commands; @@ -37,7 +39,7 @@ public static Command CreateCommand( developMcpCommand.AddCommand(CreateListEnvironmentsSubcommand(logger, toolingService)); developMcpCommand.AddCommand(CreateListServersSubcommand(logger, toolingService)); developMcpCommand.AddCommand(CreatePublishSubcommand(logger, toolingService, graphApiService)); - developMcpCommand.AddCommand(CreateUnpublishSubcommand(logger, toolingService)); + developMcpCommand.AddCommand(CreateUnpublishSubcommand(logger, toolingService, graphApiService)); developMcpCommand.AddCommand(CreateRegisterExternalMcpServerSubcommand(logger, toolingService, graphApiService)); if (evaluationPipelineService is not null) @@ -424,7 +426,8 @@ private static Command CreatePublishSubcommand( /// private static Command CreateUnpublishSubcommand( ILogger logger, - IAgent365ToolingService toolingService) + IAgent365ToolingService toolingService, + GraphApiService? graphApiService) { var command = new Command("unpublish", "Unpublish an MCP server from a Dataverse environment"); @@ -454,9 +457,12 @@ private static Command CreateUnpublishSubcommand( ); command.AddOption(verboseOption); - command.SetHandler(async (envId, serverName, dryRun, verbose) => + command.SetHandler(async (context) => { - _ = verbose; + var envId = context.ParseResult.GetValueForOption(envIdOption); + var serverName = context.ParseResult.GetValueForOption(serverNameOption); + var dryRun = context.ParseResult.GetValueForOption(dryRunOption); + try { // Validate and prompt for missing required arguments with security checks @@ -466,6 +472,7 @@ private static Command CreateUnpublishSubcommand( if (string.IsNullOrWhiteSpace(envId)) { logger.LogError("Environment ID is required"); + context.ExitCode = 1; return; } } @@ -476,6 +483,7 @@ private static Command CreateUnpublishSubcommand( if (envId == null) { logger.LogError("Invalid environment ID format"); + context.ExitCode = 1; return; } } @@ -486,6 +494,7 @@ private static Command CreateUnpublishSubcommand( if (string.IsNullOrWhiteSpace(serverName)) { logger.LogError("Server name is required"); + context.ExitCode = 1; return; } } @@ -496,6 +505,7 @@ private static Command CreateUnpublishSubcommand( if (serverName == null) { logger.LogError("Invalid server name format"); + context.ExitCode = 1; return; } } @@ -503,6 +513,7 @@ private static Command CreateUnpublishSubcommand( catch (ArgumentException ex) { logger.LogError("Input validation failed: {Message}", ex.Message); + context.ExitCode = 1; return; } @@ -517,21 +528,104 @@ private static Command CreateUnpublishSubcommand( } // Call service - var success = await toolingService.UnpublishServerAsync(envId, serverName); + var response = await toolingService.UnpublishServerAsync(envId, serverName); - if (!success) + if (response is null || !response.IsSuccess) { logger.LogError("Failed to unpublish MCP server {ServerName} from environment {EnvId}", serverName, envId); + context.ExitCode = 1; return; } logger.LogInformation("Successfully unpublished MCP server {ServerName} from environment {EnvId}", serverName, envId); - }, envIdOption, serverNameOption, dryRunOption, verboseOption); + // The platform removes the tenant publication but cannot delete Entra app registrations in the + // customer tenant, so it returns any it created for this server (in ManualCleanupRequired) for + // the CLI to clean up here. + await CleanupEntraAppsAsync(logger, graphApiService, response.ManualCleanupRequired?.Apps, serverName); + }); return command; } + /// + /// Best-effort deletion of the Entra app registrations the platform returned from an unpublish. Each + /// delete is independent so one failure does not skip the rest, and every failure is logged with the + /// app id so the user can remove it manually. When Graph is unavailable or the tenant cannot be + /// detected, the apps are listed once for manual cleanup instead. + /// + private static async Task CleanupEntraAppsAsync( + ILogger logger, + GraphApiService? graphApiService, + IReadOnlyList? appsToCleanup, + string serverName) + { + if (appsToCleanup is null || appsToCleanup.Count == 0) + { + return; + } + + // Tenant detection shells out to az, so only attempt it when Graph is available. + var tenantId = graphApiService is null + ? null + : await TenantDetectionHelper.DetectTenantIdAsync(null, logger); + + if (graphApiService is null || string.IsNullOrWhiteSpace(tenantId)) + { + var appList = string.Join( + Environment.NewLine, + appsToCleanup.Select(app => $" - {app.AppName ?? ""} (appId {app.AppId ?? ""})")); + logger.LogWarning( + "The platform could not automatically delete {Count} Entra app registration(s) for server '{ServerName}'. Delete them manually in the Azure portal:{NewLine}{AppList}", + appsToCleanup.Count, serverName, Environment.NewLine, appList); + return; + } + + logger.LogInformation("Cleaning up Entra app registrations for unpublished server '{ServerName}'...", serverName); + + foreach (var app in appsToCleanup) + { + if (string.IsNullOrWhiteSpace(app.AppId)) + { + logger.LogWarning( + "The platform returned Entra app '{AppName}' for cleanup without an appId, so it cannot be deleted automatically. If it exists, delete it manually in the Azure portal.", + app.AppName ?? ""); + continue; + } + + try + { + var objectId = await graphApiService.GetAppObjectIdByAppIdAsync(tenantId, app.AppId); + if (string.IsNullOrWhiteSpace(objectId)) + { + logger.LogWarning( + "Entra app '{AppName}' (appId {AppId}) was not found; it may already be deleted.", + app.AppName ?? "", app.AppId); + continue; + } + + var deleted = await graphApiService.DeleteEntraAppAsync(tenantId, objectId); + if (deleted) + { + logger.LogInformation("Deleted Entra app '{AppName}' (appId {AppId})", app.AppName ?? "", app.AppId); + } + else + { + logger.LogError( + "Failed to delete Entra app '{AppName}' (appId {AppId}). Delete it manually in the Azure portal.", + app.AppName ?? "", app.AppId); + } + } + catch (Exception ex) + { + logger.LogError( + ex, + "Exception deleting Entra app '{AppName}' (appId {AppId}). Delete it manually in the Azure portal.", + app.AppName ?? "", app.AppId); + } + } + } + /// /// Creates the register-external-mcp-server subcommand /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerAppEntry.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerAppEntry.cs new file mode 100644 index 00000000..45276f61 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerAppEntry.cs @@ -0,0 +1,26 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +using System.Text.Json.Serialization; + +namespace Microsoft.Agents.A365.DevTools.Cli.Models; + +/// +/// An Entra app registration associated with a published MCP server that the CLI is responsible for +/// deleting on unpublish. The platform cannot delete app registrations in the customer tenant, so it +/// returns these entries and the CLI performs the deletion using the caller's Graph permissions. +/// +public class McpServerAppEntry +{ + /// + /// Friendly name of the app registration (for logging / manual cleanup guidance). + /// + [JsonPropertyName("AppName")] + public string? AppName { get; set; } + + /// + /// The app (client) id of the Entra app registration. The CLI resolves the underlying application + /// object id from this before calling Graph's application delete. + /// + [JsonPropertyName("AppId")] + public string? AppId { get; set; } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerManualCleanup.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerManualCleanup.cs new file mode 100644 index 00000000..105ba654 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/McpServerManualCleanup.cs @@ -0,0 +1,27 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +using System.Collections.Generic; +using System.Text.Json.Serialization; + +namespace Microsoft.Agents.A365.DevTools.Cli.Models; + +/// +/// Describes resources the unpublish operation could not delete on the caller's behalf and that the +/// caller must remove manually. Mirrors the platform's response contract so any client (not just this +/// CLI) can discover the cleanup responsibility from the response body. +/// +public class McpServerManualCleanup +{ + /// + /// Human-readable explanation of why the listed resources were not deleted automatically and how + /// to remove them. + /// + [JsonPropertyName("Reason")] + public string? Reason { get; set; } + + /// + /// The Entra app registrations the caller must delete manually in their own tenant. + /// + [JsonPropertyName("Apps")] + public List? Apps { get; set; } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs new file mode 100644 index 00000000..8c7ad57b --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/UnpublishMcpServerResponse.cs @@ -0,0 +1,38 @@ +// 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 an MCP server unpublish operation. +/// +public class UnpublishMcpServerResponse +{ + /// + /// Status of the unpublish operation. + /// + [JsonPropertyName("Status")] + public string? Status { get; set; } + + /// + /// Message from the API response. + /// + [JsonPropertyName("Message")] + public string? Message { get; set; } + + /// + /// Resources the platform could not delete in the customer tenant and that the caller must remove + /// manually (with a reason and the affected app registrations). Null when there is nothing for the + /// caller to clean up (for example OOB Dataverse servers or legacy records). + /// + [JsonPropertyName("ManualCleanupRequired")] + public McpServerManualCleanup? ManualCleanupRequired { 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 578b0839..edcd4965 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs @@ -553,7 +553,7 @@ private string BuildProvisionIdentityUrl(string environment, string serverName) } /// - public async Task UnpublishServerAsync( + public async Task UnpublishServerAsync( string environmentId, string serverName, CancellationToken cancellationToken = default) @@ -587,7 +587,7 @@ public async Task UnpublishServerAsync( if (string.IsNullOrWhiteSpace(authToken)) { _logger.LogError("Failed to acquire authentication token"); - return false; + return null; } // Create authenticated HTTP client @@ -600,19 +600,41 @@ public async Task UnpublishServerAsync( using var response = await httpClient.DeleteAsync(endpointUrl, cancellationToken); // Validate response using common helper - var (isSuccess, _) = await ValidateResponseAsync(response, "unpublish MCP server", cancellationToken); + var (isSuccess, responseContent) = await ValidateResponseAsync(response, "unpublish MCP server", cancellationToken); if (!isSuccess) { - return false; + return null; } _logger.LogDebug("Successfully unpublished MCP server"); - return true; + + var unpublishResponse = string.IsNullOrWhiteSpace(responseContent) + ? null + : JsonDeserializationHelper.DeserializeWithDoubleSerialization( + responseContent, _logger); + + // The platform always returns a JSON body on a successful unpublish, so an empty body or one + // that fails to parse means we could not read the response - including any ManualCleanupRequired + // block. The unpublish itself still succeeded, so warn the user to check for leftover Entra app + // registrations rather than silently returning a clean success. + if (unpublishResponse is null) + { + _logger.LogWarning( + "The unpublish for {ServerName} succeeded but its response body was empty or could not be parsed, so any Entra app registrations the platform asked to be cleaned up may have been missed. Review the server's app registrations in the Azure portal and delete any leftovers manually.", + serverName); + return new UnpublishMcpServerResponse + { + Status = "Success", + Message = $"Successfully unpublished {serverName}" + }; + } + + return unpublishResponse; } catch (Exception ex) { _logger.LogError(ex, "Failed to unpublish MCP server {ServerName} from environment {EnvId}", serverName, environmentId); - return false; + return null; } } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs index 8ed1388f..cfa46557 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs @@ -603,6 +603,30 @@ public virtual async Task ApplicationExistsByAppIdAsync( return doc.RootElement.TryGetProperty("value", out var value) && value.GetArrayLength() > 0; } + /// + /// Resolves an application's object ID from its appId (client ID). Returns null when the application + /// cannot be found (for example it was already deleted). Virtual to allow substitution in unit tests. + /// + public virtual async Task GetAppObjectIdByAppIdAsync( + string tenantId, string appId, CancellationToken ct = default) + { + // Validate GUID format to prevent OData injection. + if (!Guid.TryParse(appId, out var validGuid)) + { + _logger.LogWarning("Invalid appId format for application lookup: {AppId}", appId); + return null; + } + + using var doc = await GraphGetAsync( + tenantId, + $"/v1.0/applications?$filter=appId eq '{validGuid:D}'&$select=id&$top=1", + ct); + if (doc == null) return null; + if (!doc.RootElement.TryGetProperty("value", out var value) || value.GetArrayLength() == 0) return null; + if (!value[0].TryGetProperty("id", out var id)) return null; + return id.GetString(); + } + /// /// Looks up the display name of a service principal by its application ID. /// Returns null if the service principal is not found. @@ -2245,42 +2269,6 @@ public virtual async Task UpdateAppPublicClientRedirectUrisAsync( return result; } - /// - /// Looks up an application by its appId (clientId) and returns the object ID. - /// Retries up to 6 times with a 10-second delay to handle replication lag for newly created apps. - /// - public virtual async Task GetAppObjectIdByClientIdAsync( - string tenantId, string clientId, CancellationToken ct = default) - { - const int maxAttempts = 6; - const int delayMs = 10_000; - - for (var attempt = 0; attempt < maxAttempts; attempt++) - { - var response = await GraphGetWithResponseAsync(tenantId, $"/v1.0/applications?$filter=appId eq '{clientId}'&$select=id", ct: ct); - if (response.IsSuccess && response.Json != null) - { - var values = response.Json.RootElement.GetProperty("value"); - if (values.GetArrayLength() > 0) - { - return values[0].GetProperty("id").GetString(); - } - } - else - { - _logger.LogDebug("App {ClientId} query failed: {Code} {Reason} (attempt {Attempt}/{Max})", clientId, response.StatusCode, response.ReasonPhrase, attempt + 1, maxAttempts); - } - - if (attempt < maxAttempts - 1) - { - _logger.LogDebug("App {ClientId} not found yet, retrying in {Delay}s (attempt {Attempt}/{Max})...", clientId, delayMs / 1000, attempt + 1, maxAttempts); - await Task.Delay(delayMs, ct); - } - } - - return null; - } - /// /// Finds an application's object ID by its display name. /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs index 91ba94e6..63afa762 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IAgent365ToolingService.cs @@ -52,8 +52,11 @@ public interface IAgent365ToolingService /// Dataverse environment ID /// MCP server name to unpublish /// Cancellation token - /// True if successful, false otherwise - Task UnpublishServerAsync( + /// + /// The unpublish response (including any Entra app registrations the CLI must delete), or null when + /// the operation failed. + /// + Task UnpublishServerAsync( string environmentId, string serverName, CancellationToken cancellationToken = default); 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 5d9a079d..7df179c2 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 @@ -72,7 +72,8 @@ public async Task AzureCliStyleParameters_AreAcceptedCorrectly(string command, p _mockToolingService.ListServersAsync(Arg.Any()).Returns(new DataverseMcpServersResponse()); _mockToolingService.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any()) .Returns(new PublishMcpServerResponse { Status = "Success" }); - _mockToolingService.UnpublishServerAsync(Arg.Any(), Arg.Any()).Returns(true); + _mockToolingService.UnpublishServerAsync(Arg.Any(), Arg.Any()) + .Returns(new UnpublishMcpServerResponse { Status = "Success" }); var fullCommand = new List { command }; fullCommand.AddRange(args); @@ -336,7 +337,8 @@ public async Task ServiceIntegration_UnpublishCommand_PassesCorrectParameters() var testEnvId = "test-environment-456"; var testServerName = "msdyn_TestServer"; - _mockToolingService.UnpublishServerAsync(testEnvId, testServerName).Returns(true); + _mockToolingService.UnpublishServerAsync(testEnvId, testServerName) + .Returns(new UnpublishMcpServerResponse { Status = "Success" }); // Act var result = await _command.InvokeAsync(new[] @@ -351,6 +353,61 @@ public async Task ServiceIntegration_UnpublishCommand_PassesCorrectParameters() await _mockToolingService.Received(1).UnpublishServerAsync(testEnvId, testServerName); } + [Fact] + public async Task UnpublishCommand_WhenServiceReturnsNull_ReturnsNonZeroExitCode() + { + // A null response means the unpublish failed; the command must log the failure and surface it to + // the shell as a non-zero exit code (not swallow it as success). + var testEnvId = "test-environment-789"; + var testServerName = "msdyn_TestServer"; + + _mockToolingService.UnpublishServerAsync(testEnvId, testServerName) + .Returns((UnpublishMcpServerResponse?)null); + + var result = await _command.InvokeAsync(new[] + { + "unpublish", + "-e", testEnvId, + "-s", testServerName + }); + + result.Should().Be(1); + await _mockToolingService.Received(1).UnpublishServerAsync(testEnvId, testServerName); + } + + [Fact] + public async Task UnpublishCommand_WithAppsToCleanup_NoGraphService_SucceedsWithoutThrow() + { + // The command is created without a GraphApiService (the default), so the returned apps cannot be + // deleted. The cleanup step must degrade gracefully (warn, no throw) rather than fail the command. + var testEnvId = "test-environment-abc"; + var testServerName = "TestCustomServer"; + + _mockToolingService.UnpublishServerAsync(testEnvId, testServerName) + .Returns(new UnpublishMcpServerResponse + { + Status = "Success", + ManualCleanupRequired = new McpServerManualCleanup + { + Reason = "The platform cannot delete Entra app registrations in your tenant.", + Apps = new List + { + new() { AppName = $"{testServerName}-PublicClients", AppId = "11111111-1111-1111-1111-111111111111" }, + }, + }, + }); + + var result = await _command.InvokeAsync(new[] + { + "unpublish", + "-e", testEnvId, + "-s", testServerName + }); + + result.Should().Be(0); + await _mockToolingService.Received(1).UnpublishServerAsync(testEnvId, testServerName); + } + [Fact] public void CommandStructure_HasNoPositionalArguments() {