Extracted a shared action logger for the audit log

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

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<void> {
     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 = {

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};
+}

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<void>;
+
+interface ActionRecorder {
+    add(data: Record<string, unknown>, options: {autoRefresh: boolean}): Promise<unknown>;
+}
+
+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);
+        }
+    };
+}

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<void>;
+    logAction?: LogAction;
 }
 
 interface ResetAuthenticationResult {
@@ -19,51 +21,40 @@ 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<ResetAuthenticationResult> {
     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});
 
         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});

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)});
 }

ghost/core/core/server/services/gift-links/service.ts

@@ -1,46 +1,33 @@
 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';
 
 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<string, unknown>, options: {autoRefresh: boolean}): Promise<unknown>;
-}
-
-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".
 const COMMANDS = {
     add: 'added',
     reset: 'edited',
     remove: 'deleted'
-} as const satisfies Record<string, 'added' | 'edited' | 'deleted'>;
+} as const satisfies Record<string, ActionEvent>;
 
 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<Post> {
@@ -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} : {})
+        });
     }
 }

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'}})
+        );
+    });
+});

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));