Added visible theme errors in admin (#16081)

refs https://github.com/TryGhost/Team/issues/2393

- During boot and loading the active theme, we now cache the result of
the gscan validation. Cache configuration can happen in
`adapters.cache.gscan`
- We now also return non-fatal errors when activating or adding a theme.
- When the `themeErrorsNotification` feature flag is on, we fetch the
active theme (which includes the validation information) when loading
admin
- If the currently active theme has errors, we show an error
notification that can open the error modal
- Added a new endpoint: `/ghost/api/admin/themes/active/` that returns
the result of the last gscan validation of the active theme. If no cache
is available, it will run a new gscan validation.
- Added new permissions for the active action/endpoint (author, editor,
administrator)
This commit is contained in:
Simon Backx 2023-01-06 13:44:27 +01:00 committed by GitHub
parent a37bd19f74
commit 7b3712a15b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
23 changed files with 277 additions and 82 deletions

View File

@ -9,4 +9,13 @@ export default class Theme extends ApplicationAdapter {
return model;
});
}
active() {
let url = `${this.buildURL('theme', 'active')}`;
return this.ajax(url, 'GET', {data: {}}).then((data) => {
this.store.pushPayload('theme', data);
return this.store.peekAll('theme').filterBy('name', data.themes[0].name).firstObject;
});
}
}

View File

@ -1,5 +1,5 @@
<div class="gh-nav-bottom">
{{#if (feature 'themeErrorsNotification')}}
{{#if (and (feature 'themeErrorsNotification') this.hasThemeErrors)}}
<button type="button" class="gh-theme-error-toast" {{on "click" (fn this.openThemeErrors null)}}>
<span class="gh-notification-title">Your theme contains errors</span>
<span class="gh-theme-error-p">Some functionality on your site may be limited &rarr;</span>

View File

@ -1,5 +1,5 @@
import AboutModal from '../modals/theme-errors';
import Component from '@ember/component';
import ThemeErrorsModal from '../modals/design/theme-errors';
import calculatePosition from 'ember-basic-dropdown/utils/calculate-position';
import classic from 'ember-classic-decorator';
import {action} from '@ember/object';
@ -14,6 +14,7 @@ export default class Footer extends Component {
@service whatsNew;
@service feature;
@service modals;
@service themeManagement;
@inject config;
@ -25,7 +26,17 @@ export default class Footer extends Component {
@action
openThemeErrors() {
this.advancedModal = this.modals.open(AboutModal);
this.advancedModal = this.modals.open(ThemeErrorsModal, {
title: 'Theme errors',
canActivate: false,
// Warnings will only be set for developers, otherwise it will always be empty
warnings: this.themeManagement.activeTheme.warnings,
errors: this.themeManagement.activeTheme.errors
});
}
get hasThemeErrors() {
return this.themeManagement.activeTheme && this.themeManagement.activeTheme.errors.length;
}
// equivalent to "left: auto; right: -20px"

View File

@ -1,22 +0,0 @@
<div class="modal-content gh-about-modal">
<header class="modal-header">
<h1>Theme errors</h1>
</header>
<button type="button" class="close" title="Close" {{on "click" @close}}>{{svg-jar "close"}}<span class="hidden">Close</span></button>
<div class="modal-body">
<div>
<h2 class="mb0 mt4 f5 fw6">Fatal Errors</h2>
<p class="mb2">Must-fix to activate theme</p>
</div>
<div>[fatal errors placeholder]</div>
<div>
<h2 class="mb0 mt4 f5 fw6">Errors</h2>
<p class="mb2">Highly recommended to fix, functionality <strong>could</strong> be restricted</p>
</div>
<div>[errors placeholder]</div>
</div>
</div>

View File

@ -1,7 +0,0 @@
import Component from '@glimmer/component';
export default class ThemeErrorsModal extends Component {
constructor() {
super(...arguments);
}
}

View File

@ -4,11 +4,11 @@ import {isBlank} from '@ember/utils';
export default Model.extend({
active: attr('boolean'),
errors: attr('raw'),
errors: attr('raw', {defaultValue: () => []}),
name: attr('string'),
package: attr('raw'),
templates: attr('raw', {defaultValue: () => []}),
warnings: attr('raw'),
warnings: attr('raw', {defaultValue: () => []}),
customTemplates: computed('templates.[]', function () {
let templates = this.templates || [];

View File

@ -20,6 +20,7 @@ export default class SessionService extends ESASessionService {
@service upgradeStatus;
@service whatsNew;
@service membersUtils;
@service themeManagement;
@inject config;
@ -45,6 +46,9 @@ export default class SessionService extends ESASessionService {
this.membersUtils.fetch()
]);
// Theme management requires features to be loaded
this.themeManagement.fetch().catch(console.error); // eslint-disable-line no-console
await this.frontend.loginIfNeeded();
// update Sentry with the full Ghost version which we only get after authentication

View File

@ -15,6 +15,8 @@ export default class ThemeManagementService extends Service {
@service settings;
@service store;
@service frontend;
@service session;
@service feature;
@inject config;
@ -22,6 +24,11 @@ export default class ThemeManagementService extends Service {
@tracked previewType = 'homepage';
@tracked previewHtml;
/**
* Contains the active theme object (includes warnings and errors)
*/
@tracked activeTheme;
allPosts = this.store.peekAll('post');
availablePreviewTypes = [{
@ -38,6 +45,20 @@ export default class ThemeManagementService extends Service {
}).lastObject;
}
async fetch() {
// contributors don't have permissions to fetch active theme
if (this.session.user && !this.session.user.isContributor && this.feature.get('themeErrorsNotification')) {
try {
const adapter = this.store.adapterFor('theme');
const activeTheme = await adapter.active();
this.activeTheme = activeTheme;
} catch (e) {
// We ignore these errors and log them because we don't want to block loading the app for this
console.error('Failed to fetch active theme', e); // eslint-disable-line no-console
}
}
}
@action
setPreviewType(type) {
if (type !== this.previewType) {
@ -90,6 +111,7 @@ export default class ThemeManagementService extends Service {
try {
const activatedTheme = yield theme.activate();
this.activeTheme = activatedTheme;
yield this.customThemeSettings.reload();

View File

@ -4,6 +4,7 @@ const models = require('../../models');
// Used to emit theme.uploaded which is used in core/server/analytics-events
const events = require('../../lib/common/events');
const {settingsCache} = require('../../services/settings-helpers');
module.exports = {
docName: 'themes',
@ -15,6 +16,15 @@ module.exports = {
}
},
readActive: {
permissions: true,
async query() {
let themeName = settingsCache.get('active_theme');
const themeErrors = await themeService.api.getThemeErrors(themeName);
return themeService.api.getJSON(themeName, themeErrors);
}
},
activate: {
headers: {
cacheInvalidate: true
@ -42,15 +52,9 @@ module.exports = {
value: themeName
}];
return themeService.api.activate(themeName)
.then((checkedTheme) => {
// @NOTE: we use the model, not the API here, as we don't want to trigger permissions
return models.Settings.edit(newSettings, frame.options)
.then(() => checkedTheme);
})
.then((checkedTheme) => {
return themeService.api.getJSON(themeName, checkedTheme);
});
const themeErrors = await themeService.api.activate(themeName);
await models.Settings.edit(newSettings, frame.options);
return themeService.api.getJSON(themeName, themeErrors);
}
},

View File

@ -0,0 +1,12 @@
const {addPermissionWithRoles} = require('../../utils');
module.exports = addPermissionWithRoles({
name: 'View active theme details',
action: 'readActive',
object: 'theme'
}, [
'Author',
'Editor',
'Administrator',
'Admin Integration'
]);

View File

@ -214,6 +214,11 @@
"action_type": "activate",
"object_type": "theme"
},
{
"name": "View active theme details",
"action_type": "readActive",
"object_type": "theme"
},
{
"name": "Upload themes",
"action_type": "add",
@ -804,7 +809,7 @@
"user": "all",
"role": "all",
"invite": "all",
"theme": ["browse"],
"theme": ["browse", "readActive"],
"email_preview": "all",
"email": "all",
"snippet": "all",
@ -819,7 +824,7 @@
"tag": ["browse", "read", "add"],
"user": ["browse", "read"],
"role": ["browse"],
"theme": ["browse"],
"theme": ["browse", "readActive"],
"email_preview": "read",
"email": "read",
"snippet": ["browse", "read"],

View File

@ -21,7 +21,7 @@ module.exports.loadAndActivate = async (themeName) => {
const loadedTheme = await themeLoader.loadOneTheme(themeName);
// Validate
// @NOTE: this is now the only usage of check, rather than checkSafe...
const checkedTheme = await validate.check(loadedTheme);
const checkedTheme = await validate.check(themeName, loadedTheme);
if (!validate.canActivate(checkedTheme)) {
logging.error(validate.getThemeValidationError('activeThemeHasFatalErrors', themeName, checkedTheme));
@ -57,6 +57,6 @@ module.exports.activate = async (themeName) => {
const checkedTheme = await validate.checkSafe(themeName, loadedTheme);
// Activate
await activator.activateFromAPI(themeName, loadedTheme, checkedTheme);
// Return the checked theme
return checkedTheme;
// Return the theme errors
return validate.getErrorsFromCheckedTheme(checkedTheme);
};

View File

@ -3,7 +3,7 @@ const themeLoader = require('./loader');
const storage = require('./storage');
const getJSON = require('./to-json');
const installer = require('./installer');
const validate = require('./validate');
const settingsCache = require('../../../shared/settings-cache');
module.exports = {
@ -11,8 +11,9 @@ module.exports = {
* Load the currently active theme
*/
init: async () => {
const themeName = settingsCache.get('active_theme');
validate.init();
const themeName = settingsCache.get('active_theme');
return activate.loadAndActivate(themeName);
},
/**
@ -25,6 +26,7 @@ module.exports = {
api: {
getJSON,
activate: activate.activate,
getThemeErrors: validate.getThemeErrors,
getZip: storage.getZip,
setFromZip: storage.setFromZip,
installFromGithub: installer.installFromGithub,

View File

@ -86,10 +86,12 @@ module.exports = {
await activator.activateFromAPIOverride(themeName, loadedTheme, checkedTheme);
}
const themeErrors = validate.getErrorsFromCheckedTheme(checkedTheme);
// @TODO: unify the name across gscan and Ghost!
return {
themeOverridden: overrideTheme,
theme: toJSON(themeName, checkedTheme)
theme: toJSON(themeName, themeErrors)
};
} catch (error) {
// restore backup if we renamed an existing theme but saving failed

View File

@ -13,10 +13,10 @@ const settingsCache = require('../../../shared/settings-cache');
* @TODO: settingsCache.get('active_theme') vs. active.get().name
*
* @param {string} [name] - the theme to output
* @param {object} [checkedTheme] - a theme result from gscan
* @param {{errors: Array, warnings: Array}} [themeErrors] - Error and warning results from checked theme (if available)
* @return {}
*/
module.exports = function toJSON(name, checkedTheme) {
module.exports = function toJSON(name, themeErrors) {
let themeResult;
let toFilter;
@ -30,12 +30,12 @@ module.exports = function toJSON(name, checkedTheme) {
themeResult = packageJSON.filter(toFilter, settingsCache.get('active_theme'));
if (checkedTheme && checkedTheme.results.warning.length > 0) {
themeResult[0].warnings = _.cloneDeep(checkedTheme.results.warning);
if (themeErrors && themeErrors.warnings.length) {
themeResult[0].warnings = _.cloneDeep(themeErrors.warnings);
}
if (checkedTheme && checkedTheme.results.error.length > 0) {
themeResult[0].errors = _.cloneDeep(checkedTheme.results.error);
if (themeErrors && themeErrors.errors.length) {
themeResult[0].errors = _.cloneDeep(themeErrors.errors);
}
}

View File

@ -5,20 +5,46 @@ const config = require('../../../shared/config');
const labs = require('../../../shared/labs');
const tpl = require('@tryghost/tpl');
const errors = require('@tryghost/errors');
const adapterManager = require('../adapter-manager');
const logging = require('@tryghost/logging');
const list = require('./list');
const messages = {
themeHasErrors: 'Theme "{theme}" is not compatible or contains errors.',
activeThemeHasFatalErrors: 'The currently active theme "{theme}" has fatal errors.',
activeThemeHasErrors: 'The currently active theme "{theme}" has errors, but will still work.'
activeThemeHasErrors: 'The currently active theme "{theme}" has errors, but will still work.',
themeNotLoaded: 'Theme "{themeName}" is not loaded and cannot be checked.'
};
/**
* @typedef {Object} CacheStore
* @property {(key: string) => Promise<any>} get - get a value from the cache. Returns undefined if not found
* @property {(key: string, value: any) => Promise<void>} set - set a value in the cache
* @property {() => Promise<void>} reset - reset the cache
*/
/**
* The cache store for storing the result of the last theme validation
* @type {CacheStore}
*/
let gscanCacheStore;
module.exports.init = () => {
gscanCacheStore = adapterManager.getAdapter('cache:gscan');
};
const canActivate = function canActivate(checkedTheme) {
// CASE: production and no fatal errors
// CASE: development returns fatal and none fatal errors, theme is only invalid if fatal errors
return !checkedTheme.results.error.length || (config.get('env') === 'development') && !checkedTheme.results.hasFatalErrors;
return !checkedTheme.results.hasFatalErrors;
};
const check = async function check(theme, isZip) {
const getErrorsFromCheckedTheme = function getErrorsFromCheckedTheme(checkedTheme) {
return {
errors: checkedTheme.results.error ?? [],
warnings: checkedTheme.results.warning ?? []
};
};
const check = async function check(themeName, theme, isZip) {
debug('Begin: Check');
// gscan can slow down boot time if we require on boot, for now nest the require.
const gscan = require('gscan');
@ -41,16 +67,59 @@ const check = async function check(theme, isZip) {
}
checkedTheme = gscan.format(checkedTheme, {
onlyFatalErrors: config.get('env') === 'production',
onlyFatalErrors: false,
checkVersion: checkedVersion
});
// In production we don't want to show warnings
// Warnings are meant for developers only
if (config.get('env') === 'production') {
checkedTheme.results.warning = [];
}
// Cache warnings and errors
try {
await gscanCacheStore.set(themeName, getErrorsFromCheckedTheme(checkedTheme));
} catch (err) {
logging.error('Failed to cache gscan result');
logging.error(err);
}
debug('End: Check');
return checkedTheme;
};
/**
* Returns the last cached errors and warnings of check() if available.
* Otherwise runs check() on the loaded theme with that name (which will always cache the error and warning results)
* @returns {Promise<{errors: Array, warnings: Array}>}
*/
const getThemeErrors = async function getThemeErrors(themeName) {
try {
const cachedThemeErrors = await gscanCacheStore.get(themeName);
if (cachedThemeErrors) {
return cachedThemeErrors;
}
} catch (err) {
logging.error('Failed to get gscan result from cache');
logging.error(err);
}
const loadedTheme = list.get(themeName);
if (!loadedTheme) {
throw new errors.ValidationError({
message: tpl(messages.themeNotLoaded, {themeName: themeName}),
errorDetails: themeName
});
}
const result = await check(themeName, loadedTheme);
return getErrorsFromCheckedTheme(result);
};
const checkSafe = async function checkSafe(themeName, theme, isZip) {
const checkedTheme = await check(theme, isZip);
const checkedTheme = await check(themeName, theme, isZip);
if (canActivate(checkedTheme)) {
return checkedTheme;
@ -83,4 +152,6 @@ const getThemeValidationError = (message, themeName, checkedTheme) => {
module.exports.check = check;
module.exports.checkSafe = checkSafe;
module.exports.canActivate = canActivate;
module.exports.getErrorsFromCheckedTheme = getErrorsFromCheckedTheme;
module.exports.getThemeValidationError = getThemeValidationError;
module.exports.getThemeErrors = getThemeErrors;

View File

@ -159,6 +159,11 @@ module.exports = function apiRoutes() {
http(api.themes.download)
);
router.get('/themes/active',
mw.authAdminApi,
http(api.themes.readActive)
);
router.post('/themes/upload',
mw.authAdminApi,
apiMw.upload.single('file'),

View File

@ -27,7 +27,8 @@
"cache": {
"active": "Memory",
"settings": {},
"imageSizes": {}
"imageSizes": {},
"gscan": {}
}
},
"storage": {

View File

@ -87,6 +87,13 @@ describe('Themes API', function () {
.expect(200);
});
it('Can fetch active theme', async function () {
await ownerRequest
.get(localUtils.API.getApiQuery('themes/active/'))
.set('Origin', config.get('url'))
.expect(200);
});
it('Can upload a valid theme', async function () {
const res = await uploadTheme({themePath: path.join(__dirname, '..', '..', 'utils', 'fixtures', 'themes', 'valid.zip')});
const jsonResponse = res.body;
@ -264,6 +271,14 @@ describe('Themes API', function () {
localUtils.API.checkResponse(testTheme2, 'theme', ['warnings', 'templates']);
testTheme2.active.should.be.true();
testTheme2.warnings.should.be.an.Array();
// Result should be the same
const activeThemeResult = await ownerRequest
.get(localUtils.API.getApiQuery('themes/active/'))
.set('Origin', config.get('url'))
.expect(200);
res2.body.should.eql(activeThemeResult.body);
});
it('Can download and install a theme from GitHub', async function () {

View File

@ -45,7 +45,7 @@ describe('Database Migration (special functions)', function () {
const permissions = this.obj;
// If you have to change this number, please add the relevant `havePermission` checks below
permissions.length.should.eql(108);
permissions.length.should.eql(109);
permissions.should.havePermission('Export database', ['Administrator', 'DB Backup Integration']);
permissions.should.havePermission('Import database', ['Administrator', 'DB Backup Integration']);
@ -80,6 +80,7 @@ describe('Database Migration (special functions)', function () {
permissions.should.havePermission('Browse themes', ['Administrator', 'Editor', 'Author', 'Contributor', 'Admin Integration']);
permissions.should.havePermission('Edit themes', ['Administrator', 'Admin Integration']);
permissions.should.havePermission('Activate themes', ['Administrator', 'Admin Integration']);
permissions.should.havePermission('View active theme details', ['Administrator', 'Editor', 'Author', 'Admin Integration']);
permissions.should.havePermission('Upload themes', ['Administrator', 'Admin Integration']);
permissions.should.havePermission('Download themes', ['Administrator', 'Admin Integration']);
permissions.should.havePermission('Delete themes', ['Administrator', 'Admin Integration']);

View File

@ -36,7 +36,7 @@ const validateRouteSettings = require('../../../../../core/server/services/route
describe('DB version integrity', function () {
// Only these variables should need updating
const currentSchemaHash = 'a3df9e11b3db1c8afdfb87c7a206b53b';
const currentFixturesHash = 'dcb7ba7c66b4b98d6c26a722985e756a';
const currentFixturesHash = 'f0ccdb0c7eccbc3311e38b5d145ed1db';
const currentSettingsHash = '9acce72858e75420b831297718595bbd';
const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01';

View File

@ -1,10 +1,11 @@
const should = require('should');
const sinon = require('sinon');
const _ = require('lodash');
const validate = require('../../../../../core/server/services/themes/validate');
const list = require('../../../../../core/server/services/themes/list');
const gscan = require('gscan');
const assert = require('assert');
const adapterManager = require('../../../../../core/server/services/adapter-manager');
describe('Themes', function () {
let checkZipStub;
@ -32,7 +33,7 @@ describe('Themes', function () {
checkZipStub.resolves({});
formatStub.returns({results: {error: []}});
return validate.check(testTheme, true)
return validate.check(testTheme.name, testTheme, true)
.then((checkedTheme) => {
checkZipStub.calledOnce.should.be.true();
checkZipStub.calledWith(testTheme).should.be.true();
@ -48,7 +49,7 @@ describe('Themes', function () {
checkStub.resolves({});
formatStub.returns({results: {error: []}});
return validate.check(testTheme, false)
return validate.check(testTheme.name, testTheme, false)
.then((checkedTheme) => {
checkZipStub.callCount.should.be.equal(0);
checkStub.calledOnce.should.be.true();
@ -73,11 +74,12 @@ describe('Themes', function () {
failures: [{}],
code: 'GS001-DEPR-CON-AC'
}
]
],
hasFatalErrors: true
}
});
return validate.check(testTheme, true)
return validate.check(testTheme.name, testTheme, true)
.then((checkedTheme) => {
checkZipStub.calledOnce.should.be.true();
checkZipStub.calledWith(testTheme).should.be.true();
@ -101,11 +103,12 @@ describe('Themes', function () {
failures: [{}],
code: 'GS001-DEPR-CON-AC'
}
]
],
hasFatalErrors: true
}
});
return validate.check(testTheme, false)
return validate.check(testTheme.name, testTheme, false)
.then((checkedTheme) => {
checkStub.calledOnce.should.be.true();
checkStub.calledWith(testTheme.path).should.be.true();
@ -120,7 +123,7 @@ describe('Themes', function () {
checkZipStub.rejects(new Error('invalid zip file'));
formatStub.returns({results: {error: []}});
return validate.check(testTheme, true)
return validate.check(testTheme.name, testTheme, true)
.then((checkedTheme) => {
checkedTheme.should.not.exist();
}).catch((error) => {
@ -133,4 +136,56 @@ describe('Themes', function () {
});
});
});
describe('getThemeErrors', function () {
const testTheme = {
name: 'supertheme',
version: '1.0.0',
path: '/path/to/theme'
};
before(function () {
list.init();
list.set(testTheme.name, testTheme);
validate.init();
});
it('Does an innitial check if not cached yet', async function () {
checkStub.resolves({});
formatStub.returns({results: {error: [{hello: 'world'}]}});
const checkedTheme = await validate.getThemeErrors(testTheme.name);
sinon.assert.calledOnce(checkStub);
sinon.assert.calledOnce(formatStub);
assert.deepEqual(checkedTheme, {errors: [{hello: 'world'}], warnings: []});
});
it('Reuses same result if called again', async function () {
const checkedTheme = await validate.getThemeErrors(testTheme.name);
sinon.assert.notCalled(checkStub);
sinon.assert.notCalled(formatStub);
assert.deepEqual(checkedTheme, {errors: [{hello: 'world'}], warnings: []});
});
it('Throws for invalid theme names', async function () {
await assert.rejects(validate.getThemeErrors('invalid-theme-name'), /Theme "invalid-theme-name" is not loaded and cannot be checked/);
});
it('Silently fails when cache adapter throws', async function () {
sinon.stub(adapterManager, 'getAdapter').returns({
get: () => {
throw new Error('test');
}
});
validate.init();
checkStub.resolves({});
formatStub.returns({results: {error: [{hello: 'world'}]}});
const checkedTheme = await validate.getThemeErrors(testTheme.name);
sinon.assert.calledOnce(checkStub);
sinon.assert.calledOnce(formatStub);
assert.deepEqual(checkedTheme, {errors: [{hello: 'world'}], warnings: []});
});
});
});

View File

@ -219,6 +219,11 @@
"action_type": "activate",
"object_type": "theme"
},
{
"name": "View active theme details",
"action_type": "readActive",
"object_type": "theme"
},
{
"name": "Upload themes",
"action_type": "add",
@ -980,7 +985,7 @@
"user": "all",
"role": "all",
"invite": "all",
"theme": ["browse"],
"theme": ["browse", "readActive"],
"email_preview": "all",
"email": "all",
"snippet": "all",
@ -995,7 +1000,7 @@
"tag": ["browse", "read", "add"],
"user": ["browse", "read"],
"role": ["browse"],
"theme": ["browse"],
"theme": ["browse", "readActive"],
"email_preview": "read",
"email": "read",
"snippet": ["browse", "read"],