-
Notifications
You must be signed in to change notification settings - Fork 249
Fix override merging for app token (and others) #3644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix override merging for app token (and others) #3644
Conversation
bgavrilMS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @christian-posta for raising and fixing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
93b33c5 to
5748d52
Compare
|
@christian-posta - Can you fix the failing unit test? |
Fix override merging for app token
Description
Fix some missing override parameters including requestAppToken
Fixes #3643