From ae58f541f1010e735b31d9d672326ca6eb91a8d7 Mon Sep 17 00:00:00 2001 From: Nick Phura Date: Wed, 26 Jun 2024 11:36:19 -0700 Subject: [PATCH 01/12] Copy existing database functions to procedures folder, where they can be migrated and maintained more easily. Update api_patch_system_user.ts to do case-insensitive comparisons when finding a system user. --- .../src/procedures/api_patch_system_user.ts | 109 ++++++++++++++++++ database/src/procedures/api_set_context.ts | 50 ++++++++ 2 files changed, 159 insertions(+) create mode 100644 database/src/procedures/api_patch_system_user.ts create mode 100644 database/src/procedures/api_set_context.ts diff --git a/database/src/procedures/api_patch_system_user.ts b/database/src/procedures/api_patch_system_user.ts new file mode 100644 index 0000000000..f85970c8a6 --- /dev/null +++ b/database/src/procedures/api_patch_system_user.ts @@ -0,0 +1,109 @@ +import { Knex } from 'knex'; + +/** + * Add/update function that patches a system user record. + * + * Steps: + * 1. Attempts to find an existing system user record + * - First using `p_system_user_guid` + * - Second using `p_user_identifier` and `p_user_identity_source_name` + * 2. If no user is found, return null + * 3. If a user is found, update the system user record with the latest information passed to this function if any of + * the incoming values are not the same as the existing values + * + * @export + * @param {Knex} knex + * @return {*} {Promise} + */ +export async function up(knex: Knex): Promise { + await knex.raw(`--sql + set search_path=biohub; + + CREATE OR REPLACE FUNCTION + api_patch_system_user ( + p_system_user_guid character varying, + p_user_identifier character varying, + p_user_identity_source_name character varying, + p_email character varying, + p_display_name character varying, + p_given_name character varying, + p_family_name character varying, + p_agency character varying + ) + RETURNS integer + LANGUAGE plpgsql + SET client_min_messages TO 'warning' + AS $$ + -- ******************************************************************* + -- Procedure: api_patch_system_user + -- Purpose: Updates a system_user record if any of the incoming values are not the same as the existing values. + -- + -- MODIFICATION HISTORY + -- Person Date Comments + -- ---------------- ----------- -------------------------------------- + -- nick.phura@quartech.com + -- 2023-08-01 initial release + -- ******************************************************************* + DECLARE + _system_user system_user%rowtype; + _user_identity_source_id user_identity_source.user_identity_source_id%type; + BEGIN + -- Attempt to find user based on guid + SELECT * INTO _system_user FROM system_user + WHERE LOWER(user_guid) = LOWER(p_system_user_guid) + AND record_end_date IS NULL + LIMIT 1; + + -- Otherwise, attempt to find user based on identifier and identity source + IF NOT found THEN + SELECT user_identity_source_id INTO strict _user_identity_source_id FROM user_identity_source + WHERE LOWER(name) = LOWER(p_user_identity_source_name) + AND record_end_date IS NULL; + + SELECT * INTO _system_user FROM system_user + WHERE user_identity_source_id = _user_identity_source_id + AND LOWER(user_identifier) = LOWER(p_user_identifier) + LIMIT 1; + END IF; + + -- If no user found, return and do nothing + IF NOT found THEN + RETURN NULL; + END IF; + + -- Otherwise, patch the system user record with the latest information passed to this function + UPDATE system_user SET + user_guid = p_system_user_guid, + user_identifier = p_user_identifier, + email = p_email, + display_name = p_display_name, + given_name = p_given_name, + family_name = p_family_name, + agency = p_agency + WHERE + system_user_id = _system_user.system_user_id + AND ( + user_guid != p_system_user_guid OR + user_identifier != p_user_identifier OR + email != p_email OR + display_name != p_display_name OR + given_name != p_given_name OR + family_name != p_family_name OR + agency != p_agency + ); + + -- Return system user id of patched record + RETURN _system_user.system_user_id; + EXCEPTION + WHEN OTHERS THEN + RAISE; + END; + $$; + + COMMENT ON FUNCTION api_patch_system_user(varchar, varchar, varchar, varchar, varchar, varchar, varchar, varchar) IS 'Updates a system_user record if any of the incoming values are not the same as the existing values.'; + `); +} + +export async function down(knex: Knex): Promise { + await knex.raw(``); +} diff --git a/database/src/procedures/api_set_context.ts b/database/src/procedures/api_set_context.ts new file mode 100644 index 0000000000..dc5e4cd48c --- /dev/null +++ b/database/src/procedures/api_set_context.ts @@ -0,0 +1,50 @@ +import { Knex } from 'knex'; + +/** + * Add/update function to set the context for the user making a database request. Used by the auditing and journaling + * triggers. + * + * @export + * @param {Knex} knex + * @return {*} {Promise} + */ +export async function up(knex: Knex): Promise { + await knex.raw(` + SET search_path = 'biohub'; + + DROP FUNCTION IF EXISTS api_set_context; + + CREATE OR REPLACE FUNCTION api_set_context(p_system_user_guid system_user.user_guid%type, p_user_identity_source_name user_identity_source.name%type) RETURNS system_user.system_user_id%type + language plpgsql + security invoker + SET client_min_messages = warning + AS + $$ + DECLARE + _system_user_id system_user.system_user_id%type; + _user_identity_source_id user_identity_source.user_identity_source_id%type; + BEGIN + SELECT user_identity_source_id INTO strict _user_identity_source_id FROM user_identity_source + WHERE LOWER(name) = LOWER(p_user_identity_source_name) + AND record_end_date IS NULL; + + SELECT system_user_id INTO strict _system_user_id FROM system_user + WHERE user_identity_source_id = _user_identity_source_id + AND LOWER(user_guid) = LOWER(p_system_user_guid); + + CREATE TEMP TABLE IF NOT EXISTS biohub_context_temp (tag varchar(200), value varchar(200)); + DELETE FROM biohub_context_temp WHERE tag = 'user_id'; + INSERT INTO biohub_context_temp (tag, value) values ('user_id', _system_user_id::varchar(200)); + + RETURN _system_user_id; + EXCEPTION + WHEN OTHERS THEN + RAISE; + END; + $$; + `); +} + +export async function down(knex: Knex): Promise { + await knex.raw(``); +} From b7961f4c0b6dd6bd5b29c7026cd8ac9193c3ee04 Mon Sep 17 00:00:00 2001 From: Nick Phura Date: Thu, 27 Jun 2024 11:09:56 -0700 Subject: [PATCH 02/12] Fix procedures not having 'seed' function --- database/src/procedures/api_patch_system_user.ts | 6 +----- database/src/procedures/api_set_context.ts | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/database/src/procedures/api_patch_system_user.ts b/database/src/procedures/api_patch_system_user.ts index f85970c8a6..381695cc20 100644 --- a/database/src/procedures/api_patch_system_user.ts +++ b/database/src/procedures/api_patch_system_user.ts @@ -15,7 +15,7 @@ import { Knex } from 'knex'; * @param {Knex} knex * @return {*} {Promise} */ -export async function up(knex: Knex): Promise { +export async function seed(knex: Knex): Promise { await knex.raw(`--sql set search_path=biohub; @@ -103,7 +103,3 @@ export async function up(knex: Knex): Promise { COMMENT ON FUNCTION api_patch_system_user(varchar, varchar, varchar, varchar, varchar, varchar, varchar, varchar) IS 'Updates a system_user record if any of the incoming values are not the same as the existing values.'; `); } - -export async function down(knex: Knex): Promise { - await knex.raw(``); -} diff --git a/database/src/procedures/api_set_context.ts b/database/src/procedures/api_set_context.ts index dc5e4cd48c..f37a5969d5 100644 --- a/database/src/procedures/api_set_context.ts +++ b/database/src/procedures/api_set_context.ts @@ -8,7 +8,7 @@ import { Knex } from 'knex'; * @param {Knex} knex * @return {*} {Promise} */ -export async function up(knex: Knex): Promise { +export async function seed(knex: Knex): Promise { await knex.raw(` SET search_path = 'biohub'; @@ -44,7 +44,3 @@ export async function up(knex: Knex): Promise { $$; `); } - -export async function down(knex: Knex): Promise { - await knex.raw(``); -} From a4c12b67644cb8aeb26edb17b248ad17012ac4e3 Mon Sep 17 00:00:00 2001 From: Macgregor Aubertin-Young Date: Thu, 27 Jun 2024 13:14:31 -0700 Subject: [PATCH 03/12] migration to fix duplicate data --- api/src/paths/administrative-activity.ts | 11 +- api/src/paths/user/add.test.ts | 6 + api/src/paths/user/add.ts | 8 + .../admin/users/AccessRequestList.test.tsx | 7 +- .../admin/users/AccessRequestList.tsx | 9 +- .../features/admin/users/ActiveUsersList.tsx | 41 ++- .../users/ReviewAccessRequestForm.test.tsx | 1 - app/src/interfaces/useAdminApi.interface.ts | 1 - app/src/pages/access/AccessRequestPage.tsx | 2 +- app/src/pages/access/IDIRRequestForm.tsx | 47 +--- ...2024062611000000_remove_duplicate_users.ts | 249 ++++++++++++++++++ .../src/procedures/api_patch_system_user.ts | 6 +- database/src/procedures/api_set_context.ts | 6 +- 13 files changed, 308 insertions(+), 86 deletions(-) create mode 100644 database/src/migrations/2024062611000000_remove_duplicate_users.ts diff --git a/api/src/paths/administrative-activity.ts b/api/src/paths/administrative-activity.ts index 04dec02ae5..33b6eec094 100644 --- a/api/src/paths/administrative-activity.ts +++ b/api/src/paths/administrative-activity.ts @@ -27,7 +27,16 @@ POST.apiDoc = { schema: { title: 'Administrative Activity request object', type: 'object', - properties: {} + additionalProperties: false, + properties: { + reason: { type: 'string', description: 'Reason for the access request.' }, + userGuid: { type: 'string', description: 'Unique keycloak GUID for the user.' }, + name: { type: 'string' }, + username: { type: 'string' }, + email: { type: 'string' }, + identitySource: { type: 'string', description: 'Whether the account is an IDIR or BCeID.' }, + displayName: { type: 'string' } + } } } } diff --git a/api/src/paths/user/add.test.ts b/api/src/paths/user/add.test.ts index ed4bb716ce..55f598527f 100644 --- a/api/src/paths/user/add.test.ts +++ b/api/src/paths/user/add.test.ts @@ -50,6 +50,8 @@ describe('user', () => { agency: null }; + const getUserByIdentifierStub = sinon.stub(UserService.prototype, 'getUserByIdentifier').resolves(); + const ensureSystemUserStub = sinon.stub(UserService.prototype, 'ensureSystemUser').resolves(mockUserObject); const adduserSystemRolesStub = sinon.stub(UserService.prototype, 'addUserSystemRoles'); @@ -58,6 +60,7 @@ describe('user', () => { await requestHandler(mockReq, mockRes, mockNext); + expect(getUserByIdentifierStub).to.have.been.calledOnce; expect(ensureSystemUserStub).to.have.been.calledOnce; expect(adduserSystemRolesStub).to.have.been.calledOnce; }); @@ -93,6 +96,8 @@ describe('user', () => { agency: null }; + const getUserByIdentifierStub = sinon.stub(UserService.prototype, 'getUserByIdentifier').resolves(); + const ensureSystemUserStub = sinon.stub(UserService.prototype, 'ensureSystemUser').resolves(mockUserObject); const adduserSystemRolesStub = sinon.stub(UserService.prototype, 'addUserSystemRoles'); @@ -100,6 +105,7 @@ describe('user', () => { const requestHandler = user.addSystemRoleUser(); await requestHandler(mockReq, mockRes, mockNext); + expect(getUserByIdentifierStub).to.have.been.calledOnce; expect(ensureSystemUserStub).to.have.been.calledOnce; expect(adduserSystemRolesStub).to.have.been.calledOnce; }); diff --git a/api/src/paths/user/add.ts b/api/src/paths/user/add.ts index abfdb376f2..f457df5b68 100644 --- a/api/src/paths/user/add.ts +++ b/api/src/paths/user/add.ts @@ -143,6 +143,14 @@ export function addSystemRoleUser(): RequestHandler { const userService = new UserService(connection); + // If user already exists, do nothing and return early + const user = await userService.getUserByIdentifier(userIdentifier, identitySource); + if (user) { + await connection.commit(); + return res.status(400).send('That user has already been added.'); + } + + // This function also verifies that the user exists, but we explicitly check above to return a useful error message. const userObject = await userService.ensureSystemUser( userGuid, userIdentifier, diff --git a/app/src/features/admin/users/AccessRequestList.test.tsx b/app/src/features/admin/users/AccessRequestList.test.tsx index b65a7973e5..da0b03f7af 100644 --- a/app/src/features/admin/users/AccessRequestList.test.tsx +++ b/app/src/features/admin/users/AccessRequestList.test.tsx @@ -74,7 +74,6 @@ describe('AccessRequestList', () => { username: 'testusername', userGuid: 'aaaa', email: 'email@email.com', - role: 2, identitySource: SYSTEM_IDENTITY_SOURCE.IDIR, displayName: 'test user', company: 'test company', @@ -110,7 +109,6 @@ describe('AccessRequestList', () => { username: 'testusername', userGuid: 'aaaa', email: 'email@email.com', - role: 2, identitySource: SYSTEM_IDENTITY_SOURCE.IDIR, displayName: 'test user', company: 'test company', @@ -147,7 +145,6 @@ describe('AccessRequestList', () => { username: 'testusername', userGuid: 'aaaa', email: 'email@email.com', - role: 2, identitySource: SYSTEM_IDENTITY_SOURCE.IDIR, displayName: 'test user', company: 'test company', @@ -211,7 +208,6 @@ describe('AccessRequestList', () => { username: 'testusername', userGuid: 'aaaa', email: 'email@email.com', - role: 2, identitySource: SYSTEM_IDENTITY_SOURCE.IDIR, displayName: 'test user', company: 'test company', @@ -239,8 +235,8 @@ describe('AccessRequestList', () => { expect(mockUseApi.admin.approveAccessRequest).toHaveBeenCalledWith(1, { displayName: 'test user', email: 'email@email.com', + roleIds: [], identitySource: 'IDIR', - roleIds: [2], userGuid: 'aaaa', userIdentifier: 'testusername' }); @@ -265,7 +261,6 @@ describe('AccessRequestList', () => { username: 'testusername', userGuid: 'aaaa', email: 'email@email.com', - role: 1, identitySource: SYSTEM_IDENTITY_SOURCE.IDIR, displayName: 'test user', company: 'test company', diff --git a/app/src/features/admin/users/AccessRequestList.tsx b/app/src/features/admin/users/AccessRequestList.tsx index 9044527cc1..fda4d33c94 100644 --- a/app/src/features/admin/users/AccessRequestList.tsx +++ b/app/src/features/admin/users/AccessRequestList.tsx @@ -14,11 +14,7 @@ import { AdministrativeActivityStatusType } from 'constants/misc'; import { DialogContext } from 'contexts/dialogContext'; import { APIError } from 'hooks/api/useAxios'; import { useBiohubApi } from 'hooks/useBioHubApi'; -import { - IAccessRequestDataObject, - IGetAccessRequestsListResponse, - IIDIRAccessRequestDataObject -} from 'interfaces/useAdminApi.interface'; +import { IGetAccessRequestsListResponse } from 'interfaces/useAdminApi.interface'; import { IGetAllCodeSetsResponse } from 'interfaces/useCodesApi.interface'; import { useContext, useState } from 'react'; import { getFormattedDate } from 'utils/Utils'; @@ -247,8 +243,7 @@ const AccessRequestList = (props: IAccessRequestListProps) => { component={{ initialValues: { ...ReviewAccessRequestFormInitialValues, - system_role: - (activeReview?.data as (IAccessRequestDataObject & IIDIRAccessRequestDataObject) | undefined)?.role ?? '' + system_role: '' }, validationSchema: ReviewAccessRequestFormYupSchema, element: activeReview ? ( diff --git a/app/src/features/admin/users/ActiveUsersList.tsx b/app/src/features/admin/users/ActiveUsersList.tsx index 1a78191b04..fb6f2338db 100644 --- a/app/src/features/admin/users/ActiveUsersList.tsx +++ b/app/src/features/admin/users/ActiveUsersList.tsx @@ -303,19 +303,34 @@ const ActiveUsersList = (props: IActiveUsersListProps) => { }); } catch (error) { const apiError = error as APIError; - dialogContext.setErrorDialog({ - open: true, - dialogTitle: AddSystemUserI18N.addUserErrorTitle, - dialogText: AddSystemUserI18N.addUserErrorText, - dialogError: apiError.message, - dialogErrorDetails: apiError.errors, - onClose: () => { - dialogContext.setErrorDialog({ open: false }); - }, - onOk: () => { - dialogContext.setErrorDialog({ open: false }); - } - }); + + if (apiError.status === 400) { + dialogContext.setErrorDialog({ + open: true, + dialogTitle: 'User already exists', + dialogText: 'One of the users you added already exists.', + onClose: () => { + dialogContext.setErrorDialog({ open: false }); + }, + onOk: () => { + dialogContext.setErrorDialog({ open: false }); + } + }); + } else { + dialogContext.setErrorDialog({ + open: true, + dialogTitle: AddSystemUserI18N.addUserErrorTitle, + dialogText: AddSystemUserI18N.addUserErrorText, + dialogError: apiError.message, + dialogErrorDetails: apiError.errors, + onClose: () => { + dialogContext.setErrorDialog({ open: false }); + }, + onOk: () => { + dialogContext.setErrorDialog({ open: false }); + } + }); + } } }; diff --git a/app/src/features/admin/users/ReviewAccessRequestForm.test.tsx b/app/src/features/admin/users/ReviewAccessRequestForm.test.tsx index 0dde419791..1c4c5a2c36 100644 --- a/app/src/features/admin/users/ReviewAccessRequestForm.test.tsx +++ b/app/src/features/admin/users/ReviewAccessRequestForm.test.tsx @@ -27,7 +27,6 @@ describe('ReviewAccessRequestForm', () => { email: 'test data email', identitySource: SYSTEM_IDENTITY_SOURCE.IDIR, displayName: 'test user', - role: 2, reason: 'reason for request' } }; diff --git a/app/src/interfaces/useAdminApi.interface.ts b/app/src/interfaces/useAdminApi.interface.ts index dfda377fa7..2f2140928d 100644 --- a/app/src/interfaces/useAdminApi.interface.ts +++ b/app/src/interfaces/useAdminApi.interface.ts @@ -1,5 +1,4 @@ export type IIDIRAccessRequestDataObject = { - role: number; reason: string; }; diff --git a/app/src/pages/access/AccessRequestPage.tsx b/app/src/pages/access/AccessRequestPage.tsx index 1fb7c85ae3..d8425daf58 100644 --- a/app/src/pages/access/AccessRequestPage.tsx +++ b/app/src/pages/access/AccessRequestPage.tsx @@ -146,7 +146,7 @@ export const AccessRequestPage: React.FC = () => { default: initialValues = IDIRRequestFormInitialValues; validationSchema = IDIRRequestFormYupSchema; - requestForm = ; + requestForm = ; } return ( diff --git a/app/src/pages/access/IDIRRequestForm.tsx b/app/src/pages/access/IDIRRequestForm.tsx index 9172f6beb0..d181fc000e 100644 --- a/app/src/pages/access/IDIRRequestForm.tsx +++ b/app/src/pages/access/IDIRRequestForm.tsx @@ -1,69 +1,24 @@ import Box from '@mui/material/Box'; -import FormControl from '@mui/material/FormControl'; -import FormHelperText from '@mui/material/FormHelperText'; -import Grid from '@mui/material/Grid'; -import InputLabel from '@mui/material/InputLabel'; -import MenuItem from '@mui/material/MenuItem'; -import Select from '@mui/material/Select'; import CustomTextField from 'components/fields/CustomTextField'; -import { useFormikContext } from 'formik'; import { IIDIRAccessRequestDataObject } from 'interfaces/useAdminApi.interface'; -import { IGetAllCodeSetsResponse } from 'interfaces/useCodesApi.interface'; -import React from 'react'; import yup from 'utils/YupSchema'; export const IDIRRequestFormInitialValues: IIDIRAccessRequestDataObject = { - role: '' as unknown as number, reason: '' }; export const IDIRRequestFormYupSchema = yup.object().shape({ - role: yup.string().required('Required'), reason: yup.string().max(300, 'Maximum 300 characters') }); -export interface IIDIRRequestFormProps { - codes?: IGetAllCodeSetsResponse; -} - /** * Access Request - IDIR request fields * * @return {*} */ -const IDIRRequestForm: React.FC = (props) => { - const { values, touched, errors, handleChange } = useFormikContext(); - const { codes } = props; - +const IDIRRequestForm = () => { return ( - - - - - Requested Role - - {errors.role} - - - - - diff --git a/database/src/migrations/2024062611000000_remove_duplicate_users.ts b/database/src/migrations/2024062611000000_remove_duplicate_users.ts new file mode 100644 index 0000000000..79e869cb23 --- /dev/null +++ b/database/src/migrations/2024062611000000_remove_duplicate_users.ts @@ -0,0 +1,249 @@ +import { Knex } from 'knex'; + +/** + * Fixes duplicate user records and references to duplicate user records + * + * - Removes references to duplicated users all tables referencing system_user_id + * - Removes duplicate records in system_user table based on (user_identifier, user_identity_source_id, record_end_date) as the unique key + * - Adds database constraints to prevent duplicates in system_user_id and administrative_activity + * + * @export + * @param {Knex} knex + * @return {*} {Promise} + */ +export async function up(knex: Knex): Promise { + await knex.raw(`--sql + + SET SEARCH_PATH='biohub'; + + ---------------------------------------------------------------------------------------- + -- Alter existing system_user_role data + ---------------------------------------------------------------------------------------- + + -- Update references to duplicated user IDs + WITH DeduplicatedUsers AS ( + SELECT + MIN(su.system_user_id) AS deduplicated_system_user_id, + su.user_identity_source_id, + su.user_identifier + FROM + system_user su + GROUP BY + su.user_identity_source_id, su.user_identifier, su.record_end_date + ) + UPDATE + system_user_role AS sur + SET + system_user_id = du.deduplicated_system_user_id + FROM + DeduplicatedUsers AS du + WHERE + sur.system_user_id IN ( + SELECT su.system_user_id + FROM system_user su + WHERE su.user_identity_source_id = du.user_identity_source_id + AND su.user_identifier = du.user_identifier + ) + AND sur.system_user_id NOT IN ( + SELECT deduplicated_system_user_id + FROM DeduplicatedUsers + ) + -- prevent updates that will introduce a duplicate and trigger a unique constraint error + AND NOT EXISTS ( + SELECT 1 + FROM system_user_role sur_check + WHERE sur_check.system_user_id = du.deduplicated_system_user_id + AND sur_check.system_role_id = sur.system_role_id + AND sur_check.system_user_role_id <> sur.system_user_role_id + ); + + ---------------------------------------------------------------------------------------- + -- Update references to duplicate user IDs in all tables with a system_user_id column + ---------------------------------------------------------------------------------------- + + -- Remove references to duplicated users in the project_participation table + WITH DeduplicatedUsers AS ( + SELECT MIN(system_user_id) AS deduplicated_system_user_id, + user_identity_source_id, + user_identifier + FROM system_user + GROUP BY user_identity_source_id, user_identifier, record_end_date + ) + UPDATE project_participation AS pp + SET system_user_id = dedup.deduplicated_system_user_id + FROM DeduplicatedUsers AS dedup + WHERE pp.system_user_id IN ( + SELECT system_user_id + FROM system_user + WHERE user_identity_source_id = dedup.user_identity_source_id + AND user_identifier = dedup.user_identifier + ) + AND pp.system_user_id NOT IN ( + SELECT deduplicated_system_user_id + FROM DeduplicatedUsers + ); + + -- Deduplicate audit_log records + WITH DeduplicatedUsers AS ( + SELECT MIN(system_user_id) AS deduplicated_system_user_id, + user_identity_source_id, + user_identifier + FROM system_user + GROUP BY user_identity_source_id, user_identifier, record_end_date + ) + UPDATE audit_log AS pp + SET system_user_id = dedup.deduplicated_system_user_id + FROM DeduplicatedUsers AS dedup + WHERE pp.system_user_id IN ( + SELECT system_user_id + FROM system_user + WHERE user_identity_source_id = dedup.user_identity_source_id + AND user_identifier = dedup.user_identifier + ) + AND pp.system_user_id NOT IN ( + SELECT deduplicated_system_user_id + FROM DeduplicatedUsers + ); + + -- Deduplicate webform_draft records + WITH DeduplicatedUsers AS ( + SELECT MIN(system_user_id) AS deduplicated_system_user_id, + user_identity_source_id, + user_identifier + FROM system_user + GROUP BY user_identity_source_id, user_identifier, record_end_date + ) + UPDATE webform_draft AS pp + SET system_user_id = dedup.deduplicated_system_user_id + FROM DeduplicatedUsers AS dedup + WHERE pp.system_user_id IN ( + SELECT system_user_id + FROM system_user + WHERE user_identity_source_id = dedup.user_identity_source_id + AND LOWER(user_identifier) = LOWER(dedup.user_identifier) + ) + AND pp.system_user_id NOT IN ( + SELECT deduplicated_system_user_id + FROM DeduplicatedUsers + ); + + -- Deduplicate user_user_group records + WITH DeduplicatedUsers AS ( + SELECT MIN(system_user_id) AS deduplicated_system_user_id, + user_identity_source_id, + user_identifier + FROM system_user + GROUP BY user_identity_source_id, user_identifier, record_end_date + ) + UPDATE user_user_group AS pp + SET system_user_id = dedup.deduplicated_system_user_id + FROM DeduplicatedUsers AS dedup + WHERE pp.system_user_id IN ( + SELECT system_user_id + FROM system_user + WHERE user_identity_source_id = dedup.user_identity_source_id + AND LOWER(user_identifier) = LOWER(dedup.user_identifierLOWER( + ) + AND pp.system_user_id NOT IN ( + SELECT deduplicated_system_user_id + FROM DeduplicatedUsers + ); + + -- Deduplicate grouping_participation records + WITH DeduplicatedUsers AS ( + SELECT MIN(system_user_id) AS deduplicated_system_user_id, + user_identity_source_id, + user_identifier + FROM system_user + GROUP BY user_identity_source_id, user_identifier, record_end_date + ) + UPDATE grouping_participation AS pp + SET system_user_id = dedup.deduplicated_system_user_id + FROM DeduplicatedUsers AS dedup + WHERE pp.system_user_id IN ( + SELECT system_user_id + FROM system_user + WHERE user_identity_source_id = dedup.user_identity_source_id + AND LOWER(user_identifier) = LOWER(dedup.user_identifier) + ) + AND pp.system_user_id NOT IN ( + SELECT deduplicated_system_user_id + FROM DeduplicatedUsers + ); + + -- Deduplicate survey_participation records + WITH DeduplicatedUsers AS ( + SELECT MIN(system_user_id) AS deduplicated_system_user_id, + user_identity_source_id, + user_identifier + FROM system_user + GROUP BY user_identity_source_id, user_identifier, record_end_date + ) + UPDATE survey_participation AS pp + SET system_user_id = dedup.deduplicated_system_user_id + FROM DeduplicatedUsers AS dedup + WHERE pp.system_user_id IN ( + SELECT system_user_id + FROM system_user + WHERE user_identity_source_id = dedup.user_identity_source_id + AND LOWER(user_identifier) = LOWER(dedup.user_identifier) + ) + AND pp.system_user_id NOT IN ( + SELECT deduplicated_system_user_id + FROM DeduplicatedUsers + ); + + ---------------------------------------------------------------------------------------- + -- Delete duplicate records + ---------------------------------------------------------------------------------------- + + -- Remove any duplicate system user role records + DELETE FROM system_user_role + WHERE system_role_id NOT IN ( + SELECT MIN(system_user_role_id) + FROM system_user_role + GROUP BY system_user_id, system_role_id + ); + + -- Remove any duplicate user records + DELETE FROM system_user + WHERE system_user_id NOT IN ( + SELECT MIN(system_user_id) + FROM system_user + GROUP BY user_identity_source_id, user_identifier, record_end_date + ); + + -- Remove any duplicate access request records + DELETE FROM administrative_activity + WHERE administrative_activity_id NOT IN ( + SELECT MIN(administrative_activity_id) + FROM administrative_activity + GROUP BY administrative_activity_type_id, reported_system_user_id + ); + + ---------------------------------------------------------------------------------------- + -- Add constraints and update views + ---------------------------------------------------------------------------------------- + + -- Add constraints and update views + SET SEARCH_PATH='biohub,biohub_dapi_v1'; + + -- Drop views + DROP VIEW IF EXISTS biohub_dapi_v1.system_user; + DROP VIEW IF EXISTS biohub_dapi_v1.administrative_activity; + + -- Add unique constraint to prevent duplicate users and access requests + ALTER TABLE biohub.system_user ADD CONSTRAINT system_user_uk2 UNIQUE (LOWER(user_identifier), user_identity_source_id, record_end_date); + ALTER TABLE biohub.administrative_activity ADD CONSTRAINT administrative_activity_uk1 UNIQUE (reported_system_user_id, administrative_activity_type_id); + + -- Recreate the view + CREATE OR REPLACE VIEW biohub_dapi_v1.system_user AS SELECT * FROM biohub.system_user; + CREATE OR REPLACE VIEW biohub_dapi_v1.administrative_activity AS SELECT * FROM biohub.administrative_activity; + + + `); +} + +export async function down(knex: Knex): Promise { + await knex.raw(``); +} diff --git a/database/src/procedures/api_patch_system_user.ts b/database/src/procedures/api_patch_system_user.ts index f85970c8a6..381695cc20 100644 --- a/database/src/procedures/api_patch_system_user.ts +++ b/database/src/procedures/api_patch_system_user.ts @@ -15,7 +15,7 @@ import { Knex } from 'knex'; * @param {Knex} knex * @return {*} {Promise} */ -export async function up(knex: Knex): Promise { +export async function seed(knex: Knex): Promise { await knex.raw(`--sql set search_path=biohub; @@ -103,7 +103,3 @@ export async function up(knex: Knex): Promise { COMMENT ON FUNCTION api_patch_system_user(varchar, varchar, varchar, varchar, varchar, varchar, varchar, varchar) IS 'Updates a system_user record if any of the incoming values are not the same as the existing values.'; `); } - -export async function down(knex: Knex): Promise { - await knex.raw(``); -} diff --git a/database/src/procedures/api_set_context.ts b/database/src/procedures/api_set_context.ts index dc5e4cd48c..f37a5969d5 100644 --- a/database/src/procedures/api_set_context.ts +++ b/database/src/procedures/api_set_context.ts @@ -8,7 +8,7 @@ import { Knex } from 'knex'; * @param {Knex} knex * @return {*} {Promise} */ -export async function up(knex: Knex): Promise { +export async function seed(knex: Knex): Promise { await knex.raw(` SET search_path = 'biohub'; @@ -44,7 +44,3 @@ export async function up(knex: Knex): Promise { $$; `); } - -export async function down(knex: Knex): Promise { - await knex.raw(``); -} From b1a86a3165b1fdf273072681e849ea645188b700 Mon Sep 17 00:00:00 2001 From: Macgregor Aubertin-Young Date: Thu, 27 Jun 2024 14:17:27 -0700 Subject: [PATCH 04/12] fix typo by removing lower from constraint --- .../2024062611000000_remove_duplicate_users.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) rename database/{src/migrations => }/2024062611000000_remove_duplicate_users.ts (96%) diff --git a/database/src/migrations/2024062611000000_remove_duplicate_users.ts b/database/2024062611000000_remove_duplicate_users.ts similarity index 96% rename from database/src/migrations/2024062611000000_remove_duplicate_users.ts rename to database/2024062611000000_remove_duplicate_users.ts index 79e869cb23..3701ce9835 100644 --- a/database/src/migrations/2024062611000000_remove_duplicate_users.ts +++ b/database/2024062611000000_remove_duplicate_users.ts @@ -120,7 +120,7 @@ export async function up(knex: Knex): Promise { SELECT system_user_id FROM system_user WHERE user_identity_source_id = dedup.user_identity_source_id - AND LOWER(user_identifier) = LOWER(dedup.user_identifier) + AND user_identifier = dedup.user_identifier ) AND pp.system_user_id NOT IN ( SELECT deduplicated_system_user_id @@ -142,7 +142,7 @@ export async function up(knex: Knex): Promise { SELECT system_user_id FROM system_user WHERE user_identity_source_id = dedup.user_identity_source_id - AND LOWER(user_identifier) = LOWER(dedup.user_identifierLOWER( + AND user_identifier = dedup.user_identifier ) AND pp.system_user_id NOT IN ( SELECT deduplicated_system_user_id @@ -164,7 +164,7 @@ export async function up(knex: Knex): Promise { SELECT system_user_id FROM system_user WHERE user_identity_source_id = dedup.user_identity_source_id - AND LOWER(user_identifier) = LOWER(dedup.user_identifier) + AND user_identifier = dedup.user_identifier ) AND pp.system_user_id NOT IN ( SELECT deduplicated_system_user_id @@ -186,7 +186,7 @@ export async function up(knex: Knex): Promise { SELECT system_user_id FROM system_user WHERE user_identity_source_id = dedup.user_identity_source_id - AND LOWER(user_identifier) = LOWER(dedup.user_identifier) + AND user_identifier = dedup.user_identifier ) AND pp.system_user_id NOT IN ( SELECT deduplicated_system_user_id @@ -233,7 +233,7 @@ export async function up(knex: Knex): Promise { DROP VIEW IF EXISTS biohub_dapi_v1.administrative_activity; -- Add unique constraint to prevent duplicate users and access requests - ALTER TABLE biohub.system_user ADD CONSTRAINT system_user_uk2 UNIQUE (LOWER(user_identifier), user_identity_source_id, record_end_date); + ALTER TABLE biohub.system_user ADD CONSTRAINT system_user_uk2 UNIQUE (user_identifier, user_identity_source_id, record_end_date); ALTER TABLE biohub.administrative_activity ADD CONSTRAINT administrative_activity_uk1 UNIQUE (reported_system_user_id, administrative_activity_type_id); -- Recreate the view From 3f0aa2cfa8c82c730e3d11eb6b3cb720ea229a47 Mon Sep 17 00:00:00 2001 From: Macgregor Aubertin-Young Date: Thu, 27 Jun 2024 21:04:27 -0700 Subject: [PATCH 05/12] ensure system user record includes guid --- ...2024062611000000_remove_duplicate_users.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) rename database/{ => src/migrations}/2024062611000000_remove_duplicate_users.ts (91%) diff --git a/database/2024062611000000_remove_duplicate_users.ts b/database/src/migrations/2024062611000000_remove_duplicate_users.ts similarity index 91% rename from database/2024062611000000_remove_duplicate_users.ts rename to database/src/migrations/2024062611000000_remove_duplicate_users.ts index 3701ce9835..ba8de8337f 100644 --- a/database/2024062611000000_remove_duplicate_users.ts +++ b/database/src/migrations/2024062611000000_remove_duplicate_users.ts @@ -57,6 +57,34 @@ export async function up(knex: Knex): Promise { AND sur_check.system_user_role_id <> sur.system_user_role_id ); + ---------------------------------------------------------------------------------------- + -- Update system_user to include guid + ---------------------------------------------------------------------------------------- + + -- Update references to duplicated user IDs + WITH DeduplicatedUsers AS ( + SELECT + MIN(su.system_user_id) AS deduplicated_system_user_id, + MAX(su.user_guid) as user_guid, + su.user_identity_source_id, + su.user_identifier, + su.record_end_date + FROM + system_user su + GROUP BY + su.user_identity_source_id, su.user_identifier, su.record_end_date + ) + UPDATE + system_user as su + SET + user_guid = du.user_guid, + record_end_date = NULL + FROM + DeduplicatedUsers du + WHERE + su.system_user_id = du.deduplicated_system_user_id + AND su.user_guid IS NULL; + ---------------------------------------------------------------------------------------- -- Update references to duplicate user IDs in all tables with a system_user_id column ---------------------------------------------------------------------------------------- From a6468555e42dc419d9e0f14aa6f32905e265e170 Mon Sep 17 00:00:00 2001 From: Nick Phura Date: Fri, 19 Jul 2024 20:29:20 -0700 Subject: [PATCH 06/12] Remove some unused references to the codes context. Update migration to combine queries into one pieces of sql. --- api/src/paths/user/add.test.ts | 6 +- api/src/paths/user/add.ts | 10 +- .../admin/users/AccessRequestList.test.tsx | 2 +- .../admin/users/AccessRequestList.tsx | 5 +- .../features/admin/users/ActiveUsersList.tsx | 4 +- .../pages/access/AccessRequestPage.test.tsx | 24 - app/src/pages/access/AccessRequestPage.tsx | 4 - ...2024062611000000_remove_duplicate_users.ts | 418 ++++++++---------- .../src/procedures/api_patch_system_user.ts | 14 +- database/src/seeds/01_db_system_users.ts | 70 --- 10 files changed, 196 insertions(+), 361 deletions(-) diff --git a/api/src/paths/user/add.test.ts b/api/src/paths/user/add.test.ts index 55f598527f..d77e563e44 100644 --- a/api/src/paths/user/add.test.ts +++ b/api/src/paths/user/add.test.ts @@ -8,7 +8,7 @@ import { SystemUser } from '../../repositories/user-repository'; import { UserService } from '../../services/user-service'; import * as keycloakUtils from '../../utils/keycloak-utils'; import { getMockDBConnection, getRequestHandlerMocks } from '../../__mocks__/db'; -import * as user from './add'; +import { addSystemRoleUser } from './add'; chai.use(sinonChai); @@ -56,7 +56,7 @@ describe('user', () => { const adduserSystemRolesStub = sinon.stub(UserService.prototype, 'addUserSystemRoles'); - const requestHandler = user.addSystemRoleUser(); + const requestHandler = addSystemRoleUser(); await requestHandler(mockReq, mockRes, mockNext); @@ -102,7 +102,7 @@ describe('user', () => { const adduserSystemRolesStub = sinon.stub(UserService.prototype, 'addUserSystemRoles'); - const requestHandler = user.addSystemRoleUser(); + const requestHandler = addSystemRoleUser(); await requestHandler(mockReq, mockRes, mockNext); expect(getUserByIdentifierStub).to.have.been.calledOnce; diff --git a/api/src/paths/user/add.ts b/api/src/paths/user/add.ts index f457df5b68..47418d16f6 100644 --- a/api/src/paths/user/add.ts +++ b/api/src/paths/user/add.ts @@ -3,6 +3,7 @@ import { Operation } from 'express-openapi'; import { SOURCE_SYSTEM, SYSTEM_IDENTITY_SOURCE } from '../../constants/database'; import { SYSTEM_ROLE } from '../../constants/roles'; import { getDBConnection, getServiceClientDBConnection } from '../../database/db'; +import { HTTP409 } from '../../errors/http-error'; import { authorizeRequestHandler } from '../../request-handlers/security/authorization'; import { UserService } from '../../services/user-service'; import { getKeycloakSource } from '../../utils/keycloak-utils'; @@ -104,6 +105,9 @@ POST.apiDoc = { 403: { $ref: '#/components/responses/403' }, + 409: { + $ref: '#/components/responses/409' + }, 500: { $ref: '#/components/responses/500' }, @@ -145,9 +149,9 @@ export function addSystemRoleUser(): RequestHandler { // If user already exists, do nothing and return early const user = await userService.getUserByIdentifier(userIdentifier, identitySource); + if (user) { - await connection.commit(); - return res.status(400).send('That user has already been added.'); + throw new HTTP409('Failed to add user. User with matching identifier already exists.'); } // This function also verifies that the user exists, but we explicitly check above to return a useful error message. @@ -173,7 +177,7 @@ export function addSystemRoleUser(): RequestHandler { return res.status(200).send(); } catch (error) { - defaultLog.error({ label: 'getUser', message: 'error', error }); + defaultLog.error({ label: 'addSystemRoleUser', message: 'error', error }); await connection.rollback(); throw error; } finally { diff --git a/app/src/features/admin/users/AccessRequestList.test.tsx b/app/src/features/admin/users/AccessRequestList.test.tsx index da0b03f7af..98af5b1f3e 100644 --- a/app/src/features/admin/users/AccessRequestList.test.tsx +++ b/app/src/features/admin/users/AccessRequestList.test.tsx @@ -235,8 +235,8 @@ describe('AccessRequestList', () => { expect(mockUseApi.admin.approveAccessRequest).toHaveBeenCalledWith(1, { displayName: 'test user', email: 'email@email.com', - roleIds: [], identitySource: 'IDIR', + roleIds: [], userGuid: 'aaaa', userIdentifier: 'testusername' }); diff --git a/app/src/features/admin/users/AccessRequestList.tsx b/app/src/features/admin/users/AccessRequestList.tsx index fda4d33c94..fbd0ac607a 100644 --- a/app/src/features/admin/users/AccessRequestList.tsx +++ b/app/src/features/admin/users/AccessRequestList.tsx @@ -241,10 +241,7 @@ const AccessRequestList = (props: IAccessRequestListProps) => { onDeny={handleReviewDialogDeny} onApprove={handleReviewDialogApprove} component={{ - initialValues: { - ...ReviewAccessRequestFormInitialValues, - system_role: '' - }, + initialValues: ReviewAccessRequestFormInitialValues, validationSchema: ReviewAccessRequestFormYupSchema, element: activeReview ? ( { } catch (error) { const apiError = error as APIError; - if (apiError.status === 400) { + if (apiError.status === 409) { dialogContext.setErrorDialog({ open: true, - dialogTitle: 'User already exists', + dialogTitle: 'Failed to create users', dialogText: 'One of the users you added already exists.', onClose: () => { dialogContext.setErrorDialog({ open: false }); diff --git a/app/src/pages/access/AccessRequestPage.test.tsx b/app/src/pages/access/AccessRequestPage.test.tsx index 75d078b5a5..3e6ef3ca7c 100644 --- a/app/src/pages/access/AccessRequestPage.test.tsx +++ b/app/src/pages/access/AccessRequestPage.test.tsx @@ -15,9 +15,6 @@ jest.mock('../../hooks/useBioHubApi'); const mockBiohubApi = useBiohubApi as jest.Mock; const mockUseApi = { - codes: { - getAllCodeSets: jest.fn, []>() - }, admin: { createAdministrativeActivity: jest.fn() } @@ -42,7 +39,6 @@ const renderContainer = () => { describe('AccessRequestPage', () => { beforeEach(() => { mockBiohubApi.mockImplementation(() => mockUseApi); - mockUseApi.codes.getAllCodeSets.mockClear(); mockUseApi.admin.createAdministrativeActivity.mockClear(); }); @@ -54,10 +50,6 @@ describe('AccessRequestPage', () => { const history = createMemoryHistory(); it('should call the auth signoutRedirect function', async () => { - mockUseApi.codes.getAllCodeSets.mockResolvedValue({ - system_roles: [{ id: 1, name: 'Creator' }] - }); - const signoutRedirectStub = jest.fn(); const authState = getMockAuthState({ @@ -90,10 +82,6 @@ describe('AccessRequestPage', () => { }); it.skip('processes a successful request submission', async () => { - mockUseApi.codes.getAllCodeSets.mockResolvedValue({ - system_roles: [{ id: 1, name: 'Creator' }] - }); - mockUseApi.admin.createAdministrativeActivity.mockResolvedValue({ id: 1 }); @@ -118,10 +106,6 @@ describe('AccessRequestPage', () => { }); it.skip('takes the user to the request-submitted page immediately if they already have an access request', async () => { - mockUseApi.codes.getAllCodeSets.mockResolvedValue({ - system_roles: [{ id: 1, name: 'Creator' }] - }); - const authState = getMockAuthState({ base: SystemAdminAuthState }); render( @@ -140,10 +124,6 @@ describe('AccessRequestPage', () => { }); it.skip('shows error dialog with api error message when submission fails', async () => { - mockUseApi.codes.getAllCodeSets.mockResolvedValue({ - system_roles: [{ id: 1, name: 'Creator' }] - }); - mockUseApi.admin.createAdministrativeActivity = jest.fn(() => Promise.reject(new Error('API Error is Here'))); const { getByText, getAllByRole, getByRole, queryByText } = renderContainer(); @@ -172,10 +152,6 @@ describe('AccessRequestPage', () => { }); it.skip('shows error dialog with default error message when response from createAdministrativeActivity is invalid', async () => { - mockUseApi.codes.getAllCodeSets.mockResolvedValue({ - system_roles: [{ id: 1, name: 'Creator' }] - }); - mockUseApi.admin.createAdministrativeActivity.mockResolvedValue({ id: null }); diff --git a/app/src/pages/access/AccessRequestPage.tsx b/app/src/pages/access/AccessRequestPage.tsx index d8425daf58..2c21f74641 100644 --- a/app/src/pages/access/AccessRequestPage.tsx +++ b/app/src/pages/access/AccessRequestPage.tsx @@ -12,7 +12,6 @@ import { Formik } from 'formik'; import { APIError } from 'hooks/api/useAxios'; import { useAuthStateContext } from 'hooks/useAuthStateContext'; import { useBiohubApi } from 'hooks/useBioHubApi'; -import useDataLoader from 'hooks/useDataLoader'; import { IBCeIDBasicAccessRequestDataObject, IBCeIDBusinessAccessRequestDataObject, @@ -67,9 +66,6 @@ export const AccessRequestPage: React.FC = () => { const [isSubmittingRequest, setIsSubmittingRequest] = useState(false); - const codesDataLoader = useDataLoader(() => biohubApi.codes.getAllCodeSets()); - codesDataLoader.load(); - const showAccessRequestErrorDialog = (textDialogProps?: Partial) => { dialogContext.setErrorDialog({ ...defaultErrorDialogProps, diff --git a/database/src/migrations/2024062611000000_remove_duplicate_users.ts b/database/src/migrations/2024062611000000_remove_duplicate_users.ts index ba8de8337f..894521ea2b 100644 --- a/database/src/migrations/2024062611000000_remove_duplicate_users.ts +++ b/database/src/migrations/2024062611000000_remove_duplicate_users.ts @@ -1,11 +1,24 @@ import { Knex } from 'knex'; /** - * Fixes duplicate user records and references to duplicate user records + * Fixes duplicate system_user_ids AND references to duplicate system_user_ids * - * - Removes references to duplicated users all tables referencing system_user_id - * - Removes duplicate records in system_user table based on (user_identifier, user_identity_source_id, record_end_date) as the unique key - * - Adds database constraints to prevent duplicates in system_user_id and administrative_activity + * Updates the following tables: + * - system_user: Update/end-dates duplicate system_user records. + * - system_user_role: Delete duplicate system_user_role records. + * - project_participation: Update system_user_id to the canonical system_user_id, and delete duplicate records. + * - survey_participation: Update system_user_id to the canonical system_user_id, and delete duplicate records. + * - webform_draft: Update system_user_id to the canonical system_user_id. + * - administrative_activity: Delete duplicate administrative_activity records. + * + * Updates/fixes several constraints: + * - system_user_nuk1: Don't allow more than 1 active record with the same user_guid. + * - system_user_nuk2: Don't allow more than 1 active record with the same user_identifier (case-insensitive) AND user_identity_source_id. + * + * Does not update the following tables: + * - audit_log: This table tracks the history of all changes to the database, including changes from this migration. + * - user_user_group: No data exists in this table at the time of writing. + * - grouping_participation: No data exists in this table at the time of writing. * * @export * @param {Knex} knex @@ -13,262 +26,191 @@ import { Knex } from 'knex'; */ export async function up(knex: Knex): Promise { await knex.raw(`--sql - - SET SEARCH_PATH='biohub'; - ---------------------------------------------------------------------------------------- - -- Alter existing system_user_role data + -- Drop existing constraints ---------------------------------------------------------------------------------------- - -- Update references to duplicated user IDs - WITH DeduplicatedUsers AS ( - SELECT - MIN(su.system_user_id) AS deduplicated_system_user_id, - su.user_identity_source_id, - su.user_identifier - FROM - system_user su - GROUP BY - su.user_identity_source_id, su.user_identifier, su.record_end_date - ) - UPDATE - system_user_role AS sur - SET - system_user_id = du.deduplicated_system_user_id - FROM - DeduplicatedUsers AS du - WHERE - sur.system_user_id IN ( - SELECT su.system_user_id - FROM system_user su - WHERE su.user_identity_source_id = du.user_identity_source_id - AND su.user_identifier = du.user_identifier - ) - AND sur.system_user_id NOT IN ( - SELECT deduplicated_system_user_id - FROM DeduplicatedUsers - ) - -- prevent updates that will introduce a duplicate and trigger a unique constraint error - AND NOT EXISTS ( - SELECT 1 - FROM system_user_role sur_check - WHERE sur_check.system_user_id = du.deduplicated_system_user_id - AND sur_check.system_role_id = sur.system_role_id - AND sur_check.system_user_role_id <> sur.system_user_role_id - ); + SET SEARCH_PATH = 'biohub'; - ---------------------------------------------------------------------------------------- - -- Update system_user to include guid - ---------------------------------------------------------------------------------------- - - -- Update references to duplicated user IDs - WITH DeduplicatedUsers AS ( - SELECT - MIN(su.system_user_id) AS deduplicated_system_user_id, - MAX(su.user_guid) as user_guid, - su.user_identity_source_id, - su.user_identifier, - su.record_end_date - FROM - system_user su - GROUP BY - su.user_identity_source_id, su.user_identifier, su.record_end_date - ) - UPDATE - system_user as su - SET - user_guid = du.user_guid, - record_end_date = NULL - FROM - DeduplicatedUsers du - WHERE - su.system_user_id = du.deduplicated_system_user_id - AND su.user_guid IS NULL; + DROP INDEX IF EXISTS system_user_uk1; + DROP INDEX IF EXISTS system_user_nuk1; ---------------------------------------------------------------------------------------- - -- Update references to duplicate user IDs in all tables with a system_user_id column + -- Find AND migrate duplicate system_user_ids ---------------------------------------------------------------------------------------- - -- Remove references to duplicated users in the project_participation table - WITH DeduplicatedUsers AS ( - SELECT MIN(system_user_id) AS deduplicated_system_user_id, - user_identity_source_id, - user_identifier - FROM system_user - GROUP BY user_identity_source_id, user_identifier, record_end_date - ) - UPDATE project_participation AS pp - SET system_user_id = dedup.deduplicated_system_user_id - FROM DeduplicatedUsers AS dedup - WHERE pp.system_user_id IN ( - SELECT system_user_id - FROM system_user - WHERE user_identity_source_id = dedup.user_identity_source_id - AND user_identifier = dedup.user_identifier - ) - AND pp.system_user_id NOT IN ( - SELECT deduplicated_system_user_id - FROM DeduplicatedUsers - ); - - -- Deduplicate audit_log records - WITH DeduplicatedUsers AS ( - SELECT MIN(system_user_id) AS deduplicated_system_user_id, - user_identity_source_id, - user_identifier - FROM system_user - GROUP BY user_identity_source_id, user_identifier, record_end_date - ) - UPDATE audit_log AS pp - SET system_user_id = dedup.deduplicated_system_user_id - FROM DeduplicatedUsers AS dedup - WHERE pp.system_user_id IN ( - SELECT system_user_id - FROM system_user - WHERE user_identity_source_id = dedup.user_identity_source_id - AND user_identifier = dedup.user_identifier - ) - AND pp.system_user_id NOT IN ( - SELECT deduplicated_system_user_id - FROM DeduplicatedUsers - ); - - -- Deduplicate webform_draft records - WITH DeduplicatedUsers AS ( - SELECT MIN(system_user_id) AS deduplicated_system_user_id, - user_identity_source_id, - user_identifier - FROM system_user - GROUP BY user_identity_source_id, user_identifier, record_end_date - ) - UPDATE webform_draft AS pp - SET system_user_id = dedup.deduplicated_system_user_id - FROM DeduplicatedUsers AS dedup - WHERE pp.system_user_id IN ( - SELECT system_user_id - FROM system_user - WHERE user_identity_source_id = dedup.user_identity_source_id - AND user_identifier = dedup.user_identifier - ) - AND pp.system_user_id NOT IN ( - SELECT deduplicated_system_user_id - FROM DeduplicatedUsers - ); - - -- Deduplicate user_user_group records - WITH DeduplicatedUsers AS ( - SELECT MIN(system_user_id) AS deduplicated_system_user_id, + WITH + -- Get all system_user records with a unique user_identifier (case-insensitive) and user_identity_source_id, + -- preferring the lowest system_user_id WHERE record_end_date is null + w_system_user_1 AS ( + SELECT + DISTINCT ON ( + LOWER(user_identifier), + user_identity_source_id + ) + system_user_id, user_identity_source_id, - user_identifier - FROM system_user - GROUP BY user_identity_source_id, user_identifier, record_end_date - ) - UPDATE user_user_group AS pp - SET system_user_id = dedup.deduplicated_system_user_id - FROM DeduplicatedUsers AS dedup - WHERE pp.system_user_id IN ( - SELECT system_user_id - FROM system_user - WHERE user_identity_source_id = dedup.user_identity_source_id - AND user_identifier = dedup.user_identifier - ) - AND pp.system_user_id NOT IN ( - SELECT deduplicated_system_user_id - FROM DeduplicatedUsers - ); - - -- Deduplicate grouping_participation records - WITH DeduplicatedUsers AS ( - SELECT MIN(system_user_id) AS deduplicated_system_user_id, + LOWER(user_identifier) AS user_identifier + FROM + system_user + ORDER BY + LOWER(user_identifier), user_identity_source_id, - user_identifier - FROM system_user - GROUP BY user_identity_source_id, user_identifier, record_end_date - ) - UPDATE grouping_participation AS pp - SET system_user_id = dedup.deduplicated_system_user_id - FROM DeduplicatedUsers AS dedup - WHERE pp.system_user_id IN ( - SELECT system_user_id - FROM system_user - WHERE user_identity_source_id = dedup.user_identity_source_id - AND user_identifier = dedup.user_identifier - ) - AND pp.system_user_id NOT IN ( - SELECT deduplicated_system_user_id - FROM DeduplicatedUsers - ); - - -- Deduplicate survey_participation records - WITH DeduplicatedUsers AS ( - SELECT MIN(system_user_id) AS deduplicated_system_user_id, + CASE WHEN record_end_date IS NULL THEN 0 ELSE 1 END, + system_user_id + ), + w_system_user_2 AS ( + -- Get all system_user records with a unique user_identifier (case-insensitive) and user_identity_source_id, + -- aggregating all additional duplicate system_user_ids into an array + SELECT + LOWER(system_user.user_identifier) AS user_identifier, user_identity_source_id, - user_identifier - FROM system_user - GROUP BY user_identity_source_id, user_identifier, record_end_date - ) - UPDATE survey_participation AS pp - SET system_user_id = dedup.deduplicated_system_user_id - FROM DeduplicatedUsers AS dedup - WHERE pp.system_user_id IN ( - SELECT system_user_id - FROM system_user - WHERE user_identity_source_id = dedup.user_identity_source_id - AND user_identifier = dedup.user_identifier + array_agg(system_user.system_user_id) duplicate_system_user_ids + FROM + system_user + GROUP BY + LOWER(system_user.user_identifier), + system_user.user_identity_source_id + ), + w_system_user_3 AS ( + -- Combine the two previous CTEs to get the canonical system_user_id to use when there are duplicate users, and + -- and a list of all system_user_ids that are duplicates (which does not include the canonical system_user_id). + SELECT + w_system_user_1.system_user_id, + w_system_user_1.user_identity_source_id, + array_remove(w_system_user_2.duplicate_system_user_ids, w_system_user_1.system_user_id) AS duplicate_system_user_ids + FROM + w_system_user_1 + left join + w_system_user_2 + ON w_system_user_1.user_identifier = w_system_user_2.user_identifier + AND w_system_user_1.user_identity_source_id = w_system_user_2.user_identity_source_id + ), + -- Update duplicate system_user_ids in the project_participation table to the canonical system_user_id, ignoring + -- records where the canonical system_user_id is already in use. + w_update_project_participation AS ( + UPDATE project_participation AS pp + SET system_user_id = wsu3.system_user_id + FROM w_system_user_3 AS wsu3 + WHERE pp.system_user_id = ANY(wsu3.duplicate_system_user_ids) + AND NOT EXISTS ( + SELECT 1 + FROM project_participation AS pp_check + WHERE pp_check.system_user_id = wsu3.system_user_id + AND pp_check.project_id = pp.project_id + ) + ), + -- Delete all remaining references to duplicate system_user_ids in the project_participation table, as long as + -- there exists another record for the same project with the canonical system_user_id (which should be all of them + -- after the previous CTE ran). + w_delete_project_participation AS ( + DELETE FROM project_participation + WHERE EXISTS ( + SELECT 1 + FROM w_system_user_3 AS wsu3 + WHERE project_participation.system_user_id = ANY(wsu3.duplicate_system_user_ids) + AND project_participation.project_id IN ( + SELECT project_id + FROM project_participation AS pp_check + WHERE pp_check.system_user_id = wsu3.system_user_id + ) + ) + ), + -- Update duplicate system_user_ids in the survey_participation table to the canonical system_user_id, ignoring + -- records where the canonical system_user_id is already in use. + w_update_survey_participation AS ( + UPDATE survey_participation AS sp + SET system_user_id = wsu3.system_user_id + FROM w_system_user_3 AS wsu3 + WHERE sp.system_user_id = ANY(wsu3.duplicate_system_user_ids) + AND NOT EXISTS ( + SELECT 1 + FROM survey_participation AS sp_check + WHERE sp_check.system_user_id = wsu3.system_user_id + AND sp_check.survey_id = sp.survey_id + ) + ), + -- Delete all remaining references to duplicate system_user_ids in the survey_participation table, as long as + -- there exists another record for the same survey with the canonical system_user_id (which should be all of them + -- after the previous CTE ran). + w_delete_survey_participation AS ( + DELETE FROM survey_participation + WHERE EXISTS ( + SELECT 1 + FROM w_system_user_3 AS wsu3 + WHERE survey_participation.system_user_id = ANY(wsu3.duplicate_system_user_ids) + AND survey_participation.survey_id IN ( + SELECT survey_id + FROM survey_participation AS sp_check + WHERE sp_check.system_user_id = wsu3.system_user_id + ) + ) + ), + -- Update duplicate system_user_ids in the webform_draft table to the canonical system_user_id + w_end_date_duplicate_webform_draft as ( + UPDATE webform_draft + SET + system_user_id = wsu3.system_user_id + FROM w_system_user_3 wsu3 + WHERE webform_draft.system_user_id = ANY(wsu3.duplicate_system_user_ids) + ), + -- Delete duplicate system_user_role records for duplicate system_user_ids + w_end_date_duplicate_system_user_role AS ( + DELETE FROM system_user_role + USING w_system_user_3 wsu3 + WHERE system_user_role.system_user_id = ANY(wsu3.duplicate_system_user_ids) + ), + -- End date all duplicate system_user records for duplicate system_user_ids + w_end_date_duplicate_system_user AS ( + UPDATE system_user su + SET + record_end_date = NOW(), + notes = 'Duplicate user record; merged into system_user_id ' || wsu3.system_user_id || '.' + FROM w_system_user_3 wsu3 + WHERE su.system_user_id = ANY(wsu3.duplicate_system_user_ids) ) - AND pp.system_user_id NOT IN ( - SELECT deduplicated_system_user_id - FROM DeduplicatedUsers - ); + -- Return the combined results of the original CTEs (have to select something to run the query) + SELECT * FROM w_system_user_3; ---------------------------------------------------------------------------------------- - -- Delete duplicate records + -- Additional cleanup ---------------------------------------------------------------------------------------- - -- Remove any duplicate system user role records - DELETE FROM system_user_role - WHERE system_role_id NOT IN ( - SELECT MIN(system_user_role_id) - FROM system_user_role - GROUP BY system_user_id, system_role_id - ); - - -- Remove any duplicate user records - DELETE FROM system_user - WHERE system_user_id NOT IN ( - SELECT MIN(system_user_id) - FROM system_user - GROUP BY user_identity_source_id, user_identifier, record_end_date - ); - - -- Remove any duplicate access request records + -- Remove any duplicate access request records, keeping the most recently updated or created record for each reported_system_user_id. + WITH w_ranked_administrative_activity AS ( + SELECT + administrative_activity_id, + reported_system_user_id, + create_date, + update_date, + ROW_NUMBER() OVER ( + PARTITION BY reported_system_user_id + ORDER BY + COALESCE(update_date, create_date) DESC + ) AS rank + FROM + administrative_activity + ) DELETE FROM administrative_activity - WHERE administrative_activity_id NOT IN ( - SELECT MIN(administrative_activity_id) - FROM administrative_activity - GROUP BY administrative_activity_type_id, reported_system_user_id - ); + WHERE + administrative_activity_id IN ( + SELECT + administrative_activity_id + FROM + w_ranked_administrative_activity + WHERE + rank > 1 + ); ---------------------------------------------------------------------------------------- - -- Add constraints and update views + -- Add updated constraints ---------------------------------------------------------------------------------------- - -- Add constraints and update views - SET SEARCH_PATH='biohub,biohub_dapi_v1'; - - -- Drop views - DROP VIEW IF EXISTS biohub_dapi_v1.system_user; - DROP VIEW IF EXISTS biohub_dapi_v1.administrative_activity; - - -- Add unique constraint to prevent duplicate users and access requests - ALTER TABLE biohub.system_user ADD CONSTRAINT system_user_uk2 UNIQUE (user_identifier, user_identity_source_id, record_end_date); - ALTER TABLE biohub.administrative_activity ADD CONSTRAINT administrative_activity_uk1 UNIQUE (reported_system_user_id, administrative_activity_type_id); - - -- Recreate the view - CREATE OR REPLACE VIEW biohub_dapi_v1.system_user AS SELECT * FROM biohub.system_user; - CREATE OR REPLACE VIEW biohub_dapi_v1.administrative_activity AS SELECT * FROM biohub.administrative_activity; - + -- Don't allow more than 1 active record with the same user_guid. + CREATE UNIQUE INDEX system_user_nuk1 ON system_user (user_guid, (record_end_date is null)) WHERE record_end_date is null; + -- Don't allow more than 1 active record with the same user_identifier (case-insensitive) AND user_identity_source_id. + CREATE UNIQUE INDEX system_user_nuk2 ON system_user(LOWER(user_identifier), user_identity_source_id, (record_end_date is null)) WHERE record_end_date is null; `); } diff --git a/database/src/procedures/api_patch_system_user.ts b/database/src/procedures/api_patch_system_user.ts index 381695cc20..d8c126529c 100644 --- a/database/src/procedures/api_patch_system_user.ts +++ b/database/src/procedures/api_patch_system_user.ts @@ -17,10 +17,10 @@ import { Knex } from 'knex'; */ export async function seed(knex: Knex): Promise { await knex.raw(`--sql - set search_path=biohub; + set search_path = 'biohub'; CREATE OR REPLACE FUNCTION - api_patch_system_user ( + biohub.api_patch_system_user ( p_system_user_guid character varying, p_user_identifier character varying, p_user_identity_source_name character varying, @@ -34,16 +34,6 @@ export async function seed(knex: Knex): Promise { LANGUAGE plpgsql SET client_min_messages TO 'warning' AS $$ - -- ******************************************************************* - -- Procedure: api_patch_system_user - -- Purpose: Updates a system_user record if any of the incoming values are not the same as the existing values. - -- - -- MODIFICATION HISTORY - -- Person Date Comments - -- ---------------- ----------- -------------------------------------- - -- nick.phura@quartech.com - -- 2023-08-01 initial release - -- ******************************************************************* DECLARE _system_user system_user%rowtype; _user_identity_source_id user_identity_source.user_identity_source_id%type; diff --git a/database/src/seeds/01_db_system_users.ts b/database/src/seeds/01_db_system_users.ts index c78ce33b5a..a0c8c0b96a 100644 --- a/database/src/seeds/01_db_system_users.ts +++ b/database/src/seeds/01_db_system_users.ts @@ -29,46 +29,6 @@ interface SystemUserSeed { } const systemUsers: SystemUserSeed[] = [ - { - identifier: 'arosenth', - type: SYSTEM_IDENTITY_SOURCE.IDIR, - role_name: SYSTEM_USER_ROLE_NAME.SYSTEM_ADMINISTRATOR, - user_guid: 'DFE2CC5E345E4B1E813EC1DC10852064', - display_name: 'XT:Rosenthal, Alfred WLRS:IN', - given_name: 'Alfred', - family_name: 'Rosenthal', - email: 'alfred.rosenthal@quartech.com' - }, - { - identifier: 'cupshall', - type: SYSTEM_IDENTITY_SOURCE.IDIR, - role_name: SYSTEM_USER_ROLE_NAME.SYSTEM_ADMINISTRATOR, - user_guid: 'C42DFA74A976490A819BC85FF5E254E4', - display_name: 'Upshall, Curtis WLRS:EX', - given_name: 'Curtis', - family_name: 'Upshall', - email: 'curtis.upshall@gov.bc.ca' - }, - { - identifier: 'jxdunsdo', - type: SYSTEM_IDENTITY_SOURCE.IDIR, - role_name: SYSTEM_USER_ROLE_NAME.SYSTEM_ADMINISTRATOR, - user_guid: '82E8D3B4BAD045E8AD3980D426EA781C', - display_name: 'Dunsdon, Jeremy X WLRS:EX', - given_name: 'Jeremy', - family_name: 'Dunsdon', - email: 'jeremy.x.dunsdon@gov.bc.ca' - }, - { - identifier: 'keinarss', - type: SYSTEM_IDENTITY_SOURCE.IDIR, - role_name: SYSTEM_USER_ROLE_NAME.SYSTEM_ADMINISTRATOR, - user_guid: 'F4663727DE89489C8B7CFA81E4FA99B3', - display_name: 'Einarsson, Kjartan WLRS:EX', - given_name: 'Kjartan', - family_name: 'Einarsson', - email: 'kjartan.einarsson@gov.bc.ca' - }, { identifier: 'nphura', type: SYSTEM_IDENTITY_SOURCE.IDIR, @@ -79,16 +39,6 @@ const systemUsers: SystemUserSeed[] = [ family_name: 'Phura', email: 'nick.phura@gov.bc.ca' }, - { - identifier: 'zochampi', - type: SYSTEM_IDENTITY_SOURCE.IDIR, - role_name: SYSTEM_USER_ROLE_NAME.SYSTEM_ADMINISTRATOR, - user_guid: '349F20767A834FB582A18E8D378973E7', - display_name: 'Champiri, Zohreh D WLRS:EX', - given_name: 'Zohreh', - family_name: 'Champiri', - email: 'zohreh.d.champiri@gov.bc.ca' - }, { identifier: 'achirico', type: SYSTEM_IDENTITY_SOURCE.IDIR, @@ -109,26 +59,6 @@ const systemUsers: SystemUserSeed[] = [ family_name: 'Deluca', email: 'mac.deluca@gov.bc.ca' }, - { - identifier: 'gstewart', - type: SYSTEM_IDENTITY_SOURCE.IDIR, - role_name: SYSTEM_USER_ROLE_NAME.SYSTEM_ADMINISTRATOR, - user_guid: '83245BCDC21F43A29CEDA78AE67DF223', - display_name: 'Stewart, Graham WLRS:EX', - given_name: 'Stewart', - family_name: 'Graham', - email: 'graham.stewart@gov.bc.ca' - }, - { - identifier: 'jkissack', - type: SYSTEM_IDENTITY_SOURCE.IDIR, - role_name: SYSTEM_USER_ROLE_NAME.SYSTEM_ADMINISTRATOR, - user_guid: 'A82FE250A5BC40E68ABC54A1D0618D75', - display_name: 'Kissack, Jeremy WLRS:EX', - given_name: 'Jeremy', - family_name: 'Kissack', - email: 'jeremy.kissack@gov.bc.ca' - }, { identifier: 'mauberti', type: SYSTEM_IDENTITY_SOURCE.IDIR, From ade20f8cecfc4e5e887d0483daee22261b6f4094 Mon Sep 17 00:00:00 2001 From: Nick Phura Date: Mon, 22 Jul 2024 14:07:18 -0700 Subject: [PATCH 07/12] Rename migration file. ignore-skip. --- ...uplicate_users.ts => 20240626110000_remove_duplicate_users.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename database/src/migrations/{2024062611000000_remove_duplicate_users.ts => 20240626110000_remove_duplicate_users.ts} (100%) diff --git a/database/src/migrations/2024062611000000_remove_duplicate_users.ts b/database/src/migrations/20240626110000_remove_duplicate_users.ts similarity index 100% rename from database/src/migrations/2024062611000000_remove_duplicate_users.ts rename to database/src/migrations/20240626110000_remove_duplicate_users.ts From dc0af233c1121141b09027ae2973a5f22a3d9d0b Mon Sep 17 00:00:00 2001 From: Nick Phura Date: Mon, 22 Jul 2024 17:03:04 -0700 Subject: [PATCH 08/12] Rename migration --- ...uplicate_users.ts => 20240722000002_remove_duplicate_users.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename database/src/migrations/{20240626110000_remove_duplicate_users.ts => 20240722000002_remove_duplicate_users.ts} (100%) diff --git a/database/src/migrations/20240626110000_remove_duplicate_users.ts b/database/src/migrations/20240722000002_remove_duplicate_users.ts similarity index 100% rename from database/src/migrations/20240626110000_remove_duplicate_users.ts rename to database/src/migrations/20240722000002_remove_duplicate_users.ts From a7e03d0df1bc467de3175c204fbbaef07bce75e4 Mon Sep 17 00:00:00 2001 From: Nick Phura Date: Tue, 23 Jul 2024 10:38:10 -0700 Subject: [PATCH 09/12] Duplicate system user sql now also patches their user record details. --- .../20240722000002_remove_duplicate_users.ts | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/database/src/migrations/20240722000002_remove_duplicate_users.ts b/database/src/migrations/20240722000002_remove_duplicate_users.ts index 894521ea2b..09e3b06219 100644 --- a/database/src/migrations/20240722000002_remove_duplicate_users.ts +++ b/database/src/migrations/20240722000002_remove_duplicate_users.ts @@ -48,9 +48,9 @@ export async function up(knex: Knex): Promise { LOWER(user_identifier), user_identity_source_id ) - system_user_id, + LOWER(user_identifier) AS user_identifier, user_identity_source_id, - LOWER(user_identifier) AS user_identifier + system_user_id FROM system_user ORDER BY @@ -65,7 +65,15 @@ export async function up(knex: Knex): Promise { SELECT LOWER(system_user.user_identifier) AS user_identifier, user_identity_source_id, - array_agg(system_user.system_user_id) duplicate_system_user_ids + array_remove(array_agg(system_user.system_user_id), null) duplicate_system_user_ids, + -- Get the first non-null value for each of the remaining user detail columns + (array_remove(array_agg(system_user.user_guid), null))[1] user_guid, + (array_remove(array_agg(system_user.display_name), null))[1] display_name, + (array_remove(array_agg(system_user.given_name), null))[1] given_name, + (array_remove(array_agg(system_user.family_name), null))[1] family_name, + (array_remove(array_agg(system_user.email), null))[1] email, + (array_remove(array_agg(system_user.agency), null))[1] agency, + (array_remove(array_agg(system_user.notes), null))[1] notes FROM system_user GROUP BY @@ -78,7 +86,14 @@ export async function up(knex: Knex): Promise { SELECT w_system_user_1.system_user_id, w_system_user_1.user_identity_source_id, - array_remove(w_system_user_2.duplicate_system_user_ids, w_system_user_1.system_user_id) AS duplicate_system_user_ids + array_remove(w_system_user_2.duplicate_system_user_ids, w_system_user_1.system_user_id) AS duplicate_system_user_ids, + w_system_user_2.user_guid, + w_system_user_2.display_name, + w_system_user_2.given_name, + w_system_user_2.family_name, + w_system_user_2.email, + w_system_user_2.agency, + w_system_user_2.notes FROM w_system_user_1 left join @@ -169,6 +184,19 @@ export async function up(knex: Knex): Promise { FROM w_system_user_3 wsu3 WHERE su.system_user_id = ANY(wsu3.duplicate_system_user_ids) ) + -- Update the user details for the canonical system user record + w_update_system_user AS ( + UPDATE system_user su + SET + user_guid = wsu3.user_guid, + display_name = wsu3.display_name, + given_name = wsu3.given_name, + family_name = wsu3.family_name, + email = wsu3.email, + agency = wsu3.agency + FROM w_system_user_3 wsu3 + WHERE su.system_user_id = wsu3.system_user_id) + ) -- Return the combined results of the original CTEs (have to select something to run the query) SELECT * FROM w_system_user_3; From 1d9c6e70cb3dc100729d32d46093e5c2748f9bd8 Mon Sep 17 00:00:00 2001 From: Nick Phura Date: Tue, 23 Jul 2024 10:50:48 -0700 Subject: [PATCH 10/12] Update authorization to reject admins with end-dated records --- .../services/authorization-service.test.ts | 25 ++++++++++++++++--- api/src/services/authorization-service.ts | 5 ++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/api/src/services/authorization-service.test.ts b/api/src/services/authorization-service.test.ts index 3c92c25a14..24ce6ae197 100644 --- a/api/src/services/authorization-service.test.ts +++ b/api/src/services/authorization-service.test.ts @@ -137,6 +137,23 @@ describe('AuthorizationService', () => { expect(isAuthorizedByServiceClient).to.equal(false); }); + it('returns false if `record_end_date` is null', async function () { + const mockDBConnection = getMockDBConnection(); + + const mockGetSystemUsersObjectResponse = { + record_end_date: '2021-01-01', + role_names: [SYSTEM_ROLE.SYSTEM_ADMIN] + } as unknown as SystemUser; + + sinon.stub(AuthorizationService.prototype, 'getSystemUserObject').resolves(mockGetSystemUsersObjectResponse); + + const authorizationService = new AuthorizationService(mockDBConnection); + + const isAuthorizedByServiceClient = await authorizationService.authorizeSystemAdministrator(); + + expect(isAuthorizedByServiceClient).to.equal(false); + }); + it('returns true if `systemUserObject` is not null and includes admin role', async function () { const mockDBConnection = getMockDBConnection(); @@ -194,7 +211,7 @@ describe('AuthorizationService', () => { }; const mockDBConnection = getMockDBConnection(); - const mockGetSystemUsersObjectResponse = { record_end_date: 'datetime' } as unknown as SystemUser; + const mockGetSystemUsersObjectResponse = { record_end_date: '2021-01-01' } as unknown as SystemUser; sinon.stub(AuthorizationService.prototype, 'getSystemUserObject').resolves(mockGetSystemUsersObjectResponse); const authorizationService = new AuthorizationService(mockDBConnection); @@ -415,7 +432,8 @@ describe('AuthorizationService', () => { }; const mockDBConnection = getMockDBConnection(); - const mockGetSystemUsersObjectResponse = { record_end_date: 'datetime' } as unknown as ProjectUser & SystemUser; + const mockGetSystemUsersObjectResponse = { record_end_date: '2021-01-01' } as unknown as ProjectUser & + SystemUser; sinon .stub(AuthorizationService.prototype, 'getProjectUserObjectByProjectId') .resolves(mockGetSystemUsersObjectResponse); @@ -538,7 +556,8 @@ describe('AuthorizationService', () => { }; const mockDBConnection = getMockDBConnection(); - const mockGetSystemUsersObjectResponse = { record_end_date: 'datetime' } as unknown as ProjectUser & SystemUser; + const mockGetSystemUsersObjectResponse = { record_end_date: '2021-01-01' } as unknown as ProjectUser & + SystemUser; sinon .stub(AuthorizationService.prototype, 'getProjectUserObjectByProjectId') .resolves(mockGetSystemUsersObjectResponse); diff --git a/api/src/services/authorization-service.ts b/api/src/services/authorization-service.ts index 8fcc049dd0..01b5880b49 100644 --- a/api/src/services/authorization-service.ts +++ b/api/src/services/authorization-service.ts @@ -199,6 +199,11 @@ export class AuthorizationService extends DBService { // Cache the _systemUser for future use, if needed this._systemUser = systemUserObject; + if (systemUserObject.record_end_date) { + // system user has an expired record + return false; + } + return systemUserObject.role_names.includes(SYSTEM_ROLE.SYSTEM_ADMIN); } From 95a12b17ab7045addc545a777c7b8f0c47276fb9 Mon Sep 17 00:00:00 2001 From: Macgregor Aubertin-Young Date: Tue, 23 Jul 2024 16:19:12 -0700 Subject: [PATCH 11/12] fix sql syntax typo --- .../migrations/20240722000002_remove_duplicate_users.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/database/src/migrations/20240722000002_remove_duplicate_users.ts b/database/src/migrations/20240722000002_remove_duplicate_users.ts index 09e3b06219..5a3afd4b1e 100644 --- a/database/src/migrations/20240722000002_remove_duplicate_users.ts +++ b/database/src/migrations/20240722000002_remove_duplicate_users.ts @@ -183,7 +183,7 @@ export async function up(knex: Knex): Promise { notes = 'Duplicate user record; merged into system_user_id ' || wsu3.system_user_id || '.' FROM w_system_user_3 wsu3 WHERE su.system_user_id = ANY(wsu3.duplicate_system_user_ids) - ) + ), -- Update the user details for the canonical system user record w_update_system_user AS ( UPDATE system_user su @@ -195,7 +195,7 @@ export async function up(knex: Knex): Promise { email = wsu3.email, agency = wsu3.agency FROM w_system_user_3 wsu3 - WHERE su.system_user_id = wsu3.system_user_id) + WHERE su.system_user_id = wsu3.system_user_id ) -- Return the combined results of the original CTEs (have to select something to run the query) SELECT * FROM w_system_user_3; @@ -235,10 +235,10 @@ export async function up(knex: Knex): Promise { ---------------------------------------------------------------------------------------- -- Don't allow more than 1 active record with the same user_guid. - CREATE UNIQUE INDEX system_user_nuk1 ON system_user (user_guid, (record_end_date is null)) WHERE record_end_date is null; + CREATE UNIQUE INDEX system_user_nuk1 ON system_user (user_guid, record_end_date is null) WHERE record_end_date is null; -- Don't allow more than 1 active record with the same user_identifier (case-insensitive) AND user_identity_source_id. - CREATE UNIQUE INDEX system_user_nuk2 ON system_user(LOWER(user_identifier), user_identity_source_id, (record_end_date is null)) WHERE record_end_date is null; + CREATE UNIQUE INDEX system_user_nuk2 ON system_user(LOWER(user_identifier), user_identity_source_id, record_end_date is null) WHERE record_end_date is null; `); } From 9a22d09fd0993835b02627155d27db9a641b9c4b Mon Sep 17 00:00:00 2001 From: Nick Phura Date: Tue, 23 Jul 2024 17:40:24 -0700 Subject: [PATCH 12/12] Update query to handle project/survey participation edge cases --- .../20240722000002_remove_duplicate_users.ts | 129 ++++++++++-------- 1 file changed, 73 insertions(+), 56 deletions(-) diff --git a/database/src/migrations/20240722000002_remove_duplicate_users.ts b/database/src/migrations/20240722000002_remove_duplicate_users.ts index 5a3afd4b1e..97b5c23c68 100644 --- a/database/src/migrations/20240722000002_remove_duplicate_users.ts +++ b/database/src/migrations/20240722000002_remove_duplicate_users.ts @@ -35,6 +35,11 @@ export async function up(knex: Knex): Promise { DROP INDEX IF EXISTS system_user_uk1; DROP INDEX IF EXISTS system_user_nuk1; + -- Drop old out-dated unique index + DROP INDEX IF EXISTS biohub.project_participation_uk1; + -- Drop constraint temporarily (added back at the end) + ALTER TABLE project_participation DROP CONSTRAINT IF EXISTS project_participation_uk2; + ---------------------------------------------------------------------------------------- -- Find AND migrate duplicate system_user_ids ---------------------------------------------------------------------------------------- @@ -101,65 +106,71 @@ export async function up(knex: Knex): Promise { ON w_system_user_1.user_identifier = w_system_user_2.user_identifier AND w_system_user_1.user_identity_source_id = w_system_user_2.user_identity_source_id ), - -- Update duplicate system_user_ids in the project_participation table to the canonical system_user_id, ignoring - -- records where the canonical system_user_id is already in use. - w_update_project_participation AS ( + -- Get all project_participation records for each project with a system_user_id that is a duplicate, unless there is + -- already a record for that project with the canonical system_user_id. + w_project_participation_1 as ( + SELECT + array_agg(pp.project_participation_id) duplicate_project_participation_ids, + wsu3.system_user_id + FROM project_participation AS pp + JOIN w_system_user_3 AS wsu3 + ON pp.system_user_id = ANY(wsu3.duplicate_system_user_ids) + WHERE NOT EXISTS ( + SELECT 1 + FROM project_participation AS pp_check + WHERE pp_check.system_user_id = wsu3.system_user_id + AND pp_check.project_id = pp.project_id + ) + GROUP BY + wsu3.system_user_id, + pp.project_id + ), + -- For each project_participation record from the previous CTE, update the system_user_id to the canonical + -- system_user_id. + w_project_participation_2 AS ( UPDATE project_participation AS pp - SET system_user_id = wsu3.system_user_id - FROM w_system_user_3 AS wsu3 - WHERE pp.system_user_id = ANY(wsu3.duplicate_system_user_ids) - AND NOT EXISTS ( - SELECT 1 - FROM project_participation AS pp_check - WHERE pp_check.system_user_id = wsu3.system_user_id - AND pp_check.project_id = pp.project_id - ) + SET system_user_id = wpp1.system_user_id + FROM w_project_participation_1 AS wpp1 + where wpp1.duplicate_project_participation_ids[1] = pp.project_participation_id ), - -- Delete all remaining references to duplicate system_user_ids in the project_participation table, as long as - -- there exists another record for the same project with the canonical system_user_id (which should be all of them - -- after the previous CTE ran). - w_delete_project_participation AS ( - DELETE FROM project_participation - WHERE EXISTS ( - SELECT 1 - FROM w_system_user_3 AS wsu3 - WHERE project_participation.system_user_id = ANY(wsu3.duplicate_system_user_ids) - AND project_participation.project_id IN ( - SELECT project_id - FROM project_participation AS pp_check - WHERE pp_check.system_user_id = wsu3.system_user_id - ) - ) + -- Delete all remaining references to duplicate system_user_ids in the project_participation table. + w_project_participation_3 AS ( + DELETE FROM project_participation AS pp + USING w_system_user_3 + WHERE pp.system_user_id = ANY(w_system_user_3.duplicate_system_user_ids) ), - -- Update duplicate system_user_ids in the survey_participation table to the canonical system_user_id, ignoring - -- records where the canonical system_user_id is already in use. - w_update_survey_participation AS ( + -- Get all survey_participation records for each survey with a system_user_id that is a duplicate, unless there is + -- already a record for that survey with the canonical system_user_id. + w_survey_participation_1 as ( + SELECT + array_agg(sp.survey_participation_id) duplicate_survey_participation_ids, + wsu3.system_user_id + FROM survey_participation AS sp + JOIN w_system_user_3 AS wsu3 + ON sp.system_user_id = ANY(wsu3.duplicate_system_user_ids) + WHERE NOT EXISTS ( + SELECT 1 + FROM survey_participation AS sp_check + WHERE sp_check.system_user_id = wsu3.system_user_id + AND sp_check.survey_id = sp.survey_id + ) + GROUP BY + wsu3.system_user_id, + sp.survey_id + ), + -- For each survey_participation record from the previous CTE, update the system_user_id to the canonical + -- system_user_id. + w_survey_participation_2 AS ( UPDATE survey_participation AS sp - SET system_user_id = wsu3.system_user_id - FROM w_system_user_3 AS wsu3 - WHERE sp.system_user_id = ANY(wsu3.duplicate_system_user_ids) - AND NOT EXISTS ( - SELECT 1 - FROM survey_participation AS sp_check - WHERE sp_check.system_user_id = wsu3.system_user_id - AND sp_check.survey_id = sp.survey_id - ) + SET system_user_id = wsp1.system_user_id + FROM w_survey_participation_1 AS wsp1 + where wsp1.duplicate_survey_participation_ids[1] = sp.survey_participation_id ), - -- Delete all remaining references to duplicate system_user_ids in the survey_participation table, as long as - -- there exists another record for the same survey with the canonical system_user_id (which should be all of them - -- after the previous CTE ran). - w_delete_survey_participation AS ( - DELETE FROM survey_participation - WHERE EXISTS ( - SELECT 1 - FROM w_system_user_3 AS wsu3 - WHERE survey_participation.system_user_id = ANY(wsu3.duplicate_system_user_ids) - AND survey_participation.survey_id IN ( - SELECT survey_id - FROM survey_participation AS sp_check - WHERE sp_check.system_user_id = wsu3.system_user_id - ) - ) + -- Delete all remaining references to duplicate system_user_ids in the survey_participation table. + w_survey_participation_3 AS ( + DELETE FROM survey_participation AS sp + USING w_system_user_3 + WHERE sp.system_user_id = ANY(w_system_user_3.duplicate_system_user_ids) ), -- Update duplicate system_user_ids in the webform_draft table to the canonical system_user_id w_end_date_duplicate_webform_draft as ( @@ -235,10 +246,16 @@ export async function up(knex: Knex): Promise { ---------------------------------------------------------------------------------------- -- Don't allow more than 1 active record with the same user_guid. - CREATE UNIQUE INDEX system_user_nuk1 ON system_user (user_guid, record_end_date is null) WHERE record_end_date is null; + CREATE UNIQUE INDEX system_user_nuk1 ON system_user (user_guid, (record_end_date is null)) WHERE record_end_date is null; -- Don't allow more than 1 active record with the same user_identifier (case-insensitive) AND user_identity_source_id. - CREATE UNIQUE INDEX system_user_nuk2 ON system_user(LOWER(user_identifier), user_identity_source_id, record_end_date is null) WHERE record_end_date is null; + CREATE UNIQUE INDEX system_user_nuk2 ON system_user(LOWER(user_identifier), user_identity_source_id, (record_end_date is null)) WHERE record_end_date is null; + + -- Don't allow the same system user to have more than one project role within a project. + ALTER TABLE biohub.project_participation ADD CONSTRAINT project_participation_uk1 UNIQUE (system_user_id, project_id); + + -- Don't allow the same system user to have more than one survey role within a survey. + ALTER TABLE biohub.survey_participation ADD CONSTRAINT survey_participation_uk1 UNIQUE (system_user_id, survey_id); `); }