Fix(users): Don't update secure profile fields

[shanavas786]

Avoid updating secure fields as password, salt ..etc through
user profile update.

Fixes #1420

[mleanos]

The naming here is a little confusing. It sounds like these are the fields that are not allowed.

[shanavas786]

Yeas :). whitelist would be far better.

[mleanos]

Thank you for taking the initiative & submitting this PR. And for the added test :)

I'm a little unsure of this solution though. I think a better solution would be to maintain two lists at the top level of this server controller.

1. Whitelist - Fields that can be updated and/or be sent back to the client.
2. Blacklist - Fields that must be hidden from the clients and/or cannot be updated.

We can keep the line with user = _.extend(user, req.body);, and then just iterate through the Blacklist & remove those fields from the User object before we delete. Or just use the Whitelist with your proposed solution. I'm leaning toward the former, but I'd like to know others' opinions.

WDYT?

[shanavas786]

@mleanos I took the black list approach because if request contains some invalid fields like invalidkey:"invalidvalue", extend will assign that also to user object.

x = { a: "a" };
_.extend(x, { b: "b" };

makes x { a: "a", b: "b" }).
But I could not observe such alteration in user object. I guess mongoose might be handling it (any clarification is appreciated :)).

[mleanos]

What do you think about having a function(s) defined in the server controller, that "cleans" the user object coming in from req.body? It could return an object that only includes the white-listed field.

You may end up with something like this for when we receive a user object from the request:

// only extend with fields we want to accept from the request
user = _.extend(user, whitelistedUser(req.body));

EDIT: whitelistedFields() might be a better name for the function?

and for returning to the client:

// only send fields that are safe to be sent back to the client
res.json(sanitizedUser(user));

Thoughts on this approach?

My understanding is that Mongoose won't update fields that aren't present in the model definition. Unless, it's an object field (dynamic?) with no strict definitions.

[shanavas786]

That looks better.
I'm a little stretched out now. Will update accordingly when I get some spare time.

[mleanos]

I think this is good now. Thanks.

@meanjs/contributors Any feedback?

[lirantal]

Looks good, thanks @mleanos and @shanavas786