refactor: dedupe auth and session helpers

2026-04-16 11:41:08 +00:00 · 2026-04-06 16:51:09 +01:00
parent e8c7481fd2
commit ad9481e2d1
3 changed files with 351 additions and 169 deletions
--- 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<SessionReferenceResolution, { ok: true }> {
+  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<string> {
+  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<SessionReferenceResolution> {
  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<Extract<SessionReferenceResolution, { ok: true }> | 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<SessionReferenceResolution | null> {
+  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<SessionReferenceResolution> {
  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({
--- 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<string | number>;
+  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<string | number>;
+  contextOwnerAllowFrom?: Array<string | number>;
+}): 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,
--- 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([