decorators: Restructure get_client_name interface.

Previously, get_client_name was responsible for both parsing the
User-Agent data as well as handling the override behavior that we want
to use "website" rather than "Mozilla" as the key for the Client object.

Now, it's just responsible for User-Agent, and the override behavior
is entirely within process_client (the function concerned with Client
objects).

This has the side effect of changing what `Client` object we'll use
for HTTP requests to /json/ endpoints that set the `client` attribute.
I think that's in line with our intent -- we only have a use case for
API clients overriding the User-Agent parsing (that feature is a
workaround for situations where the third party may not control HTTP
headers but does control the HTTP request payload).

This loses test coverage on the `request.GET['client']` code path; I
disable that for now since we don't have a real use for that behavior.

(We may want to change that logic to have Client recognize individual
browsers; doing so requires first using a better User-Agent parsing
library).

Part of #14067.

zerver/context_processors.py

@@ -102,7 +102,7 @@ def zulip_default_context(request: HttpRequest) -> Dict[str, Any]:
     # We can't use request.client here because we might not be using
     # an auth decorator that sets it, but we can call its helper to
     # get the same result.
-    platform = get_client_name(request, True)
+    platform = get_client_name(request)
 
     context = {
         'root_domain_landing_page': settings.ROOT_DOMAIN_LANDING_PAGE,

zerver/decorator.py

@@ -122,11 +122,11 @@ def require_billing_access(func: ViewFuncT) -> ViewFuncT:
 
 from zerver.lib.user_agent import parse_user_agent
 
-def get_client_name(request: HttpRequest, is_browser_view: bool) -> str:
+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:
+    if 'client' in request.GET:  # nocoverage
         return request.GET['client']
     if 'client' in request.POST:
         return request.POST['client']
@@ -135,23 +135,12 @@ def get_client_name(request: HttpRequest, is_browser_view: bool) -> str:
     else:
         user_agent = None
     if user_agent is not None:
-        # We could check for a browser's name being "Mozilla", but
+        return user_agent["name"]
-        # e.g. Opera and MobileSafari don't set that, and it seems
+
-        # more robust to just key off whether it was a browser view
+    # In the future, we will require setting USER_AGENT, but for
-        if is_browser_view and not user_agent["name"].startswith("Zulip"):
+    # now we just want to tag these requests so we can review them
-            # Avoid changing the client string for browsers, but let
+    # in logs and figure out the extent of the problem
-            # the Zulip desktop and mobile apps be themselves.
+    return "Unspecified"
-            return "website"
-        else:
-            return user_agent["name"]
-    else:
-        # 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
-        if is_browser_view:
-            return "website"
-        else:
-            return "Unspecified"
 
 def process_client(request: HttpRequest, user_profile: UserProfile,
                    *, is_browser_view: bool=False,
@@ -159,7 +148,15 @@ def process_client(request: HttpRequest, user_profile: UserProfile,
                    skip_update_user_activity: bool=False,
                    query: Optional[str]=None) -> None:
     if client_name is None:
-        client_name = get_client_name(request, is_browser_view)
+        client_name = get_client_name(request)
+
+    # We could check for a browser's name being "Mozilla", but
+    # e.g. Opera and MobileSafari don't set that, and it seems
+    # more robust to just key off whether it was a browser view
+    if is_browser_view and not client_name.startswith("Zulip"):
+        # Avoid changing the client string for browsers, but let
+        # the Zulip desktop apps be themselves.
+        client_name = "website"
 
     request.client = get_client(client_name)
     if not skip_update_user_activity:

zerver/tests/test_decorators.py

@@ -55,56 +55,32 @@ import ujson
 
 class DecoratorTestCase(TestCase):
     def test_get_client_name(self) -> None:
-        class Request:
+        req = HostRequestMock()
-            def __init__(self, GET: Dict[str, str], POST: Dict[str, str], META: Dict[str, str]) -> None:
+        self.assertEqual(get_client_name(req), 'Unspecified')
-                self.GET = GET
-                self.POST = POST
-                self.META = META
 
-        req = Request(
+        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'
-            GET=dict(),
+        self.assertEqual(get_client_name(req), 'ZulipElectron')
-            POST=dict(),
-            META=dict(),
-        )
 
-        self.assertEqual(get_client_name(req, is_browser_view=True), 'website')
+        req.META['HTTP_USER_AGENT'] = 'ZulipDesktop/0.4.4 (Mac)'
-        self.assertEqual(get_client_name(req, is_browser_view=False), 'Unspecified')
+        self.assertEqual(get_client_name(req), 'ZulipDesktop')
 
-        req = Request(
+        req.META['HTTP_USER_AGENT'] = 'ZulipMobile/26.22.145 (Android 10)'
-            GET=dict(),
+        self.assertEqual(get_client_name(req), 'ZulipMobile')
-            POST=dict(),
-            META=dict(HTTP_USER_AGENT='Mozilla/bla bla bla'),
-        )
 
-        self.assertEqual(get_client_name(req, is_browser_view=True), 'website')
+        req.META['HTTP_USER_AGENT'] = 'ZulipMobile/26.22.145 (iOS 13.3.1)'
-        self.assertEqual(get_client_name(req, is_browser_view=False), 'Mozilla')
+        self.assertEqual(get_client_name(req), 'ZulipMobile')
 
-        req = Request(
+        # TODO: This should ideally be Firefox.
-            GET=dict(),
+        req.META['HTTP_USER_AGENT'] = 'Mozilla/5.0 (X11; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0'
-            POST=dict(),
+        self.assertEqual(get_client_name(req), 'Mozilla')
-            META=dict(HTTP_USER_AGENT='ZulipDesktop/bla bla bla'),
-        )
 
-        self.assertEqual(get_client_name(req, is_browser_view=True), 'ZulipDesktop')
+        # TODO: This should ideally be Chrome.
-        self.assertEqual(get_client_name(req, is_browser_view=False), 'ZulipDesktop')
+        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')
 
-        req = Request(
+        # TODO: This should ideally be Mobile Safari if we had better user-agent parsing.
-            GET=dict(),
+        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'
-            POST=dict(),
+        self.assertEqual(get_client_name(req), 'Mozilla')
-            META=dict(HTTP_USER_AGENT='ZulipMobile/bla bla bla'),
-        )
-
-        self.assertEqual(get_client_name(req, is_browser_view=True), 'ZulipMobile')
-        self.assertEqual(get_client_name(req, is_browser_view=False), 'ZulipMobile')
-
-        req = Request(
-            GET=dict(client='fancy phone'),
-            POST=dict(),
-            META=dict(),
-        )
-
-        self.assertEqual(get_client_name(req, is_browser_view=True), 'fancy phone')
-        self.assertEqual(get_client_name(req, is_browser_view=False), 'fancy phone')
 
     def test_REQ_aliases(self) -> None:
 

zerver/tests/test_docs.py

@@ -227,7 +227,8 @@ class DocPageTest(ZulipTestCase):
 
     def test_electron_detection(self) -> None:
         result = self.client_get("/accounts/password/reset/")
-        self.assertTrue('data-platform="website"' in result.content.decode("utf-8"))
+        # TODO: Ideally, this Mozilla would be the specific browser.
+        self.assertTrue('data-platform="Mozilla"' in result.content.decode("utf-8"))
 
         result = self.client_get("/accounts/password/reset/",
                                  HTTP_USER_AGENT="ZulipElectron/1.0.0")

zerver/tests/test_messages.py

@@ -1946,7 +1946,7 @@ class MessagePOSTTest(ZulipTestCase):
                                    "client": "zephyr_mirror",
                                    "to": self.mit_email("starnine")},
                                   subdomain="zephyr")
-        self.assert_json_success(result)
+        self.assert_json_error(result, "Invalid mirrored message")
 
     def test_mirrored_personal_to_someone_else(self) -> None:
         """