Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

[Secure Storage] Should not throw when removing key that doesn't exist. #561

Closed
domagojmedo opened this issue Oct 25, 2018 · 4 comments
Closed
Labels
bug Something isn't working ready-to-implement Feature approved, specs written, and ready to implement. up-for-grabs Implementation from community can be started.
Milestone

Comments

@domagojmedo
Copy link
Contributor

domagojmedo commented Oct 25, 2018

On SecureStorage iOS implementation

bool RemoveRecord(SecRecord record)
{
        var result = SecKeyChain.Remove(record);
        if (result != SecStatusCode.Success)
            throw new Exception(string.Format($"Error removing record: {result}"));

        return true;
}

It's possible to get

System.Exception: Error removing record: ItemNotFound

Should that status code maybe not throw exception? We are trying to remove it and it's not there, so the end result is the same.

Stack trace I got

Xamarin.Essentials KeyChain.RemoveRecord (Security.SecRecord record)
Xamarin.Essentials KeyChain.SetValueForKey (System.String value, System.String key, System.String service)
Xamarin.Essentials SecureStorage.SetAsync (System.String key, System.String value, Security.SecAccessible accessible)
Xamarin.Essentials SecureStorage.PlatformSetAsync (System.String key, System.String data)
Xamarin.Essentials SecureStorage.SetAsync (System.String key, System.String value)

@domagojmedo domagojmedo changed the title iOS secure storage iOS secure storage exception Oct 25, 2018
@jamesmontemagno
Copy link
Collaborator

So, looking at the code iOS/Android can throw an exception, but UWP will not and just return false, so we should change this.

@jamesmontemagno jamesmontemagno changed the title iOS secure storage exception [Secure Storage] Should not throw when removing key that doesn't exist. Oct 25, 2018
@jamesmontemagno jamesmontemagno added bug Something isn't working up-for-grabs Implementation from community can be started. ready-to-implement Feature approved, specs written, and ready to implement. labels Oct 25, 2018
@Redth Redth added this to the 1.0.0 milestone Nov 5, 2018
@mattleibow
Copy link
Contributor

Fixed!

@domagojmedo
Copy link
Contributor Author

@mattleibow
@jamesmontemagno mentioned this issue in Android also, I couldn't get it or track it down. What's the resolution for it?

@mattleibow
Copy link
Contributor

mattleibow commented Nov 7, 2018

Thanks for the comment. I had a check and it should not throw. SecureStorage is using Preferences under the hood, which is fine for removing keys that do not exist.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working ready-to-implement Feature approved, specs written, and ready to implement. up-for-grabs Implementation from community can be started.
Projects
None yet
Development

No branches or pull requests

4 participants