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

Implement Hawk Authentication #446

Merged
merged 3 commits into from Aug 21, 2017

Conversation

@jgiovaresco
Copy link
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.

Copy link
Contributor

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!

Copy link
Contributor

gschier left a comment

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.

Copy link
@gschier

gschier Aug 18, 2017

Contributor

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.

Copy link
@gschier

gschier Aug 18, 2017

Contributor

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.

Copy link
@gschier

gschier Aug 18, 2017

Contributor

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.

Copy link
@gschier

gschier Aug 18, 2017

Contributor

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.

Copy link
@gschier

gschier Aug 18, 2017

Contributor

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.

Copy link
@gschier

gschier Aug 18, 2017

Contributor

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.

Copy link
Contributor Author

jgiovaresco commented Aug 18, 2017

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

Copy link
Contributor

gschier left a comment

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

@gschier gschier merged commit 2fcf985 into Kong:develop Aug 21, 2017
2 checks passed
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
Projects
None yet
2 participants
You can’t perform that action at this time.