-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
Shiny integration #157
base: main
Are you sure you want to change the base?
Shiny integration #157
Conversation
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 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)
} |
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 |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
R/shiny.R
Outdated
resp <- | ||
handle_oauth_callback(req, oauth_app, cookie_opts) %||% | ||
handle_logged_in(req, oauth_app, httpHandler) %||% | ||
handle_welcome(req, oauth_app, scopes, cookie_opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
welcome_ui
is never used anywhere and, based purely on name, I wonder if maybe it goes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why does resp
need to be assigned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
R/shiny.R
Outdated
# access_token, expires_in, scope, token_type, and id_token | ||
# (and possibly refresh_token) | ||
|
||
return(shiny::httpResponse( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a Shiny reason to use an explicit return()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
R/shiny.R
Outdated
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above re: return()
R/shiny.R
Outdated
# TODO: Add email? | ||
|
||
auth_url <- httr::oauth2.0_authorize_url( | ||
endpoint = gargle_outh_endpoint(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpoint = gargle_outh_endpoint(), | |
endpoint = gargle_oauth_endpoint(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the way I have it is "correct" 😬
I'm happy to fix the actual function in a separate PR if you like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 848663b, so you can merge/rebase
R/shiny.R
Outdated
auth_url <- httr::oauth2.0_authorize_url( | ||
endpoint = gargle_outh_endpoint(), | ||
oauth_app, | ||
scope = paste(scopes, collapse = " "), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Line 138 in 1bab80c
params$scope <- normalize_scopes(add_email_scope(params$scope)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. This'll be in the next commit I make.
@jennybc Ah, yes, I think (Pretty sure you suggested this in person 1-2 weeks ago but I didn't understand gargle well enough to process the idea then) |
@jennybc There is a (not insurmountable) problem with modifying AuthState's behavior. This is how .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 My proposal is:
|
Sounds good.
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 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 |
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. |
Yep, 100%.
This should still work exactly as before; all of the functionality I'm adding is opt-in, by piping your Shiny app object to 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 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. |
@hadley @jennybc I think the time has come to decide where we want this code to ship.
|
A few notes from me playing with the PR
Do we need a minimum version on shiny? In fact, should it be in
|
Oh yes so sorry. Needs shiny master. They're going to release to CRAN in January. |
#' @param ... Not used. | ||
print = function(...) { | ||
withr::local_options(list(gargle_quiet = FALSE)) | ||
ui_line("<AuthState (via gargle--with Joe's changes)>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops
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.
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:
Definitely a work in progress at the moment; item 1 has not been implemented yet, and that's the most important part.
TODO
.auth
accesses (AuthState
?)https://www.googleapis.com/auth/userinfo.email
scope, normalize scopesboxr(Doesn't appear to use gargle beyond some JWT unit tests)googleCloudStorageR(Appears to use googleAuthR rather than gargle directly?)remotes::install_github
brings in all the right versions of everything