Fix override merging for app token (and others)#3644
Conversation
bgavrilMS
left a comment
bgavrilMS
left a comment
There was a problem hiding this comment.
I would add a test that uses reflection to check that each property is copied.
|
LMK if this is what you had in mind @bgavrilMS |
jmprieur
left a comment
jmprieur
left a comment
There was a problem hiding this comment.
LGTM
Thanks @christian-posta for raising and fixing
There was a problem hiding this comment.
Pull request overview
This PR fixes missing override parameter merging in the DownstreamApiOptionsMerger class by adding support for several previously unhandled properties including RequestAppToken, HTTP-related properties (BaseUrl, HttpMethod, ContentType, AcceptHeader), and additional AcquireTokenOptions properties.
Key Changes:
- Added merging logic for
RequestAppTokento enable proper app token vs user delegation override control - Added merging support for HTTP configuration properties (
BaseUrl,HttpMethod,ContentType,AcceptHeader) - Added merging support for additional token acquisition options (
LongRunningWebApiSessionKey,PopPublicKey,CorrelationId,ManagedIdentity)
bgavrilMS
force-pushed
the
ceposta-fix-requestapptoken
branch
from
93b33c5 to
5748d52
Compare
|
@christian-posta - Can you fix the failing unit test? |
bgavrilMS
force-pushed
the
ceposta-fix-requestapptoken
branch
from
5748d52 to
f771de0
Compare
|
@christian-posta - I pushed a test fix to your branch. I hope this is ok. Your commits are still attributed to you of course. |
Fix override merging for app token
Description
Fix some missing override parameters including requestAppToken
Fixes #3643