Skip to content

feat(mattermost): per-channel allow_from restricts who can address the bot in a listed channel - #89972

Open
100yenadmin wants to merge 1 commit into
NousResearch:mainfrom
100yenadmin:upstream/mattermost-per-channel-acl
Open

100yenadmin wants to merge 1 commit into
NousResearch:mainfrom
100yenadmin:upstream/mattermost-per-channel-acl

Conversation

@100yenadmin

@100yenadmin 100yenadmin commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds an optional sender allowlist for explicitly listed Mattermost channels. A channel entry under extra.groups.<channel-id> can use allow_from or allowFrom; exact channel keys win, followed by a case-insensitive match and then the "*" key fallback. A "*" allowlist member admits any sender. Sender IDs are compared as exact strings; the adapter does not resolve usernames.

Why in the adapter: the shared gateway already recognizes this vocabulary — _adapter_group_has_sender_allowlist (gateway/authz_mixin.py:355-378) resolves groups.<id>.allow_from / allowFrom with the same exact, case-insensitive and "*" lookup, and for adapters that enforce their own access policy the own-policy path treats a configured per-group list as proof that the adapter gated senders at intake (:398-399). The Mattermost adapter had no such gate between channel resolution and mention gating, so a configured per-channel list had no effect there. This PR adds exactly that gate, with the same lookup order.

Malformed authorization config does not fail open. A bare integer or string is treated as one sender ID. Dicts, booleans, nested lists, and non-mapping channel entries log one warning per config key and deny everyone for that listed channel.

DM sender restriction already exists in the shared gateway: top-level allow_from is combined with the pairing store and platform-wide authorization. This PR deliberately does not duplicate that DM filter in the Mattermost adapter, so pairing and unauthorized_dm_behavior continue to run through the existing gateway path.

Unlisted channels—including Mattermost Group Messages (channel_type "G")—retain upstream's existing behavior. Denials in listed channels remain visible at debug level; the first denial for each (channel_id, sender_id) pair is also logged at info level, with the in-adapter set capped at 512 entries.

Related Issue

No open issue. This adds Mattermost enforcement for the per-group groups.<id>.allow_from vocabulary already recognized by shared gateway authorization.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • plugins/platforms/mattermost/adapter.py:66-130 — normalizes scalar/flat-list sender IDs, resolves exact/case-insensitive/wildcard channel keys, warns once for malformed keys, and fails closed only for the affected listed channel.
  • plugins/platforms/mattermost/adapter.py:192-193 — keeps bounded denial-log state and once-per-key malformed-config warning state.
  • plugins/platforms/mattermost/adapter.py:630-647 — applies the sender restriction only inside the existing non-DM branch, before mention gating and downstream handling.
  • tests/gateway/test_mattermost.py:397-511 — covers listed sender allow/deny, unlisted Group Message behavior, empty-list inheritance, integer/string scalars, wildcard member/key behavior, case-insensitive channel keys, and malformed-shape fail-closed warnings.

A question for you on the channel default

I chose inherit-existing for an absent or empty channel allowlist. A channel not present in groups, a listed channel with no allow_from key, or a valid empty allowlist keeps the existing Mattermost behavior. Only an explicitly configured, non-empty allowlist narrows senders. Malformed configured values are different: they warn and fail closed.

The stricter alternative is deny-empty for listed channels: once a channel appears in groups, a missing or empty allow_from would deny everyone there. That is safer for incomplete rollout config but changes the meaning of existing channel entries that carry other settings. _channel_scope_allows is the single decision point if maintainers prefer that policy.

Example:

platforms:
  mattermost:
    extra:
      groups:
        channel-id:
          allow_from:
            - mattermost-user-id

How to Test

  1. On upstream/main (1ab32b212b3828be8239bd68ac3687756bf7c5c2), retain this PR's test changes while restoring plugins/platforms/mattermost/adapter.py from the base.
  2. Run scripts/run_tests.sh tests/gateway/test_mattermost.py -q. Expected before the change: 30 passed, 8 failed.
  3. Restore the adapter from this PR head (56abbb9b94068d5ff1ad460fa4ac3bb3da905ca1) and rerun the same command. Expected: 38 passed, 0 failed.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — focused file only, per repository local-test limits
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.6.2 arm64

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — left open until maintainers settle the channel default question above
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A; this reuses existing extra.groups shapes
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure Python config and intake logic
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Not applicable; focused automated behavior proof is described above.

What this PR does NOT change: DM authorization (allow_from, pairing store, unauthorized_dm_behavior, or MATTERMOST_ALLOWED_USERS); allowed_channels, require_mention, or free_response_channels; unlisted channel behavior, including Group Messages; shared gateway authorization code; docs; or any live Mattermost/runtime deployment.

@alt-glitch alt-glitch added type/feature New feature or request comp/plugins Plugin system and bundled plugins area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 19, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Well-scoped feature reusing upstream config vocabulary (allow_from / groups.<id>.allow_from mirroring authz_mixin, so one mental model covers both), with the right default semantics (unset/empty adds no denial), independent DM/channel scopes, * catch-all, CSV-or-list normalization, and a test matrix covering every combination including the DM-doesn't-narrow-channels asymmetry. Items:

  • plugins/platforms/mattermost/adapter.py:933 — nit — denied senders are dropped at debug level only; an operator who typos a user_id in allow_from gets silence exactly where they're looking for a response — suggestion — log denials at info (once per sender/channel pair per cooldown) or expose a deny counter in status output.

  • plugins/platforms/mattermost/adapter.py:936 — nit (coverage) — no test for the "*" catch-all member or a groups entry keyed by case-insensitive channel-id match (the lookup supports both); each is two lines against the existing harness.

No blocking issues found.

— reviewer-b (automated review)

…e bot in a listed channel

Enforce groups.<channel-id>.allow_from at Mattermost WebSocket intake without duplicating the gateway's existing DM allow_from and pairing authorization path.

Exact, case-insensitive, and wildcard channel keys are supported. Scalar sender IDs normalize to one entry; malformed allowlists and non-mapping channel entries warn once per config key and fail closed for that channel. Unlisted channels and group messages retain existing behavior.

Focused behavior proof: tests/gateway/test_mattermost.py is red against upstream/main and green on this change.
@100yenadmin
100yenadmin force-pushed the upstream/mattermost-per-channel-acl branch from d14c5b3 to 56abbb9 Compare September 14, 2026 19:43
@100yenadmin 100yenadmin changed the title feat(mattermost): per-channel and DM user allowlists feat(mattermost): per-channel allow_from restricts who can address the bot in a listed channel Sep 14, 2026
@100yenadmin

Copy link
Copy Markdown
Contributor Author

Narrowed after re-verifying on current main (head 56abbb9, rebased onto 1ab32b2): the adapter-level DM filter is gone. Top-level extra.allow_from is already bridged by gateway/config_loader.py:200 and enforced for DM traffic in gateway/authz_mixin.py:419 as a union with the pairing store (:587-589), so an adapter-side copy would have dropped pairing-approved senders before the gateway saw them and made unauthorized_dm_behavior: pair unreachable.

What remains is the per-channel half: groups.<channel_id>.allow_from gates who can address the bot in a listed channel, using the same exact → case-insensitive → "*" lookup the gateway's _adapter_group_has_sender_allowlist uses. Unlisted channels, including group messages, are unchanged. Malformed allowlist shapes now warn once and fail closed instead of admitting everyone; a bare scalar id normalizes to one entry. The first denial per (channel, sender) pair is logged at info so a typo in the list is visible.

Focused tests/gateway/test_mattermost.py: 38 passed at head; the 8 new behaviour tests are red on base. The open question in the body — inherit-existing vs deny-empty for a listed channel with an empty list — is yours to call; I went with inherit-existing.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants