api: Send simple User-Agent header #471

chrisbobbe · 2023-12-30T00:23:29Z

Revises: #450
Fixes: #406
Fixes: #460

gnprice

Thanks @chrisbobbe for picking this up! I edited the description to cross-reference it with the previous PR #450.

The code generally all looks good; small comments below.

What manual testing have you done to confirm it resolves #460, and resolves #440 for older servers? (For the latter, I guess it'd suffice to test on a current server but without passing read_by_sender.)

gnprice · 2023-12-30T03:06:58Z

lib/api/core.dart

+    // Putting "ZulipMobile" in the string helps users on pre-8 servers:
+    //   - When a user sends a message from the app,
+    //     the message is read instead of unread, avoiding #440.
+    //     (With servers 8+, this is accomplished
+    //     with a new param in the send-message API; see #456.)
+    //   - Login notifications say something specific instead of
+    //     "an unknown browser on an unknown operating system", avoiding #460.


Does this latter part work if we say "ZulipFlutter" instead of "ZulipMobile"? (If not, then we might need further server changes but we'll want to figure those out.)

Yes, I just tested manually by changing the string from 'ZulipMobile (actually ZulipFlutter)' to just 'ZulipFlutter' and seeing what the login-notification email said, by visiting /emails on the dev server. It said:

Device: Zulip on an unknown operating system.

, the same as it does with 'ZulipMobile (actually ZulipFlutter)' (#471 (comment)).

Er, and I've just tested this on a dev server at 7.x (zulip/zulip@c4a8264), and got the same result. I realized that my original test here must have been done on an 8+ dev server.

gnprice · 2023-12-30T03:09:03Z

test/widgets/content_test.dart

-      check(await actualAuthHeader(tester, Uri.parse('https://chat.example/image.png')))
-        .isNotNull().equals(authHeaders['Authorization']!);
+      check(await actualHeaders(tester, Uri.parse('https://chat.example/image.png')))
+        ..['Authorization'].single.equals(authHeaders['Authorization']!)
+        ..['User-Agent'].single.equals(userAgentHeader()['User-Agent']!);


Rather than check only these two headers, I think I'd prefer to continue to have a check that there aren't any unexpected additional headers. That can be done by a deepEquals on the headers map.

chrisbobbe · 2024-01-03T01:42:12Z

manual testing have you done to confirm it resolves #460

Oh, hmm, doing this now…

Visiting /emails/ on my dev server, this is what I see when I log into an account from zulip-flutter before these PR commits:

Device: An unknown browser on an unknown operating system.

And this is what I see at the tip of the PR branch:

Device: Zulip on an unknown operating system.

Unfortunate that it doesn't even say Android or iOS, but it makes sense; in this first pass, the user-agent header doesn't carry that information (#467).

gnprice · 2024-01-03T02:22:57Z

Unfortunate that it doesn't even say Android or iOS, but it makes sense; in this first pass, the user-agent header doesn't carry that information (#467).

Cool. I think the change from "An unknown browser" to "Zulip" is enough to say this resolves #460 — I think that "unknown browser" is probably the most disconcerting part.

There'll still be a sort of followup to #460 which is to fix the operating-system part. But we can track that as just a note on #467 of something to test in a PR that fixes it.

chrisbobbe · 2024-01-03T02:33:27Z

Ah hmm, well here's a problem. The string used in this PR, 'ZulipMobile (actually ZulipFlutter)', doesn't actually parse:

https://github.com/zulip/zulip/blob/08bee0f1809a8a6a074f77d40fd67e10f665377e/zerver/lib/user_agent.py

pattern = re.compile(
    """^ (?P<name> [^/ ]* [^0-9/(]* )
    (/ (?P<version> [^/ ]* ))?
    ([ /] .*)?
    $""",
    re.X,
)

As a consequence, /register is erroring.

gnprice · 2024-01-03T02:42:00Z

As a consequence, /register is erroring.

That seems pretty busted. The server will fail to respond to a request if its bundle of heuristics for interpreting user-agent strings doesn't understand your user-agent?

chrisbobbe · 2024-01-03T02:44:08Z

2024-01-03 02:31:42.019 ERR  [django.request] Internal Server Error: /api/v1/register
Traceback (most recent call last):
  File "/srv/zulip/zerver/middleware.py", line 402, in process_exception
    raise exception  # Ensure correct sys.exc_info().
  File "/srv/zulip-py3-venv/lib/python3.8/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/srv/zulip/zerver/lib/rest.py", line 39, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "/srv/zulip-py3-venv/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 56, in wrapper_view
    return view_func(*args, **kwargs)
  File "/srv/zulip/zerver/lib/rest.py", line 203, in rest_dispatch
    return target_function(request, **kwargs)
  File "/srv/zulip-py3-venv/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 56, in wrapper_view
    return view_func(*args, **kwargs)
  File "/srv/zulip/zerver/decorator.py", line 779, in _wrapped_func_arguments
    raise err
  File "/srv/zulip/zerver/decorator.py", line 776, in _wrapped_func_arguments
    return view_func(request, user_profile, *args, **kwargs)
  File "/srv/zulip/zerver/lib/request.py", line 464, in _wrapped_req_func
    return_value = req_func(request, *args, **kwargs)
  File "/srv/zulip/zerver/views/events_register.py", line 133, in events_register_backend
    pronouns_field_type_supported = is_pronouns_field_type_supported(
  File "/srv/zulip/zerver/lib/compatibility.py", line 157, in is_pronouns_field_type_supported
    if version_lt(user_agent["version"], FIRST_VERSION_TO_SUPPORT_PRONOUNS_FIELD):
  File "/srv/zulip/zerver/lib/compatibility.py", line 69, in version_lt
    num1, rest1 = pop_numerals(ver1)
  File "/srv/zulip/zerver/lib/compatibility.py", line 46, in pop_numerals
    match = re.search(r"^( \d+ (?: \. \d+ )* ) (.*)", ver, re.X)
  File "/usr/lib/python3.8/re.py", line 201, in search
    return _compile(pattern, flags).search(string)
TypeError: expected string or bytes-like object

Aha:

def is_pronouns_field_type_supported(user_agent_str: Optional[str]) -> bool:
    # In order to avoid users having a bad experience with these
    # custom profile fields disappearing after applying migration
    # 0421_migrate_pronouns_custom_profile_fields, we provide this
    # compatibility shim to show such custom profile fields as
    # SHORT_TEXT to older mobile app clients.
    #
    # TODO/compatibility(7.0): Because this is a relatively minor
    # detail, we can remove this compatibility hack once most users
    # have upgraded to a sufficiently new mobile client.
    if user_agent_str is None:
        return True

    user_agent = parse_user_agent(user_agent_str)
    if user_agent["name"] != "ZulipMobile":
        return True

    FIRST_VERSION_TO_SUPPORT_PRONOUNS_FIELD = "27.192"
    if version_lt(user_agent["version"], FIRST_VERSION_TO_SUPPORT_PRONOUNS_FIELD):
        return False

    return True

gnprice · 2024-01-03T02:50:40Z

Hmm, I see.

Let's discuss in #api design how to pick a value that navigates the various server-side hacks:
https://chat.zulip.org/#narrow/stream/378-api-design/topic/zulip-flutter.20user.20agent/near/1712186

chrisbobbe · 2024-01-17T07:20:57Z

Revision pushed, following CZO discussion: https://chat.zulip.org/#narrow/stream/378-api-design/topic/zulip-flutter.20user.20agent/near/1712928

What manual testing have you done to confirm it resolves #460, and resolves #440 for older servers? (For the latter, I guess it'd suffice to test on a current server but without passing read_by_sender.)

See: #471 (comment)

Also, while I had that 7.x dev server handy, I went ahead and tested the 'ZulipMobile/flutter' override on send-message requests, as another way to confirm that it fixes #440 for older servers. With the override, the messages were marked as read; without it, they were not marked as read.

gnprice

Thanks for the revision! This all looks good, with one nit below.

Since you're out today, I'll just fix that and merge.

gnprice · 2024-02-06T00:04:31Z

test/api/route/messages_test.dart

+          expectedBodyFields: {
+            'type': 'stream',
+            'to': streamId.toString(),
+            'topic': topic,
+            'content': content,
+          },
+          readBySender: null,
+          expectedUserAgent: 'ZulipMobile/flutter');


nit:

Suggested change

expectedBodyFields: {

'type': 'stream',

'to': streamId.toString(),

'topic': topic,

'content': content,

},

readBySender: null,

expectedUserAgent: 'ZulipMobile/flutter');

readBySender: null,

expectedBodyFields: {

'type': 'stream',

'to': streamId.toString(),

'topic': topic,

'content': content,

},

expectedUserAgent: 'ZulipMobile/flutter');

because readBySender specifies what to do, and so comes logically before what to expect from the result

…mage

This will make it easy to check the 'User-Agent' header in these tests, coming up.

Fixes: zulip#406 Fixes: zulip#460 [chris: changed commit message; changed user-agent string; included in image requests]

The one caller passes read_by_sender, so these tests are now more representative of how the binding actually gets exercised. Next, we'll have the binding condition something on whether or not read_by_sender is passed, and that'll call for tests that omit it, which we'll add.

This resolves zulip#440 for older servers.

chrisbobbe added the a-api Implementing specific parts of the Zulip server API label Dec 30, 2023

chrisbobbe requested review from gnprice and sirpengi December 30, 2023 00:23

gnprice reviewed Dec 30, 2023

View reviewed changes

gnprice mentioned this pull request Dec 30, 2023

Send hard-coded User-Agent in requests #450

Closed

gnprice mentioned this pull request Jan 3, 2024

In user-agent, include platform, platform version, and app version #467

Closed

chrisbobbe force-pushed the pr-simple-user-agent branch from ac00529 to 5363be5 Compare January 17, 2024 07:19

gnprice reviewed Feb 6, 2024

View reviewed changes

chrisbobbe and others added 5 commits February 5, 2024 16:07

content [nfc]: Refactor auth-header inclusion in RealmContentNetworkI…

1c23682

…mage

content test [nfc]: Change actualAuthHeader to return all headers

7cbb743

This will make it easy to check the 'User-Agent' header in these tests, coming up.

api: Send simple User-Agent header

4d5b3e1

Fixes: zulip#406 Fixes: zulip#460 [chris: changed commit message; changed user-agent string; included in image requests]

api: In send-message requests, override user agent for old servers

a0f4462

This resolves zulip#440 for older servers.

gnprice force-pushed the pr-simple-user-agent branch from 5363be5 to a0f4462 Compare February 6, 2024 00:08

gnprice merged commit a0f4462 into zulip:main Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

api: Send simple User-Agent header #471

api: Send simple User-Agent header #471

Uh oh!

chrisbobbe commented Dec 30, 2023 •

edited by gnprice

Loading

Uh oh!

gnprice left a comment

Uh oh!

gnprice Dec 30, 2023

Uh oh!

chrisbobbe Jan 3, 2024

Uh oh!

chrisbobbe Jan 17, 2024

Uh oh!

gnprice Dec 30, 2023

Uh oh!

chrisbobbe commented Jan 3, 2024

Uh oh!

gnprice commented Jan 3, 2024

Uh oh!

chrisbobbe commented Jan 3, 2024 •

edited

Loading

Uh oh!

gnprice commented Jan 3, 2024

Uh oh!

chrisbobbe commented Jan 3, 2024

Uh oh!

gnprice commented Jan 3, 2024

Uh oh!

chrisbobbe commented Jan 17, 2024

Uh oh!

gnprice left a comment

Uh oh!

gnprice Feb 6, 2024

Uh oh!

Uh oh!

api: Send simple User-Agent header #471

api: Send simple User-Agent header #471

Uh oh!

Conversation

chrisbobbe commented Dec 30, 2023 • edited by gnprice Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 30, 2023

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jan 3, 2024

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jan 17, 2024

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 30, 2023

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jan 3, 2024

Uh oh!

gnprice commented Jan 3, 2024

Uh oh!

chrisbobbe commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Jan 3, 2024

Uh oh!

chrisbobbe commented Jan 3, 2024

Uh oh!

gnprice commented Jan 3, 2024

Uh oh!

chrisbobbe commented Jan 17, 2024

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisbobbe commented Dec 30, 2023 •

edited by gnprice

Loading

chrisbobbe commented Jan 3, 2024 •

edited

Loading