From 0a69362f25f08f39f28c1b642a74853dbdf90177 Mon Sep 17 00:00:00 2001 From: SamTV12345 <40429738+samtv12345@users.noreply.github.com> Date: Fri, 12 Jun 2026 22:20:48 +0200 Subject: [PATCH] feat(parity): real getHTML export, padCopy/padRemove hooks, public-status guard, listAuthors filter Next parity round vs the original etherpad: getHTML: - the API getHTML returned fake HTML (it string-replaced newlines with
and dropped all formatting). It now renders through the existing HTML exporter (GetPadHTMLDocument), preserving bold/italic/underline, links, lists, headings and line markers, mirroring upstream getHTML which delegates to exportHtml.getPadHTMLDocument. padCopy / padRemove plugin hooks: - ported the two missing pad-lifecycle hooks. padRemove fires from Manager.RemovePad (the single removal choke point: deletePad, movePad source removal and force-overwrite of a copy destination), capturing the loaded pad before deletion. padCopy fires from copyPad, copyPadWithoutHistory and movePad with the source and destination pads. public-status group guard: - getPublicStatus/setPublicStatus now reject non-group pads (id without a $), mirroring the original checkGroupPad guard, instead of silently operating on pads that can never be public. listAuthorsOfPad: - filter out the synthetic SystemAuthorId (a.etherpad-system), used for unattributed inserts, so it no longer surfaces as a real contributor through the public API (matches upstream's Pad.SYSTEM_AUTHOR_ID filter). Tests: padCopy/padRemove hook firing, public-status non-group rejection, system-author exclusion; existing public-status tests updated to use valid group pad ids. Co-Authored-By: Claude Fable 5 --- lib/api/errors/httpErrors.go | 7 ++ lib/api/pad/copyMove.go | 46 +++++++++++- lib/api/pad/operations.go | 30 ++++---- lib/hooks/HookConstants.go | 2 + lib/models/pad/PadDefaultContent.go | 16 ++++ lib/pad/padManager.go | 10 +++ lib/test/api/pad/pad_api_test.go | 112 ++++++++++++++++++++++++++-- 7 files changed, 198 insertions(+), 25 deletions(-) diff --git a/lib/api/errors/httpErrors.go b/lib/api/errors/httpErrors.go index c1c0a0ae..fef1f8fa 100644 --- a/lib/api/errors/httpErrors.go +++ b/lib/api/errors/httpErrors.go @@ -39,6 +39,13 @@ var PadNotFoundError = Error{ Error: 404, } +// NotAGroupPadError mirrors the original Etherpad checkGroupPad guard: public +// status can only be read or changed on pads that belong to a group. +var NotAGroupPadError = Error{ + Message: "You can only get/set the publicStatus of pads that belong to a group", + Error: 400, +} + var AuthorNotFoundError = Error{ Message: "Author not found", Error: 404, diff --git a/lib/api/pad/copyMove.go b/lib/api/pad/copyMove.go index b834032e..a44110e8 100644 --- a/lib/api/pad/copyMove.go +++ b/lib/api/pad/copyMove.go @@ -6,7 +6,9 @@ import ( "github.com/ether/etherpad-go/lib" errors2 "github.com/ether/etherpad-go/lib/api/errors" utils2 "github.com/ether/etherpad-go/lib/api/utils" + "github.com/ether/etherpad-go/lib/hooks" db2 "github.com/ether/etherpad-go/lib/models/db" + padModel "github.com/ether/etherpad-go/lib/models/pad" "github.com/gofiber/fiber/v3" ) @@ -106,6 +108,28 @@ func copyPadRecords(initStore *lib.InitStore, sourceID string, destinationID str return nil } +// firePadCopy notifies plugins that a pad was copied, mirroring the original +// Etherpad padCopy hook which is fired with the source and destination pads. +func firePadCopy(initStore *lib.InitStore, srcPad *padModel.Pad, dstId string) { + dstPad, err := initStore.PadManager.GetPad(dstId, nil, nil) + if err != nil { + initStore.Logger.Errorf("padCopy hook: failed to load destination pad %s: %v", dstId, err) + return + } + initStore.Hooks.ExecuteHooks(hooks.PadCopyString, padModel.Copy{ + SrcPad: srcPad, + DstPad: dstPad, + SrcId: srcPad.Id, + DstId: dstId, + }) +} + +// isGroupPad mirrors the original Etherpad checkGroupPad guard: only pads that +// belong to a group (their id contains a '$') expose a public status. +func isGroupPad(padId string) bool { + return strings.Contains(padId, "$") +} + // CopyPad godoc // @Summary Copy a pad // @Description Copies a pad including its full revision and chat history to a new pad. Fails if the destination exists unless force is set. @@ -130,7 +154,8 @@ func CopyPad(initStore *lib.InitStore) fiber.Handler { } // Verify source pad exists - if _, err := utils2.GetPadSafe(padId, true, nil, nil, initStore.PadManager); err != nil { + srcPad, err := utils2.GetPadSafe(padId, true, nil, nil, initStore.PadManager) + if err != nil { return c.Status(404).JSON(errors2.PadNotFoundError) } @@ -143,6 +168,8 @@ func CopyPad(initStore *lib.InitStore) fiber.Handler { return c.Status(500).JSON(errors2.InternalServerError) } + firePadCopy(initStore, srcPad, request.DestinationID) + return c.JSON(PadIDResponse{ PadID: request.DestinationID, }) @@ -195,6 +222,8 @@ func CopyPadWithoutHistory(initStore *lib.InitStore) fiber.Handler { return c.Status(500).JSON(errors2.InternalServerError) } + firePadCopy(initStore, sourcePad, request.DestinationID) + return c.JSON(PadIDResponse{ PadID: request.DestinationID, }) @@ -225,7 +254,8 @@ func MovePad(initStore *lib.InitStore) fiber.Handler { } // Verify source pad exists - if _, err := utils2.GetPadSafe(padId, true, nil, nil, initStore.PadManager); err != nil { + srcPad, err := utils2.GetPadSafe(padId, true, nil, nil, initStore.PadManager) + if err != nil { return c.Status(404).JSON(errors2.PadNotFoundError) } @@ -238,7 +268,9 @@ func MovePad(initStore *lib.InitStore) fiber.Handler { return c.Status(500).JSON(errors2.InternalServerError) } - // Remove the source pad after a successful copy + firePadCopy(initStore, srcPad, request.DestinationID) + + // Remove the source pad after a successful copy (fires padRemove) if err := initStore.PadManager.RemovePad(padId); err != nil { return c.Status(500).JSON(errors2.InternalServerError) } @@ -264,6 +296,10 @@ func GetPublicStatus(initStore *lib.InitStore) fiber.Handler { return func(c fiber.Ctx) error { padId := c.Params("padId") + if !isGroupPad(padId) { + return c.Status(400).JSON(errors2.NotAGroupPadError) + } + // Get the pad pad, err := utils2.GetPadSafe(padId, true, nil, nil, initStore.PadManager) if err != nil { @@ -298,6 +334,10 @@ func SetPublicStatus(initStore *lib.InitStore) fiber.Handler { return c.Status(400).JSON(errors2.InvalidRequestError) } + if !isGroupPad(padId) { + return c.Status(400).JSON(errors2.NotAGroupPadError) + } + // Get the pad pad, err := utils2.GetPadSafe(padId, true, nil, nil, initStore.PadManager) if err != nil { diff --git a/lib/api/pad/operations.go b/lib/api/pad/operations.go index e38c7f45..7300ad3a 100644 --- a/lib/api/pad/operations.go +++ b/lib/api/pad/operations.go @@ -9,6 +9,8 @@ import ( utils2 "github.com/ether/etherpad-go/lib/api/utils" "github.com/ether/etherpad-go/lib/apool" "github.com/ether/etherpad-go/lib/changeset" + io2 "github.com/ether/etherpad-go/lib/io" + padModel "github.com/ether/etherpad-go/lib/models/pad" "github.com/ether/etherpad-go/lib/utils" "github.com/gofiber/fiber/v3" ) @@ -253,10 +255,13 @@ func ListAuthorsOfPad(initStore *lib.InitStore) fiber.Handler { return c.Status(404).JSON(errors2.PadNotFoundError) } - // Get all authors from the pool + // Get all authors from the pool. SystemAuthorId is the synthetic author + // used for unattributed inserts (HTTP API writes without an authorId, + // server-side imports); it is a changeset-bookkeeping detail, not a real + // contributor, so it must not surface through this public API. authorIDs := make([]string, 0) pad.Pool.EachAttrib(func(attr apool.Attribute) { - if attr.Key == "author" && attr.Value != "" { + if attr.Key == "author" && attr.Value != "" && attr.Value != padModel.SystemAuthorId { // Check if not already in list found := false for _, id := range authorIDs { @@ -432,21 +437,16 @@ func GetHTML(initStore *lib.InitStore) fiber.Handler { rev = revNum } - // Get the HTML (using exporter if available) - var text string - if rev != nil { - atext := pad.GetInternalRevisionAText(*rev) - if atext == nil { - return c.Status(500).JSON(errors2.InternalApiError) - } - text = atext.Text - } else { - text = pad.Text() + // Render the full HTML document through the real exporter so that + // formatting (bold/italic/underline, links, lists, headings, line + // markers) is preserved. Mirrors the original Etherpad getHTML, which + // delegates to exportHtml.getPadHTMLDocument. + exporter := io2.NewExportHtml(initStore.PadManager, initStore.AuthorManager, initStore.Hooks) + html, err := exporter.GetPadHTMLDocument(padId, rev, nil) + if err != nil { + return c.Status(500).JSON(errors2.InternalApiError) } - // Simple HTML conversion (basic) - html := "" + strings.ReplaceAll(text, "\n", "
") + "" - return c.JSON(fiber.Map{ "html": html, }) diff --git a/lib/hooks/HookConstants.go b/lib/hooks/HookConstants.go index 0c8879fe..f8cedd06 100644 --- a/lib/hooks/HookConstants.go +++ b/lib/hooks/HookConstants.go @@ -6,3 +6,5 @@ const PreAuthzFailureString = "preAuthzFailure" const PadLoadString = "padLoad" const PadCreateString = "padCreate" const PadUpdateString = "padUpdate" +const PadCopyString = "padCopy" +const PadRemoveString = "padRemove" diff --git a/lib/models/pad/PadDefaultContent.go b/lib/models/pad/PadDefaultContent.go index d09e12d1..b54bd735 100644 --- a/lib/models/pad/PadDefaultContent.go +++ b/lib/models/pad/PadDefaultContent.go @@ -29,3 +29,19 @@ type Create struct { PadId string AuthorId string } + +// Copy is the context passed to the padCopy hook after a pad is copied to a new +// destination (copyPad, copyPadWithoutHistory, movePad). Mirrors the original +// Etherpad context, which exposes the source and destination pads. +type Copy struct { + SrcPad *Pad + DstPad *Pad + SrcId string + DstId string +} + +// Remove is the context passed to the padRemove hook when a pad is deleted. +type Remove struct { + Pad *Pad + PadId string +} diff --git a/lib/pad/padManager.go b/lib/pad/padManager.go index 9ac68f80..520f63ca 100644 --- a/lib/pad/padManager.go +++ b/lib/pad/padManager.go @@ -128,12 +128,22 @@ func (m *Manager) SanitizePadId(padID string) (*string, error) { } func (m *Manager) RemovePad(padID string) error { + // Capture the loaded pad (if any) before deletion so the padRemove hook can + // hand listeners the pad context, mirroring the original Etherpad which + // fires padRemove from Pad.remove() with `this`. + removedPad := m.globalPadCache.GetPad(padID) + if err := m.store.RemovePad(padID); err != nil { return err } m.globalPadCache.DeletePad(padID) m.padList.RemovePad(padID) + m.hook.ExecuteHooks(hooks.PadRemoveString, pad.Remove{ + Pad: removedPad, + PadId: padID, + }) + return nil } diff --git a/lib/test/api/pad/pad_api_test.go b/lib/test/api/pad/pad_api_test.go index 2816a082..c4c87f51 100644 --- a/lib/test/api/pad/pad_api_test.go +++ b/lib/test/api/pad/pad_api_test.go @@ -8,6 +8,8 @@ import ( "testing" "github.com/ether/etherpad-go/lib/api/pad" + "github.com/ether/etherpad-go/lib/hooks" + padModel "github.com/ether/etherpad-go/lib/models/pad" "github.com/ether/etherpad-go/lib/test/testutils" "github.com/stretchr/testify/assert" ) @@ -195,6 +197,18 @@ func TestPadAPI(t *testing.T) { Name: "GetPublicStatus pad not found returns 404", Test: testGetPublicStatusNotFound, }, + testutils.TestRunConfig{ + Name: "PublicStatus rejects non-group pads", + Test: testPublicStatusRejectsNonGroupPad, + }, + testutils.TestRunConfig{ + Name: "padCopy hook fires on copy", + Test: testPadCopyHookFires, + }, + testutils.TestRunConfig{ + Name: "padRemove hook fires on remove", + Test: testPadRemoveHookFires, + }, ) defer testDb.StartTestDBHandler() @@ -542,6 +556,8 @@ func testListAuthorsOfPad(t *testing.T, tsStore testutils.TestDataStore) { initStore := tsStore.ToInitStore() pad.Init(initStore) + // Created without an authorId, so the only contributor in the pool is the + // synthetic system author, which must be filtered out of the public list. text := "Author test\n" createTestPad(t, tsStore, "authorpad", text) @@ -556,6 +572,7 @@ func testListAuthorsOfPad(t *testing.T, tsStore testutils.TestDataStore) { _ = json.Unmarshal(body, &response) assert.NotNil(t, response.AuthorIDs) + assert.NotContains(t, response.AuthorIDs, padModel.SystemAuthorId) } // ========== Last Edited ========== @@ -955,9 +972,9 @@ func testGetPublicStatusDefault(t *testing.T, tsStore testutils.TestDataStore) { initStore := tsStore.ToInitStore() pad.Init(initStore) - createTestPad(t, tsStore, "publicpad", "Public status test\n") + createTestPad(t, tsStore, "g.0123456789abcdef$publicpad", "Public status test\n") - req := httptest.NewRequest("GET", "/admin/api/pads/publicpad/publicStatus", nil) + req := httptest.NewRequest("GET", "/admin/api/pads/g.0123456789abcdef$publicpad/publicStatus", nil) resp, err := initStore.C.Test(req) assert.NoError(t, err) @@ -974,14 +991,14 @@ func testSetPublicStatusPersists(t *testing.T, tsStore testutils.TestDataStore) initStore := tsStore.ToInitStore() pad.Init(initStore) - createTestPad(t, tsStore, "publicpad2", "Public status test\n") + createTestPad(t, tsStore, "g.0123456789abcdef$publicpad2", "Public status test\n") reqBody := pad.PublicStatusRequest{ PublicStatus: true, } body, _ := json.Marshal(reqBody) - req := httptest.NewRequest("POST", "/admin/api/pads/publicpad2/publicStatus", bytes.NewBuffer(body)) + req := httptest.NewRequest("POST", "/admin/api/pads/g.0123456789abcdef$publicpad2/publicStatus", bytes.NewBuffer(body)) req.Header.Set("Content-Type", "application/json") resp, err := initStore.C.Test(req) @@ -989,9 +1006,9 @@ func testSetPublicStatusPersists(t *testing.T, tsStore testutils.TestDataStore) assert.Equal(t, 200, resp.StatusCode) // Evict the pad from the manager cache so the next read comes from the database - tsStore.PadManager.UnloadPad("publicpad2") + tsStore.PadManager.UnloadPad("g.0123456789abcdef$publicpad2") - req = httptest.NewRequest("GET", "/admin/api/pads/publicpad2/publicStatus", nil) + req = httptest.NewRequest("GET", "/admin/api/pads/g.0123456789abcdef$publicpad2/publicStatus", nil) resp, err = initStore.C.Test(req) assert.NoError(t, err) @@ -1008,13 +1025,94 @@ func testGetPublicStatusNotFound(t *testing.T, tsStore testutils.TestDataStore) initStore := tsStore.ToInitStore() pad.Init(initStore) - req := httptest.NewRequest("GET", "/admin/api/pads/nosuchpublicpad/publicStatus", nil) + req := httptest.NewRequest("GET", "/admin/api/pads/g.0123456789abcdef$nosuch/publicStatus", nil) resp, err := initStore.C.Test(req) assert.NoError(t, err) assert.Equal(t, 404, resp.StatusCode) } +// testPublicStatusRejectsNonGroupPad asserts the upstream checkGroupPad guard: +// reading or setting the public status of a non-group pad must be rejected. +func testPublicStatusRejectsNonGroupPad(t *testing.T, tsStore testutils.TestDataStore) { + initStore := tsStore.ToInitStore() + pad.Init(initStore) + + createTestPad(t, tsStore, "plainpad", "Not a group pad\n") + + getReq := httptest.NewRequest("GET", "/admin/api/pads/plainpad/publicStatus", nil) + getResp, err := initStore.C.Test(getReq) + assert.NoError(t, err) + assert.Equal(t, 400, getResp.StatusCode) + + body, _ := json.Marshal(pad.PublicStatusRequest{PublicStatus: true}) + setReq := httptest.NewRequest("POST", "/admin/api/pads/plainpad/publicStatus", bytes.NewBuffer(body)) + setReq.Header.Set("Content-Type", "application/json") + setResp, err := initStore.C.Test(setReq) + assert.NoError(t, err) + assert.Equal(t, 400, setResp.StatusCode) +} + +// ========== Plugin hooks (padCopy / padRemove) ========== + +// testPadCopyHookFires asserts the padCopy hook fires with the source and +// destination pads when a pad is copied through the API. +func testPadCopyHookFires(t *testing.T, tsStore testutils.TestDataStore) { + initStore := tsStore.ToInitStore() + pad.Init(initStore) + + createTestPad(t, tsStore, "copyhooksrc", "copy hook source\n") + + var fired *padModel.Copy + tsStore.Hooks.EnqueueHook(hooks.PadCopyString, func(ctx any) { + if c, ok := ctx.(padModel.Copy); ok { + fired = &c + } + }) + + body, _ := json.Marshal(pad.CopyPadRequest{DestinationID: "copyhookdst"}) + req := httptest.NewRequest("POST", "/admin/api/pads/copyhooksrc/copy", bytes.NewBuffer(body)) + req.Header.Set("Content-Type", "application/json") + resp, err := initStore.C.Test(req) + assert.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode) + + assert.NotNil(t, fired) + if fired != nil { + assert.Equal(t, "copyhooksrc", fired.SrcId) + assert.Equal(t, "copyhookdst", fired.DstId) + assert.NotNil(t, fired.DstPad) + } +} + +// testPadRemoveHookFires asserts the padRemove hook fires when a pad is removed +// (here through a move, which deletes the source after copying it). +func testPadRemoveHookFires(t *testing.T, tsStore testutils.TestDataStore) { + initStore := tsStore.ToInitStore() + pad.Init(initStore) + + createTestPad(t, tsStore, "removehooksrc", "remove hook source\n") + + var fired *padModel.Remove + tsStore.Hooks.EnqueueHook(hooks.PadRemoveString, func(ctx any) { + if c, ok := ctx.(padModel.Remove); ok { + fired = &c + } + }) + + body, _ := json.Marshal(pad.MovePadRequest{DestinationID: "removehookdst"}) + req := httptest.NewRequest("POST", "/admin/api/pads/removehooksrc/move", bytes.NewBuffer(body)) + req.Header.Set("Content-Type", "application/json") + resp, err := initStore.C.Test(req) + assert.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode) + + assert.NotNil(t, fired) + if fired != nil { + assert.Equal(t, "removehooksrc", fired.PadId) + } +} + // ========== Check Token ========== func testCheckToken(t *testing.T, tsStore testutils.TestDataStore) {