[set-keychain-accessibility] - accessibility;

[cladvd]

Integrated the definition of accessibility level for the keychain of iOS

[gaebel]

Hi @cladvd, thanks for the contribution :) I have a few remarks regarding the implementation:

[cladvd]

Hi @gaebel thank you for the advice, I've moved the enum in KVault class and removed the optional on the property.
For the rest, after a lot of tests I've found that the "Accessible" attributes in context method doesn't allow to save and read the property in proper way and for this I've found the solution to add it in the query of Add, Read and Update.
I also updated the DebugViewController in order to print stored value;
Let me know what do you think about these changes.

[gaebel]

Hi @gaebel thank you for the advice, I've moved the enum in KVault class and removed the optional on the property.
For the rest, after a lot of tests I've found that the "Accessible" attributes in context method doesn't allow to save and read the property in proper way and for this I've found the solution to add it in the query of Add, Read and Update.
I also updated the DebugViewController in order to print stored value;
Let me know what do you think about these changes.

After having a second thought about this, I think it should only be needed to put this into the add method. The read methods would only narrow the search down. I'm not sure if I want to put this into the update query or create another method for that matter => set(key: String, accessibility: Accessibility) (wouldn't be available in common). @benjohnde what do you think?

[benjohnde]

Hi @gaebel thank you for the advice, I've moved the enum in KVault class and removed the optional on the property.
For the rest, after a lot of tests I've found that the "Accessible" attributes in context method doesn't allow to save and read the property in proper way and for this I've found the solution to add it in the query of Add, Read and Update.
I also updated the DebugViewController in order to print stored value;
Let me know what do you think about these changes.

After having a second thought about this, I think it should only be needed to put this into the add method. The read methods would only narrow the search down. I'm not sure if I want to put this into the update query or create another method for that matter => set(key: String, accessibility: Accessibility) (wouldn't be available in common). @benjohnde what do you think?

Puh, tough question! I would solely rely on the keychain instance for handling accessibility and probably leave it in the update section as well. I would probably omit it in the first query. By doing so, you could update that accessibility value according to the current keychain instance, if and only if it is resolvable for the current device state.

[benjohnde]

@gaebel The cleaner solution would be to probably have an own set function with accessibility as parameter, but as it wouldn't be available in the common part, the current solution seems to be alright. We could stretch it and add the function in addition to the common part. So it's more the choice of the developer how to handle things on the iOS side, what do you think?

[gaebel]

@gaebel The cleaner solution would be to probably have an own set function with accessibility as parameter, but as it wouldn't be available in the common part, the current solution seems to be alright. We could stretch it and add the function in addition to the common part. So it's more the choice of the developer how to handle things on the iOS side, what do you think?

Yeah, I'd probably remove it from the update function and add a set function in the iOS target for migration purposes.

[benjohnde]

@gaebel The cleaner solution would be to probably have an own set function with accessibility as parameter, but as it wouldn't be available in the common part, the current solution seems to be alright. We could stretch it and add the function in addition to the common part. So it's more the choice of the developer how to handle things on the iOS side, what do you think?

Yeah, I'd probably remove it from the update function and add a set function in the iOS target for migration purposes.

Sounds great!

[cladvd]

I've removed it from update method.
About the migration, I think that should be in charge of the user following a deleting and a new add of the values that want to update with new accessibility value because also with this integration the initialized value (WhenUnlocked) is the default option already used by the system till now and will be used in the future if the accessibility property will not be changed by the user.
Is required only to test the update on the library on a device where was a previous version to check that all already saved property are accessible.
Sorry of I misunderstood some meaning 🙌

[gaebel]

Adding the accessibility to the value (read) query will only narrow the results down, you can remove it.

[gaebel]

I've removed it from update method.
About the migration, I think that should be in charge of the user following a deleting and a new add of the values that want to update with new accessibility value because also with this integration the initialized value (WhenUnlocked) is the default option already used by the system till now and will be used in the future if the accessibility property will not be changed by the user.
Is required only to test the update on the library on a device where was a previous version to check that all already saved property are accessible.
Sorry of I misunderstood some meaning 🙌

Yeah, this would also be a possibility. I may still add get/set functions for the accessibility in the iOS target, I can use those in a unit test to verify the changes. Just remove the accessibility from the query in the value method, then I'll merge your PR. Thanks again for your contribution 😊

[cladvd]

removed 😃