Closed guanacone closed 3 years ago
We have been hit by a second mayor hurricane, cat 5 this time, on Tuesday. The resort got flooded by about 1 foot. We are fine but communities around us got flooded up to 8 feet and are pretty much destroyed... Won't probably code for another week or so.
Le mar. 17 nov. 2020 à 06:08, Edward Murphy notifications@github.com a écrit :
@edwmurph approved this pull request.
great job, left a few comments that i think would help but this all works so it's fine if you want to move forward
In server/routes/user.js https://github.com/guanacone/fullstack_app/pull/33#discussion_r525080514 :
router.post('/login',asyncHandler(userController.loginUser)); +router.post('/logout', asyncHandler(userController.logoutUser));
this should probably also use the passportAuthRefreshToken. i didn't realize this use case but because of this, it probably makes more sense to check if the token is blacklisted in the controller if necessary. eg. /logout doesnt need to check if the token is blacklisted but /refresh does need to check that
In server/controllers/userController.js https://github.com/guanacone/fullstack_app/pull/33#discussion_r525082701 :
- .save();
- return res
- .status(201)
- .json({ message: 'logged out' });
- }catch(err) {
- checkMongoError(err);
- throw err;
- } +};
+// refresh access token +exports.refreshUser = (req, res) => {
- const { authorization } = req.headers;
- const bearer = authorization.split(' ');
- const { user } = jwt.decode(bearer[1]);
- const body = { _id: user._id, email: user.email };
you dont need to decode the token again here because the auth middleware before this did that and put it in req.user. also same for the logout method above
In server/controllers/userController.js https://github.com/guanacone/fullstack_app/pull/33#discussion_r525103246 :
@@ -104,11 +108,46 @@ 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 accessToken = jwt.sign({ user: body }, process.env.TOKEN_SECRET, { expiresIn: 120 });
120 is good for testing but is too short in practice. i'd recommend increasing this to 1 hour before merging to master
In server/controllers/userController.js https://github.com/guanacone/fullstack_app/pull/33#discussion_r525104482 :
- return res
- .status(201)
- .json({ message: 'logged out' });
- }catch(err) {
- checkMongoError(err);
- throw err;
- } +};
+// refresh access token +exports.refreshUser = (req, res) => {
- const { authorization } = req.headers;
- const bearer = authorization.split(' ');
- const { user } = jwt.decode(bearer[1]);
- const body = { _id: user._id, email: user.email };
- const accessToken = jwt.sign({ user: body }, process.env.TOKEN_SECRET, { expiresIn: 120 });
might help to have a shared util somewhere for generating access tokens since it's done in a couple places
In server/models/blacklist.js https://github.com/guanacone/fullstack_app/pull/33#discussion_r525080683 :
@@ -0,0 +1,13 @@ +const mongoose = require('mongoose'); +const { Schema } = mongoose; + +const BlacklistSchema = new Schema(
- {
- refreshToken: { type: String, required: true },
- expireAt: { type: Date, required: true },
- }, +);
+BlacklistSchema.index({ expireAt: 1 }, { expireAfterSeconds : 0 });
this looks right, but ya the syntax isnt super intuitive. it's a mongo thing https://docs.mongodb.com/manual/tutorial/expire-data/#expire-documents-at-a-specific-clock-time
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/guanacone/fullstack_app/pull/33#pullrequestreview-532260211, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACZDGDP2AK7VEBGGKTF2D6DSQJRSZANCNFSM4TXS43QA .
oh god sorry that's awful. 2020 has been the worst year
There are issues with the frontend part that will be addressed in a separate PR. I tested everything successfully in Postman.