Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add 'key' parameter to askForSecret #226

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kevinushey
Copy link
Contributor

No description provided.

@nwstephens
Copy link

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

@nwstephens
Copy link

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

@nwstephens
Copy link

nwstephens commented May 1, 2021

@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
Copy link

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
Copy link

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:

  • Change askForPassword to askForSecret outright
  • Detect the keyring and expose the proper function: con <- dbConnect(params..., ifelse(keyring_configured(), askForSecret(), askForPassword()))
  • Allow the user to chose the desired function askForCredential<-function(name, encrypted=FALSE){ if(encrypted) rstudioapi::askForSecret(name) else rstudioapi::askForPassword(name) }

@nwstephens
Copy link

I believe this last issue (replacing askForPassword with askForSecret) has been handled in r-lib/keyring#108.

@nwstephens
Copy link

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

@nwstephens
Copy link

nwstephens commented May 13, 2021

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
Copy link

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
Copy link

nwstephens commented May 14, 2021

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: r-lib/keyring#109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants