From 42b9a8a0e7c935af4153f6d3ccdbf06ba2545b20 Mon Sep 17 00:00:00 2001 From: Guillaume Jacquart Date: Wed, 11 Jun 2025 11:19:18 +0200 Subject: [PATCH] feat(core): Handle scoped resource not existing with 404 (#16022) --- packages/cli/src/controller.registry.ts | 22 ++- .../__tests__/check-access.test.ts | 158 ++++++++++++++++++ .../cli/src/permissions.ee/check-access.ts | 28 +++- .../shared/middlewares/global.middleware.ts | 13 +- .../credentials/credentials.api.test.ts | 4 +- 5 files changed, 209 insertions(+), 16 deletions(-) create mode 100644 packages/cli/src/permissions.ee/__tests__/check-access.test.ts diff --git a/packages/cli/src/controller.registry.ts b/packages/cli/src/controller.registry.ts index acf2171661..37c11a0b1a 100644 --- a/packages/cli/src/controller.registry.ts +++ b/packages/cli/src/controller.registry.ts @@ -18,6 +18,8 @@ import { userHasScopes } from '@/permissions.ee/check-access'; import type { AuthenticatedRequest } from '@/requests'; import { send } from '@/response-helper'; // TODO: move `ResponseHelper.send` to this file +import { NotFoundError } from './errors/response-errors/not-found.error'; + @Service() export class ControllerRegistry { constructor( @@ -125,12 +127,20 @@ export class ControllerRegistry { const { scope, globalOnly } = accessScope; - if (!(await userHasScopes(req.user, [scope], globalOnly, req.params))) { - res.status(403).json({ - status: 'error', - message: RESPONSE_ERROR_MESSAGES.MISSING_SCOPE, - }); - return; + try { + if (!(await userHasScopes(req.user, [scope], globalOnly, req.params))) { + res.status(403).json({ + status: 'error', + message: RESPONSE_ERROR_MESSAGES.MISSING_SCOPE, + }); + return; + } + } catch (error) { + if (error instanceof NotFoundError) { + res.status(404).json({ status: 'error', message: error.message }); + return; + } + throw error; } next(); diff --git a/packages/cli/src/permissions.ee/__tests__/check-access.test.ts b/packages/cli/src/permissions.ee/__tests__/check-access.test.ts new file mode 100644 index 0000000000..bb518ab0f4 --- /dev/null +++ b/packages/cli/src/permissions.ee/__tests__/check-access.test.ts @@ -0,0 +1,158 @@ +import { + ProjectRepository, + SharedCredentialsRepository, + SharedWorkflowRepository, + type User, +} from '@n8n/db'; +import { Container } from '@n8n/di'; +import type { Scope } from '@n8n/permissions'; +import { mock } from 'jest-mock-extended'; + +import { NotFoundError } from '@/errors/response-errors/not-found.error'; + +import { userHasScopes } from '../check-access'; + +describe('userHasScopes', () => { + const findByWorkflowMock = jest.fn(); + const findByCredentialMock = jest.fn(); + + beforeAll(() => { + Container.set( + SharedWorkflowRepository, + mock({ + findBy: findByWorkflowMock, + }), + ); + + Container.set( + SharedCredentialsRepository, + mock({ + findBy: findByCredentialMock, + }), + ); + + Container.set( + ProjectRepository, + mock({ + find: jest.fn().mockResolvedValue([ + { + id: 'projectId', + projectRelations: [{ userId: 'userId', role: 'project:admin' }], + }, + ]), + }), + ); + }); + + beforeEach(() => { + findByWorkflowMock.mockReset(); + findByCredentialMock.mockReset(); + }); + + it.each<{ type: 'workflow' | 'credential'; id: string }>([ + { + type: 'workflow', + id: 'workflowId', + }, + { + type: 'credential', + id: 'credentialId', + }, + ])('should return 404 if the resource is not found', async ({ type, id }) => { + findByWorkflowMock.mockResolvedValueOnce([]); + findByCredentialMock.mockResolvedValueOnce([]); + + const user = { id: 'userId', scopes: [], role: 'global:member' } as unknown as User; + const scopes = ['workflow:read', 'credential:read'] as Scope[]; + + const params: { credentialId?: string; workflowId?: string; projectId?: string } = { + projectId: 'projectId', + }; + if (type === 'credential') { + params.credentialId = id; + } else { + params.workflowId = id; + } + await expect(userHasScopes(user, scopes, false, params)).rejects.toThrow(NotFoundError); + }); + + test.each<{ + type: 'workflow' | 'credential'; + id: string; + role: string; + scope: Scope; + userScopes: Scope[]; + expected: boolean; + }>([ + { + type: 'workflow', + id: 'workflowId', + role: 'workflow:member', + scope: 'workflow:delete', + userScopes: [], + expected: false, + }, + { + type: 'credential', + id: 'credentialId', + role: 'credential:member', + scope: 'credential:delete', + userScopes: [], + expected: false, + }, + { + type: 'workflow', + id: 'workflowId', + role: 'workflow:editor', + scope: 'workflow:read', + userScopes: ['workflow:read'], + expected: true, + }, + { + type: 'credential', + id: 'credentialId', + role: 'credential:user', + scope: 'credential:read', + userScopes: ['credential:read'], + expected: true, + }, + ])( + 'should return $expected if the user has the required scopes for a $type', + async ({ type, id, role, scope, userScopes, expected }) => { + if (type === 'workflow') { + findByWorkflowMock.mockResolvedValueOnce([ + { + workflowId: id, + projectId: 'projectId', + role, + }, + ]); + } else { + findByCredentialMock.mockResolvedValueOnce([ + { + credentialId: id, + projectId: 'projectId', + role, + }, + ]); + } + + const user = { + id: 'userId', + scopes: userScopes, + role: 'global:member', + } as unknown as User; + const scopes = [scope] as Scope[]; + const params: { credentialId?: string; workflowId?: string; projectId?: string } = { + projectId: 'projectId', + }; + if (type === 'credential') { + params.credentialId = id; + } else { + params.workflowId = id; + } + const result = await userHasScopes(user, scopes, false, params); + expect(result).toBe(expected); + }, + ); +}); diff --git a/packages/cli/src/permissions.ee/check-access.ts b/packages/cli/src/permissions.ee/check-access.ts index 277a381446..545952efb3 100644 --- a/packages/cli/src/permissions.ee/check-access.ts +++ b/packages/cli/src/permissions.ee/check-access.ts @@ -6,6 +6,8 @@ import { hasGlobalScope, rolesWithScope, type Scope } from '@n8n/permissions'; import { In } from '@n8n/typeorm'; import { UnexpectedError } from 'n8n-workflow'; +import { NotFoundError } from '@/errors/response-errors/not-found.error'; + /** * Check if a user has the required scopes. The check can be: * @@ -49,19 +51,33 @@ export async function userHasScopes( // those resource roles over the resource being checked. if (credentialId) { - return await Container.get(SharedCredentialsRepository).existsBy({ + const credentials = await Container.get(SharedCredentialsRepository).findBy({ credentialsId: credentialId, - projectId: In(userProjectIds), - role: In(rolesWithScope('credential', scopes)), }); + if (!credentials.length) { + throw new NotFoundError(`Credential with ID "${credentialId}" not found.`); + } + + return credentials.some( + (c) => + userProjectIds.includes(c.projectId) && + rolesWithScope('credential', scopes).includes(c.role), + ); } if (workflowId) { - return await Container.get(SharedWorkflowRepository).existsBy({ + const workflows = await Container.get(SharedWorkflowRepository).findBy({ workflowId, - projectId: In(userProjectIds), - role: In(rolesWithScope('workflow', scopes)), }); + + if (!workflows.length) { + throw new NotFoundError(`Workflow with ID "${workflowId}" not found.`); + } + + return workflows.some( + (w) => + userProjectIds.includes(w.projectId) && rolesWithScope('workflow', scopes).includes(w.role), + ); } if (projectId) return userProjectIds.includes(projectId); diff --git a/packages/cli/src/public-api/v1/shared/middlewares/global.middleware.ts b/packages/cli/src/public-api/v1/shared/middlewares/global.middleware.ts index 24df8a0e9b..bd7d7c1644 100644 --- a/packages/cli/src/public-api/v1/shared/middlewares/global.middleware.ts +++ b/packages/cli/src/public-api/v1/shared/middlewares/global.middleware.ts @@ -6,6 +6,7 @@ import type express from 'express'; import type { NextFunction } from 'express'; import { FeatureNotLicensedError } from '@/errors/feature-not-licensed.error'; +import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { License } from '@/license'; import { userHasScopes } from '@/permissions.ee/check-access'; import type { AuthenticatedRequest } from '@/requests'; @@ -36,8 +37,16 @@ const buildScopeMiddleware = ( params.credentialId = req.params.id; } } - if (!(await userHasScopes(req.user, scopes, globalOnly, params))) { - return res.status(403).json({ message: 'Forbidden' }); + + try { + if (!(await userHasScopes(req.user, scopes, globalOnly, params))) { + return res.status(403).json({ message: 'Forbidden' }); + } + } catch (error) { + if (error instanceof NotFoundError) { + return res.status(404).json({ message: error.message }); + } + throw error; } return next(); diff --git a/packages/cli/test/integration/credentials/credentials.api.test.ts b/packages/cli/test/integration/credentials/credentials.api.test.ts index 349f6683e9..f263d3b68c 100644 --- a/packages/cli/test/integration/credentials/credentials.api.test.ts +++ b/packages/cli/test/integration/credentials/credentials.api.test.ts @@ -1260,12 +1260,12 @@ describe('PATCH /credentials/:id', () => { expect(response.statusCode).toBe(404); }); - test('should fail with a 403 if the credential does not exist and the actor does not have the global credential:update scope', async () => { + test('should fail with a 404 if the credential does not exist and the actor does not have the global credential:update scope', async () => { const response = await authMemberAgent .patch('/credentials/123') .send(randomCredentialPayload()); - expect(response.statusCode).toBe(403); + expect(response.statusCode).toBe(404); }); test('should fail with a 400 is credential is managed', async () => {