-
Notifications
You must be signed in to change notification settings - Fork 118
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
Fix reserved words handling in putUser #120
Conversation
Before, this would check the parent `opts` object, not `metadata` properties.
`metadata` is not a reserved word in a CouchDB user document. Also, CouchDB only uses `_id` and `_rev` as underscore-prefixed reserved words. Therefore, explicitly list them and remove the `startsWith` condition in case the user wants to set others.
ec02871
to
96c4bc0
Compare
if (reservedWords.indexOf(key) !== -1 || key.startsWith('_')) { | ||
return callback(new AuthError('cannot use reserved word in metadata: "' + key + '"')); | ||
} | ||
if (reservedWords.indexOf(key) !== -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hasOwnProperty
check is not optional; it's standard for these kind of JS for loops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nolanlawson @tlvince We only have to keep the hasOwnProperty
test if it makes sense to have an opts
object that is an instance of a class in a class hierarchy. I guess for options to a function this does never occur.
However, if we choose to keep it, it has to be tested on opts.metadata
and not on opts
directly, right ?
Currently, this only check for reserved words on properties that are defined both on opts
and opts.metadata
.
TBQH I have no idea what this code was doing, either. Can you add a new test or something to confirm the behavior? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Maybe adding more tests for the reserved words would be a good thing.
if (reservedWords.indexOf(key) !== -1 || key.startsWith('_')) { | ||
return callback(new AuthError('cannot use reserved word in metadata: "' + key + '"')); | ||
} | ||
if (reservedWords.indexOf(key) !== -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nolanlawson @tlvince We only have to keep the hasOwnProperty
test if it makes sense to have an opts
object that is an instance of a class in a class hierarchy. I guess for options to a function this does never occur.
However, if we choose to keep it, it has to be tested on opts.metadata
and not on opts
directly, right ?
Currently, this only check for reserved words on properties that are defined both on opts
and opts.metadata
.
@tlvince Do you mind if I take over your pull request ? |
Please do. |
I'm a little confused by the way this was previously being handled, but it seems to me what this is supposed to do is prevent reserved words from being set in
opts.metadata
, notopts
.If that's the case,
metadata
is not a reserved word in a CouchDB user document. Also, CouchDB only uses_id
and_rev
as underscore-prefixed reserved words. Therefore, explicitly list them and remove thestartsWith
condition in case the user wants to set others.