Skip to content

User Claims missing from Identity Endpoints breaks Authorization #52142

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

Closed
1 task done
augustevn opened this issue Nov 16, 2023 · 10 comments
Closed
1 task done

User Claims missing from Identity Endpoints breaks Authorization #52142

augustevn opened this issue Nov 16, 2023 · 10 comments
Labels
area-identity Includes: Identity and providers enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-identity-apis Pillar: Dev Experience

Comments

@augustevn
Copy link

augustevn commented Nov 16, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

I did some digging, it seems like the user Claims are no longer returned from /manage/info from the .MapIdentityApi<User>().

My custom authorization was depending on getting those claims.
This change broke my app, worked in RC2 not in RTM. Or is this intentional?

IdentityApiEndpointRouteBuilderExtensions.cs, line 455:


    private static async Task<InfoResponse> CreateInfoResponseAsync<TUser>(TUser user, UserManager<TUser> userManager)
        where TUser : class
    {
        return new()
        {
            Email = await userManager.GetEmailAsync(user) ?? throw new NotSupportedException("Users must have an email."),
            IsEmailConfirmed = await userManager.IsEmailConfirmedAsync(user),
        };
    }

Expected Behavior

Return the user's Claims on authenticated GET call to /manage/info on the Identity Endpoints.

Steps To Reproduce

Upgrade from .NET 8 RC2 to .NET 8 RTM, implement the .MapIdentityApi<User>(), spot the 7 differences.

Exceptions (if any)

/

.NET Version

8.0.100

Anything else?

Possibly related to Blazor Identity UI issues: #52063
Of which it likely affects authorization.

@Tratcher Tratcher added the area-identity Includes: Identity and providers label Nov 16, 2023
@Tratcher
Copy link
Member

Yes, this was by design: #51177

@augustevn
Copy link
Author

augustevn commented Nov 16, 2023

@Tratcher How do you suggest Identity API consumers to handle authorization than, if they can't access the claims? For stateless bearer token auth. Or can Blazor (WASM)'s AuthenticationStateProvider decode that from the token somehow? Which would be a worse practice considering the implictions of that.

@augustevn augustevn reopened this Nov 16, 2023
@Tratcher
Copy link
Member

Authorization must always be done on the server side (where you have access to the Claims in the User). Anything done on the client is just for convenience. You can also expose your own endpoints for the API to check for specific permissions.

@augustevn
Copy link
Author

augustevn commented Nov 17, 2023

@halter73 @MackinnonBuck @mkArtakMSFT @Tratcher
Let me try again, I think the Blazor WASM way of informing our AuthenticationStateProvider of our claims is broken. At least the seamless integration of that with the Identity Endpoints.

The action of removing the claims from the GET /manage/info response, now broke this. I think that was still a better (authenticated) practice than having to verify and decode the bearer token to get the claims? Since that implies worse practices.

Or ... implementing our own custom endpoint to expose the claims... What did you win?

Take the below CustomAuthenticationStateProvider:

public class CustomAuthenticationStateProvider : AuthenticationStateProvider
{
    public override Task<AuthenticationState> GetAuthenticationStateAsync()
    {
        var emptyAuthState = ConstructEmptyAuthState();
        return Task.FromResult(emptyAuthState);
    }

    public void Authenticate(UserInfoResponse currentUser)
    {
        var identity = new ClaimsIdentity(
            currentUser.Claims.Select(c => new Claim(c.Key, c.Value.ToString())), // No longer possible.
            "auth");
        
        var user = new ClaimsPrincipal(identity);
        var authState = new AuthenticationState(user);
        
        NotifyAuthenticationStateChanged(Task.FromResult(authState));
    }

    public void UnsetAuthenticationState()
    {
        var emptyAuthState = ConstructEmptyAuthState();
        
        NotifyAuthenticationStateChanged(Task.FromResult(emptyAuthState));
    }

    private static AuthenticationState ConstructEmptyAuthState()
    {
        var identity = new ClaimsIdentity();
        var user = new ClaimsPrincipal(identity);
        var authState = new AuthenticationState(user);

        return authState;
    }
}

I'm having doubts on how you test and validate these entire authentication flows / scenarios from front to back.
I think it's time to pull your heads out of your back-ends.


While you're at it to make this flow viable:

  • It's lacking customizability due to DTO usage on /register, /forgotPassword,
    • Prevents passing extra user data to store in database.
    • Prevents passing extra data like a proper redirectUrl not going to the API.
    • How to validate extra passed data?

I don't understand why it would be a good idea to lead the user to your API using the confirmation link in a SPA + API setup that implements these Identity Endpoints.

I agree that I can build custom endpoints to do all of the above but then I might as well build the other endpoints myself...
I appreciate the efforts nonetheless. Could be a cool / time saving feature.

@augustevn augustevn reopened this Nov 17, 2023
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Nov 21, 2023
@mkArtakMSFT mkArtakMSFT added this to the Planning: WebUI milestone Nov 21, 2023
@julioct
Copy link

julioct commented Dec 17, 2023

I also stumbled into this today. It didn't break my app, since I was just trying to integrate my Blazor WASM app with the new identity endpoints as described in this official doc.

However, after completing the work, I realized the identity endpoints are not useful other than to register and authenticate a user.

And, the first thing you want to do after authenticating the user is check which permissions he has to decide how much to allow him to do on both the server and client side.

Without the claims, you can't tell the most basic thing: is he an admin? Can he modify things on my site?

I understand that this is not OIDC, so I would never be able to set scopes, which is the next thing I would want. But I must be able to at least check basic claims like the role the user belongs to.

To unblock, I implemented my own Identity endpoint group by extending the built-in ones:

public static class IdentityEndpoints
{
    public static RouteGroupBuilder MapIdentityEndpoints(this IEndpointRouteBuilder routes)
    {
        var group = routes.MapGroup("/identity")
                          .WithTags("Identity");

        group.MapIdentityApi<GameStoreUser>();

        group.MapPost("/logout", async (ClaimsPrincipal user, SignInManager<GameStoreUser> signInManager) =>
        {
            await signInManager.SignOutAsync();
            return TypedResults.Ok();
        });

        group.MapGet("/manage/infoWithClaims", async Task<Results<Ok<InfoWithClaimsResponse>, ValidationProblem, NotFound>>
            (ClaimsPrincipal claimsPrincipal, UserManager<GameStoreUser> userManager) =>
        {
            if (await userManager.GetUserAsync(claimsPrincipal) is not { } user)
            {
                return TypedResults.NotFound();
            }

            var email = await userManager.GetEmailAsync(user) ?? throw new NotSupportedException("Users must have an email.");

            var response = new InfoWithClaimsResponse(
                email,
                await userManager.IsEmailConfirmedAsync(user),
                claimsPrincipal.Claims.ToDictionary(c => c.Type, c => c.Value));

            return TypedResults.Ok(response);
        });

        return group;
    }
}

Could someone elaborate on the security concerns on returning the claims?

I'm also confused because I chose ASP.NET Core Identity based on this recommendation. It says there:

For apps that don't share logins with other apps, the simplest way to quickly secure an app is to use the built-in ASP.NET Core Identity provider. Otherwise, an OIDC server provided by a third-party identity management solution is needed.

I have my own Blazor WASM app that talks to my own ASP.NET Core API, and nothing else.

What's the right way to make the claims a key part of this scenario?

@augustevn
Copy link
Author

augustevn commented Dec 31, 2023

I'm not in a position to answer security best practices questions, I assume exposing that "SecurityStamp" wasn't ideal. If I'm not mistaking, the Azure AD approach also exposes claims.
I'm open for a better approach with or without recrafting the Blazor Identity components that expect those claims for the roles etc.

By now, I also found out that there is no "token-rotation" after refresh token use which means any non-expired refresh token could still be re-used for a week+-.

However, they stated this to be for simple scenario's for devices that can't handle cookies . And that is is not meant to be(come) a token server. I had hopes for this to be(come) the alternative or the preferred way over a custom JWT implementation.

@julioct Interesting approach, do you prefer checking for an empty ClaimsPrincipal over authorizing the endpoint?

    private static void MapIdentityEndpoints(this IEndpointRouteBuilder routeBuilder)
    {
        var identityGroup = routeBuilder
            .MapGroup(ApiRoutes.Auth)
            .WithTags(nameof(ApiRoutes.Auth));
            
        identityGroup.MapIdentityApi<User>();

        identityGroup.MapGet("/user-info", (ClaimsPrincipal user) =>
        {
            int.TryParse(user.FindFirstValue(CustomClaimTypes.Permissions), out int permissions);
            
            return Results.Ok(new UserInfoResponse
            {
                Id = user.FindFirstValue(ClaimTypes.NameIdentifier) ?? string.Empty,
                Email = user.FindFirstValue(ClaimTypes.Email) ?? string.Empty,
                Permissions = permissions,
                Roles = user.FindAll(ClaimTypes.Role).Select(claim => claim.Value),
                Claims = user.Claims.ToDictionary(claim => claim.Type, claim => claim.Value)
            });
        }).RequireAuthorization();
    }

@julioct
Copy link

julioct commented Dec 31, 2023

I'm not in a position to answer security best practices questions, I assume exposing that "SecurityStamp" wasn't ideal. If I'm not mistaking, the Azure AD approach also exposes claims. I'm open for a better approach with or without recrafting the Blazor Identity components that expect those claims for the roles etc.

By now, I also found out that there is no "token-rotation" after refresh token use which means any non-expired refresh token could still be re-used for a week+-.

However, they stated this to be for simple scenario's for devices that can't handle cookies . And that is is not meant to be(come) a token server. I had hopes for this to be(come) the alternative or the preferred way over a custom JWT implementation.

@julioct Interesting approach, do you prefer checking for an empty ClaimsPrincipal over authorizing the endpoint?

    private static void MapIdentityEndpoints(this IEndpointRouteBuilder routeBuilder)
    {
        var identityGroup = routeBuilder
            .MapGroup(ApiRoutes.Auth)
            .WithTags(nameof(ApiRoutes.Auth));
            
        identityGroup.MapIdentityApi<User>();

        identityGroup.MapGet("/user-info", (ClaimsPrincipal user) =>
        {
            int.TryParse(user.FindFirstValue(CustomClaimTypes.Permissions), out int permissions);
            
            return Results.Ok(new UserInfoResponse
            {
                Id = user.FindFirstValue(ClaimTypes.NameIdentifier) ?? string.Empty,
                Email = user.FindFirstValue(ClaimTypes.Email) ?? string.Empty,
                Permissions = permissions,
                Roles = user.FindAll(ClaimTypes.Role).Select(claim => claim.Value),
                Claims = user.Claims.ToDictionary(claim => claim.Type, claim => claim.Value)
            });
        }).RequireAuthorization();
    }

Ohh I just copied the implementation I found in the ASP.NET Core repo just before they removed the Claims property from the /manage/info endpoint.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@lcetinapta
Copy link

I agree to this, there is no way of getting my user claims from the api endpoints anymore.

@carlfranklin
Copy link

I've created a stopgap solution: a replacement for AuthorizeView that supports Roles in .NET 8 Blazor WebAssembly apps. All the requirements are laid out in the repo, which can be found at https://github.com/carlfranklin/BlazorAuthorizeRoleView

Comments welcome.

@mkArtakMSFT mkArtakMSFT removed this from the .NET 9 Planning milestone Sep 17, 2024
@mkArtakMSFT mkArtakMSFT added this to the .NET 10 Planning milestone Sep 17, 2024
@mikekistler
Copy link
Contributor

We don't plan to add back the user claims endpoint but we are considering providing scaffolding support to provide equivalent capabitlty #58959.

@mikekistler mikekistler closed this as not planned Won't fix, can't repro, duplicate, stale Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-identity Includes: Identity and providers enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-identity-apis Pillar: Dev Experience
Projects
None yet
Development

No branches or pull requests

8 participants