Update Secure an ASP.NET Core Blazor Web App with OpenID Connect (OIDC) to demonstrate Access Token Management #570
Conversation
…e Access Token Management
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Ok ... we're back in business here. So for future reference, always open a documentation issue for anything other than a minor patch. We have a lot going on with docs, and BIG 🐘 surprises don't go over too well around here 😆. Let's start with the technical review of this work. After this is resolved and staged for merging, we'll flip over to the article. Note in passing that after Mike/Stephen have signed off here that we'll need the updates made to the 8.0 version of this sample, too. If we don't also adopt this in the 8.0 sample, we'll have a devil of a time with versioning in the article later. Wait on making changes to the 8.0 sample. Let's get this signed off for the 9.0 sample first, and then you can do the 8.0 sample updates. That way, you'll be making the 8.0 changes from a stable app after review feedback is addressed. |
| @@ -8,6 +8,7 @@ | |||
|
|
|||
| <ItemGroup> | |||
| <ProjectReference Include="..\BlazorWebAppOidc.Client\BlazorWebAppOidc.Client.csproj" /> | |||
| <PackageReference Include="Duende.AccessTokenManagement.OpenIdConnect" Version="4.0.0-rc.2" /> | |||
There was a problem hiding this comment.
RTM is around the corner and will be updated
There was a problem hiding this comment.
I asked offline about this in a separate message chain. Generally, they aren't keen on recommending preview packages in production, which is what our readers do with these samples. Will the 3.2.0 package not work for this? If not, when is "around the corner"? Is it like a week ... or month?
There was a problem hiding this comment.
End of August most probably, unless anything slips unexpectedly
There was a problem hiding this comment.
Can you provide a brief summary of why we should use Duende.AccessTokenManagement.OpenIdConnect over the custom CookieOidcRefresher?
I know it's nice to have the common logic to refresh OAuth tokens in a library rather than app code. I'm also assuming that if you have an expired token during a long-lived Blazor Server sessions, that the DelegatingHandler added by AddUserAccessTokenHandler will take care of refreshing it whereas previously, we'd only refresh the cookie during incoming requests when it's still possible to set a response header? Is that true? Is there any other benefit to this change?
If that's basically it, I wonder if we should have a Blazor OIDC sample without server interactivity that still demonstrates CookieOidcRefresher, because it still has some advantages like working even behind non-sticky load balancers, and not forcing you to reauthenticate just because the server restarted.
This advantage doesn't matter as much if you're using server interactivity anyways, but it can be a big deal otherwise. And even in the interactive server case, it's annoying to have to login again after the server restarts. And the CookieOidcRefresher works well enough even in interactive server scenarios so long as your access token lifetime outlives your Blazor WebSocket connection.
Also, would it be worth demonstrating how to acquire a token that automatically gets refreshed if necessary (so not directly from the store) without using the DelegatingHandler?
|
|
||
| public CookieEvents(IUserTokenStore store) => _store = store; | ||
|
|
||
| public override async Task ValidatePrincipal(CookieValidatePrincipalContext context) |
There was a problem hiding this comment.
I think we should use cookieOptions.Events.OnValidatePrincipal = ... like before rather than have all the ceremony of a custom Events types. Same goes for OidcEvents.
| var token = await _store.GetTokenAsync(context.Principal!); | ||
| if (!token.Succeeded) | ||
| { | ||
| context.RejectPrincipal(); |
There was a problem hiding this comment.
It appears that the sample no longer rejects the cookie if the token has expired and you cannot use the refresh token to get an unexpired token. Can we add that functionality back?
Also, if the token is missing from the store, wouldn't it be better just to add it back from the store rather than reject the cookie. Since we're still using SaveTokens, it should still be stored int the ClaimsPrincipal.
If we cannot do that, maybe we should log something here. I know we weren't before, but I could see this coming up a lot more often now that something like a server restart can cause it.
| /// </summary> | ||
| public class ServerSideTokenStore : IUserTokenStore | ||
| { | ||
| private static readonly ConcurrentDictionary<string, TokenForParameters> _tokens = new(); |
There was a problem hiding this comment.
Are there any built-in IUserTokenStore we can use? Should we do anything to clean up stale tokens? The MSAL version of this, MsalMemoryTokenCacheProvider, uses IMemoryCache which deals with this automatically.
Even if it was just in-memory, it'd be nice to be able to reference a library-provided IUserTokenStore that deals with cleanup. The only thing potentially custom here so far seems to be using the "sub" claim as the unique identifier.
[EDIT by guardrex to add the issue]
Fixes #571
As discussed with @mikekistler