fix: add discord role allowlists (#10650) (thanks @Minidoracat)

2026-05-08 18:08:27 +00:00 · 2026-02-12 19:50:10 -06:00
parent f7adc21d31
commit 22fe30c1df
12 changed files with 293 additions and 122 deletions
--- a/src/discord/monitor/agent-components.ts
+++ b/src/discord/monitor/agent-components.ts
@@ -24,7 +24,7 @@ import {
  resolveDiscordAllowListMatch,
  resolveDiscordChannelConfigWithFallback,
  resolveDiscordGuildEntry,
-  resolveDiscordUserAllowed,
+  resolveDiscordMemberAllowed,
 } from "./allow-list.js";
 import { formatDiscordUserTag } from "./format.js";

@@ -233,6 +233,9 @@ export class AgentComponentButton extends Button {
    // when guild is not cached even though guild_id is present in rawData
    const rawGuildId = interaction.rawData.guild_id;
    const isDirectMessage = !rawGuildId;
+    const memberRoleIds = Array.isArray(interaction.rawData.member?.roles)
+      ? interaction.rawData.member.roles.map((roleId: string) => String(roleId))
+      : [];

    if (isDirectMessage) {
      const authorized = await ensureDmComponentAuthorized({
@@ -294,25 +297,26 @@ export class AgentComponentButton extends Button {
      });

      const channelUsers = channelConfig?.users ?? guildInfo?.users;
-      if (Array.isArray(channelUsers) && channelUsers.length > 0) {
-        const userOk = resolveDiscordUserAllowed({
-          allowList: channelUsers,
-          userId,
-          userName: user.username,
-          userTag: user.discriminator ? `${user.username}#${user.discriminator}` : undefined,
-        });
-        if (!userOk) {
-          logVerbose(`agent button: blocked user ${userId} (not in allowlist)`);
-          try {
-            await interaction.reply({
-              content: "You are not authorized to use this button.",
-              ephemeral: true,
-            });
-          } catch {
-            // Interaction may have expired
-          }
-          return;
+      const channelRoles = channelConfig?.roles ?? guildInfo?.roles;
+      const memberAllowed = resolveDiscordMemberAllowed({
+        userAllowList: channelUsers,
+        roleAllowList: channelRoles,
+        memberRoleIds,
+        userId,
+        userName: user.username,
+        userTag: user.discriminator ? `${user.username}#${user.discriminator}` : undefined,
+      });
+      if (!memberAllowed) {
+        logVerbose(`agent button: blocked user ${userId} (not in users/roles allowlist)`);
+        try {
+          await interaction.reply({
+            content: "You are not authorized to use this button.",
+            ephemeral: true,
+          });
+        } catch {
+          // Interaction may have expired
        }
+        return;
      }
    }

@@ -322,6 +326,7 @@ export class AgentComponentButton extends Button {
      channel: "discord",
      accountId: this.ctx.accountId,
      guildId: rawGuildId,
+      memberRoleIds,
      peer: {
        kind: isDirectMessage ? "direct" : "channel",
        id: isDirectMessage ? userId : channelId,
@@ -399,6 +404,9 @@ export class AgentSelectMenu extends StringSelectMenu {
    // when guild is not cached even though guild_id is present in rawData
    const rawGuildId = interaction.rawData.guild_id;
    const isDirectMessage = !rawGuildId;
+    const memberRoleIds = Array.isArray(interaction.rawData.member?.roles)
+      ? interaction.rawData.member.roles.map((roleId: string) => String(roleId))
+      : [];

    if (isDirectMessage) {
      const authorized = await ensureDmComponentAuthorized({
@@ -456,25 +464,26 @@ export class AgentSelectMenu extends StringSelectMenu {
      });

      const channelUsers = channelConfig?.users ?? guildInfo?.users;
-      if (Array.isArray(channelUsers) && channelUsers.length > 0) {
-        const userOk = resolveDiscordUserAllowed({
-          allowList: channelUsers,
-          userId,
-          userName: user.username,
-          userTag: user.discriminator ? `${user.username}#${user.discriminator}` : undefined,
-        });
-        if (!userOk) {
-          logVerbose(`agent select: blocked user ${userId} (not in allowlist)`);
-          try {
-            await interaction.reply({
-              content: "You are not authorized to use this select menu.",
-              ephemeral: true,
-            });
-          } catch {
-            // Interaction may have expired
-          }
-          return;
+      const channelRoles = channelConfig?.roles ?? guildInfo?.roles;
+      const memberAllowed = resolveDiscordMemberAllowed({
+        userAllowList: channelUsers,
+        roleAllowList: channelRoles,
+        memberRoleIds,
+        userId,
+        userName: user.username,
+        userTag: user.discriminator ? `${user.username}#${user.discriminator}` : undefined,
+      });
+      if (!memberAllowed) {
+        logVerbose(`agent select: blocked user ${userId} (not in users/roles allowlist)`);
+        try {
+          await interaction.reply({
+            content: "You are not authorized to use this select menu.",
+            ephemeral: true,
+          });
+        } catch {
+          // Interaction may have expired
        }
+        return;
      }
    }

@@ -488,6 +497,7 @@ export class AgentSelectMenu extends StringSelectMenu {
      channel: "discord",
      accountId: this.ctx.accountId,
      guildId: rawGuildId,
+      memberRoleIds,
      peer: {
        kind: isDirectMessage ? "direct" : "channel",
        id: isDirectMessage ? userId : channelId,
--- a/src/discord/monitor/allow-list.test.ts
+++ b/src/discord/monitor/allow-list.test.ts
@@ -1,6 +1,10 @@
 import { describe, expect, it } from "vitest";
 import type { DiscordChannelConfigResolved } from "./allow-list.js";
-import { resolveDiscordOwnerAllowFrom } from "./allow-list.js";
+import {
+  resolveDiscordMemberAllowed,
+  resolveDiscordOwnerAllowFrom,
+  resolveDiscordRoleAllowed,
+} from "./allow-list.js";

 describe("resolveDiscordOwnerAllowFrom", () => {
  it("returns undefined when no allowlist is configured", () => {
@@ -39,3 +43,87 @@ describe("resolveDiscordOwnerAllowFrom", () => {
    expect(result).toEqual(["some-user"]);
  });
 });
+
+describe("resolveDiscordRoleAllowed", () => {
+  it("allows when no role allowlist is configured", () => {
+    const allowed = resolveDiscordRoleAllowed({
+      allowList: undefined,
+      memberRoleIds: ["role-1"],
+    });
+
+    expect(allowed).toBe(true);
+  });
+
+  it("matches role IDs only", () => {
+    const allowed = resolveDiscordRoleAllowed({
+      allowList: ["123"],
+      memberRoleIds: ["123", "456"],
+    });
+
+    expect(allowed).toBe(true);
+  });
+
+  it("does not match non-ID role entries", () => {
+    const allowed = resolveDiscordRoleAllowed({
+      allowList: ["Admin"],
+      memberRoleIds: ["Admin"],
+    });
+
+    expect(allowed).toBe(false);
+  });
+
+  it("returns false when no matching role IDs", () => {
+    const allowed = resolveDiscordRoleAllowed({
+      allowList: ["456"],
+      memberRoleIds: ["123"],
+    });
+
+    expect(allowed).toBe(false);
+  });
+});
+
+describe("resolveDiscordMemberAllowed", () => {
+  it("allows when no user or role allowlists are configured", () => {
+    const allowed = resolveDiscordMemberAllowed({
+      userAllowList: undefined,
+      roleAllowList: undefined,
+      memberRoleIds: [],
+      userId: "u1",
+    });
+
+    expect(allowed).toBe(true);
+  });
+
+  it("allows when user allowlist matches", () => {
+    const allowed = resolveDiscordMemberAllowed({
+      userAllowList: ["123"],
+      roleAllowList: ["456"],
+      memberRoleIds: ["999"],
+      userId: "123",
+    });
+
+    expect(allowed).toBe(true);
+  });
+
+  it("allows when role allowlist matches", () => {
+    const allowed = resolveDiscordMemberAllowed({
+      userAllowList: ["999"],
+      roleAllowList: ["456"],
+      memberRoleIds: ["456"],
+      userId: "123",
+    });
+
+    expect(allowed).toBe(true);
+  });
+
+  it("denies when user and role allowlists do not match", () => {
+    const allowed = resolveDiscordMemberAllowed({
+      userAllowList: ["u2"],
+      roleAllowList: ["role-2"],
+      memberRoleIds: ["role-1"],
+      userId: "u1",
+    });
+
+    expect(allowed).toBe(false);
+  });
+});
--- a/src/discord/monitor/allow-list.ts
+++ b/src/discord/monitor/allow-list.ts
@@ -157,6 +157,51 @@ export function resolveDiscordUserAllowed(params: {
  });
 }

+export function resolveDiscordRoleAllowed(params: {
+  allowList?: Array<string | number>;
+  memberRoleIds: string[];
+}) {
+  // Role allowlists accept role IDs only (string or number). Names are ignored.
+  const allowList = normalizeDiscordAllowList(params.allowList, ["role:"]);
+  if (!allowList) {
+    return true;
+  }
+  if (allowList.allowAll) {
+    return true;
+  }
+  return params.memberRoleIds.some((roleId) => allowList.ids.has(roleId));
+}
+
+export function resolveDiscordMemberAllowed(params: {
+  userAllowList?: Array<string | number>;
+  roleAllowList?: Array<string | number>;
+  memberRoleIds: string[];
+  userId: string;
+  userName?: string;
+  userTag?: string;
+}) {
+  const hasUserRestriction = Array.isArray(params.userAllowList) && params.userAllowList.length > 0;
+  const hasRoleRestriction = Array.isArray(params.roleAllowList) && params.roleAllowList.length > 0;
+  if (!hasUserRestriction && !hasRoleRestriction) {
+    return true;
+  }
+  const userOk = hasUserRestriction
+    ? resolveDiscordUserAllowed({
+        allowList: params.userAllowList,
+        userId: params.userId,
+        userName: params.userName,
+        userTag: params.userTag,
+      })
+    : false;
+  const roleOk = hasRoleRestriction
+    ? resolveDiscordRoleAllowed({
+        allowList: params.roleAllowList,
+        memberRoleIds: params.memberRoleIds,
+      })
+    : false;
+  return userOk || roleOk;
+}
+
 export function resolveDiscordOwnerAllowFrom(params: {
  channelConfig?: DiscordChannelConfigResolved | null;
  guildInfo?: DiscordGuildEntryResolved | null;
@@ -184,20 +229,6 @@ export function resolveDiscordOwnerAllowFrom(params: {
  return [match.matchKey];
 }

-export function resolveDiscordRoleAllowed(params: {
-  allowList?: Array<string | number>;
-  memberRoleIds: string[];
-}) {
-  const allowList = normalizeDiscordAllowList(params.allowList, ["role:"]);
-  if (!allowList) {
-    return true;
-  }
-  if (allowList.allowAll) {
-    return true;
-  }
-  return params.memberRoleIds.some((roleId) => allowList.ids.has(roleId));
-}
-
 export function resolveDiscordCommandAuthorized(params: {
  isDirectMessage: boolean;
  allowFrom?: Array<string | number>;
--- a/src/discord/monitor/listeners.ts
+++ b/src/discord/monitor/listeners.ts
@@ -275,11 +275,15 @@ async function handleDiscordReactionEvent(params: {
    const authorLabel = message?.author ? formatDiscordUserTag(message.author) : undefined;
    const baseText = `Discord reaction ${action}: ${emojiLabel} by ${actorLabel} on ${guildSlug} ${channelLabel} msg ${data.message_id}`;
    const text = authorLabel ? `${baseText} from ${authorLabel}` : baseText;
+    const memberRoleIds = Array.isArray(data.member?.roles)
+      ? data.member.roles.map((roleId: string) => String(roleId))
+      : [];
    const route = resolveAgentRoute({
      cfg: params.cfg,
      channel: "discord",
      accountId: params.accountId,
      guildId: data.guild_id ?? undefined,
+      memberRoleIds,
      peer: {
        kind: isDirectMessage ? "direct" : isGroupDm ? "group" : "channel",
        id: isDirectMessage ? user.id : data.channel_id,
--- a/src/discord/monitor/message-handler.preflight.ts
+++ b/src/discord/monitor/message-handler.preflight.ts
@@ -38,9 +38,8 @@ import {
  resolveDiscordAllowListMatch,
  resolveDiscordChannelConfigWithFallback,
  resolveDiscordGuildEntry,
+  resolveDiscordMemberAllowed,
  resolveDiscordShouldRequireMention,
-  resolveDiscordRoleAllowed,
-  resolveDiscordUserAllowed,
  resolveGroupDmAllow,
 } from "./allow-list.js";
 import {
@@ -221,8 +220,9 @@ export async function preflightDiscordMessage(
  }

  // Fresh config for bindings lookup; other routing inputs are payload-derived.
-  // member.roles is already string[] (Snowflake IDs) per Discord API types
-  const memberRoleIds: string[] = params.data.member?.roles ?? [];
+  const memberRoleIds = Array.isArray(params.data.member?.roles)
+    ? params.data.member.roles.map((roleId: string) => String(roleId))
+    : [];
  const route = resolveAgentRoute({
    cfg: loadConfig(),
    channel: "discord",
@@ -455,6 +455,19 @@ export async function preflightDiscordMessage(
    surface: "discord",
  });
  const hasControlCommandInMessage = hasControlCommand(baseText, params.cfg);
+  const channelUsers = channelConfig?.users ?? guildInfo?.users;
+  const channelRoles = channelConfig?.roles ?? guildInfo?.roles;
+  const hasAccessRestrictions =
+    (Array.isArray(channelUsers) && channelUsers.length > 0) ||
+    (Array.isArray(channelRoles) && channelRoles.length > 0);
+  const memberAllowed = resolveDiscordMemberAllowed({
+    userAllowList: channelUsers,
+    roleAllowList: channelRoles,
+    memberRoleIds,
+    userId: sender.id,
+    userName: sender.name,
+    userTag: sender.tag,
+  });

  if (!isDirectMessage) {
    const ownerAllowList = normalizeDiscordAllowList(params.allowFrom, [
@@ -469,22 +482,12 @@ export async function preflightDiscordMessage(
          tag: sender.tag,
        })
      : false;
-    const channelUsers = channelConfig?.users ?? guildInfo?.users;
-    const usersOk =
-      Array.isArray(channelUsers) && channelUsers.length > 0
-        ? resolveDiscordUserAllowed({
-            allowList: channelUsers,
-            userId: sender.id,
-            userName: sender.name,
-            userTag: sender.tag,
-          })
-        : false;
    const useAccessGroups = params.cfg.commands?.useAccessGroups !== false;
    const commandGate = resolveControlCommandGate({
      useAccessGroups,
      authorizers: [
        { configured: ownerAllowList != null, allowed: ownerOk },
-        { configured: Array.isArray(channelUsers) && channelUsers.length > 0, allowed: usersOk },
+        { configured: hasAccessRestrictions, allowed: memberAllowed },
      ],
      modeWhenAccessGroupsOff: "configured",
      allowTextCommands,
@@ -536,35 +539,9 @@ export async function preflightDiscordMessage(
    }
  }

-  if (isGuildMessage) {
-    const channelUsers = channelConfig?.users ?? guildInfo?.users;
-    const channelRoles = channelConfig?.roles ?? guildInfo?.roles;
-    const hasUserRestriction = Array.isArray(channelUsers) && channelUsers.length > 0;
-    const hasRoleRestriction = Array.isArray(channelRoles) && channelRoles.length > 0;
-
-    if (hasUserRestriction || hasRoleRestriction) {
-      // member.roles is already string[] (Snowflake IDs) per Discord API types
-      const memberRoleIds: string[] = params.data.member?.roles ?? [];
-      const userOk = hasUserRestriction
-        ? resolveDiscordUserAllowed({
-            allowList: channelUsers,
-            userId: sender.id,
-            userName: sender.name,
-            userTag: sender.tag,
-          })
-        : false;
-      const roleOk = hasRoleRestriction
-        ? resolveDiscordRoleAllowed({
-            allowList: channelRoles,
-            memberRoleIds,
-          })
-        : false;
-
-      if (!userOk && !roleOk) {
-        logVerbose(`Blocked discord guild sender ${sender.id} (not in users/roles allowlist)`);
-        return null;
-      }
-    }
+  if (isGuildMessage && hasAccessRestrictions && !memberAllowed) {
+    logVerbose(`Blocked discord guild sender ${sender.id} (not in users/roles allowlist)`);
+    return null;
  }

  const systemLocation = resolveDiscordSystemLocation({
--- a/src/discord/monitor/native-command.ts
+++ b/src/discord/monitor/native-command.ts
@@ -50,8 +50,8 @@ import {
  normalizeDiscordSlug,
  resolveDiscordChannelConfigWithFallback,
  resolveDiscordGuildEntry,
+  resolveDiscordMemberAllowed,
  resolveDiscordOwnerAllowFrom,
-  resolveDiscordUserAllowed,
 } from "./allow-list.js";
 import { resolveDiscordChannelInfo } from "./message-utils.js";
 import { resolveDiscordSenderIdentity } from "./sender-identity.js";
@@ -540,6 +540,9 @@ async function dispatchDiscordCommandInteraction(params: {
  const channelName = channel && "name" in channel ? (channel.name as string) : undefined;
  const channelSlug = channelName ? normalizeDiscordSlug(channelName) : "";
  const rawChannelId = channel?.id ?? "";
+  const memberRoleIds = Array.isArray(interaction.rawData.member?.roles)
+    ? interaction.rawData.member.roles.map((roleId: string) => String(roleId))
+    : [];
  const ownerAllowList = normalizeDiscordAllowList(discordConfig?.dm?.allowFrom ?? [], [
    "discord:",
    "user:",
@@ -662,21 +665,24 @@ async function dispatchDiscordCommandInteraction(params: {
  }
  if (!isDirectMessage) {
    const channelUsers = channelConfig?.users ?? guildInfo?.users;
-    const hasUserAllowlist = Array.isArray(channelUsers) && channelUsers.length > 0;
-    const userOk = hasUserAllowlist
-      ? resolveDiscordUserAllowed({
-          allowList: channelUsers,
-          userId: sender.id,
-          userName: sender.name,
-          userTag: sender.tag,
-        })
-      : false;
+    const channelRoles = channelConfig?.roles ?? guildInfo?.roles;
+    const hasAccessRestrictions =
+      (Array.isArray(channelUsers) && channelUsers.length > 0) ||
+      (Array.isArray(channelRoles) && channelRoles.length > 0);
+    const memberAllowed = resolveDiscordMemberAllowed({
+      userAllowList: channelUsers,
+      roleAllowList: channelRoles,
+      memberRoleIds,
+      userId: sender.id,
+      userName: sender.name,
+      userTag: sender.tag,
+    });
    const authorizers = useAccessGroups
      ? [
          { configured: ownerAllowList != null, allowed: ownerOk },
-          { configured: hasUserAllowlist, allowed: userOk },
+          { configured: hasAccessRestrictions, allowed: memberAllowed },
        ]
-      : [{ configured: hasUserAllowlist, allowed: userOk }];
+      : [{ configured: hasAccessRestrictions, allowed: memberAllowed }];
    commandAuthorized = resolveCommandAuthorizedFromAuthorizers({
      useAccessGroups,
      authorizers,
@@ -735,6 +741,7 @@ async function dispatchDiscordCommandInteraction(params: {
    channel: "discord",
    accountId,
    guildId: interaction.guild?.id ?? undefined,
+    memberRoleIds,
    peer: {
      kind: isDirectMessage ? "direct" : isGroupDm ? "group" : "channel",
      id: isDirectMessage ? user.id : channelId,