dotnet / AspNetCore.Docs

Documentation for ASP.NET Core
https://docs.microsoft.com/aspnet/core
Creative Commons Attribution 4.0 International
12.64k stars 25.28k forks source link

Bug in sample code? #30385

Closed tolubewaji closed 1 year ago

tolubewaji commented 1 year ago

[EDIT by guardrex to fix the code samples. @tolubewaji ... Use triple-backticks above and below code in GH comments for formatting. Add "csharp" to the opening triple-backticks for C#, "razor" for Razor, "javascript" for JS, and "css" for styles.]

using System.Security.Claims;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components.Authorization;

public class CustomAuthenticationStateProvider : AuthenticationStateProvider
{
    private AuthenticationState authenticationState;

    public CustomAuthenticationStateProvider(AuthenticationService service)
    {
        authenticationState = new AuthenticationState(service.CurrentUser);

        service.UserChanged += (newUser) =>
        {
            NotifyAuthenticationStateChanged(
                Task.FromResult(new AuthenticationState(newUser)));
        };
    }

    public override Task<AuthenticationState> GetAuthenticationStateAsync() =>
        Task.FromResult(authenticationState);
}

Seems like we should have the following in the code above instead:

service.UserChanged += (newUser) =>
{
    authenticationState = new AuthenticationState(newUser);

    NotifyAuthenticationStateChanged(
        Task.FromResult(new AuthenticationState(newUser)));
};

so that components calling GetAuthenticationStateAsync get the correct state per time.


Document Details

⚠ Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

guardrex commented 1 year ago

Hello @tolubewaji ... It's correct in its present form. The current user is always used to return GetAuthenticationStateAsync. You have the sequence of calls out of order in your suggestion. Retrace the logic of the calls. The code sets the current user first. UserChanged is triggered after the current user is set. UserChanged calls NotifyAuthenticationStateChanged, which to notifies consumers of the authentication state change to rerender.

tolubewaji commented 1 year ago

I would assume so as well but the example as-is didn't work in a try out I did. The current user identity was null after log in until I made the suggested changes.

guardrex commented 1 year ago

The current user identity was null after log in until I made the suggested changes.

I see ... how are you setting the current user at login? Are you following the full example?

Either way, I'll check the full example. It used to work! πŸ˜„

tolubewaji commented 1 year ago

πŸ˜„ I get!

Yes, I'm using the full example for the AuthenticationService + CustomAuthenticationStateProvider.

Just to be sure I'm not doing something wrong, this is what I did:

I noticed that if I use the code in the sample as is, the user gets authenticated but the Identity is null, I suspect GetAuthenticationStateAsync is returning the initial principal in this case. I tried the suggested edit and it worked. The sample with CustomAuthenticationStateProvider returning a hard-coded identity in GetAuthenticationStateAsync also works.

guardrex commented 1 year ago

I'll be back in a bit. I'm sidetracked on something else right now with Blazor Web Apps.

I do see what you mean. Yes, this could be a terrible error in the example. One that has been here for a long time without being reported. AFAIK, this came over from the product unit, but I did test the scenarios ... although it was years ago at this point.

I'll get back to this in a bit. I need to sort out my Blazor Web App (BWA) problem first. All of these scenarios must be validated with the new template, and this is a good time to test this out in a BWA ... if I can get it running.

guardrex commented 1 year ago

Ok ... I think I've figured one thing out. As part of the work for .NET 8 RC2, they're doing the work to make authn/z work in a Blazor Web App. I'm hitting this ...

Error: System.InvalidOperationException: Authorization requires a cascading parameter of type Task. Consider using CascadingAuthenticationState to supply this.

... but I did indeed do that, and it's still πŸ’₯ with that error.

Ok, so I'll switch over to a Blazor Server app under 7.0. I'll also see if I can research back to find out if this code example came in from the PU like this.

guardrex commented 1 year ago

Yes! It looks like you're correct. Sorry about my earlier remark on it. I was the one NOT tracing the logic correctly. I left it in the ctor in case the current user is set when the service is created ...

public CustomAuthenticationStateProvider(AuthenticationService service)
{
    authenticationState = new AuthenticationState(service.CurrentUser);

    service.UserChanged += (newUser) =>
    {
        authenticationState = new AuthenticationState(newUser);

        NotifyAuthenticationStateChanged(
            Task.FromResult(new AuthenticationState(newUser)));
    };
}

I'm going to see if I can research this back and find out when it came in and if it came to me like this. Normally, I don't receive bad code to publish from the product unit, but I don't think I dreamed this up in a πŸ¦– hallucination! But, I suppose it's possible.

guardrex commented 1 year ago

It looks like it came in with a large number of updates on https://github.com/dotnet/AspNetCore.Docs/pull/28400. I didn't write exactly where it came from.

Well ... let's just assume that I screwed it up then. πŸ™ˆ

Thanks for reporting this and bearing with me. I'll fix it right now.