This differential adds methods to NotificationsCryptoModule to initialize olm session with the keyserver prekeys and one time keys.
Additionally the class is refactored so that every call to in-memory CryptoModule instance is wrapped around file deserialization
and serialization. This is both to ensure that any change to in-memory CryptoModule is reflected in persistent storage and to
avoid repeated code.
Details
Details
Build the app. Even though it is possible to test those methods as they are in this differential it might be cumbersome. Very next differential will use them in CommCoreModule making it easy to test them. If any reviewer requests to enrich test plan for this diff, I will provide C++ code that tests those methods but for now deferring more thorough testing for the very next differential.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Planning changes since I am going to change prekey API mplemented in a previous diff, which will affect this differential.
Comment Actions
Methods for one time keys and prekeys will not be needed for NotificationsCryptoAccount, so I plan to reuse this differential to implement methods to initilaliza session and check whether it is already initialized.
Comment Actions
Replace prekey and one time key generation methods with methods to initialize olm notifications session.
| native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp | ||
|---|---|---|
| 221 ↗ | (On Diff #25485) | What's the purpose of this initial encrypted message? The create_outbound function handles creating an initial encrypted message as part of X3DH initialization. Is a second "initial message" necessary? What does it achieve? |
| native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp | ||
|---|---|---|
| 221 ↗ | (On Diff #25485) | Never mind – I looked into some more, and this initial message is necessary. The create_inbound method on the other client needs the result of this |
| native/cpp/CommonCpp/CryptoTools/CryptoModule.h | ||
|---|---|---|
| 64 ↗ | (On Diff #25995) | I'm confused why this change was included in this diff... it doesn't seem to be relevant to the rest of the diff |
| native/cpp/CommonCpp/CryptoTools/CryptoModule.h | ||
|---|---|---|
| 64 ↗ | (On Diff #25995) | After getting through the whole diff stack, I still can't find where this is used. Do we ever need overwrite = false? If yes, can you point out where it will be used? If not, then I think we don't think as a parameter, and can just change the behavior without introducing a new parameter. Separately – can you provide some explanation for the change to default to overwriting? |
| native/cpp/CommonCpp/CryptoTools/CryptoModule.h | ||
|---|---|---|
| 64 ↗ | (On Diff #25995) | This change is essential for this differential to work properly. Flat file where olm account for notifications is stored is cleaned only during the log out process. If the user tries to log in, gets one time key and prekey from the keyserver and establishes olm session on its side, but something goes wrong on the keyserver, then we will not be able to retry log in process if we don't change the behaviour of initializeOutboundForSendingSession (its behaviour is to throw an error on attempt to initialize session with the same user again). On the other hand, CryptoModule is a general purpose class that shouldn't be restricted to the case of NotificationsCryptoModule, so we have to keep it useful for someone else that might need the old behaviour. |
| native/cpp/CommonCpp/CryptoTools/CryptoModule.h | ||
|---|---|---|
| 64 ↗ | (On Diff #25995) | If we never use overwrite = false, and are only keeping it because "someone else might need the old behaviour", then I think we should remove it now, and reintroduce it later if it is needed. As mentioned in my last comment:
|
| native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp | ||
|---|---|---|
| 229 ↗ | (On Diff #25995) | Does it makes sense to print a log message here when this happens? That way the developer can be more aware if something unexpected occurs |
Comment Actions
Merge session initialization and initial encrypted message gerenation into one meethod. This way it works with bugged olm fork and
it will still be working once the bug is fixed