From f00ff1ef629fd2736f04f79b4d26108677ed98ac Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Thu, 6 Aug 2020 17:09:59 -0700 Subject: [PATCH] middleware: Make HostDomain into a process_request, not process_response. It is more suited for `process_request`, since it should stop execution of the request if the domain is invalid. This code was likely added as a process_response (in ea39fb2556) because there was already a process_response at the time (added 7e786d5426, and no longer necessary since dce6b4a40f). It quiets an unnecessary warning when logging in at a non-existent realm. This stops performing unnecessary work when we are going to throw it away and return a 404. The edge case to this is if the request _creates_ a realm, and is made using the URL of the new realm; this change would prevent the request before it occurs. While this does arise in tests, the tests do not reflect reality -- real requests to /accounts/register/ are made via POST to the same (default) realm, redirected there from `confirm-preregistrationuser`. The tests are adjusted to reflect real behavior. Tweaked by tabbott to add a block comment in HostDomainMiddleware. --- zerver/forms.py | 7 +----- zerver/middleware.py | 45 +++++++++++++++++-------------------- zerver/tests/test_signup.py | 15 ++++--------- 3 files changed, 25 insertions(+), 42 deletions(-) diff --git a/zerver/forms.py b/zerver/forms.py index a49db22c96..2480f8cf7d 100644 --- a/zerver/forms.py +++ b/zerver/forms.py @@ -343,12 +343,7 @@ class OurAuthenticationForm(AuthenticationForm): if username is not None and password: subdomain = get_subdomain(self.request) - try: - realm = get_realm(subdomain) - except Realm.DoesNotExist: - logging.warning("User %s attempted password login to nonexistent subdomain %s", - username, subdomain) - raise ValidationError("Realm does not exist") + realm = get_realm(subdomain) return_data: Dict[str, Any] = {} try: diff --git a/zerver/middleware.py b/zerver/middleware.py index 69a6a8652b..42d9fa5b9e 100644 --- a/zerver/middleware.py +++ b/zerver/middleware.py @@ -5,7 +5,6 @@ import traceback from typing import Any, AnyStr, Callable, Dict, Iterable, List, MutableMapping, Optional, Union from django.conf import settings -from django.core.exceptions import DisallowedHost from django.core.handlers.wsgi import WSGIRequest from django.db import connection from django.http import HttpRequest, HttpResponse, StreamingHttpResponse @@ -385,32 +384,28 @@ class FlushDisplayRecipientCache(MiddlewareMixin): return response class HostDomainMiddleware(MiddlewareMixin): - def process_response(self, request: HttpRequest, response: HttpResponse) -> HttpResponse: - if getattr(response, "asynchronous", False): - # This special Tornado "asynchronous" response is - # discarded after going through this code path as Tornado - # intends to block, so we stop here to avoid unnecessary work. - return response + def process_request(self, request: HttpRequest) -> Optional[HttpResponse]: + # Match against ALLOWED_HOSTS, which is rather permissive; + # failure will raise DisallowedHost, which is a 400. + request.get_host() - try: - request.get_host() - except DisallowedHost: - # If we get a DisallowedHost exception trying to access - # the host, (1) the request is failed anyway and so the - # below code will do nothing, and (2) the below will - # trigger a recursive exception, breaking things, so we - # just return here. - return response + # This check is important to avoid doing the extra work of + # `get_realm` (which does a database query that could be + # problematic for Tornado). Also the error page below is only + # appropriate for a page visited in a browser, not the API. + # + # API authentication will end up checking for an invalid + # realm, and throw a JSON-format error if appropriate. + if request.path.startswith(("/static/", "/api/", "/json/")): + return None - if (not request.path.startswith("/static/") and not request.path.startswith("/api/") and - not request.path.startswith("/json/")): - subdomain = get_subdomain(request) - if subdomain != Realm.SUBDOMAIN_FOR_ROOT_DOMAIN: - try: - get_realm(subdomain) - except Realm.DoesNotExist: - return render(request, "zerver/invalid_realm.html", status=404) - return response + subdomain = get_subdomain(request) + if subdomain != Realm.SUBDOMAIN_FOR_ROOT_DOMAIN: + try: + request.realm = get_realm(subdomain) + except Realm.DoesNotExist: + return render(request, "zerver/invalid_realm.html", status=404) + return None class SetRemoteAddrFromForwardedFor(MiddlewareMixin): """ diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 616f81f4a7..037134f21e 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -627,12 +627,8 @@ class LoginTest(ZulipTestCase): self.assert_logged_in_user_id(None) def test_login_invalid_subdomain(self) -> None: - with self.assertLogs(level='WARNING') as warn_log: - result = self.login_with_return(self.example_email("hamlet"), "xxx", - subdomain="invalid") - self.assertEqual(warn_log.output, [ - 'WARNING:root:User hamlet@zulip.com attempted password login to nonexistent subdomain invalid' - ]) + result = self.login_with_return(self.example_email("hamlet"), "xxx", + subdomain="invalid") self.assertEqual(result.status_code, 404) self.assert_in_response("There is no Zulip organization hosted at this subdomain.", result) self.assert_logged_in_user_id(None) @@ -2407,9 +2403,7 @@ class RealmCreationTest(ZulipTestCase): result = self.submit_reg_form_for_user(email, password, realm_subdomain = string_id, - realm_name=realm_name, - # Pass HTTP_HOST for the target subdomain - HTTP_HOST=string_id + ".testserver") + realm_name=realm_name) self.assertEqual(result.status_code, 302) result = self.client_get(result.url, subdomain=string_id) @@ -2447,8 +2441,7 @@ class RealmCreationTest(ZulipTestCase): result = self.submit_reg_form_for_user(email, password, realm_subdomain = string_id, - realm_name=realm_name, - HTTP_HOST=string_id + ".testserver") + realm_name=realm_name) self.assertEqual(result.status_code, 302) result = self.client_get(result.url, subdomain=string_id)