diff --git a/api/controllers/console/workspace/members.py b/api/controllers/console/workspace/members.py index c2533c9872..dfc4142085 100644 --- a/api/controllers/console/workspace/members.py +++ b/api/controllers/console/workspace/members.py @@ -70,6 +70,12 @@ register_schema_models( ) +def _is_role_enabled(role: TenantAccountRole | str, tenant_id: str) -> bool: + if role != TenantAccountRole.DATASET_OPERATOR: + return True + return FeatureService.get_features(tenant_id=tenant_id).dataset_operator_enabled + + @console_ns.route("/workspaces/current/members") class MemberListApi(Resource): """List all members of current tenant.""" @@ -110,6 +116,8 @@ class MemberInviteEmailApi(Resource): inviter = current_user if not inviter.current_tenant: raise ValueError("No current tenant") + if not _is_role_enabled(invitee_role, inviter.current_tenant.id): + return {"code": "invalid-role", "message": "Invalid role"}, 400 # Check workspace permission for member invitations from libs.workspace_permission import check_workspace_member_invite_permission @@ -208,6 +216,8 @@ class MemberUpdateRoleApi(Resource): current_user, _ = current_account_with_tenant() if not current_user.current_tenant: raise ValueError("No current tenant") + if not _is_role_enabled(new_role, current_user.current_tenant.id): + return {"code": "invalid-role", "message": "Invalid role"}, 400 member = db.session.get(Account, str(member_id)) if not member: abort(404) @@ -215,11 +225,17 @@ class MemberUpdateRoleApi(Resource): try: assert member is not None, "Member not found" TenantService.update_member_role(current_user.current_tenant, member, new_role, current_user) + except services.errors.account.CannotOperateSelfError as e: + return {"code": "cannot-operate-self", "message": str(e)}, 400 + except services.errors.account.NoPermissionError as e: + return {"code": "forbidden", "message": str(e)}, 403 + except services.errors.account.MemberNotInTenantError as e: + return {"code": "member-not-found", "message": str(e)}, 404 + except services.errors.account.RoleAlreadyAssignedError as e: + return {"code": "role-already-assigned", "message": str(e)}, 400 except Exception as e: raise ValueError(str(e)) - # todo: 403 - return {"result": "success"} diff --git a/api/services/account_service.py b/api/services/account_service.py index b6554a3de7..744c17d126 100644 --- a/api/services/account_service.py +++ b/api/services/account_service.py @@ -1280,8 +1280,8 @@ class TenantService: """Check member permission""" perms = { "add": [TenantAccountRole.OWNER, TenantAccountRole.ADMIN], - "remove": [TenantAccountRole.OWNER], - "update": [TenantAccountRole.OWNER], + "remove": [TenantAccountRole.OWNER, TenantAccountRole.ADMIN], + "update": [TenantAccountRole.OWNER, TenantAccountRole.ADMIN], } if action not in {"add", "remove", "update"}: raise InvalidActionError("Invalid action.") @@ -1299,6 +1299,15 @@ class TenantService: if not ta_operator or ta_operator.role not in perms[action]: raise NoPermissionError(f"No permission to {action} member.") + if action == "remove" and ta_operator.role == TenantAccountRole.ADMIN and member: + ta_member = db.session.scalar( + select(TenantAccountJoin) + .where(TenantAccountJoin.tenant_id == tenant.id, TenantAccountJoin.account_id == member.id) + .limit(1) + ) + if ta_member and ta_member.role == TenantAccountRole.OWNER: + raise NoPermissionError(f"No permission to {action} member.") + @staticmethod def remove_member_from_tenant(tenant: Tenant, account: Account, operator: Account): """Remove member from tenant. @@ -1370,6 +1379,7 @@ class TenantService: def update_member_role(tenant: Tenant, member: Account, new_role: str, operator: Account): """Update member role""" TenantService.check_member_permission(tenant, operator, member, "update") + new_tenant_role = TenantAccountRole(new_role) target_member_join = db.session.scalar( select(TenantAccountJoin) @@ -1380,6 +1390,11 @@ class TenantService: if not target_member_join: raise MemberNotInTenantError("Member not in tenant.") + operator_role = TenantService.get_user_role(operator, tenant) + target_role = TenantAccountRole(target_member_join.role) + if operator_role == TenantAccountRole.ADMIN and (TenantAccountRole.OWNER in {target_role, new_tenant_role}): + raise NoPermissionError("No permission to update member.") + if target_member_join.role == new_role: raise RoleAlreadyAssignedError("The provided role is already assigned to the member.") @@ -1394,7 +1409,7 @@ class TenantService: current_owner_join.role = TenantAccountRole.ADMIN # Update the role of the target member - target_member_join.role = TenantAccountRole(new_role) + target_member_join.role = new_tenant_role db.session.commit() @staticmethod diff --git a/api/tests/unit_tests/services/test_account_service.py b/api/tests/unit_tests/services/test_account_service.py index e9d2f1481e..02013392fc 100644 --- a/api/tests/unit_tests/services/test_account_service.py +++ b/api/tests/unit_tests/services/test_account_service.py @@ -13,6 +13,7 @@ from services.errors.account import ( AccountPasswordError, AccountRegisterError, CurrentPasswordIncorrectError, + NoPermissionError, ) @@ -817,8 +818,8 @@ class TestTenantService: # Mock the database queries in update_member_role method with patch("services.account_service.db") as mock_db: - # scalar calls: permission check, target member lookup - mock_db.session.scalar.side_effect = [mock_operator_join, mock_target_join] + # scalar calls: permission check, target member lookup, operator role lookup + mock_db.session.scalar.side_effect = [mock_operator_join, mock_target_join, mock_operator_join] # Execute test TenantService.update_member_role(mock_tenant, mock_member, "admin", mock_operator) @@ -827,6 +828,65 @@ class TestTenantService: assert mock_target_join.role == "admin" self._assert_database_operations_called(mock_db) + def test_admin_can_update_admin_member_role(self): + """Test admin can update another non-owner member, including an admin.""" + mock_tenant = MagicMock() + mock_tenant.id = "tenant-456" + mock_member = TestAccountAssociatedDataFactory.create_account_mock(account_id="member-789") + mock_operator = TestAccountAssociatedDataFactory.create_account_mock(account_id="operator-123") + mock_target_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="member-789", role="admin" + ) + mock_operator_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="operator-123", role="admin" + ) + + with patch("services.account_service.db") as mock_db: + mock_db.session.scalar.side_effect = [mock_operator_join, mock_target_join, mock_operator_join] + + TenantService.update_member_role(mock_tenant, mock_member, "editor", mock_operator) + + assert mock_target_join.role == "editor" + self._assert_database_operations_called(mock_db) + + def test_admin_cannot_update_owner_member_role(self): + """Test admin cannot update an owner member.""" + mock_tenant = MagicMock() + mock_tenant.id = "tenant-456" + mock_member = TestAccountAssociatedDataFactory.create_account_mock(account_id="member-789") + mock_operator = TestAccountAssociatedDataFactory.create_account_mock(account_id="operator-123") + mock_target_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="member-789", role="owner" + ) + mock_operator_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="operator-123", role="admin" + ) + + with patch("services.account_service.db") as mock_db: + mock_db.session.scalar.side_effect = [mock_operator_join, mock_target_join, mock_operator_join] + + with pytest.raises(NoPermissionError): + TenantService.update_member_role(mock_tenant, mock_member, "editor", mock_operator) + + def test_admin_cannot_promote_member_to_owner(self): + """Test admin cannot promote a non-owner member to owner.""" + mock_tenant = MagicMock() + mock_tenant.id = "tenant-456" + mock_member = TestAccountAssociatedDataFactory.create_account_mock(account_id="member-789") + mock_operator = TestAccountAssociatedDataFactory.create_account_mock(account_id="operator-123") + mock_target_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="member-789", role="admin" + ) + mock_operator_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="operator-123", role="admin" + ) + + with patch("services.account_service.db") as mock_db: + mock_db.session.scalar.side_effect = [mock_operator_join, mock_target_join, mock_operator_join] + + with pytest.raises(NoPermissionError): + TenantService.update_member_role(mock_tenant, mock_member, "owner", mock_operator) + # ==================== Permission Check Tests ==================== def test_check_member_permission_success(self, mock_db_dependencies): @@ -864,6 +924,39 @@ class TestTenantService: "add", ) + def test_admin_can_remove_non_owner_member(self, mock_db_dependencies): + """Test admin can remove 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_operator_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="operator-123", role="admin" + ) + mock_member_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="member-789", role="admin" + ) + mock_db_dependencies["db"].session.scalar.side_effect = [mock_operator_join, mock_member_join] + + TenantService.check_member_permission(mock_tenant, mock_operator, mock_member, "remove") + + def test_admin_cannot_remove_owner_member(self, mock_db_dependencies): + """Test admin cannot remove 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_operator_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="operator-123", role="admin" + ) + mock_member_join = TestAccountAssociatedDataFactory.create_tenant_join_mock( + tenant_id="tenant-456", account_id="member-789", role="owner" + ) + mock_db_dependencies["db"].session.scalar.side_effect = [mock_operator_join, mock_member_join] + + with pytest.raises(NoPermissionError): + TenantService.check_member_permission(mock_tenant, mock_operator, mock_member, "remove") + class TestRegisterService: """ diff --git a/web/app/components/header/account-setting/members-page/__tests__/index.spec.tsx b/web/app/components/header/account-setting/members-page/__tests__/index.spec.tsx index a6d490ff77..e129800487 100644 --- a/web/app/components/header/account-setting/members-page/__tests__/index.spec.tsx +++ b/web/app/components/header/account-setting/members-page/__tests__/index.spec.tsx @@ -52,7 +52,13 @@ vi.mock('../invited-modal', () => ({ ), })) vi.mock('../operation', () => ({ - default: () =>
Member Operation
, + default: ({ member }: { member: Member }) => ( +
+ Member Operation + {' '} + {member.role} +
+ ), })) vi.mock('../operation/transfer-ownership', () => ({ default: ({ onOperate }: { onOperate: () => void }) => , @@ -296,6 +302,37 @@ describe('MembersPage', () => { expect(screen.queryByRole('button', { name: /transfer ownership/i })).not.toBeInTheDocument() }) + it('should allow admins to operate other non-owner members only', () => { + vi.mocked(useAppContext).mockReturnValue({ + userProfile: { email: 'admin@example.com' }, + currentWorkspace: { name: 'Test Workspace', role: 'admin' } as ICurrentWorkspace, + isCurrentWorkspaceOwner: false, + isCurrentWorkspaceManager: true, + } as unknown as AppContextValue) + vi.mocked(useMembers).mockReturnValue({ + data: { + accounts: [ + mockAccounts[0], + mockAccounts[1], + { ...mockAccounts[1]!, id: '3', email: 'editor@example.com', name: 'Editor User', role: 'editor' }, + { ...mockAccounts[1]!, id: '4', email: 'normal@example.com', name: 'Normal User', role: 'normal' }, + { ...mockAccounts[1]!, id: '5', email: 'dataset@example.com', name: 'Dataset User', role: 'dataset_operator' }, + { ...mockAccounts[1]!, id: '6', email: 'other-admin@example.com', name: 'Other Admin User', role: 'admin' }, + ], + }, + refetch: mockRefetch, + } as unknown as ReturnType) + + renderMembersPage() + + expect(screen.getByText('Member Operation editor'))!.toBeInTheDocument() + expect(screen.getByText('Member Operation normal'))!.toBeInTheDocument() + expect(screen.getByText('Member Operation dataset_operator'))!.toBeInTheDocument() + expect(screen.getByText('Member Operation admin'))!.toBeInTheDocument() + expect(screen.getAllByText('common.members.admin')).toHaveLength(1) + expect(screen.queryByText('Member Operation owner')).not.toBeInTheDocument() + }) + it('should use created_at as fallback when last_active_at is empty', () => { const memberNoLastActive: Member = { ...mockAccounts[1]!, diff --git a/web/app/components/header/account-setting/members-page/index.tsx b/web/app/components/header/account-setting/members-page/index.tsx index da4440eb66..694d9ffeed 100644 --- a/web/app/components/header/account-setting/members-page/index.tsx +++ b/web/app/components/header/account-setting/members-page/index.tsx @@ -1,5 +1,5 @@ 'use client' -import type { InvitationResult } from '@/models/common' +import type { InvitationResult, Member } from '@/models/common' import { Avatar } from '@langgenius/dify-ui/avatar' import { Tooltip, TooltipContent, TooltipTrigger } from '@langgenius/dify-ui/tooltip' import { useSuspenseQuery } from '@tanstack/react-query' @@ -47,6 +47,12 @@ const MembersPage = () => { const isMemberFull = enableBilling && isNotUnlimitedMemberPlan && accounts.length >= plan.total.teamMembers const [editWorkspaceModalVisible, setEditWorkspaceModalVisible] = useState(false) const [showTransferOwnershipModal, setShowTransferOwnershipModal] = useState(false) + const canOperateMember = (account: Member) => { + if (isCurrentWorkspaceOwner) + return account.role !== 'owner' + + return currentWorkspace.role === 'admin' && account.role !== 'owner' && account.email !== userProfile.email + } return ( <> @@ -146,10 +152,10 @@ const MembersPage = () => { {isCurrentWorkspaceOwner && account.role === 'owner' && !isAllowTransferWorkspace && (
{RoleMap[account.role] || RoleMap.normal}
)} - {isCurrentWorkspaceOwner && account.role !== 'owner' && ( + {account.role !== 'owner' && canOperateMember(account) && ( )} - {!isCurrentWorkspaceOwner && ( + {account.role !== 'owner' && !canOperateMember(account) && (
{RoleMap[account.role] || RoleMap.normal}
)} diff --git a/web/app/components/header/account-setting/members-page/operation/__tests__/index.spec.tsx b/web/app/components/header/account-setting/members-page/operation/__tests__/index.spec.tsx index 8c9081833d..728ccf82e1 100644 --- a/web/app/components/header/account-setting/members-page/operation/__tests__/index.spec.tsx +++ b/web/app/components/header/account-setting/members-page/operation/__tests__/index.spec.tsx @@ -8,8 +8,8 @@ const mockUpdateMemberRole = vi.fn() const mockDeleteMemberOrCancelInvitation = vi.fn() vi.mock('@/service/common', () => ({ - deleteMemberOrCancelInvitation: () => mockDeleteMemberOrCancelInvitation(), - updateMemberRole: () => mockUpdateMemberRole(), + deleteMemberOrCancelInvitation: (args: unknown) => mockDeleteMemberOrCancelInvitation(args), + updateMemberRole: (args: unknown) => mockUpdateMemberRole(args), })) const mockUseProviderContext = vi.fn(() => ({ @@ -65,18 +65,21 @@ describe('Operation', () => { expect(await screen.findByText('common.members.datasetOperator')).toBeInTheDocument() }) - it('should show owner-allowed role options when operator role is admin', async () => { + it('should show admin-allowed role options when operator role is admin', async () => { const user = userEvent.setup() renderOperation({}, 'admin') await user.click(screen.getByText('common.members.editor')) - expect(screen.queryByText('common.members.admin')).not.toBeInTheDocument() + expect(screen.getByText('common.members.admin')).toBeInTheDocument() + expect(screen.getAllByText('common.members.editor')).toHaveLength(2) expect(screen.getByText('common.members.normal')).toBeInTheDocument() + expect(screen.queryByText('common.members.datasetOperator')).not.toBeInTheDocument() + expect(screen.getByText('common.members.removeFromTeam')).toBeInTheDocument() }) - it('should not show role options when operator role is unsupported', async () => { + it('should not show role options or remove action when operator role is unsupported', async () => { const user = userEvent.setup() renderOperation({}, 'normal') @@ -84,7 +87,7 @@ describe('Operation', () => { await user.click(screen.getByText('common.members.editor')) expect(screen.queryByText('common.members.normal')).not.toBeInTheDocument() - expect(screen.getByText('common.members.removeFromTeam')).toBeInTheDocument() + expect(screen.queryByText('common.members.removeFromTeam')).not.toBeInTheDocument() }) it('should call updateMemberRole and onOperate when selecting another role', async () => { @@ -96,7 +99,10 @@ describe('Operation', () => { await user.click(await screen.findByText('common.members.normal')) await waitFor(() => { - expect(mockUpdateMemberRole).toHaveBeenCalled() + expect(mockUpdateMemberRole).toHaveBeenCalledWith({ + url: '/workspaces/current/members/member-id/update-role', + body: { role: 'normal' }, + }) expect(onOperate).toHaveBeenCalled() }) }) @@ -109,7 +115,7 @@ describe('Operation', () => { await user.click(screen.getByText('common.members.editor')) expect(await screen.findByText('common.members.datasetOperator')).toBeInTheDocument() - expect(screen.queryByText('common.members.admin')).not.toBeInTheDocument() + expect(screen.getByText('common.members.admin')).toBeInTheDocument() }) it('should fall back to normal role label when member role is unknown', () => { @@ -127,7 +133,9 @@ describe('Operation', () => { await user.click(await screen.findByText('common.members.removeFromTeam')) await waitFor(() => { - expect(mockDeleteMemberOrCancelInvitation).toHaveBeenCalled() + expect(mockDeleteMemberOrCancelInvitation).toHaveBeenCalledWith({ + url: '/workspaces/current/members/member-id', + }) expect(onOperate).toHaveBeenCalled() }) }) diff --git a/web/app/components/header/account-setting/members-page/operation/index.tsx b/web/app/components/header/account-setting/members-page/operation/index.tsx index b67fb26bcd..7d2f609177 100644 --- a/web/app/components/header/account-setting/members-page/operation/index.tsx +++ b/web/app/components/header/account-setting/members-page/operation/index.tsx @@ -26,6 +26,9 @@ const roleI18nKeyMap = { dataset_operator: { label: 'members.datasetOperator', tip: 'members.datasetOperatorTip' }, } as const type OperationRoleKey = keyof typeof roleI18nKeyMap +const nonOwnerRoles = ['admin', 'editor', 'normal'] as const +const isNonOwnerRole = (role: Member['role']) => role !== 'owner' + const Operation = ({ member, operatorRole, onOperate }: IOperationProps) => { const [open, setOpen] = useState(false) const { t } = useTranslation() @@ -48,13 +51,13 @@ const Operation = ({ member, operatorRole, onOperate }: IOperationProps) => { } if (operatorRole === 'admin') { return [ - 'editor', - 'normal', + ...nonOwnerRoles, ...(datasetOperatorEnabled ? ['dataset_operator'] as const : []), ] } return [] }, [operatorRole, datasetOperatorEnabled]) + const canRemoveMember = operatorRole === 'owner' || (operatorRole === 'admin' && isNonOwnerRole(member.role)) const handleDeleteMemberOrCancelInvitation = async () => { setOpen(false) try { @@ -81,7 +84,7 @@ const Operation = ({ member, operatorRole, onOperate }: IOperationProps) => { return ( } + render={