2
\$\begingroup\$

In the past I posted a question that tried to implement a nice API to support RBAC authorization to resources. And due to the epoch and probably to the bount it was moderatly well received by the community. Althought even I myself can now spot how fundamentally flawed was that question due to the following reasons:

  • The code was not properly tested by myself and the result was, of course, very bugy code
  • I didn't provide usage scenarios so you couldn't spot the usefulness of my implementation and why is it so good (or bad)

Having said that I applied the suggestions made by Mat, tested my code, removed many unnecessary redundant code and now I would like to get another review on the implementation.

The interfaces:

public delegate IEnumerable<string> GetUserPermissions(IPrincipal user, object resource);
public delegate IEnumerable<string> GetUserRoles(IPrincipal user, object resource);
public delegate bool IsUserInRole(IPrincipal user, object resource);

public interface IRbacSession
{
    IDictionary<string, Predicate<IPrincipal>> UserRoles { get; }
    IEnumerable<Role> RolePermissions { get; }
    IRbacQuery Query { get; }
    void AddPermission(string roleName, string action);
    void UserIsInRoleIf(string role, Predicate<IPrincipal> predicate);
    IDictionary<Type, GetUserRoles> UserRolesForType { get; }
    void AddUserRoleForTypeIf<T>(string role, IsUserInRole predicate);
}

public interface IRbacQuery
{
    bool IsUserInRole(IPrincipal user, string role);
    bool IsUserAbleTo(IPrincipal user, string action);
    bool IsUserInRole<T>(IPrincipal user, string role, T resource);
    bool IsUserAbleTo<T>(IPrincipal user, string action, T resource);
}

The query implementation

public class RbacQuery : IRbacQuery
{
    protected readonly IRbacSession _session;

    public RbacQuery(IRbacSession session)
    {
        _session = session;
    }

    public virtual IEnumerable<string> GetUserRoles<T>(IPrincipal user, T resource)
    {
        var userRoles = _session.UserRolesForType.TryGetOrEmpty(typeof(T));
        if (userRoles == null)
        {
            return Enumerable.Empty<string>();
        }
        return userRoles(user, resource);
    }

    public virtual bool IsUserInRole<T>(IPrincipal user, string role, T resource)
    {
        return user.IsInRole(role) ||
            GetUserRoles(user, resource).Contains(role, StringComparer.OrdinalIgnoreCase);
    }

    public virtual IEnumerable<string> GetUserPermissions<T>(IPrincipal user, T resource)
    {
        var userRoles = _session.UserRolesForType.TryGetOrEmpty(typeof(T));
        if (userRoles == null)
        {
            return GetUserPermissions(user);
        }
        return userRoles(user, resource).Union(GetUserPermissions(user));
    }

    public virtual bool IsUserAbleTo<T>(IPrincipal user, string action, T resource)
    {
        return GetUserRoles(user, resource).Any(r => IsUserInRole(user, r, resource)) ||
        GetUserPermissions(user, resource).Contains(action, StringComparer.OrdinalIgnoreCase);
    }

    public virtual IEnumerable<string> GetUserRoles(IPrincipal user)
    {
        var userRoles = _session.RolePermissions
            .Where(r => user.IsInRole(r.Name))
            .Select(r => r.Name);
        return _session.UserRoles
            .Where(role => role.Value(user))
            .Select(role => role.Key)
            .Union(userRoles);
    }

    public virtual IEnumerable<string> GetUserPermissions(IPrincipal user)
    {
        return GetUserRoles(user).SelectMany(GetRolePermissions);
    }

    public virtual bool IsUserInRole(IPrincipal user, string role)
    {
        if (user.IsInRole(role))
        {
            return true;
        }
        var userRole = _session.UserRoles.TryGetOrEmpty(role);
        bool roleExists = userRole != null ||
            _session.RolePermissions
            .Any(r => r.Name.Equals(role, StringComparison.OrdinalIgnoreCase));
        if (!roleExists)
        {
            throw new KeyNotFoundException($"The roleName {role} is not defined");
        }
        if (userRole == null)
        {
            return false;
        }
        return userRole(user);
    }

    public bool IsUserAbleTo(IPrincipal principal, string action)
    {
        return GetUserPermissions(principal).Contains(action, StringComparer.OrdinalIgnoreCase);
    }

    public virtual IEnumerable<string> GetRolePermissions(string roleName)
    {
        var role = _session.RolePermissions.SingleOrDefault(r => r.Name == roleName);
        if (role != null)
        {
            return role.Actions;
        }
        return Enumerable.Empty<string>();
    }
}

The RBAC session implementation

public class RbacSession : IRbacSession
{
    protected readonly IDictionary<string, Predicate<IPrincipal>> _userRoles;
    protected readonly ICollection<Role> _rolePermissions;
    private RbacQuery _query;
    private readonly IDictionary<Type, IDictionary<string, IsUserInRole>> _roleAssignment =
        new Dictionary<Type, IDictionary<string, IsUserInRole>>();
    public IRbacQuery Query
    {
        get
        {
            if (_query != null)
            {
                return _query;
            }
            return _query = new RbacQuery(this);
        }
    }
    public RbacSession()
    {
        _userRoles = new Dictionary<string, Predicate<IPrincipal>>();
        _rolePermissions = new HashSet<Role>();
    }

    IDictionary<string, Predicate<IPrincipal>> IRbacSession.UserRoles
    {
        get
        {
            return _userRoles;
        }
    }

    IEnumerable<Role> IRbacSession.RolePermissions
    {
        get
        {
            return _rolePermissions;
        }
    }

    public void AddPermission(string roleName, string action)
    {
        var role = _rolePermissions.SingleOrDefault(r => r.Name == roleName);
        if (role == null)
        {
            role = new Role(roleName);
            _rolePermissions.Add(role);
        }
        role.Actions.Add(action);
    }

    public void UserIsInRoleIf(string role, Predicate<IPrincipal> predicate)
    {
        _userRoles.Add(role, predicate);
    }

    IDictionary<Type, GetUserRoles> IRbacSession.UserRolesForType
    {
        get
        {
            var userRoles = new Dictionary<Type, GetUserRoles>();
            foreach (var assignment in _roleAssignment)
            {
                var assignmentContext = assignment;
                GetUserRoles getUserRoles = (user, resource) =>
                {
                    return assignmentContext.Value
                        .Where(e => e.Value(user, resource))
                        .Select(e => e.Key);
                };
                userRoles.Add(assignment.Key, getUserRoles);
            }
            return userRoles;
        }
    }

    public void AddUserRoleForTypeIf<T>(string role, IsUserInRole predicate)
    {
        var roleAssign = _roleAssignment.TryGetOrAdd(typeof(T), new Dictionary<string, IsUserInRole>(StringComparer.OrdinalIgnoreCase));
        roleAssign.Add(role, predicate);
    }
}

Utility methods (I do not require a review on those, but feel free to comment if you have something to say about them)

public static class DictionaryUtils
{
    public static V TryGetOrValue<K, V>(this IDictionary<K, V> dictionary, K key, V value)
    {
        V ret;
        if (dictionary.TryGetValue(key, out ret))
            return ret;
        return value;
    }

    public static V TryGetOrEmpty<K, V>(this IDictionary<K, V> dictionary, K key)
    {
        V ret;
        if (dictionary.TryGetValue(key, out ret))
            return ret;
        return default(V);
    }

    public static V TryGetOrAdd<K, V>(this IDictionary<K, V> dictionary, K key, V value)
    {
        V ret;
        if (dictionary.TryGetValue(key, out ret))
            return ret;
        dictionary.Add(key, value);
        return value;
    }

    public static V AddOrUpdate<K, V>(this IDictionary<K, V> dictionary, K key, V value, Func<V, V> newValue)
    {
        if (dictionary.ContainsKey(key))
        {
            return dictionary[key] = newValue(dictionary[key]);
        }
        return dictionary[key] = value;
    }
}

The fluent API (instead of doing review on this API you may instead prefer to discuss if it was a good, or a bad decision)

public class Rbac
{
    public class _User
    {
        private readonly IRbacSession _session;
        internal _User(IRbacSession session)
        {
            _session = session;
        }
        public UserRole Is(string role)
        {
            return new UserRole(_session, role);
        }
        [Browsable(false)]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public class UserRole
        {
            internal string Role { get; private set; }
            internal IRbacSession Session { get; private set; }
            internal UserRole(IRbacSession session, string role)
            {
                Session = session;
                Role = role;
            }

            public void If(Predicate<IPrincipal> predicate)
            {
                Session.UserIsInRoleIf(Role, predicate);
            }

            public UserRoleResource<T> Of<T>()
            {
                var session = Session;
                return new UserRoleResource<T>(this);
            }
        }

        [Browsable(false)]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public class UserRoleResource<T>
        {
            private readonly UserRole _userRole;

            internal UserRoleResource(UserRole role)
            {
                _userRole = role;
            }
            public void If(Func<IPrincipal, T, bool> pred)
            {
                var session = _userRole.Session;
                if (session == null)
                {
                    throw new NotSupportedException();
                }
                session.AddUserRoleForTypeIf<T>(_userRole.Role, (user, resource) => pred(user, (T)resource));
            }
        }

    }

    public class _Is
    {
        private readonly IRbacSession _session;
        internal _Is(IRbacSession session)
        {
            _session = session;
        }
        public Principal User(IPrincipal principal)
        {
            return new Principal(_session, principal);
        }

        [Browsable(false)]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public class Principal
        {
            internal IRbacSession Session { get; private set; }
            internal IPrincipal User { get; private set; }
            internal Principal(IRbacSession session, IPrincipal user)
            {
                Session = session;
                User = user;
            }
            public UserRole A(string role)
            {
                return new UserRole(this, role) { Result = Session.Query.IsUserInRole(User, role) };
            }

            [Browsable(false)]
            [EditorBrowsable(EditorBrowsableState.Never)]
            public class UserRole
            {
                private readonly Principal _principal;
                private readonly string _role;
                internal UserRole(Principal principal, string role)
                {
                    _principal = principal;
                    _role = role;
                }
                public bool Result { get; set; }

                public bool Of<T>(T resource)
                {
                    var session = _principal.Session;
                    Result = session.Query.IsUserInRole(_principal.User, _role, resource);
                    return Result;
                }
            }
        }
    }

    public class _Do
    {
        private readonly IRbacSession _session;
        internal _Do(IRbacSession session)
        {
            _session = session;
        }
        public _Action A(string name)
        {
            return new _Action(_session, name);
        }
        [Browsable(false)]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public class _Action
        {
            internal string Action { get; private set; }
            internal IRbacSession Session { get; private set; }

            internal _Action(IRbacSession session, string action)
            {
                Session = session;
                Action = action;
            }

            public void Requires(string role)
            {
                Session.AddPermission(role, Action);
            }

        }
    }

    public class _Can
    {
        private readonly IRbacSession _session;
        internal _Can(IRbacSession session)
        {
            _session = session;
        }
        public UserAction User(IPrincipal principal = null)
        {
            return new UserAction(_session, principal ?? Thread.CurrentPrincipal);
        }
        [Browsable(false)]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public class UserAction
        {
            internal IRbacSession Session { get; private set; }
            internal IPrincipal Principal { get; private set; }
            internal UserAction(IRbacSession session, IPrincipal principal)
            {
                Session = session;
                Principal = principal;
            }

            public ActionResource Do(string action)
            {
                return new ActionResource(this, action)
                {
                    Result = Session.Query.IsUserAbleTo(Principal, action)
                };
            }
            [Browsable(false)]
            [EditorBrowsable(EditorBrowsableState.Never)]
            public class ActionResource
            {
                private readonly UserAction _userAction;
                private readonly string _action;
                internal ActionResource(UserAction userAction, string action)
                {
                    _userAction = userAction;
                    _action = action;
                }

                public bool Result { get; set; }

                public bool The<T>(T resource)
                {
                    var session = _userAction.Session;
                    Result = session.Query.IsUserAbleTo(_userAction.Principal, _action, resource);
                    return Result;
                }
            }
        }
    }

    public Rbac(IRbacSession session)
    {
        User = new _User(session);
        Is = new _Is(session);
        Do = new _Do(session);
        Can = new _Can(session);
    }
    public _User User { get; private set; }
    public _Is Is { get; private set; }
    public _Do Do { get; private set; }
    public _Can Can { get; private set; }
}

Tests

[TestFixture]
public class TestRbac
{
    public class Principal : IPrincipal, IIdentity
    {
        private readonly HashSet<string> _roles = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
        public IIdentity Identity
        {
            get
            {
                return this;
            }
        }

        public ICollection<string> Roles { get { return _roles; } }

        public bool IsInRole(string role)
        {
            return _roles.Contains(role);
        }

        public string Name { get; set; }
        public string AuthenticationType { get; set; }
        public bool IsAuthenticated { get; set; }
    }

    private Dictionary<string, IPrincipal> _users;
    private Rbac _rbac;
    [SetUp]
    public void Init()
    {
        _users = new Dictionary<string, IPrincipal>();
        _users.Add("owner", new Principal
        {
            Roles = { "owner", "member", "user" }
        });
        _users.Add("member", new Principal
        {
            Roles = { "member", "user" }
        });
        _users.Add("user", new Principal
        {
            Roles = { "user" }
        });
        _users.Add("evaluator", new Principal
        {
            Roles = { "evaluator" }
        });
        _users.Add("teacher", new Principal
        {
            Roles = { "teacher" }
        });
        _users.Add("Bob", new Principal
        {
            Name = "Bob",
            Roles = { "owner" }
        });

        _rbac = new Rbac(new RbacSession());
        _rbac.Do.A("Delete").Requires("owner");
        _rbac.Do.A("Transfer").Requires("owner");
        _rbac.Do.A("Comment").Requires("member");
        _rbac.Do.A("Create").Requires("member");
        _rbac.Do.A("Read").Requires("user");
        _rbac.Do.A("Maintnance").Requires("mantainer");
        _rbac.User.Is("mantainer").If(u => u.Identity.Name == "Bob");

        _rbac.Do.A("Evaluation").Requires("Evaluator");
        _rbac.Do.A("Grading").Requires("Teacher");
        _rbac.User.Is("Teacher").Of<string>().If((user, resource) => resource=="Hello world");
    }


    [Test]
    public void TestHasRole()
    {
        Assert.IsTrue(_rbac.Is.User(_users["owner"]).A("owner").Result);
        Assert.IsTrue(_rbac.Is.User(_users["member"]).A("Member").Result);
        Assert.IsTrue(_rbac.Is.User(_users["member"]).A("User").Result);
        Assert.IsFalse(_rbac.Is.User(_users["member"]).A("owner").Result);
        Assert.IsTrue(_rbac.Is.User(_users["user"]).A("user").Result);
        Assert.IsFalse(_rbac.Is.User(_users["user"]).A("member").Result);
        Assert.IsTrue(_rbac.Is.User(_users["Bob"]).A("mantainer").Result);

        Assert.IsTrue(_rbac.Is.User(_users["teacher"]).A("Teacher").Of("the exam"));
        Assert.IsFalse(_rbac.Is.User(_users["user"]).A("teacher").Of("the exam"));
        Assert.IsTrue(_rbac.Is.User(_users["user"]).A("Teacher").Of("Hello world"));
    }

    [Test]
    public void TestCanDo()
    {
        Assert.IsTrue(_rbac.Can.User(_users["owner"]).Do("Delete").Result);
        Assert.IsTrue(_rbac.Can.User(_users["owner"]).Do("transfer").Result);
        Assert.IsTrue(_rbac.Can.User(_users["owner"]).Do("comment").Result);
        Assert.IsTrue(_rbac.Can.User(_users["owner"]).Do("Create").Result);
        Assert.IsFalse(_rbac.Can.User(_users["owner"]).Do("Maintnance").Result);

        Assert.IsTrue(_rbac.Can.User(_users["member"]).Do("Create").Result);
        Assert.IsTrue(_rbac.Can.User(_users["member"]).Do("read").Result);

        Assert.IsTrue(_rbac.Can.User(_users["user"]).Do("read").Result);
        Assert.IsFalse(_rbac.Can.User(_users["user"]).Do("Delete").Result);
        Assert.IsFalse(_rbac.Can.User(_users["user"]).Do("transfer").Result);

        Assert.IsTrue(_rbac.Can.User(_users["Bob"]).Do("Maintnance").Result);

        Assert.IsTrue(_rbac.Can.User(_users["evaluator"]).Do("Evaluation").The("anything :p"));
        Assert.IsTrue(_rbac.Can.User(_users["evaluator"]).Do("Evaluation").The(1));
        Assert.IsTrue(_rbac.Can.User(_users["teacher"]).Do("grading").The("the exam"));
        Assert.IsTrue(_rbac.Can.User(_users["user"]).Do("grading").The("Hello world"));
        Assert.IsFalse(_rbac.Can.User(_users["user"]).Do("grading").The("protected resource to other user"));
    }
}
\$\endgroup\$
  • \$\begingroup\$ code on github \$\endgroup\$ – Bruno Costa Jan 14 '16 at 21:48
  • \$\begingroup\$ @Mat'sMug maybe you are able to spend some time on this one again :p \$\endgroup\$ – Bruno Costa Jan 16 '16 at 7:51
3
+100
\$\begingroup\$

You know,

I love fluent interfaces.

though I feel many misconstrue their purpose.

some examples I see try to make their code a 'natural language'. but That is not the point, the point is to decrease complexity and aid understanding, often most easily done via Natural language.

I can't help but feel the above has crossed the threshold into requiring more time to learn than a standard composition. If i was dead set on fluent I would sacrifice a small amount of natural sentence structure for brevity:

 _users["owner"].Can("read").On(_rbac)

cuts 48 characters to 38 and keeps the intent clear. It also follows the consistent intention of the user of this code of asking a question about the article, in this case is the current user.

but why stop there?

why not make a DSL and eval and compile and evaluate your own custom language:

     string query = "Can *owner* Do *Delete*";
     if(eval(query)){}

why? ... because that is insane.


at the end of the day, ask yourself if you would genuinely prefer to parse the syntax:

 var hasRole = _rbac.Is.User(_users["Bob"]).A("Maintainer").Result

or the slightly more verbose:

 var user = _users["Bob"];
 var hasRole = user.HasRole("Maintainer",_schoolAdminSystem);

Take linq as an example. it takes a small bit more time to get used to than a system like this which has a more Sentence.Styled.Structure but it is a nice hybrid of english and code.

any time i see a fluent system with an Is or a Has, I have to wonder who the target audience is/ what is the objective

your team?

they understand code syntax, they don't need so much of a hand that it needs to be near english.

business users

to them code is code, they will be scared of a.sentence.like.this just as much as standard code.

the team taking over?

would you find it easy to add a feature to a system that is composed of a deeply nested structure of sub classes to enforce a sentence shape or to add an additional extension method and get 75% there?

Yourself?

well, having written the code base you presumably know it, often times a fluent interface is more verbose and so takes you longer, does it add enough value to yourself to warrant the additional workload?


So in summary i am not saying it isn't pretty cool, I can't really fault it as is designed, but as this is a code review, traditionally defined as an evaluation I have to evaluate the actual pragmatic value.

and I don't see it.

you can go fluent....er than code without architecting an entire supporting structure and creating a number of dangling hard to ducky-problem solve/ducky debug classes like _is.


TL;DR

when approaching designing a fluent interface I would err on the side of simplicity, having more tailored and specific functions like

 user.IsAllowedTo("organize")
 role.ContainsUsers("Mark","Mary")
 user.CanModify().Resource(StudentList);

may not be as all powerful but creating them when you need them is preferable to a god system that is hard to understand and edit, while not adding a lot more in terms of readability and take far longer to design and implement.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Nice. Thank you very very much to give me an explanation how a fluent API should be like. I wasn't really expecting that. Learned a lot! \$\endgroup\$ – Bruno Costa Jan 18 '16 at 17:54

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.