Skip to content

Commit 8b62981

Browse files
committed
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
1 parent d93e060 commit 8b62981

13 files changed

Lines changed: 255 additions & 34 deletions

File tree

src/Microsoft.Agents.A365.DevTools.Cli/Commands/ConfigCommand.cs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,15 +35,18 @@ private static Command CreateInitSubcommand(ILogger logger, string configDir, IC
3535
{
3636
new Option<string?>(new[] { "-c", "--configfile" }, "Path to an existing config file to import"),
3737
new Option<bool>(new[] { "--global", "-g" }, "Create config in global directory (AppData) instead of current directory"),
38+
new Option<bool>(new[] { "--yes", "-y" }, "Skip confirmation prompts and apply any required app registration changes automatically"),
3839
};
3940

4041
cmd.SetHandler(async (System.CommandLine.Invocation.InvocationContext context) =>
4142
{
4243
var configFileOption = cmd.Options.OfType<Option<string?>>().First(opt => opt.HasAlias("-c"));
4344
var globalOption = cmd.Options.OfType<Option<bool>>().First(opt => opt.HasAlias("--global"));
45+
var yesOption = cmd.Options.OfType<Option<bool>>().First(opt => opt.HasAlias("--yes"));
4446

4547
string? configFile = context.ParseResult.GetValueForOption(configFileOption);
4648
bool useGlobal = context.ParseResult.GetValueForOption(globalOption);
49+
bool yes = context.ParseResult.GetValueForOption(yesOption);
4750

4851
// Determine config path
4952
string configPath = useGlobal
@@ -95,7 +98,8 @@ private static Command CreateInitSubcommand(ILogger logger, string configDir, IC
9598
await clientAppValidator.EnsureValidClientAppAsync(
9699
importedConfig.ClientAppId,
97100
importedConfig.TenantId,
98-
context.GetCancellationToken());
101+
skipConfirmation: yes,
102+
ct: context.GetCancellationToken());
99103
}
100104
catch (ClientAppValidationException ex)
101105
{
@@ -108,9 +112,10 @@ await clientAppValidator.EnsureValidClientAppAsync(
108112
}
109113
if (ex.MitigationSteps.Count > 0)
110114
{
115+
logger.LogInformation("");
111116
foreach (var step in ex.MitigationSteps)
112117
{
113-
logger.LogError(step);
118+
logger.LogInformation(" {Step}", step.TrimEnd());
114119
}
115120
}
116121
logger.LogError("");

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ public static Command CreateCommand(
137137
await RequirementsSubcommand.RunChecksOrExitAsync(
138138
checks, setupConfig, logger, ct);
139139
}
140-
catch (Exception reqEx) when (reqEx is not OperationCanceledException)
140+
catch (Exception reqEx) when (reqEx is not OperationCanceledException && reqEx is not CleanExitException)
141141
{
142142
logger.LogError(reqEx, "Requirements check failed: {Message}", reqEx.Message);
143143
logger.LogError("Rerun with --skip-requirements to bypass.");

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ public static Command CreateCommand(
168168
await RequirementsSubcommand.RunChecksOrExitAsync(
169169
checks, setupConfig, logger, ct);
170170
}
171-
catch (Exception reqEx) when (reqEx is not OperationCanceledException)
171+
catch (Exception reqEx) when (reqEx is not OperationCanceledException && reqEx is not CleanExitException)
172172
{
173173
logger.LogError(reqEx, "Requirements check failed with an unexpected error: {Message}", reqEx.Message);
174174
logger.LogError("If you want to bypass requirement validation, rerun this command with the --skip-requirements flag.");

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ public static Command CreateCommand(
226226
await RequirementsSubcommand.RunChecksOrExitAsync(
227227
checks, setupConfig, logger, ct);
228228
}
229-
catch (Exception reqEx) when (reqEx is not OperationCanceledException)
229+
catch (Exception reqEx) when (reqEx is not OperationCanceledException && reqEx is not CleanExitException)
230230
{
231231
logger.LogError(reqEx, "Requirements check failed with an unexpected error: {Message}", reqEx.Message);
232232
logger.LogError("If you want to bypass requirement validation, rerun this command with the --skip-requirements flag.");

src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,16 @@ public static class AuthenticationConstants
3030
public const string LocalhostRedirectUri = "http://localhost:8400/";
3131

3232
/// <summary>
33-
/// Required redirect URIs for Microsoft Graph PowerShell SDK authentication.
34-
/// The SDK requires both http://localhost and http://localhost:8400/ for different auth flows.
33+
/// Required redirect URIs for authentication.
34+
/// <list type="bullet">
35+
/// <item><term>http://localhost</term><description>Required by the Microsoft Graph PowerShell SDK
36+
/// (<c>Connect-MgGraph -ClientId</c>). Without this URI, PowerShell-based operations (OAuth2 grants,
37+
/// service principal lookups) fall back to the Azure CLI token, which lacks required delegated
38+
/// permissions and causes 403 errors on inheritable permissions operations.</description></item>
39+
/// <item><term>http://localhost:8400/</term><description>Required by MSAL for interactive browser
40+
/// authentication. Uses a fixed port to ensure consistent OAuth callbacks.</description></item>
41+
/// </list>
42+
/// See also <see cref="WamBrokerRedirectUriFormat"/> for the Windows WAM broker URI.
3543
/// </summary>
3644
public static readonly string[] RequiredRedirectUris = new[]
3745
{

src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs

Lines changed: 162 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,13 @@ public sealed class ClientAppValidator : IClientAppValidator
1919
{
2020
private readonly ILogger<ClientAppValidator> _logger;
2121
private readonly GraphApiService _graphApiService;
22+
private readonly IConfirmationProvider? _confirmationProvider;
2223

23-
public ClientAppValidator(ILogger<ClientAppValidator> logger, GraphApiService graphApiService)
24+
public ClientAppValidator(ILogger<ClientAppValidator> logger, GraphApiService graphApiService, IConfirmationProvider? confirmationProvider = null)
2425
{
2526
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
2627
_graphApiService = graphApiService ?? throw new ArgumentNullException(nameof(graphApiService));
28+
_confirmationProvider = confirmationProvider;
2729
}
2830

2931
/// <summary>
@@ -33,11 +35,14 @@ public ClientAppValidator(ILogger<ClientAppValidator> logger, GraphApiService gr
3335
/// </summary>
3436
/// <param name="clientAppId">The client app ID to validate</param>
3537
/// <param name="tenantId">The tenant ID where the app should exist</param>
38+
/// <param name="skipConfirmation">When true, applies any required app registration fixes without prompting the user.
39+
/// Use for non-interactive or CI scenarios. Defaults to false (prompt before modifying the app registration).</param>
3640
/// <param name="ct">Cancellation token</param>
3741
/// <exception cref="ClientAppValidationException">Thrown when validation fails</exception>
3842
public async Task EnsureValidClientAppAsync(
3943
string clientAppId,
4044
string tenantId,
45+
bool skipConfirmation = false,
4146
CancellationToken ct = default)
4247
{
4348
ArgumentException.ThrowIfNullOrWhiteSpace(clientAppId);
@@ -71,7 +76,7 @@ public async Task EnsureValidClientAppAsync(
7176

7277
_logger.LogDebug("Found client app: {DisplayName} ({AppId})", appInfo.DisplayName, clientAppId);
7378

74-
// Step 3: Validate permissions in manifest
79+
// Step 3: Validate permissions in manifest (read-only)
7580
var missingPermissions = await ValidatePermissionsConfiguredAsync(appInfo, tenantId, ct);
7681

7782
// Step 3.5: For any unresolvable permissions (beta APIs), check oauth2PermissionGrants as fallback
@@ -87,8 +92,48 @@ public async Task EnsureValidClientAppAsync(
8792
}
8893
}
8994

95+
// Read-only pre-flight: collect what redirect URIs and public client settings need fixing
96+
var missingRedirectUris = await CollectMissingRedirectUrisAsync(clientAppId, tenantId, ct);
97+
var publicClientNeedsEnabling = await IsPublicClientFlowsDisabledAsync(clientAppId, tenantId, ct);
98+
99+
// Determine what mutations are needed
100+
bool hasMissingPermissions = missingPermissions.Count > 0;
101+
bool hasMissingRedirectUris = missingRedirectUris.Count > 0;
102+
bool needsPublicClientEnabled = publicClientNeedsEnabling;
103+
bool hasPendingMutations = hasMissingPermissions || hasMissingRedirectUris || needsPublicClientEnabled;
104+
105+
// Prompt the user before making any changes (unless skipConfirmation or no confirmation provider)
106+
bool applyFixes = true;
107+
if (hasPendingMutations && _confirmationProvider != null && !skipConfirmation)
108+
{
109+
_logger.LogInformation("The following changes will be applied to app registration ({AppId}):", clientAppId);
110+
_logger.LogInformation("");
111+
if (hasMissingPermissions)
112+
{
113+
_logger.LogInformation(" - Add permissions and grant admin consent:");
114+
foreach (var perm in missingPermissions)
115+
_logger.LogInformation(" {Permission}", perm);
116+
}
117+
if (hasMissingRedirectUris)
118+
{
119+
_logger.LogInformation(" - Add redirect URIs:");
120+
foreach (var uri in missingRedirectUris)
121+
_logger.LogInformation(" {Uri}", uri);
122+
}
123+
if (needsPublicClientEnabled)
124+
_logger.LogInformation(" - Enable 'Allow public client flows' (required for device code fallback)");
125+
_logger.LogInformation("For more information: https://learn.microsoft.com/en-us/microsoft-agent-365/developer/custom-client-app-registration");
126+
_logger.LogInformation("");
127+
128+
applyFixes = await _confirmationProvider.ConfirmAsync("Do you want to proceed? (y/N): ");
129+
if (!applyFixes)
130+
{
131+
_logger.LogInformation("App registration was not modified. Re-run and accept the prompt, or configure manually.");
132+
}
133+
}
134+
90135
// Step 3.6: Auto-provision any remaining missing permissions (self-healing)
91-
if (missingPermissions.Count > 0)
136+
if (applyFixes && missingPermissions.Count > 0)
92137
{
93138
_logger.LogInformation("Auto-provisioning {Count} missing permission(s): {Permissions}",
94139
missingPermissions.Count, string.Join(", ", missingPermissions));
@@ -125,10 +170,12 @@ public async Task EnsureValidClientAppAsync(
125170
}
126171

127172
// Step 5: Verify and fix redirect URIs
128-
await EnsureRedirectUrisAsync(clientAppId, tenantId, ct);
173+
if (applyFixes)
174+
await EnsureRedirectUrisAsync(clientAppId, tenantId, ct);
129175

130-
// Step 6: Verify and fix public client flows (required for device code fallback on non-Windows)
131-
await EnsurePublicClientFlowsEnabledAsync(clientAppId, tenantId, ct);
176+
// Step 6: Verify and fix public client flows (required for device code fallback)
177+
if (applyFixes)
178+
await EnsurePublicClientFlowsEnabledAsync(clientAppId, tenantId, ct);
132179

133180
_logger.LogDebug("Client app validation successful for {ClientAppId}", clientAppId);
134181
}
@@ -137,6 +184,11 @@ public async Task EnsureValidClientAppAsync(
137184
// Re-throw validation exceptions as-is
138185
throw;
139186
}
187+
catch (OperationCanceledException)
188+
{
189+
// Ctrl+C / cancellation — propagate immediately without wrapping
190+
throw;
191+
}
140192
catch (JsonException ex)
141193
{
142194
_logger.LogDebug(ex, "JSON parsing error during validation");
@@ -298,7 +350,10 @@ private async Task EnsurePublicClientFlowsEnabledAsync(
298350
return;
299351
}
300352

301-
_logger.LogInformation("Enabling 'Allow public client flows' on app registration (required for device code authentication fallback).");
353+
_logger.LogInformation(
354+
"Enabling 'Allow public client flows' on app registration " +
355+
"(required for device code authentication fallback on macOS, Linux, WSL, " +
356+
"headless environments, and as a Conditional Access Policy fallback on Windows).");
302357
_logger.LogInformation("Run 'a365 setup requirements' at any time to re-verify and auto-fix this setting.");
303358

304359
var patchSuccess = await _graphApiService.GraphPatchAsync(tenantId,
@@ -524,7 +579,11 @@ private async Task TryExtendConsentGrantScopesAsync(
524579

525580
if (patchSuccess)
526581
{
527-
_logger.LogInformation("Extended consent grant with scope(s): {Scopes}", string.Join(", ", scopesToAdd));
582+
_logger.LogInformation("Extending admin consent grant with {Count} new permission(s): {Scopes}.",
583+
scopesToAdd.Count, string.Join(", ", scopesToAdd));
584+
// Invalidate the process-level az CLI token cache so the next Graph call
585+
// re-acquires a token that includes the newly consented scope(s).
586+
Services.Helpers.AzCliHelper.InvalidateAzCliTokenCache();
528587
}
529588
else
530589
{
@@ -540,6 +599,101 @@ private async Task TryExtendConsentGrantScopesAsync(
540599
}
541600
}
542601

602+
/// <summary>
603+
/// Returns the subset of <see cref="AuthenticationConstants.RequiredClientAppPermissions"/>
604+
/// that are not yet present in the client app's oauth2PermissionGrant (i.e. not consented).
605+
/// </summary>
606+
public async Task<List<string>> GetUnconsentedRequiredPermissionsAsync(
607+
string clientAppId,
608+
string tenantId,
609+
CancellationToken ct = default)
610+
{
611+
var consented = await GetConsentedPermissionsAsync(clientAppId, tenantId, ct);
612+
return AuthenticationConstants.RequiredClientAppPermissions
613+
.Where(p => !consented.Contains(p, StringComparer.OrdinalIgnoreCase))
614+
.ToList();
615+
}
616+
617+
/// <summary>
618+
/// Extends the client app's oauth2PermissionGrant to include the specified permissions.
619+
/// Call after the user has confirmed they want to grant admin consent.
620+
/// </summary>
621+
public Task GrantConsentForPermissionsAsync(
622+
string clientAppId,
623+
List<string> permissions,
624+
string tenantId,
625+
CancellationToken ct = default)
626+
=> TryExtendConsentGrantScopesAsync(clientAppId, permissions, tenantId, ct);
627+
628+
629+
/// <summary>
630+
/// Read-only check: returns the redirect URIs that are missing from the app registration
631+
/// without making any changes. Used to build the pre-flight mutation summary.
632+
/// </summary>
633+
private async Task<List<string>> CollectMissingRedirectUrisAsync(
634+
string clientAppId,
635+
string tenantId,
636+
CancellationToken ct)
637+
{
638+
try
639+
{
640+
using var appDoc = await _graphApiService.GraphGetAsync(tenantId,
641+
$"/v1.0/applications?$filter=appId eq '{clientAppId}'&$select=id,publicClient", ct);
642+
643+
if (appDoc == null) return new List<string>();
644+
645+
var response = JsonNode.Parse(appDoc.RootElement.GetRawText());
646+
var apps = response?["value"]?.AsArray();
647+
if (apps == null || apps.Count == 0) return new List<string>();
648+
649+
var publicClient = apps[0]!.AsObject()["publicClient"]?.AsObject();
650+
var currentRedirectUris = publicClient?["redirectUris"]?.AsArray()
651+
?.Select(uri => uri?.GetValue<string>())
652+
.Where(uri => !string.IsNullOrWhiteSpace(uri))
653+
.Select(uri => uri!)
654+
.ToHashSet(StringComparer.OrdinalIgnoreCase) ?? new HashSet<string>(StringComparer.OrdinalIgnoreCase);
655+
656+
return AuthenticationConstants.GetRequiredRedirectUris(clientAppId)
657+
.Where(uri => !currentRedirectUris.Contains(uri))
658+
.ToList();
659+
}
660+
catch (Exception ex)
661+
{
662+
_logger.LogDebug("CollectMissingRedirectUrisAsync failed (non-fatal): {Message}", ex.Message);
663+
return new List<string>();
664+
}
665+
}
666+
667+
/// <summary>
668+
/// Read-only check: returns true if 'Allow public client flows' (isFallbackPublicClient)
669+
/// is currently disabled on the app registration, without making any changes.
670+
/// </summary>
671+
private async Task<bool> IsPublicClientFlowsDisabledAsync(
672+
string clientAppId,
673+
string tenantId,
674+
CancellationToken ct)
675+
{
676+
try
677+
{
678+
using var appDoc = await _graphApiService.GraphGetAsync(tenantId,
679+
$"/v1.0/applications?$filter=appId eq '{clientAppId}'&$select=id,isFallbackPublicClient", ct);
680+
681+
if (appDoc == null) return false;
682+
683+
var response = JsonNode.Parse(appDoc.RootElement.GetRawText());
684+
var apps = response?["value"]?.AsArray();
685+
if (apps == null || apps.Count == 0) return false;
686+
687+
var isFallbackPublicClient = apps[0]!.AsObject()["isFallbackPublicClient"]?.GetValue<bool>() ?? false;
688+
return !isFallbackPublicClient;
689+
}
690+
catch (Exception ex)
691+
{
692+
_logger.LogDebug("IsPublicClientFlowsDisabledAsync failed (non-fatal): {Message}", ex.Message);
693+
return false;
694+
}
695+
}
696+
543697
#region Private Helper Methods
544698

545699
private async Task<ClientAppInfo?> GetClientAppInfoAsync(string clientAppId, string tenantId, CancellationToken ct)

src/Microsoft.Agents.A365.DevTools.Cli/Services/ConfigurationWizardService.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -988,7 +988,7 @@ private string GetUsageLocationFromAccount(AzureAccountInfo accountInfo)
988988

989989
try
990990
{
991-
await validator.EnsureValidClientAppAsync(clientAppId, tenantId, CancellationToken.None);
991+
await validator.EnsureValidClientAppAsync(clientAppId, tenantId, ct: CancellationToken.None);
992992
Console.WriteLine("Client app validation successful!");
993993
Console.WriteLine();
994994
return clientAppId;

src/Microsoft.Agents.A365.DevTools.Cli/Services/ConsoleConfirmationProvider.cs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,18 @@ public class ConsoleConfirmationProvider : IConfirmationProvider
1616
public Task<bool> ConfirmAsync(string prompt)
1717
{
1818
Console.Write(prompt);
19-
var response = Console.ReadLine()?.Trim().ToLowerInvariant();
20-
return Task.FromResult(response == "y" || response == "yes");
19+
var response = Console.ReadLine();
20+
21+
// null means stdin was closed (Ctrl+C / EOF) — exit immediately without further output,
22+
// matching az CLI behavior where Ctrl+C terminates cleanly with no trailing messages.
23+
if (response == null)
24+
{
25+
Console.WriteLine();
26+
throw new OperationCanceledException();
27+
}
28+
29+
return Task.FromResult(response.Trim().Equals("y", StringComparison.OrdinalIgnoreCase)
30+
|| response.Trim().Equals("yes", StringComparison.OrdinalIgnoreCase));
2131
}
2232

2333
/// <summary>

0 commit comments

Comments
 (0)