Skip to content

Commit 7972cb3

Browse files
committed
feat(spec): publish ISecurityService and enforce it at both ends
The `security` service registers seven cross-package methods but had no contract in `@objectstack/spec/contracts`. Consumers duck-typed it and each invented its own fallback for a missing method or an "empty" answer. Adds `ISecurityService` and types BOTH ends against it, so the surface is enforced rather than declared: plugin-security assigns its registration to the interface (a renamed, dropped, or re-typed method fails that build), and the REST layer resolves the service as `Partial<ISecurityService>` (call sites must keep feature-detecting rather than assume the full surface). The contract states the one thing consumers cannot guess — the methods do not share a failure convention. `getReadFilter` fails CLOSED (a failure is a deny filter, never `undefined`; `undefined` means "no row restriction" and nothing else). `getReadableFields` fails SOFT, and its two empty answers are opposites: `undefined` is "no answer, use your own projection", `[]` is authoritative "no field is readable". Typing the producer immediately caught one real discrepancy, fixed here: getReadFilter declared `Record<string, unknown> | null | undefined` while every return path yields a filter or `undefined` (`filter ?? undefined` normalizes the null away). Dropping the dead `| null` leaves "no restriction" with exactly one representation. Type-level only. Tests: spec 6665, plugin-security 593, rest 399 — all passing; the three packages build clean (CJS/ESM/DTS). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KZ2BGusRo58ZW8FMkDhGTb
1 parent 415254c commit 7972cb3

6 files changed

Lines changed: 356 additions & 6 deletions

File tree

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/plugin-security": patch
4+
"@objectstack/rest": patch
5+
---
6+
7+
feat(spec): publish `ISecurityService` — the `security` service surface becomes an enforced contract
8+
9+
The `security` service registers seven cross-package methods (`getReadFilter`,
10+
`getReadableFields`, `resolvePermissionSetNames`, `explain`, and the three
11+
audience-binding suggestion calls) but had no contract in
12+
`@objectstack/spec/contracts`. Consumers duck-typed it, and each one invented its
13+
own fallback for a missing method or an "empty" answer — with more consumers
14+
arriving, that is a drift surface.
15+
16+
`ISecurityService` now documents the surface, and both ends are typed against it
17+
so it is **enforced rather than declared**: `plugin-security` assigns its
18+
registration to `ISecurityService` (a renamed, dropped, or re-typed method fails
19+
that build), and the REST layer resolves the service as a `Partial<ISecurityService>`
20+
(so call sites must keep feature-detecting instead of assuming the full surface).
21+
22+
The contract makes explicit the one thing consumers cannot guess — that the
23+
methods do **not** share a failure convention:
24+
25+
- `getReadFilter` fails **CLOSED**: a resolution failure yields a deny filter
26+
matching zero rows, never `undefined`. `undefined` means "no row restriction",
27+
and nothing else.
28+
- `getReadableFields` fails **SOFT**: `undefined` means "no answer, use your own
29+
projection", while `[]` is authoritative and means "no field is readable" —
30+
opposite instructions that a consumer must not conflate.
31+
32+
Typing the producer immediately caught one real discrepancy, fixed here:
33+
`getReadFilter` declared `Promise<Record<string, unknown> | null | undefined>`
34+
while every return path yields a filter or `undefined` (`filter ?? undefined`
35+
normalizes the null away). The dead `| null` is removed, so "no restriction" has
36+
exactly one representation. Type-level only — no runtime behaviour changes.

packages/plugins/plugin-security/src/security-plugin.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ import {
4747
RLS_MEMBERSHIP_RESOLVER_SERVICE,
4848
RESERVED_RLS_MEMBERSHIP_KEYS,
4949
type IRlsMembershipResolver,
50+
type ISecurityService,
5051
} from '@objectstack/spec/contracts';
5152
import { matchesFilterCondition } from '@objectstack/formula';
5253
import { FieldMasker } from './field-masker.js';
@@ -611,7 +612,11 @@ export class SecurityPlugin implements Plugin {
611612
resolveSets: (context: any) => this.resolvePermissionSetsForContext(context),
612613
logger: ctx.logger,
613614
};
614-
ctx.registerService('security', {
615+
// Typed against the published contract so the registered surface cannot
616+
// drift from what cross-package consumers are promised: a renamed method,
617+
// a dropped one, or a changed return type fails THIS build rather than
618+
// silently degrading a consumer's feature detection at runtime.
619+
const securityService: ISecurityService = {
615620
getReadFilter: (object: string, context?: any) => this.getReadFilter(object, context),
616621
// [#3547] Readable-field projection for a context — the authoritative
617622
// column set for a read-derived export (`export ⊆ list`, #3391).
@@ -632,8 +637,11 @@ export class SecurityPlugin implements Plugin {
632637
// [ADR-0090 D6] First-class access explanation. Same code paths as
633638
// the middleware (resolution/evaluator/RLS compiler) — explained by
634639
// construction. Explaining ANOTHER user requires `manage_users`.
635-
explain: (request: { object: string; operation: string; userId?: string }, callerContext?: any) =>
636-
this.explainAccessForCaller(request, callerContext),
640+
explain: (request, callerContext?: any) =>
641+
this.explainAccessForCaller(
642+
{ ...request, operation: String(request.operation) },
643+
callerContext,
644+
),
637645
// [ADR-0090 D5/D9] Install-time suggestion surface: packages suggest
638646
// audience-anchor bindings; a tenant admin confirms (the binding is
639647
// written under the anchor + delegated-admin gates) or dismisses.
@@ -643,7 +651,8 @@ export class SecurityPlugin implements Plugin {
643651
confirmAudienceBindingSuggestion(suggestionDeps, callerContext, id),
644652
dismissAudienceBindingSuggestion: (callerContext: any, id: string) =>
645653
dismissAudienceBindingSuggestion(suggestionDeps, callerContext, id),
646-
});
654+
};
655+
ctx.registerService('security', securityService);
647656
ctx.logger.info('[security] registered "security" service (getReadFilter, getReadableFields, explain, audience-binding suggestions) — ADR-0021 D-C / ADR-0090 D5/D6/D9 / #3547');
648657
} catch (e) {
649658
ctx.logger.warn?.('[security] failed to register "security" service', {
@@ -2093,7 +2102,7 @@ export class SecurityPlugin implements Plugin {
20932102
async getReadFilter(
20942103
object: string,
20952104
context?: any,
2096-
): Promise<Record<string, unknown> | null | undefined> {
2105+
): Promise<Record<string, unknown> | undefined> {
20972106
// System operations bypass scoping (mirrors the middleware's isSystem skip).
20982107
if (context?.isSystem) return undefined;
20992108
const positions = context?.positions ?? [];

packages/rest/src/rest-server.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { RestServerConfig, RestApiConfig, CrudEndpointsConfig, MetadataEndpoints
1111
import { DataProtocol, MetadataProtocol } from '@objectstack/spec/api';
1212
import { PUBLIC_FORM_SERVER_MANAGED_FIELDS } from '@objectstack/spec/security';
1313
import type { DroppedFieldsEvent } from '@objectstack/spec/data';
14+
import type { ISecurityService } from '@objectstack/spec/contracts';
1415
import {
1516
resolveEffectiveApiMethods,
1617
isApiOperationAllowed,
@@ -4542,8 +4543,15 @@ export class RestServer {
45424543
* Mirrors the resolver in registerSecurityExplainEndpoints. Returns
45434544
* `undefined` when no security service is reachable (no plugin-security /
45444545
* single-kernel without a provider), so callers degrade gracefully.
4546+
*
4547+
* Typed as a PARTIAL {@link ISecurityService}: an implementation may omit a
4548+
* method it cannot honour, so every call site must keep feature-detecting
4549+
* (`typeof svc.x === 'function'`) rather than assume the full surface.
45454550
*/
4546-
private async resolveSecurityService(environmentId?: string, req?: any): Promise<any | undefined> {
4551+
private async resolveSecurityService(
4552+
environmentId?: string,
4553+
req?: any,
4554+
): Promise<Partial<ISecurityService> | undefined> {
45474555
try {
45484556
const envId = await this.resolveRequestEnvironmentId(environmentId, req);
45494557
if (envId && envId !== 'platform' && this.kernelManager) {

packages/spec/src/contracts/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ export * from './workflow-service.js';
3434
export * from './export-service.js';
3535
export * from './email-service.js';
3636
export * from './sms-service.js';
37+
export * from './security-service.js';
3738
export * from './sharing-service.js';
3839
export * from './rls-membership-resolver.js';
3940
export * from './share-link-service.js';
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import type { ISecurityService } from './security-service';
5+
6+
/**
7+
* These tests pin the two things a consumer of the `security` service reasons
8+
* about and that a refactor could silently change: the SHAPE of the surface
9+
* (compile-time) and the MEANING of each "empty" answer (runtime). The second
10+
* matters more than it looks — `undefined` and `[]` from getReadableFields are
11+
* opposite instructions, and a consumer that conflates them either leaks a
12+
* column set or blanks one out.
13+
*/
14+
15+
/** Minimal stub implementing the full surface. */
16+
function makeService(overrides: Partial<ISecurityService> = {}): ISecurityService {
17+
return {
18+
getReadFilter: async () => undefined,
19+
getReadableFields: async () => [],
20+
resolvePermissionSetNames: async () => [],
21+
explain: async () => ({}) as any,
22+
listAudienceBindingSuggestions: async () => ({
23+
suggestions: [],
24+
synced: { created: 0, confirmedObserved: 0, pruned: 0 },
25+
}),
26+
confirmAudienceBindingSuggestion: async () => ({ suggestion: {}, bindingCreated: true }),
27+
dismissAudienceBindingSuggestion: async () => ({ suggestion: {} }),
28+
...overrides,
29+
};
30+
}
31+
32+
describe('Security Service Contract', () => {
33+
it('a full implementation satisfies the surface', () => {
34+
const service = makeService();
35+
for (const m of [
36+
'getReadFilter',
37+
'getReadableFields',
38+
'resolvePermissionSetNames',
39+
'explain',
40+
'listAudienceBindingSuggestions',
41+
'confirmAudienceBindingSuggestion',
42+
'dismissAudienceBindingSuggestion',
43+
] as const) {
44+
expect(typeof service[m]).toBe('function');
45+
}
46+
});
47+
48+
it('getReadFilter: undefined means NO row restriction — the only thing it may mean', async () => {
49+
// A deny is expressed as a filter that matches nothing, never as `undefined`,
50+
// so a consumer can safely read `undefined` as "apply no filter".
51+
const open = makeService({ getReadFilter: async () => undefined });
52+
await expect(open.getReadFilter('deal', { userId: 'u1' })).resolves.toBeUndefined();
53+
54+
const denied = makeService({ getReadFilter: async () => ({ id: { $eq: null } }) as any });
55+
await expect(denied.getReadFilter('deal', { userId: 'u1' })).resolves.toBeDefined();
56+
});
57+
58+
it('getReadableFields: undefined (no answer) and [] (nothing readable) are opposite answers', async () => {
59+
const noAnswer = makeService({ getReadableFields: async () => undefined });
60+
// `undefined` → the caller must fall back to its own projection…
61+
await expect(noAnswer.getReadableFields('deal', { userId: 'u1' })).resolves.toBeUndefined();
62+
63+
const nothingReadable = makeService({ getReadableFields: async () => [] });
64+
// …whereas `[]` is authoritative: expose no columns at all.
65+
await expect(nothingReadable.getReadableFields('deal', { userId: 'u1' })).resolves.toEqual([]);
66+
});
67+
68+
it('a system context is a full field-level bypass', async () => {
69+
const service = makeService({
70+
getReadableFields: async (_object, context) =>
71+
context?.isSystem ? ['id', 'name', 'secret'] : ['id', 'name'],
72+
});
73+
await expect(service.getReadableFields('deal', { isSystem: true }))
74+
.resolves.toEqual(['id', 'name', 'secret']);
75+
await expect(service.getReadableFields('deal', { userId: 'u1' }))
76+
.resolves.toEqual(['id', 'name']);
77+
});
78+
79+
it('a partial implementation is feature-detectable rather than wrong', () => {
80+
// Consumers probe (`typeof svc.getReadableFields === 'function'`) so an
81+
// implementation may omit a method it cannot honour and still be usable.
82+
const partial: Partial<ISecurityService> = { getReadFilter: async () => undefined };
83+
expect(typeof partial.getReadFilter).toBe('function');
84+
expect(typeof partial.getReadableFields).toBe('undefined');
85+
});
86+
87+
it('explain accepts a record-scoped request and an explicit target user', async () => {
88+
const seen: unknown[] = [];
89+
const service = makeService({
90+
explain: async (request) => { seen.push(request); return {} as any; },
91+
});
92+
await service.explain(
93+
{ object: 'deal', operation: 'update', userId: 'u2', recordId: 'r1' },
94+
{ userId: 'admin' },
95+
);
96+
expect(seen[0]).toEqual({ object: 'deal', operation: 'update', userId: 'u2', recordId: 'r1' });
97+
});
98+
});

0 commit comments

Comments
 (0)