Skip to content

Commit

Permalink
middleware: Move locale-setting before domain checking.
Browse files Browse the repository at this point in the history
Calling `render()` in a middleware before LocaleMiddleware has run
will pick up the most-recently-set locale.  This may be from the
_previous_ request, since the current language is thread-local.  This
results in the "Organization does not exist" page occasionally being
in not-English, depending on the preferences of the request which that
thread just finished serving.

Move HostDomainMiddleware below LocaleMiddleware; none of the earlier
middlewares call `render()`, so are safe.  This will also allow the
"Organization does not exist" page to be localized based on the user's
browser preferences.

Unfortunately, it also means that the default LocaleMiddleware catches
the 404 from the HostDomainMiddlware and helpfully tries to check if
the failure is because the URL lacks a language component (e.g.
`/en/`) by turning it into a 304 to that new URL.  We must subclass
the default LocaleMiddleware to remove this unwanted functionality.

Doing so exposes a two places in tests that relied (directly or
indirectly) upon the redirection: '/confirmation_key'
was redirected to '/en/confirmation_key', since the non-i18n version
did not exist; and requests to `/stats/realm/not_existing_realm/`
incorrectly were expecting a 302, not a 404.

This regression likely came in during f00ff1e, since prior to
that, the HostDomainMiddleware ran _after_ the rest of the request had
completed.
  • Loading branch information
alexmv authored and timabbott committed Sep 15, 2020
1 parent ab90abf commit 536bd31
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 3 deletions.
5 changes: 4 additions & 1 deletion analytics/tests/test_views.py
Expand Up @@ -52,12 +52,15 @@ def test_stats_for_realm(self) -> None:
result = self.client_get('/stats/realm/zulip/')
self.assertEqual(result.status_code, 302)

result = self.client_get('/stats/realm/not_existing_realm/')
self.assertEqual(result.status_code, 302)

user = self.example_user('hamlet')
user.is_staff = True
user.save(update_fields=['is_staff'])

result = self.client_get('/stats/realm/not_existing_realm/')
self.assertEqual(result.status_code, 302)
self.assertEqual(result.status_code, 404)

result = self.client_get('/stats/realm/zulip/')
self.assertEqual(result.status_code, 200)
Expand Down
1 change: 1 addition & 0 deletions zerver/lib/test_helpers.py
Expand Up @@ -426,6 +426,7 @@ def find_pattern(pattern: Any, prefixes: List[str]) -> None:
# static content URLs, since the content they point to may
# or may not exist.
'coverage/(?P<path>.+)',
'confirmation_key/',
'node-coverage/(?P<path>.+)',
'docs/(?P<path>.+)',
'casper/(?P<path>.+)',
Expand Down
19 changes: 19 additions & 0 deletions zerver/middleware.py
Expand Up @@ -5,11 +5,15 @@
from typing import Any, AnyStr, Callable, Dict, Iterable, List, MutableMapping, Optional, Union

from django.conf import settings
from django.conf.urls.i18n import is_language_prefix_patterns_used
from django.core.handlers.wsgi import WSGIRequest
from django.db import connection
from django.http import HttpRequest, HttpResponse, HttpResponseRedirect, StreamingHttpResponse
from django.middleware.common import CommonMiddleware
from django.middleware.locale import LocaleMiddleware as DjangoLocaleMiddleware
from django.shortcuts import render
from django.utils import translation
from django.utils.cache import patch_vary_headers
from django.utils.deprecation import MiddlewareMixin
from django.utils.translation import ugettext as _
from django.views.csrf import csrf_failure as html_csrf_failure
Expand Down Expand Up @@ -365,6 +369,21 @@ def csrf_failure(request: HttpRequest, reason: str="") -> HttpResponse:
else:
return html_csrf_failure(request, reason)

class LocaleMiddleware(DjangoLocaleMiddleware):
def process_response(self, request: HttpRequest, response: HttpResponse) -> HttpResponse:
# This is the same as the default LocaleMiddleware, minus the
# logic that redirects 404's that lack a prefixed language in
# the path into having a language. See
# https://code.djangoproject.com/ticket/32005
language = translation.get_language()
language_from_path = translation.get_language_from_path(request.path_info)
urlconf = getattr(request, 'urlconf', settings.ROOT_URLCONF)
i18n_patterns_used, _ = is_language_prefix_patterns_used(urlconf)
if not (i18n_patterns_used and language_from_path):
patch_vary_headers(response, ('Accept-Language',))
response.setdefault('Content-Language', language)
return response

class RateLimitMiddleware(MiddlewareMixin):
def set_response_headers(self, response: HttpResponse,
rate_limit_results: List[RateLimitResult]) -> None:
Expand Down
4 changes: 2 additions & 2 deletions zproject/computed_settings.py
Expand Up @@ -184,9 +184,9 @@ def get_dirs(self) -> List[str]:
'zerver.middleware.RateLimitMiddleware',
'zerver.middleware.FlushDisplayRecipientCache',
'zerver.middleware.ZulipCommonMiddleware',
'zerver.middleware.HostDomainMiddleware',
'django.contrib.sessions.middleware.SessionMiddleware',
'django.middleware.locale.LocaleMiddleware',
'zerver.middleware.LocaleMiddleware',
'zerver.middleware.HostDomainMiddleware',
'django.middleware.csrf.CsrfViewMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
# Make sure 2FA middlewares come after authentication middleware.
Expand Down
1 change: 1 addition & 0 deletions zproject/dev_urls.py
Expand Up @@ -81,6 +81,7 @@ def serve_static(request: HttpRequest, path: str) -> HttpResponse:
i18n_urls = [
path('confirmation_key/', zerver.views.development.registration.confirmation_key),
]
urls += i18n_urls

# On a production instance, these files would be served by nginx.
if settings.LOCAL_UPLOADS_DIR is not None:
Expand Down

0 comments on commit 536bd31

Please sign in to comment.