-
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
Allow OAuth authentication dance with no local webserver #33
Conversation
Sure - I'd be happy to check out a pull request. |
Note that this includes the commit from #45, since I don't know how to tell github to use that pull as the base for this pull request. |
If you now rebase this branch it won't include the changes from the other pull request. Otherwise, you need to make each pr in a separate branch. |
oauth2.0_token <- function(endpoint, app, scope = NULL, type = NULL) { | ||
authorize <- modify_url(endpoint$authorize, query = compact(list( | ||
oauth2.0_token <- function(endpoint, app, scope = NULL, type = NULL, | ||
use_local_webserver = TRUE) { |
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.
How about use_oob = FALSE
as an argument?
Could you please also add a bullet point to the NEWS file? |
So I've hit everything in this change and squashed, thinking I was done. After that, I realized I should probably add a test, but I couldn't think of anything terribly meaningful to test without doing some sort of mocking/stubbing. Would it be worth adding a test to confirm that in a non-interactive session, trying to say |
Some tests would be nice, but it's hard without a OAuth server to test against. I don't think it's worth bothering to test that |
A little googling turns up this: I could make a block that first checks to see if term.ie is available, and if so tries to do an oauth dance with it; I suspect that'll be fragile. Otherwise, I think this is good to go ... |
Allow OAuth authentication dance with no local webserver
How about you implement an OAuth server R package and then we can test against that ;) |
If you can come up with a sane reason someone would want to write an oauth server in R beyond testing this code, we can talk about implementing one. ;) |
I'd like to be able to do the OAuth2 dance with no local webserver at all, i.e. by manually copy-pasting a URL and then an authorization code. This is clearly less exciting as a user experience, but is sometimes a necessity (I often find myself doing the auth dance on a machine which isn't visible to the outside world, or only has SSH access).
I've actually got a version of this I've been using locally, and I'll submit a pull request shortly. In particular, I added an extra
use_local_webserver
argument tooauth2.0_token
, though I'm happy to thread that out as far as is reasonable. I'm also happy for someone to come up with a better name for the flag.This is the same request that was mentioned in #31, but is not quite the same as the solution proposed by #32, unless httpuv is cleverer than I think. ;)