diff --git a/src/Microsoft.Identity.Web.TokenAcquisition/MergedOptions.cs b/src/Microsoft.Identity.Web.TokenAcquisition/MergedOptions.cs index 0401c0c1f..8bd2a46fe 100644 --- a/src/Microsoft.Identity.Web.TokenAcquisition/MergedOptions.cs +++ b/src/Microsoft.Identity.Web.TokenAcquisition/MergedOptions.cs @@ -77,6 +77,42 @@ public ConfidentialClientApplicationOptions ConfidentialClientApplicationOptions */ internal bool PreserveAuthority { get; set; } + // Latch: once true, never reverts to false. Distinguishes user-configured Authority + // from the synthetic value computed by MicrosoftEntraApplicationOptions.Authority getter. + private bool _authorityExplicitlyConfigured; + internal bool AuthorityExplicitlyConfigured + { + get => _authorityExplicitlyConfigured; + set { if (value) _authorityExplicitlyConfigured = true; } + } + + // Latch: once true, never reverts to false. Distinguishes user-configured + // Instance/TenantId from values derived by ParseAuthorityIfNecessary. + private bool _instanceOrTenantIdExplicitlyConfigured; + internal bool InstanceOrTenantIdExplicitlyConfigured + { + get => _instanceOrTenantIdExplicitlyConfigured; + set { if (value) _instanceOrTenantIdExplicitlyConfigured = true; } + } + + // Set after ParseAuthorityIfNecessary runs; makes subsequent calls no-op. + private bool _authorityParsed; + + // Throws if the user explicitly configured BOTH Authority AND Instance/TenantId. + internal void ThrowIfAuthorityConflict() + { + if (AuthorityExplicitlyConfigured && + InstanceOrTenantIdExplicitlyConfigured && + !string.IsNullOrEmpty(Authority) && + (!string.IsNullOrEmpty(Instance) || !string.IsNullOrEmpty(TenantId))) + { + throw new InvalidOperationException( + $"[MsIdWeb] Both 'Authority' ('{Authority}') and 'Instance'/'TenantId' " + + $"('{Instance ?? string.Empty}', '{TenantId ?? string.Empty}') are configured. " + + "These settings conflict. Remove either 'Authority' or 'Instance'/'TenantId' from the configuration."); + } + } + /// /// Id Web will modify the instance so that it can be used by MSAL. /// This modifies this property so that the original value is not changed. @@ -281,6 +317,7 @@ internal static void UpdateMergedOptionsFromMicrosoftIdentityOptions(MicrosoftId if (string.IsNullOrEmpty(mergedOptions.Instance) && !string.IsNullOrEmpty(microsoftIdentityOptions.Instance)) { mergedOptions.Instance = microsoftIdentityOptions.Instance; + mergedOptions.InstanceOrTenantIdExplicitlyConfigured = true; } if (microsoftIdentityOptions.ResetPasswordPath != Constants.ResetPasswordPath) @@ -298,6 +335,7 @@ internal static void UpdateMergedOptionsFromMicrosoftIdentityOptions(MicrosoftId if (string.IsNullOrEmpty(mergedOptions.Authority) && !string.IsNullOrEmpty(microsoftIdentityOptions.Authority)) { mergedOptions.Authority = microsoftIdentityOptions.Authority; + mergedOptions.AuthorityExplicitlyConfigured = true; } mergedOptions.ClientCredentials ??= microsoftIdentityOptions.ClientCredentials; @@ -339,6 +377,7 @@ internal static void UpdateMergedOptionsFromMicrosoftIdentityOptions(MicrosoftId if (string.IsNullOrEmpty(mergedOptions.TenantId) && !string.IsNullOrEmpty(microsoftIdentityOptions.TenantId)) { mergedOptions.TenantId = microsoftIdentityOptions.TenantId; + mergedOptions.InstanceOrTenantIdExplicitlyConfigured = true; } mergedOptions.TokenDecryptionCertificates ??= microsoftIdentityOptions.TokenDecryptionCertificates; @@ -393,6 +432,7 @@ internal static void UpdateMergedOptionsFromConfidentialClientApplicationOptions if (string.IsNullOrEmpty(mergedOptions.Instance) && !string.IsNullOrEmpty(confidentialClientApplicationOptions.Instance)) { mergedOptions.Instance = confidentialClientApplicationOptions.Instance; + mergedOptions.InstanceOrTenantIdExplicitlyConfigured = true; } mergedOptions.IsDefaultPlatformLoggingEnabled |= confidentialClientApplicationOptions.IsDefaultPlatformLoggingEnabled; @@ -407,6 +447,7 @@ internal static void UpdateMergedOptionsFromConfidentialClientApplicationOptions if (string.IsNullOrEmpty(mergedOptions.TenantId) && !string.IsNullOrEmpty(confidentialClientApplicationOptions.TenantId)) { mergedOptions.TenantId = confidentialClientApplicationOptions.TenantId; + mergedOptions.InstanceOrTenantIdExplicitlyConfigured = true; } mergedOptions._confidentialClientApplicationOptions = null; @@ -473,22 +514,20 @@ internal static void UpdateConfidentialClientApplicationOptionsFromMergedOptions */ internal static void ParseAuthorityIfNecessary(MergedOptions mergedOptions, IdWebLogger.ILogger? logger = null) { - // Check if Authority is configured but being ignored due to Instance/TenantId taking precedence - if (!string.IsNullOrEmpty(mergedOptions.Authority) && - (!string.IsNullOrEmpty(mergedOptions.Instance) || !string.IsNullOrEmpty(mergedOptions.TenantId))) - { - // Log warning that Authority is being ignored - if (logger != null) - { - MergedOptionsLogging.AuthorityIgnored( - logger, - mergedOptions.Authority!, - mergedOptions.Instance ?? string.Empty, - mergedOptions.TenantId ?? string.Empty); - } - // Authority is ignored; Instance and TenantId take precedence - return; - } + if (mergedOptions._authorityParsed) + { + return; + } + + if (!string.IsNullOrEmpty(mergedOptions.Authority) && + (!string.IsNullOrEmpty(mergedOptions.Instance) || !string.IsNullOrEmpty(mergedOptions.TenantId))) + { + mergedOptions.ThrowIfAuthorityConflict(); + + // Authority was synthesized (e.g. by MicrosoftEntraApplicationOptions computed getter) -- not a real conflict. + mergedOptions._authorityParsed = true; + return; + } if (string.IsNullOrEmpty(mergedOptions.TenantId) && string.IsNullOrEmpty(mergedOptions.Instance) && !string.IsNullOrEmpty(mergedOptions.Authority)) { @@ -543,6 +582,8 @@ internal static void ParseAuthorityIfNecessary(MergedOptions mergedOptions, IdWe mergedOptions.Instance = mergedOptions.PreserveAuthority ? mergedOptions.Authority! : authoritySpan.Slice(0, indexTenant).ToString(); mergedOptions.TenantId = mergedOptions.PreserveAuthority ? null : authoritySpan.Slice(indexTenant + 1, indexEndOfTenant - indexTenant - 1).ToString(); } + + mergedOptions._authorityParsed = true; } } @@ -552,6 +593,7 @@ internal static void UpdateMergedOptionsFromJwtBearerOptions(JwtBearerOptions jw if (string.IsNullOrEmpty(mergedOptions.Authority) && !string.IsNullOrEmpty(jwtBearerOptions.Authority)) { mergedOptions.Authority = jwtBearerOptions.Authority; + mergedOptions.AuthorityExplicitlyConfigured = true; } } #endif diff --git a/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs b/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs index e7b967b64..7f7eb20fc 100644 --- a/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs +++ b/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs @@ -322,6 +322,9 @@ s.ServiceKey is null && MergedOptionsValidation.Validate(mergedOptions); + // Throw early if Authority conflicts with Instance/TenantId (same check as MSAL path). + mergedOptions.ThrowIfAuthorityConflict(); + if (mergedOptions.Authority != null) { mergedOptions.Authority = AuthorityHelpers.GetAuthorityWithoutQueryIfNeeded(mergedOptions); diff --git a/tests/Microsoft.Identity.Web.Test/MergedOptionsAuthorityConflictTests.cs b/tests/Microsoft.Identity.Web.Test/MergedOptionsAuthorityConflictTests.cs index 9694e2d0e..2508f0b36 100644 --- a/tests/Microsoft.Identity.Web.Test/MergedOptionsAuthorityConflictTests.cs +++ b/tests/Microsoft.Identity.Web.Test/MergedOptionsAuthorityConflictTests.cs @@ -4,8 +4,11 @@ using System; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Identity.Client; using Xunit; +using LogLevel = Microsoft.Extensions.Logging.LogLevel; + namespace Microsoft.Identity.Web.Test { public class MergedOptionsAuthorityConflictTests @@ -18,49 +21,47 @@ public MergedOptionsAuthorityConflictTests() } [Fact] - public void ParseAuthorityIfNecessary_AuthorityAndInstance_LogsWarning() + public void ParseAuthorityIfNecessary_AuthorityAndInstance_ThrowsOnConflict() { // Arrange var mergedOptions = new MergedOptions { Authority = "https://login.microsoftonline.com/common", Instance = "https://login.microsoftonline.com/", + AuthorityExplicitlyConfigured = true, + InstanceOrTenantIdExplicitlyConfigured = true, Logger = _testLogger }; - // Act - MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger); - - // Assert - Assert.Single(_testLogger.LogMessages); - Assert.Contains("Authority", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Contains("ignored", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Equal(LogLevel.Warning, _testLogger.LogLevel); + // Act & Assert + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); } [Fact] - public void ParseAuthorityIfNecessary_AuthorityAndTenantId_LogsWarning() + public void ParseAuthorityIfNecessary_AuthorityAndTenantId_ThrowsOnConflict() { // Arrange var mergedOptions = new MergedOptions { Authority = "https://login.microsoftonline.com/common", TenantId = "organizations", + AuthorityExplicitlyConfigured = true, + InstanceOrTenantIdExplicitlyConfigured = true, Logger = _testLogger }; - // Act - MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger); - - // Assert - Assert.Single(_testLogger.LogMessages); - Assert.Contains("Authority", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Contains("ignored", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Equal(LogLevel.Warning, _testLogger.LogLevel); + // Act & Assert + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); } [Fact] - public void ParseAuthorityIfNecessary_AuthorityAndInstanceAndTenantId_LogsWarning() + public void ParseAuthorityIfNecessary_AuthorityAndInstanceAndTenantId_ThrowsOnConflict() { // Arrange var mergedOptions = new MergedOptions @@ -68,17 +69,16 @@ public void ParseAuthorityIfNecessary_AuthorityAndInstanceAndTenantId_LogsWarnin Authority = "https://login.microsoftonline.com/common", Instance = "https://login.microsoftonline.com/", TenantId = "organizations", + AuthorityExplicitlyConfigured = true, + InstanceOrTenantIdExplicitlyConfigured = true, Logger = _testLogger }; - // Act - MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger); - - // Assert - Assert.Single(_testLogger.LogMessages); - Assert.Contains("Authority", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Contains("ignored", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Equal(LogLevel.Warning, _testLogger.LogLevel); + // Act & Assert + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); } [Fact] @@ -131,49 +131,47 @@ public void ParseAuthorityIfNecessary_InstanceAndTenantIdOnly_NoWarning() } [Fact] - public void ParseAuthorityIfNecessary_B2CAuthorityAndInstance_LogsWarning() + public void ParseAuthorityIfNecessary_B2CAuthorityAndInstance_ThrowsOnConflict() { // Arrange - B2C scenario var mergedOptions = new MergedOptions { Authority = "https://fabrikamb2c.b2clogin.com/fabrikamb2c.onmicrosoft.com/b2c_1_susi", Instance = "https://fabrikamb2c.b2clogin.com/", + AuthorityExplicitlyConfigured = true, + InstanceOrTenantIdExplicitlyConfigured = true, Logger = _testLogger }; - // Act - MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger); - - // Assert - Assert.Single(_testLogger.LogMessages); - Assert.Contains("Authority", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Contains("ignored", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Equal(LogLevel.Warning, _testLogger.LogLevel); + // Act & Assert + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); } [Fact] - public void ParseAuthorityIfNecessary_CiamAuthorityAndInstance_LogsWarning() + public void ParseAuthorityIfNecessary_CiamAuthorityAndInstance_ThrowsOnConflict() { // Arrange - CIAM scenario var mergedOptions = new MergedOptions { Authority = "https://contoso.ciamlogin.com/contoso.onmicrosoft.com", Instance = "https://contoso.ciamlogin.com/", + AuthorityExplicitlyConfigured = true, + InstanceOrTenantIdExplicitlyConfigured = true, Logger = _testLogger }; - // Act - MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger); - - // Assert - Assert.Single(_testLogger.LogMessages); - Assert.Contains("Authority", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Contains("ignored", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Equal(LogLevel.Warning, _testLogger.LogLevel); + // Act & Assert + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); } [Fact] - public void ParseAuthorityIfNecessary_CiamPreservedAuthorityWithInstance_LogsWarning() + public void ParseAuthorityIfNecessary_CiamPreservedAuthorityWithInstance_ThrowsOnConflict() { // Arrange - CIAM with PreserveAuthority flag var mergedOptions = new MergedOptions @@ -181,31 +179,193 @@ public void ParseAuthorityIfNecessary_CiamPreservedAuthorityWithInstance_LogsWar Authority = "https://custom.contoso.com/contoso.onmicrosoft.com", Instance = "https://custom.contoso.com/", PreserveAuthority = true, + AuthorityExplicitlyConfigured = true, + InstanceOrTenantIdExplicitlyConfigured = true, Logger = _testLogger }; - // Act - MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger); - - // Assert - Warning should still be logged even with PreserveAuthority - Assert.Single(_testLogger.LogMessages); - Assert.Contains("Authority", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Contains("ignored", _testLogger.LogMessages[0], StringComparison.OrdinalIgnoreCase); - Assert.Equal(LogLevel.Warning, _testLogger.LogLevel); + // Act & Assert + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); } [Fact] - public void ParseAuthorityIfNecessary_NoLogger_NoException() + public void ParseAuthorityIfNecessary_NoLogger_StillThrowsOnConflict() { // Arrange var mergedOptions = new MergedOptions { Authority = "https://login.microsoftonline.com/common", Instance = "https://login.microsoftonline.com/", + AuthorityExplicitlyConfigured = true, + InstanceOrTenantIdExplicitlyConfigured = true, + }; + + // Act & Assert - Throws regardless of logger presence + Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(mergedOptions, logger: null)); + } + + [Fact] + public void ParseAuthorityIfNecessary_SyntheticAuthority_NoThrow() + { + // When Authority was backfilled by the computed getter (latch not set), + // there is no real conflict -- should not throw. + var mergedOptions = new MergedOptions + { + Authority = "https://login.microsoftonline.com/common", + Instance = "https://login.microsoftonline.com/", + // AuthorityExplicitlyConfigured intentionally NOT set + Logger = _testLogger + }; + + // Act & Assert - no exception + var ex = Record.Exception(() => MergedOptions.ParseAuthorityIfNecessary(mergedOptions, _testLogger)); + Assert.Null(ex); + } + + // ----- Higher-level: config objects -> UpdateMergedOptions* -> ParseAuthorityIfNecessary ----- + // These prove the latches are set correctly through the real merge pipeline. + + [Fact] + public void FromConfigObjects_AuthorityAndInstance_ThrowsOnConflict() + { + // Arrange -- simulate what DI does: Identity options carry Authority, + // CCA options carry Instance. Both merge into MergedOptions. + var identityOptions = new MicrosoftIdentityOptions + { + Authority = "https://login.microsoftonline.com/common", + ClientId = "test-client-id" + }; + var ccaOptions = new ConfidentialClientApplicationOptions + { + Instance = "https://login.microsoftonline.com/", + ClientId = "test-client-id" + }; + + var merged = new MergedOptions(); + MergedOptions.UpdateMergedOptionsFromMicrosoftIdentityOptions(identityOptions, merged); + MergedOptions.UpdateMergedOptionsFromConfidentialClientApplicationOptions(ccaOptions, merged); + + // Act & Assert + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(merged, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void FromConfigObjects_AuthorityAndTenantId_ThrowsOnConflict() + { + var identityOptions = new MicrosoftIdentityOptions + { + Authority = "https://login.microsoftonline.com/common", + TenantId = "organizations", + ClientId = "test-client-id" + }; + + var merged = new MergedOptions(); + MergedOptions.UpdateMergedOptionsFromMicrosoftIdentityOptions(identityOptions, merged); + + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(merged, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void FromConfigObjects_AuthorityAndInstanceAndTenantId_ThrowsOnConflict() + { + var identityOptions = new MicrosoftIdentityOptions + { + Authority = "https://login.microsoftonline.com/common", + Instance = "https://login.microsoftonline.com/", + TenantId = "organizations", + ClientId = "test-client-id" }; - // Act & Assert - Should not throw when logger is null - MergedOptions.ParseAuthorityIfNecessary(mergedOptions, logger: null); + var merged = new MergedOptions(); + MergedOptions.UpdateMergedOptionsFromMicrosoftIdentityOptions(identityOptions, merged); + + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(merged, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void FromConfigObjects_InstanceAndTenantIdOnly_NoThrow() + { + var identityOptions = new MicrosoftIdentityOptions + { + Instance = "https://login.microsoftonline.com/", + TenantId = "organizations", + ClientId = "test-client-id" + }; + + var merged = new MergedOptions(); + MergedOptions.UpdateMergedOptionsFromMicrosoftIdentityOptions(identityOptions, merged); + + var ex = Record.Exception( + () => MergedOptions.ParseAuthorityIfNecessary(merged, _testLogger)); + Assert.Null(ex); + } + + [Fact] + public void FromConfigObjects_AuthorityOnly_NoThrow() + { + var identityOptions = new MicrosoftIdentityOptions + { + Authority = "https://login.microsoftonline.com/common", + ClientId = "test-client-id" + }; + + var merged = new MergedOptions(); + MergedOptions.UpdateMergedOptionsFromMicrosoftIdentityOptions(identityOptions, merged); + + var ex = Record.Exception( + () => MergedOptions.ParseAuthorityIfNecessary(merged, _testLogger)); + Assert.Null(ex); + } + + [Fact] + public void FromConfigObjects_B2CAuthorityAndInstance_ThrowsOnConflict() + { + var identityOptions = new MicrosoftIdentityOptions + { + Authority = "https://fabrikamb2c.b2clogin.com/fabrikamb2c.onmicrosoft.com/b2c_1_susi", + Instance = "https://fabrikamb2c.b2clogin.com/", + ClientId = "test-client-id" + }; + + var merged = new MergedOptions(); + MergedOptions.UpdateMergedOptionsFromMicrosoftIdentityOptions(identityOptions, merged); + + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(merged, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void FromConfigObjects_CiamAuthorityAndInstance_ThrowsOnConflict() + { + var identityOptions = new MicrosoftIdentityOptions + { + Authority = "https://contoso.ciamlogin.com/contoso.onmicrosoft.com", + Instance = "https://contoso.ciamlogin.com/", + ClientId = "test-client-id" + }; + + var merged = new MergedOptions(); + MergedOptions.UpdateMergedOptionsFromMicrosoftIdentityOptions(identityOptions, merged); + + var ex = Assert.Throws( + () => MergedOptions.ParseAuthorityIfNecessary(merged, _testLogger)); + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); } // Test helper class to capture log messages diff --git a/tests/Microsoft.Identity.Web.Test/TokenAcquisitionAuthorityTests.cs b/tests/Microsoft.Identity.Web.Test/TokenAcquisitionAuthorityTests.cs index 48225ca55..e6279baf3 100644 --- a/tests/Microsoft.Identity.Web.Test/TokenAcquisitionAuthorityTests.cs +++ b/tests/Microsoft.Identity.Web.Test/TokenAcquisitionAuthorityTests.cs @@ -158,7 +158,6 @@ public async Task VerifyCorrectRedirectUriAsync( _applicationOptionsMonitor = new TestOptionsMonitor(new ConfidentialClientApplicationOptions { - Instance = TC.AadInstance, RedirectUri = redirectUri, ClientSecret = TC.ClientSecret, }); @@ -194,7 +193,6 @@ public async Task VerifyDifferentRegionsDifferentAppAsync() _applicationOptionsMonitor = new TestOptionsMonitor(new ConfidentialClientApplicationOptions { - Instance = TC.AadInstance, RedirectUri = "http://localhost:1729/", ClientSecret = TC.ClientSecret, }); @@ -242,7 +240,6 @@ public async Task GetOrBuildCca_BearerThenTokenBinding_DoesNotReturnCachedBearer _applicationOptionsMonitor = new TestOptionsMonitor(new ConfidentialClientApplicationOptions { - Instance = TC.AadInstance, RedirectUri = "http://localhost:1729/", ClientSecret = TC.ClientSecret, }); @@ -286,7 +283,6 @@ public async Task GetOrBuildCca_TokenBindingThenBearer_DoesNotReturnCachedPopApp _applicationOptionsMonitor = new TestOptionsMonitor(new ConfidentialClientApplicationOptions { - Instance = TC.AadInstance, RedirectUri = "http://localhost:1729/", ClientSecret = TC.ClientSecret, }); diff --git a/tests/Microsoft.Identity.Web.Test/WebAppExtensionsAuthorityConflictTests.cs b/tests/Microsoft.Identity.Web.Test/WebAppExtensionsAuthorityConflictTests.cs new file mode 100644 index 000000000..a10a17fc6 --- /dev/null +++ b/tests/Microsoft.Identity.Web.Test/WebAppExtensionsAuthorityConflictTests.cs @@ -0,0 +1,377 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Linq; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authentication.OpenIdConnect; +using Microsoft.AspNetCore.DataProtection; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Configuration.Memory; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Hosting.Internal; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Microsoft.Identity.Web.Test.Common; +using NSubstitute; +using NSubstitute.Extensions; +using Xunit; + +namespace Microsoft.Identity.Web.Test +{ + /// + /// Pinning tests for the OIDC sign-in code path in + /// when Authority + /// is configured alongside Instance/TenantId/Domain/SignUpSignInPolicyId. OIDC counterpart + /// of ; see each test for parity notes. + /// + public class WebAppExtensionsAuthorityConflictTests + { + private const string OidcScheme = "OpenIdConnect-Custom"; + private const string CookieScheme = "Cookies-Custom"; + private const string ConfigSectionName = "AzureAd-Custom"; + + // All-zeros GUID lets failures fail loudly: if Authority ever stops winning, + // this value would not appear in OpenIdConnectOptions.Authority. + private const string BogusTenantGuid = "00000000-0000-0000-0000-000000000000"; + + private const string BogusAadAuthority = "https://login.microsoftonline.com/" + BogusTenantGuid + "/v2.0"; + private const string BogusB2CAuthority = "https://fakeb2c.b2clogin.com/" + BogusTenantGuid + "/b2c_1_bogus"; + private const string BogusCiamAuthority = "https://fakeciam.ciamlogin.com/" + BogusTenantGuid; + + private readonly IHostEnvironment _env = new HostingEnvironment { EnvironmentName = Environments.Development }; + + // ----- AAD: precedence (behavior pinning) ----- + + [Fact] + public void OidcSignIn_AuthorityAndInstance_ThrowsOnConflict() + { + // MSAL parity: ParseAuthorityIfNecessary_AuthorityAndInstance_LogsWarning. Now both paths throw. + var ex = Assert.Throws(() => BuildAndGetOidcOptions( + authority: BogusAadAuthority, + instance: TestConstants.AadInstance, + tenantId: null)); + + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void OidcSignIn_AuthorityAndTenantId_ThrowsOnConflict() + { + // MSAL parity: ParseAuthorityIfNecessary_AuthorityAndTenantId_LogsWarning. Now both paths throw. + var ex = Assert.Throws(() => BuildAndGetOidcOptions( + authority: BogusAadAuthority, + instance: null, + tenantId: TestConstants.TenantIdAsGuid)); + + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void OidcSignIn_AuthorityAndInstanceAndTenantId_ThrowsOnConflict() + { + // MSAL parity: ParseAuthorityIfNecessary_AuthorityAndInstanceAndTenantId_LogsWarning. Now both paths throw. + var ex = Assert.Throws(() => BuildAndGetOidcOptions( + authority: BogusAadAuthority, + instance: TestConstants.AadInstance, + tenantId: TestConstants.TenantIdAsGuid)); + + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void OidcSignIn_AuthorityOnly_PropagatesAuthorityAsIs() + { + // MSAL parity: ParseAuthorityIfNecessary_AuthorityOnly_LogsAuthorityUsedHint. Divergence: MSAL parses Authority + logs EventId 501; OIDC propagates Authority verbatim. + var options = BuildAndGetOidcOptions( + authority: TestConstants.AuthorityWithTenantSpecifiedWithV2, + instance: null, + tenantId: null); + + Assert.Equal(TestConstants.AuthorityWithTenantSpecifiedWithV2, options.Authority); + } + + [Fact] + public void OidcSignIn_InstanceAndTenantIdOnly_ComposesAuthorityFromInstanceTenantId() + { + // MSAL parity: ParseAuthorityIfNecessary_InstanceAndTenantIdOnly_NoWarning. Same outcome on both paths (no Authority -> Instance+TenantId compose). + var options = BuildAndGetOidcOptions( + authority: null, + instance: TestConstants.AadInstance, + tenantId: TestConstants.TenantIdAsGuid); + + Assert.Contains(TestConstants.TenantIdAsGuid, options.Authority, StringComparison.Ordinal); + Assert.DoesNotContain(BogusTenantGuid, options.Authority, StringComparison.Ordinal); + } + + // ----- B2C: precedence (behavior pinning) ----- + + [Fact] + public void OidcSignIn_B2CAuthorityAndInstance_ThrowsOnConflict() + { + // MSAL parity: ParseAuthorityIfNecessary_B2CAuthorityAndInstance_LogsWarning. Now both paths throw. + var ex = Assert.Throws(() => BuildAndGetOidcOptions( + authority: BogusB2CAuthority, + instance: TestConstants.B2CInstance, + tenantId: null, + domain: TestConstants.B2CTenant, + signUpSignInPolicyId: TestConstants.B2CSignUpSignInUserFlow)); + + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void OidcSignIn_B2CInstanceAndDomain_ComposesAuthorityFromInstanceDomainUserFlow() + { + // No MSAL counterpart; control: BuildAuthority composes "Instance/Domain/UserFlow/v2.0". + var options = BuildAndGetOidcOptions( + authority: null, + instance: TestConstants.B2CInstance, + tenantId: null, + domain: TestConstants.B2CTenant, + signUpSignInPolicyId: TestConstants.B2CSignUpSignInUserFlow); + + Assert.Contains(TestConstants.B2CTenant, options.Authority, StringComparison.Ordinal); + Assert.Contains(TestConstants.B2CSignUpSignInUserFlow, options.Authority, StringComparison.Ordinal); + Assert.DoesNotContain(BogusTenantGuid, options.Authority, StringComparison.Ordinal); + } + + // ----- CIAM: precedence (behavior pinning) ----- + + [Fact] + public void OidcSignIn_CiamAuthorityAndInstance_ThrowsOnConflict() + { + // MSAL parity: ParseAuthorityIfNecessary_CiamAuthorityAndInstance_LogsWarning. Now both paths throw. + var ex = Assert.Throws(() => BuildAndGetOidcOptions( + authority: BogusCiamAuthority, + instance: TestConstants.CIAMInstance, + tenantId: null)); + + Assert.Contains("Authority", ex.Message, StringComparison.Ordinal); + Assert.Contains("conflict", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void OidcSignIn_CiamAuthorityOnly_PropagatesAuthorityThroughCiamHelper() + { + // No MSAL counterpart; control: BuildCiamAuthorityIfNeeded preserves Authority, EnsureAuthorityIsV2 appends /v2.0. + var options = BuildAndGetOidcOptions( + authority: TestConstants.CIAMAuthorityV2, + instance: null, + tenantId: null); + + Assert.Contains("ciamlogin.com", options.Authority, StringComparison.Ordinal); + Assert.Contains(TestConstants.CIAMTenant, options.Authority, StringComparison.Ordinal); + } + + // ----- Log assertions (non-conflict scenarios still produce no warnings) ----- + + [Fact] + public void OidcSignIn_AuthorityOnly_DoesNotLogAuthorityUsedHint() + { + // MSAL parity: ParseAuthorityIfNecessary_AuthorityOnly_LogsAuthorityUsedHint (EventId 501, 1P hint). OIDC never surfaces this hint. + var loggerProvider = new TestLoggerProvider(); + + _ = BuildAndGetOidcOptions( + authority: TestConstants.AuthorityWithTenantSpecifiedWithV2, + instance: null, + tenantId: null, + loggerProvider: loggerProvider); + + Assert.DoesNotContain(loggerProvider.Logger.Entries, e => e.EventId.Id == 501); + } + + [Fact] + public void OidcSignIn_InstanceAndTenantIdOnly_DoesNotLogAnyAuthorityWarning() + { + // MSAL parity: ParseAuthorityIfNecessary_InstanceAndTenantIdOnly_NoWarning. Same outcome on both paths (no Authority -> no warnings). + var loggerProvider = new TestLoggerProvider(); + + _ = BuildAndGetOidcOptions( + authority: null, + instance: TestConstants.AadInstance, + tenantId: TestConstants.TenantIdAsGuid, + loggerProvider: loggerProvider); + + Assert.DoesNotContain(loggerProvider.Logger.Entries, e => e.EventId.Id == 500 || e.EventId.Id == 501); + } + + // ----- Robustness ----- + + [Fact] + public void OidcSignIn_AuthorityAndInstance_ThrowsEvenWhenNoLoggerRegistered() + { + // Conflict throw does not depend on logger registration. + Assert.Throws(() => BuildAndGetOidcOptions( + authority: BogusAadAuthority, + instance: TestConstants.AadInstance, + tenantId: null, + loggerProvider: null)); + } + + // ----- Helpers ----- + + /// + /// Builds AddMicrosoftIdentityWebApp from a synthetic config section and returns the + /// resolved OpenIdConnectOptions. Pass a loggerProvider to capture log entries. + /// + private OpenIdConnectOptions BuildAndGetOidcOptions( + string? authority, + string? instance, + string? tenantId, + string? domain = null, + string? signUpSignInPolicyId = null, + TestLoggerProvider? loggerProvider = null) + { + var configSection = BuildConfigSection( + ConfigSectionName, + authority: authority, + instance: instance, + tenantId: tenantId, + domain: domain, + signUpSignInPolicyId: signUpSignInPolicyId, + clientId: TestConstants.ClientId); + + var configMock = Substitute.For(); + configMock.Configure().GetSection(ConfigSectionName).Returns(configSection); + + var services = new ServiceCollection(); + services.AddSingleton(configMock); + services.AddDataProtection(); + services.AddSingleton(_env); + + if (loggerProvider is not null) + { + services.AddLogging(b => + { + b.SetMinimumLevel(LogLevel.Trace); + b.AddProvider(loggerProvider); + }); + } + + services.AddAuthentication() + .AddMicrosoftIdentityWebApp( + configMock, + ConfigSectionName, + OidcScheme, + CookieScheme, + subscribeToOpenIdConnectMiddlewareDiagnosticsEvents: false); + + using var provider = services.BuildServiceProvider(); + return provider.GetRequiredService>().Get(OidcScheme); + } + + /// + /// In-memory IConfigurationSection populated only with non-null keys; ClientId is required + /// to satisfy MergedOptionsValidation. Omitting keys leaves MergedOptions properties null, + /// which is what selects the precedence branches under test. + /// + private static IConfigurationSection BuildConfigSection( + string configSectionName, + string? authority, + string? instance, + string? tenantId, + string? domain, + string? signUpSignInPolicyId, + string clientId) + { + var configAsDictionary = new Dictionary + { + { configSectionName, null }, + { $"{configSectionName}:ClientId", clientId }, + }; + + if (authority is not null) + { + configAsDictionary[$"{configSectionName}:Authority"] = authority; + } + + if (instance is not null) + { + configAsDictionary[$"{configSectionName}:Instance"] = instance; + } + + if (tenantId is not null) + { + configAsDictionary[$"{configSectionName}:TenantId"] = tenantId; + } + + if (domain is not null) + { + configAsDictionary[$"{configSectionName}:Domain"] = domain; + } + + if (signUpSignInPolicyId is not null) + { + configAsDictionary[$"{configSectionName}:SignUpSignInPolicyId"] = signUpSignInPolicyId; + } + + var memoryConfigSource = new MemoryConfigurationSource { InitialData = configAsDictionary }; + + var configBuilder = new ConfigurationBuilder(); + configBuilder.Add(memoryConfigSource); + + return configBuilder.Build().GetSection(configSectionName); + } + + /// Single captured log entry. + private sealed record TestLogEntry(LogLevel LogLevel, EventId EventId, string Message, string Category); + + /// + /// ILogger that captures every call into a shared list (always enabled, all levels). + /// + private sealed class TestLogger : ILogger + { + private readonly string _category; + public List Entries { get; } + + public TestLogger(string category, List entries) + { + _category = category; + Entries = entries; + } + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + Entries.Add(new TestLogEntry(logLevel, eventId, formatter(state, exception), _category)); + } + } + + /// + /// ILoggerProvider routing every category-specific logger into a single shared + /// TestLogger so tests can assert across categories via . + /// + private sealed class TestLoggerProvider : ILoggerProvider + { + private readonly List _entries = new(); + + public TestLogger Logger { get; } + + public TestLoggerProvider() + { + Logger = new TestLogger(category: "(aggregate)", entries: _entries); + } + + public ILogger CreateLogger(string categoryName) => new TestLogger(categoryName, _entries); + + public void Dispose() + { + } + } + } +} diff --git a/tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs b/tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs index c828655f1..3f0c53ce9 100644 --- a/tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs +++ b/tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs @@ -256,12 +256,23 @@ public void AddMicrosoftIdentityWebApp_SubscribeFalse_DoesNotRegisterDiagnostics [Fact] public Task AddMicrosoftIdentityWebApp_WithConfigAuthority_TestCorrectMetadataAddressAsync() { - // Arrange + // Arrange - use a minimal config without Instance/TenantId to avoid conflict. string authority = "https://login.microsoftonline.com/some-tenant-id/v2.0"; string appId = "some-client-id"; string expectedMetadataAddress = $"{authority}/.well-known/openid-configuration?appId={appId}"; + + var authorityOnlyConfig = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + { ConfigSectionName, null }, + { $"{ConfigSectionName}:ClientId", TestConstants.TenantIdAsGuid }, + { $"{ConfigSectionName}:Domain", TestConstants.Domain }, + }) + .Build() + .GetSection(ConfigSectionName); + var configMock = Substitute.For(); - configMock.Configure().GetSection(ConfigSectionName).Returns(_configSection); + configMock.Configure().GetSection(ConfigSectionName).Returns(authorityOnlyConfig); var services = new ServiceCollection() .AddSingleton(configMock)