From e6c77d76ff8542b6d9fe29e2577fbebee46bfdc7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 07:34:37 +0000 Subject: [PATCH] Let every Duly set read business units, so a unit shows its name (#180) Members, managers and Duly admins got 403 on sys_business_unit, so the unit lookup on a task or duty rendered as a raw id, and a duly_admin who is not a platform admin could not Import samples/duties.csv (units resolve by name under the importer's read: 0/19). Grant read only, in MEMBER_OBJECTS, inherited by manager and admin. No write bit, no scope key and no field map: a name-only FLS mask also binds every admin who co-holds a Duly set and blanks Setup > Business Units (objectstack#29106). The note names what the read discloses, external_ref included. security.test.ts names the one platform read as the exception to the duly-namespace invariants; business-unit-read.test.ts drives it as real member, manager and admin holders (list, by id, by name, expanded lookup, writes refused, no fields entry on the object). Fixes #180 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01JQwxba8DtH3fgAAexeLs8H --- docs/deployment/security.md | 1 + src/security/permission-sets.ts | 48 +++++++ test/business-unit-read.test.ts | 213 ++++++++++++++++++++++++++++++++ test/security.test.ts | 41 +++++- 4 files changed, 301 insertions(+), 2 deletions(-) create mode 100644 test/business-unit-read.test.ts diff --git a/docs/deployment/security.md b/docs/deployment/security.md index 5231c5f..05f3177 100644 --- a/docs/deployment/security.md +++ b/docs/deployment/security.md @@ -23,6 +23,7 @@ apply over MCP, and never appears in an audit. | `duly_log_entry` | full control · read **own** · write **own** | inherited | inherited | | `duly_catalog_item` | read | inherited | **+ create / edit / delete** · write **org** | | `duly_assignment` | read · read **own** | **+ create / edit** · write **own** | inherited | +| `sys_business_unit` (platform) | read — every unit, every column, incl. `external_ref` · no write ([#180](https://github.com/objectstack-ai/duly/issues/180)) | inherited | inherited | | Capabilities | `duly.task.update_status` | inherited | **+ `duly.catalog.apply`, `duly.catalog.sync`** | "Inherited" is literal: `src/security/permission-sets.ts` builds each map by diff --git a/src/security/permission-sets.ts b/src/security/permission-sets.ts index a16563b..5b9d755 100644 --- a/src/security/permission-sets.ts +++ b/src/security/permission-sets.ts @@ -64,6 +64,10 @@ import type { FieldPermission, ObjectPermission } from '@objectstack/spec/securi * read-open, write-owned), which is why the admin entry has to say `org` for * "full control" to mean anything at all. * + * ── One platform object: read on `sys_business_unit` (#180) ───────────── + * Every other entry is a `duly_*` object. Business units are read so the + * unit lookups on tasks and duties render as names; see the entry's note. + * * ── Two `fields` maps: task and duty provenance (#176, #178) ───────────── * Nothing in this schema needs MASKING: the server-owned columns * (`completed_at`, `last_update_at`, `last_dispatched_period`) are already @@ -169,6 +173,50 @@ const MEMBER_OBJECTS = { allowRead: true, readScope: 'own', }, + + /** + * The one platform object this package grants: READ on business units, so + * a unit renders as its name (#180). + * + * `duly_task.business_unit` and `duly_duty.business_unit` are lookups to + * `sys_business_unit`, and the console resolves a lookup by reading the + * target AS THE VIEWER. Measured on 18.2.0 before this entry: every Duly set + * got `403` on the object (the platform baseline does not name it either), + * so a member's own task and duty pages showed the unit as a raw id, and a + * `duly_admin` who is not a platform administrator could not Import + * `samples/duties.csv` at all — the Import resolves `business_unit` BY NAME + * under the importer's read, and refused 19/19 rows with "no record + * matches". With this entry: names render, the Import lands 19/19. + * + * ── What it discloses ─────────────────────────────────────────────────── + * `sys_business_unit` declares no `sharingModel`, and a `sys_` object's + * unset model is public, so this reaches EVERY unit in the tenant, with + * every column: name, code, kind, parent unit, unit head, timezone, + * active / effective dates and `external_ref` (the HRIS id). Read only: + * no create, edit or delete bit, so every write stays `403`. + * The tree's shape is not new to a member — the platform's baseline already + * lets every member read each colleague's `primary_business_unit_id` and + * `manager_id` on `sys_user`. The names are what this adds. + * + * ⛔ Why not a name-only read. It is declarable — `readable: false` on + * every other column in `fields` — and it works for the member, but field + * permissions merge most-permissively among DECLARING sets only, and the + * platform's admin set declares nothing on these columns. So the mask also + * binds every administrator who holds a Duly set: measured, Setup → Business + * Units lost Kind, Business Unit Head and Active, and the org chart went + * flat. Filed as objectstack-ai/objectstack#29106. Until it lands, no Duly + * set declares a field permission on `sys_business_unit`, and + * `test/business-unit-read.test.ts` pins that. + * + * No row narrowing either: a predicate cannot reach across a relation, so + * "the viewer's own unit tree" is not expressible; a manager reads tasks + * across `unit_and_below`; and the importer needs every unit by name. No + * scope keys: on a public-default object `readScope` is inert (ADR-0049), + * and there is no write bit for a `writeScope` to bound. + */ + sys_business_unit: { + allowRead: true, + }, } satisfies Record; /** diff --git a/test/business-unit-read.test.ts b/test/business-unit-read.test.ts new file mode 100644 index 0000000..b516bed --- /dev/null +++ b/test/business-unit-read.test.ts @@ -0,0 +1,213 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { AppPlugin, HttpDispatcher, ObjectKernel, createStandaloneStack } from '@objectstack/runtime'; +import type { HttpProtocolContext } from '@objectstack/runtime'; +import { createIdentityObjectsPlugin } from '@objectstack/plugin-auth'; +import { SecurityPlugin, buildContextForUser } from '@objectstack/plugin-security'; +import { SharingServicePlugin } from '@objectstack/plugin-sharing'; + +import stack from '../objectstack.config.js'; +import { AdminPermissionSet, ManagerPermissionSet, MemberPermissionSet } from '../src/security/index.js'; +import { ensureUsers } from './support/users.js'; + +/** + * A Duly user reads business units, and only reads them (#180). + * + * `duly_task.business_unit` and `duly_duty.business_unit` are lookups the + * console resolves under the VIEWER's read. Measured on 18.2.0 before the + * grant: every Duly set got 403 on `sys_business_unit`, so a member's own task + * and duty pages showed the unit as a raw id (`?expand=` silently returned the + * id), and a `duly_admin` who is not a platform administrator could not Import + * `samples/duties.csv` — the Import resolves the unit by name under the + * importer's read and refused 19/19 rows. + * + * REAL holders, resolved by the platform's own `buildContextForUser` with + * `SecurityPlugin` and `SharingServicePlugin` mounted: + * - Owen holds `duly_member`. + * - Marek holds `duly_member` + `duly_manager`. + * - Ida holds `duly_member` + `duly_admin` (the importer). + * + * The console rendering and the Import itself are the browser pass on the PR. + */ + +const SYSTEM = { isSystem: true } as const; + +let kernel: any; +let data: any; +let dispatcher: HttpDispatcher; +let OWEN: HttpProtocolContext; +let MAREK: HttpProtocolContext; +let IDA: HttpProtocolContext; +const UNIT: Record<'central' | 'ops' | 'quality', string> = { central: '', ops: '', quality: '' }; +const NAME = { central: 'Central Office', ops: 'Northgate Operations', quality: 'Northgate Quality' }; + +type Attempt = { ok?: any; thrown?: any }; + +const attempt = async (fn: () => Promise): Promise => { + try { + const ok = await fn(); + const status = ok?.response?.status; + if (typeof status === 'number' && status >= 400) return { thrown: ok.response.body?.error ?? ok.response.body }; + return { ok }; + } catch (thrown) { + return { thrown }; + } +}; + +const body = (res: Attempt) => res.ok?.response?.body ?? res.ok; +const recordOf = (res: Attempt) => { + const b = body(res); + return b?.data?.record ?? b?.record ?? b?.data ?? b; +}; +const recordsOf = (res: Attempt): any[] => { + const b = body(res); + return b?.data?.records ?? b?.records ?? b?.data ?? []; +}; + +const holder = async (ql: any, userId: string, sets: string[]): Promise => { + let org = ''; + for (const permission_set of sets) { + const grant = await data.insert('sys_user_permission_set', { user_id: userId, permission_set }, { context: SYSTEM }); + org = grant.organization_id; + } + return { request: {}, executionContext: await buildContextForUser(ql, userId, Date.now(), org) }; +}; + +const settleProjection = async (expected: Record) => { + const deadline = Date.now() + 30_000; + for (;;) { + const users = await data.find('sys_user', { where: { id: { $in: Object.keys(expected) } } }, { context: SYSTEM }); + const pending = Object.entries(expected).filter( + ([id, unit]) => users.find((u: any) => u.id === id)?.primary_business_unit_id !== unit, + ); + if (pending.length === 0) return; + if (Date.now() > deadline) throw new Error(`primary_business_unit_id never projected for ${pending.map(([id]) => id).join(', ')}`); + await new Promise((resolve) => setTimeout(resolve, 100)); + } +}; + +beforeAll(async () => { + const { plugins } = await createStandaloneStack({ + databaseDriver: 'sqlite', + databaseUrl: ':memory:', + skipSeedData: true, + artifactPath: 'dist/objectstack.this-suite-must-not-load-an-artifact.json', + }); + kernel = new ObjectKernel(); + for (const plugin of plugins) await kernel.use(plugin); + await kernel.use(createIdentityObjectsPlugin()); + await kernel.use(new SharingServicePlugin()); + await kernel.use(new SecurityPlugin()); + await kernel.use(new AppPlugin(stack, undefined, { skipSeedData: true })); + await kernel.bootstrap(); + data = kernel.getService('data'); + const ql = kernel.getService('objectql'); + dispatcher = new HttpDispatcher(kernel); + + await ensureUsers(data, ['user_owen', 'user_marek', 'user_ida']); + for (const key of Object.keys(UNIT) as Array) { + UNIT[key] = String( + (await data.insert('sys_business_unit', { name: NAME[key], code: `C-${key}` }, { context: SYSTEM })).id, + ); + } + const home = { user_owen: UNIT.central, user_marek: UNIT.ops, user_ida: UNIT.quality }; + for (const [user_id, business_unit_id] of Object.entries(home)) { + await data.insert('sys_business_unit_member', { user_id, business_unit_id, is_primary: true }, { context: SYSTEM }); + } + await settleProjection(home); + + OWEN = await holder(ql, 'user_owen', ['duly_member']); + MAREK = await holder(ql, 'user_marek', ['duly_member', 'duly_manager']); + IDA = await holder(ql, 'user_ida', ['duly_member', 'duly_admin']); +}, 180_000); + +afterAll(async () => { + await kernel?.shutdown?.(); +}); + +describe('the declaration', () => { + it('every Duly set reads business units, and that is the whole grant', () => { + for (const set of [MemberPermissionSet, ManagerPermissionSet, AdminPermissionSet]) { + // As the schema parses it: every other bit false, and no scope key. + expect(set.objects.sys_business_unit, set.name).toEqual({ + allowRead: true, + allowCreate: false, + allowEdit: false, + allowDelete: false, + allowTransfer: false, + viewAllRecords: false, + modifyAllRecords: false, + }); + } + }); + + it('no Duly set declares a field permission on sys_business_unit (objectstack#29106)', () => { + // A name-only mask is declarable and works for the member, but field + // permissions merge among DECLARING sets only, so it would also mask every + // administrator who holds a Duly set: measured, Setup → Business Units lost + // Kind, Head and Active. Until #29106 lands, the read is unmasked. + for (const set of [MemberPermissionSet, ManagerPermissionSet, AdminPermissionSet]) { + const onUnit = Object.keys(set.fields ?? {}).filter((key) => key.startsWith('sys_business_unit.')); + expect(onUnit, set.name).toEqual([]); + } + }); +}); + +describe.each([ + ['a member', () => OWEN, () => UNIT.central], + ['a manager', () => MAREK, () => UNIT.ops], + ['a Duly admin', () => IDA, () => UNIT.quality], +] as const)('%s', (_label, as, ownUnit) => { + it('lists every unit in the tenant, by name', async () => { + const res = await attempt(() => dispatcher.handleData('/sys_business_unit', 'GET', {}, {}, as())); + expect(res.thrown, JSON.stringify(res.thrown)).toBeUndefined(); + expect(recordsOf(res).map((u) => u.name).sort()).toEqual(Object.values(NAME).sort()); + }); + + it('reads their own unit by id, name included', async () => { + const res = await attempt(() => dispatcher.handleData(`/sys_business_unit/${ownUnit()}`, 'GET', {}, {}, as())); + expect(res.thrown, JSON.stringify(res.thrown)).toBeUndefined(); + expect(recordOf(res).id).toBe(ownUnit()); + expect(typeof recordOf(res).name).toBe('string'); + }); + + it('resolves a unit BY NAME under their own read — the lookup the Import makes', async () => { + const rows = await data.find( + 'sys_business_unit', + { where: { name: NAME.quality }, fields: ['id', 'name'] }, + { context: as().executionContext }, + ); + expect(rows.map((r: any) => r.id)).toEqual([UNIT.quality]); + }); + + it('sees the unit\'s name on their own hand-made duty when the lookup is expanded', async () => { + const created = await attempt(() => + dispatcher.handleData('/duly_duty', 'POST', { name: `#180 ${_label}`, form: 'recurring', frequency: 'weekly' }, {}, as()), + ); + expect(created.thrown, JSON.stringify(created.thrown)).toBeUndefined(); + const id = String(body(created)?.data?.id ?? body(created)?.id); + const res = await attempt(() => + dispatcher.handleData(`/duly_duty/${id}`, 'GET', {}, { expand: 'business_unit' }, as()), + ); + expect(res.thrown, JSON.stringify(res.thrown)).toBeUndefined(); + const unit = recordOf(res).business_unit; + expect(unit?.id, 'the lookup is expanded, not left as a raw id').toBe(ownUnit()); + expect(unit?.name).toBe(Object.entries(UNIT).map(([k, v]) => (v === ownUnit() ? NAME[k as keyof typeof NAME] : '')) + .find(Boolean)); + }); + + it.each([ + ['create', () => dispatcher.handleData('/sys_business_unit', 'POST', { name: 'Forged unit' }, {}, as())], + ['edit', () => dispatcher.handleData(`/sys_business_unit/${ownUnit()}`, 'PATCH', { name: 'Renamed' }, {}, as())], + ['delete', () => dispatcher.handleData(`/sys_business_unit/${UNIT.quality}`, 'DELETE', {}, {}, as())], + ])('is refused %s', async (_verb, write) => { + const res = await attempt(write); + expect(res.thrown, `${_verb} must be refused`).toBeDefined(); + }); + + it('left every unit exactly as seeded', async () => { + const rows = await data.find('sys_business_unit', {}, { context: SYSTEM }); + expect(rows.map((r: any) => r.name).sort()).toEqual(Object.values(NAME).sort()); + }); +}); diff --git a/test/security.test.ts b/test/security.test.ts index 7d0c9f1..a775468 100644 --- a/test/security.test.ts +++ b/test/security.test.ts @@ -103,6 +103,8 @@ describe('permission sets — declared scopes, object by object', () => { // member set grants no write bit, so both fall to the baseline. duly_catalog_item: { read: 'own', write: 'own' }, duly_assignment: { read: 'own', write: 'own' }, + // #180: read only, no scope keys — see PLATFORM_READS below. + sys_business_unit: { read: 'own', write: 'own' }, }, duly_manager: { // The two ADR-0057 depth grants. Authorable because the stack declares @@ -113,6 +115,8 @@ describe('permission sets — declared scopes, object by object', () => { duly_log_entry: { read: 'own', write: 'own' }, duly_catalog_item: { read: 'own', write: 'own' }, duly_assignment: { read: 'own', write: 'own' }, + // #180: read only, no scope keys — see PLATFORM_READS below. + sys_business_unit: { read: 'own', write: 'own' }, }, duly_admin: { // Inherited from the manager set, not restated. @@ -125,13 +129,14 @@ describe('permission sets — declared scopes, object by object', () => { // edit catalog items they personally created. duly_catalog_item: { read: 'own', write: 'org' }, duly_assignment: { read: 'own', write: 'own' }, + sys_business_unit: { read: 'own', write: 'own' }, }, }; for (const set of ALL_SETS) { const expected = EXPECTED[set.name]; - it(`${set.name} grants exactly the five duly objects`, () => { + it(`${set.name} grants exactly the five duly objects, plus read on business units`, () => { expect(Object.keys(set.objects).sort()).toEqual(Object.keys(expected).sort()); // No wildcard grant anywhere. `'*'` plus a super-user bit is a // package-authored superuser (ADR-0066), and `'*'` alone is a blank @@ -577,11 +582,35 @@ describe('no write scope is wider than own', () => { }); }); +/** + * The platform objects a set may name, and the one grant each may carry. + * + * `sys_business_unit` (#180): the unit lookups on `duly_task` and `duly_duty` + * resolve under the viewer's read, so without it a member sees a raw id and a + * non-platform `duly_admin` cannot Import units by name. READ ONLY, every + * key written out: a write bit, a scope or a super-user bit here would be a + * Duly set editing the org tree. `test/business-unit-read.test.ts` drives it + * against a real member. + */ +const PLATFORM_READS: Record> = { + // As the schema parses it: every other bit false, and no scope key. + sys_business_unit: { + allowRead: true, + allowCreate: false, + allowEdit: false, + allowDelete: false, + allowTransfer: false, + viewAllRecords: false, + modifyAllRecords: false, + }, +}; + describe('objects the sets grant are the objects this app ships', () => { - it('grants nothing outside the duly namespace', () => { + it('grants nothing outside the duly namespace, bar the named platform reads', () => { const shipped = new Set([Task, Duty, LogEntry, CatalogItem, Assignment].map((o) => o.name)); for (const set of ALL_SETS) { for (const objectName of Object.keys(set.objects)) { + if (objectName in PLATFORM_READS) continue; expect(shipped, `${set.name} grants "${objectName}", which this app does not ship`).toContain( objectName, ); @@ -589,6 +618,14 @@ describe('objects the sets grant are the objects this app ships', () => { } }); + it('a named platform read is exactly the declared grant, in every set', () => { + for (const set of ALL_SETS) { + for (const [objectName, grant] of Object.entries(PLATFORM_READS)) { + expect(set.objects[objectName], `${set.name}.${objectName}`).toEqual(grant); + } + } + }); + it('every shipped object is reachable by someone', () => { // A private object with no grant in any set is invisible to every // non-admin caller — the object-level 403 that reads as "the tab is empty".