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 OAuth1 callback to specify query string parameters #2404

Closed
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
2 participants
@mitar
Collaborator

mitar commented Aug 14, 2014

They are then parsed and provided to underlying HTTP package. We have to parse them so that signing of requests works properly. In addition, nonce used in the request is stored in the response so that user can verify JWT payloads returned from the server.

Allow OAuth1 callback to specify query string parameters.
They are then parsed and provided to underlying HTTP package. We have to
parse them so that signing of requests works properly. In addition,
nonce used in the request is stored in the response so that user can
verify JWT payloads returned from the server.
@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 14, 2014

This is needed to support OAuth integration wiki MediaWiki/Wikipedia: https://github.com/mitar/bib2wikidata/tree/master/packages (Just imagine Wikipedia bots written in Meteor. ;-) )

@mitar mitar referenced this pull request Aug 14, 2014

Open

Getting started #1

var tokens = querystring.parse(response.content);
if (!tokens.oauth_token || !tokens.oauth_token_secret)
throw new Error(
"missing oauth token or secret", tokens);

This comment has been minimized.

@estark37

estark37 Aug 20, 2014

Contributor

Let's not include tokens in the error string; in case one of them is present, we probably don't want it appearing in server logs.

This comment has been minimized.

@mitar

mitar Aug 20, 2014

Collaborator

On the other hand, any error message from the server is in response.content. It is quite useful for debugging. (So maybe we should display response.content if nor tokens.oauth_token nor tokens.oauth_token is available.)

This comment has been minimized.

@estark37

estark37 Aug 20, 2014

Contributor

That sounds reasonable to me.

@@ -52,9 +60,20 @@ OAuth1Binding.prototype.prepareAccessToken = function(query, requestTokenSecret)
oauth_verifier: query.oauth_verifier
});
var response = self._call('POST', self._urls.accessToken, headers);
var accessTokenObj = url.parse(self._urls.accessToken, true);

This comment has been minimized.

@estark37

estark37 Aug 20, 2014

Contributor

Can you factor this out into a helper instead of doing it twice? Maybe splitUrl or something that takes a url with query parameters and returns a url and a params dictionary.

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 20, 2014

Meta-question: do you want me to add a new commit on top of existing, or to edit and squash them together into one?

callback(error, response);
});
// We store nonce so that JWTs can be validated
response.nonce = headers.oauth_nonce;

This comment has been minimized.

@estark37

estark37 Aug 20, 2014

Contributor

response will be undefined here if callback is passed.

@@ -29,7 +31,11 @@ OAuth._requestHandlers['1'] = function (service, query, res) {
if(typeof urls.authenticate === "function") {
redirectUrl = urls.authenticate(oauthBinding);
} else {
redirectUrl = urls.authenticate + '?oauth_token=' + oauthBinding.requestToken;
var redirectUrlObj = url.parse(urls.authenticate, true);

This comment has been minimized.

@estark37

estark37 Aug 20, 2014

Contributor

maybe a comment here that we're parsing the URL to support additional query parameters in urls.authenticate

@estark37

This comment has been minimized.

Contributor

estark37 commented Aug 20, 2014

Thanks @mitar! New commit would be great, and then we might squash it before merging.

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 20, 2014

Tell me when you are done with the review, so that I can get to it. :-)

@estark37

This comment has been minimized.

Contributor

estark37 commented Aug 20, 2014

Done for now :)

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 20, 2014

Done.

// (they are now in params)
parsedUrl.query = {};
parsedUrl.search = '';
url = urlModule.format(parsedUrl);

This comment has been minimized.

@estark37

estark37 Aug 22, 2014

Contributor

Hrm... this doesn't look right to me. For a GET request, we don't necessarily want to wipe out the query parameters. (In fact, we probably don't want to.) Can we put this in a helper function instead and only use it where we are sure we want this behavior? (i.e. prepareRequestToken and prepareAccessToken)

This comment has been minimized.

@mitar

mitar Aug 22, 2014

Collaborator

We are not removing the query parameter, params add it back then?

This comment has been minimized.

@mitar

mitar Aug 22, 2014

Collaborator

Dictionary of request parameters to be encoded and placed in the URL (for GETs) or request body (for POSTs). If content or data is specified, params will always be placed in the URL.

This comment has been minimized.

@estark37

estark37 Aug 22, 2014

Contributor

Oh got it, okay! Sounds good

This comment has been minimized.

@mitar

mitar Aug 22, 2014

Collaborator

The reason is also that one might want to use the same API for doing API requests then to the server, but that is also signed and OAuth1 wrapped.

params: params,
headers: {
Authorization: authString
}
}, callback);
}, callback && function (error, response) {
if (!error) {

This comment has been minimized.

@estark37

estark37 Aug 22, 2014

Contributor

our style guide says if (! error), not if (!error)

@@ -42,6 +50,10 @@ OAuth._requestHandlers['1'] = function (service, query, res) {
// Get the user's request token so we can verify it and clear it
var requestTokenInfo = OAuth._retrieveRequestToken(query.state);
if (!requestTokenInfo) {

This comment has been minimized.

@estark37

estark37 Aug 22, 2014

Contributor

same here, if (! requestTokenInfo)

@@ -55,6 +56,15 @@ OAuth1Binding.prototype.prepareAccessToken = function(query, requestTokenSecret)
var response = self._call('POST', self._urls.accessToken, headers);
var tokens = querystring.parse(response.content);
if (!tokens.oauth_token || !tokens.oauth_token_secret) {

This comment has been minimized.

@mitar

mitar Aug 22, 2014

Collaborator

But here is style OK?

This comment has been minimized.

@estark37

estark37 Aug 22, 2014

Contributor

No, I missed that spot; I'll update it when I merge.

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 22, 2014

Updated code style. Also in few other places. I hope it is OK.

@estark37

This comment has been minimized.

Contributor

estark37 commented Aug 22, 2014

Thanks, merged.

@estark37 estark37 closed this Aug 22, 2014

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 22, 2014

Great! Thanks!

@mitar mitar deleted the peerlibrary:oauth1-params branch Aug 22, 2014

abernix referenced this pull request Oct 17, 2016

Allow OAuth1 callback to specify query string parameters.
They are then parsed and provided to underlying HTTP package. We have to
parse them so that signing of requests works properly. In addition,
nonce used in the request is stored in the response so that user can
verify JWT payloads returned from the server.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment