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/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..0647d6fd 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,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( @@ -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,23 @@ 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)) + { + _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 +528,7 @@ internal async Task ExecuteAsync(RawRegisterArgs args, CancellationToken c IdpClientSecret = idpClientSecret, ApiKeyLocation = apiKeyLocation, ApiKeyName = apiKeyName, + Connectivity = connectivity, }; } @@ -523,6 +545,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 +713,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..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 @@ -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,245 @@ 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_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