Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions .changeset/slot-lookup-sweep-auth-session.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
---
"@objectstack/spec": patch
"@objectstack/plugin-auth": patch
"@objectstack/plugin-hono-server": patch
"@objectstack/plugin-security": patch
---

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)

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**.

**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 by MetadataProtocolPlugin under the `protocol` slot. `MetadataManager`,
which occupies `metadata`, has never had it. So the probe was false on every
deployment and the dep was never passed: imported rows reached `sys_user`
uncoerced, with the branch that says otherwise sitting right there. This is the
same shape as #4127's dead `automation.trigger` and #4321's `registerInMemory`
probes — a capability the code advertises and the runtime cannot deliver, kept
invisible by the `any`. Typing the lookup to `IMetadataService` is what turned it
into a compile error. 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 the private reach
and stubs `registry` instead.

**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:
reads once took their context inside the query while writes took it in trailing
`options.context`, so the same `{ context }` object was correct as `insert`'s 3rd
argument and **silently dropped** as `find`'s — "an intended `isSystem` bypass
just vanished". The engine accepts both channels; the contract exposed only the
query one, so callers using the trailing channel — the current-user endpoints'
permission-set loader among them — could only reach it by erasing the lookup.
Adding an optional trailing parameter breaks no implementor (the existing
minimal-implementation test proves it) and no caller. `BaseEngineOptions` was
already exported, sitting unused under the "legacy/deprecated" heading, which is
why the contract went looking and did not find it; it moves up beside the other
QueryAST-aligned types with the rationale attached. One new spec test pins the
trailing argument at the call site — the position where the old contract rejected it.

**Where the contract does not reach, the escape hatch is named.** Three slots
resist a spec type today and each gets a narrow, documented local interface
instead of `any`: `security.permissions` (plugin-security's `PermissionEvaluator`
— plugin-hono-server must not depend on an optional plugin), `settings`
(service-settings' resolver, same reason), and ObjectQL beyond `IDataEngine`
(`registry` / `getSchema` / `registerHook` / `registerMiddleware`). That last one
is deliberate scope: the standing record on `getObjectQL` in `@objectstack/runtime`
says ObjectQL is genuinely wider than `IDataEngine` and nobody has written the
wider contract, so 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.

No behavior changes beyond the two fixes above.
142 changes: 120 additions & 22 deletions packages/plugins/plugin-auth/src/auth-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,14 @@

import { Plugin, PluginContext, IHttpServer } from '@objectstack/core';
import type { BetterAuthOptions } from 'better-auth';
import { AuthConfig, type SocialProviderConfig, SystemObjectName, SystemUserId } from '@objectstack/spec/system';
import {
AuthConfig,
type SocialProviderConfig,
type SettingsChangeHandler,
type SettingsUnsubscribe,
SystemObjectName,
SystemUserId,
} from '@objectstack/spec/system';
import {
// ADR-0048 — the Setup/Studio/Account apps moved to their own packages
// (@objectstack/{setup,studio,account}); plugin-auth no longer registers them.
Expand All @@ -12,6 +19,7 @@ import {
import { SysOrganizationDetailPage, SysUserDetailPage } from '@objectstack/platform-objects/pages';
import { resolveTenancyPosture } from '@objectstack/types';
import { postureEnforcesWall, type OrgScopingEntitlement } from '@objectstack/spec/security';
import type { IDataEngine, IEmailService, ISmsService } from '@objectstack/spec/contracts';
import {
AuthManager,
resolveOidcProviderEnabled,
Expand All @@ -36,6 +44,87 @@ import {
authPluginManifestHeader,
} from './manifest.js';

/**
* The `objectql` slot BEYOND `IDataEngine` — the hook and middleware seams this
* plugin installs on the engine.
*
* [#4251] The slot's ledger entry is `IDataEngine` and it covers every read and
* write below; it does not cover `registerHook` / `registerMiddleware`, and no
* contract has been written for the wider ObjectQL surface yet (the standing
* record of why is on `getObjectQL` in `@objectstack/runtime`'s
* `DomainHandlerContext`). Declared here, narrow and named, so the extension is
* legible instead of hidden under `any` — and so it is deleted, not migrated,
* when that contract lands.
*
* Both members are optional and every call site already guards with
* `typeof … === 'function'`: the slot is satisfiable by engines that implement
* neither (mock mode), and this plugin degrades rather than fails there.
*/
interface EngineExtensionSurface {
registerHook?(
event: string,
handler: (context: any) => Promise<void> | void,
options?: { object?: string | string[]; priority?: number; packageId?: string },
): void;
registerMiddleware?(
fn: (opCtx: any, next: () => Promise<void>) => Promise<void>,
options?: { object?: string },
): void;
}

/** The engine as this plugin uses it: the data contract plus those two seams. */
type AuthEngine = IDataEngine & EngineExtensionSurface;

/**
* The `settings` slot, as this plugin reads it.
*
* [#4251] `service-settings` registers its `SettingsService` here and the slot
* carries no `packages/spec` contract, so this declares the two resolver methods
* this plugin calls. Structural on purpose — plugin-auth must not depend on
* service-settings, which is optional (both readers below already return early
* when the slot is empty or the method is missing).
*/
interface SettingsReadSurface {
/** Resolve one key; `source` distinguishes a stored/env value from a manifest default. */
get(
namespace: string,
key: string,
ctx?: Record<string, unknown>,
): Promise<{ value?: unknown; source?: string } | undefined>;
/** Resolve a whole namespace as `key → { value, source }`. */
getNamespace(
namespace: string,
ctx?: Record<string, unknown>,
): Promise<{ values: Record<string, { value?: unknown; source?: string } | undefined> }>;
/**
* Re-run a binding when the namespace changes — how the brand name, SMS
* locale and auth namespace stay live without a redeploy. Optional: an
* implementation without a change bus leaves the initial read in place, which
* is what the `typeof … === 'function'` guards at each call site already say.
*
* The handler and unsubscribe types are the SPEC's
* (`@objectstack/spec/system`), not re-declared here — the change bus has a
* published contract even though the slot does not.
*/
subscribe?(
namespace: string | undefined,
handler: SettingsChangeHandler,
): SettingsUnsubscribe;
}

/**
* The `protocol` slot's metadata reader, as the user-import route uses it.
*
* [#4251] `protocol` is a deliberately UNCONTRACTED slot (see
* `UNCONTRACTED_SLOTS` in `eslint.config.mjs`), so the one method this route
* needs is declared here rather than erased. It reads `sys_user`'s field
* definitions so imported values are coerced to their declared types —
* best-effort by contract: without it the import still runs, uncoerced.
*/
interface MetaItemReaderSurface {
getMetaItem(ref: { type: string; name: string }): Promise<any>;
}

/**
* Auth Plugin Options
* Extends AuthConfig from spec with additional runtime options
Expand Down Expand Up @@ -222,7 +311,7 @@ export class AuthPlugin implements Plugin {
// that advertises a tolerance the composition forbids is worse than no
// branch — it reads as a supported degraded mode (#4187). AuthManager keeps
// its own `dataEngine?` guards because it is usable outside this plugin.
const dataEngine = ctx.getService<any>('data');
const dataEngine = ctx.getService<IDataEngine>('data');

const authConfig: AuthManagerOptions & AuthPluginOptions = {
...this.options,
Expand Down Expand Up @@ -451,8 +540,8 @@ export class AuthPlugin implements Plugin {
if (this.authManager) {
await this.bindAuthSettings(ctx);

let emailSvc: any;
try { emailSvc = ctx.getService<any>('email'); } catch { emailSvc = undefined; }
let emailSvc: IEmailService | undefined;
try { emailSvc = ctx.getService<IEmailService>('email'); } catch { emailSvc = undefined; }
if (emailSvc) {
this.authManager.setEmailService(emailSvc);
ctx.logger.info('Auth: email service wired (transactional mail enabled)');
Expand Down Expand Up @@ -480,8 +569,8 @@ export class AuthPlugin implements Plugin {
// SMS-invite path can deliver. Same lazy-resolution contract as
// the email service: absent ⇒ OTP endpoints keep failing loudly
// (NOT_SUPPORTED) while phone+password sign-in still works.
let smsSvc: any;
try { smsSvc = ctx.getService<any>('sms'); } catch { smsSvc = undefined; }
let smsSvc: ISmsService | undefined;
try { smsSvc = ctx.getService<ISmsService>('sms'); } catch { smsSvc = undefined; }
if (smsSvc) {
this.authManager.setSmsService(smsSvc);
if (this.authManager.isPhoneNumberEnabled()) {
Expand All @@ -503,7 +592,7 @@ export class AuthPlugin implements Plugin {
// the override so the deployment's `appName` (e.g. `OS_APP_NAME`)
// keeps precedence. Mirrors EmailServicePlugin's settings binding.
try {
const settings = ctx.getService<any>('settings');
const settings = ctx.getService<SettingsReadSurface>('settings');
if (settings && typeof settings.get === 'function') {
const applyBrand = async () => {
try {
Expand Down Expand Up @@ -628,7 +717,7 @@ export class AuthPlugin implements Plugin {
// whose rows already carry an issuer costs one empty query.
ctx.hook('kernel:ready', async () => {
try {
const ql: any = ctx.getService<any>('objectql');
const ql = ctx.getService<IDataEngine>('objectql');
if (!ql) return;
const { backfillAccountIssuer } = await import('./backfill-account-issuer.js');
await backfillAccountIssuer(ql, {
Expand Down Expand Up @@ -656,7 +745,7 @@ export class AuthPlugin implements Plugin {
if (this.options.autoDefaultOrganization !== false && !postureEnforcesWall(resolveTenancyPosture())) {
const runEnsure = async () => {
try {
const ql: any = ctx.getService<any>('objectql');
const ql = ctx.getService<IDataEngine>('objectql');
if (!ql) return;
const res = await ensureDefaultOrganization(ql, { logger: ctx.logger });
if (res.defaultOrgCreated) {
Expand All @@ -675,7 +764,7 @@ export class AuthPlugin implements Plugin {
// to platform admin" case where kernel:ready fired before any user
// existed (same wiring the multi-org bootstrap uses).
try {
const ql: any = ctx.getService<any>('objectql');
const ql = ctx.getService<AuthEngine>('objectql');
if (ql && typeof ql.registerMiddleware === 'function') {
ql.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
await next();
Expand Down Expand Up @@ -708,7 +797,7 @@ export class AuthPlugin implements Plugin {
const runBackfill = (source: string): Promise<void> => {
backfillChain = backfillChain.then(async () => {
try {
const ql: any = ctx.getService<any>('objectql');
const ql = ctx.getService<IDataEngine>('objectql');
const tenancy = this.tenancy;
if (!ql || !tenancy) return;
const res = await backfillMemberships(ql, {
Expand Down Expand Up @@ -752,7 +841,7 @@ export class AuthPlugin implements Plugin {
try {
// Use the kernel's ObjectQL engine (available + hookable at kernel:ready);
// the auth manager's getDataEngine() is not yet wired this early.
const engine: any = ctx.getService<any>('objectql');
const engine = ctx.getService<AuthEngine>('objectql');
if (!engine || typeof engine.registerHook !== 'function') return;
const SYSTEM_CTX = { isSystem: true, roles: [], permissions: [] };
engine.registerHook('afterInsert', async (hookCtx: any) => {
Expand Down Expand Up @@ -794,7 +883,7 @@ export class AuthPlugin implements Plugin {
// bypass — see identity-write-guard.ts for the full contract.
ctx.hook('kernel:ready', async () => {
try {
const engine: any = ctx.getService<any>('objectql');
const engine = ctx.getService<AuthEngine>('objectql');
if (!engine || typeof engine.registerHook !== 'function') return;
registerManagedUpdateWhitelist(SystemObjectName.USER, SYS_USER_PROFILE_EDIT_FIELDS);
// [ADR-0105 D7] Extension fields ObjectStack adds to better-auth-managed
Expand All @@ -821,7 +910,7 @@ export class AuthPlugin implements Plugin {

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

let settings: any;
let settings: SettingsReadSurface | undefined;
try {
settings = ctx.getService<any>('settings');
settings = ctx.getService<SettingsReadSurface>('settings');
} catch {
return;
}
Expand Down Expand Up @@ -1136,8 +1225,8 @@ export class AuthPlugin implements Plugin {
const password = process.env.OS_SEED_ADMIN_PASSWORD?.trim() || 'admin123';
const name = process.env.OS_SEED_ADMIN_NAME?.trim() || 'Dev Admin';

let ql: any;
try { ql = ctx.getService<any>('objectql'); } catch { /* unavailable */ }
let ql: IDataEngine | undefined;
try { ql = ctx.getService<IDataEngine>('objectql'); } catch { /* unavailable */ }
if (!ql || typeof ql.find !== 'function') return;

try {
Expand Down Expand Up @@ -1627,15 +1716,24 @@ export class AuthPlugin implements Plugin {
const actor = await gateAdmin(c);
if (actor instanceof Response) return actor;
const { runAdminImportUsers } = await import('./admin-import-users.js');
const metadataService: any = (() => {
try { return ctx.getService?.('metadata'); } catch { return undefined; }
// [#4251] Resolved from `protocol`, not `metadata`. `getMetaItem` is a
// PROTOCOL method (`ObjectStackProtocolImplementation`, registered by
// MetadataProtocolPlugin); the `metadata` slot holds MetadataManager,
// which has never had it. So `metadataService?.getMetaItem` was always
// falsy and the field-coercion dep was never passed — an import row's
// values reached `sys_user` uncoerced, with the branch that says
// otherwise sitting right here. Invisible while the lookup was `any`:
// typing it to the slot's contract is what turned the dead probe into
// a compile error.
const metaReader = (() => {
try { return ctx.getService?.<MetaItemReaderSurface>('protocol'); } catch { return undefined; }
})();
const { status, body } = await runAdminImportUsers(
{
getAuthApi: () => this.authManager!.getApi() as any,
getDataEngine: () => this.authManager!.getDataEngine(),
...(metadataService?.getMetaItem
? { getMetaItem: (ref: { type: string; name: string }) => metadataService.getMetaItem(ref) }
...(typeof metaReader?.getMetaItem === 'function'
? { getMetaItem: (ref: { type: string; name: string }) => metaReader.getMetaItem(ref) }
: {}),
phoneNumberEnabled: () => this.authManager!.isPhoneNumberEnabled(),
emailServiceAvailable: () => this.authManager!.isEmailServiceAvailable(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ describe('the answer comes from the kernel that OWNS the request (cloud#927)', (
it('routes /auth/me/localization and /me/apps through the same resolution', async () => {
const envKernel = kernelWith({
auth: authFor('usr_env'),
objectql: { _registry: { getAllApps: () => [{ name: 'env_app' }] } },
// [#4251] `registry`, the public accessor — this stub pinned the
// private `_registry` the handler used to reach through `as any`.
objectql: { registry: { getAllApps: () => [{ name: 'env_app' }] } },
});
const { app } = mount({}, async () => envKernel);

Expand Down
Loading
Loading