diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f625256..0fadd11a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `a365 publish` updates manifest IDs, creates `manifest.zip`, and prints concise upload instructions for Microsoft 365 Admin Center (Agents > All agents > Upload custom agent). Interactive prompts only occur in interactive terminals; redirect stdin to suppress them in scripts. ### Fixed +- `AgentBlueprintService.SetInheritablePermissionsAsync` no longer crashes when the Graph PATCH call throws a transient exception (#366) — the exception is caught, logged, and surfaced as a structured error result +- `AgentBlueprintService.SetInheritablePermissionsAsync` now correctly propagates `OperationCanceledException` when the user cancels (Ctrl+C), instead of masking cancellation as a generic error - `A365CreateInstanceRunner` sponsor handling: sponsor is now required (Graph API rejects requests without one) — removed fallback that silently stripped the sponsor on retry, which caused `BadRequest` errors - Intermittent `ConnectionResetError (10054)` failures on corporate networks with TLS inspection proxies (Zscaler, Netskope) — Graph and ARM API calls now use direct MSAL.NET token acquisition instead of `az account get-access-token` subprocesses, bypassing the Python HTTP stack that triggered proxy resets (#321) - `a365 cleanup` blueprint deletion now succeeds for Global Administrators even when the blueprint was created by a different user diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs index e79243a8..43562a5c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs @@ -339,10 +339,11 @@ public virtual async Task DeleteAgentUserAsync( // Normalize into array form expected by Graph (each element is a single scope string) var desiredArray = desiredSet.ToArray(); + string? blueprintObjectId = null; try { // Resolve blueprintId to object ID if needed - var blueprintObjectId = await ResolveBlueprintObjectIdAsync(tenantId, blueprintId, ct, requiredScopes); + blueprintObjectId = await ResolveBlueprintObjectIdAsync(tenantId, blueprintId, ct, requiredScopes); // Retrieve existing inheritable permissions var getPath = $"/beta/applications/microsoft.graph.agentIdentityBlueprint/{blueprintObjectId}/inheritablePermissions"; @@ -388,10 +389,22 @@ public virtual async Task DeleteAgentUserAsync( } }; - var patched = await _graphApiService.GraphPatchAsync(tenantId, patchPath, patchPayload, ct, requiredScopes); + var patched = false; + try + { + patched = await _graphApiService.GraphPatchAsync(tenantId, patchPath, patchPayload, ct, requiredScopes); + } + catch (Exception patchEx) + { + if (patchEx is OperationCanceledException && ct.IsCancellationRequested) throw; + _logger.LogError(patchEx, "Exception during PATCH of inheritable permissions for blueprint {Blueprint} resource {Resource}", blueprintObjectId, resourceAppId); + return (ok: false, alreadyExists: false, error: patchEx.Message); + } + if (!patched) { - return (ok: false, alreadyExists: false, error: "PATCH failed"); + _logger.LogWarning("PATCH request to update inheritable permissions failed for blueprint {Blueprint} resource {Resource}", blueprintObjectId, resourceAppId); + return (ok: false, alreadyExists: false, error: $"Graph PATCH returned false for blueprint {blueprintObjectId} resource {resourceAppId}"); } _logger.LogDebug("Patched inheritable permissions for blueprint {Blueprint} resource {Resource}", blueprintObjectId, resourceAppId); @@ -429,7 +442,8 @@ public virtual async Task DeleteAgentUserAsync( } catch (Exception ex) { - _logger.LogError("Failed to set inheritable permissions: {Error}", ex.Message); + if (ex is OperationCanceledException && ct.IsCancellationRequested) throw; + _logger.LogError(ex, "Failed to set inheritable permissions for blueprint {Blueprint} resource {Resource}: {Error}", blueprintObjectId ?? blueprintId, resourceAppId, ex.Message); return (ok: false, alreadyExists: false, error: ex.Message); } } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs index d85e33af..628d630a 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs @@ -165,6 +165,100 @@ public async Task SetInheritablePermissionsAsync_Patches_WhenPresent() err.Should().BeNull(); } + [Fact] + public async Task SetInheritablePermissionsAsync_ReturnsFalse_WhenPatchThrows() + { + // Arrange — use a subclass that overrides GraphPatchAsync to throw, + // simulating a transient network error during the PATCH call (#366 regression). + var handler = new FakeHttpMessageHandler(); + var executor = Substitute.For(Substitute.For>()); + executor.ExecuteAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(callInfo => + { + var args = callInfo.ArgAt(1); + if (args != null && args.Contains("get-access-token", StringComparison.OrdinalIgnoreCase)) + return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "fake-token", StandardError = string.Empty }); + return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "{}", StandardError = string.Empty }); + }); + + var graphService = new ThrowingOnPatchGraphApiService(_mockGraphLogger, executor, FakeAuth(), handler); + var service = new AgentBlueprintService(_mockLogger, graphService); + + // ResolveBlueprintObjectIdAsync: 404 → resolve via appId filter + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.NotFound)); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(JsonSerializer.Serialize(new { value = new[] { new { id = "resolved-object-id" } } })) + }); + + // GET existing permissions — returns an entry so the merge+PATCH path is taken + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(JsonSerializer.Serialize(new + { + value = new[] { new { resourceAppId = "resAppId", inheritableScopes = new { scopes = new[] { "scope1" } } } } + })) + }); + + // Act + var (ok, already, err) = await service.SetInheritablePermissionsAsync("tid", "bpAppId", "resAppId", new[] { "scope2" }); + + // Assert — must not throw; must surface the failure gracefully + ok.Should().BeFalse(because: "GraphPatchAsync threw an exception and the caller must not crash"); + already.Should().BeFalse(); + err.Should().Be("Network error during PATCH"); + } + + [Fact] + public async Task SetInheritablePermissionsAsync_ReturnsFalse_WhenPatchReturnsFalse() + { + // Arrange — GraphPatchAsync succeeds at the HTTP level but returns a non-2xx status, + // causing GraphPatchAsync to return false. The method must return (ok: false) without throwing. + var handler = new FakeHttpMessageHandler(); + var executor = Substitute.For(Substitute.For>()); + executor.ExecuteAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(callInfo => + { + var args = callInfo.ArgAt(1); + if (args != null && args.Contains("get-access-token", StringComparison.OrdinalIgnoreCase)) + return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "fake-token", StandardError = string.Empty }); + return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "{}", StandardError = string.Empty }); + }); + + var graphService = new GraphApiService(_mockGraphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); + var service = new AgentBlueprintService(_mockLogger, graphService); + + // ResolveBlueprintObjectIdAsync: 404 → resolve via appId filter + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.NotFound)); + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(JsonSerializer.Serialize(new { value = new[] { new { id = "resolved-object-id" } } })) + }); + + // GET existing permissions — returns an entry so the PATCH path is taken + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(JsonSerializer.Serialize(new + { + value = new[] { new { resourceAppId = "resAppId", inheritableScopes = new { scopes = new[] { "scope1" } } } } + })) + }); + + // PATCH returns 400 Bad Request — GraphPatchAsync returns false + handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.BadRequest) + { + Content = new StringContent("{\"error\":{\"code\":\"BadRequest\",\"message\":\"Invalid payload\"}}") + }); + + // Act + var (ok, already, err) = await service.SetInheritablePermissionsAsync("tid", "bpAppId", "resAppId", new[] { "scope2" }); + + // Assert — must not throw; must surface the failure gracefully + ok.Should().BeFalse(because: "GraphPatchAsync returned false (HTTP 400)"); + already.Should().BeFalse(); + err.Should().Contain("Graph PATCH returned false", because: "error message must identify the failing operation"); + } + [Fact] public async Task DeleteAgentIdentityAsync_WithValidIdentity_ReturnsTrue() { @@ -440,6 +534,25 @@ public async Task DeleteAgentUserAsync_ReturnsFalse_OnGraphError() } } + // Test helper — overrides GraphPatchAsync to throw, simulating a transient network failure. + private sealed class ThrowingOnPatchGraphApiService : GraphApiService + { + public ThrowingOnPatchGraphApiService( + ILogger logger, + CommandExecutor executor, + IAuthenticationService authService, + HttpMessageHandler handler) + : base(logger, executor, authService, handler) { } + + public override Task GraphPatchAsync( + string tenantId, + string relativePath, + object payload, + CancellationToken ct = default, + IEnumerable? scopes = null) + => Task.FromException(new HttpRequestException("Network error during PATCH")); + } + private static IAuthenticationService FakeAuth() { var mock = Substitute.For();