Adding in an implementation for consumer to sign bodies #61
Conversation
|
Changes Unknown when pulling 2f034a9 on rkuncewicz:consumer-implementation into * on omsmith:master*. |
| @@ -1,7 +1,52 @@ | |||
| crypto = require 'crypto' | |||
| URL = require 'url-parse' | |||
There was a problem hiding this comment.
Reason for using url-parse over the built-in url?
| URL = require 'url-parse' | ||
| HMAC_SHA1 = require './hmac-sha1' | ||
| errors = require './errors' | ||
| extensions = require './extensions' |
| if typeof body isnt 'object' or body is null | ||
| throw new errors.ConsumerError 'Must specify body as an object' | ||
|
|
||
| body.oauth_nonce = crypto.randomBytes(Math.ceil(16)).toString('hex').slice(0, 32) |
There was a problem hiding this comment.
Is the slice really necessary here? You're removing bits of randomness just to get a consistent nonce length
| body.oauth_nonce = crypto.randomBytes(Math.ceil(16)).toString('hex').slice(0, 32) | ||
| body.oauth_consumer_key = @consumer_key | ||
| body.oauth_signature_method = 'HMAC-SHA1' | ||
| body.oauth_timestamp = Math.floor(new Date() / 1000) |
| body.oauth_signature_method = 'HMAC-SHA1' | ||
| body.oauth_timestamp = Math.floor(new Date() / 1000) | ||
| body.oauth_version = '1.0' | ||
| body.oauth_callback = 'about:blank' |
| body.oauth_version = '1.0' | ||
| body.oauth_callback = 'about:blank' | ||
|
|
||
| urlObject = new URL(url) |
There was a problem hiding this comment.
Can you parse the url before pulling randomness? If it's not a valid url you're wasting time.
| @body = {} | ||
|
|
||
| sign_request: (url, method = 'GET', body = {}) => | ||
| if typeof url is 'undefined' or (not url?.length) |
There was a problem hiding this comment.
how about isnt 'string instead
| if typeof url is 'undefined' or (not url?.length) | ||
| throw new errors.ConsumerError 'Must specify a non empty URL' | ||
|
|
||
| if typeof method is 'undefined' or method is null |
| @consumer_key = consumer_key | ||
| @consumer_secret = consumer_secret | ||
| @signer = new HMAC_SHA1() | ||
| @body = {} |
There was a problem hiding this comment.
doesn't seem like @body should be a thing
| @signer = new HMAC_SHA1() | ||
| @body = {} | ||
|
|
||
| sign_request: (url, method = 'GET', body = {}) => |
There was a problem hiding this comment.
This doesn't really appear to be doing anything LTI-specific. It's not adding lti params, it's not verifying lti params are present... If it's not going to do that then you might as well use a strictly (and probably better) oauth library to do the signing.
Don't see myself accepting the PR unless you found those additions worthwhile.
There was a problem hiding this comment.
The reasoning behind this was to make it simpler to sign requests as a TC since this library is already using crypto, parsing and building the request string in the proper order, and signing it properly. If you think that this won't be useful for the library going forward I can reevaluate how we are doing this on our side. I wanted to throw this into here because I thought it would be useful for others that are looking easily sign these requests. Let me know!
There was a problem hiding this comment.
All of those details are around proper oauth signatures, which an oauth signing library would also take care of.
Is there no lti-specific details that one would also want to take care of here?
There was a problem hiding this comment.
Maybe i've been generalizing this a bit too much.
The reason i've added this in is because as a tool provider using the ContentItemMessage protocol I need to send data back to the tool consumer that is signed using the key/secret. I could refactor this and put it into the Provider under something regarding signing the ContentItemMessage that needs to be sent back to the tool consumer. That being said it wouldn't necessarily have anything lti-specific in its implementation, but as a tool provider if I am using this library already I wouldn't really want to get another one to sign the message.
|
Closing this, ended up just implementing it outside of the library in our project |
Added a consumer implementation to signed request bodies, as well as some tests.
Let me know if there are some test cases i've missed that would be good to put in