Is CancellationToken missing in GetAuthorizationContextAsync method?

[KamilBEplan]

Which version of Duende IdentityServer are you using? 7.0.5

Which version of .NET are you using? 8

Describe the bug I have few endpoints where I call: IIdentityServerInteractionService.GetAuthorizationContextAsync method

The issue occurs when user cancels the request before or while the GetAuthorizationContextAsync method is being called. Then I have in logs: dbug: Microsoft.EntityFrameworkCore.Database.Connection[20000] Opening connection to database 'EplanIdentityConfiguration' on server '.'. fail: Microsoft.EntityFrameworkCore.Database.Connection[20004] An error occurred using the connection to database 'EplanIdentityConfiguration' on server '.'.

This is becouse client has canceled the request, but this method has not been canceled and is still trying to call Identity Configuration database.

To Reproduce

The easiest way to reproduce is:

1. Have enpoint where IIdentityServerInteractionService.GetAuthorizationContextAsync is used.
2. Set breakpoint on line with GetAuthorizationContextAsync method
3. Call the endpoint, wait for program to stop on breakpoint
4. Cancel the client request
5. Continue program and call GetAuthorizationContextAsync method

Expected behavior

It's possible to cancel GetAuthorizationContextAsync method (eg. pass CancellationToken as parameter).

[AndersAbel]

In IdentityServer we are not passing the CancellationToken around, instead there is a separate ICancellationTokenProvider that services can use to get access to a cancellation token for the current request. In our EF Core-based stores, we use that to pass in a cancellation token when performing database requests.

What client store implementation are you using? Is it our EF Core-based or do you have your own? If it is our implementation, we might need to look deeper into this to understand why it's not working as expected. If it is your own implementation, you could use the ICancellationTokenProvider to get a cancellation token to pass into any IO-operations.

[KamilBEplan]

I use your ClientStore. Eg. FindClientByIdAsync is one of the places where I encounter the error:

dbug: Microsoft.EntityFrameworkCore.Database.Connection[20005] Creating DbConnection. dbug: Microsoft.EntityFrameworkCore.Database.Connection[20006] Created DbConnection. (29ms). dbug: Microsoft.EntityFrameworkCore.Database.Connection[20000] Opening connection to database 'EplanIdentityConfiguration' on server '.'. fail: Microsoft.EntityFrameworkCore.Database.Connection[20004] An error occurred using the connection to database 'EplanIdentityConfiguration' on server '.'. dbug: Microsoft.EntityFrameworkCore.Query[10115] A query was canceled for context type 'Duende.IdentityServer.EntityFramework.DbContexts.ConfigurationDbContext'.

Here is call stack (Full call stack here)

Duende.IdentityServer.EntityFramework.Storage.dll!Duende.IdentityServer.EntityFramework.Stores.ClientStore.FindClientByIdAsync(string clientId) Line 78 C# Duende.IdentityServer.dll!Duende.IdentityServer.Stores.ValidatingClientStore.FindClientByIdAsync(string clientId) Line 54 C# Duende.IdentityServer.dll!Duende.IdentityServer.Stores.CachingClientStore<Duende.IdentityServer.Stores.ValidatingClientStore>.FindClientByIdAsync.AnonymousMethod__0() Line 51 C# Eplan.IdentityService.dll!Eplan.IdentityService.Services.Caching.DistributedCache.GetOrAddAsync(string key, System.TimeSpan duration, System.Func<System.Threading.Tasks.Task> get) Line 76 C# [Async] Duende.IdentityServer.dll!Duende.IdentityServer.Stores.CachingClientStore<Duende.IdentityServer.Stores.ValidatingClientStore>.FindClientByIdAsync(string clientId) Line 49 C# [Async] Duende.IdentityServer.dll!Duende.IdentityServer.Stores.IClientStoreExtensions.FindEnabledClientByIdAsync(Duende.IdentityServer.Stores.IClientStore store, string clientId) Line 23 C# [Async] Duende.IdentityServer.dll!Duende.IdentityServer.Validation.AuthorizeRequestValidator.LoadClientAsync(Duende.IdentityServer.Validation.ValidatedAuthorizeRequest request) Line 169 C# [Async] Duende.IdentityServer.dll!Duende.IdentityServer.Validation.AuthorizeRequestValidator.ValidateAsync(System.Collections.Specialized.NameValueCollection parameters, System.Security.Claims.ClaimsPrincipal subject, Duende.IdentityServer.Validation.AuthorizeRequestType authorizeRequestType) Line 79
[Async] Duende.IdentityServer.dll!Duende.IdentityServer.Services.OidcReturnUrlParser.ParseAsync(string returnUrl) Line 58 C# [Async] Duende.IdentityServer.dll!Duende.IdentityServer.Services.ReturnUrlParser.ParseAsync(string returnUrl) Line 38 C# [Async] Duende.IdentityServer.dll!Duende.IdentityServer.Services.DefaultIdentityServerInteractionService.GetAuthorizationContextAsync(string returnUrl) Line 55 C# [Async] Eplan.IdentityService.Core.dll!Eplan.IdentityService.Core.Services.Authentication.EplanIdentityServerInteractionService.GetAuthorizationContextAsync(string returnUrl) Line 40 C# [Async] Eplan.IdentityService.dll!Eplan.IdentityService.Controllers.SpaApi.ConfigurationController.GetConfiguration(string returnUrl, System.Threading.CancellationToken cancellationToken) Line 64 C#

[RacerDelux]

Out of curiosity, why do you use ICancellationTokenProvider instead of CancellationToken cancellationToken = default on the function definition?

Isn't defining the cancellation token on the function the more standard way to accomplish this? In my case I am using the IClientStore and was also wondering why the cancellation token is missing on the async methods.

[AndersAbel]

@brockallen Do you have any background/history about this design choice?

[brockallen]

The design of all of our interfaces predates the CT style, and thus the breaking change ripple effect throughout the codebase (and the dozens [if not more] of our interface contracts) was deemed to be too much change for an arguably small benefit. This is why we decided on the ICTP approach.

[RacerDelux]

@brockallen I understand - though couldn't you implement both and have a cancellation token passed in via the optional parameter override the old ICTP approach, or would that cause other issues?

To clarify, will the ICTP approach correctly cancel a task if I abort the call on the web side? If not, this would be an issue for us.

For the time being, I can live with ICTP, but it would be nice if eventually this was changed to support the CT style - if anything else but to conform to standards as much as possible.

[brockallen]

For the time being, I can live with ICTP, but it would be nice if eventually this was changed to support the CT style - if anything else but to conform to standards as much as possible.

Understood, but I think we're talking about changing dozens and dozens (if not more) API signatures.

[josephdecock]

To clarify, will the ICTP approach correctly cancel a task if I abort the call on the web side? If not, this would be an issue for us.

Yes, when an incoming http request is aborted, the provider sees that and underlying tasks do get cancelled. We actually get a lot of questions from folks who see EntityFramework log this as an error, even though it is expected behavior.

[RacerDelux]

To clarify, will the ICTP approach correctly cancel a task if I abort the call on the web side? If not, this would be an issue for us.

Yes, when an incoming http request is aborted, the provider sees that and underlying tasks do get cancelled. We actually get a lot of questions from folks who see EntityFramework log this as an error, even though it is expected behavior.

That is kind of funny. When I found out that was a feature, I was ecstatic. The amount of unneeded work that can be avoided when doing things like type ahead searches is crazy. Over a month, I saw over 200k database request reduction.

[RolandGuijt]

@RacerDelux Am I correct in assuming your questions are answered? If so I would like to close this issue. If not, feel free to add a comment.

[RacerDelux]

@RacerDelux Am I correct in assuming your questions are answered? If so I would like to close this issue. If not, feel free to add a comment.

Answered thanks. I would still love to see the work to refactor Duende to fall in line with more current practices involving cancellation tokens, but I understand the desire to focus on other items due to the work required.