Implemented the JWT Token refresh logic through the OpenIdConnectHandler

[mkArtakMSFT]

For now, I only have OpenIDConnect -based refresh support.
Will add OAuth -based support next

[Kahbazi]

?

Suggested change

	/// This is used for refreshing OIDC auth. token if needed.
	/// This is used for refreshing OIDC auth token if needed.

[Kahbazi]

Should the TokenResponseReceived Event get called after getting a new token here?
As a user do I care if a token is generated with code or refresh_token grant type?

[Kahbazi]

Should the tokenRefreshContext.Result be checked like the most of the events? Something like this:

if (tokenRefreshContext.Result != null)
{
    if (tokenRefreshContext.Result.Handled)
    {
        Logger.TokenRefreshingHandledResponse();
    }
    else if (tokenRefreshContext.Result.Skipped)
    {
        Logger.TokenRefreshingSkipped();
    }
}

[Kahbazi]

Does the other parameters need to be added like here?

aspnetcore/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

Lines 1394 to 1398 in 4141e0e

	var issuer = new[] { _configuration.Issuer };
	validationParameters.ValidIssuers = validationParameters.ValidIssuers?.Concat(issuer) ?? issuer;
	validationParameters.IssuerSigningKeys = validationParameters.IssuerSigningKeys?.Concat(_configuration.SigningKeys)
	?? _configuration.SigningKeys;

[josephdecock]

As one of the maintainers of Duende.AccessTokenManagement, I'm really interested in this work and how it will intersect with our implementation. I do see the value in having refresh token support in AspNetCore (that's part of why AccessTokenManagement exists after all!) but I'm worried that users will end up with two different packages trying to refresh their token. Given that the OAuth and OpenIdConnect handlers haven't had refresh token support for so long, tools like mine and others (e.g., Google.Apis.Auth.AspNetCore3) have implemented token refresh, along with the other features in those packages - Duende.AccessTokenManagement has support for many advanced OAuth features such as resource indicators and client assertions for authentication, and obviously the Google package is providing an entire auth handler.

We could use the new event to opt out of the new refresh token support you're building here, but I would much rather have it be an opt in feature, at least for the short to medium term.

The new event doesn't exist in older versions of the handlers that I still want to work with. So hooking into that event will require some conditional compilation in our tools that I'd like to avoid. If you make this feature opt-in initially then we don't have that problem. And someday (probably when the current version of the handlers goes out of support?) you could change your default to be enabled by default. At that point, tools in the ecosystem that would conflict with it would be able to opt out cleanly.

[mkArtakMSFT]

Thanks for your input, @josephdecock. I like the idea of making this an opt-in feature, so I will probably add a flag to control that. I assume that means that the ValidatePrincipal handler will be registered by default only when that flag is set.

As for the rest of the comments (including those from @Kahbazi) I will get to them later, as I'm still learning about the internals of the auth. system.