Skip to content

feat: use viewer credentials by default if possible #391

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.

Sign up for GitHub

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

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

feat: use viewer credentials by default if possible #391

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
821e1a2
first draft of implementation
toph-allen Apr 15, 2025
b2375d2
bugfix
toph-allen Apr 15, 2025
5061a67
small bugfix
toph-allen Apr 15, 2025
d47d247
account for explicit provision of API key
toph-allen Apr 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 43 additions & 4 deletions R/connect.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,12 +965,19 @@
#' @export
connect <- function(
server = Sys.getenv(paste0(prefix, "_SERVER"), NA_character_),
api_key = Sys.getenv(paste0(prefix, "_API_KEY"), NA_character_),
api_key,
token,
token_local_testing_key = api_key,
prefix = "CONNECT",
...,
.check_is_fatal = TRUE) {
if (!missing(api_key)) {
api_key_provided <- TRUE
} else {
api_key_provided <- FALSE
api_key <- Sys.getenv(paste0(prefix, "_API_KEY"), NA_character_)
}
Comment on lines +974 to +979
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@toph-allen toph-allen Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (is.null(api_key) || is.na(api_key) || nchar(api_key) == 0) {
msg <- "Invalid (empty) API key. Please provide a valid API key"
if (.check_is_fatal) {
Expand All @@ -981,21 +988,53 @@
}
con <- Connect$new(server = server, api_key = api_key)

if (api_key_provided) {
return(con)
}

if (on_connect()) {
comp <- compare_connect_version(con$version, "2025.01.0")
if (comp < 0) {
if (!missing(token)) {
# If running on a too-old version of Connect and token was explicitly
# passed, error.
stop(
"Running content using the viewer's credentials ",
"requires Connect v2025.01.0 or later."
)
} else {
# If running on too-old Connect and no token was explicitly passed, use
# the old behavior (run with publisher creds).
message(
"Content will run using publisher's credentials. ",
"Running content using the viewer's credentials ",
"requires Connect v2025.01.0 or later."
)
}
} else {
if (missing(token) && exists("getDefaultReactiveDomain", mode = "function")) {
# If the session token was not provided, see if we can obtain it from
# the Shiny context.
session <- getDefaultReactiveDomain()

Check warning on line 1018 in R/connect.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/connect.R,line=1018,col=20,[object_usage_linter] no visible global function definition for 'getDefaultReactiveDomain'
token <- session$request$HTTP_POSIT_CONNECT_USER_SESSION_TOKEN
}
}
}

if (!missing(token)) {
error_if_less_than(con$version, "2025.01.0")
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
con <- connect(server = server, api_key = token_local_testing_key)
message(paste0(
"Called with `token` but not running on Connect. ",
"Continuing with fallback API key."
))
con <- Connect$new(server = server, api_key = token_local_testing_key)
}
}

Expand Down
Loading