diff --git a/CHANGELOG.md b/CHANGELOG.md index 9367e48d..ec74be7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,9 @@ Blueprint agents that export telemetry through the app-only S2S endpoint don't n - `--secret-lifetime-months ` option (and matching `secretLifetimeMonths` field in the `--input-file` JSON) on `develop-mcp register-external-mcp-server` — controls the lifetime of the client secrets created on the A365Proxy and RemoteProxy Entra apps. Valid range `1-24`; omit to use the Graph default (~2 years). Calendar-aware (uses `DateTimeOffset.AddMonths`, so Jan 31 + 1 month → Feb 28/29). Added so tenants with an `appManagementPolicies` cap on client-secret lifetime — previously a hard failure inside `CreateEntraAppsAsync` with a generic "Failed to create secret" message — can fit registration inside their tenant's policy. When Graph rejects the requested (or default) lifetime with a tenant-policy error, the CLI now emits an actionable error naming the flag and the attempted value (e.g. `Tenant Entra ID policy rejected the requested 12-month lifetime ... Pass --secret-lifetime-months N with a smaller value (e.g. --secret-lifetime-months 3) that fits inside your tenant's appManagementPolicies cap.`) instead of the previous generic failure. - `--publisher-name` / `-p` option on `develop-mcp publish` — sets the publisher name written into the published MCP server's package metadata. Required for custom (user-created) MCP servers; ignored for 1p Microsoft-owned servers (e.g. `msdyn_DataverseMCPServer`), which always publish as "Microsoft". Prompted interactively when omitted. - `--yes` / `-y` option on `develop-mcp publish` — skips the interactive "Proceed with publish? (y/N)" confirmation. +- `a365 develop-mcp publish` now provisions an A365 proxy Entra app for custom (non-Dataverse) MCP servers and forwards its credentials so the Power Platform connector is created at publish time; first-party Dataverse servers skip it (#499). +- `--service-tree-id` option on `a365 develop-mcp publish` — stamps the ServiceTree ID on the Entra apps publish creates, required in Microsoft corporate tenants (#499). +- `--secret-lifetime-months ` / `-l` option on `a365 develop-mcp publish` — caps the A365 proxy app's client-secret lifetime (1-24 months) for tenants with an `appManagementPolicies` cap (#499). - `a365 develop get-token --device-code` — forces device code auth for Microsoft Graph scopes the Windows WAM broker rejects (e.g. Exchange `MailboxSettings.ReadWrite`, `ExchangeMessageTrace.Read.All`). ### Fixed @@ -168,7 +171,7 @@ Blueprint agents that export telemetry through the app-only S2S endpoint don't n - **`a365 config permissions` removed** — replace with `a365 setup permissions custom --resource-app-id --scopes `. - **`--config`/`-c` option removed from all commands** — config file is now always resolved from the current directory (`a365.config.json`). Scripts passing `--config ` will receive a parse error; change directory before running the CLI instead. - **`--tenant-id` / `-t` removed from `a365 develop-mcp register-external-mcp-server`** — the tenant is now auto-detected from the current `az login` session. Scripts passing `-t ` / `--tenant-id ` will receive a System.CommandLine parse error; run `az login --tenant ` (or `az account set --subscription `) to target a specific tenant instead. -- **`a365 develop-mcp publish` now creates a `-PublicClients` Entra app registration in your tenant** — the publish orchestration runs CLI-side, so after each publish you will see a new app registration named `-PublicClients` in your tenant's Entra ID. These are created by the CLI; clean them up with the same name if you unpublish. +- **`a365 develop-mcp publish` now creates Entra app registrations in your tenant** — every publish creates a `-PublicClients` app, and publishing a custom (non-Dataverse) server also creates a `-A365Proxy` app (first-party Dataverse servers skip the proxy app). These are created by the CLI; clean them up with the same names if you unpublish (#499). - **`a365 develop-mcp publish` now requires the `Application.ReadWrite.All` Microsoft Graph permission** — needed to create the Entra app registration above. Running publish with only read-only Graph permissions will fail. Grant `Application.ReadWrite.All` to the account (or app) running the CLI before publishing. - **`--agent-instance-only` renamed to `--agent-registration-only`** on `a365 setup all` — update any scripts using the old flag name. - **`setup permissions custom --resource-app-id --scopes` applies permissions directly to Entra ID** — unlike the former `a365 config permissions` which only wrote to `a365.config.json`, this inline mode immediately mutates the live blueprint in Entra and cannot be undone by editing a config file. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs index 19712bb0..652f1d31 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs @@ -392,6 +392,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."); @@ -412,7 +418,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 c60e3ed0..a2bbda63 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using Microsoft.Agents.A365.DevTools.Cli.Constants; using Microsoft.Agents.A365.DevTools.Cli.Helpers; using Microsoft.Agents.A365.DevTools.Cli.Models; using Microsoft.Agents.A365.DevTools.Cli.Services; @@ -19,7 +20,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,12 +71,26 @@ 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; } } + // Entra apps created for one publish. A365App* are null for first-party Dataverse servers, which + // need no proxy app (the platform fronts them with its own first-party app). 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,8 +101,16 @@ 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 call publish endpoint and back-fill PPMI scope on the created app"); + if (FirstPartyMcpServers.IsOobDataverseServer(input.ServerName)) + { + _logger.LogInformation("[DRY RUN] '{ServerName}' is a first-party Dataverse MCP server; would create only the Public Clients Entra app '{PublicClients}' (no A365 proxy app or secret needed, since the platform fronts it with its own app)", input.ServerName, $"{input.ServerName}-PublicClients"); + _logger.LogInformation("[DRY RUN] Would call the publish endpoint and back-fill the PPMI scope on the Public Clients app; no Power Platform connector is created for first-party Dataverse servers"); + return true; + } + + _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 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; } @@ -123,7 +148,13 @@ internal async Task ExecuteAsync(RawPublishArgs args, CancellationToken ct var apps = await CreateEntraAppsAsync(input, tenantId, warnings, ct); if (apps is null) return false; - ct.ThrowIfCancellationRequested(); + if (ct.IsCancellationRequested) + { + // Both apps exist but no platform call has happened yet; roll them back before surfacing + // the cancellation so the confidential proxy app and its live secret aren't leaked. + await RollbackEntraAppsAsync(apps, tenantId, ct); + ct.ThrowIfCancellationRequested(); + } var request = new PublishMcpServerRequest { @@ -131,6 +162,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; @@ -183,6 +216,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)) { @@ -276,6 +316,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) @@ -298,6 +340,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(); } @@ -318,13 +364,76 @@ private void DisplayPublishSummary(ResolvedInput input) { var provisioner = new EntraAppProvisioner(_logger, _graphApiService!, _retryHelper); - var publicClients = await provisioner.CreatePublicClientsAppAsync( - input.ServerName, tenantId, serviceTreeId: null, warnings, ct); + // Classify the server by name up front. First-party Dataverse servers are fronted by the + // platform's own Entra app, so they need no A365 proxy app/secret/connector; creating one + // would only leave an unused credential to reconcile away. Custom (non-Dataverse) servers + // get the proxy app whose credentials the platform uses to create the Power Platform + // connector. If this name list drifts from the platform's, publish still reconciles away an + // unused proxy app after the response (see ConfigureEntraAppsAsync). + var isOob = FirstPartyMcpServers.IsOobDataverseServer(input.ServerName); + + EntraAppProvisioner.ProxyAppResult? a365ProxyApp = null; + if (isOob) + { + _logger.LogInformation("'{ServerName}' is a first-party Dataverse MCP server; skipping A365 proxy app creation (the platform fronts it with its own app).", input.ServerName); + } + else + { + try + { + a365ProxyApp = await provisioner.CreateProxyAppAsync( + input.ServerName, tenantId, suffix: "A365Proxy", roleDisplay: "A365 Proxy", + serviceTreeId: input.ServiceTreeId, lifetimeMonths: input.SecretLifetimeMonths, ct: ct); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + catch (Exception ex) + { + _logger.LogError("Failed to create the A365 proxy Entra app for '{ServerName}'. Run with -v for details.", input.ServerName); + _logger.LogDebug("Exception details: {Exception}", ex.ToString()); + return null; + } - return new EntraAppSet( - PublicClientsClientId: publicClients.ClientId, - PublicClientsObjectId: publicClients.ObjectId, - PublicClientsAppName: publicClients.AppName); + if (a365ProxyApp is null) return null; + } + + // 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 (cancellation-independent) so a Graph error / + // throttling / cancellation doesn't leak a credential. + 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) + { + if (a365ProxyApp is not null) + { + _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); + } + else + { + _logger.LogError("Failed to create the Public Clients Entra app for '{ServerName}'. Run with -v for details.", input.ServerName); + _logger.LogDebug("Exception details: {Exception}", ex.ToString()); + } + + if (ex is OperationCanceledException && ct.IsCancellationRequested) throw; + return null; + } } // Best-effort compensating delete for the Entra apps created in CreateEntraAppsAsync, run when @@ -335,40 +444,52 @@ 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; } _logger.LogInformation("Rolling back Entra app registrations created for failed publish..."); - if (!string.IsNullOrWhiteSpace(apps.PublicClientsObjectId)) + // Deletes are intentionally cancellation-independent: rollback runs because the publish + // failed (often due to the same cancellation), so binding cleanup to the caller's token + // would leave the just-created apps (and the proxy secret) orphaned in the tenant. The ct + // parameter is retained for call-site symmetry only. + await TryDeleteEntraAppAsync(tenantId, apps.PublicClientsObjectId, apps.PublicClientsClientId, apps.PublicClientsAppName); + await TryDeleteEntraAppAsync(tenantId, apps.A365AppObjectId, apps.A365AppClientId, apps.A365AppName); + } + + // Best-effort compensating delete for a single Entra app. Returns true when there was nothing to + // delete (unknown object id / Graph unavailable) or the delete succeeded, false when a delete was + // attempted but failed so callers can surface a reconcile warning. Deletes with + // CancellationToken.None so cleanup still runs after a cancelled operation; never throws. + private async Task TryDeleteEntraAppAsync(string tenantId, string? objectId, string? clientId, string? appName, string successVerb = "Rolled back") + { + if (_graphApiService is null || string.IsNullOrWhiteSpace(objectId)) { - await DeleteOneAsync(apps.PublicClientsObjectId, apps.PublicClientsClientId, apps.PublicClientsAppName, ct); + return true; } - async Task DeleteOneAsync(string objectId, string? clientId, string appName, CancellationToken cancellationToken) + try { - try - { - 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); - } - } - catch (Exception ex) + var deleted = await _graphApiService.DeleteEntraAppAsync(tenantId, objectId, CancellationToken.None); + if (deleted) { - _logger.LogError( - ex, - "Exception rolling back Entra app '{AppName}' (clientId {ClientId}, objectId {ObjectId}). Delete it manually in the Azure portal.", - appName, clientId ?? "", objectId); + _logger.LogInformation("{SuccessVerb} Entra app '{AppName}' (objectId {ObjectId})", successVerb, appName ?? "", objectId); + return true; } + + _logger.LogError( + "Failed to delete Entra app '{AppName}' (clientId {ClientId}, objectId {ObjectId}). Delete it manually in the Azure portal.", + appName ?? "", clientId ?? "", objectId); + return false; + } + catch (Exception ex) + { + _logger.LogError( + ex, + "Exception deleting Entra app '{AppName}' (clientId {ClientId}, objectId {ObjectId}). Delete it manually in the Azure portal.", + appName ?? "", clientId ?? "", objectId); + return false; } } @@ -383,6 +504,40 @@ private async Task ConfigureEntraAppsAsync( var tasks = new List(); var concurrentWarnings = new System.Collections.Concurrent.ConcurrentBag(); + // Only custom servers get a Power Platform connector; the platform signals that with a connector + // id and/or a redirect URI. A classified first-party Dataverse server created no proxy app + // (A365AppObjectId is null), so there's nothing to reconcile here. + var connectorCreated = !string.IsNullOrWhiteSpace(response.A365ProxyConnectorId) + || !string.IsNullOrWhiteSpace(response.A365ProxyRedirectUri); + // A real v2 publish payload always carries McpServerAppId (the platform falls back to its own + // app id). A success response without it is the tooling layer's placeholder for a 2xx with an + // empty or undeserializable body, which proves nothing about whether the connector was created. + var hasPlatformPayload = !string.IsNullOrWhiteSpace(response.McpServerAppId); + var proxyObjectId = apps.A365AppObjectId; + if (!connectorCreated && !string.IsNullOrWhiteSpace(proxyObjectId)) + { + if (hasPlatformPayload) + { + // Real payload reporting no connector: the platform treats this server as first-party + // (the CLI's name classification drifted), so the proxy credential is unused - delete it. + _logger.LogInformation("Publish returned no A365 proxy connector for '{ServerName}'; removing the unused A365 proxy app '{A365Proxy}'.", input.ServerName, apps.A365AppName); + var removed = await TryDeleteEntraAppAsync(tenantId, proxyObjectId, apps.A365AppClientId, apps.A365AppName, successVerb: "Removed unused"); + if (!removed) + { + warnings.Add($"Unused A365 proxy app '{apps.A365AppName}' (clientId {apps.A365AppClientId ?? ""}) could not be deleted automatically. Delete it manually in the Azure portal."); + } + } + else + { + // Empty/undeserializable 2xx body: the proxy credentials already went to the platform, + // which may have used them to create the connector, so keep the app rather than orphan + // that connector's OAuth client. Name it so the user can verify and remove it if unused. + var msg = $"Publish for '{input.ServerName}' returned no platform metadata, so A365 proxy connector creation could not be confirmed. Keeping the A365 proxy app '{apps.A365AppName}' (clientId {apps.A365AppClientId ?? ""}); if no connector was created, delete it manually in the Azure portal."; + _logger.LogWarning(msg); + warnings.Add(msg); + } + } + // 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 @@ -418,6 +573,16 @@ 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 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 && !string.IsNullOrWhiteSpace(proxyObjectId)) + { + tasks.Add(AddRequiredResourceAccessAsync(tenantId, proxyObjectId, apps.A365AppName ?? proxyObjectId, resourceAppId!, resourceScopeId.Value, concurrentWarnings, ct)); + } + if (apps.PublicClientsObjectId != null) { tasks.Add(AddRequiredResourceAccessAsync(tenantId, apps.PublicClientsObjectId, apps.PublicClientsAppName, resourceAppId!, resourceScopeId.Value, concurrentWarnings, ct)); @@ -430,12 +595,71 @@ 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. + // 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 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); 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) + { + var a365ObjectId = apps.A365AppObjectId; + if (string.IsNullOrWhiteSpace(a365ObjectId)) + { + return; + } + + 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, a365ObjectId); + var success = await _retryHelper.ExecuteWithRetryAsync( + async retryCt => await _graphApiService!.UpdateAppRedirectUrisAsync(tenantId, a365ObjectId, 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/Constants/FirstPartyMcpServers.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/FirstPartyMcpServers.cs new file mode 100644 index 00000000..8bb8888b --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/FirstPartyMcpServers.cs @@ -0,0 +1,37 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +namespace Microsoft.Agents.A365.DevTools.Cli.Constants; + +/// +/// Known out-of-box (first-party) Dataverse MCP server names, mirrored by name from the MCP +/// platform's OOBDataverseServerNamesToScopeMapping (the source of truth in bap-microsoft/MCP-Platform). +/// The platform fronts these with its own first-party Entra app, so publishing them needs no A365 +/// proxy app, secret, or Power Platform connector. Matched exactly (case-insensitive), not by prefix: +/// the platform keys off exact membership, and a custom server merely named msdyn_* is treated +/// as custom. If the lists drift, publish still creates then reconciles away an unused proxy app, so +/// the mismatch costs a round-trip rather than breaking the connector. +/// +internal static class FirstPartyMcpServers +{ + private static readonly HashSet OobDataverseServerNames = new(StringComparer.OrdinalIgnoreCase) + { + "msdyn_SalesMCPServer", + "msdyn_ServiceMCPServer", + "msdyn_ERPAnalyticsMCPServer", + "msdyn_D365ContactCenterAdminMCPServer", + "msdyn_ContactCenterMCPServer", + "MCP_DataverseMCPServer", + "msdyn_DataverseMCPServer", + "msdyn_DataversePreviewMCPServer", + "msdyn_CIMCPServer", + "msdyn_FnOMCPServer", + }; + + /// + /// Returns true when is a known out-of-box Dataverse MCP server + /// the platform fronts with its own first-party app (so no A365 proxy app/connector is needed). + /// + internal static bool IsOobDataverseServer(string? serverName) => + !string.IsNullOrWhiteSpace(serverName) && OobDataverseServerNames.Contains(serverName); +} 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/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs index 9a47768f..b6b7e6cd 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Agent365ToolingService.cs @@ -239,7 +239,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/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs index 14d705a0..dd9c632a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs @@ -76,23 +76,36 @@ internal sealed record PublicClientsAppResult(string? ClientId, string? ObjectId } _logger.LogInformation("Created Entra app '{AppName}' (clientId: {ClientId})", appName, app.Value.ClientId); - var secret = await _graphApiService.AddAppPasswordAsync(tenantId, app.Value.ObjectId, lifetimeMonths: lifetimeMonths, ct: ct); - if (string.IsNullOrWhiteSpace(secret)) + // The app now exists in the tenant. If any follow-up step (secret creation, validation) + // throws, the app is orphaned with no caller-side cleanup, so compensate here before + // rethrowing. Cleanup is cancellation-independent so a Ctrl+C still removes the orphan. + try { - _logger.LogError("Failed to create secret for '{AppName}'. Run with -v for details.", appName); - await TryDeleteOrphanedAppAsync(tenantId, app.Value.ObjectId, appName, "secret-creation failed", ct); - return null; - } + var secret = await _graphApiService.AddAppPasswordAsync(tenantId, app.Value.ObjectId, lifetimeMonths: lifetimeMonths, ct: ct); + if (string.IsNullOrWhiteSpace(secret)) + { + _logger.LogError("Failed to create secret for '{AppName}'. Run with -v for details.", appName); + await TryDeleteOrphanedAppAsync(tenantId, app.Value.ObjectId, appName, "secret-creation failed"); + return null; + } + + if (string.IsNullOrWhiteSpace(app.Value.ClientId)) + { + _logger.LogError("{Role} Entra application was created but returned an empty client ID", roleDisplay); + await TryDeleteOrphanedAppAsync(tenantId, app.Value.ObjectId, appName, "empty client ID returned"); + return null; + } - if (string.IsNullOrWhiteSpace(app.Value.ClientId)) + _logger.LogDebug("Created {Role} app: {ClientId}", roleDisplay, app.Value.ClientId); + return new ProxyAppResult(app.Value.ClientId, secret, app.Value.ObjectId, appName); + } + catch (Exception ex) { - _logger.LogError("{Role} Entra application was created but returned an empty client ID", roleDisplay); - await TryDeleteOrphanedAppAsync(tenantId, app.Value.ObjectId, appName, "empty client ID returned", ct); - return null; + _logger.LogError("Provisioning '{AppName}' failed after the app was created; deleting the orphaned app.", appName); + _logger.LogDebug("Exception details: {Exception}", ex.ToString()); + await TryDeleteOrphanedAppAsync(tenantId, app.Value.ObjectId, appName, "provisioning threw after app creation"); + throw; } - - _logger.LogDebug("Created {Role} app: {ClientId}", roleDisplay, app.Value.ClientId); - return new ProxyAppResult(app.Value.ClientId, secret, app.Value.ObjectId, appName); } /// @@ -104,7 +117,7 @@ internal sealed record PublicClientsAppResult(string? ClientId, string? ObjectId /// secondary cleanup error to drown the root cause; the user can clean up manually using the /// objectId we log. /// - private async Task TryDeleteOrphanedAppAsync(string tenantId, string objectId, string appName, string reason, CancellationToken ct) + private async Task TryDeleteOrphanedAppAsync(string tenantId, string objectId, string appName, string reason) { if (string.IsNullOrWhiteSpace(objectId)) { @@ -113,7 +126,7 @@ private async Task TryDeleteOrphanedAppAsync(string tenantId, string objectId, s try { - var deleted = await _graphApiService.DeleteEntraAppAsync(tenantId, objectId, ct); + var deleted = await _graphApiService.DeleteEntraAppAsync(tenantId, objectId, CancellationToken.None); if (deleted) { _logger.LogInformation( 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/DevelopMcpCommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandTests.cs index 0f4efc91..be4e349a 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 1989ea12..b7666569 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,17 +13,18 @@ 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, - /// not Alias — (b) describe a PPMI-scope-only back-fill (no redirect-URI back-fill), and - /// (c) skip the platform publish call entirely. + /// The dry-run log must (a) name the Public Clients app — derived from ServerName, + /// 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>()); @@ -101,4 +103,48 @@ public async Task ExecuteAsync_DryRun_AcceptsPublisherName_WithoutCallingPlatfor // Dry-run short-circuits before the platform publish call regardless of publisher. await toolingService.DidNotReceiveWithAnyArgs().PublishServerAsync(default!, default!, default!, default); } + + /// + /// Dry-run for a first-party Dataverse server must describe only the Public Clients app and make + /// clear no A365 proxy app/secret/connector is created, so the preview matches classify-first + /// behavior. It must not claim it will forward proxy credentials, and must skip the platform call. + /// + [Fact] + public async Task ExecuteAsync_DryRun_FirstPartyDataverseServer_DescribesPublicClientsOnly_NoProxy() + { + var logger = Substitute.For(); + var toolingService = Substitute.For(); + + var args = new RawPublishArgs( + EnvironmentId: "00000000-0000-0000-0000-000000000000", + ServerName: "msdyn_DataverseMCPServer", + Alias: "myAlias", + DisplayName: "Test Display", + PublisherName: null, + Yes: false, + DryRun: true); + + var executor = new PublishCommandExecutor(logger, toolingService, graphApiService: null); + + await executor.ExecuteAsync(args, CancellationToken.None); + + logger.Received(1).Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => + o.ToString()!.Contains("first-party Dataverse MCP server") && + o.ToString()!.Contains("msdyn_DataverseMCPServer-PublicClients")), + Arg.Any(), + Arg.Any>()); + + // Must NOT claim it will forward proxy credentials — that line is only for custom servers. + logger.DidNotReceive().Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("forward the A365 proxy app credentials")), + Arg.Any(), + Arg.Any>()); + + await toolingService.DidNotReceiveWithAnyArgs().PublishServerAsync(default!, default!, default!, default); + } } 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..18118445 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandExecutorEntraAppTests.cs @@ -0,0 +1,549 @@ +// 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"); + } + + /// + /// 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() + { + 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_WhenConnectorCreatedButRedirectUriMissing_WarnsAndSkipsRedirectUpdate() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + 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", A365ProxyConnectorId = "connector-id" }); + + 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("connector was created but publish returned no redirect URI")), + Arg.Any(), + Arg.Any>()); + } + + /// + /// A custom server normally gets a connector, but if the platform returns a real payload that + /// reports no connector (the CLI's name classification drifted from the platform's, or the platform + /// treats the server as first-party), the proxy app created for it is unused and must be reconciled + /// away so it doesn't linger in the tenant. The Public Clients app must be left in place. A "real + /// payload" is distinguished by McpServerAppId being present - see the metadata-free placeholder + /// case in . + /// + [Fact] + public async Task ExecuteAsync_WhenNoConnectorCreated_DeletesUnusedProxyApp_AndSkipsRedirectUpdate() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + var (_, _, proxyObjectId) = ArrangeSuccessfulAppCreation(graph); + + // Real v2 payload (McpServerAppId present) with no connector id and no redirect URI => the + // platform genuinely created no connector for this server. + tooling.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new PublishMcpServerResponse { Status = "Success", McpServerAppId = "1a2a0eb6-0000-0000-0000-000000000000" }); + + 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()); + } + + /// + /// The tooling layer returns a placeholder { Status = "Success" } (every connector field and + /// McpServerAppId null) when the platform answers 2xx with an empty or undeserializable body. By + /// then the proxy client id and secret have already been sent, so the platform may have created the + /// connector. Deleting the proxy app here would orphan that connector's OAuth client. The executor + /// must therefore keep the proxy app, warn naming it and its clientId, and still exit success. + /// + [Fact] + public async Task ExecuteAsync_WhenPublishReturnsMetadataFreeSuccess_KeepsProxyApp_AndWarns() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + var (proxyClientId, _, proxyObjectId) = ArrangeSuccessfulAppCreation(graph); + + // Metadata-free placeholder: a 2xx with no body => no McpServerAppId and no connector fields. + // This must NOT be read as proof that no connector was created. + 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("a 2xx publish still succeeds even when the body carries no metadata"); + await graph.DidNotReceive().DeleteEntraAppAsync(TenantId, proxyObjectId, Arg.Any()); + logger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("connector creation could not be confirmed") + && o.ToString()!.Contains(proxyClientId)), + 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, + /// 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, + A365ProxyConnectorId = "connector-id", + }); + + 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() + { + 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()); + } + + /// + /// First-party (OOB) Dataverse servers are fronted by the platform's own Entra app. Publish must + /// classify them by name up front and NOT create an A365 proxy app or secret, and must send null + /// (not empty) proxy credentials: the platform's v2 publish binds the proxy client id as a Guid?, + /// where null means "no connector needed" and an empty string is a 400. Only the Public Clients + /// app is created, and there is no unused proxy app to reconcile away. + /// + [Fact] + public async Task ExecuteAsync_WhenFirstPartyDataverseServer_SkipsProxyApp_AndSendsNullProxyCredentials() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + graph.CreateEntraAppAsync(Arg.Any(), Arg.Is(n => n.EndsWith("-PublicClients")), Arg.Any(), Arg.Any()) + .Returns(("pc-object-id", "pc-client-id")); + graph.UpdateAppPublicClientRedirectUrisAsync(Arg.Any(), Arg.Any(), Arg.Any>(), Arg.Any()) + .Returns(true); + + PublishMcpServerRequest? capturedRequest = null; + tooling.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Do(r => capturedRequest = r), Arg.Any()) + .Returns(new PublishMcpServerResponse { Status = "Success" }); + + var executor = MakeExecutor(logger, tooling, graph); + var args = MakeArgs() with { ServerName = "msdyn_DataverseMCPServer" }; + + var result = await executor.ExecuteAsync(args, CancellationToken.None); + + result.Should().BeTrue(); + await graph.DidNotReceive().CreateEntraAppAsync( + Arg.Any(), Arg.Is(n => n.EndsWith("-A365Proxy")), Arg.Any(), Arg.Any()); + await graph.DidNotReceiveWithAnyArgs().AddAppPasswordAsync(default!, default!, default!, default, default); + await graph.Received(1).CreateEntraAppAsync( + Arg.Any(), Arg.Is(n => n.EndsWith("-PublicClients")), Arg.Any(), Arg.Any()); + + capturedRequest.Should().NotBeNull(); + capturedRequest!.A365ProxyClientId.Should().BeNull( + because: "a first-party server has no proxy app; the platform binds proxy client id as Guid? and a null (not empty) value signals 'no connector needed', avoiding a 400"); + capturedRequest.A365ProxyClientSecret.Should().BeNull( + because: "no proxy secret is created for first-party servers"); + + await graph.DidNotReceiveWithAnyArgs().DeleteEntraAppAsync(default!, default!, default); + } + + /// + /// Rollback runs because a publish failed - frequently because the caller cancelled (Ctrl+C). It + /// must therefore delete the just-created apps with a cancellation-independent token, not the + /// caller's (already-cancelled) token; otherwise the Public Clients app and the confidential proxy + /// app (with its live secret) are left orphaned in the tenant. + /// + [Fact] + public async Task RollbackEntraAppsAsync_UsesCancellationIndependentToken_WhenCallerTokenIsCancelled() + { + 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"); + + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await executor.RollbackEntraAppsAsync(apps, TenantId, cts.Token); + + await graph.Received(1).DeleteEntraAppAsync( + TenantId, "pc-object-id", Arg.Is(t => !t.IsCancellationRequested)); + await graph.Received(1).DeleteEntraAppAsync( + TenantId, "proxy-object-id", Arg.Is(t => !t.IsCancellationRequested)); + } + + /// + /// When the publish response shows no connector (the proxy app is unused) AND the automatic delete + /// of that proxy app fails, the executor must surface a user-facing warning to delete it manually. + /// A silent delete failure would leave an unused confidential app (with a live secret) in the + /// tenant with no signal to the user. + /// + [Fact] + public async Task ExecuteAsync_WhenUnusedProxyDeleteFails_SurfacesManualCleanupWarning() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + ArrangeSuccessfulAppCreation(graph); + // The reconcile delete of the unused proxy app fails (overrides the arrange's success stub). + graph.DeleteEntraAppAsync(Arg.Any(), Arg.Any(), Arg.Any()).Returns(false); + + // Real v2 payload (McpServerAppId present) with no connector => proxy is unused and the + // reconcile tries to delete it; that delete failing is what this test exercises. + tooling.PublishServerAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new PublishMcpServerResponse { Status = "Success", McpServerAppId = "1a2a0eb6-0000-0000-0000-000000000000" }); + + var executor = MakeExecutor(logger, tooling, graph); + + var result = await executor.ExecuteAsync(MakeArgs(), CancellationToken.None); + + result.Should().BeTrue("a failed cleanup of an unused proxy app is a warning, not a publish failure"); + logger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("could not be deleted automatically")), + Arg.Any(), + Arg.Any>()); + } + + /// + /// Cancellation can arrive after both Entra apps are provisioned but before the platform publish + /// call. The pre-publish cancellation check must roll back BOTH apps (including the confidential + /// proxy app and its live secret) before propagating the cancellation; otherwise they are leaked + /// in the tenant with no corresponding platform record. The direct RollbackEntraAppsAsync test + /// does not exercise this ExecuteAsync path. + /// + [Fact] + public async Task ExecuteAsync_WhenCancelledAfterProvisioning_RollsBackBothApps_AndSkipsPlatformCall() + { + var logger = Substitute.For(); + var tooling = Substitute.For(); + var graph = Substitute.For(); + + ArrangeSuccessfulAppCreation(graph); + + using var cts = new CancellationTokenSource(); + // Cancel on the last provisioning Graph call (public-client redirect URIs) so both apps are + // fully created and the next cancellation check in ExecuteAsync observes the cancellation. + graph.When(g => g.UpdateAppPublicClientRedirectUrisAsync( + Arg.Any(), Arg.Any(), Arg.Any>(), Arg.Any())) + .Do(_ => cts.Cancel()); + + var executor = MakeExecutor(logger, tooling, graph); + + var act = async () => await executor.ExecuteAsync(MakeArgs(), cts.Token); + + await act.Should().ThrowAsync( + because: "a cancellation observed after provisioning but before the platform call must propagate, not be swallowed"); + + // Both apps rolled back, with a cancellation-independent token so the already-cancelled + // caller token does not skip the deletes. + await graph.Received(1).DeleteEntraAppAsync( + TenantId, "pc-object-id", Arg.Is(t => !t.IsCancellationRequested)); + await graph.Received(1).DeleteEntraAppAsync( + TenantId, "proxy-object-id", Arg.Is(t => !t.IsCancellationRequested)); + // No platform call occurred, so there is nothing published to compensate for beyond the apps. + await tooling.DidNotReceiveWithAnyArgs().PublishServerAsync(default!, default!, default!, default); + } + + /// + /// 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); + } +} 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()); + } +} diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Constants/FirstPartyMcpServersTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Constants/FirstPartyMcpServersTests.cs new file mode 100644 index 00000000..2ec79c78 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Constants/FirstPartyMcpServersTests.cs @@ -0,0 +1,63 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Constants; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Constants; + +/// +/// Pins the first-party Dataverse server classification that drives publish's "skip the A365 proxy +/// app" decision. The names mirror the MCP platform's OOBDataverseServerNamesToScopeMapping; matching +/// must be exact and case-insensitive, never a msdyn_ prefix match, so a custom server that +/// merely starts with msdyn_ is still treated as custom and gets its proxy app. +/// +public class FirstPartyMcpServersTests +{ + [Theory] + [InlineData("msdyn_SalesMCPServer")] + [InlineData("msdyn_ServiceMCPServer")] + [InlineData("msdyn_ERPAnalyticsMCPServer")] + [InlineData("msdyn_D365ContactCenterAdminMCPServer")] + [InlineData("msdyn_ContactCenterMCPServer")] + [InlineData("msdyn_DataverseMCPServer")] + [InlineData("msdyn_DataversePreviewMCPServer")] + [InlineData("msdyn_CIMCPServer")] + [InlineData("msdyn_FnOMCPServer")] + public void IsOobDataverseServer_ReturnsTrue_ForKnownOobNames(string serverName) + { + FirstPartyMcpServers.IsOobDataverseServer(serverName).Should().BeTrue( + because: "publish must skip the A365 proxy app for servers the platform fronts with its own first-party app"); + } + + [Fact] + public void IsOobDataverseServer_ReturnsTrue_ForNonMsdynBackwardsCompatName() + { + FirstPartyMcpServers.IsOobDataverseServer("MCP_DataverseMCPServer").Should().BeTrue( + because: "the legacy backwards-compat Dataverse name has no msdyn_ prefix, so a prefix-only check would wrongly treat it as custom"); + } + + [Theory] + [InlineData("msdyn_sAlESmcpSERVER")] + [InlineData("MSDYN_DATAVERSEMCPSERVER")] + public void IsOobDataverseServer_IsCaseInsensitive(string serverName) + { + FirstPartyMcpServers.IsOobDataverseServer(serverName).Should().BeTrue( + because: "server-name casing from discovery is not guaranteed, so classification must be case-insensitive to match the platform"); + } + + [Theory] + [InlineData("mcp_TestServer")] + [InlineData("msdyn_SomethingCustomMCPServer")] + [InlineData("msdyn_")] + [InlineData("contoso_SalesMCPServer")] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void IsOobDataverseServer_ReturnsFalse_ForCustomOrEmptyNames(string? serverName) + { + FirstPartyMcpServers.IsOobDataverseServer(serverName).Should().BeFalse( + because: "only exact matches to the known first-party list skip the proxy app; a msdyn_-prefixed-but-unknown or empty name is custom and must get a proxy app"); + } +} 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 d60ebd8f..4da14a5d 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 @@ -208,6 +208,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() { 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..b6eee8b2 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 @@ -69,6 +69,29 @@ public async Task CreateProxyAppAsync_UsesSuffixInAppName() result!.AppName.Should().Be($"{ServerName}-RemoteProxy"); } + /// + /// If a follow-up step throws after the app registration already exists (e.g. Graph throttles the + /// secret creation), the app is orphaned with no caller-side cleanup. The provisioner must delete + /// the orphan and rethrow so the publish aborts rather than proceed with a half-created app. + /// + [Fact] + public async Task CreateProxyAppAsync_WhenSecretCreationThrows_DeletesOrphanAppAndRethrows() + { + _graph.CreateEntraAppAsync(TenantId, $"{ServerName}-A365Proxy", serviceTreeId: null, Arg.Any()) + .Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId))); + _graph.AddAppPasswordAsync(TenantId, AppObjectId) + .Returns(_ => throw new InvalidOperationException("graph throttled")); + _graph.DeleteEntraAppAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); + + var act = async () => await _provisioner.CreateProxyAppAsync( + ServerName, TenantId, suffix: "A365Proxy", roleDisplay: "A365 Proxy", serviceTreeId: null); + + await act.Should().ThrowAsync( + because: "a provisioning failure after the app is created must propagate so publish aborts instead of continuing with a half-created app"); + await _graph.Received(1).DeleteEntraAppAsync(TenantId, AppObjectId, Arg.Any()); + } + [Fact] public async Task CreateProxyAppAsync_WhenCreateAppReturnsNull_ReturnsNullAndLogsError() {