-
Notifications
You must be signed in to change notification settings - Fork 310
Send hard-coded User-Agent in requests #450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
And update Flutter's supporting libraries to match. Also remove unnecessary imports of `dart:ui`. Upstream Flutter (in flutter/flutter@c6741614041 ) changed `flutter/material.dart` to additionally export many types from `dart:ui`. This caused a lot of our imports to trigger an analysis warning of `unnecessary_import`.
0e208d4
to
f58d439
Compare
f58d439
to
5c4d4f2
Compare
It seemed impossible to detangle the broken tests introduced after calling |
This is required for the server to automatically mark sent messages as read. See `sent_by_human` in `zulip/zerver/models.py`. Fixes: zulip#440
5c4d4f2
to
9bb9d81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sirpengi! Comments below.
Since the explicit flag to control the read-by-self behavior is already built, the main thing we can do for #440 is to just use that. That should also be pretty simple, I think.
Then we'll still want to do this as a follow-up, in order to support 7.x and older servers. But we'll no longer need to feel rushed for it.
// TODO(#453): Create real User-Agent string | ||
Map<String, String> userAgentHeader() { | ||
return { | ||
'User-Agent': 'ZulipMobile/?.?.? (Android 14)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saying "Android 14" means making up bogus, untruthful, data — we might be on some other Android version, or on iOS — so let's avoid that.
By contrast saying "?.?.?" for the version number is fine, at least as a stopgap, because it's honest about what this code doesn't know.
// TODO(#453): Create real User-Agent string | ||
Map<String, String> userAgentHeader() { | ||
return { | ||
'User-Agent': 'ZulipMobile/?.?.? (Android 14)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's work in something here to make clear this is actually the Flutter app, not the RN app. E.g., saying "ZulipFlutter".
Tim suggests at #453 (comment) that it's enough for "ZulipMobile" to appear somewhere in the value, not necessarily at the start. Can we add something like "(like ZulipMobile)" at the end instead, and put "ZulipFlutter" at the start?
Or if we do need it at the start, in the venerable tradition of every modern browser claiming to be Mozilla 5.0:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent
then in that same venerable tradition I hope we can at least put the more informative name somewhere later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with live server and dug through https://github.com/zulip/zulip/blob/main/zerver/lib/user_agent.py . The User-Agent is parsed with this:
pattern = re.compile(
"""^ (?P<name> [^/ ]* [^0-9/(]* )
(/ (?P<version> [^/ ]* ))?
([ /] .*)?
$""",
re.X,
)
and the whitelist from https://github.com/zulip/zulip/blob/main/zerver/models/clients.py is:
sending_client
in (
"zulipandroid",
"zulipios",
"zulipdesktop",
"zulipmobile",
"zulipelectron",
"zulipterminal",
"snipe",
"website",
"ios",
"android",
)
or "desktop app" in sending_client
# Since the vast majority of messages are sent by humans
# in Zulip, treat test suite messages as such.
or (sending_client == "test suite" and settings.TEST_SUITE)
To trigger a match we could not start the string with "ZulipFlutter". I think given the options we should present as "ZulipMobile" and stick on a "(like ZulipMobile)" at the end.
We could possibly start with "ZulipFlutter; like desktop app" or "ZulipFlutter(like desktop app)" (but NOT "ZulipFlutter (like desktop app)") but I don't like these options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's do a string like ZulipMobile/1.2.3 (Android 14) (actually ZulipFlutter)
. Where instead of "Android 14" we have the actual OS name and version (and we can leave that part out for a first pass), and instead of "1.2.3" we have the actual version (and we can leave that out too for a first pass).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, better yet: once we do have the version number, it'll be from a different sequence from the zulip-mobile version numbers anyway. So rather than saying "ZulipMobile/1.2.3", we can let the version number remain absent at that spot and instead say a string like ZulipMobile (Android 14) (actually ZulipFlutter/1.2.3)
.
#456 was opened for support of the new |
With #460 having been prioritized (it has the |
Zulip server will automatically mark messages as read if the request appears to come from a recognized client. We simply need to identify using the
User-Agent
header as an approved client.See
sent_by_human
in https://github.com/zulip/zulip/blob/main/zerver/models.pyThis PR has two commits, the first is a simple hard-coded user agent that will result in the desired behavior. The second calculates the user agent from the device, but this might not be how we want it calculated (espeically since
ZulipBinding
also contains an interface into device info).A further improvement would be to get upstream to accept
ZulipFlutter
as a legitimate user agent so we aren't reusing theZulipMobile
name.Fixes: #440