Skip to content
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

Add option to retrieve oauth2.0 token using basic authentication #310

Merged
merged 4 commits into from Jan 7, 2016

Conversation

grahamrp
Copy link
Contributor

@grahamrp grahamrp commented Jan 1, 2016

Regarding issue #288 this adds an option to oauth2.0_token so that when the authorization code is exchanged for the token it can be done using http basic authentication, rather than the default method of including the app key and secret in the request body.

From the OAuth2.0 Framework RFC it looks like authorization servers should provide the http basic authentication method as a minimum, and the method of providing the app key/secret in the request body is optional. I see that the github api allows both, but fitbit only allows basic authentication.

#' @export
#' @keywords internal
init_oauth2.0 <- function(endpoint, app, scope = NULL, type = NULL,
use_oob = getOption("httr_oob_default"),
is_interactive = interactive()) {
is_interactive = interactive(),
use_basic_auth = NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more clear to make the default value FALSE

@hadley
Copy link
Member

hadley commented Jan 2, 2016

Looks good, apart from a few minor comments. Could you please also a bullet to NEWS?

code = code))
# Send credentials using HTTP Basic or as parameters in the request body
# See https://tools.ietf.org/html/rfc6749#section-2.3 (Client Authentication)
if (isTRUE(use_basic_auth)){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like there's a little too much duplication here. Could you please pull out the common body?

@grahamrp
Copy link
Contributor Author

grahamrp commented Jan 7, 2016

I've pulled out the common body now, thanks. I've also moved the NEWS bullet to the top. I hope the commits are okay - I'm a novice at collaboration on github.

@hadley hadley merged commit 691f48a into r-lib:master Jan 7, 2016
@hadley
Copy link
Member

hadley commented Jan 7, 2016

Thanks!

@grahamrp grahamrp deleted the basic-auth-option branch January 9, 2016 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants