notif ios: Decrypt E2EE notifications#2230
Conversation
869495a to
9616f4a
Compare
9616f4a to
b5abc3b
Compare
|
Thanks! I'm not yet convinced about the approach in the rename commit: I started a CZO discussion about it: #mobile-team > E2EE: parsing code @ 💬#mobile-team > E2EE: parsing code @ 💬 |
| Future<ImprovedNotificationContent> _didReceivePushNotification(NotificationContent notifContent) async { | ||
| final parsed = EncryptedApnsPayload.fromJson(notifContent.payload.cast()); |
There was a problem hiding this comment.
What happens when this receives a legacy notification? Does it just throw?
That might have the right behavior already, because IIUC we don't actually need to do anything with the legacy notification. But we should handle it explicitly; it shouldn't be treated as an error.
gnprice
left a comment
There was a problem hiding this comment.
Thanks @rajveermalviya for building this!
Comments below. See also the chat thread for Chris's comment above.
One high-level observation, which several of these comments add detail to: the main commit:
b5abc3b77 notif ios: Decrypt E2EE notifications
is doing too many things at once. It should be broken up into several separate commits, in order to make each of its changes more readable by disentangling them from the others.
| B340EB382F5B092B007AD309 /* IosNative.g.swift in Sources */ = {isa = PBXBuildFile; fileRef = B340EB372F5B092B007AD309 /* IosNative.g.swift */; }; | ||
| B34E9F092D776BEB0009AED2 /* Notifications.g.swift in Sources */ = {isa = PBXBuildFile; fileRef = B34E9F082D776BEB0009AED2 /* Notifications.g.swift */; }; | ||
| B35E11A62F484E6800DE4085 /* GeneratedPluginRegistrant.m in Sources */ = {isa = PBXBuildFile; fileRef = 1498D2331E8E89220040F4C2 /* GeneratedPluginRegistrant.m */; }; | ||
| B378A5012F45B08F0031EFA1 /* NotificationService.appex in Embed Foundation Extensions */ = {isa = PBXBuildFile; fileRef = B378A4FA2F45B08F0031EFA1 /* NotificationService.appex */; settings = {ATTRIBUTES = (RemoveHeadersOnCopy, ); }; }; | ||
| B3D425322F6D40C200F9AE69 /* IosNative.g.swift in Sources */ = {isa = PBXBuildFile; fileRef = B340EB372F5B092B007AD309 /* IosNative.g.swift */; }; |
There was a problem hiding this comment.
Hmm, this line is added in this commit:
02704311e notif: Setup IosNativeHostApi Pigeon API for NotificationService headless Flutter engine
But that file isn't new (or even touched) in that commit; in fact it's already present in main, added originally in 1d03e57 (#2195).
What caused this line to appear in this particular commit?
Should this line have been present all along? What's been the effect of it not being present?
There was a problem hiding this comment.
The changes in `project.pbxproj` are result of adding
`IosNative.g.swift` and `IosNativeHostApi.swift` to the compile
sources of the NotificationService target, by following the below
steps:
- Open the project navigator (View > Navigators > Project).
- In the main window under TARGETS, select NotificationService.
- Open the Build Phases tab.
- Expand Compile Sources.
- Click +.
- Select the desired file.
- Click Add.
Great, thanks for adding this information.
FWIW, the list of steps has more detail than I think is strictly needed here. That's fine, though; some detail is helpful.
And the crucial part is the sentence up to the comma (plus "in Xcode", which the steps imply). That's critical particularly because it lets the reader infer an answer to these questions I asked above:
Should this line have been present all along? What's been the effect of it not being present?
(Namely: it meant those files weren't included in that service; and that was fine until we start using them there in this commit.)
| B35E11A62F484E6800DE4085 /* GeneratedPluginRegistrant.m in Sources */ = {isa = PBXBuildFile; fileRef = 1498D2331E8E89220040F4C2 /* GeneratedPluginRegistrant.m */; }; | ||
| B378A5012F45B08F0031EFA1 /* NotificationService.appex in Embed Foundation Extensions */ = {isa = PBXBuildFile; fileRef = B378A4FA2F45B08F0031EFA1 /* NotificationService.appex */; settings = {ATTRIBUTES = (RemoveHeadersOnCopy, ); }; }; | ||
| B3D425322F6D40C200F9AE69 /* IosNative.g.swift in Sources */ = {isa = PBXBuildFile; fileRef = B340EB372F5B092B007AD309 /* IosNative.g.swift */; }; | ||
| B3D425332F6D40C200F9AE69 /* IosNativeHostApi.swift in Sources */ = {isa = PBXBuildFile; fileRef = B32717682F6C49E5007682B1 /* IosNativeHostApi.swift */; }; |
There was a problem hiding this comment.
This line is added in this commit (same as the comment above):
02704311e notif: Setup IosNativeHostApi Pigeon API for NotificationService headless Flutter engine
But that file isn't new in that commit, or touched there; it was introduced in the previous commit:
9882e18e2 ios [nfc]: Move IosNativeHostApiImpl to its own Swift file
Should this line have been added in that commit? What caused it to appear in this file?
It also looks quite a lot like another line a few lines above:
B32717692F6C49E5007682B1 /* IosNativeHostApi.swift in Sources */ = {isa = PBXBuildFile; fileRef = B32717682F6C49E5007682B1 /* IosNativeHostApi.swift */; };
which does appear in that earlier commit. What's the effect of having both lines, instead of just the line that was added later? Should we have only one of them?
| /// Parsed version of an FCM message, of any plaintext type. | ||
| /// An APNs payload whose contents are encrypted end-to-end from the Zulip server. | ||
| /// | ||
| /// Once decrypted, the contents will become an [NotifMessage]. |
There was a problem hiding this comment.
nit:
| /// Once decrypted, the contents will become an [NotifMessage]. | |
| /// Once decrypted, the contents will become a [NotifMessage]. |
(similarly above. "Notif" starts with a consonant sound, while "Fcm" starts with a vowel sound: "eff cee em".)
| @@ -73,12 +73,13 @@ class PushRegistration { | |||
| final PushTokenKind tokenKind; | |||
| final String token; | |||
| final int timestamp; | |||
| // final String? iosAppId; // TODO(#1764) | |||
| final String? iosAppId; | |||
There was a problem hiding this comment.
This can be easily split out into a prep commit.
There was a problem hiding this comment.
Almost NFC, because E2EE notifications are only enabled on Android.
But the PushRegistration request will now have `ios_app_id: null`
pair in the request.
Hmm, good to flag this. (Part of the benefit of a separate commit! There's now a separate commit message, making a natural space for this sort of note.)
I think the ideal implementation here would avoid sending that. Not important, though, as long as the server currently does accept this.
There was a problem hiding this comment.
Fixed this by adding a @JsonKey(includeIfNull: false) over iosAppId.
There was a problem hiding this comment.
Great, glad that was easy to fix.
| static String titleForNotifMessage(MessageNotifMessage data, ZulipLocalizations zulipLocalizations) { | ||
| return switch (data.recipient) { | ||
| NotifMessageChannelRecipient(:var channelName?, :var topic) => | ||
| '#$channelName > ${topic.displayName}', | ||
| NotifMessageChannelRecipient(:var topic) => | ||
| '#${zulipLocalizations.unknownChannelName} > ${topic.displayName}', // TODO get stream name from data | ||
| NotifMessageDmRecipient(:var allRecipientIds) when allRecipientIds.length > 2 => | ||
| zulipLocalizations.notifGroupDmConversationLabel( | ||
| data.senderFullName, allRecipientIds.length - 2), // TODO use others' names, from data | ||
| NotifMessageDmRecipient() => | ||
| data.senderFullName, | ||
| }; | ||
| } | ||
|
|
||
| static Uri notificationUrlForNotifMessage(MessageNotifMessage data) { |
There was a problem hiding this comment.
This factoring-out can happen as its own prep commit.
| Future<void> checkNotification( | ||
| FakeAsync async, |
There was a problem hiding this comment.
This parameter doesn't seem to get used.
The use of FakeAsync is a significant complication to thinking about the tests. So if it's not getting used, best to leave it out.
| payload = { | ||
| // Only `notification_url` value is read when notification is opened. | ||
| ...fakeEncryptedApnsPayload(), | ||
| 'notification_url': notificationUrl.toString(), |
There was a problem hiding this comment.
This seems like a further sign that the original payload doesn't belong in the data used for opening the notification 🙂
| final Map<Object?, Object?> payload; | ||
| if (encrypted) { | ||
| final notificationUrl = notificationUrlForMessage(account, message).toString(); | ||
| payload = { | ||
| // Only `notification_url` value is read when notification is opened. | ||
| ...fakeEncryptedApnsPayload(), | ||
| 'notification_url': notificationUrl, | ||
| }; | ||
| } else { | ||
| payload = messageLegacyApnsPayload(message, account: account); | ||
| } | ||
|
|
||
| // Set up a value to return for | ||
| // `notificationPigeonApi.getNotificationDataFromLaunch`. | ||
| final payload = messageApnsPayload(message, account: account); |
There was a problem hiding this comment.
This logic should all get moved into a helper next to messageLegacyApnsPayload — say, called messageApnsPayload. That way (a) we don't have two copies of it, here and just above; (b) those details are out of the way when reading these two helpers.
| await checkOpenNotification( | ||
| tester, | ||
| eg.selfAccount, | ||
| eg.streamMessage(), | ||
| encrypted: false); |
There was a problem hiding this comment.
nit:
| await checkOpenNotification( | |
| tester, | |
| eg.selfAccount, | |
| eg.streamMessage(), | |
| encrypted: false); | |
| await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage(), | |
| encrypted: false); |
That's both more compact, and more parallel to the test case above — makes it easier to compare them by eye and spot the one difference.
| @@ -237,6 +275,17 @@ void main() { | |||
| await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage()); | |||
| }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); | |||
|
|
|||
| testWidgets('iOS: stream message, legacy plaintext', (tester) async { | |||
There was a problem hiding this comment.
nit: "iOS legacy plaintext" is one concept here (at least, "legacy plaintext" is only meaningful within the context of "iOS", since it's about the way we were encoding things on iOS notifications specifically), so put it in one place in the description:
| testWidgets('iOS: stream message, legacy plaintext', (tester) async { | |
| testWidgets('stream message: iOS legacy plaintext', (tester) async { |
|
@rajveermalviya How have you manually tested these changes? I just tried the following procedure, and it didn't work:
This didn't produce a notification on the device, even when I put the app into the background. Investigating by looking at the logs on chat.zulip.org and the bouncer, I found:
|
b5abc3b to
1aee319
Compare
Yes, but not with CZO/zulipchat which sends push notifications via Production APNs server. Only with a dev server which was set up to communicate with Sandbox APNs server. With local builds of the app, I haven't been able to get notifications from Production APNs server (via CZO/zulipchat) even on |
|
Great, thanks. I've now successfully manually tested this against my dev server, two ways:
(This required editing Please write down some instructions for that — they can go in the Before we merge and release this, I'll also want to manually test it against CZO (as an example of a production self-hosted server) and against Zulip Cloud (because it interacts with the bouncer in a different way from self-hosted servers). On our call today, you suggested a TestFlight build as a way to do that. |
|
(Rebased past the merged version of #2156.) |
cce23f3 to
4c5fbb6
Compare
|
Thanks for the review @gnprice! Pushed a new revision with the handling of |
gnprice
left a comment
There was a problem hiding this comment.
Thanks for the revision! That split of the old main commit into more commits — I think it split into 8 of them 🙂 — is very helpful for reviewing this.
Generally this looks good. Comments below, mostly small.
The comments that potentially affect behavior directly are:
- The subtitle on group DMs.
- What happens on legacy notifications? (Which we'll still get from old servers.)
- The
ios_app_id: null, which is probably harmless.
| B340EB382F5B092B007AD309 /* IosNative.g.swift in Sources */ = {isa = PBXBuildFile; fileRef = B340EB372F5B092B007AD309 /* IosNative.g.swift */; }; | ||
| B34E9F092D776BEB0009AED2 /* Notifications.g.swift in Sources */ = {isa = PBXBuildFile; fileRef = B34E9F082D776BEB0009AED2 /* Notifications.g.swift */; }; | ||
| B35E11A62F484E6800DE4085 /* GeneratedPluginRegistrant.m in Sources */ = {isa = PBXBuildFile; fileRef = 1498D2331E8E89220040F4C2 /* GeneratedPluginRegistrant.m */; }; | ||
| B378A5012F45B08F0031EFA1 /* NotificationService.appex in Embed Foundation Extensions */ = {isa = PBXBuildFile; fileRef = B378A4FA2F45B08F0031EFA1 /* NotificationService.appex */; settings = {ATTRIBUTES = (RemoveHeadersOnCopy, ); }; }; | ||
| B3D425322F6D40C200F9AE69 /* IosNative.g.swift in Sources */ = {isa = PBXBuildFile; fileRef = B340EB372F5B092B007AD309 /* IosNative.g.swift */; }; |
There was a problem hiding this comment.
The changes in `project.pbxproj` are result of adding
`IosNative.g.swift` and `IosNativeHostApi.swift` to the compile
sources of the NotificationService target, by following the below
steps:
- Open the project navigator (View > Navigators > Project).
- In the main window under TARGETS, select NotificationService.
- Open the Build Phases tab.
- Expand Compile Sources.
- Click +.
- Select the desired file.
- Click Add.
Great, thanks for adding this information.
FWIW, the list of steps has more detail than I think is strictly needed here. That's fine, though; some detail is helpful.
And the crucial part is the sentence up to the comma (plus "in Xcode", which the steps imply). That's critical particularly because it lets the reader infer an answer to these questions I asked above:
Should this line have been present all along? What's been the effect of it not being present?
(Namely: it meant those files weren't included in that service; and that was fine until we start using them there in this commit.)
| @@ -73,12 +73,13 @@ class PushRegistration { | |||
| final PushTokenKind tokenKind; | |||
| final String token; | |||
| final int timestamp; | |||
| // final String? iosAppId; // TODO(#1764) | |||
| final String? iosAppId; | |||
There was a problem hiding this comment.
Almost NFC, because E2EE notifications are only enabled on Android.
But the PushRegistration request will now have `ios_app_id: null`
pair in the request.
Hmm, good to flag this. (Part of the benefit of a separate commit! There's now a separate commit message, making a natural space for this sort of note.)
I think the ideal implementation here would avoid sending that. Not important, though, as long as the server currently does accept this.
| // Adapted from server implementation: | ||
| // https://github.com/zulip/zulip/blob/11bf985d1/zerver/lib/push_notifications.py#L1087-L1124 | ||
| // TODO handle subtitle for user/group/wildcard mentions | ||
| final subtitle = switch (data.recipient) { | ||
| NotifPayloadDmRecipient(:var allRecipientIds) | ||
| when allRecipientIds.length <= 2 | ||
| => '', | ||
| NotifPayloadChannelRecipient() || NotifPayloadDmRecipient() | ||
| => '${data.senderFullName}:', | ||
| }; |
There was a problem hiding this comment.
Hmm interesting.
Do we want this for group DMs? Looking at the title from titleForNotifPayload, I think we probably don't — the title will already say who it's from. So then it's only for channel messages that we want this.
I think that point also indicates that this logic should live next to titleForNotifPayload, so that it's easy to compare the two. Can be another static method right after that one.
There was a problem hiding this comment.
Actually, the title will be different for group DMs too, for example see: https://github.com/rajveermalviya/zulip-flutter/blob/297184519f9e0f5b441dbb993dd7a86d605ec2b7/test/notifications/ios_service_test.dart#L100-L106
There was a problem hiding this comment.
So that reads:
expectedTitle: "${eg.thirdUser.fullName} to you and 1 other",
expectedSubtitle: '${eg.thirdUser.fullName}:');The subtitle seems redundant there to me. The title already says "Third User to you and 1 other", so it's saying the message came from Third User. The subtitle is then just restating that, right?
(I'm not sure I fully follow you, though — I'm not sure what you mean by "will be different". What's different from what?)
There was a problem hiding this comment.
The subtitle is then just restating that, right?
Ah yes, makes sense. Updated the code to only set the subtitle for channel messages.
| /// Parses the internal notification url (on Android and iOS), that was | ||
| /// created using [buildNotificationUrl], and retrieves the information | ||
| /// required for navigation. |
There was a problem hiding this comment.
No need to spell out that this applies to both Android and iOS — that's the norm across our codebase.
| /// Parses the internal notification url (on Android and iOS), that was | |
| /// created using [buildNotificationUrl], and retrieves the information | |
| /// required for navigation. | |
| /// Parses the internal notification URL that was | |
| /// created using [buildNotificationUrl], and retrieves the information | |
| /// required for navigation. |
(also two nits fixed in that version: "URL" in caps, and cut a stray comma)
| payload = { | ||
| 'notification_url': notificationUrl.toString(), | ||
| }; | ||
| } else { | ||
| payload = messageLegacyApnsPayload(message, account: account); | ||
| } | ||
| return payload; |
There was a problem hiding this comment.
nit: simplify by just returning directly rather than set payload
| // TODO run this test on iOS too. | ||
| test('update when token changes', () => awaitFakeAsync((async) async { |
There was a problem hiding this comment.
Thanks. In general, when we add a TODO comment like this, it should include the answer to that sort of question. Otherwise the TODO isn't really useful, because a reader can't tell from looking at it what one would do to act on it, or what the goal of doing so would be.
Following the link: Hmm I see. That comment is saying that it's not clear there is such a thing as the token changing on iOS, though, or as us belatedly learning the token. In that case we wouldn't want to run these tests for iOS, because we don't need the logic there that they would exercise.
In any case, I think this isn't a TODO for the tests. There's that existing TODO in the app code, which is for us to figure out whether such logic is needed; and if the answer is that we do need it, then we'll naturally add tests for that logic when we add the logic itself.
Could be useful, though, to briefly explain why it's not running on iOS, because it does look like (and is) a test of behavior that interacts with iOS vs. Android:
| // TODO run this test on iOS too. | |
| test('update when token changes', () => awaitFakeAsync((async) async { | |
| // No corresponding iOS test, because no token updates there. | |
| test('update when token changes', () => awaitFakeAsync((async) async { |
| Future<ImprovedNotificationContent> _didReceivePushNotification(NotificationContent notifContent) async { | ||
| final parsed = EncryptedApnsPayload.fromJson(notifContent.payload.cast()); |
| if (result == null) throw Exception(); // TODO(log) | ||
|
|
||
| final (data, _) = result; | ||
| return _onNotifPayload(data, notifContent); |
There was a problem hiding this comment.
This doesn't actually do anything with notifContent, right? (Seems like it hardly could, since the data is encrypted.) Can simplify, and avoid raising a question for the reader, by cutting that out.
| import 'open_test.dart'; | ||
| import 'display_test.dart' show encryptNotification, notifPayloadNewMessage; | ||
|
|
||
| /// Encode an APNs payload into the form APNs would supply it in. |
There was a problem hiding this comment.
| /// Encode an APNs payload into the form APNs would supply it in. | |
| /// Encode a notification payload into the form APNs would supply it in. |
The input here isn't really an "APNs payload"; it's the same form of data we use on Android and iOS.
| // See NotificationOpenService (in lib/notifications/ios_service.dart). | ||
| 'notification_url': notificationUrl.toString(), |
There was a problem hiding this comment.
Let's make this key a named constant. That way looking at the references to the constant lets us directly see what other code is relying on its value.
As an example for comparison, see NotificationDisplayManager.kExtraLastMessageId.
2971845 to
de20057
Compare
|
Thanks for the revision! Generally this all looks great. One comment above at #2230 (comment) ; and in our call just now, you already mentioned you're planning to act on #2230 (comment) . Then I think that's it. |
de20057 to
d25d96a
Compare
|
Thanks for the review @gnprice! Pushed an update, PTAL. |
75a9dc8 to
3062d0d
Compare
| Future<ImprovedNotificationContent> _didReceivePushNotification(NotificationContent notifContent) async { | ||
| final parsed = EncryptedApnsPayload.fromJson(notifContent.payload.cast()); |
| // to fallback and display the notification as per the incoming push | ||
| // notification content (displaying the fields in APNs payload's | ||
| // `"alert"` object). |
There was a problem hiding this comment.
Reading this raises one question in my mind, which we can answer:
| // to fallback and display the notification as per the incoming push | |
| // notification content (displaying the fields in APNs payload's | |
| // `"alert"` object). | |
| // to fallback and display the notification as per the incoming push | |
| // notification content (displaying the fields in APNs payload's | |
| // `"alert"` object). The server sets that to say "New notification". |
|
An update on the manual testing, since #2230 (comment) : later that day, I also tested against chat.zulip.org and Zulip Cloud, using a TestFlight build as I foreshadowed there. (That produced #2275 to update the relevant docs.) Just now, I repeated the manual testing with my local dev server that I described in that comment above. Things still work. This PR and #2279 look ready to merge now apart from a couple of nits. So I'm going to fix those nits and merge. Then it's time for a release! The release will involve a TestFlight build anyway, so I'll use that for some final manual testing. If a release weren't imminent, or if I were less confident that the testing will come out OK, I'd do another TestFlight build before merging. |
This commit is a result of opening context menu on the class in Xcode's editor and then selecting "Refactor > Extract to file". This is needed for (soon to be added) NotificationService target to initialize the Pigeon API.
…less Flutter engine The changes in `project.pbxproj` are result of adding `IosNative.g.swift` and `IosNativeHostApi.swift` to the compile sources of the NotificationService target, by following the below steps: - Open the project navigator (View > Navigators > Project). - In the main window under TARGETS, select NotificationService. - Open the Build Phases tab. - Expand Compile Sources. - Click +. - Select the desired file. - Click Add.
NFC, because E2EE notifications are only enabled on Android.
…ld}NotificationUrl Making the method name platform agnostic, because in next commits we will use this internal notification URL on iOS too.
Essentially NFC because currently device registration for E2EE
notifications is disabled on iOS.
There is a regression for the E2EE notification' title compared to
legacy plaintext notifications. Where Zulip Server would include
the names of other participants in a group-DM in the alert's title,
but now the title would just say "${senderName} to you and N others".
Essentially NFC because currently device registration for E2EE notifications is disabled on iOS.
|
|
Sent #2304 to update the instructions, @chrisbobbe PTAL. |
Stacked on top of #2279.