From 1318bd5015c01880e7d015caf5b903b0c56173b2 Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Mon, 14 Sep 2026 13:49:20 -0700 Subject: [PATCH 01/10] Add `a365 network vnet` for linking an Azure VNet to Agent 365 The documented subnet-injection flow ends with Enable-SubnetInjection, which takes the id of the Power Platform environment to link. Agent 365 provisions a managed environment per tenant and does not publish its id, so admins cannot finish the flow. These subcommands replace that final step: the platform resolves the environment server-side and performs the link. The policy systemId read stays here rather than in the platform. It is a plain ARM GET against a resource the admin already owns, and doing it client-side with the admin's own az login avoids giving the platform a delegated ARM consent grant it otherwise has no need for. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + docs/commands/README.md | 4 + docs/commands/network.md | 105 +++++ .../Commands/NetworkCommand.cs | 241 ++++++++++ .../Constants/CommandNames.cs | 1 + .../Models/VNetModels.cs | 77 ++++ .../Program.cs | 12 + .../Services/ArmApiService.cs | 93 ++++ .../Services/IVNetLinkService.cs | 57 +++ .../Services/VNetLinkService.cs | 240 ++++++++++ .../Commands/NetworkCommandTests.cs | 217 +++++++++ .../Services/ArmApiServiceTests.cs | 146 ++++++ .../Services/VNetLinkServiceTests.cs | 427 ++++++++++++++++++ 13 files changed, 1621 insertions(+) create mode 100644 docs/commands/network.md create mode 100644 src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs create mode 100644 src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs create mode 100644 src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs create mode 100644 src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs create mode 100644 src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs create mode 100644 src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 59aa369b..320e9957 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g **Option B — CLI** (`a365 setup admin`) has been removed in this release. Use Option A above, or copy the PowerShell instructions printed in the `a365 setup all` summary output. ### Added +- `a365 network vnet link|unlink|status` — links an Azure virtual network to Agent 365 through a Power Platform NetworkInjection enterprise policy. Replaces `Enable-SubnetInjection` from the `Microsoft.PowerPlatform.EnterprisePolicies` module, which cannot be used because it requires the id of the Agent 365 managed environment and that id is not published. The CLI reads the policy's `systemId` from Azure with your existing `az login` and the platform performs the link against the environment it resolves for your tenant. Requires Global Administrator or Power Platform Administrator. See [docs/commands/network.md](docs/commands/network.md). - Setup and bootstrap now use Microsoft's first-party Agent 365 CLI application when it is present in your tenant, validating it without changing Microsoft's app registration, and fall back to a tenant-owned "Agent 365 CLI" app when it is not (#489). - Log separator written at the start of each CLI invocation now redacts values for secret-bearing options (e.g. `--idp-client-secret`) so they are not written to the log file in plain text. - Authentication context (tenant and user) is now logged at the `Information` level whenever the resolved sign-in identity changes, giving operators a clear audit trail in the log file of who the CLI is acting as, without exposing credentials. diff --git a/docs/commands/README.md b/docs/commands/README.md index 29153973..14f3260b 100644 --- a/docs/commands/README.md +++ b/docs/commands/README.md @@ -27,6 +27,10 @@ There is reference documentation for each command. | [develop-mcp list-servers](https://learn.microsoft.com/microsoft-agent-365/developer/reference/cli/develop-mcp#develop-mcp-list-servers) | List MCP servers in a specific Dataverse environment. | | [develop-mcp publish](https://learn.microsoft.com/microsoft-agent-365/developer/reference/cli/develop-mcp#develop-mcp-publish) | Publish an MCP server to a Dataverse environment. | | [develop-mcp unpublish](https://learn.microsoft.com/microsoft-agent-365/developer/reference/cli/develop-mcp#develop-mcp-unpublish) | Unpublish an MCP server from a Dataverse environment. | +| [network](network.md) | Configure tenant networking for Agent 365. | +| [network vnet link](network.md#link) | Link a NetworkInjection enterprise policy to your Agent 365 environment. | +| [network vnet unlink](network.md#unlink) | Remove the virtual network link from your Agent 365 environment. | +| [network vnet status](network.md#status) | Show whether a virtual network policy is linked to your Agent 365 environment. | | [publish](https://learn.microsoft.com/microsoft-agent-365/developer/reference/cli/publish) | Update manifest.json ID values and publish the package. Configure federated identity and app role assignments. | | [query-entra](https://learn.microsoft.com/microsoft-agent-365/developer/reference/cli/query-entra) | Query Microsoft Entra ID for agent information including scopes, permissions, and consent status. | | [query-entra blueprint-scopes](https://learn.microsoft.com/microsoft-agent-365/developer/reference/cli/query-entra#query-entra-blueprint-scopes) | List configured scopes and consent status for the agent blueprint. | diff --git a/docs/commands/network.md b/docs/commands/network.md new file mode 100644 index 00000000..31123e53 --- /dev/null +++ b/docs/commands/network.md @@ -0,0 +1,105 @@ +# `a365 network vnet` + +Links an Azure virtual network to Agent 365 via a Power Platform **NetworkInjection enterprise +policy**, without needing the id of the Power Platform environment. + +## Why this command exists + +The documented subnet-injection flow +([Set up virtual network support](https://learn.microsoft.com/power-platform/admin/vnet-support-setup-configure)) +ends with `Enable-SubnetInjection` from the `Microsoft.PowerPlatform.EnterprisePolicies` module, +which takes an `-environmentId`. Agent 365 provisions a managed Power Platform environment for the +tenant and does not publish its id, so that final step cannot be run. + +`a365 network vnet` replaces only that last step. The CLI reads the policy's `systemId` from Azure +using your existing `az login`, then asks the Agent 365 platform to perform the link against the +environment it resolves for your tenant. + +Everything before the final step is unchanged — keep using the PowerShell module to create the +subnets, delegate them to `Microsoft.PowerPlatform/enterprisePolicies`, and create the policy with +`New-SubnetInjectionEnterprisePolicy`. + +## Prerequisites + +- **Global Administrator** or **Power Platform Administrator** in the tenant. The platform rejects + anyone else. +- An active `az login` session in the same tenant. Used only to read the enterprise policy. +- A NetworkInjection enterprise policy already created by `New-SubnetInjectionEnterprisePolicy`, + with subnets delegated to `Microsoft.PowerPlatform/enterprisePolicies`. +- Public cloud only. Sovereign clouds are not supported. + +## Subcommands + +| Command | Description | +| --- | --- | +| `a365 network vnet link` | Link a NetworkInjection enterprise policy to the tenant's Agent 365 environment. | +| `a365 network vnet unlink` | Remove the virtual network link. | +| `a365 network vnet status` | Show the current link, or check a running operation. | + +### `link` + +```bash +a365 network vnet link --policy-arm-id [--swap] [--tenant-id ] [--wait] +``` + +| Option | Description | +| --- | --- | +| `--policy-arm-id`, `-p` | **Required.** ARM resource id of the policy, as returned by `New-SubnetInjectionEnterprisePolicy`. | +| `--swap` | Replace an existing link to a *different* policy. Without it, a different existing link is reported as a conflict instead of being silently replaced. | +| `--tenant-id` | Tenant to authenticate against for the Azure policy read. Defaults to the tenant of your current `az login`. | +| `--wait` | Poll until the operation settles instead of returning an operation id. | + +Linking the policy that is already linked is a no-op and succeeds without `--swap`. + +### `unlink` + +```bash +a365 network vnet unlink [--wait] +``` + +Unlink needs no policy id — the platform remembers which policy it linked. + +### `status` + +```bash +a365 network vnet status [--operation-id ] +``` + +Without `--operation-id`, reports the environment's current link. With one, reports that specific +operation. + +## Statuses and exit codes + +| Status | Meaning | +| --- | --- | +| `Linked` | A policy is linked; `Policy` names it. | +| `NotLinked` | No policy is linked. | +| `Running` / `NotStarted` | The operation is still in flight; `Operation` is the handle to poll. | +| `Failed` | The operation failed; `Reason` explains why. | + +Exit code is `1` on `Failed` or on any request error, and `0` otherwise — including a still-running +operation, which is a legitimate outcome when `--wait` is not passed. + +## Typical flow + +```bash +# 1. Create the policy with the PowerShell module (unchanged). +./SubnetInjection/NewSubnetInjectionEnterprisePolicy.ps1 ` + -subscription -resourceGroup -enterprisePolicyName ` + -enterprisePolicyLocation -virtualNetworkId -subnetName + +# 2. Link it — this replaces Enable-SubnetInjection. +a365 network vnet link --policy-arm-id /subscriptions//resourceGroups//providers/Microsoft.PowerPlatform/enterprisePolicies/ --wait + +# 3. Confirm. +a365 network vnet status +``` + +## Troubleshooting + +| Symptom | Cause | +| --- | --- | +| `Could not determine your Azure tenant` | No `az login` session. Run `az login`, or pass `--tenant-id`. | +| `403` from the platform | Caller is not a Global or Power Platform Administrator, or the CLI app lacks consent for the `AgentTools.VNet.*` scopes. | +| Conflict reported on `link` | A *different* policy is already linked. Re-run with `--swap`, or `unlink` first. | +| Policy read fails | The policy ARM id is wrong, or your `az login` identity cannot read it. | diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs new file mode 100644 index 00000000..9f18d8ee --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs @@ -0,0 +1,241 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Models; +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging; +using System.CommandLine; +using System.CommandLine.Invocation; + +namespace Microsoft.Agents.A365.DevTools.Cli.Commands; + +/// +/// Tenant network configuration for Agent 365. +/// +/// Subnet injection normally ends with Enable-SubnetInjection from the +/// Microsoft.PowerPlatform.EnterprisePolicies module, which needs the id of the Power Platform +/// environment being linked. Agent 365 does not publish that id, so these subcommands ask the +/// platform to perform the link against the environment it resolves for your tenant. +/// +public static class NetworkCommand +{ + private static readonly TimeSpan DefaultWaitTimeout = TimeSpan.FromMinutes(10); + + /// + /// Creates the network command and its vnet subcommand tree. + /// + public static Command CreateCommand( + ILogger logger, + IVNetLinkService vnetLinkService, + IAzureCliService azureCliService) + { + var networkCommand = new Command("network", "Configure tenant networking for Agent 365"); + + var vnetCommand = new Command( + "vnet", + "Link an Azure virtual network enterprise policy to your Agent 365 environment. " + + "Requires the Global Administrator or Power Platform Administrator role."); + + vnetCommand.AddCommand(CreateLinkSubcommand(logger, vnetLinkService, azureCliService)); + vnetCommand.AddCommand(CreateUnlinkSubcommand(logger, vnetLinkService)); + vnetCommand.AddCommand(CreateStatusSubcommand(logger, vnetLinkService)); + + networkCommand.AddCommand(vnetCommand); + return networkCommand; + } + + private static Command CreateLinkSubcommand( + ILogger logger, + IVNetLinkService vnetLinkService, + IAzureCliService azureCliService) + { + var command = new Command( + "link", + "Link a NetworkInjection enterprise policy to your Agent 365 environment. " + + "Create the policy first with New-SubnetInjectionEnterprisePolicy; this replaces the " + + "Enable-SubnetInjection step that requires an environment id."); + + var policyArmIdOption = new Option( + ["--policy-arm-id", "-p"], + "ARM resource id of the NetworkInjection enterprise policy, as returned by " + + "New-SubnetInjectionEnterprisePolicy") + { + IsRequired = true, + }; + + var swapOption = new Option( + "--swap", + "Replace an existing link to a different policy. Without this, an existing different " + + "link is reported as a conflict rather than silently replaced."); + + var tenantIdOption = new Option( + "--tenant-id", + "Tenant to authenticate against for the Azure policy read. Defaults to the tenant of " + + "your current az login."); + + var waitOption = new Option( + "--wait", + "Keep polling until the link settles, instead of returning an operation id."); + + var verboseOption = new Option(["--verbose", "-v"], "Enable verbose logging"); + + command.AddOption(policyArmIdOption); + command.AddOption(swapOption); + command.AddOption(tenantIdOption); + command.AddOption(waitOption); + command.AddOption(verboseOption); + + command.SetHandler(async (InvocationContext context) => + { + var policyArmId = context.ParseResult.GetValueForOption(policyArmIdOption)!; + var swap = context.ParseResult.GetValueForOption(swapOption); + var tenantId = context.ParseResult.GetValueForOption(tenantIdOption); + var wait = context.ParseResult.GetValueForOption(waitOption); + var ct = context.GetCancellationToken(); + + if (string.IsNullOrWhiteSpace(tenantId)) + { + var account = await azureCliService.GetCurrentAccountAsync(); + tenantId = account?.TenantId; + if (string.IsNullOrWhiteSpace(tenantId)) + { + logger.LogError("Could not determine your Azure tenant. Run 'az login', or pass --tenant-id."); + context.ExitCode = 1; + return; + } + } + + var result = await vnetLinkService.LinkAsync(policyArmId, swap, tenantId, ct); + context.ExitCode = await ReportAsync(logger, vnetLinkService, result, wait, "Link", ct); + }); + + return command; + } + + private static Command CreateUnlinkSubcommand(ILogger logger, IVNetLinkService vnetLinkService) + { + var command = new Command( + "unlink", + "Remove the virtual network link from your Agent 365 environment."); + + var waitOption = new Option( + "--wait", + "Keep polling until the unlink settles, instead of returning an operation id."); + + var verboseOption = new Option(["--verbose", "-v"], "Enable verbose logging"); + + command.AddOption(waitOption); + command.AddOption(verboseOption); + + command.SetHandler(async (InvocationContext context) => + { + var wait = context.ParseResult.GetValueForOption(waitOption); + var ct = context.GetCancellationToken(); + + var result = await vnetLinkService.UnlinkAsync(ct); + context.ExitCode = await ReportAsync(logger, vnetLinkService, result, wait, "Unlink", ct); + }); + + return command; + } + + private static Command CreateStatusSubcommand(ILogger logger, IVNetLinkService vnetLinkService) + { + var command = new Command( + "status", + "Show whether a virtual network policy is linked to your Agent 365 environment."); + + var operationIdOption = new Option( + "--operation-id", + "Operation handle returned by a link or unlink that was still running."); + + var verboseOption = new Option(["--verbose", "-v"], "Enable verbose logging"); + + command.AddOption(operationIdOption); + command.AddOption(verboseOption); + + command.SetHandler(async (InvocationContext context) => + { + var operationId = context.ParseResult.GetValueForOption(operationIdOption); + var ct = context.GetCancellationToken(); + + var status = await vnetLinkService.GetStatusAsync(operationId, ct); + if (status == null) + { + context.ExitCode = 1; + return; + } + + LogStatus(logger, status); + context.ExitCode = string.Equals(status.Status, "Failed", StringComparison.OrdinalIgnoreCase) ? 1 : 0; + }); + + return command; + } + + /// + /// Renders the outcome of a link or unlink, optionally waiting for a running operation first, + /// and maps it to a process exit code. + /// + internal static async Task ReportAsync( + ILogger logger, + IVNetLinkService vnetLinkService, + VNetStatusResponse? result, + bool wait, + string operationLabel, + CancellationToken cancellationToken) + { + if (result == null) + { + return 1; + } + + if (wait && VNetLinkService.IsRunning(result.Status) && !string.IsNullOrWhiteSpace(result.OperationId)) + { + logger.LogInformation("{Operation} is running. Waiting for it to settle...", operationLabel); + result = await vnetLinkService.WaitForCompletionAsync(result.OperationId, DefaultWaitTimeout, cancellationToken); + + if (result == null) + { + return 1; + } + } + + LogStatus(logger, result); + + if (string.Equals(result.Status, "Failed", StringComparison.OrdinalIgnoreCase)) + { + return 1; + } + + if (VNetLinkService.IsRunning(result.Status)) + { + logger.LogInformation( + "{Operation} is still running. Check on it with: a365 network vnet status --operation-id {OperationId}", + operationLabel, + result.OperationId); + } + + return 0; + } + + private static void LogStatus(ILogger logger, VNetStatusResponse status) + { + logger.LogInformation("Status: {Status}", status.Status ?? "Unknown"); + + if (!string.IsNullOrWhiteSpace(status.PolicyArmId)) + { + logger.LogInformation("Policy: {PolicyArmId}", status.PolicyArmId); + } + + if (!string.IsNullOrWhiteSpace(status.OperationId)) + { + logger.LogInformation("Operation: {OperationId}", status.OperationId); + } + + if (!string.IsNullOrWhiteSpace(status.Reason)) + { + logger.LogWarning("Reason: {Reason}", status.Reason); + } + } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/CommandNames.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/CommandNames.cs index 8c82ee86..7ea93af2 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/CommandNames.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/CommandNames.cs @@ -18,4 +18,5 @@ public static class CommandNames public const string Develop = "develop"; public const string CreateInstance = "create-instance"; public const string Logs = "logs"; + public const string Network = "network"; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs new file mode 100644 index 00000000..b2701a6d --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs @@ -0,0 +1,77 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Text.Json.Serialization; + +namespace Microsoft.Agents.A365.DevTools.Cli.Models; + +/// +/// Request body for linking a virtual network enterprise policy to the tenant's Agent 365 +/// Power Platform environment. +/// +public class VNetLinkRequest +{ + /// + /// The policy's properties.systemId, shaped + /// /regions/{region}/providers/Microsoft.PowerPlatform/enterprisePolicies/{guid}. + /// Resolved from the ARM policy id by the CLI, using the caller's own Azure session. + /// + [JsonPropertyName("policySystemId")] + public string? PolicySystemId { get; set; } + + /// + /// The policy's ARM resource id. Carried for display and audit only. + /// + [JsonPropertyName("policyArmId")] + public string? PolicyArmId { get; set; } + + /// + /// Whether an existing link to a different policy may be replaced. Mirrors the -Swap switch + /// on Enable-SubnetInjection. + /// + [JsonPropertyName("swap")] + public bool Swap { get; set; } +} + +/// +/// Status of the tenant's virtual network link, and the shape returned by link and unlink +/// once they settle. +/// +public class VNetStatusResponse +{ + /// + /// NotLinked, Running, Linked, Failed, or Unknown. + /// + [JsonPropertyName("status")] + public string? Status { get; set; } + + /// + /// ARM id of the linked policy as reported by the platform. Null when nothing is linked. + /// + [JsonPropertyName("policyArmId")] + public string? PolicyArmId { get; set; } + + /// + /// Handle for an operation that is still running, or has recently settled. + /// + [JsonPropertyName("operationId")] + public string? OperationId { get; set; } + + /// + /// Failure reason, when the platform has one to report. + /// + [JsonPropertyName("reason")] + public string? Reason { get; set; } +} + +/// +/// Error body returned by the platform's virtual network endpoints. +/// +public class VNetErrorResponse +{ + /// + /// Human-readable error message. + /// + [JsonPropertyName("error")] + public string? Error { get; set; } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs index 546dd535..566a313c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs @@ -185,6 +185,10 @@ await Task.WhenAll( var logsLogger = serviceProvider.GetRequiredService>(); var logRedactionService = serviceProvider.GetRequiredService(); rootCommand.AddCommand(LogsCommand.CreateCommand(logsLogger, logRedactionService)); + var networkLogger = serviceProvider.GetRequiredService().CreateLogger("network"); + var vnetLinkService = serviceProvider.GetRequiredService(); + var azureCliService = serviceProvider.GetRequiredService(); + rootCommand.AddCommand(NetworkCommand.CreateCommand(networkLogger, vnetLinkService, azureCliService)); // Build pipeline manually so we can skip UseTypoCorrections() ("Did you mean?" noise) // and UseParseErrorReporting() (full help dump on any parse error), replacing both @@ -376,6 +380,14 @@ private static void ConfigureServices(IServiceCollection services, LogLevel mini services.AddSingleton(); services.AddSingleton(); + + // Reuses the environment the tooling service already resolved (env var, then config file), + // so the two never disagree about which Agent 365 deployment the CLI is talking to. + services.AddSingleton(provider => new VNetLinkService( + provider.GetRequiredService>(), + provider.GetRequiredService(), + provider.GetRequiredService(), + provider.GetRequiredService().Environment)); services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs index 150332dc..fee4ef14 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs @@ -25,6 +25,9 @@ public class ArmApiService : IDisposable private const string ResourceGroupApiVersion = "2021-04-01"; private const string AppServiceApiVersion = "2022-03-01"; + // Stable first: the module's own ARM templates deploy enterprise policies at 2020-10-30. + private static readonly string[] EnterprisePolicyApiVersions = ["2020-10-30", "2020-10-30-preview"]; + private readonly ILogger _logger; private readonly HttpClient _httpClient; private readonly IAuthenticationService _authService; @@ -243,4 +246,94 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke } } + /// + /// Reads a Microsoft.PowerPlatform/enterprisePolicies resource and returns its + /// properties.systemId — the only identifier the Business App Platform accepts when + /// linking a policy to an environment. Shaped + /// /regions/{region}/providers/Microsoft.PowerPlatform/enterprisePolicies/{guid}, + /// which is not derivable from the ARM resource id. + /// + /// This read happens in the CLI, using the admin's own Azure session, so the Agent 365 + /// service never needs delegated ARM access. + /// + /// Returns null when the policy cannot be read or has no systemId; the message is logged. + /// + public virtual async Task GetEnterprisePolicySystemIdAsync( + string policyArmId, + string tenantId, + CancellationToken ct = default) + { + if (string.IsNullOrWhiteSpace(policyArmId)) + throw new ArgumentException("Policy ARM id is required.", nameof(policyArmId)); + + if (!await EnsureArmHeadersAsync(tenantId, ct)) + return null; + + // The stable and the preview version both ship on this RP and differ by tenant rollout, so + // a rejected api-version is a routine outcome rather than a failure worth surfacing. + foreach (var apiVersion in EnterprisePolicyApiVersions) + { + var url = $"{ArmBaseUrl}{policyArmId}?api-version={apiVersion}"; + _logger.LogDebug("ARM GET enterprise policy (api-version {ApiVersion})", apiVersion); + + try + { + using var response = await _retryHelper.ExecuteWithRetryAsync( + ct => _httpClient.GetAsync(url, ct), cancellationToken: ct); + + if (response.StatusCode == HttpStatusCode.BadRequest) + { + _logger.LogDebug("ARM rejected api-version {ApiVersion}; trying the next one", apiVersion); + continue; + } + + if (!response.IsSuccessStatusCode) + { + _logger.LogError( + "Could not read enterprise policy {PolicyArmId}. Azure returned {StatusCode}. " + + "Check that the policy exists and that you have read access to it.", + policyArmId, + response.StatusCode); + return null; + } + + var body = await response.Content.ReadAsStringAsync(ct); + using var doc = JsonDocument.Parse(body); + + if (!doc.RootElement.TryGetProperty("properties", out var properties) || + !properties.TryGetProperty("systemId", out var systemId)) + { + _logger.LogError( + "Enterprise policy {PolicyArmId} has no systemId. The policy may still be provisioning.", + policyArmId); + return null; + } + + var value = systemId.GetString(); + if (string.IsNullOrWhiteSpace(value)) + { + _logger.LogError( + "Enterprise policy {PolicyArmId} has an empty systemId. The policy may still be provisioning.", + policyArmId); + return null; + } + + _logger.LogDebug("Resolved enterprise policy systemId"); + return value; + } + catch (Exception ex) + { + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogError(ex, "Failed to read enterprise policy {PolicyArmId}", policyArmId); + return null; + } + } + + _logger.LogError( + "Azure rejected every supported enterprise policy api-version reading {PolicyArmId}.", + policyArmId); + return null; + } } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs new file mode 100644 index 00000000..ff0037ee --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs @@ -0,0 +1,57 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Models; + +namespace Microsoft.Agents.A365.DevTools.Cli.Services; + +/// +/// Links an Azure virtual network enterprise policy to the tenant's Agent 365 Power Platform +/// environment through the Agent 365 platform, which resolves that environment itself. +/// +public interface IVNetLinkService +{ + /// + /// Links a policy. Resolves the policy's systemId from ARM using the caller's Azure session, + /// then asks the platform to perform the link. + /// + /// ARM resource id of the NetworkInjection enterprise policy. + /// Whether an existing link to a different policy may be replaced. + /// Tenant to authenticate against for the ARM read. + /// Cancellation token. + /// The resulting status, or null when the operation could not be started. + Task LinkAsync( + string policyArmId, + bool swap, + string tenantId, + CancellationToken cancellationToken = default); + + /// + /// Removes the current link. The platform supplies the policy identifier it stored at link time. + /// + /// Cancellation token. + /// The resulting status, or null when the operation could not be started. + Task UnlinkAsync(CancellationToken cancellationToken = default); + + /// + /// Reads the current link status, optionally resuming a specific operation handle. + /// + /// Handle returned by a link or unlink that was still running. + /// Cancellation token. + /// The current status, or null when it could not be read. + Task GetStatusAsync( + string? operationId = null, + CancellationToken cancellationToken = default); + + /// + /// Polls status until the operation reaches a terminal state or the timeout elapses. + /// + /// Handle of the running operation. + /// How long to keep polling. + /// Cancellation token. + /// The last status read, which may still be Running if the timeout elapsed. + Task WaitForCompletionAsync( + string operationId, + TimeSpan timeout, + CancellationToken cancellationToken = default); +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs new file mode 100644 index 00000000..f8e19e1d --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs @@ -0,0 +1,240 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Constants; +using Microsoft.Agents.A365.DevTools.Cli.Models; +using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; +using Microsoft.Agents.A365.DevTools.Cli.Services.Internal; +using Microsoft.Extensions.Logging; +using System.Diagnostics; +using System.Net; +using System.Net.Http.Headers; +using System.Text; +using System.Text.Json; + +namespace Microsoft.Agents.A365.DevTools.Cli.Services; + +/// +/// Calls the Agent 365 platform's /agents/vnet endpoints. +/// +/// The platform resolves the tenant's Power Platform environment itself, which is why this +/// replaces the final Enable-SubnetInjection step of the Microsoft.PowerPlatform.EnterprisePolicies +/// module: that cmdlet needs an environment id Agent 365 does not publish. +/// +/// The ARM read that turns a policy ARM id into the systemId the Business App Platform requires +/// happens here, in the CLI, under the admin's own Azure session. The platform therefore needs no +/// delegated ARM access of its own. +/// +public class VNetLinkService : IVNetLinkService +{ + private const string LinkPath = "/agents/vnet/link"; + private const string UnlinkPath = "/agents/vnet/unlink"; + private const string StatusPath = "/agents/vnet/status"; + + private static readonly TimeSpan PollInterval = TimeSpan.FromSeconds(10); + + private readonly ILogger _logger; + private readonly IAuthenticationService _authService; + private readonly ArmApiService _armApiService; + private readonly string _environment; + private readonly HttpMessageHandler? _handler; + + public VNetLinkService( + ILogger logger, + IAuthenticationService authService, + ArmApiService armApiService, + string environment = "prod", + HttpMessageHandler? handler = null) + { + _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + _authService = authService ?? throw new ArgumentNullException(nameof(authService)); + _armApiService = armApiService ?? throw new ArgumentNullException(nameof(armApiService)); + _environment = environment ?? "prod"; + _handler = handler; + } + + /// + public async Task LinkAsync( + string policyArmId, + bool swap, + string tenantId, + CancellationToken cancellationToken = default) + { + if (string.IsNullOrWhiteSpace(policyArmId)) + throw new ArgumentException("Policy ARM id is required.", nameof(policyArmId)); + + _logger.LogInformation("Reading enterprise policy from Azure..."); + var policySystemId = await _armApiService.GetEnterprisePolicySystemIdAsync(policyArmId, tenantId, cancellationToken); + if (string.IsNullOrWhiteSpace(policySystemId)) + { + _logger.LogError("Could not resolve the policy's systemId, so there is nothing to send to Agent 365."); + return null; + } + + var request = new VNetLinkRequest + { + PolicySystemId = policySystemId, + PolicyArmId = policyArmId, + Swap = swap, + }; + + _logger.LogInformation("Linking the policy to your Agent 365 environment..."); + return await SendAsync(HttpMethod.Post, LinkPath, request, "link virtual network", cancellationToken); + } + + /// + public async Task UnlinkAsync(CancellationToken cancellationToken = default) + { + _logger.LogInformation("Removing the virtual network link from your Agent 365 environment..."); + return await SendAsync(HttpMethod.Post, UnlinkPath, payload: null, "unlink virtual network", cancellationToken); + } + + /// + public async Task GetStatusAsync( + string? operationId = null, + CancellationToken cancellationToken = default) + { + var path = string.IsNullOrWhiteSpace(operationId) + ? StatusPath + : $"{StatusPath}?operationId={Uri.EscapeDataString(operationId)}"; + + return await SendAsync(HttpMethod.Get, path, payload: null, "read virtual network status", cancellationToken); + } + + /// + public async Task WaitForCompletionAsync( + string operationId, + TimeSpan timeout, + CancellationToken cancellationToken = default) + { + if (string.IsNullOrWhiteSpace(operationId)) + throw new ArgumentException("Operation id is required.", nameof(operationId)); + + // Wall clock, not summed sleeps: each status call costs real time, and a caller who asked + // for five minutes should not wait eight because the service was slow. + var stopwatch = Stopwatch.StartNew(); + VNetStatusResponse? last = null; + + while (true) + { + last = await GetStatusAsync(operationId, cancellationToken); + + if (last == null || !IsRunning(last.Status)) + return last; + + if (stopwatch.Elapsed + PollInterval >= timeout) + return last; + + _logger.LogInformation("Still running... ({Elapsed:0}s elapsed)", stopwatch.Elapsed.TotalSeconds); + await Task.Delay(PollInterval, cancellationToken); + } + } + + /// + /// True when the reported status means the operation has not settled yet. + /// + public static bool IsRunning(string? status) => + string.Equals(status, "Running", StringComparison.OrdinalIgnoreCase); + + private async Task SendAsync( + HttpMethod method, + string path, + object? payload, + string operationName, + CancellationToken cancellationToken) + { + var correlationId = HttpClientFactory.GenerateCorrelationId(); + var baseUrl = BuildBaseUrl(); + var url = $"{baseUrl}{path}"; + + try + { + var audience = ConfigConstants.GetAgent365ToolsResourceAppId(_environment); + var loginHint = await AzCliHelper.ResolveLoginHintAsync(); + var authToken = await _authService.GetAccessTokenAsync(audience, userId: loginHint, ct: cancellationToken); + if (string.IsNullOrWhiteSpace(authToken)) + { + _logger.LogError("Failed to acquire an Agent 365 access token."); + return null; + } + + using var httpClient = HttpClientFactory.CreateAuthenticatedClient( + authToken, correlationId: correlationId, handler: _handler); + + using var request = new HttpRequestMessage(method, url); + if (payload != null) + { + var json = JsonSerializer.Serialize(payload); + request.Content = new StringContent(json, Encoding.UTF8); + request.Content.Headers.ContentType = new MediaTypeHeaderValue("application/json"); + _logger.LogDebug("Request payload: {Payload}", json); + } + + _logger.LogDebug("{Method} {Url} (CorrelationId: {CorrelationId})", method, url, correlationId); + + using var response = await httpClient.SendAsync(request, cancellationToken); + var body = await response.Content.ReadAsStringAsync(cancellationToken); + _logger.LogDebug("Response {StatusCode}: {Body}", response.StatusCode, body); + + if (!response.IsSuccessStatusCode) + { + LogFailure(response.StatusCode, body, operationName, correlationId); + return null; + } + + // 200 and 202 share a shape as far as the CLI is concerned: a status, and an + // operationId when there is more to wait for. + return string.IsNullOrWhiteSpace(body) + ? new VNetStatusResponse() + : JsonSerializer.Deserialize(body); + } + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) + { + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogError(ex, "Failed to {Operation}. Correlation ID: {CorrelationId}", operationName, correlationId); + return null; + } + } + + private void LogFailure(HttpStatusCode statusCode, string body, string operationName, string correlationId) + { + string? message = null; + try + { + message = JsonSerializer.Deserialize(body)?.Error; + } + catch (JsonException) + { + // The platform always sends a typed error body, so a non-JSON body means something + // upstream of it answered. The status code is then the only usable signal. + } + + _logger.LogError( + "Failed to {Operation}. Status: {StatusCode}. {Message}", + operationName, + statusCode, + message ?? "No error detail was returned."); + + if (statusCode == HttpStatusCode.Forbidden) + { + _logger.LogError( + "This command requires the Global Administrator or Power Platform Administrator role, " + + "and a client application consented for AgentTools.VNet.Manage.All."); + } + + _logger.LogError("Correlation ID: {CorrelationId}", correlationId); + } + + private string BuildBaseUrl() + { + var discoverUrl = ConfigConstants.GetDiscoverEndpointUrl(_environment); + var uri = new Uri(discoverUrl); + return $"{uri.Scheme}://{uri.Authority}"; + } +} diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs new file mode 100644 index 00000000..68834a02 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs @@ -0,0 +1,217 @@ +// 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.Extensions.Logging.Abstractions; +using NSubstitute; +using System.CommandLine; +using System.CommandLine.Parsing; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; + +/// +/// Unit tests for the network command tree and its result reporting. +/// The subcommand handlers themselves are exercised through ReportAsync, which holds the +/// wait-and-exit-code logic; the handlers around it only parse options. +/// +public class NetworkCommandTests +{ + private const string OperationId = "op-abc"; + + private static Command CreateCommand(IVNetLinkService? vnet = null, IAzureCliService? azure = null) => + NetworkCommand.CreateCommand( + NullLogger.Instance, + vnet ?? Substitute.For(), + azure ?? Substitute.For()); + + // ──────────────────────────── Command tree shape ──────────────────────────── + + [Fact] + public void CreateCommand_ExposesTheVnetSubcommandTree() + { + var command = CreateCommand(); + + command.Name.Should().Be("network"); + + var vnet = command.Subcommands.Should().ContainSingle().Subject; + vnet.Name.Should().Be("vnet"); + vnet.Subcommands.Select(c => c.Name).Should().BeEquivalentTo("link", "unlink", "status"); + } + + [Fact] + public void LinkSubcommand_RequiresPolicyArmIdAndOffersTheDocumentedOptions() + { + var link = CreateCommand().Subcommands[0].Subcommands.Single(c => c.Name == "link"); + + link.Options.Select(o => o.Name).Should() + .BeEquivalentTo("policy-arm-id", "swap", "tenant-id", "wait", "verbose"); + link.Options.Single(o => o.Name == "policy-arm-id").IsRequired.Should().BeTrue(); + link.Options.Single(o => o.Name == "swap").IsRequired.Should().BeFalse(); + } + + [Fact] + public void UnlinkSubcommand_TakesNoPolicyBecauseThePlatformStoredIt() + { + var unlink = CreateCommand().Subcommands[0].Subcommands.Single(c => c.Name == "unlink"); + + unlink.Options.Select(o => o.Name).Should().BeEquivalentTo("wait", "verbose"); + } + + [Fact] + public void StatusSubcommand_AcceptsAnOperationHandle() + { + var status = CreateCommand().Subcommands[0].Subcommands.Single(c => c.Name == "status"); + + status.Options.Select(o => o.Name).Should().BeEquivalentTo("operation-id", "verbose"); + } + + [Fact] + public void LinkSubcommand_ParsesItsOptions() + { + var command = CreateCommand(); + + var parsed = command.Parse("vnet link --policy-arm-id /p/1 --swap --tenant-id tid --wait"); + + parsed.Errors.Should().BeEmpty(); + } + + [Fact] + public void LinkSubcommand_WithoutPolicyArmId_FailsToParse() + { + var command = CreateCommand(); + + var parsed = command.Parse("vnet link"); + + parsed.Errors.Should().NotBeEmpty(because: "--policy-arm-id is required"); + } + + // ───────────────────────────────── ReportAsync ────────────────────────────── + + [Fact] + public async Task ReportAsync_WhenResultNull_ReturnsFailure() + { + var vnet = Substitute.For(); + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result: null, wait: true, "Link", CancellationToken.None); + + exitCode.Should().Be(1); + await vnet.DidNotReceive().WaitForCompletionAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ReportAsync_WhenSettled_ReturnsSuccessWithoutWaiting() + { + var vnet = Substitute.For(); + var result = new VNetStatusResponse { Status = "Linked" }; + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + + exitCode.Should().Be(0); + await vnet.DidNotReceive().WaitForCompletionAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ReportAsync_WhenFailed_ReturnsFailure() + { + var vnet = Substitute.For(); + var result = new VNetStatusResponse { Status = "Failed", Reason = "Region mismatch." }; + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result, wait: false, "Link", CancellationToken.None); + + exitCode.Should().Be(1); + } + + [Fact] + public async Task ReportAsync_WhenRunningAndNotWaiting_ReturnsSuccessAndLeavesTheHandle() + { + var vnet = Substitute.For(); + var result = new VNetStatusResponse { Status = "Running", OperationId = OperationId }; + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result, wait: false, "Link", CancellationToken.None); + + exitCode.Should().Be(0, because: "an accepted operation is not itself a failure"); + await vnet.DidNotReceive().WaitForCompletionAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ReportAsync_WhenRunningAndWaiting_PollsThenReportsTheSettledStatus() + { + var vnet = Substitute.For(); + vnet.WaitForCompletionAsync(OperationId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new VNetStatusResponse { Status = "Linked" })); + var result = new VNetStatusResponse { Status = "Running", OperationId = OperationId }; + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + + exitCode.Should().Be(0); + await vnet.Received(1).WaitForCompletionAsync( + OperationId, Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ReportAsync_WhenWaitSettlesAsFailed_ReturnsFailure() + { + var vnet = Substitute.For(); + vnet.WaitForCompletionAsync(OperationId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult( + new VNetStatusResponse { Status = "Failed", Reason = "Upstream rejected the link." })); + var result = new VNetStatusResponse { Status = "Running", OperationId = OperationId }; + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + + exitCode.Should().Be(1); + } + + [Fact] + public async Task ReportAsync_WhenWaitCannotReadStatus_ReturnsFailure() + { + var vnet = Substitute.For(); + vnet.WaitForCompletionAsync(OperationId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(null)); + var result = new VNetStatusResponse { Status = "Running", OperationId = OperationId }; + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + + exitCode.Should().Be(1); + } + + [Fact] + public async Task ReportAsync_WhenRunningWithoutAHandle_DoesNotWait() + { + var vnet = Substitute.For(); + var result = new VNetStatusResponse { Status = "Running", OperationId = null }; + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + + exitCode.Should().Be(0); + await vnet.DidNotReceive().WaitForCompletionAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ReportAsync_WhenUnlinkSettles_ReturnsSuccess() + { + var vnet = Substitute.For(); + var result = new VNetStatusResponse { Status = "NotLinked" }; + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result, wait: true, "Unlink", CancellationToken.None); + + exitCode.Should().Be(0); + } +} diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs index c003ff7d..a140d33e 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs @@ -300,6 +300,152 @@ private static HttpResponseMessage BuildRoleAssignmentsResponse(string scope, st Content = new StringContent(body) }; } + + // ──────────────────────── GetEnterprisePolicySystemIdAsync ──────────────────────── + + private const string PolicyArmId = + "/subscriptions/sub-123/resourceGroups/rg-test/providers/Microsoft.PowerPlatform/enterprisePolicies/policy-1"; + + private const string PolicySystemId = + "/regions/unitedstates/providers/Microsoft.PowerPlatform/enterprisePolicies/1b2c8a4e-0000-0000-0000-000000000000"; + + private static HttpResponseMessage PolicyResponse(string body) => + new(HttpStatusCode.OK) { Content = new StringContent(body) }; + + [Fact] + public async Task GetEnterprisePolicySystemIdAsync_When200_ReturnsSystemId() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(PolicyResponse( + JsonSerializer.Serialize(new { properties = new { systemId = PolicySystemId } }))); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().Be(PolicySystemId, because: "the systemId is the only value BAP accepts for a link"); + } + + [Fact] + public async Task GetEnterprisePolicySystemIdAsync_RequestsTheArmPolicyResource() + { + HttpRequestMessage? captured = null; + using var handler = new CapturingHttpMessageHandler(r => captured = r); + handler.QueueResponse(PolicyResponse( + JsonSerializer.Serialize(new { properties = new { systemId = PolicySystemId } }))); + var svc = CreateService(handler); + + await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + captured.Should().NotBeNull(); + captured!.Method.Should().Be(HttpMethod.Get); + captured.RequestUri!.ToString().Should().Be( + $"https://management.azure.com{PolicyArmId}?api-version=2020-10-30"); + } + + [Fact] + public async Task GetEnterprisePolicySystemIdAsync_WhenStableApiVersionRejected_RetriesWithPreview() + { + var urls = new List(); + using var handler = new CapturingHttpMessageHandler(r => urls.Add(r.RequestUri!.ToString())); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("") }); + handler.QueueResponse(PolicyResponse( + JsonSerializer.Serialize(new { properties = new { systemId = PolicySystemId } }))); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().Be(PolicySystemId); + urls.Should().HaveCount(2); + urls[0].Should().EndWith("api-version=2020-10-30"); + urls[1].Should().EndWith("api-version=2020-10-30-preview"); + } + + [Fact] + public async Task GetEnterprisePolicySystemIdAsync_WhenEveryApiVersionRejected_ReturnsNull() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("") }); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("") }); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().BeNull(because: "there is no api-version left to try"); + } + + [Theory] + [InlineData(HttpStatusCode.Unauthorized)] + [InlineData(HttpStatusCode.Forbidden)] + [InlineData(HttpStatusCode.NotFound)] + [InlineData(HttpStatusCode.InternalServerError)] + public async Task GetEnterprisePolicySystemIdAsync_WhenNonSuccess_ReturnsNullWithoutRetrying(HttpStatusCode status) + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(status) { Content = new StringContent("") }); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().BeNull(because: "a rejected or missing policy is not an api-version problem"); + } + + [Theory] + [InlineData("{}")] + [InlineData("{\"properties\":{}}")] + [InlineData("{\"properties\":{\"systemId\":\"\"}}")] + [InlineData("{\"properties\":{\"systemId\":\" \"}}")] + public async Task GetEnterprisePolicySystemIdAsync_WhenSystemIdMissing_ReturnsNull(string body) + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(PolicyResponse(body)); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().BeNull(because: "a policy without a systemId is not yet usable for linking"); + } + + [Fact] + public async Task GetEnterprisePolicySystemIdAsync_WhenHttpThrows_ReturnsNull() + { + using var handler = new ThrowingHttpMessageHandler(); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().BeNull(); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public async Task GetEnterprisePolicySystemIdAsync_WhenPolicyArmIdBlank_Throws(string? policyArmId) + { + using var handler = new TestHttpMessageHandler(); + var svc = CreateService(handler); + + var act = async () => await svc.GetEnterprisePolicySystemIdAsync(policyArmId!, TenantId); + + await act.Should().ThrowAsync(); + } + + [Fact] + public async Task GetEnterprisePolicySystemIdAsync_WhenTokenUnavailable_ReturnsNullWithoutCallingArm() + { + using var handler = new TestHttpMessageHandler(); + var auth = Substitute.For(); + auth.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(string.Empty)); + var svc = new ArmApiService(NullLogger.Instance, auth, handler, + retryHelper: new RetryHelper(NullLogger.Instance, maxRetries: 1, baseDelaySeconds: 0)); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().BeNull(); + handler.RequestCount.Should().Be(0, because: "without a token there is nothing worth sending"); + } } /// diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs new file mode 100644 index 00000000..df602d60 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs @@ -0,0 +1,427 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Net; +using System.Text.Json; +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; + +/// +/// Unit tests for VNetLinkService. +/// Uses TestHttpMessageHandler / CapturingHttpMessageHandler (defined in GraphApiServiceTests.cs, +/// same assembly) to inject fake platform responses. +/// +public class VNetLinkServiceTests +{ + private const string TenantId = "tid"; + + private const string PolicyArmId = + "/subscriptions/sub-123/resourceGroups/rg-test/providers/Microsoft.PowerPlatform/enterprisePolicies/policy-1"; + + private const string PolicySystemId = + "/regions/unitedstates/providers/Microsoft.PowerPlatform/enterprisePolicies/1b2c8a4e-0000-0000-0000-000000000000"; + + private const string OperationId = "op-abc"; + + private static IAuthenticationService FakeAuth(string token = "fake-a365-token") + { + var mock = Substitute.For(); + mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(token)); + return mock; + } + + private static ArmApiService FakeArm(string? systemId = PolicySystemId) + { + var arm = Substitute.For(); + arm.GetEnterprisePolicySystemIdAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(systemId)); + return arm; + } + + private static VNetLinkService CreateService( + HttpMessageHandler handler, + ArmApiService? arm = null, + IAuthenticationService? auth = null) => + new(NullLogger.Instance, auth ?? FakeAuth(), arm ?? FakeArm(), "prod", handler); + + private static HttpResponseMessage StatusResponse( + HttpStatusCode code, + string? status = null, + string? operationId = null, + string? policyArmId = null, + string? reason = null) => + new(code) + { + Content = new StringContent(JsonSerializer.Serialize(new + { + status, + policyArmId, + operationId, + reason, + })), + }; + + // ──────────────────────────────── IsRunning ──────────────────────────────── + + [Theory] + [InlineData("Running", true)] + [InlineData("running", true)] + [InlineData("RUNNING", true)] + [InlineData("Linked", false)] + [InlineData("NotLinked", false)] + [InlineData("Failed", false)] + [InlineData("Unknown", false)] + [InlineData("", false)] + [InlineData(null, false)] + public void IsRunning_ClassifiesStatus(string? status, bool expected) + { + VNetLinkService.IsRunning(status).Should().Be(expected); + } + + // ──────────────────────────────── LinkAsync ──────────────────────────────── + + [Fact] + public async Task LinkAsync_SendsResolvedSystemIdNotTheArmId() + { + HttpRequestMessage? captured = null; + string? body = null; + using var handler = new CapturingHttpMessageHandler(r => + { + captured = r; + body = r.Content?.ReadAsStringAsync().GetAwaiter().GetResult(); + }); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked")); + var svc = CreateService(handler); + + var result = await svc.LinkAsync(PolicyArmId, swap: true, TenantId); + + result.Should().NotBeNull(); + result!.Status.Should().Be("Linked"); + result.PolicyArmId.Should().BeNull(); + result.OperationId.Should().BeNull(); + result.Reason.Should().BeNull(); + + captured.Should().NotBeNull(); + captured!.Method.Should().Be(HttpMethod.Post); + captured.RequestUri!.AbsolutePath.Should().Be("/agents/vnet/link"); + + body.Should().NotBeNull(); + using var doc = JsonDocument.Parse(body!); + doc.RootElement.GetProperty("policySystemId").GetString().Should().Be(PolicySystemId); + doc.RootElement.GetProperty("policyArmId").GetString().Should().Be(PolicyArmId); + doc.RootElement.GetProperty("swap").GetBoolean().Should().BeTrue(); + } + + [Fact] + public async Task LinkAsync_WhenSwapNotRequested_SendsSwapFalse() + { + string? body = null; + using var handler = new CapturingHttpMessageHandler(r => + body = r.Content?.ReadAsStringAsync().GetAwaiter().GetResult()); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked")); + var svc = CreateService(handler); + + await svc.LinkAsync(PolicyArmId, swap: false, TenantId); + + using var doc = JsonDocument.Parse(body!); + doc.RootElement.GetProperty("swap").GetBoolean().Should().BeFalse(); + } + + [Fact] + public async Task LinkAsync_When202_ReturnsRunningWithOperationId() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.Accepted, "Running", OperationId)); + var svc = CreateService(handler); + + var result = await svc.LinkAsync(PolicyArmId, swap: false, TenantId); + + result.Should().NotBeNull(); + result!.Status.Should().Be("Running"); + result.OperationId.Should().Be(OperationId); + result.PolicyArmId.Should().BeNull(); + result.Reason.Should().BeNull(); + } + + [Fact] + public async Task LinkAsync_WhenSystemIdCannotBeResolved_DoesNotCallThePlatform() + { + using var handler = new TestHttpMessageHandler(); + var svc = CreateService(handler, FakeArm(systemId: null)); + + var result = await svc.LinkAsync(PolicyArmId, swap: false, TenantId); + + result.Should().BeNull(); + handler.RequestCount.Should().Be(0, because: "there is no systemId to send"); + } + + [Theory] + [InlineData(HttpStatusCode.BadRequest)] + [InlineData(HttpStatusCode.Forbidden)] + [InlineData(HttpStatusCode.NotFound)] + [InlineData(HttpStatusCode.Conflict)] + [InlineData(HttpStatusCode.BadGateway)] + public async Task LinkAsync_WhenPlatformFails_ReturnsNull(HttpStatusCode status) + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(status) + { + Content = new StringContent(JsonSerializer.Serialize(new { error = "nope" })), + }); + var svc = CreateService(handler); + + var result = await svc.LinkAsync(PolicyArmId, swap: false, TenantId); + + result.Should().BeNull(); + } + + [Fact] + public async Task LinkAsync_WhenErrorBodyIsNotJson_StillReturnsNull() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadGateway) + { + Content = new StringContent("gateway"), + }); + var svc = CreateService(handler); + + var result = await svc.LinkAsync(PolicyArmId, swap: false, TenantId); + + result.Should().BeNull(because: "a non-JSON body means something upstream of the platform answered"); + } + + [Fact] + public async Task LinkAsync_WhenTokenUnavailable_ReturnsNullWithoutCallingThePlatform() + { + using var handler = new TestHttpMessageHandler(); + var svc = CreateService(handler, auth: FakeAuth(string.Empty)); + + var result = await svc.LinkAsync(PolicyArmId, swap: false, TenantId); + + result.Should().BeNull(); + handler.RequestCount.Should().Be(0); + } + + [Fact] + public async Task LinkAsync_WhenHttpThrows_ReturnsNull() + { + using var handler = new ThrowingHttpMessageHandler(); + var svc = CreateService(handler); + + var result = await svc.LinkAsync(PolicyArmId, swap: false, TenantId); + + result.Should().BeNull(); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public async Task LinkAsync_WhenPolicyArmIdBlank_Throws(string? policyArmId) + { + using var handler = new TestHttpMessageHandler(); + var svc = CreateService(handler); + + var act = async () => await svc.LinkAsync(policyArmId!, swap: false, TenantId); + + await act.Should().ThrowAsync(); + } + + // ─────────────────────────────── UnlinkAsync ─────────────────────────────── + + [Fact] + public async Task UnlinkAsync_PostsToUnlinkWithNoBody() + { + HttpRequestMessage? captured = null; + using var handler = new CapturingHttpMessageHandler(r => captured = r); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "NotLinked")); + var svc = CreateService(handler); + + var result = await svc.UnlinkAsync(); + + result.Should().NotBeNull(); + result!.Status.Should().Be("NotLinked"); + result.PolicyArmId.Should().BeNull(); + result.OperationId.Should().BeNull(); + result.Reason.Should().BeNull(); + + captured!.Method.Should().Be(HttpMethod.Post); + captured.RequestUri!.AbsolutePath.Should().Be("/agents/vnet/unlink"); + captured.Content.Should().BeNull(because: "the platform supplies the stored policy itself"); + } + + [Fact] + public async Task UnlinkAsync_WhenPlatformFails_ReturnsNull() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Conflict) + { + Content = new StringContent(JsonSerializer.Serialize(new { error = "no stored policy" })), + }); + var svc = CreateService(handler); + + var result = await svc.UnlinkAsync(); + + result.Should().BeNull(); + } + + // ────────────────────────────── GetStatusAsync ───────────────────────────── + + [Fact] + public async Task GetStatusAsync_WithoutOperationId_OmitsTheQueryString() + { + HttpRequestMessage? captured = null; + using var handler = new CapturingHttpMessageHandler(r => captured = r); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked", policyArmId: PolicyArmId)); + var svc = CreateService(handler); + + var result = await svc.GetStatusAsync(); + + result.Should().NotBeNull(); + result!.Status.Should().Be("Linked"); + result.PolicyArmId.Should().Be(PolicyArmId); + result.OperationId.Should().BeNull(); + result.Reason.Should().BeNull(); + + captured!.Method.Should().Be(HttpMethod.Get); + captured.RequestUri!.AbsolutePath.Should().Be("/agents/vnet/status"); + captured.RequestUri.Query.Should().BeEmpty(); + } + + [Fact] + public async Task GetStatusAsync_WithOperationId_EscapesItIntoTheQueryString() + { + HttpRequestMessage? captured = null; + using var handler = new CapturingHttpMessageHandler(r => captured = r); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Running", "a b/c")); + var svc = CreateService(handler); + + await svc.GetStatusAsync("a b/c"); + + captured!.RequestUri!.Query.Should().Be("?operationId=a%20b%2Fc"); + } + + [Fact] + public async Task GetStatusAsync_WhenBodyEmpty_ReturnsEmptyStatus() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(string.Empty) }); + var svc = CreateService(handler); + + var result = await svc.GetStatusAsync(); + + result.Should().NotBeNull(); + result!.Status.Should().BeNull(); + result.PolicyArmId.Should().BeNull(); + result.OperationId.Should().BeNull(); + result.Reason.Should().BeNull(); + } + + [Fact] + public async Task GetStatusAsync_SurfacesTheFailureReason() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Failed", OperationId, reason: "Region mismatch.")); + var svc = CreateService(handler); + + var result = await svc.GetStatusAsync(OperationId); + + result.Should().NotBeNull(); + result!.Status.Should().Be("Failed"); + result.Reason.Should().Be("Region mismatch."); + result.OperationId.Should().Be(OperationId); + result.PolicyArmId.Should().BeNull(); + } + + // ───────────────────────── WaitForCompletionAsync ────────────────────────── + + [Fact] + public async Task WaitForCompletionAsync_ReturnsAsSoonAsTheOperationSettles() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked", OperationId)); + var svc = CreateService(handler); + + var result = await svc.WaitForCompletionAsync(OperationId, TimeSpan.FromMinutes(5)); + + result.Should().NotBeNull(); + result!.Status.Should().Be("Linked"); + handler.RequestCount.Should().Be(1, because: "a settled operation needs no second poll"); + } + + [Fact] + public async Task WaitForCompletionAsync_WhenStillRunningAndBudgetExhausted_ReturnsRunning() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Running", OperationId)); + var svc = CreateService(handler); + + // A zero budget cannot fit another poll interval, so the first read is also the last. + var result = await svc.WaitForCompletionAsync(OperationId, TimeSpan.Zero); + + result.Should().NotBeNull(); + result!.Status.Should().Be("Running"); + result.OperationId.Should().Be(OperationId); + handler.RequestCount.Should().Be(1); + } + + [Fact] + public async Task WaitForCompletionAsync_WhenStatusCannotBeRead_ReturnsNull() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadGateway) + { + Content = new StringContent(JsonSerializer.Serialize(new { error = "upstream" })), + }); + var svc = CreateService(handler); + + var result = await svc.WaitForCompletionAsync(OperationId, TimeSpan.FromMinutes(5)); + + result.Should().BeNull(); + handler.RequestCount.Should().Be(1, because: "an unreadable status is terminal for the wait"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public async Task WaitForCompletionAsync_WhenOperationIdBlank_Throws(string? operationId) + { + using var handler = new TestHttpMessageHandler(); + var svc = CreateService(handler); + + var act = async () => await svc.WaitForCompletionAsync(operationId!, TimeSpan.FromMinutes(5)); + + await act.Should().ThrowAsync(); + } + + // ───────────────────────────── Constructor guards ────────────────────────── + + [Fact] + public void Constructor_WhenLoggerNull_Throws() + { + var act = () => new VNetLinkService(null!, FakeAuth(), FakeArm()); + act.Should().Throw(); + } + + [Fact] + public void Constructor_WhenAuthServiceNull_Throws() + { + var act = () => new VNetLinkService(NullLogger.Instance, null!, FakeArm()); + act.Should().Throw(); + } + + [Fact] + public void Constructor_WhenArmApiServiceNull_Throws() + { + var act = () => new VNetLinkService(NullLogger.Instance, FakeAuth(), null!); + act.Should().Throw(); + } +} From 5132585022fc4594b9d9503b34ba7d7c65826780 Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Wed, 23 Sep 2026 17:48:29 -0700 Subject: [PATCH 02/10] fix: address review feedback on a365 network vnet Pin the enterprise-policy ARM id to its expected shape before concatenating it onto the ARM base URL. The base has no trailing slash and the ARM bearer token is a default request header, so `--policy-arm-id "@evil.example/x"` produced `https://management.azure.com@evil.example/x` -- userinfo, not host -- and sent the token to the attacker. This was the only call site building a URL from caller input. Also: - Treat `NotStarted` as in-flight, matching what network.md documents. - Acquire the Agent 365 token for the resolved tenant rather than the signed-in default. `unlink` and `status` gain `--tenant-id` so they can do the same. - Confirm before `--swap` and `unlink`, with `--yes` for automation. Plain `link` is not gated: a different existing link is reported as a conflict rather than replaced, so it is not destructive. - Reject an explicitly blank `--tenant-id` instead of silently falling back. - Use `CommandNames.Network` rather than a literal. - Drive the handlers through `InvokeAsync` in tests. The previous doc comment claimed `ReportAsync` covered them, but tenant resolution, service calls and exit codes were untested. - Stop the VNet tests shelling out to `az account show` via `AzCliHelper`'s static cache, using the repo's existing `loginHintResolver` seam. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- docs/commands/network.md | 13 +- .../Commands/NetworkCommand.cs | 164 +++++++-- .../Program.cs | 2 +- .../Services/ArmApiService.cs | 21 ++ .../Services/IVNetLinkService.cs | 9 +- .../Services/VNetLinkService.cs | 34 +- .../Commands/NetworkCommandTests.cs | 316 ++++++++++++++++-- .../Services/ArmApiServiceTests.cs | 27 +- .../Services/VNetLinkServiceTests.cs | 98 +++++- 10 files changed, 604 insertions(+), 82 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 320e9957..528f22d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,7 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g **Option B — CLI** (`a365 setup admin`) has been removed in this release. Use Option A above, or copy the PowerShell instructions printed in the `a365 setup all` summary output. ### Added -- `a365 network vnet link|unlink|status` — links an Azure virtual network to Agent 365 through a Power Platform NetworkInjection enterprise policy. Replaces `Enable-SubnetInjection` from the `Microsoft.PowerPlatform.EnterprisePolicies` module, which cannot be used because it requires the id of the Agent 365 managed environment and that id is not published. The CLI reads the policy's `systemId` from Azure with your existing `az login` and the platform performs the link against the environment it resolves for your tenant. Requires Global Administrator or Power Platform Administrator. See [docs/commands/network.md](docs/commands/network.md). +- `a365 network vnet link|unlink|status` — links an Azure virtual network to Agent 365 through a Power Platform NetworkInjection enterprise policy, replacing `Enable-SubnetInjection` (#494). Requires Global Administrator or Power Platform Administrator. See [docs/commands/network.md](docs/commands/network.md). - Setup and bootstrap now use Microsoft's first-party Agent 365 CLI application when it is present in your tenant, validating it without changing Microsoft's app registration, and fall back to a tenant-owned "Agent 365 CLI" app when it is not (#489). - Log separator written at the start of each CLI invocation now redacts values for secret-bearing options (e.g. `--idp-client-secret`) so they are not written to the log file in plain text. - Authentication context (tenant and user) is now logged at the `Information` level whenever the resolved sign-in identity changes, giving operators a clear audit trail in the log file of who the CLI is acting as, without exposing credentials. diff --git a/docs/commands/network.md b/docs/commands/network.md index 31123e53..0fdcb746 100644 --- a/docs/commands/network.md +++ b/docs/commands/network.md @@ -39,30 +39,32 @@ subnets, delegate them to `Microsoft.PowerPlatform/enterprisePolicies`, and crea ### `link` ```bash -a365 network vnet link --policy-arm-id [--swap] [--tenant-id ] [--wait] +a365 network vnet link --policy-arm-id [--swap] [--tenant-id ] [--wait] [--yes] ``` | Option | Description | | --- | --- | | `--policy-arm-id`, `-p` | **Required.** ARM resource id of the policy, as returned by `New-SubnetInjectionEnterprisePolicy`. | | `--swap` | Replace an existing link to a *different* policy. Without it, a different existing link is reported as a conflict instead of being silently replaced. | -| `--tenant-id` | Tenant to authenticate against for the Azure policy read. Defaults to the tenant of your current `az login`. | +| `--tenant-id` | Tenant to authenticate against. Defaults to the tenant of your current `az login`. | | `--wait` | Poll until the operation settles instead of returning an operation id. | +| `--yes`, `-y` | Skip the confirmation prompt shown for `--swap`. | Linking the policy that is already linked is a no-op and succeeds without `--swap`. ### `unlink` ```bash -a365 network vnet unlink [--wait] +a365 network vnet unlink [--tenant-id ] [--wait] [--yes] ``` -Unlink needs no policy id — the platform remembers which policy it linked. +Unlink needs no policy id — the platform remembers which policy it linked. It prompts before +removing the link; pass `--yes` in automation. ### `status` ```bash -a365 network vnet status [--operation-id ] +a365 network vnet status [--operation-id ] [--tenant-id ] ``` Without `--operation-id`, reports the environment's current link. With one, reports that specific @@ -100,6 +102,7 @@ a365 network vnet status | Symptom | Cause | | --- | --- | | `Could not determine your Azure tenant` | No `az login` session. Run `az login`, or pass `--tenant-id`. | +| `--tenant-id was supplied but is empty` | `--tenant-id` was passed with a blank value. Pass a tenant id, or omit the option entirely. | | `403` from the platform | Caller is not a Global or Power Platform Administrator, or the CLI app lacks consent for the `AgentTools.VNet.*` scopes. | | Conflict reported on `link` | A *different* policy is already linked. Re-run with `--swap`, or `unlink` first. | | Policy read fails | The policy ARM id is wrong, or your `az login` identity cannot read it. | diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs index 9f18d8ee..73ca8516 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.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.Models; using Microsoft.Agents.A365.DevTools.Cli.Services; using Microsoft.Extensions.Logging; @@ -27,27 +28,85 @@ public static class NetworkCommand public static Command CreateCommand( ILogger logger, IVNetLinkService vnetLinkService, - IAzureCliService azureCliService) + IAzureCliService azureCliService, + IConfirmationProvider confirmationProvider) { - var networkCommand = new Command("network", "Configure tenant networking for Agent 365"); + var networkCommand = new Command(CommandNames.Network, "Configure tenant networking for Agent 365"); var vnetCommand = new Command( "vnet", "Link an Azure virtual network enterprise policy to your Agent 365 environment. " + "Requires the Global Administrator or Power Platform Administrator role."); - vnetCommand.AddCommand(CreateLinkSubcommand(logger, vnetLinkService, azureCliService)); - vnetCommand.AddCommand(CreateUnlinkSubcommand(logger, vnetLinkService)); - vnetCommand.AddCommand(CreateStatusSubcommand(logger, vnetLinkService)); + vnetCommand.AddCommand(CreateLinkSubcommand(logger, vnetLinkService, azureCliService, confirmationProvider)); + vnetCommand.AddCommand(CreateUnlinkSubcommand(logger, vnetLinkService, azureCliService, confirmationProvider)); + vnetCommand.AddCommand(CreateStatusSubcommand(logger, vnetLinkService, azureCliService)); networkCommand.AddCommand(vnetCommand); return networkCommand; } + /// + /// Resolves the tenant to authenticate against, or logs why it could not and returns null. + /// + /// + /// An explicitly blank --tenant-id is treated as a mistake rather than as a request for + /// the default. Falling back silently would run a tenant-wide change against whichever tenant + /// az happens to be signed in to, which is not what someone who typed the option meant. + /// + internal static async Task ResolveTenantIdAsync( + ILogger logger, + IAzureCliService azureCliService, + string? tenantIdOption) + { + if (tenantIdOption is not null) + { + if (string.IsNullOrWhiteSpace(tenantIdOption)) + { + logger.LogError( + "--tenant-id was supplied but is empty. Pass a tenant id, or omit the option " + + "to use the tenant of your current az login."); + return null; + } + + return tenantIdOption; + } + + var account = await azureCliService.GetCurrentAccountAsync(); + var tenantId = account?.TenantId; + if (string.IsNullOrWhiteSpace(tenantId)) + { + logger.LogError("Could not determine your Azure tenant. Run 'az login', or pass --tenant-id."); + return null; + } + + return tenantId; + } + + /// + /// Asks the operator to confirm a change to tenant-wide networking, naming the tenant and the + /// action so the prompt is answerable without scrolling back. + /// + internal static async Task ConfirmChangeAsync( + IConfirmationProvider confirmationProvider, + bool yes, + string action, + string tenantId) + { + if (yes) + { + return true; + } + + return await confirmationProvider.ConfirmAsync( + $"{action} for tenant {tenantId}. This changes networking for every Agent 365 agent in the tenant. Continue?"); + } + private static Command CreateLinkSubcommand( ILogger logger, IVNetLinkService vnetLinkService, - IAzureCliService azureCliService) + IAzureCliService azureCliService, + IConfirmationProvider confirmationProvider) { var command = new Command( "link", @@ -79,40 +138,55 @@ private static Command CreateLinkSubcommand( var verboseOption = new Option(["--verbose", "-v"], "Enable verbose logging"); + var yesOption = new Option( + ["--yes", "-y"], + "Skip the confirmation prompt shown when --swap would replace an existing link."); + command.AddOption(policyArmIdOption); command.AddOption(swapOption); command.AddOption(tenantIdOption); command.AddOption(waitOption); + command.AddOption(yesOption); command.AddOption(verboseOption); command.SetHandler(async (InvocationContext context) => { var policyArmId = context.ParseResult.GetValueForOption(policyArmIdOption)!; var swap = context.ParseResult.GetValueForOption(swapOption); - var tenantId = context.ParseResult.GetValueForOption(tenantIdOption); + var tenantIdOptionValue = context.ParseResult.GetValueForOption(tenantIdOption); var wait = context.ParseResult.GetValueForOption(waitOption); + var yes = context.ParseResult.GetValueForOption(yesOption); var ct = context.GetCancellationToken(); - if (string.IsNullOrWhiteSpace(tenantId)) + var tenantId = await ResolveTenantIdAsync(logger, azureCliService, tenantIdOptionValue); + if (tenantId == null) + { + context.ExitCode = 1; + return; + } + + // Only --swap needs confirming: without it an existing different link is reported as a + // conflict rather than replaced, so the command is already non-destructive. + if (swap && !await ConfirmChangeAsync( + confirmationProvider, yes, "Replace the existing virtual network link", tenantId)) { - var account = await azureCliService.GetCurrentAccountAsync(); - tenantId = account?.TenantId; - if (string.IsNullOrWhiteSpace(tenantId)) - { - logger.LogError("Could not determine your Azure tenant. Run 'az login', or pass --tenant-id."); - context.ExitCode = 1; - return; - } + logger.LogInformation("Cancelled."); + context.ExitCode = 1; + return; } var result = await vnetLinkService.LinkAsync(policyArmId, swap, tenantId, ct); - context.ExitCode = await ReportAsync(logger, vnetLinkService, result, wait, "Link", ct); + context.ExitCode = await ReportAsync(logger, vnetLinkService, result, wait, "Link", tenantId, ct); }); return command; } - private static Command CreateUnlinkSubcommand(ILogger logger, IVNetLinkService vnetLinkService) + private static Command CreateUnlinkSubcommand( + ILogger logger, + IVNetLinkService vnetLinkService, + IAzureCliService azureCliService, + IConfirmationProvider confirmationProvider) { var command = new Command( "unlink", @@ -122,24 +196,54 @@ private static Command CreateUnlinkSubcommand(ILogger logger, IVNetLinkService v "--wait", "Keep polling until the unlink settles, instead of returning an operation id."); + var tenantIdOption = new Option( + "--tenant-id", + "Tenant to authenticate against. Defaults to the tenant of your current az login."); + + var yesOption = new Option( + ["--yes", "-y"], + "Skip the confirmation prompt."); + var verboseOption = new Option(["--verbose", "-v"], "Enable verbose logging"); command.AddOption(waitOption); + command.AddOption(tenantIdOption); + command.AddOption(yesOption); command.AddOption(verboseOption); command.SetHandler(async (InvocationContext context) => { var wait = context.ParseResult.GetValueForOption(waitOption); + var tenantIdOptionValue = context.ParseResult.GetValueForOption(tenantIdOption); + var yes = context.ParseResult.GetValueForOption(yesOption); var ct = context.GetCancellationToken(); - var result = await vnetLinkService.UnlinkAsync(ct); - context.ExitCode = await ReportAsync(logger, vnetLinkService, result, wait, "Unlink", ct); + var tenantId = await ResolveTenantIdAsync(logger, azureCliService, tenantIdOptionValue); + if (tenantId == null) + { + context.ExitCode = 1; + return; + } + + if (!await ConfirmChangeAsync( + confirmationProvider, yes, "Remove the virtual network link", tenantId)) + { + logger.LogInformation("Cancelled."); + context.ExitCode = 1; + return; + } + + var result = await vnetLinkService.UnlinkAsync(tenantId, ct); + context.ExitCode = await ReportAsync(logger, vnetLinkService, result, wait, "Unlink", tenantId, ct); }); return command; } - private static Command CreateStatusSubcommand(ILogger logger, IVNetLinkService vnetLinkService) + private static Command CreateStatusSubcommand( + ILogger logger, + IVNetLinkService vnetLinkService, + IAzureCliService azureCliService) { var command = new Command( "status", @@ -149,17 +253,30 @@ private static Command CreateStatusSubcommand(ILogger logger, IVNetLinkService v "--operation-id", "Operation handle returned by a link or unlink that was still running."); + var tenantIdOption = new Option( + "--tenant-id", + "Tenant to authenticate against. Defaults to the tenant of your current az login."); + var verboseOption = new Option(["--verbose", "-v"], "Enable verbose logging"); command.AddOption(operationIdOption); + command.AddOption(tenantIdOption); command.AddOption(verboseOption); command.SetHandler(async (InvocationContext context) => { var operationId = context.ParseResult.GetValueForOption(operationIdOption); + var tenantIdOptionValue = context.ParseResult.GetValueForOption(tenantIdOption); var ct = context.GetCancellationToken(); - var status = await vnetLinkService.GetStatusAsync(operationId, ct); + var tenantId = await ResolveTenantIdAsync(logger, azureCliService, tenantIdOptionValue); + if (tenantId == null) + { + context.ExitCode = 1; + return; + } + + var status = await vnetLinkService.GetStatusAsync(tenantId, operationId, ct); if (status == null) { context.ExitCode = 1; @@ -183,6 +300,7 @@ internal static async Task ReportAsync( VNetStatusResponse? result, bool wait, string operationLabel, + string tenantId, CancellationToken cancellationToken) { if (result == null) @@ -193,7 +311,7 @@ internal static async Task ReportAsync( if (wait && VNetLinkService.IsRunning(result.Status) && !string.IsNullOrWhiteSpace(result.OperationId)) { logger.LogInformation("{Operation} is running. Waiting for it to settle...", operationLabel); - result = await vnetLinkService.WaitForCompletionAsync(result.OperationId, DefaultWaitTimeout, cancellationToken); + result = await vnetLinkService.WaitForCompletionAsync(tenantId, result.OperationId, DefaultWaitTimeout, cancellationToken); if (result == null) { diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs index 566a313c..e0e08538 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs @@ -188,7 +188,7 @@ await Task.WhenAll( var networkLogger = serviceProvider.GetRequiredService().CreateLogger("network"); var vnetLinkService = serviceProvider.GetRequiredService(); var azureCliService = serviceProvider.GetRequiredService(); - rootCommand.AddCommand(NetworkCommand.CreateCommand(networkLogger, vnetLinkService, azureCliService)); + rootCommand.AddCommand(NetworkCommand.CreateCommand(networkLogger, vnetLinkService, azureCliService, confirmationProvider)); // Build pipeline manually so we can skip UseTypoCorrections() ("Did you mean?" noise) // and UseParseErrorReporting() (full help dump on any parse error), replacing both diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs index fee4ef14..06d5efd2 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs @@ -8,6 +8,7 @@ using System.Net; using System.Net.Http.Headers; using System.Text.Json; +using System.Text.RegularExpressions; namespace Microsoft.Agents.A365.DevTools.Cli.Services; @@ -28,6 +29,15 @@ public class ArmApiService : IDisposable // Stable first: the module's own ARM templates deploy enterprise policies at 2020-10-30. private static readonly string[] EnterprisePolicyApiVersions = ["2020-10-30", "2020-10-30-preview"]; + // ArmBaseUrl has no trailing slash and the ARM bearer token is set as a default request + // header, so a policy id that does not begin with "/subscriptions/" can retarget the whole + // request: "@evil.example/x" concatenates to "https://management.azure.com@evil.example/x", + // where "management.azure.com" is userinfo and the host is the attacker's. Pinning the shape + // is what keeps the token pointed at ARM. + private static readonly Regex EnterprisePolicyArmIdPattern = new( + @"^/subscriptions/[0-9a-fA-F-]{36}/resourceGroups/[^/?#]+/providers/Microsoft\.PowerPlatform/enterprisePolicies/[^/?#]+$", + RegexOptions.Compiled | RegexOptions.CultureInvariant); + private readonly ILogger _logger; private readonly HttpClient _httpClient; private readonly IAuthenticationService _authService; @@ -266,6 +276,17 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke if (string.IsNullOrWhiteSpace(policyArmId)) throw new ArgumentException("Policy ARM id is required.", nameof(policyArmId)); + if (!EnterprisePolicyArmIdPattern.IsMatch(policyArmId)) + { + _logger.LogError( + "'{PolicyArmId}' is not an enterprise policy ARM id. Expected " + + "/subscriptions/{{subscriptionId}}/resourceGroups/{{group}}/providers/" + + "Microsoft.PowerPlatform/enterprisePolicies/{{name}}, as returned by " + + "New-SubnetInjectionEnterprisePolicy.", + policyArmId); + return null; + } + if (!await EnsureArmHeadersAsync(tenantId, ct)) return null; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs index ff0037ee..eb1854da 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs @@ -29,28 +29,35 @@ public interface IVNetLinkService /// /// Removes the current link. The platform supplies the policy identifier it stored at link time. /// + /// Tenant to authenticate against. /// Cancellation token. /// The resulting status, or null when the operation could not be started. - Task UnlinkAsync(CancellationToken cancellationToken = default); + Task UnlinkAsync( + string tenantId, + CancellationToken cancellationToken = default); /// /// Reads the current link status, optionally resuming a specific operation handle. /// + /// Tenant to authenticate against. /// Handle returned by a link or unlink that was still running. /// Cancellation token. /// The current status, or null when it could not be read. Task GetStatusAsync( + string tenantId, string? operationId = null, CancellationToken cancellationToken = default); /// /// Polls status until the operation reaches a terminal state or the timeout elapses. /// + /// Tenant to authenticate against. /// Handle of the running operation. /// How long to keep polling. /// Cancellation token. /// The last status read, which may still be Running if the timeout elapsed. Task WaitForCompletionAsync( + string tenantId, string operationId, TimeSpan timeout, CancellationToken cancellationToken = default); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs index f8e19e1d..047d976c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs @@ -38,19 +38,22 @@ public class VNetLinkService : IVNetLinkService private readonly ArmApiService _armApiService; private readonly string _environment; private readonly HttpMessageHandler? _handler; + private readonly Func> _loginHintResolver; public VNetLinkService( ILogger logger, IAuthenticationService authService, ArmApiService armApiService, string environment = "prod", - HttpMessageHandler? handler = null) + HttpMessageHandler? handler = null, + Func>? loginHintResolver = null) { _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _authService = authService ?? throw new ArgumentNullException(nameof(authService)); _armApiService = armApiService ?? throw new ArgumentNullException(nameof(armApiService)); _environment = environment ?? "prod"; _handler = handler; + _loginHintResolver = loginHintResolver ?? AzCliHelper.ResolveLoginHintAsync; } /// @@ -79,18 +82,21 @@ public VNetLinkService( }; _logger.LogInformation("Linking the policy to your Agent 365 environment..."); - return await SendAsync(HttpMethod.Post, LinkPath, request, "link virtual network", cancellationToken); + return await SendAsync(HttpMethod.Post, LinkPath, request, "link virtual network", tenantId, cancellationToken); } /// - public async Task UnlinkAsync(CancellationToken cancellationToken = default) + public async Task UnlinkAsync( + string tenantId, + CancellationToken cancellationToken = default) { _logger.LogInformation("Removing the virtual network link from your Agent 365 environment..."); - return await SendAsync(HttpMethod.Post, UnlinkPath, payload: null, "unlink virtual network", cancellationToken); + return await SendAsync(HttpMethod.Post, UnlinkPath, payload: null, "unlink virtual network", tenantId, cancellationToken); } /// public async Task GetStatusAsync( + string tenantId, string? operationId = null, CancellationToken cancellationToken = default) { @@ -98,11 +104,12 @@ public VNetLinkService( ? StatusPath : $"{StatusPath}?operationId={Uri.EscapeDataString(operationId)}"; - return await SendAsync(HttpMethod.Get, path, payload: null, "read virtual network status", cancellationToken); + return await SendAsync(HttpMethod.Get, path, payload: null, "read virtual network status", tenantId, cancellationToken); } /// public async Task WaitForCompletionAsync( + string tenantId, string operationId, TimeSpan timeout, CancellationToken cancellationToken = default) @@ -117,7 +124,7 @@ public VNetLinkService( while (true) { - last = await GetStatusAsync(operationId, cancellationToken); + last = await GetStatusAsync(tenantId, operationId, cancellationToken); if (last == null || !IsRunning(last.Status)) return last; @@ -132,15 +139,20 @@ public VNetLinkService( /// /// True when the reported status means the operation has not settled yet. + /// + /// The platform reports a queued operation as NotStarted, which is as unsettled as Running: + /// treating it as terminal would make --wait return before the work had begun. /// public static bool IsRunning(string? status) => - string.Equals(status, "Running", StringComparison.OrdinalIgnoreCase); + string.Equals(status, "Running", StringComparison.OrdinalIgnoreCase) + || string.Equals(status, "NotStarted", StringComparison.OrdinalIgnoreCase); private async Task SendAsync( HttpMethod method, string path, object? payload, string operationName, + string tenantId, CancellationToken cancellationToken) { var correlationId = HttpClientFactory.GenerateCorrelationId(); @@ -150,8 +162,12 @@ public static bool IsRunning(string? status) => try { var audience = ConfigConstants.GetAgent365ToolsResourceAppId(_environment); - var loginHint = await AzCliHelper.ResolveLoginHintAsync(); - var authToken = await _authService.GetAccessTokenAsync(audience, userId: loginHint, ct: cancellationToken); + var loginHint = await _loginHintResolver(); + + // The tenant matters as much here as on the ARM read: without it MSAL falls back to + // the common authority with only a login hint, so on a machine with several cached + // accounts the platform call can land in a different tenant than the policy read. + var authToken = await _authService.GetAccessTokenAsync(audience, tenantId, userId: loginHint, ct: cancellationToken); if (string.IsNullOrWhiteSpace(authToken)) { _logger.LogError("Failed to acquire an Agent 365 access token."); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs index 68834a02..e9e8fe37 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs @@ -14,19 +14,42 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; /// -/// Unit tests for the network command tree and its result reporting. -/// The subcommand handlers themselves are exercised through ReportAsync, which holds the -/// wait-and-exit-code logic; the handlers around it only parse options. +/// Unit tests for the network command tree, its handlers and its result reporting. +/// Handlers are driven through InvokeAsync against substituted services so that tenant +/// resolution, confirmation and exit codes are covered, not just option parsing. /// public class NetworkCommandTests { private const string OperationId = "op-abc"; + private const string TenantId = "tid"; - private static Command CreateCommand(IVNetLinkService? vnet = null, IAzureCliService? azure = null) => + private const string PolicyArmId = + "/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p"; + + private static Command CreateCommand( + IVNetLinkService? vnet = null, + IAzureCliService? azure = null, + IConfirmationProvider? confirmation = null) => NetworkCommand.CreateCommand( NullLogger.Instance, vnet ?? Substitute.For(), - azure ?? Substitute.For()); + azure ?? SignedInAzureCli(), + confirmation ?? Confirming(true)); + + private static IAzureCliService SignedInAzureCli(string? tenantId = TenantId) + { + var azure = Substitute.For(); + azure.GetCurrentAccountAsync().Returns( + Task.FromResult(tenantId == null ? null : new AzureAccountInfo { TenantId = tenantId })); + return azure; + } + + private static IConfirmationProvider Confirming(bool answer) + { + var confirmation = Substitute.For(); + confirmation.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(answer)); + return confirmation; + } // ──────────────────────────── Command tree shape ──────────────────────────── @@ -48,7 +71,7 @@ public void LinkSubcommand_RequiresPolicyArmIdAndOffersTheDocumentedOptions() var link = CreateCommand().Subcommands[0].Subcommands.Single(c => c.Name == "link"); link.Options.Select(o => o.Name).Should() - .BeEquivalentTo("policy-arm-id", "swap", "tenant-id", "wait", "verbose"); + .BeEquivalentTo("policy-arm-id", "swap", "tenant-id", "wait", "yes", "verbose"); link.Options.Single(o => o.Name == "policy-arm-id").IsRequired.Should().BeTrue(); link.Options.Single(o => o.Name == "swap").IsRequired.Should().BeFalse(); } @@ -58,7 +81,7 @@ public void UnlinkSubcommand_TakesNoPolicyBecauseThePlatformStoredIt() { var unlink = CreateCommand().Subcommands[0].Subcommands.Single(c => c.Name == "unlink"); - unlink.Options.Select(o => o.Name).Should().BeEquivalentTo("wait", "verbose"); + unlink.Options.Select(o => o.Name).Should().BeEquivalentTo("wait", "tenant-id", "yes", "verbose"); } [Fact] @@ -66,7 +89,7 @@ public void StatusSubcommand_AcceptsAnOperationHandle() { var status = CreateCommand().Subcommands[0].Subcommands.Single(c => c.Name == "status"); - status.Options.Select(o => o.Name).Should().BeEquivalentTo("operation-id", "verbose"); + status.Options.Select(o => o.Name).Should().BeEquivalentTo("operation-id", "tenant-id", "verbose"); } [Fact] @@ -89,6 +112,244 @@ public void LinkSubcommand_WithoutPolicyArmId_FailsToParse() parsed.Errors.Should().NotBeEmpty(because: "--policy-arm-id is required"); } + // ──────────────────────────── Handler invocation ──────────────────────────── + + [Fact] + public async Task LinkHandler_ResolvesTheTenantFromAzLoginAndCallsTheService() + { + var vnet = Substitute.For(); + vnet.LinkAsync(PolicyArmId, false, TenantId, Arg.Any()) + .Returns(Task.FromResult(new VNetStatusResponse { Status = "Linked" })); + var command = CreateCommand(vnet); + + var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId}"); + + exitCode.Should().Be(0); + await vnet.Received(1).LinkAsync(PolicyArmId, false, TenantId, Arg.Any()); + } + + [Fact] + public async Task LinkHandler_PrefersAnExplicitTenantOverTheAzLoginTenant() + { + var vnet = Substitute.For(); + vnet.LinkAsync(PolicyArmId, false, "other-tenant", Arg.Any()) + .Returns(Task.FromResult(new VNetStatusResponse { Status = "Linked" })); + var command = CreateCommand(vnet); + + var exitCode = await command.InvokeAsync( + $"vnet link --policy-arm-id {PolicyArmId} --tenant-id other-tenant"); + + exitCode.Should().Be(0); + await vnet.Received(1).LinkAsync(PolicyArmId, false, "other-tenant", Arg.Any()); + } + + [Fact] + public async Task LinkHandler_WithoutSwap_DoesNotPrompt() + { + var vnet = Substitute.For(); + vnet.LinkAsync(PolicyArmId, false, TenantId, Arg.Any()) + .Returns(Task.FromResult(new VNetStatusResponse { Status = "Linked" })); + var confirmation = Confirming(false); + var command = CreateCommand(vnet, confirmation: confirmation); + + var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId}"); + + exitCode.Should().Be(0, because: "a conflicting link is reported, not replaced, without --swap"); + await confirmation.DidNotReceive().ConfirmAsync(Arg.Any()); + } + + [Fact] + public async Task LinkHandler_WhenSwapDeclined_DoesNotCallTheService() + { + var vnet = Substitute.For(); + var command = CreateCommand(vnet, confirmation: Confirming(false)); + + var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId} --swap"); + + exitCode.Should().Be(1); + await vnet.DidNotReceive().LinkAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task LinkHandler_WhenSwapAndYes_SkipsThePromptAndCallsTheService() + { + var vnet = Substitute.For(); + vnet.LinkAsync(PolicyArmId, true, TenantId, Arg.Any()) + .Returns(Task.FromResult(new VNetStatusResponse { Status = "Linked" })); + var confirmation = Confirming(false); + var command = CreateCommand(vnet, confirmation: confirmation); + + var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId} --swap --yes"); + + exitCode.Should().Be(0); + await confirmation.DidNotReceive().ConfirmAsync(Arg.Any()); + await vnet.Received(1).LinkAsync(PolicyArmId, true, TenantId, Arg.Any()); + } + + [Fact] + public async Task LinkHandler_WhenTheServiceFails_ReturnsFailure() + { + var vnet = Substitute.For(); + vnet.LinkAsync(PolicyArmId, false, TenantId, Arg.Any()) + .Returns(Task.FromResult(null)); + var command = CreateCommand(vnet); + + var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId}"); + + exitCode.Should().Be(1); + } + + [Fact] + public async Task LinkHandler_WithWait_PollsUntilTheOperationSettles() + { + var vnet = Substitute.For(); + vnet.LinkAsync(PolicyArmId, false, TenantId, Arg.Any()) + .Returns(Task.FromResult( + new VNetStatusResponse { Status = "Running", OperationId = OperationId })); + vnet.WaitForCompletionAsync(TenantId, OperationId, Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(new VNetStatusResponse { Status = "Linked" })); + var command = CreateCommand(vnet); + + var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId} --wait"); + + exitCode.Should().Be(0); + await vnet.Received(1).WaitForCompletionAsync( + TenantId, OperationId, Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task LinkHandler_WhenNoTenantCanBeResolved_FailsWithoutCallingTheService() + { + var vnet = Substitute.For(); + var command = CreateCommand(vnet, SignedInAzureCli(tenantId: null)); + + var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId}"); + + exitCode.Should().Be(1); + await vnet.DidNotReceive().LinkAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task LinkHandler_WhenTenantIdSuppliedButBlank_FailsWithoutFallingBackToAzLogin() + { + var vnet = Substitute.For(); + var azure = SignedInAzureCli(); + var command = CreateCommand(vnet, azure); + + var exitCode = await command.InvokeAsync( + ["vnet", "link", "--policy-arm-id", PolicyArmId, "--tenant-id", " "]); + + exitCode.Should().Be(1, because: "a blank tenant is a mistake, not a request for the default"); + await azure.DidNotReceive().GetCurrentAccountAsync(); + await vnet.DidNotReceive().LinkAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task UnlinkHandler_PromptsThenCallsTheService() + { + var vnet = Substitute.For(); + vnet.UnlinkAsync(TenantId, Arg.Any()) + .Returns(Task.FromResult(new VNetStatusResponse { Status = "NotLinked" })); + var confirmation = Confirming(true); + var command = CreateCommand(vnet, confirmation: confirmation); + + var exitCode = await command.InvokeAsync("vnet unlink"); + + exitCode.Should().Be(0); + await confirmation.Received(1).ConfirmAsync(Arg.Any()); + await vnet.Received(1).UnlinkAsync(TenantId, Arg.Any()); + } + + [Fact] + public async Task UnlinkHandler_WhenDeclined_DoesNotCallTheService() + { + var vnet = Substitute.For(); + var command = CreateCommand(vnet, confirmation: Confirming(false)); + + var exitCode = await command.InvokeAsync("vnet unlink"); + + exitCode.Should().Be(1); + await vnet.DidNotReceive().UnlinkAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task UnlinkHandler_WithYes_SkipsThePrompt() + { + var vnet = Substitute.For(); + vnet.UnlinkAsync(TenantId, Arg.Any()) + .Returns(Task.FromResult(new VNetStatusResponse { Status = "NotLinked" })); + var confirmation = Confirming(false); + var command = CreateCommand(vnet, confirmation: confirmation); + + var exitCode = await command.InvokeAsync("vnet unlink --yes"); + + exitCode.Should().Be(0); + await confirmation.DidNotReceive().ConfirmAsync(Arg.Any()); + await vnet.Received(1).UnlinkAsync(TenantId, Arg.Any()); + } + + [Fact] + public async Task StatusHandler_ReadsTheCurrentLinkWithoutPrompting() + { + var vnet = Substitute.For(); + vnet.GetStatusAsync(TenantId, null, Arg.Any()) + .Returns(Task.FromResult( + new VNetStatusResponse { Status = "Linked", PolicyArmId = PolicyArmId })); + var confirmation = Confirming(false); + var command = CreateCommand(vnet, confirmation: confirmation); + + var exitCode = await command.InvokeAsync("vnet status"); + + exitCode.Should().Be(0); + await confirmation.DidNotReceive().ConfirmAsync(Arg.Any()); + await vnet.Received(1).GetStatusAsync(TenantId, null, Arg.Any()); + } + + [Fact] + public async Task StatusHandler_PassesTheOperationHandleThrough() + { + var vnet = Substitute.For(); + vnet.GetStatusAsync(TenantId, OperationId, Arg.Any()) + .Returns(Task.FromResult( + new VNetStatusResponse { Status = "Running", OperationId = OperationId })); + var command = CreateCommand(vnet); + + var exitCode = await command.InvokeAsync($"vnet status --operation-id {OperationId}"); + + exitCode.Should().Be(0); + await vnet.Received(1).GetStatusAsync(TenantId, OperationId, Arg.Any()); + } + + [Fact] + public async Task StatusHandler_WhenFailed_ReturnsFailure() + { + var vnet = Substitute.For(); + vnet.GetStatusAsync(TenantId, null, Arg.Any()) + .Returns(Task.FromResult( + new VNetStatusResponse { Status = "Failed", Reason = "Region mismatch." })); + var command = CreateCommand(vnet); + + var exitCode = await command.InvokeAsync("vnet status"); + + exitCode.Should().Be(1); + } + + [Fact] + public async Task StatusHandler_WhenStatusUnreadable_ReturnsFailure() + { + var vnet = Substitute.For(); + vnet.GetStatusAsync(TenantId, null, Arg.Any()) + .Returns(Task.FromResult(null)); + var command = CreateCommand(vnet); + + var exitCode = await command.InvokeAsync("vnet status"); + + exitCode.Should().Be(1); + } + // ───────────────────────────────── ReportAsync ────────────────────────────── [Fact] @@ -97,11 +358,10 @@ public async Task ReportAsync_WhenResultNull_ReturnsFailure() var vnet = Substitute.For(); var exitCode = await NetworkCommand.ReportAsync( - NullLogger.Instance, vnet, result: null, wait: true, "Link", CancellationToken.None); + NullLogger.Instance, vnet, result: null, wait: true, "Link", TenantId, CancellationToken.None); exitCode.Should().Be(1); - await vnet.DidNotReceive().WaitForCompletionAsync( - Arg.Any(), Arg.Any(), Arg.Any()); + await vnet.DidNotReceive().WaitForCompletionAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } [Fact] @@ -111,11 +371,10 @@ public async Task ReportAsync_WhenSettled_ReturnsSuccessWithoutWaiting() var result = new VNetStatusResponse { Status = "Linked" }; var exitCode = await NetworkCommand.ReportAsync( - NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + NullLogger.Instance, vnet, result, wait: true, "Link", TenantId, CancellationToken.None); exitCode.Should().Be(0); - await vnet.DidNotReceive().WaitForCompletionAsync( - Arg.Any(), Arg.Any(), Arg.Any()); + await vnet.DidNotReceive().WaitForCompletionAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } [Fact] @@ -125,7 +384,7 @@ public async Task ReportAsync_WhenFailed_ReturnsFailure() var result = new VNetStatusResponse { Status = "Failed", Reason = "Region mismatch." }; var exitCode = await NetworkCommand.ReportAsync( - NullLogger.Instance, vnet, result, wait: false, "Link", CancellationToken.None); + NullLogger.Instance, vnet, result, wait: false, "Link", TenantId, CancellationToken.None); exitCode.Should().Be(1); } @@ -137,40 +396,38 @@ public async Task ReportAsync_WhenRunningAndNotWaiting_ReturnsSuccessAndLeavesTh var result = new VNetStatusResponse { Status = "Running", OperationId = OperationId }; var exitCode = await NetworkCommand.ReportAsync( - NullLogger.Instance, vnet, result, wait: false, "Link", CancellationToken.None); + NullLogger.Instance, vnet, result, wait: false, "Link", TenantId, CancellationToken.None); exitCode.Should().Be(0, because: "an accepted operation is not itself a failure"); - await vnet.DidNotReceive().WaitForCompletionAsync( - Arg.Any(), Arg.Any(), Arg.Any()); + await vnet.DidNotReceive().WaitForCompletionAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } [Fact] public async Task ReportAsync_WhenRunningAndWaiting_PollsThenReportsTheSettledStatus() { var vnet = Substitute.For(); - vnet.WaitForCompletionAsync(OperationId, Arg.Any(), Arg.Any()) + vnet.WaitForCompletionAsync(TenantId, OperationId, Arg.Any(), Arg.Any()) .Returns(Task.FromResult(new VNetStatusResponse { Status = "Linked" })); var result = new VNetStatusResponse { Status = "Running", OperationId = OperationId }; var exitCode = await NetworkCommand.ReportAsync( - NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + NullLogger.Instance, vnet, result, wait: true, "Link", TenantId, CancellationToken.None); exitCode.Should().Be(0); - await vnet.Received(1).WaitForCompletionAsync( - OperationId, Arg.Any(), Arg.Any()); + await vnet.Received(1).WaitForCompletionAsync(TenantId, OperationId, Arg.Any(), Arg.Any()); } [Fact] public async Task ReportAsync_WhenWaitSettlesAsFailed_ReturnsFailure() { var vnet = Substitute.For(); - vnet.WaitForCompletionAsync(OperationId, Arg.Any(), Arg.Any()) + vnet.WaitForCompletionAsync(TenantId, OperationId, Arg.Any(), Arg.Any()) .Returns(Task.FromResult( new VNetStatusResponse { Status = "Failed", Reason = "Upstream rejected the link." })); var result = new VNetStatusResponse { Status = "Running", OperationId = OperationId }; var exitCode = await NetworkCommand.ReportAsync( - NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + NullLogger.Instance, vnet, result, wait: true, "Link", TenantId, CancellationToken.None); exitCode.Should().Be(1); } @@ -179,12 +436,12 @@ public async Task ReportAsync_WhenWaitSettlesAsFailed_ReturnsFailure() public async Task ReportAsync_WhenWaitCannotReadStatus_ReturnsFailure() { var vnet = Substitute.For(); - vnet.WaitForCompletionAsync(OperationId, Arg.Any(), Arg.Any()) + vnet.WaitForCompletionAsync(TenantId, OperationId, Arg.Any(), Arg.Any()) .Returns(Task.FromResult(null)); var result = new VNetStatusResponse { Status = "Running", OperationId = OperationId }; var exitCode = await NetworkCommand.ReportAsync( - NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + NullLogger.Instance, vnet, result, wait: true, "Link", TenantId, CancellationToken.None); exitCode.Should().Be(1); } @@ -196,11 +453,10 @@ public async Task ReportAsync_WhenRunningWithoutAHandle_DoesNotWait() var result = new VNetStatusResponse { Status = "Running", OperationId = null }; var exitCode = await NetworkCommand.ReportAsync( - NullLogger.Instance, vnet, result, wait: true, "Link", CancellationToken.None); + NullLogger.Instance, vnet, result, wait: true, "Link", TenantId, CancellationToken.None); exitCode.Should().Be(0); - await vnet.DidNotReceive().WaitForCompletionAsync( - Arg.Any(), Arg.Any(), Arg.Any()); + await vnet.DidNotReceive().WaitForCompletionAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } [Fact] @@ -210,7 +466,7 @@ public async Task ReportAsync_WhenUnlinkSettles_ReturnsSuccess() var result = new VNetStatusResponse { Status = "NotLinked" }; var exitCode = await NetworkCommand.ReportAsync( - NullLogger.Instance, vnet, result, wait: true, "Unlink", CancellationToken.None); + NullLogger.Instance, vnet, result, wait: true, "Unlink", TenantId, CancellationToken.None); exitCode.Should().Be(0); } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs index a140d33e..c6dfa2c2 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs @@ -304,7 +304,7 @@ private static HttpResponseMessage BuildRoleAssignmentsResponse(string scope, st // ──────────────────────── GetEnterprisePolicySystemIdAsync ──────────────────────── private const string PolicyArmId = - "/subscriptions/sub-123/resourceGroups/rg-test/providers/Microsoft.PowerPlatform/enterprisePolicies/policy-1"; + "/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg-test/providers/Microsoft.PowerPlatform/enterprisePolicies/policy-1"; private const string PolicySystemId = "/regions/unitedstates/providers/Microsoft.PowerPlatform/enterprisePolicies/1b2c8a4e-0000-0000-0000-000000000000"; @@ -312,6 +312,31 @@ private static HttpResponseMessage BuildRoleAssignmentsResponse(string scope, st private static HttpResponseMessage PolicyResponse(string body) => new(HttpStatusCode.OK) { Content = new StringContent(body) }; + [Theory] + // Userinfo trick: `management.azure.com` becomes the username and the real host is the attacker's. + [InlineData("@evil.example/x")] + [InlineData("evil.example/x")] + [InlineData("//evil.example/x")] + [InlineData("https://evil.example/x")] + [InlineData("/subscriptions/not-a-guid/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.Storage/storageAccounts/acct")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p?x=1")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p#frag")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p/../../x")] + public async Task GetEnterprisePolicySystemIdAsync_WhenArmIdIsNotAnEnterprisePolicyPath_RejectsWithoutCalling( + string policyArmId) + { + using var handler = new TestHttpMessageHandler(); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(policyArmId, TenantId); + + result.Should().BeNull(); + handler.RequestCount.Should().Be( + 0, + because: "the ARM bearer token is a default header, so a redirected host would receive it"); + } + [Fact] public async Task GetEnterprisePolicySystemIdAsync_When200_ReturnsSystemId() { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs index df602d60..b13cc86b 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs @@ -49,7 +49,19 @@ private static VNetLinkService CreateService( HttpMessageHandler handler, ArmApiService? arm = null, IAuthenticationService? auth = null) => - new(NullLogger.Instance, auth ?? FakeAuth(), arm ?? FakeArm(), "prod", handler); + new( + NullLogger.Instance, + auth ?? FakeAuth(), + arm ?? FakeArm(), + "prod", + handler, + NoLoginHint); + + /// + /// Stands in for the real resolver so the tests never shell out to `az account show`. + /// The production default caches in a static field shared with AzCliHelperTests. + /// + private static Task NoLoginHint() => Task.FromResult(null); private static HttpResponseMessage StatusResponse( HttpStatusCode code, @@ -74,6 +86,8 @@ private static HttpResponseMessage StatusResponse( [InlineData("Running", true)] [InlineData("running", true)] [InlineData("RUNNING", true)] + [InlineData("NotStarted", true)] + [InlineData("notstarted", true)] [InlineData("Linked", false)] [InlineData("NotLinked", false)] [InlineData("Failed", false)] @@ -244,7 +258,7 @@ public async Task UnlinkAsync_PostsToUnlinkWithNoBody() handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "NotLinked")); var svc = CreateService(handler); - var result = await svc.UnlinkAsync(); + var result = await svc.UnlinkAsync(TenantId); result.Should().NotBeNull(); result!.Status.Should().Be("NotLinked"); @@ -267,7 +281,7 @@ public async Task UnlinkAsync_WhenPlatformFails_ReturnsNull() }); var svc = CreateService(handler); - var result = await svc.UnlinkAsync(); + var result = await svc.UnlinkAsync(TenantId); result.Should().BeNull(); } @@ -282,7 +296,7 @@ public async Task GetStatusAsync_WithoutOperationId_OmitsTheQueryString() handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked", policyArmId: PolicyArmId)); var svc = CreateService(handler); - var result = await svc.GetStatusAsync(); + var result = await svc.GetStatusAsync(TenantId); result.Should().NotBeNull(); result!.Status.Should().Be("Linked"); @@ -303,7 +317,7 @@ public async Task GetStatusAsync_WithOperationId_EscapesItIntoTheQueryString() handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Running", "a b/c")); var svc = CreateService(handler); - await svc.GetStatusAsync("a b/c"); + await svc.GetStatusAsync(TenantId, "a b/c"); captured!.RequestUri!.Query.Should().Be("?operationId=a%20b%2Fc"); } @@ -315,7 +329,7 @@ public async Task GetStatusAsync_WhenBodyEmpty_ReturnsEmptyStatus() handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(string.Empty) }); var svc = CreateService(handler); - var result = await svc.GetStatusAsync(); + var result = await svc.GetStatusAsync(TenantId); result.Should().NotBeNull(); result!.Status.Should().BeNull(); @@ -331,7 +345,7 @@ public async Task GetStatusAsync_SurfacesTheFailureReason() handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Failed", OperationId, reason: "Region mismatch.")); var svc = CreateService(handler); - var result = await svc.GetStatusAsync(OperationId); + var result = await svc.GetStatusAsync(TenantId, OperationId); result.Should().NotBeNull(); result!.Status.Should().Be("Failed"); @@ -349,7 +363,7 @@ public async Task WaitForCompletionAsync_ReturnsAsSoonAsTheOperationSettles() handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked", OperationId)); var svc = CreateService(handler); - var result = await svc.WaitForCompletionAsync(OperationId, TimeSpan.FromMinutes(5)); + var result = await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.FromMinutes(5)); result.Should().NotBeNull(); result!.Status.Should().Be("Linked"); @@ -364,7 +378,7 @@ public async Task WaitForCompletionAsync_WhenStillRunningAndBudgetExhausted_Retu var svc = CreateService(handler); // A zero budget cannot fit another poll interval, so the first read is also the last. - var result = await svc.WaitForCompletionAsync(OperationId, TimeSpan.Zero); + var result = await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.Zero); result.Should().NotBeNull(); result!.Status.Should().Be("Running"); @@ -382,7 +396,7 @@ public async Task WaitForCompletionAsync_WhenStatusCannotBeRead_ReturnsNull() }); var svc = CreateService(handler); - var result = await svc.WaitForCompletionAsync(OperationId, TimeSpan.FromMinutes(5)); + var result = await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.FromMinutes(5)); result.Should().BeNull(); handler.RequestCount.Should().Be(1, because: "an unreadable status is terminal for the wait"); @@ -397,11 +411,73 @@ public async Task WaitForCompletionAsync_WhenOperationIdBlank_Throws(string? ope using var handler = new TestHttpMessageHandler(); var svc = CreateService(handler); - var act = async () => await svc.WaitForCompletionAsync(operationId!, TimeSpan.FromMinutes(5)); + var act = async () => await svc.WaitForCompletionAsync(TenantId, operationId!, TimeSpan.FromMinutes(5)); await act.Should().ThrowAsync(); } + // ────────────────────────── Token acquisition ────────────────────────────── + + [Fact] + public async Task LinkAsync_AcquiresTheAgent365TokenForTheRequestedTenant() + { + var auth = FakeAuth(); + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked")); + var svc = CreateService(handler, auth: auth); + + await svc.LinkAsync(PolicyArmId, swap: false, "contoso-tenant"); + + await auth.Received(1).GetAccessTokenAsync( + Arg.Any(), + "contoso-tenant", + Arg.Any(), + Arg.Any(), + Arg.Any?>(), + Arg.Any(), + Arg.Any()); + } + + [Fact] + public async Task UnlinkAsync_AcquiresTheAgent365TokenForTheRequestedTenant() + { + var auth = FakeAuth(); + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "NotLinked")); + var svc = CreateService(handler, auth: auth); + + await svc.UnlinkAsync("contoso-tenant"); + + await auth.Received(1).GetAccessTokenAsync( + Arg.Any(), + "contoso-tenant", + Arg.Any(), + Arg.Any(), + Arg.Any?>(), + Arg.Any(), + Arg.Any()); + } + + [Fact] + public async Task GetStatusAsync_AcquiresTheAgent365TokenForTheRequestedTenant() + { + var auth = FakeAuth(); + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked")); + var svc = CreateService(handler, auth: auth); + + await svc.GetStatusAsync("contoso-tenant"); + + await auth.Received(1).GetAccessTokenAsync( + Arg.Any(), + "contoso-tenant", + Arg.Any(), + Arg.Any(), + Arg.Any?>(), + Arg.Any(), + Arg.Any()); + } + // ───────────────────────────── Constructor guards ────────────────────────── [Fact] From 2e6d8966e5767b4e2a08b18fc764640c13feca8c Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Wed, 23 Sep 2026 21:37:16 -0700 Subject: [PATCH 03/10] fix: bound the wait ceiling, propagate cancellation, correct the az login prerequisite The --wait ceiling only bounded the gap between completed polls, so a poll starting just inside the budget could run to the HttpClient's own timeout and overshoot by minutes. The ceiling is now armed on the token each request is made with; a timeout mid-request reports the last known state, while a caller's Ctrl+C still propagates. ArmApiService's broad catch swallowed the OperationCanceledException that RetryHelper deliberately rethrows, so Ctrl+C during the policy read surfaced as "could not read the policy" and link carried on as though the policy did not exist. Both test helpers configured GetAccessTokenAsync without a matcher for its 8th parameter, the CancellationToken, pinning the setup to ct == default. Any call carrying a real token missed the setup and returned null, which the services report as a failed token acquisition -- so a test could not exercise any cancellation path at all. This is why the two new cancellation tests initially failed for the wrong reason. docs: the az login prerequisite claimed it was used "only to read the enterprise policy". It is actually the source of two defaults, the tenant and the signed-in account, and --tenant-id overrides only the first. Tokens are never borrowed from Azure CLI -- both the ARM read and the Agent 365 call acquire their own. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/commands/network.md | 5 +- .../Services/ArmApiService.cs | 7 +++ .../Services/VNetLinkService.cs | 23 ++++++- .../Services/ArmApiServiceTests.cs | 35 ++++++++++- .../Services/VNetLinkServiceTests.cs | 60 ++++++++++++++++++- 5 files changed, 126 insertions(+), 4 deletions(-) diff --git a/docs/commands/network.md b/docs/commands/network.md index 0fdcb746..b4757275 100644 --- a/docs/commands/network.md +++ b/docs/commands/network.md @@ -23,7 +23,10 @@ subnets, delegate them to `Microsoft.PowerPlatform/enterprisePolicies`, and crea - **Global Administrator** or **Power Platform Administrator** in the tenant. The platform rejects anyone else. -- An active `az login` session in the same tenant. Used only to read the enterprise policy. +- An active `az login` session. It supplies two defaults: the tenant to operate on, and the + signed-in account to authenticate as. `--tenant-id` overrides the first; the account still comes + from `az login`. Tokens are not borrowed from Azure CLI -- both the ARM policy read and the + Agent 365 call acquire their own tokens through the CLI's sign-in. - A NetworkInjection enterprise policy already created by `New-SubnetInjectionEnterprisePolicy`, with subnets delegated to `Microsoft.PowerPlatform/enterprisePolicies`. - Public cloud only. Sovereign clouds are not supported. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs index 06d5efd2..8a599e6f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs @@ -342,6 +342,13 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke _logger.LogDebug("Resolved enterprise policy systemId"); return value; } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // RetryHelper rethrows cancellation deliberately. Swallowing it here would report + // Ctrl+C as "policy not found" and let the caller carry on as if the read had + // simply come back empty. + throw; + } catch (Exception ex) { if (NetworkHelper.IsConnectionResetByProxy(ex)) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs index 047d976c..de56537c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs @@ -119,12 +119,31 @@ public VNetLinkService( // Wall clock, not summed sleeps: each status call costs real time, and a caller who asked // for five minutes should not wait eight because the service was slow. + // + // The stopwatch alone only bounds the gap between completed polls. A poll that starts just + // inside the ceiling can still run to the HttpClient's own timeout, overshooting by minutes, + // so the ceiling is also armed on the token every request is made with. + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeoutCts.CancelAfter(timeout); + var stopwatch = Stopwatch.StartNew(); VNetStatusResponse? last = null; while (true) { - last = await GetStatusAsync(tenantId, operationId, cancellationToken); + try + { + last = await GetStatusAsync(tenantId, operationId, timeoutCts.Token); + } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + // The ceiling elapsed mid-request. That is a timeout, not a failure: report the + // last known state, exactly as the pre-sleep check below does. + _logger.LogInformation( + "Stopped waiting after {Elapsed:0}s. The operation is still running.", + stopwatch.Elapsed.TotalSeconds); + return last; + } if (last == null || !IsRunning(last.Status)) return last; @@ -133,6 +152,8 @@ public VNetLinkService( return last; _logger.LogInformation("Still running... ({Elapsed:0}s elapsed)", stopwatch.Elapsed.TotalSeconds); + // The pre-sleep check above guarantees this delay finishes inside the ceiling, so it + // waits on the caller's token only -- the timeout can't fire here. await Task.Delay(PollInterval, cancellationToken); } } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs index c6dfa2c2..3fbead48 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs @@ -29,8 +29,12 @@ public class ArmApiServiceTests private static IAuthenticationService FakeAuth() { var mock = Substitute.For(); + + // The 8th parameter is the CancellationToken. Without a matcher the setup is pinned to + // ct == default, so any call carrying a real token misses it and the service reports a + // failed token acquisition instead of doing the work under test. mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any(), Arg.Any()) + Arg.Any?>(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult("fake-arm-token")); return mock; } @@ -430,6 +434,35 @@ public async Task GetEnterprisePolicySystemIdAsync_WhenSystemIdMissing_ReturnsNu result.Should().BeNull(because: "a policy without a systemId is not yet usable for linking"); } + [Fact] + public async Task GetEnterprisePolicySystemIdAsync_WhenCallerCancels_PropagatesRatherThanReportingNoPolicy() + { + // RetryHelper rethrows cancellation on purpose. Folding it into the broad catch would + // report Ctrl+C as "could not read the policy" and let link carry on as if the policy + // simply did not exist. + using var handler = new SlowHttpMessageHandler(TimeSpan.FromSeconds(30)); + var svc = CreateService(handler); + using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(200)); + + var act = async () => await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId, cts.Token); + + await act.Should().ThrowAsync(); + } + + /// + /// Holds each request open until the request's own token is cancelled, so a test can observe + /// what the service does with a cancellation raised mid-call. + /// + private sealed class SlowHttpMessageHandler(TimeSpan delay) : HttpMessageHandler + { + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + await Task.Delay(delay, cancellationToken); + return new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent("{}") }; + } + } + [Fact] public async Task GetEnterprisePolicySystemIdAsync_WhenHttpThrows_ReturnsNull() { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs index b13cc86b..bc7783b8 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using System.Diagnostics; using System.Net; using System.Text.Json; using FluentAssertions; @@ -31,8 +32,12 @@ public class VNetLinkServiceTests private static IAuthenticationService FakeAuth(string token = "fake-a365-token") { var mock = Substitute.For(); + + // The 8th parameter is the CancellationToken. Omitting a matcher for it pins the setup to + // ct == default, so any call carrying a real token -- a caller's, or the wait ceiling's -- + // silently misses and returns null, which the service reports as a failed token acquisition. mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any(), Arg.Any()) + Arg.Any?>(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult(token)); return mock; } @@ -416,6 +421,59 @@ public async Task WaitForCompletionAsync_WhenOperationIdBlank_Throws(string? ope await act.Should().ThrowAsync(); } + [Fact] + public async Task WaitForCompletionAsync_WhenCeilingElapsesDuringAPoll_StopsWaitingOnTheInFlightCall() + { + // The pre-sleep stopwatch check only bounds the gap between completed polls. Without the + // ceiling armed on the request's own token, a poll that starts inside the budget runs to + // the HttpClient's timeout -- minutes past what the caller asked for. + using var handler = new SlowHttpMessageHandler( + TimeSpan.FromSeconds(30), + () => StatusResponse(HttpStatusCode.OK, "Running", OperationId)); + var svc = CreateService(handler); + + var stopwatch = Stopwatch.StartNew(); + var result = await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.FromMilliseconds(200)); + stopwatch.Stop(); + + result.Should().BeNull(because: "the ceiling elapsed before any status was read"); + stopwatch.Elapsed.Should().BeLessThan( + TimeSpan.FromSeconds(10), + because: "the wait must abandon the in-flight request rather than block on it"); + } + + [Fact] + public async Task WaitForCompletionAsync_WhenCallerCancels_PropagatesRatherThanReportingATimeout() + { + // The timeout and a Ctrl+C both surface as OperationCanceledException. Only the timeout is + // swallowed into "still running"; a caller cancel has to reach the caller. + using var handler = new SlowHttpMessageHandler( + TimeSpan.FromSeconds(30), + () => StatusResponse(HttpStatusCode.OK, "Running", OperationId)); + var svc = CreateService(handler); + using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(200)); + + var act = async () => await svc.WaitForCompletionAsync( + TenantId, OperationId, TimeSpan.FromMinutes(5), cts.Token); + + await act.Should().ThrowAsync(); + } + + /// + /// Holds each request open for unless the request's own token is + /// cancelled first, so a test can tell "abandoned the call" from "waited for the response". + /// + private sealed class SlowHttpMessageHandler(TimeSpan delay, Func responseFactory) + : HttpMessageHandler + { + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + await Task.Delay(delay, cancellationToken); + return responseFactory(); + } + } + // ────────────────────────── Token acquisition ────────────────────────────── [Fact] From fcfa12176a31c3dc1c204235771134ba5de586a9 Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Wed, 23 Sep 2026 22:07:54 -0700 Subject: [PATCH 04/10] Do not rethrow HttpClient's own timeout as cancellation SendAsync rethrew every OperationCanceledException. HttpClient's own timeout surfaces as one with no token cancelled, so a bare link, unlink or status threw at the caller instead of returning the documented null and logging the failure. It now rethrows only when the supplied token is actually cancelled, which still covers both a Ctrl+C and the wait ceiling firing on its linked token. Same shape as the ArmApiService fix in 2e6d896; caught on the GSA PR, which carries the identical code. 2118 passed, 0 failed, 12 skipped. --- .../Services/VNetLinkService.cs | 6 ++- .../Services/VNetLinkServiceTests.cs | 41 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs index de56537c..e049bbac 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs @@ -225,7 +225,11 @@ public static bool IsRunning(string? status) => ? new VNetStatusResponse() : JsonSerializer.Deserialize(body); } - catch (OperationCanceledException) + // Cancellation is the caller's business, or the wait ceiling firing on a linked token. + // HttpClient's own timeout also surfaces as OperationCanceledException with no token + // cancelled, and that is an ordinary request failure — it belongs in the catch below so it + // is logged and reported, not thrown at whoever called link, unlink or status. + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs index bc7783b8..36eeff61 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs @@ -459,6 +459,47 @@ public async Task WaitForCompletionAsync_WhenCallerCancels_PropagatesRatherThanR await act.Should().ThrowAsync(); } + [Fact] + public async Task GetStatusAsync_WhenTheTransportTimesOutWithNoCancellation_ReturnsNullRatherThanThrowing() + { + // HttpClient's own timeout surfaces as an OperationCanceledException with no token + // cancelled. That is an ordinary request failure, and callers of a bare link, unlink or + // status expect the documented null, not an exception thrown at them. + using var handler = new CancelThrowingHttpMessageHandler(); + var svc = CreateService(handler); + + var result = await svc.GetStatusAsync(TenantId); + + result.Should().BeNull(); + } + + [Fact] + public async Task GetStatusAsync_WhenTheCallerCancels_PropagatesRatherThanReturningNull() + { + using var cts = new CancellationTokenSource(); + await cts.CancelAsync(); + using var handler = new CancelThrowingHttpMessageHandler(); + var svc = CreateService(handler); + + var act = async () => await svc.GetStatusAsync(TenantId, operationId: null, cts.Token); + + await act.Should().ThrowAsync(); + } + + /// + /// Fails every request the way HttpClient's own timeout does — an OperationCanceledException + /// with no token cancelled — so a test can pin how the service classifies it. + /// + private sealed class CancelThrowingHttpMessageHandler : HttpMessageHandler + { + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + throw new TaskCanceledException("The request timed out."); + } + } + /// /// Holds each request open for unless the request's own token is /// cancelled first, so a test can tell "abandoned the call" from "waited for the response". From 419e2f3ed633e50a934c056a1a08c26e7db20c45 Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Wed, 23 Sep 2026 22:39:29 -0700 Subject: [PATCH 05/10] Do not let a per-request client dispose a shared handler CreateAuthenticatedClient built the client with HttpClient's default handler ownership, so a caller that holds one handler and builds a client per request lost the handler to the first client's disposal. VNetLinkService is exactly that shape, so the second poll of WaitForCompletionAsync would have failed with ObjectDisposedException against any handler that honours Dispose. A supplied handler now stays owned by whoever supplied it. Found on #497, which carries the identical factory; not flagged here. --- .../Services/Internal/HttpClientFactory.cs | 9 ++- .../Internal/HttpClientFactoryTests.cs | 50 +++++++++++++++ .../Services/VNetLinkServiceTests.cs | 61 +++++++++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/HttpClientFactory.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/HttpClientFactory.cs index 1b2bb95d..fff80898 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/HttpClientFactory.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/HttpClientFactory.cs @@ -20,6 +20,13 @@ public static class HttpClientFactory /// Optional correlation ID for request tracing. If null, empty, or whitespace, /// a new GUID will be generated automatically. /// + /// + /// Optional message handler. It stays owned by whoever supplied it: the returned client does + /// not dispose it, so one handler can back several clients. Callers that build a client per + /// request from a handler they hold as a field depend on this — the default + /// ownership would let the first client's disposal take the shared + /// handler down and fail every later request with . + /// /// A configured HttpClient instance with the correlation ID applied. public static HttpClient CreateAuthenticatedClient( string? authToken = null, @@ -28,7 +35,7 @@ public static HttpClient CreateAuthenticatedClient( HttpMessageHandler? handler = null) { var client = handler != null - ? new HttpClient(handler) { Timeout = TimeSpan.FromMinutes(2) } + ? new HttpClient(handler, disposeHandler: false) { Timeout = TimeSpan.FromMinutes(2) } : new HttpClient { Timeout = TimeSpan.FromMinutes(2) }; if (!string.IsNullOrWhiteSpace(authToken)) diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Internal/HttpClientFactoryTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Internal/HttpClientFactoryTests.cs index f91a6fa8..f1e81b01 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Internal/HttpClientFactoryTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Internal/HttpClientFactoryTests.cs @@ -243,4 +243,54 @@ public void CreateAuthenticatedClient_WithGeneratedCorrelationId_BothHeadersMatc correlationId.Should().Be(clientRequestId, "Both headers should have the same auto-generated correlation ID"); } + + [Fact] + public void CreateAuthenticatedClient_WithASuppliedHandler_LeavesTheHandlerAliveAfterTheClientIsDisposed() + { + // A supplied handler belongs to the caller, who commonly holds one as a field and builds a + // client per request. HttpClient's default ownership would have the first client's disposal + // take that handler down, failing every later request with ObjectDisposedException. + using var handler = new CountingHttpMessageHandler(); + + using (HttpClientFactory.CreateAuthenticatedClient(handler: handler)) + { + } + + handler.DisposeCount.Should().Be(0); + } + + [Fact] + public void CreateAuthenticatedClient_WithASuppliedHandler_CanBackSeveralClients() + { + using var handler = new CountingHttpMessageHandler(); + + var first = HttpClientFactory.CreateAuthenticatedClient(handler: handler); + using var second = HttpClientFactory.CreateAuthenticatedClient(handler: handler); + + first.Should().NotBeSameAs(second); + + // Disposing one client must not take the shared handler, or the other client is already + // broken before it sends anything. + first.Dispose(); + + handler.DisposeCount.Should().Be(0); + } + + /// + /// Counts disposals so a test can pin who owns a supplied handler. + /// + private sealed class CountingHttpMessageHandler : HttpMessageHandler + { + public int DisposeCount { get; private set; } + + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) => + throw new NotSupportedException("This handler exists only to observe disposal."); + + protected override void Dispose(bool disposing) + { + DisposeCount++; + base.Dispose(disposing); + } + } } \ No newline at end of file diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs index 36eeff61..b1bbaab5 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs @@ -486,6 +486,37 @@ public async Task GetStatusAsync_WhenTheCallerCancels_PropagatesRatherThanReturn await act.Should().ThrowAsync(); } + [Fact] + public async Task GetStatusAsync_CalledTwice_DoesNotDisposeTheInjectedHandlerOnTheFirstCall() + { + // A client is built per request, but the handler is a field and outlives all of them. + // HttpClient's default ownership would have the first client's disposal take the handler + // down with it, so every later request -- including every poll after the first in + // WaitForCompletionAsync -- would fail with ObjectDisposedException. + using var handler = new DisposalAwareHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Running", OperationId)); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked", OperationId)); + var svc = CreateService(handler); + + var first = await svc.GetStatusAsync(TenantId); + var second = await svc.GetStatusAsync(TenantId); + + first.Should().NotBeNull(); + first!.Status.Should().Be("Running"); + first.OperationId.Should().Be(OperationId); + first.PolicyArmId.Should().BeNull(); + first.Reason.Should().BeNull(); + + second.Should().NotBeNull(); + second!.Status.Should().Be("Linked"); + second.OperationId.Should().Be(OperationId); + second.PolicyArmId.Should().BeNull(); + second.Reason.Should().BeNull(); + + handler.DisposeCount.Should().Be(0); + handler.RequestCount.Should().Be(2); + } + /// /// Fails every request the way HttpClient's own timeout does — an OperationCanceledException /// with no token cancelled — so a test can pin how the service classifies it. @@ -515,6 +546,36 @@ protected override async Task SendAsync( } } + /// + /// Mimics a real handler's reaction to being disposed: it counts disposals and refuses to + /// serve afterwards, so a test can prove the service never disposes a handler it does not own. + /// A handler that ignores Dispose would let the defect pass unnoticed. + /// + private sealed class DisposalAwareHttpMessageHandler : HttpMessageHandler + { + private readonly Queue _responses = new(); + + public int DisposeCount { get; private set; } + + public int RequestCount { get; private set; } + + public void QueueResponse(HttpResponseMessage response) => _responses.Enqueue(response); + + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + ObjectDisposedException.ThrowIf(DisposeCount > 0, this); + RequestCount++; + return Task.FromResult(_responses.Dequeue()); + } + + protected override void Dispose(bool disposing) + { + DisposeCount++; + base.Dispose(disposing); + } + } + // ────────────────────────── Token acquisition ────────────────────────────── [Fact] From c6d9f7bf229a91e336314545b3399c1f8eb1485f Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Thu, 24 Sep 2026 05:35:39 -0700 Subject: [PATCH 06/10] fix(cli): make enterprise policy ARM id validation case-insensitive and reject dot segments ARM ids are case-insensitive and the portal, CLI and ARM itself emit different casings, so the case-sensitive literals rejected legitimate ids. Dot segments passed validation and normalized to a different resource path. Whitespace is now excluded from the segment classes; that, not the \z anchor, is what rejects a trailing newline, since the old character class absorbed it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Services/ArmApiService.cs | 12 ++++++-- .../Services/ArmApiServiceTests.cs | 28 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs index 8a599e6f..93285ff9 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs @@ -34,9 +34,17 @@ public class ArmApiService : IDisposable // request: "@evil.example/x" concatenates to "https://management.azure.com@evil.example/x", // where "management.azure.com" is userinfo and the host is the attacker's. Pinning the shape // is what keeps the token pointed at ARM. + // + // IgnoreCase because ARM ids are case-insensitive and are commonly seen as "resourcegroups" + // or "microsoft.powerplatform"; the shape stays pinned either way. Segments exclude + // whitespace as well as delimiters, which is what actually rejects a trailing newline -- + // \z alone does not, because [^/?#]+ would absorb the newline before the anchor is reached. + // The dot-segment lookaheads keep the path from normalizing into a different resource -- + // harmless while the host is fixed, but the validated string should be the string that gets + // requested. private static readonly Regex EnterprisePolicyArmIdPattern = new( - @"^/subscriptions/[0-9a-fA-F-]{36}/resourceGroups/[^/?#]+/providers/Microsoft\.PowerPlatform/enterprisePolicies/[^/?#]+$", - RegexOptions.Compiled | RegexOptions.CultureInvariant); + @"^/subscriptions/[0-9a-f-]{36}/resourceGroups/(?!\.{1,2}(?:/|\z))[^/?#\s]+/providers/Microsoft\.PowerPlatform/enterprisePolicies/(?!\.{1,2}\z)[^/?#\s]+\z", + RegexOptions.Compiled | RegexOptions.CultureInvariant | RegexOptions.IgnoreCase); private readonly ILogger _logger; private readonly HttpClient _httpClient; diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs index 3fbead48..e00a739e 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs @@ -327,6 +327,15 @@ private static HttpResponseMessage PolicyResponse(string body) => [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p?x=1")] [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p#frag")] [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p/../../x")] + // Dot segments normalize the request path into a different resource before it is sent. + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/../providers/Microsoft.PowerPlatform/enterprisePolicies/p")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/./providers/Microsoft.PowerPlatform/enterprisePolicies/p")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/..")] + // .NET's $ matches before a trailing newline, and [^/?#]+ would have absorbed the newline + // anyway, so segments exclude whitespace and the anchor is \z. + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p\n")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p ")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/r g/providers/Microsoft.PowerPlatform/enterprisePolicies/p")] public async Task GetEnterprisePolicySystemIdAsync_WhenArmIdIsNotAnEnterprisePolicyPath_RejectsWithoutCalling( string policyArmId) { @@ -341,6 +350,25 @@ public async Task GetEnterprisePolicySystemIdAsync_WhenArmIdIsNotAnEnterprisePol because: "the ARM bearer token is a default header, so a redirected host would receive it"); } + [Theory] + // ARM ids are case-insensitive, and the portal, the CLI and ARM itself all emit different + // casings of the same id. Rejecting any of them would fail a perfectly valid --policy-arm-id. + [InlineData("/subscriptions/8D1E5B21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourcegroups/rg/providers/microsoft.powerplatform/enterprisepolicies/p")] + [InlineData("/SUBSCRIPTIONS/8D1E5B21-0000-0000-0000-000000000000/RESOURCEGROUPS/RG/PROVIDERS/MICROSOFT.POWERPLATFORM/ENTERPRISEPOLICIES/P")] + public async Task GetEnterprisePolicySystemIdAsync_AcceptsAnyCasingOfTheArmId(string policyArmId) + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(PolicyResponse( + JsonSerializer.Serialize(new { properties = new { systemId = PolicySystemId } }))); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(policyArmId, TenantId); + + result.Should().Be(PolicySystemId); + handler.RequestCount.Should().Be(1); + } + [Fact] public async Task GetEnterprisePolicySystemIdAsync_When200_ReturnsSystemId() { From b0a71d71a535afd8fb7038eeea657a429559c1ad Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Wed, 30 Sep 2026 11:16:01 -0700 Subject: [PATCH 07/10] fix(vnet): reject enterprise policies that are not NetworkInjection The design moved the kind check to the CLI on the grounds that it holds the full policy object and can produce a better error than BAP. It never landed. Every enterprise policy kind carries a systemId, so a CMK or Identity policy read back cleanly and failed later at BAP with an error naming nothing actionable. Six existing tests had ARM payloads with no kind at all, which is why none of them caught it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Services/ArmApiService.cs | 24 ++++++- .../Services/ArmApiServiceTests.cs | 66 ++++++++++++++++--- 2 files changed, 80 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs index 93285ff9..d1217903 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs @@ -272,9 +272,13 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke /// which is not derivable from the ARM resource id. /// /// This read happens in the CLI, using the admin's own Azure session, so the Agent 365 - /// service never needs delegated ARM access. + /// service never needs delegated ARM access. Holding the whole policy object here is also what + /// lets the kind check happen with a useful message: every enterprise policy kind carries a + /// systemId, so a CMK or Identity policy would otherwise sail through to BAP and come back as a + /// generic upstream rejection. /// - /// Returns null when the policy cannot be read or has no systemId; the message is logged. + /// Returns null when the policy cannot be read, is not a NetworkInjection policy, or has no + /// systemId; the message is logged. /// public virtual async Task GetEnterprisePolicySystemIdAsync( string policyArmId, @@ -329,6 +333,22 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke var body = await response.Content.ReadAsStringAsync(ct); using var doc = JsonDocument.Parse(body); + // Every enterprise policy kind has a systemId, so without this the wrong kind reaches + // BAP and returns a generic rejection that names nothing the admin can act on. + var kind = doc.RootElement.TryGetProperty("kind", out var kindElement) + ? kindElement.GetString() + : null; + + if (!string.Equals(kind, "NetworkInjection", StringComparison.OrdinalIgnoreCase)) + { + _logger.LogError( + "Enterprise policy {PolicyArmId} is of kind '{Kind}', not NetworkInjection. " + + "Supply a subnet injection policy, as returned by New-SubnetInjectionEnterprisePolicy.", + policyArmId, + string.IsNullOrWhiteSpace(kind) ? "unknown" : kind); + return null; + } + if (!doc.RootElement.TryGetProperty("properties", out var properties) || !properties.TryGetProperty("systemId", out var systemId)) { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs index e00a739e..7c7a2526 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs @@ -360,7 +360,7 @@ public async Task GetEnterprisePolicySystemIdAsync_AcceptsAnyCasingOfTheArmId(st { using var handler = new TestHttpMessageHandler(); handler.QueueResponse(PolicyResponse( - JsonSerializer.Serialize(new { properties = new { systemId = PolicySystemId } }))); + JsonSerializer.Serialize(new { kind = "NetworkInjection", properties = new { systemId = PolicySystemId } }))); var svc = CreateService(handler); var result = await svc.GetEnterprisePolicySystemIdAsync(policyArmId, TenantId); @@ -374,7 +374,7 @@ public async Task GetEnterprisePolicySystemIdAsync_When200_ReturnsSystemId() { using var handler = new TestHttpMessageHandler(); handler.QueueResponse(PolicyResponse( - JsonSerializer.Serialize(new { properties = new { systemId = PolicySystemId } }))); + JsonSerializer.Serialize(new { kind = "NetworkInjection", properties = new { systemId = PolicySystemId } }))); var svc = CreateService(handler); var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); @@ -388,7 +388,7 @@ public async Task GetEnterprisePolicySystemIdAsync_RequestsTheArmPolicyResource( HttpRequestMessage? captured = null; using var handler = new CapturingHttpMessageHandler(r => captured = r); handler.QueueResponse(PolicyResponse( - JsonSerializer.Serialize(new { properties = new { systemId = PolicySystemId } }))); + JsonSerializer.Serialize(new { kind = "NetworkInjection", properties = new { systemId = PolicySystemId } }))); var svc = CreateService(handler); await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); @@ -406,7 +406,7 @@ public async Task GetEnterprisePolicySystemIdAsync_WhenStableApiVersionRejected_ using var handler = new CapturingHttpMessageHandler(r => urls.Add(r.RequestUri!.ToString())); handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("") }); handler.QueueResponse(PolicyResponse( - JsonSerializer.Serialize(new { properties = new { systemId = PolicySystemId } }))); + JsonSerializer.Serialize(new { kind = "NetworkInjection", properties = new { systemId = PolicySystemId } }))); var svc = CreateService(handler); var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); @@ -447,10 +447,10 @@ public async Task GetEnterprisePolicySystemIdAsync_WhenNonSuccess_ReturnsNullWit } [Theory] - [InlineData("{}")] - [InlineData("{\"properties\":{}}")] - [InlineData("{\"properties\":{\"systemId\":\"\"}}")] - [InlineData("{\"properties\":{\"systemId\":\" \"}}")] + [InlineData("{\"kind\":\"NetworkInjection\"}")] + [InlineData("{\"kind\":\"NetworkInjection\",\"properties\":{}}")] + [InlineData("{\"kind\":\"NetworkInjection\",\"properties\":{\"systemId\":\"\"}}")] + [InlineData("{\"kind\":\"NetworkInjection\",\"properties\":{\"systemId\":\" \"}}")] public async Task GetEnterprisePolicySystemIdAsync_WhenSystemIdMissing_ReturnsNull(string body) { using var handler = new TestHttpMessageHandler(); @@ -462,6 +462,56 @@ public async Task GetEnterprisePolicySystemIdAsync_WhenSystemIdMissing_ReturnsNu result.Should().BeNull(because: "a policy without a systemId is not yet usable for linking"); } + [Theory] + // Every enterprise policy kind carries a systemId, so a wrong-kind policy reads back perfectly + // well. Without the kind check it would reach BAP and return an upstream rejection naming + // nothing the admin can act on. Enable-SubnetInjection makes the same check for the same reason. + [InlineData("Encryption")] + [InlineData("Identity")] + [InlineData("networkinjectionx")] + public async Task GetEnterprisePolicySystemIdAsync_WhenPolicyIsNotNetworkInjection_ReturnsNull(string kind) + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(PolicyResponse( + JsonSerializer.Serialize(new { kind, properties = new { systemId = PolicySystemId } }))); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().BeNull(because: "only a NetworkInjection policy can be linked as a virtual network"); + } + + [Theory] + [InlineData("{\"properties\":{\"systemId\":\"/regions/unitedstates/providers/Microsoft.PowerPlatform/enterprisePolicies/1b2c8a4e-0000-0000-0000-000000000000\"}}")] + [InlineData("{\"kind\":null,\"properties\":{\"systemId\":\"/regions/unitedstates/providers/Microsoft.PowerPlatform/enterprisePolicies/1b2c8a4e-0000-0000-0000-000000000000\"}}")] + [InlineData("{\"kind\":\"\",\"properties\":{\"systemId\":\"/regions/unitedstates/providers/Microsoft.PowerPlatform/enterprisePolicies/1b2c8a4e-0000-0000-0000-000000000000\"}}")] + public async Task GetEnterprisePolicySystemIdAsync_WhenKindAbsent_ReturnsNull(string body) + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(PolicyResponse(body)); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().BeNull(because: "an unstated kind is not proof the policy is a subnet injection policy"); + } + + [Theory] + [InlineData("networkinjection")] + [InlineData("NETWORKINJECTION")] + [InlineData("NetworkInjection")] + public async Task GetEnterprisePolicySystemIdAsync_AcceptsAnyCasingOfKind(string kind) + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(PolicyResponse( + JsonSerializer.Serialize(new { kind, properties = new { systemId = PolicySystemId } }))); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().Be(PolicySystemId, because: "ARM does not guarantee the casing it echoes back"); + } + [Fact] public async Task GetEnterprisePolicySystemIdAsync_WhenCallerCancels_PropagatesRatherThanReportingNoPolicy() { From 9f6fd17f39ef4c8d2f2b9ea332d7ca09f6349b6e Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Wed, 30 Sep 2026 11:16:12 -0700 Subject: [PATCH 08/10] feat(vnet): resolve and send policySystemId on unlink The platform no longer retains the systemId, so a bodyless unlink is now rejected. Resolve it here instead, doing the same two reads Disable-SubnetInjection does internally: read the linked policy's ARM id from the status endpoint, then read that policy from ARM on the admin's own Azure session. No bodyless fallback -- there is nothing on the platform side to fall back to. A failed ARM read names the policy, because the usual cause is that it was deleted from Azure while still linked and the admin cannot act on that without knowing which one. The command surface is unchanged: unlink still takes no policy argument. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/commands/network.md | 7 +- .../Models/VNetModels.cs | 19 ++++ .../Services/VNetLinkService.cs | 29 +++++- .../Services/VNetLinkServiceTests.cs | 96 +++++++++++++++++-- 4 files changed, 140 insertions(+), 11 deletions(-) diff --git a/docs/commands/network.md b/docs/commands/network.md index b4757275..cc7cc9fd 100644 --- a/docs/commands/network.md +++ b/docs/commands/network.md @@ -61,8 +61,11 @@ Linking the policy that is already linked is a no-op and succeeds without `--swa a365 network vnet unlink [--tenant-id ] [--wait] [--yes] ``` -Unlink needs no policy id — the platform remembers which policy it linked. It prompts before -removing the link; pass `--yes` in automation. +Unlink needs no policy id from you, but it does need your Azure session: it reads which policy is +linked, resolves that policy in Azure to get the identifier the platform requires, then unlinks. If +the policy has been deleted from Azure, unlink fails and names it — use the PowerShell +`Disable-SubnetInjection` module instead. It prompts before removing the link; pass `--yes` in +automation. ### `status` diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs index b2701a6d..8f79c655 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs @@ -33,6 +33,25 @@ public class VNetLinkRequest public bool Swap { get; set; } } +/// +/// Request body for unlinking the virtual network enterprise policy from the tenant's Agent 365 +/// Power Platform environment. +/// +/// +/// The platform holds no copy of the systemId, so this is required on every unlink. The CLI reads +/// the linked policy's ARM id from the status endpoint and resolves it against ARM on the caller's +/// own Azure session — the same thing Disable-SubnetInjection does internally. +/// +public class VNetUnlinkRequest +{ + /// + /// The policy's properties.systemId, shaped + /// /regions/{region}/providers/Microsoft.PowerPlatform/enterprisePolicies/{guid}. + /// + [JsonPropertyName("policySystemId")] + public string? PolicySystemId { get; set; } +} + /// /// Status of the tenant's virtual network link, and the shape returned by link and unlink /// once they settle. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs index e049bbac..961282da 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs @@ -90,8 +90,35 @@ public VNetLinkService( string tenantId, CancellationToken cancellationToken = default) { + // The platform keeps no copy of the systemId, so there is nothing to fall back on: resolve it + // here or fail loudly. Disable-SubnetInjection does the same two reads — the environment for + // the ARM id, then ARM for the systemId — just on the other side of the wire. + var status = await GetStatusAsync(tenantId, operationId: null, cancellationToken); + if (status is null) + return null; + + if (string.IsNullOrWhiteSpace(status.PolicyArmId)) + { + _logger.LogInformation("No virtual network policy is linked to your Agent 365 environment."); + return status; + } + + _logger.LogInformation("Reading enterprise policy from Azure..."); + var policySystemId = await _armApiService.GetEnterprisePolicySystemIdAsync(status.PolicyArmId, tenantId, cancellationToken); + if (string.IsNullOrWhiteSpace(policySystemId)) + { + // Naming the policy matters here: the usual cause is that it was deleted from Azure while + // still linked, and the admin cannot act on that without knowing which one. + _logger.LogError( + "Could not resolve the systemId of the linked policy '{PolicyArmId}'. It may have been deleted, or your Azure session may not have access to it.", + status.PolicyArmId); + return null; + } + + var request = new VNetUnlinkRequest { PolicySystemId = policySystemId }; + _logger.LogInformation("Removing the virtual network link from your Agent 365 environment..."); - return await SendAsync(HttpMethod.Post, UnlinkPath, payload: null, "unlink virtual network", tenantId, cancellationToken); + return await SendAsync(HttpMethod.Post, UnlinkPath, request, "unlink virtual network", tenantId, cancellationToken); } /// diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs index b1bbaab5..a25e269d 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs @@ -256,12 +256,22 @@ public async Task LinkAsync_WhenPolicyArmIdBlank_Throws(string? policyArmId) // ─────────────────────────────── UnlinkAsync ─────────────────────────────── [Fact] - public async Task UnlinkAsync_PostsToUnlinkWithNoBody() + public async Task UnlinkAsync_ReadsStatusThenResolvesSystemIdAndSendsIt() { - HttpRequestMessage? captured = null; - using var handler = new CapturingHttpMessageHandler(r => captured = r); + var captured = new List(); + var bodies = new List(); + using var handler = new CapturingHttpMessageHandler(r => + { + captured.Add(r); + + // Read here, not after the call: HttpClient disposes request content once the request + // completes, so the assertion below would otherwise fault on a disposed StringContent. + bodies.Add(r.Content?.ReadAsStringAsync().GetAwaiter().GetResult()); + }); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked", policyArmId: PolicyArmId)); handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "NotLinked")); - var svc = CreateService(handler); + var arm = FakeArm(); + var svc = CreateService(handler, arm); var result = await svc.UnlinkAsync(TenantId); @@ -271,18 +281,88 @@ public async Task UnlinkAsync_PostsToUnlinkWithNoBody() result.OperationId.Should().BeNull(); result.Reason.Should().BeNull(); - captured!.Method.Should().Be(HttpMethod.Post); - captured.RequestUri!.AbsolutePath.Should().Be("/agents/vnet/unlink"); - captured.Content.Should().BeNull(because: "the platform supplies the stored policy itself"); + captured.Should().HaveCount(2); + captured[0].Method.Should().Be(HttpMethod.Get); + captured[0].RequestUri!.AbsolutePath.Should().Be("/agents/vnet/status"); + bodies[0].Should().BeNull(); + + captured[1].Method.Should().Be(HttpMethod.Post); + captured[1].RequestUri!.AbsolutePath.Should().Be("/agents/vnet/unlink"); + bodies[1].Should().Contain("policySystemId").And.Contain(PolicySystemId); + + // The ARM read must use the ARM id the platform reported, not anything the admin typed: + // unlink takes no policy argument at all. + await arm.Received(1).GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId, Arg.Any()); + } + + [Fact] + public async Task UnlinkAsync_WhenNothingLinked_ReturnsStatusWithoutCallingArmOrUnlink() + { + var captured = new List(); + using var handler = new CapturingHttpMessageHandler(r => captured.Add(r)); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "NotLinked")); + var arm = FakeArm(); + var svc = CreateService(handler, arm); + + var result = await svc.UnlinkAsync(TenantId); + + result.Should().NotBeNull(); + result!.Status.Should().Be("NotLinked"); + result.PolicyArmId.Should().BeNull(); + result.OperationId.Should().BeNull(); + result.Reason.Should().BeNull(); + + captured.Should().HaveCount(1); + captured[0].Method.Should().Be(HttpMethod.Get); + await arm.DidNotReceive().GetEnterprisePolicySystemIdAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task UnlinkAsync_WhenStatusFails_ReturnsNullWithoutUnlinking() + { + var captured = new List(); + using var handler = new CapturingHttpMessageHandler(r => captured.Add(r)); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Forbidden) + { + Content = new StringContent(JsonSerializer.Serialize(new { error = "forbidden" })), + }); + var svc = CreateService(handler); + + var result = await svc.UnlinkAsync(TenantId); + + result.Should().BeNull(); + captured.Should().HaveCount(1); + } + + /// + /// The policy was deleted from Azure while still linked, or the caller's Azure session cannot + /// read it. There is no stored copy on the platform to fall back to, so this has to be a clear + /// client-side failure rather than a bodyless request the platform would reject anyway. + /// + [Fact] + public async Task UnlinkAsync_WhenSystemIdCannotBeResolved_ReturnsNullWithoutUnlinking() + { + var captured = new List(); + using var handler = new CapturingHttpMessageHandler(r => captured.Add(r)); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked", policyArmId: PolicyArmId)); + var svc = CreateService(handler, FakeArm(systemId: null)); + + var result = await svc.UnlinkAsync(TenantId); + + result.Should().BeNull(); + captured.Should().HaveCount(1, because: "the unlink must not be sent without a systemId"); + captured[0].Method.Should().Be(HttpMethod.Get); } [Fact] public async Task UnlinkAsync_WhenPlatformFails_ReturnsNull() { using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked", policyArmId: PolicyArmId)); handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Conflict) { - Content = new StringContent(JsonSerializer.Serialize(new { error = "no stored policy" })), + Content = new StringContent(JsonSerializer.Serialize(new { error = "policy mismatch" })), }); var svc = CreateService(handler); From 4c0f5029a84390f2d1ac4ed9fd841825895768d3 Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Wed, 30 Sep 2026 12:52:23 -0700 Subject: [PATCH 09/10] Address review round 3 on the vnet link CLI Security and correctness fixes from DheerajPannala's review: - ArmApiService: tighten the --policy-arm-id regex to a strict GUID plus an ARM-name allowlist. The old [0-9a-f-]{36} / [^/?#\s]+ shape admitted '%' and '\', which Uri unescapes and normalizes before the request is sent -- so an encoded dot segment could redirect the admin's ARM bearer at a different resource. Rejected vectors are pinned as test rows. - ArmApiService: only advance to the next api-version on an api-version error code. Any other 400 is about this request, and retrying buried the real reason behind "Azure rejected every supported api-version". - VNetLinkService: pass authorityHost. This was the only Agent 365 Tools call authenticating against the commercial cloud on a sovereign one. - VNetLinkService: build the endpoint through ConfigConstants rather than a second copy of the origin table. - VNetLinkService: tolerate three consecutive failed status reads. A single transient 5xx reported a running link as failed, with no operation id to resume from. - VNetLinkService: distinguish the two 403s. A scope failure carries required_scope; a role failure does not. Naming the wrong one sent the admin after a consent they cannot grant. - VNetLinkService: honour the caller's token while resolving the login hint. - NetworkCommand: confirm every link, not only --swap. Enabling subnet delegation can destabilise the environment for up to 30 minutes. - NetworkCommand: reject a blank --policy-arm-id, treat Unknown as a failure with tenant-scoping guidance, and name --tenant-id in every resume hint. - docs/commands/network.md: drop the az login contradiction, narrow the login hint claim, replace NotStarted with Unknown in the status table, and use the current New-SubnetInjectionEnterprisePolicy syntax. Tests: widen every FakeAuth matcher to all nine parameters -- authorityHost was the third time an omitted matcher silently pinned a setup to a default and made a success-path test exercise nothing. Pass constructor arguments to Substitute.For so the real AuthenticationService constructor does not run. The wait ceiling is now injectable so retry tolerance is testable without ten-second sleeps. 2420 passed, 0 failed, 12 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/commands/network.md | 40 +++- .../Commands/NetworkCommand.cs | 58 ++++- .../Models/VNetModels.cs | 16 +- .../Services/ArmApiService.cs | 67 ++++-- .../Services/IVNetLinkService.cs | 2 +- .../Services/VNetLinkService.cs | 119 ++++++++-- .../Commands/NetworkCommandTests.cs | 75 +++++- .../Services/ArmApiServiceTests.cs | 72 +++++- .../Services/VNetLinkServiceTests.cs | 222 ++++++++++++++++-- 9 files changed, 582 insertions(+), 89 deletions(-) diff --git a/docs/commands/network.md b/docs/commands/network.md index cc7cc9fd..41128de4 100644 --- a/docs/commands/network.md +++ b/docs/commands/network.md @@ -12,8 +12,8 @@ which takes an `-environmentId`. Agent 365 provisions a managed Power Platform e tenant and does not publish its id, so that final step cannot be run. `a365 network vnet` replaces only that last step. The CLI reads the policy's `systemId` from Azure -using your existing `az login`, then asks the Agent 365 platform to perform the link against the -environment it resolves for your tenant. +Resource Manager with your own sign-in, then asks the Agent 365 platform to perform the link +against the environment it resolves for your tenant. Everything before the final step is unchanged — keep using the PowerShell module to create the subnets, delegate them to `Microsoft.PowerPlatform/enterprisePolicies`, and create the policy with @@ -24,9 +24,11 @@ subnets, delegate them to `Microsoft.PowerPlatform/enterprisePolicies`, and crea - **Global Administrator** or **Power Platform Administrator** in the tenant. The platform rejects anyone else. - An active `az login` session. It supplies two defaults: the tenant to operate on, and the - signed-in account to authenticate as. `--tenant-id` overrides the first; the account still comes - from `az login`. Tokens are not borrowed from Azure CLI -- both the ARM policy read and the - Agent 365 call acquire their own tokens through the CLI's sign-in. + signed-in account used as a login hint for the Agent 365 call. `--tenant-id` overrides the + first. Tokens are not borrowed from Azure CLI -- both the ARM policy read and the Agent 365 call + acquire their own tokens through the CLI's sign-in. The ARM read is not given the login hint, so + with several cached accounts in the same tenant it may sign in as a different one; sign out of + the accounts you do not want the CLI to use. - A NetworkInjection enterprise policy already created by `New-SubnetInjectionEnterprisePolicy`, with subnets delegated to `Microsoft.PowerPlatform/enterprisePolicies`. - Public cloud only. Sovereign clouds are not supported. @@ -49,9 +51,13 @@ a365 network vnet link --policy-arm-id [--swap] [--tenant-id ] [- | --- | --- | | `--policy-arm-id`, `-p` | **Required.** ARM resource id of the policy, as returned by `New-SubnetInjectionEnterprisePolicy`. | | `--swap` | Replace an existing link to a *different* policy. Without it, a different existing link is reported as a conflict instead of being silently replaced. | -| `--tenant-id` | Tenant to authenticate against. Defaults to the tenant of your current `az login`. | +| `--tenant-id` | Tenant to authenticate against, for both the Azure policy read and the Agent 365 call. Defaults to the tenant of your current `az login`. | | `--wait` | Poll until the operation settles instead of returning an operation id. | -| `--yes`, `-y` | Skip the confirmation prompt shown for `--swap`. | +| `--yes`, `-y` | Skip the confirmation prompt. | + +Every `link` prompts for confirmation: Power Platform documents that enabling subnet delegation can +leave the environment unstable for up to 30 minutes while connections re-initialize, and the change +applies to every Agent 365 agent in the tenant. Pass `--yes` in automation. Linking the policy that is already linked is a no-op and succeeds without `--swap`. @@ -82,19 +88,20 @@ operation. | --- | --- | | `Linked` | A policy is linked; `Policy` names it. | | `NotLinked` | No policy is linked. | -| `Running` / `NotStarted` | The operation is still in flight; `Operation` is the handle to poll. | +| `Running` | The operation is still in flight; `Operation` is the handle to poll. | | `Failed` | The operation failed; `Reason` explains why. | +| `Unknown` | The operation id is not recognised — mistyped, expired, or from another tenant. Operation ids are tenant-scoped, so check `--tenant-id`; to read the current link instead, run `status` with no `--operation-id`. | -Exit code is `1` on `Failed` or on any request error, and `0` otherwise — including a still-running -operation, which is a legitimate outcome when `--wait` is not passed. +Exit code is `1` on `Failed`, on `Unknown`, and on any request error; `0` otherwise — including a +still-running operation, which is a legitimate outcome when `--wait` is not passed. ## Typical flow ```bash # 1. Create the policy with the PowerShell module (unchanged). -./SubnetInjection/NewSubnetInjectionEnterprisePolicy.ps1 ` - -subscription -resourceGroup -enterprisePolicyName ` - -enterprisePolicyLocation -virtualNetworkId -subnetName +New-SubnetInjectionEnterprisePolicy ` + -SubscriptionId -ResourceGroupName -PolicyName ` + -PolicyLocation -VirtualNetworkId -SubnetName # 2. Link it — this replaces Enable-SubnetInjection. a365 network vnet link --policy-arm-id /subscriptions//resourceGroups//providers/Microsoft.PowerPlatform/enterprisePolicies/ --wait @@ -103,6 +110,13 @@ a365 network vnet link --policy-arm-id /subscriptions//resourceGroups// a365 network vnet status ``` +Two things the Learn walkthrough covers that are easy to miss: + +- Geographies with two supported regions (`unitedstates`, for example) need a second virtual + network: pass `-VirtualNetworkId2` and `-SubnetName2` as well. +- The CLI reads the policy with *your* identity, so whoever runs `link` needs read access on the + policy even if someone else created it. + ## Troubleshooting | Symptom | Cause | diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs index 73ca8516..4f4cce47 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs @@ -99,7 +99,7 @@ internal static async Task ConfirmChangeAsync( } return await confirmationProvider.ConfirmAsync( - $"{action} for tenant {tenantId}. This changes networking for every Agent 365 agent in the tenant. Continue?"); + $"{action} for tenant {tenantId}. This changes networking for every Agent 365 agent in the tenant. Continue? (y/N): "); } private static Command CreateLinkSubcommand( @@ -129,8 +129,8 @@ private static Command CreateLinkSubcommand( var tenantIdOption = new Option( "--tenant-id", - "Tenant to authenticate against for the Azure policy read. Defaults to the tenant of " + - "your current az login."); + "Tenant to authenticate against, for both the Azure policy read and the Agent 365 call. " + + "Defaults to the tenant of your current az login."); var waitOption = new Option( "--wait", @@ -140,7 +140,7 @@ private static Command CreateLinkSubcommand( var yesOption = new Option( ["--yes", "-y"], - "Skip the confirmation prompt shown when --swap would replace an existing link."); + "Skip the confirmation prompt."); command.AddOption(policyArmIdOption); command.AddOption(swapOption); @@ -158,6 +158,15 @@ private static Command CreateLinkSubcommand( var yes = context.ParseResult.GetValueForOption(yesOption); var ct = context.GetCancellationToken(); + // IsRequired is satisfied by an explicitly blank value, which would otherwise reach + // LinkAsync and surface as "Unexpected error occurred. This may be a bug in the CLI." + if (string.IsNullOrWhiteSpace(policyArmId)) + { + logger.LogError("--policy-arm-id is required and cannot be blank."); + context.ExitCode = 1; + return; + } + var tenantId = await ResolveTenantIdAsync(logger, azureCliService, tenantIdOptionValue); if (tenantId == null) { @@ -165,10 +174,14 @@ private static Command CreateLinkSubcommand( return; } - // Only --swap needs confirming: without it an existing different link is reported as a - // conflict rather than replaced, so the command is already non-destructive. - if (swap && !await ConfirmChangeAsync( - confirmationProvider, yes, "Replace the existing virtual network link", tenantId)) + // Every link is confirmed, not just --swap. Power Platform documents that enabling + // subnet delegation can leave the environment unstable for up to 30 minutes while + // connections re-initialize, and the change is tenant-wide either way. + var action = swap + ? "Replace the existing virtual network link" + : "Link a virtual network"; + + if (!await ConfirmChangeAsync(confirmationProvider, yes, action, tenantId)) { logger.LogInformation("Cancelled."); context.ExitCode = 1; @@ -326,12 +339,37 @@ internal static async Task ReportAsync( return 1; } + // The platform answers 200 with Unknown for an operation id it cannot find -- mistyped, + // expired, or from another tenant. Nothing settled, so exiting 0 would report success for + // an operation that was never observed to finish. + if (string.Equals(result.Status, "Unknown", StringComparison.OrdinalIgnoreCase)) + { + logger.LogError( + "The operation id is unknown or has expired, so its outcome could not be read. " + + "Operation ids are scoped to a tenant; check the id and --tenant-id, or read the " + + "current link with: a365 network vnet status --tenant-id {TenantId}", + tenantId); + return 1; + } + if (VNetLinkService.IsRunning(result.Status)) { + // The platform always returns an id alongside a running status. Without one there is + // nothing to resume from, so this is an unexpected response rather than a nudge. + if (string.IsNullOrWhiteSpace(result.OperationId)) + { + logger.LogError( + "{Operation} reported as running but returned no operation id, so it cannot be resumed.", + operationLabel); + return 1; + } + logger.LogInformation( - "{Operation} is still running. Check on it with: a365 network vnet status --operation-id {OperationId}", + "{Operation} is still running. Check on it with: " + + "a365 network vnet status --operation-id {OperationId} --tenant-id {TenantId}", operationLabel, - result.OperationId); + result.OperationId, + tenantId); } return 0; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs index 8f79c655..4240194c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs @@ -89,8 +89,22 @@ public class VNetStatusResponse public class VNetErrorResponse { /// - /// Human-readable error message. + /// Human-readable error message. The role-check 403 puts a sentence here; a scope failure puts + /// the code "insufficient_scope" and the sentence in . /// [JsonPropertyName("error")] public string? Error { get; set; } + + /// + /// Sentence describing a scope failure. Absent on the role-check 403. + /// + [JsonPropertyName("error_description")] + public string? ErrorDescription { get; set; } + + /// + /// Scope the token was missing. Present only on a scope failure, which is what tells the two + /// kinds of 403 apart. + /// + [JsonPropertyName("required_scope")] + public string? RequiredScope { get; set; } } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs index d1217903..8321bee5 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs @@ -29,21 +29,23 @@ public class ArmApiService : IDisposable // Stable first: the module's own ARM templates deploy enterprise policies at 2020-10-30. private static readonly string[] EnterprisePolicyApiVersions = ["2020-10-30", "2020-10-30-preview"]; - // ArmBaseUrl has no trailing slash and the ARM bearer token is set as a default request - // header, so a policy id that does not begin with "/subscriptions/" can retarget the whole - // request: "@evil.example/x" concatenates to "https://management.azure.com@evil.example/x", - // where "management.azure.com" is userinfo and the host is the attacker's. Pinning the shape - // is what keeps the token pointed at ARM. - // - // IgnoreCase because ARM ids are case-insensitive and are commonly seen as "resourcegroups" - // or "microsoft.powerplatform"; the shape stays pinned either way. Segments exclude - // whitespace as well as delimiters, which is what actually rejects a trailing newline -- - // \z alone does not, because [^/?#]+ would absorb the newline before the anchor is reached. - // The dot-segment lookaheads keep the path from normalizing into a different resource -- - // harmless while the host is fixed, but the validated string should be the string that gets - // requested. + // ARM's vocabulary for "this api-version is not one I serve here". Anything else in a 400 is + // about the request itself and must not be retried against the next version. + private static readonly HashSet UnsupportedApiVersionCodes = new(StringComparer.OrdinalIgnoreCase) + { + "NoRegisteredProviderFound", + "InvalidApiVersionParameter", + "UnsupportedApiVersion", + }; + + // ArmBaseUrl has no trailing slash and the ARM bearer is a default request header, so an id + // that does not begin with "/subscriptions/" retargets the request: "@evil.example/x" makes + // "management.azure.com" userinfo and the attacker's host the target. The segment classes are + // an allowlist of ARM name characters rather than a delimiter blocklist, because Uri + // normalizes "%2e%2e" and "\" before the request goes out -- so anything the pattern admits + // that Uri rewrites would break "what was validated is what gets requested". private static readonly Regex EnterprisePolicyArmIdPattern = new( - @"^/subscriptions/[0-9a-f-]{36}/resourceGroups/(?!\.{1,2}(?:/|\z))[^/?#\s]+/providers/Microsoft\.PowerPlatform/enterprisePolicies/(?!\.{1,2}\z)[^/?#\s]+\z", + @"^/subscriptions/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/resourceGroups/(?!\.{1,2}(?:/|\z))[-\w.()]+/providers/Microsoft\.PowerPlatform/enterprisePolicies/(?!\.{1,2}\z)[-\w.()]+\z", RegexOptions.Compiled | RegexOptions.CultureInvariant | RegexOptions.IgnoreCase); private readonly ILogger _logger; @@ -316,8 +318,41 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke if (response.StatusCode == HttpStatusCode.BadRequest) { - _logger.LogDebug("ARM rejected api-version {ApiVersion}; trying the next one", apiVersion); - continue; + // Only an api-version complaint is worth retrying on the next version. Any + // other 400 is about this request, and discarding it ends the loop in + // "Azure rejected every supported api-version", which sends the admin after + // the wrong problem. + var badRequestBody = await response.Content.ReadAsStringAsync(ct); + string? errorCode = null; + string? errorMessage = null; + + try + { + using var errorDoc = JsonDocument.Parse(badRequestBody); + if (errorDoc.RootElement.TryGetProperty("error", out var error)) + { + errorCode = error.TryGetProperty("code", out var c) ? c.GetString() : null; + errorMessage = error.TryGetProperty("message", out var m) ? m.GetString() : null; + } + } + catch (JsonException) + { + // A non-JSON 400 came from something in front of ARM. Fall through and + // treat it as a real rejection rather than an api-version miss. + } + + if (UnsupportedApiVersionCodes.Contains(errorCode ?? string.Empty)) + { + _logger.LogDebug("ARM rejected api-version {ApiVersion}; trying the next one", apiVersion); + continue; + } + + _logger.LogError( + "Could not read enterprise policy {PolicyArmId}. Azure returned 400 ({Code}): {Message}", + policyArmId, + errorCode ?? "no error code", + errorMessage ?? "No error detail was returned."); + return null; } if (!response.IsSuccessStatusCode) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs index eb1854da..aca33d60 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs @@ -17,7 +17,7 @@ public interface IVNetLinkService /// /// ARM resource id of the NetworkInjection enterprise policy. /// Whether an existing link to a different policy may be replaced. - /// Tenant to authenticate against for the ARM read. + /// Tenant to authenticate against, for both the ARM read and the platform call. /// Cancellation token. /// The resulting status, or null when the operation could not be started. Task LinkAsync( diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs index 961282da..758abe67 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs @@ -31,7 +31,11 @@ public class VNetLinkService : IVNetLinkService private const string UnlinkPath = "/agents/vnet/unlink"; private const string StatusPath = "/agents/vnet/status"; - private static readonly TimeSpan PollInterval = TimeSpan.FromSeconds(10); + private static readonly TimeSpan DefaultPollInterval = TimeSpan.FromSeconds(10); + + // A status read can fail transiently while the operation itself is fine. Tolerate a few in a + // row so a single 5xx does not report a running link as failed. + private const int MaxConsecutivePollFailures = 3; private readonly ILogger _logger; private readonly IAuthenticationService _authService; @@ -39,6 +43,7 @@ public class VNetLinkService : IVNetLinkService private readonly string _environment; private readonly HttpMessageHandler? _handler; private readonly Func> _loginHintResolver; + private readonly TimeSpan _pollInterval; public VNetLinkService( ILogger logger, @@ -46,7 +51,8 @@ public VNetLinkService( ArmApiService armApiService, string environment = "prod", HttpMessageHandler? handler = null, - Func>? loginHintResolver = null) + Func>? loginHintResolver = null, + TimeSpan? pollInterval = null) { _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _authService = authService ?? throw new ArgumentNullException(nameof(authService)); @@ -54,6 +60,7 @@ public VNetLinkService( _environment = environment ?? "prod"; _handler = handler; _loginHintResolver = loginHintResolver ?? AzCliHelper.ResolveLoginHintAsync; + _pollInterval = pollInterval ?? DefaultPollInterval; } /// @@ -155,41 +162,86 @@ public VNetLinkService( var stopwatch = Stopwatch.StartNew(); VNetStatusResponse? last = null; + var consecutiveFailures = 0; while (true) { try { - last = await GetStatusAsync(tenantId, operationId, timeoutCts.Token); + var polled = await GetStatusAsync(tenantId, operationId, timeoutCts.Token); + + if (polled == null) + { + // A single transient 5xx or dropped connection is not the operation failing. + // Ending the wait here would report a link that is still running as failed, + // and the caller never printed an operation id to resume from. + consecutiveFailures++; + if (consecutiveFailures > MaxConsecutivePollFailures) + { + _logger.LogError( + "Gave up after {Count} consecutive failed status reads. The operation may still be running; " + + "check on it with: a365 network vnet status --operation-id {OperationId} --tenant-id {TenantId}", + consecutiveFailures, + operationId, + tenantId); + return null; + } + + _logger.LogWarning( + "Status read failed ({Count} of {Max}). Retrying...", + consecutiveFailures, + MaxConsecutivePollFailures); + } + else + { + consecutiveFailures = 0; + last = polled; + } } catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) { // The ceiling elapsed mid-request. That is a timeout, not a failure: report the // last known state, exactly as the pre-sleep check below does. _logger.LogInformation( - "Stopped waiting after {Elapsed:0}s. The operation is still running.", - stopwatch.Elapsed.TotalSeconds); + "Stopped waiting after {Elapsed:0}s. The operation is still running. " + + "Check on it with: a365 network vnet status --operation-id {OperationId} --tenant-id {TenantId}", + stopwatch.Elapsed.TotalSeconds, + operationId, + tenantId); return last; } - if (last == null || !IsRunning(last.Status)) + if (consecutiveFailures == 0 && !IsRunning(last?.Status)) return last; - if (stopwatch.Elapsed + PollInterval >= timeout) + if (stopwatch.Elapsed + _pollInterval >= timeout) + { + _logger.LogInformation( + "Stopped waiting after {Elapsed:0}s. Check on it with: " + + "a365 network vnet status --operation-id {OperationId} --tenant-id {TenantId}", + stopwatch.Elapsed.TotalSeconds, + operationId, + tenantId); return last; + } + + if (consecutiveFailures == 0) + { + _logger.LogInformation("Still running... ({Elapsed:0}s elapsed)", stopwatch.Elapsed.TotalSeconds); + } - _logger.LogInformation("Still running... ({Elapsed:0}s elapsed)", stopwatch.Elapsed.TotalSeconds); // The pre-sleep check above guarantees this delay finishes inside the ceiling, so it // waits on the caller's token only -- the timeout can't fire here. - await Task.Delay(PollInterval, cancellationToken); + await Task.Delay(_pollInterval, cancellationToken); } } /// /// True when the reported status means the operation has not settled yet. /// - /// The platform reports a queued operation as NotStarted, which is as unsettled as Running: - /// treating it as terminal would make --wait return before the work had begun. + /// The platform collapses every unsettled upstream state to Running, so NotStarted is + /// defensive rather than expected -- but it is as unsettled as Running, and treating it as + /// terminal would make --wait return before the work had begun. /// public static bool IsRunning(string? status) => string.Equals(status, "Running", StringComparison.OrdinalIgnoreCase) @@ -204,18 +256,25 @@ public static bool IsRunning(string? status) => CancellationToken cancellationToken) { var correlationId = HttpClientFactory.GenerateCorrelationId(); - var baseUrl = BuildBaseUrl(); - var url = $"{baseUrl}{path}"; + var url = ConfigConstants.BuildAgent365ToolsEndpointUrl(_environment, path); try { var audience = ConfigConstants.GetAgent365ToolsResourceAppId(_environment); - var loginHint = await _loginHintResolver(); + var loginHint = await _loginHintResolver().WaitAsync(cancellationToken); // The tenant matters as much here as on the ARM read: without it MSAL falls back to // the common authority with only a login hint, so on a machine with several cached // accounts the platform call can land in a different tenant than the policy read. - var authToken = await _authService.GetAccessTokenAsync(audience, tenantId, userId: loginHint, ct: cancellationToken); + // The authority host comes from the configured environment for the same reason every + // other Agent 365 Tools call passes it: without it a sovereign configuration would + // authenticate against the commercial cloud. + var authToken = await _authService.GetAccessTokenAsync( + audience, + tenantId, + userId: loginHint, + ct: cancellationToken, + authorityHost: ConfigConstants.GetAuthorityHost(_environment)); if (string.IsNullOrWhiteSpace(authToken)) { _logger.LogError("Failed to acquire an Agent 365 access token."); @@ -273,9 +332,12 @@ public static bool IsRunning(string? status) => private void LogFailure(HttpStatusCode statusCode, string body, string operationName, string correlationId) { string? message = null; + string? requiredScope = null; try { - message = JsonSerializer.Deserialize(body)?.Error; + var error = JsonSerializer.Deserialize(body); + message = error?.ErrorDescription ?? error?.Error; + requiredScope = error?.RequiredScope; } catch (JsonException) { @@ -291,18 +353,23 @@ private void LogFailure(HttpStatusCode statusCode, string body, string operation if (statusCode == HttpStatusCode.Forbidden) { - _logger.LogError( - "This command requires the Global Administrator or Power Platform Administrator role, " + - "and a client application consented for AgentTools.VNet.Manage.All."); + // The platform distinguishes the two 403s: a scope failure carries required_scope, a + // role failure does not. Naming the wrong one sends the admin after a consent they + // cannot grant, or after a role they already hold. + if (!string.IsNullOrWhiteSpace(requiredScope)) + { + _logger.LogError( + "The client application is missing the {RequiredScope} scope. Ask an administrator " + + "to consent it for the application you signed in with.", + requiredScope); + } + else + { + _logger.LogError( + "This command requires the Global Administrator or Power Platform Administrator role."); + } } _logger.LogError("Correlation ID: {CorrelationId}", correlationId); } - - private string BuildBaseUrl() - { - var discoverUrl = ConfigConstants.GetDiscoverEndpointUrl(_environment); - var uri = new Uri(discoverUrl); - return $"{uri.Scheme}://{uri.Authority}"; - } } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs index e9e8fe37..21ebd8dd 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs @@ -144,18 +144,66 @@ public async Task LinkHandler_PrefersAnExplicitTenantOverTheAzLoginTenant() } [Fact] - public async Task LinkHandler_WithoutSwap_DoesNotPrompt() + public async Task LinkHandler_WithoutSwap_StillPrompts() { var vnet = Substitute.For(); vnet.LinkAsync(PolicyArmId, false, TenantId, Arg.Any()) .Returns(Task.FromResult(new VNetStatusResponse { Status = "Linked" })); - var confirmation = Confirming(false); + var confirmation = Confirming(true); var command = CreateCommand(vnet, confirmation: confirmation); var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId}"); - exitCode.Should().Be(0, because: "a conflicting link is reported, not replaced, without --swap"); + exitCode.Should().Be(0); + await confirmation.Received(1).ConfirmAsync( + Arg.Is(p => p.Contains(TenantId) && p.EndsWith("Continue? (y/N): "))); + await vnet.Received(1).LinkAsync(PolicyArmId, false, TenantId, Arg.Any()); + } + + [Fact] + public async Task LinkHandler_WhenAPlainLinkIsDeclined_DoesNotCallTheService() + { + var vnet = Substitute.For(); + var command = CreateCommand(vnet, confirmation: Confirming(false)); + + var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId}"); + + exitCode.Should().Be( + 1, + because: "enabling subnet delegation can destabilise the environment for 30 minutes"); + await vnet.DidNotReceive().LinkAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task LinkHandler_WhenYes_SkipsThePromptWithoutSwap() + { + var vnet = Substitute.For(); + vnet.LinkAsync(PolicyArmId, false, TenantId, Arg.Any()) + .Returns(Task.FromResult(new VNetStatusResponse { Status = "Linked" })); + var confirmation = Confirming(false); + var command = CreateCommand(vnet, confirmation: confirmation); + + var exitCode = await command.InvokeAsync($"vnet link --policy-arm-id {PolicyArmId} --yes"); + + exitCode.Should().Be(0); await confirmation.DidNotReceive().ConfirmAsync(Arg.Any()); + await vnet.Received(1).LinkAsync(PolicyArmId, false, TenantId, Arg.Any()); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public async Task LinkHandler_WhenPolicyArmIdBlank_FailsWithATargetedMessage(string policyArmId) + { + var vnet = Substitute.For(); + var command = CreateCommand(vnet); + + var exitCode = await command.InvokeAsync(["vnet", "link", "--policy-arm-id", policyArmId]); + + exitCode.Should().Be(1); + await vnet.DidNotReceive().LinkAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } [Fact] @@ -455,7 +503,26 @@ public async Task ReportAsync_WhenRunningWithoutAHandle_DoesNotWait() var exitCode = await NetworkCommand.ReportAsync( NullLogger.Instance, vnet, result, wait: true, "Link", TenantId, CancellationToken.None); - exitCode.Should().Be(0); + exitCode.Should().Be( + 1, + because: "a running operation with no id cannot be resumed, so it is an unexpected response"); + await vnet.DidNotReceive().WaitForCompletionAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Theory] + [InlineData("Unknown")] + [InlineData("unknown")] + public async Task ReportAsync_WhenUnknown_ReturnsFailure(string status) + { + var vnet = Substitute.For(); + var result = new VNetStatusResponse { Status = status, OperationId = OperationId }; + + var exitCode = await NetworkCommand.ReportAsync( + NullLogger.Instance, vnet, result, wait: false, "Status", TenantId, CancellationToken.None); + + exitCode.Should().Be( + 1, + because: "an unrecognised operation id is a mistake to surface, not a success"); await vnet.DidNotReceive().WaitForCompletionAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs index 7c7a2526..e56bc144 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs @@ -336,6 +336,16 @@ private static HttpResponseMessage PolicyResponse(string body) => [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p\n")] [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p ")] [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/r g/providers/Microsoft.PowerPlatform/enterprisePolicies/p")] + // Percent-encoded dot segments. Uri unescapes before normalizing, so these would climb the + // path after the regex had already passed them. An allowlist that excludes '%' is what stops + // them -- no Uri.AbsolutePath check is reachable, which is why these live in the reject list. + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/%2e%2e/providers/Microsoft.PowerPlatform/enterprisePolicies/p")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/%2e%2e")] + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg%2Fx/providers/Microsoft.PowerPlatform/enterprisePolicies/p")] + // Backslashes are path separators to Uri on Windows and climb the same way. + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg\\..\\..\\x/providers/Microsoft.PowerPlatform/enterprisePolicies/p")] + // 36 characters, but not a GUID: the old [0-9a-f-]{36} accepted this. + [InlineData("/subscriptions/------------------------------------/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p")] public async Task GetEnterprisePolicySystemIdAsync_WhenArmIdIsNotAnEnterprisePolicyPath_RejectsWithoutCalling( string policyArmId) { @@ -356,6 +366,8 @@ public async Task GetEnterprisePolicySystemIdAsync_WhenArmIdIsNotAnEnterprisePol [InlineData("/subscriptions/8D1E5B21-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.PowerPlatform/enterprisePolicies/p")] [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourcegroups/rg/providers/microsoft.powerplatform/enterprisepolicies/p")] [InlineData("/SUBSCRIPTIONS/8D1E5B21-0000-0000-0000-000000000000/RESOURCEGROUPS/RG/PROVIDERS/MICROSOFT.POWERPLATFORM/ENTERPRISEPOLICIES/P")] + // ARM resource names allow dots, hyphens, underscores and parentheses. + [InlineData("/subscriptions/8d1e5b21-0000-0000-0000-000000000000/resourceGroups/rg-prod.01_(east)/providers/Microsoft.PowerPlatform/enterprisePolicies/policy.v2_(new)")] public async Task GetEnterprisePolicySystemIdAsync_AcceptsAnyCasingOfTheArmId(string policyArmId) { using var handler = new TestHttpMessageHandler(); @@ -404,7 +416,7 @@ public async Task GetEnterprisePolicySystemIdAsync_WhenStableApiVersionRejected_ { var urls = new List(); using var handler = new CapturingHttpMessageHandler(r => urls.Add(r.RequestUri!.ToString())); - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("") }); + handler.QueueResponse(ApiVersionRejection("NoRegisteredProviderFound")); handler.QueueResponse(PolicyResponse( JsonSerializer.Serialize(new { kind = "NetworkInjection", properties = new { systemId = PolicySystemId } }))); var svc = CreateService(handler); @@ -417,19 +429,73 @@ public async Task GetEnterprisePolicySystemIdAsync_WhenStableApiVersionRejected_ urls[1].Should().EndWith("api-version=2020-10-30-preview"); } + [Theory] + [InlineData("NoRegisteredProviderFound")] + [InlineData("InvalidApiVersionParameter")] + [InlineData("UnsupportedApiVersion")] + [InlineData("unsupportedapiversion")] + public async Task GetEnterprisePolicySystemIdAsync_WhenApiVersionCodeReturned_TriesTheNextVersion(string code) + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(ApiVersionRejection(code)); + handler.QueueResponse(PolicyResponse( + JsonSerializer.Serialize(new { kind = "NetworkInjection", properties = new { systemId = PolicySystemId } }))); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().Be(PolicySystemId); + handler.RequestCount.Should().Be(2); + } + + [Theory] + // A 400 about the request itself. Retrying the next api-version buries the real reason and + // ends the loop in "Azure rejected every supported api-version". + [InlineData("{\"error\":{\"code\":\"InvalidResourceType\",\"message\":\"No such resource type.\"}}")] + [InlineData("{\"error\":{\"code\":\"InvalidSubscriptionId\",\"message\":\"Bad subscription.\"}}")] + // A non-JSON 400 came from something in front of ARM, and is equally not an api-version miss. + [InlineData("")] + [InlineData("gateway error")] + // Well-formed JSON with no recognisable error envelope. + [InlineData("{}")] + public async Task GetEnterprisePolicySystemIdAsync_WhenBadRequestIsNotAboutTheApiVersion_StopsImmediately( + string body) + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent(body) }); + var svc = CreateService(handler); + + var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); + + result.Should().BeNull(); + handler.RequestCount.Should().Be( + 1, + because: "the next api-version would return the same 400 and hide the real reason"); + } + [Fact] public async Task GetEnterprisePolicySystemIdAsync_WhenEveryApiVersionRejected_ReturnsNull() { using var handler = new TestHttpMessageHandler(); - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("") }); - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("") }); + handler.QueueResponse(ApiVersionRejection("NoRegisteredProviderFound")); + handler.QueueResponse(ApiVersionRejection("NoRegisteredProviderFound")); var svc = CreateService(handler); var result = await svc.GetEnterprisePolicySystemIdAsync(PolicyArmId, TenantId); result.Should().BeNull(because: "there is no api-version left to try"); + handler.RequestCount.Should().Be(2); } + private static HttpResponseMessage ApiVersionRejection(string code) => + new(HttpStatusCode.BadRequest) + { + Content = new StringContent(JsonSerializer.Serialize(new + { + error = new { code, message = "The api-version is not supported." }, + })), + }; + [Theory] [InlineData(HttpStatusCode.Unauthorized)] [InlineData(HttpStatusCode.Forbidden)] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs index a25e269d..ea237bc1 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs @@ -5,7 +5,9 @@ using System.Net; using System.Text.Json; using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Constants; using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using Xunit; @@ -33,18 +35,27 @@ private static IAuthenticationService FakeAuth(string token = "fake-a365-token") { var mock = Substitute.For(); - // The 8th parameter is the CancellationToken. Omitting a matcher for it pins the setup to - // ct == default, so any call carrying a real token -- a caller's, or the wait ceiling's -- - // silently misses and returns null, which the service reports as a failed token acquisition. + // Every optional parameter needs a matcher. Omitting one pins the setup to that + // parameter's default, so a call supplying a real value silently misses and returns null, + // which the service reports as a failed token acquisition. The CancellationToken and + // authorityHost have both caused that already. mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any?>(), Arg.Any(), Arg.Any(), Arg.Any()) + Arg.Any?>(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any()) .Returns(Task.FromResult(token)); return mock; } private static ArmApiService FakeArm(string? systemId = PolicySystemId) { - var arm = Substitute.For(); + // Pass constructor arguments: the parameterless overload builds a real + // AuthenticationService, which creates the CLI's app-data directory and deletes any legacy + // token file on the machine running the tests. + var arm = Substitute.For( + NullLogger.Instance, + Substitute.For(), + null, + null); arm.GetEnterprisePolicySystemIdAsync(Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult(systemId)); return arm; @@ -53,14 +64,17 @@ private static ArmApiService FakeArm(string? systemId = PolicySystemId) private static VNetLinkService CreateService( HttpMessageHandler handler, ArmApiService? arm = null, - IAuthenticationService? auth = null) => + IAuthenticationService? auth = null, + ILogger? logger = null, + TimeSpan? pollInterval = null) => new( - NullLogger.Instance, + logger ?? NullLogger.Instance, auth ?? FakeAuth(), arm ?? FakeArm(), "prod", handler, - NoLoginHint); + NoLoginHint, + pollInterval); /// /// Stands in for the real resolver so the tests never shell out to `az account show`. @@ -462,8 +476,9 @@ public async Task WaitForCompletionAsync_WhenStillRunningAndBudgetExhausted_Retu handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Running", OperationId)); var svc = CreateService(handler); - // A zero budget cannot fit another poll interval, so the first read is also the last. - var result = await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.Zero); + // Shorter than the poll interval, so the first read is also the last -- without arming + // the ceiling at zero, which races the first poll and cancels it before it can return. + var result = await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.FromSeconds(5)); result.Should().NotBeNull(); result!.Status.Should().Be("Running"); @@ -475,16 +490,22 @@ public async Task WaitForCompletionAsync_WhenStillRunningAndBudgetExhausted_Retu public async Task WaitForCompletionAsync_WhenStatusCannotBeRead_ReturnsNull() { using var handler = new TestHttpMessageHandler(); - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadGateway) + for (var i = 0; i < 4; i++) { - Content = new StringContent(JsonSerializer.Serialize(new { error = "upstream" })), - }); - var svc = CreateService(handler); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadGateway) + { + Content = new StringContent(JsonSerializer.Serialize(new { error = "upstream" })), + }); + } + + var svc = CreateService(handler, pollInterval: TimeSpan.FromMilliseconds(1)); var result = await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.FromMinutes(5)); result.Should().BeNull(); - handler.RequestCount.Should().Be(1, because: "an unreadable status is terminal for the wait"); + handler.RequestCount.Should().Be( + 4, + because: "a status read is retried a few times before the wait gives up on it"); } [Theory] @@ -675,6 +696,8 @@ await auth.Received(1).GetAccessTokenAsync( Arg.Any(), Arg.Any?>(), Arg.Any(), + Arg.Any(), + Arg.Any(), Arg.Any()); } @@ -695,6 +718,8 @@ await auth.Received(1).GetAccessTokenAsync( Arg.Any(), Arg.Any?>(), Arg.Any(), + Arg.Any(), + Arg.Any(), Arg.Any()); } @@ -715,6 +740,8 @@ await auth.Received(1).GetAccessTokenAsync( Arg.Any(), Arg.Any?>(), Arg.Any(), + Arg.Any(), + Arg.Any(), Arg.Any()); } @@ -740,4 +767,169 @@ public void Constructor_WhenArmApiServiceNull_Throws() var act = () => new VNetLinkService(NullLogger.Instance, FakeAuth(), null!); act.Should().Throw(); } + + // ───────────────────── Authority host and endpoint origin ────────────────── + + [Fact] + public async Task SendAsync_AuthenticatesAgainstTheEnvironmentAuthorityHost() + { + var auth = FakeAuth(); + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked")); + var svc = new VNetLinkService( + NullLogger.Instance, auth, FakeArm(), "dogfood", handler, NoLoginHint); + + await svc.GetStatusAsync(TenantId); + + await auth.Received(1).GetAccessTokenAsync( + Arg.Any(), + TenantId, + Arg.Any(), + Arg.Any(), + Arg.Any?>(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + ConfigConstants.GetAuthorityHost("dogfood")); + } + + [Fact] + public async Task SendAsync_TargetsTheSharedAgent365ToolsOriginForTheEnvironment() + { + Uri? requestUri = null; + using var handler = new CapturingHttpMessageHandler(r => requestUri = r.RequestUri); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked")); + var svc = new VNetLinkService( + NullLogger.Instance, FakeAuth(), FakeArm(), "dogfood", handler, NoLoginHint); + + await svc.GetStatusAsync(TenantId); + + requestUri.Should().NotBeNull(); + requestUri!.ToString().Should().StartWith( + ConfigConstants.BuildAgent365ToolsEndpointUrl("dogfood", "/agents/vnet/status"), + because: "the origin must come from the shared helper, not a second copy of the table"); + } + + // ──────────────────────────── 403 disambiguation ─────────────────────────── + + [Fact] + public async Task SendAsync_WhenForbiddenWithRequiredScope_NamesTheMissingScope() + { + var logger = new CapturingLogger(); + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Forbidden) + { + Content = new StringContent(JsonSerializer.Serialize(new + { + error = "insufficient_scope", + error_description = "The token is missing a required scope.", + required_scope = "AgentTools.VNet.Manage.All", + })), + }); + var svc = CreateService(handler, logger: logger); + + var result = await svc.GetStatusAsync(TenantId); + + result.Should().BeNull(); + logger.Messages.Should().Contain(m => m.Contains("The token is missing a required scope.")); + logger.Messages.Should().Contain(m => m.Contains("AgentTools.VNet.Manage.All") && m.Contains("consent")); + logger.Messages.Should().NotContain(m => m.Contains("Global Administrator")); + } + + [Fact] + public async Task SendAsync_WhenForbiddenWithoutRequiredScope_NamesTheMissingRole() + { + var logger = new CapturingLogger(); + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Forbidden) + { + Content = new StringContent(JsonSerializer.Serialize(new + { + error = "The signed-in user is not a Power Platform administrator.", + })), + }); + var svc = CreateService(handler, logger: logger); + + var result = await svc.GetStatusAsync(TenantId); + + result.Should().BeNull(); + logger.Messages.Should().Contain(m => m.Contains("not a Power Platform administrator")); + logger.Messages.Should().Contain(m => + m.Contains("Global Administrator") && m.Contains("Power Platform Administrator role")); + logger.Messages.Should().NotContain(m => m.Contains("consent")); + } + + // ───────────────────── Transient poll failure tolerance ──────────────────── + + [Fact] + public async Task WaitForCompletionAsync_WhenAPollFailsTransiently_KeepsWaiting() + { + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Running", OperationId)); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) + { + Content = new StringContent("{}"), + }); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Linked", OperationId)); + var svc = CreateService(handler, pollInterval: TimeSpan.FromMilliseconds(1)); + + var result = await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.FromMinutes(5)); + + result.Should().NotBeNull(); + result!.Status.Should().Be("Linked"); + result.OperationId.Should().Be(OperationId); + result.PolicyArmId.Should().BeNull(); + result.Reason.Should().BeNull(); + handler.RequestCount.Should().Be(3, because: "the failed read is retried rather than ending the wait"); + } + + [Fact] + public async Task WaitForCompletionAsync_WhenPollsFailConsecutivelyPastTheLimit_ReturnsNull() + { + using var handler = new TestHttpMessageHandler(); + for (var i = 0; i < 4; i++) + { + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) + { + Content = new StringContent("{}"), + }); + } + + var logger = new CapturingLogger(); + var svc = CreateService(handler, logger: logger, pollInterval: TimeSpan.FromMilliseconds(1)); + + var result = await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.FromMinutes(5)); + + result.Should().BeNull(); + handler.RequestCount.Should().Be(4, because: "three failures are tolerated and the fourth gives up"); + logger.Messages.Should().Contain(m => + m.Contains($"--operation-id {OperationId}") && m.Contains($"--tenant-id {TenantId}")); + } + + [Fact] + public async Task WaitForCompletionAsync_WhenBudgetExhausted_PrintsAResumeCommandNamingTheTenant() + { + var logger = new CapturingLogger(); + using var handler = new TestHttpMessageHandler(); + handler.QueueResponse(StatusResponse(HttpStatusCode.OK, "Running", OperationId)); + var svc = CreateService(handler, logger: logger); + + await svc.WaitForCompletionAsync(TenantId, OperationId, TimeSpan.FromSeconds(5)); + + logger.Messages.Should().Contain(m => + m.Contains($"--operation-id {OperationId}") && m.Contains($"--tenant-id {TenantId}")); + } + + private sealed class CapturingLogger : ILogger + { + public List Messages { get; } = []; + + public IDisposable? BeginScope(TState state) + where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) => Messages.Add(formatter(state, exception)); + } } From 0f40bfab10cd850e24368268862dab48a5e692f6 Mon Sep 17 00:00:00 2001 From: "Lala Sushant Srivastava (from Dev Box)" Date: Wed, 30 Sep 2026 13:02:28 -0700 Subject: [PATCH 10/10] Treat a missing operation status as Unknown LogStatus already renders a null or blank status as "Unknown", but ReportAsync only matched the literal string, so a malformed response printed "Unknown" and exited 0 -- reporting success for an operation that was never observed to finish. 2423 passed, 0 failed, 12 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Commands/NetworkCommand.cs | 6 ++++-- .../Commands/NetworkCommandTests.cs | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs index 4f4cce47..4be34cf2 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs @@ -341,8 +341,10 @@ internal static async Task ReportAsync( // The platform answers 200 with Unknown for an operation id it cannot find -- mistyped, // expired, or from another tenant. Nothing settled, so exiting 0 would report success for - // an operation that was never observed to finish. - if (string.Equals(result.Status, "Unknown", StringComparison.OrdinalIgnoreCase)) + // an operation that was never observed to finish. A missing status is the same situation + // arrived at differently: LogStatus already renders it as Unknown, so exit the same way. + if (string.IsNullOrWhiteSpace(result.Status) + || string.Equals(result.Status, "Unknown", StringComparison.OrdinalIgnoreCase)) { logger.LogError( "The operation id is unknown or has expired, so its outcome could not be read. " + diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs index 21ebd8dd..cd9d91fc 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs @@ -512,7 +512,10 @@ public async Task ReportAsync_WhenRunningWithoutAHandle_DoesNotWait() [Theory] [InlineData("Unknown")] [InlineData("unknown")] - public async Task ReportAsync_WhenUnknown_ReturnsFailure(string status) + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public async Task ReportAsync_WhenUnknown_ReturnsFailure(string? status) { var vnet = Substitute.For(); var result = new VNetStatusResponse { Status = status, OperationId = OperationId }; @@ -522,7 +525,7 @@ public async Task ReportAsync_WhenUnknown_ReturnsFailure(string status) exitCode.Should().Be( 1, - because: "an unrecognised operation id is a mistake to surface, not a success"); + because: "an unrecognised or missing status is a mistake to surface, not a success"); await vnet.DidNotReceive().WaitForCompletionAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); }