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(#8886): block users from editing important fields in their settings #9107
Conversation
) { | ||
'use strict'; | ||
'ngInject'; | ||
|
||
const getHtml = string => { | ||
return $sanitize(PrivacyPolicies.decodeUnicode(string)); |
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.
This is a refactor that I found while working out the privacy policy fields in user settings - by inlining this one function we can drop the entire service.
@@ -13,11 +13,42 @@ function(newDoc, oldDoc, userCtx) { | |||
throw({ forbidden: msg }); | |||
}; | |||
|
|||
var hasRole = function(roles, role) { |
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.
Copied from the server validate_doc_update function. We don't yet have a sensible way to include functions like this.
return true; | ||
} | ||
|
||
if (secObj.admins && secObj.admins.roles) { | ||
for (var i = 0; i < userCtx.roles; i++) { | ||
if (hasRole(secObj.admins, userCtx.roles[i])) { | ||
for (var i = 0; i < userCtx.roles.length; i++) { |
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.
There was a bug here - without the .length
this loop was never executing so national_admins were blocked. Found when adding unit tests for this PR.
// A valid user-settings doc. Does not require type property because that is | ||
// already matched before passing to the validation function. | ||
userSettings = { | ||
const wrapValidateDocUpdate = (file) => eval(`log = console.log; (${fs.readFileSync(__dirname + file)})`); |
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.
I refactored this significantly to make it easier to add to. Sorry for the noise!
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.
Sweet!
Description
#8886
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.