JWT #8
JWT #8
Conversation
|
|
Timeout is because app has no DB to connect to when running on Travis. The test code is just waiting until a DB connection has been made. |
vipulrawat
force-pushed
the
JWT
branch
2 times, most recently
from
a0c4059 to
7db9817
Compare
packages/api/src/services/mail.js
Outdated
| @@ -0,0 +1,17 @@ | |||
| const sgMail = require('@sendgrid/mail'); | |||
|
|
|||
| sgMail.setApiKey('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'); | |||
There was a problem hiding this comment.
should this be in the environment?
There was a problem hiding this comment.
No.
I'll put this in Travis's env. variables.
There was a problem hiding this comment.
So it should not be a plain string here, right?
There was a problem hiding this comment.
Yes. It shouldn't be.
packages/api/src/services/token.js
Outdated
|
|
||
| function generate(payload, expiresIn = '1d') { | ||
| return new Promise((resolve, reject) => { | ||
| jwt.sign(payload, process.env.JWT_SECRET, { expiresIn }, (err, token) => { |
There was a problem hiding this comment.
Get the secret in a config file instead of calling process.env multiple times. Though, it may not be that bad in simple application. However, this routing will be called multiple times and since we don't expect the SECRET to change, we can cache it in the application layer.
Read this.
|
@ArfatSalman It's done. You can check it now. |
packages/api/config/ci.js
Outdated
| PORT: 5000, | ||
| EMAIL_VERIFICATION_TOKEN_EXPIRY: 15 * 60, // 15 minutes | ||
| LOGIN_TOKEN_EXPIRY: '28d', | ||
| LOGIN_COOKIE_EXPIRY: 4 * 7 * 24 * 60 * 60 * 1000, // 4 weeks |
There was a problem hiding this comment.
Ok. I see that you have ci.js, dev.js and prod.js with repeated key-val pairs.
I think you can do this -
const commonConfig = { ... };
const ci = {
...commonConfig,
EMAIL_VERIFICATION_TOKEN_EXPIRY: over-ridden valueIt's not good for a single value to live in 3 files.
What do you say?
There was a problem hiding this comment.
That's true. Modified the code now.
packages/api/__tests__/index.js
Outdated
|
|
||
| beforeAll((done) => { | ||
| jest.setTimeout(10000); | ||
| mongoose.connect(process.env.DB_URL, { useNewUrlParser: true }, () => { |
There was a problem hiding this comment.
You can use your config files here too for importing constants.
packages/api/src/index.js
Outdated
| // In other cases, such as integration tests, we want to start the server elsewhere | ||
| // so we can stop it when the tests are done | ||
| if (!module.parent) { | ||
| if (!PORT) throw new Error('port not specified'); |
There was a problem hiding this comment.
I see that you have a file called ERR_MSGS.
Can you put this error in there too?
There was a problem hiding this comment.
Yes. Added new errors into ERR_MSGS file
packages/api/src/routes/index.js
Outdated
| }); | ||
| }) | ||
| .catch(() => { | ||
| // TODO: error logging |
packages/api/src/services/token.js
Outdated
| function decode(token) { | ||
| return new Promise((resolve, reject) => { | ||
| jwt.verify(token, JWT_SECRET, (err, decoded) => { | ||
| if (err) reject(err); |
|
@vipulrawat Please see a new set of changes required. |
OK |
packages/api/config/ci.js
Outdated
|
|
||
| module.exports = { | ||
| ...commonConfig, | ||
| ...process.env, |
There was a problem hiding this comment.
Can you put the ...process.env in the commnConfig too? Since it'll already override thecommonConfig so we don't need to spread process.env thrice in 3 files.
What do you say?
No description provided.