From ccf63ac66bdaa221c8859f711f8e7d53a834fc58 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Sun, 8 Mar 2020 13:12:38 -0700 Subject: [PATCH] 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 | 2 +- zerver/decorator.py | 37 +++++++++----------- zerver/tests/test_decorators.py | 62 ++++++++++----------------------- zerver/tests/test_docs.py | 3 +- zerver/tests/test_messages.py | 2 +- 5 files changed, 40 insertions(+), 66 deletions(-) diff --git a/zerver/context_processors.py b/zerver/context_processors.py index d95b7cd61a..0b63c5dd68 100644 --- a/zerver/context_processors.py +++ b/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, diff --git a/zerver/decorator.py b/zerver/decorator.py index 01833ba226..8f06520755 100644 --- a/zerver/decorator.py +++ b/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 - # 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 user_agent["name"].startswith("Zulip"): - # Avoid changing the client string for browsers, but let - # the Zulip desktop and mobile apps be themselves. - 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" + 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, *, 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: diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index 4a1ee6cfd2..54b60feace 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -55,56 +55,32 @@ import ujson class DecoratorTestCase(TestCase): def test_get_client_name(self) -> None: - class Request: - def __init__(self, GET: Dict[str, str], POST: Dict[str, str], META: Dict[str, str]) -> None: - self.GET = GET - self.POST = POST - self.META = META + req = HostRequestMock() + self.assertEqual(get_client_name(req), 'Unspecified') - req = Request( - GET=dict(), - POST=dict(), - META=dict(), - ) + 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(get_client_name(req, is_browser_view=True), 'website') - self.assertEqual(get_client_name(req, is_browser_view=False), 'Unspecified') + req.META['HTTP_USER_AGENT'] = 'ZulipDesktop/0.4.4 (Mac)' + self.assertEqual(get_client_name(req), 'ZulipDesktop') - req = Request( - GET=dict(), - POST=dict(), - META=dict(HTTP_USER_AGENT='Mozilla/bla bla bla'), - ) + req.META['HTTP_USER_AGENT'] = 'ZulipMobile/26.22.145 (Android 10)' + self.assertEqual(get_client_name(req), 'ZulipMobile') - self.assertEqual(get_client_name(req, is_browser_view=True), 'website') - self.assertEqual(get_client_name(req, is_browser_view=False), 'Mozilla') + req.META['HTTP_USER_AGENT'] = 'ZulipMobile/26.22.145 (iOS 13.3.1)' + self.assertEqual(get_client_name(req), 'ZulipMobile') - req = Request( - GET=dict(), - POST=dict(), - META=dict(HTTP_USER_AGENT='ZulipDesktop/bla bla bla'), - ) + # 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(get_client_name(req, is_browser_view=True), 'ZulipDesktop') - self.assertEqual(get_client_name(req, is_browser_view=False), 'ZulipDesktop') + # 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') - req = Request( - GET=dict(), - POST=dict(), - 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') + # 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') def test_REQ_aliases(self) -> None: diff --git a/zerver/tests/test_docs.py b/zerver/tests/test_docs.py index ed289eb66b..ddd684d72e 100644 --- a/zerver/tests/test_docs.py +++ b/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") diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py index 363ef0350d..1fbe9f343e 100644 --- a/zerver/tests/test_messages.py +++ b/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: """