From 8b62981bb8a5c7fd7475de3d75805223c2eb82f5 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Fri, 27 Mar 2026 12:55:45 -0700 Subject: [PATCH 1/5] Add confirmation prompts for app registration changes - Prompt user before mutating app registrations; add --yes flag to skip prompts for CI/non-interactive use - Show mutation summary before applying changes (permissions, redirect URIs, public client flows) - Improve error and warning logging for requirement checks - Propagate OperationCanceledException for clean Ctrl+C exits - Enhance ClientAppValidator with read-only mutation checks and consent helpers - Expand documentation and update tests for new behaviors --- .../Commands/ConfigCommand.cs | 9 +- .../SetupSubcommands/AdminSubcommand.cs | 2 +- .../SetupSubcommands/AllSubcommand.cs | 2 +- .../SetupSubcommands/BlueprintSubcommand.cs | 2 +- .../Constants/AuthenticationConstants.cs | 12 +- .../Services/ClientAppValidator.cs | 170 +++++++++++++++++- .../Services/ConfigurationWizardService.cs | 2 +- .../Services/ConsoleConfirmationProvider.cs | 14 +- .../Services/IClientAppValidator.cs | 4 +- .../Services/Requirements/RequirementCheck.cs | 19 +- .../ClientAppRequirementCheck.cs | 12 +- .../ClientAppRequirementCheckTests.cs | 37 +++- .../FrontierPreviewRequirementCheckTests.cs | 4 +- 13 files changed, 255 insertions(+), 34 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/ConfigCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/ConfigCommand.cs index 0d77e5b3..3d4ea6b0 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/ConfigCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/ConfigCommand.cs @@ -35,15 +35,18 @@ private static Command CreateInitSubcommand(ILogger logger, string configDir, IC { new Option(new[] { "-c", "--configfile" }, "Path to an existing config file to import"), new Option(new[] { "--global", "-g" }, "Create config in global directory (AppData) instead of current directory"), + new Option(new[] { "--yes", "-y" }, "Skip confirmation prompts and apply any required app registration changes automatically"), }; cmd.SetHandler(async (System.CommandLine.Invocation.InvocationContext context) => { var configFileOption = cmd.Options.OfType>().First(opt => opt.HasAlias("-c")); var globalOption = cmd.Options.OfType>().First(opt => opt.HasAlias("--global")); + var yesOption = cmd.Options.OfType>().First(opt => opt.HasAlias("--yes")); string? configFile = context.ParseResult.GetValueForOption(configFileOption); bool useGlobal = context.ParseResult.GetValueForOption(globalOption); + bool yes = context.ParseResult.GetValueForOption(yesOption); // Determine config path string configPath = useGlobal @@ -95,7 +98,8 @@ private static Command CreateInitSubcommand(ILogger logger, string configDir, IC await clientAppValidator.EnsureValidClientAppAsync( importedConfig.ClientAppId, importedConfig.TenantId, - context.GetCancellationToken()); + skipConfirmation: yes, + ct: context.GetCancellationToken()); } catch (ClientAppValidationException ex) { @@ -108,9 +112,10 @@ await clientAppValidator.EnsureValidClientAppAsync( } if (ex.MitigationSteps.Count > 0) { + logger.LogInformation(""); foreach (var step in ex.MitigationSteps) { - logger.LogError(step); + logger.LogInformation(" {Step}", step.TrimEnd()); } } logger.LogError(""); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs index 1380e0ba..28990f18 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs @@ -137,7 +137,7 @@ public static Command CreateCommand( await RequirementsSubcommand.RunChecksOrExitAsync( checks, setupConfig, logger, ct); } - catch (Exception reqEx) when (reqEx is not OperationCanceledException) + catch (Exception reqEx) when (reqEx is not OperationCanceledException && reqEx is not CleanExitException) { logger.LogError(reqEx, "Requirements check failed: {Message}", reqEx.Message); logger.LogError("Rerun with --skip-requirements to bypass."); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs index cc194edd..99f4943b 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs @@ -168,7 +168,7 @@ public static Command CreateCommand( await RequirementsSubcommand.RunChecksOrExitAsync( checks, setupConfig, logger, ct); } - catch (Exception reqEx) when (reqEx is not OperationCanceledException) + catch (Exception reqEx) when (reqEx is not OperationCanceledException && reqEx is not CleanExitException) { logger.LogError(reqEx, "Requirements check failed with an unexpected error: {Message}", reqEx.Message); logger.LogError("If you want to bypass requirement validation, rerun this command with the --skip-requirements flag."); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs index b7ce3663..b132dd7f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs @@ -226,7 +226,7 @@ public static Command CreateCommand( await RequirementsSubcommand.RunChecksOrExitAsync( checks, setupConfig, logger, ct); } - catch (Exception reqEx) when (reqEx is not OperationCanceledException) + catch (Exception reqEx) when (reqEx is not OperationCanceledException && reqEx is not CleanExitException) { logger.LogError(reqEx, "Requirements check failed with an unexpected error: {Message}", reqEx.Message); logger.LogError("If you want to bypass requirement validation, rerun this command with the --skip-requirements flag."); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs index 16b94499..860fb0b3 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs @@ -30,8 +30,16 @@ public static class AuthenticationConstants public const string LocalhostRedirectUri = "http://localhost:8400/"; /// - /// Required redirect URIs for Microsoft Graph PowerShell SDK authentication. - /// The SDK requires both http://localhost and http://localhost:8400/ for different auth flows. + /// Required redirect URIs for authentication. + /// + /// http://localhostRequired by the Microsoft Graph PowerShell SDK + /// (Connect-MgGraph -ClientId). Without this URI, PowerShell-based operations (OAuth2 grants, + /// service principal lookups) fall back to the Azure CLI token, which lacks required delegated + /// permissions and causes 403 errors on inheritable permissions operations. + /// http://localhost:8400/Required by MSAL for interactive browser + /// authentication. Uses a fixed port to ensure consistent OAuth callbacks. + /// + /// See also for the Windows WAM broker URI. /// public static readonly string[] RequiredRedirectUris = new[] { diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs index eb155eb4..7e6f3165 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs @@ -19,11 +19,13 @@ public sealed class ClientAppValidator : IClientAppValidator { private readonly ILogger _logger; private readonly GraphApiService _graphApiService; + private readonly IConfirmationProvider? _confirmationProvider; - public ClientAppValidator(ILogger logger, GraphApiService graphApiService) + public ClientAppValidator(ILogger logger, GraphApiService graphApiService, IConfirmationProvider? confirmationProvider = null) { _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _graphApiService = graphApiService ?? throw new ArgumentNullException(nameof(graphApiService)); + _confirmationProvider = confirmationProvider; } /// @@ -33,11 +35,14 @@ public ClientAppValidator(ILogger logger, GraphApiService gr /// /// The client app ID to validate /// The tenant ID where the app should exist + /// When true, applies any required app registration fixes without prompting the user. + /// Use for non-interactive or CI scenarios. Defaults to false (prompt before modifying the app registration). /// Cancellation token /// Thrown when validation fails public async Task EnsureValidClientAppAsync( string clientAppId, string tenantId, + bool skipConfirmation = false, CancellationToken ct = default) { ArgumentException.ThrowIfNullOrWhiteSpace(clientAppId); @@ -71,7 +76,7 @@ public async Task EnsureValidClientAppAsync( _logger.LogDebug("Found client app: {DisplayName} ({AppId})", appInfo.DisplayName, clientAppId); - // Step 3: Validate permissions in manifest + // Step 3: Validate permissions in manifest (read-only) var missingPermissions = await ValidatePermissionsConfiguredAsync(appInfo, tenantId, ct); // Step 3.5: For any unresolvable permissions (beta APIs), check oauth2PermissionGrants as fallback @@ -87,8 +92,48 @@ public async Task EnsureValidClientAppAsync( } } + // Read-only pre-flight: collect what redirect URIs and public client settings need fixing + var missingRedirectUris = await CollectMissingRedirectUrisAsync(clientAppId, tenantId, ct); + var publicClientNeedsEnabling = await IsPublicClientFlowsDisabledAsync(clientAppId, tenantId, ct); + + // Determine what mutations are needed + bool hasMissingPermissions = missingPermissions.Count > 0; + bool hasMissingRedirectUris = missingRedirectUris.Count > 0; + bool needsPublicClientEnabled = publicClientNeedsEnabling; + bool hasPendingMutations = hasMissingPermissions || hasMissingRedirectUris || needsPublicClientEnabled; + + // Prompt the user before making any changes (unless skipConfirmation or no confirmation provider) + bool applyFixes = true; + if (hasPendingMutations && _confirmationProvider != null && !skipConfirmation) + { + _logger.LogInformation("The following changes will be applied to app registration ({AppId}):", clientAppId); + _logger.LogInformation(""); + if (hasMissingPermissions) + { + _logger.LogInformation(" - Add permissions and grant admin consent:"); + foreach (var perm in missingPermissions) + _logger.LogInformation(" {Permission}", perm); + } + if (hasMissingRedirectUris) + { + _logger.LogInformation(" - Add redirect URIs:"); + foreach (var uri in missingRedirectUris) + _logger.LogInformation(" {Uri}", uri); + } + if (needsPublicClientEnabled) + _logger.LogInformation(" - Enable 'Allow public client flows' (required for device code fallback)"); + _logger.LogInformation("For more information: https://learn.microsoft.com/en-us/microsoft-agent-365/developer/custom-client-app-registration"); + _logger.LogInformation(""); + + applyFixes = await _confirmationProvider.ConfirmAsync("Do you want to proceed? (y/N): "); + if (!applyFixes) + { + _logger.LogInformation("App registration was not modified. Re-run and accept the prompt, or configure manually."); + } + } + // Step 3.6: Auto-provision any remaining missing permissions (self-healing) - if (missingPermissions.Count > 0) + if (applyFixes && missingPermissions.Count > 0) { _logger.LogInformation("Auto-provisioning {Count} missing permission(s): {Permissions}", missingPermissions.Count, string.Join(", ", missingPermissions)); @@ -125,10 +170,12 @@ public async Task EnsureValidClientAppAsync( } // Step 5: Verify and fix redirect URIs - await EnsureRedirectUrisAsync(clientAppId, tenantId, ct); + if (applyFixes) + await EnsureRedirectUrisAsync(clientAppId, tenantId, ct); - // Step 6: Verify and fix public client flows (required for device code fallback on non-Windows) - await EnsurePublicClientFlowsEnabledAsync(clientAppId, tenantId, ct); + // Step 6: Verify and fix public client flows (required for device code fallback) + if (applyFixes) + await EnsurePublicClientFlowsEnabledAsync(clientAppId, tenantId, ct); _logger.LogDebug("Client app validation successful for {ClientAppId}", clientAppId); } @@ -137,6 +184,11 @@ public async Task EnsureValidClientAppAsync( // Re-throw validation exceptions as-is throw; } + catch (OperationCanceledException) + { + // Ctrl+C / cancellation — propagate immediately without wrapping + throw; + } catch (JsonException ex) { _logger.LogDebug(ex, "JSON parsing error during validation"); @@ -298,7 +350,10 @@ private async Task EnsurePublicClientFlowsEnabledAsync( return; } - _logger.LogInformation("Enabling 'Allow public client flows' on app registration (required for device code authentication fallback)."); + _logger.LogInformation( + "Enabling 'Allow public client flows' on app registration " + + "(required for device code authentication fallback on macOS, Linux, WSL, " + + "headless environments, and as a Conditional Access Policy fallback on Windows)."); _logger.LogInformation("Run 'a365 setup requirements' at any time to re-verify and auto-fix this setting."); var patchSuccess = await _graphApiService.GraphPatchAsync(tenantId, @@ -524,7 +579,11 @@ private async Task TryExtendConsentGrantScopesAsync( if (patchSuccess) { - _logger.LogInformation("Extended consent grant with scope(s): {Scopes}", string.Join(", ", scopesToAdd)); + _logger.LogInformation("Extending admin consent grant with {Count} new permission(s): {Scopes}.", + scopesToAdd.Count, string.Join(", ", scopesToAdd)); + // Invalidate the process-level az CLI token cache so the next Graph call + // re-acquires a token that includes the newly consented scope(s). + Services.Helpers.AzCliHelper.InvalidateAzCliTokenCache(); } else { @@ -540,6 +599,101 @@ private async Task TryExtendConsentGrantScopesAsync( } } + /// + /// Returns the subset of + /// that are not yet present in the client app's oauth2PermissionGrant (i.e. not consented). + /// + public async Task> GetUnconsentedRequiredPermissionsAsync( + string clientAppId, + string tenantId, + CancellationToken ct = default) + { + var consented = await GetConsentedPermissionsAsync(clientAppId, tenantId, ct); + return AuthenticationConstants.RequiredClientAppPermissions + .Where(p => !consented.Contains(p, StringComparer.OrdinalIgnoreCase)) + .ToList(); + } + + /// + /// Extends the client app's oauth2PermissionGrant to include the specified permissions. + /// Call after the user has confirmed they want to grant admin consent. + /// + public Task GrantConsentForPermissionsAsync( + string clientAppId, + List permissions, + string tenantId, + CancellationToken ct = default) + => TryExtendConsentGrantScopesAsync(clientAppId, permissions, tenantId, ct); + + + /// + /// Read-only check: returns the redirect URIs that are missing from the app registration + /// without making any changes. Used to build the pre-flight mutation summary. + /// + private async Task> CollectMissingRedirectUrisAsync( + string clientAppId, + string tenantId, + CancellationToken ct) + { + try + { + using var appDoc = await _graphApiService.GraphGetAsync(tenantId, + $"/v1.0/applications?$filter=appId eq '{clientAppId}'&$select=id,publicClient", ct); + + if (appDoc == null) return new List(); + + var response = JsonNode.Parse(appDoc.RootElement.GetRawText()); + var apps = response?["value"]?.AsArray(); + if (apps == null || apps.Count == 0) return new List(); + + var publicClient = apps[0]!.AsObject()["publicClient"]?.AsObject(); + var currentRedirectUris = publicClient?["redirectUris"]?.AsArray() + ?.Select(uri => uri?.GetValue()) + .Where(uri => !string.IsNullOrWhiteSpace(uri)) + .Select(uri => uri!) + .ToHashSet(StringComparer.OrdinalIgnoreCase) ?? new HashSet(StringComparer.OrdinalIgnoreCase); + + return AuthenticationConstants.GetRequiredRedirectUris(clientAppId) + .Where(uri => !currentRedirectUris.Contains(uri)) + .ToList(); + } + catch (Exception ex) + { + _logger.LogDebug("CollectMissingRedirectUrisAsync failed (non-fatal): {Message}", ex.Message); + return new List(); + } + } + + /// + /// Read-only check: returns true if 'Allow public client flows' (isFallbackPublicClient) + /// is currently disabled on the app registration, without making any changes. + /// + private async Task IsPublicClientFlowsDisabledAsync( + string clientAppId, + string tenantId, + CancellationToken ct) + { + try + { + using var appDoc = await _graphApiService.GraphGetAsync(tenantId, + $"/v1.0/applications?$filter=appId eq '{clientAppId}'&$select=id,isFallbackPublicClient", ct); + + if (appDoc == null) return false; + + var response = JsonNode.Parse(appDoc.RootElement.GetRawText()); + var apps = response?["value"]?.AsArray(); + if (apps == null || apps.Count == 0) return false; + + var isFallbackPublicClient = apps[0]!.AsObject()["isFallbackPublicClient"]?.GetValue() ?? false; + return !isFallbackPublicClient; + } + catch (Exception ex) + { + _logger.LogDebug("IsPublicClientFlowsDisabledAsync failed (non-fatal): {Message}", ex.Message); + return false; + } + } + #region Private Helper Methods private async Task GetClientAppInfoAsync(string clientAppId, string tenantId, CancellationToken ct) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConfigurationWizardService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConfigurationWizardService.cs index 36950c66..f0fa2829 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConfigurationWizardService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConfigurationWizardService.cs @@ -988,7 +988,7 @@ private string GetUsageLocationFromAccount(AzureAccountInfo accountInfo) try { - await validator.EnsureValidClientAppAsync(clientAppId, tenantId, CancellationToken.None); + await validator.EnsureValidClientAppAsync(clientAppId, tenantId, ct: CancellationToken.None); Console.WriteLine("Client app validation successful!"); Console.WriteLine(); return clientAppId; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConsoleConfirmationProvider.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConsoleConfirmationProvider.cs index 0f4a2dc1..8c044972 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConsoleConfirmationProvider.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConsoleConfirmationProvider.cs @@ -16,8 +16,18 @@ public class ConsoleConfirmationProvider : IConfirmationProvider public Task ConfirmAsync(string prompt) { Console.Write(prompt); - var response = Console.ReadLine()?.Trim().ToLowerInvariant(); - return Task.FromResult(response == "y" || response == "yes"); + var response = Console.ReadLine(); + + // null means stdin was closed (Ctrl+C / EOF) — exit immediately without further output, + // matching az CLI behavior where Ctrl+C terminates cleanly with no trailing messages. + if (response == null) + { + Console.WriteLine(); + throw new OperationCanceledException(); + } + + return Task.FromResult(response.Trim().Equals("y", StringComparison.OrdinalIgnoreCase) + || response.Trim().Equals("yes", StringComparison.OrdinalIgnoreCase)); } /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IClientAppValidator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IClientAppValidator.cs index 199f0996..b218c24c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/IClientAppValidator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/IClientAppValidator.cs @@ -15,9 +15,11 @@ public interface IClientAppValidator /// /// The client app ID to validate /// The tenant ID where the app should exist + /// When true, applies any required app registration fixes without prompting the user. + /// Use for non-interactive or CI scenarios. Defaults to false (prompt before modifying the app registration). /// Cancellation token /// Thrown when validation fails - Task EnsureValidClientAppAsync(string clientAppId, string tenantId, CancellationToken ct = default); + Task EnsureValidClientAppAsync(string clientAppId, string tenantId, bool skipConfirmation = false, CancellationToken ct = default); /// /// Ensures the client app has required redirect URIs configured for Microsoft Graph PowerShell SDK. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs index c75e98ac..3b663e10 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs @@ -37,7 +37,7 @@ protected virtual void LogCheckSuccess(ILogger logger, string? details = null) /// protected virtual void LogCheckWarning(ILogger logger, string? message = null) { - logger.LogWarning("[WARN] {Name}{Details}", Name, + logger.LogWarning("WARNING: {Name}{Details}", Name, string.IsNullOrWhiteSpace(message) ? "" : $" - {message}"); } @@ -46,9 +46,20 @@ protected virtual void LogCheckWarning(ILogger logger, string? message = null) /// protected virtual void LogCheckFailure(ILogger logger, string errorMessage, string resolutionGuidance) { - logger.LogError("[FAIL] {Name}", Name); - logger.LogError(" Issue: {ErrorMessage}", errorMessage); - logger.LogError(" Resolution: {ResolutionGuidance}", resolutionGuidance); + // Name logged at Error level (red) — formatter already prefixes ERROR: + logger.LogError("{Name}", Name); + + // Error details in red (split multi-line messages into separate lines) + foreach (var line in errorMessage.Split('\n', StringSplitOptions.RemoveEmptyEntries)) + logger.LogError(" {Line}", line.TrimEnd()); + + // Resolution guidance in white (not red) — it is helpful guidance, not an error + if (!string.IsNullOrWhiteSpace(resolutionGuidance)) + { + logger.LogInformation(""); + foreach (var step in resolutionGuidance.Split('\n', StringSplitOptions.RemoveEmptyEntries)) + logger.LogInformation(" {Step}", step.TrimEnd()); + } } /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/ClientAppRequirementCheck.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/ClientAppRequirementCheck.cs index 7d485788..fcfdf7f7 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/ClientAppRequirementCheck.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/ClientAppRequirementCheck.cs @@ -66,7 +66,7 @@ private async Task CheckImplementationAsync(Agent365Conf await _clientAppValidator.EnsureValidClientAppAsync( config.ClientAppId, config.TenantId, - cancellationToken + ct: cancellationToken ); return RequirementCheckResult.Success( @@ -75,13 +75,19 @@ await _clientAppValidator.EnsureValidClientAppAsync( } catch (ClientAppValidationException ex) { - // Convert ClientAppValidationException to RequirementCheckResult + // Use IssueDescription + ErrorDetails to avoid exposing internal error codes from ex.Message + var errorLines = new List { ex.IssueDescription }; + errorLines.AddRange(ex.ErrorDetails); return RequirementCheckResult.Failure( - errorMessage: ex.Message, + errorMessage: string.Join("\n", errorLines), resolutionGuidance: string.Join("\n", ex.MitigationSteps), details: $"Client app validation failed for {config.ClientAppId}. Please ensure the app exists and has the required configuration." ); } + catch (OperationCanceledException) + { + throw; + } catch (Exception ex) { logger.LogDebug(ex, "Unexpected error validating client app"); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/ClientAppRequirementCheckTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/ClientAppRequirementCheckTests.cs index fa2ba32f..4374af04 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/ClientAppRequirementCheckTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/ClientAppRequirementCheckTests.cs @@ -85,7 +85,7 @@ public async Task CheckAsync_WithValidClientApp_ShouldReturnSuccess() _mockValidator.EnsureValidClientAppAsync( config.ClientAppId, config.TenantId, - Arg.Any()) + ct: Arg.Any()) .Returns(Task.CompletedTask); // Act @@ -102,7 +102,7 @@ public async Task CheckAsync_WithValidClientApp_ShouldReturnSuccess() await _mockValidator.Received(1).EnsureValidClientAppAsync( config.ClientAppId, config.TenantId, - Arg.Any()); + ct: Arg.Any()); } [Fact] @@ -131,7 +131,7 @@ public async Task CheckAsync_WithClientAppValidationException_ShouldReturnFailur _mockValidator.EnsureValidClientAppAsync( config.ClientAppId, config.TenantId, - Arg.Any()) + ct: Arg.Any()) .Throws(validationException); // Act @@ -163,7 +163,7 @@ public async Task CheckAsync_WithUnexpectedException_ShouldReturnFailure() _mockValidator.EnsureValidClientAppAsync( config.ClientAppId, config.TenantId, - Arg.Any()) + ct: Arg.Any()) .Throws(unexpectedException); // Act @@ -260,6 +260,31 @@ public async Task CheckAsync_WithEmptyStringTenantId_ShouldReturnFailure() result.ErrorMessage.Should().Contain("tenantId is not configured"); } + [Fact] + public async Task CheckAsync_WhenValidatorThrowsOperationCanceledException_ShouldPropagate() + { + // Arrange + var check = new ClientAppRequirementCheck(_mockValidator); + var config = new Agent365Config + { + ClientAppId = "test-client-app-id", + TenantId = "test-tenant-id" + }; + + _mockValidator.EnsureValidClientAppAsync( + config.ClientAppId, + config.TenantId, + ct: Arg.Any()) + .Throws(new OperationCanceledException()); + + // Act + var act = async () => await check.CheckAsync(config, _mockLogger); + + // Assert — Ctrl+C must propagate, not be swallowed into a failure result + await act.Should().ThrowAsync( + because: "cancellation must exit cleanly without logging a failure result"); + } + [Fact] public async Task CheckAsync_ShouldPassCancellationTokenToValidator() { @@ -275,7 +300,7 @@ public async Task CheckAsync_ShouldPassCancellationTokenToValidator() _mockValidator.EnsureValidClientAppAsync( config.ClientAppId, config.TenantId, - Arg.Any()) + ct: Arg.Any()) .Returns(Task.CompletedTask); // Act @@ -285,6 +310,6 @@ public async Task CheckAsync_ShouldPassCancellationTokenToValidator() await _mockValidator.Received(1).EnsureValidClientAppAsync( config.ClientAppId, config.TenantId, - cancellationToken); + ct: cancellationToken); } } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/FrontierPreviewRequirementCheckTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/FrontierPreviewRequirementCheckTests.cs index d82c14ff..3277dee4 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/FrontierPreviewRequirementCheckTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/FrontierPreviewRequirementCheckTests.cs @@ -52,11 +52,11 @@ public async Task CheckAsync_ShouldLogMainWarningMessage() // Act await check.CheckAsync(config, _mockLogger); - // Assert — [WARN] output is logged at Warning severity (yellow color, no WARNING: text prefix from formatter) + // Assert — WARNING: prefix is included in the message (logged at Warning severity → yellow in formatter) _mockLogger.Received().Log( LogLevel.Warning, Arg.Any(), - Arg.Is(o => o.ToString()!.Contains("[WARN] Frontier Preview Program")), + Arg.Is(o => o.ToString()!.Contains("WARNING: Frontier Preview Program")), Arg.Any(), Arg.Any>()); } From d690e539ccde1667e50daebc6a3828613315af2d Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Fri, 27 Mar 2026 14:18:28 -0700 Subject: [PATCH 2/5] Fix PR comments. Improve error handling for AAD app config checks Switch to fail-closed logic in client app validation methods to ensure users are prompted on errors, preventing silent misconfiguration. Use GraphApiService with correct permissions for service principal propagation checks to avoid 403s and unnecessary retries. Update log messages to clarify intentional fail-closed behavior for safer, more robust error handling. --- .../SetupSubcommands/BlueprintSubcommand.cs | 16 ++++++++++------ .../Services/ClientAppValidator.cs | 12 ++++++++---- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs index b132dd7f..3655e129 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs @@ -1060,12 +1060,16 @@ public static async Task EnsureDelegatedConsentWithRetriesAsync( var spPropagated = await retryHelper.ExecuteWithRetryAsync( async ct => { - // Probe oauth2PermissionGrants directly — a 200 (even empty list) confirms - // the SP's clientId is visible to the grants API replication layer. - // GET /servicePrincipals resolves too fast and gives false confidence. - using var checkResp = await httpClient.GetAsync( - $"{Constants.GraphApiConstants.BaseUrl}/v1.0/oauth2PermissionGrants?$filter=clientId eq '{servicePrincipalId}'", ct); - return checkResp.IsSuccessStatusCode; + // Probe oauth2PermissionGrants via GraphApiService (which acquires a token + // with DelegatedPermissionGrant.ReadWrite.All scope). A non-null response — + // even an empty list — confirms the SP's clientId is visible to the grants + // API replication layer. Using the raw httpClient here (Application.ReadWrite.All + // scope only) caused 403s on every probe, wasting 8+ minutes of retries. + using var checkDoc = await graphApiService.GraphGetAsync( + setupConfig.TenantId!, + $"/v1.0/oauth2PermissionGrants?$filter=clientId eq '{servicePrincipalId}'", + ct); + return checkDoc != null; }, result => !result, maxRetries: 12, diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs index 7e6f3165..16b35eea 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs @@ -659,8 +659,10 @@ private async Task> CollectMissingRedirectUrisAsync( } catch (Exception ex) { - _logger.LogDebug("CollectMissingRedirectUrisAsync failed (non-fatal): {Message}", ex.Message); - return new List(); + // On error, assume all redirect URIs are missing so the prompt still appears. + // Failing closed (prompt) is safer than failing open (silent mutation without disclosure). + _logger.LogDebug("CollectMissingRedirectUrisAsync failed — assuming all redirect URIs missing: {Message}", ex.Message); + return AuthenticationConstants.GetRequiredRedirectUris(clientAppId).ToList(); } } @@ -689,8 +691,10 @@ private async Task IsPublicClientFlowsDisabledAsync( } catch (Exception ex) { - _logger.LogDebug("IsPublicClientFlowsDisabledAsync failed (non-fatal): {Message}", ex.Message); - return false; + // On error, assume public client flows need enabling so the prompt still appears. + // Failing closed (prompt) is safer than failing open (silent mutation without disclosure). + _logger.LogDebug("IsPublicClientFlowsDisabledAsync failed — assuming public client flows need enabling: {Message}", ex.Message); + return true; } } From 07d91ed6a060fdae8296520157d35a63c94a9123 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Fri, 27 Mar 2026 14:25:46 -0700 Subject: [PATCH 3/5] fix PR comment: Update warning log prefix to [WARN] in checks and tests Changed warning log prefix from "WARNING:" to "[WARN]" in RequirementCheck. Updated related unit test to expect the new prefix, ensuring consistency between implementation and test expectations. --- .../Services/Requirements/RequirementCheck.cs | 2 +- .../Requirements/FrontierPreviewRequirementCheckTests.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs index 3b663e10..30f07470 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs @@ -37,7 +37,7 @@ protected virtual void LogCheckSuccess(ILogger logger, string? details = null) /// protected virtual void LogCheckWarning(ILogger logger, string? message = null) { - logger.LogWarning("WARNING: {Name}{Details}", Name, + logger.LogWarning("[WARN] {Name}{Details}", Name, string.IsNullOrWhiteSpace(message) ? "" : $" - {message}"); } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/FrontierPreviewRequirementCheckTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/FrontierPreviewRequirementCheckTests.cs index 3277dee4..1a6dc6be 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/FrontierPreviewRequirementCheckTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/FrontierPreviewRequirementCheckTests.cs @@ -52,11 +52,11 @@ public async Task CheckAsync_ShouldLogMainWarningMessage() // Act await check.CheckAsync(config, _mockLogger); - // Assert — WARNING: prefix is included in the message (logged at Warning severity → yellow in formatter) + // Assert — [WARN] prefix is included in the message (logged at Warning severity → yellow in formatter) _mockLogger.Received().Log( LogLevel.Warning, Arg.Any(), - Arg.Is(o => o.ToString()!.Contains("WARNING: Frontier Preview Program")), + Arg.Is(o => o.ToString()!.Contains("[WARN] Frontier Preview Program")), Arg.Any(), Arg.Any>()); } From b03a45d337a76012841fa2bdee6a177540b13acc Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Fri, 27 Mar 2026 15:40:49 -0700 Subject: [PATCH 4/5] Fix copilot PR comments. Improve client app validation prompts and test coverage Enhance ClientAppValidator to throw detailed exceptions when the user declines to apply required fixes (permissions, redirect URIs, public client flows), listing all pending issues for manual configuration. Update BlueprintSubcommand to clarify delegated scope usage. Add "[FAIL]" prefix to failed requirement logs. Introduce comprehensive tests for confirmation prompt logic, ensuring correct behavior for both acceptance and decline scenarios, and improve test setup utilities. These changes increase robustness, user feedback, and test coverage for the validation and self-healing process. --- .../SetupSubcommands/BlueprintSubcommand.cs | 7 +- .../Services/ClientAppValidator.cs | 12 + .../Services/Requirements/RequirementCheck.cs | 2 +- .../Services/ClientAppValidatorTests.cs | 300 ++++++++++++++++++ 4 files changed, 317 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs index 3655e129..fc67041e 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs @@ -1060,15 +1060,16 @@ public static async Task EnsureDelegatedConsentWithRetriesAsync( var spPropagated = await retryHelper.ExecuteWithRetryAsync( async ct => { - // Probe oauth2PermissionGrants via GraphApiService (which acquires a token - // with DelegatedPermissionGrant.ReadWrite.All scope). A non-null response — + // Probe oauth2PermissionGrants via GraphApiService with explicit delegated + // scopes (DelegatedPermissionGrant.ReadWrite.All). A non-null response — // even an empty list — confirms the SP's clientId is visible to the grants // API replication layer. Using the raw httpClient here (Application.ReadWrite.All // scope only) caused 403s on every probe, wasting 8+ minutes of retries. using var checkDoc = await graphApiService.GraphGetAsync( setupConfig.TenantId!, $"/v1.0/oauth2PermissionGrants?$filter=clientId eq '{servicePrincipalId}'", - ct); + ct, + scopes: AuthenticationConstants.RequiredPermissionGrantScopes); return checkDoc != null; }, result => !result, diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs index 16b35eea..bdc7e086 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs @@ -129,6 +129,18 @@ public async Task EnsureValidClientAppAsync( if (!applyFixes) { _logger.LogInformation("App registration was not modified. Re-run and accept the prompt, or configure manually."); + + var details = new List(); + if (hasMissingPermissions) + details.Add($"Missing permissions: {string.Join(", ", missingPermissions)}"); + if (hasMissingRedirectUris) + details.Add($"Missing redirect URIs: {string.Join(", ", missingRedirectUris)}"); + if (needsPublicClientEnabled) + details.Add("Public client flows ('Allow public client flows') must be enabled"); + throw ClientAppValidationException.ValidationFailed( + "App registration changes were declined — manual configuration required", + details, + clientAppId); } } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs index 30f07470..191900fa 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs @@ -47,7 +47,7 @@ protected virtual void LogCheckWarning(ILogger logger, string? message = null) protected virtual void LogCheckFailure(ILogger logger, string errorMessage, string resolutionGuidance) { // Name logged at Error level (red) — formatter already prefixes ERROR: - logger.LogError("{Name}", Name); + logger.LogError("[FAIL] {Name}", Name); // Error details in red (split multi-line messages into separate lines) foreach (var line in errorMessage.Split('\n', StringSplitOptions.RemoveEmptyEntries)) diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs index e92d1a55..17c6815f 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs @@ -319,6 +319,306 @@ public async Task EnsureValidClientAppAsync_WhenPublicClientFlowsPatchFails_Does #endregion + #region Confirmation Prompt Tests + + private ClientAppValidator CreateValidatorWithConfirmation(IConfirmationProvider confirmationProvider) + { + var executorLogger = Substitute.For>(); + var executor = Substitute.For(executorLogger); + var graphServiceLogger = Substitute.For>(); + var graphApiService = Substitute.For(graphServiceLogger, executor); + + // Wire up the same app/permission mocks used by the happy-path tests + var requiredResourceAccess = $$""" + [ + { + "resourceAppId": "{{AuthenticationConstants.MicrosoftGraphResourceAppId}}", + "resourceAccess": [ + {"id": "{{ApplicationReadWriteAllId}}", "type": "Scope"}, + {"id": "{{AgentBlueprintReadWriteAllId}}", "type": "Scope"}, + {"id": "{{AgentBlueprintUpdateAuthId}}", "type": "Scope"}, + {"id": "{{AgentBlueprintAddRemoveCredsId}}", "type": "Scope"}, + {"id": "{{DelegatedPermissionGrantReadWriteAllId}}", "type": "Scope"}, + {"id": "{{DirectoryReadAllId}}", "type": "Scope"} + ] + } + ] + """; + + var appJson = $$""" + { + "value": [{ + "id": "{{AppObjId}}", + "appId": "{{ValidClientAppId}}", + "displayName": "Test App", + "requiredResourceAccess": {{requiredResourceAccess}} + }] + } + """; + + graphApiService.GraphGetWithResponseAsync( + Arg.Any(), + Arg.Is(p => p.Contains("displayName")), + Arg.Any(), + Arg.Any?>()) + .Returns(_ => Task.FromResult(new GraphApiService.GraphResponse + { + IsSuccess = true, + StatusCode = 200, + Json = JsonDocument.Parse(appJson) + })); + + var permJson = $$""" + { + "value": [{ + "id": "graph-sp-id-123", + "oauth2PermissionScopes": [ + {"id": "{{ApplicationReadWriteAllId}}", "value": "Application.ReadWrite.All"}, + {"id": "{{AgentBlueprintReadWriteAllId}}", "value": "AgentIdentityBlueprint.ReadWrite.All"}, + {"id": "{{AgentBlueprintUpdateAuthId}}", "value": "AgentIdentityBlueprint.UpdateAuthProperties.All"}, + {"id": "{{AgentBlueprintAddRemoveCredsId}}", "value": "AgentIdentityBlueprint.AddRemoveCreds.All"}, + {"id": "{{DelegatedPermissionGrantReadWriteAllId}}", "value": "DelegatedPermissionGrant.ReadWrite.All"}, + {"id": "{{DirectoryReadAllId}}", "value": "Directory.Read.All"} + ] + }] + } + """; + + graphApiService.GraphGetAsync( + Arg.Any(), + Arg.Is(p => p.Contains("oauth2PermissionScopes")), + Arg.Any(), + Arg.Any?>()) + .Returns(_ => Task.FromResult(JsonDocument.Parse(permJson))); + + return new ClientAppValidator(_logger, graphApiService, confirmationProvider); + } + + [Fact] + public async Task EnsureValidClientAppAsync_WhenUserDeclinesWithMissingPermissions_ThrowsImmediately() + { + var confirmationProvider = Substitute.For(); + confirmationProvider.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(false)); + + var executorLogger = Substitute.For>(); + var executor = Substitute.For(executorLogger); + var graphServiceLogger = Substitute.For>(); + var graphApiService = Substitute.For(graphServiceLogger, executor); + + // App exists but has no permissions → triggers missing permissions mutation + var appJson = $$""" + { + "value": [{ + "id": "{{AppObjId}}", + "appId": "{{ValidClientAppId}}", + "displayName": "Test App", + "requiredResourceAccess": [] + }] + } + """; + + graphApiService.GraphGetWithResponseAsync( + Arg.Any(), + Arg.Is(p => p.Contains("displayName")), + Arg.Any(), + Arg.Any?>()) + .Returns(_ => Task.FromResult(new GraphApiService.GraphResponse + { + IsSuccess = true, StatusCode = 200, Json = JsonDocument.Parse(appJson) + })); + + var permJson = $$""" + { + "value": [{ + "id": "graph-sp-id-123", + "oauth2PermissionScopes": [ + {"id": "{{ApplicationReadWriteAllId}}", "value": "Application.ReadWrite.All"}, + {"id": "{{DelegatedPermissionGrantReadWriteAllId}}", "value": "DelegatedPermissionGrant.ReadWrite.All"} + ] + }] + } + """; + + graphApiService.GraphGetAsync( + Arg.Any(), + Arg.Is(p => p.Contains("oauth2PermissionScopes")), + Arg.Any(), + Arg.Any?>()) + .Returns(_ => Task.FromResult(JsonDocument.Parse(permJson))); + + var validator = new ClientAppValidator(_logger, graphApiService, confirmationProvider); + + var exception = await Assert.ThrowsAsync( + () => validator.EnsureValidClientAppAsync(ValidClientAppId, ValidTenantId)); + + exception.IssueDescription.Should().Contain("declined"); + exception.ErrorDetails.Should().Contain(d => d.Contains("Missing permissions")); + + // Confirm no Graph mutations were attempted after the decline + await graphApiService.DidNotReceive().GraphPatchAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()); + } + + [Fact] + public async Task EnsureValidClientAppAsync_WhenUserDeclinesWithMissingRedirectUris_ThrowsImmediately() + { + var confirmationProvider = Substitute.For(); + confirmationProvider.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(false)); + + var validator = CreateValidatorWithConfirmation(confirmationProvider); + + // Return empty redirect URIs → triggers missing redirect URI mutation + var redirectUriJson = $$""" + { + "value": [{ + "id": "{{AppObjId}}", + "publicClient": { "redirectUris": [] } + }] + } + """; + + // Wire redirect URI GET on the underlying graphApiService — re-fetch it via reflection isn't clean; + // instead call EnsureRedirectUrisAsync indirectly via EnsureValidClientAppAsync. + // The CreateValidatorWithConfirmation graph mock returns null for unmatched calls, + // so set up the publicClient query on the shared _graphApiService substitute. + _graphApiService.GraphGetAsync( + Arg.Any(), + Arg.Is(p => p.Contains("publicClient") && !p.Contains("displayName")), + Arg.Any(), + Arg.Any?>()) + .Returns(_ => Task.FromResult(JsonDocument.Parse(redirectUriJson))); + + // Build a fresh validator wired to _graphApiService so the redirect URI mock is reachable + SetupAppInfoWithAllPermissions(ValidClientAppId); + SetupPermissionResolution(); + var validatorWithSharedGraph = new ClientAppValidator(_logger, _graphApiService, confirmationProvider); + + var exception = await Assert.ThrowsAsync( + () => validatorWithSharedGraph.EnsureValidClientAppAsync(ValidClientAppId, ValidTenantId)); + + exception.IssueDescription.Should().Contain("declined"); + exception.ErrorDetails.Should().Contain(d => d.Contains("redirect URI")); + + await _graphApiService.DidNotReceive().GraphPatchAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()); + } + + [Fact] + public async Task EnsureValidClientAppAsync_WhenUserDeclinesWithPublicClientDisabled_ThrowsImmediately() + { + var confirmationProvider = Substitute.For(); + confirmationProvider.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(false)); + + SetupAppInfoWithAllPermissions(ValidClientAppId); + SetupPermissionResolution(); + SetupPublicClientFlowsGet(enabled: false); + + var validator = new ClientAppValidator(_logger, _graphApiService, confirmationProvider); + + var exception = await Assert.ThrowsAsync( + () => validator.EnsureValidClientAppAsync(ValidClientAppId, ValidTenantId)); + + exception.IssueDescription.Should().Contain("declined"); + exception.ErrorDetails.Should().Contain(d => d.Contains("public client flows")); + + await _graphApiService.DidNotReceive().GraphPatchAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()); + } + + [Fact] + public async Task EnsureValidClientAppAsync_WhenUserDeclinesWithAllMutationsPending_ThrowsWithAllDetails() + { + var confirmationProvider = Substitute.For(); + confirmationProvider.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(false)); + + // App missing permissions + SetupAppInfoGet(ValidClientAppId, requiredResourceAccess: "[]"); + SetupPermissionResolution(); + SetupPublicClientFlowsGet(enabled: false); + + var redirectUriJson = $$""" + { + "value": [{ + "id": "{{AppObjId}}", + "publicClient": { "redirectUris": [] } + }] + } + """; + _graphApiService.GraphGetAsync( + Arg.Any(), + Arg.Is(p => p.Contains("publicClient") && !p.Contains("displayName")), + Arg.Any(), + Arg.Any?>()) + .Returns(_ => Task.FromResult(JsonDocument.Parse(redirectUriJson))); + + var validator = new ClientAppValidator(_logger, _graphApiService, confirmationProvider); + + var exception = await Assert.ThrowsAsync( + () => validator.EnsureValidClientAppAsync(ValidClientAppId, ValidTenantId)); + + exception.IssueDescription.Should().Contain("declined"); + exception.ErrorDetails.Should().Contain(d => d.Contains("Missing permissions")); + exception.ErrorDetails.Should().Contain(d => d.Contains("redirect URI")); + exception.ErrorDetails.Should().Contain(d => d.Contains("public client flows")); + } + + [Fact] + public async Task EnsureValidClientAppAsync_WhenUserAcceptsConfirmation_ProceedsWithFixes() + { + var confirmationProvider = Substitute.For(); + confirmationProvider.ConfirmAsync(Arg.Any()).Returns(Task.FromResult(true)); + + SetupAppInfoWithAllPermissions(ValidClientAppId); + SetupPermissionResolution(); + SetupPublicClientFlowsGet(enabled: false); + _graphApiService.GraphPatchAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(true)); + + var validator = new ClientAppValidator(_logger, _graphApiService, confirmationProvider); + + // Should not throw — fixes were accepted and applied + await validator.EnsureValidClientAppAsync(ValidClientAppId, ValidTenantId); + + await _graphApiService.Received(1).GraphPatchAsync( + Arg.Any(), + Arg.Is(p => p.Contains(AppObjId)), + Arg.Any(), + Arg.Any(), + Arg.Any?>()); + } + + [Fact] + public async Task EnsureValidClientAppAsync_WhenSkipConfirmationTrue_AppliesFixesWithoutPrompting() + { + SetupAppInfoWithAllPermissions(ValidClientAppId); + SetupPermissionResolution(); + SetupPublicClientFlowsGet(enabled: false); + _graphApiService.GraphPatchAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(true)); + + var confirmationProvider = Substitute.For(); + var validator = new ClientAppValidator(_logger, _graphApiService, confirmationProvider); + + await validator.EnsureValidClientAppAsync(ValidClientAppId, ValidTenantId, skipConfirmation: true); + + // Prompt must never be shown when skipConfirmation=true + await confirmationProvider.DidNotReceive().ConfirmAsync(Arg.Any()); + + // But fix must still be applied + await _graphApiService.Received(1).GraphPatchAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()); + } + + #endregion + #region EnsureRedirectUrisAsync Tests [Fact] From 979b3dd46c5251b7d04f3efaf963f02f3e730a61 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Fri, 27 Mar 2026 16:00:51 -0700 Subject: [PATCH 5/5] Fix silent merge conflict from MSAL.NET token refactor The MSAL.NET PR deleted AzCliHelper.InvalidateAzCliTokenCache() from ClientAppValidator, but our branch had modified the same line to use the fully-qualified name. Git's ort strategy silently kept our version instead of applying the deletion, leaving a call to a now-removed method. Remove the stale call and update GraphGetWithResponseAsync mock signatures in tests to match the new forceRefresh parameter added in the same PR. Co-Authored-By: Claude Sonnet 4.6 --- .../Services/ClientAppValidator.cs | 3 --- .../Services/ClientAppValidatorTests.cs | 10 ++++++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs index d8348ed1..f35890a7 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs @@ -593,9 +593,6 @@ private async Task TryExtendConsentGrantScopesAsync( { _logger.LogInformation("Extending admin consent grant with {Count} new permission(s): {Scopes}.", scopesToAdd.Count, string.Join(", ", scopesToAdd)); - // Invalidate the process-level az CLI token cache so the next Graph call - // re-acquires a token that includes the newly consented scope(s). - Services.Helpers.AzCliHelper.InvalidateAzCliTokenCache(); } else { diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs index 408037b6..98f415b0 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs @@ -361,8 +361,9 @@ private ClientAppValidator CreateValidatorWithConfirmation(IConfirmationProvider graphApiService.GraphGetWithResponseAsync( Arg.Any(), Arg.Is(p => p.Contains("displayName")), - Arg.Any(), - Arg.Any?>()) + Arg.Any(), + Arg.Any?>(), + Arg.Any()) .Returns(_ => Task.FromResult(new GraphApiService.GraphResponse { IsSuccess = true, @@ -422,8 +423,9 @@ public async Task EnsureValidClientAppAsync_WhenUserDeclinesWithMissingPermissio graphApiService.GraphGetWithResponseAsync( Arg.Any(), Arg.Is(p => p.Contains("displayName")), - Arg.Any(), - Arg.Any?>()) + Arg.Any(), + Arg.Any?>(), + Arg.Any()) .Returns(_ => Task.FromResult(new GraphApiService.GraphResponse { IsSuccess = true, StatusCode = 200, Json = JsonDocument.Parse(appJson)