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

Implement Hawk Authentication #446

Merged
merged 3 commits into from Aug 21, 2017

Conversation

Projects
None yet
2 participants
@jgiovaresco
Contributor

jgiovaresco commented Aug 17, 2017

What

  • Implement Hawk Authentication support (Closes #437)

Screenshots

screen shot 2017-08-18 at 09 13 33

Notes

Finally a basic implementation was not so hard to do :)
This implementation is enough for my needs. I've not read the entire spec to handle all specific cases.

What do you think about this ?

@gschier

This comment has been minimized.

Collaborator

gschier commented Aug 18, 2017

I took a look at the HAWK authentication library and it looks like you implemented everything needed for the most common use case. The only thing I might add is the ability to specify the algorithm (sha256 or sha1).

Edit: Just noticed you did add a spot for the algorithm

There is also a bewit authentication type but it doesn't seem like that would be a very popular use case, and I don't think it's necessary to implement response validation.

I will give an official code review tomorrow with some comments around naming and style, but I think it looks good overall. Nice work!

@gschier

A couple comments @jgiovaresco. All minor things. It's looking great!

Also, can you post a screenshot of the auth UI?

@@ -149,6 +150,7 @@ const authTypesMap = {
[AUTH_BEARER]: ['Bearer', 'Bearer Token'],
[AUTH_OAUTH_1]: ['OAuth 1', 'OAuth 1.0'],
[AUTH_OAUTH_2]: ['OAuth 2', 'OAuth 2.0'],
[AUTH_HAWK]: ['Hawk', 'Hawk Authentication'],

This comment has been minimized.

@gschier

gschier Aug 18, 2017

Collaborator

The word "Authentication" is pretty redundant here. Could probably just be ['Hawk', 'Hawk'].

@@ -36,6 +36,8 @@ export async function exportHarWithRequest (renderedRequest, addContentLength =
if (!misc.hasAuthHeader(renderedRequest.headers)) {
const header = await getAuthHeader(
renderedRequest._id,
renderedRequest.url,

This comment has been minimized.

@gschier

gschier Aug 18, 2017

Collaborator

The URL here should actually be the value of what is on line 48. Should pull that into a variable and use it in both places.

export async function getAuthHeader (requestId, authentication) {
export async function getAuthHeader (requestId, requestUrl, requestMethod, authentication) {

This comment has been minimized.

@gschier

gschier Aug 18, 2017

Collaborator

These params can just be called url and method.

@@ -473,6 +473,8 @@ export function _actuallySend (
} else {
const authHeader = await getAuthHeader(
renderedRequest._id,
renderedRequest.url,

This comment has been minimized.

@gschier

gschier Aug 18, 2017

Collaborator

This should actually be using finalUrl instead of renderedRequest.url.

@@ -70,6 +70,7 @@ class AuthDropdown extends PureComponent {
{this.renderAuthType(AUTH_NTLM)}
{this.renderAuthType(AUTH_AWS_IAM)}
{this.renderAuthType(AUTH_NETRC)}
{this.renderAuthType(AUTH_HAWK)}

This comment has been minimized.

@gschier

gschier Aug 18, 2017

Collaborator

Also need to add a default object to models/request.js newAuth() function.

@@ -27,6 +28,27 @@ export async function getAuthHeader (requestId, authentication) {
}
}
if (authentication.type === AUTH_HAWK) {
const {hawkAuthId, hawkAuthKey, algorithm} = authentication;

This comment has been minimized.

@gschier

gschier Aug 18, 2017

Collaborator

Can you rename these to id, key, algorithm to match the Hawk names? Since the authentication type is Hawk, there is no need to prefix the keys.

@jgiovaresco

This comment has been minimized.

Contributor

jgiovaresco commented Aug 18, 2017

@gschier I let you take a look at the 2 new commits. All changes should be done.

@gschier

Might make a few minor tweaks after merging, but looks good! Thanks for the PR 😄

@gschier gschier merged commit 2fcf985 into getinsomnia:develop Aug 21, 2017

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