From b0d924676838e6414beebe1b8eccff0eb0278108 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 7 Mar 2026 23:39:04 +0000 Subject: [PATCH] refactor: share matched group policy evaluation --- extensions/nextcloud-talk/src/policy.test.ts | 107 ++++++++++++++++++- extensions/nextcloud-talk/src/policy.ts | 42 +++++--- src/plugin-sdk/group-access.test.ts | 59 ++++++++++ src/plugin-sdk/group-access.ts | 49 +++++++++ src/plugin-sdk/index.ts | 3 + src/plugin-sdk/nextcloud-talk.ts | 1 + src/security/dm-policy-shared.test.ts | 32 ++++++ src/security/dm-policy-shared.ts | 28 +++-- 8 files changed, 293 insertions(+), 28 deletions(-) diff --git a/extensions/nextcloud-talk/src/policy.test.ts b/extensions/nextcloud-talk/src/policy.test.ts index 6faea0afb72..383a627fc31 100644 --- a/extensions/nextcloud-talk/src/policy.test.ts +++ b/extensions/nextcloud-talk/src/policy.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { resolveNextcloudTalkAllowlistMatch } from "./policy.js"; +import { resolveNextcloudTalkAllowlistMatch, resolveNextcloudTalkGroupAllow } from "./policy.js"; describe("nextcloud-talk policy", () => { describe("resolveNextcloudTalkAllowlistMatch", () => { @@ -30,4 +30,109 @@ describe("nextcloud-talk policy", () => { ).toBe(false); }); }); + + describe("resolveNextcloudTalkGroupAllow", () => { + it("blocks disabled policy", () => { + expect( + resolveNextcloudTalkGroupAllow({ + groupPolicy: "disabled", + outerAllowFrom: ["owner"], + innerAllowFrom: ["room-user"], + senderId: "owner", + }), + ).toEqual({ + allowed: false, + outerMatch: { allowed: false }, + innerMatch: { allowed: false }, + }); + }); + + it("allows open policy", () => { + expect( + resolveNextcloudTalkGroupAllow({ + groupPolicy: "open", + outerAllowFrom: [], + innerAllowFrom: [], + senderId: "owner", + }), + ).toEqual({ + allowed: true, + outerMatch: { allowed: true }, + innerMatch: { allowed: true }, + }); + }); + + it("blocks allowlist mode when both outer and inner allowlists are empty", () => { + expect( + resolveNextcloudTalkGroupAllow({ + groupPolicy: "allowlist", + outerAllowFrom: [], + innerAllowFrom: [], + senderId: "owner", + }), + ).toEqual({ + allowed: false, + outerMatch: { allowed: false }, + innerMatch: { allowed: false }, + }); + }); + + it("requires inner match when only room-specific allowlist is configured", () => { + expect( + resolveNextcloudTalkGroupAllow({ + groupPolicy: "allowlist", + outerAllowFrom: [], + innerAllowFrom: ["room-user"], + senderId: "room-user", + }), + ).toEqual({ + allowed: true, + outerMatch: { allowed: false }, + innerMatch: { allowed: true, matchKey: "room-user", matchSource: "id" }, + }); + }); + + it("blocks when outer allowlist misses even if inner allowlist matches", () => { + expect( + resolveNextcloudTalkGroupAllow({ + groupPolicy: "allowlist", + outerAllowFrom: ["team-owner"], + innerAllowFrom: ["room-user"], + senderId: "room-user", + }), + ).toEqual({ + allowed: false, + outerMatch: { allowed: false }, + innerMatch: { allowed: true, matchKey: "room-user", matchSource: "id" }, + }); + }); + + it("allows when both outer and inner allowlists match", () => { + expect( + resolveNextcloudTalkGroupAllow({ + groupPolicy: "allowlist", + outerAllowFrom: ["team-owner"], + innerAllowFrom: ["room-user"], + senderId: "team-owner", + }), + ).toEqual({ + allowed: false, + outerMatch: { allowed: true, matchKey: "team-owner", matchSource: "id" }, + innerMatch: { allowed: false }, + }); + + expect( + resolveNextcloudTalkGroupAllow({ + groupPolicy: "allowlist", + outerAllowFrom: ["shared-user"], + innerAllowFrom: ["shared-user"], + senderId: "shared-user", + }), + ).toEqual({ + allowed: true, + outerMatch: { allowed: true, matchKey: "shared-user", matchSource: "id" }, + innerMatch: { allowed: true, matchKey: "shared-user", matchSource: "id" }, + }); + }); + }); }); diff --git a/extensions/nextcloud-talk/src/policy.ts b/extensions/nextcloud-talk/src/policy.ts index 329aaeb3d40..1157384b578 100644 --- a/extensions/nextcloud-talk/src/policy.ts +++ b/extensions/nextcloud-talk/src/policy.ts @@ -6,6 +6,7 @@ import type { } from "openclaw/plugin-sdk/nextcloud-talk"; import { buildChannelKeyCandidates, + evaluateMatchedGroupAccessForPolicy, normalizeChannelSlug, resolveChannelEntryMatchWithFallback, resolveMentionGatingWithBypass, @@ -128,19 +129,8 @@ export function resolveNextcloudTalkGroupAllow(params: { innerAllowFrom: Array | undefined; senderId: string; }): { allowed: boolean; outerMatch: AllowlistMatch; innerMatch: AllowlistMatch } { - if (params.groupPolicy === "disabled") { - return { allowed: false, outerMatch: { allowed: false }, innerMatch: { allowed: false } }; - } - if (params.groupPolicy === "open") { - return { allowed: true, outerMatch: { allowed: true }, innerMatch: { allowed: true } }; - } - const outerAllow = normalizeNextcloudTalkAllowlist(params.outerAllowFrom); const innerAllow = normalizeNextcloudTalkAllowlist(params.innerAllowFrom); - if (outerAllow.length === 0 && innerAllow.length === 0) { - return { allowed: false, outerMatch: { allowed: false }, innerMatch: { allowed: false } }; - } - const outerMatch = resolveNextcloudTalkAllowlistMatch({ allowFrom: params.outerAllowFrom, senderId: params.senderId, @@ -149,14 +139,32 @@ export function resolveNextcloudTalkGroupAllow(params: { allowFrom: params.innerAllowFrom, senderId: params.senderId, }); - const allowed = resolveNestedAllowlistDecision({ - outerConfigured: outerAllow.length > 0 || innerAllow.length > 0, - outerMatched: outerAllow.length > 0 ? outerMatch.allowed : true, - innerConfigured: innerAllow.length > 0, - innerMatched: innerMatch.allowed, + const access = evaluateMatchedGroupAccessForPolicy({ + groupPolicy: params.groupPolicy, + allowlistConfigured: outerAllow.length > 0 || innerAllow.length > 0, + allowlistMatched: resolveNestedAllowlistDecision({ + outerConfigured: outerAllow.length > 0 || innerAllow.length > 0, + outerMatched: outerAllow.length > 0 ? outerMatch.allowed : true, + innerConfigured: innerAllow.length > 0, + innerMatched: innerMatch.allowed, + }), }); - return { allowed, outerMatch, innerMatch }; + return { + allowed: access.allowed, + outerMatch: + params.groupPolicy === "open" + ? { allowed: true } + : params.groupPolicy === "disabled" + ? { allowed: false } + : outerMatch, + innerMatch: + params.groupPolicy === "open" + ? { allowed: true } + : params.groupPolicy === "disabled" + ? { allowed: false } + : innerMatch, + }; } export function resolveNextcloudTalkMentionGate(params: { diff --git a/src/plugin-sdk/group-access.test.ts b/src/plugin-sdk/group-access.test.ts index 9a834464ba0..b44c71c447e 100644 --- a/src/plugin-sdk/group-access.test.ts +++ b/src/plugin-sdk/group-access.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { evaluateGroupRouteAccessForPolicy, + evaluateMatchedGroupAccessForPolicy, evaluateSenderGroupAccess, evaluateSenderGroupAccessForPolicy, resolveSenderScopedGroupPolicy, @@ -120,6 +121,64 @@ describe("evaluateGroupRouteAccessForPolicy", () => { }); }); +describe("evaluateMatchedGroupAccessForPolicy", () => { + it("blocks disabled policy", () => { + expect( + evaluateMatchedGroupAccessForPolicy({ + groupPolicy: "disabled", + allowlistConfigured: true, + allowlistMatched: true, + }), + ).toEqual({ + allowed: false, + groupPolicy: "disabled", + reason: "disabled", + }); + }); + + it("blocks allowlist without configured entries", () => { + expect( + evaluateMatchedGroupAccessForPolicy({ + groupPolicy: "allowlist", + allowlistConfigured: false, + allowlistMatched: false, + }), + ).toEqual({ + allowed: false, + groupPolicy: "allowlist", + reason: "empty_allowlist", + }); + }); + + it("blocks unmatched allowlist sender", () => { + expect( + evaluateMatchedGroupAccessForPolicy({ + groupPolicy: "allowlist", + allowlistConfigured: true, + allowlistMatched: false, + }), + ).toEqual({ + allowed: false, + groupPolicy: "allowlist", + reason: "not_allowlisted", + }); + }); + + it("allows open policy", () => { + expect( + evaluateMatchedGroupAccessForPolicy({ + groupPolicy: "open", + allowlistConfigured: false, + allowlistMatched: false, + }), + ).toEqual({ + allowed: true, + groupPolicy: "open", + reason: "allowed", + }); + }); +}); + describe("evaluateSenderGroupAccess", () => { it("defaults missing provider config to allowlist", () => { const decision = evaluateSenderGroupAccess({ diff --git a/src/plugin-sdk/group-access.ts b/src/plugin-sdk/group-access.ts index e8bc817fadb..ce80b04b1d7 100644 --- a/src/plugin-sdk/group-access.ts +++ b/src/plugin-sdk/group-access.ts @@ -27,6 +27,18 @@ export type GroupRouteAccessDecision = { reason: GroupRouteAccessReason; }; +export type MatchedGroupAccessReason = + | "allowed" + | "disabled" + | "empty_allowlist" + | "not_allowlisted"; + +export type MatchedGroupAccessDecision = { + allowed: boolean; + groupPolicy: GroupPolicy; + reason: MatchedGroupAccessReason; +}; + export function resolveSenderScopedGroupPolicy(params: { groupPolicy: GroupPolicy; groupAllowFrom: string[]; @@ -83,6 +95,43 @@ export function evaluateGroupRouteAccessForPolicy(params: { }; } +export function evaluateMatchedGroupAccessForPolicy(params: { + groupPolicy: GroupPolicy; + allowlistConfigured: boolean; + allowlistMatched: boolean; +}): MatchedGroupAccessDecision { + if (params.groupPolicy === "disabled") { + return { + allowed: false, + groupPolicy: params.groupPolicy, + reason: "disabled", + }; + } + + if (params.groupPolicy === "allowlist") { + if (!params.allowlistConfigured) { + return { + allowed: false, + groupPolicy: params.groupPolicy, + reason: "empty_allowlist", + }; + } + if (!params.allowlistMatched) { + return { + allowed: false, + groupPolicy: params.groupPolicy, + reason: "not_allowlisted", + }; + } + } + + return { + allowed: true, + groupPolicy: params.groupPolicy, + reason: "allowed", + }; +} + export function evaluateSenderGroupAccessForPolicy(params: { groupPolicy: GroupPolicy; providerMissingFallbackApplied?: boolean; diff --git a/src/plugin-sdk/index.ts b/src/plugin-sdk/index.ts index 829ef4392cb..d9b8b035971 100644 --- a/src/plugin-sdk/index.ts +++ b/src/plugin-sdk/index.ts @@ -280,11 +280,14 @@ export { } from "./allow-from.js"; export { evaluateGroupRouteAccessForPolicy, + evaluateMatchedGroupAccessForPolicy, evaluateSenderGroupAccess, evaluateSenderGroupAccessForPolicy, resolveSenderScopedGroupPolicy, type GroupRouteAccessDecision, type GroupRouteAccessReason, + type MatchedGroupAccessDecision, + type MatchedGroupAccessReason, type SenderGroupAccessDecision, type SenderGroupAccessReason, } from "./group-access.js"; diff --git a/src/plugin-sdk/nextcloud-talk.ts b/src/plugin-sdk/nextcloud-talk.ts index 1fd4d1d6010..ff22f937cf7 100644 --- a/src/plugin-sdk/nextcloud-talk.ts +++ b/src/plugin-sdk/nextcloud-talk.ts @@ -37,6 +37,7 @@ export type { ChannelPlugin } from "../channels/plugins/types.plugin.js"; export { createReplyPrefixOptions } from "../channels/reply-prefix.js"; export type { OpenClawConfig } from "../config/config.js"; export { mapAllowFromEntries } from "./channel-config-helpers.js"; +export { evaluateMatchedGroupAccessForPolicy } from "./group-access.js"; export { GROUP_POLICY_BLOCKED_LABEL, resolveAllowlistProviderRuntimeGroupPolicy, diff --git a/src/security/dm-policy-shared.test.ts b/src/security/dm-policy-shared.test.ts index 0fa92bbb1b8..ec747170b10 100644 --- a/src/security/dm-policy-shared.test.ts +++ b/src/security/dm-policy-shared.test.ts @@ -388,6 +388,38 @@ describe("security/dm-policy-shared", () => { }); for (const channel of channels) { + it(`[${channel}] blocks groups when group allowlist is empty`, () => { + const decision = resolveDmGroupAccessDecision({ + isGroup: true, + dmPolicy: "pairing", + groupPolicy: "allowlist", + effectiveAllowFrom: ["owner"], + effectiveGroupAllowFrom: [], + isSenderAllowed: () => false, + }); + expect(decision).toEqual({ + decision: "block", + reasonCode: DM_GROUP_ACCESS_REASON.GROUP_POLICY_EMPTY_ALLOWLIST, + reason: "groupPolicy=allowlist (empty allowlist)", + }); + }); + + it(`[${channel}] allows groups when group policy is open`, () => { + const decision = resolveDmGroupAccessDecision({ + isGroup: true, + dmPolicy: "pairing", + groupPolicy: "open", + effectiveAllowFrom: ["owner"], + effectiveGroupAllowFrom: [], + isSenderAllowed: () => false, + }); + expect(decision).toEqual({ + decision: "allow", + reasonCode: DM_GROUP_ACCESS_REASON.GROUP_POLICY_ALLOWED, + reason: "groupPolicy=open", + }); + }); + it(`[${channel}] blocks DM allowlist mode when allowlist is empty`, () => { const decision = resolveDmGroupAccessDecision({ isGroup: false, diff --git a/src/security/dm-policy-shared.ts b/src/security/dm-policy-shared.ts index 2b400734a2a..c23833f9ec3 100644 --- a/src/security/dm-policy-shared.ts +++ b/src/security/dm-policy-shared.ts @@ -2,6 +2,7 @@ import { mergeDmAllowFromSources, resolveGroupAllowFromSources } from "../channe import { resolveControlCommandGate } from "../channels/command-gating.js"; import type { ChannelId } from "../channels/plugins/types.js"; import { readChannelAllowFromStore } from "../pairing/pairing-store.js"; +import { evaluateMatchedGroupAccessForPolicy } from "../plugin-sdk/group-access.js"; import { normalizeStringEntries } from "../shared/string-normalization.js"; export function resolvePinnedMainDmOwnerFromAllowlist(params: { @@ -118,22 +119,28 @@ export function resolveDmGroupAccessDecision(params: { const effectiveGroupAllowFrom = normalizeStringEntries(params.effectiveGroupAllowFrom); if (params.isGroup) { - if (groupPolicy === "disabled") { - return { - decision: "block", - reasonCode: DM_GROUP_ACCESS_REASON.GROUP_POLICY_DISABLED, - reason: "groupPolicy=disabled", - }; - } - if (groupPolicy === "allowlist") { - if (effectiveGroupAllowFrom.length === 0) { + const groupAccess = evaluateMatchedGroupAccessForPolicy({ + groupPolicy, + allowlistConfigured: effectiveGroupAllowFrom.length > 0, + allowlistMatched: params.isSenderAllowed(effectiveGroupAllowFrom), + }); + + if (!groupAccess.allowed) { + if (groupAccess.reason === "disabled") { + return { + decision: "block", + reasonCode: DM_GROUP_ACCESS_REASON.GROUP_POLICY_DISABLED, + reason: "groupPolicy=disabled", + }; + } + if (groupAccess.reason === "empty_allowlist") { return { decision: "block", reasonCode: DM_GROUP_ACCESS_REASON.GROUP_POLICY_EMPTY_ALLOWLIST, reason: "groupPolicy=allowlist (empty allowlist)", }; } - if (!params.isSenderAllowed(effectiveGroupAllowFrom)) { + if (groupAccess.reason === "not_allowlisted") { return { decision: "block", reasonCode: DM_GROUP_ACCESS_REASON.GROUP_POLICY_NOT_ALLOWLISTED, @@ -141,6 +148,7 @@ export function resolveDmGroupAccessDecision(params: { }; } } + return { decision: "allow", reasonCode: DM_GROUP_ACCESS_REASON.GROUP_POLICY_ALLOWED,