diff --git a/.changeset/upgrade-objectstack-v17-rc.md b/.changeset/upgrade-objectstack-v17-rc.md index 622c4b7d..24dde21c 100644 --- a/.changeset/upgrade-objectstack-v17-rc.md +++ b/.changeset/upgrade-objectstack-v17-rc.md @@ -6,3 +6,11 @@ Upgrade HotCRM to ObjectStack 17.0.0-rc.1. The app now declares protocol 17 compatibility, preserves existing required-field database constraints explicitly, uses the live metadata contracts for skills, flows, and APIs, and enables the date buckets that make CRM analytics aggregate by month, day, and quarter. + +Also completes the 17.0 permission migration that the version bump alone leaves +half-done: `allowExport` became an opt-in bit whose absence DENIES, so every +CSV/XLSX list export and every report export is authored explicitly on the +profiles that need it. Approval nodes drop their hand-rolled org-owner backstop +for the native `onEmptyApprovers` policy, media fields declare the `accept` and +`maxSize` constraints 17.0 enforces server-side, and the platform-upgrade +checklist now covers the `os migrate` data gates. diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e4628c9..7c8d4ed7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ Everything merged after the 2.2.2 tag. Not yet versioned or published. ### Changed - **ObjectStack platform → 17.0.0-rc.1** across all `@objectstack/*` packages (from 16.1.0). The manifest protocol handshake and marketplace `specVersion` now declare the 17.0.0-rc.1 compatibility line; API methods, skills and flow notifications use only live metadata keys; pre-17 required fields preserve their database `NOT NULL` constraints explicitly; and the analytics semantic layer now declares the month/day/quarter buckets v17 executes correctly. +- **`allowExport` is authored on every profile that needs it (17.0 opt-in export axis, upstream #3544).** Before 17.0 an unset `allowExport` inherited read, so "can list ⇒ can export". 17.0 inverted the default: `resolveUserExportAllowed` demands an explicit `allowExport: true` and neither `viewAllRecords` nor `modifyAllRecords` substitutes, so an unset bit now DENIES — at both bulk-egress doors, the list views' built-in `exportOptions` and `ReportService.assertExportAllowed`. No profile carried the bit, which would have 403'd every CSV/XLSX and report export for every user, `system_admin` included, while `os validate`, `os build` and the whole test suite stayed green. The five objects with a real export surface (`crm_account`, `crm_case`, `crm_contact`, `crm_lead`, `crm_opportunity`) now carry the grant on each profile that already reads them; `guest_portal` deliberately carries none (ADR-0090 D9 forbids binding a high-privilege set to the `guest` anchor). Pinned by `test/authorization-coverage.test.ts`, which fails both on an export surface nobody can reach and on a grant with no surface behind it. +- Approval nodes in `opportunity_approval` use the native `onEmptyApprovers: 'admin_rescue'` policy instead of the hand-rolled `org_membership_level: 'owner'` approver. That entry existed only to avoid the empty-position dead-end, and with `behavior: 'first_response'` it overshot — it made an org owner a routine approver on every deal over $100K, not a rescue when the bench is empty. +- Media fields (`crm_product.image`, `crm_product.datasheet`, `crm_account.logo`, `crm_contact.avatar`) declare `accept` and `maxSize`. 17.0 enforces both server-side against the stored `sys_file` (ADR-0104 D3 wave 2); before, the upload widget read them but `FieldSchema` dropped them at parse, so the constraint never reached a direct API caller. +- Docs: [`docs/MAINTENANCE.md`](docs/MAINTENANCE.md) §3.2 covers the `os migrate` data gates (`files-to-references`, `value-shapes`) and the `OS_ALLOW_LAX_*` escape hatches. `pnpm verify` and `demo:reset` cannot catch these — they start from an empty database, so a green local run says nothing about an in-place upgrade. - **ObjectStack platform → 16.1.0 stable** across all `@objectstack/*` packages (from the 16.0.0-rc.1 line pinned in 2.2.0). [#465](https://github.com/objectstack-ai/hotcrm/pull/465) - CI: bump `actions/checkout` 4 → 7 ([#422](https://github.com/objectstack-ai/hotcrm/pull/422)) and `actions/setup-node` 4 → 6 ([#421](https://github.com/objectstack-ai/hotcrm/pull/421)). - Docs: README hero states HotCRM's size in tokens ([#483](https://github.com/objectstack-ai/hotcrm/pull/483)); drifted README counts fixed ([#466](https://github.com/objectstack-ai/hotcrm/pull/466)). diff --git a/docs/MAINTENANCE.md b/docs/MAINTENANCE.md index 5a2a0f85..39d10a59 100644 --- a/docs/MAINTENANCE.md +++ b/docs/MAINTENANCE.md @@ -68,6 +68,47 @@ can silently invalidate existing metadata or **seed data** (see §4). Treat ever native binary was built for a different Node ABI — `pnpm rebuild better-sqlite3` and restart. This is an environment issue, not an app change. 7. Note the new platform version in `CHANGELOG.md`. +8. **Check the release notes for `os migrate` steps that run against DATA, not + metadata** — see §3.2. `pnpm verify` cannot catch these: they gate runtime + behaviour on a deployment flag, so a fresh install is clean and an in-place + upgrade is not. + +### 3.2 Data migrations and enforcement gates (`os migrate`) + +Steps 1–7 cover the app's own metadata. A major can additionally ship +migrations that rewrite or re-validate **stored rows**, gated behind a +deployment flag so the new enforcement turns on only once the data is known +clean. `demo:reset` hides these entirely — it starts from an empty database, so +a green local run says nothing about an existing deployment. + +Run the metadata replay first, then each data gate as a dry run before +`--apply`: + +```bash +os migrate meta --from # replays renames and key conversions +os migrate files-to-references # dry run: media fields → sys_file records +os migrate files-to-references --apply # convert, verify, record the flag +os migrate value-shapes # dry run: scan reference & JSON validity +os migrate value-shapes --apply # record the gate if the scan is clean +``` + +For **17.0** specifically: `files-to-references` backfills the four media +fields this app declares (`crm_product.image`, `crm_product.datasheet`, +`crm_account.logo`, `crm_contact.avatar`) into `sys_file` records, and +`value-shapes` scans reference and JSON columns. Neither is needed for a fresh +install — no seed data populates a media field — but an in-place upgrade needs +both before strict validation is safe to enable. + +If a scan reports rows it cannot convert, the escape hatches downgrade the new +enforcement to warnings while you fix the data. They are temporary, not a +destination: + +| Variable | Effect | +| --- | --- | +| `OS_ALLOW_LAX_MEDIA_VALUES=1` | File-value verification warns instead of failing | +| `OS_ALLOW_LAX_VALUE_SHAPES=1` | Reference/JSON validation warns instead of failing | +| `OS_ALLOW_LAX_ACTION_PARAMS=1` | Action-param shape enforcement warns instead of failing | +| `OS_DATA_VALUE_SHAPE_STRICT_ENABLED=1` | Opt into strict value shapes immediately, without the gate | ### 3.1 Destructive schema drift — database-only columns after an upgrade diff --git a/src/flows/opportunity-approval.flow.ts b/src/flows/opportunity-approval.flow.ts index 428eab98..1f5b8a09 100644 --- a/src/flows/opportunity-approval.flow.ts +++ b/src/flows/opportunity-approval.flow.ts @@ -75,15 +75,18 @@ export const OpportunityApprovalFlow: Flow = { type: 'approval', label: 'Sales Manager Review', config: { - // Org-owner backstop: approvers snapshot at request creation, and a - // position with no holders leaves the request undecidable while - // lockRecord holds the record hostage (no admin override exists). - // behavior:'first_response' means whoever responds first wins, so the - // backstop changes nothing when the sales-manager bench is staffed. - approvers: [ - { type: 'position', value: 'sales_manager' }, - { type: 'org_membership_level', value: 'owner' }, - ], + // The empty-position dead-end (approvers snapshot at request creation, + // so a position with no holders left the request undecidable while + // lockRecord held the record hostage) is a NODE POLICY on @objectstack + // 17, not something the approver list has to work around. The + // `{ type: 'org_membership_level', value: 'owner' }` entry that used to + // sit here was that workaround, and it overshot: with + // `behavior: 'first_response'` it made an org owner a routine approver + // for every deal, not just a rescue when the bench is empty. + approvers: [{ type: 'position', value: 'sales_manager' }], + // Explicit even though `admin_rescue` is the schema default — this is + // the mechanism the node depends on, so it is authored, not inherited. + onEmptyApprovers: 'admin_rescue', behavior: 'first_response', lockRecord: true, approvalStatusField: 'approval_status', @@ -104,11 +107,9 @@ export const OpportunityApprovalFlow: Flow = { type: 'approval', label: 'Sales Director Sign-off', config: { - // Same org-owner backstop as manager_review (empty-position dead-end). - approvers: [ - { type: 'position', value: 'sales_director' }, - { type: 'org_membership_level', value: 'owner' }, - ], + // Same node policy as manager_review — see the note there. + approvers: [{ type: 'position', value: 'sales_director' }], + onEmptyApprovers: 'admin_rescue', behavior: 'first_response', lockRecord: true, approvalStatusField: 'approval_status', diff --git a/src/objects/account.object.ts b/src/objects/account.object.ts index 45542ff0..332b7988 100644 --- a/src/objects/account.object.ts +++ b/src/objects/account.object.ts @@ -153,10 +153,13 @@ export const Account = ObjectSchema.create({ group: 'branding', }), - // Company logo (uploaded image) + // Company logo (uploaded image). `accept` / `maxSize` are server-enforced + // from @objectstack 17 — see the note on `crm_product.image`. logo: Field.image({ label: 'Company Logo', group: 'branding', + accept: ['image/png', 'image/jpeg', 'image/svg+xml', 'image/webp'], + maxSize: 2 * 1024 * 1024, }), // Date field diff --git a/src/objects/contact.object.ts b/src/objects/contact.object.ts index ac25da68..9a990038 100644 --- a/src/objects/contact.object.ts +++ b/src/objects/contact.object.ts @@ -58,10 +58,13 @@ export const Contact = ObjectSchema.create({ group: 'identity', }), - // Avatar field + // Avatar field. `accept` / `maxSize` are server-enforced from + // @objectstack 17 — see the note on `crm_product.image`. avatar: Field.avatar({ label: 'Profile Picture', group: 'identity', + accept: ['image/png', 'image/jpeg', 'image/webp'], + maxSize: 2 * 1024 * 1024, }), // Relationship: Link to Account (Master-Detail) diff --git a/src/objects/product.object.ts b/src/objects/product.object.ts index c677cef3..329ac231 100644 --- a/src/objects/product.object.ts +++ b/src/objects/product.object.ts @@ -146,14 +146,22 @@ export const Product = ObjectSchema.create({ }), // Images and Assets + // `accept` / `maxSize` are declarable AND server-enforced from + // @objectstack 17 (ADR-0104 D3 wave 2). Before that the upload widget read + // them but `FieldSchema` dropped them at parse, so the constraint existed + // only in the browser and any direct API caller walked past it. image: Field.image({ label: 'Product Image', group: 'metadata', + accept: ['image/png', 'image/jpeg', 'image/webp'], + maxSize: 5 * 1024 * 1024, }), datasheet: Field.file({ label: 'Datasheet', group: 'metadata', + accept: ['application/pdf'], + maxSize: 20 * 1024 * 1024, }), // Tax & billing diff --git a/src/profiles/guest-portal.profile.ts b/src/profiles/guest-portal.profile.ts index 37b20df6..62d01d04 100644 --- a/src/profiles/guest-portal.profile.ts +++ b/src/profiles/guest-portal.profile.ts @@ -25,6 +25,10 @@ export const GuestPortalProfile = { description: 'Anonymous visitors submitting public Web-to-Lead / Web-to-Case forms. ' + 'INSERT-only on lead and case; no read/edit/delete on any object.', + // No `allowExport` anywhere below, deliberately: this set is bound to the + // `guest` anchor, and ADR-0090 D9 classes `allowExport` as a high-privilege + // bit no anchor may confer — the set would stop binding at all, on top of + // handing anonymous visitors bulk table egress. See `src/profiles/index.ts`. objects: { crm_lead: { allowCreate: true, diff --git a/src/profiles/index.ts b/src/profiles/index.ts index 4376450b..a4a3a89f 100644 --- a/src/profiles/index.ts +++ b/src/profiles/index.ts @@ -2,6 +2,42 @@ /** * Profile Definitions Barrel + * + * ─── `allowExport` — the opt-in bulk-egress axis (@objectstack 17, #3544) ─── + * + * This is the canonical note; the profiles point here. + * + * Before 17.0 the export axis was advisory: an unset `allowExport` inherited + * read, so "can list ⇒ can export" and the bit only ever hid a button. 17.0 + * makes it a real gate and inverts the default — `resolveUserExportAllowed` + * (plugin-security) returns true only for an explicit `allowExport: true`, and + * neither `viewAllRecords` nor `modifyAllRecords` substitutes for it. An unset + * bit now DENIES. Both bulk-egress doors ask before they read: the list views' + * built-in `exportOptions`, and `ReportService.assertExportAllowed`, which + * fails a report export closed with `EXPORT_NOT_PERMITTED`. + * + * So the grants below are authored, not inherited. The rule, pinned by + * `test/authorization-coverage.test.ts`: + * + * a profile grants `allowExport` on an object IFF it already holds + * `allowRead` there AND the app ships an export surface for that object — + * a list view declaring `exportOptions`, or a report whose dataset is + * built on it. + * + * That union is exactly `crm_account`, `crm_case`, `crm_contact`, `crm_lead`, + * `crm_opportunity` today. Adding `exportOptions` to a view (or a report over + * a new dataset) means adding the matching grant here in the same change — the + * guard fails otherwise, which is the point: a surface nobody can use is the + * failure this axis exists to make loud instead of silent. + * + * Export is READ-DERIVED (`export ⊆ list`), so the grant opens the door but + * does not widen the rows: record scope, RLS and sharing still apply on top. A + * `sales_rep` exporting `crm_opportunity` gets their own book, not the org's. + * + * `guest_portal` deliberately carries none. Per ADR-0090 D9 a set holding + * `allowExport` is high-privilege and cannot be bound to the `everyone` or + * `guest` anchors at all — granting it there would both hand anonymous + * visitors bulk table egress and make the set unbindable. */ export { GuestPortalProfile } from './guest-portal.profile'; export { MarketingUserProfile } from './marketing-user.profile'; diff --git a/src/profiles/marketing-user.profile.ts b/src/profiles/marketing-user.profile.ts index b5884f18..041fa54c 100644 --- a/src/profiles/marketing-user.profile.ts +++ b/src/profiles/marketing-user.profile.ts @@ -4,15 +4,19 @@ export const MarketingUserProfile = { name: 'marketing_user', label: 'Marketing User', objects: { - crm_lead: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, - crm_account: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, - crm_contact: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, + // `allowExport` where an export surface exists — canonical note in + // `src/profiles/index.ts`. Lead and contact list exports are marketing's + // core targeting workflow; account/opportunity back the campaign-ROI + // reports. No `crm_case` grant here, so no export bit for it either. + crm_lead: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false, allowExport: true }, + crm_account: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false, allowExport: true }, + crm_contact: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false, allowExport: true }, crm_campaign: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, // Org-wide read: marketing attributes campaign → pipeline ROI, which needs // every opportunity, not just self-owned. viewAllRecords was false here // while every other object on this set is true — an oversight that hid all // pipeline from marketing (and tripped security-private-no-readscope). - crm_opportunity: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, + crm_opportunity: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false, allowExport: true }, // Campaign membership is THIS profile's core write surface: the // "Add to Campaign" action (`src/actions/lead.actions.ts`) inserts // `crm_campaign_member` rows, and before #488 no permission set granted the diff --git a/src/profiles/sales-manager.profile.ts b/src/profiles/sales-manager.profile.ts index cfb2e5d0..e9ab32c0 100644 --- a/src/profiles/sales-manager.profile.ts +++ b/src/profiles/sales-manager.profile.ts @@ -10,15 +10,18 @@ export const SalesManagerProfile = { name: 'sales_manager', label: 'Sales Manager', objects: { - crm_lead: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, - crm_account: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, - crm_contact: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, - crm_opportunity: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, + // `allowExport` where an export surface exists — canonical note in + // `src/profiles/index.ts`. A manager owns the number, so the pipeline and + // book exports that feed offline forecasting are part of the job. + crm_lead: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true, allowExport: true }, + crm_account: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true, allowExport: true }, + crm_contact: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true, allowExport: true }, + crm_opportunity: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true, allowExport: true }, crm_quote: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, crm_contract: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, crm_product: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, crm_campaign: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, - crm_case: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, + crm_case: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false, allowExport: true }, crm_task: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, // The forecast IS the manager's job: they read every rep's snapshot and // adjust the committed number, so this is org-wide read AND write on a diff --git a/src/profiles/sales-rep.profile.ts b/src/profiles/sales-rep.profile.ts index b319d19c..6a168285 100644 --- a/src/profiles/sales-rep.profile.ts +++ b/src/profiles/sales-rep.profile.ts @@ -12,21 +12,24 @@ export const SalesRepProfile = { // need cross-team visibility get it through the account team / territory // sharing rules, not by widening the base set. objects: { - crm_lead: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const }, - crm_account: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const }, + // `allowExport` where an export surface exists — canonical note in + // `src/profiles/index.ts`. Safe alongside `readScope: 'own'`: export is + // read-derived, so a rep's CSV carries their own book, not the org's. + crm_lead: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const, allowExport: true }, + crm_account: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const, allowExport: true }, // NO readScope: `crm_contact` is `controlled_by_parent` (master-detail to // crm_account), so its rows derive from the account the rep can read — // territory- and team-shared accounts included. A `readScope` here was // inert (the sharing service only applies owner scope to `private` objects) // and read as a promise the engine never kept: it said "own contacts only" // while access actually followed the account (#488). - crm_contact: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false }, - crm_opportunity: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const }, + crm_contact: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, allowExport: true }, + crm_opportunity: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const, allowExport: true }, crm_quote: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const }, crm_contract: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const }, crm_product: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, crm_campaign: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, - crm_case: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const }, + crm_case: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const, allowExport: true }, crm_task: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const }, // Reference catalog (public_read OWD): reps read knowledge articles, which // are authored by service. diff --git a/src/profiles/service-agent.profile.ts b/src/profiles/service-agent.profile.ts index 75c0ec1b..46faa8ff 100644 --- a/src/profiles/service-agent.profile.ts +++ b/src/profiles/service-agent.profile.ts @@ -10,13 +10,16 @@ export const ServiceAgentProfile = { // security-private-no-readscope warning's real signal: allowRead on a // private object with no scope had silently locked agents out of every // account they didn't personally own. - crm_lead: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, - crm_account: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, - crm_contact: { allowCreate: false, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, + // `allowExport` where an export surface exists — canonical note in + // `src/profiles/index.ts`. `crm_opportunity` carries no export bit: this + // set has no read on it at all, and the axis never widens read. + crm_lead: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false, allowExport: true }, + crm_account: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false, allowExport: true }, + crm_contact: { allowCreate: false, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: false, allowExport: true }, crm_opportunity: { allowCreate: false, allowRead: false, allowEdit: false, allowDelete: false, viewAllRecords: false, modifyAllRecords: false }, // Cases + tasks: an agent's own queue by default (readScope: 'own'); // cross-agent visibility comes from the case-escalation sharing rule. - crm_case: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const }, + crm_case: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const, allowExport: true }, crm_task: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: false, modifyAllRecords: false, readScope: 'own' as const }, crm_product: { allowCreate: false, allowRead: true, allowEdit: false, allowDelete: false, viewAllRecords: true, modifyAllRecords: false }, // The knowledge base is this team's own surface: agents draft and revise diff --git a/src/profiles/system-admin.profile.ts b/src/profiles/system-admin.profile.ts index e9847e47..c87abea8 100644 --- a/src/profiles/system-admin.profile.ts +++ b/src/profiles/system-admin.profile.ts @@ -14,15 +14,19 @@ export const SystemAdminProfile = { name: 'system_admin', label: 'System Administrator', objects: { - crm_lead: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, - crm_account: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, - crm_contact: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, - crm_opportunity: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, + // `allowExport` on the five objects with an export surface — see the + // canonical note in `src/profiles/index.ts`. It is authored even here: + // 17.0 gates export on the explicit bit, and `modifyAllRecords` does not + // stand in for it, so without these an admin cannot export either. + crm_lead: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true, allowExport: true }, + crm_account: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true, allowExport: true }, + crm_contact: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true, allowExport: true }, + crm_opportunity: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true, allowExport: true }, crm_quote: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, crm_contract: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, crm_product: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, crm_campaign: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, - crm_case: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, + crm_case: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true, allowExport: true }, crm_task: { allowCreate: true, allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, // The objects below shipped with navigation, views, hooks and seed data // but no grant in ANY permission set (#488): "Knowledge" and "Forecasts" diff --git a/test/authorization-coverage.test.ts b/test/authorization-coverage.test.ts index 61d13552..14f0d99b 100644 --- a/test/authorization-coverage.test.ts +++ b/test/authorization-coverage.test.ts @@ -448,3 +448,107 @@ describe('#488 regressions stay fixed', () => { expect(readable, 'the guest set must stay INSERT-only').toEqual([]); }); }); + +/** + * `allowExport` coverage — the opt-in bulk-egress axis (@objectstack 17, #3544). + * + * 17.0 inverted this bit's default: unset used to inherit read, so the axis + * only hid a button; now `resolveUserExportAllowed` (plugin-security) demands + * an explicit `allowExport: true` and neither `viewAllRecords` nor + * `modifyAllRecords` substitutes. Unset DENIES, at both bulk-egress doors — + * the list views' built-in `exportOptions`, and `assertExportAllowed`, which + * fails a report export closed with `EXPORT_NOT_PERMITTED`. + * + * That makes an unauthored export bit a SILENT outage: `os validate`, `os + * build` and every metadata test still pass while the Export button 403s for + * every user, admins included. Nothing else in this suite would catch it, + * because the grant is well-formed — it just isn't there. + * + * The rule these guards pin (canonical note: `src/profiles/index.ts`): + * a profile grants `allowExport` on an object IFF it already holds `allowRead` + * there AND the app ships an export surface for it. Both directions matter — + * a surface nobody can use is the outage, and a grant behind no surface is + * bulk egress nobody asked for. + */ +describe('allowExport tracks the app’s real export surfaces', () => { + const datasetByName = new Map( + ((stack as any).datasets ?? []).map((d: AnyRec) => [d.name as string, d] as [string, AnyRec]), + ); + const views: AnyRec[] = (stack as any).views ?? []; + const reports: AnyRec[] = (stack as any).reports ?? []; + + /** Every `dataset:` reference anywhere in a report, including `blocks[]`. */ + const datasetRefsIn = (node: unknown, out: string[] = []): string[] => { + if (Array.isArray(node)) node.forEach((n) => datasetRefsIn(n, out)); + else if (node && typeof node === 'object') { + for (const [k, v] of Object.entries(node as AnyRec)) { + if (k === 'dataset' && typeof v === 'string') out.push(v); + else datasetRefsIn(v, out); + } + } + return out; + }; + + /** + * Objects with a bulk-egress door: a list view declaring `exportOptions`, + * or a report whose dataset is built on them. + */ + const exportSurfaces = new Set(); + for (const v of views) { + const defaultObject = v.list?.data?.object; + for (const list of [v.list, ...Object.values(v.listViews ?? {})].filter(Boolean) as AnyRec[]) { + if (!(list.exportOptions ?? []).length) continue; + const objectName = list.data?.object ?? defaultObject; + if (typeof objectName === 'string') exportSurfaces.add(objectName); + } + } + for (const r of reports) { + for (const ref of datasetRefsIn(r)) { + const objectName = datasetByName.get(ref)?.object; + if (typeof objectName === 'string') exportSurfaces.add(objectName); + } + } + + /** `[setName, objectName]` for every grant carrying the export bit. */ + const exportGrants: Array<[string, string, AnyRec]> = permissionSets.flatMap((ps) => + Object.entries((ps.objects ?? {}) as Record) + .filter(([, perm]) => perm.allowExport === true) + .map(([objectName, perm]) => [ps.name as string, objectName, perm] as [string, string, AnyRec]), + ); + + it('the app actually has export surfaces (the guard is wired to real metadata)', () => { + expect( + [...exportSurfaces].sort(), + 'no exportOptions view and no report dataset resolved — this guard has gone blind', + ).not.toEqual([]); + }); + + it('every export surface is reachable by at least one profile', () => { + const stranded = [...exportSurfaces] + .filter((objectName) => !exportGrants.some(([, granted]) => granted === objectName)) + .map((objectName) => `${objectName}: has an export surface, but no permission set grants allowExport`); + expect(stranded, `export surfaces nobody can use:\n ${stranded.join('\n ')}`).toEqual([]); + }); + + it('no profile grants export on an object with no export surface', () => { + const gratuitous = exportGrants + .filter(([, objectName]) => !exportSurfaces.has(objectName)) + .map(([set, objectName]) => `${set}.${objectName}: allowExport with no export surface behind it`); + expect(gratuitous, `undeclared bulk egress:\n ${gratuitous.join('\n ')}`).toEqual([]); + }); + + it('export never outruns read — the axis widens egress, never visibility', () => { + const bad = exportGrants + .filter(([, , perm]) => perm.allowRead !== true) + .map(([set, objectName]) => `${set}.${objectName}: allowExport without allowRead`); + expect(bad, `export grants with no read behind them:\n ${bad.join('\n ')}`).toEqual([]); + }); + + it('the guest set carries no export bit (ADR-0090 D9 anchor rule)', () => { + // `allowExport` is high-privilege: a set holding it cannot bind to the + // `everyone` or `guest` anchors at all, so granting it here would break the + // binding on top of handing anonymous visitors bulk table egress. + const guestExports = exportGrants.filter(([set]) => set === 'guest_portal').map(([, o]) => o); + expect(guestExports, 'the guest set must never carry allowExport').toEqual([]); + }); +}); diff --git a/test/metadata-references.test.ts b/test/metadata-references.test.ts index 4432bfc0..a749b96e 100644 --- a/test/metadata-references.test.ts +++ b/test/metadata-references.test.ts @@ -721,16 +721,30 @@ describe('object references outside views resolve', () => { describe('list-level action references resolve', () => { const actions: AnyRec[] = (stack as any).actions ?? []; const actionNames = new Set(actions.map((a) => a.name)); - // Row/bulk affordances the list renderer provides without an Action def. - const BUILTIN = new Set(['edit', 'delete', 'view']); + /** + * There is NO builtin escape hatch (@objectstack 17). + * + * This guard used to whitelist `edit` / `delete` / `view` as "affordances the + * list renderer provides without an Action def". 17.0's `action-name-undefined` + * validator rule refuses exactly that: a `rowActions` string naming no defined + * action is reported as a dead affordance — "the button renders and does + * nothing when clicked" — and `os validate` fails the build. + * + * So the whitelist was a trap: it let metadata pass CI that the platform then + * rejected. Row-menu entries come from an Action declaring + * `locations: ['list_item']`, which auto-injects them; naming one here as a + * string is the legacy path the next test forbids. Between the two rules, + * `rowActions` has no correct use in this app today — which is why no view + * declares it. + */ it('every rowAction / bulkAction names a defined action', () => { const bad: string[] = []; for (const v of views) { const lists = [v.list, ...Object.values(v.listViews ?? {})].filter(Boolean) as AnyRec[]; for (const list of lists) { for (const name of [...(list.rowActions ?? []), ...(list.bulkActions ?? [])]) { - if (typeof name === 'string' && !BUILTIN.has(name) && !actionNames.has(name)) { + if (typeof name === 'string' && !actionNames.has(name)) { bad.push(`view "${list.name ?? 'default'}": action "${name}" is not defined`); } }