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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RSA-SHA1 signing to OAuth 1.0 #611

Merged
merged 2 commits into from Jan 16, 2018

Conversation

Projects
None yet
2 participants
@c22
Contributor

c22 commented Nov 20, 2017

Attempts to close #606 but isn't quite there.

I took stab at #606, couldn't wrap my head around rendering on events (ie. hiding the privateKey input until the correct dropdown is selected) so I tried to get the basics of just a key input box to render and now I get Render Failure: CodeEditor is not defined whenever I try to access the OAuth 1 menu. Obviously this should not be merged as is, because it completely breaks the OAuth 1 UI, but maybe I can get some assistance?

As far as the actual logic for RSA_SHA1 goes, I think it should work but I haven't been able to test it because I've broken the UI 馃憤

@gschier

This comment has been minimized.

Collaborator

gschier commented Nov 20, 2017

Hey @c22, this looks like a great start. To use the CodeMirror component, you'll have to add the import first to the top of the file 馃憤

@c22

This comment has been minimized.

Contributor

c22 commented Nov 21, 2017

Thanks again for the pointer @gschier. I have made the changes, as well as added a couple workarounds to get the PEM encoded key to play nice with ddo/oauth1.0a. I managed to get it all working in a REPL but somewhere along the line I seem to have made a change which causes the web request to just not send from within the Electron dev app ("Loading" indefinitely when I click send).

Any chance you could have another look and see if it's something simple I've missed?

@c22

This comment has been minimized.

Contributor

c22 commented Nov 21, 2017

I checked the test output and I can see there's an issue with the way I am overriding the getSigningKey method from oauth-1.0a.

What's the right way to do this? Feels kinda hacky using this library for RSA-SHA1 signing, even though there are GitHub issues on the project that say it's supported. Perhaps it's the way I am using it.

@gschier

This comment has been minimized.

Collaborator

gschier commented Nov 21, 2017

Would it produce the same result if you decoded the token in the hash function instead of overriding the method to prevent encoding? Always best to try and avoid overriding undocumented functions.

The error that you see is from the Flow type checker because it does not know about that function. For it to work, you must define it on here:

type OAuth1Config = {
consumer: {
key: string,
secret: string
},
signature_method: 'HMAC-SHA1' | 'RSA-SHA1' | 'PLAINTEXT';
version: '1.0',
hash_function: (signatureMethod: SignatureMethod, key: string) => string
};

@c22

This comment has been minimized.

Contributor

c22 commented Nov 23, 2017

@gschier unfortunately not, as the encoding happens further down the call stack. Either getSigningKey needs to be overridden to not call percentEncode, or getSignature needs to be overridden so that it doesn't call getSigningKey.

I tried unwinding the stack essentially re-implementing all of oauth-1.0a's authorize - which would mean not having to override any functions - but then I end up in all sorts of type safety hell and have to add more and more types and signatures in from oauth-1.0a (so far I think the types definition file for oauth-1.0a has doubled in size and I'm still getting complaints from flow check).

Perhaps I am barking up the wrong tree and should see if @ddo would be willing to merge some changes to oauth-1.0a which would improve interoperability with RSA-SHA1 signing, first.

@gschier

This comment has been minimized.

Collaborator

gschier commented Nov 23, 2017

I'm actually fine with the hacky solution for now. For the type definition, you should just be able to add something like this:

image

Also, can you add an RSA-SHA1 test case here? That way we'll be able to catch any breaking changes if the lib gets updated/changed by mistake. https://github.com/getinsomnia/insomnia/blob/develop/app/network/__tests__/authentication.test.js

@c22

This comment has been minimized.

Contributor

c22 commented Nov 23, 2017

@gschier done, though I really must be losing it as now requests do not seem to be working again (I swear they were working earlier today). Do you see the culprit?

Edit: Also, http://term.ie/oauth/example/?sig_method=RSA-SHA1 comes in handy for testing.

@gschier

This comment has been minimized.

Collaborator

gschier commented Nov 24, 2017

Everything seems to look fine at first glance. Are there any errors in the Chrome DevTools console?

@gschier

This comment has been minimized.

Collaborator

gschier commented Dec 4, 2017

Were you able to get this working @c22?

@c22

This comment has been minimized.

Contributor

c22 commented Dec 5, 2017

@gschier I put it on the back-burner as I wasn't sure why things were only working sometimes. I just took another look now and it seems that the build scripts have changed, also I can't even get it to compile properly in the develop branch (I have been developing against the develop branch).

I'm getting the following issues:

lerna ERR!     The module '../packages/insomnia-libcurl/node_modules/insomnia-node-libcurl/lib/binding/node_libcurl.node'
lerna ERR!     was compiled against a different Node.js version using
lerna ERR!     NODE_MODULE_VERSION 54. This version of Node.js requires
lerna ERR!     NODE_MODULE_VERSION 57. Please try re-compiling or re-installing
lerna ERR!     the module (for instance, using `npm rebuild` or `npm install`).

I get these errors from the develop branch even without my changes applied. I'm running Node v8.9.1 if that helps any.

When I try to run the app, despite the errors above, I have an issue where the menus aren't showing properly. Should I use a different branch?

@gschier

This comment has been minimized.

Collaborator

gschier commented Dec 5, 2017

Hey @c22,

Yes, that must be a pain. I just rearranged the whole project. I can do the merge if you want to leave your branch pointing to an older version of develop.

As for the error, you should just need to run npm run bootstrap to rebuild the native modules for the Electron runtime.

@c22

This comment has been minimized.

Contributor

c22 commented Dec 5, 2017

@gschier sorry, I probably should've been more specific. I have run bootstrap, I tried doing a npm run clean and then bootstrap then npm run build-app, I still get the same issue. I then deleted the entire repo, cloned from the latest develop branch and tried from scratch doing bootstrap, app-build and then test, I still get the same issue.

I just tried now manually going into the ../packages/insomnia-libcurl/ directory, removing node_modules and then running npm install which rebuilt the module, but then I end up with a different error, TypeError: Curl is not a constructor.

Any other ideas?

@gschier

This comment has been minimized.

Collaborator

gschier commented Dec 6, 2017

Hmm, that's quite strange. I've never seen that error before. Seems like that may have been caused by a failed build.

I just tried this on my end, which worked. I also tried it on a fresh clone with no problems.

# With Node 8 available
npm run clean && npm run bootstrap && npm run app-start

Let me know if that series of steps works. You shouldn't need to do anything else. If it worked for you before the refactor it should work for you now so I'm not quite sure what's going on.

@c22

This comment has been minimized.

Contributor

c22 commented Dec 6, 2017

@gschier the reason that works for you without any issues at all is I'm guessing you have lerna installed globally. npm run bootstrap doesn't work without lerna installed first, so you need to run npm install before npm bootstrap if you don't have lerna installed globally.

Anyway, after a lot of fiddling around I managed to get an environment which doesn't break. I'm not sure what I did, so I just have to be careful not to sneeze on it. I suspect after all the issues I've been having that it is indeed something on my end, so thanks for your help. I'll let you know how my changes go on the latest dev branch.

@gschier

This comment has been minimized.

Collaborator

gschier commented Dec 6, 2017

Well this is weird. I don't have Lerna installed globally and just tried the command again and it failed to find lerna. Not sure how/why it worked the time before...

Lerna is a dev dependency so it would work if you ran npm install first. I'm going to update the bootstrap script to install the root dependencies first before referencing lerna.

Thanks for being patient through this. The codebase is kind of in a bad state right now since everything changed. I'm going to try to see if I can make some changes to ease development for everyone.

@c22

This comment has been minimized.

Contributor

c22 commented Dec 6, 2017

I rebased against the dev branch and I can happily say RSA-SHA1 signing works! 馃榾

I've tested it against http://term.ie/oauth/example/?sig_method=RSA-SHA1 and against the Xero API, both are working.

@gschier did you want to take a crack at hiding the key input unless RSA-SHA1 is selected from the dropdown box? Or should that be captured in a separate issue?

@c22

This comment has been minimized.

Contributor

c22 commented Jan 14, 2018

Any chance to look at this @gschier?

@gschier

This comment has been minimized.

Collaborator

gschier commented Jan 16, 2018

Going to try and get this done today! Haven't really done any work on the app since the holidays but am about to get back into things.

@gschier

Looks great @c22! Thanks for this 馃槃

@gschier gschier merged commit dd51487 into getinsomnia:develop Jan 16, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@welcome

This comment has been minimized.

welcome bot commented Jan 16, 2018

Congrats on merging your first pull request! 馃帀馃帀馃帀 You're helping make Insomnia awesome! 馃檶

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment