Skip to content

Commit cd288b4

Browse files
claude[bot]claude
andauthored
fix(plugin-auth): register the auth service-composition bindings independently of registerRoutes (#14810)
* fix(plugin-auth): register the auth service-composition bindings independently of registerRoutes `AuthPlugin` bound the outbound mail transport, the SMS transport, the deployment email locale, the brand name and the SMS locale inside the same `kernel:ready` hook that mounts `/api/v1/auth/*`, and that hook was gated on `registerRoutes`. `registerRoutes` answers a transport-mounting question. The bindings are service composition and are true of an embedding regardless of who serves the routes, so every routes-less embedding came up with no mail transport, no locale on either channel and no brand binding — silently, because the `logger.info` lines that would have reported the wiring sat inside the same skipped block. Split the hook: the composition block moves verbatim into its own unconditional `ctx.hook('kernel:ready', …)`, registered before the route hook so the ordering a routing host had is preserved. Route registration itself stays under `if (this.options.registerRoutes)`. This is the shape the sibling hooks in this file already use and already name ("Registered independently of `registerRoutes` so an embedding that serves no auth routes still gets the diagnosis") — the file applied the distinction to the diagnosis hook and not to the wiring the diagnosis exists to report on. The `#14319` describe block now runs against both values of `registerRoutes` rather than only the default, and pins both acceptance criteria: the composition completes either way, and a `registerRoutes: false` kernel still mounts no auth routes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * chore(auth): add the changeset and re-anchor the system-context census row The census row for `plugin-auth`'s session-resolution middleware anchors a LINE NUMBER in `auth-plugin.ts`; the hook split moved that read site from :1353 to :1380 without changing a character of it. Re-anchored with the gate's own `--fix`, which is the repair it prescribes for pure line rot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f3ae441 commit cd288b4

4 files changed

Lines changed: 415 additions & 254 deletions

File tree

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
"@objectstack/plugin-auth": patch
3+
---
4+
5+
fix(plugin-auth): the mail transport, brand and locale bindings no longer depend on `registerRoutes`
6+
7+
`AuthPlugin` bound five things to the live kernel on `kernel:ready` — the
8+
outbound mail transport (`setEmailService`), the SMS transport
9+
(`setSmsService`), the deployment email locale (`setDefaultEmailLocale`), the
10+
brand name (`setAppName`) and the SMS locale (`setDefaultSmsLocale`) — from
11+
inside the same hook that mounts `/api/v1/auth/*`, and that hook was gated on
12+
`registerRoutes`.
13+
14+
`registerRoutes` answers a transport-mounting question: should this plugin put
15+
its own routes on the kernel's `http-server`. The bindings are service
16+
composition, and they are true of an embedding regardless of who serves the
17+
routes. So an embedding that serves auth routes itself — the whole point of
18+
`registerRoutes: false` — came up with no mail transport, no locale on either
19+
channel and no brand binding. Silently: the `logger.info` lines that report the
20+
wiring were inside the same skipped block, and the `localization` settings
21+
namespace was not even read. One visible consequence was that the workspace
22+
language could not reach auth mail on such a host at all, and
23+
`/api/v1/auth/config` answered `requireEmailVerification: false` because
24+
`resolveRequireEmailVerification()` saw no transport.
25+
26+
The composition block now registers as its own unconditional
27+
`ctx.hook('kernel:ready', …)` — the shape the sibling diagnosis and dev-seed
28+
hooks in this plugin already use — placed before the route hook so a routing
29+
host keeps the ordering the single combined hook gave it.
30+
31+
Route registration itself stays gated: a `registerRoutes: false` kernel still
32+
mounts no auth routes.
33+
34+
**Behaviour change for `registerRoutes: false` embeddings.** They now resolve
35+
the `email`, `sms`, `i18n` and `settings` services at `kernel:ready`, apply
36+
`branding.workspace_name` and `localization.locale` (subscribing to both), seed
37+
the built-in auth SMS templates when phone sign-in is enabled, and emit the
38+
four wiring `info` lines. Hosts that had compensated by wiring these by hand
39+
should expect the plugin's own binding to run as well; both paths are
40+
idempotent setters, and an explicit workspace setting keeps outranking a
41+
manifest default exactly as it does on a routing host. Nothing changes for a
42+
host that leaves `registerRoutes` at its default.

content/docs/permissions/system-context.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ that silently does not happen.
9797
| 8 | `explain()` may target a principal other than the caller | plugin-security | Get: no `manage_users` / delegated-admin check | `security-plugin.ts:3857` |
9898
| 9 | Anonymous-deny treats the caller as authenticated | core | Get: passes the 401 seam with no `userId` | `anonymous-deny.ts:154` |
9999
| 10 | Permission-set projection middleware skipped | plugin-security | Lose: projection of permission-set-derived columns | `permission-set-projection.ts:1015` |
100-
| 11 | Session-resolution middleware skipped | plugin-auth | Get: no session lookup attempted | `auth-plugin.ts:1353` |
100+
| 11 | Session-resolution middleware skipped | plugin-auth | Get: no session lookup attempted | `auth-plugin.ts:1380` |
101101
| 12 | Per-request performance timings disclosed | observability | Get: timing headers a normal caller cannot pull | `perf-timing.ts:474` |
102102
| 13 | Permission-set **overlay discard** skips the tenant-admin assertion | plugin-security | Get: an overlay can be discarded with no authenticated tenant administrator | `permission-set-overlay-discard.ts:142` |
103103
| 14 | MCP stdio bridge skips the object API-exposure gate | mcp | Get: the bridge reaches objects whose `apiEnabled` / `apiMethods` would refuse an external caller | `stdio-data-bridge.ts:246` |

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

Lines changed: 148 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -541,6 +541,9 @@ describe('AuthPlugin', () => {
541541
let hookCapture: ReturnType<typeof createHookCapture>;
542542
let setEmailLocaleSpy: ReturnType<typeof vi.spyOn>;
543543
let setSmsLocaleSpy: ReturnType<typeof vi.spyOn>;
544+
let setEmailServiceSpy: ReturnType<typeof vi.spyOn>;
545+
let rawApp: { all: ReturnType<typeof vi.fn>; get: ReturnType<typeof vi.fn>; post: ReturnType<typeof vi.fn>; use: ReturnType<typeof vi.fn> };
546+
let httpServer: { getRawApp: ReturnType<typeof vi.fn>; use: ReturnType<typeof vi.fn> };
544547

545548
type Resolved = { value: unknown; source: string };
546549

@@ -576,17 +579,38 @@ describe('AuthPlugin', () => {
576579
* `i18n` is passed as `null` to mean "no such service" — `getService`
577580
* THROWS for an unregistered service, which is the shape the plugin
578581
* probes for, not a falsy return.
582+
*
583+
* `registerRoutes` is threaded through on purpose (#14724). It answers a
584+
* TRANSPORT-MOUNTING question — "does this plugin mount its own
585+
* `/api/v1/auth/*` routes" — and must not decide any of the SERVICE
586+
* COMPOSITION this block is about. A routes-less embedding (every cloud
587+
* tenant environment kernel is one) has the same mail transport, brand
588+
* and locale needs as a routing one; before #14724 all of it hung off the
589+
* routing flag, so a `registerRoutes: false` kernel did not even READ the
590+
* `localization` namespace.
579591
*/
580-
const boot = async (opts: { settings?: unknown; i18nDefault?: string | null }) => {
592+
const boot = async (opts: {
593+
settings?: unknown;
594+
i18nDefault?: string | null;
595+
registerRoutes?: boolean;
596+
email?: unknown;
597+
}) => {
581598
hookCapture = createHookCapture();
582599
mockContext.hook = hookCapture.hookFn;
600+
rawApp = { all: vi.fn(), get: vi.fn(), post: vi.fn(), use: vi.fn() };
601+
httpServer = { getRawApp: vi.fn(() => rawApp), use: vi.fn() };
583602
// `: any` on the RETURN, not a cast on the assignment: `getService` is
584603
// generic (`<T>(name: string) => T`), so an inferred union return is a
585604
// TS2322 — the exact debt `check:test-typecheck` ledgers for the older
586605
// doubles in this file. That ledger only ratchets down, so this one
587606
// states its shape instead of adding to it.
588607
mockContext.getService = vi.fn((name: string): any => {
589608
if (name === 'manifest') return { register: vi.fn() };
609+
if (name === 'http-server') return httpServer;
610+
if (name === 'email') {
611+
if (opts.email === undefined) throw new Error('Service not found: email');
612+
return opts.email;
613+
}
590614
if (name === 'settings') {
591615
if (opts.settings === undefined) throw new Error('Service not found: settings');
592616
return opts.settings;
@@ -599,9 +623,11 @@ describe('AuthPlugin', () => {
599623
});
600624
setEmailLocaleSpy = vi.spyOn(AuthManager.prototype, 'setDefaultEmailLocale');
601625
setSmsLocaleSpy = vi.spyOn(AuthManager.prototype, 'setDefaultSmsLocale');
626+
setEmailServiceSpy = vi.spyOn(AuthManager.prototype, 'setEmailService');
602627
authPlugin = new AuthPlugin({
603628
secret: 'test-secret-at-least-32-chars-long',
604629
baseUrl: 'http://localhost:3000',
630+
...(opts.registerRoutes === undefined ? {} : { registerRoutes: opts.registerRoutes }),
605631
});
606632
await authPlugin.init(mockContext);
607633
await authPlugin.start(mockContext);
@@ -611,72 +637,138 @@ describe('AuthPlugin', () => {
611637
afterEach(() => {
612638
setEmailLocaleSpy?.mockRestore();
613639
setSmsLocaleSpy?.mockRestore();
614-
});
640+
setEmailServiceSpy?.mockRestore();
641+
});
642+
643+
// #14724 — run the whole block against BOTH values of the routing flag.
644+
// Covering only the default is how the split defect survived: a test that
645+
// exercises one branch of a flag cannot see the other.
646+
describe.each([
647+
['registerRoutes: true (default)', true],
648+
['registerRoutes: false (routes-less embedding)', false],
649+
] as const)('%s', (_label, registerRoutes) => {
650+
it('a zh-CN workspace binds zh-CN on the EMAIL channel, not just on SMS', async () => {
651+
const settings = makeSettings({ value: 'zh-CN', source: 'tenant' });
652+
await boot({ settings, i18nDefault: 'en', registerRoutes });
653+
654+
expect(settings.get).toHaveBeenCalledWith('localization', 'locale', {});
655+
// The regression, stated as the two channels agreeing. Before #14319 the
656+
// SMS assertion passed and the email one read 'en'.
657+
expect(setSmsLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
658+
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
659+
});
615660

616-
it('a zh-CN workspace binds zh-CN on the EMAIL channel, not just on SMS', async () => {
617-
const settings = makeSettings({ value: 'zh-CN', source: 'tenant' });
618-
await boot({ settings, i18nDefault: 'en' });
661+
it.each(['ja-JP', 'es-ES'])('and the same for a %s workspace', async (locale) => {
662+
await boot({
663+
settings: makeSettings({ value: locale, source: 'global' }),
664+
i18nDefault: 'en',
665+
registerRoutes,
666+
});
667+
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith(locale);
668+
});
619669

620-
expect(settings.get).toHaveBeenCalledWith('localization', 'locale', {});
621-
// The regression, stated as the two channels agreeing. Before #14319 the
622-
// SMS assertion passed and the email one read 'en'.
623-
expect(setSmsLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
624-
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
625-
});
670+
it('a workspace that never chose a language keeps the app build-time default', async () => {
671+
// `get` answers the manifest default ('en-US') for an untouched
672+
// workspace, so taking `value` unconditionally would demote every
673+
// deployment that declared `i18n.defaultLocale` — the #8195 behaviour
674+
// this change must not regress.
675+
await boot({
676+
settings: makeSettings({ value: 'en-US', source: 'default' }),
677+
i18nDefault: 'zh-CN',
678+
registerRoutes,
679+
});
680+
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
681+
});
626682

627-
it.each(['ja-JP', 'es-ES'])('and the same for a %s workspace', async (locale) => {
628-
await boot({ settings: makeSettings({ value: locale, source: 'global' }), i18nDefault: 'en' });
629-
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith(locale);
630-
});
683+
it('names NO locale when neither producer speaks — the documented en-US fallback', async () => {
684+
// The issue's second acceptance criterion: absent language ⇒ English.
685+
// Spelled as an ABSENT locale rather than 'en-US', because that is what
686+
// `EmailService`'s ladder contract ("no locale means the DOCUMENTED
687+
// default") is written against.
688+
await boot({
689+
settings: makeSettings({ value: 'en-US', source: 'default' }),
690+
registerRoutes,
691+
});
692+
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith(undefined);
693+
});
631694

632-
it('a workspace that never chose a language keeps the app build-time default', async () => {
633-
// `get` answers the manifest default ('en-US') for an untouched
634-
// workspace, so taking `value` unconditionally would demote every
635-
// deployment that declared `i18n.defaultLocale` — the #8195 behaviour
636-
// this change must not regress.
637-
await boot({
638-
settings: makeSettings({ value: 'en-US', source: 'default' }),
639-
i18nDefault: 'zh-CN',
695+
it('binds the build-time default when there is no settings service at all', async () => {
696+
await boot({ i18nDefault: 'ja-JP', registerRoutes });
697+
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('ja-JP');
640698
});
641-
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
642-
});
643699

644-
it('names NO locale when neither producer speaks — the documented en-US fallback', async () => {
645-
// The issue's second acceptance criterion: absent language ⇒ English.
646-
// Spelled as an ABSENT locale rather than 'en-US', because that is what
647-
// `EmailService`'s ladder contract ("no locale means the DOCUMENTED
648-
// default") is written against.
649-
await boot({ settings: makeSettings({ value: 'en-US', source: 'default' }) });
650-
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith(undefined);
651-
});
700+
it('re-binds live when the workspace switches language', async () => {
701+
const settings = makeSettings({ value: 'en-US', source: 'default' });
702+
await boot({ settings, i18nDefault: 'en', registerRoutes });
703+
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('en');
652704

653-
it('binds the build-time default when there is no settings service at all', async () => {
654-
await boot({ i18nDefault: 'ja-JP' });
655-
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('ja-JP');
656-
});
705+
expect(settings.subscribe).toHaveBeenCalledWith('localization', expect.any(Function));
706+
settings.set({ value: 'zh-CN', source: 'tenant' });
707+
for (const handler of settings.handlers) handler();
708+
// The subscribe handler is fire-and-forget (`void`); flush its promise.
709+
await new Promise((resolve) => setImmediate(resolve));
657710

658-
it('re-binds live when the workspace switches language', async () => {
659-
const settings = makeSettings({ value: 'en-US', source: 'default' });
660-
await boot({ settings, i18nDefault: 'en' });
661-
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('en');
711+
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
712+
expect(setSmsLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
713+
});
662714

663-
expect(settings.subscribe).toHaveBeenCalledWith('localization', expect.any(Function));
664-
settings.set({ value: 'zh-CN', source: 'tenant' });
665-
for (const handler of settings.handlers) handler();
666-
// The subscribe handler is fire-and-forget (`void`); flush its promise.
667-
await new Promise((resolve) => setImmediate(resolve));
715+
it('leaves the build-time default standing when the settings read fails', async () => {
716+
const settings = makeSettings(new Error('boom'));
717+
await expect(
718+
boot({ settings, i18nDefault: 'zh-CN', registerRoutes }),
719+
).resolves.toBeUndefined();
720+
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
721+
expect(mockContext.logger.warn).toHaveBeenCalledWith(
722+
expect.stringContaining('failed to apply localization.locale'),
723+
);
724+
});
668725

669-
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
670-
expect(setSmsLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
671-
});
726+
// ── #14724 acceptance criterion 1 ──────────────────────────────────
727+
// The service composition is complete at the end of `kernel:ready`
728+
// whichever way the routing flag is set. Measured before the split with
729+
// `registerRoutes: false`: settingsRead [], emailLocaleCalls [],
730+
// smsLocaleCalls [], emailServiceCalls [] — the namespace was not even
731+
// read.
732+
it('ends kernel:ready with the transport wired and BOTH locales bound', async () => {
733+
const settings = makeSettings({ value: 'zh-CN', source: 'tenant' });
734+
const email = { send: vi.fn() };
735+
await boot({ settings, email, i18nDefault: 'en', registerRoutes });
736+
737+
expect(mockContext.getService).toHaveBeenCalledWith('email');
738+
expect(setEmailServiceSpy).toHaveBeenCalledWith(email);
739+
expect(settings.get).toHaveBeenCalledWith('localization', 'locale', {});
740+
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
741+
expect(setSmsLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
742+
});
672743

673-
it('leaves the build-time default standing when the settings read fails', async () => {
674-
const settings = makeSettings(new Error('boom'));
675-
await expect(boot({ settings, i18nDefault: 'zh-CN' })).resolves.toBeUndefined();
676-
expect(setEmailLocaleSpy).toHaveBeenLastCalledWith('zh-CN');
677-
expect(mockContext.logger.warn).toHaveBeenCalledWith(
678-
expect.stringContaining('failed to apply localization.locale'),
679-
);
744+
// ── #14724 acceptance criterion 2 — the regression guard ───────────
745+
// Route registration itself stays gated. This is the half that must
746+
// NOT move: `registerRoutes: false` mounts no auth routes, and it is
747+
// pinned here rather than left to prose.
748+
it(`${registerRoutes ? 'mounts' : 'mounts NO'} auth routes`, async () => {
749+
await boot({
750+
settings: makeSettings({ value: 'zh-CN', source: 'tenant' }),
751+
email: { send: vi.fn() },
752+
i18nDefault: 'en',
753+
registerRoutes,
754+
});
755+
756+
const httpServerLookups = (mockContext.getService as ReturnType<typeof vi.fn>).mock.calls
757+
.filter((args: unknown[]) => args[0] === 'http-server');
758+
759+
if (registerRoutes) {
760+
expect(httpServerLookups.length).toBeGreaterThan(0);
761+
expect(httpServer.getRawApp).toHaveBeenCalled();
762+
expect(rawApp.all).toHaveBeenCalledWith('/api/v1/auth/*', expect.any(Function));
763+
} else {
764+
// The `http-server` service is present in this harness, so a zero
765+
// lookup count is a statement about the gate and not about the
766+
// service being absent.
767+
expect(httpServerLookups.length).toBe(0);
768+
expect(httpServer.getRawApp).not.toHaveBeenCalled();
769+
expect(rawApp.all).not.toHaveBeenCalled();
770+
}
771+
});
680772
});
681773
});
682774

0 commit comments

Comments
 (0)