Liftric / KVault

Secure key-value storage for Kotlin Multiplatform projects.
MIT License
221 stars 20 forks source link

KVault.set() uses sharedPreferences.commit() instead of sharedPreferences.apply() #49

Open rishabh876 opened 1 year ago

rishabh876 commented 1 year ago

SharedPreference provides two ways to write changes i.e., commit() and apply(). Where commit() is a synchronous call and can block Mainthread. Whereas apply() write in-memory first followed by asynchronously to the disk. More details on apply() can be found [here](https://developer.android.com/reference/android/content/SharedPreferences.Editor#apply()).

Can we add option to use apply() by introducing a new API KVault.setAsync(... , ...)?

benjohnde commented 11 months ago

@rishabh876 feel free to give it a try!

benjohnde commented 11 months ago

@rishabh876 you can actually perform the KVault calls async, after some thinking I believe this is suboptimal, we do not really have an async alternative on iOS and just using commit does not guarantee that the shared prefs were written. Which seems to be okay (in some circumstances) for shared prefs but for secrets I would certainly want some "guarantee" rather than a "will probably be saved". You can reproduce cases in which data gets lost (commit and quick app crash or closing).

yuroyami commented 10 months ago

Under the hood, .commit() simply executes the operation on the current thread, whilst .apply() delegates it to an ephemeral background thread not controlled by you. .commit() rather gives you thread control, you can make use of the kotlin coroutines to do what you're aiming for:

val prefScope = CoroutineScope(Dispatchers.IO + prefMotherJob)

...

prefScope.launch {
    KVault.set()...
}
rishabh876 commented 8 months ago

I tried moving set operation of KVault to IO thread. Turns out keystore are not thread safe on some devices. I started seeing

KeyStoreException the master key android-keystore://_androidx_security_master_key_ exists but is unusable

caused by -> IllegalBlockSizeException caused by -> KeyStoreException - Invalid operation handle

After digging a bit more I stumbled upon this thread. https://github.com/FlowCrypt/flowcrypt-android/issues/235#issuecomment-470531823 which suggests its a Thread safety issue.

I would highly recommend adding this thread-safety disclaimer in documentation of Kvault as well. It must always be used on main thread.

cristhianescobar commented 7 months ago

Can someone make a release with this fix please?