From 5b504d9496a679a3ee2a4cae086613e2922abd99 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Fri, 13 Sep 2024 14:25:20 -0700 Subject: [PATCH 01/15] change over role checks to internal check --- .../administrativeAreasController.ts | 3 +- .../agencies/agenciesController.ts | 2 +- .../buildings/buildingsController.ts | 6 ++-- .../notifications/notificationsController.ts | 9 ++--- .../controllers/parcels/parcelsController.ts | 7 ++-- .../projects/projectsController.ts | 18 +++++----- .../properties/propertiesController.ts | 11 +++--- .../src/controllers/users/usersController.ts | 9 ++--- express-api/src/middleware/activeUserCheck.ts | 7 +--- .../services/buildings/buildingServices.ts | 5 +-- .../src/services/parcels/parcelServices.ts | 5 +-- .../src/services/projects/projectsServices.ts | 4 +-- .../src/services/users/usersServices.ts | 24 +++++++++++++ .../src/utilities/authorizationChecks.ts | 34 +++++-------------- 14 files changed, 77 insertions(+), 67 deletions(-) diff --git a/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts b/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts index 7b4773758..982d4d25e 100644 --- a/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts +++ b/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts @@ -19,8 +19,7 @@ export const getAdministrativeAreas = async (req: Request, res: Response) => { const filter = AdministrativeAreaFilterSchema.safeParse(req.query); if (filter.success) { const adminAreas = await administrativeAreasServices.getAdministrativeAreas(filter.data); - // TODO: Do we still need this condition? Few fields are trimmed since moving to view. - if (!ssoUser.hasRoles([Roles.ADMIN])) { + if (!userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN])) { const trimmed = AdministrativeAreaPublicResponseSchema.array().parse(adminAreas.data); return res.status(200).send({ ...adminAreas, diff --git a/express-api/src/controllers/agencies/agenciesController.ts b/express-api/src/controllers/agencies/agenciesController.ts index 8bbeb2acb..452730959 100644 --- a/express-api/src/controllers/agencies/agenciesController.ts +++ b/express-api/src/controllers/agencies/agenciesController.ts @@ -18,7 +18,7 @@ export const getAgencies = async (req: Request, res: Response) => { const filter = AgencyFilterSchema.safeParse(req.query); if (filter.success) { const agencies = await agencyService.getAgencies(filter.data); - if (!ssoUser.client_roles || !ssoUser.client_roles.includes(Roles.ADMIN)) { + if (!userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN])) { const trimmed = AgencyPublicResponseSchema.array().parse(agencies); return res.status(200).send({ ...agencies, diff --git a/express-api/src/controllers/buildings/buildingsController.ts b/express-api/src/controllers/buildings/buildingsController.ts index fb22ee6c4..2a3710e5f 100644 --- a/express-api/src/controllers/buildings/buildingsController.ts +++ b/express-api/src/controllers/buildings/buildingsController.ts @@ -4,7 +4,7 @@ import { BuildingFilter, BuildingFilterSchema } from '@/services/buildings/build import userServices from '@/services/users/usersServices'; import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Building } from '@/typeorm/Entities/Building'; -import { checkUserAgencyPermission, isAdmin, isAuditor } from '@/utilities/authorizationChecks'; +import { checkUserAgencyPermission } from '@/utilities/authorizationChecks'; import { Roles } from '@/constants/roles'; import { AppDataSource } from '@/appDataSource'; import { exposedProjectStatuses } from '@/constants/projectStatus'; @@ -24,7 +24,9 @@ export const getBuildings = async (req: Request, res: Response) => { return res.status(400).send('Could not parse filter.'); } const filterResult = filter.data; - if (!(isAdmin(kcUser) || isAuditor(kcUser))) { + if ( + !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, Roles.AUDITOR])) + ) { // get array of user's agencies const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); filterResult.agencyId = usersAgencies; diff --git a/express-api/src/controllers/notifications/notificationsController.ts b/express-api/src/controllers/notifications/notificationsController.ts index 7b6f2f8ec..ce596eca9 100644 --- a/express-api/src/controllers/notifications/notificationsController.ts +++ b/express-api/src/controllers/notifications/notificationsController.ts @@ -5,7 +5,6 @@ import userServices from '@/services/users/usersServices'; import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Request, Response } from 'express'; import { DisposalNotificationFilterSchema } from './notificationsSchema'; -import { isAdmin, isAuditor } from '@/utilities/authorizationChecks'; import projectServices from '@/services/projects/projectsServices'; import { Roles } from '@/constants/roles'; import logger from '@/utilities/winstonLogger'; @@ -26,7 +25,9 @@ export const getNotificationsByProjectId = async (req: Request, res: Response) = const kcUser = req.user as unknown as SSOUser; const user = await userServices.getUser((req.user as SSOUser).preferred_username); - if (!(isAdmin(kcUser) || isAuditor(kcUser))) { + if ( + !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, Roles.AUDITOR])) + ) { // get array of user's agencies const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); @@ -57,7 +58,7 @@ export const getNotificationsByProjectId = async (req: Request, res: Response) = export const resendNotificationById = async (req: Request, res: Response) => { const kcUser = req.user; - if (!kcUser.hasRoles([Roles.ADMIN])) + if (!userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN])) return res.status(403).send('User lacks permissions to resend notification.'); const id = Number(req.params.id); const notification = await notificationServices.getNotificationById(id); @@ -75,7 +76,7 @@ export const resendNotificationById = async (req: Request, res: Response) => { export const cancelNotificationById = async (req: Request, res: Response) => { const kcUser = req.user; - if (!kcUser.hasRoles([Roles.ADMIN])) + if (!userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN])) return res.status(403).send('User lacks permissions to cancel notification.'); const id = Number(req.params.id); const notification = await notificationServices.getNotificationById(id); diff --git a/express-api/src/controllers/parcels/parcelsController.ts b/express-api/src/controllers/parcels/parcelsController.ts index 6f68cb69f..0ed798e9e 100644 --- a/express-api/src/controllers/parcels/parcelsController.ts +++ b/express-api/src/controllers/parcels/parcelsController.ts @@ -5,7 +5,7 @@ import { SSOUser } from '@bcgov/citz-imb-sso-express'; import userServices from '@/services/users/usersServices'; import { Parcel } from '@/typeorm/Entities/Parcel'; import { Roles } from '@/constants/roles'; -import { checkUserAgencyPermission, isAdmin, isAuditor } from '@/utilities/authorizationChecks'; +import { checkUserAgencyPermission } from '@/utilities/authorizationChecks'; import { AppDataSource } from '@/appDataSource'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; import { exposedProjectStatuses } from '@/constants/projectStatus'; @@ -105,7 +105,10 @@ export const getParcels = async (req: Request, res: Response) => { return res.status(400).send('Could not parse filter.'); } const filterResult = filter.data; - if (!(isAdmin(kcUser) || isAuditor(kcUser))) { + + if ( + !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, Roles.AUDITOR])) + ) { // get array of user's agencies const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); filterResult.agencyId = usersAgencies; diff --git a/express-api/src/controllers/projects/projectsController.ts b/express-api/src/controllers/projects/projectsController.ts index 60c59c141..55d5f28e0 100644 --- a/express-api/src/controllers/projects/projectsController.ts +++ b/express-api/src/controllers/projects/projectsController.ts @@ -3,7 +3,7 @@ import { Request, Response } from 'express'; import { SSOUser } from '@bcgov/citz-imb-sso-express'; import projectServices, { ProjectPropertyIds } from '@/services/projects/projectsServices'; import userServices from '@/services/users/usersServices'; -import { isAdmin, isAuditor, checkUserAgencyPermission } from '@/utilities/authorizationChecks'; +import { checkUserAgencyPermission } from '@/utilities/authorizationChecks'; import { DeepPartial } from 'typeorm'; import { Project } from '@/typeorm/Entities/Project'; import { Roles } from '@/constants/roles'; @@ -54,7 +54,8 @@ export const getDisposalProject = async (req: Request, res: Response) => { */ export const updateDisposalProject = async (req: Request, res: Response) => { // Only admins can edit projects - if (!isAdmin(req.user)) { + const isAdmin = await userServices.hasOneOfRoles(req.user?.preferred_username, [Roles.ADMIN]); + if (!isAdmin) { return res.status(403).send('Projects only editable by Administrator role.'); } @@ -86,17 +87,13 @@ export const updateDisposalProject = async (req: Request, res: Response) => { * @returns {Response} A 200 status with the deleted project. */ export const deleteDisposalProject = async (req: Request, res: Response) => { - // Only admins can delete projects - if (!isAdmin(req.user)) { - return res.status(403).send('Projects can only be deleted by Administrator role.'); - } - const projectId = Number(req.params.projectId); if (isNaN(projectId)) { return res.status(400).send('Invalid Project ID'); } // Only admins can delete projects - if (!isAdmin(req.user)) { + const isAdmin = await userServices.hasOneOfRoles(req.user?.preferred_username, [Roles.ADMIN]); + if (!isAdmin) { return res.status(403).send('Projects can only be deleted by Administrator role.'); } @@ -117,7 +114,7 @@ export const deleteDisposalProject = async (req: Request, res: Response) => { */ export const addDisposalProject = async (req: Request, res: Response) => { // Auditors can no add projects - if (isAuditor(req.user)) { + if (await userServices.hasOneOfRoles(req.user?.preferred_username, [Roles.AUDITOR])) { return res.status(403).send('Projects can not be added by user with Auditor role.'); } // Extract project data from request body @@ -159,7 +156,8 @@ export const getProjects = async (req: Request, res: Response) => { } const filterResult = filter.data; const kcUser = req.user as unknown as SSOUser; - if (!isAdmin(kcUser)) { + const isAdmin = await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN]); + if (!isAdmin) { // get array of user's agencies const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); filterResult.agencyId = usersAgencies; diff --git a/express-api/src/controllers/properties/propertiesController.ts b/express-api/src/controllers/properties/propertiesController.ts index 91bd8b4a8..42ec96419 100644 --- a/express-api/src/controllers/properties/propertiesController.ts +++ b/express-api/src/controllers/properties/propertiesController.ts @@ -7,7 +7,7 @@ import { MapFilterSchema, PropertyUnionFilterSchema, } from '@/controllers/properties/propertiesSchema'; -import { checkUserAgencyPermission, isAdmin, isAuditor } from '@/utilities/authorizationChecks'; +import { checkUserAgencyPermission } from '@/utilities/authorizationChecks'; import userServices from '@/services/users/usersServices'; import { Worker } from 'worker_threads'; import path from 'path'; @@ -30,7 +30,8 @@ export const getPropertiesFuzzySearch = async (req: Request, res: Response) => { const take = req.query.take ? Number(req.query.take) : undefined; const kcUser = req.user; let userAgencies; - if (!isAdmin(kcUser)) { + const isAdmin = await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN]); + if (!isAdmin) { userAgencies = await userServices.getAgencies(kcUser.preferred_username); } const result = await propertyServices.propertiesFuzzySearch(keyword, take, userAgencies); @@ -90,7 +91,7 @@ export const getPropertiesForMap = async (req: Request, res: Response) => { const kcUser = req.user; const permittedRoles = [Roles.ADMIN, Roles.AUDITOR]; // Admins and auditors see all, otherwise... - if (!(isAdmin(kcUser) || isAuditor(kcUser))) { + if (!userServices.hasOneOfRoles(kcUser.preferred_username, permittedRoles)) { const requestedAgencies = filterResult.AgencyIds; const userHasAgencies = await checkUserAgencyPermission( kcUser, @@ -214,7 +215,9 @@ export const getPropertyUnion = async (req: Request, res: Response) => { // Prevent getting back unrelated agencies for general users const kcUser = req.user as unknown as SSOUser; const filterResult = filter.data; - if (!(isAdmin(kcUser) || isAuditor(kcUser))) { + if ( + !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, Roles.AUDITOR])) + ) { // get array of user's agencies const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); filterResult.agencyIds = usersAgencies; diff --git a/express-api/src/controllers/users/usersController.ts b/express-api/src/controllers/users/usersController.ts index b2794b65a..7887d40e9 100644 --- a/express-api/src/controllers/users/usersController.ts +++ b/express-api/src/controllers/users/usersController.ts @@ -3,10 +3,10 @@ import { Request, Response } from 'express'; import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { UserFiltering, UserFilteringSchema } from '@/controllers/users/usersSchema'; import { z } from 'zod'; -import { isAdmin } from '@/utilities/authorizationChecks'; import notificationServices from '@/services/notifications/notificationServices'; import getConfig from '@/constants/config'; import logger from '@/utilities/winstonLogger'; +import { Roles } from '@/constants/roles'; /** * @description Submits a user access request. @@ -89,7 +89,8 @@ export const getUsers = async (req: Request, res: Response) => { const filterResult = filter.data; let users; - if (isAdmin(ssoUser)) { + const isAdmin = await userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN]); + if (isAdmin) { users = await userServices.getUsers(filterResult as UserFiltering); } else { // Get agencies associated with the requesting user @@ -112,9 +113,9 @@ export const getUserById = async (req: Request, res: Response) => { const ssoUser = req.user as unknown as SSOUser; if (uuid.success) { const user = await userServices.getUserById(uuid.data); - if (user) { - if (!isAdmin(ssoUser)) { + const isAdmin = await userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN]); + if (!isAdmin) { // check if user has the correct agencies const usersAgencies = await userServices.hasAgencies(ssoUser.preferred_username, [ user.AgencyId, diff --git a/express-api/src/middleware/activeUserCheck.ts b/express-api/src/middleware/activeUserCheck.ts index d31df8bd5..d3568e759 100644 --- a/express-api/src/middleware/activeUserCheck.ts +++ b/express-api/src/middleware/activeUserCheck.ts @@ -1,5 +1,4 @@ import { AppDataSource } from '@/appDataSource'; -import { Roles } from '@/constants/roles'; import { User } from '@/typeorm/Entities/User'; import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { NextFunction, RequestHandler, Response } from 'express'; @@ -38,11 +37,7 @@ const activeUserCheck: unknown = async ( } // Check that user has a role - if ( - !req.user?.hasRoles([Roles.ADMIN, Roles.AUDITOR, Roles.GENERAL_USER], { - requireAllRoles: false, - }) - ) { + if (!user.RoleId) { return res.status(403).send('Request forbidden. User has no assigned role.'); } next(); diff --git a/express-api/src/services/buildings/buildingServices.ts b/express-api/src/services/buildings/buildingServices.ts index 9c206862b..662e2cb40 100644 --- a/express-api/src/services/buildings/buildingServices.ts +++ b/express-api/src/services/buildings/buildingServices.ts @@ -9,7 +9,7 @@ import { BuildingFiscal } from '@/typeorm/Entities/BuildingFiscal'; import logger from '@/utilities/winstonLogger'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; import { SSOUser } from '@bcgov/citz-imb-sso-express'; -import { isAdmin } from '@/utilities/authorizationChecks'; +import { Roles } from '@/constants/roles'; const buildingRepo = AppDataSource.getRepository(Building); @@ -69,7 +69,8 @@ export const updateBuildingById = async (building: DeepPartial, ssoUse throw new ErrorWithCode('Building does not exists.', 404); } const validUserAgencies = await userServices.getAgencies(ssoUser.preferred_username); - if (!isAdmin(ssoUser) && !validUserAgencies.includes(building.AgencyId)) { + const isAdmin = await userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN]); + if (!isAdmin && !validUserAgencies.includes(building.AgencyId)) { throw new ErrorWithCode('This agency change is not permitted.', 403); } if (building.Fiscals && building.Fiscals.length) { diff --git a/express-api/src/services/parcels/parcelServices.ts b/express-api/src/services/parcels/parcelServices.ts index f99cde25d..c55a4be75 100644 --- a/express-api/src/services/parcels/parcelServices.ts +++ b/express-api/src/services/parcels/parcelServices.ts @@ -8,8 +8,8 @@ import { ParcelFiscal } from '@/typeorm/Entities/ParcelFiscal'; import userServices from '../users/usersServices'; import logger from '@/utilities/winstonLogger'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; -import { isAdmin } from '@/utilities/authorizationChecks'; import { SSOUser } from '@bcgov/citz-imb-sso-express'; +import { Roles } from '@/constants/roles'; const parcelRepo = AppDataSource.getRepository(Parcel); @@ -164,7 +164,8 @@ const updateParcel = async (incomingParcel: DeepPartial, ssoUser: SSOUse throw new ErrorWithCode('Parcel not found', 404); } const validUserAgencies = await userServices.getAgencies(ssoUser.preferred_username); - if (!isAdmin(ssoUser) && !validUserAgencies.includes(incomingParcel.AgencyId)) { + const isAdmin = await userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN]); + if (!isAdmin && !validUserAgencies.includes(incomingParcel.AgencyId)) { throw new ErrorWithCode('This agency change is not permitted.', 403); } if (incomingParcel.Fiscals && incomingParcel.Fiscals.length) { diff --git a/express-api/src/services/projects/projectsServices.ts b/express-api/src/services/projects/projectsServices.ts index 3c425daef..50e779caf 100644 --- a/express-api/src/services/projects/projectsServices.ts +++ b/express-api/src/services/projects/projectsServices.ts @@ -37,7 +37,7 @@ import { ProjectMonetary } from '@/typeorm/Entities/ProjectMonetary'; import { NotificationQueue } from '@/typeorm/Entities/NotificationQueue'; import { SortOrders } from '@/constants/types'; import { ProjectJoin } from '@/typeorm/Entities/views/ProjectJoinView'; -import { isAdmin } from '@/utilities/authorizationChecks'; +import { Roles } from '@/constants/roles'; const projectRepo = AppDataSource.getRepository(Project); @@ -680,7 +680,7 @@ const updateProject = async ( //Agency change disallowed unless admin. project.AgencyId && originalProject.AgencyId !== project.AgencyId && - !isAdmin(user) + !(await userServices.hasOneOfRoles(user.preferred_username, [Roles.ADMIN])) ) { throw new ErrorWithCode('Project Agency may not be changed.', 403); } diff --git a/express-api/src/services/users/usersServices.ts b/express-api/src/services/users/usersServices.ts index f339e5c73..a50c0f7b2 100644 --- a/express-api/src/services/users/usersServices.ts +++ b/express-api/src/services/users/usersServices.ts @@ -7,6 +7,7 @@ import { randomUUID, UUID } from 'crypto'; import KeycloakService from '@/services/keycloak/keycloakService'; import { ErrorWithCode } from '@/utilities/customErrors/ErrorWithCode'; import { UserFiltering } from '@/controllers/users/usersSchema'; +import { Roles } from '@/constants/roles'; interface NormalizedKeycloakUser { first_name: string; @@ -127,6 +128,28 @@ const hasAgencies = async (username: string, agencies: number[]) => { return result; }; +const hasOneOfRoles = async (username: string, roles: Roles[]) => { + // No username or roles, then no permission. + if (!username || !roles.length) { + return false; + } + // Create where objects + const orWhereClauses = roles.map((r) => ({ + Username: username, + Role: { + Name: r, + }, + })); + // Does the user exist with one of these roles? + const result = await AppDataSource.getRepository(User).exists({ + relations: { + Role: true, + }, + where: orWhereClauses, + }); + return result; +}; + /** * Maps the sort key and direction to the corresponding object structure for TypeORM find options. * @param sortKey - The key to sort by. @@ -285,6 +308,7 @@ const userServices = { getUser, addKeycloakUserOnHold, hasAgencies, + hasOneOfRoles, getAgencies, normalizeKeycloakUser, getUsers, diff --git a/express-api/src/utilities/authorizationChecks.ts b/express-api/src/utilities/authorizationChecks.ts index e6d26533c..8a6ca21cd 100644 --- a/express-api/src/utilities/authorizationChecks.ts +++ b/express-api/src/utilities/authorizationChecks.ts @@ -1,26 +1,6 @@ import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Roles } from '@/constants/roles'; import userServices, { getUser } from '@/services/users/usersServices'; -/** - * @description Function to check if user is an admin - * @param {SSOUser} user Incoming Keycloak user. - * @returns {boolean} A boolean for whether admin or not. - */ -export const isAdmin = (user: SSOUser): boolean => { - // Check if the user has the ADMIN role - return user.client_roles?.includes(Roles.ADMIN); -}; - -/** - * Function to check if user is an auditor (view only role). - * - * @param {SSOUser} user - The user object containing information about the user. - * @returns True if the user has the AUDITOR role, false otherwise. - */ -export const isAuditor = (user: SSOUser): boolean => { - // Check if the user has the AUDITOR role - return user.client_roles?.includes(Roles.AUDITOR); -}; /** * Function to check if user can edit. @@ -28,11 +8,12 @@ export const isAuditor = (user: SSOUser): boolean => { * @param user - The user object containing information about the user. * @returns A boolean value indicating whether the user can edit or not. */ -export const canUserEdit = (user: SSOUser): boolean => { +export const canUserEdit = async (user: SSOUser): Promise => { // as they are not an auditor the user can edit - return ( - user.client_roles?.includes(Roles.GENERAL_USER) || user.client_roles?.includes(Roles.ADMIN) - ); + return await userServices.hasOneOfRoles(user.preferred_username, [ + Roles.GENERAL_USER, + Roles.ADMIN, + ]); }; /** @@ -73,9 +54,10 @@ export const checkUserAgencyPermission = async ( if (!agencyIds || agencyIds.length === 0 || !agencyIds.at(0)) { return false; } - const userRolePermission = kcUser?.hasRoles(permittedRoles, { requireAllRoles: false }); // if the user is not an admin, nor has a permitted role scope results - if (!isAdmin(kcUser) && !userRolePermission) { + if ( + !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, ...permittedRoles])) + ) { // check if current user belongs to any of the specified agencies const userAgencies = await userServices.hasAgencies(kcUser.preferred_username, agencyIds); return userAgencies; From c1d44dee64dae5c7508adcc29087b60da2d94826 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Fri, 13 Sep 2024 14:30:55 -0700 Subject: [PATCH 02/15] change role name source --- .../src/controllers/properties/propertiesController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/express-api/src/controllers/properties/propertiesController.ts b/express-api/src/controllers/properties/propertiesController.ts index 42ec96419..7cd692364 100644 --- a/express-api/src/controllers/properties/propertiesController.ts +++ b/express-api/src/controllers/properties/propertiesController.ts @@ -133,7 +133,7 @@ export const importProperties = async (req: Request, res: Response) => { const fileName = req.file.originalname; const ssoUser = req.user; const user = await userServices.getUser(ssoUser.preferred_username); - const roles = ssoUser.client_roles; + const role = user.Role?.Name; try { readFile(filePath, { WTF: true }); //With this read option disabled it will throw if unexpected data is present. } catch (e) { @@ -153,7 +153,7 @@ export const importProperties = async (req: Request, res: Response) => { }); const workerPath = `../../services/properties/propertyWorker.${process.env.NODE_ENV === 'production' ? 'js' : 'ts'}`; const worker = new Worker(path.resolve(__dirname, workerPath), { - workerData: { filePath, resultRowId: resultRow.Id, user, roles }, + workerData: { filePath, resultRowId: resultRow.Id, user, roles: role }, execArgv: [ '--require', 'ts-node/register', From 7b03fa2290acd757d22f1853265415e6cd59bf0d Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Fri, 13 Sep 2024 16:21:16 -0700 Subject: [PATCH 03/15] convert checks to middleware. --- express-api/src/constants/roles.ts | 6 ++-- .../administrativeAreasController.ts | 8 ++--- .../agencies/agenciesController.ts | 10 +++--- .../buildings/buildingsController.ts | 19 +++++------ .../notifications/notificationsController.ts | 22 +++++-------- .../controllers/parcels/parcelsController.ts | 20 +++++------- .../projects/projectsController.ts | 32 +++++++++---------- .../properties/propertiesController.ts | 30 ++++++++--------- .../controllers/reports/reportsController.ts | 2 +- .../src/controllers/users/usersController.ts | 16 ++++------ express-api/src/middleware/activeUserCheck.ts | 28 +++++++++++++++- .../services/buildings/buildingServices.ts | 11 ++++--- express-api/src/services/ches/chesServices.ts | 13 +++++--- .../notifications/notificationServices.ts | 4 +-- .../src/services/parcels/parcelServices.ts | 8 ++--- .../src/services/projects/projectsServices.ts | 14 ++++---- .../services/properties/propertiesServices.ts | 7 ++-- .../src/services/users/usersServices.ts | 24 -------------- .../src/utilities/authorizationChecks.ts | 16 ++++------ 19 files changed, 135 insertions(+), 155 deletions(-) diff --git a/express-api/src/constants/roles.ts b/express-api/src/constants/roles.ts index f96fe0dfa..0009ec730 100644 --- a/express-api/src/constants/roles.ts +++ b/express-api/src/constants/roles.ts @@ -3,7 +3,7 @@ * The values in this enum must exactly mirror the names of the Keycloak roles. */ export enum Roles { - ADMIN = 'Administrator', - GENERAL_USER = 'General User', - AUDITOR = 'Auditor', + ADMIN = '00000000-0000-0000-0000-000000000000', + GENERAL_USER = '00000000-0000-0000-0000-000000000001', + AUDITOR = '00000000-0000-0000-0000-000000000002', } diff --git a/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts b/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts index 982d4d25e..f2c23c9ac 100644 --- a/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts +++ b/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts @@ -1,12 +1,10 @@ import { Request, Response } from 'express'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { AdministrativeAreaFilterSchema, AdministrativeAreaPublicResponseSchema, } from '@/services/administrativeAreas/administrativeAreaSchema'; import administrativeAreasServices from '@/services/administrativeAreas/administrativeAreasServices'; import { Roles } from '@/constants/roles'; -import userServices from '@/services/users/usersServices'; /** * @description Gets a list of administrative areas. @@ -15,11 +13,11 @@ import userServices from '@/services/users/usersServices'; * @returns {Response} A 200 status with a list of administrative areas. */ export const getAdministrativeAreas = async (req: Request, res: Response) => { - const ssoUser = req.user; + const user = req.pimsUser; const filter = AdministrativeAreaFilterSchema.safeParse(req.query); if (filter.success) { const adminAreas = await administrativeAreasServices.getAdministrativeAreas(filter.data); - if (!userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN])) { + if (!user.hasOneOfRoles([Roles.ADMIN])) { const trimmed = AdministrativeAreaPublicResponseSchema.array().parse(adminAreas.data); return res.status(200).send({ ...adminAreas, @@ -39,7 +37,7 @@ export const getAdministrativeAreas = async (req: Request, res: Response) => { * @returns {Response} A 201 status and response with the added administrative area. */ export const addAdministrativeArea = async (req: Request, res: Response) => { - const user = await userServices.getUser((req.user as SSOUser).preferred_username); + const user = req.pimsUser; const addBody = { ...req.body, CreatedById: user.Id }; const response = await administrativeAreasServices.addAdministrativeArea(addBody); return res.status(201).send(response); diff --git a/express-api/src/controllers/agencies/agenciesController.ts b/express-api/src/controllers/agencies/agenciesController.ts index 452730959..5d6bac77b 100644 --- a/express-api/src/controllers/agencies/agenciesController.ts +++ b/express-api/src/controllers/agencies/agenciesController.ts @@ -2,9 +2,7 @@ import { Request, Response } from 'express'; import * as agencyService from '@/services/agencies/agencyServices'; import { AgencyFilterSchema, AgencyPublicResponseSchema } from '@/services/agencies/agencySchema'; import { z } from 'zod'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Roles } from '@/constants/roles'; -import userServices from '@/services/users/usersServices'; import { Agency } from '@/typeorm/Entities/Agency'; /** @@ -14,11 +12,11 @@ import { Agency } from '@/typeorm/Entities/Agency'; * @returns {Response} A 200 status with a list of agencies. */ export const getAgencies = async (req: Request, res: Response) => { - const ssoUser = req.user; + const user = req.pimsUser; const filter = AgencyFilterSchema.safeParse(req.query); if (filter.success) { const agencies = await agencyService.getAgencies(filter.data); - if (!userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN])) { + if (!user.hasOneOfRoles([Roles.ADMIN])) { const trimmed = AgencyPublicResponseSchema.array().parse(agencies); return res.status(200).send({ ...agencies, @@ -38,7 +36,7 @@ export const getAgencies = async (req: Request, res: Response) => { * @returns {Response} A 201 status and the data of the agency added. */ export const addAgency = async (req: Request, res: Response) => { - const user = await userServices.getUser((req.user as SSOUser).preferred_username); + const user = req.pimsUser; const agency = await agencyService.addAgency({ ...req.body, CreatedById: user.Id }); return res.status(201).send(agency); @@ -77,7 +75,7 @@ export const updateAgencyById = async (req: Request, res: Response) => { if (updateInfo.ParentId != null && updateInfo.ParentId === updateInfo.Id) { return res.status(403).send('An agency cannot be its own parent.'); } - const user = await userServices.getUser((req.user as SSOUser).preferred_username); + const user = req.pimsUser; const agency = await agencyService.updateAgencyById({ ...req.body, UpdatedById: user.Id }); return res.status(200).send(agency); }; diff --git a/express-api/src/controllers/buildings/buildingsController.ts b/express-api/src/controllers/buildings/buildingsController.ts index 2a3710e5f..4b5b51301 100644 --- a/express-api/src/controllers/buildings/buildingsController.ts +++ b/express-api/src/controllers/buildings/buildingsController.ts @@ -2,7 +2,6 @@ import { Request, Response } from 'express'; import * as buildingService from '@/services/buildings/buildingServices'; import { BuildingFilter, BuildingFilterSchema } from '@/services/buildings/buildingSchema'; import userServices from '@/services/users/usersServices'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Building } from '@/typeorm/Entities/Building'; import { checkUserAgencyPermission } from '@/utilities/authorizationChecks'; import { Roles } from '@/constants/roles'; @@ -19,16 +18,14 @@ import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; export const getBuildings = async (req: Request, res: Response) => { const filter = BuildingFilterSchema.safeParse(req.query); const includeRelations = req.query.includeRelations === 'true'; - const kcUser = req.user as unknown as SSOUser; + const user = req.pimsUser; if (!filter.success) { return res.status(400).send('Could not parse filter.'); } const filterResult = filter.data; - if ( - !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, Roles.AUDITOR])) - ) { + if (!user.hasOneOfRoles([Roles.ADMIN, Roles.AUDITOR])) { // get array of user's agencies - const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); + const usersAgencies = await userServices.getAgencies(user.Username); filterResult.agencyId = usersAgencies; } // Get parcels associated with agencies of the requesting user @@ -53,7 +50,7 @@ export const getBuilding = async (req: Request, res: Response) => { // admin and auditors are permitted to see any building const permittedRoles = [Roles.ADMIN, Roles.AUDITOR]; - const kcUser = req.user as unknown as SSOUser; + const user = req.pimsUser; const building = await buildingService.getBuildingById(buildingId); if (!building) { @@ -77,7 +74,7 @@ export const getBuilding = async (req: Request, res: Response) => { ); if ( - !(await checkUserAgencyPermission(kcUser, [building.AgencyId], permittedRoles)) && + !(await checkUserAgencyPermission(user, [building.AgencyId], permittedRoles)) && !isVisibleToOtherAgencies ) { return res.status(403).send('You are not authorized to view this building.'); @@ -96,9 +93,9 @@ export const updateBuilding = async (req: Request, res: Response) => { if (isNaN(buildingId) || buildingId !== req.body.Id) { return res.status(400).send('Building ID was invalid or mismatched with body.'); } - const user = await userServices.getUser((req.user as SSOUser).preferred_username); + const user = req.pimsUser; const updateBody = { ...req.body, UpdatedById: user.Id }; - const building = await buildingService.updateBuildingById(updateBody, req.user); + const building = await buildingService.updateBuildingById(updateBody, req.pimsUser); return res.status(200).send(building); }; @@ -128,7 +125,7 @@ export const deleteBuilding = async (req: Request, res: Response) => { * Note: the original implementation returns 200, but as a resource is created 201 is better. */ export const addBuilding = async (req: Request, res: Response) => { - const user = await userServices.getUser((req.user as SSOUser).preferred_username); + const user = req.pimsUser; const createBody: Building = { ...req.body, CreatedById: user.Id }; createBody.Evaluations = createBody.Evaluations?.map((evaluation) => ({ ...evaluation, diff --git a/express-api/src/controllers/notifications/notificationsController.ts b/express-api/src/controllers/notifications/notificationsController.ts index ce596eca9..3fde9ab2b 100644 --- a/express-api/src/controllers/notifications/notificationsController.ts +++ b/express-api/src/controllers/notifications/notificationsController.ts @@ -2,7 +2,6 @@ import notificationServices, { NotificationStatus, } from '@/services/notifications/notificationServices'; import userServices from '@/services/users/usersServices'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Request, Response } from 'express'; import { DisposalNotificationFilterSchema } from './notificationsSchema'; import projectServices from '@/services/projects/projectsServices'; @@ -22,14 +21,11 @@ export const getNotificationsByProjectId = async (req: Request, res: Response) = return res.status(400).send({ message: 'Could not parse filter.' }); } const filterResult = filter.data; - const kcUser = req.user as unknown as SSOUser; - const user = await userServices.getUser((req.user as SSOUser).preferred_username); + const user = req.pimsUser; - if ( - !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, Roles.AUDITOR])) - ) { + if (!user.hasOneOfRoles([Roles.ADMIN, Roles.AUDITOR])) { // get array of user's agencies - const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); + const usersAgencies = await userServices.getAgencies(user.Username); const project = await projectServices.getProjectById(filterResult.projectId); if (!usersAgencies.includes(project.AgencyId)) { @@ -57,16 +53,15 @@ export const getNotificationsByProjectId = async (req: Request, res: Response) = }; export const resendNotificationById = async (req: Request, res: Response) => { - const kcUser = req.user; - if (!userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN])) + const user = req.pimsUser; + if (!user.hasOneOfRoles([Roles.ADMIN])) return res.status(403).send('User lacks permissions to resend notification.'); const id = Number(req.params.id); const notification = await notificationServices.getNotificationById(id); if (!notification) { return res.status(404).send('Notification not found.'); } - const resultantNotification = await notificationServices.sendNotification(notification, kcUser); - const user = await userServices.getUser(kcUser.preferred_username); + const resultantNotification = await notificationServices.sendNotification(notification, user); const updatedNotification = await notificationServices.updateNotificationStatus( resultantNotification.Id, user, @@ -75,15 +70,14 @@ export const resendNotificationById = async (req: Request, res: Response) => { }; export const cancelNotificationById = async (req: Request, res: Response) => { - const kcUser = req.user; - if (!userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN])) + const user = req.pimsUser; + if (!user.hasOneOfRoles([Roles.ADMIN])) return res.status(403).send('User lacks permissions to cancel notification.'); const id = Number(req.params.id); const notification = await notificationServices.getNotificationById(id); if (!notification) { return res.status(404).send('Notification not found.'); } - const user = await userServices.getUser(kcUser.preferred_username); const resultantNotification = await notificationServices.cancelNotificationById( notification.Id, user, diff --git a/express-api/src/controllers/parcels/parcelsController.ts b/express-api/src/controllers/parcels/parcelsController.ts index 0ed798e9e..e81cd7c5a 100644 --- a/express-api/src/controllers/parcels/parcelsController.ts +++ b/express-api/src/controllers/parcels/parcelsController.ts @@ -1,7 +1,6 @@ import { Request, Response } from 'express'; import parcelServices from '@/services/parcels/parcelServices'; import { ParcelFilter, ParcelFilterSchema } from '@/services/parcels/parcelSchema'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import userServices from '@/services/users/usersServices'; import { Parcel } from '@/typeorm/Entities/Parcel'; import { Roles } from '@/constants/roles'; @@ -24,7 +23,7 @@ export const getParcel = async (req: Request, res: Response) => { // admin and auditors are permitted to see any parcel const permittedRoles = [Roles.ADMIN, Roles.AUDITOR]; - const kcUser = req.user as unknown as SSOUser; + const user = req.pimsUser; const parcel = await parcelServices.getParcelById(parcelId); if (!parcel) { @@ -48,7 +47,7 @@ export const getParcel = async (req: Request, res: Response) => { ); if ( - !(await checkUserAgencyPermission(kcUser, [parcel.AgencyId], permittedRoles)) && + !(await checkUserAgencyPermission(user, [parcel.AgencyId], permittedRoles)) && !isVisibleToOtherAgencies ) { return res.status(403).send('You are not authorized to view this parcel.'); @@ -67,9 +66,9 @@ export const updateParcel = async (req: Request, res: Response) => { if (isNaN(parcelId) || parcelId !== req.body.Id) { return res.status(400).send('Parcel ID was invalid or mismatched with body.'); } - const user = await userServices.getUser((req.user as SSOUser).preferred_username); + const user = req.pimsUser; const updateBody = { ...req.body, UpdatedById: user.Id }; - const parcel = await parcelServices.updateParcel(updateBody, req.user); + const parcel = await parcelServices.updateParcel(updateBody, req.pimsUser); if (!parcel) { return res.status(404).send('Parcel matching this internal ID not found.'); } @@ -100,17 +99,14 @@ export const deleteParcel = async (req: Request, res: Response) => { export const getParcels = async (req: Request, res: Response) => { const filter = ParcelFilterSchema.safeParse(req.query); const includeRelations = req.query.includeRelations === 'true'; - const kcUser = req.user as unknown as SSOUser; if (!filter.success) { return res.status(400).send('Could not parse filter.'); } const filterResult = filter.data; - - if ( - !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, Roles.AUDITOR])) - ) { + const user = req.pimsUser; + if (!user.hasOneOfRoles([Roles.ADMIN, Roles.AUDITOR])) { // get array of user's agencies - const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); + const usersAgencies = await userServices.getAgencies(user.Username); filterResult.agencyId = usersAgencies; } // Get parcels associated with agencies of the requesting user @@ -132,7 +128,7 @@ export const getParcels = async (req: Request, res: Response) => { * Note: the original implementation returns 200, but as a resource is created 201 is better. */ export const addParcel = async (req: Request, res: Response) => { - const user = await userServices.getUser((req.user as SSOUser).preferred_username); + const user = req.pimsUser; const parcel: Parcel = { ...req.body, CreatedById: user.Id }; parcel.Evaluations = parcel.Evaluations?.map((evaluation) => ({ ...evaluation, diff --git a/express-api/src/controllers/projects/projectsController.ts b/express-api/src/controllers/projects/projectsController.ts index 55d5f28e0..abee41521 100644 --- a/express-api/src/controllers/projects/projectsController.ts +++ b/express-api/src/controllers/projects/projectsController.ts @@ -1,6 +1,5 @@ import { ProjectFilterSchema, ProjectFilter } from '@/services/projects/projectSchema'; import { Request, Response } from 'express'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import projectServices, { ProjectPropertyIds } from '@/services/projects/projectsServices'; import userServices from '@/services/users/usersServices'; import { checkUserAgencyPermission } from '@/utilities/authorizationChecks'; @@ -19,7 +18,7 @@ import { exposedProjectStatuses } from '@/constants/projectStatus'; export const getDisposalProject = async (req: Request, res: Response) => { // admins are permitted to view any project const permittedRoles = [Roles.ADMIN]; - const user = req.user as SSOUser; + const user = req.pimsUser; const projectId = Number(req.params.projectId); if (isNaN(projectId)) { return res.status(400).send('Project ID was invalid.'); @@ -54,7 +53,8 @@ export const getDisposalProject = async (req: Request, res: Response) => { */ export const updateDisposalProject = async (req: Request, res: Response) => { // Only admins can edit projects - const isAdmin = await userServices.hasOneOfRoles(req.user?.preferred_username, [Roles.ADMIN]); + const user = req.pimsUser; + const isAdmin = user.hasOneOfRoles([Roles.ADMIN]); if (!isAdmin) { return res.status(403).send('Projects only editable by Administrator role.'); } @@ -74,9 +74,12 @@ export const updateDisposalProject = async (req: Request, res: Response) => { return res.status(400).send('The param ID does not match the request body.'); } // need to coordinate how we want tasks to be translated - const user = await userServices.getUser(req.user.preferred_username); const updateBody = { ...req.body.project, UpdatedById: user.Id }; - const project = await projectServices.updateProject(updateBody, req.body.propertyIds, req.user); + const project = await projectServices.updateProject( + updateBody, + req.body.propertyIds, + req.pimsUser, + ); return res.status(200).send(project); }; @@ -92,7 +95,8 @@ export const deleteDisposalProject = async (req: Request, res: Response) => { return res.status(400).send('Invalid Project ID'); } // Only admins can delete projects - const isAdmin = await userServices.hasOneOfRoles(req.user?.preferred_username, [Roles.ADMIN]); + const user = req.pimsUser; + const isAdmin = user.hasOneOfRoles([Roles.ADMIN]); if (!isAdmin) { return res.status(403).send('Projects can only be deleted by Administrator role.'); } @@ -114,7 +118,8 @@ export const deleteDisposalProject = async (req: Request, res: Response) => { */ export const addDisposalProject = async (req: Request, res: Response) => { // Auditors can no add projects - if (await userServices.hasOneOfRoles(req.user?.preferred_username, [Roles.AUDITOR])) { + const user = req.pimsUser; + if (user.hasOneOfRoles([Roles.AUDITOR])) { return res.status(403).send('Projects can not be added by user with Auditor role.'); } // Extract project data from request body @@ -123,7 +128,6 @@ export const addDisposalProject = async (req: Request, res: Response) => { project, projectPropertyIds, }: { project: DeepPartial; projectPropertyIds: ProjectPropertyIds } = req.body; - const user = await userServices.getUser((req.user as SSOUser).preferred_username); const addBody = { ...project, CreatedById: user.Id, @@ -132,11 +136,7 @@ export const addDisposalProject = async (req: Request, res: Response) => { }; // Call the addProject service function with the project data - const newProject = await projectServices.addProject( - addBody, - projectPropertyIds, - req.user as SSOUser, - ); + const newProject = await projectServices.addProject(addBody, projectPropertyIds, req.pimsUser); // Return the new project in the response return res.status(201).json(newProject); @@ -155,11 +155,11 @@ export const getProjects = async (req: Request, res: Response) => { return res.status(400).send('Could not parse filter.'); } const filterResult = filter.data; - const kcUser = req.user as unknown as SSOUser; - const isAdmin = await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN]); + const user = req.pimsUser; + const isAdmin = user.hasOneOfRoles([Roles.ADMIN]); if (!isAdmin) { // get array of user's agencies - const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); + const usersAgencies = await userServices.getAgencies(user.Username); filterResult.agencyId = usersAgencies; } // Get projects associated with agencies of the requesting user diff --git a/express-api/src/controllers/properties/propertiesController.ts b/express-api/src/controllers/properties/propertiesController.ts index 7cd692364..7acef9c2f 100644 --- a/express-api/src/controllers/properties/propertiesController.ts +++ b/express-api/src/controllers/properties/propertiesController.ts @@ -12,7 +12,6 @@ import userServices from '@/services/users/usersServices'; import { Worker } from 'worker_threads'; import path from 'path'; import fs from 'fs'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { AppDataSource } from '@/appDataSource'; import { ImportResult } from '@/typeorm/Entities/ImportResult'; import { readFile } from 'xlsx'; @@ -28,11 +27,11 @@ import { Roles } from '@/constants/roles'; export const getPropertiesFuzzySearch = async (req: Request, res: Response) => { const keyword = String(req.query.keyword); const take = req.query.take ? Number(req.query.take) : undefined; - const kcUser = req.user; + const user = req.pimsUser; let userAgencies; - const isAdmin = await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN]); + const isAdmin = user.hasOneOfRoles([Roles.ADMIN]); if (!isAdmin) { - userAgencies = await userServices.getAgencies(kcUser.preferred_username); + userAgencies = await userServices.getAgencies(user.Username); } const result = await propertyServices.propertiesFuzzySearch(keyword, take, userAgencies); return res.status(200).send(result); @@ -88,20 +87,20 @@ export const getPropertiesForMap = async (req: Request, res: Response) => { }; // Controlling for agency search visibility - const kcUser = req.user; const permittedRoles = [Roles.ADMIN, Roles.AUDITOR]; // Admins and auditors see all, otherwise... - if (!userServices.hasOneOfRoles(kcUser.preferred_username, permittedRoles)) { + const user = req.pimsUser; + if (!user.hasOneOfRoles(permittedRoles)) { const requestedAgencies = filterResult.AgencyIds; const userHasAgencies = await checkUserAgencyPermission( - kcUser, + user, requestedAgencies, permittedRoles, ); // If not agencies were requested or if the user doesn't have those requested agencies if (!requestedAgencies || !userHasAgencies) { // Then only show that user's agencies instead. - const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); + const usersAgencies = await userServices.getAgencies(user.Username); filterResult.UserAgencies = usersAgencies; } } @@ -131,8 +130,7 @@ export const getPropertiesForMap = async (req: Request, res: Response) => { export const importProperties = async (req: Request, res: Response) => { const filePath = req.file.path; const fileName = req.file.originalname; - const ssoUser = req.user; - const user = await userServices.getUser(ssoUser.preferred_username); + const user = req.pimsUser; const role = user.Role?.Name; try { readFile(filePath, { WTF: true }); //With this read option disabled it will throw if unexpected data is present. @@ -190,12 +188,12 @@ export const importProperties = async (req: Request, res: Response) => { * @returns Response with ImportFilterResult. */ export const getImportResults = async (req: Request, res: Response) => { - const kcUser = req.user as SSOUser; + const user = req.pimsUser; const filter = ImportResultFilterSchema.safeParse(req.query); if (filter.success == false) { return res.status(400).send(filter.error); } - const results = await propertyServices.getImportResults(filter.data, kcUser); + const results = await propertyServices.getImportResults(filter.data, user); return res.status(200).send(results); }; @@ -213,13 +211,11 @@ export const getPropertyUnion = async (req: Request, res: Response) => { return res.status(400).send(filter.error); } // Prevent getting back unrelated agencies for general users - const kcUser = req.user as unknown as SSOUser; + const user = req.pimsUser; const filterResult = filter.data; - if ( - !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, Roles.AUDITOR])) - ) { + if (!user.hasOneOfRoles([Roles.ADMIN, Roles.AUDITOR])) { // get array of user's agencies - const usersAgencies = await userServices.getAgencies(kcUser.preferred_username); + const usersAgencies = await userServices.getAgencies(user.Username); filterResult.agencyIds = usersAgencies; } diff --git a/express-api/src/controllers/reports/reportsController.ts b/express-api/src/controllers/reports/reportsController.ts index df2c3444a..33bab37a2 100644 --- a/express-api/src/controllers/reports/reportsController.ts +++ b/express-api/src/controllers/reports/reportsController.ts @@ -37,7 +37,7 @@ export const submitErrorReport = async (req: Request, res: Response) => { body: emailBody, }; - const response = await chesServices.sendEmailAsync(email, req.user); + const response = await chesServices.sendEmailAsync(email, req.pimsUser); return res.status(200).send({ ...errorParse, chesResponse: response, diff --git a/express-api/src/controllers/users/usersController.ts b/express-api/src/controllers/users/usersController.ts index 7887d40e9..b8bbc82f8 100644 --- a/express-api/src/controllers/users/usersController.ts +++ b/express-api/src/controllers/users/usersController.ts @@ -33,7 +33,7 @@ export const submitUserAccessRequest = async (req: Request, res: Response) => { }, config.accessRequest.notificationTemplateRPD, ); - await notificationServices.sendNotification(notifRPD, req.user); + await notificationServices.sendNotification(notifRPD, req.pimsUser); } catch (e) { logger.error(`Failed to deliver access request notification: ${e.message}`); } @@ -81,7 +81,7 @@ export const getSelf = async (req: Request, res: Response) => { * @returns {Response} A 200 status with a list of users. */ export const getUsers = async (req: Request, res: Response) => { - const ssoUser = req.user as unknown as SSOUser; + const user = req.pimsUser; const filter = UserFilteringSchema.safeParse(req.query); if (!filter.success) { return res.status(400).send('Failed to parse filter query.'); @@ -89,12 +89,12 @@ export const getUsers = async (req: Request, res: Response) => { const filterResult = filter.data; let users; - const isAdmin = await userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN]); + const isAdmin = user.hasOneOfRoles([Roles.ADMIN]); if (isAdmin) { users = await userServices.getUsers(filterResult as UserFiltering); } else { // Get agencies associated with the requesting user - const usersAgencies = await userServices.getAgencies(ssoUser.preferred_username); + const usersAgencies = await userServices.getAgencies(user.Username); filterResult.agencyId = usersAgencies; users = await userServices.getUsers(filterResult as UserFiltering); } @@ -110,16 +110,14 @@ export const getUsers = async (req: Request, res: Response) => { export const getUserById = async (req: Request, res: Response) => { const id = req.params.id; const uuid = z.string().uuid().safeParse(id); - const ssoUser = req.user as unknown as SSOUser; + const pimsUser = req.pimsUser; if (uuid.success) { const user = await userServices.getUserById(uuid.data); if (user) { - const isAdmin = await userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN]); + const isAdmin = pimsUser.hasOneOfRoles([Roles.ADMIN]); if (!isAdmin) { // check if user has the correct agencies - const usersAgencies = await userServices.hasAgencies(ssoUser.preferred_username, [ - user.AgencyId, - ]); + const usersAgencies = await userServices.hasAgencies(user.Username, [user.AgencyId]); if (!usersAgencies) { return res.status(403).send('User does not have permission to view this user'); } diff --git a/express-api/src/middleware/activeUserCheck.ts b/express-api/src/middleware/activeUserCheck.ts index d3568e759..7e194aebb 100644 --- a/express-api/src/middleware/activeUserCheck.ts +++ b/express-api/src/middleware/activeUserCheck.ts @@ -1,4 +1,5 @@ import { AppDataSource } from '@/appDataSource'; +import { Roles } from '@/constants/roles'; import { User } from '@/typeorm/Entities/User'; import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { NextFunction, RequestHandler, Response } from 'express'; @@ -11,7 +12,7 @@ import { NextFunction, RequestHandler, Response } from 'express'; * Also checks that user has a role parsed from their token. */ const activeUserCheck: unknown = async ( - req: Request & { user: SSOUser }, + req: Request & { user: SSOUser; pimsUser: PimsRequestUser }, res: Response, next: NextFunction, ) => { @@ -31,6 +32,17 @@ const activeUserCheck: unknown = async ( return res.status(404).send('Requesting user not found.'); } + const hasOneOfRoles = (roles: Roles[]): boolean => { + // No roles, then no permission. + if (!roles.length) { + return false; + } + return roles.includes(user.RoleId as Roles); + }; + + // Add this user info to the request so we don't have to query the database again. + req.pimsUser = { ...user, hasOneOfRoles }; + // Checking user status if (user.Status !== 'Active') { return res.status(403).send('Request forbidden. User lacks Active status.'); @@ -43,4 +55,18 @@ const activeUserCheck: unknown = async ( next(); }; +export type PimsRequestUser = User & { + hasOneOfRoles: (roles: Roles[]) => boolean; +}; + +// The token and user properties are not a part of the Request object by default. +declare global { + // eslint-disable-next-line @typescript-eslint/no-namespace + namespace Express { + interface Request { + pimsUser?: PimsRequestUser; + } + } +} + export default activeUserCheck as RequestHandler; diff --git a/express-api/src/services/buildings/buildingServices.ts b/express-api/src/services/buildings/buildingServices.ts index 662e2cb40..15862c1ec 100644 --- a/express-api/src/services/buildings/buildingServices.ts +++ b/express-api/src/services/buildings/buildingServices.ts @@ -8,8 +8,8 @@ import { BuildingEvaluation } from '@/typeorm/Entities/BuildingEvaluation'; import { BuildingFiscal } from '@/typeorm/Entities/BuildingFiscal'; import logger from '@/utilities/winstonLogger'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Roles } from '@/constants/roles'; +import { PimsRequestUser } from '@/middleware/activeUserCheck'; const buildingRepo = AppDataSource.getRepository(Building); @@ -63,13 +63,16 @@ export const getBuildingById = async (buildingId: number) => { * @returns {Building} The updated building * @throws {ErrorWithCode} Throws and error with 404 status if building does not exist. */ -export const updateBuildingById = async (building: DeepPartial, ssoUser: SSOUser) => { +export const updateBuildingById = async ( + building: DeepPartial, + user: PimsRequestUser, +) => { const existingBuilding = await getBuildingById(building.Id); if (!existingBuilding) { throw new ErrorWithCode('Building does not exists.', 404); } - const validUserAgencies = await userServices.getAgencies(ssoUser.preferred_username); - const isAdmin = await userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN]); + const validUserAgencies = await userServices.getAgencies(user.Username); + const isAdmin = user.hasOneOfRoles([Roles.ADMIN]); if (!isAdmin && !validUserAgencies.includes(building.AgencyId)) { throw new ErrorWithCode('This agency change is not permitted.', 403); } diff --git a/express-api/src/services/ches/chesServices.ts b/express-api/src/services/ches/chesServices.ts index 9884a7b0c..8b80f2b6d 100644 --- a/express-api/src/services/ches/chesServices.ts +++ b/express-api/src/services/ches/chesServices.ts @@ -1,10 +1,10 @@ import config from '@/constants/config'; import urls from '@/constants/urls'; import { ChesFilter } from '@/controllers/tools/toolsSchema'; +import { PimsRequestUser } from '@/middleware/activeUserCheck'; import { ErrorWithCode } from '@/utilities/customErrors/ErrorWithCode'; import { decodeJWT } from '@/utilities/decodeJWT'; import logger from '@/utilities/winstonLogger'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; let _token: TokenResponse = null; @@ -160,7 +160,10 @@ export interface IEmailSentResponse { * @param user - The SSO user information. * @returns A promise that resolves to the response of sending the email or null. */ -const sendEmailAsync = async (email: IEmail, user: SSOUser): Promise => { +const sendEmailAsync = async ( + email: IEmail, + user: PimsRequestUser, +): Promise => { const cfg = config(); if (email == null) { throw new ErrorWithCode('Null argument for email.', 400); @@ -169,7 +172,7 @@ const sendEmailAsync = async (email: IEmail, user: SSOUser): Promise email.trim()) - : [user.email]; - email.cc = email.cc?.length ? [user.email] : []; + : [user.Email]; + email.cc = email.cc?.length ? [user.Email] : []; email.bcc = []; } diff --git a/express-api/src/services/notifications/notificationServices.ts b/express-api/src/services/notifications/notificationServices.ts index fc7914d55..7339d1b6c 100644 --- a/express-api/src/services/notifications/notificationServices.ts +++ b/express-api/src/services/notifications/notificationServices.ts @@ -15,12 +15,12 @@ import chesServices, { IChesStatusResponse, IEmail, } from '../ches/chesServices'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { ProjectAgencyResponse } from '@/typeorm/Entities/ProjectAgencyResponse'; import logger from '@/utilities/winstonLogger'; import getConfig from '@/constants/config'; import { getDaysBetween } from '@/utilities/helperFunctions'; import { ProjectStatusHistory } from '@/typeorm/Entities/ProjectStatusHistory'; +import { PimsRequestUser } from '@/middleware/activeUserCheck'; export interface AccessRequestData { FirstName: string; @@ -380,7 +380,7 @@ const generateProjectNotifications = async ( */ const sendNotification = async ( notification: NotificationQueue, - user: SSOUser, + user: PimsRequestUser, queryRunner?: QueryRunner, ) => { const query = queryRunner ?? AppDataSource.createQueryRunner(); diff --git a/express-api/src/services/parcels/parcelServices.ts b/express-api/src/services/parcels/parcelServices.ts index c55a4be75..20d40687e 100644 --- a/express-api/src/services/parcels/parcelServices.ts +++ b/express-api/src/services/parcels/parcelServices.ts @@ -8,8 +8,8 @@ import { ParcelFiscal } from '@/typeorm/Entities/ParcelFiscal'; import userServices from '../users/usersServices'; import logger from '@/utilities/winstonLogger'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Roles } from '@/constants/roles'; +import { PimsRequestUser } from '@/middleware/activeUserCheck'; const parcelRepo = AppDataSource.getRepository(Parcel); @@ -155,7 +155,7 @@ const getParcels = async (filter: ParcelFilter, includeRelations: boolean = fals * @returns updated parcel information and status * @throws Error with code if parcel is not found or if an unexpected error is hit on update */ -const updateParcel = async (incomingParcel: DeepPartial, ssoUser: SSOUser) => { +const updateParcel = async (incomingParcel: DeepPartial, user: PimsRequestUser) => { if (incomingParcel.PID == null && incomingParcel.PIN == null) { throw new ErrorWithCode('Must include PID or PIN in parcel data.', 400); } @@ -163,8 +163,8 @@ const updateParcel = async (incomingParcel: DeepPartial, ssoUser: SSOUse if (findParcel == null || findParcel.Id !== incomingParcel.Id) { throw new ErrorWithCode('Parcel not found', 404); } - const validUserAgencies = await userServices.getAgencies(ssoUser.preferred_username); - const isAdmin = await userServices.hasOneOfRoles(ssoUser.preferred_username, [Roles.ADMIN]); + const validUserAgencies = await userServices.getAgencies(user.Username); + const isAdmin = user.hasOneOfRoles([Roles.ADMIN]); if (!isAdmin && !validUserAgencies.includes(incomingParcel.AgencyId)) { throw new ErrorWithCode('This agency change is not permitted.', 403); } diff --git a/express-api/src/services/projects/projectsServices.ts b/express-api/src/services/projects/projectsServices.ts index 50e779caf..d54b4f57f 100644 --- a/express-api/src/services/projects/projectsServices.ts +++ b/express-api/src/services/projects/projectsServices.ts @@ -26,7 +26,6 @@ import { ProjectFilter } from '@/services/projects/projectSchema'; import { PropertyType } from '@/constants/propertyType'; import { ProjectRisk } from '@/constants/projectRisk'; import notificationServices, { AgencyResponseType } from '../notifications/notificationServices'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import userServices from '../users/usersServices'; import { constructFindOptionFromQuery, @@ -38,6 +37,7 @@ import { NotificationQueue } from '@/typeorm/Entities/NotificationQueue'; import { SortOrders } from '@/constants/types'; import { ProjectJoin } from '@/typeorm/Entities/views/ProjectJoinView'; import { Roles } from '@/constants/roles'; +import { PimsRequestUser } from '@/middleware/activeUserCheck'; const projectRepo = AppDataSource.getRepository(Project); @@ -123,14 +123,14 @@ const getProjectById = async (id: number) => { * * @param project - The project object to be added. * @param propertyIds - The IDs of the properties (parcels and buildings) to be associated with the project. - * @param {SSOUser} ssoUser The user making the add request. + * @param {PimsRequestUser} user The user making the add request. * @returns The newly created project. * @throws ErrorWithCode - If the project name is missing, agency is not found, or there is an error creating the project. */ const addProject = async ( project: DeepPartial, propertyIds: ProjectPropertyIds, - ssoUser: SSOUser, + user: PimsRequestUser, ) => { // Does the project have a name? if (!project.Name) throw new ErrorWithCode('Projects must have a name.', 400); @@ -173,7 +173,7 @@ const addProject = async ( newProject.Id, null, newProject.AgencyResponses ?? [], - ssoUser, + user, queryRunner, ); await queryRunner.commitTransaction(); @@ -388,7 +388,7 @@ const handleProjectNotifications = async ( projectId: number, previousStatus: number, responses: ProjectAgencyResponse[], - user: SSOUser, + user: PimsRequestUser, queryRunner: QueryRunner, ) => { const projectWithRelations = await queryRunner.manager.findOne(Project, { @@ -655,7 +655,7 @@ const getAgencyResponseChanges = async ( const updateProject = async ( project: DeepPartial, propertyIds: ProjectPropertyIds, - user: SSOUser, + user: PimsRequestUser, ) => { // Project must still have a name // undefined is allowed because it is not always updated @@ -680,7 +680,7 @@ const updateProject = async ( //Agency change disallowed unless admin. project.AgencyId && originalProject.AgencyId !== project.AgencyId && - !(await userServices.hasOneOfRoles(user.preferred_username, [Roles.ADMIN])) + !user.hasOneOfRoles([Roles.ADMIN]) ) { throw new ErrorWithCode('Project Agency may not be changed.', 403); } diff --git a/express-api/src/services/properties/propertiesServices.ts b/express-api/src/services/properties/propertiesServices.ts index 54abf50a6..548959469 100644 --- a/express-api/src/services/properties/propertiesServices.ts +++ b/express-api/src/services/properties/propertiesServices.ts @@ -31,13 +31,13 @@ import { } from '@/utilities/helperFunctions'; import userServices from '../users/usersServices'; import { Brackets, FindOptionsWhere, ILike, In, QueryRunner } from 'typeorm'; -import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { PropertyType } from '@/constants/propertyType'; import { exposedProjectStatuses, ProjectStatus } from '@/constants/projectStatus'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; import { ProjectStatus as ProjectStatusEntity } from '@/typeorm/Entities/ProjectStatus'; import { parentPort } from 'worker_threads'; import { ErrorWithCode } from '@/utilities/customErrors/ErrorWithCode'; +import { PimsRequestUser } from '@/middleware/activeUserCheck'; /** * Perform a fuzzy search for properties based on the provided keyword. @@ -790,11 +790,10 @@ const importPropertiesAsJSON = async ( /** * Retrieves import results based on the provided filter and user. * @param filter - The filter to apply to the import results. - * @param ssoUser - The SSO user requesting the import results. + * @param user - The SSO user requesting the import results. * @returns A promise that resolves to the import results matching the filter criteria. */ -const getImportResults = async (filter: ImportResultFilter, ssoUser: SSOUser) => { - const user = await userServices.getUser(ssoUser.preferred_username); +const getImportResults = async (filter: ImportResultFilter, user: PimsRequestUser) => { return AppDataSource.getRepository(ImportResult).find({ where: { CreatedById: user.Id, diff --git a/express-api/src/services/users/usersServices.ts b/express-api/src/services/users/usersServices.ts index a50c0f7b2..f339e5c73 100644 --- a/express-api/src/services/users/usersServices.ts +++ b/express-api/src/services/users/usersServices.ts @@ -7,7 +7,6 @@ import { randomUUID, UUID } from 'crypto'; import KeycloakService from '@/services/keycloak/keycloakService'; import { ErrorWithCode } from '@/utilities/customErrors/ErrorWithCode'; import { UserFiltering } from '@/controllers/users/usersSchema'; -import { Roles } from '@/constants/roles'; interface NormalizedKeycloakUser { first_name: string; @@ -128,28 +127,6 @@ const hasAgencies = async (username: string, agencies: number[]) => { return result; }; -const hasOneOfRoles = async (username: string, roles: Roles[]) => { - // No username or roles, then no permission. - if (!username || !roles.length) { - return false; - } - // Create where objects - const orWhereClauses = roles.map((r) => ({ - Username: username, - Role: { - Name: r, - }, - })); - // Does the user exist with one of these roles? - const result = await AppDataSource.getRepository(User).exists({ - relations: { - Role: true, - }, - where: orWhereClauses, - }); - return result; -}; - /** * Maps the sort key and direction to the corresponding object structure for TypeORM find options. * @param sortKey - The key to sort by. @@ -308,7 +285,6 @@ const userServices = { getUser, addKeycloakUserOnHold, hasAgencies, - hasOneOfRoles, getAgencies, normalizeKeycloakUser, getUsers, diff --git a/express-api/src/utilities/authorizationChecks.ts b/express-api/src/utilities/authorizationChecks.ts index 8a6ca21cd..222eb4ab8 100644 --- a/express-api/src/utilities/authorizationChecks.ts +++ b/express-api/src/utilities/authorizationChecks.ts @@ -1,6 +1,7 @@ import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Roles } from '@/constants/roles'; import userServices, { getUser } from '@/services/users/usersServices'; +import { PimsRequestUser } from '@/middleware/activeUserCheck'; /** * Function to check if user can edit. @@ -8,12 +9,9 @@ import userServices, { getUser } from '@/services/users/usersServices'; * @param user - The user object containing information about the user. * @returns A boolean value indicating whether the user can edit or not. */ -export const canUserEdit = async (user: SSOUser): Promise => { +export const canUserEdit = async (user: PimsRequestUser): Promise => { // as they are not an auditor the user can edit - return await userServices.hasOneOfRoles(user.preferred_username, [ - Roles.GENERAL_USER, - Roles.ADMIN, - ]); + return user.hasOneOfRoles([Roles.GENERAL_USER, Roles.ADMIN]); }; /** @@ -46,7 +44,7 @@ export const isUserActive = async (kcUser: SSOUser): Promise => { * @returns A Promise that resolves to a boolean indicating whether the user has read permission. */ export const checkUserAgencyPermission = async ( - kcUser: SSOUser, + user: PimsRequestUser, agencyIds: number[], permittedRoles: Roles[], ): Promise => { @@ -55,11 +53,9 @@ export const checkUserAgencyPermission = async ( return false; } // if the user is not an admin, nor has a permitted role scope results - if ( - !(await userServices.hasOneOfRoles(kcUser.preferred_username, [Roles.ADMIN, ...permittedRoles])) - ) { + if (!user.hasOneOfRoles([Roles.ADMIN, ...permittedRoles])) { // check if current user belongs to any of the specified agencies - const userAgencies = await userServices.hasAgencies(kcUser.preferred_username, agencyIds); + const userAgencies = await userServices.hasAgencies(user.Username, agencyIds); return userAgencies; } // Admins have permission by default From 6087cac83a45ae31cd8ff0fb8f9fdb8ed6d9e511 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Fri, 13 Sep 2024 16:26:08 -0700 Subject: [PATCH 04/15] replace more getUser calls where possible --- express-api/src/controllers/buildings/buildingsController.ts | 5 +---- express-api/src/controllers/parcels/parcelsController.ts | 2 +- express-api/src/controllers/projects/projectsController.ts | 5 +---- express-api/src/services/buildings/buildingServices.ts | 4 ++-- express-api/src/services/parcels/parcelServices.ts | 4 ++-- express-api/src/services/projects/projectsServices.ts | 5 ++--- 6 files changed, 9 insertions(+), 16 deletions(-) diff --git a/express-api/src/controllers/buildings/buildingsController.ts b/express-api/src/controllers/buildings/buildingsController.ts index 4b5b51301..f8ad3a245 100644 --- a/express-api/src/controllers/buildings/buildingsController.ts +++ b/express-api/src/controllers/buildings/buildingsController.ts @@ -110,10 +110,7 @@ export const deleteBuilding = async (req: Request, res: Response) => { if (isNaN(buildingId)) { return res.status(400).send('Building ID was invalid.'); } - const delResult = await buildingService.deleteBuildingById( - buildingId, - req.user.preferred_username, - ); + const delResult = await buildingService.deleteBuildingById(buildingId, req.pimsUser); return res.status(200).send(delResult); }; diff --git a/express-api/src/controllers/parcels/parcelsController.ts b/express-api/src/controllers/parcels/parcelsController.ts index e81cd7c5a..fd669c059 100644 --- a/express-api/src/controllers/parcels/parcelsController.ts +++ b/express-api/src/controllers/parcels/parcelsController.ts @@ -86,7 +86,7 @@ export const deleteParcel = async (req: Request, res: Response) => { if (isNaN(parcelId)) { return res.status(400).send('Parcel ID was invalid.'); } - const delResult = await parcelServices.deleteParcelById(parcelId, req.user.preferred_username); + const delResult = await parcelServices.deleteParcelById(parcelId, req.pimsUser); return res.status(200).send(delResult); }; diff --git a/express-api/src/controllers/projects/projectsController.ts b/express-api/src/controllers/projects/projectsController.ts index abee41521..1f226b6f1 100644 --- a/express-api/src/controllers/projects/projectsController.ts +++ b/express-api/src/controllers/projects/projectsController.ts @@ -101,10 +101,7 @@ export const deleteDisposalProject = async (req: Request, res: Response) => { return res.status(403).send('Projects can only be deleted by Administrator role.'); } - const delProject = await projectServices.deleteProjectById( - projectId, - req.user.preferred_username, - ); + const delProject = await projectServices.deleteProjectById(projectId, req.pimsUser); const notifications = await notificationServices.cancelProjectNotifications(projectId); return res.status(200).send({ project: delProject, notifications }); diff --git a/express-api/src/services/buildings/buildingServices.ts b/express-api/src/services/buildings/buildingServices.ts index 15862c1ec..d5e8a15b8 100644 --- a/express-api/src/services/buildings/buildingServices.ts +++ b/express-api/src/services/buildings/buildingServices.ts @@ -10,6 +10,7 @@ import logger from '@/utilities/winstonLogger'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; import { Roles } from '@/constants/roles'; import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { User } from '@/typeorm/Entities/User'; const buildingRepo = AppDataSource.getRepository(Building); @@ -128,7 +129,7 @@ export const updateBuildingById = async ( * @returns A promise that resolves to the removed building entity. * @throws Error if the building does not exist, is linked to projects, or an error occurs during the deletion process. */ -export const deleteBuildingById = async (buildingId: number, username: string) => { +export const deleteBuildingById = async (buildingId: number, user: User) => { const existingBuilding = await getBuildingById(buildingId); if (!existingBuilding) { throw new ErrorWithCode('Building does not exists.', 404); @@ -142,7 +143,6 @@ export const deleteBuildingById = async (buildingId: number, username: string) = 403, ); } - const user = await userServices.getUser(username); const queryRunner = await AppDataSource.createQueryRunner(); await queryRunner.startTransaction(); try { diff --git a/express-api/src/services/parcels/parcelServices.ts b/express-api/src/services/parcels/parcelServices.ts index 20d40687e..a02ead314 100644 --- a/express-api/src/services/parcels/parcelServices.ts +++ b/express-api/src/services/parcels/parcelServices.ts @@ -10,6 +10,7 @@ import logger from '@/utilities/winstonLogger'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; import { Roles } from '@/constants/roles'; import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { User } from '@/typeorm/Entities/User'; const parcelRepo = AppDataSource.getRepository(Parcel); @@ -42,7 +43,7 @@ const addParcel = async (parcel: DeepPartial) => { * @returns object with data on number of rows affected. * @throws ErrorWithCode if no parcels have the ID sent in */ -const deleteParcelById = async (parcelId: number, username: string) => { +const deleteParcelById = async (parcelId: number, user: User) => { const existingParcel = await getParcelById(parcelId); if (!existingParcel) { throw new ErrorWithCode('Parcel PID was not found.', 404); @@ -56,7 +57,6 @@ const deleteParcelById = async (parcelId: number, username: string) => { 403, ); } - const user = await userServices.getUser(username); const queryRunner = await AppDataSource.createQueryRunner(); await queryRunner.startTransaction(); try { diff --git a/express-api/src/services/projects/projectsServices.ts b/express-api/src/services/projects/projectsServices.ts index d54b4f57f..d5d0b1d56 100644 --- a/express-api/src/services/projects/projectsServices.ts +++ b/express-api/src/services/projects/projectsServices.ts @@ -26,7 +26,6 @@ import { ProjectFilter } from '@/services/projects/projectSchema'; import { PropertyType } from '@/constants/propertyType'; import { ProjectRisk } from '@/constants/projectRisk'; import notificationServices, { AgencyResponseType } from '../notifications/notificationServices'; -import userServices from '../users/usersServices'; import { constructFindOptionFromQuery, constructFindOptionFromQuerySingleSelect, @@ -38,6 +37,7 @@ import { SortOrders } from '@/constants/types'; import { ProjectJoin } from '@/typeorm/Entities/views/ProjectJoinView'; import { Roles } from '@/constants/roles'; import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { User } from '@/typeorm/Entities/User'; const projectRepo = AppDataSource.getRepository(Project); @@ -796,11 +796,10 @@ const updateProject = async ( * @returns {Promise} - A promise that resolves to the delete result. * @throws {ErrorWithCode} - If the project does not exist, or if there is an error deleting the project. */ -const deleteProjectById = async (id: number, username: string) => { +const deleteProjectById = async (id: number, user: User) => { if (!(await projectRepo.exists({ where: { Id: id } }))) { throw new ErrorWithCode('Project does not exist.', 404); } - const user = await userServices.getUser(username); const queryRunner = await AppDataSource.createQueryRunner(); await queryRunner.startTransaction(); try { From bc4c263fad39c83234c2f2930a28177c13c77d98 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Tue, 17 Sep 2024 08:20:30 -0700 Subject: [PATCH 05/15] active user middleware update --- express-api/src/express.ts | 2 +- express-api/src/middleware/activeUserCheck.ts | 83 ++++++++++--------- .../src/routes/administrativeAreasRouter.ts | 9 +- 3 files changed, 51 insertions(+), 43 deletions(-) diff --git a/express-api/src/express.ts b/express-api/src/express.ts index 47d038ebb..549dab666 100644 --- a/express-api/src/express.ts +++ b/express-api/src/express.ts @@ -83,7 +83,7 @@ app.use(`/v2/health`, router.healthRouter); // Protected Routes app.use(`/v2/ltsa`, protectedRoute(), router.ltsaRouter); -app.use(`/v2/administrativeAreas`, protectedRoute(), router.administrativeAreasRouter); +app.use(`/v2/administrativeAreas`, router.administrativeAreasRouter); app.use(`/v2/agencies`, protectedRoute(), router.agenciesRouter); app.use('/v2/lookup', protectedRoute(), router.lookupRouter); app.use(`/v2/users`, protectedRoute(), router.usersRouter); diff --git a/express-api/src/middleware/activeUserCheck.ts b/express-api/src/middleware/activeUserCheck.ts index 7e194aebb..c22424b6d 100644 --- a/express-api/src/middleware/activeUserCheck.ts +++ b/express-api/src/middleware/activeUserCheck.ts @@ -11,48 +11,57 @@ import { NextFunction, RequestHandler, Response } from 'express'; * Successful checks result in the request passed on. * Also checks that user has a role parsed from their token. */ -const activeUserCheck: unknown = async ( - req: Request & { user: SSOUser; pimsUser: PimsRequestUser }, - res: Response, - next: NextFunction, -) => { - // Checking Keycloak user - const kcUser = req.user; - if (!kcUser) { - return res.status(401).send('Unauthorized request.'); - } +const activeUserCheck = (requiredRoles?: Roles[]): RequestHandler => { + const check = async ( + req: Request & { user?: SSOUser; pimsUser?: PimsRequestUser }, + res: Response, + next: NextFunction, + ) => { + // Checking Keycloak user + const kcUser = req.user; + if (!kcUser) { + return res.status(401).send('Requestor not authenticated by Keycloak.'); + } - // Checking user existence - const user = await AppDataSource.getRepository(User).findOne({ - where: { - Username: kcUser.preferred_username, - }, - }); - if (!user) { - return res.status(404).send('Requesting user not found.'); - } + // Checking user existence + const user = await AppDataSource.getRepository(User).findOne({ + where: { + Username: kcUser.preferred_username, + }, + }); + if (!user) { + return res.status(404).send('Requesting user not found.'); + } + + const hasOneOfRoles = (roles: Roles[]): boolean => { + // No roles, then no permission. + if (!roles || !roles.length) { + return false; + } + return roles.includes(user.RoleId as Roles); + }; - const hasOneOfRoles = (roles: Roles[]): boolean => { - // No roles, then no permission. - if (!roles.length) { - return false; + // Check that user has a role, any role + if (!user.RoleId) { + return res.status(403).send('Request forbidden. User has no assigned role.'); } - return roles.includes(user.RoleId as Roles); - }; - // Add this user info to the request so we don't have to query the database again. - req.pimsUser = { ...user, hasOneOfRoles }; + // Were specific roles required for access? + if (requiredRoles && !hasOneOfRoles(requiredRoles)) { + return res.status(403).send('Request forbidden. User lacks required roles.'); + } - // Checking user status - if (user.Status !== 'Active') { - return res.status(403).send('Request forbidden. User lacks Active status.'); - } + // Checking user status + if (user.Status !== 'Active') { + return res.status(403).send('Request forbidden. User lacks Active status.'); + } - // Check that user has a role - if (!user.RoleId) { - return res.status(403).send('Request forbidden. User has no assigned role.'); - } - next(); + // Add this user info to the request so we don't have to query the database again. + req.pimsUser = { ...user, hasOneOfRoles }; + + next(); + }; + return check as unknown as RequestHandler; }; export type PimsRequestUser = User & { @@ -69,4 +78,4 @@ declare global { } } -export default activeUserCheck as RequestHandler; +export default activeUserCheck; diff --git a/express-api/src/routes/administrativeAreasRouter.ts b/express-api/src/routes/administrativeAreasRouter.ts index cd46cfd20..b8c42e22a 100644 --- a/express-api/src/routes/administrativeAreasRouter.ts +++ b/express-api/src/routes/administrativeAreasRouter.ts @@ -7,7 +7,6 @@ import { } from '@/controllers/administrativeAreas/administrativeAreasController'; import activeUserCheck from '@/middleware/activeUserCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; -import { protectedRoute } from '@bcgov/citz-imb-sso-express'; import express from 'express'; const router = express.Router(); @@ -15,12 +14,12 @@ const router = express.Router(); // Endpoints for Admin Administrative Areas router .route(`/`) - .get(activeUserCheck, catchErrors(getAdministrativeAreas)) - .post(protectedRoute([Roles.ADMIN]), activeUserCheck, catchErrors(addAdministrativeArea)); + .get(activeUserCheck([Roles.ADMIN]), catchErrors(getAdministrativeAreas)) + .post(activeUserCheck([Roles.ADMIN]), catchErrors(addAdministrativeArea)); router .route(`/:id`) - .get(activeUserCheck, catchErrors(getAdministrativeAreaById)) - .put(protectedRoute([Roles.ADMIN]), activeUserCheck, catchErrors(updateAdministrativeAreaById)); + .get(activeUserCheck([Roles.ADMIN]), catchErrors(getAdministrativeAreaById)) + .put(activeUserCheck([Roles.ADMIN]), catchErrors(updateAdministrativeAreaById)); export default router; From 73fc134a1af57a46f4d83bcb98357462dfc93a88 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Tue, 17 Sep 2024 10:37:38 -0700 Subject: [PATCH 06/15] userAuthCheck application --- express-api/src/express.ts | 30 ++++++++++++++----- .../{activeUserCheck.ts => userAuthCheck.ts} | 15 ++++++---- .../src/routes/administrativeAreasRouter.ts | 11 ++----- express-api/src/routes/agenciesRouter.ts | 14 +++------ express-api/src/routes/buildingsRouter.ts | 12 +++----- express-api/src/routes/ltsaRouter.ts | 3 +- express-api/src/routes/notificationsRouter.ts | 9 +++--- express-api/src/routes/parcelsRouter.ts | 12 +++----- express-api/src/routes/projectsRouter.ts | 13 ++++---- express-api/src/routes/propertiesRouter.ts | 21 ++++++++----- express-api/src/routes/toolsRouter.ts | 7 ++--- express-api/src/routes/usersRouter.ts | 11 ++++--- .../services/buildings/buildingServices.ts | 2 +- express-api/src/services/ches/chesServices.ts | 2 +- .../notifications/notificationServices.ts | 2 +- .../src/services/parcels/parcelServices.ts | 2 +- .../src/services/projects/projectsServices.ts | 2 +- .../services/properties/propertiesServices.ts | 2 +- .../src/utilities/authorizationChecks.ts | 2 +- 19 files changed, 86 insertions(+), 86 deletions(-) rename express-api/src/middleware/{activeUserCheck.ts => userAuthCheck.ts} (86%) diff --git a/express-api/src/express.ts b/express-api/src/express.ts index 549dab666..d69e5f9a8 100644 --- a/express-api/src/express.ts +++ b/express-api/src/express.ts @@ -4,7 +4,7 @@ import cookieParser from 'cookie-parser'; import compression from 'compression'; import cors from 'cors'; import rateLimit from 'express-rate-limit'; -import { sso, protectedRoute } from '@bcgov/citz-imb-sso-express'; +import { protectedRoute, sso } from '@bcgov/citz-imb-sso-express'; import router from '@/routes'; import middleware from '@/middleware'; import constants from '@/constants'; @@ -15,6 +15,8 @@ import errorHandler from '@/middleware/errorHandler'; import { EndpointNotFound404 } from '@/constants/errors'; import nunjucks from 'nunjucks'; import OPENAPI_OPTIONS from '@/swagger/swaggerConfig'; +import userAuthCheck from '@/middleware/userAuthCheck'; +import { Roles } from '@/constants/roles'; const app: Application = express(); @@ -82,18 +84,30 @@ app.use(`/v2`, headerHandler as RequestHandler); app.use(`/v2/health`, router.healthRouter); // Protected Routes -app.use(`/v2/ltsa`, protectedRoute(), router.ltsaRouter); -app.use(`/v2/administrativeAreas`, router.administrativeAreasRouter); -app.use(`/v2/agencies`, protectedRoute(), router.agenciesRouter); +// userRequestCheck applied here if same permissions throughout route +// These routes must use protectedRoute before userAuthCheck +app.use(`/v2/ltsa`, protectedRoute(), userAuthCheck(), router.ltsaRouter); +app.use( + `/v2/administrativeAreas`, + protectedRoute(), + userAuthCheck({ requiredRoles: [Roles.ADMIN] }), + router.administrativeAreasRouter, +); +app.use( + `/v2/agencies`, + protectedRoute(), + userAuthCheck({ requiredRoles: [Roles.ADMIN] }), + router.agenciesRouter, +); app.use('/v2/lookup', protectedRoute(), router.lookupRouter); app.use(`/v2/users`, protectedRoute(), router.usersRouter); app.use(`/v2/properties`, protectedRoute(), router.propertiesRouter); -app.use(`/v2/parcels`, protectedRoute(), router.parcelsRouter); -app.use(`/v2/buildings`, protectedRoute(), router.buildingsRouter); +app.use(`/v2/parcels`, protectedRoute(), userAuthCheck(), router.parcelsRouter); +app.use(`/v2/buildings`, protectedRoute(), userAuthCheck(), router.buildingsRouter); app.use(`/v2/notifications`, protectedRoute(), router.notificationsRouter); app.use(`/v2/projects`, protectedRoute(), router.projectsRouter); -app.use(`/v2/reports`, protectedRoute(), router.reportsRouter); -app.use(`/v2/tools`, protectedRoute(), router.toolsRouter); +app.use(`/v2/reports`, protectedRoute(), userAuthCheck(), router.reportsRouter); +app.use(`/v2/tools`, protectedRoute(), userAuthCheck(), router.toolsRouter); // If a non-existent route is called. Must go after other routes. app.use('*', (_req, _res, next) => next(EndpointNotFound404)); diff --git a/express-api/src/middleware/activeUserCheck.ts b/express-api/src/middleware/userAuthCheck.ts similarity index 86% rename from express-api/src/middleware/activeUserCheck.ts rename to express-api/src/middleware/userAuthCheck.ts index c22424b6d..6b151112c 100644 --- a/express-api/src/middleware/activeUserCheck.ts +++ b/express-api/src/middleware/userAuthCheck.ts @@ -4,6 +4,11 @@ import { User } from '@/typeorm/Entities/User'; import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { NextFunction, RequestHandler, Response } from 'express'; +export interface UserCheckOptions { + requiredRoles?: Roles[]; + onlyAuthenticated?: boolean; +} + /** * Middleware that checks for a user with Active status. * If the user lacks that status, isn't found, @@ -11,7 +16,7 @@ import { NextFunction, RequestHandler, Response } from 'express'; * Successful checks result in the request passed on. * Also checks that user has a role parsed from their token. */ -const activeUserCheck = (requiredRoles?: Roles[]): RequestHandler => { +const userAuthCheck = (options?: UserCheckOptions): RequestHandler => { const check = async ( req: Request & { user?: SSOUser; pimsUser?: PimsRequestUser }, res: Response, @@ -22,7 +27,6 @@ const activeUserCheck = (requiredRoles?: Roles[]): RequestHandler => { if (!kcUser) { return res.status(401).send('Requestor not authenticated by Keycloak.'); } - // Checking user existence const user = await AppDataSource.getRepository(User).findOne({ where: { @@ -32,7 +36,6 @@ const activeUserCheck = (requiredRoles?: Roles[]): RequestHandler => { if (!user) { return res.status(404).send('Requesting user not found.'); } - const hasOneOfRoles = (roles: Roles[]): boolean => { // No roles, then no permission. if (!roles || !roles.length) { @@ -47,7 +50,7 @@ const activeUserCheck = (requiredRoles?: Roles[]): RequestHandler => { } // Were specific roles required for access? - if (requiredRoles && !hasOneOfRoles(requiredRoles)) { + if (options?.requiredRoles && !hasOneOfRoles(options.requiredRoles)) { return res.status(403).send('Request forbidden. User lacks required roles.'); } @@ -68,7 +71,7 @@ export type PimsRequestUser = User & { hasOneOfRoles: (roles: Roles[]) => boolean; }; -// The token and user properties are not a part of the Request object by default. +// Ensure pimsUsers is a part of the Request object by default. declare global { // eslint-disable-next-line @typescript-eslint/no-namespace namespace Express { @@ -78,4 +81,4 @@ declare global { } } -export default activeUserCheck; +export default userAuthCheck; diff --git a/express-api/src/routes/administrativeAreasRouter.ts b/express-api/src/routes/administrativeAreasRouter.ts index b8c42e22a..26b075c3b 100644 --- a/express-api/src/routes/administrativeAreasRouter.ts +++ b/express-api/src/routes/administrativeAreasRouter.ts @@ -1,25 +1,20 @@ -import { Roles } from '@/constants/roles'; import { getAdministrativeAreas, addAdministrativeArea, getAdministrativeAreaById, updateAdministrativeAreaById, } from '@/controllers/administrativeAreas/administrativeAreasController'; -import activeUserCheck from '@/middleware/activeUserCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; import express from 'express'; const router = express.Router(); // Endpoints for Admin Administrative Areas -router - .route(`/`) - .get(activeUserCheck([Roles.ADMIN]), catchErrors(getAdministrativeAreas)) - .post(activeUserCheck([Roles.ADMIN]), catchErrors(addAdministrativeArea)); +router.route(`/`).get(catchErrors(getAdministrativeAreas)).post(catchErrors(addAdministrativeArea)); router .route(`/:id`) - .get(activeUserCheck([Roles.ADMIN]), catchErrors(getAdministrativeAreaById)) - .put(activeUserCheck([Roles.ADMIN]), catchErrors(updateAdministrativeAreaById)); + .get(catchErrors(getAdministrativeAreaById)) + .put(catchErrors(updateAdministrativeAreaById)); export default router; diff --git a/express-api/src/routes/agenciesRouter.ts b/express-api/src/routes/agenciesRouter.ts index 788d570de..800de1adf 100644 --- a/express-api/src/routes/agenciesRouter.ts +++ b/express-api/src/routes/agenciesRouter.ts @@ -1,4 +1,3 @@ -import { Roles } from '@/constants/roles'; import { addAgency, deleteAgencyById, @@ -6,23 +5,18 @@ import { getAgencyById, updateAgencyById, } from '@/controllers/agencies/agenciesController'; -import activeUserCheck from '@/middleware/activeUserCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; -import { protectedRoute } from '@bcgov/citz-imb-sso-express'; import express from 'express'; const router = express.Router(); // Endpoints for Admin Agencies -router - .route(`/`) - .get(activeUserCheck, catchErrors(getAgencies)) - .post(protectedRoute([Roles.ADMIN]), activeUserCheck, catchErrors(addAgency)); +router.route(`/`).get(catchErrors(getAgencies)).post(catchErrors(addAgency)); router .route(`/:id`) - .get(activeUserCheck, catchErrors(getAgencyById)) - .patch(protectedRoute([Roles.ADMIN]), activeUserCheck, catchErrors(updateAgencyById)) - .delete(protectedRoute([Roles.ADMIN]), activeUserCheck, catchErrors(deleteAgencyById)); + .get(catchErrors(getAgencyById)) + .patch(catchErrors(updateAgencyById)) + .delete(catchErrors(deleteAgencyById)); export default router; diff --git a/express-api/src/routes/buildingsRouter.ts b/express-api/src/routes/buildingsRouter.ts index c05ae9c64..04263614a 100644 --- a/express-api/src/routes/buildingsRouter.ts +++ b/express-api/src/routes/buildingsRouter.ts @@ -1,5 +1,4 @@ import controllers from '@/controllers'; -import activeUserCheck from '@/middleware/activeUserCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; import express from 'express'; @@ -10,12 +9,9 @@ const { getBuilding, updateBuilding, deleteBuilding, getBuildings, addBuilding } // Endpoints for buildings data manipulation router .route(`/:buildingId`) - .get(activeUserCheck, catchErrors(getBuilding)) - .put(activeUserCheck, catchErrors(updateBuilding)) - .delete(activeUserCheck, catchErrors(deleteBuilding)); -router - .route('/') - .get(activeUserCheck, catchErrors(getBuildings)) - .post(activeUserCheck, catchErrors(addBuilding)); + .get(catchErrors(getBuilding)) + .put(catchErrors(updateBuilding)) + .delete(catchErrors(deleteBuilding)); +router.route('/').get(catchErrors(getBuildings)).post(catchErrors(addBuilding)); export default router; diff --git a/express-api/src/routes/ltsaRouter.ts b/express-api/src/routes/ltsaRouter.ts index 69f05f6a3..00e5bd0cc 100644 --- a/express-api/src/routes/ltsaRouter.ts +++ b/express-api/src/routes/ltsaRouter.ts @@ -1,11 +1,10 @@ import controllers from '@/controllers'; -import activeUserCheck from '@/middleware/activeUserCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; import express from 'express'; const router = express.Router(); // Endpoints for LTSA title information -router.route('/land/title').get(activeUserCheck, catchErrors(controllers.getLTSA)); +router.route('/land/title').get(catchErrors(controllers.getLTSA)); export default router; diff --git a/express-api/src/routes/notificationsRouter.ts b/express-api/src/routes/notificationsRouter.ts index 59b07a368..20a731b44 100644 --- a/express-api/src/routes/notificationsRouter.ts +++ b/express-api/src/routes/notificationsRouter.ts @@ -1,8 +1,7 @@ import { Roles } from '@/constants/roles'; import controllers from '@/controllers'; -import activeUserCheck from '@/middleware/activeUserCheck'; +import userAuthCheck from '@/middleware/userAuthCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; -import { protectedRoute } from '@bcgov/citz-imb-sso-express'; import express from 'express'; const router = express.Router(); @@ -16,11 +15,11 @@ const { getNotificationsByProjectId, resendNotificationById, cancelNotificationB //that it might be an individual "notification id". router .route(NOTIFICATION_QUEUE_ROUTE) - .get(activeUserCheck, catchErrors(getNotificationsByProjectId)); + .get(userAuthCheck(), catchErrors(getNotificationsByProjectId)); router .route(`${NOTIFICATION_QUEUE_ROUTE}/:id`) - .put(protectedRoute([Roles.ADMIN]), activeUserCheck, catchErrors(resendNotificationById)) - .delete(protectedRoute([Roles.ADMIN]), activeUserCheck, catchErrors(cancelNotificationById)); + .put(userAuthCheck({ requiredRoles: [Roles.ADMIN] }), catchErrors(resendNotificationById)) + .delete(userAuthCheck({ requiredRoles: [Roles.ADMIN] }), catchErrors(cancelNotificationById)); export default router; diff --git a/express-api/src/routes/parcelsRouter.ts b/express-api/src/routes/parcelsRouter.ts index 2cb7f283a..555892d08 100644 --- a/express-api/src/routes/parcelsRouter.ts +++ b/express-api/src/routes/parcelsRouter.ts @@ -1,5 +1,4 @@ import controllers from '@/controllers'; -import activeUserCheck from '@/middleware/activeUserCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; import express from 'express'; @@ -10,12 +9,9 @@ const { getParcel, updateParcel, deleteParcel, getParcels, addParcel } = control // Endpoints for parcels data manipulation router .route(`/:parcelId`) - .get(activeUserCheck, catchErrors(getParcel)) - .put(activeUserCheck, catchErrors(updateParcel)) - .delete(activeUserCheck, catchErrors(deleteParcel)); -router - .route(`/`) - .get(activeUserCheck, catchErrors(getParcels)) - .post(activeUserCheck, catchErrors(addParcel)); + .get(catchErrors(getParcel)) + .put(catchErrors(updateParcel)) + .delete(catchErrors(deleteParcel)); +router.route(`/`).get(catchErrors(getParcels)).post(catchErrors(addParcel)); export default router; diff --git a/express-api/src/routes/projectsRouter.ts b/express-api/src/routes/projectsRouter.ts index 3be65ea44..e51769325 100644 --- a/express-api/src/routes/projectsRouter.ts +++ b/express-api/src/routes/projectsRouter.ts @@ -1,5 +1,6 @@ +import { Roles } from '@/constants/roles'; import controllers from '@/controllers'; -import activeUserCheck from '@/middleware/activeUserCheck'; +import userAuthCheck from '@/middleware/userAuthCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; import express from 'express'; @@ -18,13 +19,13 @@ const { //These originally had a separate route for numeric id and projectNumber, but I don't think express supports this pattern. router .route(`${PROJECT_DISPOSAL}/:projectId`) - .get(activeUserCheck, catchErrors(getDisposalProject)) - .put(activeUserCheck, catchErrors(updateDisposalProject)) - .delete(activeUserCheck, catchErrors(deleteDisposalProject)); + .get(userAuthCheck(), catchErrors(getDisposalProject)) + .put(userAuthCheck({ requiredRoles: [Roles.ADMIN] }), catchErrors(updateDisposalProject)) + .delete(userAuthCheck({ requiredRoles: [Roles.ADMIN] }), catchErrors(deleteDisposalProject)); -router.route(`${PROJECT_DISPOSAL}`).post(activeUserCheck, catchErrors(addDisposalProject)); +router.route(`${PROJECT_DISPOSAL}`).post(userAuthCheck(), catchErrors(addDisposalProject)); //Omitting search endpoints. -router.route('/').get(activeUserCheck, catchErrors(getProjects)); +router.route('/').get(userAuthCheck(), catchErrors(getProjects)); export default router; diff --git a/express-api/src/routes/propertiesRouter.ts b/express-api/src/routes/propertiesRouter.ts index 48aaf7f00..31cb538b1 100644 --- a/express-api/src/routes/propertiesRouter.ts +++ b/express-api/src/routes/propertiesRouter.ts @@ -1,10 +1,11 @@ import controllers from '@/controllers'; -import activeUserCheck from '@/middleware/activeUserCheck'; +import userAuthCheck from '@/middleware/userAuthCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; import { bulkUploadMimeTypeWhitelist } from '@/utilities/uploadWhitelist'; import express, { NextFunction } from 'express'; import multer from 'multer'; import { Request, Response } from 'express'; +import { Roles } from '@/constants/roles'; const router = express.Router(); @@ -17,11 +18,11 @@ const { getLinkedProjects, } = controllers; -router.route('/search/fuzzy').get(activeUserCheck, catchErrors(getPropertiesFuzzySearch)); +router.route('/search/fuzzy').get(userAuthCheck(), catchErrors(getPropertiesFuzzySearch)); -router.route('/search/geo').get(activeUserCheck, catchErrors(getPropertiesForMap)); // Formerly wfs route +router.route('/search/geo').get(userAuthCheck(), catchErrors(getPropertiesForMap)); // Formerly wfs route -router.route('/search/linkedProjects').get(activeUserCheck, catchErrors(getLinkedProjects)); +router.route('/search/linkedProjects').get(userAuthCheck(), catchErrors(getLinkedProjects)); const upload = multer({ dest: 'uploads/', @@ -41,8 +42,14 @@ const uploadHandler = async (req: Request, res: Response, next: NextFunction) => next(); }); }; -router.route('/import').post(activeUserCheck, uploadHandler, catchErrors(importProperties)); -router.route('/import/results').get(activeUserCheck, catchErrors(getImportResults)); -router.route('/').get(activeUserCheck, catchErrors(getPropertyUnion)); +router + .route('/import') + .post( + userAuthCheck({ requiredRoles: [Roles.ADMIN] }), + uploadHandler, + catchErrors(importProperties), + ); +router.route('/import/results').get(userAuthCheck(), catchErrors(getImportResults)); +router.route('/').get(userAuthCheck(), catchErrors(getPropertyUnion)); export default router; diff --git a/express-api/src/routes/toolsRouter.ts b/express-api/src/routes/toolsRouter.ts index 0624b4ba7..6d7b8d3f1 100644 --- a/express-api/src/routes/toolsRouter.ts +++ b/express-api/src/routes/toolsRouter.ts @@ -1,5 +1,4 @@ import controllers from '@/controllers'; -import activeUserCheck from '@/middleware/activeUserCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; import express from 'express'; @@ -7,9 +6,7 @@ const router = express.Router(); const { searchGeocoderAddresses, searchGeocoderSiteId } = controllers; -router.route(`/geocoder/addresses`).get(activeUserCheck, catchErrors(searchGeocoderAddresses)); -router - .route(`/geocoder/parcels/pids/:siteId`) - .get(activeUserCheck, catchErrors(searchGeocoderSiteId)); +router.route(`/geocoder/addresses`).get(catchErrors(searchGeocoderAddresses)); +router.route(`/geocoder/parcels/pids/:siteId`).get(catchErrors(searchGeocoderSiteId)); export default router; diff --git a/express-api/src/routes/usersRouter.ts b/express-api/src/routes/usersRouter.ts index ae188d8bc..c2abe92c1 100644 --- a/express-api/src/routes/usersRouter.ts +++ b/express-api/src/routes/usersRouter.ts @@ -1,8 +1,7 @@ import { Roles } from '@/constants/roles'; import controllers from '@/controllers'; -import activeUserCheck from '@/middleware/activeUserCheck'; +import userAuthCheck from '@/middleware/userAuthCheck'; import catchErrors from '@/utilities/controllerErrorWrapper'; -import { protectedRoute } from '@bcgov/citz-imb-sso-express'; import express from 'express'; const router = express.Router(); @@ -12,13 +11,13 @@ const { getSelf, submitUserAccessRequest, getUserAgencies, getUserById, getUsers router.route(`/self`).get(catchErrors(getSelf)); router.route(`/access/requests`).post(catchErrors(submitUserAccessRequest)); -router.route(`/agencies/:username`).get(activeUserCheck, catchErrors(getUserAgencies)); +router.route(`/agencies/:username`).get(userAuthCheck(), catchErrors(getUserAgencies)); -router.route(`/`).get(activeUserCheck, catchErrors(getUsers)); +router.route(`/`).get(userAuthCheck(), catchErrors(getUsers)); router .route(`/:id`) - .get(activeUserCheck, catchErrors(getUserById)) - .put(protectedRoute([Roles.ADMIN]), activeUserCheck, catchErrors(updateUserById)); + .get(userAuthCheck(), catchErrors(getUserById)) + .put(userAuthCheck({ requiredRoles: [Roles.ADMIN] }), catchErrors(updateUserById)); export default router; diff --git a/express-api/src/services/buildings/buildingServices.ts b/express-api/src/services/buildings/buildingServices.ts index d5e8a15b8..63cb189f4 100644 --- a/express-api/src/services/buildings/buildingServices.ts +++ b/express-api/src/services/buildings/buildingServices.ts @@ -9,7 +9,7 @@ import { BuildingFiscal } from '@/typeorm/Entities/BuildingFiscal'; import logger from '@/utilities/winstonLogger'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; import { Roles } from '@/constants/roles'; -import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { PimsRequestUser } from '@/middleware/userAuthCheck'; import { User } from '@/typeorm/Entities/User'; const buildingRepo = AppDataSource.getRepository(Building); diff --git a/express-api/src/services/ches/chesServices.ts b/express-api/src/services/ches/chesServices.ts index 8b80f2b6d..5f564dc0d 100644 --- a/express-api/src/services/ches/chesServices.ts +++ b/express-api/src/services/ches/chesServices.ts @@ -1,7 +1,7 @@ import config from '@/constants/config'; import urls from '@/constants/urls'; import { ChesFilter } from '@/controllers/tools/toolsSchema'; -import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { PimsRequestUser } from '@/middleware/userAuthCheck'; import { ErrorWithCode } from '@/utilities/customErrors/ErrorWithCode'; import { decodeJWT } from '@/utilities/decodeJWT'; import logger from '@/utilities/winstonLogger'; diff --git a/express-api/src/services/notifications/notificationServices.ts b/express-api/src/services/notifications/notificationServices.ts index 7339d1b6c..951a68dee 100644 --- a/express-api/src/services/notifications/notificationServices.ts +++ b/express-api/src/services/notifications/notificationServices.ts @@ -20,7 +20,7 @@ import logger from '@/utilities/winstonLogger'; import getConfig from '@/constants/config'; import { getDaysBetween } from '@/utilities/helperFunctions'; import { ProjectStatusHistory } from '@/typeorm/Entities/ProjectStatusHistory'; -import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { PimsRequestUser } from '@/middleware/userAuthCheck'; export interface AccessRequestData { FirstName: string; diff --git a/express-api/src/services/parcels/parcelServices.ts b/express-api/src/services/parcels/parcelServices.ts index a02ead314..2580e748c 100644 --- a/express-api/src/services/parcels/parcelServices.ts +++ b/express-api/src/services/parcels/parcelServices.ts @@ -9,7 +9,7 @@ import userServices from '../users/usersServices'; import logger from '@/utilities/winstonLogger'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; import { Roles } from '@/constants/roles'; -import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { PimsRequestUser } from '@/middleware/userAuthCheck'; import { User } from '@/typeorm/Entities/User'; const parcelRepo = AppDataSource.getRepository(Parcel); diff --git a/express-api/src/services/projects/projectsServices.ts b/express-api/src/services/projects/projectsServices.ts index d5d0b1d56..d68e14393 100644 --- a/express-api/src/services/projects/projectsServices.ts +++ b/express-api/src/services/projects/projectsServices.ts @@ -36,7 +36,7 @@ import { NotificationQueue } from '@/typeorm/Entities/NotificationQueue'; import { SortOrders } from '@/constants/types'; import { ProjectJoin } from '@/typeorm/Entities/views/ProjectJoinView'; import { Roles } from '@/constants/roles'; -import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { PimsRequestUser } from '@/middleware/userAuthCheck'; import { User } from '@/typeorm/Entities/User'; const projectRepo = AppDataSource.getRepository(Project); diff --git a/express-api/src/services/properties/propertiesServices.ts b/express-api/src/services/properties/propertiesServices.ts index 548959469..cf037b0ef 100644 --- a/express-api/src/services/properties/propertiesServices.ts +++ b/express-api/src/services/properties/propertiesServices.ts @@ -37,7 +37,7 @@ import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; import { ProjectStatus as ProjectStatusEntity } from '@/typeorm/Entities/ProjectStatus'; import { parentPort } from 'worker_threads'; import { ErrorWithCode } from '@/utilities/customErrors/ErrorWithCode'; -import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { PimsRequestUser } from '@/middleware/userAuthCheck'; /** * Perform a fuzzy search for properties based on the provided keyword. diff --git a/express-api/src/utilities/authorizationChecks.ts b/express-api/src/utilities/authorizationChecks.ts index 222eb4ab8..44879f4f0 100644 --- a/express-api/src/utilities/authorizationChecks.ts +++ b/express-api/src/utilities/authorizationChecks.ts @@ -1,7 +1,7 @@ import { SSOUser } from '@bcgov/citz-imb-sso-express'; import { Roles } from '@/constants/roles'; import userServices, { getUser } from '@/services/users/usersServices'; -import { PimsRequestUser } from '@/middleware/activeUserCheck'; +import { PimsRequestUser } from '@/middleware/userAuthCheck'; /** * Function to check if user can edit. From c89abe5f43e75c3f885487d836c90900bfb1c6a2 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Tue, 17 Sep 2024 10:44:46 -0700 Subject: [PATCH 07/15] commenting --- express-api/src/middleware/userAuthCheck.ts | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/express-api/src/middleware/userAuthCheck.ts b/express-api/src/middleware/userAuthCheck.ts index 6b151112c..f21739202 100644 --- a/express-api/src/middleware/userAuthCheck.ts +++ b/express-api/src/middleware/userAuthCheck.ts @@ -6,17 +6,24 @@ import { NextFunction, RequestHandler, Response } from 'express'; export interface UserCheckOptions { requiredRoles?: Roles[]; - onlyAuthenticated?: boolean; } /** - * Middleware that checks for a user with Active status. - * If the user lacks that status, isn't found, - * or is missing a token, a rejected response is sent. - * Successful checks result in the request passed on. - * Also checks that user has a role parsed from their token. + * Middleware function to check user authentication and authorization. + * Must be preceeded by protectedRoute (from SSO packages) at some point. + * + * @param options.requiredRoles - A list of Roles needed to pass this check. + * @returns Express RequestHandler function to perform user authentication checks. + * @example app.use(`routeName`, protectedRoute(), userAuthCheck({ requiredRoles: [Roles.ADMIN] }), router.myRouter); */ const userAuthCheck = (options?: UserCheckOptions): RequestHandler => { + /** + * Middleware function to check user authentication and authorization. + * + * @param req - Express request object with user and pimsUser properties. + * @param res - Express response object. + * @param next - Express next function. + */ const check = async ( req: Request & { user?: SSOUser; pimsUser?: PimsRequestUser }, res: Response, @@ -36,6 +43,8 @@ const userAuthCheck = (options?: UserCheckOptions): RequestHandler => { if (!user) { return res.status(404).send('Requesting user not found.'); } + + // Returns a boolean indicating if user has a required role const hasOneOfRoles = (roles: Roles[]): boolean => { // No roles, then no permission. if (!roles || !roles.length) { From 839ea6b7cc3a096f5a0863e1cece168b9d868b2c Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Tue, 17 Sep 2024 12:11:49 -0700 Subject: [PATCH 08/15] update userAuthCheck test --- express-api/tests/testUtils/factories.ts | 7 +- .../unit/middleware/activeUserCheck.test.ts | 58 ---------- .../unit/middleware/userAuthCheck.test.ts | 102 ++++++++++++++++++ 3 files changed, 104 insertions(+), 63 deletions(-) delete mode 100644 express-api/tests/unit/middleware/activeUserCheck.test.ts create mode 100644 express-api/tests/unit/middleware/userAuthCheck.test.ts diff --git a/express-api/tests/testUtils/factories.ts b/express-api/tests/testUtils/factories.ts index 2c7e4d1ff..c4fffee69 100644 --- a/express-api/tests/testUtils/factories.ts +++ b/express-api/tests/testUtils/factories.ts @@ -83,7 +83,7 @@ export class MockReq { headers = {}; files: any[] = []; - public setUser = (userData: object) => { + public setUser = (userData?: object) => { const defaultUserObject = { guid: 'W7802F34D2390EFA9E7JK15923770279', identity_provider: 'idir', @@ -96,12 +96,9 @@ export class MockReq { email: 'john.doe@gov.bc.ca', client_roles: [] as string[], hasRoles: () => true, - //originalData: - }; - this.user = { - ...defaultUserObject, ...userData, }; + this.user = defaultUserObject; }; } diff --git a/express-api/tests/unit/middleware/activeUserCheck.test.ts b/express-api/tests/unit/middleware/activeUserCheck.test.ts deleted file mode 100644 index 986b3358b..000000000 --- a/express-api/tests/unit/middleware/activeUserCheck.test.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { Request, Response } from 'express'; -import { MockReq, MockRes, getRequestHandlerMocks, produceUser } from 'tests/testUtils/factories'; -import activeUserCheck from '@/middleware/activeUserCheck'; -import { AppDataSource } from '@/appDataSource'; -import { User, UserStatus } from '@/typeorm/Entities/User'; -import { Roles } from '@/constants/roles'; - -let mockRequest: Request & MockReq, mockResponse: Response & MockRes; -const _findUserMock = jest - .spyOn(AppDataSource.getRepository(User), 'findOne') - .mockImplementation(async () => produceUser()); - -describe('UNIT - activeUserCheck middleware', () => { - beforeEach(() => { - jest.clearAllMocks(); - - const { mockReq, mockRes } = getRequestHandlerMocks(); - mockRequest = mockReq; - mockResponse = mockRes; - mockRequest.setUser({ client_roles: [Roles.ADMIN] }); - }); - const nextFunction = jest.fn(); - - it('should give return a 401 response if the Keycloak User is missing', async () => { - mockRequest.user = undefined; - await activeUserCheck(mockRequest, mockResponse, nextFunction); - expect(mockResponse.status).toHaveBeenCalledWith(401); - expect(mockResponse.send).toHaveBeenCalledWith('Unauthorized request.'); - expect(_findUserMock).toHaveBeenCalledTimes(0); - }); - - it('should give return a 404 response if the user is not found', async () => { - _findUserMock.mockImplementationOnce(async () => null); - await activeUserCheck(mockRequest, mockResponse, nextFunction); - expect(mockResponse.status).toHaveBeenCalledWith(404); - expect(mockResponse.send).toHaveBeenCalledWith('Requesting user not found.'); - expect(_findUserMock).toHaveBeenCalledTimes(1); - }); - - it('should give return a 403 response if the user does not have Active status', async () => { - _findUserMock.mockImplementationOnce(async () => - produceUser({ - Status: UserStatus.OnHold, - }), - ); - await activeUserCheck(mockRequest, mockResponse, nextFunction); - expect(mockResponse.status).toHaveBeenCalledWith(403); - expect(mockResponse.send).toHaveBeenCalledWith('Request forbidden. User lacks Active status.'); - expect(_findUserMock).toHaveBeenCalledTimes(1); - }); - - it('should give return a 403 response if the Keycloak User does not have a role', async () => { - mockRequest.setUser({ client_roles: undefined, hasRoles: () => false }); - await activeUserCheck(mockRequest, mockResponse, nextFunction); - expect(mockResponse.status).toHaveBeenCalledWith(403); - expect(mockResponse.send).toHaveBeenCalledWith('Request forbidden. User has no assigned role.'); - }); -}); diff --git a/express-api/tests/unit/middleware/userAuthCheck.test.ts b/express-api/tests/unit/middleware/userAuthCheck.test.ts new file mode 100644 index 000000000..fff2d93d2 --- /dev/null +++ b/express-api/tests/unit/middleware/userAuthCheck.test.ts @@ -0,0 +1,102 @@ +import { Request, Response } from 'express'; +import { MockReq, MockRes, getRequestHandlerMocks, produceUser } from 'tests/testUtils/factories'; +import userAuthCheck from '@/middleware/userAuthCheck'; +import { AppDataSource } from '@/appDataSource'; +import { User, UserStatus } from '@/typeorm/Entities/User'; +import { Roles } from '@/constants/roles'; + +let mockRequest: Request & MockReq, mockResponse: Response & MockRes; +const _findUserMock = jest + .spyOn(AppDataSource.getRepository(User), 'findOne') + .mockImplementation(async () => produceUser()); + +describe('UNIT - userAuthCheck middleware', () => { + beforeEach(() => { + jest.clearAllMocks(); + + const { mockReq, mockRes } = getRequestHandlerMocks(); + mockRequest = mockReq; + mockResponse = mockRes; + mockRequest.setUser(); + }); + const nextFunction = jest.fn(); + const testFunction = userAuthCheck(); + + it('should give return a 401 response if the Keycloak User is missing', async () => { + mockRequest.user = undefined; + await testFunction(mockRequest, mockResponse, nextFunction); + expect(mockResponse.status).toHaveBeenCalledWith(401); + expect(mockResponse.send).toHaveBeenCalledWith('Requestor not authenticated by Keycloak.'); + expect(_findUserMock).toHaveBeenCalledTimes(0); + }); + + it('should give return a 404 response if the user is not found', async () => { + _findUserMock.mockImplementationOnce(async () => null); + await testFunction(mockRequest, mockResponse, nextFunction); + expect(mockResponse.status).toHaveBeenCalledWith(404); + expect(mockResponse.send).toHaveBeenCalledWith('Requesting user not found.'); + expect(_findUserMock).toHaveBeenCalledTimes(1); + }); + + it('should give return a 403 response if the user does not have Active status', async () => { + _findUserMock.mockImplementationOnce(async () => + produceUser({ + Status: UserStatus.OnHold, + RoleId: Roles.ADMIN, + }), + ); + await testFunction(mockRequest, mockResponse, nextFunction); + expect(mockResponse.status).toHaveBeenCalledWith(403); + expect(mockResponse.send).toHaveBeenCalledWith('Request forbidden. User lacks Active status.'); + expect(_findUserMock).toHaveBeenCalledTimes(1); + }); + + it('should give return a 403 response if the user does not have a role', async () => { + _findUserMock.mockImplementationOnce(async () => + produceUser({ + Status: UserStatus.OnHold, + }), + ); + await testFunction(mockRequest, mockResponse, nextFunction); + expect(mockResponse.status).toHaveBeenCalledWith(403); + expect(mockResponse.send).toHaveBeenCalledWith('Request forbidden. User has no assigned role.'); + expect(_findUserMock).toHaveBeenCalledTimes(1); + }); + + it('should give return a 403 response if the required roles are not found', async () => { + const testFunction = userAuthCheck({ requiredRoles: [Roles.AUDITOR] }); + _findUserMock.mockImplementationOnce(async () => + produceUser({ + Status: UserStatus.Active, + RoleId: Roles.ADMIN, + }), + ); + await testFunction(mockRequest, mockResponse, nextFunction); + expect(mockResponse.status).toHaveBeenCalledWith(403); + expect(mockResponse.send).toHaveBeenCalledWith('Request forbidden. User lacks required roles.'); + }); + + it('should give return a 403 response if the required roles are blank', async () => { + const testFunction = userAuthCheck({ requiredRoles: [] }); + _findUserMock.mockImplementationOnce(async () => + produceUser({ + Status: UserStatus.Active, + RoleId: Roles.ADMIN, + }), + ); + await testFunction(mockRequest, mockResponse, nextFunction); + expect(mockResponse.status).toHaveBeenCalledWith(403); + expect(mockResponse.send).toHaveBeenCalledWith('Request forbidden. User lacks required roles.'); + }); + + it('should add the PIMS user to the request when all checks have passed', async () => { + _findUserMock.mockImplementationOnce(async () => + produceUser({ + Status: UserStatus.Active, + RoleId: Roles.ADMIN, + }), + ); + await testFunction(mockRequest, mockResponse, nextFunction); + expect(mockRequest.pimsUser).toBeDefined(); + }); +}); From 5c85ec41b46333bb90b49bf5e3fbf033460cbd99 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Tue, 17 Sep 2024 14:31:56 -0700 Subject: [PATCH 09/15] controller test updates --- .../src/utilities/authorizationChecks.ts | 2 +- express-api/tests/testUtils/factories.ts | 19 +++++++++++++ .../administrativeAreasController.test.ts | 1 + .../agencies/agenciesController.test.ts | 1 + .../buildings/buildingsController.test.ts | 4 +++ .../controllers/ltsa/ltsaController.test.ts | 3 ++- .../notificationsController.test.ts | 14 +++++++--- .../parcels/parcelsController.test.ts | 6 ++++- .../projects/projectsController.test.ts | 14 ++++++++++ .../properties/propertiesController.test.ts | 10 +++++++ .../controllers/users/usersController.test.ts | 3 +++ .../utilities/authorizationChecks.test.ts | 27 +++++-------------- 12 files changed, 78 insertions(+), 26 deletions(-) diff --git a/express-api/src/utilities/authorizationChecks.ts b/express-api/src/utilities/authorizationChecks.ts index 44879f4f0..45e006d0b 100644 --- a/express-api/src/utilities/authorizationChecks.ts +++ b/express-api/src/utilities/authorizationChecks.ts @@ -9,7 +9,7 @@ import { PimsRequestUser } from '@/middleware/userAuthCheck'; * @param user - The user object containing information about the user. * @returns A boolean value indicating whether the user can edit or not. */ -export const canUserEdit = async (user: PimsRequestUser): Promise => { +export const canUserEdit = (user: PimsRequestUser): boolean => { // as they are not an auditor the user can edit return user.hasOneOfRoles([Roles.GENERAL_USER, Roles.ADMIN]); }; diff --git a/express-api/tests/testUtils/factories.ts b/express-api/tests/testUtils/factories.ts index c4fffee69..1c91aefc3 100644 --- a/express-api/tests/testUtils/factories.ts +++ b/express-api/tests/testUtils/factories.ts @@ -51,6 +51,7 @@ import { PropertyUnion } from '@/typeorm/Entities/views/PropertyUnionView'; import { ImportResult } from '@/typeorm/Entities/ImportResult'; import { ProjectJoin } from '@/typeorm/Entities/views/ProjectJoinView'; import { ImportRow } from '@/services/properties/propertiesServices'; +import { PimsRequestUser } from '@/middleware/userAuthCheck'; export class MockRes { statusValue: any; @@ -82,6 +83,7 @@ export class MockReq { user = {}; headers = {}; files: any[] = []; + pimsUser = {}; public setUser = (userData?: object) => { const defaultUserObject = { @@ -100,6 +102,14 @@ export class MockReq { }; this.user = defaultUserObject; }; + + public setPimsUser = (userData?: Partial) => { + const defaultObject = producePimsRequestUser(); + this.pimsUser = { + ...defaultObject, + ...userData, + }; + }; } /** @@ -211,6 +221,15 @@ export const produceSSO = (props?: Partial): SSOUser => { }; }; +export const producePimsRequestUser = (props?: Partial): PimsRequestUser => { + const user = produceUser(); + return { + ...user, + hasOneOfRoles: () => true, + ...props, + }; +}; + export const produceParcel = (props?: Partial): Parcel => { const id = faker.number.int({ max: 10 }); return { diff --git a/express-api/tests/unit/controllers/administrativeAreas/administrativeAreasController.test.ts b/express-api/tests/unit/controllers/administrativeAreas/administrativeAreasController.test.ts index be239750a..0036156f0 100644 --- a/express-api/tests/unit/controllers/administrativeAreas/administrativeAreasController.test.ts +++ b/express-api/tests/unit/controllers/administrativeAreas/administrativeAreasController.test.ts @@ -60,6 +60,7 @@ describe('UNIT - Administrative Areas Admin', () => { describe('Controller getAdministrativeAreas', () => { // TODO: enable other tests when controller is complete it('should return status 200 and a list of administrative areas', async () => { + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getAdministrativeAreas(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); }); diff --git a/express-api/tests/unit/controllers/agencies/agenciesController.test.ts b/express-api/tests/unit/controllers/agencies/agenciesController.test.ts index be5256416..9d2ef95bd 100644 --- a/express-api/tests/unit/controllers/agencies/agenciesController.test.ts +++ b/express-api/tests/unit/controllers/agencies/agenciesController.test.ts @@ -44,6 +44,7 @@ describe('UNIT - Agencies Admin', () => { const { mockReq, mockRes } = getRequestHandlerMocks(); mockRequest = mockReq; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockResponse = mockRes; }); diff --git a/express-api/tests/unit/controllers/buildings/buildingsController.test.ts b/express-api/tests/unit/controllers/buildings/buildingsController.test.ts index 16b02a954..a41677df7 100644 --- a/express-api/tests/unit/controllers/buildings/buildingsController.test.ts +++ b/express-api/tests/unit/controllers/buildings/buildingsController.test.ts @@ -57,6 +57,7 @@ describe('UNIT - Buildings', () => { mockRequest.params.buildingId = '1'; _hasAgencies.mockImplementationOnce(() => true); mockRequest.setUser({ hasRoles: () => true }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); _getBuildingById.mockImplementationOnce(() => buildingWithAgencyId1); await controllers.getBuilding(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); @@ -78,6 +79,7 @@ describe('UNIT - Buildings', () => { it('should return 403 when user does not have correct agencies', async () => { mockRequest.params.buildingId = '1'; mockRequest.setUser({ client_roles: [Roles.GENERAL_USER], hasRoles: () => false }); + mockRequest.setPimsUser({ RoleId: Roles.GENERAL_USER, hasOneOfRoles: () => false }); _hasAgencies.mockImplementationOnce(() => false); await controllers.getBuilding(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(403); @@ -115,6 +117,7 @@ describe('UNIT - Buildings', () => { describe('GET /properties/buildings', () => { it('should return 200 and a list of buildings', async () => { + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await controllers.getBuildings(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); }); @@ -126,6 +129,7 @@ describe('UNIT - Buildings', () => { it('should return 200 with an admin user', async () => { // Mock an admin user mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await controllers.getBuildings(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue)).toBeTruthy(); diff --git a/express-api/tests/unit/controllers/ltsa/ltsaController.test.ts b/express-api/tests/unit/controllers/ltsa/ltsaController.test.ts index 3596ef8a8..f68609c42 100644 --- a/express-api/tests/unit/controllers/ltsa/ltsaController.test.ts +++ b/express-api/tests/unit/controllers/ltsa/ltsaController.test.ts @@ -2,7 +2,7 @@ import { Request, Response } from 'express'; import controllers from '@/controllers'; import { ErrorWithCode } from '@/utilities/customErrors/ErrorWithCode'; import ltsaService from '@/services/ltsa/ltsaServices'; -import { produceLtsaOrder } from 'tests/testUtils/factories'; +import { produceLtsaOrder, producePimsRequestUser } from 'tests/testUtils/factories'; describe('UNIT - Testing controllers for /ltsa routes', () => { const mockRequest = { @@ -18,6 +18,7 @@ describe('UNIT - Testing controllers for /ltsa routes', () => { email: 'john.doe@gov.bc.ca', client_roles: ['Admin'], }, + pimsUser: producePimsRequestUser(), } as unknown as Request; const mockResponse = { send: jest.fn().mockReturnThis(), diff --git a/express-api/tests/unit/controllers/notifications/notificationsController.test.ts b/express-api/tests/unit/controllers/notifications/notificationsController.test.ts index 9e5ca91bf..b59ff56c3 100644 --- a/express-api/tests/unit/controllers/notifications/notificationsController.test.ts +++ b/express-api/tests/unit/controllers/notifications/notificationsController.test.ts @@ -5,9 +5,9 @@ import { MockRes, getRequestHandlerMocks, produceUser, - produceSSO, produceProject, produceNotificationQueue, + producePimsRequestUser, } from 'tests/testUtils/factories'; import projectServices from '@/services/projects/projectsServices'; import { randomUUID } from 'crypto'; @@ -85,8 +85,8 @@ describe('UNIT - Testing controllers for notifications routes.', () => { }); it('should return 403 if user is not authorized', async () => { - const kcUser = produceSSO(); - mockRequest.user = kcUser; + mockRequest.setUser(); + mockRequest.setPimsUser({ RoleId: undefined, hasOneOfRoles: () => false }); mockRequest.query = { projectId: '1' }; await controllers.getNotificationsByProjectId(mockRequest, mockResponse); @@ -98,6 +98,7 @@ describe('UNIT - Testing controllers for notifications routes.', () => { const mockRequest = { query: { projectId: '123' }, user: { agencies: [1] }, + pimsUser: producePimsRequestUser({ RoleId: Roles.ADMIN, hasOneOfRoles: () => false }), } as unknown as Request; const mockProject = produceProject({ @@ -124,6 +125,7 @@ describe('UNIT - Testing controllers for notifications routes.', () => { it('should read a notification and try to send it through ches again, status 200', async () => { mockRequest.params.id = '1'; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await controllers.resendNotificationById(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(mockResponse.sendValue.Id).toBe(1); @@ -131,6 +133,7 @@ describe('UNIT - Testing controllers for notifications routes.', () => { it('should 404 if notif not found', async () => { mockRequest.params.id = '1'; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); _getNotifById.mockImplementationOnce(() => null); await controllers.resendNotificationById(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(404); @@ -138,6 +141,7 @@ describe('UNIT - Testing controllers for notifications routes.', () => { it('should 403 if user lacks permissions', async () => { mockRequest.params.id = '1'; mockRequest.setUser({ client_roles: [], hasRoles: () => false }); + mockRequest.setPimsUser({ RoleId: undefined, hasOneOfRoles: () => false }); await controllers.resendNotificationById(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(403); }); @@ -146,6 +150,7 @@ describe('UNIT - Testing controllers for notifications routes.', () => { it('should try to cancel a notification, status 200', async () => { mockRequest.params.id = '1'; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await controllers.cancelNotificationById(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(mockResponse.sendValue.Id).toBe(1); @@ -154,6 +159,7 @@ describe('UNIT - Testing controllers for notifications routes.', () => { it('should 404 if no notif found', async () => { mockRequest.params.id = '1'; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); _getNotifById.mockImplementationOnce(() => null); await controllers.cancelNotificationById(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(404); @@ -161,6 +167,7 @@ describe('UNIT - Testing controllers for notifications routes.', () => { it('should 400 if the notification came back with non-cancelled status', async () => { mockRequest.params.id = '1'; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); _cancelNotifById.mockImplementationOnce((id: number) => produceNotificationQueue({ Id: id, Status: NotificationStatus.Completed }), ); @@ -172,6 +179,7 @@ describe('UNIT - Testing controllers for notifications routes.', () => { it('should 403 if user lacks permissions', async () => { mockRequest.params.id = '1'; mockRequest.setUser({ client_roles: [], hasRoles: () => false }); + mockRequest.setPimsUser({ RoleId: Roles.GENERAL_USER, hasOneOfRoles: () => false }); await controllers.cancelNotificationById(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(403); }); diff --git a/express-api/tests/unit/controllers/parcels/parcelsController.test.ts b/express-api/tests/unit/controllers/parcels/parcelsController.test.ts index 3458a1250..f8c533daf 100644 --- a/express-api/tests/unit/controllers/parcels/parcelsController.test.ts +++ b/express-api/tests/unit/controllers/parcels/parcelsController.test.ts @@ -51,8 +51,8 @@ describe('UNIT - Parcels', () => { describe('GET /properties/parcels/:parcelId', () => { it('should return 200 with a correct response body', async () => { mockRequest.params.parcelId = '1'; - // _hasAgencies.mockImplementationOnce(() => true); mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await controllers.getParcel(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); }); @@ -80,6 +80,7 @@ describe('UNIT - Parcels', () => { it('should return with status 403 when user doenst have permission to view parcel', async () => { mockRequest.params.parcelId = '1'; mockRequest.setUser({ client_roles: [Roles.GENERAL_USER], hasRoles: () => false }); + mockRequest.setPimsUser({ RoleId: Roles.GENERAL_USER, hasOneOfRoles: () => false }); _hasAgencies.mockImplementationOnce(() => false); await controllers.getParcel(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(403); @@ -161,13 +162,16 @@ describe('UNIT - Parcels', () => { const { mockReq, mockRes } = getRequestHandlerMocks(); mockRequest = mockReq; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockResponse = mockRes; + _getParcels.mockImplementationOnce(() => [produceParcel()]); await controllers.getParcels(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue)).toBeTruthy(); }); it('should return 200 with a correct response body', async () => { mockRequest.query.pid = '1'; + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await controllers.getParcels(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue)).toBeTruthy(); diff --git a/express-api/tests/unit/controllers/projects/projectsController.test.ts b/express-api/tests/unit/controllers/projects/projectsController.test.ts index 04b2d19cf..0f69b70b3 100644 --- a/express-api/tests/unit/controllers/projects/projectsController.test.ts +++ b/express-api/tests/unit/controllers/projects/projectsController.test.ts @@ -93,6 +93,7 @@ describe('UNIT - Testing controllers for users routes.', () => { const { mockReq, mockRes } = getRequestHandlerMocks(); mockRequest = mockReq; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockResponse = mockRes; jest.spyOn(ProjectFilterSchema, 'safeParse').mockReturnValueOnce({ success: true, @@ -124,6 +125,7 @@ describe('UNIT - Testing controllers for users routes.', () => { mockRequest = mockReq; mockRequest.query.excelExport = 'true'; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockResponse = mockRes; jest.spyOn(ProjectFilterSchema, 'safeParse').mockReturnValueOnce({ success: true, @@ -156,6 +158,7 @@ describe('UNIT - Testing controllers for users routes.', () => { const { mockReq, mockRes } = getRequestHandlerMocks(); mockRequest = mockReq; mockRequest.setUser({ client_roles: [Roles.GENERAL_USER] }); + mockRequest.setPimsUser({ RoleId: Roles.GENERAL_USER }); mockResponse = mockRes; jest.spyOn(ProjectFilterSchema, 'safeParse').mockReturnValueOnce({ success: true, @@ -222,18 +225,21 @@ describe('UNIT - Testing controllers for users routes.', () => { it('should return status 200 and a project when user is admin', async () => { mockRequest.params.projectId = '1'; mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await controllers.getDisposalProject(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); }); it('should return status 200 and a project when user is auditor', async () => { mockRequest.params.projectId = '1'; mockRequest.setUser({ client_roles: [Roles.AUDITOR] }); + mockRequest.setPimsUser({ RoleId: Roles.AUDITOR }); await controllers.getDisposalProject(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); }); it('should return status 403 when user does not have correct agencies', async () => { mockRequest.params.projectId = '1'; mockRequest.setUser({ client_roles: [Roles.GENERAL_USER], hasRoles: () => false }); + mockRequest.setPimsUser({ RoleId: Roles.GENERAL_USER, hasOneOfRoles: () => false }); _hasAgencies.mockImplementationOnce(() => false); await controllers.getDisposalProject(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(403); @@ -254,6 +260,7 @@ describe('UNIT - Testing controllers for users routes.', () => { describe('PUT /projects/disposal/:projectId', () => { it('should return status 200 on successful update', async () => { mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockRequest.params.projectId = '1'; mockRequest.body = { project: produceProject({ Id: 1 }), @@ -265,6 +272,7 @@ describe('UNIT - Testing controllers for users routes.', () => { it('should return status 403 when user is not an admin', async () => { mockRequest.setUser({ client_roles: [Roles.GENERAL_USER] }); + mockRequest.setPimsUser({ RoleId: Roles.GENERAL_USER, hasOneOfRoles: () => false }); mockRequest.params.projectId = '1'; mockRequest.body = { project: produceProject({ Id: 1 }), @@ -276,6 +284,7 @@ describe('UNIT - Testing controllers for users routes.', () => { it('should return status 400 on mistmatched id', async () => { mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockRequest.params.projectId = '1'; mockRequest.body = { project: { @@ -288,6 +297,7 @@ describe('UNIT - Testing controllers for users routes.', () => { }); it('should return status 400 on invalid id', async () => { mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockRequest.params.projectId = 'abc'; mockRequest.body = { project: { @@ -300,6 +310,7 @@ describe('UNIT - Testing controllers for users routes.', () => { }); it('should return status 400 on missing fields', async () => { mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockRequest.params.projectId = '1'; mockRequest.body = { Id: 1, @@ -313,6 +324,7 @@ describe('UNIT - Testing controllers for users routes.', () => { it('should return status 200 on successful deletion', async () => { mockRequest.params.projectId = '1'; mockRequest.setUser({ client_roles: [Roles.ADMIN], hasRoles: () => true }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await controllers.deleteDisposalProject(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); }); @@ -320,6 +332,7 @@ describe('UNIT - Testing controllers for users routes.', () => { it('should return status 404 on no resource', async () => { mockRequest.params.projectId = 'abc'; mockRequest.setUser({ client_roles: [Roles.ADMIN], hasRoles: () => true }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await controllers.deleteDisposalProject(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(400); }); @@ -328,6 +341,7 @@ describe('UNIT - Testing controllers for users routes.', () => { describe('POST /projects/disposal', () => { it('should return status 201 on successful project addition', async () => { mockRequest.body = produceProject(); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN, hasOneOfRoles: () => false }); await controllers.addDisposalProject(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(201); }); diff --git a/express-api/tests/unit/controllers/properties/propertiesController.test.ts b/express-api/tests/unit/controllers/properties/propertiesController.test.ts index 96f08e5a5..b1994a1bf 100644 --- a/express-api/tests/unit/controllers/properties/propertiesController.test.ts +++ b/express-api/tests/unit/controllers/properties/propertiesController.test.ts @@ -111,6 +111,7 @@ describe('UNIT - Properties', () => { it('should return 200 with a list of properties', async () => { mockRequest.query.keyword = '123'; mockRequest.query.take = '3'; + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getPropertiesFuzzySearch(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue.Parcels)).toBe(true); @@ -118,6 +119,7 @@ describe('UNIT - Properties', () => { }); it('should return 200 with a list of properties', async () => { mockRequest.query.keyword = '123'; + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getPropertiesFuzzySearch(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue.Parcels)).toBe(true); @@ -136,6 +138,7 @@ describe('UNIT - Properties', () => { it('should return 200 with a list of properties', async () => { mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getPropertiesForMap(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(mockResponse.sendValue.length).toBeGreaterThanOrEqual(1); @@ -144,6 +147,7 @@ describe('UNIT - Properties', () => { it('should return 200 with a list of properties when filtered by the array fields', async () => { mockRequest.setUser({ client_roles: [Roles.AUDITOR] }); + mockRequest.setPimsUser({ RoleId: Roles.AUDITOR }); mockRequest.query = { AgencyIds: '12,13', ClassificationIds: '1,2', @@ -158,6 +162,7 @@ describe('UNIT - Properties', () => { it('should return 400 if the query params do not pass the schema', async () => { mockRequest.setUser({ client_roles: [Roles.ADMIN] }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockRequest.query = { AgencyIds: ['h'], }; @@ -175,6 +180,7 @@ describe('UNIT - Properties', () => { jest .spyOn(AppDataSource.getRepository(Agency), 'find') .mockImplementation(async () => [produceAgency()]); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getPropertiesForMap(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(mockResponse.sendValue.length).toBeGreaterThanOrEqual(1); @@ -183,6 +189,7 @@ describe('UNIT - Properties', () => { describe('GET /properties/', () => { it('should return status 200', async () => { + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getPropertyUnion(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue)).toBe(true); @@ -202,6 +209,7 @@ describe('UNIT - Properties', () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any mockRequest.file = { path: '/a/b', filename: 'aaa' } as any; mockRequest.user = produceSSO(); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await importProperties(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); }); @@ -211,12 +219,14 @@ describe('UNIT - Properties', () => { it('should return status 200', async () => { mockRequest.query = { quantity: '1' }; mockRequest.user = produceSSO(); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getImportResults(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); }); it('should return status 400', async () => { mockRequest.query = { quantity: [{}] }; mockRequest.user = produceSSO(); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getImportResults(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(400); }); diff --git a/express-api/tests/unit/controllers/users/usersController.test.ts b/express-api/tests/unit/controllers/users/usersController.test.ts index 9e6b44a82..56af5e76f 100644 --- a/express-api/tests/unit/controllers/users/usersController.test.ts +++ b/express-api/tests/unit/controllers/users/usersController.test.ts @@ -153,6 +153,7 @@ describe('UNIT - Testing controllers for users routes.', () => { describe('Controller getUsers', () => { it('should return status 200 and a list of users', async () => { + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getUsers(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue)).toBe(true); @@ -162,6 +163,7 @@ describe('UNIT - Testing controllers for users routes.', () => { mockRequest.query = { position: 'Tester', }; + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getUsers(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue)).toBe(true); @@ -186,6 +188,7 @@ describe('UNIT - Testing controllers for users routes.', () => { it('should return status 200 and the user info', async () => { mockRequest.params.id = user.Id; + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getUserById(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(mockResponse.sendValue.Id).toBe(user.Id); diff --git a/express-api/tests/unit/utilities/authorizationChecks.test.ts b/express-api/tests/unit/utilities/authorizationChecks.test.ts index 73b324f0a..d93c250be 100644 --- a/express-api/tests/unit/utilities/authorizationChecks.test.ts +++ b/express-api/tests/unit/utilities/authorizationChecks.test.ts @@ -1,12 +1,7 @@ -import { - isAdmin, - isAuditor, - canUserEdit, - isUserDisabled, - isUserActive, -} from '@/utilities/authorizationChecks'; +import { canUserEdit, isUserDisabled, isUserActive } from '@/utilities/authorizationChecks'; import { getUser as getUserService } from '@/services/users/usersServices'; -import { produceSSO } from '@/../tests/testUtils/factories'; +import { producePimsRequestUser, produceSSO } from '@/../tests/testUtils/factories'; +import { Roles } from '@/constants/roles'; // Mock the getUser function jest.mock('@/services/users/usersServices', () => ({ @@ -14,21 +9,13 @@ jest.mock('@/services/users/usersServices', () => ({ })); describe('Authorization Checks', () => { - const mockUser = produceSSO(); - mockUser.client_roles.push('Administrator'); + const mockUser = producePimsRequestUser({ RoleId: Roles.ADMIN, hasOneOfRoles: () => true }); + const mockKeycloakUser = produceSSO(); beforeEach(() => { jest.clearAllMocks(); }); - it('isAdmin should return true if user has ADMIN role', () => { - expect(isAdmin(mockUser)).toBe(true); - }); - - it('isAuditor should return false if user does not have AUDITOR role', () => { - expect(isAuditor(mockUser)).toBe(false); - }); - it('canUserEdit should return true if user has GENERAL_USER or ADMIN role', () => { expect(canUserEdit(mockUser)).toBe(true); }); @@ -40,7 +27,7 @@ describe('Authorization Checks', () => { // Mock the getUser function to return the mockDisabledUser (getUserService as jest.Mock).mockResolvedValue(mockDisabledUser); - const result = await isUserDisabled(mockUser); + const result = await isUserDisabled(mockKeycloakUser); expect(result).toBe(true); }); @@ -51,7 +38,7 @@ describe('Authorization Checks', () => { // Mock the getUser function to return the mockDisabledUser (getUserService as jest.Mock).mockResolvedValue(mockActiveUser); - const result = await isUserActive(mockUser); + const result = await isUserActive(mockKeycloakUser); expect(result).toBe(true); }); }); From b070b2a7b4c9be251daead8e2be2444740744497 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Tue, 17 Sep 2024 14:50:09 -0700 Subject: [PATCH 10/15] fix services tests --- .../parcels/parcelsController.test.ts | 2 +- .../buildings/buildingService.test.ts | 15 ++++-- .../unit/services/ches/chesServices.test.ts | 8 ++-- .../notificationServices.test.ts | 12 +++-- .../services/parcels/parcelsService.test.ts | 12 ++--- .../projects/projectsServices.test.ts | 48 +++++++++---------- .../properties/propertyServices.test.ts | 4 +- 7 files changed, 53 insertions(+), 48 deletions(-) diff --git a/express-api/tests/unit/controllers/parcels/parcelsController.test.ts b/express-api/tests/unit/controllers/parcels/parcelsController.test.ts index f8c533daf..4669972e7 100644 --- a/express-api/tests/unit/controllers/parcels/parcelsController.test.ts +++ b/express-api/tests/unit/controllers/parcels/parcelsController.test.ts @@ -164,7 +164,6 @@ describe('UNIT - Parcels', () => { mockRequest.setUser({ client_roles: [Roles.ADMIN] }); mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); mockResponse = mockRes; - _getParcels.mockImplementationOnce(() => [produceParcel()]); await controllers.getParcels(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue)).toBeTruthy(); @@ -188,6 +187,7 @@ describe('UNIT - Parcels', () => { }); it('should throw an error when getParcels service throws an error', async () => { mockRequest.query.pid = '1'; + mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); _getParcels.mockImplementationOnce(() => { throw new Error(); }); diff --git a/express-api/tests/unit/services/buildings/buildingService.test.ts b/express-api/tests/unit/services/buildings/buildingService.test.ts index 9b9677d9d..18d660a7b 100644 --- a/express-api/tests/unit/services/buildings/buildingService.test.ts +++ b/express-api/tests/unit/services/buildings/buildingService.test.ts @@ -4,7 +4,7 @@ import { produceBuilding, produceBuildingEvaluations, produceBuildingFiscals, - produceSSO, + producePimsRequestUser, produceUser, } from 'tests/testUtils/factories'; import * as buildingService from '@/services/buildings/buildingServices'; @@ -68,7 +68,7 @@ jest.spyOn(AppDataSource, 'createQueryRunner').mockReturnValue({ manager: _mockEntityManager, }); jest.spyOn(buildingRepo, 'find').mockImplementation(async () => [produceBuilding()]); -const adminUser = produceSSO({ client_roles: [Roles.ADMIN] }); +const adminUser = producePimsRequestUser({ RoleId: Roles.ADMIN, hasOneOfRoles: () => true }); describe('UNIT - Building Services', () => { describe('addBuilding', () => { beforeEach(() => jest.clearAllMocks()); @@ -91,14 +91,16 @@ describe('deleteBuildingById', () => { it('should delete a building and return a 204 status code', async () => { const buildingToDelete = produceBuilding(); _buildingFindOne.mockResolvedValueOnce(buildingToDelete); - await buildingService.deleteBuildingById(buildingToDelete.Id, ''); + await buildingService.deleteBuildingById(buildingToDelete.Id, adminUser); expect(_mockBuildinglUpdate).toHaveBeenCalledTimes(3); }); it('should throw a 404 error when the building does not exist', async () => { const buildingToDelete = produceBuilding(); _buildingFindOne.mockResolvedValueOnce(null); // Act & Assert - await expect(buildingService.deleteBuildingById(buildingToDelete.Id, '')).rejects.toThrow(); + await expect( + buildingService.deleteBuildingById(buildingToDelete.Id, adminUser), + ).rejects.toThrow(); }); }); describe('getBuildingById', () => { @@ -141,7 +143,10 @@ describe('updateBuildingById', () => { }); it('should throw an error if the building does not belong to the user and user is not admin', async () => { - const generalUser = produceSSO({ client_roles: [Roles.GENERAL_USER] }); + const generalUser = producePimsRequestUser({ + RoleId: Roles.GENERAL_USER, + hasOneOfRoles: () => false, + }); const updateBuilding = produceBuilding(); expect( async () => await buildingService.updateBuildingById(updateBuilding, generalUser), diff --git a/express-api/tests/unit/services/ches/chesServices.test.ts b/express-api/tests/unit/services/ches/chesServices.test.ts index 3da62535c..85d2345e9 100644 --- a/express-api/tests/unit/services/ches/chesServices.test.ts +++ b/express-api/tests/unit/services/ches/chesServices.test.ts @@ -1,6 +1,6 @@ import chesServices, { IEmail } from '@/services/ches/chesServices'; import { randomUUID } from 'crypto'; -import { produceEmail, produceSSO } from 'tests/testUtils/factories'; +import { produceEmail, producePimsRequestUser } from 'tests/testUtils/factories'; import * as config from '@/constants/config'; const _fetch = jest.fn().mockImplementation(() => { return { @@ -18,7 +18,7 @@ describe('UNIT - Ches Services', () => { describe('sendEmailSync', () => { it('should return a valid token response', async () => { const email = produceEmail({ cc: ['john@doe.com'], bcc: ['john@doe.com'] }); - const keycloak = produceSSO(); + const keycloak = producePimsRequestUser(); _fetch.mockImplementationOnce(() => ({ text: () => '{"access_token":"eyAiYSI6IDEgfQ==.ewoiZXhwIjoxCn0="}', })); @@ -30,7 +30,7 @@ describe('UNIT - Ches Services', () => { }); it('should throw an error on null email', async () => { const email: IEmail = null; - const keycloak = produceSSO(); + const keycloak = producePimsRequestUser(); _fetch.mockImplementationOnce(() => ({ text: () => '{"access_token":"eyAiYSI6IDEgfQ==.ewoiZXhwIjoxCn0="}', })); @@ -41,7 +41,7 @@ describe('UNIT - Ches Services', () => { }); it('should send email with extra config', async () => { const email: IEmail = produceEmail({}); - const keycloak = produceSSO(); + const keycloak = producePimsRequestUser(); _config.mockImplementationOnce(() => ({ ches: { emailEnabled: true, diff --git a/express-api/tests/unit/services/notifications/notificationServices.test.ts b/express-api/tests/unit/services/notifications/notificationServices.test.ts index e22036641..05a7ed27b 100644 --- a/express-api/tests/unit/services/notifications/notificationServices.test.ts +++ b/express-api/tests/unit/services/notifications/notificationServices.test.ts @@ -11,10 +11,10 @@ import { produceAgencyResponse, produceNotificationQueue, produceNotificationTemplate, + producePimsRequestUser, produceProject, produceProjectNotification, produceProjectStatusHistory, - produceSSO, produceUser, } from 'tests/testUtils/factories'; import { DeepPartial, EntityTarget, FindOptionsWhere, ObjectLiteral, UpdateResult } from 'typeorm'; @@ -184,7 +184,10 @@ describe('UNIT - Notification Services', () => { const sendThis = produceNotificationQueue(); sendThis.ChesMessageId = '00000000-0000-0000-0000-000000000000'; sendThis.ChesTransactionId = '00000000-0000-0000-0000-000000000001'; - const notifResult = await notificationServices.sendNotification(sendThis, produceSSO()); + const notifResult = await notificationServices.sendNotification( + sendThis, + producePimsRequestUser(), + ); expect(notifResult.ChesTransactionId).toBeTruthy(); expect(notifResult.ChesMessageId).toBeTruthy(); }); @@ -193,7 +196,10 @@ describe('UNIT - Notification Services', () => { _sendEmailAsync.mockImplementationOnce(() => { throw Error(); }); - const notifResult = await notificationServices.sendNotification(sendThis, produceSSO()); + const notifResult = await notificationServices.sendNotification( + sendThis, + producePimsRequestUser(), + ); expect(notifResult.Status).toBe(NotificationStatus.Failed); }); }); diff --git a/express-api/tests/unit/services/parcels/parcelsService.test.ts b/express-api/tests/unit/services/parcels/parcelsService.test.ts index 931da63a5..adae511ee 100644 --- a/express-api/tests/unit/services/parcels/parcelsService.test.ts +++ b/express-api/tests/unit/services/parcels/parcelsService.test.ts @@ -4,7 +4,7 @@ import { produceParcel, produceParcelEvaluations, produceParcelFiscals, - produceSSO, + producePimsRequestUser, produceUser, } from 'tests/testUtils/factories'; import parcelService from '@/services/parcels/parcelServices'; @@ -17,8 +17,6 @@ import userServices from '@/services/users/usersServices'; import { ProjectProperty } from '@/typeorm/Entities/ProjectProperty'; import { Roles } from '@/constants/roles'; -//jest.setTimeout(30000); - const parcelRepo = AppDataSource.getRepository(Parcel); const _parcelSave = jest @@ -87,7 +85,7 @@ jest.spyOn(AppDataSource, 'createQueryRunner').mockReturnValue({ release: jest.fn(async () => {}), manager: _mockEntityManager, }); -const adminUser = produceSSO({ client_roles: [Roles.ADMIN] }); +const adminUser = producePimsRequestUser({ RoleId: Roles.ADMIN, hasOneOfRoles: () => true }); describe('UNIT - Parcel Services', () => { describe('addParcel', () => { beforeEach(() => jest.clearAllMocks()); @@ -130,14 +128,14 @@ describe('UNIT - Parcel Services', () => { it('should delete a parcel and return a 204 status code', async () => { const parcelToDelete = produceParcel(); _parcelFindOne.mockResolvedValueOnce(parcelToDelete); - await parcelService.deleteParcelById(parcelToDelete.Id, ''); + await parcelService.deleteParcelById(parcelToDelete.Id, adminUser); expect(_mockParcelUpdate).toHaveBeenCalledTimes(3); }); it('should throw an error if the PID does not exist in the parcel table', () => { const parcelToDelete = produceParcel(); _parcelFindOne.mockResolvedValueOnce(null); expect( - async () => await parcelService.deleteParcelById(parcelToDelete.Id, ''), + async () => await parcelService.deleteParcelById(parcelToDelete.Id, adminUser), ).rejects.toThrow(); }); it('should throw an error if the Parcel has a child Parcel relationship', async () => { @@ -148,7 +146,7 @@ describe('UNIT - Parcel Services', () => { throw new ErrorWithCode(errorMessage); }); expect( - async () => await parcelService.deleteParcelById(newParentParcel.Id, ''), + async () => await parcelService.deleteParcelById(newParentParcel.Id, adminUser), ).rejects.toThrow(); }); }); diff --git a/express-api/tests/unit/services/projects/projectsServices.test.ts b/express-api/tests/unit/services/projects/projectsServices.test.ts index 5d602501f..e243ea252 100644 --- a/express-api/tests/unit/services/projects/projectsServices.test.ts +++ b/express-api/tests/unit/services/projects/projectsServices.test.ts @@ -24,13 +24,13 @@ import { produceNote, produceNotificationQueue, produceParcel, + producePimsRequestUser, produceProject, produceProjectJoin, produceProjectMonetary, produceProjectProperty, produceProjectTask, produceProjectTimestamp, - produceSSO, produceUser, } from 'tests/testUtils/factories'; import { @@ -259,14 +259,13 @@ describe('UNIT - Project Services', () => { it('should add a project and its relevant project property entries', async () => { const project = produceProject({ Name: 'Test Project' }); - const keycloak = produceSSO(); const result = await projectServices.addProject( project, { parcels: [3], buildings: [1], }, - keycloak, + producePimsRequestUser(), ); // Agency is checked for existance expect(_agencyExists).toHaveBeenCalledTimes(1); @@ -290,7 +289,6 @@ describe('UNIT - Project Services', () => { it('should throw an error if the project is missing a name', async () => { const project = produceProject({ Name: undefined }); - const keycloak = produceSSO(); expect( async () => await projectServices.addProject( @@ -299,7 +297,7 @@ describe('UNIT - Project Services', () => { parcels: [1], buildings: [1], }, - keycloak, + producePimsRequestUser(), ), ).rejects.toThrow(new ErrorWithCode('Projects must have a name.', 400)); }); @@ -307,7 +305,6 @@ describe('UNIT - Project Services', () => { it('should throw an error if the agency does not exist', async () => { _agencyExists.mockImplementationOnce(async () => false); const project = produceProject({}); - const keycloak = produceSSO(); expect( async () => await projectServices.addProject( @@ -316,7 +313,7 @@ describe('UNIT - Project Services', () => { parcels: [1], buildings: [1], }, - keycloak, + producePimsRequestUser(), ), ).rejects.toThrow(new ErrorWithCode(`Agency with ID ${project.AgencyId} not found.`, 404)); }); @@ -326,7 +323,6 @@ describe('UNIT - Project Services', () => { throw new Error(); }); const project = produceProject({}); - const keycloak = produceSSO(); expect( async () => await projectServices.addProject( @@ -335,7 +331,7 @@ describe('UNIT - Project Services', () => { parcels: [1], buildings: [1], }, - keycloak, + producePimsRequestUser(), ), ).rejects.toThrow(new ErrorWithCode('Error creating project.', 500)); }); @@ -343,7 +339,6 @@ describe('UNIT - Project Services', () => { it('should throw an error if the parcel attached to project does not exist', async () => { _parcelManagerFindOne.mockImplementationOnce(async () => null); const project = produceProject({}); - const keycloak = produceSSO(); expect( async () => await projectServices.addProject( @@ -352,7 +347,7 @@ describe('UNIT - Project Services', () => { parcels: [1], buildings: [1], }, - keycloak, + producePimsRequestUser(), ), ).rejects.toThrow(new ErrorWithCode(`Parcel with ID 1 does not exist.`, 404)); }); @@ -361,7 +356,6 @@ describe('UNIT - Project Services', () => { jest.clearAllMocks(); _buildingManagerFindOne.mockImplementationOnce(async () => null); const project = produceProject({}); - const keycloak = produceSSO(); expect( async () => await projectServices.addProject( @@ -370,7 +364,7 @@ describe('UNIT - Project Services', () => { parcels: [1], buildings: [1], }, - keycloak, + producePimsRequestUser(), ), ).rejects.toThrow(new ErrorWithCode(`Building with ID 1 does not exist.`, 404)); }); @@ -378,7 +372,6 @@ describe('UNIT - Project Services', () => { it('should throw an error if the parcel belongs to another project', async () => { const existingProject = produceProject({ StatusId: ProjectStatus.APPROVED_FOR_ERP }); const project = produceProject({ Id: existingProject.Id + 1 }); - const keycloak = produceSSO(); _projectPropertiesManagerFind.mockImplementationOnce(async () => { return [ produceProjectProperty({ @@ -395,7 +388,7 @@ describe('UNIT - Project Services', () => { { parcels: [1], }, - keycloak, + producePimsRequestUser(), ), ).rejects.toThrow( new ErrorWithCode(`Parcel with ID 1 already belongs to another active project.`, 400), @@ -405,7 +398,6 @@ describe('UNIT - Project Services', () => { it('should throw an error if the building belongs to another project', async () => { const existingProject = produceProject({ StatusId: ProjectStatus.APPROVED_FOR_ERP }); const project = produceProject({ Id: existingProject.Id + 1 }); - const keycloak = produceSSO(); _projectPropertiesManagerFindOne.mockImplementationOnce(async () => null); _projectPropertiesManagerFind.mockImplementationOnce(async () => { return [ @@ -423,7 +415,7 @@ describe('UNIT - Project Services', () => { { buildings: [1], }, - keycloak, + producePimsRequestUser(), ), ).rejects.toThrow( new ErrorWithCode(`Building with ID 1 already belongs to another active project.`, 400), @@ -455,7 +447,7 @@ describe('UNIT - Project Services', () => { jest.clearAllMocks(); }); it('should delete a project and return the DeleteResult object', async () => { - const result = await projectServices.deleteProjectById(1, ''); + const result = await projectServices.deleteProjectById(1, producePimsRequestUser()); // Was the project checked for existance? expect(_projectExists).toHaveBeenCalledTimes(1); @@ -475,7 +467,7 @@ describe('UNIT - Project Services', () => { it('should throw an error if the project does not exist', async () => { _projectExists.mockImplementationOnce(async () => false); - expect(projectServices.deleteProjectById(1, '')).rejects.toThrow( + expect(projectServices.deleteProjectById(1, producePimsRequestUser())).rejects.toThrow( new ErrorWithCode('Project does not exist.', 404), ); }); @@ -525,7 +517,7 @@ describe('UNIT - Project Services', () => { parcels: [1, 3], buildings: [4, 5], }, - produceSSO({ client_roles: [Roles.ADMIN] }), + producePimsRequestUser({ RoleId: Roles.ADMIN }), ); expect(result.StatusId).toBe(2); expect(result.Name).toBe('New Name'); @@ -542,7 +534,7 @@ describe('UNIT - Project Services', () => { StatusId: 2, }, {}, - produceSSO(), + producePimsRequestUser(), ), ).rejects.toThrow(new ErrorWithCode('Projects must have a name.', 400)); }); @@ -557,7 +549,7 @@ describe('UNIT - Project Services', () => { StatusId: 2, }, {}, - produceSSO(), + producePimsRequestUser(), ), ).rejects.toThrow(new ErrorWithCode('Project does not exist.', 404)); }); @@ -575,7 +567,7 @@ describe('UNIT - Project Services', () => { ProjectNumber: 'not a number', }, {}, - produceSSO(), + producePimsRequestUser(), ), ).rejects.toThrow(new ErrorWithCode('Project Number may not be changed.', 403)); }); @@ -591,7 +583,7 @@ describe('UNIT - Project Services', () => { AgencyId: 5, }, {}, - produceSSO(), + producePimsRequestUser({ hasOneOfRoles: () => false }), ), ).rejects.toThrow(new ErrorWithCode('Project Agency may not be changed.', 403)); }); @@ -608,7 +600,7 @@ describe('UNIT - Project Services', () => { parcels: [1, 3], buildings: [4, 5], }, - produceSSO({ client_roles: [Roles.ADMIN] }), + producePimsRequestUser(), ), ).rejects.toThrow(new ErrorWithCode('Error updating project: bad save', 500)); }); @@ -617,7 +609,11 @@ describe('UNIT - Project Services', () => { const oldProject = produceProject({}); const projUpd = { ...oldProject, AgencyResponses: [produceAgencyResponse()] }; _projectFindOne.mockImplementationOnce(async () => oldProject); - await projectServices.updateProject(projUpd, { parcels: [], buildings: [] }, produceSSO()); + await projectServices.updateProject( + projUpd, + { parcels: [], buildings: [] }, + producePimsRequestUser(), + ); expect(_generateProjectWatchNotifications).toHaveBeenCalled(); }); diff --git a/express-api/tests/unit/services/properties/propertyServices.test.ts b/express-api/tests/unit/services/properties/propertyServices.test.ts index 4b0a97c22..8c14ade82 100644 --- a/express-api/tests/unit/services/properties/propertyServices.test.ts +++ b/express-api/tests/unit/services/properties/propertyServices.test.ts @@ -46,10 +46,10 @@ import { produceParcelFiscals, produceBuildingEvaluations, produceBuildingFiscals, - produceSSO, produceProjectStatus, produceProjectProperty, produceImportRow, + producePimsRequestUser, } from 'tests/testUtils/factories'; import { DeepPartial, EntityTarget, ObjectLiteral } from 'typeorm'; import xlsx, { WorkSheet } from 'xlsx'; @@ -410,7 +410,7 @@ describe('UNIT - Property Services', () => { { quantity: 1, }, - produceSSO(), + producePimsRequestUser(), ); expect(Array.isArray(result)).toBe(true); expect(result.at(0)).toHaveProperty('CompletionPercentage'); From 0e958e1afebef9cf7d92ff9c65e1e5400de31fbf Mon Sep 17 00:00:00 2001 From: LawrenceLau2020 <68400651+LawrenceLau2020@users.noreply.github.com> Date: Fri, 13 Sep 2024 11:22:46 -0700 Subject: [PATCH 11/15] PIMS-2086: Remove getPIDs function (#2677) Co-authored-by: dbarkowsky --- .../src/controllers/tools/toolsController.ts | 12 ------- express-api/src/routes/tools.swagger.yaml | 31 ------------------- express-api/src/routes/toolsRouter.ts | 3 +- .../src/services/geocoder/geocoderService.ts | 25 --------------- .../controllers/tools/toolsController.test.ts | 10 ------ .../services/geocoder/geocoderService.test.ts | 26 ---------------- 6 files changed, 1 insertion(+), 106 deletions(-) diff --git a/express-api/src/controllers/tools/toolsController.ts b/express-api/src/controllers/tools/toolsController.ts index 5bd96fe41..0924c0317 100644 --- a/express-api/src/controllers/tools/toolsController.ts +++ b/express-api/src/controllers/tools/toolsController.ts @@ -90,15 +90,3 @@ export const searchGeocoderAddresses = async (req: Request, res: Response) => { const geoReturn = await geocoderService.getSiteAddresses(address, minScore, maxResults); return res.status(200).send(geoReturn); }; - -/** - * @description Search Geocoder for the pid of a certain siteId. - * @param {Request} req Incoming request. - * @param {Response} res Outgoing response. - * @returns {Response} A 200 status with a siteId object. - */ -export const searchGeocoderSiteId = async (req: Request, res: Response) => { - const siteId = String(req.params.siteId); - const result = await geocoderService.getPids(siteId); - return res.status(200).send(result); -}; diff --git a/express-api/src/routes/tools.swagger.yaml b/express-api/src/routes/tools.swagger.yaml index 37c42389e..0d8178fea 100644 --- a/express-api/src/routes/tools.swagger.yaml +++ b/express-api/src/routes/tools.swagger.yaml @@ -1,36 +1,5 @@ ### PATHS ### paths: - /tools/geocoder/parcels/pids/{siteID}: - get: - security: - - bearerAuth: [] - tags: - - Tools - summary: Returns an object with the siteID and a pids property. - description: > - The `pids` property seems to only be a single PID string. - This response comes from the BC Geocoder Service. - Capable of any error code from BC Geocoder. - parameters: - - in: path - name: siteID - schema: - type: string - format: uuid - responses: - '200': - content: - application/json: - schema: - type: object - properties: - siteID: - type: string - format: uuid - pids: - type: string - example: 014128446 - description: Has leading zeroes. Seemingly only one PID despite pluralization. /tools/geocoder/addresses: get: security: diff --git a/express-api/src/routes/toolsRouter.ts b/express-api/src/routes/toolsRouter.ts index 6d7b8d3f1..9d714f8a1 100644 --- a/express-api/src/routes/toolsRouter.ts +++ b/express-api/src/routes/toolsRouter.ts @@ -4,9 +4,8 @@ import express from 'express'; const router = express.Router(); -const { searchGeocoderAddresses, searchGeocoderSiteId } = controllers; +const { searchGeocoderAddresses } = controllers; router.route(`/geocoder/addresses`).get(catchErrors(searchGeocoderAddresses)); -router.route(`/geocoder/parcels/pids/:siteId`).get(catchErrors(searchGeocoderSiteId)); export default router; diff --git a/express-api/src/services/geocoder/geocoderService.ts b/express-api/src/services/geocoder/geocoderService.ts index 29d0249ac..a70352885 100644 --- a/express-api/src/services/geocoder/geocoderService.ts +++ b/express-api/src/services/geocoder/geocoderService.ts @@ -4,7 +4,6 @@ import constants from '@/constants'; import { IFeatureModel } from '@/services/geocoder/interfaces/IFeatureModel'; import { getLongitude, getLatitude, getAddress1 } from './geocoderUtils'; import { ErrorWithCode } from '@/utilities/customErrors/ErrorWithCode'; -import { ISitePidsResponseModel } from './interfaces/ISitePidsResponseModel'; const mapFeatureToAddress = (feature: IFeatureModel): IAddressModel => { return { @@ -55,32 +54,8 @@ export const getSiteAddresses = async ( return addressInformation; }; -/** - * @description Sends a request to Geocoder for all parcel identifiers (PIDs) associated with an individual site. - * @param siteId a unique identifier assigned to every site in B.C. - * @returns Valid 'siteId' values for an address - * @throws ErrorWithCode if result is not 200 OK - */ -export const getPids = async (siteId: string) => { - const url = new URL(`/parcels/pids/${siteId}.json`, constants.GEOCODER.HOSTURI); - const result = await fetch(url.toString(), { - headers: { - apiKey: constants.GEOCODER.KEY, - }, - }); - - if (result.status != 200) { - throw new ErrorWithCode(result.statusText, result.status); - } - - const resultData = await result.json(); - const pidInformation: ISitePidsResponseModel = resultData; - return pidInformation; -}; - const geocoderService = { getSiteAddresses, - getPids, }; export default geocoderService; diff --git a/express-api/tests/unit/controllers/tools/toolsController.test.ts b/express-api/tests/unit/controllers/tools/toolsController.test.ts index c965b8f3e..bb4e5b259 100644 --- a/express-api/tests/unit/controllers/tools/toolsController.test.ts +++ b/express-api/tests/unit/controllers/tools/toolsController.test.ts @@ -5,8 +5,6 @@ import { MockRes, getRequestHandlerMocks, produceEmailStatus, - produceGeocoderAddress, - producePidsResponse, } from '../../../testUtils/factories'; import { randomUUID } from 'crypto'; @@ -36,14 +34,6 @@ jest.mock('@/services/ches/chesServices.ts', () => ({ sendEmailAsync: () => _sendEmailAsync(), })); -const _getSiteAddresses = jest.fn().mockImplementation(() => [produceGeocoderAddress()]); -const _getPids = jest.fn().mockImplementation(() => producePidsResponse()); - -jest.mock('@/services/geocoder/geocoderService', () => ({ - getSiteAddresses: () => _getSiteAddresses(), - getPids: () => _getPids(), -})); - describe('UNIT - Tools', () => { let mockRequest: Request & MockReq, mockResponse: Response & MockRes; diff --git a/express-api/tests/unit/services/geocoder/geocoderService.test.ts b/express-api/tests/unit/services/geocoder/geocoderService.test.ts index cf905b4f5..8448e3d86 100644 --- a/express-api/tests/unit/services/geocoder/geocoderService.test.ts +++ b/express-api/tests/unit/services/geocoder/geocoderService.test.ts @@ -99,30 +99,4 @@ describe('UNIT - Geoserver services', () => { }).rejects.toThrow(); }); }); - - describe('getPids', () => { - const pidData = { - siteID: 'eccd759a-8476-46b0-af5d-e1c071f8e78e', - pids: '000382345', - }; - const stringPids = JSON.stringify(pidData); - - it('should get a list of PIDs connected to the site address.', async () => { - jest - .spyOn(global, 'fetch') - .mockImplementationOnce(() => Promise.resolve(new Response(stringPids))); - const pids = await geocoderService.getPids('eccd759a-8476-46b0-af5d-e1c071f8e78e'); - expect(typeof pids === 'object' && !Array.isArray(pids) && pids !== null).toBe(true); - expect(typeof pids.pids === 'string' && pids.pids === '000382345').toBe(true); - }); - - it('should thow an error if geocoder service is down.', async () => { - jest - .spyOn(global, 'fetch') - .mockImplementationOnce(() => Promise.resolve(new Response('', { status: 500 }))); - expect(async () => { - await geocoderService.getPids(''); - }).rejects.toThrow(); - }); - }); }); From bee59ec732774df7fd333a422411e23a93da55af Mon Sep 17 00:00:00 2001 From: LawrenceLau2020 <68400651+LawrenceLau2020@users.noreply.github.com> Date: Tue, 17 Sep 2024 08:25:59 -0700 Subject: [PATCH 12/15] PIMS-2087: Update express and remove body-parser (#2680) --- express-api/package.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/express-api/package.json b/express-api/package.json index d08487c98..258fe4f2e 100644 --- a/express-api/package.json +++ b/express-api/package.json @@ -21,12 +21,11 @@ "dependencies": { "@bcgov/citz-imb-kc-css-api": "https://github.com/bcgov/citz-imb-kc-css-api/releases/download/v1.4.0/bcgov-citz-imb-kc-css-api-1.4.0.tgz", "@bcgov/citz-imb-sso-express": "1.0.1", - "body-parser": "1.20.2", "compression": "1.7.4", "cookie-parser": "1.4.6", "cors": "2.8.5", "dotenv": "16.4.1", - "express": "4.19.2", + "express": "4.21.0", "express-rate-limit": "7.4.0", "morgan": "1.10.0", "multer": "1.4.5-lts.1", From 3bba4d7870e5f4976c52796301e0c9acdb043bd9 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Tue, 17 Sep 2024 15:44:53 -0700 Subject: [PATCH 13/15] more tests --- .../administrativeAreasController.ts | 2 +- .../administrativeAreaSchema.ts | 2 +- .../src/services/agencies/agencySchema.ts | 6 +- express-api/tests/testUtils/factories.ts | 2 +- .../administrativeAreasController.test.ts | 16 ++++-- .../agencies/agenciesController.test.ts | 18 +++++- .../lookup/lookupController.test.ts | 56 ++++++++++++++----- 7 files changed, 76 insertions(+), 26 deletions(-) diff --git a/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts b/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts index f2c23c9ac..6928b6759 100644 --- a/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts +++ b/express-api/src/controllers/administrativeAreas/administrativeAreasController.ts @@ -18,7 +18,7 @@ export const getAdministrativeAreas = async (req: Request, res: Response) => { if (filter.success) { const adminAreas = await administrativeAreasServices.getAdministrativeAreas(filter.data); if (!user.hasOneOfRoles([Roles.ADMIN])) { - const trimmed = AdministrativeAreaPublicResponseSchema.array().parse(adminAreas.data); + const trimmed = AdministrativeAreaPublicResponseSchema.array().parse(adminAreas); return res.status(200).send({ ...adminAreas, data: trimmed, diff --git a/express-api/src/services/administrativeAreas/administrativeAreaSchema.ts b/express-api/src/services/administrativeAreas/administrativeAreaSchema.ts index a48af739f..4e305f1d0 100644 --- a/express-api/src/services/administrativeAreas/administrativeAreaSchema.ts +++ b/express-api/src/services/administrativeAreas/administrativeAreaSchema.ts @@ -15,7 +15,7 @@ export const AdministrativeAreaFilterSchema = z.object({ export const AdministrativeAreaPublicResponseSchema = z.object({ Id: z.number(), Name: z.string(), - RegionalDistrictName: z.string(), + RegionalDistrictId: z.number(), ProvinceId: z.string(), CreatedOn: z.date(), }); diff --git a/express-api/src/services/agencies/agencySchema.ts b/express-api/src/services/agencies/agencySchema.ts index 6ee0c7ca2..8f72a3693 100644 --- a/express-api/src/services/agencies/agencySchema.ts +++ b/express-api/src/services/agencies/agencySchema.ts @@ -41,10 +41,10 @@ export const AgencyPublicResponseSchema = z.object({ Name: z.string(), SortOrder: z.number(), Code: z.string(), - Description: z.string().nullable(), + Description: z.string().optional(), IsDisabled: z.boolean(), - ParentId: z.number().int().nullable(), - ParentName: z.string(), + ParentId: z.number().int().optional(), + ParentName: z.string().optional(), }); export type Agency = z.infer; diff --git a/express-api/tests/testUtils/factories.ts b/express-api/tests/testUtils/factories.ts index 1c91aefc3..1bbf7a88e 100644 --- a/express-api/tests/testUtils/factories.ts +++ b/express-api/tests/testUtils/factories.ts @@ -483,7 +483,7 @@ export const produceConstructionType = (props?: Partial) return constructionType; }; -export const produceRegionalDistrict = (props: Partial) => { +export const produceRegionalDistrict = (props?: Partial) => { const regionalDistrict: RegionalDistrict = { Id: faker.number.int(), Abbreviation: faker.string.alpha(5), diff --git a/express-api/tests/unit/controllers/administrativeAreas/administrativeAreasController.test.ts b/express-api/tests/unit/controllers/administrativeAreas/administrativeAreasController.test.ts index 0036156f0..fef507d80 100644 --- a/express-api/tests/unit/controllers/administrativeAreas/administrativeAreasController.test.ts +++ b/express-api/tests/unit/controllers/administrativeAreas/administrativeAreasController.test.ts @@ -34,10 +34,10 @@ const mockAdministrativeArea: IAdministrativeArea = { regionalDistrict: 'CPRD', }; -const _getAdminAreas = jest.fn().mockImplementation(() => [produceAdminArea({})]); -const _getAdminAreaById = jest.fn().mockImplementation(() => produceAdminArea({})); -const _updateAdminAreaById = jest.fn().mockImplementation(() => produceAdminArea({})); -const _addAdminArea = jest.fn().mockImplementation(() => produceAdminArea({})); +const _getAdminAreas = jest.fn().mockImplementation(() => [produceAdminArea()]); +const _getAdminAreaById = jest.fn().mockImplementation(() => produceAdminArea()); +const _updateAdminAreaById = jest.fn().mockImplementation(() => produceAdminArea()); +const _addAdminArea = jest.fn().mockImplementation(() => produceAdminArea()); jest.mock('@/services/administrativeAreas/administrativeAreasServices', () => ({ getAdministrativeAreas: () => _getAdminAreas(), @@ -58,12 +58,18 @@ describe('UNIT - Administrative Areas Admin', () => { mockResponse = mockRes; }); describe('Controller getAdministrativeAreas', () => { - // TODO: enable other tests when controller is complete it('should return status 200 and a list of administrative areas', async () => { mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); await getAdministrativeAreas(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); }); + it('should return status 200 and a list of admin areas with trimmed properties if not an Admin', async () => { + mockRequest.setPimsUser({ hasOneOfRoles: () => false }); + await getAdministrativeAreas(mockRequest, mockResponse); + expect(mockResponse.statusValue).toBe(200); + expect(mockResponse.sendValue.data[0].Name).toBeDefined(); + expect(mockResponse.sendValue.data[0].CreatedBy).not.toBeDefined(); + }); it('should return status 400 when parse fails', async () => { mockRequest.query = { name: ['a'] }; await getAdministrativeAreas(mockRequest, mockResponse); diff --git a/express-api/tests/unit/controllers/agencies/agenciesController.test.ts b/express-api/tests/unit/controllers/agencies/agenciesController.test.ts index 9d2ef95bd..2c05014f7 100644 --- a/express-api/tests/unit/controllers/agencies/agenciesController.test.ts +++ b/express-api/tests/unit/controllers/agencies/agenciesController.test.ts @@ -54,10 +54,12 @@ describe('UNIT - Agencies Admin', () => { expect(mockResponse.statusValue).toBe(200); }); - it('should return status 200 and a list of agencies with no filter', async () => { - _getKeycloakUserRoles.mockImplementationOnce(() => []); + it('should return status 200 and a list of agencies with trimmed properties if not an Admin', async () => { + mockRequest.setPimsUser({ hasOneOfRoles: () => false }); await controllers.getAgencies(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); + expect(mockResponse.sendValue.data[0].Name).toBeDefined(); + expect(mockResponse.sendValue.data[0].CreatedBy).not.toBeDefined(); }); it('should return status 200 and a list of agencies when given a filter', async () => { @@ -127,6 +129,13 @@ describe('UNIT - Agencies Admin', () => { expect(mockResponse.statusValue).toBe(200); expect(mockResponse.sendValue.Id).toBe(agency.Id); }); + it('should return status 400 upon a failed filter', async () => { + mockRequest.params.id = {} as unknown as string; + const agency = produceAgency(); + mockRequest.body = agency; + await controllers.updateAgencyById(mockRequest, mockResponse); + expect(mockResponse.statusValue).toBe(400); + }); it('should return status 400 and specify that there was a mismatch', async () => { const agency = produceAgency(); mockRequest.params.id = 'asdf'; @@ -165,6 +174,11 @@ describe('UNIT - Agencies Admin', () => { // expect(mockResponse.sendValue.Id).toBe(agency.Id); expect(mockResponse.sendValue).toBeUndefined(); }); + it('should return status 400 upon a failed filter', async () => { + mockRequest.params.id = {} as unknown as string; + await controllers.deleteAgencyById(mockRequest, mockResponse); + expect(mockResponse.statusValue).toBe(400); + }); it('should throw an error when deleteAgencyById service throws an error', async () => { _deleteAgencyById.mockImplementationOnce(() => { throw new ErrorWithCode('', 400); diff --git a/express-api/tests/unit/controllers/lookup/lookupController.test.ts b/express-api/tests/unit/controllers/lookup/lookupController.test.ts index daf58396c..39406da16 100644 --- a/express-api/tests/unit/controllers/lookup/lookupController.test.ts +++ b/express-api/tests/unit/controllers/lookup/lookupController.test.ts @@ -29,6 +29,8 @@ import { lookupBuildingPredominateUse, lookupMonetaryTypes, lookupNoteTypes, + lookupProjectStatuses, + lookupPropertyTypes, lookupRegionalDistricts, lookupTasks, lookupTimestampTypes, @@ -48,21 +50,30 @@ import { AdministrativeArea } from '@/typeorm/Entities/AdministrativeArea'; const { lookupAll, lookupProjectTierLevels, lookupPropertyClassifications } = controllers; +// Returning 2 entries for many of these to trigger the .sort calls. Otherwise it is skipped. const _next = jest.fn(); -const _findClassification = jest.fn().mockImplementation(() => [produceClassification({})]); -const _findUses = jest.fn().mockImplementation(() => [producePredominateUse({})]); -const _findConstruction = jest.fn().mockImplementation(() => [produceConstructionType({})]); -const _findRegionalDistricts = jest.fn().mockImplementation(() => [produceRegionalDistrict({})]); -const _findTierLevel = jest.fn().mockImplementation(() => [produceTierLevel()]); -const _findTasks = jest.fn().mockImplementation(() => [produceTask()]); -const _findNoteTypes = jest.fn().mockImplementation(() => [produceNoteType()]); -const _findTimestampTypes = jest.fn().mockImplementation(() => [produceTimestampType()]); -const _findMonetaryTypes = jest.fn().mockImplementation(() => [produceMonetaryType()]); +const _findClassification = jest.fn().mockImplementation(() => [produceClassification()]); +const _findUses = jest.fn().mockImplementation(() => [producePredominateUse()]); +const _findConstruction = jest.fn().mockImplementation(() => [produceConstructionType()]); +const _findRegionalDistricts = jest + .fn() + .mockImplementation(() => [produceRegionalDistrict(), produceRegionalDistrict()]); +const _findTierLevel = jest.fn().mockImplementation(() => [produceTierLevel(), produceTierLevel()]); +const _findTasks = jest.fn().mockImplementation(() => [produceTask(), produceTask()]); +const _findNoteTypes = jest.fn().mockImplementation(() => [produceNoteType(), produceNoteType()]); +const _findTimestampTypes = jest + .fn() + .mockImplementation(() => [produceTimestampType(), produceTimestampType()]); +const _findMonetaryTypes = jest + .fn() + .mockImplementation(() => [produceMonetaryType(), produceMonetaryType()]); const _findProjectRisks = jest.fn().mockImplementation(() => [produceRisk()]); const _findPropertyTypes = jest.fn().mockImplementation(() => [producePropertyType()]); -const _findProjectStatuses = jest.fn().mockImplementation(() => [produceProjectStatus]); +const _findProjectStatuses = jest + .fn() + .mockImplementation(() => [produceProjectStatus(), produceProjectStatus()]); const _findRoles = jest.fn().mockImplementation(() => [produceRole()]); -const _findAgencies = jest.fn().mockImplementation(() => [produceAgency()]); +const _findAgencies = jest.fn().mockImplementation(() => [produceAgency(), produceAgency()]); const _findAdminAreas = jest.fn().mockImplementation(() => [produceAdminArea()]); jest @@ -238,6 +249,18 @@ describe('UNIT - Lookup Controller', () => { }); }); + describe('GET /lookup/projectStatus', () => { + it('should return status 200 and a list of statuses', async () => { + await lookupProjectStatuses(mockRequest, mockResponse); + expect(mockResponse.statusValue).toBe(200); + }); + it('should return 400 on bad parse', async () => { + _findProjectStatuses.mockImplementationOnce(() => [{ Name: [] }]); + await lookupProjectStatuses(mockRequest, mockResponse); + expect(mockResponse.statusValue).toBe(400); + }); + }); + describe('GET /lookup/tasks', () => { it('should return status 200 and a list of tasks', async () => { await lookupTasks(mockRequest, mockResponse); @@ -250,6 +273,13 @@ describe('UNIT - Lookup Controller', () => { }); }); + describe('GET /lookup/propertyTypes', () => { + it('should return status 200 and a list of property types', async () => { + await lookupPropertyTypes(mockRequest, mockResponse); + expect(mockResponse.statusValue).toBe(200); + }); + }); + describe('GET /lookup/noteTypes', () => { it('should return status 200 and a list of note types', async () => { await lookupNoteTypes(mockRequest, mockResponse); @@ -293,9 +323,9 @@ describe('UNIT - Lookup Controller', () => { // Check that something was returned expect(mockResponse.sendValue.AdministrativeAreas).toHaveLength(1); expect(mockResponse.sendValue.Roles).toHaveLength(1); - expect(mockResponse.sendValue.Agencies).toHaveLength(1); + expect(mockResponse.sendValue.Agencies).toHaveLength(2); expect(mockResponse.sendValue.Classifications).toHaveLength(1); - expect(mockResponse.sendValue.Tasks).toHaveLength(1); + expect(mockResponse.sendValue.Tasks).toHaveLength(2); }); }); }); From c5edfb52f17576313b3bffdda0b555aaacccab90 Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Tue, 17 Sep 2024 15:50:54 -0700 Subject: [PATCH 14/15] most test coverage --- .../properties/propertiesController.test.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/express-api/tests/unit/controllers/properties/propertiesController.test.ts b/express-api/tests/unit/controllers/properties/propertiesController.test.ts index b1994a1bf..f402be753 100644 --- a/express-api/tests/unit/controllers/properties/propertiesController.test.ts +++ b/express-api/tests/unit/controllers/properties/propertiesController.test.ts @@ -111,7 +111,7 @@ describe('UNIT - Properties', () => { it('should return 200 with a list of properties', async () => { mockRequest.query.keyword = '123'; mockRequest.query.take = '3'; - mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN, hasOneOfRoles: () => false }); await getPropertiesFuzzySearch(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue.Parcels)).toBe(true); @@ -153,6 +153,7 @@ describe('UNIT - Properties', () => { ClassificationIds: '1,2', PropertyTypeIds: '1,2', AdministrativeAreaIds: '1,2', + RegionalDistrictIds: '1,2', }; await getPropertiesForMap(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); @@ -189,7 +190,7 @@ describe('UNIT - Properties', () => { describe('GET /properties/', () => { it('should return status 200', async () => { - mockRequest.setPimsUser({ RoleId: Roles.ADMIN }); + mockRequest.setPimsUser({ RoleId: Roles.ADMIN, hasOneOfRoles: () => false }); await getPropertyUnion(mockRequest, mockResponse); expect(mockResponse.statusValue).toBe(200); expect(Array.isArray(mockResponse.sendValue)).toBe(true); @@ -239,5 +240,15 @@ describe('UNIT - Properties', () => { expect(mockResponse.statusValue).toBe(200); expect(mockResponse.sendValue).toEqual([{ id: 1, name: 'Linked Project 1', buildingId: 1 }]); }); + + it('should return 200 with linked projects for a parcel ID', async () => { + _findLinkedProjectsForProperty.mockImplementationOnce(async () => { + return [{ id: 1, name: 'Linked Project 1', parcelId: 1 }]; + }); + mockRequest.query.parcelId = '1'; + await getLinkedProjects(mockRequest, mockResponse); + expect(mockResponse.statusValue).toBe(200); + expect(mockResponse.sendValue).toEqual([{ id: 1, name: 'Linked Project 1', parcelId: 1 }]); + }); }); }); From b5e59b11ba33c04eda96947f3410d6e1835350ad Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Wed, 18 Sep 2024 09:50:05 -0700 Subject: [PATCH 15/15] comment fix --- express-api/src/constants/roles.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/express-api/src/constants/roles.ts b/express-api/src/constants/roles.ts index 0009ec730..68ca1ae5e 100644 --- a/express-api/src/constants/roles.ts +++ b/express-api/src/constants/roles.ts @@ -1,6 +1,6 @@ /** * @enum - * The values in this enum must exactly mirror the names of the Keycloak roles. + * The values in this enum must exactly mirror the IDs in the Role table. */ export enum Roles { ADMIN = '00000000-0000-0000-0000-000000000000',