5

How would you solve the concurrency issue with the following code? In this example, we'd like to know why the user failed authentication. The problem is that this code makes two separate calls to the database, but we'd like the whole method to happen inside of a conceptual transaction. Specifically we're interested in isolation. We don't want concurrent writes during the execution of this method to affect our reads before we've determined the reason for the authentication failure.

A few solutions come to mind: Thread Locking, TransactionScope and Optimistic Locking. I really like the idea of Optimistic Locking, since I think conflicts are likely to be rare, but there's nothing built into .NET to do this, right?

Also - is this something to REALLY be concerned about in this case? When are concurrency issues like this important to consider and when aren't they? What needs to be considered in implementing a solution? Performance? The duration of the lock? How likely conflicts are to occur?

Edit: After reviewing Aristos' answer, I think what I'm really after is some sort of "snapshot" isolation level for the Authenticate method.

public MembershipStatus Authenticate(string username, string password)
    {
        MembershipUser user = Membership.GetUser(username);
        if (user == null)
        {
            // user did not exist as of Membership.GetUser
            return MembershipStatus.InvalidUsername;
        }

        if (user.IsLockedOut)
        {
            // user was locked out as of Membership.GetUser
            return MembershipStatus.AccountLockedOut;
        }

        if (Membership.ValidateUser(username, password))
        {
            // user was valid as of Membership.ValidateUser
            return MembershipStatus.Valid;
        }

        // user was not valid as of Membership.ValidateUser BUT we don't really
        // know why because we don't have ISOLATION.  The user's status may have changed
        // between the call to Membership.GetUser and Membership.ValidateUser.
        return MembershipStatus.InvalidPassword;
    }
Cœur
  • 37,241
  • 25
  • 195
  • 267
Brandon
  • 1,412
  • 11
  • 15
  • 1
    I am not sure that you really need to worry about isolation here. What are the chances that a user will be deleted or become locked out between the call to `GetUser` and `ValidateUser`? And more to the point, you shouldn't be exposing this information to the user anyway. If you show different messages for "wrong password" vs "user doesn't exist" vs "account locked out", someone attempting to hack your site may leverage that information to get a list of valid usernames. You don't want that, since those usernames may be helpful in exploiting some other vulnerability in your infrastructure. – Chris Shain Feb 14 '12 at 18:55
  • +1 for the response. I probably used a poor example here (although I was planning on distinguishing between "invalid credentials" and "account locked out" and maybe I need to rethink that). I just don't like the fact that data can change underneath me! Concurrency is hard! What if I NEEDED the data to be consistant, what would you suggest? – Brandon Feb 14 '12 at 19:15

2 Answers2

1

I will use mutex using the name as locking parametre, so only the same user may can lock out for awhile. This for me is more safe for one computer because with mutex I can capture all possible threads from different pools, or web calls.

public MembershipStatus AuthenticateLock(string username, string password)
{
    if(string.IsNullOrEmpty(username))
      return MembershipStatus.InvalidUsername;

    // TODO: Here you must check and clear for non valid characters on mutex name
    using (var mutex = new Mutex (false, username))
    {
         // possible lock and wait, more than 16 seconds and the user can go...
         mutex.WaitOne (TimeSpan.FromSeconds(16), false);

         // here I call your function anyway ! and what ever done...
         //  at least I get a result
         return Authenticate(username, password)
    }
}

More comments: Both Membership.ValidateUser and Membership.GetUser make call to the database.

But if you use the standard asp.net session for the pages that make this calls and affect this parameters, then the page all ready lock the one the other so I think that there is no chance to need this mutex call. Because the lock of the session is enough to synchronize and this part. I remind that the session is locking the page from the start to the end for all users.

About session lock: Replacing ASP.Net's session entirely

jQuery Ajax calls to web service seem to be synchronous

Community
  • 1
  • 1
Aristos
  • 66,005
  • 16
  • 114
  • 150
  • +1 for taking the time to answer this question and for the Session information. So you're suggesting thread locking. Thread locking does provide a sort of "soft" isolation, but won't be helpful if you've got multiple processes (not threads) trying to access a shared resource. I think what I'm really after is some sort of "snapshot" isolation level for the Authenticate method. – Brandon Feb 14 '12 at 18:37
  • 1
    @Brandon The mutex lock that I use here is helful for multiple processes !. But I say that you do not need even this, if you use session in all your calls to this function. Session lock all ready the pages, so you do not need second lock. – Aristos Feb 14 '12 at 20:58
  • Interesting! I did not know that "Named system mutexes are visible throughout the operating system, and can be used to synchronize the activities of processes." So I guess the boundary is the PC/OS, e.g. you can't synchronize processes with a mutex across multiple PCs/OSs? – Brandon Feb 14 '12 at 23:19
1

Based on my reading here and here, it seems like a System.Transactions.TransactionScope wrapping your whole method should automatically enlist your database calls in a common transaction, resulting in transactional safety across the whole Transaction scope.

You'd want to do something like this:

public MembershipStatus Authenticate(string username, string password)
{
    using (TransactionScope scope = new TransactionScope(TransactionScopeOption.Required, new TransactionOptions { IsolationLevel = IsolationLevel.Snapshot }))
    {
    MembershipUser user = Membership.GetUser(username);
        if (user == null)
        {
            // user did not exist as of Membership.GetUser
            return MembershipStatus.InvalidUsername;
        }

        if (user.IsLockedOut)
        {
            // user was locked out as of Membership.GetUser
            return MembershipStatus.AccountLockedOut;
        }

        if (Membership.ValidateUser(username, password))
        {
            // user was valid as of Membership.ValidateUser
            return MembershipStatus.Valid;
        }

        // user was not valid as of Membership.ValidateUser BUT we don't really
        // know why because we don't have ISOLATION.  The user's status may have changed
        // between the call to Membership.GetUser and Membership.ValidateUser.
        return MembershipStatus.InvalidPassword;
    }
}
Chris Shain
  • 50,833
  • 6
  • 93
  • 125
  • +1 for taking the time to respond. Why would I need to implement my own membership provider? Couldn't I just use the TransactionScope class "in which transactions are automatically managed by the infrastructure"? – Brandon Feb 14 '12 at 23:14
  • @Brandon sorry it took so long to reply- I wanted to do some research before answering this. I've updated my answer to reflect the use of TransactionScope. – Chris Shain Feb 17 '12 at 01:03
  • thanks for the response. I'm marking this as the accepted answer, mostly for your first comment to my question (I probably shouldn't be doing this to begin with and even if I wanted to it's really not an issue). – Brandon Feb 22 '12 at 17:24