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.

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

define users table and authorization logic #7

Merged
merged 3 commits into from
Mar 18, 2021
Merged

Conversation

vaniri
Copy link
Owner

@vaniri vaniri commented Mar 7, 2021

Working on task #3.

I tested most of this with some of the work on the frontend I was doing in parallel, so there is some confidence in it working! :)

Comment on lines +25 to +29
image: {
type: DataTypes.STRING,
isUrl: true,
notNull: true
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

for this project we won't really worry about image uploads so you don't have to worry about this

server/app.js Outdated
Comment on lines 17 to 22
app.use(function (req, res, next) {
res.header("Access-Control-Allow-Origin", "*");
res.header("Access-Control-Allow-Headers", "Origin, X-Requested-With, Content-Type, Accept, Authorization");
res.header("Access-Control-Allow-Methods", "GET, OPTIONS, POST, DELETE, PATCH");
next();
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

these shouldn't be necessary the way we have the server/client set up

Comment on lines 14 to 15
let result = await db.User.create(req.body);
let userId = result.id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

these can be const

req.body.password = await argon2.hash(req.body.password);
let result = await db.User.create(req.body);
let userId = result.id;
res.status(201).json({ result, userId, token: generateToken(userId) });
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice attention to detail with the http code

req.body.password = await argon2.hash(req.body.password);
let result = await db.User.create(req.body);
let userId = result.id;
res.status(201).json({ result, userId, token: generateToken(userId) });
Copy link
Collaborator

Choose a reason for hiding this comment

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

We prefer using httpOnly cookies.
Nice job storing the userId in the token

Comment on lines 40 to 49
if (!userRecord) {
res.status(404).send({ "message": "User not found" });
return;
}

let correctPassword = await argon2.verify(userRecord.password, req.body.password);
if (!correctPassword) {
res.status(401).send({ "message": "Invalid password" });
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

usually we can combine these two checks to just send a 400 error with a generic message of Invalid Credentials or something similar. The reason is that in case of malicious activity, we don't want to let the attacker know if the problem is the user not existing or the password being incorrect

try {
console.log(req.body);
let userRecord = await db.User.findOne({ where: { email: req.body.email } });
console.log(userRecord);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove unnecessary console.logs from code that is going to be merged into dev

@@ -0,0 +1,9 @@
const jwt = require('jsonwebtoken');

const jwtSecret = "this is my very important secret";
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be in your .env file which is gitignored

@@ -0,0 +1,6 @@
const { Sequelize } = require('sequelize');
const sequelize = new Sequelize(
process.env.JAWSDB_URL || 'postgresql://messenger_user:messenger_password@localhost:5432/messenger_db'
Copy link
Collaborator

Choose a reason for hiding this comment

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

best to remove your personal details :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks, I'll move that to my .env file that I didn't have in the beginning!

res.status(409);
res.send({ "message": "User already exists" });
} else {
res.status(500);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what error would this be?

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's any error we don't expect here (we only expect unique constraint violations), so we send internal server error code.

@vaniri vaniri merged commit a819b66 into master Mar 18, 2021
@vaniri vaniri deleted the users_db_and_routes branch March 18, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants