[Fix]: Saving users to DB before validating custom fields

packages/rocketchat-api/server/v1/users.js

@@ -19,12 +19,17 @@ RocketChat.API.v1.addRoute('users.create', { authRequired: true }, {
 			this.bodyParams.joinDefaultChannels = true;
 		}
 
+		if (this.bodyParams.customFields) {
+			RocketChat.validateCustomFields(this.bodyParams.customFields);
+		}
+
 		const newUserId = RocketChat.saveUser(this.userId, this.bodyParams);
 
 		if (this.bodyParams.customFields) {
-			RocketChat.saveCustomFields(newUserId, this.bodyParams.customFields);
+			RocketChat.saveCustomFieldsWithoutValidation(newUserId, this.bodyParams.customFields);
 		}
 
+
 		if (typeof this.bodyParams.active !== 'undefined') {
 			Meteor.runAsUser(this.userId, () => {
 				Meteor.call('setUserActiveStatus', newUserId, this.bodyParams.active);

packages/rocketchat-lib/package.js

@@ -82,6 +82,7 @@ Package.onUse(function(api) {
 	api.addFiles('server/functions/removeUserFromRoom.js', 'server');
 	api.addFiles('server/functions/saveUser.js', 'server');
 	api.addFiles('server/functions/saveCustomFields.js', 'server');
+	api.addFiles('server/functions/saveCustomFieldsWithoutValidation.js', 'server');
 	api.addFiles('server/functions/sendMessage.js', 'server');
 	api.addFiles('server/functions/settings.js', 'server');
 	api.addFiles('server/functions/setUserAvatar.js', 'server');
@@ -90,6 +91,7 @@ Package.onUse(function(api) {
 	api.addFiles('server/functions/setEmail.js', 'server');
 	api.addFiles('server/functions/unarchiveRoom.js', 'server');
 	api.addFiles('server/functions/updateMessage.js', 'server');
+	api.addFiles('server/functions/validateCustomFields.js', 'server');
 	api.addFiles('server/functions/Notifications.js', 'server');
 
 	// SERVER LIB

packages/rocketchat-lib/server/functions/saveCustomFields.js

@@ -1,56 +1,6 @@
 RocketChat.saveCustomFields = function(userId, formData) {
 	if (s.trim(RocketChat.settings.get('Accounts_CustomFields')) !== '') {
-		let customFieldsMeta;
-		try {
-			customFieldsMeta = JSON.parse(RocketChat.settings.get('Accounts_CustomFields'));
-		} catch (e) {
-			throw new Meteor.Error('error-invalid-customfield-json', 'Invalid JSON for Custom Fields');
-		}
-
-		const customFields = {};
-
-		Object.keys(customFieldsMeta).forEach((fieldName) => {
-			const field = customFieldsMeta[fieldName];
-
-			customFields[fieldName] = formData[fieldName];
-			if (field.required && !formData[fieldName]) {
-				throw new Meteor.Error('error-user-registration-custom-field', `Field ${ fieldName } is required`, { method: 'registerUser' });
-			}
-
-			if (field.type === 'select' && field.options.indexOf(formData[fieldName]) === -1) {
-				throw new Meteor.Error('error-user-registration-custom-field', `Value for field ${ fieldName } is invalid`, { method: 'registerUser' });
-			}
-
-			if (field.maxLength && formData[fieldName].length > field.maxLength) {
-				throw new Meteor.Error('error-user-registration-custom-field', `Max length of field ${ fieldName } ${ field.maxLength }`, { method: 'registerUser' });
-			}
-
-			if (field.minLength && formData[fieldName].length < field.minLength) {
-				throw new Meteor.Error('error-user-registration-custom-field', `Min length of field ${ fieldName } ${ field.minLength }`, { method: 'registerUser' });
-			}
-		});
-
-		// for fieldName, field of customFieldsMeta
-		RocketChat.models.Users.setCustomFields(userId, customFields);
-
-		Object.keys(customFields).forEach((fieldName) => {
-			if (!customFieldsMeta[fieldName].modifyRecordField) {
-				return;
-			}
-
-			const modifyRecordField = customFieldsMeta[fieldName].modifyRecordField;
-			const update = {};
-			if (modifyRecordField.array) {
-				update.$addToSet = {};
-				update.$addToSet[modifyRecordField.field] = customFields[fieldName];
-			} else {
-				update.$set = {};
-				update.$set[modifyRecordField.field] = customFields[fieldName];
-			}
-
-			RocketChat.models.Users.update(userId, update);
-		});
-
-		return true;
+		RocketChat.validateCustomFields(formData);
+		return RocketChat.saveCustomFieldsWithoutValidation(userId, formData);
 	}
 };

packages/rocketchat-lib/server/functions/saveCustomFieldsWithoutValidation.js

@@ -0,0 +1,33 @@
+RocketChat.saveCustomFieldsWithoutValidation = function(userId, formData) {
+	if (s.trim(RocketChat.settings.get('Accounts_CustomFields')) !== '') {
+		let customFieldsMeta;
+		try {
+			customFieldsMeta = JSON.parse(RocketChat.settings.get('Accounts_CustomFields'));
+		} catch (e) {
+			throw new Meteor.Error('error-invalid-customfield-json', 'Invalid JSON for Custom Fields');
+		}
+
+		const customFields = formData;
+
+		// for fieldName, field of customFieldsMeta
+		RocketChat.models.Users.setCustomFields(userId, customFields);
+
+		Object.keys(customFields).forEach((fieldName) => {
+			if (!customFieldsMeta[fieldName].modifyRecordField) {
+				return;
+			}
+
+			const modifyRecordField = customFieldsMeta[fieldName].modifyRecordField;
+			const update = {};
+			if (modifyRecordField.array) {
+				update.$addToSet = {};
+				update.$addToSet[modifyRecordField.field] = customFields[fieldName];
+			} else {
+				update.$set = {};
+				update.$set[modifyRecordField.field] = customFields[fieldName];
+			}
+
+			RocketChat.models.Users.update(userId, update);
+		});
+	}
+};

packages/rocketchat-lib/server/functions/validateCustomFields.js

@@ -0,0 +1,39 @@
+RocketChat.validateCustomFields = function(fields) {
+	// Special Case:
+	// If an admin didn't set any custom fields there's nothing to validate against so consider any customFields valid
+	if (s.trim(RocketChat.settings.get('Accounts_CustomFields')) === '') {
+		return;
+	}
+
+	let customFieldsMeta;
+	try {
+		customFieldsMeta = JSON.parse(RocketChat.settings.get('Accounts_CustomFields'));
+	} catch (e) {
+		throw new Meteor.Error('error-invalid-customfield-json', 'Invalid JSON for Custom Fields');
+	}
+
+	const customFields = {};
+
+	Object.keys(customFieldsMeta).forEach((fieldName) => {
+		const field = customFieldsMeta[fieldName];
+
+		customFields[fieldName] = fields[fieldName];
+		const fieldValue = s.trim(fields[fieldName]);
+
+		if (field.required && fieldValue === '') {
+			throw new Meteor.Error('error-user-registration-custom-field', `Field ${ fieldName } is required`, {method: 'registerUser'});
+		}
+
+		if (field.type === 'select' && field.options.indexOf(fields[fieldName]) === -1) {
+			throw new Meteor.Error('error-user-registration-custom-field', `Value for field ${ fieldName } is invalid`, {method: 'registerUser'});
+		}
+
+		if (field.maxLength && fieldValue.length > field.maxLength) {
+			throw new Meteor.Error('error-user-registration-custom-field', `Max length of field ${ fieldName } ${ field.maxLength }`, {method: 'registerUser'});
+		}
+
+		if (field.minLength && fieldValue.length < field.minLength) {
+			throw new Meteor.Error('error-user-registration-custom-field', `Min length of field ${ fieldName } ${ field.minLength }`, {method: 'registerUser'});
+		}
+	});
+};

tests/data/custom-fields.js

@@ -0,0 +1,28 @@
+import {getCredentials, request, api, credentials} from './api-data.js';
+
+export const customFieldText = {
+	type: 'text',
+	required: true,
+	minLength: 2,
+	maxLength: 10
+};
+
+export function setCustomFields(customFields, done) {
+	getCredentials((error) => {
+		if (error) {
+			return done(error);
+		}
+
+		const stringified = customFields ? JSON.stringify(customFields) : '';
+
+		request.post(api('settings/Accounts_CustomFields'))
+			.set(credentials)
+			.send({ 'value': stringified })
+			.expect(200)
+			.end(done);
+	});
+}
+
+export function clearCustomFields(done = () => {}) {
+	setCustomFields(null, done);
+}

tests/end-to-end/api/01-users.js

@@ -5,140 +5,236 @@
 import {getCredentials, api, login, request, credentials, apiEmail, apiUsername, targetUser, log} from '../../data/api-data.js';
 import {adminEmail, password} from '../../data/user.js';
 import {imgURL} from '../../data/interactions.js';
+import {customFieldText, clearCustomFields, setCustomFields} from '../../data/custom-fields.js';
 
 describe('[Users]', function() {
 	this.retries(0);
 
 	before(done => getCredentials(done));
 
-	it('/users.create:', (done) => {
-		request.post(api('users.create'))
-			.set(credentials)
-			.send({
-				email: apiEmail,
-				name: apiUsername,
-				username: apiUsername,
-				password,
-				active: true,
-				roles: ['user'],
-				joinDefaultChannels: true,
-				verified:true
-			})
-			.expect('Content-Type', 'application/json')
-			.expect(200)
-			.expect((res) => {
-				expect(res.body).to.have.property('success', true);
-				expect(res.body).to.have.deep.property('user.username', apiUsername);
-				expect(res.body).to.have.deep.property('user.emails[0].address', apiEmail);
-				expect(res.body).to.have.deep.property('user.active', true);
-				expect(res.body).to.have.deep.property('user.name', apiUsername);
-				targetUser._id = res.body.user._id;
-			})
-			.end(done);
-	});
+	describe('[/users.create]', () => {
+		before(done => clearCustomFields(done));
+		after(done => clearCustomFields(done));
+
+		it('should create a new user', (done) => {
+			request.post(api('users.create'))
+				.set(credentials)
+				.send({
+					email: apiEmail,
+					name: apiUsername,
+					username: apiUsername,
+					password,
+					active: true,
+					roles: ['user'],
+					joinDefaultChannels: true,
+					verified: true
+				})
+				.expect('Content-Type', 'application/json')
+				.expect(200)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', true);
+					expect(res.body).to.have.deep.property('user.username', apiUsername);
+					expect(res.body).to.have.deep.property('user.emails[0].address', apiEmail);
+					expect(res.body).to.have.deep.property('user.active', true);
+					expect(res.body).to.have.deep.property('user.name', apiUsername);
 
-	it('/users.info:', (done) => {
-		request.get(api('users.info'))
-			.set(credentials)
-			.query({
-				userId: targetUser._id
-			})
-			.expect('Content-Type', 'application/json')
-			.expect(200)
-			.expect((res) => {
-				expect(res.body).to.have.property('success', true);
-				expect(res.body).to.have.deep.property('user.username', apiUsername);
-				expect(res.body).to.have.deep.property('user.emails[0].address', apiEmail);
-				expect(res.body).to.have.deep.property('user.active', true);
-				expect(res.body).to.have.deep.property('user.name', apiUsername);
-			})
-			.end(done);
+					expect(res.body).to.not.have.deep.property('user.customFields');
+
+					targetUser._id = res.body.user._id;
+				})
+				.end(done);
+		});
+
+		it('should create a new user with custom fields', (done) => {
+			setCustomFields({customFieldText}, (error) => {
+				if (error) {
+					return done(error);
+				}
+
+				const username = `customField_${ apiUsername }`;
+				const email = `customField_${ apiEmail }`;
+				const customFields = { customFieldText: 'success' };
+
+				request.post(api('users.create'))
+					.set(credentials)
+					.send({
+						email,
+						name: username,
+						username,
+						password,
+						active: true,
+						roles: ['user'],
+						joinDefaultChannels: true,
+						verified: true,
+						customFields
+					})
+					.expect('Content-Type', 'application/json')
+					.expect(200)
+					.expect((res) => {
+						expect(res.body).to.have.property('success', true);
+						expect(res.body).to.have.deep.property('user.username', username);
+						expect(res.body).to.have.deep.property('user.emails[0].address', email);
+						expect(res.body).to.have.deep.property('user.active', true);
+						expect(res.body).to.have.deep.property('user.name', username);
+						expect(res.body).to.have.deep.property('user.customFields.customFieldText', 'success');
+					})
+					.end(done);
+			});
+		});
+
+		function failUserWithCustomField(field) {
+			it(`should not create a user if a custom field ${ field.reason }`, (done) => {
+				setCustomFields({ customFieldText }, (error) => {
+					if (error) {
+						return done(error);
+					}
+
+					const customFields = {};
+					customFields[field.name] = field.value;
+
+					request.post(api('users.create'))
+						.set(credentials)
+						.send({
+							email: `customField_fail_${ apiEmail }`,
+							name: `customField_fail_${ apiUsername }`,
+							username: `customField_fail_${ apiUsername }`,
+							password,
+							active: true,
+							roles: ['user'],
+							joinDefaultChannels: true,
+							verified: true,
+							customFields
+						})
+						.expect('Content-Type', 'application/json')
+						.expect(400)
+						.expect((res) => {
+							expect(res.body).to.have.property('success', false);
+							expect(res.body).to.have.property('errorType', 'error-user-registration-custom-field');
+						})
+						.end(done);
+				});
+			});
+		}
+
+		[
+			{name: 'customFieldText', value: '', reason: 'is required and missing'},
+			{name: 'customFieldText', value: '0', reason: 'length is less than minLength'},
+			{name: 'customFieldText', value: '0123456789-0', reason: 'length is more than maxLength'}
+		].forEach((field) => { failUserWithCustomField(field); });
 	});
 
-	it('/users.getPresence:', (done) => {
-		request.get(api('users.getPresence'))
-			.set(credentials)
-			.query({
-				userId: targetUser._id
-			})
-			.expect('Content-Type', 'application/json')
-			.expect(200)
-			.expect((res) => {
-				expect(res.body).to.have.property('success', true);
-				expect(res.body).to.have.deep.property('presence', 'offline');
-			})
-			.end(done);
+	describe('[/users.info]', () => {
+		it('should query information about a user by userId', (done) => {
+			request.get(api('users.info'))
+				.set(credentials)
+				.query({
+					userId: targetUser._id
+				})
+				.expect('Content-Type', 'application/json')
+				.expect(200)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', true);
+					expect(res.body).to.have.deep.property('user.username', apiUsername);
+					expect(res.body).to.have.deep.property('user.emails[0].address', apiEmail);
+					expect(res.body).to.have.deep.property('user.active', true);
+					expect(res.body).to.have.deep.property('user.name', apiUsername);
+				})
+				.end(done);
+		});
 	});
 
-	it('/users.list:', (done) => {
-		request.get(api('users.list'))
-			.set(credentials)
-			.expect('Content-Type', 'application/json')
-			.expect(200)
-			.expect((res) => {
-				expect(res.body).to.have.property('success', true);
-				expect(res.body).to.have.property('count');
-				expect(res.body).to.have.property('total');
-			})
-			.end(done);
+	describe('[/users.getPresence]', () => {
+		it('should query a user\'s presence by userId', (done) => {
+			request.get(api('users.getPresence'))
+				.set(credentials)
+				.query({
+					userId: targetUser._id
+				})
+				.expect('Content-Type', 'application/json')
+				.expect(200)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', true);
+					expect(res.body).to.have.deep.property('presence', 'offline');
+				})
+				.end(done);
+		});
 	});
 
-	it.skip('/users.list:', (done) => {
-		//filtering user list
-		request.get(api('users.list'))
-			.set(credentials)
-			.query({
-				name: { '$regex': 'g' }
-			})
-			.field('username', 1)
-			.sort('createdAt', -1)
-			.expect(log)
-			.expect('Content-Type', 'application/json')
-			.expect(200)
-			.expect((res) => {
-				expect(res.body).to.have.property('success', true);
-				expect(res.body).to.have.property('count');
-				expect(res.body).to.have.property('total');
-			})
-			.end(done);
+	describe('[/users.list]', () => {
+		it('should query all users in the system', (done) => {
+			request.get(api('users.list'))
+				.set(credentials)
+				.expect('Content-Type', 'application/json')
+				.expect(200)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', true);
+					expect(res.body).to.have.property('count');
+					expect(res.body).to.have.property('total');
+				})
+				.end(done);
+		});
+
+		it.skip('should query all users in the system by name', (done) => {
+			//filtering user list
+			request.get(api('users.list'))
+				.set(credentials)
+				.query({
+					name: { '$regex': 'g' }
+				})
+				.field('username', 1)
+				.sort('createdAt', -1)
+				.expect(log)
+				.expect('Content-Type', 'application/json')
+				.expect(200)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', true);
+					expect(res.body).to.have.property('count');
+					expect(res.body).to.have.property('total');
+				})
+				.end(done);
+		});
 	});
 
-	it.skip('/users.setAvatar:', (done) => {
-		request.post(api('users.setAvatar'))
-			.set(credentials)
-			.attach('avatarUrl', imgURL)
-			.expect('Content-Type', 'application/json')
-			.expect(200)
-			.expect((res) => {
-				expect(res.body).to.have.property('success', true);
-			})
-			.end(done);
+	describe('[/users.setAvatar]', () => {
+		it.skip('should set the avatar of the auth user', (done) => {
+			request.post(api('users.setAvatar'))
+				.set(credentials)
+				.attach('avatarUrl', imgURL)
+				.expect('Content-Type', 'application/json')
+				.expect(200)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', true);
+				})
+				.end(done);
+		});
 	});
 
-	it('/users.update:', (done) => {
-		request.post(api('users.update'))
-			.set(credentials)
-			.send({
-				userId: targetUser._id,
-				data :{
-					email: apiEmail,
-					name: `edited${ apiUsername }`,
-					username: `edited${ apiUsername }`,
-					password,
-					active: true,
-					roles: ['user']
-				}
-			})
-			.expect('Content-Type', 'application/json')
-			.expect(200)
-			.expect((res) => {
-				expect(res.body).to.have.property('success', true);
-				expect(res.body).to.have.deep.property('user.username', `edited${ apiUsername }`);
-				expect(res.body).to.have.deep.property('user.emails[0].address', apiEmail);
-				expect(res.body).to.have.deep.property('user.active', true);
-				expect(res.body).to.have.deep.property('user.name', `edited${ apiUsername }`);
-			})
-			.end(done);
+	describe('[/users.update]', () => {
+		it('should update a user\'s info by userId', (done) => {
+			request.post(api('users.update'))
+				.set(credentials)
+				.send({
+					userId: targetUser._id,
+					data :{
+						email: apiEmail,
+						name: `edited${ apiUsername }`,
+						username: `edited${ apiUsername }`,
+						password,
+						active: true,
+						roles: ['user']
+					}
+				})
+				.expect('Content-Type', 'application/json')
+				.expect(200)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', true);
+					expect(res.body).to.have.deep.property('user.username', `edited${ apiUsername }`);
+					expect(res.body).to.have.deep.property('user.emails[0].address', apiEmail);
+					expect(res.body).to.have.deep.property('user.active', true);
+					expect(res.body).to.have.deep.property('user.name', `edited${ apiUsername }`);
+				})
+				.end(done);
+		});
 	});
 
 	describe('[/users.createToken]', () => {