diff --git a/src/services/extensions/v2/README.md b/src/services/extensions/v2/README.md index 73acdab2..75895ff4 100644 --- a/src/services/extensions/v2/README.md +++ b/src/services/extensions/v2/README.md @@ -54,6 +54,7 @@ An extension exists from `POST /extensions`, not from approval. There is no sepa - `POST /extensions` creates the record and reserves its id. It stays out of public listings until its first revision is approved. - `PUT /extensions/{id}` proposes an edit. Published content is unchanged until approval. One unreviewed revision per extension at a time. +- `PUT /extensions/{id}/revisions/{revisionId}` lets the active owner replace that pending revision; see [Editing a pending submission](#editing-a-pending-submission). - `DELETE /extensions/{id}` withdraws an unpublished extension and releases its id. Published extensions cannot be withdrawn by owners. - `POST /extensions/{id}/revisions/{revisionId}/approve` publishes. `reject` leaves published content untouched. - `POST /extensions/{id}/delist` (moderator-only) hides a published extension for cause without deleting content or history. `POST /extensions/{id}/relist` restores it. Both accept `?notify=false`. @@ -61,6 +62,14 @@ An extension exists from `POST /extensions`, not from approval. There is no sepa An edit cannot rename an extension or move it to another developer. A user owns at most one developer profile, so no request body names one. +### Editing a pending submission + +`PUT /extensions/{id}/revisions/{revisionId}` replaces pending content using the same body as `PUT /extensions/{id}`. Only an active human owner can call it, including for publisher-created submissions. The replacement records that owner as `submitted_by` and counts toward their pending-revision and write limits. + +The response is `202` with a new `revision_id` and the extension's canonical `id` casing. The supplied revision must still be pending at the current ownership epoch. A stale or reviewed revision returns `409`; refresh before retrying. + +Deletion and insertion run in one transaction: a failed write keeps the original pending revision. The new ID prevents stale edits or moderator decisions from applying to the replacement. Published content and reviewed history stay unchanged. + ## Reads `GET /extensions/{id}` and `GET /developers/{id}` are role-aware: anonymous callers get the published projection; owners and moderators get the full object. For extensions, 404 hides drafts and delisted rows. diff --git a/src/services/extensions/v2/db/revisions.ts b/src/services/extensions/v2/db/revisions.ts index 0fe75f04..bf9af00c 100644 --- a/src/services/extensions/v2/db/revisions.ts +++ b/src/services/extensions/v2/db/revisions.ts @@ -181,6 +181,85 @@ export class ExtensionRevisionsDatabase { return { data: { id }, error: null }; } + // Owners can replace publisher submissions; attribute the new row to the owner. + // A fresh ID invalidates stale edits and reviews. Both writes must be atomic + // so admission failures keep the original pending revision. + async replacePending(input: { + extensionId: string; + revisionId: string; + callerId: string; + content: ExtensionContent; + }): Promise> { + const id = crypto.randomUUID(); + try { + const results = await this.db.$client.batch<{ extensionId: string }>([ + this.db.$client + .prepare( + ` + DELETE FROM extension_revisions + WHERE id = ? AND status = 'pending' + AND EXISTS ( + SELECT 1 FROM extensions e JOIN developers d ON d.id = e.developer_id + JOIN users u ON u.id = d.owner_user_id + WHERE LOWER(e.id) = LOWER(?) AND e.id = extension_revisions.extension_id + AND d.id = extension_revisions.developer_id + AND d.ownership_epoch = extension_revisions.ownership_epoch + AND d.owner_user_id = ? AND u.deleted_at IS NULL + ) + AND (submitted_by = ? OR ( + SELECT COUNT(*) FROM extension_revisions WHERE submitted_by = ? AND status = 'pending' + ) < ?) + ` + ) + .bind( + input.revisionId, + input.extensionId, + input.callerId, + input.callerId, + input.callerId, + MAX_PENDING_REVISIONS_PER_USER + ), + this.db.$client + .prepare( + ` + INSERT INTO extension_revisions + (id, extension_id, developer_id, submitted_by, status, content, ownership_epoch) + SELECT ?, e.id, d.id, ?, 'pending', ?, d.ownership_epoch + FROM extensions e JOIN developers d ON d.id = e.developer_id + WHERE LOWER(e.id) = LOWER(?) AND changes() = 1 + RETURNING extension_id AS extensionId + ` + ) + .bind( + id, + input.callerId, + JSON.stringify(input.content), + input.extensionId + ) + ]); + const [replacement] = results[1].results; + if (replacement) + return { + data: { id, extensionId: replacement.extensionId }, + error: null + }; + const error = await this.proposeBlockedError(input); + return { + data: null, + error: + error.code === "CONFLICT" + ? { + code: "CONFLICT", + message: + "The pending submission changed, was reviewed, or the pending-revision limit was reached. Reload before editing again." + } + : error + }; + } catch (error) { + return contentAdmissionError("replacePending", error); + } + } + // Distinguishes the three ways the guard above can reject a write, so the // route can map them to 404/403/409 rather than one opaque conflict. private async proposeBlockedError(input: { diff --git a/src/services/extensions/v2/index.ts b/src/services/extensions/v2/index.ts index 6b575248..ef563fac 100644 --- a/src/services/extensions/v2/index.ts +++ b/src/services/extensions/v2/index.ts @@ -57,7 +57,7 @@ extensionsV2.on( ); extensionsV2.on( "PUT", - "/extensions/:id", + ["/extensions/:id", "/extensions/:id/revisions/:revisionId"], paceContentIp(), boundContentRequest() ); diff --git a/src/services/extensions/v2/routes/owner-extensions.ts b/src/services/extensions/v2/routes/owner-extensions.ts index 483ec02a..3421e00a 100644 --- a/src/services/extensions/v2/routes/owner-extensions.ts +++ b/src/services/extensions/v2/routes/owner-extensions.ts @@ -273,6 +273,79 @@ export function registerOwnerExtensionsRoutes(app: ExtensionsV2App): void { ); }); + const replacePendingRoute = createRoute({ + method: "put", + path: "/extensions/{id}/revisions/{revisionId}", + tags: ["Extensions"], + summary: "Replace a pending submission for an extension the caller owns", + description: + "The active owner can replace publisher-created submissions. The replacement records the owner as submitter.", + security: [{ Bearer: [] }], + middleware: [requireActiveAuth(), paceContentAccount()] as const, + request: { + params: RevisionIdParamSchema, + body: { + content: { "application/json": { schema: ExtensionUpdateSchema } } + } + }, + responses: { + 202: { + content: { "application/json": { schema: AcceptedRevisionSchema } }, + description: + "Replaced with a new pending revision ID. Published content and reviewed history are unchanged." + }, + ...authErrorResponses(), + 403: { + ...ActiveAccountRequiredResponse, + description: + "The account is inactive, or the caller does not own this extension" + }, + 404: errorResponse("No extension with that id"), + 409: errorResponse( + "The pending revision changed, was reviewed, ownership changed, or the pending-revision limit was reached" + ), + 413: errorResponse("Raw request exceeds 512 KiB"), + 429: errorResponse( + "Extension write allowance exhausted; see Retry-After" + ), + 503: errorResponse("Write admission unavailable"), + 422: errorResponse("Body failed validation"), + 500: errorResponse("Database error") + } + }); + + app.openapi(replacePendingRoute, async (c) => { + const auth = getAuth(c); + const { id, revisionId } = c.req.valid("param"); + const content = c.req.valid("json"); + const db = new ExtensionRevisionsDatabase( + getExtensionsDb(c.env.DB_EXTENSIONS) + ); + const { data, error } = await db.replacePending({ + extensionId: id, + revisionId, + callerId: auth.userId, + content + }); + if (error || !data) { + setContentRetryAfter(c, error?.code); + return c.json( + errorBody(error, "Unable to submit edit"), + statusFromContentWriteError(error?.code) + ); + } + return c.json( + { + result: { + id: data.extensionId, + revision_id: data.id, + status: "pending" as const + } + }, + 202 + ); + }); + const withdrawRoute = createRoute({ method: "delete", path: "/extensions/{id}", diff --git a/test/services/extensions/v2/pending-edits.test.ts b/test/services/extensions/v2/pending-edits.test.ts new file mode 100644 index 00000000..41c53f6c --- /dev/null +++ b/test/services/extensions/v2/pending-edits.test.ts @@ -0,0 +1,247 @@ +import { describe, it, expect, vi } from "vitest"; +import { env } from "cloudflare:workers"; +import { wrapD1WithHook } from "./db-interceptor"; +import { + setupExtensionsV2Tests, + db, + authHeaders, + post, + put, + sampleCreate, + sampleContent, + seedDeveloper +} from "./harness"; +import { + getRevision, + getExtension, + listRevisions, + insertUser, + bumpDeveloperOwnership, + insertRevision, + insertUnpublishedExtension +} from "./db-fixtures"; +vi.mock("@octokit/request", async () => + (await import("../../../mocks/octokit")).octokitRequestMock() +); +setupExtensionsV2Tests(); +const root = "/extensions/v2/extensions/new-ext"; +async function pending() { + await seedDeveloper("dev", "owner"); + const response = await post( + "/extensions/v2/extensions", + await authHeaders("owner"), + sampleCreate() + ); + expect(response.status).toBe(201); + return ((await response.json()) as { result: { revision_id: string } }).result + .revision_id; +} +async function replace(id: string, user = "owner") { + return put( + `${root}/revisions/${id}`, + await authHeaders(user), + sampleContent({ name: "Updated" }) + ); +} +async function moderator() { + await insertUser(db, { id: "moderator", is_moderator: 1 }); + return authHeaders("moderator"); +} +describe("pending submission replacement", () => { + it("rotates pending identity without publishing and rejects stale edits and decisions", async () => { + const old = await pending(); + const response = await replace(old); + expect(response.status).toBe(202); + const { result } = (await response.json()) as { + result: { revision_id: string }; + }; + expect(result.revision_id).not.toBe(old); + expect(await getRevision(db, old)).toBeNull(); + expect( + JSON.parse((await getRevision(db, result.revision_id))!.content!) + ).toMatchObject({ name: "Updated" }); + expect((await getExtension(db, "new-ext"))!.published_at).toBeNull(); + expect((await replace(old)).status).toBe(409); + for (const action of ["approve", "reject"]) { + const stale = await post( + `${root}/revisions/${old}/${action}`, + await moderator(), + action === "reject" ? { review_note: "Requested changes" } : {} + ); + expect(stale.status).toBe(404); + } + expect(await listRevisions(db)).toHaveLength(1); + }); + it("lets the owner replace a publisher submission and attributes the replacement to the owner", async () => { + await seedDeveloper("dev", "owner"); + await insertUnpublishedExtension(db, { + id: "new-ext", + developer_id: "dev" + }); + await insertRevision(db, { + id: "publisher-revision", + extension_id: "new-ext", + developer_id: "dev", + submitted_by: "publisher-client", + content: JSON.stringify(sampleContent()) + }); + expect( + (await replace("publisher-revision", "publisher-client")).status + ).toBe(403); + expect((await getRevision(db, "publisher-revision"))!.submitted_by).toBe( + "publisher-client" + ); + const response = await replace("publisher-revision"); + expect(response.status).toBe(202); + const { result } = (await response.json()) as { + result: { revision_id: string }; + }; + expect((await getRevision(db, result.revision_id))!.submitted_by).toBe( + "owner" + ); + expect(await getRevision(db, "publisher-revision")).toBeNull(); + expect(await listRevisions(db)).toHaveLength(1); + }); + it.each(["new-ext", "New-Ext"])( + "returns canonical stored extension casing for %s", + async (extensionId) => { + await seedDeveloper("dev", "owner"); + await insertUnpublishedExtension(db, { + id: extensionId, + developer_id: "dev" + }); + await insertRevision(db, { + id: "canonical-revision", + extension_id: extensionId, + developer_id: "dev", + submitted_by: "owner", + content: JSON.stringify(sampleContent()) + }); + const response = await put( + "/extensions/v2/extensions/NEW-EXT/revisions/canonical-revision", + await authHeaders("owner"), + sampleContent({ name: "Updated" }) + ); + expect(response.status).toBe(202); + const { result } = (await response.json()) as { + result: { id: string; revision_id: string }; + }; + expect(result.id).toBe(extensionId); + expect((await getRevision(db, result.revision_id))!.extension_id).toBe( + result.id + ); + } + ); + it("preserves the published version and reviewed history", async () => { + const approved = await pending(); + expect( + ( + await post( + `${root}/revisions/${approved}/approve`, + await moderator(), + {} + ) + ).status + ).toBe(200); + const next = await put( + root, + await authHeaders("owner"), + sampleContent({ name: "Proposed" }) + ); + const { result } = (await next.json()) as { + result: { revision_id: string }; + }; + expect((await replace(result.revision_id)).status).toBe(202); + expect((await getExtension(db, "new-ext"))!.name).toBe("New Extension"); + expect((await getRevision(db, approved))!.status).toBe("approved"); + expect(await listRevisions(db)).toHaveLength(2); + }); + it("rejects a review that won the race without creating another submission", async () => { + const old = await pending(); + const headers = await moderator(); + let fired = false; + env.DB_EXTENSIONS = wrapD1WithHook(db, async (sql) => { + if (!fired && sql.includes("DELETE FROM extension_revisions")) { + fired = true; + // The competing request uses the real binding, then restore the hook. + const hooked = env.DB_EXTENSIONS; + env.DB_EXTENSIONS = db; + expect( + (await post(`${root}/revisions/${old}/approve`, headers, {})).status + ).toBe(200); + env.DB_EXTENSIONS = hooked; + } + }); + expect((await replace(old)).status).toBe(409); + expect((await getRevision(db, old))!.status).toBe("approved"); + expect(await listRevisions(db)).toHaveLength(1); + }); + it("rolls back deletion if the new revision cannot be inserted", async () => { + const old = await pending(); + env.DB_EXTENSIONS = wrapD1WithHook(db, (sql) => { + if (sql.includes("INSERT INTO extension_revisions")) + return db.prepare("INSERT INTO extension_revisions (id) VALUES (NULL)"); + }); + expect((await replace(old)).status).toBe(503); + expect((await getRevision(db, old))!.status).toBe("pending"); + expect(await listRevisions(db)).toHaveLength(1); + }); + it("enforces owner, active account, and ownership epoch checks", async () => { + const old = await pending(); + expect((await replace(old, "other")).status).toBe(403); + await bumpDeveloperOwnership(db, "dev", "owner"); + expect((await replace(old)).status).toBe(409); + await db + .prepare( + "UPDATE users SET deleted_at = CURRENT_TIMESTAMP WHERE id = 'owner'" + ) + .run(); + expect((await replace(old)).status).toBe(403); + expect((await getRevision(db, old))!.status).toBe("pending"); + }); + it("does not publish approval content prepared before a replacement", async () => { + const old = await pending(); + const headers = await moderator(); + let fired = false; + env.DB_EXTENSIONS = wrapD1WithHook(db, async (sql) => { + if (!fired && sql.includes("SET status = 'approved'")) { + fired = true; + const hooked = env.DB_EXTENSIONS; + env.DB_EXTENSIONS = db; + expect((await replace(old)).status).toBe(202); + env.DB_EXTENSIONS = hooked; + } + }); + expect( + (await post(`${root}/revisions/${old}/approve`, headers, {})).status + ).toBe(409); + expect(fired).toBe(true); + expect((await getExtension(db, "new-ext"))!.published_at).toBeNull(); + expect((await listRevisions(db))[0].status).toBe("pending"); + }); + it("keeps the pending revision when rate admission rejects a replacement", async () => { + let current = await pending(); + for (let i = 0; i < 4; i++) { + const response = await replace(current); + expect(response.status).toBe(202); + current = ((await response.json()) as { result: { revision_id: string } }) + .result.revision_id; + } + const response = await replace(current); + expect(response.status).toBe(429); + expect(response.headers.get("Retry-After")).toBeTruthy(); + expect((await getRevision(db, current))!.status).toBe("pending"); + expect(await listRevisions(db)).toHaveLength(1); + }); + it("validates the replacement before changing the pending submission", async () => { + const old = await pending(); + expect( + ( + await put(`${root}/revisions/${old}`, await authHeaders("owner"), { + name: "Invalid" + }) + ).status + ).toBe(422); + expect((await getRevision(db, old))!.status).toBe("pending"); + }); +});