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

oauth1.0 rsa-sha1 signature support #316

Merged
merged 12 commits into from
Jan 8, 2016

Conversation

nathangoulding
Copy link
Contributor

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:

endpoint <- oauth_endpoint(
  request = "https://yourjira.atlassian.net/plugins/servlet/oauth/request-token",
  authorize = "https://yourjira.atlassian.net/plugins/servlet/oauth/authorize",
  access = "https://yourjira.atlassian.net/plugins/servlet/oauth/access-token")
app <- oauth_app("jira", key = "yourkey", secret = "yoursecret")
key <- openssl::read_key("/path/to/key.pem")
token <- oauth1.0_token(endpoint, app, private_key = key)
GET("https://yourjira.atlassian.net/rest/api/2/search?jql=assignee=fred", config(token = token))

Enables support for authenticating against JIRA. Fixes #307. Fixes #308.

@nathangoulding
Copy link
Contributor Author

Switched to use openssl after looking back at the commits and realizing PKI wasn't being used at all anymore.

callback = oauth_callback()))
}

# 1. Get an unauthorized request token
response <- GET(endpoint$request, oauth_sig(endpoint$request, "GET"))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

That was probably my bad merge conflict handling :/

@nathangoulding
Copy link
Contributor Author

I think I've addressed your comments and roxygenned.

@hadley
Copy link
Member

hadley commented Jan 8, 2016

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
Copy link
Member

Choose a reason for hiding this comment

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

I try to avoid using dev roxygen with other packages, but it's not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh OK, my bad. I have to resolve the conflict with NEWS so I'll revert this change as well.

@nathangoulding
Copy link
Contributor Author

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.

hadley added a commit that referenced this pull request Jan 8, 2016
@hadley hadley merged commit fa7b50e into r-lib:master Jan 8, 2016
@hadley
Copy link
Member

hadley commented Jan 8, 2016

Thanks!

@hadley
Copy link
Member

hadley commented Jan 8, 2016

Ooops, just noticed you forgot to document private_key. Could you please do a new PR?

@nathangoulding
Copy link
Contributor Author

Ahh crap, yeah one sec.

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