Page MenuHomePhabricator
Differential D12668

Implement peer notifications session creation as outbound on native
ClosedPublic
Actions

Authored by marcin on Jul 4 2024, 9:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 22, 5:32 PM
Unknown Object (File)
Sun, Dec 22, 5:32 PM
Unknown Object (File)
Sun, Dec 22, 5:32 PM
Unknown Object (File)
Sun, Dec 22, 5:32 PM
Unknown Object (File)
Sun, Dec 22, 5:32 PM
Unknown Object (File)
Sun, Dec 22, 5:32 PM
Unknown Object (File)
Sun, Dec 22, 5:32 PM
Unknown Object (File)
Fri, Dec 6, 2:03 AM
View All Files
Subscribers
ashoat
kamil

Details

Reviewers
tomek
bartek
kamil
Commits
rCOMM85ab94df4f74: Implement peer notifications session creation as outbound on native
Summary

This differential implements methods that allow to create and persist outboud notifications session on native.

Test Plan

Tested in D12673

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin created this revision.Jul 4 2024, 9:21 AM
Herald added a subscriber: ashoat. · View Herald TranscriptJul 4 2024, 9:21 AM
marcin added a child revision: D12669: Implement peer notifications session creation as outbound on web and add notifs outbound session creator to OlmAPI.Jul 4 2024, 9:24 AM
Harbormaster completed remote builds in B30147: Diff 42038.Jul 4 2024, 9:38 AM
marcin requested review of this revision.Jul 4 2024, 9:38 AM
marcin edited the test plan for this revision. (Show Details)Jul 4 2024, 9:44 AM
tomek accepted this revision.Jul 8 2024, 4:00 AM
tomek added inline comments.
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
202 ↗(On Diff #42038)

The logic could probably be simplified by treating a keyserver as a sender. Then we wouldn't need to have a flag telling whether it is a keyserver session - all the sessions would be stored using "DEVICE." + deviceID + ".NOTIFS_SESSION" schema. But I don't think it is a good idea to perform such refactoring immediately.

This revision is now accepted and ready to land.Jul 8 2024, 4:00 AM
marcin added inline comments.Jul 12 2024, 2:24 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
202 ↗(On Diff #42038)

AS explained in other differentials I am aware of low quality of this design but I wanted to avoid working on migration of keyserver sessions to new schema.

kamil accepted this revision.Jul 16 2024, 4:12 AM
kamil added a subscriber: kamil.

I really don't like the idea (looking at this diff and the entire stack) of passing isKeyserverSession - notification crypto module should treat all devices the same regardless of whether this is a keyserver or client device, and we should only have senderID, because the current design makes it confusing. But given this is implemented and tested I believe we can proceed, you could create a follow-up task to improve it in future but for now LGTM.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1489 ↗(On Diff #42038)

can we add a code comment explaining this - this looks really confusing

1499–1505 ↗(On Diff #42038)

We can create a util function to create jsi::Object from crypto::EncryptedData to avoid code duplication but can be done separately

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
154–157 ↗(On Diff #42038)

should we be strict here and either rename the method to getDeviceNotificationsSessionKey or update the key to have a PEER prefix? Additionally keyserver is also a device

kamil mentioned this in D12670: Implement notification encryption on native.Jul 16 2024, 4:43 AM
ashoat added a comment.Jul 16 2024, 6:45 AM
In D12668#361372, @kamil wrote:

I really don't like the idea (looking at this diff and the entire stack) of passing isKeyserverSession - notification crypto module should treat all devices the same regardless of whether this is a keyserver or client device, and we should only have senderID, because the current design makes it confusing. But given this is implemented and tested I believe we can proceed, you could create a follow-up task to improve it in future but for now LGTM.

Curious for @marcin's perspective on this, and how much time it might take to fix up

marcin added a comment.EditedJul 24 2024, 2:48 AM
In D12668#361448, @ashoat wrote:
In D12668#361372, @kamil wrote:

I really don't like the idea (looking at this diff and the entire stack) of passing isKeyserverSession - notification crypto module should treat all devices the same regardless of whether this is a keyserver or client device, and we should only have senderID, because the current design makes it confusing. But given this is implemented and tested I believe we can proceed, you could create a follow-up task to improve it in future but for now LGTM.

Curious for @marcin's perspective on this, and how much time it might take to fix up

It would require us to implement migration for currently existing users from KEYSERVER prefixed key schema to DEVICE prefixed key schema. I initially thought that it will be easier to skip the migration and treat session separately.

Now I think that migration might not be that hard to implement. However I am worried that treating peer-based and keyserver-based notifications the same way will force us to use the same decryption function for both. Decryption function for peer-based notifications (D12750) is complex and might have bugs. Therefore the advantage of treating them separately is that we guarantee we won't break already existing and working functionality (keyserver-based notifications).

marcin updated this revision to Diff 42726.Jul 24 2024, 5:44 AM

Address review

Harbormaster failed remote builds in B30630: Diff 42726!Jul 24 2024, 5:46 AM
marcin updated this revision to Diff 42934.Jul 30 2024, 3:22 AM

Update phab

Harbormaster completed remote builds in B30789: Diff 42934.Jul 30 2024, 3:41 AM
ashoat added a comment.Jul 31 2024, 10:15 PM
In D12668#364083, @marcin wrote:
In D12668#361448, @ashoat wrote:
In D12668#361372, @kamil wrote:

I really don't like the idea (looking at this diff and the entire stack) of passing isKeyserverSession - notification crypto module should treat all devices the same regardless of whether this is a keyserver or client device, and we should only have senderID, because the current design makes it confusing. But given this is implemented and tested I believe we can proceed, you could create a follow-up task to improve it in future but for now LGTM.

Curious for @marcin's perspective on this, and how much time it might take to fix up

It would require us to implement migration for currently existing users from KEYSERVER prefixed key schema to DEVICE prefixed key schema. I initially thought that it will be easier to skip the migration and treat session separately.

Now I think that migration might not be that hard to implement. However I am worried that treating peer-based and keyserver-based notifications the same way will force us to use the same decryption function for both. Decryption function for peer-based notifications (D12750) is complex and might have bugs. Therefore the advantage of treating them separately is that we guarantee we won't break already existing and working functionality (keyserver-based notifications).

If we skip the work of unifying this logic now, we should at least create a follow-up task to address it later.

marcin added a comment.Aug 1 2024, 5:24 AM
In D12668#366328, @ashoat wrote:
In D12668#364083, @marcin wrote:
In D12668#361448, @ashoat wrote:
In D12668#361372, @kamil wrote:

I really don't like the idea (looking at this diff and the entire stack) of passing isKeyserverSession - notification crypto module should treat all devices the same regardless of whether this is a keyserver or client device, and we should only have senderID, because the current design makes it confusing. But given this is implemented and tested I believe we can proceed, you could create a follow-up task to improve it in future but for now LGTM.

Curious for @marcin's perspective on this, and how much time it might take to fix up

It would require us to implement migration for currently existing users from KEYSERVER prefixed key schema to DEVICE prefixed key schema. I initially thought that it will be easier to skip the migration and treat session separately.

Now I think that migration might not be that hard to implement. However I am worried that treating peer-based and keyserver-based notifications the same way will force us to use the same decryption function for both. Decryption function for peer-based notifications (D12750) is complex and might have bugs. Therefore the advantage of treating them separately is that we guarantee we won't break already existing and working functionality (keyserver-based notifications).

If we skip the work of unifying this logic now, we should at least create a follow-up task to address it later.

Created task to track.

marcin updated this revision to Diff 43071.Aug 5 2024, 4:04 AM

Rebase before landing

Harbormaster completed remote builds in B30891: Diff 43071.Aug 5 2024, 4:26 AM
Closed by commit rCOMM85ab94df4f74: Implement peer notifications session creation as outbound on native (authored by marcin, committed by Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local>). · Explain WhyAug 5 2024, 6:39 AM
This revision was automatically updated to reflect the committed changes.
Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local> added a commit: rCOMM85ab94df4f74: Implement peer notifications session creation as outbound on native.

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
NativeModules/
7 lines
95 lines
Notifications/
BackgroundDataStorage/
14 lines
89 lines
_generated/
schema/
7 lines

Diff 43129

View Options

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

Loading...
View Options

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp

Loading...
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.h

Loading...
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp

Loading...
View Options

native/cpp/CommonCpp/_generated/commJSI-generated.cpp

Loading...
View Options

native/cpp/CommonCpp/_generated/commJSI.h

Loading...
View Options

native/schema/CommCoreModuleSchema.js

Loading...