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: """