This differential implements methods that allow to create and persist outboud notifications session on native.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| 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. |
| 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. |
Comment Actions
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 |
Comment Actions
Curious for @marcin's perspective on this, and how much time it might take to fix up
Comment Actions
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).
Comment Actions
If we skip the work of unifying this logic now, we should at least create a follow-up task to address it later.