From 116edb1515c439759275ecbbb3a7663bd1689d30 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Thu, 26 Mar 2026 18:00:13 -0700 Subject: [PATCH 1/3] fix: replace az CLI token acquisition with MSAL.NET to eliminate proxy resets (#321) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Users on corporate networks with TLS inspection proxies intermittently saw ConnectionResetError (10054) during 'a365 setup all'. The failure was non-deterministic — different steps each run, restarting sometimes helped. Root cause: all token acquisition called 'az account get-access-token' as a subprocess. The Azure CLI is Python-based and uses an HTTP connection pool that the TLS proxy would reset mid-execution during longer commands. Fix: replaced all subprocess token acquisition with direct MSAL.NET calls via AuthenticationService. On Windows, MSAL uses WAM — an OS-level broker that communicates over IPC, bypassing the proxy entirely. On macOS/Linux, MSAL uses browser or device code flow. Changes: - GraphApiService and ArmApiService now acquire tokens via IAuthenticationService (MSAL) instead of spawning 'az account get-access-token' subprocesses - AzCliHelper retains only login-hint resolution (az account show); token acquisition methods removed - BotConfigurator reads tenantId from config instead of 'az account show' - NetworkHelper centralises connection-reset detection and warning message - RetryHelper accepts constructor-level defaults; injectable in tests to skip backoff delays - AuthenticationService gains ResolveLoginHintFromCacheAsync as fallback when az CLI is not available - Added tests for ResolveLoginHintFromCacheAsync and NetworkHelper --- CHANGELOG.md | 1 + .../SetupSubcommands/BlueprintSubcommand.cs | 1 + .../InfrastructureSubcommand.cs | 40 +- .../Program.cs | 1 + .../Services/ArmApiService.cs | 49 ++- .../Services/AuthenticationService.cs | 70 +++- .../Services/BotConfigurator.cs | 58 +-- .../Services/ClientAppValidator.cs | 3 +- .../Services/DelegatedConsentService.cs | 6 +- .../Services/GraphApiService.cs | 365 ++++++++---------- .../Services/Helpers/AzCliHelper.cs | 120 +----- .../Services/Helpers/NetworkHelper.cs | 34 ++ .../Services/Helpers/RetryHelper.cs | 74 ++-- .../design.md | 57 ++- .../Commands/BlueprintSubcommandTests.cs | 6 +- .../CleanupCommandBotEndpointTests.cs | 2 +- .../Commands/CleanupCommandTests.cs | 2 +- .../Services/AgentBlueprintServiceTests.cs | 20 +- .../Services/ArmApiServiceTests.cs | 16 +- .../Services/AuthenticationServiceTests.cs | 77 ++++ .../Services/ClientAppValidatorTests.cs | 1 + ...piServiceAddRequiredResourceAccessTests.cs | 37 +- .../GraphApiServiceIsApplicationOwnerTests.cs | 42 +- .../Services/GraphApiServiceTests.cs | 83 ++-- .../GraphApiServiceTokenCacheTests.cs | 219 ----------- .../Services/GraphApiServiceTokenTrimTests.cs | 19 +- ...erviceVerifyInheritablePermissionsTests.cs | 16 +- .../Services/Helpers/AzCliHelperTests.cs | 96 +---- .../Services/Helpers/NetworkHelperTests.cs | 60 +++ 29 files changed, 672 insertions(+), 903 deletions(-) create mode 100644 src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/NetworkHelper.cs delete mode 100644 src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTokenCacheTests.cs create mode 100644 src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/NetworkHelperTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index a791e03a..8028f0e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `a365 publish` updates manifest IDs, creates `manifest.zip`, and prints concise upload instructions for Microsoft 365 Admin Center (Agents > All agents > Upload custom agent). Interactive prompts only occur in interactive terminals; redirect stdin to suppress them in scripts. ### Fixed +- Intermittent `ConnectionResetError (10054)` failures on corporate networks with TLS inspection proxies (Zscaler, Netskope) — Graph and ARM API calls now use direct MSAL.NET token acquisition instead of `az account get-access-token` subprocesses, bypassing the Python HTTP stack that triggered proxy resets (#321) - `a365 cleanup` blueprint deletion now succeeds for Global Administrators even when the blueprint was created by a different user - `a365 setup all` no longer times out for non-admin users — the CLI immediately surfaces a consent URL to share with an administrator instead of waiting for a browser prompt - `a365 setup all` requests admin consent once for all resources instead of prompting once per resource 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..262c6d5a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs @@ -386,6 +386,7 @@ public static async Task CreateBlueprintImplementationA new GraphApiService( cleanLoggerFactory.CreateLogger(), executor, + new AuthenticationService(cleanLoggerFactory.CreateLogger()), graphBaseUrl: setupConfig.GraphBaseUrl)); // Use DI-provided GraphApiService which already has MicrosoftGraphTokenProvider configured diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/InfrastructureSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/InfrastructureSubcommand.cs index 73f18fbc..75e5baf9 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/InfrastructureSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/InfrastructureSubcommand.cs @@ -300,42 +300,10 @@ public static async Task ValidateAzureCliAuthenticationAsync( logger.LogDebug("Azure CLI already authenticated as {LoginHint}", loginHint); } - // Verify we have the management scope (token is cached at process level by AzCliHelper). - logger.LogDebug("Verifying access to Azure management resources..."); - var managementToken = await AzCliHelper.AcquireAzCliTokenAsync(ArmApiService.ArmResource, tenantId); - - if (string.IsNullOrWhiteSpace(managementToken)) - { - logger.LogWarning("Unable to acquire management scope token. Attempting re-authentication..."); - logger.LogInformation("A browser window will open for authentication."); - - var loginResult = await executor.ExecuteAsync("az", $"login --tenant {tenantId}", cancellationToken: cancellationToken); - - if (!loginResult.Success) - { - logger.LogError("Azure CLI login with management scope failed. Please run manually: az login --scope https://management.core.windows.net//.default"); - return false; - } - - logger.LogInformation("Azure CLI re-authentication successful!"); - AzCliHelper.InvalidateAzCliTokenCache(); - await Task.Delay(2000, cancellationToken); - - var retryToken = await AzCliHelper.AcquireAzCliTokenAsync(ArmApiService.ArmResource, tenantId); - if (string.IsNullOrWhiteSpace(retryToken)) - { - logger.LogWarning("Still unable to acquire management scope token after re-authentication."); - logger.LogWarning("Continuing anyway - you may encounter permission errors later."); - } - else - { - logger.LogDebug("Management scope token acquired successfully!"); - } - } - else - { - logger.LogDebug("Management scope verified successfully"); - } + // ARM token acquisition is handled lazily by ArmApiService via MSAL (WAM/browser/device-code). + // No eager token pre-fetch is needed here — MSAL will prompt for sign-in when the + // first ARM call is made if no valid cached token exists. + logger.LogDebug("Azure authentication verified. ARM token will be acquired on first resource call."); return true; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs index 09d9c276..7878f4ea 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Program.cs @@ -254,6 +254,7 @@ private static void ConfigureServices(IServiceCollection services, LogLevel mini services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); + services.AddSingleton(sp => sp.GetRequiredService()); services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs index 2fec7fc4..c60430b3 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs @@ -15,8 +15,8 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Services; /// Service for Azure Resource Manager (ARM) existence checks via direct HTTP. /// Replaces subprocess-based 'az group exists', 'az appservice plan show', and /// 'az webapp show' calls — each drops from ~15-20s to ~0.5s. -/// Token acquisition is handled by AzCliHelper (process-level cache shared with -/// other services using the management endpoint). +/// Token acquisition uses MSAL via AuthenticationService (WAM on Windows, +/// browser/device-code on macOS/Linux) — no az CLI subprocess involved. /// public class ArmApiService : IDisposable { @@ -27,17 +27,21 @@ public class ArmApiService : IDisposable private readonly ILogger _logger; private readonly HttpClient _httpClient; + private readonly IAuthenticationService _authService; + private readonly RetryHelper _retryHelper; - // Allow injecting a custom HttpMessageHandler for unit testing. - public ArmApiService(ILogger logger, HttpMessageHandler? handler = null) + // Allow injecting a custom HttpMessageHandler and RetryHelper for unit testing. + public ArmApiService(ILogger logger, IAuthenticationService authService, HttpMessageHandler? handler = null, RetryHelper? retryHelper = null) { _logger = logger; + _authService = authService; _httpClient = handler != null ? new HttpClient(handler) : HttpClientFactory.CreateAuthenticatedClient(); + _retryHelper = retryHelper ?? new RetryHelper(_logger); } // Parameterless constructor to ease test mocking/substitution frameworks. public ArmApiService() - : this(NullLogger.Instance, null) + : this(NullLogger.Instance, new AuthenticationService(NullLogger.Instance), null) { } @@ -45,7 +49,7 @@ public ArmApiService() private async Task EnsureArmHeadersAsync(string tenantId, CancellationToken ct) { - var token = await AzCliHelper.AcquireAzCliTokenAsync(ArmResource, tenantId); + var token = await _authService.GetAccessTokenAsync(ArmResource, tenantId); if (string.IsNullOrWhiteSpace(token)) { _logger.LogWarning("Unable to acquire ARM access token for tenant {TenantId}", tenantId); @@ -74,7 +78,8 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke try { - using var response = await _httpClient.GetAsync(url, ct); + using var response = await _retryHelper.ExecuteWithRetryAsync( + ct => _httpClient.GetAsync(url, ct), cancellationToken: ct); _logger.LogDebug("ARM resource group check: {StatusCode}", response.StatusCode); if (response.StatusCode == HttpStatusCode.OK) return true; if (response.StatusCode == HttpStatusCode.NotFound) return false; @@ -82,7 +87,10 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke } catch (Exception ex) { - _logger.LogDebug(ex, "ARM resource group check failed — will fall back to az CLI"); + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "ARM resource group check failed — will fall back to az CLI"); return null; } } @@ -106,7 +114,8 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke try { - using var response = await _httpClient.GetAsync(url, ct); + using var response = await _retryHelper.ExecuteWithRetryAsync( + ct => _httpClient.GetAsync(url, ct), cancellationToken: ct); _logger.LogDebug("ARM app service plan check: {StatusCode}", response.StatusCode); if (response.StatusCode == HttpStatusCode.OK) return true; if (response.StatusCode == HttpStatusCode.NotFound) return false; @@ -114,7 +123,10 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke } catch (Exception ex) { - _logger.LogDebug(ex, "ARM app service plan check failed — will fall back to az CLI"); + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "ARM app service plan check failed — will fall back to az CLI"); return null; } } @@ -138,7 +150,8 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke try { - using var response = await _httpClient.GetAsync(url, ct); + using var response = await _retryHelper.ExecuteWithRetryAsync( + ct => _httpClient.GetAsync(url, ct), cancellationToken: ct); _logger.LogDebug("ARM web app check: {StatusCode}", response.StatusCode); if (response.StatusCode == HttpStatusCode.OK) return true; if (response.StatusCode == HttpStatusCode.NotFound) return false; @@ -146,7 +159,10 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke } catch (Exception ex) { - _logger.LogDebug(ex, "ARM web app check failed — will fall back to az CLI"); + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "ARM web app check failed — will fall back to az CLI"); return null; } } @@ -186,7 +202,8 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke try { - using var response = await _httpClient.GetAsync(url, ct); + using var response = await _retryHelper.ExecuteWithRetryAsync( + ct => _httpClient.GetAsync(url, ct), cancellationToken: ct); if (!response.IsSuccessStatusCode) { _logger.LogDebug("ARM role assignment check returned {StatusCode}", response.StatusCode); @@ -218,8 +235,12 @@ private async Task EnsureArmHeadersAsync(string tenantId, CancellationToke } catch (Exception ex) { - _logger.LogDebug(ex, "ARM role assignment check failed — will fall back to az CLI"); + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "ARM role assignment check failed — will fall back to az CLI"); return null; } } + } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs index edb47613..5e6b74b0 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs @@ -12,6 +12,25 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Services; +/// +/// Abstraction over MSAL token acquisition used by ArmApiService and GraphApiService. +/// Enables test substitution without triggering real interactive authentication. +/// Co-located with AuthenticationService per the related-interfaces convention. +/// +public interface IAuthenticationService +{ + Task GetAccessTokenAsync( + string resourceUrl, + string? tenantId = null, + bool forceRefresh = false, + string? clientId = null, + IEnumerable? scopes = null, + bool useInteractiveBrowser = true, + string? userId = null); + + Task ResolveLoginHintFromCacheAsync(); +} + /// /// Service for handling authentication to Agent 365 Tools and Microsoft Graph API. /// @@ -37,7 +56,7 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Services; /// - Subsequent commands: 0 prompts (uses cached tokens) /// - Token refresh: Automatic when within 5 minutes of expiration /// -public class AuthenticationService +public class AuthenticationService : IAuthenticationService { private readonly ILogger _logger; private readonly string _tokenCachePath; @@ -559,6 +578,55 @@ protected virtual TokenCredential CreateDeviceCodeCredential(string clientId, st }); } + /// + /// Resolves the login hint (UPN) from the persistent token cache by decoding a cached JWT. + /// Used to pre-select the correct account for WAM/MSAL when az CLI is not available. + /// Returns null if no cached token exists or the UPN claim cannot be read. + /// + public async Task ResolveLoginHintFromCacheAsync() + { + if (!File.Exists(_tokenCachePath)) + return null; + try + { + var json = await File.ReadAllTextAsync(_tokenCachePath); + var cache = JsonSerializer.Deserialize(json); + if (cache?.Tokens == null) return null; + foreach (var entry in cache.Tokens.Values) + { + var upn = TryExtractUpnFromJwt(entry.AccessToken); + if (!string.IsNullOrWhiteSpace(upn)) + return upn; + } + } + catch (Exception ex) + { + _logger.LogDebug(ex, "Failed to resolve login hint from token cache"); + } + return null; + } + + private static string? TryExtractUpnFromJwt(string? jwt) + { + if (string.IsNullOrWhiteSpace(jwt)) return null; + try + { + var parts = jwt.Split('.'); + if (parts.Length < 2) return null; + var payload = parts[1]; + // Restore Base64 padding stripped by JWT encoding. + payload = payload.PadRight(payload.Length + (4 - payload.Length % 4) % 4, '='); + var bytes = Convert.FromBase64String(payload); + using var doc = JsonDocument.Parse(bytes); + if (doc.RootElement.TryGetProperty("upn", out var upn) && !string.IsNullOrWhiteSpace(upn.GetString())) + return upn.GetString(); + if (doc.RootElement.TryGetProperty("preferred_username", out var pref) && !string.IsNullOrWhiteSpace(pref.GetString())) + return pref.GetString(); + } + catch { } // Static helper — no logger access. Caller logs via ResolveLoginHintFromCacheAsync. + return null; + } + /// /// Clears cached authentication token(s) /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/BotConfigurator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/BotConfigurator.cs index d389c333..6ebbd5d9 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/BotConfigurator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/BotConfigurator.cs @@ -61,26 +61,9 @@ public async Task CreateEndpointWithAgentBlueprintAs try { - // Get subscription info for tenant ID - var subscriptionResult = await _executor.ExecuteAsync("az", "account show", captureOutput: true); - if (subscriptionResult == null) - { - _logger.LogError("Failed to execute account show command - null result"); - return EndpointRegistrationResult.Failed; - } - - if (!subscriptionResult.Success) - { - _logger.LogError("Failed to get subscription information for endpoint creation"); - return EndpointRegistrationResult.Failed; - } - - var cleanedOutput = JsonDeserializationHelper.CleanAzureCliJsonOutput(subscriptionResult.StandardOutput); - var subscriptionInfo = JsonSerializer.Deserialize(cleanedOutput); - var tenantId = subscriptionInfo.GetProperty("tenantId").GetString(); - var currentUser = subscriptionInfo.TryGetProperty("user", out var userProp) && - userProp.TryGetProperty("name", out var nameProp) - ? nameProp.GetString() : null; + // Load config first to get tenant ID — avoids az CLI subprocess for account info. + var config = await _configService.LoadAsync(); + var tenantId = config.TenantId; if (string.IsNullOrEmpty(tenantId)) { @@ -88,12 +71,17 @@ public async Task CreateEndpointWithAgentBlueprintAs return EndpointRegistrationResult.Failed; } + // Resolve login hint for account pre-selection in WAM/MSAL. + // Try az CLI first (if the user has run 'az login'); fall back to the + // UPN embedded in a previously cached MSAL token — non-fatal if unavailable. + var currentUser = await AzCliHelper.ResolveLoginHintAsync() + ?? await _authService.ResolveLoginHintFromCacheAsync(); + // Create new endpoint with agent blueprint identity _logger.LogInformation("Creating new endpoint with Agent Blueprint Identity..."); try { - var config = await _configService.LoadAsync(); var createEndpointUrl = EndpointHelper.GetCreateEndpointUrl(config.Environment); _logger.LogInformation("Calling create endpoint directly..."); @@ -254,27 +242,8 @@ public async Task DeleteEndpointWithAgentBlueprintAsync( try { - // Get subscription info for tenant ID - var subscriptionResult = await _executor.ExecuteAsync("az", "account show", captureOutput: true); - if (subscriptionResult == null) - { - _logger.LogError("Failed to execute account show command - null result"); - return false; - } - - if (!subscriptionResult.Success) - { - _logger.LogError("Failed to get subscription information for endpoint deletion"); - return false; - } - - var cleanedOutput = JsonDeserializationHelper.CleanAzureCliJsonOutput(subscriptionResult.StandardOutput); - var subscriptionInfo = JsonSerializer.Deserialize(cleanedOutput); - var tenantId = subscriptionInfo.GetProperty("tenantId").GetString(); - var currentUser = subscriptionInfo.TryGetProperty("user", out var userProp) && - userProp.TryGetProperty("name", out var nameProp) - ? nameProp.GetString() : null; - _logger.LogDebug("ATG token request — current user from az account: {CurrentUser}", currentUser ?? "(null)"); + var config = await _configService.LoadAsync(); + var tenantId = config.TenantId; if (string.IsNullOrEmpty(tenantId)) { @@ -282,12 +251,15 @@ public async Task DeleteEndpointWithAgentBlueprintAsync( return false; } + var currentUser = await AzCliHelper.ResolveLoginHintAsync() + ?? await _authService.ResolveLoginHintFromCacheAsync(); + _logger.LogDebug("ATG token request — current user: {CurrentUser}", currentUser ?? "(null)"); + // Delete endpoint with agent blueprint identity _logger.LogInformation("Deleting endpoint with Agent Blueprint Identity..."); try { - var config = await _configService.LoadAsync(); var deleteEndpointUrl = EndpointHelper.GetDeleteEndpointUrl(config.Environment); _logger.LogInformation("Calling delete endpoint directly..."); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs index eb155eb4..047fac91 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs @@ -561,8 +561,7 @@ private async Task TryExtendConsentGrantScopesAsync( return null; } - _logger.LogDebug("Graph app query returned 401 — invalidating token cache and retrying (possible CAE revocation)"); - AzCliHelper.InvalidateAzCliTokenCache(); + _logger.LogDebug("Graph app query returned 401 — retrying with fresh token (possible CAE revocation)"); graphResponse = await _graphApiService.GraphGetWithResponseAsync(tenantId, string.Format(path, clientAppId), ct); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/DelegatedConsentService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/DelegatedConsentService.cs index 1c7e7be9..9af5df0e 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/DelegatedConsentService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/DelegatedConsentService.cs @@ -275,13 +275,9 @@ public async Task EnsureBlueprintPermissionGrantAsync( return null; } - // The new az login session invalidates any previously cached tokens. - AzCliHelper.InvalidateAzCliTokenCache(); - _logger.LogInformation(" Acquiring fresh Graph API token..."); - // Re-populate the process-level cache with the new session's token. - var token = await AzCliHelper.AcquireAzCliTokenAsync(GraphApiConstants.GetResource(GraphApiConstants.BaseUrl), tenantId); + var token = await _graphService.GetGraphAccessTokenAsync(tenantId); if (!string.IsNullOrWhiteSpace(token)) { diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs index c4dd688b..1d079419 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs @@ -24,20 +24,20 @@ public class GraphApiService private readonly CommandExecutor _executor; private readonly HttpClient _httpClient; private readonly IMicrosoftGraphTokenProvider? _tokenProvider; + private readonly IAuthenticationService _authService; + private readonly RetryHelper _retryHelper; private string _graphBaseUrl; - // Token caching is handled at the process level by AzCliHelper.AcquireAzCliTokenAsync. - // All GraphApiService instances (and other services) share a single token per - // (resource, tenantId) pair — no per-instance cache needed. - - // Login hint resolved once per GraphApiService instance from 'az account show'. - // Used to direct MSAL/WAM to the correct Azure CLI identity, preventing the Windows - // account (WAM default) or a stale cached MSAL account from being used instead. + // Login hint resolved once per GraphApiService instance. + // Used to direct MSAL/WAM to the correct identity, preventing the Windows default + // account (WAM) or a stale cached MSAL account from being used instead. + // Resolved from az CLI if available, otherwise from the AuthenticationService token cache. private string? _loginHint; private bool _loginHintResolved; - // Resolver delegate for the login hint. Defaults to AzCliHelper.ResolveLoginHintAsync; - // injectable via constructor so unit tests can bypass the real 'az account show' process. + // Resolver delegate for the login hint. + // Defaults to az CLI first, then AuthenticationService JWT cache as fallback. + // Injectable via constructor so unit tests can bypass the real az process. private readonly Func> _loginHintResolver; /// @@ -68,164 +68,69 @@ public record GraphResponse } // Allow injecting a custom HttpMessageHandler for unit testing. - // loginHintResolver: optional override for 'az account show' login-hint resolution. - // Pass () => Task.FromResult(null) in unit tests to skip the real az process. - public GraphApiService(ILogger logger, CommandExecutor executor, HttpMessageHandler? handler = null, IMicrosoftGraphTokenProvider? tokenProvider = null, Func>? loginHintResolver = null, string? graphBaseUrl = null) + // loginHintResolver: optional override for login-hint resolution. + // Pass () => Task.FromResult(null) in unit tests to skip login-hint resolution. + public GraphApiService(ILogger logger, CommandExecutor executor, IAuthenticationService authService, HttpMessageHandler? handler = null, IMicrosoftGraphTokenProvider? tokenProvider = null, Func>? loginHintResolver = null, string? graphBaseUrl = null, RetryHelper? retryHelper = null) { _logger = logger; _executor = executor; + _authService = authService; _httpClient = handler != null ? new HttpClient(handler) : HttpClientFactory.CreateAuthenticatedClient(); _tokenProvider = tokenProvider; - _loginHintResolver = loginHintResolver ?? AzCliHelper.ResolveLoginHintAsync; + _retryHelper = retryHelper ?? new RetryHelper(_logger); + // Default: try az CLI first (if present), fall back to JWT cache in AuthenticationService. + _loginHintResolver = loginHintResolver ?? (() => ResolveLoginHintWithFallbackAsync(authService)); _graphBaseUrl = string.IsNullOrWhiteSpace(graphBaseUrl) ? GraphApiConstants.BaseUrl : graphBaseUrl; } // Parameterless constructor to ease test mocking/substitution frameworks which may // require creating proxy instances without providing constructor arguments. public GraphApiService() - : this(NullLogger.Instance, new CommandExecutor(NullLogger.Instance), null, null, null) + : this(NullLogger.Instance, new CommandExecutor(NullLogger.Instance), new AuthenticationService(NullLogger.Instance), null, null, null) { } // Two-argument convenience constructor used by tests and callers that supply // a logger and an existing CommandExecutor (no custom handler). public GraphApiService(ILogger logger, CommandExecutor executor) - : this(logger ?? NullLogger.Instance, executor ?? throw new ArgumentNullException(nameof(executor)), null, null, null) + : this(logger ?? NullLogger.Instance, executor ?? throw new ArgumentNullException(nameof(executor)), new AuthenticationService(NullLogger.Instance), null, null, null) + { + } + + private static async Task ResolveLoginHintWithFallbackAsync(IAuthenticationService authService) { + // Try az CLI first — most reliable when the user has run 'az login'. + var hint = await AzCliHelper.ResolveLoginHintAsync(); + if (!string.IsNullOrWhiteSpace(hint)) + return hint; + // Fall back to the UPN embedded in a previously cached MSAL JWT. + return await authService.ResolveLoginHintFromCacheAsync(); } /// - /// Get access token for Microsoft Graph API using Azure CLI + /// Acquires an access token for Microsoft Graph API via MSAL (WAM on Windows, + /// browser/device-code on macOS/Linux). Token is cached persistently by + /// AuthenticationService — no az CLI subprocess involved. /// public virtual async Task GetGraphAccessTokenAsync(string tenantId, CancellationToken ct = default) { _logger.LogDebug("Acquiring Graph API access token for tenant {TenantId}", tenantId); - try { var resource = GraphApiConstants.GetResource(_graphBaseUrl); - - // Check the process-level cache first. AzCliHelper caches tokens per (resource, tenantId) - // for the process lifetime — avoids spawning duplicate 'az account get-access-token' - // subprocesses when multiple services request the same token. - var cachedToken = await AzCliHelper.AcquireAzCliTokenAsync(resource, tenantId); - if (!string.IsNullOrWhiteSpace(cachedToken)) - { - _logger.LogDebug("Graph API access token acquired from cache"); - return cachedToken; - } - - // Cache miss or az CLI not authenticated — check login state via the injectable resolver. - // Using _loginHintResolver (not the static helper directly) keeps the test seam consistent. var loginHint = await _loginHintResolver(); - if (loginHint == null) - { - _logger.LogInformation("Azure CLI not authenticated. Initiating login..."); - _logger.LogInformation("A browser window will open for authentication. Please check your taskbar or browser if you don't see it."); - var loginResult = await _executor.ExecuteAsync( - "az", - $"login --tenant {tenantId}", - cancellationToken: ct); - - if (!loginResult.Success) - { - _logger.LogError("Azure CLI login failed"); - return null; - } - - // Bust the caches so the fresh login identity and token are picked up. - AzCliHelper.InvalidateLoginHintCache(); - AzCliHelper.InvalidateAzCliTokenCache(); - } - - // Acquire the token — this goes through AzCliHelper so it is cached for all - // subsequent callers (including those that call AzCliHelper directly). - var tokenResult = await _executor.ExecuteAsync( - "az", - $"account get-access-token --resource {resource} --tenant {tenantId} --query accessToken -o tsv", - captureOutput: true, - cancellationToken: ct); - - if (tokenResult.Success && !string.IsNullOrWhiteSpace(tokenResult.StandardOutput)) + var token = await _authService.GetAccessTokenAsync(resource, tenantId, userId: loginHint); + if (!string.IsNullOrWhiteSpace(token)) { - var token = tokenResult.StandardOutput.Trim(); - // Warm the shared cache so other services that call AzCliHelper.AcquireAzCliTokenAsync - // directly receive this token without spawning another subprocess. - AzCliHelper.WarmAzCliTokenCache(resource, tenantId, token); _logger.LogDebug("Graph API access token acquired successfully"); return token; } - - // Check for CAE-related errors in the error output - var errorOutput = tokenResult.StandardError ?? ""; - if (errorOutput.Contains("AADSTS50173", StringComparison.OrdinalIgnoreCase) || - errorOutput.Contains("session", StringComparison.OrdinalIgnoreCase) || - errorOutput.Contains("expired", StringComparison.OrdinalIgnoreCase)) - { - _logger.LogWarning("Authentication session may have expired. Attempting fresh login..."); - - // Force logout and re-login - _logger.LogInformation("Logging out of Azure CLI..."); - await _executor.ExecuteAsync("az", "logout", suppressErrorLogging: true, cancellationToken: ct); - - _logger.LogInformation("Initiating fresh login..."); - var freshLoginResult = await _executor.ExecuteAsync( - "az", - $"login --tenant {tenantId}", - cancellationToken: ct); - - if (!freshLoginResult.Success) - { - _logger.LogError("Fresh login failed. Please manually run: az login --tenant {TenantId}", tenantId); - return null; - } - - // Bust caches after re-authentication so stale tokens are not returned. - AzCliHelper.InvalidateLoginHintCache(); - AzCliHelper.InvalidateAzCliTokenCache(); - - // Retry token acquisition - _logger.LogInformation("Retrying token acquisition..."); - var retryTokenResult = await _executor.ExecuteAsync( - "az", - $"account get-access-token --resource {resource} --tenant {tenantId} --query accessToken -o tsv", - captureOutput: true, - cancellationToken: ct); - - if (retryTokenResult.Success && !string.IsNullOrWhiteSpace(retryTokenResult.StandardOutput)) - { - var token = retryTokenResult.StandardOutput.Trim(); - AzCliHelper.WarmAzCliTokenCache(resource, tenantId, token); - _logger.LogInformation("Graph API access token acquired successfully after re-authentication"); - return token; - } - - _logger.LogError("Failed to acquire token after re-authentication: {Error}", retryTokenResult.StandardError); - return null; - } - - _logger.LogError("Failed to acquire Graph API access token: {Error}", tokenResult.StandardError); + _logger.LogError("Failed to acquire Graph API access token for tenant {TenantId}", tenantId); return null; } catch (Exception ex) { _logger.LogError(ex, "Error acquiring Graph API access token"); - - // Check for CAE-related exceptions - if (ex.Message.Contains("TokenIssuedBeforeRevocationTimestamp", StringComparison.OrdinalIgnoreCase) || - ex.Message.Contains("InteractionRequired", StringComparison.OrdinalIgnoreCase)) - { - _logger.LogError(""); - _logger.LogError("=== AUTHENTICATION SESSION EXPIRED ==="); - _logger.LogError("Your authentication session is no longer valid."); - _logger.LogError(""); - _logger.LogError("TO RESOLVE:"); - _logger.LogError(" 1. Run: az logout"); - _logger.LogError(" 2. Run: az login --tenant {TenantId}", tenantId); - _logger.LogError(" 3. Retry your command"); - _logger.LogError(""); - } - return null; } } @@ -234,9 +139,9 @@ public GraphApiService(ILogger logger, CommandExecutor executor private async Task EnsureGraphHeadersAsync(string tenantId, CancellationToken ct = default, IEnumerable? scopes = null) { // Authentication Strategy: - // 1. If specific scopes required AND token provider configured: Use interactive browser auth (cached) - // 2. Otherwise: Use Azure CLI token (requires 'az login') - // This dual approach minimizes auth prompts while supporting special scope requirements + // 1. If specific scopes required AND token provider configured: Use MSAL with delegated scopes (WAM/browser/device-code) + // 2. Otherwise: Use MSAL via AuthenticationService (WAM/browser/device-code, persistent cache) + // All paths go through MSAL — no az CLI subprocess involved. string? token; @@ -263,25 +168,13 @@ private async Task EnsureGraphHeadersAsync(string tenantId, CancellationTo } else { - // Use the process-level token cache in AzCliHelper — shared across all service - // instances so a token acquired in any phase is reused by subsequent phases. - token = await AzCliHelper.AcquireAzCliTokenAsync(GraphApiConstants.GetResource(_graphBaseUrl), tenantId); + // Default path: acquire via AuthenticationService (MSAL, persistent disk cache). + token = await GetGraphAccessTokenAsync(tenantId, ct); if (string.IsNullOrWhiteSpace(token)) { - // Cache miss or az CLI not authenticated — run full auth + recovery flow. - _logger.LogDebug("Process-level token cache miss; running full auth flow for tenant {TenantId}", tenantId); - token = await GetGraphAccessTokenAsync(tenantId, ct); - - if (string.IsNullOrWhiteSpace(token)) - { - _logger.LogError("Failed to acquire Graph token via Azure CLI. Ensure 'az login' is completed."); - return false; - } - - // Warm the process-level cache so subsequent callers (including other - // GraphApiService instances and services) skip the auth flow entirely. - AzCliHelper.WarmAzCliTokenCache(GraphApiConstants.GetResource(_graphBaseUrl), tenantId, token); + _logger.LogError("Failed to acquire Graph token. Sign-in will be prompted on the next attempt."); + return false; } } @@ -328,16 +221,27 @@ public virtual async Task ServicePrincipalExistsAsync(string tenantId, str { if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) return null; var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); - using var resp = await _httpClient.GetAsync(url, ct); - if (!resp.IsSuccessStatusCode) + try + { + using var resp = await _retryHelper.ExecuteWithRetryAsync( + ct => _httpClient.GetAsync(url, ct), cancellationToken: ct); + if (!resp.IsSuccessStatusCode) + { + var errorBody = await resp.Content.ReadAsStringAsync(ct); + _logger.LogDebug("Graph GET {Url} failed {Code} {Reason}: {Body}", url, (int)resp.StatusCode, resp.ReasonPhrase, errorBody); + return null; + } + var json = await resp.Content.ReadAsStringAsync(ct); + return JsonDocument.Parse(json); + } + catch (Exception ex) when (ex is not OperationCanceledException) { - var errorBody = await resp.Content.ReadAsStringAsync(ct); - _logger.LogDebug("Graph GET {Url} failed {Code} {Reason}: {Body}", url, (int)resp.StatusCode, resp.ReasonPhrase, errorBody); + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "Graph GET {Url} failed with exception", url); return null; } - var json = await resp.Content.ReadAsStringAsync(ct); - - return JsonDocument.Parse(json); } /// @@ -377,7 +281,10 @@ public virtual async Task GraphGetWithResponseAsync(string tenant } catch (Exception ex) { - _logger.LogDebug(ex, "Graph GET {Url} threw an exception", url); + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "Graph GET {Url} threw an exception", url); return new GraphResponse { IsSuccess = false, StatusCode = 0, ReasonPhrase = ex.Message, Body = string.Empty }; } } @@ -387,20 +294,31 @@ public virtual async Task GraphGetWithResponseAsync(string tenant if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) return null; var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); var content = new StringContent(JsonSerializer.Serialize(payload), Encoding.UTF8, "application/json"); - using var resp = await _httpClient.PostAsync(url, content, ct); - var body = await resp.Content.ReadAsStringAsync(ct); - if (!resp.IsSuccessStatusCode) + try { - var errorMessage = TryExtractGraphErrorMessage(body); - if (errorMessage != null) - _logger.LogWarning("Graph POST {Url} failed: {ErrorMessage}", url, errorMessage); + using var resp = await _httpClient.PostAsync(url, content, ct); + var body = await resp.Content.ReadAsStringAsync(ct); + if (!resp.IsSuccessStatusCode) + { + var errorMessage = TryExtractGraphErrorMessage(body); + if (errorMessage != null) + _logger.LogWarning("Graph POST {Url} failed: {ErrorMessage}", url, errorMessage); + else + _logger.LogWarning("Graph POST {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); + _logger.LogDebug("Graph POST response body: {Body}", body); + return null; + } + + return string.IsNullOrWhiteSpace(body) ? null : JsonDocument.Parse(body); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); else - _logger.LogWarning("Graph POST {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); - _logger.LogDebug("Graph POST response body: {Body}", body); - return null; + _logger.LogDebug(ex, "Graph POST {Url} failed with exception", url); + throw; } - - return string.IsNullOrWhiteSpace(body) ? null : JsonDocument.Parse(body); } /// @@ -416,23 +334,34 @@ public virtual async Task GraphPostWithResponseAsync(string tenan var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); var content = new StringContent(JsonSerializer.Serialize(payload), Encoding.UTF8, "application/json"); - using var resp = await _httpClient.PostAsync(url, content, ct); - var body = await resp.Content.ReadAsStringAsync(ct); - - JsonDocument? json = null; - if (!string.IsNullOrWhiteSpace(body)) + try { - try { json = JsonDocument.Parse(body); } catch { /* ignore parse errors */ } - } + using var resp = await _httpClient.PostAsync(url, content, ct); + var body = await resp.Content.ReadAsStringAsync(ct); - return new GraphResponse + JsonDocument? json = null; + if (!string.IsNullOrWhiteSpace(body)) + { + try { json = JsonDocument.Parse(body); } catch { /* ignore parse errors */ } + } + + return new GraphResponse + { + IsSuccess = resp.IsSuccessStatusCode, + StatusCode = (int)resp.StatusCode, + ReasonPhrase = resp.ReasonPhrase ?? string.Empty, + Body = body ?? string.Empty, + Json = json + }; + } + catch (Exception ex) when (ex is not OperationCanceledException) { - IsSuccess = resp.IsSuccessStatusCode, - StatusCode = (int)resp.StatusCode, - ReasonPhrase = resp.ReasonPhrase ?? string.Empty, - Body = body ?? string.Empty, - Json = json - }; + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "Graph POST {Url} failed with exception", url); + throw; + } } /// @@ -444,22 +373,33 @@ public virtual async Task GraphPatchAsync(string tenantId, string relative if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) return false; var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); var content = new StringContent(JsonSerializer.Serialize(payload), Encoding.UTF8, "application/json"); - using var request = new HttpRequestMessage(new HttpMethod("PATCH"), url) { Content = content }; - using var resp = await _httpClient.SendAsync(request, ct); + try + { + using var request = new HttpRequestMessage(new HttpMethod("PATCH"), url) { Content = content }; + using var resp = await _httpClient.SendAsync(request, ct); - // Many PATCH calls return 204 NoContent on success - if (!resp.IsSuccessStatusCode) + // Many PATCH calls return 204 NoContent on success + if (!resp.IsSuccessStatusCode) + { + var body = await resp.Content.ReadAsStringAsync(ct); + var errorMessage = TryExtractGraphErrorMessage(body); + if (errorMessage != null) + _logger.LogError("Graph PATCH {Url} failed: {ErrorMessage}", url, errorMessage); + else + _logger.LogError("Graph PATCH {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); + _logger.LogDebug("Graph PATCH response body: {Body}", body); + } + + return resp.IsSuccessStatusCode; + } + catch (Exception ex) when (ex is not OperationCanceledException) { - var body = await resp.Content.ReadAsStringAsync(ct); - var errorMessage = TryExtractGraphErrorMessage(body); - if (errorMessage != null) - _logger.LogError("Graph PATCH {Url} failed: {ErrorMessage}", url, errorMessage); + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); else - _logger.LogError("Graph PATCH {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); - _logger.LogDebug("Graph PATCH response body: {Body}", body); + _logger.LogDebug(ex, "Graph PATCH {Url} failed with exception", url); + throw; } - - return resp.IsSuccessStatusCode; } public async Task GraphDeleteAsync( @@ -473,25 +413,36 @@ public async Task GraphDeleteAsync( var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); - using var req = new HttpRequestMessage(HttpMethod.Delete, url); - using var resp = await _httpClient.SendAsync(req, ct); + try + { + using var req = new HttpRequestMessage(HttpMethod.Delete, url); + using var resp = await _httpClient.SendAsync(req, ct); + + // 404 can be considered success for idempotent deletes + if (treatNotFoundAsSuccess && (int)resp.StatusCode == 404) return true; - // 404 can be considered success for idempotent deletes - if (treatNotFoundAsSuccess && (int)resp.StatusCode == 404) return true; + if (!resp.IsSuccessStatusCode) + { + var body = await resp.Content.ReadAsStringAsync(ct); + var errorMessage = TryExtractGraphErrorMessage(body); + if (errorMessage != null) + _logger.LogError("Graph DELETE {Url} failed: {ErrorMessage}", url, errorMessage); + else + _logger.LogError("Graph DELETE {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); + _logger.LogDebug("Graph DELETE response body: {Body}", body); + return false; + } - if (!resp.IsSuccessStatusCode) + return true; + } + catch (Exception ex) when (ex is not OperationCanceledException) { - var body = await resp.Content.ReadAsStringAsync(ct); - var errorMessage = TryExtractGraphErrorMessage(body); - if (errorMessage != null) - _logger.LogError("Graph DELETE {Url} failed: {ErrorMessage}", url, errorMessage); + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); else - _logger.LogError("Graph DELETE {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); - _logger.LogDebug("Graph DELETE response body: {Body}", body); - return false; + _logger.LogDebug(ex, "Graph DELETE {Url} failed with exception", url); + throw; } - - return true; } /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AzCliHelper.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AzCliHelper.cs index f1f93439..da519e7c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AzCliHelper.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/AzCliHelper.cs @@ -2,7 +2,6 @@ // Licensed under the MIT License. using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; -using System.Collections.Concurrent; using System.Diagnostics; using System.Runtime.InteropServices; using System.Text.Json; @@ -11,12 +10,14 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; /// /// Shared helper for invoking the Azure CLI and parsing its output. -/// Consolidates az CLI interactions to ensure consistent behavior across services. +/// Used for login-hint resolution (az account show) and az CLI command execution. +/// Token acquisition has been moved to AuthenticationService (MSAL/WAM) — this +/// helper no longer acquires tokens directly. /// internal static class AzCliHelper { // Process-level cache: 'az account show' returns the same user for the lifetime of a CLI - // invocation. Caching eliminates repeated 20-40s subprocess calls that occur when multiple + // invocation. Caching eliminates repeated subprocess calls that occur when multiple // services and commands each call ResolveLoginHintAsync independently. private static volatile Task? _cachedLoginHintTask; @@ -41,119 +42,6 @@ internal static class AzCliHelper /// Clears the login-hint process-level cache. For use in tests only. internal static void ResetLoginHintCacheForTesting() => _cachedLoginHintTask = null; - // ------------------------------------------------------------------------- - // az account get-access-token — process-level token cache - // ------------------------------------------------------------------------- - // Tokens acquired via 'az account get-access-token' are valid for 60+ minutes. - // Caching at the process level means a single CLI invocation only spawns one - // subprocess per (resource, tenantId) pair, regardless of how many services or - // commands request the same token. Expected savings: 40–60s per command run. - - private static readonly ConcurrentDictionary> _azCliTokenCache = new(); - - // Test seam: replace the underlying acquirer without touching the cache layer. - // The override is invoked inside GetOrAdd, so the result is still cached after - // the first call — only one invocation per cache key, even in tests. - internal static Func>? AzCliTokenAcquirerOverride { get; set; } - - /// - /// Acquires an Azure CLI access token for the given resource and tenant. - /// The result is cached for the process lifetime — a single CLI command cannot - /// invalidate a token except through explicit re-authentication (az login). - /// Call after 'az login' to bust the cache. - /// - internal static Task AcquireAzCliTokenAsync(string resource, string tenantId = "", CancellationToken ct = default) - { - var key = $"{resource}::{tenantId}"; - return _azCliTokenCache.GetOrAdd(key, _ => - AzCliTokenAcquirerOverride != null - ? AzCliTokenAcquirerOverride(resource, tenantId) - : AcquireAzCliTokenCoreAsync(resource, tenantId, ct)); - } - - /// - /// Injects a token acquired via an alternative auth flow (e.g., after 'az login' recovery) - /// so that subsequent callers across all services receive the fresh token from cache. - /// - internal static void WarmAzCliTokenCache(string resource, string tenantId, string token) - { - var key = $"{resource}::{tenantId}"; - _azCliTokenCache[key] = Task.FromResult(token); - } - - /// - /// Clears the token cache. Call after 'az login' or 'az logout' to ensure - /// subsequent callers acquire a fresh token rather than a now-invalid cached one. - /// - internal static void InvalidateAzCliTokenCache() => _azCliTokenCache.Clear(); - - /// Clears the token cache. For use in tests only. - internal static void ResetAzCliTokenCacheForTesting() => _azCliTokenCache.Clear(); - - private static async Task AcquireAzCliTokenCoreAsync(string resource, string tenantId, CancellationToken ct = default) - { - Process? process = null; - try - { - // On Windows az is az.cmd which requires cmd.exe to launch. On all platforms - // arguments are passed via ArgumentList (not string interpolation) so - // resource/tenantId values cannot alter the command line. - var isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); - var startInfo = new ProcessStartInfo - { - FileName = isWindows ? "cmd.exe" : "az", - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true - }; - // On Windows: cmd.exe /c az . On other platforms: az . - if (isWindows) - { - startInfo.ArgumentList.Add("/c"); - startInfo.ArgumentList.Add("az"); - } - startInfo.ArgumentList.Add("account"); - startInfo.ArgumentList.Add("get-access-token"); - startInfo.ArgumentList.Add("--resource"); - startInfo.ArgumentList.Add(resource); - if (!string.IsNullOrEmpty(tenantId)) - { - startInfo.ArgumentList.Add("--tenant"); - startInfo.ArgumentList.Add(tenantId); - } - startInfo.ArgumentList.Add("--query"); - startInfo.ArgumentList.Add("accessToken"); - startInfo.ArgumentList.Add("-o"); - startInfo.ArgumentList.Add("tsv"); - - process = Process.Start(startInfo); - if (process == null) return null; - // Start reads concurrently so the pipe buffers never fill up and block the process. - // WaitForExitAsync(ct) is awaited first so cancellation is observed immediately; - // the reads complete naturally once the process exits and the pipes close. - var outputTask = process.StandardOutput.ReadToEndAsync(); - var errorTask = process.StandardError.ReadToEndAsync(); - await process.WaitForExitAsync(ct); - await Task.WhenAll(outputTask, errorTask); - var output = outputTask.Result.Trim(); - return process.ExitCode == 0 && !string.IsNullOrWhiteSpace(output) ? output : null; - } - catch (OperationCanceledException) - { - try { process?.Kill(entireProcessTree: true); } catch { } - throw; - } - catch - { - return null; - } - finally - { - process?.Dispose(); - } - } - private static async Task ResolveLoginHintCoreAsync() { try diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/NetworkHelper.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/NetworkHelper.cs new file mode 100644 index 00000000..95b86259 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/NetworkHelper.cs @@ -0,0 +1,34 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Net.Sockets; + +namespace Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; + +/// +/// Shared network diagnostics helpers used by ArmApiService and GraphApiService. +/// +internal static class NetworkHelper +{ + /// + /// Returns true when the exception chain contains a SocketException with ConnectionReset (10054). + /// This error occurs on corporate networks where a TLS inspection proxy (Zscaler, Netskope, etc.) + /// drops long-running connections. Logging a targeted warning helps users self-diagnose and retry. + /// + internal static bool IsConnectionResetByProxy(Exception ex) + { + var inner = ex.InnerException; + while (inner != null) + { + if (inner is SocketException se && se.SocketErrorCode == SocketError.ConnectionReset) + return true; + inner = inner.InnerException; + } + return ex is SocketException root && root.SocketErrorCode == SocketError.ConnectionReset; + } + + internal const string ConnectionResetWarning = + "Network connection was reset by a remote host (SocketError 10054). " + + "This often occurs on corporate networks with TLS inspection proxies. " + + "Re-running the command usually succeeds."; +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/RetryHelper.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/RetryHelper.cs index 9979790b..eab9e917 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/RetryHelper.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/RetryHelper.cs @@ -12,10 +12,14 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; public class RetryHelper { private readonly ILogger _logger; + private readonly int _maxRetries; + private readonly int _baseDelaySeconds; - public RetryHelper(ILogger logger) + public RetryHelper(ILogger logger, int maxRetries = 3, int baseDelaySeconds = 1) { _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + _maxRetries = maxRetries; + _baseDelaySeconds = baseDelaySeconds; } /// @@ -24,8 +28,8 @@ public RetryHelper(ILogger logger) /// Return type of the operation /// The async operation to execute. Receives a cancellation token and returns a result. /// Predicate that determines if retry is needed. Returns TRUE when the operation should be retried (operation failed), FALSE when operation succeeded and no retry is needed. - /// Maximum number of retry attempts before giving up (default: 5) - /// Base delay in seconds for exponential backoff calculation (default: 2). Actual delay doubles with each attempt. + /// Maximum number of retry attempts. Defaults to the value set in the constructor. + /// Base delay in seconds for exponential backoff. Defaults to the value set in the constructor. /// Cancellation token to cancel the operation /// Result of the operation when shouldRetry returns false (success), or the last result after all retries are exhausted (may be null/default(T) if operation never succeeded) /// Thrown when HTTP request fails on the last retry attempt @@ -33,15 +37,17 @@ public RetryHelper(ILogger logger) public async Task ExecuteWithRetryAsync( Func> operation, Func shouldRetry, - int maxRetries = 5, - int baseDelaySeconds = 2, + int? maxRetries = null, + int? baseDelaySeconds = null, CancellationToken cancellationToken = default) { + var retries = maxRetries ?? _maxRetries; + var delay = baseDelaySeconds ?? _baseDelaySeconds; int attempt = 0; Exception? lastException = null; T? lastResult = default; - while (attempt < maxRetries) + while (attempt < retries) { try { @@ -52,14 +58,14 @@ public async Task ExecuteWithRetryAsync( return lastResult; } - if (attempt < maxRetries - 1) + if (attempt < retries - 1) { - var delay = CalculateDelay(attempt, baseDelaySeconds); + var delaySpan = CalculateDelay(attempt, delay); _logger.LogInformation( "Retry attempt {AttemptNumber} of {MaxRetries}. Waiting {DelaySeconds} seconds...", - attempt + 1, maxRetries, (int)delay.TotalSeconds); + attempt + 1, retries, (int)delaySpan.TotalSeconds); - await Task.Delay(delay, cancellationToken); + await Task.Delay(delaySpan, cancellationToken); } attempt++; @@ -72,14 +78,14 @@ public async Task ExecuteWithRetryAsync( lastException = ex; _logger.LogWarning("Exception: {Message}", ex.Message); - if (attempt < maxRetries - 1) + if (attempt < retries - 1) { - var delay = CalculateDelay(attempt, baseDelaySeconds); + var delaySpan = CalculateDelay(attempt, delay); _logger.LogInformation( "Retry attempt {AttemptNumber} of {MaxRetries}. Waiting {DelaySeconds} seconds...", - attempt + 1, maxRetries, (int)delay.TotalSeconds); + attempt + 1, retries, (int)delaySpan.TotalSeconds); - await Task.Delay(delay, cancellationToken); + await Task.Delay(delaySpan, cancellationToken); } attempt++; @@ -97,7 +103,7 @@ public async Task ExecuteWithRetryAsync( { throw new RetryExhaustedException( "Async operation with retry", - maxRetries, + retries, "Operation did not return a value and no exception was thrown"); } @@ -109,14 +115,14 @@ public async Task ExecuteWithRetryAsync( /// /// Return type of the operation /// The operation to execute - /// Maximum number of retry attempts (default: 5) - /// Base delay in seconds for exponential backoff (default: 2) + /// Maximum number of retry attempts. Defaults to the value set in the constructor. + /// Base delay in seconds for exponential backoff. Defaults to the value set in the constructor. /// Cancellation token /// Result of the operation public async Task ExecuteWithRetryAsync( Func> operation, - int maxRetries = 5, - int baseDelaySeconds = 2, + int? maxRetries = null, + int? baseDelaySeconds = null, CancellationToken cancellationToken = default) { return await ExecuteWithRetryAsync( @@ -135,22 +141,24 @@ public async Task ExecuteWithRetryAsync( /// Return type of the operation /// The async operation to execute. Receives a cancellation token and returns a result. /// Async predicate that determines if retry is needed. Returns TRUE when the operation should be retried, FALSE when done. - /// Maximum number of retry attempts before giving up (default: 5) - /// Base delay in seconds for exponential backoff calculation (default: 2). + /// Maximum number of retry attempts. Defaults to the value set in the constructor. + /// Base delay in seconds for exponential backoff. Defaults to the value set in the constructor. /// Cancellation token to cancel the operation /// Result of the operation when shouldRetryAsync returns false (success), or the last result after all retries are exhausted. public async Task ExecuteWithRetryAsync( Func> operation, Func> shouldRetryAsync, - int maxRetries = 5, - int baseDelaySeconds = 2, + int? maxRetries = null, + int? baseDelaySeconds = null, CancellationToken cancellationToken = default) { + var retries = maxRetries ?? _maxRetries; + var delay = baseDelaySeconds ?? _baseDelaySeconds; int attempt = 0; Exception? lastException = null; T? lastResult = default; - while (attempt < maxRetries) + while (attempt < retries) { try { @@ -161,14 +169,14 @@ public async Task ExecuteWithRetryAsync( return lastResult; } - if (attempt < maxRetries - 1) + if (attempt < retries - 1) { - var delay = CalculateDelay(attempt, baseDelaySeconds); + var delaySpan = CalculateDelay(attempt, delay); _logger.LogInformation( "Retry attempt {AttemptNumber} of {MaxRetries}. Waiting {DelaySeconds} seconds...", - attempt + 1, maxRetries, (int)delay.TotalSeconds); + attempt + 1, retries, (int)delaySpan.TotalSeconds); - await Task.Delay(delay, cancellationToken); + await Task.Delay(delaySpan, cancellationToken); } attempt++; @@ -181,14 +189,14 @@ public async Task ExecuteWithRetryAsync( lastException = ex; _logger.LogWarning("Exception: {Message}", ex.Message); - if (attempt < maxRetries - 1) + if (attempt < retries - 1) { - var delay = CalculateDelay(attempt, baseDelaySeconds); + var delaySpan = CalculateDelay(attempt, delay); _logger.LogInformation( "Retry attempt {AttemptNumber} of {MaxRetries}. Waiting {DelaySeconds} seconds...", - attempt + 1, maxRetries, (int)delay.TotalSeconds); + attempt + 1, retries, (int)delaySpan.TotalSeconds); - await Task.Delay(delay, cancellationToken); + await Task.Delay(delaySpan, cancellationToken); } attempt++; @@ -204,7 +212,7 @@ public async Task ExecuteWithRetryAsync( { throw new RetryExhaustedException( "Async operation with retry", - maxRetries, + retries, "Operation did not return a value and no exception was thrown"); } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/design.md b/src/Microsoft.Agents.A365.DevTools.Cli/design.md index bde68d4d..60344271 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/design.md +++ b/src/Microsoft.Agents.A365.DevTools.Cli/design.md @@ -171,7 +171,62 @@ By default the CLI targets the commercial Microsoft Graph endpoint. For sovereig This field is optional. When omitted, `https://graph.microsoft.com` is used. -The value is read from `Agent365Config.GraphBaseUrl` and forwarded to `GraphApiService` via its `GraphBaseUrl` property after config is loaded. This controls both the HTTP endpoint used for all Graph API calls and the token resource identifier passed to `az account get-access-token`. +The value is read from `Agent365Config.GraphBaseUrl` and forwarded to `GraphApiService` via its `GraphBaseUrl` property after config is loaded. This controls both the HTTP endpoint used for all Graph API calls and the token resource identifier passed to `AuthenticationService.GetAccessTokenAsync`. + +--- + +## Authentication Architecture + +All token acquisition goes through **MSAL.NET via `AuthenticationService`**. No az CLI subprocess is used for tokens. + +### Token Acquisition Flow + +``` +All callers (GraphApiService, ArmApiService, BotConfigurator, ...) + | + v +AuthenticationService.GetAccessTokenAsync(resource, tenantId) + | + +-- Check persistent disk cache (%LocalAppData%\Agent365\token-cache.json) + | Cache key: {resource}[:tenant:{tenantId}][:user:{userId}] + | + +-- Cache hit + not expiring: return token immediately (0 prompts) + | + +-- Cache miss / expired: MsalBrowserCredential.GetTokenAsync(scopes) + | + +-- Windows: WAM broker (no browser, SSO, CAP-compliant) + +-- macOS: System browser → device code fallback if restricted + +-- Linux: Device code flow +``` + +### Two Auth Paths + +| Path | When used | Scopes | Client app | +|---|---|---|---| +| **Default (MSAL)** | ARM, Graph `.default` calls | `{resource}/.default` | PowerShell client ID | +| **Delegated scopes (MSAL)** | Graph calls needing specific permissions (e.g. `AgentInstance.ReadWrite.All`) | Explicit scope list | `clientAppId` from config | + +### Login Hint Resolution + +To prevent WAM from selecting a stale or wrong account, a login hint (UPN) is resolved before interactive auth: +1. `AzCliHelper.ResolveLoginHintAsync()` — reads `az account show` if az CLI is present +2. `AuthenticationService.ResolveLoginHintFromCacheAsync()` — decodes `upn`/`preferred_username` from a cached JWT if az CLI is unavailable + +### IAuthenticationService Interface + +`IAuthenticationService` is defined in the same file as `AuthenticationService` (`AuthenticationService.cs`). This is intentional — the interface is narrow (two methods), tightly coupled to its single implementation, and co-location follows the **related-interfaces convention** in the copilot instructions. It exists solely to enable test substitution in `ArmApiService` and `GraphApiService` without triggering real MSAL/WAM prompts. + +Only `GetAccessTokenAsync` and `ResolveLoginHintFromCacheAsync` are on the interface. Other methods (`GetAccessTokenWithScopesAsync`, `GetAccessTokenForMcpAsync`, `ClearCache`) stay on the concrete class and are used by commands that take `AuthenticationService` directly. + +### Token Caching + +- **Persistent cache** (`AuthenticationService`): survives across CLI invocations, keyed by resource + tenant + user +- **Process-level login hint cache** (`AzCliHelper`): caches the result of `az account show` for the process lifetime — invalidated after `az login` operations + +### Platform Notes + +- **Windows**: WAM handles token acquisition at the OS level — no browser popup, no Python subprocess, corporate proxy not involved +- **macOS/Linux**: Browser redirect or device code — falls back to device code automatically if browser auth is blocked by tenant policy (e.g., corp-managed macOS) --- diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs index c0d9c8da..f2d6d4d7 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs @@ -51,12 +51,8 @@ public BlueprintSubcommandTests() var mockPlatformDetectorLogger = Substitute.For>(); _mockPlatformDetector = Substitute.ForPartsOf(mockPlatformDetectorLogger); _mockBotConfigurator = Substitute.For(); - // Pass a no-op loginHintResolver to prevent AzCliHelper.ResolveLoginHintAsync from spawning - // a real "az account show" process on every test that touches GraphApiService. - Func> noOpLoginHint = () => Task.FromResult(null); _mockGraphApiService = Substitute.ForPartsOf( - Substitute.For>(), _mockExecutor, - (HttpMessageHandler?)null, (IMicrosoftGraphTokenProvider?)null, noOpLoginHint, (string?)null); + Substitute.For>(), _mockExecutor); _mockBlueprintService = Substitute.ForPartsOf(Substitute.For>(), _mockGraphApiService); _mockClientAppValidator = Substitute.For(); _mockBlueprintLookupService = Substitute.ForPartsOf(Substitute.For>(), _mockGraphApiService); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandBotEndpointTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandBotEndpointTests.cs index d03e8782..3a4e90f3 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandBotEndpointTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandBotEndpointTests.cs @@ -68,7 +68,7 @@ public CleanupCommandBotEndpointTests() .Returns("test-token"); var mockGraphLogger = Substitute.For>(); - _graphApiService = new GraphApiService(mockGraphLogger, _mockExecutor, null, _mockTokenProvider); + _graphApiService = new GraphApiService(mockGraphLogger, _mockExecutor, Substitute.For(), null, _mockTokenProvider); var mockBlueprintLogger = Substitute.For>(); _agentBlueprintService = new AgentBlueprintService(mockBlueprintLogger, _graphApiService); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandTests.cs index fee5cb22..f6f4768c 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandTests.cs @@ -63,7 +63,7 @@ public CleanupCommandTests() // Pass a TestHttpMessageHandler (returns 404 when queue empty) instead of null to avoid // real HTTPS calls to graph.microsoft.com — the handler returns immediately, no network needed. var mockGraphLogger = Substitute.For>(); - _graphApiService = new GraphApiService(mockGraphLogger, _mockExecutor, new TestHttpMessageHandler(), _mockTokenProvider, + _graphApiService = new GraphApiService(mockGraphLogger, _mockExecutor, Substitute.For(), new TestHttpMessageHandler(), _mockTokenProvider, loginHintResolver: () => Task.FromResult(null)); // Create AgentBlueprintService wrapping GraphApiService diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs index c053c8a3..d85e33af 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs @@ -7,8 +7,8 @@ using FluentAssertions; using Microsoft.Agents.A365.DevTools.Cli.Models; using Microsoft.Agents.A365.DevTools.Cli.Services; -using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using Xunit; @@ -30,7 +30,6 @@ public AgentBlueprintServiceTests() // instead of falling through to the real implementation and spawning actual az processes. _mockExecutor = Substitute.For(mockExecutorLogger); _mockTokenProvider = Substitute.For(); - AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "tid", "fake-graph-token"); } [Fact] @@ -63,7 +62,7 @@ public async Task SetInheritablePermissionsAsync_Creates_WhenMissing() return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var graphService = new GraphApiService(_mockGraphLogger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var graphService = new GraphApiService(_mockGraphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(_mockLogger, graphService); // ResolveBlueprintObjectIdAsync: First GET to check if blueprintAppId is objectId (returns 404 NotFound) @@ -123,7 +122,7 @@ public async Task SetInheritablePermissionsAsync_Patches_WhenPresent() return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var graphService = new GraphApiService(_mockGraphLogger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var graphService = new GraphApiService(_mockGraphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(_mockLogger, graphService); // Existing entry with one scope @@ -232,7 +231,7 @@ public async Task DeleteAgentIdentityAsync_WhenTokenProviderIsNull_ReturnsFalse( { // Arrange using var handler = new FakeHttpMessageHandler(); - var graphService = new GraphApiService(_mockGraphLogger, _mockExecutor, handler, tokenProvider: null); + var graphService = new GraphApiService(_mockGraphLogger, _mockExecutor, Substitute.For(), handler, tokenProvider: null); var service = new AgentBlueprintService(_mockLogger, graphService); const string tenantId = "12345678-1234-1234-1234-123456789012"; @@ -441,6 +440,15 @@ public async Task DeleteAgentUserAsync_ReturnsFalse_OnGraphError() } } + private static IAuthenticationService FakeAuth() + { + var mock = Substitute.For(); + mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("fake-token")); + return mock; + } + private (AgentBlueprintService service, FakeHttpMessageHandler handler) CreateServiceWithFakeHandler() { var handler = new FakeHttpMessageHandler(); @@ -456,7 +464,7 @@ public async Task DeleteAgentUserAsync_ReturnsFalse_OnGraphError() // Pass a no-op login hint resolver to skip the real 'az account show' process spawned by // AzCliHelper.ResolveLoginHintAsync — that static call bypasses the mocked CommandExecutor // and causes each test to wait several seconds for the real az CLI. - var graphService = new GraphApiService(_mockGraphLogger, executor, handler, _mockTokenProvider, + var graphService = new GraphApiService(_mockGraphLogger, executor, Substitute.For(), handler, _mockTokenProvider, loginHintResolver: () => Task.FromResult(null)); return (new AgentBlueprintService(_mockLogger, graphService), handler); } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs index 5ec9f0f3..c003ff7d 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs @@ -7,6 +7,7 @@ using Microsoft.Agents.A365.DevTools.Cli.Services; using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; using Xunit; namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; @@ -14,10 +15,8 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; /// /// Unit tests for ArmApiService. /// Uses TestHttpMessageHandler (defined in GraphApiServiceTests.cs, same assembly) -/// to inject fake HTTP responses. The AzCliHelper process-level token cache is -/// pre-warmed in the constructor so no real az subprocess is spawned. +/// to inject fake HTTP responses. /// -[Collection("AzCliTokenCache")] public class ArmApiServiceTests { private const string TenantId = "tid"; @@ -27,14 +26,17 @@ public class ArmApiServiceTests private const string WebAppName = "webapp-test"; private const string UserObjectId = "user-obj-id"; - public ArmApiServiceTests() + private static IAuthenticationService FakeAuth() { - AzCliHelper.ResetAzCliTokenCacheForTesting(); - AzCliHelper.WarmAzCliTokenCache(ArmApiService.ArmResource, TenantId, "fake-arm-token"); + var mock = Substitute.For(); + mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("fake-arm-token")); + return mock; } private static ArmApiService CreateService(HttpMessageHandler handler) => - new ArmApiService(NullLogger.Instance, handler); + new ArmApiService(NullLogger.Instance, FakeAuth(), handler, retryHelper: new RetryHelper(NullLogger.Instance, maxRetries: 1, baseDelaySeconds: 0)); // ──────────────────────────── ResourceGroupExistsAsync ──────────────────────────── diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AuthenticationServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AuthenticationServiceTests.cs index 6a040653..4d29a9ec 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AuthenticationServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AuthenticationServiceTests.cs @@ -979,4 +979,81 @@ await act.Should().ThrowAsync( } #endregion + + #region ResolveLoginHintFromCacheAsync + + private static string BuildJwt(object payload) + { + var json = System.Text.Json.JsonSerializer.Serialize(payload); + var payloadB64 = Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes(json)).TrimEnd('='); + return $"header.{payloadB64}.signature"; + } + + private void WriteTokenCache(string accessToken) + { + var cacheDir = Path.GetDirectoryName(_testCachePath)!; + Directory.CreateDirectory(cacheDir); + var cache = new + { + Tokens = new Dictionary + { + ["key"] = new { AccessToken = accessToken, ExpiresOn = DateTime.UtcNow.AddHours(1), TenantId = "tid" } + } + }; + File.WriteAllText(_testCachePath, System.Text.Json.JsonSerializer.Serialize(cache)); + } + + [Fact] + public async Task ResolveLoginHintFromCacheAsync_WhenCacheHasUpnClaim_ReturnsUpn() + { + WriteTokenCache(BuildJwt(new { upn = "user@test.com" })); + + var result = await _authService.ResolveLoginHintFromCacheAsync(); + + result.Should().Be("user@test.com", + because: "the upn claim in the cached JWT should be returned as the login hint"); + } + + [Fact] + public async Task ResolveLoginHintFromCacheAsync_WhenCacheHasPreferredUsernameClaim_ReturnsIt() + { + WriteTokenCache(BuildJwt(new { preferred_username = "user@contoso.com" })); + + var result = await _authService.ResolveLoginHintFromCacheAsync(); + + result.Should().Be("user@contoso.com", + because: "preferred_username is the fallback claim when upn is absent"); + } + + [Fact] + public async Task ResolveLoginHintFromCacheAsync_WhenCacheFileDoesNotExist_ReturnsNull() + { + _authService.ClearCache(); + + var result = await _authService.ResolveLoginHintFromCacheAsync(); + + result.Should().BeNull(because: "no cache file means no login hint can be resolved"); + } + + [Fact] + public async Task ResolveLoginHintFromCacheAsync_WhenJwtHasNoUpnClaims_ReturnsNull() + { + WriteTokenCache(BuildJwt(new { sub = "some-subject", oid = "some-oid" })); + + var result = await _authService.ResolveLoginHintFromCacheAsync(); + + result.Should().BeNull(because: "a JWT without upn or preferred_username cannot provide a login hint"); + } + + [Fact] + public async Task ResolveLoginHintFromCacheAsync_WhenJwtIsMalformed_ReturnsNull() + { + WriteTokenCache("not-a-valid-jwt"); + + var result = await _authService.ResolveLoginHintFromCacheAsync(); + + result.Should().BeNull(because: "a malformed JWT should be swallowed and null returned"); + } + + #endregion } 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..3da5c7de 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 @@ -6,6 +6,7 @@ using Microsoft.Agents.A365.DevTools.Cli.Exceptions; using Microsoft.Agents.A365.DevTools.Cli.Services; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using System.Text.Json; using Xunit; diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceAddRequiredResourceAccessTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceAddRequiredResourceAccessTests.cs index 909846c4..c8495c70 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceAddRequiredResourceAccessTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceAddRequiredResourceAccessTests.cs @@ -5,20 +5,18 @@ using System.Text.Json; using FluentAssertions; using Microsoft.Agents.A365.DevTools.Cli.Services; -using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using Xunit; namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; /// -/// Isolated from other tests because AzCliHelper token cache is static state. -/// Without isolation, parallel tests calling ResetAzCliTokenCacheForTesting() clear -/// the warmup and cause real az subprocess spawns (~20s per test). +/// Tests for AgentBlueprintService.AddRequiredResourceAccessAsync. /// [Collection("AgentBlueprintServiceAddRequiredResourceAccessTests")] -public class AgentBlueprintServiceAddRequiredResourceAccessTests : IDisposable +public class AgentBlueprintServiceAddRequiredResourceAccessTests { private const string TenantId = "test-tenant-id"; private const string AppId = "test-app-id"; @@ -26,14 +24,6 @@ public class AgentBlueprintServiceAddRequiredResourceAccessTests : IDisposable private const string ObjectId = "object-id-123"; private const string SpObjectId = "sp-object-id-456"; - public AgentBlueprintServiceAddRequiredResourceAccessTests() - { - AzCliHelper.ResetAzCliTokenCacheForTesting(); - AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", TenantId, "fake-graph-token"); - } - - public void Dispose() => AzCliHelper.ResetAzCliTokenCacheForTesting(); - [Fact] public async Task AddRequiredResourceAccessAsync_Success_WithValidPermissionIds() { @@ -42,7 +32,7 @@ public async Task AddRequiredResourceAccessAsync_Success_WithValidPermissionIds( var graphLogger = Substitute.For>(); var blueprintLogger = Substitute.For>(); var executor = CreateMockExecutor(); - var graphService = new GraphApiService(graphLogger, executor, handler); + var graphService = new GraphApiService(graphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(blueprintLogger, graphService); // Queue responses @@ -70,7 +60,7 @@ public async Task AddRequiredResourceAccessAsync_HandlesNullPermissionId_Gracefu var graphLogger = Substitute.For>(); var blueprintLogger = Substitute.For>(); var executor = CreateMockExecutor(); - var graphService = new GraphApiService(graphLogger, executor, handler); + var graphService = new GraphApiService(graphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(blueprintLogger, graphService); // Queue responses @@ -97,7 +87,7 @@ public async Task AddRequiredResourceAccessAsync_FailsWhen_ApplicationNotFound() var graphLogger = Substitute.For>(); var blueprintLogger = Substitute.For>(); var executor = CreateMockExecutor(); - var graphService = new GraphApiService(graphLogger, executor, handler); + var graphService = new GraphApiService(graphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(blueprintLogger, graphService); // Queue empty application response @@ -125,7 +115,7 @@ public async Task AddRequiredResourceAccessAsync_FailsWhen_ApplicationIdIsNull() var graphLogger = Substitute.For>(); var blueprintLogger = Substitute.For>(); var executor = CreateMockExecutor(); - var graphService = new GraphApiService(graphLogger, executor, handler); + var graphService = new GraphApiService(graphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(blueprintLogger, graphService); // Queue application response with null id @@ -150,7 +140,7 @@ public async Task AddRequiredResourceAccessAsync_SkipsInvalidScopes_AndProcesses var graphLogger = Substitute.For>(); var blueprintLogger = Substitute.For>(); var executor = CreateMockExecutor(); - var graphService = new GraphApiService(graphLogger, executor, handler); + var graphService = new GraphApiService(graphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(blueprintLogger, graphService); // Queue responses @@ -192,7 +182,7 @@ public async Task AddRequiredResourceAccessAsync_MergesWithExisting_Permissions( var graphLogger = Substitute.For>(); var blueprintLogger = Substitute.For>(); var executor = CreateMockExecutor(); - var graphService = new GraphApiService(graphLogger, executor, handler); + var graphService = new GraphApiService(graphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(blueprintLogger, graphService); // Application with existing requiredResourceAccess @@ -238,6 +228,15 @@ public async Task AddRequiredResourceAccessAsync_MergesWithExisting_Permissions( result.Should().BeTrue(); } + private static IAuthenticationService FakeAuth() + { + var mock = Substitute.For(); + mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("fake-token")); + return mock; + } + private static CommandExecutor CreateMockExecutor() { var executor = Substitute.For(Substitute.For>()); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceIsApplicationOwnerTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceIsApplicationOwnerTests.cs index 9ae8d3b1..8de6a56e 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceIsApplicationOwnerTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceIsApplicationOwnerTests.cs @@ -6,7 +6,6 @@ using System.Text.Json; using FluentAssertions; using Microsoft.Agents.A365.DevTools.Cli.Services; -using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; using Microsoft.Extensions.Logging; using NSubstitute; using Xunit; @@ -23,32 +22,25 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; /// all queued responses in their Dispose methods. Suppressing CA2000 for this pattern. /// #pragma warning disable CA2000 // Dispose objects before losing scope -[Collection("AzCliTokenCache")] public class GraphApiServiceIsApplicationOwnerTests { private readonly ILogger _mockLogger; private readonly CommandExecutor _mockExecutor; + private static IAuthenticationService FakeAuth() + { + var mock = Substitute.For(); + mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("fake-token")); + return mock; + } + public GraphApiServiceIsApplicationOwnerTests() { _mockLogger = Substitute.For>(); var mockExecutorLogger = Substitute.For>(); _mockExecutor = Substitute.ForPartsOf(mockExecutorLogger); - AzCliHelper.ResetAzCliTokenCacheForTesting(); - AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "tenant-123", "fake-graph-token"); - - // Mock Azure CLI authentication - _mockExecutor.ExecuteAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) - .Returns(callInfo => - { - var cmd = callInfo.ArgAt(0); - var args = callInfo.ArgAt(1); - if (cmd == "az" && args != null && args.StartsWith("account show", StringComparison.OrdinalIgnoreCase)) - return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "{}", StandardError = string.Empty }); - if (cmd == "az" && args != null && args.Contains("get-access-token", StringComparison.OrdinalIgnoreCase)) - return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "fake-token", StandardError = string.Empty }); - return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); - }); } [Fact] @@ -56,7 +48,7 @@ public async Task IsApplicationOwnerAsync_WhenUserIsOwner_ReturnsTrue() { // Arrange using var handler = new TestHttpMessageHandler(); - var service = new GraphApiService(_mockLogger, _mockExecutor, handler); + var service = new GraphApiService(_mockLogger, _mockExecutor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var tenantId = "tenant-123"; var appObjectId = "app-obj-456"; @@ -88,7 +80,7 @@ public async Task IsApplicationOwnerAsync_WhenUserIsNotOwner_ReturnsFalse() { // Arrange using var handler = new TestHttpMessageHandler(); - var service = new GraphApiService(_mockLogger, _mockExecutor, handler); + var service = new GraphApiService(_mockLogger, _mockExecutor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var tenantId = "tenant-123"; var appObjectId = "app-obj-456"; @@ -120,7 +112,7 @@ public async Task IsApplicationOwnerAsync_WhenNoOwners_ReturnsFalse() { // Arrange using var handler = new TestHttpMessageHandler(); - var service = new GraphApiService(_mockLogger, _mockExecutor, handler); + var service = new GraphApiService(_mockLogger, _mockExecutor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var tenantId = "tenant-123"; var appObjectId = "app-obj-456"; @@ -152,7 +144,7 @@ public async Task IsApplicationOwnerAsync_WithoutUserObjectId_RetrievesCurrentUs capturedMeRequest = req; } }); - var service = new GraphApiService(_mockLogger, _mockExecutor, handler); + var service = new GraphApiService(_mockLogger, _mockExecutor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var tenantId = "tenant-123"; var appObjectId = "app-obj-456"; @@ -191,7 +183,7 @@ public async Task IsApplicationOwnerAsync_WhenGetCurrentUserFails_ReturnsFalse() { // Arrange using var handler = new TestHttpMessageHandler(); - var service = new GraphApiService(_mockLogger, _mockExecutor, handler); + var service = new GraphApiService(_mockLogger, _mockExecutor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var tenantId = "tenant-123"; var appObjectId = "app-obj-456"; @@ -214,7 +206,7 @@ public async Task IsApplicationOwnerAsync_IsCaseInsensitiveForUserIdComparison() { // Arrange using var handler = new TestHttpMessageHandler(); - var service = new GraphApiService(_mockLogger, _mockExecutor, handler); + var service = new GraphApiService(_mockLogger, _mockExecutor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var tenantId = "tenant-123"; var appObjectId = "app-obj-456"; @@ -245,7 +237,7 @@ public async Task IsApplicationOwnerAsync_WhenGraphApiCallFails_ReturnsFalse() { // Arrange using var handler = new TestHttpMessageHandler(); - var service = new GraphApiService(_mockLogger, _mockExecutor, handler); + var service = new GraphApiService(_mockLogger, _mockExecutor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var tenantId = "tenant-123"; var appObjectId = "app-obj-456"; @@ -276,7 +268,7 @@ public async Task IsApplicationOwnerAsync_UsesV1Endpoint() capturedOwnersRequest = req; } }); - var service = new GraphApiService(_mockLogger, _mockExecutor, handler); + var service = new GraphApiService(_mockLogger, _mockExecutor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var tenantId = "tenant-123"; var appObjectId = "app-obj-456"; diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs index 7866a283..d90de8b9 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs @@ -9,12 +9,12 @@ using Microsoft.Agents.A365.DevTools.Cli.Services; using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using Xunit; namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; -[Collection("AzCliTokenCache")] public class GraphApiServiceTests { private readonly ILogger _mockLogger; @@ -27,9 +27,6 @@ public GraphApiServiceTests() var mockExecutorLogger = Substitute.For>(); _mockExecutor = Substitute.ForPartsOf(mockExecutorLogger); _mockTokenProvider = Substitute.For(); - AzCliHelper.ResetAzCliTokenCacheForTesting(); - AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "tenant-123", "fake-graph-token"); - AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "tid", "fake-graph-token"); } @@ -53,7 +50,7 @@ public async Task GraphPostWithResponseAsync_Returns_Success_And_ParsesJson() return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); // Queue successful POST with JSON body var bodyObj = new { result = "ok" }; @@ -94,7 +91,7 @@ public async Task GraphPostWithResponseAsync_Returns_Failure_With_Body() return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); // Queue failing POST with JSON error body var errorBody = new { error = new { code = "Authorization_RequestDenied", message = "Insufficient privileges" } }; @@ -162,7 +159,7 @@ public async Task LookupServicePrincipalAsync_DoesNotIncludeConsistencyLevelHead }); // Create GraphApiService with our capturing handler - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); // Queue response for service principal lookup var spResponse = new { value = new[] { new { id = "sp-object-id-123", appId = "blueprint-456" } } }; @@ -244,7 +241,7 @@ public async Task GraphGetAsync_SanitizesTokenWithNewlineCharacters(string token return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); // Queue a successful response using var queuedResponse = new HttpResponseMessage(HttpStatusCode.OK) @@ -293,7 +290,7 @@ public async Task GraphGetAsync_TokenFromTokenProvider_SanitizesNewlines() Arg.Any()) .Returns("token-from-provider\r\nwith-embedded-newlines\n"); - var service = new GraphApiService(logger, executor, handler, tokenProvider, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, FakeAuth(), handler, tokenProvider, loginHintResolver: () => Task.FromResult(null)); // Queue a successful response using var queuedResponse = new HttpResponseMessage(HttpStatusCode.OK) @@ -318,15 +315,9 @@ public async Task GraphGetAsync_TokenFromTokenProvider_SanitizesNewlines() [Fact] public async Task CheckServicePrincipalCreationPrivilegesAsync_SanitizesTokenWithNewlines() { - // This test verifies that CheckServicePrincipalCreationPrivilegesAsync also - // sanitizes tokens with newlines. This method has its own token handling code - // separate from EnsureGraphHeadersAsync. - - // Overwrite the "fake-graph-token" warmed in the constructor with a token that has - // embedded newlines. GetGraphAccessTokenAsync returns it from the process-level cache; - // CheckServicePrincipalCreationPrivilegesAsync must trim it before using it in the - // Authorization header. Warming directly avoids spawning a real az subprocess. - AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "tenant-123", "privileges-check-token\r\n\n"); + // This test verifies that CheckServicePrincipalCreationPrivilegesAsync sanitizes + // tokens with embedded whitespace before setting the Authorization header. + // Token acquisition now goes through IAuthenticationService (MSAL), not az CLI. // Arrange HttpRequestMessage? capturedRequest = null; @@ -334,38 +325,13 @@ public async Task CheckServicePrincipalCreationPrivilegesAsync_SanitizesTokenWit var logger = Substitute.For>(); var executor = Substitute.For(Substitute.For>()); - // Mock az CLI to return a token WITH newline characters - executor.ExecuteAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) - .Returns(callInfo => - { - var cmd = callInfo.ArgAt(0); - var args = callInfo.ArgAt(1); + // Auth service returns a token WITH embedded newlines — simulates a whitespace-padded token. + var authWithNewlines = Substitute.For(); + authWithNewlines.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("privileges-check-token\r\n\n")); - if (cmd == "az" && args != null && args.StartsWith("account show", StringComparison.OrdinalIgnoreCase)) - { - return Task.FromResult(new CommandResult - { - ExitCode = 0, - StandardOutput = "{}", - StandardError = string.Empty - }); - } - - if (cmd == "az" && args != null && args.Contains("get-access-token", StringComparison.OrdinalIgnoreCase)) - { - // Return token WITH embedded newlines - return Task.FromResult(new CommandResult - { - ExitCode = 0, - StandardOutput = "privileges-check-token\r\n\n", - StandardError = string.Empty - }); - } - - return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); - }); - - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, authWithNewlines, handler, loginHintResolver: () => Task.FromResult(null)); // Queue a successful response for the directory roles query using var queuedResponse = new HttpResponseMessage(HttpStatusCode.OK) @@ -415,7 +381,7 @@ public async Task GetServicePrincipalDisplayNameAsync_SuccessfulLookup_ReturnsDi return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); // Queue successful response with Microsoft Graph service principal var spResponse = new { value = new[] { new { displayName = "Microsoft Graph" } } }; @@ -451,7 +417,7 @@ public async Task GetServicePrincipalDisplayNameAsync_ServicePrincipalNotFound_R return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); // Queue response with empty array (service principal not found) var spResponse = new { value = Array.Empty() }; @@ -487,7 +453,7 @@ public async Task GetServicePrincipalDisplayNameAsync_NullResponse_ReturnsNull() return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); // Queue error response (simulating network error or Graph API error) handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.InternalServerError) @@ -522,7 +488,7 @@ public async Task GetServicePrincipalDisplayNameAsync_MissingDisplayNameProperty return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); // Queue response with malformed object (missing displayName) var spResponse = new { value = new[] { new { id = "sp-id-123", appId = "00000003-0000-0000-c000-000000000000" } } }; @@ -608,6 +574,15 @@ public async Task IsCurrentUserAdminAsync_GraphFails_ReturnsUnknown() #region IsCurrentUserAgentIdAdminAsync + private static IAuthenticationService FakeAuth() + { + var mock = Substitute.For(); + mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("fake-token")); + return mock; + } + private static GraphApiService CreateServiceWithTokenProvider(TestHttpMessageHandler handler) { var logger = Substitute.For>(); @@ -617,7 +592,7 @@ private static GraphApiService CreateServiceWithTokenProvider(TestHttpMessageHan Arg.Any(), Arg.Any>(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns("fake-token"); - return new GraphApiService(logger, executor, handler, tokenProvider, loginHintResolver: () => Task.FromResult(null)); + return new GraphApiService(logger, executor, FakeAuth(), handler, tokenProvider, loginHintResolver: () => Task.FromResult(null), retryHelper: new RetryHelper(NullLogger.Instance, maxRetries: 1, baseDelaySeconds: 0)); } [Fact] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTokenCacheTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTokenCacheTests.cs deleted file mode 100644 index 93e9d9da..00000000 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTokenCacheTests.cs +++ /dev/null @@ -1,219 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using System.Net; -using System.Net.Http; -using FluentAssertions; -using Microsoft.Agents.A365.DevTools.Cli.Services; -using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; -using Microsoft.Extensions.Logging; -using NSubstitute; -using Xunit; - -namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; - -/// -/// Tests to validate that Azure CLI Graph tokens are cached at the process level -/// (via AzCliHelper) so a single CLI invocation only spawns one 'az' subprocess -/// per (resource, tenantId) pair, regardless of how many GraphApiService instances -/// or callers request the same token. -/// -[Collection("GraphApiServiceTokenCacheTests")] -public class GraphApiServiceTokenCacheTests : IDisposable -{ - public GraphApiServiceTokenCacheTests() - { - AzCliHelper.AzCliTokenAcquirerOverride = null; - AzCliHelper.ResetAzCliTokenCacheForTesting(); - } - - public void Dispose() - { - AzCliHelper.AzCliTokenAcquirerOverride = null; - AzCliHelper.ResetAzCliTokenCacheForTesting(); - } - - /// - /// Sets the process-level token acquirer override and returns a counter reference. - /// The override is invoked inside GetOrAdd, so the cache still applies — only one - /// invocation per (resource, tenantId) key within a test. - /// - private static int[] SetupTokenAcquirerWithCounter(string token = "cached-token") - { - var callCount = new int[1]; - AzCliHelper.AzCliTokenAcquirerOverride = (resource, tenantId) => - { - callCount[0]++; - return Task.FromResult(token); - }; - return callCount; - } - - private static (GraphApiService service, TestHttpMessageHandler handler) CreateService() - { - var handler = new TestHttpMessageHandler(); - var logger = Substitute.For>(); - var executor = Substitute.For(Substitute.For>()); - - // 'az account show' is still used in GetGraphAccessTokenAsync for the auth-check - // fallback path; stub it to succeed so tests that hit the fallback don't hang. - executor.ExecuteAsync( - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any()) - .Returns(callInfo => - { - var args = callInfo.ArgAt(1); - if (args != null && args.StartsWith("account show", StringComparison.OrdinalIgnoreCase)) - return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "{}", StandardError = string.Empty }); - return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); - }); - - var service = new GraphApiService(logger, executor, handler); - return (service, handler); - } - - [Fact] - public async Task MultipleGraphGetAsync_SameTenant_AcquiresTokenOnlyOnce() - { - var callCount = SetupTokenAcquirerWithCounter(); - var (service, handler) = CreateService(); - - try - { - for (int i = 0; i < 3; i++) - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent("{\"value\":[]}") }); - - await service.GraphGetAsync("tenant-1", "/v1.0/path1"); - await service.GraphGetAsync("tenant-1", "/v1.0/path2"); - await service.GraphGetAsync("tenant-1", "/v1.0/path3"); - - callCount[0].Should().Be(1, - because: "the process-level cache must serve the same (resource, tenant) token " + - "from the first acquisition — re-running az account get-access-token on every " + - "Graph call within a single command costs 20-40s per call"); - } - finally { handler.Dispose(); } - } - - [Fact] - public async Task GraphGetAsync_DifferentTenants_AcquiresTokenForEach() - { - var callCount = SetupTokenAcquirerWithCounter(); - var (service, handler) = CreateService(); - - try - { - for (int i = 0; i < 2; i++) - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent("{\"value\":[]}") }); - - await service.GraphGetAsync("tenant-1", "/v1.0/path1"); - await service.GraphGetAsync("tenant-2", "/v1.0/path2"); - - callCount[0].Should().Be(2, - because: "different tenant IDs are different cache keys — each tenant requires " + - "its own 'az account get-access-token --tenant' call"); - } - finally { handler.Dispose(); } - } - - [Fact] - public async Task MixedGraphOperations_SameTenant_AcquiresTokenOnlyOnce() - { - var callCount = SetupTokenAcquirerWithCounter(); - var (service, handler) = CreateService(); - - try - { - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent("{\"value\":[]}") }); - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent("{\"id\":\"123\"}") }); - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent("{\"value\":[]}") }); - - await service.GraphGetAsync("tenant-1", "/v1.0/path1"); - await service.GraphPostAsync("tenant-1", "/v1.0/path2", new { name = "test" }); - await service.GraphGetAsync("tenant-1", "/v1.0/path3"); - - callCount[0].Should().Be(1, - because: "GET and POST operations share the same process-level token cache — " + - "mixed Graph operations within a command must not each re-acquire a token"); - } - finally { handler.Dispose(); } - } - - [Fact] - public async Task MultipleGraphApiServiceInstances_SameTenant_AcquireTokenOnlyOnce() - { - // This is the key regression scenario: previously, each GraphApiService instance had - // its own instance-level cache, so a new instance in each setup phase would re-run - // 'az account get-access-token'. With a process-level cache, all instances share one token. - var callCount = SetupTokenAcquirerWithCounter(); - - var handler1 = new TestHttpMessageHandler(); - var handler2 = new TestHttpMessageHandler(); - - try - { - handler1.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent("{\"value\":[]}") }); - handler2.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent("{\"value\":[]}") }); - - var logger = Substitute.For>(); - var executor = Substitute.For(Substitute.For>()); - executor.ExecuteAsync(Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any()) - .Returns(Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = "{}", StandardError = string.Empty })); - - var service1 = new GraphApiService(logger, executor, handler1); - var service2 = new GraphApiService(logger, executor, handler2); - - await service1.GraphGetAsync("tenant-1", "/v1.0/path1"); - await service2.GraphGetAsync("tenant-1", "/v1.0/path1"); - - callCount[0].Should().Be(1, - because: "the process-level cache is shared across all GraphApiService instances — " + - "a second instance must not re-run 'az account get-access-token' for the same tenant"); - } - finally - { - handler1.Dispose(); - handler2.Dispose(); - } - } - - [Fact] - public async Task GraphGetAsync_AfterCacheInvalidation_AcquiresNewToken() - { - // Validates that InvalidateAzCliTokenCache() forces fresh token acquisition — - // used by ClientAppValidator and DelegatedConsentService after az login/CAE events. - var callCount = SetupTokenAcquirerWithCounter(); - var (service, handler) = CreateService(); - - try - { - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent("{\"value\":[]}") }); - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent("{\"value\":[]}") }); - - await service.GraphGetAsync("tenant-1", "/v1.0/path1"); - - // Simulate a CAE event or forced re-auth that invalidates all cached tokens - AzCliHelper.InvalidateAzCliTokenCache(); - - await service.GraphGetAsync("tenant-1", "/v1.0/path2"); - - callCount[0].Should().Be(2, - because: "InvalidateAzCliTokenCache clears the process-level cache — " + - "the next call must re-acquire a fresh token (e.g., after CAE revocation or az login)"); - } - finally { handler.Dispose(); } - } -} - -[CollectionDefinition("GraphApiServiceTokenCacheTests", DisableParallelization = true)] -public class GraphApiServiceTokenCacheTestCollection { } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTokenTrimTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTokenTrimTests.cs index ec07a71b..80a2b253 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTokenTrimTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTokenTrimTests.cs @@ -5,8 +5,8 @@ using System.Net.Http; using FluentAssertions; using Microsoft.Agents.A365.DevTools.Cli.Services; -using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using Xunit; @@ -20,9 +20,6 @@ public class GraphApiServiceTokenTrimTests { public GraphApiServiceTokenTrimTests() { - // Pre-warm the process-level token cache with a token that includes a newline so - // EnsureGraphHeadersAsync reads from cache and the trimming at line 256 is exercised. - AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "tid", "fake-graph-token\n"); } [Theory] @@ -52,7 +49,11 @@ public async Task EnsureGraphHeadersAsync_TrimsNewlineCharactersFromToken(string return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var mockAuth = Substitute.For(); + mockAuth.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(tokenWithWhitespace)); + var service = new GraphApiService(logger, executor, mockAuth, handler, loginHintResolver: () => Task.FromResult(null)); // Queue successful GET response using var response = new HttpResponseMessage(HttpStatusCode.OK) @@ -87,7 +88,7 @@ public async Task EnsureGraphHeadersAsync_WithTokenProvider_TrimsNewlineCharacte Arg.Any()) .Returns("fake-token\n"); - var service = new GraphApiService(logger, executor, handler, tokenProvider, loginHintResolver: () => Task.FromResult(null)); + var service = new GraphApiService(logger, executor, Substitute.For(), handler, tokenProvider, loginHintResolver: () => Task.FromResult(null)); // Queue successful GET response using var response = new HttpResponseMessage(HttpStatusCode.OK) @@ -123,7 +124,11 @@ public async Task CheckServicePrincipalCreationPrivilegesAsync_TrimsNewlineChara return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var service = new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult(null)); + var mockAuth = Substitute.For(); + mockAuth.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("fake-token\r\n")); + var service = new GraphApiService(logger, executor, mockAuth, handler, loginHintResolver: () => Task.FromResult(null)); // Queue successful response for directory roles using var response = new HttpResponseMessage(HttpStatusCode.OK) diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceVerifyInheritablePermissionsTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceVerifyInheritablePermissionsTests.cs index ee4b32a5..10b31b51 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceVerifyInheritablePermissionsTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceVerifyInheritablePermissionsTests.cs @@ -5,8 +5,8 @@ using System.Text.Json; using FluentAssertions; using Microsoft.Agents.A365.DevTools.Cli.Services; -using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using Xunit; @@ -14,9 +14,13 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; public class AgentBlueprintServiceVerifyInheritablePermissionsTests { - public AgentBlueprintServiceVerifyInheritablePermissionsTests() + private static IAuthenticationService FakeAuth() { - AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "tid", "fake-graph-token"); + var mock = Substitute.For(); + mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult("fake-token")); + return mock; } [Fact] @@ -40,7 +44,7 @@ public async Task VerifyInheritablePermissionsAsync_PermissionsExist_ReturnsScop return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var graphService = new GraphApiService(graphLogger, executor, handler); + var graphService = new GraphApiService(graphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(blueprintLogger, graphService); var response = new @@ -103,7 +107,7 @@ public async Task VerifyInheritablePermissionsAsync_PermissionsNotFound_ReturnsF return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var graphService = new GraphApiService(graphLogger, executor, handler); + var graphService = new GraphApiService(graphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(blueprintLogger, graphService); var response = new @@ -163,7 +167,7 @@ public async Task VerifyInheritablePermissionsAsync_ApiFailure_ReturnsError() return Task.FromResult(new CommandResult { ExitCode = 0, StandardOutput = string.Empty, StandardError = string.Empty }); }); - var graphService = new GraphApiService(graphLogger, executor, handler); + var graphService = new GraphApiService(graphLogger, executor, FakeAuth(), handler, loginHintResolver: () => Task.FromResult(null)); var service = new AgentBlueprintService(blueprintLogger, graphService); // Simulate 404 Not Found to trigger API failure path diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/AzCliHelperTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/AzCliHelperTests.cs index 29379277..429cd7d7 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/AzCliHelperTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/AzCliHelperTests.cs @@ -16,11 +16,9 @@ public class AzCliHelperTests : IDisposable { public AzCliHelperTests() { - // Start each test with a clean slate — both static caches + // Start each test with a clean slate AzCliHelper.LoginHintResolverOverride = null; AzCliHelper.ResetLoginHintCacheForTesting(); - AzCliHelper.AzCliTokenAcquirerOverride = null; - AzCliHelper.ResetAzCliTokenCacheForTesting(); } public void Dispose() @@ -28,8 +26,6 @@ public void Dispose() // Restore static state so other tests are not affected AzCliHelper.LoginHintResolverOverride = null; AzCliHelper.ResetLoginHintCacheForTesting(); - AzCliHelper.AzCliTokenAcquirerOverride = null; - AzCliHelper.ResetAzCliTokenCacheForTesting(); } [Fact] @@ -102,96 +98,6 @@ public async Task ResolveLoginHintAsync_AfterCacheReset_InvokesResolverAgain() callCount.Should().Be(2, because: "ResetLoginHintCacheForTesting clears the cache, forcing a fresh resolve — required for test isolation"); } - - // ------------------------------------------------------------------------- - // AcquireAzCliTokenAsync — process-level token cache - // ------------------------------------------------------------------------- - - [Fact] - public async Task AcquireAzCliTokenAsync_WhenOverrideSet_ReturnsOverrideValue() - { - AzCliHelper.AzCliTokenAcquirerOverride = (_, __) => Task.FromResult("test-token"); - - var result = await AzCliHelper.AcquireAzCliTokenAsync("https://graph.microsoft.com/", "tenant-1"); - - result.Should().Be("test-token", - because: "the override replaces the real az subprocess — used in tests to inject known tokens"); - } - - [Fact] - public async Task AcquireAzCliTokenAsync_CalledTwiceSameKey_InvokesAcquirerOnce() - { - var callCount = 0; - AzCliHelper.AzCliTokenAcquirerOverride = (_, __) => - { - callCount++; - return Task.FromResult("shared-token"); - }; - - await AzCliHelper.AcquireAzCliTokenAsync("https://graph.microsoft.com/", "tenant-1"); - await AzCliHelper.AcquireAzCliTokenAsync("https://graph.microsoft.com/", "tenant-1"); - - callCount.Should().Be(1, - because: "the process-level cache must serve the same (resource, tenant) token " + - "after the first acquisition — calling az account get-access-token on every " + - "request costs 20-40s per call"); - } - - [Fact] - public async Task AcquireAzCliTokenAsync_DifferentTenants_InvokesAcquirerForEach() - { - var callCount = 0; - AzCliHelper.AzCliTokenAcquirerOverride = (_, __) => - { - callCount++; - return Task.FromResult("token"); - }; - - await AzCliHelper.AcquireAzCliTokenAsync("https://graph.microsoft.com/", "tenant-1"); - await AzCliHelper.AcquireAzCliTokenAsync("https://graph.microsoft.com/", "tenant-2"); - - callCount.Should().Be(2, - because: "different tenant IDs are different cache keys — each tenant requires its own token"); - } - - [Fact] - public async Task AcquireAzCliTokenAsync_AfterInvalidation_InvokesAcquirerAgain() - { - var callCount = 0; - AzCliHelper.AzCliTokenAcquirerOverride = (_, __) => - { - callCount++; - return Task.FromResult("token"); - }; - - await AzCliHelper.AcquireAzCliTokenAsync("https://graph.microsoft.com/", "tenant-1"); - AzCliHelper.InvalidateAzCliTokenCache(); - await AzCliHelper.AcquireAzCliTokenAsync("https://graph.microsoft.com/", "tenant-1"); - - callCount.Should().Be(2, - because: "InvalidateAzCliTokenCache clears the cache — the next call must re-acquire " + - "a fresh token; this is required after 'az login' or a CAE token revocation event"); - } - - [Fact] - public async Task WarmAzCliTokenCache_InjectedToken_ReturnedOnNextCall() - { - // Override that always fails — should NOT be called after warming the cache - AzCliHelper.AzCliTokenAcquirerOverride = (_, __) => - Task.FromResult(null); - - AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "tenant-1", "warmed-token"); - - // The warmup bypasses the GetOrAdd — the cache entry is set directly. - // Reset override so we can verify the warmed value is returned, not re-acquired. - AzCliHelper.AzCliTokenAcquirerOverride = null; - var result = await AzCliHelper.AcquireAzCliTokenAsync("https://graph.microsoft.com/", "tenant-1"); - - result.Should().Be("warmed-token", - because: "WarmAzCliTokenCache injects a token acquired via auth recovery into the " + - "process-level cache — subsequent callers must receive the injected token " + - "without re-running az account get-access-token"); - } } [CollectionDefinition("AzCliHelperTests", DisableParallelization = true)] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/NetworkHelperTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/NetworkHelperTests.cs new file mode 100644 index 00000000..820cad27 --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/NetworkHelperTests.cs @@ -0,0 +1,60 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Net.Sockets; +using FluentAssertions; +using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services.Helpers; + +public class NetworkHelperTests +{ + [Fact] + public void IsConnectionResetByProxy_WhenDirectSocketException_ReturnsTrue() + { + var ex = new SocketException((int)SocketError.ConnectionReset); + + NetworkHelper.IsConnectionResetByProxy(ex).Should().BeTrue( + because: "a direct SocketException with ConnectionReset is a proxy-reset signal"); + } + + [Fact] + public void IsConnectionResetByProxy_WhenHttpRequestExceptionWrapsConnectionReset_ReturnsTrue() + { + var inner = new SocketException((int)SocketError.ConnectionReset); + var ex = new HttpRequestException("Connection reset", inner); + + NetworkHelper.IsConnectionResetByProxy(ex).Should().BeTrue( + because: "HttpRequestException wrapping a ConnectionReset SocketException is the typical TLS proxy reset pattern"); + } + + [Fact] + public void IsConnectionResetByProxy_WhenSocketExceptionIsNotConnectionReset_ReturnsFalse() + { + var ex = new SocketException((int)SocketError.TimedOut); + + NetworkHelper.IsConnectionResetByProxy(ex).Should().BeFalse( + because: "only SocketError.ConnectionReset (10054) indicates a proxy reset — other socket errors should not be misclassified"); + } + + [Fact] + public void IsConnectionResetByProxy_WhenNoSocketExceptionInChain_ReturnsFalse() + { + var ex = new InvalidOperationException("some other error"); + + NetworkHelper.IsConnectionResetByProxy(ex).Should().BeFalse( + because: "exceptions unrelated to socket errors are not proxy resets"); + } + + [Fact] + public void IsConnectionResetByProxy_WhenDeeplyNestedConnectionReset_ReturnsTrue() + { + var socketEx = new SocketException((int)SocketError.ConnectionReset); + var mid = new IOException("IO error", socketEx); + var outer = new HttpRequestException("outer", mid); + + NetworkHelper.IsConnectionResetByProxy(outer).Should().BeTrue( + because: "the chain walk must find SocketException at any nesting depth"); + } +} From abd2a636f53a5d59dcfc4196fbfd7b46761e5161 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Fri, 27 Mar 2026 09:53:12 -0700 Subject: [PATCH 2/3] Fixed PR comments. Add forceRefresh to Graph token flow; improve testability Introduce a forceRefresh parameter to Microsoft Graph token acquisition, allowing explicit cache bypass and fresh token retrieval. Update IMicrosoftGraphTokenProvider and MicrosoftGraphTokenProvider to support this, and refactor GraphApiService to propagate forceRefresh throughout token and header acquisition. Update all call sites to use named arguments for clarity. Enhance ClientAppValidator and related tests to exercise token refresh logic. Improve test setup for GraphApiService and fix JWT Base64Url encoding/decoding in AuthenticationService and tests. These changes increase reliability and testability, especially for scenarios involving token revocation or conditional access. --- .../SetupSubcommands/BlueprintSubcommand.cs | 2 +- .../Services/A365CreateInstanceRunner.cs | 8 +- .../Services/AuthenticationService.cs | 2 + .../Services/ClientAppValidator.cs | 4 +- .../Services/DelegatedConsentService.cs | 2 +- .../Services/GraphApiService.cs | 1759 +++++++++-------- .../Internal/IMicrosoftGraphTokenProvider.cs | 3 +- .../Internal/MicrosoftGraphTokenProvider.cs | 11 +- .../Commands/BlueprintSubcommandTests.cs | 2 +- .../Services/AuthenticationServiceTests.cs | 6 +- .../Services/ClientAppValidatorTests.cs | 15 +- 11 files changed, 920 insertions(+), 894 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 262c6d5a..adb8f062 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs @@ -762,7 +762,7 @@ public static async Task EnsureDelegatedConsentWithRetriesAsync( existingServicePrincipalId = null; // SP missing for an existing app — attempt creation so downstream steps have a valid SP. logger.LogInformation("Service principal not found for existing blueprint — attempting to create it..."); - var spToken = await graphApiService.GetGraphAccessTokenAsync(tenantId, ct); + var spToken = await graphApiService.GetGraphAccessTokenAsync(tenantId, ct: ct); if (!string.IsNullOrWhiteSpace(spToken)) { using var spHttpClient = Services.Internal.HttpClientFactory.CreateAuthenticatedClient(spToken); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/A365CreateInstanceRunner.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/A365CreateInstanceRunner.cs index aaa66491..448bb3eb 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/A365CreateInstanceRunner.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/A365CreateInstanceRunner.cs @@ -483,7 +483,7 @@ string GetConfig(string name) => try { // Use Azure CLI token to get current user (this requires delegated context) - var delegatedToken = await _graphService.GetGraphAccessTokenAsync(tenantId, ct); + var delegatedToken = await _graphService.GetGraphAccessTokenAsync(tenantId, ct: ct); if (!string.IsNullOrWhiteSpace(delegatedToken)) { using var delegatedClient = HttpClientFactory.CreateAuthenticatedClient(delegatedToken, correlationId: correlationId); @@ -680,7 +680,7 @@ string GetConfig(string name) => _logger.LogInformation(" - Agent Identity ID: {Id}", agenticAppId); // Get Graph access token - var graphToken = await _graphService.GetGraphAccessTokenAsync(tenantId, ct); + var graphToken = await _graphService.GetGraphAccessTokenAsync(tenantId, ct: ct); if (string.IsNullOrWhiteSpace(graphToken)) { _logger.LogError("Failed to acquire Graph API access token"); @@ -940,7 +940,7 @@ private async Task AssignLicensesAsync( _logger.LogInformation("Assigning licenses to user {UserId} using Graph API (CorrelationId: {CorrelationId})", userId, correlationId); // Get Graph access token - var graphToken = await _graphService.GetGraphAccessTokenAsync(tenantId, cancellationToken); + var graphToken = await _graphService.GetGraphAccessTokenAsync(tenantId, ct: cancellationToken); if (string.IsNullOrWhiteSpace(graphToken)) { _logger.LogError("Failed to acquire Graph API access token for license assignment"); @@ -1151,7 +1151,7 @@ private async Task VerifyServicePrincipalExistsAsync( try { // Use Graph API to check if service principal exists - var graphToken = await _graphService.GetGraphAccessTokenAsync(tenantId, ct); + var graphToken = await _graphService.GetGraphAccessTokenAsync(tenantId, ct: ct); if (string.IsNullOrWhiteSpace(graphToken)) { _logger.LogWarning("Failed to acquire Graph token for service principal verification"); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs index 5e6b74b0..c3947fd4 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs @@ -614,6 +614,8 @@ protected virtual TokenCredential CreateDeviceCodeCredential(string clientId, st var parts = jwt.Split('.'); if (parts.Length < 2) return null; var payload = parts[1]; + // JWT uses Base64Url encoding: replace URL-safe chars before standard Base64 decode. + payload = payload.Replace('-', '+').Replace('_', '/'); // Restore Base64 padding stripped by JWT encoding. payload = payload.PadRight(payload.Length + (4 - payload.Length % 4) % 4, '='); var bytes = Convert.FromBase64String(payload); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs index 047fac91..c37bcfc2 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs @@ -548,7 +548,7 @@ private async Task TryExtendConsentGrantScopesAsync( const string path = "/v1.0/applications?$filter=appId eq '{0}'&$select=id,appId,displayName,requiredResourceAccess"; var graphResponse = await _graphApiService.GraphGetWithResponseAsync(tenantId, - string.Format(path, clientAppId), ct); + string.Format(path, clientAppId), ct: ct); if (graphResponse == null || !graphResponse.IsSuccess) { @@ -563,7 +563,7 @@ private async Task TryExtendConsentGrantScopesAsync( _logger.LogDebug("Graph app query returned 401 — retrying with fresh token (possible CAE revocation)"); graphResponse = await _graphApiService.GraphGetWithResponseAsync(tenantId, - string.Format(path, clientAppId), ct); + string.Format(path, clientAppId), forceRefresh: true, ct: ct); if (!graphResponse.IsSuccess) throw ClientAppValidationException.TokenRevoked(clientAppId); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/DelegatedConsentService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/DelegatedConsentService.cs index 9af5df0e..74f1cc35 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/DelegatedConsentService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/DelegatedConsentService.cs @@ -69,7 +69,7 @@ public async Task EnsureBlueprintPermissionGrantAsync( // Get Graph access token with required scopes _logger.LogInformation("Acquiring Graph API access token..."); - var graphToken = await _graphService.GetGraphAccessTokenAsync(tenantId, cancellationToken); + var graphToken = await _graphService.GetGraphAccessTokenAsync(tenantId, ct: cancellationToken); if (string.IsNullOrWhiteSpace(graphToken)) { _logger.LogError("Failed to acquire Graph API access token"); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs index 1d079419..7ee33719 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs @@ -1,873 +1,886 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using Microsoft.Agents.A365.DevTools.Cli.Constants; -using Microsoft.Agents.A365.DevTools.Cli.Models; -using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; -using Microsoft.Agents.A365.DevTools.Cli.Services.Internal; -using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Logging.Abstractions; -using System.Net; -using System.Net.Http.Headers; -using System.Text; -using System.Text.Json; -using System.Text.Json.Nodes; - -namespace Microsoft.Agents.A365.DevTools.Cli.Services; - -/// -/// Service for managing Microsoft Graph API permissions and registrations -/// -public class GraphApiService -{ - private readonly ILogger _logger; - private readonly CommandExecutor _executor; - private readonly HttpClient _httpClient; - private readonly IMicrosoftGraphTokenProvider? _tokenProvider; - private readonly IAuthenticationService _authService; - private readonly RetryHelper _retryHelper; - private string _graphBaseUrl; - - // Login hint resolved once per GraphApiService instance. - // Used to direct MSAL/WAM to the correct identity, preventing the Windows default - // account (WAM) or a stale cached MSAL account from being used instead. - // Resolved from az CLI if available, otherwise from the AuthenticationService token cache. - private string? _loginHint; - private bool _loginHintResolved; - - // Resolver delegate for the login hint. - // Defaults to az CLI first, then AuthenticationService JWT cache as fallback. - // Injectable via constructor so unit tests can bypass the real az process. - private readonly Func> _loginHintResolver; - - /// - /// Optional custom client app ID to use for authentication with Microsoft Graph PowerShell. - /// When set, this will be passed to Connect-MgGraph -ClientId parameter. - /// - public string? CustomClientAppId { get; set; } - - /// - /// Override the Microsoft Graph base URL for sovereign / government cloud tenants. - /// Defaults to (commercial cloud). - /// Set this after construction when the config is available (e.g. from Agent365Config.GraphBaseUrl). - /// - public string GraphBaseUrl - { - get => _graphBaseUrl; - set => _graphBaseUrl = string.IsNullOrWhiteSpace(value) ? GraphApiConstants.BaseUrl : value; - } - - // Lightweight wrapper to surface HTTP status, reason and body to callers - public record GraphResponse - { - public bool IsSuccess { get; init; } - public int StatusCode { get; init; } - public string ReasonPhrase { get; init; } = string.Empty; - public string Body { get; init; } = string.Empty; - public JsonDocument? Json { get; init; } - } - - // Allow injecting a custom HttpMessageHandler for unit testing. - // loginHintResolver: optional override for login-hint resolution. - // Pass () => Task.FromResult(null) in unit tests to skip login-hint resolution. - public GraphApiService(ILogger logger, CommandExecutor executor, IAuthenticationService authService, HttpMessageHandler? handler = null, IMicrosoftGraphTokenProvider? tokenProvider = null, Func>? loginHintResolver = null, string? graphBaseUrl = null, RetryHelper? retryHelper = null) - { - _logger = logger; - _executor = executor; - _authService = authService; - _httpClient = handler != null ? new HttpClient(handler) : HttpClientFactory.CreateAuthenticatedClient(); - _tokenProvider = tokenProvider; - _retryHelper = retryHelper ?? new RetryHelper(_logger); - // Default: try az CLI first (if present), fall back to JWT cache in AuthenticationService. - _loginHintResolver = loginHintResolver ?? (() => ResolveLoginHintWithFallbackAsync(authService)); - _graphBaseUrl = string.IsNullOrWhiteSpace(graphBaseUrl) ? GraphApiConstants.BaseUrl : graphBaseUrl; - } - - // Parameterless constructor to ease test mocking/substitution frameworks which may - // require creating proxy instances without providing constructor arguments. - public GraphApiService() - : this(NullLogger.Instance, new CommandExecutor(NullLogger.Instance), new AuthenticationService(NullLogger.Instance), null, null, null) - { - } - - // Two-argument convenience constructor used by tests and callers that supply - // a logger and an existing CommandExecutor (no custom handler). - public GraphApiService(ILogger logger, CommandExecutor executor) - : this(logger ?? NullLogger.Instance, executor ?? throw new ArgumentNullException(nameof(executor)), new AuthenticationService(NullLogger.Instance), null, null, null) - { - } - - private static async Task ResolveLoginHintWithFallbackAsync(IAuthenticationService authService) - { - // Try az CLI first — most reliable when the user has run 'az login'. - var hint = await AzCliHelper.ResolveLoginHintAsync(); - if (!string.IsNullOrWhiteSpace(hint)) - return hint; - // Fall back to the UPN embedded in a previously cached MSAL JWT. - return await authService.ResolveLoginHintFromCacheAsync(); - } - - /// - /// Acquires an access token for Microsoft Graph API via MSAL (WAM on Windows, - /// browser/device-code on macOS/Linux). Token is cached persistently by - /// AuthenticationService — no az CLI subprocess involved. - /// - public virtual async Task GetGraphAccessTokenAsync(string tenantId, CancellationToken ct = default) - { - _logger.LogDebug("Acquiring Graph API access token for tenant {TenantId}", tenantId); - try - { - var resource = GraphApiConstants.GetResource(_graphBaseUrl); - var loginHint = await _loginHintResolver(); - var token = await _authService.GetAccessTokenAsync(resource, tenantId, userId: loginHint); - if (!string.IsNullOrWhiteSpace(token)) - { - _logger.LogDebug("Graph API access token acquired successfully"); - return token; - } - _logger.LogError("Failed to acquire Graph API access token for tenant {TenantId}", tenantId); - return null; - } - catch (Exception ex) - { - _logger.LogError(ex, "Error acquiring Graph API access token"); - return null; - } - } - - - private async Task EnsureGraphHeadersAsync(string tenantId, CancellationToken ct = default, IEnumerable? scopes = null) - { - // Authentication Strategy: - // 1. If specific scopes required AND token provider configured: Use MSAL with delegated scopes (WAM/browser/device-code) - // 2. Otherwise: Use MSAL via AuthenticationService (WAM/browser/device-code, persistent cache) - // All paths go through MSAL — no az CLI subprocess involved. - - string? token; - - if (scopes != null && _tokenProvider != null) - { - // Use token provider with delegated scopes (interactive browser auth with caching) - _logger.LogDebug("Acquiring Graph token with specific scopes via token provider: {Scopes}", string.Join(", ", scopes)); - var loginHint = await ResolveLoginHintAsync(); - token = await _tokenProvider.GetMgGraphAccessTokenAsync(tenantId, scopes, false, CustomClientAppId, ct, loginHint); - - if (string.IsNullOrWhiteSpace(token)) - { - _logger.LogError("Failed to acquire Graph token with scopes: {Scopes}", string.Join(", ", scopes)); - return false; - } - - _logger.LogDebug("Successfully acquired Graph token with specific scopes (cached or new)"); - } - else if (scopes != null && _tokenProvider == null) - { - // Scopes required but no token provider - this is a configuration issue - _logger.LogError("Token provider is not configured, but specific scopes are required: {Scopes}", string.Join(", ", scopes)); - return false; - } - else - { - // Default path: acquire via AuthenticationService (MSAL, persistent disk cache). - token = await GetGraphAccessTokenAsync(tenantId, ct); - - if (string.IsNullOrWhiteSpace(token)) - { - _logger.LogError("Failed to acquire Graph token. Sign-in will be prompted on the next attempt."); - return false; - } - } - - // Remove all newline characters and trim whitespace to prevent header validation errors - token = token.ReplaceLineEndings(string.Empty).Trim(); - - _httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", token); - // NOTE: Do NOT add "ConsistencyLevel: eventual" header here. - // This header is only required for advanced Graph query capabilities ($count, $search, certain $filter operations). - // For simple queries like service principal lookups, this header is not needed and causes HTTP 400 errors. - // See: https://learn.microsoft.com/en-us/graph/aad-advanced-queries - - return true; - } - - /// - /// Returns the object ID of the currently signed-in user via GET /v1.0/me. - /// Replaces 'az ad signed-in-user show --query id -o tsv' (~30s) with a Graph HTTP call (~200ms). - /// Returns null if the call fails (caller should fall back to az CLI). - /// - public virtual async Task GetCurrentUserObjectIdAsync(string tenantId, CancellationToken ct = default) - { - using var doc = await GraphGetAsync(tenantId, "/v1.0/me?$select=id", ct); - if (doc == null) return null; - return doc.RootElement.TryGetProperty("id", out var idEl) ? idEl.GetString() : null; - } - - /// - /// Checks whether a service principal with the given object ID exists in the tenant. - /// Replaces 'az ad sp show --id {principalId}' (~30s) with a Graph HTTP call (~200ms). - /// Used for MSI propagation polling — returns true when the SP is visible in the tenant. - /// - public virtual async Task ServicePrincipalExistsAsync(string tenantId, string principalId, CancellationToken ct = default) - { - using var doc = await GraphGetAsync(tenantId, $"/v1.0/servicePrincipals/{principalId}?$select=id", ct); - return doc != null; - } - - /// - /// Executes a GET request to Microsoft Graph API. - /// Virtual to allow mocking in unit tests using Moq. - /// - public virtual async Task GraphGetAsync(string tenantId, string relativePath, CancellationToken ct = default, IEnumerable? scopes = null) - { - if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) return null; - var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); - try - { - using var resp = await _retryHelper.ExecuteWithRetryAsync( - ct => _httpClient.GetAsync(url, ct), cancellationToken: ct); - if (!resp.IsSuccessStatusCode) - { - var errorBody = await resp.Content.ReadAsStringAsync(ct); - _logger.LogDebug("Graph GET {Url} failed {Code} {Reason}: {Body}", url, (int)resp.StatusCode, resp.ReasonPhrase, errorBody); - return null; - } - var json = await resp.Content.ReadAsStringAsync(ct); - return JsonDocument.Parse(json); - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - if (NetworkHelper.IsConnectionResetByProxy(ex)) - _logger.LogWarning(NetworkHelper.ConnectionResetWarning); - else - _logger.LogDebug(ex, "Graph GET {Url} failed with exception", url); - return null; - } - } - - /// - /// GET from Graph and always return HTTP response details (status, body, parsed JSON). - /// Use this instead of GraphGetAsync when the caller needs to distinguish auth failures - /// (401) from transient server errors (503, 429, network exceptions). - /// - public virtual async Task GraphGetWithResponseAsync(string tenantId, string relativePath, CancellationToken ct = default, IEnumerable? scopes = null) - { - if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) - return new GraphResponse { IsSuccess = false, StatusCode = 0, ReasonPhrase = "NoAuth", Body = "Failed to acquire token" }; - - var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); - - try - { - using var resp = await _httpClient.GetAsync(url, ct); - var body = await resp.Content.ReadAsStringAsync(ct); - - JsonDocument? json = null; - if (resp.IsSuccessStatusCode && !string.IsNullOrWhiteSpace(body)) - { - try { json = JsonDocument.Parse(body); } catch { /* ignore parse errors */ } - } - - if (!resp.IsSuccessStatusCode) - _logger.LogDebug("Graph GET {Url} failed {Code} {Reason}: {Body}", url, (int)resp.StatusCode, resp.ReasonPhrase, body); - - return new GraphResponse - { - IsSuccess = resp.IsSuccessStatusCode, - StatusCode = (int)resp.StatusCode, - ReasonPhrase = resp.ReasonPhrase ?? string.Empty, - Body = body ?? string.Empty, - Json = json - }; - } - catch (Exception ex) - { - if (NetworkHelper.IsConnectionResetByProxy(ex)) - _logger.LogWarning(NetworkHelper.ConnectionResetWarning); - else - _logger.LogDebug(ex, "Graph GET {Url} threw an exception", url); - return new GraphResponse { IsSuccess = false, StatusCode = 0, ReasonPhrase = ex.Message, Body = string.Empty }; - } - } - - public virtual async Task GraphPostAsync(string tenantId, string relativePath, object payload, CancellationToken ct = default, IEnumerable? scopes = null) - { - if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) return null; - var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); - var content = new StringContent(JsonSerializer.Serialize(payload), Encoding.UTF8, "application/json"); - try - { - using var resp = await _httpClient.PostAsync(url, content, ct); - var body = await resp.Content.ReadAsStringAsync(ct); - if (!resp.IsSuccessStatusCode) - { - var errorMessage = TryExtractGraphErrorMessage(body); - if (errorMessage != null) - _logger.LogWarning("Graph POST {Url} failed: {ErrorMessage}", url, errorMessage); - else - _logger.LogWarning("Graph POST {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); - _logger.LogDebug("Graph POST response body: {Body}", body); - return null; - } - - return string.IsNullOrWhiteSpace(body) ? null : JsonDocument.Parse(body); - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - if (NetworkHelper.IsConnectionResetByProxy(ex)) - _logger.LogWarning(NetworkHelper.ConnectionResetWarning); - else - _logger.LogDebug(ex, "Graph POST {Url} failed with exception", url); - throw; - } - } - - /// - /// POST to Graph but always return HTTP response details (status, body, parsed JSON) - /// - public virtual async Task GraphPostWithResponseAsync(string tenantId, string relativePath, object payload, CancellationToken ct = default, IEnumerable? scopes = null) - { - if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) - { - return new GraphResponse { IsSuccess = false, StatusCode = 0, ReasonPhrase = "NoAuth", Body = "Failed to acquire token" }; - } - - var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); - - var content = new StringContent(JsonSerializer.Serialize(payload), Encoding.UTF8, "application/json"); - try - { - using var resp = await _httpClient.PostAsync(url, content, ct); - var body = await resp.Content.ReadAsStringAsync(ct); - - JsonDocument? json = null; - if (!string.IsNullOrWhiteSpace(body)) - { - try { json = JsonDocument.Parse(body); } catch { /* ignore parse errors */ } - } - - return new GraphResponse - { - IsSuccess = resp.IsSuccessStatusCode, - StatusCode = (int)resp.StatusCode, - ReasonPhrase = resp.ReasonPhrase ?? string.Empty, - Body = body ?? string.Empty, - Json = json - }; - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - if (NetworkHelper.IsConnectionResetByProxy(ex)) - _logger.LogWarning(NetworkHelper.ConnectionResetWarning); - else - _logger.LogDebug(ex, "Graph POST {Url} failed with exception", url); - throw; - } - } - - /// - /// Executes a PATCH request to Microsoft Graph API. - /// Virtual to allow mocking in unit tests using Moq. - /// - public virtual async Task GraphPatchAsync(string tenantId, string relativePath, object payload, CancellationToken ct = default, IEnumerable? scopes = null) - { - if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) return false; - var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); - var content = new StringContent(JsonSerializer.Serialize(payload), Encoding.UTF8, "application/json"); - try - { - using var request = new HttpRequestMessage(new HttpMethod("PATCH"), url) { Content = content }; - using var resp = await _httpClient.SendAsync(request, ct); - - // Many PATCH calls return 204 NoContent on success - if (!resp.IsSuccessStatusCode) - { - var body = await resp.Content.ReadAsStringAsync(ct); - var errorMessage = TryExtractGraphErrorMessage(body); - if (errorMessage != null) - _logger.LogError("Graph PATCH {Url} failed: {ErrorMessage}", url, errorMessage); - else - _logger.LogError("Graph PATCH {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); - _logger.LogDebug("Graph PATCH response body: {Body}", body); - } - - return resp.IsSuccessStatusCode; - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - if (NetworkHelper.IsConnectionResetByProxy(ex)) - _logger.LogWarning(NetworkHelper.ConnectionResetWarning); - else - _logger.LogDebug(ex, "Graph PATCH {Url} failed with exception", url); - throw; - } - } - - public async Task GraphDeleteAsync( - string tenantId, - string relativePath, - CancellationToken ct = default, - bool treatNotFoundAsSuccess = true, - IEnumerable? scopes = null) - { - if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) return false; - - var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); - - try - { - using var req = new HttpRequestMessage(HttpMethod.Delete, url); - using var resp = await _httpClient.SendAsync(req, ct); - - // 404 can be considered success for idempotent deletes - if (treatNotFoundAsSuccess && (int)resp.StatusCode == 404) return true; - - if (!resp.IsSuccessStatusCode) - { - var body = await resp.Content.ReadAsStringAsync(ct); - var errorMessage = TryExtractGraphErrorMessage(body); - if (errorMessage != null) - _logger.LogError("Graph DELETE {Url} failed: {ErrorMessage}", url, errorMessage); - else - _logger.LogError("Graph DELETE {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); - _logger.LogDebug("Graph DELETE response body: {Body}", body); - return false; - } - - return true; - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - if (NetworkHelper.IsConnectionResetByProxy(ex)) - _logger.LogWarning(NetworkHelper.ConnectionResetWarning); - else - _logger.LogDebug(ex, "Graph DELETE {Url} failed with exception", url); - throw; - } - } - - /// - /// Looks up a service principal by its application (client) ID. - /// Virtual to allow mocking in unit tests using Moq. - /// - public virtual async Task LookupServicePrincipalByAppIdAsync( - string tenantId, string appId, CancellationToken ct = default, IEnumerable? scopes = null) - { - // $filter=appId eq is "Default+Advanced" per Graph docs -� no ConsistencyLevel header required. - // The token must have Application.Read.All; pass scopes to ensure MSAL token is used when needed. - using var doc = await GraphGetAsync( - tenantId, - $"/v1.0/servicePrincipals?$filter=appId eq '{appId}'&$select=id", - ct, - scopes); - if (doc == null) return null; - if (!doc.RootElement.TryGetProperty("value", out var value) || value.GetArrayLength() == 0) return null; - return value[0].GetProperty("id").GetString(); - } - - /// - /// Looks up the display name of a service principal by its application ID. - /// Returns null if the service principal is not found. - /// Virtual to allow substitution in unit tests using NSubstitute. - /// - public virtual async Task GetServicePrincipalDisplayNameAsync( - string tenantId, string appId, CancellationToken ct = default, IEnumerable? scopes = null) - { - // Validate GUID format to prevent OData injection - if (!Guid.TryParse(appId, out var validGuid)) - { - _logger.LogWarning("Invalid appId format for service principal lookup: {AppId}", appId); - return null; - } - - // Use validated GUID in normalized format to prevent OData injection - using var doc = await GraphGetAsync(tenantId, $"/v1.0/servicePrincipals?$filter=appId eq '{validGuid:D}'&$select=displayName", ct, scopes); - if (doc == null) return null; - if (!doc.RootElement.TryGetProperty("value", out var value) || value.GetArrayLength() == 0) return null; - if (!value[0].TryGetProperty("displayName", out var displayName)) return null; - return displayName.GetString(); - } - - /// - /// Ensures a service principal exists for the given application ID. - /// Creates the service principal if it doesn't already exist. - /// Virtual to allow mocking in unit tests using Moq. - /// - public virtual async Task EnsureServicePrincipalForAppIdAsync( - string tenantId, string appId, CancellationToken ct = default, IEnumerable? scopes = null) - { - // Try existing - var spId = await LookupServicePrincipalByAppIdAsync(tenantId, appId, ct, scopes); - if (!string.IsNullOrWhiteSpace(spId)) return spId!; - - // Create SP for this application - var created = await GraphPostAsync(tenantId, "/v1.0/servicePrincipals", new { appId }, ct, scopes); - if (created == null || !created.RootElement.TryGetProperty("id", out var idProp)) - throw new InvalidOperationException($"Failed to create servicePrincipal for appId {appId}"); - - return idProp.GetString()!; - } - - public async Task CreateOrUpdateOauth2PermissionGrantAsync( - string tenantId, - string clientSpObjectId, - string resourceSpObjectId, - IEnumerable scopes, - CancellationToken ct = default, - IEnumerable? permissionGrantScopes = null) - { - var desiredScopeString = string.Join(' ', scopes); - - // Read existing — extract string values immediately so JsonDocument can be disposed - string? existingId = null; - string existingScopes = ""; - - using (var listDoc = await GraphGetAsync( - tenantId, - $"/v1.0/oauth2PermissionGrants?$filter=clientId eq '{clientSpObjectId}' and resourceId eq '{resourceSpObjectId}'", - ct, - permissionGrantScopes)) - { - if (listDoc?.RootElement.TryGetProperty("value", out var arr) == true && arr.GetArrayLength() > 0) - { - var grant = arr[0]; - existingId = grant.TryGetProperty("id", out var idProp) ? idProp.GetString() : null; - existingScopes = grant.TryGetProperty("scope", out var scopeProp) ? scopeProp.GetString() ?? "" : ""; - } - } - - if (string.IsNullOrWhiteSpace(existingId)) - { - // AllPrincipals (tenant-wide) grants require Global Administrator. - // Only called from admin paths (setup admin or setup all run by GA). - var payload = new - { - clientId = clientSpObjectId, - consentType = "AllPrincipals", - resourceId = resourceSpObjectId, - scope = desiredScopeString - }; - - _logger.LogDebug("Graph POST /v1.0/oauth2PermissionGrants body: {Body}", JsonSerializer.Serialize(payload)); - - // A freshly-created service principal may not yet be visible to the - // oauth2PermissionGrants replica (Directory_ObjectNotFound). Retry with - // exponential back-off so the command is self-healing without user intervention. - const int maxRetries = 8; - const int baseDelaySeconds = 5; - for (int attempt = 0; attempt < maxRetries; attempt++) - { - var grantResponse = await GraphPostWithResponseAsync(tenantId, "/v1.0/oauth2PermissionGrants", payload, ct, permissionGrantScopes); - // Dispose the error JSON immediately — only IsSuccess and Body are needed below. - grantResponse.Json?.Dispose(); - - if (grantResponse.IsSuccess) - return true; - - if (!grantResponse.Body.Contains("Directory_ObjectNotFound", StringComparison.OrdinalIgnoreCase)) - return false; // non-transient error, do not retry - - if (attempt < maxRetries - 1) - { - var delaySecs = (int)Math.Min(baseDelaySeconds * Math.Pow(2, attempt), 60); - _logger.LogWarning( - "Service principal not yet replicated to grants endpoint — retrying in {Delay}s (attempt {Attempt}/{Max})...", - delaySecs, attempt + 1, maxRetries - 1); - await Task.Delay(TimeSpan.FromSeconds(delaySecs), ct); - } - } - - _logger.LogWarning( - "OAuth2 permission grant failed after {MaxRetries} retries — service principal may still be propagating. " + - "Re-run 'a365 setup admin' to retry.", - maxRetries); - return false; - } - - // Merge scopes if needed - var currentSet = new HashSet(existingScopes.Split(' ', StringSplitOptions.RemoveEmptyEntries), StringComparer.OrdinalIgnoreCase); - var desiredSet = new HashSet(desiredScopeString.Split(' ', StringSplitOptions.RemoveEmptyEntries), StringComparer.OrdinalIgnoreCase); - - if (desiredSet.IsSubsetOf(currentSet)) return true; // already satisfied - - currentSet.UnionWith(desiredSet); - var merged = string.Join(' ', currentSet); - - return await GraphPatchAsync(tenantId, $"/v1.0/oauth2PermissionGrants/{existingId}", new { scope = merged }, ct, permissionGrantScopes); - } - - /// - /// Checks if the current user has sufficient privileges to create service principals. - /// Virtual to allow mocking in unit tests using Moq. - /// - /// The tenant ID - /// Cancellation token - /// True if user has required roles, false otherwise - public virtual async Task<(bool hasPrivileges, List roles)> CheckServicePrincipalCreationPrivilegesAsync( - string tenantId, - CancellationToken ct = default) - { - try - { - _logger.LogDebug("Checking user's directory roles for service principal creation privileges"); - - var token = await GetGraphAccessTokenAsync(tenantId, ct); - if (token == null) - { - _logger.LogWarning("Could not acquire Graph token to check privileges"); - return (false, new List()); - } - - // Trim token to remove any newline characters that may cause header validation errors - token = token.Trim(); - - using var request = new HttpRequestMessage(HttpMethod.Get, - $"{_graphBaseUrl}/v1.0/me/memberOf/microsoft.graph.directoryRole"); - request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); - - using var response = await _httpClient.SendAsync(request, ct); - if (!response.IsSuccessStatusCode) - { - _logger.LogWarning("Could not retrieve user's directory roles: {Status}", response.StatusCode); - return (false, new List()); - } - - var json = await response.Content.ReadAsStringAsync(ct); - var doc = JsonDocument.Parse(json); - - var roles = new List(); - if (doc.RootElement.TryGetProperty("value", out var rolesArray)) - { - roles = rolesArray.EnumerateArray() - .Where(role => role.TryGetProperty("displayName", out var displayName)) - .Select(role => role.GetProperty("displayName").GetString()) - .Where(roleName => !string.IsNullOrEmpty(roleName)) - .ToList()!; - } - - _logger.LogDebug("User has {Count} directory roles", roles.Count); - - // Check for required roles - var requiredRoles = new[] - { - "Application Administrator", - "Cloud Application Administrator", - "Global Administrator" - }; - - var hasRequiredRole = roles.Any(r => requiredRoles.Contains(r, StringComparer.OrdinalIgnoreCase)); - - if (hasRequiredRole) - { - _logger.LogDebug("User has sufficient privileges for service principal creation"); - } - else - { - _logger.LogDebug("User does not have required roles for service principal creation. Roles: {Roles}", - string.Join(", ", roles)); - } - - return (hasRequiredRole, roles); - } - catch (Exception ex) - { - _logger.LogWarning(ex, "Failed to check service principal creation privileges: {Message}", ex.Message); - return (false, new List()); - } - } - - /// - /// Checks if a user is an owner of an application (read-only validation). - /// Does not attempt to add the user as owner, only verifies ownership. - /// - /// The tenant ID - /// The application object ID (not the client/app ID) - /// The user's object ID to check. If null, uses the current authenticated user. - /// Cancellation token - /// OAuth2 scopes for elevated permissions (e.g., Application.ReadWrite.All, Directory.ReadWrite.All) - /// True if the user is an owner, false otherwise - public virtual async Task IsApplicationOwnerAsync( - string tenantId, - string applicationObjectId, - string? userObjectId = null, - CancellationToken ct = default, - IEnumerable? scopes = null) - { - try - { - // Get current user's object ID if not provided - if (string.IsNullOrWhiteSpace(userObjectId)) - { - if (!await EnsureGraphHeadersAsync(tenantId, ct, scopes)) - { - _logger.LogWarning("Could not acquire Graph token to check application owner"); - return false; - } - - using var meRequest = new HttpRequestMessage(HttpMethod.Get, - $"{_graphBaseUrl}/v1.0/me?$select=id"); - meRequest.Headers.Authorization = _httpClient.DefaultRequestHeaders.Authorization; - - using var meResponse = await _httpClient.SendAsync(meRequest, ct); - if (!meResponse.IsSuccessStatusCode) - { - _logger.LogWarning("Could not retrieve current user's ID: {Status}", meResponse.StatusCode); - return false; - } - - var meJson = await meResponse.Content.ReadAsStringAsync(ct); - using var meDoc = JsonDocument.Parse(meJson); - - if (!meDoc.RootElement.TryGetProperty("id", out var idElement)) - { - _logger.LogWarning("Could not extract user ID from Graph response"); - return false; - } - - userObjectId = idElement.GetString(); - } - - if (string.IsNullOrWhiteSpace(userObjectId)) - { - _logger.LogWarning("User object ID is empty, cannot check owner"); - return false; - } - - // Check if user is an owner - _logger.LogDebug("Checking if user {UserId} is an owner of application {AppObjectId}", userObjectId, applicationObjectId); - - var ownersDoc = await GraphGetAsync(tenantId, $"/v1.0/applications/{applicationObjectId}/owners?$select=id", ct, scopes); - if (ownersDoc != null && ownersDoc.RootElement.TryGetProperty("value", out var ownersArray)) - { - var isOwner = ownersArray.EnumerateArray() - .Where(owner => owner.TryGetProperty("id", out var ownerId)) - .Any(owner => string.Equals(owner.GetProperty("id").GetString(), userObjectId, StringComparison.OrdinalIgnoreCase)); - - return isOwner; - } - - return false; - } - catch (Exception ex) - { - _logger.LogWarning(ex, "Error checking if user is owner of application: {Message}", ex.Message); - return false; - } - } - - /// - /// Checks whether the currently signed-in user holds the Global Administrator role, - /// which is required to grant tenant-wide admin consent interactively. - /// Uses only — works for both admin and non-admin users. - /// Returns (non-blocking) if the check cannot be completed. - /// - public virtual async Task IsCurrentUserAdminAsync( - string tenantId, - CancellationToken ct = default) - { - return await CheckDirectoryRoleAsync(tenantId, AuthenticationConstants.GlobalAdminRoleTemplateId, ct); - } - - /// - /// Checks whether the currently signed-in user holds the Agent ID Administrator role, - /// which is required to create or update inheritable permissions on agent blueprints. - /// Uses only — works for both admin and non-admin users. - /// Returns (non-blocking) if the check cannot be completed. - /// - public virtual async Task IsCurrentUserAgentIdAdminAsync( - string tenantId, - CancellationToken ct = default) - { - return await CheckDirectoryRoleAsync(tenantId, AuthenticationConstants.AgentIdAdminRoleTemplateId, ct); - } - - /// - /// Returns if the role is confirmed active, - /// if confirmed absent, or - /// if the check itself failed (e.g. network error, - /// throttling, auth failure) — in which case the caller should attempt the operation - /// anyway and let the API surface the real error. - /// Queries /me/transitiveMemberOf/microsoft.graph.directoryRole, which requires only - /// User.Read and succeeds for both admin and non-admin users. - /// Note: PIM-eligible-but-not-activated assignments are not considered active. - /// - private async Task CheckDirectoryRoleAsync(string tenantId, string roleTemplateId, CancellationToken ct) - { - try - { - // /me/transitiveMemberOf is a directory query — Directory.Read.All is required. - // User.Read is insufficient and would return Unknown for most users. - IEnumerable? scopes = _tokenProvider != null - ? [AuthenticationConstants.DirectoryReadAllScope] - : null; - - string? nextUrl = "/v1.0/me/transitiveMemberOf/microsoft.graph.directoryRole?$select=roleTemplateId"; - - while (nextUrl != null) - { - using var doc = await GraphGetAsync(tenantId, nextUrl, ct, scopes); - - if (doc == null) - return Models.RoleCheckResult.Unknown; - - if (!doc.RootElement.TryGetProperty("value", out var roles)) - { - _logger.LogWarning("Unexpected Graph response shape — 'value' property missing from transitiveMemberOf response."); - return Models.RoleCheckResult.Unknown; - } - - if (roles.EnumerateArray().Any(r => - r.TryGetProperty("roleTemplateId", out var id) && - string.Equals(id.GetString(), roleTemplateId, StringComparison.OrdinalIgnoreCase))) - return Models.RoleCheckResult.HasRole; - - nextUrl = doc.RootElement.TryGetProperty("@odata.nextLink", out var nextLink) - ? nextLink.GetString() - : null; - } - - return Models.RoleCheckResult.DoesNotHaveRole; - } - catch (Exception ex) - { - _logger.LogDebug(ex, "Role check for {TemplateId} failed — will attempt operation anyway: {Message}", - roleTemplateId, ex.Message); - return Models.RoleCheckResult.Unknown; - } - } - - /// - /// Resolves the Azure CLI login hint once per instance from 'az account show'. - /// The hint is passed to MSAL so that WAM and silent auth target the correct - /// Azure CLI identity instead of the Windows default account. - /// Returns null if az account show fails or the user field is absent. - /// - private async Task ResolveLoginHintAsync() - { - if (_loginHintResolved) - return _loginHint; - - _loginHintResolved = true; - _loginHint = await _loginHintResolver(); - return _loginHint; - } - - /// - /// Attempts to extract a human-readable error message from a Graph API JSON error response body. - /// Returns null if the body cannot be parsed or does not contain an error message. - /// - private static string? TryExtractGraphErrorMessage(string body) - { - if (string.IsNullOrWhiteSpace(body)) return null; - try - { - using var doc = JsonDocument.Parse(body); - if (doc.RootElement.TryGetProperty("error", out var error) && - error.TryGetProperty("message", out var msg)) - { - return msg.GetString(); - } - } - catch { /* ignore parse errors */ } - return null; - } -} +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Constants; +using Microsoft.Agents.A365.DevTools.Cli.Models; +using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; +using Microsoft.Agents.A365.DevTools.Cli.Services.Internal; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using System.Net; +using System.Net.Http.Headers; +using System.Text; +using System.Text.Json; +using System.Text.Json.Nodes; + +namespace Microsoft.Agents.A365.DevTools.Cli.Services; + +/// +/// Service for managing Microsoft Graph API permissions and registrations +/// +public class GraphApiService +{ + private readonly ILogger _logger; + private readonly CommandExecutor _executor; + private readonly HttpClient _httpClient; + private readonly IMicrosoftGraphTokenProvider? _tokenProvider; + private readonly IAuthenticationService _authService; + private readonly RetryHelper _retryHelper; + private string _graphBaseUrl; + + // Login hint resolved once per GraphApiService instance. + // Used to direct MSAL/WAM to the correct identity, preventing the Windows default + // account (WAM) or a stale cached MSAL account from being used instead. + // Resolved from az CLI if available, otherwise from the AuthenticationService token cache. + private string? _loginHint; + private bool _loginHintResolved; + + // Resolver delegate for the login hint. + // Defaults to az CLI first, then AuthenticationService JWT cache as fallback. + // Injectable via constructor so unit tests can bypass the real az process. + private readonly Func> _loginHintResolver; + + /// + /// Optional custom client app ID to use for authentication with Microsoft Graph PowerShell. + /// When set, this will be passed to Connect-MgGraph -ClientId parameter. + /// + public string? CustomClientAppId { get; set; } + + /// + /// Override the Microsoft Graph base URL for sovereign / government cloud tenants. + /// Defaults to (commercial cloud). + /// Set this after construction when the config is available (e.g. from Agent365Config.GraphBaseUrl). + /// + public string GraphBaseUrl + { + get => _graphBaseUrl; + set => _graphBaseUrl = string.IsNullOrWhiteSpace(value) ? GraphApiConstants.BaseUrl : value; + } + + // Lightweight wrapper to surface HTTP status, reason and body to callers + public record GraphResponse + { + public bool IsSuccess { get; init; } + public int StatusCode { get; init; } + public string ReasonPhrase { get; init; } = string.Empty; + public string Body { get; init; } = string.Empty; + public JsonDocument? Json { get; init; } + } + + // Allow injecting a custom HttpMessageHandler for unit testing. + // loginHintResolver: optional override for login-hint resolution. + // Pass () => Task.FromResult(null) in unit tests to skip login-hint resolution. + public GraphApiService(ILogger logger, CommandExecutor executor, IAuthenticationService authService, HttpMessageHandler? handler = null, IMicrosoftGraphTokenProvider? tokenProvider = null, Func>? loginHintResolver = null, string? graphBaseUrl = null, RetryHelper? retryHelper = null) + { + _logger = logger; + _executor = executor; + _authService = authService; + _httpClient = handler != null ? new HttpClient(handler) : HttpClientFactory.CreateAuthenticatedClient(); + _tokenProvider = tokenProvider; + _retryHelper = retryHelper ?? new RetryHelper(_logger); + // Default: try az CLI first (if present), fall back to JWT cache in AuthenticationService. + _loginHintResolver = loginHintResolver ?? (() => ResolveLoginHintWithFallbackAsync(authService)); + _graphBaseUrl = string.IsNullOrWhiteSpace(graphBaseUrl) ? GraphApiConstants.BaseUrl : graphBaseUrl; + } + + // Parameterless constructor to ease test mocking/substitution frameworks which may + // require creating proxy instances without providing constructor arguments. + public GraphApiService() + : this(NullLogger.Instance, new CommandExecutor(NullLogger.Instance), new AuthenticationService(NullLogger.Instance), null, null, null) + { + } + + // Convenience constructors for tests. Castle DynamicProxy (used by NSubstitute) resolves + // constructors by exact argument count — it does not handle C# optional parameters. + // Both forms must exist as separate constructors so Castle can proxy either call site. + // + // 2-arg form: used by tests that do not need loginHintResolver control. + public GraphApiService(ILogger logger, CommandExecutor executor) + : this(logger ?? NullLogger.Instance, executor ?? throw new ArgumentNullException(nameof(executor)), new AuthenticationService(NullLogger.Instance), null, null, null) + { + } + + // 3-arg form: used by partial-mock tests that inject loginHintResolver to prevent + // a real az account show subprocess. Pass () => Task.FromResult(null). + public GraphApiService(ILogger logger, CommandExecutor executor, Func>? loginHintResolver) + : this(logger ?? NullLogger.Instance, executor ?? throw new ArgumentNullException(nameof(executor)), new AuthenticationService(NullLogger.Instance), null, null, loginHintResolver) + { + } + + private static async Task ResolveLoginHintWithFallbackAsync(IAuthenticationService authService) + { + // Try az CLI first — most reliable when the user has run 'az login'. + var hint = await AzCliHelper.ResolveLoginHintAsync(); + if (!string.IsNullOrWhiteSpace(hint)) + return hint; + // Fall back to the UPN embedded in a previously cached MSAL JWT. + return await authService.ResolveLoginHintFromCacheAsync(); + } + + /// + /// Acquires an access token for Microsoft Graph API via MSAL (WAM on Windows, + /// browser/device-code on macOS/Linux). Token is cached persistently by + /// AuthenticationService — no az CLI subprocess involved. + /// + public virtual async Task GetGraphAccessTokenAsync(string tenantId, bool forceRefresh = false, CancellationToken ct = default) + { + _logger.LogDebug("Acquiring Graph API access token for tenant {TenantId}", tenantId); + try + { + var resource = GraphApiConstants.GetResource(_graphBaseUrl); + var loginHint = await _loginHintResolver(); + var token = await _authService.GetAccessTokenAsync(resource, tenantId, forceRefresh: forceRefresh, userId: loginHint); + if (!string.IsNullOrWhiteSpace(token)) + { + _logger.LogDebug("Graph API access token acquired successfully"); + return token; + } + _logger.LogError("Failed to acquire Graph API access token for tenant {TenantId}", tenantId); + return null; + } + catch (Exception ex) + { + _logger.LogError(ex, "Error acquiring Graph API access token"); + return null; + } + } + + + private async Task EnsureGraphHeadersAsync(string tenantId, bool forceRefresh = false, IEnumerable? scopes = null, CancellationToken ct = default) + { + // Authentication Strategy: + // 1. If specific scopes required AND token provider configured: Use MSAL with delegated scopes (WAM/browser/device-code) + // 2. Otherwise: Use MSAL via AuthenticationService (WAM/browser/device-code, persistent cache) + // All paths go through MSAL — no az CLI subprocess involved. + + string? token; + + if (scopes != null && _tokenProvider != null) + { + // Use token provider with delegated scopes (interactive browser auth with caching) + _logger.LogDebug("Acquiring Graph token with specific scopes via token provider: {Scopes}", string.Join(", ", scopes)); + var loginHint = await ResolveLoginHintAsync(); + token = await _tokenProvider.GetMgGraphAccessTokenAsync(tenantId, scopes, false, CustomClientAppId, ct, loginHint, forceRefresh); + + if (string.IsNullOrWhiteSpace(token)) + { + _logger.LogError("Failed to acquire Graph token with scopes: {Scopes}", string.Join(", ", scopes)); + return false; + } + + _logger.LogDebug("Successfully acquired Graph token with specific scopes (cached or new)"); + } + else if (scopes != null && _tokenProvider == null) + { + // Scopes required but no token provider - this is a configuration issue + _logger.LogError("Token provider is not configured, but specific scopes are required: {Scopes}", string.Join(", ", scopes)); + return false; + } + else + { + // Default path: acquire via AuthenticationService (MSAL, persistent disk cache). + token = await GetGraphAccessTokenAsync(tenantId, forceRefresh: forceRefresh, ct: ct); + + if (string.IsNullOrWhiteSpace(token)) + { + _logger.LogError("Failed to acquire Graph token. Sign-in will be prompted on the next attempt."); + return false; + } + } + + // Remove all newline characters and trim whitespace to prevent header validation errors + token = token.ReplaceLineEndings(string.Empty).Trim(); + + _httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", token); + // NOTE: Do NOT add "ConsistencyLevel: eventual" header here. + // This header is only required for advanced Graph query capabilities ($count, $search, certain $filter operations). + // For simple queries like service principal lookups, this header is not needed and causes HTTP 400 errors. + // See: https://learn.microsoft.com/en-us/graph/aad-advanced-queries + + return true; + } + + /// + /// Returns the object ID of the currently signed-in user via GET /v1.0/me. + /// Replaces 'az ad signed-in-user show --query id -o tsv' (~30s) with a Graph HTTP call (~200ms). + /// Returns null if the call fails (caller should fall back to az CLI). + /// + public virtual async Task GetCurrentUserObjectIdAsync(string tenantId, CancellationToken ct = default) + { + using var doc = await GraphGetAsync(tenantId, "/v1.0/me?$select=id", ct); + if (doc == null) return null; + return doc.RootElement.TryGetProperty("id", out var idEl) ? idEl.GetString() : null; + } + + /// + /// Checks whether a service principal with the given object ID exists in the tenant. + /// Replaces 'az ad sp show --id {principalId}' (~30s) with a Graph HTTP call (~200ms). + /// Used for MSI propagation polling — returns true when the SP is visible in the tenant. + /// + public virtual async Task ServicePrincipalExistsAsync(string tenantId, string principalId, CancellationToken ct = default) + { + using var doc = await GraphGetAsync(tenantId, $"/v1.0/servicePrincipals/{principalId}?$select=id", ct); + return doc != null; + } + + /// + /// Executes a GET request to Microsoft Graph API. + /// Virtual to allow mocking in unit tests using Moq. + /// + public virtual async Task GraphGetAsync(string tenantId, string relativePath, CancellationToken ct = default, IEnumerable? scopes = null) + { + if (!await EnsureGraphHeadersAsync(tenantId, scopes: scopes, ct: ct)) return null; + var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); + try + { + using var resp = await _retryHelper.ExecuteWithRetryAsync( + ct => _httpClient.GetAsync(url, ct), cancellationToken: ct); + if (!resp.IsSuccessStatusCode) + { + var errorBody = await resp.Content.ReadAsStringAsync(ct); + _logger.LogDebug("Graph GET {Url} failed {Code} {Reason}: {Body}", url, (int)resp.StatusCode, resp.ReasonPhrase, errorBody); + return null; + } + var json = await resp.Content.ReadAsStringAsync(ct); + return JsonDocument.Parse(json); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "Graph GET {Url} failed with exception", url); + return null; + } + } + + /// + /// GET from Graph and always return HTTP response details (status, body, parsed JSON). + /// Use this instead of GraphGetAsync when the caller needs to distinguish auth failures + /// (401) from transient server errors (503, 429, network exceptions). + /// + public virtual async Task GraphGetWithResponseAsync(string tenantId, string relativePath, bool forceRefresh = false, IEnumerable? scopes = null, CancellationToken ct = default) + { + if (!await EnsureGraphHeadersAsync(tenantId, forceRefresh: forceRefresh, scopes: scopes, ct: ct)) + return new GraphResponse { IsSuccess = false, StatusCode = 0, ReasonPhrase = "NoAuth", Body = "Failed to acquire token" }; + + var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); + + try + { + return await _retryHelper.ExecuteWithRetryAsync(async (token) => + { + using var resp = await _httpClient.GetAsync(url, token); + var body = await resp.Content.ReadAsStringAsync(token); + + JsonDocument? json = null; + if (resp.IsSuccessStatusCode && !string.IsNullOrWhiteSpace(body)) + { + try { json = JsonDocument.Parse(body); } catch { /* ignore parse errors */ } + } + + if (!resp.IsSuccessStatusCode) + _logger.LogDebug("Graph GET {Url} failed {Code} {Reason}: {Body}", url, (int)resp.StatusCode, resp.ReasonPhrase, body); + + return new GraphResponse + { + IsSuccess = resp.IsSuccessStatusCode, + StatusCode = (int)resp.StatusCode, + ReasonPhrase = resp.ReasonPhrase ?? string.Empty, + Body = body ?? string.Empty, + Json = json + }; + }, cancellationToken: ct); + } + catch (Exception ex) + { + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "Graph GET {Url} threw an exception", url); + return new GraphResponse { IsSuccess = false, StatusCode = 0, ReasonPhrase = ex.Message, Body = string.Empty }; + } + } + + public virtual async Task GraphPostAsync(string tenantId, string relativePath, object payload, CancellationToken ct = default, IEnumerable? scopes = null) + { + if (!await EnsureGraphHeadersAsync(tenantId, scopes: scopes, ct: ct)) return null; + var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); + using var content = new StringContent(JsonSerializer.Serialize(payload), Encoding.UTF8, "application/json"); + try + { + using var resp = await _httpClient.PostAsync(url, content, ct); + var body = await resp.Content.ReadAsStringAsync(ct); + if (!resp.IsSuccessStatusCode) + { + var errorMessage = TryExtractGraphErrorMessage(body); + if (errorMessage != null) + _logger.LogWarning("Graph POST {Url} failed: {ErrorMessage}", url, errorMessage); + else + _logger.LogWarning("Graph POST {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); + _logger.LogDebug("Graph POST response body: {Body}", body); + return null; + } + + return string.IsNullOrWhiteSpace(body) ? null : JsonDocument.Parse(body); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "Graph POST {Url} failed with exception", url); + throw; + } + } + + /// + /// POST to Graph but always return HTTP response details (status, body, parsed JSON) + /// + public virtual async Task GraphPostWithResponseAsync(string tenantId, string relativePath, object payload, CancellationToken ct = default, IEnumerable? scopes = null) + { + if (!await EnsureGraphHeadersAsync(tenantId, scopes: scopes, ct: ct)) + { + return new GraphResponse { IsSuccess = false, StatusCode = 0, ReasonPhrase = "NoAuth", Body = "Failed to acquire token" }; + } + + var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); + + using var content = new StringContent(JsonSerializer.Serialize(payload), Encoding.UTF8, "application/json"); + try + { + using var resp = await _httpClient.PostAsync(url, content, ct); + var body = await resp.Content.ReadAsStringAsync(ct); + + JsonDocument? json = null; + if (!string.IsNullOrWhiteSpace(body)) + { + try { json = JsonDocument.Parse(body); } catch { /* ignore parse errors */ } + } + + return new GraphResponse + { + IsSuccess = resp.IsSuccessStatusCode, + StatusCode = (int)resp.StatusCode, + ReasonPhrase = resp.ReasonPhrase ?? string.Empty, + Body = body ?? string.Empty, + Json = json + }; + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "Graph POST {Url} failed with exception", url); + throw; + } + } + + /// + /// Executes a PATCH request to Microsoft Graph API. + /// Virtual to allow mocking in unit tests using Moq. + /// + public virtual async Task GraphPatchAsync(string tenantId, string relativePath, object payload, CancellationToken ct = default, IEnumerable? scopes = null) + { + if (!await EnsureGraphHeadersAsync(tenantId, scopes: scopes, ct: ct)) return false; + var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); + var content = new StringContent(JsonSerializer.Serialize(payload), Encoding.UTF8, "application/json"); + try + { + using var request = new HttpRequestMessage(new HttpMethod("PATCH"), url) { Content = content }; + using var resp = await _httpClient.SendAsync(request, ct); + + // Many PATCH calls return 204 NoContent on success + if (!resp.IsSuccessStatusCode) + { + var body = await resp.Content.ReadAsStringAsync(ct); + var errorMessage = TryExtractGraphErrorMessage(body); + if (errorMessage != null) + _logger.LogError("Graph PATCH {Url} failed: {ErrorMessage}", url, errorMessage); + else + _logger.LogError("Graph PATCH {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); + _logger.LogDebug("Graph PATCH response body: {Body}", body); + } + + return resp.IsSuccessStatusCode; + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "Graph PATCH {Url} failed with exception", url); + throw; + } + } + + public async Task GraphDeleteAsync( + string tenantId, + string relativePath, + CancellationToken ct = default, + bool treatNotFoundAsSuccess = true, + IEnumerable? scopes = null) + { + if (!await EnsureGraphHeadersAsync(tenantId, scopes: scopes, ct: ct)) return false; + + var url = GraphApiConstants.BuildUrl(_graphBaseUrl, relativePath); + + try + { + using var req = new HttpRequestMessage(HttpMethod.Delete, url); + using var resp = await _httpClient.SendAsync(req, ct); + + // 404 can be considered success for idempotent deletes + if (treatNotFoundAsSuccess && (int)resp.StatusCode == 404) return true; + + if (!resp.IsSuccessStatusCode) + { + var body = await resp.Content.ReadAsStringAsync(ct); + var errorMessage = TryExtractGraphErrorMessage(body); + if (errorMessage != null) + _logger.LogError("Graph DELETE {Url} failed: {ErrorMessage}", url, errorMessage); + else + _logger.LogError("Graph DELETE {Url} failed {Code} {Reason}", url, (int)resp.StatusCode, resp.ReasonPhrase); + _logger.LogDebug("Graph DELETE response body: {Body}", body); + return false; + } + + return true; + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + if (NetworkHelper.IsConnectionResetByProxy(ex)) + _logger.LogWarning(NetworkHelper.ConnectionResetWarning); + else + _logger.LogDebug(ex, "Graph DELETE {Url} failed with exception", url); + throw; + } + } + + /// + /// Looks up a service principal by its application (client) ID. + /// Virtual to allow mocking in unit tests using Moq. + /// + public virtual async Task LookupServicePrincipalByAppIdAsync( + string tenantId, string appId, CancellationToken ct = default, IEnumerable? scopes = null) + { + // $filter=appId eq is "Default+Advanced" per Graph docs -� no ConsistencyLevel header required. + // The token must have Application.Read.All; pass scopes to ensure MSAL token is used when needed. + using var doc = await GraphGetAsync( + tenantId, + $"/v1.0/servicePrincipals?$filter=appId eq '{appId}'&$select=id", + ct, + scopes); + if (doc == null) return null; + if (!doc.RootElement.TryGetProperty("value", out var value) || value.GetArrayLength() == 0) return null; + return value[0].GetProperty("id").GetString(); + } + + /// + /// Looks up the display name of a service principal by its application ID. + /// Returns null if the service principal is not found. + /// Virtual to allow substitution in unit tests using NSubstitute. + /// + public virtual async Task GetServicePrincipalDisplayNameAsync( + string tenantId, string appId, CancellationToken ct = default, IEnumerable? scopes = null) + { + // Validate GUID format to prevent OData injection + if (!Guid.TryParse(appId, out var validGuid)) + { + _logger.LogWarning("Invalid appId format for service principal lookup: {AppId}", appId); + return null; + } + + // Use validated GUID in normalized format to prevent OData injection + using var doc = await GraphGetAsync(tenantId, $"/v1.0/servicePrincipals?$filter=appId eq '{validGuid:D}'&$select=displayName", ct, scopes); + if (doc == null) return null; + if (!doc.RootElement.TryGetProperty("value", out var value) || value.GetArrayLength() == 0) return null; + if (!value[0].TryGetProperty("displayName", out var displayName)) return null; + return displayName.GetString(); + } + + /// + /// Ensures a service principal exists for the given application ID. + /// Creates the service principal if it doesn't already exist. + /// Virtual to allow mocking in unit tests using Moq. + /// + public virtual async Task EnsureServicePrincipalForAppIdAsync( + string tenantId, string appId, CancellationToken ct = default, IEnumerable? scopes = null) + { + // Try existing + var spId = await LookupServicePrincipalByAppIdAsync(tenantId, appId, ct, scopes); + if (!string.IsNullOrWhiteSpace(spId)) return spId!; + + // Create SP for this application + var created = await GraphPostAsync(tenantId, "/v1.0/servicePrincipals", new { appId }, ct, scopes); + if (created == null || !created.RootElement.TryGetProperty("id", out var idProp)) + throw new InvalidOperationException($"Failed to create servicePrincipal for appId {appId}"); + + return idProp.GetString()!; + } + + public async Task CreateOrUpdateOauth2PermissionGrantAsync( + string tenantId, + string clientSpObjectId, + string resourceSpObjectId, + IEnumerable scopes, + CancellationToken ct = default, + IEnumerable? permissionGrantScopes = null) + { + var desiredScopeString = string.Join(' ', scopes); + + // Read existing — extract string values immediately so JsonDocument can be disposed + string? existingId = null; + string existingScopes = ""; + + using (var listDoc = await GraphGetAsync( + tenantId, + $"/v1.0/oauth2PermissionGrants?$filter=clientId eq '{clientSpObjectId}' and resourceId eq '{resourceSpObjectId}'", + ct, + permissionGrantScopes)) + { + if (listDoc?.RootElement.TryGetProperty("value", out var arr) == true && arr.GetArrayLength() > 0) + { + var grant = arr[0]; + existingId = grant.TryGetProperty("id", out var idProp) ? idProp.GetString() : null; + existingScopes = grant.TryGetProperty("scope", out var scopeProp) ? scopeProp.GetString() ?? "" : ""; + } + } + + if (string.IsNullOrWhiteSpace(existingId)) + { + // AllPrincipals (tenant-wide) grants require Global Administrator. + // Only called from admin paths (setup admin or setup all run by GA). + var payload = new + { + clientId = clientSpObjectId, + consentType = "AllPrincipals", + resourceId = resourceSpObjectId, + scope = desiredScopeString + }; + + _logger.LogDebug("Graph POST /v1.0/oauth2PermissionGrants body: {Body}", JsonSerializer.Serialize(payload)); + + // A freshly-created service principal may not yet be visible to the + // oauth2PermissionGrants replica (Directory_ObjectNotFound). Retry with + // exponential back-off so the command is self-healing without user intervention. + const int maxRetries = 8; + const int baseDelaySeconds = 5; + for (int attempt = 0; attempt < maxRetries; attempt++) + { + var grantResponse = await GraphPostWithResponseAsync(tenantId, "/v1.0/oauth2PermissionGrants", payload, ct, permissionGrantScopes); + // Dispose the error JSON immediately — only IsSuccess and Body are needed below. + grantResponse.Json?.Dispose(); + + if (grantResponse.IsSuccess) + return true; + + if (!grantResponse.Body.Contains("Directory_ObjectNotFound", StringComparison.OrdinalIgnoreCase)) + return false; // non-transient error, do not retry + + if (attempt < maxRetries - 1) + { + var delaySecs = (int)Math.Min(baseDelaySeconds * Math.Pow(2, attempt), 60); + _logger.LogWarning( + "Service principal not yet replicated to grants endpoint — retrying in {Delay}s (attempt {Attempt}/{Max})...", + delaySecs, attempt + 1, maxRetries - 1); + await Task.Delay(TimeSpan.FromSeconds(delaySecs), ct); + } + } + + _logger.LogWarning( + "OAuth2 permission grant failed after {MaxRetries} retries — service principal may still be propagating. " + + "Re-run 'a365 setup admin' to retry.", + maxRetries); + return false; + } + + // Merge scopes if needed + var currentSet = new HashSet(existingScopes.Split(' ', StringSplitOptions.RemoveEmptyEntries), StringComparer.OrdinalIgnoreCase); + var desiredSet = new HashSet(desiredScopeString.Split(' ', StringSplitOptions.RemoveEmptyEntries), StringComparer.OrdinalIgnoreCase); + + if (desiredSet.IsSubsetOf(currentSet)) return true; // already satisfied + + currentSet.UnionWith(desiredSet); + var merged = string.Join(' ', currentSet); + + return await GraphPatchAsync(tenantId, $"/v1.0/oauth2PermissionGrants/{existingId}", new { scope = merged }, ct, permissionGrantScopes); + } + + /// + /// Checks if the current user has sufficient privileges to create service principals. + /// Virtual to allow mocking in unit tests using Moq. + /// + /// The tenant ID + /// Cancellation token + /// True if user has required roles, false otherwise + public virtual async Task<(bool hasPrivileges, List roles)> CheckServicePrincipalCreationPrivilegesAsync( + string tenantId, + CancellationToken ct = default) + { + try + { + _logger.LogDebug("Checking user's directory roles for service principal creation privileges"); + + var token = await GetGraphAccessTokenAsync(tenantId, ct: ct); + if (token == null) + { + _logger.LogWarning("Could not acquire Graph token to check privileges"); + return (false, new List()); + } + + // Trim token to remove any newline characters that may cause header validation errors + token = token.Trim(); + + using var request = new HttpRequestMessage(HttpMethod.Get, + $"{_graphBaseUrl}/v1.0/me/memberOf/microsoft.graph.directoryRole"); + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + + using var response = await _httpClient.SendAsync(request, ct); + if (!response.IsSuccessStatusCode) + { + _logger.LogWarning("Could not retrieve user's directory roles: {Status}", response.StatusCode); + return (false, new List()); + } + + var json = await response.Content.ReadAsStringAsync(ct); + var doc = JsonDocument.Parse(json); + + var roles = new List(); + if (doc.RootElement.TryGetProperty("value", out var rolesArray)) + { + roles = rolesArray.EnumerateArray() + .Where(role => role.TryGetProperty("displayName", out var displayName)) + .Select(role => role.GetProperty("displayName").GetString()) + .Where(roleName => !string.IsNullOrEmpty(roleName)) + .ToList()!; + } + + _logger.LogDebug("User has {Count} directory roles", roles.Count); + + // Check for required roles + var requiredRoles = new[] + { + "Application Administrator", + "Cloud Application Administrator", + "Global Administrator" + }; + + var hasRequiredRole = roles.Any(r => requiredRoles.Contains(r, StringComparer.OrdinalIgnoreCase)); + + if (hasRequiredRole) + { + _logger.LogDebug("User has sufficient privileges for service principal creation"); + } + else + { + _logger.LogDebug("User does not have required roles for service principal creation. Roles: {Roles}", + string.Join(", ", roles)); + } + + return (hasRequiredRole, roles); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Failed to check service principal creation privileges: {Message}", ex.Message); + return (false, new List()); + } + } + + /// + /// Checks if a user is an owner of an application (read-only validation). + /// Does not attempt to add the user as owner, only verifies ownership. + /// + /// The tenant ID + /// The application object ID (not the client/app ID) + /// The user's object ID to check. If null, uses the current authenticated user. + /// Cancellation token + /// OAuth2 scopes for elevated permissions (e.g., Application.ReadWrite.All, Directory.ReadWrite.All) + /// True if the user is an owner, false otherwise + public virtual async Task IsApplicationOwnerAsync( + string tenantId, + string applicationObjectId, + string? userObjectId = null, + CancellationToken ct = default, + IEnumerable? scopes = null) + { + try + { + // Get current user's object ID if not provided + if (string.IsNullOrWhiteSpace(userObjectId)) + { + if (!await EnsureGraphHeadersAsync(tenantId, scopes: scopes, ct: ct)) + { + _logger.LogWarning("Could not acquire Graph token to check application owner"); + return false; + } + + using var meRequest = new HttpRequestMessage(HttpMethod.Get, + $"{_graphBaseUrl}/v1.0/me?$select=id"); + meRequest.Headers.Authorization = _httpClient.DefaultRequestHeaders.Authorization; + + using var meResponse = await _httpClient.SendAsync(meRequest, ct); + if (!meResponse.IsSuccessStatusCode) + { + _logger.LogWarning("Could not retrieve current user's ID: {Status}", meResponse.StatusCode); + return false; + } + + var meJson = await meResponse.Content.ReadAsStringAsync(ct); + using var meDoc = JsonDocument.Parse(meJson); + + if (!meDoc.RootElement.TryGetProperty("id", out var idElement)) + { + _logger.LogWarning("Could not extract user ID from Graph response"); + return false; + } + + userObjectId = idElement.GetString(); + } + + if (string.IsNullOrWhiteSpace(userObjectId)) + { + _logger.LogWarning("User object ID is empty, cannot check owner"); + return false; + } + + // Check if user is an owner + _logger.LogDebug("Checking if user {UserId} is an owner of application {AppObjectId}", userObjectId, applicationObjectId); + + var ownersDoc = await GraphGetAsync(tenantId, $"/v1.0/applications/{applicationObjectId}/owners?$select=id", ct, scopes); + if (ownersDoc != null && ownersDoc.RootElement.TryGetProperty("value", out var ownersArray)) + { + var isOwner = ownersArray.EnumerateArray() + .Where(owner => owner.TryGetProperty("id", out var ownerId)) + .Any(owner => string.Equals(owner.GetProperty("id").GetString(), userObjectId, StringComparison.OrdinalIgnoreCase)); + + return isOwner; + } + + return false; + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Error checking if user is owner of application: {Message}", ex.Message); + return false; + } + } + + /// + /// Checks whether the currently signed-in user holds the Global Administrator role, + /// which is required to grant tenant-wide admin consent interactively. + /// Uses only — works for both admin and non-admin users. + /// Returns (non-blocking) if the check cannot be completed. + /// + public virtual async Task IsCurrentUserAdminAsync( + string tenantId, + CancellationToken ct = default) + { + return await CheckDirectoryRoleAsync(tenantId, AuthenticationConstants.GlobalAdminRoleTemplateId, ct); + } + + /// + /// Checks whether the currently signed-in user holds the Agent ID Administrator role, + /// which is required to create or update inheritable permissions on agent blueprints. + /// Uses only — works for both admin and non-admin users. + /// Returns (non-blocking) if the check cannot be completed. + /// + public virtual async Task IsCurrentUserAgentIdAdminAsync( + string tenantId, + CancellationToken ct = default) + { + return await CheckDirectoryRoleAsync(tenantId, AuthenticationConstants.AgentIdAdminRoleTemplateId, ct); + } + + /// + /// Returns if the role is confirmed active, + /// if confirmed absent, or + /// if the check itself failed (e.g. network error, + /// throttling, auth failure) — in which case the caller should attempt the operation + /// anyway and let the API surface the real error. + /// Queries /me/transitiveMemberOf/microsoft.graph.directoryRole, which requires only + /// User.Read and succeeds for both admin and non-admin users. + /// Note: PIM-eligible-but-not-activated assignments are not considered active. + /// + private async Task CheckDirectoryRoleAsync(string tenantId, string roleTemplateId, CancellationToken ct) + { + try + { + // /me/transitiveMemberOf is a directory query — Directory.Read.All is required. + // User.Read is insufficient and would return Unknown for most users. + IEnumerable? scopes = _tokenProvider != null + ? [AuthenticationConstants.DirectoryReadAllScope] + : null; + + string? nextUrl = "/v1.0/me/transitiveMemberOf/microsoft.graph.directoryRole?$select=roleTemplateId"; + + while (nextUrl != null) + { + using var doc = await GraphGetAsync(tenantId, nextUrl, ct, scopes); + + if (doc == null) + return Models.RoleCheckResult.Unknown; + + if (!doc.RootElement.TryGetProperty("value", out var roles)) + { + _logger.LogWarning("Unexpected Graph response shape — 'value' property missing from transitiveMemberOf response."); + return Models.RoleCheckResult.Unknown; + } + + if (roles.EnumerateArray().Any(r => + r.TryGetProperty("roleTemplateId", out var id) && + string.Equals(id.GetString(), roleTemplateId, StringComparison.OrdinalIgnoreCase))) + return Models.RoleCheckResult.HasRole; + + nextUrl = doc.RootElement.TryGetProperty("@odata.nextLink", out var nextLink) + ? nextLink.GetString() + : null; + } + + return Models.RoleCheckResult.DoesNotHaveRole; + } + catch (Exception ex) + { + _logger.LogDebug(ex, "Role check for {TemplateId} failed — will attempt operation anyway: {Message}", + roleTemplateId, ex.Message); + return Models.RoleCheckResult.Unknown; + } + } + + /// + /// Resolves the Azure CLI login hint once per instance from 'az account show'. + /// The hint is passed to MSAL so that WAM and silent auth target the correct + /// Azure CLI identity instead of the Windows default account. + /// Returns null if az account show fails or the user field is absent. + /// + private async Task ResolveLoginHintAsync() + { + if (_loginHintResolved) + return _loginHint; + + _loginHintResolved = true; + _loginHint = await _loginHintResolver(); + return _loginHint; + } + + /// + /// Attempts to extract a human-readable error message from a Graph API JSON error response body. + /// Returns null if the body cannot be parsed or does not contain an error message. + /// + private static string? TryExtractGraphErrorMessage(string body) + { + if (string.IsNullOrWhiteSpace(body)) return null; + try + { + using var doc = JsonDocument.Parse(body); + if (doc.RootElement.TryGetProperty("error", out var error) && + error.TryGetProperty("message", out var msg)) + { + return msg.GetString(); + } + } + catch { /* ignore parse errors */ } + return null; + } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/IMicrosoftGraphTokenProvider.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/IMicrosoftGraphTokenProvider.cs index e64c711e..df1c41d0 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/IMicrosoftGraphTokenProvider.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/IMicrosoftGraphTokenProvider.cs @@ -23,5 +23,6 @@ public interface IMicrosoftGraphTokenProvider bool useDeviceCode = false, string? clientAppId = null, CancellationToken ct = default, - string? loginHint = null); + string? loginHint = null, + bool forceRefresh = false); } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/MicrosoftGraphTokenProvider.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/MicrosoftGraphTokenProvider.cs index b988ee49..a4caa792 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/MicrosoftGraphTokenProvider.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/MicrosoftGraphTokenProvider.cs @@ -91,11 +91,12 @@ public MicrosoftGraphTokenProvider( bool useDeviceCode = false, string? clientAppId = null, CancellationToken ct = default, - string? loginHint = null) + string? loginHint = null, + bool forceRefresh = false) { var validatedScopes = ValidateAndPrepareScopes(scopes); ValidateTenantId(tenantId); - + if (!string.IsNullOrWhiteSpace(clientAppId)) { ValidateClientAppId(clientAppId); @@ -104,6 +105,10 @@ public MicrosoftGraphTokenProvider( var cacheKey = MakeCacheKey(tenantId, validatedScopes, clientAppId, loginHint); var tokenExpirationMinutes = AuthenticationConstants.TokenExpirationBufferMinutes; + // When forceRefresh is requested, evict the cached entry so the acquire path always runs. + if (forceRefresh) + _tokenCache.TryRemove(cacheKey, out _); + // Fast path: cached + not expiring soon if (_tokenCache.TryGetValue(cacheKey, out var cached) && cached.ExpiresOnUtc > DateTimeOffset.UtcNow.AddMinutes(tokenExpirationMinutes) && @@ -119,7 +124,7 @@ public MicrosoftGraphTokenProvider( await gate.WaitAsync(ct); try { - // Re-check inside lock + // Re-check inside lock (forceRefresh already evicted the entry above the lock) if (_tokenCache.TryGetValue(cacheKey, out cached) && cached.ExpiresOnUtc > DateTimeOffset.UtcNow.AddMinutes(tokenExpirationMinutes) && !string.IsNullOrWhiteSpace(cached.AccessToken)) diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs index f2d6d4d7..4217fff6 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs @@ -52,7 +52,7 @@ public BlueprintSubcommandTests() _mockPlatformDetector = Substitute.ForPartsOf(mockPlatformDetectorLogger); _mockBotConfigurator = Substitute.For(); _mockGraphApiService = Substitute.ForPartsOf( - Substitute.For>(), _mockExecutor); + Substitute.For>(), _mockExecutor, (Func>)(() => Task.FromResult(null))); _mockBlueprintService = Substitute.ForPartsOf(Substitute.For>(), _mockGraphApiService); _mockClientAppValidator = Substitute.For(); _mockBlueprintLookupService = Substitute.ForPartsOf(Substitute.For>(), _mockGraphApiService); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AuthenticationServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AuthenticationServiceTests.cs index 4d29a9ec..1a8217a5 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AuthenticationServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AuthenticationServiceTests.cs @@ -985,8 +985,10 @@ await act.Should().ThrowAsync( private static string BuildJwt(object payload) { var json = System.Text.Json.JsonSerializer.Serialize(payload); - var payloadB64 = Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes(json)).TrimEnd('='); - return $"header.{payloadB64}.signature"; + // JWT uses Base64Url: replace standard Base64 chars to match real MSAL token format. + var payloadB64Url = Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes(json)) + .Replace('+', '-').Replace('/', '_').TrimEnd('='); + return $"header.{payloadB64Url}.signature"; } private void WriteTokenCache(string accessToken) 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 3da5c7de..cb8bacf6 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 @@ -129,8 +129,9 @@ public async Task EnsureValidClientAppAsync_WhenGraphQueryFails_ThrowsClientAppV _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 = false, @@ -463,8 +464,9 @@ private void SetupAppInfoGet(string appId, string requiredResourceAccess = "[]") _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, @@ -481,8 +483,9 @@ private void SetupAppInfoGetEmpty() _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, From cadebaacd41798fe4a6338a5968cf138cd7ec39e Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Fri, 27 Mar 2026 10:03:53 -0700 Subject: [PATCH 3/3] Lower log level to Debug; add forceRefresh cache bypass test Changed log statements in AuthenticationService from Information to Debug to reduce log verbosity. Added a unit test to ensure forceRefresh bypasses the token cache in MicrosoftGraphTokenProvider. --- .../Services/AuthenticationService.cs | 14 ++++---- .../MicrosoftGraphTokenProviderTests.cs | 34 +++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs index c3947fd4..14cf72ce 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs @@ -124,14 +124,14 @@ public async Task GetAccessTokenAsync( } else { - _logger.LogInformation("Using cached authentication token for {ResourceUrl} (tenant: {TenantId})", + _logger.LogDebug("Using cached authentication token for {ResourceUrl} (tenant: {TenantId})", resourceUrl, tenantId); return cachedToken.AccessToken; } } else { - _logger.LogInformation("Using cached authentication token for {ResourceUrl}", resourceUrl); + _logger.LogDebug("Using cached authentication token for {ResourceUrl}", resourceUrl); return cachedToken.AccessToken; } } @@ -187,8 +187,8 @@ private async Task AuthenticateInteractivelyAsync( // This creates the format required by Azure AD for the TokenRequestContext: {resourceAppId}/{scope} // Example: "ea9ffc3e-8a23-4a7d-836d-234d7c7565c1/McpServers.Mail.All" scopes = explicitScopes.Select(s => $"{resourceUrl}/{s}").ToArray(); - _logger.LogInformation("Using explicit scopes for authentication: {Scopes}", string.Join(", ", explicitScopes)); - _logger.LogInformation("Formatted as: {FormattedScopes}", string.Join(", ", scopes)); + _logger.LogDebug("Using explicit scopes for authentication: {Scopes}", string.Join(", ", explicitScopes)); + _logger.LogDebug("Formatted as: {FormattedScopes}", string.Join(", ", scopes)); } else { @@ -225,13 +225,13 @@ private async Task AuthenticateInteractivelyAsync( scope = resourceUrl.EndsWith("/.default", StringComparison.OrdinalIgnoreCase) ? resourceUrl : $"{resourceUrl}/.default"; - _logger.LogInformation("Using custom resource for authentication: {Resource}", resourceUrl); + _logger.LogDebug("Using custom resource for authentication: {Resource}", resourceUrl); } scopes = [scope]; - _logger.LogInformation($"Token scope: {scope}"); + _logger.LogDebug("Token scope: {Scope}", scope); } - _logger.LogInformation("Authenticating for tenant: {TenantId}", effectiveTenantId); + _logger.LogDebug("Authenticating for tenant: {TenantId}", effectiveTenantId); // Use provided client ID or default to PowerShell client ID effectiveClientId = string.IsNullOrWhiteSpace(clientId) diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/MicrosoftGraphTokenProviderTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/MicrosoftGraphTokenProviderTests.cs index 66b68b96..dd87869b 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/MicrosoftGraphTokenProviderTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/MicrosoftGraphTokenProviderTests.cs @@ -348,4 +348,38 @@ public async Task GetMgGraphAccessTokenAsync_EscapesSingleQuotesInClientAppId() // Assert token.Should().Be(expectedToken); } + + [Fact] + public async Task GetMgGraphAccessTokenAsync_WithForceRefresh_BypassesCache() + { + // Arrange + var tenantId = "12345678-1234-1234-1234-123456789abc"; + var scopes = new[] { "AgentIdentityBlueprint.DeleteRestore.All" }; + var clientAppId = "87654321-4321-4321-4321-cba987654321"; + // Valid JWT with a future exp claim (year 2099) + var msalToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzZWxsYWsiLCJleHAiOjQwNzA5MDg4MDB9.signature"; + var callCount = 0; + + var provider = new MicrosoftGraphTokenProvider(_executor, _logger) + { + MsalTokenAcquirerOverride = (_, _, _, _) => + { + callCount++; + return Task.FromResult(msalToken); + } + }; + + // Prime the cache with a first call + await provider.GetMgGraphAccessTokenAsync(tenantId, scopes, false, clientAppId); + callCount.Should().Be(1); + + // Act — second call with forceRefresh: true should bypass the cache + var token = await provider.GetMgGraphAccessTokenAsync(tenantId, scopes, false, clientAppId, forceRefresh: true); + + // Assert + token.Should().Be(msalToken); + callCount.Should().Be(2, + because: "forceRefresh: true must evict the cached token and re-invoke MSAL, " + + "ensuring a stale CAE-revoked token is not reused"); + } }