5
\$\begingroup\$

In my case, I needed to give roles to users, but to specific departments, and I did not want to implement a new authorization mechanism which will require me to access the db everytime I want to check if the user is authorized for a specific role to a specific department.

So I thought I will just create an extension method IsInRole with two parameters (role, departmentId). Here's the exact code:

public static bool IsInRole(this IPrincipal principal, string role, int departmentId)
{
    var ident = principal?.Identity;
    if (ident == null)
        return false;

    if (departmentId <= 0)
        return false;

    var roleDeparments = ((ClaimsIdentity)ident).FindFirst($"{role} departments")?.Value;
    if (string.IsNullOrWhiteSpace(roleDeparments))
        return false;

    var inInRole = roleDeparments
        .Split(',')
        .Any(d => d == departmentId.ToString() || d == "all");

    return inInRole;
}

The user roles are saved in the db and are fetched once upon logging in, if the departmentId is set to NULL then it means this role is general to all departments. The Identity object is created at login time using the following method:

private ClaimsIdentity CreateIdentity(User user)
{
    var identity = new ClaimsIdentity(MyAuthentication.ApplicationCookie, ClaimsIdentity.DefaultNameClaimType, ClaimsIdentity.DefaultRoleClaimType);
    identity.AddClaim(new Claim("http://schemas.microsoft.com/accesscontrolservice/2010/07/claims/identityprovider", "Active Directory"));
    identity.AddClaim(new Claim(ClaimTypes.Name, user.Id));
    identity.AddClaim(new Claim(ClaimTypes.GivenName, user.Name));
    identity.AddClaim(new Claim("DepartmentId", user.DepartmentId.ToString()));

    var roles = user.UserRoles
        .GroupBy(r => r.Role);

    foreach (var role in roles)
    {
        var departments = role
            .Select(r => r.DepartmentId)
            .Distinct()
            .ToList();

        bool isAll = departments.Any(d => d == null);

        string departmentIds = isAll
            ? "all"
            : string.Join(",", departments);

        identity.AddClaim(new Claim($"{role.Key.Name} departments", departmentIds));
    }

    return identity;
}

Now, I can simply do something like:

if (User.IsInRole("schedule viewer", 2))
{
   .....
}

Is this the right approach?

\$\endgroup\$
  • 1
    \$\begingroup\$ @πάνταῥεῖ not familiar with the culture of this SE site, do not know how do people ask here in code review, not sure why is the downvote, i read the rules and it says that I should post working code, which I did. However any suggestions on what is an acceptable closing question of the post should be? I think new users should be "taught" on the specific culture of the site, not downvoted with a smart comment :) just saying. thanks anyway \$\endgroup\$ – Him Feb 27 '18 at 22:26
  • 1
    \$\begingroup\$ "I actually did." Where is your [informed] badge then? \$\endgroup\$ – πάντα ῥεῖ Feb 27 '18 at 22:34
  • 1
    \$\begingroup\$ I read the on-topic page, I did not read the tour page since this is not my first SE site. I know how to use the site, all i need is the on-topic page of the specific site to get the "general culture" of that SE site. \$\endgroup\$ – Him Feb 27 '18 at 22:44
  • 1
    \$\begingroup\$ I see that there is a close vote, which I'm not totally on board with, but I agree that a slightly more concrete example of how/why you'd use this would be useful. \$\endgroup\$ – Dannnno Feb 27 '18 at 23:18
  • 2
    \$\begingroup\$ @πάνταῥεῖ This question is being discussed on Meta. \$\endgroup\$ – 200_success Feb 27 '18 at 23:23
4
\$\begingroup\$

IsInRole()

I don't like how the method parameters are validated. IMO the null-propagation operator shouldn't be used for everything because it reduces the readability. Having a null-propagation operator for "safely" "accessing" a property is OK if the object is the first on the right hand of an assignment like in checking for var ident = principal?.Identity; but nevertheless I personally would stick to a combined null check like so

if (principal == null || principal.Identity == null) { return false; }  

But why areyou checking this at the beginning of the method ? I would check simple types first like departmentId <= 0.

The null-propagation operator here

var roleDeparments = ((ClaimsIdentity)ident).FindFirst($"{role} departments")?.Value;  

could be missed at first glance making a maintainer of the code wonder what this is about, at least if he/she has not that good eyes like me. He/she can't grasp at first glance that you "safely" access the Value property here.

Can you spot the error here ?

var inInRole = roleDeparments
    .Split(',')
    .Any(d => d == departmentId.ToString() || d == "all");  

Aside from spelling error in inInRole you could store the result of departmentId.ToString() in a variable to check in the Any(), but maybe the compiler is smart enough to do this for you.

Omitting braces {} for single line if statemenst is your responsibility. I wouldn't do it. If I would do it then only like if (departmentId <= 0) return false;

Using abbreviations leads to less readable code. Why don't you rename ident to identity ?

Implementing the mentioned points leads to

public static bool IsInRole(this IPrincipal principal, string role, int departmentId)
{
    if (departmentId <= 0) { return false; }
    if (principal == null || principal.Identity == null) { return false; }  

    var identity = principal.Identity;

    var claim = ((ClaimsIdentity)identity).FindFirst($"{role} departments");
    if (claim == null) { return false; }

    var roleDeparments = claim.Value;
    if (string.IsNullOrWhiteSpace(roleDeparments)) { return false; }

    var isInRole = roleDeparments
        .Split(',')
        .Any(d => d == departmentId.ToString() || d == "all");

    return isInRole;
}

CreateIdentity()

This made me wonder

bool isAll = departments.Any(d => d == null);  

reading isAll and Any. After rereading your question I stumbled over

if the departmentId is set to NULL then it means this role is general to all departments

This is screaming for a comment because a maintainer of the code should know this as well. Together with a better name it would be clear for a maintainer what is happening here like so

// If one of the user.UserRoles has a DepartmentId which is null,
// the role is general for all departments
bool isGeneralForAllDepartments = departments.Any(d => d == null);  

string departmentIds = isGeneralForAllDepartments 
    ? "all"
    : string.Join(",", departments);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.