From 87fdc12dcef152c6bf27646edcde79cdc42cb0b4 Mon Sep 17 00:00:00 2001 From: John Cardinal Date: Sat, 24 Oct 2026 14:39:40 -0700 Subject: [PATCH] case 4712 - notification subscriptions are the caller's own; object events only to subscribers who may read the record type Step 6 of 4712 (security review F17). A subscription kept the UserId sent by the client, and object events (created, modified, deleted, age) were matched by record type only, so a user could get record names of types it may not read, delivered to any address. Now a subscription is created for the caller and an update cannot move it to another user, unless the caller administers users; and object events skip subscribers without read rights on the record type. Checked: Service's subscription asked for SuperUser is saved as its own; with "customer created" subscriptions, Inventory (no read on customers) got 0 events, Service 1. Full suite: 109 passed, 5 skipped, the same 2 known failures. Co-Authored-By: Claude Opus 5.5 --- devdocs/modernization/NET10-PORT.md | 2 +- devdocs/modernization/SECURITY-REVIEW.md | 2 +- server/AyaNova/biz/NotifyEventHelper.cs | 15 +++++++++++++++ server/AyaNova/biz/NotifySubscriptionBiz.cs | 6 ++++++ 4 files changed, 23 insertions(+), 2 deletions(-) diff --git a/devdocs/modernization/NET10-PORT.md b/devdocs/modernization/NET10-PORT.md index 7f850e96..1df00612 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 **done**; 4 pick lists for contacts **done**; 5 schedule staff rights **done**; 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 **done**; 5 schedule staff rights **done**; 6 notification subscriptions **done**. All steps done; John to click through the customer screens as a contact before release | | 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, and F43 (unit pick-list variant parsed as a number, found during 4712, test `FetchUnitPickListCustomerVariant`). **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 3a34200d..c35a4c0b 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/devdocs/modernization/SECURITY-REVIEW.md @@ -202,7 +202,7 @@ take the next free number; a fixed or accepted finding keeps its ID and changes | F14 | High | **fixed in case 4712** | Customer contacts can read, and by code change, delete and create, any customer's service requests | | F15 | High | **fixed in case 4712** | Customer contacts can list every customer (and vendors, users, parts...) with any pick-list columns, phones and emails included | | F16 | High | **fixed in case 4712** | `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 | +| F17 | Medium | **fixed in case 4712** | Notification subscriptions accept any `UserId`, and object events reach subscribers without a rights check: record names to any email address | | F20 | High | candidate | Report templates receive the renderer's token (SuperUser for customer notification reports) and the report browser can reach any network address | | F27 | High | **confirmed** | Restricted roles (TechRestricted, SubContractor, SubContractorRestricted) get whole work-order items through report data, rendering and export | | F28 | High | candidate | Users with change rights on Customer (Service, Sales, Accounting) can give customer contact logins any role, BizAdmin and OpsAdmin included | diff --git a/server/AyaNova/biz/NotifyEventHelper.cs b/server/AyaNova/biz/NotifyEventHelper.cs index f8e0ce9c..63245b39 100644 --- a/server/AyaNova/biz/NotifyEventHelper.cs +++ b/server/AyaNova/biz/NotifyEventHelper.cs @@ -72,6 +72,13 @@ namespace AyaNova.Biz // PROCESS STANDARD CREATE NOTIFICATION // // + //case 4712: object events (created, modified, deleted, age) go only to subscribers with read rights on that type + private static async Task SubscriberMayReadAsync(AyContext ct, long userId, AyaType aType) + { + var roles = await ct.User.AsNoTracking().Where(z => z.Id == userId).Select(z => z.Roles).FirstOrDefaultAsync(); + return AyaNova.Api.ControllerHelpers.Authorized.HasReadFullRole(roles, aType); + } + public static async Task ProcessStandardObjectCreatedEvents(ICoreBizObjectModel newObject, AyContext ct) { //CREATED SUBSCRIPTIONS @@ -81,6 +88,8 @@ namespace AyaNova.Biz { //not for inactive users if (!await UserBiz.UserIsActive(sub.UserId)) continue; + //case 4712: not for users who may not read this type of record + if (!await SubscriberMayReadAsync(ct, sub.UserId, newObject.AyaType)) continue; if (ObjectHasAllSubscriptionTags(newObject.Tags, sub.Tags)) { NotifyEvent n = new NotifyEvent() @@ -107,6 +116,8 @@ namespace AyaNova.Biz { //not for inactive users if (!await UserBiz.UserIsActive(sub.UserId)) continue; + //case 4712: not for users who may not read this type of record + if (!await SubscriberMayReadAsync(ct, sub.UserId, newObject.AyaType)) continue; if (ObjectHasAllSubscriptionTags(newObject.Tags, sub.Tags)) { //Note: age is set by advance notice which is consulted by CoreJobNotify in it's run so the deliver date is not required here only the reference EventDate to check for deliver @@ -145,6 +156,8 @@ namespace AyaNova.Biz { //not for inactive users if (!await UserBiz.UserIsActive(sub.UserId)) continue; + //case 4712: not for users who may not read this type of record + if (!await SubscriberMayReadAsync(ct, sub.UserId, newObject.AyaType)) continue; if (ObjectHasAllSubscriptionTags(newObject.Tags, sub.Tags)) { NotifyEvent n = new NotifyEvent() @@ -185,6 +198,8 @@ namespace AyaNova.Biz { //not for inactive users if (!await UserBiz.UserIsActive(sub.UserId)) continue; + //case 4712: not for users who may not read this type of record + if (!await SubscriberMayReadAsync(ct, sub.UserId, bizObject.AyaType)) continue; if (ObjectHasAllSubscriptionTags(bizObject.Tags, sub.Tags)) { //TODO: On deliver should point to history event log record or take from there and insert into delivery message? diff --git a/server/AyaNova/biz/NotifySubscriptionBiz.cs b/server/AyaNova/biz/NotifySubscriptionBiz.cs index 29ccac2a..d48fb706 100644 --- a/server/AyaNova/biz/NotifySubscriptionBiz.cs +++ b/server/AyaNova/biz/NotifySubscriptionBiz.cs @@ -38,6 +38,9 @@ namespace AyaNova.Biz // internal async Task CreateAsync(NotifySubscription newObject) { + //case 4712: a subscription is the caller's own unless the caller administers users + if (!AyaNova.Api.ControllerHelpers.Authorized.HasModifyRole(CurrentUserRoles, AyaType.User)) + newObject.UserId = UserId; await ValidateAsync(newObject); if (HasErrors) return null; @@ -78,6 +81,9 @@ namespace AyaNova.Biz AddError(ApiErrorCode.NOT_FOUND, "id"); return null; } + //case 4712: an update cannot move a subscription to another user unless the caller administers users + if (!AyaNova.Api.ControllerHelpers.Authorized.HasModifyRole(CurrentUserRoles, AyaType.User)) + putObject.UserId = dbObject.UserId; if (dbObject.Concurrency != putObject.Concurrency) { AddError(ApiErrorCode.CONCURRENCY_CONFLICT);