From 04a34c975740f7b0e5fee17c16ed15c11fba9acb Mon Sep 17 00:00:00 2001 From: Sacha Al Himdani Date: Thu, 11 Jun 2026 10:40:33 +0200 Subject: [PATCH] Require explicit approver_ids when publishing a document major version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The publish flow ignored a document's stored default approvers and only requested approval when approver_ids were passed in the call, so a major publish with no approver_ids silently published directly without routing through the approval flow — there was no way to tell "caller forgot approvers" (null) from "caller wants no approval" (empty). Make approver_ids an explicit choice, enforced once in the service so it covers every caller (console, MCP, n8n): - major publish: approver_ids must be set; an empty list publishes directly, a non-empty list requests approval. - minor publish: approver_ids must be omitted (approvers are ignored). Validate this in PublishDocumentRequest.Validate(), update the console publish dialog and the n8n publish node to honour the contract, document it in the MCP tool spec, and cover it with e2e tests. Signed-off-by: Sacha Al Himdani --- .../documents/_components/PublishDialog.tsx | 2 +- e2e/console/document_version_test.go | 136 +++++++++++++++++- .../actions/document/publish.operation.ts | 10 +- pkg/probo/document_service.go | 21 +++ pkg/server/api/mcp/v1/specification.yaml | 4 +- 5 files changed, 160 insertions(+), 13 deletions(-) diff --git a/apps/console/src/pages/organizations/documents/_components/PublishDialog.tsx b/apps/console/src/pages/organizations/documents/_components/PublishDialog.tsx index 1b979954a..7ba319f9a 100644 --- a/apps/console/src/pages/organizations/documents/_components/PublishDialog.tsx +++ b/apps/console/src/pages/organizations/documents/_components/PublishDialog.tsx @@ -153,7 +153,7 @@ export function PublishDialog({ input: { documentId, minor, - approverIds: minor ? [] : data.approverIds, + approverIds: minor ? null : data.approverIds, changelog: data.changelog, }, }, diff --git a/e2e/console/document_version_test.go b/e2e/console/document_version_test.go index f8685e47b..22764a061 100644 --- a/e2e/console/document_version_test.go +++ b/e2e/console/document_version_test.go @@ -190,9 +190,10 @@ func publishMajorDocumentVersion(t *testing.T, owner *testutil.Client, docID str } `, map[string]any{ "input": map[string]any{ - "minor": false, - "documentId": docID, - "changelog": "Major release", + "minor": false, + "documentId": docID, + "approverIds": []string{}, + "changelog": "Major release", }, }, &result) require.NoError(t, err) @@ -1425,9 +1426,10 @@ func TestDocumentVersion_PublishBlockedWhenPendingApproval(t *testing.T) { } `, map[string]any{ "input": map[string]any{ - "minor": false, - "documentId": docID, - "changelog": "Major release", + "minor": false, + "documentId": docID, + "approverIds": []string{}, + "changelog": "Major release", }, }) require.Error(t, err) @@ -1453,6 +1455,128 @@ func TestDocumentVersion_PublishBlockedWhenPendingApproval(t *testing.T) { }) } +// TestDocumentVersion_PublishApproverIDsContract verifies that approver_ids must +// be an explicit choice when publishing: required for a major version (empty +// list publishes directly, non-empty requests approval) and rejected for a minor +// version (which ignores approvers). +func TestDocumentVersion_PublishApproverIDsContract(t *testing.T) { + t.Parallel() + + const publishMutation = ` + mutation($input: PublishDocumentInput!) { + publishDocument(input: $input) { + documentVersion { status } + approvalQuorum { id } + } + } + ` + + type publishResult struct { + PublishDocument struct { + DocumentVersion struct { + Status string `json:"status"` + } `json:"documentVersion"` + ApprovalQuorum *struct { + ID string `json:"id"` + } `json:"approvalQuorum"` + } `json:"publishDocument"` + } + + t.Run("major publish without approver_ids is rejected", func(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + docID, _ := createTestDocument(t, owner) + + _, err := owner.Do(publishMutation, map[string]any{ + "input": map[string]any{ + "minor": false, + "documentId": docID, + "changelog": "Major release", + }, + }) + require.Error(t, err) + }) + + t.Run("major publish with empty approver_ids publishes directly", func(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + docID, _ := createTestDocument(t, owner) + + var result publishResult + + err := owner.Execute(publishMutation, map[string]any{ + "input": map[string]any{ + "minor": false, + "documentId": docID, + "approverIds": []string{}, + "changelog": "Direct major release", + }, + }, &result) + require.NoError(t, err) + assert.Equal(t, "PUBLISHED", result.PublishDocument.DocumentVersion.Status) + assert.Nil(t, result.PublishDocument.ApprovalQuorum) + }) + + t.Run("major publish with approver_ids requests approval", func(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + docID, _ := createTestDocument(t, owner) + + var result publishResult + + err := owner.Execute(publishMutation, map[string]any{ + "input": map[string]any{ + "minor": false, + "documentId": docID, + "approverIds": []string{getOwnerProfileID(t, owner)}, + "changelog": "Major release needing approval", + }, + }, &result) + require.NoError(t, err) + assert.Equal(t, "PENDING_APPROVAL", result.PublishDocument.DocumentVersion.Status) + require.NotNil(t, result.PublishDocument.ApprovalQuorum) + }) + + t.Run("minor publish without approver_ids is allowed", func(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + docID, _ := createTestDocument(t, owner) + publishMajorDocumentVersion(t, owner, docID) + updateDocumentContent(t, owner, docID, "Updated content for minor") + + var result publishResult + + err := owner.Execute(publishMutation, map[string]any{ + "input": map[string]any{ + "minor": true, + "documentId": docID, + "changelog": "Minor release", + }, + }, &result) + require.NoError(t, err) + assert.Equal(t, "PUBLISHED", result.PublishDocument.DocumentVersion.Status) + assert.Nil(t, result.PublishDocument.ApprovalQuorum) + }) + + t.Run("minor publish with approver_ids is rejected", func(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + docID, _ := createTestDocument(t, owner) + publishMajorDocumentVersion(t, owner, docID) + updateDocumentContent(t, owner, docID, "Updated content for rejected minor") + + _, err := owner.Do(publishMutation, map[string]any{ + "input": map[string]any{ + "minor": true, + "documentId": docID, + "approverIds": []string{getOwnerProfileID(t, owner)}, + "changelog": "Minor release", + }, + }) + require.Error(t, err) + }) +} + func TestDocument_DefaultApprovers(t *testing.T) { t.Parallel() owner := testutil.NewClient(t, testutil.RoleOwner) diff --git a/packages/n8n-node/nodes/Probo/actions/document/publish.operation.ts b/packages/n8n-node/nodes/Probo/actions/document/publish.operation.ts index 1b1979c6b..31c5115d8 100644 --- a/packages/n8n-node/nodes/Probo/actions/document/publish.operation.ts +++ b/packages/n8n-node/nodes/Probo/actions/document/publish.operation.ts @@ -55,7 +55,7 @@ export const description: INodeProperties[] = [ }, }, default: '', - description: 'Comma-separated list of approver profile IDs. When provided, an approval is requested instead of publishing immediately.', + description: 'Comma-separated list of approver profile IDs. Provide IDs to request approval; leave empty to publish the major version directly without approval.', }, { displayName: 'Changelog', @@ -122,12 +122,14 @@ export async function execute( `; const input: Record = { documentId, minor, changelog }; - if (!minor && approverIdsRaw) { - const approverIds = approverIdsRaw + if (!minor) { + // A major publish must set approverIds explicitly: an empty list publishes + // directly without approval, a non-empty list requests approval. It must be + // omitted for a minor publish, which ignores approvers. + input.approverIds = approverIdsRaw .split(',') .map(id => id.trim()) .filter(Boolean); - if (approverIds.length > 0) input.approverIds = approverIds; } const responseData = await proboApiRequest.call(this, query, { input }); diff --git a/pkg/probo/document_service.go b/pkg/probo/document_service.go index b8932a306..6bf5a5b76 100644 --- a/pkg/probo/document_service.go +++ b/pkg/probo/document_service.go @@ -184,6 +184,27 @@ func (req *PublishDocumentRequest) Validate() error { }) v.Check(req.Changelog, "changelog", validator.Required(), validator.SafeText(5000)) + // approver_ids must be an explicit choice for a major publish (an empty list + // publishes directly without approval, a non-empty list requests approval) + // and must be omitted for a minor publish, which ignores approvers. + if req.Minor && req.ApproverIDs != nil { + v.Check(req.ApproverIDs, "approver_ids", func(any) *validator.ValidationError { + return &validator.ValidationError{ + Code: validator.ErrorCodeCustom, + Message: "must not be set when publishing a minor version", + } + }) + } + + if !req.Minor && req.ApproverIDs == nil { + v.Check(req.ApproverIDs, "approver_ids", func(any) *validator.ValidationError { + return &validator.ValidationError{ + Code: validator.ErrorCodeCustom, + Message: "must be set when publishing a major version: provide approver profile IDs to request approval, or an empty list to publish directly without approval", + } + }) + } + return v.Error() } diff --git a/pkg/server/api/mcp/v1/specification.yaml b/pkg/server/api/mcp/v1/specification.yaml index d4193e56e..348b70670 100644 --- a/pkg/server/api/mcp/v1/specification.yaml +++ b/pkg/server/api/mcp/v1/specification.yaml @@ -6305,12 +6305,12 @@ components: description: Document ID minor: type: boolean - description: When true, publish the draft as a minor version and ignore approver_ids. When false, publish as a new major version; if approver_ids are provided, an approval is requested instead. + description: When true, publish the draft as a minor version; approver_ids must be omitted. When false, publish as a new major version; approver_ids is required (a non-empty list requests approval, an empty list publishes directly without approval). approver_ids: type: array items: $ref: "#/components/schemas/GID" - description: Approver profile IDs (ignored when minor is true) + description: "Approver profile IDs. Required when minor is false: pass a non-empty list to request approval (approvers receive an email notification), or an empty list to publish directly without approval. Must be omitted when minor is true." changelog: type: string description: Changelog for this version