Adding fix for OBO cache key error in long running OBO with DefaultAuthorizationHeaderProvider #3381
Conversation
src/Microsoft.Identity.Web.TokenAcquisition/DefaultAuthorizationHeaderProvider.cs
Outdated
Show resolved
Hide resolved
| @@ -27,13 +27,16 @@ public async Task<string> CreateAuthorizationHeaderForUserAsync( | |||
| ClaimsPrincipal? claimsPrincipal = null, | |||
| CancellationToken cancellationToken = default) | |||
| { | |||
| var newTokenAcquisitionOptions = CreateTokenAcquisitionOptionsFromApiOptions(downstreamApiOptions, cancellationToken); | |||
There was a problem hiding this comment.
can you help me understand this a bit more?
Does this option get captured in the result object as part of GetAuthenticationResultForUserAsync/GetAuthenticationResultForAppAsync?
Can you point to where the modification is to AcquireTokenOptions?
Would it be a better fix to make it so that the code that is changing LongRunningWebApiSessionKey doesn't mutate input?
There was a problem hiding this comment.
Yeah, it is an interesting design, I would also prefer that it did not mutate the AcquireTokenOptions after the authentication but this is how it works at the moment. This PR is to fix this bug in particular, but a follow up PR can be used to refactor how the Long running OBO works all together. It would take some discussion on the API and user experience.
When the options are provided with a LongRunningWebApiSessionKey that is equal to LongRunningWebApiSessionKeyAuto ("AllocateForMe") , the code will set this value to the cache key that MSAL calculates after initiating the long running OBO session. It is then expected for the developer to save the updated key from the modified AcquireTokenOptions and provide it in subsequent authentication calls.
See https://github.com/AzureAD/microsoft-identity-web/wiki/get-token-in-event-handler#principle
and
There was a problem hiding this comment.
Also, it does not happen for GetAuthenticationResultForAppAsync since this code path does not use msal's OBO flow
There was a problem hiding this comment.
FYI @jmprieur @bgavrilMS ^
| claimsPrincipal); | ||
|
|
||
| Assert.NotNull(result); | ||
| Assert.NotEqual(options.AcquireTokenOptions.LongRunningWebApiSessionKey, TokenAcquisitionOptions.LongRunningWebApiSessionKeyAuto); |
There was a problem hiding this comment.
It is set to the hash of the AT, which is acquired from MSAL
|
|
||
| using (mockHttpClient) | ||
| { | ||
| mockHttpClient!.AddMockHandler(MockHttpCreator.CreateClientCredentialTokenHandler()); |
There was a problem hiding this comment.
You should add a mock response that has AT, RT, IdT
|
|
||
| using (mockHttpClient) | ||
| { | ||
| mockHttpClient!.AddMockHandler(MockHttpCreator.CreateClientCredentialTokenHandler()); |
There was a problem hiding this comment.
You should add a mock response that has AT, RT, IdT, client_info
|
|
||
| using (mockHttpClient) | ||
| { | ||
| mockHttpClient!.AddMockHandler(MockHttpCreator.CreateClientCredentialTokenHandler()); |
There was a problem hiding this comment.
You should add a mock response that has AT, RT, IdT, client_info
Description
Updates the
DefaultAuthorizationHeaderProviderto update theAcquireTokenOptions.LongRunningWebApiSessionKeyafter the token is acquired so that the key can be used in the next OBO call.Fixes #3382