Skip to content
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

add soft validation for page put #688

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 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: 2 additions & 0 deletions lib/bootstrap.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const _ = require('lodash'),
filename = __filename.split('/').pop().split('.').shift(),
lib = require('./bootstrap'),
siteService = require('./services/sites'),
validationService = require('./services/validation'),
expect = require('chai').expect,
sinon = require('sinon'),
storage = require('../test/fixtures/mocks/storage');
Expand Down Expand Up @@ -52,6 +53,7 @@ describe(_.startCase(filename), function () {
lib.setLog(fakeLog);
sandbox.stub(siteService);
siteService.sites.returns(_.cloneDeep(sitesFake));
sandbox.stub(validationService);
db = storage();
lib.setDb(db);
db.batch.callsFake(db.batchToInMem); // we want to make sure to send to the actual in-mem batch
Expand Down
8 changes: 8 additions & 0 deletions lib/services/components.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const _ = require('lodash'),
files = require('../files'),
models = require('./models'),
dbOps = require('./db-operations'),
validation = require('./validation'),
{ getComponentName, replaceVersion } = require('clayutils');

/**
Expand Down Expand Up @@ -42,6 +43,8 @@ function put(uri, data, locals) {
callHooks = _.get(locals, 'hooks') !== 'false',
result;

validation.validateComponent(uri, data, locals);

if (model && _.isFunction(model.save) && callHooks) {
result = models.put(model, uri, data, locals);
} else {
Expand All @@ -60,6 +63,8 @@ function put(uri, data, locals) {
*/
function publish(uri, data, locals) {
if (data && _.size(data) > 0) {
validation.validateComponent(uri, data, locals);

return dbOps.cascadingPut(put)(uri, data, locals);
}

Expand All @@ -76,6 +81,9 @@ function publish(uri, data, locals) {
*/
function post(uri, data, locals) {
uri += '/' + uid.get();

validation.validateComponent(uri, data, locals);

return dbOps.cascadingPut(put)(uri, data, locals)
.then(result => {
result._ref = uri;
Expand Down
2 changes: 2 additions & 0 deletions lib/services/components.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const _ = require('lodash'),
sinon = require('sinon'),
files = require('../files'),
siteService = require('./sites'),
validationService = require('./validation'),
composer = require('./composer'),
models = require('./models'),
dbOps = require('./db-operations'),
Expand All @@ -19,6 +20,7 @@ describe(_.startCase(filename), function () {

sandbox.stub(composer, 'resolveComponentReferences');
sandbox.stub(siteService);
sandbox.stub(validationService);
sandbox.stub(files, 'getComponentModule');
sandbox.stub(models, 'put');
sandbox.stub(models, 'get');
Expand Down
7 changes: 3 additions & 4 deletions lib/services/pages.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const _ = require('lodash'),
{ getComponentName, replaceVersion, getPrefix, isLayout } = require('clayutils'),
publishService = require('./publish'),
bus = require('./bus'),
validation = require('./validation'),
timeoutPublishCoefficient = 5;

/**
Expand Down Expand Up @@ -334,10 +335,8 @@ function create(uri, data, locals) {
function putLatest(uri, data, locals) {
const user = locals && locals.user;

// check the page for a proper layout
if (!data.layout || !isLayout(data.layout)) {
throw Error('Page must contain a `layout` property whose value is a `_layouts` instance');
}
// should we promisify and add to chain?
validation.validatePage(uri, data, locals);

// continue saving the page normally
return db.getLatestData(uri)
Expand Down
6 changes: 4 additions & 2 deletions lib/services/pages.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const _ = require('lodash'),
notifications = require('./notifications'),
sinon = require('sinon'),
siteService = require('./sites'),
validationService = require('./validation'),
timer = require('../timer'),
meta = require('./metadata'),
schema = require('../schema'),
Expand All @@ -31,6 +32,7 @@ describe(_.startCase(filename), function () {
sandbox.stub(components, 'get');
sandbox.stub(layouts, 'get');
sandbox.stub(siteService, 'getSiteFromPrefix');
sandbox.stub(validationService);
sandbox.stub(notifications, 'notify');
sandbox.stub(dbOps);
sandbox.stub(timer);
Expand Down Expand Up @@ -375,7 +377,7 @@ describe(_.startCase(filename), function () {
db.getLatestData.returns(Promise.resolve({}));
db.put.returns(Promise.resolve());

return fn('domain.com/path/pages', {layout: 'domain.com/path/_layouts/thing'})
return fn('domain.com/path/_pages', {layout: 'domain.com/path/_layouts/thing'})
.then(() => {
sinon.assert.notCalled(bus.publish);
});
Expand All @@ -385,7 +387,7 @@ describe(_.startCase(filename), function () {
db.getLatestData.returns(Promise.reject());
db.put.returns(Promise.resolve());

return fn('domain.com/path/pages', {layout: 'domain.com/path/_layouts/thing'}).then(function () {
return fn('domain.com/path/_pages', {layout: 'domain.com/path/_layouts/thing'}).then(function () {
sinon.assert.calledOnce(bus.publish);
expect(bus.publish.getCall(0).args[0]).to.equal('createPage');
});
Expand Down
96 changes: 96 additions & 0 deletions lib/services/validation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
'use strict';

const sites = require('./sites'),
references = require('./references'),
{ getPrefix, isPage, isComponent, isLayout } = require('clayutils');

/**
* gets the site's slug identifier from a site obj
* @param {Object} site
* @returns {string}
*/
function getSiteSlug(site) {
return site && (site.subsiteSlug || site.slug);
}

/**
* soft validation to make sure a uri is for the current site
* @param {string} uri
* @param {Object} locals
* @throws {Error}
*/
function validateSite(uri, locals) {
const uriSite = sites.getSiteFromPrefix(getPrefix(uri)),
site = locals && locals.site,
uriSlug = getSiteSlug(uriSite),
siteSlug = getSiteSlug(site);

if (!uriSite) {
throw new Error(`Site for URI not found, ${uri}`);
}

if (!site) {
throw new Error('Site not found on locals.');
}

if (uriSlug !== siteSlug) {
throw new Error(`URI Site (${uriSlug}) not the same as current site (${siteSlug})`);
}
}

/**
* soft validation for a component to make sure all references are for the same, current site.
* @param {string} uri
* @param {Object} data
* @param {Object} locals
*/
function validateComponent(uri, data, locals) {
// uri is from site
validateSite(uri, locals);

// make sure all _refs are for the same current site
if (data) {
const refs = references.listDeepObjects(data, '_ref');

for (const ref of refs) {
validateComponent(ref._ref, ref, locals);
}
}
}

/**
* soft validation for a page to make sure all references are for the same, current site.
* @param {string} uri
* @param {Object} data
* @param {Object} locals
*/
function validatePage(uri, data, locals) {
const layout = data && data.layout,
componentList = references.getPageReferences(data);

if (!uri || !isPage(uri)) {
throw new Error(`Page URI invalid, '${uri}'`);
}

// page is for this site
validateSite(uri, locals);

if (!layout || !isLayout(layout)) {
throw new Error('Page must contain a `layout` property whose value is a `_layouts` instance');
}

// check to make sure it's from this site
validateSite(layout, locals);

for (let i = 0; i < componentList.length; i++) {
if (!isComponent(componentList[i])) {
throw new Error(`Page references a non-valid component: ${componentList[i]}`);
}

validateComponent(componentList[i], null, locals);
}
}

module.exports.validateSite = validateSite;
module.exports.validateComponent = validateComponent;
module.exports.validatePage = validatePage;
152 changes: 152 additions & 0 deletions lib/services/validation.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
'use strict';

const _ = require('lodash'),
filename = __filename.split('/').pop().split('.').shift(),
lib = require('./' + filename),
sinon = require('sinon'),
siteService = require('./sites'),
expect = require('chai').expect,
locals = {
site: {
slug: 'domain'
}
};

describe(_.startCase(filename), function () {
let sandbox;

beforeEach(function () {
sandbox = sinon.sandbox.create();

sandbox.stub(siteService, 'getSiteFromPrefix').callsFake(function fake(prefix) {
const sites = {
'domain.com': locals.site,
'other.com': { slug: 'notit' }
};

return sites[prefix];
});
});

afterEach(function () {
sandbox.restore();
});

describe('validateSite', function () {
const fn = lib.validateSite;

it('will not throw if no error', function () {
const uri = 'domain.com/_pages/foo';

fn(uri, locals);
sinon.assert.calledOnce(siteService.getSiteFromPrefix);
});

it('will throw if site does not exist for uri', function () {
const uri = 'idontexist.com/_pages/foo';

try {
fn(uri, locals);
} catch (e) {
expect(e.message).to.eql(`Site for URI not found, ${uri}`);
}
});

it('will throw if locals does not have a site', function () {
const uri = 'domain.com/_pages/foo';

try {
fn(uri, {});
} catch (e) {
expect(e.message).to.eql('Site not found on locals.');
}
});

it('will throw if uri different site than locals', function () {
const uri = 'other.com/_pages/foo';

try {
fn(uri, locals);
} catch (e) {
expect(e.message).to.eql('URI Site (notit) not the same as current site (domain)');
}
});
});

describe('validatePage', function () {
const fn = lib.validatePage;

it('will not throw if no error', function () {
const uri = 'domain.com/_pages/foo',
data = {
head: [
'domain.com/_components/header/instances/foo'
],
main:[
'domain.com/_components/article/instances/foo'
],
layout: 'domain.com/_layouts/layout/instances/article'
};

fn(uri, data, locals);
sinon.assert.callCount(siteService.getSiteFromPrefix, 4);
});

it('will throw error if page URI invalid', function () {
const uri = 'domain.com/im/not/a/page/uri',
data = {
head: [
'domain.com/_components/header/instances/foo'
],
main:[
'domain.com/_components/article/instances/foo'
],
layout: 'domain.com/_layouts/layout/instances/article'
};

try {
fn(uri, data, locals);
} catch (e) {
expect(e.message).to.eql(`Page URI invalid, '${uri}'`);
}
});

it('will throw error if layout URI invalid', function () {
const uri = 'domain.com/_pages/foo',
data = {
head: [
'domain.com/_components/header/instances/foo'
],
main:[
'domain.com/_components/article/instances/foo'
],
layout: 'domain.com/im/not/a/layout'
};

try {
fn(uri, data, locals);
} catch (e) {
expect(e.message).to.eql('Page must contain a `layout` property whose value is a `_layouts` instance');
}
});

it('will not throw if no error', function () {
const uri = 'domain.com/_pages/foo',
data = {
head: [
'domain.com/_components/header/instances/foo'
],
main:[
'domain.com/im/not/a/component'
],
layout: 'domain.com/_layouts/layout/instances/article'
};

try {
fn(uri, data, locals);
} catch (e) {
expect(e.message).to.eql('Page references a non-valid component: domain.com/im/not/a/component');
}
});
});
});