diff --git a/.changeset/spaces-reference-parity.md b/.changeset/spaces-reference-parity.md index bc40f9e..a2f8bdf 100644 --- a/.changeset/spaces-reference-parity.md +++ b/.changeset/spaces-reference-parity.md @@ -3,8 +3,22 @@ '@pdsjs/spaces': minor --- -Bring permissioned data back in line with the reference implementation, and -answer unregistered XRPC methods the way the protocol specifies. +Describe what this server serves, bring permissioned data back in line with the +reference implementation, and answer unregistered XRPC methods the way the +protocol specifies. + +`community.lexicon.service.describe` is served: one unauthenticated query +answering with this service's roles and every XRPC method it routes. Optional +endpoints cannot be discovered any other way, since atproto publishes no method +list, and a client that has to call a space method and read the failure cannot +tell an absent feature from a broken one. The answer is derived from the route +table rather than kept beside it, so it cannot drift and it names the space +methods exactly when spaces are enabled. + +`com.atproto.space.getSpace` answers alongside +`com.atproto.simplespace.getSpace`. The proposal moved the method between +namespaces while implementations were already shipping, and both names are in +use. An unrouted `/xrpc/` returned a `text/plain` 404, which a client cannot tell apart from a transport failure. It now returns 501 with a diff --git a/docs/permissioned-data.md b/docs/permissioned-data.md index 577bfd8..a540330 100644 --- a/docs/permissioned-data.md +++ b/docs/permissioned-data.md @@ -14,6 +14,18 @@ application. scopes, and the `com.atproto.simplespace` management endpoints. Node, Deno, and Cloudflare Workers all support it. +## Telling a client spaces are here + +`community.lexicon.service.describe` answers with this server's roles and every +XRPC method it routes, so the space methods appear in that list exactly when +`PDS_ENABLE_SPACES` is set. A client has no other way to find out: atproto +publishes no method list, and calling a space method to see what happens cannot +separate a server without the feature from one that is broken. + +`com.atproto.space.getSpace` and `com.atproto.simplespace.getSpace` both answer. +The proposal moved the method between namespaces after implementations had +shipped, and a client that guesses the wrong one reads the feature as absent. + ## Blobs A space record's blobs are ordinary account blobs. They are uploaded through diff --git a/packages/core/src/pds.js b/packages/core/src/pds.js index 451acc3..b55fe75 100644 --- a/packages/core/src/pds.js +++ b/packages/core/src/pds.js @@ -456,6 +456,7 @@ export class PersonalDataServer { // a route table and passes it in. Core never imports @pdsjs/spaces; this is // the only seam. With it absent, space endpoints do not exist and fall // through to the normal 404. + /** @type {Routes} */ this.routes = { ...routes, ...this._sync.routes, @@ -469,6 +470,11 @@ export class PersonalDataServer { ...this._oauth.routes, ...this._backup.routes, ...(spaceRoutes ?? {}), + // Declared last so it can see every route above it. The handler reads + // this.routes when it runs, not now. + '/xrpc/community.lexicon.service.describe': { + handler: async () => this.describeService(), + }, }; this.spacesEnabled = Boolean(spaceRoutes); @@ -639,6 +645,35 @@ export class PersonalDataServer { return publicKey; } + /** + * community.lexicon.service.describe - what this service is and serves. + * + * Optional endpoints cannot be discovered any other way. atproto publishes no + * method list, so a client asking "does this server do permissioned data" + * otherwise has to call a space method and read the failure, where an absent + * feature and a broken one look alike. A client two hops away sees neither. + * + * The answer is derived from the route table rather than kept beside it, so + * it cannot drift and it names the space methods exactly when spaces are on. + * Proxied `app.bsky.*` methods are absent because they are not routed here; + * claiming them would describe the AppView's capabilities as this server's. + * + * @returns {Response} + */ + describeService() { + const methods = Object.keys(this.routes) + .filter((path) => path.startsWith('/xrpc/')) + .map((path) => path.slice('/xrpc/'.length)) + // `_health` is an endpoint, not a lexicon method. + .filter((nsid) => nsid.includes('.')) + .sort() + .map((nsid) => ({ + $type: 'community.lexicon.service.describe#nsid', + value: nsid, + })); + return Response.json({ roles: ['pds'], methods }); + } + /** * Handle incoming request - main entry point * @param {Request} request diff --git a/packages/spaces/src/handlers/manage.js b/packages/spaces/src/handlers/manage.js index b1ac2f1..8a022d4 100644 --- a/packages/spaces/src/handlers/manage.js +++ b/packages/spaces/src/handlers/manage.js @@ -252,6 +252,22 @@ export function createManageRoutes(ctx) { return row; } + /** @type {import('@pdsjs/core/pds').Route} */ + const getSpaceRoute = { + auth: 'optional', + handler: async (request, url, auth) => { + const space = url.searchParams.get('space'); + if (!space) return errorResponse('InvalidRequest', 'space is required'); + const denied = await authorizeRead(request, space, auth); + if (denied) return denied; + const row = await spaceStorage.getSpace(space); + if (!row?.isOwner) { + return errorResponse('SpaceNotFound', 'Space not found', 404); + } + return Response.json({ uri: row.uri, ...writeConfig(row) }); + }, + }; + return { '/xrpc/com.atproto.simplespace.createSpace': { method: 'POST', @@ -369,20 +385,14 @@ export function createManageRoutes(ctx) { // Authority role: describe a space. The config names the member policy and // the clients allowed to reach it, so a caller outside the space perimeter // does not get to read it. - '/xrpc/com.atproto.simplespace.getSpace': { - auth: 'optional', - handler: async (request, url, auth) => { - const space = url.searchParams.get('space'); - if (!space) return errorResponse('InvalidRequest', 'space is required'); - const denied = await authorizeRead(request, space, auth); - if (denied) return denied; - const row = await spaceStorage.getSpace(space); - if (!row?.isOwner) { - return errorResponse('SpaceNotFound', 'Space not found', 404); - } - return Response.json({ uri: row.uri, ...writeConfig(row) }); - }, - }, + '/xrpc/com.atproto.simplespace.getSpace': getSpaceRoute, + + // The proposal moved this method from com.atproto.space to + // com.atproto.simplespace while implementations were already shipping, and + // both names are in use: the atproto reference PR serves the second, + // ngerakines.me/atproto-crates the first. Both answer here until the draft + // settles, since a client that guesses wrong reads the feature as absent. + '/xrpc/com.atproto.space.getSpace': getSpaceRoute, '/xrpc/com.atproto.simplespace.addMember': { method: 'POST', diff --git a/packages/spaces/test/manage.test.js b/packages/spaces/test/manage.test.js index 47e200c..289af0a 100644 --- a/packages/spaces/test/manage.test.js +++ b/packages/spaces/test/manage.test.js @@ -373,6 +373,15 @@ describe('getSpace', () => { expect(res.status).toBe(404); }); + it('answers under the com.atproto.space name too', async () => { + await store.putSpace(makeSpaceRow(SPACE, { isOwner: true })); + const res = await get('/xrpc/com.atproto.space.getSpace', { + space: SPACE, + }); + expect(res.status).toBe(200); + expect((await res.json()).uri).toBe(SPACE); + }); + it('refuses a caller with neither a session nor a credential', async () => { await store.putSpace(makeSpaceRow(SPACE, { isOwner: true })); const res = await get( diff --git a/test/service-describe.test.js b/test/service-describe.test.js new file mode 100644 index 0000000..7974bbd --- /dev/null +++ b/test/service-describe.test.js @@ -0,0 +1,108 @@ +// community.lexicon.service.describe is the only way a client learns whether an +// optional feature is served here. atproto publishes no method list, so without +// this a caller has to invoke a space method and read the failure, where an +// absent feature and a broken one look the same. +import { describe, expect, it } from 'vitest'; +import { PersonalDataServer } from '../packages/core/src/pds.js'; + +const PATH = '/xrpc/community.lexicon.service.describe'; + +/** @param {import('../packages/core/src/pds.js').Routes} [spaceRoutes] */ +function createPds(spaceRoutes) { + // Nothing here reaches storage, so the ports can be bare stubs. + return new PersonalDataServer({ + actorStorage: /** @type {any} */ ({}), + sharedStorage: /** @type {any} */ ({}), + blobs: /** @type {any} */ ({}), + jwtSecret: 'test-secret', + spaceRoutes, + }); +} + +/** @param {PersonalDataServer} pds */ +async function describeBody(pds) { + const response = await pds.fetch( + new Request(`https://pds.example.com${PATH}`), + ); + expect(response.status).toBe(200); + return response.json(); +} + +describe('community.lexicon.service.describe', () => { + it('answers unauthenticated, naming the pds role', async () => { + const body = await describeBody(createPds()); + expect(body.roles).toEqual(['pds']); + }); + + it('names each method with its union member', async () => { + const body = await describeBody(createPds()); + for (const entry of body.methods) { + expect(entry.$type).toBe('community.lexicon.service.describe#nsid'); + expect(entry.value).toMatch(/^[a-z][a-z0-9.]*\.[a-zA-Z]+$/); + } + }); + + it('describes exactly the routed methods, sorted', async () => { + const pds = createPds(); + const body = await describeBody(pds); + + const routed = Object.keys(pds.routes) + .filter((p) => p.startsWith('/xrpc/')) + .map((p) => p.slice('/xrpc/'.length)) + .filter((n) => n.includes('.')) + .sort(); + + expect(body.methods.map((/** @type {any} */ m) => m.value)).toEqual(routed); + }); + + it('describes itself', async () => { + const body = await describeBody(createPds()); + expect(body.methods.map((/** @type {any} */ m) => m.value)).toContain( + 'community.lexicon.service.describe', + ); + }); + + it('omits _health, which is an endpoint rather than a method', async () => { + const body = await describeBody(createPds()); + const values = body.methods.map((/** @type {any} */ m) => m.value); + expect(values).not.toContain('_health'); + expect(values.some((/** @type {string} */ v) => v.startsWith('_'))).toBe( + false, + ); + }); + + it('omits the proxied app.bsky methods it does not route', async () => { + const body = await describeBody(createPds()); + const values = body.methods.map((/** @type {any} */ m) => m.value); + // Served locally, so declared. + expect(values).toContain('app.bsky.actor.getPreferences'); + // Forwarded to the AppView; claiming it would describe the AppView. + expect(values).not.toContain('app.bsky.feed.getTimeline'); + }); + + it('names no space method while spaces are off', async () => { + const body = await describeBody(createPds()); + const values = body.methods.map((/** @type {any} */ m) => m.value); + expect(values.some((/** @type {string} */ v) => v.includes('space'))).toBe( + false, + ); + }); + + it('names the space methods once spaces are on', async () => { + const body = await describeBody( + createPds( + /** @type {any} */ ({ + '/xrpc/com.atproto.space.listSpaces': { + handler: async () => new Response(), + }, + '/xrpc/com.atproto.simplespace.createSpace': { + handler: async () => new Response(), + }, + }), + ), + ); + const values = body.methods.map((/** @type {any} */ m) => m.value); + expect(values).toContain('com.atproto.space.listSpaces'); + expect(values).toContain('com.atproto.simplespace.createSpace'); + }); +});