search

anitab-org / mentorship-backend

Mentorship System is an application that matches women in tech to mentor each other, on career development, through 1:1 relations during a certain period of time. This is the backend of this system.
https://mentorship-backend-temp.herokuapp.com/
GNU General Public License v3.0
196 stars 449 forks source link

Feature: Setting a rate limit on the endpoints using flask limiter #1006

Open epicadk opened 3 years ago

epicadk commented 3 years ago

Is your feature request related to a problem? Please describe.

Heroku Does offer DDoS mitigation however, according to this post they strongly recommend using a rate limiter library.

Describe the solution you'd like

Rate limit the endpoints of the api using flask limiter.

Additional context

The time and amount of requests that should be allowed should be discussed, possibly in the mentorship session.

epicadk commented 3 years ago

Marking this as status on hold because it needs to be approved by @isabelcosta .

isabelcosta commented 3 years ago

@epicadk this sounds amazing. This is the kind of thing I only learned this year, so I really appreciate you coming with these "out of the box" ideas 🤗 I will approve this. Could you link the library in the description of the issue and add a little more information if you have more.

epicadk commented 3 years ago

@isabelcosta 😅 Thankyou : ) . I have linked the library in the issue however I haven't used it much so I don't really have any more information about it. We also need to discuss the limit or the amount of times that a user should be allowed to query the backend.

isabelcosta commented 3 years ago

thank you so much @epicadk !

jalajcodes commented 3 years ago

I'd like to work on it I'll be in the next open session to discuss about this issue.

epicadk commented 3 years ago

I'd like to work on it I'll be in the next open session to discuss about this issue.

I think we already discussed it. Since Isabel has approved I'll assign you. Happy coding : ) .

jalajcodes commented 3 years ago

I'd like to work on it I'll be in the next open session to discuss about this issue.

I think we already discussed it. Since Isabel has approved I'll assign you. Happy coding : ) .

Ohk! Do you mind sharing the meeting docs link? I'd like to go through the discussion.

epicadk commented 3 years ago

I'd like to work on it I'll be in the next open session to discuss about this issue.

I think we already discussed it. Since Isabel has approved I'll assign you. Happy coding : ) .

Ohk! Do you mind sharing the meeting docs link? I'd like to go through the discussion.

I went through the docs and didn't find any thing related to this issue so maybe I was mistaken. But isabel has apporved it so you can work on it. What we need to discuss is the rates for each endpoint. You can still work on it and submit a pr. Although you might have to change only the rates later on so up to you.

jalajcodes commented 3 years ago

I'd like to work on it I'll be in the next open session to discuss about this issue.

I think we already discussed it. Since Isabel has approved I'll assign you. Happy coding : ) .

Ohk! Do you mind sharing the meeting docs link? I'd like to go through the discussion.

I went through the docs and didn't find any thing related to this issue so maybe I was mistaken. But isabel has apporved it so you can work on it. What we need to discuss is the rates for each endpoint. You can still work on it and submit a pr. Although you might have to change only the rates later on so up to you.

Ok cool I'll start working on it then ;)

vj-codes commented 3 years ago

@jalajcodes hey any updates?

epicadk commented 3 years ago

@mtreacy002 I think this issue can cause problems for BIT. we'll have to exclude the BIT server from the rate limiting.

jalajcodes commented 3 years ago

@jalajcodes hey any updates?

I'll create PR tomorrow or if time permits, today

vj-codes commented 3 years ago

@jalajcodes take your time, just update the progress after every 3 days if it's taking long

mtreacy002 commented 3 years ago

@mtreacy002 I think this issue can cause problems for BIT. we'll have to exclude the BIT server from the rate limiting.

@epicadk , I agree, this could cause issue if the rate limiter sets the amount of requests BIT can do, since in BIT-MS integration the number of requests sent would be extensive 🤣🤣. If possible, @jalajcodes, please share the approach you're thinking of doing (e.g. what type of limiter you will apply here), so we can assess the impact to BIT better. Perhaps we should add CROSS-PORJECT ISSUE on the title so I can monitor the progress made here as it would impact BIT. cc @vj-codes and @isabelcosta

jalajcodes commented 3 years ago

If possible, @jalajcodes, please share the approach you're thinking of doing (e.g. what type of limiter you will apply here), so we can assess the impact to BIT better.

As we haven't decided rates for specific endpoints yet, I am thinking of adding a generous rate limit globally for all endpoints and exempt it for those endpoints which don't need to be rate limited like login etc.

regarding the BIT-MS integration, I think flask-limiter allows us to customize rate limits to be based on characteristics of the incoming request. I am not sure but I guess we could add some kind of check to determine if the request is coming from BIT and increase the limit or maybe disable it completely?

epicadk commented 3 years ago

If possible, @jalajcodes, please share the approach you're thinking of doing (e.g. what type of limiter you will apply here), so we can assess the impact to BIT better.

As we haven't decided rates for specific endpoints yet, I am thinking of adding a generous rate limit globally for all endpoints and exempt it for those endpoints which don't need to be rate limited like login etc.

regarding the BIT-MS integration, I think flask-limiter allows us to customize rate limits to be based on characteristics of the incoming request. I am not sure but I guess we could add some kind of check to determine if the request is coming from BIT and increase the limit or maybe disable it completely?

Login is probably the endpoint we want to rate limit first. 😄 .

jalajcodes commented 3 years ago

@epicadk 😂😅 my bad, just assume any other endpoint that doesn't require limiting

Anmollenka commented 3 years ago

I would like to take up this issue if it's available.

jalajcodes commented 3 years ago

I'll create PR today

battuAshita commented 3 years ago

Hi @epicadk and @isabelcosta. Please assign this issue to me if it is open :)

isabelcosta commented 3 years ago

@battuAshita are you still interested in the issue?

battuAshita commented 3 years ago

Hi @isabelcosta. I am actually caught up in some work. It is free to be assigned to someone else. Thank you :)

isabelcosta commented 3 years ago

No worries @battuAshita ! thank you for responding so quickly 🤗 in this way we can assign to someone else who is interested!

sakshivij commented 3 years ago

Hi @isabelcosta. Is this open to work on? If yes, can you please assign this to me? Thanks!

vj-codes commented 3 years ago

Assigning you @sakshivij Happy coding!

sakshivij commented 3 years ago

@vj-codes to decide on rate-limit was there any doc created as mentioned on the closed PR? Or I would need to create one?

vj-codes commented 3 years ago

@epicadk can you answer that please? ⬆️

epicadk commented 3 years ago

@epicadk can you answer that please? ⬆️

That's something we need to discuss in the mentorship session. I think you could probably use a constant right now and then change it later? cc @isabelcosta

sakshivij commented 3 years ago

@epicadk can you answer that please? ⬆️

That's something we need to discuss in the mentorship session. I think you could probably use a constant right now and then change it later? cc @isabelcosta

Makes sense. If you mean the sync up session, I'll try to join as well.

vj-codes commented 3 years ago

Yes the sync up session, it is conducted on Saturdays biweekly at 6:30 pm IST The next one scheduled is this Saturday