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 adb8f062..37d8f00f 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."); @@ -1061,12 +1061,17 @@ 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 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, + scopes: AuthenticationConstants.RequiredPermissionGrantScopes); + return checkDoc != null; }, result => !result, maxRetries: 12, 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 c37bcfc2..f35890a7 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,60 @@ 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."); + + 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); + } + } + // 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 +182,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 +196,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 +362,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 +591,8 @@ 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)); } else { @@ -540,6 +608,105 @@ 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) + { + // 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(); + } + } + + /// + /// 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) + { + // 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; + } + } + #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..191900fa 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheck.cs @@ -46,9 +46,20 @@ 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("[FAIL] {Name}", Name); - logger.LogError(" Issue: {ErrorMessage}", errorMessage); - logger.LogError(" Resolution: {ResolutionGuidance}", resolutionGuidance); + + // 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/ClientAppValidatorTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs index cb8bacf6..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 @@ -321,6 +321,308 @@ 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?>(), + 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?>(), + 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] 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..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,7 +52,7 @@ 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 — [WARN] prefix is included in the message (logged at Warning severity → yellow in formatter) _mockLogger.Received().Log( LogLevel.Warning, Arg.Any(),