From 4c62b50485b28c19a68982b195dc2e3acbcabd1f Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Wed, 18 Dec 2024 03:28:29 +0000 Subject: [PATCH] Added support for updating the site actor closes https://linear.app/ghost/issue/AP-592 closes https://linear.app/ghost/issue/AP-637 We've refactored the update of the site actor so that it can be used in multiple places. On top of that we've refactored fetching user data from the database, and setting it, and all places which need this data use the helpers so we can ensure that all data stored is complete. This fixes the bug AP-637 which was caused by incomplete user data stored in the database. With the refactored update of the site actor we're able to include updates when we fetch site settings, which happens on every boot of Ghost - this ensures that newly onboarded sites as existing sites will always update their actor to have the latest data on boot. --- features/step_definitions/stepdefs.js | 22 +++ src/app.ts | 43 +++-- src/handlers.ts | 99 +++++------ src/helpers/activitypub/actor.ts | 76 ++++++++- src/helpers/activitypub/actor.unit.test.ts | 189 ++++++++++++++++++++- src/helpers/user.ts | 82 ++++++--- src/helpers/user.unit.test.ts | 4 +- 7 files changed, 406 insertions(+), 109 deletions(-) diff --git a/features/step_definitions/stepdefs.js b/features/step_definitions/stepdefs.js index 1bfee14c..80f56d89 100644 --- a/features/step_definitions/stepdefs.js +++ b/features/step_definitions/stepdefs.js @@ -414,6 +414,28 @@ BeforeAll(async () => { }, }, ); + + ghostActivityPub.register( + { + method: 'GET', + endpoint: '/ghost/api/admin/site', + }, + { + status: 200, + body: { + settings: { + site: { + title: 'Testing Blog', + icon: 'https://ghost.org/favicon.ico', + description: 'A blog for testing', + }, + }, + }, + headers: { + 'Content-Type': 'application/json', + }, + }, + ); }); AfterAll(async () => { diff --git a/src/app.ts b/src/app.ts index e8f6a50c..0f5f02f6 100644 --- a/src/app.ts +++ b/src/app.ts @@ -78,6 +78,7 @@ import { } from './dispatchers'; import { followAction, + getSiteDataHandler, inboxHandler, likeAction, noteAction, @@ -571,30 +572,29 @@ if (queue instanceof GCloudPubSubPushMessageQueue) { app.post('/.ghost/activitypub/mq', spanWrapper(handlePushMessage(queue))); } +app.use(async (ctx, next) => { + const request = ctx.req; + const host = request.header('host'); + if (!host) { + ctx.get('logger').info('No Host header'); + return new Response('No Host header', { + status: 401, + }); + } + + const scopedDb = scopeKvStore(db, ['sites', host]); + + ctx.set('db', scopedDb); + ctx.set('globaldb', db); + + await next(); +}); // This needs to go before the middleware which loads the site // Because the site doesn't always exist - this is how it's created app.get( '/.ghost/activitypub/site', requireRole(GhostRole.Owner), - async (ctx) => { - const request = ctx.req; - const host = request.header('host'); - if (!host) { - ctx.get('logger').info('No Host header'); - return new Response('No Host header', { - status: 401, - }); - } - - const site = await getSite(host, true); - - return new Response(JSON.stringify(site), { - status: 200, - headers: { - 'Content-Type': 'application/json', - }, - }); - }, + getSiteDataHandler, ); app.use(async (ctx, next) => { @@ -606,9 +606,6 @@ app.use(async (ctx, next) => { status: 401, }); } - - const scopedDb = scopeKvStore(db, ['sites', host]); - const site = await getSite(host); if (!site) { @@ -618,8 +615,6 @@ app.use(async (ctx, next) => { }); } - ctx.set('db', scopedDb); - ctx.set('globaldb', db); ctx.set('site', site); await next(); diff --git a/src/handlers.ts b/src/handlers.ts index 6db452ec..2533fba9 100644 --- a/src/handlers.ts +++ b/src/handlers.ts @@ -10,7 +10,6 @@ import { PUBLIC_COLLECTION, type RequestContext, Undo, - Update, isActor, } from '@fedify/fedify'; import { Temporal } from '@js-temporal/polyfill'; @@ -22,13 +21,15 @@ import { buildActivity, prepareNoteContent, } from './helpers/activitypub/activity'; -import { getSiteSettings } from './helpers/ghost'; import { toURL } from './helpers/uri'; -import type { PersonData } from './helpers/user'; +import { getUserData } from './helpers/user'; import { addToList, removeFromList } from './kv-helpers'; import { lookupActor, lookupObject } from './lookup-helpers'; import z from 'zod'; +import { getSite } from './db'; +import { updateSiteActor } from './helpers/activitypub/actor'; +import { getSiteSettings } from './helpers/ghost'; const PostSchema = z.object({ uuid: z.string().uuid(), @@ -545,80 +546,62 @@ export async function postPublishedWebhook( }); } -export async function siteChangedWebhook( +export async function getSiteDataHandler( ctx: Context<{ Variables: HonoContextVariables }>, - next: Next, ) { - try { - // Retrieve site settings from Ghost - const host = ctx.req.header('host') || ''; - - const settings = await getSiteSettings(host); - - // Retrieve the persisted actor details and check if anything has changed - const handle = ACTOR_DEFAULT_HANDLE; - const db = ctx.get('db'); + const request = ctx.req; + const host = request.header('host'); + if (!host) { + ctx.get('logger').info('No Host header'); + return new Response('No Host header', { + status: 401, + }); + } - const current = await db.get(['handle', handle]); + const handle = ACTOR_DEFAULT_HANDLE; + const apCtx = fedify.createContext(ctx.req.raw as Request, { + db: ctx.get('db'), + globaldb: ctx.get('globaldb'), + logger: ctx.get('logger'), + }); - if ( - current && - current.icon === settings.site.icon && - current.name === settings.site.title && - current.summary === settings.site.description - ) { - ctx.get('logger').info('No site settings changed, nothing to do'); + const site = await getSite(host, true); - return new Response(JSON.stringify({}), { - headers: { - 'Content-Type': 'application/activity+json', - }, - status: 200, - }); - } + // This is to ensure that the actor exists - e.g. for a brand new a site + await getUserData(apCtx, handle); - ctx.get('logger').info('Site settings changed, will notify followers'); + await updateSiteActor(apCtx, getSiteSettings); - // Update the database if the site settings have changed - const updated = { - ...current, - icon: settings.site.icon, - name: settings.site.title, - summary: settings.site.description, - }; + return new Response(JSON.stringify(site), { + status: 200, + headers: { + 'Content-Type': 'application/json', + }, + }); +} - await db.set(['handle', handle], updated); +export async function siteChangedWebhook( + ctx: Context<{ Variables: HonoContextVariables }>, +) { + try { + const host = ctx.req.header('host') || ''; + const db = ctx.get('db'); + const globaldb = ctx.get('globaldb'); + const logger = ctx.get('logger'); - // Publish activity if the site settings have changed const apCtx = fedify.createContext(ctx.req.raw as Request, { db, - globaldb: ctx.get('globaldb'), - logger: ctx.get('logger'), + globaldb, + logger, }); - const actor = await apCtx.getActor(handle); - - const update = new Update({ - id: apCtx.getObjectUri(Update, { id: uuidv4() }), - actor: actor?.id, - to: PUBLIC_COLLECTION, - object: actor?.id, - cc: apCtx.getFollowersUri('index'), - }); - - await ctx - .get('globaldb') - .set([update.id!.href], await update.toJsonLd()); - await apCtx.sendActivity({ handle }, 'followers', update, { - preferSharedInbox: true, - }); + await updateSiteActor(apCtx, getSiteSettings); } catch (err) { ctx.get('logger').error('Site changed webhook failed: {error}', { error: err, }); } - // Return 200 OK return new Response(JSON.stringify({}), { headers: { 'Content-Type': 'application/activity+json', diff --git a/src/helpers/activitypub/actor.ts b/src/helpers/activitypub/actor.ts index cb73eaed..f103a27d 100644 --- a/src/helpers/activitypub/actor.ts +++ b/src/helpers/activitypub/actor.ts @@ -1,4 +1,16 @@ -import { type Actor, type KvStore, PropertyValue } from '@fedify/fedify'; +import { + type Actor, + Image, + type KvStore, + PUBLIC_COLLECTION, + PropertyValue, + type RequestContext, + Update, +} from '@fedify/fedify'; +import { v4 as uuidv4 } from 'uuid'; +import type { ContextData } from '../../app'; +import { ACTOR_DEFAULT_HANDLE } from '../../constants'; +import { type UserData, getUserData, setUserData } from '../user'; interface Attachment { name: string; @@ -63,3 +75,65 @@ export async function isFollowing( export function isHandle(handle: string): boolean { return /^@([\w-]+)@([\w-]+\.[\w.-]+)$/.test(handle); } + +export async function updateSiteActor( + apCtx: RequestContext, + getSiteSettings: (host: string) => Promise<{ + site: { icon: string; title: string; description: string }; + }>, +) { + const settings = await getSiteSettings(apCtx.host); + const handle = ACTOR_DEFAULT_HANDLE; + + const current = await getUserData(apCtx, handle); + + if ( + current && + current.icon.url?.toString() === settings.site.icon && + current.name === settings.site.title && + current.summary === settings.site.description + ) { + apCtx.data.logger.info( + 'No site settings changed, not updating site actor', + ); + return false; + } + + const updated: UserData = { + ...current, + }; + + try { + updated.icon = new Image({ url: new URL(settings.site.icon) }); + } catch (err) { + apCtx.data.logger.error( + 'Could not create Image from Icon value ({icon}): {error}', + { icon: settings.site.icon, error: err }, + ); + } + + updated.name = settings.site.title; + updated.summary = settings.site.description; + + await setUserData(apCtx, updated, handle); + + apCtx.data.logger.info('Site settings changed, will notify followers'); + + const actor = await apCtx.getActor(handle); + + const update = new Update({ + id: apCtx.getObjectUri(Update, { id: uuidv4() }), + actor: actor?.id, + to: PUBLIC_COLLECTION, + object: actor?.id, + cc: apCtx.getFollowersUri('index'), + }); + + await apCtx.data.globaldb.set([update.id!.href], await update.toJsonLd()); + + await apCtx.sendActivity({ handle }, 'followers', update, { + preferSharedInbox: true, + }); + + return true; +} diff --git a/src/helpers/activitypub/actor.unit.test.ts b/src/helpers/activitypub/actor.unit.test.ts index 4c8a52f6..d1f111f2 100644 --- a/src/helpers/activitypub/actor.unit.test.ts +++ b/src/helpers/activitypub/actor.unit.test.ts @@ -1,7 +1,14 @@ import { describe, expect, it, vi } from 'vitest'; -import { type Actor, type KvStore, PropertyValue } from '@fedify/fedify'; - +import { + type Actor, + type KvStore, + PropertyValue, + type RequestContext, +} from '@fedify/fedify'; + +import type { Logger } from '@logtape/logtape'; +import type { ContextData } from '../../app'; import { getAttachments, getFollowerCount, @@ -9,6 +16,7 @@ import { getHandle, isFollowing, isHandle, + updateSiteActor, } from './actor'; describe('getAttachments', () => { @@ -226,3 +234,180 @@ describe('isHandle', () => { expect(isHandle('@foo@@example.com')).toBe(false); }); }); + +describe('updateSiteActor', () => { + function mockApContext(db: KvStore, globaldb: KvStore) { + return { + data: { + db, + globaldb, + logger: console as unknown as Logger, + }, + getActor: vi.fn().mockResolvedValue({}), + getInboxUri: vi + .fn() + .mockReturnValue(new URL('https://example.com/inbox')), + getOutboxUri: vi + .fn() + .mockReturnValue(new URL('https://example.com/outbox')), + getLikedUri: vi + .fn() + .mockReturnValue(new URL('https://example.com/liked')), + getFollowingUri: vi + .fn() + .mockReturnValue(new URL('https://example.com/following')), + getActorUri: vi + .fn() + .mockReturnValue(new URL('https://example.com/user/1')), + getActorKeyPairs: vi.fn().mockReturnValue([ + { + cryptographicKey: 'abc123', + }, + ]), + getObjectUri: vi + .fn() + .mockReturnValue(new URL('https://example.com')), + getFollowersUri: vi + .fn() + .mockReturnValue(new URL('https://example.com/followers')), + sendActivity: vi.fn(), + host: 'example.com', + } as unknown as RequestContext; + } + + it('should return false if the site settings have not changed', async () => { + const db = { + get: vi.fn().mockResolvedValue({ + id: 'https://example.com/user/1', + name: 'Site Title', + summary: 'Site Description', + preferredUsername: 'index', + icon: 'https://example.com/icon.png', + inbox: 'https://example.com/inbox', + outbox: 'https://example.com/outbox', + following: 'https://example.com/following', + followers: 'https://example.com/followers', + liked: 'https://example.com/liked', + url: 'https://example.com/', + }), + set: vi.fn(), + delete: vi.fn(), + }; + + const globaldb = { + get: vi.fn().mockResolvedValue(null), + set: vi.fn(), + delete: vi.fn(), + }; + + const getSiteSettings = vi.fn().mockResolvedValue({ + site: { + description: 'Site Description', + title: 'Site Title', + icon: 'https://example.com/icon.png', + }, + }); + + const apCtx = mockApContext(db, globaldb); + + const result = await updateSiteActor(apCtx, getSiteSettings); + + expect(result).toBe(false); + }); + + it('should update the site actor if one does not exist', async () => { + const db = { + get: vi.fn().mockResolvedValue(undefined), + set: vi.fn(), + delete: vi.fn(), + }; + + const globaldb = { + get: vi.fn().mockResolvedValue(null), + set: vi.fn(), + delete: vi.fn(), + }; + + const getSiteSettings = vi.fn().mockResolvedValue({ + site: { + description: 'New Site Description', + title: 'New Site Title', + icon: 'https://example.com/icon.png', + }, + }); + + const apCtx = mockApContext(db, globaldb); + + const result = await updateSiteActor(apCtx, getSiteSettings); + + expect(result).toBe(true); + + expect(db.set.mock.lastCall?.[1]).toStrictEqual({ + id: 'https://example.com/user/1', + name: 'New Site Title', + summary: 'New Site Description', + preferredUsername: 'index', + icon: 'https://example.com/icon.png', + inbox: 'https://example.com/inbox', + outbox: 'https://example.com/outbox', + following: 'https://example.com/following', + followers: 'https://example.com/followers', + liked: 'https://example.com/liked', + url: 'https://example.com/', + }); + }); + + it('should update the site actor if the site settings have changed', async () => { + const db = { + get: vi.fn().mockResolvedValue({ + id: 'https://example.com/user/1', + name: 'Site Title', + summary: 'Site Description', + preferredUsername: 'index', + icon: 'https://example.com/icon.png', + inbox: 'https://example.com/inbox', + outbox: 'https://example.com/outbox', + following: 'https://example.com/following', + followers: 'https://example.com/followers', + liked: 'https://example.com/liked', + url: 'https://example.com/', + }), + set: vi.fn(), + delete: vi.fn(), + }; + + const globaldb = { + get: vi.fn().mockResolvedValue(null), + set: vi.fn(), + delete: vi.fn(), + }; + + const getSiteSettings = vi.fn().mockResolvedValue({ + site: { + description: 'New Site Description', + title: 'New Site Title', + icon: 'https://example.com/icon.png', + }, + }); + + const apCtx = mockApContext(db, globaldb); + + const result = await updateSiteActor(apCtx, getSiteSettings); + + expect(result).toBe(true); + + expect(db.set.mock.calls[0][1]).toStrictEqual({ + id: 'https://example.com/user/1', + name: 'New Site Title', + summary: 'New Site Description', + preferredUsername: 'index', + icon: 'https://example.com/icon.png', + inbox: 'https://example.com/inbox', + outbox: 'https://example.com/outbox', + following: 'https://example.com/following', + followers: 'https://example.com/followers', + liked: 'https://example.com/liked', + url: 'https://example.com/', + }); + }); +}); diff --git a/src/helpers/user.ts b/src/helpers/user.ts index 96db2e7e..c108918f 100644 --- a/src/helpers/user.ts +++ b/src/helpers/user.ts @@ -1,5 +1,6 @@ import { type Context, + type CryptographicKey, Image, exportJwk, generateCryptoKeyPair, @@ -26,7 +27,25 @@ export type PersonData = { url: string; }; -export async function getUserData(ctx: Context, handle: string) { +export type UserData = { + id: URL; + name: string; + summary: string; + preferredUsername: string; + icon: Image; + inbox: URL; + outbox: URL; + following: URL; + followers: URL; + liked: URL; + url: URL; + publicKeys: CryptographicKey[]; +}; + +export async function getUserData( + ctx: Context, + handle: string, +): Promise { const existing = await ctx.data.db.get(['handle', handle]); if (existing) { @@ -38,6 +57,7 @@ export async function getUserData(ctx: Context, handle: string) { 'Could not create Image from Icon value ({icon}): {error}', { icon: existing.icon, error: err }, ); + icon = new Image({ url: new URL(ACTOR_DEFAULT_ICON) }); } let url = null; @@ -48,25 +68,33 @@ export async function getUserData(ctx: Context, handle: string) { 'Could not create URL from value ({url}): {error}', { url: existing.url, error: err }, ); + url = new URL(`https://${ctx.host}`); + } + try { + return { + id: new URL(existing.id), + name: existing.name, + summary: existing.summary, + preferredUsername: existing.preferredUsername, + icon, + inbox: new URL(existing.inbox), + outbox: new URL(existing.outbox), + following: new URL(existing.following), + followers: new URL(existing.followers), + liked: existing.liked + ? new URL(existing.liked) + : ctx.getLikedUri(handle), + publicKeys: (await ctx.getActorKeyPairs(handle)).map( + (key) => key.cryptographicKey, + ), + url, + }; + } catch (err) { + ctx.data.logger.error( + 'Could not create UserData from store value (id: {id}): {error}', + { id: existing.id, error: err }, + ); } - return { - id: new URL(existing.id), - name: existing.name, - summary: existing.summary, - preferredUsername: existing.preferredUsername, - icon, - inbox: new URL(existing.inbox), - outbox: new URL(existing.outbox), - following: new URL(existing.following), - followers: new URL(existing.followers), - liked: existing.liked - ? new URL(existing.liked) - : ctx.getLikedUri(handle), - publicKeys: (await ctx.getActorKeyPairs(handle)).map( - (key) => key.cryptographicKey, - ), - url, - }; } const data = { @@ -86,12 +114,24 @@ export async function getUserData(ctx: Context, handle: string) { url: new URL(`https://${ctx.host}`), }; + await setUserData(ctx, data, handle); + + return data; +} + +// TODO: Consider using handle from `data` +export async function setUserData( + ctx: Context, + data: UserData, + handle: string, +) { + const iconUrl = data.icon.url?.toString() || ''; const dataToStore: PersonData = { id: data.id.href, name: data.name, summary: data.summary, preferredUsername: data.preferredUsername, - icon: ACTOR_DEFAULT_ICON, + icon: iconUrl, inbox: data.inbox.href, outbox: data.outbox.href, following: data.following.href, @@ -101,8 +141,6 @@ export async function getUserData(ctx: Context, handle: string) { }; await ctx.data.db.set(['handle', handle], dataToStore); - - return data; } export async function getUserKeypair( diff --git a/src/helpers/user.unit.test.ts b/src/helpers/user.unit.test.ts index ff6303d7..9d104e12 100644 --- a/src/helpers/user.unit.test.ts +++ b/src/helpers/user.unit.test.ts @@ -174,7 +174,7 @@ describe('getUserData', () => { name: 'foo', summary: 'bar', preferredUsername: HANDLE, - icon: null, + icon: new Image({ url: new URL(ACTOR_DEFAULT_ICON) }), inbox: new URL(INBOX_URI), outbox: new URL(OUTBOX_URI), liked: new URL(LIKED_URI), @@ -220,7 +220,7 @@ describe('getUserData', () => { following: new URL(FOLLOWING_URI), followers: new URL(FOLLOWERS_URI), publicKeys: ['abc123'], - url: null, + url: new URL(`https://${ctx.host}`), }; expect(ctx.data.db.set).toBeCalledTimes(0);