animir
Consider applying inMemoryBlockOnConsumed to RateLimiter.get(key) as well as consume().
The example given for login protection at https://github.com/animir/node-rate-limiter-flexible/wiki/Overall-example#login-endpoint-protection does seem to require using the get(key) calls at the start, as it should probably not consume on the limiters until the the login attempt has failed (and users existence has been verified for user/ip combo).
However this means that using inMemoryBlockOnConsumed does not actually help reduce database traffic due to the need for the RateLimiter.get(key) call, which doesn't check the in memory version like consume does. => What is worse, it causes the consumedPoints checks that depend on the get(key) call at the start (prior to trying to login the user) to never trigger properly (unless inMemoryBlockOnConsumed is larger than points) because it blocks on consume in memory only and never writes the failing point deduction to the database; but the database is what is being checked with the get(key) call that can then therefore never fail.
Maybe there is a way to refactor this example to make use of the inMemoryBlockOnConsumed optimization/protection, but at present I don't see a nice way to do that. Additionally since even trying to use the inMemoryBlockOnConsumed with the example seems to break the use case, it seems that the get(key) should probably use inMemoryBlockOnConsumed in addition to consume.
I assume that there is a reason inMemoryBlockOnConsumed is presently ONLY used in consume(), but it seems like if there was another place that it makes sense to use/check it, it would be for local read only (ie the get(key) call).
I guess an alternative could be to have an additional call, like isBlockedInMemoryForConsume(key) which could be checked/called in the example/use case before the more expensive (more DDoS risky) database requesting get() calls. Might that be better?
Still thinking about this issue, I had another thought which I thought I would bounce here for the possibility of feedback.
Maybe the login point protection shouldn't have the get(key) manual checks at the start, and just do the consume from the outset with inMemoryBlockOnConsumed set, and after a successful logins it would then delete the keys from all limiters instead of just the one for limiterConsecutiveFailsByUsernameAndIP. It looking at the source code, it does look like delete clears the key for the in memory storage created by inMemoryBlockOnConsumed.
Is there a reason the example is written as is though that would make this hypothetical rewrite into a drawback I don't see atm?
@jeremykentbgross Hi, thank you for sharing your thoughts.
inMemoryBlockOnConsumed was added to avoid unnecessary and potentially malicious upsert requests to databases. It is quite expensive call when value should be atomically incremented. get requests are not blocking operation on all databases and usually even small database may handle millions of get requests. Also, with get I would like to see the real number of points consumed not from memory.
unless inMemoryBlockOnConsumed is larger than points
It should be larger than points. Otherwise a limiter wouldn't count points correctly. I added this to docs. Thanks.
Maybe the login point protection shouldn't have the get(key) manual checks at the start
Good catch. Yes, the example could be rewritten to make two consume calls but there are a couple of drawbacks. First, consume calls are expensive. Imagine, somebody DDoSes the login endpoint and a database got millions of upsert requests. Second, if there is a consume call by random username allowed it can be used to overflow the storage with junk keys.
Thanks for your feedback. I didn't see the reply from last week at the time, as I only seem to have gotten the notifications in my inbox when the ticket was closed. I think I understand what you are saying though.
If I understand correctly, inMemoryBlockOnConsumed isn't useful in this use case because the read only checks at the beginning always need to check the DB (which is way way cheaper than consume because it lacks upsert), and given that check, it will fail before any inMemoryBlockOnConsumed protection would kick in during the consume call anyway, and thus be of no additional help here. Is my understanding correct?