Introduce NotifPayload types; split E2EE and legacy plaintext payload parsing by rajveermalviya · Pull Request #2279 · zulip/zulip-flutter

rajveermalviya · 2026-04-17T17:28:16Z

Pre-requisite for #2230.

chrisbobbe

Thanks! Is there an obstacle to moving all the FCM-message classes, for both protocols, to a new file like fcm_message.dart, as I proposed in #mobile-team > E2EE: parsing code @ 💬? I think it would be valuable for lib/api/notifications.dart to be platform-agnostic, as I mentioned there.

Comments below from skimming the first few commits; then I'll look closer on the next round.

chrisbobbe · 2026-04-17T17:43:05Z

@@ -303,6 +303,284 @@ class RemoveFcmMessage extends FcmMessageWithIdentity {
  Map<String, dynamic> toJson() => _$RemoveFcmMessageToJson(this);
 }

+//|//////////////////////////////////////////////////////////////
+// Types for parsing only legacy plaintext notification payloads.


notif [nfc]: Add LegacyFcmMessage types, parsing only legacy plaintext notifications

Commit-message nit:

NFC, because this commit only adds code for parsing the payloads (and related tests) but doesn't use them parse any notifications yet.

should say "doesn't use them to parse"

chrisbobbe · 2026-04-17T17:53:02Z

-      case 'message': return MessageFcmMessage.fromJson(json);
-      case 'remove': return RemoveFcmMessage.fromJson(json);
-      default: return UnexpectedFcmMessage.fromJson(json);
+sealed class LegacyFcmMessage implements NotifPayload {


This LegacyFcmMessage class has this note in its dartdoc:

/// For partial API docs, see: /// https://zulip.com/api/mobile-notifications

That doc isn't a source of truth for the legacy format, right (it only covers E2EE); probably we need to point to something in the server implementation, at a specific commit, instead, if anything?

chrisbobbe · 2026-04-17T19:22:02Z

-/// Base class for [FcmMessage]s that identify what Zulip account they're for.
+/// Base class for [LegacyFcmMessage]s that identify what Zulip account they're for.
 ///
 /// This includes all known types of FCM messages from Zulip
-/// (all [FcmMessage] subclasses other than [UnexpectedFcmMessage]),
+/// (all [LegacyFcmMessage] subclasses other than [UnexpectedLegacyFcmMessage]),
 /// and it seems likely that it always will.
-sealed class FcmMessageWithIdentity extends FcmMessage {
+sealed class LegacyFcmMessageWithIdentity extends LegacyFcmMessage implements NotifPayloadWithIdentity {


The comment "This includes all known types of FCM messages […] and it seems likely that it always will" is wrong because of the FCM message in the E2EE form, which doesn't have these realmUrl and realmName fields. (That information is in the encrypted payload.)

chrisbobbe · 2026-04-17T20:20:09Z

+/// A [LegacyFcmMessage] of a type (a value of `event`) we didn't know about.
+class UnexpectedLegacyFcmMessage extends LegacyFcmMessage implements UnexpectedNotifPayload {


There's a category of "unexpected FCM message" whose data flow becomes less correct with this change: an FCM message whose form (pre-decryption) we don't recognize in either protocol, but because of server/client disagreements about current protocols rather than legacy ones. From reading code, it looks like those would be pigeonholed into this UnexpectedLegacyFcmMessage class, which isn't ideal.

Is this "FCM-legacy-and-unexpected" category useful? What if we kept the "FCM-unexpected" category? Probably the natural way to do that is by having an FcmMessage base class representing any FCM message, with subclasses for:

expected-E2EE

expected-'message' (legacy, marked in the class's name or even just dartdoc)

expected-'remove' (legacy)

unexpected, without a "legacy" mark; a catch-all for anything not matching the other 3 forms

Possibly the legacy forms would benefit from an intermediate subclass under FcmMessage; not sure.
Not sure yet what all the right answers are :) and I think it would help to see all the FCM code together in a separate file, as I proposed in #mobile-team > E2EE: parsing code @ 💬.

Regarding separate file, see #2279 (comment).

Ah, I'd missed that this was still open — thanks for flagging at #2279 (comment) below.

To make sure I'm not missing an aspect of this: this doesn't currently affect the app's behavior, right? For an FCM message in that category, the current revision of this PR takes it to this line:

case UnexpectedLegacyFcmMessage(): return; // TODO(log)

so the FCM message effectively gets dropped, and I think that's also the behavior we'd want to have after a refactor of this sort.

(Then the // TODO(log) marks that ideally we'd report that error through something like Sentry; and if we did that, then we'd want that error to correctly say "unexpected FCM message" without claiming it's necessarily the legacy protocol. So that's a future stage where the behavior would diverge on account of this point.)

Pushed an added commit that I think addresses this — PTAL.

d185f409f notif [nfc]: Clarify an unexpected FCM message might not be legacy

Yep, that looks right! Thanks!

rajveermalviya · 2026-04-20T13:21:07Z

Thanks for the review @chrisbobbe! Pushed an update, PTAL.

Is there an obstacle to moving all the FCM-message classes, for both protocols, to a new file like fcm_message.dart

Yes, (as mentioned in ce84854) basically there are some sealed types (LegacyFcmMessageWithIdentity, LegacyFcmMessageRecipient, LegacyFcmMessage) that implement non-legacy FcmMessage* types (which gets renamed to NotifPayload* in later commits). And if we move the LegacyFcmMessage* types to a new file, we would need to instead make them abstract basically losing the ability to use exhaustive switch statements. So, that's why I instead opted to keep them in the same file.

gnprice

Thanks to you both! This split definitely looks like an improvement to me. Comments below.

gnprice · 2026-04-21T02:48:30Z

-      default: return UnexpectedFcmMessage.fromJson(json);
+/// See pre-E2EE server implementation for reference:
+///   https://github.com/zulip/zulip/blob/10.x/zerver/lib/push_notifications.py#L963
+sealed class LegacyFcmMessage implements NotifPayload {


This class and its subclasses are based on an earlier version of this file, right? It'd be good to make that explicit in the commit message (and say which version you were using) — that's helpful for understanding the reasoning behind all this code.

From looking at the log and taking a couple of diffs, it looks like the version you started from was
b2e41f7. Is that right?

Ah but with realmName added (per aacf52a / #2136). Perhaps other things? It'd be good to document just what your sources were. 🙂

This class and its subclasses are based on an earlier version of this file, right?

Correct, added the links for the commits I referred to while implementing this.

Most of this parsing code is from an earlier version of this file, basically before #2074 added changes for handling E2EE notifications, specifically referred to the following commit: https://github.com/zulip/zulip-flutter/commit/86c8a1cf9

Great, thanks for adding this.

Is that commit 86c8a1c really the starting point, though? The version you added has a bunch of changes that happened in the 6 commits after that:
59bd6c9 api/notif [nfc]: Move type converters to be explicit per field
700fe0b api/notif [nfc]: Rename message ID fields s/zulip_//, internally
f9fb7a3 api/notif [nfc]: Rename channelId field from streamId
f2b82e3 api/notif [nfc]: Rename channelName field from streamName
ece0151 api/notif test: Split new vs old copies of test fixture data
b2e41f7 api/notif: Cut obsolete fields server, realm_id

It looks like it has basically b2e41f7 minus the test changes in ece0151; or said another way, f2b82e3 plus the changes in b2e41f7.

(Then plus the changes in aacf52a, as your next paragraph says.)

Actually I had use that commit as the starting point, but then made some changes to match the existing FcmMessage types (like the renames + type converters per field + removal of server, realm_id).

Ah, hadn't seen this as of #2279 (comment) or I would have replied here directly. (GitHub strikes again: this page was open in a tab, and appeared up to date because it showed other changes like your later comment #2279 (comment) , but it showed this thread without this comment so I didn't know you'd replied.)

That information would be good to include in the commit message too. Since this is otherwise ready to merge, I'll add it now.

gnprice · 2026-04-21T03:20:50Z

+        check(parse({ ...streamJson }..remove('server'))).jsonEquals(baseline);
+        check(parse({ ...streamJson }..remove('realm_id'))).jsonEquals(baseline);


These two seem like some sort of mismerge. They're not present in streamJson to begin with. (Similarly another mention of these fields below.)

gnprice · 2026-04-21T03:24:26Z

-Future<RemoteMessage> encodeFcmMessage(FcmMessageWithIdentity data, {
-  bool encrypted = true,
-}) async {
+Future<RemoteMessage> encodeFcmMessage(NotifPayloadWithIdentity data) async {


comment and doc need updating

gnprice · 2026-04-21T03:30:44Z

-    test('optional fields missing cause no error', () {
-      check(parse({ ...streamJson }..remove('realm_name')))
-        .realmName.isNull();


This is still optional in the code, so we should continue to test that.

gnprice · 2026-04-21T03:30:50Z

-      check(parse({ ...streamJson }..remove('channel_name')))
-        .recipient.isA<FcmMessageChannelRecipient>().which((it) => it
-          ..channelId.equals(42)
-          ..channelName.isNull());


gnprice · 2026-04-21T03:35:02Z

+        for (final badIntList in [
+          ['abc', 34], [12, 'abc'], [12, '34'], ['12', '34'], ['abc', 'xyz'],
+        ]) {
+          test("${n++}", () => checkParseFails({ ...baseJson, 'message_ids': badIntList }));


Now that this is just a List<int> with standard json_serializable behavior, no need to include specific tests for it.

gnprice · 2026-04-21T03:37:14Z

+    "realm_name": ?realmName,
+    "realm_url": account.realmUrl.toString(),


nit: realm_url comes first because it's the stable identifier, while realm_name is some extra data for display

(here and below)

gnprice · 2026-04-21T03:41:21Z

+/// Parsed version of decrypted data from an [EncryptedFcmMessage].
 ///
 /// For partial API docs, see:
 ///   https://zulip.com/api/mobile-notifications
+sealed class NotifPayload {


This summary line isn't right: a NotifPayload doesn't necessarily come from an encrypted anything.

The existing doc seems right, in fact. Can replace "an FCM message" with "a notification payload", to match the rename.

rajveermalviya · 2026-04-21T16:30:58Z

Thanks for the review @gnprice! Pushed an update, PTAL.

gnprice

Thanks for the revision! This looks great; a few small comments.

(I think ideally the FCM stuff would be in a separate file fcm_message.dart, and there are some ways to get around the difficulty in #2279 (comment) and make that happen. But those ways involve some refactoring and so aren't suitable to include in the initial commit that introduces LegacyFcmMessage, because the refactoring would complicate the diff against the original version found in the Git history. Given that, they can just as well go in a follow-up change after this PR and #2230 are merged.)

gnprice · 2026-04-21T17:14:26Z

-/// If [encrypted] is true, then produce an E2EE notification,
-/// encrypted to the latest push key found on the account
-/// that the payload is addressed to.
+/// If the provided data is not a (sub)type of [LegacyFcmMessageWithIdentity],
+/// then produce an E2EE notification, encrypted to the latest push key found
+/// on the account that the payload is addressed to.
 /// Otherwise, produce a legacy non-E2EE notification.


nit: no need to re-wrap the parts that are unchanged; that makes the diff bigger and harder to read

In general, in our prose we prefer to use "semantic line breaks" rather than wrap maximally to any given width, because that allows keeping diffs minimal this way. See these resources:
https://github.com/dart-lang/site-shared/blob/3408a7468/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks
https://rhodesmill.org/brandon/2012/one-sentence-per-line/

The existing text here looks to have been written that way, in particular with the newline placed after that comma.

gnprice · 2026-04-21T17:15:30Z

-/// If [encrypted] is true, then produce an E2EE notification,
-/// encrypted to the latest push key found on the account
-/// that the payload is addressed to.
+/// If the provided data is not a (sub)type of [LegacyFcmMessageWithIdentity],


Suggested change

/// If the provided data is not a (sub)type of [LegacyFcmMessageWithIdentity],

/// If the provided data is not a [LegacyFcmMessageWithIdentity],

gnprice · 2026-04-21T17:18:10Z

-      default: return UnexpectedFcmMessage.fromJson(json);
+/// See pre-E2EE server implementation for reference:
+///   https://github.com/zulip/zulip/blob/10.x/zerver/lib/push_notifications.py#L963
+sealed class LegacyFcmMessage implements NotifPayload {


Most of this parsing code is from an earlier version of this file, basically before #2074 added changes for handling E2EE notifications, specifically referred to the following commit: https://github.com/zulip/zulip-flutter/commit/86c8a1cf9

Great, thanks for adding this.

Is that commit 86c8a1c really the starting point, though? The version you added has a bunch of changes that happened in the 6 commits after that:
59bd6c9 api/notif [nfc]: Move type converters to be explicit per field
700fe0b api/notif [nfc]: Rename message ID fields s/zulip_//, internally
f9fb7a3 api/notif [nfc]: Rename channelId field from streamId
f2b82e3 api/notif [nfc]: Rename channelName field from streamName
ece0151 api/notif test: Split new vs old copies of test fixture data
b2e41f7 api/notif: Cut obsolete fields server, realm_id

It looks like it has basically b2e41f7 minus the test changes in ece0151; or said another way, f2b82e3 plus the changes in b2e41f7.

(Then plus the changes in aacf52a, as your next paragraph says.)

gnprice · 2026-04-21T17:29:26Z

+// Types for parsing E2EE notification payloads.
+//
+
+/// Parsed version of a notification payload, of any plaintext type.


nit: this edit from "an FCM message" to "a notification payload" currently happens in this commit:
78ec917a7 notif: Make NotifPayload to parse only newer E2EE payloads

but it seems like it more logically belongs as part of the preceding commit, the rename:
ccfd46848 notif [nfc]: Rename FcmMessage types and related symbols to NotifPayload

rajveermalviya · 2026-04-21T20:49:54Z

Thanks for the review @gnprice! Pushed an update, PTAL.

gnprice · 2026-04-21T20:53:41Z

Thanks! All looks good now except the bit in #2279 (comment) .

chrisbobbe · 2026-04-21T21:13:43Z

I think the substantive part of #2279 (comment) is still unresolved? (From a quick skim of the changed files)

In the next commits, we are introducing types that only parse E2EE or only parse legacy plaintext notifications. So, create a new group now to avoid the indentation changes being included in more complex commits.

…t notifications See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/E2EE.3A.20parsing.20code/with/2420939 NFC, because this commit only adds code for parsing the payloads (and related tests) but doesn't use them to parse any notifications yet. They live in the same file that contains FcmMessage types because LegacyFcmMessage types `implement` FcmMessage types. And because there are some `sealed` types, we can't `implement` types that are in another file/library. Most of this parsing code is from an earlier version of this file, basically before zulip#2074 added changes for handling E2EE notifications, specifically referred to the following commit: zulip@86c8a1cf9 The code was then updated to match the existing FcmMessage types, corresponding to some of the commits in zulip#2074 up to: zulip@b2e41f7fa Additionally, also included changes for reading `realm_name`, because it was added in zulip#2136 (after zulip#2074 was merged), specifically referred to the following commit: zulip@aacf52af4

Use the recently added LegacyFcmMessage to parse the plaintext FCM message. The existing helpers that generate JSON for (non-legacy) FcmMessage types will soon generate only E2EE payloads, so this commit also adds new helpers for legacy FCM messages (which currently generates exact same JSON). Also updates plaintext notification related tests, including changing one test "stream message: stream name omitted" to use legacy payloads, because stream name/channel name is always present in newer E2EE payloads.

See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/E2EE.3A.20parsing.20code/with/2420939

Thanks to Chris for pointing this out: zulip#2279 (comment)

gnprice · 2026-04-22T00:29:12Z

OK, and merging! (Both those subthreads are now resolved.)

chrisbobbe self-requested a review April 17, 2026 17:42

chrisbobbe self-assigned this Apr 17, 2026

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Apr 17, 2026

rajveermalviya mentioned this pull request Apr 17, 2026

notif ios: Decrypt E2EE notifications #2230

Merged

chrisbobbe reviewed Apr 17, 2026

View reviewed changes

rajveermalviya force-pushed the pr-notif-payloads branch 2 times, most recently from 75d726c to 8e87663 Compare April 20, 2026 13:20

gnprice reviewed Apr 21, 2026

View reviewed changes

rajveermalviya force-pushed the pr-notif-payloads branch from 8e87663 to 78ec917 Compare April 21, 2026 16:29

rajveermalviya requested a review from gnprice April 21, 2026 16:31

gnprice reviewed Apr 21, 2026

View reviewed changes

rajveermalviya force-pushed the pr-notif-payloads branch 2 times, most recently from 24b945c to 487eced Compare April 21, 2026 20:49

rajveermalviya requested a review from gnprice April 21, 2026 20:49

gnprice force-pushed the pr-notif-payloads branch from d185f40 to 525e713 Compare April 21, 2026 23:21

rajveermalviya and others added 6 commits April 21, 2026 17:25

notif test [nfc]: Move FcmMessage parsing tests in a new group

adbd92f

In the next commits, we are introducing types that only parse E2EE or only parse legacy plaintext notifications. So, create a new group now to avoid the indentation changes being included in more complex commits.

notif [nfc]: Rename FcmMessage types and related symbols to NotifPayload

00a52ff

notif: Make NotifPayload to parse only newer E2EE payloads

dfe4e5f

See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/E2EE.3A.20parsing.20code/with/2420939

notif [nfc]: Clarify an unexpected FCM message might not be legacy

debb92b

Thanks to Chris for pointing this out: zulip#2279 (comment)

gnprice force-pushed the pr-notif-payloads branch from 525e713 to debb92b Compare April 22, 2026 00:29

gnprice merged commit debb92b into zulip:main Apr 22, 2026
2 checks passed

		/// A [LegacyFcmMessage] of a type (a value of `event`) we didn't know about.
		class UnexpectedLegacyFcmMessage extends LegacyFcmMessage implements UnexpectedNotifPayload {

		check(parse({ ...streamJson }..remove('server'))).jsonEquals(baseline);
		check(parse({ ...streamJson }..remove('realm_id'))).jsonEquals(baseline);

		"realm_name": ?realmName,
		"realm_url": account.realmUrl.toString(),

	/// If the provided data is not a (sub)type of [LegacyFcmMessageWithIdentity],
	/// If the provided data is not a [LegacyFcmMessageWithIdentity],

Conversation

rajveermalviya commented Apr 17, 2026

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented Apr 20, 2026

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented Apr 21, 2026

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented Apr 21, 2026

Uh oh!

gnprice commented Apr 21, 2026

Uh oh!

chrisbobbe commented Apr 21, 2026

Uh oh!

gnprice commented Apr 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

chrisbobbe Apr 17, 2026 •

edited

Loading