From 6b75d99ff13a4ea19a908d8b13d0fb48d9bc29da Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Mon, 21 Sep 2026 12:58:19 -0700 Subject: [PATCH 1/2] Add --connectivity to register-external-mcp-server Lets the admin say whether the MCP server is internet-reachable or only reachable inside the environment's VNet, which decides whether the connector keeps VNet injection. Validated in the handler rather than via FromAmong so the error message matches the command's other options. The summary warns on 'public' that the bypass depends on environment enablement: Power Platform silently ignores the request for environments not enabled for it and still reports success, so we can report what was asked for but never what took effect. Inert until the platform side ships; an older platform ignores the unknown property. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Commands/DevelopMcpCommand.cs | 4 + .../Commands/RegisterCommandExecutor.cs | 25 +++++ .../Models/AddMcpServerRequest.cs | 6 ++ .../Models/RegisterExternalMcpServerInput.cs | 7 ++ .../register-external-mcp-server-sample.json | 1 + .../Commands/RegisterCommandExecutorTests.cs | 102 +++++++++++++++++- 6 files changed, 143 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs index 941b1d10..ba44d20f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -595,6 +595,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'. Defaults to 'private', which keeps environment-level VNet injection on the connector. 'public' asks Power Platform to bypass that injection; the bypass applies only to environments enabled for it, so verify connectivity afterwards."); + command.AddOption(connectivityOption); + var dryRunOption = new Option("--dry-run", description: "Show what would be done without executing"); command.AddOption(dryRunOption); @@ -622,6 +625,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..b79205f7 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); /// @@ -82,6 +83,7 @@ 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( @@ -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,19 @@ internal async Task ExecuteAsync(RawRegisterArgs args, CancellationToken c return null; } + if (!string.IsNullOrWhiteSpace(connectivity)) + { + connectivity = connectivity.Trim(); + if (!connectivity.Equals("public", StringComparison.OrdinalIgnoreCase) + && !connectivity.Equals("private", StringComparison.OrdinalIgnoreCase)) + { + _logger.LogError("--connectivity must be 'public' or 'private'. Got: {Value}", 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 +524,7 @@ internal async Task ExecuteAsync(RawRegisterArgs args, CancellationToken c IdpClientSecret = idpClientSecret, ApiKeyLocation = apiKeyLocation, ApiKeyName = apiKeyName, + Connectivity = connectivity, }; } @@ -523,6 +541,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) @@ -685,6 +709,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..6ceb7950 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 @@ -12,8 +12,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 +67,102 @@ 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>()); + } } From 428c6f95d9684fa608438cbe7ecd8a892097741c Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Wed, 23 Sep 2026 18:03:07 -0700 Subject: [PATCH 2/2] fix: address review feedback on --connectivity Reject a supplied-but-blank `--connectivity` rather than treating it as absent. The old `IsNullOrWhiteSpace` guard let `--connectivity " "` skip validation and register the server as private -- the opposite of what someone typing the option intends -- and let a blank CLI value quietly beat a valid input-file value through the `??=` merge. Cover the accepted path, not just the rejected one. `ResolveInputsAsync` and `ResolvedInput` become internal (the test assembly already has InternalsVisibleTo) so the normalised value can be asserted where it is resolved, including CLI-over-input-file precedence. Reaching `AddMcpServerAsync` itself is not testable: it sits behind concrete `GraphApiService` Entra app creation. Adds the missing CHANGELOG entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + .../Commands/RegisterCommandExecutor.cs | 12 +- .../Commands/RegisterCommandExecutorTests.cs | 147 +++++++++++++++++- 3 files changed, 154 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59aa369b..73ca2248 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,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. Defaults to `private`; `public` only takes effect in environments enabled for connector-level bypass (#498). - Setup and bootstrap now use Microsoft's first-party Agent 365 CLI application when it is present in your tenant, validating it without changing Microsoft's app registration, and fall back to a tenant-owned "Agent 365 CLI" app when it is not (#489). - Log separator written at the start of each CLI invocation now redacts values for secret-bearing options (e.g. `--idp-client-secret`) so they are not written to the log file in plain text. - Authentication context (tenant and user) is now logged at the `Information` level whenever the resolved sign-in identity changes, giving operators a clear audit trail in the log file of who the CLI is acting as, without exposing credentials. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs index b79205f7..0647d6fd 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs @@ -58,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; } @@ -193,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; @@ -315,13 +315,17 @@ internal async Task ExecuteAsync(RawRegisterArgs args, CancellationToken c return null; } - if (!string.IsNullOrWhiteSpace(connectivity)) + // 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)) { - _logger.LogError("--connectivity must be 'public' or 'private'. Got: {Value}", connectivity); + _logger.LogError("--connectivity must be 'public' or 'private'. Got: '{Value}'", connectivity); return 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 6ceb7950..d721f20e 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 @@ -101,7 +101,7 @@ public async Task RegisterExternalMcpServer_WithInvalidConnectivity_ReturnsExitC && state.ToString()!.Contains("--connectivity") && state.ToString()!.Contains("public") && state.ToString()!.Contains("private") - && state.ToString()!.Contains($"Got: {connectivityArg.Trim()}")), + && state.ToString()!.Contains($"Got: '{connectivityArg.Trim()}'")), Arg.Any(), Arg.Any>()); @@ -165,4 +165,147 @@ public async Task RegisterExternalMcpServer_WithValidConnectivity_PassesConnecti 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_Rejects() + { + 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")), + Arg.Any(), + Arg.Any>()); + } +} \ No newline at end of file