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.

Sign up for GitHub

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

Jump to bottom

Refresh token #30

Closed
wants to merge 12 commits into from
Closed

Refresh token #30

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
85f4e27
added dotenv
guanacone Nov 5, 2020
99b2e86
backend implementation of refresh token on login.
guanacone Nov 5, 2020
36ac05e
updated gatsby
guanacone Nov 5, 2020
81f5496
added dotenv
guanacone Nov 5, 2020
b3a112e
backend implementation of refresh token on login.
guanacone Nov 5, 2020
03aac25
Merge branch 'refresh-token' of github.com:guanacone/fullstack_app in…
guanacone Nov 5, 2020
506a31b
updated node
guanacone Nov 5, 2020
cbf9409
downgraded gatsby-plugin-nodejs
guanacone Nov 5, 2020
b5a40d2
Update node.js.yml
guanacone Nov 6, 2020
3182d83
Update node.js.yml
guanacone Nov 6, 2020
80d0ea7
Update node.js.yml
guanacone Nov 6, 2020
0900c5f
Update node.js.yml
guanacone Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
v12.18.3
v14.15.0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice this looks like the new LTS for node but now you should also make sure the Dockerfile and github workflows are updated to also use this node version

you could update your github actions to automatically use the value in the nvmrc

or it looks like nvm was added to the github action virtual environments so you could also just update your github actions to use nvm directly

5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,12 @@
"bcrypt": "5.0.0",
"bootstrap": "4.5.2",
"cors": "2.8.5",
"dotenv": "8.2.0",
"express": "4.17.1",
"express-async-handler": "1.1.4",
"gatsby": "2.24.52",
"gatsby": "2.25.2",
"gatsby-plugin-create-client-paths": "2.3.11",
"gatsby-plugin-nodejs": "0.7.0",
"gatsby-plugin-nodejs": "0.6.4",
Comment on lines -26 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the build issue you were seeing was caused by other issues and these downgrades can probably be reverted

"gatsby-plugin-styled-components": "3.3.12",
"http-errors": "1.8.0",
"jsonwebtoken": "8.5.1",
Expand Down
5 changes: 4 additions & 1 deletion server/auth/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ const localStrategy = require('passport-local').Strategy;
const User = require('../models/user');
const JWTstrategy = require('passport-jwt').Strategy;
const ExtractJWT = require('passport-jwt').ExtractJwt;
const dotenv = require('dotenv');

dotenv.config({ path: '.env' });
guanacone marked this conversation as resolved.
Show resolved Hide resolved

passport.use(
'login',
Expand Down Expand Up @@ -35,7 +38,7 @@ passport.use(
passport.use(
new JWTstrategy(
{
secretOrKey: 'TOP_SECRET',
secretOrKey: process.env.TOKEN_SECRET,
guanacone marked this conversation as resolved.
Show resolved Hide resolved
jwtFromRequest: ExtractJWT.fromAuthHeaderAsBearerToken(),
},
async (token, done) => {
Expand Down
24 changes: 23 additions & 1 deletion server/controllers/userController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ const User = require('../models/user');
const passport = require('passport');
const jwt = require('jsonwebtoken');
const createError = require('http-errors');
const dotenv = require('dotenv');

dotenv.config({ path: '.env' });

const checkMongoError = (ex) => {
if (ex.name === 'ValidationError') {
Expand Down Expand Up @@ -104,7 +107,26 @@ exports.loginUser = async (req, res, next) => {
if (err) return next(err);

const body = { _id: user._id, email: user.email };
const token = jwt.sign({ user: body }, 'TOP_SECRET');
const token = jwt.sign(
{ user: body },
process.env.TOKEN_SECRET,
{
expiresIn: 120,
});

const refreshToken = jwt.sign(
{ user: body },
process.env.REFRESH_TOKEN_SECRET,
);
Comment on lines +117 to +120
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this refresh token should probably have some expiration like 24 hours or something


await User.findByIdAndUpdate(
user._id,
{ $set:
{ refreshToken },
},
{ useFindAndModify: false,
new: true },
);

return res.json({ token, info });
Copy link

@edwmurph edwmurph Nov 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that when a user logs in a refresh token is saved to the DB and deleted when he logs out. On the front end if a response comes back with a status code 404 and if there's a expired token it will retrieve the refresh token from the DB and if there's a refresh token hit a refresh route to get a new token.

it might help to get a complete picture to update the logout route to delete the refresh token in this iteration?

also it looks like this implementation doesnt pass the refresh token back to the user but i read a few articles on this and it looks like they all suggest passing the refresh token back to the user. and then when the user needs to get a fresh token, it passes that refresh token in the request. otherwise anyone could get a refreshed token without auth?

also it would help if you also implemented the route for refreshing the token in this iteration (assuming you're implementing refresh in that way) so we can make sure refreshing a token automatically works. it's probably good tho to ya at least defer the frontend implementation for this to a separate PR

},
Expand Down