This repository was archived by the owner on May 15, 2024. It is now read-only.
GH-304: Added Remove and RemoveAll methods to SecureStorage
#330
Conversation
This file contains hidden or 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
We no longer consider the `SecAccessible` value when _retrieving_ KeyChain records anymore, so it’s unnecessary to pass this value into the Get call, and unnecessary to have a platform specific overload with this parameter for getting a record.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mattleibow
reviewed
Jun 26, 2018
| var kc = new KeyChain(accessible); | ||
|
|
||
| return Task.FromResult(kc.ValueForKey(key, Alias)); | ||
| } |
Contributor
There was a problem hiding this comment.
What happened to this method?
Contributor
There was a problem hiding this comment.
We have the set below...
Member
Author
There was a problem hiding this comment.
Having an overload with SecAccessible is no longer necessary since the actual get request never uses this value to look up an existing key.
| return Task.FromResult(kc.ValueForKey(key, Alias)); | ||
| } | ||
|
|
||
| public static Task SetAsync(string key, string value, SecAccessible accessible) |
This comment has been minimized.
This comment has been minimized.
Collaborator
|
build |
We don’t use the overload with SecAccessible any longer.
This comment has been minimized.
This comment has been minimized.
jamesmontemagno
previously approved these changes
Jun 26, 2018
Collaborator
|
build |
2 similar comments
Collaborator
|
build |
Collaborator
|
build |
Contributor
Collaborator
|
build |
Collaborator
|
@Redth looks like everything is passing now... not sure why it still said failure |
Collaborator
|
@Redth looks like it may need a reboot as iOS sim wont launch i guess. |
Collaborator
|
build |
1 similar comment
Member
Author
|
build |
Contributor
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description of Change
Adds
Remove(string key)andRemoveAll()API's to SecureStorage.Also removes
GetAsync(string key, SecAccessible accessible)from iOS as we no longer useSecAccessibleit to query for existing records, so this overload is unnecessary.Bugs Fixed
API Changes
List all API changes here (or just put None), example:
Added:
bool SecureStorage.Remove(string key);void SecureStorage.RemoveAll();Removed:
Task<string> GetAsync(string key, SecAccessible accessible);PR Checklist