Skip to content

Commit 123f5d3

Browse files
os-zhuangclaude
andcommitted
fix(spec,plugins): sweep the auth/session slot lookups (#4251)
Batch B2 of the #4251 sweep: every service-lookup erasure in the auth/session family. plugin-auth/auth-plugin.ts (20), plugin-hono-server/current-user-endpoints.ts (10) and plugin-security/security-plugin.ts (1) now pass the slot's contract type; the ratchet baseline drops 171 -> 140 sites, 40 -> 37 files, and all three files leave the grandfather list. The yield: POST /admin/import-users resolved the `metadata` slot and probed `metadataService?.getMetaItem` to decide whether to pass the import's field-coercion dependency. `getMetaItem` is a PROTOCOL method (ObjectStackProtocolImplementation, registered under `protocol`); MetadataManager, which occupies `metadata`, has never had it. The probe was false on every deployment and the dep was never passed -- imported rows reached sys_user uncoerced, with the branch claiming otherwise sitting right there. Same shape as #4127's dead automation.trigger. The route reads `protocol` now. /me/apps reached ObjectQL's PRIVATE `_registry` through `as any` while /auth/me/permissions, two handlers up in the same file, read the public `registry` getter over the same field of the same object. Both read the public accessor now; the one test that stubbed `_registry` was pinning that private reach and stubs `registry`. Contract, from evidence: IDataEngine's read methods (find / findOne / count / aggregate) declare the trailing `options?: BaseEngineOptions` argument they have always accepted. ObjectQL's own doc explains why it exists -- the same `{ context }` object was correct as insert's 3rd argument and SILENTLY DROPPED as find's, so an intended isSystem bypass just vanished. The contract exposed only query.context, so callers using the trailing channel could reach it only by erasing the lookup. Adding an optional trailing parameter breaks no implementor or caller; BaseEngineOptions was already exported but sat unused under the legacy/deprecated heading, and moves up with the rationale attached. One new spec test pins the argument at the call site. Where the contract does not reach, the escape hatch is named rather than `any`: PermissionEvaluatorSurface (security.permissions), SettingsReadSurface (settings), EngineRegistrySurface / EngineExtensionSurface (objectql beyond IDataEngine). That last one is deliberate scope -- the standing record on getObjectQL in @objectstack/ runtime says ObjectQL is genuinely wider than IDataEngine and typing the whole thing IDataEngine would be the more comfortable-looking lie. These declarations are what that contract gets written from, and what it deletes. Verified: full-repo typecheck 110/110; spec 7157/278, objectql 1394/87, runtime 951/66, plugin-auth 579/26, plugin-security 677/32, plugin-hono-server 135/12; eslint clean on every changed file; check:slot-lookup 140 sites in 37 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 6a67d7a commit 123f5d3

9 files changed

Lines changed: 362 additions & 50 deletions

File tree

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/plugin-auth": patch
4+
"@objectstack/plugin-hono-server": patch
5+
"@objectstack/plugin-security": patch
6+
---
7+
8+
fix(spec,plugins): sweep the auth/session slot lookups — 31 sites typed, and the user-import metadata reader was pointed at a service that never had the method (#4251)
9+
10+
Batch B2 of the #4251 sweep: every service-lookup erasure in the auth/session
11+
family. `plugin-auth/auth-plugin.ts` (20), `plugin-hono-server/current-user-endpoints.ts`
12+
(10) and `plugin-security/security-plugin.ts` (1) now pass the slot's contract
13+
type; the ratchet baseline drops **171 → 140 sites, 40 → 37 files**.
14+
15+
**The yield.** `POST /admin/import-users` resolved the `metadata` slot and probed
16+
`metadataService?.getMetaItem` to decide whether to pass the import's field-coercion
17+
dependency. `getMetaItem` is a **protocol** method — `ObjectStackProtocolImplementation`,
18+
registered by MetadataProtocolPlugin under the `protocol` slot. `MetadataManager`,
19+
which occupies `metadata`, has never had it. So the probe was false on every
20+
deployment and the dep was never passed: imported rows reached `sys_user`
21+
uncoerced, with the branch that says otherwise sitting right there. This is the
22+
same shape as #4127's dead `automation.trigger` and #4321's `registerInMemory`
23+
probes — a capability the code advertises and the runtime cannot deliver, kept
24+
invisible by the `any`. Typing the lookup to `IMetadataService` is what turned it
25+
into a compile error. The route reads `protocol` now.
26+
27+
`/me/apps` reached ObjectQL's **private** `_registry` through `as any` while
28+
`/auth/me/permissions`, two handlers up in the same file, read the public
29+
`registry` getter over the same field of the same object. Both read the public
30+
accessor now; the one test that stubbed `_registry` was pinning the private reach
31+
and stubs `registry` instead.
32+
33+
**Contract, from evidence.** `IDataEngine`'s read methods (`find` / `findOne` /
34+
`count` / `aggregate`) declare the trailing `options?: BaseEngineOptions`
35+
argument they have always accepted. ObjectQL's own doc explains why it exists:
36+
reads once took their context inside the query while writes took it in trailing
37+
`options.context`, so the same `{ context }` object was correct as `insert`'s 3rd
38+
argument and **silently dropped** as `find`'s — "an intended `isSystem` bypass
39+
just vanished". The engine accepts both channels; the contract exposed only the
40+
query one, so callers using the trailing channel — the current-user endpoints'
41+
permission-set loader among them — could only reach it by erasing the lookup.
42+
Adding an optional trailing parameter breaks no implementor (the existing
43+
minimal-implementation test proves it) and no caller. `BaseEngineOptions` was
44+
already exported, sitting unused under the "legacy/deprecated" heading, which is
45+
why the contract went looking and did not find it; it moves up beside the other
46+
QueryAST-aligned types with the rationale attached. One new spec test pins the
47+
trailing argument at the call site — the position where the old contract rejected it.
48+
49+
**Where the contract does not reach, the escape hatch is named.** Three slots
50+
resist a spec type today and each gets a narrow, documented local interface
51+
instead of `any`: `security.permissions` (plugin-security's `PermissionEvaluator`
52+
— plugin-hono-server must not depend on an optional plugin), `settings`
53+
(service-settings' resolver, same reason), and ObjectQL beyond `IDataEngine`
54+
(`registry` / `getSchema` / `registerHook` / `registerMiddleware`). That last one
55+
is deliberate scope: the standing record on `getObjectQL` in `@objectstack/runtime`
56+
says ObjectQL is genuinely wider than `IDataEngine` and nobody has written the
57+
wider contract, so typing the whole thing `IDataEngine` would be "the more
58+
comfortable-looking lie". These declarations are what that contract gets written
59+
from, and what it deletes.
60+
61+
No behavior changes beyond the two fixes above.

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

Lines changed: 120 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,14 @@
22

33
import { Plugin, PluginContext, IHttpServer } from '@objectstack/core';
44
import type { BetterAuthOptions } from 'better-auth';
5-
import { AuthConfig, type SocialProviderConfig, SystemObjectName, SystemUserId } from '@objectstack/spec/system';
5+
import {
6+
AuthConfig,
7+
type SocialProviderConfig,
8+
type SettingsChangeHandler,
9+
type SettingsUnsubscribe,
10+
SystemObjectName,
11+
SystemUserId,
12+
} from '@objectstack/spec/system';
613
import {
714
// ADR-0048 — the Setup/Studio/Account apps moved to their own packages
815
// (@objectstack/{setup,studio,account}); plugin-auth no longer registers them.
@@ -12,6 +19,7 @@ import {
1219
import { SysOrganizationDetailPage, SysUserDetailPage } from '@objectstack/platform-objects/pages';
1320
import { resolveTenancyPosture } from '@objectstack/types';
1421
import { postureEnforcesWall, type OrgScopingEntitlement } from '@objectstack/spec/security';
22+
import type { IDataEngine, IEmailService, ISmsService } from '@objectstack/spec/contracts';
1523
import {
1624
AuthManager,
1725
resolveOidcProviderEnabled,
@@ -36,6 +44,87 @@ import {
3644
authPluginManifestHeader,
3745
} from './manifest.js';
3846

47+
/**
48+
* The `objectql` slot BEYOND `IDataEngine` — the hook and middleware seams this
49+
* plugin installs on the engine.
50+
*
51+
* [#4251] The slot's ledger entry is `IDataEngine` and it covers every read and
52+
* write below; it does not cover `registerHook` / `registerMiddleware`, and no
53+
* contract has been written for the wider ObjectQL surface yet (the standing
54+
* record of why is on `getObjectQL` in `@objectstack/runtime`'s
55+
* `DomainHandlerContext`). Declared here, narrow and named, so the extension is
56+
* legible instead of hidden under `any` — and so it is deleted, not migrated,
57+
* when that contract lands.
58+
*
59+
* Both members are optional and every call site already guards with
60+
* `typeof … === 'function'`: the slot is satisfiable by engines that implement
61+
* neither (mock mode), and this plugin degrades rather than fails there.
62+
*/
63+
interface EngineExtensionSurface {
64+
registerHook?(
65+
event: string,
66+
handler: (context: any) => Promise<void> | void,
67+
options?: { object?: string | string[]; priority?: number; packageId?: string },
68+
): void;
69+
registerMiddleware?(
70+
fn: (opCtx: any, next: () => Promise<void>) => Promise<void>,
71+
options?: { object?: string },
72+
): void;
73+
}
74+
75+
/** The engine as this plugin uses it: the data contract plus those two seams. */
76+
type AuthEngine = IDataEngine & EngineExtensionSurface;
77+
78+
/**
79+
* The `settings` slot, as this plugin reads it.
80+
*
81+
* [#4251] `service-settings` registers its `SettingsService` here and the slot
82+
* carries no `packages/spec` contract, so this declares the two resolver methods
83+
* this plugin calls. Structural on purpose — plugin-auth must not depend on
84+
* service-settings, which is optional (both readers below already return early
85+
* when the slot is empty or the method is missing).
86+
*/
87+
interface SettingsReadSurface {
88+
/** Resolve one key; `source` distinguishes a stored/env value from a manifest default. */
89+
get(
90+
namespace: string,
91+
key: string,
92+
ctx?: Record<string, unknown>,
93+
): Promise<{ value?: unknown; source?: string } | undefined>;
94+
/** Resolve a whole namespace as `key → { value, source }`. */
95+
getNamespace(
96+
namespace: string,
97+
ctx?: Record<string, unknown>,
98+
): Promise<{ values: Record<string, { value?: unknown; source?: string } | undefined> }>;
99+
/**
100+
* Re-run a binding when the namespace changes — how the brand name, SMS
101+
* locale and auth namespace stay live without a redeploy. Optional: an
102+
* implementation without a change bus leaves the initial read in place, which
103+
* is what the `typeof … === 'function'` guards at each call site already say.
104+
*
105+
* The handler and unsubscribe types are the SPEC's
106+
* (`@objectstack/spec/system`), not re-declared here — the change bus has a
107+
* published contract even though the slot does not.
108+
*/
109+
subscribe?(
110+
namespace: string | undefined,
111+
handler: SettingsChangeHandler,
112+
): SettingsUnsubscribe;
113+
}
114+
115+
/**
116+
* The `protocol` slot's metadata reader, as the user-import route uses it.
117+
*
118+
* [#4251] `protocol` is a deliberately UNCONTRACTED slot (see
119+
* `UNCONTRACTED_SLOTS` in `eslint.config.mjs`), so the one method this route
120+
* needs is declared here rather than erased. It reads `sys_user`'s field
121+
* definitions so imported values are coerced to their declared types —
122+
* best-effort by contract: without it the import still runs, uncoerced.
123+
*/
124+
interface MetaItemReaderSurface {
125+
getMetaItem(ref: { type: string; name: string }): Promise<any>;
126+
}
127+
39128
/**
40129
* Auth Plugin Options
41130
* Extends AuthConfig from spec with additional runtime options
@@ -222,7 +311,7 @@ export class AuthPlugin implements Plugin {
222311
// that advertises a tolerance the composition forbids is worse than no
223312
// branch — it reads as a supported degraded mode (#4187). AuthManager keeps
224313
// its own `dataEngine?` guards because it is usable outside this plugin.
225-
const dataEngine = ctx.getService<any>('data');
314+
const dataEngine = ctx.getService<IDataEngine>('data');
226315

227316
const authConfig: AuthManagerOptions & AuthPluginOptions = {
228317
...this.options,
@@ -451,8 +540,8 @@ export class AuthPlugin implements Plugin {
451540
if (this.authManager) {
452541
await this.bindAuthSettings(ctx);
453542

454-
let emailSvc: any;
455-
try { emailSvc = ctx.getService<any>('email'); } catch { emailSvc = undefined; }
543+
let emailSvc: IEmailService | undefined;
544+
try { emailSvc = ctx.getService<IEmailService>('email'); } catch { emailSvc = undefined; }
456545
if (emailSvc) {
457546
this.authManager.setEmailService(emailSvc);
458547
ctx.logger.info('Auth: email service wired (transactional mail enabled)');
@@ -480,8 +569,8 @@ export class AuthPlugin implements Plugin {
480569
// SMS-invite path can deliver. Same lazy-resolution contract as
481570
// the email service: absent ⇒ OTP endpoints keep failing loudly
482571
// (NOT_SUPPORTED) while phone+password sign-in still works.
483-
let smsSvc: any;
484-
try { smsSvc = ctx.getService<any>('sms'); } catch { smsSvc = undefined; }
572+
let smsSvc: ISmsService | undefined;
573+
try { smsSvc = ctx.getService<ISmsService>('sms'); } catch { smsSvc = undefined; }
485574
if (smsSvc) {
486575
this.authManager.setSmsService(smsSvc);
487576
if (this.authManager.isPhoneNumberEnabled()) {
@@ -503,7 +592,7 @@ export class AuthPlugin implements Plugin {
503592
// the override so the deployment's `appName` (e.g. `OS_APP_NAME`)
504593
// keeps precedence. Mirrors EmailServicePlugin's settings binding.
505594
try {
506-
const settings = ctx.getService<any>('settings');
595+
const settings = ctx.getService<SettingsReadSurface>('settings');
507596
if (settings && typeof settings.get === 'function') {
508597
const applyBrand = async () => {
509598
try {
@@ -628,7 +717,7 @@ export class AuthPlugin implements Plugin {
628717
// whose rows already carry an issuer costs one empty query.
629718
ctx.hook('kernel:ready', async () => {
630719
try {
631-
const ql: any = ctx.getService<any>('objectql');
720+
const ql = ctx.getService<IDataEngine>('objectql');
632721
if (!ql) return;
633722
const { backfillAccountIssuer } = await import('./backfill-account-issuer.js');
634723
await backfillAccountIssuer(ql, {
@@ -656,7 +745,7 @@ export class AuthPlugin implements Plugin {
656745
if (this.options.autoDefaultOrganization !== false && !postureEnforcesWall(resolveTenancyPosture())) {
657746
const runEnsure = async () => {
658747
try {
659-
const ql: any = ctx.getService<any>('objectql');
748+
const ql = ctx.getService<IDataEngine>('objectql');
660749
if (!ql) return;
661750
const res = await ensureDefaultOrganization(ql, { logger: ctx.logger });
662751
if (res.defaultOrgCreated) {
@@ -675,7 +764,7 @@ export class AuthPlugin implements Plugin {
675764
// to platform admin" case where kernel:ready fired before any user
676765
// existed (same wiring the multi-org bootstrap uses).
677766
try {
678-
const ql: any = ctx.getService<any>('objectql');
767+
const ql = ctx.getService<AuthEngine>('objectql');
679768
if (ql && typeof ql.registerMiddleware === 'function') {
680769
ql.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
681770
await next();
@@ -708,7 +797,7 @@ export class AuthPlugin implements Plugin {
708797
const runBackfill = (source: string): Promise<void> => {
709798
backfillChain = backfillChain.then(async () => {
710799
try {
711-
const ql: any = ctx.getService<any>('objectql');
800+
const ql = ctx.getService<IDataEngine>('objectql');
712801
const tenancy = this.tenancy;
713802
if (!ql || !tenancy) return;
714803
const res = await backfillMemberships(ql, {
@@ -752,7 +841,7 @@ export class AuthPlugin implements Plugin {
752841
try {
753842
// Use the kernel's ObjectQL engine (available + hookable at kernel:ready);
754843
// the auth manager's getDataEngine() is not yet wired this early.
755-
const engine: any = ctx.getService<any>('objectql');
844+
const engine = ctx.getService<AuthEngine>('objectql');
756845
if (!engine || typeof engine.registerHook !== 'function') return;
757846
const SYSTEM_CTX = { isSystem: true, roles: [], permissions: [] };
758847
engine.registerHook('afterInsert', async (hookCtx: any) => {
@@ -794,7 +883,7 @@ export class AuthPlugin implements Plugin {
794883
// bypass — see identity-write-guard.ts for the full contract.
795884
ctx.hook('kernel:ready', async () => {
796885
try {
797-
const engine: any = ctx.getService<any>('objectql');
886+
const engine = ctx.getService<AuthEngine>('objectql');
798887
if (!engine || typeof engine.registerHook !== 'function') return;
799888
registerManagedUpdateWhitelist(SystemObjectName.USER, SYS_USER_PROFILE_EDIT_FIELDS);
800889
// [ADR-0105 D7] Extension fields ObjectStack adds to better-auth-managed
@@ -821,7 +910,7 @@ export class AuthPlugin implements Plugin {
821910

822911
// Register auth middleware on ObjectQL engine (if available)
823912
try {
824-
const ql = ctx.getService<any>('objectql');
913+
const ql = ctx.getService<AuthEngine>('objectql');
825914
if (ql && typeof ql.registerMiddleware === 'function') {
826915
ql.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
827916
// If context already has userId or isSystem, skip auth resolution
@@ -850,9 +939,9 @@ export class AuthPlugin implements Plugin {
850939
private async bindAuthSettings(ctx: PluginContext): Promise<void> {
851940
if (!this.authManager) return;
852941

853-
let settings: any;
942+
let settings: SettingsReadSurface | undefined;
854943
try {
855-
settings = ctx.getService<any>('settings');
944+
settings = ctx.getService<SettingsReadSurface>('settings');
856945
} catch {
857946
return;
858947
}
@@ -1136,8 +1225,8 @@ export class AuthPlugin implements Plugin {
11361225
const password = process.env.OS_SEED_ADMIN_PASSWORD?.trim() || 'admin123';
11371226
const name = process.env.OS_SEED_ADMIN_NAME?.trim() || 'Dev Admin';
11381227

1139-
let ql: any;
1140-
try { ql = ctx.getService<any>('objectql'); } catch { /* unavailable */ }
1228+
let ql: IDataEngine | undefined;
1229+
try { ql = ctx.getService<IDataEngine>('objectql'); } catch { /* unavailable */ }
11411230
if (!ql || typeof ql.find !== 'function') return;
11421231

11431232
try {
@@ -1627,15 +1716,24 @@ export class AuthPlugin implements Plugin {
16271716
const actor = await gateAdmin(c);
16281717
if (actor instanceof Response) return actor;
16291718
const { runAdminImportUsers } = await import('./admin-import-users.js');
1630-
const metadataService: any = (() => {
1631-
try { return ctx.getService?.('metadata'); } catch { return undefined; }
1719+
// [#4251] Resolved from `protocol`, not `metadata`. `getMetaItem` is a
1720+
// PROTOCOL method (`ObjectStackProtocolImplementation`, registered by
1721+
// MetadataProtocolPlugin); the `metadata` slot holds MetadataManager,
1722+
// which has never had it. So `metadataService?.getMetaItem` was always
1723+
// falsy and the field-coercion dep was never passed — an import row's
1724+
// values reached `sys_user` uncoerced, with the branch that says
1725+
// otherwise sitting right here. Invisible while the lookup was `any`:
1726+
// typing it to the slot's contract is what turned the dead probe into
1727+
// a compile error.
1728+
const metaReader = (() => {
1729+
try { return ctx.getService?.<MetaItemReaderSurface>('protocol'); } catch { return undefined; }
16321730
})();
16331731
const { status, body } = await runAdminImportUsers(
16341732
{
16351733
getAuthApi: () => this.authManager!.getApi() as any,
16361734
getDataEngine: () => this.authManager!.getDataEngine(),
1637-
...(metadataService?.getMetaItem
1638-
? { getMetaItem: (ref: { type: string; name: string }) => metadataService.getMetaItem(ref) }
1735+
...(typeof metaReader?.getMetaItem === 'function'
1736+
? { getMetaItem: (ref: { type: string; name: string }) => metaReader.getMetaItem(ref) }
16391737
: {}),
16401738
phoneNumberEnabled: () => this.authManager!.isPhoneNumberEnabled(),
16411739
emailServiceAvailable: () => this.authManager!.isEmailServiceAvailable(),

packages/plugins/plugin-hono-server/src/current-user-endpoints-multi-tenant.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,9 @@ describe('the answer comes from the kernel that OWNS the request (cloud#927)', (
9696
it('routes /auth/me/localization and /me/apps through the same resolution', async () => {
9797
const envKernel = kernelWith({
9898
auth: authFor('usr_env'),
99-
objectql: { _registry: { getAllApps: () => [{ name: 'env_app' }] } },
99+
// [#4251] `registry`, the public accessor — this stub pinned the
100+
// private `_registry` the handler used to reach through `as any`.
101+
objectql: { registry: { getAllApps: () => [{ name: 'env_app' }] } },
100102
});
101103
const { app } = mount({}, async () => envKernel);
102104

0 commit comments

Comments
 (0)