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

Support signing OAuth2 requests via a header parameter #34

Merged
merged 1 commit into from Jul 25, 2013

Conversation

@craigcitro
Copy link
Contributor

@craigcitro craigcitro commented Jul 25, 2013

The OAuth2 standard recommends signing requests via a header, instead of via a query parameter. The gist of this is that URIs tend to be logged or copy-pasted, whereas headers are less likely to float around. (I've often seen this with users trying to debug a failing API call on a mailing list or stackoverflow, for instance.)

This is easy enough to support -- I'm happy to send a pull request, but wanted to see if there was a reason query was currently the default. In particular, would switching the default (as a follow-up change) be reasonable?

@hadley
Copy link
Member

@hadley hadley commented May 13, 2013

Sure, sounds good. Not sure why I didn't put in headers - probably just found it easier to debug in the query and then forgot to migrate.

@craigcitro
Copy link
Contributor Author

@craigcitro craigcitro commented Jul 25, 2013

I even added a test before you asked. :P

url$query$access_token <- access_token
list(url = build_url(url), config = config())
})
sign_oauth2.0 <- function(access_token, as_header=TRUE) {

This comment has been minimized.

@hadley

hadley Jul 25, 2013
Member

Spaces around = please

This comment has been minimized.

@craigcitro

craigcitro Jul 25, 2013
Author Contributor

done.

})
sign_oauth2.0 <- function(access_token, as_header=TRUE) {
if (as_header) {
add_headers(

This comment has been minimized.

@hadley

hadley Jul 25, 2013
Member

Can you put this on one line?

This comment has been minimized.

@craigcitro

craigcitro Jul 25, 2013
Author Contributor

done.

url_response <- GET(request_url, config = url_signer)
response_content <- content(url_response)$headers
expect_equal(NULL, response_content$Authorization)
expect_that(request_url, not(equals(url_response$url)))

This comment has been minimized.

@hadley

hadley Jul 25, 2013
Member

Off topic, but do you have any ideas how I could make negations fit into the same pattern as the rest of test_that?

This comment has been minimized.

@craigcitro

craigcitro Jul 25, 2013
Author Contributor

I think the usual approach is to just double the number of assertions (expectations in this case); this is what they do in Python:
http://docs.python.org/2/library/unittest.html#classes-and-functions
I'll see if I can think up something classier ...

@craigcitro
Copy link
Contributor Author

@craigcitro craigcitro commented Jul 25, 2013

Cleaned & squashed. ;)

@hadley
Copy link
Member

@hadley hadley commented Jul 25, 2013

Oh, and a bullet in NEWS please

@hadley
Copy link
Member

@hadley hadley commented Jul 25, 2013

And you should probably re-run document()

@craigcitro
Copy link
Contributor Author

@craigcitro craigcitro commented Jul 25, 2013

Done!

hadley added a commit that referenced this pull request Jul 25, 2013
Support signing OAuth2 requests via a header parameter
@hadley hadley merged commit 53d1989 into r-lib:master Jul 25, 2013
@hadley
Copy link
Member

@hadley hadley commented Jul 25, 2013

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants