蛮力保护者等级
我试图保护一个网站的密码登录免受自动猜测。在多次尝试IP地址失败后，它向用户提供了强制性的CAPTCHA测试。
类在成功登录后不应该完全原谅源，失败次数越多，惩罚越重。
这里有任何逻辑或锁失败吗？
public class BruteforceProtector
{
const int max_log_per_source = 20;
private ConcurrentDictionary<string, LinkedList<DateTimeOffset>> requests;
public BruteforceProtector()
{
requests = new ConcurrentDictionary<string, LinkedList<DateTimeOffset>>();
}
public bool isBlocked(string source, int limit)
{
LinkedList<DateTimeOffset> list;
var haslist = requests.TryGetValue(source, out list);
if (!haslist)
return false;
lock (list)
{
removeOld(list);
return list.Count > limit;
}
}
public void ForgiveSource(string source)
{
LinkedList<DateTimeOffset> list;
if (!requests.TryGetValue(source, out list))
return;
lock (list)
{
if (list.Count > 0)
list.RemoveLast();
}
}
public void HandleRequest(string source, int limit, out bool blocked)
{
blocked = true;
bool haslist = true;
var list = requests.GetOrAdd(source, (x) =>
{
var newlist = new LinkedList<DateTimeOffset>();
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 > max_log_per_source)
list.RemoveFirst();
blocked = list.Count > limit;
}
}
}
private void removeOld(LinkedList<DateTimeOffset> list)
{
if (list.Count < 1)
return;
var oldest = list.First.Value;
while (oldest.AddHours(list.Count) < DateTimeOffset.Now)
{
list.RemoveFirst();
if (list.Count > 0)
oldest = list.First.Value;
else
break;
}
}
}回答 1
Code Review用户
发布于 2017-05-15 10:10:04
我先从一些很小的问题开始。
通常在C#名称中不使用下划线，PascalCase和camelCase通常是首选(参见max_log_per_source)。函数通常是PascalCase (例如，isBlocked应该是IsBlocked)。
私有字段(如requests)不需要任何特殊的前缀，但是为了清晰起见，最好使用前缀(例如_)或者像this.requests那样访问它们。
我在您的类中没有看到任何扩展点，那么它应该是标记sealed。
仅在ctor中初始化且不应更改的字段应标记为readonly：private readonly ConcurrentDictionary<string, LinkedList<DateTimeOffset>> requests;。
在isBlocked()中，并不真正需要haslist局部变量。它可以简化为(您已经在ForgiveSource()中这样做了)：
LinkedList<DateTimeOffset> list;
if (!requests.TryGetValue(source, out list))
return false;如果您使用的是C# 7代码，则可能会进一步简化：
if (!requests.TryGetValue(source, out var list))
return false;ForgiveSource()和HandleRequest()似乎协调工作，但是我会更改调用逻辑，以避免在登录有效时添加条目。
此代码：
if (condition) {
// Something
return;
} else {
// Something else
}可以简化，这里不需要else：
if (condition) {
// Something
return;
}
// Something else我发现通常list.Count == 0比list.Count < 1要清晰得多。
对我来说，一种被声明为ConcurrentDictionary<string, LinkedList<DateTimeOffset>>的类型错过了引入一个单独的专门类的机会。更重要的是，您有一个线程安全的ConcurrentDictionary和一个内部的LinkedList，这不是。为什么不介绍一种MultiMap<string, DateTimeOffset>类型呢？这是一种非常常见的数据结构，可以重用。此外，您也可以将其包装(或扩展)到您自己的LogInAttemptsCollection中(这将是线程安全的)。通过这种方式，您可以将逻辑(“列表逻辑”和“被阻塞的”逻辑)分开。
limit是否是一个动态参数，它将对每个请求进行更改？如果没有，您可以考虑将其移动到属性(或在ctor中设置的readonly字段)。
为什么HandleRequest()有一个out参数和一个空返回类型?在可能的情况下，应该避免使用out参数，在这种情况下，只需要将返回类型更改为bool。
注意:我觉得removeOld()算法太复杂了(一般来说，它的实现不是这样的)。这背后有什么道理吗？
https://codereview.stackexchange.com/questions/163350
复制相似问题
腾讯云开发者
