Checking for user permissions

The idea here is we have Roles, Permissions, and a table called PermissionRoles that connect the two. So a Permissions can be in many Roles, and many Permissions can have the same Role. So what the following code does is:

• Get all the permissions required for a user to authenticate, by

ActionName

• Loop through each of these permissions

• Get the list PermissionsRole records that have one of those Permission objects

• Get the list of Roles from the list of PermissionRoles

• Loop through that list of Roles

• Check if a user is in any of those roles

This is the code, I feel like there may be a more efficient way to write this. It is implied that I have instantiated a UserManager and Database Context.

string[] permissions = Permissions.Split(',').ToArray();

IEnumerable perms = permissions.Intersect(db.Permissions.Select(p => p.ActionName));
List roles = new List();

if (perms.Count() > 0)
{
    foreach (var item in perms)
    {
        var currentUserId = httpContext.User.Identity.GetUserId();

        var permissionsRoles = db.PermissionRoles.Where(p => p.Permission.ActionName == item && p.CompanyId == companyId).ToList();
        var existingRoles = dbu.Roles.Select(x => x.Id).Intersect(permissionsRoles.Select(x => x.RoleId)).ToList();
        foreach (var role in existingRoles)
        {
            ApplicationRole thisRole = dbu.Roles.Find(role);
            if (userManager.IsInRole(currentUserId, thisRole.Name))
            {
                return true;
            }
        }
    }
}
return false;

I would say you do too many things at once. The important thing here is this check:

if (userManager.IsInRole(currentUserId, thisRole.Name))
{
    return true;
}

This needs a list of role-names to iterate over:

foreach (var roleName in roleNames)
{
    if (userManager.IsInRole(userId, roleName))
    {
        return true;
    }
}

return false;

The list of role-names seems to be dependent of some given permissions and a company:

var givenPermissions = GetGivenPermissions();
var roleNames = GetRoleNames(companyId, givenPermissions);

The first is easy enough to extract from the given code:

IEnumerable GetGivenPermissions()
{
    return Permissions.Split(',').ToArray();
}

The other one is trickier, but I think I got it right:

IEnumerable GetRoleNames(string companyId, IEnumerable givenPermissions)
{
    var existingPermissions = db.Permissions.Select(p => p.ActionName);
    var relevantPermissions = givenPermissions.Intersect(existingPermissions);

    foreach (var permission in relevantPermissions)
    {
        var allRoles = db.Roles.Select(x => x.Id);
        var relevantPermissionRoles = db.PermissionRoles
                                        .Where(p => p.Permission.ActionName == permission && p.CompanyId == companyId)
                                        .Select(x => x.RoleId)
                                        .ToList();

        var relevantRoles = allRoles.Intersect(relevantPermissionRoles).ToList();

        foreach (var role in relevantRoles)
        {
            var thisRole = db.Roles.Find(role);
            var name = thisRole.Name;
            yield return name;
        }
    }
}

This is one nasty query:

• It's a good mix of things querying the database and things filtered in memory. - It's hard to figure out what it actually do, and what the requirements for the roles really are.

• It's also a mix of code operating on objects and primitive values, which is probably why db.Roles is queried twice.

What I would like to see in here is something that:

• First retrieve necessary data from the database, once.

• Filter it against in-memory data.

• Return the resulting role names.

StackExchange Code Review Q#92680, answer score: 2