Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Replace SharedPreferences with DataStore Preferences #629

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

ovitrif
Copy link

@ovitrif ovitrif commented Mar 14, 2024

Closes #628

This PR replaces the usage of SharedPreferences with DataStore.

Why

Google is recommending in their official documentation to migrate to DataStore from SharedPreferences, and they also provided a very easy-to-use auto-migration tool that will move the data to the new DataStore, then clean up the entries from SharedPreferences.

Their official guide for saving simple data with SharedPreferences displays an alert about it.

For security concerns as well, it is recommended to migrate to DataStore.

How

Calls to read and write to DataStore are forced to run synchronously based on the example provided by the Android team in Use DataStore in synchronous code.

Data Migration

SharedPreferencesMigration is used to migrate the entries stored in the RN_KEYCHAIN shared prefs file to data store. Upon completion, the SharedPreferences get removed.

◐ Preview File System migration
before after
before after

Testing

  • junit test were removed from the project
  • Done manual testing via an app using React Native to verify migrations work seamlessly.
  • Also fixed an issue with concurrent cipher operations, which worked great after the fix.

@ovitrif
Copy link
Author

ovitrif commented Mar 27, 2024

@oblador Any idea why the error about compileSdkVersion is not specified. Please add it to build.gradle in the unit tests?

Could it be a side-effect of adding Kotlin?! 🤷🏻

Copy link

@enahum enahum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a good addition to the library

build.gradle.kts Outdated Show resolved Hide resolved
@DorianMazur
Copy link
Collaborator

@ovitrif Can you update this PR?

# Conflicts:
#	KeychainExample/android/build.gradle
#	README.md
#	android/build.gradle
#	android/src/main/java/com/oblador/keychain/KeychainModule.java
#	android/src/test/java/com/oblador/keychain/KeychainModuleTests.java
#	build.gradle.kts
Reimplement data store refactoring on version 9.1.0
@ovitrif
Copy link
Author

ovitrif commented Nov 5, 2024

@ovitrif Can you update this PR?

Updated sir, also now I could make use of the coroutineScope you introduced with the Kotlin migration, and hot reload works again 🎉 (was actually broken)

@ovitrif
Copy link
Author

ovitrif commented Nov 5, 2024

@DorianMazur Please let me know if you're suggesting other updates 👍🏻

@ovitrif
Copy link
Author

ovitrif commented Nov 5, 2024

FYI I found an issue that is not related to the datastore update but highlighted by it:

Unknown error with alias: pin, error: Cannot update the same operation concurrently. (Ask Gemini)
com.oblador.keychain.exceptions.CryptoFailedException: Unknown error with alias: pin, error: Cannot update the same operation concurrently.
	at com.oblador.keychain.cipherStorage.CipherStorageKeystoreAesCbc.encrypt(CipherStorageKeystoreAesCbc.kt:101)
	at com.oblador.keychain.KeychainModule$setGenericPassword$1.invokeSuspend(KeychainModule.kt:210)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:584)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:793)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:697)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:684)
Caused by: java.lang.IllegalThreadStateException: Cannot update the same operation concurrently.
	at android.security.KeyStoreOperation.handleExceptions(KeyStoreOperation.java:74)
	at android.security.KeyStoreOperation.update(KeyStoreOperation.java:118)
	at android.security.keystore2.KeyStoreCryptoOperationChunkedStreamer$MainDataStream.update(KeyStoreCryptoOperationChunkedStreamer.java:222)
	at android.security.keystore2.KeyStoreCryptoOperationChunkedStreamer.update(KeyStoreCryptoOperationChunkedStreamer.java:156)
	at android.security.keystore2.AndroidKeyStoreCipherSpiBase.engineUpdate(AndroidKeyStoreCipherSpiBase.java:436)
	at javax.crypto.Cipher.update(Cipher.java:1741)
	at javax.crypto.CipherOutputStream.write(CipherOutputStream.java:158)
	at javax.crypto.CipherOutputStream.write(CipherOutputStream.java:144)
	at com.oblador.keychain.cipherStorage.CipherStorageBase.encryptString(CipherStorageBase.kt:369)
	at com.oblador.keychain.cipherStorage.CipherStorageKeystoreAesCbc.encryptString(CipherStorageKeystoreAesCbc.kt:238)
	at com.oblador.keychain.cipherStorage.CipherStorageKeystoreAesCbc.encrypt(CipherStorageKeystoreAesCbc.kt:97)
	at com.oblador.keychain.KeychainModule$setGenericPassword$1.invokeSuspend(KeychainModule.kt:210at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:584at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:793at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:697at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:684Suppressed: java.lang.IllegalThreadStateException: Cannot update the same operation concurrently.
		at android.security.KeyStoreOperation.handleExceptions(KeyStoreOperation.java:74)
		at android.security.KeyStoreOperation.finish(KeyStoreOperation.java:132)
		at android.security.keystore2.KeyStoreCryptoOperationChunkedStreamer$MainDataStream.finish(KeyStoreCryptoOperationChunkedStreamer.java:228)
		at android.security.keystore2.KeyStoreCryptoOperationChunkedStreamer.doFinal(KeyStoreCryptoOperationChunkedStreamer.java:181)
		at android.security.keystore2.AndroidKeyStoreCipherSpiBase.engineDoFinal(AndroidKeyStoreCipherSpiBase.java:626)
		at javax.crypto.Cipher.doFinal(Cipher.java:1957)
		at javax.crypto.CipherOutputStream.close(CipherOutputStream.java:210)

Working on a fix.

Fixed in: eb6df3b

@DorianMazur
Copy link
Collaborator

Great work @ovitrif Thank you! I'll check out that PR and merge it, probably this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace SharedPreferences with DataStore Preferences on Android
3 participants