add support for reading token from cookie #43

lallenfrancisl · 2021-11-14T17:34:06Z

This PR aims to fix #28 by adding two fields in settings to set the header name and cookie name. Then while reading the token instead of hardcoding the header name or cookie name we read from what is specified in the settings.

Let me know if there is any changes that should be done in the PR

Thank you

lallenfrancisl · 2021-11-14T19:16:36Z

Ouch. Ok will fix this up. Are these tests same as the local tests ?

kanongil

Thanks for the contribution. I'm sure it is a useful feature. However, this needs another pass including tests and docs before it can be merged.

lib/plugin.js

kanongil · 2021-11-14T19:28:06Z

lib/plugin.js

+    // authorization header as the default
+    const authorization =
+        request.headers[settings.headerName]
+            || getTokenFromCookie(request.headers.cookie, settings.cookieName)


I'm not a fan of the custom cookie header parsing. I would prefer that the existing hapi parser is used.

How do you use the existing parser to parse the cookie header value ? Like we need to be able to read the field and its value from something like token=ejTdsfsdhfsdjfsdjfsdfs.sdfsjdflkjsldfkjsdf.sdflsjdlfjlsdf; another=gibberish, so that we get token field's value right ? Could give me some pointers on this ?

https://hapi.dev/api?v=20.2.0#-serverstatename-options / https://hapi.dev/api?v=20.2.0#-await-serverstatesparseheader.

You can also check out https://github.com/hapijs/cookie/blob/master/lib/index.js for an example of how another plugin handles cookies.

ok I will read these and do the changes. This was something I had some doubts about earlier, so thank you for clearing that up

Oh the cookies are already parsed for us to use 😃. I didn't know that. I have done the changes and will push the changes now

kanongil · 2021-11-14T19:51:59Z

lib/plugin.js

-        .required()
+        .required(),
+
+    headerName: Joi.alternatives()


Ideally the string would be restricted to valid header names using an appropriate regex.

kanongil · 2021-11-14T19:52:20Z

lib/plugin.js

+        .try(Joi.string(), Joi.boolean()).default('authorization').optional(),
+
+    cookieName: Joi.alternatives()
+        .try(Joi.string(), Joi.boolean()).default('token').optional()


Ideally the string would be restricted to valid cookie names using an appropriate regex.

But doesn't that reduce flexibility? Also the issue author and also a lot of other people actually have a requirement to use the names that they need. Like for example if they have a refresh token and an auth token then it makes sense for them to have two cookies in which the authentication one might be better named as auth_token

I'm talking about verifying that the string value is valid to include as a cookie name, eg. no spaces, etc.

This is now added, now only valid cookie names and header names are accepted

kanongil · 2021-11-14T19:53:09Z

lib/plugin.js

+        .try(Joi.string(), Joi.boolean()).default('authorization').optional(),
+
+    cookieName: Joi.alternatives()
+        .try(Joi.string(), Joi.boolean()).default('token').optional()


Why a default value? That does not seem safe.

I added a default value for providing a sane default. Having the header named authorization and cookie named token is a common usage that's why I did that. Also it would ensure parity with the current behaviour of using the authorization header

This is a breaking change, since existing implementations will suddenly also validate tokens provided in Cookie: token=<jwt>.

updated this so that by default it only reads from authorization header but not from cookie

lib/plugin.js

- make settings be specified in alphabetic order - ditch using custom parser and just use the parsed tokens from the request object - make sure everything follow the styleguide

lallenfrancisl · 2021-11-14T21:52:05Z

I have done all the changes that I have given thumbs up. Some of the changes that you requested are no longer relevant as they were related to parsing the cookies, but rest of the thumbs upped are done

lallenfrancisl · 2021-11-15T14:49:38Z

The checks are shown as failed because of test coverage right ?

kanongil · 2021-11-15T19:50:45Z

The checks are shown as failed because of test coverage right ?

Yes. Hapi requires all merged code to have 100% test coverage.

You can run the tests locally using: npm test

For more coverage details you can do npm run test-cov-html, and open the generated coverage.html file.

lallenfrancisl · 2021-11-16T05:15:58Z

Is there something I can do about JWKS endpoint error when running the tests ?

devinivy · 2022-07-23T19:25:08Z

@lallenfrancisl my guess is that you may have been seeing JWKS errors on node v17+. If you use node v16 to write the tests, then you shouldn't run into any JWKS errors 👍

These will be resolved in the next version of @hapi/jwt which will support node v18 (#47).

lallenfrancisl · 2022-07-23T22:33:19Z

Oh hey @devinivy kind of forgot about this pr, I will brush this up and do the updates and also try running the tests with node v16

lallenfrancisl · 2022-07-23T23:09:09Z

Ok the issue was that the host when running from macos is set to something else instead of localhost so that was the reason for failing tests. So I set the host to localhost explicitly on everywhere its mocked

lallenfrancisl · 2022-07-24T01:03:11Z

@devinivy @kanongil Updated the pr but now its not backwards compatible. I still haven't updated the tests but before that I wanted to know how you want the behavior to be, should I keep backwards compatibility or a breaking change

devinstewart · 2022-07-24T13:45:03Z

I personally, perhaps selfishly, would like the defaults to currently work as is. I have not tested it out yet, but it looks like I will have no issues with 3.0.0 as I am currently running Node v14 and hapi v20 in CJS.

Basically, if you are sending your JWT as Authorization: Bearer <token>, (case insensitive) no changes are needed to your plugin config.

lallenfrancisl · 2022-07-25T18:39:25Z

Ok cool, then I will make the pr like this. The default behavior will be kept. That is it will accept authorization header by default but it will not read from any cookie by default. Then if cookie name is specified it does not allow you to specify the header name and will only read from cookie name

…ding to settings specified

…hentication works

lallenfrancisl · 2022-07-27T19:37:39Z

@kanongil @devinivy updated the pr with unit tests, requested changes and docs

…n macos

lallenfrancisl · 2022-07-27T20:55:19Z

The lint seems to be the one failing not tests right ? Or is it some issue from my side ?

lallenfrancisl · 2022-07-27T22:05:45Z

Ok bumped @hapi/lab version now the tests should pass

lallenfrancisl · 2022-07-28T13:34:45Z

@kanongil @devinivy Can any one of you trigger the workflow so that I can see if the test breaking issue is fixed ?

lallenfrancisl · 2022-07-30T17:15:17Z

@kanongil could you do the review again with all the changes that happened ?

devinivy

Looks good to me—thank you for the contribution! I just left some final touchups for consistency.

test/utils.js

lib/plugin.js

API.md

lallenfrancisl · 2022-08-29T21:34:14Z

Thanks @devinivy will definitely try to keep these changes next time in mind

add support for reading token from cookie

b6ec9cc

lallenfrancisl mentioned this pull request Nov 14, 2021

Load the JWT from an arbitrary location in the request #28

Closed

lallenfrancisl changed the title ~~add support for reading token from cookie fixes #28~~ add support for reading token from cookie resolves #28 Nov 14, 2021

kanongil requested changes Nov 14, 2021

View reviewed changes

make changes as requested in code review

448954a

- make settings be specified in alphabetic order - ditch using custom parser and just use the parsed tokens from the request object - make sure everything follow the styleguide

lallenfrancisl requested a review from kanongil November 14, 2021 21:52

Merge branch 'hapijs:master' into load-token-from-custom-headers

13329b6

lallenfrancisl changed the title ~~add support for reading token from cookie resolves #28~~ add support for reading token from cookie Jul 24, 2022

lallenfrancisl added 2 commits July 24, 2022 06:30

Update settings validator to check for valid cookie name and header name

0a080dd

Remove fallback behavior and only use either from header or from cookie

e90406e

lallenfrancisl added 5 commits July 28, 2022 00:53

feat: add valid http token validation schema

be7665f

test: add unit test for http token validation schema

3e3c67f

feat: implement accepting token from cookie name or header name accor…

febed8e

…ding to settings specified

test: add unit tests to test if header name and cookie name based aut…

58fb576

…hentication works

docs: update docs

9fecc3a

lallenfrancisl added 3 commits July 28, 2022 01:09

test: update test to include realistic http tokens

84c6c7f

test: fix typo in test title

ea0c21d

test: explicitly set the host as localhost so that tests don't fail i…

3c9c4d4

…n macos

chore: bumped hapi lab version

63ad3de

devinivy approved these changes Aug 29, 2022

View reviewed changes

Minor fixes for consistency

7adf6e3

devinivy reviewed Aug 29, 2022

View reviewed changes

API.md Outdated Show resolved Hide resolved

Minor doc fix.

06a342a

devinivy merged commit eadc1ee into hapijs:master Aug 29, 2022

devinivy added the feature New functionality or improvement label Aug 29, 2022

devinivy self-assigned this Aug 29, 2022

devinivy added this to the 2.2.0 milestone Aug 29, 2022

lallenfrancisl deleted the load-token-from-custom-headers branch August 29, 2022 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for reading token from cookie #43

add support for reading token from cookie #43

lallenfrancisl commented Nov 14, 2021

lallenfrancisl commented Nov 14, 2021 •

edited

kanongil left a comment

kanongil Nov 14, 2021

lallenfrancisl Nov 14, 2021

kanongil Nov 14, 2021

kanongil Nov 14, 2021

lallenfrancisl Nov 14, 2021

lallenfrancisl Nov 14, 2021 •

edited

kanongil Nov 14, 2021

kanongil Nov 14, 2021

lallenfrancisl Nov 14, 2021 •

edited

kanongil Nov 15, 2021

lallenfrancisl Jul 24, 2022

kanongil Nov 14, 2021

lallenfrancisl Nov 14, 2021

kanongil Nov 15, 2021

lallenfrancisl Jul 27, 2022

lallenfrancisl commented Nov 14, 2021

lallenfrancisl commented Nov 15, 2021

kanongil commented Nov 15, 2021

lallenfrancisl commented Nov 16, 2021

devinivy commented Jul 23, 2022 •

edited

lallenfrancisl commented Jul 23, 2022

lallenfrancisl commented Jul 23, 2022

lallenfrancisl commented Jul 24, 2022

devinstewart commented Jul 24, 2022

lallenfrancisl commented Jul 25, 2022

lallenfrancisl commented Jul 27, 2022

lallenfrancisl commented Jul 27, 2022

lallenfrancisl commented Jul 27, 2022 •

edited

lallenfrancisl commented Jul 28, 2022 •

edited

lallenfrancisl commented Jul 30, 2022

devinivy left a comment

lallenfrancisl commented Aug 29, 2022

add support for reading token from cookie #43

add support for reading token from cookie #43

Conversation

lallenfrancisl commented Nov 14, 2021

lallenfrancisl commented Nov 14, 2021 • edited

kanongil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lallenfrancisl Nov 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lallenfrancisl Nov 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lallenfrancisl commented Nov 14, 2021

lallenfrancisl commented Nov 15, 2021

kanongil commented Nov 15, 2021

lallenfrancisl commented Nov 16, 2021

devinivy commented Jul 23, 2022 • edited

lallenfrancisl commented Jul 23, 2022

lallenfrancisl commented Jul 23, 2022

lallenfrancisl commented Jul 24, 2022

devinstewart commented Jul 24, 2022

lallenfrancisl commented Jul 25, 2022

lallenfrancisl commented Jul 27, 2022

lallenfrancisl commented Jul 27, 2022

lallenfrancisl commented Jul 27, 2022 • edited

lallenfrancisl commented Jul 28, 2022 • edited

lallenfrancisl commented Jul 30, 2022

devinivy left a comment

Choose a reason for hiding this comment

lallenfrancisl commented Aug 29, 2022

lallenfrancisl commented Nov 14, 2021 •

edited

lallenfrancisl Nov 14, 2021 •

edited

lallenfrancisl Nov 14, 2021 •

edited

devinivy commented Jul 23, 2022 •

edited

lallenfrancisl commented Jul 27, 2022 •

edited

lallenfrancisl commented Jul 28, 2022 •

edited