Authenticating user with Active Directory without placing incorrect username or password error inside catch block

Question

I'm trying to authenticate using Active Directory. This works fine but how can I authenticate without placing the incorrect username/password message inside the catch block? Isn't this bad practice? Many examples I find have this suggested. Does it even matter?

public void ADLogin()
{
    try
    {
        DirectoryEntry root = new DirectoryEntry("LDAP://" + "domain", txtUsername.Value, txtPassword.Value);
        DirectorySearcher searcher = new DirectorySearcher(root, "(sAMAccountName=" + txtUsername.Value + ")");
        SearchResult result = searcher.FindOne();
        if (result.Properties["sAMAccountName"] != null)
        {
            //AD Login Success        
            FormsAuthentication.RedirectFromLoginPage(txtUsername.Value, Login1.RememberMeSet);
        }
    }
    catch (Exception)
    {
            //AD Login failed         
            //Display Error Message
    }            
}

I've tried placing the catch block inside this if statement but it throws an exception before it reaches it:

public void ADLogin()
{
    DirectoryEntry root = new DirectoryEntry("LDAP://" + "domain", txtUsername.Value, txtPassword.Value);
    DirectorySearcher searcher = new DirectorySearcher(root, "(sAMAccountName=" + txtUsername.Value + ")");
    SearchResult result = searcher.FindOne();
    if (result.Properties["sAMAccountName"] != null)
    {
        //AD Login Success        
        FormsAuthentication.RedirectFromLoginPage(txtUsername.Value, Login1.RememberMeSet);
    }
    if (result.Properties["sAMAccountName"] == null)
    {
        //AD Login failed         
        //Display Error Message
    }            
}

Gabriel Luci · Accepted Answer · 2019-07-30T20:48:17.830

There's nothing wrong with catching the exception. You don't control the code inside DirectorySearcher, so you can't help that it throws an exception if something is wrong. However, you might want to differentiate the type of exceptions thrown, so you can tell the difference between bad credentials and a network error, for example.

Note that, if the credentials are bad, the exception will be thrown by searcher.FindOne(), since you are using the user's credentials to connect to AD.

This isn't the fastest way to validate the credentials. If you want something with better performance, you could use the LdapConnection solution here. It just performs an LDAP bind with the user's credentials, without doing a search like your code does. It has the added benefit of being able to tell you why the authentication failed, as that answer describes.

If you do need to find information from the user's account, then yes, you'll need to search for it. But you should use the DirectorySearcher.PropertiesToLoad collection. If you don't specify otherwise, DirectorySearcher will retrieve every attribute that has a value for each result. That can be a lot of data you don't need (especially if your organization stores user photos in AD). Instead, you can tell DirectorySearcher which attributes you need, and it will only retrieve those:

DirectorySearcher searcher = new DirectorySearcher(root, "(sAMAccountName=" + txtUsername.Value + ")");
searcher.PropertiesToLoad.Add("sAMAccountName");
result = searcher.FindOne();

I wrote a whole article about performance considerations when programming against AD that you might find interesting: Active Directory: Better performance

score 0 · Answer 2 · answered Jul 30 '19 at 19:16

What if you declared the SearchResult outside of the try/catch block?

public void ADLogin()
{
    SearchResult result = null;
    try
    {
        DirectoryEntry root = new DirectoryEntry("LDAP://" + "domain", txtUsername.Value, txtPassword.Value);
        DirectorySearcher searcher = new DirectorySearcher(root, "(sAMAccountName=" + txtUsername.Value + ")");
        result = searcher.FindOne();
    }
    catch (Exception)
    {
    }

    if (result != null && result.Properties["sAMAccountName"] != null)
    {
        //AD Login Success        
        FormsAuthentication.RedirectFromLoginPage(txtUsername.Value, Login1.RememberMeSet);
    }
    else
    {
        //AD Login failed         
        //Display Error Message
    }
}

Not sure why you think it's bad practice the way you had it though. The best practice (without having more context of where this code is) would probably be to do your error logging in the catch block and then re-throw; silent failure is a bad practice.

Bad credentials would throw an exception at `searcher.FindOne()`, since you're using the user's credentials to search AD. — Gabriel Luci, Jul 30 '19 at 19:19

Authenticating user with Active Directory without placing incorrect username or password error inside catch block

2 Answers2