fix: incorrect default iOS keychain accessibility level

[JoniVR]

Hopefully fixes #246, #195 completely.

I've also added deprecation warnings for levels ALWAYS and ALWAYS_THIS_DEVICE_ONLY since these are deprecated in iOS 16.

Important note: This won't help in updating existing stores with the new accessibility level. I recommend documenting for users how they can recreate their instance.

My personal approach was to add a .withInstanceID('encrypted') to my instantiation. Which would then create a new instance with the correct value. My personal setup looks like this currently (without this PR):

export const encryptedStorage = new MMKVLoader()
  .setAccessibleIOS(IOSAccessibleStates.AFTER_FIRST_UNLOCK)
  .withInstanceID('encrypted')
  .withEncryption()
  .initialize();

Perhaps an actual migration could be written so no data is lost? Not sure how that'd work with redux-persist though.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
rnmmkv	✅ Ready (Inspect)	Visit Preview	Jun 30, 2022 at 6:33PM (UTC)

[ammarahm-ed]

@JoniVR Thanks! I hope this is the final fix we would need. I was wondering if we still need the logic to recursively check for values available?

[JoniVR]

@ammarahm-ed It's not going to hurt to still have it for now I think. Not sure if there is still a possibility where the values aren't ready yet. I'd keep it to be sure because it did seem to help in some cases where the app wasn't launched in the background many hours in advance.

[ammarahm-ed]

@JoniVR I think these are two different issues. One, the keychain is not available on launch, and two, values unavailable in background.

However doing the recursion on JS did help more because it did not block the main thread?

[JoniVR]

@ammarahm-ed Yes but they're related to the same thing: data not being readable.

I've noticed in all of my logging 2 types of cases of this happening:

1. isProtectedDataAvailable being false when launching the app (probably an Apple related bug for which many articles have been written) -> The native fix seems to resolve this issue entirely, though the usual recommended fix here is to listen to the notifications instead of polling, but this seems a bit too hard for us to do.
2. The app being launched in the background due to things like a notification or "device machine learning / prewarming" -> In this case the native fix doesn't work because the accessibility value disallowed reading encrypted data while the device was locked (whereas AFTER_FIRST_UNLOCK should always work, because the app will never be launched in the background before first unlock), this should be fixed by this PR. You can also clearly tell by the logging when this happens because there's usually a few hours between the occurance of data read failure and effective app launch.

I pretty much stopped relying on the JS fix because it required the same fix as the iOS native fix but less directly so (the isProtectedData check is the one Apple intends you to use) and the native fix just seems safer in general imo.

[JoniVR]

Important note: I also haven't really confirmed that this will effectively fix it. I released my first beta build with the above mentioned fix about an hour ago, so I'll still be monitoring the issue.

But, judging by the code (if I understand it correctly) and based on my knowledge around this issue, it seems to me like WHEN_UNLOCKED was the default instantiated value, which will definitely cause the issue mentioned in case 2 in above comment.

However doing the recursion on JS did help more because it did not block the main thread?

To answer this definitively: I didn't really try the JS approach because I didn't like having to swap out the JS functions myself. If you do decide to go with an approach like this in the future, it's very important to also have a limited amount of attempts or timeout on it.

[JoniVR]

Also, what do I need to do to fix the iOS pod in the CI build? I can't figure it out, haven't really built a react native library with native iOS layer before.

[ammarahm-ed]

Also, what do I need to do to fix the iOS pod in the CI build? I can't figure it out, haven't really built a react native library with native iOS layer before.

On your local machine just navigate to example folder and install deps. Then install pods by running pod install --repo-update.

This should generate a new Podfile.lock which you can then commit.

[ammarahm-ed]

And after some evaluation, I think merging this PR can break many apps that already have WHEN_UNLOCKED set for their encrypted stores and internally initializing the store with a changed default could break the app/or users might need to relogin. I am not sure if it is a good idea to change the default now because devs can manually set the property to AFTER_FIRST_UNLOCK when required. Probably why it is already configurable. Writing a migration step could become tricky in this case. Maybe making it more clear in the documentation is enough?

[JoniVR]

And after some evaluation, I think merging this PR can break many apps that already have WHEN_UNLOCKED set for their encrypted stores and internally initializing the store with a changed default could break the app/or users might need to relogin. I am not sure if it is a good idea to change the default now because devs can manually set the property to AFTER_FIRST_UNLOCK when required. Probably why it is already configurable. Writing a migration step could become tricky in this case. Maybe making it more clear in the documentation is enough?

Yes, my idea was that (in order to not make this a breaking change), you can just document migration steps in the release notes.

I don't think this will be a breaking change for existing apps, it will only affect new users since the accessibility level for a storage instance can't be changed without effectively recreating it. I think it's also a better default in general for new users.

So yeah, I agree, don't automatically migrate existing stores. You could either provide documentation about it in the release notes OR (and this might be even better) provide a function that will automatically migrate the existing stores without data loss BUT needs to be explicitly called by the user. I'm pretty sure that should be possible, since we can iterate existing instances and just recreate them.

I do still believe that the default should be changed since this is effectively very strange behavior to have as default. Most often people will be using encrypted storage for storing auth tokens, which do need to be read in the background on some occasions. There's a valid case for this default as you can clearly see by the amount of activity on the initial issue imo.

[JoniVR]

Side note: I had to also remove the existing Pods folder and Podfile.lock for pod install to work, otherwise I got:

[!] CocoaPods could not find compatible versions for pod "MMKV":
  In snapshot (Podfile.lock):
    MMKV (= 1.2.10)

  In Podfile:
    react-native-mmkv-storage (from `../../react-native-mmkv-storage.podspec`) was resolved to 0.7.6, which depends on
      MMKV (= 1.2.13)

You have either:
 * changed the constraints of dependency `MMKV` inside your development pod `react-native-mmkv-storage`.
   You should run `pod update MMKV` to apply changes you've made.

[ammarahm-ed]

We can merge this if:

1. Changing default value from WHEN_UNLOCKED to AFTER_FIRST_UNLOCK of for existing stores will not break them and they would still be accessible.

2. We have a way to migrate to a new store. By the way how did you migrate from old store to a new one for your users? Won't it be a sufficient solution that existing users are not affected and if they get logged out and re-login, new values will be stored using the new default store value?

I know you have provided an example for migrating but it does not mention getting values from the existing store.

[JoniVR]

Won't it be a sufficient solution that existing users are not affected and if they get logged out and re-login, new values will be stored using the new default store value?

It might, I haven't tested that as there was no easy way for me to view the accessibility values of stored items. If you know a way to debug that, I'd be happy to.

By the way how did you migrate from old store to a new one for your users? I just changed the instance name, which does indeed make your users "lose data" because in my case they'll have to sign in again. Not sure if just changing the accessibility value will use the new value when the items update.

In theory, changing the accessibility value doesn't change anything for existing stored items. It's impossible to modify an existing accessibility level (Apple's API's doesn't support it), so it doesn't change anything unless the user updates the data. Even if it did, AFTER_FIRST_UNLOCK is more often accessible vs WHEN_UNLOCKED. WHEN_UNLOCKED only allows reading the encrypted store while the device is effectively unlocked, while AFTER_FIRST_UNLOCK allows reading the store after the first device unlock after booting the device.

It's probably easily enough to migrate the data: iterate over the instances (especially the default instance), or let the user pass the instances through a parameter (they can still pass a call to get all instances), then store the data in a variable and create a new instance (for example with a prefix) for each instance.

I haven't put any time into migrating our data (I might in the future but we're still deciding if it's worth it), and I don't think providing an out-of-the-box migration function is a must-have to merge this PR at all. This PR just changes the default to a more sensible one (read: one that won't generate issues of users and cause unexpected behavior).

You're free to merge it, or don't, it's your call really. If you don't feel like merging it, I would at least document the behavior.

[JoniVR]

Another option (and perhaps easiest option) is to make a breaking change release, notify users of the change and what it exactly does, and then tell them how to override the default accessibility value (like I did in the example snippet) to maintain the previous behavior.

[ammarahm-ed]

@JoniVR I think we should release v0.8.0 then and make it a breaking change.