From 64dcebb649fb8f27590578c1fb04bf98d8dc300a Mon Sep 17 00:00:00 2001 From: Sandra Zollner Date: Wed, 12 Aug 2026 14:10:11 +0200 Subject: [PATCH] feat(core): Restrict workflow review decisions to assigned reviewers (#36040) --- .../get-workflow-review-request-detail.dto.ts | 5 +- ...flow-review-request-reviewer.repository.ts | 13 ++ ...rkflow-review-activity.integration.test.ts | 18 +- ...orkflow-review-eligibility.service.test.ts | 63 +++++-- ...rkflow-review-inbox.detail.service.test.ts | 6 +- ...flow-review-request.decide.service.test.ts | 103 ++++++++++- ...ew-requests.controller.integration.test.ts | 171 ++++++++++++++++-- .../workflow-review-eligibility.service.ts | 51 ++++-- .../workflow-review-request.service.ts | 133 ++++++++++++-- .../WorkflowReviewDetailTabs.test.ts | 2 +- .../views/WorkflowReviewRequestsView.test.ts | 2 +- 11 files changed, 487 insertions(+), 80 deletions(-) diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts index 203102dd3df..4922a91848d 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts @@ -32,7 +32,10 @@ export interface WorkflowReviewRequestWorkflowDetail { * map reasons to copy; unknown future reasons should fall back to a generic * hint. */ -export type WorkflowReviewDecisionIneligibilityReason = 'author' | 'missing_publish_permission'; +export type WorkflowReviewDecisionIneligibilityReason = + | 'author' + | 'missing_permission' + | 'missing_reviewer_permission'; export interface WorkflowReviewRequestDetail extends WorkflowReviewInboxItem { description: string | null; diff --git a/packages/@n8n/db/src/repositories/workflow-review-request-reviewer.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-request-reviewer.repository.ts index b5ae67f8d32..cccfd19c32f 100644 --- a/packages/@n8n/db/src/repositories/workflow-review-request-reviewer.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-review-request-reviewer.repository.ts @@ -60,6 +60,19 @@ export class WorkflowReviewRequestReviewerRepository extends BaseRepository { + return await this.managerFor(ctx).existsBy(WorkflowReviewRequestReviewer, { + workflowReviewRequestId: input.workflowReviewRequestId, + userId: input.userId, + }); + } + async findByRequestId(requestId: string): Promise { return await this.find({ where: { workflowReviewRequestId: requestId }, diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts index 8e9d681d1d0..c5a6ac783d5 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts @@ -15,6 +15,7 @@ import { WorkflowReviewActivityRepository, WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, + WorkflowReviewRequestReviewerRepository, WorkflowReviewRequestWorkflowRepository, } from '@n8n/db'; import { Container } from '@n8n/di'; @@ -187,6 +188,10 @@ describe('Commenting on a review', () => { test('lets a reviewer who can approve the review comment on it', async () => { const { request } = await seedReviewInTeamProject(owner); + await Container.get(WorkflowReviewRequestReviewerRepository).addReviewers( + { workflowReviewRequestId: request.id, userIds: [member.id] }, + {}, + ); await memberAgent .post(`/workflow-review-requests/${request.id}/comments`) @@ -195,6 +200,7 @@ describe('Commenting on a review', () => { const detail = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200); expect(detail.body.data.viewerCanComment).toBe(true); + expect(detail.body.data.viewerCanDecide).toBe(true); }); test('lets a reader who cannot approve read the feed but not post to it', async () => { @@ -235,13 +241,13 @@ describe('Commenting on a review', () => { test('lets a requester downgraded to view-only keep commenting on their own review', async () => { // Opening a review needs workflow:publish, so this state is only reachable by a // downgrade. project:viewer keeps workflow:read, and read is the scope that gates - // commenting, so this must pass while deciding does not. + // commenting, so this must pass while deciding does not (author block). const { request } = await seedReviewInTeamProject(viewer); const detail = await viewerAgent.get(`/workflow-review-requests/${request.id}`).expect(200); expect(detail.body.data.viewerCanComment).toBe(true); expect(detail.body.data.viewerCanDecide).toBe(false); - expect(detail.body.data.viewerDecisionIneligibilityReason).toBe('missing_publish_permission'); + expect(detail.body.data.viewerDecisionIneligibilityReason).toBe('author'); await viewerAgent .post(`/workflow-review-requests/${request.id}/comments`) @@ -531,6 +537,10 @@ describe('Reading the activity feed', () => { test('keeps each comment with its own body and author', async () => { const { request } = await seedReviewInTeamProject(owner); + await Container.get(WorkflowReviewRequestReviewerRepository).addReviewers( + { workflowReviewRequestId: request.id, userIds: [member.id] }, + {}, + ); await ownerAgent .post(`/workflow-review-requests/${request.id}/comments`) .send({ body: 'From the owner' }) @@ -561,6 +571,10 @@ describe('Reading the activity feed', () => { test('keeps a comment readable after its author is deleted from the instance', async () => { const { request } = await seedReviewInTeamProject(owner); + await Container.get(WorkflowReviewRequestReviewerRepository).addReviewers( + { workflowReviewRequestId: request.id, userIds: [member.id] }, + {}, + ); await memberAgent .post(`/workflow-review-requests/${request.id}/comments`) .send({ body: 'Written before leaving' }) diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts index 82fa45660dc..3da3da9b758 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts @@ -4,6 +4,7 @@ import type { WorkflowEntity, WorkflowReviewRequest, WorkflowReviewRequestAuthorRepository, + WorkflowReviewRequestReviewerRepository, } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; @@ -20,11 +21,13 @@ const memberUser = (id = 'user-1') => mock({ id, role: { slug: 'global:mem describe('WorkflowReviewEligibilityService', () => { const workflowFinderService = mock(); const authorRepository = mock(); + const reviewerRepository = mock(); const projectRelationRepository = mock(); const service = new WorkflowReviewEligibilityService( workflowFinderService, authorRepository, + reviewerRepository, projectRelationRepository, ); @@ -43,11 +46,12 @@ describe('WorkflowReviewEligibilityService', () => { vi.resetAllMocks(); workflowFinderService.findWorkflowForUser.mockResolvedValue(mock()); authorRepository.isAuthor.mockResolvedValue(false); + reviewerRepository.isReviewer.mockResolvedValue(true); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); }); describe('who may decide', () => { - it('lets a non-author with publish access decide', async () => { + it('lets an assigned non-author viewer decide', async () => { const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); expect(eligibility).toEqual({ @@ -58,12 +62,25 @@ describe('WorkflowReviewEligibilityService', () => { expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith( workflowId, expect.anything(), - ['workflow:publish'], + ['workflow:read'], ); }); - it('stops an author from approving their own review', async () => { + it('reports a non-assigned viewer as ineligible', async () => { + reviewerRepository.isReviewer.mockResolvedValue(false); + + const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); + + expect(eligibility).toEqual({ + canDecide: false, + decisionIneligibilityReason: 'missing_reviewer_permission', + canComment: false, + }); + }); + + it('stops an author from approving their own review even when assigned', async () => { authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(true); const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); @@ -78,6 +95,7 @@ describe('WorkflowReviewEligibilityService', () => { 'lets an instance %s decide on a review they authored', async (slug) => { authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(false); const admin = mock({ id: 'user-1', role: { slug } }); const eligibility = await service.resolveViewerEligibility(admin, readable()); @@ -88,11 +106,15 @@ describe('WorkflowReviewEligibilityService', () => { canComment: true, }); expect(projectRelationRepository.getAccessibleProjectsByRoles).not.toHaveBeenCalled(); + // Authorship still resolves — it feeds canComment. + expect(authorRepository.isAuthor).toHaveBeenCalled(); + expect(reviewerRepository.isReviewer).not.toHaveBeenCalled(); }, ); it('lets an author who is a project admin of the review project decide', async () => { authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(false); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([projectId]); const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); @@ -102,6 +124,7 @@ describe('WorkflowReviewEligibilityService', () => { decisionIneligibilityReason: null, canComment: true, }); + expect(reviewerRepository.isReviewer).not.toHaveBeenCalled(); }); it('still stops an author whose project-admin rights are in another project', async () => { @@ -117,14 +140,15 @@ describe('WorkflowReviewEligibilityService', () => { }); }); - it('does not look up project roles for someone who is not an author', async () => { + it('still queries project roles for a non-admin', async () => { await service.resolveViewerEligibility(memberUser(), readable()); - expect(projectRelationRepository.getAccessibleProjectsByRoles).not.toHaveBeenCalled(); + // Only global admin/owner short-circuit; everyone else hits the project-role lookup. + expect(projectRelationRepository.getAccessibleProjectsByRoles).toHaveBeenCalledOnce(); }); - it('tells an author without publish rights about the permission, not about their authorship', async () => { - // An author without publish access would hit the endpoint's 404 first, + it('tells an author without view access about the permission, not about their authorship', async () => { + // An author who cannot view the workflow would hit the endpoint's 404 first, // so the surfaced reason must be the permission one, not 'author'. workflowFinderService.findWorkflowForUser.mockResolvedValue(null); authorRepository.isAuthor.mockResolvedValue(true); @@ -132,12 +156,13 @@ describe('WorkflowReviewEligibilityService', () => { const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); // Authorship still resolves — it feeds `canComment`, which survives the missing - // publish right as long as the author can read the pinned workflow. + // finder result as long as the author can read the pinned workflow. expect(eligibility).toEqual({ canDecide: false, - decisionIneligibilityReason: 'missing_publish_permission', + decisionIneligibilityReason: 'missing_permission', canComment: true, }); + expect(reviewerRepository.isReviewer).not.toHaveBeenCalled(); }); // The capability answers "who", not "when": a closed review still reports the @@ -162,14 +187,26 @@ describe('WorkflowReviewEligibilityService', () => { }); describe('who may comment', () => { - it('refuses commenting to a reader who cannot approve the review', async () => { + it('refuses commenting to a viewer who cannot open the workflow under review', async () => { workflowFinderService.findWorkflowForUser.mockResolvedValue(null); const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); expect(eligibility).toEqual({ canDecide: false, - decisionIneligibilityReason: 'missing_publish_permission', + decisionIneligibilityReason: 'missing_permission', + canComment: false, + }); + }); + + it('refuses commenting to a non-assigned viewer even when they can open the workflow', async () => { + reviewerRepository.isReviewer.mockResolvedValue(false); + + const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); + + expect(eligibility).toEqual({ + canDecide: false, + decisionIneligibilityReason: 'missing_reviewer_permission', canComment: false, }); }); @@ -185,7 +222,7 @@ describe('WorkflowReviewEligibilityService', () => { expect(eligibility).toEqual({ canDecide: false, - decisionIneligibilityReason: 'missing_publish_permission', + decisionIneligibilityReason: 'missing_permission', canComment: false, }); }); @@ -198,7 +235,7 @@ describe('WorkflowReviewEligibilityService', () => { expect(eligibility).toEqual({ canDecide: false, - decisionIneligibilityReason: 'missing_publish_permission', + decisionIneligibilityReason: 'missing_permission', canComment: false, }); expect(workflowFinderService.findWorkflowForUser).not.toHaveBeenCalled(); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts index 3bf36f56714..4c51da67b71 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts @@ -272,7 +272,7 @@ describe('WorkflowReviewInboxService.getDetail', () => { }); it('checks what the viewer may do against the workflow under review, even one they cannot open', async () => { - // The requester keeps their record after losing read access to the covered + // The requester keeps their record after losing view access to the covered // workflow — eligibility must still be checked against that pinned row. accessService.findReadableRequestOrFail.mockResolvedValue({ request: reviewRequest(), @@ -282,7 +282,7 @@ describe('WorkflowReviewInboxService.getDetail', () => { }); eligibilityService.resolveViewerEligibility.mockResolvedValue({ canDecide: false, - decisionIneligibilityReason: 'missing_publish_permission', + decisionIneligibilityReason: 'missing_permission', canComment: false, }); @@ -296,7 +296,7 @@ describe('WorkflowReviewInboxService.getDetail', () => { }); expect(detail.workflows).toEqual([]); expect(detail.viewerCanDecide).toBe(false); - expect(detail.viewerDecisionIneligibilityReason).toBe('missing_publish_permission'); + expect(detail.viewerDecisionIneligibilityReason).toBe('missing_permission'); expect(detail.viewerCanComment).toBe(false); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts index 320f24ce262..dbb804df1de 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts @@ -37,6 +37,11 @@ import { WorkflowReviewFeatureGate } from '../workflow-review-feature-gate.servi import { WorkflowReviewRequestService } from '../workflow-review-request.service'; const memberUser = (id = 'user-1') => mock({ id, role: { slug: 'global:member' } }); +const requesterUser = mock({ + id: 'requester-1', + disabled: false, + role: { slug: 'global:member' }, +}); const requestId = 'req-1'; const projectId = 'proj-1'; @@ -84,6 +89,7 @@ describe('WorkflowReviewRequestService.decide', () => { new WorkflowReviewEligibilityService( workflowFinderService, authorRepository, + reviewerRepository, projectRelationRepository, ), roleService, @@ -100,6 +106,7 @@ describe('WorkflowReviewRequestService.decide', () => { decision: 'pending', closedById: null, approvedAt: null, + createdById: requesterUser.id, createdAt: new Date('2026-07-20T10:00:00.000Z'), updatedAt: new Date('2026-07-20T11:00:00.000Z'), ...overrides, @@ -119,7 +126,9 @@ describe('WorkflowReviewRequestService.decide', () => { mock({ isArchived: false }), ); authorRepository.isAuthor.mockResolvedValue(false); + reviewerRepository.isReviewer.mockResolvedValue(true); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); + userRepository.findManyByIds.mockResolvedValue([requesterUser]); requestRepository.saveRequest.mockImplementation(async (request) => request); }; @@ -167,7 +176,7 @@ describe('WorkflowReviewRequestService.decide', () => { expect(dbLockService.withLockContext).not.toHaveBeenCalled(); }); - it('throws NotFoundError when the user lacks publish access to the workflow', async () => { + it('throws NotFoundError when the user cannot view the workflow', async () => { mockSuccessfulDecidePath(); workflowFinderService.findWorkflowForUser.mockResolvedValue(null); @@ -178,11 +187,22 @@ describe('WorkflowReviewRequestService.decide', () => { expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith( 'wf-1', expect.anything(), - ['workflow:publish'], + ['workflow:read'], ); expect(dbLockService.withLockContext).not.toHaveBeenCalled(); }); + it('throws NotFoundError for a non-assigned viewer without an admin override', async () => { + mockSuccessfulDecidePath(); + reviewerRepository.isReviewer.mockResolvedValue(false); + + await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow( + NotFoundError, + ); + + expect(dbLockService.withLockContext).not.toHaveBeenCalled(); + }); + it.each([ ['closed', openRequest({ state: 'closed' })], ['approved', openRequest({ decision: 'approved' })], @@ -198,9 +218,10 @@ describe('WorkflowReviewRequestService.decide', () => { }); describe('author eligibility', () => { - it('throws ForbiddenError for an author without an admin override', async () => { + it('throws ForbiddenError for an author without an admin override, even when assigned', async () => { mockSuccessfulDecidePath(); authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(true); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow( @@ -235,6 +256,7 @@ describe('WorkflowReviewRequestService.decide', () => { async (slug) => { mockSuccessfulDecidePath(); authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(false); const admin = mock({ id: 'user-1', role: { slug } }); const result = await service.decide(admin, requestId, approveDto); @@ -247,6 +269,7 @@ describe('WorkflowReviewRequestService.decide', () => { it('allows an author who is a project admin of the review project', async () => { mockSuccessfulDecidePath(); authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(false); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([projectId]); const result = await service.decide(memberUser(), requestId, approveDto); @@ -382,20 +405,20 @@ describe('WorkflowReviewRequestService.decide', () => { expect(result.workflowVersionId).toBe('ver-2'); // The published version must be the one the approval was recorded against. expect(workflowService.activateWorkflow).toHaveBeenCalledWith( - expect.objectContaining({ id: 'user-1' }), + expect.objectContaining({ id: requesterUser.id }), 'wf-1', { versionId: 'ver-2', source: 'review-approval' }, ); }); describe('auto-publish on approval', () => { - it('publishes the pinned version as the reviewer, after the approval commits', async () => { + it('publishes the pinned version as the requester, after the approval commits', async () => { mockSuccessfulDecidePath(); const result = await service.decide(memberUser(), requestId, approveDto); expect(workflowService.activateWorkflow).toHaveBeenCalledExactlyOnceWith( - expect.objectContaining({ id: 'user-1' }), + expect.objectContaining({ id: requesterUser.id }), 'wf-1', { versionId: 'ver-1', source: 'review-approval' }, ); @@ -412,7 +435,10 @@ describe('WorkflowReviewRequestService.decide', () => { await service.decide(memberUser(), requestId, approveDto); - expect(collaborationService.broadcastWorkflowUpdate).toHaveBeenCalledWith('wf-1', 'user-1'); + expect(collaborationService.broadcastWorkflowUpdate).toHaveBeenCalledWith( + 'wf-1', + requesterUser.id, + ); }); it('never publishes on changes_requested and omits the outcome', async () => { @@ -449,6 +475,69 @@ describe('WorkflowReviewRequestService.decide', () => { expect(collaborationService.broadcastWorkflowUpdate).not.toHaveBeenCalled(); }); + it('keeps the approval as a system close when the requester user has been deleted', async () => { + mockSuccessfulDecidePath(); + userRepository.findManyByIds.mockResolvedValue([]); + + const result = await service.decide(memberUser(), requestId, approveDto); + + expect(userRepository.findManyByIds).toHaveBeenCalledWith([requesterUser.id], { + includeRole: true, + }); + expect(requestRepository.saveRequest.mock.calls[0]?.[0]).toMatchObject({ + decision: 'approved', + state: 'closed', + closedById: null, + }); + expect(workflowService.activateWorkflow).not.toHaveBeenCalled(); + expect(result.autoPublish).toEqual({ + status: 'failed', + message: 'The review requester is no longer available', + }); + }); + + it('keeps the approval as a system close when the requester has been deactivated', async () => { + mockSuccessfulDecidePath(); + userRepository.findManyByIds.mockResolvedValue([ + mock({ id: requesterUser.id, disabled: true, role: { slug: 'global:member' } }), + ]); + + const result = await service.decide(memberUser(), requestId, approveDto); + + expect(requestRepository.saveRequest.mock.calls[0]?.[0]).toMatchObject({ + decision: 'approved', + state: 'closed', + closedById: null, + }); + expect(workflowService.activateWorkflow).not.toHaveBeenCalled(); + expect(result.autoPublish).toEqual({ + status: 'failed', + message: 'The review requester is no longer available', + }); + }); + + it('keeps the approval as a system close when the requester lost publish rights', async () => { + mockSuccessfulDecidePath(); + workflowFinderService.findWorkflowForUser + // Decider: workflow:read + .mockResolvedValueOnce(mock({ isArchived: false })) + // Requester publishability: workflow:publish + .mockResolvedValueOnce(null); + + const result = await service.decide(memberUser(), requestId, approveDto); + + expect(requestRepository.saveRequest.mock.calls[0]?.[0]).toMatchObject({ + decision: 'approved', + state: 'closed', + closedById: null, + }); + expect(workflowService.activateWorkflow).not.toHaveBeenCalled(); + expect(result.autoPublish).toEqual({ + status: 'failed', + message: 'The review requester no longer has permission to publish this workflow', + }); + }); + it('skips publishing and reports a failure when the pinned version was pruned', async () => { mockSuccessfulDecidePath(); workflowRepository.findByRequestId.mockResolvedValue([pinnedRow(null)]); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts index 2afa62828a3..a312d1506be 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts @@ -1316,6 +1316,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () state?: 'open' | 'closed'; decision?: 'pending' | 'changes_requested' | 'approved'; } = {}, + reviewerIds: string[] = [member.id], ) { const workflow = await createWorkflow({}, teamProject); await createWorkflowHistoryItem(workflow.id, { versionId: 'version-1' }); @@ -1340,6 +1341,12 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () { workflowReviewRequestId: request.id, userId: author.id }, {}, ); + if (reviewerIds.length > 0) { + await reviewerRepository.addReviewers( + { workflowReviewRequestId: request.id, userIds: reviewerIds }, + {}, + ); + } return { request, workflow }; } @@ -1376,7 +1383,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () expect(updated?.approvedAt).toBeInstanceOf(Date); }); - test('approval publishes the pinned version under the reviewer identity', async () => { + test('approval publishes the pinned version under the requester identity', async () => { const { request, workflow } = await seedRequest(owner); await memberAgent @@ -1387,13 +1394,73 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () expect( (await workflowEntityRepository.findOneByOrFail({ id: workflow.id })).activeVersionId, ).toBe('version-1'); - // Publish history must record the approving reviewer, not the author. + // Publish history must record the requester, not the approving reviewer. const records = await publishHistoryRepository.findBy({ workflowId: workflow.id }); expect(records).toEqual([ - expect.objectContaining({ event: 'activated', versionId: 'version-1', userId: member.id }), + expect.objectContaining({ event: 'activated', versionId: 'version-1', userId: owner.id }), ]); }); + test('approval without a requester closes as system and reports the publish failure', async () => { + const { request, workflow } = await seedRequest(owner); + const current = await requestRepository.findById(request.id, {}); + current!.createdById = null; + await requestRepository.saveRequest(current!, {}); + + const response = await memberAgent + .post(`/workflow-review-requests/${request.id}/decision`) + .send({ decision: 'approved' }) + .expect(200); + + expect(response.body.data).toMatchObject({ + state: 'closed', + decision: 'approved', + autoPublish: { + status: 'failed', + message: 'The review requester is no longer available', + }, + }); + expect(await requestRepository.findById(request.id, {})).toMatchObject({ + state: 'closed', + decision: 'approved', + closedById: null, + updatedById: member.id, + }); + expect( + (await workflowEntityRepository.findOneByOrFail({ id: workflow.id })).activeVersionId, + ).toBeNull(); + }); + + test('approval when the requester lost publish rights closes as system and reports the failure', async () => { + const demotedRequester = await createUser(); + await linkUserToProject(demotedRequester, teamProject, 'project:editor'); + const { request, workflow } = await seedRequest(demotedRequester); + // Downgrade after the review was opened — they can no longer publish. + await linkUserToProject(demotedRequester, teamProject, 'project:viewer'); + + const response = await memberAgent + .post(`/workflow-review-requests/${request.id}/decision`) + .send({ decision: 'approved' }) + .expect(200); + + expect(response.body.data).toMatchObject({ + state: 'closed', + decision: 'approved', + autoPublish: { + status: 'failed', + message: 'The review requester no longer has permission to publish this workflow', + }, + }); + expect(await requestRepository.findById(request.id, {})).toMatchObject({ + state: 'closed', + decision: 'approved', + closedById: null, + }); + expect( + (await workflowEntityRepository.findOneByOrFail({ id: workflow.id })).activeVersionId, + ).toBeNull(); + }); + test('requests changes: the review stays open, unstamped and unpublished', async () => { const { request, workflow } = await seedRequest(owner); @@ -1515,7 +1582,61 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () .expect(200); }); - test('returns 404 for a project:viewer (lacks workflow:publish)', async () => { + test('returns 404 for a non-assigned editor', async () => { + const { request } = await seedRequest(owner, {}, []); + + await memberAgent + .post(`/workflow-review-requests/${request.id}/decision`) + .send({ decision: 'approved' }) + .expect(404); + }); + + test('allows an assigned viewer to decide', async () => { + const { request } = await seedRequest(owner, {}, [viewer.id]); + + await viewerAgent + .post(`/workflow-review-requests/${request.id}/decision`) + .send({ decision: 'changes_requested' }) + .expect(200); + + expect(await requestRepository.findById(request.id, {})).toMatchObject({ + state: 'open', + decision: 'changes_requested', + updatedById: viewer.id, + }); + }); + + test('lets an assigned viewer approve and publishes under the requester identity', async () => { + // The decider holds workflow:read only, so publishing can only work + // because it runs as the requester. + const { request, workflow } = await seedRequest(owner, {}, [viewer.id]); + + const response = await viewerAgent + .post(`/workflow-review-requests/${request.id}/decision`) + .send({ decision: 'approved' }) + .expect(200); + + expect(response.body.data).toMatchObject({ + state: 'closed', + decision: 'approved', + autoPublish: { status: 'published' }, + }); + expect(await requestRepository.findById(request.id, {})).toMatchObject({ + state: 'closed', + decision: 'approved', + updatedById: viewer.id, + closedById: viewer.id, + }); + expect( + (await workflowEntityRepository.findOneByOrFail({ id: workflow.id })).activeVersionId, + ).toBe('version-1'); + const records = await publishHistoryRepository.findBy({ workflowId: workflow.id }); + expect(records).toEqual([ + expect.objectContaining({ event: 'activated', versionId: 'version-1', userId: owner.id }), + ]); + }); + + test('returns 404 for a project:viewer who is not assigned', async () => { const { request } = await seedRequest(owner); await viewerAgent @@ -1634,7 +1755,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () }); describe('GET /workflow-review-requests/eligible-reviewers', () => { - test('returns publish-capable project and instance users, excluding everyone else', async () => { + test('returns project viewers, editors, and instance users, excluding everyone else', async () => { const project = await createTeamProject('team', owner); // The requester holds workflow:publish through project:editor await linkUserToProject(member, project, 'project:editor'); @@ -1643,10 +1764,10 @@ describe('GET /workflow-review-requests/eligible-reviewers', () => { await linkUserToProject(projectAdmin, project, 'project:admin'); const projectEditor = await createUser(); await linkUserToProject(projectEditor, project, 'project:editor'); - const globalAdmin = await createAdmin(); - const projectViewer = await createUser(); await linkUserToProject(projectViewer, project, 'project:viewer'); + const globalAdmin = await createAdmin(); + const disabledEditor = await createUser({ disabled: true }); await linkUserToProject(disabledEditor, project, 'project:editor'); const pendingEditor = await createUser({ password: null }); @@ -1660,10 +1781,10 @@ describe('GET /workflow-review-requests/eligible-reviewers', () => { .query({ workflowId: workflow.id }) .expect(200); - expect(response.body.data.count).toBe(4); + expect(response.body.data.count).toBe(5); const ids = response.body.data.data.map((reviewer: { id: string }) => reviewer.id); expect(ids.sort()).toEqual( - [owner.id, projectAdmin.id, projectEditor.id, globalAdmin.id].sort(), + [owner.id, projectAdmin.id, projectEditor.id, projectViewer.id, globalAdmin.id].sort(), ); }); @@ -1713,7 +1834,7 @@ describe('GET /workflow-review-requests/eligible-reviewers', () => { .query({ workflowId: workflow.id }) .expect(200); - // The requesting owner is excluded; the plain member holds no publish rights + // The requesting owner is excluded; the plain member holds no read rights on this personal project expect(response.body.data.count).toBe(1); expect(response.body.data.data[0].id).toBe(globalAdmin.id); }); @@ -2765,9 +2886,13 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { }); describe('viewer decision eligibility', () => { - test('tells a non-author with publish access that they can decide', async () => { + test('tells an assigned reviewer that they can decide', async () => { const workflow = await createWorkflow({}, teamProject); const request = await seedRequest(workflow.id, null, owner); + await reviewerRepository.addReviewers( + { workflowReviewRequestId: request.id, userIds: [member.id] }, + {}, + ); const response = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200); @@ -2775,9 +2900,25 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { expect(response.body.data.viewerDecisionIneligibilityReason).toBeNull(); }); - test('tells an author why they cannot decide their own review', async () => { + test('tells a non-assigned editor that they cannot decide', async () => { + const workflow = await createWorkflow({}, teamProject); + const request = await seedRequest(workflow.id, null, owner); + + const response = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200); + + expect(response.body.data.viewerCanDecide).toBe(false); + expect(response.body.data.viewerDecisionIneligibilityReason).toBe( + 'missing_reviewer_permission', + ); + }); + + test('tells an assigned author why they cannot decide their own review', async () => { const workflow = await createWorkflow({}, teamProject); const request = await seedRequest(workflow.id, null, member); + await reviewerRepository.addReviewers( + { workflowReviewRequestId: request.id, userIds: [member.id] }, + {}, + ); const response = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200); @@ -2795,7 +2936,7 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { expect(response.body.data.viewerDecisionIneligibilityReason).toBeNull(); }); - test('reports missing publish permission once the workflow moved out of the requester reach', async () => { + test('reports missing permission once the workflow moved out of the requester reach', async () => { // Same transfer scenario the read filter covers: the requester keeps the // record, but could no longer decide — and the reason says why. const destinationProject = await createTeamProject('Out Of Reach', owner); @@ -2806,9 +2947,7 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { const response = await viewerAgent.get(`/workflow-review-requests/${request.id}`).expect(200); expect(response.body.data.viewerCanDecide).toBe(false); - expect(response.body.data.viewerDecisionIneligibilityReason).toBe( - 'missing_publish_permission', - ); + expect(response.body.data.viewerDecisionIneligibilityReason).toBe('missing_permission'); }); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts index b7f2c1689e4..1e7a4f963fd 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts @@ -2,6 +2,7 @@ import type { WorkflowReviewDecisionIneligibilityReason } from '@n8n/api-types'; import { ProjectRelationRepository, WorkflowReviewRequestAuthorRepository, + WorkflowReviewRequestReviewerRepository, type User, } from '@n8n/db'; import { Service } from '@n8n/di'; @@ -30,6 +31,7 @@ export class WorkflowReviewEligibilityService { constructor( private readonly workflowFinderService: WorkflowFinderService, private readonly workflowReviewRequestAuthorRepository: WorkflowReviewRequestAuthorRepository, + private readonly workflowReviewRequestReviewerRepository: WorkflowReviewRequestReviewerRepository, private readonly projectRelationRepository: ProjectRelationRepository, ) {} @@ -51,10 +53,10 @@ export class WorkflowReviewEligibilityService { /** * `canDecide` is an advisory read-time snapshot of whether the viewer could decide - * the request, mirroring `decide()`'s authorization checks in order (publish on the - * pinned workflow first, then authorship) so the surfaced reason matches the error - * the endpoint would return. The endpoint remains the source of truth and re-checks - * under its lock. + * the request, mirroring `decide()`'s authorization checks in order (read on the + * pinned workflow, then admin / author / assignee) so the surfaced reason matches + * the error the endpoint would return. The endpoint remains the source of truth + * and re-checks under its lock. * * Deliberately viewer-scoped: `decide()`'s `assertRequestUpdatable` lifecycle * guard is not mirrored here. It is shared with the update path and is not @@ -73,36 +75,53 @@ export class WorkflowReviewEligibilityService { if (!pinnedWorkflowId) { return { canDecide: false, - decisionIneligibilityReason: 'missing_publish_permission', + decisionIneligibilityReason: 'missing_permission', canComment: false, }; } const [workflow, isAuthor] = await Promise.all([ - this.workflowFinderService.findWorkflowForUser(pinnedWorkflowId, user, ['workflow:publish']), + this.workflowFinderService.findWorkflowForUser(pinnedWorkflowId, user, ['workflow:read']), this.workflowReviewRequestAuthorRepository.isAuthor( { workflowReviewRequestId: request.id, userId: user.id }, {}, ), ]); - // Authorship is history; access is not. An author keeps commenting only while they - // can still read the pinned workflow — `workflow:read`, not `workflow:publish`, so a - // personal-project requester is not locked out of their own review. - const canComment = isAuthor ? canReadPinnedWorkflow : Boolean(workflow); - if (!workflow) { return { canDecide: false, - decisionIneligibilityReason: 'missing_publish_permission', - canComment, + decisionIneligibilityReason: 'missing_permission', + // Authorship still resolves — an author keeps commenting only while they + // can still read the pinned workflow. + canComment: isAuthor ? canReadPinnedWorkflow : false, }; } - if (isAuthor && !(await this.hasAdminOverride(user, request.projectId))) { - return { canDecide: false, decisionIneligibilityReason: 'author', canComment }; + if (await this.hasAdminOverride(user, request.projectId)) { + return { canDecide: true, decisionIneligibilityReason: null, canComment: true }; } - return { canDecide: true, decisionIneligibilityReason: null, canComment }; + if (isAuthor) { + return { + canDecide: false, + decisionIneligibilityReason: 'author', + canComment: canReadPinnedWorkflow, + }; + } + + const isAssignedReviewer = await this.workflowReviewRequestReviewerRepository.isReviewer( + { workflowReviewRequestId: request.id, userId: user.id }, + {}, + ); + if (!isAssignedReviewer) { + return { + canDecide: false, + decisionIneligibilityReason: 'missing_reviewer_permission', + canComment: false, + }; + } + + return { canDecide: true, decisionIneligibilityReason: null, canComment: true }; } } diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts index a6e0493220b..8c8aa6f2e89 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts @@ -85,8 +85,8 @@ export class WorkflowReviewRequestService { private async findEligibleReviewers(projectId: string, excludeUserId: string): Promise { const [projectRoleSlugs, globalRoleSlugs] = await Promise.all([ - this.roleService.rolesWithScope('project', ['workflow:publish']), - this.roleService.rolesWithScope('global', ['workflow:publish']), + this.roleService.rolesWithScope('project', ['workflow:read']), + this.roleService.rolesWithScope('global', ['workflow:read']), ]); const users = await this.userRepository.findEligibleByProjectOrGlobalRoles({ @@ -615,7 +615,7 @@ export class WorkflowReviewRequestService { const workflow = await this.workflowFinderService.findWorkflowForUser( workflowRow.workflowId, user, - ['workflow:publish'], + ['workflow:read'], ); if (!workflow) { throw new NotFoundError('Could not find workflow'); @@ -631,12 +631,24 @@ export class WorkflowReviewRequestService { request.projectId, ); - // Fast path: reject a known author before queueing on the lock. + // Fast path: reject a known author / non-assignee before queueing on the lock. const isAuthor = await this.workflowReviewRequestAuthorRepository.isAuthor( { workflowReviewRequestId, userId: user.id }, {}, ); - this.assertDecisionAllowed(isAuthor, hasAdminOverride); + const isAssignedReviewer = await this.workflowReviewRequestReviewerRepository.isReviewer( + { workflowReviewRequestId, userId: user.id }, + {}, + ); + this.assertDecisionAllowed(isAuthor, isAssignedReviewer, hasAdminOverride); + + // Resolved pre-lock (role/user lookups must not run inside the lock transaction). + // Drives both auto-publish and whether the close is attributed to the reviewer + // or left as a system close (`closedById = null`). + const requesterPublishability = + dto.decision === 'approved' + ? await this.resolveRequesterPublishability(request.createdById, workflowRow.workflowId) + : null; const { request: saved, pinnedVersionId } = await this.dbLockService.withLockContext( DbLock.WORKFLOW_REVIEW_REQUEST_CREATE, @@ -654,12 +666,17 @@ export class WorkflowReviewRequestService { // Re-check authorship here — a sync that won the lock first has // added its syncer to the author set since the pre-lock check, and that - // syncer must not be able to decide. + // syncer must not be able to decide. Assignment can also change; keep + // the same gate under the lock. const isAuthorNow = await this.workflowReviewRequestAuthorRepository.isAuthor( { workflowReviewRequestId, userId: user.id }, ctx, ); - this.assertDecisionAllowed(isAuthorNow, hasAdminOverride); + const isAssignedReviewerNow = await this.workflowReviewRequestReviewerRepository.isReviewer( + { workflowReviewRequestId, userId: user.id }, + ctx, + ); + this.assertDecisionAllowed(isAuthorNow, isAssignedReviewerNow, hasAdminOverride); // Re-read the pinned row too: a concurrent sync that won the lock may // have re-pinned, and the summary must reflect the version being decided on. @@ -676,7 +693,9 @@ export class WorkflowReviewRequestService { current.updatedById = user.id; if (dto.decision === 'approved') { current.state = 'closed'; - current.closedById = user.id; + // No closer when the requester can't publish — UI treats null as a + // system close ("Review got closed by system"). + current.closedById = requesterPublishability?.canPublish === true ? user.id : null; current.approvedAt = new Date(); } @@ -689,18 +708,78 @@ export class WorkflowReviewRequestService { const summary = this.toSummary(saved, pinnedVersionId); - if (dto.decision !== 'approved') { + if (dto.decision !== 'approved' || requesterPublishability === null) { return summary; } + if (!requesterPublishability.canPublish) { + this.logger.warn('Cannot publish approved review: requester cannot publish', { + workflowId: workflowRow.workflowId, + message: requesterPublishability.failureMessage, + }); + return { + ...summary, + autoPublish: { + status: 'failed', + message: requesterPublishability.failureMessage, + }, + }; + } + return { ...summary, - autoPublish: await this.publishApprovedVersion(user, workflowRow.workflowId, pinnedVersionId), + autoPublish: await this.publishApprovedVersion( + requesterPublishability.requester, + workflowRow.workflowId, + pinnedVersionId, + ), }; } + /** + * Approval publishes as the review requester, not the deciding reviewer — + * so assigned reviewers only need `workflow:read` to decide. Resolved before + * the decision lock so a missing/unpublishable requester can close with + * `closedById = null` instead of attributing the close to the reviewer. + */ + private async resolveRequesterPublishability( + createdById: string | null, + workflowId: string, + ): Promise< + { canPublish: true; requester: User } | { canPublish: false; failureMessage: string } + > { + if (!createdById) { + return { + canPublish: false, + failureMessage: 'The review requester is no longer available', + }; + } + + const [requester] = await this.userRepository.findManyByIds([createdById], { + includeRole: true, + }); + if (!requester || requester.disabled) { + return { + canPublish: false, + failureMessage: 'The review requester is no longer available', + }; + } + + const workflow = await this.workflowFinderService.findWorkflowForUser(workflowId, requester, [ + 'workflow:publish', + ]); + if (!workflow) { + return { + canPublish: false, + failureMessage: 'The review requester no longer has permission to publish this workflow', + }; + } + + return { canPublish: true, requester }; + } + private async publishApprovedVersion( - user: User, + requester: User, workflowId: string, pinnedVersionId: string | null, ): Promise { @@ -714,7 +793,7 @@ export class WorkflowReviewRequestService { } try { - await this.workflowService.activateWorkflow(user, workflowId, { + await this.workflowService.activateWorkflow(requester, workflowId, { versionId: pinnedVersionId, source: 'review-approval', }); @@ -730,7 +809,7 @@ export class WorkflowReviewRequestService { // Same broadcast the manual activate endpoint sends, so open editor // sessions pick up the newly published version. this.collaborationService - .broadcastWorkflowUpdate(workflowId, user.id) + .broadcastWorkflowUpdate(workflowId, requester.id) .catch((error) => this.logger.warn('Failed to broadcast workflow update', { workflowId, error }), ); @@ -739,16 +818,30 @@ export class WorkflowReviewRequestService { } /** - * Authors cannot decide their own review request, unless an admin override - * applies. Called before and again inside the decision lock, since the author - * set can change while the caller waits for the lock. The override is resolved - * once, pre-lock: the lock guards the author set, not role membership — like - * every other authorization check in `decide`, roles are evaluated up front. + * Only assigned reviewers (or admins) may decide. Authors / version syncers + * are always blocked unless an admin override applies — being assigned does + * not lift that. Called before and again inside the decision lock, since the + * author set can change while the caller waits for the lock. The override is + * resolved once, pre-lock: the lock guards author/assignee rows, not role + * membership — like every other authorization check in `decide`, roles are + * evaluated up front. */ - private assertDecisionAllowed(isAuthor: boolean, hasAdminOverride: boolean): void { - if (isAuthor && !hasAdminOverride) { + private assertDecisionAllowed( + isAuthor: boolean, + isAssignedReviewer: boolean, + hasAdminOverride: boolean, + ): void { + if (hasAdminOverride) { + return; + } + + if (isAuthor) { throw new ForbiddenError('Authors cannot decide on their own review request'); } + + if (!isAssignedReviewer) { + throw new NotFoundError('Could not find workflow'); + } } /** diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.test.ts index c5022ce366c..85269f382b1 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.test.ts @@ -353,7 +353,7 @@ describe('WorkflowReviewDetailTabs', () => { props: { review: makeDetail({ viewerCanDecide: false, - viewerDecisionIneligibilityReason: 'missing_publish_permission', + viewerDecisionIneligibilityReason: 'missing_reviewer_permission', }), tab: 'activity', deciding: false, diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/views/WorkflowReviewRequestsView.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/views/WorkflowReviewRequestsView.test.ts index 2ac6b46c2fa..8384e9a5574 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/views/WorkflowReviewRequestsView.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/views/WorkflowReviewRequestsView.test.ts @@ -675,7 +675,7 @@ describe('WorkflowReviewRequestsView', () => { it('falls back to the generic permission hint for any other reason', async () => { store.detail = createDetail({ viewerCanDecide: false, - viewerDecisionIneligibilityReason: 'missing_publish_permission', + viewerDecisionIneligibilityReason: 'missing_reviewer_permission', }); const { getByTestId } = renderComponent();