diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 75e533f0ed282..c4527c6bff637 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -15,6 +15,7 @@ import { InternalHooks } from '@/InternalHooks'; import { listQueryMiddleware } from '@/middlewares'; import { Logger } from '@/Logger'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; +import { UnauthorizedError } from '@/errors/response-errors/unauthorized.error'; export const credentialsController = express.Router(); credentialsController.use('/', EECredentialsController); @@ -142,10 +143,15 @@ credentialsController.patch( ResponseHelper.send(async (req: CredentialRequest.Update): Promise => { const { id: credentialId } = req.params; - const sharing = await CredentialsService.getSharing(req.user, credentialId, { - allowGlobalScope: true, - globalScope: 'credential:update', - }); + const sharing = await CredentialsService.getSharing( + req.user, + credentialId, + { + allowGlobalScope: true, + globalScope: 'credential:update', + }, + ['credentials', 'role'], + ); if (!sharing) { Container.get(Logger).info( @@ -160,6 +166,17 @@ credentialsController.patch( ); } + if (sharing.role.name !== 'owner' && !(await req.user.hasGlobalScope('credential:update'))) { + Container.get(Logger).info( + 'Attempt to delete credential blocked due to lack of permissions', + { + credentialId, + userId: req.user.id, + }, + ); + throw new UnauthorizedError('You can only update credentials owned by you'); + } + const { credentials: credential } = sharing; const decryptedData = CredentialsService.decrypt(credential); @@ -195,10 +212,15 @@ credentialsController.delete( ResponseHelper.send(async (req: CredentialRequest.Delete) => { const { id: credentialId } = req.params; - const sharing = await CredentialsService.getSharing(req.user, credentialId, { - allowGlobalScope: true, - globalScope: 'credential:delete', - }); + const sharing = await CredentialsService.getSharing( + req.user, + credentialId, + { + allowGlobalScope: true, + globalScope: 'credential:delete', + }, + ['credentials', 'role'], + ); if (!sharing) { Container.get(Logger).info( @@ -213,6 +235,17 @@ credentialsController.delete( ); } + if (sharing.role.name !== 'owner' && !(await req.user.hasGlobalScope('credential:delete'))) { + Container.get(Logger).info( + 'Attempt to delete credential blocked due to lack of permissions', + { + credentialId, + userId: req.user.id, + }, + ); + throw new UnauthorizedError('You can only remove credentials owned by you'); + } + const { credentials: credential } = sharing; await CredentialsService.delete(credential); diff --git a/packages/cli/src/permissions/roles.ts b/packages/cli/src/permissions/roles.ts index 59b46408285ae..9db80b3d2b318 100644 --- a/packages/cli/src/permissions/roles.ts +++ b/packages/cli/src/permissions/roles.ts @@ -4,7 +4,6 @@ export const ownerPermissions: Scope[] = [ 'auditLogs:manage', 'credential:create', 'credential:read', - 'credential:update', 'credential:delete', 'credential:list', 'credential:share', diff --git a/packages/cli/test/integration/credentials.ee.test.ts b/packages/cli/test/integration/credentials.ee.test.ts index d77bf886c8376..f0792244aaff0 100644 --- a/packages/cli/test/integration/credentials.ee.test.ts +++ b/packages/cli/test/integration/credentials.ee.test.ts @@ -2,7 +2,7 @@ import type { SuperAgentTest } from 'supertest'; import { In } from 'typeorm'; import type { IUser } from 'n8n-workflow'; -import type { Credentials } from '@/requests'; +import type { ListQuery } from '@/requests'; import * as UserManagementHelpers from '@/UserManagement/UserManagementHelper'; import type { Role } from '@db/entities/Role'; import type { User } from '@db/entities/User'; @@ -99,10 +99,10 @@ describe('GET /credentials', () => { expect(response.statusCode).toBe(200); expect(response.body.data).toHaveLength(2); // owner retrieved owner cred and member cred const ownerCredential = response.body.data.find( - (e: Credentials.WithOwnedByAndSharedWith) => e.ownedBy?.id === owner.id, + (e: ListQuery.Credentials.WithOwnedByAndSharedWith) => e.ownedBy?.id === owner.id, ); const memberCredential = response.body.data.find( - (e: Credentials.WithOwnedByAndSharedWith) => e.ownedBy?.id === member1.id, + (e: ListQuery.Credentials.WithOwnedByAndSharedWith) => e.ownedBy?.id === member1.id, ); validateMainCredentialData(ownerCredential); @@ -540,7 +540,7 @@ describe('PUT /credentials/:id/share', () => { }); }); -function validateMainCredentialData(credential: Credentials.WithOwnedByAndSharedWith) { +function validateMainCredentialData(credential: ListQuery.Credentials.WithOwnedByAndSharedWith) { expect(typeof credential.name).toBe('string'); expect(typeof credential.type).toBe('string'); expect(typeof credential.nodesAccess[0].nodeType).toBe('string'); diff --git a/packages/cli/test/integration/credentials.test.ts b/packages/cli/test/integration/credentials.test.ts index 90d8deaf4557f..09a8c2973fcd9 100644 --- a/packages/cli/test/integration/credentials.test.ts +++ b/packages/cli/test/integration/credentials.test.ts @@ -2,7 +2,7 @@ import type { SuperAgentTest } from 'supertest'; import config from '@/config'; import * as UserManagementHelpers from '@/UserManagement/UserManagementHelper'; -import type { Credentials } from '@/requests'; +import type { ListQuery } from '@/requests'; import type { Role } from '@db/entities/Role'; import type { User } from '@db/entities/User'; @@ -10,7 +10,7 @@ import { randomCredentialPayload, randomName, randomString } from './shared/rand import * as testDb from './shared/testDb'; import type { SaveCredentialFunction } from './shared/types'; import * as utils from './shared/utils/'; -import { affixRoleToSaveCredential } from './shared/db/credentials'; +import { affixRoleToSaveCredential, shareCredentialWithUsers } from './shared/db/credentials'; import { getCredentialOwnerRole, getGlobalMemberRole, getGlobalOwnerRole } from './shared/db/roles'; import { createManyUsers, createUser } from './shared/db/users'; import { CredentialsRepository } from '@db/repositories/credentials.repository'; @@ -25,6 +25,7 @@ let globalOwnerRole: Role; let globalMemberRole: Role; let owner: User; let member: User; +let secondMember: User; let authOwnerAgent: SuperAgentTest; let authMemberAgent: SuperAgentTest; let saveCredential: SaveCredentialFunction; @@ -36,6 +37,7 @@ beforeAll(async () => { owner = await createUser({ globalRole: globalOwnerRole }); member = await createUser({ globalRole: globalMemberRole }); + secondMember = await createUser({ globalRole: globalMemberRole }); saveCredential = affixRoleToSaveCredential(credentialOwnerRole); @@ -63,7 +65,7 @@ describe('GET /credentials', () => { expect(response.body.data.length).toBe(2); // owner retrieved owner cred and member cred const savedCredentialsIds = [savedOwnerCredentialId, savedMemberCredentialId]; - response.body.data.forEach((credential: Credentials.WithOwnedByAndSharedWith) => { + response.body.data.forEach((credential: ListQuery.Credentials.WithOwnedByAndSharedWith) => { validateMainCredentialData(credential); expect('data' in credential).toBe(false); expect(savedCredentialsIds).toContain(credential.id); @@ -225,6 +227,26 @@ describe('DELETE /credentials/:id', () => { expect(deletedSharedCredential).toBeDefined(); // not deleted }); + test('should not delete non-owned but shared cred for member', async () => { + const savedCredential = await saveCredential(randomCredentialPayload(), { user: secondMember }); + + await shareCredentialWithUsers(savedCredential, [member]); + + const response = await authMemberAgent.delete(`/credentials/${savedCredential.id}`); + + expect(response.statusCode).toBe(404); + + const shellCredential = await Container.get(CredentialsRepository).findOneBy({ + id: savedCredential.id, + }); + + expect(shellCredential).toBeDefined(); // not deleted + + const deletedSharedCredential = await Container.get(SharedCredentialsRepository).findOneBy({}); + + expect(deletedSharedCredential).toBeDefined(); // not deleted + }); + test('should fail if cred not found', async () => { const response = await authOwnerAgent.delete('/credentials/123'); @@ -269,7 +291,7 @@ describe('PATCH /credentials/:id', () => { expect(sharedCredential.credentials.name).toBe(patchPayload.name); // updated }); - test('should update non-owned cred for owner', async () => { + test('should not update non-owned cred for owner', async () => { const savedCredential = await saveCredential(randomCredentialPayload(), { user: member }); const patchPayload = randomCredentialPayload(); @@ -277,25 +299,14 @@ describe('PATCH /credentials/:id', () => { .patch(`/credentials/${savedCredential.id}`) .send(patchPayload); - expect(response.statusCode).toBe(200); - - const { id, name, type, nodesAccess, data: encryptedData } = response.body.data; - - expect(name).toBe(patchPayload.name); - expect(type).toBe(patchPayload.type); - - if (!patchPayload.nodesAccess) { - fail('Payload did not contain a nodesAccess array'); - } - expect(nodesAccess[0].nodeType).toBe(patchPayload.nodesAccess[0].nodeType); - - expect(encryptedData).not.toBe(patchPayload.data); + expect(response.statusCode).toBe(404); - const credential = await Container.get(CredentialsRepository).findOneByOrFail({ id }); + const credential = await Container.get(CredentialsRepository).findOneByOrFail({ + id: savedCredential.id, + }); - expect(credential.name).toBe(patchPayload.name); - expect(credential.type).toBe(patchPayload.type); - expect(credential.nodesAccess[0].nodeType).toBe(patchPayload.nodesAccess[0].nodeType); + expect(credential.name).not.toBe(patchPayload.name); + expect(credential.type).not.toBe(patchPayload.type); expect(credential.data).not.toBe(patchPayload.data); const sharedCredential = await Container.get(SharedCredentialsRepository).findOneOrFail({ @@ -303,7 +314,7 @@ describe('PATCH /credentials/:id', () => { where: { credentialsId: credential.id }, }); - expect(sharedCredential.credentials.name).toBe(patchPayload.name); // updated + expect(sharedCredential.credentials.name).not.toBe(patchPayload.name); // updated }); test('should update owned cred for member', async () => { @@ -360,6 +371,42 @@ describe('PATCH /credentials/:id', () => { expect(shellCredential.name).not.toBe(patchPayload.name); // not updated }); + test('should not update non-owned but shared cred for member', async () => { + const savedCredential = await saveCredential(randomCredentialPayload(), { user: secondMember }); + await shareCredentialWithUsers(savedCredential, [member]); + const patchPayload = randomCredentialPayload(); + + const response = await authMemberAgent + .patch(`/credentials/${savedCredential.id}`) + .send(patchPayload); + + expect(response.statusCode).toBe(404); + + const shellCredential = await Container.get(CredentialsRepository).findOneByOrFail({ + id: savedCredential.id, + }); + + expect(shellCredential.name).not.toBe(patchPayload.name); // not updated + }); + + test('should not update non-owned but shared cred for instance owner', async () => { + const savedCredential = await saveCredential(randomCredentialPayload(), { user: secondMember }); + await shareCredentialWithUsers(savedCredential, [owner]); + const patchPayload = randomCredentialPayload(); + + const response = await authOwnerAgent + .patch(`/credentials/${savedCredential.id}`) + .send(patchPayload); + + expect(response.statusCode).toBe(404); + + const shellCredential = await Container.get(CredentialsRepository).findOneByOrFail({ + id: savedCredential.id, + }); + + expect(shellCredential.name).not.toBe(patchPayload.name); // not updated + }); + test('should fail with invalid inputs', async () => { const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner }); @@ -497,7 +544,7 @@ describe('GET /credentials/:id', () => { }); }); -function validateMainCredentialData(credential: Credentials.WithOwnedByAndSharedWith) { +function validateMainCredentialData(credential: ListQuery.Credentials.WithOwnedByAndSharedWith) { const { name, type, nodesAccess, sharedWith, ownedBy } = credential; expect(typeof name).toBe('string');