diff --git a/ghost/core/core/server/api/endpoints/authentication.js b/ghost/core/core/server/api/endpoints/authentication.js index 43582cd1094..a91d1efb501 100644 --- a/ghost/core/core/server/api/endpoints/authentication.js +++ b/ghost/core/core/server/api/endpoints/authentication.js @@ -14,6 +14,7 @@ const UsersService = require('../../services/users'); const userService = new UsersService({dbBackup, models, auth, apiMail, apiSettings}); const adapterManager = require('../../services/adapter-manager'); const schedulerAdapter = adapterManager.getAdapter('scheduling'); +const {requestContextFromFrame} = require('./utils/request-context'); async function destroyRequestSession(req) { if (!req || !req.session) { @@ -241,7 +242,7 @@ const controller = { }, permissions: true, async query(frame) { - const result = await auth.resetAuthentication({ + const result = await auth.resetAuthentication(requestContextFromFrame(frame), { schedulerAdapter, userService, options: frame.options diff --git a/ghost/core/core/server/api/endpoints/gift-links.ts b/ghost/core/core/server/api/endpoints/gift-links.ts index 59d42571f22..c8a8e427198 100644 --- a/ghost/core/core/server/api/endpoints/gift-links.ts +++ b/ghost/core/core/server/api/endpoints/gift-links.ts @@ -1,4 +1,5 @@ -import {service, type RequestContext} from '../../services/gift-links'; +import {service} from '../../services/gift-links'; +import {requestContextFromFrame} from './utils/request-context'; const permissionsService = require('../../services/permissions'); @@ -16,17 +17,6 @@ async function assertCanEditAndGift(frame: Frame): Promise { await permissionsService.canThis(context).edit.post(id); } -function requestContextFromFrame(frame: Frame): RequestContext { - const context = (frame.options.context ?? {}) as {user?: string; integration?: {id: string}}; - if (context.integration) { - return {actor: {id: context.integration.id, type: 'integration'}}; - } - if (context.user) { - return {actor: {id: context.user, type: 'user'}}; - } - return {actor: null}; -} - const noCacheInvalidation = {cacheInvalidate: false}; const controller = { diff --git a/ghost/core/core/server/api/endpoints/utils/request-context.ts b/ghost/core/core/server/api/endpoints/utils/request-context.ts new file mode 100644 index 00000000000..894c509d1d6 --- /dev/null +++ b/ghost/core/core/server/api/endpoints/utils/request-context.ts @@ -0,0 +1,20 @@ +import type {RequestContext} from '../../../services/actions'; + +interface FrameLike { + options: { + context?: unknown; + }; +} + +// Integration wins over user: an integration request also carries the owning user, so checking user +// first would misattribute the action. +export function requestContextFromFrame(frame: FrameLike): RequestContext { + const context = (frame.options.context ?? {}) as {user?: string; integration?: {id: string}}; + if (context.integration) { + return {actor: {id: context.integration.id, type: 'integration'}}; + } + if (context.user) { + return {actor: {id: context.user, type: 'user'}}; + } + return {actor: null}; +} diff --git a/ghost/core/core/server/services/actions/index.ts b/ghost/core/core/server/services/actions/index.ts new file mode 100644 index 00000000000..3e3f3ef9c28 --- /dev/null +++ b/ghost/core/core/server/services/actions/index.ts @@ -0,0 +1,46 @@ +import logging from '@tryghost/logging'; + +export interface Actor { + id: string; + type: 'user' | 'integration'; +} + +export interface RequestContext { + actor: Actor | null; +} + +// The history UI keys its icons and filters off these three events. +export type ActionEvent = 'added' | 'edited' | 'deleted'; + +// actionName refines the label, but the history UI only surfaces it for 'edited' events; 'added' and +// 'deleted' show the bare event. +export interface ActionEntry { + event: ActionEvent; + resourceType: string; + resourceId: string | null; + actionName?: string; + actor: Actor; +} + +export type LogAction = (entry: ActionEntry) => Promise; + +interface ActionRecorder { + add(data: Record, options: {autoRefresh: boolean}): Promise; +} + +export function actionLogger(Action: ActionRecorder): LogAction { + return async (entry) => { + try { + await Action.add({ + event: entry.event, + resource_type: entry.resourceType, + resource_id: entry.resourceId, + actor_type: entry.actor.type, + actor_id: entry.actor.id, + ...(entry.actionName ? {context: {action_name: entry.actionName}} : {}) + }, {autoRefresh: false}); + } catch (err) { + logging.error(err); + } + }; +} diff --git a/ghost/core/core/server/services/auth/reset-authentication.ts b/ghost/core/core/server/services/auth/reset-authentication.ts index 95de8744f38..600e03caee9 100644 --- a/ghost/core/core/server/services/auth/reset-authentication.ts +++ b/ghost/core/core/server/services/auth/reset-authentication.ts @@ -1,4 +1,5 @@ import type {Knex} from 'knex'; +import {actionLogger, type LogAction, type RequestContext} from '../actions'; import type {InternalApiKey, InternalKeys} from '../internal-keys'; import internalKeysDefault from '../internal-keys'; const modelsDefault = require('../../models'); @@ -11,6 +12,7 @@ interface ResetAuthenticationArgs { models?: any; internalKeys?: InternalKeys; deleteAllSessions?: () => Promise; + logAction?: LogAction; } interface ResetAuthenticationResult { @@ -19,26 +21,20 @@ interface ResetAuthenticationResult { } /** - * Rotation, user lock and the audit row commit in a single transaction so - * app crashes mid-flight can't leave the system half-rotated or lose the - * audit trail. Session deletion runs immediately after the commit, before - * the rescheduleAll, so a failure inside the adapter can't leave stale - * session rows live for an attacker. - * - * The schedulerAdapter and userService come from boot's lifecycle, so they - * are explicit parameters. The auth-domain primitives (models, internalKeys, - * sessions) default to their module singletons; tests pass overrides. + * Rotation and user lock commit in a single transaction so a crash mid-flight can't half-rotate the + * system. Session deletion runs after the commit but before rescheduleAll, so an adapter failure + * can't leave stale sessions live for an attacker. */ -export default async function resetAuthentication({ +export default async function resetAuthentication(context: RequestContext, { schedulerAdapter, userService, options, models = modelsDefault, internalKeys = internalKeysDefault, - deleteAllSessions = deleteAllSessionsDefault + deleteAllSessions = deleteAllSessionsDefault, + logAction = actionLogger(modelsDefault.Action) }: ResetAuthenticationArgs): Promise { const previousSchedulerKey = await internalKeys.get('ghost-scheduler'); - const actorId = options?.context?.user ?? null; const {apiKeysRotated, usersLocked} = await models.Base.transaction(async (tx: Knex.Transaction) => { const txOptions = Object.assign({}, options, {transacting: tx}); @@ -46,24 +42,19 @@ export default async function resetAuthentication({ const {count: rotated} = await models.ApiKey.refreshAllSecrets(txOptions); const {count: locked} = await userService.lockAll(txOptions); - if (actorId) { - await models.Action.add({ - event: 'edited', - resource_type: 'security_action', - resource_id: null, - actor_type: 'user', - actor_id: actorId, - context: { - action_name: 'reset_authentication', - api_keys_rotated: rotated, - users_locked: locked - } - }, {transacting: tx, autoRefresh: false}); - } - return {apiKeysRotated: rotated, usersLocked: locked}; }); + if (context.actor) { + await logAction({ + event: 'edited', + resourceType: 'security_action', + resourceId: null, + actionName: 'reset_authentication', + actor: context.actor + }); + } + internalKeys.clear(); await deleteAllSessions(); await schedulerAdapter.rescheduleAll({previousKey: previousSchedulerKey}); diff --git a/ghost/core/core/server/services/gift-links/index.ts b/ghost/core/core/server/services/gift-links/index.ts index b139245f269..d8a51963845 100644 --- a/ghost/core/core/server/services/gift-links/index.ts +++ b/ghost/core/core/server/services/gift-links/index.ts @@ -1,6 +1,5 @@ import {GiftLinksService} from './service'; - -export type {RequestContext} from './service'; +import {actionLogger} from '../actions'; // Set by init() at boot, not at import: knex only exists once the DB has connected. export let service: GiftLinksService | undefined; @@ -13,5 +12,5 @@ export function init(): void { const {knex} = require('../../data/db'); const models = require('../../models'); - service = new GiftLinksService({knex, Action: models.Action}); + service = new GiftLinksService({knex, logAction: actionLogger(models.Action)}); } diff --git a/ghost/core/core/server/services/gift-links/service.ts b/ghost/core/core/server/services/gift-links/service.ts index eb012aee722..35f3087fc57 100644 --- a/ghost/core/core/server/services/gift-links/service.ts +++ b/ghost/core/core/server/services/gift-links/service.ts @@ -1,8 +1,8 @@ import crypto from 'crypto'; import errors from '@tryghost/errors'; -import logging from '@tryghost/logging'; import {z} from 'zod'; import type {Knex} from 'knex'; +import {type ActionEvent, type LogAction, type RequestContext} from '../actions'; import {GiftLinkToken, type GiftLink, type Post} from './models'; import * as queries from './queries'; @@ -10,19 +10,6 @@ export function generateGiftLinkToken(): GiftLinkToken { return GiftLinkToken.parse(crypto.randomBytes(24).toString('base64url')); } -export interface Actor { - id: string; - type: 'user' | 'integration'; -} - -interface ActionRecorder { - add(data: Record, options: {autoRefresh: boolean}): Promise; -} - -export interface RequestContext { - actor: Actor | null; -} - // The history UI only surfaces a verb-specific label (action_name) for 'edited' events; 'added' and // 'deleted' render as the bare event. So 'reset' maps to 'edited' to read as "reset", while // 'add'/'remove' read as plain "added"/"deleted". @@ -30,17 +17,17 @@ const COMMANDS = { add: 'added', reset: 'edited', remove: 'deleted' -} as const satisfies Record; +} as const satisfies Record; type GiftLinkVerb = keyof typeof COMMANDS; export class GiftLinksService { private knex: Knex; - private Action: ActionRecorder; + private logAction: LogAction; - constructor({knex, Action}: {knex: Knex; Action: ActionRecorder}) { + constructor({knex, logAction}: {knex: Knex; logAction: LogAction}) { this.knex = knex; - this.Action = Action; + this.logAction = logAction; } async getPost(postId: string): Promise { @@ -111,17 +98,12 @@ export class GiftLinksService { return; } const event = COMMANDS[verb]; - try { - await this.Action.add({ - event, - resource_type: 'gift_link', - resource_id: subject, - actor_type: context.actor.type, - actor_id: context.actor.id, - ...(event === 'edited' ? {context: {action_name: verb}} : {}) - }, {autoRefresh: false}); - } catch (err) { - logging.error(err); - } + await this.logAction({ + event, + resourceType: 'gift_link', + resourceId: subject, + actor: context.actor, + ...(event === 'edited' ? {actionName: verb} : {}) + }); } } diff --git a/ghost/core/test/integration/services/actions.test.ts b/ghost/core/test/integration/services/actions.test.ts new file mode 100644 index 00000000000..36ef45f0532 --- /dev/null +++ b/ghost/core/test/integration/services/actions.test.ts @@ -0,0 +1,57 @@ +import {afterAll, afterEach, beforeAll, describe, it} from 'vitest'; +import assert from 'node:assert/strict'; +import {actionLogger} from '../../../core/server/services/actions'; + +const testUtils = require('../../utils'); +const models = require('../../../core/server/models'); + +describe('actionLogger (integration)', function () { + const log = actionLogger(models.Action); + const rows = () => models.Base.knex('actions').orderBy('created_at'); + const actionNameOf = (row: {context: unknown}): string => + (typeof row.context === 'string' ? JSON.parse(row.context) : row.context).action_name; + + afterAll(testUtils.teardownDb); + + beforeAll(async function () { + await testUtils.teardownDb(); + await testUtils.setup('users:roles')(); + }); + + afterEach(async function () { + await models.Base.knex('actions').del(); + }); + + it('writes an action row from a domain entry', async function () { + await log({event: 'edited', resourceType: 'gift_link', resourceId: 'post-1', actionName: 'reset', actor: {id: 'u1', type: 'user'}}); + + const [row] = await rows(); + assert.ok(row, 'an action row should be written'); + assert.equal(row.event, 'edited'); + assert.equal(row.resource_type, 'gift_link'); + assert.equal(row.resource_id, 'post-1'); + assert.equal(row.actor_type, 'user'); + assert.equal(row.actor_id, 'u1'); + assert.equal(actionNameOf(row), 'reset'); + }); + + it('omits action_name when the entry has none', async function () { + await log({event: 'deleted', resourceType: 'gift_link', resourceId: null, actor: {id: 'u', type: 'user'}}); + + const [row] = await rows(); + assert.equal(row.event, 'deleted'); + assert.ok(!row.context, 'no context stored when actionName is omitted'); + }); + + it('is best-effort: a failing recorder does not throw', async function () { + const broken = actionLogger({ + add: async () => { + throw new Error('boom'); + } + }); + + await assert.doesNotReject( + () => broken({event: 'added', resourceType: 'x', resourceId: null, actor: {id: 'u', type: 'user'}}) + ); + }); +}); diff --git a/ghost/core/test/integration/services/gift-links.test.ts b/ghost/core/test/integration/services/gift-links.test.ts index bb5f884c876..609aa0bde1d 100644 --- a/ghost/core/test/integration/services/gift-links.test.ts +++ b/ghost/core/test/integration/services/gift-links.test.ts @@ -1,7 +1,8 @@ import {afterAll, afterEach, beforeAll, describe, it} from 'vitest'; import assert from 'node:assert/strict'; import errors from '@tryghost/errors'; -import {GiftLinksService, type RequestContext} from '../../../core/server/services/gift-links/service'; +import {GiftLinksService} from '../../../core/server/services/gift-links/service'; +import {actionLogger, type RequestContext} from '../../../core/server/services/actions'; import type {GiftLink} from '../../../core/server/services/gift-links/models'; const testUtils = require('../../utils'); @@ -26,7 +27,7 @@ describe('GiftLinksService (integration)', function () { otherPostId = testUtils.DataGenerator.Content.posts[1].id; service = new GiftLinksService({ knex: models.Base.knex, - Action: models.Action + logAction: actionLogger(models.Action) }); }); @@ -182,9 +183,9 @@ describe('GiftLinksService (integration)', function () { it('does not fail the command when recording the action throws', async function () { const failing = new GiftLinksService({ knex: models.Base.knex, - Action: {add: async () => { + logAction: actionLogger({add: async () => { throw new Error('action write failed'); - }} + }}) }); await assert.doesNotReject(() => failing.create(CTX, postId)); diff --git a/ghost/core/test/unit/server/services/auth/reset-authentication.test.ts b/ghost/core/test/unit/server/services/auth/reset-authentication.test.ts index f1d6b8c2d76..1a2e5058c13 100644 --- a/ghost/core/test/unit/server/services/auth/reset-authentication.test.ts +++ b/ghost/core/test/unit/server/services/auth/reset-authentication.test.ts @@ -1,25 +1,22 @@ import assert from 'node:assert/strict'; import resetAuthentication from '../../../../../core/server/services/auth/reset-authentication'; import {AutoFillingMap} from '../../../../../core/server/lib/auto-filling-map'; +import type {ActionEntry, RequestContext} from '../../../../../core/server/services/actions'; import type {InternalApiKey, InternalIntegrationSlug} from '../../../../../core/server/services/internal-keys'; -interface ActionRow { - event: string; - resource_type: string; - actor_id: string; - context: {action_name: string; api_keys_rotated: number; users_locked: number}; -} - type TxCallback = (_tx: object) => Promise; +const USER: RequestContext = {actor: {id: 'user-1', type: 'user'}}; + /** * In-memory pretend of the auth-domain modules. We pass it as overrides so * the test exercises the real orchestration body but observes outcomes - * through state we control. + * through state we control. The audit is observed through an injected logAction + * shim that collects the domain entries — never the actions table. */ function buildAuthDomain({apiKeysToRotate, usersToLock, currentKey}: {apiKeysToRotate: number; usersToLock: number; currentKey: {id: string; secret: string}}) { const recorded = { - actions: [] as ActionRow[], + entries: [] as ActionEntry[], sessionsDeleted: false, cacheCleared: false, committed: false @@ -35,15 +32,13 @@ function buildAuthDomain({apiKeysToRotate, usersToLock, currentKey}: {apiKeysToR }, ApiKey: { refreshAllSecrets: async () => ({count: apiKeysToRotate}) - }, - Action: { - add: async (payload: ActionRow) => { - recorded.actions.push(payload); - return {}; - } } }; + const logAction = async (entry: ActionEntry) => { + recorded.entries.push(entry); + }; + const internalKeys = new AutoFillingMap>( (slug) => { throw new Error(`Test internalKeys not seeded for slug ${slug}`); @@ -62,88 +57,92 @@ function buildAuthDomain({apiKeysToRotate, usersToLock, currentKey}: {apiKeysToR const userService = {lockAll: async () => ({count: usersToLock})}; - return {models, internalKeys, deleteAllSessions, userService, recorded}; + return {models, logAction, internalKeys, deleteAllSessions, userService, recorded}; } describe('resetAuthentication', function () { - it('rotates keys, locks users, writes audit row with counts, returns counts', async function () { + it('rotates keys, locks users, records an audit action post-commit, returns counts', async function () { const env = buildAuthDomain({apiKeysToRotate: 4, usersToLock: 3, currentKey: {id: 'k', secret: 'old'}}); - const adapter = {rescheduleAll: async () => {}}; - const result = await resetAuthentication({ - schedulerAdapter: adapter, + const result = await resetAuthentication(USER, { + schedulerAdapter: {rescheduleAll: async () => {}}, userService: env.userService, - options: {context: {user: 'user-1'}}, + options: {}, models: env.models, internalKeys: env.internalKeys, - deleteAllSessions: env.deleteAllSessions + deleteAllSessions: env.deleteAllSessions, + logAction: env.logAction }); assert.deepEqual(result, {apiKeysRotated: 4, usersLocked: 3}); assert.equal(env.recorded.committed, true); - assert.equal(env.recorded.actions.length, 1); - assert.equal(env.recorded.actions[0].event, 'edited'); - assert.equal(env.recorded.actions[0].resource_type, 'security_action'); - assert.equal(env.recorded.actions[0].actor_id, 'user-1'); - assert.equal(env.recorded.actions[0].context.action_name, 'reset_authentication'); - assert.equal(env.recorded.actions[0].context.api_keys_rotated, 4); - assert.equal(env.recorded.actions[0].context.users_locked, 3); + assert.equal(env.recorded.entries.length, 1); + assert.deepEqual(env.recorded.entries[0], { + event: 'edited', + resourceType: 'security_action', + resourceId: null, + actionName: 'reset_authentication', + actor: {id: 'user-1', type: 'user'} + }); }); it('asks the scheduler adapter to reschedule with the pre-rotation key', async function () { const env = buildAuthDomain({apiKeysToRotate: 1, usersToLock: 1, currentKey: {id: 'k', secret: 'pre-rotation'}}); let observed: {id: string; secret: string} | undefined; - await resetAuthentication({ + await resetAuthentication(USER, { schedulerAdapter: {rescheduleAll: async (opts) => { observed = opts.previousKey; }}, userService: env.userService, - options: {context: {user: 'user-1'}}, + options: {}, models: env.models, internalKeys: env.internalKeys, - deleteAllSessions: env.deleteAllSessions + deleteAllSessions: env.deleteAllSessions, + logAction: env.logAction }); assert.deepEqual(observed, {id: 'k', secret: 'pre-rotation'}); }); - it('skips the audit row when no actor is in context', async function () { + it('records nothing for an actor-less (internal) context', async function () { const env = buildAuthDomain({apiKeysToRotate: 1, usersToLock: 0, currentKey: {id: 'k', secret: 's'}}); - await resetAuthentication({ + await resetAuthentication({actor: null}, { schedulerAdapter: {rescheduleAll: async () => {}}, userService: env.userService, options: {}, models: env.models, internalKeys: env.internalKeys, - deleteAllSessions: env.deleteAllSessions + deleteAllSessions: env.deleteAllSessions, + logAction: env.logAction }); - assert.equal(env.recorded.actions.length, 0); + assert.equal(env.recorded.entries.length, 0); }); - it('rolls back rotation and skips sessions + reschedule when lock fails', async function () { + it('rolls back rotation and skips sessions + reschedule + audit when lock fails', async function () { const env = buildAuthDomain({apiKeysToRotate: 2, usersToLock: 0, currentKey: {id: 'k', secret: 's'}}); let rescheduleCalled = false; await assert.rejects( - resetAuthentication({ + resetAuthentication(USER, { schedulerAdapter: {rescheduleAll: async () => { rescheduleCalled = true; }}, userService: {lockAll: async () => { throw new Error('lock failed'); }}, - options: {context: {user: 'user-1'}}, + options: {}, models: env.models, internalKeys: env.internalKeys, - deleteAllSessions: env.deleteAllSessions + deleteAllSessions: env.deleteAllSessions, + logAction: env.logAction }), /lock failed/ ); - assert.equal(env.recorded.actions.length, 0, 'audit row not written on rollback'); + assert.equal(env.recorded.entries.length, 0, 'no audit recorded on rollback'); assert.equal(env.recorded.sessionsDeleted, false, 'sessions are not wiped on rollback'); assert.equal(env.recorded.cacheCleared, false, 'internal-keys cache not cleared on rollback'); assert.equal(rescheduleCalled, false, 'adapter is not asked to reschedule on rollback'); @@ -153,15 +152,16 @@ describe('resetAuthentication', function () { const env = buildAuthDomain({apiKeysToRotate: 1, usersToLock: 1, currentKey: {id: 'k', secret: 's'}}); let sessionsWipedBeforeReschedule = false; - await resetAuthentication({ + await resetAuthentication(USER, { schedulerAdapter: {rescheduleAll: async () => { sessionsWipedBeforeReschedule = env.recorded.sessionsDeleted; }}, userService: env.userService, - options: {context: {user: 'user-1'}}, + options: {}, models: env.models, internalKeys: env.internalKeys, - deleteAllSessions: env.deleteAllSessions + deleteAllSessions: env.deleteAllSessions, + logAction: env.logAction }); assert.equal(sessionsWipedBeforeReschedule, true);