IdentityManager / IdentityManager.AspNetIdentity

ASP.NET Identity support for Thinktecture IdentityManager
Apache License 2.0
60 stars 51 forks source link

Support for conventional MVC roles? #3

Closed JayVDZ closed 9 years ago

JayVDZ commented 9 years ago

I'm not sure if this issue is best placed here or in the IdentityManager project, but it's aspnet specific...

Can we get support for or a guide on how to manage conventional ASP.NET MVC roles with AspNetIdentity? I'm using IdentityManager.AspNetIdentity to manage the users in my MVC app and the roles created are not recognised by MVC.

Thanks!

brockallen commented 9 years ago

There's already a RoleClaimType property on the RoleMetadata, but it certainly could be easier to configure. I'll see what I can do.

JayVDZ commented 9 years ago

Thanks, I'm using the nuget package and don't see what to change. Any help appreciated.

brockallen commented 9 years ago

It's a property on the RoleMetadata returned from the GetMetadataAsync on the IdentityManagerService-derived class. IOW, it's cumbersome for you to set it :) but it is possible -- you just have to derive and override and change the property.

chrishasz commented 9 years ago

I ran into this problem myself - the root cause is that both AddUserClaimAsync and RemoveUserClaimAsync in AspNetIdentityManagerService.cs only add and remove claims, even if the claim type is "role". I have a local patch that I used to work around this.

Brock - let me know if you are interested in having me commit this update, otherwise here is the full updated source for AddUserClaimAsync and RemoveUserClaimAsync:

public async Task<IdentityManagerResult> AddUserClaimAsync(string subject, string type, string value)
{
    TUserKey key = ConvertUserSubjectToKey(subject);
    var user = await this.userManager.FindByIdAsync(key);
    if (user == null)
    {
        return new IdentityManagerResult("Invalid subject");
    }

    if (type.Equals("role"))
    {
        var existingRoles = await userManager.GetRolesAsync(key);
        if (!existingRoles.Any(x => x.Equals(value)))
        {
            var roleResult = await this.userManager.AddToRoleAsync(key, value);
            if (!roleResult.Succeeded)
            {
                return new IdentityManagerResult<CreateResult>(roleResult.Errors.ToArray());
            }
        }
    }
    var existingClaims = await userManager.GetClaimsAsync(key);
    if (!existingClaims.Any(x => x.Type == type && x.Value == value))
    {
        var result = await this.userManager.AddClaimAsync(key, new System.Security.Claims.Claim(type, value));
        if (!result.Succeeded)
        {
            return new IdentityManagerResult<CreateResult>(result.Errors.ToArray());
        }
    }

    return IdentityManagerResult.Success;
}

public async Task<IdentityManagerResult> RemoveUserClaimAsync(string subject, string type, string value)
{
    TUserKey key = ConvertUserSubjectToKey(subject);
    var user = await this.userManager.FindByIdAsync(key);
    if (user == null)
    {
        return new IdentityManagerResult("Invalid subject");
    }

    if (type.Equals("role"))
    {
        var roleResult = await this.userManager.RemoveFromRoleAsync(key, value);
        if (!roleResult.Succeeded)
        {
            return new IdentityManagerResult<CreateResult>(roleResult.Errors.ToArray());
        }
    }
    var claimResult = await this.userManager.RemoveClaimAsync(key, new System.Security.Claims.Claim(type, value));
    if (!claimResult.Succeeded)
    {
        return new IdentityManagerResult<CreateResult>(claimResult.Errors.ToArray());
    }
    return IdentityManagerResult.Success;
}
brockallen commented 9 years ago

Oh I see what you're asking for. I misunderstood what you were asking for @Amethi. I have no short term plans to implement it.

Personally I have a huge distaste for the design to special case roles in ASP.NET Identity. The team could not explain why it was really needed (since a role is just a claim).

chrishasz commented 9 years ago

I agree that the overall role implementation in asp.net Identity is pretty ugly, unfortunately too many existing core and 3rd party tools rely on .IsInRole() to fully discount it yet.

brockallen commented 9 years ago

You mean User.IsInRole from MVC or Web API? These work just fine without putting roles in the "roles" part of ASP.NET Identity. If you add roles to the claims collection, then when the user is authenticated those role claims are perfectly valid for the IsInRole checks. This is why I have such a distaste for their design -- the special casing of roles is redundant and superfluous.

danjohnso commented 9 years ago

Based on this, how would I add roles to a user? The roles in AspNetUserRoles seem to be completely ignored. Do I need to create an override in my role manager to add them as user claims instead of traditional roles? My use case is when I create users through code I normally use UserManager.AddToRoleAsync to add the roles. Seems like that would be incorrect?

chrishasz commented 9 years ago

I haven't had a chance to validate this yet, but according to the .NET 4.5 documentation, if you issue a role claim using the correct role URI "http://schemas.microsoft.com/ws/2008/06/identity/claims/role", then IPrincipal.IsInRole() will behave as desired.

Documentation: http://msdn.microsoft.com/en-us/library/hh545448.aspx

Notes: Obviously, you would want to update your role attribution code to no longer use "AddToRole[Async]", but rather use AddClaim(). Additionally, you'll likely need to update all of your assemblies to reference .NET 4.5 builds and test appropriately.

Finally: @brockallen - would it make sense to update Identitymanager to use this role URI?

p.s. thanks for the motivation to actually do the research on this.

JayVDZ commented 9 years ago

It works! Thanks @chrishasz! User.IsInRole("blah") and [Authorize(Roles = "blah")] work as expected.

danjohnso commented 9 years ago

Ah, I figured it out. I was incorrectly using my custom user service which is why I did not find the roles. You can use the normal aspnetidentiy roles, you just have to add them as claims (in my case as In the custom user service though I believe the OOB user service does the same thing).

@chrishasz you don't need to use the Microsoft claims type for this, the Identity Server claim type works fine with InRole. Edit: One caveat, if you take a look at the MVC Owin Hybrid Sample, in the AuthenticationCodeReceived action of the OpenID settings, the last line where the authentication tickets gets resigned needs to be changed to this: n.AuthenticationTicket = new AuthenticationTicket(new ClaimsIdentity(claims.Distinct(new ClaimComparer()), n.AuthenticationTicket.Identity.AuthenticationType, ISConstants.ClaimTypes.Name, ISConstants.ClaimTypes.Role), n.AuthenticationTicket.Properties);

If you don't set the claim types to the identity server claim types, User.Identity.Name will be null and the role checks will fail. Changing the name and role claim types like above will fix both of those problems.

fretje commented 9 years ago

@danda1man You're talking about IdentityServer, but this issue is about IdentityManager... I have the same problem with IdentityManager, and IsInRole or Authorize(Roles="...") definitely doesn't work with the simple "role" claimtype. I have a workaround now where I add the roles again after sign-in (in "GenerateUserIdentityAsync"), using the Microsoft ClaimTypes.Role, and then everything works fine... Just wondering if this is the right approach.

brockallen commented 9 years ago

Ok, took me forever, but I just added a RoleClaimType to the AspNetIdentityManagerService so you can pick what claim types you want for roles.

I did not change anything to work with the AspID role plumbing.

DarioN1 commented 8 years ago

Brock, this works fine for me, I have modify the code in this way:

public class ApplicationIdentityManagerService : 
        AspNetIdentityManagerService<ApplicationUser, string, IdentityRole, string>
    {
        public ApplicationIdentityManagerService(ApplicationUserManager userMgr, ApplicationRoleManager roleMgr)
            : base(userMgr, roleMgr)
        {
            RoleClaimType = ClaimTypes.Role; 
        }

    }

And now IdentityManager fill the Claim table with the Microsoft Schema ("http://schemas.microsoft.com/ws/2008/06/identity/claims/role")

The checks: [Authorize(Roles = "ASPNET_IDENTITYMANAGER")]

and

User.IsInRole("ADMINISTRATOR") now works properly.

I have a question now:

Checking the AspNet sql tables I notice that AspNetUserRoles is empty and that the AspNetUserClaims table is filled properly...

The application works but I don't know if this can cause some problem...

Thanks to support