diff --git a/express-api/src/controllers/admin/users/usersController.ts b/express-api/src/controllers/admin/users/usersController.ts index 39cc66122..23176704b 100644 --- a/express-api/src/controllers/admin/users/usersController.ts +++ b/express-api/src/controllers/admin/users/usersController.ts @@ -166,8 +166,12 @@ export const getAllRoles = async (req: Request, res: Response) => { "bearerAuth": [] }] */ - const roles = await userServices.getKeycloakRoles(); - return res.status(200).send(roles); + try { + const roles = await userServices.getKeycloakRoles(); + return res.status(200).send(roles); + } catch (e) { + return res.status(500).send('Something went wrong accessing the keycloak service.'); + } }; /** diff --git a/express-api/src/services/keycloak/keycloakService.ts b/express-api/src/services/keycloak/keycloakService.ts index 66ef96016..3e3af2990 100644 --- a/express-api/src/services/keycloak/keycloakService.ts +++ b/express-api/src/services/keycloak/keycloakService.ts @@ -80,23 +80,23 @@ const syncKeycloakRoles = async () => { }; await rolesServices.updateRole(overwriteRole); } + } + //This deletion section is somewhat clunky. Could consider delete cascade on the schema to avoid some of this. + const internalRolesForDeletion = await AppDataSource.getRepository(Role).findBy({ + Name: Not(In(roles.map((a) => a.name))), + }); - //This deletion section is somewhat clunky. Could consider delete cascade on the schema to avoid some of this. - const internalRolesForDeletion = await AppDataSource.getRepository(Role).findBy({ - Name: Not(In(roles.map((a) => a.name))), + if (internalRolesForDeletion.length) { + const roleIdsForDeletion = internalRolesForDeletion.map((role) => role.Id); + await AppDataSource.getRepository(User) + .createQueryBuilder() + .update(User) + .set({ RoleId: null }) + .where('RoleId IN (:...ids)', { ids: roleIdsForDeletion }) + .execute(); + await AppDataSource.getRepository(Role).delete({ + Id: In(roleIdsForDeletion), }); - if (internalRolesForDeletion.length) { - const roleIdsForDeletion = internalRolesForDeletion.map((role) => role.Id); - await AppDataSource.getRepository(User) - .createQueryBuilder() - .update(User) - .set({ RoleId: null }) - .where('RoleId IN (:...ids)', { ids: roleIdsForDeletion }) - .execute(); - await AppDataSource.getRepository(Role).delete({ - Id: In(roleIdsForDeletion), - }); - } } return roles; }; diff --git a/express-api/tests/unit/controllers/admin/roles/rolesController.test.ts b/express-api/tests/unit/controllers/admin/roles/rolesController.test.ts index de5b18588..7b881154d 100644 --- a/express-api/tests/unit/controllers/admin/roles/rolesController.test.ts +++ b/express-api/tests/unit/controllers/admin/roles/rolesController.test.ts @@ -27,6 +27,12 @@ jest.mock('@/services/admin/rolesServices', () => ({ removeRole: (role: RolesEntity) => _deleteRole(role), })); +const _syncKeycloakRoles = jest.fn().mockImplementation(() => {}); + +jest.mock('@/services/keycloak/keycloakService', () => ({ + syncKeycloakRoles: () => _syncKeycloakRoles(), +})); + describe('UNIT - Roles Admin', () => { beforeEach(() => { const { mockReq, mockRes } = getRequestHandlerMocks(); @@ -43,7 +49,7 @@ describe('UNIT - Roles Admin', () => { }); it('should return status 200 and a filtered roles', async () => { - mockRequest.body.filter = { name: 'big name' }; + mockRequest.query = { name: 'big name' }; const role = produceRole(); role.Name = 'big name'; await getRoles(mockRequest, mockResponse); diff --git a/express-api/tests/unit/controllers/admin/users/usersController.test.ts b/express-api/tests/unit/controllers/admin/users/usersController.test.ts index 8d11fd259..e5eb7617f 100644 --- a/express-api/tests/unit/controllers/admin/users/usersController.test.ts +++ b/express-api/tests/unit/controllers/admin/users/usersController.test.ts @@ -243,23 +243,18 @@ describe('UNIT - Users Admin', () => { }); describe('Controller getAllRoles', () => { - const user = produceUser(); - beforeEach(() => { - mockRequest.params = { - username: user.Username, - }; - }); it('should return status 200 and a list of roles assigned to a user', async () => { await getAllRoles(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(mockResponse.sendValue.at(0)).toBe('admin'); }); - it('should return status 400 when no username is provided', async () => { - mockRequest.params = {}; + it('should return status 400 when internal service throws', async () => { + _getRoles.mockImplementationOnce(() => { + throw Error(); + }); await getAllRoles(mockRequest, mockResponse); - expect(mockResponse.statusValue).toBe(400); - expect(mockResponse.sendValue).toBe('Username was empty.'); + expect(mockResponse.statusValue).toBe(500); }); }); diff --git a/express-api/tests/unit/controllers/users/usersController.test.ts b/express-api/tests/unit/controllers/users/usersController.test.ts index f7b9ab262..30cb07309 100644 --- a/express-api/tests/unit/controllers/users/usersController.test.ts +++ b/express-api/tests/unit/controllers/users/usersController.test.ts @@ -34,6 +34,7 @@ const _getUser = jest .fn() .mockImplementation((guid: string) => ({ ...produceUser(), KeycloakUserId: guid })); const _normalizeKeycloakUser = jest.fn().mockImplementation(() => {}); + jest.mock('@/services/users/usersServices', () => ({ activateUser: () => _activateUser(), getAccessRequest: () => _getAccessRequest(), @@ -48,6 +49,16 @@ jest.mock('@/services/users/usersServices', () => ({ normalizeKeycloakUser: () => _normalizeKeycloakUser(), })); +const _syncKeycloakRoles = jest.fn(); +const _syncKeycloakUser = jest + .fn() + .mockImplementation((username: string) => ({ ...produceUser(), Username: username })); + +jest.mock('@/services/keycloak/keycloakService.ts', () => ({ + syncKeycloakRoles: () => _syncKeycloakRoles(), + syncKeycloakUser: () => _syncKeycloakUser(), +})); + describe('UNIT - Testing controllers for users routes.', () => { let mockRequest: Request & MockReq, mockResponse: Response & MockRes; diff --git a/express-api/tests/unit/services/admin/usersServices.test.ts b/express-api/tests/unit/services/admin/usersServices.test.ts index 7c3d26833..3853ba289 100644 --- a/express-api/tests/unit/services/admin/usersServices.test.ts +++ b/express-api/tests/unit/services/admin/usersServices.test.ts @@ -31,6 +31,7 @@ jest.mock('@/services/keycloak/keycloakService', () => ({ getKeycloakUserRoles: (): IKeycloakRole[] => [{ name: 'abc' }], updateKeycloakUserRoles: (username: string, roles: string[]): IKeycloakRole[] => roles.map((a) => ({ name: a })), + syncKeycloakUser: (username: string) => ({ ...produceUser(), Username: username }), })); describe('UNIT - admin user services', () => { diff --git a/express-api/tests/unit/services/keycloak/keycloakService.test.ts b/express-api/tests/unit/services/keycloak/keycloakService.test.ts index 764b35d4b..f831d1aec 100644 --- a/express-api/tests/unit/services/keycloak/keycloakService.test.ts +++ b/express-api/tests/unit/services/keycloak/keycloakService.test.ts @@ -12,6 +12,11 @@ import { unassignUserRole, } from '@bcgov/citz-imb-kc-css-api'; import { faker } from '@faker-js/faker'; +import { produceRole, produceUser } from 'tests/testUtils/factories'; +import { AppDataSource } from '@/appDataSource'; +import { Role } from '@/typeorm/Entities/Role'; +import { User } from '@/typeorm/Entities/User'; +import { DeepPartial } from 'typeorm'; jest.mock('@bcgov/citz-imb-kc-css-api'); @@ -24,17 +29,87 @@ const mockUser: IKeycloakUser = { display_name: [faker.person.fullName()], }, }; +// eslint-disable-next-line @typescript-eslint/no-unused-vars +const _updateUser = jest.fn().mockImplementation((_user: DeepPartial) => ({ raw: {} })); + +jest.mock('@/services/admin/usersServices', () => ({ + getUsers: () => [produceUser()], + getUserById: () => produceUser(), + updateUser: (user: DeepPartial) => _updateUser(user), +})); + +const _getRoles = jest.fn(); +const _addRole = jest.fn(); +const _updateRole = jest.fn(); +const _getRoleByName = jest.fn().mockImplementation(async () => produceRole()); + +jest.mock('@/services/admin/rolesServices', () => ({ + getRoles: () => _getRoles(), + addRole: () => _addRole(), + updateRole: () => _updateRole(), + getRoleByName: () => _getRoleByName(), +})); + +const _getKeycloakRoles = jest.spyOn(KeycloakService, 'getKeycloakRoles'); +const _getKeycloakUserRoles = jest.spyOn(KeycloakService, 'getKeycloakUserRoles'); + +const _repoFindBy = jest.spyOn(AppDataSource.getRepository(Role), 'findBy'); +const _repoDelete = jest + .spyOn(AppDataSource.getRepository(Role), 'delete') + .mockImplementation(async () => ({ raw: {} })); +// eslint-disable-next-line @typescript-eslint/no-explicit-any +const userCreateQueryBuilder: any = { + update: () => userCreateQueryBuilder, + set: () => userCreateQueryBuilder, + where: () => userCreateQueryBuilder, + execute: () => {}, +}; +jest + .spyOn(AppDataSource.getRepository(User), 'createQueryBuilder') + .mockImplementation(() => userCreateQueryBuilder); describe('UNIT - KeycloakService', () => { beforeEach(() => { - jest.resetAllMocks(); + jest.clearAllMocks(); }); describe('syncKeycloakRoles', () => { - // TODO: finish tests when function is implemented - xit('should add roles to PIMS if they do not already exist', () => {}); - xit('should remove roles from PIMS if they do not exist in Keycloak', () => {}); - xit('should update roles in PIMS if they do not match what is in Keycloak', () => {}); + beforeEach(() => jest.clearAllMocks()); + + it('should add roles to PIMS if they do not already exist', async () => { + _getKeycloakRoles.mockImplementationOnce(async () => [{ name: 'Administrator' }]); + _getRoles.mockImplementationOnce(async () => []); + _addRole.mockImplementationOnce(async (role) => role); + _repoFindBy.mockImplementationOnce(async () => []); + await KeycloakService.syncKeycloakRoles(); + + expect(_addRole).toHaveBeenCalledTimes(1); + expect(_repoDelete).toHaveBeenCalledTimes(0); + }); + it('should remove roles from PIMS if they do not exist in Keycloak', async () => { + _getKeycloakRoles.mockImplementationOnce(async () => []); + _getRoles.mockImplementationOnce(async () => []); + _repoFindBy.mockImplementationOnce(async () => { + return [{ ...produceRole(), Name: 'OldRole' }]; + }); + await KeycloakService.syncKeycloakRoles(); + + expect(_addRole).toHaveBeenCalledTimes(0); + expect(_repoDelete).toHaveBeenCalledTimes(1); + }); + it('should update roles in PIMS if they do not match what is in Keycloak', async () => { + _getKeycloakRoles.mockImplementationOnce(async () => [{ name: 'Administrator' }]); + _getRoles.mockImplementationOnce(async () => + Promise.resolve([{ ...produceRole(), Name: 'Administrator' }]), + ); + _updateRole.mockImplementationOnce(async (role) => role); + _repoFindBy.mockImplementationOnce(async () => []); + + await KeycloakService.syncKeycloakRoles(); + + expect(_updateRole).toHaveBeenCalledTimes(1); + expect(_repoDelete).toHaveBeenCalledTimes(0); + }); }); describe('getKeycloakRoles', () => { @@ -114,8 +189,16 @@ describe('UNIT - KeycloakService', () => { describe('syncKeycloakUser', () => { // TODO: finish tests when function is implemented - xit('should add update user roles in PIMS', () => {}); - xit('should add user and roles in PIMS if they do not exist', () => {}); + it('should add update user roles in PIMS', async () => { + _getKeycloakUserRoles.mockImplementationOnce(async () => [{ name: 'Test' }]); + await KeycloakService.syncKeycloakUser('test'); + expect(_updateUser.mock.calls[0][0].RoleId).toBeTruthy(); + }); + it('should null role if no role is present', async () => { + _getKeycloakUserRoles.mockImplementationOnce(async () => []); + await KeycloakService.syncKeycloakUser('test'); + expect(_updateUser.mock.calls[0][0].RoleId).toBeNull(); + }); }); describe('getKeycloakUsers', () => {