2
\$\begingroup\$

I have a Web API method that returns the Active Directory security groups for the specific login user. The below code was working fine but it was taking so munch time nearly 45 sec to get the results.

DirectoryEntry root = GetDirectoryEntry()
  using (var groups = root.Children.Find("OU=Sample Security Groups"))
                        {
                            using (var directory = groups.Children.Find("OU=Permissions"))
                            {
                                using (var searcher = new DirectorySearcher(directory))
                                {
                                    searcher.Filter = `filter condition`
                                    var results = searcher.FindAll();
                                    foreach (SearchResult result in results)
                                    {
                                        if (result != null)
                                        {
                                            using (DirectoryEntry group = result.GetDirectoryEntry())
                                            {
                                                items.Add((string)group.Properties["sAMAccountName"].Value);
                                            }
                                        }
                                    }
                                }
                            }
                        }

Update: In the GetDirectoryEntry method we are creating the connection to the active directory using the username and password.

Can anyone help to optimize the code using Parallel.ForEach or threading etc..

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! To get better answers you should add some more code, e.g the whole method and the GetDirectoryEntry() method as well. \$\endgroup\$ – Heslacher Oct 17 at 4:05
1
\$\begingroup\$

Let us first review the code.

  • Stacking usings will reduce the level of indentation.
  • The Filter of the DirectorySearcher can be passed to the constructor
  • If the expected result of the FindAll() call will be huge, you should consider to return only needed properties, by using this overloaded constructor of the DirectorySearcher class.
  • The SearchResultCollection returned by DirectorySearcher.FindAll() implements the IDisposable interface hence it should be enclosed in a using as well. The remarks section of the documentation states:

    Due to implementation restrictions, the SearchResultCollection class cannot release all of its unmanaged resources when it is garbage collected. To prevent a memory leak, you must call the Dispose method when the SearchResultCollection object is no longer needed.

Implementing some of this changes could look like so

DirectoryEntry root = GetDirectoryEntry();
using (var groups = root.Children.Find("OU=Sample Security Groups"))
using (var directory = groups.Children.Find("OU=Permissions"))
using (var searcher = new DirectorySearcher(directory, "filter condition"))
using (var results = searcher.FindAll())
{
    foreach (SearchResult result in results)
    {
        if (result != null)
        {
            using (var group = result.GetDirectoryEntry())
            {
                items.Add((string)group.Properties["sAMAccountName"].Value);
            }
        }
    }
}

If you only need to get the SamAccountName's of the groups a specific user is a member of you can use UserPrincipal.FindByIdentity() like so

UserPrincipal user = UserPrincipal.FindByIdentity(new PrincipalContext(ContextType.Domain, "domain", "username", "password"), IdentityType.SamAccountName, "loginUser");

foreach (GroupPrincipal group in user.GetGroups())
{
    Console.Out.WriteLine(group.SamAccountName);
}
\$\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.