From 4b6cbe345133a980511323fc5ec90fa93c6a4bc2 Mon Sep 17 00:00:00 2001 From: Jason Sherman Date: Thu, 12 Dec 2024 16:09:42 -0800 Subject: [PATCH 1/4] FORMS-1563: auto-approve new ext api when same ministry/url already approved. Signed-off-by: Jason Sherman --- app/src/forms/admin/service.js | 28 ++++++-- app/src/forms/form/externalApi/service.js | 72 ++++++++++++++----- app/tests/unit/forms/admin/service.spec.js | 4 ++ .../forms/form/externalApi/service.spec.js | 6 ++ 4 files changed, 89 insertions(+), 21 deletions(-) diff --git a/app/src/forms/admin/service.js b/app/src/forms/admin/service.js index 2338a5f3b..a1d906d07 100644 --- a/app/src/forms/admin/service.js +++ b/app/src/forms/admin/service.js @@ -1,3 +1,4 @@ +const { ExternalAPIStatuses } = require('../common/constants'); const { Form, FormVersion, User, UserFormAccess, FormComponentsProactiveHelp, AdminExternalAPI, ExternalAPI, ExternalAPIStatusCode } = require('../common/models'); const { queryUtils } = require('../common/utils'); const { v4: uuidv4 } = require('uuid'); @@ -148,14 +149,33 @@ const service = { upd['userTokenHeader'] = null; upd['userTokenBearer'] = false; } - - await ExternalAPI.query().patchAndFetchById(id, upd); - - return ExternalAPI.query().findById(id); + let trx; + try { + trx = await ExternalAPI.startTransaction(); + await ExternalAPI.query(trx).patchAndFetchById(id, upd); + await service._approveMany(id, data, trx); + await trx.commit(); + return ExternalAPI.query().findById(id); + } catch (err) { + if (trx) await trx.rollback(); + throw err; + } }, getExternalAPIStatusCodes: async () => { return ExternalAPIStatusCode.query(); }, + _approveMany: async (id, data, trx) => { + // if we are setting to approved, approve all similar endpoints. + // same ministry, same base url... + if (data.code === ExternalAPIStatuses.APPROVED) { + const adminExternalAPI = await AdminExternalAPI.query(trx).findById(id); + const delimiter = '?'; + const baseUrl = data.endpointUrl.split(delimiter)[0]; + await ExternalAPI.query(trx) + .patch({ code: ExternalAPIStatuses.APPROVED }) + .whereRaw(`"endpointUrl" like '${baseUrl}%' and code = 'SUBMITTED' and "formId" in (select id from form where ministry = '${adminExternalAPI.ministry}')`); + } + }, /** * @function createFormComponentsProactiveHelp diff --git a/app/src/forms/form/externalApi/service.js b/app/src/forms/form/externalApi/service.js index ebf9f6cd1..fd607da85 100644 --- a/app/src/forms/form/externalApi/service.js +++ b/app/src/forms/form/externalApi/service.js @@ -3,7 +3,7 @@ const Problem = require('api-problem'); const { v4: uuidv4 } = require('uuid'); const { ExternalAPIStatuses } = require('../../common/constants'); -const { ExternalAPI, ExternalAPIStatusCode } = require('../../common/models'); +const { ExternalAPI, ExternalAPIStatusCode, Form, AdminExternalAPI } = require('../../common/models'); const { ENCRYPTION_ALGORITHMS } = require('../../../components/encryptionService'); @@ -60,20 +60,45 @@ const service = { } }, + _updateAllPreApproved: async (formId, data, trx) => { + let result = 0; + const form = await Form.query().findById(formId); + const delimiter = '?'; + const baseUrl = data.endpointUrl.split(delimiter)[0]; + // check if there are matching api endpoints for the same ministry as our form that have been previously approved. + const approvedApis = await AdminExternalAPI.query(trx).whereRaw(`"endpointUrl" like '${baseUrl}%' and ministry = '${form.ministry}' and code = 'APPROVED'`); + if (approvedApis && approvedApis.length) { + // ok, since we've already approved a matching api endpoint, make sure others on this form are approved too. + result = await ExternalAPI.query(trx) + .patch({ code: ExternalAPIStatuses.APPROVED }) + .whereRaw(`"endpointUrl" like '${baseUrl}%' and code = 'SUBMITTED' and "formId" = '${formId}'`); + } + return result; + }, + createExternalAPI: async (formId, data, currentUser) => { service.validateExternalAPI(data); data.id = uuidv4(); - // set status to SUBMITTED + // always create as SUBMITTED. data.code = ExternalAPIStatuses.SUBMITTED; // ensure that new records don't send user tokens. service.checkAllowSendUserToken(data, false); - await ExternalAPI.query().insert({ - ...data, - createdBy: currentUser.usernameIdp, - }); - - return ExternalAPI.query().findById(data.id); + let trx; + try { + trx = await ExternalAPI.startTransaction(); + await ExternalAPI.query(trx).insert({ + ...data, + createdBy: currentUser.usernameIdp, + }); + // any urls on this form pre-approved? + await service._updateAllPreApproved(formId, data, trx); + await trx.commit(); + return ExternalAPI.query().findById(data.id); + } catch (err) { + if (trx) await trx.rollback(); + throw err; + } }, updateExternalAPI: async (formId, externalAPIId, data, currentUser) => { @@ -82,17 +107,30 @@ const service = { const existing = await ExternalAPI.query().modify('findByIdAndFormId', externalAPIId, formId).first().throwIfNotFound(); // let's use a different method for the administrators to update status code and allow send user token - // this method should not change the status code. + // this method should not change the status code data.code = existing.code; + if (existing.endpointUrl.split('?')[0] !== data.endpointUrl.split('?')[0]) { + // url changed, so save as SUBMITTED. + data.code = ExternalAPIStatuses.SUBMITTED; + } service.checkAllowSendUserToken(data, existing.allowSendUserToken); - await ExternalAPI.query() - .modify('findByIdAndFormId', externalAPIId, formId) - .update({ - ...data, - updatedBy: currentUser.usernameIdp, - }); - - return ExternalAPI.query().findById(externalAPIId); + let trx; + try { + trx = await ExternalAPI.startTransaction(); + await ExternalAPI.query(trx) + .modify('findByIdAndFormId', externalAPIId, formId) + .update({ + ...data, + updatedBy: currentUser.usernameIdp, + }); + // any urls on this form pre-approved? + await service._updateAllPreApproved(formId, data, trx); + await trx.commit(); + return ExternalAPI.query().findById(data.id); + } catch (err) { + if (trx) await trx.rollback(); + throw err; + } }, deleteExternalAPI: async (formId, externalAPIId) => { diff --git a/app/tests/unit/forms/admin/service.spec.js b/app/tests/unit/forms/admin/service.spec.js index 23c959124..a92c71f79 100644 --- a/app/tests/unit/forms/admin/service.spec.js +++ b/app/tests/unit/forms/admin/service.spec.js @@ -137,6 +137,7 @@ describe('Admin service', () => { }); it('updateExternalAPI should patch and fetch', async () => { + service._approveMany = jest.fn().mockResolvedValueOnce(); await service.updateExternalAPI('id', { code: 'APPROVED', allowSendUserToken: true }); expect(MockModel.query).toBeCalledTimes(3); expect(MockModel.patchAndFetchById).toBeCalledTimes(1); @@ -145,9 +146,11 @@ describe('Admin service', () => { code: 'APPROVED', allowSendUserToken: true, }); + expect(service._approveMany).toBeCalledWith('id', { code: 'APPROVED', allowSendUserToken: true }, expect.anything()); }); it('updateExternalAPI should patch and fetch and update user token fields', async () => { + service._approveMany = jest.fn().mockResolvedValueOnce(); await service.updateExternalAPI('id', { code: 'APPROVED', allowSendUserToken: false }); expect(MockModel.query).toBeCalledTimes(3); expect(MockModel.patchAndFetchById).toBeCalledTimes(1); @@ -160,6 +163,7 @@ describe('Admin service', () => { userTokenHeader: null, userTokenBearer: false, }); + expect(service._approveMany).toBeCalledWith('id', { code: 'APPROVED', allowSendUserToken: false }, expect.anything()); }); it('getExternalAPIStatusCodes should fetch data', async () => { diff --git a/app/tests/unit/forms/form/externalApi/service.spec.js b/app/tests/unit/forms/form/externalApi/service.spec.js index 620429d02..e083354e4 100644 --- a/app/tests/unit/forms/form/externalApi/service.spec.js +++ b/app/tests/unit/forms/form/externalApi/service.spec.js @@ -155,6 +155,7 @@ describe('createExternalAPI', () => { }); it('should insert valid data', async () => { + service._updateAllPreApproved = jest.fn().mockResolvedValueOnce(0); validData.id = null; validData.code = null; await service.createExternalAPI(validData.formId, validData, user); @@ -164,6 +165,7 @@ describe('createExternalAPI', () => { code: ExternalAPIStatuses.SUBMITTED, ...validData, }); + expect(service._updateAllPreApproved).toBeCalledWith(validData.formId, validData, expect.anything()); }); it('should raise errors', async () => { @@ -205,6 +207,7 @@ describe('updateExternalAPI', () => { }); it('should update valid data', async () => { + service._updateAllPreApproved = jest.fn().mockResolvedValueOnce(0); MockModel.throwIfNotFound = jest.fn().mockResolvedValueOnce(Object.assign({}, validData)); // we do not update (status) code - must stay SUBMITTED @@ -217,6 +220,7 @@ describe('updateExternalAPI', () => { code: ExternalAPIStatuses.SUBMITTED, ...validData, }); + expect(service._updateAllPreApproved).toBeCalledWith(validData.formId, validData, expect.anything()); }); it('should update user token fields when allowed', async () => { @@ -226,6 +230,7 @@ describe('updateExternalAPI', () => { validData.userTokenHeader = 'Authorization'; validData.userTokenBearer = true; MockModel.throwIfNotFound = jest.fn().mockResolvedValueOnce(Object.assign({}, validData)); + service._updateAllPreApproved = jest.fn().mockResolvedValueOnce(0); await service.updateExternalAPI(validData.formId, validData.id, validData, user); expect(MockModel.update).toBeCalledTimes(1); @@ -243,6 +248,7 @@ describe('updateExternalAPI', () => { validData.userTokenHeader = 'Authorization'; validData.userTokenBearer = true; MockModel.throwIfNotFound = jest.fn().mockResolvedValueOnce(Object.assign({}, validData)); + service._updateAllPreApproved = jest.fn().mockResolvedValueOnce(0); await service.updateExternalAPI(validData.formId, validData.id, validData, user); expect(MockModel.update).toBeCalledTimes(1); From 72be12786782c481d74a3d356a9ff689cbca7750 Mon Sep 17 00:00:00 2001 From: Jason Sherman Date: Mon, 16 Dec 2024 19:43:48 -0800 Subject: [PATCH 2/4] limit usage of raw sql Signed-off-by: Jason Sherman --- app/src/forms/admin/service.js | 4 ++- app/src/forms/form/externalApi/service.js | 32 ++++++++++++++----- .../forms/form/externalApi/service.spec.js | 32 ++++++++++++++++--- 3 files changed, 55 insertions(+), 13 deletions(-) diff --git a/app/src/forms/admin/service.js b/app/src/forms/admin/service.js index a1d906d07..a6d1051f3 100644 --- a/app/src/forms/admin/service.js +++ b/app/src/forms/admin/service.js @@ -173,7 +173,9 @@ const service = { const baseUrl = data.endpointUrl.split(delimiter)[0]; await ExternalAPI.query(trx) .patch({ code: ExternalAPIStatuses.APPROVED }) - .whereRaw(`"endpointUrl" like '${baseUrl}%' and code = 'SUBMITTED' and "formId" in (select id from form where ministry = '${adminExternalAPI.ministry}')`); + .whereRaw(`"formId" in (select id from form where ministry = '${adminExternalAPI.ministry}')`) + .andWhere('endpointUrl', 'ilike', `${baseUrl}%`) + .andWhere('code', ExternalAPIStatuses.SUBMITTED); } }, diff --git a/app/src/forms/form/externalApi/service.js b/app/src/forms/form/externalApi/service.js index fd607da85..aef2d7a2f 100644 --- a/app/src/forms/form/externalApi/service.js +++ b/app/src/forms/form/externalApi/service.js @@ -41,6 +41,9 @@ const service = { throw new Problem(422, `'userTokenHeader' is required when 'sendUserToken' is true.`); } } + if (!data.endpointUrl || (data.endpointUrl && !(data.endpointUrl.startsWith('https://') || data.endpointUrl.startsWith('http://')))) { + throw new Problem(422, `'endpointUrl' is required and must start with 'http://' or 'https://'`); + } }, checkAllowSendUserToken: (data, allowSendUserToken) => { @@ -66,12 +69,17 @@ const service = { const delimiter = '?'; const baseUrl = data.endpointUrl.split(delimiter)[0]; // check if there are matching api endpoints for the same ministry as our form that have been previously approved. - const approvedApis = await AdminExternalAPI.query(trx).whereRaw(`"endpointUrl" like '${baseUrl}%' and ministry = '${form.ministry}' and code = 'APPROVED'`); + const approvedApis = await AdminExternalAPI.query(trx) + .where('endpointUrl', 'ilike', `${baseUrl}%`) + .andWhere('ministry', form.ministry) + .andWhere('code', ExternalAPIStatuses.APPROVED); if (approvedApis && approvedApis.length) { // ok, since we've already approved a matching api endpoint, make sure others on this form are approved too. result = await ExternalAPI.query(trx) .patch({ code: ExternalAPIStatuses.APPROVED }) - .whereRaw(`"endpointUrl" like '${baseUrl}%' and code = 'SUBMITTED' and "formId" = '${formId}'`); + .where('endpointUrl', 'ilike', `${baseUrl}%`) + .andWhere('formId', formId) + .andWhere('code', ExternalAPIStatuses.SUBMITTED); } return result; }, @@ -117,12 +125,20 @@ const service = { let trx; try { trx = await ExternalAPI.startTransaction(); - await ExternalAPI.query(trx) - .modify('findByIdAndFormId', externalAPIId, formId) - .update({ - ...data, - updatedBy: currentUser.usernameIdp, - }); + await ExternalAPI.query(trx).modify('findByIdAndFormId', externalAPIId, formId).update({ + formId: formId, + name: data.name, + code: data.code, + endpointUrl: data.endpointUrl, + sendApiKey: data.sendApiKey, + apiKeyHeader: data.apiKeyHeader, + apiKey: data.apiKey, + allowSendUserToken: data.allowSendUserToken, + sendUserToken: data.sendUserToken, + userTokenHeader: data.userTokenHeader, + userTokenBearer: data.userTokenBearer, + updatedBy: currentUser.usernameIdp, + }); // any urls on this form pre-approved? await service._updateAllPreApproved(formId, data, trx); await trx.commit(); diff --git a/app/tests/unit/forms/form/externalApi/service.spec.js b/app/tests/unit/forms/form/externalApi/service.spec.js index e083354e4..e2ea6c577 100644 --- a/app/tests/unit/forms/form/externalApi/service.spec.js +++ b/app/tests/unit/forms/form/externalApi/service.spec.js @@ -218,7 +218,16 @@ describe('updateExternalAPI', () => { expect(MockModel.update).toBeCalledWith({ updatedBy: user.usernameIdp, code: ExternalAPIStatuses.SUBMITTED, - ...validData, + formId: validData.formId, + name: validData.name, + endpointUrl: validData.endpointUrl, + sendApiKey: validData.sendApiKey, + apiKeyHeader: validData.apiKeyHeader, + apiKey: validData.apiKey, + allowSendUserToken: validData.allowSendUserToken, + sendUserToken: validData.sendUserToken, + userTokenHeader: validData.userTokenHeader, + userTokenBearer: validData.userTokenBearer, }); expect(service._updateAllPreApproved).toBeCalledWith(validData.formId, validData, expect.anything()); }); @@ -237,13 +246,22 @@ describe('updateExternalAPI', () => { expect(MockModel.update).toBeCalledWith({ updatedBy: user.usernameIdp, code: ExternalAPIStatuses.SUBMITTED, - ...validData, + formId: validData.formId, + name: validData.name, + endpointUrl: validData.endpointUrl, + sendApiKey: validData.sendApiKey, + apiKeyHeader: validData.apiKeyHeader, + apiKey: validData.apiKey, + allowSendUserToken: validData.allowSendUserToken, + sendUserToken: validData.sendUserToken, + userTokenHeader: validData.userTokenHeader, + userTokenBearer: validData.userTokenBearer, }); }); it('should blank out user token fields when not allowed', async () => { // mark as allowed by admin, and set some user token config values... - validData.allowSendUserToken = true; + validData.allowSendUserToken = false; validData.sendUserToken = false; // don't want to throw a 422... validData.userTokenHeader = 'Authorization'; validData.userTokenBearer = true; @@ -255,10 +273,16 @@ describe('updateExternalAPI', () => { expect(MockModel.update).toBeCalledWith({ updatedBy: user.usernameIdp, code: ExternalAPIStatuses.SUBMITTED, + allowSendUserToken: false, sendUserToken: false, userTokenHeader: null, userTokenBearer: false, - ...validData, + formId: validData.formId, + name: validData.name, + endpointUrl: validData.endpointUrl, + sendApiKey: validData.sendApiKey, + apiKeyHeader: validData.apiKeyHeader, + apiKey: validData.apiKey, }); }); From c2cfaa84acc3a6e10a2edb0259de2555f39f7728 Mon Sep 17 00:00:00 2001 From: Jason Sherman Date: Tue, 17 Dec 2024 15:39:15 -0800 Subject: [PATCH 3/4] add a sql injection block Signed-off-by: Jason Sherman --- app/src/forms/admin/service.js | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/app/src/forms/admin/service.js b/app/src/forms/admin/service.js index a6d1051f3..5ffbd3d1a 100644 --- a/app/src/forms/admin/service.js +++ b/app/src/forms/admin/service.js @@ -169,13 +169,18 @@ const service = { // same ministry, same base url... if (data.code === ExternalAPIStatuses.APPROVED) { const adminExternalAPI = await AdminExternalAPI.query(trx).findById(id); - const delimiter = '?'; - const baseUrl = data.endpointUrl.split(delimiter)[0]; - await ExternalAPI.query(trx) - .patch({ code: ExternalAPIStatuses.APPROVED }) - .whereRaw(`"formId" in (select id from form where ministry = '${adminExternalAPI.ministry}')`) - .andWhere('endpointUrl', 'ilike', `${baseUrl}%`) - .andWhere('code', ExternalAPIStatuses.SUBMITTED); + const regex = /^[A-Z]{2,4}$/; // Ministry constants are in the Frontend, they are 2,3,or 4 Capital chars + if (regex.test(adminExternalAPI.ministry)) { + // this will protect from sql injection. + // this should be removed when form API and db are updated to restrict form Ministry values. + const delimiter = '?'; + const baseUrl = data.endpointUrl.split(delimiter)[0]; + await ExternalAPI.query(trx) + .patch({ code: ExternalAPIStatuses.APPROVED }) + .whereRaw(`"formId" in (select id from form where ministry = '${adminExternalAPI.ministry}')`) + .andWhere('endpointUrl', 'ilike', `${baseUrl}%`) + .andWhere('code', ExternalAPIStatuses.SUBMITTED); + } } }, From d0c94593434c8ad9dec89329fcf71f3bcfd72700 Mon Sep 17 00:00:00 2001 From: Jason Sherman Date: Wed, 18 Dec 2024 18:35:45 -0800 Subject: [PATCH 4/4] Show sendApiKey fo admins before approving an api Signed-off-by: Jason Sherman --- .../src/components/admin/AdminAPIsTable.vue | 13 +++++++++ .../components/admin/AdminAPIsTable.spec.js | 6 +++++ ...241218233455_062_update_external_api_vw.js | 27 +++++++++++++++++++ 3 files changed, 46 insertions(+) create mode 100644 app/src/db/migrations/20241218233455_062_update_external_api_vw.js diff --git a/app/frontend/src/components/admin/AdminAPIsTable.vue b/app/frontend/src/components/admin/AdminAPIsTable.vue index 961435f78..fd427de1b 100644 --- a/app/frontend/src/components/admin/AdminAPIsTable.vue +++ b/app/frontend/src/components/admin/AdminAPIsTable.vue @@ -22,6 +22,7 @@ const editDialog = ref({ endpointUrl: null, code: null, allowSendUserToken: false, + sendApiKey: false, }, show: false, }); @@ -108,6 +109,7 @@ function resetEditDialog() { endpointUrl: null, code: null, allowSendUserToken: false, + sendApiKey: false, }, show: false, }; @@ -292,6 +294,17 @@ async function saveItem() { + + +