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

file: heads up for creating the keyring in set #108

Closed gaborcsardi closed 2 years ago

gaborcsardi commented 3 years ago

Also reverse the order of the dialogs, the key comes first, the keyring password is next.

cc @nwstephens

Closes #107.

nwstephens commented 3 years ago

@gaborcsardi Thanks for reversing the order and fixing the cancel button. I tested this out, and it works great. A few comments:

  1. The keyring::keyring_unlock() pulls up this dialog (see blow), but the title says "Password", which is confusing. I might expect it to say something like, "Unlock the keyring" instead. (I think keeping "Keyring password" in the description is fine).

image

  1. If the keyring has been created, then rstudioapi::askForSecret() prompts for the key first and then they kerying second. However, if the keyring has not been created, then rstudioapi::askForSecret() prompts for the keyring first and then the key. I would expect the order of the dialog boxes to be the same regardless of whether or not the keyring exists.

  2. If the keyring doesn't exist and I run keyring_lock(), I get prompted to create a keyring. This is a little confusing, because it's locking something that doesn't yet exist (while prompting me to create it). I would expect an error message instead saying something like, "keyring doesn't exist, run keyring_create to create one".

gaborcsardi commented 3 years ago

The keyring::keyring_unlock() pulls up this dialog (see blow), but the title says "Password", which is confusing

This is not really an issue with this PR, but in any case, we can't change that. There is no way to change the window title. FWIW it is not shown in RStudio Desktop I think:

Screen Shot 2021-05-11 at 10 57 18

If the keyring has been created, then rstudioapi::askForSecret() prompts for the key first and then they kerying second. However, if the keyring has not been created, then rstudioapi::askForSecret() prompts for the keyring first and then the key. I would expect the order of the dialog boxes to be the same regardless of whether or not the keyring exists.

You mean when the keyring is locked? Yeah, we can change the order, but then isn't it confusing that you type in your password, but then you can't save it because you cannot unlock the keyring?

If the keyring doesn't exist and I run keyring_lock(), I get prompted to create a keyring. This is a little confusing, because it's locking something that doesn't yet exist (while prompting me to create it). I would expect an error message instead saying something like, "keyring doesn't exist, run keyring_create to create one".

Right, agreed.

nwstephens commented 3 years ago

Summary

  1. Changing titles requires changes to RStudio, not keyring. (No change)
  2. Changing the order of the dialogs is possible, but is desirable? (Possible change)
  3. Changing the keyring_lock will be implemented. (Change)
codecov-commenter commented 2 years ago

Codecov Report

Merging #108 (29dbc94) into main (56ce25e) will decrease coverage by 0.14%. The diff coverage is 18.75%.

:exclamation: Current head 29dbc94 differs from pull request most recent head 0bd95ae. Consider uploading reports for the commit 0bd95ae to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
- Coverage   32.05%   31.91%   -0.15%     
==========================================
  Files          15       15              
  Lines        1304     1316      +12     
==========================================
+ Hits          418      420       +2     
- Misses        886      896      +10     
Impacted Files Coverage Δ
R/backend-file.R 87.60% <18.75%> (-2.55%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 56ce25e...0bd95ae. Read the comment docs.