Issue/59/put me#64
Conversation
|
Also unit test. |
kparkov
left a comment
kparkov
left a comment
There was a problem hiding this comment.
All unit tests are failing...
|
Unit tests needs to be fixed. |
… the issue. Still WIP.
If I am admin@users, I can post a user with any privilege, thereby creating a security hole which allows me to gain access to everything - as long as I can create users administratively. The error can most likely be attributed to commenting out a central piece of middleware: https://github.com/bitkompagniet/mugs/pull/64/files#diff-457f38d417a80e0636ff11bb65028830L12
kparkov
left a comment
kparkov
left a comment
There was a problem hiding this comment.
Please make edits to ensure the integrity of the security model.
| requireUserAdmin(), | ||
| ensureFirstLastname(), | ||
| validateBodyRoleAssignments(), | ||
| // validateBodyRoleAssignments(), |
There was a problem hiding this comment.
Be very careful when commenting out middleware inclusions. Was it just for testing purposes, or did you consider it obsolete? I have added a test that demonstrates a severe security breach when posting users administratively. Please update and run the tests.
|
I can't really wrap my head around this error. Lets look at it next time |
|
When you have the time, please review the newest changes @kparkov. |
| filteredRoles = req.body.roles.filter((role) => { | ||
| if (typeof role === 'object') { | ||
| if (!role.scope && role.role) { | ||
| return null; |
There was a problem hiding this comment.
.filters callback skal returnere en boolean. Man returnerer ikke objektet, kun om det skal inkluderes i resultatet. I dette tilfælde virker det alligevel, fordi null er falsy, og role er truthy.
| let addExtraRoles = null; | ||
| let filteredRoles; | ||
| if (req.body.roles) { | ||
| filteredRoles = req.body.roles.filter((role) => { |
There was a problem hiding this comment.
Hele udsagnet kan reduceres til:
const _ = require('lodash');
filteredRoles = req.body.roles.filter((role) => !((_.isPlainObject(role) && (!role.scope && role.role)) || (_.isString(role) && role.indexOf('@') === -1)));
| console.log('identity'); | ||
| console.log(req.identity.roles.toArray()); | ||
| console.log('role'); | ||
| console.log(role); |
There was a problem hiding this comment.
Fjern altid console.log før merge.
| } | ||
|
|
||
| if (addExtraRoles) req.body.roles = addExtraRoles; | ||
| // if (addExtraRoles) req.body.roles = addExtraRoles; |
There was a problem hiding this comment.
Efterlad ikke udkommenteret kode. Hellere bare slette det. Det findes jo i git history.
2 participants
Fixes #59