Adding user to role is not working as expected

[Avareto]

Adding a user to a role does not create an entry for AspNetUserRoles.

[brockallen]

That's by design. It adds a role claim to the user. The ASP.NET Identity special treatment of roles is unnecessary.

[Avareto]

But how than handle [Authorize(Roles="Admin")] attribute for MVC/WebApi controller? Does it check for role claims as well?

[brockallen]

This is up to how you configure your ClaimsIdentity class in your code.

[gavinbarron]

What about the scenario where one might want to find users based on Role? Without the entries in the AspNetUserRoles table you can't find users in a specific role using the RoleManager.FindByNameAsync. The only option in this case is to iterate over every user and perform a claim check. (Unless I'm missing something?)

[brockallen]

And what about finding users by email domain, and finding users by airline status level. These are all application level requirements. If you need those specialized queries, then sure -- you do need some sort of additional lookup mechanism. It's just unnecessary to build the roles mechanism the way the ASP.NET team did with ASP.NET Identity. If it does 100% of what you need, then great. But we're not catering to that customization.

Now, having said all of that -- I'm happy to add a flag or virtual or something that allows merging of the two. If you guys can think of a nice generalized approach then I'm all ears.

[gavinbarron]

I'll take a climb into the code that sets roles as claims and wrap my head around it and then get back to you. Agreed this is an application specific requirement but I think Identity manger should optionally support writing to the AspNetUserRoles table in addition to the the AspNetUserClaims table

On 21 September 2015 at 11:30, Brock Allen notifications@github.com wrote:

And what about finding users by email domain, and finding users by airline status level. These are all application level requirements. If you need those specialized queries, then sure -- you do need some sort of additional lookup mechanism. It's just unnecessary to build the roles mechanism the way the ASP.NET team did with ASP.NET Identity. If it does 100% of what you need, then great. But we're not catering to that customization.

Now, having said all of that -- I'm happy to add a flag or virtual or something that allows merging of the two. If you guys can think of a nice generalized approach then I'm all ears.

— Reply to this email directly or view it on GitHub https://github.com/IdentityManager/IdentityManager.AspNetIdentity/issues/21#issuecomment-141845010 .

[brockallen]

Ok, keep me posted.

[Avareto]

I think the source of confusion on my side was caused by the expectation that the IdentityManager is 100% compatible to the original AspNet Identiy approach. But I think your implementation of the IdentityManager is not because the AspNet Identity role management api is not working any more. AspNet Identity is a framework, as IdentityManager is, which developer can use without thinking about the implementation. Here is my approach to make it compatible again. It’s probably not the best solution but its working for me.

public class ApplicationIdentityManagerService : 
AspNetIdentityManagerService<ApplicationUser, string, Role, string>
{
    public ApplicationIdentityManagerService(ApplicationUserManager userMgr, ApplicationRoleManager roleMgr)
        : base(userMgr, roleMgr)
    {
    }
    public override async Task<IdentityManagerResult> AddUserClaimAsync(string subject, string type, string value)
    {
        var result = await base.AddUserClaimAsync(subject, type, value);
        if (!result.IsSuccess)
        {
            return new IdentityManagerResult<CreateResult>(result.Errors.ToArray());
        }
        if (type == RoleClaimType)
        {
            var key = ConvertUserSubjectToKey(subject);
            var status = await this.userManager.AddToRoleAsync(key, value);
            if (!status.Succeeded)
            {
                return new IdentityManagerResult<CreateResult>(result.Errors.ToArray());
            }
        }

        return IdentityManagerResult.Success;
    }

    public override async Task<IdentityManagerResult> RemoveUserClaimAsync(string subject, string type, string value)
    {
        var result = await base.RemoveUserClaimAsync(subject, type, value);
        if (!result.IsSuccess)
        {
            return new IdentityManagerResult<CreateResult>(result.Errors.ToArray());
        }
        if (type == RoleClaimType)
        {
            var key = ConvertUserSubjectToKey(subject);
            var status = await this.userManager.RemoveFromRoleAsync(key, value);
            if (!status.Succeeded)
            {
                return new IdentityManagerResult<CreateResult>(result.Errors.ToArray());
            }
        }

        return IdentityManagerResult.Success;
    }
}

[brockallen]

I don't understand or follow the question....

[brockallen]

This issue is the middle one.

[bartvanderwal]

I was under the same assumption @brockallen! That IdentityManager was compatible with the AspNet Identity approach.

E.g. that 'we' could still use familiar and well documented stuff such as [Authorize(Roles="Admin")] as annotation on an action method and User.IsInRole("Admin)) in logic.

But luckily there is this issue already. Your two overrides methods worked great for me @Avareto. They sync up the AspNetUserRoles table with the assigned claims (AspNetUserClaims).

Note to others who adopt this:

• You have to redo any roles you assigned in the IdentityManager before adding this.
• You get some (neat) red error messages when deleting roles that only had a claim and not a UserRole yet, but after that everything works fine.

[NikolayTimofeev]

I understand @brockallen point of view that it is unnecessary to build the roles mechanism the way the ASP.NET team did with ASP.NET Identity. But unfortunately they did things the way they did them. And if someone is looking for a way to use it as it is you can change code that looks like this

@if(Request.IsAuthenticated && User.IsInRole("admin") )
{
    <p>
        @Html.ActionLink("Create New", "Create")
    </p>
}

to this

@if(Request.IsAuthenticated && ((System.Security.Claims.ClaimsIdentity)User.Identity).HasClaim("role", "admin"))
{
    <p>
        @Html.ActionLink("Create New", "Create")
    </p>
}

and that should work as before.

Although kudos to @Avareto quick fix on the ApplicationIdentityManagerService class that manages to make this whole thing work with the usual ASP.NET Identity approach.

[jhoerr]

@Avareto Thanks for the guidance and sample code. With your implementation it seems that both a AspNetUserRoles record and a AspNetUserClaims record are being added. Here's an implementation of ApplicationIdentityManagerService that converts the role claims to/from AspNetUserRoles and avoids adding the role claim to the database. Note the override of GetUserAsync that adds corresponding role claims to the UserDetail.Claims -- this allows IdentityManager to think of everything as claims while preserving Asp.Net Identity's standard treatment of roles.

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

    public override async Task<IdentityManagerResult<UserDetail>> GetUserAsync(string subject)
    {
        var user = await base.GetUserAsync(subject);
        var roles = await userManager.GetRolesAsync(subject);
        var claims = user.Result.Claims.ToList();
        claims.AddRange(roles.Select(role => new ClaimValue() { Type = RoleClaimType, Value = role }));
        user.Result.Claims = claims;
        return user;
    }

    public override async Task<IdentityManagerResult> AddUserClaimAsync(string subject, string type, string value)
    {
        if (type == RoleClaimType)
        {
            var status = await userManager.AddToRoleAsync(ConvertUserSubjectToKey(subject), value);
            return status.Succeeded 
                ? IdentityManagerResult.Success 
                : new IdentityManagerResult<CreateResult>(status.Errors.ToArray());
        }

        return await base.AddUserClaimAsync(subject, type, value);
    }

    public override async Task<IdentityManagerResult> RemoveUserClaimAsync(string subject, string type, string value)
    {
        if (type == RoleClaimType)
        {
            var status = await userManager.RemoveFromRoleAsync(ConvertUserSubjectToKey(subject), value);
            return status.Succeeded
                ? IdentityManagerResult.Success
                : new IdentityManagerResult<CreateResult>(status.Errors.ToArray());
        }

        return await base.RemoveUserClaimAsync(subject, type, value);
    }
}

[brockallen]

Relevant blog post for this thread: https://leastprivilege.com/2016/08/21/why-does-my-authorize-attribute-not-work/

[jhoerr]

@brockallen I agree with you re: Asp.Net Identity's funky/legacy/snowflake treatment of roles. And I agree that claims checks are in general the better way to approach things.

I can also see how it would be reasonable for people to assume that if IdentityServer/IdentityManager are billed as having first-class support of Asp.Net Identity then they also support its treatment of roles, warts and all. I also think that it might send a mixed message to have given Role management first-class treatment in the IDM UI/API and to have made the User/Role/Claim management appear to align so precisely with ASP.Net Identity's model when it ultimately breaks that model in a fundamental way. To put it another way, if we shouldn't be using roles, then IDM maybe ought not to present itself as supporting them.

I say all this with a ton of respect for the work that y'all have done for the community and with great appreciation for the value of these libraries and the time/effort/pain they've saved me. As with encryption it seems like the prevailing message about identity is, "use mature libraries and for heaven's sake don't roll your own implementation because it is way harder than it looks and you will totally blow it." And I buy that message! I think this particular issue is just an illustration of a pain point for folks (like me) who are trying to do the right thing while also trying to figure out exactly what the 'right thing' is. :)

[brockallen]

I'd prefer to help educate people on how things work, and then they can decide for themselves which approach to take. I really dislike the typical Microsoft mentality of hiding the details and obscuring how things work with the goal of making it easier. It eventually becomes magic, and that's an awful foundation for your security architecture. I'm all for abstractions and helper libraries, but it does not excuse the need to understand how they work.

As for IDM specifically, it supports the role manager's list of roles as a convenience for populating the UI, as that's fundamentally all the role manager is good for. It's also configurable such that you can disable it. So in short, yes, I hear what you're saying but my opinion on what the ASP.NET Identity's features really are might be different than others.

[jhoerr]

Agreed all around, particularly with regard to education and magic.

As for IDM specifically, it supports the role manager's list of roles as a convenience for populating the UI, as that's fundamentally all the role manager is good for.

That makes sense now. This feels like a documentation / expectations management issue. I'm not aware of anything in the IDM or docs that make it explictly clear how roles are used (and not used), and as a result I -- and I imagine the folks upthread -- expected it to work in the normal Asp.Net way. It might be reasonable to spell this out on the IDM page where roles are assigned to users. Would you consider a PR to that effect?

[brockallen]

Fair enough. The above blog post was necessary because people are still confused even today despite claims being added in .NET framework 4.5 in 2012.

[Noor1978]

i need full authentication for MVC Core 2.x please ?

[Noor1978]

i m 0 and i love MVC Core 2.x please any link of pdf or video i m 0 and want to be developer