Revalidating auth improvement. Fixes #12692 #12909
Conversation
src/Components/Server/src/Circuits/RevalidatingServerAuthenticationStateProvider.cs
Show resolved
Hide resolved
...tes/content/BlazorServerWeb-CSharp/Areas/Identity/RevalidatingAuthenticationStateProvider.cs
Outdated
Show resolved
Hide resolved
SteveSandersonMS
added
the
tell-mode
src/Components/Server/src/Circuits/RevalidatingServerAuthenticationStateProvider.cs
Outdated
Show resolved
Hide resolved
|
|
||
| while (!cancellationToken.IsCancellationRequested) | ||
| // Get the user manager from a new scope to ensure it fetches fresh data | ||
| using (var scope = _scopeFactory.CreateScope()) |
There was a problem hiding this comment.
This needs to be await using #12918
There was a problem hiding this comment.
Not sure it will be that easy since IServiceScope doesn't derive from IAsyncDisposable (only the MSFT DI implementation does). You'll likely have to do some as casting 😕
There was a problem hiding this comment.
That's quite a nasty mess given that this is going into user code in the template.
It feels like await using should be able to work with IDisposable too. If this can be implemented in user code without there being anything opinionated about it, the compiler should be able to codegen the same.
There was a problem hiding this comment.
I've handled it in what seems to be the most beginner-friendly manner, but it's still verbose and lame 😞
...tes/content/BlazorServerWeb-CSharp/Areas/Identity/RevalidatingAuthenticationStateProvider.cs
Outdated
Show resolved
Hide resolved
...tes/content/BlazorServerWeb-CSharp/Areas/Identity/RevalidatingAuthenticationStateProvider.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // Act | ||
| ((IDisposable)provider).Dispose(); | ||
| await Task.Delay(200); |
There was a problem hiding this comment.
Think about providing access to the actual validation task somehow so we can await it. This kind of thing ends up being flake eventually usually. Yeah I know that 200 > 50
There was a problem hiding this comment.
That's what I have done in the cases where there is a validation task that can be awaited (it's NextValidateAuthenticationStateAsyncCall).
However for the tests where we're asserting that no further calls get made, such as this one, there's no validation task to await.
There was a problem hiding this comment.
Without changing the public API in an unnatural test-centric manner (e.g., passing in some IDelayTaskProvider) I think this is the best we can do, so I propose leaving it. I haven't observed any hints of flakiness when running it locally so far.
If this does become a problem in the future, we do have the option to refactor without breaking any compatibility, e.g.:
- Add a new
internalproperty of typeIDelayTaskProvider - If that is assigned, then use it instead of
Task.Delay - Use it from tests
src/Components/Server/src/Circuits/RevalidatingServerAuthenticationStateProvider.cs
Outdated
Show resolved
Hide resolved
SteveSandersonMS
force-pushed
the
stevesa/revalidating-auth-improvement
branch
from
a05a141
to
29fc528
Compare
ghost
deleted the
|
Merged because I think the CR feedback is adequately addressed, but @rynowak please let me know if you disagree. |
This resolves the problem with identity template's authentication state provider. Previously it was getting the user info from
HttpContext.User, and it was revalidating viaSignInManager. Neither of these are actually safe to use because in Azure SignalR service, there's noHttpContext.Since it turns out that we can't use
SignInManager, we need to do as @rynowak suggested and use something "more granuar". The more granular thing isUserManager, since that provides access to the security stamp in the underlying store, without being coupled toHttpContext, at least as far as I can tell.UserManagerdoes take anIServiceProvider, but doesn't refer to the concept of "HTTP" anywhere. A developer could configure their ownUserManagerthat does useIHttpContextAccessor, but that would just fail to work on Azure SignalR (and since the error handling is so conservative, force the user to be logged out).The drawback to using
UserManagerdirectly is that we have to duplicate some (~10 lines) of code fromSignInManagerthat does the validation of security stamps. I felt that this pushed the complexity of the user code in the project template a bit too far, so to mitigate it, I've:RevalidatingServerAuthenticationStateProvider. Awfully long name, I know, but it's for clarity. This also means we can have a reasonable set of unit tests for that stuff. This base class remains decoupled from Identity, and would be a sensible starting point for any server-based mechanism for periodic revalidation of authentication state.SignInManager's very conciseValidateSecurityStampAsynclogic into something functionally identical but more explicit and readable. At this point I'm now satisfied it's understandable enough that it's not dangerous to have it in user code.To validate this actually behaves correctly on Azure SignalR, I'll need to wait to get an updated SDK build that includes the changes in this PR as well as some of the more recent API review PR changes, since the project template won't build otherwise. We have a lot of time before P9 lockdown so that should not be a blocker.