feat: Add rate limit to endpoints #1045

jalajcodes · 2021-03-14T12:37:55Z

Description

Added rate limiting to all the endpoints except admin.
I used a dummy rate limit for now. We have to change it before merging this PR.

Fixes #1006

Type of Change:

Code

Code/Quality Assurance Only

New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Tested locally through Swagger UI

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials
I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
Update requirements.txt

Code/Quality Assurance Only

My changes generate no new warnings
New and existing unit tests pass locally with my changes

codecov · 2021-03-14T12:42:58Z

Codecov Report

Merging #1045 (d48fa4f) into develop (92453b7) will increase coverage by 0.12%.
The diff coverage is 97.91%.

@@             Coverage Diff             @@
##           develop    #1045      +/-   ##
===========================================
+ Coverage    92.50%   92.62%   +0.12%     
===========================================
  Files           38       40       +2     
  Lines         2014     2062      +48     
===========================================
+ Hits          1863     1910      +47     
- Misses         151      152       +1

Impacted Files	Coverage Δ
app/rate_limiter.py	`85.71% <85.71%> (ø)`
app/api/resources/mentorship_relation.py	`96.53% <100.00%> (+0.23%)`	⬆️
app/api/resources/task.py	`100.00% <100.00%> (ø)`
app/api/resources/task_comment.py	`100.00% <100.00%> (ø)`
app/api/resources/user.py	`90.32% <100.00%> (+0.66%)`	⬆️
app/rate_limits.py	`100.00% <100.00%> (ø)`
run.py	`96.42% <100.00%> (+0.42%)`	⬆️

mtreacy002

Hi @jalajcodes , I'm new to this rate limiter and want to learn more. Can you please answer my question below? Thanks

mtreacy002 · 2021-03-16T21:37:24Z

app/rate_limits.py

@@ -0,0 +1,3 @@
+GLOBAL_LIMIT = ["200 per day", "50 per hour"]


@jalajcodes , can you please tell me how you decided to use these numbers (200/day, 50/hr)? What is the assumption on the number of users for this one? I'm assuming that the rate limiter does not look on a particular user's request, but for the requests coming in to that endpoint regardless of IP address of the requesters (please do correct me if I'm wrong)? If this is so, what if we have a large number of users (e.g. > 200) that means some of our users won't be able to use the app if the rate limit has been reached, right? This could raise issue 🤔. Can you see if you can apply the rate limiter based on specific IP address? This GitHub discussion indicated that the default which limit per IP address has been deprecated but can still be used providing you explicitly use it in your code.

It is applied based on the ip address afaik.

Hi @mtreacy002,

can you please tell me how you decided to use these numbers (200/day, 50/hr)?

As I mentioned in the description, these numbers are just example values, we need to brainstorm appropriate rate limits before this PR is merged.

that means some of our users won't be able to use the app if the rate limit has been reached, right?

rate limit is based on IP address, so 200 requests per day for an ip addr.

200 is still pretty high for login and signup.

It is applied based on the ip address afaik.

Yes, its done via this line

200 is still pretty high for login and signup.

Thats global limit, we override this in login route

mtreacy002

@jalajcodes can you please response to the question below? Thanks

mtreacy002 · 2021-03-20T01:23:09Z

app/api/resources/mentorship_relation.py

@@ -24,6 +26,8 @@

 @mentorship_relation_ns.route("mentorship_relation/send_request")
 class SendRequest(Resource):
+    decorators = [limiter.limit(rate_limits.LIMIT_1)]


@jalajcodes , I don't see this variable "decorators" being used anywhere in the code. Do we need to declare it as variable? Can we use the pie syntax (@) instead just like we used the @email_verification_required in MS. You can define the limiter decorator function inside the same file (the app/utils/decorator_utils.py).

jalajcodes · 2021-03-20T05:08:46Z

You can define the limiter decorator function inside the same file

That's what I did at first but it doesn't work.

flask-limiter's @limiter decorator doesn't work with class based views.
Only way to make it work is to use the decorator variable approach, here's the relevant documentation
https://flask-limiter.readthedocs.io/en/stable/#using-flask-pluggable-views @mtreacy002

epicadk · 2021-04-09T11:14:12Z

app/api/resources/mentorship_relation.py

@@ -3,6 +3,8 @@
 from flask_jwt_extended import jwt_required, get_jwt_identity


Can you create a doc listing the rates for each endpoint or display the rate limit on the swagger UI?

I will create the doc, but I will need help in deciding the rate limit for each endpoint

You can ask it on zulip itself

vj-codes · 2021-05-05T17:36:13Z

@jalajcodes hey any updates?

vj-codes

@jalajcodes please resolve the merge conflicts

epicadk · 2021-06-08T03:52:44Z

Closing due to inactivity

feat: Add rate limit in endpoints

d48fa4f

devkapilbansal added the Status: Needs Review PR needs an additional review or a maintainer's review. label Mar 14, 2021

mtreacy002 reviewed Mar 16, 2021

View reviewed changes

mtreacy002 reviewed Mar 20, 2021

View reviewed changes

epicadk reviewed Apr 9, 2021

View reviewed changes

epicadk added the Status: Changes Requested Changes are required to be done by the PR author. label Apr 9, 2021

devkapilbansal removed the Status: Needs Review PR needs an additional review or a maintainer's review. label Apr 13, 2021

vj-codes requested changes May 18, 2021

View reviewed changes

epicadk closed this Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add rate limit to endpoints #1045

feat: Add rate limit to endpoints #1045

jalajcodes commented Mar 14, 2021

codecov bot commented Mar 14, 2021

mtreacy002 left a comment

mtreacy002 Mar 16, 2021

epicadk Mar 17, 2021

jalajcodes Mar 17, 2021 •

edited

Loading

epicadk Mar 17, 2021

jalajcodes Mar 17, 2021

jalajcodes Mar 17, 2021

mtreacy002 left a comment

mtreacy002 Mar 20, 2021

jalajcodes commented Mar 20, 2021 •

edited

Loading

epicadk Apr 9, 2021

jalajcodes Apr 10, 2021

devkapilbansal Apr 13, 2021

vj-codes commented May 5, 2021

vj-codes left a comment

epicadk commented Jun 8, 2021

		@@ -0,0 +1,3 @@
		GLOBAL_LIMIT = ["200 per day", "50 per hour"]

		@@ -3,6 +3,8 @@
		from flask_jwt_extended import jwt_required, get_jwt_identity

feat: Add rate limit to endpoints #1045

feat: Add rate limit to endpoints #1045

Conversation

jalajcodes commented Mar 14, 2021

Description

Type of Change:

How Has This Been Tested?

Checklist:

codecov bot commented Mar 14, 2021

Codecov Report

mtreacy002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalajcodes Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalajcodes commented Mar 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vj-codes commented May 5, 2021

vj-codes left a comment

Choose a reason for hiding this comment

epicadk commented Jun 8, 2021

jalajcodes Mar 17, 2021 •

edited

Loading

jalajcodes commented Mar 20, 2021 •

edited

Loading