From d6e89f96d66c5c84ba968749ffa6f7f3d2965670 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 4 Apr 2026 02:29:10 +0900 Subject: [PATCH] refactor: share gateway config auth helpers --- src/gateway/auth.test.ts | 51 ++++++++ src/gateway/auth.ts | 27 +++++ .../server-methods/config.shared-auth.test.ts | 109 ++++++------------ .../server-methods/config.test-helpers.ts | 67 +++++++++++ src/gateway/server-methods/config.test.ts | 41 ++----- src/gateway/server-methods/config.ts | 38 ++---- .../server.shared-auth-rotation.test.ts | 98 ++++++---------- 7 files changed, 241 insertions(+), 190 deletions(-) create mode 100644 src/gateway/server-methods/config.test-helpers.ts diff --git a/src/gateway/auth.test.ts b/src/gateway/auth.test.ts index 71265e6aae1..9a56dd1a8ed 100644 --- a/src/gateway/auth.test.ts +++ b/src/gateway/auth.test.ts @@ -4,6 +4,7 @@ import { assertGatewayAuthConfigured, authorizeGatewayConnect, authorizeHttpGatewayConnect, + resolveEffectiveSharedGatewayAuth, authorizeWsControlUiGatewayConnect, resolveGatewayAuth, } from "./auth.js"; @@ -104,6 +105,56 @@ describe("gateway auth", () => { }); }); + it("resolves the active shared token auth only", () => { + expect( + resolveEffectiveSharedGatewayAuth({ + authConfig: { + mode: "token", + token: "config-token", + password: "config-password", + }, + env: {} as NodeJS.ProcessEnv, + }), + ).toEqual({ + mode: "token", + secret: "config-token", + }); + }); + + it("resolves the active shared password auth only", () => { + expect( + resolveEffectiveSharedGatewayAuth({ + authConfig: { + mode: "password", + token: "config-token", + password: "config-password", + }, + env: {} as NodeJS.ProcessEnv, + }), + ).toEqual({ + mode: "password", + secret: "config-password", + }); + }); + + it("returns null for non-shared gateway auth modes", () => { + expect( + resolveEffectiveSharedGatewayAuth({ + authConfig: { mode: "none" }, + env: {} as NodeJS.ProcessEnv, + }), + ).toBeNull(); + expect( + resolveEffectiveSharedGatewayAuth({ + authConfig: { + mode: "trusted-proxy", + trustedProxy: { userHeader: "x-user" }, + }, + env: {} as NodeJS.ProcessEnv, + }), + ).toBeNull(); + }); + it("keeps gateway auth config values ahead of env overrides", () => { expect( resolveGatewayAuth({ diff --git a/src/gateway/auth.ts b/src/gateway/auth.ts index 42802548d16..e445181d9be 100644 --- a/src/gateway/auth.ts +++ b/src/gateway/auth.ts @@ -38,6 +38,11 @@ export type ResolvedGatewayAuth = { trustedProxy?: GatewayTrustedProxyConfig; }; +export type EffectiveSharedGatewayAuth = { + mode: "token" | "password"; + secret: string | undefined; +}; + export type GatewayAuthResult = { ok: boolean; method?: @@ -288,6 +293,28 @@ export function resolveGatewayAuth(params: { }; } +export function resolveEffectiveSharedGatewayAuth(params: { + authConfig?: GatewayAuthConfig | null; + authOverride?: GatewayAuthConfig | null; + env?: NodeJS.ProcessEnv; + tailscaleMode?: GatewayTailscaleMode; +}): EffectiveSharedGatewayAuth | null { + const resolvedAuth = resolveGatewayAuth(params); + if (resolvedAuth.mode === "token") { + return { + mode: "token", + secret: resolvedAuth.token, + }; + } + if (resolvedAuth.mode === "password") { + return { + mode: "password", + secret: resolvedAuth.password, + }; + } + return null; +} + export function assertGatewayAuthConfigured( auth: ResolvedGatewayAuth, rawAuthConfig?: GatewayAuthConfig | null, diff --git a/src/gateway/server-methods/config.shared-auth.test.ts b/src/gateway/server-methods/config.shared-auth.test.ts index 22bc9467661..9a93c9e1aa9 100644 --- a/src/gateway/server-methods/config.shared-auth.test.ts +++ b/src/gateway/server-methods/config.shared-auth.test.ts @@ -1,6 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; -import type { GatewayRequestHandlerOptions } from "./types.js"; +import { + createConfigHandlerHarness, + createConfigWriteSnapshot, + flushConfigHandlerMicrotasks, +} from "./config.test-helpers.js"; const readConfigFileSnapshotForWriteMock = vi.fn(); const writeConfigFileMock = vi.fn(); @@ -38,54 +42,6 @@ vi.mock("../../infra/restart.js", () => ({ const { configHandlers } = await import("./config.js"); -function createConfigSnapshot(config: OpenClawConfig) { - return { - snapshot: { - path: "/tmp/openclaw.json", - exists: true, - raw: JSON.stringify(config, null, 2), - parsed: config, - sourceConfig: config, - resolved: config, - valid: true, - runtimeConfig: config, - config, - hash: "base-hash", - issues: [], - warnings: [], - legacyIssues: [], - }, - writeOptions: {} as Record, - }; -} - -function createOptions( - params: unknown, - contextOverrides?: Partial, -): GatewayRequestHandlerOptions { - return { - req: { type: "req", id: "1", method: "config.patch" }, - params, - client: null, - isWebchatConnect: () => false, - respond: vi.fn(), - context: { - logGateway: { - error: vi.fn(), - warn: vi.fn(), - info: vi.fn(), - debug: vi.fn(), - }, - disconnectClientsUsingSharedGatewayAuth: vi.fn(), - ...contextOverrides, - }, - } as unknown as GatewayRequestHandlerOptions; -} - -async function flushMicrotaskQueue() { - await new Promise((resolve) => queueMicrotask(resolve)); -} - afterEach(() => { vi.clearAllMocks(); }); @@ -116,18 +72,21 @@ describe("config shared auth disconnects", () => { }, }, }; - readConfigFileSnapshotForWriteMock.mockResolvedValue(createConfigSnapshot(prevConfig)); + readConfigFileSnapshotForWriteMock.mockResolvedValue(createConfigWriteSnapshot(prevConfig)); - const opts = createOptions({ - raw: JSON.stringify(nextConfig, null, 2), - baseHash: "base-hash", + const { options, disconnectClientsUsingSharedGatewayAuth } = createConfigHandlerHarness({ + method: "config.set", + params: { + raw: JSON.stringify(nextConfig, null, 2), + baseHash: "base-hash", + }, }); - await configHandlers["config.set"](opts); - await flushMicrotaskQueue(); + await configHandlers["config.set"](options); + await flushConfigHandlerMicrotasks(); expect(writeConfigFileMock).toHaveBeenCalledWith(nextConfig, {}); - expect(opts.context.disconnectClientsUsingSharedGatewayAuth).not.toHaveBeenCalled(); + expect(disconnectClientsUsingSharedGatewayAuth).not.toHaveBeenCalled(); expect(scheduleGatewaySigusr1RestartMock).not.toHaveBeenCalled(); }); @@ -140,19 +99,22 @@ describe("config shared auth disconnects", () => { }, }, }; - readConfigFileSnapshotForWriteMock.mockResolvedValue(createConfigSnapshot(prevConfig)); + readConfigFileSnapshotForWriteMock.mockResolvedValue(createConfigWriteSnapshot(prevConfig)); - const opts = createOptions({ - baseHash: "base-hash", - raw: JSON.stringify({ gateway: { auth: { token: "new-token" } } }), - restartDelayMs: 1_000, + const { options, disconnectClientsUsingSharedGatewayAuth } = createConfigHandlerHarness({ + method: "config.patch", + params: { + baseHash: "base-hash", + raw: JSON.stringify({ gateway: { auth: { token: "new-token" } } }), + restartDelayMs: 1_000, + }, }); - await configHandlers["config.patch"](opts); - await flushMicrotaskQueue(); + await configHandlers["config.patch"](options); + await flushConfigHandlerMicrotasks(); expect(scheduleGatewaySigusr1RestartMock).toHaveBeenCalledTimes(1); - expect(opts.context.disconnectClientsUsingSharedGatewayAuth).toHaveBeenCalledTimes(1); + expect(disconnectClientsUsingSharedGatewayAuth).toHaveBeenCalledTimes(1); }); it("does not disconnect shared-auth clients when config.patch changes only inactive password auth", async () => { @@ -164,18 +126,21 @@ describe("config shared auth disconnects", () => { }, }, }; - readConfigFileSnapshotForWriteMock.mockResolvedValue(createConfigSnapshot(prevConfig)); + readConfigFileSnapshotForWriteMock.mockResolvedValue(createConfigWriteSnapshot(prevConfig)); - const opts = createOptions({ - baseHash: "base-hash", - raw: JSON.stringify({ gateway: { auth: { password: "new-password" } } }), - restartDelayMs: 1_000, + const { options, disconnectClientsUsingSharedGatewayAuth } = createConfigHandlerHarness({ + method: "config.patch", + params: { + baseHash: "base-hash", + raw: JSON.stringify({ gateway: { auth: { password: "new-password" } } }), + restartDelayMs: 1_000, + }, }); - await configHandlers["config.patch"](opts); - await flushMicrotaskQueue(); + await configHandlers["config.patch"](options); + await flushConfigHandlerMicrotasks(); expect(scheduleGatewaySigusr1RestartMock).toHaveBeenCalledTimes(1); - expect(opts.context.disconnectClientsUsingSharedGatewayAuth).not.toHaveBeenCalled(); + expect(disconnectClientsUsingSharedGatewayAuth).not.toHaveBeenCalled(); }); }); diff --git a/src/gateway/server-methods/config.test-helpers.ts b/src/gateway/server-methods/config.test-helpers.ts new file mode 100644 index 00000000000..3916d939173 --- /dev/null +++ b/src/gateway/server-methods/config.test-helpers.ts @@ -0,0 +1,67 @@ +import { vi } from "vitest"; +import type { OpenClawConfig } from "../../config/types.openclaw.js"; +import type { GatewayRequestHandlerOptions } from "./types.js"; + +function createGatewayLog() { + return { + error: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + debug: vi.fn(), + }; +} + +export function createConfigWriteSnapshot(config: OpenClawConfig) { + return { + snapshot: { + path: "/tmp/openclaw.json", + exists: true, + raw: JSON.stringify(config, null, 2), + parsed: config, + sourceConfig: config, + resolved: config, + valid: true, + runtimeConfig: config, + config, + hash: "base-hash", + issues: [], + warnings: [], + legacyIssues: [], + }, + writeOptions: {} as Record, + }; +} + +export function createConfigHandlerHarness(args?: { + method?: string; + params?: unknown; + overrides?: Partial; + contextOverrides?: Partial; +}) { + const logGateway = createGatewayLog(); + const disconnectClientsUsingSharedGatewayAuth = vi.fn(); + const respond = vi.fn(); + const options = { + req: { type: "req", id: "1", method: args?.method ?? "config.get" }, + params: args?.params ?? {}, + client: null, + isWebchatConnect: () => false, + respond, + context: { + logGateway, + disconnectClientsUsingSharedGatewayAuth, + ...args?.contextOverrides, + }, + ...args?.overrides, + } as unknown as GatewayRequestHandlerOptions; + return { + options, + respond, + logGateway, + disconnectClientsUsingSharedGatewayAuth, + }; +} + +export async function flushConfigHandlerMicrotasks() { + await new Promise((resolve) => queueMicrotask(resolve)); +} diff --git a/src/gateway/server-methods/config.test.ts b/src/gateway/server-methods/config.test.ts index e6ef3d52d96..182428f5c2c 100644 --- a/src/gateway/server-methods/config.test.ts +++ b/src/gateway/server-methods/config.test.ts @@ -1,7 +1,7 @@ import { execFile } from "node:child_process"; import { afterEach, describe, expect, it, vi } from "vitest"; import { configHandlers, resolveConfigOpenCommand } from "./config.js"; -import type { GatewayRequestHandlerOptions } from "./types.js"; +import { createConfigHandlerHarness } from "./config.test-helpers.js"; vi.mock("node:child_process", async (importOriginal) => { const actual = await importOriginal(); @@ -17,27 +17,6 @@ function invokeExecFileCallback(args: unknown[], error: Error | null) { (callback as (error: Error | null) => void)(error); } -function createOptions( - overrides?: Partial, -): GatewayRequestHandlerOptions { - return { - req: { type: "req", id: "1", method: "config.openFile" }, - params: {}, - client: null, - isWebchatConnect: () => false, - respond: vi.fn(), - context: { - logGateway: { - error: vi.fn(), - warn: vi.fn(), - info: vi.fn(), - debug: vi.fn(), - }, - }, - ...overrides, - } as unknown as GatewayRequestHandlerOptions; -} - describe("resolveConfigOpenCommand", () => { it("uses open on macOS", () => { expect(resolveConfigOpenCommand("/tmp/openclaw.json", "darwin")).toEqual({ @@ -81,10 +60,10 @@ describe("config.openFile", () => { return {} as never; }) as unknown as typeof execFile); - const opts = createOptions(); - await configHandlers["config.openFile"](opts); + const { options, respond } = createConfigHandlerHarness({ method: "config.openFile" }); + await configHandlers["config.openFile"](options); - expect(opts.respond).toHaveBeenCalledWith( + expect(respond).toHaveBeenCalledWith( true, { ok: true, @@ -104,10 +83,12 @@ describe("config.openFile", () => { return {} as never; }) as unknown as typeof execFile); - const opts = createOptions(); - await configHandlers["config.openFile"](opts); + const { options, respond, logGateway } = createConfigHandlerHarness({ + method: "config.openFile", + }); + await configHandlers["config.openFile"](options); - expect(opts.respond).toHaveBeenCalledWith( + expect(respond).toHaveBeenCalledWith( true, { ok: false, @@ -116,8 +97,6 @@ describe("config.openFile", () => { }, undefined, ); - expect(opts.context.logGateway.warn).toHaveBeenCalledWith( - expect.stringContaining("spawn xdg-open ENOENT"), - ); + expect(logGateway.warn).toHaveBeenCalledWith(expect.stringContaining("spawn xdg-open ENOENT")); }); }); diff --git a/src/gateway/server-methods/config.ts b/src/gateway/server-methods/config.ts index 0935185878f..a0c46f0e121 100644 --- a/src/gateway/server-methods/config.ts +++ b/src/gateway/server-methods/config.ts @@ -28,7 +28,7 @@ import { } from "../../infra/restart-sentinel.js"; import { scheduleGatewaySigusr1Restart } from "../../infra/restart.js"; import { prepareSecretsRuntimeSnapshot } from "../../secrets/runtime.js"; -import { resolveGatewayAuth } from "../auth.js"; +import { resolveEffectiveSharedGatewayAuth } from "../auth.js"; import { diffConfigPaths } from "../config-reload.js"; import { formatControlPlaneActor, @@ -222,33 +222,17 @@ function parseValidateConfigFromRawOrRespond( return { config: validated.config, schema }; } -function getEffectiveSharedGatewayAuth(config: OpenClawConfig): { - mode: "token" | "password"; - secret: string | undefined; -} | null { - const resolvedAuth = resolveGatewayAuth({ - authConfig: config.gateway?.auth, - env: process.env, - tailscaleMode: config.gateway?.tailscale?.mode, - }); - if (resolvedAuth.mode === "token") { - return { - mode: "token", - secret: resolvedAuth.token, - }; - } - if (resolvedAuth.mode === "password") { - return { - mode: "password", - secret: resolvedAuth.password, - }; - } - return null; -} - function didSharedGatewayAuthChange(prev: OpenClawConfig, next: OpenClawConfig): boolean { - const prevAuth = getEffectiveSharedGatewayAuth(prev); - const nextAuth = getEffectiveSharedGatewayAuth(next); + const prevAuth = resolveEffectiveSharedGatewayAuth({ + authConfig: prev.gateway?.auth, + env: process.env, + tailscaleMode: prev.gateway?.tailscale?.mode, + }); + const nextAuth = resolveEffectiveSharedGatewayAuth({ + authConfig: next.gateway?.auth, + env: process.env, + tailscaleMode: next.gateway?.tailscale?.mode, + }); if (prevAuth === null || nextAuth === null) { return prevAuth !== nextAuth; } diff --git a/src/gateway/server.shared-auth-rotation.test.ts b/src/gateway/server.shared-auth-rotation.test.ts index d825569e198..c9fd47052ad 100644 --- a/src/gateway/server.shared-auth-rotation.test.ts +++ b/src/gateway/server.shared-auth-rotation.test.ts @@ -19,7 +19,6 @@ const ORIGINAL_GATEWAY_TOKEN_ENV = process.env.OPENCLAW_GATEWAY_TOKEN; const OLD_TOKEN = "shared-token-old"; const NEW_TOKEN = "shared-token-new"; const DEFERRED_RESTART_DELAY_MS = 1_000; -const RESTARTING_AUTH_CHANGE_METHODS = ["config.patch", "config.apply"] as const; const SECRET_REF_TOKEN_ID = "OPENCLAW_SHARED_AUTH_ROTATION_SECRET_REF"; let server: Awaited>; @@ -111,52 +110,43 @@ async function loadCurrentConfig(ws: WebSocket): Promise<{ }; } -async function sendAuthTokenChange( - ws: WebSocket, - method: (typeof RESTARTING_AUTH_CHANGE_METHODS)[number], -): Promise<{ ok: boolean }> { +async function sendSharedTokenRotationPatch(ws: WebSocket): Promise<{ ok: boolean }> { const current = await loadCurrentConfig(ws); - const currentConfig = current.config; - const gateway = (currentConfig.gateway ??= {}) as Record; - const auth = (gateway.auth ??= {}) as Record; - auth.token = NEW_TOKEN; + return await rpcReq(ws, "config.patch", { + baseHash: current.hash, + raw: JSON.stringify({ gateway: { auth: { token: NEW_TOKEN } } }), + restartDelayMs: DEFERRED_RESTART_DELAY_MS, + }); +} - if (method === "config.patch") { - return await rpcReq(ws, "config.patch", { - baseHash: current.hash, - raw: JSON.stringify({ gateway: { auth: { token: NEW_TOKEN } } }), - restartDelayMs: DEFERRED_RESTART_DELAY_MS, - }); - } - - return await rpcReq(ws, method, { - raw: JSON.stringify(currentConfig, null, 2), +async function applyCurrentConfig(ws: WebSocket) { + const current = await loadCurrentConfig(ws); + return await rpcReq(ws, "config.apply", { + raw: JSON.stringify(current.config, null, 2), }); } describe("gateway shared auth rotation", () => { - for (const method of RESTARTING_AUTH_CHANGE_METHODS) { - it(`disconnects existing shared-token websocket sessions after ${method}`, async () => { - const ws = await openAuthenticatedWs(OLD_TOKEN); - try { - const closed = waitForClose(ws); - const res = await sendAuthTokenChange(ws, method); + it("disconnects existing shared-token websocket sessions after config.patch rotates auth", async () => { + const ws = await openAuthenticatedWs(OLD_TOKEN); + try { + const closed = waitForClose(ws); + const res = await sendSharedTokenRotationPatch(ws); - expect(res.ok).toBe(true); - await expect(closed).resolves.toMatchObject({ - code: 4001, - reason: "gateway auth changed", - }); - } finally { - ws.close(); - } - }); - } + expect(res.ok).toBe(true); + await expect(closed).resolves.toMatchObject({ + code: 4001, + reason: "gateway auth changed", + }); + } finally { + ws.close(); + } + }); it("keeps existing device-token websocket sessions connected after shared token rotation", async () => { const ws = await openDeviceTokenWs(); try { - const res = await sendAuthTokenChange(ws, "config.patch"); + const res = await sendSharedTokenRotationPatch(ws); expect(res.ok).toBe(true); const followUp = await rpcReq<{ hash?: string }>(ws, "config.get", {}); @@ -196,29 +186,17 @@ describe("gateway shared auth rotation with unchanged SecretRefs", () => { return ws; } - for (const method of ["config.set", "config.apply"] as const) { - it(`keeps shared-auth websocket sessions connected when ${method} reapplies an unchanged SecretRef token`, async () => { - const ws = await openSecretRefAuthenticatedWs(); - try { - const current = await rpcReq<{ - hash?: string; - config?: Record; - }>(ws, "config.get", {}); - expect(current.ok).toBe(true); - expect(typeof current.payload?.hash).toBe("string"); + it("keeps shared-auth websocket sessions connected when config.apply reapplies an unchanged SecretRef token", async () => { + const ws = await openSecretRefAuthenticatedWs(); + try { + const res = await applyCurrentConfig(ws); + expect(res.ok).toBe(true); - const res = await rpcReq(ws, method, { - raw: JSON.stringify(current.payload?.config ?? {}, null, 2), - ...(method === "config.set" ? { baseHash: current.payload?.hash } : {}), - }); - expect(res.ok).toBe(true); - - const followUp = await rpcReq<{ hash?: string }>(ws, "config.get", {}); - expect(followUp.ok).toBe(true); - expect(typeof followUp.payload?.hash).toBe("string"); - } finally { - ws.close(); - } - }); - } + const followUp = await rpcReq<{ hash?: string }>(ws, "config.get", {}); + expect(followUp.ok).toBe(true); + expect(typeof followUp.payload?.hash).toBe("string"); + } finally { + ws.close(); + } + }); });