Game Permission System

Question

I have just finished coding a permission system for my game server but one method in the class is seeming to be very complicated, and maybe some people on this site can help me improve the method?

Let me walk you through the permission system, which will give you a better understanding of the question, and how you can review my code.

The screenshot clearly explains a lot more just by looking at it.

PermissionManager.cs:

namespace MyApp.Permissions
{
    using System;
    using System.Collections.Generic;
    using System.Linq;
    using Players.Players;
    using Utilities;

    internal sealed class PermissionManager : IManager
    {
        private readonly Dictionary<int, Permission> _permissions;

        public PermissionManager()
        {
            _permissions = new Dictionary<int, Permission>();
        }

        public void Initialize()
        {
            using (var databaseConnection = Hariak.HariakServer.Database.NewDatabaseConnection)
            {
                databaseConnection.SetQuery("SELECT * FROM `game_permissions`");

                using (var reader = databaseConnection.ExecuteReader())
                {
                    while (reader.Read())
                    {
                        _permissions.Add(reader.GetInt32("id"), new Permission(
                            reader.GetString("permission_name"),
                            reader.GetString("ranks_allowed"),
                            reader.GetString("usernames_allowed"),
                            reader.GetInt32("minimum_rank"),
                            reader.GetInt32("maximum_rank"),
                            reader.GetString("user_ids_allowed"),
                            reader.GetString("custom_column_set")));
                    }
                }
            }
        }

        public bool CheckPermission(PlayerConnection playerConnection, string permissionName)
        {
            try
            {
                foreach (var permission in _permissions.Values.Where(permission => permission.PermissionName == permissionName))
                {
                    if (permission.RanksAllowedString.Contains(","))
                    {
                        var ranksAllowed = permission.RanksAllowedString.Contains(",") ? permission.RanksAllowedString.Split(',').Select(int.Parse).ToList() : new List<int>();

                        if (ranksAllowed.Any(rankAllowed => playerConnection.SelectColumnInt("rank") == rankAllowed))
                        {
                            return true;
                        }
                    }
                    else if (HelperUtilities.IsInteger(permission.RanksAllowedString) && playerConnection.SelectColumnInt("rank") == int.Parse(permission.RanksAllowedString))
                    {
                        return true;
                    }

                    if (permission.UsernamesAllowedString.Contains(","))
                    {
                        var usernamesAllowed = permission.UsernamesAllowedString.Contains(",") ? permission.UsernamesAllowedString.Split(',').ToList() : new List<string>();

                        if (usernamesAllowed.Any(usernameAllowed => usernameAllowed.StartsWith("\"") && usernameAllowed.EndsWith("\"") && usernameAllowed.Trim('"') == playerConnection.SelectColumn("username")))
                        {
                            return true;
                        }
                    }
                    else if (permission.UsernamesAllowedString.StartsWith("\"") && permission.UsernamesAllowedString.EndsWith("\"") && permission.UsernamesAllowedString.Trim('"') == playerConnection.SelectColumn("username"))
                    {
                        return true;
                    }

                    if (permission.MinimumRank >= 1 && playerConnection.SelectColumnInt("rank") >= permission.MinimumRank && (permission.MaximumRank == 0 || playerConnection.SelectColumnInt("rank") < permission.MaximumRank + 1))
                    {
                        return true;
                    }

                    if (permission.MinimumRank < 1 && permission.MaximumRank > 1 && playerConnection.SelectColumnInt("rank") < permission.MaximumRank + 1)
                    {
                        return true;
                    }

                    if (permission.UserIdsAllowedString.Contains(","))
                    {
                        var idsAllowed = permission.UserIdsAllowedString.Contains(",") ? permission.UserIdsAllowedString.Split(',').Select(int.Parse).ToList() : new List<int>();

                        if (idsAllowed.Any(idAllowed => playerConnection.SelectColumnInt("id") == idAllowed))
                        {
                            return true;
                        }
                    }
                    else if (HelperUtilities.IsInteger(permission.UserIdsAllowedString) && playerConnection.SelectColumnInt("id") == int.Parse(permission.UserIdsAllowedString))
                    {
                        return true;
                    }

                    if (!permission.CustomColumnSet.Contains("}{"))
                    {
                        continue;
                    }

                    foreach (var customColumnString in permission.CustomColumnSet.Split('}'))
                    {
                        if (!customColumnString.StartsWith("{"))
                        {
                            continue;
                        }

                        var cleanString = customColumnString.Substring(1);

                        var requestKey = cleanString.Split('=')[0];
                        requestKey = requestKey.Substring(1, requestKey.Length - 2);

                        var requestValue = cleanString.Split('=')[1];
                        requestValue = requestValue.Substring(1, requestValue.Length - 2);

                        if (requestKey.Trim('\'').Contains("."))
                        {
                            var tableAsking = requestKey.Split('.')[0];
                            var columnAsking = requestKey.Split('.')[1];

                            try
                            {
                                using (var databaseConnection = Hariak.HariakServer.Database.NewDatabaseConnection)
                                {
                                    databaseConnection.SetQuery("SELECT `" + columnAsking + "` FROM `" + tableAsking + "` WHERE `id` = @userId");
                                    databaseConnection.AppendParameter("userId", playerConnection.SelectColumnInt("id"));

                                    if (databaseConnection.ExecuteSingleString() == requestValue)
                                    {
                                        return true;
                                    }
                                }
                            }
                            catch (Exception e)
                            {
                                Console.WriteLine(e);
                                throw;
                            }
                        }

                        else if (playerConnection.SelectColumn(requestKey) == requestValue)
                        {
                            return true;
                        }
                    }
                }

                return false;
            }
            catch (Exception)
            {
                return false;
            }
        }
    }
}

Permission.cs:

namespace MyApp.Permissions
{
    internal sealed class Permission
    {
        public Permission(string permissionName, string ranksAllowedString, string usernamesAllowString, int minimumRank, int maximumRank, string userIdsAllowedString, string customColumnSet)
        {
            PermissionName = permissionName;
            RanksAllowedString = ranksAllowedString;
            UsernamesAllowedString = usernamesAllowString;
            MinimumRank = minimumRank;
            MaximumRank = maximumRank;
            UserIdsAllowedString = userIdsAllowedString;
            CustomColumnSet = customColumnSet;
        }

        internal string PermissionName { get; }

        internal string RanksAllowedString { get; }

        internal string UsernamesAllowedString { get; }

        internal int MinimumRank { get; }

        internal int MaximumRank { get; }

        internal string UserIdsAllowedString { get; }

        internal string CustomColumnSet { get; }
    }
}

In my opinion, most of the code is clean. The main thing I am looking for help with is the CheckPermission method. It's very large (compared to other methods in my Solution) and I feel like it could be improved a lot.

What does SelectColumnInt do? Simply selected a column from the 'users' table with the player's id.

I haven't checked for just one rule in the custom_column_set column because I am under the impression the foreach will handle it even if it doesn't contain a, (multiple {}'s? If I am wrong, please correct me).

Mike Brant · Accepted Answer · 2017-03-23 14:05:13Z

From a purely MySQL perspective, I would comment that your schema does not seem appropriate.

Why reference both username AND user id for permissioning? Should user id be the authoritative identifier?

Why store users with given permission in a comma-separated list as opposed to using an actual many-to-many join table between permissions and users which would be a typically normalized approach? The way you have it now, you will have inefficient queries when looking up permissions for a given user id.

The same concern might be true about ranks_allowed, though it is unclear how this is used in your application.

Just generally your approach seems odd here in looking at your code. Normally, one might model permissions as being attached to a user object. That is when you marshall a user object for use in your system, you would also attach the permissions that user has within the system. These truly are properties of the user in a real-world sense. So it would seem to me that, if that were correctly modeled, all your permissions management code would need to do is to match those user permissions against the permissions required to perform some functionality.

The way you are doing it now is sort of the inverse. You look at the permission, and then determine if permission is appropriate for a given user, which just seems an odd way to model this, unless your primary use case was not to determine what an individual user has access to, but rather determining all users who have a permission as one might in the admin portion of an application.

My best advice, is to try to model your classes (and related database tables) in as close to a real world sense as you can. This typically helps simplify your code, and put the proper functionality in the proper classes.

Adriano Repetti · Accepted Answer · 2017-03-23 09:47:10Z

Let me start with Permission class. It's declared internal but this is redundant and I'd simply drop it. All its methods are internal with the exception of the ctor, this is little bit confusing. Is there any special reason for that? Make ctor internal if this is your coding style. What I usually do is to design the class as it was public (but still making it internal), it helps me to have consistent declarations and to have an easy transition if I will make it public in future. Just matter of personal style, I think.

More important than this is those constructor with long list of parameters. That's bad for two reasons:

Calling point is hard to read. What if you invert two strings? Error is not obvious at calling point and it may go unnoticed. Yes you may use named parameters but...
If you need another value then you need to add another parameter.
If that value is unknown when object is constructed then you will have a mixed initialization style.

What I'd do is to drop all parameters and let them be read/write properties. To ensure all values are properly initialized you have two techniques:

Debug.Assert() and tests. You should already have these in-place but it's nice to remember that you should validate object state. For internal objects this is pretty easy and Debug.Assert() should be enough.
Use a dumb data-only entity, this is similar to what, for example, ProcessStartInfo is. In this way you ctor will have just one parameter.

You may want to add an extra check to be sure that properties aren't overwritten:

public string Foo
{
    get { return _foo; }
    set
    {
        if (_foo != null)
            throw new InvalidOperationException("Cannot set this property twice.");

        if (_foo == null)
            throw new ArgumentNullException();

        _foo = value;
    }
}

Yes it's verbose but it may be embedded in an helper method (do not use this code as-is, it's just an example):

static void Set<T>(ref T backingStore, T value)
{
    if (!Equals(default(T), backingStore))
        throw new InvalidOperationException("...");

    if (value == default(T))
        throw new ArgumentException("...");

    backingStore = value;
}

Let's talk about PermissionManager. I agree with Mike about the responsibility of these checks.

You probably do not need a manager here (as someone said when you have a class named manager then you're probably picking the wrong design). It's, however, handy to have a separate object delegated to perform these checks because it may:

To keep a cache of those permissions (drop that Initialize() method, if you do not want to do it in the ctor then make it lazy) when initialization is expensive.
To change user permissions on-the-fly without needing to search all objects that holds the permission set.
Centralize security access into a single class which calling code won't even see. As Mike said you should, however, change calling code to simply query for its required permission and that code should delegate to this class. This will also let you switch implementation pretty easily (also at run-time.)

All these said then you're right, this class is too complex but solution is just to split responsibilities. CheckPermission() does too many things in one single method, let's split it.

public bool CheckPermission(PlayerConnection playerConnection, string permissionName)
{
    foreach (var permission in GetPermissionsByName(permissionName))
    {
        if (IsGranted(playerConnection, permission))
            return true;
    }

    return false;
}

Few things to note. First of all drop that catch (Exception). You do not want to swallow every possible error simply returning false unless you want to spend time trying to understand why you randomly you do not have permissions to do something, after few days of investigation you will find it happens only when server is highly loaded and another day later you will see that it happens because you're running out-of-memory and OutOfMemoryException is thrown... Catch what you can handle and leave everything else to an outer handler (and do not log to console!)

Let's try to name things and move responsibilities. permission.RanksAllowedString it may be a list, right? Make it a list and do not parse its value each time (also do not repeat type name in property name, it should be just AllowedRanks, it doesn't matter its type). Split the list when you build the Permission object. It also applies to other properties too (for example UserIdsAllowedString.) Note that Int32.Parse will parse numbers using current UI culture, it's not probably what you expect, specify a CultureInfo (for example CultureInfo.InvariantCulture). That line of code may simply be (no need to explicitly check if string contains ,.)

permission.AllowedRanks = allowedRanksString.Split(',')
    .Select(x => Int32.Parse(x, CultureInfo.InvariantCulture)
    .ToList();

How IsGranted() looks like? Let's start to write it:

bool IsGranted(PlayerConnection playerConnection, Permission permission)
{
    if (IsGrantedByRank(playerConnection, permission))
        return true;

    if (IsGrantedByUserName(playerConnection, permission))
        return true;

    // And so on...
}

bool IsGrantedByRank(PlayerConnection playerConnection, Permission permission)
{
    return permission.AllowedRanks.Contains(playerConnection.SelectColumnInt("rank"));
}

Note that SelectColumInt() is a big source of mistakes. You will probably have "rank" string splatted all over your code, let's introduce a typed entity to hold that data!

bool IsGrantedByRank(PlayerConnection playerConnection, Permission permission)
{
    return permission.AllowedRanks.Contains(playerConnection.Rank);
}

I don't even start to check when you're reading database, it's not something you want to do during game playing, do not repeat thousands times what might be done just once (server CPU time is precious!)

I won't rewrite all your code here (there are other small nitpicks), the point I want to make clear is that each function must be as small and descriptive as possible. Your time is precious and you have to read as little code as possible to understand what it is doing.

score 4 · Accepted Answer · 2017-03-23 00:11:00Z

4

A couple of things I noticed:

This:

var ranksAllowed = permission.RanksAllowedString.Contains(",") ? permission.RanksAllowedString.Split(',').Select(int.Parse).ToList() : new List<int>();

Is redundant. This line can only be executed if permission.RanksAllowedString.Contains(","). You can simplify it to:

var ranksAllowed = permission.RanksAllowedString.Split(',').Select(int.Parse).ToList();

The same thing here:

var usernamesAllowed = permission.UsernamesAllowedString.Contains(",") ? permission.UsernamesAllowedString.Split(',').ToList() : new List<string>();

Also it looks to me you're allowing the strings to represent too many things. It appears they can have multiple parts or they can be numbers. I think that a refactor would be in order. Each string should represent only one type of value(s).

edited Mar 23, 2017 at 0:11

answered Mar 23, 2017 at 0:08

user33306

1

\$\begingroup\$ @LiamSavage - I added another thought for you. \$\endgroup\$

user33306
– user33306

2017-03-23 00:12:31 +00:00
Commented Mar 23, 2017 at 0:12
\$\begingroup\$ Thanks you, I'll take note of your suggestions and start refactoring around your suggestions. \$\endgroup\$

Josh Hallow
– Josh Hallow

2017-03-23 00:15:24 +00:00
Commented Mar 23, 2017 at 0:15
\$\begingroup\$ One thing that might help, is using lists instead of delimited strings. \$\endgroup\$

user33306
– user33306

2017-03-23 00:16:38 +00:00
Commented Mar 23, 2017 at 0:16
\$\begingroup\$ Could you expand on that? \$\endgroup\$

Josh Hallow
– Josh Hallow

2017-03-23 00:21:28 +00:00
Commented Mar 23, 2017 at 0:21
1

\$\begingroup\$ You seem to be storing multiple values in one string. Adding those values to a list, instead, would help to make your code more efficient. \$\endgroup\$

user33306
– user33306

2017-03-23 01:33:36 +00:00
Commented Mar 23, 2017 at 1:33

Add a comment |

Stack Exchange Network

Game Permission System

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Game Permission System

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions