Improved validation layer (#9427)

refs #3658

- the `validateSchema` helper was a bit broken
  - if you add a user without email, you will receive a database error
  - but the validation error should catch that email is passed with null
- it was broken, because:
  - A: it called `toJSON` -> this can remove properties from the output (e.g. password)
  - B: we only validated fields, which were part of the JSON data (model.hasOwnProperty)
- we now differentiate between schema validation for update and insert
- fixed one broken import test
  - if you import a post without a status, it should not error
  - it falls back to the default value
- removed user model `onValidate`
  - the user model added a custom implementation of `onValidate`, because of a bug which we experienced (see #3638)
  - with the refactoring this is no longer required - we only validate fields which have changed when updating resources
  - also, removed extra safe catch when logging in (no longer needed - unit tested)
- add lot's of unit tests to proof the code change
- always call the base class, except you have a good reason

core/server/data/validation/index.js

@@ -161,19 +161,40 @@ validatePassword = function validatePassword(password, email, blogTitle) {
     return validationResult;
 };
 
-// Validation against schema attributes
-// values are checked against the validation objects from schema.js
-validateSchema = function validateSchema(tableName, model) {
+/**
+ * Validate model against schema.
+ *
+ * ## on model update
+ * - only validate changed fields
+ * - otherwise we could throw errors which the user is out of control
+ * - e.g.
+ *   - we add a new field without proper validation, release goes out
+ *   - we add proper validation for a single field
+ * - if you call `user.save()` the default fallback in bookshelf is `options.method=update`.
+ * - we set `options.method` explicit for adding resources (because otherwise bookshelf uses `update`)
+ *
+ * ## on model add
+ * - validate everything to catch required fields
+ */
+validateSchema = function validateSchema(tableName, model, options) {
+    options = options || {};
+
     var columns = _.keys(schema[tableName]),
         validationErrors = [];
 
     _.each(columns, function each(columnKey) {
         var message = '',
-            strVal = _.toString(model[columnKey]);
+            strVal = _.toString(model.get(columnKey)); // KEEP: Validator.js only validates strings.
+
+        if (options.method !== 'insert' && !_.has(model.changed, columnKey)) {
+            return;
+        }
 
         // check nullable
-        if (model.hasOwnProperty(columnKey) && schema[tableName][columnKey].hasOwnProperty('nullable')
-            && schema[tableName][columnKey].nullable !== true) {
+        if (schema[tableName][columnKey].hasOwnProperty('nullable') &&
+            schema[tableName][columnKey].nullable !== true &&
+            !schema[tableName][columnKey].hasOwnProperty('defaultTo')
+        ) {
             if (validator.empty(strVal)) {
                 message = common.i18n.t('notices.data.validation.index.valueCannotBeBlank', {
                     tableName: tableName,
@@ -187,7 +208,7 @@ validateSchema = function validateSchema(tableName, model) {
         }
 
         // validate boolean columns
-        if (model.hasOwnProperty(columnKey) && schema[tableName][columnKey].hasOwnProperty('type')
+        if (schema[tableName][columnKey].hasOwnProperty('type')
             && schema[tableName][columnKey].type === 'bool') {
             if (!(validator.isBoolean(strVal) || validator.empty(strVal))) {
                 message = common.i18n.t('notices.data.validation.index.valueMustBeBoolean', {
@@ -202,7 +223,7 @@ validateSchema = function validateSchema(tableName, model) {
         }
 
         // TODO: check if mandatory values should be enforced
-        if (model[columnKey] !== null && model[columnKey] !== undefined) {
+        if (model.get(columnKey) !== null && model.get(columnKey) !== undefined) {
             // check length
             if (schema[tableName][columnKey].hasOwnProperty('maxlength')) {
                 if (!validator.isLength(strVal, 0, schema[tableName][columnKey].maxlength)) {

core/server/models/base/index.js

@@ -78,11 +78,6 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
         return _.keys(schema.tables[this.tableName]);
     },
 
-    // Bookshelf `defaults` - default values setup on every model creation
-    defaults: function defaults() {
-        return {};
-    },
-
     // When loading an instance, subclasses can specify default to fetch
     defaultColumnsToFetch: function defaultColumnsToFetch() {
         return [];
@@ -136,8 +131,12 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
         proto.initialize.call(this);
     },
 
-    onValidate: function onValidate() {
-        return validation.validateSchema(this.tableName, this.toJSON());
+    /**
+     * Do not call `toJSON`. This can remove properties e.g. password.
+     * @returns {*}
+     */
+    onValidate: function onValidate(model, columns, options) {
+        return validation.validateSchema(this.tableName, this, options);
     },
 
     /**

core/server/models/post.js

@@ -20,6 +20,22 @@ Post = ghostBookshelf.Model.extend({
 
     tableName: 'posts',
 
+    /**
+     * ## NOTE:
+     * We define the defaults on the schema (db) and model level.
+     * When inserting resources, the defaults are available **after** calling `.save`.
+     * But they are available when the model hooks are triggered (e.g. onSaving).
+     * It might be useful to keep them in the model layer for any connected logic.
+     *
+     * e.g. if `model.get('status') === draft; do something;
+     */
+    defaults: function defaults() {
+        return {
+            uuid: uuid.v4(),
+            status: 'draft'
+        };
+    },
+
     relationships: ['tags'],
 
     /**
@@ -49,13 +65,6 @@ Post = ghostBookshelf.Model.extend({
         common.events.emit(resourceType + '.' + event, this, options);
     },
 
-    defaults: function defaults() {
-        return {
-            uuid: uuid.v4(),
-            status: 'draft'
-        };
-    },
-
     /**
      * We update the tags after the Post was inserted.
      * We update the tags before the Post was updated, see `onSaving` event.

core/server/models/settings.js

@@ -77,12 +77,12 @@ Settings = ghostBookshelf.Model.extend({
     },
 
     onValidate: function onValidate() {
-        var self = this,
-            setting = this.toJSON();
+        var self = this;
 
-        return validation.validateSchema(self.tableName, setting).then(function then() {
-            return validation.validateSettings(getDefaultSettings(), self);
-        });
+        return ghostBookshelf.Model.prototype.onValidate.apply(this, arguments)
+            .then(function then() {
+                return validation.validateSettings(getDefaultSettings(), self);
+            });
     }
 }, {
     findOne: function (data, options) {

core/server/models/user.js

@@ -24,11 +24,9 @@ User = ghostBookshelf.Model.extend({
     tableName: 'users',
 
     defaults: function defaults() {
-        var baseDefaults = ghostBookshelf.Model.prototype.defaults.call(this);
-
-        return _.merge({
+        return {
             password: security.identifier.uid(50)
-        }, baseDefaults);
+        };
     },
 
     emitChange: function emitChange(event, options) {
@@ -90,6 +88,22 @@ User = ghostBookshelf.Model.extend({
 
         ghostBookshelf.Model.prototype.onSaving.apply(this, arguments);
 
+        /**
+         * Bookshelf call order:
+         *   - onSaving
+         *   - onValidate (validates the model against the schema)
+         *
+         * Before we can generate a slug, we have to ensure that the name is not blank.
+         */
+        if (!this.get('name')) {
+            throw new common.errors.ValidationError({
+                message: common.i18n.t('notices.data.validation.index.valueCannotBeBlank', {
+                    tableName: this.tableName,
+                    columnKey: 'name'
+                })
+            });
+        }
+
         // If the user's email is set & has changed & we are not importing
         if (self.hasChanged('email') && self.get('email') && !options.importing) {
             tasks.gravatar = (function lookUpGravatar() {
@@ -169,24 +183,6 @@ User = ghostBookshelf.Model.extend({
         return Promise.props(tasks);
     },
 
-    // For the user model ONLY it is possible to disable validations.
-    // This is used to bypass validation during the credential check, and must never be done with user-provided data
-    // Should be removed when #3691 is done
-    onValidate: function validate() {
-        var opts = arguments[1],
-            userData;
-
-        if (opts && _.has(opts, 'validate') && opts.validate === false) {
-            return;
-        }
-
-        // use the base toJSON since this model's overridden toJSON
-        // removes fields and we want everything to run through the validator.
-        userData = ghostBookshelf.Model.prototype.toJSON.call(this);
-
-        return validation.validateSchema(this.tableName, userData);
-    },
-
     toJSON: function toJSON(unfilteredOptions) {
         var options = User.filterOptions(unfilteredOptions, 'toJSON'),
             attrs = ghostBookshelf.Model.prototype.toJSON.call(this, options);
@@ -658,50 +654,41 @@ User = ghostBookshelf.Model.extend({
     check: function check(object) {
         var self = this;
 
-        return this.getByEmail(object.email).then(function then(user) {
-            if (!user) {
-                return Promise.reject(new common.errors.NotFoundError({
-                    message: common.i18n.t('errors.models.user.noUserWithEnteredEmailAddr')
-                }));
-            }
+        return this.getByEmail(object.email)
+            .then(function then(user) {
+                if (!user) {
+                    throw new common.errors.NotFoundError({
+                        message: common.i18n.t('errors.models.user.noUserWithEnteredEmailAddr')
+                    });
+                }
 
-            if (user.isLocked()) {
-                return Promise.reject(new common.errors.NoPermissionError({
-                    message: common.i18n.t('errors.models.user.accountLocked')
-                }));
-            }
+                if (user.isLocked()) {
+                    throw new common.errors.NoPermissionError({
+                        message: common.i18n.t('errors.models.user.accountLocked')
+                    });
+                }
 
-            if (user.isInactive()) {
-                return Promise.reject(new common.errors.NoPermissionError({
-                    message: common.i18n.t('errors.models.user.accountSuspended')
-                }));
-            }
+                if (user.isInactive()) {
+                    throw new common.errors.NoPermissionError({
+                        message: common.i18n.t('errors.models.user.accountSuspended')
+                    });
+                }
 
-            return self.isPasswordCorrect({plainPassword: object.password, hashedPassword: user.get('password')})
-                .then(function then() {
-                    return Promise.resolve(user.set({status: 'active', last_seen: new Date()}).save({validate: false}))
-                        .catch(function handleError(err) {
-                            // If we get a validation or other error during this save, catch it and log it, but don't
-                            // cause a login error because of it. The user validation is not important here.
-                            common.logging.error(new common.errors.GhostError({
-                                err: err,
-                                context: common.i18n.t('errors.models.user.userUpdateError.context'),
-                                help: common.i18n.t('errors.models.user.userUpdateError.help')
-                            }));
-
-                            return user;
-                        });
-                })
-                .catch(function onError(err) {
-                    return Promise.reject(err);
-                });
-        }, function handleError(error) {
-            if (error.message === 'NotFound' || error.message === 'EmptyResponse') {
-                return Promise.reject(new common.errors.NotFoundError({message: common.i18n.t('errors.models.user.noUserWithEnteredEmailAddr')}));
-            }
+                return self.isPasswordCorrect({plainPassword: object.password, hashedPassword: user.get('password')})
+                    .then(function then() {
+                        user.set({status: 'active', last_seen: new Date()});
+                        return user.save();
+                    });
+            })
+            .catch(function (err) {
+                if (err.message === 'NotFound' || err.message === 'EmptyResponse') {
+                    throw new common.errors.NotFoundError({
+                        message: common.i18n.t('errors.models.user.noUserWithEnteredEmailAddr')
+                    });
+                }
 
-            return Promise.reject(error);
-        });
+                throw err;
+            });
     },
 
     isPasswordCorrect: function isPasswordCorrect(object) {

core/test/integration/data/importer/importers/data_spec.js

@@ -69,6 +69,23 @@ describe('Import', function () {
             }).catch(done);
         });
 
+        it('can import user with missing allowed fields', function (done) {
+            var exportData;
+
+            testUtils.fixtures.loadExportFixture('export-003-missing-allowed-fields', {lts: true}).then(function (exported) {
+                exportData = exported;
+                return dataImporter.doImport(exportData, importOptions);
+            }).then(function (importResult) {
+                importResult.data.users[0].hasOwnProperty('website');
+                importResult.data.users[0].hasOwnProperty('bio');
+                importResult.data.users[0].hasOwnProperty('accessibility');
+                importResult.data.users[0].hasOwnProperty('cover_image');
+                importResult.problems.length.should.eql(1);
+
+                done();
+            }).catch(done);
+        });
+
         it('removes duplicate tags and updates associations', function (done) {
             var exportData;
 
@@ -488,14 +505,11 @@ describe('Import', function () {
             }).then(function () {
                 done(new Error('Allowed import of invalid post data'));
             }).catch(function (response) {
-                response.length.should.equal(2, response);
+                response.length.should.equal(1, response);
 
                 response[0].errorType.should.equal('ValidationError');
                 response[0].message.should.eql('Value in [posts.title] cannot be blank.');
 
-                response[1].errorType.should.equal('ValidationError');
-                response[1].message.should.eql('Value in [posts.status] cannot be blank.');
-
                 done();
             }).catch(done);
         });