Skip to content

Commit

Permalink
fix(users): security review, filter user data return by default 🐛
Browse files Browse the repository at this point in the history
  • Loading branch information
PierreBrisorgueil committed May 1, 2020
1 parent fed473e commit af800d1
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 13 deletions.
4 changes: 2 additions & 2 deletions config/defaults/development.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ module.exports = {
},
whitelists: {
users: {
default: ['_id', 'id', 'firstName', 'lastName', 'displayName', 'email', 'roles', 'avatar', 'resetPasswordToken', 'resetPasswordExpires'],
default: ['_id', 'id', 'firstName', 'lastName', 'displayName', 'email', 'roles', 'avatar', 'provider', 'updatedAt', 'createdAt', 'resetPasswordToken', 'resetPasswordExpires'],
update: ['firstName', 'lastName', 'email', 'avatar'],
updateAdmin: ['firstName', 'lastName', 'email', 'avatar', 'roles'],
recover: ['password', 'resetPasswordToken', 'resetPasswordExpires'],
Expand All @@ -108,7 +108,7 @@ module.exports = {
},
// jwt is for token authentification
jwt: {
secret: 'test', // secret for hash
secret: 'WaosSecretKeyExampleToChnageAbsolutely', // secret for hash
expiresIn: 7 * 24 * 60 * 60, // token expire in x sec
},
mailer: {
Expand Down
2 changes: 1 addition & 1 deletion modules/users/config/strategies/jwt.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const cookieExtractor = (req) => {

async function verifyCallback(jwtPayload, done) {
try {
const user = await UserService.get({ id: jwtPayload.userId });
const user = await UserService.getBrut({ id: jwtPayload.userId });
if (user) return done(null, user);

return done(null, false);
Expand Down
2 changes: 1 addition & 1 deletion modules/users/controllers/admin.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ exports.delete = async (req, res) => {
*/
exports.userByID = async (req, res, next, id) => {
try {
const user = await UserService.get({ id });
const user = await UserService.getBrut({ id });
if (!user) responses.error(res, 404, 'Not Found', 'No User with that identifier has been found')();
else {
req.model = user;
Expand Down
4 changes: 2 additions & 2 deletions modules/users/controllers/users/users.password.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ exports.forgot = async (req, res) => {

// get user generate and add token
try {
user = await UserService.get({ email: req.body.email });
user = await UserService.getBrut({ email: req.body.email });
if (!user) return responses.error(res, 400, 'Bad Request', 'No account with that email has been found')();
if (user.provider !== 'local') return responses.error(res, 400, 'Bad Request', `It seems like you signed up using your ${user.provider} account`)();

Expand Down Expand Up @@ -136,7 +136,7 @@ exports.updatePassword = async (req, res) => {

// get user, check password, update user, login again
try {
user = await UserService.get({ id: req.user.id });
user = await UserService.getBrut({ id: req.user.id });
if (!user) return responses.error(res, 400, 'Bad Request', 'User is not found')();

if (!await UserService.comparePassword(req.body.currentPassword, user.password)) return responses.error(res, 422, 'Unprocessable Entity', 'Current password is incorrect')();
Expand Down
25 changes: 18 additions & 7 deletions modules/users/services/user.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ const saltRounds = 10;
* @param {Object} user
* @return {Object} user
*/
const removeSensitive = (user) => {
const removeSensitive = (user, conf) => {
if (!user || typeof user !== 'object') return null;
return _.assignIn(user, _.pick(user, config.whitelists.users.default));
const keys = conf || config.whitelists.users.default;
return _.pick(user, keys);
};

/**
Expand All @@ -29,7 +30,7 @@ const removeSensitive = (user) => {
*/
exports.list = async () => {
const result = await UserRepository.list();
return Promise.resolve(result);
return Promise.resolve(result.map((user) => removeSensitive(user)));
};

/**
Expand Down Expand Up @@ -65,14 +66,24 @@ exports.get = async (user) => {
return Promise.resolve(removeSensitive(result));
};

/**
* @desc Function to ask repository to get a user by id or email without filter data return (test & intern usage)
* @param {Object} user.id / user.email
* @return {Object} user
*/
exports.getBrut = async (user) => {
const result = await UserRepository.get(user);
return Promise.resolve(result);
};

/**
* @desc Function to ask repository to search users by request
* @param {Object} mongoose input request
* @return {Array} users
*/
exports.search = async (input) => {
const result = await UserRepository.search(input);
return Promise.resolve(removeSensitive(result));
return Promise.resolve(result.map((user) => removeSensitive(user)));
};

/**
Expand All @@ -83,9 +94,9 @@ exports.search = async (input) => {
* @return {Promise} user -
*/
exports.update = async (user, body, option) => {
if (!option) user = _.assignIn(user, _.pick(body, config.whitelists.users.update));
else if (option === 'admin') user = _.assignIn(user, _.pick(body, config.whitelists.users.updateAdmin));
else if (option === 'recover') user = _.assignIn(user, _.pick(body, config.whitelists.users.recover));
if (!option) user = _.assignIn(user, removeSensitive(body, config.whitelists.users.update));
else if (option === 'admin') user = _.assignIn(user, removeSensitive(body, config.whitelists.users.updateAdmin));
else if (option === 'recover') user = _.assignIn(user, removeSensitive(body, config.whitelists.users.recover));

user.updated = Date.now();

Expand Down

0 comments on commit af800d1

Please sign in to comment.