[FIX] Error when user roles is missing or is invalid #9040
Conversation
paulovitin
changed the title
server/lib/accounts.js
Outdated
| @@ -168,6 +168,12 @@ Accounts.validateLoginAttempt(function(login) { | |||
| }); | |||
| } | |||
|
|
|||
| if (!{}.hasOwnProperty.call(login.user, 'roles') || !Array.isArray(login.user.roles)) { | |||
There was a problem hiding this comment.
I'm not sure about others, but I find !{}.hasOwnProperty.call to be a tad confusing at first glance. Can we change this to something like if (login.user && (!login.user.roles || Array.isArray(login.user.roles)) {}?
There was a problem hiding this comment.
Sorry about that, I like to follow no-prototype-builtins.
But, in this code I needs to check the presence of property, !login.user.roles can be true if roles is false.
I know the value needs to be an array, but (!login.user.roles) is not better approach to check presence of property.
There was a problem hiding this comment.
Because if you look through the rest of our code base, you will see the style is how I mentioned. So, it's more about being consistent than anything to be honest.
@RocketChat/core can correct me if I am wrong though.
There was a problem hiding this comment.
Maybe we should look at making part of our style guide if this is a more proper way of checking for the property?
But I do agree on we need to stay consistent, I don't think we use that elsewhere. Not to say we shouldn't... just we don't so does make it stick out.
Even a comment with a note about the style guide would help a bit in regards to readability here.
All for better code.
There was a problem hiding this comment.
I agree about the consistency of style. In this particular piece of code, even the user.roles is undef or null or false, we have the second criteria to check if is array, so this dont cause any bug.
But is good idea too make the things in most safety away. Is a good idea add this in style guide in future.
There was a problem hiding this comment.
I believe we have an issue discussing code style #1163 this would be good to add to discussion as something to include
There was a problem hiding this comment.
Sure @geekgonecrazy :)
Adds: - Check if roles is present in user and is valid array; - i18n messages;
rodrigok
changed the title
[FIX] Error when user roles is missing or is invalid
@RocketChat/core
Closes #8958
Adds: