Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jotaviobiondo opened this issue May 20, 2022 · 2 comments

Comments

@jotaviobiondo
Copy link
Member

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?

@gabrielpra1
Copy link
Member

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

@jotaviobiondo
Copy link
Member Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants