Skip to content

Commit 97c72a0

Browse files
os-zhuangclaude
andauthored
fix(types,app-shell): one ObjectPermission, and the preview stops hiding three of its fields (objectstack#4115) (#3077)
objectui had two mutually incompatible types named `ObjectPermission`. Neither was the spec's. **The legacy one was dead** — `field-types.ts` declared a pre-spec shape (`profile` / `roles` / `create|read|update|delete` / `record_level` / `field_permissions`) whose only referent was `ObjectSchemaMetadata. permissions`, and `ObjectSchemaMetadata` itself has no consumer outside `packages/types`. Nothing in the repo reads `permissions?.create`, `record_level` or `own_records_only`. Its `SharingRule` was likewise reachable only through `record_level.sharing_rules`. Renaming would have kept dead weight under a new name, so the whole block is deleted; spec's `PermissionSet` / `ObjectPermission` is the model that survives. **The other was a hand copy that had silently gone stale.** `PermissionPreview` declared its own 9-key `ObjectPermission` against the spec's 12. The three it omitted were not cosmetic: `allowExport`, and the ADR-0057 access-depth axis `readScope` / `writeScope`. A permission set granting export — or writing wider than it reads — rendered *identically* to one that did neither, so the reviewer the preview exists to inform had no way to see it. It now imports the spec types, and the matrix gains an Export column and a read/write Scope column. Because `CAPS` is keyed by `keyof ObjectPermission`, a capability the spec renames is now a compile error rather than a column that quietly vanishes. Also adds the warning that axis makes checkable, in the same class as the existing Modify-All-without-View-All one: a write scope wider than the read scope means edits can target records the user cannot see. Ledger 126 -> 124, regenerated with `--ledger`, diff is exactly `ObjectPermission` and `SharingRule`. Mutation-tested: removing the Export column and disabling the scope warning each fail their assertion by name. 78/78 type-check; full suite 8786 green. BREAKING CHANGE: `ObjectPermission` and `SharingRule` are removed from `@object-ui/types`, along with `ObjectSchemaMetadata.permissions`. Consumers that want object permissions should use `ObjectPermission` from `@objectstack/spec/security`. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent df613fa commit 97c72a0

5 files changed

Lines changed: 127 additions & 93 deletions

File tree

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* PermissionPreview ↔ spec `ObjectPermission` (objectstack#4115).
5+
*
6+
* The preview used to declare its own 9-key `ObjectPermission` interface, a
7+
* hand copy of the spec's 12-key one. The three it omitted were not cosmetic:
8+
* `allowExport` and the ADR-0057 access-depth axis (`readScope`/`writeScope`).
9+
* A permission set granting export, or writing wider than it reads, rendered
10+
* identically to one that did neither — the reviewer had no way to see it.
11+
*
12+
* These tests pin the three formerly-invisible fields. The typing is what
13+
* keeps them honest going forward: `CAPS` is keyed by
14+
* `keyof ObjectPermission`, so a capability the spec renames breaks the build
15+
* rather than silently vanishing from the matrix.
16+
*/
17+
18+
import { describe, it, expect, afterEach } from 'vitest';
19+
import { render, screen, cleanup, within } from '@testing-library/react';
20+
import { PermissionPreview } from './PermissionPreview';
21+
22+
afterEach(cleanup);
23+
24+
const baseProps = { type: 'permission', name: 'sales_rep', locale: 'en-US' as const };
25+
26+
function renderPreview(objects: Record<string, unknown>) {
27+
return render(
28+
<PermissionPreview
29+
{...(baseProps as never)}
30+
draft={{ name: 'sales_rep', label: 'Sales Rep', objects }}
31+
/>,
32+
);
33+
}
34+
35+
describe('PermissionPreview surfaces the spec fields the hand copy omitted', () => {
36+
it('renders an Export column — allowExport used to be undeclared entirely', () => {
37+
renderPreview({ opportunity: { allowRead: true, allowExport: true } });
38+
const header = screen.getByTitle('Export');
39+
expect(header).toBeTruthy();
40+
expect(header.textContent).toBe('E');
41+
});
42+
43+
it('shows the read/write access depth instead of dropping it', () => {
44+
renderPreview({ opportunity: { allowRead: true, readScope: 'unit', writeScope: 'own' } });
45+
const row = screen.getByText('opportunity').closest('tr')!;
46+
const cells = within(row).getAllByRole('cell');
47+
const scopeCell = cells[cells.length - 1];
48+
expect(scopeCell.textContent).toContain('unit');
49+
expect(scopeCell.textContent).toContain('own');
50+
});
51+
52+
it('marks an object with no scopes set as using the default', () => {
53+
renderPreview({ opportunity: { allowRead: true } });
54+
const row = screen.getByText('opportunity').closest('tr')!;
55+
const cells = within(row).getAllByRole('cell');
56+
expect(cells[cells.length - 1].textContent).toBe('default');
57+
});
58+
});
59+
60+
describe('scope warnings (same class as Modify-All without View-All)', () => {
61+
it('warns when the write scope is wider than the read scope', () => {
62+
// Editable-but-invisible records: the reviewer needs this called out, and
63+
// before the spec fields were declared the preview could not even see it.
64+
renderPreview({ opportunity: { allowRead: true, allowEdit: true, readScope: 'own', writeScope: 'unit' } });
65+
expect(screen.getByText(/Write scope \(unit\) is wider than read scope \(own\)/)).toBeTruthy();
66+
});
67+
68+
it('stays quiet when write is no wider than read', () => {
69+
renderPreview({ opportunity: { allowRead: true, allowEdit: true, readScope: 'org', writeScope: 'own' } });
70+
expect(screen.queryByText(/wider than read scope/)).toBeNull();
71+
});
72+
73+
it('stays quiet when neither scope is set', () => {
74+
renderPreview({ opportunity: { allowRead: true, allowEdit: true } });
75+
expect(screen.queryByText(/wider than read scope/)).toBeNull();
76+
});
77+
78+
it('still reports the pre-existing capability warnings', () => {
79+
renderPreview({ opportunity: { allowEdit: true, modifyAllRecords: true } });
80+
expect(screen.getByText(/Edit granted without Read/)).toBeTruthy();
81+
expect(screen.getByText(/Modify All without View All/)).toBeTruthy();
82+
});
83+
});

packages/app-shell/src/views/metadata-admin/previews/PermissionPreview.tsx

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -37,38 +37,41 @@ import {
3737
Star,
3838
Tag,
3939
} from 'lucide-react';
40+
import type { ObjectAccessScope, ObjectPermission, FieldPermission } from '@objectstack/spec/security';
4041
import type { MetadataPreviewProps } from '../preview-registry';
4142
import { PreviewShell, PreviewMessage, PreviewErrorBoundary } from './PreviewShell';
4243

43-
interface ObjectPermission {
44-
allowCreate?: boolean;
45-
allowRead?: boolean;
46-
allowEdit?: boolean;
47-
allowDelete?: boolean;
48-
allowTransfer?: boolean;
49-
allowRestore?: boolean;
50-
allowPurge?: boolean;
51-
viewAllRecords?: boolean;
52-
modifyAllRecords?: boolean;
53-
}
54-
55-
interface FieldPermission {
56-
readable?: boolean;
57-
editable?: boolean;
58-
}
59-
44+
/**
45+
* Boolean capabilities, in matrix-column order.
46+
*
47+
* Typed against the spec's `ObjectPermission` so a capability the spec adds
48+
* cannot stay invisible here indefinitely — the hand-written copy this
49+
* replaced was missing `allowExport` outright, so a permission set granting
50+
* export rendered identically to one that did not (objectstack#4115).
51+
*/
6052
const CAPS: Array<{ key: keyof ObjectPermission; short: string; long: string; danger?: boolean }> = [
6153
{ key: 'allowCreate', short: 'C', long: 'Create' },
6254
{ key: 'allowRead', short: 'R', long: 'Read' },
6355
{ key: 'allowEdit', short: 'U', long: 'Edit' },
6456
{ key: 'allowDelete', short: 'D', long: 'Delete' },
57+
{ key: 'allowExport', short: 'E', long: 'Export' },
6558
{ key: 'allowTransfer', short: 'T', long: 'Transfer' },
6659
{ key: 'allowRestore', short: 'Re', long: 'Restore' },
6760
{ key: 'allowPurge', short: 'P', long: 'Purge', danger: true },
6861
{ key: 'viewAllRecords', short: 'V*', long: 'View All', danger: true },
6962
{ key: 'modifyAllRecords', short: 'M*', long: 'Modify All', danger: true },
7063
];
7164

65+
/** Access-depth axis (ADR-0057), narrowest first — also the widening order. */
66+
const SCOPE_ORDER: ObjectAccessScope[] = ['own', 'own_and_reports', 'unit', 'unit_and_below', 'org'];
67+
const SCOPE_LABEL: Record<ObjectAccessScope, string> = {
68+
own: 'own',
69+
own_and_reports: 'own+reports',
70+
unit: 'unit',
71+
unit_and_below: 'unit+below',
72+
org: 'org',
73+
};
74+
7275
interface Warning {
7376
object: string;
7477
message: string;
@@ -81,6 +84,16 @@ function findWarnings(objects: Record<string, ObjectPermission>): Warning[] {
8184
if (p.allowDelete && !p.allowRead) out.push({ object: obj, message: 'Delete granted without Read.' });
8285
if (p.modifyAllRecords && !p.viewAllRecords) out.push({ object: obj, message: 'Modify All without View All — modifications may target invisible records.' });
8386
if (p.allowPurge && !p.allowDelete) out.push({ object: obj, message: 'Purge (hard delete) granted without Delete.' });
87+
// Same class as Modify-All-without-View-All, one axis down: a write scope
88+
// wider than the read scope lets a user edit records they cannot see.
89+
const read = p.readScope ? SCOPE_ORDER.indexOf(p.readScope) : -1;
90+
const write = p.writeScope ? SCOPE_ORDER.indexOf(p.writeScope) : -1;
91+
if (read >= 0 && write > read) {
92+
out.push({
93+
object: obj,
94+
message: `Write scope (${SCOPE_LABEL[p.writeScope!]}) is wider than read scope (${SCOPE_LABEL[p.readScope!]}) — edits may target invisible records.`,
95+
});
96+
}
8497
}
8598
return out;
8699
}
@@ -167,6 +180,9 @@ export function PermissionPreview({ name, draft }: MetadataPreviewProps) {
167180
{c.short}
168181
</th>
169182
))}
183+
<th className="px-2 py-1.5 text-left font-medium" title="Read / write access depth (ADR-0057)">
184+
Scope
185+
</th>
170186
</tr>
171187
</thead>
172188
<tbody className="divide-y">
@@ -183,6 +199,17 @@ export function PermissionPreview({ name, draft }: MetadataPreviewProps) {
183199
</td>
184200
);
185201
})}
202+
<td className="px-2 py-1 whitespace-nowrap text-[10px] text-muted-foreground">
203+
{p.readScope || p.writeScope ? (
204+
<>
205+
<span title="Read scope">{p.readScope ? SCOPE_LABEL[p.readScope] : '—'}</span>
206+
<span className="mx-1 opacity-50">/</span>
207+
<span title="Write scope">{p.writeScope ? SCOPE_LABEL[p.writeScope] : '—'}</span>
208+
</>
209+
) : (
210+
<span className="opacity-40">default</span>
211+
)}
212+
</td>
186213
</tr>
187214
);
188215
})}

packages/types/src/field-types.ts

Lines changed: 0 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -784,73 +784,6 @@ export interface ObjectTrigger {
784784
config?: Record<string, any>;
785785
}
786786

787-
/**
788-
* Object permission configuration (Phase 3.1.4)
789-
*/
790-
export interface ObjectPermission {
791-
/**
792-
* Permission profile name
793-
*/
794-
profile?: string;
795-
/**
796-
* Role-based permissions
797-
*/
798-
roles?: string[];
799-
/**
800-
* CRUD permissions
801-
*/
802-
create?: boolean;
803-
read?: boolean;
804-
update?: boolean;
805-
delete?: boolean;
806-
/**
807-
* Record-level permissions
808-
*/
809-
record_level?: {
810-
/**
811-
* User can only access own records
812-
*/
813-
own_records_only?: boolean;
814-
/**
815-
* Sharing rules
816-
*/
817-
sharing_rules?: SharingRule[];
818-
};
819-
/**
820-
* Field-level permissions
821-
*/
822-
field_permissions?: Record<string, {
823-
read?: boolean;
824-
edit?: boolean;
825-
}>;
826-
}
827-
828-
/**
829-
* Sharing rule configuration
830-
*/
831-
export interface SharingRule {
832-
/**
833-
* Rule name
834-
*/
835-
name: string;
836-
/**
837-
* Criteria for sharing
838-
*/
839-
criteria?: Record<string, any>;
840-
/**
841-
* Access level granted
842-
*/
843-
access_level: 'read' | 'edit' | 'full';
844-
/**
845-
* Share with users/roles
846-
*/
847-
share_with?: {
848-
users?: string[];
849-
roles?: string[];
850-
groups?: string[];
851-
};
852-
}
853-
854787
/**
855788
* Object schema definition
856789
* Phase 3.1: Enhanced with inheritance, triggers, permissions, and caching
@@ -876,11 +809,6 @@ export interface ObjectSchemaMetadata {
876809
*/
877810
fields: Record<string, FieldMetadata>;
878811

879-
/**
880-
* Permissions (Phase 3.1.4: Enhanced permissions)
881-
*/
882-
permissions?: ObjectPermission;
883-
884812
/**
885813
* Parent object to inherit from (Phase 3.1.2)
886814
*/

packages/types/src/index.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -428,8 +428,6 @@ export type {
428428
MasterDetailFieldMetadata,
429429
FieldMetadata,
430430
ObjectTrigger,
431-
ObjectPermission,
432-
SharingRule,
433431
ObjectSchemaMetadata,
434432
ObjectIndex,
435433
ObjectRelationship,

scripts/check-spec-symbol-derivation.mjs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,6 @@ const DEBT = {
157157
"NavigationAreaSchema",
158158
"NavigationItem",
159159
"NavigationItemSchema",
160-
"ObjectPermission",
161160
"OfflineConfig",
162161
"PageRegion",
163162
"PageRegionSchema",
@@ -167,7 +166,6 @@ const DEBT = {
167166
"ScriptValidation",
168167
"SelectOption",
169168
"SelectOptionSchema",
170-
"SharingRule",
171169
"StateMachineValidation",
172170
"Theme",
173171
"WidgetManifest",

0 commit comments

Comments
 (0)