7

Trying to manage access to a web site I created some necessary entities enter image description here

The goal is use a custom permission attribute for some controller's action method of my MVC application. 

[Permissions(PermissionType.SomePermissionName, CrudType.CanDelete)]
public ActionResult SomeAction()
{
}

For this operation I have two enums

[Flags]
public enum CrudType
{
    CanCreate = 0x1,
    CanRead = 0x2,
    CanUpdate = 0x4,
    CanDelete = 0x8,
}

[Flags]
public enum PermissionType
{
   SomePermissionName = 0x1,
   //... 
}

Now I want the method below to check permissions

public static bool CanAccess(RolePermissions rp, CrudType crudType)
{
    var pInfo = rp.GetType().GetProperties();
    var res = pInfo.FirstOrDefault(x => x.Name == crudType.ToString());
    if(res != null)
    {
        return Convert.ToBoolean(res.GetValue(rp, null));
    }
    return false;
}

It works good but is it safe to use reflection here? Is it a good style?
One more question is about such piece of code

var permission = PermissionService.GetByName(permissionType.ToString());

Here I'm trying to get a permission object from a database using some named constant from the PermissionType enum.
In both cases the correct work depends on relationships between enums and some table fields or records. On other side I have a good mechanism of controlling logic (as it seems to me). Is that a good way?

Alex Kovanev
  • 1,858
  • 1
  • 16
  • 29
  • Is this is not available in ASP.NET Membership APi ? – Surjit Samra Dec 17 '11 at 11:42
  • 1
    I really don't like Membership. Besides I'd like to use my own classes, own tables, own control etc – Alex Kovanev Dec 17 '11 at 11:45
  • Any reason you don't like, Other than own classes, own tables , own controls ? As I was thinking to move to Membership APi :( – Surjit Samra Dec 17 '11 at 11:47
  • 2
    Ok. The first reason [here](http://stackoverflow.com/questions/8330340/a-way-to-avoid-deriving-from-the-provider-classes-in-mvc-authentication). The second - it's convenient for me to be able to control everything myself. Of course it's a matter of taste... – Alex Kovanev Dec 17 '11 at 11:51
  • Thanks for the link , I am feeling bit better now ;) – Surjit Samra Dec 17 '11 at 11:55
  • 2
    Wow. A well thought-out, well stated question having the proper code. Now this is a good question. I just wish I had a good answer for you... Unfortunately, my Reflection fu is weak. – squillman Dec 17 '11 at 15:53

2 Answers2

3

ANOTHER EDIT
In your case it would make sense to create a readonly property ExistingPermissions for the RolePermissions class, and do the merging of the four booleans into one CrudType within that property getter. Then you can just do rp.ExistingPermissions.HasFlag(permissionToCheck).

EDITED

Thanks to @DevDelivery for pointing out the issue - good catch. Unfortunately the fixed solution isn't as pretty as I was hoping for, so in this case it might make sense to go with @DevDelivery's approach.

Since you have your CrudType as "bitfields", you can use a cleaner approach (less code and better readability):

public static bool CanAccess(RolePermissions rp, CrudType permissionToCheck)
{
    CrudType existingPermissions = 
                                SetPermissionFlag(CrudType.CanCreate, rp.CanCreate) |
                                SetPermissionFlag(CrudType.CanRead, rp.CanRead) | 
                                SetPermissionFlag(CrudType.CanUpdate, rp.CanUpdate) |
                                SetPermissionFlag(CrudType.CanDelete, rp.CanDelete);

    return existingPermissions.HasFlag(permissionToCheck);
}

public static CrudType SetPermissionFlag(CrudType crudType, bool permission)
{
    return (CrudType)((int)crudType * Convert.ToInt32(permission));
}

The drawback compared to your solution is that you will have to modify this method in case you add more operations (to the existing CanRead, etc.).

Yakimych
  • 17,612
  • 7
  • 52
  • 69
  • The problem here is that the RolePermission properties are booleans. Or'ing a bunch of booleans will just give you another boolean. – DevDelivery Dec 17 '11 at 16:09
  • @DevDelivery - good catch, that wouldn't work indeed. Seems like the solution is not that pretty after the fix. – Yakimych Dec 17 '11 at 16:31
1

Using reflection has a performance impact and the late-binding means that changing a name of a enum or property will not get caught by the compiler.

Plus, this code is very hard to understand, thus difficult to maintain.

Here there are only 4 options to check. A simple switch statement is easier, faster, and cleaner.

Using reflection would make sense if you were trying to allow for changes to the database or for third-party components to introduce new permissions.

DevDelivery
  • 447
  • 3
  • 5