trjhtr

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)
 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?

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)
 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() 

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);