1
\$\begingroup\$

The users get permissions to do specific things for a specific group of employees. These employee groups have a self reference. So an employee group (C) can be a child of group (B), and group (B) is child of (A). The user is granted permission to employee group (A), so if I want to check if the permission is granted for that user on employee group (C) it should give me true because the user is granted the permission on the parent or grandparent group. Currently, I am using this method and it works fine but I find it ugly and complicated a bit. Is there a more elegant way to do it?

This is the method:

public static bool CheckPermission(Session session, string right, int? emplooyeGroupID = null)
{
    using (Context db = new Context())
    {
        bool pass = false;
        var assigns = db.SecurityAssigns
            .Where(g => g.UserID == session.UserID);

        // if employeeGroupID is supplied we need to do some work here...
        if (emplooyeGroupID.HasValue)
        {
            // first, we get all 'groups' that have the requested SecurityRight
            assigns = assigns.Where(g => g.SecurityGroup.SecurityRightsInGroups.Any(r => r.SecurityRight.Right == right));

            foreach (var grp in assigns)
            {
                // if the employee group matches, then its a pass
                if (grp.EmployeeGroupID == emplooyeGroupID)
                {
                    pass = true;
                    break;
                }

                // if we reach here, it means the EmployeeGroupID does not match the EmployeeGroupID assigned for the SecurityRight. 
                // so we need to check if the requested EmployeeGroupID is a child of the EmployeeGroupID assigned to the SecurityRight
                var curGroup = db.EmployeeGroups.Single(g => g.ID == emplooyeGroupID);

                // loop through parents of the requested EmployeeGroup all the way up
                while (curGroup.ParentGroupID.HasValue)
                {
                    // is the parent authorized?
                    if (curGroup.ParentGroupID == grp.EmployeeGroupID)
                    {
                        pass = true;
                        break;
                    }

                    // parent is not authorized, go up one more step
                    curGroup = db.EmployeeGroups.Single(g => g.ID == curGroup.ParentGroupID);
                }
            }

        }
        else
        {
            pass = assigns.Any(g => g.SecurityGroup.SecurityRightsInGroups.Any(r => r.SecurityRight.Right == right));
        }

        if (!pass)
            throw new FaultException("Not Authorized!", new FaultCode(Common.ErrorCodes.NOT_AUTHORIZED_ERROR_CODE));

        return true;
    }
}

This is some the tables I am working with in the code above:

enter image description here

P.S. I am not a programmer and I didn't get any education on programming, it is just a hobby. So go easy on me if my coding is ugly :)

\$\endgroup\$
  • \$\begingroup\$ It seems that my question is seriously bad :/ \$\endgroup\$ – user32631 Dec 3 '13 at 19:44
2
\$\begingroup\$

No. I don't think your question is bad. It's only hard to answer on, since there are so many different ways to solve the problem your method addresses. And if they are better or not is kind of subjective. With that said here are some comment on your method:

Your method is too long and does too many things. I would recommend breaking it up into smaller method that does one thing; one that get the stuff from the database, one that checks the group permission and one that checks the parent group permission. Then the responsibility of the main method is to invoke the other if needed (and in what order).

I don't think the signature of the method is the best. Having int? emplooyeGroupID = null as a parameter is a clear indication that your method does two different major things; one if int? emplooyeGroupID is null and one if not. I would break it into two methods:

static bool CheckPermission(Session session, string right);
static bool CheckGroupPermission(Session session, string right, int emplooyeGroupID);

Then it will be up to the caller (client) to choose the right one. And since the caller knows if the emplooyeGroupID is null or not, the CheckPermission method don't need to "if" on it.

Why do you return true on the last line? I would definitely return false!

Otherwise you should be happy if the method does what you want it to.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy