middleware: Simplify logic for parsing user-agent.

This avoids calling parse_user_agent twice when dealing with official
Zulip clients, and also makes the logical flow hopefully easier to read.

We move get_client_name out of decorator.py, since it no longer
belongs there, and give it a nicer name.

zerver/decorator.py

@@ -43,7 +43,6 @@ from zerver.lib.response import json_error, json_method_not_allowed, json_succes
 from zerver.lib.subdomains import get_subdomain, user_matches_subdomain
 from zerver.lib.timestamp import datetime_to_timestamp, timestamp_to_datetime
 from zerver.lib.types import ViewFuncT
-from zerver.lib.user_agent import parse_user_agent
 from zerver.lib.utils import has_api_key_format, statsd
 from zerver.models import Realm, UserProfile, get_client, get_user_profile_by_api_key
 
@@ -165,27 +164,6 @@ def require_billing_access(func: ViewFuncT) -> ViewFuncT:
     return cast(ViewFuncT, wrapper)  # https://github.com/python/mypy/issues/1927
 
 
-def get_client_name(request: HttpRequest) -> str:
-    # If the API request specified a client in the request content,
-    # that has priority.  Otherwise, extract the client from the
-    # User-Agent.
-    if "client" in request.GET:  # nocoverage
-        return request.GET["client"]
-    if "client" in request.POST:
-        return request.POST["client"]
-    if "HTTP_USER_AGENT" in request.META:
-        user_agent: Optional[Dict[str, str]] = parse_user_agent(request.META["HTTP_USER_AGENT"])
-    else:
-        user_agent = None
-    if user_agent is not None:
-        return user_agent["name"]
-
-    # In the future, we will require setting USER_AGENT, but for
-    # now we just want to tag these requests so we can review them
-    # in logs and figure out the extent of the problem
-    return "Unspecified"
-
-
 def process_client(
     request: HttpRequest,
     user_profile: UserProfile,

zerver/middleware.py

@@ -2,7 +2,18 @@ import cProfile
 import logging
 import time
 import traceback
-from typing import Any, AnyStr, Callable, Dict, Iterable, List, MutableMapping, Optional, Union
+from typing import (
+    Any,
+    AnyStr,
+    Callable,
+    Dict,
+    Iterable,
+    List,
+    MutableMapping,
+    Optional,
+    Tuple,
+    Union,
+)
 
 from django.conf import settings
 from django.conf.urls.i18n import is_language_prefix_patterns_used
@@ -20,7 +31,6 @@ from django.views.csrf import csrf_failure as html_csrf_failure
 from sentry_sdk import capture_exception
 from sentry_sdk.integrations.logging import ignore_logger
 
-from zerver.decorator import get_client_name
 from zerver.lib.cache import get_remote_cache_requests, get_remote_cache_time
 from zerver.lib.db import reset_queries
 from zerver.lib.debug import maybe_tracemalloc_listen
@@ -290,16 +300,41 @@ class RequestContext(MiddlewareMixin):
             unset_request()
 
 
+def parse_client(request: HttpRequest) -> Tuple[str, Optional[str]]:
+    # If the API request specified a client in the request content,
+    # that has priority.  Otherwise, extract the client from the
+    # User-Agent.
+    if "client" in request.GET:  # nocoverage
+        return request.GET["client"], None
+    if "client" in request.POST:
+        return request.POST["client"], None
+    if "HTTP_USER_AGENT" in request.META:
+        user_agent: Optional[Dict[str, str]] = parse_user_agent(request.META["HTTP_USER_AGENT"])
+    else:
+        user_agent = None
+    if user_agent is None:
+        # In the future, we will require setting USER_AGENT, but for
+        # now we just want to tag these requests so we can review them
+        # in logs and figure out the extent of the problem
+        return "Unspecified", None
+
+    client_name = user_agent["name"]
+    if client_name.startswith("Zulip"):
+        return client_name, user_agent.get("version")
+
+    # We could show browser versions in logs, and it'd probably be a
+    # good idea, but the current parsing will just get you Mozilla/5.0.
+    #
+    # Fixing this probably means using a third-party library, and
+    # making sure it's fast enough that we're happy to do it even on
+    # hot-path cases.
+    return client_name, None
+
+
 class LogRequests(MiddlewareMixin):
     # We primarily are doing logging using the process_view hook, but
     # for some views, process_view isn't run, so we call the start
     # method here too
-    def process_user_agent(self, request: HttpRequest) -> None:
-        request.client_name = get_client_name(request)
-        request.client_version = None
-        if request.client_name.startswith("Zulip"):
-            request.client_version = parse_user_agent(request.META["HTTP_USER_AGENT"])["version"]
-
     def process_request(self, request: HttpRequest) -> None:
         maybe_tracemalloc_listen()
 
@@ -310,8 +345,8 @@ class LogRequests(MiddlewareMixin):
 
             # Avoid re-initializing request._log_data if it's already there.
             return
-        self.process_user_agent(request)
 
+        request.client_name, request.client_version = parse_client(request)
         request._log_data = {}
         record_request_start_data(request._log_data)
 

zerver/tests/test_decorators.py

@@ -18,7 +18,6 @@ from zerver.decorator import (
     authenticated_rest_api_view,
     authenticated_uploads_api_view,
     cachify,
-    get_client_name,
     internal_notify_view,
     is_local_addr,
     rate_limit,
@@ -82,6 +81,7 @@ from zerver.lib.validator import (
     to_non_negative_int,
     to_positive_or_allowed_int,
 )
+from zerver.middleware import parse_client
 from zerver.models import Realm, UserProfile, get_realm, get_user
 
 if settings.ZILENCER_ENABLED:
@@ -89,41 +89,41 @@ if settings.ZILENCER_ENABLED:
 
 
 class DecoratorTestCase(ZulipTestCase):
-    def test_get_client_name(self) -> None:
+    def test_parse_client(self) -> None:
         req = HostRequestMock()
-        self.assertEqual(get_client_name(req), "Unspecified")
+        self.assertEqual(parse_client(req), ("Unspecified", None))
 
         req.META[
             "HTTP_USER_AGENT"
         ] = "ZulipElectron/4.0.3 Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_3) AppleWebKit/537.36 (KHTML, like Gecko) Zulip/4.0.3 Chrome/66.0.3359.181 Electron/3.1.10 Safari/537.36"
-        self.assertEqual(get_client_name(req), "ZulipElectron")
+        self.assertEqual(parse_client(req), ("ZulipElectron", "4.0.3"))
 
         req.META["HTTP_USER_AGENT"] = "ZulipDesktop/0.4.4 (Mac)"
-        self.assertEqual(get_client_name(req), "ZulipDesktop")
+        self.assertEqual(parse_client(req), ("ZulipDesktop", "0.4.4"))
 
         req.META["HTTP_USER_AGENT"] = "ZulipMobile/26.22.145 (Android 10)"
-        self.assertEqual(get_client_name(req), "ZulipMobile")
+        self.assertEqual(parse_client(req), ("ZulipMobile", "26.22.145"))
 
         req.META["HTTP_USER_AGENT"] = "ZulipMobile/26.22.145 (iOS 13.3.1)"
-        self.assertEqual(get_client_name(req), "ZulipMobile")
+        self.assertEqual(parse_client(req), ("ZulipMobile", "26.22.145"))
 
         # TODO: This should ideally be Firefox.
         req.META[
             "HTTP_USER_AGENT"
         ] = "Mozilla/5.0 (X11; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0"
-        self.assertEqual(get_client_name(req), "Mozilla")
+        self.assertEqual(parse_client(req), ("Mozilla", None))
 
         # TODO: This should ideally be Chrome.
         req.META[
             "HTTP_USER_AGENT"
         ] = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.43 Safari/537.36"
-        self.assertEqual(get_client_name(req), "Mozilla")
+        self.assertEqual(parse_client(req), ("Mozilla", None))
 
         # TODO: This should ideally be Mobile Safari if we had better user-agent parsing.
         req.META[
             "HTTP_USER_AGENT"
         ] = "Mozilla/5.0 (Linux; Android 8.0.0; SM-G930F) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.132 Mobile Safari/537.36"
-        self.assertEqual(get_client_name(req), "Mozilla")
+        self.assertEqual(parse_client(req), ("Mozilla", None))
 
     def test_REQ_aliases(self) -> None:
         @has_request_variables