From 1411fbb124c7718e637bf01bc3bf4e3b41f2d6bc Mon Sep 17 00:00:00 2001 From: Robert Jakubowicz Date: Mon, 17 Jul 2017 11:48:20 -0400 Subject: [PATCH] [Fix]: Saving users to DB before validating custom fields --- packages/rocketchat-api/server/v1/users.js | 7 +- packages/rocketchat-lib/package.js | 2 + .../server/functions/saveCustomFields.js | 54 +-- .../saveCustomFieldsWithoutValidation.js | 33 ++ .../server/functions/validateCustomFields.js | 39 +++ tests/data/custom-fields.js | 28 ++ tests/end-to-end/api/01-users.js | 330 +++++++++++------- 7 files changed, 323 insertions(+), 170 deletions(-) create mode 100644 packages/rocketchat-lib/server/functions/saveCustomFieldsWithoutValidation.js create mode 100644 packages/rocketchat-lib/server/functions/validateCustomFields.js create mode 100644 tests/data/custom-fields.js diff --git a/packages/rocketchat-api/server/v1/users.js b/packages/rocketchat-api/server/v1/users.js index 2a9651c3098b..608fbb413f22 100644 --- a/packages/rocketchat-api/server/v1/users.js +++ b/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); diff --git a/packages/rocketchat-lib/package.js b/packages/rocketchat-lib/package.js index 075f694d6baa..a9c823b9bf06 100644 --- a/packages/rocketchat-lib/package.js +++ b/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 diff --git a/packages/rocketchat-lib/server/functions/saveCustomFields.js b/packages/rocketchat-lib/server/functions/saveCustomFields.js index 4b3717897f02..8088ed49d6a2 100644 --- a/packages/rocketchat-lib/server/functions/saveCustomFields.js +++ b/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); } }; diff --git a/packages/rocketchat-lib/server/functions/saveCustomFieldsWithoutValidation.js b/packages/rocketchat-lib/server/functions/saveCustomFieldsWithoutValidation.js new file mode 100644 index 000000000000..27d610bd2771 --- /dev/null +++ b/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); + }); + } +}; diff --git a/packages/rocketchat-lib/server/functions/validateCustomFields.js b/packages/rocketchat-lib/server/functions/validateCustomFields.js new file mode 100644 index 000000000000..83e60415fb0e --- /dev/null +++ b/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'}); + } + }); +}; diff --git a/tests/data/custom-fields.js b/tests/data/custom-fields.js new file mode 100644 index 000000000000..65c0f24fb1e2 --- /dev/null +++ b/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); +} diff --git a/tests/end-to-end/api/01-users.js b/tests/end-to-end/api/01-users.js index 7357fe5505a1..ec6b23c92009 100644 --- a/tests/end-to-end/api/01-users.js +++ b/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]', () => {