diff --git a/.claude/agents/pr-code-reviewer.md b/.claude/agents/pr-code-reviewer.md index 0eda80e2..cbc5b0e7 100644 --- a/.claude/agents/pr-code-reviewer.md +++ b/.claude/agents/pr-code-reviewer.md @@ -592,6 +592,68 @@ A method with return type `bool?` (where `null` signals "fall back to az CLI") r return null; // 401/403/5xx — caller falls back to az CLI ``` +### 19. Unreachable Catch Clause — Inner Method Already Handles the Exception Internally + +A catch block in the caller catches exception type `E`, but the method being called already handles that same `E` internally before propagating. The outer catch becomes dead code. A related risk: if the inner method attempts a fallback (e.g., device code) and that fallback also throws the same exception shape, the outer catch re-triggers the same fallback — a "double attempt" that can produce confusing error messages or partial state. + +- **Pattern to catch**: + 1. Outer method `A` has `catch (ExcType ex) when (inner condition)` wrapping a call to inner method `B` + 2. Inner method `B` is in the same diff or codebase and already contains `catch (ExcType ex) when (same condition)` — meaning `B` never actually throws it + 3. The outer catch attempts the same fallback logic `B` already tried +- **Severity**: `high` — dead code that creates a double-attempt risk; misleads reviewers into thinking coverage exists +- **Check**: For every new `catch` block in the diff, read the source of the called method (use `Read` tool) and verify whether the exception type/condition can actually propagate out. If the inner method swallows it, the outer catch is dead code. +- **Fix**: Remove the outer catch block; rely on the inner implementation's guarantee. + +**Example (from PR #323 — C1/C2):** +```csharp +// AuthenticationService.cs — DEAD CODE: +// MsalBrowserCredential.GetTokenAsync already catches MsalServiceException(AADSTS53003) +// internally and falls back to device code before propagating. This catch never fires +// in production and risks a double device code attempt if that internal fallback also fails. +catch (MsalAuthenticationFailedException ex) when ( + useInteractiveBrowser && + ex.InnerException is MsalServiceException svcEx && + svcEx.Message.Contains("AADSTS53003", StringComparison.Ordinal)) +{ + // Unreachable — remove this catch block +} +``` + +### 20. `MsalServiceException.ErrorCode` Used When AADSTS Code Is Expected + +`MsalServiceException.ErrorCode` is the OAuth 2.0 error code (e.g., `"access_denied"`, `"invalid_request"`) — it is NOT the AADSTS error code (e.g., `"AADSTS53003"`, `"AADSTS70011"`). When the intent is to log or display the specific policy-level error for diagnostics, using `ex.ErrorCode` in the message placeholder is misleading — the AADSTS code appears only in `ex.Message`. + +- **Pattern to catch**: Inside `catch (MsalServiceException ex) when (ex.Message.Contains("AADSTS..."))`, a log call that uses `ex.ErrorCode` as the structured log parameter when the intent is to surface the AADSTS code +- **Severity**: `medium` — log entries record `"access_denied"` instead of `"AADSTS53003"`, making incident investigation harder +- **Check**: For every `catch (MsalServiceException ex)` in the diff, look for `ex.ErrorCode` in log calls. If the when-clause matched on an AADSTS code in `ex.Message`, the log should use the AADSTS code, not `ex.ErrorCode`. +- **Fix**: Extract the AADSTS code from the message or use the constants matched in the when-clause: + ```csharp + var aadErrorCode = ex.Message.Contains(AuthenticationConstants.ConditionalAccessPolicyBlockedError, StringComparison.Ordinal) + ? AuthenticationConstants.ConditionalAccessPolicyBlockedError + : AuthenticationConstants.DeviceCompliancePolicyBlockedError; + _logger?.LogWarning("Blocked by Conditional Access Policy ({ErrorCode}).", aadErrorCode); + ``` + +### 21. Log Message / Comment Covers Fewer Cases Than the Code Handles + +When a catch block, comment, or doc string covers multiple distinct error conditions (e.g., both AADSTS53003 and AADSTS53000), but the associated log messages and comments only mention one of them — the omitted condition goes undocumented, misleading operators during incident triage. + +- **Pattern to catch**: + - A `when`-clause that checks for two or more constants/codes (e.g., `Contains("AADSTS53003") || Contains("AADSTS53000")`) but the log message only names one (`"Conditional Access Policy"` without mentioning device compliance) + - XML doc comments on constants that claim "Device code flow bypasses this policy" or "not subject to these policies" when that statement is configuration-dependent and not universally true +- **Severity**: `medium` — operators see `"Conditional Access Policy"` in logs when the real trigger was AADSTS53000 (device compliance); inaccurate docs create false confidence in fallback guarantees +- **Fix 1** — Log message: include all covered codes or use a broader description: + ```csharp + _logger.LogWarning( + "Authentication blocked by a Conditional Access or device compliance policy ({ErrorCode}). " + + "Retrying with device code authentication...", aadErrorCode); + ``` +- **Fix 2** — Doc comment: replace absolute "bypasses" claims with conditional language: + ```csharp + // Before: "Device code flow bypasses CAP — the CLI falls back automatically." + // After: "Device code flow may succeed depending on your tenant's CAP configuration." + ``` + **MANDATORY REPORTING RULE**: Whenever the diff contains any test file (`.Tests.cs`), you MUST emit a named finding for this check — even if no violation is found. The finding must appear in the review output with one of three statuses: - **`high` severity** if a violation is found (missing warmup, dead executor mock, etc.) - **`info` — FIXED** if the PR is fixing a prior violation (warmup added to previously-cold classes) — list each class fixed and its measured or estimated speedup diff --git a/CHANGELOG.md b/CHANGELOG.md index 68b9e93f..a791e03a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `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 +- Browser and WAM authentication blocked by Conditional Access Policy (AADSTS53003, AADSTS53000) now automatically falls back to device code flow (#294) - macOS/Linux: device code fallback when browser authentication is unavailable (#309) - Linux: MSAL fallback when PowerShell `Connect-MgGraph` fails in non-TTY environments (#309) - Admin consent polling no longer times out after 180s — blueprint service principal now resolved with correct MSAL token (#309) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs index e7e3d353..16b94499 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs @@ -201,4 +201,19 @@ public static string[] GetRequiredRedirectUris(string clientAppId) /// for testing purposes only. It should NOT be deployed to production Azure environments. /// public const string BearerTokenEnvironmentVariable = "BEARER_TOKEN"; + + /// + /// AADSTS53003: Access blocked by Conditional Access Policy. + /// MSAL throws MsalServiceException with ErrorCode "access_denied" and this code in the Message. + /// Device code flow may succeed depending on your tenant's Conditional Access Policy configuration. + /// Reference: https://learn.microsoft.com/en-us/entra/identity-platform/reference-error-codes + /// + public const string ConditionalAccessPolicyBlockedError = "AADSTS53003"; + + /// + /// AADSTS53000: Device does not comply with device compliance policy (a subset of CAP). + /// Treated identically to AADSTS53003 for fallback purposes. + /// Device code flow may succeed depending on your tenant's Conditional Access Policy configuration. + /// + public const string DeviceCompliancePolicyBlockedError = "AADSTS53000"; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs index 826eec1d..edb47613 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs @@ -252,7 +252,6 @@ private async Task AuthenticateInteractivelyAsync( var deviceCodeCredential = CreateDeviceCodeCredential(effectiveClientId, effectiveTenantId); tokenResult = await deviceCodeCredential.GetTokenAsync(tokenRequestContext, default); } - _logger.LogInformation("Authentication successful!"); return new TokenInfo 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 89456b4f..b988ee49 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/MicrosoftGraphTokenProvider.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/MicrosoftGraphTokenProvider.cs @@ -153,6 +153,19 @@ public MicrosoftGraphTokenProvider( var script = BuildPowerShellScript(tenantId, validatedScopes, useDeviceCode, clientAppId); var result = await ExecuteWithFallbackAsync(script, ct); token = ProcessResult(result); + + // If PowerShell browser auth was blocked by Conditional Access Policy, retry with + // device code. This covers the case where clientAppId is null (MSAL skipped) and the + // user is on a CAP-enforced tenant where browser auth is blocked. + if (string.IsNullOrWhiteSpace(token) && !useDeviceCode && IsConditionalAccessError(result)) + { + _logger.LogWarning( + "PowerShell browser authentication blocked by a Conditional Access or device compliance policy (AADSTS53003/AADSTS53000). " + + "Retrying with device code authentication..."); + var deviceCodeScript = BuildPowerShellScript(tenantId, validatedScopes, useDeviceCode: true, clientAppId); + var deviceCodeResult = await ExecuteWithFallbackAsync(deviceCodeScript, ct); + token = ProcessResult(deviceCodeResult); + } } if (string.IsNullOrWhiteSpace(token)) @@ -476,6 +489,13 @@ private static string BuildPowerShellArguments(string shell, string script) return token; } + private static bool IsConditionalAccessError(CommandResult result) + { + return !string.IsNullOrWhiteSpace(result.StandardError) && + (result.StandardError.Contains(AuthenticationConstants.ConditionalAccessPolicyBlockedError, StringComparison.Ordinal) || + result.StandardError.Contains(AuthenticationConstants.DeviceCompliancePolicyBlockedError, StringComparison.Ordinal)); + } + private static bool IsPowerShellNotFoundError(CommandResult result) { if (string.IsNullOrWhiteSpace(result.StandardError)) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs index 6fe06eff..f6230bdc 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs @@ -420,6 +420,22 @@ public override async ValueTask GetTokenAsync( _logger?.LogWarning("Browser cannot be opened on this platform: {Message}", ex.Message); return await AcquireTokenWithDeviceCodeFallbackAsync(scopes, cancellationToken); } + catch (MsalServiceException ex) when ( + ex.Message.Contains(AuthenticationConstants.ConditionalAccessPolicyBlockedError, StringComparison.Ordinal) || + ex.Message.Contains(AuthenticationConstants.DeviceCompliancePolicyBlockedError, StringComparison.Ordinal)) + { + // Conditional Access Policy (AADSTS53003) or device compliance policy (AADSTS53000) + // blocks interactive browser/WAM authentication. Device code flow may still be affected + // by these policies depending on your tenant configuration — attempting fallback. + var aadErrorCode = ex.Message.Contains(AuthenticationConstants.ConditionalAccessPolicyBlockedError, StringComparison.Ordinal) + ? AuthenticationConstants.ConditionalAccessPolicyBlockedError + : AuthenticationConstants.DeviceCompliancePolicyBlockedError; + _logger?.LogWarning( + "Interactive authentication blocked by Conditional Access Policy ({ErrorCode}). " + + "Falling back to device code authentication.", + aadErrorCode); + return await AcquireTokenWithDeviceCodeFallbackAsync(scopes, cancellationToken); + } catch (MsalException ex) { _logger?.LogDebug(ex, "MSAL authentication failed"); 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 a66a405d..6a040653 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 @@ -915,8 +915,67 @@ public async Task GetAccessTokenAsync_WhenBrowserThrowsNonPlatformException_Does forceRefresh: true, useInteractiveBrowser: true); - // Assert — no fallback; error propagates as AzureAuthenticationException - await act.Should().ThrowAsync(); + // Assert — non-CAP errors must NOT trigger device code fallback; only AADSTS53003/53000 should + await act.Should().ThrowAsync( + because: "unrecognized auth errors (e.g. user cancelled) must not silently fall back " + + "to device code — only Conditional Access Policy errors qualify for automatic fallback"); + } + + [Fact] + public async Task GetAccessTokenAsync_WhenBrowserThrowsMsalServiceExceptionWithUnrelatedCode_DoesNotFallBack() + { + // Arrange — browser fails with a real MsalServiceException but for an unrelated reason + // (e.g. AADSTS70011 "invalid scope") — must NOT fall back to device code + var msalServiceEx = new Microsoft.Identity.Client.MsalServiceException( + "invalid_request", + "AADSTS70011: The provided request must include a 'scope' input parameter."); + var browserCredential = new ThrowingTokenCredential( + new MsalAuthenticationFailedException("Invalid scope request.", msalServiceEx)); + + var deviceCodeCredential = new StubTokenCredential("should-not-be-used", DateTimeOffset.UtcNow.AddHours(1)); + + var logger = Substitute.For>(); + var sut = new TestableAuthenticationService(logger, browserCredential, deviceCodeCredential); + + // Act + Func act = async () => await sut.GetAccessTokenAsync( + "https://agent365.svc.cloud.microsoft", + forceRefresh: true, + useInteractiveBrowser: true); + + // Assert + await act.Should().ThrowAsync( + because: "only AADSTS53003 and AADSTS53000 qualify for automatic device code fallback; " + + "other MsalServiceException codes must surface as errors so the user can diagnose them"); + } + + [Fact] + public async Task GetAccessTokenAsync_WhenDeviceCodeFailsWithCapError_ThrowsAzureAuthenticationException() + { + // Arrange — useInteractiveBrowser: false means device code is the first (and only) path. + // If device code itself fails, there is no further fallback — the error must surface. + var msalServiceEx = new Microsoft.Identity.Client.MsalServiceException( + "access_denied", + "AADSTS53003: Access has been blocked by Conditional Access policies."); + var browserCredential = new StubTokenCredential("unused", DateTimeOffset.UtcNow.AddHours(1)); + var deviceCodeCredential = new ThrowingTokenCredential( + new MsalAuthenticationFailedException( + "Device code authentication failed.", + msalServiceEx)); + + var logger = Substitute.For>(); + var sut = new TestableAuthenticationService(logger, browserCredential, deviceCodeCredential); + + // Act + Func act = async () => await sut.GetAccessTokenAsync( + "https://agent365.svc.cloud.microsoft", + forceRefresh: true, + useInteractiveBrowser: false); + + // Assert + await act.Should().ThrowAsync( + because: "when device code itself fails there is no further fallback — " + + "the error must surface to the user rather than looping"); } #endregion