From 760119a90fef9ca6dab62119b22cd299c5af1e1d Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 4 Nov 2026 06:34:52 -0500 Subject: [PATCH 1/3] OpenConceptLab/ocl_online#648 | Organization roles: admin and member A membership now has a role, through a Role model and an explicit through model on the existing membership table. Only an admin or staff can edit or delete an organization or manage its members and their roles, and the last admin can't be removed or demoted. Any member keeps what they have on the organization's repositories, private ones included, and can still leave. Every existing member becomes an admin, so existing organizations behave the same after the migration. An organization's creator is an admin from now on. The members list gains each member's role, the organization detail gains the requester's role, and PUT on a member takes an optional role. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AaZ4qUSJnKeDYRubKa6DYK --- core/common/mixins.py | 3 +- core/common/permissions.py | 9 + core/common/tests.py | 3 + core/fixtures/org_roles.yaml | 10 + core/importers/models.py | 2 +- core/importers/tests.py | 20 + .../integration_tests/tests_access_control.py | 69 ++- core/integration_tests/tests_orgs.py | 465 +++++++++++++++++- core/orgs/constants.py | 15 + .../0023_role_organizationmember.py | 70 +++ core/orgs/models.py | 60 ++- core/orgs/serializers.py | 7 +- core/orgs/tests/tests.py | 98 +++- core/orgs/views.py | 60 ++- .../0038_alter_userprofile_organizations.py | 23 + core/users/models.py | 4 +- core/users/serializers.py | 16 +- core/users/views.py | 8 + 18 files changed, 882 insertions(+), 60 deletions(-) create mode 100644 core/fixtures/org_roles.yaml create mode 100644 core/orgs/migrations/0023_role_organizationmember.py create mode 100644 core/users/migrations/0038_alter_userprofile_organizations.py diff --git a/core/common/mixins.py b/core/common/mixins.py index d15cda8e7..72f0386fb 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -384,6 +384,7 @@ class PathWalkerMixin: A Mixin with methods that help resolve a resource path to a resource object """ path_info = None + is_parent_lookup = False # this view is only finding the parent of what the request is for @staticmethod def get_parent_in_path(path_info, levels=1): @@ -404,7 +405,7 @@ def get_parent_in_path(path_info, levels=1): @staticmethod def get_object_for_path(path_info, request): callback, _, callback_kwargs = resolve(path_info) - view = callback.cls(request=request, kwargs=callback_kwargs) + view = callback.cls(request=request, kwargs=callback_kwargs, is_parent_lookup=True) view.initialize(request, path_info, **callback_kwargs) return view.get_object() diff --git a/core/common/permissions.py b/core/common/permissions.py index 204ea6e6a..2375715d6 100644 --- a/core/common/permissions.py +++ b/core/common/permissions.py @@ -49,6 +49,15 @@ def has_object_permission(self, request, view, obj): return False +class CanAdministerOrganization(BasePermission): + """ + The user is staff or an admin of the referenced organization. Its repositories don't need this: any member + can edit them. + """ + def has_object_permission(self, request, view, obj): + return obj.has_admin_access(request.user) + + class CanViewConceptDictionary(HasPrivateAccess): """ The user can view this source. A version is viewable only while its HEAD is too, as a version keeps the diff --git a/core/common/tests.py b/core/common/tests.py index 2917a50ac..84ff48292 100644 --- a/core/common/tests.py +++ b/core/common/tests.py @@ -323,6 +323,7 @@ def setUpClass(cls): call_command("loaddata", "core/fixtures/base_entities.yaml", "core/fixtures/ocl_org.yaml") call_command("loaddata", "core/fixtures/auth_groups.yaml") call_command("loaddata", "core/fixtures/capabilities.yaml") + call_command("loaddata", "core/fixtures/org_roles.yaml") call_command("loaddata", "core/fixtures/toggles.json") call_command("seed_group_capabilities", file=TEST_GROUPS_CONFIG_FILE) org = Organization.objects.get(id=1) @@ -336,6 +337,7 @@ def setUpClass(cls): call_command("loaddata", "core/fixtures/base_entities.yaml", "core/fixtures/ocl_org.yaml") call_command("loaddata", "core/fixtures/auth_groups.yaml") call_command("loaddata", "core/fixtures/capabilities.yaml") + call_command("loaddata", "core/fixtures/org_roles.yaml") call_command("loaddata", "core/fixtures/toggles.json") call_command("seed_group_capabilities", file=TEST_GROUPS_CONFIG_FILE) org = Organization.objects.get(id=1) @@ -353,6 +355,7 @@ def setUpTestData(cls): call_command("loaddata", "core/fixtures/base_entities.yaml", "core/fixtures/ocl_org.yaml") call_command("loaddata", "core/fixtures/auth_groups.yaml") call_command("loaddata", "core/fixtures/capabilities.yaml") + call_command("loaddata", "core/fixtures/org_roles.yaml") call_command("loaddata", "core/fixtures/toggles.json") call_command("seed_group_capabilities", file=TEST_GROUPS_CONFIG_FILE) diff --git a/core/fixtures/org_roles.yaml b/core/fixtures/org_roles.yaml new file mode 100644 index 000000000..6d94a11ee --- /dev/null +++ b/core/fixtures/org_roles.yaml @@ -0,0 +1,10 @@ +- model: "orgs.role" + pk: 1 + fields: + name: admin + description: Can edit or delete the organization, manage its members and change their roles +- model: "orgs.role" + pk: 2 + fields: + name: member + description: Can work on the organization's repositories, private ones included diff --git a/core/importers/models.py b/core/importers/models.py index 0f4575730..88dfa737c 100644 --- a/core/importers/models.py +++ b/core/importers/models.py @@ -240,7 +240,7 @@ def process(self): def delete(self): if self.exists(): org = self.get_queryset().first() - if self.user and (self.user.is_staff or org.is_member(self.user)): + if org.has_admin_access(self.user): org.updated_by = self.user org.save(update_fields=['updated_by']) delete_organization(org.id) diff --git a/core/importers/tests.py b/core/importers/tests.py index 4b032094b..24d26a326 100644 --- a/core/importers/tests.py +++ b/core/importers/tests.py @@ -2088,6 +2088,26 @@ def test_organization_importer_delete_permission_denied(self): self.assertEqual(importer.delete(), PERMISSION_DENIED) self.assertTrue(Organization.objects.filter(mnemonic=org.mnemonic).exists()) + def test_organization_importer_delete_refuses_a_plain_member(self): + org = OrganizationFactory(mnemonic='MemberOrg') + member = UserProfileFactory(organizations=[org]) + + self.assertEqual(OrganizationImporter({'id': 'MemberOrg'}, member).delete(), PERMISSION_DENIED) + self.assertTrue(Organization.objects.filter(mnemonic='MemberOrg').exists()) + + def test_organization_importer_delete_by_an_admin(self): + admin = UserProfileFactory() + OrganizationFactory(mnemonic='AdminOrg', created_by=admin, updated_by=admin) + + self.assertEqual(OrganizationImporter({'id': 'AdminOrg'}, admin).delete(), DELETED) + self.assertFalse(Organization.objects.filter(mnemonic='AdminOrg').exists()) + + def test_organization_importer_makes_the_creator_an_admin(self): + user = UserProfileFactory() + + self.assertEqual(OrganizationImporter({'id': 'ImportedOrg', 'name': 'Imported'}, user).run(), CREATED) + self.assertTrue(Organization.objects.get(mnemonic='ImportedOrg').is_admin(user)) + def test_organization_importer_does_not_create_org_whose_name_differs_only_in_case(self): OrganizationFactory(mnemonic='TwinOrg') importer = OrganizationImporter({'id': 'twinorg', 'name': 'Twin'}, UserProfileFactory()) diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 7da91566f..399bbe1ed 100644 --- a/core/integration_tests/tests_access_control.py +++ b/core/integration_tests/tests_access_control.py @@ -15,6 +15,7 @@ from core.concepts.models import Concept from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory, ConceptDescriptionFactory from core.map_projects.tests.factories import MapProjectFactory, AutomatchRunFactory +from core.orgs.constants import ROLE_ADMIN from core.orgs.models import Organization from core.orgs.tests.factories import OrganizationFactory from core.pins.models import Pin @@ -30,14 +31,16 @@ class AccessTestMixin: """ - Staff, an org with one member, the owner of user-owned resources, and an outsider: a logged-in user who belongs - to no org and owns nothing under test. + Staff, an org with one member and one admin, the owner of user-owned resources, and an outsider: a logged-in user + who belongs to no org and owns nothing under test. """ def setUp(self): super().setUp() self.staff = UserProfile.objects.get(username='ocladmin') self.org = OrganizationFactory(public_access=ACCESS_TYPE_VIEW) self.member = UserProfileFactory(organizations=[self.org]) + self.admin = UserProfileFactory() + self.org.set_member_role(self.admin, ROLE_ADMIN) self.owner = UserProfileFactory() self.outsider = UserProfileFactory() @@ -68,8 +71,11 @@ def test_outsider_cannot_edit(self): def test_anonymous_cannot_edit(self): self.assert_not_renamed(self.rename(None), 401) - def test_member_can_edit(self): - self.assert_renamed(self.rename(self.member)) + def test_member_cannot_edit(self): + self.assert_not_renamed(self.rename(self.member), 403) + + def test_admin_can_edit(self): + self.assert_renamed(self.rename(self.admin)) def test_staff_can_edit(self): self.assert_renamed(self.rename(self.staff)) @@ -118,8 +124,11 @@ def test_outsider_cannot_edit(self): def test_anonymous_cannot_edit(self): self.assert_not_edited(self.edit(None), 401) - def test_member_can_edit(self): - self.assert_edited(self.edit(self.member)) + def test_member_cannot_edit(self): + self.assert_not_edited(self.edit(self.member), 403) + + def test_admin_can_edit(self): + self.assert_edited(self.edit(self.admin)) def test_staff_can_edit(self): self.assert_edited(self.edit(self.staff)) @@ -130,8 +139,16 @@ def test_outsider_cannot_delete(self): def test_anonymous_cannot_delete(self): self.assert_not_deleted(self.request('delete', self.get_url()), 401) - def test_member_and_staff_delete_clears_overview_only(self): - for user in [self.member, self.staff]: + def test_member_cannot_delete(self): + self.org.overview = {'text': 'Existing'} + self.org.save() + + self.assertEqual(self.request('delete', self.get_url(), self.member).status_code, 403) + self.org.refresh_from_db() + self.assertEqual(self.org.overview, {'text': 'Existing'}) + + def test_admin_and_staff_delete_clears_overview_only(self): + for user in [self.admin, self.staff]: self.org.overview = {'text': 'Existing'} self.org.save() @@ -179,14 +196,17 @@ def test_outsider_cannot_upload(self): def test_anonymous_cannot_upload(self): self.assert_not_uploaded(self.upload(None), 401) - def test_member_can_upload(self): - self.assert_uploaded(self.upload(self.member)) + def test_member_cannot_upload(self): + self.assert_not_uploaded(self.upload(self.member), 403) + + def test_admin_can_upload(self): + self.assert_uploaded(self.upload(self.admin)) def test_staff_can_upload(self): self.assert_uploaded(self.upload(self.staff)) def test_delete_clears_logo_only(self): - for user, status_code in [(None, 401), (self.outsider, 403)]: + for user, status_code in [(None, 401), (self.outsider, 403), (self.member, 403)]: self.org.logo_path = f'orgs/{self.org.mnemonic}/logo.png' self.org.save() @@ -194,7 +214,7 @@ def test_delete_clears_logo_only(self): self.org.refresh_from_db() self.assertEqual(self.org.logo_path, f'orgs/{self.org.mnemonic}/logo.png') - for user in [self.member, self.staff]: + for user in [self.admin, self.staff]: self.org.logo_path = f'orgs/{self.org.mnemonic}/logo.png' self.org.save() @@ -747,10 +767,15 @@ def test_anonymous_cannot_edit_or_delete(self): self.assertEqual(self.delete(None).status_code, 401) self.assert_extras({'foo': 'bar'}) - def test_member_can_edit_and_delete(self): - self.assertEqual(self.edit(self.member).status_code, 200) + def test_member_cannot_edit_or_delete(self): + self.assertEqual(self.edit(self.member).status_code, 403) + self.assertEqual(self.delete(self.member).status_code, 403) + self.assert_extras({'foo': 'bar'}) + + def test_admin_can_edit_and_delete(self): + self.assertEqual(self.edit(self.admin).status_code, 200) self.assert_extras({'foo': 'changed'}) - self.assertEqual(self.delete(self.member).status_code, 204) + self.assertEqual(self.delete(self.admin).status_code, 204) self.assert_extras({}) def test_staff_can_edit(self): @@ -825,8 +850,8 @@ def test_staff_can_upload(self): class ClientConfigAccessTest(AccessTestMixin, OCLAPITestCase): """ - Adding a config to an org needs staff or membership, and to a repo staff or edit access. Changing a config needs - staff or its creator. + Adding a config to an org needs staff or an admin of it, and to a repo staff or edit access. Changing a config + needs staff or its creator. """ payload = {'name': 'Home', 'type': 'home', 'config': {'tabs': [{'type': 'concepts', 'default': True}]}} @@ -850,7 +875,9 @@ def assert_cannot_create(self, resource): def test_create_on_org(self): self.assert_cannot_create(self.org) - self.assert_can_create(self.org, self.member) + self.assertEqual(self.create(self.org, self.member).status_code, 403) + self.assertFalse(self.org.client_configs.exists()) + self.assert_can_create(self.org, self.admin) self.assert_can_create(self.org, self.staff) def test_create_on_source(self): @@ -862,16 +889,16 @@ def test_create_on_collection(self): self.assert_can_create(self.collection, self.member) def test_edit(self): - config_id = self.assert_can_create(self.org, self.member) + config_id = self.assert_can_create(self.org, self.admin) url = f'/client-configs/{config_id}/' - for user in [self.outsider, self.owner]: + for user in [self.outsider, self.owner, self.member]: self.assertEqual(self.request('put', url, user, {'name': 'Renamed'}).status_code, 403) self.assertEqual(self.request('patch', url, user, {'name': 'Renamed'}).status_code, 403) self.assertEqual(self.request('put', url, None, {'name': 'Renamed'}).status_code, 401) self.assertEqual(ClientConfig.objects.get(id=config_id).name, 'Home') - self.assertEqual(self.request('put', url, self.member, {'name': 'Renamed'}).status_code, 200) + self.assertEqual(self.request('put', url, self.admin, {'name': 'Renamed'}).status_code, 200) self.assertEqual(ClientConfig.objects.get(id=config_id).name, 'Renamed') self.assertEqual(self.request('put', url, self.staff, {'name': 'Renamed again'}).status_code, 200) self.assertEqual(ClientConfig.objects.get(id=config_id).name, 'Renamed again') diff --git a/core/integration_tests/tests_orgs.py b/core/integration_tests/tests_orgs.py index 2a758b0b0..a45af9cfe 100644 --- a/core/integration_tests/tests_orgs.py +++ b/core/integration_tests/tests_orgs.py @@ -3,6 +3,7 @@ from django.db import connection from django.test import override_settings, tag +from django.test.utils import CaptureQueriesContext from mock import patch from mock.mock import Mock, ANY from rest_framework.exceptions import ErrorDetail @@ -12,6 +13,8 @@ from core.collections.tests.factories import OrganizationCollectionFactory, UserCollectionFactory from core.common.constants import ACCESS_TYPE_NONE, ACCESS_TYPE_VIEW, RETIRED_ACCESS_TYPE_EDIT from core.common.tests import OCLAPITestCase, OCLAPITransactionTestCase +from core.events.models import Event +from core.orgs.constants import ROLE_ADMIN, ROLE_MEMBER from core.orgs.models import Organization from core.orgs.tests.factories import OrganizationFactory from core.sources.documents import SourceDocument @@ -175,7 +178,8 @@ def test_post_400(self): class OrganizationDetailViewTest(OCLAPITestCase): def setUp(self): self.org = OrganizationFactory(name='Stark Enterprises') - self.user = UserProfileFactory(organizations=[self.org]) + self.user = UserProfileFactory() + self.org.set_member_role(self.user, ROLE_ADMIN) self.superuser = UserProfile.objects.get(is_superuser=True) self.token = self.user.get_token() @@ -534,11 +538,12 @@ def test_delete_204(self): def test_delete_204_creator(self): creator = UserProfileFactory() org = OrganizationFactory(created_by=creator, updated_by=creator) - member = UserProfileFactory(organizations=[org]) + other_admin = UserProfileFactory() + org.set_member_role(other_admin, ROLE_ADMIN) response = self.client.delete( f'/orgs/{org.mnemonic}/members/{creator.username}/', - HTTP_AUTHORIZATION='Token ' + member.get_token(), + HTTP_AUTHORIZATION='Token ' + other_admin.get_token(), format='json' ) @@ -548,11 +553,12 @@ def test_delete_204_creator(self): def test_delete_204_last_editor(self): self.org.updated_by = self.user self.org.save() - member = UserProfileFactory(organizations=[self.org]) + admin = UserProfileFactory() + self.org.set_member_role(admin, ROLE_ADMIN) response = self.client.delete( f'/orgs/{self.org.mnemonic}/members/{self.user.username}/', - HTTP_AUTHORIZATION='Token ' + member.get_token(), + HTTP_AUTHORIZATION='Token ' + admin.get_token(), format='json' ) @@ -590,7 +596,7 @@ def test_delete_409_only_member_by_staff(self): def test_delete_404(self): response = self.client.delete( f'/orgs/{self.org.mnemonic}/members/foobar/', - HTTP_AUTHORIZATION='Token ' + self.token, + HTTP_AUTHORIZATION='Token ' + self.superuser.get_token(), format='json' ) @@ -605,7 +611,7 @@ def test_two_members_removing_each_other_at_once_leave_one_member(self): alice = UserProfileFactory(username='alice') bob = UserProfileFactory(username='bob') org = OrganizationFactory(mnemonic='race-org', created_by=alice, updated_by=alice) - org.members.add(bob) + org.set_member_role(bob, ROLE_ADMIN) tokens = {'alice': alice.get_token(), 'bob': bob.get_token()} is_only_member = Organization.is_only_member @@ -641,6 +647,442 @@ def remove(actor, target): self.assertEqual(org.members.count(), 1) +class OrganizationAdminOnlyTest(OCLAPITestCase): + """Editing or deleting an org needs an admin of it, or staff. A plain member gets a 403 and changes nothing.""" + ACTIONS = ( + ('put', '/orgs/stark/', {'name': 'Wayne'}, 200), + ('patch', '/orgs/stark/', {'name': 'Wayne'}, 200), + ('post', '/orgs/stark/logo/', {'base64': 'base64-data'}, 200), + ('delete', '/orgs/stark/logo/', None, 204), + ('put', '/orgs/stark/overview/', {'overview': {'foo': 'bar'}}, 200), + ('delete', '/orgs/stark/overview/', None, 204), + ('put', '/orgs/stark/extras/foo/', {'foo': 'baz'}, 200), + ('delete', '/orgs/stark/extras/foo/', None, 204), + ('post', '/orgs/stark/client-configs/', {'name': 'custom', 'config': {'tabs': [{'default': True}]}}, 201), + ('delete', '/orgs/stark/?inline=true', None, 204), + ) + + def setUp(self): + super().setUp() + self.org = OrganizationFactory( + mnemonic='stark', name='Stark', extras={'foo': 'bar'}, overview={'foo': 'bar'}, logo_path='logo.png') + self.member = UserProfileFactory(organizations=[self.org]) + self.admin = UserProfileFactory(organizations=[self.org]) + self.org.set_member_role(self.admin, ROLE_ADMIN) + + def request(self, method, url, data, user): + return getattr(self.client, method)(url, data, HTTP_AUTHORIZATION='Token ' + user.get_token(), format='json') + + @staticmethod + def get_state(): + return Organization.objects.filter(mnemonic='stark').values( + 'name', 'extras', 'overview', 'logo_path').first() + + def test_member_is_refused(self): + before = self.get_state() + for method, url, data, _ in self.ACTIONS: + with self.subTest(method=method, url=url): + self.assertEqual(self.request(method, url, data, self.member).status_code, 403) + self.assertEqual(self.get_state(), before) + self.assertEqual(self.org.client_configs.count(), 0) + + @patch('core.services.storages.cloud.aws.S3.upload_base64', Mock(return_value='orgs/stark/logo.png')) + def test_admin_is_allowed(self): + for method, url, data, expected_status in self.ACTIONS: + with self.subTest(method=method, url=url): + self.assertEqual(self.request(method, url, data, self.admin).status_code, expected_status) + self.assertFalse(Organization.objects.filter(mnemonic='stark').exists()) + + @patch('core.services.storages.cloud.aws.S3.upload_base64', Mock(return_value='orgs/stark/logo.png')) + def test_staff_who_is_not_a_member_is_allowed(self): + staff = UserProfileFactory(is_staff=True) + for method, url, data, expected_status in self.ACTIONS: + with self.subTest(method=method, url=url): + self.assertEqual(self.request(method, url, data, staff).status_code, expected_status) + self.assertFalse(Organization.objects.filter(mnemonic='stark').exists()) + + +class OrganizationMemberRepositoryAccessTest(OCLAPITestCase): + """A plain member keeps everything they have on the org's repositories, private ones included.""" + def setUp(self): + super().setUp() + self.org = OrganizationFactory(mnemonic='stark', public_access=ACCESS_TYPE_NONE) + self.member = UserProfileFactory(organizations=[self.org]) + self.source = OrganizationSourceFactory( + organization=self.org, mnemonic='private-source', public_access=ACCESS_TYPE_NONE) + self.auth = {'HTTP_AUTHORIZATION': 'Token ' + self.member.get_token()} + + def test_views_the_private_org_and_its_private_repository(self): + self.assertEqual(self.client.get('/orgs/stark/', **self.auth).status_code, 200) + self.assertEqual(self.client.get('/orgs/stark/sources/private-source/', **self.auth).status_code, 200) + + def test_creates_a_repository(self): + response = self.client.post( + '/orgs/stark/sources/', + {'id': 'new-source', 'name': 'New', 'short_code': 'new', 'public_access': ACCESS_TYPE_NONE}, + format='json', **self.auth + ) + + self.assertEqual(response.status_code, 201) + + def test_edits_a_private_repository(self): + collection = OrganizationCollectionFactory( + organization=self.org, mnemonic='private-collection', public_access=ACCESS_TYPE_NONE) + + for method, url, repo in ( + ('put', '/orgs/stark/sources/private-source/', self.source), + ('patch', '/orgs/stark/sources/private-source/', self.source), + ('put', '/orgs/stark/collections/private-collection/', collection), + ('patch', '/orgs/stark/collections/private-collection/', collection), + ): + with self.subTest(method=method, url=url): + name = f'Renamed by {method}' + + response = getattr(self.client, method)(url, {'name': name}, format='json', **self.auth) + + self.assertEqual(response.status_code, 200) + repo.refresh_from_db() + self.assertEqual(repo.name, name) + + def test_is_refused_editing_a_repository_outside_the_org(self): + stranger = {'HTTP_AUTHORIZATION': 'Token ' + UserProfileFactory().get_token()} + + response = self.client.put( + '/orgs/stark/sources/private-source/', {'name': 'Renamed'}, format='json', **stranger) + + self.assertEqual(response.status_code, 403) + self.source.refresh_from_db() + self.assertNotEqual(self.source.name, 'Renamed') + + @patch('core.common.models.delete_s3_objects', Mock()) + def test_deletes_a_repository(self): + response = self.client.delete('/orgs/stark/sources/private-source/?inline=true', **self.auth) + + self.assertEqual(response.status_code, 204) + + def test_manages_exports_of_a_repository(self): + self.assertTrue(self.member.is_admin_for(self.source)) + self.assertFalse(UserProfileFactory().is_admin_for(self.source)) + + def test_is_refused_outside_the_org(self): + stranger = {'HTTP_AUTHORIZATION': 'Token ' + UserProfileFactory().get_token()} + + self.assertEqual(self.client.get('/orgs/stark/sources/private-source/', **stranger).status_code, 403) + + +class OrganizationMemberRoleTest(OCLAPITestCase): + def setUp(self): + super().setUp() + self.admin = UserProfileFactory() + self.org = OrganizationFactory(mnemonic='stark', created_by=self.admin, updated_by=self.admin) + self.member = UserProfileFactory(organizations=[self.org]) + + def put(self, target, data=None, actor=None): + return self.client.put( + f'/orgs/stark/members/{target.username}/', data, format='json', + HTTP_AUTHORIZATION='Token ' + (actor or self.admin).get_token()) + + def delete(self, target, actor=None): + return self.client.delete( + f'/orgs/stark/members/{target.username}/', HTTP_AUTHORIZATION='Token ' + (actor or self.admin).get_token()) + + def get_role(self, user): + return self.org.get_member_role(user) + + def test_put_adds_a_new_member_as_member_by_default(self): + user = UserProfileFactory() + + response = self.put(user) + + self.assertEqual(response.status_code, 204) + self.assertEqual(self.get_role(user), ROLE_MEMBER) + self.assertEqual(Event.objects.filter(event_type=Event.JOINED, object_url=user.uri).count(), 1) + + def test_put_adds_a_new_member_as_admin(self): + user = UserProfileFactory() + + self.assertEqual(self.put(user, {'role': ROLE_ADMIN}).status_code, 204) + self.assertEqual(self.get_role(user), ROLE_ADMIN) + + def test_put_without_a_role_keeps_an_existing_admin_as_admin(self): + self.org.set_member_role(self.member, ROLE_ADMIN) + + self.assertEqual(self.put(self.member).status_code, 204) + self.assertEqual(self.get_role(self.member), ROLE_ADMIN) + + def test_put_on_an_existing_member_records_no_second_joined_event(self): + self.assertEqual(self.put(self.member, {'role': ROLE_ADMIN}).status_code, 204) + self.assertEqual(self.put(self.member).status_code, 204) + + self.assertFalse(Event.objects.filter(event_type=Event.JOINED, object_url=self.member.uri).exists()) + + def test_put_promotes_and_demotes(self): + self.assertEqual(self.put(self.member, {'role': ROLE_ADMIN}).status_code, 204) + self.assertEqual(self.get_role(self.member), ROLE_ADMIN) + + self.assertEqual(self.put(self.member, {'role': ROLE_MEMBER}).status_code, 204) + self.assertEqual(self.get_role(self.member), ROLE_MEMBER) + + def test_put_400_for_a_role_that_does_not_exist(self): + user = UserProfileFactory() + for role in ('owner', '', 1, ['admin'], {'name': 'admin'}): + with self.subTest(role=role): + response = self.put(user, {'role': role}) + + self.assertEqual(response.status_code, 400) + self.assertEqual(list(response.data), ['role']) + self.assertFalse(self.org.is_member(user)) + + def test_put_403_for_a_member(self): + user = UserProfileFactory() + + self.assertEqual(self.put(user, actor=self.member).status_code, 403) + self.assertFalse(self.org.is_member(user)) + + def test_put_403_for_a_member_promoting_themselves(self): + self.assertEqual(self.put(self.member, {'role': ROLE_ADMIN}, actor=self.member).status_code, 403) + self.assertEqual(self.get_role(self.member), ROLE_MEMBER) + + def test_put_403_for_a_stranger(self): + stranger = UserProfileFactory() + + self.assertEqual(self.put(stranger, {'role': ROLE_ADMIN}, actor=stranger).status_code, 403) + self.assertFalse(self.org.is_member(stranger)) + + def test_put_by_staff_who_is_not_a_member(self): + staff = UserProfileFactory(is_staff=True) + + self.assertEqual(self.put(self.member, {'role': ROLE_ADMIN}, actor=staff).status_code, 204) + self.assertEqual(self.get_role(self.member), ROLE_ADMIN) + self.assertFalse(self.org.is_member(staff)) + + def test_put_409_demoting_the_last_admin(self): + response = self.put(self.admin, {'role': ROLE_MEMBER}) + + self.assertEqual(response.status_code, 409) + self.assertEqual(response.data, {'detail': 'Cannot remove or demote the last admin of an organization.'}) + self.assertEqual(self.get_role(self.admin), ROLE_ADMIN) + + def test_put_409_demoting_the_last_admin_by_staff(self): + staff = UserProfileFactory(is_staff=True) + + self.assertEqual(self.put(self.admin, {'role': ROLE_MEMBER}, actor=staff).status_code, 409) + self.assertEqual(self.get_role(self.admin), ROLE_ADMIN) + + def test_put_409_demoting_the_last_active_admin(self): + deactivated = UserProfileFactory(organizations=[self.org], is_active=False) + self.org.set_member_role(deactivated, ROLE_ADMIN) + + self.assertEqual(self.put(self.admin, {'role': ROLE_MEMBER}).status_code, 409) + + def test_put_demotes_an_admin_when_another_remains(self): + self.org.set_member_role(self.member, ROLE_ADMIN) + + self.assertEqual(self.put(self.admin, {'role': ROLE_MEMBER}).status_code, 204) + self.assertEqual(self.get_role(self.admin), ROLE_MEMBER) + + def test_delete_by_an_admin(self): + self.assertEqual(self.delete(self.member).status_code, 204) + self.assertFalse(self.org.is_member(self.member)) + + def test_delete_403_for_a_member_removing_someone_else(self): + other = UserProfileFactory(organizations=[self.org]) + + self.assertEqual(self.delete(other, actor=self.member).status_code, 403) + self.assertEqual(self.delete(self.admin, actor=self.member).status_code, 403) + self.assertTrue(self.org.is_member(other)) + self.assertTrue(self.org.is_member(self.admin)) + + def test_delete_lets_a_member_leave(self): + self.assertEqual(self.delete(self.member, actor=self.member).status_code, 204) + self.assertFalse(self.org.is_member(self.member)) + + def test_delete_lets_an_admin_leave_when_another_remains(self): + self.org.set_member_role(self.member, ROLE_ADMIN) + + self.assertEqual(self.delete(self.admin, actor=self.admin).status_code, 204) + self.assertFalse(self.org.is_member(self.admin)) + + def test_delete_409_for_the_last_admin(self): + for actor in (self.admin, UserProfileFactory(is_staff=True)): + with self.subTest(actor=actor.username): + response = self.delete(self.admin, actor=actor) + + self.assertEqual(response.status_code, 409) + self.assertEqual( + response.data, {'detail': 'Cannot remove or demote the last admin of an organization.'}) + self.assertTrue(self.org.is_member(self.admin)) + + def test_delete_409_when_the_only_other_admin_is_deactivated(self): + deactivated = UserProfileFactory(organizations=[self.org], is_active=False) + self.org.set_member_role(deactivated, ROLE_ADMIN) + + self.assertEqual(self.delete(self.admin, actor=self.admin).status_code, 409) + self.assertTrue(self.org.is_member(self.admin)) + + def test_delete_removes_a_deactivated_admin(self): + deactivated = UserProfileFactory(organizations=[self.org], is_active=False) + self.org.set_member_role(deactivated, ROLE_ADMIN) + + self.assertEqual(self.delete(deactivated).status_code, 204) + self.assertFalse(self.org.is_member(deactivated)) + + def test_a_demoted_admin_loses_the_rights_at_once(self): + self.org.set_member_role(self.member, ROLE_ADMIN) + self.org.set_member_role(self.member, ROLE_MEMBER) + + response = self.client.put( + '/orgs/stark/', {'name': 'Wayne'}, format='json', + HTTP_AUTHORIZATION='Token ' + self.member.get_token()) + + self.assertEqual(response.status_code, 403) + + +class OrganizationMemberRoleReadTest(OCLAPITestCase): + def setUp(self): + super().setUp() + self.admin = UserProfileFactory(username='the-admin') + self.org = OrganizationFactory(mnemonic='stark', created_by=self.admin, updated_by=self.admin) + self.member = UserProfileFactory(username='the-member', organizations=[self.org]) + + def test_members_list_has_each_role_in_every_shape(self): + for params in ('', '?verbose=true', '?summary=true'): + with self.subTest(params=params): + response = self.client.get(f'/orgs/stark/members/{params}') + + self.assertEqual(response.status_code, 200) + self.assertEqual( + {row['username']: row['role'] for row in response.data}, + {'the-admin': 'admin', 'the-member': 'member'} + ) + + def test_users_alias_of_the_members_list_has_each_role(self): + response = self.client.get('/orgs/stark/users/') + + self.assertEqual({row['username']: row['role'] for row in response.data}, + {'the-admin': 'admin', 'the-member': 'member'}) + + def test_role_is_from_this_org_not_another(self): + other = OrganizationFactory(mnemonic='other', created_by=self.member, updated_by=self.member) + other.members.add(self.admin) + + response = self.client.get('/orgs/other/members/') + + self.assertEqual({row['username']: row['role'] for row in response.data}, + {'the-admin': 'member', 'the-member': 'admin'}) + + def test_role_query_does_not_grow_with_the_members(self): + def count_role_queries(): + with CaptureQueriesContext(connection) as context: + self.assertEqual(self.client.get('/orgs/stark/members/').status_code, 200) + return len([query for query in context.captured_queries if 'org_roles' in query['sql']]) + + self.assertEqual(count_role_queries(), 1) + + UserProfileFactory(organizations=[self.org]) + UserProfileFactory(organizations=[self.org]) + + self.assertEqual(count_role_queries(), 1) + + def test_user_lists_outside_an_org_have_no_role(self): + response = self.client.get( + '/users/', HTTP_AUTHORIZATION='Token ' + self.admin.get_token()) + + self.assertEqual(response.status_code, 200) + self.assertTrue(response.data) + self.assertFalse(any('role' in row for row in response.data)) + + def test_org_detail_has_the_role_of_the_requester(self): + for user, expected in ( + (self.admin, 'admin'), + (self.member, 'member'), + (UserProfileFactory(), None), + (UserProfileFactory(is_staff=True), None), + ): + with self.subTest(user=user.username): + response = self.client.get('/orgs/stark/', HTTP_AUTHORIZATION='Token ' + user.get_token()) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data['user_role'], expected) + + def test_org_detail_has_no_role_for_anonymous(self): + response = self.client.get('/orgs/stark/') + + self.assertEqual(response.status_code, 200) + self.assertIsNone(response.data['user_role']) + + def test_creating_an_org_makes_the_creator_its_admin(self): + user = UserProfileFactory() + + response = self.client.post( + '/orgs/', {'id': 'new-org', 'name': 'New'}, format='json', + HTTP_AUTHORIZATION='Token ' + user.get_token()) + + self.assertEqual(response.status_code, 201) + self.assertEqual(response.data['user_role'], 'admin') + self.assertTrue(Organization.objects.get(mnemonic='new-org').is_admin(user)) + + +@tag('es') +class OrganizationMemberRoleSearchTest(OCLAPITestCase): + def test_search_results_have_each_role(self): + admin = UserProfileFactory(username='searchable-admin') + org = OrganizationFactory(mnemonic='stark', created_by=admin, updated_by=admin) + member = UserProfileFactory(username='searchable-member', organizations=[org]) + admin.save() + member.save() + + response = self.client.get('/orgs/stark/members/?q=searchable') + + self.assertEqual(response.status_code, 200) + self.assertEqual({row['username']: row['role'] for row in response.data}, + {'searchable-admin': 'admin', 'searchable-member': 'member'}) + + +class OrganizationAdminConcurrencyTest(OCLAPITransactionTestCase): + """Runs two demotions at once, on separate connections, to check that every org keeps an admin.""" + + def test_two_admins_demoting_each_other_at_once_leave_one_admin(self): + alice = UserProfileFactory(username='alice') + bob = UserProfileFactory(username='bob') + org = OrganizationFactory(mnemonic='race-org', created_by=alice, updated_by=alice) + org.set_member_role(bob, ROLE_ADMIN) + tokens = {'alice': alice.get_token(), 'bob': bob.get_token()} + is_last_admin = Organization.is_last_admin + + def slow_is_last_admin(organization, user_profile): + result = is_last_admin(organization, user_profile) + time.sleep(0.5) # widens the gap between the check and the change + return result + + barrier = threading.Barrier(2) + statuses = {} + + def demote(actor, target): + try: + client = APIClient() + barrier.wait() + response = client.put( + f'/orgs/race-org/members/{target}/', {'role': ROLE_MEMBER}, format='json', + HTTP_AUTHORIZATION='Token ' + tokens[actor]) + statuses[actor] = response.status_code + finally: + connection.close() + + with patch.object(Organization, 'is_last_admin', slow_is_last_admin): + threads = [ + threading.Thread(target=demote, args=('alice', 'bob')), + threading.Thread(target=demote, args=('bob', 'alice')), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + self.assertEqual(sorted(statuses.values()), [204, 409]) + self.assertEqual(org.memberships.filter(role__name=ROLE_ADMIN).count(), 1) + + class OrganizationExtrasViewTest(OCLAPITestCase): def test_get_200(self): org = OrganizationFactory(extras={}) @@ -673,7 +1115,8 @@ def setUp(self): super().setUp() self.extras = {'foo': 'bar', 'tao': 'ching'} self.organization = OrganizationFactory(extras=self.extras) - self.user = UserProfileFactory(organizations=[self.organization]) + self.user = UserProfileFactory() + self.organization.set_member_role(self.user, ROLE_ADMIN) self.token = self.user.get_token() def test_get_200(self): @@ -753,7 +1196,8 @@ class OrganizationLogoViewTest(OCLAPITestCase): def setUp(self): super().setUp() self.organization = OrganizationFactory(mnemonic='org-1') - self.user = UserProfileFactory(organizations=[self.organization]) + self.user = UserProfileFactory() + self.organization.set_member_role(self.user, ROLE_ADMIN) self.token = self.user.get_token() @patch('core.services.storages.cloud.aws.S3.upload_base64') @@ -781,7 +1225,8 @@ def test_post_200(self, upload_base64_mock): class OrganizationOverviewViewTest(OCLAPITestCase): def setUp(self): self.org = OrganizationFactory(name='Stark Enterprises') - self.user = UserProfileFactory(organizations=[self.org]) + self.user = UserProfileFactory() + self.org.set_member_role(self.user, ROLE_ADMIN) self.superuser = UserProfile.objects.get(is_superuser=True) self.token = self.user.get_token() diff --git a/core/orgs/constants.py b/core/orgs/constants.py index 9db9f2359..96efb9908 100644 --- a/core/orgs/constants.py +++ b/core/orgs/constants.py @@ -4,3 +4,18 @@ DELETE_ACCEPTED = 'Successfully accepted deleted Organization.' NO_MEMBERS = 'noMembers' CANNOT_REMOVE_ONLY_MEMBER = 'Cannot remove the only member of an organization.' +CANNOT_REMOVE_LAST_ADMIN = 'Cannot remove or demote the last admin of an organization.' + +INVALID_ROLE = 'Must be admin or member.' + +ROLE_ADMIN = 'admin' +ROLE_MEMBER = 'member' + +ROLE_ADMIN_ID = 1 +ROLE_MEMBER_ID = 2 + +ROLE_ID_BY_NAME = { + ROLE_ADMIN: ROLE_ADMIN_ID, + ROLE_MEMBER: ROLE_MEMBER_ID, +} +ROLES = tuple(ROLE_ID_BY_NAME) diff --git a/core/orgs/migrations/0023_role_organizationmember.py b/core/orgs/migrations/0023_role_organizationmember.py new file mode 100644 index 000000000..b1fbe7af2 --- /dev/null +++ b/core/orgs/migrations/0023_role_organizationmember.py @@ -0,0 +1,70 @@ +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +def seed_roles(apps, _): + """Only what the new column needs. load_fixtures fills in the rest from core/fixtures/org_roles.yaml.""" + role = apps.get_model('orgs', 'Role') + role.objects.bulk_create([role(id=1, name='admin'), role(id=2, name='member')]) + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0037_userprofile_sso_id'), + ('orgs', '0022_retire_public_edit_access'), + ] + + operations = [ + migrations.CreateModel( + name='Role', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('name', models.CharField(max_length=100, unique=True)), + ('description', models.TextField(blank=True, default='')), + ], + options={ + 'db_table': 'org_roles', + }, + ), + migrations.RunPython(seed_roles, migrations.RunPython.noop), + # The table exists already, as the automatic many-to-many table of UserProfile.organizations. + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.CreateModel( + name='OrganizationMember', + fields=[ + ('id', models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('organization', models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, related_name='memberships', + to='orgs.organization')), + ('userprofile', models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, related_name='org_memberships', + to=settings.AUTH_USER_MODEL)), + ], + options={ + 'db_table': 'user_profiles_organizations', + 'unique_together': {('userprofile', 'organization')}, + }, + ), + ], + ), + # Every existing member becomes an admin, so that nobody loses what they can do today. + migrations.AddField( + model_name='organizationmember', + name='role', + field=models.ForeignKey( + default=1, on_delete=django.db.models.deletion.PROTECT, related_name='memberships', to='orgs.role'), + ), + # From here on a new member is a plain member. The column keeps that default for inserts from code that + # doesn't know the column yet, as during a rollout or a rollback. + migrations.AlterField( + model_name='organizationmember', + name='role', + field=models.ForeignKey( + db_default=2, default=2, on_delete=django.db.models.deletion.PROTECT, related_name='memberships', + to='orgs.role'), + ), + ] diff --git a/core/orgs/models.py b/core/orgs/models.py index 4035e59df..53a78086a 100644 --- a/core/orgs/models.py +++ b/core/orgs/models.py @@ -8,10 +8,31 @@ from core.common.constants import NAMESPACE_REGEX, ACCESS_TYPE_VIEW from core.common.mixins import SourceContainerMixin from core.common.models import BaseResourceModel -from core.orgs.constants import ORG_OBJECT_TYPE +from core.orgs.constants import ORG_OBJECT_TYPE, ROLE_ADMIN_ID, ROLE_ID_BY_NAME, ROLE_MEMBER_ID from core.users.models import Follow +class Role(models.Model): + """What a member may do in an organization. The rows are in core/fixtures/org_roles.yaml.""" + class Meta: + db_table = 'org_roles' + + name = models.CharField(max_length=100, unique=True) + description = models.TextField(blank=True, default='') + + +class OrganizationMember(models.Model): + """A user's membership of an organization, with their role in it.""" + class Meta: + db_table = 'user_profiles_organizations' + unique_together = ('userprofile', 'organization') + + userprofile = models.ForeignKey('users.UserProfile', on_delete=models.CASCADE, related_name='org_memberships') + organization = models.ForeignKey('orgs.Organization', on_delete=models.CASCADE, related_name='memberships') + role = models.ForeignKey( + Role, on_delete=models.PROTECT, related_name='memberships', default=ROLE_MEMBER_ID, db_default=ROLE_MEMBER_ID) + + class Organization(BaseResourceModel, SourceContainerMixin, ChecksumModel): class Meta: db_table = 'organizations' @@ -70,6 +91,39 @@ def has_edit_access(self, user): return False return user.is_staff or self.is_member(user) + def is_admin(self, user_profile): + return bool(user_profile) and self.memberships.filter( + userprofile_id=user_profile.id, role_id=ROLE_ADMIN_ID).exists() + + def has_admin_access(self, user): + """Whether `user` may edit or delete the org and manage its members: staff and its admins.""" + if not (user and user.is_authenticated): + return False + return user.is_staff or self.is_admin(user) + + def is_last_admin(self, user_profile): + """True if no other active admin would remain without `user_profile`. Every org keeps at least one.""" + return self.is_admin(user_profile) and not self.memberships.filter( + role_id=ROLE_ADMIN_ID, userprofile__is_active=True).exclude(userprofile_id=user_profile.id).exists() + + def get_member_role(self, user_profile): + """The name of the role `user_profile` has in the org, or None if they aren't a member.""" + return self.memberships.filter( + userprofile_id=get(user_profile, 'id')).values_list('role__name', flat=True).first() + + @property + def member_roles(self): + return dict(self.memberships.values_list('userprofile_id', 'role__name')) + + def set_member_role(self, user_profile, role=None): + """ + Adds `user_profile` to the org or changes their role, and returns whether they're new to it. With no role, + a new member gets the default one and an existing member keeps theirs. + """ + _, created = self.memberships.update_or_create( + userprofile=user_profile, defaults={'role_id': ROLE_ID_BY_NAME[role]} if role else {}) + return created + def is_only_member(self, user_profile): """True if `user_profile` is the org's only member. Every org keeps at least one member.""" return bool(self.is_member(user_profile)) and not self.members.exclude(id=user_profile.id).exists() @@ -98,14 +152,14 @@ def get_visible_to(cls, user, queryset): def save(self, *args, force_insert=False, force_update=False, using=None, update_fields=None): """ - The creator becomes a member when the org is created. Later saves leave the members alone, so editing + The creator becomes an admin when the org is created. Later saves leave the members alone, so editing an org doesn't make the editor a member and removing a member sticks. """ is_new = not self.id super().save(*args, force_insert=force_insert, force_update=force_update, using=using, update_fields=update_fields) if is_new: - self.members.add(self.created_by) + self.members.add(self.created_by, through_defaults={'role_id': ROLE_ADMIN_ID}) def delete(self, using=None, keep_parents=False, sync=False): # pylint: disable=arguments-differ with transaction.atomic(): diff --git a/core/orgs/serializers.py b/core/orgs/serializers.py index b12f8db37..c2a52b767 100644 --- a/core/orgs/serializers.py +++ b/core/orgs/serializers.py @@ -91,6 +91,7 @@ class OrganizationDetailSerializer(AbstractResourceSerializer): members_url = serializers.URLField(read_only=True) extras = serializers.JSONField(required=False, allow_null=True) client_configs = SerializerMethodField() + user_role = SerializerMethodField() class Meta: model = Organization @@ -98,7 +99,7 @@ class Meta: 'type', 'uuid', 'id', 'public_access', 'name', 'company', 'website', 'location', 'members', 'created_on', 'updated_on', 'url', 'extras', 'members_url', 'created_by', 'updated_by', 'location', 'sources_url', 'public_sources', 'collections_url', 'public_collections', 'logo_url', 'description', - 'client_configs', 'text', 'overview' + 'client_configs', 'text', 'overview', 'user_role' ) def __init__(self, *args, **kwargs): @@ -119,6 +120,10 @@ def get_client_configs(self, obj): return ClientConfigSerializer(obj.client_configs.filter( is_active=True), many=True).data if self.include_client_configs else None + def get_user_role(self, obj): + """The requester's role in the org, for clients to decide which controls to show.""" + return obj.get_member_role(get(self, 'context.request.user')) + def update(self, instance, validated_data): request_user = self.context['request'].user instance.description = validated_data.get('description', instance.description) diff --git a/core/orgs/tests/tests.py b/core/orgs/tests/tests.py index 61a5eec05..e985d8b87 100644 --- a/core/orgs/tests/tests.py +++ b/core/orgs/tests/tests.py @@ -3,6 +3,7 @@ from django.apps import apps from django.contrib.auth.models import AnonymousUser from django.core.exceptions import ValidationError +from django.db.models import ProtectedError from mock import patch, Mock from core.collections.models import Collection, CollectionReference, Expansion @@ -13,9 +14,9 @@ from core.concepts.tests.factories import ConceptFactory from core.mappings.models import Mapping from core.mappings.tests.factories import MappingFactory -from core.orgs.constants import ORG_OBJECT_TYPE +from core.orgs.constants import ORG_OBJECT_TYPE, ROLE_ADMIN, ROLE_MEMBER from core.orgs.documents import OrganizationDocument -from core.orgs.models import Organization +from core.orgs.models import Organization, Role from core.orgs.tests.factories import OrganizationFactory from core.sources.models import Source from core.sources.tests.factories import OrganizationSourceFactory @@ -52,6 +53,99 @@ def test_has_edit_access(self): for user in (member, outsider): self.assertEqual(source.has_parent_edit_access(user), org.has_edit_access(user)) + def test_creator_is_an_admin(self): + creator = UserProfileFactory() + org = OrganizationFactory(created_by=creator, updated_by=creator) + + self.assertTrue(org.is_member(creator)) + self.assertTrue(org.is_admin(creator)) + + def test_saving_again_does_not_change_the_role_of_the_creator(self): + creator = UserProfileFactory() + org = OrganizationFactory(created_by=creator, updated_by=creator) + org.set_member_role(creator, ROLE_MEMBER) + + org.save() + + self.assertFalse(org.is_admin(creator)) + + def test_a_member_added_directly_is_not_an_admin(self): + org = OrganizationFactory() + member = UserProfileFactory() + + org.members.add(member) + + self.assertTrue(org.is_member(member)) + self.assertFalse(org.is_admin(member)) + + def test_has_admin_access(self): + org = OrganizationFactory() + member, admin, outsider = UserProfileFactory(), UserProfileFactory(), UserProfileFactory() + org.members.add(member) + org.set_member_role(admin, ROLE_ADMIN) + + self.assertTrue(org.has_admin_access(admin)) + self.assertTrue(org.has_admin_access(UserProfileFactory(is_staff=True))) + self.assertFalse(org.has_admin_access(member)) + self.assertFalse(org.has_admin_access(outsider)) + self.assertFalse(org.has_admin_access(AnonymousUser())) + self.assertFalse(org.has_admin_access(None)) + + def test_is_last_admin(self): + creator = UserProfileFactory() + org = OrganizationFactory(created_by=creator, updated_by=creator) + member = UserProfileFactory(organizations=[org]) + + self.assertTrue(org.is_last_admin(creator)) + self.assertFalse(org.is_last_admin(member)) + self.assertFalse(org.is_last_admin(UserProfileFactory())) + self.assertFalse(org.is_last_admin(None)) + + org.set_member_role(member, ROLE_ADMIN) + + self.assertFalse(org.is_last_admin(creator)) + self.assertFalse(org.is_last_admin(member)) + + def test_is_last_admin_ignores_deactivated_admins(self): + creator = UserProfileFactory() + org = OrganizationFactory(created_by=creator, updated_by=creator) + deactivated = UserProfileFactory(is_active=False) + org.set_member_role(deactivated, ROLE_ADMIN) + + self.assertTrue(org.is_last_admin(creator)) + self.assertFalse(org.is_last_admin(deactivated)) + + def test_get_member_role(self): + creator = UserProfileFactory() + org = OrganizationFactory(created_by=creator, updated_by=creator) + member = UserProfileFactory(organizations=[org]) + + self.assertEqual(org.get_member_role(creator), ROLE_ADMIN) + self.assertEqual(org.get_member_role(member), ROLE_MEMBER) + self.assertIsNone(org.get_member_role(UserProfileFactory())) + self.assertIsNone(org.get_member_role(AnonymousUser())) + self.assertIsNone(org.get_member_role(None)) + + def test_set_member_role(self): + org = OrganizationFactory() + user = UserProfileFactory() + + self.assertTrue(org.set_member_role(user)) + self.assertEqual(org.get_member_role(user), ROLE_MEMBER) + + self.assertFalse(org.set_member_role(user, ROLE_ADMIN)) + self.assertEqual(org.get_member_role(user), ROLE_ADMIN) + + self.assertFalse(org.set_member_role(user)) + self.assertEqual(org.get_member_role(user), ROLE_ADMIN) + + def test_a_membership_in_use_keeps_its_role(self): + org = OrganizationFactory() + org.members.add(UserProfileFactory()) + + with self.assertRaises(ProtectedError): + Role.objects.get(name=ROLE_MEMBER).delete() + def test_members(self): org = Organization(id=123) self.assertEqual(org.members.count(), 0) diff --git a/core/orgs/views.py b/core/orgs/views.py index 28175aab2..732e2db45 100644 --- a/core/orgs/views.py +++ b/core/orgs/views.py @@ -13,14 +13,15 @@ from core.collections.views import CollectionListView from core.common.constants import NOT_FOUND, MUST_SPECIFY_EXTRA_PARAM_IN_BODY, HEAD, UPDATED_BY_USERNAME_PARAM from core.common.mixins import ListWithHeadersMixin -from core.common.permissions import HasPrivateAccess, CanViewConceptDictionary +from core.common.permissions import CanAdministerOrganization, CanViewConceptDictionary, HasPrivateAccess from core.common.swagger_parameters import org_no_members_param from core.common.tasks import delete_organization from core.common.throttling import ThrottleUtil from core.common.utils import parse_updated_since_param, get_truthy_values from core.common.views import BaseAPIView, BaseLogoView from core.map_projects.views import MapProjectListView -from core.orgs.constants import NO_MEMBERS, CANNOT_REMOVE_ONLY_MEMBER +from core.orgs.constants import NO_MEMBERS, CANNOT_REMOVE_ONLY_MEMBER, CANNOT_REMOVE_LAST_ADMIN, INVALID_ROLE, \ + ROLES, ROLE_MEMBER from core.orgs.documents import OrganizationDocument from core.orgs.models import Organization from core.orgs.serializers import OrganizationDetailSerializer, OrganizationListSerializer, \ @@ -109,11 +110,16 @@ class OrganizationBaseView(BaseAPIView, RetrieveAPIView, DestroyAPIView): queryset = Organization.objects.filter(is_active=True) def get_permissions(self): - """Reads follow the org's public access. Writes need staff or membership of the org.""" + """ + Reads follow the org's public access. Writes need staff or an admin of the org. Editing one of the org's + repositories looks the org up here with the same request, and needs only a member. + """ if self.request.method in ['GET', 'HEAD']: return [CanViewConceptDictionary(), ] + if self.is_parent_lookup: + return [HasPrivateAccess(), ] - return [HasPrivateAccess(), ] + return [CanAdministerOrganization(), ] class OrganizationLogoView(OrganizationBaseView, BaseLogoView): @@ -193,12 +199,13 @@ class OrganizationClientConfigsView(ResourceClientConfigsView): lookup_field = 'org' model = Organization queryset = Organization.objects.filter(is_active=True) - edit_permission_class = HasPrivateAccess + edit_permission_class = CanAdministerOrganization class OrganizationMemberView(generics.GenericAPIView): userprofile = None user_in_org = False + user_is_admin = False serializer_class = UserDetailSerializer def get_throttles(self): @@ -218,6 +225,7 @@ def initial(self, request, *args, **kwargs): self.user_in_org = request.user.is_staff or ( request.user.is_authenticated and self.organization.is_member(request.user) ) + self.user_is_admin = self.organization.has_admin_access(request.user) except UserProfile.DoesNotExist: # pragma: no cover pass super().initial(request, *args, **kwargs) @@ -233,32 +241,50 @@ def get(self, request, *args, **kwargs): return Response(status=status.HTTP_404_NOT_FOUND) # pragma: no cover def put(self, request, **kwargs): # pylint: disable=unused-argument + """Adds the user to the org or changes their role. With no role, an existing member keeps theirs.""" if not self.userprofile: return Response(status=status.HTTP_404_NOT_FOUND) - if not self.user_in_org: + if not self.user_is_admin: return Response(status=status.HTTP_403_FORBIDDEN) - self.userprofile.organizations.add(self.organization) - # ES Index - self.organization.save() - self.userprofile.save() - from core.events.models import Event - Event.record(self.organization, Event.JOINED, self.userprofile, request.user) + role = get(request.data, 'role') + if role is not None and role not in ROLES: + return Response({'role': [INVALID_ROLE]}, status=status.HTTP_400_BAD_REQUEST) + + with transaction.atomic(): + # Locks the org so that concurrent changes can't each see another admin and leave it with none. + Organization.objects.select_for_update().get(id=self.organization.id) + if role == ROLE_MEMBER and self.organization.is_last_admin(self.userprofile): + return Response({'detail': CANNOT_REMOVE_LAST_ADMIN}, status=status.HTTP_409_CONFLICT) + joined = self.organization.set_member_role(self.userprofile, role) + + if joined: + # ES Index + self.organization.save() + self.userprofile.save() + from core.events.models import Event + Event.record(self.organization, Event.JOINED, self.userprofile, request.user) return Response(status=status.HTTP_204_NO_CONTENT) def delete(self, request, **kwargs): # pylint: disable=unused-argument - """Removes the user from the org, unless they are its only member: every org keeps at least one.""" - if not request.user.is_staff and not self.user_in_org: + """ + Removes the user from the org. An admin can remove anyone and a member can leave, unless that would leave + the org with no member or no admin. + """ + is_leaving = self.user_in_org and request.user == self.userprofile + if not request.user.is_staff and not self.user_is_admin and not is_leaving: return Response(status=status.HTTP_403_FORBIDDEN) if not self.organization or not self.userprofile: return Response(status=status.HTTP_404_NOT_FOUND) with transaction.atomic(): - # Locks the org so that concurrent removals can't each see another member and leave it with none. + # Locks the org so that concurrent changes can't each see another member or admin and leave it with none. Organization.objects.select_for_update().get(id=self.organization.id) if self.organization.is_only_member(self.userprofile): return Response({'detail': CANNOT_REMOVE_ONLY_MEMBER}, status=status.HTTP_409_CONFLICT) + if self.organization.is_last_admin(self.userprofile): + return Response({'detail': CANNOT_REMOVE_LAST_ADMIN}, status=status.HTTP_409_CONFLICT) self.userprofile.organizations.remove(self.organization) # ES Index @@ -306,11 +332,11 @@ def get_throttles(self): return ThrottleUtil.get_throttles_by_user_plan(self.request.user) def get_permissions(self): - """Reads follow the org's public access. Writes need staff or membership of the org.""" + """Reads follow the org's public access. Writes need staff or an admin of the org.""" if self.request.method in ['GET', 'HEAD']: return [IsAuthenticated(), CanViewConceptDictionary()] - return [IsAuthenticated(), HasPrivateAccess()] + return [IsAuthenticated(), CanAdministerOrganization()] def get_object(self): instance = Organization.objects.filter(is_active=True, mnemonic=self.kwargs['org']).first() diff --git a/core/users/migrations/0038_alter_userprofile_organizations.py b/core/users/migrations/0038_alter_userprofile_organizations.py new file mode 100644 index 000000000..9c8f59266 --- /dev/null +++ b/core/users/migrations/0038_alter_userprofile_organizations.py @@ -0,0 +1,23 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('orgs', '0023_role_organizationmember'), + ('users', '0037_userprofile_sso_id'), + ] + + operations = [ + # The table doesn't change: orgs 0023 has already taken it over as OrganizationMember. + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.AlterField( + model_name='userprofile', + name='organizations', + field=models.ManyToManyField( + related_name='members', through='orgs.OrganizationMember', to='orgs.organization'), + ), + ], + ), + ] diff --git a/core/users/models.py b/core/users/models.py index 947561ad4..f5165f823 100644 --- a/core/users/models.py +++ b/core/users/models.py @@ -56,7 +56,8 @@ class Meta: OBJECT_TYPE = USER_OBJECT_TYPE first_name = models.CharField(max_length=100, blank=True) last_name = models.CharField(max_length=100, blank=True) - organizations = models.ManyToManyField('orgs.Organization', related_name='members') + organizations = models.ManyToManyField( + 'orgs.Organization', related_name='members', through='orgs.OrganizationMember') company = models.TextField(null=True, blank=True) sso_id = models.CharField(max_length=64, null=True, blank=True, unique=True) location = models.TextField(null=True, blank=True) @@ -170,6 +171,7 @@ def set_token(self, token): Token.objects.create(user=self, key=token) def is_admin_for(self, concept_container): # pragma: no cover + """Whether the user owns a repository or is any member of the org that does. Not an org's admin role.""" user_id = concept_container.user_id if user_id and user_id == self.id: return True diff --git a/core/users/serializers.py b/core/users/serializers.py index 9c8f26a34..d55f91f44 100644 --- a/core/users/serializers.py +++ b/core/users/serializers.py @@ -39,7 +39,17 @@ def to_representation(self, instance): return data -class UserListSerializer(AbstractResourceSerializer): +class OrganizationRoleMixin: + """On an org's member list, each member's role in that org.""" + def to_representation(self, instance): + data = super().to_representation(instance) + member_roles = get(self, 'context.member_roles') + if member_roles is not None: + data['role'] = member_roles.get(instance.id) + return data + + +class UserListSerializer(OrganizationRoleMixin, AbstractResourceSerializer): type = serializers.CharField(source='resource_type', read_only=True) class Meta: @@ -49,7 +59,7 @@ class Meta: ) -class UserSummarySerializer(PrivateUserFieldsMixin, serializers.ModelSerializer): +class UserSummarySerializer(OrganizationRoleMixin, PrivateUserFieldsMixin, serializers.ModelSerializer): sources = IntegerField(source='public_sources') collections = IntegerField(source='public_collections') organizations = IntegerField(source='orgs_count') @@ -174,7 +184,7 @@ def get_object(obj): return following.get_brief_serializer()(following).data -class UserDetailSerializer(PrivateUserFieldsMixin, AbstractResourceSerializer): +class UserDetailSerializer(OrganizationRoleMixin, PrivateUserFieldsMixin, AbstractResourceSerializer): type = serializers.CharField(source='resource_type', read_only=True) uuid = serializers.CharField(source='id', read_only=True) username = serializers.CharField(required=False, validators=[ diff --git a/core/users/views.py b/core/users/views.py index e4eb50c26..51a2ffae8 100644 --- a/core/users/views.py +++ b/core/users/views.py @@ -249,6 +249,7 @@ class UserListView(UserBaseView, ListWithHeadersMixin, mixins.CreateModelMixin): allow_csv_export = False # user rows carry private account fields + organization = None def get_serializer_class(self): if self.request.query_params.get('summary') in TRUTHY and self.request.method == 'GET': @@ -265,6 +266,12 @@ def get_permissions(self): return [IsAdminUser()] return [] + def get_serializer_context(self): + context = super().get_serializer_context() + if self.organization: + context['member_roles'] = self.organization.member_roles + return context + def can_view(self, organization): user = self.request.user return organization.public_can_view or user.is_staff or organization.is_member(user) @@ -284,6 +291,7 @@ def get(self, request, *args, **kwargs): if not self.can_view(organization): return Response(status=status.HTTP_403_FORBIDDEN) + self.organization = organization queryset = organization.members if not self.should_include_inactive(): queryset = queryset.filter(is_active=True) From 733ee9f90f18d9c80fef0c6efcd12814e9608586 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 4 Nov 2026 06:49:23 -0500 Subject: [PATCH 2/3] OpenConceptLab/ocl_online#648 | Rights are read under the org's lock, a hard delete keeps the last admin A request that waited for the org's lock now uses the rights its sender has once it gets the lock, so an admin being demoted can't promote themselves back. A user hard delete is refused while they're the last active admin of an org. Deactivating stays possible, as it's how staff block an account. A membership row inserted without a role is an admin, which is what code older than the column means by a member. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AaZ4qUSJnKeDYRubKa6DYK --- core/integration_tests/tests_orgs.py | 69 ++++++++++++++++--- core/integration_tests/tests_users.py | 37 ++++++++++ .../0023_role_organizationmember.py | 10 +-- core/orgs/models.py | 4 +- core/orgs/tests/tests.py | 14 ++++ core/orgs/views.py | 22 +++--- core/users/models.py | 5 ++ core/users/views.py | 7 ++ 8 files changed, 136 insertions(+), 32 deletions(-) diff --git a/core/integration_tests/tests_orgs.py b/core/integration_tests/tests_orgs.py index a45af9cfe..b201f1410 100644 --- a/core/integration_tests/tests_orgs.py +++ b/core/integration_tests/tests_orgs.py @@ -596,7 +596,7 @@ def test_delete_409_only_member_by_staff(self): def test_delete_404(self): response = self.client.delete( f'/orgs/{self.org.mnemonic}/members/foobar/', - HTTP_AUTHORIZATION='Token ' + self.superuser.get_token(), + HTTP_AUTHORIZATION='Token ' + self.token, format='json' ) @@ -607,7 +607,7 @@ def test_delete_404(self): class OrganizationMemberRemovalConcurrencyTest(OCLAPITransactionTestCase): """Runs two removals at once, on separate connections, to check that every org keeps a member.""" - def test_two_members_removing_each_other_at_once_leave_one_member(self): + def test_two_members_leaving_at_once_leave_one_member(self): alice = UserProfileFactory(username='alice') bob = UserProfileFactory(username='bob') org = OrganizationFactory(mnemonic='race-org', created_by=alice, updated_by=alice) @@ -623,20 +623,20 @@ def slow_is_only_member(organization, user_profile): barrier = threading.Barrier(2) statuses = {} - def remove(actor, target): + def leave(actor): try: client = APIClient() barrier.wait() response = client.delete( - f'/orgs/race-org/members/{target}/', HTTP_AUTHORIZATION='Token ' + tokens[actor]) + f'/orgs/race-org/members/{actor}/', HTTP_AUTHORIZATION='Token ' + tokens[actor]) statuses[actor] = response.status_code finally: connection.close() with patch.object(Organization, 'is_only_member', slow_is_only_member): threads = [ - threading.Thread(target=remove, args=('alice', 'bob')), - threading.Thread(target=remove, args=('bob', 'alice')), + threading.Thread(target=leave, args=('alice', )), + threading.Thread(target=leave, args=('bob', )), ] for thread in threads: thread.start() @@ -1042,7 +1042,7 @@ def test_search_results_have_each_role(self): class OrganizationAdminConcurrencyTest(OCLAPITransactionTestCase): """Runs two demotions at once, on separate connections, to check that every org keeps an admin.""" - def test_two_admins_demoting_each_other_at_once_leave_one_admin(self): + def test_two_admins_demoting_themselves_at_once_leave_one_admin(self): alice = UserProfileFactory(username='alice') bob = UserProfileFactory(username='bob') org = OrganizationFactory(mnemonic='race-org', created_by=alice, updated_by=alice) @@ -1058,12 +1058,12 @@ def slow_is_last_admin(organization, user_profile): barrier = threading.Barrier(2) statuses = {} - def demote(actor, target): + def demote_self(actor): try: client = APIClient() barrier.wait() response = client.put( - f'/orgs/race-org/members/{target}/', {'role': ROLE_MEMBER}, format='json', + f'/orgs/race-org/members/{actor}/', {'role': ROLE_MEMBER}, format='json', HTTP_AUTHORIZATION='Token ' + tokens[actor]) statuses[actor] = response.status_code finally: @@ -1071,8 +1071,8 @@ def demote(actor, target): with patch.object(Organization, 'is_last_admin', slow_is_last_admin): threads = [ - threading.Thread(target=demote, args=('alice', 'bob')), - threading.Thread(target=demote, args=('bob', 'alice')), + threading.Thread(target=demote_self, args=('alice', )), + threading.Thread(target=demote_self, args=('bob', )), ] for thread in threads: thread.start() @@ -1083,6 +1083,53 @@ def demote(actor, target): self.assertEqual(org.memberships.filter(role__name=ROLE_ADMIN).count(), 1) +class OrganizationDemotedAdminConcurrencyTest(OCLAPITransactionTestCase): + """A request that was waiting for the org's lock uses the rights its sender has once it gets the lock.""" + + def test_an_admin_being_demoted_cannot_promote_themselves_back(self): + alice = UserProfileFactory(username='alice') + bob = UserProfileFactory(username='bob') + org = OrganizationFactory(mnemonic='race-org', created_by=alice, updated_by=alice) + org.set_member_role(bob, ROLE_ADMIN) + tokens = {'alice': alice.get_token(), 'bob': bob.get_token()} + is_last_admin = Organization.is_last_admin + alice_holds_the_lock = threading.Event() + + def slow_is_last_admin(organization, user_profile): + result = is_last_admin(organization, user_profile) + alice_holds_the_lock.set() + time.sleep(1) # bob's request starts and waits for the lock + return result + + statuses = {} + + def send(actor, role): + try: + response = APIClient().put( + '/orgs/race-org/members/bob/', {'role': role}, format='json', + HTTP_AUTHORIZATION='Token ' + tokens[actor]) + statuses[actor] = response.status_code + finally: + connection.close() + + def promote_once_alice_holds_the_lock(): + alice_holds_the_lock.wait(timeout=5) + send('bob', ROLE_ADMIN) + + with patch.object(Organization, 'is_last_admin', slow_is_last_admin): + threads = [ + threading.Thread(target=send, args=('alice', ROLE_MEMBER)), + threading.Thread(target=promote_once_alice_holds_the_lock), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + self.assertEqual(statuses, {'alice': 204, 'bob': 403}) + self.assertEqual(org.get_member_role(bob), ROLE_MEMBER) + + class OrganizationExtrasViewTest(OCLAPITestCase): def test_get_200(self): org = OrganizationFactory(extras={}) diff --git a/core/integration_tests/tests_users.py b/core/integration_tests/tests_users.py index 7eeaba2ac..b730e6ce4 100644 --- a/core/integration_tests/tests_users.py +++ b/core/integration_tests/tests_users.py @@ -8,6 +8,7 @@ from core.common.constants import ACCESS_TYPE_NONE, ACCESS_TYPE_VIEW, RETIRED_ACCESS_TYPE_EDIT from core.common.tests import OCLAPITestCase, PREVIEW_GROUP_NAME +from core.orgs.constants import ROLE_ADMIN, ROLE_MEMBER from core.orgs.documents import OrganizationDocument from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import OrganizationSourceFactory @@ -981,6 +982,42 @@ def test_hard_delete_204(self): self.assertFalse(UserProfile.objects.filter(username=random_user.username).exists()) +class UserHardDeleteOrganizationAdminTest(OCLAPITestCase): + def setUp(self): + super().setUp() + self.staff = UserProfileFactory(is_staff=True) + self.member = UserProfileFactory() + self.org = OrganizationFactory(mnemonic='stark', created_by=self.member, updated_by=self.member) + self.org.set_member_role(self.member, ROLE_MEMBER) + self.user = UserProfileFactory() + self.org.set_member_role(self.user, ROLE_ADMIN) + + def delete(self, params='?hardDelete=true'): + return self.client.delete( + f'/users/{self.user.username}/{params}', HTTP_AUTHORIZATION='Token ' + self.staff.get_token()) + + def test_refuses_the_last_admin_of_an_org(self): + response = self.delete() + + self.assertEqual(response.status_code, 409) + self.assertEqual(response.data, { + 'detail': 'Cannot remove or demote the last admin of an organization.', 'orgs': ['stark'] + }) + self.assertTrue(UserProfile.objects.filter(id=self.user.id).exists()) + + def test_allows_an_admin_when_another_remains(self): + self.org.set_member_role(self.member, ROLE_ADMIN) + + self.assertEqual(self.delete().status_code, 204) + self.assertFalse(UserProfile.objects.filter(id=self.user.id).exists()) + + def test_deactivating_the_last_admin_stays_possible(self): + self.assertEqual(self.delete(params='').status_code, 204) + self.user.refresh_from_db() + self.assertFalse(self.user.is_active) + self.assertTrue(self.org.is_admin(self.user)) + + class UserReactivateViewTest(OCLAPITestCase): def setUp(self): super().setUp() diff --git a/core/orgs/migrations/0023_role_organizationmember.py b/core/orgs/migrations/0023_role_organizationmember.py index b1fbe7af2..fa1613b2d 100644 --- a/core/orgs/migrations/0023_role_organizationmember.py +++ b/core/orgs/migrations/0023_role_organizationmember.py @@ -56,15 +56,7 @@ class Migration(migrations.Migration): model_name='organizationmember', name='role', field=models.ForeignKey( - default=1, on_delete=django.db.models.deletion.PROTECT, related_name='memberships', to='orgs.role'), - ), - # From here on a new member is a plain member. The column keeps that default for inserts from code that - # doesn't know the column yet, as during a rollout or a rollback. - migrations.AlterField( - model_name='organizationmember', - name='role', - field=models.ForeignKey( - db_default=2, default=2, on_delete=django.db.models.deletion.PROTECT, related_name='memberships', + db_default=1, default=2, on_delete=django.db.models.deletion.PROTECT, related_name='memberships', to='orgs.role'), ), ] diff --git a/core/orgs/models.py b/core/orgs/models.py index 53a78086a..026c037dd 100644 --- a/core/orgs/models.py +++ b/core/orgs/models.py @@ -29,8 +29,10 @@ class Meta: userprofile = models.ForeignKey('users.UserProfile', on_delete=models.CASCADE, related_name='org_memberships') organization = models.ForeignKey('orgs.Organization', on_delete=models.CASCADE, related_name='memberships') + # A new member is a plain member. Only a row inserted without a role gets the column's default: code older than + # this column, as during a rollout or after a rollback, where any member could do everything an admin can. role = models.ForeignKey( - Role, on_delete=models.PROTECT, related_name='memberships', default=ROLE_MEMBER_ID, db_default=ROLE_MEMBER_ID) + Role, on_delete=models.PROTECT, related_name='memberships', default=ROLE_MEMBER_ID, db_default=ROLE_ADMIN_ID) class Organization(BaseResourceModel, SourceContainerMixin, ChecksumModel): diff --git a/core/orgs/tests/tests.py b/core/orgs/tests/tests.py index e985d8b87..5a574588c 100644 --- a/core/orgs/tests/tests.py +++ b/core/orgs/tests/tests.py @@ -3,6 +3,7 @@ from django.apps import apps from django.contrib.auth.models import AnonymousUser from django.core.exceptions import ValidationError +from django.db import connection from django.db.models import ProtectedError from mock import patch, Mock @@ -139,6 +140,19 @@ def test_set_member_role(self): self.assertFalse(org.set_member_role(user)) self.assertEqual(org.get_member_role(user), ROLE_ADMIN) + def test_an_insert_that_names_no_role_is_an_admin(self): + """Code older than the role column inserts this way, during a rollout or after a rollback.""" + org = OrganizationFactory() + user = UserProfileFactory() + + with connection.cursor() as cursor: + cursor.execute( + 'INSERT INTO user_profiles_organizations (userprofile_id, organization_id) VALUES (%s, %s)', + [user.id, org.id] + ) + + self.assertTrue(org.is_admin(user)) + def test_a_membership_in_use_keeps_its_role(self): org = OrganizationFactory() org.members.add(UserProfileFactory()) diff --git a/core/orgs/views.py b/core/orgs/views.py index 732e2db45..9f3560cf8 100644 --- a/core/orgs/views.py +++ b/core/orgs/views.py @@ -205,7 +205,6 @@ class OrganizationClientConfigsView(ResourceClientConfigsView): class OrganizationMemberView(generics.GenericAPIView): userprofile = None user_in_org = False - user_is_admin = False serializer_class = UserDetailSerializer def get_throttles(self): @@ -225,7 +224,6 @@ def initial(self, request, *args, **kwargs): self.user_in_org = request.user.is_staff or ( request.user.is_authenticated and self.organization.is_member(request.user) ) - self.user_is_admin = self.organization.has_admin_access(request.user) except UserProfile.DoesNotExist: # pragma: no cover pass super().initial(request, *args, **kwargs) @@ -244,16 +242,16 @@ def put(self, request, **kwargs): # pylint: disable=unused-argument """Adds the user to the org or changes their role. With no role, an existing member keeps theirs.""" if not self.userprofile: return Response(status=status.HTTP_404_NOT_FOUND) - if not self.user_is_admin: - return Response(status=status.HTTP_403_FORBIDDEN) role = get(request.data, 'role') - if role is not None and role not in ROLES: - return Response({'role': [INVALID_ROLE]}, status=status.HTTP_400_BAD_REQUEST) - with transaction.atomic(): - # Locks the org so that concurrent changes can't each see another admin and leave it with none. + # Locks the org so that concurrent changes can't each see another admin and leave it with none, or act on + # rights their sender has just lost. Organization.objects.select_for_update().get(id=self.organization.id) + if not self.organization.has_admin_access(request.user): + return Response(status=status.HTTP_403_FORBIDDEN) + if role is not None and role not in ROLES: + return Response({'role': [INVALID_ROLE]}, status=status.HTTP_400_BAD_REQUEST) if role == ROLE_MEMBER and self.organization.is_last_admin(self.userprofile): return Response({'detail': CANNOT_REMOVE_LAST_ADMIN}, status=status.HTTP_409_CONFLICT) joined = self.organization.set_member_role(self.userprofile, role) @@ -272,15 +270,17 @@ def delete(self, request, **kwargs): # pylint: disable=unused-argument Removes the user from the org. An admin can remove anyone and a member can leave, unless that would leave the org with no member or no admin. """ - is_leaving = self.user_in_org and request.user == self.userprofile - if not request.user.is_staff and not self.user_is_admin and not is_leaving: + if not request.user.is_staff and not self.user_in_org: return Response(status=status.HTTP_403_FORBIDDEN) if not self.organization or not self.userprofile: return Response(status=status.HTTP_404_NOT_FOUND) with transaction.atomic(): - # Locks the org so that concurrent changes can't each see another member or admin and leave it with none. + # Locks the org so that concurrent changes can't each see another member or admin and leave it with none, + # or act on rights their sender has just lost. Organization.objects.select_for_update().get(id=self.organization.id) + if request.user != self.userprofile and not self.organization.has_admin_access(request.user): + return Response(status=status.HTTP_403_FORBIDDEN) if self.organization.is_only_member(self.userprofile): return Response({'detail': CANNOT_REMOVE_ONLY_MEMBER}, status=status.HTTP_409_CONFLICT) if self.organization.is_last_admin(self.userprofile): diff --git a/core/users/models.py b/core/users/models.py index f5165f823..21f339a90 100644 --- a/core/users/models.py +++ b/core/users/models.py @@ -189,6 +189,11 @@ def __delete_token(self): def orgs_count(self): return self.organizations.count() + @property + def orgs_needing_this_admin(self): + """The orgs that would be left with no active admin without this user.""" + return [org for org in self.organizations.all() if org.is_last_admin(self)] + @property def owned_orgs_count(self): return self.organizations.filter(created_by=self).count() diff --git a/core/users/views.py b/core/users/views.py index 51a2ffae8..ff85f8c20 100644 --- a/core/users/views.py +++ b/core/users/views.py @@ -28,6 +28,7 @@ from core.common.utils import parse_updated_since_param, from_string_to_date, get_truthy_values, \ get_resource_class_from_resource_uri from core.common.views import BaseAPIView, BaseLogoView +from core.orgs.constants import CANNOT_REMOVE_LAST_ADMIN from core.orgs.models import Organization from core.services.auth.core import AuthService from core.services.auth.openid import OpenIDAuthService @@ -485,6 +486,12 @@ def delete(self, request, *args, **kwargs): if self.user_is_self: return Response(status=status.HTTP_405_METHOD_NOT_ALLOWED) if self.is_hard_delete_requested(): + orgs = obj.orgs_needing_this_admin + if orgs: + return Response( + {'detail': CANNOT_REMOVE_LAST_ADMIN, 'orgs': [org.mnemonic for org in orgs]}, + status=status.HTTP_409_CONFLICT + ) try: obj.delete() except Exception as ex: From e735a2187ef1e85d4bac5870ff16b09be5d49b41 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 4 Nov 2026 07:08:04 -0500 Subject: [PATCH 3/3] OpenConceptLab/ocl_online#648 | Cut tests that repeat each other, and unused related names Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AaZ4qUSJnKeDYRubKa6DYK --- core/importers/tests.py | 6 - .../integration_tests/tests_access_control.py | 5 + core/integration_tests/tests_orgs.py | 212 +----------------- .../0023_role_organizationmember.py | 6 +- core/orgs/models.py | 8 +- core/orgs/tests/tests.py | 85 +------ 6 files changed, 19 insertions(+), 303 deletions(-) diff --git a/core/importers/tests.py b/core/importers/tests.py index 24d26a326..8b1580aba 100644 --- a/core/importers/tests.py +++ b/core/importers/tests.py @@ -2102,12 +2102,6 @@ def test_organization_importer_delete_by_an_admin(self): self.assertEqual(OrganizationImporter({'id': 'AdminOrg'}, admin).delete(), DELETED) self.assertFalse(Organization.objects.filter(mnemonic='AdminOrg').exists()) - def test_organization_importer_makes_the_creator_an_admin(self): - user = UserProfileFactory() - - self.assertEqual(OrganizationImporter({'id': 'ImportedOrg', 'name': 'Imported'}, user).run(), CREATED) - self.assertTrue(Organization.objects.get(mnemonic='ImportedOrg').is_admin(user)) - def test_organization_importer_does_not_create_org_whose_name_differs_only_in_case(self): OrganizationFactory(mnemonic='TwinOrg') importer = OrganizationImporter({'id': 'twinorg', 'name': 'Twin'}, UserProfileFactory()) diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 399bbe1ed..d6eb447e1 100644 --- a/core/integration_tests/tests_access_control.py +++ b/core/integration_tests/tests_access_control.py @@ -73,6 +73,11 @@ def test_anonymous_cannot_edit(self): def test_member_cannot_edit(self): self.assert_not_renamed(self.rename(self.member), 403) + self.assert_not_renamed(self.request('patch', self.org.uri, self.member, {'name': 'Renamed'}), 403) + + def test_member_cannot_delete(self): + self.assertEqual(self.request('delete', self.org.uri + '?inline=true', self.member).status_code, 403) + self.assertTrue(Organization.objects.filter(id=self.org.id).exists()) def test_admin_can_edit(self): self.assert_renamed(self.rename(self.admin)) diff --git a/core/integration_tests/tests_orgs.py b/core/integration_tests/tests_orgs.py index b201f1410..a778a0318 100644 --- a/core/integration_tests/tests_orgs.py +++ b/core/integration_tests/tests_orgs.py @@ -3,7 +3,6 @@ from django.db import connection from django.test import override_settings, tag -from django.test.utils import CaptureQueriesContext from mock import patch from mock.mock import Mock, ANY from rest_framework.exceptions import ErrorDetail @@ -647,61 +646,6 @@ def leave(actor): self.assertEqual(org.members.count(), 1) -class OrganizationAdminOnlyTest(OCLAPITestCase): - """Editing or deleting an org needs an admin of it, or staff. A plain member gets a 403 and changes nothing.""" - ACTIONS = ( - ('put', '/orgs/stark/', {'name': 'Wayne'}, 200), - ('patch', '/orgs/stark/', {'name': 'Wayne'}, 200), - ('post', '/orgs/stark/logo/', {'base64': 'base64-data'}, 200), - ('delete', '/orgs/stark/logo/', None, 204), - ('put', '/orgs/stark/overview/', {'overview': {'foo': 'bar'}}, 200), - ('delete', '/orgs/stark/overview/', None, 204), - ('put', '/orgs/stark/extras/foo/', {'foo': 'baz'}, 200), - ('delete', '/orgs/stark/extras/foo/', None, 204), - ('post', '/orgs/stark/client-configs/', {'name': 'custom', 'config': {'tabs': [{'default': True}]}}, 201), - ('delete', '/orgs/stark/?inline=true', None, 204), - ) - - def setUp(self): - super().setUp() - self.org = OrganizationFactory( - mnemonic='stark', name='Stark', extras={'foo': 'bar'}, overview={'foo': 'bar'}, logo_path='logo.png') - self.member = UserProfileFactory(organizations=[self.org]) - self.admin = UserProfileFactory(organizations=[self.org]) - self.org.set_member_role(self.admin, ROLE_ADMIN) - - def request(self, method, url, data, user): - return getattr(self.client, method)(url, data, HTTP_AUTHORIZATION='Token ' + user.get_token(), format='json') - - @staticmethod - def get_state(): - return Organization.objects.filter(mnemonic='stark').values( - 'name', 'extras', 'overview', 'logo_path').first() - - def test_member_is_refused(self): - before = self.get_state() - for method, url, data, _ in self.ACTIONS: - with self.subTest(method=method, url=url): - self.assertEqual(self.request(method, url, data, self.member).status_code, 403) - self.assertEqual(self.get_state(), before) - self.assertEqual(self.org.client_configs.count(), 0) - - @patch('core.services.storages.cloud.aws.S3.upload_base64', Mock(return_value='orgs/stark/logo.png')) - def test_admin_is_allowed(self): - for method, url, data, expected_status in self.ACTIONS: - with self.subTest(method=method, url=url): - self.assertEqual(self.request(method, url, data, self.admin).status_code, expected_status) - self.assertFalse(Organization.objects.filter(mnemonic='stark').exists()) - - @patch('core.services.storages.cloud.aws.S3.upload_base64', Mock(return_value='orgs/stark/logo.png')) - def test_staff_who_is_not_a_member_is_allowed(self): - staff = UserProfileFactory(is_staff=True) - for method, url, data, expected_status in self.ACTIONS: - with self.subTest(method=method, url=url): - self.assertEqual(self.request(method, url, data, staff).status_code, expected_status) - self.assertFalse(Organization.objects.filter(mnemonic='stark').exists()) - - class OrganizationMemberRepositoryAccessTest(OCLAPITestCase): """A plain member keeps everything they have on the org's repositories, private ones included.""" def setUp(self): @@ -744,22 +688,6 @@ def test_edits_a_private_repository(self): repo.refresh_from_db() self.assertEqual(repo.name, name) - def test_is_refused_editing_a_repository_outside_the_org(self): - stranger = {'HTTP_AUTHORIZATION': 'Token ' + UserProfileFactory().get_token()} - - response = self.client.put( - '/orgs/stark/sources/private-source/', {'name': 'Renamed'}, format='json', **stranger) - - self.assertEqual(response.status_code, 403) - self.source.refresh_from_db() - self.assertNotEqual(self.source.name, 'Renamed') - - @patch('core.common.models.delete_s3_objects', Mock()) - def test_deletes_a_repository(self): - response = self.client.delete('/orgs/stark/sources/private-source/?inline=true', **self.auth) - - self.assertEqual(response.status_code, 204) - def test_manages_exports_of_a_repository(self): self.assertTrue(self.member.is_admin_for(self.source)) self.assertFalse(UserProfileFactory().is_admin_for(self.source)) @@ -843,12 +771,6 @@ def test_put_403_for_a_member_promoting_themselves(self): self.assertEqual(self.put(self.member, {'role': ROLE_ADMIN}, actor=self.member).status_code, 403) self.assertEqual(self.get_role(self.member), ROLE_MEMBER) - def test_put_403_for_a_stranger(self): - stranger = UserProfileFactory() - - self.assertEqual(self.put(stranger, {'role': ROLE_ADMIN}, actor=stranger).status_code, 403) - self.assertFalse(self.org.is_member(stranger)) - def test_put_by_staff_who_is_not_a_member(self): staff = UserProfileFactory(is_staff=True) @@ -857,17 +779,14 @@ def test_put_by_staff_who_is_not_a_member(self): self.assertFalse(self.org.is_member(staff)) def test_put_409_demoting_the_last_admin(self): - response = self.put(self.admin, {'role': ROLE_MEMBER}) - - self.assertEqual(response.status_code, 409) - self.assertEqual(response.data, {'detail': 'Cannot remove or demote the last admin of an organization.'}) - self.assertEqual(self.get_role(self.admin), ROLE_ADMIN) - - def test_put_409_demoting_the_last_admin_by_staff(self): - staff = UserProfileFactory(is_staff=True) + for actor in (self.admin, UserProfileFactory(is_staff=True)): + with self.subTest(actor=actor.username): + response = self.put(self.admin, {'role': ROLE_MEMBER}, actor=actor) - self.assertEqual(self.put(self.admin, {'role': ROLE_MEMBER}, actor=staff).status_code, 409) - self.assertEqual(self.get_role(self.admin), ROLE_ADMIN) + self.assertEqual(response.status_code, 409) + self.assertEqual( + response.data, {'detail': 'Cannot remove or demote the last admin of an organization.'}) + self.assertEqual(self.get_role(self.admin), ROLE_ADMIN) def test_put_409_demoting_the_last_active_admin(self): deactivated = UserProfileFactory(organizations=[self.org], is_active=False) @@ -875,16 +794,6 @@ def test_put_409_demoting_the_last_active_admin(self): self.assertEqual(self.put(self.admin, {'role': ROLE_MEMBER}).status_code, 409) - def test_put_demotes_an_admin_when_another_remains(self): - self.org.set_member_role(self.member, ROLE_ADMIN) - - self.assertEqual(self.put(self.admin, {'role': ROLE_MEMBER}).status_code, 204) - self.assertEqual(self.get_role(self.admin), ROLE_MEMBER) - - def test_delete_by_an_admin(self): - self.assertEqual(self.delete(self.member).status_code, 204) - self.assertFalse(self.org.is_member(self.member)) - def test_delete_403_for_a_member_removing_someone_else(self): other = UserProfileFactory(organizations=[self.org]) @@ -893,16 +802,6 @@ def test_delete_403_for_a_member_removing_someone_else(self): self.assertTrue(self.org.is_member(other)) self.assertTrue(self.org.is_member(self.admin)) - def test_delete_lets_a_member_leave(self): - self.assertEqual(self.delete(self.member, actor=self.member).status_code, 204) - self.assertFalse(self.org.is_member(self.member)) - - def test_delete_lets_an_admin_leave_when_another_remains(self): - self.org.set_member_role(self.member, ROLE_ADMIN) - - self.assertEqual(self.delete(self.admin, actor=self.admin).status_code, 204) - self.assertFalse(self.org.is_member(self.admin)) - def test_delete_409_for_the_last_admin(self): for actor in (self.admin, UserProfileFactory(is_staff=True)): with self.subTest(actor=actor.username): @@ -913,13 +812,6 @@ def test_delete_409_for_the_last_admin(self): response.data, {'detail': 'Cannot remove or demote the last admin of an organization.'}) self.assertTrue(self.org.is_member(self.admin)) - def test_delete_409_when_the_only_other_admin_is_deactivated(self): - deactivated = UserProfileFactory(organizations=[self.org], is_active=False) - self.org.set_member_role(deactivated, ROLE_ADMIN) - - self.assertEqual(self.delete(self.admin, actor=self.admin).status_code, 409) - self.assertTrue(self.org.is_member(self.admin)) - def test_delete_removes_a_deactivated_admin(self): deactivated = UserProfileFactory(organizations=[self.org], is_active=False) self.org.set_member_role(deactivated, ROLE_ADMIN) @@ -927,17 +819,6 @@ def test_delete_removes_a_deactivated_admin(self): self.assertEqual(self.delete(deactivated).status_code, 204) self.assertFalse(self.org.is_member(deactivated)) - def test_a_demoted_admin_loses_the_rights_at_once(self): - self.org.set_member_role(self.member, ROLE_ADMIN) - self.org.set_member_role(self.member, ROLE_MEMBER) - - response = self.client.put( - '/orgs/stark/', {'name': 'Wayne'}, format='json', - HTTP_AUTHORIZATION='Token ' + self.member.get_token()) - - self.assertEqual(response.status_code, 403) - - class OrganizationMemberRoleReadTest(OCLAPITestCase): def setUp(self): super().setUp() @@ -956,12 +837,6 @@ def test_members_list_has_each_role_in_every_shape(self): {'the-admin': 'admin', 'the-member': 'member'} ) - def test_users_alias_of_the_members_list_has_each_role(self): - response = self.client.get('/orgs/stark/users/') - - self.assertEqual({row['username']: row['role'] for row in response.data}, - {'the-admin': 'admin', 'the-member': 'member'}) - def test_role_is_from_this_org_not_another(self): other = OrganizationFactory(mnemonic='other', created_by=self.member, updated_by=self.member) other.members.add(self.admin) @@ -971,19 +846,6 @@ def test_role_is_from_this_org_not_another(self): self.assertEqual({row['username']: row['role'] for row in response.data}, {'the-admin': 'member', 'the-member': 'admin'}) - def test_role_query_does_not_grow_with_the_members(self): - def count_role_queries(): - with CaptureQueriesContext(connection) as context: - self.assertEqual(self.client.get('/orgs/stark/members/').status_code, 200) - return len([query for query in context.captured_queries if 'org_roles' in query['sql']]) - - self.assertEqual(count_role_queries(), 1) - - UserProfileFactory(organizations=[self.org]) - UserProfileFactory(organizations=[self.org]) - - self.assertEqual(count_role_queries(), 1) - def test_user_lists_outside_an_org_have_no_role(self): response = self.client.get( '/users/', HTTP_AUTHORIZATION='Token ' + self.admin.get_token()) @@ -1023,66 +885,6 @@ def test_creating_an_org_makes_the_creator_its_admin(self): self.assertTrue(Organization.objects.get(mnemonic='new-org').is_admin(user)) -@tag('es') -class OrganizationMemberRoleSearchTest(OCLAPITestCase): - def test_search_results_have_each_role(self): - admin = UserProfileFactory(username='searchable-admin') - org = OrganizationFactory(mnemonic='stark', created_by=admin, updated_by=admin) - member = UserProfileFactory(username='searchable-member', organizations=[org]) - admin.save() - member.save() - - response = self.client.get('/orgs/stark/members/?q=searchable') - - self.assertEqual(response.status_code, 200) - self.assertEqual({row['username']: row['role'] for row in response.data}, - {'searchable-admin': 'admin', 'searchable-member': 'member'}) - - -class OrganizationAdminConcurrencyTest(OCLAPITransactionTestCase): - """Runs two demotions at once, on separate connections, to check that every org keeps an admin.""" - - def test_two_admins_demoting_themselves_at_once_leave_one_admin(self): - alice = UserProfileFactory(username='alice') - bob = UserProfileFactory(username='bob') - org = OrganizationFactory(mnemonic='race-org', created_by=alice, updated_by=alice) - org.set_member_role(bob, ROLE_ADMIN) - tokens = {'alice': alice.get_token(), 'bob': bob.get_token()} - is_last_admin = Organization.is_last_admin - - def slow_is_last_admin(organization, user_profile): - result = is_last_admin(organization, user_profile) - time.sleep(0.5) # widens the gap between the check and the change - return result - - barrier = threading.Barrier(2) - statuses = {} - - def demote_self(actor): - try: - client = APIClient() - barrier.wait() - response = client.put( - f'/orgs/race-org/members/{actor}/', {'role': ROLE_MEMBER}, format='json', - HTTP_AUTHORIZATION='Token ' + tokens[actor]) - statuses[actor] = response.status_code - finally: - connection.close() - - with patch.object(Organization, 'is_last_admin', slow_is_last_admin): - threads = [ - threading.Thread(target=demote_self, args=('alice', )), - threading.Thread(target=demote_self, args=('bob', )), - ] - for thread in threads: - thread.start() - for thread in threads: - thread.join() - - self.assertEqual(sorted(statuses.values()), [204, 409]) - self.assertEqual(org.memberships.filter(role__name=ROLE_ADMIN).count(), 1) - - class OrganizationDemotedAdminConcurrencyTest(OCLAPITransactionTestCase): """A request that was waiting for the org's lock uses the rights its sender has once it gets the lock.""" diff --git a/core/orgs/migrations/0023_role_organizationmember.py b/core/orgs/migrations/0023_role_organizationmember.py index fa1613b2d..f3ca71cfa 100644 --- a/core/orgs/migrations/0023_role_organizationmember.py +++ b/core/orgs/migrations/0023_role_organizationmember.py @@ -41,8 +41,7 @@ class Migration(migrations.Migration): on_delete=django.db.models.deletion.CASCADE, related_name='memberships', to='orgs.organization')), ('userprofile', models.ForeignKey( - on_delete=django.db.models.deletion.CASCADE, related_name='org_memberships', - to=settings.AUTH_USER_MODEL)), + on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), ], options={ 'db_table': 'user_profiles_organizations', @@ -56,7 +55,6 @@ class Migration(migrations.Migration): model_name='organizationmember', name='role', field=models.ForeignKey( - db_default=1, default=2, on_delete=django.db.models.deletion.PROTECT, related_name='memberships', - to='orgs.role'), + db_default=1, default=2, on_delete=django.db.models.deletion.PROTECT, to='orgs.role'), ), ] diff --git a/core/orgs/models.py b/core/orgs/models.py index 026c037dd..1c99df8d4 100644 --- a/core/orgs/models.py +++ b/core/orgs/models.py @@ -27,12 +27,11 @@ class Meta: db_table = 'user_profiles_organizations' unique_together = ('userprofile', 'organization') - userprofile = models.ForeignKey('users.UserProfile', on_delete=models.CASCADE, related_name='org_memberships') + userprofile = models.ForeignKey('users.UserProfile', on_delete=models.CASCADE) organization = models.ForeignKey('orgs.Organization', on_delete=models.CASCADE, related_name='memberships') # A new member is a plain member. Only a row inserted without a role gets the column's default: code older than # this column, as during a rollout or after a rollback, where any member could do everything an admin can. - role = models.ForeignKey( - Role, on_delete=models.PROTECT, related_name='memberships', default=ROLE_MEMBER_ID, db_default=ROLE_ADMIN_ID) + role = models.ForeignKey(Role, on_delete=models.PROTECT, default=ROLE_MEMBER_ID, db_default=ROLE_ADMIN_ID) class Organization(BaseResourceModel, SourceContainerMixin, ChecksumModel): @@ -94,8 +93,7 @@ def has_edit_access(self, user): return user.is_staff or self.is_member(user) def is_admin(self, user_profile): - return bool(user_profile) and self.memberships.filter( - userprofile_id=user_profile.id, role_id=ROLE_ADMIN_ID).exists() + return self.memberships.filter(userprofile_id=user_profile.id, role_id=ROLE_ADMIN_ID).exists() def has_admin_access(self, user): """Whether `user` may edit or delete the org and manage its members: staff and its admins.""" diff --git a/core/orgs/tests/tests.py b/core/orgs/tests/tests.py index 5a574588c..66a893a05 100644 --- a/core/orgs/tests/tests.py +++ b/core/orgs/tests/tests.py @@ -4,7 +4,6 @@ from django.contrib.auth.models import AnonymousUser from django.core.exceptions import ValidationError from django.db import connection -from django.db.models import ProtectedError from mock import patch, Mock from core.collections.models import Collection, CollectionReference, Expansion @@ -15,9 +14,9 @@ from core.concepts.tests.factories import ConceptFactory from core.mappings.models import Mapping from core.mappings.tests.factories import MappingFactory -from core.orgs.constants import ORG_OBJECT_TYPE, ROLE_ADMIN, ROLE_MEMBER +from core.orgs.constants import ORG_OBJECT_TYPE, ROLE_ADMIN from core.orgs.documents import OrganizationDocument -from core.orgs.models import Organization, Role +from core.orgs.models import Organization from core.orgs.tests.factories import OrganizationFactory from core.sources.models import Source from core.sources.tests.factories import OrganizationSourceFactory @@ -54,31 +53,6 @@ def test_has_edit_access(self): for user in (member, outsider): self.assertEqual(source.has_parent_edit_access(user), org.has_edit_access(user)) - def test_creator_is_an_admin(self): - creator = UserProfileFactory() - org = OrganizationFactory(created_by=creator, updated_by=creator) - - self.assertTrue(org.is_member(creator)) - self.assertTrue(org.is_admin(creator)) - - def test_saving_again_does_not_change_the_role_of_the_creator(self): - creator = UserProfileFactory() - org = OrganizationFactory(created_by=creator, updated_by=creator) - org.set_member_role(creator, ROLE_MEMBER) - - org.save() - - self.assertFalse(org.is_admin(creator)) - - def test_a_member_added_directly_is_not_an_admin(self): - org = OrganizationFactory() - member = UserProfileFactory() - - org.members.add(member) - - self.assertTrue(org.is_member(member)) - self.assertFalse(org.is_admin(member)) - def test_has_admin_access(self): org = OrganizationFactory() member, admin, outsider = UserProfileFactory(), UserProfileFactory(), UserProfileFactory() @@ -92,54 +66,6 @@ def test_has_admin_access(self): self.assertFalse(org.has_admin_access(AnonymousUser())) self.assertFalse(org.has_admin_access(None)) - def test_is_last_admin(self): - creator = UserProfileFactory() - org = OrganizationFactory(created_by=creator, updated_by=creator) - member = UserProfileFactory(organizations=[org]) - - self.assertTrue(org.is_last_admin(creator)) - self.assertFalse(org.is_last_admin(member)) - self.assertFalse(org.is_last_admin(UserProfileFactory())) - self.assertFalse(org.is_last_admin(None)) - - org.set_member_role(member, ROLE_ADMIN) - - self.assertFalse(org.is_last_admin(creator)) - self.assertFalse(org.is_last_admin(member)) - - def test_is_last_admin_ignores_deactivated_admins(self): - creator = UserProfileFactory() - org = OrganizationFactory(created_by=creator, updated_by=creator) - deactivated = UserProfileFactory(is_active=False) - org.set_member_role(deactivated, ROLE_ADMIN) - - self.assertTrue(org.is_last_admin(creator)) - self.assertFalse(org.is_last_admin(deactivated)) - - def test_get_member_role(self): - creator = UserProfileFactory() - org = OrganizationFactory(created_by=creator, updated_by=creator) - member = UserProfileFactory(organizations=[org]) - - self.assertEqual(org.get_member_role(creator), ROLE_ADMIN) - self.assertEqual(org.get_member_role(member), ROLE_MEMBER) - self.assertIsNone(org.get_member_role(UserProfileFactory())) - self.assertIsNone(org.get_member_role(AnonymousUser())) - self.assertIsNone(org.get_member_role(None)) - - def test_set_member_role(self): - org = OrganizationFactory() - user = UserProfileFactory() - - self.assertTrue(org.set_member_role(user)) - self.assertEqual(org.get_member_role(user), ROLE_MEMBER) - - self.assertFalse(org.set_member_role(user, ROLE_ADMIN)) - self.assertEqual(org.get_member_role(user), ROLE_ADMIN) - - self.assertFalse(org.set_member_role(user)) - self.assertEqual(org.get_member_role(user), ROLE_ADMIN) - def test_an_insert_that_names_no_role_is_an_admin(self): """Code older than the role column inserts this way, during a rollout or after a rollback.""" org = OrganizationFactory() @@ -153,13 +79,6 @@ def test_an_insert_that_names_no_role_is_an_admin(self): self.assertTrue(org.is_admin(user)) - def test_a_membership_in_use_keeps_its_role(self): - org = OrganizationFactory() - org.members.add(UserProfileFactory()) - - with self.assertRaises(ProtectedError): - Role.objects.get(name=ROLE_MEMBER).delete() - def test_members(self): org = Organization(id=123) self.assertEqual(org.members.count(), 0)