MemoryCache - Login Security

I have a WCF service which should authenticate user against any bruteforce attack.
Below is the code to avoid any bruteforce attack. I have used MemoryCache to keep track of invalid attempts by a user.

Could you please review the code and let me know if there are any issues?

Following defined in config file:

Code to add invalid login to cache and lock the account if invalid logins reaches threshold set in config.

```
private readonly CacheItemPolicy cacheInvalidLoginPolicy = new CacheItemPolicy();
private readonly ObjectCache cacheInvalidLogins = MemoryCache.Default; //Cache to hold Invalid Login Attempts: {UserId, obj UserLogin}

public class UserLogin
{
public string UserName { get; set; }
public List InvalidLoginTimestamp { get; set; }
public bool IsLocked { get; set; }
}
private bool AuthenticateUser(string userName, string password)
{
bool validCredentials = false;
string userNameUpperCase = userName.ToUpper(); //MemoryCache treats 'a' and 'A' as different names
if (cacheInvalidLogins.Contains(userNameUpperCase) && ((UserLogin)cacheInvalidLogins[userNameUpperCase]).IsLocked)
{
log.ErrorFormat("UserId: {0} tried to access locked account at {1}", userName, DateTime.Now);
throw new FaultException("Your account is locked out. Please try after some time.", new FaultCode("AccountLocked"));
}

try
{
validCredentials = securityService.Authenticate(userName, password);

if (!validCredentials)
{
AddInvalidLoginToCache(userNameUpperCase);
}
else if (validCredentials && cacheInvalidLogins.Contains(userNameUpperCase))
{
//For valid credentials - remove from cache
cacheInvalidLogins.Remove(userNameUpperCase);
}
}
catch (Exception exception)
{
log.Error(exception);
}

return validCredentials;
}

private void AddInvalidLoginToCache(string userName)
{
if (cacheInvalidLogins.Contains(u

-
If I'm right it's not thread-safe. I suppose two browsers with the same username could login at the same time which means that the AuthenticateUser method runs parallel on two threads.

Suppose the following:

• the user already has an invalid login attempt,

• two new threads enter AddInvalidLoginToCache the same time,

• invalidLoginThresholdFromConfig = 2.

A possible execution order is the following:

// InvalidLoginTimestamp.Count = 1, we already have an invalid login attempt
thread1: enters AddInvalidLoginToCache
thread2: enters AddInvalidLoginToCache
thread1: runs invalidLogin.InvalidLoginTimestamp.Add(DateTime.Now); // InvalidLoginTimestamp.Count = 2
thread2: runs invalidLogin.InvalidLoginTimestamp.Add(DateTime.Now); // InvalidLoginTimestamp.Count = 3
thread1: runs if (invalidLogin.InvalidLoginTimestamp.Count == 2)) // false 
thread2: runs if (invalidLogin.InvalidLoginTimestamp.Count == 2)) // false

The result is that a lucky attacker will never be locked out.

The second problem is visibility. Since InvalidLoginTimestamp and invalidLogin are shared between threads you need some synchronization to ensure other threads will see modifications (not just stale data or invalid state).

• C# multi-threading: Acquire read lock necessary?

• How to synchronize the access to a List used in ASP.NET?

-

private readonly ObjectCache cacheInvalidLogins = MemoryCache.Default; //Cache to hold Invalid Login Attempts: {UserId, obj UserLogin}

I'd put the comment before the line to avoid horizontal scrolling:

//Cache to hold Invalid Login Attempts: {UserId, obj UserLogin}
private readonly ObjectCache cacheInvalidLogins = MemoryCache.Default;

Furthermore, I'd rename it to invalidLoginAttemptsCache. The cacheInvalidLogins name looks like a method name since it starts with a verb (cache...).

-

bool validCredentials = false;
... // other statements
try
{
    validCredentials = securityService.Authenticate(userName, password);

The boolean could be declared right before the first use with a smaller scope:

...  // other statements
bool validCredentials = false;
try
{
    validCredentials = securityService.Authenticate(userName, password);

Should I declare variables as close as possible to the scope where they will be used?

-

private void AddInvalidLoginToCache(string userName)
{
    ...
    cacheInvalidLogins.Set(userName.ToUpper(), invalidLogin, LockOutCachePolicyFromConfig());

The code calls this method with the uppercased username. I'd rename the parameter to upperCasedUserName and remove the .ToUpper() call.

-
This variable is never user, I guess you could remove it:

string clientId = userName.Split('\\')[0];

-
Username seems to be one word, I'd not capitalize the n.

StackExchange Code Review Q#44496, answer score: 4