notifications: normalize case of realm_uri and account.realm #3671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Thanks for tracking this down! The realm URL that appears in the notification data is the one the server has for itself (for that realm). Thinking about... you know what, I'm going to move this to chat. 🙂 |
|
It's worth noting that the right fix here is probably not to do any sort of URL mangling: it's to ask the server what it calls itself at login (or thereabouts), then use that as a comparand. We've been getting a number of Sentry errors indicating that people are connecting to renamed servers. |
|
Sorry, it seems I dropped this thread. Added more just now in chat after you pinged me in person; thanks! Sounds like we both agree on the right direction for a complete fix: we'd drop any attempt at canonicalizing the URL ourselves, and just quote the server's idea of it from That shouldn't be super complicated, but this commit is very simple, so it's a fine quick mitigation. Code LGTM; my only comments are
|
The Zulip server's `realm_uri` may not match `account.realm`, for a number of reasons. One of the more banal ones is that Zulip servers generally send `realm_uri` in lowercase, as is customary for hostnames... but users will often type caPITAL LETTERS IN THE REAlm field at initial logon. (This being a mobile app, though, it more often results from autocorrect autocapitalizating than from fumble-fingering Caps Lock.) The proper fix is to compare `realm_uri` with the equivalent token we get from `server_settings` at logon. In the interim, we can stem the flood of unhelpful errors in Sentry somewhat just by making the realm lowercase.
rk-for-zulip
force-pushed
the
case-normalize-realm
branch
from
d94795e
to
95947de
Compare
|
🏓 |
gnprice
added a commit
to gnprice/zulip-mobile
that referenced
this pull request
Oct 28, 2020
TODO file a bug for this; find corresponding reports and complaints, to reply to them when it's fixed. We have a bug where trying to open a notification doesn't actually navigate to the conversation. I think this may currently affect all notifications. The bug is a bit like a revival of zulip#3567: we get the error "notification realm_uri not found in accounts", and it turns out that all the accounts have URLs with a trailing slash `/`, while the `realm_uri` value in the notification doesn't. On further inspection, it looks like this was introduced when we started using a URL object for `realm` values, in 865914f. Previously, since the fix for zulip#3567, we'd stripped trailing slashes from `realm` values, which were URL strings. But: > new URL('https://example').toString() 'https://example/' parsing as a URL object and converting that to a string normalizes the URL, and one thing that normalization does is *add* a trailing slash to a URL like our realm URLs (or in general, fill in the path as `/` if empty.) When a `realm_uri` with no slash is compared to one of those, it never matches. Fix the issue by doubling down on parsing as URL objects. As a side effect, this normalizes case in the URL's host (and scheme). We'd previously discussed doing that, at zulip#3671 and here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm.20URL/near/795201 and concluded that parsing as URL objects would be the cleanest way. We didn't then have an appropriate `URL` implementation handy, but now we do. :-)
gnprice
added a commit
that referenced
this pull request
Oct 29, 2020
This fixes #4290, a regression in the last release, where trying to open a notification doesn't actually navigate to the conversation. The bug is a bit like a revival of #3567: we get the error "notification realm_uri not found in accounts", and it turns out that all the accounts have URLs with a trailing slash `/`, while the `realm_uri` value in the notification doesn't. On further inspection, it looks like this was introduced when we started using a URL object for `realm` values, in 865914f. Previously, since the fix for #3567, we'd stripped trailing slashes from `realm` values, which were URL strings. But: > new URL('https://example').toString() 'https://example/' parsing as a URL object and converting that to a string normalizes the URL, and one thing that normalization does is *add* a trailing slash to a URL like our realm URLs (or in general, fill in the path as `/` if empty.) When a `realm_uri` with no slash is compared to one of those, it never matches. Fix the issue by doubling down on parsing as URL objects. As a side effect, this normalizes case in the URL's host (and scheme). We'd previously discussed doing that, at #3671 and here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm.20URL/near/795201 and concluded that parsing as URL objects would be the cleanest way. We didn't then have an appropriate `URL` implementation handy, but now we do. :-) Fixes: #4290
|
We're now accomplishing this by parsing both URLs as In the chat thread linked above, we'd discussed that as the ideal way to solve this problem. At the time we didn't have a useful |
abhi0504
pushed a commit
to abhi0504/zulip-mobile
that referenced
this pull request
Nov 24, 2020
Thanks as always to our kind volunteer translators. i18n: Sync recently-added message strings across languages. webview build: Spell stdin as `-` for reading rsync filter rules. On Windows (in Git Bash) there's no /dev/stdin; but this works instead, as we learned here: https://chat.zulip.org/#narrow/stream/48-mobile/topic/issue/near/1047294 (Things still aren't working there as a whole, but we seem to get past one error and reach another one.) Conversely, this exact construct `--filter='. -'` appears in an example in the rsync man page, even at the ancient rsync 2.6.9 that Apple provides on macOS. Suggested-by: Anders Kaseorg <[email protected]> README: Migrate Travis badge to travis-ci.com. Signed-off-by: Anders Kaseorg <[email protected]> android notif: Correctly stringify pmUsers to fix navigation to group PMs. Navigation to a group PM on pressing a notification was broken because pmUsers was incorrectly stringified in GroupPm.getPmUsersString. E.g., for a group PM among user IDs 13313, 13434, and 13657, it would stringify to (newline added for readability): "GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657])" It should instead stringify to "13313, 13434, 13657". (Later in this series of commits, we remove the space.) Fix and add a test. notif tests: Ensure tests pass with representative pm_users values. To be reverted in the next commit. In the previous commit, we changed the return value of GroupPm.getPmUsersString in our Kotlin code from garbage separated by ', ' to numbers separated by ', '. This commit aims to prove that ', '-separated numbers will be handled correctly, at least as far as our tests can tell. But we really want it to be ','-separated (no space), which we do in the next commit. notif: Separate ids in pm_users for group PMs with ',' instead of ', '. ', ' would have been handled correctly, but seemingly by accident; in getNarrowFromNotificationData, pm_users was split on ',' to give ['1', ' 2', ' 3'] (note the spaces), then each element of that array was converted to a number. Also, replace the confusing + syntax, as in +idStrs[i], with parseInt. logging jsdoc: Move "see also" before parameters, to fix parse. When writing a call to a function that has jsdoc, VS Code shows a handy popup with the documentation. It shows first the text for the parameter you're currently typing, then the text for the function as a whole. That popup was showing the "See also" as part of the last parameter's documentation, rather than that for the function as a whole. In particular this means it was only visible when typing the last parameter. Fix the jsdoc parse, by moving everything that isn't part of a parameter's documentation to before the first @param marker. notif: Normalize realm_uri by parsing it as a URL. This fixes zulip#4290, a regression in the last release, where trying to open a notification doesn't actually navigate to the conversation. The bug is a bit like a revival of zulip#3567: we get the error "notification realm_uri not found in accounts", and it turns out that all the accounts have URLs with a trailing slash `/`, while the `realm_uri` value in the notification doesn't. On further inspection, it looks like this was introduced when we started using a URL object for `realm` values, in 865914f. Previously, since the fix for zulip#3567, we'd stripped trailing slashes from `realm` values, which were URL strings. But: > new URL('https://example').toString() 'https://example/' parsing as a URL object and converting that to a string normalizes the URL, and one thing that normalization does is *add* a trailing slash to a URL like our realm URLs (or in general, fill in the path as `/` if empty.) When a `realm_uri` with no slash is compared to one of those, it never matches. Fix the issue by doubling down on parsing as URL objects. As a side effect, this normalizes case in the URL's host (and scheme). We'd previously discussed doing that, at zulip#3671 and here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm.20URL/near/795201 and concluded that parsing as URL objects would be the cleanest way. We didn't then have an appropriate `URL` implementation handy, but now we do. :-) Fixes: zulip#4290 UserItem [nfc]: Take user as one structured object. This will help us switch from emails to user IDs in downstream bits of code. Also adjust several of these call sites to use user IDs for `key`, rather than emails. UserItem [nfc]: Pass whole user to callback, rather than email. This allows UserItem call sites whose callbacks are ready to work in terms of user IDs to do so without workarounds. At the same time, passing whole user objects rather than *just* IDs allows other call sites to continue to use emails without similar, inverse workarounds. notif: Always sort user IDs in pm_users. We already ensure this in the Android case (in FcmMessage.kt); do so in the iOS case too, and document it in the type. In practice the list should already have always been sorted: the server sends it in that form, and has always done so since the pm_users field was introduced in server commit 1.7.0-2360-g693a9a5e7. (To see this in the history, try the following Git commands: git log -L :get_message_payload:zerver/lib/push_notifications.py git log -L :huddle_users:zerver/lib/message.py .) So the only way this could have gone wrong is if a rogue server changed that behavior for some reason; and the main effect of this commit is really just to document this invariant. narrow [nfc]: Document more details on identifying group PMs. Which turned up a couple of bugs! We'll fix those later in this series. example data: Take sender and recipients as pmMessage arguments. As demonstrated, this allows callers to customize these a lot more cleanly than they can by overriding the actual message properties directly. There are a few call sites we don't update here, in narrowsReducer-test.js; that file hasn't yet been upgraded to be well-typed, and so those call sites don't have real User objects to provide. example data [nfc]: Use cleaner workaround for Flow "unsealed" issue. We discovered this nicer one after having used the other one here. Reminded of the contrast in discussion on other changes in this file: zulip#4294 (comment) types: Make some more indexer-using object types inexact. I just ran into this issue with CaughtUpState when making another change. Apply the workaround there and on the remaining example in this file, and mark all instances with a conditional TODO.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
account.realmis an only-lightly-normalized form of whatever theuser typed in; what little normalization is done does not include
case-folding.
On the other hand, based on errors in Sentry, the notification's
provided URL is case-normalized.
N.B.: case-normalization may be destructive to non-ASCII URLs or in
Turkish locales.
Non-ASCII URL comparison is probably already broken, because we
don't do punycode encoding on our end.
Turkish locale users with ASCII realm URLs should be mostly
unaffected, unless they have typed a capital (dotless) I in their
realm URL. In this case they will need to replace it with a
lowercase (dotted) i.