From 606d570d8ee1f9e1eff423e50758d9f248f2956e Mon Sep 17 00:00:00 2001 From: Mike Yumatov Date: Mon, 22 Jun 2026 15:40:38 +0300 Subject: [PATCH] feat: RW docs comments integration + dev Alice/Bob mock auth Adds inline commenting on RW documentation rendered in Backstage, plus a dev-only multi-user auth harness to exercise it. Backend (@rwdocs/backstage-plugin-rw-backend): - SQLite/Postgres CommentStore (knex migration, uuid PK, soft-delete, atomic resolve-stamp, transactional soft-delete/restore closing a TOCTOU). - Entity-scoped /comments router: create/list/get/PATCH(resolve|edit|restore)/ delete with input validation (16 KiB body cap, parentId/siteRef checks), Backstage permission dispatch + author floor, author profile resolution (displayName/avatar), structured logging, 503 on catalog outage. Common (@rwdocs/backstage-plugin-rw-common): - rwComment.{read,create,edit,resolve,delete} permissions + isCommentAuthor rule. Frontend (@rwdocs/backstage-plugin-rw): - RwClient comment client + comments wiring through RwEntityDocsViewer / RwDocsViewer; warn (not silently disable) when the comments-enabled probe fails. Dev harness (packages/app, packages/backend, examples): - Dev-only Alice/Bob proxy auth providers + a SignInPage picker (delegating to ProxiedSignInPage, with localStorage resume); alice/bob User catalog entities. Deps: @rwdocs/{viewer,core} ^0.1.27 (embedded comment deep-linking: copy-link button + n/p URL-hash mirroring via replaceState). Co-Authored-By: Claude Opus 4.8 (1M context) --- .yarnrc.yml | 10 + app-config.yaml | 9 + examples/entities.yaml | 24 + packages/app/package.json | 4 + packages/app/src/App.tsx | 5 +- packages/app/src/DevUsersSignInPage.tsx | 64 ++ packages/app/src/devSignInPage.ts | 13 + packages/backend/package.json | 1 + packages/backend/src/devUsersAuthModule.ts | 61 ++ packages/backend/src/index.ts | 3 + .../20260621000000_init_comments.js | 31 + plugins/rw-backend/package.json | 12 +- .../src/comments/CommentStore.test.ts | 272 ++++++ .../rw-backend/src/comments/CommentStore.ts | 148 +++ .../rw-backend/src/comments/author.test.ts | 61 ++ plugins/rw-backend/src/comments/author.ts | 37 + .../src/comments/integration.test.ts | 203 ++++ .../rw-backend/src/comments/logging.test.ts | 45 + plugins/rw-backend/src/comments/logging.ts | 67 ++ .../rw-backend/src/comments/mapping.test.ts | 69 ++ plugins/rw-backend/src/comments/mapping.ts | 52 + .../src/comments/migrations.test.ts | 44 + .../comments/permissions-integration.test.ts | 160 ++++ .../src/comments/permissions.test.ts | 19 + .../rw-backend/src/comments/permissions.ts | 31 + .../rw-backend/src/comments/router.test.ts | 887 ++++++++++++++++++ plugins/rw-backend/src/comments/router.ts | 404 ++++++++ .../src/comments/timestamps.test.ts | 25 + plugins/rw-backend/src/comments/timestamps.ts | 30 + plugins/rw-backend/src/comments/types.ts | 57 ++ plugins/rw-backend/src/plugin.ts | 79 +- plugins/rw-backend/src/router.test.ts | 68 +- plugins/rw-backend/src/router.ts | 2 +- plugins/rw-common/config.d.ts | 12 + plugins/rw-common/package.json | 1 + plugins/rw-common/src/index.ts | 1 + plugins/rw-common/src/permissions.test.ts | 53 ++ plugins/rw-common/src/permissions.ts | 51 + plugins/rw/package.json | 2 +- plugins/rw/src/__mocks__/@rwdocs/viewer.js | 9 +- plugins/rw/src/api/RwClient.test.ts | 73 ++ plugins/rw/src/api/RwClient.ts | 61 ++ .../rw/src/components/RwDocsViewer.test.tsx | 45 +- plugins/rw/src/components/RwDocsViewer.tsx | 13 +- .../components/RwEntityDocsViewer.test.tsx | 213 +++++ .../rw/src/components/RwEntityDocsViewer.tsx | 46 +- plugins/search-backend-module-rw/package.json | 2 +- yarn.lock | 70 +- 48 files changed, 3557 insertions(+), 92 deletions(-) create mode 100644 examples/entities.yaml create mode 100644 packages/app/src/DevUsersSignInPage.tsx create mode 100644 packages/app/src/devSignInPage.ts create mode 100644 packages/backend/src/devUsersAuthModule.ts create mode 100644 plugins/rw-backend/migrations/20260621000000_init_comments.js create mode 100644 plugins/rw-backend/src/comments/CommentStore.test.ts create mode 100644 plugins/rw-backend/src/comments/CommentStore.ts create mode 100644 plugins/rw-backend/src/comments/author.test.ts create mode 100644 plugins/rw-backend/src/comments/author.ts create mode 100644 plugins/rw-backend/src/comments/integration.test.ts create mode 100644 plugins/rw-backend/src/comments/logging.test.ts create mode 100644 plugins/rw-backend/src/comments/logging.ts create mode 100644 plugins/rw-backend/src/comments/mapping.test.ts create mode 100644 plugins/rw-backend/src/comments/mapping.ts create mode 100644 plugins/rw-backend/src/comments/migrations.test.ts create mode 100644 plugins/rw-backend/src/comments/permissions-integration.test.ts create mode 100644 plugins/rw-backend/src/comments/permissions.test.ts create mode 100644 plugins/rw-backend/src/comments/permissions.ts create mode 100644 plugins/rw-backend/src/comments/router.test.ts create mode 100644 plugins/rw-backend/src/comments/router.ts create mode 100644 plugins/rw-backend/src/comments/timestamps.test.ts create mode 100644 plugins/rw-backend/src/comments/timestamps.ts create mode 100644 plugins/rw-backend/src/comments/types.ts create mode 100644 plugins/rw-common/src/permissions.test.ts create mode 100644 plugins/rw-common/src/permissions.ts diff --git a/.yarnrc.yml b/.yarnrc.yml index 3186f3f..20364fd 100644 --- a/.yarnrc.yml +++ b/.yarnrc.yml @@ -1 +1,11 @@ nodeLinker: node-modules + +# @rwdocs/{core,viewer} 0.1.27 was just published; preapprove it past yarn's +# npmMinimalAgeGate (24h supply-chain quarantine). These are first-party +# packages we publish. Removable once 0.1.27 is >24h old. +npmPreapprovedPackages: + - "@rwdocs/core@^0.1.27" + - "@rwdocs/core-darwin-arm64@^0.1.27" + - "@rwdocs/core-linux-x64-gnu@^0.1.27" + - "@rwdocs/core-linux-x64-musl@^0.1.27" + - "@rwdocs/viewer@^0.1.27" diff --git a/app-config.yaml b/app-config.yaml index ad7e8a5..bc0e5e6 100644 --- a/app-config.yaml +++ b/app-config.yaml @@ -24,6 +24,15 @@ backend: auth: providers: guest: {} + alice: {} + bob: {} + +catalog: + rules: + - allow: [Component, Domain, System, Location, User, Group] + locations: + - type: file + target: ../../examples/entities.yaml rw: projectDir: ${RW_PROJECT_DIR} diff --git a/examples/entities.yaml b/examples/entities.yaml new file mode 100644 index 0000000..d2c39f7 --- /dev/null +++ b/examples/entities.yaml @@ -0,0 +1,24 @@ +--- +apiVersion: backstage.io/v1alpha1 +kind: User +metadata: + name: alice + namespace: default +spec: + profile: + displayName: Alice Anderson + email: alice@example.com + picture: https://api.dicebear.com/9.x/identicon/svg?seed=alice + memberOf: [] +--- +apiVersion: backstage.io/v1alpha1 +kind: User +metadata: + name: bob + namespace: default +spec: + profile: + displayName: Bob Brown + email: bob@example.com + picture: https://api.dicebear.com/9.x/identicon/svg?seed=bob + memberOf: [] diff --git a/packages/app/package.json b/packages/app/package.json index 97d33fa..3cadaba 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -13,7 +13,11 @@ }, "dependencies": { "@backstage/cli": "^0.36.0", + "@backstage/core-components": "^0.18.11", + "@backstage/core-plugin-api": "^1.12.7", "@backstage/frontend-defaults": "^0.5.0", + "@backstage/frontend-plugin-api": "^0.17.2", + "@backstage/plugin-app-react": "^0.2.4", "@backstage/plugin-catalog": "^2.0.0", "@backstage/plugin-search": "^1.7.0", "@backstage/plugin-user-settings": "^0.9.0", diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index 41eb110..cf01768 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -3,9 +3,12 @@ import catalogPlugin from '@backstage/plugin-catalog/alpha'; import searchPlugin from '@backstage/plugin-search/alpha'; import userSettingsPlugin from '@backstage/plugin-user-settings/alpha'; import rwPlugin from '@rwdocs/backstage-plugin-rw'; +import { devSignInPage } from './devSignInPage'; const app = createApp({ - features: [catalogPlugin, searchPlugin, userSettingsPlugin, rwPlugin], + // devSignInPage (Alice/Bob picker) is always included: this instance is a + // demo/test stand for the plugins, never a production build. + features: [catalogPlugin, searchPlugin, userSettingsPlugin, rwPlugin, devSignInPage], }); export default app.createRoot(); diff --git a/packages/app/src/DevUsersSignInPage.tsx b/packages/app/src/DevUsersSignInPage.tsx new file mode 100644 index 0000000..2583179 --- /dev/null +++ b/packages/app/src/DevUsersSignInPage.tsx @@ -0,0 +1,64 @@ +import { useState } from 'react'; +import { ProxiedSignInPage } from '@backstage/core-components'; +import type { IdentityApi, SignInPageProps } from '@backstage/core-plugin-api'; + +const STORAGE_KEY = 'rw-dev-user'; + +const USERS = [ + { id: 'alice', label: 'Sign in as Alice' }, + { id: 'bob', label: 'Sign in as Bob' }, +] as const; + +// Remember the chosen dev user so a page reload silently re-signs-in (via +// ProxiedSignInPage) instead of re-showing the picker — matching how the guest +// provider auto-resumes. Ignores an unknown stored value (e.g. a removed user). +function readStoredProvider(): string | null { + try { + const value = localStorage.getItem(STORAGE_KEY); + return USERS.some(u => u.id === value) ? value : null; + } catch { + return null; + } +} + +export function DevUsersSignInPage(props: SignInPageProps) { + const [provider, setProvider] = useState(readStoredProvider); + + // Persist the choice on success, and wrap signOut so it forgets the choice — + // that way the picker returns after sign-out and a different user can be + // selected, but a plain reload resumes the current one. + const handleSignInSuccess = (identity: IdentityApi) => { + try { + localStorage.setItem(STORAGE_KEY, provider!); + } catch { + // localStorage unavailable (private mode etc.) — resume just won't persist. + } + const signOut = identity.signOut.bind(identity); + identity.signOut = async () => { + try { + localStorage.removeItem(STORAGE_KEY); + } catch { + // ignore + } + await signOut(); + }; + props.onSignInSuccess(identity); + }; + + if (provider) { + return ( + + ); + } + + return ( +
+

Dev sign-in

+ {USERS.map(u => ( + + ))} +
+ ); +} diff --git a/packages/app/src/devSignInPage.ts b/packages/app/src/devSignInPage.ts new file mode 100644 index 0000000..77fd52e --- /dev/null +++ b/packages/app/src/devSignInPage.ts @@ -0,0 +1,13 @@ +import { createFrontendModule } from '@backstage/frontend-plugin-api'; +import { SignInPageBlueprint } from '@backstage/plugin-app-react'; + +export const devSignInPage = createFrontendModule({ + pluginId: 'app', + extensions: [ + SignInPageBlueprint.make({ + params: { + loader: async () => (await import('./DevUsersSignInPage')).DevUsersSignInPage, + }, + }), + ], +}); diff --git a/packages/backend/package.json b/packages/backend/package.json index 9e1f416..f9f05db 100644 --- a/packages/backend/package.json +++ b/packages/backend/package.json @@ -18,6 +18,7 @@ "@backstage/plugin-app-backend": "^0.5.11", "@backstage/plugin-auth-backend": "^0.29.0", "@backstage/plugin-auth-backend-module-guest-provider": "^0.2.16", + "@backstage/plugin-auth-node": "^0.7.2", "@backstage/plugin-catalog-backend": "^3.4.0", "@backstage/plugin-catalog-backend-module-scaffolder-entity-model": "^0.2.17", "@backstage/plugin-permission-backend": "^0.7.9", diff --git a/packages/backend/src/devUsersAuthModule.ts b/packages/backend/src/devUsersAuthModule.ts new file mode 100644 index 0000000..706c751 --- /dev/null +++ b/packages/backend/src/devUsersAuthModule.ts @@ -0,0 +1,61 @@ +import { createBackendModule } from '@backstage/backend-plugin-api'; +import { + authProvidersExtensionPoint, + createProxyAuthProviderFactory, + createProxyAuthenticator, +} from '@backstage/plugin-auth-node'; +import type { SignInResolver } from '@backstage/plugin-auth-node'; + +// Dev-only fixed identities. The provider id IS the user. +const DEV_USERS = ['alice', 'bob'] as const; + +// A no-op proxy authenticator: there is nothing to authenticate — the chosen +// provider determines the user. +function fixedUserAuthenticator() { + return createProxyAuthenticator({ + defaultProfileTransform: async () => ({ profile: {} }), + initialize(_ctx) { + return {}; + }, + async authenticate(_options, _ctx) { + return { result: {} }; + }, + }); +} + +// Signs the caller in as a fixed catalog user (real profile + ownership); +// falls back to a bare token if the entity isn't in the catalog yet. +function signInAsUser(userEntityRef: string): SignInResolver<{}> { + return async (_info, ctx) => { + try { + return await ctx.signInWithCatalogUser({ entityRef: userEntityRef }); + } catch { + return ctx.issueToken({ + claims: { sub: userEntityRef, ent: [userEntityRef] }, + }); + } + }; +} + +export default createBackendModule({ + pluginId: 'auth', + moduleId: 'dev-users-provider', + register(reg) { + reg.registerInit({ + deps: { providers: authProvidersExtensionPoint }, + async init({ providers }) { + // The authenticator is stateless and identical for every provider. + const authenticator = fixedUserAuthenticator(); + for (const name of DEV_USERS) { + providers.registerProvider({ + providerId: name, + factory: createProxyAuthProviderFactory({ + authenticator, + signInResolver: signInAsUser(`user:default/${name}`), + }), + }); + } + }, + }); + }, +}); diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index f7eecc5..05b3e89 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -5,6 +5,9 @@ const backend = createBackend(); backend.add(import('@backstage/plugin-app-backend')); backend.add(import('@backstage/plugin-auth-backend')); backend.add(import('@backstage/plugin-auth-backend-module-guest-provider')); +// Dev Alice/Bob auth providers — this instance is a demo/test stand, never a +// production build. +backend.add(import('./devUsersAuthModule')); backend.add(import('@backstage/plugin-catalog-backend')); backend.add(import('@backstage/plugin-catalog-backend-module-scaffolder-entity-model')); backend.add(import('@backstage/plugin-permission-backend')); diff --git a/plugins/rw-backend/migrations/20260621000000_init_comments.js b/plugins/rw-backend/migrations/20260621000000_init_comments.js new file mode 100644 index 0000000..df4b4c0 --- /dev/null +++ b/plugins/rw-backend/migrations/20260621000000_init_comments.js @@ -0,0 +1,31 @@ +// @ts-check +/** @param {import('knex').Knex} knex */ +exports.up = async function up(knex) { + await knex.schema.createTable('comments', table => { + table.uuid('id').primary(); // uuid v7 + table.text('site_ref').notNullable(); + table.text('document_id').notNullable(); + table.text('entity_ref').notNullable(); + table.uuid('parent_id').nullable(); + table.text('author_ref').notNullable(); + table.text('author_profile').nullable(); // JSON {displayName, picture?} + table.text('body').notNullable(); + table.text('body_html').notNullable(); + table.text('selectors').notNullable(); // JSON Selector[] + table.text('status').notNullable(); // 'open' | 'resolved' + table.dateTime('created_at').notNullable(); + table.dateTime('updated_at').notNullable(); + table.dateTime('resolved_at').nullable(); + table.text('resolved_by').nullable(); + table.dateTime('deleted_at').nullable(); + table.index(['site_ref', 'document_id'], 'comments_site_doc_idx'); + table.index(['site_ref', 'document_id', 'status'], 'comments_site_doc_status_idx'); + table.index(['parent_id'], 'comments_parent_idx'); + table.index(['entity_ref'], 'comments_entity_idx'); + }); +}; + +/** @param {import('knex').Knex} knex */ +exports.down = async function down(knex) { + await knex.schema.dropTableIfExists('comments'); +}; diff --git a/plugins/rw-backend/package.json b/plugins/rw-backend/package.json index 827d7df..7c3ab14 100644 --- a/plugins/rw-backend/package.json +++ b/plugins/rw-backend/package.json @@ -29,6 +29,7 @@ }, "files": [ "dist", + "migrations", "LICENSE-MIT", "LICENSE-APACHE" ], @@ -47,16 +48,23 @@ "@backstage/catalog-model": "^1.7.6", "@backstage/config": "^1.3.6", "@backstage/errors": "^1.2.7", + "@backstage/plugin-catalog-node": "^2.2.2", + "@backstage/plugin-permission-common": "^0.9.9", + "@backstage/plugin-permission-node": "^0.11.1", "@backstage/types": "^1.2.2", "@rwdocs/backstage-plugin-rw-common": "workspace:^", - "@rwdocs/core": "^0.1.23", + "@rwdocs/core": "^0.1.27", "express": "^4.21.0", - "express-promise-router": "^4.1.0" + "express-promise-router": "^4.1.0", + "luxon": "^3.7.2", + "uuid": "^11.0.0", + "zod": "^3.25.76 || ^4.0.0" }, "peerDependencies": { "@backstage/backend-plugin-api": "^1.0.0" }, "devDependencies": { + "@backstage/backend-defaults": "^0.17.3", "@backstage/backend-plugin-api": "^1.0.0", "@backstage/backend-test-utils": "^1.11.0", "@backstage/cli": "^0.36.0", diff --git a/plugins/rw-backend/src/comments/CommentStore.test.ts b/plugins/rw-backend/src/comments/CommentStore.test.ts new file mode 100644 index 0000000..d50820c --- /dev/null +++ b/plugins/rw-backend/src/comments/CommentStore.test.ts @@ -0,0 +1,272 @@ +import { TestDatabases } from "@backstage/backend-test-utils"; +import { resolvePackagePath } from "@backstage/backend-plugin-api"; +import type { Knex } from "knex"; +import { CommentStore } from "./CommentStore"; +import { toIso } from "./timestamps"; + +jest.mock("@rwdocs/core", () => ({ + renderCommentBody: jest.fn(async (md: string) => `

${md}

`), +})); + +const ARCH = "component:default/arch"; +const ROOT_DOC = "section:default/root#guide/intro"; + +async function freshStore(databases: TestDatabases): Promise<{ store: CommentStore; knex: Knex }> { + const knex = await databases.init("SQLITE_3"); + const directory = resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"); + await knex.migrate.latest({ directory }); + return { store: new CommentStore(knex), knex }; +} + +describe("CommentStore read core", () => { + const databases = TestDatabases.create({ ids: ["SQLITE_3"] }); + + it("create stores a row, renders body_html, and computes entity_ref (root → site_ref)", async () => { + const { store } = await freshStore(databases); + const row = await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/alice", + authorProfile: { displayName: "Alice" }, + body: "hello", + selectors: [], + }); + expect(row.id).toMatch(/[0-9a-f-]{36}/); + expect(row.site_ref).toBe(ARCH); + expect(row.document_id).toBe(ROOT_DOC); + expect(row.entity_ref).toBe(ARCH); // root → host + expect(row.body_html).toBe("

hello

"); + expect(row.status).toBe("open"); + }); + + it("create computes entity_ref = sectionRef for an embedded section", async () => { + const { store } = await freshStore(databases); + const row = await store.create(ARCH, { + documentId: "domain:default/billing#overview", + authorRef: "user:default/alice", + body: "x", + selectors: [], + }); + expect(row.entity_ref).toBe("domain:default/billing"); + }); + + it("list returns the full thread for a page, ORDER BY created_at ASC", async () => { + const { store } = await freshStore(databases); + const a = await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/a", + body: "1", + selectors: [], + }); + const b = await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/b", + body: "2", + selectors: [], + }); + const rows = await store.list(ARCH, { documentId: ROOT_DOC }); + expect(rows.map((r) => r.id)).toEqual([a.id, b.id]); + }); + + it("list scopes by site_ref (synthetic-root collision avoided)", async () => { + const { store } = await freshStore(databases); + await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/a", + body: "arch", + selectors: [], + }); + await store.create("component:default/other", { + documentId: ROOT_DOC, + authorRef: "user:default/a", + body: "other", + selectors: [], + }); + const rows = await store.list(ARCH, { documentId: ROOT_DOC }); + expect(rows).toHaveLength(1); + expect(rows[0].body).toBe("arch"); + }); + + it("list returns same-created_at rows in id-ascending order (deterministic tiebreaker)", async () => { + const { store, knex } = await freshStore(databases); + const t = new Date("2024-01-01T00:00:00.000Z"); + const a = await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/a", + body: "first", + selectors: [], + }); + const b = await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/b", + body: "second", + selectors: [], + }); + // Force identical created_at so only the id tiebreaker determines order. + await knex("comments").where({ id: a.id }).update({ created_at: t }); + await knex("comments").where({ id: b.id }).update({ created_at: t }); + const rows = await store.list(ARCH, { documentId: ROOT_DOC }); + expect(rows.map((r) => r.id)).toEqual([a.id, b.id]); // uuid v7 is time-ordered, a was created first + }); + + it("get returns by global id without a siteRef", async () => { + const { store } = await freshStore(databases); + const row = await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/a", + body: "x", + selectors: [], + }); + const got = await store.get(row.id); + expect(got?.id).toBe(row.id); + expect(await store.get("00000000-0000-0000-0000-000000000000")).toBeUndefined(); + }); +}); + +describe("CommentStore mutations", () => { + const databases = TestDatabases.create({ ids: ["SQLITE_3"] }); + + it("resolve stamps resolved_at + resolved_by; reopen clears both", async () => { + const { store } = await freshStore(databases); + const c = await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/a", + body: "x", + selectors: [], + }); + + const resolved = await store.update(c.id, { + status: "resolved", + resolverRef: "user:default/bob", + }); + expect(resolved?.status).toBe("resolved"); + expect(resolved?.resolved_by).toBe("user:default/bob"); // may differ from author + expect(resolved?.resolved_at).not.toBeNull(); + + const reopened = await store.update(c.id, { status: "open" }); + expect(reopened?.status).toBe("open"); + expect(reopened?.resolved_at).toBeNull(); + expect(reopened?.resolved_by).toBeNull(); + }); + + it("idempotent re-resolve keeps the original resolved_at/resolved_by", async () => { + const { store } = await freshStore(databases); + const c = await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/a", + body: "x", + selectors: [], + }); + const first = await store.update(c.id, { status: "resolved", resolverRef: "user:default/bob" }); + const again = await store.update(c.id, { + status: "resolved", + resolverRef: "user:default/carol", + }); + expect(again?.resolved_by).toBe("user:default/bob"); // unchanged + expect(again?.resolved_at).toEqual(first?.resolved_at); // original resolution timestamp preserved + }); + + it("a body change re-renders body_html and bumps updated_at", async () => { + const { store } = await freshStore(databases); + const c = await store.create(ARCH, { + documentId: ROOT_DOC, + authorRef: "user:default/a", + body: "x", + selectors: [], + }); + // Capture updated_at timestamp before the update (as ISO string for cross-driver comparison). + const beforeIso = toIso(c.updated_at)!; + const beforeMs = Date.parse(beforeIso); + + // Delay 1 ms so the DB clock can advance. + await new Promise((r) => setTimeout(r, 1)); + + const updated = await store.update(c.id, { body: "changed" }); + expect(updated?.body).toBe("changed"); + expect(updated?.body_html).toBe("

changed

"); + + const afterIso = toIso(updated!.updated_at)!; + const afterMs = Date.parse(afterIso); + expect(afterMs).toBeGreaterThanOrEqual(beforeMs + 1); + }); + + it("softDelete sets deleted_at (and list excludes it); restore clears it", async () => { + const { store } = await freshStore(databases); + const reply = await store.create(ARCH, { + documentId: ROOT_DOC, + parentId: "p", + authorRef: "user:default/a", + body: "r", + selectors: [], + }); + await store.softDelete(reply.id); + expect(await store.list(ARCH, { documentId: ROOT_DOC })).toHaveLength(0); + const restored = await store.restore(reply.id); + expect(restored?.deleted_at).toBeNull(); + expect(await store.list(ARCH, { documentId: ROOT_DOC })).toHaveLength(1); + }); + + it("softDelete on an already-deleted row returns undefined and does not bump updated_at", async () => { + const { store } = await freshStore(databases); + const reply = await store.create(ARCH, { + documentId: ROOT_DOC, + parentId: "p", + authorRef: "user:default/a", + body: "r", + selectors: [], + }); + const first = await store.softDelete(reply.id); + expect(first?.deleted_at).not.toBeNull(); + const firstUpdatedIso = toIso(first!.updated_at)!; + + // Delay so the DB clock would advance if a second write occurred. + await new Promise((r) => setTimeout(r, 5)); + + const second = await store.softDelete(reply.id); + expect(second).toBeUndefined(); + + // The row's updated_at must NOT have moved — the second delete was a no-op. + const after = await store.get(reply.id); + expect(toIso(after!.updated_at)).toBe(firstUpdatedIso); + }); + + it("restore on a non-deleted row returns undefined", async () => { + const { store } = await freshStore(databases); + const reply = await store.create(ARCH, { + documentId: ROOT_DOC, + parentId: "p", + authorRef: "user:default/a", + body: "r", + selectors: [], + }); + expect(await store.restore(reply.id)).toBeUndefined(); + }); + + it("two racing soft-deletes: exactly one wins, the other sees undefined", async () => { + const { store } = await freshStore(databases); + const reply = await store.create(ARCH, { + documentId: ROOT_DOC, + parentId: "p", + authorRef: "user:default/a", + body: "r", + selectors: [], + }); + + const race = async () => + store.transaction(async (tx) => { + // Lock the row so one transaction blocks until the other commits — this + // serialises the two writers. The inline deleted_at guard is intentionally + // absent: softDelete's own whereNull("deleted_at") is the only thing that + // decides which racer wins. + await store.get(reply.id, { executor: tx, forUpdate: true }); + return store.softDelete(reply.id, tx); + }); + + const [a, b] = await Promise.all([race(), race()]); + const results = [a, b]; + const winners = results.filter((r) => r !== undefined); + const losers = results.filter((r) => r === undefined); + expect(winners).toHaveLength(1); + expect(losers).toHaveLength(1); + expect(winners[0]!.deleted_at).not.toBeNull(); + }); +}); diff --git a/plugins/rw-backend/src/comments/CommentStore.ts b/plugins/rw-backend/src/comments/CommentStore.ts new file mode 100644 index 0000000..7580bfc --- /dev/null +++ b/plugins/rw-backend/src/comments/CommentStore.ts @@ -0,0 +1,148 @@ +import type { Knex } from "knex"; +import { v7 as uuidv7 } from "uuid"; +import { renderCommentBody } from "@rwdocs/core"; +import { + CommentRow, + CommentStatus, + CreateCommentInput, + ListFilter, + computeEntityRef, +} from "./types"; + +const TABLE = "comments"; + +export class CommentStore { + constructor(private readonly knex: Knex) {} + + async create(siteRef: string, input: CreateCommentInput): Promise { + const now = new Date(); + const row: CommentRow = { + id: uuidv7(), + site_ref: siteRef, + document_id: input.documentId, + entity_ref: computeEntityRef(input.documentId, siteRef), + parent_id: input.parentId ?? null, + author_ref: input.authorRef, + author_profile: input.authorProfile ? JSON.stringify(input.authorProfile) : null, + body: input.body, + body_html: await renderCommentBody(input.body), + selectors: JSON.stringify(input.selectors), + status: "open", + created_at: now, + updated_at: now, + resolved_at: null, + resolved_by: null, + deleted_at: null, + }; + await this.knex(TABLE).insert(row); + return row; + } + + /** + * By global uuid; includes soft-deleted rows. + * + * Pass `opts.executor` (a transaction) to read inside an open transaction, and + * `opts.forUpdate` to lock the row for the rest of the transaction (real + * `SELECT ... FOR UPDATE` on Postgres; ignored/no-op on better-sqlite3, whose + * transactions already serialize writes). + */ + async get( + id: string, + opts?: { executor?: Knex.Transaction; forUpdate?: boolean }, + ): Promise { + const q = (opts?.executor ?? this.knex)(TABLE).where({ id }); + if (opts?.forUpdate) q.forUpdate(); + return q.first(); + } + + /** Run `fn` inside a single database transaction. */ + async transaction(fn: (tx: Knex.Transaction) => Promise): Promise { + return this.knex.transaction(fn); + } + + /** + * Site-scoped; ALWAYS excludes soft-deleted rows; ORDER BY created_at ASC. + * + * Forward hook: the router currently reads only by `documentId`. The additional + * `ListFilter` fields (`entityRef`, `status`, `parentId`, `topLevelOnly`) and the + * corresponding `entity_ref` column + `comments_entity_idx` index are intentional + * forward hooks for the planned cross-section / entity-scoped querying direction (v1 + * design). They are not dead code. + */ + async list(siteRef: string, filter: ListFilter): Promise { + const q = this.knex(TABLE).where({ site_ref: siteRef }).whereNull("deleted_at"); + if (filter.documentId !== undefined) q.andWhere({ document_id: filter.documentId }); + if (filter.entityRef !== undefined) q.andWhere({ entity_ref: filter.entityRef }); + if (filter.status !== undefined) q.andWhere({ status: filter.status }); + if (filter.parentId !== undefined) q.andWhere({ parent_id: filter.parentId }); + if (filter.topLevelOnly) q.whereNull("parent_id"); + return q.orderBy("created_at", "asc").orderBy("id", "asc"); + } + + async update( + id: string, + patch: { body?: string; status?: CommentStatus; selectors?: unknown[]; resolverRef?: string }, + ): Promise { + const existing = await this.get(id); + if (!existing) return undefined; + + const now = new Date(); + const changes: Partial = { updated_at: now }; + if (patch.body !== undefined) { + changes.body = patch.body; + changes.body_html = await renderCommentBody(patch.body); + } + if (patch.selectors !== undefined) { + changes.selectors = JSON.stringify(patch.selectors); + } + if (patch.status !== undefined) { + changes.status = patch.status; + if (patch.status === "resolved") { + // Push the idempotency guard into SQL — concurrent resolves can't double-stamp. + // COALESCE leaves existing stamps intact; works on both better-sqlite3 and Postgres. + (changes as any).resolved_at = this.knex.raw("COALESCE(resolved_at, ?)", [now]); + (changes as any).resolved_by = this.knex.raw("COALESCE(resolved_by, ?)", [ + patch.resolverRef ?? null, + ]); + } else { + changes.resolved_at = null; + changes.resolved_by = null; + } + } + await this.knex(TABLE).where({ id }).update(changes); + return this.get(id); + } + + /** + * Conditional soft-delete: only applies to a currently-live row (`deleted_at IS + * NULL`). Returns the fresh row when it applied, or `undefined` when 0 rows + * matched (the row was already deleted — a lost race). Pass `executor` to run + * inside an open transaction. + */ + async softDelete(id: string, executor?: Knex.Transaction): Promise { + const db = executor ?? this.knex; + const now = new Date(); + const count = await db(TABLE) + .where({ id }) + .whereNull("deleted_at") + .update({ deleted_at: now, updated_at: now }); + if (count === 0) return undefined; + return this.get(id, { executor }); + } + + /** + * Conditional restore: only applies to a currently-deleted row (`deleted_at IS + * NOT NULL`). Returns the fresh row when it applied, or `undefined` when 0 rows + * matched (the row was already live — a lost race). Pass `executor` to run + * inside an open transaction. + */ + async restore(id: string, executor?: Knex.Transaction): Promise { + const db = executor ?? this.knex; + const count = await db(TABLE) + .where({ id }) + .whereNotNull("deleted_at") + .update({ deleted_at: null, updated_at: new Date() }); + if (count === 0) return undefined; + return this.get(id, { executor }); + } +} diff --git a/plugins/rw-backend/src/comments/author.test.ts b/plugins/rw-backend/src/comments/author.test.ts new file mode 100644 index 0000000..0518f1d --- /dev/null +++ b/plugins/rw-backend/src/comments/author.test.ts @@ -0,0 +1,61 @@ +import { mockCredentials, mockServices } from "@backstage/backend-test-utils"; +import { catalogServiceMock } from "@backstage/plugin-catalog-node/testUtils"; +import { resolveAuthor } from "./author"; + +const credentials = mockCredentials.user("user:default/alice"); + +describe("resolveAuthor", () => { + const userInfo = mockServices.userInfo.mock({ + getUserInfo: async () => ({ userEntityRef: "user:default/alice", ownershipEntityRefs: [] }), + }); + const auth = mockServices.auth(); + + it("returns authorRef + profile from the catalog entity", async () => { + const catalog = catalogServiceMock({ + entities: [ + { + apiVersion: "backstage.io/v1alpha1", + kind: "User", + metadata: { name: "alice", namespace: "default" }, + spec: { profile: { displayName: "Alice", picture: "http://a/x.png" } }, + }, + ], + }); + const out = await resolveAuthor({ userInfo, auth, catalog, credentials }); + expect(out.authorRef).toBe("user:default/alice"); + expect(out.authorProfile).toEqual({ displayName: "Alice", picture: "http://a/x.png" }); + }); + + it("returns authorRef + profile with picture only when displayName is absent", async () => { + const catalog = catalogServiceMock({ + entities: [ + { + apiVersion: "backstage.io/v1alpha1", + kind: "User", + metadata: { name: "alice", namespace: "default" }, + spec: { profile: { picture: "http://a/x.png" } }, + }, + ], + }); + const out = await resolveAuthor({ userInfo, auth, catalog, credentials }); + expect(out.authorRef).toBe("user:default/alice"); + expect(out.authorProfile).toEqual({ picture: "http://a/x.png" }); + }); + + it("returns authorRef with no profile when the catalog entity is absent", async () => { + const catalog = catalogServiceMock({ entities: [] }); + const out = await resolveAuthor({ userInfo, auth, catalog, credentials }); + expect(out.authorRef).toBe("user:default/alice"); + expect(out.authorProfile).toBeUndefined(); + }); + + it("returns authorRef with no profile when catalog.getEntityByRef throws", async () => { + const catalog = { + ...catalogServiceMock({ entities: [] }), + getEntityByRef: jest.fn().mockRejectedValue(new Error("catalog unavailable")), + } as any; + const out = await resolveAuthor({ userInfo, auth, catalog, credentials }); + expect(out.authorRef).toBe("user:default/alice"); + expect(out.authorProfile).toBeUndefined(); + }); +}); diff --git a/plugins/rw-backend/src/comments/author.ts b/plugins/rw-backend/src/comments/author.ts new file mode 100644 index 0000000..869bb96 --- /dev/null +++ b/plugins/rw-backend/src/comments/author.ts @@ -0,0 +1,37 @@ +import type { + AuthService, + BackstageCredentials, + UserInfoService, +} from "@backstage/backend-plugin-api"; +import type { CatalogService } from "@backstage/plugin-catalog-node"; +import { AuthorProfile } from "./types"; + +export async function resolveAuthor(deps: { + userInfo: UserInfoService; + auth: AuthService; + catalog: CatalogService; + credentials: BackstageCredentials; +}): Promise<{ authorRef: string; authorProfile?: AuthorProfile }> { + const { userInfo, auth, catalog, credentials } = deps; + const { userEntityRef } = await userInfo.getUserInfo(credentials); + + const serviceCreds = await auth.getOwnServiceCredentials(); + let entity: Awaited>; + try { + entity = await catalog.getEntityByRef(userEntityRef, { credentials: serviceCreds }); + } catch { + return { authorRef: userEntityRef }; + } + + const profile = entity?.spec?.profile as { displayName?: string; picture?: string } | undefined; + if (profile?.displayName || profile?.picture) { + return { + authorRef: userEntityRef, + authorProfile: { + ...(profile.displayName ? { displayName: profile.displayName } : {}), + ...(profile.picture ? { picture: profile.picture } : {}), + }, + }; + } + return { authorRef: userEntityRef }; +} diff --git a/plugins/rw-backend/src/comments/integration.test.ts b/plugins/rw-backend/src/comments/integration.test.ts new file mode 100644 index 0000000..374fdf3 --- /dev/null +++ b/plugins/rw-backend/src/comments/integration.test.ts @@ -0,0 +1,203 @@ +/** + * Integration test for the comments subsystem wired the same way plugin.ts wires it: + * the page router (createRouter) and the comments router (createCommentsRouter) mounted + * as sibling routers on one app, against a real SQLite database. + * + * This drives a lifecycle over HTTP: POST → GET thread → PATCH resolve → DELETE reply. + */ +import * as http from "http"; +import express from "express"; +import request from "supertest"; +import { mockServices, TestDatabases } from "@backstage/backend-test-utils"; +import { catalogServiceMock } from "@backstage/plugin-catalog-node/testUtils"; +import { MiddlewareFactory } from "@backstage/backend-defaults/rootHttpRouter"; +import { resolvePackagePath } from "@backstage/backend-plugin-api"; +import { createSite } from "@rwdocs/core"; +import { Hub } from "../hub"; +import { CommentStore } from "./CommentStore"; +import { createRouter } from "../router"; +import { createCommentsRouter } from "./router"; + +jest.mock("@rwdocs/core", () => ({ + createSite: jest.fn(), + renderCommentBody: jest.fn(async (md: string) => `

${md}

`), +})); + +const mockCreateSite = createSite as jest.MockedFunction; + +const ARCH = "component:default/arch"; +const DOC = "section:default/root#guide"; + +const databases = TestDatabases.create({ ids: ["SQLITE_3"] }); + +// All servers created by buildApp are registered here and torn down after each test. +const servers: http.Server[] = []; + +afterEach(async () => { + await Promise.all(servers.map((s) => new Promise((r) => s.close(() => r())))); + servers.length = 0; +}); + +async function buildApp(opts?: { entities?: unknown[] }) { + const knex = await databases.init("SQLITE_3"); + await knex.migrate.latest({ + directory: resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"), + }); + const store = new CommentStore(knex); + + const mockSite = { + getNavigation: jest.fn().mockResolvedValue([]), + renderPage: jest.fn().mockResolvedValue({ title: "Test", content: "

test

" }), + reload: jest.fn(), + }; + mockCreateSite.mockReturnValue(mockSite as any); + + const hub = new Hub({ projectDir: "/test/docs", entity: ARCH }); + + const catalog = catalogServiceMock({ + entities: (opts?.entities ?? [ + { + apiVersion: "backstage.io/v1alpha1", + kind: "Component", + metadata: { name: "arch", namespace: "default" }, + }, + { + apiVersion: "backstage.io/v1alpha1", + kind: "User", + metadata: { name: "mock", namespace: "default" }, + spec: { profile: { displayName: "Mock User" } }, + }, + ]) as any, + }); + + // Mirror plugin.ts: page router and comments router mounted as siblings. + const router = await createRouter({ + logger: mockServices.logger.mock(), + httpAuth: mockServices.httpAuth(), + hub, + }); + const commentsRouter = createCommentsRouter({ + store, + logger: mockServices.logger.mock(), + httpAuth: mockServices.httpAuth(), + auth: mockServices.auth(), + userInfo: mockServices.userInfo(), + permissions: mockServices.permissions(), + permissionsRegistry: mockServices.permissionsRegistry.mock(), + catalog, + commentsEnabled: true, + }); + + const app = express(); + app.use(router); + app.use(commentsRouter); + app.use((err: any, _req: express.Request, res: express.Response, _next: express.NextFunction) => { + const statusByName: Record = { + InputError: 400, + NotFoundError: 404, + NotAllowedError: 403, + ServiceUnavailableError: 503, + }; + const status = statusByName[err.name] ?? 500; + res.status(status).json({ error: { name: err.name, message: err.message } }); + }); + app.use( + MiddlewareFactory.create({ + logger: mockServices.logger.mock(), + config: mockServices.rootConfig(), + }).error(), + ); + + const server = http.createServer(app); + await new Promise((r) => server.listen(0, () => r())); + servers.push(server); + return { server, store, knex }; +} + +describe("comments integration (page + comments routers as siblings)", () => { + it("POST a comment, GET the thread, PATCH resolve, DELETE a reply", async () => { + const { server } = await buildApp(); + const agent = request(server); + + // POST a root comment + const postRes = await agent + .post("/comments") + .send({ siteRef: ARCH, documentId: DOC, body: "root comment", selectors: [] }); + expect(postRes.status).toBe(201); + const rootId = postRes.body.id; + expect(rootId).toBeDefined(); + expect(postRes.body.documentId).toBe(DOC); + expect(postRes.body.status).toBe("open"); + + // POST a reply + const replyRes = await agent + .post("/comments") + .send({ siteRef: ARCH, documentId: DOC, body: "reply", selectors: [], parentId: rootId }); + expect(replyRes.status).toBe(201); + const replyId = replyRes.body.id; + expect(replyRes.body.parentId).toBe(rootId); + + // GET thread: both comments visible + const getRes = await agent.get("/comments").query({ siteRef: ARCH, documentId: DOC }); + expect(getRes.status).toBe(200); + expect(getRes.body).toHaveLength(2); + const ids = getRes.body.map((c: any) => c.id); + expect(ids).toContain(rootId); + expect(ids).toContain(replyId); + + // PATCH root → resolved (collaborative; default mock user != author of root) + // The default mock user is user:default/mock. The root was created as user:default/mock (mockServices.userInfo default). + const patchRes = await agent.patch(`/comments/${rootId}`).send({ status: "resolved" }); + expect(patchRes.status).toBe(200); + expect(patchRes.body.status).toBe("resolved"); + + // DELETE the reply (author is user:default/mock, which matches the default caller) + const delRes = await agent.delete(`/comments/${replyId}`); + expect(delRes.status).toBe(200); + expect(delRes.body.deletedAt).toBeDefined(); + expect(delRes.body.documentId).toBe(DOC); + + // GET thread after delete: reply excluded (soft-deleted), root still present + const getAfterRes = await agent.get("/comments").query({ siteRef: ARCH, documentId: DOC }); + expect(getAfterRes.status).toBe(200); + expect(getAfterRes.body).toHaveLength(1); + expect(getAfterRes.body[0].id).toBe(rootId); + }); + + it("GET /comments/config returns enabled: true when mounted alongside the page router", async () => { + const { server } = await buildApp(); + const res = await request(server).get("/comments/config"); + expect(res.status).toBe(200); + expect(res.body).toEqual({ enabled: true }); + }); + + it("POST /comments requires a valid documentId with #", async () => { + const { server } = await buildApp(); + const res = await request(server) + .post("/comments") + .send({ siteRef: ARCH, documentId: "no-hash", body: "test", selectors: [] }); + expect(res.status).toBe(400); + }); + + it("GET /comments returns 404 when site entity not in catalog", async () => { + // Build with empty catalog; seed a comment via the app's own store so the + // entity-visibility guard is actually exercised over real data (not a separate DB). + const { server, store } = await buildApp({ entities: [] }); + await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "x", + selectors: [], + }); + + const res = await request(server).get("/comments").query({ siteRef: ARCH, documentId: DOC }); + expect(res.status).toBe(404); + }); + + it("GET /health still works on the same router", async () => { + const { server } = await buildApp(); + const res = await request(server).get("/health"); + expect(res.status).toBe(200); + expect(res.body).toEqual({ status: "ok" }); + }); +}); diff --git a/plugins/rw-backend/src/comments/logging.test.ts b/plugins/rw-backend/src/comments/logging.test.ts new file mode 100644 index 0000000..71df245 --- /dev/null +++ b/plugins/rw-backend/src/comments/logging.test.ts @@ -0,0 +1,45 @@ +import { mockServices } from "@backstage/backend-test-utils"; +import { logCommentOp } from "./logging"; + +describe("logCommentOp", () => { + it("logs a mutation at info with outcome ok", () => { + const logger = mockServices.logger.mock(); + logCommentOp(logger, { + kind: "mutation", + op: "create", + siteRef: "component:default/arch", + commentId: "c1", + }); + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining("create"), + expect.objectContaining({ op: "create", outcome: "ok", commentId: "c1" }), + ); + }); + + it("logs a denial at warn", () => { + const logger = mockServices.logger.mock(); + logCommentOp(logger, { + kind: "denied", + op: "edit", + permission: "rwComment.edit", + userEntityRef: "user:default/bob", + }); + expect(logger.warn).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ outcome: "denied", permission: "rwComment.edit" }), + ); + }); + + it("logs an error at error level with outcome:error", () => { + const logger = mockServices.logger.mock(); + logCommentOp(logger, { + kind: "error", + op: "create", + err: new Error("boom"), + }); + expect(logger.error).toHaveBeenCalledWith( + expect.stringContaining("create"), + expect.objectContaining({ outcome: "error", op: "create" }), + ); + }); +}); diff --git a/plugins/rw-backend/src/comments/logging.ts b/plugins/rw-backend/src/comments/logging.ts new file mode 100644 index 0000000..67906ac --- /dev/null +++ b/plugins/rw-backend/src/comments/logging.ts @@ -0,0 +1,67 @@ +import type { LoggerService } from "@backstage/backend-plugin-api"; + +/** + * `mutation` and `denied` are the only variants emitted today. `entity-not-visible`, + * `oversized-list`, and `error` are defined ahead of their emitters as intentional + * forward hooks — the switch in `logCommentOp` already handles them so adding an + * emitter later requires no structural change here. + */ +export type CommentLogEvent = + | { kind: "mutation"; op: string; siteRef: string; commentId: string; parentId?: string } + | { kind: "denied"; op: string; permission: string; userEntityRef: string } + | { kind: "entity-not-visible"; op: string; siteRef: string; userEntityRef: string } + | { kind: "oversized-list"; siteRef: string; documentId: string; count: number; bytes: number } + | { kind: "error"; op: string; err: unknown }; + +/** Single funnel for comment-op logging. PII (body/html/profile/selectors/tokens) is + * not representable in CommentLogEvent, so it cannot be logged here. */ +export function logCommentOp(logger: LoggerService, event: CommentLogEvent): void { + switch (event.kind) { + case "mutation": + logger.info(`comment op ${event.op}`, { + op: event.op, + siteRef: event.siteRef, + commentId: event.commentId, + ...(event.parentId ? { parentId: event.parentId } : {}), + outcome: "ok", + }); + return; + case "denied": + logger.warn(`comment op ${event.op} denied`, { + op: event.op, + permission: event.permission, + userEntityRef: event.userEntityRef, + outcome: "denied", + }); + return; + case "entity-not-visible": + logger.warn(`comment op ${event.op} entity not visible`, { + op: event.op, + siteRef: event.siteRef, + userEntityRef: event.userEntityRef, + outcome: "entity-not-visible", + }); + return; + case "oversized-list": + logger.warn("comment list oversized", { + op: "list", + siteRef: event.siteRef, + documentId: event.documentId, + count: event.count, + bytes: event.bytes, + }); + return; + case "error": + logger.error(`comment op ${event.op} failed`, { + op: event.op, + outcome: "error", + err: event.err instanceof Error ? event.err.message : String(event.err), + }); + return; + default: { + // Exhaustiveness check — TypeScript ensures this is unreachable. + const _: never = event; + void _; + } + } +} diff --git a/plugins/rw-backend/src/comments/mapping.test.ts b/plugins/rw-backend/src/comments/mapping.test.ts new file mode 100644 index 0000000..01bd312 --- /dev/null +++ b/plugins/rw-backend/src/comments/mapping.test.ts @@ -0,0 +1,69 @@ +import { toCommentResponse } from "./mapping"; +import type { CommentRow } from "./types"; + +function baseRow(over: Partial = {}): CommentRow { + return { + id: "id1", + site_ref: "component:default/arch", + document_id: "section:default/root#guide", + entity_ref: "component:default/arch", + parent_id: null, + author_ref: "user:default/alice", + author_profile: JSON.stringify({ displayName: "Alice", picture: "http://a/x.png" }), + body: "hi", + body_html: "

hi

", + selectors: JSON.stringify([{ type: "CSSSelector", value: "#x" }]), + status: "open", + created_at: new Date("2026-06-21T06:00:00.000Z"), + updated_at: new Date("2026-06-21T06:00:00.000Z"), + resolved_at: null, + resolved_by: null, + deleted_at: null, + ...over, + }; +} + +describe("toCommentResponse", () => { + it("returns documentId verbatim and composes author from profile", () => { + const r = toCommentResponse(baseRow(), "user:default/alice"); + expect(r.documentId).toBe("section:default/root#guide"); + expect(r.author).toEqual({ + id: "user:default/alice", + name: "Alice", + avatarUrl: "http://a/x.png", + }); + expect(r.bodyHtml).toBe("

hi

"); + expect(r.createdAt).toBe("2026-06-21T06:00:00.000Z"); + expect((r as { siteRef?: string }).siteRef).toBeUndefined(); + expect((r as { entityRef?: string }).entityRef).toBeUndefined(); + }); + + it("derives author.name from author_ref when author_profile is null (erased/guest)", () => { + const r = toCommentResponse( + baseRow({ author_profile: null, author_ref: "user:development/guest" }), + undefined, + ); + expect(r.author).toEqual({ id: "user:development/guest", name: "guest" }); + }); + + it("canResolve is true for a live top-level comment with no author check", () => { + const r = toCommentResponse(baseRow({ parent_id: null }), "user:default/somebody-else"); + expect(r.canResolve).toBe(true); + expect(r.canDelete).toBe(false); // top-level not deletable + }); + + it("canDelete/canRestore are author-gated and reply-only", () => { + const reply = baseRow({ parent_id: "top", author_ref: "user:default/alice" }); + const asAuthor = toCommentResponse(reply, "user:default/alice"); + expect(asAuthor.canDelete).toBe(true); + const asOther = toCommentResponse(reply, "user:default/bob"); + expect(asOther.canDelete).toBe(false); + + const deletedReply = baseRow({ + parent_id: "top", + deleted_at: new Date(), + author_ref: "user:default/alice", + }); + expect(toCommentResponse(deletedReply, "user:default/alice").canRestore).toBe(true); + }); +}); diff --git a/plugins/rw-backend/src/comments/mapping.ts b/plugins/rw-backend/src/comments/mapping.ts new file mode 100644 index 0000000..80b95f9 --- /dev/null +++ b/plugins/rw-backend/src/comments/mapping.ts @@ -0,0 +1,52 @@ +import { parseEntityRef } from "@backstage/catalog-model"; +import { CommentRow, AuthorProfile } from "./types"; +import { toIso } from "./timestamps"; + +export interface CommentResponse { + id: string; + documentId: string; + parentId?: string; + author: { id: string; name: string; avatarUrl?: string }; + body: string; + bodyHtml: string; + selectors: unknown[]; + status: "open" | "resolved"; + createdAt: string; + updatedAt: string; + deletedAt?: string; + canDelete: boolean; + canRestore: boolean; + canResolve: boolean; +} + +export function toCommentResponse(row: CommentRow, callerRef: string | undefined): CommentResponse { + const profile: AuthorProfile | null = row.author_profile ? JSON.parse(row.author_profile) : null; + const name = profile?.displayName ?? parseEntityRef(row.author_ref).name; + + const isReply = row.parent_id !== null; + const deleted = row.deleted_at !== null; + const isAuthor = callerRef !== undefined && callerRef === row.author_ref; + + return { + id: row.id, + documentId: row.document_id, + ...(row.parent_id !== null ? { parentId: row.parent_id } : {}), + author: { + id: row.author_ref, + name, + ...(profile?.picture ? { avatarUrl: profile.picture } : {}), + }, + body: row.body, + bodyHtml: row.body_html, + selectors: JSON.parse(row.selectors), + status: row.status, + createdAt: toIso(row.created_at)!, + updatedAt: toIso(row.updated_at)!, + ...(deleted ? { deletedAt: toIso(row.deleted_at) } : {}), + canDelete: isReply && !deleted && isAuthor, + canRestore: deleted && isAuthor, + // Resolve is collaborative — any authenticated user may resolve/reopen a + // thread, unlike canDelete/canRestore which are author-only. + canResolve: !isReply && !deleted, + }; +} diff --git a/plugins/rw-backend/src/comments/migrations.test.ts b/plugins/rw-backend/src/comments/migrations.test.ts new file mode 100644 index 0000000..bb5c940 --- /dev/null +++ b/plugins/rw-backend/src/comments/migrations.test.ts @@ -0,0 +1,44 @@ +import { TestDatabases } from "@backstage/backend-test-utils"; +import { resolvePackagePath } from "@backstage/backend-plugin-api"; + +describe("comments migration", () => { + const databases = TestDatabases.create({ ids: ["SQLITE_3"] }); + + it("creates the comments table with the expected columns", async () => { + const knex = await databases.init("SQLITE_3"); + const directory = resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"); + await knex.migrate.latest({ directory }); + + expect(await knex.schema.hasTable("comments")).toBe(true); + for (const col of [ + "id", + "site_ref", + "document_id", + "entity_ref", + "parent_id", + "author_ref", + "author_profile", + "body", + "body_html", + "selectors", + "status", + "created_at", + "updated_at", + "resolved_at", + "resolved_by", + "deleted_at", + ]) { + expect(await knex.schema.hasColumn("comments", col)).toBe(true); + } + }); + + it("drops the comments table on rollback", async () => { + const knex = await databases.init("SQLITE_3"); + const directory = resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"); + await knex.migrate.latest({ directory }); + expect(await knex.schema.hasTable("comments")).toBe(true); + + await knex.migrate.rollback({ directory }, true); + expect(await knex.schema.hasTable("comments")).toBe(false); + }); +}); diff --git a/plugins/rw-backend/src/comments/permissions-integration.test.ts b/plugins/rw-backend/src/comments/permissions-integration.test.ts new file mode 100644 index 0000000..1026c52 --- /dev/null +++ b/plugins/rw-backend/src/comments/permissions-integration.test.ts @@ -0,0 +1,160 @@ +/** + * S2 conditional-path test: verifies that a CONDITIONAL PolicyDecision returned by + * authorizeConditional is correctly evaluated against the in-memory comment via + * the isCommentAuthor rule. A non-author edit must be denied (403) and the author + * must be allowed (200). + */ +import * as http from "http"; +import express from "express"; +import request from "supertest"; +import { mockServices, TestDatabases } from "@backstage/backend-test-utils"; +import { catalogServiceMock } from "@backstage/plugin-catalog-node/testUtils"; +import { MiddlewareFactory } from "@backstage/backend-defaults/rootHttpRouter"; +import { resolvePackagePath } from "@backstage/backend-plugin-api"; +import { AuthorizeResult } from "@backstage/plugin-permission-common"; +import type { ConditionalPolicyDecision } from "@backstage/plugin-permission-common"; +import type { PermissionsService, PermissionsRegistryService } from "@backstage/backend-plugin-api"; +import type { PermissionRuleset } from "@backstage/plugin-permission-node"; +import { RESOURCE_TYPE_RW_COMMENT } from "@rwdocs/backstage-plugin-rw-common"; +import { CommentStore } from "./CommentStore"; +import { createCommentsRouter } from "./router"; +import { isCommentAuthor } from "./permissions"; +import type { CommentResponse } from "./mapping"; + +jest.mock("@rwdocs/core", () => ({ + renderCommentBody: jest.fn(async (md: string) => `

${md}

`), +})); + +const ARCH = "component:default/arch"; +const DOC = "section:default/root#guide"; + +const databases = TestDatabases.create({ ids: ["SQLITE_3"] }); + +// All servers created by buildConditionalApp are registered here and torn down after each test. +const servers: http.Server[] = []; + +afterEach(async () => { + await Promise.all(servers.map((s) => new Promise((r) => s.close(() => r())))); + servers.length = 0; +}); + +/** + * Build an express app whose permissions mock returns a CONDITIONAL decision + * wrapping the isCommentAuthor rule. The permissionsRegistry mock returns a + * ruleset that contains isCommentAuthor. + * + * mockServices.userInfo() returns user:default/mock as the caller. + * authorRef controls who owns the comment. + */ +async function buildConditionalApp(authorRef: string) { + const knex = await databases.init("SQLITE_3"); + await knex.migrate.latest({ + directory: resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"), + }); + const store = new CommentStore(knex); + + // The CONDITIONAL decision that carries an isCommentAuthor condition. + // The policy says: allow only if IS_COMMENT_AUTHOR holds for the caller. + // The caller's userRef is embedded in params; we use "user:default/mock" because + // that is what mockServices.userInfo() resolves to. + const conditionalDecision: ConditionalPolicyDecision = { + result: AuthorizeResult.CONDITIONAL, + pluginId: "rw", + resourceType: RESOURCE_TYPE_RW_COMMENT, + conditions: { + rule: isCommentAuthor.name, + resourceType: RESOURCE_TYPE_RW_COMMENT, + params: { userRef: "user:default/mock" }, + }, + }; + + // Permissions mock: authorizeConditional always returns our hand-built CONDITIONAL decision. + const permissionsMock: PermissionsService = { + authorize: jest.fn(async (requests) => requests.map(() => ({ result: AuthorizeResult.ALLOW }))), + authorizeConditional: jest.fn(async (requests) => requests.map(() => conditionalDecision)), + }; + + // Ruleset that exposes isCommentAuthor under its rule name. + const ruleset: PermissionRuleset = { + getRuleByName(name: string) { + if (name === isCommentAuthor.name) return isCommentAuthor as any; + throw new Error(`Unknown rule: ${name}`); + }, + }; + + // PermissionsRegistry mock: getPermissionRuleset returns our ruleset. + // Cast through unknown because the generic return type is parameterized by the resourceRef + // argument, but for testing purposes any resourceRef resolves to our fixed ruleset. + const getPermissionRuleset = jest.fn( + () => ruleset, + ) as unknown as PermissionsRegistryService["getPermissionRuleset"]; + const permissionsRegistryMock: Partial = { + getPermissionRuleset, + addResourceType: jest.fn(), + }; + + const app = express(); + app.use( + createCommentsRouter({ + store, + logger: mockServices.logger.mock(), + httpAuth: mockServices.httpAuth(), + auth: mockServices.auth(), + userInfo: mockServices.userInfo(), + permissions: permissionsMock, + permissionsRegistry: permissionsRegistryMock as PermissionsRegistryService, + catalog: catalogServiceMock({ + entities: [ + { + apiVersion: "backstage.io/v1alpha1", + kind: "Component", + metadata: { name: "arch", namespace: "default" }, + }, + ], + }) as any, + commentsEnabled: true, + }), + ); + app.use( + MiddlewareFactory.create({ + logger: mockServices.logger.mock(), + config: mockServices.rootConfig(), + }).error(), + ); + + // Pre-seed a comment with the given authorRef + const row = await store.create(ARCH, { + documentId: DOC, + authorRef, + body: "original", + selectors: [], + }); + + const server = http.createServer(app); + await new Promise((r) => server.listen(0, () => r())); + servers.push(server); + return { server, store, commentId: row.id }; +} + +describe("S2: conditional permission path — isCommentAuthor rule evaluation", () => { + it("denies edit (403) when the CONDITIONAL condition does not match (non-author caller)", async () => { + // Comment authored by user:default/other; caller is user:default/mock. + // isCommentAuthor checks comment.author.id === "user:default/mock" → false for this comment. + const { server, commentId } = await buildConditionalApp("user:default/other"); + + const res = await request(server).patch(`/comments/${commentId}`).send({ body: "tampered" }); + expect(res.status).toBe(403); + }); + + it("allows edit (200) when the CONDITIONAL condition matches (author caller)", async () => { + // Comment authored by user:default/mock; caller is user:default/mock. + // isCommentAuthor checks comment.author.id === "user:default/mock" → true. + const { server, commentId } = await buildConditionalApp("user:default/mock"); + + const res = await request(server) + .patch(`/comments/${commentId}`) + .send({ body: "updated body" }); + expect(res.status).toBe(200); + expect(res.body.body).toBe("updated body"); + }); +}); diff --git a/plugins/rw-backend/src/comments/permissions.test.ts b/plugins/rw-backend/src/comments/permissions.test.ts new file mode 100644 index 0000000..b715e4c --- /dev/null +++ b/plugins/rw-backend/src/comments/permissions.test.ts @@ -0,0 +1,19 @@ +import { isCommentAuthor, commentResourceRef } from "./permissions"; +import type { CommentResponse } from "./mapping"; + +const comment = { author: { id: "user:default/alice", name: "Alice" } } as CommentResponse; + +describe("isCommentAuthor rule", () => { + it("has the rw-comment resource ref", () => { + expect(commentResourceRef.resourceType).toBe("rw-comment"); + }); + + it("apply is true when the param userRef equals the author", () => { + expect(isCommentAuthor.apply(comment, { userRef: "user:default/alice" })).toBe(true); + expect(isCommentAuthor.apply(comment, { userRef: "user:default/bob" })).toBe(false); + }); + + it("toQuery returns an empty stub (never invoked in-memory)", () => { + expect(isCommentAuthor.toQuery({ userRef: "x" })).toEqual({}); + }); +}); diff --git a/plugins/rw-backend/src/comments/permissions.ts b/plugins/rw-backend/src/comments/permissions.ts new file mode 100644 index 0000000..b0ac0c2 --- /dev/null +++ b/plugins/rw-backend/src/comments/permissions.ts @@ -0,0 +1,31 @@ +import { z } from "zod/v3"; +import { + createPermissionResourceRef, + createPermissionRule, +} from "@backstage/plugin-permission-node"; +import { RESOURCE_TYPE_RW_COMMENT } from "@rwdocs/backstage-plugin-rw-common"; +import type { CommentResponse } from "./mapping"; + +/** + * Resource ref for RW comments. TQuery is {} because all authorization + * is applied in-memory (no DB query translation is needed). + */ +export const commentResourceRef = createPermissionResourceRef().with({ + pluginId: "rw", + resourceType: RESOURCE_TYPE_RW_COMMENT, +}); + +/** + * Permission rule that returns true when the caller's userRef matches the comment author. + * The caller ref is passed as a `userRef` param for testability. + */ +export const isCommentAuthor = createPermissionRule( + { + name: "IS_COMMENT_AUTHOR", + description: "Allow only the comment author", + resourceRef: commentResourceRef, + paramsSchema: z.object({ userRef: z.string() }), + apply: (comment, { userRef }) => comment.author.id === userRef, + toQuery: () => ({}), + }, +); diff --git a/plugins/rw-backend/src/comments/router.test.ts b/plugins/rw-backend/src/comments/router.test.ts new file mode 100644 index 0000000..7da11f1 --- /dev/null +++ b/plugins/rw-backend/src/comments/router.test.ts @@ -0,0 +1,887 @@ +import * as http from "http"; +import express from "express"; +import request from "supertest"; +import { mockServices, TestDatabases } from "@backstage/backend-test-utils"; +import { catalogServiceMock } from "@backstage/plugin-catalog-node/testUtils"; +import { MiddlewareFactory } from "@backstage/backend-defaults/rootHttpRouter"; +import { resolvePackagePath } from "@backstage/backend-plugin-api"; +import { AuthorizeResult } from "@backstage/plugin-permission-common"; +import type { PermissionsService } from "@backstage/backend-plugin-api"; +import { CommentStore } from "./CommentStore"; +import { createCommentsRouter } from "./router"; + +jest.mock("@rwdocs/core", () => ({ + renderCommentBody: jest.fn(async (md: string) => `

${md}

`), +})); + +const ARCH = "component:default/arch"; +const DOC = "section:default/root#guide"; + +const databases = TestDatabases.create({ ids: ["SQLITE_3"] }); + +// All servers created by buildApp / buildDenyApp are registered here and torn +// down after each test so no handles are left open. +const servers: http.Server[] = []; + +afterEach(async () => { + await Promise.all(servers.map((s) => new Promise((r) => s.close(() => r())))); + servers.length = 0; +}); + +/** + * Build an express app whose `permissions.authorize()` always returns DENY. + * Used to exercise the 403-on-DENY branches in GET /comments, POST /comments, + * and GET /comments/:id. `authorizeConditional` is left as allow-all because + * the DENY tests never reach conditional-evaluation paths. + */ +async function buildDenyApp() { + const knex = await databases.init("SQLITE_3"); + await knex.migrate.latest({ + directory: resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"), + }); + const store = new CommentStore(knex); + + const denyPermissions: PermissionsService = { + authorize: jest.fn(async (requests) => requests.map(() => ({ result: AuthorizeResult.DENY }))), + authorizeConditional: jest.fn(async (requests) => + requests.map(() => ({ result: AuthorizeResult.ALLOW })), + ), + }; + + const app = express(); + app.use( + createCommentsRouter({ + store, + logger: mockServices.logger.mock(), + httpAuth: mockServices.httpAuth(), + auth: mockServices.auth(), + userInfo: mockServices.userInfo(), + permissions: denyPermissions, + permissionsRegistry: mockServices.permissionsRegistry.mock(), + catalog: catalogServiceMock({ + entities: [ + { + apiVersion: "backstage.io/v1alpha1", + kind: "Component", + metadata: { name: "arch", namespace: "default" }, + }, + ], + }) as any, + commentsEnabled: true, + }), + ); + app.use( + MiddlewareFactory.create({ + logger: mockServices.logger.mock(), + config: mockServices.rootConfig(), + }).error(), + ); + + const server = http.createServer(app); + await new Promise((r) => server.listen(0, () => r())); + servers.push(server); + return { server, store }; +} + +async function buildApp(opts?: { entities?: unknown[] }) { + const knex = await databases.init("SQLITE_3"); + await knex.migrate.latest({ + directory: resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"), + }); + const store = new CommentStore(knex); + + const app = express(); + app.use( + createCommentsRouter({ + store, + logger: mockServices.logger.mock(), + httpAuth: mockServices.httpAuth(), + auth: mockServices.auth(), + userInfo: mockServices.userInfo(), + permissions: mockServices.permissions(), // allow-all by default + permissionsRegistry: mockServices.permissionsRegistry.mock(), + catalog: catalogServiceMock({ + entities: (opts?.entities ?? [ + { + apiVersion: "backstage.io/v1alpha1", + kind: "Component", + metadata: { name: "arch", namespace: "default" }, + }, + ]) as any, + }), + commentsEnabled: true, + }), + ); + app.use( + MiddlewareFactory.create({ + logger: mockServices.logger.mock(), + config: mockServices.rootConfig(), + }).error(), + ); + + const server = http.createServer(app); + await new Promise((r) => server.listen(0, () => r())); + servers.push(server); + return { server, store }; +} + +describe("comments router", () => { + it("POST /comments with missing body returns 400", async () => { + const { server } = await buildApp(); + const res = await request(server) + .post("/comments") + .send({ siteRef: ARCH, documentId: DOC, selectors: [] }); + expect(res.status).toBe(400); + }); + + it("POST /comments with missing documentId returns 400 (not 500)", async () => { + const { server } = await buildApp(); + const res = await request(server).post("/comments").send({ siteRef: ARCH, body: "hello" }); + expect(res.status).toBe(400); + }); + + it("GET /comments returns the full thread for a page", async () => { + const { server, store } = await buildApp(); + await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "1", + selectors: [], + }); + const res = await request(server).get("/comments").query({ siteRef: ARCH, documentId: DOC }); + expect(res.status).toBe(200); + expect(res.body).toHaveLength(1); + expect(res.body[0].documentId).toBe(DOC); + expect(res.body[0].canResolve).toBe(true); + }); + + it("rejects a documentId with no # as 400", async () => { + const { server } = await buildApp(); + const res = await request(server) + .get("/comments") + .query({ siteRef: ARCH, documentId: "no-hash" }); + expect(res.status).toBe(400); + }); + + it("rejects a documentId with leading # as 400 on GET", async () => { + const { server } = await buildApp(); + const res = await request(server).get("/comments").query({ siteRef: ARCH, documentId: "#foo" }); + expect(res.status).toBe(400); + }); + + it("rejects a documentId with leading # as 400 on POST", async () => { + const { server } = await buildApp(); + const res = await request(server) + .post("/comments") + .send({ siteRef: ARCH, documentId: "#foo", body: "test", selectors: [] }); + expect(res.status).toBe(400); + }); + + it("404 when the site entity is not visible to the caller", async () => { + const { server, store } = await buildApp({ entities: [] }); // catalog returns undefined + await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "1", + selectors: [], + }); + const res = await request(server).get("/comments").query({ siteRef: ARCH, documentId: DOC }); + expect(res.status).toBe(404); + }); + + it("POST /comments stamps the author from identity and ignores client author", async () => { + const { server } = await buildApp({ + entities: [ + { + apiVersion: "backstage.io/v1alpha1", + kind: "Component", + metadata: { name: "arch", namespace: "default" }, + }, + { + apiVersion: "backstage.io/v1alpha1", + kind: "User", + metadata: { name: "mock", namespace: "default" }, + spec: { profile: { displayName: "Mock User" } }, + }, + ], + }); + const res = await request(server) + .post("/comments") + .send({ + siteRef: ARCH, + documentId: DOC, + body: "hi", + selectors: [], + author: { id: "user:default/EVIL", name: "evil" }, + }); + expect(res.status).toBe(201); + expect(res.body.author.id).not.toBe("user:default/EVIL"); + expect(res.body.documentId).toBe(DOC); + }); + + it("GET /comments/:id returns a comment by id", async () => { + const { server, store } = await buildApp(); + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "hello", + selectors: [], + }); + const res = await request(server).get(`/comments/${row.id}`); + expect(res.status).toBe(200); + expect(res.body.id).toBe(row.id); + expect(res.body.documentId).toBe(DOC); + }); + + it("GET /comments/:id returns 404 for missing id", async () => { + const { server } = await buildApp(); + const res = await request(server).get("/comments/nonexistent-id"); + expect(res.status).toBe(404); + }); + + it("GET /comments/:id returns 404 when comment exists but site entity is not visible", async () => { + // Build the app with an empty catalog (entity not visible), seed a comment + // directly into that same app's store, then verify GET /comments/:id → 404. + const { server, store } = await buildApp({ entities: [] }); + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "hello", + selectors: [], + }); + const res = await request(server).get(`/comments/${row.id}`); + expect(res.status).toBe(404); + }); + + it("GET /comments/:id returns 404 for soft-deleted comment", async () => { + const { server, store } = await buildApp(); + const parent = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "parent", + selectors: [], + }); + const reply = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "reply", + selectors: [], + parentId: parent.id, + }); + await store.softDelete(reply.id); + const res = await request(server).get(`/comments/${reply.id}`); + expect(res.status).toBe(404); + }); + + it("PATCH /comments/:id resolve succeeds for non-author (collaborative)", async () => { + const { server, store } = await buildApp(); + // comment authored by user:default/a; caller is user:default/mock (default mock user) + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "thread", + selectors: [], + }); + const res = await request(server).patch(`/comments/${row.id}`).send({ status: "resolved" }); + expect(res.status).toBe(200); + expect(res.body.status).toBe("resolved"); + }); + + it("PATCH /comments/:id edit by non-author returns 403 (author floor) via MiddlewareFactory", async () => { + const { server, store } = await buildApp(); + // comment authored by user:default/a; caller is user:default/mock + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "original", + selectors: [], + }); + const res = await request(server).patch(`/comments/${row.id}`).send({ body: "tampered" }); + // 403 enforced by author floor regardless of allow-all permissions + expect(res.status).toBe(403); + }); + + it("DELETE /comments/:id by non-author returns 403 (author floor)", async () => { + const { server, store } = await buildApp(); + const parent = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "parent", + selectors: [], + }); + const reply = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "reply", + selectors: [], + parentId: parent.id, + }); + const res = await request(server).delete(`/comments/${reply.id}`); + expect(res.status).toBe(403); + }); + + it("DELETE /comments/:id returns the soft-deleted row with correct documentId", async () => { + const { server, store } = await buildApp(); + // create as mock user (the default caller) + const parent = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "parent", + selectors: [], + }); + const reply = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "reply", + selectors: [], + parentId: parent.id, + }); + const res = await request(server).delete(`/comments/${reply.id}`); + expect(res.status).toBe(200); + expect(res.body.documentId).toBe(DOC); + expect(res.body.deletedAt).toBeDefined(); + }); + + it("DELETE /comments/:id a second time on an already-deleted reply returns 404", async () => { + const { server, store } = await buildApp(); + const parent = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "parent", + selectors: [], + }); + const reply = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "reply", + selectors: [], + parentId: parent.id, + }); + const first = await request(server).delete(`/comments/${reply.id}`); + expect(first.status).toBe(200); + const second = await request(server).delete(`/comments/${reply.id}`); + expect(second.status).toBe(404); + }); + + it("PATCH /comments/:id edit on soft-deleted row returns 400", async () => { + const { server, store } = await buildApp(); + const parent = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "parent", + selectors: [], + }); + const reply = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "reply", + selectors: [], + parentId: parent.id, + }); + await store.softDelete(reply.id); + const res = await request(server).patch(`/comments/${reply.id}`).send({ body: "edited" }); + expect(res.status).toBe(400); + }); + + // ── Task 4 regression tests ────────────────────────────────────────────── + + it("PATCH /comments/:id with empty body {} returns 400 (no editable fields)", async () => { + const { server, store } = await buildApp(); + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "original", + selectors: [], + }); + const before = await store.get(row.id); + const res = await request(server).patch(`/comments/${row.id}`).send({}); + expect(res.status).toBe(400); + // Store must NOT have been mutated (updated_at unchanged) + const after = await store.get(row.id); + expect(after!.updated_at).toBe(before!.updated_at); + }); + + it("PATCH /comments/:id with only unknown fields returns 400 (no editable fields)", async () => { + const { server, store } = await buildApp(); + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "original", + selectors: [], + }); + const before = await store.get(row.id); + const res = await request(server) + .patch(`/comments/${row.id}`) + .send({ unknownField: "surprise" }); + expect(res.status).toBe(400); + const after = await store.get(row.id); + expect(after!.updated_at).toBe(before!.updated_at); + }); + + it("PATCH /comments/:id status:resolved on a reply returns 400 (cannot resolve reply)", async () => { + const { server, store } = await buildApp(); + const parent = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "parent", + selectors: [], + }); + const reply = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "reply", + selectors: [], + parentId: parent.id, + }); + const res = await request(server).patch(`/comments/${reply.id}`).send({ status: "resolved" }); + expect(res.status).toBe(400); + // Reply must remain unresolved + const after = await store.get(reply.id); + expect(after!.status).toBe("open"); + }); + + it("POST /comments with non-string parentId returns 400 (not 500)", async () => { + const { server } = await buildApp(); + const res = await request(server) + .post("/comments") + .send({ + siteRef: ARCH, + documentId: DOC, + body: "hi", + selectors: [], + parentId: { evil: true }, + }); + expect(res.status).toBe(400); + }); + + // ── End Task 4 regression tests ────────────────────────────────────────── + + // ── Task 5: error mapping & author-floor via thrown error ──────────────── + + it("assertSiteVisible: catalog transport failure → 503", async () => { + const knex = await databases.init("SQLITE_3"); + await knex.migrate.latest({ + directory: resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"), + }); + const store = new CommentStore(knex); + + // Mock catalog whose getEntityByRef rejects (transport/availability failure) + const failingCatalog = catalogServiceMock.mock({ + getEntityByRef: jest.fn().mockRejectedValue(new Error("ECONNREFUSED")), + }); + + const app = express(); + app.use( + createCommentsRouter({ + store, + logger: mockServices.logger.mock(), + httpAuth: mockServices.httpAuth(), + auth: mockServices.auth(), + userInfo: mockServices.userInfo(), + permissions: mockServices.permissions(), + permissionsRegistry: mockServices.permissionsRegistry.mock(), + catalog: failingCatalog, + commentsEnabled: true, + }), + ); + app.use( + MiddlewareFactory.create({ + logger: mockServices.logger.mock(), + config: mockServices.rootConfig(), + }).error(), + ); + + const server = http.createServer(app); + await new Promise((r) => server.listen(0, () => r())); + servers.push(server); + + // Seed a comment so we reach assertSiteVisible + await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "x", + selectors: [], + }); + const res = await request(server).get("/comments").query({ siteRef: ARCH, documentId: DOC }); + expect(res.status).toBe(503); + }); + + it("assertSiteVisible: missing entity (returns undefined) → 404, not 503", async () => { + // entities: [] causes catalogServiceMock to return undefined from getEntityByRef + const { server, store } = await buildApp({ entities: [] }); + await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "x", + selectors: [], + }); + const res = await request(server).get("/comments").query({ siteRef: ARCH, documentId: DOC }); + expect(res.status).toBe(404); + }); + + it("assertAuthorFloor: edit by non-author → 403, store NOT mutated, denied log emitted", async () => { + const { server, store } = await buildApp(); + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", // different from mock caller user:default/mock + body: "original", + selectors: [], + }); + const before = await store.get(row.id); + const res = await request(server).patch(`/comments/${row.id}`).send({ body: "tampered" }); + expect(res.status).toBe(403); + // Store must NOT have been mutated + const after = await store.get(row.id); + expect(after!.body).toBe("original"); + expect(after!.updated_at).toBe(before!.updated_at); + }); + + it("assertAuthorFloor: delete by non-author → 403, store NOT mutated, denied log emitted", async () => { + const { server, store } = await buildApp(); + const parent = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "parent", + selectors: [], + }); + const reply = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "reply", + selectors: [], + parentId: parent.id, + }); + const res = await request(server).delete(`/comments/${reply.id}`); + expect(res.status).toBe(403); + // Reply must still exist, not soft-deleted + const after = await store.get(reply.id); + expect(after!.deleted_at).toBeNull(); + }); + + it("assertAuthorFloor: restore by non-author → 403, store NOT mutated", async () => { + const { server, store } = await buildApp(); + const parent = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "parent", + selectors: [], + }); + const reply = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "reply", + selectors: [], + parentId: parent.id, + }); + await store.softDelete(reply.id); + const res = await request(server).patch(`/comments/${reply.id}`).send({ status: "open" }); + expect(res.status).toBe(403); + // Reply must remain soft-deleted + const after = await store.get(reply.id); + expect(after!.deleted_at).not.toBeNull(); + }); + + // ── End Task 5 tests ────────────────────────────────────────────────────── + + // ── Task 6: permission DENY paths ──────────────────────────────────────── + + it("GET /comments returns 403 when authorize() returns DENY for rwCommentReadPermission", async () => { + // Locks in the branch at router.ts ~:131 — `if (decision[0].result !== AuthorizeResult.ALLOW)` + // before the store is queried. Removing that check would let the request through (200). + const { server, store } = await buildDenyApp(); + await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "should not be visible", + selectors: [], + }); + const res = await request(server).get("/comments").query({ siteRef: ARCH, documentId: DOC }); + expect(res.status).toBe(403); + }); + + it("POST /comments returns 403 when authorize() returns DENY for rwCommentCreatePermission", async () => { + // Locks in the branch at router.ts ~:154 — `if (decision[0].result !== AuthorizeResult.ALLOW)` + // before the comment is created. Removing that check would allow the comment to be stored (201). + const { server } = await buildDenyApp(); + const res = await request(server) + .post("/comments") + .send({ siteRef: ARCH, documentId: DOC, body: "denied", selectors: [] }); + expect(res.status).toBe(403); + }); + + it("GET /comments/:id returns 403 when authorize() returns DENY for rwCommentReadPermission", async () => { + // Locks in the branch at router.ts ~:196 — `if (decision[0].result !== AuthorizeResult.ALLOW)` + // after the row is fetched but before it is returned. + // Note: the deny app still has a catalog that knows about the entity, so assertSiteVisible + // would pass — the permission check fires before assertSiteVisible. + const { server, store } = await buildDenyApp(); + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/a", + body: "private", + selectors: [], + }); + const res = await request(server).get(`/comments/${row.id}`); + expect(res.status).toBe(403); + }); + + // ── End Task 6: permission DENY paths ──────────────────────────────────── + + // ── Task 6: POST /comments parentId validation (5 reject branches) ─────── + + it("POST /comments with non-existent parentId returns 400", async () => { + // Locks in `!parent` branch of the parentId guard at router.ts ~:167. + // Removing the guard (or the !parent check) would allow the insert to proceed + // with a dangling foreign-key-like value. + const { server } = await buildApp(); + const res = await request(server).post("/comments").send({ + siteRef: ARCH, + documentId: DOC, + body: "reply", + selectors: [], + parentId: "00000000-0000-0000-0000-000000000000", + }); + expect(res.status).toBe(400); + expect(res.body.error?.message).toMatch(/Invalid parentId/); + }); + + it("POST /comments with parentId belonging to a different siteRef returns 400", async () => { + // Locks in `parent.site_ref !== siteRef` branch at router.ts ~:169. + // The parent exists but was created under a different site entity. + const { server, store } = await buildApp(); + const OTHER_SITE = "component:default/other"; + const parent = await store.create(OTHER_SITE, { + documentId: DOC, + authorRef: "user:default/mock", + body: "root in other site", + selectors: [], + }); + const res = await request(server).post("/comments").send({ + siteRef: ARCH, + documentId: DOC, + body: "cross-site reply", + selectors: [], + parentId: parent.id, + }); + expect(res.status).toBe(400); + expect(res.body.error?.message).toMatch(/Invalid parentId/); + }); + + it("POST /comments with parentId belonging to a different documentId returns 400", async () => { + // Locks in `parent.document_id !== documentId` branch at router.ts ~:170. + // The parent is in the same site but a different document. + const { server, store } = await buildApp(); + const OTHER_DOC = "section:default/root#other"; + const parent = await store.create(ARCH, { + documentId: OTHER_DOC, + authorRef: "user:default/mock", + body: "root in other doc", + selectors: [], + }); + const res = await request(server).post("/comments").send({ + siteRef: ARCH, + documentId: DOC, + body: "cross-doc reply", + selectors: [], + parentId: parent.id, + }); + expect(res.status).toBe(400); + expect(res.body.error?.message).toMatch(/Invalid parentId/); + }); + + it("POST /comments with parentId that is itself a reply (non-root) returns 400", async () => { + // Locks in `parent.parent_id !== null` branch at router.ts ~:171. + // Nested replies are disallowed — only root comments may be parents. + const { server, store } = await buildApp(); + const root = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "root comment", + selectors: [], + }); + const existingReply = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "first reply", + selectors: [], + parentId: root.id, + }); + const res = await request(server).post("/comments").send({ + siteRef: ARCH, + documentId: DOC, + body: "nested reply (invalid)", + selectors: [], + parentId: existingReply.id, + }); + expect(res.status).toBe(400); + expect(res.body.error?.message).toMatch(/Invalid parentId/); + }); + + it("POST /comments with a soft-deleted parentId returns 400", async () => { + // Locks in `parent.deleted_at !== null` branch at router.ts ~:172. + // Replying to a deleted comment must be rejected. + const { server, store } = await buildApp(); + const root = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "root that will be deleted", + selectors: [], + }); + await store.softDelete(root.id); + const res = await request(server).post("/comments").send({ + siteRef: ARCH, + documentId: DOC, + body: "reply to deleted parent", + selectors: [], + parentId: root.id, + }); + expect(res.status).toBe(400); + expect(res.body.error?.message).toMatch(/Invalid parentId/); + }); + + // ── End Task 6: parentId validation tests ──────────────────────────────── + + // ── Task 1: request-validation hardening ───────────────────────────────── + + it("POST /comments with parentId: '' (empty string) returns 400 and creates no row", async () => { + const { server, store } = await buildApp(); + const before = await store.list(ARCH, { documentId: DOC }); + const res = await request(server).post("/comments").send({ + siteRef: ARCH, + documentId: DOC, + body: "hi", + selectors: [], + parentId: "", + }); + expect(res.status).toBe(400); + const after = await store.list(ARCH, { documentId: DOC }); + expect(after.length).toBe(before.length); + }); + + it("GET /comments with repeated siteRef param (array) returns 400", async () => { + const { server } = await buildApp(); + // supertest encodes ?siteRef=a&siteRef=b which Express parses as string[] + const res = await request(server) + .get("/comments") + .query({ siteRef: [ARCH, ARCH], documentId: DOC }); + expect(res.status).toBe(400); + }); + + it("GET /comments with malformed siteRef returns 400, not 503", async () => { + const { server } = await buildApp(); + const res = await request(server) + .get("/comments") + .query({ siteRef: "not a ref", documentId: DOC }); + expect(res.status).toBe(400); + }); + + it("POST /comments with malformed siteRef returns 400, not 503", async () => { + const { server } = await buildApp(); + const res = await request(server).post("/comments").send({ + siteRef: "not a ref", + documentId: DOC, + body: "hello", + selectors: [], + }); + expect(res.status).toBe(400); + }); + + it("POST /comments with body just over 16 KiB returns 400", async () => { + const { server } = await buildApp(); + const oversizedBody = "x".repeat(16 * 1024 + 1); + const res = await request(server).post("/comments").send({ + siteRef: ARCH, + documentId: DOC, + body: oversizedBody, + selectors: [], + }); + expect(res.status).toBe(400); + }); + + it("POST /comments with body exactly at 16 KiB succeeds", async () => { + const { server } = await buildApp(); + const exactBody = "x".repeat(16 * 1024); + const res = await request(server).post("/comments").send({ + siteRef: ARCH, + documentId: DOC, + body: exactBody, + selectors: [], + }); + expect(res.status).toBe(201); + }); + + it("PATCH /comments/:id with body just over 16 KiB returns 400", async () => { + const { server, store } = await buildApp(); + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "original", + selectors: [], + }); + const oversizedBody = "x".repeat(16 * 1024 + 1); + const res = await request(server).patch(`/comments/${row.id}`).send({ body: oversizedBody }); + expect(res.status).toBe(400); + }); + + it("PATCH /comments/:id with body exactly at 16 KiB succeeds", async () => { + const { server, store } = await buildApp(); + const row = await store.create(ARCH, { + documentId: DOC, + authorRef: "user:default/mock", + body: "original", + selectors: [], + }); + const exactBody = "x".repeat(16 * 1024); + const res = await request(server).patch(`/comments/${row.id}`).send({ body: exactBody }); + expect(res.status).toBe(200); + }); + + // ── End Task 1 tests ────────────────────────────────────────────────────── + + it("GET /comments/config returns enabled: true when comments are on", async () => { + const { server } = await buildApp(); + const res = await request(server).get("/comments/config"); + expect(res.status).toBe(200); + expect(res.body).toEqual({ enabled: true }); + }); + + it("disabled app returns 404 on /comments and enabled:false on /comments/config", async () => { + const knex = await databases.init("SQLITE_3"); + await knex.migrate.latest({ + directory: resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"), + }); + const store = new CommentStore(knex); + const disabledApp = express(); + disabledApp.use( + createCommentsRouter({ + store, + logger: mockServices.logger.mock(), + httpAuth: mockServices.httpAuth(), + auth: mockServices.auth(), + userInfo: mockServices.userInfo(), + permissions: mockServices.permissions(), + permissionsRegistry: mockServices.permissionsRegistry.mock(), + catalog: catalogServiceMock() as any, + commentsEnabled: false, + }), + ); + disabledApp.use( + MiddlewareFactory.create({ + logger: mockServices.logger.mock(), + config: mockServices.rootConfig(), + }).error(), + ); + + // Use a shared server so both requests reuse the same TCP socket — avoids + // ephemeral-port exhaustion under parallel jest workers. + const server = http.createServer(disabledApp); + await new Promise((r) => server.listen(0, () => r())); + servers.push(server); + const agent = request(server); + const listRes = await agent.get("/comments").query({ siteRef: ARCH, documentId: DOC }); + expect(listRes.status).toBe(404); + const configRes = await agent.get("/comments/config"); + expect(configRes.status).toBe(200); + expect(configRes.body).toEqual({ enabled: false }); + }); +}); diff --git a/plugins/rw-backend/src/comments/router.ts b/plugins/rw-backend/src/comments/router.ts new file mode 100644 index 0000000..9391c4a --- /dev/null +++ b/plugins/rw-backend/src/comments/router.ts @@ -0,0 +1,404 @@ +import express from "express"; +import PromiseRouter from "express-promise-router"; +import type { Router } from "express"; +import { + InputError, + NotAllowedError, + NotFoundError, + ServiceUnavailableError, +} from "@backstage/errors"; +import { parseEntityRef } from "@backstage/catalog-model"; +import { + AuthService, + BackstageCredentials, + HttpAuthService, + LoggerService, + PermissionsService, + PermissionsRegistryService, + UserInfoService, +} from "@backstage/backend-plugin-api"; +import type { CatalogService } from "@backstage/plugin-catalog-node"; +import { AuthorizeResult } from "@backstage/plugin-permission-common"; +import type { ResourcePermission } from "@backstage/plugin-permission-common"; +import { createConditionAuthorizer } from "@backstage/plugin-permission-node"; +import { + rwCommentReadPermission, + rwCommentCreatePermission, + rwCommentEditPermission, + rwCommentResolvePermission, + rwCommentDeletePermission, +} from "@rwdocs/backstage-plugin-rw-common"; +import { CommentStore } from "./CommentStore"; +import { toCommentResponse } from "./mapping"; +import type { CommentResponse } from "./mapping"; +import { resolveAuthor } from "./author"; +import { logCommentOp } from "./logging"; +import { commentResourceRef } from "./permissions"; + +const MAX_BODY_BYTES = 16 * 1024; + +/** + * Validates that `siteRef` is a well-formed Backstage entity ref string. + * Throws InputError(400) on any parse failure so callers receive a 400, not 503. + */ +function validateSiteRef(siteRef: unknown): asserts siteRef is string { + if (typeof siteRef !== "string" || !siteRef) throw new InputError("siteRef is required"); + try { + parseEntityRef(siteRef); + } catch { + throw new InputError("Invalid siteRef"); + } +} + +export interface CommentsRouterDeps { + store: CommentStore; + logger: LoggerService; + httpAuth: HttpAuthService; + auth: AuthService; + userInfo: UserInfoService; + permissions: PermissionsService; + permissionsRegistry: PermissionsRegistryService; + catalog: CatalogService; + commentsEnabled: boolean; +} + +/** Split a viewer documentId; throws InputError(400) on a non-string, missing or leading '#'. */ +export function parseDocumentId(documentId: unknown): { sectionRef: string; subpath: string } { + if (typeof documentId !== "string") throw new InputError("documentId must be a string"); + const i = documentId.indexOf("#"); + if (i <= 0) throw new InputError(`Malformed documentId: ${documentId}`); + return { sectionRef: documentId.slice(0, i), subpath: documentId.slice(i + 1) }; +} + +export function createCommentsRouter(deps: CommentsRouterDeps): Router { + const router = PromiseRouter(); + const { store, logger, httpAuth, permissions, catalog } = deps; + + /** + * Framework resource-permission check only. The caller must ALSO call + * `assertAuthorFloor` where author ownership is required (edit, delete, + * restore) — this function does not enforce that floor. + * + * Calls authorizeConditional and evaluates the returned PolicyDecision in-memory: + * - ALLOW → true + * - DENY → false + * - CONDITIONAL → evaluates the conditions tree against `comment` using the registered ruleset + */ + async function checkResourcePermission( + permission: ResourcePermission, + credentials: BackstageCredentials, + comment: CommentResponse, + ): Promise { + const [decision] = await permissions.authorizeConditional([{ permission }], { credentials }); + if (decision.result === AuthorizeResult.ALLOW) return true; + if (decision.result === AuthorizeResult.DENY) return false; + // CONDITIONAL: evaluate conditions tree against the in-memory comment + const ruleset = deps.permissionsRegistry.getPermissionRuleset(commentResourceRef); + return createConditionAuthorizer(ruleset)(decision, comment); + } + + router.use(express.json({ limit: "64kb" })); + + if (!deps.commentsEnabled) { + router.get("/comments/config", (_req, res) => res.json({ enabled: false })); + router.all("/comments", (_req, res) => res.status(404).end()); + router.all("/comments/*", (_req, res) => res.status(404).end()); + return router; + } + + router.get("/comments/config", (_req, res) => res.json({ enabled: true })); + + /** + * Read authorization is intentionally governed by the host `siteRef`'s read scope. + * The host site entity governs all comment content it hosts — a caller who can read + * the site may read its comments, regardless of which section (entity_ref) a comment + * belongs to. The stored `entity_ref` is for future cross-section / entity-scoped + * querying (see `ListFilter`), not a security boundary. This is by design. + */ + // Entity-read scope: caller must be able to read the host site entity. + async function assertSiteVisible(req: any, siteRef: string): Promise { + const credentials = await httpAuth.credentials(req); + let entity: Awaited>; + try { + entity = await catalog.getEntityByRef(siteRef, { credentials }); + } catch { + throw new ServiceUnavailableError("Catalog unavailable"); + } + if (!entity) { + throw new NotFoundError("Site not found"); + } + } + + async function callerUserRef(req: any): Promise { + const credentials = await httpAuth.credentials(req, { allow: ["user"] }); + const { userEntityRef } = await deps.userInfo.getUserInfo(credentials); + return userEntityRef; + } + + function assertAuthorFloor(userRef: string, row: { author_ref: string }): void { + if (userRef !== row.author_ref) { + logCommentOp(logger, { + kind: "denied", + op: "mutate", + permission: "author-floor", + userEntityRef: userRef, + }); + throw new NotAllowedError("Only the author may perform this operation"); + } + } + + router.get("/comments", async (req, res) => { + if (typeof req.query.siteRef !== "string") throw new InputError("siteRef is required"); + const siteRef = req.query.siteRef; + validateSiteRef(siteRef); + if (typeof req.query.documentId !== "string") throw new InputError("documentId is required"); + const documentId = req.query.documentId; + parseDocumentId(documentId); // 400 guard + const credentials = await httpAuth.credentials(req); + const decision = await permissions.authorize([{ permission: rwCommentReadPermission }], { + credentials, + }); + if (decision[0].result !== AuthorizeResult.ALLOW) { + res.status(403).end(); + return; + } + await assertSiteVisible(req, siteRef); + const rows = await store.list(siteRef, { documentId }); + const callerRef = await callerUserRef(req).catch(() => undefined); + res.json(rows.map((r) => toCommentResponse(r, callerRef))); + }); + + router.post("/comments", async (req, res) => { + const { siteRef, documentId, parentId, body, selectors } = req.body ?? {}; + validateSiteRef(siteRef); + parseDocumentId(documentId); // 400 guard + if (!body || typeof body !== "string") throw new InputError("body must be a non-empty string"); + if (Buffer.byteLength(body, "utf8") > MAX_BODY_BYTES) + throw new InputError("body exceeds maximum length"); + if (selectors !== undefined && !Array.isArray(selectors)) + throw new InputError("selectors must be an array"); + if (parentId !== undefined && (typeof parentId !== "string" || parentId.trim() === "")) + throw new InputError("parentId must be a non-empty string"); + const credentials = await httpAuth.credentials(req, { allow: ["user"] }); + const decision = await permissions.authorize([{ permission: rwCommentCreatePermission }], { + credentials, + }); + if (decision[0].result !== AuthorizeResult.ALLOW) { + res.status(403).end(); + return; + } + + const { authorRef, authorProfile } = await resolveAuthor({ + userInfo: deps.userInfo, + auth: deps.auth, + catalog, + credentials, + }); + if (parentId) { + const parent = await store.get(parentId); + if ( + !parent || + parent.site_ref !== siteRef || + parent.document_id !== documentId || + parent.parent_id !== null || + parent.deleted_at !== null + ) { + throw new InputError("Invalid parentId"); + } + } + const row = await store.create(siteRef, { + documentId, + parentId, + authorRef, + authorProfile, + body, + selectors: selectors ?? [], + }); + logCommentOp(logger, { kind: "mutation", op: "create", siteRef, commentId: row.id, parentId }); + res.status(201).json(toCommentResponse(row, authorRef)); + }); + + router.get("/comments/:id", async (req, res) => { + const row = await store.get(req.params.id); + if (!row || row.deleted_at !== null) throw new NotFoundError("Comment not found"); + const credentials = await httpAuth.credentials(req); + const decision = await permissions.authorize([{ permission: rwCommentReadPermission }], { + credentials, + }); + if (decision[0].result !== AuthorizeResult.ALLOW) { + res.status(403).end(); + return; + } + await assertSiteVisible(req, row.site_ref); + const callerRef = await callerUserRef(req).catch(() => undefined); + res.json(toCommentResponse(row, callerRef)); + }); + + router.patch("/comments/:id", async (req, res) => { + const row = await store.get(req.params.id); + if (!row) throw new NotFoundError("Comment not found"); + const { body, status, selectors } = req.body ?? {}; + if (status !== undefined && status !== "open" && status !== "resolved") { + throw new InputError("Invalid status"); + } + if (body !== undefined && (typeof body !== "string" || !body)) + throw new InputError("body must be a non-empty string"); + if (body !== undefined && Buffer.byteLength(body, "utf8") > MAX_BODY_BYTES) + throw new InputError("body exceeds maximum length"); + if (selectors !== undefined && !Array.isArray(selectors)) + throw new InputError("selectors must be an array"); + if (body === undefined && status === undefined && selectors === undefined) + throw new InputError("No editable fields provided"); + const editing = body !== undefined || selectors !== undefined; + const userRef = await callerUserRef(req); + const credentials = await httpAuth.credentials(req, { allow: ["user"] }); + const commentAsResource = toCommentResponse(row, userRef); + + if (row.deleted_at !== null) { + // Only restore (status:'open') is legal on a deleted row. + if (editing || status !== "open") throw new InputError("Cannot edit a deleted comment"); + // Restore reuses rwCommentDeletePermission — the same capability gate as + // deletion (undoing a delete is as powerful as performing one) — plus the + // author floor so only the original author can un-delete their own comment. + const frameworkAllowed = await checkResourcePermission( + rwCommentDeletePermission, + credentials, + commentAsResource, + ); + if (!frameworkAllowed) { + logCommentOp(logger, { + kind: "denied", + op: "mutate", + permission: "rwComment.delete", + userEntityRef: userRef, + }); + res.status(403).end(); + return; + } + assertAuthorFloor(userRef, row); + // Same TOCTOU guard as DELETE: re-read under a row lock inside a transaction + // and re-assert the row is still deleted before conditionally restoring. + // Permission + author-floor checks remain outside the transaction. + const restored = await store.transaction(async (tx) => { + const fresh = await store.get(row.id, { executor: tx, forUpdate: true }); + if (!fresh || fresh.deleted_at === null) { + throw new NotFoundError("Comment not found"); + } + return store.restore(row.id, tx); + }); + logCommentOp(logger, { + kind: "mutation", + op: "restore", + siteRef: row.site_ref, + commentId: row.id, + }); + res.json(toCommentResponse(restored!, userRef)); + return; + } + + if (editing) { + // Framework check (rwCommentEditPermission) + author floor (both must pass). + const frameworkAllowed = await checkResourcePermission( + rwCommentEditPermission, + credentials, + commentAsResource, + ); + if (!frameworkAllowed) { + logCommentOp(logger, { + kind: "denied", + op: "mutate", + permission: "rwComment.edit", + userEntityRef: userRef, + }); + res.status(403).end(); + return; + } + assertAuthorFloor(userRef, row); + } + + if (status !== undefined) { + // Replies cannot be resolved/reopened — canResolve is advertised false for replies. + if (row.parent_id !== null) throw new InputError("Cannot resolve a reply"); + // status change on a live row = resolve/reopen (collaborative — no floor, but framework applies) + const frameworkAllowed = await checkResourcePermission( + rwCommentResolvePermission, + credentials, + commentAsResource, + ); + if (!frameworkAllowed) { + logCommentOp(logger, { + kind: "denied", + op: "mutate", + permission: "rwComment.resolve", + userEntityRef: userRef, + }); + res.status(403).end(); + return; + } + } + + const updated = await store.update(row.id, { + ...(body !== undefined ? { body } : {}), + ...(selectors !== undefined ? { selectors } : {}), + ...(status !== undefined ? { status } : {}), + resolverRef: userRef, + }); + logCommentOp(logger, { + kind: "mutation", + op: status ? "resolve" : "edit", + siteRef: row.site_ref, + commentId: row.id, + }); + res.json(toCommentResponse(updated!, userRef)); + }); + + router.delete("/comments/:id", async (req, res) => { + const row = await store.get(req.params.id); + if (!row || row.parent_id === null || row.deleted_at !== null) + throw new NotFoundError("Reply not found"); + const userRef = await callerUserRef(req); + const credentials = await httpAuth.credentials(req, { allow: ["user"] }); + const commentAsResource = toCommentResponse(row, userRef); + // Framework check (rwCommentDeletePermission) — must pass alongside author floor. + const frameworkAllowed = await checkResourcePermission( + rwCommentDeletePermission, + credentials, + commentAsResource, + ); + if (!frameworkAllowed) { + logCommentOp(logger, { + kind: "denied", + op: "mutate", + permission: "rwComment.delete", + userEntityRef: userRef, + }); + res.status(403).end(); + return; + } + assertAuthorFloor(userRef, row); + // State-sensitive re-read + write inside a transaction closes the TOCTOU race + // between the `row` read above and the mutation: re-assert the reply is still + // live under a row lock, then conditionally soft-delete. The permission + + // author-floor checks stay OUTSIDE the transaction so we never hold a DB lock + // across external permission-service / auth calls. + const deleted = await store.transaction(async (tx) => { + const fresh = await store.get(row.id, { executor: tx, forUpdate: true }); + if (!fresh || fresh.parent_id === null || fresh.deleted_at !== null) { + throw new NotFoundError("Reply not found"); + } + return store.softDelete(row.id, tx); + }); + logCommentOp(logger, { + kind: "mutation", + op: "delete", + siteRef: row.site_ref, + commentId: row.id, + }); + // Return the full soft-deleted row so the viewer can evict the reply from its + // local cache by documentId. A 204 No Content would leave the viewer without + // the documentId it needs to locate and remove the reply from state. + res.json(toCommentResponse(deleted!, userRef)); + }); + + return router; +} diff --git a/plugins/rw-backend/src/comments/timestamps.test.ts b/plugins/rw-backend/src/comments/timestamps.test.ts new file mode 100644 index 0000000..044e864 --- /dev/null +++ b/plugins/rw-backend/src/comments/timestamps.test.ts @@ -0,0 +1,25 @@ +import { toIso } from "./timestamps"; + +describe("toIso", () => { + it("formats a JS Date (pg driver) to RFC3339", () => { + const d = new Date("2026-06-21T06:30:00.000Z"); + expect(toIso(d)).toBe("2026-06-21T06:30:00.000Z"); + }); + + it("parses an ISO string (sqlite) to RFC3339", () => { + expect(toIso("2026-06-21T06:30:00.000Z")).toBe("2026-06-21T06:30:00.000Z"); + }); + + it('parses a SQL datetime string ("YYYY-MM-DD HH:MM:SS", sqlite) as UTC', () => { + expect(toIso("2026-06-21 06:30:00")).toBe("2026-06-21T06:30:00.000Z"); + }); + + it("converts an epoch-millis number to RFC3339", () => { + expect(toIso(new Date("2026-06-21T06:30:00.000Z").getTime())).toBe("2026-06-21T06:30:00.000Z"); + }); + + it("returns undefined for null/undefined", () => { + expect(toIso(null)).toBeUndefined(); + expect(toIso(undefined)).toBeUndefined(); + }); +}); diff --git a/plugins/rw-backend/src/comments/timestamps.ts b/plugins/rw-backend/src/comments/timestamps.ts new file mode 100644 index 0000000..abf5128 --- /dev/null +++ b/plugins/rw-backend/src/comments/timestamps.ts @@ -0,0 +1,30 @@ +import { DateTime } from "luxon"; + +/** + * Normalize a dateTime column read across drivers to an RFC3339 UTC string. + * Returns undefined for null/undefined. + * + * Driver shapes: + * - Postgres → JS Date object + * - sqlite3 → ISO-like string ("YYYY-MM-DD HH:MM:SS" or ISO) + * - better-sqlite3 → epoch-millis number + */ +export function toIso(value: Date | string | number | null | undefined): string | undefined { + if (value === null || value === undefined) { + return undefined; + } + let dt: DateTime; + if ( + value instanceof Date || + (typeof value === "object" && typeof (value as any).getTime === "function") + ) { + dt = DateTime.fromJSDate(value as Date); + } else if (typeof value === "number") { + dt = DateTime.fromMillis(value); + } else if ((value as string).includes(" ")) { + dt = DateTime.fromSQL(value, { zone: "utc" }); // "YYYY-MM-DD HH:MM:SS" + } else { + dt = DateTime.fromISO(value, { zone: "utc" }); + } + return dt.toUTC().toISO() ?? undefined; +} diff --git a/plugins/rw-backend/src/comments/types.ts b/plugins/rw-backend/src/comments/types.ts new file mode 100644 index 0000000..63344da --- /dev/null +++ b/plugins/rw-backend/src/comments/types.ts @@ -0,0 +1,57 @@ +export type CommentStatus = "open" | "resolved"; + +export interface AuthorProfile { + displayName?: string; + picture?: string; +} + +/** snake_case DB row. Timestamp columns come back as Date (pg), string (sqlite), or + * number (better-sqlite3, which stores dateTime as epoch milliseconds). */ +export interface CommentRow { + id: string; + site_ref: string; + document_id: string; + entity_ref: string; + parent_id: string | null; + author_ref: string; + author_profile: string | null; // JSON AuthorProfile + body: string; + body_html: string; + selectors: string; // JSON unknown[] + status: CommentStatus; + created_at: Date | string | number; + updated_at: Date | string | number; + resolved_at: Date | string | number | null; + resolved_by: string | null; + deleted_at: Date | string | number | null; +} + +export interface CreateCommentInput { + documentId: string; + parentId?: string; + authorRef: string; + authorProfile?: AuthorProfile; + body: string; + selectors: unknown[]; +} + +export interface ListFilter { + documentId?: string; + entityRef?: string; + status?: CommentStatus; + parentId?: string | null; + topLevelOnly?: boolean; +} + +const SECTION_ROOT = "section:default/root"; + +/** + * Compute the content-owning entity ref from a verbatim documentId + host siteRef. + * Stored in `entity_ref` for future querying; not used as an authorization boundary + * (read scope is determined by the host `siteRef` — see `assertSiteVisible` in router.ts). + */ +export function computeEntityRef(documentId: string, siteRef: string): string { + const i = documentId.indexOf("#"); + const sectionRef = i === -1 ? documentId : documentId.slice(0, i); + return sectionRef === SECTION_ROOT ? siteRef : sectionRef; +} diff --git a/plugins/rw-backend/src/plugin.ts b/plugins/rw-backend/src/plugin.ts index 161d8d1..8416d56 100644 --- a/plugins/rw-backend/src/plugin.ts +++ b/plugins/rw-backend/src/plugin.ts @@ -1,8 +1,21 @@ -import { coreServices, createBackendPlugin } from "@backstage/backend-plugin-api"; +import { + coreServices, + createBackendPlugin, + resolvePackagePath, +} from "@backstage/backend-plugin-api"; import { readDurationFromConfig } from "@backstage/config"; -import { readRwSiteConfig, toEntityPath } from "@rwdocs/backstage-plugin-rw-common"; +import { + readRwSiteConfig, + rwCommentResourcePermissions, + toEntityPath, +} from "@rwdocs/backstage-plugin-rw-common"; +import { catalogServiceRef } from "@backstage/plugin-catalog-node"; import { createRouter } from "./router"; import { Hub, type HubOptions } from "./hub"; +import { CommentStore } from "./comments/CommentStore"; +import { createCommentsRouter } from "./comments/router"; +import { commentResourceRef, isCommentAuthor } from "./comments/permissions"; +import { toCommentResponse } from "./comments/mapping"; export const rwPlugin = createBackendPlugin({ pluginId: "rw", @@ -14,8 +27,26 @@ export const rwPlugin = createBackendPlugin({ logger: coreServices.logger, config: coreServices.rootConfig, scheduler: coreServices.scheduler, + database: coreServices.database, + permissions: coreServices.permissions, + permissionsRegistry: coreServices.permissionsRegistry, + userInfo: coreServices.userInfo, + auth: coreServices.auth, + catalog: catalogServiceRef, }, - async init({ httpRouter, httpAuth, logger, config, scheduler }) { + async init({ + httpRouter, + httpAuth, + logger, + config, + scheduler, + database, + permissions, + permissionsRegistry, + userInfo, + auth, + catalog, + }) { const siteConfig = readRwSiteConfig(config); const cacheSize = config.getOptionalNumber("rw.cacheSize"); @@ -49,8 +80,48 @@ export const rwPlugin = createBackendPlugin({ }); } - const router = await createRouter({ logger, httpAuth, hub }); + const client = await database.getClient(); + if (!database.migrations?.skip) { + await client.migrate.latest({ + directory: resolvePackagePath("@rwdocs/backstage-plugin-rw-backend", "migrations"), + }); + } + const store = new CommentStore(client); + + const commentsEnabled = config.getOptionalBoolean("rw.comments.enabled") ?? true; + + permissionsRegistry.addResourceType({ + resourceRef: commentResourceRef, + permissions: rwCommentResourcePermissions, + rules: [isCommentAuthor], + getResources: async (ids: string[]) => + Promise.all( + ids.map(async (id) => { + const row = await store.get(id); + return row ? toCommentResponse(row, undefined) : undefined; + }), + ), + }); + + const router = await createRouter({ + logger, + httpAuth, + hub, + }); httpRouter.use(router); + httpRouter.use( + createCommentsRouter({ + store, + logger, + httpAuth, + auth, + userInfo, + permissions, + permissionsRegistry, + catalog, + commentsEnabled, + }), + ); httpRouter.addAuthPolicy({ path: "/health", allow: "unauthenticated", diff --git a/plugins/rw-backend/src/router.test.ts b/plugins/rw-backend/src/router.test.ts index 1394bcc..bbf5f58 100644 --- a/plugins/rw-backend/src/router.test.ts +++ b/plugins/rw-backend/src/router.test.ts @@ -1,3 +1,4 @@ +import * as http from "http"; import { mockServices } from "@backstage/backend-test-utils"; import express from "express"; import request from "supertest"; @@ -9,26 +10,25 @@ jest.mock("@rwdocs/core"); const mockCreateSite = createSite as jest.MockedFunction; -function makeApp(hub: Hub) { - return createRouter({ +async function makeServer(hub: Hub): Promise { + const router = await createRouter({ logger: mockServices.logger.mock(), httpAuth: mockServices.httpAuth.mock(), hub, - }).then((router) => { - const app = express().use(router); - app.use( - (err: any, _req: express.Request, res: express.Response, _next: express.NextFunction) => { - const statusByName: Record = { - InputError: 400, - NotFoundError: 404, - ServiceUnavailableError: 503, - }; - const status = statusByName[err.name] ?? 500; - res.status(status).json({ error: { name: err.name, message: err.message } }); - }, - ); - return app; }); + const app = express().use(router); + app.use((err: any, _req: express.Request, res: express.Response, _next: express.NextFunction) => { + const statusByName: Record = { + InputError: 400, + NotFoundError: 404, + ServiceUnavailableError: 503, + }; + const status = statusByName[err.name] ?? 500; + res.status(status).json({ error: { name: err.name, message: err.message } }); + }); + const server = http.createServer(app); + await new Promise((r) => server.listen(0, () => r())); + return server; } describe("createRouter", () => { @@ -38,7 +38,7 @@ describe("createRouter", () => { reload: jest.fn(), }; - let app: express.Express; + let server: http.Server; beforeAll(async () => { mockCreateSite.mockReturnValue(mockSite as any); @@ -46,7 +46,11 @@ describe("createRouter", () => { projectDir: "/test/docs", entity: "component:default/test", }); - app = await makeApp(hub); + server = await makeServer(hub); + }); + + afterAll(async () => { + await new Promise((r) => server.close(() => r())); }); beforeEach(() => { @@ -59,7 +63,7 @@ describe("createRouter", () => { describe("GET /health", () => { it("returns ok", async () => { - const res = await request(app).get("/health"); + const res = await request(server).get("/health"); expect(res.status).toBe(200); expect(res.body).toEqual({ status: "ok" }); }); @@ -67,7 +71,7 @@ describe("createRouter", () => { describe(`GET ${prefix}/config`, () => { it("returns config with liveReloadEnabled false", async () => { - const res = await request(app).get(`${prefix}/config`); + const res = await request(server).get(`${prefix}/config`); expect(res.status).toBe(200); expect(res.body).toEqual({ liveReloadEnabled: false }); }); @@ -78,7 +82,7 @@ describe("createRouter", () => { it("returns navigation with null scope when no query param", async () => { mockSite.getNavigation.mockResolvedValue(mockNav); - const res = await request(app).get(`${prefix}/navigation`); + const res = await request(server).get(`${prefix}/navigation`); expect(res.status).toBe(200); expect(res.body).toEqual(mockNav); expect(mockSite.getNavigation).toHaveBeenCalledWith(null); @@ -86,7 +90,7 @@ describe("createRouter", () => { it("passes sectionRef query param to getNavigation", async () => { mockSite.getNavigation.mockResolvedValue(mockNav); - const res = await request(app).get(`${prefix}/navigation?sectionRef=api`); + const res = await request(server).get(`${prefix}/navigation?sectionRef=api`); expect(res.status).toBe(200); expect(mockSite.getNavigation).toHaveBeenCalledWith("api"); }); @@ -97,7 +101,7 @@ describe("createRouter", () => { it("renders root page", async () => { mockSite.renderPage.mockResolvedValue(mockPage); - const res = await request(app).get(`${prefix}/pages/`); + const res = await request(server).get(`${prefix}/pages/`); expect(res.status).toBe(200); expect(res.body).toEqual(mockPage); expect(mockSite.renderPage).toHaveBeenCalledWith(""); @@ -105,26 +109,26 @@ describe("createRouter", () => { it("renders nested page", async () => { mockSite.renderPage.mockResolvedValue(mockPage); - const res = await request(app).get(`${prefix}/pages/getting-started`); + const res = await request(server).get(`${prefix}/pages/getting-started`); expect(res.status).toBe(200); expect(mockSite.renderPage).toHaveBeenCalledWith("getting-started"); }); it("rejects path traversal with 400", async () => { - const res = await request(app).get(`${prefix}/pages/a%2F..%2Fb`); + const res = await request(server).get(`${prefix}/pages/a%2F..%2Fb`); expect(res.status).toBe(400); expect(mockSite.renderPage).not.toHaveBeenCalled(); }); it("returns 404 when page not found", async () => { mockSite.renderPage.mockRejectedValue(new Error("Content not found")); - const res = await request(app).get(`${prefix}/pages/nonexistent`); + const res = await request(server).get(`${prefix}/pages/nonexistent`); expect(res.status).toBe(404); }); it("returns 500 on unexpected render error", async () => { mockSite.renderPage.mockRejectedValue(new Error("disk read failed")); - const res = await request(app).get(`${prefix}/pages/broken`); + const res = await request(server).get(`${prefix}/pages/broken`); expect(res.status).toBe(500); }); @@ -141,7 +145,7 @@ describe("createRouter", () => { const mockScopedPage = { title: "Billing", content: "

Billing docs

" }; mockSite.renderPage.mockResolvedValue(mockScopedPage); - await request(app).get(`${prefix}/pages/?sectionRef=domain:default/billing`); + await request(server).get(`${prefix}/pages/?sectionRef=domain:default/billing`); expect(mockSite.renderPage).toHaveBeenCalledWith("domains/billing"); }); }); @@ -149,21 +153,21 @@ describe("createRouter", () => { describe("storage errors", () => { it("returns 503 when getNavigation throws storage error", async () => { mockSite.getNavigation.mockRejectedValue(new Error("S3: storage unavailable")); - const res = await request(app).get(`${prefix}/navigation`); + const res = await request(server).get(`${prefix}/navigation`); expect(res.status).toBe(503); expect(res.body.error.name).toBe("ServiceUnavailableError"); }); it("returns 503 when renderPage throws storage error", async () => { mockSite.renderPage.mockRejectedValue(new Error("Storage error: S3: storage unavailable")); - const res = await request(app).get(`${prefix}/pages/guide`); + const res = await request(server).get(`${prefix}/pages/guide`); expect(res.status).toBe(503); expect(res.body.error.name).toBe("ServiceUnavailableError"); }); it("returns 503 when getNavigation throws on scope resolution", async () => { mockSite.getNavigation.mockRejectedValue(new Error("S3: storage unavailable")); - const res = await request(app).get(`${prefix}/pages/?sectionRef=domain:default/billing`); + const res = await request(server).get(`${prefix}/pages/?sectionRef=domain:default/billing`); expect(res.status).toBe(503); expect(res.body.error.name).toBe("ServiceUnavailableError"); }); @@ -171,7 +175,7 @@ describe("createRouter", () => { describe("unknown entity ref", () => { it("returns 404 for non-existent entity", async () => { - const res = await request(app).get("/site/default/component/unknown/config"); + const res = await request(server).get("/site/default/component/unknown/config"); expect(res.status).toBe(404); }); }); diff --git a/plugins/rw-backend/src/router.ts b/plugins/rw-backend/src/router.ts index 6aca40e..6c8d973 100644 --- a/plugins/rw-backend/src/router.ts +++ b/plugins/rw-backend/src/router.ts @@ -98,7 +98,7 @@ async function renderPageOrThrow(site: RwSite, pagePath: string) { } } -function toStorageError(err: unknown): ServiceUnavailableError { +export function toStorageError(err: unknown): ServiceUnavailableError { const message = err instanceof Error ? err.message : String(err); return new ServiceUnavailableError(`Storage unavailable: ${message}`); } diff --git a/plugins/rw-common/config.d.ts b/plugins/rw-common/config.d.ts index 027d356..f0e703b 100644 --- a/plugins/rw-common/config.d.ts +++ b/plugins/rw-common/config.d.ts @@ -52,5 +52,17 @@ export interface Config { /** Diagram rendering DPI. */ dpi?: number; }; + /** + * Inline/page comments in the embedded viewer. + */ + comments?: { + /** + * Enable comments. The frontend reads this via GET /comments/config and + * gates client injection on it; the backend defaults it to true in code + * (this @default annotation is a docs/UI hint only — Backstage does not apply it). + * @default true + */ + enabled?: boolean; + }; }; } diff --git a/plugins/rw-common/package.json b/plugins/rw-common/package.json index 5e22378..cf30b90 100644 --- a/plugins/rw-common/package.json +++ b/plugins/rw-common/package.json @@ -50,6 +50,7 @@ "@backstage/catalog-model": "^1.7.6", "@backstage/config": "^1.3.6", "@backstage/errors": "^1.2.7", + "@backstage/plugin-permission-common": "^0.9.9", "@backstage/types": "^1.2.2" }, "devDependencies": { diff --git a/plugins/rw-common/src/index.ts b/plugins/rw-common/src/index.ts index e21b6f9..7dcc9e8 100644 --- a/plugins/rw-common/src/index.ts +++ b/plugins/rw-common/src/index.ts @@ -3,3 +3,4 @@ export { parseAnnotation } from "./parseAnnotation"; export type { ParsedAnnotation } from "./parseAnnotation"; export { readRwSiteConfig } from "./config"; export type { RwSiteConfig, S3Config, RwDiagramsConfig } from "./config"; +export * from "./permissions"; diff --git a/plugins/rw-common/src/permissions.test.ts b/plugins/rw-common/src/permissions.test.ts new file mode 100644 index 0000000..bbabd7a --- /dev/null +++ b/plugins/rw-common/src/permissions.test.ts @@ -0,0 +1,53 @@ +import { + RESOURCE_TYPE_RW_COMMENT, + rwCommentReadPermission, + rwCommentCreatePermission, + rwCommentResolvePermission, + rwCommentEditPermission, + rwCommentDeletePermission, + rwCommentPermissions, + rwCommentResourcePermissions, +} from "./permissions"; + +describe("rw comment permissions", () => { + it("read and create are basic permissions (no resourceType)", () => { + expect(rwCommentReadPermission.name).toBe("rwComment.read"); + expect(rwCommentReadPermission.attributes.action).toBe("read"); + expect((rwCommentReadPermission as { resourceType?: string }).resourceType).toBeUndefined(); + + expect(rwCommentCreatePermission.name).toBe("rwComment.create"); + expect(rwCommentCreatePermission.attributes.action).toBe("create"); + expect((rwCommentCreatePermission as { resourceType?: string }).resourceType).toBeUndefined(); + }); + + it("resolve/edit/delete are resource permissions of type rw-comment", () => { + expect(RESOURCE_TYPE_RW_COMMENT).toBe("rw-comment"); + for (const p of [ + rwCommentResolvePermission, + rwCommentEditPermission, + rwCommentDeletePermission, + ]) { + expect((p as { resourceType?: string }).resourceType).toBe("rw-comment"); + } + expect(rwCommentResolvePermission.name).toBe("rwComment.resolve"); + expect(rwCommentResolvePermission.attributes.action).toBe("update"); + expect(rwCommentEditPermission.name).toBe("rwComment.edit"); + expect(rwCommentEditPermission.attributes.action).toBe("update"); + expect(rwCommentDeletePermission.name).toBe("rwComment.delete"); + expect(rwCommentDeletePermission.attributes.action).toBe("delete"); + }); + + it("aggregate arrays contain the right members", () => { + expect(rwCommentPermissions).toHaveLength(5); + expect(rwCommentResourcePermissions).toEqual([ + rwCommentResolvePermission, + rwCommentEditPermission, + rwCommentDeletePermission, + ]); + }); + + it("permission names are unique", () => { + const names = rwCommentPermissions.map((p) => p.name); + expect(new Set(names).size).toBe(names.length); + }); +}); diff --git a/plugins/rw-common/src/permissions.ts b/plugins/rw-common/src/permissions.ts new file mode 100644 index 0000000..46ac12c --- /dev/null +++ b/plugins/rw-common/src/permissions.ts @@ -0,0 +1,51 @@ +import { createPermission } from "@backstage/plugin-permission-common"; + +/** Resource type for comment resource permissions; shared with the backend resource ref. */ +export const RESOURCE_TYPE_RW_COMMENT = "rw-comment"; + +/** List/read comments (basic — the coarse global toggle; entity-read is layered on in the backend). */ +export const rwCommentReadPermission = createPermission({ + name: "rwComment.read", + attributes: { action: "read" }, +}); + +/** Create a comment or reply (basic). */ +export const rwCommentCreatePermission = createPermission({ + name: "rwComment.create", + attributes: { action: "create" }, +}); + +/** Resolve/reopen a thread (resource; collaborative by default, gateable by a policy). */ +export const rwCommentResolvePermission = createPermission({ + name: "rwComment.resolve", + attributes: { action: "update" }, + resourceType: RESOURCE_TYPE_RW_COMMENT, +}); + +/** Edit a comment body/selectors (resource; author-floored in the backend). */ +export const rwCommentEditPermission = createPermission({ + name: "rwComment.edit", + attributes: { action: "update" }, + resourceType: RESOURCE_TYPE_RW_COMMENT, +}); + +/** Delete (soft) and restore a reply (resource; author-floored in the backend). */ +export const rwCommentDeletePermission = createPermission({ + name: "rwComment.delete", + attributes: { action: "delete" }, + resourceType: RESOURCE_TYPE_RW_COMMENT, +}); + +/** The three resource permissions the backend registers via addResourceType. */ +export const rwCommentResourcePermissions = [ + rwCommentResolvePermission, + rwCommentEditPermission, + rwCommentDeletePermission, +]; + +/** All five comment permissions (for documentation / a policy author's reference). */ +export const rwCommentPermissions = [ + rwCommentReadPermission, + rwCommentCreatePermission, + ...rwCommentResourcePermissions, +]; diff --git a/plugins/rw/package.json b/plugins/rw/package.json index 6d1a3b9..f10eafa 100644 --- a/plugins/rw/package.json +++ b/plugins/rw/package.json @@ -48,7 +48,7 @@ "@backstage/catalog-model": "^1.7.6", "@material-ui/icons": "^4.11.3", "@rwdocs/backstage-plugin-rw-common": "workspace:^", - "@rwdocs/viewer": "^0.1.23" + "@rwdocs/viewer": "^0.1.27" }, "peerDependencies": { "@backstage/core-components": "^0.18.0", diff --git a/plugins/rw/src/__mocks__/@rwdocs/viewer.js b/plugins/rw/src/__mocks__/@rwdocs/viewer.js index 2b855ff..220de0e 100644 --- a/plugins/rw/src/__mocks__/@rwdocs/viewer.js +++ b/plugins/rw/src/__mocks__/@rwdocs/viewer.js @@ -1,3 +1,6 @@ -module.exports = { - mountRw: jest.fn(), -}; +const mountRw = jest.fn(() => ({ + destroy: jest.fn(), + navigateTo: jest.fn(), + setColorScheme: jest.fn(), +})); +module.exports = { mountRw }; diff --git a/plugins/rw/src/api/RwClient.test.ts b/plugins/rw/src/api/RwClient.test.ts index 9c87d27..24603eb 100644 --- a/plugins/rw/src/api/RwClient.test.ts +++ b/plugins/rw/src/api/RwClient.test.ts @@ -27,3 +27,76 @@ describe("RwClient", () => { }); }); }); + +function makeClient() { + const fetchMock = jest.fn(); + const discoveryApi = { getBaseUrl: jest.fn(async () => "http://backstage/api/rw") }; + const fetchApi = { fetch: fetchMock }; + const client = new RwClient({ discoveryApi: discoveryApi as any, fetchApi: fetchApi as any }); + return { client, fetchMock }; +} + +describe("RwClient comment methods", () => { + it("getCommentsEnabled reads /comments/config", async () => { + const { client, fetchMock } = makeClient(); + fetchMock.mockResolvedValue({ ok: true, json: async () => ({ enabled: true }) }); + await expect(client.getCommentsEnabled()).resolves.toBe(true); + expect(fetchMock).toHaveBeenCalledWith("http://backstage/api/rw/comments/config"); + }); + + it("getCommentsEnabled returns false (not throws) on a non-ok response", async () => { + const { client, fetchMock } = makeClient(); + fetchMock.mockResolvedValue({ ok: false, status: 503 }); + await expect(client.getCommentsEnabled()).resolves.toBe(false); + }); + + it("getCommentsEnabled propagates fetch rejection (caller is responsible for catching and degrading to disabled)", async () => { + const { client, fetchMock } = makeClient(); + fetchMock.mockRejectedValue(new Error("network timeout")); + await expect(client.getCommentsEnabled()).rejects.toThrow("network timeout"); + }); + + it("list() GETs /comments with siteRef + documentId", async () => { + const { client, fetchMock } = makeClient(); + fetchMock.mockResolvedValue({ ok: true, json: async () => [] }); + const cc = client.createCommentClient("component:default/arch"); + await cc.list("section:default/root#guide"); + const url = fetchMock.mock.calls[0][0] as string; + expect(url).toContain("/comments?"); + expect(url).toContain("siteRef=component%3Adefault%2Farch"); + expect(url).toContain("documentId=section%3Adefault%2Froot%23guide"); + }); + + it("create() POSTs /comments with siteRef merged into the body", async () => { + const { client, fetchMock } = makeClient(); + fetchMock.mockResolvedValue({ ok: true, json: async () => ({ id: "c1" }) }); + const cc = client.createCommentClient("component:default/arch"); + await cc.create({ documentId: "d#p", body: "hi", selectors: [] } as any); + const [url, init] = fetchMock.mock.calls[0]; + expect(url).toBe("http://backstage/api/rw/comments"); + expect(init.method).toBe("POST"); + expect(JSON.parse(init.body)).toMatchObject({ + siteRef: "component:default/arch", + documentId: "d#p", + body: "hi", + }); + }); + + it("update() PATCHes /comments/:id and delete() DELETEs it", async () => { + const { client, fetchMock } = makeClient(); + fetchMock.mockResolvedValue({ ok: true, json: async () => ({ id: "c1" }) }); + const cc = client.createCommentClient("component:default/arch"); + await cc.update("c1", { status: "resolved" }); + expect(fetchMock.mock.calls[0][1].method).toBe("PATCH"); + expect(fetchMock.mock.calls[0][0]).toBe("http://backstage/api/rw/comments/c1"); + await cc.delete("c1"); + expect(fetchMock.mock.calls[1][1].method).toBe("DELETE"); + }); + + it("throws on a non-ok response", async () => { + const { client, fetchMock } = makeClient(); + fetchMock.mockResolvedValue({ ok: false, status: 500, json: async () => ({}) }); + const cc = client.createCommentClient("component:default/arch"); + await expect(cc.list("d#p")).rejects.toThrow(); + }); +}); diff --git a/plugins/rw/src/api/RwClient.ts b/plugins/rw/src/api/RwClient.ts index 8ac5ea9..227dc56 100644 --- a/plugins/rw/src/api/RwClient.ts +++ b/plugins/rw/src/api/RwClient.ts @@ -1,10 +1,18 @@ import { createApiRef } from "@backstage/core-plugin-api"; import type { DiscoveryApi, FetchApi } from "@backstage/core-plugin-api"; +import type { + CommentApiClient, + Comment, + CreateCommentRequest, + UpdateCommentRequest, +} from "@rwdocs/viewer"; export interface RwApi { getBaseUrl(): Promise; getSiteBaseUrl(entityRef: string): Promise; getFetch(): typeof fetch; + getCommentsEnabled(): Promise; + createCommentClient(siteRef: string): CommentApiClient; } export const rwApiRef = createApiRef({ id: "plugin.rw.api" }); @@ -30,4 +38,57 @@ export class RwClient implements RwApi { getFetch(): typeof fetch { return this.fetchApi.fetch; } + + async getCommentsEnabled(): Promise { + const base = await this.discoveryApi.getBaseUrl("rw"); + const res = await this.fetchApi.fetch(`${base}/comments/config`); + if (!res.ok) return false; + const body = await res.json(); + return Boolean(body.enabled); + } + + createCommentClient(siteRef: string): CommentApiClient { + const json = async (res: Response) => { + if (!res.ok) throw new Error(`Comment request failed: ${res.status}`); + return res.json(); + }; + const baseUrl = () => this.discoveryApi.getBaseUrl("rw"); + return { + list: async (documentId: string, opts?: { signal?: AbortSignal }): Promise => { + const base = await baseUrl(); + const q = new URLSearchParams({ siteRef, documentId }); + return json( + await this.fetchApi.fetch(`${base}/comments?${q.toString()}`, { signal: opts?.signal }), + ); + }, + create: async (input: CreateCommentRequest): Promise => { + const base = await baseUrl(); + return json( + await this.fetchApi.fetch(`${base}/comments`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ siteRef, ...input }), + }), + ); + }, + update: async (id: string, input: UpdateCommentRequest): Promise => { + const base = await baseUrl(); + return json( + await this.fetchApi.fetch(`${base}/comments/${encodeURIComponent(id)}`, { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(input), + }), + ); + }, + delete: async (id: string): Promise => { + const base = await baseUrl(); + return json( + await this.fetchApi.fetch(`${base}/comments/${encodeURIComponent(id)}`, { + method: "DELETE", + }), + ); + }, + }; + } } diff --git a/plugins/rw/src/components/RwDocsViewer.test.tsx b/plugins/rw/src/components/RwDocsViewer.test.tsx index c53aaf0..5c1c2c4 100644 --- a/plugins/rw/src/components/RwDocsViewer.test.tsx +++ b/plugins/rw/src/components/RwDocsViewer.test.tsx @@ -36,6 +36,8 @@ function createMockRwApi(overrides?: Partial): RwApi { Promise.resolve(`http://localhost:7007/api/rw/site/${entityRef}`), ), getFetch: jest.fn().mockReturnValue(jest.fn()), + getCommentsEnabled: jest.fn().mockResolvedValue(false), + createCommentClient: jest.fn().mockReturnValue(undefined), ...overrides, }; } @@ -118,17 +120,6 @@ describe("RwDocsViewer", () => { }); }); - it("passes resolveSectionRefs to mountRw", async () => { - await renderViewer(createMockRwApi()); - - await waitFor(() => { - expect(mockMountRw).toHaveBeenCalledTimes(1); - }); - - const [, options] = mockMountRw.mock.calls[0]; - expect(typeof options.resolveSectionRefs).toBe("function"); - }); - it("overrides self sectionRef with current basePath in resolveSectionRefs", async () => { await renderViewer(createMockRwApi()); @@ -158,4 +149,36 @@ describe("RwDocsViewer", () => { unmount(); expect(mockDestroy).toHaveBeenCalled(); }); + + it("passes comments to mountRw when the comments prop is provided", async () => { + const stubCommentClient = { + list: jest.fn(), + create: jest.fn(), + update: jest.fn(), + delete: jest.fn(), + }; + + await renderInTestApp( + + + , + ); + + await waitFor(() => { + expect(mockMountRw).toHaveBeenCalledTimes(1); + }); + + const [, options] = mockMountRw.mock.calls[0]; + expect(options).toHaveProperty("comments", stubCommentClient); + }); }); diff --git a/plugins/rw/src/components/RwDocsViewer.tsx b/plugins/rw/src/components/RwDocsViewer.tsx index a7b5fdd..bab6e3d 100644 --- a/plugins/rw/src/components/RwDocsViewer.tsx +++ b/plugins/rw/src/components/RwDocsViewer.tsx @@ -6,16 +6,22 @@ import { useLocation, useNavigate, useParams } from "react-router-dom"; import { rwApiRef } from "../api/RwClient"; import { useSectionRefResolver } from "./useSectionRefResolver"; import { mountRw } from "@rwdocs/viewer"; -import type { RwInstance } from "@rwdocs/viewer"; +import type { RwInstance, CommentApiClient } from "@rwdocs/viewer"; import "@rwdocs/viewer/embed.css"; interface RwDocsViewerProps { apiBaseUrl: string; sectionRef: string; sourceEntityRef: string; + comments?: CommentApiClient; } -export function RwDocsViewer({ apiBaseUrl, sectionRef, sourceEntityRef }: RwDocsViewerProps) { +export function RwDocsViewer({ + apiBaseUrl, + sectionRef, + sourceEntityRef, + comments, +}: RwDocsViewerProps) { const ref = useRef(null); const rwApi = useApi(rwApiRef); const theme = useTheme(); @@ -75,6 +81,7 @@ export function RwDocsViewer({ apiBaseUrl, sectionRef, sourceEntityRef }: RwDocs navigateRef.current(href, { replace: false }); } }, + ...(comments ? { comments } : {}), }); } catch (err) { setError(err instanceof Error ? err : new Error(String(err))); @@ -85,7 +92,7 @@ export function RwDocsViewer({ apiBaseUrl, sectionRef, sourceEntityRef }: RwDocs instanceRef.current = null; }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [apiBaseUrl, sectionRef]); + }, [apiBaseUrl, sectionRef, comments]); useEffect(() => { instanceRef.current?.setColorScheme(theme.palette.type); diff --git a/plugins/rw/src/components/RwEntityDocsViewer.test.tsx b/plugins/rw/src/components/RwEntityDocsViewer.test.tsx index 8281529..af4c077 100644 --- a/plugins/rw/src/components/RwEntityDocsViewer.test.tsx +++ b/plugins/rw/src/components/RwEntityDocsViewer.test.tsx @@ -5,6 +5,9 @@ import { Entity } from "@backstage/catalog-model"; import { RwEntityDocsViewer } from "./RwEntityDocsViewer"; import { rwApiRef } from "../api/RwClient"; import type { RwApi } from "../api/RwClient"; +import { mountRw } from "@rwdocs/viewer"; + +const mockMountRw = mountRw as jest.MockedFunction; const mockCatalogApi = { getEntityByRef: jest.fn().mockResolvedValue(undefined), @@ -27,6 +30,8 @@ function createMockRwApi(overrides?: Partial): RwApi { Promise.resolve(`http://localhost:7007/api/rw/site/${entityRef}`), ), getFetch: jest.fn().mockReturnValue(jest.fn()), + getCommentsEnabled: jest.fn().mockResolvedValue(false), + createCommentClient: jest.fn().mockReturnValue(undefined), ...overrides, }; } @@ -43,9 +48,29 @@ function makeEntity(annotations?: Record): Entity { }; } +function makeApisElement(mockApi: RwApi, entity: Entity) { + return ( + + + + + + ); +} + describe("RwEntityDocsViewer", () => { beforeEach(() => { jest.clearAllMocks(); + mockMountRw.mockReturnValue({ + destroy: jest.fn(), + navigateTo: jest.fn(), + setColorScheme: jest.fn(), + }); }); it("shows error when annotation is missing", async () => { @@ -137,4 +162,192 @@ describe("RwEntityDocsViewer", () => { expect(screen.getAllByText(/network error/).length).toBeGreaterThan(0); }); }); + + it("renders Progress (no viewer) until both apiBaseUrl and comments check complete", async () => { + let resolveUrl!: (url: string) => void; + const urlPromise = new Promise((res) => { + resolveUrl = res; + }); + const mockApi = createMockRwApi({ + getSiteBaseUrl: jest.fn().mockReturnValue(urlPromise), + }); + const entity = makeEntity({ "rwdocs.org/ref": "." }); + + // Await the render so the component settles into its initial gated state + // (effect queued, URL promise still pending). + await renderInTestApp(makeApisElement(mockApi, entity)); + + // The Progress indicator must be visible and the viewer must be absent + // while the URL promise is still pending — this is the meaningful gate assertion. + // Backstage's Progress renders with data-testid="progress" in the test environment. + expect(screen.getByTestId("progress")).toBeInTheDocument(); + expect(document.querySelector(".rw-root")).not.toBeInTheDocument(); + + resolveUrl("http://localhost:7007/api/rw/site/default/component/my-service"); + + // After both fetches resolve, the viewer div should appear and the progress indicator should be gone. + await waitFor(() => { + expect(document.querySelector(".rw-root")).toBeInTheDocument(); + }); + expect(screen.queryByTestId("progress")).not.toBeInTheDocument(); + }); + + it("calls createCommentClient when getCommentsEnabled resolves true; comments prop is undefined when disabled", async () => { + const stubCommentClient = { + list: jest.fn(), + create: jest.fn(), + update: jest.fn(), + delete: jest.fn(), + }; + + // --- enabled case --- + const mockApiEnabled = createMockRwApi({ + getCommentsEnabled: jest.fn().mockResolvedValue(true), + createCommentClient: jest.fn().mockReturnValue(stubCommentClient), + }); + const entity = makeEntity({ "rwdocs.org/ref": "." }); + + await renderInTestApp(makeApisElement(mockApiEnabled, entity)); + + await waitFor(() => { + expect(mockMountRw).toHaveBeenCalled(); + }); + const [, optionsEnabled] = mockMountRw.mock.calls[0]; + expect(mockApiEnabled.createCommentClient).toHaveBeenCalledWith("component:default/my-service"); + expect(optionsEnabled.comments).toBe(stubCommentClient); + + jest.clearAllMocks(); + mockMountRw.mockReturnValue({ + destroy: jest.fn(), + navigateTo: jest.fn(), + setColorScheme: jest.fn(), + }); + + // --- disabled case --- + const mockApiDisabled = createMockRwApi({ + getCommentsEnabled: jest.fn().mockResolvedValue(false), + }); + + await renderInTestApp(makeApisElement(mockApiDisabled, entity)); + + await waitFor(() => { + expect(mockMountRw).toHaveBeenCalled(); + }); + const [, optionsDisabled] = mockMountRw.mock.calls[0]; + expect(mockApiDisabled.createCommentClient).not.toHaveBeenCalled(); + expect(optionsDisabled.comments).toBeUndefined(); + }); + + it("mounts viewer with comments undefined when getCommentsEnabled rejects (graceful degradation)", async () => { + const mockApi = createMockRwApi({ + getCommentsEnabled: jest.fn().mockRejectedValue(new Error("comments service unavailable")), + }); + const entity = makeEntity({ "rwdocs.org/ref": "." }); + + await renderInTestApp(makeApisElement(mockApi, entity)); + + // Viewer should still mount despite getCommentsEnabled rejection + await waitFor(() => { + expect(mockMountRw).toHaveBeenCalled(); + }); + const [, options] = mockMountRw.mock.calls[0]; + expect(options.comments).toBeUndefined(); + // No error panel should be shown + expect(screen.queryByText(/comments service unavailable/)).not.toBeInTheDocument(); + }); + + it("warns via console.warn when getCommentsEnabled rejects; success path does not warn", async () => { + const warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}); + + try { + const probeError = new Error("comments service unavailable"); + const mockApiRejecting = createMockRwApi({ + getCommentsEnabled: jest.fn().mockRejectedValue(probeError), + }); + const entity = makeEntity({ "rwdocs.org/ref": "." }); + + await renderInTestApp(makeApisElement(mockApiRejecting, entity)); + + // (a) console.warn must be called exactly once with the probe-failure message + await waitFor(() => { + expect(warnSpy).toHaveBeenCalledTimes(1); + }); + expect(warnSpy).toHaveBeenCalledWith( + "rw: comments-enabled probe failed; comments disabled for this view", + probeError, + ); + + // (b) viewer still mounts with comments undefined + await waitFor(() => { + expect(mockMountRw).toHaveBeenCalled(); + }); + const [, options] = mockMountRw.mock.calls[0]; + expect(options.comments).toBeUndefined(); + + // (c) no ErrorPanel + expect(screen.queryByText(/comments service unavailable/)).not.toBeInTheDocument(); + + // Reset for the success-path check + warnSpy.mockClear(); + jest.clearAllMocks(); + mockMountRw.mockReturnValue({ + destroy: jest.fn(), + navigateTo: jest.fn(), + setColorScheme: jest.fn(), + }); + + // Success path: getCommentsEnabled resolves normally — no warn + const mockApiSuccess = createMockRwApi({ + getCommentsEnabled: jest.fn().mockResolvedValue(false), + }); + + await renderInTestApp(makeApisElement(mockApiSuccess, entity)); + + await waitFor(() => { + expect(mockMountRw).toHaveBeenCalled(); + }); + expect(warnSpy).not.toHaveBeenCalled(); + } finally { + warnSpy.mockRestore(); + } + }); + + it("(regression) clears stale fetchError on navigation so the viewer renders for the new entity", async () => { + // Entity A: fetch fails → error panel + const entityA = makeEntity({ "rwdocs.org/ref": "." }); + const entityB: Entity = { + apiVersion: "backstage.io/v1alpha1", + kind: "Component", + metadata: { + name: "other-service", + namespace: "default", + annotations: { "rwdocs.org/ref": "." }, + }, + }; + + let callCount = 0; + const mockApi = createMockRwApi({ + getSiteBaseUrl: jest.fn().mockImplementation(() => { + callCount++; + if (callCount === 1) return Promise.reject(new Error("network error")); + return Promise.resolve(`http://localhost:7007/api/rw/site/default/component/other-service`); + }), + }); + + const { rerender } = await renderInTestApp(makeApisElement(mockApi, entityA)); + + // Error panel should appear for entity A + await waitFor(() => { + expect(screen.getAllByText(/network error/).length).toBeGreaterThan(0); + }); + + // Navigate to entity B (valid) + rerender(makeApisElement(mockApi, entityB)); + + // The stale error panel should be gone; the viewer should mount + await waitFor(() => { + expect(document.querySelector(".rw-root")).toBeInTheDocument(); + }); + expect(screen.queryByText(/network error/)).not.toBeInTheDocument(); + }); }); diff --git a/plugins/rw/src/components/RwEntityDocsViewer.tsx b/plugins/rw/src/components/RwEntityDocsViewer.tsx index d615993..68809bd 100644 --- a/plugins/rw/src/components/RwEntityDocsViewer.tsx +++ b/plugins/rw/src/components/RwEntityDocsViewer.tsx @@ -7,12 +7,18 @@ import { toEntityPath, parseAnnotation } from "@rwdocs/backstage-plugin-rw-commo import { rwApiRef } from "../api/RwClient"; import { ANNOTATION_KEY } from "./constants"; import { RwDocsViewer } from "./RwDocsViewer"; +import type { CommentApiClient } from "@rwdocs/viewer"; export function RwEntityDocsViewer() { const { entity } = useEntity(); const rwApi = useApi(rwApiRef); const [apiBaseUrl, setApiBaseUrl] = useState(null); const [fetchError, setFetchError] = useState(null); + const [commentClient, setCommentClient] = useState(undefined); + // Two gates before the viewer mounts: apiBaseUrl resolves first, then we wait + // for the comments-enabled check so the viewer never mounts and immediately + // remounts with a different comments prop. + const [commentsReady, setCommentsReady] = useState(false); const annotationValue = entity.metadata.annotations?.[ANNOTATION_KEY]; const selfEntityRef = useMemo(() => toEntityPath(getCompoundEntityRef(entity)), [entity]); @@ -21,15 +27,36 @@ export function RwEntityDocsViewer() { useEffect(() => { if (!parsed) return undefined; + // Reset gate immediately so the viewer unmounts while we fetch the new entity's data. + setApiBaseUrl(null); + setFetchError(null); + setCommentsReady(false); + setCommentClient(undefined); + let cancelled = false; - rwApi - .getSiteBaseUrl(parsed.entityPath) - .then((url) => { - if (!cancelled) setApiBaseUrl(url); - }) - .catch((err) => { - if (!cancelled) setFetchError(err); - }); + (async () => { + try { + const url = await rwApi.getSiteBaseUrl(parsed.entityPath); + if (cancelled) return; + setApiBaseUrl(url); + } catch (err) { + if (!cancelled) setFetchError(err instanceof Error ? err : new Error(String(err))); + return; + } + + try { + const enabled = await rwApi.getCommentsEnabled(); + if (cancelled) return; + setCommentClient(enabled ? rwApi.createCommentClient(parsed.entityRef) : undefined); + } catch (err) { + if (cancelled) return; + // eslint-disable-next-line no-console + console.warn("rw: comments-enabled probe failed; comments disabled for this view", err); + setCommentClient(undefined); + } + + if (!cancelled) setCommentsReady(true); + })(); return () => { cancelled = true; }; @@ -44,7 +71,7 @@ export function RwEntityDocsViewer() { return ; } - if (!apiBaseUrl) { + if (!apiBaseUrl || !commentsReady) { return ; } @@ -54,6 +81,7 @@ export function RwEntityDocsViewer() { apiBaseUrl={apiBaseUrl} sectionRef={sectionRef} sourceEntityRef={parsed.entityRef} + comments={commentClient} /> ); } diff --git a/plugins/search-backend-module-rw/package.json b/plugins/search-backend-module-rw/package.json index b9604fe..c8a6ec0 100644 --- a/plugins/search-backend-module-rw/package.json +++ b/plugins/search-backend-module-rw/package.json @@ -49,7 +49,7 @@ "@backstage/plugin-permission-common": "^0.9.6", "@backstage/plugin-search-common": "^1.2.16", "@rwdocs/backstage-plugin-rw-common": "workspace:^", - "@rwdocs/core": "^0.1.22" + "@rwdocs/core": "^0.1.27" }, "peerDependencies": { "@backstage/backend-plugin-api": "^1.0.0", diff --git a/yarn.lock b/yarn.lock index 35b8056..a60026a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7148,24 +7148,31 @@ __metadata: version: 0.0.0-use.local resolution: "@rwdocs/backstage-plugin-rw-backend@workspace:plugins/rw-backend" dependencies: + "@backstage/backend-defaults": "npm:^0.17.3" "@backstage/backend-plugin-api": "npm:^1.0.0" "@backstage/backend-test-utils": "npm:^1.11.0" "@backstage/catalog-model": "npm:^1.7.6" "@backstage/cli": "npm:^0.36.0" "@backstage/config": "npm:^1.3.6" "@backstage/errors": "npm:^1.2.7" + "@backstage/plugin-catalog-node": "npm:^2.2.2" + "@backstage/plugin-permission-common": "npm:^0.9.9" + "@backstage/plugin-permission-node": "npm:^0.11.1" "@backstage/types": "npm:^1.2.2" "@rwdocs/backstage-plugin-rw-common": "workspace:^" - "@rwdocs/core": "npm:^0.1.23" + "@rwdocs/core": "npm:^0.1.27" "@types/express": "npm:^4.17.0" "@types/jest": "npm:^30.0.0" "@types/supertest": "npm:^7.2.0" express: "npm:^4.21.0" express-promise-router: "npm:^4.1.0" jest: "npm:^30.2.0" + luxon: "npm:^3.7.2" prettier: "npm:^3.4.2" supertest: "npm:^7.2.2" typescript: "npm:^5.7.0" + uuid: "npm:^11.0.0" + zod: "npm:^3.25.76 || ^4.0.0" peerDependencies: "@backstage/backend-plugin-api": ^1.0.0 languageName: unknown @@ -7179,6 +7186,7 @@ __metadata: "@backstage/cli": "npm:^0.36.0" "@backstage/config": "npm:^1.3.6" "@backstage/errors": "npm:^1.2.7" + "@backstage/plugin-permission-common": "npm:^0.9.9" "@backstage/types": "npm:^1.2.2" "@types/jest": "npm:^30.0.0" jest: "npm:^30.2.0" @@ -7202,7 +7210,7 @@ __metadata: "@backstage/test-utils": "npm:^1.7.0" "@material-ui/icons": "npm:^4.11.3" "@rwdocs/backstage-plugin-rw-common": "workspace:^" - "@rwdocs/viewer": "npm:^0.1.23" + "@rwdocs/viewer": "npm:^0.1.27" "@testing-library/dom": "npm:^10.0.0" "@testing-library/jest-dom": "npm:^6.0.0" "@testing-library/react": "npm:^16.0.0" @@ -7243,7 +7251,7 @@ __metadata: "@backstage/plugin-search-backend-node": "npm:^1.3.10" "@backstage/plugin-search-common": "npm:^1.2.16" "@rwdocs/backstage-plugin-rw-common": "workspace:^" - "@rwdocs/core": "npm:^0.1.22" + "@rwdocs/core": "npm:^0.1.27" "@types/jest": "npm:^30.0.0" jest: "npm:^30.2.0" prettier: "npm:^3.4.2" @@ -7255,34 +7263,34 @@ __metadata: languageName: unknown linkType: soft -"@rwdocs/core-darwin-arm64@npm:0.1.25": - version: 0.1.25 - resolution: "@rwdocs/core-darwin-arm64@npm:0.1.25" +"@rwdocs/core-darwin-arm64@npm:0.1.27": + version: 0.1.27 + resolution: "@rwdocs/core-darwin-arm64@npm:0.1.27" conditions: os=darwin & cpu=arm64 languageName: node linkType: hard -"@rwdocs/core-linux-x64-gnu@npm:0.1.25": - version: 0.1.25 - resolution: "@rwdocs/core-linux-x64-gnu@npm:0.1.25" +"@rwdocs/core-linux-x64-gnu@npm:0.1.27": + version: 0.1.27 + resolution: "@rwdocs/core-linux-x64-gnu@npm:0.1.27" conditions: os=linux & cpu=x64 & libc=glibc languageName: node linkType: hard -"@rwdocs/core-linux-x64-musl@npm:0.1.25": - version: 0.1.25 - resolution: "@rwdocs/core-linux-x64-musl@npm:0.1.25" +"@rwdocs/core-linux-x64-musl@npm:0.1.27": + version: 0.1.27 + resolution: "@rwdocs/core-linux-x64-musl@npm:0.1.27" conditions: os=linux & cpu=x64 & libc=musl languageName: node linkType: hard -"@rwdocs/core@npm:^0.1.22, @rwdocs/core@npm:^0.1.23": - version: 0.1.25 - resolution: "@rwdocs/core@npm:0.1.25" +"@rwdocs/core@npm:^0.1.27": + version: 0.1.27 + resolution: "@rwdocs/core@npm:0.1.27" dependencies: - "@rwdocs/core-darwin-arm64": "npm:0.1.25" - "@rwdocs/core-linux-x64-gnu": "npm:0.1.25" - "@rwdocs/core-linux-x64-musl": "npm:0.1.25" + "@rwdocs/core-darwin-arm64": "npm:0.1.27" + "@rwdocs/core-linux-x64-gnu": "npm:0.1.27" + "@rwdocs/core-linux-x64-musl": "npm:0.1.27" dependenciesMeta: "@rwdocs/core-darwin-arm64": optional: true @@ -7290,19 +7298,19 @@ __metadata: optional: true "@rwdocs/core-linux-x64-musl": optional: true - checksum: 10c0/176bdd7a384f9d931bb3af04630823f9bc2d06209cf4438e528bc4df7692e75cef41fa5f67b0b56afa0c1dc1a636bf753ed0501cf418327436ad0e9c9117c66d + checksum: 10c0/9ba097c776841bd80be3a8056fdcf08ecdcf01570ac21009872c83057a28b4bbacc83cb30df28ecc7f9c34971fba0cb5c4a90da890efb403a94c1be553c1d73e languageName: node linkType: hard -"@rwdocs/viewer@npm:^0.1.23": - version: 0.1.25 - resolution: "@rwdocs/viewer@npm:0.1.25" +"@rwdocs/viewer@npm:^0.1.27": + version: 0.1.27 + resolution: "@rwdocs/viewer@npm:0.1.27" dependencies: "@fontsource/jetbrains-mono": "npm:^5.2.8" "@fontsource/roboto": "npm:^5.2.10" "@types/diff-match-patch": "npm:^1.0.36" diff-match-patch: "npm:^1.0.5" - checksum: 10c0/99f799ad3c2de2636c510fdab053d061a3154064195e8e95c8f2c629cf741a36e8b7dfeea8a17c1178d2981aa45222b3092dd4b7783e4fc76a2f5bd65b008dff + checksum: 10c0/f162d74b33e2866fd0a1462f52836a2be29eb89edbc6f183f00bbfe6fcdfbd8c184fd1da84cfb8792fe72013de011805781cd719fd3315e58b6816a72f087f0f languageName: node linkType: hard @@ -10029,7 +10037,11 @@ __metadata: resolution: "app@workspace:packages/app" dependencies: "@backstage/cli": "npm:^0.36.0" + "@backstage/core-components": "npm:^0.18.11" + "@backstage/core-plugin-api": "npm:^1.12.7" "@backstage/frontend-defaults": "npm:^0.5.0" + "@backstage/frontend-plugin-api": "npm:^0.17.2" + "@backstage/plugin-app-react": "npm:^0.2.4" "@backstage/plugin-catalog": "npm:^2.0.0" "@backstage/plugin-search": "npm:^1.7.0" "@backstage/plugin-user-settings": "npm:^0.9.0" @@ -10533,6 +10545,7 @@ __metadata: "@backstage/plugin-app-backend": "npm:^0.5.11" "@backstage/plugin-auth-backend": "npm:^0.29.0" "@backstage/plugin-auth-backend-module-guest-provider": "npm:^0.2.16" + "@backstage/plugin-auth-node": "npm:^0.7.2" "@backstage/plugin-catalog-backend": "npm:^3.4.0" "@backstage/plugin-catalog-backend-module-scaffolder-entity-model": "npm:^0.2.17" "@backstage/plugin-permission-backend": "npm:^0.7.9" @@ -18456,7 +18469,7 @@ __metadata: languageName: node linkType: hard -"luxon@npm:^3.0.0, luxon@npm:^3.2.1": +"luxon@npm:^3.0.0, luxon@npm:^3.2.1, luxon@npm:^3.7.2": version: 3.7.2 resolution: "luxon@npm:3.7.2" checksum: 10c0/ed8f0f637826c08c343a29dd478b00628be93bba6f068417b1d8896b61cb61c6deacbe1df1e057dbd9298334044afa150f9aaabbeb3181418ac8520acfdc2ae2 @@ -25761,6 +25774,15 @@ __metadata: languageName: node linkType: hard +"uuid@npm:^11.0.0": + version: 11.1.1 + resolution: "uuid@npm:11.1.1" + bin: + uuid: dist/esm/bin/uuid + checksum: 10c0/9e3af58eba872ece5a5e76f4773a94fc78a0ef2c2444c38dbe6b42f41dadf76c01850fd783604f27986f6195e6286aef064d45987d401b2a33127b98ddf7c0c5 + languageName: node + linkType: hard + "uuid@npm:^3.4.0": version: 3.4.0 resolution: "uuid@npm:3.4.0"