mirror of
https://github.com/n8n-io/n8n.git
synced 2026-08-28 17:22:01 +08:00
refactor(core): Authorize workflow review decisions against every covered workflow (no-changelog) (#36985)
This commit is contained in:
+7
-1
@@ -39,7 +39,13 @@ export type WorkflowReviewDecisionIneligibilityReason =
|
||||
| 'missing_permission'
|
||||
| 'missing_reviewer_permission';
|
||||
|
||||
export interface WorkflowReviewRequestDetail extends WorkflowReviewInboxItem {
|
||||
/**
|
||||
* The inbox item's flat `workflowName` / `workflowVersionId` are omitted: they
|
||||
* summarize the review for the list card, which holds exactly one workflow today
|
||||
* (create caps the list at one), while the detail lists every covered workflow.
|
||||
*/
|
||||
export interface WorkflowReviewRequestDetail
|
||||
extends Omit<WorkflowReviewInboxItem, 'workflowName' | 'workflowVersionId'> {
|
||||
description: string | null;
|
||||
workflows: WorkflowReviewRequestWorkflowDetail[];
|
||||
/**
|
||||
|
||||
@@ -136,7 +136,11 @@ export class WorkflowReviewRequestWorkflowRepository extends BaseRepository<Work
|
||||
});
|
||||
}
|
||||
|
||||
/** One workflow per review for now; multi-workflow "primary" selection can wait. */
|
||||
/**
|
||||
* One workflow per review today, so one name per request is enough. With
|
||||
* several rows the Map below would keep an arbitrary one — the inbox card
|
||||
* needs a different treatment (and query) before multi-workflow lands.
|
||||
*/
|
||||
async findLinkedWorkflowsByRequestIds(
|
||||
requestIds: string[],
|
||||
): Promise<Map<string, WorkflowReviewRequestLinkedWorkflow>> {
|
||||
|
||||
+16
-19
@@ -216,24 +216,22 @@ describe('WorkflowReviewAuthorizationService: visibility and the read gate', ()
|
||||
);
|
||||
});
|
||||
|
||||
it('treats the first covered workflow as the one under review', async () => {
|
||||
it('returns the covered workflows together with the ones the caller may read', async () => {
|
||||
mockReadableReviewProject();
|
||||
mockChildRow();
|
||||
|
||||
const result = await service.findReadableRequestOrFail(requester, requestId);
|
||||
|
||||
expect(result.readableWorkflowRows).toEqual([
|
||||
{
|
||||
workflowId,
|
||||
workflowName: 'My workflow',
|
||||
workflowVersionId: 'ver-pinned',
|
||||
activeVersionId: null,
|
||||
baselineVersionId: null,
|
||||
requestState: 'open',
|
||||
},
|
||||
]);
|
||||
expect(result.pinnedWorkflowId).toBe(workflowId);
|
||||
expect(result.canReadPinnedWorkflow).toBe(true);
|
||||
const expectedRow = {
|
||||
workflowId,
|
||||
workflowName: 'My workflow',
|
||||
workflowVersionId: 'ver-pinned',
|
||||
activeVersionId: null,
|
||||
baselineVersionId: null,
|
||||
requestState: 'open',
|
||||
};
|
||||
expect(result.workflowRows).toEqual([expectedRow]);
|
||||
expect(result.readableWorkflowRows).toEqual([expectedRow]);
|
||||
expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith(
|
||||
workflowId,
|
||||
requester,
|
||||
@@ -277,18 +275,17 @@ describe('WorkflowReviewAuthorizationService: visibility and the read gate', ()
|
||||
requestState: 'open',
|
||||
},
|
||||
]);
|
||||
// Eligibility still resolves against the pinned row, which they cannot read
|
||||
expect(result.pinnedWorkflowId).toBe(workflowId);
|
||||
expect(result.canReadPinnedWorkflow).toBe(false);
|
||||
// Eligibility still sees the full coverage, unreadable rows included
|
||||
expect(result.workflowRows.map((row) => row.workflowId)).toEqual([workflowId, 'wf-2']);
|
||||
});
|
||||
|
||||
it('has no workflow under review once the review covers none', async () => {
|
||||
it('returns empty row sets once the review covers no workflow', async () => {
|
||||
mockReadableReviewProject();
|
||||
|
||||
const result = await service.findReadableRequestOrFail(requester, requestId);
|
||||
|
||||
expect(result.pinnedWorkflowId).toBeNull();
|
||||
expect(result.canReadPinnedWorkflow).toBe(false);
|
||||
expect(result.workflowRows).toEqual([]);
|
||||
expect(result.readableWorkflowRows).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
+25
-5
@@ -6,6 +6,7 @@ import type {
|
||||
WorkflowReviewRequestAuthorRepository,
|
||||
WorkflowReviewRequestRepository,
|
||||
WorkflowReviewRequestReviewerRepository,
|
||||
WorkflowReviewRequestWorkflowDetailRow,
|
||||
WorkflowReviewRequestWorkflowRepository,
|
||||
} from '@n8n/db';
|
||||
import { mock } from 'vitest-mock-extended';
|
||||
@@ -42,11 +43,15 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => {
|
||||
|
||||
const request = () => mock<WorkflowReviewRequest>({ id: requestId, projectId });
|
||||
|
||||
const row = (id = 'wf-1') => mock<WorkflowReviewRequestWorkflowDetailRow>({ workflowId: id });
|
||||
|
||||
/** By default the review covers one workflow and the viewer can read it. */
|
||||
const readable = (
|
||||
overrides: Partial<Parameters<typeof service.resolveViewerEligibility>[1]> = {},
|
||||
) => ({
|
||||
request: request(),
|
||||
canReadPinnedWorkflow: true,
|
||||
workflowRows: [row()],
|
||||
readableWorkflowRows: [row()],
|
||||
...overrides,
|
||||
});
|
||||
|
||||
@@ -166,7 +171,7 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => {
|
||||
|
||||
const eligibility = await service.resolveViewerEligibility(
|
||||
memberUser(),
|
||||
readable({ canReadPinnedWorkflow: false }),
|
||||
readable({ readableWorkflowRows: [] }),
|
||||
);
|
||||
|
||||
expect(eligibility).toEqual({
|
||||
@@ -196,13 +201,28 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => {
|
||||
canComment: true,
|
||||
});
|
||||
});
|
||||
|
||||
it('requires read access to every covered workflow, not just one of them', async () => {
|
||||
const rows = [row('wf-1'), row('wf-2')];
|
||||
|
||||
const eligibility = await service.resolveViewerEligibility(
|
||||
memberUser(),
|
||||
readable({ workflowRows: rows, readableWorkflowRows: rows.slice(1) }),
|
||||
);
|
||||
|
||||
expect(eligibility).toEqual({
|
||||
canDecide: false,
|
||||
decisionIneligibilityReason: 'missing_permission',
|
||||
canComment: false,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('who may comment', () => {
|
||||
it('refuses commenting to a viewer who cannot open the workflow under review', async () => {
|
||||
const eligibility = await service.resolveViewerEligibility(
|
||||
memberUser(),
|
||||
readable({ canReadPinnedWorkflow: false }),
|
||||
readable({ readableWorkflowRows: [] }),
|
||||
);
|
||||
|
||||
expect(eligibility).toEqual({
|
||||
@@ -229,7 +249,7 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => {
|
||||
|
||||
const eligibility = await service.resolveViewerEligibility(
|
||||
memberUser(),
|
||||
readable({ canReadPinnedWorkflow: false }),
|
||||
readable({ readableWorkflowRows: [] }),
|
||||
);
|
||||
|
||||
expect(eligibility).toEqual({
|
||||
@@ -242,7 +262,7 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => {
|
||||
it('refuses both deciding and commenting on a review whose workflow is gone', async () => {
|
||||
const eligibility = await service.resolveViewerEligibility(
|
||||
memberUser(),
|
||||
readable({ canReadPinnedWorkflow: false }),
|
||||
readable({ readableWorkflowRows: [] }),
|
||||
);
|
||||
|
||||
expect(eligibility).toEqual({
|
||||
|
||||
+39
-49
@@ -83,13 +83,15 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
});
|
||||
}
|
||||
|
||||
/** The read gate resolved: `readableWorkflowRows` are what the caller may still read. */
|
||||
function mockGate(readableWorkflowRows: WorkflowReviewRequestWorkflowDetailRow[] = []) {
|
||||
/** The read gate resolved: the caller may read everything the review covers. */
|
||||
function mockGate(
|
||||
workflowRows: WorkflowReviewRequestWorkflowDetailRow[] = [],
|
||||
request = reviewRequest(),
|
||||
) {
|
||||
authorizationService.findReadableRequestOrFail.mockResolvedValue({
|
||||
request: reviewRequest(),
|
||||
readableWorkflowRows,
|
||||
pinnedWorkflowId: readableWorkflowRows.at(0)?.workflowId ?? null,
|
||||
canReadPinnedWorkflow: readableWorkflowRows.length > 0,
|
||||
request,
|
||||
workflowRows,
|
||||
readableWorkflowRows: workflowRows,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -157,13 +159,15 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
decision: 'pending',
|
||||
title: 'Please review',
|
||||
description: 'Some context',
|
||||
workflowName: 'My workflow',
|
||||
workflowVersionId: 'ver-pinned',
|
||||
createdAt: '2026-07-01T00:00:00.000Z',
|
||||
updatedAt: '2026-07-02T00:00:00.000Z',
|
||||
});
|
||||
expect(detail.workflows).toHaveLength(1);
|
||||
expect(detail.workflows[0]).toMatchObject({ workflowId, workflowName: 'My workflow' });
|
||||
// The covered workflows live only in `workflows` — the inbox card's flat
|
||||
// summary fields are not part of the detail response.
|
||||
expect(detail).not.toHaveProperty('workflowName');
|
||||
expect(detail).not.toHaveProperty('workflowVersionId');
|
||||
});
|
||||
|
||||
// A covered workflow is removed along with the workflow itself, so a closed
|
||||
@@ -175,8 +179,6 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
const detail = await service.getDetail(requester, requestId);
|
||||
|
||||
expect(detail.workflows).toEqual([]);
|
||||
expect(detail.workflowName).toBeNull();
|
||||
expect(detail.workflowVersionId).toBeNull();
|
||||
});
|
||||
|
||||
// An open review can transiently cover no workflow when a delete orphaned
|
||||
@@ -188,7 +190,6 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
|
||||
expect(detail.state).toBe('open');
|
||||
expect(detail.workflows).toEqual([]);
|
||||
expect(detail.workflowName).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -234,14 +235,14 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
expect(detail.viewerCanComment).toBe(true);
|
||||
});
|
||||
|
||||
it('checks what the viewer may do against the workflow under review, even one they cannot open', async () => {
|
||||
it('checks what the viewer may do against every covered workflow, even ones they cannot open', async () => {
|
||||
// The requester keeps their record after losing view access to the covered
|
||||
// workflow — eligibility must still be checked against that pinned row.
|
||||
// workflow — eligibility must still see the full coverage.
|
||||
const coveredRow = mock<WorkflowReviewRequestWorkflowDetailRow>({ workflowId });
|
||||
authorizationService.findReadableRequestOrFail.mockResolvedValue({
|
||||
request: reviewRequest(),
|
||||
workflowRows: [coveredRow],
|
||||
readableWorkflowRows: [],
|
||||
pinnedWorkflowId: workflowId,
|
||||
canReadPinnedWorkflow: false,
|
||||
});
|
||||
authorizationService.resolveViewerEligibility.mockResolvedValue({
|
||||
canDecide: false,
|
||||
@@ -253,9 +254,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
|
||||
expect(authorizationService.resolveViewerEligibility).toHaveBeenCalledWith(requester, {
|
||||
request: expect.objectContaining({ id: requestId }),
|
||||
workflowRows: [coveredRow],
|
||||
readableWorkflowRows: [],
|
||||
pinnedWorkflowId: workflowId,
|
||||
canReadPinnedWorkflow: false,
|
||||
});
|
||||
expect(detail.workflows).toEqual([]);
|
||||
expect(detail.viewerCanDecide).toBe(false);
|
||||
@@ -263,7 +263,7 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
expect(detail.viewerCanComment).toBe(false);
|
||||
});
|
||||
|
||||
it('passes no workflow id when a closed review no longer covers any workflow', async () => {
|
||||
it('passes empty coverage when a closed review no longer covers any workflow', async () => {
|
||||
requestRepository.findById.mockResolvedValue(reviewRequest({ state: 'closed' }));
|
||||
workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]);
|
||||
|
||||
@@ -271,7 +271,7 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
|
||||
expect(authorizationService.resolveViewerEligibility).toHaveBeenCalledWith(
|
||||
requester,
|
||||
expect.objectContaining({ pinnedWorkflowId: null, canReadPinnedWorkflow: false }),
|
||||
expect.objectContaining({ workflowRows: [], readableWorkflowRows: [] }),
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -349,9 +349,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
});
|
||||
|
||||
it('uses the frozen baseline on a closed review, not the live published pointer', async () => {
|
||||
authorizationService.findReadableRequestOrFail.mockResolvedValue({
|
||||
request: reviewRequest({ state: 'closed', decision: 'approved' }),
|
||||
readableWorkflowRows: [
|
||||
mockGate(
|
||||
[
|
||||
{
|
||||
workflowId,
|
||||
workflowName: 'My workflow',
|
||||
@@ -361,9 +360,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
requestState: 'closed',
|
||||
},
|
||||
],
|
||||
pinnedWorkflowId: workflowId,
|
||||
canReadPinnedWorkflow: true,
|
||||
});
|
||||
reviewRequest({ state: 'closed', decision: 'approved' }),
|
||||
);
|
||||
workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) =>
|
||||
historyVersion(versionId),
|
||||
);
|
||||
@@ -376,9 +374,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
|
||||
it('uses a frozen baseline whatever state accompanies it', async () => {
|
||||
// Only an approval ever writes a baseline, so a frozen one can be trusted alone.
|
||||
authorizationService.findReadableRequestOrFail.mockResolvedValue({
|
||||
request: reviewRequest({ state: 'open', decision: 'pending' }),
|
||||
readableWorkflowRows: [
|
||||
mockGate(
|
||||
[
|
||||
{
|
||||
workflowId,
|
||||
workflowName: 'My workflow',
|
||||
@@ -388,9 +385,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
requestState: 'open',
|
||||
},
|
||||
],
|
||||
pinnedWorkflowId: workflowId,
|
||||
canReadPinnedWorkflow: true,
|
||||
});
|
||||
reviewRequest({ state: 'open', decision: 'pending' }),
|
||||
);
|
||||
workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) =>
|
||||
historyVersion(versionId),
|
||||
);
|
||||
@@ -401,9 +397,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
});
|
||||
|
||||
it('returns no baseline for a closed review when none was captured', async () => {
|
||||
authorizationService.findReadableRequestOrFail.mockResolvedValue({
|
||||
request: reviewRequest({ state: 'closed', decision: 'approved' }),
|
||||
readableWorkflowRows: [
|
||||
mockGate(
|
||||
[
|
||||
{
|
||||
workflowId,
|
||||
workflowName: 'My workflow',
|
||||
@@ -413,9 +408,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
requestState: 'closed',
|
||||
},
|
||||
],
|
||||
pinnedWorkflowId: workflowId,
|
||||
canReadPinnedWorkflow: true,
|
||||
});
|
||||
reviewRequest({ state: 'closed', decision: 'approved' }),
|
||||
);
|
||||
workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) =>
|
||||
historyVersion(versionId),
|
||||
);
|
||||
@@ -429,9 +423,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
// The request is fetched before its rows, so an approval landing in between
|
||||
// leaves the request looking open. The row's own state has to win: a frozen null
|
||||
// baseline would otherwise read as "still open" and resolve the live version.
|
||||
authorizationService.findReadableRequestOrFail.mockResolvedValue({
|
||||
request: reviewRequest({ state: 'open', decision: 'pending' }),
|
||||
readableWorkflowRows: [
|
||||
mockGate(
|
||||
[
|
||||
{
|
||||
workflowId,
|
||||
workflowName: 'My workflow',
|
||||
@@ -441,9 +434,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
requestState: 'closed',
|
||||
},
|
||||
],
|
||||
pinnedWorkflowId: workflowId,
|
||||
canReadPinnedWorkflow: true,
|
||||
});
|
||||
reviewRequest({ state: 'open', decision: 'pending' }),
|
||||
);
|
||||
workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) =>
|
||||
historyVersion(versionId),
|
||||
);
|
||||
@@ -454,9 +446,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
});
|
||||
|
||||
it('returns no baseline for a closed review that was never approved', async () => {
|
||||
authorizationService.findReadableRequestOrFail.mockResolvedValue({
|
||||
request: reviewRequest({ state: 'closed', decision: 'pending' }),
|
||||
readableWorkflowRows: [
|
||||
mockGate(
|
||||
[
|
||||
{
|
||||
workflowId,
|
||||
workflowName: 'My workflow',
|
||||
@@ -466,9 +457,8 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
requestState: 'closed',
|
||||
},
|
||||
],
|
||||
pinnedWorkflowId: workflowId,
|
||||
canReadPinnedWorkflow: true,
|
||||
});
|
||||
reviewRequest({ state: 'closed', decision: 'pending' }),
|
||||
);
|
||||
workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) =>
|
||||
historyVersion(versionId),
|
||||
);
|
||||
|
||||
+17
@@ -216,6 +216,23 @@ describe('WorkflowReviewRequestService.decide', () => {
|
||||
expect(dbLockService.withLockContext).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('throws NotFoundError when the user cannot view every workflow the request covers', async () => {
|
||||
mockSuccessfulDecidePath();
|
||||
workflowRepository.findByRequestId.mockResolvedValue([
|
||||
pinnedRow('ver-1', 'wf-1'),
|
||||
pinnedRow('ver-2', 'wf-2'),
|
||||
]);
|
||||
workflowFinderService.findWorkflowForUser.mockImplementation(async (workflowId) =>
|
||||
workflowId === 'wf-1' ? mock<WorkflowEntity>({ isArchived: false }) : null,
|
||||
);
|
||||
|
||||
await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow(
|
||||
NotFoundError,
|
||||
);
|
||||
|
||||
expect(dbLockService.withLockContext).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('throws NotFoundError for a non-assigned viewer without an admin override', async () => {
|
||||
mockSuccessfulDecidePath();
|
||||
reviewerRepository.isReviewer.mockResolvedValue(false);
|
||||
|
||||
+4
-5
@@ -3347,11 +3347,13 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => {
|
||||
decision: 'pending',
|
||||
title: 'Please review',
|
||||
description: 'Some context',
|
||||
workflowName: 'Reviewed workflow',
|
||||
workflowVersionId: 'version-pinned',
|
||||
requester: { id: owner.id, email: owner.email },
|
||||
reviewers: [{ id: reviewer.id, email: reviewer.email }],
|
||||
});
|
||||
// The covered workflows live only in `workflows` — the inbox card's flat
|
||||
// summary fields are not part of the detail response.
|
||||
expect(response.body.data).not.toHaveProperty('workflowName');
|
||||
expect(response.body.data).not.toHaveProperty('workflowVersionId');
|
||||
|
||||
expect(response.body.data.workflows).toHaveLength(1);
|
||||
const [child] = response.body.data.workflows;
|
||||
@@ -3517,7 +3519,6 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => {
|
||||
expect(response.body.data.id).toBe(request.id);
|
||||
expect(response.body.data.state).toBe('open');
|
||||
expect(response.body.data.workflows).toEqual([]);
|
||||
expect(response.body.data.workflowName).toBeNull();
|
||||
});
|
||||
|
||||
test('still opens a closed review after its workflow was deleted', async () => {
|
||||
@@ -3549,8 +3550,6 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => {
|
||||
|
||||
expect(response.body.data.id).toBe(request.id);
|
||||
expect(response.body.data.workflows).toEqual([]);
|
||||
expect(response.body.data.workflowName).toBeNull();
|
||||
expect(response.body.data.workflowVersionId).toBeNull();
|
||||
});
|
||||
|
||||
test('lets an assigned reviewer in the review project open it', async () => {
|
||||
|
||||
+16
-18
@@ -28,9 +28,9 @@ import { resolveDecisionCapability } from './workflow-review-decision-policy';
|
||||
|
||||
export interface ReadableWorkflowReviewRequest {
|
||||
request: WorkflowReviewRequest;
|
||||
workflowRows: WorkflowReviewRequestWorkflowDetailRow[];
|
||||
/** The subset of {@link workflowRows} the caller may currently read. */
|
||||
readableWorkflowRows: WorkflowReviewRequestWorkflowDetailRow[];
|
||||
pinnedWorkflowId: string | null;
|
||||
canReadPinnedWorkflow: boolean;
|
||||
}
|
||||
|
||||
export interface WorkflowReviewViewerEligibility {
|
||||
@@ -164,17 +164,7 @@ export class WorkflowReviewAuthorizationService {
|
||||
throw new NotFoundError('Could not find review request');
|
||||
}
|
||||
|
||||
// One workflow per review for now, so the only row is the pinned one. Ids are
|
||||
// nanoids, so the query's id ordering just makes the pick deterministic.
|
||||
const pinnedWorkflowId = workflowRows.at(0)?.workflowId ?? null;
|
||||
return {
|
||||
request,
|
||||
readableWorkflowRows,
|
||||
pinnedWorkflowId,
|
||||
canReadPinnedWorkflow: readableWorkflowRows.some(
|
||||
(row) => row.workflowId === pinnedWorkflowId,
|
||||
),
|
||||
};
|
||||
return { request, workflowRows, readableWorkflowRows };
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -278,12 +268,20 @@ export class WorkflowReviewAuthorizationService {
|
||||
*/
|
||||
async resolveViewerEligibility(
|
||||
user: User,
|
||||
access: Pick<ReadableWorkflowReviewRequest, 'request' | 'canReadPinnedWorkflow'>,
|
||||
access: Pick<
|
||||
ReadableWorkflowReviewRequest,
|
||||
'request' | 'workflowRows' | 'readableWorkflowRows'
|
||||
>,
|
||||
): Promise<WorkflowReviewViewerEligibility> {
|
||||
const { request, canReadPinnedWorkflow } = access;
|
||||
const { request, workflowRows, readableWorkflowRows } = access;
|
||||
|
||||
// No participation lookup is worth running without read on the pinned version.
|
||||
if (!canReadPinnedWorkflow) {
|
||||
// A decision or comment covers the whole review, so both need read access to
|
||||
// every covered workflow.
|
||||
const canReadEveryWorkflow =
|
||||
workflowRows.length > 0 && readableWorkflowRows.length === workflowRows.length;
|
||||
|
||||
// No participation lookup is worth running without read on every workflow.
|
||||
if (!canReadEveryWorkflow) {
|
||||
return {
|
||||
canDecide: false,
|
||||
decisionIneligibilityReason: 'missing_permission',
|
||||
@@ -299,7 +297,7 @@ export class WorkflowReviewAuthorizationService {
|
||||
]);
|
||||
|
||||
const capability = resolveDecisionCapability({
|
||||
canReadPinnedWorkflow,
|
||||
canReadEveryWorkflow,
|
||||
isAuthor,
|
||||
isAssignedReviewer,
|
||||
hasAdminOverride,
|
||||
|
||||
@@ -2,8 +2,11 @@ import type { WorkflowReviewDecisionIneligibilityReason } from '@n8n/api-types';
|
||||
|
||||
/** The facts a decision verdict is derived from, resolved by the caller. */
|
||||
export interface WorkflowReviewDecisionFacts {
|
||||
/** Reading the version under review is the floor for deciding it. */
|
||||
canReadPinnedWorkflow: boolean;
|
||||
/**
|
||||
* Reading every workflow the review covers is the floor for deciding it. A
|
||||
* decision applies to all of them, and no workflow on a review outranks another.
|
||||
*/
|
||||
canReadEveryWorkflow: boolean;
|
||||
isAuthor: boolean;
|
||||
isAssignedReviewer: boolean;
|
||||
hasAdminOverride: boolean;
|
||||
@@ -29,9 +32,9 @@ const ALLOWED: WorkflowReviewDecisionCapability = { allowed: true };
|
||||
export function resolveDecisionCapability(
|
||||
facts: WorkflowReviewDecisionFacts,
|
||||
): WorkflowReviewDecisionCapability {
|
||||
// Checked first so someone who cannot see the workflow hears about the
|
||||
// permission rather than about their authorship.
|
||||
if (!facts.canReadPinnedWorkflow) {
|
||||
// Checked first so someone who cannot see every covered workflow hears about
|
||||
// the permission rather than about their authorship.
|
||||
if (!facts.canReadEveryWorkflow) {
|
||||
return { allowed: false, reason: 'missing_permission' };
|
||||
}
|
||||
|
||||
|
||||
@@ -109,14 +109,13 @@ export class WorkflowReviewInboxService {
|
||||
const [workflows, participants, eligibility] = await Promise.all([
|
||||
Promise.all(readableWorkflowRows.map(async (row) => await this.toWorkflowDetail(row))),
|
||||
this.participantResolver.resolve([request]),
|
||||
// Reuses the snapshot above: capabilities are resolved against the pinned
|
||||
// row decide() authorizes against, not against every readable row.
|
||||
// Reuses the snapshot above: capabilities are resolved against every covered
|
||||
// row, matching what decide() authorizes against.
|
||||
this.authorizationService.resolveViewerEligibility(user, access),
|
||||
]);
|
||||
|
||||
return {
|
||||
// One workflow per review for now, so the summary fields mirror the first row
|
||||
...this.toInboxItem(request, workflows.at(0) ?? null, participants.for(request.id)),
|
||||
...this.toReviewSummary(request, participants.for(request.id)),
|
||||
description: request.description,
|
||||
workflows,
|
||||
viewerCanDecide: eligibility.canDecide,
|
||||
@@ -219,17 +218,15 @@ export class WorkflowReviewInboxService {
|
||||
return { createdAt, id };
|
||||
}
|
||||
|
||||
private toInboxItem(
|
||||
/** The review fields shared by the inbox card and the detail response. */
|
||||
private toReviewSummary(
|
||||
entity: WorkflowReviewRequest,
|
||||
linkedWorkflow: WorkflowReviewRequestLinkedWorkflow | null,
|
||||
{ requester, authors, reviewers }: WorkflowReviewParticipants,
|
||||
): WorkflowReviewInboxItem {
|
||||
): Omit<WorkflowReviewInboxItem, 'workflowName' | 'workflowVersionId'> {
|
||||
return {
|
||||
id: entity.id,
|
||||
projectId: entity.projectId,
|
||||
title: entity.title,
|
||||
workflowName: linkedWorkflow?.workflowName ?? null,
|
||||
workflowVersionId: linkedWorkflow?.workflowVersionId ?? null,
|
||||
decision: entity.decision,
|
||||
state: entity.state,
|
||||
createdAt: entity.createdAt.toISOString(),
|
||||
@@ -239,4 +236,16 @@ export class WorkflowReviewInboxService {
|
||||
reviewers,
|
||||
};
|
||||
}
|
||||
|
||||
private toInboxItem(
|
||||
entity: WorkflowReviewRequest,
|
||||
linkedWorkflow: WorkflowReviewRequestLinkedWorkflow | null,
|
||||
participants: WorkflowReviewParticipants,
|
||||
): WorkflowReviewInboxItem {
|
||||
return {
|
||||
...this.toReviewSummary(entity, participants),
|
||||
workflowName: linkedWorkflow?.workflowName ?? null,
|
||||
workflowVersionId: linkedWorkflow?.workflowVersionId ?? null,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -666,20 +666,29 @@ export class WorkflowReviewRequestService {
|
||||
workflowReviewRequestId,
|
||||
{},
|
||||
);
|
||||
const workflowRow = workflowRows[0];
|
||||
// Reviews hold exactly one workflow today (create caps the list at one).
|
||||
// Baseline capture and activity data below already cover every row, but
|
||||
// publishing, broadcasts, and events still assume this single row.
|
||||
const [workflowRow] = workflowRows;
|
||||
if (!workflowRow) {
|
||||
throw new NotFoundError('Could not find review request');
|
||||
}
|
||||
|
||||
const canReadPinnedWorkflow = Boolean(
|
||||
await this.workflowFinderService.findWorkflowForUser(workflowRow.workflowId, user, [
|
||||
'workflow:read',
|
||||
]),
|
||||
// A decision covers every workflow the review holds, so all of them must be
|
||||
// readable — no workflow on a review outranks another.
|
||||
const readableWorkflows = await Promise.all(
|
||||
workflowRows.map(
|
||||
async (row) =>
|
||||
await this.workflowFinderService.findWorkflowForUser(row.workflowId, user, [
|
||||
'workflow:read',
|
||||
]),
|
||||
),
|
||||
);
|
||||
const canReadEveryWorkflow = readableWorkflows.every((workflow) => workflow !== null);
|
||||
// The policy's `missing_permission` verdict, applied here rather than below:
|
||||
// 404 (not 403) so callers without access can't probe which requests exist, and
|
||||
// ahead of the lifecycle check so a closed review does not leak one either.
|
||||
if (!canReadPinnedWorkflow) {
|
||||
if (!canReadEveryWorkflow) {
|
||||
throw new NotFoundError('Could not find review request');
|
||||
}
|
||||
|
||||
@@ -703,7 +712,7 @@ export class WorkflowReviewRequestService {
|
||||
{},
|
||||
);
|
||||
this.assertDecisionAllowed({
|
||||
canReadPinnedWorkflow,
|
||||
canReadEveryWorkflow,
|
||||
isAuthor,
|
||||
isAssignedReviewer,
|
||||
hasAdminOverride,
|
||||
@@ -752,7 +761,7 @@ export class WorkflowReviewRequestService {
|
||||
ctx,
|
||||
);
|
||||
this.assertDecisionAllowed({
|
||||
canReadPinnedWorkflow,
|
||||
canReadEveryWorkflow,
|
||||
isAuthor: isAuthorNow,
|
||||
isAssignedReviewer: isAssignedReviewerNow,
|
||||
hasAdminOverride,
|
||||
|
||||
-2
@@ -60,8 +60,6 @@ function makeDetail(
|
||||
id: 'req-1',
|
||||
projectId: 'proj-1',
|
||||
title: 'Update Payment Handler',
|
||||
workflowName: 'Payment Handler',
|
||||
workflowVersionId: 'version-1',
|
||||
requester,
|
||||
// The backend always carries the requester in `authors` too.
|
||||
authors: [{ ...requester }, laterAuthor],
|
||||
|
||||
-2
@@ -33,8 +33,6 @@ const review: WorkflowReviewRequestDetail = {
|
||||
id: 'req-1',
|
||||
projectId: 'proj-1',
|
||||
title: 'Needs review',
|
||||
workflowName: 'My workflow',
|
||||
workflowVersionId: null,
|
||||
requester: null,
|
||||
authors: [],
|
||||
reviewers: [],
|
||||
|
||||
@@ -784,8 +784,6 @@ function createDetail(): WorkflowReviewRequestDetail {
|
||||
id: 'req-1',
|
||||
projectId: 'proj-1',
|
||||
title: 'Review',
|
||||
workflowName: 'My workflow',
|
||||
workflowVersionId: null,
|
||||
requester: null,
|
||||
authors: [],
|
||||
reviewers: [],
|
||||
|
||||
Reference in New Issue
Block a user