Shiny integration #157

jcheng5 · 2020-11-04T23:01:53Z

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:

Provide seamless reuse of Shiny-provided auth tokens across gargle-derived packages (googledrive, googlesheets4, etc.).
Allow credentials to be remembered (i.e. not having to log in on every single visit).
Get rid of the oauth callback query params from the URL (code, state, etc.) while the app is being used.
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

jcheng5 · 2020-11-05T00:43:37Z

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

MarkEdmondson1234 · 2020-11-05T07:02:58Z

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?.

jcheng5 · 2020-11-05T15:32:39Z

@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.

jennybc

I've read through this once and can only make minor / rather formal comments. This is definitely not the sort of code I can run in my head, since Shiny is pretty outside my wheelhouse.

As for the drive_token() design question, is there no way for us to put the "consider ambient token" logic inside the AuthState?

jennybc · 2020-11-05T17:20:39Z

R/shiny.R

+    resp <-
+      handle_oauth_callback(req, oauth_app, cookie_opts) %||%
+      handle_logged_in(req, oauth_app, httpHandler) %||%
+      handle_welcome(req, oauth_app, scopes, cookie_opts)


welcome_ui is never used anywhere and, based purely on name, I wonder if maybe it goes here?

Out of curiosity, why does resp need to be assigned?

Yep, welcome_ui is not implemented yet but it will go there.

I assigned resp for two pretty trivial reasons: 1) to get the handle_* calls to be all indented together, and 2) to make it clear that there's a return value here that matters (vs. calling handle_* invoking some side effect, which is the way many web frameworks work).

jennybc · 2020-11-05T17:30:43Z

R/shiny.R

+        # access_token, expires_in, scope, token_type, and id_token
+        # (and possibly refresh_token)
+
+        return(shiny::httpResponse(


Is there a Shiny reason to use an explicit return()?

I often use explicit return() to emphasize that an expression needs to be the return value, and to be resilient to future code changes that might make this no longer the last expression. Especially in an if as in this case (and the one below), as I think the curly braces obscure the last value somewhat.

I know @hadley tends to only use them when necessary, we have a difference of opinion here. This being your package, I'm happy to adhere to whatever style you prefer.

jennybc · 2020-11-05T17:31:00Z

R/shiny.R

+handle_logged_in <- function(req, oauth_app, httpHandler) {
+  if (!is.null(read_creds_from_cookies(req, oauth_app))) {
+    # User is already logged in, proceed
+    return(httpHandler(req))


Same question as above re: return()

jennybc · 2020-11-05T17:31:53Z

R/shiny.R

+  # TODO: Add email?
+
+  auth_url <- httr::oauth2.0_authorize_url(
+    endpoint = gargle_outh_endpoint(),


Suggested change

endpoint = gargle_outh_endpoint(),

endpoint = gargle_oauth_endpoint(),

Actually the way I have it is "correct" 😬

I'm happy to fix the actual function in a separate PR if you like?

Fixed in 848663b, so you can merge/rebase

jennybc · 2020-11-05T17:42:13Z

R/shiny.R

+  auth_url <- httr::oauth2.0_authorize_url(
+    endpoint = gargle_outh_endpoint(),
+    oauth_app,
+    scope = paste(scopes, collapse = " "),


Elsewhere, gargle always adds the "https://www.googleapis.com/auth/userinfo.email" scope and "normalizes" the scopes. The motivation for both is to make a better key for the token, i.e. be able to handle same person dealing with same API with 2 different Google accounts and to not be sensitive to the order in which scopes are provided. I assume the same considerations apply here, but I'm not very confident in that assumption.

gargle/R/Gargle-class.R

Line 138 in 1bab80c

params$scope <- normalize_scopes(add_email_scope(params$scope))

The userinfo.email scope is also just plain handy, because it allows client packages to at least display some information about who we are currently auth'd as. Some APIs have a proper 'who am I?" endpoint, e.g. Drive user, whereas others do not.

Those considerations don't apply here, but I also think there's no reason not to; the Google consent screen doesn't even mention the email and profile scopes, whether you include them or not.

Yeah, thus far at least, the userinfo.email is semi-officially regarded as a non-sensitive scope and IME has never appeared on the consent screen. I suppose that could change one day.

Can you amplify on why this scope would never be handy in a Shiny context, the way it is elsewhere?

The reason email doesn't matter for Shiny, is because Shiny never stores multiple tokens in one place, like the gargle cache does, and thus there never needs to be any way to identify or compare a token (i.e. no need to call Gargle2.0$hash()). (Shiny never uses the gargle cache, every visitor needs to auth themselves to prove they are who they say they are; and those credentials are "cached" in the cookie.)

I think the reason to get this scope is also for the sake of the client package and, potentially, Shiny app -- so not just about labelling tokens in a store. It seems this could be needed to, for example, show the user who they are logged in as.

Sounds good. This'll be in the next commit I make.

jcheng5 · 2020-11-06T21:33:30Z

@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 · 2020-11-11T18:49:04Z

@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:

I'll file a PR on all gargle-derived packages making this change, and change gargle docs to recommend the new pattern.
For all such packages, take note of the minimum version that will work with the new Shiny OAuth.
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.

jennybc · 2020-11-11T18:57:37Z

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 · 2020-11-11T19:14:25Z

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.

jcheng5 · 2020-11-11T19:54:19Z

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 · 2020-11-16T19:00:14Z

@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

jennybc · 2020-11-19T21:07:57Z

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’

jcheng5 · 2020-11-19T21:08:55Z

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

jcheng5 · 2020-11-20T17:55:04Z

R/AuthState-class.R

+    #' @param ... Not used.
+    print = function(...) {
+      withr::local_options(list(gargle_quiet = FALSE))
+      ui_line("<AuthState (via gargle--with Joe's changes)>")


Creating the .auth object directly at the top-level means the code from gargle is snapshotted at build time. This is needed for r-lib/gargle#157, but is a good change even without that PR.

jcheng5 added 3 commits November 2, 2020 20:12

Add require_oauth() wrapper for shinyApp objects

e538f06

Use authenticated encryption for cookie

c766744

Add support for cookie options

6e2f71b

jennybc reviewed Nov 5, 2020

View reviewed changes

jcheng5 added 2 commits November 5, 2020 10:51

Store full-fledged gargle token, not just creds$access_token

c2c9feb

Update example app

132d549

jcheng5 added 2 commits November 9, 2020 18:36

Shiny: Normalize scopes, add email scopes; comments

97b10eb

Add cred_access_decorators extension point

9c44358

Implement welcome UI, sign-in button, logout

5d8d2c5

jcheng5 added 6 commits November 11, 2020 16:16

Oops--correctly invoke off() on exit

4bfc29f

Add (rough) login UI to example app

d0112ca

Correctly revoke token on logout

6aee032

Reorganize Shiny code into multiple files; some comments

1025134

Note about using a PACKAGE_PASSWORD with RStudio Connect

83c3e19

More doc work; close to passing R CMD check

bd35392

jcheng5 commented Nov 20, 2020

View reviewed changes

jcheng5 mentioned this pull request Nov 20, 2020

Don't init .auth object until .onLoad andrie/gcalendr#18

Open

jcheng5 mentioned this pull request Nov 20, 2020

Don't init .auth object until .onLoad benrwoodard/timecardassistant#1

Merged

MarkEdmondson1234 mentioned this pull request Dec 2, 2020

Plan for migration to googleAuthR 2.0 - splitting out into smaller packages MarkEdmondson1234/googleAuthR#198

Open

MarkEdmondson1234 mentioned this pull request Jan 7, 2021

Remove with_shiny() MarkEdmondson1234/googleAuthR#201

Open

hadley mentioned this pull request Jun 18, 2021

Figure out shiny integration r-lib/httr2#47

Open

hadley mentioned this pull request Jul 4, 2021

Get imputed URL for current app/session rstudio/shiny#3460

Open

sckott mentioned this pull request Jan 25, 2024

Proof Login/logout FredHutch/shiny-cromwell#21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shiny integration #157

Shiny integration #157

jcheng5 commented Nov 4, 2020 •

edited

Loading

jcheng5 commented Nov 5, 2020

MarkEdmondson1234 commented Nov 5, 2020 •

edited

Loading

jcheng5 commented Nov 5, 2020

jennybc left a comment

jennybc Nov 5, 2020

jennybc Nov 5, 2020

jcheng5 Nov 6, 2020

jennybc Nov 5, 2020

jcheng5 Nov 6, 2020

jennybc Nov 5, 2020

jennybc Nov 5, 2020

jcheng5 Nov 6, 2020

jennybc Nov 7, 2020

jennybc Nov 7, 2020

jennybc Nov 5, 2020

jennybc Nov 5, 2020

jcheng5 Nov 6, 2020

jennybc Nov 7, 2020

jcheng5 Nov 9, 2020

jennybc Nov 9, 2020 •

edited

Loading

jcheng5 Nov 10, 2020

jcheng5 commented Nov 6, 2020 •

edited

Loading

jcheng5 commented Nov 11, 2020

jennybc commented Nov 11, 2020 •

edited

Loading

jennybc commented Nov 11, 2020

jcheng5 commented Nov 11, 2020

jcheng5 commented Nov 16, 2020

jennybc commented Nov 19, 2020

jcheng5 commented Nov 19, 2020 •

edited

Loading

jcheng5 Nov 20, 2020

	endpoint = gargle_outh_endpoint(),
	endpoint = gargle_oauth_endpoint(),

Shiny integration #157

Are you sure you want to change the base?

Shiny integration #157

Conversation

jcheng5 commented Nov 4, 2020 • edited Loading

TODO

jcheng5 commented Nov 5, 2020

MarkEdmondson1234 commented Nov 5, 2020 • edited Loading

jcheng5 commented Nov 5, 2020

jennybc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcheng5 commented Nov 6, 2020 • edited Loading

jcheng5 commented Nov 11, 2020

jennybc commented Nov 11, 2020 • edited Loading

jennybc commented Nov 11, 2020

jcheng5 commented Nov 11, 2020

jcheng5 commented Nov 16, 2020

jennybc commented Nov 19, 2020

jcheng5 commented Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

jcheng5 commented Nov 4, 2020 •

edited

Loading

MarkEdmondson1234 commented Nov 5, 2020 •

edited

Loading

jennybc Nov 9, 2020 •

edited

Loading

jcheng5 commented Nov 6, 2020 •

edited

Loading

jennybc commented Nov 11, 2020 •

edited

Loading

jcheng5 commented Nov 19, 2020 •

edited

Loading