From 9fa987a550e749e2593c56c4e637ab5d5b56f9ad Mon Sep 17 00:00:00 2001 From: wangxiaolei Date: Thu, 1 Oct 2026 02:04:16 +0000 Subject: [PATCH] fix: fix migrate permission not exclude admin and owner (#43311) --- api/commands/rbac.py | 40 +++++++++++++++-- .../test_legacy_model_type_migration.py | 3 ++ .../commands/test_rbac_agent_migration.py | 45 +++++++++++++++++-- 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/api/commands/rbac.py b/api/commands/rbac.py index ada2f9606d9..0d937152c4d 100644 --- a/api/commands/rbac.py +++ b/api/commands/rbac.py @@ -463,6 +463,36 @@ def _workspace_member_account_id_batches(tenant_id: str, batch_size: int) -> Ite last_join_id = str(rows[-1].id) +def _workspace_non_admin_account_id_batches( + tenant_id: str, + operator_account_id: str, + batch_size: int, +) -> Iterator[list[str]]: + """Yield workspace members except accounts bound to RBAC's builtin Owner/Admin roles. + + Resource administrators receive access implicitly through RBAC and their + resource access-policy bindings are locked. Resolve their current RBAC + bindings in batches instead of relying on the legacy workspace role column. + """ + for batch in _workspace_member_account_id_batches(tenant_id, batch_size): + member_roles = RBACService.MemberRoles.batch_get( + tenant_id=tenant_id, + account_id=operator_account_id, + member_account_ids=batch, + ) + protected_account_ids = { + member.account_id + for member in member_roles + if any( + role.is_builtin and role.category == "global_system_default" and role.role_tag in {"owner", "admin"} + for role in member.roles + ) + } + writable_batch = [account_id for account_id in batch if account_id not in protected_account_ids] + if writable_batch: + yield writable_batch + + def _replace_resource_whitelist( resource_type: str, *, @@ -725,7 +755,7 @@ def migrate_resource_whitelist_scopes_to_automatic_include( migrated_count += 1 if scope is RBACResourceWhitelistScope.ALL: - for batch in _workspace_member_account_id_batches(workspace_id, member_batch_size): + for batch in _workspace_non_admin_account_id_batches(workspace_id, operator_account_id, member_batch_size): _replace_resource_default_access_policies( current_resource_type, tenant_id=workspace_id, @@ -877,7 +907,7 @@ def migrate_only_me_resource_whitelist_scopes_to_automatic_include( ) migrated_count += 1 - for batch in _workspace_member_account_id_batches(workspace_id, member_batch_size): + for batch in _workspace_non_admin_account_id_batches(workspace_id, operator_account_id, member_batch_size): _replace_resource_default_access_policies( current_resource_type, tenant_id=workspace_id, @@ -1187,7 +1217,11 @@ def _report_backing_app_specific_whitelist(options: _AgentAccessBootstrapOptions def _write_agent_access_rows(options: _AgentAccessBootstrapOptions) -> None: - for batch in _workspace_member_account_id_batches(options.tenant_id, options.member_batch_size): + for batch in _workspace_non_admin_account_id_batches( + options.tenant_id, + options.operator_account_id, + options.member_batch_size, + ): RBACService.AgentAccess.replace_user_access_policies( tenant_id=options.tenant_id, account_id=options.operator_account_id, diff --git a/api/tests/unit_tests/commands/test_legacy_model_type_migration.py b/api/tests/unit_tests/commands/test_legacy_model_type_migration.py index 71d8e0f4f1c..ffdfd30c8db 100644 --- a/api/tests/unit_tests/commands/test_legacy_model_type_migration.py +++ b/api/tests/unit_tests/commands/test_legacy_model_type_migration.py @@ -526,6 +526,7 @@ def test_resource_whitelist_scope_migration_specific_preserves_existing_members( "replace_user_access_policies", lambda **kwargs: replace_policy_calls.append(kwargs), ) + monkeypatch.setattr(rbac_module.RBACService.MemberRoles, "batch_get", lambda **kwargs: []) command_module.migrate_resource_whitelist_scopes_to_automatic_include.callback( tenant_id=None, @@ -581,6 +582,7 @@ def test_resource_whitelist_scope_migration_all_syncs_workspace_members( "replace_user_access_policies", lambda **kwargs: replace_policy_calls.append(kwargs), ) + monkeypatch.setattr(rbac_module.RBACService.MemberRoles, "batch_get", lambda **kwargs: []) command_module.migrate_resource_whitelist_scopes_to_automatic_include.callback( tenant_id=None, @@ -636,6 +638,7 @@ def test_only_me_resource_whitelist_scope_migration_syncs_workspace_members( "replace_user_access_policies", lambda **kwargs: replace_policy_calls.append(kwargs), ) + monkeypatch.setattr(rbac_module.RBACService.MemberRoles, "batch_get", lambda **kwargs: []) command_module.migrate_only_me_resource_whitelist_scopes_to_automatic_include.callback( tenant_id=None, diff --git a/api/tests/unit_tests/commands/test_rbac_agent_migration.py b/api/tests/unit_tests/commands/test_rbac_agent_migration.py index 7a324e33034..c09c48855ca 100644 --- a/api/tests/unit_tests/commands/test_rbac_agent_migration.py +++ b/api/tests/unit_tests/commands/test_rbac_agent_migration.py @@ -11,6 +11,8 @@ from commands.rbac import migrate_agent_permissions_to_rbac from services.enterprise.rbac_service import ( LegacyAgentMigrationReport, LegacyAgentRoleMigration, + MemberRolesResponse, + RBACRole, _LegacyResourceWhitelistConfig, ) @@ -85,6 +87,7 @@ class _AgentPhaseSetup: agent_configs: list[_LegacyResourceWhitelistConfig] | None = None app_config: _LegacyResourceWhitelistConfig = field(default_factory=_whitelist_config) workspace_members: list[str] = field(default_factory=lambda: ["m1", "m2", "m3"]) + protected_members: list[str] = field(default_factory=list) owner_account_id: str = "owner-1" @@ -133,6 +136,30 @@ def _run_agent_phase(args: list[str], setup: _AgentPhaseSetup) -> tuple[Result, ), write_order=MagicMock(), ) + + def _member_roles( + *, tenant_id: str, account_id: str | None, member_account_ids: list[str] + ) -> list[MemberRolesResponse]: + del tenant_id, account_id + return [ + MemberRolesResponse( + account_id=account_id, + roles=[ + RBACRole( + id=f"role-{account_id}", + type="workspace", + category="global_system_default", + name=account_id, + is_builtin=True, + role_tag="owner" if account_id == "owner-1" else "admin", + ) + ], + ) + for account_id in member_account_ids + if account_id in setup.protected_members + ] + + stack.enter_context(patch(f"{MODULE}.RBACService.MemberRoles.batch_get", side_effect=_member_roles)) mocks.write_order.attach_mock(mocks.replace_user_access_policies, "seed_members") mocks.write_order.attach_mock(mocks.sync_creator_bindings, "sync_creator") mocks.write_order.attach_mock(mocks.replace_whitelist, "replace_whitelist") @@ -147,7 +174,11 @@ def _write_order(mocks: _AgentPhaseMocks) -> list[str]: def test_agent_bootstrap_apply_writes_whitelist_member_batches_and_creator_sync() -> None: result, mocks = _run_agent_phase( ["--apply", "--member-batch-size", "2"], - _AgentPhaseSetup(agents=[("ag1", "c1", None)], workspace_members=["m1", "m2", "m3"]), + _AgentPhaseSetup( + agents=[("ag1", "c1", None)], + workspace_members=["owner-1", "m1", "admin-1", "m2", "m3"], + protected_members=["owner-1", "admin-1"], + ), ) assert result.exit_code == 0, result.output @@ -160,9 +191,9 @@ def test_agent_bootstrap_apply_writes_whitelist_member_batches_and_creator_sync( assert mocks.replace_whitelist.call_args.kwargs["payload"].automatic_include_workspace_members is True mocks.member_batches.assert_called_once_with("t1", 2) - assert mocks.replace_user_access_policies.call_count == 2 + assert mocks.replace_user_access_policies.call_count == 3 calls = mocks.replace_user_access_policies.call_args_list - assert [call.kwargs["payload"].account_ids for call in calls] == [["m1", "m2"], ["m3"]] + assert [call.kwargs["payload"].account_ids for call in calls] == [["m1"], ["m2"], ["m3"]] assert all(call.kwargs["payload"].access_policy_ids == ["default"] for call in calls) assert all(call.kwargs["target_account_id"] is None for call in calls) @@ -171,7 +202,13 @@ def test_agent_bootstrap_apply_writes_whitelist_member_batches_and_creator_sync( assert mocks.sync_creator_bindings.call_args.kwargs["account_id"] == "c1" assert "1 agent(s) changed, 0 already initialised" in result.output - assert _write_order(mocks) == ["seed_members", "seed_members", "sync_creator", "replace_whitelist"] + assert _write_order(mocks) == [ + "seed_members", + "seed_members", + "seed_members", + "sync_creator", + "replace_whitelist", + ] def test_agent_bootstrap_is_idempotent_on_a_second_apply() -> None: