From 564aa90499ea54f4aaf8cbee55a673e407172bd8 Mon Sep 17 00:00:00 2001 From: shadrak gurupnor Date: Wed, 25 Aug 2021 17:54:09 +0530 Subject: [PATCH 1/7] chore: overriding of modified_by and owner --- frappe/model/document.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index 37549e2001..d0d66ac3de 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -469,7 +469,7 @@ class Document(BaseDocument): if not self.creation: self.creation = self.modified if not self.owner: - self.owner = self.modified_by + self.owner = self.flags.owner or self.modified_by for d in self.get_all_children(): d.modified = self.modified From f3bc29cbdc19066c14acdfa8b7a47f3dbcc9c73e Mon Sep 17 00:00:00 2001 From: shadrak gurupnor Date: Tue, 31 Aug 2021 12:14:12 +0530 Subject: [PATCH 2/7] feat: applied rate-limiting on web-forms to avoid bulk submission --- frappe/model/document.py | 2 +- frappe/rate_limiter.py | 10 ++++++++-- frappe/website/doctype/web_form/web_form.py | 3 ++- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index d0d66ac3de..37549e2001 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -469,7 +469,7 @@ class Document(BaseDocument): if not self.creation: self.creation = self.modified if not self.owner: - self.owner = self.flags.owner or self.modified_by + self.owner = self.modified_by for d in self.get_all_children(): d.modified = self.modified diff --git a/frappe/rate_limiter.py b/frappe/rate_limiter.py index 528e5d6b56..743ddcfe24 100644 --- a/frappe/rate_limiter.py +++ b/frappe/rate_limiter.py @@ -107,8 +107,14 @@ def rate_limit(key: str, limit: Union[int, Callable] = 5, seconds: int= 24*60*60 _limit = limit() if callable(limit) else limit - identity = frappe.form_dict[key] - cache_key = f"rl:{frappe.form_dict.cmd}:{identity}" + cmd = (frappe.form_dict.cmd).split('.')[-1] + user_key=frappe.form_dict[key] + ip = frappe.local.request_ip + + # cmd "accept" is used for web-forms only + ip_based_key = ":".join([ip, user_key]) if cmd == 'accept' else ip + + cache_key = f"rl:{frappe.form_dict.cmd}:{ip_based_key}" value = frappe.cache().get_value(cache_key, expires=True) or 0 if not value: diff --git a/frappe/website/doctype/web_form/web_form.py b/frappe/website/doctype/web_form/web_form.py index 32f7e030a6..948ce46283 100644 --- a/frappe/website/doctype/web_form/web_form.py +++ b/frappe/website/doctype/web_form/web_form.py @@ -13,7 +13,7 @@ from frappe.modules.utils import export_module_json, get_doc_module from frappe.utils import cstr from frappe.website.utils import get_comment_list from frappe.website.website_generator import WebsiteGenerator - +from frappe.rate_limiter import rate_limit class WebForm(WebsiteGenerator): website = frappe._dict( @@ -365,6 +365,7 @@ def get_context(context): @frappe.whitelist(allow_guest=True) +@rate_limit(key='web_form', limit=5, seconds=60, methods=['POST']) def accept(web_form, data, docname=None, for_payment=False): '''Save the web form''' data = frappe._dict(json.loads(data)) From 5cc437aa6b593d822e79588c64c85a21631a3033 Mon Sep 17 00:00:00 2001 From: shadrak gurupnor Date: Tue, 7 Sep 2021 19:45:00 +0530 Subject: [PATCH 3/7] feat: made key optional and added IP flag --- frappe/core/doctype/user/user.py | 2 +- frappe/rate_limiter.py | 23 +++++++++++++++-------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 1336f6eab7..2fea80b137 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -788,7 +788,7 @@ def sign_up(email, full_name, redirect_to): return 2, _("Please ask your administrator to verify your sign-up") @frappe.whitelist(allow_guest=True) -@rate_limit(key='user', limit=get_password_reset_limit, seconds = 24*60*60, methods=['POST']) +@rate_limit(limit=get_password_reset_limit, seconds = 24*60*60, methods=['POST']) def reset_password(user): if user=="Administrator": return 'not allowed' diff --git a/frappe/rate_limiter.py b/frappe/rate_limiter.py index 743ddcfe24..3d1c889cfc 100644 --- a/frappe/rate_limiter.py +++ b/frappe/rate_limiter.py @@ -82,19 +82,21 @@ class RateLimiter: if self.rejected: return Response(_("Too Many Requests"), status=429) -def rate_limit(key: str, limit: Union[int, Callable] = 5, seconds: int= 24*60*60, methods: Union[str, list]='ALL'): +def rate_limit(key: str=None, limit: Union[int, Callable] = 5, seconds: int= 24*60*60, methods: Union[str, list]='ALL', ip_based: bool=True): """Decorator to rate limit an endpoint. This will limit Number of requests per endpoint to `limit` within `seconds`. Uses redis cache to track request counts. - :param key: Key is used to identify the requests uniqueness + :param key: Key is used to identify the requests uniqueness (Optional) :param limit: Maximum number of requests to allow with in window time :type limit: Callable or Integer :param seconds: window time to allow requests :param methods: Limit the validation for these methods. `ALL` is a wildcard that applies rate limit on all methods. :type methods: string or list or tuple + :param ip_based: flag to allow ip based rate-limiting + :type ip_based: Boolean :returns: a decorator function that limit the number of requests per endpoint """ @@ -107,14 +109,19 @@ def rate_limit(key: str, limit: Union[int, Callable] = 5, seconds: int= 24*60*60 _limit = limit() if callable(limit) else limit - cmd = (frappe.form_dict.cmd).split('.')[-1] - user_key=frappe.form_dict[key] ip = frappe.local.request_ip - - # cmd "accept" is used for web-forms only - ip_based_key = ":".join([ip, user_key]) if cmd == 'accept' else ip - cache_key = f"rl:{frappe.form_dict.cmd}:{ip_based_key}" + if key == None and ip_based == False: + frappe.throw(_('Either key or IP flag is required.')) + elif key == None: + identity = ip + elif ip_based == False: + identity = frappe.form_dict[key] + else: + user_key=frappe.form_dict[key] + identity = ":".join([ip, user_key]) + + cache_key = f"rl:{frappe.form_dict.cmd}:{identity}" value = frappe.cache().get_value(cache_key, expires=True) or 0 if not value: From 5343b28ab5ee1e555276a63eddc6760c0fbb0e1e Mon Sep 17 00:00:00 2001 From: shadrak gurupnor Date: Thu, 9 Sep 2021 22:35:32 +0530 Subject: [PATCH 4/7] fix: expiry was not setting on key --- frappe/rate_limiter.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/rate_limiter.py b/frappe/rate_limiter.py index 3d1c889cfc..307dfbe6f1 100644 --- a/frappe/rate_limiter.py +++ b/frappe/rate_limiter.py @@ -123,9 +123,9 @@ def rate_limit(key: str=None, limit: Union[int, Callable] = 5, seconds: int= 24* cache_key = f"rl:{frappe.form_dict.cmd}:{identity}" - value = frappe.cache().get_value(cache_key, expires=True) or 0 + value = frappe.cache().get(cache_key) or 0 if not value: - frappe.cache().set_value(cache_key, 0, expires_in_sec=seconds) + frappe.cache().setex(cache_key, seconds, 0) value = frappe.cache().incrby(cache_key, 1) if value > _limit: From 930dddc558c3e4ff098ed8e873569e66973f044e Mon Sep 17 00:00:00 2001 From: shadrak gurupnor Date: Tue, 14 Sep 2021 21:00:40 +0530 Subject: [PATCH 5/7] fix: test cases for web forms --- frappe/rate_limiter.py | 2 +- .../website/doctype/web_form/test_web_form.py | 20 +++++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/frappe/rate_limiter.py b/frappe/rate_limiter.py index 307dfbe6f1..6392ad54fb 100644 --- a/frappe/rate_limiter.py +++ b/frappe/rate_limiter.py @@ -104,7 +104,7 @@ def rate_limit(key: str=None, limit: Union[int, Callable] = 5, seconds: int= 24* @wraps(fun) def wrapper(*args, **kwargs): # Do not apply rate limits if method is not opted to check - if methods != 'ALL' and frappe.request.method.upper() not in methods: + if methods != 'ALL' and frappe.request and frappe.request.method and frappe.request.method.upper() not in methods: return frappe.call(fun, **frappe.form_dict or kwargs) _limit = limit() if callable(limit) else limit diff --git a/frappe/website/doctype/web_form/test_web_form.py b/frappe/website/doctype/web_form/test_web_form.py index 603b0c9df7..f189f65ea2 100644 --- a/frappe/website/doctype/web_form/test_web_form.py +++ b/frappe/website/doctype/web_form/test_web_form.py @@ -16,15 +16,26 @@ class TestWebForm(unittest.TestCase): def tearDown(self): frappe.conf.disable_website_cache = False frappe.local.path = None + frappe.local.request_ip = None + frappe.form_dict.web_form = None + frappe.form_dict.data = None + frappe.form_dict.docname = None def test_accept(self): frappe.set_user("Administrator") - accept(web_form='manage-events', data=json.dumps({ + + doc = { 'doctype': 'Event', 'subject': '_Test Event Web Form', 'description': '_Test Event Description', 'starts_on': '2014-09-09' - })) + } + + frappe.form_dict.web_form = "manage-events" + frappe.form_dict.data = json.dumps(doc) + frappe.local.request_ip = '127.0.0.1' + + accept(web_form='manage-events', data=json.dumps(doc)) self.event_name = frappe.db.get_value("Event", {"subject": "_Test Event Web Form"}) @@ -32,6 +43,7 @@ class TestWebForm(unittest.TestCase): def test_edit(self): self.test_accept() + doc={ 'doctype': 'Event', 'subject': '_Test Event Web Form', @@ -43,6 +55,10 @@ class TestWebForm(unittest.TestCase): self.assertNotEqual(frappe.db.get_value("Event", self.event_name, "description"), doc.get('description')) + frappe.form_dict.web_form = 'manage-events' + frappe.form_dict.docname = self.event_name + frappe.form_dict.data = json.dumps(doc) + accept(web_form='manage-events', docname=self.event_name, data=json.dumps(doc)) self.assertEqual(frappe.db.get_value("Event", From 851778e5616db4c9f7a40fb6bd52514f28a10770 Mon Sep 17 00:00:00 2001 From: shadrak gurupnor Date: Wed, 15 Sep 2021 07:41:03 +0530 Subject: [PATCH 6/7] fix(minor): sider issues --- frappe/rate_limiter.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frappe/rate_limiter.py b/frappe/rate_limiter.py index 6392ad54fb..1927d6c47b 100644 --- a/frappe/rate_limiter.py +++ b/frappe/rate_limiter.py @@ -82,7 +82,7 @@ class RateLimiter: if self.rejected: return Response(_("Too Many Requests"), status=429) -def rate_limit(key: str=None, limit: Union[int, Callable] = 5, seconds: int= 24*60*60, methods: Union[str, list]='ALL', ip_based: bool=True): +def rate_limit(key: str = None, limit: Union[int, Callable] = 5, seconds: int = 24*60*60, methods: Union[str, list] = 'ALL', ip_based: bool = True): """Decorator to rate limit an endpoint. This will limit Number of requests per endpoint to `limit` within `seconds`. @@ -111,11 +111,11 @@ def rate_limit(key: str=None, limit: Union[int, Callable] = 5, seconds: int= 24* ip = frappe.local.request_ip - if key == None and ip_based == False: + if key is None and ip_based is False: frappe.throw(_('Either key or IP flag is required.')) - elif key == None: + elif key is None: identity = ip - elif ip_based == False: + elif ip_based is False: identity = frappe.form_dict[key] else: user_key=frappe.form_dict[key] From b057a07259c465afac0ee13727a8dac40b978470 Mon Sep 17 00:00:00 2001 From: shadrak gurupnor Date: Wed, 15 Sep 2021 16:07:03 +0530 Subject: [PATCH 7/7] fix(minor): code clean up --- frappe/rate_limiter.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/frappe/rate_limiter.py b/frappe/rate_limiter.py index 1927d6c47b..1255b10ed7 100644 --- a/frappe/rate_limiter.py +++ b/frappe/rate_limiter.py @@ -109,18 +109,20 @@ def rate_limit(key: str = None, limit: Union[int, Callable] = 5, seconds: int = _limit = limit() if callable(limit) else limit - ip = frappe.local.request_ip + ip = frappe.local.request_ip if ip_based is True else None - if key is None and ip_based is False: - frappe.throw(_('Either key or IP flag is required.')) - elif key is None: - identity = ip - elif ip_based is False: - identity = frappe.form_dict[key] - else: - user_key=frappe.form_dict[key] + user_key = frappe.form_dict[key] if key else None + + identity = None + + if key and ip_based: identity = ":".join([ip, user_key]) + identity = identity or ip or user_key + + if not identity: + frappe.throw(_('Either key or IP flag is required.')) + cache_key = f"rl:{frappe.form_dict.cmd}:{identity}" value = frappe.cache().get(cache_key) or 0