diff --git a/.claude/skills/approval-module/SKILL.md b/.claude/skills/approval-module/SKILL.md index f5dc0d43e..00fc280ab 100644 --- a/.claude/skills/approval-module/SKILL.md +++ b/.claude/skills/approval-module/SKILL.md @@ -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"的早退分支短路，掩盖错误（首次报错、二次假成功）。无场景时每次点击都应一致报错。 --- diff --git a/src/backend/bisheng/knowledge/domain/services/knowledge_space_service.py b/src/backend/bisheng/knowledge/domain/services/knowledge_space_service.py index bd6b16695..0c3742609 100644 --- a/src/backend/bisheng/knowledge/domain/services/knowledge_space_service.py +++ b/src/backend/bisheng/knowledge/domain/services/knowledge_space_service.py @@ -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( diff --git a/src/backend/test/approval/test_knowledge_space_subscription_approval_integration.py b/src/backend/test/approval/test_knowledge_space_subscription_approval_integration.py index f8bf428db..e23c7a18a 100644 --- a/src/backend/test/approval/test_knowledge_space_subscription_approval_integration.py +++ b/src/backend/test/approval/test_knowledge_space_subscription_approval_integration.py @@ -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