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.

Sign up for GitHub

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

feat: Add rate limit to endpoints #1045

Closed
wants to merge 1 commit into from
Closed

feat: Add rate limit to endpoints #1045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions app/api/resources/mentorship_relation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
from flask_jwt_extended import jwt_required, get_jwt_identity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can ask it on zulip itself

from http import HTTPStatus

from app import rate_limits
from app.rate_limiter import limiter
from app import messages
from app.api.resources.common import auth_header_parser
from app.api.dao.mentorship_relation import MentorshipRelationDAO
Expand All @@ -24,6 +26,8 @@

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).


@classmethod
@jwt_required
@mentorship_relation_ns.doc("send_request")
Expand Down Expand Up @@ -121,6 +125,8 @@ def is_valid_data(data):

@mentorship_relation_ns.route("mentorship_relations")
class GetAllMyMentorshipRelation(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@mentorship_relation_ns.doc("get_all_user_mentorship_relations")
Expand Down Expand Up @@ -173,6 +179,8 @@ def get(cls):

@mentorship_relation_ns.route("mentorship_relation/<int:request_id>/accept")
class AcceptMentorshipRelation(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@mentorship_relation_ns.doc("accept_mentorship_relation")
Expand Down Expand Up @@ -222,6 +230,8 @@ def put(cls, request_id):

@mentorship_relation_ns.route("mentorship_relation/<int:request_id>/reject")
class RejectMentorshipRelation(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@mentorship_relation_ns.doc("reject_mentorship_relation")
Expand Down Expand Up @@ -266,6 +276,8 @@ def put(cls, request_id):

@mentorship_relation_ns.route("mentorship_relation/<int:request_id>/cancel")
class CancelMentorshipRelation(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@mentorship_relation_ns.doc("cancel_mentorship_relation")
Expand Down Expand Up @@ -309,6 +321,8 @@ def put(cls, request_id):

@mentorship_relation_ns.route("mentorship_relation/<int:request_id>")
class DeleteMentorshipRelation(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@mentorship_relation_ns.doc("delete_mentorship_relation")
Expand Down Expand Up @@ -352,6 +366,8 @@ def delete(cls, request_id):

@mentorship_relation_ns.route("mentorship_relations/past")
class ListPastMentorshipRelations(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@mentorship_relation_ns.doc("get_past_mentorship_relations")
Expand Down Expand Up @@ -391,6 +407,8 @@ def get(cls):

@mentorship_relation_ns.route("mentorship_relations/current")
class ListCurrentMentorshipRelation(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@mentorship_relation_ns.doc("get_current_mentorship_relation")
Expand Down Expand Up @@ -431,6 +449,8 @@ def get(cls):

@mentorship_relation_ns.route("mentorship_relations/pending")
class ListPendingMentorshipRequests(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@mentorship_relation_ns.doc("get_pending_mentorship_relations")
Expand Down
10 changes: 10 additions & 0 deletions app/api/resources/task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
from flask_jwt_extended import jwt_required, get_jwt_identity
from http import HTTPStatus

from app import rate_limits
from app.rate_limiter import limiter
from app import messages
from app.api.dao.task import TaskDAO
from app.api.resources.common import auth_header_parser
Expand All @@ -18,6 +20,8 @@

@task_ns.route("mentorship_relation/<int:request_id>/task")
class CreateTask(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@task_ns.doc("create_task_in_mentorship_relation")
Expand Down Expand Up @@ -74,6 +78,8 @@ def is_valid_data(data):

@task_ns.route("mentorship_relation/<int:request_id>/task/<int:task_id>")
class DeleteTask(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@task_ns.doc("delete_task_in_mentorship_relation")
Expand Down Expand Up @@ -118,6 +124,8 @@ def delete(cls, request_id, task_id):

@task_ns.route("mentorship_relation/<int:request_id>/tasks")
class ListTasks(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@task_ns.doc("list_tasks_in_mentorship_relation")
Expand Down Expand Up @@ -166,6 +174,8 @@ def get(cls, request_id):

@task_ns.route("mentorship_relation/<int:request_id>/task/<int:task_id>/complete")
class UpdateTask(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@task_ns.doc("update_task_in_mentorship_relation")
Expand Down
8 changes: 8 additions & 0 deletions app/api/resources/task_comment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
from flask_restx import Resource, Namespace, marshal
from flask_jwt_extended import jwt_required, get_jwt_identity
from http import HTTPStatus
from app import rate_limits
from app.rate_limiter import limiter
from app import messages
from app.api.resources.common import auth_header_parser

Expand All @@ -20,6 +22,8 @@
"mentorship_relation/<int:relation_id>/task/<int:task_id>/comment"
)
class CreateTaskComment(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@task_comment_ns.expect(auth_header_parser, task_comment_model)
Expand Down Expand Up @@ -60,6 +64,8 @@ def post(cls, relation_id, task_id):
"mentorship_relation/<int:relation_id>/task/<int:task_id>/comment/<int:comment_id>"
)
class TaskComment(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@task_comment_ns.expect(auth_header_parser, task_comment_model)
Expand Down Expand Up @@ -131,6 +137,8 @@ def delete(cls, relation_id, task_id, comment_id):
"mentorship_relation/<int:relation_id>/task/<int:task_id>/comments/"
)
class TaskComments(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@task_comment_ns.expect(auth_header_parser)
Expand Down
26 changes: 26 additions & 0 deletions app/api/resources/user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
)
from flask_restx import Resource, marshal, Namespace

from app.rate_limiter import limiter
from app import rate_limits
from app import messages
from app.api.validations.user import *
from app.api.email_utils import send_email_verification_message
Expand All @@ -35,6 +37,8 @@
)
# TODO: @users_ns.response(404, 'User does not exist.')
class UserList(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@users_ns.doc(
Expand Down Expand Up @@ -79,6 +83,8 @@ def get(cls):
@users_ns.route("users/<int:user_id>")
@users_ns.param("user_id", "The user identifier")
class OtherUser(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@users_ns.doc("get_user")
Expand Down Expand Up @@ -120,6 +126,8 @@ def get(cls, user_id):
)
@users_ns.response(HTTPStatus.NOT_FOUND.value, "%s" % messages.USER_DOES_NOT_EXIST)
class MyUserProfile(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@users_ns.doc("get_user")
Expand Down Expand Up @@ -208,6 +216,8 @@ def delete(cls):
)
@users_ns.route("user/change_password")
class ChangeUserPassword(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@users_ns.doc("update_user_password")
Expand Down Expand Up @@ -241,6 +251,8 @@ def put(cls):
)
@users_ns.route("users/verified")
class VerifiedUser(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@users_ns.doc(
Expand Down Expand Up @@ -286,6 +298,8 @@ def get(cls):

@users_ns.route("register")
class UserRegister(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@users_ns.doc("create_user")
@users_ns.response(
Expand Down Expand Up @@ -355,6 +369,8 @@ def post(cls):
)
@users_ns.param("token", "Token sent to the user's email")
class UserEmailConfirmation(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
def get(cls, token):
"""Confirms the user's account.
Expand All @@ -378,6 +394,8 @@ def get(cls, token):
HTTPStatus.NOT_FOUND.value, "%s" % messages.USER_IS_NOT_REGISTERED_IN_THE_SYSTEM
)
class UserResendEmailConfirmation(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@users_ns.expect(resend_email_request_body_model)
def post(cls):
Expand Down Expand Up @@ -409,6 +427,8 @@ def post(cls):

@users_ns.route("refresh")
class RefreshUser(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_refresh_token_required
@users_ns.doc("refresh")
Expand Down Expand Up @@ -442,6 +462,8 @@ def post(cls):

@users_ns.route("login")
class LoginUser(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@users_ns.doc("login")
@users_ns.response(
Expand Down Expand Up @@ -518,6 +540,8 @@ def post(cls):
)
@users_ns.response(HTTPStatus.NOT_FOUND.value, "%s" % messages.USER_NOT_FOUND)
class UserHomeStatistics(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@users_ns.expect(auth_header_parser)
Expand All @@ -543,6 +567,8 @@ def get(cls):
)
@users_ns.response(HTTPStatus.NOT_FOUND.value, "User not found")
class UserDashboard(Resource):
decorators = [limiter.limit(rate_limits.LIMIT_1)]

@classmethod
@jwt_required
@users_ns.expect(auth_header_parser)
Expand Down
11 changes: 11 additions & 0 deletions app/rate_limiter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from flask_limiter import Limiter
from flask_limiter.util import get_remote_address
from flask import jsonify, make_response, request

from app.rate_limits import GLOBAL_LIMIT

limiter = Limiter(key_func=get_remote_address, default_limits=GLOBAL_LIMIT)


def rate_limit_exceeded(e):
return make_response(jsonify(error=f"rate limit exceeded: {e.description}"), 429)
3 changes: 3 additions & 0 deletions app/rate_limits.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
GLOBAL_LIMIT = ["200 per day", "50 per hour"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is applied based on the ip address afaik.

Copy link
Member Author

@jalajcodes jalajcodes Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

200 is still pretty high for login and signup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is applied based on the ip address afaik.

Yes, its done via this line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

200 is still pretty high for login and signup.

Thats global limit, we override this in login route


LIMIT_1 = "20 per minute"
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Flask-Mail==0.9.1
Flask-Migrate==2.5.3
flask-restx==0.2.0
Flask-Testing==0.8.0
Flask-Limiter==1.4
gunicorn==20.0.4
psycopg2-binary==2.8.6
python-dotenv==0.14.0
Expand Down
7 changes: 5 additions & 2 deletions run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
from flask import Flask
from config import get_env_config
from flask_migrate import Migrate
from app.rate_limiter import limiter, rate_limit_exceeded


def create_app(config_filename: str) -> Flask:
app = Flask(__name__)

# initialize flask limiter
limiter.init_app(app)
# setup application environment
app.config.from_object(config_filename)
app.url_map.strict_slashes = False

# error handler for when rate limit is exceeded
app.register_error_handler(429, rate_limit_exceeded)
from app.database.sqlalchemy_extension import db

db.init_app(app)
Expand Down