6
\$\begingroup\$

I'm writing an application that needs to verify that an Active Directory user is in a certain OU or group. This allows me to tell if the user is a Field Agent or a Home Office User. However, I didn't want to fixate on one OU and one group, so I allow the OU names and group names to be comma separate values in the web.config file. I also added a feature where I can whitelist specific usernames, as well, for testing and/or programmer access.

This is untested as I'm just starting on the project. The method is kind of long and feels like it can be optimized. Should I add a helper method to check the lists or try a Linq expression or something else? Let me know if the Active Directory code can be improved, as well.

private LpaUserType VerifyUserAccess(string Username)
{
    try
    {
        // Load whitelist values
        // Load list of OU names
        var fieldOrgUnits = new List<string>(Settings.Default.AllowedFieldOrganizationUnits.Split(','));
        var homeOfficeOrgUnits = new List<string>(Settings.Default.AllowedHomeOfficeOrganizationUnits.Split(','));

        // Load list of Group names
        var fieldGroups = new List<string>(Settings.Default.AllowedFieldGroups.Split(','));
        var homeOfficeGroups = new List<string>(Settings.Default.AllowedHomeOfficeGroups.Split(','));

        // Load list of User names
        var fieldUsers = new List<string>(Settings.Default.AllowedFieldUsers.Split(','));
        var homeOfficeUsers = new List<string>(Settings.Default.AllowedHomeOfficeUsers.Split(','));

        // Create context object for the configured AD Domain
        var pc = new PrincipalContext(ContextType.Domain, Settings.Default.ActiveDirectoryDomainName);

        // Check username first; saves on loading more objects
        if (fieldUsers.Contains(Username))
        {
            return LpaUserType.FieldAgent;
        }

        if (homeOfficeUsers.Contains(Username))
        {
            return LpaUserType.HomeOffice;
        }

        // No match found by username, fetch user security object
        var user = UserPrincipal.FindByIdentity(pc, Username);

        // Get list of user's groups
        var userGroups = user.GetGroups();

        // Check all of the user's groups and see if one matches a whitelist group
        foreach (var group in userGroups)
        {
            if (fieldGroups.Contains(group.Name))
            {
                return LpaUserType.FieldAgent;
            }
        }

        // Checking again for Home Office users; don't want to do this check inside same loop
        // In the unlikely event that a user is in a field group and home office group
        // I want the field one to return first, as it is more limited
        foreach (var group in userGroups)
        {
            if (homeOfficeGroups.Contains(group.Name))
            {
                return LpaUserType.HomeOffice;
            }
        }

        // Finally, we will check for Organization Unit
        DirectoryEntry deUser = user.GetUnderlyingObject() as DirectoryEntry;
        DirectoryEntry deUserContainer = deUser.Parent;

        // Sample value: OU=Users,OU=BusinessTechnology,OU=HOWOW,DC=hoad,DC=local
        var props = deUserContainer.Properties["distinguishedName"].Value as string;

        if (String.IsNullOrEmpty(props))
        {
            // If we got this far and the props variable is empty, we have a problem
            return LpaUserType.Unknown;
        }

        // Check for Field OU
        foreach (var fieldOrgUnit in fieldOrgUnits)
        {
            if (props.Contains("OU=" + fieldOrgUnit))
            {
                return LpaUserType.FieldAgent;
            }
        }

        // Check for Home Office OU
        foreach (var homeOfficeOrgUnit in homeOfficeOrgUnits)
        {
            if (props.Contains("OU=" + homeOfficeOrgUnit))
            {
                return LpaUserType.HomeOffice;
            }
        }

        // If we get to here, we found nothing...
        return LpaUserType.Unknown;
    }
    catch (Exception ex)
    {
        //TODO: Log exception
        return LpaUserType.Unknown;
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ "This is untested as I'm just starting on the project." According to codereview.stackexchange.com/help/on-topic the code you want us to review needs to work. \$\endgroup\$ – BCdotWEB Oct 27 '14 at 14:00
  • \$\begingroup\$ @BCdotNET, assuming you had Doug Dawson's settings file and the LpaUserType definition, this wouldn't work for you anyway (Active directory based things are environment specific). I would argue that this meets the basic definition of "working", as it can compile and there are no immediate "bugs". OP is just asking for advice on how to reduce lines of code/complexity. \$\endgroup\$ – AWinkle Oct 27 '14 at 15:05
  • \$\begingroup\$ @BCdotNET Thanks for pointing that out. I was thinking along the lines of AWinkle. The method was complete even though the rest of the project wasn't done yet, so I figured it was ok to post. I just wanted to get some input before I got too far down the road. \$\endgroup\$ – Doug Dawson Oct 27 '14 at 15:31
  • \$\begingroup\$ Could you please change the title, to say what your code does. \$\endgroup\$ – bhathiya-perera Oct 31 '14 at 2:23
3
\$\begingroup\$

To refactor your looping, you could use a lambda query. I'm also a big fan of trying to make as much repeat code a function as possible, even if it is simplistic because you don't know what might need to change 6+ months from now.

So take

    // Check username first; saves on loading more objects
    if (fieldUsers.Contains(Username))
    {
        return LpaUserType.FieldAgent;
    }

and

    var userGroups = user.GetGroups();

    // Check all of the user's groups and see if one matches a whitelist group
    foreach (var group in userGroups)
    {
        if (fieldGroups.Contains(group.Name))
        {
            return LpaUserType.FieldAgent;
        }
    }

and make them into

    if (ContainsUserName(fieldUsers, UserName) || ContainsGroup(user.GetGroups(), fieldGroups)
    {
        return LpaUserType.FieldAgent;
    }

with the two new methods of:

    public bool ContainsUserName(List<string> list, string userName) 
    {
        return list.Contains(userName);
    }
    public bool ContainsGroup(PrincipalSearchResult<Principal> list, List<string> groups) {
        return list.Any(x => groups.Contains(x.Name));
    }

you could also just do this:

    if (fieldUsers.Contains(UserName) || user.GetGroups().Any(x => fieldGroups.Contains(x.Name))
    {
         return LpaUserType.FieldAgent;
    } 

if this is more agreeable.

\$\endgroup\$
5
\$\begingroup\$

I would add that where you initialise a new list by using a split to use the ToList method.

var fieldOrgUnits = new List<string>(Settings.Default.AllowedFieldOrganizationUnits.Split(','));

would become:

var fieldOrgUnits = Settings.Default.AllowedFieldOrganizationUnits.Split(',').ToList();

This is a little more concise in my opinion.

\$\endgroup\$

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.