diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs index 407fc31b..644b4bc2 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs @@ -317,7 +317,7 @@ private void DisplayPublishSummary(ResolvedInput input) var provisioner = new EntraAppProvisioner(_logger, _graphApiService!, _retryHelper); var publicClients = await provisioner.CreatePublicClientsAppAsync( - input.ServerName, tenantId, serviceTreeId: null, warnings, ct); + input.ServerName, tenantId, serviceTreeId: null, warnings, ct: ct); return new EntraAppSet( PublicClientsClientId: publicClients.ClientId, diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs index 622e23c0..9b3fccc2 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/RegisterCommandExecutor.cs @@ -703,20 +703,25 @@ private async Task ConfigureEntraAppsAsync( } else { - var msg = "A365 Proxy redirect URI was not returned by the server. Redirect URI configuration skipped."; + var msg = "A365 Proxy redirect URI was not returned by the server. Setting consent redirect URIs only."; _logger.LogWarning(msg); concurrentWarnings.Add(msg); + tasks.Add(SetConsentOnlyRedirectUrisAsync(tenantId, apps.A365AppObjectId, apps.A365AppName, concurrentWarnings, ct)); } if (input.IsEntra && !string.IsNullOrWhiteSpace(remoteRedirectUri) && apps.RemoteProxyObjectId != null) { tasks.Add(UpdateRemoteProxyRedirectUrisAsync(tenantId, apps, remoteRedirectUri, concurrentWarnings, ct)); } - else if (input.IsEntra && string.IsNullOrWhiteSpace(remoteRedirectUri)) + else if (input.IsEntra && apps.RemoteProxyObjectId != null) { - var msg = "Remote MCP Proxy redirect URI was not returned by the server. Redirect URI configuration skipped."; - _logger.LogWarning(msg); - concurrentWarnings.Add(msg); + if (string.IsNullOrWhiteSpace(remoteRedirectUri)) + { + var msg = "Remote MCP Proxy redirect URI was not returned by the server. Setting consent redirect URIs only."; + _logger.LogWarning(msg); + concurrentWarnings.Add(msg); + } + tasks.Add(SetConsentOnlyRedirectUrisAsync(tenantId, apps.RemoteProxyObjectId, apps.RemoteProxyAppName, concurrentWarnings, ct)); } else if (input.IsEntra && apps.RemoteProxyObjectId == null) { @@ -783,7 +788,10 @@ private async Task UpdateA365RedirectUrisAsync( { var a365TcUri = DevelopMcpCommand.AddTcPrefix(a365RedirectUri); var a365NonTcUri = DevelopMcpCommand.RemoveTcPrefix(a365RedirectUri); - var a365Uris = DevelopMcpCommand.BuildRedirectUriList(a365RedirectUri, a365TcUri, a365NonTcUri); + var a365Uris = DevelopMcpCommand.BuildRedirectUriList(a365RedirectUri, a365TcUri, a365NonTcUri) + .Concat(EntraAppProvisioner.GetConsentRedirectUris()) + .Distinct(StringComparer.Ordinal) + .ToArray(); _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), @@ -817,7 +825,10 @@ private async Task UpdateRemoteProxyRedirectUrisAsync( { var remoteTcUri = DevelopMcpCommand.AddTcPrefix(remoteRedirectUri); var remoteNonTcUri = DevelopMcpCommand.RemoveTcPrefix(remoteRedirectUri); - var remoteUris = DevelopMcpCommand.BuildRedirectUriList(remoteRedirectUri, remoteTcUri, remoteNonTcUri); + var remoteUris = DevelopMcpCommand.BuildRedirectUriList(remoteRedirectUri, remoteTcUri, remoteNonTcUri) + .Concat(EntraAppProvisioner.GetConsentRedirectUris()) + .Distinct(StringComparer.Ordinal) + .ToArray(); _logger.LogDebug("Updating redirect URIs on '{AppName}' ({ObjectId})", apps.RemoteProxyAppName, apps.RemoteProxyObjectId); var success = await _retryHelper.ExecuteWithRetryAsync( async retryCt => await _graphApiService!.UpdateAppRedirectUrisAsync(tenantId, apps.RemoteProxyObjectId!, remoteUris, retryCt), @@ -842,6 +853,37 @@ private async Task UpdateRemoteProxyRedirectUrisAsync( } } + private async Task SetConsentOnlyRedirectUrisAsync( + string tenantId, string objectId, string appName, + System.Collections.Concurrent.ConcurrentBag concurrentWarnings, + CancellationToken ct = default) + { + try + { + var consentUris = EntraAppProvisioner.GetConsentRedirectUris(); + var success = await _retryHelper.ExecuteWithRetryAsync( + async retryCt => await _graphApiService!.UpdateAppRedirectUrisAsync(tenantId, objectId, consentUris, retryCt), + result => !result, + cancellationToken: ct); + if (!success) + { + var msg = $"Failed to set web redirect URIs on '{appName}' after retries."; + _logger.LogError(msg); + concurrentWarnings.Add(msg); + } + else + { + _logger.LogDebug("Set {Count} web redirect URIs on '{AppName}'", consentUris.Length, appName); + } + } + catch (Exception ex) + { + var msg = $"Failed to set web redirect URIs on '{appName}': {ex.Message}"; + _logger.LogError(msg); + concurrentWarnings.Add(msg); + } + } + private async Task AddRemoteProxyScopePermissionAsync( string tenantId, ResolvedInput input, EntraAppSet apps, System.Collections.Concurrent.ConcurrentBag concurrentWarnings, diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs index 14d705a0..f907bd46 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs @@ -22,6 +22,27 @@ internal class EntraAppProvisioner "http://localhost", ]; + private static readonly string[] ProdConsentRedirectUris = + [ + "https://admin.cloud.microsoft/?ref=tools/consent", + ]; + + internal static string[] GetConsentRedirectUris(string? environment = null) + { + var trimmed = environment?.Trim(); + var resolved = string.IsNullOrEmpty(trimmed) + ? Environment.GetEnvironmentVariable("A365_ENVIRONMENT")?.Trim() ?? "prod" + : trimmed; + var envKey = resolved.ToUpperInvariant(); + var customUris = Environment.GetEnvironmentVariable($"A365_CONSENT_REDIRECT_URIS_{envKey}"); + if (!string.IsNullOrWhiteSpace(customUris)) + { + return customUris.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + } + + return [.. ProdConsentRedirectUris]; + } + private readonly ILogger _logger; private readonly GraphApiService _graphApiService; private readonly RetryHelper _retryHelper; @@ -95,6 +116,30 @@ internal sealed record PublicClientsAppResult(string? ClientId, string? ObjectId return new ProxyAppResult(app.Value.ClientId, secret, app.Value.ObjectId, appName); } + private async Task SetWebConsentRedirectUrisAsync( + string tenantId, string objectId, string appName, string roleDisplay, + string? environment, CancellationToken ct, List? warnings = null) + { + var consentUris = GetConsentRedirectUris(environment); + + var success = await _retryHelper.ExecuteWithRetryAsync( + async retryCt => await _graphApiService.UpdateAppRedirectUrisAsync(tenantId, objectId, consentUris, retryCt), + result => !result, + cancellationToken: ct); + if (success) + { + _logger.LogDebug( + "Set {RedirectUriCount} web redirect URIs on '{AppName}' ({ObjectId}): {RedirectUris}", + consentUris.Length, appName, objectId, string.Join(", ", consentUris)); + } + else + { + var msg = $"Failed to set web redirect URIs on {roleDisplay} app '{appName}' after retries."; + _logger.LogWarning(msg); + warnings?.Add(msg); + } + } + /// /// Best-effort compensating delete for an Entra app that was successfully created but failed /// a follow-up step (secret creation, post-create validation). Without this, partial failures @@ -147,6 +192,7 @@ internal virtual async Task CreatePublicClientsAppAsync( string tenantId, string? serviceTreeId, List warnings, + string? environment = null, CancellationToken ct = default) { var appName = $"{serverName}-PublicClients"; @@ -176,19 +222,21 @@ internal virtual async Task CreatePublicClientsAppAsync( cancellationToken: ct); if (!success) { - var msg = $"Failed to set redirect URIs on Public Clients app '{appName}' after retries."; + var msg = $"Failed to set publicClient redirect URIs on Public Clients app '{appName}' after retries."; _logger.LogError(msg); warnings.Add(msg); } else { _logger.LogDebug( - "Set {RedirectUriCount} redirect URIs on '{AppName}' ({ObjectId}): {RedirectUris}", + "Set {RedirectUriCount} publicClient redirect URIs on '{AppName}' ({ObjectId}): {RedirectUris}", publicClientUris.Length, appName, objectId, string.Join(", ", publicClientUris)); } + + await SetWebConsentRedirectUrisAsync(tenantId, objectId, appName, "Public Clients", environment, ct, warnings); } catch (Exception ex) { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/EntraAppProvisionerTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/EntraAppProvisionerTests.cs index 4b7c6624..10d52cf4 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/EntraAppProvisionerTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/EntraAppProvisionerTests.cs @@ -10,12 +10,16 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; +[CollectionDefinition("EntraAppProvisionerTests", DisableParallelization = true)] +public class EntraAppProvisionerTestsCollection { } + /// /// Tests for . The provisioner absorbed the per-app creation flows /// that previously lived inline in PublishCommandExecutor and RegisterCommandExecutor. These tests /// pin every branch (success + each failure mode) so the executors can compose the provisioner /// without needing their own per-step coverage. /// +[Collection("EntraAppProvisionerTests")] public class EntraAppProvisionerTests { private const string ServerName = "mcp_TestServer"; @@ -136,20 +140,27 @@ public async Task CreateProxyAppAsync_WhenClientIdIsEmpty_ReturnsNullAndLogsErro } [Fact] - public async Task CreatePublicClientsAppAsync_HappyPath_ReturnsIdsAndSetsBrokerAndCanonicalRedirectUris() + public async Task CreatePublicClientsAppAsync_ProdEnvironment_SetsPublicClientAndWebConsentRedirectUris() { - var capturedUris = new List(); + var capturedPublicClientUris = new List(); + var capturedWebUris = new List(); _graph.CreateEntraAppAsync(TenantId, $"{ServerName}-PublicClients", serviceTreeId: "svc-tree-9", Arg.Any()) .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); _graph.UpdateAppPublicClientRedirectUrisAsync( TenantId, AppObjectId, - Arg.Do(uris => capturedUris.Add(uris)), + Arg.Do(uris => capturedPublicClientUris.Add(uris)), + Arg.Any()) + .Returns(Task.FromResult(true)); + _graph.UpdateAppRedirectUrisAsync( + TenantId, + AppObjectId, + Arg.Do(uris => capturedWebUris.Add(uris)), Arg.Any()) .Returns(Task.FromResult(true)); var warnings = new List(); - var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: "svc-tree-9", warnings); + var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: "svc-tree-9", warnings, environment: "prod"); result.Should().NotBeNull(); result.ClientId.Should().Be(AppClientId); @@ -157,9 +168,8 @@ public async Task CreatePublicClientsAppAsync_HappyPath_ReturnsIdsAndSetsBrokerA result.AppName.Should().Be($"{ServerName}-PublicClients"); warnings.Should().BeEmpty(); - capturedUris.Should().ContainSingle(); - var uris = capturedUris[0]; - uris.Should().BeEquivalentTo( + capturedPublicClientUris.Should().ContainSingle(); + capturedPublicClientUris[0].Should().BeEquivalentTo( new[] { $"ms-appx-web://Microsoft.AAD.BrokerPlugin/{AppClientId}", @@ -173,6 +183,78 @@ public async Task CreatePublicClientsAppAsync_HappyPath_ReturnsIdsAndSetsBrokerA "on Windows, the localhost callbacks support MSAL.NET and Copilot CLI flows, " + "and vscode.dev/redirect supports the VS Code web client. Silently " + "adding/removing/reordering entries breaks one of those flows."); + + capturedWebUris.Should().ContainSingle(); + capturedWebUris[0].Should().BeEquivalentTo( + new[] + { + "https://admin.cloud.microsoft/?ref=tools/consent", + }, + opt => opt.WithStrictOrdering(), + because: "Prod consent redirect URIs are required by the admin.cloud.microsoft " + + "consent portal. Changing these URIs breaks the admin consent flow."); + } + + [Fact] + public void GetConsentRedirectUris_UsesEnvVarOverride() + { + const string envKey = "A365_CONSENT_REDIRECT_URIS_PREPROD"; + var prev = Environment.GetEnvironmentVariable(envKey); + try + { + Environment.SetEnvironmentVariable(envKey, "https://custom1.example.com/consent, https://custom2.example.com/consent"); + var uris = EntraAppProvisioner.GetConsentRedirectUris("preprod"); + uris.Should().BeEquivalentTo( + new[] { "https://custom1.example.com/consent", "https://custom2.example.com/consent" }, + opt => opt.WithStrictOrdering(), + because: "Non-prod consent redirect URIs are read from A365_CONSENT_REDIRECT_URIS_{ENV} " + + "to avoid leaking internal URLs in source code."); + } + finally + { + Environment.SetEnvironmentVariable(envKey, prev); + } + } + + [Fact] + public void GetConsentRedirectUris_TrimsWhitespaceFromEnvironment() + { + const string envKey = "A365_CONSENT_REDIRECT_URIS_TEST"; + var prev = Environment.GetEnvironmentVariable(envKey); + try + { + Environment.SetEnvironmentVariable(envKey, "https://trimmed.example.com/consent"); + var uris = EntraAppProvisioner.GetConsentRedirectUris(" test "); + uris.Should().BeEquivalentTo( + new[] { "https://trimmed.example.com/consent" }, + because: "Environment values with leading/trailing whitespace (common in " + + "shell/env var setups) must resolve to the correct env var key."); + } + finally + { + Environment.SetEnvironmentVariable(envKey, prev); + } + } + + [Fact] + public void GetConsentRedirectUris_FallsToProdWhenNoEnvVar() + { + const string envKey = "A365_CONSENT_REDIRECT_URIS_PREPROD"; + var prev = Environment.GetEnvironmentVariable(envKey); + try + { + Environment.SetEnvironmentVariable(envKey, null); + var uris = EntraAppProvisioner.GetConsentRedirectUris("preprod"); + uris.Should().BeEquivalentTo( + new[] { "https://admin.cloud.microsoft/?ref=tools/consent" }, + opt => opt.WithStrictOrdering(), + because: "When no env var override is set, all environments fall back to the " + + "prod consent redirect URI."); + } + finally + { + Environment.SetEnvironmentVariable(envKey, prev); + } } [Fact] @@ -194,13 +276,16 @@ await _graph.DidNotReceive().UpdateAppPublicClientRedirectUrisAsync( } [Fact] - public async Task CreatePublicClientsAppAsync_WhenRedirectUriUpdateReturnsFalse_ReturnsIdsAndAppendsRetryWarning() + public async Task CreatePublicClientsAppAsync_WhenPublicClientRedirectUriUpdateReturnsFalse_AppendsWarning() { _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); _graph.UpdateAppPublicClientRedirectUrisAsync( TenantId, AppObjectId, Arg.Any(), Arg.Any()) .Returns(Task.FromResult(false)); + _graph.UpdateAppRedirectUrisAsync( + TenantId, AppObjectId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); var warnings = new List(); var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: null, warnings); @@ -208,7 +293,26 @@ public async Task CreatePublicClientsAppAsync_WhenRedirectUriUpdateReturnsFalse_ result.ClientId.Should().Be(AppClientId); result.ObjectId.Should().Be(AppObjectId); result.AppName.Should().Be($"{ServerName}-PublicClients"); - warnings.Should().ContainSingle().Which.Should().Be($"Failed to set redirect URIs on Public Clients app '{ServerName}-PublicClients' after retries."); + warnings.Should().ContainSingle().Which.Should().Be($"Failed to set publicClient redirect URIs on Public Clients app '{ServerName}-PublicClients' after retries."); + } + + [Fact] + public async Task CreatePublicClientsAppAsync_WhenWebRedirectUriUpdateReturnsFalse_AppendsWarning() + { + _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.UpdateAppPublicClientRedirectUrisAsync( + TenantId, AppObjectId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); + _graph.UpdateAppRedirectUrisAsync( + TenantId, AppObjectId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(false)); + + var warnings = new List(); + var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: null, warnings); + + result.ClientId.Should().Be(AppClientId); + warnings.Should().ContainSingle().Which.Should().Be($"Failed to set web redirect URIs on Public Clients app '{ServerName}-PublicClients' after retries."); } [Fact] @@ -228,4 +332,24 @@ public async Task CreatePublicClientsAppAsync_WhenRedirectUriUpdateThrows_Return result.AppName.Should().Be($"{ServerName}-PublicClients"); warnings.Should().ContainSingle().Which.Should().Be("Failed to set redirect URIs on Public Clients app: Graph blew up"); } + + [Fact] + public async Task CreatePublicClientsAppAsync_WhenWebRedirectUriUpdateThrows_ReturnsIdsAndAppendsExceptionWarning() + { + _graph.CreateEntraAppAsync(TenantId, Arg.Any(), serviceTreeId: Arg.Any(), Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.UpdateAppPublicClientRedirectUrisAsync( + TenantId, AppObjectId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); + _graph.UpdateAppRedirectUrisAsync( + TenantId, AppObjectId, Arg.Any(), Arg.Any()) + .Returns>(_ => throw new InvalidOperationException("Graph consent update failed")); + + var warnings = new List(); + var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: null, warnings); + + result.ClientId.Should().Be(AppClientId); + result.ObjectId.Should().Be(AppObjectId); + warnings.Should().ContainSingle().Which.Should().Be("Failed to set redirect URIs on Public Clients app: Graph consent update failed"); + } }