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

hapi v17 #249

Merged
merged 12 commits into from Feb 26, 2018

Conversation

@salzhrani
Copy link
Contributor

salzhrani commented Nov 9, 2017

update to be compatible with hapi v17 ...
still missing documentation and branch coverage is less the 100 and couldn't figure it out.

some tooling have been updating to be able to use es6 features.

also need linting ... goodparts does not like const or let

fixes #248

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 9, 2017

Codecov Report

Merging #249 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #249   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines         109     92   -17     
=====================================
- Hits          109     92   -17
Impacted Files Coverage Δ
lib/index.js 100% <100%> (ø) ⬆️
lib/extract.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2560e6...eda4df7. Read the comment docs.

@nelsonic

This comment has been minimized.

Copy link
Member

nelsonic commented Nov 9, 2017

@salzhrani thank you for creating this PR! 👍
one of the existing contributors will check it out soon! ⌛️

@samrocksc

This comment has been minimized.

Copy link

samrocksc commented Nov 14, 2017

Verification that this is working.....I've been using it.

@jhawlwut

This comment has been minimized.

Copy link

jhawlwut commented Nov 17, 2017

@nelsonic can we get this merged in ASAP please? :)

@UGLHong

This comment has been minimized.

Copy link

UGLHong commented Nov 18, 2017

HI lets merge? =D

@qraynaud

This comment has been minimized.

Copy link

qraynaud commented Nov 21, 2017

hapi branch 17 got its latest tag on npm today: it is becoming more important to merge this

@alexgreencode

This comment has been minimized.

Copy link

alexgreencode commented Nov 22, 2017

Looking forward to merging!

@alexisohayon

This comment has been minimized.

Copy link

alexisohayon commented Nov 23, 2017

Looking forward too :) <3

@robmcguinness robmcguinness referenced this pull request Nov 26, 2017

Merged

Upgrade Hapi to v17 #481

0 of 3 tasks complete
@jhawlwut

This comment has been minimized.

Copy link

jhawlwut commented Nov 27, 2017

@dwyl/core been sitting for a little bit, can we get this merged please so we have hapi 17 support?

@iteles

This comment has been minimized.

Copy link
Member

iteles commented Nov 28, 2017

@jhawlwut and everyone - our apologies for this, it's coincided with some massive deadlines in our day jobs! We're half way through reviewing it and hope to get through it in the next week!
In the meantime, if anyone else wants to review the code or provide feedback like @samrocksc, always appreciated 💖

@ferrao

This comment has been minimized.

Copy link

ferrao commented Nov 30, 2017

Just run all my tests against salzhrani:v-17 and everything seems to work fine.

README.md Outdated
var server = new Hapi.Server();
server.connection({ port: 8000 });
const init = async () => {
var server = new Hapi.Server({ port: 8000 });

This comment has been minimized.

@sauramirez

sauramirez Dec 1, 2017

Should this be a const now?

README.md Outdated
- `key` - the secret key (or array of keys to try)
- `extraInfo` - (***optional***) any additional information that you would like to use in
`validateFunc` which can be accessed via `request.plugins['hapi-auth-jwt2'].extraInfo`
- `validateFunc` - (***required***) the function which is run once the Token has been decoded with
signature `function(decoded, request, callback)` where:
- `validate` - (***required***) the function which is run once the Token has been decoded with

This comment has been minimized.

@sauramirez

sauramirez Dec 1, 2017

The documentation changed to accept validate but it still uses validateFunc everywhere. We should migrate to use validate like in hapi-auth-basic

README.md Outdated
@@ -529,11 +531,10 @@ JTW was signed using the `JWT_SECRET` (*secret key*).
If you prefer specifying your own verification logic instead of having a `validateFunc`, simply define a `verifyFunc` instead when initializing the plugin.

- `verifyFunc` - (***optional***) the function which is run once the Token has been decoded
(*instead of a `validateFunc`*) with signature `function(decoded, request, callback)` where:
(*instead of a `validateFunc`*) with signature `async function(decoded, request)` where:

This comment has been minimized.

@sauramirez

sauramirez Dec 1, 2017

should we also migrate verifyFunc to just be verify

var extract = require('./extract'); // extract token from Auth Header, URL or Coookie
var pkg = require('../package.json'); // use package name and version rom package.json
var internals = {}; // see: http://hapijs.com/styleguide#module-globals
let Boom = require('boom'); // error handling https://github.com/hapijs/boom

This comment has been minimized.

@sauramirez

sauramirez Dec 1, 2017

Should these be consts according to the hapi styleguide?

@salzhrani

This comment has been minimized.

Copy link
Contributor

salzhrani commented Dec 2, 2017

I also have a branch to use ESlint instead of jshint. will make a PR after this is merged

@puchesjr

This comment has been minimized.

Copy link

puchesjr commented Dec 4, 2017

Hey all, after waiting for this pull request to be merged, I decided to create a new hapi auth package. It was inspired by this project as well as the hapi-auth-bearer-token project. Take a look - hapi-now-auth

@puchesjr
Copy link

puchesjr left a comment

if you're going to use the for loop I would suggest break; once you verify, no need to run through all of the remaining keys. You could also use Array.some() and return true if key is correct, else return false to continue.

I believe the for loop is more performant, but I used Array.some() on my plugin

@jamesdixon

This comment has been minimized.

Copy link

jamesdixon commented Dec 5, 2017

Any update on getting this merged?

}
const init = async() => {
const server = new Hapi.Server({ port: port });
await server.register(hapiAuthJWT);
// see: http://hapijs.com/api#serverauthschemename-scheme
server.auth.strategy('jwt', 'jwt',
{ key: secret, validateFunc: validate,

This comment has been minimized.

@thingthing

thingthing Dec 5, 2017

You call the key validate in the readme but still validateFunc here


// quick check for validity of token format
if (!extract.isValid(token)) {
console.log('bad token', token)

This comment has been minimized.

@thingthing

thingthing Dec 5, 2017

you could remove this console.log i think

@unicodeveloper

This comment has been minimized.

Copy link

unicodeveloper commented Dec 7, 2017

Any update on getting this merged? It's really needed!!!

@unicodeveloper

This comment has been minimized.

Copy link

unicodeveloper commented Dec 7, 2017

@nelsonic please can you merge this PR?

@feugy

This comment has been minimized.

Copy link

feugy commented Dec 9, 2017

While core developers are busy reviewing it, rather than putting pressure on them, consider npm git dependencies.

"hapi-auth-jwt2": "salzhrani/hapi-auth-jwt2#v-17",

Thanks you @salzhrani for your work !

@jamesdixon

This comment has been minimized.

Copy link

jamesdixon commented Dec 16, 2017

@salzhrani or anyone using this, what's the correct way to pass back data once validated?

In the previous version, I was doing the following:

return callback(null, true, JSON.parse(value)); // value was pulled from redis

For this updated version, I'm seeing:

return { valid: true };

However, not sure where that last parameter of the callback would be mapped to in the updated version.

Thanks for your hard work @salzhrani !

@iagomelanias

This comment has been minimized.

Copy link

iagomelanias commented Dec 16, 2017

@jamesdixon you can use the parameter credentials.

Example:

return { isValid: true, credentials: { userId: 1 } };

It's acessible in the request object via request.auth.credentials.userId.

@jamesdixon

This comment has been minimized.

Copy link

jamesdixon commented Dec 16, 2017

@iagomelanias thank you! much appreciated!

@roytz

This comment has been minimized.

Copy link

roytz commented Dec 25, 2017

Hi, is this about to be merged anytime soon? I prefer working with the master instead of a specific branch..
Also, @salzhrani I saw you changed the name of validateFunc to be validate. Are there any other naming changes?

@salzhrani salzhrani force-pushed the salzhrani:v-17 branch from b5c23dd to 15a2ec0 Jan 28, 2018

@optii optii referenced this pull request Feb 1, 2018

Closed

V17 headless #262

@SimonSchick

This comment has been minimized.

Copy link

SimonSchick commented Feb 11, 2018

Hey, since you guys are shipping a typings file (not in the repo), via npm, you might want to start actually vendoring that file.

Typings for hapi 17 were released, the update process for the typings should be trivial.

V17 headless (#1)
* Allow the configuration option 'headerless' for headerless jwt's

* rename headerless to headless
@phenomax

This comment has been minimized.

Copy link

phenomax commented Feb 22, 2018

Thanks for your work!
Do you have any plans for using async/await, instead of callbacks?

@ferrao

This comment has been minimized.

Copy link

ferrao commented Feb 23, 2018

callbacks just seem soooooo v16....

@SimonSchick

This comment has been minimized.

Copy link

SimonSchick commented Feb 24, 2018

Is there any eta on this getting merged/released?
Is there any way I can help?
I'm willing to update all existing typings for this once released and put them on DT.

{ method: 'POST', path: '/try', handler: privado, config: { auth: { mode: 'try', strategy: 'jwt' } } },
{ method: 'POST', path: '/headless', handler: privado, config: { auth: 'jwt-headless' } }
]);
} catch(e) {

This comment has been minimized.

@nelsonic

nelsonic Feb 26, 2018

Member

I don't think there is a need to catch and then throw the error,
but it's fine. as it makes it clearer. 👍


process.on('unhandledRejection', function(reason, p){

This comment has been minimized.

@nelsonic

nelsonic Feb 26, 2018

Member

image
...
😉

@nelsonic
Copy link
Member

nelsonic left a comment

@salzhrani you've done a superb job with this PR! 🎉
We are very grateful! ❤️

Thanks to everyone for trying out the code.

@nelsonic nelsonic merged commit 5817d6b into dwyl:master Feb 26, 2018

4 checks passed

Node Security No known vulnerabilities found
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 91111ce
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@iagomelanias

This comment has been minimized.

Copy link

iagomelanias commented Feb 26, 2018

Finally! 🎉
Thank you for merging, @nelsonic!

@iteles

This comment has been minimized.

Copy link
Member

iteles commented Feb 26, 2018

@salzhrani You're a star, thank you!
@nelsonic, thanks for the reviewing and merge (not just today but over the last couple of months).

Thanks all for trying out the code, helping debug early stages and for the kind words scattered throughout this PR <3 We're getting plans in place for these rewrites to be a little more maintainable in future!

@felipegcampos

This comment has been minimized.

Copy link

felipegcampos commented Feb 26, 2018

It's so good to hear. Thanks for all efforts.

nelsonic added a commit that referenced this pull request Feb 26, 2018

nelsonic added a commit that referenced this pull request Feb 26, 2018

@nelsonic

This comment has been minimized.

Copy link
Member

nelsonic commented Feb 26, 2018

hapi-auth-jwt2@8.0.0 is now on NPM. thanks everyone! 🎉

@clarkie

This comment has been minimized.

Copy link

clarkie commented Feb 27, 2018

Thanks all 👍

@unicodeveloper

This comment has been minimized.

Copy link

unicodeveloper commented Feb 27, 2018

@aestrro

This comment has been minimized.

Copy link

aestrro commented Mar 16, 2018

Yay

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