diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index de8a486..efaf3aa 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,4 +1,52 @@ -# GitHub Copilot Instructions for Agent365-dotnet +# GitHub Copilot Instructions for Agent365-devTools + +## Agent365 CLI Development Guidelines + +### Engineering Principles +- Follow KISS, DRY, SOLID, and YAGNI principles +- Align CLI patterns with Azure CLI (`az`) conventions +- Keep changes minimal and focused on the problem at hand +- Reuse existing functions across commands; avoid duplication +- Critically review all changes before committing + +### Code Organization +- Keep files small and focused +- Use constants for strings and values (see `Constants/` folder) +- Use `ErrorCodes.cs` and `ErrorMessages.cs` for error handling + +### File Organization Guidelines + +#### Multiple Classes Per File - Allowed Cases +- **Model/DTO files**: Related model classes, records, or structs can be grouped in a single file +- **Request/Response pairs**: API request and response classes for the same endpoint +- **Small supporting types**: Enums, small records, or helper classes closely tied to a main class +- **Nested or related interfaces**: Interface and its related types + +#### When to Separate +- Large classes with significant logic +- Classes that could be reused independently +- Classes with different lifecycle or ownership + +### Cross-Platform Compatibility +- All code must work on Windows, macOS, and Linux +- Test file paths, line endings, and shell commands for compatibility + +### Testing Standards +- Use xUnit, FluentAssertions, and NSubstitute +- Focus on quality over quantity of tests +- Add regression tests for bug fixes +- Tests should verify CLI reliability + +### Output and Logging +- No emojis or special characters in logs, output, or comments +- Keep user-facing messages clear and professional +- Follow client-facing help text conventions + +### Code Review Mindset +- Be cautious about deleting code; avoid `git restore` without review +- Do not create unnecessary documentation files + +--- ## Code Review Rules diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 60a003d..f0dae82 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -14,6 +14,7 @@ + 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 109ec68..f99469c 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs @@ -1214,19 +1214,18 @@ await SetupHelpers.EnsureResourcePermissionsAsync( /// /// Extracts the access token from a GraphServiceClient for use in direct HTTP calls. - /// This uses InteractiveBrowserCredential directly which is simpler and more reliable. + /// This uses MsalBrowserCredential, which performs platform-appropriate interactive authentication (WAM on Windows, browser-based flow on other platforms). /// private static async Task GetTokenFromGraphClient(ILogger logger, GraphServiceClient graphClient, string tenantId, string clientAppId) { try { - // Use Azure.Identity to get the token directly - // This is cleaner and more reliable than trying to extract it from GraphServiceClient - var credential = new InteractiveBrowserCredential(new InteractiveBrowserCredentialOptions - { - TenantId = tenantId, - ClientId = clientAppId - }); + // Use MsalBrowserCredential which handles WAM on Windows and browser on other platforms + var credential = new MsalBrowserCredential( + clientAppId, + tenantId, + redirectUri: null, // Let MsalBrowserCredential use WAM on Windows + logger); var tokenRequestContext = new TokenRequestContext(new[] { "https://graph.microsoft.com/.default" }); var token = await credential.GetTokenAsync(tokenRequestContext, CancellationToken.None); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs index 4e68601..4211d66 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs @@ -39,6 +39,30 @@ public static class AuthenticationConstants "http://localhost:8400/" }; + /// + /// WAM (Windows Authentication Broker) redirect URI format. + /// This URI is required for WAM-based authentication on Windows. + /// The {0} placeholder should be replaced with the client app ID. + /// + public const string WamBrokerRedirectUriFormat = "ms-appx-web://microsoft.aad.brokerplugin/{0}"; + + /// + /// Gets all required redirect URIs including the WAM broker URI for a specific client app. + /// Note: This method allocates a new array on each call. Callers should cache the result + /// if they need to use it multiple times. + /// + /// The client application ID + /// Array of all required redirect URIs + public static string[] GetRequiredRedirectUris(string clientAppId) + { + var uris = new List(RequiredRedirectUris); + if (!string.IsNullOrWhiteSpace(clientAppId)) + { + uris.Add(string.Format(WamBrokerRedirectUriFormat, clientAppId)); + } + return uris.ToArray(); + } + /// /// Application name for cache directory /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Microsoft.Agents.A365.DevTools.Cli.csproj b/src/Microsoft.Agents.A365.DevTools.Cli/Microsoft.Agents.A365.DevTools.Cli.csproj index a2c21a1..9789baa 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Microsoft.Agents.A365.DevTools.Cli.csproj +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Microsoft.Agents.A365.DevTools.Cli.csproj @@ -22,6 +22,7 @@ + diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs index 3058d88..2c592f7 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs @@ -193,28 +193,20 @@ private async Task AuthenticateInteractivelyAsync( if (useInteractiveBrowser) { - // Use InteractiveBrowserCredential with redirect URI for better public client support - _logger.LogInformation("Using interactive browser authentication..."); - _logger.LogInformation("IMPORTANT: A browser window will open for authentication."); + // Use MsalBrowserCredential which handles WAM on Windows and browser on other platforms + _logger.LogInformation("Using interactive authentication..."); _logger.LogInformation("Please sign in with your Microsoft account and grant consent for the requested permissions."); _logger.LogInformation(""); - credential = new InteractiveBrowserCredential(new InteractiveBrowserCredentialOptions - { - TenantId = effectiveTenantId, - ClientId = effectiveClientId, - AuthorityHost = AzureAuthorityHosts.AzurePublicCloud, - RedirectUri = new Uri(AuthenticationConstants.LocalhostRedirectUri), - TokenCachePersistenceOptions = new TokenCachePersistenceOptions - { - Name = AuthenticationConstants.ApplicationName - } - }); + credential = new MsalBrowserCredential( + effectiveClientId, + effectiveTenantId, + redirectUri: null, // Let MsalBrowserCredential use WAM on Windows + _logger); } else { - // For Power Platform API authentication, use device code flow to avoid URL length issues - // InteractiveBrowserCredential with Power Platform scopes can create URLs that exceed browser limits + // Device code flow - works in all environments including SSH/remote sessions _logger.LogInformation("Using device code authentication..."); _logger.LogInformation("Please sign in with your Microsoft account"); @@ -254,12 +246,12 @@ private async Task AuthenticateInteractivelyAsync( TenantId = effectiveTenantId }; } - catch (AuthenticationFailedException ex) when (ex.Message.Contains("code_expired") || ex.InnerException?.Message.Contains("code_expired") == true) + catch (MsalAuthenticationFailedException ex) when (ex.Message.Contains("code_expired") || ex.InnerException?.Message.Contains("code_expired") == true) { _logger.LogError("Device code expired - authentication not completed in time"); throw new AzureAuthenticationException("Device code authentication timed out - please complete authentication promptly when retrying"); } - catch (AuthenticationFailedException ex) + catch (MsalAuthenticationFailedException ex) { _logger.LogError("Interactive authentication failed: {Message}", ex.Message); _logger.LogError("Exception type: {Type}", ex.GetType().FullName); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AzureWebAppCreator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AzureWebAppCreator.cs index f9c8800..dcf3c35 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AzureWebAppCreator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AzureWebAppCreator.cs @@ -33,22 +33,22 @@ public async Task CreateWebAppAsync( try { ArmClient armClient; - if (!string.IsNullOrWhiteSpace(tenantId)) + // Use DefaultAzureCredential with InteractiveBrowserCredential excluded to avoid + // Windows Authentication Broker (WAM) issues in console apps. + // Users should run 'az login' before using this command. + // See GitHub issues #146 and #151. + var credentialOptions = new DefaultAzureCredentialOptions { - var credential = new DefaultAzureCredential(new DefaultAzureCredentialOptions - { - VisualStudioTenantId = tenantId, - SharedTokenCacheTenantId = tenantId, - InteractiveBrowserTenantId = tenantId, - ExcludeInteractiveBrowserCredential = false - }); - armClient = new ArmClient(credential, subscriptionId); - } - else + ExcludeInteractiveBrowserCredential = true + }; + + if (!string.IsNullOrWhiteSpace(tenantId)) { - armClient = new ArmClient(new DefaultAzureCredential(), subscriptionId); + credentialOptions.TenantId = tenantId; } + armClient = new ArmClient(new DefaultAzureCredential(credentialOptions), subscriptionId); + var subscription = armClient.GetSubscriptionResource(new ResourceIdentifier($"/subscriptions/{subscriptionId}")); var resourceGroup = await subscription.GetResourceGroups().GetAsync(resourceGroupName); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs index 392ad82..e62de94 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs @@ -195,8 +195,9 @@ public async Task EnsureRedirectUrisAsync( .Select(uri => uri!) .ToHashSet(StringComparer.OrdinalIgnoreCase) ?? new HashSet(StringComparer.OrdinalIgnoreCase); - // Check if required URIs are present - var missingUris = AuthenticationConstants.RequiredRedirectUris + // Check if required URIs are present (including WAM broker URI) + var requiredUris = AuthenticationConstants.GetRequiredRedirectUris(clientAppId); + var missingUris = requiredUris .Where(uri => !currentRedirectUris.Contains(uri)) .ToList(); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/InteractiveGraphAuthService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/InteractiveGraphAuthService.cs index 91072dd..7112c1f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/InteractiveGraphAuthService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/InteractiveGraphAuthService.cs @@ -7,6 +7,7 @@ using Microsoft.Agents.A365.DevTools.Cli.Exceptions; using Microsoft.Extensions.Logging; using Microsoft.Graph; +using Microsoft.Identity.Client; namespace Microsoft.Agents.A365.DevTools.Cli.Services; @@ -78,7 +79,7 @@ public Task GetAuthenticatedGraphClientAsync( _logger.LogInformation("Attempting to authenticate to Microsoft Graph interactively..."); _logger.LogInformation("This requires permissions defined in AuthenticationConstants.RequiredClientAppPermissions for Agent Blueprint operations."); _logger.LogInformation(""); - _logger.LogInformation("IMPORTANT: A browser window will open for authentication."); + _logger.LogInformation("IMPORTANT: Interactive authentication is required."); _logger.LogInformation("Please sign in with an account that has Global Administrator or similar privileges."); _logger.LogInformation(""); @@ -88,19 +89,15 @@ public Task GetAuthenticatedGraphClientAsync( try { - var browserCredential = new InteractiveBrowserCredential(new InteractiveBrowserCredentialOptions - { - TenantId = tenantId, - ClientId = _clientAppId, - AuthorityHost = AzureAuthorityHosts.AzurePublicCloud, - RedirectUri = new Uri(AuthenticationConstants.LocalhostRedirectUri), - TokenCachePersistenceOptions = new TokenCachePersistenceOptions - { - Name = AuthenticationConstants.ApplicationName - } - }); + // Use MsalBrowserCredential which handles WAM on Windows and browser on other platforms + // Pass null for redirectUri to let MsalBrowserCredential decide based on platform + var browserCredential = new MsalBrowserCredential( + _clientAppId, + tenantId, + redirectUri: null, // Let MsalBrowserCredential use WAM on Windows + _logger); - _logger.LogInformation("Opening browser for authentication..."); + _logger.LogInformation("Authenticating to Microsoft Graph..."); _logger.LogInformation("IMPORTANT: You must grant consent for all required permissions."); _logger.LogInformation("Required permissions are defined in AuthenticationConstants.RequiredClientAppPermissions."); _logger.LogInformation($"See {ConfigConstants.Agent365CliDocumentationUrl} for the complete list."); @@ -118,13 +115,13 @@ public Task GetAuthenticatedGraphClientAsync( return Task.FromResult(graphClient); } - catch (Azure.Identity.AuthenticationFailedException ex) when (ex.Message.Contains("invalid_grant")) + catch (MsalAuthenticationFailedException ex) when (ex.Message.Contains("invalid_grant")) { // Most specific: permissions issue - don't try fallback ThrowInsufficientPermissionsException(ex); throw; // Unreachable but required for compiler } - catch (Azure.Identity.AuthenticationFailedException ex) when ( + catch (MsalAuthenticationFailedException ex) when ( ex.Message.Contains("localhost") || ex.Message.Contains("connection") || ex.Message.Contains("redirect_uri")) @@ -134,12 +131,12 @@ public Task GetAuthenticatedGraphClientAsync( _logger.LogInformation(""); shouldTryDeviceCode = true; } - catch (Azure.Identity.CredentialUnavailableException) + catch (Microsoft.Identity.Client.MsalServiceException ex) when (ex.ErrorCode == "access_denied") { - _logger.LogError("Interactive browser authentication is not available"); + _logger.LogError("Authentication was denied or cancelled"); throw new GraphApiException( "Interactive browser authentication", - "Not available in non-interactive environments or when browser is unavailable", + "Authentication was denied or cancelled by the user", isPermissionIssue: false); } catch (Exception ex) @@ -151,7 +148,12 @@ public Task GetAuthenticatedGraphClientAsync( isPermissionIssue: false); } - // Fallback to Device Code Flow if browser authentication had infrastructure issues + // DeviceCodeCredential fallback safety net: + // If browser authentication fails due to infrastructure issues (localhost connectivity, + // redirect URI problems, etc.), this fallback provides an alternative authentication path. + // The device code flow displays a code that users can enter at microsoft.com/devicelogin, + // which works even in environments where browser-based OAuth redirects fail. + // This fallback is preserved even after the WAM fix (GitHub issues #146, #151). if (shouldTryDeviceCode) { try diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs new file mode 100644 index 0000000..6b5b10e --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs @@ -0,0 +1,226 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Azure.Core; +using Microsoft.Agents.A365.DevTools.Cli.Constants; +using Microsoft.Extensions.Logging; +using Microsoft.Identity.Client; +using Microsoft.Identity.Client.Broker; +using System.Diagnostics; +using System.Runtime.InteropServices; +using System.Runtime.Versioning; + +namespace Microsoft.Agents.A365.DevTools.Cli.Services; + +/// +/// A custom TokenCredential that uses MSAL directly for interactive authentication. +/// On Windows, this uses WAM (Windows Authentication Broker) for a native sign-in experience +/// that doesn't require opening a browser. On other platforms, it falls back to system browser. +/// +/// See: https://learn.microsoft.com/en-us/entra/msal/dotnet/acquiring-tokens/desktop-mobile/wam +/// Fixes GitHub issues #146 and #151. +/// +public sealed class MsalBrowserCredential : TokenCredential +{ + private readonly IPublicClientApplication _publicClientApp; + private readonly ILogger? _logger; + private readonly string _tenantId; + private readonly bool _useWam; + private readonly IntPtr _windowHandle; + + // P/Invoke is required for WAM window handle in console applications. + // There is no managed .NET API for console/desktop window handles - these are Windows-specific. + // This is the standard approach documented by Microsoft for WAM integration: + // https://learn.microsoft.com/en-us/entra/msal/dotnet/acquiring-tokens/desktop-mobile/wam + [SupportedOSPlatform("windows")] + [DllImport("kernel32.dll")] + private static extern IntPtr GetConsoleWindow(); + + [SupportedOSPlatform("windows")] + [DllImport("user32.dll")] + private static extern IntPtr GetDesktopWindow(); + + [SupportedOSPlatform("windows")] + [DllImport("user32.dll")] + private static extern IntPtr GetForegroundWindow(); + /// + /// Creates a new instance of MsalBrowserCredential. + /// + /// The application (client) ID. + /// The directory (tenant) ID. + /// The redirect URI for authentication callbacks. + /// Optional logger for diagnostic output. + /// Whether to use WAM on Windows. Default is true. + public MsalBrowserCredential( + string clientId, + string tenantId, + string? redirectUri = null, + ILogger? logger = null, + bool useWam = true) + { + if (string.IsNullOrWhiteSpace(clientId)) + { + throw new ArgumentNullException(nameof(clientId)); + } + + if (string.IsNullOrWhiteSpace(tenantId)) + { + throw new ArgumentNullException(nameof(tenantId)); + } + + _tenantId = tenantId; + _logger = logger; + + // Get window handle for WAM on Windows + // Try multiple sources: console window, foreground window, or desktop window + _windowHandle = IntPtr.Zero; + _useWam = useWam && OperatingSystem.IsWindows(); + + if (OperatingSystem.IsWindows() && _useWam) + { + try + { + _windowHandle = GetWindowHandleForWam(); + _logger?.LogDebug("Window handle for WAM: {Handle}", _windowHandle); + } + catch (Exception ex) + { + _logger?.LogWarning(ex, "Failed to get window handle, falling back to system browser"); + _useWam = false; + } + } + + var builder = PublicClientApplicationBuilder + .Create(clientId) + .WithAuthority(AzureCloudInstance.AzurePublic, tenantId); + + if (_useWam) + { + // Use WAM broker on Windows for native authentication experience + // WAM provides SSO with Windows accounts and doesn't require browser + _logger?.LogDebug("Configuring WAM broker for Windows authentication"); + + var brokerOptions = new BrokerOptions(BrokerOptions.OperatingSystems.Windows) + { + Title = "Agent365 Tools Authentication" + }; + + builder = builder + .WithBroker(brokerOptions) + .WithParentActivityOrWindow(() => _windowHandle) + .WithRedirectUri($"ms-appx-web://microsoft.aad.brokerplugin/{clientId}"); + } + else + { + // Use system browser on non-Windows platforms or when WAM isn't available + _logger?.LogDebug("Using system browser for authentication"); + var effectiveRedirectUri = redirectUri ?? AuthenticationConstants.LocalhostRedirectUri; + builder = builder.WithRedirectUri(effectiveRedirectUri); + } + + _publicClientApp = builder.Build(); + } + + /// + public override AccessToken GetToken(TokenRequestContext requestContext, CancellationToken cancellationToken) + { + return GetTokenAsync(requestContext, cancellationToken).GetAwaiter().GetResult(); + } + + /// + /// Gets a window handle for WAM authentication on Windows. + /// For CLI apps, uses GetConsoleWindow() with GetDesktopWindow() as fallback. + /// + [SupportedOSPlatform("windows")] + private static IntPtr GetWindowHandleForWam() + { + // Try console window first (works for cmd.exe, PowerShell) + var handle = GetConsoleWindow(); + + // If no console window, try foreground window (works for Windows Terminal) + if (handle == IntPtr.Zero) + { + handle = GetForegroundWindow(); + } + + // Last resort: use desktop window (always valid) + if (handle == IntPtr.Zero) + { + handle = GetDesktopWindow(); + } + + + return handle; + } + + /// + public override async ValueTask GetTokenAsync( + TokenRequestContext requestContext, + CancellationToken cancellationToken) + { + var scopes = requestContext.Scopes; + + try + { + // First, try to acquire token silently from cache + var accounts = await _publicClientApp.GetAccountsAsync(); + var account = accounts.FirstOrDefault(); + + if (account != null) + { + try + { + _logger?.LogDebug("Attempting to acquire token silently from cache..."); + var silentResult = await _publicClientApp + .AcquireTokenSilent(scopes, account) + .ExecuteAsync(cancellationToken); + + _logger?.LogDebug("Successfully acquired token from cache."); + return new AccessToken(silentResult.AccessToken, silentResult.ExpiresOn); + } + catch (MsalUiRequiredException) + { + _logger?.LogDebug("Token cache miss or expired, interactive authentication required."); + } + } + + // Acquire token interactively + AuthenticationResult interactiveResult; + + if (_useWam) + { + // WAM on Windows - native authentication dialog, no browser needed + _logger?.LogInformation("Authenticating via Windows Account Manager..."); + interactiveResult = await _publicClientApp + .AcquireTokenInteractive(scopes) + .ExecuteAsync(cancellationToken); + } + else + { + // System browser on Mac/Linux + _logger?.LogInformation("Opening browser for authentication..."); + interactiveResult = await _publicClientApp + .AcquireTokenInteractive(scopes) + .WithUseEmbeddedWebView(false) + .ExecuteAsync(cancellationToken); + } + + _logger?.LogDebug("Successfully acquired token via interactive authentication."); + return new AccessToken(interactiveResult.AccessToken, interactiveResult.ExpiresOn); + } + catch (MsalException ex) + { + _logger?.LogError(ex, "MSAL authentication failed: {Message}", ex.Message); + throw new MsalAuthenticationFailedException($"Failed to acquire token: {ex.Message}", ex); + } + } +} + +/// +/// Exception thrown when MSAL-based authentication fails. +/// +public class MsalAuthenticationFailedException : Exception +{ + public MsalAuthenticationFailedException(string message) : base(message) { } + public MsalAuthenticationFailedException(string message, Exception innerException) : base(message, innerException) { } +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/src.code-workspace b/src/Microsoft.Agents.A365.DevTools.Cli/src.code-workspace new file mode 100644 index 0000000..d19ba6d --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/src.code-workspace @@ -0,0 +1,14 @@ +{ + "folders": [ + { + "path": ".." + }, + { + "path": "../.." + } + ], + "settings": { + "vscode-nmake-tools.installRazzleSourcesFormatterExtension": false, + "vscode-nmake-tools.installOsRepoRustHelperExtension": false + } +} \ No newline at end of file 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 44cce02..7db998b 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 @@ -515,12 +515,14 @@ public async Task EnsureRedirectUrisAsync_WhenAllUrisPresent_DoesNotUpdate() { // Arrange var token = "test-token"; + // Include all required URIs: localhost, localhost:8400, and WAM broker URI + var wamBrokerUri = $"ms-appx-web://microsoft.aad.brokerplugin/{ValidClientAppId}"; var appResponseJson = $$""" { "value": [{ "id": "object-id-123", "publicClient": { - "redirectUris": ["http://localhost", "http://localhost:8400/"] + "redirectUris": ["http://localhost", "http://localhost:8400/", "{{wamBrokerUri}}"] } }] } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/InteractiveGraphAuthServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/InteractiveGraphAuthServiceTests.cs index 5c2294d..37fa6c3 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/InteractiveGraphAuthServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/InteractiveGraphAuthServiceTests.cs @@ -99,4 +99,89 @@ public void Constructor_WithNullLogger_ShouldThrowArgumentNullException() // Act & Assert Assert.Throws(() => new InteractiveGraphAuthService(null!, validGuid)); } + + #region WAM Configuration Tests (GitHub Issues #146 and #151) + + /// + /// Verifies that MsalBrowserCredential can be constructed with valid parameters. + /// + [Fact] + public void MsalBrowserCredential_WithValidParameters_ShouldConstruct() + { + // Arrange + var clientId = "12345678-1234-1234-1234-123456789abc"; + var tenantId = "87654321-4321-4321-4321-cba987654321"; + var redirectUri = "http://localhost:8400"; + + // Act + var credential = new MsalBrowserCredential(clientId, tenantId, redirectUri); + + // Assert + Assert.NotNull(credential); + } + + /// + /// Verifies that MsalBrowserCredential throws on null client ID. + /// + [Fact] + public void MsalBrowserCredential_WithNullClientId_ShouldThrow() + { + // Arrange + var tenantId = "87654321-4321-4321-4321-cba987654321"; + + // Act & Assert + Assert.Throws(() => new MsalBrowserCredential(null!, tenantId)); + } + + /// + /// Verifies that MsalBrowserCredential throws on null tenant ID. + /// + [Fact] + public void MsalBrowserCredential_WithNullTenantId_ShouldThrow() + { + // Arrange + var clientId = "12345678-1234-1234-1234-123456789abc"; + + // Act & Assert + Assert.Throws(() => new MsalBrowserCredential(clientId, null!)); + } + + /// + /// Integration test for WAM configuration - can be run manually to verify the fix. + /// This test is skipped by default in automated runs as it requires user interaction. + /// + /// To run manually: dotnet test --filter "Category=Integration" + /// + [Fact(Skip = "Integration test requires manual verification on Windows 10/11")] + [Trait("Category", "Integration")] + public void MsalBrowserCredential_ManualTest_ShouldUseWAMOnWindows() + { + // This test is marked as Integration and should be skipped in CI/CD pipelines. + // To verify the WAM fix works: + // + // 1. Run this command on Windows 10/11: + // a365 setup all + // + // 2. Expected behavior on Windows: + // - Native WAM dialog appears (Windows Account Manager) + // - No browser window opens + // - WAM broker redirect URI auto-configured: ms-appx-web://microsoft.aad.brokerplugin/{clientId} + // - No "window handle" error + // - No AADSTS50011 redirect URI mismatch error + // + // 3. Expected behavior on macOS/Linux: + // - System browser opens for authentication + // - Uses localhost redirect URI + // + // 4. The implementation uses MSAL with: + // PublicClientApplicationBuilder.Create(clientId) + // .WithAuthority(...) + // .WithBroker(new BrokerOptions(BrokerOptions.OperatingSystems.Windows)) // WAM enabled + // .WithParentActivityOrWindow(() => windowHandle) // P/Invoke for console apps + // .Build() + + Assert.True(true, "Manual verification required"); + } + + #endregion } \ No newline at end of file diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/MsalBrowserCredentialTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/MsalBrowserCredentialTests.cs new file mode 100644 index 0000000..e9c737c --- /dev/null +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/MsalBrowserCredentialTests.cs @@ -0,0 +1,235 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Extensions.Logging; +using NSubstitute; +using System.Runtime.InteropServices; +using Xunit; + +namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Services; + +public class MsalBrowserCredentialTests +{ + private const string ValidClientId = "12345678-1234-1234-1234-123456789abc"; + private const string ValidTenantId = "87654321-4321-4321-4321-cba987654321"; + private const string ValidRedirectUri = "http://localhost:8400"; + + #region Constructor Tests + + [Fact] + public void Constructor_WithValidParameters_ShouldSucceed() + { + // Arrange & Act + var credential = new MsalBrowserCredential(ValidClientId, ValidTenantId, ValidRedirectUri); + + // Assert + Assert.NotNull(credential); + } + + [Fact] + public void Constructor_WithNullRedirectUri_ShouldSucceed() + { + // Arrange & Act - redirectUri is optional + var credential = new MsalBrowserCredential(ValidClientId, ValidTenantId, redirectUri: null); + + // Assert + Assert.NotNull(credential); + } + + [Fact] + public void Constructor_WithLogger_ShouldSucceed() + { + // Arrange + var logger = Substitute.For(); + + // Act + var credential = new MsalBrowserCredential(ValidClientId, ValidTenantId, ValidRedirectUri, logger); + + // Assert + Assert.NotNull(credential); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void Constructor_WithNullOrEmptyClientId_ShouldThrowArgumentNullException(string? clientId) + { + // Act & Assert + Assert.Throws(() => + new MsalBrowserCredential(clientId!, ValidTenantId, ValidRedirectUri)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void Constructor_WithNullOrEmptyTenantId_ShouldThrowArgumentNullException(string? tenantId) + { + // Act & Assert + Assert.Throws(() => + new MsalBrowserCredential(ValidClientId, tenantId!, ValidRedirectUri)); + } + + #endregion + + #region WAM Configuration Tests + + [Fact] + public void Constructor_WithUseWamTrue_OnWindows_ShouldConfigureForWam() + { + // Arrange & Act + var credential = new MsalBrowserCredential( + ValidClientId, + ValidTenantId, + redirectUri: null, // WAM uses broker redirect URI + logger: null, + useWam: true); + + // Assert - credential should be created successfully + // On Windows, WAM will be enabled; on other platforms, it falls back to browser + Assert.NotNull(credential); + } + + [Fact] + public void Constructor_WithUseWamFalse_ShouldConfigureForBrowser() + { + // Arrange & Act + var credential = new MsalBrowserCredential( + ValidClientId, + ValidTenantId, + ValidRedirectUri, + logger: null, + useWam: false); + + // Assert + Assert.NotNull(credential); + } + + [Fact] + public void Constructor_WithUseWamTrue_OnNonWindows_ShouldFallbackToBrowser() + { + // This test verifies the fallback behavior + // On non-Windows platforms, useWam=true should still work by falling back to browser + + // Arrange + var logger = Substitute.For(); + + // Act - should not throw regardless of platform + var credential = new MsalBrowserCredential( + ValidClientId, + ValidTenantId, + ValidRedirectUri, + logger, + useWam: true); + + // Assert + Assert.NotNull(credential); + } + + #endregion + + #region Platform Detection Tests + + [Fact] + public void WamShouldOnlyBeEnabledOnWindows() + { + // This test documents the expected platform behavior: + // - Windows: WAM is enabled (native authentication dialog) + // - macOS/Linux: Browser-based authentication + + var isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); + + // The credential should be constructable on all platforms + var credential = new MsalBrowserCredential( + ValidClientId, + ValidTenantId, + redirectUri: null, + useWam: true); + + Assert.NotNull(credential); + + // Note: We can't directly verify _useWam field as it's private, + // but the constructor should succeed on all platforms + } + + #endregion + + #region Window Handle Fallback Tests (Windows-specific behavior documentation) + + /// + /// Documents the window handle fallback chain used for WAM on Windows: + /// 1. GetConsoleWindow() - Works for cmd.exe, PowerShell + /// 2. GetForegroundWindow() - Works for Windows Terminal + /// 3. GetDesktopWindow() - Always returns a valid handle + /// + /// This test verifies the credential can be constructed, which exercises + /// the window handle detection code on Windows. + /// + [Fact] + public void Constructor_OnWindows_ShouldHandleWindowHandleDetection() + { + // Arrange + var logger = Substitute.For(); + + // Act - On Windows, this exercises the P/Invoke window handle detection + var credential = new MsalBrowserCredential( + ValidClientId, + ValidTenantId, + redirectUri: null, + logger, + useWam: true); + + // Assert + Assert.NotNull(credential); + + // On Windows, logger should have received debug messages about window handle + // On other platforms, WAM is disabled so no window handle detection occurs + } + + #endregion + + #region Exception Type Tests + + [Fact] + public void MsalAuthenticationFailedException_WithMessage_ShouldSetMessage() + { + // Arrange + var message = "Test error message"; + + // Act + var exception = new MsalAuthenticationFailedException(message); + + // Assert + Assert.Equal(message, exception.Message); + Assert.Null(exception.InnerException); + } + + [Fact] + public void MsalAuthenticationFailedException_WithMessageAndInnerException_ShouldSetBoth() + { + // Arrange + var message = "Test error message"; + var innerException = new InvalidOperationException("Inner error"); + + // Act + var exception = new MsalAuthenticationFailedException(message, innerException); + + // Assert + Assert.Equal(message, exception.Message); + Assert.Same(innerException, exception.InnerException); + } + + [Fact] + public void MsalAuthenticationFailedException_ShouldInheritFromException() + { + // Arrange & Act + var exception = new MsalAuthenticationFailedException("Test"); + + // Assert + Assert.IsAssignableFrom(exception); + } + + #endregion +}