fix(core): Better input validation for the changeRole endpoint (#8189)

also refactored the code to
1. stop passing around `scope === 'global'`, since this code can be used
only for changing globalRole.
2. leak less details when input validation fails.

## Review / Merge checklist
- [x] PR title and summary are descriptive
- [x] Tests included

packages/cli/test/integration/users.api.test.ts

@@ -361,15 +361,8 @@ describe('PATCH /users/:id/role', () => {
 	let memberAgent: SuperAgentTest;
 	let authlessAgent: SuperAgentTest;
 
-	const {
-		MISSING_NEW_ROLE_KEY,
-		MISSING_NEW_ROLE_VALUE,
-		NO_ADMIN_ON_OWNER,
-		NO_USER_TO_OWNER,
-		NO_USER,
-		NO_OWNER_ON_OWNER,
-		NO_ADMIN_IF_UNLICENSED,
-	} = UsersController.ERROR_MESSAGES.CHANGE_ROLE;
+	const { NO_ADMIN_ON_OWNER, NO_USER, NO_OWNER_ON_OWNER } =
+		UsersController.ERROR_MESSAGES.CHANGE_ROLE;
 
 	const UNAUTHORIZED = 'Unauthorized';
 
@@ -393,17 +386,31 @@ describe('PATCH /users/:id/role', () => {
 	describe('unauthenticated user', () => {
 		test('should receive 401', async () => {
 			const response = await authlessAgent.patch(`/users/${member.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(401);
 		});
 	});
 
+	describe('Invalid payload should return 400 when newRoleName', () => {
+		test.each([
+			['is missing', {}],
+			['is `owner`', { newRoleName: 'owner' }],
+			['is an array', { newRoleName: ['owner'] }],
+		])('%s', async (_, payload) => {
+			const response = await adminAgent.patch(`/users/${member.id}/role`).send(payload);
+			expect(response.statusCode).toBe(400);
+			expect(response.body.message).toBe(
+				'newRoleName must be one of the following values: member, admin',
+			);
+		});
+	});
+
 	describe('member', () => {
 		test('should fail to demote owner to member', async () => {
 			const response = await memberAgent.patch(`/users/${owner.id}/role`).send({
-				newRole: { scope: 'global', name: 'member' },
+				newRoleName: 'member',
 			});
 
 			expect(response.statusCode).toBe(403);
@@ -412,7 +419,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should fail to demote owner to admin', async () => {
 			const response = await memberAgent.patch(`/users/${owner.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(403);
@@ -421,7 +428,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should fail to demote admin to member', async () => {
 			const response = await memberAgent.patch(`/users/${admin.id}/role`).send({
-				newRole: { scope: 'global', name: 'member' },
+				newRoleName: 'member',
 			});
 
 			expect(response.statusCode).toBe(403);
@@ -430,7 +437,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should fail to promote other member to owner', async () => {
 			const response = await memberAgent.patch(`/users/${otherMember.id}/role`).send({
-				newRole: { scope: 'global', name: 'owner' },
+				newRoleName: 'owner',
 			});
 
 			expect(response.statusCode).toBe(403);
@@ -439,7 +446,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should fail to promote other member to admin', async () => {
 			const response = await memberAgent.patch(`/users/${otherMember.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(403);
@@ -448,7 +455,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should fail to promote self to admin', async () => {
 			const response = await memberAgent.patch(`/users/${member.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(403);
@@ -457,7 +464,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should fail to promote self to owner', async () => {
 			const response = await memberAgent.patch(`/users/${member.id}/role`).send({
-				newRole: { scope: 'global', name: 'owner' },
+				newRoleName: 'owner',
 			});
 
 			expect(response.statusCode).toBe(403);
@@ -466,25 +473,11 @@ describe('PATCH /users/:id/role', () => {
 	});
 
 	describe('admin', () => {
-		test('should receive 400 on invalid payload', async () => {
-			const response = await adminAgent.patch(`/users/${member.id}/role`).send({});
-
-			expect(response.statusCode).toBe(400);
-			expect(response.body.message).toBe(MISSING_NEW_ROLE_KEY);
-
-			const _response = await adminAgent.patch(`/users/${member.id}/role`).send({
-				newRole: {},
-			});
-
-			expect(_response.statusCode).toBe(400);
-			expect(_response.body.message).toBe(MISSING_NEW_ROLE_VALUE);
-		});
-
 		test('should receive 404 on unknown target user', async () => {
 			const response = await adminAgent
 				.patch('/users/c2317ff3-7a9f-4fd4-ad2b-7331f6359260/role')
 				.send({
-					newRole: { scope: 'global', name: 'member' },
+					newRoleName: 'member',
 				});
 
 			expect(response.statusCode).toBe(404);
@@ -493,7 +486,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should fail to demote owner to admin', async () => {
 			const response = await adminAgent.patch(`/users/${owner.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(403);
@@ -502,45 +495,27 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should fail to demote owner to member', async () => {
 			const response = await adminAgent.patch(`/users/${owner.id}/role`).send({
-				newRole: { scope: 'global', name: 'member' },
+				newRoleName: 'member',
 			});
 
 			expect(response.statusCode).toBe(403);
 			expect(response.body.message).toBe(NO_ADMIN_ON_OWNER);
 		});
 
-		test('should fail to promote member to owner', async () => {
-			const response = await adminAgent.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 fail to promote admin to owner', async () => {
-			const response = await adminAgent.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 fail to promote member to admin if not licensed', async () => {
 			testServer.license.disable('feat:advancedPermissions');
 
 			const response = await adminAgent.patch(`/users/${member.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(403);
-			expect(response.body.message).toBe(NO_ADMIN_IF_UNLICENSED);
+			expect(response.body.message).toBe('Plan lacks license for this feature');
 		});
 
 		test('should be able to demote admin to member', async () => {
 			const response = await adminAgent.patch(`/users/${otherAdmin.id}/role`).send({
-				newRole: { scope: 'global', name: 'member' },
+				newRoleName: 'member',
 			});
 
 			expect(response.statusCode).toBe(200);
@@ -559,7 +534,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should be able to demote self to member', async () => {
 			const response = await adminAgent.patch(`/users/${admin.id}/role`).send({
-				newRole: { scope: 'global', name: 'member' },
+				newRoleName: 'member',
 			});
 
 			expect(response.statusCode).toBe(200);
@@ -578,7 +553,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should be able to promote member to admin if licensed', async () => {
 			const response = await adminAgent.patch(`/users/${member.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(200);
@@ -599,7 +574,7 @@ 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' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(403);
@@ -608,45 +583,27 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should fail to demote self to member', async () => {
 			const response = await ownerAgent.patch(`/users/${owner.id}/role`).send({
-				newRole: { scope: 'global', name: 'member' },
+				newRoleName: '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 fail to promote member to admin if not licensed', async () => {
 			testServer.license.disable('feat:advancedPermissions');
 
 			const response = await ownerAgent.patch(`/users/${member.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(403);
-			expect(response.body.message).toBe(NO_ADMIN_IF_UNLICENSED);
+			expect(response.body.message).toBe('Plan lacks license for this feature');
 		});
 
 		test('should be able to promote member to admin if licensed', async () => {
 			const response = await ownerAgent.patch(`/users/${member.id}/role`).send({
-				newRole: { scope: 'global', name: 'admin' },
+				newRoleName: 'admin',
 			});
 
 			expect(response.statusCode).toBe(200);
@@ -665,7 +622,7 @@ describe('PATCH /users/:id/role', () => {
 
 		test('should be able to demote admin to member', async () => {
 			const response = await ownerAgent.patch(`/users/${admin.id}/role`).send({
-				newRole: { scope: 'global', name: 'member' },
+				newRoleName: 'member',
 			});
 
 			expect(response.statusCode).toBe(200);