fix(gateway): respect own-policy group_policy when resolving unauthorized DM fallback - #34791
fix(gateway): respect own-policy group_policy when resolving unauthorized DM fallback#34791Dusk1e wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused parity fix. The premise remains present on current main: GatewayAuthorizationMixin._get_unauthorized_dm_behavior() checks restrictive dm_policy values at gateway/authz_mixin.py:653-669, then falls through to pair at gateway/authz_mixin.py:710; it does not inspect group_policy.
Problems
- The PR targets the pre-refactor location in
gateway/run.py; currentGatewayRunnernow inheritsGatewayAuthorizationMixinatgateway/run.py:2775, and GitHub reports this branch as conflicting. - Current resolution is profile-aware. A direct
config.extraread would bypass the live profile adapter, while_adapter_group_policy(..., profile=profile)already provides the required effective-policy lookup atgateway/authz_mixin.py:161-194.
Suggested changes
- Salvage the group-policy branch into
gateway/authz_mixin.py:607using_adapter_enforces_own_access_policy(..., profile=profile)and_adapter_group_policy(..., profile=profile). - Port the regression tests and add a multiplexed secondary-profile group-policy case; current multiplex coverage covers only DM policy at
tests/gateway/test_multiplex_profile_authz.py:130-139.
This is an automated hermes-sweeper review.
| @@ -6835,6 +6837,10 @@ def _get_unauthorized_dm_behavior(self, platform: Optional[Platform]) -> str: | |||
| return "pair" | |||
There was a problem hiding this comment.
Current main moved this resolver to gateway/authz_mixin.py and made it profile-aware. When salvaging, use _adapter_group_policy(platform, profile=profile) behind _adapter_enforces_own_access_policy(..., profile=profile) rather than reading only config.extra, so multiplexed and environment-resolved adapter policies are honored.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Seven PRs address related own-policy gateway authorization gaps, but they cover distinct paths: #45444/#45634 establish fail-closed baseline semantics, #37916/#56838/#62340 address Feishu chat-scoped rules, #55875 addresses WhatsApp group authorization, and #34791 addresses unauthorized-DM fallback under restrictive group policy. The diffs show that the remaining fixes must use current, profile-aware effective-policy resolution rather than stale files, adapter-wide flags, or the default adapter registry.
Related pull requests
- #34791
related— (+50/-9) — salvage: The diff correctly makes restrictive own-policy group settings suppress pairing-code replies, but it modifies the pre-refactor gateway/run.py path and reads config.extra directly. Consistent with the keep_open review on #34791, port it to gateway/authz_mixin.py using profile-aware effective group-policy lookup and add a secondary-profile regression. - #37916
related— (+13/-0) — superseded: The diff adds only enforces_own_access_policy to the obsolete gateway/platforms/feishu.py location and cannot represent Feishu's per-chat group_rules policy. Despite the keep_open review on #37916, the complete diff is neither current-main-applicable nor sufficient; #62340 implements the required exact-rule handling in the bundled adapter and gateway. - #45444 [closed]
related— (+158/-55) — merged via #45634: This closed PR supplied the core fail-closed change, trusting own-policy adapters only for effective allowlist restrictions rather than default open or pairing behavior. It remains relevant as the authored source that #45634 cherry-picked and extended. - #45634 [merged]
related— (+258/-55) — merged reference implementation: It landed #45444's fail-closed authorization model and added the WeCom groups.<group_id>.allow_from sibling case with regression coverage. This is the baseline against which the remaining platform-specific diffs must be integrated. - #55875
related— (+126/-0) — salvage: The diff correctly separates WhatsApp's chat-scoped group allowlist from its DM/user allowlist, but resolves self.adapters without source.profile and therefore risks authorizing a secondary-profile message from the default profile's adapter. Consistent with the keep_open review on #55875, retain the narrow fix only after switching to the profile-aware adapter resolver and adding an isolation regression. - #56838
related— (+14/-0) — superseded duplicate of #37916: The diff updates the current Feishu adapter but still adds only a static capability flag plus generic flag coverage; it does not resolve effective group_rules policy for source.chat_id. Despite the keep_open review on #56838, #62340's exact active-rule check addresses the documented open/allowlist inversion that this diff leaves unresolved. - #62340
related— (+170/-7) — merge candidate: Consistent with the keep_open review on #62340, the diff adds profile-aware lookup and trusts only an exact active Feishu group_rules entry, preserving fail-closed behavior for broad/default open policy and disabled rules. Its complete six-file diff does not add a secondary-profile or multiplex isolation test, so that regression should be added before merge even though the implementation passes source.profile into authorization lookup.
Duplicates
#37916 and #56838 are substantially the same insufficient capability-flag-only Feishu fix; #56838 merely targets the migrated adapter and extends generic flag coverage. #45444 and #45634 contain the same core fail-closed hardening, with #45634 being the merged salvage plus the WeCom per-group follow-up.
Suggested consolidation
Merge #62340 after adding an explicit secondary-profile/multiplex isolation regression; close #37916 and #56838 as superseded duplicates. Keep #45634 as the merged baseline and #45444 closed as its preserved source, then salvage #34791 and #55875 as separate profile-aware follow-ups because their unauthorized-DM and WhatsApp group-chat paths are not implemented by #62340.
Cross-PR triage: Reviewed 7 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 67 kB of PR diffs, 12 kB of issue/PR text, 16 kB of discussion (15 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
This brings the config-driven own-policy gateway adapters into parity with the existing allowlist-aware unauthorized-DM fallback.
Before this change,
GatewayRunner._get_unauthorized_dm_behavior()already defaulted toignorewhen env-based user/group allowlists were configured, but the config-driven own-policy path only looked atdm_policy. That left a gap for WeCom, Weixin, Yuanbao, and QQBot: operators could restrict group access withgroup_policy: allowlistorgroup_policy: disabled, yet unauthorized DMs would still receive pairing codes unless they also set an explicit DM restriction or explicitunauthorized_dm_behavior.This patch treats restrictive
group_policyvalues on own-policy adapters the same way as the existing allowlist-aware fallback:group_policy: allowlist=>ignoregroup_policy: disabled=>ignoreExplicit overrides still win:
unauthorized_dm_behaviorunauthorized_dm_behaviordm_policy: pairingWhy this is correct
These adapters already enforce
group_policyat intake, so a restrictive group config is an operator-declared access restriction, just like the env-based allowlist cases already handled in_get_unauthorized_dm_behavior().That makes the previous behavior inconsistent and noisy: a stranger DM could still get a pairing code even though the operator had intentionally locked group access down on the same adapter.
Parity / prior art
This is a parity follow-up to the allowlist-aware unauthorized-DM fallback work from
#9337.#9337 taught
_get_unauthorized_dm_behavior()to default toignorewhen env-based allowlists were present, to avoid pairing spam / unnecessary info leakage to unauthorized contacts. This PR applies the same principle to the config-driven own-policy sibling path.Related background:
#34515established the own-policy adapter contract and gateway trust path for WeCom / Weixin / Yuanbao / QQBot.Changes
_get_unauthorized_dm_behavior()to treat restrictivegroup_policyvalues asignorefor adapters that declareenforces_own_access_policy.unauthorized_dm_behavior: pairbehavior unchanged.group_policy in {"allowlist", "disabled"}unauthorized_dm_behavior: pairoverriding the restrictive defaultTests
Passed:
tests/gateway/test_config_driven_access_policy.pytests/gateway/test_unauthorized_dm_behavior.pytests/gateway/test_wecom.pytests/gateway/test_weixin.pytests/gateway/test_qqbot.pytests/test_yuanbao_pipeline.pyLocal result:
413 passed, 4 warningsThe warnings are pre-existing QQBot test warnings and are not introduced by this change.