Add support for calling WithClientClaims flow for token acquisition #3623
Conversation
| private static IDictionary<string, string>? GetClientClaimsIfExist(TokenAcquisitionOptions? tokenAcquisitionOptions) | ||
| { | ||
| IDictionary<string, string>? clientClaims = null; | ||
| if (tokenAcquisitionOptions is not null && tokenAcquisitionOptions.ExtraParameters is not null && |
There was a problem hiding this comment.
Hmm, im not sure it is best to use client claims with the ExtraParameters collection. This collection is meant for very generic parameters that we dont know all the details of. For example, the attribute feature was not meant to be exposed in MSAL/IdWeb so this generic collection was used. I think in this case, we should expose the client claims in the abstractions because it is a known value for MSAL. CC @bgavrilMS
There was a problem hiding this comment.
+1 to what Travis is saying. If developers need to know about it it should be in Abstractions. If we don't want people to mess around with it it should be in ExtraParameters.
Aside:
- is it a per request or per app parameter. If it's considered at CCA building time as it seems to be, shouldn't it be in MicrosoftIdentityApplicationOptions ?
There was a problem hiding this comment.
- I think it is a fairly obscure scenario , I am not aware of any 3p needing this. I'm fine either way, with a slight inclination towards keeping it in ExtraParameters
- yeah in MSAL it is on a "per app" basis. I believe the CX needs it on a per app basis as well - @trwalke pls confirm.
There was a problem hiding this comment.
@bgavrilMS @trwalke please ping me offline with the customer scenario.
This PR, @trwalke, is not compatible with AzureAD/microsoft-identity-abstractions-for-dotnet#230
There was a problem hiding this comment.
Yes, I agree with that comment. This was a PoC. The whole point was to make it work quickly with avoiding having to update abstractions, but then we agreed that it could stay that way (?)
There was a problem hiding this comment.
Currently customers were passing it per request with Adal... but Im not sure it would be changing that often. There would probably be a grouping per application.
There was a problem hiding this comment.
It could. I just don't understand if it's on the request or the app. I see both in different places in these 2 prs
| public HttpClient GetHttpClient() => _httpClient; | ||
| } | ||
|
|
||
| private class CapturingHandler : HttpMessageHandler |
There was a problem hiding this comment.
we already have a mocking framework to capture MSAL traffic and reply with test responses. i can update the tests to use them
see: https://github.com/AzureAD/microsoft-identity-web/blob/master/tests/Microsoft.Identity.Web.Test.Common/Mocks/MockHttpClientFactory.cs
src/Microsoft.Identity.Web.TokenAcquisition/ConfidentialClientApplicationBuilderExtension.cs
Show resolved
Hide resolved
src/Microsoft.Identity.Web.TokenAcquisition/ConfidentialClientApplicationBuilderExtension.cs
Show resolved
Hide resolved
| private static IDictionary<string, string>? GetClientClaimsIfExist(TokenAcquisitionOptions? tokenAcquisitionOptions) | ||
| { | ||
| IDictionary<string, string>? clientClaims = null; | ||
| if (tokenAcquisitionOptions is not null && tokenAcquisitionOptions.ExtraParameters is not null && |
There was a problem hiding this comment.
+1 to what Travis is saying. If developers need to know about it it should be in Abstractions. If we don't want people to mess around with it it should be in ExtraParameters.
Aside:
- is it a per request or per app parameter. If it's considered at CCA building time as it seems to be, shouldn't it be in MicrosoftIdentityApplicationOptions ?
Add support for calling WithClientClaims flow for token acquisition
Enabling MSAL's WithClientClaims flavor of client assertions call and exposing it through IdWeb.