Bruteforce protector class I'm trying to protect the password login for a website against automated guessing. After too many failed attempts from an IP address, it presents a mandatory CAPTCHA test to the user.

The class is supposed to not forgive the source completely after a successful login attempt, and the more failed attempts the more penalties.

Are there any logic or lock fails here?

public class BruteforceProtector
{
    const int max_log_per_source = 20;
    private ConcurrentDictionary&lt;string, LinkedList&lt;DateTimeOffset&gt;&gt; requests;

    public BruteforceProtector()
    {
        requests = new ConcurrentDictionary&lt;string, LinkedList&lt;DateTimeOffset&gt;&gt;();
    }

    public bool isBlocked(string source, int limit)
    {
        LinkedList&lt;DateTimeOffset&gt; list;
        var haslist = requests.TryGetValue(source, out list);

        if (!haslist)
            return false;

        lock (list)
        {
            removeOld(list);

            return list.Count &gt; limit;
        }
    }

    public void ForgiveSource(string source)
    {
        LinkedList&lt;DateTimeOffset&gt; list;
        if (!requests.TryGetValue(source, out list))
            return;

        lock (list)
        {
            if (list.Count &gt; 0)
                list.RemoveLast();
        }
    }

    public void HandleRequest(string source, int limit, out bool blocked)
    {
        blocked = true;
        bool haslist = true;

        var list = requests.GetOrAdd(source, (x) =&gt;
        {
            var newlist = new LinkedList&lt;DateTimeOffset&gt;();
            newlist.AddLast(DateTimeOffset.Now);
            haslist = false;
            return newlist;
        });

        if (!haslist)
        {
            blocked = false;
            return;
        }
        else
        {
            lock (list)
            {
                list.AddLast(DateTimeOffset.Now);

                removeOld(list);

                if (list.Count &gt; max_log_per_source)
                    list.RemoveFirst();

                blocked = list.Count &gt; limit;
            }
        }
    }

    private void removeOld(LinkedList&lt;DateTimeOffset&gt; list)
    {
        if (list.Count &lt; 1)
            return;

        var oldest = list.First.Value;
        while (oldest.AddHours(list.Count) &lt; DateTimeOffset.Now)
        {
            list.RemoveFirst();

            if (list.Count &gt; 0)
                oldest = list.First.Value;
            else
                break;
        }
    }
}
I'd start with very minor issues.

Usually underscore isn't used in C# names, PascalCase and camelCase are usually preferred (see max_log_per_source). Functions are usually PascalCase (then, for example, isBlocked should be IsBlocked).

Private fields (like requests) do not need any special prefix but you'd better (for clarity) to use a prefix (for example _) or access them like this.requests.

I do not see any extension point in your class then it should be marker sealed.

Fields you initialize only in ctor and that aren't supposed to be changed should be marked readonly: private readonly ConcurrentDictionary&lt;string, LinkedList&lt;DateTimeOffset&gt;&gt; requests;.



In isBlocked() you do not really need haslist local variable. It may be simplified to (you're already doing this in ForgiveSource()):

LinkedList&lt;DateTimeOffset&gt; list;
if (!requests.TryGetValue(source, out list))
    return false;


If you're using C# 7 code might be further simplified:

if (!requests.TryGetValue(source, out var list))
    return false;




ForgiveSource() and HandleRequest() seem to work in concert however I'd change calling logic to avoid to add an entry when login is valid.



This code:

if (condition) {
    // Something
    return;
} else {
    // Something else
}


Can be simplified, you don't need an else here:

if (condition) {
    // Something
    return;
}

// Something else




I find that usually list.Count == 0 is much much clearer than list.Count &lt; 1.



To me a type declared as ConcurrentDictionary&lt;string, LinkedList&lt;DateTimeOffset&gt;&gt; is a missing opportunity to introduce a separate specialized class. More than that you have a thread-safe ConcurrentDictionary and an inner LinkedList which is not. Why don't you introduce a MultiMap&lt;string, DateTimeOffset&gt; type? It's a pretty common data structure to reuse. Also you may wrap it (or extend it) into your own LogInAttemptsCollection (which will be thread-safe). In this way you might separate logic ("list logic" from "is blocked" logic)



Is limit a dynamic parameter which will change for each request? If not then you may consider to move it to a property (or to a readonly field set in ctor).



Why HandleRequest() has an out parameter and a void return type? out parameters should be avoided, when possible, and in this case you just need to change return type to bool.



One note: I feel removeOld() algorithm is way too complex (in general, not for its implementation). Is any reasoning behind this?