-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
SpeziStorage #108
SpeziStorage #108
Conversation
Currently, Android uses a different encryption mechanism than iOS for the files, but otherwise it pretty much follows the existing interface from iOS (couldn't find an exact match fast). I left out some of the platform-specific features of iOS (accessGroups, userPresence check, secure enclave, etc), if you know about Android-specific features that should be integrated or could replace the equivalent(s) in iOS, let me know! |
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/local/LocalStorage.kt
Fixed
Show fixed
Hide fixed
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/local/LocalStorageSetting.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/local/LocalStorageSetting.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/secure/SecureStorage.kt
Outdated
Show resolved
Hide resolved
.../storage/src/main/kotlin/edu/stanford/spezi/modules/storage/secure/SecureStorageItemTypes.kt
Outdated
Show resolved
Hide resolved
.../storage/src/main/kotlin/edu/stanford/spezi/modules/storage/secure/SecureStorageItemTypes.kt
Outdated
Show resolved
Hide resolved
.../storage/src/main/kotlin/edu/stanford/spezi/modules/storage/secure/SecureStorageItemTypes.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/local/LocalStorage.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/secure/SecureStorage.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/local/LocalStorageSetting.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/local/LocalStorage.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/local/LocalStorage.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/local/LocalStorage.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/main/kotlin/edu/stanford/spezi/modules/storage/secure/Credentials.kt
Outdated
Show resolved
Hide resolved
modules/storage/src/androidTest/kotlin/edu/stanford/spezi/modules/storage/LocalStorageTests.kt
Outdated
Show resolved
Hide resolved
# 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. - In previous PRs we already implemented `KeyValueStorage` which provides an API to write encrypted data for primitive types in [SharedPreferences](https://developer.android.com/reference/android/content/SharedPreferences). This component now got customized to write in shared preferences in encrypted and unencrypted way via `KeyValueStorageFactory#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` - <img width="738" alt="image" src="https://github.com/user-attachments/assets/dfbb4573-16d1-40fa-ae69-1e1f012a7b4e"> - This component was breaking single responsibility principle in my opinion. On one hand, it was providing methods to store, read, delete and update `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 of `LocalStorage` when using `LocalStorageSetting.EncryptedUsingKeyStore` setting to store a data in a file. - Removes key related from `SecureStorage` and 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, the `SecureStorage` uses an encrypted `KeyValueStorage` that persists the encrypted json of `Credentials` object. - I would propose to rename this component to `CredentialsSecureStorage` or `CredentialsStorage`, but it requires alignment with iOS - ![image](https://github.com/user-attachments/assets/f517d380-2736-49b8-9a9d-e3b1973279fa) - Introduces public component `AndroidKeyStore` which can be used to create public/private key pairs which then can later on be used in the context of `LocalStorageSetting.Encrypted(KeyPair)` - ![image](https://github.com/user-attachments/assets/fe021ed8-af45-4e3b-a245-b69ff20f3aad) - Each method of `SecureStorage` was requesting `server` as a separate parameter, while it in my opinion belongs to the `Credentials` type - A hint comes also from this [swiftlin disable](https://github.com/StanfordSpezi/SpeziStorage/blob/main/Sources/SpeziSecureStorage/SecureStorage.swift#L310). Hence, `Credentials` now receives a nullable server property as well. - `SecureStorageItemType` was indicating three cases `KEY; SERVER_CREDENTIALS; NON_SERVER_CREDENTIALS;`, however `KEY` was never used in the context of `SecureStorage`. Furthermore, the storage is offering a method to `deleteAllCredentials(SecureStorageItemType)`, if `KEY` would be part of the types, we would be all the `PrivateKey` and `PublicKey`s of the keystore, which is a side effect and a buggy behaviour. I removed `KEY` from `SecureStorageItemType` which complies semantically with `Credentials` type, which can either have a `server` property or not (`null`). - `LocalStorage` - Keeps similar API as iOS by using kotlin serialization - ![image](https://github.com/user-attachments/assets/df0cdbde-1ffa-413c-bdae-45cb8b559334) - Every public component of the library is provides as an interface, and the corresponding implementation as an internal component which is bound by default in hilt graph in `StorageModule.kt`. `StorageModule` (hilt module) serves at the same time also as the public api of the module itself. - All components have been tested - A recording of the public API and generated files and their content: https://github.com/user-attachments/assets/45bfad64-22b4-436f-976c-b136642b5395 ## ⚙️ 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](https://github.com/StanfordSpezi/.github/blob/main/DOCUMENTATIONGUIDE.md).* *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](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md) and [Contributing Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md): - [ ] I agree to follow the [Code of Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md) and [Contributing Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md). --------- Co-authored-by: Kilian Schneider <48420258+Basler182@users.noreply.github.com> Co-authored-by: Paul Kraft <pauljohanneskraft@users.noreply.github.com>
b1efc61
to
cafe5d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from my point of view, everything looks great in terms of code. regarding api adaptation to iOS, you and eldi are deeper into it. thank you for the adjustments and the implementation of the feedback. good work @pauljohanneskraft & @eldcn
SpeziStorage
♻️ Current situation & Problem
This pull request adds SpeziStorage functionality to this repository.
⚙️ 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: