JudahGabriel / RavenDB.Identity

RavenDB Identity provider for ASP.NET Core. Let RavenDB manage your users and logins.
https://www.nuget.org/packages/RavenDB.Identity/1.0.0
MIT License
61 stars 29 forks source link

Lowercase roles and JWT tokens #10

Closed JoshClose closed 5 years ago

JoshClose commented 5 years ago

I see in UserStore.AddToRoleAsync, the roleName is getting ToLower called on it and that is what is being stored in the users roles array.

I'm using Microsoft.AspNetCore.Authentication.JwtBearer for authentication. I create claims for the roles which are all lowercase. I have a role SiteAdmin that is stored as siteadmin under the user. The lowercase version is shipped off to the client. When the client makes requests, it sends that lowercase version to the server to authenticate.

I have controller actions that I'm decorating with Authorize(Roles = "SiteAdmin) because that is how I created it when doing AddToRoleAsync(user, "SiteAdmin"). This doesn't work though because the JWT token has the lowercase version in it.

Why are you doing a ToLower on the roleName? I did notice that it comes in normalized as an uppercase string, but curious if there is a specific reason you're lowering it. I also see that a role is created if it doesn't already exist, and that is using the lowercase name also. This seems a little odd especially if the user has extended IdentityRole and added some extra properties.

Since the roleName comes in normalized, do you see any issue with finding the role in the db first, and using the normal non-normalized name to store in IdentityUser.Roles instead so it matches IdentityRole.Name?

I believe all of this is only manifested because I'm using a JWT and it doesn't re-lookup the claims on an Authorize.

JudahGabriel commented 5 years ago

Hey Josh. Just got back from my travels, sorry for the late reply.

When I moved to MS Identity latest major version, I saw that the normalized name was being used through the framework. If I recall right, authorization wouldn't work if the role name differed by case.

I will re-validate that assumption today; if Identity framework doesn't care, I'll make it so that RavenDB.Identity doesn't care, and just uses whatever you pass in.

I'll comment here later with the results.

JoshClose commented 5 years ago

I think there needs to be consistency between User.Roles and Role.Name. I could query the database and get the role names before adding it to the JWT, but it seems like they should match since it's sort of a foreign key.

I could be completely missing something too. These are just my observations so far.

JudahGabriel commented 5 years ago

OK, now that I've dug into this more and re-read your comment, I see the problem.

The problem is:

  1. Create your own role (e.g. via RoleManager<IdentityUser>.CreateAsync(...)) with a case-varying role name (e.g. "SiteAdmin").
  2. When you call userManager.AddToRoleAsync(user, "SiteAdmin"), it stores the user with .Roles populated, but uses the .ToLowered() role name (e.g. { Roles = ["siteadmin"] }).
  3. This breaks things relying on the case name, like JWT.

The fix for this is, when we call userManager.AddToRoleAsync(...), we should lookup the real name of the role ("SiteAdmin") and use that, rather than the normalized name.

JudahGabriel commented 5 years ago

I've fixed this issue and pushed out a new version, RavenDB.Identity 6.0.6, which includes the fix.

If you want to see the fix in action, I've added an MVC sample with authorization that uses mixed-case role names (e.g. "Admin"). Running the sample will create the roles and users, and there will be consistency between role.Name and user.Roles; they'll both have preserved case of the role ("Admin").

JoshClose commented 5 years ago

Thanks. That looks great.