diff --git a/tests/gateway/test_discord_bot_auth_bypass.py b/tests/gateway/test_discord_bot_auth_bypass.py index 8ff39a1bf49..7d86e034eb3 100644 --- a/tests/gateway/test_discord_bot_auth_bypass.py +++ b/tests/gateway/test_discord_bot_auth_bypass.py @@ -172,42 +172,49 @@ def test_bot_bypass_does_not_leak_to_other_platforms(monkeypatch): # ----------------------------------------------------------------------------- -# DISCORD_ALLOWED_ROLES gateway-layer bypass (#7871) +# DISCORD_ALLOWED_ROLES no longer bypasses the gateway allowlist (#30742) +# +# Prior behavior: setting DISCORD_ALLOWED_ROLES caused _is_user_authorized +# to return True for ANY Discord event, on the assumption that the adapter +# pre-filter had already validated role membership. That allowed slash +# commands and synthetic voice events to bypass role checks. PR #30742 +# removed the shortcut — Discord auth now flows through the same allowlist +# / pairing / allow-all path as every other platform. # ----------------------------------------------------------------------------- -def test_discord_role_config_bypasses_gateway_allowlist(monkeypatch): - """When DISCORD_ALLOWED_ROLES is set, _is_user_authorized must trust - the adapter's pre-filter and authorize. Without this, role-only setups - (DISCORD_ALLOWED_ROLES populated, DISCORD_ALLOWED_USERS empty) would - hit the 'no allowlists configured' branch and get rejected. +def test_discord_role_config_does_not_bypass_gateway_allowlist(monkeypatch): + """DISCORD_ALLOWED_ROLES alone must NOT authorize at the gateway layer + (regression guard for #30742). Role-based access is enforced by the + adapter pre-filter on real message events; the gateway layer requires + an explicit allowlist hit or pairing approval. """ runner = _make_bare_runner() monkeypatch.setenv("DISCORD_ALLOWED_ROLES", "1493705176387948674") - # Note: DISCORD_ALLOWED_USERS is NOT set — the entire point. + # DISCORD_ALLOWED_USERS deliberately NOT set — verifies the role + # config alone no longer grants authorization. source = _make_discord_human_source(user_id="999888777") - assert runner._is_user_authorized(source) is True + assert runner._is_user_authorized(source) is False -def test_discord_role_config_still_authorizes_alongside_users(monkeypatch): - """Sanity: setting both DISCORD_ALLOWED_ROLES and DISCORD_ALLOWED_USERS - doesn't break the user-id path. Users in the allowlist should still be - authorized even if they don't have a role. (OR semantics.) +def test_discord_user_allowlist_still_authorizes_when_role_is_also_configured(monkeypatch): + """Sanity: DISCORD_ALLOWED_USERS still authorizes users on the list, + independent of DISCORD_ALLOWED_ROLES. This guards against a future + regression that ties the user-allowlist check to the (now-removed) + role bypass. """ runner = _make_bare_runner() monkeypatch.setenv("DISCORD_ALLOWED_ROLES", "1493705176387948674") monkeypatch.setenv("DISCORD_ALLOWED_USERS", "100200300") - # User on the user allowlist, no role → still authorized at gateway - # level via the role bypass (adapter already approved them). source = _make_discord_human_source(user_id="100200300") assert runner._is_user_authorized(source) is True -def test_discord_role_bypass_does_not_leak_to_other_platforms(monkeypatch): +def test_discord_role_config_does_not_leak_to_other_platforms(monkeypatch): """DISCORD_ALLOWED_ROLES must only affect Discord. Setting it should not suddenly start authorizing Telegram users whose platform has its own empty allowlist. diff --git a/tests/gateway/test_feishu_approval_buttons.py b/tests/gateway/test_feishu_approval_buttons.py index 41335f1141a..e739d47b087 100644 --- a/tests/gateway/test_feishu_approval_buttons.py +++ b/tests/gateway/test_feishu_approval_buttons.py @@ -506,6 +506,7 @@ class TestCardActionCallbackResponse: adapter = _make_adapter() adapter._loop = MagicMock() adapter._loop.is_closed = MagicMock(return_value=False) + adapter._allowed_group_users = {"ou_user1"} adapter._approval_state[2] = { "session_key": "sess-2", "message_id": "msg-2", @@ -552,6 +553,7 @@ class TestCardActionCallbackResponse: adapter = _make_adapter() adapter._loop = MagicMock() adapter._loop.is_closed = MagicMock(return_value=False) + adapter._allowed_group_users = {"ou_unknown"} adapter._approval_state[3] = { "session_key": "sess-3", "message_id": "msg-3", @@ -572,6 +574,7 @@ class TestCardActionCallbackResponse: adapter = _make_adapter() adapter._loop = MagicMock() adapter._loop.is_closed = MagicMock(return_value=False) + adapter._allowed_group_users = {"ou_expired"} adapter._approval_state[4] = { "session_key": "sess-4", "message_id": "msg-4", diff --git a/tests/gateway/test_msgraph_webhook.py b/tests/gateway/test_msgraph_webhook.py index 55eb5f7711f..bddcf419014 100644 --- a/tests/gateway/test_msgraph_webhook.py +++ b/tests/gateway/test_msgraph_webhook.py @@ -77,7 +77,8 @@ class TestMSGraphValidationHandshake: adapter = MSGraphWebhookAdapter(PlatformConfig(enabled=True, extra={})) connected = await adapter.connect() assert connected is False - assert adapter.is_connected() is False + # is_connected is a @property on the base adapter, not a method. + assert adapter.is_connected is False @pytest.mark.anyio async def test_validation_token_echo_on_get(self): diff --git a/tests/tools/test_local_interrupt_cleanup.py b/tests/tools/test_local_interrupt_cleanup.py index a9b74559380..67d9e9e6b54 100644 --- a/tests/tools/test_local_interrupt_cleanup.py +++ b/tests/tools/test_local_interrupt_cleanup.py @@ -48,8 +48,14 @@ def _process_group_snapshot(pgid: int) -> str: ).stdout.strip() -def _wait_for_pgid_exit(pgid: int, timeout: float = 10.0) -> bool: - """Wait for a process group to disappear under loaded xdist hosts.""" +def _wait_for_pgid_exit(pgid: int, timeout: float = 30.0) -> bool: + """Wait for a process group to disappear under loaded xdist hosts. + + The cleanup chain is: SIGTERM → 3s TimeoutStopSec → SIGKILL → reap. + Under heavy xdist load (40 parallel workers, 6-shard CI), the full + sequence can exceed 10s. Default timeout is generous to avoid CI + flakes; in practice the wait returns in <1s on quiet hosts. + """ deadline = time.monotonic() + timeout while time.monotonic() < deadline: if not _pgid_still_alive(pgid): @@ -166,9 +172,11 @@ def test_wait_for_process_kills_subprocess_on_keyboardinterrupt(): assert ret == 1, f"SetAsyncExc returned {ret}, expected 1" # Give the worker a moment to: hit the exception at the next poll, - # run the except-block cleanup (_kill_process), and exit. - t.join(timeout=5.0) - assert not t.is_alive(), "worker didn't exit within 5 s of the interrupt" + # run the except-block cleanup (_kill_process), and exit. Under + # xdist load the SIGTERM → 3s wait → SIGKILL chain can take longer + # than 5s before the worker's join() returns; bumped to 15s. + t.join(timeout=15.0) + assert not t.is_alive(), "worker didn't exit within 15 s of the interrupt" # The critical assertion: the subprocess GROUP must be dead. Not # just the bash wrapper — the 'sleep 30' child too. Under xdist load,