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( 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/', 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'); + }); + }); }); 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/api/endpoints/index.js b/ghost/core/core/server/api/endpoints/index.js index 3453f3ac16e..fc06ce1ba14 100644 --- a/ghost/core/core/server/api/endpoints/index.js +++ b/ghost/core/core/server/api/endpoints/index.js @@ -312,6 +312,14 @@ module.exports = { return apiFramework.pipeline(require('./feedback-members'), localUtils, 'members'); }, + get membersAccount() { + 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/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/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/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/account-service.ts b/ghost/core/core/server/services/members/account-service.ts new file mode 100644 index 00000000000..3131c678f7e --- /dev/null +++ b/ghost/core/core/server/services/members/account-service.ts @@ -0,0 +1,144 @@ +import _ from 'lodash'; +import { MEMBERS } from '../members-custom-fields'; + +/** + * 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; +} + +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, + 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> { + // 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. + 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..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,8 @@ 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'); @@ -356,11 +358,27 @@ module.exports = function MembersAPI({ } async function getMemberIdentityData(email) { - return memberBREADService.read({ email }, { withCustomFields: false }); + return memberBREADService.read({ email }, { customFieldsFor: null }); + } + + const account = new MemberAccountService({ + memberBREADService, + members: users, + emailSuppressionList, + customFieldValues: customFields.values, + }); + + 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 }); + return memberBREADService.read({ transient_id: transientId }, { customFieldsFor: null }); } async function cycleTransientId(memberId) { @@ -500,6 +518,7 @@ module.exports = function MembersAPI({ getMemberIdentityToken, getMemberEntitlementToken, getMemberIdentityDataFromTransientId, + getMemberIdentity, getMemberIdentityData, cycleTransientId, setMemberGeolocationFromIp, @@ -508,6 +527,7 @@ module.exports = function MembersAPI({ sendEmailWithMagicLink, getMagicLink, members: users, + account, memberBREADService, events: eventRepository, productRepository, 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..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 @@ -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) ?? {}; } @@ -574,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; @@ -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/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/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/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/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..f3354ce2ac1 --- /dev/null +++ b/ghost/core/core/server/web/members/account/routes.ts @@ -0,0 +1,69 @@ +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), + ); + + // 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', + 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/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'); }); }); 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 new file mode 100644 index 00000000000..ca9847f9dc6 --- /dev/null +++ b/ghost/core/test/e2e-api/members/session-auth.test.ts @@ -0,0 +1,232 @@ +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. + * + * 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: + * + * 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: 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 () { + ({ adminAgent, membersAgent } = await agentProvider.getAgentsForMembers()); + await fixtureManager.init('newsletters', 'members:newsletters'); + await adminAgent.loginAsOwner(); + }); + + 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 () { + // 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, 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, 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(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, 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('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', + ); + }); + }); +}); 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/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); 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); - }); -});