From 267c9d625a44a9dcdbcab17917a41a353cbfab94 Mon Sep 17 00:00:00 2001 From: bhuvan-aot <61068301+bhuvan-aot@users.noreply.github.com> Date: Thu, 19 Sep 2024 10:54:57 -0700 Subject: [PATCH] Fix: FORMS-1012 make add team member in team management case insensitive (#1475) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix Team management - Add a new member - type BCEID - Business/Basic textfield is case sensitive - FORMS-1012 Fix copy submission option not visible for submissions other than SUBMITTED state. - FORMS-1277 * Fix Team management - Add a new member - type BCEID - Business/Basic textfield is case sensitive - FORMS-1012 Fix copy submission option not visible for submissions other than SUBMITTED state. - FORMS-1277 * Fix PR comments. * Fix unit test cases. * Fix PR comments, update query builder to match variables. --------- Co-authored-by: “bhuvan-aot” <“bhuvanesh.p@aot-technologies.com”> --- .../forms/submission/MySubmissionsActions.vue | 3 +- .../forms/manage/AddTeamMember.spec.js | 4 + .../forms/manage/TeamManagement.spec.js | 4 + .../submission/MySubmissionsActions.spec.js | 28 +++++- app/src/components/idpService.js | 30 ++++--- ...06171846_update_identity_provider_extra.js | 89 +++++++++++++++++++ app/src/forms/common/models/tables/user.js | 12 ++- .../fixtures/form/identity_providers.json | 12 ++- app/tests/unit/components/idpService.spec.js | 6 +- app/tests/unit/forms/user/service.spec.js | 4 +- docs/chefs-identity-provider-changes.md | 40 +++++---- 11 files changed, 186 insertions(+), 46 deletions(-) create mode 100644 app/src/db/migrations/20240806171846_update_identity_provider_extra.js diff --git a/app/frontend/src/components/forms/submission/MySubmissionsActions.vue b/app/frontend/src/components/forms/submission/MySubmissionsActions.vue index 0f0023ca8..5fdb59125 100644 --- a/app/frontend/src/components/forms/submission/MySubmissionsActions.vue +++ b/app/frontend/src/components/forms/submission/MySubmissionsActions.vue @@ -87,7 +87,8 @@ defineExpose({ diff --git a/app/frontend/tests/unit/components/forms/manage/AddTeamMember.spec.js b/app/frontend/tests/unit/components/forms/manage/AddTeamMember.spec.js index 918c35ea4..c44538518 100644 --- a/app/frontend/tests/unit/components/forms/manage/AddTeamMember.spec.js +++ b/app/frontend/tests/unit/components/forms/manage/AddTeamMember.spec.js @@ -83,6 +83,7 @@ const BCEIDBASIC = { param: 'username', exact: true, required: 2, + caseSensitive: false, }, { name: 'filterFullName', @@ -104,6 +105,7 @@ const BCEIDBASIC = { exact: true, param: 'email', required: 2, + caseSensitive: false, }, { name: 'filterSearch', @@ -164,6 +166,7 @@ const BCEIDBUSINESS = { param: 'username', exact: true, required: 2, + caseSensitive: false, }, { name: 'filterFullName', @@ -185,6 +188,7 @@ const BCEIDBUSINESS = { exact: true, param: 'email', required: 2, + caseSensitive: false, }, { name: 'filterSearch', diff --git a/app/frontend/tests/unit/components/forms/manage/TeamManagement.spec.js b/app/frontend/tests/unit/components/forms/manage/TeamManagement.spec.js index 1d077238e..71e311524 100644 --- a/app/frontend/tests/unit/components/forms/manage/TeamManagement.spec.js +++ b/app/frontend/tests/unit/components/forms/manage/TeamManagement.spec.js @@ -108,6 +108,7 @@ const BCEIDBASIC = { param: 'username', exact: true, required: 2, + caseSensitive: false, }, { name: 'filterFullName', @@ -129,6 +130,7 @@ const BCEIDBASIC = { exact: true, param: 'email', required: 2, + caseSensitive: false, }, { name: 'filterSearch', @@ -189,6 +191,7 @@ const BCEIDBUSINESS = { param: 'username', exact: true, required: 2, + caseSensitive: false, }, { name: 'filterFullName', @@ -210,6 +213,7 @@ const BCEIDBUSINESS = { exact: true, param: 'email', required: 2, + caseSensitive: false, }, { name: 'filterSearch', diff --git a/app/frontend/tests/unit/components/forms/submission/MySubmissionsActions.spec.js b/app/frontend/tests/unit/components/forms/submission/MySubmissionsActions.spec.js index 81d35de05..6a20c307e 100644 --- a/app/frontend/tests/unit/components/forms/submission/MySubmissionsActions.spec.js +++ b/app/frontend/tests/unit/components/forms/submission/MySubmissionsActions.spec.js @@ -79,9 +79,9 @@ describe('MySubmissionsActions', () => { ); }); - it('renders if this is a submitted submission and isCopyFromExistingSubmissionEnabled is true', () => { + it('renders if this is a completed submission and isCopyFromExistingSubmissionEnabled is true', () => { const SUBMISSION = { - status: 'SUBMITTED', + status: 'COMPLETED', }; formStore.form = { enableCopyExistingSubmission: true, @@ -103,6 +103,30 @@ describe('MySubmissionsActions', () => { ); }); + it('renders if this is a completed submission and isCopyFromExistingSubmissionEnabled is true', () => { + const SUBMISSION = { + status: 'ASSIGNED', + }; + formStore.form = { + enableCopyExistingSubmission: true, + }; + + const wrapper = shallowMount(MySubmissionsActions, { + props: { + formId: FORM_ID, + submission: SUBMISSION, + }, + global: { + plugins: [pinia, router], + stubs: STUBS, + }, + }); + + expect(wrapper.html()).not.toContain( + 'trans.mySubmissionsActions.copyThisSubmission' + ); + }); + it('hasDeletePermission returns true if the submission has the create permission', () => { let SUBMISSION = { permissions: [FormPermissions.SUBMISSION_CREATE], diff --git a/app/src/components/idpService.js b/app/src/components/idpService.js index c647f5ef6..c5172084a 100644 --- a/app/src/components/idpService.js +++ b/app/src/components/idpService.js @@ -156,19 +156,11 @@ class IdpService { let groupValid = reqd === 1 ? true : false; for (const f of filters) { // add the filter to the query... - const filterName = f.name; - const paramName = f.param; - const value = params[paramName]; - if ('exact' in f) { - const exact = f.exact; - q.modify(filterName, value, exact); - } else { - q.modify(filterName, value); - } - + this.applyUserSearchFilters(f, params, q); // // ok, check for required... // + const value = params[f.param]; if (reqd < 1) { // if required < 1, do nothing, always valid groupValid = true; @@ -198,14 +190,28 @@ class IdpService { return User.query() .modify('filterIdpUserId', params.idpUserId) .modify('filterIdpCode', params.idpCode) - .modify('filterUsername', params.username, false) + .modify('filterUsername', params.username, false, false) .modify('filterFullName', params.fullName) .modify('filterFirstName', params.firstName) .modify('filterLastName', params.lastName) - .modify('filterEmail', params.email, false) + .modify('filterEmail', params.email, false, false) .modify('filterSearch', params.search) .modify('orderLastFirstAscending'); } + + applyUserSearchFilters(filter, params, query) { + const filterName = filter.name; + const paramName = filter.param; + const value = params[paramName]; + let exact = 'exact' in filter ? filter.exact : false; + let caseSensitive = 'caseSensitive' in filter ? filter.caseSensitive : true; + if (exact || caseSensitive) { + query.modify(filterName, value, exact, caseSensitive); + } else { + query.modify(filterName, value); + } + return value; + } } let idpService = new IdpService(); diff --git a/app/src/db/migrations/20240806171846_update_identity_provider_extra.js b/app/src/db/migrations/20240806171846_update_identity_provider_extra.js new file mode 100644 index 000000000..68f715762 --- /dev/null +++ b/app/src/db/migrations/20240806171846_update_identity_provider_extra.js @@ -0,0 +1,89 @@ +const BCEID_EXTRAS_EXACT_IGNORE_CASE = { + formAccessSettings: 'idim', + addTeamMemberSearch: { + text: { + minLength: 6, + message: 'trans.manageSubmissionUsers.searchInputLength', + }, + email: { + exact: true, + message: 'trans.manageSubmissionUsers.exactBCEIDSearch', + }, + }, + userSearch: { + filters: [ + { name: 'filterIdpUserId', param: 'idpUserId', required: 0 }, + { name: 'filterIdpCode', param: 'idpCode', required: 0 }, + { name: 'filterUsername', param: 'username', required: 2, exact: true, caseSensitive: false }, + { name: 'filterFullName', param: 'fullName', required: 0 }, + { name: 'filterFirstName', param: 'firstName', required: 0 }, + { name: 'filterLastName', param: 'lastName', required: 0 }, + { name: 'filterEmail', param: 'email', required: 2, exact: true, caseSensitive: false }, + { name: 'filterSearch', param: 'search', required: 0 }, + ], + detail: 'Could not retrieve BCeID users. Invalid options provided.', + }, +}; + +const BCEID_EXTRAS_EXACT = { + formAccessSettings: 'idim', + addTeamMemberSearch: { + text: { + minLength: 6, + message: 'trans.manageSubmissionUsers.searchInputLength', + }, + email: { + exact: true, + message: 'trans.manageSubmissionUsers.exactBCEIDSearch', + }, + }, + userSearch: { + filters: [ + { name: 'filterIdpUserId', param: 'idpUserId', required: 0 }, + { name: 'filterIdpCode', param: 'idpCode', required: 0 }, + { name: 'filterUsername', param: 'username', required: 2, exact: true }, + { name: 'filterFullName', param: 'fullName', required: 0 }, + { name: 'filterFirstName', param: 'firstName', required: 0 }, + { name: 'filterLastName', param: 'lastName', required: 0 }, + { name: 'filterEmail', param: 'email', required: 2, exact: true }, + { name: 'filterSearch', param: 'search', required: 0 }, + ], + detail: 'Could not retrieve BCeID users. Invalid options provided.', + }, +}; +/** + * @param { import("knex").Knex } knex + * @returns { Promise } + */ +exports.up = function (knex) { + return Promise.resolve().then(() => + knex.schema + .hasTable('identity_provider') + .then(() => + knex('identity_provider').where({ code: 'bceid-business' }).update({ + extra: BCEID_EXTRAS_EXACT_IGNORE_CASE, + }) + ) + .then(() => + knex('identity_provider').where({ code: 'bceid-basic' }).update({ + extra: BCEID_EXTRAS_EXACT_IGNORE_CASE, + }) + ) + ); +}; +exports.down = function (knex) { + return Promise.resolve().then(() => + knex.schema + .hasTable('identity_provider') + .then(() => + knex('identity_provider').where({ code: 'bceid-business' }).update({ + extra: BCEID_EXTRAS_EXACT, + }) + ) + .then(() => + knex('identity_provider').where({ code: 'bceid-basic' }).update({ + extra: BCEID_EXTRAS_EXACT, + }) + ) + ); +}; diff --git a/app/src/forms/common/models/tables/user.js b/app/src/forms/common/models/tables/user.js index 9d4d939de..417d29a88 100644 --- a/app/src/forms/common/models/tables/user.js +++ b/app/src/forms/common/models/tables/user.js @@ -40,10 +40,12 @@ class User extends Timestamps(Model) { query.where('idpCode', value); } }, - filterUsername(query, value, exact = false) { + filterUsername(query, value, exact = false, caseSensitive = true) { if (value) { - if (exact) query.where('username', value); + if (exact && caseSensitive) query.where('username', value); // ilike is postgres case insensitive like + else if (exact && !caseSensitive) query.where('username', 'ilike', value); + else if (!exact && caseSensitive) query.where('username', 'like', `%${value}%`); else query.where('username', 'ilike', `%${value}%`); } }, @@ -65,10 +67,12 @@ class User extends Timestamps(Model) { query.where('fullName', 'ilike', `%${value}%`); } }, - filterEmail(query, value, exact = false) { + filterEmail(query, value, exact = false, caseSensitive = true) { if (value) { - if (exact) query.where('email', value); + if (exact && caseSensitive) query.where('email', value); // ilike is postgres case insensitive like + else if (exact && !caseSensitive) query.where('email', 'ilike', value); + else if (!exact && caseSensitive) query.where('email', 'like', `%${value}%`); else query.where('email', 'ilike', `%${value}%`); } }, diff --git a/app/tests/fixtures/form/identity_providers.json b/app/tests/fixtures/form/identity_providers.json index 726289ccb..cf1a59615 100644 --- a/app/tests/fixtures/form/identity_providers.json +++ b/app/tests/fixtures/form/identity_providers.json @@ -77,7 +77,8 @@ "name": "filterUsername", "exact": true, "param": "username", - "required": 2 + "required": 2, + "caseSensitive": false }, { "name": "filterFullName", @@ -98,7 +99,8 @@ "name": "filterEmail", "exact": true, "param": "email", - "required": 2 + "required": 2, + "caseSensitive": false }, { "name": "filterSearch", @@ -161,7 +163,8 @@ "name": "filterUsername", "exact": true, "param": "username", - "required": 2 + "required": 2, + "caseSensitive": false }, { "name": "filterFullName", @@ -182,7 +185,8 @@ "name": "filterEmail", "exact": true, "param": "email", - "required": 2 + "required": 2, + "caseSensitive": false }, { "name": "filterSearch", diff --git a/app/tests/unit/components/idpService.spec.js b/app/tests/unit/components/idpService.spec.js index 300d6274b..9d2a02d6f 100644 --- a/app/tests/unit/components/idpService.spec.js +++ b/app/tests/unit/components/idpService.spec.js @@ -139,15 +139,15 @@ describe('idpService', () => { expect(MockModel.query).toBeCalledTimes(1); expect(MockModel.modify).toBeCalledTimes(9); expect(MockModel.modify).toBeCalledWith('filterIdpCode', 'idir'); - expect(MockModel.modify).toBeCalledWith('filterEmail', 'em@il.com', false); + expect(MockModel.modify).toBeCalledWith('filterEmail', 'em@il.com', false, false); }); it('should return a customized user search', async () => { const s = await idpService.userSearch({ idpCode: 'bceid-business', email: 'em@il.com' }); expect(s).toBeFalsy(); expect(MockModel.query).toBeCalledTimes(1); - expect(MockModel.modify).toBeCalledWith('filterIdpCode', 'bceid-business'); - expect(MockModel.modify).toBeCalledWith('filterEmail', 'em@il.com', true); + expect(MockModel.modify).toBeCalledWith('filterIdpCode', 'bceid-business', false, true); + expect(MockModel.modify).toBeCalledWith('filterEmail', 'em@il.com', true, false); expect(MockModel.modify).toBeCalledTimes(9); }); diff --git a/app/tests/unit/forms/user/service.spec.js b/app/tests/unit/forms/user/service.spec.js index ba77baf76..6a8582bc2 100644 --- a/app/tests/unit/forms/user/service.spec.js +++ b/app/tests/unit/forms/user/service.spec.js @@ -38,11 +38,11 @@ describe('list', () => { expect(MockModel.modify).toBeCalledTimes(9); expect(MockModel.modify).toBeCalledWith('filterIdpUserId', params.idpUserId); expect(MockModel.modify).toBeCalledWith('filterIdpCode', params.idpCode); - expect(MockModel.modify).toBeCalledWith('filterUsername', params.username, false); + expect(MockModel.modify).toBeCalledWith('filterUsername', params.username, false, false); expect(MockModel.modify).toBeCalledWith('filterFullName', params.fullName); expect(MockModel.modify).toBeCalledWith('filterFirstName', params.firstName); expect(MockModel.modify).toBeCalledWith('filterLastName', params.lastName); - expect(MockModel.modify).toBeCalledWith('filterEmail', params.email, false); + expect(MockModel.modify).toBeCalledWith('filterEmail', params.email, false, false); expect(MockModel.modify).toBeCalledWith('filterSearch', params.search); expect(MockModel.modify).toBeCalledWith('orderLastFirstAscending'); }); diff --git a/docs/chefs-identity-provider-changes.md b/docs/chefs-identity-provider-changes.md index 657a4491f..670ff2028 100644 --- a/docs/chefs-identity-provider-changes.md +++ b/docs/chefs-identity-provider-changes.md @@ -4,26 +4,27 @@ Within the CHEFs application a user's identity provider determines a lot of thei A User's Identity Provider (IDP) is who vouches for them. In a simplified manner: they provide a username and password (generally) and an Identity Provder verifies them and they end up with a token. Currently for CHEFs we have 3 Identity Providers: `IDIR`, `BCeID Basic` and `BCeID Business`. `IDIR` is for employees/contractors on the BC Government. In CHEFs, the `IDIR` Identity Provider allows for greater power within CHEFs; as far as the CHEFs application is concerned IDIR is the `primary` Identity Provider. -Previously, all IDP logic was hardcoded within the frontend code and was difficult to change and maintain. +Previously, all IDP logic was hardcoded within the frontend code and was difficult to change and maintain. **Example pseudocode:** ``` - if user has idp === 'IDIR' then + if user has idp === 'IDIR' then enable create forms button ``` By removing the hardcode, we can add in new IDPs and redefine which IDP is the `primary`. This opens up CHEFs for installations in non-BC Government environments. ## Identity Provider Table + Columns are added to the Identity Provider table to support runtime configuration. -* `primary`: boolean, which IDP is the highest level access (currently IDIR) -* `login`: boolean, if this IDP should appear as a login option (Public does not) -* `permissions`: string array, what permissions within CHEFS (not forms) does this IDP have -* `roles`: string array, what Form Roles does this IDP have (designer, owner, submitter, etc) -* `tokenmap`: json blob. this contains the mapping of IDP token fields to userInfo fields. -* `extra`: json blob. this is where non-standard configuration goes. we don't want a column for everything. +- `primary`: boolean, which IDP is the highest level access (currently IDIR) +- `login`: boolean, if this IDP should appear as a login option (Public does not) +- `permissions`: string array, what permissions within CHEFS (not forms) does this IDP have +- `roles`: string array, what Form Roles does this IDP have (designer, owner, submitter, etc) +- `tokenmap`: json blob. this contains the mapping of IDP token fields to userInfo fields. +- `extra`: json blob. this is where non-standard configuration goes. we don't want a column for everything. ### Application Permissions @@ -58,6 +59,7 @@ Identity Provider also sets the scope of what roles a user can be assigned to an ``` ### Extra + This is a `json` field with no predetermined structure. For BC Gov, we use it for extra functionality for the BCeID IDPs. There are UX "enhancements" (frontend) and user search restrictions (server side) that were hardcoded, so now moved into this `json`. Any use of `extra` should assume that data fields may not exist or have null values. @@ -78,15 +80,15 @@ Currently, `IDIR` has no data in `extra`. }, }, userSearch: { - filters: [ - { name: 'filterIdpUserId', param: 'idpUserId', required: 0 }, - { name: 'filterIdpCode', param: 'idpCode', required: 0 }, - { name: 'filterUsername', param: 'username', required: 2, exact: true }, - { name: 'filterFullName', param: 'fullName', required: 0 }, - { name: 'filterFirstName', param: 'firstName', required: 0 }, - { name: 'filterLastName', param: 'lastName', required: 0 }, - { name: 'filterEmail', param: 'email', required: 2, exact: true }, - { name: 'filterSearch', param: 'search', required: 0 }, + filters: [ + { name: 'filterIdpUserId', param: 'idpUserId', required: 0 }, + { name: 'filterIdpCode', param: 'idpCode', required: 0 }, + { name: 'filterUsername', param: 'username', required: 2, exact: true, caseSensitive: false}, + { name: 'filterFullName', param: 'fullName', required: 0 }, + { name: 'filterFirstName', param: 'firstName', required: 0 }, + { name: 'filterLastName', param: 'lastName', required: 0 }, + { name: 'filterEmail', param: 'email', required: 2, exact: true, caseSensitive: false}, + { name: 'filterSearch', param: 'search', required: 0 }, ], detail: 'Could not retrieve BCeID users. Invalid options provided.' } @@ -94,6 +96,7 @@ Currently, `IDIR` has no data in `extra`. ``` ### Tokenmap + As part of the transistion to a new managed Keycloak realm, we lose the ability to do mapping of Identity Provider attributes to tokens. We do expect our User Information to be standardized and independent of the IDP, so we need to to the mapping ourselves. The `tokenmap` is a `json` blob that is effectively a `userInfo` property name mapped to a `token` attribute. Each Identity Provider must provide a mapping so we can build out our `userInfo` object (our current user). @@ -125,10 +128,11 @@ The code (both server and frontend) is confusing since `code` and `idp` fields w Basically, be aware and cautious with `code`, `idp`, `hint` and `idpHint` until this is addressed. ## Frontend - idpStore + When the application is loaded, we query and store the Identity Providers. This can be found in `frontend/store/identityProviders.js`. This has helper methods for building the login buttons, getting login hints, the primary IDP and getting data from `extra`. All access to the cached IDP data should come through this store. ## Backend - IdpService -Logic for new Identity Provider fields encapsulated in `components/idpService.js`. The queries and logic for parsing the token (use `tokenmap` field to transform token to userInfo). Also, `userSearch` is here as BCeID has specific requirements that are contained in the `extra` field. +Logic for new Identity Provider fields encapsulated in `components/idpService.js`. The queries and logic for parsing the token (use `tokenmap` field to transform token to userInfo). Also, `userSearch` is here as BCeID has specific requirements that are contained in the `extra` field.