feat: use viewer credentials by default if possible #391
Conversation
| if (!missing(api_key)) { | ||
| api_key_provided <- TRUE | ||
| } else { | ||
| api_key_provided <- FALSE | ||
| api_key <- Sys.getenv(paste0(prefix, "_API_KEY"), NA_character_) | ||
| } |
There was a problem hiding this comment.
This seems equivalent to the original implementation, plus checking if the api_key is NA_character_. If it's NA_character_, it wasn't provided as a function argument or an environment variable.
Am I missing something?
There was a problem hiding this comment.
Yeah — in the original implementation plus checking to see if the key is NA_character_, we can't tell the difference between a key being explicitly provided (connect(api_key=my_api_key)) vs the function being called with no args (connect()). That led to a bug where we always used the viewer token, even if the code explicitly tried to pass in the API key they wanted to use.
It feels a bit more idiomatically R to set optional default values to NULL and check for them with is.null. However, this doesn't work for the token argument, because that is NULL when running locally. I think the function should at least be internally consistent with how it specifies optional vs required args. However this change will make the function signature less informative than the original.
| if (on_connect()) { | ||
| visitor_creds <- get_oauth_credentials( | ||
| con, | ||
| user_session_token = token, | ||
| requested_token_type = "urn:posit:connect:api-key" | ||
| ) | ||
| con <- connect(server = server, api_key = visitor_creds$access_token) | ||
| con <- Connect$new(server = server, api_key = visitor_creds$access_token) |
There was a problem hiding this comment.
This will fail unless the integration is attached to the content. I dont remember where that landed but will you be using the viewer api key integration or something in the rstudio-credentials header?
There was a problem hiding this comment.
The viewer API key for now, as unless I'm mistaken we haven't moved that work forward yet.
I'm aware that it'll fail on first deploy in that case, and (cc @jonkeane) I think we're willing to accept that for now? I do think better error messaging in get_oauth_credentials() in this function are critical to smoothing that experience over! get_oauth_credentials() currently just prints a 400 message, and that should be improved (in a different PR). This function should catch that error and add to the message to specifically advise on the type of integration to select.
@jonkeane I guess whether to proceed with landing this PR now hinges on whether we think "initial deploy fails to run until you add the integration" or "runs with admin credentials" is the least bad user experience. Thoughts?
There was a problem hiding this comment.
Failing on initial deploy until configuration is correct is (unfortunately) pretty common for Connect today, so this isn't much worse than other cases (e.g. when someone needs to set a specific env var). But it is a poor experience, and yet another toe-stub on not having the visitor API key on automatically.
Intent
Automatically assume the viewer's credentials on Connect if possible.
Fixes #384
Approach
As of 2025-04-15, the current draft includes a lot of conditional logic. It flows kind of confusingly right now, and I want to simplify that, but first I want some input on whether I'm thinking this through correctly.
There are two test apps available. The first uses the released version of
connectapi, the second uses this dev version:https://rsc.radixu.com/default-viewer-credential-test-0-7-0/
https://rsc.radixu.com/default-viewer-credential-test-devel/
The function
connect()can be called with an API key or token passed explicitly, or with nothing provided.connect()):* This is only possible in Shiny apps, and on Connect v2025.01.0 or later. This adds more conditional branches to emit the correct messaging.
I'm going to take a second look at the code and different conditions we account for.
Checklist
NEWS.md(referencing the connected issue if necessary)?devtools::document()?