refactor(core,phrases,test): add user role APIs guard (#4382)

refactor(core,phrases): update roles API logics since type field is added to roles

refactor(test): add integration test cases

fix(core): should not manually filter out m2m role assigned to user

chore: silently filter out existing entities

packages/core/src/__mocks__/index.ts

@@ -74,6 +74,14 @@ export const mockAdminUserRole2: Role = {
   type: RoleType.User,
 };
 
+export const mockAdminUserRole3: Role = {
+  tenantId: 'fake_tenant',
+  id: 'role_id3',
+  name: 'admin3',
+  description: 'admin3',
+  type: RoleType.MachineToMachine,
+};
+
 export const mockAdminConsoleData: AdminConsoleData = {
   livePreviewChecked: false,
   applicationCreated: false,

packages/core/src/queries/users-roles.ts

@@ -1,5 +1,5 @@
 import type { CreateUsersRole, UsersRole } from '@logto/schemas';
-import { RolesScopes, UsersRoles } from '@logto/schemas';
+import { UsersRoles } from '@logto/schemas';
 import { conditionalSql, convertToIdentifiers } from '@logto/shared';
 import type { CommonQueryMethods } from 'slonik';
 import { sql } from 'slonik';
@@ -58,7 +58,7 @@ export const createUsersRolesQueries = (pool: CommonQueryMethods) => {
     `);
 
     if (rowCount < 1) {
-      throw new DeletionError(RolesScopes.table);
+      throw new DeletionError(UsersRoles.table);
     }
   };
 

packages/core/src/routes/admin-user-role.test.ts

@@ -1,9 +1,11 @@
+import { RoleType } from '@logto/schemas';
 import { pickDefault } from '@logto/shared/esm';
 
 import {
   mockAdminUserRole,
   mockUser,
   mockAdminUserRole2,
+  mockAdminUserRole3,
   mockUserRole,
 } from '#src/__mocks__/index.js';
 import { mockId, mockStandardId } from '#src/test-utils/nanoid.js';
@@ -29,6 +31,7 @@ const usersRoles = {
   deleteUsersRolesByUserIdAndRoleId: jest.fn(),
 };
 const { findUsersRolesByUserId, insertUsersRoles, deleteUsersRolesByUserIdAndRoleId } = usersRoles;
+const { findRolesByRoleIds } = roles;
 
 const tenantContext = new MockTenant(undefined, { usersRoles, users, roles });
 
@@ -44,30 +47,62 @@ describe('user role routes', () => {
     expect(response.body).toEqual([mockAdminUserRole]);
   });
 
-  it('POST /users/:id/roles', async () => {
-    findUsersRolesByUserId.mockResolvedValueOnce([]);
-    const response = await roleRequester.post(`/users/${mockUser.id}/roles`).send({
-      roleIds: [mockAdminUserRole.id],
+  describe('POST /users/:id/roles', () => {
+    it('Succeed', async () => {
+      findUsersRolesByUserId.mockResolvedValueOnce([]);
+      findRolesByRoleIds.mockResolvedValueOnce([mockAdminUserRole]);
+      const response = await roleRequester.post(`/users/${mockUser.id}/roles`).send({
+        roleIds: [mockAdminUserRole.id],
+      });
+      expect(response.status).toEqual(201);
+      expect(insertUsersRoles).toHaveBeenCalledWith([
+        { id: mockId, userId: mockUser.id, roleId: mockAdminUserRole.id },
+      ]);
+      insertUsersRoles.mockClear();
+    });
+
+    it('Can not assign machine to machine roles to user', async () => {
+      findUsersRolesByUserId.mockResolvedValueOnce([]);
+      findRolesByRoleIds.mockResolvedValueOnce([
+        { ...mockAdminUserRole, type: RoleType.MachineToMachine },
+      ]);
+      const response = await roleRequester.post(`/users/${mockUser.id}/roles`).send({
+        roleIds: [mockAdminUserRole.id],
+      });
+      expect(response.status).toEqual(422);
     });
-    expect(response.status).toEqual(201);
-    expect(insertUsersRoles).toHaveBeenCalledWith([
-      { id: mockId, userId: mockUser.id, roleId: mockAdminUserRole.id },
-    ]);
   });
 
-  it('PUT /users/:id/roles', async () => {
-    findUsersRolesByUserId.mockResolvedValueOnce([mockUserRole]);
-    const response = await roleRequester.put(`/users/${mockUser.id}/roles`).send({
-      roleIds: [mockAdminUserRole2.id],
+  describe('PUT /users/:id/roles', () => {
+    it('Succeed', async () => {
+      findUsersRolesByUserId.mockResolvedValueOnce([mockUserRole]);
+      findRolesByRoleIds.mockResolvedValueOnce([mockAdminUserRole2]);
+      const response = await roleRequester.put(`/users/${mockUser.id}/roles`).send({
+        roleIds: [mockAdminUserRole2.id],
+      });
+      expect(response.status).toEqual(200);
+      expect(deleteUsersRolesByUserIdAndRoleId).toHaveBeenCalledWith(
+        mockUser.id,
+        mockUserRole.roleId
+      );
+      expect(insertUsersRoles).toHaveBeenCalledWith([
+        { id: mockId, userId: mockUser.id, roleId: mockAdminUserRole2.id },
+      ]);
+      insertUsersRoles.mockClear();
+      deleteUsersRolesByUserIdAndRoleId.mockClear();
+    });
+
+    it('Machine to machine roles will be filtered out', async () => {
+      findUsersRolesByUserId.mockResolvedValueOnce([
+        { ...mockUserRole, roleId: mockAdminUserRole3.id },
+      ]);
+      const response = await roleRequester.put(`/users/${mockUser.id}/roles`).send({
+        roleIds: [mockAdminUserRole3.id],
+      });
+      expect(response.status).toEqual(200);
+      expect(deleteUsersRolesByUserIdAndRoleId).not.toHaveBeenCalled();
+      expect(insertUsersRoles).toHaveBeenCalledWith([]);
     });
-    expect(response.status).toEqual(200);
-    expect(deleteUsersRolesByUserIdAndRoleId).toHaveBeenCalledWith(
-      mockUser.id,
-      mockAdminUserRole.id
-    );
-    expect(insertUsersRoles).toHaveBeenCalledWith([
-      { id: mockId, userId: mockUser.id, roleId: mockAdminUserRole2.id },
-    ]);
   });
 
   it('DELETE /users/:id/roles/:roleId', async () => {

packages/core/src/routes/admin-user-role.ts

@@ -1,4 +1,4 @@
-import { Roles } from '@logto/schemas';
+import { RoleType, Roles } from '@logto/schemas';
 import { generateStandardId } from '@logto/shared';
 import { tryThat } from '@silverhand/essentials';
 import { array, object, string } from 'zod';
@@ -16,7 +16,7 @@ export default function adminUserRoleRoutes<T extends AuthedRouter>(
   ...[router, { queries }]: RouterInitArgs<T>
 ) {
   const {
-    roles: { findRoleById, countRoles, findRoles },
+    roles: { findRoleById, countRoles, findRoles, findRolesByRoleIds },
     users: { findUserById },
     usersRoles: { deleteUsersRolesByUserIdAndRoleId, findUsersRolesByUserId, insertUsersRoles },
   } = queries;
@@ -82,22 +82,24 @@ export default function adminUserRoleRoutes<T extends AuthedRouter>(
 
       await findUserById(userId);
       const usersRoles = await findUsersRolesByUserId(userId);
+      const existingRoleIds = new Set(usersRoles.map(({ roleId }) => roleId));
+      const roleIdsToAdd = roleIds.filter((id) => !existingRoleIds.has(id)); // ignore existing roles.
+      const roles = await findRolesByRoleIds(roleIdsToAdd);
 
-      for (const roleId of roleIds) {
+      for (const role of roles) {
         assertThat(
-          !usersRoles.some(({ roleId: _roleId }) => _roleId === roleId),
-          new RequestError({
-            code: 'user.role_exists',
-            status: 422,
-            roleId,
-          })
+          role.type === RoleType.User,
+          new RequestError({ code: 'user.invalid_role_type', status: 422, roleId: role.id })
+        );
+      }
+
+      if (roleIdsToAdd.length > 0) {
+        await Promise.all(roleIdsToAdd.map(async (roleId) => findRoleById(roleId)));
+        await insertUsersRoles(
+          roleIdsToAdd.map((roleId) => ({ id: generateStandardId(), userId, roleId }))
         );
       }
 
-      await Promise.all(roleIds.map(async (roleId) => findRoleById(roleId)));
-      await insertUsersRoles(
-        roleIds.map((roleId) => ({ id: generateStandardId(), userId, roleId }))
-      );
       ctx.status = 201;
 
       return next();

packages/core/src/routes/role.test.ts

@@ -1,4 +1,5 @@
 import type { Role } from '@logto/schemas';
+import { RoleType } from '@logto/schemas';
 import { pickDefault } from '@logto/shared/esm';
 
 import { mockAdminUserRole, mockScope, mockUser, mockResource } from '#src/__mocks__/index.js';
@@ -60,14 +61,12 @@ const usersRoles = {
   insertUsersRoles: jest.fn(),
   countUsersRolesByRoleId: jest.fn(),
   findUsersRolesByRoleId: jest.fn(),
-  findFirstUsersRolesByRoleIdAndUserIds: jest.fn(),
   deleteUsersRolesByUserIdAndRoleId: jest.fn(),
 };
 const {
   insertUsersRoles,
   findUsersRolesByRoleId,
   deleteUsersRolesByUserIdAndRoleId,
-  findFirstUsersRolesByRoleIdAndUserIds,
   countUsersRolesByRoleId,
 } = usersRoles;
 
@@ -178,16 +177,29 @@ describe('role routes', () => {
     expect(response.body[0]).toHaveProperty('id', mockUser.id);
   });
 
-  it('POST /roles/:id/users', async () => {
-    findRoleById.mockResolvedValueOnce(mockAdminUserRole);
-    findFirstUsersRolesByRoleIdAndUserIds.mockResolvedValueOnce(null);
-    const response = await roleRequester.post(`/roles/${mockAdminUserRole.id}/users`).send({
-      userIds: [mockUser.id],
+  describe('POST /roles/:id/users', () => {
+    it('Succeed', async () => {
+      findRoleById.mockResolvedValueOnce(mockAdminUserRole);
+      findUsersRolesByRoleId.mockResolvedValueOnce([]);
+      const response = await roleRequester.post(`/roles/${mockAdminUserRole.id}/users`).send({
+        userIds: [mockUser.id],
+      });
+      expect(response.status).toEqual(201);
+      expect(insertUsersRoles).toHaveBeenCalledWith([
+        { id: mockId, userId: mockUser.id, roleId: mockAdminUserRole.id },
+      ]);
+    });
+
+    it('Should throw error when trying to assign machine to machine role to users', async () => {
+      const mockMachineToMachineRole = { ...mockAdminUserRole, type: RoleType.MachineToMachine };
+      findRoleById.mockResolvedValueOnce(mockMachineToMachineRole);
+      const response = await roleRequester
+        .post(`/roles/${mockMachineToMachineRole.id}/users`)
+        .send({
+          userIds: [mockUser.id],
+        });
+      expect(response.status).toEqual(422);
     });
-    expect(response.status).toEqual(201);
-    expect(insertUsersRoles).toHaveBeenCalledWith([
-      { id: mockId, userId: mockUser.id, roleId: mockAdminUserRole.id },
-    ]);
   });
 
   it('DELETE /roles/:id/users/:userId', async () => {

packages/core/src/routes/role.ts

@@ -1,5 +1,11 @@
 import type { RoleResponse } from '@logto/schemas';
-import { userInfoSelectFields, userProfileResponseGuard, Roles, Users } from '@logto/schemas';
+import {
+  userInfoSelectFields,
+  userProfileResponseGuard,
+  Roles,
+  RoleType,
+  Users,
+} from '@logto/schemas';
 import { generateStandardId } from '@logto/shared';
 import { pick, tryThat } from '@silverhand/essentials';
 import { object, string, z, number } from 'zod';
@@ -39,7 +45,6 @@ export default function roleRoutes<T extends AuthedRouter>(
     usersRoles: {
       countUsersRolesByRoleId,
       deleteUsersRolesByUserIdAndRoleId,
-      findFirstUsersRolesByRoleIdAndUserIds,
       findUsersRolesByRoleId,
       findUsersRolesByUserId,
       insertUsersRoles,
@@ -282,21 +287,22 @@ export default function roleRoutes<T extends AuthedRouter>(
         body: { userIds },
       } = ctx.guard;
 
-      await findRoleById(id);
-      const existingRecord = await findFirstUsersRolesByRoleIdAndUserIds(id, userIds);
-
-      if (existingRecord) {
-        throw new RequestError({
-          code: 'role.user_exists',
-          status: 422,
-          userId: existingRecord.userId,
-        });
-      }
-
-      await Promise.all(userIds.map(async (userId) => findUserById(userId)));
-      await insertUsersRoles(
-        userIds.map((userId) => ({ id: generateStandardId(), roleId: id, userId }))
+      const role = await findRoleById(id);
+      assertThat(
+        role.type === RoleType.User,
+        new RequestError({ code: 'user.invalid_role_type', status: 422, roleId: role.id })
       );
+
+      const usersRoles = await findUsersRolesByRoleId(id);
+      const existingUserIds = new Set(usersRoles.map(({ userId }) => userId));
+      const userIdsToAdd = userIds.filter((id) => !existingUserIds.has(id)); // Skip existing user ids.
+
+      if (userIdsToAdd.length > 0) {
+        await Promise.all(userIdsToAdd.map(async (userId) => findUserById(userId)));
+        await insertUsersRoles(
+          userIdsToAdd.map((userId) => ({ id: generateStandardId(), roleId: id, userId }))
+        );
+      }
       ctx.status = 201;
 
       return next();

packages/integration-tests/src/tests/api/admin-user.roles.test.ts

@@ -1,3 +1,4 @@
+import { RoleType } from '@logto/schemas';
 import { HTTPError } from 'got';
 
 import { assignRolesToUser, getUserRoles, deleteRoleFromUser } from '#src/api/index.js';
@@ -12,12 +13,19 @@ describe('admin console user management (roles)', () => {
     expect(roles.length).toBe(0);
   });
 
-  it('should assign role to user and get list successfully', async () => {
+  it('should successfully assign user role to user and get list, but failed to assign m2m role to user', async () => {
     const user = await createUserByAdmin();
     const role = await createRole({});
 
+    const m2mRole = await createRole({ type: RoleType.MachineToMachine });
+    await expectRejects(assignRolesToUser(user.id, [m2mRole.id]), {
+      code: 'user.invalid_role_type',
+      statusCode: 422,
+    });
+
     await assignRolesToUser(user.id, [role.id]);
     const roles = await getUserRoles(user.id);
+    expect(roles.length).toBe(1);
     expect(roles[0]).toHaveProperty('id', role.id);
   });
 
@@ -26,10 +34,10 @@ describe('admin console user management (roles)', () => {
     const role = await createRole({});
 
     await assignRolesToUser(user.id, [role.id]);
-    await expectRejects(assignRolesToUser(user.id, [role.id]), {
-      code: 'user.role_exists',
-      statusCode: 422,
-    });
+    await assignRolesToUser(user.id, [role.id]);
+    const roles = await getUserRoles(user.id);
+    expect(roles.length).toBe(1);
+    expect(roles[0]).toHaveProperty('id', role.id);
   });
 
   it('should delete role from user successfully', async () => {

packages/integration-tests/src/tests/api/role.user.test.ts

@@ -1,7 +1,9 @@
+import { RoleType } from '@logto/schemas';
 import { HTTPError } from 'got';
 
 import { createUser } from '#src/api/index.js';
 import { assignUsersToRole, createRole, deleteUserFromRole, getRoleUsers } from '#src/api/role.js';
+import { expectRejects } from '#src/helpers/index.js';
 import { generateNewUserProfile } from '#src/helpers/user.js';
 
 describe('roles users', () => {
@@ -30,6 +32,18 @@ describe('roles users', () => {
     expect(users.length).toBe(2);
   });
 
+  it('should throw when assigning users to m2m role', async () => {
+    const m2mRole = await createRole({ type: RoleType.MachineToMachine });
+    const user = await createUser(generateNewUserProfile({}));
+    await expectRejects(assignUsersToRole([user.id], m2mRole.id), {
+      code: 'user.invalid_role_type',
+      statusCode: 422,
+    });
+    const users = await getRoleUsers(m2mRole.id);
+
+    expect(users.length).toBe(0);
+  });
+
   it('should fail when try to assign empty users', async () => {
     const role = await createRole({});
     const response = await assignUsersToRole([], role.id).catch((error: unknown) => error);
@@ -79,7 +93,7 @@ describe('roles users', () => {
     expect(response instanceof HTTPError && response.response.statusCode).toBe(404);
   });
 
-  it('should fail if user not found when trying to  remove user from role', async () => {
+  it('should fail if user not found when trying to remove user from role', async () => {
     const role = await createRole({});
     const response = await deleteUserFromRole('not-found', role.id).catch(
       (error: unknown) => error

packages/phrases/src/locales/de/errors/user.ts

@@ -32,6 +32,8 @@ const user = {
   user_not_exist: 'Der Benutzer mit {{ identifier }} existiert nicht.',
   missing_profile: 'Sie müssen zusätzliche Informationen angeben, bevor Sie sich anmelden können.',
   role_exists: 'Die Rollen-ID {{roleId}} wurde diesem Benutzer bereits hinzugefügt.',
+  invalid_role_type:
+    'Ungültiger Rollentyp, kann keine Maschinen-zu-Maschinen-Rolle einem Benutzer zuweisen.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/en/errors/user.ts

@@ -28,6 +28,7 @@ const user = {
   user_not_exist: 'User with {{ identifier }} does not exist.',
   missing_profile: 'You need to provide additional info before signing-in.',
   role_exists: 'The role id {{roleId}} is already been added to this user',
+  invalid_role_type: 'Invalid role type, can not assign machine-to-machine role to user.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/es/errors/user.ts

@@ -29,6 +29,8 @@ const user = {
   user_not_exist: 'No existe un usuario con {{ identifier }}.',
   missing_profile: 'Debes proporcionar información adicional antes de iniciar sesión.',
   role_exists: 'El id de rol {{roleId}} ya ha sido agregado a este usuario',
+  invalid_role_type:
+    'Tipo de rol no válido, no se puede asignar un rol de máquina a máquina a un usuario.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/fr/errors/user.ts

@@ -28,6 +28,8 @@ const user = {
   user_not_exist: "L'utilisateur avec {{ identifier }} n'existe pas.",
   missing_profile: 'Vous devez fournir des informations supplémentaires avant de vous connecter.',
   role_exists: "L'ID de rôle {{roleId}} a déjà été ajouté à cet utilisateur",
+  invalid_role_type:
+    'Le type de rôle est invalide, il est impossible d\'assigner un rôle "machine-to-machine" à un utilisateur.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/it/errors/user.ts

@@ -28,6 +28,8 @@ const user = {
   user_not_exist: "L'utente con {{ identifier }} non esiste.",
   missing_profile: 'È necessario fornire informazioni aggiuntive prima di accedere.',
   role_exists: "L'ID ruolo {{roleId}} è già stato aggiunto a questo utente",
+  invalid_role_type:
+    'Tipo di ruolo non valido, non è possibile assegnare un ruolo da macchina a utente.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/ja/errors/user.ts

@@ -28,6 +28,8 @@ const user = {
   user_not_exist: '{{ identifier }}を持つユーザーは存在しません。',
   missing_profile: 'サインインする前に追加情報を提供する必要があります。',
   role_exists: 'このユーザーには既に役割ID {{roleId}}が追加されています。',
+  invalid_role_type:
+    '役割タイプが無効です。ユーザーにはマシン対マシンの役割を割り当てることはできません。',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/ko/errors/user.ts

@@ -27,6 +27,8 @@ const user = {
   user_not_exist: '{{identifier}}의 사용자가 아직 등록되지 않았어요.',
   missing_profile: '로그인 전에 추가 정보를 제공해야 해요.',
   role_exists: '역할 ID {{roleId}}은/는 이미 이 사용자에게 할당되어 있어요.',
+  invalid_role_type:
+    '유효하지 않은 역할 유형입니다. 사용자에게 기계 대 기계 역할을 할당할 수 없습니다.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/pl-pl/errors/user.ts

@@ -29,6 +29,7 @@ const user = {
   user_not_exist: 'Użytkownik z identyfikatorem {{ identifier }} nie istnieje.',
   missing_profile: 'Musisz podać dodatkowe informacje przed zalogowaniem.',
   role_exists: 'Identyfikator roli {{roleId}} jest już dodany do tego użytkownika',
+  invalid_role_type: 'Nieprawidłowy typ roli, nie można przypisać roli maszynowej do użytkownika.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/pt-br/errors/user.ts

@@ -28,6 +28,8 @@ const user = {
   user_not_exist: 'O usuário com {{ identifier }} não existe',
   missing_profile: 'Você precisa fornecer informações adicionais antes de fazer login.',
   role_exists: 'O id da função {{roleId}} já foi adicionado a este usuário',
+  invalid_role_type:
+    'Tipo de função inválido, não é possível atribuir uma função máquina a usuário.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/pt-pt/errors/user.ts

@@ -28,6 +28,8 @@ const user = {
   user_not_exist: 'O utilizador com {{ identifier }} não existe.',
   missing_profile: 'Precisa de fornecer informações adicionais antes de iniciar sessão.',
   role_exists: 'O id da função {{roleId}} já foi adicionado a este utilizador.',
+  invalid_role_type:
+    'Tipo de função inválido, não é possível atribuir uma função máquina a máquina ao utilizador.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/ru/errors/user.ts

@@ -29,6 +29,7 @@ const user = {
   user_not_exist: 'Пользователя с {{ identifier }} не существует.',
   missing_profile: 'Вы должны предоставить дополнительную информацию перед входом в систему.',
   role_exists: 'Идентификатор роли {{roleId}} уже добавлен в этого пользователя',
+  invalid_role_type: 'Недопустимый тип роли, роль машины к машине нельзя назначить пользователю.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/tr-tr/errors/user.ts

@@ -28,6 +28,7 @@ const user = {
   user_not_exist: '{{identifier}} kimliğine sahip kullanıcı mevcut değil.',
   missing_profile: 'Oturum açmadan önce ek bilgi sağlamanız gerekiyor.',
   role_exists: '{{roleId}} rol kimliği bu kullanıcıya zaten eklenmiştir.',
+  invalid_role_type: 'Geçersiz rol türü, makine-makine rolü kullanıcıya atanamaz.',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/zh-cn/errors/user.ts

@@ -27,6 +27,7 @@ const user = {
   user_not_exist: '未找到与 {{identifier}} 相关联的用户。',
   missing_profile: '请于登录时提供必要的用户补充信息。',
   role_exists: '角色 ID {{roleId}} 已添加到此用户',
+  invalid_role_type: '无效的角色类型，无法将机器到机器角色分配给用户。',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/zh-hk/errors/user.ts

@@ -27,6 +27,7 @@ const user = {
   user_not_exist: '未找到與 {{identifier}} 相關聯的使用者。',
   missing_profile: '請於登錄時提供必要的使用者補充資訊。',
   role_exists: '角色 ID {{roleId}} 已添加到此使用者',
+  invalid_role_type: '無法設置機械到機械角色給使用者。',
 };
 
 export default Object.freeze(user);

packages/phrases/src/locales/zh-tw/errors/user.ts

@@ -27,6 +27,7 @@ const user = {
   user_not_exist: '未找到與 {{identifier}} 相關聯的用戶。',
   missing_profile: '請於登錄時提供必要的用戶補充信息。',
   role_exists: '角色 ID {{roleId}} 已添加到此用戶',
+  invalid_role_type: '無效角色類型，無法將機器對機器角色分配給用戶。',
 };
 
 export default Object.freeze(user);