diff --git a/CHANGELOG.md b/CHANGELOG.md index 94ccb70e..25383e78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g **Option B — CLI** (`a365 setup admin`) has been removed in this release. Use Option A above, or copy the PowerShell instructions printed in the `a365 setup all` summary output. ### Added +- `--connectivity public|private` on `a365 develop-mcp register-external-mcp-server` records whether the created Power Platform connector should bypass environment-level VNet injection, defaulting to `private` (#498). - `a365 develop-mcp grant-agents-access --agent-blueprint-id --mcp-server-name ` reports which agent instances of a blueprint are missing the permission to call a BYO MCP server, and prompts you to select which ones to grant it to (#500). - When more than one Entra application shares the MCP server's name, `a365 develop-mcp grant-agents-access` now lists them all and asks which one to use instead of failing (#500). - `a365 develop-mcp grant-agents-access --help` now lists Microsoft's first-party agent blueprint names and IDs, and the same list is printed when `--agent-blueprint-id` is missing or not a GUID, so you can find the ID without looking it up elsewhere (#500). diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs index 19712bb0..2b3633b2 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -601,6 +601,9 @@ private static Command CreateRegisterExternalMcpServerSubcommand( var descriptionOption = new Option("--description", description: "Server description (required, used in MOS package metadata)"); command.AddOption(descriptionOption); + var connectivityOption = new Option("--connectivity", description: "Whether the remote MCP server is reachable publicly or only inside the environment's VNet: 'public' or 'private' (default). 'public' asks Power Platform to bypass VNet injection on the connector, which takes effect only in environments enabled for it."); + command.AddOption(connectivityOption); + var dryRunOption = new Option("--dry-run", description: "Show what would be done without executing"); command.AddOption(dryRunOption); @@ -628,6 +631,7 @@ private static Command CreateRegisterExternalMcpServerSubcommand( SecretLifetimeMonths: context.ParseResult.GetValueForOption(secretLifetimeMonthsOption), PublisherName: context.ParseResult.GetValueForOption(publisherOption), Description: context.ParseResult.GetValueForOption(descriptionOption), + Connectivity: context.ParseResult.GetValueForOption(connectivityOption), DryRun: context.ParseResult.GetValueForOption(dryRunOption)); var executor = new RegisterCommandExecutor(logger, toolingService, graphApiService); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs index 481324c3..897745e4 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs @@ -32,6 +32,7 @@ internal record RawRegisterArgs( int? SecretLifetimeMonths, string? PublisherName, string? Description, + string? Connectivity, bool DryRun); /// @@ -57,7 +58,7 @@ internal RegisterCommandExecutor( _retryHelper = retryHelper ?? new RetryHelper(logger, maxRetries: 5, baseDelaySeconds: 3); } - private sealed record ResolvedInput + internal sealed record ResolvedInput { public required string ServerName { get; init; } public required string ServerUrl { get; init; } @@ -82,9 +83,10 @@ private sealed record ResolvedInput public string? IdpClientSecret { get; init; } public string? ApiKeyLocation { get; init; } public string? ApiKeyName { get; init; } + public string? Connectivity { get; init; } } - private sealed record EntraAppSet( + internal sealed record EntraAppSet( string A365AppClientId, string A365AppSecret, string A365AppObjectId, @@ -191,7 +193,7 @@ internal async Task ExecuteAsync(RawRegisterArgs args, CancellationToken c return true; } - private async Task ResolveInputsAsync(RawRegisterArgs args) + internal async Task ResolveInputsAsync(RawRegisterArgs args) { var serverName = args.ServerName; var serverUrl = args.ServerUrl; @@ -210,6 +212,7 @@ internal async Task ExecuteAsync(RawRegisterArgs args, CancellationToken c var secretLifetimeMonths = args.SecretLifetimeMonths; var publisherName = args.PublisherName; var serverDescription = args.Description; + var connectivity = args.Connectivity; RegisterExternalMcpServerInput? inputFileData = null; if (!string.IsNullOrWhiteSpace(args.InputFile)) @@ -244,6 +247,7 @@ internal async Task ExecuteAsync(RawRegisterArgs args, CancellationToken c secretLifetimeMonths ??= inputFileData.SecretLifetimeMonths; publisherName ??= inputFileData.PublisherName; serverDescription ??= inputFileData.Description; + connectivity ??= inputFileData.Connectivity; if (inputFileData.ExternalOAuth is not null) { @@ -311,6 +315,26 @@ internal async Task ExecuteAsync(RawRegisterArgs args, CancellationToken c return null; } + // Non-null rather than non-blank: `--connectivity " "` is a mistake, and treating it as + // absent would silently register the server as private, which is the opposite of what + // someone typing the option intends. It would also let a blank CLI value quietly + // override a valid input-file value through the ??= merge above. + if (connectivity is not null) + { + connectivity = connectivity.Trim(); + if (!connectivity.Equals("public", StringComparison.OrdinalIgnoreCase) + && !connectivity.Equals("private", StringComparison.OrdinalIgnoreCase)) + { + // The value may have come from either source, and naming the wrong one sends + // the caller editing a file they never passed. + var source = args.Connectivity is not null ? "--connectivity" : "connectivity in the input file"; + _logger.LogError("{Source} must be 'public' or 'private'. Got: '{Value}'", source, connectivity); + return null; + } + + connectivity = connectivity.ToLowerInvariant(); + } + if (string.IsNullOrWhiteSpace(authType)) { authType = DevelopMcpCommand.InputValidator.PromptAndValidateRequiredInput("Enter authentication type (EntraOAuth, ExternalOAuth, APIKey, or NoAuth): ", "Auth type", 20); @@ -507,6 +531,7 @@ internal async Task ExecuteAsync(RawRegisterArgs args, CancellationToken c IdpClientSecret = idpClientSecret, ApiKeyLocation = apiKeyLocation, ApiKeyName = apiKeyName, + Connectivity = connectivity, }; } @@ -523,6 +548,12 @@ private void DisplayRegistrationSummary(ResolvedInput input) DevelopMcpCommand.WriteLabel(" Auth Type: "); Console.WriteLine(input.AuthType); DevelopMcpCommand.WriteLabel(" Publisher: "); Console.WriteLine(input.PublisherName); DevelopMcpCommand.WriteLabel(" Description: "); Console.WriteLine(input.Description); + DevelopMcpCommand.WriteLabel(" Connectivity: "); Console.WriteLine(input.Connectivity ?? "private (default)"); + if (string.Equals(input.Connectivity, "public", StringComparison.OrdinalIgnoreCase)) + { + Console.WriteLine(" Note: the VNet bypass for 'public' applies only to environments enabled for it."); + Console.WriteLine(" Verify the server is reachable after registration."); + } DevelopMcpCommand.WriteLabel(" Tools:"); Console.WriteLine(); foreach (var tool in input.ToolList) @@ -626,7 +657,7 @@ private void DisplayRegistrationSummary(ResolvedInput input) PublicClientsAppName: publicClients.AppName); } - private static AddMcpServerRequest BuildRequest(ResolvedInput input, EntraAppSet apps) + internal static AddMcpServerRequest BuildRequest(ResolvedInput input, EntraAppSet apps) { AddMcpServerAuthMetadata authMetadata; @@ -685,6 +716,7 @@ private static AddMcpServerRequest BuildRequest(ResolvedInput input, EntraAppSet RemoteServerScopes = input.RemoteScopes, PublisherName = input.PublisherName, Description = input.Description, + Connectivity = input.Connectivity, CopilotClientAppId = apps.PublicClientsClientId, }; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/AddMcpServerRequest.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/AddMcpServerRequest.cs index d0d1363c..3fa56f4d 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/AddMcpServerRequest.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/AddMcpServerRequest.cs @@ -87,6 +87,12 @@ public class AddMcpServerRequest /// [JsonPropertyName("force")] public bool Force { get; set; } + + /// + /// Connectivity of the remote MCP server: "public" or "private". Null means private. + /// + [JsonPropertyName("connectivity")] + public string? Connectivity { get; set; } } /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/RegisterExternalMcpServerInput.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/RegisterExternalMcpServerInput.cs index 265c8979..3c1e7327 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/RegisterExternalMcpServerInput.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/RegisterExternalMcpServerInput.cs @@ -72,6 +72,13 @@ public class RegisterExternalMcpServerInput [JsonPropertyName("secretLifetimeMonths")] public int? SecretLifetimeMonths { get; set; } + /// + /// Whether the remote MCP server is reachable publicly or only inside the environment's VNet: + /// "public" or "private". Defaults to "private" when omitted. Overridden by --connectivity. + /// + [JsonPropertyName("connectivity")] + public string? Connectivity { get; set; } + /// /// External OAuth configuration (required when authType is ExternalOAuth) /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Templates/register-external-mcp-server-sample.json b/src/Microsoft.Agents.A365.DevTools.Cli/Templates/register-external-mcp-server-sample.json index 175c4f76..beea7da6 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Templates/register-external-mcp-server-sample.json +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Templates/register-external-mcp-server-sample.json @@ -18,6 +18,7 @@ "tenantId": null, "serviceTreeId": null, "secretLifetimeMonths": null, + "connectivity": "private", "force": false, "externalOAuth": { "authorizationUrl": null, diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/RegisterCommandExecutorTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/RegisterCommandExecutorTests.cs index 62ec885d..382a265a 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/RegisterCommandExecutorTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/RegisterCommandExecutorTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System.CommandLine; +using System.Text.Json; using FluentAssertions; using Microsoft.Agents.A365.DevTools.Cli.Commands; using Microsoft.Agents.A365.DevTools.Cli.Services; @@ -12,8 +13,8 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; /// -/// Invocation tests for the register-external-mcp-server command's --secret-lifetime-months -/// pre-flight range validation. Exercises the [1, 24] guard in +/// Invocation tests for the register-external-mcp-server command's pre-flight validation of +/// --secret-lifetime-months and --connectivity. Exercises the guards in /// via the full System.CommandLine pipeline so the /// resulting exit code is asserted as the user would observe it. /// @@ -67,4 +68,316 @@ await toolingService.DidNotReceive().LogRegisterUsageAsync( Arg.Any(), Arg.Any()); } -} + + [Theory] + [InlineData("publik")] + [InlineData("vnet")] + [InlineData("Public Internet")] + public async Task RegisterExternalMcpServer_WithInvalidConnectivity_ReturnsExitCode1AndDoesNotCallTooling(string connectivityArg) + { + // Arrange + var logger = Substitute.For(); + var toolingService = Substitute.For(); + var command = DevelopMcpCommand.CreateCommand(logger, toolingService, graphApiService: null); + + var args = new[] + { + "register-external-mcp-server", + "--server-name", "ext_Test", + "--server-url", "https://example.com/mcp", + "--connectivity", connectivityArg, + }; + + // Act + var exitCode = await command.InvokeAsync(args); + + // Assert — exit code surfaces failure as the user would observe it + exitCode.Should().Be(1); + + // Assert — error log names the option and both accepted values + logger.Received().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(state => state != null + && state.ToString()!.Contains("--connectivity") + && state.ToString()!.Contains("public") + && state.ToString()!.Contains("private") + && state.ToString()!.Contains($"Got: '{connectivityArg.Trim()}'")), + Arg.Any(), + Arg.Any>()); + + // Assert — validation short-circuits before any downstream tooling call + await toolingService.DidNotReceive().AddMcpServerAsync( + Arg.Any(), + Arg.Any(), + Arg.Any()); + await toolingService.DidNotReceive().LogRegisterUsageAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()); + } + + [Theory] + [InlineData("public")] + [InlineData("PUBLIC")] + [InlineData("Public ")] + [InlineData(" private")] + public async Task RegisterExternalMcpServer_WithValidConnectivity_PassesConnectivityGuardAndReachesAuthTypeValidation(string connectivityArg) + { + // Arrange — a deliberately invalid --auth-type stops the run at the guard immediately + // after the connectivity guard, so the run terminates without prompting and reaching + // that guard proves connectivity was accepted. + var logger = Substitute.For(); + var toolingService = Substitute.For(); + var command = DevelopMcpCommand.CreateCommand(logger, toolingService, graphApiService: null); + + var args = new[] + { + "register-external-mcp-server", + "--server-name", "ext_Test", + "--server-url", "https://example.com/mcp", + "--connectivity", connectivityArg, + "--auth-type", "NotAnAuthType", + }; + + // Act + var exitCode = await command.InvokeAsync(args); + + // Assert + exitCode.Should().Be(1); + + // Assert — surrounding whitespace and casing are tolerated, so the connectivity guard + // never fires + logger.DidNotReceive().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(state => state != null + && state.ToString()!.Contains("--connectivity")), + Arg.Any(), + Arg.Any>()); + + // Assert — execution reached the next guard, which is what stopped the run + logger.Received().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(state => state != null + && state.ToString()!.Contains("Invalid auth type")), + Arg.Any(), + Arg.Any>()); + } + + // ─────────── Connectivity as it is resolved onto the platform request ─────────── + + /// + /// Everything the command would otherwise prompt for is supplied through an input file, so + /// ResolveInputsAsync runs to completion without touching the console. + /// + private static string WriteInputFile(string? connectivity) + { + var path = Path.Combine(Path.GetTempPath(), $"a365-connectivity-{Guid.NewGuid():N}.json"); + var connectivityLine = connectivity is null + ? string.Empty + : $"\"connectivity\": {System.Text.Json.JsonSerializer.Serialize(connectivity)},"; + File.WriteAllText(path, $$""" + { + "serverName": "ext_Test", + "serverUrl": "https://example.com/mcp", + "authType": "NoAuth", + "publisherName": "Contoso", + "description": "Test server", + {{connectivityLine}} + "tools": [ { "name": "search", "description": "Searches things" } ] + } + """); + return path; + } + + private static RawRegisterArgs FileBackedArgs(string? connectivity, string inputFile) => + new( + ServerName: null, + ServerUrl: null, + AuthType: null, + IdpAuthUrl: null, + IdpTokenUrl: null, + IdpScopes: null, + IdpClientId: null, + IdpClientSecret: null, + ApiKeyLocation: null, + ApiKeyName: null, + ToolsInput: null, + InputFile: inputFile, + RemoteScopes: null, + TenantId: null, + ServiceTreeId: null, + SecretLifetimeMonths: null, + PublisherName: null, + Description: null, + Connectivity: connectivity, + DryRun: true); + + private static RegisterCommandExecutor CreateExecutor(ILogger logger) => + new(logger, Substitute.For(), graphApiService: null); + + private static async Task ResolveAsync( + ILogger logger, string? cliConnectivity, string? fileConnectivity) + { + var inputFile = WriteInputFile(fileConnectivity); + try + { + return await CreateExecutor(logger).ResolveInputsAsync(FileBackedArgs(cliConnectivity, inputFile)); + } + finally + { + File.Delete(inputFile); + } + } + + [Theory] + [InlineData("public", "public")] + [InlineData("PUBLIC", "public")] + [InlineData("Public ", "public")] + [InlineData(" private", "private")] + [InlineData("PrIvAtE", "private")] + public async Task ResolveInputsAsync_NormalisesAnAcceptedConnectivityToLowercase(string supplied, string expected) + { + var resolved = await ResolveAsync(Substitute.For(), supplied, fileConnectivity: null); + + resolved.Should().NotBeNull(); + resolved!.Connectivity.Should().Be(expected); + } + + [Fact] + public async Task ResolveInputsAsync_WhenConnectivityOmittedEverywhere_LeavesItUnsetSoThePlatformDefaultApplies() + { + var resolved = await ResolveAsync(Substitute.For(), cliConnectivity: null, fileConnectivity: null); + + resolved.Should().NotBeNull(); + resolved!.Connectivity.Should().BeNull(); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public async Task ResolveInputsAsync_WhenConnectivitySuppliedButBlank_Rejects(string supplied) + { + var logger = Substitute.For(); + + var resolved = await ResolveAsync(logger, supplied, fileConnectivity: "public"); + + resolved.Should().BeNull(because: "a blank value is a mistake, not a request for the default"); + logger.Received().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(state => state != null && state.ToString()!.Contains("--connectivity")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task ResolveInputsAsync_WhenConnectivityOmittedOnTheCommandLine_TakesTheInputFileValue() + { + var resolved = await ResolveAsync( + Substitute.For(), cliConnectivity: null, fileConnectivity: "PUBLIC"); + + resolved.Should().NotBeNull(); + resolved!.Connectivity.Should().Be("public", because: "the file value is normalised by the same guard"); + } + + [Fact] + public async Task ResolveInputsAsync_WhenConnectivityGivenOnBoth_PrefersTheCommandLine() + { + var resolved = await ResolveAsync( + Substitute.For(), cliConnectivity: "private", fileConnectivity: "public"); + + resolved.Should().NotBeNull(); + resolved!.Connectivity.Should().Be("private"); + } + + [Fact] + public async Task ResolveInputsAsync_WhenTheInputFileConnectivityIsInvalid_NamesTheInputFileNotTheOption() + { + var logger = Substitute.For(); + + var resolved = await ResolveAsync(logger, cliConnectivity: null, fileConnectivity: "publik"); + + resolved.Should().BeNull(); + logger.Received().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(state => state != null + && state.ToString()!.Contains("connectivity in the input file") + && !state.ToString()!.Contains("--connectivity")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task ResolveInputsAsync_WhenTheCommandLineConnectivityIsInvalid_NamesTheOption() + { + var logger = Substitute.For(); + + var resolved = await ResolveAsync(logger, cliConnectivity: "publik", fileConnectivity: null); + + resolved.Should().BeNull(); + logger.Received().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(state => state != null && state.ToString()!.Contains("--connectivity")), + Arg.Any(), + Arg.Any>()); + } + + // ───────────────────── Serialized request property name ──────────────────── + + [Theory] + [InlineData("public")] + [InlineData("private")] + [InlineData(null)] + public void BuildRequest_SerialisesConnectivityUnderTheNameThePlatformReads(string? connectivity) + { + var input = new RegisterCommandExecutor.ResolvedInput + { + ServerName = "ext_Test", + ServerUrl = "https://example.com/mcp", + AuthType = "NoAuth", + IsEntra = false, + IsExternalIdp = false, + IsNoAuth = true, + IsApiKey = false, + ToolList = [], + ToolDescriptions = [], + PublisherName = "Contoso", + Description = "Test server", + DryRun = false, + Connectivity = connectivity, + }; + + var apps = new RegisterCommandExecutor.EntraAppSet( + "a365-client-id", "a365-secret", "a365-object-id", "a365-name", + null, null, null, "remote-proxy-name", null, null, "public-clients-name"); + + var request = RegisterCommandExecutor.BuildRequest(input, apps); + + request.Connectivity.Should().Be(connectivity); + + // The platform binds on the JSON name, so a rename here silently drops the value: the + // request still succeeds and the connector is created private. + var json = JsonSerializer.Serialize(request); + using var doc = JsonDocument.Parse(json); + + if (connectivity == null) + { + // Null may be written or omitted depending on the serializer options in force; either + // way the platform sees no value and its default applies. + if (doc.RootElement.TryGetProperty("connectivity", out var written)) + { + written.ValueKind.Should().Be(JsonValueKind.Null); + } + } + else + { + doc.RootElement.GetProperty("connectivity").GetString().Should().Be(connectivity); + } + } +} \ No newline at end of file