Open NachoJusticia opened 6 years ago
Hello @NachoJusticia
Let's first discuss about the inclusion of the JWT code. I would prefer to have them in separate files, not only because it will be more tidy and organized on the generated project, but also because it's easier to manage by the generator code. If most or all of the logic lives on separate files, all you have to do is to include or skip them based on a bolean. You have an example on the DockerFile inclusion:
As you can see, if the user wants docker we copy the dockerfile, if the user does not want docker we do nothing. You can do the same for JWT if it lives on a separate plugin, which for me is also the preferred way to include stuff in a hapi server.
About the code you have posted.... Why are you returning a promise for the registration ? Does hapi accept a promise instead of a callback ? Do you have any link to the documentation ?
Thanks and regards
Está bien que lo hayas puesto en inglés, esto es un proyecto open-source y así más gente puede entenderlo y participar. Un saludo
Hi again @danielo515 and thank you for your answer.
Regarding the JWT code, I thought in put this code in separate files as I showed you in my previous comment. But I think that even doing this I don't get how to achieve the problem that I have to include this plugin to the hapi server object and I guess I should do it in this file.
I am returning a promise because I load a plugins module like this
LoadPlugins(server).then(() => {
...
}
This works fine.
Hello @NachoJusticia
LoadPlugins(server).then(() => { ... } This works fine.
That is not how we register plugins on the generated project, we use glue. I'm not saying that your approach is good or bad, but I want to be consistent on the style, and I want to register all the plugins through glue.
You are right about where to include the plugin registration, just add a conditional registration on the start.js file Something like this
registrations: [
....
<% if(useJwt){ %>
{
plugin: './api/login'
},
{
plugin: './api/logout'
},
{
plugin: './yourJWTStuff'
},
<% } %>
....
]
Ok @danielo515 , then I will try to be consistent with the plugin registration like you suggest, I imagine this won't be a problem for me. I understand now how to add the new plugin by condition thanks to your example above.
Thank you!
Hi @danielo515 I think I have done a lot of progress with the JWT authentication. You can see my changes in my fork
After fixing some bugs that my code had, I am able to launch the server and get no errors. The problem is that I am able to see only 1 endpoint in the hapi-swagger documentation site (the vision endpoint). Maybe it is a problem related with Glue...
Then, I would like you to have a look at my code and help me please! 👍
I'm registering the plugins like this:
registrations: [
...
{
plugin: './api/version'
},
{
plugin: './api/healthcheck'
},
<% if(useAuthentication) { %>{
plugin: './api/users/register.POST'
},
{
plugin: './api/users/login.POST'
},
{
plugin: './../authentication'
}<% } %>
]
In addition, I have created a simple DAO module and I decorate the server when init:
Server.init(internals.manifest, internals.composeOptions, (err, server) => {
Hoek.assert(!err, err);
const URL = 'mongodb://localhost:27017';
Mongo.connect(URL)
.then((db) => {
const _DAO = DAO(db);
server.decorate('request', 'DAO', _DAO);
server.log(process.env.npm_package_name + ' v' + process.env.npm_package_version + ' started at: ' + server.info.uri);
})
.catch((error) => {
if (error) {
throw (error);
}
});
});
Well I think it's not a good idea to put more parts of my code here, it should be easier for you to have a look at my code directly.
Thank!
Just one question:
why do you put return next()
in version plugin but in the healthcheck you do just next()
?
The same happends with the reply of the handler.
healthcheck.js
'use strict';
exports.register = function (server, options, next) {
server.route({
path: '/ops/healthcheck',
method: 'GET',
handler(request, reply) {
reply({ message: 'ok' });
}
});
next();
};
exports.register.attributes = {
name: 'healthcheck'
};
version.js
'use strict';
const Joi = require('joi');
const internals = {
response: {
version: process.env.npm_package_version
}
};
exports.register = (server, options, next) => {
server.route({
method: 'GET',
path: '/ops/version',
config: {
description: 'Returns the version of the server',
notes: 'Based on the package version',
tags: ['meta', 'ops', 'api'],
response: {
status: {
200: Joi.object()
}
},
handler(request, reply) {
return reply(internals.response);
},
}
});
return next();
};
exports.register.attributes = {
name: 'version'
};
Ok I have just noticed that I am missing this line:
tags: ['meta', 'users', 'api'],
in the plugins that are not available in the /documentation
site. I can continue working now!
I have added 3 endpoints:
In the hapi-auth-jwt2 module they put a URL to a issue that they have with glue... https://www.npmjs.com/package/hapi-auth-jwt2
Then I have done what a user propose there, and it seems it works fine with glue now. Anyhow, the users/me endpoint is not working because it doesn't allow me to use authentication header. In this endpoint, I am specifying that I want to use JWT authentication like this:
server.route({
method: 'GET',
path: '/users/me',
config: {
auth: 'jwt',
description: 'Get the current user information details - [ Authentication required ]',
tags: ['meta', 'users', 'api'],
handler(request, reply) {
return reply(request.auth.credentials);
}
}
});
In the following image you can see that the endpoint /users/me doesn't allow to send the Authorization header, that is my problem...
Now I post you an image of a similar backend (without Glue) that I have and in this case the /users/me endpoint works fine:
As you can see when an user logins he receive a token in the Authorization header, but I have also included it in the object response because this way the developer will notice easier that he can handle this token. Anyhow you can tell me if putting the token just in the Authentication header is enough.
Do you think I am missing something important here? Thanks again.
Hello @NachoJusticia
Thanks for your efforts. I have been checking your work and there are a couple of things that I want to check with you before we go any further. Most of them are style things.
lf
instead of crlf
linebreaks. Probably this is my fault because I have not configured it on the .editorconfig
file, but this is importantl. Please configure your VSCode to use lf
for line-breaks. You can fix this problem on the affected files just running dos2unix filename
or dos2unix generators/**
<% if(useAuthentication){ -%>"hapi-auth-jwt2": "^7.3.0",
<%
} %>
Why is that split on three lines when it can fit on just one ? And, why are you duplicating that for both hapi-auth-jwt2 and jsonwebtoken ? EJS is weird and annoying enough, try to use as less of it's syntax as possible.
{
"server": {
"port": <%- service.port %><% if(useAuthentication){ -%>,
"authKey": "H5SA7hdVmtwT0lpemh2KN7GGln1oOU99"<%} %>
}
}
What's the reasoning behind that ? Why are you merging two lines that have nothing to do with each other ? Put the useAuthentication
on it's own line
There are several other problems. Please branch your work, and go ahead with a pull request, review will be far easier there.
Thanks and regards
Hi @danielo515
<% if(useAuthentication){ -%>"hapi-auth-jwt2": "^7.3.0",
<%
} %>
is
"hapi-auth-jwt2": "^7.3.0",
in line with the rest of the dependencies.
Anyhow you are right, I am checking the value of useAuthentication twice. Then I am going to write it as:
<% if(useAuthentication){ -%>"hapi-auth-jwt2": "^7.3.0",
"jsonwebtoken": "^7.1.9",
<%
} %>
3.2. The reasoning behind that
{
"server": {
"port": <%- service.port %><% if(useAuthentication){ -%>,
"authKey": "H5SA7hdVmtwT0lpemh2KN7GGln1oOU99"<%} %>
}
}
is that I only want to get
"server": {
"port": 3333
}
}
when the user doesn't want authentication. Therefore, I put the , inside the if(useAuthentication). Please launch my project in order to see if I am wrong with the things I am saying, because I don't think so.
Anyhow I am going to write here the result of some of the files when I select that I want authentication: default.json:
{
"server": {
"port": 3000,
"authKey": "H5SA7hdVmtwT0lpemh2KN7GGln1oOU99"
}
}
package.json --> just the dependencies:
"dependencies": {
"boom": "^4.3.1",
"chalk": "^2.1.0",
"getconfig": "^3.1.0",
"glue": "^4.2.0",
"good": "^7.3.0",
"good-console": "^6.4.0",
"good-squeeze": "^5.0.2",
"hapi": "^16.6.2",
"hapi-auth-jwt2": "^7.3.0",
"jsonwebtoken": "^7.1.9",
"hapi-swagger": "^7.8.1",
"hoek": "5.0.0",
"inert": "^4.2.1",
"joi": "^11.1.1",
"lout": "^10.0.3",
"mongodb": "^2.2.31",
"vision": "^4.1.1"
},
Hello again @NachoJusticia
We should prefer template maintainability over beautiful code output. The main reason is that it can become very hard to maintain and read certain template structures while editing the final code can be trivial, and probably some linter will take care of it. That said, you are missing one EJS feature: slurp
So the following
<% if(useAuthentication){ -%>"hapi-auth-jwt2": "^7.3.0",
"jsonwebtoken": "^7.1.9",
<%
} %>
can be written as
<% if(useAuthentication){ -%>
"hapi-auth-jwt2": "^7.3.0",
"jsonwebtoken": "^7.1.9",
<% } -%>
And no difference on the output should happen. Have you tried the output of your templates before changing them ?
We should also agree on some EJS style-guides. For example, I always prefer <% } -%>
instead of breaking it on several lines.
Again, thanks for your time and keep up the good work.
Hi @danielo515, I think it's a good idea to prefer tamplate maintainability too, so I have just made the changes like you suggested me.
Tell me if there anything else you want to change. I would like also you help me with the me endpoint, I am not able to use the Authorization header at this point. After searching this problem I think it could be a problem related to glue.
As I said @NachoJusticia go ahead with the PR so we can better work together on the code.
Thanks and regards
@danielo515 I opened the PR 10 days ago
Sorry @NachoJusticia I didn't noticed
Then, I think we can close this issue and continue in the PR, do you agree?
Hi, I would like to add JWT authentication because I think it would very interesting for this hapijs generator. I have a hapijs backend in which I use the module hapi-auth-jwt2 and it is a plugin for the server object. I have been able to:
The code I would like to add is something like this:
Maybe I can put the plugin registration in an independent module like I show above, but another solution could be add it directly here:
default, development and test JSON files:
Could you please give me some guides about this problem? Thanks :)
Perdona por el inglés, no sé porqué cuando me dí cuenta ya llevaba más de medio Pull Request 👍