From 05f32756b004c12cdcb7100601b24c8b2dbe48db Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Tue, 24 Mar 2026 16:31:43 -0700 Subject: [PATCH 1/3] fix: automatic device code fallback when Conditional Access Policy blocks browser/WAM auth (#294) When AADSTS53003 (Conditional Access Policy) or AADSTS53000 (device compliance policy) blocks interactive browser or WAM authentication, the CLI now automatically falls back to device code flow instead of failing with no recovery path. Covers all 6 browser auth locations: - MsalBrowserCredential (primary fix, covers AuthenticationService, InteractiveGraphAuthService, MicrosoftGraphTokenProvider MSAL path, and BlueprintSubcommand) - MicrosoftGraphTokenProvider PowerShell path (retries Connect-MgGraph with -UseDeviceCode) - AuthenticationService belt-and-suspenders catch for future custom credential implementations Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 1 + .../Constants/AuthenticationConstants.cs | 14 ++ .../Services/AuthenticationService.cs | 19 ++ .../Internal/MicrosoftGraphTokenProvider.cs | 20 ++ .../Services/MsalBrowserCredential.cs | 13 ++ .../Services/AuthenticationServiceTests.cs | 175 +++++++++++++++++- 6 files changed, 240 insertions(+), 2 deletions(-) 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..395f3107 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs @@ -201,4 +201,18 @@ 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 bypasses CAP — the CLI falls back automatically when this is detected. + /// 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. + /// + 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..0eccdbf1 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs @@ -4,6 +4,7 @@ using Azure.Core; using Azure.Identity; using Microsoft.Extensions.Logging; +using Microsoft.Identity.Client; using Microsoft.Agents.A365.DevTools.Cli.Constants; using Microsoft.Agents.A365.DevTools.Cli.Exceptions; using Microsoft.Agents.A365.DevTools.Cli.Models; @@ -252,6 +253,24 @@ private async Task AuthenticateInteractivelyAsync( var deviceCodeCredential = CreateDeviceCodeCredential(effectiveClientId, effectiveTenantId); tokenResult = await deviceCodeCredential.GetTokenAsync(tokenRequestContext, default); } + catch (MsalAuthenticationFailedException ex) when ( + useInteractiveBrowser && + ex.InnerException is MsalServiceException svcEx && + (svcEx.Message.Contains(AuthenticationConstants.ConditionalAccessPolicyBlockedError, StringComparison.Ordinal) || + svcEx.Message.Contains(AuthenticationConstants.DeviceCompliancePolicyBlockedError, StringComparison.Ordinal))) + { + // Belt-and-suspenders: MsalBrowserCredential already handles this error internally + // by falling back to device code before throwing. This catch fires only when a + // custom credential implementation (e.g., test double) propagates the error instead + // of absorbing it — ensuring any future credential swap is still CAP-resilient. + _logger.LogWarning( + "Interactive authentication blocked by Conditional Access Policy. " + + "Falling back to device code flow..."); + _logger.LogInformation("Using device code authentication..."); + _logger.LogInformation("Please sign in with your Microsoft account"); + var deviceCodeCredential = CreateDeviceCodeCredential(effectiveClientId, effectiveTenantId); + tokenResult = await deviceCodeCredential.GetTokenAsync(tokenRequestContext, default); + } _logger.LogInformation("Authentication successful!"); 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..fc4f8b42 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 Conditional Access Policy. " + + "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..36d121c6 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs @@ -420,6 +420,19 @@ 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 is not subject to + // these policies — fall back automatically so the user is not blocked. + _logger?.LogWarning( + "Interactive authentication blocked by Conditional Access Policy ({ErrorCode}). " + + "Falling back to device code authentication.", + ex.ErrorCode); + 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..da3f1a92 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,179 @@ 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_WhenBrowserBlockedByConditionalAccessPolicy_FallsBackToDeviceCode() + { + // Arrange — browser credential blocked by Conditional Access Policy (AADSTS53003) + var msalServiceEx = new Microsoft.Identity.Client.MsalServiceException( + "access_denied", + $"AADSTS53003: Access has been blocked by Conditional Access policies. Contact your IT administrator."); + var browserCredential = new ThrowingTokenCredential( + new MsalAuthenticationFailedException( + $"Interactive authentication blocked by Conditional Access Policy ({AuthenticationConstants.ConditionalAccessPolicyBlockedError}).", + msalServiceEx)); + + var expectedToken = "device-code-cap-fallback-token"; + var deviceCodeCredential = new StubTokenCredential(expectedToken, DateTimeOffset.UtcNow.AddHours(1)); + + var logger = Substitute.For>(); + var sut = new TestableAuthenticationService(logger, browserCredential, deviceCodeCredential); + + try + { + // Act + var result = await sut.GetAccessTokenAsync( + "https://agent365.svc.cloud.microsoft", + forceRefresh: true, + useInteractiveBrowser: true); + + // Assert + result.Should().Be(expectedToken, + because: "when Conditional Access Policy blocks browser auth (AADSTS53003), " + + "the CLI must automatically fall back to device code flow so the user is not blocked"); + } + finally + { + sut.ClearCache(); + } + } + + [Fact] + public async Task GetAccessTokenAsync_WhenBrowserBlockedByDeviceCompliancePolicy_FallsBackToDeviceCode() + { + // Arrange — browser credential blocked by device compliance policy (AADSTS53000) + var msalServiceEx = new Microsoft.Identity.Client.MsalServiceException( + "access_denied", + $"AADSTS53000: Device is not in a required state."); + var browserCredential = new ThrowingTokenCredential( + new MsalAuthenticationFailedException( + $"Interactive authentication blocked by device compliance policy ({AuthenticationConstants.DeviceCompliancePolicyBlockedError}).", + msalServiceEx)); + + var expectedToken = "device-code-compliance-fallback-token"; + var deviceCodeCredential = new StubTokenCredential(expectedToken, DateTimeOffset.UtcNow.AddHours(1)); + + var logger = Substitute.For>(); + var sut = new TestableAuthenticationService(logger, browserCredential, deviceCodeCredential); + + try + { + // Act + var result = await sut.GetAccessTokenAsync( + "https://agent365.svc.cloud.microsoft", + forceRefresh: true, + useInteractiveBrowser: true); + + // Assert + result.Should().Be(expectedToken, + because: "when device compliance policy blocks browser auth (AADSTS53000), " + + "the CLI must automatically fall back to device code flow so the user is not blocked"); + } + finally + { + sut.ClearCache(); + } + } + + [Fact] + public async Task GetAccessTokenAsync_WhenBrowserBlockedByConditionalAccessPolicy_LogsWarning() + { + // Arrange + var msalServiceEx = new Microsoft.Identity.Client.MsalServiceException( + "access_denied", + "AADSTS53003: Access has been blocked by Conditional Access policies."); + var browserCredential = new ThrowingTokenCredential( + new MsalAuthenticationFailedException( + "Interactive authentication blocked by Conditional Access Policy.", + msalServiceEx)); + + var deviceCodeCredential = new StubTokenCredential("token", DateTimeOffset.UtcNow.AddHours(1)); + var logger = Substitute.For>(); + var sut = new TestableAuthenticationService(logger, browserCredential, deviceCodeCredential); + + try + { + // Act + await sut.GetAccessTokenAsync( + "https://agent365.svc.cloud.microsoft", + forceRefresh: true, + useInteractiveBrowser: true); + + // Assert — warning logged explaining why device code was triggered + logger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("Conditional Access Policy")), + Arg.Any(), + Arg.Any>()); + } + finally + { + sut.ClearCache(); + } + } + + [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 From c2edf748c4510732b9ac7ce0f955052b0416ffc7 Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Tue, 24 Mar 2026 16:45:44 -0700 Subject: [PATCH 2/3] fix: address PR #323 Copilot review comments for CAP auth fallback - Remove belt-and-suspenders CAP catch from AuthenticationService (C1/C2): unreachable in production and risked double device code attempt - Fix doc comment in AuthenticationConstants: device code flow may still be affected by CAP policies, not "bypasses" them (C3) - Fix log message in MicrosoftGraphTokenProvider to cover both AADSTS53003 and AADSTS53000 (Conditional Access and device compliance) (C4) - Fix ErrorCode placeholder in MsalBrowserCredential to log the AADSTS code (extracted from message) instead of the OAuth "access_denied" code (C5) - Fix comment in MsalBrowserCredential: "may still be affected" instead of "not subject to these policies" (C6) - Remove 3 tests that covered the removed belt-and-suspenders catch; keep the device code error surface test (still valid) Co-Authored-By: Claude Sonnet 4.6 --- .../Constants/AuthenticationConstants.cs | 3 +- .../Services/AuthenticationService.cs | 20 ---- .../Internal/MicrosoftGraphTokenProvider.cs | 2 +- .../Services/MsalBrowserCredential.cs | 9 +- .../Services/AuthenticationServiceTests.cs | 112 ------------------ 5 files changed, 9 insertions(+), 137 deletions(-) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs index 395f3107..16b94499 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs @@ -205,7 +205,7 @@ public static string[] GetRequiredRedirectUris(string clientAppId) /// /// AADSTS53003: Access blocked by Conditional Access Policy. /// MSAL throws MsalServiceException with ErrorCode "access_denied" and this code in the Message. - /// Device code flow bypasses CAP — the CLI falls back automatically when this is detected. + /// 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"; @@ -213,6 +213,7 @@ public static string[] GetRequiredRedirectUris(string clientAppId) /// /// 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 0eccdbf1..edb47613 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AuthenticationService.cs @@ -4,7 +4,6 @@ using Azure.Core; using Azure.Identity; using Microsoft.Extensions.Logging; -using Microsoft.Identity.Client; using Microsoft.Agents.A365.DevTools.Cli.Constants; using Microsoft.Agents.A365.DevTools.Cli.Exceptions; using Microsoft.Agents.A365.DevTools.Cli.Models; @@ -253,25 +252,6 @@ private async Task AuthenticateInteractivelyAsync( var deviceCodeCredential = CreateDeviceCodeCredential(effectiveClientId, effectiveTenantId); tokenResult = await deviceCodeCredential.GetTokenAsync(tokenRequestContext, default); } - catch (MsalAuthenticationFailedException ex) when ( - useInteractiveBrowser && - ex.InnerException is MsalServiceException svcEx && - (svcEx.Message.Contains(AuthenticationConstants.ConditionalAccessPolicyBlockedError, StringComparison.Ordinal) || - svcEx.Message.Contains(AuthenticationConstants.DeviceCompliancePolicyBlockedError, StringComparison.Ordinal))) - { - // Belt-and-suspenders: MsalBrowserCredential already handles this error internally - // by falling back to device code before throwing. This catch fires only when a - // custom credential implementation (e.g., test double) propagates the error instead - // of absorbing it — ensuring any future credential swap is still CAP-resilient. - _logger.LogWarning( - "Interactive authentication blocked by Conditional Access Policy. " + - "Falling back to device code flow..."); - _logger.LogInformation("Using device code authentication..."); - _logger.LogInformation("Please sign in with your Microsoft account"); - 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 fc4f8b42..b988ee49 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/MicrosoftGraphTokenProvider.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/Internal/MicrosoftGraphTokenProvider.cs @@ -160,7 +160,7 @@ public MicrosoftGraphTokenProvider( if (string.IsNullOrWhiteSpace(token) && !useDeviceCode && IsConditionalAccessError(result)) { _logger.LogWarning( - "PowerShell browser authentication blocked by Conditional Access Policy. " + + "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); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs index 36d121c6..f6230bdc 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs @@ -425,12 +425,15 @@ public override async ValueTask GetTokenAsync( ex.Message.Contains(AuthenticationConstants.DeviceCompliancePolicyBlockedError, StringComparison.Ordinal)) { // Conditional Access Policy (AADSTS53003) or device compliance policy (AADSTS53000) - // blocks interactive browser/WAM authentication. Device code flow is not subject to - // these policies — fall back automatically so the user is not blocked. + // 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.", - ex.ErrorCode); + aadErrorCode); return await AcquireTokenWithDeviceCodeFallbackAsync(scopes, cancellationToken); } catch (MsalException ex) 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 da3f1a92..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 @@ -949,118 +949,6 @@ await act.Should().ThrowAsync( "other MsalServiceException codes must surface as errors so the user can diagnose them"); } - [Fact] - public async Task GetAccessTokenAsync_WhenBrowserBlockedByConditionalAccessPolicy_FallsBackToDeviceCode() - { - // Arrange — browser credential blocked by Conditional Access Policy (AADSTS53003) - var msalServiceEx = new Microsoft.Identity.Client.MsalServiceException( - "access_denied", - $"AADSTS53003: Access has been blocked by Conditional Access policies. Contact your IT administrator."); - var browserCredential = new ThrowingTokenCredential( - new MsalAuthenticationFailedException( - $"Interactive authentication blocked by Conditional Access Policy ({AuthenticationConstants.ConditionalAccessPolicyBlockedError}).", - msalServiceEx)); - - var expectedToken = "device-code-cap-fallback-token"; - var deviceCodeCredential = new StubTokenCredential(expectedToken, DateTimeOffset.UtcNow.AddHours(1)); - - var logger = Substitute.For>(); - var sut = new TestableAuthenticationService(logger, browserCredential, deviceCodeCredential); - - try - { - // Act - var result = await sut.GetAccessTokenAsync( - "https://agent365.svc.cloud.microsoft", - forceRefresh: true, - useInteractiveBrowser: true); - - // Assert - result.Should().Be(expectedToken, - because: "when Conditional Access Policy blocks browser auth (AADSTS53003), " + - "the CLI must automatically fall back to device code flow so the user is not blocked"); - } - finally - { - sut.ClearCache(); - } - } - - [Fact] - public async Task GetAccessTokenAsync_WhenBrowserBlockedByDeviceCompliancePolicy_FallsBackToDeviceCode() - { - // Arrange — browser credential blocked by device compliance policy (AADSTS53000) - var msalServiceEx = new Microsoft.Identity.Client.MsalServiceException( - "access_denied", - $"AADSTS53000: Device is not in a required state."); - var browserCredential = new ThrowingTokenCredential( - new MsalAuthenticationFailedException( - $"Interactive authentication blocked by device compliance policy ({AuthenticationConstants.DeviceCompliancePolicyBlockedError}).", - msalServiceEx)); - - var expectedToken = "device-code-compliance-fallback-token"; - var deviceCodeCredential = new StubTokenCredential(expectedToken, DateTimeOffset.UtcNow.AddHours(1)); - - var logger = Substitute.For>(); - var sut = new TestableAuthenticationService(logger, browserCredential, deviceCodeCredential); - - try - { - // Act - var result = await sut.GetAccessTokenAsync( - "https://agent365.svc.cloud.microsoft", - forceRefresh: true, - useInteractiveBrowser: true); - - // Assert - result.Should().Be(expectedToken, - because: "when device compliance policy blocks browser auth (AADSTS53000), " + - "the CLI must automatically fall back to device code flow so the user is not blocked"); - } - finally - { - sut.ClearCache(); - } - } - - [Fact] - public async Task GetAccessTokenAsync_WhenBrowserBlockedByConditionalAccessPolicy_LogsWarning() - { - // Arrange - var msalServiceEx = new Microsoft.Identity.Client.MsalServiceException( - "access_denied", - "AADSTS53003: Access has been blocked by Conditional Access policies."); - var browserCredential = new ThrowingTokenCredential( - new MsalAuthenticationFailedException( - "Interactive authentication blocked by Conditional Access Policy.", - msalServiceEx)); - - var deviceCodeCredential = new StubTokenCredential("token", DateTimeOffset.UtcNow.AddHours(1)); - var logger = Substitute.For>(); - var sut = new TestableAuthenticationService(logger, browserCredential, deviceCodeCredential); - - try - { - // Act - await sut.GetAccessTokenAsync( - "https://agent365.svc.cloud.microsoft", - forceRefresh: true, - useInteractiveBrowser: true); - - // Assert — warning logged explaining why device code was triggered - logger.Received().Log( - LogLevel.Warning, - Arg.Any(), - Arg.Is(o => o.ToString()!.Contains("Conditional Access Policy")), - Arg.Any(), - Arg.Any>()); - } - finally - { - sut.ClearCache(); - } - } - [Fact] public async Task GetAccessTokenAsync_WhenDeviceCodeFailsWithCapError_ThrowsAzureAuthenticationException() { From d9e53752f5dbe4387fa3448fecd849d7df75faed Mon Sep 17 00:00:00 2001 From: Sellakumaran Kanagarathnam Date: Tue, 24 Mar 2026 16:53:24 -0700 Subject: [PATCH 3/3] feat(review): add anti-patterns #19-21 from PR #323 Copilot findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Anti-pattern #19: Unreachable catch clause — when the inner method already handles the exception before propagating, the outer catch is dead code and risks a double-attempt on the same fallback (the AuthenticationService belt-and-suspenders catch that slipped through review-staged). Anti-pattern #20: MsalServiceException.ErrorCode used when AADSTS code expected — ErrorCode is the OAuth code ("access_denied"), the AADSTS code (e.g. AADSTS53003) is only in ex.Message; using ErrorCode in log messages produces misleading diagnostics. Anti-pattern #21: Log message / comment covers fewer cases than the code handles — when a when-clause matches two error codes but the log only names one, operators are misled during triage; also catches "bypasses" / "not subject to" claims in comments that are not universally true. Co-Authored-By: Claude Sonnet 4.6 --- .claude/agents/pr-code-reviewer.md | 62 ++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) 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