From 18012d98c8fd6290041a2faae5ea51f82a6ea8e1 Mon Sep 17 00:00:00 2001 From: Charlie Kolb Date: Fri, 31 Oct 2025 11:14:14 +0100 Subject: [PATCH] fix(core): Grant admins full data table access (#21316) --- .../src/roles/scopes/global-scopes.ee.ts | 7 ++++ ...e-aggregate.controller.integration.test.ts | 21 ++++++++---- ...able-aggregate.service.integration.test.ts | 24 +++++++++++++ .../data-table.controller.integration.test.ts | 34 +++++++++---------- .../data-table-aggregate.service.ts | 6 ++++ .../data-table/data-table.controller.ts | 24 ++++++++++++- 6 files changed, 91 insertions(+), 25 deletions(-) diff --git a/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts b/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts index 92f26c7a0e3..22853a71d59 100644 --- a/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts +++ b/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts @@ -90,7 +90,14 @@ export const GLOBAL_OWNER_SCOPES: Scope[] = [ 'folder:list', 'oidc:manage', 'provisioning:manage', + 'dataTable:create', + 'dataTable:delete', + 'dataTable:read', + 'dataTable:update', 'dataTable:list', + 'dataTable:listProject', + 'dataTable:readRow', + 'dataTable:writeRow', 'role:manage', 'mcp:manage', 'mcpApiKey:create', diff --git a/packages/cli/src/modules/data-table/__tests__/data-table-aggregate.controller.integration.test.ts b/packages/cli/src/modules/data-table/__tests__/data-table-aggregate.controller.integration.test.ts index 0af345bf6b3..fe97ebdf3a7 100644 --- a/packages/cli/src/modules/data-table/__tests__/data-table-aggregate.controller.integration.test.ts +++ b/packages/cli/src/modules/data-table/__tests__/data-table-aggregate.controller.integration.test.ts @@ -65,21 +65,28 @@ describe('GET /data-tables-global', () => { expect(response.body.data.data).toHaveLength(0); }); - test('should not list data tables from projects admin has no access to', async () => { + test('should list data tables from projects admin is not explicitly a part of', async () => { const project = await createTeamProject('test project', owner); await createDataTable(project, { name: 'Test Data Table' }); const response = await authAdminAgent.get('/data-tables-global').expect(200); - expect(response.body.data.count).toBe(0); - expect(response.body.data.data).toHaveLength(0); + expect(response.body.data.count).toBe(1); + expect(response.body.data.data[0].name).toEqual('Test Data Table'); }); - test("should not list data tables from another user's personal project", async () => { + test("should only return data tables from another user's personal project for admins", async () => { await createDataTable(ownerProject, { name: 'Personal Data Table' }); - const response = await authAdminAgent.get('/data-tables-global').expect(200); - expect(response.body.data.count).toBe(0); - expect(response.body.data.data).toHaveLength(0); + { + const response = await authAdminAgent.get('/data-tables-global').expect(200); + expect(response.body.data.count).toBe(1); + expect(response.body.data.data[0].name).toBe('Personal Data Table'); + } + { + const response = await authMemberAgent.get('/data-tables-global').expect(200); + expect(response.body.data.count).toBe(0); + expect(response.body.data.data).toHaveLength(0); + } }); test('should list data tables from team projects where user has project:viewer role', async () => { diff --git a/packages/cli/src/modules/data-table/__tests__/data-table-aggregate.service.integration.test.ts b/packages/cli/src/modules/data-table/__tests__/data-table-aggregate.service.integration.test.ts index fd113c4879f..9060a64bf5b 100644 --- a/packages/cli/src/modules/data-table/__tests__/data-table-aggregate.service.integration.test.ts +++ b/packages/cli/src/modules/data-table/__tests__/data-table-aggregate.service.integration.test.ts @@ -7,6 +7,7 @@ import { type Project, type User, PROJECT_ADMIN_ROLE, + GLOBAL_ADMIN_ROLE, } from '@n8n/db'; import { Container } from '@n8n/di'; import type { EntityManager } from '@n8n/typeorm'; @@ -135,6 +136,29 @@ describe('dataTableAggregate', () => { expect(result.count).toBe(0); }); + it('should list all tables for owners and admins', async () => { + // ARRANGE + const currentUser = await createUser({ role: GLOBAL_ADMIN_ROLE }); + + const dt1 = await dataTableService.createDataTable(project1.id, { + name: 'dataTable1', + columns: [], + }); + projectRelationRepository.find.mockResolvedValueOnce([]); + + // ACT + const result = await dataTableAggregateService.getManyAndCount(currentUser, { + skip: 0, + take: 10, + }); + + // ASSERT + expect(result.data).toEqual( + expect.arrayContaining([expect.objectContaining({ id: dt1.id, name: dt1.name })]), + ); + expect(result.count).toBe(1); + }); + it('should return only the data table matching the given data table id filter', async () => { // ARRANGE await dataTableService.createDataTable(project1.id, { diff --git a/packages/cli/src/modules/data-table/__tests__/data-table.controller.integration.test.ts b/packages/cli/src/modules/data-table/__tests__/data-table.controller.integration.test.ts index 1d4380292be..b1824eb9b8f 100644 --- a/packages/cli/src/modules/data-table/__tests__/data-table.controller.integration.test.ts +++ b/packages/cli/src/modules/data-table/__tests__/data-table.controller.integration.test.ts @@ -78,8 +78,8 @@ describe('POST /projects/:projectId/data-tables', () => { }; await authMemberAgent.post('/projects/non-existing-id/data-tables').send(payload).expect(403); - await authAdminAgent.post('/projects/non-existing-id/data-tables').send(payload).expect(403); - await authOwnerAgent.post('/projects/non-existing-id/data-tables').send(payload).expect(403); + await authAdminAgent.post('/projects/non-existing-id/data-tables').send(payload).expect(404); + await authOwnerAgent.post('/projects/non-existing-id/data-tables').send(payload).expect(404); }); test('should not create data table when name is empty', async () => { @@ -229,8 +229,8 @@ describe('POST /projects/:projectId/data-tables', () => { describe('GET /projects/:projectId/data-tables', () => { test('should not list data tables when project does not exist', async () => { await authMemberAgent.get('/projects/non-existing-id/data-tables').expect(403); - await authAdminAgent.get('/projects/non-existing-id/data-tables').expect(403); - await authOwnerAgent.get('/projects/non-existing-id/data-tables').expect(403); + await authAdminAgent.get('/projects/non-existing-id/data-tables').expect(404); + await authOwnerAgent.get('/projects/non-existing-id/data-tables').expect(404); }); test('should not list data tables if user has no access to project', async () => { @@ -239,10 +239,10 @@ describe('GET /projects/:projectId/data-tables', () => { await authMemberAgent.get(`/projects/${project.id}/data-tables`).expect(403); }); - test('should not list data tables if admin has no access to project', async () => { + test('should list data tables for admins', async () => { const project = await createTeamProject('test project', owner); - await authAdminAgent.get(`/projects/${project.id}/data-tables`).expect(403); + await authAdminAgent.get(`/projects/${project.id}/data-tables`).expect(200); }); test("should not list data tables from another user's personal project", async () => { @@ -520,7 +520,7 @@ describe('PATCH /projects/:projectId/data-tables/:dataTableId', () => { await authOwnerAgent .patch('/projects/non-existing-id/data-tables/some-data-table-id') .send(payload) - .expect(403); + .expect(404); }); test('should not update data table when data table does not exist', async () => { @@ -663,7 +663,7 @@ describe('DELETE /projects/:projectId/data-tables/:dataTableId', () => { await authOwnerAgent .delete('/projects/non-existing-id/data-tables/some-data-table-id') .send({}) - .expect(403); + .expect(404); }); test('should not delete data table when data table does not exist', async () => { @@ -790,7 +790,7 @@ describe('GET /projects/:projectId/data-tables/:dataTableId/columns', () => { test('should not list columns when project does not exist', async () => { await authOwnerAgent .get('/projects/non-existing-id/data-tables/non-existing-id/columns') - .expect(403); + .expect(404); }); test('should not list columns if user has no access to project', async () => { @@ -889,7 +889,7 @@ describe('POST /projects/:projectId/data-tables/:dataTableId/columns', () => { await authOwnerAgent .post('/projects/non-existing-id/data-tables/some-data-table-id/columns') .send(payload) - .expect(403); + .expect(404); }); test('should not create column when data table does not exist', async () => { @@ -1115,7 +1115,7 @@ describe('DELETE /projects/:projectId/data-tables/:dataTableId/columns/:columnId await authOwnerAgent .delete('/projects/non-existing-id/data-tables/some-data-table-id/columns/some-column-id') .send({}) - .expect(403); + .expect(404); }); test('should not delete column when data table does not exist', async () => { @@ -1302,7 +1302,7 @@ describe('PATCH /projects/:projectId/data-tables/:dataTableId/columns/:columnId/ await authOwnerAgent .patch('/projects/non-existing-id/data-tables/some-data-table-id/columns/some-column-id/move') .send(payload) - .expect(403); + .expect(404); }); test('should not move column when data table does not exist', async () => { @@ -1526,7 +1526,7 @@ describe('GET /projects/:projectId/data-tables/:dataTableId/rows', () => { test('should not list rows when project does not exist', async () => { await authOwnerAgent .get('/projects/non-existing-id/data-tables/some-data-table-id/rows') - .expect(403); + .expect(404); }); test('should not list rows when data table does not exist', async () => { @@ -1907,7 +1907,7 @@ describe('POST /projects/:projectId/data-tables/:dataTableId/insert', () => { await authOwnerAgent .post('/projects/non-existing-id/data-tables/some-data-table-id/insert') .send(payload) - .expect(403); + .expect(404); }); test('should not insert rows when data table does not exist', async () => { @@ -2508,7 +2508,7 @@ describe('DELETE /projects/:projectId/data-tables/:dataTableId/rows', () => { filters: [{ columnName: 'first', condition: 'eq', value: 'test value' }], }), }) - .expect(403); + .expect(404); }); test('should not delete rows when data table does not exist', async () => { @@ -2874,7 +2874,7 @@ describe('POST /projects/:projectId/data-tables/:dataTableId/upsert', () => { await authOwnerAgent .post('/projects/non-existing-id/data-tables/some-data-table-id/upsert') .send(payload) - .expect(403); + .expect(404); }); test('should not upsert rows when data table does not exist', async () => { @@ -3121,7 +3121,7 @@ describe('PATCH /projects/:projectId/data-tables/:dataTableId/rows', () => { await authOwnerAgent .patch('/projects/non-existing-id/data-tables/some-data-table-id/rows') .send(payload) - .expect(403); + .expect(404); }); test('should not update row when data table does not exist', async () => { diff --git a/packages/cli/src/modules/data-table/data-table-aggregate.service.ts b/packages/cli/src/modules/data-table/data-table-aggregate.service.ts index 4cf3367f3a8..b0fe46b5295 100644 --- a/packages/cli/src/modules/data-table/data-table-aggregate.service.ts +++ b/packages/cli/src/modules/data-table/data-table-aggregate.service.ts @@ -6,6 +6,7 @@ import { Service } from '@n8n/di'; import { ProjectService } from '@/services/project.service.ee'; import { DataTableRepository } from './data-table.repository'; +import { hasGlobalScope } from '@n8n/permissions'; @Service() export class DataTableAggregateService { @@ -20,7 +21,12 @@ export class DataTableAggregateService { async shutdown() {} async getManyAndCount(user: User, options: ListDataTableQueryDto) { + if (hasGlobalScope(user, 'dataTable:listProject')) { + return await this.dataTableRepository.getManyAndCount(options); + } + const projects = await this.projectService.getProjectRelationsForUser(user); + let projectIds = projects.map((x) => x.projectId); if (options.filter?.projectId) { const mask = [options.filter?.projectId].flat(); diff --git a/packages/cli/src/modules/data-table/data-table.controller.ts b/packages/cli/src/modules/data-table/data-table.controller.ts index e322f06769c..196b3df707a 100644 --- a/packages/cli/src/modules/data-table/data-table.controller.ts +++ b/packages/cli/src/modules/data-table/data-table.controller.ts @@ -15,6 +15,7 @@ import { Body, Delete, Get, + Middleware, Param, Patch, Post, @@ -22,6 +23,7 @@ import { Query, RestController, } from '@n8n/decorators'; +import { NextFunction, Response } from 'express'; import { DataTableRowReturn } from 'n8n-workflow'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; @@ -36,10 +38,30 @@ import { DataTableNameConflictError } from './errors/data-table-name-conflict.er import { DataTableNotFoundError } from './errors/data-table-not-found.error'; import { DataTableSystemColumnNameConflictError } from './errors/data-table-system-column-name-conflict.error'; import { DataTableValidationError } from './errors/data-table-validation.error'; +import { ProjectService } from '@/services/project.service.ee'; @RestController('/projects/:projectId/data-tables') export class DataTableController { - constructor(private readonly dataTableService: DataTableService) {} + constructor( + private readonly dataTableService: DataTableService, + private readonly projectService: ProjectService, + ) {} + + @Middleware() + async validateProjectExists( + req: AuthenticatedRequest<{ projectId: string }>, + res: Response, + next: NextFunction, + ) { + try { + const { projectId } = req.params; + await this.projectService.getProject(projectId); + next(); + } catch (e) { + res.status(404).send('Project not found'); + return; + } + } @Post('/') @ProjectScope('dataTable:create')