Add initial oauth integration credential exchange impl

[dbkegley]

Adds an initial oauth credential exchange implementation for the R client. @toph-allen I could use some guidance on what needs to be done to get the docs in man/ updated. Other than tests, is there anything else that I'm missing here? Error handling, etc.?

Do I need to define an R6 class for the credentials response? or is returning the raw response object preferred for simple responses?

I ran devtools::document() but there were quite a few changes to files that I didn't touch. Were the docs out of date or should I be generating the docs in a different way?

Example Shiny usage:

# shiny
library(connectapi)
library(shiny)

ui <- fluidPage(textOutput("access_token"))

server <- function(input, output, session) {

  # read the user-session-token header
  user_session_token <- session$request$HTTP_POSIT_CONNECT_USER_SESSION_TOKEN
  
  # init Connect api client
  client <- connect()

  output$access_token <- renderText({
    # token exchange request
    resp <- client$oauth_credentials(user_session_token)
    resp$access_token
  })
}

# start the Shiny app
shinyApp(ui = ui, server = server)

TODO:

• Tests
• Docs
• Update cookbook examples + Connect docs

[dbkegley]

This function may eventually need to support other types besides a user_session_token, should we use a named argument here that defaults to null user_session_token = NULL?

That's what we do on the Python side: https://github.com/posit-dev/posit-sdk-py/blob/0b662ba5d59e0c01afaf17014486832664cb9fac/src/posit/connect/oauth.py#L23

[aronatkins]

The same approach looks to be appropriate; conditionally add the type and token when you have an incoming token.

What other types might be supported? How will the caller determine which field to supply?

[toph-allen]

If user_session_token is not a required argument, then it should default to NULL, yeah. If it is required for now, you don't have to give it a default now.

If you give it a default NULL with no other arguments available, check what is returned when it's called with no argument to make sure the error returned is understandable.

[dbkegley]

Only urn:posit:connect:user-session-token is currently supported. The "urn:posit:connect:api-key" token type is not implemented in Connect yet. I'll omit the default for now since it is required and we can add a NULL default if/when we implement other types of tokens

[aronatkins]

The other option to consider is using separate functions for each source token type. oauth_credentials_from_user_token() and oauth_credentials_from_api_key(), for example. Those are long names, but have the benefit of each having a very specific required argument. You don't need to guess that the first positional argument is the user session token.

connectapi needs to decide if it wants a small surface area (functions) with more complicated argument requirements, or a larger surface area (functions) with simple argument requirements. One general-purpose constructor versus purpose-built constructors.

I think I have a slight preference to what you have now, but that's not a strong position.

[nealrichardson]

Do I need to define an R6 class for the credentials response? or is returning the raw response object preferred for simple responses?

No--in fact, we've been discussing evolving the package away from using R6 for everything. It's not very idiomatic R, and isn't really necessary for this kind of API client.

I'd go farther and say that you can just make this a function that takes client as its argument, rather than a method on Connect. You're not mutating the Connect object, so there's no benefit to attaching it to the R6 class.

[nealrichardson]

I ran devtools::document() but there were quite a few changes to files that I didn't touch. Were the docs out of date or should I be generating the docs in a different way?

I think it's a mix of slightly out of date rendered docs and changes to roxygen2. Double-check that you have the latest roxygen2 package (i.e. that your changes are updating and not reverting previous changes to its logic), but it's not crazy to me that you would have these changes in your PR if you re-ran document().

[aronatkins]

Suggested change

	#' or, to read the token from an inbound HTTP request with Plumbler:
	#' or, to read the token from an inbound HTTP request with Plumber:

[aronatkins]

The same approach looks to be appropriate; conditionally add the type and token when you have an incoming token.

What other types might be supported? How will the caller determine which field to supply?

[toph-allen]

@nealrichardson's right that there were just some stale docs in there. That multiple files changed is alright. On my to-do list is adding something to make this less of an issue going forward (e.g. a CI check to make sure docs are updated).

You definitely don't need to define a class for the response. As Neal said we're thinking about moving away from R6.

Like @nealrichardson says, you could definitely move it to its own function, something like get_oauth_credentials(connect, user_session_token). Accessing functions with $ just feels kinda non-idiomatic, and there is already precedence in the package for not having everything be a method on the R6 object (see other files for examples).

Current conventions aren't really consistent across the package, but IMHO client argument should be called connect (it's also called src in some functions). Check the client with validate_R6_class(connect, "Connect").

I think in service of moving away from R6 classes, let's move this to its own function. You can put it in get.R, which has lots of functions for getting information from the server.

[toph-allen]

• If this is a user-facing function, it needs #' @export at the bottom of its roxygen2 block.
• Consider adding #' @return Description of the value returned
• If you haven't already, good practice to check the docs after running document() to make sure everything looks right. With the package loaded:

> ?oath_credentials

[toph-allen]

If this is a function, and not an active property, write:

Suggested change

	- Implement `connect$oauth_credentials` for interacting with Connect's
	- Implement `connect$oauth_credentials()` for interacting with Connect's

[toph-allen]

If user_session_token is not a required argument, then it should default to NULL, yeah. If it is required for now, you don't have to give it a default now.

If you give it a default NULL with no other arguments available, check what is returned when it's called with no argument to make sure the error returned is understandable.

[dbkegley]

@toph-allen I think I've captured everything, let me know if there's any more changes you'd like to see here

[toph-allen]

Looks good, pending CI passing. I'm gonna look at those issues now.

[toph-allen]

Found what might be the problem with CI

[toph-allen]

Suggested change

	#' @param client A Connect R6 object.
	#' @param connect A Connect R6 object.