From c09ea2fc3bd9e1db03df7105bb148960e48e4519 Mon Sep 17 00:00:00 2001 From: fatelei Date: Fri, 29 May 2026 10:16:31 +0800 Subject: [PATCH] fix: fix permission check --- .gitignore | 1 + api/controllers/openapi/workspaces.py | 1 + api/services/account_service.py | 45 ++++++++++-- api/services/dataset_service.py | 20 +++++- .../services/test_account_service.py | 58 +++++++++++++++ .../services/test_dataset_service_dataset.py | 72 +++++++++++++++++++ 6 files changed, 190 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 5b434ee4ec..bb2a151292 100644 --- a/.gitignore +++ b/.gitignore @@ -259,3 +259,4 @@ scripts/stress-test/reports/ .qoder/* .context/ .eslintcache +node_modules/ diff --git a/api/controllers/openapi/workspaces.py b/api/controllers/openapi/workspaces.py index b23012a810..8888a65311 100644 --- a/api/controllers/openapi/workspaces.py +++ b/api/controllers/openapi/workspaces.py @@ -1,3 +1,4 @@ + """User-scoped workspace reads and member management under /openapi/v1/workspaces. Bearer-authed counterparts to the cookie-authed /console/api/workspaces diff --git a/api/services/account_service.py b/api/services/account_service.py index 65b3773023..dedfe59051 100644 --- a/api/services/account_service.py +++ b/api/services/account_service.py @@ -193,6 +193,23 @@ class AccountService: role_ids=[resolved_role_id], ) + @staticmethod + def get_workspace_permission_keys(tenant_id: str, account_id: str) -> set[str]: + permissions = RBACService.MyPermissions.get(tenant_id, account_id) + return set(getattr(getattr(permissions, "workspace", None), "permission_keys", []) or []) + + @staticmethod + def is_rbac_workspace_owner(tenant_id: str, actor_account_id: str, member_account_id: str) -> bool: + roles = RBACService.MemberRoles.get( + tenant_id=tenant_id, + account_id=actor_account_id, + member_account_id=member_account_id, + ).roles + return any( + role.is_builtin and role.category == "global_system_default" and role.name in {"所有者", "owner"} + for role in roles + ) + @staticmethod def _get_refresh_token_key(refresh_token: str) -> str: return f"{REFRESH_TOKEN_PREFIX}{refresh_token}" @@ -1538,11 +1555,6 @@ class TenantService: @staticmethod def check_member_permission(tenant: Tenant, operator: Account, member: Account | None, action: str): """Check member permission""" - perms = { - "add": [TenantAccountRole.OWNER, TenantAccountRole.ADMIN], - "remove": [TenantAccountRole.OWNER, TenantAccountRole.ADMIN], - "update": [TenantAccountRole.OWNER, TenantAccountRole.ADMIN], - } if action not in {"add", "remove", "update"}: raise InvalidActionError("Invalid action.") @@ -1550,6 +1562,29 @@ class TenantService: if operator.id == member.id: raise CannotOperateSelfError("Cannot operate self.") + if dify_config.RBAC_ENABLED: + workspace_permission_keys = AccountService.get_workspace_permission_keys( + str(tenant.id), + str(operator.id), + ) + required_permission_key = ( + "workspace.member.manage" if action in {"add", "remove"} else "workspace.role.manage" + ) + if required_permission_key not in workspace_permission_keys: + raise NoPermissionError(f"No permission to {action} member.") + + if action == "remove" and member and AccountService.is_rbac_workspace_owner( + str(tenant.id), str(operator.id), str(member.id) + ): + raise NoPermissionError(f"No permission to {action} member.") + return + + perms = { + "add": [TenantAccountRole.OWNER, TenantAccountRole.ADMIN], + "remove": [TenantAccountRole.OWNER, TenantAccountRole.ADMIN], + "update": [TenantAccountRole.OWNER, TenantAccountRole.ADMIN], + } + ta_operator = db.session.scalar( select(TenantAccountJoin) .where(TenantAccountJoin.tenant_id == tenant.id, TenantAccountJoin.account_id == operator.id) diff --git a/api/services/dataset_service.py b/api/services/dataset_service.py index 2e593ea71f..21976ad421 100644 --- a/api/services/dataset_service.py +++ b/api/services/dataset_service.py @@ -67,6 +67,7 @@ from models.source import DataSourceOauthBinding from models.workflow import Workflow from services.document_indexing_proxy.document_indexing_task_proxy import DocumentIndexingTaskProxy from services.document_indexing_proxy.duplicate_document_indexing_task_proxy import DuplicateDocumentIndexingTaskProxy +from services.enterprise import rbac_service as enterprise_rbac_service from services.entities.knowledge_entities.knowledge_entities import ( ChildChunkUpdateArgs, KnowledgeConfig, @@ -229,6 +230,15 @@ class _EstimateArgs(BaseModel): class DatasetService: + @staticmethod + def _can_manage_all_datasets(tenant_id: str, account_id: str) -> bool: + if not dify_config.RBAC_ENABLED: + return False + + permissions = enterprise_rbac_service.RBACService.MyPermissions.get(tenant_id, account_id) + workspace_permission_keys = getattr(getattr(permissions, "workspace", None), "permission_keys", []) or [] + return "dataset.create_and_management" in workspace_permission_keys + @staticmethod def get_datasets(page, per_page, tenant_id=None, user=None, search=None, tag_ids=None, include_all=False): query = select(Dataset).where(Dataset.tenant_id == tenant_id).order_by(Dataset.created_at.desc(), Dataset.id) @@ -242,7 +252,7 @@ class DatasetService: ).all() permitted_dataset_ids = {dp.dataset_id for dp in dataset_permission} if dataset_permission else None - if user.current_role == TenantAccountRole.DATASET_OPERATOR: + if not dify_config.RBAC_ENABLED and user.current_role == TenantAccountRole.DATASET_OPERATOR: # only show datasets that the user has permission to access # Check if permitted_dataset_ids is not empty to avoid WHERE false condition if permitted_dataset_ids and len(permitted_dataset_ids) > 0: @@ -250,7 +260,13 @@ class DatasetService: else: return [], 0 else: - if user.current_role != TenantAccountRole.OWNER or not include_all: + if dify_config.RBAC_ENABLED: + can_manage_all_datasets = DatasetService._can_manage_all_datasets(str(tenant_id), str(user.id)) + should_show_all_datasets = include_all and can_manage_all_datasets + else: + should_show_all_datasets = user.current_role == TenantAccountRole.OWNER and include_all + + if not should_show_all_datasets: # show all datasets that the user has permission to access # Check if permitted_dataset_ids is not empty to avoid WHERE false condition if permitted_dataset_ids and len(permitted_dataset_ids) > 0: diff --git a/api/tests/unit_tests/services/test_account_service.py b/api/tests/unit_tests/services/test_account_service.py index c529e80289..c2c09ff5bd 100644 --- a/api/tests/unit_tests/services/test_account_service.py +++ b/api/tests/unit_tests/services/test_account_service.py @@ -1061,6 +1061,64 @@ class TestTenantService: with pytest.raises(NoPermissionError): TenantService.check_member_permission(mock_tenant, mock_operator, mock_member, "remove") + def test_rbac_member_can_remove_non_owner_member(self): + """Test RBAC workspace.member.manage allows removing a non-owner member.""" + mock_tenant = MagicMock() + mock_tenant.id = "tenant-456" + mock_operator = TestAccountAssociatedDataFactory.create_account_mock(account_id="operator-123") + mock_member = TestAccountAssociatedDataFactory.create_account_mock(account_id="member-789") + + mock_permissions = MagicMock() + mock_permissions.workspace = MagicMock(permission_keys=["workspace.member.manage"]) + + with ( + patch("services.account_service.dify_config.RBAC_ENABLED", True), + patch("services.account_service.RBACService.MyPermissions.get", return_value=mock_permissions), + ): + TenantService.check_member_permission(mock_tenant, mock_operator, mock_member, "remove") + + def test_rbac_member_cannot_remove_without_permission(self): + """Test RBAC permission check rejects removal without workspace.member.manage.""" + mock_tenant = MagicMock() + mock_tenant.id = "tenant-456" + mock_operator = TestAccountAssociatedDataFactory.create_account_mock(account_id="operator-123") + mock_member = TestAccountAssociatedDataFactory.create_account_mock(account_id="member-789") + + mock_permissions = MagicMock() + mock_permissions.workspace = MagicMock(permission_keys=["workspace.role.manage"]) + + with ( + patch("services.account_service.dify_config.RBAC_ENABLED", True), + patch("services.account_service.RBACService.MyPermissions.get", return_value=mock_permissions), + ): + with pytest.raises(NoPermissionError): + TenantService.check_member_permission(mock_tenant, mock_operator, mock_member, "remove") + + def test_rbac_member_cannot_remove_owner_member(self): + """Test RBAC permission check rejects removing an owner member.""" + mock_tenant = MagicMock() + mock_tenant.id = "tenant-456" + mock_operator = TestAccountAssociatedDataFactory.create_account_mock(account_id="operator-123") + mock_member = TestAccountAssociatedDataFactory.create_account_mock(account_id="member-789") + + mock_permissions = MagicMock() + mock_permissions.workspace = MagicMock(permission_keys=["workspace.member.manage"]) + + mock_owner_role = MagicMock() + mock_owner_role.is_builtin = True + mock_owner_role.category = "global_system_default" + mock_owner_role.name = "所有者" + mock_member_roles = MagicMock() + mock_member_roles.roles = [mock_owner_role] + + with ( + patch("services.account_service.dify_config.RBAC_ENABLED", True), + patch("services.account_service.RBACService.MyPermissions.get", return_value=mock_permissions), + patch("services.account_service.RBACService.MemberRoles.get", return_value=mock_member_roles), + ): + with pytest.raises(NoPermissionError): + TenantService.check_member_permission(mock_tenant, mock_operator, mock_member, "remove") + class TestRegisterService: """ diff --git a/api/tests/unit_tests/services/test_dataset_service_dataset.py b/api/tests/unit_tests/services/test_dataset_service_dataset.py index 3d08b6fd09..c7ca8c8a27 100644 --- a/api/tests/unit_tests/services/test_dataset_service_dataset.py +++ b/api/tests/unit_tests/services/test_dataset_service_dataset.py @@ -15,6 +15,7 @@ from .dataset_service_test_helpers import ( ProviderTokenNotInitError, RagPipelineDatasetCreateEntity, SimpleNamespace, + TenantAccountRole, _make_knowledge_configuration, _make_retrieval_model, _make_session_context, @@ -167,6 +168,77 @@ class TestDatasetServiceValidation: DatasetService.check_is_multimodal_model("tenant-1", "provider", "embedding-model") +class TestDatasetServiceRetrievalPermissions: + """Unit tests for dataset list permission branching.""" + + def test_get_datasets_rbac_include_all_uses_workspace_permission(self): + mock_db = MagicMock() + mock_db.session.scalars.return_value.all.return_value = [] + mock_db.paginate.return_value.items = [] + mock_db.paginate.return_value.total = 0 + + user = DatasetServiceUnitDataFactory.create_user_mock(role=TenantAccountRole.NORMAL) + mock_permissions = SimpleNamespace(workspace=SimpleNamespace(permission_keys=["dataset.create_and_management"])) + + with ( + patch("services.dataset_service.db", mock_db), + patch("services.dataset_service.dify_config.RBAC_ENABLED", True), + patch( + "services.dataset_service.enterprise_rbac_service.RBACService.MyPermissions.get", + return_value=mock_permissions, + ), + ): + DatasetService.get_datasets(page=1, per_page=20, tenant_id="tenant-1", user=user, include_all=True) + + mock_db.session.scalars.assert_called_once() + mock_db.paginate.assert_called_once() + select_stmt = mock_db.paginate.call_args.kwargs["select"] + assert len(select_stmt._where_criteria) == 1 + + def test_get_datasets_rbac_include_all_falls_back_without_workspace_permission(self): + mock_db = MagicMock() + mock_db.session.scalars.return_value.all.return_value = [] + mock_db.paginate.return_value.items = [] + mock_db.paginate.return_value.total = 0 + + user = DatasetServiceUnitDataFactory.create_user_mock(role=TenantAccountRole.NORMAL) + mock_permissions = SimpleNamespace(workspace=SimpleNamespace(permission_keys=[])) + + with ( + patch("services.dataset_service.db", mock_db), + patch("services.dataset_service.dify_config.RBAC_ENABLED", True), + patch( + "services.dataset_service.enterprise_rbac_service.RBACService.MyPermissions.get", + return_value=mock_permissions, + ), + ): + DatasetService.get_datasets(page=1, per_page=20, tenant_id="tenant-1", user=user, include_all=True) + + mock_db.session.scalars.assert_called_once() + mock_db.paginate.assert_called_once() + select_stmt = mock_db.paginate.call_args.kwargs["select"] + assert len(select_stmt._where_criteria) == 2 + + def test_get_datasets_legacy_owner_include_all_keeps_full_access(self): + mock_db = MagicMock() + mock_db.session.scalars.return_value.all.return_value = [] + mock_db.paginate.return_value.items = [] + mock_db.paginate.return_value.total = 0 + + user = DatasetServiceUnitDataFactory.create_user_mock(role=TenantAccountRole.OWNER) + + with ( + patch("services.dataset_service.db", mock_db), + patch("services.dataset_service.dify_config.RBAC_ENABLED", False), + ): + DatasetService.get_datasets(page=1, per_page=20, tenant_id="tenant-1", user=user, include_all=True) + + mock_db.session.scalars.assert_called_once() + mock_db.paginate.assert_called_once() + select_stmt = mock_db.paginate.call_args.kwargs["select"] + assert len(select_stmt._where_criteria) == 1 + + class TestDatasetServiceCreationAndUpdate: """Unit tests for dataset creation and update helpers."""