2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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 an UserPrincipal and one to get a GroupPrincipal.

private UserPrincipal FetchUserPrincipal(string domainName, string userName, string password, string user)  
{
    using (var ctx = new PrincipalContext(ContextType.Domain, domainName, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, userName, password))
    {
        return UserPrincipal.FindByIdentity(ctx, 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 ctx = new PrincipalContext(ContextType.Domain, domainName, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, userName, password))
    {
        return GroupPrincipal.FindByIdentity(ctx, 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.

|improve this answer|||||
\$\endgroup\$
  • 2
    \$\begingroup\$ I'd use stackoverflow.com/a/15464440/648075 instead of .ToLower().Contains();. Make it an extension method and you can easily reuse it everywhere. \$\endgroup\$ – BCdotWEB Dec 19 '19 at 14:44
  • \$\begingroup\$ Thanks! Kinda shocked how bad my logic was. The return on the onAuthorization was something I was seeing on tutorials for custom Authorization attributes. I'm gonna test to see if it works as intended if that isn't there. \$\endgroup\$ – EarthBound Dec 19 '19 at 17:57
  • \$\begingroup\$ Found an issue with the solution when calling IsMemberOf. It leverages the PrincipalContext used in the group and user so you can't dispose of them until after that call. \$\endgroup\$ – EarthBound Dec 27 '19 at 19:14

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.