r-lib / keyring

:closed_lock_with_key: Access the system credential store from R
https://keyring.r-lib.org/
Other
196 stars 28 forks source link

keyring_unlock() fails if backend is "env" #74

Closed eliocamp closed 3 years ago

eliocamp commented 5 years ago

I'm using {keyring} in one of my packages and I'm getting this error in some machines:

Error in keyring::keyring_unlock() : attempt to apply non-function
In addition: Warning message:
In default_backend_auto() :
  Selecting ‘env’ backend. Secrets are stored in environment variables

I resolved it by checking if default_backend()$name != "env" before unlocking. Shouldn't keyring_unlock() itself check this (or, alternatively, add an "unlock" function to env backend that does nothing)?

gaborcsardi commented 5 years ago

You don't need to unlock the env backend, it is not possible to lock it.

Maybe it should have dummy lock() and unlock() methods?

eliocamp commented 5 years ago

Yep, that's what I meant by an unlock function that does nothing, sorry I wasn't clear! I think that would further abstract away the specific backend implementation.

gaborcsardi commented 3 years ago

I am not sure about this, actually, because it could mislead you into thinking that the keyring is locked, and in reality it is not. Maybe lock() could have a warning?

gaborcsardi commented 3 years ago

Well, actually, you can call $has_keyring_support() to check if a backend supports keyrings and thus locking:

❯ kb <- backend_env$new()
❯ kb
<keyring backend: ‘env’>
Store secrets in environment variables.

 $get                  query a key from the keyring
 $set                  set a key in the keyring (interactive)
 $set_with_value       set a key in the keyring
 $delete               delete a key
 $list                 list keys in a keyring
 $has_keyring_support  TRUE if multiple keyrings are supported

❯ kb$has_keyring_support()
[1] FALSE

So maybe we do not need to add dummy methods, you just need to check before you lock/unlock?