Verifying that an Active Directory user is in a certain OU or group

Question

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;
    }
}

Answer 1

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.

Answer 2

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.