From 62dd3998be7406f8eabd950d76129fffa8f02f96 Mon Sep 17 00:00:00 2001 From: Deepali Garg Date: Mon, 21 Sep 2026 21:53:30 -0700 Subject: [PATCH 1/5] Provision A365 proxy app on develop-mcp publish Publish now creates the confidential A365 proxy Entra app (app + secret) alongside the PublicClients app and forwards its credentials to the platform, so custom (non-Dataverse) MCP servers get a Power Platform connector created at publish time instead of the platform logging A365ProxyConnectorCreation=SkippedNoCredentials. Mirrors the register flow: proxy app created first (fatal on failure, with self-cleanup), request carries the proxy clientId/secret, proxy redirect URIs are updated post-publish, and rollback deletes both apps. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + .../Commands/PublishCommandExecutor.cs | 78 ++++++- .../Models/PublishMcpServerRequest.cs | 16 ++ .../Models/PublishMcpServerResponse.cs | 16 ++ .../DevelopMcpCommandRegressionTests.cs | 17 ++ .../PublishCommandExecutorDryRunTests.cs | 4 +- .../PublishCommandExecutorEntraAppTests.cs | 213 ++++++++++++++++++ 7 files changed, 339 insertions(+), 6 deletions(-) create mode 100644 src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 59aa369b..ba229c5f 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 +- `develop-mcp publish` now creates the A365 proxy Entra app and sends its credentials to the platform, so custom (non-Dataverse) MCP servers get a Power Platform connector created at publish time. - 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/PublishCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs index c60e3ed0..031357b3 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs @@ -73,7 +73,11 @@ private sealed record ResolvedInput internal sealed record EntraAppSet( string? PublicClientsClientId, string? PublicClientsObjectId, - string PublicClientsAppName); + string PublicClientsAppName, + string A365AppClientId, + string A365AppSecret, + string A365AppObjectId, + string A365AppName); internal async Task ExecuteAsync(RawPublishArgs args, CancellationToken ct = default) { @@ -84,7 +88,7 @@ internal async Task ExecuteAsync(RawPublishArgs args, CancellationToken ct if (input.DryRun) { - _logger.LogInformation("[DRY RUN] Would create Entra app '{PublicClients}' in tenant", $"{input.ServerName}-PublicClients"); + _logger.LogInformation("[DRY RUN] Would create Entra apps '{PublicClients}' and '{A365Proxy}' in tenant", $"{input.ServerName}-PublicClients", $"{input.ServerName}-A365Proxy"); _logger.LogInformation("[DRY RUN] Would call publish endpoint and back-fill PPMI scope on the created app"); return true; } @@ -131,6 +135,8 @@ internal async Task ExecuteAsync(RawPublishArgs args, CancellationToken ct DisplayName = input.DisplayName, PublicClientsAppId = apps.PublicClientsClientId, PublisherName = input.PublisherName, + A365ProxyClientId = apps.A365AppClientId, + A365ProxyClientSecret = apps.A365AppSecret, }; PublishMcpServerResponse? publishResponse; @@ -318,13 +324,24 @@ private void DisplayPublishSummary(ResolvedInput input) { var provisioner = new EntraAppProvisioner(_logger, _graphApiService!, _retryHelper); + // Confidential A365 proxy app + secret. Required for custom (non-Dataverse) servers: the + // platform creates the Power Platform connector only when its credentials are supplied. + var a365ProxyApp = await provisioner.CreateProxyAppAsync( + input.ServerName, tenantId, suffix: "A365Proxy", roleDisplay: "A365 Proxy", + serviceTreeId: null, ct: ct); + if (a365ProxyApp is null) return null; + var publicClients = await provisioner.CreatePublicClientsAppAsync( input.ServerName, tenantId, serviceTreeId: null, warnings, ct); return new EntraAppSet( PublicClientsClientId: publicClients.ClientId, PublicClientsObjectId: publicClients.ObjectId, - PublicClientsAppName: publicClients.AppName); + PublicClientsAppName: publicClients.AppName, + A365AppClientId: a365ProxyApp.ClientId, + A365AppSecret: a365ProxyApp.Secret, + A365AppObjectId: a365ProxyApp.ObjectId, + A365AppName: a365ProxyApp.AppName); } // Best-effort compensating delete for the Entra apps created in CreateEntraAppsAsync, run when @@ -335,7 +352,7 @@ internal async Task RollbackEntraAppsAsync(EntraAppSet apps, string tenantId, Ca { 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); + _logger.LogWarning("Graph API service is unavailable; cannot roll back Entra apps '{PublicClients}' and '{A365Proxy}'. Delete them manually in the Azure portal.", apps.PublicClientsAppName, apps.A365AppName); return; } @@ -346,6 +363,11 @@ internal async Task RollbackEntraAppsAsync(EntraAppSet apps, string tenantId, Ca await DeleteOneAsync(apps.PublicClientsObjectId, apps.PublicClientsClientId, apps.PublicClientsAppName, ct); } + if (!string.IsNullOrWhiteSpace(apps.A365AppObjectId)) + { + await DeleteOneAsync(apps.A365AppObjectId, apps.A365AppClientId, apps.A365AppName, ct); + } + async Task DeleteOneAsync(string objectId, string? clientId, string appName, CancellationToken cancellationToken) { try @@ -430,12 +452,60 @@ private async Task ConfigureEntraAppsAsync( concurrentWarnings.Add(msg); } + // Custom (non-Dataverse) servers get a Power Platform connector whose redirect URI the + // platform returns here. Write it onto the A365 proxy app so the connector's OAuth flow works. + var a365RedirectUri = response.A365ProxyRedirectUri; + if (!string.IsNullOrWhiteSpace(a365RedirectUri)) + { + tasks.Add(UpdateA365RedirectUrisAsync(tenantId, apps, a365RedirectUri, concurrentWarnings, ct)); + } + else + { + var msg = "A365 Proxy redirect URI was not returned by publish. Redirect URI configuration skipped."; + _logger.LogWarning(msg); + concurrentWarnings.Add(msg); + } + await Task.WhenAll(tasks); foreach (var w in concurrentWarnings) warnings.Add(w); } + private async Task UpdateA365RedirectUrisAsync( + string tenantId, EntraAppSet apps, string a365RedirectUri, + System.Collections.Concurrent.ConcurrentBag concurrentWarnings, + CancellationToken ct = default) + { + try + { + var a365TcUri = DevelopMcpCommand.AddTcPrefix(a365RedirectUri); + var a365NonTcUri = DevelopMcpCommand.RemoveTcPrefix(a365RedirectUri); + var a365Uris = DevelopMcpCommand.BuildRedirectUriList(a365RedirectUri, a365TcUri, a365NonTcUri); + _logger.LogDebug("Updating redirect URIs on '{AppName}' ({ObjectId})", apps.A365AppName, apps.A365AppObjectId); + var success = await _retryHelper.ExecuteWithRetryAsync( + async retryCt => await _graphApiService!.UpdateAppRedirectUrisAsync(tenantId, apps.A365AppObjectId, a365Uris, retryCt), + result => !result, + cancellationToken: ct); + if (!success) + { + var msg = $"Failed to update redirect URIs on A365 Proxy app '{apps.A365AppName}' after retries."; + _logger.LogError(msg); + concurrentWarnings.Add(msg); + } + else + { + _logger.LogInformation("Updated redirect URIs on '{AppName}'", apps.A365AppName); + } + } + catch (Exception ex) + { + var msg = $"Failed to update redirect URIs on A365 Proxy app: {ex.Message}"; + _logger.LogError(msg); + concurrentWarnings.Add(msg); + } + } + private async Task AddRequiredResourceAccessAsync( string tenantId, string appObjectId, diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerRequest.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerRequest.cs index ceb92080..b767f6ce 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerRequest.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerRequest.cs @@ -37,4 +37,20 @@ public class PublishMcpServerRequest /// [JsonPropertyName("publisherName")] public string? PublisherName { get; set; } + + /// + /// A365 proxy (confidential) Entra app client id created CLI-side. The platform's v2 publish + /// path creates the Power Platform connector for custom (non-Dataverse) servers only when both + /// this and are supplied; otherwise connector creation is + /// skipped (A365ProxyConnectorCreation=SkippedNoCredentials). + /// + [JsonPropertyName("a365ProxyClientId")] + public string? A365ProxyClientId { get; set; } + + /// + /// Client secret for the A365 proxy Entra app. Paired with so the + /// platform can create the Power Platform connector for custom servers. + /// + [JsonPropertyName("a365ProxyClientSecret")] + public string? A365ProxyClientSecret { 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 2ea1e2b9..e9e05166 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerResponse.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerResponse.cs @@ -47,6 +47,22 @@ public class PublishMcpServerResponse [JsonPropertyName("PublicClientsAppId")] public string? PublicClientsAppId { get; set; } + /// + /// Redirect URI the platform assigns to the A365 proxy connector for custom servers. When + /// present, the CLI writes the tc/non-tc redirect URI list onto the A365 proxy Entra app it + /// created. Emitted PascalCase by the platform, same as . + /// + [JsonPropertyName("A365ProxyRedirectUri")] + public string? A365ProxyRedirectUri { get; set; } + + /// + /// Id of the Power Platform connector the platform created for the custom server, when proxy + /// credentials were supplied. Surfaced for logging/parity; empty when connector creation was + /// skipped. + /// + [JsonPropertyName("A365ProxyConnectorId")] + public string? A365ProxyConnectorId { get; set; } + /// /// Whether the operation was successful. /// 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..5915ce78 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 @@ -155,6 +155,12 @@ public async Task PublishCommand_ForwardsParsedParametersToToolingService() TestTenantId, TestPublicClientsObjectId, Arg.Any(), Arg.Any()) .Returns(Task.FromResult(true)); + // Publish now also creates the confidential A365 proxy app + secret (required so the platform + // creates the Power Platform connector for custom servers). Stub the secret so proxy creation succeeds. + graphApiService.AddAppPasswordAsync( + TestTenantId, Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("a365-proxy-secret")); + // Mock Graph for ConfigureEntraAppsAsync → required-resource-access grant on Public Clients. graphApiService.GetOAuth2PermissionScopeIdAsync( TestTenantId, Arg.Any(), Arg.Any(), Arg.Any()) @@ -224,6 +230,14 @@ public async Task PublishCommand_ForwardsParsedParametersToToolingService() 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."); + capturedRequest.A365ProxyClientId.Should().NotBeNullOrEmpty( + because: "the confidential A365 proxy app's clientId must be forwarded so the platform " + + "creates the Power Platform connector for custom servers instead of logging " + + "SkippedNoCredentials."); + capturedRequest.A365ProxyClientSecret.Should().Be( + "a365-proxy-secret", + because: "the proxy app's secret must be forwarded alongside its clientId; the platform " + + "requires both to create the connector."); } /// @@ -256,6 +270,9 @@ public async Task PublishCommand_ExplicitEmptyPublisherName_SkipsPromptAndForwar graphApiService.UpdateAppPublicClientRedirectUrisAsync( TestTenantId, TestPublicClientsObjectId, Arg.Any(), Arg.Any()) .Returns(Task.FromResult(true)); + graphApiService.AddAppPasswordAsync( + TestTenantId, Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("a365-proxy-secret")); graphApiService.GetOAuth2PermissionScopeIdAsync( TestTenantId, Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult(Guid.NewGuid())); 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 index 1989ea12..6816c19b 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorDryRunTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorDryRunTests.cs @@ -13,12 +13,12 @@ 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. +/// created — the {ServerName}-PublicClients and {ServerName}-A365Proxy apps. /// public class PublishCommandExecutorDryRunTests { /// - /// The dry-run log must (a) name only the Public Clients app — derived from ServerName, + /// The dry-run log must (a) name 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. /// diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs new file mode 100644 index 00000000..4c25b2e4 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs @@ -0,0 +1,213 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Commands; +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; +using NSubstitute; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; + +/// +/// Covers the Entra-app orchestration performs for custom +/// (non-Dataverse) MCP servers: the confidential A365 proxy app must be created and its credentials +/// forwarded to the platform so the Power Platform connector is created at publish time, and both +/// created apps must be rolled back on failure. These invariants are what let the platform stop +/// logging A365ProxyConnectorCreation=SkippedNoCredentials. +/// +/// Tests substitute the concrete (all Entra calls are virtual) and let +/// the real run against it, mirroring the production wiring. +/// +public class PublishCommandExecutorEntraAppTests +{ + private const string TenantId = "00000000-0000-0000-0000-000000000001"; + private const string EnvironmentId = "00000000-0000-0000-0000-000000000000"; + private const string ServerName = "mcp_TestServer"; + + private static RawPublishArgs MakeArgs() => new( + EnvironmentId: EnvironmentId, + ServerName: ServerName, + Alias: "myAlias", + DisplayName: "Test Display", + PublisherName: "Contoso", + Yes: true, + DryRun: false); + + private static PublishCommandExecutor MakeExecutor( + ILogger logger, IAgent365ToolingService tooling, GraphApiService graph) + { + var retry = new RetryHelper(logger, maxRetries: 1, baseDelaySeconds: 0); + return new TestablePublishCommandExecutor(logger, tooling, graph, retry, TenantId); + } + + /// + /// Stubs a successful two-app creation: the A365 proxy app (with a secret) and the Public + /// Clients app. Returns the proxy client id / secret / object id the tests assert on. + /// + private static (string ProxyClientId, string ProxySecret, string ProxyObjectId) ArrangeSuccessfulAppCreation(GraphApiService graph) + { + const string proxyObjectId = "proxy-object-id"; + const string proxyClientId = "proxy-client-id"; + const string proxySecret = "proxy-secret"; + + graph.CreateEntraAppAsync(Arg.Any(), Arg.Is(n => n.EndsWith("-A365Proxy")), Arg.Any(), Arg.Any()) + .Returns((proxyObjectId, proxyClientId)); + graph.CreateEntraAppAsync(Arg.Any(), Arg.Is(n => n.EndsWith("-PublicClients")), Arg.Any(), Arg.Any()) + .Returns(("pc-object-id", "pc-client-id")); + graph.AddAppPasswordAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(proxySecret); + graph.UpdateAppPublicClientRedirectUrisAsync(Arg.Any(), Arg.Any(), Arg.Any>(), Arg.Any()) + .Returns(true); + graph.UpdateAppRedirectUrisAsync(Arg.Any(), Arg.Any(), Arg.Any>(), Arg.Any()) + .Returns(true); + graph.DeleteEntraAppAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(true); + + return (proxyClientId, proxySecret, proxyObjectId); + } + + [Fact] + public async Task ExecuteAsync_WhenProxyAppCreationFails_AbortsPublish_WithoutCallingPlatform() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + // Proxy app creation fails; public-clients creation is never reached because the proxy app + // is mandatory for custom servers. + graph.CreateEntraAppAsync(Arg.Any(), Arg.Is(n => n.EndsWith("-A365Proxy")), Arg.Any(), Arg.Any()) + .Returns(((string, string)?)null); + + var executor = MakeExecutor(logger, tooling, graph); + + var result = await executor.ExecuteAsync(MakeArgs(), CancellationToken.None); + + result.Should().BeFalse("proxy app creation failure must fail the publish for custom servers"); + await tooling.DidNotReceiveWithAnyArgs().PublishServerAsync(default!, default!, default!, default); + } + + [Fact] + public async Task ExecuteAsync_ForwardsProxyCredentials_ToPlatformPublishRequest() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + var (proxyClientId, proxySecret, _) = ArrangeSuccessfulAppCreation(graph); + + PublishMcpServerRequest? capturedRequest = null; + tooling.PublishServerAsync(EnvironmentId, ServerName, Arg.Do(r => capturedRequest = r), Arg.Any()) + .Returns(new PublishMcpServerResponse { Status = "Success" }); + + var executor = MakeExecutor(logger, tooling, graph); + + var result = await executor.ExecuteAsync(MakeArgs(), CancellationToken.None); + + result.Should().BeTrue(); + capturedRequest.Should().NotBeNull(); + capturedRequest!.A365ProxyClientId.Should().Be(proxyClientId, + because: "the platform creates the Power Platform connector only when the proxy app's client id is supplied"); + capturedRequest.A365ProxyClientSecret.Should().Be(proxySecret, + because: "the platform needs the proxy app's secret to create the connector; without it it logs SkippedNoCredentials"); + } + + [Fact] + public async Task ExecuteAsync_WhenProxyRedirectUriReturned_UpdatesProxyAppRedirectUris() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + var (_, _, proxyObjectId) = ArrangeSuccessfulAppCreation(graph); + + tooling.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new PublishMcpServerResponse + { + Status = "Success", + A365ProxyRedirectUri = "https://global.consent.azure-apim.net/redirect", + }); + + var executor = MakeExecutor(logger, tooling, graph); + + var result = await executor.ExecuteAsync(MakeArgs(), CancellationToken.None); + + result.Should().BeTrue(); + await graph.Received(1).UpdateAppRedirectUrisAsync( + TenantId, proxyObjectId, Arg.Any>(), Arg.Any()); + } + + [Fact] + public async Task ExecuteAsync_WhenProxyRedirectUriMissing_WarnsAndSkipsRedirectUpdate() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + ArrangeSuccessfulAppCreation(graph); + + tooling.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new PublishMcpServerResponse { Status = "Success" }); + + var executor = MakeExecutor(logger, tooling, graph); + + var result = await executor.ExecuteAsync(MakeArgs(), CancellationToken.None); + + result.Should().BeTrue(); + await graph.DidNotReceive().UpdateAppRedirectUrisAsync( + Arg.Any(), Arg.Any(), Arg.Any>(), Arg.Any()); + logger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("A365 Proxy redirect URI was not returned")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task RollbackEntraAppsAsync_DeletesBothPublicClientsAndProxyApps() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + graph.DeleteEntraAppAsync(Arg.Any(), Arg.Any(), Arg.Any()).Returns(true); + + var executor = MakeExecutor(logger, tooling, graph); + + var apps = new PublishCommandExecutor.EntraAppSet( + PublicClientsClientId: "pc-client-id", + PublicClientsObjectId: "pc-object-id", + PublicClientsAppName: $"{ServerName}-PublicClients", + A365AppClientId: "proxy-client-id", + A365AppSecret: "proxy-secret", + A365AppObjectId: "proxy-object-id", + A365AppName: $"{ServerName}-A365Proxy"); + + await executor.RollbackEntraAppsAsync(apps, TenantId, CancellationToken.None); + + await graph.Received(1).DeleteEntraAppAsync(TenantId, "pc-object-id", Arg.Any()); + await graph.Received(1).DeleteEntraAppAsync(TenantId, "proxy-object-id", Arg.Any()); + } + + /// + /// Overrides only the tenant-detection seam (which shells out to Azure CLI) so the rest of the + /// executor runs unchanged against the substituted Graph and tooling services. + /// + private sealed class TestablePublishCommandExecutor : PublishCommandExecutor + { + private readonly string _tenantId; + + public TestablePublishCommandExecutor( + ILogger logger, IAgent365ToolingService toolingService, GraphApiService graphApiService, + RetryHelper retryHelper, string tenantId) + : base(logger, toolingService, graphApiService, retryHelper) + { + _tenantId = tenantId; + } + + protected override Task DetectTenantIdAsync() => Task.FromResult(_tenantId); + } +} From 620eda8b78730f23a49dcd821a5c2aa562271dbc Mon Sep 17 00:00:00 2001 From: Deepali Garg Date: Mon, 21 Sep 2026 22:39:16 -0700 Subject: [PATCH 2/5] Grant McpServer resource access on A365 proxy app The platform wires the A365 proxy connector with the proxy app as its OAuth client and McpServerAppId as the resource, so the proxy app must hold the McpServerScope required-resource-access grant or Entra rejects the connector's token request with AADSTS650057. ConfigureEntraAppsAsync previously granted this only on the PublicClients app; now it grants on both the proxy app and the PublicClients app, mirroring register. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Commands/PublishCommandExecutor.cs | 6 +++ .../PublishCommandExecutorEntraAppTests.cs | 45 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs index 031357b3..c0d6b5d0 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs @@ -440,6 +440,12 @@ private async Task ConfigureEntraAppsAsync( if (resourceScopeId.HasValue) { + // The platform wires the A365 proxy connector with the proxy app as its OAuth client and + // McpServerAppId as the resource, so the proxy app must hold this required-resource-access + // grant or Entra rejects the token request (AADSTS650057). Grant it on both the proxy app + // and the Public Clients app, mirroring register. + tasks.Add(AddRequiredResourceAccessAsync(tenantId, apps.A365AppObjectId, apps.A365AppName, resourceAppId!, resourceScopeId.Value, concurrentWarnings, ct)); + if (apps.PublicClientsObjectId != null) { tasks.Add(AddRequiredResourceAccessAsync(tenantId, apps.PublicClientsObjectId, apps.PublicClientsAppName, resourceAppId!, resourceScopeId.Value, concurrentWarnings, ct)); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs index 4c25b2e4..7e212c08 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs @@ -167,6 +167,51 @@ await graph.DidNotReceive().UpdateAppRedirectUrisAsync( Arg.Any>()); } + /// + /// The A365 proxy app is the OAuth client of the platform-created connector, with McpServerAppId + /// as its resource. Without a required-resource-access grant for that resource on the proxy app, + /// Entra rejects the connector's token request (AADSTS650057). The grant must therefore land on + /// BOTH the proxy app and the Public Clients app. + /// + [Fact] + public async Task ExecuteAsync_GrantsMcpServerResourceAccess_OnBothProxyAndPublicClientsApps() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + var (_, _, proxyObjectId) = ArrangeSuccessfulAppCreation(graph); + + const string mcpServerAppId = "1a2a0eb6-0000-0000-0000-000000000000"; + const string mcpServerScope = "Tools.ListInvoke.All"; + var scopeId = Guid.NewGuid(); + + graph.GetOAuth2PermissionScopeIdAsync(TenantId, mcpServerAppId, mcpServerScope, Arg.Any()) + .Returns(scopeId); + graph.AddRequiredResourceAccessAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(true); + + tooling.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new PublishMcpServerResponse + { + Status = "Success", + McpServerAppId = mcpServerAppId, + McpServerScope = mcpServerScope, + }); + + var executor = MakeExecutor(logger, tooling, graph); + + var result = await executor.ExecuteAsync(MakeArgs(), CancellationToken.None); + + result.Should().BeTrue(); + await graph.Received(1).AddRequiredResourceAccessAsync( + TenantId, proxyObjectId, mcpServerAppId, scopeId, Arg.Any()); + await graph.Received(1).AddRequiredResourceAccessAsync( + TenantId, "pc-object-id", mcpServerAppId, scopeId, Arg.Any()); + await graph.Received(2).AddRequiredResourceAccessAsync( + Arg.Any(), Arg.Any(), mcpServerAppId, scopeId, Arg.Any()); + } + [Fact] public async Task RollbackEntraAppsAsync_DeletesBothPublicClientsAndProxyApps() { From 9c97cc0cbc4d2927485d5a2cfc3d74df1e1448c9 Mon Sep 17 00:00:00 2001 From: Deepali Garg Date: Tue, 22 Sep 2026 10:58:17 -0700 Subject: [PATCH 3/5] Redact a365ProxyClientSecret from request logging The publish request now carries the newly created A365 proxy Entra app secret as a365ProxyClientSecret. RedactSecretFields only masked clientApp1Secret/clientApp2Secret/clientSecret, so verbose request-payload logging wrote the live client secret in plaintext. Add a365ProxyClientSecret to the redaction key set with a regression test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Services/Agent365ToolingService.cs | 2 +- .../Agent365ToolingServicePureFunctionTests.cs | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs index 578b0839..d65b5c93 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs @@ -196,7 +196,7 @@ private static void RedactSecretFields(System.Text.Json.Nodes.JsonObject obj) { var secretKeys = new HashSet(StringComparer.OrdinalIgnoreCase) { - "clientApp1Secret", "clientApp2Secret", "clientSecret" + "clientApp1Secret", "clientApp2Secret", "clientSecret", "a365ProxyClientSecret" }; foreach (var key in obj.Select(p => p.Key).ToList()) diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Agent365ToolingServicePureFunctionTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Agent365ToolingServicePureFunctionTests.cs index 86539da0..33ba7c99 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Agent365ToolingServicePureFunctionTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Agent365ToolingServicePureFunctionTests.cs @@ -119,6 +119,19 @@ public void RedactSecretsFromPayload_RedactsClientSecret() result.Should().Contain("myid"); } + [Fact] + public void RedactSecretsFromPayload_RedactsA365ProxyClientSecret() + { + // The publish request serializes the newly created A365 proxy Entra app secret as + // a365ProxyClientSecret; it must be redacted so verbose request logging never writes the + // live client secret in plaintext. + var payload = """{"a365ProxyClientId":"proxy-id","a365ProxyClientSecret":"proxysecret"}"""; + var result = Agent365ToolingService.RedactSecretsFromPayload(payload); + result.Should().NotContain("proxysecret", because: "the A365 proxy client secret must never be logged in plaintext"); + result.Should().Contain("***REDACTED***"); + result.Should().Contain("proxy-id", because: "the non-secret proxy client id is safe to log and aids diagnostics"); + } + [Fact] public void RedactSecretsFromPayload_PreservesNonSecretFields() { From 7fe23fd1ffc0d5faff174830f6afd7253bf19c4e Mon Sep 17 00:00:00 2001 From: Deepali Garg Date: Tue, 29 Sep 2026 12:11:45 -0700 Subject: [PATCH 4/5] Address PR #499 review: gate proxy app, add options, harden cleanup Publish still forwards the A365 proxy app credentials on every call (the CLI can't classify custom vs first-party before the platform does), but now reconciles after publish: when the response shows no connector was created, the unused proxy app is deleted so no orphaned credential lingers. - Post-publish cleanup of the unused proxy app for first-party/Dataverse servers, gated on the platform returning a connector id / redirect URI. - Redirect-URI warning now fires only when a connector was actually created but no URI came back, not on every first-party publish. - Proxy required-resource-access grant applied only when a connector exists; Public Clients grant unchanged. - New --service-tree-id and --secret-lifetime-months options on publish, threaded to both created Entra apps, mirroring register. - Orphaned proxy app is deleted if Public Clients creation throws after the proxy app was created. - Dry-run output now describes proxy creation, permission/redirect config, and cleanup. - CHANGELOG entry references (#499). Tests: proxy grant on both apps only when a connector exists, unused-proxy deletion, orphan-cleanup-on-throw, option flow-through, and updated publish option/dry-run assertions with documented requirement changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- .../Commands/DevelopMcpCommand.cs | 10 +- .../Commands/PublishCommandExecutor.cs | 152 ++++++++++++------ .../Commands/DevelopMcpCommandTests.cs | 23 ++- .../PublishCommandExecutorDryRunTests.cs | 14 +- .../PublishCommandExecutorEntraAppTests.cs | 103 +++++++++++- 6 files changed, 240 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba229c5f..84591d0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +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 -- `develop-mcp publish` now creates the A365 proxy Entra app and sends its credentials to the platform, so custom (non-Dataverse) MCP servers get a Power Platform connector created at publish time. +- `develop-mcp publish` now creates the A365 proxy Entra app and sends its credentials to the platform, so custom (non-Dataverse) MCP servers get a Power Platform connector created at publish time. The proxy app is removed again when the server turns out not to need a connector, and `--service-tree-id` / `--secret-lifetime-months` options are honored for it (#499). - 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..630397cd 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -386,6 +386,12 @@ private static Command CreatePublishSubcommand( 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); + var serviceTreeIdOption = new Option("--service-tree-id", description: "ServiceTree ID for Entra app registration (required in Microsoft corporate tenants)"); + command.AddOption(serviceTreeIdOption); + + var secretLifetimeMonthsOption = new Option(["--secret-lifetime-months", "-l"], description: "Lifetime in months (1-24) for the generated client secret on the A365 proxy Entra app. Default is 2 years. Set a value smaller than the appManagementPolicies cap in your tenant."); + command.AddOption(secretLifetimeMonthsOption); + var yesOption = new Option( ["--yes", "-y"], description: "Skip the interactive 'Proceed with publish? (y/N)' confirmation."); @@ -406,7 +412,9 @@ private static Command CreatePublishSubcommand( DisplayName: context.ParseResult.GetValueForOption(displayNameOption), PublisherName: context.ParseResult.GetValueForOption(publisherNameOption), Yes: context.ParseResult.GetValueForOption(yesOption), - DryRun: context.ParseResult.GetValueForOption(dryRunOption)); + DryRun: context.ParseResult.GetValueForOption(dryRunOption), + ServiceTreeId: context.ParseResult.GetValueForOption(serviceTreeIdOption), + SecretLifetimeMonths: context.ParseResult.GetValueForOption(secretLifetimeMonthsOption)); var executor = new PublishCommandExecutor(logger, toolingService, graphApiService); var success = await executor.ExecuteAsync(args, context.GetCancellationToken()); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs index c0d6b5d0..835bade5 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs @@ -19,7 +19,9 @@ internal record RawPublishArgs( string? DisplayName, string? PublisherName, bool Yes, - bool DryRun); + bool DryRun, + string? ServiceTreeId = null, + int? SecretLifetimeMonths = null); /// /// Orchestrates first-party MCP server publish in one CLI command. The shape mirrors @@ -68,6 +70,14 @@ private sealed record ResolvedInput // 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; } + + // ServiceTree ID stamped onto the Entra apps created here. Required in Microsoft corporate + // tenants; null elsewhere. Applied to both the A365 proxy and Public Clients apps. + public string? ServiceTreeId { get; init; } + + // Optional client-secret lifetime (months) for the A365 proxy app's secret. Null uses Graph's + // default; a smaller value avoids the appManagementPolicies cap failing publish in strict tenants. + public int? SecretLifetimeMonths { get; init; } } internal sealed record EntraAppSet( @@ -89,7 +99,8 @@ internal async Task ExecuteAsync(RawPublishArgs args, CancellationToken ct if (input.DryRun) { _logger.LogInformation("[DRY RUN] Would create Entra apps '{PublicClients}' and '{A365Proxy}' in tenant", $"{input.ServerName}-PublicClients", $"{input.ServerName}-A365Proxy"); - _logger.LogInformation("[DRY RUN] Would call publish endpoint and back-fill PPMI scope on the created app"); + _logger.LogInformation("[DRY RUN] Would call the publish endpoint and forward the A365 proxy app credentials so the platform can create the Power Platform connector for custom (non-Dataverse) servers"); + _logger.LogInformation("[DRY RUN] Would back-fill the PPMI scope on the created apps, add the McpServer API permission and connector redirect URI to the A365 proxy app, and delete the proxy app when the publish response shows no connector was created"); return true; } @@ -282,6 +293,8 @@ internal async Task ExecuteAsync(RawPublishArgs args, CancellationToken ct PublisherName = string.IsNullOrWhiteSpace(publisherName) ? null : publisherName, Yes = args.Yes, DryRun = args.DryRun, + ServiceTreeId = string.IsNullOrWhiteSpace(args.ServiceTreeId) ? null : args.ServiceTreeId, + SecretLifetimeMonths = args.SecretLifetimeMonths, }; } catch (ArgumentException ex) @@ -304,6 +317,10 @@ private void DisplayPublishSummary(ResolvedInput input) 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)"); + if (input.SecretLifetimeMonths is { } lifetime) + { + DevelopMcpCommand.WriteLabel(" Secret Lifetime: "); Console.WriteLine($"{lifetime} month(s)"); + } Console.WriteLine(); } @@ -328,20 +345,36 @@ private void DisplayPublishSummary(ResolvedInput input) // platform creates the Power Platform connector only when its credentials are supplied. var a365ProxyApp = await provisioner.CreateProxyAppAsync( input.ServerName, tenantId, suffix: "A365Proxy", roleDisplay: "A365 Proxy", - serviceTreeId: null, ct: ct); + serviceTreeId: input.ServiceTreeId, lifetimeMonths: input.SecretLifetimeMonths, ct: ct); if (a365ProxyApp is null) return null; - var publicClients = await provisioner.CreatePublicClientsAppAsync( - input.ServerName, tenantId, serviceTreeId: null, warnings, ct); - - return new EntraAppSet( - PublicClientsClientId: publicClients.ClientId, - PublicClientsObjectId: publicClients.ObjectId, - PublicClientsAppName: publicClients.AppName, - A365AppClientId: a365ProxyApp.ClientId, - A365AppSecret: a365ProxyApp.Secret, - A365AppObjectId: a365ProxyApp.ObjectId, - A365AppName: a365ProxyApp.AppName); + // If Public Clients creation throws after the proxy app exists, the proxy app (with its + // secret) is orphaned — RollbackEntraAppsAsync only runs once we have a full EntraAppSet and + // the platform call fails. Clean it up here so a Graph error / throttling / cancellation + // doesn't leak a credential. Use CancellationToken.None for the compensating delete so a + // caller Ctrl+C still removes the orphan. + try + { + var publicClients = await provisioner.CreatePublicClientsAppAsync( + input.ServerName, tenantId, serviceTreeId: input.ServiceTreeId, warnings, ct); + + return new EntraAppSet( + PublicClientsClientId: publicClients.ClientId, + PublicClientsObjectId: publicClients.ObjectId, + PublicClientsAppName: publicClients.AppName, + A365AppClientId: a365ProxyApp.ClientId, + A365AppSecret: a365ProxyApp.Secret, + A365AppObjectId: a365ProxyApp.ObjectId, + A365AppName: a365ProxyApp.AppName); + } + catch (Exception ex) + { + _logger.LogError("Failed to create the Public Clients Entra app after the A365 proxy app was created; deleting the orphaned proxy app '{A365Proxy}'.", a365ProxyApp.AppName); + _logger.LogDebug("Exception details: {Exception}", ex.ToString()); + await TryDeleteEntraAppAsync(tenantId, a365ProxyApp.ObjectId, a365ProxyApp.ClientId, a365ProxyApp.AppName, CancellationToken.None); + if (ex is OperationCanceledException && ct.IsCancellationRequested) throw; + return null; + } } // Best-effort compensating delete for the Entra apps created in CreateEntraAppsAsync, run when @@ -358,40 +391,41 @@ internal async Task RollbackEntraAppsAsync(EntraAppSet apps, string tenantId, Ca _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); - } + await TryDeleteEntraAppAsync(tenantId, apps.PublicClientsObjectId, apps.PublicClientsClientId, apps.PublicClientsAppName, ct); + await TryDeleteEntraAppAsync(tenantId, apps.A365AppObjectId, apps.A365AppClientId, apps.A365AppName, ct); + } - if (!string.IsNullOrWhiteSpace(apps.A365AppObjectId)) + // Best-effort compensating delete for a single Entra app. No-op when the object id is unknown or + // Graph is unavailable. Failures are logged with both clientId and objectId so the user can clean + // up manually; the delete never throws. + private async Task TryDeleteEntraAppAsync(string tenantId, string? objectId, string? clientId, string appName, CancellationToken ct) + { + if (_graphApiService is null || string.IsNullOrWhiteSpace(objectId)) { - await DeleteOneAsync(apps.A365AppObjectId, apps.A365AppClientId, apps.A365AppName, ct); + return; } - async Task DeleteOneAsync(string objectId, string? clientId, string appName, CancellationToken cancellationToken) + try { - try + var deleted = await _graphApiService.DeleteEntraAppAsync(tenantId, objectId, ct); + if (deleted) { - 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); - } + _logger.LogInformation("Rolled back Entra app '{AppName}' (objectId {ObjectId})", appName, objectId); } - catch (Exception ex) + else { _logger.LogError( - ex, - "Exception rolling back Entra app '{AppName}' (clientId {ClientId}, objectId {ObjectId}). Delete it manually in the Azure portal.", + "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( @@ -405,6 +439,19 @@ private async Task ConfigureEntraAppsAsync( var tasks = new List(); var concurrentWarnings = new System.Collections.Concurrent.ConcurrentBag(); + // The proxy app + secret are forwarded on every publish because the CLI can't classify the + // server (custom vs 1p/Dataverse) before the platform does. Only custom servers actually get + // a Power Platform connector; the platform signals that by returning a connector id and/or a + // redirect URI. When neither is present the proxy app is unused, so delete it here rather than + // leave an unused credential in the tenant (the Public Clients app and its PPMI grant remain). + var connectorCreated = !string.IsNullOrWhiteSpace(response.A365ProxyConnectorId) + || !string.IsNullOrWhiteSpace(response.A365ProxyRedirectUri); + if (!connectorCreated) + { + _logger.LogInformation("Publish returned no A365 proxy connector for '{ServerName}' (first-party / Dataverse server); removing the unused A365 proxy app '{A365Proxy}'.", input.ServerName, apps.A365AppName); + await TryDeleteEntraAppAsync(tenantId, apps.A365AppObjectId, apps.A365AppClientId, apps.A365AppName, ct); + } + // 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 @@ -442,9 +489,13 @@ private async Task ConfigureEntraAppsAsync( { // The platform wires the A365 proxy connector with the proxy app as its OAuth client and // McpServerAppId as the resource, so the proxy app must hold this required-resource-access - // grant or Entra rejects the token request (AADSTS650057). Grant it on both the proxy app - // and the Public Clients app, mirroring register. - tasks.Add(AddRequiredResourceAccessAsync(tenantId, apps.A365AppObjectId, apps.A365AppName, resourceAppId!, resourceScopeId.Value, concurrentWarnings, ct)); + // grant or Entra rejects the token request (AADSTS650057). Grant it on the proxy app only + // when a connector was created (otherwise the proxy app was just deleted above); always + // grant it on the Public Clients app, mirroring register. + if (connectorCreated) + { + tasks.Add(AddRequiredResourceAccessAsync(tenantId, apps.A365AppObjectId, apps.A365AppName, resourceAppId!, resourceScopeId.Value, concurrentWarnings, ct)); + } if (apps.PublicClientsObjectId != null) { @@ -460,16 +511,21 @@ private async Task ConfigureEntraAppsAsync( // Custom (non-Dataverse) servers get a Power Platform connector whose redirect URI the // platform returns here. Write it onto the A365 proxy app so the connector's OAuth flow works. - var a365RedirectUri = response.A365ProxyRedirectUri; - if (!string.IsNullOrWhiteSpace(a365RedirectUri)) - { - tasks.Add(UpdateA365RedirectUrisAsync(tenantId, apps, a365RedirectUri, concurrentWarnings, ct)); - } - else + // Only relevant when a connector was created; first-party servers get no connector (and the + // proxy app was already removed), so no redirect URI is expected and none is warned about. + if (connectorCreated) { - var msg = "A365 Proxy redirect URI was not returned by publish. Redirect URI configuration skipped."; - _logger.LogWarning(msg); - concurrentWarnings.Add(msg); + var a365RedirectUri = response.A365ProxyRedirectUri; + if (!string.IsNullOrWhiteSpace(a365RedirectUri)) + { + tasks.Add(UpdateA365RedirectUrisAsync(tenantId, apps, a365RedirectUri, concurrentWarnings, ct)); + } + else + { + var msg = "A365 Proxy connector was created but publish returned no redirect URI. Redirect URI configuration skipped."; + _logger.LogWarning(msg); + concurrentWarnings.Add(msg); + } } await Task.WhenAll(tasks); 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 32d7c3f6..9bac3bca 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 @@ -124,8 +124,10 @@ public void PublishSubcommand_HasCorrectOptionsWithAliases() var options = subcommand.Options.ToList(); // 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. + // session, so publish does not expose --tenant-id. Publish now registers the A365 proxy and + // Public Clients Entra apps in the operator's own tenant (via az login) — which may be a + // ServiceTree-enrolled Microsoft corp tenant — so it exposes --service-tree-id and + // --secret-lifetime-months, mirroring register. var optionNames = options.Select(o => o.Name).ToList(); optionNames.Should().Contain("environment-id"); optionNames.Should().Contain("server-name"); @@ -135,10 +137,16 @@ public void PublishSubcommand_HasCorrectOptionsWithAliases() "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( + optionNames.Should().Contain( "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."); + because: "publish creates Entra app registrations in the operator's own tenant, which may " + + "be ServiceTree-enrolled; those registrations are rejected without a " + + "serviceManagementReference, so --service-tree-id must be available (reviewer request on #499, same as #496)."); + optionNames.Should().Contain( + "secret-lifetime-months", + because: "the A365 proxy app's client secret must fit under the tenant's appManagementPolicies " + + "lifetime cap or publish fails in strict tenants; --secret-lifetime-months lets the " + + "operator set a compliant lifetime, mirroring register."); optionNames.Should().Contain("dry-run"); // Verify critical aliases for Azure CLI compliance @@ -153,6 +161,11 @@ public void PublishSubcommand_HasCorrectOptionsWithAliases() var displayNameOption = options.FirstOrDefault(o => o.Name == "display-name"); displayNameOption!.Aliases.Should().Contain("-d"); + + var secretLifetimeOption = options.FirstOrDefault(o => o.Name == "secret-lifetime-months"); + secretLifetimeOption!.Aliases.Should().Contain( + "-l", + because: "register exposes --secret-lifetime-months as -l; publish must use the same alias for consistency."); } [Fact] 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 index 6816c19b..7795fe4c 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorDryRunTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorDryRunTests.cs @@ -19,11 +19,12 @@ public class PublishCommandExecutorDryRunTests { /// /// The dry-run log must (a) name 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. + /// not Alias — and (b) describe the full post-publish configuration: PPMI-scope back-fill, + /// the A365 proxy app's API permission and redirect URI, and its removal when no connector is + /// created. It must also (c) skip the platform publish call entirely. /// [Fact] - public async Task ExecuteAsync_DryRun_NamesPublicClientsApp_AndBackfillsPpmiScopeOnly() + public async Task ExecuteAsync_DryRun_NamesPublicClientsApp_AndDescribesProxyConfiguration() { var logger = Substitute.For(); var toolingService = Substitute.For(); @@ -58,13 +59,14 @@ public async Task ExecuteAsync_DryRun_NamesPublicClientsApp_AndBackfillsPpmiScop Arg.Any(), Arg.Any>()); - // The back-fill line now mentions only PPMI scope, not redirect URI. + // The back-fill line now also describes the proxy app's permission, redirect URI, and cleanup. logger.Received(1).Log( LogLevel.Information, Arg.Any(), Arg.Is(o => - o.ToString()!.Contains("back-fill PPMI scope") && - !o.ToString()!.Contains("redirect URI")), + o.ToString()!.Contains("PPMI scope") && + o.ToString()!.Contains("A365 proxy app") && + o.ToString()!.Contains("redirect URI")), Arg.Any(), Arg.Any>()); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs index 7e212c08..12f90a8e 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs @@ -115,6 +115,39 @@ public async Task ExecuteAsync_ForwardsProxyCredentials_ToPlatformPublishRequest because: "the platform needs the proxy app's secret to create the connector; without it it logs SkippedNoCredentials"); } + /// + /// ServiceTree-enrolled tenants reject app registrations without a serviceManagementReference, + /// and strict tenants cap secret lifetimes; both the A365 proxy and Public Clients apps must + /// therefore receive --service-tree-id, and the proxy secret must honor + /// --secret-lifetime-months, exactly as the register flow does. + /// + [Fact] + public async Task ExecuteAsync_ForwardsServiceTreeIdAndSecretLifetime_ToEntraAppCreation() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + ArrangeSuccessfulAppCreation(graph); + + tooling.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new PublishMcpServerResponse { Status = "Success" }); + + var args = MakeArgs() with { ServiceTreeId = "st-123", SecretLifetimeMonths = 6 }; + + var executor = MakeExecutor(logger, tooling, graph); + + var result = await executor.ExecuteAsync(args, CancellationToken.None); + + result.Should().BeTrue(); + await graph.Received(1).CreateEntraAppAsync( + TenantId, Arg.Is(n => n.EndsWith("-A365Proxy")), "st-123", Arg.Any()); + await graph.Received(1).CreateEntraAppAsync( + TenantId, Arg.Is(n => n.EndsWith("-PublicClients")), "st-123", Arg.Any()); + await graph.Received(1).AddAppPasswordAsync( + TenantId, "proxy-object-id", Arg.Any(), 6, Arg.Any()); + } + [Fact] public async Task ExecuteAsync_WhenProxyRedirectUriReturned_UpdatesProxyAppRedirectUris() { @@ -141,7 +174,7 @@ await graph.Received(1).UpdateAppRedirectUrisAsync( } [Fact] - public async Task ExecuteAsync_WhenProxyRedirectUriMissing_WarnsAndSkipsRedirectUpdate() + public async Task ExecuteAsync_WhenConnectorCreatedButRedirectUriMissing_WarnsAndSkipsRedirectUpdate() { var logger = Substitute.For(); var tooling = Substitute.For(); @@ -149,8 +182,10 @@ public async Task ExecuteAsync_WhenProxyRedirectUriMissing_WarnsAndSkipsRedirect ArrangeSuccessfulAppCreation(graph); + // Connector was created (id present) but no redirect URI came back — a real anomaly worth a + // warning, unlike the first-party case where no connector is expected at all. tooling.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) - .Returns(new PublishMcpServerResponse { Status = "Success" }); + .Returns(new PublishMcpServerResponse { Status = "Success", A365ProxyConnectorId = "connector-id" }); var executor = MakeExecutor(logger, tooling, graph); @@ -162,11 +197,72 @@ await graph.DidNotReceive().UpdateAppRedirectUrisAsync( logger.Received().Log( LogLevel.Warning, Arg.Any(), - Arg.Is(o => o.ToString()!.Contains("A365 Proxy redirect URI was not returned")), + Arg.Is(o => o.ToString()!.Contains("connector was created but publish returned no redirect URI")), Arg.Any(), Arg.Any>()); } + /// + /// The proxy app + secret are forwarded on every publish because the CLI can't classify the + /// server before the platform does. When the response shows no connector was created (a + /// first-party / Dataverse server), the proxy credential is unused and must be deleted so it + /// doesn't linger in the tenant. The Public Clients app must be left in place. + /// + [Fact] + public async Task ExecuteAsync_WhenNoConnectorCreated_DeletesUnusedProxyApp_AndSkipsRedirectUpdate() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + var (_, _, proxyObjectId) = ArrangeSuccessfulAppCreation(graph); + + // No connector id and no redirect URI => the platform created no connector for this server. + tooling.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new PublishMcpServerResponse { Status = "Success" }); + + var executor = MakeExecutor(logger, tooling, graph); + + var result = await executor.ExecuteAsync(MakeArgs(), CancellationToken.None); + + result.Should().BeTrue(); + await graph.Received(1).DeleteEntraAppAsync(TenantId, proxyObjectId, Arg.Any()); + await graph.DidNotReceive().DeleteEntraAppAsync(TenantId, "pc-object-id", Arg.Any()); + await graph.DidNotReceive().UpdateAppRedirectUrisAsync( + Arg.Any(), Arg.Any(), Arg.Any>(), Arg.Any()); + } + + /// + /// If Public Clients creation throws after the confidential proxy app (with its secret) is + /// created, the proxy app is orphaned unless explicitly cleaned up — the failure predates the + /// full EntraAppSet that the platform-failure rollback path deletes. The executor must + /// delete the proxy app itself and fail the publish without calling the platform. + /// + [Fact] + public async Task ExecuteAsync_WhenPublicClientsCreationThrows_DeletesOrphanedProxyApp_AndAbortsPublish() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + const string proxyObjectId = "proxy-object-id"; + graph.CreateEntraAppAsync(Arg.Any(), Arg.Is(n => n.EndsWith("-A365Proxy")), Arg.Any(), Arg.Any()) + .Returns((proxyObjectId, "proxy-client-id")); + graph.AddAppPasswordAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns("proxy-secret"); + graph.CreateEntraAppAsync(Arg.Any(), Arg.Is(n => n.EndsWith("-PublicClients")), Arg.Any(), Arg.Any()) + .Returns<(string, string)?>(_ => throw new InvalidOperationException("graph throttled")); + graph.DeleteEntraAppAsync(Arg.Any(), Arg.Any(), Arg.Any()).Returns(true); + + var executor = MakeExecutor(logger, tooling, graph); + + var result = await executor.ExecuteAsync(MakeArgs(), CancellationToken.None); + + result.Should().BeFalse("a failure creating the Public Clients app must abort the publish"); + await graph.Received(1).DeleteEntraAppAsync(TenantId, proxyObjectId, Arg.Any()); + await tooling.DidNotReceiveWithAnyArgs().PublishServerAsync(default!, default!, default!, default); + } + /// /// The A365 proxy app is the OAuth client of the platform-created connector, with McpServerAppId /// as its resource. Without a required-resource-access grant for that resource on the proxy app, @@ -197,6 +293,7 @@ public async Task ExecuteAsync_GrantsMcpServerResourceAccess_OnBothProxyAndPubli Status = "Success", McpServerAppId = mcpServerAppId, McpServerScope = mcpServerScope, + A365ProxyConnectorId = "connector-id", }); var executor = MakeExecutor(logger, tooling, graph); From 1cd1e901fb4bf959c561f5eb48c759d8f03a1010 Mon Sep 17 00:00:00 2001 From: Deepali Garg Date: Tue, 29 Sep 2026 16:29:23 -0700 Subject: [PATCH 5/5] Address PR #499 review: validate publish secret lifetime, condense CHANGELOG - Reject --secret-lifetime-months outside 1-24 before any Entra app or platform call, matching register's pre-flight guard (copilot review). - Condense the [Unreleased] entry to one consumer-facing sentence (copilot review). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- .../Commands/PublishCommandExecutor.cs | 7 ++ .../Commands/PublishCommandExecutorTests.cs | 68 +++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 84591d0e..972b26dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +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 -- `develop-mcp publish` now creates the A365 proxy Entra app and sends its credentials to the platform, so custom (non-Dataverse) MCP servers get a Power Platform connector created at publish time. The proxy app is removed again when the server turns out not to need a connector, and `--service-tree-id` / `--secret-lifetime-months` options are honored for it (#499). +- `develop-mcp publish` now creates the Power Platform connector for custom (non-Dataverse) MCP servers at publish time (#499). - 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/PublishCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs index 835bade5..8b0efa93 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs @@ -200,6 +200,13 @@ internal async Task ExecuteAsync(RawPublishArgs args, CancellationToken ct try { + // Reject an out-of-range secret lifetime before prompting or creating apps; register enforces the same 1-24 bound. + if (args.SecretLifetimeMonths is { } lifetime && (lifetime < 1 || lifetime > 24)) + { + _logger.LogError("--secret-lifetime-months must be between 1 and 24 (Graph's maximum is ~2 years). Got: {Value}", lifetime); + return null; + } + var environmentId = args.EnvironmentId; if (string.IsNullOrWhiteSpace(environmentId)) { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorTests.cs new file mode 100644 index 00000000..25d8c5a8 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorTests.cs @@ -0,0 +1,68 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.CommandLine; +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; + +/// +/// Invocation tests for the publish subcommand's --secret-lifetime-months pre-flight range +/// validation. Exercises the [1, 24] guard in via the full +/// System.CommandLine pipeline so the resulting exit code is asserted as the user would observe it. +/// +public class PublishCommandExecutorTests +{ + [Theory] + [InlineData("0")] + [InlineData("25")] + [InlineData("-1")] + [InlineData("48")] + public async Task Publish_WithOutOfRangeSecretLifetimeMonths_ReturnsExitCode1AndDoesNotCallTooling(string lifetimeArg) + { + // Arrange + var logger = Substitute.For(); + var toolingService = Substitute.For(); + var command = DevelopMcpCommand.CreateCommand(logger, toolingService, graphApiService: null); + + var args = new[] + { + "publish", + "--environment-id", "env-123", + "--server-name", "Test_Server", + "--alias", "testalias", + "--display-name", "Test Display", + "--yes", + "--secret-lifetime-months", lifetimeArg, + }; + + // Act + var exitCode = await command.InvokeAsync(args); + + // Assert — exit code surfaces failure as the user would observe it + exitCode.Should().Be(1, because: "an out-of-range secret lifetime must fail publish before any Entra app is created or the platform is called"); + + // Assert — error log names the valid range and the rejected value so the user can recover + logger.Received().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(state => state != null + && state.ToString()!.Contains("--secret-lifetime-months") + && state.ToString()!.Contains("between 1 and 24") + && state.ToString()!.Contains($"Got: {lifetimeArg}")), + Arg.Any(), + Arg.Any>()); + + // Assert — validation short-circuits before any downstream publish call + await toolingService.DidNotReceive().PublishServerAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()); + } +}