fix(core): Ensure member and admin cannot be promoted to owner (#7830)

https://linear.app/n8n/issue/PAY-985/add-user-role-modification-endpoint#comment-62355f6b
n8n-io · Nov 27, 2023 · 9b87a59 · 9b87a59
1 parent ac744d6
commit 9b87a59
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 30 deletions.
diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts
@@ -40,7 +40,7 @@ export class UsersController {
 			NO_USER: 'Target user not found',
 			NO_ADMIN_ON_OWNER: 'Admin cannot change role on global owner',
 			NO_OWNER_ON_OWNER: 'Owner cannot change role on global owner',
-			NO_ADMIN_TO_OWNER: 'Admin cannot promote user to global owner',
+			NO_USER_TO_OWNER: 'Cannot promote user to global owner',
 		},
 	} as const;
 
@@ -330,7 +330,7 @@ export class UsersController {
 			MISSING_NEW_ROLE_KEY,
 			MISSING_NEW_ROLE_VALUE,
 			NO_ADMIN_ON_OWNER,
-			NO_ADMIN_TO_OWNER,
+			NO_USER_TO_OWNER,
 			NO_USER,
 			NO_OWNER_ON_OWNER,
 		} = UsersController.ERROR_MESSAGES.CHANGE_ROLE;
@@ -349,13 +349,8 @@ export class UsersController {
 			throw new BadRequestError(MISSING_NEW_ROLE_VALUE);
 		}
 
-		if (
-			req.user.globalRole.scope === 'global' &&
-			req.user.globalRole.name === 'admin' &&
-			newRole.scope === 'global' &&
-			newRole.name === 'owner'
-		) {
-			throw new UnauthorizedError(NO_ADMIN_TO_OWNER);
+		if (newRole.scope === 'global' && newRole.name === 'owner') {
+			throw new UnauthorizedError(NO_USER_TO_OWNER);
 		}
 
 		const targetUser = await this.userService.findOne({

diff --git a/packages/cli/test/integration/users.api.test.ts b/packages/cli/test/integration/users.api.test.ts
@@ -359,7 +359,7 @@ describe('PATCH /users/:id/role', () => {
 		MISSING_NEW_ROLE_KEY,
 		MISSING_NEW_ROLE_VALUE,
 		NO_ADMIN_ON_OWNER,
-		NO_ADMIN_TO_OWNER,
+		NO_USER_TO_OWNER,
 		NO_USER,
 		NO_OWNER_ON_OWNER,
 	} = UsersController.ERROR_MESSAGES.CHANGE_ROLE;
@@ -506,7 +506,7 @@ describe('PATCH /users/:id/role', () => {
 			});
 
 			expect(response.statusCode).toBe(403);
-			expect(response.body.message).toBe(NO_ADMIN_TO_OWNER);
+			expect(response.body.message).toBe(NO_USER_TO_OWNER);
 		});
 
 		test('should fail to promote admin to owner', async () => {
@@ -515,7 +515,7 @@ describe('PATCH /users/:id/role', () => {
 			});
 
 			expect(response.statusCode).toBe(403);
-			expect(response.body.message).toBe(NO_ADMIN_TO_OWNER);
+			expect(response.body.message).toBe(NO_USER_TO_OWNER);
 		});
 
 		test('should be able to demote admin to member', async () => {
@@ -577,6 +577,42 @@ describe('PATCH /users/:id/role', () => {
 	});
 
 	describe('owner', () => {
+		test('should fail to demote self to admin', async () => {
+			const response = await ownerAgent.patch(`/users/${owner.id}/role`).send({
+				newRole: { scope: 'global', name: 'admin' },
+			});
+
+			expect(response.statusCode).toBe(403);
+			expect(response.body.message).toBe(NO_OWNER_ON_OWNER);
+		});
+
+		test('should fail to demote self to member', async () => {
+			const response = await ownerAgent.patch(`/users/${owner.id}/role`).send({
+				newRole: { scope: 'global', name: 'member' },
+			});
+
+			expect(response.statusCode).toBe(403);
+			expect(response.body.message).toBe(NO_OWNER_ON_OWNER);
+		});
+
+		test('should fail to promote admin to owner', async () => {
+			const response = await ownerAgent.patch(`/users/${admin.id}/role`).send({
+				newRole: { scope: 'global', name: 'owner' },
+			});
+
+			expect(response.statusCode).toBe(403);
+			expect(response.body.message).toBe(NO_USER_TO_OWNER);
+		});
+
+		test('should fail to promote member to owner', async () => {
+			const response = await ownerAgent.patch(`/users/${member.id}/role`).send({
+				newRole: { scope: 'global', name: 'owner' },
+			});
+
+			expect(response.statusCode).toBe(403);
+			expect(response.body.message).toBe(NO_USER_TO_OWNER);
+		});
+
 		test('should be able to promote member to admin', async () => {
 			const response = await ownerAgent.patch(`/users/${member.id}/role`).send({
 				newRole: { scope: 'global', name: 'admin' },
@@ -614,23 +650,5 @@ describe('PATCH /users/:id/role', () => {
 			admin = await createAdmin();
 			adminAgent = testServer.authAgentFor(admin);
 		});
-
-		test('should fail to demote self to admin', async () => {
-			const response = await ownerAgent.patch(`/users/${owner.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
-			});
-
-			expect(response.statusCode).toBe(403);
-			expect(response.body.message).toBe(NO_OWNER_ON_OWNER);
-		});
-
-		test('should fail to demote self to member', async () => {
-			const response = await ownerAgent.patch(`/users/${owner.id}/role`).send({
-				newRole: { scope: 'global', name: 'member' },
-			});
-
-			expect(response.statusCode).toBe(403);
-			expect(response.body.message).toBe(NO_OWNER_ON_OWNER);
-		});
 	});
 });