From c1e32b725095ac58e1a1afe3257ea94b95aec406 Mon Sep 17 00:00:00 2001 From: yaojin Date: Fri, 21 Aug 2026 05:05:32 -0700 Subject: [PATCH] fix(permission): tool manager must not inherit manage_tool_owner (IKABS3) The tool permission template's `default_permission_ids_for_relation` used a generic level-based calculation that handed a manager the `manage_tool_owner` permission (same can_manage level as `manage_tool_manager`). The authorization service then accepted a manager's revoke of an owner-tier grant, and the permission dialog's "manage" dropdown surfaced the owner row's delete action, so a manager could remove an owner's permission on a tool. Switch the tool template to an explicit per-tier default (mirroring channel_permission_template) so a manager only inherits `manage_tool_manager` and `manage_tool_viewer`. The owner tier still receives all three `manage_tool_*` permissions, and the underlying `_can_grant_model` check is unchanged. Update the related default test and add a regression test that pins both the manager's permission set and the authorize-side guard. Refs: gitee IKABS3 --- .../domain/tool_permission_template.py | 63 +++++++++++-------- .../test_permission_relation_defaults.py | 9 +++ .../test_resource_authorization_service.py | 36 +++++++++++ 3 files changed, 83 insertions(+), 25 deletions(-) diff --git a/src/backend/bisheng/permission/domain/tool_permission_template.py b/src/backend/bisheng/permission/domain/tool_permission_template.py index 6eb564d4de..3d78e6d1fd 100644 --- a/src/backend/bisheng/permission/domain/tool_permission_template.py +++ b/src/backend/bisheng/permission/domain/tool_permission_template.py @@ -4,24 +4,37 @@ from typing import Dict, List, Set -_RELATION_LEVEL: Dict[str, int] = { - 'can_read': 1, - 'can_edit': 2, - 'can_manage': 3, - 'can_delete': 4, -} - -_MODEL_LEVEL: Dict[str, int] = { - 'viewer': 1, - 'editor': 2, - 'manager': 3, - 'owner': 4, -} -_COMPUTED_TO_MODEL_RELATION: Dict[str, str] = { - 'can_read': 'viewer', - 'can_edit': 'editor', - 'can_manage': 'manager', - 'can_delete': 'owner', +# NOTE: keep these tier lists in lockstep with the channel template +# (channel_permission_template._DEFAULT_PERMISSION_IDS_BY_RELATION) so a +# "manager" subject never inherits the "manage_*_owner" permission by default — +# that would let a manager add or remove owners, breaking the owner/manager +# hierarchy. The previous level-based calculation gave can_manage-level managers +# the owner-tier manage permission as well (IKABS3). +_DEFAULT_PERMISSION_IDS_BY_RELATION: Dict[str, Set[str]] = { + 'owner': { + 'view_tool', + 'use_tool', + 'edit_tool', + 'delete_tool', + 'manage_tool_owner', + 'manage_tool_manager', + 'manage_tool_viewer', + }, + 'manager': { + 'view_tool', + 'use_tool', + 'edit_tool', + 'manage_tool_manager', + 'manage_tool_viewer', + }, + 'editor': { + 'view_tool', + 'use_tool', + 'edit_tool', + }, + 'viewer': { + 'view_tool', + }, } TOOL_PERMISSION_TEMPLATE: dict = { @@ -61,10 +74,10 @@ def tool_template_permissions() -> List[dict]: def default_permission_ids_for_relation(relation: str) -> Set[str]: - normalized = _COMPUTED_TO_MODEL_RELATION.get(relation, relation) - relation_level = _MODEL_LEVEL.get(normalized, 0) - return { - item['id'] - for item in tool_template_permissions() - if relation_level >= _RELATION_LEVEL.get(item['relation'], 99) - } + """System-model default permissions for owner/manager/editor/viewer. + + A manager is intentionally denied ``manage_tool_owner`` so the manager + cannot add or remove owners — only the owner can. This mirrors the + channel and application templates. + """ + return set(_DEFAULT_PERMISSION_IDS_BY_RELATION.get(relation, set())) diff --git a/src/backend/test/permission/test_permission_relation_defaults.py b/src/backend/test/permission/test_permission_relation_defaults.py index 385c379337..0b128254a1 100644 --- a/src/backend/test/permission/test_permission_relation_defaults.py +++ b/src/backend/test/permission/test_permission_relation_defaults.py @@ -33,11 +33,20 @@ def test_application_permission_defaults_accept_computed_relations(): def test_tool_permission_defaults_accept_computed_relations(): + # IKABS3: manager must not inherit manage_tool_owner — only the owner tier + # is allowed to add or remove owners. assert default_tool_permission_ids_for_relation("can_read") == {"view_tool", "use_tool"} assert default_tool_permission_ids_for_relation("can_manage") == { "view_tool", "use_tool", "edit_tool", + "manage_tool_manager", + "manage_tool_viewer", + } + assert "manage_tool_owner" not in default_tool_permission_ids_for_relation("can_manage") + # owner gets every permission + assert default_tool_permission_ids_for_relation("can_delete") >= { + "delete_tool", "manage_tool_owner", "manage_tool_manager", "manage_tool_viewer", diff --git a/src/backend/test/permission/test_resource_authorization_service.py b/src/backend/test/permission/test_resource_authorization_service.py index 3277d3f56a..e89547fb30 100644 --- a/src/backend/test/permission/test_resource_authorization_service.py +++ b/src/backend/test/permission/test_resource_authorization_service.py @@ -221,3 +221,39 @@ async def test_default_service_reads_relation_models_and_bindings_from_domain_st read_models.assert_awaited_once() read_bindings.assert_awaited_once() + + +# IKABS3: a tool manager must not be able to revoke an owner-tier grant. The +# earlier level-based default gave managers manage_tool_owner (same relation +# level as manage_tool_manager) so the authorization service approved the +# revoke; the fix moves tool defaults to an explicit per-tier table that omits +# manage_tool_owner for managers. +async def test_manager_cannot_revoke_tool_owner_grant(): + from bisheng.permission.domain.services.resource_authorization_service import ( + _can_grant_model, + ) + from bisheng.permission.domain.tool_permission_template import ( + default_permission_ids_for_relation, + ) + + manager_permissions = default_permission_ids_for_relation("manager") + # Regression check: a manager on a tool never inherits manage_tool_owner. + assert "manage_tool_owner" not in manager_permissions + + owner_model = { + "id": "owner", + "name": "所有者", + "relation": "owner", + "grant_tier": "owner", + "permissions": [], + "permissions_explicit": False, + "is_system": True, + } + # The revoke check: even if a UI bug forwards the call, the backend must + # refuse to grant/revoke an owner-tier relation when the caller only has + # the manager-tier permission. + assert _can_grant_model("tool", "owner", owner_model, manager_permissions) is False + + # Sanity: an owner still passes the check. + owner_permissions = default_permission_ids_for_relation("owner") + assert _can_grant_model("tool", "owner", owner_model, owner_permissions) is True