-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Update user on reconnect. Fixes #12051 #12421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update user on reconnect. Fixes #12051 #12421
Conversation
using Microsoft.AspNetCore.Components.Routing; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Http.Features; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Logging; | ||
using Microsoft.JSInterop; | ||
using System.Threading.Tasks; | ||
using System.Security.Claims; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort 😆
@@ -109,6 +110,16 @@ public Task<ComponentRenderedText> PrerenderComponentAsync(Type componentType, P | |||
}); | |||
} | |||
|
|||
public void SetCircuitUser(ClaimsPrincipal user) | |||
{ | |||
var authenticationStateProvider = Services.GetService<AuthenticationStateProvider>() as IHostEnvironmentAuthenticationStateProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make this fail hard if the cast doesn't succeed? For instance if someone wanted to customize the auth state provider, they could easily miss this detail and go into the weeds. I don't have a strong feeling about which is better to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is for "receiving the user info from the host environment" to be an optional feature for authentication state providers. If you want to build a custom one, maybe you want it to do something with ASP.NET Core's normal server-side User
(in which case, implement IHostEnvironmentAuthenticationStateProvider
), or maybe you don't (in which case, don't).
As long as this is documented with other coverage for "building a custom authentication state provider" then I think people will be on track to succeed.
97bec92
to
d9298d2
Compare
d9298d2
to
bbd895b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, It totally got lost on my stream of reviews. It looks great!
This address the issue in #12051, which is that Windows authentication becomes broken if you reconnect. Previously it continued trying to use the circuit's original
ClaimsPrincipal
which becomes invalid once the SignalR connection drops, because SignalR disposes theWindowsPrincipal
.The fix is to update the circuit's authentication state to match whatever SignalR gives us each time the user connects. This means we have the same level of Windows Authentication support that SignalR itself does:
ClaimsPrincipal
fromhubContext.User
. This has some limitations with Windows Authentication (because they clone the principal) but it's the same limitations that SignalR itself has.Important This also changes how other auth types work with server-side Blazor. Now, whenever you reconnect, we will update the circuit to the latest
ClaimsPrincipal
for the connection. This means you will observe any changes to roles, etc. If you're using cookie auth and the user has logged out or their cookie expired, then on reconnection they will become logged out, etc. If they logged in as a different person entirely, then on reconnection the circuit becomes that new user. This is consistent with how Blazor's authentication system was originally designed to support changing the authentication state arbitrarily during the lifetime of the circuit: all the UI will update to reflect the new state, declarative authentication for the current route will be re-evaluated, etc.@blowdart @GrabYourPitchforks I'm not expecting you to review the code (but feel free to if you want). I'm including you on the list of reviewers to make sure you're up-to-date on where this all ended up. Please let me know if you have any thoughts about it!