diff --git a/core/server/api/settings.js b/core/server/api/settings.js index 97132a3bfe..7aa5bd24bb 100644 --- a/core/server/api/settings.js +++ b/core/server/api/settings.js @@ -66,6 +66,7 @@ updateConfigCache = function () { config.set('theme:facebook', (settingsCache.facebook && settingsCache.facebook.value) || ''); config.set('theme:timezone', (settingsCache.activeTimezone && settingsCache.activeTimezone.value) || config.get('theme').timezone); config.set('theme:url', config.get('url') ? generalUtils.url.urlJoin(config.get('url'), '/') : ''); + config.set('theme:amp', (settingsCache.amp && settingsCache.amp.value === 'true')); _.each(labsValue, function (value, key) { config.set('labs:' + key, value); diff --git a/core/server/apps/amp/index.js b/core/server/apps/amp/index.js index f5aedd6308..6bc8e3e094 100644 --- a/core/server/apps/amp/index.js +++ b/core/server/apps/amp/index.js @@ -1,8 +1,6 @@ var router = require('./lib/router'), registerHelpers = require('./lib/helpers'), - - // Dirty requires - config = require('../../config'); + config = require('../../config'); module.exports = { activate: function activate(ghost) { diff --git a/core/server/apps/amp/lib/router.js b/core/server/apps/amp/lib/router.js index 095741c103..72b93ebd6f 100644 --- a/core/server/apps/amp/lib/router.js +++ b/core/server/apps/amp/lib/router.js @@ -2,8 +2,10 @@ var path = require('path'), express = require('express'), _ = require('lodash'), ampRouter = express.Router(), + i18n = require('../../../i18n'), // Dirty requires + config = require('../../../config'), errors = require('../../../errors'), templates = require('../../../controllers/frontend/templates'), postLookup = require('../../../controllers/frontend/post-lookup'), @@ -12,7 +14,7 @@ var path = require('path'), function controller(req, res, next) { var defaultView = path.resolve(__dirname, 'views', 'amp.hbs'), paths = templates.getActiveThemePaths(req.app.get('activeTheme')), - data = req.amp; + data = req.body || {}; if (res.error) { data.error = res.error; @@ -33,32 +35,53 @@ function controller(req, res, next) { } function getPostData(req, res, next) { - // Create a req property where we can store our data - req.amp = {}; + req.body = req.body || {}; postLookup(res.locals.relativeUrl) .then(function (result) { if (result && result.post) { - req.amp.post = result.post; + req.body.post = result.post; } next(); }) .catch(function (err) { - if (err instanceof errors.NotFoundError) { - return next(err); - } - next(err); }); } +function checkIfAMPIsEnabled(req, res, next) { + var ampIsEnabled = config.get('theme:amp'); + + if (ampIsEnabled) { + return next(); + } + + // CASE: we don't support amp pages for static pages + if (req.body.post && req.body.post.page) { + return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')})); + } + + /** + * CASE: amp is disabled, we serve 404 + * + * Alternatively we could redirect to the original post, as the user can enable/disable AMP every time. + * + * If we would call `next()`, express jumps to the frontend controller (server/controllers/frontend/index.js fn single) + * and tries to lookup the post (again) and checks whether the post url equals the requested url (post.url !== req.path). + * This check would fail if the blog is setup on a subdirectory. + */ + return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')})); +} + // AMP frontend route ampRouter.route('/') .get( getPostData, + checkIfAMPIsEnabled, controller ); module.exports = ampRouter; module.exports.controller = controller; module.exports.getPostData = getPostData; +module.exports.checkIfAMPIsEnabled = checkIfAMPIsEnabled; diff --git a/core/server/apps/amp/tests/router_spec.js b/core/server/apps/amp/tests/router_spec.js index 5902332d23..affdf1938b 100644 --- a/core/server/apps/amp/tests/router_spec.js +++ b/core/server/apps/amp/tests/router_spec.js @@ -44,7 +44,8 @@ describe('AMP Controller', function () { configUtils.set({ theme: { - permalinks: '/:slug/' + permalinks: '/:slug/', + amp: true } }); }); @@ -165,18 +166,16 @@ describe('AMP getPostData', function () { postLookupStub.returns(new Promise.resolve({ post: { id: '1', - slug: 'welcome-to-ghost', - isAmpURL: true + slug: 'welcome-to-ghost' } })); ampController.__set__('postLookup', postLookupStub); ampController.getPostData(req, res, function () { - req.amp.post.should.be.eql({ + req.body.post.should.be.eql({ id: '1', - slug: 'welcome-to-ghost', - isAmpURL: true + slug: 'welcome-to-ghost' } ); done(); @@ -197,7 +196,7 @@ describe('AMP getPostData', function () { err.message.should.be.eql('not found'); err.statusCode.should.be.eql(404); err.errorType.should.be.eql('NotFoundError'); - req.amp.should.be.eql({}); + req.body.should.be.eql({}); done(); }); }); @@ -210,7 +209,7 @@ describe('AMP getPostData', function () { ampController.getPostData(req, res, function (err) { should.exist(err); err.should.be.eql('not found'); - req.amp.should.be.eql({}); + req.body.should.be.eql({}); done(); }); }); diff --git a/core/server/controllers/frontend/index.js b/core/server/controllers/frontend/index.js index 34dcdd2d53..c80471b2f3 100644 --- a/core/server/controllers/frontend/index.js +++ b/core/server/controllers/frontend/index.js @@ -69,6 +69,11 @@ frontendControllers = { return next(); } + // CASE: postlookup can detect options for example /edit, unknown options get ignored and end in 404 + if (lookup.isUnknownOption) { + return next(); + } + // CASE: last param is of url is /edit, redirect to admin if (lookup.isEditURL) { return res.redirect(utils.url.urlJoin(utils.url.getSubdir(), '/ghost/editor', post.id, '/')); diff --git a/core/server/controllers/frontend/post-lookup.js b/core/server/controllers/frontend/post-lookup.js index ffca9b9f7f..dcfec10def 100644 --- a/core/server/controllers/frontend/post-lookup.js +++ b/core/server/controllers/frontend/post-lookup.js @@ -16,7 +16,6 @@ function postLookup(postUrl) { postPermalink = config.get('theme').permalinks, pagePermalink = '/:slug/', isEditURL = false, - isAmpURL = false, matchFuncPost, matchFuncPage, postParams, @@ -38,23 +37,12 @@ function postLookup(postUrl) { } // If params contains options, and it is equal to 'edit', this is an edit URL - // If params contains options, and it is equal to 'amp', this is an amp URL if (params.options && params.options.toLowerCase() === 'edit') { isEditURL = true; - } else if (params.options && params.options.toLowerCase() === 'amp') { - isAmpURL = true; - } else if (params.options !== undefined) { - // Unknown string in URL, return empty - return Promise.resolve(); } - // Sanitize params we're going to use to lookup the post. - params = _.pick(params, 'slug', 'id'); - // Add author & tag - params.include = 'author,tags'; - // Query database to find post - return api.posts.read(params).then(function then(result) { + return api.posts.read(_.extend(_.pick(params, 'slug', 'id'), {include: 'author,tags'})).then(function then(result) { var post = result.posts[0]; if (!post) { @@ -71,15 +59,10 @@ function postLookup(postUrl) { return Promise.resolve(); } - // We don't support AMP for static pages yet - if (post.page && isAmpURL) { - return Promise.resolve(); - } - return { post: post, isEditURL: isEditURL, - isAmpURL: isAmpURL + isUnknownOption: isEditURL ? false : params.options ? true : false }; }); } diff --git a/core/server/data/schema/default-settings.json b/core/server/data/schema/default-settings.json index 4b882d993f..648cebae61 100644 --- a/core/server/data/schema/default-settings.json +++ b/core/server/data/schema/default-settings.json @@ -65,6 +65,9 @@ "notContains": "/ghost/" } }, + "amp": { + "defaultValue" : "true" + }, "ghost_head": { "defaultValue" : "" }, diff --git a/core/server/helpers/ghost_head.js b/core/server/helpers/ghost_head.js index 67beb29e66..409df617b5 100644 --- a/core/server/helpers/ghost_head.js +++ b/core/server/helpers/ghost_head.js @@ -98,7 +98,8 @@ function ghost_head(options) { escapeExpression(metaData.canonicalUrl) + '" />'); head.push(''); - if (_.includes(context, 'post') && !_.includes(context, 'amp')) { + // show amp link in post when 1. we are not on the amp page and 2. amp is enabled + if (_.includes(context, 'post') && !_.includes(context, 'amp') && config.get('theme:amp')) { head.push(''); } diff --git a/core/test/functional/routes/frontend_spec.js b/core/test/functional/routes/frontend_spec.js index 1a6801a3cf..8272698ea6 100644 --- a/core/test/functional/routes/frontend_spec.js +++ b/core/test/functional/routes/frontend_spec.js @@ -8,6 +8,7 @@ var request = require('supertest'), moment = require('moment'), cheerio = require('cheerio'), testUtils = require('../../utils'), + configUtils = require('../../utils/configUtils'), ghost = testUtils.startGhost; describe('Frontend Routing', function () { @@ -309,6 +310,19 @@ describe('Frontend Routing', function () { .expect(/Page not found/) .end(doEnd(done)); }); + + it('should not render AMP, when AMP is disabled', function (done) { + after(function () { + configUtils.restore(); + }); + + configUtils.set('theme:amp', false); + + request.get('/welcome-to-ghost/amp/') + .expect(404) + .expect(/Page not found/) + .end(doEnd(done)); + }); }); describe('Static assets', function () { @@ -534,6 +548,17 @@ describe('Frontend Routing', function () { .expect(200) .end(doEnd(done)); }); + + it('/blog/welcome-to-ghost/amp/ should 200', function (done) { + after(function () { + configUtils.restore(); + }); + + configUtils.set('theme:amp', true); + request.get('/blog/welcome-to-ghost/amp/') + .expect(200) + .end(doEnd(done)); + }); }); describe('Subdirectory (with slash)', function () { @@ -612,6 +637,11 @@ describe('Frontend Routing', function () { }); it('/blog/welcome-to-ghost/amp/ should 200', function (done) { + after(function () { + configUtils.restore(); + }); + + configUtils.set('theme:amp', true); request.get('/blog/welcome-to-ghost/amp/') .expect(200) .end(doEnd(done)); diff --git a/core/test/unit/controllers/frontend/post-lookup_spec.js b/core/test/unit/controllers/frontend/post-lookup_spec.js index 96c4a8d3d2..76efbb0929 100644 --- a/core/test/unit/controllers/frontend/post-lookup_spec.js +++ b/core/test/unit/controllers/frontend/post-lookup_spec.js @@ -40,7 +40,6 @@ describe('postLookup', function () { should.exist(lookup.post); lookup.post.should.have.property('url', '/welcome-to-ghost/'); lookup.isEditURL.should.be.false(); - lookup.isAmpURL.should.be.false(); done(); }).catch(done); @@ -54,7 +53,6 @@ describe('postLookup', function () { should.exist(lookup.post); lookup.post.should.have.property('url', '/welcome-to-ghost/'); lookup.isEditURL.should.be.false(); - lookup.isAmpURL.should.be.false(); done(); }).catch(done); @@ -116,7 +114,6 @@ describe('postLookup', function () { should.exist(lookup.post); lookup.post.should.have.property('url', '/2016/01/01/welcome-to-ghost/'); lookup.isEditURL.should.be.false(); - lookup.isAmpURL.should.be.false(); done(); }).catch(done); @@ -130,7 +127,6 @@ describe('postLookup', function () { should.exist(lookup.post); lookup.post.should.have.property('url', '/2016/01/01/welcome-to-ghost/'); lookup.isEditURL.should.be.false(); - lookup.isAmpURL.should.be.false(); done(); }).catch(done); @@ -154,7 +150,6 @@ describe('postLookup', function () { postLookup(testUrl).then(function (lookup) { lookup.post.should.have.property('url', '/welcome-to-ghost/'); lookup.isEditURL.should.be.true(); - lookup.isAmpURL.should.be.false(); done(); }).catch(done); }); @@ -165,7 +160,6 @@ describe('postLookup', function () { postLookup(testUrl).then(function (lookup) { lookup.post.should.have.property('url', '/welcome-to-ghost/'); lookup.isEditURL.should.be.true(); - lookup.isAmpURL.should.be.false(); done(); }).catch(done); }); @@ -192,7 +186,8 @@ describe('postLookup', function () { var testUrl = '/welcome-to-ghost/notedit/'; postLookup(testUrl).then(function (lookup) { - should.not.exist(lookup); + lookup.post.should.have.property('url', '/welcome-to-ghost/'); + lookup.isUnknownOption.should.eql(true); done(); }).catch(done); }); @@ -213,7 +208,6 @@ describe('postLookup', function () { postLookup(testUrl).then(function (lookup) { lookup.post.should.have.property('url', '/welcome-to-ghost/'); - lookup.isAmpURL.should.be.true(); lookup.isEditURL.should.be.false(); done(); }).catch(done); @@ -224,7 +218,6 @@ describe('postLookup', function () { postLookup(testUrl).then(function (lookup) { lookup.post.should.have.property('url', '/welcome-to-ghost/'); - lookup.isAmpURL.should.be.true(); lookup.isEditURL.should.be.false(); done(); }).catch(done); @@ -247,14 +240,5 @@ describe('postLookup', function () { done(); }).catch(done); }); - - it('cannot lookup relative url: /:slug/notamp/', function (done) { - var testUrl = '/welcome-to-ghost/notamp/'; - - postLookup(testUrl).then(function (lookup) { - should.not.exist(lookup); - done(); - }).catch(done); - }); }); }); diff --git a/core/test/unit/server_helpers/ghost_head_spec.js b/core/test/unit/server_helpers/ghost_head_spec.js index bbfef9ed03..8847e80370 100644 --- a/core/test/unit/server_helpers/ghost_head_spec.js +++ b/core/test/unit/server_helpers/ghost_head_spec.js @@ -52,7 +52,8 @@ describe('{{ghost_head}} helper', function () { theme: { title: 'Ghost', description: 'blog description', - cover: '/content/images/blog-cover.png' + cover: '/content/images/blog-cover.png', + amp: true } }); }); @@ -866,7 +867,8 @@ describe('{{ghost_head}} helper', function () { theme: { title: 'Ghost', description: 'blog description', - cover: '/content/images/blog-cover.png' + cover: '/content/images/blog-cover.png', + amp: true } }); }); @@ -894,7 +896,8 @@ describe('{{ghost_head}} helper', function () { theme: { title: 'Ghost', description: 'blog description', - cover: '/content/images/blog-cover.png' + cover: '/content/images/blog-cover.png', + amp: true }, referrerPolicy: 'origin' }); @@ -920,7 +923,8 @@ describe('{{ghost_head}} helper', function () { theme: { title: 'Ghost', description: 'blog description', - cover: '/content/images/blog-cover.png' + cover: '/content/images/blog-cover.png', + amp: true }, privacy: { useStructuredData: false @@ -1091,4 +1095,44 @@ describe('{{ghost_head}} helper', function () { }); }); }); + + describe('amp is disabled', function () { + beforeEach(function () { + configUtils.set({ + url: 'http://testurl.com/', + theme: { + amp: false + } + }); + }); + + it('does not contain amphtml link', function (done) { + var post = { + meta_description: 'blog description', + title: 'Welcome to Ghost', + image: 'content/images/test-image.png', + published_at: moment('2008-05-31T19:18:15').toISOString(), + updated_at: moment('2014-10-06T15:23:54').toISOString(), + tags: [{name: 'tag1'}, {name: 'tag2'}, {name: 'tag3'}], + author: { + name: 'Author name', + url: 'http//:testauthorurl.com', + slug: 'Author', + image: 'content/images/test-author-image.png', + website: 'http://authorwebsite.com', + facebook: 'testuser', + twitter: '@testuser' + } + }; + + helpers.ghost_head.call( + {relativeUrl: '/post/', safeVersion: '0.3', context: ['post'], post: post}, + {data: {root: {context: ['post']}}} + ).then(function (rendered) { + should.exist(rendered); + rendered.string.should.not.match(/