github-mirror/n8n
1
0
Fork 0
mirror of https://github.com/n8n-io/n8n.git synced 2026-10-17 13:39:44 +02:00
Code Issues Actions 14 Packages Projects Releases Wiki Activity Graph

feat(core): Allow reviewer to decide after version submit (#36282)

Browse Source 
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai <kai.hartlage@n8n.io>
This commit is contained in:
Sandra Zollner 2026-08-18 14:12:26 +00:00 committed by GitHub
parent f76b35ca5a
commit 8fa6cde8a1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 86 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts
View File

@ -78,12 +78,25 @@ describe('WorkflowReviewEligibilityService', () => {
});
});
it('stops an author from approving their own review even when assigned', async () => {
it('lets an assigned reviewer decide even when they authored a version', async () => {
authorRepository.isAuthor.mockResolvedValue(true);
reviewerRepository.isReviewer.mockResolvedValue(true);
const eligibility = await service.resolveViewerEligibility(memberUser(), readable());
expect(eligibility).toEqual({
canDecide: true,
decisionIneligibilityReason: null,
canComment: true,
});
});
it('stops a non-assigned author from approving their own review', async () => {
authorRepository.isAuthor.mockResolvedValue(true);
reviewerRepository.isReviewer.mockResolvedValue(false);
const eligibility = await service.resolveViewerEligibility(memberUser(), readable());
expect(eligibility).toEqual({
canDecide: false,
decisionIneligibilityReason: 'author',
@ -129,6 +142,7 @@ describe('WorkflowReviewEligibilityService', () => {
it('still stops an author whose project-admin rights are in another project', async () => {
authorRepository.isAuthor.mockResolvedValue(true);
reviewerRepository.isReviewer.mockResolvedValue(false);
projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue(['other-proj']);
const eligibility = await service.resolveViewerEligibility(memberUser(), readable());

39
packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts
View File

@ -231,12 +231,23 @@ describe('WorkflowReviewRequestService.decide', () => {
});
describe('author eligibility', () => {
it('throws ForbiddenError for an author without an admin override, even when assigned', async () => {
it('allows an assigned reviewer to decide even when they authored a version', async () => {
mockSuccessfulDecidePath();
authorRepository.isAuthor.mockResolvedValue(true);
reviewerRepository.isReviewer.mockResolvedValue(true);
projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]);
const result = await service.decide(memberUser(), requestId, approveDto);
expect(result.decision).toBe('approved');
});
it('throws ForbiddenError for a non-assigned author without an admin override', async () => {
mockSuccessfulDecidePath();
authorRepository.isAuthor.mockResolvedValue(true);
reviewerRepository.isReviewer.mockResolvedValue(false);
projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]);
await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow(
ForbiddenError,
);
@ -244,11 +255,12 @@ describe('WorkflowReviewRequestService.decide', () => {
expect(dbLockService.withLockContext).not.toHaveBeenCalled();
});
// The missing note is a payload problem, and an author is not entitled to hear about it:
// they may not decide at all, whatever they sent.
// The missing note is a payload problem, and a non-reviewer author is not entitled to
// hear about it: they may not decide at all, whatever they sent.
it('tells an author they may not decide even when their note is missing too', async () => {
mockSuccessfulDecidePath();
authorRepository.isAuthor.mockResolvedValue(true);
reviewerRepository.isReviewer.mockResolvedValue(false);
projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]);
await expect(
@ -256,23 +268,33 @@ describe('WorkflowReviewRequestService.decide', () => {
).rejects.toThrow(ForbiddenError);
});
it('rejects a caller who became an author while waiting for the lock', async () => {
it('still allows an assigned reviewer who became an author while waiting for the lock', async () => {
mockSuccessfulDecidePath();
// Not an author before the lock, but a version sync won the lock first and
// added them to the author set before the critical section runs.
authorRepository.isAuthor.mockResolvedValueOnce(false).mockResolvedValueOnce(true);
projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]);
await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow(
ForbiddenError,
);
const result = await service.decide(memberUser(), requestId, approveDto);
// The re-check must run against the lock's transaction, not a fresh read.
expect(result.decision).toBe('approved');
expect(authorRepository.isAuthor).toHaveBeenNthCalledWith(
2,
expect.objectContaining({ workflowReviewRequestId: requestId }),
ctx,
);
expect(requestRepository.saveRequest).toHaveBeenCalled();
});
it('rejects a caller unassigned while waiting for the lock', async () => {
mockSuccessfulDecidePath();
reviewerRepository.isReviewer.mockResolvedValueOnce(true).mockResolvedValueOnce(false);
authorRepository.isAuthor.mockResolvedValue(false);
projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]);
await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow(
NotFoundError,
);
expect(requestRepository.saveRequest).not.toHaveBeenCalled();
});
@ -309,6 +331,7 @@ describe('WorkflowReviewRequestService.decide', () => {
it('throws ForbiddenError for an author who is only a project admin elsewhere', async () => {
mockSuccessfulDecidePath();
authorRepository.isAuthor.mockResolvedValue(true);
reviewerRepository.isReviewer.mockResolvedValue(false);
projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue(['other-proj']);
await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow(

18
packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts
View File

@ -1589,7 +1589,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', ()
});
test('returns 403 for the requesting author without admin override', async () => {
const { request } = await seedRequest(member);
const { request } = await seedRequest(member, {}, [owner.id]);
await memberAgent
.post(`/workflow-review-requests/${request.id}/decision`)
@ -1602,7 +1602,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', ()
});
});
test('returns 403 for a user who became an author via update-version', async () => {
test('allows an assigned reviewer to decide after they update the review version', async () => {
const { request, workflow } = await seedRequest(owner);
await createWorkflowHistoryItem(workflow.id, { versionId: 'version-2' });
@ -1618,7 +1618,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', ()
await memberAgent
.post(`/workflow-review-requests/${request.id}/decision`)
.send({ decision: 'approved' })
.expect(403);
.expect(200);
});
test('allows the instance owner to decide their own review (admin override)', async () => {
@ -3358,7 +3358,7 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => {
// `missing_reviewer_permission` cannot happen over HTTP. The eligibility service
// unit tests cover that branch.
test('tells an assigned author why they cannot decide their own review', async () => {
test('lets an assigned author decide', async () => {
const workflow = await createWorkflow({}, teamProject);
const request = await seedRequest(workflow.id, null, member);
await reviewerRepository.addReviewers(
@ -3368,6 +3368,16 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => {
const response = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200);
expect(response.body.data.viewerCanDecide).toBe(true);
expect(response.body.data.viewerDecisionIneligibilityReason).toBeNull();
});
test('tells a non-assigned author why they cannot decide their own review', async () => {
const workflow = await createWorkflow({}, teamProject);
const request = await seedRequest(workflow.id, null, member);
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('author');
});

31
packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts
View File

@ -54,8 +54,9 @@ 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 (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
* pinned workflow, then admin / assignee / author) so the surfaced reason matches
* the error the endpoint would return. Assigned reviewers stay eligible even if
* they later submitted a version. The endpoint remains the source of truth
* and re-checks under its lock.
*
* Deliberately viewer-scoped: `decide()`'s `assertRequestUpdatable` lifecycle
@ -102,6 +103,14 @@ export class WorkflowReviewEligibilityService {
return { canDecide: true, decisionIneligibilityReason: null, canComment: true };
}
const isAssignedReviewer = await this.workflowReviewRequestReviewerRepository.isReviewer(
{ workflowReviewRequestId: request.id, userId: user.id },
{},
);
if (isAssignedReviewer) {
return { canDecide: true, decisionIneligibilityReason: null, canComment: true };
}
if (isAuthor) {
return {
canDecide: false,
@ -110,18 +119,10 @@ export class WorkflowReviewEligibilityService {
};
}
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 };
return {
canDecide: false,
decisionIneligibilityReason: 'missing_reviewer_permission',
canComment: false,
};
}
}

23
packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts
View File

@ -714,7 +714,7 @@ export class WorkflowReviewRequestService {
request.projectId,
);
// Fast path: reject a known author / non-assignee before queueing on the lock.
// Fast path: reject ineligible callers before queueing on the lock.
const isAuthor = await this.workflowReviewRequestAuthorRepository.isAuthor(
{ workflowReviewRequestId, userId: user.id },
{},
@ -754,10 +754,9 @@ export class WorkflowReviewRequestService {
}
this.assertRequestUpdatable(current);
// 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. Assignment can also change; keep
// the same gate under the lock.
// Re-check authorship and assignment under the lock. A concurrent
// sync may add the caller as an author; assigned reviewers stay
// eligible. Assignment can also change.
const isAuthorNow = await this.workflowReviewRequestAuthorRepository.isAuthor(
{ workflowReviewRequestId, userId: user.id },
ctx,
@ -938,10 +937,10 @@ export class WorkflowReviewRequestService {
}
/**
* 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
* Assigned reviewers (or admins) may decide, including after they submit or
* sync a version. Non-reviewer authors stay blocked unless an admin override
* applies. Called before and again inside the decision lock, since the author
* and assignee sets can change while the caller waits. 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.
@ -951,7 +950,7 @@ export class WorkflowReviewRequestService {
isAssignedReviewer: boolean,
hasAdminOverride: boolean,
): void {
if (hasAdminOverride) {
if (hasAdminOverride || isAssignedReviewer) {
return;
}
@ -959,9 +958,7 @@ export class WorkflowReviewRequestService {
throw new ForbiddenError('Authors cannot decide on their own review request');
}
if (!isAssignedReviewer) {
throw new NotFoundError('Could not find workflow');
}
throw new NotFoundError('Could not find workflow');
}
/**