rstudio / rstudioapi

Safely access RStudio's API (when available)
http://rstudio.github.io/rstudioapi
Other
165 stars 35 forks source link

add 'key' parameter to askForSecret #226

Open kevinushey opened 3 years ago

nwstephens commented 3 years ago

@kevinushey this looks really good. could you add a similar change to askForPassword()?

nwstephens commented 3 years ago

Thank you, @kevinushey, I will test out deployments to Connect with askForPassword()!

nwstephens commented 3 years ago

@kevinushey I published a Shiny app with askForPasword("test", key="test") and got the following warning message:

The secret associated with key 'test' is not set or could not be retrieved.

Matching the key to the environment variable requires me to pull up the help menu. Instead, it would be easier if the warning message read:

The secret associated with the environment variable 'RSTUDIOAPI_SECRET_test' is not set or could not be retrieved.

Also, I noticed that the key "test" was entered as lowercase, but RStudio Connect expected the key to be upper case. In other words RSTUDIOAPI_SECRET_test failed in RStudio Connect, even though that's what I would have expected. Instead, the app only worked when I named the environment variable to RSTUDIOAPI_SECRET_TEST, which isn't what I specified in my code.

nwstephens commented 3 years ago

I talked to @kellobri , and she said the argument name key could be misleading. I tend to agree. Maybe change the argument name key to something more generic like id, var, input, or name.

nwstephens commented 3 years ago

I would like to make askForSecret available to the pro driver snippets. If we change the behavior of the snippets, we will want to make sure that it still works on previous versions of RStudio. Here are a few ideas on how to change the defaults:

nwstephens commented 3 years ago

I believe this last issue (replacing askForPassword with askForSecret) has been handled in https://github.com/r-lib/keyring/pull/108.

nwstephens commented 3 years ago

I took a crack at fixing these myself in #229.

nwstephens commented 3 years ago

Closing #229. Can we change the key parameter to tag as discussed? Also, can the error message be changed so that users can easily see in the logs what environment variable should be set?

"The %s associated with 'RSTUDIOAPI_SECRET_%s' is not set or could not be retrieved."
nwstephens commented 3 years ago

The warning message says:

The secret associated with tag 'rsc' is not set or could not be retrieved.

I would expect the message to say something like:

The secret associated with the `RSTUDIOAPI_SECRET_RSC` environment variable is not set or could not be retrieved.

I know that the tag argument is designed to be extensible, but right now the code does set this particular environment variable in non-interactive sessions. Is there a middle ground that would allow users to copy/paste the environment variable while still keeping the function extensible?

nwstephens commented 3 years ago

I tested out the new name functionality, and I ran into a problem. If you uncheck Remember with keyring, then the secret is removed from the keying. I would expect the secret to stay in the keyring even if that box is unchecked. Here is a video that illustrates the problem:

https://drive.google.com/file/d/15cmMy3hYGXwqDrsDBbj6_8PvRjTatCE1/view?usp=sharing

I also tested this out on the keyring and rstudioapi packages currently on CRAN and ran into the same issue, so I think this is an independent issue. I will log this under the keyring package as: https://github.com/r-lib/keyring/issues/109