diff --git a/apps/data-management/e2e/specs/workspaces.spec.ts b/apps/data-management/e2e/specs/workspaces.spec.ts index 726821ff3..a2ece3d61 100644 --- a/apps/data-management/e2e/specs/workspaces.spec.ts +++ b/apps/data-management/e2e/specs/workspaces.spec.ts @@ -306,7 +306,7 @@ test.describe('workspace management', () => { ).toHaveCount(0) }) - test('editor controls follow resource permissions instead of ownership', async ({ + test('editor can edit workspace contents but cannot manage the workspace itself', async ({ page, }) => { await authenticateSession(page, users.editor.email, users.editor.password) @@ -320,12 +320,27 @@ test.describe('workspace management', () => { ) await expect( workspaceItem.locator('[data-testid^="workspace-edit-"]') - ).toBeEnabled() + ).toBeDisabled() await expect( workspaceItem.locator('[data-testid^="workspace-delete-"]') + ).toBeDisabled() + await expect(page.getByLabel('Make this workspace private')).toBeDisabled() + await expect(page.getByRole('tab', { name: 'Ownership' })).toHaveCount(0) + + await page.getByRole('tab', { name: 'Metadata' }).click() + await page.getByTestId('metadata-scope-workspace').click() + await page.getByRole('tab', { name: 'Methods' }).click() + await expect(page.getByTestId('add-workspace-metadata-item')).toBeEnabled() + await expect( + page.getByTestId( + `edit-metadata-${fixtures.metadata.privateAssignedMethod.id}` + ) + ).toBeEnabled() + await expect( + page.getByTestId( + `delete-metadata-${fixtures.metadata.privateAssignedMethod.id}` + ) ).toBeEnabled() - await expect(page.getByLabel('Make this workspace private')).toBeEnabled() - await expect(page.getByRole('tab', { name: 'Ownership' })).toBeVisible() }) test('workspace selection carries into the rest of the management app', async ({ diff --git a/apps/data-management/src/composables/__tests__/useWorkspacePermissions.spec.ts b/apps/data-management/src/composables/__tests__/useWorkspacePermissions.spec.ts index f08672703..a18443827 100644 --- a/apps/data-management/src/composables/__tests__/useWorkspacePermissions.spec.ts +++ b/apps/data-management/src/composables/__tests__/useWorkspacePermissions.spec.ts @@ -67,6 +67,43 @@ describe('useWorkspacePermissions', () => { ).toBe(true) }) + it.each([PermissionResource.Global, PermissionResource.Workspace])( + 'reserves workspace management for owners and admins despite %s grants', + (resource) => { + const workspace = workspaceWithPermissions( + Object.values(PermissionAction).map((action) => ({ resource, action })) + ) + const { hasPermission } = useWorkspacePermissions() + + expect( + hasPermission( + PermissionResource.Workspace, + PermissionAction.View, + workspace + ) + ).toBe(true) + for (const action of [PermissionAction.Edit, PermissionAction.Delete]) { + expect( + hasPermission(PermissionResource.Workspace, action, workspace) + ).toBe(false) + + useUserStore().user.email = 'owner@example.com' + expect( + hasPermission(PermissionResource.Workspace, action, workspace) + ).toBe(true) + + useUserStore().user.email = 'admin@example.com' + useUserStore().user.isSuperuser = true + expect( + hasPermission(PermissionResource.Workspace, action, workspace) + ).toBe(true) + + useUserStore().user.isSuperuser = false + useUserStore().user.email = 'viewer@example.com' + } + } + ) + it('applies a resource wildcard only to the explicitly granted action', () => { const workspace = workspaceWithPermissions([ { diff --git a/apps/data-management/src/composables/useWorkspacePermissions.ts b/apps/data-management/src/composables/useWorkspacePermissions.ts index a71ab65ec..98beaf158 100644 --- a/apps/data-management/src/composables/useWorkspacePermissions.ts +++ b/apps/data-management/src/composables/useWorkspacePermissions.ts @@ -53,6 +53,13 @@ export function useWorkspacePermissions( if (!w) return false if (isOwner(w) || isAdmin()) return true + if ( + resource === PermissionResource.Workspace && + [PermissionAction.Edit, PermissionAction.Delete].includes(action) + ) { + return false + } + const perms = w.collaboratorRole?.permissions ?? [] return perms.some( (permission) => diff --git a/django/core/iam/permissions/mixins.py b/django/core/iam/permissions/mixins.py index 7c213b424..1f3b97d66 100644 --- a/django/core/iam/permissions/mixins.py +++ b/django/core/iam/permissions/mixins.py @@ -87,7 +87,8 @@ def can_edit(self, resource: object) -> bool: Superusers are granted access without a workspace lookup. For all others, the resource's registered workspace_field is used to resolve the workspace - before checking collaborator permissions. + before checking permissions. Editing the workspace itself requires ownership; + collaborator permissions only grant edits to its contents. """ if "can_edit" in resource.__dict__: @@ -114,7 +115,8 @@ def can_delete(self, resource: object) -> bool: Superusers are granted access without a workspace lookup. For all others, the resource's registered workspace_field is used to resolve the workspace - before checking collaborator permissions. + before checking permissions. Deleting the workspace itself requires ownership; + collaborator permissions only grant deletion of its contents. """ if "can_delete" in resource.__dict__: @@ -153,6 +155,10 @@ def _permission_q(self, model: type, permission_field: str) -> Q: if workspace_field is None: return Q(pk__isnull=False) if permission_field == "can_view" else Q(pk__in=[]) + # Workspace management cannot be delegated through collaborator roles. + if workspace_field is ... and permission_field in ("can_edit", "can_delete"): + return self._owner_q() + accessible_ids = self._accessible_workspace_ids( model.resource_type, permission_field # noqa ) @@ -228,7 +234,8 @@ def has_permission( Return True if this principal has a specific permission on a workspace. Checks in order: superuser status, workspace ownership, then collaborator - role permissions. The workspace may be None for resources that support + role permissions. Workspace edits and deletion are never granted by roles. + The workspace may be None for resources that support being created outside any workspace (e.g., global vocabulary) — only superusers are granted permission in that case. """ @@ -244,6 +251,10 @@ def has_permission( if isinstance(self, User) and workspace.owner_id == self.id: return True + # Workspace management cannot be delegated through collaborator roles. + if resource_type == "Workspace" and permission_field in ("can_edit", "can_delete"): + return False + collaborators = Collaborator.objects.filter( workspace=workspace, role__permissions__resource_type__in=(resource_type, "*"), diff --git a/django/interfaces/api/services/iam/workspace.py b/django/interfaces/api/services/iam/workspace.py index 6e332e4b7..8ee694851 100644 --- a/django/interfaces/api/services/iam/workspace.py +++ b/django/interfaces/api/services/iam/workspace.py @@ -264,7 +264,10 @@ def transfer( ): workspace, _ = self.get_workspace(principal=principal, workspace_id=uid) - if not principal.can_edit(workspace): + if not ( + principal.is_superuser_principal() + or (isinstance(principal, User) and workspace.owner_id == principal.id) + ): raise PermissionDeniedError( "You do not have permission to transfer this workspace" ) diff --git a/django/tests/conftest.py b/django/tests/conftest.py new file mode 100644 index 000000000..b877d9164 --- /dev/null +++ b/django/tests/conftest.py @@ -0,0 +1,11 @@ +import pytest + +from django.core.management import call_command + +from core.iam.models import Role + + +@pytest.fixture +def default_editor_role(db): + call_command("loaddata", "default_roles", verbosity=0) + return Role.objects.get(name="Editor", workspace=None) diff --git a/django/tests/core/iam/permissions/test_mixins.py b/django/tests/core/iam/permissions/test_mixins.py index c40fc2d26..5ab5a7846 100644 --- a/django/tests/core/iam/permissions/test_mixins.py +++ b/django/tests/core/iam/permissions/test_mixins.py @@ -484,6 +484,68 @@ def test_filter_by_permission_returns_every_monitoring_site_for_superuser(): # --- annotate_permissions: consistency with can_view/can_edit/can_delete ----- +@pytest.mark.django_db +@pytest.mark.parametrize("grant", ["Editor", "*", "Workspace"]) +@pytest.mark.parametrize( + "principal_kind", ["collaborator", "service_account", "owner", "superuser", "anonymous"] +) +def test_workspace_management_requires_owner_or_superuser( + default_editor_role, grant, principal_kind +): + workspace = WorkspaceFactory(is_private=True) + if grant == "Editor": + role = default_editor_role + else: + role = RoleFactory(workspace=workspace) + PermissionFactory(role=role, resource_type=grant, full_access=True) + collaborator = CollaboratorFactory( + workspace=workspace, + role=role, + service_account_collaborator=principal_kind == "service_account", + ) + if principal_kind == "superuser": + principal = UserFactory(is_superuser=True) + else: + principal = { + "collaborator": collaborator.user, + "service_account": collaborator.service_account, + "owner": workspace.owner, + "anonymous": AnonymousPrincipal(), + }[principal_kind] + can_manage = principal_kind in ("owner", "superuser") + can_view = principal_kind != "anonymous" + annotated = principal.annotate_permissions(Workspace.objects.all()).get(pk=workspace.pk) + + for field, expected in [ + ("can_view", can_view), ("can_edit", can_manage), ("can_delete", can_manage) + ]: + assert principal.has_permission( + workspace, resource_type="Workspace", permission_field=field + ) is expected + assert getattr(principal, field)(workspace) is expected + assert getattr(annotated, field) is expected + assert getattr(principal, field)(annotated) is expected + assert principal.filter_by_permission( + Workspace.objects.filter(pk=workspace.pk), field + ).exists() is expected + + +@pytest.mark.django_db +def test_editor_retains_write_access_to_workspace_contents(default_editor_role): + workspace = WorkspaceFactory(is_private=True) + editor = CollaboratorFactory(workspace=workspace, role=default_editor_role).user + site = MonitoringSiteFactory(workspace=workspace, is_private=True) + annotated = editor.annotate_permissions(MonitoringSite.objects.all()).get(pk=site.pk) + + assert editor.can_create(MonitoringSite, workspace) is True + for field in ("can_view", "can_edit", "can_delete"): + assert getattr(editor, field)(site) is True + assert getattr(annotated, field) is True + assert editor.filter_by_permission( + MonitoringSite.objects.filter(pk=site.pk), field + ).exists() is True + + @pytest.mark.django_db def test_annotate_permissions_matches_per_object_checks_for_owner(): owner = UserFactory() diff --git a/django/tests/interfaces/api/test_workspace_views.py b/django/tests/interfaces/api/test_workspace_views.py index 7cedc876f..6b33f8efa 100644 --- a/django/tests/interfaces/api/test_workspace_views.py +++ b/django/tests/interfaces/api/test_workspace_views.py @@ -3,6 +3,7 @@ from django.test.utils import CaptureQueriesContext from django.db import connection +from core.iam.permissions.mixins import ResourcePermissionMixin from tests.core.iam.factories import ( CollaboratorFactory, PermissionFactory, @@ -465,14 +466,15 @@ def test_get_workspaces_include_all_three_does_not_scale_queries_with_workspace_ # --- update_workspace --------------------------------------------------------------- -def test_update_workspace_succeeds_for_owner(client): +@pytest.mark.parametrize("as_superuser", [False, True]) +def test_update_workspace_succeeds_for_owner_or_superuser(client, as_superuser): owner = UserFactory() workspace = WorkspaceFactory(owner=owner, name="Original Name") - client.force_login(owner) + client.force_login(UserFactory(is_superuser=True) if as_superuser else owner) response = client.patch( _detail_url(workspace.id), - data={"name": "Updated Name"}, + data={"name": "Updated Name", "isPrivate": True}, content_type="application/json", ) @@ -481,6 +483,7 @@ def test_update_workspace_succeeds_for_owner(client): detail = client.get(_detail_url(workspace.id)) assert detail.json()["data"]["name"] == "Updated Name" + assert detail.json()["data"]["isPrivate"] is True def test_update_workspace_returns_403_for_viewer_collaborator(client): @@ -497,6 +500,36 @@ def test_update_workspace_returns_403_for_viewer_collaborator(client): assert response.status_code == 403 +@pytest.mark.parametrize("operation", ["rename", "publish", "delete", "transfer"]) +def test_editor_cannot_manage_workspace(client, default_editor_role, operation): + workspace = WorkspaceFactory(is_private=True) + original_name, original_owner = workspace.name, workspace.owner + collaborator = CollaboratorFactory(workspace=workspace, role=default_editor_role) + client.force_login(collaborator.user) + + if operation == "delete": + response = client.delete(_detail_url(workspace.id)) + elif operation == "transfer": + response = client.post( + _transfer_url(workspace.id), + data={"newOwner": collaborator.user.email}, + content_type="application/json", + ) + else: + response = client.patch( + _detail_url(workspace.id), + data={"name": "Renamed"} if operation == "rename" else {"isPrivate": False}, + content_type="application/json", + ) + + assert response.status_code == 403 + workspace.refresh_from_db() + assert workspace.name == original_name + assert workspace.is_private is True + assert workspace.owner == original_owner + assert workspace.transfer is None + + def test_update_workspace_returns_400_for_blank_name(client): owner = UserFactory() workspace = WorkspaceFactory(owner=owner) @@ -514,10 +547,11 @@ def test_update_workspace_returns_400_for_blank_name(client): # --- delete_workspace --------------------------------------------------------------- -def test_delete_workspace_succeeds_for_owner(client): +@pytest.mark.parametrize("as_superuser", [False, True]) +def test_delete_workspace_succeeds_for_owner_or_superuser(client, as_superuser): owner = UserFactory() workspace = WorkspaceFactory(owner=owner) - client.force_login(owner) + client.force_login(UserFactory(is_superuser=True) if as_superuser else owner) response = client.delete(_detail_url(workspace.id)) @@ -538,11 +572,12 @@ def test_delete_workspace_returns_403_for_viewer_collaborator(client): # --- transfer_workspace --------------------------------------------------------------- -def test_transfer_workspace_succeeds_for_owner(client): +@pytest.mark.parametrize("as_superuser", [False, True]) +def test_transfer_workspace_succeeds_for_owner_or_superuser(client, as_superuser): owner = UserFactory() workspace = WorkspaceFactory(owner=owner) new_owner = UserFactory() - client.force_login(owner) + client.force_login(UserFactory(is_superuser=True) if as_superuser else owner) response = client.post( _transfer_url(workspace.id), @@ -583,6 +618,26 @@ def test_transfer_workspace_returns_403_for_viewer_collaborator(client): assert response.status_code == 403 +def test_transfer_requires_ownership_even_with_edit_permission( + client, default_editor_role, monkeypatch +): + workspace = WorkspaceFactory(is_private=True) + collaborator = CollaboratorFactory(workspace=workspace, role=default_editor_role) + client.force_login(collaborator.user) + # Keep transfer owner-only even if workspace edits become delegable. + monkeypatch.setattr(ResourcePermissionMixin, "can_edit", lambda self, resource: True) + + response = client.post( + _transfer_url(workspace.id), + data={"newOwner": collaborator.user.email}, + content_type="application/json", + ) + + assert response.status_code == 403 + workspace.refresh_from_db() + assert workspace.transfer is None + + # --- accept_workspace_transfer ---------------------------------------------------------