Android alignment #25

pauljohanneskraft · 2024-10-15T22:10:31Z

Android alignment

♻️ Current situation & Problem

@eldcn brought up a couple of improvement ideas for SpeziStorage. Since we aim to keep the Android and iOS interfaces similar, this pull request brings those changes to the iOS frameworks. This is mostly a conversation starter to discuss those changes and ultimately build the best version of Spezi for both platforms.

@Supereg Please let me know what you think. More context regarding these changes is also contained in StanfordSpezi/SpeziKt#123 and StanfordSpezi/SpeziKt#108.

⚙️ Release Notes

Separates SecureStorage into CredentialStorage and KeyStorage.

📚 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:

I agree to follow the Code of Conduct and Contributing Guidelines.

codecov · 2024-10-15T23:46:54Z

Codecov Report

Attention: Patch coverage is 79.80132% with 61 lines in your changes missing coverage. Please review.

Project coverage is 82.36%. Comparing base (6096237) to head (3626a28).

Files with missing lines	Patch %	Lines
...iSecureStorage/Credentials/CredentialStorage.swift	75.23%	28 Missing ⚠️
Sources/SpeziSecureStorage/Keys/KeyStorage.swift	81.19%	19 Missing ⚠️
...eziSecureStorage/Credentials/CredentialTypes.swift	0.00%	13 Missing ⚠️
...ources/SpeziSecureStorage/SecureStorageError.swift	92.31%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
- Coverage   90.28%   82.36%   -7.92%     
==========================================
  Files           6       10       +4     
  Lines         473      544      +71     
==========================================
+ Hits          427      448      +21     
- Misses         46       96      +50

Files with missing lines	Coverage Δ
Sources/SpeziLocalStorage/LocalStorage.swift	`85.11% <100.00%> (+2.05%)`	⬆️
...ources/SpeziLocalStorage/LocalStorageSetting.swift	`100.00% <100.00%> (+2.71%)`	⬆️
...es/SpeziSecureStorage/Credentials/Credential.swift	`100.00% <100.00%> (ø)`
Sources/SpeziSecureStorage/SecureStorage.swift	`94.21% <100.00%> (+0.24%)`	⬆️
...es/SpeziSecureStorage/SecureStorageItemTypes.swift	`100.00% <100.00%> (ø)`
...ources/SpeziSecureStorage/SecureStorageError.swift	`92.31% <92.31%> (ø)`
...eziSecureStorage/Credentials/CredentialTypes.swift	`0.00% <0.00%> (ø)`
Sources/SpeziSecureStorage/Keys/KeyStorage.swift	`81.19% <81.19%> (ø)`
...iSecureStorage/Credentials/CredentialStorage.swift	`75.23% <75.23%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6096237...3626a28. Read the comment docs.

eldcn

Thanks for adaptions, I would propose to wait for @PSchmiedmayer and @Supereg input as well, but I will leave some additional thoughts on the state and some improvements from my pov:

SecureStorageScope – belongs to the LocalStorage target. In the SecureStorage target, we are only using the accessGroup property of it which can safely be required as a nullable string in the store method. A hint in the code that these components do not belong together: assert(!(.secureEnclave ~= storageScope), "Storing of keys in the secure enclave is not supported by Apple.").

Newly added KeyStorage also belongs to the LocalStorage target.
Dependencies should then be reversed: SecureStorage should depend on LocalStorage – however only as long as we continue to support the deprecations.

After that, we can completely cut the dependency between the two targets - We achieve a lower coupling this way between the targets

eldcn · 2024-10-16T21:13:52Z

Sources/SpeziSecureStorage/Credentials/Credential.swift

-/// A pair of username and password credentials.
-public struct Credentials: Equatable, Identifiable {
+/// A user's credential containing username and password.
+public struct Credential: Equatable, Identifiable {
    /// The username.
    public var username: String
    /// The password.


Should we consider adding server parameter as well? It is not a breaking change if you initialize it with null

This would be the only change that would kind of force us to make a major-version update... Is it really worth it?

Alternatively, we could have those deprecation functions with the server-property being explicit and then we override in the deprecated function just to keep the minor version update. But what behavior should we choose there? Keeping the old struct with the slightly different name but without the server-property, always overriding the struct-property with the function parameter or doing something like server ?? credential.server or the other way around?

nevermind, changed it and deprecated SecureStorage as a whole. We now don't even have SecureStorage on Android anymore, since it isn't needed and just builds a Façade that isn't really necessary.

I think that would make sense to ensure that we don't have to take a breaking change for now.

But soft-deprecation is fine, right? Like I still kept the implementation of SecureStorage on iOS, but marked the whole class as deprecated. So, one can still use it as-is, but internally we are pretty much just calling the new functions instead.

Credential now has a server property, but if you use the deprecated functions, we internally only use the function parameter and ignore what was set in the Credential struct.

Exactly; that would be ideal. Once we tag a new major version we can remove all these deprecated function implementations.

Sounds like a good plan!

eldcn · 2024-10-16T22:04:27Z

Sources/SpeziSecureStorage/Keys/KeyStorage.swift

+    ///   - size: The size of the key in bits. The default value is 256 bits.
+    ///   - storageScope: The  ``SecureStorageScope`` used to store the newly generate key.
+    /// - Returns: Returns the `SecKey` private key generated and stored in the keychain or the secure enclave.
+    @discardableResult


Shouldn't be needed, it's quite an expensive operation and we should at least warn the clients to use the result 😅

I think it would be fine to allow this result be be discardable; given the fact that the key generation already takes a decent amount of time this might not make a huge difference. I don't think we require the user to store or process the private key as they could always get it again if needed.

It may not be obvious, but both on Android and on iOS the key is actually already stored in the create function. So, even if you throw away the result, it wouldn't really matter much... I'm ultimately indifferent about this being marked discardableResult or not though, so I would rather keep it as it was.

Sounds good; we can keep it as it was for now, makes this backward compatible as well 👍

PSchmiedmayer

Thank you for all the improvements; great to see this aligned with Android @pauljohanneskraft!

I only had minor comments, they were mainly targeted at enduring backward compatibility before e update to a few major version.

Thank you for the input @eldcn!

Happy to see this merged once the comments and discussion elements are addressed.

Sources/SpeziSecureStorage/SecureStorage.swift

PSchmiedmayer · 2024-10-21T21:47:09Z

Sources/SpeziSecureStorage/SecureStorage.swift

@@ -306,17 +192,23 @@ public final class SecureStorage: Module, DefaultInitializable, EnvironmentAcces
    ///   - removeDuplicate: A flag indicating if any existing key for the `username` of the new credentials and `newServer`
    ///                      combination should be overwritten when storing the credentials.
    ///   - storageScope: The ``SecureStorageScope`` of the newly stored credentials.
-    public func updateCredentials( // swiftlint:disable:this function_default_parameter_at_end
+    public func updateCredential( // swiftlint:disable:this function_default_parameter_at_end


We will also add a deprecated overload here; please ensure this for all methods that have changed.

I now made sure to not change the interface of SecureStorage at all and just deprecated it as a whole.

Sounds good 👍

PSchmiedmayer · 2024-10-21T21:52:35Z

Sources/SpeziSecureStorage/Keys/KeyStorage.swift

+    ///   - size: The size of the key in bits. The default value is 256 bits.
+    ///   - storageScope: The  ``SecureStorageScope`` used to store the newly generate key.
+    /// - Returns: Returns the `SecKey` private key generated and stored in the keychain or the secure enclave.
+    @discardableResult


I think it would be fine to allow this result be be discardable; given the fact that the key generation already takes a decent amount of time this might not make a huge difference. I don't think we require the user to store or process the private key as they could always get it again if needed.

…le also soft-deprecating it

pauljohanneskraft added 4 commits October 15, 2024 14:58

Separate SecureStorage into CredentialStorage and KeyStorage

39c9377

Group files

ba89616

Update

1b956a5

lint

a8a9c08

pauljohanneskraft self-assigned this Oct 15, 2024

pauljohanneskraft added 5 commits October 15, 2024 16:22

Keep backwards compatibility

9a543d4

lint

49e8dd0

lint

df1071d

Move deprecated API into its own extension

32aaa0b

lint

3a0dcbb

Remove catch-all rethrowing clauses

a6d78e8

eldcn reviewed Oct 16, 2024

View reviewed changes

PSchmiedmayer approved these changes Oct 21, 2024

View reviewed changes

PSchmiedmayer added the enhancement New feature or request label Oct 21, 2024

pauljohanneskraft added 5 commits October 21, 2024 15:11

Update

f2e2bd3

Make sure to keep 100% of the original interface of SpeziAccount, whi…

d964013

…le also soft-deprecating it

Deprecate more types

430fac2

REUSE

e922a4f

Fix

3626a28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android alignment #25

Android alignment #25

pauljohanneskraft commented Oct 15, 2024

codecov bot commented Oct 15, 2024 •

edited

Loading

eldcn left a comment

eldcn Oct 16, 2024

pauljohanneskraft Oct 21, 2024 •

edited

Loading

pauljohanneskraft Oct 21, 2024

PSchmiedmayer Oct 21, 2024

pauljohanneskraft Oct 22, 2024

PSchmiedmayer Oct 22, 2024

eldcn Oct 16, 2024

PSchmiedmayer Oct 21, 2024

pauljohanneskraft Oct 21, 2024

PSchmiedmayer Oct 22, 2024

PSchmiedmayer left a comment

PSchmiedmayer Oct 21, 2024

pauljohanneskraft Oct 21, 2024

PSchmiedmayer Oct 22, 2024

PSchmiedmayer Oct 21, 2024

Android alignment #25

Are you sure you want to change the base?

Android alignment #25

Conversation

pauljohanneskraft commented Oct 15, 2024

Android alignment

♻️ Current situation & Problem

⚙️ Release Notes

📚 Documentation

✅ Testing

📝 Code of Conduct & Contributing Guidelines

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

eldcn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauljohanneskraft Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PSchmiedmayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 15, 2024 •

edited

Loading

pauljohanneskraft Oct 21, 2024 •

edited

Loading