From 5bf3fe9cb8353d9b36801d7d7b0107300e0a4419 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Mon, 31 Oct 2022 12:01:39 +0000 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20error=20preventing?= =?UTF-8?q?=20admin=20area=20being=20usable=20by=20staff=20users=20with=20?= =?UTF-8?q?Contributor=20role?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://github.com/TryGhost/Team/issues/2190 - added a guard around the tier fetches in `membersUtils` service so the fetch doesn't occur unless we have a logged in user and they aren't a contributor - extracted the `withPermissionsCheck` mirage util function and added role checks around the mocked tiers endpoints - added an acceptance test that loads the content screen and creates a draft post as a contributor to help catch regressions --- ghost/admin/app/services/members-utils.js | 19 ++++-- ghost/admin/app/templates/posts.hbs | 32 +++++----- ghost/admin/mirage/config/members.js | 61 ++++++-------------- ghost/admin/mirage/config/tiers.js | 37 +++++++++--- ghost/admin/mirage/utils.js | 32 ++++++++++ ghost/admin/tests/acceptance/content-test.js | 32 ++++++++-- 6 files changed, 135 insertions(+), 78 deletions(-) diff --git a/ghost/admin/app/services/members-utils.js b/ghost/admin/app/services/members-utils.js index 7a659f63a7..456f8f461b 100644 --- a/ghost/admin/app/services/members-utils.js +++ b/ghost/admin/app/services/members-utils.js @@ -4,6 +4,7 @@ export default class MembersUtilsService extends Service { @service config; @service settings; @service feature; + @service session; @service store; paidTiers = null; @@ -29,15 +30,21 @@ export default class MembersUtilsService extends Service { return; } - return this.store.query('tier', {filter: 'type:paid+active:true', limit: 'all'}).then((tiers) => { - this.paidTiers = tiers; - }); + // contributors don't have permissions to fetch tiers + if (this.session.user && !this.session.user.isContributor) { + return this.store.query('tier', {filter: 'type:paid+active:true', limit: 'all'}).then((tiers) => { + this.paidTiers = tiers; + }); + } } async reload() { - return this.store.query('tier', {filter: 'type:paid+active:true', limit: 'all'}).then((tiers) => { - this.paidTiers = tiers; - }); + // contributors don't have permissions to fetch tiers + if (this.session.user && !this.session.user.isContributor) { + return this.store.query('tier', {filter: 'type:paid+active:true', limit: 'all'}).then((tiers) => { + this.paidTiers = tiers; + }); + } } /** diff --git a/ghost/admin/app/templates/posts.hbs b/ghost/admin/app/templates/posts.hbs index dcc25abf98..dbd8463997 100644 --- a/ghost/admin/app/templates/posts.hbs +++ b/ghost/admin/app/templates/posts.hbs @@ -35,22 +35,22 @@ data-test-post-id={{post.id}} /> {{else}} -
  • -
    - {{#if this.showingAll}} - {{svg-jar "posts-placeholder" class="gh-posts-placeholder"}} -

    Start creating content.

    - - Write a new post - - {{else}} -

    No posts match the current filter

    - - Show all posts - - {{/if}} -
    -
    • +
  • +
    + {{#if this.showingAll}} + {{svg-jar "posts-placeholder" class="gh-posts-placeholder"}} +

    Start creating content.

    + + Write a new post + + {{else}} +

    No posts match the current filter

    + + Show all posts + + {{/if}} +
    +
    • {{/each}} diff --git a/ghost/admin/mirage/config/members.js b/ghost/admin/mirage/config/members.js index 8bc2ac38ab..9ebae0853f 100644 --- a/ghost/admin/mirage/config/members.js +++ b/ghost/admin/mirage/config/members.js @@ -2,43 +2,20 @@ import faker from 'faker'; import moment from 'moment-timezone'; import nql from '@tryghost/nql'; import {Response} from 'miragejs'; -import {extractFilterParam, paginateModelCollection} from '../utils'; +import { + extractFilterParam, + paginateModelCollection, + withPermissionsCheck +} from '../utils'; import {underscore} from '@ember/string'; -function hasInvalidPermissions() { - const {schema, request} = this; - - // always allow dev requests through - the logged in user will be real so - // we can't check against it in the mocked db - if (!request.requestHeaders['X-Test-User']) { - return false; - } - - const invalidPermsResponse = new Response(403, {}, { - errors: [{ - type: 'NoPermissionError', - message: 'You do not have permission to perform this action' - }] - }); - - const user = schema.users.find(request.requestHeaders['X-Test-User']); - const adminRoles = user.roles.filter(role => ['Owner', 'Administrator'].includes(role.name)); - - if (adminRoles.length === 0) { - return invalidPermsResponse; - } -} - -function withPermissionsCheck(fn) { - return function () { - const boundPermsCheck = hasInvalidPermissions.bind(this); - const boundFn = fn.bind(this); - return boundPermsCheck() || boundFn(...arguments); - }; -} +const ALLOWED_ROLES = [ + 'Owner', + 'Administrator' +]; export function mockMembersStats(server) { - server.get('/members/stats/count', withPermissionsCheck(function (db, {queryParams}) { + server.get('/members/stats/count', withPermissionsCheck(ALLOWED_ROLES, function (db, {queryParams}) { let {days} = queryParams; let firstSubscriberDays = faker.datatype.number({min: 30, max: 600}); @@ -94,12 +71,12 @@ export function mockMembersStats(server) { } export default function mockMembers(server) { - server.post('/members/', withPermissionsCheck(function ({members}) { + server.post('/members/', withPermissionsCheck(ALLOWED_ROLES, function ({members}) { const attrs = this.normalizedRequestAttrs(); return members.create(attrs); })); - server.get('/members/', withPermissionsCheck(function ({members}, {queryParams}) { + server.get('/members/', withPermissionsCheck(ALLOWED_ROLES, function ({members}, {queryParams}) { let {filter, search, page, limit} = queryParams; page = +page || 1; @@ -164,7 +141,7 @@ export default function mockMembers(server) { return paginateModelCollection('members', collection, page, limit); })); - server.del('/members/', withPermissionsCheck(function ({members}, {queryParams}) { + server.del('/members/', withPermissionsCheck(ALLOWED_ROLES, function ({members}, {queryParams}) { if (!queryParams.filter && !queryParams.search && queryParams.all !== 'true') { return new Response(422, {}, {errors: [{ type: 'IncorrectUsageError', @@ -200,7 +177,7 @@ export default function mockMembers(server) { }; })); - server.get('/members/:id/', withPermissionsCheck(function ({members}, {params}) { + server.get('/members/:id/', withPermissionsCheck(ALLOWED_ROLES, function ({members}, {params}) { let {id} = params; let member = members.find(id); @@ -212,7 +189,7 @@ export default function mockMembers(server) { }); })); - server.put('/members/:id/', withPermissionsCheck(function ({members, tiers, subscriptions}, {params}) { + server.put('/members/:id/', withPermissionsCheck(ALLOWED_ROLES, function ({members, tiers, subscriptions}, {params}) { const attrs = this.normalizedRequestAttrs(); const member = members.find(params.id); @@ -282,12 +259,12 @@ export default function mockMembers(server) { return member.update(attrs); })); - server.del('/members/:id/', withPermissionsCheck(function ({members}, request) { + server.del('/members/:id/', withPermissionsCheck(ALLOWED_ROLES, function ({members}, request) { const id = request.params.id; members.find(id).destroy(); })); - server.get('/members/upload/', withPermissionsCheck(function () { + server.get('/members/upload/', withPermissionsCheck(ALLOWED_ROLES, function () { return new Response(200, { 'Content-Disposition': 'attachment', filename: `members.${moment().format('YYYY-MM-DD')}.csv`, @@ -295,7 +272,7 @@ export default function mockMembers(server) { }, ''); })); - server.post('/members/upload/', withPermissionsCheck(function ({labels}, request) { + server.post('/members/upload/', withPermissionsCheck(ALLOWED_ROLES, function ({labels}, request) { const label = labels.create(); // TODO: parse CSV and create member records @@ -312,7 +289,7 @@ export default function mockMembers(server) { }); })); - server.get('/members/events/', withPermissionsCheck(function ({memberActivityEvents}, {queryParams}) { + server.get('/members/events/', withPermissionsCheck(ALLOWED_ROLES, function ({memberActivityEvents}, {queryParams}) { let {limit} = queryParams; limit = +limit || 15; diff --git a/ghost/admin/mirage/config/tiers.js b/ghost/admin/mirage/config/tiers.js index 37c24eb9e6..99ab53368e 100644 --- a/ghost/admin/mirage/config/tiers.js +++ b/ghost/admin/mirage/config/tiers.js @@ -1,11 +1,27 @@ -import {paginatedResponse} from '../utils'; +import {paginatedResponse, withPermissionsCheck} from '../utils'; + +const ALLOWED_WRITE_ROLES = [ + 'Owner', + 'Administrator' +]; +const ALLOWED_READ_ROLES = [ + 'Owner', + 'Administrator', + 'Editor', + 'Author' +]; export default function mockTiers(server) { - server.post('/tiers/'); + // CREATE + server.post('/tiers/', withPermissionsCheck(ALLOWED_WRITE_ROLES, function ({tiers}) { + const attrs = this.normalizedRequestAttrs(); + return tiers.create(attrs); + })); - server.get('/tiers/', paginatedResponse('tiers')); + // READ + server.get('/tiers/', withPermissionsCheck(ALLOWED_READ_ROLES, paginatedResponse('tiers'))); - server.get('/tiers/:id/', function ({tiers}, {params}) { + server.get('/tiers/:id/', withPermissionsCheck(ALLOWED_READ_ROLES, function ({tiers}, {params}) { let {id} = params; let tier = tiers.find(id); @@ -15,16 +31,21 @@ export default function mockTiers(server) { message: 'Tier not found.' }] }); - }); + })); - server.put('/tiers/:id/', function ({tiers}, {params}) { + // UPDATE + server.put('/tiers/:id/', withPermissionsCheck(ALLOWED_WRITE_ROLES, function ({tiers}, {params}) { const attrs = this.normalizedRequestAttrs(); const tier = tiers.find(params.id); tier.update(attrs); return tier.save(); - }); + })); - server.del('/tiers/:id/'); + // DELETE + server.del('/tiers/:id/', withPermissionsCheck(ALLOWED_WRITE_ROLES, function (schema, request) { + const id = request.params.id; + schema.tiers.find(id).destroy(); + })); } diff --git a/ghost/admin/mirage/utils.js b/ghost/admin/mirage/utils.js index b22167cdb4..eef745cf0e 100644 --- a/ghost/admin/mirage/utils.js +++ b/ghost/admin/mirage/utils.js @@ -122,3 +122,35 @@ export function extractFilterParam(param, filter = '') { return normalizeBooleanParams(normalizeStringParams(match)); } + +export function hasInvalidPermissions(allowedRoles) { + const {schema, request} = this; + + // always allow dev requests through - the logged in user will be real so + // we can't check against it in the mocked db + if (!request.requestHeaders['X-Test-User']) { + return false; + } + + const invalidPermsResponse = new Response(403, {}, { + errors: [{ + type: 'NoPermissionError', + message: 'You do not have permission to perform this action' + }] + }); + + const user = schema.users.find(request.requestHeaders['X-Test-User']); + const adminRoles = user.roles.filter(role => allowedRoles.includes(role.name)); + + if (adminRoles.length === 0) { + return invalidPermsResponse; + } +} + +export function withPermissionsCheck(allowedRoles, fn) { + return function () { + const boundPermsCheck = hasInvalidPermissions.bind(this); + const boundFn = fn.bind(this); + return boundPermsCheck(allowedRoles) || boundFn(...arguments); + }; +} diff --git a/ghost/admin/tests/acceptance/content-test.js b/ghost/admin/tests/acceptance/content-test.js index 61c9f3a7d9..e939f3c41f 100644 --- a/ghost/admin/tests/acceptance/content-test.js +++ b/ghost/admin/tests/acceptance/content-test.js @@ -1,6 +1,6 @@ import {authenticateSession, invalidateSession} from 'ember-simple-auth/test-support'; import {beforeEach, describe, it} from 'mocha'; -import {click, currentURL, fillIn, find, findAll, settled, visit} from '@ember/test-helpers'; +import {blur, click, currentURL, fillIn, find, findAll, settled, visit} from '@ember/test-helpers'; import {clickTrigger, selectChoose} from 'ember-power-select/test-support/helpers'; import {expect} from 'chai'; import {setupApplicationTest} from 'ember-mocha'; @@ -247,13 +247,33 @@ describe('Acceptance: Content', function () { describe('as contributor', function () { beforeEach(async function () { - let adminRole = this.server.create('role', {name: 'Administrator'}); - let admin = this.server.create('user', {roles: [adminRole]}); - - // Create posts - this.server.create('post', {authors: [admin], status: 'scheduled', title: 'Admin Post'}); + let contributorRole = this.server.create('role', {name: 'Contributor'}); + let contributor = this.server.create('user', {roles: [contributorRole]}); return await authenticateSession(); }); + + it('shows posts list and allows post creation', async function () { + await visit('/posts'); + + // has an empty state + expect(findAll('[data-test-post-id]')).to.have.length(0); + expect(find('[data-test-no-posts-box]')).to.exist; + expect(find('[data-test-link="write-a-new-post"]')).to.exist; + + await click('[data-test-link="write-a-new-post"]'); + + expect(currentURL()).to.equal('/editor/post'); + + await fillIn('[data-test-editor-title-input]', 'First contributor post'); + await blur('[data-test-editor-title-input]'); + + expect(currentURL()).to.equal('/editor/post/1'); + + await click('[data-test-link="posts"]'); + + expect(findAll('[data-test-post-id]')).to.have.length(1); + expect(find('[data-test-no-posts-box]')).to.not.exist; + }); }); }); From b272b34a444bdb085d395a2f04bbe40e29024aa2 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Mon, 31 Oct 2022 12:12:58 +0000 Subject: [PATCH 2/4] Fixed linter error --- ghost/admin/tests/acceptance/content-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ghost/admin/tests/acceptance/content-test.js b/ghost/admin/tests/acceptance/content-test.js index e939f3c41f..7eea8bd7f3 100644 --- a/ghost/admin/tests/acceptance/content-test.js +++ b/ghost/admin/tests/acceptance/content-test.js @@ -248,7 +248,7 @@ describe('Acceptance: Content', function () { describe('as contributor', function () { beforeEach(async function () { let contributorRole = this.server.create('role', {name: 'Contributor'}); - let contributor = this.server.create('user', {roles: [contributorRole]}); + this.server.create('user', {roles: [contributorRole]}); return await authenticateSession(); }); From 8537239548cfe175e15f987f35377b001c1777f9 Mon Sep 17 00:00:00 2001 From: James Morris Date: Mon, 31 Oct 2022 12:10:36 +0000 Subject: [PATCH 3/4] Moved the audience feedback setting into the footer refs https://github.com/TryGhost/Team/issues/2191 --- .../modals/newsletters/edit/design.hbs | 18 ++++++++++++++++++ .../modals/newsletters/edit/design.js | 10 ++++++++++ .../modals/newsletters/edit/settings.hbs | 17 ----------------- ghost/admin/app/styles/components/modals.css | 4 ++++ 4 files changed, 32 insertions(+), 17 deletions(-) diff --git a/ghost/admin/app/components/modals/newsletters/edit/design.hbs b/ghost/admin/app/components/modals/newsletters/edit/design.hbs index 5f03c17b64..0877aaa898 100644 --- a/ghost/admin/app/components/modals/newsletters/edit/design.hbs +++ b/ghost/admin/app/components/modals/newsletters/edit/design.hbs @@ -173,6 +173,24 @@ {{#liquid-if isOpen}}
    + + {{#if (feature "audienceFeedback")}} + + +
    +
    + + +
    +
    +     + {{/if}} +
    - - {{#if (feature "audienceFeedback")}} - - -
    -
    - - -
    -
    -     - {{/if}}
    {{/liquid-if}} diff --git a/ghost/admin/app/styles/components/modals.css b/ghost/admin/app/styles/components/modals.css index c2e4e97c53..033c9a2dd9 100644 --- a/ghost/admin/app/styles/components/modals.css +++ b/ghost/admin/app/styles/components/modals.css @@ -694,6 +694,10 @@ padding: 12px 0; } +.modal-fullsettings-tab-expanded .gh-setting.gh-setting-extra { + padding-bottom: 24px; +} + .modal-fullsettings-form-labs .for-checkbox .input-toggle-component { background: var(--white); } From b2ad52fbf5617e0d0b7b69695a1c57b87f9f7f47 Mon Sep 17 00:00:00 2001 From: Ghost CI <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 31 Oct 2022 17:49:43 +0000 Subject: [PATCH 4/4] v5.22.1 --- ghost/admin/package.json | 2 +- ghost/core/package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ghost/admin/package.json b/ghost/admin/package.json index abf576656e..43f7fdefb7 100644 --- a/ghost/admin/package.json +++ b/ghost/admin/package.json @@ -1,6 +1,6 @@ { "name": "ghost-admin", - "version": "5.22.0", + "version": "5.22.1", "description": "Ember.js admin client for Ghost", "author": "Ghost Foundation", "homepage": "http://ghost.org", diff --git a/ghost/core/package.json b/ghost/core/package.json index bc92fe9f6a..a50caa2e27 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -1,6 +1,6 @@ { "name": "ghost", - "version": "5.22.0", + "version": "5.22.1", "description": "The professional publishing platform", "author": "Ghost Foundation", "homepage": "https://ghost.org",