API bindings for E2EE notifications#2176
Conversation
gnprice
force-pushed
the
pr-e2een-api
branch
from
68d3efb to
e5701c7
Compare
|
The bindings on DeviceUpdateEvent weren't quite right, as I learned when I tested this code on a live server today for the first time. I've pushed a revision that fixes the way JsonNullable is used there, and adds tests, plus a commit expanding the docs on JsonNullable. |
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
This is exciting!! Small comments below.
test/api/model/events_test.dart
Outdated
| final baseJson = {'id': 1, 'type': 'device', 'op': 'update', | ||
| 'device_id': 3 }; |
There was a problem hiding this comment.
nit: I think this could all fit on one line?
lib/api/notifications.dart
Outdated
| // The doc still says `push_account_id` here, but that's an oversight: | ||
| // https://chat.zulip.org/#narrow/channel/378-api-design/topic/E2EE.20-.20key.20rotation/near/2384969 | ||
| // (This is a part of the API docs that isn't checked by tests.) |
There was a problem hiding this comment.
Looks like this was fixed in zulip/zulip#38124, which was merged.
lib/api/notifications.dart
Outdated
| EncryptedFcmMessage({ | ||
| required this.pushKeyId, required this.encryptedData}); |
There was a problem hiding this comment.
nit: can this fit on one line?
lib/api/route/notifications.dart
Outdated
| 'push_key': RawParameter(key.pushKey), | ||
| }, | ||
| if (token != null) ...{ | ||
| 'token_kind': RawParameter(token.tokenKind.name), |
There was a problem hiding this comment.
Claude flagged the use of enum .name here 🙂 what do you think? Quoting from Claude:
- One small thing: token_kind is passed as RawParameter(token.tokenKind.name) (line 26). The .name property gives the
Dart enum name ('fcm' or 'apns'), which happens to match the API values. This works but it's worth noting it relies on
the enum member names matching the API values exactly — if someone renamed the enum member, it would silently break.
The PushRegistration.toJson() path uses the generated _$PushTokenKindEnumMap which is more robust. This is a very
minor fragility, and the same pattern appears elsewhere in the codebase, so this is consistent.
There was a problem hiding this comment.
Well, it's not really a coincidence that the names in the code match the names in the API. But sure, we can do the more explicit pattern. I guess that's what we do on UpdateMessageFlagsOp, and there aren't many other enums in lib/api/route/.
| /// Generate a suitable value to pass as `pushKeyId` to [registerPushDevice]. | ||
| static int generatePushKeyId() { | ||
| final rand = Random.secure(); | ||
| return rand.nextInt(1 << 32); |
There was a problem hiding this comment.
Claude says:
generatePushKeyId(): Uses Random.secure().nextInt(1 << 32) for a 32-bit unsigned random int. Note: 1 << 32 works on
native (64-bit) but would be 1 on web (32-bit shifts). Not a concern since the app targets Android/iOS only, but could
be a gotcha if web were ever added.
Is it true that 1 << 32 could silently give 1? That sounds problematic if it happened in the app; are there any realistic conditions that could cause that, or is it right to imply that it would only ever happen on web?
Then I guess in the absence of that problem, there's still this in the dartdoc of Random.nextInt:
/// Implementation note: The default implementation supports [max] values
/// between 1 and (1<<32) inclusive.Is the "default implementation" actually what gets used?
…Ah, reading the dartdoc on the whole Random class, I think "default implementation" is the antonym of the "secure" implementation:
/// The default implementation supplies a stream of pseudo-random bits that are
/// not suitable for cryptographic purposes.
///
/// Use the [Random.secure] constructor for cryptographic purposes.which we're using here, and yeah, one would hope and expect the "secure" implementation to support 1<<32 for a max value. 🙂
There was a problem hiding this comment.
Flutter web isn't fundamentally a good fit for Zulip, so we don't really need to worry about the Flutter app ever supporting web.
It looks like Claude is mistaken about the behavior on web in any case. Opening a browser console, it's true that in JavaScript 1 << 32 apparently evaluates to 1. But opening https://dartpad.dev/ in order to run Dart code on web:
void main() {
print(1 << 32);
}the result is 0, not 1. And then if you try to pass that to Random.nextInt:
import 'dart:math';
void main() {
print(Random.secure().nextInt(1 << 32));
}it throws an error:
Uncaught Error, error: Error: RangeError: max must be in range 0 < max ≤ 2^32, was 0
So even if this Dart code did for some reason end up getting run on web, it would promptly give an error.
This keeps eg.account and eg.initialSnapshot using recent versions by default. This change has no effect on any existing logic, as seen by scanning the output of the following search: $ git grep 'zulipFeatureLevel [<>]' lib All the thresholds we currently condition on are from before 382.
This is only the second time we're using JsonNullable, and the first time we happen to be using it directly in`readValue`. It took me a bit to work out just how to use it here; I'll add more docs on JsonNullable in the next commit.
Based on the experience of using it for the second time, for DeviceUpdateEvent, in the preceding commit.
…vice This logic is more about Zulip's API than it is about the platform's support for notifications. As we implement the more complex token-registration model needed for E2EE notifications, we'll be adding further details from the Zulip API. So this is the better home for that logic.
|
Thanks for the review! Revision pushed. |
|
Thanks! LGTM, merging. |
2 participants
After the revisions we made to the API for E2EE notifications #1764 in the past few weeks (zulip/zulip#37731; #api design > E2EE - key rotation @ 💬 and topics that branched from there), I believe it's now stable.
So that makes this a good time to review and merge the API bindings on the client side. These commits, like #2160, are taken from my draft branch for #1764.