initial support for oauth_body_hash on json payloads

[aesopwolf]

Original thread to add oauth_body_hash signing.

This isn't quite ready. Just looking for feedback to see if things look right so far.

Still needs tests and also support for body payloads other than json

cc: @simov

[simov]

Your code is looking good, but I have a few comments:

1. This code is entirely related to oauth, so it should be placed in lib/oauth in separate function (like createHash or something) that's called from onRequest In each of these modules you have access to the request instance through self.request Probably the oauth call code on line 584 should be moved after the condition on line 649, that's the place where you have whatever that is in self.body so this should be used in your method via self.request.body in order to generate the hash and the signature at the same time
2. These comments are overly verbose, you basically have more comments than code there. Putting the link to the spec on top of the method createHash might be good, but the rest of the comments should be removed. And I think your code is perfectly readable without them, not to mention that once you write the tests for this peace of code, the comments will become redundant. (btw if this is helping you while developing this feature - that's fine)
3. options.json can be undefined there, but that should be fixed in 1.

[aesopwolf]

Thanks for the feedback!

1. I followed all your suggestions and it's much better now
2. Yeah comments were just helping me while developing
3. I'm no longer using options.json

Let me know if you have any other thoughts :)

[simov]

This comment should be moved back to the place where it was. It is related to all of the auth schemes not only oauth. Here you can write a short single line comment saying that oauth's body_hash might need the body.

[simov]

Just added two comments in the code. Other than that I think it looks reasonable :) but without tests no one can really say if it works or not.

[aesopwolf]

@simov just added a test!

[simov]

@aesopwolf can you pull this branch into yours? First I simplified your test, then merged master, and finally added a test for manual built body_hash code

Too bad coveralls is still not reporting back the coverage as it should but I think you need one more test for the case where the user is using signature_method:'PLAINTEXT' (we support that) and the buildBodyHash is throwing an error.

[aesopwolf]

Cool, I pulled your branch into mine. Also, I made it throw for PLAINTEXT because of this line from the spec:

If the OAuth signature method is PLAINTEXT, use of this specification provides no security benefit and is NOT RECOMMENDED.

[simov]

Hmm, you forgot to push probably.

[aesopwolf]

I'm pretty sure I pushed. I see your commits in https://github.com/aesopwolf/request/commits/master unless I'm overlooking something?

[simov]

Something got lost in translation, what I'm saying is that you need one more test for the case when the signature_method is 'PLAINTEXT` I was expecting to see the test :) Currently the two tests we have does not cover that path.

[aesopwolf]

gotcha, just added one. Does it make sense to have a test for PLAINTEXT though, considering that's an error case?

[simov]

The test should be actually this :)

tape('body_hash PLAINTEXT signature_method', function(t) {
  t.throws(function() {
    request.post(
    { url: 'http://example.com'
    , oauth:
      { consumer_secret: 'consumer_secret'
      , body_hash: true
      , signature_method: 'PLAINTEXT'
      }
    , json: {foo: 'bar'}
    }, function (err, res, body) {
      t.fail('body_hash is not allowed with PLAINTEXT signature_method')
      t.end()
    })
  }, /oauth: PLAINTEXT signature_method not supported with body_hash signing/)
  t.end()
})

I just searched in the project for tests that throws an error, can you update it and push again? Other than that this PR is starting to look good.

As for having this test case - yes it makes sense. First we're saying that this behavior is expected. Second if someone makes refactoring in future and for some reason breaks that logic, he will be notified.

[aesopwolf]

Thanks, I wasn't aware of the t.throws method. I thought something about having try ... catch in the test felt weird.

Sounds good regarding having this test case. Thanks for clarifying that :)

[simov]

Thanks for your work on this one @aesopwolf 👍

[aesopwolf]

Woohoo! Thank you @simov 😀