Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 49 additions & 1 deletion .github/copilot-instructions.md
Original file line number Diff line number Diff line change
@@ -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

Expand Down
1 change: 1 addition & 0 deletions src/Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<PackageVersion Include="System.Text.Json" Version="9.0.8" />
<!-- Security -->
<PackageVersion Include="Microsoft.Identity.Client" Version="4.78.0" />
<PackageVersion Include="Microsoft.Identity.Client.Broker" Version="4.78.0" />
<PackageVersion Include="System.Security.Claims" Version="4.3.0" />
<PackageVersion Include="System.Security.Principal" Version="4.3.0" />
<!-- Logging -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1214,19 +1214,18 @@ await SetupHelpers.EnsureResourcePermissionsAsync(

/// <summary>
/// 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).
/// </summary>
private static async Task<string?> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,30 @@ public static class AuthenticationConstants
"http://localhost:8400/"
};

/// <summary>
/// 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.
/// </summary>
public const string WamBrokerRedirectUriFormat = "ms-appx-web://microsoft.aad.brokerplugin/{0}";

/// <summary>
/// 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.
/// </summary>
/// <param name="clientAppId">The client application ID</param>
/// <returns>Array of all required redirect URIs</returns>
public static string[] GetRequiredRedirectUris(string clientAppId)
{
var uris = new List<string>(RequiredRedirectUris);
if (!string.IsNullOrWhiteSpace(clientAppId))
{
uris.Add(string.Format(WamBrokerRedirectUriFormat, clientAppId));
}
return uris.ToArray();
}

/// <summary>
/// Application name for cache directory
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
<ItemGroup>
<!-- CLI Framework -->
<PackageReference Include="Microsoft.Identity.Client" />
<PackageReference Include="Microsoft.Identity.Client.Broker" />
<PackageReference Include="System.CommandLine" />

<!-- Configuration & JSON -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,28 +193,20 @@ private async Task<TokenInfo> 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");

Expand Down Expand Up @@ -254,12 +246,12 @@ private async Task<TokenInfo> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,22 @@ public async Task<bool> 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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,9 @@ public async Task EnsureRedirectUrisAsync(
.Select(uri => uri!)
.ToHashSet(StringComparer.OrdinalIgnoreCase) ?? new HashSet<string>(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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -78,7 +79,7 @@ public Task<GraphServiceClient> 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("");

Expand All @@ -88,19 +89,15 @@ public Task<GraphServiceClient> 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.");
Expand All @@ -118,13 +115,13 @@ public Task<GraphServiceClient> 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"))
Expand All @@ -134,12 +131,12 @@ public Task<GraphServiceClient> 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)
Expand All @@ -151,7 +148,12 @@ public Task<GraphServiceClient> 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
Expand Down
Loading