paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-02 04:53:36 +00:00
Code Issues Packages Projects Releases Wiki Activity

decorator: Skip rate limiting when accessing user uploads.

Browse Source 
The code paths for accessing user-uploaded files are both (A) highly
optimized so as to not require a ton of work, and (B) a code path
where it's totally reasonable for a client to need to fetch 100+
images all at once (e.g. if it's the first browser open in a setting
with a lot of distinct senders with avatars or a lot of image
previews).

Additionally, we've been seeing exceptions logged in the production
redis configuration caused by this code path (basically, locking
failures trying to update the rate-limit data structures).

So we skip running our current rate limiting algorithm for these views.
This commit is contained in:
Tim Abbott
2018-12-11 11:46:52 -08:00
parent 9f8cc925b8
commit 15d4b71e2e
3 changed files with 104 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
zerver/decorator.py
View File

@@ -558,7 +558,7 @@ def authenticated_api_view(is_webhook: bool=False) -> Callable[[ViewFuncT], View
# This API endpoint is used only for the mobile apps. It is part of a
# workaround for the fact that React Native doesn't support setting
# HTTP basic authentication headers.
def authenticated_uploads_api_view() -> Callable[[ViewFuncT], ViewFuncT]:
def authenticated_uploads_api_view(skip_rate_limiting: bool=False) -> Callable[[ViewFuncT], ViewFuncT]:
def _wrapped_view_func(view_func: ViewFuncT) -> ViewFuncT:
@csrf_exempt
@has_request_variables
@@ -567,7 +567,10 @@ def authenticated_uploads_api_view() -> Callable[[ViewFuncT], ViewFuncT]:
api_key: str=REQ(),
*args: Any, **kwargs: Any) -> HttpResponse:
user_profile = validate_api_key(request, None, api_key, False)
limited_func = rate_limit()(view_func)
if not skip_rate_limiting:
limited_func = rate_limit()(view_func)
else:
limited_func = view_func
return limited_func(request, user_profile, *args, **kwargs)
return _wrapped_func_arguments
return _wrapped_view_func
@@ -578,7 +581,8 @@ def authenticated_uploads_api_view() -> Callable[[ViewFuncT], ViewFuncT]:
# If webhook_client_name is specific, the request is a webhook view
# with that string as the basis for the client string.
def authenticated_rest_api_view(*, webhook_client_name: Optional[str]=None,
is_webhook: bool=False) -> Callable[[ViewFuncT], ViewFuncT]:
is_webhook: bool=False,
skip_rate_limiting: bool=False) -> Callable[[ViewFuncT], ViewFuncT]:
def _wrapped_view_func(view_func: ViewFuncT) -> ViewFuncT:
@csrf_exempt
@wraps(view_func)
@@ -606,8 +610,12 @@ def authenticated_rest_api_view(*, webhook_client_name: Optional[str]=None,
except JsonableError as e:
return json_unauthorized(e.msg)
try:
# Apply rate limiting
return rate_limit()(view_func)(request, profile, *args, **kwargs)
if not skip_rate_limiting:
# Apply rate limiting
target_view_func = rate_limit()(view_func)
else:
target_view_func = view_func
return target_view_func(request, profile, *args, **kwargs)
except Exception as err:
if is_webhook or webhook_client_name is not None:
request_body = request.POST.get('payload')
@@ -650,7 +658,8 @@ def process_as_post(view_func: ViewFuncT) -> ViewFuncT:
def authenticate_log_and_execute_json(request: HttpRequest,
view_func: ViewFuncT,
*args: Any, **kwargs: Any) -> HttpResponse:
*args: Any, skip_rate_limiting: bool = False,
**kwargs: Any) -> HttpResponse:
if not request.user.is_authenticated:
return json_error(_("Not logged in"), status=401)
user_profile = request.user
@@ -662,7 +671,9 @@ def authenticate_log_and_execute_json(request: HttpRequest,
process_client(request, user_profile, is_browser_view=True,
query=view_func.__name__)
request._email = user_profile.delivery_email
return rate_limit()(view_func)(request, user_profile, *args, **kwargs)
if not skip_rate_limiting:
view_func = rate_limit()(view_func)
return view_func(request, user_profile, *args, **kwargs)
# Checks if the request is a POST request and that the user is logged
# in. If not, return an error (the @login_required behavior of
@@ -676,10 +687,11 @@ def authenticated_json_post_view(view_func: ViewFuncT) -> ViewFuncT:
return authenticate_log_and_execute_json(request, view_func, *args, **kwargs)
return _wrapped_view_func # type: ignore # https://github.com/python/mypy/issues/1927
def authenticated_json_view(view_func: ViewFuncT) -> ViewFuncT:
def authenticated_json_view(view_func: ViewFuncT, skip_rate_limiting: bool=False) -> ViewFuncT:
@wraps(view_func)
def _wrapped_view_func(request: HttpRequest,
*args: Any, **kwargs: Any) -> HttpResponse:
kwargs["skip_rate_limiting"] = skip_rate_limiting
return authenticate_log_and_execute_json(request, view_func, *args, **kwargs)
return _wrapped_view_func # type: ignore # https://github.com/python/mypy/issues/1927

19
zerver/lib/rest.py
View File

@@ -85,18 +85,25 @@ def rest_dispatch(request: HttpRequest, **kwargs: Any) -> HttpResponse:
if ('override_api_url_scheme' in view_flags and
request.META.get('HTTP_AUTHORIZATION', None) is not None):
# This request uses standard API based authentication.
target_function = authenticated_rest_api_view()(target_function)
# For override_api_url_scheme views, we skip our normal
# rate limiting, because there are good reasons clients
# might need to (e.g.) request a large number of uploaded
# files or avatars in quick succession.
target_function = authenticated_rest_api_view(skip_rate_limiting=True)(target_function)
elif ('override_api_url_scheme' in view_flags and
request.GET.get('api_key') is not None):
# This request uses legacy API authentication. We
# unfortunately need that in the React Native mobile
# apps, because there's no way to set
# HTTP_AUTHORIZATION in React Native.
target_function = authenticated_uploads_api_view()(target_function)
# unfortunately need that in the React Native mobile apps,
# because there's no way to set HTTP_AUTHORIZATION in
# React Native. See last block for rate limiting notes.
target_function = authenticated_uploads_api_view(skip_rate_limiting=True)(target_function)
# /json views (web client) validate with a session token (cookie)
elif not request.path.startswith("/api") and request.user.is_authenticated:
# Authenticated via sessions framework, only CSRF check needed
target_function = csrf_protect(authenticated_json_view(target_function))
auth_kwargs = {}
if 'override_api_url_scheme' in view_flags:
auth_kwargs["skip_rate_limiting"] = True
target_function = csrf_protect(authenticated_json_view(target_function, **auth_kwargs))
# most clients (mobile, bots, etc) use HTTP Basic Auth and REST calls, where instead of
# username:password, we use email:apiKey

72
zerver/tests/test_decorators.py
View File

@@ -25,7 +25,7 @@ from zerver.lib.test_classes import (
ZulipTestCase,
WebhookTestCase,
)
from zerver.lib.response import json_response
from zerver.lib.response import json_response, json_success
from zerver.lib.users import get_api_key
from zerver.lib.user_agent import parse_user_agent
from zerver.lib.request import \
@@ -34,7 +34,9 @@ from zerver.lib.request import \
from zerver.decorator import (
api_key_only_webhook_view,
authenticated_api_view,
authenticated_json_view,
authenticated_rest_api_view,
authenticated_uploads_api_view,
authenticate_notify, cachify,
get_client_name, internal_notify_view, is_local_addr,
rate_limit, validate_api_key, logged_in_and_active,
@@ -391,6 +393,74 @@ body:
with self.assertRaisesRegex(JsonableError, "This organization has been deactivated"):
my_webhook(request) # type: ignore # mypy doesn't seem to apply the decorator
class SkipRateLimitingTest(ZulipTestCase):
def test_authenticated_rest_api_view(self) -> None:
@authenticated_rest_api_view(skip_rate_limiting=False)
def my_rate_limited_view(request: HttpRequest, user_profile: UserProfile) -> str:
return json_success() # nocoverage # mock prevents this from being called
@authenticated_rest_api_view(skip_rate_limiting=True)
def my_unlimited_view(request: HttpRequest, user_profile: UserProfile) -> str:
return json_success()
request = HostRequestMock(host="zulip.testserver")
request.META['HTTP_AUTHORIZATION'] = self.encode_credentials(self.example_email("hamlet"))
request.method = 'POST'
with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock:
result = my_unlimited_view(request) # type: ignore # mypy doesn't seem to apply the decorator
self.assert_json_success(result)
self.assertFalse(rate_limit_mock.called)
with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock:
result = my_rate_limited_view(request) # type: ignore # mypy doesn't seem to apply the decorator
# Don't assert json_success, since it'll be the rate_limit mock object
self.assertTrue(rate_limit_mock.called)
def test_authenticated_uploads_api_view(self) -> None:
@authenticated_uploads_api_view(skip_rate_limiting=False)
def my_rate_limited_view(request: HttpRequest, user_profile: UserProfile) -> str:
return json_success() # nocoverage # mock prevents this from being called
@authenticated_uploads_api_view(skip_rate_limiting=True)
def my_unlimited_view(request: HttpRequest, user_profile: UserProfile) -> str:
return json_success()
request = HostRequestMock(host="zulip.testserver")
request.method = 'POST'
request.POST['api_key'] = get_api_key(self.example_user("hamlet"))
with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock:
result = my_unlimited_view(request) # type: ignore # mypy doesn't seem to apply the decorator
self.assert_json_success(result)
self.assertFalse(rate_limit_mock.called)
with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock:
result = my_rate_limited_view(request) # type: ignore # mypy doesn't seem to apply the decorator
# Don't assert json_success, since it'll be the rate_limit mock object
self.assertTrue(rate_limit_mock.called)
def test_authenticated_json_view(self) -> None:
def my_view(request: HttpRequest, user_profile: UserProfile) -> str:
return json_success()
my_rate_limited_view = authenticated_json_view(my_view, skip_rate_limiting=False)
my_unlimited_view = authenticated_json_view(my_view, skip_rate_limiting=True)
request = HostRequestMock(host="zulip.testserver")
request.method = 'POST'
request.is_authenticated = True # type: ignore # HostRequestMock doesn't have is_authenticated
request.user = self.example_user("hamlet")
with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock:
result = my_unlimited_view(request) # type: ignore # mypy doesn't seem to apply the decorator
self.assert_json_success(result)
self.assertFalse(rate_limit_mock.called)
with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock:
result = my_rate_limited_view(request) # type: ignore # mypy doesn't seem to apply the decorator
# Don't assert json_success, since it'll be the rate_limit mock object
self.assertTrue(rate_limit_mock.called)
class DecoratorLoggingTestCase(ZulipTestCase):
def test_authenticated_api_view_logging(self) -> None: