From 02ae71f27f4edecceab9f44c7c0ec5d232708c30 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Fri, 13 Jul 2018 16:28:16 +0530 Subject: [PATCH] api: Stop using API keys for Django->Tornado authentication. As part of our effort to change the data model away from each user having a single API key, we're eliminating the couple requests that were made from Django to Tornado (as part of a /register or home request) where we used the user's API key grabbed from the database for authentication. Instead, we use the (already existing) internal_notify_view authentication mechanism, which uses the SHARED_SECRET setting for security, for these requests, and just fetch the user object using get_user_profile_by_id directly. Tweaked by Yago to include the new /api/v1/events/internal endpoint in the exempt_patterns list in test_helpers, since it's an endpoint we call through Tornado. Also added a couple missing return type annotations. --- docs/subsystems/logging.md | 4 +-- .../files/nginx/sites-available/loadbalancer | 1 + zerver/lib/test_helpers.py | 1 + zerver/tornado/application.py | 10 ++++--- zerver/tornado/event_queue.py | 27 ++++++++++--------- zerver/tornado/views.py | 17 +++++++++--- zproject/urls.py | 1 + 7 files changed, 40 insertions(+), 21 deletions(-) diff --git a/docs/subsystems/logging.md b/docs/subsystems/logging.md index 9c579d092a..5b0805074b 100644 --- a/docs/subsystems/logging.md +++ b/docs/subsystems/logging.md @@ -73,8 +73,8 @@ can be extremely valuable for investigating performance problems. 2016-05-20 14:50:22.272 INFO [zr] 127.0.0.1 GET 200 124ms (db: 3ms/2q) /login/ (unauth via ?) 2016-05-20 14:50:26.333 INFO [zr] 127.0.0.1 POST 302 37ms (db: 6ms/7q) /accounts/login/local/ (unauth via ?) [20/May/2016 14:50:26]"POST /accounts/login/local/ HTTP/1.0" 302 0 -2016-05-20 14:50:26.538 INFO [zr] 127.0.0.1 GET 200 12ms (db: 1ms/2q) (+start: 53ms) /api/v1/events [1463769771:0/0] (cordelia@zulip.com via internal) -2016-05-20 14:50:26.657 INFO [zr] 127.0.0.1 GET 200 10ms (+start: 8ms) /api/v1/events [1463769771:0/0] (cordelia@zulip.com via internal) +2016-05-20 14:50:26.538 INFO [zr] 127.0.0.1 POST 200 12ms (db: 1ms/2q) (+start: 53ms) /api/v1/events/internal [1463769771:0/0] (cordelia@zulip.com via internal) +2016-05-20 14:50:26.657 INFO [zr] 127.0.0.1 POST 200 10ms (+start: 8ms) /api/v1/events/internal [1463769771:0/0] (cordelia@zulip.com via internal) 2016-05-20 14:50:26.959 INFO [zr] 127.0.0.1 GET 200 588ms (db: 26ms/21q) / [1463769771:0] (cordelia@zulip.com via website) ``` diff --git a/puppet/zulip_ops/files/nginx/sites-available/loadbalancer b/puppet/zulip_ops/files/nginx/sites-available/loadbalancer index 2d8157524c..efb0fc8d32 100644 --- a/puppet/zulip_ops/files/nginx/sites-available/loadbalancer +++ b/puppet/zulip_ops/files/nginx/sites-available/loadbalancer @@ -35,6 +35,7 @@ server { include /etc/nginx/zulip-include/location-sockjs; } + # We don't need /api/v1/events/internal, because that doesn't go through the loadbalancer. location ~ /json/events|/api/v1/events { proxy_pass https://staging; include /etc/nginx/zulip-include/proxy_longpolling; diff --git a/zerver/lib/test_helpers.py b/zerver/lib/test_helpers.py index eac40c4fdd..cb192930d6 100644 --- a/zerver/lib/test_helpers.py +++ b/zerver/lib/test_helpers.py @@ -378,6 +378,7 @@ def write_instrumentation_reports(full_suite: bool) -> None: exempt_patterns = set([ # We exempt some patterns that are called via Tornado. 'api/v1/events', + 'api/v1/events/internal', 'api/v1/register', # We also exempt some development environment debugging # static content URLs, since the content they point to may diff --git a/zerver/tornado/application.py b/zerver/tornado/application.py index 6619514de6..e7c14defa9 100644 --- a/zerver/tornado/application.py +++ b/zerver/tornado/application.py @@ -18,10 +18,12 @@ def setup_tornado_rabbitmq() -> None: # nocoverage autoreload.add_reload_hook(lambda: queue_client.close()) def create_tornado_application() -> tornado.web.Application: - urls = (r"/notify_tornado", - r"/json/events", - r"/api/v1/events", - ) + urls = ( + r"/notify_tornado", + r"/json/events", + r"/api/v1/events", + r"/api/v1/events/internal", + ) # Application is an instance of Django's standard wsgi handler. return tornado.web.Application(([(url, AsyncDjangoHandler) for url in urls] + diff --git a/zerver/tornado/event_queue.py b/zerver/tornado/event_queue.py index c806bf1ec2..fbb25e0bbc 100644 --- a/zerver/tornado/event_queue.py +++ b/zerver/tornado/event_queue.py @@ -560,17 +560,18 @@ def request_event_queue(user_profile: UserProfile, user_client: Client, apply_ma 'client_gravatar': ujson.dumps(client_gravatar), 'all_public_streams': ujson.dumps(all_public_streams), 'client': 'internal', + 'user_profile_id': user_profile.id, 'user_client': user_client.name, 'narrow': ujson.dumps(narrow), + 'secret': settings.SHARED_SECRET, 'lifespan_secs': queue_lifespan_secs} if event_types is not None: req['event_types'] = ujson.dumps(event_types) try: - resp = requests_client.get(settings.TORNADO_SERVER + '/api/v1/events', - auth=requests.auth.HTTPBasicAuth( - user_profile.email, user_profile.api_key), - params=req) + resp = requests_client.post(settings.TORNADO_SERVER + + '/api/v1/events/internal', + data=req) except requests.adapters.ConnectionError: logging.error('Tornado server does not seem to be running, check %s ' 'and %s for more information.' % @@ -587,14 +588,16 @@ def request_event_queue(user_profile: UserProfile, user_client: Client, apply_ma def get_user_events(user_profile: UserProfile, queue_id: str, last_event_id: int) -> List[Dict[Any, Any]]: if settings.TORNADO_SERVER: - resp = requests_client.get(settings.TORNADO_SERVER + '/api/v1/events', - auth=requests.auth.HTTPBasicAuth( - user_profile.email, user_profile.api_key), - params={'queue_id': queue_id, - 'last_event_id': last_event_id, - 'dont_block': 'true', - 'client': 'internal'}) - + post_data = { + 'queue_id': queue_id, + 'last_event_id': last_event_id, + 'dont_block': 'true', + 'user_profile_id': user_profile.id, + 'secret': settings.SHARED_SECRET, + 'client': 'internal' + } # type: Dict[str, Any] + resp = requests_client.post(settings.TORNADO_SERVER + '/api/v1/events/internal', + data=post_data) resp.raise_for_status() return extract_json_response(resp)['events'] diff --git a/zerver/tornado/views.py b/zerver/tornado/views.py index d8f9b8ea9b..26d31e271b 100644 --- a/zerver/tornado/views.py +++ b/zerver/tornado/views.py @@ -9,10 +9,10 @@ from django.utils.translation import ugettext as _ from zerver.decorator import REQ, RespondAsynchronously, \ _RespondAsynchronously, asynchronous, \ - has_request_variables, internal_notify_view + has_request_variables, internal_notify_view, process_client from zerver.lib.response import json_error, json_success from zerver.lib.validator import check_bool, check_list, check_string -from zerver.models import Client, UserProfile, get_client +from zerver.models import Client, UserProfile, get_client, get_user_profile_by_id from zerver.tornado.event_queue import fetch_events, \ get_client_descriptor, process_notification from zerver.tornado.exceptions import BadEventQueueIdError @@ -35,7 +35,18 @@ def cleanup_event_queue(request: HttpRequest, user_profile: UserProfile, return json_success() @asynchronous -def get_events(request: HttpRequest, user_profile: UserProfile, handler: BaseHandler): +@internal_notify_view(True) +@has_request_variables +def get_events_internal(request: HttpRequest, handler: BaseHandler, + user_profile_id: int=REQ()) -> Union[HttpResponse, _RespondAsynchronously]: + user_profile = get_user_profile_by_id(user_profile_id) + request._email = user_profile.email + process_client(request, user_profile, client_name="internal") + return get_events_backend(request, user_profile, handler) + +@asynchronous +def get_events(request: HttpRequest, user_profile: UserProfile, + handler: BaseHandler) -> Union[HttpResponse, _RespondAsynchronously]: return get_events_backend(request, user_profile, handler) @has_request_variables diff --git a/zproject/urls.py b/zproject/urls.py index b6edfb973d..6e9a1876bd 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -613,6 +613,7 @@ for app_name in settings.EXTRA_INSTALLED_APPS: urls += [ # Used internally for communication between Django and Tornado processes url(r'^notify_tornado$', zerver.tornado.views.notify, name='zerver.tornado.views.notify'), + url(r'^api/v1/events/internal$', zerver.tornado.views.get_events_internal), ] # Python Social Auth