CHI-3705: Permissions for updating fields#997
Conversation
|
@copilot Add some additional service tests under hrm-service/service-tests/contacts in a file called contactFieldPermissions specifically to test the various scenarios around users being unable to update certain fields under the rawJson property of a contact, following the introduction of contact field specific permissions in this PR. Test cases should include, but not be limited to:
Make any fixes required to the implementation to make the tests pass once written Ensure the licence header at the top of all files is added to any new ones Run the following checks before every commit Run |
|
@stephenhand I've opened a new pull request, #998, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
- Broaden ContactFieldSpecificCondition field type to accept any string after the form name - Fix prettier linting issues in contactFieldExclusions.test.ts Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
The test was failing with 404 because the middleware requires viewContact permission to retrieve the contact before checking field-level permissions. Explicitly set viewContact to ['everyone'] to ensure the contact can be viewed. Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
The test was creating contacts with a random accountSid that didn't match the mocked authentication system, causing 404 errors when trying to access the contacts. Fixed by importing and using the standard accountSid from mocks. Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
CHI-3705: Add tests for contact field permissions and fix isOwner condition bug
There was a problem hiding this comment.
Pull request overview
Implements field-level contact update permissions by introducing a new contactField target kind and applying per-field exclusions during contact PATCH operations, while also renaming request-carried rule data from permissions to permissionRules.
Changes:
- Add
contactFieldactions/rule support and evaluate field-specific permissions to compute excluded fields. - Apply excluded-field handling in the contact update SQL path so disallowed fields retain their prior values.
- Update routing/tests to use
permissionRulesand add new test coverage around field exclusions.
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| hrm-domain/hrm-service/service-tests/mocks.ts | Updates test permission context shape (permissionRules, permissionCheckContact). |
| hrm-domain/hrm-service/service-tests/contact/contactFieldPermissions.test.ts | Adds service-level tests validating field-level PATCH behavior under various rule setups. |
| hrm-domain/hrm-core/unit-tests/permissions/canPerformActionsOnObject.test.ts | Adjusts action coverage to account for new contactField target kind behavior. |
| hrm-domain/hrm-core/unit-tests/mocks.ts | Updates unit-test permission context shape (permissionRules, permissionCheckContact). |
| hrm-domain/hrm-core/unit-tests/contact/contactService.test.ts | Updates expectations for new data-layer patch signature (adds exclusions arg). |
| hrm-domain/hrm-core/unit-tests/contact/contactFieldExclusions.test.ts | Adds unit tests for excluded-field computation. |
| hrm-domain/hrm-core/unit-tests/case/caseService.test.ts | Renames permissions context to permissionRules in tests. |
| hrm-domain/hrm-core/profile/profileRoutesV0.ts | Renames request permissions prop usage to permissionRules. |
| hrm-domain/hrm-core/permissions/rulesMap.ts | Adds contactField condition typing/validation support and unsupported-condition rules. |
| hrm-domain/hrm-core/permissions/permissions-routes-v0.ts | Returns raw rules payload directly (removes legacy alias fields). |
| hrm-domain/hrm-core/permissions/initializeCanForRules.ts | Adds contactField condition evaluation and exports helpers for exclusions logic. |
| hrm-domain/hrm-core/permissions/index.ts | Renames request-attached rules to permissionRules and updates max-permissions mock. |
| hrm-domain/hrm-core/permissions/canPerformActionOnObject.ts | Adds contactField handling and improves error metadata/return style. |
| hrm-domain/hrm-core/permissions/actions.ts | Introduces contactField actions and target-kind mapping. |
| hrm-domain/hrm-core/permission-rules/zw.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/zm.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/za.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/usvc.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/uscr.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/usch.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/ukmh.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/tz.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/th.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/sg.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/ph.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/open.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/nzba.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/nz.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/mw.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/mt.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/jm.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/in.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/hu.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/eumc.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/et.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/e2e.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/dev.json | Adds viewContactField / editContactField defaults and normalizes JSON formatting. |
| hrm-domain/hrm-core/permission-rules/demo.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/co.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/closed.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/clhs.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/cl.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/ca.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/permission-rules/br.json | Adds viewContactField / editContactField defaults. |
| hrm-domain/hrm-core/contact/sql/contact-update-sql.ts | Generates update SQL dynamically to preserve excluded fields during rawJson form updates. |
| hrm-domain/hrm-core/contact/contactService.ts | Computes excluded fields and passes them through the patch pipeline; renames ctx to permissionRules. |
| hrm-domain/hrm-core/contact/contactRoutesV0.ts | Plumbs permissionRules and permissionCheckContact into PATCH handling. |
| hrm-domain/hrm-core/contact/contactFieldExclusions.ts | Adds excluded-field computation based on editContactField rules. |
| hrm-domain/hrm-core/contact/contactDataAccess.ts | Updates patch signature to accept exclusions and uses generated SQL. |
| hrm-domain/hrm-core/contact/canPerformContactAction.ts | Stores contact on request for downstream exclusions computation. |
| hrm-domain/hrm-core/case/caseService.ts | Renames permissions context to permissionRules throughout case flows. |
| hrm-domain/hrm-core/case/caseSection/caseSectionService.ts | Renames permissions context to permissionRules for timeline access. |
| hrm-domain/hrm-core/case/caseRoutesV0.ts | Renames request permissions prop usage to permissionRules. |
| .github/instructions/global.instructions.md | Adds repository instructions scoped to plugin-hrm-form/**. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface Request { | ||
| permissions: RulesFile; | ||
| permissionRules: RulesFile; | ||
| can: InitializedCan; | ||
| } | ||
| } |
There was a problem hiding this comment.
permissionCheckContact is used on req (set in canPerformContactAction and read in contactRoutesV0), but it is not declared on the global Express.Request type here. This will fail TypeScript compilation unless you extend the request type to include permissionCheckContact?: Contact (or an appropriate type) alongside permissionRules.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@stephenhand I've opened a new pull request, #1000, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
…s' into CHI-3705-update_field_permissions # Conflicts: # hrm-domain/hrm-core/permissions/rulesMap.ts
Fix contactField permission validation to support isOwner condition
|
@stephenhand I've opened a new pull request, #1001, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
|
@copilot - enhance the validation in
Note, 2 different fields in 2 different condition sets are allowed, so:
|
|
@stephenhand I've opened a new pull request, #1002, to work on those changes. Once the pull request is ready, I'll request review from you. |
Add permissionCheckContact to Express.Request type declaration
…s and multiple field conditions per set Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
CHI-3705: Add validation for contactField permission conditions
…s' into CHI-3705-update_field_permissions
There was a problem hiding this comment.
Looks great to me!
I have a clarifying question:
Does something like
"viewContactField": [{ "field": 'rawJson.caseInformation.callSummary' }, 'isSupervisor']
means that "only supervisor can see rawJson.caseInformation.callSummary"?
Also, as a future refactor we could consider combining the root level with the "post processing" clauses, so that instead of
"viewContact": [["isSupervisor"], ["isOwner"]],
"editContact": [["isOwner"],["isSupervisor"]],
"viewContactField":[{ "field": 'rawJson.caseInformation.callSummary' }, 'isSupervisor'],
"editContactField": [["everyone"]],
we could have something like
"contact": {
"view:": {
"conditions": [["isSupervisor"], ["isOwner"]],
"viewFields: [{ "field": 'rawJson.caseInformation.callSummary' }, 'isSupervisor']
},
"edit": {
......
}
}
I think this will be easier to read in the long run, and it may solve some of the existing type-issues with the TKinds and all that stuff (maybe?).
Co-authored-by: Gianfranco <paolonigianfranco@gmail.com>
| if (fieldConditions.length > 1) { | ||
| console.error( | ||
| 'Invalid permission configuration. Multiple field conditions in a single condition set are not valid. Bottom level condition sets have AND logic, having 2 fields here means a field being checked needs to be 2 different fields at the same time. This condition set will always evaluate false', | ||
| 'action:', | ||
| action, | ||
| 'condition set: ', | ||
| conditionSet, | ||
| ); |
There was a problem hiding this comment.
I have two questions regarding this:
- Why is that we only permit a single condition per field? Wouldn't make sense to support things like "isOwner and contact is less than 24 hours old"?
- I think having the
getExcludedFieldsin thecontactmodules makes sense, but this chunk is actually checking if permission rules are valid, which I would expect to live within thepermissionsmodule instead. Is there a reason why splitting the "are conditions valid" checks makes more sense here?
Yes you're right and yes I agree, perhaps if we extend this to cases we can do something like this? I'm not sure I'm convinced that grouping viewField under view is the best way to go, but grouping by target type for sure |
Description
This adds support for applying permissions to specific contact fields with regards to editing them
contactFieldviewContactFieldandeditContactField{ field: 'rawJson.callerInformationTab.myAwesomeField' }["isSupervisor"]will allow supervisors full access to all fieldsAlso reworked the elasticsearch setup for the unit tests to get past a new error about out of date API versions
Checklist
Other Related Issues
CHI-3566
Verification steps
AFTER YOU MERGE
You are responsible for ensuring the above steps are completed. If you move a ticket into QA without advising what version to test, the QA team will assume the latest tag has the changes. If it does not, the following confusion is on you! :-P