rajska icon indicating copy to clipboard operation
rajska copied to clipboard

Proposal: add function to reset a rate limit set by the Rajska.RateLimiter

Open jotaviobiondo opened this issue 2 years ago • 2 comments

Sometimes is useful to have a way to reset the RateLimiter, e.g. rate limiting the login and be able to clear the rate limit when user resets his password.

For this we can add a function reset(identifier) to the Rajska.RateLimiter module that calls the https://hexdocs.pm/hammer/Hammer.html#delete_buckets/2.

Besides this low level function, we could also have a middleware for this.

Also, another suggestion is to improve the RateLimiter identifier config. Today we have 3 ways of specifying a identifier:

  1. specifying an id option
  2. specifying a keys option
  3. not specifying anything and fallbacks to the IP

It would be nice, given the idea to have a middleware for limiting and reseting, do something like this:

field :login, :session do
  ...
  middleware Rajska.RateLimiter, namespace: :login, limit_by: :ip
  resolve &AccountsResolver.login/2
end

field :reset_password, :session do
  ...
  resolve &AccountsResolver.reset_password/2
  middleware Rajska.RateLimiter.Reset, namespace: :login, limit_by: :ip
end

With this, we are identifying by both an namespace and user IP. I see this useful for readability: we use a namespace to better uderstand that we are limiting in one mutation and reseting in another. Without this, it's still possible to implement, but it's harder to see why that reset is there:

field :login, :session do
  ...
  middleware Rajska.RateLimiter
  resolve &AccountsResolver.login/2
end

field :reset_password, :session do
  ...
  resolve &AccountsResolver.reset_password/2
  middleware Rajska.RateLimiter.Reset
end

The limit_by could be:

  • :ip -> uses user IP
  • {:keys, list} -> same way keys option today works
  • {:id, any_fixed_id} -> same way id option works

The Hammer id built with this, would be something like "query:#{namespace}:#{limit_by}"

What do you guys think?

jotaviobiondo avatar May 20 '22 16:05 jotaviobiondo

Very nice proposal! I would break this into 2 tasks, though.

One to implement the Reset middleware without any breaking changes in the API, and later this general refactoring, which we could wait to bundle with other breaking changes before releasing a new major version.

Another option would be to have the namespace as the query/mutation name by default. That way, we could already implement the concept of namespaces without a breaking change

gabrielpra1 avatar May 23 '22 02:05 gabrielpra1

Yeah, I thought about making 2 PR's for this too. But I also was thinking implementing this new options without breaking changes. The approach would be supporting the old options (informing that they are deprecated) and the new options.

For the namespace I was thinking that if it's not informed, the identification would be something like "query:#{limit_by}" (skipping the namespace concatenation). But your idea of using the name of the query/mutation as default is nice :)

jotaviobiondo avatar May 23 '22 13:05 jotaviobiondo