Add wrappers for SecItemDelete and SecItemUpdate
#210
Conversation
Thank you for the great library! This PR adds `delete_item`, `update_item` and `ItemUpdateOptions`. The functions reuse `SearchItemParams` to identify which items to modify and so I've also added a `to_dictionary` method to `SearchItemParams`. In fact, it would probably make sense to replace `SearchItemParams::search` with a `search_item` function in line with the add/update/delete API, but I didn't want to make unnecessary changes. Also in the spirit of minimising changes to existing code: - I added a slightly strange API to `ItemUpdateOptions::set_class` to allow for rewriting the secret data without overwriting the class. Alternatively, I could add an `ItemUpdateValue` that is the same as `ItemAddValue`, but doesn't take a class. - `ItemUpdateOptions` is basically a duplicate of `ItemAddOptions`. I could instead define `struct ItemAddOptions(ItemUpdateOptions)` and then just forward all the methods. Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com>
security-framework/src/item.rs
Outdated
| /// This overrides the class provided by `value`. | ||
| /// | ||
| /// If set to `None`, `value`'s class is ignored, and not added to the `CFDictionary`. | ||
| pub class: Option<Option<ItemClass>>, |
There was a problem hiding this comment.
option in option is weird.
Can you explicitly contrast in the docs what's the difference between None Some(None)?
There was a problem hiding this comment.
option in option is weird.
I've added an ItemUpdateValue instead that removes the need for the Option<Option<_>>
security-framework/src/item.rs
Outdated
| @@ -762,6 +917,20 @@ pub fn add_item(add_params: CFDictionary) -> Result<()> { | |||
| cvt(unsafe { SecItemAdd(add_params.as_concrete_TypeRef(), std::ptr::null_mut()) }) | |||
| } | |||
|
|
|||
| /// Translates to `SecItemUpdate`. Use `ItemSearchOptions` to build a `search_params` `CFDictionary` | |||
| /// and `ItemUpdateOptions` to build an `update_params` `CFDictionary`. | |||
| pub fn update_item(search_params: CFDictionary, update_params: CFDictionary) -> Result<()> { | |||
There was a problem hiding this comment.
I'd prefer not to expose bare ObjC types in the high-level Rust wrapper (existing APIs that do that are bad too)
There was a problem hiding this comment.
Should I also bring add_item into ItemAddOptions::add_item and deprecate/remove the old add_item? I could also make ItemAddOptions::to_dictionary private/deprecated?
There was a problem hiding this comment.
Don't worry about add_item, unless you want to. I don't want to throw unrelated refactorings at you.
Here take &ItemSearchOptions and &ItemUpdateOptions
There was a problem hiding this comment.
I figured I may as well deprecate add_item and create an ItemAddOptions::add method. Also an ItemSearchOptions::delete instead of delete_item.
tsoutsman
force-pushed
the
update-delete-keychain-item
branch
from
2c96328
to
a795a23
Compare
Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com>
tsoutsman
force-pushed
the
update-delete-keychain-item
branch
from
a795a23
to
52eddd4
Compare
|
Thank you |
Thank you for the great library!
This PR adds
delete_item,update_itemandItemUpdateOptions. The functions reuseSearchItemParamsto identify which items to modify and so I've also added ato_dictionarymethod toSearchItemParams. In fact, it would probably make sense to replaceSearchItemParams::searchwith asearch_itemfunction in line with the add/update/delete API, but I didn't want to make unnecessary changes.Also in the spirit of minimising changes to existing code:
ItemUpdateOptions::set_classto allow for rewriting the secret data without overwriting the class. Alternatively, I could add anItemUpdateValuethat is the same asItemAddValue, but doesn't take a class.ItemUpdateOptionsis basically a duplicate ofItemAddOptions. I could instead definestruct ItemAddOptions(ItemUpdateOptions)and then just forward all the methods.