fix: Remove variable:read as a valid API key scope and make variable:update selectable in the UI (no-changelog) (#15356)

Co-authored-by: Ricardo Espinoza <ricardo@n8n.io>

packages/@n8n/permissions/src/constants.ee.ts

@@ -30,7 +30,7 @@ export const RESOURCES = {
 export const API_KEY_RESOURCES = {
 	tag: [...DEFAULT_OPERATIONS] as const,
 	workflow: [...DEFAULT_OPERATIONS, 'move', 'activate', 'deactivate'] as const,
-	variable: DEFAULT_OPERATIONS,
+	variable: ['create', 'update', 'delete', 'list'] as const,
 	securityAudit: ['generate'] as const,
 	project: ['create', 'update', 'delete', 'list'] as const,
 	user: ['read', 'list', 'create', 'changeRole', 'delete'] as const,

packages/@n8n/permissions/src/public-api-permissions.ee.ts

@@ -15,6 +15,7 @@ export const OWNER_API_KEY_SCOPES: ApiKeyScope[] = [
 	'variable:create',
 	'variable:delete',
 	'variable:list',
+	'variable:update',
 	'tag:create',
 	'tag:read',
 	'tag:update',

packages/cli/test/integration/public-api/endpoints-with-scopes-enabled.test.ts

@@ -1,4 +1,4 @@
-import type { TagEntity } from '@n8n/db';
+import type { TagEntity, Variables } from '@n8n/db';
 import { ApiKeyRepository } from '@n8n/db';
 import { CredentialsRepository } from '@n8n/db';
 import { ProjectRepository } from '@n8n/db';
@@ -22,7 +22,7 @@ import {
 	createUser,
 	getUserById,
 } from '@test-integration/db/users';
-import { createVariable, getVariableOrFail } from '@test-integration/db/variables';
+import { createVariable, getVariableByIdOrFail } from '@test-integration/db/variables';
 import { createWorkflow } from '@test-integration/db/workflows';
 import { randomName } from '@test-integration/random';
 import type { CredentialPayload, SaveCredentialFunction } from '@test-integration/types';
@@ -57,6 +57,7 @@ describe('Public API endpoints with feat:apiKeyScopes enabled', () => {
 			'quota:maxTeamProjects': -1,
 		},
 	});
+
 	let apiKeyRepository: ApiKeyRepository;
 
 	beforeAll(async () => {
@@ -979,7 +980,7 @@ describe('Public API endpoints with feat:apiKeyScopes enabled', () => {
 					 * Assert
 					 */
 					expect(response.status).toBe(201);
-					await expect(getVariableOrFail(response.body.id)).resolves.toEqual(
+					await expect(getVariableByIdOrFail(response.body.id)).resolves.toEqual(
 						expect.objectContaining(variablePayload),
 					);
 				});
@@ -1005,6 +1006,36 @@ describe('Public API endpoints with feat:apiKeyScopes enabled', () => {
 					expect(response.status).toBe(403);
 				});
 			});
+
+			describe('PUT /variables/:id', () => {
+				const variablePayload = { key: 'updatedKey', value: 'updatedValue' };
+				let variable: Variables;
+				beforeEach(async () => {
+					variable = await createVariable();
+				});
+
+				it('should update a variable if API key has scope "variable:update"', async () => {
+					const owner = await createOwnerWithApiKey({ scopes: ['variable:update'] });
+
+					const response = await testServer
+						.publicApiAgentFor(owner)
+						.put(`/variables/${variable.id}`)
+						.send(variablePayload);
+
+					expect(response.status).toBe(204);
+					const updatedVariable = await getVariableByIdOrFail(variable.id);
+					expect(updatedVariable).toEqual(expect.objectContaining(variablePayload));
+				});
+
+				test('should fail to update variable when API key doesn\'t have "variable:update" scope', async () => {
+					const owner = await createOwnerWithApiKey({ scopes: ['variable:list'] });
+
+					await testServer
+						.publicApiAgentFor(owner)
+						.put(`/variables/${variable.id}`)
+						.send(variablePayload);
+				});
+			});
 		});
 
 		describe('projects', () => {

packages/cli/test/integration/public-api/variables.test.ts

@@ -2,7 +2,7 @@ import type { User, Variables } from '@n8n/db';
 
 import { FeatureNotLicensedError } from '@/errors/feature-not-licensed.error';
 import { createOwnerWithApiKey } from '@test-integration/db/users';
-import { createVariable, getVariableOrFail } from '@test-integration/db/variables';
+import { createVariable, getVariableByIdOrFail } from '@test-integration/db/variables';
 import { setupTestServer } from '@test-integration/utils';
 
 import * as testDb from '../shared/test-db';
@@ -83,7 +83,7 @@ describe('Variables in Public API', () => {
 			 * Assert
 			 */
 			expect(response.status).toBe(201);
-			await expect(getVariableOrFail(response.body.id)).resolves.toEqual(
+			await expect(getVariableByIdOrFail(response.body.id)).resolves.toEqual(
 				expect.objectContaining(variablePayload),
 			);
 		});
@@ -126,7 +126,7 @@ describe('Variables in Public API', () => {
 				.send(variablePayload);
 
 			expect(response.status).toBe(204);
-			const updatedVariable = await getVariableOrFail(variable.id);
+			const updatedVariable = await getVariableByIdOrFail(variable.id);
 			expect(updatedVariable).toEqual(expect.objectContaining(variablePayload));
 		});
 
@@ -164,7 +164,7 @@ describe('Variables in Public API', () => {
 			 * Assert
 			 */
 			expect(response.status).toBe(204);
-			await expect(getVariableOrFail(variable.id)).rejects.toThrow();
+			await expect(getVariableByIdOrFail(variable.id)).rejects.toThrow();
 		});
 
 		it('if not licensed, should reject', async () => {

packages/cli/test/integration/shared/db/variables.ts

@@ -3,10 +3,34 @@ import { VariablesRepository } from '@n8n/db';
 import { Container } from '@n8n/di';
 import { randomString } from 'n8n-workflow';
 
+import { VariablesService } from '@/environments.ee/variables/variables.service.ee';
+
 export async function createVariable(key = randomString(5), value = randomString(5)) {
-	return await Container.get(VariablesRepository).save({ id: generateNanoId(), key, value });
+	const result = await Container.get(VariablesRepository).save({
+		id: generateNanoId(),
+		key,
+		value,
+	});
+	await Container.get(VariablesService).updateCache();
+	return result;
 }
 
-export async function getVariableOrFail(id: string) {
+export async function getVariableByIdOrFail(id: string) {
 	return await Container.get(VariablesRepository).findOneOrFail({ where: { id } });
 }
+
+export async function getVariableByKey(key: string) {
+	return await Container.get(VariablesRepository).findOne({
+		where: {
+			key,
+		},
+	});
+}
+
+export async function getVariableById(id: string) {
+	return await Container.get(VariablesRepository).findOne({
+		where: {
+			id,
+		},
+	});
+}

packages/cli/test/integration/variables.test.ts

@@ -1,10 +1,8 @@
 import type { Variables } from '@n8n/db';
-import { generateNanoId } from '@n8n/db';
-import { VariablesRepository } from '@n8n/db';
 import { Container } from '@n8n/di';
 
-import { VariablesService } from '@/environments.ee/variables/variables.service.ee';
 import { CacheService } from '@/services/cache/cache.service';
+import { createVariable, getVariableById, getVariableByKey } from '@test-integration/db/variables';
 
 import { createOwner, createUser } from './shared/db/users';
 import * as testDb from './shared/test-db';
@@ -17,32 +15,6 @@ let authMemberAgent: SuperAgentTest;
 const testServer = utils.setupTestServer({ endpointGroups: ['variables'] });
 const license = testServer.license;
 
-async function createVariable(key: string, value: string) {
-	const result = await Container.get(VariablesRepository).save({
-		id: generateNanoId(),
-		key,
-		value,
-	});
-	await Container.get(VariablesService).updateCache();
-	return result;
-}
-
-async function getVariableByKey(key: string) {
-	return await Container.get(VariablesRepository).findOne({
-		where: {
-			key,
-		},
-	});
-}
-
-async function getVariableById(id: string) {
-	return await Container.get(VariablesRepository).findOne({
-		where: {
-			id,
-		},
-	});
-}
-
 beforeAll(async () => {
 	const owner = await createOwner();
 	authOwnerAgent = testServer.authAgentFor(owner);