Skip to content

Commit 877cfc3

Browse files
jmprieurCopilotCopilot
authored
Jmprieur/fix authority parsing (#3620)
* Fix Authority-only configuration: Parse to Instance/TenantId for AAD, CIAM, and B2C (#3614) * Initial plan * Implement authority-only configuration support for AAD, CIAM, and B2C - Move ParseAuthorityIfNecessary to start of UpdateConfidentialClientApplicationOptionsFromMergedOptions - Add defensive fallback parse in PrepareAuthorityInstanceForMsal - Add TODO comment for future warning logging (issue #3611) - Create new unit test file MergedOptionsAuthorityParsingTests.cs with 7 tests - Modify E2E test to use Authority-only configuration - Add new B2C authority-only E2E test All unit tests pass successfully Co-authored-by: jmprieur <[email protected]> * Address code review feedback - Add explicit using statement for Microsoft.Identity.Web in test file - Improve defensive fallback to also check TenantId to avoid redundant parsing All tests still pass Co-authored-by: jmprieur <[email protected]> * Restore original test and create new Authority-only test - Restore AcquireToken_WithMicrosoftIdentityApplicationOptions_ClientCredentialsAsync with Instance/TenantId - Create new AcquireToken_WithMicrosoftIdentityApplicationOptions_Authority_ClientCredentialsAsync for Authority-only scenario Both tests now exist to demonstrate both recommended (Instance/TenantId) and Authority-only approaches Co-authored-by: jmprieur <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jmprieur <[email protected]> * Add high-performance warning logging for Authority + Instance/TenantId conflicts (#3615) * Initial plan * Add authority conflict warning logging infrastructure Co-authored-by: jmprieur <[email protected]> * Add comprehensive unit tests for authority conflict warnings Co-authored-by: jmprieur <[email protected]> * Wire up logger factory to MergedOptionsStore for runtime logging Co-authored-by: jmprieur <[email protected]> * Fix MergedOptionsStore constructor signature for DI compatibility Co-authored-by: jmprieur <[email protected]> * Rename MsalLogger alias to IdWebLogger for clarity Co-authored-by: jmprieur <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jmprieur <[email protected]> Co-authored-by: Jean-Marc Prieur <[email protected]> * Add comprehensive test matrix for authority parsing logic (#3610) (#3616) * Initial plan * Add comprehensive authority parsing test matrix - all unit tests created and passing Co-authored-by: jmprieur <[email protected]> * Implement full E2E authority matrix tests following existing patterns Co-authored-by: jmprieur <[email protected]> * Remove duplicate test files and consolidate unique edge case tests Co-authored-by: jmprieur <[email protected]> * Remove performance smoke tests as they are not on the hot path Co-authored-by: jmprieur <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jmprieur <[email protected]> * Update version to 4.1.1 (#3619) * Initial plan * Update version to 4.1.1 and add changelog entry Co-authored-by: jmprieur <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jmprieur <[email protected]> * Update src/Microsoft.Identity.Web.TokenAcquisition/MergedOptions.cs Co-authored-by: Copilot <[email protected]> * Update changelog.md Co-authored-by: Copilot <[email protected]> * [WIP] Update authority parsing fix for ILoggerFactory usage (#3621) * Initial plan * Update MergedOptionsStore to use IServiceProvider instead of ILoggerFactory Co-authored-by: jmprieur <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jmprieur <[email protected]> * Adding more resilience to configuration (common on App token, and no domain for b2c) --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: jmprieur <[email protected]> Co-authored-by: Copilot <[email protected]>
1 parent 233f2e0 commit 877cfc3

18 files changed

+778
-14
lines changed

‎Directory.Build.props‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
<!-- This needs to be greater than or equal to the validation baseline version. The conditional logic around TargetNetNext is there
55
to avoid NU5104 for packing a release version library with prerelease deps. By adding preview to it, that warning is avoided.
66
-->
7-
<MicrosoftIdentityWebVersion Condition="'$(MicrosoftIdentityWebVersion)' == ''">4.1.0</MicrosoftIdentityWebVersion>
7+
<MicrosoftIdentityWebVersion Condition="'$(MicrosoftIdentityWebVersion)' == ''">4.1.1</MicrosoftIdentityWebVersion>
88
<!--This will generate AssemblyVersion, AssemblyFileVersion and AssemblyInformationVersion-->
99
<Version>$(MicrosoftIdentityWebVersion)</Version>
1010

‎changelog.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,12 @@
1+
## 4.1.1
2+
3+
### Added
4+
- Authority-only configuration parsing improvements (Issue #3612): Early parsing of Authority into Instance/TenantId and defensive fallback in PrepareAuthorityInstanceForMsal. Behavior is backward compatible; Authority is still ignored when Instance/TenantId explicitly provided—now surfaced via a warning.
5+
- Warning diagnostics for conflicting Authority vs Instance/TenantId (Issue #3611): Emits a single structured warning when both styles are provided.
6+
7+
### Fundamentals
8+
- Expanded authority test matrix (Issue #3610): Coverage for AAD (v1/v2), B2C (/tfp/ normalization, policy path), CIAM (PreserveAuthority), query parameters, scheme-less forms, and conflict scenarios.
9+
110
4.1.0
211
=========
312
### New features

‎src/Microsoft.Identity.Web.TokenAcquisition/LoggingEventId.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ internal static class LoggingEventId
2929
public static readonly EventId CredentialLoadAttemptFailed = new EventId(406, "CredentialLoadAttemptFailed");
3030
public static readonly EventId UsingSignedAssertionFromCustomProvider = new EventId(407, "UsingSignedAssertionFromCustomProvider");
3131

32+
// MergedOptions EventIds 408+
33+
public static readonly EventId AuthorityConflict = new EventId(408, "AuthorityConflict");
34+
3235
#pragma warning restore IDE1006 // Naming Styles
3336
}
3437
}

‎src/Microsoft.Identity.Web.TokenAcquisition/MergedOptions.cs‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System;
55
using System.Collections.Generic;
66
using System.Linq;
7+
using IdWebLogger = Microsoft.Extensions.Logging;
78
using Microsoft.Identity.Abstractions;
89

910
#if !NETSTANDARD2_0 && !NET462 && !NET472
@@ -22,6 +23,11 @@ internal sealed class MergedOptions : MicrosoftIdentityOptions
2223
{
2324
private ConfidentialClientApplicationOptions? _confidentialClientApplicationOptions;
2425

26+
/// <summary>
27+
/// Logger instance for diagnostics.
28+
/// </summary>
29+
internal IdWebLogger.ILogger? Logger { get; set; }
30+
2531
public ConfidentialClientApplicationOptions ConfidentialClientApplicationOptions
2632
{
2733
get
@@ -386,6 +392,8 @@ internal static void UpdateMergedOptionsFromConfidentialClientApplicationOptions
386392

387393
internal static void UpdateConfidentialClientApplicationOptionsFromMergedOptions(MergedOptions mergedOptions, ConfidentialClientApplicationOptions confidentialClientApplicationOptions)
388394
{
395+
ParseAuthorityIfNecessary(mergedOptions, mergedOptions.Logger);
396+
389397
confidentialClientApplicationOptions.AadAuthorityAudience = mergedOptions.AadAuthorityAudience;
390398
confidentialClientApplicationOptions.AzureCloudInstance = mergedOptions.AzureCloudInstance;
391399
if (string.IsNullOrEmpty(confidentialClientApplicationOptions.AzureRegion) && !string.IsNullOrEmpty(mergedOptions.AzureRegion))
@@ -416,8 +424,6 @@ internal static void UpdateConfidentialClientApplicationOptionsFromMergedOptions
416424

417425
confidentialClientApplicationOptions.EnablePiiLogging = mergedOptions.EnablePiiLogging;
418426

419-
ParseAuthorityIfNecessary(mergedOptions);
420-
421427
if (string.IsNullOrEmpty(confidentialClientApplicationOptions.Instance) && !string.IsNullOrEmpty(mergedOptions.Instance))
422428
{
423429
confidentialClientApplicationOptions.Instance = mergedOptions.Instance;
@@ -438,8 +444,25 @@ internal static void UpdateConfidentialClientApplicationOptionsFromMergedOptions
438444
}
439445
}
440446

441-
internal static void ParseAuthorityIfNecessary(MergedOptions mergedOptions)
447+
internal static void ParseAuthorityIfNecessary(MergedOptions mergedOptions, IdWebLogger.ILogger? logger = null)
442448
{
449+
// Check if Authority is configured but being ignored due to Instance/TenantId taking precedence
450+
if (!string.IsNullOrEmpty(mergedOptions.Authority) &&
451+
(!string.IsNullOrEmpty(mergedOptions.Instance) || !string.IsNullOrEmpty(mergedOptions.TenantId)))
452+
{
453+
// Log warning that Authority is being ignored
454+
if (logger != null)
455+
{
456+
MergedOptionsLogging.AuthorityIgnored(
457+
logger,
458+
mergedOptions.Authority!,
459+
mergedOptions.Instance ?? string.Empty,
460+
mergedOptions.TenantId ?? string.Empty);
461+
}
462+
// Authority is ignored; Instance and TenantId take precedence
463+
return;
464+
}
465+
443466
if (string.IsNullOrEmpty(mergedOptions.TenantId) && string.IsNullOrEmpty(mergedOptions.Instance) && !string.IsNullOrEmpty(mergedOptions.Authority))
444467
{
445468
ReadOnlySpan<char> doubleSlash = "//".AsSpan();
@@ -473,6 +496,11 @@ internal static void UpdateMergedOptionsFromJwtBearerOptions(JwtBearerOptions jw
473496

474497
public void PrepareAuthorityInstanceForMsal()
475498
{
499+
if (string.IsNullOrEmpty(Instance) && string.IsNullOrEmpty(TenantId) && !string.IsNullOrEmpty(Authority))
500+
{
501+
ParseAuthorityIfNecessary(this, this.Logger);
502+
}
503+
476504
if (string.IsNullOrEmpty(Instance))
477505
{
478506
return;

‎src/Microsoft.Identity.Web.TokenAcquisition/MergedOptionsLogging.cs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
using System;
5+
using Microsoft.Extensions.Logging;
6+
7+
namespace Microsoft.Identity.Web
8+
{
9+
/// <summary>
10+
/// High-performance logging for MergedOptions operations.
11+
/// </summary>
12+
internal static partial class MergedOptionsLogging
13+
{
14+
private static readonly Action<ILogger, string, string, string, Exception?> s_authorityIgnored =
15+
LoggerMessage.Define<string, string, string>(
16+
LogLevel.Warning,
17+
LoggingEventId.AuthorityConflict,
18+
"[MsIdWeb] Authority '{Authority}' is being ignored because Instance '{Instance}' and/or TenantId '{TenantId}' are already configured. To use Authority, remove Instance and TenantId from the configuration.");
19+
20+
/// <summary>
21+
/// Logs a warning when Authority is configured alongside Instance and/or TenantId.
22+
/// </summary>
23+
/// <param name="logger">The logger instance.</param>
24+
/// <param name="authority">The Authority value that is being ignored.</param>
25+
/// <param name="instance">The Instance value that takes precedence.</param>
26+
/// <param name="tenantId">The TenantId value that takes precedence.</param>
27+
public static void AuthorityIgnored(
28+
ILogger logger,
29+
string authority,
30+
string instance,
31+
string tenantId)
32+
{
33+
s_authorityIgnored(logger, authority, instance, tenantId, null);
34+
}
35+
}
36+
}

‎src/Microsoft.Identity.Web.TokenAcquisition/MergedOptionsStore.cs‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,35 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4+
using System;
45
using System.Collections.Concurrent;
6+
using Microsoft.Extensions.DependencyInjection;
7+
using Microsoft.Extensions.Logging;
58

69
namespace Microsoft.Identity.Web
710
{
811
internal class MergedOptionsStore : IMergedOptionsStore
912
{
1013
private readonly ConcurrentDictionary<string, MergedOptions> _options;
14+
private readonly ILoggerFactory? _loggerFactory;
1115

12-
public MergedOptionsStore()
16+
public MergedOptionsStore(IServiceProvider serviceProvider)
1317
{
1418
_options = new ConcurrentDictionary<string, MergedOptions>();
19+
_loggerFactory = serviceProvider?.GetService<ILoggerFactory>();
1520
}
1621

1722
public MergedOptions Get(string name)
1823
{
19-
return _options.GetOrAdd(name, key => new MergedOptions());
24+
return _options.GetOrAdd(name, key =>
25+
{
26+
var mergedOptions = new MergedOptions();
27+
if (_loggerFactory != null)
28+
{
29+
mergedOptions.Logger = _loggerFactory.CreateLogger<MergedOptions>();
30+
}
31+
return mergedOptions;
32+
});
2033
}
2134
}
2235
}

‎src/Microsoft.Identity.Web.TokenAcquisition/PublicAPI/net10.0/InternalAPI.Unshipped.txt‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
11
#nullable enable
22
const Microsoft.Identity.Web.Constants.UserIdKey = "IDWEB_USER_ID" -> string!
33
readonly Microsoft.Identity.Web.TokenAcquisition._certificatesObservers -> System.Collections.Generic.IReadOnlyList<Microsoft.Identity.Web.Experimental.ICertificatesObserver!>!
4+
Microsoft.Identity.Web.MergedOptions.Logger.get -> Microsoft.Extensions.Logging.ILogger?
5+
Microsoft.Identity.Web.MergedOptions.Logger.set -> void
6+
Microsoft.Identity.Web.MergedOptionsLogging
7+
Microsoft.Identity.Web.MergedOptionsStore.MergedOptionsStore(System.IServiceProvider! serviceProvider) -> void
8+
static Microsoft.Identity.Web.MergedOptions.ParseAuthorityIfNecessary(Microsoft.Identity.Web.MergedOptions! mergedOptions, Microsoft.Extensions.Logging.ILogger? logger = null) -> void
9+
static Microsoft.Identity.Web.MergedOptionsLogging.AuthorityIgnored(Microsoft.Extensions.Logging.ILogger! logger, string! authority, string! instance, string! tenantId) -> void
410
static Microsoft.Identity.Web.TokenAcquisition.MergeExtraQueryParameters(Microsoft.Identity.Web.MergedOptions! mergedOptions, Microsoft.Identity.Web.TokenAcquisitionOptions? tokenAcquisitionOptions) -> System.Collections.Generic.Dictionary<string!, (string! value, bool includeInCacheKey)>?
11+
static readonly Microsoft.Identity.Web.LoggingEventId.AuthorityConflict -> Microsoft.Extensions.Logging.EventId

‎src/Microsoft.Identity.Web.TokenAcquisition/PublicAPI/net462/InternalAPI.Unshipped.txt‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,9 @@
11
#nullable enable
2+
Microsoft.Identity.Web.MergedOptions.Logger.get -> Microsoft.Extensions.Logging.ILogger?
3+
Microsoft.Identity.Web.MergedOptions.Logger.set -> void
4+
Microsoft.Identity.Web.MergedOptionsLogging
5+
Microsoft.Identity.Web.MergedOptionsStore.MergedOptionsStore(System.IServiceProvider! serviceProvider) -> void
6+
static Microsoft.Identity.Web.MergedOptions.ParseAuthorityIfNecessary(Microsoft.Identity.Web.MergedOptions! mergedOptions, Microsoft.Extensions.Logging.ILogger? logger = null) -> void
7+
static Microsoft.Identity.Web.MergedOptionsLogging.AuthorityIgnored(Microsoft.Extensions.Logging.ILogger! logger, string! authority, string! instance, string! tenantId) -> void
28
static Microsoft.Identity.Web.TokenAcquisition.MergeExtraQueryParameters(Microsoft.Identity.Web.MergedOptions! mergedOptions, Microsoft.Identity.Web.TokenAcquisitionOptions? tokenAcquisitionOptions) -> System.Collections.Generic.Dictionary<string!, (string! value, bool includeInCacheKey)>?
9+
static readonly Microsoft.Identity.Web.LoggingEventId.AuthorityConflict -> Microsoft.Extensions.Logging.EventId

‎src/Microsoft.Identity.Web.TokenAcquisition/PublicAPI/net472/InternalAPI.Unshipped.txt‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,9 @@
11
#nullable enable
2+
Microsoft.Identity.Web.MergedOptions.Logger.get -> Microsoft.Extensions.Logging.ILogger?
3+
Microsoft.Identity.Web.MergedOptions.Logger.set -> void
4+
Microsoft.Identity.Web.MergedOptionsLogging
5+
Microsoft.Identity.Web.MergedOptionsStore.MergedOptionsStore(System.IServiceProvider! serviceProvider) -> void
6+
static Microsoft.Identity.Web.MergedOptions.ParseAuthorityIfNecessary(Microsoft.Identity.Web.MergedOptions! mergedOptions, Microsoft.Extensions.Logging.ILogger? logger = null) -> void
7+
static Microsoft.Identity.Web.MergedOptionsLogging.AuthorityIgnored(Microsoft.Extensions.Logging.ILogger! logger, string! authority, string! instance, string! tenantId) -> void
28
static Microsoft.Identity.Web.TokenAcquisition.MergeExtraQueryParameters(Microsoft.Identity.Web.MergedOptions! mergedOptions, Microsoft.Identity.Web.TokenAcquisitionOptions? tokenAcquisitionOptions) -> System.Collections.Generic.Dictionary<string!, (string! value, bool includeInCacheKey)>?
9+
static readonly Microsoft.Identity.Web.LoggingEventId.AuthorityConflict -> Microsoft.Extensions.Logging.EventId

‎src/Microsoft.Identity.Web.TokenAcquisition/PublicAPI/net8.0/InternalAPI.Unshipped.txt‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,9 @@
11
#nullable enable
2+
Microsoft.Identity.Web.MergedOptions.Logger.get -> Microsoft.Extensions.Logging.ILogger?
3+
Microsoft.Identity.Web.MergedOptions.Logger.set -> void
4+
Microsoft.Identity.Web.MergedOptionsLogging
5+
Microsoft.Identity.Web.MergedOptionsStore.MergedOptionsStore(System.IServiceProvider! serviceProvider) -> void
6+
static Microsoft.Identity.Web.MergedOptions.ParseAuthorityIfNecessary(Microsoft.Identity.Web.MergedOptions! mergedOptions, Microsoft.Extensions.Logging.ILogger? logger = null) -> void
7+
static Microsoft.Identity.Web.MergedOptionsLogging.AuthorityIgnored(Microsoft.Extensions.Logging.ILogger! logger, string! authority, string! instance, string! tenantId) -> void
28
static Microsoft.Identity.Web.TokenAcquisition.MergeExtraQueryParameters(Microsoft.Identity.Web.MergedOptions! mergedOptions, Microsoft.Identity.Web.TokenAcquisitionOptions? tokenAcquisitionOptions) -> System.Collections.Generic.Dictionary<string!, (string! value, bool includeInCacheKey)>?
9+
static readonly Microsoft.Identity.Web.LoggingEventId.AuthorityConflict -> Microsoft.Extensions.Logging.EventId

0 commit comments

Comments
 (0)