refactor: normalize runtime group sender gating decisions

2026-03-12 07:20:45 +00:00 · 2026-03-07 21:50:19 +00:00
parent 5eba663c38
commit 27dad962fe
8 changed files with 190 additions and 109 deletions
--- a/extensions/mattermost/src/mattermost/monitor-auth.ts
+++ b/extensions/mattermost/src/mattermost/monitor-auth.ts
@@ -1,5 +1,6 @@
 import type { OpenClawConfig } from "openclaw/plugin-sdk/mattermost";
 import {
+  evaluateSenderGroupAccessForPolicy,
  isDangerousNameMatchingEnabled,
  resolveAllowlistMatchSimple,
  resolveControlCommandGate,
@@ -231,7 +232,20 @@ export function authorizeMattermostCommandInvocation(params: {
      };
    }
  } else {
-    if (groupPolicy === "disabled") {
+    const senderGroupAccess = evaluateSenderGroupAccessForPolicy({
+      groupPolicy,
+      groupAllowFrom: effectiveGroupAllowFrom,
+      senderId,
+      isSenderAllowed: (_senderId, allowFrom) =>
+        isMattermostSenderAllowed({
+          senderId,
+          senderName,
+          allowFrom,
+          allowNameMatching,
+        }),
+    });
+
+    if (!senderGroupAccess.allowed && senderGroupAccess.reason === "disabled") {
      return {
        ok: false,
        denyReason: "channels-disabled",
@@ -245,33 +259,32 @@ export function authorizeMattermostCommandInvocation(params: {
      };
    }

-    if (groupPolicy === "allowlist") {
-      if (effectiveGroupAllowFrom.length === 0) {
-        return {
-          ok: false,
-          denyReason: "channel-no-allowlist",
-          commandAuthorized: false,
-          channelInfo,
-          kind,
-          chatType,
-          channelName,
-          channelDisplay,
-          roomLabel,
-        };
-      }
-      if (!groupAllowedForCommands) {
-        return {
-          ok: false,
-          denyReason: "unauthorized",
-          commandAuthorized: false,
-          channelInfo,
-          kind,
-          chatType,
-          channelName,
-          channelDisplay,
-          roomLabel,
-        };
-      }
+    if (!senderGroupAccess.allowed && senderGroupAccess.reason === "empty_allowlist") {
+      return {
+        ok: false,
+        denyReason: "channel-no-allowlist",
+        commandAuthorized: false,
+        channelInfo,
+        kind,
+        chatType,
+        channelName,
+        channelDisplay,
+        roomLabel,
+      };
+    }
+
+    if (!senderGroupAccess.allowed && senderGroupAccess.reason === "sender_not_allowlisted") {
+      return {
+        ok: false,
+        denyReason: "unauthorized",
+        commandAuthorized: false,
+        channelInfo,
+        kind,
+        chatType,
+        channelName,
+        channelDisplay,
+        roomLabel,
+      };
    }

    if (commandGate.shouldBlock) {
--- a/extensions/msteams/src/monitor-handler/message-handler.ts
+++ b/extensions/msteams/src/monitor-handler/message-handler.ts
@@ -6,6 +6,7 @@ import {
  DEFAULT_GROUP_HISTORY_LIMIT,
  createScopedPairingAccess,
  logInboundDrop,
+  evaluateSenderGroupAccessForPolicy,
  recordPendingHistoryEntryIfEnabled,
  resolveControlCommandGate,
  resolveDefaultGroupPolicy,
@@ -230,46 +231,57 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) {
    }

    if (!isDirectMessage && msteamsCfg) {
-      if (groupPolicy === "disabled") {
+      if (channelGate.allowlistConfigured && !channelGate.allowed) {
+        log.debug?.("dropping group message (not in team/channel allowlist)", {
+          conversationId,
+          teamKey: channelGate.teamKey ?? "none",
+          channelKey: channelGate.channelKey ?? "none",
+          channelMatchKey: channelGate.channelMatchKey ?? "none",
+          channelMatchSource: channelGate.channelMatchSource ?? "none",
+        });
+        return;
+      }
+      const senderGroupAccess = evaluateSenderGroupAccessForPolicy({
+        groupPolicy,
+        groupAllowFrom:
+          effectiveGroupAllowFrom.length > 0 || !channelGate.allowlistConfigured
+            ? effectiveGroupAllowFrom
+            : ["*"],
+        senderId,
+        isSenderAllowed: (_senderId, allowFrom) =>
+          resolveMSTeamsAllowlistMatch({
+            allowFrom,
+            senderId,
+            senderName,
+            allowNameMatching: isDangerousNameMatchingEnabled(msteamsCfg),
+          }).allowed,
+      });
+
+      if (!senderGroupAccess.allowed && senderGroupAccess.reason === "disabled") {
        log.debug?.("dropping group message (groupPolicy: disabled)", {
          conversationId,
        });
        return;
      }
-
-      if (groupPolicy === "allowlist") {
-        if (channelGate.allowlistConfigured && !channelGate.allowed) {
-          log.debug?.("dropping group message (not in team/channel allowlist)", {
-            conversationId,
-            teamKey: channelGate.teamKey ?? "none",
-            channelKey: channelGate.channelKey ?? "none",
-            channelMatchKey: channelGate.channelMatchKey ?? "none",
-            channelMatchSource: channelGate.channelMatchSource ?? "none",
-          });
-          return;
-        }
-        if (effectiveGroupAllowFrom.length === 0 && !channelGate.allowlistConfigured) {
-          log.debug?.("dropping group message (groupPolicy: allowlist, no allowlist)", {
-            conversationId,
-          });
-          return;
-        }
-        if (effectiveGroupAllowFrom.length > 0 && access.decision !== "allow") {
-          const allowMatch = resolveMSTeamsAllowlistMatch({
-            allowFrom: effectiveGroupAllowFrom,
-            senderId,
-            senderName,
-            allowNameMatching: isDangerousNameMatchingEnabled(msteamsCfg),
-          });
-          if (!allowMatch.allowed) {
-            log.debug?.("dropping group message (not in groupAllowFrom)", {
-              sender: senderId,
-              label: senderName,
-              allowlistMatch: formatAllowlistMatchMeta(allowMatch),
-            });
-            return;
-          }
-        }
+      if (!senderGroupAccess.allowed && senderGroupAccess.reason === "empty_allowlist") {
+        log.debug?.("dropping group message (groupPolicy: allowlist, no allowlist)", {
+          conversationId,
+        });
+        return;
+      }
+      if (!senderGroupAccess.allowed && senderGroupAccess.reason === "sender_not_allowlisted") {
+        const allowMatch = resolveMSTeamsAllowlistMatch({
+          allowFrom: effectiveGroupAllowFrom,
+          senderId,
+          senderName,
+          allowNameMatching: isDangerousNameMatchingEnabled(msteamsCfg),
+        });
+        log.debug?.("dropping group message (not in groupAllowFrom)", {
+          sender: senderId,
+          label: senderName,
+          allowlistMatch: formatAllowlistMatchMeta(allowMatch),
+        });
+        return;
      }
    }

--- a/src/line/bot-handlers.ts
+++ b/src/line/bot-handlers.ts
@@ -30,6 +30,7 @@ import {
  readChannelAllowFromStore,
  upsertChannelPairingRequest,
 } from "../pairing/pairing-store.js";
+import { evaluateSenderGroupAccessForPolicy } from "../plugin-sdk/group-access.js";
 import { resolveAgentRoute } from "../routing/resolve-route.js";
 import type { RuntimeEnv } from "../runtime.js";
 import {
@@ -344,23 +345,31 @@ async function shouldProcessLineEvent(
        return denied;
      }
    }
-    if (groupPolicy === "disabled") {
+    if (groupPolicy === "allowlist" && !senderId) {
+      logVerbose("Blocked line group message (no sender ID, groupPolicy: allowlist)");
+      return denied;
+    }
+    const senderGroupAccess = evaluateSenderGroupAccessForPolicy({
+      groupPolicy,
+      groupAllowFrom: effectiveGroupAllow.entries,
+      senderId,
+      isSenderAllowed: (candidateSenderId, allowFrom) =>
+        isSenderAllowed({
+          allow: normalizeAllowFrom(allowFrom),
+          senderId: candidateSenderId,
+        }),
+    });
+    if (!senderGroupAccess.allowed && senderGroupAccess.reason === "disabled") {
      logVerbose("Blocked line group message (groupPolicy: disabled)");
      return denied;
    }
-    if (groupPolicy === "allowlist") {
-      if (!senderId) {
-        logVerbose("Blocked line group message (no sender ID, groupPolicy: allowlist)");
-        return denied;
-      }
-      if (!effectiveGroupAllow.hasEntries) {
-        logVerbose("Blocked line group message (groupPolicy: allowlist, no groupAllowFrom)");
-        return denied;
-      }
-      if (!isSenderAllowed({ allow: effectiveGroupAllow, senderId })) {
-        logVerbose(`Blocked line group message from ${senderId} (groupPolicy: allowlist)`);
-        return denied;
-      }
+    if (!senderGroupAccess.allowed && senderGroupAccess.reason === "empty_allowlist") {
+      logVerbose("Blocked line group message (groupPolicy: allowlist, no groupAllowFrom)");
+      return denied;
+    }
+    if (!senderGroupAccess.allowed && senderGroupAccess.reason === "sender_not_allowlisted") {
+      logVerbose(`Blocked line group message from ${senderId} (groupPolicy: allowlist)`);
+      return denied;
    }
    return {
      allowed: true,
--- a/src/plugin-sdk/group-access.test.ts
+++ b/src/plugin-sdk/group-access.test.ts
@@ -1,5 +1,33 @@
 import { describe, expect, it } from "vitest";
-import { evaluateSenderGroupAccess } from "./group-access.js";
+import { evaluateSenderGroupAccess, evaluateSenderGroupAccessForPolicy } from "./group-access.js";
+
+describe("evaluateSenderGroupAccessForPolicy", () => {
+  it("blocks disabled policy", () => {
+    const decision = evaluateSenderGroupAccessForPolicy({
+      groupPolicy: "disabled",
+      groupAllowFrom: ["123"],
+      senderId: "123",
+      isSenderAllowed: () => true,
+    });
+
+    expect(decision).toMatchObject({ allowed: false, reason: "disabled", groupPolicy: "disabled" });
+  });
+
+  it("blocks allowlist with empty list", () => {
+    const decision = evaluateSenderGroupAccessForPolicy({
+      groupPolicy: "allowlist",
+      groupAllowFrom: [],
+      senderId: "123",
+      isSenderAllowed: () => true,
+    });
+
+    expect(decision).toMatchObject({
+      allowed: false,
+      reason: "empty_allowlist",
+      groupPolicy: "allowlist",
+    });
+  });
+});

 describe("evaluateSenderGroupAccess", () => {
  it("defaults missing provider config to allowlist", () => {
--- a/src/plugin-sdk/group-access.ts
+++ b/src/plugin-sdk/group-access.ts
@@ -14,6 +14,48 @@ export type SenderGroupAccessDecision = {
  reason: SenderGroupAccessReason;
 };

+export function evaluateSenderGroupAccessForPolicy(params: {
+  groupPolicy: GroupPolicy;
+  providerMissingFallbackApplied?: boolean;
+  groupAllowFrom: string[];
+  senderId: string;
+  isSenderAllowed: (senderId: string, allowFrom: string[]) => boolean;
+}): SenderGroupAccessDecision {
+  if (params.groupPolicy === "disabled") {
+    return {
+      allowed: false,
+      groupPolicy: params.groupPolicy,
+      providerMissingFallbackApplied: Boolean(params.providerMissingFallbackApplied),
+      reason: "disabled",
+    };
+  }
+  if (params.groupPolicy === "allowlist") {
+    if (params.groupAllowFrom.length === 0) {
+      return {
+        allowed: false,
+        groupPolicy: params.groupPolicy,
+        providerMissingFallbackApplied: Boolean(params.providerMissingFallbackApplied),
+        reason: "empty_allowlist",
+      };
+    }
+    if (!params.isSenderAllowed(params.senderId, params.groupAllowFrom)) {
+      return {
+        allowed: false,
+        groupPolicy: params.groupPolicy,
+        providerMissingFallbackApplied: Boolean(params.providerMissingFallbackApplied),
+        reason: "sender_not_allowlisted",
+      };
+    }
+  }
+
+  return {
+    allowed: true,
+    groupPolicy: params.groupPolicy,
+    providerMissingFallbackApplied: Boolean(params.providerMissingFallbackApplied),
+    reason: "allowed",
+  };
+}
+
 export function evaluateSenderGroupAccess(params: {
  providerConfigPresent: boolean;
  configuredGroupPolicy?: GroupPolicy;
@@ -28,37 +70,11 @@ export function evaluateSenderGroupAccess(params: {
    defaultGroupPolicy: params.defaultGroupPolicy,
  });

-  if (groupPolicy === "disabled") {
-    return {
-      allowed: false,
-      groupPolicy,
-      providerMissingFallbackApplied,
-      reason: "disabled",
-    };
-  }
-  if (groupPolicy === "allowlist") {
-    if (params.groupAllowFrom.length === 0) {
-      return {
-        allowed: false,
-        groupPolicy,
-        providerMissingFallbackApplied,
-        reason: "empty_allowlist",
-      };
-    }
-    if (!params.isSenderAllowed(params.senderId, params.groupAllowFrom)) {
-      return {
-        allowed: false,
-        groupPolicy,
-        providerMissingFallbackApplied,
-        reason: "sender_not_allowlisted",
-      };
-    }
-  }
-
-  return {
-    allowed: true,
+  return evaluateSenderGroupAccessForPolicy({
    groupPolicy,
    providerMissingFallbackApplied,
-    reason: "allowed",
-  };
+    groupAllowFrom: params.groupAllowFrom,
+    senderId: params.senderId,
+    isSenderAllowed: params.isSenderAllowed,
+  });
 }
--- a/src/plugin-sdk/index.ts
+++ b/src/plugin-sdk/index.ts
@@ -277,6 +277,7 @@ export {
 } from "./allow-from.js";
 export {
  evaluateSenderGroupAccess,
+  evaluateSenderGroupAccessForPolicy,
  type SenderGroupAccessDecision,
  type SenderGroupAccessReason,
 } from "./group-access.js";
--- a/src/plugin-sdk/mattermost.ts
+++ b/src/plugin-sdk/mattermost.ts
@@ -95,6 +95,7 @@ export {
  resolveDmGroupAccessWithLists,
  resolveEffectiveAllowFromLists,
 } from "../security/dm-policy-shared.js";
+export { evaluateSenderGroupAccessForPolicy } from "./group-access.js";
 export type { WizardPrompter } from "../wizard/prompts.js";
 export { buildAgentMediaPayload } from "./agent-media-payload.js";
 export { loadOutboundMediaFromUrl } from "./outbound-media.js";
--- a/src/plugin-sdk/msteams.ts
+++ b/src/plugin-sdk/msteams.ts
@@ -91,6 +91,7 @@ export {
  resolveDmGroupAccessWithLists,
  resolveEffectiveAllowFromLists,
 } from "../security/dm-policy-shared.js";
+export { evaluateSenderGroupAccessForPolicy } from "./group-access.js";
 export { formatDocsLink } from "../terminal/links.js";
 export { sleep } from "../utils.js";
 export { loadWebMedia } from "../web/media.js";