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 ability to optionally use Hawk payload validation. #1339

Merged

Conversation

Projects
None yet
2 participants
@rpless
Copy link
Contributor

commented Jan 18, 2019

Closes #1321
This change let's a user select whether Hawk Auth should use Payload Validation or not ). In terms of changes to the UI, it adds an additional checkbox to the Hawk Auth tab (which is off by default). I was a little unsure about the change I made to the signature of getAuthHeader so if there's a better way to get at the body and the content type without passing the whole request in let me know and I can adjust this.

@gschier
Copy link
Collaborator

left a comment

Thanks for this! Just one comment

@@ -596,8 +596,7 @@ export async function _actuallySend(
} else {
const authHeader = await getAuthHeader(

This comment has been minimized.

Copy link
@gschier

gschier Jan 21, 2019

Collaborator

finalUrl still needs to be passed here as it might not be the same as renderedRequest.URL.

I'm fine with passing the whole request in (along with finalUrl). However, your change makes the first and last arguments redundant now, so they could be removed.

This comment has been minimized.

Copy link
@rpless

rpless Jan 21, 2019

Author Contributor

Ok so just to confirm, I should change the definition of getAuthHeader to be something like:

export async function getAuthHeader(renderedRequest: RenderedRequest, url: string): Promise<Header | null> {
  const { method, authentication } = renderedRequest;
  const requestId = renderedRequest._id;
...

And then adjust the call-sites and tests accordingly.

@rpless

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

Thanks for reviewing @gschier. I've made those changes. Let me know if there is anything else you would like changed or if I should squash this down to a single commit.

@gschier
Copy link
Collaborator

left a comment

Awesome stuff! It'll be included in the next release 😄

@gschier gschier merged commit d22e05a into getinsomnia:develop Mar 11, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.