From 46854dd552e533af32380209f38e361af5fc19c8 Mon Sep 17 00:00:00 2001 From: Christian Posta Date: Fri, 5 Dec 2025 14:14:34 -0700 Subject: [PATCH 1/3] Fix override merging for app token (and others), fixes https://github.com/AzureAD/microsoft-identity-web/issues/3643 --- .../DownstreamApiOptionsMerger.cs | 50 +++++++- .../DownstreamApiOptionsMergeTests.cs | 111 +++++++++++++++++- 2 files changed, 158 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Identity.Web.Sidecar/DownstreamApiOptionsMerger.cs b/src/Microsoft.Identity.Web.Sidecar/DownstreamApiOptionsMerger.cs index 97e3c1423..c285e3613 100644 --- a/src/Microsoft.Identity.Web.Sidecar/DownstreamApiOptionsMerger.cs +++ b/src/Microsoft.Identity.Web.Sidecar/DownstreamApiOptionsMerger.cs @@ -21,6 +21,37 @@ public static DownstreamApiOptions MergeOptions(DownstreamApiOptions left, Downs res.Scopes = right.Scopes; } + // RequestAppToken determines whether to use client credentials (app token) or user delegation (OBO) + if (right.RequestAppToken) + { + res.RequestAppToken = right.RequestAppToken; + } + + if (!string.IsNullOrEmpty(right.BaseUrl)) + { + res.BaseUrl = right.BaseUrl; + } + + if (!string.IsNullOrEmpty(right.RelativePath)) + { + res.RelativePath = right.RelativePath; + } + + if (!string.IsNullOrEmpty(right.HttpMethod)) + { + res.HttpMethod = right.HttpMethod; + } + + if (!string.IsNullOrEmpty(right.ContentType)) + { + res.ContentType = right.ContentType; + } + + if (!string.IsNullOrEmpty(right.AcceptHeader)) + { + res.AcceptHeader = right.AcceptHeader; + } + if (!string.IsNullOrEmpty(right.AcquireTokenOptions.Tenant)) { res.AcquireTokenOptions.Tenant = right.AcquireTokenOptions.Tenant; @@ -41,9 +72,24 @@ public static DownstreamApiOptions MergeOptions(DownstreamApiOptions left, Downs res.AcquireTokenOptions.FmiPath = right.AcquireTokenOptions.FmiPath; } - if (!string.IsNullOrEmpty(right.RelativePath)) + if (!string.IsNullOrEmpty(right.AcquireTokenOptions.LongRunningWebApiSessionKey)) { - res.RelativePath = right.RelativePath; + res.AcquireTokenOptions.LongRunningWebApiSessionKey = right.AcquireTokenOptions.LongRunningWebApiSessionKey; + } + + if (!string.IsNullOrEmpty(right.AcquireTokenOptions.PopPublicKey)) + { + res.AcquireTokenOptions.PopPublicKey = right.AcquireTokenOptions.PopPublicKey; + } + + if (right.AcquireTokenOptions.CorrelationId != Guid.Empty) + { + res.AcquireTokenOptions.CorrelationId = right.AcquireTokenOptions.CorrelationId; + } + + if (right.AcquireTokenOptions.ManagedIdentity is not null) + { + res.AcquireTokenOptions.ManagedIdentity = right.AcquireTokenOptions.ManagedIdentity; } res.AcquireTokenOptions.ForceRefresh = right.AcquireTokenOptions.ForceRefresh; diff --git a/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs b/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs index ebbe22715..0b911a6b1 100644 --- a/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs +++ b/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs @@ -3,7 +3,6 @@ using Microsoft.Identity.Abstractions; using Microsoft.Identity.Web.Sidecar; -using Microsoft.Identity.Web.Sidecar.Endpoints; using Xunit; namespace Sidecar.Tests; @@ -553,4 +552,114 @@ public void MergeDownstreamApiOptionsOverrides_WithRightExtraQueryParametersButL Assert.Single(result.ExtraQueryParameters); Assert.Equal("value1", result.ExtraQueryParameters["param1"]); } + + [Fact] + public void MergeDownstreamApiOptionsOverrides_WithRequestAppTokenTrue_OverridesRequestAppToken() + { + // Arrange + var left = new DownstreamApiOptions + { + RequestAppToken = false + }; + var right = new DownstreamApiOptions + { + RequestAppToken = true + }; + + // Act + var result = DownstreamApiOptionsMerger.MergeOptions(left, right); + + // Assert + Assert.True(result.RequestAppToken); + } + + [Fact] + public void MergeDownstreamApiOptionsOverrides_WithRequestAppTokenFalse_DoesNotOverride() + { + // Arrange + var left = new DownstreamApiOptions + { + RequestAppToken = true + }; + var right = new DownstreamApiOptions + { + RequestAppToken = false + }; + + // Act + var result = DownstreamApiOptionsMerger.MergeOptions(left, right); + + // Assert - left value preserved because right is false (default) + Assert.True(result.RequestAppToken); + } + + [Fact] + public void MergeDownstreamApiOptionsOverrides_WithBaseUrlOverride_OverridesBaseUrl() + { + // Arrange + var left = new DownstreamApiOptions + { + BaseUrl = "https://original.api.com/" + }; + var right = new DownstreamApiOptions + { + BaseUrl = "https://new.api.com/" + }; + + // Act + var result = DownstreamApiOptionsMerger.MergeOptions(left, right); + + // Assert + Assert.Equal("https://new.api.com/", result.BaseUrl); + } + + [Fact] + public void MergeDownstreamApiOptionsOverrides_WithHttpMethodOverride_OverridesHttpMethod() + { + // Arrange + var left = new DownstreamApiOptions + { + HttpMethod = "GET" + }; + var right = new DownstreamApiOptions + { + HttpMethod = "POST" + }; + + // Act + var result = DownstreamApiOptionsMerger.MergeOptions(left, right); + + // Assert + Assert.Equal("POST", result.HttpMethod); + } + + [Fact] + public void MergeDownstreamApiOptionsOverrides_WithManagedIdentityOverride_OverridesManagedIdentity() + { + // Arrange + var left = new DownstreamApiOptions + { + AcquireTokenOptions = new AcquireTokenOptions + { + ManagedIdentity = null + } + }; + var right = new DownstreamApiOptions + { + AcquireTokenOptions = new AcquireTokenOptions + { + ManagedIdentity = new ManagedIdentityOptions + { + UserAssignedClientId = "test-client-id" + } + } + }; + + // Act + var result = DownstreamApiOptionsMerger.MergeOptions(left, right); + + // Assert + Assert.NotNull(result.AcquireTokenOptions.ManagedIdentity); + Assert.Equal("test-client-id", result.AcquireTokenOptions.ManagedIdentity.UserAssignedClientId); + } } From f771de082328570d0f2031c521e12a443e8223fc Mon Sep 17 00:00:00 2001 From: Christian Posta Date: Mon, 8 Dec 2025 14:50:43 -0700 Subject: [PATCH 2/3] added reflection test --- .../DownstreamApiOptionsMergeTests.cs | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs b/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs index 0b911a6b1..377406079 100644 --- a/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs +++ b/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs @@ -662,4 +662,97 @@ public void MergeDownstreamApiOptionsOverrides_WithManagedIdentityOverride_Overr Assert.NotNull(result.AcquireTokenOptions.ManagedIdentity); Assert.Equal("test-client-id", result.AcquireTokenOptions.ManagedIdentity.UserAssignedClientId); } + + /// + /// This test uses reflection to ensure all properties of DownstreamApiOptions are handled by the merger. + /// If a new property is added to DownstreamApiOptions and not handled, this test will fail. + /// + [Fact] + public void MergeDownstreamApiOptionsOverrides_AllPropertiesAreCopied() + { + // Arrange - Properties that are expected to be merged from DownstreamApiOptions + var handledProperties = new HashSet(StringComparer.OrdinalIgnoreCase) + { + // Direct properties + "Scopes", + "RequestAppToken", + "BaseUrl", + "RelativePath", + "HttpMethod", + "ContentType", + "AcceptHeader", + "ExtraHeaderParameters", + "ExtraQueryParameters", + // AcquireTokenOptions is handled specially (nested object) + "AcquireTokenOptions", + // Properties intentionally not merged (they use CustomizeHttpRequestMessage pattern or are clone-only) + "Clone", + "CustomizeHttpRequestMessage", + "Serializer", + "Deserializer", + "ProtocolScheme", + }; + + // Act - Get all public instance properties of DownstreamApiOptions + var allProperties = typeof(DownstreamApiOptions) + .GetProperties(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance) + .Where(p => p.CanRead && p.CanWrite) + .Select(p => p.Name) + .ToList(); + + // Assert - Every property should be in our handled list + var unhandledProperties = allProperties + .Where(p => !handledProperties.Contains(p)) + .ToList(); + + Assert.True( + unhandledProperties.Count == 0, + $"The following properties of DownstreamApiOptions are not handled by the merger: {string.Join(", ", unhandledProperties)}. " + + "Please add handling for these properties in DownstreamApiOptionsMerger.MergeOptions() and add them to this test's handledProperties list."); + } + + /// + /// This test uses reflection to ensure all properties of AcquireTokenOptions are handled by the merger. + /// + [Fact] + public void MergeDownstreamApiOptionsOverrides_AllAcquireTokenOptionsPropertiesAreCopied() + { + // Arrange - Properties that are expected to be merged from AcquireTokenOptions + var handledProperties = new HashSet(StringComparer.OrdinalIgnoreCase) + { + "Tenant", + "Claims", + "AuthenticationOptionsName", + "FmiPath", + "LongRunningWebApiSessionKey", + "PopPublicKey", + "CorrelationId", + "ManagedIdentity", + "ForceRefresh", + "ExtraQueryParameters", + "ExtraParameters", + "ExtraHeadersParameters", + // Properties intentionally not merged + "UserFlow", + "PopCryptoProvider", + "PoPConfiguration", + }; + + // Act - Get all public instance properties of AcquireTokenOptions + var allProperties = typeof(AcquireTokenOptions) + .GetProperties(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance) + .Where(p => p.CanRead && p.CanWrite) + .Select(p => p.Name) + .ToList(); + + // Assert - Every property should be in our handled list + var unhandledProperties = allProperties + .Where(p => !handledProperties.Contains(p)) + .ToList(); + + Assert.True( + unhandledProperties.Count == 0, + $"The following properties of AcquireTokenOptions are not handled by the merger: {string.Join(", ", unhandledProperties)}. " + + "Please add handling for these properties in DownstreamApiOptionsMerger.MergeOptions() and add them to this test's handledProperties list."); + } } From 4b861af069f7a15acdb24a4f8b7fece3c38dc309 Mon Sep 17 00:00:00 2001 From: Bogdan Gavril Date: Wed, 4 Feb 2026 13:41:34 +0000 Subject: [PATCH 3/3] Fix test --- .../DownstreamApiOptionsMerger.cs | 5 +++++ .../Sidecar.Tests/DownstreamApiOptionsMergeTests.cs | 1 + 2 files changed, 6 insertions(+) diff --git a/src/Microsoft.Identity.Web.Sidecar/DownstreamApiOptionsMerger.cs b/src/Microsoft.Identity.Web.Sidecar/DownstreamApiOptionsMerger.cs index c285e3613..7251f2272 100644 --- a/src/Microsoft.Identity.Web.Sidecar/DownstreamApiOptionsMerger.cs +++ b/src/Microsoft.Identity.Web.Sidecar/DownstreamApiOptionsMerger.cs @@ -82,6 +82,11 @@ public static DownstreamApiOptions MergeOptions(DownstreamApiOptions left, Downs res.AcquireTokenOptions.PopPublicKey = right.AcquireTokenOptions.PopPublicKey; } + if (!string.IsNullOrEmpty(right.AcquireTokenOptions.PopClaim)) + { + res.AcquireTokenOptions.PopClaim = right.AcquireTokenOptions.PopClaim; + } + if (right.AcquireTokenOptions.CorrelationId != Guid.Empty) { res.AcquireTokenOptions.CorrelationId = right.AcquireTokenOptions.CorrelationId; diff --git a/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs b/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs index 377406079..718947df1 100644 --- a/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs +++ b/tests/E2E Tests/Sidecar.Tests/DownstreamApiOptionsMergeTests.cs @@ -732,6 +732,7 @@ public void MergeDownstreamApiOptionsOverrides_AllAcquireTokenOptionsPropertiesA "ExtraQueryParameters", "ExtraParameters", "ExtraHeadersParameters", + "PopClaim", // Properties intentionally not merged "UserFlow", "PopCryptoProvider",