oauth1.0 rsa-sha1 signature support #316
Conversation
|
Switched to use |
| callback = oauth_callback())) | ||
| } | ||
|
|
||
| # 1. Get an unauthorized request token | ||
| response <- GET(endpoint$request, oauth_sig(endpoint$request, "GET")) |
There was a problem hiding this comment.
I'm slightly concerned whether this change is safe in general, but I tried it out with four oauth1 demos, and it seemed to be ok
There was a problem hiding this comment.
Yeah, I'm hyper sensitive to not wanting to introduce regressions with this change, though the RFC5849 and its predecessor are fairly prescriptive that it should be POST unless otherwise noted:
The client obtains a set of temporary credentials from the server by
making an authenticated (Section 3) HTTP "POST" request to the
Temporary Credential Request endpoint (unless the server advertises
another HTTP request method for the client to use).
And:
To request an Access Token, the Consumer makes an HTTP request to the
Service Provider's Access Token URL. The Service Provider documentation
specifies the HTTP method for this request, and HTTP POST is RECOMMENDED.
The request MUST be signed per Signing Requests, and contains the
following parameters:
There was a problem hiding this comment.
Is it worth it to introduce the request_method = "POST" and access_method = "POST" parameters to oauth_endpoint() so someone can specify what HTTP method to use, if this change does have a greater than 0% chance of introducing problems?
There was a problem hiding this comment.
Given that the RFC prescriptive and the change works for four implementations in the wild, I'd say it's fine to change the default. Let's not worry about making it optional until someone actually complains about it.
There was a problem hiding this comment.
I'm going to start the release process either later today or on Tuesday. I'll give a two week notice, and explicitly encourage people to test it if they're using OAuth.
| @@ -110,7 +111,7 @@ init_oauth2.0 <- function(endpoint, app, scope = NULL, user_params = NULL, | |||
| req <- POST(endpoint$access, encode = "form", body = req_params, | |||
| authenticate(app$key, app$secret, type = "basic")) | |||
| } else { | |||
| body$client_secret <- app$secret | |||
| req_params$client_secret <- app$secret | |||
There was a problem hiding this comment.
It seems like there was a regression introduced with the basic auth PR. If you'd rather this be in its own PR just let me know.
There was a problem hiding this comment.
That was probably my bad merge conflict handling :/
|
I think I've addressed your comments and roxygenned. |
|
LGTM - can you please add a bullet to NEWS? |
| @@ -27,4 +27,4 @@ Suggests: | |||
| VignetteBuilder: knitr | |||
| License: MIT + file LICENSE | |||
| URL: https://github.com/hadley/httr | |||
| RoxygenNote: 5.0.1 | |||
| RoxygenNote: 5.0.1.9000 | |||
There was a problem hiding this comment.
I try to avoid using dev roxygen with other packages, but it's not a big deal.
There was a problem hiding this comment.
Ahh OK, my bad. I have to resolve the conflict with NEWS so I'll revert this change as well.
|
OK all set with NEWS and DESCRIPTION. Let me know if you want me to close this, rebase, and re-open a new PR to clean up the commit log. |
oauth1.0 rsa-sha1 signature support
|
Thanks! |
|
Ooops, just noticed you forgot to document |
|
Ahh crap, yeah one sec. |
Implements RSA-SHA1 signature support for OAuth1.0. Assuming you've created an application link according to this documentation with your local RSA private key or
key.pem:Enables support for authenticating against JIRA. Fixes #307. Fixes #308.