My first blog post about AD authentication proven to be very popular – amount of visits to this post in the last month have beaten the previous all-popular post about HTTPS in MVC and even about configuring Dependency Injection with Identity. So I decided to write a follow-up with clarifications and corrections.

Guest account and security issue

I’ve been working with that code for a while now and it was in production since start of the year. However I have discovered an unsettling security issue. The issue was not exposed in my sample code – it was covered by try-catch expression, but for a wrong reason. I’d like to correct the issue here.

In the sample code for authentication I’m using code like this: isAuthenticated = principalContext.ValidateCredentials(username, password, ContextOptions.Negotiate);. But it turns out that ValidateCredentials will return true for an unknown user if Guest account is enabled in the system. Why does it do it – beyond me; I did look into source code of System.DirectoryServices.AccountManagement module and it looks like the problem is actually with Active Directory replies rather than with .Net library that deals with AD.

To prevent this issue affecting our code in any way, in ActiveDirectoryAuthentication.cs starting from line #41 replace code with this:

try
{
    userPrincipal = UserPrincipal.FindByIdentity(principalContext, username);
    if (userPrincipal != null)
    {
        isAuthenticated = principalContext.ValidateCredentials(username, password, ContextOptions.Negotiate);
    }
}
catch (Exception exception)
{
    //TODO log exception in your ELMAH like this:
    //Elmah.ErrorSignal.FromCurrentContext().Raise(exception);
    return new AuthenticationResult("Username or Password is not correct");
}

This basically swaps the 2 queries – first verify that account exists, and if it exists check that password is valid. And really the exception handling is not needed here – I’ve not seen an exception been thrown there ever (given production system with about 2000 users daily). But better keep it for a good measure -)))

Roles

Multiple users left comments asking how roles should be applied and why do they not work. Well.. I did not need roles so I did not implement them, but they are pretty simple. In AdAuthenticationService.CreateIdentity() method all you need to add is this:

var groups = userPrincipal.GetAuthorizationGroups();
foreach (var @group in groups)
{
    identity.AddClaim(new Claim(ClaimTypes.Role, @group.Name));
}

The entire method will be:

    private ClaimsIdentity CreateIdentity(UserPrincipal userPrincipal)
    {
        var identity = new ClaimsIdentity(MyAuthentication.ApplicationCookie, ClaimsIdentity.DefaultNameClaimType, ClaimsIdentity.DefaultRoleClaimType);
        identity.AddClaim(new Claim("http://schemas.microsoft.com/accesscontrolservice/2010/07/claims/identityprovider", "Active Directory"));
        identity.AddClaim(new Claim(ClaimTypes.Name, userPrincipal.SamAccountName));
        identity.AddClaim(new Claim(ClaimTypes.NameIdentifier, userPrincipal.SamAccountName));
        if (!String.IsNullOrEmpty(userPrincipal.EmailAddress))
        {
            identity.AddClaim(new Claim(ClaimTypes.Email, userPrincipal.EmailAddress));
        }

        var groups = userPrincipal.GetAuthorizationGroups();
        foreach (var @group in groups)
        {
            identity.AddClaim(new Claim(ClaimTypes.Role, @group.Name));
        }

        // add your own claims if you need to add more information stored on the cookie

        return identity;
    }

Then you’ll be able to restrict access to controllers by good-old Authorize attribute on top of your controllers or actions: [Authorize(Roles = "Users")]. To check if this works – add role authorization to some role that does not exist: [Authorize(Roles = "NonExistingRole")] – you should be redirected to the login screen.

If you are migrating your project from MembershipProvider – you might get SQL connection exception here – for these cases check if you have something about RoleProvider in your web.config.

Samples

As per usual, I’ve updated the code samples in github repository and added some more useful bits. If you are the “show-me-the-codez” type of developer, go to github and download the sample solution with AD authentication implemented and working.

  • Chris Cooper

    var groups = userPrincipal.GetAuthorizationGroups();
    foreach (var @group in groups)
    {
    identity.AddClaim(new Claim(ClaimTypes.Role, @group.Name));
    }

    This throws a System.IO.FileNotFoundException when iterating through groups. Any idea what might cause this?

  • David Engel

    Thanks so much for these posts, it has really helped. I have played with this solution today and all worked well except the performance of SignIn and CreateIdentity was taking over 20 seconds. I am using local machine authentication with only 2 users and a handful of groups. After some testing I found the FindByIdentity method and GetAuthorizationGroups method to be very slow. After researching, i replaced the FindByIDentity method with PrincipalSearcher like this:

    PrincipalContext principalContext = new PrincipalContext(authenticationType);
    bool isAuthenticated = false;
    UserPrincipal userPrincipal = new UserPrincipal(principalContext);
    userPrincipal.SamAccountName = username;
    var searcher = new PrincipalSearcher(userPrincipal);
    try
    {
    //userPrincipal = UserPrincipal.FindByIdentity(principalContext, username);
    userPrincipal = searcher.FindOne() as UserPrincipal;

    I also replaced the GetAuthorizationGroups method also like this:

    var claims = new List();
    var groups = new GroupPrincipal(userPrincipal.Context);
    var searcher = new PrincipalSearcher(groups);
    foreach (Principal item in searcher.FindAll())
    {
    var foundGroup = item as GroupPrincipal;
    if (foundGroup != null && foundGroup.IsSecurityGroup == true)
    {
    claims.Add(new Claim(ClaimTypes.Role, foundGroup.Name));
    }
    }
    if (claims.Count > 0)
    {
    identity.AddClaims(claims);
    }

    Now the login is almost immediate. Hope it helps!

    Here is a reference page:
    https://connect.microsoft.com/VisualStudio/feedback/details/757710/findbyidentity-much-slower-than-using-principalsearcher

    • O_O I knew the AD operations were slow, but not 20-seconds slow – have not seen this before. Thanks for sharing, I’ll try it out when I have a moment.

    • Sephiroth

      Hi

      Just a quick question. I can’t use GetAuthorizationGroups() since my Active Directory is on a different domain and calling GetAuthorizationGroups() yields an exception.

      So I’m using your method with PrincipalSearcher…But instead of giving back the roles of the user, It returns ALL roles from the Active Directory.

      Can you please help me understand why it would do this?

      Thanks

      • Sephiroth

        I got a fix by doing the following:

        var claims = new List();
        DirectoryEntry dirEntry = (DirectoryEntry)userPrincipal.GetUnderlyingObject();
        foreach (string groupDN in dirEntry.Properties[“memberOf”])
        {
        string[] parts = groupDN.Replace(“CN=”, “”).Split(‘,’);
        claims.Add(new Claim(ClaimTypes.Role, parts[0]));
        }

        if (claims.Count > 0)
        {
        identity.AddClaims(claims);
        }

        I instantiate a DirectoryEntry class by calling GetUnderlyingObject() from userPrinciple.
        then I loop through the properties named “memberOf” of the DirectoryEntry.
        I find the text part that names the Roles and add it to the Claims.

        • Great solution! Thanks for sharing

    • Ritz Monterrey

      Dude this is blazing fast…it was taking me 50 secs, now 1 sec. love it. thanks my friend

  • joe

    Thanks for the articles.

    I have a question and a very nit-picky comment.

    Question: I have implemented this code and it works, but if I login correctly with an account that isn’t part of a role that has access, the user is redirected back to the login page instead of displaying a 401. Any ideas on this?

    Comment: in your code here (https://github.com/trailmax/OwinADAuthentication/blob/master/ActiveDirectoryAuthentication/Models/AdAuthenticationService.cs), in the comments you use the word “weather”, but I think you mean “whether”.