From f8405e731b35bf20abf501851ec08d929aa2ca31 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 31 Mar 2026 21:46:48 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=82=EF=B8=8F=20fix:=20Allow=20Empty-Ov?= =?UTF-8?q?errides=20Scope=20Creation=20in=20Admin=20Config=20(#12492)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Allow empty-overrides scope creation when priority is provided The upsertConfigOverrides handler short-circuited when overrides was empty, returning a plain message instead of creating the config document. This broke the admin panel's "create blank scope" flow which sends `{ overrides: {}, priority: N }` — the missing `config` property in the response caused an `_id` error on the client. The early return now only triggers when both overrides are empty and no priority is provided. Per-section permission checks are scoped to cases where override sections are actually present. * test: Add tests for empty-overrides scope creation with priority * test: Address review nits for empty-overrides scope tests - Add res.statusCode/res.body assertions to capability-check test - Add 403/401 tests for empty overrides + priority path - Use mockResolvedValue(null) for consistency on bare jest.fn() - Remove narrating comment; fold intent into test name --- packages/api/src/admin/config.handler.spec.ts | 87 +++++++++++++++++++ packages/api/src/admin/config.ts | 20 +++-- 2 files changed, 98 insertions(+), 9 deletions(-) diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index bd1664fc90..ad33f5f158 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -535,6 +535,93 @@ describe('createAdminConfigHandlers', () => { }); }); + describe('upsertConfigOverrides — empty-overrides scope creation', () => { + it('creates config document when overrides is empty but priority is provided', async () => { + const upsertConfig = jest.fn().mockResolvedValue({ + _id: 'c1', + principalType: 'role', + principalId: 'admin', + overrides: {}, + configVersion: 1, + }); + const { handlers } = createHandlers({ upsertConfig }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { overrides: {}, priority: 5 }, + }); + const res = mockRes(); + + await handlers.upsertConfigOverrides(req, res); + + expect(res.statusCode).toBe(201); + expect(res.body).toHaveProperty('config'); + expect(res.body?.config).toHaveProperty('_id', 'c1'); + expect(upsertConfig).toHaveBeenCalledWith('role', 'admin', expect.anything(), {}, 5); + }); + + it('returns no-op message when overrides is empty and no priority is provided', async () => { + const upsertConfig = jest.fn().mockResolvedValue(null); + const { handlers } = createHandlers({ upsertConfig }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { overrides: {} }, + }); + const res = mockRes(); + + await handlers.upsertConfigOverrides(req, res); + + expect(res.statusCode).toBe(200); + expect(res.body).toHaveProperty('message', 'No actionable override sections provided'); + expect(upsertConfig).not.toHaveBeenCalled(); + }); + + it('calls general manage check exactly once when overrides is empty with priority', async () => { + const hasConfigCapability = jest.fn().mockResolvedValue(true); + const { handlers } = createHandlers({ hasConfigCapability }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { overrides: {}, priority: 3 }, + }); + const res = mockRes(); + + await handlers.upsertConfigOverrides(req, res); + + expect(hasConfigCapability).toHaveBeenCalledTimes(1); + expect(hasConfigCapability).toHaveBeenCalledWith(expect.anything(), null, 'manage'); + expect(res.statusCode).toBe(201); + expect(res.body).toHaveProperty('config'); + }); + + it('returns 403 for empty overrides with priority when user lacks MANAGE_CONFIGS', async () => { + const { handlers } = createHandlers({ + hasConfigCapability: jest.fn().mockResolvedValue(false), + }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { overrides: {}, priority: 5 }, + }); + const res = mockRes(); + + await handlers.upsertConfigOverrides(req, res); + + expect(res.statusCode).toBe(403); + }); + + it('returns 401 for empty overrides with priority when unauthenticated', async () => { + const { handlers } = createHandlers(); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { overrides: {}, priority: 5 }, + user: undefined, + }); + const res = mockRes(); + + await handlers.upsertConfigOverrides(req, res); + + expect(res.statusCode).toBe(401); + }); + }); + // ── Invariant tests: rules that must hold across ALL handlers ────── const MUTATION_HANDLERS: Array<{ diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index cca2d9901c..357096da9b 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -333,18 +333,20 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps) { const overrideSections = Object.keys(filteredOverrides); - if (overrideSections.length === 0) { + if (overrideSections.length === 0 && priority == null) { return res.status(200).json({ message: 'No actionable override sections provided' }); } - const allowed = await Promise.all( - overrideSections.map((s) => hasConfigCapability(user, s as ConfigSection, 'manage')), - ); - const denied = overrideSections.find((_, i) => !allowed[i]); - if (denied) { - return res.status(403).json({ - error: `Insufficient permissions for config section: ${denied}`, - }); + if (overrideSections.length > 0) { + const allowed = await Promise.all( + overrideSections.map((s) => hasConfigCapability(user, s as ConfigSection, 'manage')), + ); + const denied = overrideSections.find((_, i) => !allowed[i]); + if (denied) { + return res.status(403).json({ + error: `Insufficient permissions for config section: ${denied}`, + }); + } } const config = await upsertConfig(