chore: admin also has the permission of changing role (#36069)

Co-authored-by: Yansong Zhang <916125788@qq.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This commit is contained in:
Joel 2026-05-12 18:15:05 +08:00 committed by GitHub
parent c26be9d3f4
commit 680ef077ae
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 217 additions and 35 deletions

View File

@ -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"}

View File

@ -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

View File

@ -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:
"""

View File

@ -52,7 +52,13 @@ vi.mock('../invited-modal', () => ({
),
}))
vi.mock('../operation', () => ({
default: () => <div>Member Operation</div>,
default: ({ member }: { member: Member }) => (
<div>
Member Operation
{' '}
{member.role}
</div>
),
}))
vi.mock('../operation/transfer-ownership', () => ({
default: ({ onOperate }: { onOperate: () => void }) => <button onClick={onOperate}>Transfer ownership</button>,
@ -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<typeof useMembers>)
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]!,

View File

@ -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 && (
<div className="px-3 system-sm-regular text-text-secondary">{RoleMap[account.role] || RoleMap.normal}</div>
)}
{isCurrentWorkspaceOwner && account.role !== 'owner' && (
{account.role !== 'owner' && canOperateMember(account) && (
<Operation member={account} operatorRole={currentWorkspace.role} onOperate={refetch} />
)}
{!isCurrentWorkspaceOwner && (
{account.role !== 'owner' && !canOperateMember(account) && (
<div className="px-3 system-sm-regular text-text-secondary">{RoleMap[account.role] || RoleMap.normal}</div>
)}
</div>

View File

@ -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()
})
})

View File

@ -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 (
<DropdownMenu open={open} onOpenChange={setOpen}>
<DropdownMenuTrigger
render={<div className={cn('group flex h-full w-full cursor-pointer items-center justify-between px-3 system-sm-regular text-text-secondary hover:bg-state-base-hover', open && 'bg-state-base-hover')} />}
render={<button type="button" className={cn('group flex h-full w-full cursor-pointer items-center justify-between border-none bg-transparent px-3 text-left system-sm-regular text-text-secondary hover:bg-state-base-hover', open && 'bg-state-base-hover')} />}
>
{RoleMap[member.role] || RoleMap.normal}
<span aria-hidden className={cn('i-ri-arrow-down-s-line h-4 w-4 shrink-0 group-hover:block', open ? 'block' : 'hidden')} />
@ -108,19 +111,23 @@ const Operation = ({ member, operatorRole, onOperate }: IOperationProps) => {
</DropdownMenuItem>
))}
</div>
<DropdownMenuSeparator className="my-0" />
<div className="p-1">
<DropdownMenuItem
className="h-auto items-start gap-2 rounded-lg px-3 py-2"
onClick={handleDeleteMemberOrCancelInvitation}
>
<span aria-hidden className="mt-[2px] h-4 w-4 shrink-0" />
<div>
<div className="system-sm-semibold whitespace-nowrap text-text-secondary">{t('members.removeFromTeam', { ns: 'common' })}</div>
<div className="system-xs-regular whitespace-nowrap text-text-tertiary">{t('members.removeFromTeamTip', { ns: 'common' })}</div>
{canRemoveMember && (
<>
<DropdownMenuSeparator className="my-0" />
<div className="p-1">
<DropdownMenuItem
className="h-auto items-start gap-2 rounded-lg px-3 py-2"
onClick={handleDeleteMemberOrCancelInvitation}
>
<span aria-hidden className="mt-[2px] h-4 w-4 shrink-0" />
<div>
<div className="system-sm-semibold whitespace-nowrap text-text-secondary">{t('members.removeFromTeam', { ns: 'common' })}</div>
<div className="system-xs-regular whitespace-nowrap text-text-tertiary">{t('members.removeFromTeamTip', { ns: 'common' })}</div>
</div>
</DropdownMenuItem>
</div>
</DropdownMenuItem>
</div>
</>
)}
</DropdownMenuContent>
</DropdownMenu>
)