Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion backend/db/models/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ module.exports = (sequelize, DataTypes) => {
afterUpdate: async (user, options) => {
const {context, transaction} = options;
const {userRoles, roleMap} = context || {};
if (userRoles && roleMap) {
if (userRoles) {
await assignUserRoles(user, userRoles, roleMap, true, transaction);
}
},
Expand Down
7 changes: 4 additions & 3 deletions backend/webserver/sockets/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ class UserSocket extends Socket {
*/
async bulkCreateUsers(data) {
const users = data["users"];
const roleMap = data["roleMap"];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roleMap comes from the client, and assignUserRoles in backend/db/models/user.js assigns any role name it gets. A caller can send roleMap: {"Student*in": "admin"} together with an existing user's email and make that account an admin, because userBulkCreate has no isAdmin() check (the other handlers in this class do have one). Please guard the handler with if (!(await this.isAdmin())) throw ... and drop any mapped value that is not in the allowed role list, since the "no admin" filter in RoleMappingStep.vue only applies to the dropdown.


const createdUsers = [];
const errors = [];
Expand All @@ -233,7 +234,7 @@ class UserSocket extends Socket {
if (!user.exists) {
createdUser = await this.models["user"].add(user, {
transaction, context: {
userRoles: user.roles, roleMap: data["moodleCareRoleMap"],
userRoles: user.roles, roleMap,
},
})

Expand All @@ -244,7 +245,7 @@ class UserSocket extends Socket {
firstName: user.firstName, lastName: user.lastName, extId: user.extId, emailVerified: true,
}, {
transaction, context: {
userRoles: user.roles, roleMap: data["moodleCareRoleMap"],
userRoles: user.roles, roleMap,
}
});
} else {
Expand Down Expand Up @@ -525,4 +526,4 @@ class UserSocket extends Socket {
}
};

module.exports = UserSocket;
module.exports = UserSocket;
63 changes: 57 additions & 6 deletions backend/webserver/utils/settingSave.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,17 @@
const { assertStableEmailTemplateContent } = require("../../utils/helper/templateResolver");

const MAIL_SERVICE_KEY_PREFIX = "system.mailService.";
const PRESERVE_WHITESPACE_SETTING_TYPES = new Set(["edits", "text"]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRESERVE_WHITESPACE_SETTING_TYPES lists the types to skip, so shouldTrimSetting trims every type that is not in the set. Any setting type added later, and any entry whose type could not be resolved, gets trimmed by default without anyone noticing. Flip it to an allow-list of types that should be trimmed (for example "string", "number", "select", "color"), so a new type has to opt in.


/**
* Returns whether a setting value should be trimmed before saving.
*
* @param {Object} setting setting entry
* @returns {boolean}
*/
function shouldTrimSetting(setting) {
return !PRESERVE_WHITESPACE_SETTING_TYPES.has(setting?.type);
}

/**
* Reject email.template.* settings that point at a missing or incomplete template.
Expand Down Expand Up @@ -66,16 +77,49 @@ function payloadTouchesMailService(settings) {
* Normalize setting values to string payload format expected by the settings model.
*
* @param {*} value setting value
* @param {Object} [setting] setting entry
* @returns {string}
*/
function normalizeSettingValue(value) {
function normalizeSettingValue(value, setting = {}) {
let normalized;
if (value === null || value === undefined) {
return "";
normalized = "";
} else if (typeof value === "object") {
// NOTE: Coerce object/array payloads to JSON; persisted settings are always strings.
normalized = JSON.stringify(value);
} else {
normalized = String(value);
}
if (typeof value === "object") {
return JSON.stringify(value);
return shouldTrimSetting(setting) ? normalized.trim() : normalized;
}

/**
* Load persisted setting types for payload entries that do not include type metadata.
*
* @param {Object} Setting setting model
* @param {Object[]} settings setting entries
* @param {Object} [options] additional options
* @returns {Promise<Map<string, string>>}
*/
async function getSettingTypeByKey(Setting, settings, options = {}) {
if (typeof Setting.findAll !== "function") {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both callers of saveSettings pass a real Sequelize model, so Setting.findAll is always a function and this branch never runs. If it ever did run it would return an empty Map, and every setting would fall back to being trimmed, including the "text" and "edits" types this PR wants to protect. Drop the guard, or throw here, rather than silently changing how values are saved.

return new Map();
}

const keys = [...new Set(settings
.filter((setting) => setting && typeof setting.key === "string" && !setting.type)
.map((setting) => setting.key))];
if (!keys.length) {
return new Map();
}
return String(value);

const rows = await Setting.findAll({
where: { key: keys },
attributes: ["key", "type"],
raw: true,
transaction: options.transaction,
});
return new Map(rows.map((row) => [row.key, row.type]));
}

/**
Expand All @@ -96,19 +140,26 @@ async function saveSettings(Setting, settings, options = {}) {
await validateEmailTemplateSettings(options.models, list, options);
}
const touchesMailService = payloadTouchesMailService(list);
const settingTypeByKey = await getSettingTypeByKey(Setting, list, options);
for (const setting of list) {
if (!setting || typeof setting.key !== "string" || setting.key.trim() === "") {
continue;
}
await Setting.set(setting.key, normalizeSettingValue(setting.value), {
const settingWithType = setting.type ? setting : {
...setting,
type: settingTypeByKey.get(setting.key),
};
await Setting.set(setting.key, normalizeSettingValue(setting.value, settingWithType), {
transaction: options.transaction,
});
}
return { touchesMailService };
}

module.exports = {
getSettingTypeByKey,
payloadTouchesMailService,
normalizeSettingValue,
saveSettings,
shouldTrimSetting,
};
8 changes: 7 additions & 1 deletion docs/source/for_researchers/moodle_usage.rst
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,20 @@ Before using this feature, make sure Moodle API access is configured as describe
User data can be imported from Moodle using the course ID:

1. In the Dashboard navigate to ``Users > Import via Moodle``
2. CARE will match users by email address
2. After CARE retrieves the Moodle users, review the role mapping step
3. Map each Moodle role label to the corresponding CARE role, or choose ``Do not assign additional role`` if the role should not grant additional CARE permissions
4. CARE will match users by email address

CARE handles three scenarios:

- New users are created if not found
- Duplicate users are merged based on email match
- Conflicts (e.g., mismatched emails) require manual correction

Moodle role labels can vary by Moodle instance and may include multilingual markup such as ``{mlang de}Lehrende{mlang}{mlang other}Lecturer{mlang}``. CARE hides this markup in the mapping step while keeping the original Moodle role string for the import.

CSV user import also includes a role mapping step. CARE reads the distinct values from the CSV ``roles`` column and asks you to map each value to a CARE role before previewing the import. Multiple roles in one CSV cell should be separated by commas.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CARE docs name the role instead of addressing the reader, so "asks you to map" does not match the rest of this page. Please write "asks the admin to map each value to a CARE role".


.. warning::

Never delete a user with an ``extId`` unless you are certain it won't be needed. This could prevent future updates or synchronization.
Expand Down
5 changes: 1 addition & 4 deletions frontend/src/components/dashboard/Settings.vue
Original file line number Diff line number Diff line change
Expand Up @@ -356,15 +356,12 @@ export default {
save() {
this.$socket.emit("settingSave", this.settings, (res) => {
if (res.success) {
this.settings.forEach((s) => {
this.$store.commit("settings/set", { key: s.key, value: s.value });
});
this.eventBus.emit("toast", {
title: "Success",
message: res.data,
variant: "success",
});
this.setSettingsSnapshot();
this.load(false);
} else {
this.eventBus.emit("toast", {
title: "Error Saving Settings",
Expand Down
Loading
Loading