This differential introduces MMKV to iOS app. At this point there are only methods to set and get string, but future diffs will add more methods. This
differential introduces simplest API we can use and test.
Details
Details
- Reviewers
tomek bartek - Commits
- rCOMM61f0e7c40f51: Introduce MMKV to iOS app and NSE
- Below the comm::CommMMKV::initialize() line add line that sets value under a string key.
- In NotificationService.mm at the beginning add line that tries to get string from the same key and logs this key in case it is present.
- Send notification and check it the same string is logged.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp | ||
|---|---|---|
| 3 ↗ | (On Diff #37010) | This is temporary - Android implementation is introduced in separate diff to make this one more concise and easy to review. |
| 42 ↗ | (On Diff #37010) | Same as above. |
| native/ios/Comm/CommMMKV.mm | ||
| 12 ↗ | (On Diff #37010) | They say 16 bytes at most so it is best to get as much bytes as possible for enchanced security. |
| 23 ↗ | (On Diff #37010) | I examined MMKv implementation and I couldn't identify code paths that would lead as to null being returned here. The only case was if we pass nullish mmkvID value but the value is a hardcoded constant in our case. |
| 43 ↗ | (On Diff #37010) | The NSE shouldn't be allowed to set the encryption key. The only case it would try to do so would be if we received notification before the first app start. This way of checking if we are running in app extension is taken from MMKV implementation. |
| 67 ↗ | (On Diff #37010) | I examined the code and it can be false in case when the file doesn't exists. Therefore I added call to getMMKVInstance (which creates a file if it doesn't exist) at the end of initialize and at the end of this function to ensure we can filter out this edge case and treat false value as some real error. Creating new empty file of mmkv after calling clearSensitiveData is consistent with what we do with SQLite. |
| native/ios/Comm/CommMMKV.mm | ||
|---|---|---|
| 22 ↗ | (On Diff #37010) | This parameter determines whether MMKV implementation checks for app specific or app group root directory. The NSE and the main app are separate processes so we must use group root directory. |
Comment Actions
Wondering if we can perform the init logic in C++ instead so that it can be shared between platforms.
| native/cpp/CommonCpp/Tools/CommMMKV.h | ||
|---|---|---|
| 11 ↗ | (On Diff #37010) | It should probably clear all the data |
| native/ios/Comm/CommMMKV.mm | ||
| 23 ↗ | (On Diff #37010) | Wondering if it is the right approach to crash the app in this case. If it happens, the app should work ok, but without the correct unread count... not ideal, but better than crashing. What do you think? |
| 43 ↗ | (On Diff #37010) | Can we add a code comment explaining why we're checking it like that? |
Comment Actions
This differential is incorrect. The reason is that in case user logs out the main app will update encryption key in secure store, but the NSE will the old encryption key in a static variable. The NSE is not guaranteed to be restarted for each notif so this is a concern. To mitigate this we need to fetch encryption key each time we call initialize provided we are running in the NSE. Unfortunately it is not enough. MMKV is implemented in such a way that there is a global dictionary mapping from mmkv id to MMKV* instance. When we call mmkvWithID the library first checks if an instance is in the dictionary and returns it in case it is. Each MMKV* instance keeps cryptKey as an instance variable. Therefore even if we fetch new encryption key in running NSE process we will still get MMKV* instance with outdated encryption key. To fully solve the issue we should rotate both encryption key and mmkv id. I will update the diff shortly.
Wondering if we can perform the init logic in C++ instead so that it can be shared between platforms.
The initialize implementation here will change noticeably to address issue described above. I will re-request review after updating the diff so that you can reconsider this idea.
| native/cpp/CommonCpp/Tools/CommMMKV.h | ||
|---|---|---|
| 11 ↗ | (On Diff #37010) | Actually it clears all the data since it removes the mmkv file. This method is named clearSensitiveData to match convention from SQLiteQueryExecutor where clearSensitiveData removes database file. Every data that we keep in any database that we use is considered sensitive. |
| native/ios/Comm/CommMMKV.mm | ||
| 23 ↗ | (On Diff #37010) | This API is intended to be used a library. Therefore I think that error handling is the responsibility of the caller. For the particular use case of unread count error handling is performed in D11071. |
| 43 ↗ | (On Diff #37010) | Sure - will do it. |