From ad9481e2d11cf206ee6920a842f46807fdc59489 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 6 Apr 2026 16:51:09 +0100 Subject: [PATCH] refactor: dedupe auth and session helpers --- src/agents/tools/sessions-resolution.ts | 199 +++++++++------- src/auto-reply/command-auth.ts | 288 ++++++++++++++++-------- src/auto-reply/command-control.test.ts | 33 +++ 3 files changed, 351 insertions(+), 169 deletions(-) diff --git a/src/agents/tools/sessions-resolution.ts b/src/agents/tools/sessions-resolution.ts index 34c1265b573..243cd16d32d 100644 --- a/src/agents/tools/sessions-resolution.ts +++ b/src/agents/tools/sessions-resolution.ts @@ -201,6 +201,55 @@ export type VisibleSessionReferenceResolution = displayKey: string; }; +function buildResolvedSessionReference(params: { + key: string; + alias: string; + mainKey: string; + resolvedViaSessionId: boolean; +}): Extract { + return { + ok: true, + key: params.key, + displayKey: resolveDisplaySessionKey({ + key: params.key, + alias: params.alias, + mainKey: params.mainKey, + }), + resolvedViaSessionId: params.resolvedViaSessionId, + }; +} + +function buildSessionIdResolveParams(params: { + sessionId: string; + requesterInternalKey?: string; + restrictToSpawned: boolean; +}) { + return { + sessionId: params.sessionId, + spawnedBy: params.restrictToSpawned ? params.requesterInternalKey : undefined, + includeGlobal: !params.restrictToSpawned, + includeUnknown: !params.restrictToSpawned, + }; +} + +async function callGatewayResolveSessionId(params: { + sessionId: string; + requesterInternalKey?: string; + restrictToSpawned: boolean; +}): Promise { + const result = await sessionsResolutionDeps.callGateway<{ key?: string }>({ + method: "sessions.resolve", + params: buildSessionIdResolveParams(params), + }); + const key = typeof result?.key === "string" ? result.key.trim() : ""; + if (!key) { + throw new Error( + `Session not found: ${params.sessionId} (use the full sessionKey from sessions_list)`, + ); + } + return key; +} + async function resolveSessionKeyFromSessionId(params: { sessionId: string; alias: string; @@ -210,31 +259,13 @@ async function resolveSessionKeyFromSessionId(params: { }): Promise { try { // Resolve via gateway so we respect store routing and visibility rules. - const result = await sessionsResolutionDeps.callGateway<{ key?: string }>({ - method: "sessions.resolve", - params: { - sessionId: params.sessionId, - spawnedBy: params.restrictToSpawned ? params.requesterInternalKey : undefined, - includeGlobal: !params.restrictToSpawned, - includeUnknown: !params.restrictToSpawned, - }, - }); - const key = typeof result?.key === "string" ? result.key.trim() : ""; - if (!key) { - throw new Error( - `Session not found: ${params.sessionId} (use the full sessionKey from sessions_list)`, - ); - } - return { - ok: true, + const key = await callGatewayResolveSessionId(params); + return buildResolvedSessionReference({ key, - displayKey: resolveDisplaySessionKey({ - key, - alias: params.alias, - mainKey: params.mainKey, - }), + alias: params.alias, + mainKey: params.mainKey, resolvedViaSessionId: true, - }; + }); } catch (err) { if (params.restrictToSpawned) { return { @@ -274,16 +305,12 @@ async function resolveSessionKeyFromKey(params: { if (!key) { return null; } - return { - ok: true, + return buildResolvedSessionReference({ key, - displayKey: resolveDisplaySessionKey({ - key, - alias: params.alias, - mainKey: params.mainKey, - }), + alias: params.alias, + mainKey: params.mainKey, resolvedViaSessionId: false, - }; + }); } catch { return null; } @@ -297,34 +324,62 @@ async function tryResolveSessionKeyFromSessionId(params: { restrictToSpawned: boolean; }): Promise | null> { try { - const result = await sessionsResolutionDeps.callGateway<{ key?: string }>({ - method: "sessions.resolve", - params: { - sessionId: params.sessionId, - spawnedBy: params.restrictToSpawned ? params.requesterInternalKey : undefined, - includeGlobal: !params.restrictToSpawned, - includeUnknown: !params.restrictToSpawned, - }, - }); - const key = typeof result?.key === "string" ? result.key.trim() : ""; - if (!key) { - return null; - } - return { - ok: true, + const key = await callGatewayResolveSessionId(params); + return buildResolvedSessionReference({ key, - displayKey: resolveDisplaySessionKey({ - key, - alias: params.alias, - mainKey: params.mainKey, - }), + alias: params.alias, + mainKey: params.mainKey, resolvedViaSessionId: true, - }; + }); } catch { return null; } } +async function resolveSessionReferenceByKeyOrSessionId(params: { + raw: string; + alias: string; + mainKey: string; + requesterInternalKey?: string; + restrictToSpawned: boolean; + allowUnresolvedSessionId: boolean; + skipKeyLookup?: boolean; + forceSessionIdLookup?: boolean; +}): Promise { + if (!params.skipKeyLookup) { + // Prefer key resolution to avoid misclassifying custom keys as sessionIds. + const resolvedByKey = await resolveSessionKeyFromKey({ + key: params.raw, + alias: params.alias, + mainKey: params.mainKey, + requesterInternalKey: params.requesterInternalKey, + restrictToSpawned: params.restrictToSpawned, + }); + if (resolvedByKey) { + return resolvedByKey; + } + } + if (!(params.forceSessionIdLookup || shouldResolveSessionIdInput(params.raw))) { + return null; + } + if (params.allowUnresolvedSessionId) { + return await tryResolveSessionKeyFromSessionId({ + sessionId: params.raw, + alias: params.alias, + mainKey: params.mainKey, + requesterInternalKey: params.requesterInternalKey, + restrictToSpawned: params.restrictToSpawned, + }); + } + return await resolveSessionKeyFromSessionId({ + sessionId: params.raw, + alias: params.alias, + mainKey: params.mainKey, + requesterInternalKey: params.requesterInternalKey, + restrictToSpawned: params.restrictToSpawned, + }); +} + export async function resolveSessionReference(params: { sessionKey: string; alias: string; @@ -334,50 +389,34 @@ export async function resolveSessionReference(params: { }): Promise { const rawInput = params.sessionKey.trim(); if (rawInput === "current") { - if (!params.restrictToSpawned) { - const resolvedByKey = await resolveSessionKeyFromKey({ - key: rawInput, - alias: params.alias, - mainKey: params.mainKey, - requesterInternalKey: params.requesterInternalKey, - restrictToSpawned: false, - }); - if (resolvedByKey) { - return resolvedByKey; - } - } - const resolvedBySessionId = await tryResolveSessionKeyFromSessionId({ - sessionId: rawInput, + const resolvedCurrent = await resolveSessionReferenceByKeyOrSessionId({ + raw: rawInput, alias: params.alias, mainKey: params.mainKey, requesterInternalKey: params.requesterInternalKey, restrictToSpawned: params.restrictToSpawned, + allowUnresolvedSessionId: true, + skipKeyLookup: params.restrictToSpawned, + forceSessionIdLookup: true, }); - if (resolvedBySessionId) { - return resolvedBySessionId; + if (resolvedCurrent) { + return resolvedCurrent; } } const raw = rawInput === "current" && params.requesterInternalKey ? params.requesterInternalKey : rawInput; if (shouldResolveSessionIdInput(raw)) { - // Prefer key resolution to avoid misclassifying custom keys as sessionIds. - const resolvedByKey = await resolveSessionKeyFromKey({ - key: raw, + const resolvedByGateway = await resolveSessionReferenceByKeyOrSessionId({ + raw, alias: params.alias, mainKey: params.mainKey, requesterInternalKey: params.requesterInternalKey, restrictToSpawned: params.restrictToSpawned, + allowUnresolvedSessionId: false, }); - if (resolvedByKey) { - return resolvedByKey; + if (resolvedByGateway) { + return resolvedByGateway; } - return await resolveSessionKeyFromSessionId({ - sessionId: raw, - alias: params.alias, - mainKey: params.mainKey, - requesterInternalKey: params.requesterInternalKey, - restrictToSpawned: params.restrictToSpawned, - }); } const resolvedKey = resolveInternalSessionKey({ diff --git a/src/auto-reply/command-auth.ts b/src/auto-reply/command-auth.ts index 060ba242c3f..da6fa78f881 100644 --- a/src/auto-reply/command-auth.ts +++ b/src/auto-reply/command-auth.ts @@ -30,6 +30,20 @@ type InferredProviderProbe = { droppedResolutionError: boolean; }; +type ProviderAllowFromResolution = { + allowFrom: Array; + allowFromList: string[]; + hadResolutionError: boolean; +}; + +type OwnerAuthorizationState = { + allowAll: boolean; + ownerAllowAll: boolean; + ownerCandidatesForCommands: string[]; + explicitOwners: string[]; + ownerList: string[]; +}; + function resolveProviderFromContext( ctx: MsgContext, cfg: OpenClawConfig, @@ -88,18 +102,12 @@ function probeInferredProviders(ctx: MsgContext, cfg: OpenClawConfig): InferredP let droppedResolutionError = false; const candidates = listChannelPlugins() .map((plugin) => { - const resolvedAllowFrom = resolveProviderAllowFrom({ + const resolvedAllowFrom = buildProviderAllowFromResolution({ plugin, cfg, accountId: ctx.AccountId, }); - const allowFrom = formatAllowFromList({ - plugin, - cfg, - accountId: ctx.AccountId, - allowFrom: resolvedAllowFrom.allowFrom, - }); - if (allowFrom.length === 0) { + if (resolvedAllowFrom.allowFromList.length === 0) { if (resolvedAllowFrom.hadResolutionError) { droppedResolutionError = true; } @@ -148,6 +156,18 @@ function normalizeAllowFromEntry(params: { return normalized.filter((entry) => entry.trim().length > 0); } +function isWildcardAllowFromEntry(entry: string): boolean { + return entry.trim() === "*"; +} + +function hasWildcardAllowFrom(list: string[]): boolean { + return list.some((entry) => isWildcardAllowFromEntry(entry)); +} + +function stripWildcardAllowFrom(list: string[]): string[] { + return list.filter((entry) => !isWildcardAllowFromEntry(entry)); +} + function resolveProviderAllowFrom(params: { plugin?: ChannelPlugin; cfg: OpenClawConfig; @@ -197,6 +217,39 @@ function resolveProviderAllowFrom(params: { } } +function buildProviderAllowFromResolution(params: { + plugin?: ChannelPlugin; + cfg: OpenClawConfig; + accountId?: string | null; + providerId?: ChannelId; + forceFallbackResolutionError?: boolean; +}): ProviderAllowFromResolution { + const providerId = params.providerId ?? params.plugin?.id; + const resolvedAllowFrom = params.forceFallbackResolutionError + ? { + allowFrom: resolveFallbackAllowFrom({ + cfg: params.cfg, + providerId, + accountId: params.accountId, + }), + hadResolutionError: true, + } + : resolveProviderAllowFrom({ + plugin: params.plugin, + cfg: params.cfg, + accountId: params.accountId, + }); + return { + ...resolvedAllowFrom, + allowFromList: formatAllowFromList({ + plugin: params.plugin, + cfg: params.cfg, + accountId: params.accountId, + allowFrom: resolvedAllowFrom.allowFrom, + }), + }; +} + function describeAllowFromResolutionError(err: unknown): string { if (err instanceof Error) { const name = err.name.trim(); @@ -282,6 +335,115 @@ function resolveCommandsAllowFromList(params: { }); } +function resolveOwnerCandidatesForCommands(params: { + plugin?: ChannelPlugin; + cfg: OpenClawConfig; + accountId?: string | null; + to?: string; + allowAll: boolean; + allowFromList: string[]; +}): string[] { + if (params.allowAll) { + return []; + } + const ownerCandidatesForCommands = stripWildcardAllowFrom(params.allowFromList); + if (ownerCandidatesForCommands.length > 0 || !params.to) { + return ownerCandidatesForCommands; + } + const normalizedTo = normalizeAllowFromEntry({ + plugin: params.plugin, + cfg: params.cfg, + accountId: params.accountId, + value: params.to, + }); + return normalizedTo.length > 0 ? [...ownerCandidatesForCommands, ...normalizedTo] : []; +} + +function resolveOwnerAuthorizationState(params: { + plugin?: ChannelPlugin; + cfg: OpenClawConfig; + accountId?: string | null; + providerId?: ChannelId; + to?: string; + allowFromList: string[]; + hadResolutionError: boolean; + configOwnerAllowFrom?: Array; + contextOwnerAllowFrom?: Array; +}): OwnerAuthorizationState { + const configOwnerAllowFromList = resolveOwnerAllowFromList({ + plugin: params.plugin, + cfg: params.cfg, + accountId: params.accountId, + providerId: params.providerId, + allowFrom: params.configOwnerAllowFrom, + }); + const contextOwnerAllowFromList = resolveOwnerAllowFromList({ + plugin: params.plugin, + cfg: params.cfg, + accountId: params.accountId, + providerId: params.providerId, + allowFrom: params.contextOwnerAllowFrom, + }); + const allowAll = + !params.hadResolutionError && + (params.allowFromList.length === 0 || hasWildcardAllowFrom(params.allowFromList)); + const ownerCandidatesForCommands = resolveOwnerCandidatesForCommands({ + plugin: params.plugin, + cfg: params.cfg, + accountId: params.accountId, + to: params.to, + allowAll, + allowFromList: params.allowFromList, + }); + const ownerAllowAll = hasWildcardAllowFrom(configOwnerAllowFromList); + const explicitOwners = stripWildcardAllowFrom(configOwnerAllowFromList); + const explicitOverrides = stripWildcardAllowFrom(contextOwnerAllowFromList); + const ownerList = Array.from( + new Set( + explicitOwners.length > 0 + ? explicitOwners + : ownerAllowAll + ? [] + : explicitOverrides.length > 0 + ? explicitOverrides + : ownerCandidatesForCommands, + ), + ); + return { + allowAll, + ownerAllowAll, + ownerCandidatesForCommands, + explicitOwners, + ownerList, + }; +} + +function resolveCommandSenderAuthorization(params: { + commandAuthorized: boolean; + isOwnerForCommands: boolean; + senderCandidates: string[]; + commandsAllowFromList: string[] | null; + providerResolutionError: boolean; + commandsAllowFromConfigured: boolean; +}): boolean { + if ( + params.commandsAllowFromList !== null || + (params.providerResolutionError && params.commandsAllowFromConfigured) + ) { + const commandsAllowFromList = params.commandsAllowFromList; + const commandsAllowAll = + !params.providerResolutionError && + Boolean(commandsAllowFromList && hasWildcardAllowFrom(commandsAllowFromList)); + const matchedCommandsAllowFrom = commandsAllowFromList?.length + ? params.senderCandidates.find((candidate) => commandsAllowFromList.includes(candidate)) + : undefined; + return ( + !params.providerResolutionError && (commandsAllowAll || Boolean(matchedCommandsAllowFrom)) + ); + } + return params.commandAuthorized && params.isOwnerForCommands; +} + function isConversationLikeIdentity(value: string): boolean { const normalized = value.trim().toLowerCase(); if (!normalized) { @@ -477,70 +639,24 @@ export function resolveCommandAuthorization(params: { providerId, }); - const resolvedAllowFrom = providerResolutionError - ? { - allowFrom: resolveFallbackAllowFrom({ - cfg, - providerId, - accountId: ctx.AccountId, - }), - hadResolutionError: true, - } - : resolveProviderAllowFrom({ - plugin, - cfg, - accountId: ctx.AccountId, - }); - const allowFromList = formatAllowFromList({ - plugin, - cfg, - accountId: ctx.AccountId, - allowFrom: resolvedAllowFrom.allowFrom, - }); - const configOwnerAllowFromList = resolveOwnerAllowFromList({ + const resolvedAllowFrom = buildProviderAllowFromResolution({ plugin, cfg, accountId: ctx.AccountId, providerId, - allowFrom: cfg.commands?.ownerAllowFrom, + forceFallbackResolutionError: providerResolutionError, }); - const contextOwnerAllowFromList = resolveOwnerAllowFromList({ + const ownerState = resolveOwnerAuthorizationState({ plugin, cfg, accountId: ctx.AccountId, providerId, - allowFrom: ctx.OwnerAllowFrom, + to, + allowFromList: resolvedAllowFrom.allowFromList, + hadResolutionError: resolvedAllowFrom.hadResolutionError, + configOwnerAllowFrom: cfg.commands?.ownerAllowFrom, + contextOwnerAllowFrom: ctx.OwnerAllowFrom, }); - const allowAll = - !resolvedAllowFrom.hadResolutionError && - (allowFromList.length === 0 || allowFromList.some((entry) => entry.trim() === "*")); - - const ownerCandidatesForCommands = allowAll ? [] : allowFromList.filter((entry) => entry !== "*"); - if (!allowAll && ownerCandidatesForCommands.length === 0 && to) { - const normalizedTo = normalizeAllowFromEntry({ - plugin, - cfg, - accountId: ctx.AccountId, - value: to, - }); - if (normalizedTo.length > 0) { - ownerCandidatesForCommands.push(...normalizedTo); - } - } - const ownerAllowAll = configOwnerAllowFromList.some((entry) => entry.trim() === "*"); - const explicitOwners = configOwnerAllowFromList.filter((entry) => entry !== "*"); - const explicitOverrides = contextOwnerAllowFromList.filter((entry) => entry !== "*"); - const ownerList = Array.from( - new Set( - explicitOwners.length > 0 - ? explicitOwners - : ownerAllowAll - ? [] - : explicitOverrides.length > 0 - ? explicitOverrides - : ownerCandidatesForCommands, - ), - ); const senderCandidates = resolveSenderCandidates({ plugin, @@ -552,11 +668,13 @@ export function resolveCommandAuthorization(params: { from, chatType: ctx.ChatType, }); - const matchedSender = ownerList.length - ? senderCandidates.find((candidate) => ownerList.includes(candidate)) + const matchedSender = ownerState.ownerList.length + ? senderCandidates.find((candidate) => ownerState.ownerList.includes(candidate)) : undefined; - const matchedCommandOwner = ownerCandidatesForCommands.length - ? senderCandidates.find((candidate) => ownerCandidatesForCommands.includes(candidate)) + const matchedCommandOwner = ownerState.ownerCandidatesForCommands.length + ? senderCandidates.find((candidate) => + ownerState.ownerCandidatesForCommands.includes(candidate), + ) : undefined; const senderId = matchedSender ?? senderCandidates[0]; @@ -566,40 +684,32 @@ export function resolveCommandAuthorization(params: { isInternalMessageChannel(ctx.Provider) && Array.isArray(ctx.GatewayClientScopes) && ctx.GatewayClientScopes.includes("operator.admin"); - const ownerAllowlistConfigured = ownerAllowAll || explicitOwners.length > 0; + const ownerAllowlistConfigured = ownerState.ownerAllowAll || ownerState.explicitOwners.length > 0; const senderIsOwner = ctx.ForceSenderIsOwnerFalse ? false - : senderIsOwnerByIdentity || senderIsOwnerByScope || ownerAllowAll; + : senderIsOwnerByIdentity || senderIsOwnerByScope || ownerState.ownerAllowAll; const requireOwner = enforceOwner || ownerAllowlistConfigured; const isOwnerForCommands = !requireOwner ? true - : ownerAllowAll + : ownerState.ownerAllowAll ? true : ownerAllowlistConfigured ? senderIsOwner - : allowAll || ownerCandidatesForCommands.length === 0 || Boolean(matchedCommandOwner); - - // If commands.allowFrom is configured, use it for command authorization - // Otherwise, fall back to existing behavior (channel allowFrom + owner checks) - let isAuthorizedSender: boolean; - if (commandsAllowFromList !== null || (providerResolutionError && commandsAllowFromConfigured)) { - // commands.allowFrom is configured - use it for authorization - const commandsAllowAll = - !providerResolutionError && - Boolean(commandsAllowFromList?.some((entry) => entry.trim() === "*")); - const matchedCommandsAllowFrom = commandsAllowFromList?.length - ? senderCandidates.find((candidate) => commandsAllowFromList.includes(candidate)) - : undefined; - isAuthorizedSender = - !providerResolutionError && (commandsAllowAll || Boolean(matchedCommandsAllowFrom)); - } else { - // Fall back to existing behavior - isAuthorizedSender = commandAuthorized && isOwnerForCommands; - } + : ownerState.allowAll || + ownerState.ownerCandidatesForCommands.length === 0 || + Boolean(matchedCommandOwner); + const isAuthorizedSender = resolveCommandSenderAuthorization({ + commandAuthorized, + isOwnerForCommands, + senderCandidates, + commandsAllowFromList, + providerResolutionError, + commandsAllowFromConfigured, + }); return { providerId, - ownerList, + ownerList: ownerState.ownerList, senderId: senderId || undefined, senderIsOwner, isAuthorizedSender, diff --git a/src/auto-reply/command-control.test.ts b/src/auto-reply/command-control.test.ts index 6212a295086..77a3eef849f 100644 --- a/src/auto-reply/command-control.test.ts +++ b/src/auto-reply/command-control.test.ts @@ -159,6 +159,39 @@ describe("resolveCommandAuthorization", () => { expect(otherAuth.isAuthorizedSender).toBe(false); }); + it("uses explicit owner allowlist when allowFrom is empty", () => { + const cfg = { + commands: { ownerAllowFrom: ["whatsapp:+15551234567"] }, + channels: { whatsapp: {} }, + } as OpenClawConfig; + + const ownerAuth = resolveCommandAuthorization({ + ctx: { + Provider: "whatsapp", + Surface: "whatsapp", + From: "whatsapp:+15551234567", + SenderE164: "+15551234567", + } as MsgContext, + cfg, + commandAuthorized: true, + }); + expect(ownerAuth.senderIsOwner).toBe(true); + expect(ownerAuth.isAuthorizedSender).toBe(true); + + const otherAuth = resolveCommandAuthorization({ + ctx: { + Provider: "whatsapp", + Surface: "whatsapp", + From: "whatsapp:+19995551234", + SenderE164: "+19995551234", + } as MsgContext, + cfg, + commandAuthorized: true, + }); + expect(otherAuth.senderIsOwner).toBe(false); + expect(otherAuth.isAuthorizedSender).toBe(false); + }); + it("uses owner allowlist override from context when configured", () => { setActivePluginRegistry( createTestRegistry([