Checks if user is part of Active Directory group

Question

I'm working in C# using PrincipalContext to do a check if user is part of security group for access to the site. Site is asp.net MVC and part of an internal network with security groups set up via active directory.

Currently load times for pages are about 2.5-3 seconds on first load and then about 1.4-1.5 seconds on subsequent loads for each action. I'd like to decrease the time if possible and have set the query to check the domain of the user which cut some time out.

public override void OnAuthorization(AuthorizationContext filterContext)
{
    var alias = HttpContext.Current.User.Identity.Name;
    var domain = alias.Substring(0, alias.IndexOf("\\"));
    alias = alias.Substring(alias.IndexOf("\\") + 1);
    var accessLevel = this.Roles;

    if (CheckAccess(accessLevel, alias, domain))
    {
        return;
    }
}

private bool CheckAccess(string group, string user, string userDomain)
{
    PrincipalContext ctx;
    GroupPrincipal AdGroup = null;
    UserPrincipal AdUser = null;
    using (Forest forest = Forest.GetCurrentForest())
    {
        foreach(Domain domain in forest.Domains)
        {
            if (domain.Name.Contains("domain1"))
            {
                ctx = new PrincipalContext(ContextType.Domain, domain.Name, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, "username", "password");
                if (group == "WRITE")
                {
                    AdGroup = GroupPrincipal.FindByIdentity(ctx, "group1");
                }

                if (userDomain.ToLower().Contains("domain1"))
                {
                    AdUser = UserPrincipal.FindByIdentity(ctx, user);
                }
            }
            else if (domain.Name.Contains("domain2"))
            {
                if (userDomain.ToLower().Contains("domain2"))
                {
                    ctx = new PrincipalContext(ContextType.Domain, domain.Name, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, "username", "password");
                    AdUser = UserPrincipal.FindByIdentity(ctx, user);
                }
            }
        }

        if (AdGroup != null && AdUser != null)
        {
            if (AdUser.IsMemberOf(AdGroup))
            {
                return true;
            }
            else
            {
               return false;
            }
        }
        else
        {
            return false;
        }
     }
}

Also first question on CodeReview, let me know if there is anything to do to improve the question that I might have missed in the FAQ.

Answer 1

OnAuthorization()

If the possibility exists that HttpContext.Current.User.Identity.Name dosn't contain \ the call to alias.Substring() will fail. Instead of calling two times alias.IndexOf("\\") you could use string.Split().

This

if (CheckAccess(accessLevel, alias, domain))
{
    return;
}  

is a little bit useless. You don't rellay care about the returned value of CheckAccess() hence you could remove it. But I guess thats not what you want. Instead you should e.g throw an AuthenticationException.

CheckAccess

PrincipalContext, GroupPrincipal and UserPrincipal are implementing the IDisposable interface, hence its best practice to enclose the usage in a using block.

Local variables should, based on the .NET Naming Guidelines, be named using camelCase casing and shouldn't use abbreviations. Hence AdUser should be activeDirectoryUser and AdGroup should be activeDirectoryGroup.

You should validate group and userDomain wether you really need to do the loop and you could store the result in a bool as well like so

bool userDomainIsInDomain1 = userDomain.ToLower().Contains("domain1");
bool userDomainIsInDomain2 = userDomain.ToLower().Contains("domain2");
bool groupCanWrite = group == "WRITE";  

if(!userDomainIsInDomain && !userDomainIsInDomain2 && !groupCanWrite)
{ 
    return false;
}  

We can now use the introduced bool variables in the loop like so

foreach (Domain domain in forest.Domains)
{
    if (domain.Name.Contains("domain1"))
    {
        using (var ctx = new PrincipalContext(ContextType.Domain, domain.Name, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, "username", "password"))
        {
            if (groupCanWrite)
            {
                activeDirectoryGroup = GroupPrincipal.FindByIdentity(ctx, "group1");
            }

            if (userDomainIsInDomain1)
            {
                activeDirectoryUser = UserPrincipal.FindByIdentity(ctx, user);
            }
        }
    }
    else if (domain.Name.Contains("domain2"))
    {
        if (userDomainIsInDomain2)
        {
            using(var ctx = new PrincipalContext(ContextType.Domain, domain.Name, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, "username", "password"))
            {
                activeDirectoryUser = UserPrincipal.FindByIdentity(ctx, user);
            }
        }
    }
}  

but wait we can do better than that. We should extract two methods, one to get the UserPrincipal and one to get the GroupPrincipal.

private UserPrincipal FetchUserPrincipal(string domainName, string userName, string password, string user)  
{
    using (var principalContext = new PrincipalContext(ContextType.Domain, domainName, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, userName, password))
    {
        return UserPrincipal.FindByIdentity(principalContext, user);
    }
}

I am unsure about the user variable because I don't know if user is the same as "username".

private GroupPrincipal FetchGroupPrincipal(string domainName, string userName, string password, string groupName)  
{
    using (var principalContext= new PrincipalContext(ContextType.Domain, domainName, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, userName, password))
    {
        return GroupPrincipal.FindByIdentity(principalContext, groupName);
    }
}  

After implementing these methods the former CheckAccess() method will look like this

private bool CheckAccess(string group, string user, string userDomain)
{
    bool userDomainIsInDomain1 = userDomain.ToLower().Contains("domain1");
    bool userDomainIsInDomain2 = userDomain.ToLower().Contains("domain2");
    bool groupCanWrite = group == "WRITE";  
    
    if(!userDomainIsInDomain && !userDomainIsInDomain2 && !groupCanWrite)
    { 
        return false;
    }  

    GroupPrincipal activeDirectoryGroup = null;
    UserPrincipal activeDirectoryUser = null;
    using (Forest forest = Forest.GetCurrentForest())
    {
        foreach (Domain domain in forest.Domains)
        {
            if (domain.Name.Contains("domain1"))
            {
                if (groupCanWrite)
                {
                    activeDirectoryGroup = FetchGroupPrincipal(domain.Name, "username", "password", "group1"); 
                }

                if (userDomainIsInDomain1)
                {
                    activeDirectoryUser = FetchUserPrincipal(domain.Name, "username", "password", user); 
                }
            }
            else if (userDomainIsInDomain2 && domain.Name.Contains("domain2"))
            {
                activeDirectoryUser = FetchUserPrincipal(domain.Name, "username", "password", user); 
            }
        }  

        if (AdGroup != null && AdUser != null)
        {
            return AdUser.IsMemberOf(AdGroup);
        }
        return false;
     }
}

As you can see, I have simplified the if..else at the bottom as well.