SpeziStorage - Review and API proposals #123
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/local/LocalStorage.kt
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Good job. I like the code, I just added a few small comments. @pauljohanneskraft could certainly do a more detailed review regarding the iOS API adaptation.
| putString(sharedPreferencesKey(server, credentials.username), credentials.password) | ||
| override fun retrieveUserCredentials(username: String): List<Credentials> { | ||
| return storage.allKeys().mapNotNull { key -> | ||
| if (key.substringAfter(SERVER_USERNAME_SEPARATOR) == username) { |
There was a problem hiding this comment.
We could consider outsourcing this functionality to a method, as it occurs several times here
private fun isUserKey(key: String, username: String): Boolean = key.substringAfter(SERVER_USERNAME_SEPARATOR) == username
| } | ||
| } | ||
|
|
||
| private fun createCipher(mode: Int, key: Key): Cipher = |
There was a problem hiding this comment.
Could renaming createCipher to e.g. getInitalizedCipher make its purpose of create and initalize clearer?
| sharedPreferences.edit { putFloat(key, value) } | ||
| } | ||
|
|
||
| override fun getByteArray(key: String, default: ByteArray): ByteArray { |
There was a problem hiding this comment.
Is there a reason why getByteArray doesnt exists without the default value?
There was a problem hiding this comment.
not really, since it is using string under the hood and shared prefs provides nullable get for string, added the non default one for bytearray as well now 👍
|
|
||
| override fun getByteArray(key: String, default: ByteArray): ByteArray { | ||
| val encoded = sharedPreferences.getString(key, null) | ||
| return encoded?.let { Base64.decode(it, Base64.DEFAULT) } ?: default |
There was a problem hiding this comment.
Could it make sense to pay attention to whether an error might occur during decoding? Like:
override fun getByteArray(key: String, default: ByteArray): ByteArray {
val encoded = sharedPreferences.getString(key, null)
return try {
encoded?.let { Base64.decode(it, Base64.DEFAULT) } ?: default
} catch (e: IllegalArgumentException) {
logger.e {}
default
}
}Throws: IllegalArgumentException – if the input contains incorrect padding
| ) | ||
|
|
||
| KeyValueStorageType.ENCRYPTED -> { | ||
| val masterKey = MasterKey |
There was a problem hiding this comment.
We could think about lazy initalize and reuse the key, since its an expansive operation.
private val masterKey: MasterKey by lazy {
MasterKey.Builder(context)
.setKeyScheme(MasterKey.KeyScheme.AES256_GCM)
.build()
}| ) | ||
|
|
||
| // when | ||
| val storedLetter = localStorage.read<Letter>( |
There was a problem hiding this comment.
I really like the syntax we got by those changes. 🚀
|
Thank you @eldcn @Basler182 and @pauljohanneskraft for working on the API alignments and merging this! |
SpeziStorage - Review and API proposals
What was done
We have aligned with @pauljohanneskraft that I will bring my proposals in the former PR #108 in a new PR.
KeyValueStoragewhich provides an API to write encrypted data for primitive types in SharedPreferences. This component now got customized to write in shared preferences in encrypted and unencrypted way viaKeyValueStorageFactory#create(fileName, KeyValueStorageType). Consumer of the API have the opportunity to either use the default storages, or create a custom one via the factory. Furthermore, since we were using data store in the previous version of LocalKeyValueStorage, it got removed now and all the functions of the key value storage are not suspending anymore - This is safe as shared preferences caches all the changes in memory first, and writes in the disc asynchronously.SecureStorage-Credentials, on the other hand was providing some methods to create public and private keys from key store (key chain in iOS). Key related methods however, were solely used in the context ofLocalStoragewhen usingLocalStorageSetting.EncryptedUsingKeyStoresetting to store a data in a file.SecureStorageand solely provides CRUD methods to it to handle Credentials. Furthermore, the api got extended to retrieve all credentials of a user, and delete credentials per user and per server separately. Under the hood, theSecureStorageuses an encryptedKeyValueStoragethat persists the encrypted json ofCredentialsobject.CredentialsSecureStorageorCredentialsStorage, but it requires alignment with iOSAndroidKeyStorewhich can be used to create public/private key pairs which then can later on be used in the context ofLocalStorageSetting.Encrypted(KeyPair)SecureStoragewas requestingserveras a separate parameter, while it in my opinion belongs to theCredentialstype - A hint comes also from this swiftlin disable. Hence,Credentialsnow receives a nullable server property as well.SecureStorageItemTypewas indicating three casesKEY; SERVER_CREDENTIALS; NON_SERVER_CREDENTIALS;, howeverKEYwas never used in the context ofSecureStorage. Furthermore, the storage is offering a method todeleteAllCredentials(SecureStorageItemType), ifKEYwould be part of the types, we would be all thePrivateKeyandPublicKeys of the keystore, which is a side effect and a buggy behaviour. I removedKEYfromSecureStorageItemTypewhich complies semantically withCredentialstype, which can either have aserverproperty or not (null).LocalStorage- Keeps similar API as iOS by using kotlin serializationStorageModule.kt.StorageModule(hilt module) serves at the same time also as the public api of the module itself.spezi-storage.mp4
⚙️ Release Notes
Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Include code snippets that provide examples of the feature implemented or links to the documentation if it appends or changes the public interface.
📚 Documentation
Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.
✅ Testing
Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: