gargle icon indicating copy to clipboard operation
gargle copied to clipboard

Shiny integration

Open jcheng5 opened this issue 5 years ago • 11 comments

This might not end up being part of the gargle package, but I'm creating a draft PR here for discussion purposes.

The goal of this branch is to provide a gargle/shiny integration that lets Shiny app visitors log in via Google auth and have access to their own resources (their spreadsheets, their drives, their Gmail account). Compared to the excellent efforts of @MarkEdmondson1234 on googleAuthR, we want to:

  1. Provide seamless reuse of Shiny-provided auth tokens across gargle-derived packages (googledrive, googlesheets4, etc.).
  2. Allow credentials to be remembered (i.e. not having to log in on every single visit).
  3. Get rid of the oauth callback query params from the URL (code, state, etc.) while the app is being used.
  4. Make it as difficult as possible to accidentally use the wrong token!

Definitely a work in progress at the moment; item 1 has not been implemented yet, and that's the most important part.

TODO

  • [x] Intercept .auth accesses (AuthState?)
  • [x] Stir in https://www.googleapis.com/auth/userinfo.email scope, normalize scopes
  • [x] Provide UI for logging out
  • [x] Allow customization of pre-login page (include options for generic login prompt and instant redirect)
  • [x] BUG: After a Shiny+OAuth app has run, googledrive::drive_token() returns NULL
  • [ ] Allow service account token and user token to both be used from Shiny
  • [ ] Code comments, polish
  • [ ] Getting started guide
    • [ ] How to determine what scopes to use
  • [ ] Reference docs, NEWS, vignette
  • [ ] Fully fleshed out example
  • [ ] Tests
    • [ ] Call google API from both server (including async) and ui function (including async)
  • [ ] File PRs on dependent projects to create .auth object on load
    • [x] bigrquery - https://github.com/r-dbi/bigrquery/pull/420 - 1.3.2.9001 ✅
    • [x] gmailr - https://github.com/r-lib/gmailr/pull/147 - 1.0.0.9001 ✅
    • [x] googleAuthR - https://github.com/MarkEdmondson1234/googleAuthR/pull/195 - 1.3.0.9001 ✅
    • [x] googledrive - https://github.com/tidyverse/googledrive/pull/321 - 1.0.1.9001 ✅
    • [x] googlesheets4 - https://github.com/tidyverse/googlesheets4/pull/198 - 0.2.0.9001 ✅
    • [x] ~boxr~ (Doesn't appear to use gargle beyond some JWT unit tests)
    • [x] ~googleCloudStorageR~ (Appears to use googleAuthR rather than gargle directly?)
    • [x] https://github.com/andrie/gcalendr - https://github.com/andrie/gcalendr/pull/18 - 0.2.1.9005
    • [x] https://github.com/benrwoodard/timecardassistant - https://github.com/benrwoodard/timecardassistant/pull/1 - 0.0.0.9551
  • [ ] Give warning if auth is attempted in a Shiny app but the dependent package is known to be old
  • [ ] Give warning if server platform doesn't support cookies
  • [ ] Make sure that a clean install from remotes::install_github brings in all the right versions of everything

jcheng5 avatar Nov 04 '20 23:11 jcheng5

Here's some example code from googledrive that's quoted in the gargle docs:

drive_token <- function() {
  if (isFALSE(.auth$auth_active)) {
    return(NULL)
  }
  if (!drive_has_token()) {
    drive_auth()
  }
  httr::config(token = .auth$cred)
}

# googledrive::
drive_has_token <- function() {
  inherits(.auth$cred, "Token2.0")
}

We need to somehow inject ourselves into this process and let the Shiny-session-owned auth token take precedence over the global .auth.

My first take looked like this:

drive_token <- function() {
  ambient_token <- gargle::ambient_token()
  if (!is.null(ambient_token)) {
    return(httr::config(token = ambient_token))
  }

  if (isFALSE(.auth$auth_active)) {
    return(NULL)
  }
  if (!drive_has_token()) {
    drive_auth()
  }
  httr::config(token = .auth$cred)
}

but this is starting to feel like code that's too specific to be copied and pasted into every gargle-derived packages.

What if this logic lived in gargle?

drive_token <- function() {
  gargle::get_token(auth_state = .auth, auth_func = drive_auth)
}

If we want to give users control over whether they want to respect the ambient creds from Shiny:

drive_token <- function(allow_ambient = TRUE) {
  gargle::get_token(auth_state = .auth, auth_func = drive_auth, allow_ambient)
}

cc @jennybc @hadley

jcheng5 avatar Nov 05 '20 00:11 jcheng5

Really looking forward to this 👍

I presume you want to do this R-server side, but I did find using the client-side JavaScript Google auth scripts ultimately the easiest way to handle things like cookie management etc. (2, 3, 4 on your list) and since Shiny always needs JS no friction for the end users. Perhaps both can be supported (client and server side) in gargle?.

MarkEdmondson1234 avatar Nov 05 '20 07:11 MarkEdmondson1234

@MarkEdmondson1234 Oh, thanks for the pointer; that didn’t even occur to me, I’ve only ever used OAuth myself.

No regrets though, the server side approach is already working well and my intention is to also adapt this code for other OAuth providers like Microsoft/Azure and GitHub. I also don’t want to access cookies from JS, our commercial customers’ automated testing tools often flag non-HttpOnly cookies as a security risk.

jcheng5 avatar Nov 05 '20 15:11 jcheng5

@jennybc Ah, yes, I think AuthState will work! I will need to change cred and auth_active to active properties, but other than that, it looks like it should be simple. Thanks!

(Pretty sure you suggested this in person 1-2 weeks ago but I didn't understand gargle well enough to process the idea then)

jcheng5 avatar Nov 06 '20 21:11 jcheng5

@jennybc There is a (not insurmountable) problem with modifying AuthState's behavior.

This is how .auth is declared in googledrive:

.auth <- gargle::init_AuthState(...)

This code gets executed at package build time, not at package load time. So the AuthState object is sort of frozen and serialized at the time that googledrive is built. Any changes that I make now to AuthState will not apply to that object, even if googledrive is reinstalled (from binary).

What needs to happen instead is something like:

.auth <- NULL

.onLoad <- function(libname, pkgname) {
  .auth <<- gargle::init_AuthState(
    package     = "googledrive",
    auth_active = TRUE
  )
}

Without this change, the symptom will be, if someone tries to use the Shiny OAuth integration then calls to drive_token() will either return nothing, or return a token that is already activated on the googledrive::.auth object.

My proposal is:

  1. I'll file a PR on all gargle-derived packages making this change, and change gargle docs to recommend the new pattern.
  2. For all such packages, take note of the minimum version that will work with the new Shiny OAuth.
  3. While Shiny OAuth is running, register a token_fetch function that detects calls by obsolete versions, and emit a message instructing them to upgrade to the correct version.

jcheng5 avatar Nov 11 '20 18:11 jcheng5

modifying AuthState's behavior

Sounds good.

So the AuthState object is sort of frozen and serialized at the time that googledrive is built.

Well, the logic and capabilities of AuthState are determined at build time, but not the actual tokens that it manages. Right? Tokens persist between sessions via the cache, not via .auth.

But, yes, client packages will need to change to work with Shiny-capable gargle.

Do you agree with this take?

Maybe we can build something into gargle::init_AuthState() that somehow determines it's being called with a client package using the old/original design. Like, maybe check that it's being executed from .onLoad(). Or add a required argument relating to Shiny.

jennybc avatar Nov 11 '20 18:11 jennybc

One thing we haven't discussed is people who want to use a cached OAuth token or a service account token in a Shiny app, i.e. Shiny apps that don't want to auth as the user of the app. Maybe this already "just works" or, more accurately, "still works". Because this is the one thing that was already working with existing gargle and client packages.

jennybc avatar Nov 11 '20 19:11 jennybc

Do you agree with this take?

Yep, 100%.

One thing we haven't discussed is people who want to use a cached OAuth token or a service account token in a Shiny app, i.e. Shiny apps that don't want to auth as the user of the app. Maybe this already "just works" or, more accurately, "still works". Because this is the one thing that was already working with existing gargle and client packages.

This should still work exactly as before; all of the functionality I'm adding is opt-in, by piping your Shiny app object to require_oauth:

shinyApp(ui, server) %>% require_oauth(oauth_app, scopes)

What I am precluding is the possibility of using a cached token or service account while also using require_oauth (i.e. some calls use a cached/service token, some use the visitor's token). This isn't an inherent limitation of the approach, but I thought it was the right place to start, given how disastrous it'd be to accidentally let random visitors use a cached credential, and how difficult it is to detect this when you as a developer are probably testing by logging in using your own credentials that are also cached!

My approach also does nothing for apps that want to make login optional, or to progressively reveal functionality if the user logs in from a button within an app. We'll need to make it clear in the docs exactly what is supported.

jcheng5 avatar Nov 11 '20 19:11 jcheng5

@hadley @jennybc I think the time has come to decide where we want this code to ship.

  • In {gargle}? Then {base64enc} becomes a dependency and {shiny} (and some packages shiny already depends on) becomes a Suggests or Enhances.
  • In {shiny}? Feels too domain-specific, especially given the explicit tie to Google
  • In a new {shinygargle} package
  • In a new {shinyoauth} package - but I don't know whether/when I'd get around to adapting this code for non-Google scenarios

jcheng5 avatar Nov 16 '20 19:11 jcheng5

A few notes from me playing with the PR


> checking dependencies in R code ... NOTE
  Missing or unexported object: ‘shiny::httpResponse’

Do we need a minimum version on shiny? In fact, should it be in Remotes:, i.e. we need a dev version?


> checking Rd cross-references ... WARNING
  Missing link or links in documentation object 'require_oauth.Rd':
    ‘google_signin_button’

jennybc avatar Nov 19 '20 21:11 jennybc

Oh yes so sorry. Needs shiny master. They're going to release to CRAN in January.

jcheng5 avatar Nov 19 '20 21:11 jcheng5