This differential addresses concerns outlined in the following Linear issue:
https://linear.app/comm/issue/ENG-7638/investigate-android-crash-on-sqlite-migration-38
Details
Details
- Reviewers
tomek bartek kamil - Commits
- rCOMMe9036e146415: Turn migration 38 into a no-op
- Revert "Introduce fallback mechanism for notifications decryption if MMKV initialization hasn't been executed yet." and "Implement notifications sessions storage
on top of MMKV. Migrate existing notifs account to SQLite" diffs.
- Build Android and iOS app and log in. this leaves the app in legacy state with notifications session in a single flat file.
- Reset reverts. Add logging to NotificationsCryptoModule informing that legacy notifications session storage was used to decrypt a notif.
- Build the app again. Send two notifs.
- Ensure that the first notif results in a log but the second doesn't (session was succesfully migrated to new place).
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-7638
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp | ||
|---|---|---|
| 299–305 ↗ | (On Diff #38888) | Can you explain these changes? |
| native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp | ||
|---|---|---|
| 299–305 ↗ | (On Diff #38888) | This line migrates existing notifications session from flat file to MMKV.
This is basically what migration 38 would do but without moving notifs account to SQLite. One thing I notices - this line has to be wrapped with try catch since before user opens the app after upgrading this line won't work and the user will keep using old session storage until they open the app. |
Comment Actions
Add a try catch to grafecully handle the case if the user doesn't open the app after upgrading it.
| native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp | ||
|---|---|---|
| 299–305 ↗ | (On Diff #38888) | Thanks for explaining! |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 558–560 ↗ | (On Diff #38901) | Is legacyCryptoAccountDataKey used currently? Should we delete it from the secure store? |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 975 ↗ | (On Diff #38901) | Maybe we should set empty migration here to avoid confusion? Now it looks like we are migrating the accounts. Also, we probably don't need to have a transaction for this migration. |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 558–560 ↗ | (On Diff #38901) |
It is currently used. CommCoreModule already uses this key (the value "cryptoAccountDataKey") to store pickling for content account. See: https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp#L443 and https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/NativeModules/CommCoreModule.h#L31
Theoretically yes - there is no reason to keep it in secure store. We should keep it in the SQLite as a column in olm_persist_account as we do on the keyserver. This requires migration and separate diff however so not in the scope of this diff. The fact that we keep pickling key for content account is a relict from the period when the SQLite wasn't encrypted and we probably wanted to create some "form" of security. |
Comment Actions
Remove unnecessary migration function and replace it with dummy lambda that is not run inside transaction.