-
Notifications
You must be signed in to change notification settings - Fork 2k
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
clear token on oauth2.0 error #315
Conversation
This works better. On the first try, I'm seeing:
Only the second try opens a browser for re-authentication. Furthermore, from RStudio, I was able to connect even after I had revoked the access from the Google console and after restarting RStudio. Then I tried from the R console, which led to the behavior above. I'm not sure if this is due to a timeout or some magic cache. |
has_oauth2.0_error <- function(response) { | ||
if (status_code(response) %in% oauth2.0_error_codes) { | ||
content <- content(response) | ||
if (content["error"] %in% oauth2.0_errors) { |
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.
That should be content$error
or content[["error"]]
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.
But it seems like it would be safer to assume that any error implies that it failed.
@@ -211,7 +211,12 @@ Token2.0 <- R6::R6Class("Token2.0", inherit = Token, list( | |||
refresh = function() { | |||
self$credentials <- refresh_oauth2.0(self$endpoint, self$app, | |||
self$credentials, self$params$user_params) | |||
self$cache() |
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 logic here could be made more clear. Maybe:
creds <- refresh_oauth2.0(self$endpoint, self$app,
self$credentials, self$params$user_params)
if (is.null(creds) {
remove_cached_token(self)
warning("Unable refresh token", call. = FALSE)
} else {
self$credentials <- creds
self$cache()
}
self
?
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.
Or maybe the warning should occur in refresh_oauth2.0()
?
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, I think the warning should probably occur in refresh_oauth2.0
in thinking about it.
PTAL |
I'm now reliably seeing the following behavior (using nathangoulding/bigrquery#71):
|
@nathangoulding LTGM - can you please add a bullet to NEWS? |
@krlmlr I think eliminating the second refresh will require a tweak in bigrquery |
Updated NEWS. |
Thanks! |
This implements OAuth2.0 error handling as described in the RFC on refresh, and clears the token locally.
Fixes #311. BigQuery respects the OAuth2.0 RFC, so the behavior he describes is resolved without any changes to
bigrquery
.Tested locally both using the local file cache and not.