mirror of
https://github.com/n8n-io/n8n.git
synced 2026-10-17 13:39:44 +02:00
feat(core): Restrict workflow review decisions to assigned reviewers (#36040)
This commit is contained in:
parent
aee23f7b74
commit
64dcebb649
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -60,6 +60,19 @@ export class WorkflowReviewRequestReviewerRepository extends BaseRepository<Work
|
|||
});
|
||||
}
|
||||
|
||||
async isReviewer(
|
||||
input: {
|
||||
workflowReviewRequestId: string;
|
||||
userId: string;
|
||||
},
|
||||
ctx: OperationContext,
|
||||
): Promise<boolean> {
|
||||
return await this.managerFor(ctx).existsBy(WorkflowReviewRequestReviewer, {
|
||||
workflowReviewRequestId: input.workflowReviewRequestId,
|
||||
userId: input.userId,
|
||||
});
|
||||
}
|
||||
|
||||
async findByRequestId(requestId: string): Promise<WorkflowReviewRequestReviewer[]> {
|
||||
return await this.find({
|
||||
where: { workflowReviewRequestId: requestId },
|
||||
|
|
|
|||
|
|
@ -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' })
|
||||
|
|
|
|||
|
|
@ -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<User>({ id, role: { slug: 'global:mem
|
|||
describe('WorkflowReviewEligibilityService', () => {
|
||||
const workflowFinderService = mock<WorkflowFinderService>();
|
||||
const authorRepository = mock<WorkflowReviewRequestAuthorRepository>();
|
||||
const reviewerRepository = mock<WorkflowReviewRequestReviewerRepository>();
|
||||
const projectRelationRepository = mock<ProjectRelationRepository>();
|
||||
|
||||
const service = new WorkflowReviewEligibilityService(
|
||||
workflowFinderService,
|
||||
authorRepository,
|
||||
reviewerRepository,
|
||||
projectRelationRepository,
|
||||
);
|
||||
|
||||
|
|
@ -43,11 +46,12 @@ describe('WorkflowReviewEligibilityService', () => {
|
|||
vi.resetAllMocks();
|
||||
workflowFinderService.findWorkflowForUser.mockResolvedValue(mock<WorkflowEntity>());
|
||||
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<User>({ 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();
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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<User>({ id, role: { slug: 'global:member' } });
|
||||
const requesterUser = mock<User>({
|
||||
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<WorkflowEntity>({ 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<User>({ 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<User>({ 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<WorkflowEntity>({ 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)]);
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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 };
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -85,8 +85,8 @@ export class WorkflowReviewRequestService {
|
|||
|
||||
private async findEligibleReviewers(projectId: string, excludeUserId: string): Promise<User[]> {
|
||||
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<WorkflowReviewAutoPublishOutcome> {
|
||||
|
|
@ -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');
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -353,7 +353,7 @@ describe('WorkflowReviewDetailTabs', () => {
|
|||
props: {
|
||||
review: makeDetail({
|
||||
viewerCanDecide: false,
|
||||
viewerDecisionIneligibilityReason: 'missing_publish_permission',
|
||||
viewerDecisionIneligibilityReason: 'missing_reviewer_permission',
|
||||
}),
|
||||
tab: 'activity',
|
||||
deciding: false,
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user