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 support for PLAINTEXT signature method #12

Merged
merged 2 commits into from Jan 4, 2015

Conversation

simov
Copy link
Member

@simov simov commented Jan 1, 2015

I was trying to authenticate to Freshbooks using request, and it turned out that they currently support only PLAINTEXT as a signature method http://developers.freshbooks.com/authentication-2/

So I read about it here http://oauth.net/core/1.0/#anchor22 and I figured that it should be the same as the hmacsign minus the base string.

Now obviously the first 3 arguments are not needed in this case, but I kept them here because I just pointed my request's oauth-sign dependency to my patched version and it worked

I can remove these 3 arguments and after this is being published I can patch the request's source code as well, or we can just leave it as it is. Let me know what do you think.

Also summoning @nylen

@nylen
Copy link
Member

nylen commented Jan 2, 2015

I don't like the extra arguments because we export plaintext as its own function. We should not change the signature of sign though, so you could do something like this:

var method
var skipArgs = 1
...
  case 'PLAINTEXT':
    method = plaintext
    skipArgs = 4
...
return method.apply(null, [].slice.call(arguments, skipArgs))

@simov
Copy link
Member Author

simov commented Jan 2, 2015

Right, @nylen can you take a look at my last commit as well? I just added 2 simple unit tests to prove our point.

@nylen
Copy link
Member

nylen commented Jan 4, 2015

👍

New npm version coming up shortly, then I can write tests and docs this week and get this merged into request, unless you want to beat me to it :)

nylen added a commit that referenced this pull request Jan 4, 2015
Add support for PLAINTEXT signature method
@nylen nylen merged commit 99fceca into request:master Jan 4, 2015
@nylen
Copy link
Member

nylen commented Jan 4, 2015

version 0.6.0 is out.

@simov
Copy link
Member Author

simov commented Jan 4, 2015

Thanks, about request - that would be great, let me know if I need to add anything about this feature there 👍

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