iOS Keychain - Use accessiblity option for all operations and throw PlatformException for SecErrors #602
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@juliansteenbakker Could you review the changes? |
|
Thank you for your work! Sorry for the late response, i'm planning on picking up some overdue work this plugin in the comming days. I think it will be good to add the same fixes on macOS. I can test them since i do have a mac. If you can create a separate PR for this, and for the new fetching and listening functions that would be great. Also, can you update this PR with the latest code base available? |
|
Thanks for your feedback. I plan to work on it on Saturday. |
|
PR is now update to date with latest code base |
This was referenced Oct 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the issue that the accessiblity option was ignored for containtsKey and delete. Since all functions now require the conversion for the accessiblity option, I refactored that into a seperate function.
While working on this, I noticed that errors were not being propagated properly. So I improved the error propagation so that now errors will lead to a PlatformException.
I also noticed that the implementation of containsKey could be optimized. Reading the value is not neccessary therefore I removed it and use the status to determine if the key is contained.
All changes currently apply to iOS only. Since the macOS implementation is almost identical, I could apply the fixes there too, but I can't test them because I don't have a Mac. What's your oppinion on that?
The work on this PR started as an attempt to fix #524. The changes does not fully fix the issue descripted in #524. To do fix them you need to wait until the function
applicationProtectedDataDidBecomeAvailableis called and stop reading values whenapplicationProtectedDataWillBecomeUnavailableis called. I created a Gist containing the necessary code to do that. https://gist.github.com/bierbaumtim/63c5057fc2889558d1801d95d93b6c6cI think this plugin should support fetching and listening to changes in the availability of the keychain. But I want to know your opinion on that and if support should be added, should this be a seperate PR?