From d75c3eecea925a03a1d9ecbcfa26077d8edefaa1 Mon Sep 17 00:00:00 2001 From: John Cardinal Date: Sat, 24 Oct 2026 14:02:19 -0700 Subject: [PATCH] case 4712 - customer contacts work only with their own service requests and units Step 3 of 4712 (security review F14). The Customer role has change rights on service requests and nothing checked whose they were. For customer contacts: another customer's request is not found (read, update, delete); creating or moving a request to a customer the contact does not own is refused (403); a contact's request is always requested by that contact. unit/customerunit adds units only to the contact's own customer (or one of its head office's). Checked: another customer's request 404 (was 200), create for another customer 403, own request created and deleted. Co-Authored-By: Claude Opus 5.5 --- devdocs/modernization/NET10-PORT.md | 2 +- devdocs/modernization/SECURITY-REVIEW.md | 2 +- .../CustomerServiceRequestController.cs | 37 ++++++++++++++++++- server/AyaNova/Controllers/UnitController.cs | 3 ++ 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/devdocs/modernization/NET10-PORT.md b/devdocs/modernization/NET10-PORT.md index faebcd6d..4e5940f8 100644 --- a/devdocs/modernization/NET10-PORT.md +++ b/devdocs/modernization/NET10-PORT.md @@ -179,7 +179,7 @@ John's "next release" cases for AyaNova (exported 2026-10-24). Each is its own s |---|---|---| | 4710 | 2FA step requires the temp token (raven and sockeye) | F5, F41. **Done**: raven `141bdeb8` (proof script 401 on both bypasses; integration test `TwoFactor` in `8e5eec4`); sockeye `f7c1d7c` (same checks on local Sockeye with a temporary user). Sockeye to be deployed to its new droplet by John; 2FA can be turned back on once each server runs the fix | | 4711 | JWT secret setting removed: the server generates its signing key once into `jwt-signing.key` in the data folder (4698 folded in) | F1. **Done** (placeholder-forged token 401; key survives restart; auth tests pass). Hosted setup script's secret step now does nothing; tidy later in `sockeye-client` | -| 4712 | Customer contact route allowlist + ownership checks | F13, F14, F15, F16, F17. In 6 steps: 1 allowlist **done**; 2 work-order ownership **done**; 3 CSR and unit ownership; 4 pick lists for contacts; 5 schedule staff rights; 6 notification subscriptions | +| 4712 | Customer contact route allowlist + ownership checks | F13, F14, F15, F16, F17. In 6 steps: 1 allowlist **done**; 2 work-order ownership **done**; 3 CSR and unit ownership **done**; 4 pick lists for contacts; 5 schedule staff rights; 6 notification subscriptions | | 4713 | Contact users limited to customer roles | F28 | | 4715 | Download file names checked (one helper) | F21. **Done** (`FileUtil.FullPathInFolderOrNull` in temp/backup exists checks and both log routes; test `DownloadNames`) | | 4716 | Pick-list search text as SQL parameters | F18. **Done** (`@plq`/`@pltag` parameters; test `FetchUserPickListAutoCompleteApostrophe` fails on the old code, passes now; 9/9 pick-list tests) | diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md index 799fa246..6fd58d5d 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/devdocs/modernization/SECURITY-REVIEW.md @@ -198,7 +198,7 @@ take the next free number; a fixed or accepted finding keeps its ID and changes | F18 | Critical | **fixed in case 4716** | Pick-list search text is put into SQL without escaping; any logged-in user can reach it | | F26 | Critical | **fixed in case 4717** | KPI `interval` criterion is put into SQL unchecked; reachable by staff users down to TechRestricted | | F13 | High | **fixed in case 4712** | Customer contacts can open any customer's work order (customer view, customer report, attachments) by id | -| F14 | High | **confirmed** | Customer contacts can read, and by code change, delete and create, any customer's service requests | +| F14 | High | **fixed in case 4712** | Customer contacts can read, and by code change, delete and create, any customer's service requests | | F15 | High | **confirmed** | Customer contacts can list every customer (and vendors, users, parts...) with any pick-list columns, phones and emails included | | F16 | High | **confirmed** | `schedule/svc` and `schedule/user` have no role check: anyone logged in sees the whole service schedule | | F17 | Medium | confirmed (code) | Notification subscriptions accept any `UserId`, and object events reach subscribers without a rights check: record names to any email address | diff --git a/server/AyaNova/Controllers/CustomerServiceRequestController.cs b/server/AyaNova/Controllers/CustomerServiceRequestController.cs index 43ab11dc..c5d337f8 100644 --- a/server/AyaNova/Controllers/CustomerServiceRequestController.cs +++ b/server/AyaNova/Controllers/CustomerServiceRequestController.cs @@ -1,6 +1,8 @@ +using System.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; +using Microsoft.EntityFrameworkCore; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Authorization; using Microsoft.Extensions.Logging; @@ -52,6 +54,12 @@ namespace AyaNova.Api.Controllers return StatusCode(403, new ApiNotAuthorizedResponse()); if (!ModelState.IsValid) return BadRequest(new ApiErrorResponse(ModelState)); + if (!await ContactMayUseCustomerAsync(newObject.CustomerId)) + return StatusCode(403, new ApiNotAuthorizedResponse()); + //case 4712: a contact's request is always requested by that contact + var userType = UserTypeFromContext.Type(HttpContext.Items); + if (userType == UserType.Customer || userType == UserType.HeadOffice) + newObject.RequestedByUserId = UserIdFromContext.Id(HttpContext.Items); CustomerServiceRequest o = await biz.CreateAsync(newObject); if (o == null) return BadRequest(new ApiErrorResponse(biz.Errors)); @@ -76,6 +84,8 @@ namespace AyaNova.Api.Controllers return StatusCode(403, new ApiNotAuthorizedResponse()); if (!ModelState.IsValid) return BadRequest(new ApiErrorResponse(ModelState)); + if (!await ContactMayUseExistingAsync(id)) + return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND)); var o = await biz.GetAsync(id); if (o == null) return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND)); return Ok(ApiOkResponse.Response(o)); @@ -96,7 +106,11 @@ namespace AyaNova.Api.Controllers CustomerServiceRequestBiz biz = CustomerServiceRequestBiz.GetBiz(ct, HttpContext); if (!Authorized.HasModifyRole(HttpContext.Items, biz.BizType)) return StatusCode(403, new ApiNotAuthorizedResponse()); - var o = await biz.PutAsync(updatedObject); + if (!await ContactMayUseExistingAsync(updatedObject.Id)) + return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND)); + if (!await ContactMayUseCustomerAsync(updatedObject.CustomerId)) + return StatusCode(403, new ApiNotAuthorizedResponse()); + var o = await biz.PutAsync(updatedObject); if (o == null) { if (biz.Errors.Exists(z => z.Code == ApiErrorCode.CONCURRENCY_CONFLICT)) @@ -122,11 +136,32 @@ namespace AyaNova.Api.Controllers CustomerServiceRequestBiz biz = CustomerServiceRequestBiz.GetBiz(ct, HttpContext); if (!Authorized.HasDeleteRole(HttpContext.Items, biz.BizType)) return StatusCode(403, new ApiNotAuthorizedResponse()); + if (!await ContactMayUseExistingAsync(id)) + return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND)); if (!await biz.DeleteAsync(id)) return BadRequest(new ApiErrorResponse(biz.Errors)); return NoContent(); } + //case 4712: a customer contact works only with its own customer's service requests (staff: always true) + private async Task ContactMayUseCustomerAsync(long customerId) + { + var userType = UserTypeFromContext.Type(HttpContext.Items); + if (userType != UserType.Customer && userType != UserType.HeadOffice) + return true; + return await UserBiz.CustomerContactOwnsCustomerAsync(ct, UserIdFromContext.Id(HttpContext.Items), customerId); + } + + //case 4712: same for an existing service request (true if it does not exist, so the normal not found applies) + private async Task ContactMayUseExistingAsync(long id) + { + var userType = UserTypeFromContext.Type(HttpContext.Items); + if (userType != UserType.Customer && userType != UserType.HeadOffice) + return true; + var customerId = await ct.CustomerServiceRequest.AsNoTracking().Where(z => z.Id == id).Select(z => (long?)z.CustomerId).FirstOrDefaultAsync(); + return customerId == null || await UserBiz.CustomerContactOwnsCustomerAsync(ct, UserIdFromContext.Id(HttpContext.Items), (long)customerId); + } + diff --git a/server/AyaNova/Controllers/UnitController.cs b/server/AyaNova/Controllers/UnitController.cs index 33488733..8d79f081 100644 --- a/server/AyaNova/Controllers/UnitController.cs +++ b/server/AyaNova/Controllers/UnitController.cs @@ -80,6 +80,9 @@ namespace AyaNova.Api.Controllers if (!ModelState.IsValid) return BadRequest(new ApiErrorResponse(ModelState)); + //case 4712: a contact adds units only to its own customer (or, head office contact, one of its customers) + if (!await UserBiz.CustomerContactOwnsCustomerAsync(ct, biz.UserId, newObject.CustomerId)) + return StatusCode(403, new ApiNotAuthorizedResponse()); long? o = await biz.CustomerCreateAsync(newObject); if (o == null) return BadRequest(new ApiErrorResponse(biz.Errors));