Make ConfidentialClientApplicationBuilderExtension.WithClientCredentials async #2567
Conversation
|
@microsoft-github-policy-service agree |
| /// <param name="authenticationScheme">Authentication scheme. If null, will use OpenIdConnectDefault.AuthenticationScheme | ||
| /// if called from a web app, and JwtBearerDefault.AuthenticationScheme if called from a web API.</param> | ||
| /// <param name="httpResponse">The <see cref="HttpResponse"/> to update.</param> | ||
| Task ReplyForbiddenWithWwwAuthenticateHeaderAsync( |
There was a problem hiding this comment.
This is a breaking change in a public interface: https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/breaking-changes
Can you elaborate on why this is needed?
There was a problem hiding this comment.
I love the fact that the credential loaders can be called async.
For the WWAuthenticate, indeed, how is it related?
There was a problem hiding this comment.
Of coarse I can elaborate. The method ReplyForbiddenWithWwwAuthenticateHeader uses the internal method GetOrBuildConfidentialClientApplication, which calls the private method BuildConfidentialClientApplication which calls builder.WithClientCredentials. Because I made the last method async, I also think the path with ReplyForbiddenWithWwwAuthenticateHeader should be made async.
The only thing about this change that is breaking is that I added and removed the [Obsolete] attribute where applicable. I removed it from the ITokenAcquisition.ReplyForbiddenWithWwwAuthenticateHeaderAsync since that should be the method used instead of the ITokenAcquisition.ReplyForbiddenWithWwwAuthenticateHeader. I've kept the sync version, added [Obsolete] as a warning and implemented it using a GetAwaiter().GetResult(). The consumers of this Interface initially don't need to change anything and just receive a warning the sync-method is obsolete.
If you think I should solve this (ReplyForbiddenWithWwwAuthenticateHeader) another way, I would love any input and will gladly implement/change that in my PR.
There was a problem hiding this comment.
This change adds a method to the interface, ReplyForbiddenWithWwwAuthenticateHeaderAsync that also takes an authenticationScheme, that is a breaking change for us. From the doc I linked:
Adding anything to an interface will cause existing types that implement it to fail.There was a problem hiding this comment.
Thanks for explaining @keegan-caruso. I'm not used to these requirements, because I mainly work on internal software, but thinking it through I understand your perspective. Sorry for not picking it up right after your first message.
I've added another commit to my PR where I remove the changes in the ITokenAcquisition interface.
This introduces a new GetAwaiter().GetResult() in the implementation of the sync version of ReplyForbiddenWithWwwAuthenticateHeader, but users already have the option to use the async version: ReplyForbiddenWithWwwAuthenticateHeaderAsync.
What do you prefer I do with the [Obsolete]-attributes? Are they correct the way the are now in my PR? Or do you want to redirect users using the sync version ReplyForbiddenWithWwwAuthenticateHeader towards the async version ReplyForbiddenWithWwwAuthenticateHeaderAsync?
There was a problem hiding this comment.
not a worry at all 😄
We will work on adding the breaking api check to automation soon, it will be much clearer for contributors then.
I'm going to defer to @jmprieur on the API / obsolete questions
There was a problem hiding this comment.
Any update on this PR @keegan-caruso or @jmprieur ?
I've been running the code from this branch in production for over a month now, and haven't seen any blocked threads any more (while more users are using our Blazor Server-website). So I'm confident making this path async solved our problem.
There was a problem hiding this comment.
We cannot take a breaking change without doing a major version, which we are not quite ready for. Are you willing to keep this branch up to date until we take a 3x (maybe May/June, but really cannot commit to anything right now), otherwise will try to think of another way, such as using an extension method.
There was a problem hiding this comment.
@jennyf19 thanks for your reply!
Could you please elaborate on what you consider a "breaking change" in my current PR?
I have the feeling that I've removed all things that are considered breaking in my latest commit (no interfaces are changed and no public classes are changed, only an internal class).
The only question I still have is is removing an Obsolete-attribute considered breaking? I could also put that attribute back if this is considered breaking.
If needed I can update this branch until the next major release, but I'm hoping this isn't necessary.
There was a problem hiding this comment.
Got it, thanks @xs4free . This won't make the 2.17.1 release (this week), but our next one. I'd like some time to review and will try to prioritize for that by end of week. Appreciate your patience on this.
# Conflicts: # src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs # tests/Microsoft.Identity.Web.Test/TokenAcquisitionAuthorityTests.cs
Make calls to
ConfidentialClientApplicationBuilderExtension.WithClientCredentialsfully async.Description
Instead of using
GetAwaiter().GetResult()in the code, I've added an overloadWithClientCredentialsAsyncwhich calls all credential-loaders asynchronously. This probably fixes our bug in production which hangs our Blazor Server during login.Fixes #2566