feat: Allow instance owners and admins to edit all credentials (#8716)

Co-authored-by: Danny Martini <despair.blue@gmail.com>

packages/cli/src/credentials/credentials.controller.ts

@@ -74,7 +74,9 @@ export class CredentialsController {
 
 			credential = this.ownershipService.addOwnedByAndSharedWith(credential);
 
-			if (!includeDecryptedData || !userSharing || userSharing.role !== 'credential:owner') {
+			// Below, if `userSharing` does not exist, it means this credential is being
+			// fetched by the instance owner or an admin. In this case, they get the full data
+			if (!includeDecryptedData || userSharing?.role === 'credential:user') {
 				const { data: _, ...rest } = credential;
 				return { ...rest };
 			}

packages/cli/src/permissions/roles.ts

@@ -4,6 +4,7 @@ export const ownerPermissions: Scope[] = [
 	'auditLogs:manage',
 	'credential:create',
 	'credential:read',
+	'credential:update',
 	'credential:delete',
 	'credential:list',
 	'credential:share',

packages/cli/test/integration/credentials.ee.test.ts

@@ -229,7 +229,7 @@ describe('GET /credentials/:id', () => {
 		expect(response2.statusCode).toBe(200);
 
 		validateMainCredentialData(response2.body.data);
-		expect(response2.body.data.data).toBeUndefined();
+		expect(response2.body.data.data).toBeDefined(); // Instance owners should be capable of editing all credentials
 		expect(response2.body.data.sharedWith).toHaveLength(1);
 	});
 

packages/cli/test/integration/credentials.test.ts

@@ -14,6 +14,7 @@ import type { SaveCredentialFunction } from './shared/types';
 import * as utils from './shared/utils/';
 import { affixRoleToSaveCredential, shareCredentialWithUsers } from './shared/db/credentials';
 import { createManyUsers, createUser } from './shared/db/users';
+import { Credentials } from 'n8n-core';
 
 // mock that credentialsSharing is not enabled
 jest.spyOn(License.prototype, 'isSharingEnabled').mockReturnValue(false);
@@ -283,7 +284,7 @@ describe('PATCH /credentials/:id', () => {
 		expect(sharedCredential.credentials.name).toBe(patchPayload.name); // updated
 	});
 
-	test('should not update non-owned cred for owner', async () => {
+	test('should update non-owned cred for owner', async () => {
 		const savedCredential = await saveCredential(randomCredentialPayload(), { user: member });
 		const patchPayload = randomCredentialPayload();
 
@@ -291,22 +292,29 @@ describe('PATCH /credentials/:id', () => {
 			.patch(`/credentials/${savedCredential.id}`)
 			.send(patchPayload);
 
-		expect(response.statusCode).toBe(404);
+		expect(response.statusCode).toBe(200);
 
 		const credential = await Container.get(CredentialsRepository).findOneByOrFail({
 			id: savedCredential.id,
 		});
 
-		expect(credential.name).not.toBe(patchPayload.name);
+		expect(credential.name).toBe(patchPayload.name);
-		expect(credential.type).not.toBe(patchPayload.type);
+		expect(credential.type).toBe(patchPayload.type);
-		expect(credential.data).not.toBe(patchPayload.data);
+
+		const credentialObject = new Credentials(
+			{ id: credential.id, name: credential.name },
+			credential.type,
+			credential.nodesAccess,
+			credential.data,
+		);
+		expect(credentialObject.getData()).toStrictEqual(patchPayload.data);
 
 		const sharedCredential = await Container.get(SharedCredentialsRepository).findOneOrFail({
 			relations: ['credentials'],
 			where: { credentialsId: credential.id },
 		});
 
-		expect(sharedCredential.credentials.name).not.toBe(patchPayload.name); // updated
+		expect(sharedCredential.credentials.name).toBe(patchPayload.name); // updated
 	});
 
 	test('should update owned cred for member', async () => {
@@ -381,7 +389,7 @@ describe('PATCH /credentials/:id', () => {
 		expect(shellCredential.name).not.toBe(patchPayload.name); // not updated
 	});
 
-	test('should not update non-owned but shared cred for instance owner', async () => {
+	test('should update non-owned but shared cred for instance owner', async () => {
 		const savedCredential = await saveCredential(randomCredentialPayload(), { user: secondMember });
 		await shareCredentialWithUsers(savedCredential, [owner]);
 		const patchPayload = randomCredentialPayload();
@@ -390,13 +398,13 @@ describe('PATCH /credentials/:id', () => {
 			.patch(`/credentials/${savedCredential.id}`)
 			.send(patchPayload);
 
-		expect(response.statusCode).toBe(404);
+		expect(response.statusCode).toBe(200);
 
 		const shellCredential = await Container.get(CredentialsRepository).findOneByOrFail({
 			id: savedCredential.id,
 		});
 
-		expect(shellCredential.name).not.toBe(patchPayload.name); // not updated
+		expect(shellCredential.name).toBe(patchPayload.name); // updated
 	});
 
 	test('should fail with invalid inputs', async () => {

packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue

@@ -6,7 +6,7 @@
 			:message="
 				$locale.baseText(
 					`credentialEdit.credentialConfig.pleaseCheckTheErrorsBelow${
-						credentialPermissions.isOwner ? '' : '.sharee'
+						credentialPermissions.update || credentialPermissions.isOwner ? '' : '.sharee'
 					}`,
 					{ interpolate: { owner: credentialOwnerName } },
 				)
@@ -19,7 +19,7 @@
 			:message="
 				$locale.baseText(
 					`credentialEdit.credentialConfig.couldntConnectWithTheseSettings${
-						credentialPermissions.isOwner ? '' : '.sharee'
+						credentialPermissions.update || credentialPermissions.isOwner ? '' : '.sharee'
 					}`,
 					{ interpolate: { owner: credentialOwnerName } },
 				)

packages/editor-ui/src/permissions.ts

@@ -89,7 +89,8 @@ export const getCredentialPermissions = (user: IUser | null, credential: ICreden
 		},
 		{
 			name: 'update',
-			test: (permissions) => !!permissions.isOwner,
+			test: (permissions) =>
+				hasPermission(['rbac'], { rbac: { scope: 'credential:update' } }) || !!permissions.isOwner,
 		},
 		{
 			name: 'share',