fix(knowledge): run approval gate before persisting space membership

For APPROVAL knowledge spaces, subscribe_space() wrote the PENDING space_channel_member row *before* invoking the approval gate. When the scenario was missing/disabled the gate raised ApprovalScenarioDisabledError, so the user saw an error but the PENDING membership was already committed. On the next click the top-of-function 'already pending -> return pending' early-return short-circuited before the gate, masking the error (first click errors, second click silently succeeds). Invoke the gate first and persist membership only after it decides (pass -> ACTIVE, pending/exception -> PENDING) via a new _persist_space_member() helper. A missing scenario now leaves no membership and errors consistently on every click. Also repair pre-existing bitrot in the approval integration tests (quota is_admin, str-enum decision/task_ids, unmocked department DAO).
2026-08-28 17:14:44 +08:00 · 2026-06-18 15:58:31 +08:00
parent 76dd5469c6
commit 87bfe5cff6
3 changed files with 178 additions and 69 deletions
@@ -144,6 +144,7 @@ ApprovalCenterService.decide_task()
 - **Handler**：`KnowledgeSpaceSubscribeScenarioHandler`
 - 通过 / ACTIVE 路径调 `sync_direct_space_user_permissions()` 写 ReBAC 关系
 - PENDING 时调 `_send_space_approval_notification()` 通知审批人
+- **不变量：先过网关、再落 membership。** `subscribe_space` 对 APPROVAL 空间必须先 `await gate.request_or_pass()`，按 gate 结果（pass→ACTIVE / pending·exception→PENDING）才通过 `_persist_space_member()` 写 `space_channel_member`。**严禁在调网关前预写 PENDING membership**——否则场景未配置/未启用时网关 `raise ApprovalScenarioDisabledError`，但 PENDING 行已落库，下次点"关注"会被 `subscribe_space` 顶部"已 PENDING 直接返回 pending"的早退分支短路，掩盖错误（首次报错、二次假成功）。无场景时每次点击都应一致报错。

 ---

@@ -4323,21 +4323,14 @@ class KnowledgeSpaceService(KnowledgeUtils):
                    raise SpaceSubscribeLimitError(quota=effective)

        previous_status = existing.status if existing else None
-        if existing:
-            existing.status = target_status
-            existing = await SpaceChannelMemberDao.update(existing)
-            member = existing
-        else:
-            member = SpaceChannelMember(
-                business_id=str(space_id),
-                business_type=BusinessTypeEnum.SPACE,
-                user_id=self.login_user.user_id,
-                user_role=UserRoleEnum.MEMBER,
-                status=target_status,
-            )
-            await SpaceChannelMemberDao.async_insert_member(member)

        if space.auth_type == AuthTypeEnum.APPROVAL:
+            # Run the approval gate BEFORE persisting any membership change. If the
+            # scenario is missing/disabled the gate raises ApprovalScenarioDisabledError;
+            # we must not leave a stray PENDING membership behind, otherwise the early
+            # "already pending" return above would short-circuit before the gate on the
+            # next click and mask the error (first click errors, second click silently
+            # "succeeds"). The membership is written only once the gate has decided.
            gate = self.approval_gate or self._build_space_approval_gate()
            primary_dept = await UserDepartmentDao.aget_user_primary_department(self.login_user.user_id)
            gate_result = await gate.request_or_pass(
@@ -4360,12 +4353,14 @@ class KnowledgeSpaceService(KnowledgeUtils):
                    ip_address=get_request_ip(self.request) if self.request else None,
                )
            )
+
+            # PASS → activate immediately; PENDING/EXCEPTION → keep as pending member.
+            resolved_status = (
+                MembershipStatusEnum.ACTIVE if gate_result.decision == "pass" else MembershipStatusEnum.PENDING
+            )
+            member = await self._persist_space_member(existing, space_id, resolved_status)
+
            if gate_result.decision == "pass":
-                member.status = MembershipStatusEnum.ACTIVE
-                if existing:
-                    member = await SpaceChannelMemberDao.update(member)
-                else:
-                    member = await SpaceChannelMemberDao.update(member)
                await self.__class__.sync_direct_space_user_permissions(
                    space_id,
                    member.user_id,
@@ -4376,13 +4371,21 @@ class KnowledgeSpaceService(KnowledgeUtils):
                    "status": "subscribed",
                    "space_id": space_id,
                }
+
            if gate_result.decision == ApprovalGateDecision.PENDING and gate_result.task_ids and self.message_service:
                await self._send_space_approval_notification(
                    space=space,
                    instance_id=gate_result.instance_id,
                    task_ids=gate_result.task_ids,
                )
-        elif previous_status != MembershipStatusEnum.PENDING:
+            return {
+                "status": "pending",
+                "space_id": space_id,
+            }
+
+        # PUBLIC space → activate immediately, no approval gate.
+        member = await self._persist_space_member(existing, space_id, target_status)
+        if previous_status != MembershipStatusEnum.PENDING:
            await self._send_subscription_notification(space)

        if member.status == MembershipStatusEnum.ACTIVE:
@@ -4398,6 +4401,25 @@ class KnowledgeSpaceService(KnowledgeUtils):
            "space_id": space_id,
        }

+    async def _persist_space_member(self, existing, space_id: int, status: MembershipStatusEnum):
+        """Insert or update the current user's space membership to ``status``.
+
+        Centralizes the insert/update branch so callers can defer persistence
+        until after a decision (e.g. the approval gate) has been made.
+        """
+        if existing:
+            existing.status = status
+            return await SpaceChannelMemberDao.update(existing)
+        member = SpaceChannelMember(
+            business_id=str(space_id),
+            business_type=BusinessTypeEnum.SPACE,
+            user_id=self.login_user.user_id,
+            user_role=UserRoleEnum.MEMBER,
+            status=status,
+        )
+        await SpaceChannelMemberDao.async_insert_member(member)
+        return member
+
    def _build_space_approval_gate(self) -> ApprovalGate:
        registry = ApprovalRegistry.with_default_presets()
        registry.register_handler(
@@ -2,90 +2,176 @@ from types import SimpleNamespace
 from unittest.mock import AsyncMock, patch

 import pytest
+
 from test.knowledge.test_knowledge_space_service import _load_service_class


@pytest.mark.asyncio
 async def test_approval_space_subscription_uses_approval_gate_pending():
    from bisheng.common.models.space_channel_member import (
-        BusinessTypeEnum,
        MembershipStatusEnum,
        UserRoleEnum,
    )
    from bisheng.knowledge.domain.models.knowledge import AuthTypeEnum, KnowledgeTypeEnum

-    service = _load_service_class()(None, SimpleNamespace(user_id=42, user_name='alice', tenant_id=7))
+    service = _load_service_class()(None, SimpleNamespace(user_id=42, user_name="alice", tenant_id=7))
    service.message_service = SimpleNamespace(send_generic_approval=AsyncMock())
    service.approval_gate = SimpleNamespace(
-        request_or_pass=AsyncMock(return_value=SimpleNamespace(decision='pending', instance_id=21))
+        request_or_pass=AsyncMock(return_value=SimpleNamespace(decision="pending", instance_id=21, task_ids=[]))
    )

-    space = SimpleNamespace(id=12, name='研发知识空间', type=KnowledgeTypeEnum.SPACE.value, auth_type=AuthTypeEnum.APPROVAL)
+    space = SimpleNamespace(
+        id=12, name="研发知识空间", type=KnowledgeTypeEnum.SPACE.value, auth_type=AuthTypeEnum.APPROVAL
+    )

-    with patch(
-        'bisheng.knowledge.domain.services.knowledge_space_service.KnowledgeDao.aquery_by_id',
-        new_callable=AsyncMock,
-        return_value=space,
-    ), patch(
-        'bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_find_member',
-        new_callable=AsyncMock,
-        return_value=None,
-    ), patch(
-        'bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_count_user_space_subscriptions',
-        new_callable=AsyncMock,
-        return_value=0,
-    ), patch(
-        'bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_insert_member',
-        new_callable=AsyncMock,
-    ) as mock_insert:
+    with (
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.KnowledgeDao.aquery_by_id",
+            new_callable=AsyncMock,
+            return_value=space,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.UserDepartmentDao.aget_user_primary_department",
+            new_callable=AsyncMock,
+            return_value=None,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_find_member",
+            new_callable=AsyncMock,
+            return_value=None,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.QuotaService.get_effective_quota",
+            new_callable=AsyncMock,
+            return_value=-1,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_insert_member",
+            new_callable=AsyncMock,
+        ) as mock_insert,
+    ):
        result = await service.subscribe_space(12)

-    assert result == {'status': 'pending', 'space_id': 12}
+    assert result == {"status": "pending", "space_id": 12}
    mock_insert.assert_awaited_once()
    inserted = mock_insert.await_args.args[0]
-    assert inserted.business_id == '12'
+    assert inserted.business_id == "12"
    assert inserted.user_role == UserRoleEnum.MEMBER
    assert inserted.status == MembershipStatusEnum.PENDING
    service.approval_gate.request_or_pass.assert_awaited_once()
    service.message_service.send_generic_approval.assert_not_awaited()


+@pytest.mark.asyncio
+async def test_approval_space_subscription_gate_failure_leaves_no_membership():
+    """When the approval scenario is missing/disabled the gate raises; the
+    membership must NOT be persisted.
+
+    Otherwise the leftover PENDING membership makes the early "already pending"
+    return short-circuit before the gate on the next click, masking the error
+    ("first click errors, second click succeeds"). With no scenario configured,
+    every click must fail consistently.
+    """
+    from bisheng.common.errcode.approval import ApprovalScenarioDisabledError
+    from bisheng.knowledge.domain.models.knowledge import AuthTypeEnum, KnowledgeTypeEnum
+
+    service = _load_service_class()(None, SimpleNamespace(user_id=42, user_name="alice", tenant_id=7))
+    service.approval_gate = SimpleNamespace(request_or_pass=AsyncMock(side_effect=ApprovalScenarioDisabledError()))
+
+    space = SimpleNamespace(
+        id=12, name="研发知识空间", type=KnowledgeTypeEnum.SPACE.value, auth_type=AuthTypeEnum.APPROVAL
+    )
+
+    with (
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.KnowledgeDao.aquery_by_id",
+            new_callable=AsyncMock,
+            return_value=space,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.UserDepartmentDao.aget_user_primary_department",
+            new_callable=AsyncMock,
+            return_value=None,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_find_member",
+            new_callable=AsyncMock,
+            return_value=None,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.QuotaService.get_effective_quota",
+            new_callable=AsyncMock,
+            return_value=-1,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_insert_member",
+            new_callable=AsyncMock,
+        ) as mock_insert,
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.update",
+            new_callable=AsyncMock,
+            side_effect=lambda row: row,
+        ) as mock_update,
+    ):
+        with pytest.raises(ApprovalScenarioDisabledError):
+            await service.subscribe_space(12)
+
+    service.approval_gate.request_or_pass.assert_awaited_once()
+    mock_insert.assert_not_awaited()
+    mock_update.assert_not_awaited()
+
+
@pytest.mark.asyncio
 async def test_approval_space_subscription_direct_pass_activates_member():
    from bisheng.common.models.space_channel_member import MembershipStatusEnum
    from bisheng.knowledge.domain.models.knowledge import AuthTypeEnum, KnowledgeTypeEnum

-    login_user = SimpleNamespace(user_id=42, user_name='alice', tenant_id=7)
+    login_user = SimpleNamespace(user_id=42, user_name="alice", tenant_id=7)
    service = _load_service_class()(None, login_user)
    service.approval_gate = SimpleNamespace(
-        request_or_pass=AsyncMock(return_value=SimpleNamespace(decision='pass', instance_id=22))
+        request_or_pass=AsyncMock(return_value=SimpleNamespace(decision="pass", instance_id=22))
+    )
+    space = SimpleNamespace(
+        id=12, name="研发知识空间", type=KnowledgeTypeEnum.SPACE.value, auth_type=AuthTypeEnum.APPROVAL
+    )
+    membership = SimpleNamespace(
+        id=9, business_id="12", user_id=42, user_role="member", status=MembershipStatusEnum.REJECTED
    )
-    space = SimpleNamespace(id=12, name='研发知识空间', type=KnowledgeTypeEnum.SPACE.value, auth_type=AuthTypeEnum.APPROVAL)
-    membership = SimpleNamespace(id=9, business_id='12', user_id=42, user_role='member', status=MembershipStatusEnum.REJECTED)

-    with patch(
-        'bisheng.knowledge.domain.services.knowledge_space_service.KnowledgeDao.aquery_by_id',
-        new_callable=AsyncMock,
-        return_value=space,
-    ), patch(
-        'bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_find_member',
-        new_callable=AsyncMock,
-        return_value=membership,
-    ), patch(
-        'bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_count_user_space_subscriptions',
-        new_callable=AsyncMock,
-        return_value=0,
-    ), patch(
-        'bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.update',
-        new_callable=AsyncMock,
-        side_effect=lambda row: row,
-    ) as mock_update, patch(
-        'bisheng.knowledge.domain.services.knowledge_space_service.KnowledgeSpaceService.sync_direct_space_user_permissions',
-        new_callable=AsyncMock,
-    ) as mock_sync:
+    with (
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.KnowledgeDao.aquery_by_id",
+            new_callable=AsyncMock,
+            return_value=space,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.UserDepartmentDao.aget_user_primary_department",
+            new_callable=AsyncMock,
+            return_value=None,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.async_find_member",
+            new_callable=AsyncMock,
+            return_value=membership,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.QuotaService.get_effective_quota",
+            new_callable=AsyncMock,
+            return_value=-1,
+        ),
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.SpaceChannelMemberDao.update",
+            new_callable=AsyncMock,
+            side_effect=lambda row: row,
+        ) as mock_update,
+        patch(
+            "bisheng.knowledge.domain.services.knowledge_space_service.KnowledgeSpaceService.sync_direct_space_user_permissions",
+            new_callable=AsyncMock,
+        ) as mock_sync,
+    ):
        result = await service.subscribe_space(12)

-    assert result == {'status': 'subscribed', 'space_id': 12}
+    assert result == {"status": "subscribed", "space_id": 12}
    assert membership.status == MembershipStatusEnum.ACTIVE
    mock_update.assert_awaited()
    mock_sync.assert_awaited()
@@ -98,18 +184,18 @@ async def test_knowledge_space_subscribe_scenario_handler_updates_membership_sta
    )
    from bisheng.common.models.space_channel_member import MembershipStatusEnum

-    membership = SimpleNamespace(id=1, status=MembershipStatusEnum.PENDING, user_id=42, user_role='member')
+    membership = SimpleNamespace(id=1, status=MembershipStatusEnum.PENDING, user_id=42, user_role="member")
    handler = KnowledgeSpaceSubscribeScenarioHandler(
        find_member=AsyncMock(return_value=membership),
        update_member=AsyncMock(side_effect=lambda row: row),
        sync_permissions=AsyncMock(),
    )

-    payload = {'space_id': 12, 'space_name': '研发知识空间', 'applicant_user_id': 42}
+    payload = {"space_id": 12, "space_name": "研发知识空间", "applicant_user_id": 42}
    await handler.on_approved(instance_id=1, payload_snapshot=payload)
    assert membership.status == MembershipStatusEnum.ACTIVE
    handler.sync_permissions.assert_awaited_once()

    membership.status = MembershipStatusEnum.PENDING
-    await handler.on_rejected(instance_id=1, payload_snapshot=payload, reason='reject')
+    await handler.on_rejected(instance_id=1, payload_snapshot=payload, reason="reject")
    assert membership.status == MembershipStatusEnum.REJECTED