Synchronize code on login method

Question

I have the following snippet of Java code in my website

public boolean login(String username, string password){
    if(isUserLocked(username)){
        return false;
    }
    if(isPasswordCorrect(username, password)){
        return true;
    }
    else{
        increaseFailedAttempts(username);
        if(getFailedAttempts(username) > MAXIMUM_FAILED_ATTEMPTS)
        {
            lockUser(username);
        }

        return false;
    }
}

Unfortunately, this code can be hacked by using a tool that sends hundreds of requests at the same time. A hacker could brute force guess hundreds of user/password combinations before the database call that locks the user is succesfully executed. What I face is a synchronous request problem. The naive thing would be to synchronize on the login method. Sure, that prevents any duplicate request from being executed, but it is also slows down the application to an unacceptable rate for our business. What are some good practies to synchronize on instead?

The following methods are relevant in locking the user, and they need to work in tandem correctly.

• isUserLocked(): goes to db to check if the field "locked" has been set.
• increaseFailedAttempts(): goes to db to do +1 in the attempts field
• getFailedAttempts(): reads the db to get the value of the attempts field
• lockUser(): sets the "locked" flag in the db for the user.

Answer 1

Why ever allow for more than one simultaneous login attempt?

// resizing is expensive, try to estimate the right size up front
private Map<String, Boolean> attempts = new ConcurrentHashMap<>(1024);

public void login(String username, String password) {
  // putIfAbsent returns previous value or null if there was no mapping for the key
  // not null => login for the username in progress
  // null => new user, proceed
  if (attempts.putIfAbsent(username, Boolean.TRUE) != null) {
    throw new RuntimeException("Login attempt in progress, request should be  discarded");
  }
  try {
    // this part remains unchanged
    // if the user locked, return false
    // if the password ok, reset failed attempts, return true
    // otherwise increase failed attempts
    //    if too many failed attempts, lock the user
    // return false
  } finally {
    attempts.remove(username);
  }
}

ConcurrentHashMap doesn't require additional synchronization, the operation used above are atomic.

Of course in order to speed up isUserLocked you could cache the lock state in a HashMap or maybe HTTP request – it has to be carefully implemented though.

In memory cache alone is not an option – what if a legitimate user locks himself out, calls support line to be unlocked, the unlock status gets removed from the database yet the user still cannot login because of the in memory cache?

So the content of cache should be synchronized with the database state once in a while using a background thread.

Answer 2

Synchronizing the presented login() method is a bit heavy-handed, because that serializes access for all login requests. It seems that it would be sufficient to serialize requests on a per-user basis. Additionally, your method is somewhat of a soft target because it makes more round trips to the DB than it needs to do. Even a single one is fairly costly -- this is likely why synchronizing the method extracts such a heavy toll.

I suggest

1. Tracking the users for which login requests are being processed at any given time, and serializing those on a per-user basis.

2. Improving the overall behavior of login() by minimizing the number of DB round-trips to at most two -- one to read all needed current data for the specified user and one to update it. You might consider even caching these data, which you could get for nearly free if you were using JPA to access your user data.

With respect to (1), here's one way you might serialize logins on a per-username basis:

public class UserLoginSerializer {
    private Map<String, Counter> pendingCounts = new HashMap<>();

    public boolean login(String username, String password) {
        Counter numPending;
        boolean result;

        synchronized (pendingCounts) {
            numPending = pendingCounts.get(username);
            if (numPending == null) {
                numPending = new Counter(1);
                pendingCounts.put(username, numPending);
            } else {
                numPending.increment();
            }
        }

        try {
            // username-scoped synchronization:
            synchronized (numPending) {
                result = doLogin(username, password);
            }
        } finally {
            synchronized (pendingCounts) {
                if (numPending.decrement() <= 0) {
                    pendingCounts.remove(username);
                }
            }
        }

        return result;
    }

    /** performs the actual login check */
    private boolean doLogin(String username, String password) {
        // ...
    }
}

class Counter {
    private int value;

    public Counter(int i) {
        value = i;
    }

    /** increments this counter and returns the new value */
    public int increment() {
        return ++value;
    }

    /** decrements this counter and returns the new value */
    public int decrement() {
        return --value;
    }
}

Every thread synchronizes on the pendingCounts map, but only long enough to obtain and / or update a username-specific object at the beginning, and to update and possibly remove that object at the end. This will slightly delay concurrent logins, but not nearly so much as if the critical regions performed database accesses. In between, each thread synchronizes on an object associated with the requested username. This serializes login attempts for the same username, but allows logins for different usernames to proceed in parallel. Obviously, all logins would need to go through the same instance of that class.