From 13bf6280511f64ae7bdf49becc6633bf6c01c4c6 Mon Sep 17 00:00:00 2001 From: Peter Zimon Date: Thu, 3 Sep 2026 08:42:36 +0200 Subject: [PATCH 1/8] Changed Admin 7 font features for headings (#30486) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/ghost/issue/DES-1509 ## Why The display-oriented `dlig` and `cv05` Inter features introduced with Admin 7 page chrome look noisy in compact labels and controls. They should remain part of the editorial heading treatment without changing the rest of the Milestone 1 typography rollout. ## What changed - keep `zero` and `ss01` across eligible Admin 7 typography - apply `dlig` and `cv05` only to semantic `h1`–`h6` headings - preserve the existing Admin 7 flag and portal boundaries for Admin and Settings - add computed-style acceptance coverage for both Admin and Settings --- .../analytics/analytics.acceptance.test.tsx | 23 +++++++++++++++++++ apps/admin/src/index.css | 21 ++++++++++++++++- .../src/settings/layout.acceptance.test.tsx | 11 +++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/apps/admin/src/analytics/analytics.acceptance.test.tsx b/apps/admin/src/analytics/analytics.acceptance.test.tsx index 23ef7eb7853..f7bf256af81 100644 --- a/apps/admin/src/analytics/analytics.acceptance.test.tsx +++ b/apps/admin/src/analytics/analytics.acceptance.test.tsx @@ -176,6 +176,29 @@ describe('Analytics overview', () => { .toContain('Inter Admin 7'); }); + it('uses display font features only on Admin 7 headings', async () => { + seedAnalyticsWorld(); + seedTopPostsViews(); + await renderAdminApp('/analytics', { + labs: { admin7PageChrome: true }, + boot: webAnalyticsBootOverrides(), + }); + + await expect.element(analyticsScreen.membersValue()).toHaveTextContent('175'); + const heading = page.getByRole('heading', { name: 'Analytics' }); + await expect.element(heading).toBeVisible(); + const headingFeatures = getComputedStyle(heading.element()).fontFeatureSettings; + const smallTextFeatures = getComputedStyle( + analyticsScreen.activeVisitors().element(), + ).fontFeatureSettings; + expect(headingFeatures).toContain('dlig'); + expect(headingFeatures).toContain('cv05'); + expect(smallTextFeatures).toContain('zero'); + expect(smallTextFeatures).toContain('ss01'); + expect(smallTextFeatures).not.toContain('dlig'); + expect(smallTextFeatures).not.toContain('cv05'); + }); + it('re-queries Tinybird when the date range changes', async () => { const { kpisApi } = seedAnalyticsWorld(); seedTopPostsViews(); diff --git a/apps/admin/src/index.css b/apps/admin/src/index.css index e67dd43e0d9..5ea6d8dd18c 100644 --- a/apps/admin/src/index.css +++ b/apps/admin/src/index.css @@ -25,13 +25,32 @@ 'Inter Admin 7', Inter, -apple-system, BlinkMacSystemFont, avenir next, avenir, helvetica neue, helvetica, ubuntu, roboto, noto, segoe ui, arial, sans-serif; --font-family: var(--font-sans); - --font-feature-settings: 'dlig' 1, 'zero' 1, 'ss01' 1, 'cv05' 1; + --font-feature-settings: 'zero' 1, 'ss01' 1; + --heading-font-feature-settings: 'dlig' 1, 'zero' 1, 'ss01' 1, 'cv05' 1; font-family: var(--font-sans); font-optical-sizing: none; font-variation-settings: 'opsz' 14; font-feature-settings: var(--font-feature-settings); } +/* Display alternates feel crowded at small sizes, so reserve them for + semantic headings across both Admin screens and Settings overlays. */ +:where( + .admin7, + body.react-admin:has(> #root .admin7) + > :is( + .shade.shade-admin, + #ember-basic-dropdown-wormhole, + #ember-modal-wormhole, + #ember-liquid-wormhole, + #ember-alerts-wormhole, + #ember-notifications-wormhole + ) + ) + :where(h1, h2, h3, h4, h5, h6) { + font-feature-settings: var(--heading-font-feature-settings); +} + /* Browser form-control defaults can reset these inherited font properties. Keep the rule weaker than any explicit component typography choice. */ :where( diff --git a/apps/admin/src/settings/layout.acceptance.test.tsx b/apps/admin/src/settings/layout.acceptance.test.tsx index 8d8c3ee62a2..8be8edb84c5 100644 --- a/apps/admin/src/settings/layout.acceptance.test.tsx +++ b/apps/admin/src/settings/layout.acceptance.test.tsx @@ -21,6 +21,7 @@ describe('Settings layout', () => { await renderAdminApp('/settings', { labs: { admin7PageChrome: enabled } }); await expect.element(settingsScreen.search()).toBeVisible(); + const heading = page.getByRole('heading', { name: 'General settings', exact: true }).first(); const titleAndDescriptionEdit = settingsScreen .titleAndDescription() .getByRole('button', { name: 'Edit' }); @@ -35,6 +36,16 @@ describe('Settings layout', () => { .poll(() => getComputedStyle(element).fontFamily.includes('Inter Admin 7')) .toBe(enabled); } + if (enabled) { + const headingFeatures = getComputedStyle(heading.element()).fontFeatureSettings; + const searchFeatures = getComputedStyle( + settingsScreen.search().element(), + ).fontFeatureSettings; + expect(headingFeatures).toContain('dlig'); + expect(headingFeatures).toContain('cv05'); + expect(searchFeatures).not.toContain('dlig'); + expect(searchFeatures).not.toContain('cv05'); + } expect(document.querySelector('.admin7') !== null).toBe(enabled); expect( getComputedStyle(document.querySelector('#root > div')!).getPropertyValue( From 1e39926cede387cb475941373637280f38ccaff5 Mon Sep 17 00:00:00 2001 From: Princi Vershwal Date: Thu, 3 Sep 2026 13:04:05 +0530 Subject: [PATCH 2/8] Migrated sendWebmentions to the class-based jobs service (#30392) ref https://linear.app/ghost/issue/HKG-1979 - The outbound webmention job was an inline closure on the legacy mentions JobManager - live objects that can never leave the process. Moving it behind the class-based interface as a serialisable message is the precondition for the durable backend, and retires that queue's last tenant ahead of the legacy job system removal. - dispatches an event-time snapshot (source URL, html, previous html) instead of a closure, so the job survives serialisation and a future replay recomputes the same bounded target set - registers on the routed webmentions lane at the legacy queue's concurrency, because publish-triggered external HTTP is slow and flood-prone - the isolation the old second job manager existed for must survive the migration - makes the jobs service instance process-lifetime: services capture it behind their init guards, so per-boot instances stranded those references on dead queues under the test harness's in-process restarts, silently dropping dispatches - a latent problem for the inbound job too - rewrites the outbound e2e waits to track job processing rather than queue state, so the tests assume nothing about the in-memory backend and keep working when a durable one arrives - leaves production behavior unchanged beyond the platform deltas every migrated job shares: JobsService lifecycle logging, Sentry capture for send failures, and a bounded shutdown drain --- ghost/core/core/boot.js | 2 + .../server/services/jobs-service/index.ts | 9 + .../services/jobs-service/jobs-service.ts | 9 + .../jobs-service/register-job-handlers.ts | 12 ++ .../mentions/mention-sending-service.d.ts | 7 + .../mentions/mention-sending-service.js | 35 +++- .../services/mentions/send-webmentions-job.ts | 28 +++ .../core/server/services/mentions/service.js | 32 +--- .../test/e2e-server/services/mentions.test.js | 165 ++++++++++------ .../mentions/send-webmentions-job.test.ts | 68 +++++++ .../services/jobs-service/index.test.js | 49 ++++- .../jobs-service/jobs-service.test.ts | 20 ++ .../register-job-handlers.test.ts | 23 +++ .../mentions/mention-sending-service.test.js | 179 ++++++++++++------ .../mentions/send-webmentions-job.test.ts | 47 +++++ .../server/services/mentions/service.test.js | 62 +----- 16 files changed, 536 insertions(+), 211 deletions(-) create mode 100644 ghost/core/core/server/services/mentions/mention-sending-service.d.ts create mode 100644 ghost/core/core/server/services/mentions/send-webmentions-job.ts create mode 100644 ghost/core/test/integration/services/mentions/send-webmentions-job.test.ts create mode 100644 ghost/core/test/unit/server/services/mentions/send-webmentions-job.test.ts diff --git a/ghost/core/core/boot.js b/ghost/core/core/boot.js index eec2dcc815e..699b1f77588 100644 --- a/ghost/core/core/boot.js +++ b/ghost/core/core/boot.js @@ -690,12 +690,14 @@ async function bootGhost({ backend = true, frontend = true, server = true } = {} memberJobs.init(); assert(gifts.service, 'Gift service should be initialized'); assert(mentionsService.controller, 'Mentions controller should be initialized'); + assert(mentionsService.sendingService, 'Mentions sending service should be initialized'); registerJobHandlers({ jobsService, memberJobs, giftService: gifts.service, mediaInliner: mediaInliner.getInstance(), mentionsController: mentionsService.controller, + mentionsSendingService: mentionsService.sendingService, }); await jobsService.start(); debug('End: Register job handlers'); diff --git a/ghost/core/core/server/services/jobs-service/index.ts b/ghost/core/core/server/services/jobs-service/index.ts index 60bb5a1810c..aea9c11549b 100644 --- a/ghost/core/core/server/services/jobs-service/index.ts +++ b/ghost/core/core/server/services/jobs-service/index.ts @@ -5,6 +5,15 @@ import type { JobsShutdownOptions } from '@tryghost/adapter-base-jobs'; let instance: JobsService | undefined; export function init(): JobsService { + // The instance lives for the whole process: the didInit-guarded mentions + // service captures it in MentionController and MentionSendingService, so + // an in-process restart (test harness) must revive the same object rather + // than strand those references on a stopped queue. + if (instance) { + instance.clearHandlers(); + return instance; + } + const adapterManager = require('../adapter-manager').default; const logging = require('@tryghost/logging'); const sentry = require('../../../shared/sentry'); diff --git a/ghost/core/core/server/services/jobs-service/jobs-service.ts b/ghost/core/core/server/services/jobs-service/jobs-service.ts index bd8ce9ba7e1..f1ee98be509 100644 --- a/ghost/core/core/server/services/jobs-service/jobs-service.ts +++ b/ghost/core/core/server/services/jobs-service/jobs-service.ts @@ -152,6 +152,15 @@ export class JobsService { await this.#backend.shutdown(options); } + // An in-process restart (test harness) re-runs handler registration on the + // same instance, so all registration state resets - handlers and queue + // declarations alike. The duplicate-type guard still holds within a boot. + clearHandlers(): void { + this.#registry.clear(); + this.#queueByType.clear(); + this.#queues.clear(); + } + #buildEnvelope(job: Job): JobEnvelope { return { type: this.#typeOf(job), payload: JSON.stringify(job) }; } diff --git a/ghost/core/core/server/services/jobs-service/register-job-handlers.ts b/ghost/core/core/server/services/jobs-service/register-job-handlers.ts index 3968120a54f..0c95b19d6cf 100644 --- a/ghost/core/core/server/services/jobs-service/register-job-handlers.ts +++ b/ghost/core/core/server/services/jobs-service/register-job-handlers.ts @@ -10,7 +10,9 @@ import ContentCSVImportJob from '../content-import/jobs/content-csv-import-job'; import * as contentImport from '../content-import'; import UpdateCheckJob from '../update-check/jobs/update-check-job'; import type MentionController from '../mentions/mention-controller'; +import type MentionSendingService from '../mentions/mention-sending-service'; import ProcessWebmentionJob from '../mentions/process-webmention-job'; +import SendWebmentionsJob from '../mentions/send-webmentions-job'; const updateCheck = require('../update-check'); @@ -31,6 +33,7 @@ interface RegisterJobHandlersDependencies { giftService: GiftService; mediaInliner: ExternalMediaInliner; mentionsController: MentionController; + mentionsSendingService: MentionSendingService; } export default function registerJobHandlers({ @@ -39,6 +42,7 @@ export default function registerJobHandlers({ giftService, mediaInliner, mentionsController, + mentionsSendingService, }: RegisterJobHandlersDependencies): void { jobsService.handle(CleanTokensJob, async () => { await memberJobs.cleanTokens(); @@ -71,4 +75,12 @@ export default function registerJobHandlers({ }, WEBMENTIONS_QUEUE, ); + + jobsService.handle( + SendWebmentionsJob, + async (job) => { + await mentionsSendingService.sendWebmentions(job); + }, + WEBMENTIONS_QUEUE, + ); } diff --git a/ghost/core/core/server/services/mentions/mention-sending-service.d.ts b/ghost/core/core/server/services/mentions/mention-sending-service.d.ts new file mode 100644 index 00000000000..536e55fc8b1 --- /dev/null +++ b/ghost/core/core/server/services/mentions/mention-sending-service.d.ts @@ -0,0 +1,7 @@ +import type SendWebmentionsJob from './send-webmentions-job'; + +declare class MentionSendingService { + sendWebmentions(job: SendWebmentionsJob): Promise; +} + +export = MentionSendingService; diff --git a/ghost/core/core/server/services/mentions/mention-sending-service.js b/ghost/core/core/server/services/mentions/mention-sending-service.js index 9ffd794fb7a..a321a1d6256 100644 --- a/ghost/core/core/server/services/mentions/mention-sending-service.js +++ b/ghost/core/core/server/services/mentions/mention-sending-service.js @@ -1,5 +1,6 @@ const errors = require('@tryghost/errors'); const logging = require('@tryghost/logging'); +const SendWebmentionsJob = require('./send-webmentions-job').default; module.exports = class MentionSendingService { #discoveryService; @@ -8,8 +9,12 @@ module.exports = class MentionSendingService { #getPostData; #getPostUrl; #isEnabled; - #jobService; + #jobsService; + /** + * @param {object} deps + * @param {import('../jobs-service/jobs-service').JobsService} deps.jobsService + */ constructor({ discoveryService, externalRequest, @@ -17,7 +22,7 @@ module.exports = class MentionSendingService { getPostData, getPostUrl, isEnabled, - jobService, + jobsService, }) { this.#discoveryService = discoveryService; this.#externalRequest = externalRequest; @@ -25,7 +30,7 @@ module.exports = class MentionSendingService { this.#getPostData = getPostData; this.#getPostUrl = getPostUrl; this.#isEnabled = isEnabled; - this.#jobService = jobService; + this.#jobsService = jobsService; } get siteUrl() { @@ -82,13 +87,13 @@ module.exports = class MentionSendingService { // Capture the source URL now, from the event's data, rather than // deferring the model into the job. const url = new URL(await this.#resolvePostUrl(post)); - await this.#jobService.addJob('sendWebmentions', async () => { - await this.sendForHTMLResource({ - url, + await this.#jobsService.dispatch( + new SendWebmentionsJob({ + sourceUrl: url.href, html: html, previousHtml: previousHtml, - }); - }); + }), + ); } } catch (e) { logging.error('Error in webmention sending service post update event handler:'); @@ -157,11 +162,23 @@ module.exports = class MentionSendingService { }); } + /** + * Send the webmentions for a delivered job. + * @param {import('./send-webmentions-job').default} job + */ + async sendWebmentions(job) { + await this.sendForHTMLResource({ + url: new URL(job.sourceUrl), + html: job.html, + previousHtml: job.previousHtml, + }); + } + /** * Send a webmention call for the links in a resource. * @param {object} resource * @param {URL} resource.url - * @param {string} resource.html + * @param {string|null} resource.html * @param {string|null} [resource.previousHtml] */ async sendForHTMLResource(resource) { diff --git a/ghost/core/core/server/services/mentions/send-webmentions-job.ts b/ghost/core/core/server/services/mentions/send-webmentions-job.ts new file mode 100644 index 00000000000..c080dab6170 --- /dev/null +++ b/ghost/core/core/server/services/mentions/send-webmentions-job.ts @@ -0,0 +1,28 @@ +import { Job } from '../jobs-service/job'; + +// Carries the post's full html twice - the largest job payload in the system; +// revisit if a durable backend puts size limits on envelopes. +export default class SendWebmentionsJob extends Job { + static type = 'send-webmentions'; + + readonly sourceUrl: string; + + readonly html: string | null; + + readonly previousHtml: string | null; + + constructor({ + sourceUrl, + html, + previousHtml, + }: { + sourceUrl: string; + html: string | null; + previousHtml: string | null; + }) { + super(); + this.sourceUrl = sourceUrl; + this.html = html ?? null; + this.previousHtml = previousHtml ?? null; + } +} diff --git a/ghost/core/core/server/services/mentions/service.js b/ghost/core/core/server/services/mentions/service.js index f8fad73415e..5a5c11c2795 100644 --- a/ghost/core/core/server/services/mentions/service.js +++ b/ghost/core/core/server/services/mentions/service.js @@ -14,8 +14,6 @@ const outputSerializerUrlUtil = require('../../../server/api/endpoints/utils/ser const urlService = require('../url'); const settingsCache = require('../../../shared/settings-cache'); const DomainEvents = require('@tryghost/domain-events'); -const logging = require('@tryghost/logging'); -const mentionsJobService = require('../mentions-jobs'); // Serializes a post model to the data the URL service needs, loading the // relations it reads for filtered collections (event-emitted models don't @@ -38,33 +36,6 @@ function getPostUrl(id, postData) { return jsonModel.url; } -// Reports the same queued/started/finished/failed lifecycle for every mentions -// background job. The wrapped callback's result and errors pass through unchanged, -// so job manager outcomes and retries are unaffected. -function makeLoggingJobService() { - return { - async addJob(name, fn) { - logging.info(`[Background Job] ${name} queued`); - mentionsJobService.addJob({ - name, - job: async () => { - const startedAt = Date.now(); - logging.info(`[Background Job] ${name} started`); - try { - const result = await fn(); - logging.info(`[Background Job] ${name} completed in ${Date.now() - startedAt}ms`); - return result; - } catch (err) { - logging.error(err, `[Background Job] ${name} failed after ${Date.now() - startedAt}ms`); - throw err; - } - }, - offloaded: false, - }); - }, - }; -} - module.exports = { /** @type {import('./mentions-api')} */ api: null, @@ -141,7 +112,7 @@ module.exports = { getPostData: (post) => getPostData(post), getPostUrl: (id, data) => getPostUrl(id, data), isEnabled: () => !settingsCache.get('is_private'), - jobService: makeLoggingJobService(), + jobsService, }); sendingService.listen(events); @@ -152,4 +123,3 @@ module.exports = { // exposed for testing module.exports.getPostData = getPostData; module.exports.getPostUrl = getPostUrl; -module.exports.makeLoggingJobService = makeLoggingJobService; diff --git a/ghost/core/test/e2e-server/services/mentions.test.js b/ghost/core/test/e2e-server/services/mentions.test.js index 8e851537d22..3841dcd4f5d 100644 --- a/ghost/core/test/e2e-server/services/mentions.test.js +++ b/ghost/core/test/e2e-server/services/mentions.test.js @@ -1,10 +1,12 @@ +/* global vi */ const { agentProvider, fixtureManager, mockManager } = require('../../utils/e2e-framework'); const nock = require('nock'); const sinon = require('sinon'); const assert = require('node:assert/strict'); const markdownToLexical = require('../../utils/fixtures/data-generator').markdownToLexical; -const jobsService = require('../../../core/server/services/mentions-jobs'); +const mentionsService = require('../../../core/server/services/mentions'); const urlService = require('../../../core/server/services/url'); +const events = require('../../../core/server/lib/common/events'); let agent; let mentionUrl = new URL('https://www.otherghostsite.com/'); @@ -19,6 +21,78 @@ let mentionMock; let endpointMock; const DomainEvents = require('@tryghost/domain-events'); +// sendForPost runs from model events that fire on transaction commit and +// awaits DB work before dispatching. Track every run so tests can drain them. +const mentionEvents = [ + 'post.published', + 'post.published.edited', + 'post.unpublished', + 'page.published', + 'page.published.edited', + 'page.unpublished', +]; +const pendingSendForPost = []; +const wrappedListeners = []; + +function trackSendForPost() { + for (const eventName of mentionEvents) { + const original = events.listeners(eventName).find((l) => l.name === 'bound sendForPost'); + assert.ok(original, `expected a sendForPost listener on ${eventName}`); + const wrapper = (...args) => { + const run = original(...args); + pendingSendForPost.push(run); + return run; + }; + events.removeListener(eventName, original); + events.on(eventName, wrapper); + wrappedListeners.push({ eventName, original, wrapper }); + } +} + +function untrackSendForPost() { + for (const { eventName, original, wrapper } of wrappedListeners) { + events.removeListener(eventName, wrapper); + events.on(eventName, original); + } + wrappedListeners.length = 0; +} + +// Draining the runs proves no job was dispatched - for the cases whose +// filters return before reaching the queue. +async function sendForPostSettled() { + while (pendingSendForPost.length) { + await Promise.all(pendingSendForPost.splice(0)); + } + await DomainEvents.allSettled(); +} + +let processedJobs; + +function recordProcessed(resource) { + processedJobs.push(resource); +} + +// Wrap the seam the job handler calls: completion here means the job ran, +// without coupling the test to the queue or its backend. +function trackWebmentionSending() { + const sendingService = mentionsService.sendingService; + const original = sendingService.sendForHTMLResource.bind(sendingService); + sinon.stub(sendingService, 'sendForHTMLResource').callsFake(async (resource) => { + try { + return await original(resource); + } finally { + // Recorded in finally: "processed" means the handler finished, + // successful or not - the nock assertions decide correctness. + recordProcessed(resource); + } + }); +} + +async function waitForSends(count, { timeoutMs = 5000 } = {}) { + await vi.waitUntil(() => processedJobs.length >= count, { timeout: timeoutMs }); + await DomainEvents.allSettled(); +} + const mentionsPost = { title: 'testing sending webmentions', lexical: markdownToLexical(mentionHtml), @@ -45,6 +119,11 @@ describe('Mentions Service', function () { agent = await agentProvider.getAdminAPIAgent(); await fixtureManager.init('users'); await agent.loginAsAdmin(); + trackSendForPost(); + }); + + afterAll(function () { + untrackSendForPost(); }); beforeEach(async function () { @@ -54,8 +133,9 @@ describe('Mentions Service', function () { // mock response from website mentioned by post to provide endpoint addMentionMocks(); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await sendForPostSettled(); + processedJobs = []; + trackWebmentionSending(); }); afterEach(async function () { @@ -72,8 +152,7 @@ describe('Mentions Service', function () { .body({ posts: [draftPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await sendForPostSettled(); assert.equal(mentionMock.isDone(), false); assert.equal(endpointMock.isDone(), false); @@ -86,8 +165,7 @@ describe('Mentions Service', function () { .body({ posts: [publishedPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await sendForPostSettled(); assert.equal(mentionMock.isDone(), false); assert.equal(endpointMock.isDone(), false); @@ -104,8 +182,7 @@ describe('Mentions Service', function () { .body({ posts: [publishedPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await sendForPostSettled(); assert.equal(mentionMock.isDone(), false); assert.equal(endpointMock.isDone(), false); @@ -118,8 +195,7 @@ describe('Mentions Service', function () { .body({ pages: [draftPage] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await sendForPostSettled(); assert.equal(mentionMock.isDone(), false); assert.equal(endpointMock.isDone(), false); @@ -134,8 +210,7 @@ describe('Mentions Service', function () { .body({ posts: [publishedPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); assert.equal(mentionMock.isDone(), true); assert.equal(endpointMock.isDone(), true); @@ -148,8 +223,7 @@ describe('Mentions Service', function () { .body({ posts: [publishedPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); // while not the point of the test, we should have real links/mentions to start with assert.equal(mentionMock.isDone(), true); @@ -171,8 +245,9 @@ describe('Mentions Service', function () { .body({ posts: [editedPost] }) .expectStatus(200); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + // The edit dispatches a job (the html changed); "does not send" is + // the handler's link diff yielding nothing, so wait for processing. + await waitForSends(2); assert.equal(mentionMock.isDone(), false); assert.equal(endpointMock.isDone(), false); @@ -185,8 +260,7 @@ describe('Mentions Service', function () { .body({ posts: [publishedPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); // while not the point of the test, we should have real links/mentions to start with assert.equal(mentionMock.isDone(), true); @@ -216,8 +290,7 @@ describe('Mentions Service', function () { .body({ posts: [editedPost] }) .expectStatus(200); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(2); assert.equal(mentionMockTwo.isDone(), true); assert.equal(endpointMockTwo.isDone(), true); @@ -234,8 +307,7 @@ describe('Mentions Service', function () { .body({ posts: [publishedPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); // while not the point of the test, we should have real links/mentions to start with assert.equal(mentionMock.isDone(), true); @@ -261,8 +333,7 @@ describe('Mentions Service', function () { .body({ posts: [unpublishedPost] }) .expectStatus(200); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(2); assert.equal(mentionMockTwo.isDone(), true); assert.equal(endpointMockTwo.isDone(), true); @@ -279,8 +350,7 @@ describe('Mentions Service', function () { .body({ posts: [publishedPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); assert.equal(endpointMock.isDone(), true); nock.cleanAll(); @@ -296,8 +366,7 @@ describe('Mentions Service', function () { const postId = res.body.posts[0].id; await agent.delete(`posts/${postId}/`).expectStatus(204); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(2); // the webmention for the removed content went out... assert.equal(endpointMock.isDone(), true); @@ -317,8 +386,7 @@ describe('Mentions Service', function () { .body({ pages: [publishedPage] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); assert.equal(mentionMock.isDone(), true); assert.equal(endpointMock.isDone(), true); @@ -331,8 +399,7 @@ describe('Mentions Service', function () { .body({ pages: [publishedPage] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); // while not the point of the test, we should have real links/mentions to start with assert.equal(mentionMock.isDone(), true); @@ -354,8 +421,9 @@ describe('Mentions Service', function () { .body({ pages: [editedPage] }) .expectStatus(200); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + // The edit dispatches a job (the html changed); "does not send" is + // the handler's link diff yielding nothing, so wait for processing. + await waitForSends(2); assert.equal(mentionMock.isDone(), false); assert.equal(mentionMock.isDone(), false); @@ -368,8 +436,7 @@ describe('Mentions Service', function () { .body({ pages: [publishedPage] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); // while not the point of the test, we should have real links/mentions to start with assert.equal(mentionMock.isDone(), true); @@ -399,8 +466,7 @@ describe('Mentions Service', function () { .body({ pages: [editedPage] }) .expectStatus(200); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(2); assert.equal(mentionMockTwo.isDone(), true); assert.equal(endpointMockTwo.isDone(), true); @@ -417,8 +483,7 @@ describe('Mentions Service', function () { .body({ pages: [publishedPage] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); // while not the point of the test, we should have real links/mentions to start with assert.equal(mentionMock.isDone(), true); @@ -444,8 +509,7 @@ describe('Mentions Service', function () { .body({ pages: [unpublishedPage] }) .expectStatus(200); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(2); assert.equal(mentionMockTwo.isDone(), true); assert.equal(endpointMockTwo.isDone(), true); @@ -458,8 +522,7 @@ describe('Mentions Service', function () { .body({ posts: [publishedPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); // while not the point of the test, we should have real links/mentions to start with assert.equal(mentionMock.isDone(), true); @@ -485,8 +548,7 @@ describe('Mentions Service', function () { .body({ posts: [editedPost] }) .expectStatus(200); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(2); assert.equal(mentionMockTwo.isDone(), true); assert.equal(endpointMockTwo.isDone(), true); @@ -499,8 +561,7 @@ describe('Mentions Service', function () { .body({ pages: [publishedPage] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); // while not the point of the test, we should have real links/mentions to start with assert.equal(mentionMock.isDone(), true); @@ -526,8 +587,7 @@ describe('Mentions Service', function () { .body({ pages: [editedPage] }) .expectStatus(200); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(2); assert.equal(mentionMockTwo.isDone(), true); assert.equal(endpointMockTwo.isDone(), true); @@ -541,8 +601,7 @@ describe('Mentions Service', function () { .body({ posts: [publishedPost] }) .expectStatus(201); - await jobsService.allSettled(); - await DomainEvents.allSettled(); + await waitForSends(1); assert.equal(mentionMock.isDone(), true); assert.equal(endpointMock.isDone(), true); diff --git a/ghost/core/test/integration/services/mentions/send-webmentions-job.test.ts b/ghost/core/test/integration/services/mentions/send-webmentions-job.test.ts new file mode 100644 index 00000000000..5ed5acaf62d --- /dev/null +++ b/ghost/core/test/integration/services/mentions/send-webmentions-job.test.ts @@ -0,0 +1,68 @@ +import { describe, it, beforeAll, afterAll } from 'vitest'; +import assert from 'node:assert/strict'; +import nock from 'nock'; + +const { agentProvider, fixtureManager } = require('../../../utils/e2e-framework'); +const { getInstance: getJobsService } = require('../../../../core/server/services/jobs-service'); +const SendWebmentionsJob = + require('../../../../core/server/services/mentions/send-webmentions-job').default; + +async function waitFor( + check: () => boolean | Promise, + { timeoutMs = 5000, intervalMs = 25 } = {}, +): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (await check()) { + return true; + } + await new Promise((resolve) => { + setTimeout(resolve, intervalMs); + }); + } + return false; +} + +describe('Job: Send webmentions', function () { + beforeAll(async function () { + await agentProvider.getAdminAPIAgent(); + await fixtureManager.init('posts'); + }); + + afterAll(function () { + nock.cleanAll(); + }); + + it('sends a webmention to the linked target when the dispatched job runs', async function () { + const targetUrl = new URL('https://target-of-outbound-webmention.com/article/'); + const endpointUrl = new URL('https://target-of-outbound-webmention.com/webmention-endpoint/'); + const targetHtml = `Some content`; + + nock(targetUrl.origin) + .persist() + .get(targetUrl.pathname) + .reply(200, targetHtml, { 'Content-Type': 'text/html' }); + let receivedBody: string | null = null; + const endpointScope = nock(endpointUrl.origin) + .post(endpointUrl.pathname, (body) => { + receivedBody = new URLSearchParams(body).toString(); + return true; + }) + .reply(201); + + await getJobsService().dispatch( + new SendWebmentionsJob({ + sourceUrl: 'http://127.0.0.1:2369/source-post/', + html: `linked`, + previousHtml: null, + }), + ); + + const delivered = await waitFor(() => endpointScope.isDone()); + assert.ok(delivered, 'The webmention endpoint received the notification'); + + const body = new URLSearchParams(receivedBody!); + assert.equal(body.get('source'), 'http://127.0.0.1:2369/source-post/'); + assert.equal(body.get('target'), targetUrl.href); + }); +}); diff --git a/ghost/core/test/unit/server/services/jobs-service/index.test.js b/ghost/core/test/unit/server/services/jobs-service/index.test.js index a18170c6f28..527d86b7b30 100644 --- a/ghost/core/test/unit/server/services/jobs-service/index.test.js +++ b/ghost/core/test/unit/server/services/jobs-service/index.test.js @@ -33,15 +33,27 @@ describe('jobs-service wrapper', function () { assert.throws(() => jobsService.getInstance(), /used before init/); }); - it('init builds the service from the jobs adapter and getInstance returns it', function () { - const fakeBackend = { + function makeFakeBackend() { + return { requiredFns: ['start', 'enqueue', 'scheduleRecurring', 'shutdown'], + shutdownCalls: 0, start() {}, enqueue() {}, scheduleRecurring() {}, - async shutdown() {}, + async shutdown() { + this.shutdownCalls += 1; + }, }; - sinon.stub(adapterManager, 'getAdapter').withArgs('jobs').returns(fakeBackend); + } + + function stubAdapters() { + const jobsBackend = makeFakeBackend(); + sinon.stub(adapterManager, 'getAdapter').withArgs('jobs').returns(jobsBackend); + return { jobsBackend }; + } + + it('init builds the service from the jobs adapter and getInstance returns it', function () { + stubAdapters(); const service = jobsService.init(); @@ -51,4 +63,33 @@ describe('jobs-service wrapper', function () { 'getInstance returns the instance built by init', ); }); + + it('init after a shutdown reuses the same instance, so captured references stay live', async function () { + stubAdapters(); + + const service = jobsService.init(); + await jobsService.shutdown({ timeoutMs: 10 }); + + assert.equal(jobsService.init(), service, 'the instance survives a reboot'); + }); + + it('re-init clears handlers so a reboot can register the same job types again', function () { + stubAdapters(); + + class RebootJob { + static type = 'reboot-job'; + } + jobsService.init().handle(RebootJob, async () => {}); + + jobsService.init().handle(RebootJob, async () => {}); + }); + + it('shutdown after init shuts down the backend', async function () { + const { jobsBackend } = stubAdapters(); + + jobsService.init(); + await jobsService.shutdown({ timeoutMs: 10 }); + + assert.equal(jobsBackend.shutdownCalls, 1, 'the jobs backend was shut down'); + }); }); diff --git a/ghost/core/test/unit/server/services/jobs-service/jobs-service.test.ts b/ghost/core/test/unit/server/services/jobs-service/jobs-service.test.ts index 75bd8d877f5..920e4f6e6a6 100644 --- a/ghost/core/test/unit/server/services/jobs-service/jobs-service.test.ts +++ b/ghost/core/test/unit/server/services/jobs-service/jobs-service.test.ts @@ -367,4 +367,24 @@ describe('JobsService', function () { assert.deepEqual(backend.shutdownCalls, [{ timeoutMs: 42 }]); }); }); + + describe('restart', function () { + it('clearHandlers lets a rebooted process register the same job types again', function () { + const service = makeService(); + service.handle(GreetJob, async () => {}); + + service.clearHandlers(); + + service.handle(GreetJob, async () => {}); + }); + + it('clearHandlers resets queue declarations so a reboot can re-declare them', function () { + const service = makeService(); + service.handle(GreetJob, async () => {}, { queue: 'greetings', concurrency: 1 }); + + service.clearHandlers(); + + service.handle(GreetJob, async () => {}, { queue: 'greetings', concurrency: 2 }); + }); + }); }); diff --git a/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts b/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts index 3864779a4b9..3ebb6fb1b50 100644 --- a/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts +++ b/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts @@ -7,6 +7,7 @@ import ExternalMediaInlinerJob from '../../../../../core/server/services/media-i import ContentCSVImportJob from '../../../../../core/server/services/content-import/jobs/content-csv-import-job'; import UpdateCheckJob from '../../../../../core/server/services/update-check/jobs/update-check-job'; import ProcessWebmentionJob from '../../../../../core/server/services/mentions/process-webmention-job'; +import SendWebmentionsJob from '../../../../../core/server/services/mentions/send-webmentions-job'; const registerJobHandlers = require('../../../../../core/server/services/jobs-service/register-job-handlers').default; @@ -17,6 +18,7 @@ describe('register-job-handlers', function () { let memberJobs: { cleanTokens: sinon.SinonStub; cleanExpiredComped: sinon.SinonStub }; let giftService: { cleanup: sinon.SinonStub }; let mentionsController: { processWebmention: sinon.SinonStub }; + let mentionsSendingService: { sendWebmentions: sinon.SinonStub }; // Handlers are looked up by their job type rather than registration order, // so adding a handler does not silently shift which one a test exercises. @@ -41,6 +43,7 @@ describe('register-job-handlers', function () { }; giftService = { cleanup: sinon.stub().resolves() }; mentionsController = { processWebmention: sinon.stub().resolves() }; + mentionsSendingService = { sendWebmentions: sinon.stub().resolves() }; registerJobHandlers({ jobsService, @@ -48,6 +51,7 @@ describe('register-job-handlers', function () { giftService, mediaInliner, mentionsController, + mentionsSendingService, }); }); @@ -148,4 +152,23 @@ describe('register-job-handlers', function () { assert.deepEqual(registration.args[2], { queue: 'webmentions', concurrency: 3 }); }); + + it('runs send-webmentions with the injected mentions sending service', async function () { + const sendWebmentionsHandler = handlerFor('send-webmentions'); + const job = new SendWebmentionsJob({ + sourceUrl: 'https://site.com/post/', + html: 'link', + previousHtml: null, + }); + + await sendWebmentionsHandler(job); + + assert.ok(mentionsSendingService.sendWebmentions.calledOnceWithExactly(job)); + }); + + it('registers send-webmentions on the dedicated webmentions queue', function () { + const registration = registrationFor('send-webmentions'); + + assert.deepEqual(registration.args[2], { queue: 'webmentions', concurrency: 3 }); + }); }); diff --git a/ghost/core/test/unit/server/services/mentions/mention-sending-service.test.js b/ghost/core/test/unit/server/services/mentions/mention-sending-service.test.js index 3a5f53f9587..8b1786e44b3 100644 --- a/ghost/core/test/unit/server/services/mentions/mention-sending-service.test.js +++ b/ghost/core/test/unit/server/services/mentions/mention-sending-service.test.js @@ -8,12 +8,8 @@ const logging = require('@tryghost/logging'); const dnsPromises = require('node:dns').promises; const { createModel } = require('./utils/index.js'); -// mock up job service -let jobService = { - async addJob(name, fn) { - return fn(); - }, -}; +const SendWebmentionsJob = + require('../../../../../core/server/services/mentions/send-webmentions-job').default; describe('MentionSendingService', function () { let errorLogStub; @@ -53,39 +49,81 @@ describe('MentionSendingService', function () { describe('sendForPost', function () { it('Ignores if disabled', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => false, + getPostData: () => ({}), + getPostUrl: () => 'https://site.com/post/', + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); - await service.sendForPost({}); - sinon.assert.notCalled(stub); + await service.sendForPost( + createModel({ + status: 'published', + html: 'same', + previous: { + status: 'draft', + html: 'same', + }, + }), + ); + sinon.assert.notCalled(jobsService.dispatch); + sinon.assert.notCalled(errorLogStub); }); it('Ignores if importing data', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, + getPostData: () => ({}), + getPostUrl: () => 'https://site.com/post/', + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); - let options = { importing: true }; - await service.sendForPost({}, options); - sinon.assert.notCalled(stub); + await service.sendForPost( + createModel({ + status: 'published', + html: 'same', + previous: { + status: 'draft', + html: 'same', + }, + }), + { importing: true }, + ); + sinon.assert.notCalled(jobsService.dispatch); + sinon.assert.notCalled(errorLogStub); }); it('Ignores if internal context', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, + getPostData: () => ({}), + getPostUrl: () => 'https://site.com/post/', + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); - let options = { context: { internal: true } }; - await service.sendForPost({}, options); - sinon.assert.notCalled(stub); + await service.sendForPost( + createModel({ + status: 'published', + html: 'same', + previous: { + status: 'draft', + html: 'same', + }, + }), + { context: { internal: true } }, + ); + sinon.assert.notCalled(jobsService.dispatch); + sinon.assert.notCalled(errorLogStub); }); it('Ignores draft posts', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, + getPostData: () => ({}), + getPostUrl: () => 'https://site.com/post/', + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); await service.sendForPost( createModel({ status: 'draft', @@ -96,14 +134,18 @@ describe('MentionSendingService', function () { }, }), ); - sinon.assert.notCalled(stub); + sinon.assert.notCalled(jobsService.dispatch); + sinon.assert.notCalled(errorLogStub); }); it('Ignores if html was not changed', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, + getPostData: () => ({}), + getPostUrl: () => 'https://site.com/post/', + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); await service.sendForPost( createModel({ status: 'published', @@ -114,14 +156,18 @@ describe('MentionSendingService', function () { }, }), ); - sinon.assert.notCalled(stub); + sinon.assert.notCalled(jobsService.dispatch); + sinon.assert.notCalled(errorLogStub); }); it('Ignores email only posts', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, + getPostData: () => ({}), + getPostUrl: () => 'https://site.com/post/', + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); await service.sendForPost( createModel({ status: 'send', @@ -132,17 +178,18 @@ describe('MentionSendingService', function () { }, }), ); - sinon.assert.notCalled(stub); + sinon.assert.notCalled(jobsService.dispatch); + sinon.assert.notCalled(errorLogStub); }); it('Sends on publish', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, getPostData: () => ({}), getPostUrl: () => 'https://site.com/post/', - jobService: jobService, + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); await service.sendForPost( createModel({ status: 'published', @@ -153,21 +200,22 @@ describe('MentionSendingService', function () { }, }), ); - sinon.assert.calledOnce(stub); - const firstCall = stub.getCall(0).args[0]; - assert.equal(firstCall.url.toString(), 'https://site.com/post/'); - assert.equal(firstCall.html, 'same'); - assert.equal(firstCall.previousHtml, null); + sinon.assert.calledOnce(jobsService.dispatch); + const job = jobsService.dispatch.getCall(0).args[0]; + assert.ok(job instanceof SendWebmentionsJob); + assert.equal(job.sourceUrl, 'https://site.com/post/'); + assert.equal(job.html, 'same'); + assert.equal(job.previousHtml, null); }); it('Sends on unpublish', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, getPostData: () => ({}), getPostUrl: () => 'https://site.com/post/', - jobService: jobService, + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); await service.sendForPost( createModel({ status: 'draft', @@ -178,11 +226,12 @@ describe('MentionSendingService', function () { }, }), ); - sinon.assert.calledOnce(stub); - const firstCall = stub.getCall(0).args[0]; - assert.equal(firstCall.url.toString(), 'https://site.com/post/'); - assert.equal(firstCall.html, null); - assert.equal(firstCall.previousHtml, 'same'); + sinon.assert.calledOnce(jobsService.dispatch); + const job = jobsService.dispatch.getCall(0).args[0]; + assert.ok(job instanceof SendWebmentionsJob); + assert.equal(job.sourceUrl, 'https://site.com/post/'); + assert.equal(job.html, null); + assert.equal(job.previousHtml, 'same'); }); it('Resolves the url from previous data when the post was destroyed', async function () { @@ -190,13 +239,13 @@ describe('MentionSendingService', function () { // already destroyed: own attributes cleared, previous state kept. const getPostData = sinon.stub().resolves({}); const getPostUrl = sinon.stub().returns('https://site.com/gone/'); + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, getPostData, getPostUrl, - jobService: jobService, + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); const previous = { id: 'post-id', @@ -215,8 +264,8 @@ describe('MentionSendingService', function () { await service.sendForPost(destroyedPost); - sinon.assert.calledOnce(stub); - assert.equal(stub.getCall(0).args[0].url.toString(), 'https://site.com/gone/'); + sinon.assert.calledOnce(jobsService.dispatch); + assert.equal(jobsService.dispatch.getCall(0).args[0].sourceUrl, 'https://site.com/gone/'); // resolved from the destroyed model's previous data, not by loading it sinon.assert.notCalled(getPostData); assert.equal(getPostUrl.getCall(0).args[0], 'post-id'); @@ -225,13 +274,13 @@ describe('MentionSendingService', function () { }); it('Sends on html change', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, getPostData: () => ({}), getPostUrl: () => 'https://site.com/post/', - jobService: jobService, + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); await service.sendForPost( createModel({ status: 'published', @@ -242,20 +291,24 @@ describe('MentionSendingService', function () { }, }), ); - sinon.assert.calledOnce(stub); - const firstCall = stub.getCall(0).args[0]; - assert.equal(firstCall.url.toString(), 'https://site.com/post/'); - assert.equal(firstCall.html, 'updated'); - assert.equal(firstCall.previousHtml, 'same'); + sinon.assert.calledOnce(jobsService.dispatch); + const job = jobsService.dispatch.getCall(0).args[0]; + assert.ok(job instanceof SendWebmentionsJob); + assert.equal(job.sourceUrl, 'https://site.com/post/'); + assert.equal(job.html, 'updated'); + assert.equal(job.previousHtml, 'same'); }); it('Catches and logs errors', async function () { + const jobsService = { + dispatch: sinon.stub().rejects(new Error('Internal error test')), + }; const service = new MentionSendingService({ isEnabled: () => true, getPostData: () => ({}), getPostUrl: () => 'https://site.com/post/', + jobsService, }); - sinon.stub(service, 'sendForHTMLResource').rejects(new Error('Internal error test')); await service.sendForPost( createModel({ status: 'published', @@ -270,13 +323,13 @@ describe('MentionSendingService', function () { }); it('Sends no mentions for posts without html and previous html', async function () { + const jobsService = { dispatch: sinon.stub().resolves() }; const service = new MentionSendingService({ isEnabled: () => true, getPostData: () => ({}), getPostUrl: () => 'https://site.com/post/', - jobService: jobService, + jobsService, }); - const stub = sinon.stub(service, 'sendForHTMLResource'); await service.sendForPost( createModel({ status: 'published', @@ -287,7 +340,28 @@ describe('MentionSendingService', function () { }, }), ); - sinon.assert.notCalled(stub); + sinon.assert.notCalled(jobsService.dispatch); + }); + }); + + describe('sendWebmentions', function () { + it('rehydrates the source url and forwards the html fields', async function () { + const service = new MentionSendingService({}); + const stub = sinon.stub(service, 'sendForHTMLResource').resolves(); + const job = new SendWebmentionsJob({ + sourceUrl: 'https://site.com/post/', + html: 'link', + previousHtml: null, + }); + + await service.sendWebmentions(job); + + sinon.assert.calledOnce(stub); + const resource = stub.firstCall.args[0]; + assert.ok(resource.url instanceof URL, 'the source url is rehydrated into a URL'); + assert.equal(resource.url.href, 'https://site.com/post/'); + assert.equal(resource.html, job.html); + assert.equal(resource.previousHtml, null); }); }); @@ -380,7 +454,6 @@ describe('MentionSendingService', function () { discoveryService: { getEndpoint: async () => new URL('https://example.org/webmentions-test'), }, - jobService: jobService, }); await service.sendForHTMLResource({ url: new URL('https://site.com'), diff --git a/ghost/core/test/unit/server/services/mentions/send-webmentions-job.test.ts b/ghost/core/test/unit/server/services/mentions/send-webmentions-job.test.ts new file mode 100644 index 00000000000..2d23f036c66 --- /dev/null +++ b/ghost/core/test/unit/server/services/mentions/send-webmentions-job.test.ts @@ -0,0 +1,47 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'vitest'; +import SendWebmentionsJob from '../../../../../core/server/services/mentions/send-webmentions-job'; + +describe('SendWebmentionsJob', function () { + it('is dispatched under its own type', function () { + assert.equal(SendWebmentionsJob.type, 'send-webmentions'); + }); + + it('survives the round trip through the queue', function () { + const job = new SendWebmentionsJob({ + sourceUrl: 'https://site.com/post/', + html: 'link', + previousHtml: 'old link', + }); + + const revived = new SendWebmentionsJob(JSON.parse(JSON.stringify(job))); + + assert.deepEqual(revived, job); + }); + + it('round-trips a first publish, where there is no previous html', function () { + const job = new SendWebmentionsJob({ + sourceUrl: 'https://site.com/post/', + html: 'link', + previousHtml: null, + }); + + const revived = new SendWebmentionsJob(JSON.parse(JSON.stringify(job))); + + assert.deepEqual(revived, job); + assert.equal(revived.previousHtml, null); + }); + + it('round-trips an unpublish, where there is no current html', function () { + const job = new SendWebmentionsJob({ + sourceUrl: 'https://site.com/post/', + html: null, + previousHtml: 'link', + }); + + const revived = new SendWebmentionsJob(JSON.parse(JSON.stringify(job))); + + assert.deepEqual(revived, job); + assert.equal(revived.html, null); + }); +}); diff --git a/ghost/core/test/unit/server/services/mentions/service.test.js b/ghost/core/test/unit/server/services/mentions/service.test.js index 4b48d17db2d..d7ab0726b4a 100644 --- a/ghost/core/test/unit/server/services/mentions/service.test.js +++ b/ghost/core/test/unit/server/services/mentions/service.test.js @@ -2,12 +2,7 @@ const assert = require('node:assert/strict'); const sinon = require('sinon'); const urlService = require('../../../../../core/server/services/url'); const outputSerializerUrlUtil = require('../../../../../core/server/api/endpoints/utils/serializers/output/utils/url'); -const jobsService = require('../../../../../core/server/services/mentions-jobs'); -const { - getPostData, - getPostUrl, - makeLoggingJobService, -} = require('../../../../../core/server/services/mentions/service'); +const { getPostData, getPostUrl } = require('../../../../../core/server/services/mentions/service'); describe('Mentions service post url helpers', function () { afterEach(function () { @@ -82,58 +77,3 @@ describe('Mentions service post url helpers', function () { sinon.assert.notCalled(post.load); }); }); - -// Both mentions jobs are queued through this wrapper, so it has to hand the job -// manager the same job it was given: same name, same inline flag, same result, -// same error. The lifecycle logging it adds is deliberately not asserted here: -// that would mean stubbing the shared logger, which is order-dependent under the -// unit project's `isolate: false`. -describe('Mentions service background job wrapper', function () { - let addJob; - - // Runs the job the wrapper handed to the job service, the way the job - // manager runs an inline job. - function runQueuedJob() { - return addJob.firstCall.args[0].job(); - } - - beforeEach(function () { - addJob = sinon.stub(jobsService, 'addJob'); - }); - - afterEach(function () { - sinon.restore(); - }); - - it('queues the job under its own name without running it', async function () { - const fn = sinon.stub().resolves(); - - await makeLoggingJobService().addJob('processWebmention', fn); - - sinon.assert.calledOnce(addJob); - assert.equal(addJob.firstCall.args[0].name, 'processWebmention'); - assert.equal(addJob.firstCall.args[0].offloaded, false); - assert.notEqual(addJob.firstCall.args[0].job, fn, 'the job is wrapped'); - assert.ok(fn.notCalled, 'the job is not run at queue time'); - }); - - it('runs the job once and returns its result untouched', async function () { - const result = { mentions: 1 }; - const fn = sinon.stub().resolves(result); - - await makeLoggingJobService().addJob('sendWebmentions', fn); - const returned = await runQueuedJob(); - - sinon.assert.calledOnce(fn); - assert.equal(returned, result, 'the wrapped result is passed through by reference'); - }); - - it('rethrows the original error', async function () { - const failure = new Error('Job failed'); - const fn = sinon.stub().rejects(failure); - - await makeLoggingJobService().addJob('sendWebmentions', fn); - - await assert.rejects(runQueuedJob(), (error) => error === failure); - }); -}); From 6fbac17e99952f9f3d4f4ed8be5818323fc052fd Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 2 Sep 2026 14:32:07 +0100 Subject: [PATCH 3/8] Added tests pinning what the members API does with no session ref https://linear.app/ghost/issue/BER-3794/members-manage-their-own-custom-fields-in-their-account Several endpoints on the members API exist only to serve the member making the request, and each works out who that is for itself. Nothing said what they should answer when nobody is signed in, so each arrived at its own answer: reading the member gives an empty response, while changing one or clearing a suppression give a bad-request whose reason names an internal cookie. Recorded before any of it moves, so that changing an answer is a decision this file states rather than something noticed afterwards. --- .../test/e2e-api/members/session-auth.test.ts | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 ghost/core/test/e2e-api/members/session-auth.test.ts diff --git a/ghost/core/test/e2e-api/members/session-auth.test.ts b/ghost/core/test/e2e-api/members/session-auth.test.ts new file mode 100644 index 00000000000..97ee3b8f55b --- /dev/null +++ b/ghost/core/test/e2e-api/members/session-auth.test.ts @@ -0,0 +1,86 @@ +import assert from 'node:assert/strict'; + +const { agentProvider, fixtureManager } = require('../../utils/e2e-framework'); + +/** + * What the members API does when it cannot tell who is asking. + * + * Several endpoints exist only to serve the member making the request, and each + * works out who that is on its own. Nothing states what they should answer when + * nobody is signed in, so each has arrived at its own answer, and these record + * what those answers currently are. + * + * Recorded before changing any of it. Where a later change moves one of these, the + * change to this file is the statement of what was decided, rather than something + * noticed afterwards. + */ +describe('Members API session authentication', function () { + let membersAgent: { + get: (_url: string) => any; + put: (_url: string) => any; + delete: (_url: string) => any; + loginAs: (_email: string) => Promise; + duplicate: () => any; + }; + + beforeAll(async function () { + membersAgent = await agentProvider.getMembersAPIAgent(); + await fixtureManager.init('newsletters', 'members:newsletters'); + }); + + describe('with nobody signed in', function () { + // A fresh agent per case: signing in is what these are the absence of, so a + // shared one would only be signed out while these happened to run first. + const anonymous = () => membersAgent.duplicate(); + + it('answers an ask for the member themselves with nothing', async function () { + const { statusCode } = await anonymous().get('/api/member/'); + + // The one endpoint where not knowing is an ordinary answer: a themed page + // asks this on every view and most of those views have no member. + assert.equal(statusCode, 204); + }); + + it('refuses a change to the member', async function () { + const { statusCode, text } = await anonymous().put('/api/member/').body({ name: 'Nobody' }); + + // 400 rather than 401, and the reason names an internal cookie. Both are + // recorded as they are rather than as they should be, so that changing them + // is a decision this file shows. + assert.equal(statusCode, 400); + assert.match(text, /ghost-members-ssr/); + }); + + it('refuses removing an email from the suppression list', async function () { + const { statusCode, text } = await anonymous().delete('/api/member/suppression'); + + assert.equal(statusCode, 400); + assert.match(text, /ghost-members-ssr/); + }); + }); + + describe('with a member signed in', function () { + beforeAll(async function () { + await membersAgent.loginAs('session-auth@example.com'); + }); + + it('answers an ask for the member themselves', async function () { + const { body } = await membersAgent.get('/api/member/').expectStatus(200); + + assert.equal(body.email, 'session-auth@example.com'); + }); + + it('applies a change to the member', async function () { + const { body } = await membersAgent + .put('/api/member/') + .body({ name: 'Signed In' }) + .expectStatus(200); + + assert.equal(body.name, 'Signed In'); + }); + + it('accepts removing an email from the suppression list', async function () { + await membersAgent.delete('/api/member/suppression').expectStatus(204); + }); + }); +}); From 957803a935da4b9e0bb98591e64c6bc5f56c803a Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 2 Sep 2026 15:30:02 +0100 Subject: [PATCH 4/8] Changed the members API to establish who is asking, once, at the route ref https://linear.app/ghost/issue/BER-3794/members-manage-their-own-custom-fields-in-their-account Every endpoint that exists to serve the member making the request worked out who that was for itself, so each also decided for itself what to do when nobody was there, and they decided differently: reading a member answered with nothing, while changing one answered that a cookie was missing, under a status saying the request was malformed rather than that nobody was signed in. One endpoint forgot to decide at all. A route now says which it is by the gate it sits behind, and that gate establishes an identity rather than loading a member, because what to load is something only the endpoint knows: most of them need to act on a member rather than describe one, and a member read before a write is stale by the time it is answered anyway. Reading and changing a member's own record are endpoints now, so the projection of a member is reached the way every other one is, by the endpoint's name, and errors are answered the way the rest of this API already answers them. What a member may change about themselves moved out of the request handler with them, because which fields are writable is a fact about members rather than a question about HTTP. Nothing a member receives changes, and the snapshots recording it are unchanged. --- ghost/core/core/server/api/endpoints/index.js | 4 + .../server/api/endpoints/members-account.ts | 65 ++++++ .../utils/serializers/output/index.js | 4 + .../serializers/output/members-account.ts | 24 ++ .../services/members/account-service.ts | 105 +++++++++ .../members/members-api/members-api.js | 18 ++ .../server/services/members/members-ssr.js | 18 ++ .../server/services/members/middleware.js | 176 ++++++++------- .../core/server/web/members/account/index.ts | 1 + .../core/server/web/members/account/routes.ts | 59 +++++ ghost/core/core/server/web/members/app.js | 22 +- .../e2e-api/members/member-projection.test.ts | 7 +- .../test/e2e-api/members/session-auth.test.ts | 210 +++++++++++++++--- 13 files changed, 578 insertions(+), 135 deletions(-) create mode 100644 ghost/core/core/server/api/endpoints/members-account.ts create mode 100644 ghost/core/core/server/api/endpoints/utils/serializers/output/members-account.ts create mode 100644 ghost/core/core/server/services/members/account-service.ts create mode 100644 ghost/core/core/server/web/members/account/index.ts create mode 100644 ghost/core/core/server/web/members/account/routes.ts diff --git a/ghost/core/core/server/api/endpoints/index.js b/ghost/core/core/server/api/endpoints/index.js index 3453f3ac16e..0eb04ea2632 100644 --- a/ghost/core/core/server/api/endpoints/index.js +++ b/ghost/core/core/server/api/endpoints/index.js @@ -312,6 +312,10 @@ module.exports = { return apiFramework.pipeline(require('./feedback-members'), localUtils, 'members'); }, + get membersAccount() { + return apiFramework.pipeline(require('./members-account'), localUtils, 'members'); + }, + get giftsMembers() { return apiFramework.pipeline(require('./gifts-members'), localUtils, 'members'); }, diff --git a/ghost/core/core/server/api/endpoints/members-account.ts b/ghost/core/core/server/api/endpoints/members-account.ts new file mode 100644 index 00000000000..85b6c355633 --- /dev/null +++ b/ghost/core/core/server/api/endpoints/members-account.ts @@ -0,0 +1,65 @@ +const membersService = require('../../services/members'); + +/** + * A member's own record, as the member themselves. + * + * The route establishes who is asking before this runs, and answers for an + * unknown caller itself, so there is always a member here. What to load about them + * is decided here rather than there, because this is what knows what it renders. + * + * `docName` is this endpoint's own, not the Admin API's `members`, because the two + * describe the same record to different audiences and a serializer is chosen by + * that name. Sharing it would mean sharing the shape. + */ + +interface Frame { + data: Record; + options: { + context?: { member?: { id: string; email: string } | null }; + }; +} + +const memberOf = (frame: Frame) => frame.options?.context?.member ?? null; + +const controller = { + docName: 'members_account', + + read: { + headers: { cacheInvalidate: false }, + permissions: false, + query(frame: Frame) { + return membersService.api.account.read(memberOf(frame)!.id); + }, + }, + + /** + * Let Ghost email this member again. + * + * Under the account rather than as a resource of its own: being emailable is a + * fact about a member, and the only person who can restore it here is the member + * themselves. + */ + destroySuppression: { + statusCode: 204, + headers: { cacheInvalidate: false }, + permissions: false, + query(frame: Frame) { + const member = memberOf(frame)!; + return membersService.api.account.allowEmail(member.id, member.email); + }, + }, + + // `update` rather than `edit`: the framework reserves `edit` for the Admin API's + // enveloped bodies, and this request has always carried a bare one. The + // members-facing gift endpoints name their own verbs for the same reason. + update: { + headers: { cacheInvalidate: false }, + permissions: false, + query(frame: Frame) { + return membersService.api.account.edit(frame.data, memberOf(frame)!.id); + }, + }, +}; + +export default controller; +module.exports = controller; diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/output/index.js b/ghost/core/core/server/api/endpoints/utils/serializers/output/index.js index 337da6e8a94..ca20630fb5c 100644 --- a/ghost/core/core/server/api/endpoints/utils/serializers/output/index.js +++ b/ghost/core/core/server/api/endpoints/utils/serializers/output/index.js @@ -20,6 +20,10 @@ module.exports = { return require('./member-commenting'); }, + get members_account() { + return require('./members-account'); + }, + get authentication() { return require('./authentication'); }, diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/output/members-account.ts b/ghost/core/core/server/api/endpoints/utils/serializers/output/members-account.ts new file mode 100644 index 00000000000..09dc90c88b9 --- /dev/null +++ b/ghost/core/core/server/api/endpoints/utils/serializers/output/members-account.ts @@ -0,0 +1,24 @@ +const { formattedMemberResponse } = require('../../../../../services/members/utils'); + +interface Frame { + response?: unknown; +} + +/** + * A member's own record, as this API has always written one down. + * + * The projection itself is unchanged and still lives beside the other member + * formatting; what changes is that it is now reached the way every other + * serializer is, by this endpoint's `docName`, rather than being called by hand + * from a request handler. + */ +const serialize = (member: unknown, _apiConfig: unknown, frame: Frame): void => { + frame.response = formattedMemberResponse(member); +}; + +// The API framework loads this file with `require()`, so it exports CommonJS-style; +// `export default` would not be picked up. +module.exports = { + read: serialize, + update: serialize, +}; diff --git a/ghost/core/core/server/services/members/account-service.ts b/ghost/core/core/server/services/members/account-service.ts new file mode 100644 index 00000000000..57c6d6051b9 --- /dev/null +++ b/ghost/core/core/server/services/members/account-service.ts @@ -0,0 +1,105 @@ +import _ from 'lodash'; + +/** + * A member's own account: what they are shown about themselves, and what they may + * change. + * + * Separate from the request handlers that used to hold this, because none of it is + * a question about HTTP. Which fields a member may set is a fact about members, and + * the answer is the same whoever is asking. + * + * Takes a member id rather than a member. Everything here either writes or reads + * afresh, and a record loaded before a write is stale by the time the answer is + * built, so there is nothing a caller could usefully hand over. + */ + +/** What a member is allowed to change about themselves. */ +const WRITABLE_FIELDS = [ + 'name', + 'expertise', + 'subscribed', + 'newsletters', + 'enable_comment_notifications', + 'enable_updates_and_announcements', +] as const; + +/** + * The relations a write needs loaded to work out what it is changing. + * + * Newsletters because the older `subscribed` flag is stored as a list of them, and + * the subscription chain because changing what a member is entitled to has to + * reconcile against what they are paying for. + */ +const WRITE_RELATIONS = [ + 'stripeSubscriptions', + 'stripeSubscriptions.customer', + 'stripeSubscriptions.stripePrice', + 'newsletters', +]; + +interface MemberBreadService { + read( + data: { id: string }, + options?: Record, + ): Promise | null>; +} + +interface MemberRepository { + update(data: Record, options: Record): Promise; + update(data: Record, options: Record): Promise; +} + +interface EmailSuppressionList { + removeEmail(email: string): Promise; +} + +export interface MemberAccountServiceDeps { + memberBREADService: MemberBreadService; + members: MemberRepository; + emailSuppressionList: EmailSuppressionList; +} + +export class MemberAccountService { + #memberBREADService: MemberBreadService; + #members: MemberRepository; + #emailSuppressionList: EmailSuppressionList; + + constructor({ memberBREADService, members, emailSuppressionList }: MemberAccountServiceDeps) { + this.#memberBREADService = memberBREADService; + this.#members = members; + this.#emailSuppressionList = emailSuppressionList; + } + + /** Everything a member is shown about themselves. */ + async read(memberId: string): Promise | null> { + // Without the extra fields a publisher defines: nothing in this projection + // carries them yet, so fetching them would cost a query per request and be + // discarded. The session read leaves them off for the same reason. + return this.#memberBREADService.read({ id: memberId }, { withCustomFields: false }); + } + + /** Apply what a member asked to change about themselves, and say what they now hold. */ + async edit(data: Record, memberId: string) { + await this.#members.update(_.pick(data, WRITABLE_FIELDS), { + id: memberId, + withRelated: WRITE_RELATIONS, + }); + + // Read back rather than returning what was written: a member is told what + // Ghost now holds, which is not always what they sent. Setting the older + // `subscribed` flag, for one, is stored as a list of newsletters. + return this.read(memberId); + } + + /** + * Let Ghost email this member again. + * + * Two records rather than one: the address is on a list the email provider also + * writes to, and the member carries a flag of their own. A member asking to hear + * from a site again means both. + */ + async allowEmail(memberId: string, email: string): Promise { + await this.#emailSuppressionList.removeEmail(email); + await this.#members.update({ email_disabled: false }, { id: memberId }); + } +} diff --git a/ghost/core/core/server/services/members/members-api/members-api.js b/ghost/core/core/server/services/members/members-api/members-api.js index 88fbd9c2937..8a110ccf677 100644 --- a/ghost/core/core/server/services/members/members-api/members-api.js +++ b/ghost/core/core/server/services/members/members-api/members-api.js @@ -7,6 +7,7 @@ const PaymentsService = require('./services/payments-service'); const TokenService = require('./services/token-service'); const GeolocationService = require('./services/geolocation-service'); const MemberBREADService = require('./services/member-bread-service'); +const { MemberAccountService } = require('../account-service'); const MemberRepository = require('./repositories/member-repository'); const NextPaymentCalculator = require('./services/next-payment-calculator'); @@ -359,6 +360,21 @@ module.exports = function MembersAPI({ return memberBREADService.read({ email }, { withCustomFields: false }); } + const account = new MemberAccountService({ + memberBREADService, + members: users, + emailSuppressionList, + }); + + async function getMemberIdentity(transientId) { + if (!transientId) { + return null; + } + + const member = await users.get({ transient_id: transientId }); + return member ? { id: member.id, email: member.get('email') } : null; + } + async function getMemberIdentityDataFromTransientId(transientId) { return memberBREADService.read({ transient_id: transientId }, { withCustomFields: false }); } @@ -500,6 +516,7 @@ module.exports = function MembersAPI({ getMemberIdentityToken, getMemberEntitlementToken, getMemberIdentityDataFromTransientId, + getMemberIdentity, getMemberIdentityData, cycleTransientId, setMemberGeolocationFromIp, @@ -508,6 +525,7 @@ module.exports = function MembersAPI({ sendEmailWithMagicLink, getMagicLink, members: users, + account, memberBREADService, events: eventRepository, productRepository, diff --git a/ghost/core/core/server/services/members/members-ssr.js b/ghost/core/core/server/services/members/members-ssr.js index fb6c437c0f0..26ad46e9a07 100644 --- a/ghost/core/core/server/services/members/members-ssr.js +++ b/ghost/core/core/server/services/members/members-ssr.js @@ -307,6 +307,24 @@ class MembersSSR { * * @returns {Promise} */ + /** + * Who this session belongs to, without loading them. + * + * The counterpart to `getMemberDataFromSession`, for callers that need to know a + * member is there rather than what is on their record. + * + * @method getMemberIdentityFromSession + * @param {Request} req + * @param {Response} res + * + * @returns {Promise<{id: string, email: string} | null>} + */ + async getMemberIdentityFromSession(req, res) { + const transientId = this._getSessionCookies(req, res); + const api = await this._getMembersApi(); + return api.getMemberIdentity(transientId); + } + async getMemberDataFromSession(req, res) { const transientId = this._getSessionCookies(req, res); const member = await this._getMemberIdentityDataFromTransientId(transientId); diff --git a/ghost/core/core/server/services/members/middleware.js b/ghost/core/core/server/services/members/middleware.js index bb0034a0d43..9c23263e9d3 100644 --- a/ghost/core/core/server/services/members/middleware.js +++ b/ghost/core/core/server/services/members/middleware.js @@ -2,11 +2,10 @@ const crypto = require('crypto'); const _ = require('lodash'); const logging = require('@tryghost/logging'); const membersService = require('./service'); -const emailSuppressionList = require('../email-suppression-list'); const models = require('../../models'); const urlUtils = require('../../../shared/url-utils').default; const spamPrevention = require('../../web/shared/middleware/api/spam-prevention'); -const { formattedMemberResponse, formatNewsletterResponse } = require('./utils'); +const { formatNewsletterResponse } = require('./utils'); const errors = require('@tryghost/errors'); const tpl = require('@tryghost/tpl'); const onHeaders = require('on-headers'); @@ -18,6 +17,9 @@ const messages = { missingUuid: 'Missing uuid.', invalidUuid: 'Invalid uuid.', invalidKey: 'Invalid key.', + // Says that nobody is signed in, without naming the cookie that would have said + // so. Which cookie carries a session is Ghost's business, not the caller's. + noMemberSession: 'You must be signed in to do this.', }; const getFreeTier = async function getFreeTier() { @@ -116,6 +118,94 @@ const getRedirectUrl = function getRedirectUrl({ action, referrer, searchParams, return redirectUrl.href; }; +/** + * Establish who is signed in, without loading them. + * + * Sets `req.identity` to an id and an email address, or to null when nobody is + * signed in. One lookup against an indexed column, with no relations attached and + * nothing fetched from outside Ghost. + * + * Deliberately not the member's record. Most endpoints that serve a member need to + * act on one rather than describe one, and the one that describes a member reads + * it again after writing anyway, because the response has to say what Ghost now + * holds rather than what was sent. What to load is the endpoint's own business. + * + * Distinct from `loadMemberSession`, which loads the whole member and is what a + * themed page renders from. + */ +const loadMemberIdentity = async function loadMemberIdentity(req, res, next) { + try { + Object.assign(req, { + identity: await membersService.ssr.getMemberIdentityFromSession(req, res), + }); + } catch (err) { + // Only a missing or unreadable cookie means nobody is signed in. Establishing + // an identity also reaches the database, and answering "you are not signed in" + // to that failing would tell a signed-in member something false and leave the + // real fault unreported. + if (!errors.utils.isGhostError(err) || err.errorType !== 'BadRequestError') { + return next(err); + } + Object.assign(req, { identity: null }); + } + + return next(); +}; + +/** + * Hand the identity to whatever comes next, in the place the API framework reads. + * + * The framework takes whoever is acting from `req.member` and nowhere else, so a + * controller behind either gate below finds the identity there. + */ +const carryIdentity = function carryIdentity(req, res) { + Object.assign(req, { member: req.identity }); + res.locals.member = req.member; +}; + +/** + * Refuse a request that `loadMemberIdentity` could not put a name to. + * + * For endpoints where an unknown caller is asking for something they have no right + * to: changing a member's record, or reading what a publisher collects. + * + * Reads only `req.identity`, so a route using this must register + * `loadMemberIdentity` first. It looks nothing up itself: what a route needs is + * declared by the middleware it lists, rather than depending on what some earlier + * one happened to leave behind. + */ +const rejectWhenAnonymous = function rejectWhenAnonymous(req, res, next) { + if (!req.identity) { + return next( + new errors.UnauthorizedError({ + message: tpl(messages.noMemberSession), + }), + ); + } + + carryIdentity(req, res); + return next(); +}; + +/** + * Answer with nothing when `loadMemberIdentity` could not put a name to the request. + * + * For reading who you are, where not knowing is an ordinary answer rather than a + * failure: a themed page asks on every view and most of those views have no member. + * + * The counterpart to `rejectWhenAnonymous`, and separate from it because which of + * the two applies is a fact about the endpoint, not a setting on a shared one. + */ +const emptyWhenAnonymous = function emptyWhenAnonymous(req, res, next) { + if (!req.identity) { + res.writeHead(204); + return res.end(); + } + + carryIdentity(req, res); + return next(); +}; + /** * Require member authentication, and make it possible to authenticate via uuid + hashed key. * You can chain this after loadMemberSession to make it possible to authenticate via both the uuid and the session. @@ -243,42 +333,6 @@ const deleteSession = async function deleteSession(req, res) { } }; -const getMemberData = async function getMemberData(req, res) { - try { - const member = await membersService.ssr.getMemberDataFromSession(req, res); - if (member) { - res.json(formattedMemberResponse(member)); - } else { - res.json(null); - } - } catch (err) { - res.writeHead(204); - res.end(); - } -}; - -const deleteSuppression = async function deleteSuppression(req, res) { - try { - const member = await membersService.ssr.getMemberDataFromSession(req, res); - const options = { - id: member.id, - }; - await emailSuppressionList.removeEmail(member.email); - await membersService.api.members.update({ email_disabled: false }, options); - - res.writeHead(204); - res.end(); - } catch (err) { - if (!err.statusCode) { - logging.error(err); - } - res.writeHead(err.statusCode ?? 500, { - 'Content-Type': 'text/plain;charset=UTF-8', - }); - res.end(err.message); - } -}; - const getMemberNewsletters = async function getMemberNewsletters(req, res) { try { const memberData = req.member; // validation assumed @@ -351,46 +405,6 @@ const updateMemberNewsletters = async function updateMemberNewsletters(req, res) } }; -const updateMemberData = async function updateMemberData(req, res) { - try { - const data = _.pick( - req.body, - 'name', - 'expertise', - 'subscribed', - 'newsletters', - 'enable_comment_notifications', - 'enable_updates_and_announcements', - ); - const member = await membersService.ssr.getMemberDataFromSession(req, res); - if (member) { - const options = { - id: member.id, - withRelated: [ - 'stripeSubscriptions', - 'stripeSubscriptions.customer', - 'stripeSubscriptions.stripePrice', - 'newsletters', - ], - }; - await membersService.api.members.update(data, options); - const updatedMember = await membersService.ssr.getMemberDataFromSession(req, res); - - res.json(formattedMemberResponse(updatedMember)); - } else { - res.json(null); - } - } catch (err) { - if (!err.statusCode) { - logging.error(err); - } - res.writeHead(err.statusCode ?? 500, { - 'Content-Type': 'text/plain;charset=UTF-8', - }); - res.end(err.message); - } -}; - const createSessionFromMagicLink = async function createSessionFromMagicLink(req, res, next) { if (!req.url.includes('token=')) { return next(); @@ -500,17 +514,17 @@ const createSessionFromMagicLink = async function createSessionFromMagicLink(req // Set req.member & res.locals.member if a cookie is set module.exports = { loadMemberSession, + loadMemberIdentity, + rejectWhenAnonymous, + emptyWhenAnonymous, authMemberByUuid, createSessionFromMagicLink, getIdentityToken, getEntitlementToken, getMemberNewsletters, - getMemberData, - updateMemberData, updateMemberNewsletters, deleteSession, accessInfoSession, - deleteSuppression, createIntegrityToken, verifyIntegrityToken, }; diff --git a/ghost/core/core/server/web/members/account/index.ts b/ghost/core/core/server/web/members/account/index.ts new file mode 100644 index 00000000000..3b3a4503eb8 --- /dev/null +++ b/ghost/core/core/server/web/members/account/index.ts @@ -0,0 +1 @@ +module.exports = require('./routes'); diff --git a/ghost/core/core/server/web/members/account/routes.ts b/ghost/core/core/server/web/members/account/routes.ts new file mode 100644 index 00000000000..8d37db33342 --- /dev/null +++ b/ghost/core/core/server/web/members/account/routes.ts @@ -0,0 +1,59 @@ +const express = require('../../../../shared/express'); +const bodyParser = require('body-parser'); +const config = require('../../../../shared/config'); +const { http } = require('@tryghost/api-framework'); +const api = require('../../../api').endpoints; +const middleware = require('../../../services/members/middleware'); + +/** + * A member's own account, over HTTP. + * + * Everything reached by a signed-in member acting on their own record lives here, + * so how these routes authenticate is a property of the module rather than a line + * each one repeats. Establishing who is asking happens once; each route then says + * what it wants done when there is nobody. + * + * Endpoints reached by someone who is not signed in stay outside this: the + * unsubscribe links identify a member by a signed link, and changing an email + * address by a token in the body, because in both cases the person following them + * has no session. + */ +module.exports = function accountRoutes() { + const router = express.Router('member-account'); + + router.use(middleware.loadMemberIdentity); + + // Reading who you are. Answering with nothing when there is no member is the + // right answer rather than a failure: a themed page asks this on every view. + if (config.get('cacheMembersContent:enabled')) { + router.get( + '/', + // Not for this route's benefit. This configuration stamps a cookie saying + // what the reader is entitled to, so a cache can vary on it, and that needs + // the member's subscriptions rather than only their identity. It rides here + // because Portal asks this endpoint on every page load. + middleware.loadMemberSession, + middleware.accessInfoSession, + middleware.emptyWhenAnonymous, + http(api.membersAccount.read), + ); + } else { + router.get('/', middleware.emptyWhenAnonymous, http(api.membersAccount.read)); + } + + router.put( + '/', + bodyParser.json({ limit: '50mb' }), + middleware.rejectWhenAnonymous, + http(api.membersAccount.update), + ); + + // Letting Ghost email this member again after it stopped. + router.delete( + '/suppression', + middleware.rejectWhenAnonymous, + http(api.membersAccount.destroySuppression), + ); + + return router; +}; diff --git a/ghost/core/core/server/web/members/app.js b/ghost/core/core/server/web/members/app.js index 386712d4614..5feb3439aef 100644 --- a/ghost/core/core/server/web/members/app.js +++ b/ghost/core/core/server/web/members/app.js @@ -8,10 +8,10 @@ const audienceFeedbackService = require('../../services/audience-feedback'); const middleware = membersService.middleware; const shared = require('../shared'); const errorHandler = require('@tryghost/mw-error-handler'); -const config = require('../../../shared/config'); const { http } = require('@tryghost/api-framework'); const api = require('../../api').endpoints; +const accountRoutes = require('./account'); const commentRouter = require('../comments'); const announcementRouter = require('../announcement'); const corsMiddleware = require('./middleware/cors'); @@ -57,21 +57,9 @@ module.exports = function setupMembersApp() { middleware.updateMemberNewsletters, ); - // Get and update member data - // Caching members content is an experimental feature - const shouldCacheMembersContent = config.get('cacheMembersContent:enabled'); - if (shouldCacheMembersContent) { - membersApp.get( - '/api/member', - middleware.loadMemberSession, - middleware.accessInfoSession, - middleware.getMemberData, - ); - } else { - membersApp.get('/api/member', middleware.getMemberData); - } - - membersApp.put('/api/member', bodyParser.json({ limit: '50mb' }), middleware.updateMemberData); + // A member's own account. Its routes and how they authenticate live together. + membersApp.use('/api/member', accountRoutes()); + membersApp.post('/api/member/email', bodyParser.json({ limit: '50mb' }), (req, res, next) => membersService.api.middleware.updateEmailAddress(req, res, next), ); @@ -86,8 +74,6 @@ module.exports = function setupMembersApp() { ); // Remove email from suppression list - membersApp.delete('/api/member/suppression', middleware.deleteSuppression); - // Manage session membersApp.get('/api/session', middleware.getIdentityToken); membersApp.delete('/api/session', bodyParser.json({ limit: '5mb' }), middleware.deleteSession); diff --git a/ghost/core/test/e2e-api/members/member-projection.test.ts b/ghost/core/test/e2e-api/members/member-projection.test.ts index 07ec092f229..42ad5b1dacd 100644 --- a/ghost/core/test/e2e-api/members/member-projection.test.ts +++ b/ghost/core/test/e2e-api/members/member-projection.test.ts @@ -172,10 +172,9 @@ describe('Members API member projection', function () { const { statusCode } = await signedOut.get('/api/member/'); - // 204, and not by the route deciding so: resolving the session throws when - // there is none, and the handler's catch answers empty. Worth pinning - // because the deliberate no-session branch a few lines above it returns 200 - // with a null body, and only one of the two is what anyone actually sees. + // The route decides this, rather than it falling out of a handler failing: + // reading who you are is the one place where not knowing is an ordinary + // answer, because a themed page asks on every view. assert.equal(statusCode, 204); }); diff --git a/ghost/core/test/e2e-api/members/session-auth.test.ts b/ghost/core/test/e2e-api/members/session-auth.test.ts index 97ee3b8f55b..ca9847f9dc6 100644 --- a/ghost/core/test/e2e-api/members/session-auth.test.ts +++ b/ghost/core/test/e2e-api/members/session-auth.test.ts @@ -1,31 +1,100 @@ import assert from 'node:assert/strict'; const { agentProvider, fixtureManager } = require('../../utils/e2e-framework'); +const DomainEvents = require('@tryghost/domain-events'); +const { + EmailBouncedEvent, +} = require('../../../core/server/services/email-service/events/email-bounced-event'); + +const SIGNED_IN_EMAIL = 'session-auth@example.com'; +const ANONYMOUS_TARGET_EMAIL = 'session-auth-suppressed@example.com'; + +interface Agent { + get: (_url: string) => any; + put: (_url: string) => any; + post: (_url: string) => any; + delete: (_url: string) => any; +} + +interface MembersAgent extends Agent { + loginAs: (_email: string) => Promise; + duplicate: () => MembersAgent; +} + +interface AdminAgent extends Agent { + loginAsOwner: () => Promise; +} /** * What the members API does when it cannot tell who is asking. * - * Several endpoints exist only to serve the member making the request, and each - * works out who that is on its own. Nothing states what they should answer when - * nobody is signed in, so each has arrived at its own answer, and these record - * what those answers currently are. + * Endpoints that exist only to serve the member making the request now say so, by + * sitting behind a gate that resolves the member and answers for them when there + * is none. How it answers is the endpoint's choice, and there are two: * - * Recorded before changing any of it. Where a later change moves one of these, the - * change to this file is the statement of what was decided, rather than something - * noticed afterwards. + * Reading the member is answered with nothing, because a themed page asks on every + * view and most of those views have no member. Everything else is refused, because + * changing a member's record is not something an unknown caller has a right to. + * + * Every fact these tests set up or check is set up and checked through an API: the + * Admin API stands in for the publisher, the members API for the member. The one + * exception is putting an address on the email suppression list, which no API can + * do — see `suppressEmail`. */ describe('Members API session authentication', function () { - let membersAgent: { - get: (_url: string) => any; - put: (_url: string) => any; - delete: (_url: string) => any; - loginAs: (_email: string) => Promise; - duplicate: () => any; - }; + let membersAgent: MembersAgent; + let adminAgent: AdminAgent; + + /** + * Put an address on the email suppression list, the way Ghost itself does. + * + * No API can do this. The Admin API takes an address off the list + * (`DELETE /members/:id/suppression`) but nothing puts one on: an address gets + * there only when the email provider reports a permanent bounce or a spam + * complaint, and Ghost hears about that as a domain event. Dispatching that + * event is the same entry point the provider's report arrives through, so this + * is setup through Ghost rather than around it — no row is written here and + * nothing depends on the shape of the table. + * + * Being unemailable is two records, and the real path sets both: the address + * joins a list the provider also writes to, and the member picks up a flag of + * their own. Everything below observes the pair through the Admin API, which + * reports a member as suppressed while either is set. + */ + async function suppressEmail(email: string, memberId: string) { + DomainEvents.dispatch( + EmailBouncedEvent.create({ + email, + memberId, + // No email was really sent, and nothing here is about which one it was. + emailId: null, + emailRecipientId: null, + // Ghost suppresses an address on a permanent failure and ignores the rest, + // so a code it acts on is the situation this is setting up. + error: { message: 'Recipient address rejected', code: 607 }, + }), + ); + await DomainEvents.allSettled(); + } + + async function readMemberAsStaff(email: string) { + const { body } = await adminAgent.get(`members/?filter=email:'${email}'`).expectStatus(200); + assert.equal(body.members.length, 1, `exactly one member holds ${email}`); + return body.members[0]; + } + + /** Every member the site holds, by name, as staff can see them. */ + async function memberNamesAsStaff(): Promise> { + const { body } = await adminAgent.get('members/?limit=all').expectStatus(200); + return Object.fromEntries( + body.members.map((member: { id: string; name: string }) => [member.id, member.name]), + ); + } beforeAll(async function () { - membersAgent = await agentProvider.getMembersAPIAgent(); + ({ adminAgent, membersAgent } = await agentProvider.getAgentsForMembers()); await fixtureManager.init('newsletters', 'members:newsletters'); + await adminAgent.loginAsOwner(); }); describe('with nobody signed in', function () { @@ -34,53 +103,130 @@ describe('Members API session authentication', function () { const anonymous = () => membersAgent.duplicate(); it('answers an ask for the member themselves with nothing', async function () { - const { statusCode } = await anonymous().get('/api/member/'); + // The site has members. Without that, an empty answer would only be saying + // the site is empty rather than that this request names nobody. + assert.ok( + Object.keys(await memberNamesAsStaff()).length > 0, + 'the site holds members this could have answered with', + ); + + const { statusCode, text } = await anonymous().get('/api/member/'); // The one endpoint where not knowing is an ordinary answer: a themed page // asks this on every view and most of those views have no member. assert.equal(statusCode, 204); + // And nothing in the body, because "no content" that carried a member would + // be handing one out to a caller who named none. + assert.equal(text, ''); }); - it('refuses a change to the member', async function () { - const { statusCode, text } = await anonymous().put('/api/member/').body({ name: 'Nobody' }); - - // 400 rather than 401, and the reason names an internal cookie. Both are - // recorded as they are rather than as they should be, so that changing them - // is a decision this file shows. - assert.equal(statusCode, 400); - assert.match(text, /ghost-members-ssr/); + it('refuses a change to the member, and changes nothing', async function () { + // An anonymous request names no member, so what it must not do is change any + // of them. Every member is recorded, not one, because a write that reached + // some other member than the one a test happened to pick would still be a + // write that should not have happened. + const before = await memberNamesAsStaff(); + assert.ok(Object.keys(before).length > 0, 'there are members a write could have reached'); + + const { statusCode, body } = await anonymous().put('/api/member/').body({ name: 'Nobody' }); + + // Refused as unauthorized, which it is. This used to be a bad-request whose + // reason named the session cookie, telling a caller both the wrong thing and + // more than it needed to know. + assert.equal(statusCode, 401); + assert.match(body.errors[0].message, /you must be signed in/i); + assert.ok( + !JSON.stringify(body).includes('ghost-members-ssr'), + 'the refusal does not name the cookie a session is carried in', + ); + + // Refusing is only half of it: a refusal that still wrote would pass on the + // status alone. + assert.deepEqual(await memberNamesAsStaff(), before); }); - it('refuses removing an email from the suppression list', async function () { - const { statusCode, text } = await anonymous().delete('/api/member/suppression'); - - assert.equal(statusCode, 400); - assert.match(text, /ghost-members-ssr/); + it('refuses removing an email from the suppression list, and removes nothing', async function () { + // A member of its own, so this does not depend on the member these tests + // sign in as, which does not exist until they do. + const { body: created } = await adminAgent + .post('members/') + .body({ members: [{ email: ANONYMOUS_TARGET_EMAIL, name: 'Suppressed' }] }) + .expectStatus(201); + await suppressEmail(ANONYMOUS_TARGET_EMAIL, created.members[0].id); + assert.equal( + (await readMemberAsStaff(ANONYMOUS_TARGET_EMAIL)).email_suppression.suppressed, + true, + 'the member starts out unemailable, so there is something to remove', + ); + + const { statusCode, body } = await anonymous().delete('/api/member/suppression'); + + assert.equal(statusCode, 401); + assert.match(body.errors[0].message, /you must be signed in/i); + assert.ok( + !JSON.stringify(body).includes('ghost-members-ssr'), + 'the refusal does not name the cookie a session is carried in', + ); + + assert.equal( + (await readMemberAsStaff(ANONYMOUS_TARGET_EMAIL)).email_suppression.suppressed, + true, + 'the member is still unemailable', + ); }); }); describe('with a member signed in', function () { beforeAll(async function () { - await membersAgent.loginAs('session-auth@example.com'); + await membersAgent.loginAs(SIGNED_IN_EMAIL); }); it('answers an ask for the member themselves', async function () { const { body } = await membersAgent.get('/api/member/').expectStatus(200); - assert.equal(body.email, 'session-auth@example.com'); + assert.equal(body.email, SIGNED_IN_EMAIL); + // The same record staff are looking at, rather than something that merely + // carries the right address. + assert.equal(body.uuid, (await readMemberAsStaff(SIGNED_IN_EMAIL)).uuid); }); it('applies a change to the member', async function () { + const before = await readMemberAsStaff(SIGNED_IN_EMAIL); + assert.notEqual(before.name, 'Signed In', 'the name is not already what this sets it to'); + const { body } = await membersAgent .put('/api/member/') .body({ name: 'Signed In' }) .expectStatus(200); assert.equal(body.name, 'Signed In'); + + // Asked again rather than trusting the response. That the response happens + // to be a fresh read is this endpoint's business, not something a test of + // what it stores should rely on, and staff reading the same record is what + // says it was stored rather than echoed. + assert.equal((await readMemberAsStaff(SIGNED_IN_EMAIL)).name, 'Signed In'); }); - it('accepts removing an email from the suppression list', async function () { + it('lets Ghost email a member again after it stopped', async function () { + const member = await readMemberAsStaff(SIGNED_IN_EMAIL); + await suppressEmail(SIGNED_IN_EMAIL, member.id); + assert.equal( + (await readMemberAsStaff(SIGNED_IN_EMAIL)).email_suppression.suppressed, + true, + 'the member starts out unemailable, so there is something to undo', + ); + await membersAgent.delete('/api/member/suppression').expectStatus(204); + + // The status alone would pass whether or not anything was undone, which is + // the whole point of the endpoint. Staff are told a member is suppressed + // while either half of it holds, so this reading false is both halves gone. + assert.equal( + (await readMemberAsStaff(SIGNED_IN_EMAIL)).email_suppression.suppressed, + false, + 'the member is emailable again', + ); }); }); }); From 11b94d81dc5a3e1fae820a477ee4429221ad1b2d Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 2 Sep 2026 11:58:26 +0100 Subject: [PATCH 5/8] Added the entry point a custom field read is made through ref https://linear.app/ghost/issue/BER-3794/members-manage-their-own-custom-fields-in-their-account Which of the extra fields a publisher defines about their members may be seen, and by whom, is about to stop being the same answer everywhere: staff and the member themselves will be told different things about the same field. Nothing in the domain could express that, because nothing recorded which side of Ghost was asking. The read carried a boolean instead, meaning only whether to bother fetching at all, which is a question about cost rather than about access. This replaces it with the audience making the request, so the decision has somewhere to live before there is a rule to put there. Every answer it gives today is the one Ghost already gives. The argument is required rather than defaulted, which is what makes a new caller state which door it came through instead of inheriting whatever the last author assumed, and adding it turned the one existing caller that had never said so, the value collected at checkout, into a compile error until it did. --- .../services/members-custom-fields/access.ts | 44 +++++++++++++++++++ .../members-custom-fields/bindings-service.ts | 9 +++- .../definitions-service.ts | 8 +++- .../services/members-custom-fields/index.ts | 5 +++ .../services/members-custom-fields/schema.ts | 4 ++ .../members-custom-fields/values-service.ts | 16 ++++++- .../members/members-api/members-api.js | 4 +- .../services/member-bread-service.js | 28 +++++++----- .../members/members-api/members-api.test.js | 2 +- .../services/members-bread-service.test.js | 2 +- 10 files changed, 102 insertions(+), 20 deletions(-) create mode 100644 ghost/core/core/server/services/members-custom-fields/access.ts diff --git a/ghost/core/core/server/services/members-custom-fields/access.ts b/ghost/core/core/server/services/members-custom-fields/access.ts new file mode 100644 index 00000000000..e43d5223b32 --- /dev/null +++ b/ghost/core/core/server/services/members-custom-fields/access.ts @@ -0,0 +1,44 @@ +/** + * Who is asking, in the only terms that decide what they may see or change. + * + * Not which user, but which door they came through. Staff reaching a member + * through the Admin API and a member reaching their own record through Portal are + * asking different questions about the same fields, and the answers will differ + * per field once a publisher can say so. Naming the door gives that decision + * somewhere to live. + * + * `internal` is neither: an importer, a value collected at checkout, a job. Those + * act on nobody's behalf and are bounded by whatever set them off rather than by + * who is looking. + * + * A member's own entry carries no id. Which member is asking is already settled by + * the query, which is scoped to them, so repeating it here would be a second answer + * to a question that is already decided and could disagree with the first. + */ +export type Audience = { entry: 'admin' } | { entry: 'members' } | { entry: 'internal' }; + +export const ADMIN: Audience = { entry: 'admin' }; +export const MEMBERS: Audience = { entry: 'members' }; +export const INTERNAL: Audience = { entry: 'internal' }; + +/** + * Which of the site's fields this audience may see. + * + * Every field, whichever door they came through. Written as a function rather than + * left unwritten so that when a publisher can mark a field as staff only, one + * function changes and every caller is already asking. + */ +export function readableFields(_audience: Audience, fields: T[]): T[] { + return fields; +} + +/** + * Whether this audience may write this field. + * + * Also total today. Kept separate from `readableFields` because the asymmetry to + * expect is a field a member may read but not change, which one combined + * permission could not express. + */ +export function canWrite(_audience: Audience, _field: { key: string }): boolean { + return true; +} diff --git a/ghost/core/core/server/services/members-custom-fields/bindings-service.ts b/ghost/core/core/server/services/members-custom-fields/bindings-service.ts index 21dcc23ff5a..eee6bb3a8f7 100644 --- a/ghost/core/core/server/services/members-custom-fields/bindings-service.ts +++ b/ghost/core/core/server/services/members-custom-fields/bindings-service.ts @@ -2,6 +2,7 @@ import ObjectID from 'bson-objectid'; import logging from '@tryghost/logging'; import type { Knex } from 'knex'; import type { FieldType } from '@tryghost/custom-field-types'; +import { INTERNAL } from './access'; import { DbBoundField, FIELD_STATUS } from './schema'; import type { CustomFieldValuesService, PlannedWrite } from './values-service'; @@ -83,7 +84,13 @@ export class CustomFieldBindingsService { private async writeOne(memberId: string, into: BoundField, value: unknown): Promise { let planned: PlannedWrite[]; try { - planned = await this.values.planWrite({ [`${CUSTOM_NAMESPACE}.${into.key}`]: value }); + // Internal: a value collected by Stripe at checkout is written on nobody's + // behalf, and what may be set is bounded by the binding rather than by who + // is looking. + planned = await this.values.planWrite( + { [`${CUSTOM_NAMESPACE}.${into.key}`]: value }, + INTERNAL, + ); } catch (err) { logging.warn( { diff --git a/ghost/core/core/server/services/members-custom-fields/definitions-service.ts b/ghost/core/core/server/services/members-custom-fields/definitions-service.ts index 4c7d0b09d8d..9ec4d8b60ae 100644 --- a/ghost/core/core/server/services/members-custom-fields/definitions-service.ts +++ b/ghost/core/core/server/services/members-custom-fields/definitions-service.ts @@ -7,6 +7,7 @@ import { FieldTypeSchema, type FieldType } from '@tryghost/custom-field-types'; import { CUSTOM_NAMESPACE } from '@tryghost/custom-field-types/identity'; import { customFieldCodec } from './codec'; import { FIELD_STATUS, FieldStatusSchema } from './schema'; +import { ADMIN, readableFields, type Audience } from './access'; import { activeFields, fieldByKey, inFieldOrder, type DefinitionQuery } from './queries'; import { KEY_CHARACTERS, mintableKey } from './key'; import { type RecordCustomFieldAction, type RequestContext } from './actions'; @@ -141,7 +142,10 @@ export class CustomFieldDefinitionsService { } } - async browse(options: { namespace?: string; filter?: string } = {}): Promise { + async browse( + options: { namespace?: string; filter?: string } = {}, + audience: Audience = ADMIN, + ): Promise { if (options.namespace !== undefined && !this.isStored(options.namespace)) { return []; } @@ -155,7 +159,7 @@ export class CustomFieldDefinitionsService { const query = options.filter ? applyFilter(this.knex(TABLE), options.filter) : activeFields(this.knex); - return this.list(query); + return readableFields(audience, await this.list(query)); } /** diff --git a/ghost/core/core/server/services/members-custom-fields/index.ts b/ghost/core/core/server/services/members-custom-fields/index.ts index ff9f5d84fd9..739c71c5099 100644 --- a/ghost/core/core/server/services/members-custom-fields/index.ts +++ b/ghost/core/core/server/services/members-custom-fields/index.ts @@ -10,6 +10,11 @@ export { actingContext } from './actions'; export type { BoundField } from './bindings-service'; export type { WrittenBy } from './schema'; +// Which door a request came through, which is what decides how much of a member's +// answers it may see or change. Required wherever that is asked, so a new caller +// has to name itself rather than inherit an answer by default. +export { ADMIN, INTERNAL, MEMBERS, canWrite, readableFields, type Audience } from './access'; + // Three services from one module, split along aggregate boundaries rather than // technical layers: `definitions` owns the field definitions, which belong to the // site's settings, `values` owns the per-member values, which belong to the diff --git a/ghost/core/core/server/services/members-custom-fields/schema.ts b/ghost/core/core/server/services/members-custom-fields/schema.ts index 7d24f509231..0c107780474 100644 --- a/ghost/core/core/server/services/members-custom-fields/schema.ts +++ b/ghost/core/core/server/services/members-custom-fields/schema.ts @@ -38,6 +38,10 @@ export const WrittenBy = z.discriminatedUnion('type', [ z.object({ type: z.literal('integration'), id: z.string() }), z.object({ type: z.literal('binding'), id: z.string() }), z.object({ type: z.literal('import'), id: z.null() }), + // A member writing their own answers. Resolvable in `members`, like the others, + // and the only writer whose changes leave nothing in the staff action log: that + // log records what staff did. + z.object({ type: z.literal('member'), id: z.string() }), ]); export type WrittenBy = z.infer; diff --git a/ghost/core/core/server/services/members-custom-fields/values-service.ts b/ghost/core/core/server/services/members-custom-fields/values-service.ts index 08b17a5d7a3..70e7b07c5c3 100644 --- a/ghost/core/core/server/services/members-custom-fields/values-service.ts +++ b/ghost/core/core/server/services/members-custom-fields/values-service.ts @@ -12,6 +12,7 @@ import { } from '@tryghost/custom-field-types/identity'; import { DbCustomFieldLeaf, DbCustomFieldValue, FIELD_STATUS, type WrittenBy } from './schema'; import { activeFields } from './queries'; +import { canWrite, readableFields, type Audience } from './access'; import { leavesToWrite, valuesFromLeaves, type StoredLeaf } from './storage'; const FIELDS_TABLE = 'members_custom_fields'; @@ -92,6 +93,7 @@ export class CustomFieldValuesService { async getValuesForMembers( memberIds: string[], + audience: Audience, ): Promise>>> { if (memberIds.length === 0) { return new Map(); @@ -130,7 +132,10 @@ export class CustomFieldValuesService { } } - const flat = valuesFromLeaves(leaves); + // Narrowed before assembly, not after: a composite is one value spread across + // several rows, and dropping some of its parts later would hand back half an + // address rather than no address. + const flat = valuesFromLeaves(readableFields(audience, leaves)); return new Map( memberIds.map((memberId) => [memberId, { [CUSTOM_NAMESPACE]: flat.get(memberId) ?? {} }]), ); @@ -190,7 +195,7 @@ export class CustomFieldValuesService { * validate before opening a transaction it would otherwise have to unwind, then apply * the same plan without re-resolving it. */ - async planWrite(input: unknown): Promise { + async planWrite(input: unknown, audience: Audience): Promise { const values = this.parseValues(input); const identities = Object.keys(values); @@ -216,6 +221,13 @@ export class CustomFieldValuesService { }); } + if (!canWrite(audience, field)) { + throw new errors.ValidationError({ + message: `Cannot set custom field: ${identity}`, + property: wireProperty(identity), + }); + } + // `null` clears any field, and `''` clears one with no parts. For a value // with parts `''` names nothing, so it is left to fail validation rather than // being read as a silent delete. diff --git a/ghost/core/core/server/services/members/members-api/members-api.js b/ghost/core/core/server/services/members/members-api/members-api.js index 8a110ccf677..aa680dbf276 100644 --- a/ghost/core/core/server/services/members/members-api/members-api.js +++ b/ghost/core/core/server/services/members/members-api/members-api.js @@ -357,7 +357,7 @@ module.exports = function MembersAPI({ } async function getMemberIdentityData(email) { - return memberBREADService.read({ email }, { withCustomFields: false }); + return memberBREADService.read({ email }, { customFieldsFor: null }); } const account = new MemberAccountService({ @@ -376,7 +376,7 @@ module.exports = function MembersAPI({ } async function getMemberIdentityDataFromTransientId(transientId) { - return memberBREADService.read({ transient_id: transientId }, { withCustomFields: false }); + return memberBREADService.read({ transient_id: transientId }, { customFieldsFor: null }); } async function cycleTransientId(memberId) { diff --git a/ghost/core/core/server/services/members/members-api/services/member-bread-service.js b/ghost/core/core/server/services/members/members-api/services/member-bread-service.js index 881f5c47e8f..95c441c2d13 100644 --- a/ghost/core/core/server/services/members/members-api/services/member-bread-service.js +++ b/ghost/core/core/server/services/members/members-api/services/member-bread-service.js @@ -1,4 +1,5 @@ const errors = require('@tryghost/errors'); +const { ADMIN } = require('../../../members-custom-fields'); const logging = require('@tryghost/logging'); const tpl = require('@tryghost/tpl'); const moment = require('moment'); @@ -102,12 +103,12 @@ module.exports = class MemberBREADService { * @param {string[]} memberIds * @returns {Promise> | null>} */ - async fetchCustomFieldValues(memberIds) { + async fetchCustomFieldValues(memberIds, audience) { if (!(await this.customFieldDefinitions.hasAnyActive())) { return null; } - return this.customFieldValues.getValuesForMembers(memberIds); + return this.customFieldValues.getValuesForMembers(memberIds, audience); } /** @@ -384,13 +385,15 @@ module.exports = class MemberBREADService { /** * @param {object} data * @param {object} [options] - * @param {boolean} [options.withCustomFields] Pass false to leave custom field values - * off the result. Ghost calls this method to identify a signed-in reader on every page - * view of a themed site, and that caller renders the member through a fixed list of - * fields that has never included custom ones. Fetching them there costs two database - * queries per page view whose results are then thrown away. + * @param {import('../../../members-custom-fields').Audience | null} [options.customFieldsFor] + * Who the extra fields a publisher defined are being read for, or null to leave them + * off entirely. Null is not the same as "nobody may see them": it means this caller + * never shows them, so fetching them is two database queries whose results are thrown + * away. Ghost identifies a signed-in reader on every page view of a themed site + * through this method, and that caller renders a member through a fixed list of + * fields which has never included these. */ - async read(data, { withCustomFields = true, ...options } = {}) { + async read(data, { customFieldsFor = ADMIN, ...options } = {}) { const defaultWithRelated = [ 'labels', 'stripeSubscriptions', @@ -451,8 +454,8 @@ module.exports = class MemberBREADService { const unsubscribeUrl = this.settingsHelpers.createUnsubscribeUrl(member.uuid); member.unsubscribe_url = unsubscribeUrl; - if (withCustomFields) { - const customFields = await this.fetchCustomFieldValues([member.id]); + if (customFieldsFor) { + const customFields = await this.fetchCustomFieldValues([member.id], customFieldsFor); if (customFields) { member.metafields = customFields.get(member.id) ?? {}; } @@ -792,7 +795,10 @@ module.exports = class MemberBREADService { // One query for the whole page, not one per member. `null` when the flag // is off or the caller didn't ask — the same truthiness guard read uses. const customFieldsByMember = options.includeCustomFields - ? await this.fetchCustomFieldValues(page.data.map((model) => model.id)) + ? await this.fetchCustomFieldValues( + page.data.map((model) => model.id), + ADMIN, + ) : null; const data = page.data.map((model, index) => { diff --git a/ghost/core/test/unit/server/services/members/members-api/members-api.test.js b/ghost/core/test/unit/server/services/members/members-api/members-api.test.js index 666696e29b5..def46c9246c 100644 --- a/ghost/core/test/unit/server/services/members/members-api/members-api.test.js +++ b/ghost/core/test/unit/server/services/members/members-api/members-api.test.js @@ -168,7 +168,7 @@ describe('MembersAPI', function () { sinon.assert.calledWithExactly( MemberBREADService.prototype.read.firstCall, { email: 'jamie@example.com' }, - { withCustomFields: false }, + { customFieldsFor: null }, ); sinon.assert.calledOnceWithExactly(memberLoginEvent.add, { member_id: 'member_1' }); sinon.assert.calledOnceWithExactly(giftRedeem, { diff --git a/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js b/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js index 8fe1a83c591..f65d75d04a7 100644 --- a/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js +++ b/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js @@ -583,7 +583,7 @@ describe('MemberBreadService', function () { const customFieldValues = createCustomFieldValuesStub(); const memberBreadService = getService({ customFieldDefinitions, customFieldValues }); - const member = await memberBreadService.read({ id: MEMBER_ID }, { withCustomFields: false }); + const member = await memberBreadService.read({ id: MEMBER_ID }, { customFieldsFor: null }); assert.equal(Object.hasOwn(member, 'metafields'), false); assert.equal(customFieldDefinitions.hasAnyActive.called, false); From 2b7192ebe04dcfd1e25b4d489644cb81a9d67f22 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 2 Sep 2026 13:22:43 +0100 Subject: [PATCH 6/8] Added members reading and changing their own custom fields ref https://linear.app/ghost/issue/BER-3794/members-manage-their-own-custom-fields-in-their-account The extra fields a publisher defines about their members could only be changed by staff, so the person who knows the right answer was the one person who could not give it, and the data went stale as people moved and changed jobs. A member's own account now carries what they hold in those fields and accepts a change back, alongside their name, because their account panel is one form with one Save and splitting the write would leave a half-succeeded state to explain. What there is to fill in is a separate request, because it describes the site rather than the member and changes on a different schedule. Values are resolved before the member is touched, so a field nobody has defined costs the whole request rather than leaving a member renamed and their answers rejected; the write it sits beside reconciles subscriptions with Stripe and sends events, and no rollback would undo those. A site that has defined no fields is unaffected: the key is absent, and the snapshots pinning what a member receives are unchanged. --- ghost/core/core/server/api/endpoints/index.js | 4 + .../endpoints/member-metafields-members.ts | 43 +++ .../services/members/account-service.ts | 49 +++- .../members/members-api/members-api.js | 2 + .../services/member-bread-service.js | 2 +- .../core/server/services/members/utils.js | 9 + .../core/server/web/members/account/routes.ts | 10 + .../e2e-api/members/custom-fields.test.ts | 268 ++++++++++++++---- 8 files changed, 332 insertions(+), 55 deletions(-) create mode 100644 ghost/core/core/server/api/endpoints/member-metafields-members.ts diff --git a/ghost/core/core/server/api/endpoints/index.js b/ghost/core/core/server/api/endpoints/index.js index 0eb04ea2632..fc06ce1ba14 100644 --- a/ghost/core/core/server/api/endpoints/index.js +++ b/ghost/core/core/server/api/endpoints/index.js @@ -316,6 +316,10 @@ module.exports = { return apiFramework.pipeline(require('./members-account'), localUtils, 'members'); }, + get memberMetafieldsMembers() { + return apiFramework.pipeline(require('./member-metafields-members'), localUtils, 'members'); + }, + get giftsMembers() { return apiFramework.pipeline(require('./gifts-members'), localUtils, 'members'); }, diff --git a/ghost/core/core/server/api/endpoints/member-metafields-members.ts b/ghost/core/core/server/api/endpoints/member-metafields-members.ts new file mode 100644 index 00000000000..47f00439ee9 --- /dev/null +++ b/ghost/core/core/server/api/endpoints/member-metafields-members.ts @@ -0,0 +1,43 @@ +import { MEMBERS, definitions } from '../../services/members-custom-fields'; + +/** + * The extra fields a publisher has defined, as a member's own client reads them. + * + * The same definitions Admin reads, answering a different audience. A member is + * shown what there is to fill in; what they have filled in is on their own record, + * not here, because the two change on different schedules and a client caches them + * differently. + * + * Signed in, but not about any particular member: every member of a site is offered + * the same fields. The route resolves who is asking and refuses an unknown caller, + * so there is always a member by the time this runs. + */ + +interface Frame { + options: { + namespace: string; + }; +} + +const controller = { + // The Admin resource's name, so the response Portal reads is the one Admin's + // serializer already produces. Only who may ask differs, and that is the route's + // business rather than the response's. + docName: 'members_metafields', + + browse: { + headers: { cacheInvalidate: false }, + options: ['namespace'], + validation: { options: { namespace: { required: true } } }, + permissions: false, + query(frame: Frame) { + // No `filter`, which Admin offers: whether a field is archived is a + // publisher's business, and a member is only ever shown what they can still + // fill in. + return definitions!.browse({ namespace: frame.options.namespace }, MEMBERS); + }, + }, +}; + +export default controller; +module.exports = controller; diff --git a/ghost/core/core/server/services/members/account-service.ts b/ghost/core/core/server/services/members/account-service.ts index 57c6d6051b9..3131c678f7e 100644 --- a/ghost/core/core/server/services/members/account-service.ts +++ b/ghost/core/core/server/services/members/account-service.ts @@ -1,4 +1,5 @@ import _ from 'lodash'; +import { MEMBERS } from '../members-custom-fields'; /** * A member's own account: what they are shown about themselves, and what they may @@ -53,38 +54,76 @@ interface EmailSuppressionList { removeEmail(email: string): Promise; } +interface CustomFieldValues { + unwrapWire(input: unknown): unknown; + planWrite(values: unknown, audience: unknown): Promise; + applyWrite( + memberId: string, + writes: unknown[], + options: { writtenBy: { type: string; id: string } }, + ): Promise; +} + export interface MemberAccountServiceDeps { memberBREADService: MemberBreadService; members: MemberRepository; emailSuppressionList: EmailSuppressionList; + customFieldValues: CustomFieldValues; } export class MemberAccountService { #memberBREADService: MemberBreadService; #members: MemberRepository; #emailSuppressionList: EmailSuppressionList; + #customFieldValues: CustomFieldValues; - constructor({ memberBREADService, members, emailSuppressionList }: MemberAccountServiceDeps) { + constructor({ + memberBREADService, + members, + emailSuppressionList, + customFieldValues, + }: MemberAccountServiceDeps) { this.#memberBREADService = memberBREADService; this.#members = members; this.#emailSuppressionList = emailSuppressionList; + this.#customFieldValues = customFieldValues; } /** Everything a member is shown about themselves. */ async read(memberId: string): Promise | null> { - // Without the extra fields a publisher defines: nothing in this projection - // carries them yet, so fetching them would cost a query per request and be - // discarded. The session read leaves them off for the same reason. - return this.#memberBREADService.read({ id: memberId }, { withCustomFields: false }); + // As the member rather than as staff: how much of the extra fields a publisher + // defines is answered depends on which side of Ghost is asking. + return this.#memberBREADService.read({ id: memberId }, { customFieldsFor: MEMBERS }); } /** Apply what a member asked to change about themselves, and say what they now hold. */ async edit(data: Record, memberId: string) { + // Worked out before the member is touched, so a value the catalog refuses fails + // the whole request rather than leaving a member renamed with their answers + // rejected. The write below reconciles subscriptions with Stripe and sends + // events, none of which giving up halfway could undo. + const plannedCustomFields = + data.metafields === undefined + ? null + : await this.#customFieldValues.planWrite( + this.#customFieldValues.unwrapWire(data.metafields), + MEMBERS, + ); + await this.#members.update(_.pick(data, WRITABLE_FIELDS), { id: memberId, withRelated: WRITE_RELATIONS, }); + if (plannedCustomFields) { + // A member is recorded as the author of their own answers, and is the one + // writer whose changes leave nothing in the staff action log: that log + // records what staff did. + await this.#customFieldValues.applyWrite(memberId, plannedCustomFields, { + writtenBy: { type: 'member', id: memberId }, + }); + } + // Read back rather than returning what was written: a member is told what // Ghost now holds, which is not always what they sent. Setting the older // `subscribed` flag, for one, is stored as a list of newsletters. diff --git a/ghost/core/core/server/services/members/members-api/members-api.js b/ghost/core/core/server/services/members/members-api/members-api.js index aa680dbf276..38df5ce46b4 100644 --- a/ghost/core/core/server/services/members/members-api/members-api.js +++ b/ghost/core/core/server/services/members/members-api/members-api.js @@ -7,6 +7,7 @@ const PaymentsService = require('./services/payments-service'); const TokenService = require('./services/token-service'); const GeolocationService = require('./services/geolocation-service'); const MemberBREADService = require('./services/member-bread-service'); +const customFields = require('../../members-custom-fields'); const { MemberAccountService } = require('../account-service'); const MemberRepository = require('./repositories/member-repository'); const NextPaymentCalculator = require('./services/next-payment-calculator'); @@ -364,6 +365,7 @@ module.exports = function MembersAPI({ memberBREADService, members: users, emailSuppressionList, + customFieldValues: customFields.values, }); async function getMemberIdentity(transientId) { diff --git a/ghost/core/core/server/services/members/members-api/services/member-bread-service.js b/ghost/core/core/server/services/members/members-api/services/member-bread-service.js index 95c441c2d13..8a9329bf8ca 100644 --- a/ghost/core/core/server/services/members/members-api/services/member-bread-service.js +++ b/ghost/core/core/server/services/members/members-api/services/member-bread-service.js @@ -577,7 +577,7 @@ module.exports = class MemberBREADService { // plan to apply once below, so the values aren't resolved and validated // twice. const plannedCustomFields = writeCustomFields - ? await this.customFieldValues.planWrite(customFields) + ? await this.customFieldValues.planWrite(customFields, ADMIN) : null; let model; diff --git a/ghost/core/core/server/services/members/utils.js b/ghost/core/core/server/services/members/utils.js index f5a13c378a7..739c1a9c5ac 100644 --- a/ghost/core/core/server/services/members/utils.js +++ b/ghost/core/core/server/services/members/utils.js @@ -41,5 +41,14 @@ module.exports.formattedMemberResponse = function formattedMemberResponse(member data.email_suppression = member.email_suppression; } + // Absent rather than empty on a site that has defined no extra fields, which is + // most of them, so those members' accounts read exactly as they did before this + // existed. Present but empty means the publisher has defined fields and this + // member has answered none, which a client renders as blank inputs rather than + // as nothing to fill in. + if (member.metafields) { + data.metafields = member.metafields; + } + return data; }; diff --git a/ghost/core/core/server/web/members/account/routes.ts b/ghost/core/core/server/web/members/account/routes.ts index 8d37db33342..f3354ce2ac1 100644 --- a/ghost/core/core/server/web/members/account/routes.ts +++ b/ghost/core/core/server/web/members/account/routes.ts @@ -48,6 +48,16 @@ module.exports = function accountRoutes() { http(api.membersAccount.update), ); + // The fields a publisher has defined, which a member's client renders inputs + // from. Under the account because a member reads them to fill in their own, and + // refused to an unknown caller because what a publisher collects is their + // configuration rather than something the site announces. + router.get( + '/metafields/:namespace', + middleware.rejectWhenAnonymous, + http(api.memberMetafieldsMembers.browse), + ); + // Letting Ghost email this member again after it stopped. router.delete( '/suppression', diff --git a/ghost/core/test/e2e-api/members/custom-fields.test.ts b/ghost/core/test/e2e-api/members/custom-fields.test.ts index be718334e1b..463f03728e5 100644 --- a/ghost/core/test/e2e-api/members/custom-fields.test.ts +++ b/ghost/core/test/e2e-api/members/custom-fields.test.ts @@ -1,86 +1,256 @@ import assert from 'node:assert/strict'; const { agentProvider, fixtureManager, mockManager } = require('../../utils/e2e-framework'); -const models = require('../../../core/server/models'); - -// Custom fields are staff-only. Nothing in the members API is written to expose or -// accept them — the member response is built from a field whitelist that predates the -// feature, and the update path drops unknown keys — but nothing asserted it. These pin -// the two endpoints that hand a member their own payload. Other member-facing surfaces -// (newsletter preferences, theme member data, the comments author shape) narrow through -// their own whitelists and are not covered here. + +const MEMBER_EMAIL = 'member@example.com'; +const SHOE_SIZE = 'Shoe size'; + +interface Agent { + get: (_url: string) => any; + put: (_url: string) => any; + post: (_url: string) => any; + delete: (_url: string) => any; +} + +interface MembersAgent extends Agent { + loginAs: (_email: string) => Promise; + duplicate: () => MembersAgent; +} + +interface AdminAgent extends Agent { + loginAsOwner: () => Promise; +} + +// The extra fields a publisher defines about their members used to be staff-only. A +// member can now read and change their own, through the two endpoints that hand them +// their own payload, which is what these cover. Other member-facing surfaces +// (newsletter preferences, theme member data, the comments author shape) build their +// responses from their own field lists and still do not carry these. +// +// Everything here is set up and checked through an API: the publisher's side through +// the Admin API, the member's through the members API. Nothing reads or writes a +// table, so none of this is pinned to how the fields happen to be stored. describe('Member Custom Fields Members API', function () { - let adminAgent: { - get: (_url: string) => any; - put: (_url: string) => any; - post: (_url: string) => any; - loginAsOwner: () => Promise; - }; - let membersAgent: { - get: (_url: string) => any; - put: (_url: string) => any; - loginAs: (_email: string) => Promise; - }; + let adminAgent: AdminAgent; + let membersAgent: MembersAgent; let memberId: string; let fieldKey: string; - let fieldCounter = 0; - async function readValuesAsStaff() { + /** Define a field, as a publisher does, and hand back the key Ghost minted for it. */ + /** Every field this suite defined, so cleanup can undo its own work and no more. */ + const defined = new Set(); + + async function defineField(name: string, type = 'short_text'): Promise { + const { body } = await adminAgent + .post('members/metafields/custom/') + .body({ members_metafields: [{ name, type }] }) + .expectStatus(201); + const { key } = body.members_metafields[0]; + defined.add(key); + return key; + } + + async function archiveField(key: string): Promise { + await adminAgent + .put(`members/metafields/custom/${key}/`) + .body({ members_metafields: [{ status: 'archived' }] }) + .expectStatus(200); + } + + /** + * Undo the definitions this suite made, through the API that made them. + * + * Whether a member payload carries these fields at all follows from the site + * defining any, so a definition left behind changes the shape of member + * responses for every test that runs after it. + * + * Only the ones defined here. Test files share a worker, and so a database, so + * removing every field a site has would take another suite's fixtures with it. + */ + async function removeFieldsDefinedHere(): Promise { + const { body } = await adminAgent + .get('members/metafields/custom/?filter=status:[active,archived]') + .expectStatus(200); + + const mine = body.members_metafields.filter((field: { key: string }) => defined.has(field.key)); + + for (const field of mine) { + // Only an archived field can be deleted, and deleting one takes the answers + // members gave for it along with it. + if (field.status !== 'archived') { + await archiveField(field.key); + } + await adminAgent.delete(`members/metafields/custom/${field.key}/`).expectStatus(204); + defined.delete(field.key); + } + } + + /** What the member looks like to staff, which is the second opinion on every write. */ + async function readMemberAsStaff() { const { body } = await adminAgent.get(`members/${memberId}/`).expectStatus(200); - return body.members[0].metafields?.custom ?? {}; + return body.members[0]; + } + + async function setValuesAsStaff(values: Record): Promise { + await adminAgent + .put(`members/${memberId}/`) + .body({ members: [{ metafields: { custom: values } }] }) + .expectStatus(200); } beforeAll(async function () { ({ adminAgent, membersAgent } = await agentProvider.getAgentsForMembers()); await fixtureManager.init('newsletters', 'members:newsletters'); await adminAgent.loginAsOwner(); - await membersAgent.loginAs('member@example.com'); + await membersAgent.loginAs(MEMBER_EMAIL); + // Defining and removing fields is behind the flag, so it is on for the whole + // file rather than for each test: every test both makes and unmakes one. + mockManager.mockLabsEnabled('membersCustomFields'); + + const { body } = await adminAgent + .get(`members/?filter=email:'${MEMBER_EMAIL}'`) + .expectStatus(200); + assert.equal(body.members.length, 1, `exactly one member holds ${MEMBER_EMAIL}`); + memberId = body.members[0].id; + }); - const member = await models.Member.findOne({ email: 'member@example.com' }, { require: true }); - memberId = member.id; + afterAll(function () { + mockManager.restore(); }); beforeEach(async function () { - mockManager.mockLabsEnabled('membersCustomFields'); + fieldKey = await defineField(SHOE_SIZE); + await setValuesAsStaff({ [fieldKey]: '9' }); + }); - fieldCounter += 1; - const { body } = await adminAgent - .post('members/metafields/custom/') - .body({ members_metafields: [{ name: `Shoe size ${fieldCounter}`, type: 'short_text' }] }) - .expectStatus(201); - fieldKey = body.members_metafields[0].key; + afterEach(async function () { + await removeFieldsDefinedHere(); + }); - await adminAgent - .put(`members/${memberId}/`) - .body({ members: [{ metafields: { custom: { [fieldKey]: '9' } } }] }) + it('offers a member the fields there are to fill in', async function () { + // A second field, so this is a list rather than a single value that happens to + // be right, and so the order it comes back in means something. + const colourKey = await defineField('Favourite colour'); + + const { body } = await membersAgent.get('/api/member/metafields/custom/').expectStatus(200); + + assert.deepEqual( + body.members_metafields.map((field: { key: string }) => field.key), + [fieldKey, colourKey], + 'every field the publisher defined, in the order they defined them', + ); + + const [field] = body.members_metafields; + assert.equal(field.name, SHOE_SIZE); + assert.equal(field.type, 'short_text'); + assert.equal(field.namespace, 'custom'); + + // No database id: a field is addressed by its namespace and key, and neither + // is reissued once minted. + assert.equal(Object.hasOwn(field, 'id'), false); + }); + + it('says nothing about a field the publisher has archived', async function () { + // A field a member once answered, which the publisher has since retired. The + // answer stays on the record; what changes is that the member is no longer + // asked about it and no longer told what they said. + const retiredKey = await defineField('Former employer'); + await setValuesAsStaff({ [retiredKey]: 'Acme' }); + await archiveField(retiredKey); + + const { body: offered } = await membersAgent + .get('/api/member/metafields/custom/') .expectStatus(200); + const offeredKeys = offered.members_metafields.map((field: { key: string }) => field.key); + assert.deepEqual(offeredKeys, [fieldKey], 'only the field still in use is offered'); + + const { body: account } = await membersAgent.get('/api/member/').expectStatus(200); + assert.deepEqual( + account.metafields, + { custom: { [fieldKey]: '9' } }, + 'and the answer they gave for the archived one is not handed back', + ); }); - afterEach(async function () { - mockManager.restore(); - // Whether a member payload carries custom fields at all follows from the site defining - // any, so a definition left behind here changes the shape of member responses in every - // suite that runs after this one. - await models.Base.knex('members_custom_field_values').del(); - await models.Base.knex('members_custom_fields').del(); + it('will not name the fields to someone who is not signed in', async function () { + // Which fields a publisher collects is their configuration, not something the + // site announces. A fresh agent rather than this suite's, which is signed in. + const signedOut = membersAgent.duplicate(); + + const { statusCode, body } = await signedOut.get('/api/member/metafields/custom/'); + + assert.equal(statusCode, 401); + assert.match(body.errors[0].message, /you must be signed in/i); + + // The status alone would pass a response that refused and listed them anyway, + // which is the disclosure this is here to prevent. + assert.equal(Object.hasOwn(body, 'members_metafields'), false); + const refusal = JSON.stringify(body); + assert.ok(!refusal.includes(fieldKey), 'the key of a defined field is not named'); + assert.ok(!refusal.includes(SHOE_SIZE), 'nor the name the publisher gave it'); }); - it('does not return custom fields to the member who holds them', async function () { + it('returns a member the values they hold', async function () { const { body } = await membersAgent.get('/api/member/').expectStatus(200); - assert.equal(Object.hasOwn(body, 'metafields'), false); + // Namespaced, the same way staff are given them, so a client reads one shape + // whichever side of Ghost it is talking to. + assert.deepEqual(body.metafields, { custom: { [fieldKey]: '9' } }); }); - it('does not write custom fields a member sends', async function () { + it('writes the values a member sends about themselves', async function () { const { body } = await membersAgent .put('/api/member/') .body({ name: 'Renamed', metafields: { custom: { [fieldKey]: '12' } } }) .expectStatus(200); - // The rest of the body still applies, so the value is dropped rather than - // the request being rejected — the same way `email` behaves here. assert.equal(body.name, 'Renamed'); - assert.equal(Object.hasOwn(body, 'metafields'), false); - assert.equal((await readValuesAsStaff())[fieldKey], '9'); + assert.equal(body.metafields.custom[fieldKey], '12'); + + // Read back through the Admin API, because the value being in the response is + // only half the claim: staff and the member are looking at one stored answer, + // not at two that could drift. '12' rather than the '9' it started as, so a + // write that did nothing could not pass this. + const stored = await readMemberAsStaff(); + assert.equal(stored.name, 'Renamed'); + assert.equal(stored.metafields.custom[fieldKey], '12'); + }); + + it('refuses a field nobody has defined, and changes nothing', async function () { + const before = await readMemberAsStaff(); + assert.notEqual(before.name, 'Not renamed', 'the name is not already what this sets it to'); + + const { body } = await membersAgent + .put('/api/member/') + .body({ + name: 'Not renamed', + metafields: { + custom: { + // A field that does exist, named first. A handler that wrote each value + // as it resolved it would already have written this one by the time it + // reached the unknown field below, so without it the whole-request part + // of the claim would not be exercised at all. + [fieldKey]: '12', + nothing_by_this_name: 'x', + }, + }, + }) + .expectStatus(422); + + // Refused rather than ignored, unlike an unrecognised key at the top level: a + // named field that does not exist means the client believes something false. + // The reason reaches the member's client, which is what lets it name the field + // that was wrong rather than only saying that something was. + const [error] = body.errors; + assert.equal(error.message, 'Unknown custom field: custom.nothing_by_this_name'); + assert.equal(error.property, 'metafields.custom.nothing_by_this_name'); + + // Nothing the request named was applied: not the good field beside the bad one, + // and not the name sent alongside both. The values are resolved before the + // member is touched, so one bad field costs the whole request rather than + // leaving a member renamed and their answers half-written. + const after = await readMemberAsStaff(); + assert.equal(after.name, before.name); + assert.equal(after.metafields.custom[fieldKey], '9', 'the defined field kept its value'); }); }); From 2b1c9abbf1c956c2f471e51845894fba60862a32 Mon Sep 17 00:00:00 2001 From: louisghost Date: Thu, 3 Sep 2026 12:01:01 +0200 Subject: [PATCH 7/8] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20LinkedIn=20company?= =?UTF-8?q?=20and=20school=20URLs=20with=20underscores=20being=20rejected?= =?UTF-8?q?=20as=20invalid=20(#30490)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- apps/admin/src/settings/utils/social-urls/linkedin.ts | 10 ++++++++-- .../src/settings/utils/social-urls/social-urls.test.ts | 9 +++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/apps/admin/src/settings/utils/social-urls/linkedin.ts b/apps/admin/src/settings/utils/social-urls/linkedin.ts index 0854dcb6edc..6e327aa8cad 100644 --- a/apps/admin/src/settings/utils/social-urls/linkedin.ts +++ b/apps/admin/src/settings/utils/social-urls/linkedin.ts @@ -4,12 +4,18 @@ import type { UsernameRule } from './platform-validator'; // validation info: https://www.linkedin.com/help/linkedin/answer/a542685/manage-your-public-profile-url?lang=en // Letters and numbers in any language (company/school slugs and vanity URLs // can contain accented characters — see ONC-1856), hyphens, 3–100 characters. +// Personal /in/ and /pub/ profiles: alphanumeric (any script) and hyphen only. +// Company and school pages also allow underscores (e.g. /company/some_company). const LINKEDIN_USERNAME_RULE: UsernameRule = { unicode: true, extra: '-', min: 3, max: 100, }; +const LINKEDIN_COMPANY_USERNAME_RULE: UsernameRule = { + ...LINKEDIN_USERNAME_RULE, + extra: '-_', +}; // /in/ profiles are stored as a bare handle; the other path types keep their // prefix in storage. Regional subdomains (uk.linkedin.com) are valid input and @@ -25,8 +31,8 @@ const linkedin = createPlatformValidator({ storagePrefix: 'pub/', rule: { ...LINKEDIN_USERNAME_RULE, nestedSegments: true }, }, - { urlPrefix: 'company/', storagePrefix: 'company/', rule: LINKEDIN_USERNAME_RULE }, - { urlPrefix: 'school/', storagePrefix: 'school/', rule: LINKEDIN_USERNAME_RULE }, + { urlPrefix: 'company/', storagePrefix: 'company/', rule: LINKEDIN_COMPANY_USERNAME_RULE }, + { urlPrefix: 'school/', storagePrefix: 'school/', rule: LINKEDIN_COMPANY_USERNAME_RULE }, ], errors: { invalidUrl: 'The URL must be in a format like https://www.linkedin.com/in/yourUsername', diff --git a/apps/admin/src/settings/utils/social-urls/social-urls.test.ts b/apps/admin/src/settings/utils/social-urls/social-urls.test.ts index 59aaeba5a0c..caac34ff0c5 100644 --- a/apps/admin/src/settings/utils/social-urls/social-urls.test.ts +++ b/apps/admin/src/settings/utils/social-urls/social-urls.test.ts @@ -472,6 +472,10 @@ const FIXTURES: PlatformFixture[] = [ ['linkedin.com/in/山田太郎', 'https://www.linkedin.com/in/山田太郎'], // decomposed accents (e + combining acute) normalise to composed form ['linkedin.com/in/josé-garcia', 'https://www.linkedin.com/in/josé-garcia'], + // company/school pages allow underscores; personal /in/ does not + ['https://www.linkedin.com/company/eyeshot_2/', 'https://www.linkedin.com/company/eyeshot_2'], + ['linkedin.com/company/some_company', 'https://www.linkedin.com/company/some_company'], + ['linkedin.com/school/some_school', 'https://www.linkedin.com/school/some_school'], ], invalid: [ ['https://twitter.com/johnsmith', LINKEDIN_URL_ERROR], @@ -484,6 +488,7 @@ const FIXTURES: PlatformFixture[] = [ ['linkedin.com/in/john%20smith', LINKEDIN_USERNAME_ERROR], // percent-encoded space ['linkedin.com/in/john%3Fsmith', LINKEDIN_USERNAME_ERROR], // percent-encoded ? ['linkedin.com/in/john%2smith', LINKEDIN_USERNAME_ERROR], // malformed percent-encoding + ['linkedin.com/in/john_smith', LINKEDIN_USERNAME_ERROR], // underscores not allowed on /in/ // a leftover '@' after the 'company/' prefix mixes two // incompatible URL conventions and is rejected, not silently stripped ['linkedin.com/company/@acme', LINKEDIN_USERNAME_ERROR], @@ -496,6 +501,8 @@ const FIXTURES: PlatformFixture[] = [ ['in/johnsmith', 'https://www.linkedin.com/in/johnsmith'], ['company/ghost-foundation', 'https://www.linkedin.com/company/ghost-foundation'], ['company/ghost-foundation/', 'https://www.linkedin.com/company/ghost-foundation'], + ['company/eyeshot_2', 'https://www.linkedin.com/company/eyeshot_2'], + ['school/some_school', 'https://www.linkedin.com/school/some_school'], ['pub/johnsmith/12/34/567', 'https://www.linkedin.com/pub/johnsmith/12/34/567'], [ 'company/la-revue-européenne-des-médias-et-du-numérique', @@ -506,6 +513,7 @@ const FIXTURES: PlatformFixture[] = [ ['john@smith', LINKEDIN_USERNAME_ERROR], ['john#smith', LINKEDIN_USERNAME_ERROR], ['john.smith', LINKEDIN_USERNAME_ERROR], // dots are not allowed on linkedin + ['john_smith', LINKEDIN_USERNAME_ERROR], // underscores not allowed on personal handles ['jo', LINKEDIN_USERNAME_ERROR], // too short ['a'.repeat(101), LINKEDIN_USERNAME_ERROR], // too long ['company/@acme', LINKEDIN_USERNAME_ERROR], @@ -524,6 +532,7 @@ const FIXTURES: PlatformFixture[] = [ ['linkedin.com/pub/johnsmith/12/34/567', 'pub/johnsmith/12/34/567'], ['https://www.linkedin.com/company/ghost-foundation', 'company/ghost-foundation'], ['https://www.linkedin.com/company/ghost-foundation/', 'company/ghost-foundation'], + ['https://www.linkedin.com/company/eyeshot_2/', 'company/eyeshot_2'], ['linkedin.com/school/mit', 'school/mit'], [ 'https://www.linkedin.com/company/la-revue-europ%C3%A9enne-des-m%C3%A9dias-et-du-num%C3%A9rique/', From 8477639150f11ac2e7e6c9410060fb3f4f2654c4 Mon Sep 17 00:00:00 2001 From: Zach Date: Thu, 3 Sep 2026 03:08:16 -0700 Subject: [PATCH 8/8] =?UTF-8?q?=F0=9F=8E=A8=20Improved=20focus=20when=20ad?= =?UTF-8?q?ding=20a=20filter=20(#30394)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/ghost/issue/DES-1280/auto-focus-newly-added-filter-fields-in-shade-filter > **Scope: Shade only.** This fixes the component. Admin needs a separate change (DES-1496) before the behaviour is visible on Members or Comments — see Testing. ### Why Adding a filter you type into — Name, Email, a number — left the caret behind, so you had to click into the empty box you'd just asked for. Filters that open a picker never had this problem, which made it read as an inconsistency rather than a missing feature. Reported by Peter. ### What it does The value input takes focus when its filter was just added. One file plus tests, no consumer changes. Excluded on purpose: dates and ranges arrive already carrying a value rather than waiting on input, so taking the caret would interrupt; pickers already focus their own menu; booleans have nothing to type into. ### Testing **This won't visibly fix Members or Comments yet.** Those pages render two `` and swap between them when the first filter is added, which destroys the component mid-action. Separate Admin bug — DES-1496. To see it working: Storybook → `Patterns / Filters`, or add a *second* filter in Admin, which already works. ### Tests Three added — text and number take focus, date is left alone. The existing 20 pass. --- - [x] I've read and followed the [Contributor Guide](https://github.com/TryGhost/Ghost/blob/main/.github/CONTRIBUTING.md) - [x] I've explained my change - [x] I've written an automated test to prove my change works --- .../shade/src/components/patterns/filters.tsx | 40 +++++++ .../unit/components/patterns/filters.test.tsx | 101 ++++++++++++++++++ 2 files changed, 141 insertions(+) diff --git a/apps/shade/src/components/patterns/filters.tsx b/apps/shade/src/components/patterns/filters.tsx index 22c31811d48..6d29def364f 100644 --- a/apps/shade/src/components/patterns/filters.tsx +++ b/apps/shade/src/components/patterns/filters.tsx @@ -524,11 +524,16 @@ function FilterInput({ onKeyDown, onInputChange, className, + inputRef, ...props }: React.InputHTMLAttributes & { className?: string; field?: FilterFieldConfig; onInputChange?: (e: React.ChangeEvent) => void; + // Taken as a plain prop rather than through forwardRef: this is an internal + // helper, not an exported Shade component, and forwardRef composes badly with + // its generic parameter. + inputRef?: React.Ref; }) { const context = useFilterContext(); const [isValid, setIsValid] = useState(true); @@ -649,6 +654,7 @@ function FilterInput({
{ field: FilterFieldConfig; values: T[]; @@ -1478,6 +1491,8 @@ interface FilterValueSelectorProps { operator: string; onOperatorChange?: (operator: string) => void; readOnly?: boolean; + /** Focus the value input on mount — set for a filter the user just added. */ + autoFocus?: boolean; } interface SelectOptionsPopoverProps { @@ -2053,10 +2068,12 @@ function FilterValueSelector({ operator, onOperatorChange, readOnly, + autoFocus, }: FilterValueSelectorProps) { const [open, setOpen] = useState(false); const [searchInput, setSearchInput] = useState(''); const context = useFilterContext(); + const valueInputRef = useRef(null); // Focus the search input when the popover opens useEffect(() => { @@ -2071,6 +2088,14 @@ function FilterValueSelector({ } }, [open, field.searchable]); + // A filter the user just added lands with an empty value, so put the caret + // where the answer goes instead of leaving them to click into it. + useEffect(() => { + if (autoFocus) { + valueInputRef.current?.focus(); + } + }, [autoFocus]); + // Hide value input for empty/not empty operators if (operator === 'empty' || operator === 'not_empty') { return null; @@ -2279,6 +2304,7 @@ function FilterValueSelector({ ({ ({ ({ const [addFilterOpen, setAddFilterOpen] = useState(false); const [selectedFieldKeyForOptions, setSelectedFieldKeyForOptions] = useState(null); const [tempSelectedValues, setTempSelectedValues] = useState([]); + // The filter added most recently, so its input can take focus once it renders. + // Holding the id rather than a boolean keeps it pinned to that one row, which + // a positional guess would lose as soon as filters are added or removed. + const [autoFocusFilterId, setAutoFocusFilterId] = useState(null); // The field-picker search, controlled so a `previewLimit` group can uncap while // the user is searching. `expandedGroups` holds the groups whose "Show more" was // clicked. Both reset when the picker closes. @@ -2874,6 +2906,13 @@ export function Filters({ const newFilter = createFilter(fieldKey, defaultOperator, defaultValues as T[]); onChange([...filters, newFilter]); + + // Picker types are excluded here because adding one opens its options + // popover, which already takes focus. + if (TYPED_VALUE_FIELD_TYPES.includes(field.type || '')) { + setAutoFocusFilterId(newFilter.id); + } + closeFilterPopover(); }, [allowMultiple, closeFilterPopover, fieldsMap, filters, onChange], @@ -3052,6 +3091,7 @@ export function Filters({ static segments, so the filter stays legible while only the remove control acts. */} + autoFocus={filter.id === autoFocusFilterId} field={field} operator={filter.operator} readOnly={field.readOnly} diff --git a/apps/shade/test/unit/components/patterns/filters.test.tsx b/apps/shade/test/unit/components/patterns/filters.test.tsx index 371cf9bf50b..dd8cd574ec2 100644 --- a/apps/shade/test/unit/components/patterns/filters.test.tsx +++ b/apps/shade/test/unit/components/patterns/filters.test.tsx @@ -739,4 +739,105 @@ describe('Filters', () => { expect(screen.queryByRole('button', { name: 'is' })).toBeNull(); }); }); + + describe('focus on a newly revealed value input', () => { + // The add-filter popover renders a cmdk command menu, which needs both of + // these and jsdom provides neither. + beforeAll(() => { + global.ResizeObserver = class { + observe() { + return undefined; + } + + unobserve() { + return undefined; + } + + disconnect() { + return undefined; + } + } as unknown as typeof ResizeObserver; + HTMLElement.prototype.scrollIntoView = vi.fn(); + }); + + function FocusTestFilters({ + initialFilters = [], + }: Readonly<{ initialFilters?: Filter[] }>) { + const [filters, setFilters] = useState[]>(initialFilters); + const fields = useMemo( + () => [ + { + key: 'name', + label: 'Name', + type: 'text' as const, + operators: [ + { value: 'is', label: 'is' }, + { value: 'empty', label: 'is empty' }, + ], + defaultOperator: 'is', + }, + { + key: 'count', + label: 'Count', + type: 'number' as const, + operators: [{ value: 'is', label: 'is' }], + defaultOperator: 'is', + }, + { + key: 'created', + label: 'Created', + type: 'date' as const, + operators: [{ value: 'is', label: 'is' }], + defaultOperator: 'is', + }, + ], + [], + ); + + return ( + + ); + } + + const addFilterNamed = async (name: string) => { + fireEvent.click(screen.getByRole('button', { name: 'Add filter' })); + fireEvent.click(await screen.findByRole('option', { name })); + }; + + it('puts the caret in a newly added text filter so it can be typed into without a click', async () => { + render(); + + await addFilterNamed('Name'); + + const input = await screen.findByRole('textbox'); + await waitFor(() => expect(document.activeElement).toBe(input)); + }); + + it('puts the caret in a newly added number filter', async () => { + render(); + + await addFilterNamed('Count'); + + const input = await screen.findByRole('spinbutton'); + await waitFor(() => expect(document.activeElement).toBe(input)); + }); + + it('leaves a newly added date filter alone, since it is not waiting on typed input', async () => { + render(); + + await addFilterNamed('Created'); + + // The date control renders its own input, but it arrives ready to use + // rather than empty, so stealing the caret would be noise. + await waitFor(() => expect(screen.getByText('Created')).toBeDefined()); + expect(document.activeElement?.getAttribute('data-slot')).not.toBe('filters-input'); + }); + }); });